Messages in this thread | | | Subject | Re: [PATCH 1/2] ipmi: Move BT capabilities detection to the detect call | From | Corey Minyard <> | Date | Mon, 27 Aug 2018 15:47:48 -0500 |
| |
On 08/27/2018 03:16 PM, Banman, Andrew wrote: > Tested-by: Andrew Banman > > Ran through 100+ boots with no error with your 1st patch applied. I don't see any endcases to worry about. >
Thanks for the testing, it's queued for the next release.
-corey
> Thanks Corey! > > Andrew > >> -----Original Message----- >> From: Corey Minyard [mailto:tcminyard@gmail.com] On Behalf Of >> minyard@acm.org >> Sent: Thursday, August 23, 2018 3:37 PM >> To: Banman, Andrew <abanman@hpe.com> >> Cc: openipmi-developer@lists.sourceforge.net; linux- >> kernel@vger.kernel.org; Anderson, Russ <russ.anderson@hpe.com>; Ramsay, >> Frank <frank.ramsay@hpe.com>; Ernst, Justin <justin.ernst@hpe.com>; >> Corey Minyard <cminyard@mvista.com> >> Subject: [PATCH 1/2] ipmi: Move BT capabilities detection to the detect >> call >> >> From: Corey Minyard <cminyard@mvista.com> >> >> The capabilities detection was being done as part of the normal >> state machine, but it was possible for it to be running while >> the upper layers of the IPMI driver were initializing the >> device, resulting in error and failure to initialize. >> >> Move the capabilities detection to the the detect function, >> so it's done before anything else runs on the device. This also >> simplifies the state machine and removes some code, as a bonus. >> >> Signed-off-by: Corey Minyard <cminyard@mvista.com> >> Reported-by: Andrew Banman <abanman@hpe.com> >> --- >> drivers/char/ipmi/ipmi_bt_sm.c | 92 ++++++++++++++++++++++------------- >> ------- >> 1 file changed, 48 insertions(+), 44 deletions(-) >> >> diff --git a/drivers/char/ipmi/ipmi_bt_sm.c >> b/drivers/char/ipmi/ipmi_bt_sm.c >> index cbc6126..b4133832e 100644 >> --- a/drivers/char/ipmi/ipmi_bt_sm.c >> +++ b/drivers/char/ipmi/ipmi_bt_sm.c >> @@ -59,8 +59,6 @@ enum bt_states { >> BT_STATE_RESET3, >> BT_STATE_RESTART, >> BT_STATE_PRINTME, >> - BT_STATE_CAPABILITIES_BEGIN, >> - BT_STATE_CAPABILITIES_END, >> BT_STATE_LONG_BUSY /* BT doesn't get hosed :-) */ >> }; >> >> @@ -86,7 +84,6 @@ struct si_sm_data { >> int error_retries; /* end of "common" fields */ >> int nonzero_status; /* hung BMCs stay all 0 */ >> enum bt_states complete; /* to divert the state machine */ >> - int BT_CAP_outreqs; >> long BT_CAP_req2rsp; >> int BT_CAP_retries; /* Recommended retries */ >> }; >> @@ -137,8 +134,6 @@ static char *state2txt(unsigned char state) >> case BT_STATE_RESET3: return("RESET3"); >> case BT_STATE_RESTART: return("RESTART"); >> case BT_STATE_LONG_BUSY: return("LONG_BUSY"); >> - case BT_STATE_CAPABILITIES_BEGIN: return("CAP_BEGIN"); >> - case BT_STATE_CAPABILITIES_END: return("CAP_END"); >> } >> return("BAD STATE"); >> } >> @@ -185,7 +180,6 @@ static unsigned int bt_init_data(struct si_sm_data >> *bt, struct si_sm_io *io) >> bt->complete = BT_STATE_IDLE; /* end here */ >> bt->BT_CAP_req2rsp = BT_NORMAL_TIMEOUT * USEC_PER_SEC; >> bt->BT_CAP_retries = BT_NORMAL_RETRY_LIMIT; >> - /* BT_CAP_outreqs == zero is a flag to read BT Capabilities */ >> return 3; /* We claim 3 bytes of space; ought to check SPMI table >> */ >> } >> >> @@ -447,7 +441,7 @@ static enum si_sm_result error_recovery(struct >> si_sm_data *bt, >> >> static enum si_sm_result bt_event(struct si_sm_data *bt, long time) >> { >> - unsigned char status, BT_CAP[8]; >> + unsigned char status; >> static enum bt_states last_printed = BT_STATE_PRINTME; >> int i; >> >> @@ -500,12 +494,6 @@ static enum si_sm_result bt_event(struct si_sm_data >> *bt, long time) >> if (status & BT_H_BUSY) /* clear a leftover H_BUSY */ >> BT_CONTROL(BT_H_BUSY); >> >> - bt->timeout = bt->BT_CAP_req2rsp; >> - >> - /* Read BT capabilities if it hasn't been done yet */ >> - if (!bt->BT_CAP_outreqs) >> - BT_STATE_CHANGE(BT_STATE_CAPABILITIES_BEGIN, >> - SI_SM_CALL_WITHOUT_DELAY); >> BT_SI_SM_RETURN(SI_SM_IDLE); >> >> case BT_STATE_XACTION_START: >> @@ -610,37 +598,6 @@ static enum si_sm_result bt_event(struct si_sm_data >> *bt, long time) >> BT_STATE_CHANGE(BT_STATE_XACTION_START, >> SI_SM_CALL_WITH_DELAY); >> >> - /* >> - * Get BT Capabilities, using timing of upper level state machine. >> - * Set outreqs to prevent infinite loop on timeout. >> - */ >> - case BT_STATE_CAPABILITIES_BEGIN: >> - bt->BT_CAP_outreqs = 1; >> - { >> - unsigned char GetBT_CAP[] = { 0x18, 0x36 }; >> - bt->state = BT_STATE_IDLE; >> - bt_start_transaction(bt, GetBT_CAP, sizeof(GetBT_CAP)); >> - } >> - bt->complete = BT_STATE_CAPABILITIES_END; >> - BT_STATE_CHANGE(BT_STATE_XACTION_START, >> - SI_SM_CALL_WITH_DELAY); >> - >> - case BT_STATE_CAPABILITIES_END: >> - i = bt_get_result(bt, BT_CAP, sizeof(BT_CAP)); >> - bt_init_data(bt, bt->io); >> - if ((i == 8) && !BT_CAP[2]) { >> - bt->BT_CAP_outreqs = BT_CAP[3]; >> - bt->BT_CAP_req2rsp = BT_CAP[6] * USEC_PER_SEC; >> - bt->BT_CAP_retries = BT_CAP[7]; >> - } else >> - pr_warn("IPMI BT: using default values\n"); >> - if (!bt->BT_CAP_outreqs) >> - bt->BT_CAP_outreqs = 1; >> - pr_warn("IPMI BT: req2rsp=%ld secs retries=%d\n", >> - bt->BT_CAP_req2rsp / USEC_PER_SEC, bt->BT_CAP_retries); >> - bt->timeout = bt->BT_CAP_req2rsp; >> - return SI_SM_CALL_WITHOUT_DELAY; >> - >> default: /* should never occur */ >> return error_recovery(bt, >> status, >> @@ -651,6 +608,11 @@ static enum si_sm_result bt_event(struct si_sm_data >> *bt, long time) >> >> static int bt_detect(struct si_sm_data *bt) >> { >> + unsigned char GetBT_CAP[] = { 0x18, 0x36 }; >> + unsigned char BT_CAP[8]; >> + enum si_sm_result smi_result; >> + int rv; >> + >> /* >> * It's impossible for the BT status and interrupt registers to be >> * all 1's, (assuming a properly functioning, self-initialized BMC) >> @@ -661,6 +623,48 @@ static int bt_detect(struct si_sm_data *bt) >> if ((BT_STATUS == 0xFF) && (BT_INTMASK_R == 0xFF)) >> return 1; >> reset_flags(bt); >> + >> + /* >> + * Try getting the BT capabilities here. >> + */ >> + rv = bt_start_transaction(bt, GetBT_CAP, sizeof(GetBT_CAP)); >> + if (rv) { >> + dev_warn(bt->io->dev, >> + "Can't start capabilities transaction: %d\n", rv); >> + goto out_no_bt_cap; >> + } >> + >> + smi_result = SI_SM_CALL_WITHOUT_DELAY; >> + for (;;) { >> + if (smi_result == SI_SM_CALL_WITH_DELAY || >> + smi_result == SI_SM_CALL_WITH_TICK_DELAY) { >> + schedule_timeout_uninterruptible(1); >> + smi_result = bt_event(bt, jiffies_to_usecs(1)); >> + } else if (smi_result == SI_SM_CALL_WITHOUT_DELAY) { >> + smi_result = bt_event(bt, 0); >> + } else >> + break; >> + } >> + >> + rv = bt_get_result(bt, BT_CAP, sizeof(BT_CAP)); >> + bt_init_data(bt, bt->io); >> + if (rv < 8) { >> + dev_warn(bt->io->dev, "bt cap response too short: %d\n", rv); >> + goto out_no_bt_cap; >> + } >> + >> + if (BT_CAP[2]) { >> + dev_warn(bt->io->dev, "Error fetching bt cap: %x\n", >> BT_CAP[2]); >> +out_no_bt_cap: >> + dev_warn(bt->io->dev, "using default values\n"); >> + } else { >> + bt->BT_CAP_req2rsp = BT_CAP[6] * USEC_PER_SEC; >> + bt->BT_CAP_retries = BT_CAP[7]; >> + } >> + >> + dev_info(bt->io->dev, "req2rsp=%ld secs retries=%d\n", >> + bt->BT_CAP_req2rsp / USEC_PER_SEC, bt->BT_CAP_retries); >> + >> return 0; >> } >> >> -- >> 2.7.4
| |