Skip to contentSkip to conversation

scsi: ufs: core: Keep clock-gating hold on failed W-LUN runtime resume

PATCH Applied Stanley Jhu 4 participants 5 messages Oct 29 – Nov 9

Subsystems (3)
Stable branch SCSI UFS host controller
lore
Share & tools
lore

Stanley Jhu

author
#0 lore raw

PATCH 1/1 scsi: ufs: core: Keep clock-gating hold on failed W-LUN runtime resume

1. __ufshcd_wl_suspend() takes one ufshcd_hold() reference and sets hba->clk_gating.is_suspended while the UFS device W-LUN enters RPM_SUSPENDED.
2. Since commit eef1d74bf88f ("PM: runtime: Only set runtime_error on suspend callback failures"), a failed runtime resume (for example START STOP UNIT completing with an error) leaves the W-LUN in RPM_SUSPENDED with power.runtime_error == 0, while __ufshcd_wl_resume() already clears hba->clk_gating.is_suspended and calls ufshcd_release().
3. The next W-LUN runtime resume runs __ufshcd_wl_resume() again without an intervening suspend and calls ufshcd_release() a second time, dropping a reference it does not own and underflowing hba->clk_gating.active_reqs on UFSHCD_CAP_CLK_GATING hosts.

Keep hba->clk_gating.is_suspended and the suspend hold across a failed W-LUN runtime resume, so the hold stays paired with the W-LUN RPM_SUSPENDED state and the next successful resume releases it.

Fixes eef1d74bf88f ("PM: runtime: Only set runtime_error on suspend callback failures")
Cc [email protected] # v7.3+
Signed-off-by Stanley Jhu <[email protected]>
---
 drivers/ufs/core/ufshcd.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/ufs/core/ufshcd.c +8 -2
index cc75384c6502..9be370cea52a 100644
@@ -10626,8 +10626,14 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 out:
 	if (ret)
 		ufshcd_update_evt_hist(hba, UFS_EVT_WL_RES_ERR, (u32)ret);
-	hba->clk_gating.is_suspended = false;
-	ufshcd_release(hba);
+	/*
+	 * A failed runtime resume leaves the W-LUN RPM_SUSPENDED, so keep
+	 * is_suspended and the suspend hold for the next resume.
+	 */
+	if (!ret || pm_op != UFS_RUNTIME_PM) {
+		hba->clk_gating.is_suspended = false;
+		ufshcd_release(hba);
+	}
 	hba->pm_op_in_progress = false;
 	return ret;
 }
Signature · 1 line
2.56.0.313.g2479322e68-goog
Reviewed-by on v1 1/1
#1 lore raw

Hi Stanley,

I'm not extremely familiar with the UFS or block layers here, so I'm just coming from a general driver + runtime PM perspective. That said, I have some thoughts:

Quoted 5 lines · Stanley Jhu

1. __ufshcd_wl_suspend() takes one ufshcd_hold() reference and sets hba->clk_gating.is_suspended while the UFS device W-LUN enters RPM_SUSPENDED.
2. Since commit eef1d74bf88f ("PM: runtime: Only set runtime_error on suspend callback failures"), a failed runtime resume (for example

Quoted 8 lines

START STOP UNIT completing with an error) leaves the W-LUN in RPM_SUSPENDED with power.runtime_error == 0, while __ufshcd_wl_resume() already clears hba->clk_gating.is_suspended and calls ufshcd_release().
3. The next W-LUN runtime resume runs __ufshcd_wl_resume() again without an intervening suspend and calls ufshcd_release() a second time, dropping a reference it does not own and underflowing hba->clk_gating.active_reqs on UFSHCD_CAP_CLK_GATING hosts.

I _think_ this reasoning and patch all looks good, with one possible
exception: ufshcd has explicit recovery in ufshcd_recover_pm_error(), which relies on checking power.runtime_error, and then explicitly overrides the status to active to clear the error.

It seems to me like commit eef1d74bf88f might have subverted some of that logic too, because resume failures will no longer latch a runtime_error value, so that recovery function may not be doing much/anything. Is that a problem too?

Quoted 19 lines

Keep hba->clk_gating.is_suspended and the suspend hold across a failed W-LUN runtime resume, so the hold stays paired with the W-LUN RPM_SUSPENDED state and the next successful resume releases it.

Fixes eef1d74bf88f ("PM: runtime: Only set runtime_error on suspend callback failures")
Cc [email protected] # v7.3+
Signed-off-by Stanley Jhu <[email protected]>
---
 drivers/ufs/core/ufshcd.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/ufs/core/ufshcd.c +0 -0
index cc75384c6502..9be370cea52a 100644
@@ -10626,8 +10626,14 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 out:
 	if (ret)
 		ufshcd_update_evt_hist(hba, UFS_EVT_WL_RES_ERR, (u32)ret);

drivers/ufs/core/ufshcd.c static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) · line 10636

Quoted 10 lines
drivers/ufs/core/ufshcd.c +8 -2
-	hba->clk_gating.is_suspended = false;
-	ufshcd_release(hba);
+	/*
+	 * A failed runtime resume leaves the W-LUN RPM_SUSPENDED, so keep
+	 * is_suspended and the suspend hold for the next resume.
+	 */
+	if (!ret || pm_op != UFS_RUNTIME_PM) {
+		hba->clk_gating.is_suspended = false;
+		ufshcd_release(hba);
+	}

This looks right to me, as a non-expert in this code, and it may even have been wrong before commit eef1d74bf88f.

Reviewed-by Brian Norris <[email protected]>
Quoted 6 lines
drivers/ufs/core/ufshcd.c +0 -0
 	hba->pm_op_in_progress = false;
 	return ret;
 }
Signature · 1 line
2.56.0.313.g2479322e68-goog
Reviewed-by on v1 1/1
#2 lore raw
In reply to Stanley Jhu
Quoted 21 lines · Stanley Jhu

1. __ufshcd_wl_suspend() takes one ufshcd_hold() reference and sets hba->clk_gating.is_suspended while the UFS device W-LUN enters RPM_SUSPENDED.
2. Since commit eef1d74bf88f ("PM: runtime: Only set runtime_error on suspend callback failures"), a failed runtime resume (for example START STOP UNIT completing with an error) leaves the W-LUN in RPM_SUSPENDED with power.runtime_error == 0, while __ufshcd_wl_resume() already clears hba->clk_gating.is_suspended and
   calls ufshcd_release().
3. The next W-LUN runtime resume runs __ufshcd_wl_resume() again without
   an intervening suspend and calls ufshcd_release() a second time, dropping a reference it does not own and underflowing hba->clk_gating.active_reqs on UFSHCD_CAP_CLK_GATING hosts.

Keep hba->clk_gating.is_suspended and the suspend hold across a failed
W-LUN runtime resume, so the hold stays paired with the W-LUN RPM_SUSPENDED state and the next successful resume releases it.

Quoted 5 lines
Fixes eef1d74bf88f ("PM: runtime: Only set runtime_error on suspend

callback failures")

Cc [email protected] # v7.3+
Signed-off-by Stanley Jhu <[email protected]>

---

Reviewed-by Peter Wang <[email protected]>
maintainer Applied
#3 lore raw
In reply to Stanley Jhu

Stanley,

Quoted 5 lines

1. __ufshcd_wl_suspend() takes one ufshcd_hold() reference and sets hba->clk_gating.is_suspended while the UFS device W-LUN enters RPM_SUSPENDED.
2. Since commit eef1d74bf88f ("PM: runtime: Only set runtime_error on suspend callback failures"), a failed runtime resume (for example

Quoted 8 lines

START STOP UNIT completing with an error) leaves the W-LUN in RPM_SUSPENDED with power.runtime_error == 0, while __ufshcd_wl_resume() already clears hba->clk_gating.is_suspended and calls ufshcd_release().
3. The next W-LUN runtime resume runs __ufshcd_wl_resume() again without an intervening suspend and calls ufshcd_release() a second time, dropping a reference it does not own and underflowing hba->clk_gating.active_reqs on UFSHCD_CAP_CLK_GATING hosts.

Applied to 7.4/scsi-fixes, thanks!

Signature · 1 line
Martin K. Petersen
maintainer Applied
#4 lore raw
In reply to Stanley Jhu
Quoted 14 lines · Stanley Jhu

1. __ufshcd_wl_suspend() takes one ufshcd_hold() reference and sets hba->clk_gating.is_suspended while the UFS device W-LUN enters RPM_SUSPENDED.
2. Since commit eef1d74bf88f ("PM: runtime: Only set runtime_error on suspend callback failures"), a failed runtime resume (for example START STOP UNIT completing with an error) leaves the W-LUN in RPM_SUSPENDED with power.runtime_error == 0, while __ufshcd_wl_resume() already clears hba->clk_gating.is_suspended and calls ufshcd_release().
3. The next W-LUN runtime resume runs __ufshcd_wl_resume() again without an intervening suspend and calls ufshcd_release() a second time, dropping a reference it does not own and underflowing hba->clk_gating.active_reqs on UFSHCD_CAP_CLK_GATING hosts.

Quoted 1 line

[...]

Applied to 7.4/scsi-fixes, thanks!

[1/1] scsi: ufs: core: Keep clock-gating hold on failed W-LUN runtime resume

      https://git.kernel.org/mkp/scsi/c/6da1bc55c060
Signature · 1 line
Martin K. Petersen