scsi: ufs: correct ufshcd_shutdown flow

PATCH Superseded [email protected] 3 participants 8 messages Jul 19, 2022 – Jul 25, 2022

Subsystems (3)

SCSI Arm MediaTek UFS host controller

lore

Share & tools

lore

[email protected]

author

2022-07-19 13:22 UTC #0 lore raw

PATCH 1/1 scsi: ufs: correct ufshcd_shutdown flow

From: Peter Wang <[email protected]>

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 <[email protected]>

---
 drivers/ufs/core/ufshcd.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

drivers/ufs/core/ufshcd.c +7 -3

index c7b337480e3e..1c11af48b584 100644
@@ -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:

Signature · 1 line

2.18.0

Bart Van Assche

reviewer

2022-07-20 21:40 UTC #1 lore raw

Quoted 7 lines · On 7/19/22 06:02, [email protected] wrote:

From: Peter Wang <[email protected]>
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.

drivers/ufs/core/ufshcd.c EXPORT_SYMBOL(ufshcd_runtime_resume); · line 9464

Quoted 11 lines

drivers/ufs/core/ufshcd.c +1 -2

index c7b337480e3e..1c11af48b584 100644
@@ -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().

drivers/ufs/core/ufshcd.c EXPORT_SYMBOL(ufshcd_runtime_resume); · line 9471

Quoted 7 lines

drivers/ufs/core/ufshcd.c +6 -1

-	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.

Peter Wang

author

2022-07-21 04:30 UTC #2 lore raw

Hi Bart

Quoted 10 lines · Bart Van Assche

Quoted 7 lines · On 7/19/22 06:02, [email protected] wrote:
From: Peter Wang <[email protected]>
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?

Quoted 14 lines

drivers/ufs/core/ufshcd.c EXPORT_SYMBOL(ufshcd_runtime_resume); · line 9464
Quoted 12 lines
drivers/ufs/core/ufshcd.c +1 -2
index c7b337480e3e..1c11af48b584 100644
@@ -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?

Quoted 2 lines

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

Okay, will change next version.

Quoted 11 lines

drivers/ufs/core/ufshcd.c EXPORT_SYMBOL(ufshcd_runtime_resume); · line 9471
Quoted 9 lines
drivers/ufs/core/ufshcd.c +6 -1
-    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.

Quoted 4 lines

Thanks,
Bart.

Chaotian Jing

2022-07-22 01:27 UTC #3 lore raw

Quoted 58 lines · Peter Wang

Hi Bart
Quoted 9 lines · Bart Van Assche
Quoted 7 lines · On 7/19/22 06:02, [email protected] wrote:
From: Peter Wang <[email protected]>
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.
Quoted 3 lines
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?
Quoted 16 lines
Quoted 7 lines
drivers/ufs/core/ufshcd.c +0 -0
index c7b337480e3e..1c11af48b584 100644
drivers/ufs/core/ufshcd.c EXPORT_SYMBOL(ufshcd_runtime_resume); · line 9465
Quoted 7 lines
drivers/ufs/core/ufshcd.c +1 -2
@@ -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?
Quoted 4 lines
Additionally, given the large timeout, please use jiffies instead of
ktime_get().
Okay, will change next version.
Quoted 11 lines
Quoted 10 lines
drivers/ufs/core/ufshcd.c +6 -1
-    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);
+    }

Quoted 3 lines

Quoted 1 line
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).

Quoted 15 lines

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.
Quoted 4 lines
Thanks,
Bart.

Peter Wang

author

2022-07-22 02:32 UTC #4 lore raw

Hi Chaotian,

Quoted 50 lines · Chaotian Jing

Quoted 49 lines · Peter Wang
Hi Bart
Quoted 11 lines · Bart Van Assche
Quoted 7 lines · On 7/19/22 06:02, [email protected] wrote:
From: Peter Wang <[email protected]>
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?
Quoted 6 lines
Quoted 6 lines
drivers/ufs/core/ufshcd.c +0 -0
index c7b337480e3e..1c11af48b584 100644
@@ -9461,10 +9461,14 @@ EXPORT_SYMBOL(ufshcd_runtime_resume);
Quoted 8 lines
drivers/ufs/core/ufshcd.c EXPORT_SYMBOL(ufshcd_runtime_resume); · line 9464
Quoted 7 lines
drivers/ufs/core/ufshcd.c +1 -2
    */
   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?
Quoted 3 lines
Additionally, given the large timeout, please use jiffies instead of
ktime_get().
Okay, will change next version.
Quoted 9 lines
Quoted 9 lines
drivers/ufs/core/ufshcd.c +6 -1
-    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);
+    }

Quoted 6 lines

Quoted 2 lines
Quoted 1 line
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.

Quoted 4 lines

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.

Quoted 13 lines

Quoted 13 lines
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.
Quoted 3 lines
Thanks,
Bart.

Bart Van Assche

reviewer

2022-07-22 18:04 UTC #5 lore raw

In reply to Peter Wang

Quoted 8 lines · Peter Wang

Quoted 5 lines · Bart Van Assche
Quoted 2 lines · On 7/19/22 06:02, [email protected] wrote:
From: Peter Wang <[email protected]>
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.

Peter Wang

author

2022-07-25 03:46 UTC #6 lore raw

Quoted 13 lines · Bart Van Assche

Quoted 8 lines · Peter Wang
Quoted 5 lines · Bart Van Assche
Quoted 2 lines · On 7/19/22 06:02, [email protected] wrote:
From: Peter Wang <[email protected]>
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:

Quoted 7 lines

/* 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

Bart Van Assche

reviewer

2022-07-22 21:07 UTC #7 lore raw

In reply to Peter Wang

Quoted 11 lines · Peter Wang

Quoted 10 lines · Bart Van Assche
Quoted 8 lines · On 7/19/22 06:02, [email protected] 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);
+    }

Quoted 3 lines

Quoted 1 line
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.