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

[Bart Van Assche]

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.