Re: [PATCH v5 04/15] scsi: ufs: verify hba controller hce reg value

[ygardi@codeaurora.org]

> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
>> Sometimes due to hw issues it takes some time to the
>> host controller register to update. In order to verify the register
>> has updated, a polling is done until its value is set.
>>
>> In addition the functions ufshcd_hba_stop() and
>> ufshcd_wait_for_register() was updated with an additional input
>> parameter, indicating the timeout between reads will
>> be done by sleeping or spinning the cpu.
>>
>> Signed-off-by: Raviv Shvili <rshvili@codeaurora.org>
>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 53
>> ++++++++++++++++++++++++++++-------------------
>>  drivers/scsi/ufs/ufshcd.h | 12 +++--------
>>  2 files changed, 35 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 3400ceb..80031e6 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -240,11 +240,13 @@ static inline void ufshcd_disable_irq(struct
>> ufs_hba *hba)
>>   * @val - wait condition
>>   * @interval_us - polling interval in microsecs
>>   * @timeout_ms - timeout in millisecs
>> + * @can_sleep - perform sleep or just spin
>>   *
>>   * Returns -ETIMEDOUT on error, zero on success
>>   */
>> -static int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32
>> mask,
>> -		u32 val, unsigned long interval_us, unsigned long timeout_ms)
>> +int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
>> +				u32 val, unsigned long interval_us,
>> +				unsigned long timeout_ms, bool can_sleep)
>>  {
>>  	int err = 0;
>>  	unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms);
>> @@ -253,9 +255,10 @@ static int ufshcd_wait_for_register(struct ufs_hba
>> *hba, u32 reg, u32 mask,
>>  	val = val & mask;
>>
>>  	while ((ufshcd_readl(hba, reg) & mask) != val) {
>> -		/* wakeup within 50us of expiry */
>> -		usleep_range(interval_us, interval_us + 50);
>> -
>> +		if (can_sleep)
>> +			usleep_range(interval_us, interval_us + 50);
>> +		else
>> +			udelay(interval_us);
>>  		if (time_after(jiffies, timeout)) {
>>  			if ((ufshcd_readl(hba, reg) & mask) != val)
>>  				err = -ETIMEDOUT;
>> @@ -1459,7 +1462,7 @@ ufshcd_clear_cmd(struct ufs_hba *hba, int tag)
>>  	 */
>>  	err = ufshcd_wait_for_register(hba,
>>  			REG_UTP_TRANSFER_REQ_DOOR_BELL,
>> -			mask, ~mask, 1000, 1000);
>> +			mask, ~mask, 1000, 1000, true);
>>
>>  	return err;
>>  }
>> @@ -2815,6 +2818,23 @@ out:
>>  }
>>
>>  /**
>> + * ufshcd_hba_stop - Send controller to reset state
>> + * @hba: per adapter instance
>> + * @can_sleep: perform sleep or just spin
>> + */
>> +static inline void ufshcd_hba_stop(struct ufs_hba *hba, bool can_sleep)
>> +{
>> +	int err;
>> +
>> +	ufshcd_writel(hba, CONTROLLER_DISABLE,  REG_CONTROLLER_ENABLE);
>> +	err = ufshcd_wait_for_register(hba, REG_CONTROLLER_ENABLE,
>> +					CONTROLLER_ENABLE, CONTROLLER_DISABLE,
>> +					10, 1, can_sleep);
>> +	if (err)
>> +		dev_err(hba->dev, "%s: Controller disable failed\n", __func__);
>> +}
>> +
> Shouldn't you return an error here?
> If the controller disable failed you probably need a hard reset or
> something, otherwise I would assume that every other command from that
> point on will not work as expected.
>
> Cheers,
>
> Hannes


Hello Hannes,
The original routine signature is:
void ufshcd_hba_stop(struct ufs_hba *hba);

as you can see, no return value, the reason is simple - there is nothing
we can do if writing to the register fails.

all we wanted to do here, was to add a graceful time to change the
register value. also, we decided to add error msg in case the value is not
change within this timeout.
We can not do anything else, not to say, return error, as there is no
error handling in such case.

So, as far as i see it, we only improved the already exists logic, by
adding some graceful time to the register change, and also, by adding an
error message that was absent before.

thanks,
Yaniv




> --
> Dr. Hannes Reinecke		      zSeries & Storage
> hare@suse.de			      +49 911 74053 688
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>