Linux SCSI subsystem development
 help / color / mirror / Atom feed
* [PATCH v8 1/3] scsi: ufs: core: Clear saved errors in ufshcd_link_recovery()
       [not found] ` <cover.1794196863.git.hy50.seo@samsung.com>
@ 2026-11-09  4:09   ` HOYOUNG SEO
  2026-11-09 15:28     ` Bean Huo
  2026-11-09  4:09   ` [PATCH v8 2/3] scsi: ufs: core: Hold host_sem in ufshcd_wl_poweroff() HOYOUNG SEO
  2026-11-09  4:09   ` [PATCH v8 3/3] scsi: ufs: core: Limit early link recovery in the error handler to runtime PM HOYOUNG SEO
  2 siblings, 1 reply; 11+ messages in thread
From: HOYOUNG SEO @ 2026-11-09  4:09 UTC (permalink / raw)
  To: linux-scsi, linux-kernel, bvanassche, mkp, martin.petersen,
	stanleyjhu, peter.wang, alim.akhtar, kwangwon.min, kwmad.kim,
	cpgs, h10.kim
  Cc: 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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v8 2/3] scsi: ufs: core: Hold host_sem in ufshcd_wl_poweroff()
       [not found] ` <cover.1794196863.git.hy50.seo@samsung.com>
  2026-11-09  4:09   ` [PATCH v8 1/3] scsi: ufs: core: Clear saved errors in ufshcd_link_recovery() HOYOUNG SEO
@ 2026-11-09  4:09   ` HOYOUNG SEO
  2026-11-09  5:18     ` sashiko-bot
  2026-11-09  9:34     ` Peter Wang
  2026-11-09  4:09   ` [PATCH v8 3/3] scsi: ufs: core: Limit early link recovery in the error handler to runtime PM HOYOUNG SEO
  2 siblings, 2 replies; 11+ messages in thread
From: HOYOUNG SEO @ 2026-11-09  4:09 UTC (permalink / raw)
  To: linux-scsi, linux-kernel, bvanassche, mkp, martin.petersen,
	stanleyjhu, peter.wang, alim.akhtar, kwangwon.min, kwmad.kim,
	cpgs, h10.kim
  Cc: 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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v8 3/3] scsi: ufs: core: Limit early link recovery in the error handler to runtime PM
       [not found] ` <cover.1794196863.git.hy50.seo@samsung.com>
  2026-11-09  4:09   ` [PATCH v8 1/3] scsi: ufs: core: Clear saved errors in ufshcd_link_recovery() HOYOUNG SEO
  2026-11-09  4:09   ` [PATCH v8 2/3] scsi: ufs: core: Hold host_sem in ufshcd_wl_poweroff() HOYOUNG SEO
@ 2026-11-09  4:09   ` HOYOUNG SEO
  2026-11-09  5:21     ` sashiko-bot
                       ` (2 more replies)
  2 siblings, 3 replies; 11+ messages in thread
From: HOYOUNG SEO @ 2026-11-09  4:09 UTC (permalink / raw)
  To: linux-scsi, linux-kernel, bvanassche, mkp, martin.petersen,
	stanleyjhu, peter.wang, alim.akhtar, kwangwon.min, kwmad.kim,
	cpgs, h10.kim
  Cc: 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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v8 2/3] scsi: ufs: core: Hold host_sem in ufshcd_wl_poweroff()
  2026-11-09  4:09   ` [PATCH v8 2/3] scsi: ufs: core: Hold host_sem in ufshcd_wl_poweroff() HOYOUNG SEO
@ 2026-11-09  5:18     ` sashiko-bot
  2026-11-09  9:34     ` Peter Wang
  1 sibling, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-11-09  5:18 UTC (permalink / raw)
  To: HOYOUNG SEO, linux-scsi

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] scsi: ufs: core: unhandled __ufshcd_wl_suspend() error in
  ufshcd_wl_poweroff()
--

commit 3da1cf6f30fec549f078cc8184c250d266766b37
Author: HOYOUNG SEO <hy50.seo@samsung.com>

scsi: ufs: core: Hold host_sem in ufshcd_wl_poweroff()

This commit acquires hba->host_sem in ufshcd_wl_poweroff() so that
hibernation poweroff serializes with the error handler and balances the
up(&hba->host_sem) in ufshcd_wl_resume() if hibernation is aborted.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 5a3ca3b4e6666..c2c34b044c3ec 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -11583,6 +11583,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;
>  }

[Severity: High]
Does ignoring the return value of __ufshcd_wl_suspend() here leave
hba->host_sem held when __ufshcd_wl_suspend() fails?

drivers/ufs/core/ufshcd.c:ufshcd_wl_suspend() {
	...
	ret = __ufshcd_wl_suspend(hba, UFS_SYSTEM_PM);
	if (ret) {
		dev_err(&sdev->sdev_gendev, "%s failed: %d\n", __func__,  ret);
		up(&hba->host_sem);
	}

out:
	if (!ret)
		hba->is_sys_suspended = true;
	return ret;
}

If __ufshcd_wl_suspend() fails (for example, when the device or link is not
active and it schedules hba->eh_work via ufshcd_force_error_recovery()),
ufshcd_err_handler() will block forever on down(&hba->host_sem).  Because
__ufshcd_wl_suspend() already calls ufshcd_release(hba) on error while
ufshcd_wl_poweroff() still returns 0, aborting hibernation later also causes
ufshcd_wl_resume() -> __ufshcd_wl_resume() to call ufshcd_release(hba) a
second time and underflow hba->clk_gating.active_reqs.

In addition, ufshcd_wl_poweroff() does not set hba->is_sys_suspended = true
on success, so if .restore (ufshcd_wl_resume()) fails,
ufshcd_err_handling_prepare() will skip re-enabling regulators, clocks, and
IRQs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/728258269.41794200283102.JavaMail.epsvc@epcpadp2new?part=2

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v8 3/3] scsi: ufs: core: Limit early link recovery in the error handler to runtime PM
  2026-11-09  4:09   ` [PATCH v8 3/3] scsi: ufs: core: Limit early link recovery in the error handler to runtime PM HOYOUNG SEO
@ 2026-11-09  5:21     ` sashiko-bot
  2026-11-09  9:32     ` Peter Wang
  2026-11-09 17:08     ` Bean Huo
  2 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-11-09  5:21 UTC (permalink / raw)
  To: HOYOUNG SEO, linux-scsi

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] scsi: ufs: core: unsynchronized link recovery in
  ufshcd_err_handler()
--

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

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

This commit replaces the hba->pm_op_in_progress check in
ufshcd_err_handler() with ufshcd_rpm_transition_in_progress() so that early
link recovery without hba->host_sem only runs when the WLUN runtime PM
status is RPM_SUSPENDING or RPM_RESUMING.

> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
> index 99f654b79035f..c576f648f242d 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -418,6 +418,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 c2c34b044c3ec..1608a856cac71 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -7094,9 +7094,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;

[Severity: High]
Does checking runtime_status here widen the window for unsynchronized
ufshcd_link_recovery() outside the ufshcd_hold() region of
__ufshcd_wl_suspend() and __ufshcd_wl_resume()?

The runtime PM core sets runtime_status to RPM_SUSPENDING before
sdev_runtime_suspend() runs blk_pre_runtime_suspend(), and keeps
RPM_SUSPENDING or RPM_RESUMING until after sdev_runtime_suspend() or
sdev_runtime_resume() returns.  That window spans before
__ufshcd_wl_suspend() calls ufshcd_hold() and after __ufshcd_wl_suspend() or
__ufshcd_wl_resume() calls ufshcd_release() and clears
hba->pm_op_in_progress:

CPU0                                  CPU1 (eh_wq)
rpm_suspend()
  runtime_status = RPM_SUSPENDING
  ...
  __ufshcd_wl_suspend()
    ufshcd_hold(hba)
    ufshcd_link_state_transition()
      /* fails */
    ufshcd_force_error_recovery(hba)
      queue_work(..., &hba->eh_work)
    ...
    ufshcd_release(hba)
    hba->pm_op_in_progress = false
                                      ufshcd_err_handler()
                                        ufshcd_rpm_transition_in_progress()
                                          /* still RPM_SUSPENDING */
                                        ufshcd_link_recovery(hba)
                                          /* runs without ufshcd_hold() */
  blk_post_runtime_suspend()
  runtime_status = RPM_ACTIVE

Because ufshcd_link_recovery() runs in this window without ufshcd_hold(),
hba->host_sem, or ufshcd_err_handling_prepare(), it can access controller
MMIO registers while clocks are gated off (for example, during
blk_pre_runtime_suspend() or after ufshcd_release()), or reset and activate
the host and link after a successful __ufshcd_wl_suspend() right before
rpm_suspend() marks the WLUN RPM_SUSPENDED.

Also, during runtime PM itself, if a UIC error schedules eh_work while
__ufshcd_wl_resume() fails ufshcd_uic_hibern8_exit(), or while
__ufshcd_wl_suspend() rolls back at set_dev_active, ufshcd_err_handler() and
the runtime PM callback still run ufshcd_link_recovery() concurrently.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/728258269.41794200283102.JavaMail.epsvc@epcpadp2new?part=3

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v8 3/3] scsi: ufs: core: Limit early link recovery in the error handler to runtime PM
  2026-11-09  4:09   ` [PATCH v8 3/3] scsi: ufs: core: Limit early link recovery in the error handler to runtime PM HOYOUNG SEO
  2026-11-09  5:21     ` sashiko-bot
@ 2026-11-09  9:32     ` Peter Wang
  2026-11-09 17:08     ` Bean Huo
  2 siblings, 0 replies; 11+ messages in thread
From: Peter Wang @ 2026-11-09  9:32 UTC (permalink / raw)
  To: HOYOUNG SEO, linux-scsi, linux-kernel, bvanassche, mkp,
	martin.petersen, stanleyjhu, alim.akhtar, kwangwon.min, kwmad.kim,
	cpgs, h10.kim

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>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v8 2/3] scsi: ufs: core: Hold host_sem in ufshcd_wl_poweroff()
  2026-11-09  4:09   ` [PATCH v8 2/3] scsi: ufs: core: Hold host_sem in ufshcd_wl_poweroff() HOYOUNG SEO
  2026-11-09  5:18     ` sashiko-bot
@ 2026-11-09  9:34     ` Peter Wang
  2026-11-09 18:19       ` Bart Van Assche
  1 sibling, 1 reply; 11+ messages in thread
From: Peter Wang @ 2026-11-09  9:34 UTC (permalink / raw)
  To: HOYOUNG SEO, linux-scsi, linux-kernel, bvanassche, mkp,
	martin.petersen, stanleyjhu, alim.akhtar, kwangwon.min, kwmad.kim,
	cpgs, h10.kim

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v8 1/3] scsi: ufs: core: Clear saved errors in ufshcd_link_recovery()
  2026-11-09  4:09   ` [PATCH v8 1/3] scsi: ufs: core: Clear saved errors in ufshcd_link_recovery() HOYOUNG SEO
@ 2026-11-09 15:28     ` Bean Huo
  0 siblings, 0 replies; 11+ messages in thread
From: Bean Huo @ 2026-11-09 15:28 UTC (permalink / raw)
  To: HOYOUNG SEO, linux-scsi, linux-kernel, bvanassche, mkp,
	martin.petersen, stanleyjhu, peter.wang, alim.akhtar,
	kwangwon.min, kwmad.kim, cpgs, h10.kim


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v8 3/3] scsi: ufs: core: Limit early link recovery in the error handler to runtime PM
  2026-11-09  4:09   ` [PATCH v8 3/3] scsi: ufs: core: Limit early link recovery in the error handler to runtime PM HOYOUNG SEO
  2026-11-09  5:21     ` sashiko-bot
  2026-11-09  9:32     ` Peter Wang
@ 2026-11-09 17:08     ` Bean Huo
  2026-11-09 18:20       ` Bart Van Assche
  2 siblings, 1 reply; 11+ messages in thread
From: Bean Huo @ 2026-11-09 17:08 UTC (permalink / raw)
  To: HOYOUNG SEO, linux-scsi, linux-kernel, bvanassche, mkp,
	martin.petersen, stanleyjhu, peter.wang, alim.akhtar,
	kwangwon.min, kwmad.kim, cpgs, h10.kim

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v8 2/3] scsi: ufs: core: Hold host_sem in ufshcd_wl_poweroff()
  2026-11-09  9:34     ` Peter Wang
@ 2026-11-09 18:19       ` Bart Van Assche
  0 siblings, 0 replies; 11+ messages in thread
From: Bart Van Assche @ 2026-11-09 18:19 UTC (permalink / raw)
  To: Peter Wang, HOYOUNG SEO, linux-scsi, linux-kernel, mkp,
	martin.petersen, stanleyjhu, alim.akhtar, kwangwon.min, kwmad.kim,
	cpgs, h10.kim

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v8 3/3] scsi: ufs: core: Limit early link recovery in the error handler to runtime PM
  2026-11-09 17:08     ` Bean Huo
@ 2026-11-09 18:20       ` Bart Van Assche
  0 siblings, 0 replies; 11+ messages in thread
From: Bart Van Assche @ 2026-11-09 18:20 UTC (permalink / raw)
  To: Bean Huo, HOYOUNG SEO, linux-scsi, linux-kernel, mkp,
	martin.petersen, stanleyjhu, peter.wang, alim.akhtar,
	kwangwon.min, kwmad.kim, cpgs, h10.kim

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2026-11-09 18:20 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <CGME20261109041346epcas2p403d660b62bc9f227fef868e8b19816af@epcas2p4.samsung.com>
     [not found] ` <cover.1794196863.git.hy50.seo@samsung.com>
2026-11-09  4:09   ` [PATCH v8 1/3] scsi: ufs: core: Clear saved errors in ufshcd_link_recovery() HOYOUNG SEO
2026-11-09 15:28     ` Bean Huo
2026-11-09  4:09   ` [PATCH v8 2/3] scsi: ufs: core: Hold host_sem in ufshcd_wl_poweroff() HOYOUNG SEO
2026-11-09  5:18     ` sashiko-bot
2026-11-09  9:34     ` Peter Wang
2026-11-09 18:19       ` Bart Van Assche
2026-11-09  4:09   ` [PATCH v8 3/3] scsi: ufs: core: Limit early link recovery in the error handler to runtime PM HOYOUNG SEO
2026-11-09  5:21     ` sashiko-bot
2026-11-09  9:32     ` Peter Wang
2026-11-09 17:08     ` Bean Huo
2026-11-09 18:20       ` Bart Van Assche

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox