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

[Bart Van Assche <bvanassche@acm.org>]

On 11/5/26 6:15 PM, HOYOUNG SEO wrote:
 > +	down(&hba->host_sem);
 > +	hba->shutting_down = true;
 > +	up(&hba->host_sem);
 > +
 >  	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);

I agree with both issues reported by Sashiko:
- ufs_mtk_system_suspend() is also the .poweroff callback of the
   MediaTek host controller device and returns -EBUSY if
   hba->shutting_down has been set. Since the .poweroff callback of the
   WLUN is called before that of its parent, this patch makes
   hibernation fail on MediaTek hosts.
- If hibernation is aborted after the WLUN .poweroff callback has been
   called, ufshcd_wl_resume() is called as .restore callback.
   Nothing clears hba->shutting_down, so the error handler and user
   access (ufshcd_is_user_access_allowed()) remain disabled.

Additionally, the existing code already has an unbalanced
up(&hba->host_sem) on that path: ufshcd_wl_poweroff() does not acquire
host_sem but ufshcd_wl_resume() releases it.

How about making ufshcd_wl_poweroff() behave like ufshcd_wl_suspend()
instead, i.e. acquire host_sem and keep holding it until .restore
releases it? Something like this (untested):

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;
}

That serializes the error handler against poweroff the same way as for
system suspend, fixes the unbalanced up() call and leaves
hba->shutting_down alone. Please also add a Fixes: tag and move this
patch in front of patch 2/3.

Thanks,

Bart.