Re: [PATCH v1] scsi: ufs: core: add is_sys_suspended state before recovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: ufs: ufshcd_err_handler() still runs recovery after ufshcd_wl_resume()
--

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

scsi: ufs: core: add is_sys_suspended state before recovery

This commit checks !hba->is_sys_suspended before calling
ufshcd_link_recovery() in ufshcd_err_handler() to avoid redundant link
recovery when a linereset and hibern8_exit failure occur during
ufshcd_wl_resume().

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index cc75384c65021..988cb127ef0e8 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -7041,7 +7041,7 @@ static void ufshcd_err_handler(struct work_struct *work)
>  		 * is already in progress.
>  		 */
>  		ufshcd_rpm_get_noresume(hba);
> -		if (hba->pm_op_in_progress) {
> +		if (hba->pm_op_in_progress && !hba->is_sys_suspended) {

[Severity: Medium]
Does skipping the early return here actually prevent ufshcd_err_handler()
from running recovery after ufshcd_wl_resume() finishes?

When a linereset or UIC error and hibern8_exit failure occur during
ufshcd_wl_resume() -> __ufshcd_wl_resume(), the interrupt handler latches the
error bits into hba->saved_err and hba->saved_uic_err and schedules
hba->eh_work.

In drivers/ufs/core/ufshcd.c, __ufshcd_wl_resume() then calls
ufshcd_link_recovery():

	if (ufshcd_is_link_hibern8(hba)) {
		ret = ufshcd_uic_hibern8_exit(hba);
		if (!ret) {
			ufshcd_set_link_active(hba);
		} else {
			dev_err(hba->dev, "%s: hibern8 exit failed %d\n",
					__func__, ret);
			ret = ufshcd_link_recovery(hba);
			if (ret)
				goto vendor_suspend;
		}
	}

Unlike ufshcd_reset_and_restore(), neither ufshcd_link_recovery() nor
ufshcd_host_reset_and_restore() clears hba->saved_err, hba->saved_uic_err, or
hba->force_reset.

With this change, ufshcd_err_handler() sees hba->is_sys_suspended is true,
skips the early return block, and waits on down(&hba->host_sem):

		ufshcd_rpm_get_noresume(hba);
		if (hba->pm_op_in_progress && !hba->is_sys_suspended) {
			ufshcd_link_recovery(hba);
			ufshcd_rpm_put(hba);
			return;
		}
		ufshcd_rpm_put(hba);
	}

	down(&hba->host_sem);
	spin_lock_irqsave(&hba->host->host_lock, flags);
	if (ufshcd_err_handling_should_stop(hba)) {

Once ufshcd_wl_resume() releases hba->host_sem, ufshcd_err_handler() wakes up
and checks ufshcd_err_handling_should_stop():

static inline bool ufshcd_err_handling_should_stop(struct ufs_hba *hba)
{
	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))));
}

Because hba->saved_err and hba->saved_uic_err are still non-zero after
ufshcd_link_recovery(), ufshcd_err_handling_should_stop() returns false.

Wouldn't ufshcd_err_handler() then proceed into ufshcd_err_handling_prepare()
to quiesce all SCSI queues and run redundant error recovery (such as
ufshcd_reset_and_restore() when UFSHCD_UIC_HIBERN8_MASK or a fatal/DL UIC
error was latched, or ufshcd_abort_all()) on the link that __ufshcd_wl_resume()
already recovered?

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