[Skiboot] [PATCH] bt: Cleanup bt state machine and fix SMS_ATN bug
Alistair Popple
alistair at popple.id.au
Tue Feb 17 15:07:26 AEDT 2015
Actually don't apply this - Ben pointed out how scary my dubious locking was
so I'm going to do another version based on his suggestions to clean up the
locking some more.
- Alistair
On Tue, 17 Feb 2015 13:34:18 Alistair Popple wrote:
> Currenlty the SMS_ATN bit is processed by the bt interrupt
> handler. This means that on systems without a functional bmc interrupt
> (such as during early bring up) an SMS_ATN will not be noticed,
> including the watchdog pre-timeout which results in the wdt expiring.
>
> This patch moves the check for SMS_ATN into the main bt polling loop,
> ensuring it is processed even without functional bmc interrupts. It
> also cleans up the logic in bt_poll() to make it clearer and reduce
> the number of lpc register reads.
>
> Signed-off-by: Alistair Popple <alistair at popple.id.au>
> ---
> hw/bt.c | 139
> +++++++++++++++++++++++++++++++++++++--------------------------- 1 file
> changed, 80 insertions(+), 59 deletions(-)
>
> diff --git a/hw/bt.c b/hw/bt.c
> index d5c71a8..2f4877d 100644
> --- a/hw/bt.c
> +++ b/hw/bt.c
> @@ -120,7 +120,9 @@ static inline void bt_set_h_busy(bool value)
>
> static inline bool bt_idle(void)
> {
> - return !(bt_inb(BT_CTRL) & (BT_CTRL_B_BUSY | BT_CTRL_H2B_ATN));
> + uint8_t bt_ctrl = bt_inb(BT_CTRL);
> +
> + return !(bt_ctrl & BT_CTRL_B_BUSY) && !(bt_ctrl & BT_CTRL_H2B_ATN);
> }
>
> static inline void bt_set_state(enum bt_states next_state)
> @@ -153,18 +155,17 @@ static void bt_reset_interface(void)
> bt_init_interface();
> }
>
> -static bool bt_try_send_msg(void)
> +/* Try and send a message from the message queue. Caller must hold
> + * bt.bt_lock and bt.msgq_lock and ensue the message queue is not
> + * empty. */
> +static void bt_try_send_msg(void)
> {
> int i;
> struct bt_msg *bt_msg;
> struct ipmi_msg *ipmi_msg;
>
> - lock(&bt.msgq_lock);
> bt_msg = list_top(&bt.msgq, struct bt_msg, link);
> - if (!bt_msg) {
> - unlock(&bt.msgq_lock);
> - return true;
> - }
> + assert(bt_msg);
>
> ipmi_msg = &bt_msg->ipmi_msg;
>
> @@ -172,7 +173,7 @@ static bool bt_try_send_msg(void)
> prerror("BT: Interface in an unexpected state, attempting
reset\n");
> bt_reset_interface();
> unlock(&bt.msgq_lock);
> - return false;
> + return;
> }
>
> /* Send the message */
> @@ -197,9 +198,8 @@ static bool bt_try_send_msg(void)
> bt_msg->tb = mftb();
> bt_outb(BT_CTRL_H2B_ATN, BT_CTRL);
> bt_set_state(BT_STATE_RESP_WAIT);
> - unlock(&bt.msgq_lock);
>
> - return true;
> + return;
> }
>
> static void bt_flush_msg(void)
> @@ -208,7 +208,7 @@ static void bt_flush_msg(void)
> bt_set_h_busy(false);
> }
>
> -static bool bt_get_resp(void)
> +static void bt_get_resp(void)
> {
> int i;
> struct bt_msg *bt_msg;
> @@ -216,14 +216,11 @@ static bool bt_get_resp(void)
> uint8_t resp_len, netfn, seq, cmd;
> uint8_t cc = IPMI_CC_NO_ERROR;
>
> - /* Wait for BMC to signal response */
> + /* Do this here not in the caller because we need to unlock
> + it here before calling ipmi_cmd_done() */
> lock(&bt.msgq_lock);
> - if (!(bt_inb(BT_CTRL) & BT_CTRL_B2H_ATN)) {
> - unlock(&bt.msgq_lock);
> - return true;
> - }
>
> - /* Indicate BMC that we are busy */
> + /* Indicate to the BMC that we are busy */
> bt_set_h_busy(true);
>
> /* Clear B2H_ATN and read pointer */
> @@ -259,7 +256,7 @@ static bool bt_get_resp(void)
> bt_flush_msg();
> bt_set_state(BT_STATE_B_BUSY);
> unlock(&bt.msgq_lock);
> - return false;
> + return;
> }
>
> ipmi_msg = &bt_msg->ipmi_msg;
> @@ -297,7 +294,7 @@ static bool bt_get_resp(void)
> ipmi_cmd_done(cmd, netfn, cc, ipmi_msg);
>
> /* Immediately send the next message */
> - return false;
> + return;
> }
>
> static void bt_expire_old_msg(void)
> @@ -322,53 +319,83 @@ static void bt_expire_old_msg(void)
> unlock(&bt.msgq_lock);
> }
>
> +#if BT_QUEUE_DEBUG
> +static void print_debug_queue_info(void)
> +{
> + struct bt_msg *msg;
> + static bool printed = false;
> +
> + if (!list_empty(&bt.msgq)) {
> + printed = false;
> + prlog(PR_DEBUG, "-------- BT Msg Queue --------\n");
> + list_for_each(&bt.msgq, msg, link) {
> + prlog(PR_DEBUG, "Seq: 0x%02x Cmd: 0x%02x\n", msg->seq,
> msg->ipmi_msg.cmd); + }
> + prlog(PR_DEBUG, "-----------------------------\n");
> + } else if (!printed) {
> + printed = true;
> + prlog(PR_DEBUG, "----- BT Msg Queue Empty -----\n");
> + }
> +}
> +#else
> +static void print_debug_queue_info(void) {}
> +#endif
> +
> static void bt_poll(struct timer *t __unused, void *data __unused)
> {
> - bool ret = true;
> + /* If we can't get the lock assume someone else will notice
> + * the new message and process it. */
> + if (try_lock(&bt.bt_lock)) {
> + while(true) {
> + uint8_t bt_ctrl = bt_inb(BT_CTRL);
>
> - do {
> + print_debug_queue_info();
>
> -#if BT_QUEUE_DEBUG
> - struct bt_msg *msg;
> - static bool printed = false;
> - lock(&bt.msgq_lock);
> - if (!list_empty(&bt.msgq)) {
> - printed = false;
> - prlog(PR_DEBUG, "-------- BT Msg Queue --------\n");
> - list_for_each(&bt.msgq, msg, link) {
> - prlog(PR_DEBUG, "Seq: 0x%02x Cmd: 0x%02x\n",
msg->seq,
> msg->ipmi_msg.cmd); - }
> - prlog(PR_DEBUG, "-----------------------------\n");
> - } else if (!printed) {
> - printed = true;
> - prlog(PR_DEBUG, "----- BT Msg Queue Empty -----\n");
> - }
> - unlock(&bt.msgq_lock);
> -#endif
> + /* Is there a response waiting for us? */
> + if (bt.state == BT_STATE_RESP_WAIT &&
> + (bt_ctrl & BT_CTRL_B2H_ATN))
> + bt_get_resp();
> +
> + if (bt.state == BT_STATE_B_BUSY && bt_idle())
> + bt_set_state(BT_STATE_IDLE);
>
> - ret = true;
> - if (try_lock(&bt.bt_lock)) {
> bt_expire_old_msg();
>
> - switch(bt.state) {
> - case BT_STATE_IDLE:
> - ret = bt_try_send_msg();
> + /* At this point we've made as much forward
> + progress as we can if we're in the middle
> + of sending a message, so if we're not in
> + the idle state we can just leave. */
> + if (bt.state != BT_STATE_IDLE) {
> + unlock(&bt.bt_lock);
> break;
> + }
>
> - case BT_STATE_RESP_WAIT:
> - ret = bt_get_resp();
> - break;
> + /* Check for sms_atn. This may add
> + * things to the message queue. */
> + if (bt_inb(BT_CTRL) & BT_CTRL_SMS_ATN) {
> + bt_outb(BT_CTRL_SMS_ATN, BT_CTRL);
> + ipmi_sms_attention();
> + }
>
> - case BT_STATE_B_BUSY:
> - if (bt_idle()) {
> - bt_set_state(BT_STATE_IDLE);
> - ret = false;
> - }
> + lock(&bt.msgq_lock);
> + if (list_empty(&bt.msgq)) {
> + /* Nothing to do, bail. Ordering is
> + * important here - we need to unlock
> + * the bt_lock first otherwise a
> + * message could be added to the queue
> + * without us noticing.
> + */
> + unlock(&bt.bt_lock);
> + unlock(&bt.msgq_lock);
> break;
> }
> - unlock(&bt.bt_lock);
> +
> + /* There is a message in the queue. Start
> + * sending it. */
> + bt_try_send_msg();
> + unlock(&bt.msgq_lock);
> }
> - } while(!ret);
> + }
>
> schedule_timer(&bt.poller,
> bt.irq_ok ? TIMER_POLL :
msecs_to_tb(BT_DEFAULT_POLL_MS));
> @@ -418,18 +445,12 @@ static int bt_add_ipmi_msg(struct ipmi_msg *ipmi_msg)
> void bt_irq(void)
> {
> uint8_t ireg = bt_inb(BT_INTMASK);
> - uint8_t ctrl = bt_inb(BT_CTRL);
>
> bt.irq_ok = true;
> if (ireg & BT_INTMASK_B2H_IRQ) {
> bt_outb(BT_INTMASK_B2H_IRQ | BT_INTMASK_B2H_IRQEN,
BT_INTMASK);
> bt_poll(NULL, NULL);
> }
> -
> - if (ctrl & BT_CTRL_SMS_ATN) {
> - bt_outb(BT_CTRL_SMS_ATN, BT_CTRL);
> - ipmi_sms_attention();
> - }
> }
>
> /*
More information about the Skiboot
mailing list