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

[Hannes Reinecke]

On 03/01/2016 09:32 PM, ygardi@xxxxxxxxxxxxxx wrote:
>> 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@xxxxxxxxxxxxxx>
>>> Signed-off-by: Yaniv Gardi <ygardi@xxxxxxxxxxxxxx>
>>>
>>> ---
>>>  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 for the explanation.

Reviewed-by: Hannes Reinecke <hare@xxxxxxxx>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@xxxxxxx			      +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)