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

["hoyoung seo" <hy50.seo@samsung.com>]

Hi Bart,

Thank you for the review and the Reviewed-by on patch 1.

I will take your suggestion for ufshcd_wl_poweroff(): it will take
host_sem and keep holding it until ufshcd_wl_resume() releases it, so it
behaves like ufshcd_wl_suspend() and the unbalanced up() on the
.restore path goes away too. I will add the Fixes: tag (b294ff3e3449,
which added the WLUN .poweroff callback) and Suggested-by, and put the
patch in front of the one that limits the early link recovery.

For the patch that limits the early link recovery, v7 will:
 - use READ_ONCE() in ufshcd_err_handler() and WRITE_ONCE() in
   ufshcd_wl_runtime_suspend() and ufshcd_wl_runtime_resume();
 - describe the behavior change during shutdown in the commit message:
   errors that occur while the final START STOP UNIT command is being
   processed are no longer handled by the error handler, recovery then
   only happens through ufshcd_eh_timed_out();
 - correct the poweroff statement: the error handler waits for host_sem
   there, it is not stopped by hba->shutting_down.

I will send v7 shortly.

Thanks,

> -----Original Message-----
> From: Bart Van Assche <bvanassche@acm.org>
> Sent: Friday, November 6, 2026 1:50 PM
> To: HOYOUNG SEO <hy50.seo@samsung.com>; linux-scsi@vger.kernel.org; linux-
> kernel@vger.kernel.org; mkp@kernel.org; martin.petersen@oracle.com;
> stanleyjhu@google.com; alim.akhtar@samsung.com; kwangwon.min@samsung.com;
> kwmad.kim@samsung.com; cpgs@samsung.com; h10.kim@samsung.com
> Subject: Re: [PATCH v6 3/3] scsi: ufs: core: Set shutting_down in
> ufshcd_wl_poweroff()
> 
> 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.