Re: [PATCH v8 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:
- [High] scsi: ufs: core: unhandled __ufshcd_wl_suspend() error in
  ufshcd_wl_poweroff()
--

commit 3da1cf6f30fec549f078cc8184c250d266766b37
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() so that
hibernation poweroff serializes with the error handler and balances the
up(&hba->host_sem) in ufshcd_wl_resume() if hibernation is aborted.

> 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: High]
Does ignoring the return value of __ufshcd_wl_suspend() here leave
hba->host_sem held when __ufshcd_wl_suspend() fails?

drivers/ufs/core/ufshcd.c:ufshcd_wl_suspend() {
	...
	ret = __ufshcd_wl_suspend(hba, UFS_SYSTEM_PM);
	if (ret) {
		dev_err(&sdev->sdev_gendev, "%s failed: %d\n", __func__,  ret);
		up(&hba->host_sem);
	}

out:
	if (!ret)
		hba->is_sys_suspended = true;
	return ret;
}

If __ufshcd_wl_suspend() fails (for example, when the device or link is not
active and it schedules hba->eh_work via ufshcd_force_error_recovery()),
ufshcd_err_handler() will block forever on down(&hba->host_sem).  Because
__ufshcd_wl_suspend() already calls ufshcd_release(hba) on error while
ufshcd_wl_poweroff() still returns 0, aborting hibernation later also causes
ufshcd_wl_resume() -> __ufshcd_wl_resume() to call ufshcd_release(hba) a
second time and underflow hba->clk_gating.active_reqs.

In addition, ufshcd_wl_poweroff() does not set hba->is_sys_suspended = true
on success, so if .restore (ufshcd_wl_resume()) fails,
ufshcd_err_handling_prepare() will skip re-enabling regulators, clocks, and
IRQs.

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