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

[PATCH v4 0/2] scsi: ufs: core: Avoid redundant link recovery from the error handler during 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 restricts the early link recovery in ufshcd_err_handler(), which
does not hold host_sem, to runtime PM. For system PM the error handler
now waits for host_sem, and for shutdown it stops.

The early link recovery is kept for runtime suspend and runtime resume.
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 v4 (as suggested by Bart Van Assche):
- Replace the is_sys_suspended check of v3 patch 2 with a check for
  runtime PM, which also covers system suspend failure and shutdown.
- 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.

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: Limit early link recovery in the error handler to
    runtime PM

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

-- 
2.34.1

[PATCH v4 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 v4 2/2] scsi: ufs: core: Limit early link recovery in the error handler to runtime 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, system resume and
shutdown, 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.

Only take the early path while a runtime PM operation is in progress.
For the other PM operations the regular error handling is safe:

  - the system PM callbacks hold host_sem from ufshcd_wl_suspend() until
    ufshcd_wl_resume() returns, so the error handler waits for them;
  - during shutdown, hba->shutting_down makes the error handler stop.

Since pm_op_in_progress does not tell which PM operation is running, add
hba->runtime_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 | 14 +++++++++++---
 include/ufs/ufshcd.h      |  2 ++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 3b5580ac390c..e3908d6af3ae 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -7089,11 +7089,15 @@ static void ufshcd_err_handler(struct work_struct *work)
 		 * Use ufshcd_rpm_get_noresume() here to safely perform link
 		 * recovery even if an error occurs during runtime suspend or
 		 * runtime resume. This avoids potential deadlocks that could
-		 * happen if we tried to resume the device while a PM operation
-		 * is already in progress.
+		 * happen if we tried to resume the device while a runtime PM
+		 * operation is already in progress.
+		 *
+		 * Do not do this for system PM and shutdown. 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->runtime_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->runtime_pm_in_progress = (pm_op == UFS_RUNTIME_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->runtime_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->runtime_pm_in_progress = (pm_op == UFS_RUNTIME_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->runtime_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..8b5bfc83fea2 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.
+ * @runtime_pm_in_progress: whether or not a runtime 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 runtime_pm_in_progress;
 
 	/* Auto-Hibernate Idle Timer register value */
 	u32 ahit;
-- 
2.34.1

Re: [PATCH v4 2/2] scsi: ufs: core: Limit early link recovery in the error handler to runtime PM

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: ufs: core: concurrent error recovery during ufshcd_wl_poweroff()
--

commit 6f60eb14bc24b472557f86d1f4db1435491fade7
Author: HOYOUNG SEO <hy50.seo@samsung.com>

scsi: ufs: core: Limit early link recovery in the error handler to runtime PM

This commit restricts the early link recovery path in ufshcd_err_handler()
to runtime PM operations by introducing hba->runtime_pm_in_progress, so
that system PM and shutdown use the regular error handling path instead of
resetting the link concurrently.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 3b5580ac390cb..e3908d6af3ae4 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -7089,11 +7089,15 @@ static void ufshcd_err_handler(struct work_struct *work)
>  		 * Use ufshcd_rpm_get_noresume() here to safely perform link
>  		 * recovery even if an error occurs during runtime suspend or
>  		 * runtime resume. This avoids potential deadlocks that could
> -		 * happen if we tried to resume the device while a PM operation
> -		 * is already in progress.
> +		 * happen if we tried to resume the device while a runtime PM
> +		 * operation is already in progress.
> +		 *
> +		 * Do not do this for system PM and shutdown. 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->runtime_pm_in_progress) {
>  			ufshcd_link_recovery(hba);
>  			ufshcd_rpm_put(hba);
>  			return;

[Severity: High]
Can this allow ufshcd_err_handler() to run regular error recovery
concurrently with hibernation poweroff in ufshcd_wl_poweroff()?

During hibernation, the PM core runs .freeze (ufshcd_wl_suspend(), which
takes hba->host_sem) followed by .thaw (ufshcd_wl_resume(), which releases
hba->host_sem) before writing the hibernation image, and then invokes
.poweroff:

drivers/ufs/core/ufshcd.c:ufshcd_wl_poweroff() {
	struct scsi_device *sdev = to_scsi_device(dev);
	struct ufs_hba *hba = shost_priv(sdev->host);

	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
	return 0;
}

When ufshcd_wl_poweroff() calls __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM),
hba->runtime_pm_in_progress is set to false. However, unlike
ufshcd_wl_suspend(), ufshcd_wl_poweroff() does not hold hba->host_sem, and
unlike ufshcd_wl_shutdown(), it does not set hba->shutting_down = true.

If an error interrupt schedules hba->eh_work while ufshcd_wl_poweroff() is
running, ufshcd_err_handler() will skip the early return because
hba->runtime_pm_in_progress is false, acquire the uncontended
hba->host_sem, see ufshcd_err_handling_should_stop(hba) return false, and
proceed with ufshcd_err_handling_prepare() and ufshcd_reset_and_restore()
at the same time __ufshcd_wl_suspend() is powering down the device and
link.

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