| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <32b44854-93bd-57cd-2aab-f8cebc1bc0a9@acm.org>
Date: Mon, 27 Aug 2018 15:47:48 -0500
From: Corey Minyard <minyard@....org>
To: "Banman, Andrew" <abanman@....com>
Cc: "openipmi-developer@...ts.sourceforge.net"
<openipmi-developer@...ts.sourceforge.net>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Anderson, Russ" <russ.anderson@....com>,
"Ramsay, Frank" <frank.ramsay@....com>,
"Ernst, Justin" <justin.ernst@....com>,
Corey Minyard <cminyard@...sta.com>
Subject: Re: [PATCH 1/2] ipmi: Move BT capabilities detection to the detect
call
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@...il.com] On Behalf Of
>> minyard@....org
>> Sent: Thursday, August 23, 2018 3:37 PM
>> To: Banman, Andrew <abanman@....com>
>> Cc: openipmi-developer@...ts.sourceforge.net; linux-
>> kernel@...r.kernel.org; Anderson, Russ <russ.anderson@....com>; Ramsay,
>> Frank <frank.ramsay@....com>; Ernst, Justin <justin.ernst@....com>;
>> Corey Minyard <cminyard@...sta.com>
>> Subject: [PATCH 1/2] ipmi: Move BT capabilities detection to the detect
>> call
>>
>> From: Corey Minyard <cminyard@...sta.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@...sta.com>
>> Reported-by: Andrew Banman <abanman@....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
Powered by blists - more mailing lists