Re: [PATCH v5 2/2] scsi: ufs: core: Do not recover the link early in the error handler during system PM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: ufs: core: race on system_pm_in_progress in ufshcd_err_handler()
--

commit 991f616e0ee805a5ba3aa9d0edc90a319abd06b6
Author: HOYOUNG SEO <hy50.seo@samsung.com>

scsi: ufs: core: Do not recover the link early in the error handler during system PM

This commit adds hba->system_pm_in_progress so that ufshcd_err_handler()
skips its early unlocked ufshcd_link_recovery() path during system suspend
and system resume, waiting on hba->host_sem instead.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 3b5580ac390cb..761ffc8f0a32a 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -7091,9 +7091,13 @@ static void ufshcd_err_handler(struct work_struct *work)
>  		 * runtime resume. This avoids potential deadlocks that could
>  		 * happen if we tried to resume the device while a PM operation
>  		 * is already in progress.
> +		 *
> +		 * Do not do this for system PM. 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->pm_op_in_progress && !hba->system_pm_in_progress) {
>  			ufshcd_link_recovery(hba);
>  			ufshcd_rpm_put(hba);
>  			return;
> @@ -10393,6 +10397,7 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  	enum uic_link_state req_link_state;
>  
>  	hba->pm_op_in_progress = true;
> +	hba->system_pm_in_progress = (pm_op == UFS_SYSTEM_PM);
>  	if (pm_op != UFS_SHUTDOWN_PM) {
>  		pm_lvl = pm_op == UFS_RUNTIME_PM ?
>  			 hba->rpm_lvl : hba->spm_lvl;
> @@ -10565,6 +10570,7 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  		hba->clk_gating.is_suspended = false;
>  		ufshcd_release(hba);
>  	}
> +	hba->system_pm_in_progress = false;
>  	hba->pm_op_in_progress = false;
>  	return ret;
>  }
> @@ -10576,6 +10582,7 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  	enum uic_link_state old_link_state = hba->uic_link_state;
>  
>  	hba->pm_op_in_progress = true;
> +	hba->system_pm_in_progress = (pm_op == UFS_SYSTEM_PM);
>  
>  	/*
>  	 * Call vendor specific resume callback. As these callbacks may access
> @@ -10666,6 +10673,7 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  		ufshcd_update_evt_hist(hba, UFS_EVT_WL_RES_ERR, (u32)ret);
>  	hba->clk_gating.is_suspended = false;
>  	ufshcd_release(hba);
> +	hba->system_pm_in_progress = false;
>  	hba->pm_op_in_progress = false;
>  	return ret;
>  }

[Severity: Medium]
Can ufshcd_err_handler() still observe hba->pm_op_in_progress == true and
hba->system_pm_in_progress == false during system suspend or system resume?

ufshcd_err_handler() reads both flags without holding hba->host_sem or any
other lock shared with __ufshcd_wl_suspend() and __ufshcd_wl_resume().

At the entry of __ufshcd_wl_suspend() and __ufshcd_wl_resume(),
hba->pm_op_in_progress = true is written before hba->system_pm_in_progress is
set, and at exit hba->system_pm_in_progress = false is cleared before
hba->pm_op_in_progress = false, without barriers or READ_ONCE()/WRITE_ONCE().

In particular, when __ufshcd_wl_suspend() encounters an error during
UFS_SYSTEM_PM, it calls ufshcd_force_error_recovery() (which immediately
queues hba->eh_work) and jumps to out:

drivers/ufs/core/ufshcd.c:__ufshcd_wl_suspend() {
	...
	if (ret && pm_op != UFS_SHUTDOWN_PM) {
		ufshcd_force_error_recovery(hba);
		ret = -EBUSY;
	}
	...
out:
	...
	if (ret) {
		ufshcd_update_evt_hist(hba, UFS_EVT_WL_SUSP_ERR, (u32)ret);
		hba->clk_gating.is_suspended = false;
		ufshcd_release(hba);
	}
	hba->system_pm_in_progress = false;
	hba->pm_op_in_progress = false;
	return ret;
}

If ufshcd_err_handler() evaluates the condition in the entry or exit window
(or due to compiler/CPU reordering), it will enter the early
ufshcd_link_recovery() path without hba->host_sem or a clock-gating hold and
return early without running the regular error handling path (such as
ufshcd_err_handling_prepare() and ufshcd_recover_pm_error()).

Would ordering the updates (or encoding the active PM operation in a single
variable accessed with READ_ONCE()/WRITE_ONCE()) avoid this race window?

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