[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

[Bart Van Assche]

On 11/13/26 3:12 AM, Stanley Jhu wrote:
> Recovering the link there needs four more changes. [ ... ]
One change per patch please. If you are using an AI coding assistant to
prepare patches, a possible approach is to ask your coding assistant to
split this patch into logical changes.

Thanks,

Bart.