Re: [PATCH v7 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 1 potential
issue to consider:
- [Medium] scsi: ufs: core: missing error and RPM handling in
  ufshcd_wl_poweroff()
--

commit 9a163b2c91f3c9901825d9d544ee48c174c9b6c6
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(hba, UFS_SHUTDOWN_PM) so that error recovery is
serialized against hibernation poweroff and hba->host_sem remains balanced
if hibernation is aborted and ufshcd_wl_resume() runs.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 5a3ca3b4e6666..c2c34b044c3ec 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -11583,6 +11583,8 @@ static int ufshcd_wl_poweroff(struct device *dev)
>  	struct scsi_device *sdev = to_scsi_device(dev);
>  	struct ufs_hba *hba = shost_priv(sdev->host);
>  
> +	/* Released by ufshcd_wl_resume() if hibernation is aborted. */
> +	down(&hba->host_sem);
>  	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
>  	return 0;
>  }

[Severity: Medium]
What happens if __ufshcd_wl_suspend() fails here or if dev is already
runtime-suspended?

When __ufshcd_wl_suspend() fails, it drops its initial ufshcd_hold(hba)
via ufshcd_release(hba) before returning:

drivers/ufs/core/ufshcd.c:__ufshcd_wl_suspend() {
    ...
	if (ret) {
		ufshcd_update_evt_hist(hba, UFS_EVT_WL_SUSP_ERR, (u32)ret);
		hba->clk_gating.is_suspended = false;
		ufshcd_release(hba);
	}
    ...
}

Unlike ufshcd_wl_suspend(), ufshcd_wl_poweroff() ignores that failure,
leaves hba->host_sem held (blocking ufshcd_err_handler()), and returns 0
so the parent controller suspends while the UFS device and link are still
active.  If hibernation is then aborted, ufshcd_wl_resume() calls
__ufshcd_wl_resume(hba, UFS_SYSTEM_PM), which calls ufshcd_release(hba) a
second time and underflows hba->clk_gating.active_reqs.

In addition, ufshcd_suspend_prepare() leaves ufs_device_wlun
runtime-suspended when ufshcd_rpm_ok_for_spm(hba) is true, but
ufshcd_wl_poweroff() does not check pm_runtime_suspended(dev) (unlike
ufshcd_wl_suspend() and ufshcd_wl_resume()): if the device is in
UFS_SLEEP_PWR_MODE or UIC_LINK_HIBERN8_STATE, __ufshcd_wl_suspend() hits
!ufshcd_is_ufs_dev_active(hba) || !ufshcd_is_link_active(hba), schedules
ufshcd_force_error_recovery(hba) (which blocks on hba->host_sem), and
clears hba->clk_gating.is_suspended; if it is already in
UFS_POWERDOWN_PWR_MODE and UIC_LINK_OFF_STATE, __ufshcd_wl_suspend() takes
a second ufshcd_hold(hba) that ufshcd_wl_resume() never releases because
pm_runtime_suspended(dev) is true.

Finally, ufshcd_wl_poweroff() does not set hba->is_sys_suspended = true on
success like ufshcd_wl_suspend() does, so if __ufshcd_wl_resume() fails
during restore, ufshcd_err_handling_prepare() will skip re-enabling
regulators, clocks, and IRQs.

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