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

[PATCH v6 0/3] 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 and poweroff it stops.

Patch 3 sets hba->shutting_down in ufshcd_wl_poweroff() like
ufshcd_wl_shutdown() does, so that the error handler stops during
poweroff.

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 v6:
- Go back to a single flag for runtime PM, hba->runtime_pm_in_progress,
  instead of the system PM flag of v5, and set it in
  ufshcd_wl_runtime_suspend() and ufshcd_wl_runtime_resume(). The error
  handler no longer reads two variables that are not updated atomically
  (Sashiko review of v5).
- Add patch 2. With patch 3, poweroff no longer takes the early path,
  and ufshcd_wl_poweroff() holds neither host_sem nor sets
  hba->shutting_down (Sashiko review of v4).

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.

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.

v5: https://lore.kernel.org/linux-scsi/cover.1793865172.git.hy50.seo@samsung.com/
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 (3):
  scsi: ufs: core: Clear saved errors in ufshcd_link_recovery()
  scsi: ufs: core: Limit early link recovery in the error handler to
    runtime PM
  scsi: ufs: core: Set shutting_down in ufshcd_wl_poweroff()

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

-- 
2.34.1

[PATCH v6 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>
---
 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

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

[Bart Van Assche]

On 11/5/26 6:15 PM, HOYOUNG SEO wrote:
> 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.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

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

[Peter Wang]

On Fri, 2026-11-06 at 11:15 +0900, HOYOUNG SEO wrote:
> 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>

[PATCH v6 2/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 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 and poweroff, 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. It is set in ufshcd_wl_runtime_suspend() and
ufshcd_wl_runtime_resume() around the PM operation, so it covers
pm_op_in_progress for runtime PM and is never set for the other PM
operations. The error handler tests only this flag.

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 | 11 ++++++++++-
 include/ufs/ufshcd.h      |  2 ++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 3b5580ac390c..95beb95bb036 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 not do this for system PM, shutdown and poweroff. The
+		 * system PM 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 (hba->runtime_pm_in_progress) {
 			ufshcd_link_recovery(hba);
 			ufshcd_rpm_put(hba);
 			return;
@@ -10678,11 +10683,13 @@ static int ufshcd_wl_runtime_suspend(struct device *dev)
 	ktime_t start = ktime_get();
 
 	hba = shost_priv(sdev->host);
+	hba->runtime_pm_in_progress = true;
 
 	ret = __ufshcd_wl_suspend(hba, UFS_RUNTIME_PM);
 	if (ret)
 		dev_err(&sdev->sdev_gendev, "%s failed: %d\n", __func__, ret);
 
+	hba->runtime_pm_in_progress = false;
 	trace_ufshcd_wl_runtime_suspend(hba, ret,
 		ktime_to_us(ktime_sub(ktime_get(), start)),
 		hba->curr_dev_pwr_mode, hba->uic_link_state);
@@ -10698,11 +10705,13 @@ static int ufshcd_wl_runtime_resume(struct device *dev)
 	ktime_t start = ktime_get();
 
 	hba = shost_priv(sdev->host);
+	hba->runtime_pm_in_progress = true;
 
 	ret = __ufshcd_wl_resume(hba, UFS_RUNTIME_PM);
 	if (ret)
 		dev_err(&sdev->sdev_gendev, "%s failed: %d\n", __func__, ret);
 
+	hba->runtime_pm_in_progress = false;
 	trace_ufshcd_wl_runtime_resume(hba, ret,
 		ktime_to_us(ktime_sub(ktime_get(), start)),
 		hba->curr_dev_pwr_mode, hba->uic_link_state);
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 v6 2/3] scsi: ufs: core: Limit early link recovery in the error handler to runtime PM

[Bart Van Assche]

On 11/5/26 6:15 PM, HOYOUNG SEO wrote:
 > 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 and poweroff, hba->shutting_down makes the error
 >     handler stop.

The second claim only holds for poweroff after patch 3/3 has been
applied. With this patch applied but not patch 3/3, an error that is
reported during hibernation poweroff makes the error handler skip the
early path, acquire host_sem (ufshcd_wl_poweroff() does not hold it) and
call ufshcd_err_handling_prepare() and ufshcd_reset_and_restore()
concurrently with __ufshcd_wl_suspend(). Please move the patch that
serializes the error handler against ufshcd_wl_poweroff() in front of
this patch so that the series remains bisectable.

Additionally, 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(). I'm
fine with that, but please mention this behavior change in the patch
description.

 > -		if (hba->pm_op_in_progress) {
 > +		if (hba->runtime_pm_in_progress) {

runtime_pm_in_progress is modified by the runtime PM callbacks and read
by the error handler without any locking. Please use READ_ONCE() here
and WRITE_ONCE() in ufshcd_wl_runtime_suspend() and
ufshcd_wl_runtime_resume() to annotate these data races.

Thanks,

Bart.

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

[Peter Wang]

On Fri, 2026-11-06 at 11:15 +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
> 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.
> 

Hi HOYOUNG,

Since ufshcd_err_handler already calls ufshcd_link_recovery,
wouldn’t it be better if __ufshcd_wl_resume just triggers 
the err handler and waits for the kworker to finish,
rather than calling ufshcd_link_recovery directly?

Thanks
Peter

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

[hoyoung seo]

Hi Peter,

Thank you for the review, and for the Reviewed-by on patch 1. I will add
it. Note that I have already posted v7, which reorders the series
following Bart's comments:
https://lore.kernel.org/linux-scsi/cover.1793943246.git.hy50.seo@samsung.com/

On your question about making __ufshcd_wl_resume() trigger the error
handler and wait for it instead of calling ufshcd_link_recovery()
directly: I think that is a different design from the one in this
series, and I see a few problems with it. I may be missing something.

 - While the system PM callbacks run, host_sem is held from
   ufshcd_wl_suspend() until ufshcd_wl_resume() returns. If
   __ufshcd_wl_resume() waited for the error handler, the error handler
   could not wait for host_sem (that would be a deadlock), so it would
   have to keep taking the early link recovery path during system
   resume. This series does the opposite, as Bart suggested: for system
   PM the error handler waits for host_sem, and the early path is only
   used for runtime PM.

 - The link recovery was put into the resume path on purpose. Commit
   35dabf4503b9 ("scsi: ufs: core: Use link recovery when h8 exit fails
   during runtime resume") says that the error handler got stuck when the
   h8 exit failed during runtime resume, and commit 62c015373e1c moved it
   to __ufshcd_wl_resume(). As far as I can see this is because
   ufshcd_err_handling_prepare() waits for the runtime PM operation that
   is in progress. Waiting for the error handler from the resume path
   would bring that dependency back.

 - __ufshcd_wl_resume() needs to know whether the recovery succeeded to
   decide how to continue, while the error handler returns no result, so
   it would have to look at the HBA state afterwards. It would also need
   to avoid queueing the error handler again if it is already running,
   similar to what __ufshcd_wl_suspend() does with
   ufshcd_eh_in_progress(), to prevent a second recovery.

 - Other callers of ufshcd_link_recovery() (ufshcd_eh_timed_out(),
   ufshcd_eh_host_reset_handler(), ufs_mtk_resume()) would still call it
   directly.

For these reasons I would prefer to keep the direct call in the resume
path and fix the race on the error handler side, as in this series. If
you see a way to wait for the error handler without the runtime resume
dependency, I am happy to look at it as a follow-up.

Thanks,
Hoyoung

> -----Original Message-----
> From: Peter Wang <peter.wang@mediatek.com>
> Sent: Friday, November 6, 2026 4:11 PM
> To: HOYOUNG SEO <hy50.seo@samsung.com>; linux-scsi@vger.kernel.org; linux-
> kernel@vger.kernel.org; bvanassche@acm.org; mkp@kernel.org;
> martin.petersen@oracle.com; stanleyjhu@google.com; alim.akhtar@samsung.com;
> kwangwon.min@samsung.com; kwmad.kim@samsung.com; cpgs@samsung.com;
> h10.kim@samsung.com
> Subject: Re: [PATCH v6 2/3] scsi: ufs: core: Limit early link recovery in
> the error handler to runtime PM
> 
> On Fri, 2026-11-06 at 11:15 +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 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.
> >
> 
> Hi HOYOUNG,
> 
> Since ufshcd_err_handler already calls ufshcd_link_recovery, wouldn’t it
> be better if __ufshcd_wl_resume just triggers the err handler and waits
> for the kworker to finish, rather than calling ufshcd_link_recovery
> directly?
> 
> Thanks
> Peter

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

[Peter Wang]

On Fri, 2026-11-06 at 16:58 +0900, hoyoung seo wrote:
> Hi Peter,
> 
> Thank you for the review, and for the Reviewed-by on patch 1. I will
> add
> it. Note that I have already posted v7, which reorders the series
> following Bart's comments:
> https://lore.kernel.org/linux-scsi/cover.1793943246.git.hy50.seo@samsung.com/
>  
> 
> On your question about making __ufshcd_wl_resume() trigger the error
> handler and wait for it instead of calling ufshcd_link_recovery()
> directly: I think that is a different design from the one in this
> series, and I see a few problems with it. I may be missing something.
> 
>  - While the system PM callbacks run, host_sem is held from
>    ufshcd_wl_suspend() until ufshcd_wl_resume() returns. If
>    __ufshcd_wl_resume() waited for the error handler, the error
> handler
>    could not wait for host_sem (that would be a deadlock), so it
> would
>    have to keep taking the early link recovery path during system
>    resume. This series does the opposite, as Bart suggested: for
> system
>    PM the error handler waits for host_sem, and the early path is
> only
>    used for runtime PM.
> 

Hi hoyoung,

I think both ways could solve this issue. But excluding system PM fast 
recovery seems more complicated. Perhaps we should wait for Bart’s
thoughts, but for me:
1. When wl resume fails, having different error handling would confuse
me.
2. Does the runtime resume error path still have a chance of concurrent
recovery?


>  - The link recovery was put into the resume path on purpose. Commit
>    35dabf4503b9 ("scsi: ufs: core: Use link recovery when h8 exit
> fails
>    during runtime resume") says that the error handler got stuck when
> the
>    h8 exit failed during runtime resume, and commit 62c015373e1c
> moved it
>    to __ufshcd_wl_resume(). As far as I can see this is because
>    ufshcd_err_handling_prepare() waits for the runtime PM operation
> that
>    is in progress. Waiting for the error handler from the resume path
>    would bring that dependency back.
> 

Yes, but now error handler recovery will be handled first, so it won’t
reach 
ufshcd_err_handling_prepare. So, there should be no deadlock concern,
right?


>  - __ufshcd_wl_resume() needs to know whether the recovery succeeded
> to
>    decide how to continue, while the error handler returns no result,
> so
>    it would have to look at the HBA state afterwards. It would also
> need
>    to avoid queueing the error handler again if it is already
> running,
>    similar to what __ufshcd_wl_suspend() does with
>    ufshcd_eh_in_progress(), to prevent a second recovery.
> 

It only needs to queue_work and wait for flush_work, and believe that 
the error handler calling ufshcd_link_recovery will make everything
good. 
Otherwise, __ufshcd_wl_resume has no way to handle it either, right?
Even if it is queued twice, the second time it will check 
ufshcd_err_handling_should_stop and do nothing.


>  - Other callers of ufshcd_link_recovery() (ufshcd_eh_timed_out(),
>    ufshcd_eh_host_reset_handler(), ufs_mtk_resume()) would still call
> it
>    directly.
> 

My earlier thinking was that if an error occurs during resume, 
doing recovery directly is faster, because it isn’t necessarily
accompanied by an err handler worker.
But if the err handler might also be triggered, it would be 
better to simply trigger the error handler, let it perform the 
recovery, and wait for it to finish. Would that make more sense?

Thanks
Peter

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

[Bart Van Assche]

On 11/6/26 1:44 AM, Peter Wang wrote:
 > Yes, but now error handler recovery will be handled first, so it won't
 > reach ufshcd_err_handling_prepare. So, there should be no deadlock
 > concern, right?

Hi Peter,

I don't think that's guaranteed. ufshcd_err_handler() tests
hba->pm_op_in_progress only once, when it starts, and without holding
any lock. An error handler that started before that flag was set skips
the early path. For runtime PM it then blocks in ufshcd_rpm_get_sync()
in ufshcd_err_handling_prepare(), waiting for the runtime resume that
is in progress. If __ufshcd_wl_resume() waits for that error handler
with flush_work(), neither can proceed. This is the deadlock that
commit 35dabf4503b9 addressed and that the comment in
ufshcd_eh_timed_out() describes.

System PM has a similar problem. __ufshcd_wl_suspend() clears
hba->pm_op_in_progress before it returns, but host_sem stays held
until ufshcd_wl_resume() returns. An error handler that runs between
system suspend and system resume therefore takes the regular path and
blocks in down(&hba->host_sem). A flush_work() call from
__ufshcd_wl_resume() would then never return.

Additionally, ufshcd_schedule_eh_work() does not queue the error
handler if the HBA state is UFSHCD_STATE_ERROR. In that case
queue_work() + flush_work() would not perform any recovery, and
__ufshcd_wl_resume() would have no way to find out whether recovery
succeeded.

 > 1. When wl resume fails, having different error handling would confuse
 > me.

The resume path is not modified by this series: __ufshcd_wl_resume()
calls ufshcd_link_recovery() for both runtime and system PM. What
changes is that the error handler no longer takes the early path, which
does not hold host_sem, during system PM. That path is a workaround for
a runtime PM deadlock. During system PM the error handler can simply
wait for host_sem, which serializes it against the PM callbacks. Doing
that is easier to reason about than letting two concurrent link
recoveries race.

 > 2. Does the runtime resume error path still have a chance of concurrent
 > recovery?

Yes. The cover letter mentions this. That issue predates this series,
and I think it should be addressed separately instead of holding up
these fixes.

Hence, I prefer the approach of v7 of this series.

Thanks,

Bart.

[PATCH v6 3/3] scsi: ufs: core: Set shutting_down in ufshcd_wl_poweroff()

[HOYOUNG SEO]

ufshcd_wl_shutdown() sets hba->shutting_down under host_sem before it
suspends the device, so that ufshcd_err_handling_should_stop() makes the
error handler return instead of recovering the HBA while it is being shut
down.

ufshcd_wl_poweroff(), which is used for hibernation, also suspends the
device with UFS_SHUTDOWN_PM but neither holds host_sem nor sets
hba->shutting_down. An error handler that runs in the meantime can reset
the link concurrently with __ufshcd_wl_suspend().

Set hba->shutting_down in ufshcd_wl_poweroff() in the same way as
ufshcd_wl_shutdown() does.

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

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 95beb95bb036..a52d6ccfa56b 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -11583,6 +11583,10 @@ static int ufshcd_wl_poweroff(struct device *dev)
 	struct scsi_device *sdev = to_scsi_device(dev);
 	struct ufs_hba *hba = shost_priv(sdev->host);
 
+	down(&hba->host_sem);
+	hba->shutting_down = true;
+	up(&hba->host_sem);
+
 	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
 	return 0;
 }
-- 
2.34.1

Re: [PATCH v6 3/3] scsi: ufs: core: Set shutting_down in ufshcd_wl_poweroff()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: ufs: core: ufshcd_wl_poweroff() breaks ufs_mtk_system_suspend()
- [High] scsi: ufs: core: ufshcd_wl_poweroff() corrupts state on restore rollback
--

commit 744164ab90d2e91d1a44bcd61cfe43ee02a7ac78
Author: HOYOUNG SEO <hy50.seo@samsung.com>

scsi: ufs: core: Set shutting_down in ufshcd_wl_poweroff()

This commit sets hba->shutting_down under hba->host_sem in
ufshcd_wl_poweroff() before calling __ufshcd_wl_suspend() so that the error
handler does not attempt link recovery during hibernation poweroff.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 8571e91263e51..e59cdc224ef96 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -11592,6 +11592,10 @@ static int ufshcd_wl_poweroff(struct device *dev)
>  	struct scsi_device *sdev = to_scsi_device(dev);
>  	struct ufs_hba *hba = shost_priv(sdev->host);
>  
> +	down(&hba->host_sem);
> +	hba->shutting_down = true;

[Severity: High]
Will setting hba->shutting_down = true in ufshcd_wl_poweroff() cause
hibernation poweroff to fail on MediaTek controllers?

During hibernation poweroff via:

  hibernate() ->
    power_down() ->
      hibernation_platform_enter() ->
        dpm_suspend_start() ->
          dpm_suspend()

devices are suspended in child-before-parent order, so ufshcd_wl_poweroff()
on the child device wlun (&hba->ufs_device_wlun->sdev_gendev) runs before the
parent controller's .poweroff callback on hba->dev.

In drivers/ufs/host/ufs-mediatek.c, ufs_mtk_pm_ops wires .poweroff to
ufs_mtk_system_suspend():

static const struct dev_pm_ops ufs_mtk_pm_ops = {
	SYSTEM_SLEEP_PM_OPS(ufs_mtk_system_suspend, ufs_mtk_system_resume)
...
};

And ufs_mtk_system_suspend() checks hba->shutting_down at entry:

static int ufs_mtk_system_suspend(struct device *dev)
{
	struct ufs_hba *hba = dev_get_drvdata(dev);
	struct arm_smccc_res res;
	int ret;

	if (hba->shutting_down) {
		ret = -EBUSY;
		goto out;
	}

When ufshcd_wl_poweroff() sets hba->shutting_down = true first, won't
ufs_mtk_system_suspend() immediately fail and abort hibernation?

> +	up(&hba->host_sem);

[Severity: High]
What happens if hibernation poweroff rolls back and invokes .restore?

In drivers/ufs/core/ufshcd.c, ufshcd_wl_pm_ops pairs .poweroff with .restore:

static const struct dev_pm_ops ufshcd_wl_pm_ops = {
#ifdef CONFIG_PM_SLEEP
	.suspend = ufshcd_wl_suspend,
	.resume = ufshcd_wl_resume,
	.freeze = ufshcd_wl_suspend,
	.thaw = ufshcd_wl_resume,
	.poweroff = ufshcd_wl_poweroff,
	.restore = ufshcd_wl_resume,
#endif

If hibernation_platform_enter() aborts after ufshcd_wl_poweroff() has run
(for example due to a pending wakeup event or a later device failing its
poweroff callback), dpm_resume_end() calls ufshcd_wl_resume():

static int ufshcd_wl_resume(struct device *dev)
{
...
	if (!ret)
		hba->is_sys_suspended = false;
	up(&hba->host_sem);
	return ret;
}

Unlike ufshcd_wl_suspend(), which keeps hba->host_sem held until
ufshcd_wl_resume() releases it, ufshcd_wl_poweroff() releases hba->host_sem
immediately, and ufshcd_wl_resume() never clears hba->shutting_down.

After a hibernation rollback, doesn't this perform an unbalanced
up(&hba->host_sem) (incrementing the semaphore count to 2) and leave
hba->shutting_down permanently set to true on the running system, disabling
error recovery in ufshcd_err_handling_should_stop() and blocking user
accesses in ufshcd_is_user_access_allowed()?

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

Re: [PATCH v6 3/3] scsi: ufs: core: Set shutting_down in ufshcd_wl_poweroff()

[Bart Van Assche]

On 11/5/26 6:15 PM, HOYOUNG SEO wrote:
 > +	down(&hba->host_sem);
 > +	hba->shutting_down = true;
 > +	up(&hba->host_sem);
 > +
 >  	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);

I agree with both issues reported by Sashiko:
- ufs_mtk_system_suspend() is also the .poweroff callback of the
   MediaTek host controller device and returns -EBUSY if
   hba->shutting_down has been set. Since the .poweroff callback of the
   WLUN is called before that of its parent, this patch makes
   hibernation fail on MediaTek hosts.
- If hibernation is aborted after the WLUN .poweroff callback has been
   called, ufshcd_wl_resume() is called as .restore callback.
   Nothing clears hba->shutting_down, so the error handler and user
   access (ufshcd_is_user_access_allowed()) remain disabled.

Additionally, the existing code already has an unbalanced
up(&hba->host_sem) on that path: ufshcd_wl_poweroff() does not acquire
host_sem but ufshcd_wl_resume() releases it.

How about making ufshcd_wl_poweroff() behave like ufshcd_wl_suspend()
instead, i.e. acquire host_sem and keep holding it until .restore
releases it? Something like this (untested):

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;
}

That serializes the error handler against poweroff the same way as for
system suspend, fixes the unbalanced up() call and leaves
hba->shutting_down alone. Please also add a Fixes: tag and move this
patch in front of patch 2/3.

Thanks,

Bart.

RE: [PATCH v6 3/3] scsi: ufs: core: Set shutting_down in ufshcd_wl_poweroff()

[hoyoung seo]

Hi Bart,

Thank you for the review and the Reviewed-by on patch 1.

I will take your suggestion for ufshcd_wl_poweroff(): it will take
host_sem and keep holding it until ufshcd_wl_resume() releases it, so it
behaves like ufshcd_wl_suspend() and the unbalanced up() on the
.restore path goes away too. I will add the Fixes: tag (b294ff3e3449,
which added the WLUN .poweroff callback) and Suggested-by, and put the
patch in front of the one that limits the early link recovery.

For the patch that limits the early link recovery, v7 will:
 - use READ_ONCE() in ufshcd_err_handler() and WRITE_ONCE() in
   ufshcd_wl_runtime_suspend() and ufshcd_wl_runtime_resume();
 - describe the behavior change during shutdown in the commit message:
   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();
 - correct the poweroff statement: the error handler waits for host_sem
   there, it is not stopped by hba->shutting_down.

I will send v7 shortly.

Thanks,

> -----Original Message-----
> From: Bart Van Assche <bvanassche@acm.org>
> Sent: Friday, November 6, 2026 1:50 PM
> To: HOYOUNG SEO <hy50.seo@samsung.com>; linux-scsi@vger.kernel.org; linux-
> kernel@vger.kernel.org; mkp@kernel.org; martin.petersen@oracle.com;
> stanleyjhu@google.com; alim.akhtar@samsung.com; kwangwon.min@samsung.com;
> kwmad.kim@samsung.com; cpgs@samsung.com; h10.kim@samsung.com
> Subject: Re: [PATCH v6 3/3] scsi: ufs: core: Set shutting_down in
> ufshcd_wl_poweroff()
> 
> On 11/5/26 6:15 PM, HOYOUNG SEO wrote:
>  > +	down(&hba->host_sem);
>  > +	hba->shutting_down = true;
>  > +	up(&hba->host_sem);
>  > +
>  >  	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
> 
> I agree with both issues reported by Sashiko:
> - ufs_mtk_system_suspend() is also the .poweroff callback of the
>    MediaTek host controller device and returns -EBUSY if
>    hba->shutting_down has been set. Since the .poweroff callback of the
>    WLUN is called before that of its parent, this patch makes
>    hibernation fail on MediaTek hosts.
> - If hibernation is aborted after the WLUN .poweroff callback has been
>    called, ufshcd_wl_resume() is called as .restore callback.
>    Nothing clears hba->shutting_down, so the error handler and user
>    access (ufshcd_is_user_access_allowed()) remain disabled.
> 
> Additionally, the existing code already has an unbalanced
> up(&hba->host_sem) on that path: ufshcd_wl_poweroff() does not acquire
> host_sem but ufshcd_wl_resume() releases it.
> 
> How about making ufshcd_wl_poweroff() behave like ufshcd_wl_suspend()
> instead, i.e. acquire host_sem and keep holding it until .restore releases
> it? Something like this (untested):
> 
> 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;
> }
> 
> That serializes the error handler against poweroff the same way as for
> system suspend, fixes the unbalanced up() call and leaves
> hba->shutting_down alone. Please also add a Fixes: tag and move this
> patch in front of patch 2/3.
> 
> Thanks,
> 
> Bart.