Re: [PATCH v2 2/2] scsi: ufs: core: add is_sys_suspended state before recovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: ufs: core: ufshcd_err_handler() skips recovery after failed resume
--

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

scsi: ufs: core: add is_sys_suspended state before recovery

This commit skips the early link recovery in ufshcd_err_handler() when
hba->is_sys_suspended is set so that error recovery waits on hba->host_sem
instead of racing with __ufshcd_wl_resume() during system resume.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 0e6e593c24cfd..4b4d473530b16 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -7052,9 +7052,14 @@ 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.
> +		 *
> +		 * During system resume, host_sem is held from
> +		 * ufshcd_wl_suspend() until ufshcd_wl_resume() returns, so wait
> +		 * for it below instead of recovering the link concurrently with
> +		 * the resume path.
>  		 */
>  		ufshcd_rpm_get_noresume(hba);
> -		if (hba->pm_op_in_progress) {
> +		if (hba->pm_op_in_progress && !hba->is_sys_suspended) {

[Severity: High]
The commit message notes that if system resume fails, hba->is_sys_suspended
stays set and ufshcd_err_handler() performs a full recovery as usual.
However, when ufshcd_err_handler() wakes from down(&hba->host_sem) after
ufshcd_wl_resume() -> __ufshcd_wl_resume() fails, it immediately checks
ufshcd_err_handling_should_stop():

drivers/ufs/core/ufshcd.c:ufshcd_err_handler() {
	down(&hba->host_sem);
	spin_lock_irqsave(&hba->host->host_lock, flags);
	if (ufshcd_err_handling_should_stop(hba)) {
		if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
			hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
		spin_unlock_irqrestore(&hba->host->host_lock, flags);
		up(&hba->host_sem);
		return;
	}
}

where ufshcd_err_handling_should_stop() is defined as:

drivers/ufs/core/ufshcd.c:ufshcd_err_handling_should_stop() {
	return (!hba->is_powered || hba->shutting_down ||
		!hba->ufs_device_wlun ||
		hba->ufshcd_state == UFSHCD_STATE_ERROR ||
		(!(hba->saved_err || hba->saved_uic_err || hba->force_reset ||
		   ufshcd_is_link_broken(hba))));
}

If ufshcd_uic_hibern8_exit() fails in __ufshcd_wl_resume() and the subsequent
call to ufshcd_link_recovery() also fails, ufshcd_link_recovery() sets
hba->ufshcd_state = UFSHCD_STATE_ERROR even though it restores hba->saved_err
and hba->saved_uic_err:

drivers/ufs/core/ufshcd.c:ufshcd_link_recovery() {
	...
	spin_lock_irqsave(&hba->host->host_lock, flags);
	if (ret) {
		hba->ufshcd_state = UFSHCD_STATE_ERROR;
		hba->saved_err |= saved_err;
		hba->saved_uic_err |= saved_uic_err;
	}
	ufshcd_clear_eh_in_progress(hba);
	spin_unlock_irqrestore(&hba->host->host_lock, flags);
}

Alternatively, if ufshcd_link_recovery() succeeds in __ufshcd_wl_resume()
(clearing hba->saved_err, hba->saved_uic_err, and hba->force_reset) and a
later step in __ufshcd_wl_resume() such as ufshcd_set_dev_pwr_mode() fails,
the saved error flags remain zero.

In both cases, doesn't ufshcd_err_handling_should_stop() evaluate to true and
cause ufshcd_err_handler() to return immediately after acquiring
hba->host_sem, skipping full recovery and ufshcd_recover_pm_error() while
leaving the HBA in UFSHCD_STATE_ERROR or stuck with hba->is_sys_suspended
set?

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