[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

[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()

[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.