[PATCH v5 0/2] scsi: ufs: core: Avoid redundant link recovery from the error handler during system PM

[PATCH v5 0/2] scsi: ufs: core: Avoid redundant link recovery from the error handler during system PM

[HOYOUNG SEO]

If a LINERESET is reported and the hibern8 exit fails during system
resume, both ufshcd_err_handler() and __ufshcd_wl_resume() call
ufshcd_link_recovery(), and the two recoveries are not serialized.

Patch 1 makes ufshcd_link_recovery() clear the saved errors like
ufshcd_reset_and_restore() does, so the error handler does not act on
errors that have already been recovered.

Patch 2 skips the early link recovery in ufshcd_err_handler(), which
does not hold host_sem, while a system PM operation is in progress. The
system PM callbacks hold host_sem, so the error handler now waits for
them.

The early link recovery is kept for runtime PM, shutdown and poweroff,
which do not hold host_sem. During runtime resume it can still run
concurrently with __ufshcd_wl_resume(). Removing it there needs a closer
look at the deadlock that it avoids.

Changes in v5:
- Patch 2: track system PM (hba->system_pm_in_progress) instead of
  runtime PM, so that the early link recovery is only skipped where
  host_sem is held. ufshcd_wl_poweroff() holds neither host_sem nor sets
  hba->shutting_down, so poweroff keeps the early link recovery as
  before (Sashiko review).

Changes in v4 (as suggested by Bart Van Assche):
- Replace the is_sys_suspended check of v3 patch 2 with a flag set in
  __ufshcd_wl_suspend() and __ufshcd_wl_resume(), which also covers a
  system suspend failure.
- Add Fixes: tags.
- Do not serialize ufshcd_link_recovery() with a mutex: it calls
  ufshcd_probe_hba(), and a device command timing out inside it ends up
  in ufshcd_eh_timed_out(), which calls ufshcd_link_recovery() again.

Changes in v3:
- Patch 2: reword the commit message only, no code change. The error
  handler does not always run a full recovery after a failed system
  resume; it stops according to ufshcd_err_handling_should_stop() (e.g.
  in UFSHCD_STATE_ERROR). Describe this behavior and refer to the commit
  that added the early link recovery (Sashiko review).
- Patch 1: no change.

Changes in v2:
- Add patch 1. As pointed out by the Sashiko review, saved_err and
  saved_uic_err are still set after the resume path recovered the link,
  so with v1 alone ufshcd_err_handler() would run a redundant recovery
  once host_sem is released.
- Patch 2: describe the race and the system/runtime PM difference in
  the commit message, and add a comment.

v4: https://lore.kernel.org/linux-scsi/cover.1793851432.git.hy50.seo@samsung.com/
v3: https://lore.kernel.org/linux-scsi/cover.1792989286.git.hy50.seo@samsung.com/
v2: https://lore.kernel.org/linux-scsi/cover.1792755091.git.hy50.seo@samsung.com/

HOYOUNG SEO (2):
  scsi: ufs: core: Clear saved errors in ufshcd_link_recovery
  scsi: ufs: core: Do not recover the link early in the error handler
    during system PM

 drivers/ufs/core/ufshcd.c | 25 +++++++++++++++++++++++--
 include/ufs/ufshcd.h      |  2 ++
 2 files changed, 25 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v5 1/2] scsi: ufs: core: Clear saved errors in ufshcd_link_recovery

[HOYOUNG SEO]

ufshcd_link_recovery() resets both the device and the host and then
re-establishes the link. Unlike ufshcd_reset_and_restore(), however, it
does not clear hba->saved_err, hba->saved_uic_err and hba->force_reset,
so the errors latched before the recovery remain pending after the link
has already been recovered.

When ufshcd_err_handler() runs afterwards,
ufshcd_err_handling_should_stop() sees these stale errors. The error
handler then quiesces all queues and, depending on the latched errors
(e.g. a fatal error or UFSHCD_UIC_DL_PA_INIT_ERROR), performs another
full ufshcd_reset_and_restore() on a link that is already operational.

Cache and clear the saved errors before the reset, as
ufshcd_reset_and_restore() does, so that only errors raised during or
after the recovery are left to the error handler. Put them back if the
recovery fails.

Signed-off-by: HOYOUNG SEO <hy50.seo@samsung.com>
---
 drivers/ufs/core/ufshcd.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index fd52deb72d21..3b5580ac390c 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4695,10 +4695,20 @@ EXPORT_SYMBOL_GPL(ufshcd_uic_change_pwr_mode);
 
 int ufshcd_link_recovery(struct ufs_hba *hba)
 {
+	u32 saved_err, saved_uic_err;
 	int ret;
 	unsigned long flags;
 
 	spin_lock_irqsave(&hba->host->host_lock, flags);
+	/*
+	 * The errors saved so far are handled by this full reset, so cache and
+	 * clear them first, in case new errors are generated during recovery.
+	 */
+	saved_err = hba->saved_err;
+	saved_uic_err = hba->saved_uic_err;
+	hba->saved_err = 0;
+	hba->saved_uic_err = 0;
+	hba->force_reset = false;
 	hba->ufshcd_state = UFSHCD_STATE_RESET;
 	ufshcd_set_eh_in_progress(hba);
 	spin_unlock_irqrestore(&hba->host->host_lock, flags);
@@ -4709,8 +4719,11 @@ int ufshcd_link_recovery(struct ufs_hba *hba)
 	ret = ufshcd_host_reset_and_restore(hba);
 
 	spin_lock_irqsave(&hba->host->host_lock, flags);
-	if (ret)
+	if (ret) {
 		hba->ufshcd_state = UFSHCD_STATE_ERROR;
+		hba->saved_err |= saved_err;
+		hba->saved_uic_err |= saved_uic_err;
+	}
 	ufshcd_clear_eh_in_progress(hba);
 	spin_unlock_irqrestore(&hba->host->host_lock, flags);
 
-- 
2.34.1

[PATCH v5 2/2] scsi: ufs: core: Do not recover the link early in the error handler during system PM

[HOYOUNG SEO]

ufshcd_err_handler() recovers the link without taking hba->host_sem
whenever hba->pm_op_in_progress is set. That early path was added by
commit f966e02ae521 ("scsi: ufs: core: Fix runtime suspend error
deadlock") because the regular error handling would wait for a runtime
PM operation that cannot complete while the error handler is waiting.

pm_op_in_progress is also set during system suspend and system resume,
where the early path is not needed and runs concurrently with the PM
path itself. For example, if a LINERESET is reported and the hibern8
exit fails in __ufshcd_wl_resume() during system resume, both
ufshcd_err_handler() and __ufshcd_wl_resume() call
ufshcd_link_recovery(), and the host and the device may be reset at the
same time.

The system PM callbacks hold host_sem from ufshcd_wl_suspend() until
ufshcd_wl_resume() returns, so the regular error handling can simply
wait for them. Skip the early link recovery while a system PM operation
is in progress. Runtime PM, shutdown and poweroff do not hold host_sem
and keep the early link recovery.

Since pm_op_in_progress does not tell which PM operation is running, add
hba->system_pm_in_progress and set it in __ufshcd_wl_suspend() and
__ufshcd_wl_resume().

Fixes: e23ef4f22db3 ("scsi: ufs: core: Fix error handler host_sem issue")
Signed-off-by: HOYOUNG SEO <hy50.seo@samsung.com>
---
 drivers/ufs/core/ufshcd.c | 10 +++++++++-
 include/ufs/ufshcd.h      |  2 ++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 3b5580ac390c..761ffc8f0a32 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -7091,9 +7091,13 @@ static void ufshcd_err_handler(struct work_struct *work)
 		 * runtime resume. This avoids potential deadlocks that could
 		 * happen if we tried to resume the device while a PM operation
 		 * is already in progress.
+		 *
+		 * Do not do this for system PM. The system PM callbacks hold
+		 * host_sem, so the regular path below runs after them instead
+		 * of resetting the link concurrently.
 		 */
 		ufshcd_rpm_get_noresume(hba);
-		if (hba->pm_op_in_progress) {
+		if (hba->pm_op_in_progress && !hba->system_pm_in_progress) {
 			ufshcd_link_recovery(hba);
 			ufshcd_rpm_put(hba);
 			return;
@@ -10393,6 +10397,7 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	enum uic_link_state req_link_state;
 
 	hba->pm_op_in_progress = true;
+	hba->system_pm_in_progress = (pm_op == UFS_SYSTEM_PM);
 	if (pm_op != UFS_SHUTDOWN_PM) {
 		pm_lvl = pm_op == UFS_RUNTIME_PM ?
 			 hba->rpm_lvl : hba->spm_lvl;
@@ -10565,6 +10570,7 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 		hba->clk_gating.is_suspended = false;
 		ufshcd_release(hba);
 	}
+	hba->system_pm_in_progress = false;
 	hba->pm_op_in_progress = false;
 	return ret;
 }
@@ -10576,6 +10582,7 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	enum uic_link_state old_link_state = hba->uic_link_state;
 
 	hba->pm_op_in_progress = true;
+	hba->system_pm_in_progress = (pm_op == UFS_SYSTEM_PM);
 
 	/*
 	 * Call vendor specific resume callback. As these callbacks may access
@@ -10666,6 +10673,7 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 		ufshcd_update_evt_hist(hba, UFS_EVT_WL_RES_ERR, (u32)ret);
 	hba->clk_gating.is_suspended = false;
 	ufshcd_release(hba);
+	hba->system_pm_in_progress = false;
 	hba->pm_op_in_progress = false;
 	return ret;
 }
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index be62165b785e..0d28853e4107 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -974,6 +974,7 @@ enum ufshcd_mcq_opr {
  * @spm_lvl: desired UFS power management level during system PM.
  * @pm_lvl_min: minimum supported power management level.
  * @pm_op_in_progress: whether or not a PM operation is in progress.
+ * @system_pm_in_progress: whether or not a system PM operation is in progress.
  * @ahit: value of Auto-Hibernate Idle Timer register.
  * @outstanding_tasks: Bits representing outstanding task requests
  * @outstanding_lock: Protects @outstanding_reqs.
@@ -1129,6 +1130,7 @@ struct ufs_hba {
 	enum ufs_pm_level spm_lvl;
 	enum ufs_pm_level pm_lvl_min;
 	int pm_op_in_progress;
+	bool system_pm_in_progress;
 
 	/* Auto-Hibernate Idle Timer register value */
 	u32 ahit;
-- 
2.34.1

Re: [PATCH v5 2/2] scsi: ufs: core: Do not recover the link early in the error handler during system PM

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: ufs: core: race on system_pm_in_progress in ufshcd_err_handler()
--

commit 991f616e0ee805a5ba3aa9d0edc90a319abd06b6
Author: HOYOUNG SEO <hy50.seo@samsung.com>

scsi: ufs: core: Do not recover the link early in the error handler during system PM

This commit adds hba->system_pm_in_progress so that ufshcd_err_handler()
skips its early unlocked ufshcd_link_recovery() path during system suspend
and system resume, waiting on hba->host_sem instead.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 3b5580ac390cb..761ffc8f0a32a 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -7091,9 +7091,13 @@ static void ufshcd_err_handler(struct work_struct *work)
>  		 * runtime resume. This avoids potential deadlocks that could
>  		 * happen if we tried to resume the device while a PM operation
>  		 * is already in progress.
> +		 *
> +		 * Do not do this for system PM. The system PM callbacks hold
> +		 * host_sem, so the regular path below runs after them instead
> +		 * of resetting the link concurrently.
>  		 */
>  		ufshcd_rpm_get_noresume(hba);
> -		if (hba->pm_op_in_progress) {
> +		if (hba->pm_op_in_progress && !hba->system_pm_in_progress) {
>  			ufshcd_link_recovery(hba);
>  			ufshcd_rpm_put(hba);
>  			return;
> @@ -10393,6 +10397,7 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  	enum uic_link_state req_link_state;
>  
>  	hba->pm_op_in_progress = true;
> +	hba->system_pm_in_progress = (pm_op == UFS_SYSTEM_PM);
>  	if (pm_op != UFS_SHUTDOWN_PM) {
>  		pm_lvl = pm_op == UFS_RUNTIME_PM ?
>  			 hba->rpm_lvl : hba->spm_lvl;
> @@ -10565,6 +10570,7 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  		hba->clk_gating.is_suspended = false;
>  		ufshcd_release(hba);
>  	}
> +	hba->system_pm_in_progress = false;
>  	hba->pm_op_in_progress = false;
>  	return ret;
>  }
> @@ -10576,6 +10582,7 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  	enum uic_link_state old_link_state = hba->uic_link_state;
>  
>  	hba->pm_op_in_progress = true;
> +	hba->system_pm_in_progress = (pm_op == UFS_SYSTEM_PM);
>  
>  	/*
>  	 * Call vendor specific resume callback. As these callbacks may access
> @@ -10666,6 +10673,7 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  		ufshcd_update_evt_hist(hba, UFS_EVT_WL_RES_ERR, (u32)ret);
>  	hba->clk_gating.is_suspended = false;
>  	ufshcd_release(hba);
> +	hba->system_pm_in_progress = false;
>  	hba->pm_op_in_progress = false;
>  	return ret;
>  }

[Severity: Medium]
Can ufshcd_err_handler() still observe hba->pm_op_in_progress == true and
hba->system_pm_in_progress == false during system suspend or system resume?

ufshcd_err_handler() reads both flags without holding hba->host_sem or any
other lock shared with __ufshcd_wl_suspend() and __ufshcd_wl_resume().

At the entry of __ufshcd_wl_suspend() and __ufshcd_wl_resume(),
hba->pm_op_in_progress = true is written before hba->system_pm_in_progress is
set, and at exit hba->system_pm_in_progress = false is cleared before
hba->pm_op_in_progress = false, without barriers or READ_ONCE()/WRITE_ONCE().

In particular, when __ufshcd_wl_suspend() encounters an error during
UFS_SYSTEM_PM, it calls ufshcd_force_error_recovery() (which immediately
queues hba->eh_work) and jumps to out:

drivers/ufs/core/ufshcd.c:__ufshcd_wl_suspend() {
	...
	if (ret && pm_op != UFS_SHUTDOWN_PM) {
		ufshcd_force_error_recovery(hba);
		ret = -EBUSY;
	}
	...
out:
	...
	if (ret) {
		ufshcd_update_evt_hist(hba, UFS_EVT_WL_SUSP_ERR, (u32)ret);
		hba->clk_gating.is_suspended = false;
		ufshcd_release(hba);
	}
	hba->system_pm_in_progress = false;
	hba->pm_op_in_progress = false;
	return ret;
}

If ufshcd_err_handler() evaluates the condition in the entry or exit window
(or due to compiler/CPU reordering), it will enter the early
ufshcd_link_recovery() path without hba->host_sem or a clock-gating hold and
return early without running the regular error handling path (such as
ufshcd_err_handling_prepare() and ufshcd_recover_pm_error()).

Would ordering the updates (or encoding the active PM operation in a single
variable accessed with READ_ONCE()/WRITE_ONCE()) avoid this race window?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793865172.git.hy50.seo@samsung.com?part=2