[PATCH v8 1/3] scsi: ufs: core: Clear saved errors in ufshcd_link_recovery()

[PATCH v8 1/3] 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.

Fixes: 35dabf4503b9 ("scsi: ufs: core: Use link recovery when h8 exit fails during runtime resume")
Signed-off-by: HOYOUNG SEO <hy50.seo@samsung.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Peter Wang <peter.wang@mediatek.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 v8 2/3] scsi: ufs: core: Hold host_sem in ufshcd_wl_poweroff()

[HOYOUNG SEO]

ufshcd_wl_suspend() takes hba->host_sem and ufshcd_wl_resume() releases
it, which serializes the error handler against system suspend and
resume. ufshcd_wl_poweroff(), the hibernation poweroff callback, also
suspends the device with UFS_SHUTDOWN_PM but does not take host_sem. An
error handler that runs in the meantime can reset the link concurrently
with __ufshcd_wl_suspend().

It also leaves host_sem unbalanced: if hibernation is aborted after
ufshcd_wl_poweroff() has been called, ufshcd_wl_resume() is called as
the .restore callback and releases host_sem, which was not acquired.

Make ufshcd_wl_poweroff() behave like ufshcd_wl_suspend() and keep
holding host_sem. It is released by ufshcd_wl_resume() if hibernation is
aborted. Do not use hba->shutting_down here: it makes
ufs_mtk_system_suspend(), which is also the .poweroff callback of the
MediaTek host controller, fail with -EBUSY, and nothing clears it when
hibernation is aborted.

Fixes: b294ff3e3449 ("scsi: ufs: core: Enable power management for wlun")
Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: HOYOUNG SEO <hy50.seo@samsung.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 3b5580ac390c..1c935a8d016d 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -11574,6 +11574,8 @@ static int ufshcd_wl_poweroff(struct device *dev)
 	struct scsi_device *sdev = to_scsi_device(dev);
 	struct ufs_hba *hba = shost_priv(sdev->host);
 
+	/* Released by ufshcd_wl_resume() if hibernation is aborted. */
+	down(&hba->host_sem);
 	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
 	return 0;
 }
-- 
2.34.1

[PATCH v8 3/3] 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,
shutdown and poweroff, 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 suspend or resume of the WLUN
is in progress. The runtime PM core sets the runtime status of the WLUN
to RPM_SUSPENDING or RPM_RESUMING while ufshcd_wl_runtime_suspend() or
ufshcd_wl_runtime_resume() runs, and it is RPM_ACTIVE or RPM_SUSPENDED
during system PM. Read it with READ_ONCE() without power.lock held in
the new helper ufshcd_rpm_transition_in_progress(), next to the other
ufshcd_rpm_*() helpers. The status is only available with CONFIG_PM, so
the helper returns false without it.

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, and ufshcd_wl_poweroff() holds it as
    well, so the error handler waits for them;
  - during shutdown, hba->shutting_down makes the error handler stop.

This changes the behavior during shutdown: errors that occur while the
final START STOP UNIT command is being processed are no longer handled
by the error handler. Recovery then only happens through
ufshcd_eh_timed_out().

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

diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index e55c2a02c1f5..ae3eaf5ab017 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -413,6 +413,19 @@ static inline void ufshcd_rpm_put(struct ufs_hba *hba)
 	pm_runtime_put(&hba->ufs_device_wlun->sdev_gendev);
 }
 
+/* Whether a runtime suspend or resume of the WLUN is in progress. */
+static inline bool ufshcd_rpm_transition_in_progress(struct ufs_hba *hba)
+{
+#ifdef CONFIG_PM
+	enum rpm_status status =
+		READ_ONCE(hba->ufs_device_wlun->sdev_gendev.power.runtime_status);
+
+	return status == RPM_SUSPENDING || status == RPM_RESUMING;
+#else
+	return false;
+#endif
+}
+
 /**
  * ufs_is_valid_unit_desc_lun - checks if the given LUN has a unit descriptor
  * @dev_info: pointer of instance of struct ufs_dev_info
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 1c935a8d016d..a79951cfcb29 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -7091,9 +7091,14 @@ 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 this only while a runtime PM operation is in progress. The
+		 * system PM and poweroff callbacks hold host_sem, and
+		 * shutting_down makes the regular path below stop, so it does
+		 * not reset the link concurrently with them.
 		 */
 		ufshcd_rpm_get_noresume(hba);
-		if (hba->pm_op_in_progress) {
+		if (ufshcd_rpm_transition_in_progress(hba)) {
 			ufshcd_link_recovery(hba);
 			ufshcd_rpm_put(hba);
 			return;
-- 
2.34.1

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

[Peter Wang]

On Mon, 2026-11-09 at 13:09 +0900, HOYOUNG SEO wrote:
> 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,
> shutdown and poweroff, 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 suspend or resume of the
> WLUN
> is in progress. The runtime PM core sets the runtime status of the
> WLUN
> to RPM_SUSPENDING or RPM_RESUMING while ufshcd_wl_runtime_suspend()
> or
> ufshcd_wl_runtime_resume() runs, and it is RPM_ACTIVE or
> RPM_SUSPENDED
> during system PM. Read it with READ_ONCE() without power.lock held in
> the new helper ufshcd_rpm_transition_in_progress(), next to the other
> ufshcd_rpm_*() helpers. The status is only available with CONFIG_PM,
> so
> the helper returns false without it.
> 
> 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, and ufshcd_wl_poweroff() holds it as
>     well, so the error handler waits for them;
>   - during shutdown, hba->shutting_down makes the error handler stop.
> 
> This changes the behavior during shutdown: errors that occur while
> the
> final START STOP UNIT command is being processed are no longer
> handled
> by the error handler. Recovery then only happens through
> ufshcd_eh_timed_out().
> 
> Fixes: e23ef4f22db3 ("scsi: ufs: core: Fix error handler host_sem
> issue")
> Signed-off-by: HOYOUNG SEO <hy50.seo@samsung.com>
> ---

Sashiko report issue is previous issue and should be 
fixed separately.

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

Re: [PATCH v8 2/3] scsi: ufs: core: Hold host_sem in ufshcd_wl_poweroff()

[Peter Wang]

On Mon, 2026-11-09 at 13:09 +0900, HOYOUNG SEO wrote:
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 3b5580ac390c..1c935a8d016d 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -11574,6 +11574,8 @@ static int ufshcd_wl_poweroff(struct device
> *dev)
>  	struct scsi_device *sdev = to_scsi_device(dev);
>  	struct ufs_hba *hba = shost_priv(sdev->host);
>  
> +	/* Released by ufshcd_wl_resume() if hibernation is aborted.
> */
> +	down(&hba->host_sem);
>  	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
>  	return 0;
>  }

Hi Hoyoung,

Although not checking the return value was an original 
mistake, this patch would make the previous mistake worse
and cause a system hang. Could we at least add a check 
to release the lock?

+	ret = __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
+	if (ret)
+		up(&hba->host_sem);

Thanks
Peter

Re: [PATCH v8 1/3] scsi: ufs: core: Clear saved errors in ufshcd_link_recovery()

[Bean Huo]

Reviewed-by: Bean Huo <beanhuo@micron.com>

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

[Bean Huo]

On Mon, 2026-11-09 at 13:09 +0900, HOYOUNG SEO wrote:
> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
> index e55c2a02c1f5..ae3eaf5ab017 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -413,6 +413,19 @@ static inline void ufshcd_rpm_put(struct ufs_hba *hba)
>         pm_runtime_put(&hba->ufs_device_wlun->sdev_gendev);
>  }
>  
> +/* Whether a runtime suspend or resume of the WLUN is in progress. */
> +static inline bool ufshcd_rpm_transition_in_progress(struct ufs_hba *hba)
> +{
> +#ifdef CONFIG_PM
> +       enum rpm_status status =
> +               READ_ONCE(hba->ufs_device_wlun-
> >sdev_gendev.power.runtime_status);
> +
> +       return status == RPM_SUSPENDING || status == RPM_RESUMING;



should the helper also check pm_op_in_progress?

return hba->pm_op_in_progress && (status == RPM_SUSPENDING || status ==
RPM_RESUMING);


checking (status == RPM_SUSPENDING || status == RPM_RESUMING) fixes for system
PM; 

Checking pm_op_in_progress limits the early path to __ufshcd_wl_suspend() and
__ufshcd_wl_resume() during runtime PM, it was before this patch.

Or what else I missed?

Kind regards,
Bean

Re: [PATCH v8 2/3] scsi: ufs: core: Hold host_sem in ufshcd_wl_poweroff()

[Bart Van Assche]

On 11/9/26 1:34 AM, Peter Wang wrote:
 > Although not checking the return value was an original
 > mistake, this patch would make the previous mistake worse
 > and cause a system hang. Could we at least add a check
 > to release the lock?
 >
 > +	ret = __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
 > +	if (ret)
 > +		up(&hba->host_sem);


I agree with Peter. If __ufshcd_wl_suspend() fails, host_sem must be
released so that ufshcd_err_handler() does not deadlock waiting on
down(&hba->host_sem). Also, please propagate 'ret' instead of returning
0 on failure.

Thanks,

Bart.

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

[Bart Van Assche]

On 11/9/26 9:08 AM, Bean Huo wrote:
 > On Mon, 2026-11-09 at 13:09 +0900, HOYOUNG SEO wrote:
 >> diff --git a/drivers/ufs/core/ufshcd-priv.h 
b/drivers/ufs/core/ufshcd-priv.h
 >> index e55c2a02c1f5..ae3eaf5ab017 100644
 >> --- a/drivers/ufs/core/ufshcd-priv.h
 >> +++ b/drivers/ufs/core/ufshcd-priv.h
 >> @@ -413,6 +413,19 @@ static inline void ufshcd_rpm_put(struct 
ufs_hba *hba)
 >>  	pm_runtime_put(&hba->ufs_device_wlun->sdev_gendev);
 >>  }
 >>
 >> +/* Whether a runtime suspend or resume of the WLUN is in progress. */
 >> +static inline bool ufshcd_rpm_transition_in_progress(struct ufs_hba 
*hba)
 >> +{
 >> +#ifdef CONFIG_PM
 >> +	enum rpm_status status =
 >> +		READ_ONCE(hba->ufs_device_wlun->sdev_gendev.power.runtime_status);
 >> +
 >> +	return status == RPM_SUSPENDING || status == RPM_RESUMING;
 >
 > should the helper also check pm_op_in_progress?
 >
 > return hba->pm_op_in_progress && (status == RPM_SUSPENDING || status ==
 > RPM_RESUMING);
 >
 > checking (status == RPM_SUSPENDING || status == RPM_RESUMING) fixes 
for system
 > PM;
 >
 > Checking pm_op_in_progress limits the early path to 
__ufshcd_wl_suspend() and
 > __ufshcd_wl_resume() during runtime PM, it was before this patch.
 >
 > Or what else I missed?

Good catch. The runtime PM core sets runtime_status to RPM_SUSPENDING
before calling the device's runtime suspend callback (and before
ufshcd_hold() is called), and keeps it set until after
blk_post_runtime_suspend() (after ufshcd_release() has been called).

Checking both hba->pm_op_in_progress and the WLUN runtime_status ensures
the early recovery path only runs while __ufshcd_wl_suspend() or
__ufshcd_wl_resume() is actively running and clocks are held.

Hoyoung, please include the pm_op_in_progress check in the helper or at
the call site in v9.

Thanks,

Bart.