[PATCH v1] scsi: ufs: correct ufshcd_shutdown flow

[PATCH v1] scsi: ufs: correct ufshcd_shutdown flow

[peter.wang]

From: Peter Wang <peter.wang@mediatek.com>

After ufshcd_wl_shutdown set device poweroff and link off,
ufshcd_shutdown not turn off regulators/clocks.
Correct the flow to wait ufshcd_wl_shutdown done and turn off
regulators/clocks by polling ufs device/link state 500ms.
Also remove pm_runtime_get_sync because it is unnecessary.

Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/core/ufshcd.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index c7b337480e3e..1c11af48b584 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9461,10 +9461,14 @@ EXPORT_SYMBOL(ufshcd_runtime_resume);
  */
 int ufshcd_shutdown(struct ufs_hba *hba)
 {
-	if (ufshcd_is_ufs_dev_poweroff(hba) && ufshcd_is_link_off(hba))
-		goto out;
+	ktime_t timeout = ktime_add_ms(ktime_get(), 500);
 
-	pm_runtime_get_sync(hba->dev);
+	/* Wait ufshcd_wl_shutdown clear ufs state, timeout 500 ms */
+	while (!ufshcd_is_ufs_dev_poweroff(hba) || !ufshcd_is_link_off(hba)) {
+		if (ktime_after(ktime_get(), timeout))
+			goto out;
+		msleep(1);
+	}
 
 	ufshcd_suspend(hba);
 out:
-- 
2.18.0

Re: [PATCH v1] scsi: ufs: correct ufshcd_shutdown flow

[Bart Van Assche]

On 7/19/22 06:02, peter.wang@mediatek.com wrote:
> From: Peter Wang <peter.wang@mediatek.com>
> 
> After ufshcd_wl_shutdown set device poweroff and link off,
> ufshcd_shutdown not turn off regulators/clocks.
> Correct the flow to wait ufshcd_wl_shutdown done and turn off
> regulators/clocks by polling ufs device/link state 500ms.
> Also remove pm_runtime_get_sync because it is unnecessary.

Please explain in the patch description why the pm_runtime_get_sync() 
call is not necessary.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index c7b337480e3e..1c11af48b584 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -9461,10 +9461,14 @@ EXPORT_SYMBOL(ufshcd_runtime_resume);
>    */
>   int ufshcd_shutdown(struct ufs_hba *hba)
>   {
> -	if (ufshcd_is_ufs_dev_poweroff(hba) && ufshcd_is_link_off(hba))
> -		goto out;
> +	ktime_t timeout = ktime_add_ms(ktime_get(), 500);

Where does the 500 ms timeout come from?

Additionally, given the large timeout, please use jiffies instead of 
ktime_get().

> -	pm_runtime_get_sync(hba->dev);
> +	/* Wait ufshcd_wl_shutdown clear ufs state, timeout 500 ms */
> +	while (!ufshcd_is_ufs_dev_poweroff(hba) || !ufshcd_is_link_off(hba)) {
> +		if (ktime_after(ktime_get(), timeout))
> +			goto out;
> +		msleep(1);
> +	}

Please explain why this wait loop has been introduced.

Thanks,

Bart.

Re: [PATCH v1] scsi: ufs: correct ufshcd_shutdown flow

[Peter Wang]

Hi Bart

On 7/21/22 5:40 AM, Bart Van Assche wrote:
> On 7/19/22 06:02, peter.wang@mediatek.com wrote:
>> From: Peter Wang <peter.wang@mediatek.com>
>>
>> After ufshcd_wl_shutdown set device poweroff and link off,
>> ufshcd_shutdown not turn off regulators/clocks.
>> Correct the flow to wait ufshcd_wl_shutdown done and turn off
>> regulators/clocks by polling ufs device/link state 500ms.
>> Also remove pm_runtime_get_sync because it is unnecessary.
>
> Please explain in the patch description why the pm_runtime_get_sync() 
> call is not necessary.

Because shutdown is focus on turn off clock/power, we don't need turn 
on(resume) and turn off, right?

>
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index c7b337480e3e..1c11af48b584 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -9461,10 +9461,14 @@ EXPORT_SYMBOL(ufshcd_runtime_resume);
>>    */
>>   int ufshcd_shutdown(struct ufs_hba *hba)
>>   {
>> -    if (ufshcd_is_ufs_dev_poweroff(hba) && ufshcd_is_link_off(hba))
>> -        goto out;
>> +    ktime_t timeout = ktime_add_ms(ktime_get(), 500);
>
> Where does the 500 ms timeout come from?

It is a time to wait device into power off, if the 500 ms is not 
suitable, could you suggess a value?

>
> Additionally, given the large timeout, please use jiffies instead of 
> ktime_get().

Okay, will change next version.

>
>> -    pm_runtime_get_sync(hba->dev);
>> +    /* Wait ufshcd_wl_shutdown clear ufs state, timeout 500 ms */
>> +    while (!ufshcd_is_ufs_dev_poweroff(hba) || 
>> !ufshcd_is_link_off(hba)) {
>> +        if (ktime_after(ktime_get(), timeout))
>> +            goto out;
>> +        msleep(1);
>> +    }
>
> Please explain why this wait loop has been introduced.

Both ufshcd_shtdown and ufshcd_wl_shutdown could run concurrently.

if ufshcd_wl_shutdown -> ufshcd_shtdown, clock/power off should ok.

If ufshcd_shtdown -> ufshcd_wl_shutdown, wait ufshcd_wl_shutdown set 
device to power off and turn off clock/power.

If timeout happen, means device still in active mode, cannot turn off 
clock/power directly. Skip and keep clock/power on in this case.


>
> Thanks,
>
> Bart.

Re: [PATCH v1] scsi: ufs: correct ufshcd_shutdown flow

[Chaotian Jing]

On Thu, 2022-07-21 at 12:30 +0800, Peter Wang wrote:
> Hi Bart
> 
> On 7/21/22 5:40 AM, Bart Van Assche wrote:
> > On 7/19/22 06:02, peter.wang@mediatek.com wrote:
> > > From: Peter Wang <peter.wang@mediatek.com>
> > > 
> > > After ufshcd_wl_shutdown set device poweroff and link off,
> > > ufshcd_shutdown not turn off regulators/clocks.
> > > Correct the flow to wait ufshcd_wl_shutdown done and turn off
> > > regulators/clocks by polling ufs device/link state 500ms.
> > > Also remove pm_runtime_get_sync because it is unnecessary.
> > 
> > Please explain in the patch description why the
> > pm_runtime_get_sync() 
> > call is not necessary.
> 
> Because shutdown is focus on turn off clock/power, we don't need
> turn 
> on(resume) and turn off, right?
> 
> > 
> > > diff --git a/drivers/ufs/core/ufshcd.c
> > > b/drivers/ufs/core/ufshcd.c
> > > index c7b337480e3e..1c11af48b584 100644
> > > --- a/drivers/ufs/core/ufshcd.c
> > > +++ b/drivers/ufs/core/ufshcd.c
> > > @@ -9461,10 +9461,14 @@ EXPORT_SYMBOL(ufshcd_runtime_resume);
> > >    */
> > >   int ufshcd_shutdown(struct ufs_hba *hba)
> > >   {
> > > -    if (ufshcd_is_ufs_dev_poweroff(hba) &&
> > > ufshcd_is_link_off(hba))
> > > -        goto out;
> > > +    ktime_t timeout = ktime_add_ms(ktime_get(), 500);
> > 
> > Where does the 500 ms timeout come from?
> 
> It is a time to wait device into power off, if the 500 ms is not 
> suitable, could you suggess a value?
> 
> > 
> > Additionally, given the large timeout, please use jiffies instead
> > of 
> > ktime_get().
> 
> Okay, will change next version.
> 
> > 
> > > -    pm_runtime_get_sync(hba->dev);
> > > +    /* Wait ufshcd_wl_shutdown clear ufs state, timeout 500 ms
> > > */
> > > +    while (!ufshcd_is_ufs_dev_poweroff(hba) || 
> > > !ufshcd_is_link_off(hba)) {
> > > +        if (ktime_after(ktime_get(), timeout))
> > > +            goto out;
> > > +        msleep(1);
> > > +    }
> > 
> > Please explain why this wait loop has been introduced.
> 
> Both ufshcd_shtdown and ufshcd_wl_shutdown could run concurrently.

Is it possible to avoid the dev's shutdown and its parent's shutdown
run concurrently ? if cannnot avoid it, then seems the concurrently run
case may happen at any device and its parent device! then how do deal
with it ?

Also, the timeout 500ms may make no sense as the child device may not
get the device lock of its parent(it must wait parent's shutdown()
return then it can get the device lock).
> 
> if ufshcd_wl_shutdown -> ufshcd_shtdown, clock/power off should ok.
> 
> If ufshcd_shtdown -> ufshcd_wl_shutdown, wait ufshcd_wl_shutdown set 
> device to power off and turn off clock/power.
> 
> If timeout happen, means device still in active mode, cannot turn
> off 
> clock/power directly. Skip and keep clock/power on in this case.
> 
> 
> > 
> > Thanks,
> > 
> > Bart.

Re: [PATCH v1] scsi: ufs: correct ufshcd_shutdown flow

[Peter Wang]

Hi Chaotian,

On 7/22/22 9:27 AM, Chaotian Jing wrote:
> On Thu, 2022-07-21 at 12:30 +0800, Peter Wang wrote:
>> Hi Bart
>>
>> On 7/21/22 5:40 AM, Bart Van Assche wrote:
>>> On 7/19/22 06:02, peter.wang@mediatek.com wrote:
>>>> From: Peter Wang <peter.wang@mediatek.com>
>>>>
>>>> After ufshcd_wl_shutdown set device poweroff and link off,
>>>> ufshcd_shutdown not turn off regulators/clocks.
>>>> Correct the flow to wait ufshcd_wl_shutdown done and turn off
>>>> regulators/clocks by polling ufs device/link state 500ms.
>>>> Also remove pm_runtime_get_sync because it is unnecessary.
>>> Please explain in the patch description why the
>>> pm_runtime_get_sync()
>>> call is not necessary.
>> Because shutdown is focus on turn off clock/power, we don't need
>> turn
>> on(resume) and turn off, right?
>>
>>>> diff --git a/drivers/ufs/core/ufshcd.c
>>>> b/drivers/ufs/core/ufshcd.c
>>>> index c7b337480e3e..1c11af48b584 100644
>>>> --- a/drivers/ufs/core/ufshcd.c
>>>> +++ b/drivers/ufs/core/ufshcd.c
>>>> @@ -9461,10 +9461,14 @@ EXPORT_SYMBOL(ufshcd_runtime_resume);
>>>>     */
>>>>    int ufshcd_shutdown(struct ufs_hba *hba)
>>>>    {
>>>> -    if (ufshcd_is_ufs_dev_poweroff(hba) &&
>>>> ufshcd_is_link_off(hba))
>>>> -        goto out;
>>>> +    ktime_t timeout = ktime_add_ms(ktime_get(), 500);
>>> Where does the 500 ms timeout come from?
>> It is a time to wait device into power off, if the 500 ms is not
>> suitable, could you suggess a value?
>>
>>> Additionally, given the large timeout, please use jiffies instead
>>> of
>>> ktime_get().
>> Okay, will change next version.
>>
>>>> -    pm_runtime_get_sync(hba->dev);
>>>> +    /* Wait ufshcd_wl_shutdown clear ufs state, timeout 500 ms
>>>> */
>>>> +    while (!ufshcd_is_ufs_dev_poweroff(hba) ||
>>>> !ufshcd_is_link_off(hba)) {
>>>> +        if (ktime_after(ktime_get(), timeout))
>>>> +            goto out;
>>>> +        msleep(1);
>>>> +    }
>>> Please explain why this wait loop has been introduced.
>> Both ufshcd_shtdown and ufshcd_wl_shutdown could run concurrently.
> Is it possible to avoid the dev's shutdown and its parent's shutdown
> run concurrently ? if cannnot avoid it, then seems the concurrently run
> case may happen at any device and its parent device! then how do deal
> with it ?

Can't avoid in current shutdown design, so device driver should have 
protect in this situation now.

>
> Also, the timeout 500ms may make no sense as the child device may not
> get the device lock of its parent(it must wait parent's shutdown()
> return then it can get the device lock).

Yes, this should improve in shutdown flow. But current is no guarantee, 
right?

In most case, ufshcd_shutdown no need wait because ufshcd_wl_shutdown is 
finish.

For concurrent case, 500ms is a safer value that ufshcd_wl_shutdown may 
take.


>> if ufshcd_wl_shutdown -> ufshcd_shtdown, clock/power off should ok.
>>
>> If ufshcd_shtdown -> ufshcd_wl_shutdown, wait ufshcd_wl_shutdown set
>> device to power off and turn off clock/power.
>>
>> If timeout happen, means device still in active mode, cannot turn
>> off
>> clock/power directly. Skip and keep clock/power on in this case.
>>
>>
>>> Thanks,
>>>
>>> Bart.

Re: [PATCH v1] scsi: ufs: correct ufshcd_shutdown flow

[Bart Van Assche]

On 7/20/22 21:30, Peter Wang wrote:
> On 7/21/22 5:40 AM, Bart Van Assche wrote:
>> On 7/19/22 06:02, peter.wang@mediatek.com wrote:
>>> From: Peter Wang <peter.wang@mediatek.com>
>>> Also remove pm_runtime_get_sync because it is unnecessary.
>>
>> Please explain in the patch description why the pm_runtime_get_sync() 
>> call is not necessary.
> 
> Because shutdown is focus on turn off clock/power, we don't need turn 
> on(resume) and turn off, right?
Hi Peter,

I think that removing the pm_runtime_get_sync() call is safe because the 
device driver core already performs a runtime resume before the UFS 
driver shutdown callback function is called. From drivers/base/core.c:

		/* Don't allow any more runtime suspends */
		pm_runtime_get_noresume(dev);
		pm_runtime_barrier(dev);

Thanks,

Bart.

Re: [PATCH v1] scsi: ufs: correct ufshcd_shutdown flow

[Bart Van Assche]

On 7/20/22 21:30, Peter Wang wrote:
> On 7/21/22 5:40 AM, Bart Van Assche wrote:
>> On 7/19/22 06:02, peter.wang@mediatek.com wrote:
>>> -    pm_runtime_get_sync(hba->dev);
>>> +    /* Wait ufshcd_wl_shutdown clear ufs state, timeout 500 ms */
>>> +    while (!ufshcd_is_ufs_dev_poweroff(hba) || 
>>> !ufshcd_is_link_off(hba)) {
>>> +        if (ktime_after(ktime_get(), timeout))
>>> +            goto out;
>>> +        msleep(1);
>>> +    }
>>
>> Please explain why this wait loop has been introduced.
> 
> Both ufshcd_shutdown and ufshcd_wl_shutdown could run concurrently.

Are you sure of this? In drivers/base/core.c I see a sequential loop in 
the device_shutdown() function. So how could two shutdown functions run 
concurrently?

Thanks,

Bart.

Re: [PATCH v1] scsi: ufs: correct ufshcd_shutdown flow

[Peter Wang]

On 7/23/22 2:04 AM, Bart Van Assche wrote:
> On 7/20/22 21:30, Peter Wang wrote:
>> On 7/21/22 5:40 AM, Bart Van Assche wrote:
>>> On 7/19/22 06:02, peter.wang@mediatek.com wrote:
>>>> From: Peter Wang <peter.wang@mediatek.com>
>>>> Also remove pm_runtime_get_sync because it is unnecessary.
>>>
>>> Please explain in the patch description why the 
>>> pm_runtime_get_sync() call is not necessary.
>>
>> Because shutdown is focus on turn off clock/power, we don't need turn 
>> on(resume) and turn off, right?
> Hi Peter,
>
> I think that removing the pm_runtime_get_sync() call is safe because 
> the device driver core already performs a runtime resume before the 
> UFS driver shutdown callback function is called. From 
> drivers/base/core.c:
>
>         /* Don't allow any more runtime suspends */
>         pm_runtime_get_noresume(dev);
>         pm_runtime_barrier(dev);
>
> Thanks,
>
> Bart.

Hi Bart,

No, in drivers/base/core.c:
pm_runtime_get_noresume(dev); => No guarantee device will resume
pm_runtime_barrier(dev);      => Only flush pending pm request like 
RPM_SUSPENDING/RPM_RESUMING

So, If below two condition is meet.
(1) device is already in RPM_SUSPENDED
(2) device resume is required.
Then driver still need call pm_runtime_get_sync just like 
ufshcd_wl_shutdown.

The reason why here can remove pm_runtime_get_sync is because,
(1) ufshcd_wl_shutdown -> pm_runtime_get_sync, will resume hba->dev too.
(2) device resume(turn on clk/power) is not required, even if device is 
in RPM_SUSPENDED.

Thanks.
Peter