Re: [PATCH v9 2/3] scsi: ufs: core: Hold host_sem in ufshcd_wl_poweroff()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] scsi: ufs: core: ufshcd_wl_poweroff() fails on
  runtime-suspended WLUN
- [Medium] scsi: ufs: core: missing is_sys_suspended set in
  ufshcd_wl_poweroff()
- [Medium] scsi: ufs: core: unnormalized return value in
  ufshcd_wl_poweroff()
--

commit adb43cfc03f9f1ca024eaa8ee5c87641ce38cdf8
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() before calling
__ufshcd_wl_suspend() and propagates any error returned by
__ufshcd_wl_suspend().

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index d95b929b282da..688760a98d4b4 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -11586,9 +11586,15 @@ static int ufshcd_wl_poweroff(struct device *dev)
>  {
>  	struct scsi_device *sdev = to_scsi_device(dev);
>  	struct ufs_hba *hba = shost_priv(sdev->host);
> +	int ret;
>  
> -	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
> -	return 0;
> +	/* Released by ufshcd_wl_resume() if hibernation is aborted. */
> +	down(&hba->host_sem);
> +	ret = __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
> +	if (ret)
> +		up(&hba->host_sem);
> +
> +	return ret;
>  }
>  #endif

[Severity: High]
Can ufshcd_wl_poweroff() fail with -EBUSY when hba->ufs_device_wlun is
runtime-suspended?

During hibernation prepare, __ufshcd_suspend_prepare() leaves
ufs_device_wlun runtime-suspended whenever ufshcd_rpm_ok_for_spm(hba) is
true (for example, when hba->rpm_lvl and hba->spm_lvl are both
UFS_PM_LVL_3).  Unlike ufshcd_wl_suspend() (which checks
pm_runtime_suspended(dev)) and ufshcd_wl_shutdown() (which calls
ufshcd_rpm_get_sync(hba) first), ufshcd_wl_poweroff() calls
__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM) directly:

drivers/ufs/core/ufshcd.c:__ufshcd_wl_suspend() {
	...
	/* UFS device & link must be active before we enter in this function */
	if (!ufshcd_is_ufs_dev_active(hba) || !ufshcd_is_link_active(hba)) {
		/*  Wait err handler finish or trigger err recovery */
		if (!ufshcd_eh_in_progress(hba))
			ufshcd_force_error_recovery(hba);
		ret = -EBUSY;
		goto enable_scaling;
	}
	...
}

When the WLUN is runtime-suspended in UFS_SLEEP_PWR_MODE /
UIC_LINK_HIBERN8_STATE, __ufshcd_wl_suspend() triggers error recovery,
clears hba->clk_gating.is_suspended while runtime-suspended, and returns
-EBUSY.  Propagating ret from ufshcd_wl_poweroff() then causes
dpm_suspend_start(PMSG_HIBERNATE) to fail and abort hibernation.

In addition, if the WLUN is runtime-suspended at UFS_PM_LVL_5
(UFS_POWERDOWN_PWR_MODE and UIC_LINK_OFF_STATE), __ufshcd_wl_suspend()
takes a second ufshcd_hold(hba) reference and returns 0 via the
enable_scaling path without releasing it.  If hibernation is later
aborted, ufshcd_wl_resume() skips __ufshcd_wl_resume() because
pm_runtime_suspended(dev) is true, leaking that ufshcd_hold(hba)
reference.

[Severity: Medium]
Should ufshcd_wl_poweroff() also set hba->is_sys_suspended = true when
__ufshcd_wl_suspend() succeeds, matching ufshcd_wl_suspend()?

If hibernation is aborted after ufshcd_wl_poweroff() succeeds and the
subsequent .restore callback (ufshcd_wl_resume()) fails in
__ufshcd_wl_resume(), ufshcd_wl_resume() releases hba->host_sem so
ufshcd_err_handler() can recover the link:

drivers/ufs/core/ufshcd.c:ufshcd_err_handling_prepare() {
	...
	if (pm_runtime_status_suspended(&hba->ufs_device_wlun->sdev_gendev) ||
	    hba->is_sys_suspended) {
		enum ufs_pm_op pm_op;
		...
		pm_op = hba->is_sys_suspended ? UFS_SYSTEM_PM : UFS_RUNTIME_PM;
		ufshcd_vops_resume(hba, pm_op);
	}
	...
}

Because hba->is_sys_suspended was never set to true,
ufshcd_err_handling_prepare() sees both pm_runtime_status_suspended() and
hba->is_sys_suspended as false, skipping regulator, IRQ, and clock
restoration as well as ufshcd_vops_resume(hba, UFS_SYSTEM_PM) during error
recovery.

[Severity: Medium]
Can ufshcd_wl_poweroff() return a positive UIC status code and skip error
recovery when a power mode or link state transition fails?

In __ufshcd_wl_suspend(), the error handling blocks after
ufshcd_set_dev_pwr_mode() and ufshcd_link_state_transition() are guarded
by pm_op != UFS_SHUTDOWN_PM:

drivers/ufs/core/ufshcd.c:__ufshcd_wl_suspend() {
	...
	ret = ufshcd_link_state_transition(hba, req_link_state, check_for_bkops);
	if (ret && pm_op != UFS_SHUTDOWN_PM) {
		ufshcd_force_error_recovery(hba);
		ret = -EBUSY;
	}
	if (ret)
		goto set_dev_active;
	...
}

When ufshcd_uic_pwr_ctrl() fails with a non-PWR_LOCAL status in
ufshcd_get_upmcrs(hba), it skips scheduling eh_work because
hba->pm_op_in_progress is true and returns that positive UPMCRS status
code.  Because ufshcd_wl_poweroff() passes UFS_SHUTDOWN_PM,
__ufshcd_wl_suspend() neither calls ufshcd_force_error_recovery(hba) nor
normalizes ret to -EBUSY, so power_down() in kernel/power/hibernate.c
(which only rolls back on -EBUSY or -EAGAIN) does not roll back
hibernation cleanly and error recovery is not scheduled if hibernation
aborts.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1582071873.61794277203231.JavaMail.epsvc@epcpadp1new?part=2