| 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
| ||
|
Date: Mon, 27 Aug 2018 20:16:43 +0000
From: "Banman, Andrew" <abanman@....com>
To: "minyard@....org" <minyard@....org>
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
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 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