Re: [PATCH 1/2] ipmi: Move BT capabilities detection to the detect call

[Corey Minyard <minyard@acm.org>]

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