Re: [PATCH v2 2/2] scsi: ufs: core: Do not recover the link concurrently with a PM callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] scsi: ufs: core: unrecovered non-fatal SSU failure in
  __ufshcd_wl_resume()
- [Medium] scsi: ufs: core: stale error flags after
  ufshcd_link_recovery() cause double reset
--

commit 1891702caa856d9953dfbbc2c8ef55704458a505
Author: Brian Kao <powenkao@google.com>

scsi: ufs: core: Do not recover the link concurrently with a PM callback

This commit removes the pm_op_in_progress link recovery path from
ufshcd_err_handler() and adds link recovery in __ufshcd_wl_resume() when
START STOP UNIT fails while error handling is scheduled for a fatal error.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 0f5365bd90f04..825906b643faf 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -7074,23 +7074,6 @@ static void ufshcd_err_handler(struct work_struct *work)
>  		 hba->saved_uic_err, hba->force_reset,
>  		 ufshcd_is_link_broken(hba) ? "; link is broken" : "");
>  
> -	if (hba->ufs_device_wlun) {
> -		/*
> -		 * 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.
> -		 */
> -		ufshcd_rpm_get_noresume(hba);
> -		if (hba->pm_op_in_progress) {
> -			ufshcd_link_recovery(hba);
> -			ufshcd_rpm_put(hba);
> -			return;
> -		}
> -		ufshcd_rpm_put(hba);
> -	}
> -
>  	down(&hba->host_sem);
>  	spin_lock_irqsave(&hba->host->host_lock, flags);
>  	if (ufshcd_err_handling_should_stop(hba)) {
> @@ -10638,6 +10621,8 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  
>  	if (!ufshcd_is_ufs_dev_active(hba)) {
>  		ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
> +		if (ret && hba->ufshcd_state == UFSHCD_STATE_EH_SCHEDULED_FATAL)
> +			ret = ufshcd_link_recovery(hba);
>  		if (ret)
>  			goto set_old_link_state;

[Severity: High]
What happens if START STOP UNIT fails without timing out while
hba->ufshcd_state is UFSHCD_STATE_EH_SCHEDULED_NON_FATAL?

UIC errors not matched by ufshcd_is_saved_err_fatal(), such as
UFSHCD_UIC_DL_TCx_REPLAY_ERROR or UFSHCD_UIC_DL_NAC_RECEIVED_ERROR, set
hba->ufshcd_state to UFSHCD_STATE_EH_SCHEDULED_NON_FATAL and queue
ufshcd_err_handler(), which now blocks in ufshcd_err_handling_prepare() ->
ufshcd_rpm_get_sync() waiting for __ufshcd_wl_resume().  Under
UFSHCD_STATE_EH_SCHEDULED_NON_FATAL, ufshcd_queuecommand() still dispatches
START STOP UNIT, and if it completes with an OCS error such as
OCS_PEER_COMM_FAILURE (mapped to DID_ERROR), fenced is false and
ufshcd_is_hba_active(hba) remains true, so neither ufshcd_set_dev_pwr_mode()
nor __ufshcd_wl_resume() calls ufshcd_link_recovery().  __ufshcd_wl_resume()
then returns -EIO, and when ufshcd_err_handler() wakes in rpm_resume() and
retries sdev_runtime_resume(), blk_post_runtime_resume() calls
blk_clear_pm_only() a second time without an intervening suspend,
underflowing q->pm_only and hanging the queue.

Also, if ufshcd_link_recovery() fails here, jumping to set_old_link_state
calls ufshcd_link_state_transition(hba, old_link_state, 0) on a failed
controller whose link state was reset to UIC_LINK_OFF_STATE, rather than
jumping to vendor_suspend as the hibern8-exit recovery path does.

[Severity: Medium]
Does calling ufshcd_link_recovery() here leave hba->force_reset,
hba->saved_err, and hba->saved_uic_err set, causing ufshcd_err_handler() to
reset the device and host controller a second time?

When ufshcd_set_dev_pwr_mode() submits START STOP UNIT while
hba->ufshcd_state is UFSHCD_STATE_EH_SCHEDULED_FATAL, ufshcd_queuecommand()
sets hba->force_reset = true and fails the command with DID_BAD_TARGET:

drivers/ufs/core/ufshcd.c:ufshcd_queuecommand() {
    ...
	case UFSHCD_STATE_EH_SCHEDULED_FATAL:
		if (hba->pm_op_in_progress) {
			hba->force_reset = true;
			set_host_byte(cmd, DID_BAD_TARGET);
			scsi_done(cmd);
			goto out;
		}
    ...
}

Unlike ufshcd_reset_and_restore(), ufshcd_link_recovery() does not clear
hba->saved_err, hba->saved_uic_err, or hba->force_reset.  Once
__ufshcd_wl_resume() finishes and unblocks ufshcd_err_handling_prepare(),
ufshcd_err_handling_should_stop() sees those flags still set and returns
false, so ufshcd_err_handler() proceeds to do_reset and runs
ufshcd_reset_and_restore() again.  In addition, if a non-fatal UIC error
interrupt fires during ufshcd_link_recovery() after interrupts are
re-enabled, ufshcd_schedule_eh_work() sees the stale hba->force_reset and
fatal error bits and sets hba->ufshcd_state back to
UFSHCD_STATE_EH_SCHEDULED_FATAL, preventing ufshcd_process_probe_result()
from transitioning hba->ufshcd_state to UFSHCD_STATE_OPERATIONAL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113111202.3551341-1-stanleyjhu@google.com?part=2