Re: [PATCH v6 3/3] scsi: ufs: core: Set shutting_down in ufshcd_wl_poweroff()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: ufs: core: ufshcd_wl_poweroff() breaks ufs_mtk_system_suspend()
- [High] scsi: ufs: core: ufshcd_wl_poweroff() corrupts state on restore rollback
--

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

scsi: ufs: core: Set shutting_down in ufshcd_wl_poweroff()

This commit sets hba->shutting_down under hba->host_sem in
ufshcd_wl_poweroff() before calling __ufshcd_wl_suspend() so that the error
handler does not attempt link recovery during hibernation poweroff.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 8571e91263e51..e59cdc224ef96 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -11592,6 +11592,10 @@ static int ufshcd_wl_poweroff(struct device *dev)
>  	struct scsi_device *sdev = to_scsi_device(dev);
>  	struct ufs_hba *hba = shost_priv(sdev->host);
>  
> +	down(&hba->host_sem);
> +	hba->shutting_down = true;

[Severity: High]
Will setting hba->shutting_down = true in ufshcd_wl_poweroff() cause
hibernation poweroff to fail on MediaTek controllers?

During hibernation poweroff via:

  hibernate() ->
    power_down() ->
      hibernation_platform_enter() ->
        dpm_suspend_start() ->
          dpm_suspend()

devices are suspended in child-before-parent order, so ufshcd_wl_poweroff()
on the child device wlun (&hba->ufs_device_wlun->sdev_gendev) runs before the
parent controller's .poweroff callback on hba->dev.

In drivers/ufs/host/ufs-mediatek.c, ufs_mtk_pm_ops wires .poweroff to
ufs_mtk_system_suspend():

static const struct dev_pm_ops ufs_mtk_pm_ops = {
	SYSTEM_SLEEP_PM_OPS(ufs_mtk_system_suspend, ufs_mtk_system_resume)
...
};

And ufs_mtk_system_suspend() checks hba->shutting_down at entry:

static int ufs_mtk_system_suspend(struct device *dev)
{
	struct ufs_hba *hba = dev_get_drvdata(dev);
	struct arm_smccc_res res;
	int ret;

	if (hba->shutting_down) {
		ret = -EBUSY;
		goto out;
	}

When ufshcd_wl_poweroff() sets hba->shutting_down = true first, won't
ufs_mtk_system_suspend() immediately fail and abort hibernation?

> +	up(&hba->host_sem);

[Severity: High]
What happens if hibernation poweroff rolls back and invokes .restore?

In drivers/ufs/core/ufshcd.c, ufshcd_wl_pm_ops pairs .poweroff with .restore:

static const struct dev_pm_ops ufshcd_wl_pm_ops = {
#ifdef CONFIG_PM_SLEEP
	.suspend = ufshcd_wl_suspend,
	.resume = ufshcd_wl_resume,
	.freeze = ufshcd_wl_suspend,
	.thaw = ufshcd_wl_resume,
	.poweroff = ufshcd_wl_poweroff,
	.restore = ufshcd_wl_resume,
#endif

If hibernation_platform_enter() aborts after ufshcd_wl_poweroff() has run
(for example due to a pending wakeup event or a later device failing its
poweroff callback), dpm_resume_end() calls ufshcd_wl_resume():

static int ufshcd_wl_resume(struct device *dev)
{
...
	if (!ret)
		hba->is_sys_suspended = false;
	up(&hba->host_sem);
	return ret;
}

Unlike ufshcd_wl_suspend(), which keeps hba->host_sem held until
ufshcd_wl_resume() releases it, ufshcd_wl_poweroff() releases hba->host_sem
immediately, and ufshcd_wl_resume() never clears hba->shutting_down.

After a hibernation rollback, doesn't this perform an unbalanced
up(&hba->host_sem) (incrementing the semaphore count to 2) and leave
hba->shutting_down permanently set to true on the running system, disabling
error recovery in ufshcd_err_handling_should_stop() and blocking user
accesses in ufshcd_is_user_access_allowed()?

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