Re: [PATCH v4 2/2] scsi: ufs: core: Limit early link recovery in the error handler to runtime PM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: ufs: core: concurrent error recovery during ufshcd_wl_poweroff()
--

commit 6f60eb14bc24b472557f86d1f4db1435491fade7
Author: HOYOUNG SEO <hy50.seo@samsung.com>

scsi: ufs: core: Limit early link recovery in the error handler to runtime PM

This commit restricts the early link recovery path in ufshcd_err_handler()
to runtime PM operations by introducing hba->runtime_pm_in_progress, so
that system PM and shutdown use the regular error handling path instead of
resetting the link concurrently.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 3b5580ac390cb..e3908d6af3ae4 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -7089,11 +7089,15 @@ static void ufshcd_err_handler(struct work_struct *work)
>  		 * Use ufshcd_rpm_get_noresume() here to safely perform link
>  		 * recovery even if an error occurs during runtime suspend or
>  		 * runtime resume. This avoids potential deadlocks that could
> -		 * happen if we tried to resume the device while a PM operation
> -		 * is already in progress.
> +		 * happen if we tried to resume the device while a runtime PM
> +		 * operation is already in progress.
> +		 *
> +		 * Do not do this for system PM and shutdown. The system PM
> +		 * callbacks hold host_sem, so the regular path below runs
> +		 * after them instead of resetting the link concurrently.
>  		 */
>  		ufshcd_rpm_get_noresume(hba);
> -		if (hba->pm_op_in_progress) {
> +		if (hba->runtime_pm_in_progress) {
>  			ufshcd_link_recovery(hba);
>  			ufshcd_rpm_put(hba);
>  			return;

[Severity: High]
Can this allow ufshcd_err_handler() to run regular error recovery
concurrently with hibernation poweroff in ufshcd_wl_poweroff()?

During hibernation, the PM core runs .freeze (ufshcd_wl_suspend(), which
takes hba->host_sem) followed by .thaw (ufshcd_wl_resume(), which releases
hba->host_sem) before writing the hibernation image, and then invokes
.poweroff:

drivers/ufs/core/ufshcd.c:ufshcd_wl_poweroff() {
	struct scsi_device *sdev = to_scsi_device(dev);
	struct ufs_hba *hba = shost_priv(sdev->host);

	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
	return 0;
}

When ufshcd_wl_poweroff() calls __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM),
hba->runtime_pm_in_progress is set to false. However, unlike
ufshcd_wl_suspend(), ufshcd_wl_poweroff() does not hold hba->host_sem, and
unlike ufshcd_wl_shutdown(), it does not set hba->shutting_down = true.

If an error interrupt schedules hba->eh_work while ufshcd_wl_poweroff() is
running, ufshcd_err_handler() will skip the early return because
hba->runtime_pm_in_progress is false, acquire the uncontended
hba->host_sem, see ufshcd_err_handling_should_stop(hba) return false, and
proceed with ufshcd_err_handling_prepare() and ufshcd_reset_and_restore()
at the same time __ufshcd_wl_suspend() is powering down the device and
link.

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