Re: [PATCH 1/2] scsi: ufs: core: Skip redundant variant resume after error recovery power-up

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: ufs: core: W-LUN left RPM_SUSPENDED after error recovery reset
--

commit 2e7bb517d17be41f390aa16033a12c2fb8953c6f
Author: Stanley Jhu <stanleyjhu@google.com>

scsi: ufs: core: Skip redundant variant resume after error recovery power-up

This commit tracks POST_CHANGE variant suspend state in
hba->is_vops_suspended so ufshcd_vops_resume() is skipped if the variant is
already resumed when ufs_device_wlun remains RPM_SUSPENDED across error
recovery.

> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
> index e55c2a02c1f50..9e4d8174a0f08 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -236,20 +236,31 @@ static inline void ufshcd_vops_fixup_dev_quirks(struct ufs_hba *hba)
>  }
>  
>  static inline int ufshcd_vops_suspend(struct ufs_hba *hba, enum ufs_pm_op op,
> -				enum ufs_notify_change_status status)
> +				      enum ufs_notify_change_status status)
>  {
> +	int ret = 0;
> +
>  	if (hba->vops && hba->vops->suspend)
> -		return hba->vops->suspend(hba, op, status);
> +		ret = hba->vops->suspend(hba, op, status);
> +	if (!ret && status == POST_CHANGE)
> +		hba->is_vops_suspended = true;
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static inline int ufshcd_vops_resume(struct ufs_hba *hba, enum ufs_pm_op op)
>  {
> +	int ret = 0;
> +
> +	if (!hba->is_vops_suspended)
> +		return 0;

[Severity: High]
Does skipping ufshcd_vops_resume() here leave ufs_device_wlun out of sync
with the actual hardware and clock-gating state when error recovery resets
and restores the host after a failed W-LUN runtime resume?

When __ufshcd_wl_suspend() suspends the W-LUN, it calls ufshcd_hold(hba)
(incrementing hba->clk_gating.active_reqs) and sets
hba->clk_gating.is_suspended = true. If a subsequent __ufshcd_wl_resume()
fails (for example, in ufshcd_set_dev_pwr_mode()), it rolls back to
vendor_suspend: and exits through out:, which clears is_suspended and drops
the clock-gating hold without a matching ufshcd_hold(hba) on future resume
attempts:

drivers/ufs/core/ufshcd.c:__ufshcd_wl_resume() {
	...
vendor_suspend:
	ufshcd_vops_suspend(hba, pm_op, POST_CHANGE);
out:
	if (ret)
		ufshcd_update_evt_hist(hba, UFS_EVT_WL_RES_ERR, (u32)ret);
	hba->clk_gating.is_suspended = false;
	ufshcd_release(hba);
	hba->pm_op_in_progress = false;
	return ret;
}

Because commit eef1d74bf88f ("PM: runtime: Only set runtime_error on
suspend callback failures") leaves ufs_device_wlun->sdev_gendev in
RPM_SUSPENDED with power.runtime_error == 0, every subsequent call to
__ufshcd_wl_resume() without an intervening __ufshcd_wl_suspend() (first
from ufshcd_err_handling_prepare() -> ufshcd_rpm_get_sync(), and again on
the next runtime resume after recovery) calls ufshcd_release(hba) without
a matching ufshcd_hold(hba), underflowing hba->clk_gating.active_reqs.

Furthermore, during ufshcd_err_handler(), ufshcd_err_handling_prepare()
powers on the regulators, clocks, and variant, and ufshcd_reset_and_restore()
restores the controller, setting hba->uic_link_state = UIC_LINK_ACTIVE_STATE
and hba->curr_dev_pwr_mode = UFS_ACTIVE_PWR_MODE. However,
ufshcd_recover_pm_error() only marks ufs_device_wlun active if
power.runtime_error is set:

drivers/ufs/core/ufshcd.c:ufshcd_recover_pm_error() {
	...
	if (hba->ufs_device_wlun->sdev_gendev.power.runtime_error) {
		pm_runtime_get_sync(&starget->dev);
		pm_runtime_set_active(&hba->ufs_device_wlun->sdev_gendev);
		pm_runtime_put_sync(&starget->dev);
		resume_sdev_queues = true;
	}
	...
}

With power.runtime_error == 0, ufs_device_wlun, starget->dev, and
shost->shost_gendev remain in RPM_SUSPENDED (and consumer SCSI device
queues are not resumed here, though a later commit wakes consumer queues).
When ufshcd_err_handling_unprepare() then finishes:

drivers/ufs/core/ufshcd.c:ufshcd_err_handling_unprepare() {
	...
	if (ufshcd_is_clkscaling_supported(hba))
		ufshcd_clk_scaling_suspend(hba, false);
	ufshcd_rpm_put(hba);
	pm_runtime_put(hba->dev);
}

ufshcd_rpm_put(hba) will not run __ufshcd_wl_suspend() because
ufs_device_wlun is already RPM_SUSPENDED, and pm_runtime_put(hba->dev) can
runtime suspend hba->dev via ufshcd_suspend() while the UFS device is in
UFS_ACTIVE_PWR_MODE, the UniPro link is in UIC_LINK_ACTIVE_STATE, devfreq
clock scaling is active, and the variant is resumed.

Should ufshcd_recover_pm_error() instead transition ufs_device_wlun back to
RPM_ACTIVE when ufshcd_reset_and_restore() has powered up the device and
link, rather than leaving ufs_device_wlun in RPM_SUSPENDED with the hardware
and variant resumed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105071518.1458428-1-stanleyjhu@google.com?part=1