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

["hoyoung seo" <hy50.seo@samsung.com>]

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