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

[PATCH v9 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>
Reviewed-by: Bean Huo <beanhuo@micron.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 v9 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. If __ufshcd_wl_suspend() fails, release host_sem and return the
error as ufshcd_wl_suspend() does, otherwise the error handler, which
waits for host_sem, would block forever.

This changes the behavior on failure: the error is now returned from the
.poweroff callback, which aborts hibernation, instead of being ignored.

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>
---
 drivers/ufs/core/ufshcd.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 3b5580ac390c..0d86c67f0b1a 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -11573,9 +11573,15 @@ static int ufshcd_wl_poweroff(struct device *dev)
 {
 	struct scsi_device *sdev = to_scsi_device(dev);
 	struct ufs_hba *hba = shost_priv(sdev->host);
+	int ret;
 
-	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
-	return 0;
+	/* Released by ufshcd_wl_resume() if hibernation is aborted. */
+	down(&hba->host_sem);
+	ret = __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
+	if (ret)
+		up(&hba->host_sem);
+
+	return ret;
 }
 #endif
 
-- 
2.34.1

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

[Peter Wang]

On Tue, 2026-11-10 at 11:03 +0900, HOYOUNG SEO wrote:
> 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. If __ufshcd_wl_suspend() fails, release host_sem and return
> the
> error as ufshcd_wl_suspend() does, otherwise the error handler, which
> waits for host_sem, would block forever.
> 
> This changes the behavior on failure: the error is now returned from
> the
> .poweroff callback, which aborts hibernation, instead of being
> ignored.
> 
> 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: Peter Wang <peter.wang@mediatek.com>

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

[Bart Van Assche]

On 11/9/26 6:03 PM, HOYOUNG SEO wrote:
 > -	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
 > -	return 0;
 > +	/* Released by ufshcd_wl_resume() if hibernation is aborted. */
 > +	down(&hba->host_sem);
 > +	ret = __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
 > +	if (ret)
 > +		up(&hba->host_sem);
 > +
 > +	return ret;

Now that the __ufshcd_wl_suspend() return value is propagated, I think
that the issues reported by Sashiko must be addressed:

- If the WLUN is runtime suspended when .poweroff is called (this
   happens if ufshcd_rpm_ok_for_spm() returned true in
   __ufshcd_suspend_prepare(), e.g. with the default rpm_lvl ==
   spm_lvl), __ufshcd_wl_suspend() triggers error recovery and returns
   -EBUSY. power_down() in kernel/power/hibernate.c interprets -EBUSY
   as a wakeup event and rolls back hibernation. ufshcd_wl_suspend()
   checks pm_runtime_suspended(dev) and ufshcd_wl_shutdown() calls
   ufshcd_rpm_get_sync(). ufshcd_wl_poweroff() does neither.

- For UFS_SHUTDOWN_PM, failures of ufshcd_set_dev_pwr_mode() and
   ufshcd_link_state_transition() are not converted into -EBUSY, so a
   positive SCSI or UIC status can be returned from a dev_pm_ops
   callback.

- hba->is_sys_suspended is not set if .poweroff succeeds.

How about moving the body of ufshcd_wl_suspend() into a helper
function that accepts a pm_op argument and calling that helper from
both ufshcd_wl_suspend() and ufshcd_wl_poweroff()? That would make the
poweroff path identical to the system suspend path except for the
target power mode and link state.

Thanks,

Bart.

[PATCH v9 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 pm_op_in_progress is set and 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. The runtime status
alone is not enough: it also covers the block layer steps before and
after the driver callbacks, where the early link recovery must not run.
pm_op_in_progress is only set while __ufshcd_wl_suspend() or
__ufshcd_wl_resume() run, which is inside that window.

Both checks are done in the new helper
ufshcd_rpm_transition_in_progress(), next to the other ufshcd_rpm_*()
helpers. The runtime status is read with READ_ONCE() and without
power.lock held. It is only available with CONFIG_PM, so the helper
returns false without it. Both values are read without locking. As the
window of pm_op_in_progress is contained in the runtime PM window, a
stale value of either one can only make the error handler take the
regular path, like an error handler that starts just before or after
the PM operation.

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 | 17 +++++++++++++++++
 drivers/ufs/core/ufshcd.c      |  7 ++++++-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index e55c2a02c1f5..8cd9fabc4c02 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -413,6 +413,23 @@ static inline void ufshcd_rpm_put(struct ufs_hba *hba)
 	pm_runtime_put(&hba->ufs_device_wlun->sdev_gendev);
 }
 
+/*
+ * Whether __ufshcd_wl_suspend() or __ufshcd_wl_resume() runs for a runtime
+ * suspend or resume of the WLUN.
+ */
+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 hba->pm_op_in_progress &&
+	       (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 0d86c67f0b1a..0cca33d59065 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 the PM callbacks run for a runtime PM
+		 * operation. 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 v9 3/3] scsi: ufs: core: Limit early link recovery in the error handler to runtime PM

[Peter Wang]

On Tue, 2026-11-10 at 11:03 +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 pm_op_in_progress is set and 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. The runtime status
> alone is not enough: it also covers the block layer steps before and
> after the driver callbacks, where the early link recovery must not
> run.
> pm_op_in_progress is only set while __ufshcd_wl_suspend() or
> __ufshcd_wl_resume() run, which is inside that window.
> 
> Both checks are done in the new helper
> ufshcd_rpm_transition_in_progress(), next to the other ufshcd_rpm_*()
> helpers. The runtime status is read with READ_ONCE() and without
> power.lock held. It is only available with CONFIG_PM, so the helper
> returns false without it. Both values are read without locking. As
> the
> window of pm_op_in_progress is contained in the runtime PM window, a
> stale value of either one can only make the error handler take the
> regular path, like an error handler that starts just before or after
> the PM operation.
> 
> 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>
> ---

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

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

[Bart Van Assche]

On 11/9/26 6:03 PM, HOYOUNG SEO wrote:
 > +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 hba->pm_op_in_progress &&
 > +	       (status == RPM_SUSPENDING || status == RPM_RESUMING);

hba->pm_op_in_progress is modified by __ufshcd_wl_suspend() and
__ufshcd_wl_resume() without holding any lock. Please use READ_ONCE()
here and WRITE_ONCE() in the functions that modify this member, or at
least make the patch description consistent with the code ("Both
values are read without locking" while only one of the two reads uses
READ_ONCE()).

Additionally, since this function has only one caller, please make it
a static function in ufshcd.c instead of adding it to ufshcd-priv.h. A
name that makes it clear that this function checks whether a runtime
PM callback of the WLUN is in progress, e.g.
ufshcd_in_wl_rpm_callback(), would be more accurate.

Otherwise this patch looks good to me. Please note that the
correctness of this patch depends on patch 2/3, which needs to be
revised.

Thanks,

Bart.