[PATCH v2 0/2] scsi: ufs: core: Fix device management and PM recovery

[PATCH v2 0/2] scsi: ufs: core: Fix device management and PM recovery

[Stanley Jhu]

A UFS power management callback holds hba->dev_cmd.lock and the only
reserved tag while it sends device management commands. When one of
these commands times out, ufshcd_eh_timed_out() calls
ufshcd_link_recovery() inline, which needs the same lock and reserved
tag. ufshcd_err_handler() also calls ufshcd_link_recovery()
concurrently with an active power management callback when
pm_op_in_progress is set.

Patch 1 stops the host controller in ufshcd_eh_timed_out() instead of
running link recovery inline, recovers the link in
ufshcd_set_dev_pwr_mode() for START STOP UNIT, and forces
ufshcd_err_handler() for timed-out reserved requests.

Patch 2 removes the pm_op_in_progress shortcut from
ufshcd_err_handler() so error handling waits for the callback, and
recovers the link in __ufshcd_wl_resume() when START STOP UNIT fails
while a fatal error is scheduled.

Based on mkp/scsi 7.4/scsi-fixes.

Differences from v1 [1]:
- Replace patches 1/9 to 8/9 with patch 1/2 per Bart Van Assche [2]
  and drop the external series dependency.
- Rebase v1 patch 9/9 onto patch 1/2 as patch 2/2 and recover the
  link in __ufshcd_wl_resume() on UFSHCD_STATE_EH_SCHEDULED_FATAL.

[1] https://lore.kernel.org/r/20261112051542.1539779-1-stanleyjhu@google.com
[2] https://lore.kernel.org/r/59c577b6-4749-4edc-9dd9-a83f2a124adb@acm.org

Brian Kao (1):
  scsi: ufs: core: Do not recover the link concurrently with a PM
    callback

Stanley Jhu (1):
  scsi: ufs: core: Recover link in ufshcd_set_dev_pwr_mode() on timeout

 drivers/ufs/core/ufshcd.c | 62 +++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 29 deletions(-)


base-commit: e550f2b2b4aab4c5f12db89d04d8978ce4ed7b4e
-- 
2.56.0.611.g1030f0b940-goog

[PATCH v2 1/2] scsi: ufs: core: Recover link in ufshcd_set_dev_pwr_mode() on timeout

[Stanley Jhu]

ufshcd_eh_timed_out() calls ufshcd_link_recovery() when a command
times out during a power management operation. Link recovery sends a
NOP OUT, which needs hba->dev_cmd.lock and the only reserved tag.

1. Runtime resume sends a query with the reserved tag while it holds
   hba->dev_cmd.lock.
2. The device does not answer and the query times out.
3. ufshcd_eh_timed_out() calls ufshcd_link_recovery().
4. In MCQ mode ufshcd_verify_dev_init() waits for hba->dev_cmd.lock
   while __ufshcd_wl_resume() waits for the query. Resume hangs.
5. In legacy mode the query completes but the timeout handler still
   holds the reserved tag. The NOP OUT fails with -ENOMEM and the
   host enters UFSHCD_STATE_ERROR.

Stop the host controller in ufshcd_eh_timed_out() and recover the
link in ufshcd_set_dev_pwr_mode(), which issues START STOP UNIT
without hba->dev_cmd.lock or the reserved tag. For modes other than
active, ufshcd_set_dev_pwr_mode() then fails and the caller aborts
the power transition.

Recovering the link there needs four more changes. First, START STOP
UNIT is completed with DID_TRANSPORT_FAILFAST and is not retried.
ufshcd_set_dev_pwr_mode() also recovers the link if START STOP UNIT
succeeds but the controller is disabled. Hosts with
UFSHCI_QUIRK_BROKEN_HCE do not use the HCE bit, so
ufshcd_set_dev_pwr_mode() checks only the timeout result on them.
Second, in MCQ mode ufshcd_eh_timed_out() reaps the completion queue
before it completes the command, so a late completion cannot complete
the tag twice. Third, a timed-out reserved request forces
ufshcd_err_handler(), because no power management caller recovers
after it. Fourth, in MCQ mode ufshcd_err_handler() does not poll the
completion queues while the controller is disabled.

Fixes: 01d5e237b339 ("scsi: ufs: core: Handle PM commands timeout before SCSI EH")
Cc: stable@vger.kernel.org # 7.2.x
Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Stanley Jhu <stanleyjhu@google.com>
---
 drivers/ufs/core/ufshcd.c | 43 ++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index ddc953b45ce9..0f5365bd90f0 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -7108,8 +7108,12 @@ static void ufshcd_err_handler(struct work_struct *work)
 	ufshcd_set_eh_in_progress(hba);
 	spin_unlock_irqrestore(&hba->host->host_lock, flags);
 
-	/* Complete requests that have door-bell cleared by h/w */
-	ufshcd_complete_requests(hba, false);
+	/*
+	 * Complete requests that have door-bell cleared by h/w.
+	 * CQTPy must not be read while the controller is disabled.
+	 */
+	if (!hba->mcq_enabled || ufshcd_is_hba_active(hba))
+		ufshcd_complete_requests(hba, false);
 	spin_lock_irqsave(&hba->host->host_lock, flags);
 again:
 	needs_restore = false;
@@ -9660,17 +9664,20 @@ static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
 	}
 
 	/*
-	 * Handle the timeout directly to prevent a deadlock between
-	 * ufshcd_set_dev_pwr_mode() and ufshcd_err_handler().
+	 * Stop the host controller so that @scmd can be completed here.
+	 * ufshcd_set_dev_pwr_mode() recovers the link for START STOP
+	 * UNIT. ufshcd_err_handler() recovers it for other commands.
 	 */
-	ufshcd_link_recovery(hba);
-	dev_info(hba->dev, "%s() finished; outstanding_tasks = %#lx.\n",
-		 __func__, hba->outstanding_tasks);
+	ufshcd_hba_stop(hba);
+	if (!ufshcd_is_scsi_cmd(scmd))
+		ufshcd_force_error_recovery(hba);
+	if (hba->mcq_enabled) {
+		struct ufs_hw_queue *hwq =
+			ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(scmd));
+
+		ufshcd_mcq_compl_all_cqes_lock(hba, hwq);
+	}
 
-	/*
-	 * ufshcd_link_recovery() may already have completed @scmd, e.g. via
-	 * the existing MCQ force-completion path.
-	 */
 	if (!test_bit(SCMD_STATE_COMPLETE, &scmd->state)) {
 		if (!hba->mcq_enabled) {
 			struct request *rq = scsi_cmd_to_rq(scmd);
@@ -9680,7 +9687,7 @@ static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
 		}
 
 		if (ufshcd_is_scsi_cmd(scmd)) {
-			set_host_byte(scmd, DID_REQUEUE);
+			set_host_byte(scmd, DID_TRANSPORT_FAILFAST);
 			ufshcd_release_scsi_cmd(hba, scmd);
 		} else {
 			set_host_byte(scmd, DID_TIME_OUT);
@@ -10137,6 +10144,9 @@ static int ufshcd_execute_start_stop(struct scsi_device *sdev,
 {
 	const unsigned char cdb[6] = { START_STOP, 0, 0, 0, pwr_mode << 4, 0 };
 	struct scsi_failure failure_defs[] = {
+		{
+			.result = DID_TRANSPORT_FAILFAST << 16,
+		},
 		{
 			.allowed = 2,
 			.result = SCMD_FAILURE_RESULT_ANY,
@@ -10172,6 +10182,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 	struct scsi_sense_hdr sshdr;
 	struct scsi_device *sdp;
 	unsigned long flags;
+	bool fenced;
 	int ret;
 
 	spin_lock_irqsave(&hba->host->host_lock, flags);
@@ -10199,6 +10210,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 	 * already suspended childs.
 	 */
 	ret = ufshcd_execute_start_stop(sdp, pwr_mode, &sshdr);
+	fenced = ret > 0 && host_byte(ret) == DID_TRANSPORT_FAILFAST;
 	if (ret) {
 		sdev_printk(KERN_WARNING, sdp,
 			    "START_STOP failed for power mode: %d, result %x\n",
@@ -10212,6 +10224,13 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 		hba->curr_dev_pwr_mode = pwr_mode;
 	}
 
+	if (fenced || (!(hba->quirks & UFSHCI_QUIRK_BROKEN_HCE) &&
+		       !ufshcd_is_hba_active(hba))) {
+		ret = ufshcd_link_recovery(hba);
+		if (!ret && hba->curr_dev_pwr_mode != pwr_mode)
+			ret = -EIO;
+	}
+
 	scsi_device_put(sdp);
 	WRITE_ONCE(hba->host->eh_noresume, 0);
 	return ret;
-- 
2.56.0.611.g1030f0b940-goog

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

[Stanley Jhu]

From: Brian Kao <powenkao@google.com>

ufshcd_set_dev_pwr_mode() now recovers the link after a START STOP
UNIT timeout. ufshcd_err_handler() must not recover the link while a
PM callback runs. Its pm_op_in_progress branch still does so when a
fatal UIC error arrives during that recovery.

Remove the pm_op_in_progress branch, so ufshcd_err_handler() waits
for the callback and only one context enables the host controller.

1. Runtime suspend sends START STOP UNIT and the device does not
   answer.
2. ufshcd_set_dev_pwr_mode() starts link recovery after the timeout.
3. A fatal UIC error queues ufshcd_err_handler().
4. ufshcd_err_handler() sees pm_op_in_progress and starts a second
   link recovery. Both enable the host controller at once.

Removing the pm_op_in_progress branch needs one more change. A fatal
error during runtime resume then has no recovery:

1. A fatal UIC error queues ufshcd_err_handler(), which waits for
   the runtime resume.
2. START STOP UNIT fails with DID_BAD_TARGET and resume fails.
3. ufshcd_err_handler() resumes again and fails the same way. The
   block layer pm_only count underflows and I/O hangs.

Recover the link in __ufshcd_wl_resume() when START STOP UNIT fails
while error handling is scheduled for a fatal error, as it does after
a failed hibern8 exit.

Fixes: f966e02ae521 ("scsi: ufs: core: Fix runtime suspend error deadlock")
Fixes: e23ef4f22db3 ("scsi: ufs: core: Fix error handler host_sem issue")
Cc: stable@vger.kernel.org # 7.2.x
Signed-off-by: Brian Kao <powenkao@google.com>
[stanleyjhu: recover the link in __ufshcd_wl_resume() after a
 fatal error]
Signed-off-by: Stanley Jhu <stanleyjhu@google.com>
---
 drivers/ufs/core/ufshcd.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 0f5365bd90f0..825906b643fa 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;
 		ufshcd_set_timestamp_attr(hba);
-- 
2.56.0.611.g1030f0b940-goog

Re: [PATCH v2 1/2] scsi: ufs: core: Recover link in ufshcd_set_dev_pwr_mode() on timeout

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] scsi: ufs: core: START STOP retry on stopped HBA in
  ufshcd_eh_timed_out()
- [Medium] scsi: ufs: core: TOCTOU race on ufshcd_is_hba_active() after
  timeout
--

commit 904c924ede89828a6af258eb088b07dc5e864450
Author: Stanley Jhu <stanleyjhu@google.com>

scsi: ufs: core: Recover link in ufshcd_set_dev_pwr_mode() on timeout

This commit moves link recovery on power management command timeouts out of
ufshcd_eh_timed_out() into ufshcd_set_dev_pwr_mode() and
ufshcd_err_handler(), stopping the host controller and failing START STOP
UNIT with DID_TRANSPORT_FAILFAST on timeout.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index ddc953b45ce95..0f5365bd90f04 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c

[ ... ]

> @@ -9660,17 +9664,20 @@ static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
>  	}
>  
>  	/*
> -	 * Handle the timeout directly to prevent a deadlock between
> -	 * ufshcd_set_dev_pwr_mode() and ufshcd_err_handler().
> +	 * Stop the host controller so that @scmd can be completed here.
> +	 * ufshcd_set_dev_pwr_mode() recovers the link for START STOP
> +	 * UNIT. ufshcd_err_handler() recovers it for other commands.
>  	 */
> -	ufshcd_link_recovery(hba);
> -	dev_info(hba->dev, "%s() finished; outstanding_tasks = %#lx.\n",
> -		 __func__, hba->outstanding_tasks);
> +	ufshcd_hba_stop(hba);
> +	if (!ufshcd_is_scsi_cmd(scmd))
> +		ufshcd_force_error_recovery(hba);
> +	if (hba->mcq_enabled) {
> +		struct ufs_hw_queue *hwq =
> +			ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(scmd));
> +
> +		ufshcd_mcq_compl_all_cqes_lock(hba, hwq);
> +	}
>  
> -	/*
> -	 * ufshcd_link_recovery() may already have completed @scmd, e.g. via
> -	 * the existing MCQ force-completion path.
> -	 */
>  	if (!test_bit(SCMD_STATE_COMPLETE, &scmd->state)) {
>  		if (!hba->mcq_enabled) {
>  			struct request *rq = scsi_cmd_to_rq(scmd);
> @@ -9680,7 +9687,7 @@ static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
>  		}
>  
>  		if (ufshcd_is_scsi_cmd(scmd)) {
> -			set_host_byte(scmd, DID_REQUEUE);
> +			set_host_byte(scmd, DID_TRANSPORT_FAILFAST);
>  			ufshcd_release_scsi_cmd(hba, scmd);
>  		} else {
>  			set_host_byte(scmd, DID_TIME_OUT);
> @@ -10137,6 +10144,9 @@ static int ufshcd_execute_start_stop(struct scsi_device *sdev,
>  {
>  	const unsigned char cdb[6] = { START_STOP, 0, 0, 0, pwr_mode << 4, 0 };
>  	struct scsi_failure failure_defs[] = {
> +		{
> +			.result = DID_TRANSPORT_FAILFAST << 16,
> +		},
>  		{
>  			.allowed = 2,
>  			.result = SCMD_FAILURE_RESULT_ANY,

[Severity: Medium]
Can scsi_execute_cmd() retry START STOP UNIT while the controller is
already stopped by ufshcd_hba_stop()?

If ufshcd_mcq_compl_all_cqes_lock() (or a completion interrupt just before
ufshcd_hba_stop()) completes scmd with a non-zero status such as
SAM_STAT_CHECK_CONDITION, SCMD_STATE_COMPLETE is set and
ufshcd_eh_timed_out() skips setting DID_TRANSPORT_FAILFAST.
failure_defs[] then matches SCMD_FAILURE_RESULT_ANY, so scsi_execute_cmd()
submits a retry while HCE is 0 and hba->ufshcd_state is still
UFSHCD_STATE_OPERATIONAL, writing to the doorbell or SQTP of a disabled
controller and stalling for another 10-second timeout.

In addition, if scmd completes with result 0 during ufshcd_eh_timed_out()
on a host with UFSHCI_QUIRK_BROKEN_HCE, both fenced and
!(hba->quirks & UFSHCI_QUIRK_BROKEN_HCE) are false in
ufshcd_set_dev_pwr_mode(), so ufshcd_link_recovery() is skipped even though
ufshcd_hba_stop() disabled the controller.

> @@ -10172,6 +10182,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
>  	struct scsi_sense_hdr sshdr;
>  	struct scsi_device *sdp;
>  	unsigned long flags;
> +	bool fenced;
>  	int ret;
>  
>  	spin_lock_irqsave(&hba->host->host_lock, flags);
> @@ -10199,6 +10210,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
>  	 * already suspended childs.
>  	 */
>  	ret = ufshcd_execute_start_stop(sdp, pwr_mode, &sshdr);
> +	fenced = ret > 0 && host_byte(ret) == DID_TRANSPORT_FAILFAST;
>  	if (ret) {
>  		sdev_printk(KERN_WARNING, sdp,
>  			    "START_STOP failed for power mode: %d, result %x\n",
> @@ -10212,6 +10224,13 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
>  		hba->curr_dev_pwr_mode = pwr_mode;
>  	}
>  
> +	if (fenced || (!(hba->quirks & UFSHCI_QUIRK_BROKEN_HCE) &&
> +		       !ufshcd_is_hba_active(hba))) {
> +		ret = ufshcd_link_recovery(hba);
> +		if (!ret && hba->curr_dev_pwr_mode != pwr_mode)
> +			ret = -EIO;
> +	}
> +
>  	scsi_device_put(sdp);
>  	WRITE_ONCE(hba->host->eh_noresume, 0);
>  	return ret;

[Severity: Medium]
Can ufshcd_set_dev_pwr_mode() check ufshcd_is_hba_active() before a racing
ufshcd_eh_timed_out() finishes stopping the controller?

CPU0 (IRQ)               CPU1 (timeout)           CPU2 (PM caller)
ufshcd_intr()            ufshcd_eh_timed_out()
  ...                      ufshcd_hba_stop()
    scsi_done(scmd)          ufshcd_disable_irq()
                                                  ufshcd_execute_start_stop()
                                                    returns 0
                                                  ufshcd_is_hba_active(hba)
                                                    --> true (recovery skipped)
                             writel(CONTROLLER_DISABLE,
                                    REG_CONTROLLER_ENABLE)
                               <-- HBA left disabled

When a normal completion interrupt races with request timeout expiration,
scsi_done() wakes scsi_execute_cmd() on CPU2 while CPU1 is still in
ufshcd_disable_irq() inside ufshcd_hba_stop().  CPU2 sees ret == 0 and
ufshcd_is_hba_active(hba) == true, so it skips ufshcd_link_recovery(), and
CPU1 then clears HCE and returns SCSI_EH_DONE because SCMD_STATE_COMPLETE
is already set, leaving the controller disabled.

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

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

[sashiko-bot]

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