[PATCH 0/3] ufs: core: Improve error handling

[PATCH 0/3] ufs: core: Improve error handling

[Bart Van Assche]

Hi Martin,

This patch series fixes two subtle race conditions related to UFS error handling.
Please consider these changes for the next merge window.

Thanks,

Bart.

Bart Van Assche (3):
  ufs: core: Simplify timeout handling for START STOP UNIT commands
  ufs: core: Fix a race condition triggered by ufshcd_eh_timed_out()
  ufs: core: Do not forcibly complete SCSI commands

 drivers/ufs/core/ufs-mcq.c     | 26 ----------
 drivers/ufs/core/ufshcd-priv.h |  2 -
 drivers/ufs/core/ufshcd.c      | 88 ++++++++--------------------------
 3 files changed, 19 insertions(+), 97 deletions(-)

[PATCH 1/3] ufs: core: Simplify timeout handling for START STOP UNIT commands

[Bart Van Assche]

The set_host_byte(cmd, DID_REQUEUE) code has been copied from
ufshcd_mcq_force_compl_one(). That completion status is useful for SCSI
commands that should be retried but not for START STOP UNIT commands,
the only type of SCSI command for which ufshcd_eh_timed_out() calls
set_host_byte(). The SCMD_FAIL_IF_RECOVERING flag is set for START STOP
UNIT commands. This causes scsi_queue_rq() to fail SCSI commands. Simplify
timeout handling for START STOP UNIT commands by skipping command
resubmission and by failing START STOP UNIT commands directly. See also
commit 01d5e237b339 ("scsi: ufs: core: Handle PM commands timeout before
SCSI EH").

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index a51e071916cf..73f1e8817f44 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9604,13 +9604,9 @@ static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
 			spin_unlock_irqrestore(&hba->outstanding_lock, flags);
 		}
 
-		if (ufshcd_is_scsi_cmd(scmd)) {
-			set_host_byte(scmd, DID_REQUEUE);
+		set_host_byte(scmd, DID_TIME_OUT);
+		if (ufshcd_is_scsi_cmd(scmd))
 			ufshcd_release_scsi_cmd(hba, scmd);
-		} else {
-			set_host_byte(scmd, DID_TIME_OUT);
-		}
-
 		scsi_done(scmd);
 	}
 

Re: [PATCH 1/3] ufs: core: Simplify timeout handling for START STOP UNIT commands

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Replacing DID_REQUEUE with DID_TIME_OUT for START STOP UNIT commands inadvertently routes them to the SCSI Error Handler, risking deadlock and breaking PM recovery.
--

--- Patch [1]: [PATCH 1/3] ufs: core: Simplify timeout handling for START STOP UNIT commands ---
commit 457c19a7449f41757fe8a8fc3aa57e02dd255499
Author: Bart Van Assche <bvanassche@acm.org>
    
ufs: core: Simplify timeout handling for START STOP UNIT commands
    
This commit simplifies timeout handling for START STOP UNIT commands by failing
them directly instead of requeuing them. It updates the ufshcd_eh_timed_out
handler to use DID_TIME_OUT rather than DID_REQUEUE for SCSI commands.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -9572,13 +9572,9 @@ static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
>  			spin_unlock_irqrestore(&hba->outstanding_lock, flags);
>  		}
>  
> -		if (ufshcd_is_scsi_cmd(scmd)) {
> -			set_host_byte(scmd, DID_REQUEUE);
> +		set_host_byte(scmd, DID_TIME_OUT);

[Severity: High]
Will this inadvertently route START STOP UNIT commands to the SCSI Error
Handler and risk deadlocks during PM recovery?

Looking at scsi_decide_disposition(), setting DID_TIME_OUT for commands other
than INQUIRY or TEST_UNIT_READY (such as START STOP UNIT, opcode 0x1B)
evaluates to FAILED. The SCSI core then proceeds to call scsi_eh_scmd_add(),
which flags the host for SHOST_RECOVERY and wakes the SCSI Error Handler
thread.

Waking the EH thread during PM transitions appears to break the UFS
subsystem's custom inline recovery and reintroduces the deadlocks that commit
01d5e237b339 originally resolved.

If the goal is to fail the command directly without retries and without
invoking the EH, would an error status that evaluates to SUCCESS in
scsi_decide_disposition() (such as DID_BAD_TARGET) be more appropriate here?

> +		if (ufshcd_is_scsi_cmd(scmd))
>  			ufshcd_release_scsi_cmd(hba, scmd);
> -		} else {
> -			set_host_byte(scmd, DID_TIME_OUT);
> -		}
> -
>  		scsi_done(scmd);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786558726.git.bvanassche@acm.org?part=1

[PATCH 2/3] ufs: core: Fix a race condition triggered by ufshcd_eh_timed_out()

[Bart Van Assche]

If a START STOP UNIT command times out, the ufshcd_link_recovery() call
in ufshcd_eh_timed_out() may call scsi_done() for that command via the
force-completion mechanism. This may cause ufshcd_set_dev_pwr_mode() to
return before link recovery has finished. Fix this race condition by
moving the ufshcd_link_recovery() call into ufshcd_set_dev_pwr_mode().

Fixes: 7029e2151a7c ("scsi: ufs: Fix a deadlock between PM and the SCSI error handler")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 36 ++++++++++++------------------------
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 73f1e8817f44..0618712be32c 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9582,34 +9582,18 @@ static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
 		return SCSI_EH_NOT_HANDLED;
 	}
 
-	/*
-	 * Handle the timeout directly to prevent a deadlock between
-	 * ufshcd_set_dev_pwr_mode() and ufshcd_err_handler().
-	 */
-	ufshcd_link_recovery(hba);
-	dev_info(hba->dev, "%s() finished; outstanding_tasks = %#lx.\n",
-		 __func__, hba->outstanding_tasks);
-
-	/*
-	 * 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) {
-			unsigned long flags;
-			struct request *rq = scsi_cmd_to_rq(scmd);
+	if (!hba->mcq_enabled) {
+		struct request *rq = scsi_cmd_to_rq(scmd);
 
-			spin_lock_irqsave(&hba->outstanding_lock, flags);
+		scoped_guard(spinlock_irqsave, &hba->outstanding_lock)
 			__clear_bit(rq->tag, &hba->outstanding_reqs);
-			spin_unlock_irqrestore(&hba->outstanding_lock, flags);
-		}
-
-		set_host_byte(scmd, DID_TIME_OUT);
-		if (ufshcd_is_scsi_cmd(scmd))
-			ufshcd_release_scsi_cmd(hba, scmd);
-		scsi_done(scmd);
 	}
 
+	set_host_byte(scmd, DID_TIME_OUT);
+	if (ufshcd_is_scsi_cmd(scmd))
+		ufshcd_release_scsi_cmd(hba, scmd);
+	scsi_done(scmd);
+
 	return SCSI_EH_DONE;
 }
 
@@ -10123,9 +10107,13 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 		sdev_printk(KERN_WARNING, sdp,
 			    "START_STOP failed for power mode: %d, result %x\n",
 			    pwr_mode, ret);
+
+		ufshcd_link_recovery(hba);
+
 		if (ret > 0) {
 			if (scsi_sense_valid(&sshdr))
 				scsi_print_sense_hdr(sdp, NULL, &sshdr);
+
 			ret = -EIO;
 		}
 	} else {

Re: [PATCH 2/3] ufs: core: Fix a race condition triggered by ufshcd_eh_timed_out()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Race condition with hardware completions leading to double-release of DMA and PM resources.
- [High] Unconditional hardware link recovery for non-timeout START STOP UNIT failures.
--

--- Patch [2]: [PATCH 2/3] ufs: core: Fix a race condition triggered by ufshcd_eh_timed_out() ---
commit 5124f897a6830560d1ea1c644750541f1ef1fc0b
Author: Bart Van Assche <bvanassche@acm.org>

ufs: core: Fix a race condition triggered by ufshcd_eh_timed_out()

This patch moves ufshcd_link_recovery() out of the timeout handler and into
ufshcd_set_dev_pwr_mode() to prevent deadlocks. It also removes the checking
of SCMD_STATE_COMPLETE before command cleanup in the timeout handler.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 6b94f0d91790c..cdfcd3d4c4c97 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -9550,34 +9550,18 @@ static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
>  		return SCSI_EH_NOT_HANDLED;
>  	}
>  
> -	/*
> -	 * Handle the timeout directly to prevent a deadlock between
> -	 * ufshcd_set_dev_pwr_mode() and ufshcd_err_handler().
> -	 */
> -	ufshcd_link_recovery(hba);
> -	dev_info(hba->dev, "%s() finished; outstanding_tasks = %#lx.\n",
> -		 __func__, hba->outstanding_tasks);
> -
> -	/*
> -	 * 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) {
> -			unsigned long flags;
> -			struct request *rq = scsi_cmd_to_rq(scmd);
> +	if (!hba->mcq_enabled) {
> +		struct request *rq = scsi_cmd_to_rq(scmd);
>  
> -			spin_lock_irqsave(&hba->outstanding_lock, flags);
> +		scoped_guard(spinlock_irqsave, &hba->outstanding_lock)
>  			__clear_bit(rq->tag, &hba->outstanding_reqs);
> -			spin_unlock_irqrestore(&hba->outstanding_lock, flags);
> -		}
> -
> -		set_host_byte(scmd, DID_TIME_OUT);
> -		if (ufshcd_is_scsi_cmd(scmd))
> -			ufshcd_release_scsi_cmd(hba, scmd);
> -		scsi_done(scmd);
>  	}
>  
> +	set_host_byte(scmd, DID_TIME_OUT);
> +	if (ufshcd_is_scsi_cmd(scmd))
> +		ufshcd_release_scsi_cmd(hba, scmd);

[Severity: Critical]
Since the synchronous ufshcd_link_recovery() call has been removed from this
handler, the controller is still running and could complete this command
concurrently.

By removing the check for SCMD_STATE_COMPLETE, doesn't this create a race
where both the timeout handler and the hardware completion interrupt
(via ufshcd_compl_one_cqe()) might call ufshcd_release_scsi_cmd() on the
same command simultaneously?

This could lead to double-unmapping DMA regions and decrementing PM clock
gating refcounts twice.

> +	scsi_done(scmd);
> +
>  	return SCSI_EH_DONE;
>  }
>  
> @@ -10091,9 +10075,13 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
>  		sdev_printk(KERN_WARNING, sdp,
>  			    "START_STOP failed for power mode: %d, result %x\n",
>  			    pwr_mode, ret);
> +
> +		ufshcd_link_recovery(hba);

[Severity: High]
Does this unconditionally trigger a full hardware link recovery for any
non-zero return value from scsi_execute_cmd()?

The scsi_execute_cmd() call can fail for reasons other than timeouts,
such as memory pressure returning -ENOMEM or normal device check conditions.
Calling ufshcd_link_recovery() in these scenarios seems like it would cause
severe performance degradation and disruptive device resets for routine errors.

Should this check the specific failure reason, like DID_TIME_OUT, before
initiating recovery?

> +
>  		if (ret > 0) {
>  			if (scsi_sense_valid(&sshdr))
>  				scsi_print_sense_hdr(sdp, NULL, &sshdr);
> +
>  			ret = -EIO;
>  		}
>  	} else {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786558726.git.bvanassche@acm.org?part=2

[PATCH 3/3] ufs: core: Do not forcibly complete SCSI commands

[Bart Van Assche]

The SCSI core error handler is responsible for deciding whether to abort or
to requeue SCSI commands. A SCSI LLD shouldn't interfere with this decision.
Hence, do not forcibly complete SCSI commands from inside the UFS error
handler. This patch makes the MCQ behavior consistent with the behavior for
legacy mode.

Fixes: ab248643d3d6 ("scsi: ufs: core: Add error handling for MCQ mode")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufs-mcq.c     | 26 ------------------
 drivers/ufs/core/ufshcd-priv.h |  2 --
 drivers/ufs/core/ufshcd.c      | 48 +++++-----------------------------
 3 files changed, 7 insertions(+), 69 deletions(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 8106d55f4041..55ed72d15ada 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -328,32 +328,6 @@ static void ufshcd_mcq_process_cqe(struct ufs_hba *hba,
 	}
 }
 
-/*
- * This function is called from the UFS error handler with the UFS host
- * controller disabled (HCE = 0). Reading host controller registers, e.g. the
- * CQ tail pointer (CQTPy), may not be safe with the host controller disabled.
- * Hence, iterate over all completion queue entries. This won't result in
- * double completions because ufshcd_mcq_process_cqe() clears a CQE after it
- * has been processed.
- */
-void ufshcd_mcq_compl_all_cqes_lock(struct ufs_hba *hba,
-				    struct ufs_hw_queue *hwq)
-{
-	unsigned long flags;
-	u32 entries = hwq->max_entries;
-
-	spin_lock_irqsave(&hwq->cq_lock, flags);
-	while (entries > 0) {
-		ufshcd_mcq_process_cqe(hba, hwq);
-		ufshcd_mcq_inc_cq_head_slot(hwq);
-		entries--;
-	}
-
-	ufshcd_mcq_update_cq_tail_slot(hwq);
-	hwq->cq_head_slot = hwq->cq_tail_slot;
-	spin_unlock_irqrestore(&hwq->cq_lock, flags);
-}
-
 unsigned long ufshcd_mcq_poll_cqe_lock(struct ufs_hba *hba,
 				       struct ufs_hw_queue *hwq)
 {
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index e55c2a02c1f5..8ddc19143abf 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -73,8 +73,6 @@ int ufshcd_get_hba_mac(struct ufs_hba *hba);
 int ufshcd_mcq_memory_alloc(struct ufs_hba *hba);
 struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba,
 					   struct request *req);
-void ufshcd_mcq_compl_all_cqes_lock(struct ufs_hba *hba,
-				    struct ufs_hw_queue *hwq);
 bool ufshcd_cmd_inflight(struct scsi_cmnd *cmd);
 int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag);
 int ufshcd_mcq_abort(struct scsi_cmnd *cmd);
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 0618712be32c..963784f8c0a6 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -6018,34 +6018,6 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
 	return completed_reqs != 0;
 }
 
-static bool ufshcd_mcq_force_compl_one(struct request *rq, void *priv)
-{
-	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
-	struct scsi_device *sdev = rq->q->queuedata;
-	struct Scsi_Host *shost = sdev->host;
-	struct ufs_hba *hba = shost_priv(shost);
-	struct ufs_hw_queue *hwq = ufshcd_mcq_req_to_hwq(hba, rq);
-
-	if (blk_mq_is_reserved_rq(rq) || !hwq)
-		return true;
-
-	ufshcd_mcq_compl_all_cqes_lock(hba, hwq);
-
-	/*
-	 * For those cmds of which the cqes are not present in the cq, complete
-	 * them explicitly.
-	 */
-	scoped_guard(spinlock_irqsave, &hwq->cq_lock) {
-		if (!test_bit(SCMD_STATE_COMPLETE, &cmd->state)) {
-			set_host_byte(cmd, DID_REQUEUE);
-			ufshcd_release_scsi_cmd(hba, cmd);
-			scsi_done(cmd);
-		}
-	}
-
-	return true;
-}
-
 static bool ufshcd_mcq_compl_one(struct request *rq, void *priv)
 {
 	struct scsi_device *sdev = rq->q->queuedata;
@@ -6066,16 +6038,10 @@ static bool ufshcd_mcq_compl_one(struct request *rq, void *priv)
  * the scsi command.
  *
  * @hba: per adapter instance
- * @force_compl: This flag is set to true when invoked
- * from ufshcd_host_reset_and_restore() in which case it requires special
- * handling because the host controller has been reset by ufshcd_hba_stop().
  */
-static void ufshcd_mcq_compl_pending_transfer(struct ufs_hba *hba,
-					      bool force_compl)
+static void ufshcd_mcq_compl_pending_transfer(struct ufs_hba *hba)
 {
-	blk_mq_tagset_busy_iter(&hba->host->tag_set,
-				force_compl ? ufshcd_mcq_force_compl_one :
-					      ufshcd_mcq_compl_one,
+	blk_mq_tagset_busy_iter(&hba->host->tag_set, ufshcd_mcq_compl_one,
 				NULL);
 }
 
@@ -6632,10 +6598,10 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
 }
 
 /* Complete requests that have door-bell cleared */
-static void ufshcd_complete_requests(struct ufs_hba *hba, bool force_compl)
+static void ufshcd_complete_requests(struct ufs_hba *hba)
 {
 	if (hba->mcq_enabled)
-		ufshcd_mcq_compl_pending_transfer(hba, force_compl);
+		ufshcd_mcq_compl_pending_transfer(hba);
 	else
 		ufshcd_transfer_req_compl(hba);
 
@@ -6936,7 +6902,7 @@ static bool ufshcd_abort_all(struct ufs_hba *hba)
 
 out:
 	/* Complete the requests that are cleared by s/w */
-	ufshcd_complete_requests(hba, false);
+	ufshcd_complete_requests(hba);
 
 	return ret != 0;
 }
@@ -6998,7 +6964,7 @@ static void ufshcd_err_handler(struct work_struct *work)
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
 	/* Complete requests that have door-bell cleared by h/w */
-	ufshcd_complete_requests(hba, false);
+	ufshcd_complete_requests(hba);
 	spin_lock_irqsave(hba->host->host_lock, flags);
 again:
 	needs_restore = false;
@@ -8229,7 +8195,7 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
 	 */
 	ufshcd_hba_stop(hba);
 	hba->silence_err_logs = true;
-	ufshcd_complete_requests(hba, true);
+	ufshcd_complete_requests(hba);
 	hba->silence_err_logs = false;
 
 	/* scale up clocks to max frequency before full reinitialization */

Re: [PATCH 3/3] ufs: core: Do not forcibly complete SCSI commands

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Polling the MCQ Completion Queue by reading hardware registers while the host controller is disabled (HCE=0) causes a hard CPU lockup due to an infinite loop.
--

--- Patch [3]: [PATCH 3/3] ufs: core: Do not forcibly complete SCSI commands ---
commit af070348e9f00f73ade393f3d9421e41e0afb4fb
Author: Bart Van Assche <bvanassche@acm.org>

ufs: core: Do not forcibly complete SCSI commands

This patch removes the forced completion of SCSI commands from the UFS error
handler for MCQ mode, delegating abort and requeue decisions to the SCSI core.
It makes MCQ behavior consistent with legacy mode by removing
ufshcd_mcq_compl_all_cqes_lock and the force_compl flag.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index cdfcd3d4c4c97..26bc2b0c08124 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
[ ... ]
> @@ -8197,7 +8163,7 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
>  	 */
>  	ufshcd_hba_stop(hba);
>  	hba->silence_err_logs = true;
> -	ufshcd_complete_requests(hba, true);
> +	ufshcd_complete_requests(hba);

[Severity: Critical]
Can this lead to an infinite loop and hard lockup during error recovery?

After ufshcd_hba_stop() disables the host controller,
ufshcd_complete_requests() now eventually calls ufshcd_mcq_poll_cqe_lock():

ufshcd_host_reset_and_restore()
  ufshcd_hba_stop()
  ufshcd_complete_requests()
    ufshcd_mcq_compl_pending_transfer()
      blk_mq_tagset_busy_iter(..., ufshcd_mcq_compl_one, ...)
        ufshcd_mcq_compl_one()
          ufshcd_mcq_poll_cqe_lock()

In ufshcd_mcq_poll_cqe_lock():

    ufshcd_mcq_update_cq_tail_slot(hwq);
    while (!ufshcd_mcq_is_cq_empty(hwq)) {
        ...
        ufshcd_mcq_inc_cq_head_slot(hwq);
    }

If reading the disabled MMIO register in ufshcd_mcq_update_cq_tail_slot()
returns 0xFFFFFFFF, the tail slot becomes out-of-bounds. Since the head slot
wraps around at hwq->max_entries, ufshcd_mcq_is_cq_empty() will never evaluate
to true.

Because this loop runs with the hwq->cq_lock spinlock held and interrupts
disabled, will this permanently hang the CPU?

>  	hba->silence_err_logs = false;
>  
>  	/* scale up clocks to max frequency before full reinitialization */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786558726.git.bvanassche@acm.org?part=3