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