[PATCH v2 0/2] scsi: ufs: core: Avoid redundant link recovery during system resume

[PATCH v2 0/2] scsi: ufs: core: Avoid redundant link recovery during system resume

[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 makes ufshcd_err_handler() wait for host_sem instead of
recovering the link early when the hba is system suspended.

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.

HOYOUNG SEO (2):
  scsi: ufs: core: Clear saved errors in ufshcd_link_recovery
  scsi: ufs: core: add is_sys_suspended state before recovery

 drivers/ufs/core/ufshcd.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] 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.

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 bcc2512bf34b..74dfbb98129e 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4690,10 +4690,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);
@@ -4704,8 +4714,11 @@ int ufshcd_link_recovery(struct ufs_hba *hba)
 	ret = ufshcd_host_reset_and_restore(hba);
 
 	spin_lock_irqsave(&hba->host->host_lock, flags);
-	if (ret)
+	if (ret) {
 		hba->ufshcd_state = UFSHCD_STATE_ERROR;
+		hba->saved_err |= saved_err;
+		hba->saved_uic_err |= saved_uic_err;
+	}
 	ufshcd_clear_eh_in_progress(hba);
 	spin_unlock_irqrestore(&hba->host->host_lock, flags);
 
-- 
2.34.1

[PATCH v2 2/2] scsi: ufs: core: add is_sys_suspended state before recovery

[HOYOUNG SEO]

If a LINERESET is reported and the hibern8 exit fails in
__ufshcd_wl_resume() during system resume, the link is recovered twice:

  - the UIC error interrupt schedules eh_work, and ufshcd_err_handler()
    calls ufshcd_link_recovery() because hba->pm_op_in_progress is set;
  - __ufshcd_wl_resume() calls ufshcd_link_recovery() itself after the
    hibern8 exit failure.

Since the early recovery path in ufshcd_err_handler() does not take
hba->host_sem, the two recoveries are not serialized and may reset the
host and the device concurrently.

During system resume, hba->host_sem is held from ufshcd_wl_suspend()
until ufshcd_wl_resume() returns, so the error handler can simply wait
for it. Skip the early link recovery when hba->is_sys_suspended is set.
With the previous patch, the errors already recovered by the resume path
are cleared, so ufshcd_err_handler() stops once host_sem is released
unless new errors occurred. If the resume failed, is_sys_suspended stays
set and the error handler performs a full recovery as usual.

The early link recovery is kept for runtime PM, where host_sem is not
held and the error handler cannot wait for the PM operation to finish.

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

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 74dfbb98129e..d085d394858f 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -7053,9 +7053,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.
+		 *
+		 * During system resume, host_sem is held from
+		 * ufshcd_wl_suspend() until ufshcd_wl_resume() returns, so wait
+		 * for it below instead of recovering the link concurrently with
+		 * the resume path.
 		 */
 		ufshcd_rpm_get_noresume(hba);
-		if (hba->pm_op_in_progress) {
+		if (hba->pm_op_in_progress && !hba->is_sys_suspended) {
 			ufshcd_link_recovery(hba);
 			ufshcd_rpm_put(hba);
 			return;
-- 
2.34.1

Re: [PATCH v2 2/2] scsi: ufs: core: add is_sys_suspended state before recovery

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: ufs: core: ufshcd_err_handler() skips recovery after failed resume
--

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

scsi: ufs: core: add is_sys_suspended state before recovery

This commit skips the early link recovery in ufshcd_err_handler() when
hba->is_sys_suspended is set so that error recovery waits on hba->host_sem
instead of racing with __ufshcd_wl_resume() during system resume.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 0e6e593c24cfd..4b4d473530b16 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -7052,9 +7052,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.
> +		 *
> +		 * During system resume, host_sem is held from
> +		 * ufshcd_wl_suspend() until ufshcd_wl_resume() returns, so wait
> +		 * for it below instead of recovering the link concurrently with
> +		 * the resume path.
>  		 */
>  		ufshcd_rpm_get_noresume(hba);
> -		if (hba->pm_op_in_progress) {
> +		if (hba->pm_op_in_progress && !hba->is_sys_suspended) {

[Severity: High]
The commit message notes that if system resume fails, hba->is_sys_suspended
stays set and ufshcd_err_handler() performs a full recovery as usual.
However, when ufshcd_err_handler() wakes from down(&hba->host_sem) after
ufshcd_wl_resume() -> __ufshcd_wl_resume() fails, it immediately checks
ufshcd_err_handling_should_stop():

drivers/ufs/core/ufshcd.c:ufshcd_err_handler() {
	down(&hba->host_sem);
	spin_lock_irqsave(&hba->host->host_lock, flags);
	if (ufshcd_err_handling_should_stop(hba)) {
		if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
			hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
		spin_unlock_irqrestore(&hba->host->host_lock, flags);
		up(&hba->host_sem);
		return;
	}
}

where ufshcd_err_handling_should_stop() is defined as:

drivers/ufs/core/ufshcd.c:ufshcd_err_handling_should_stop() {
	return (!hba->is_powered || hba->shutting_down ||
		!hba->ufs_device_wlun ||
		hba->ufshcd_state == UFSHCD_STATE_ERROR ||
		(!(hba->saved_err || hba->saved_uic_err || hba->force_reset ||
		   ufshcd_is_link_broken(hba))));
}

If ufshcd_uic_hibern8_exit() fails in __ufshcd_wl_resume() and the subsequent
call to ufshcd_link_recovery() also fails, ufshcd_link_recovery() sets
hba->ufshcd_state = UFSHCD_STATE_ERROR even though it restores hba->saved_err
and hba->saved_uic_err:

drivers/ufs/core/ufshcd.c:ufshcd_link_recovery() {
	...
	spin_lock_irqsave(&hba->host->host_lock, flags);
	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);
}

Alternatively, if ufshcd_link_recovery() succeeds in __ufshcd_wl_resume()
(clearing hba->saved_err, hba->saved_uic_err, and hba->force_reset) and a
later step in __ufshcd_wl_resume() such as ufshcd_set_dev_pwr_mode() fails,
the saved error flags remain zero.

In both cases, doesn't ufshcd_err_handling_should_stop() evaluate to true and
cause ufshcd_err_handler() to return immediately after acquiring
hba->host_sem, skipping full recovery and ufshcd_recover_pm_error() while
leaving the HBA in UFSHCD_STATE_ERROR or stuck with hba->is_sys_suspended
set?

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

RE: [PATCH v2 2/2] scsi: ufs: core: add is_sys_suspended state before recovery

[hoyoung seo]

Hi,
The commit message is too broad here: the error handler does not
always run a full recovery after a failed resume. It stops when
ufshcd_err_handling_should_stop() says so, and I will fix the wording in
v3. The behavior in the two cases you describe is the same as without
this patch, though:

- ufshcd_link_recovery() failed in __ufshcd_wl_resume(): the HBA is in
  UFSHCD_STATE_ERROR, which the error handler treats as final, and
  ufshcd_schedule_eh_work() does not queue it in that state. The same
  happens today when eh_work runs after the PM operation has finished.
  The early ufshcd_link_recovery() call in the error handler is the same
  operation that has just failed in the resume path, and it ran before the
  resume path's own recovery, not as a retry after it.

- ufshcd_link_recovery() succeeded and a later step failed: nothing is
  pending, so the error handler has nothing to recover. Without this
  patch, its early path would only have called ufshcd_link_recovery()
  once more; it never ran the full reset or ufshcd_recover_pm_error().

If the resume fails without having done a recovery itself (e.g. the SSU
command is failed with DID_BAD_TARGET and force_reset is set in
ufshcd_queuecommand()), nothing clears saved_err/force_reset, so the
error handler does run the full reset and ufshcd_recover_pm_error() once
host_sem is released, as intended by ufshcd_err_handling_prepare().

v3 will reword the last paragraph of the commit message accordingly.

Thanks,
Hoyoung


> -----Original Message-----
> From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
> Sent: Friday, October 23, 2026 9:00 PM
> To: HOYOUNG SEO <hy50.seo@samsung.com>; linux-scsi@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] scsi: ufs: core: add is_sys_suspended state
> before recovery
> 
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [High] scsi: ufs: core: ufshcd_err_handler() skips recovery after failed
> resume
> --
> 
> commit c53108b377c62c7e01a8b52d302e15dfdce25f17
> Author: HOYOUNG SEO <hy50.seo@samsung.com>
> 
> scsi: ufs: core: add is_sys_suspended state before recovery
> 
> This commit skips the early link recovery in ufshcd_err_handler() when
> hba->is_sys_suspended is set so that error recovery waits on
> hba->hba->host_sem
> instead of racing with __ufshcd_wl_resume() during system resume.
> 
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index 0e6e593c24cfd..4b4d473530b16 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -7052,9 +7052,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.
> > +		 *
> > +		 * During system resume, host_sem is held from
> > +		 * ufshcd_wl_suspend() until ufshcd_wl_resume() returns, so
> wait
> > +		 * for it below instead of recovering the link concurrently
> with
> > +		 * the resume path.
> >  		 */
> >  		ufshcd_rpm_get_noresume(hba);
> > -		if (hba->pm_op_in_progress) {
> > +		if (hba->pm_op_in_progress && !hba->is_sys_suspended) {
> 
> [Severity: High]
> The commit message notes that if system resume fails, hba-
> >is_sys_suspended stays set and ufshcd_err_handler() performs a full
> recovery as usual.
> However, when ufshcd_err_handler() wakes from down(&hba->host_sem) after
> ufshcd_wl_resume() -> __ufshcd_wl_resume() fails, it immediately checks
> ufshcd_err_handling_should_stop():
> 
> drivers/ufs/core/ufshcd.c:ufshcd_err_handler() {
> 	down(&hba->host_sem);
> 	spin_lock_irqsave(&hba->host->host_lock, flags);
> 	if (ufshcd_err_handling_should_stop(hba)) {
> 		if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
> 			hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
> 		spin_unlock_irqrestore(&hba->host->host_lock, flags);
> 		up(&hba->host_sem);
> 		return;
> 	}
> }
> 
> where ufshcd_err_handling_should_stop() is defined as:
> 
> drivers/ufs/core/ufshcd.c:ufshcd_err_handling_should_stop() {
> 	return (!hba->is_powered || hba->shutting_down ||
> 		!hba->ufs_device_wlun ||
> 		hba->ufshcd_state == UFSHCD_STATE_ERROR ||
> 		(!(hba->saved_err || hba->saved_uic_err || hba->force_reset
> ||
> 		   ufshcd_is_link_broken(hba))));
> }
> 
> If ufshcd_uic_hibern8_exit() fails in __ufshcd_wl_resume() and the
> subsequent call to ufshcd_link_recovery() also fails,
> ufshcd_link_recovery() sets
> hba->ufshcd_state = UFSHCD_STATE_ERROR even though it restores
> hba->hba->saved_err
> and hba->saved_uic_err:
> 
> drivers/ufs/core/ufshcd.c:ufshcd_link_recovery() {
> 	...
> 	spin_lock_irqsave(&hba->host->host_lock, flags);
> 	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); }
> 
> Alternatively, if ufshcd_link_recovery() succeeds in __ufshcd_wl_resume()
> (clearing hba->saved_err, hba->saved_uic_err, and hba->force_reset) and a
> later step in __ufshcd_wl_resume() such as ufshcd_set_dev_pwr_mode() fails,
> the saved error flags remain zero.
> 
> In both cases, doesn't ufshcd_err_handling_should_stop() evaluate to true
> and cause ufshcd_err_handler() to return immediately after acquiring
> hba->host_sem, skipping full recovery and ufshcd_recover_pm_error()
> hba->while
> leaving the HBA in UFSHCD_STATE_ERROR or stuck with hba->is_sys_suspended
> set?
> 
> --
> Sashiko AI review · https://protect2.fireeye.com/v1/url?k=944015f7-
> cbdc3cdd-94419eb8-000babe598f7-c051d8d5cf854368&q=1&e=01b7134f-5807-4f86-
> 9107-
> f67a15a683c0&u=https%3A%2F%2Fsashiko.dev%2F%23%2Fpatchset%2Fcover.17927550
> 91.git.hy50.seo%40samsung.com%3Fpart%3D2