[PATCH 0/2] scsi: ufs: core: Fix SCSI EH command ownership and remove force_compl

[PATCH 0/2] scsi: ufs: core: Fix SCSI EH command ownership and remove force_compl

[Stanley Jhu]

In ufshcd_host_reset_and_restore(), ufshcd_complete_requests(hba, true)
couples LLD resource release (ufshcd_release_scsi_cmd()) with command
completion (scsi_done()) right after ufshcd_hba_stop():

1. EH-owned SCSI commands (SCMD_STATE_COMPLETE set): skipping
   scsi_done() in ufshcd_mcq_force_compl_one() also skips
   ufshcd_release_scsi_cmd(), leaking DMA mappings and
   clk_gating.active_reqs whenever ufshcd_abort() fails.
2. Non-EH SCSI commands (!SCMD_STATE_COMPLETE): calling scsi_done()
   right after ufshcd_hba_stop() completes them before link recovery
   finishes.

This series enforces a strict ownership boundary between LLD hardware
resources and SCSI command completion across SDB and MCQ error recovery:

- LLD resources (DMA mappings, crypto PRDT, clk_gating.active_reqs) are
  tied to controller execution and are released whenever the hardware
  stops executing a command, regardless of whether SCSI EH owns it.
- Command completion (scsi_done()) belongs exclusively to SCSI EH once
  SCMD_STATE_COMPLETE is set. For commands where SCMD_STATE_COMPLETE is
  not set, UFS cannot delegate completion to SCSI EH because UFS also
  performs autonomous resets (ufshcd_err_handler() on UIC/controller
  errors) outside of scsi_error_handler(). Since ufshcd_hba_stop()
  (HCE = 0) wipes all in-flight hardware transfers while SCSI EH is
  inactive, the driver itself requeues halted non-EH commands with
  DID_REQUEUE after recovery finishes so they do not stall for the 30s
  block timeout.

- Patch 1 tracks controller resource ownership with lrbp->in_flight and
  halted non-EH commands with lrbp->pending_requeue, splits host-reset
  cleanup into ufshcd_release_stopped_reqs() (at controller stop) and
  ufshcd_requeue_non_eh_cmds() (after recovery finishes), and aligns
  ownership across ufshcd_compl_one_cqe(), ufshcd_abort(), and
  ufshcd_clear_lu_cmds().
- Patch 2 removes the unused force_compl parameter from
  ufshcd_complete_requests() and ufshcd_mcq_compl_pending_transfer(),
  along with the now-unreachable ufshcd_mcq_force_compl_one() and
  ufshcd_mcq_compl_all_cqes_lock() helpers.

This supersedes the v2 series [3].

Link: https://lore.kernel.org/linux-scsi/eacb6c2a-9219-4e9b-8726-8d38b641f543@acm.org/ [1]
Link: https://lore.kernel.org/linux-scsi/1ddc5181-f547-465c-bfbe-dbf14a91493e@acm.org/ [2]
Link: https://lore.kernel.org/linux-scsi/20260918143809.3034592-1-stanleyjhu@google.com/ [3]
Link: https://lore.kernel.org/linux-scsi/20260920135014.3528082-1-stanleyjhu@google.com/ [4]

Tested:
Verified on QEMU ARM64 (SDB and MCQ) across probe, I/O, reset, and
unbind without UAF or leaks.

Stanley Jhu (2):
  scsi: ufs: core: Release command resources instead of force-completing
  scsi: ufs: core: Remove unused force_compl parameter and MCQ helper

 drivers/ufs/core/ufs-mcq.c      |  26 ----
 drivers/ufs/core/ufshcd-priv.h  |   2 -
 drivers/ufs/core/ufshcd.c       | 213 +++++++++++++++++++++++---------
 include/ufs/ufshcd.h            |   4 +
 4 files changed, 157 insertions(+), 88 deletions(-)

-- 
2.51.0

[PATCH 1/2] scsi: ufs: core: Release command resources instead of force-completing

[Stanley Jhu]

In ufshcd_host_reset_and_restore(), ufshcd_complete_requests(hba, true)
couples LLD resource release (ufshcd_release_scsi_cmd()) with command
completion (scsi_done()) right after ufshcd_hba_stop():

1. EH-owned SCSI commands (SCMD_STATE_COMPLETE set): skipping
   scsi_done() in ufshcd_mcq_force_compl_one() also skips
   ufshcd_release_scsi_cmd(), leaking DMA mappings and
   clk_gating.active_reqs whenever ufshcd_abort() fails.
2. Non-EH SCSI commands (!SCMD_STATE_COMPLETE): calling scsi_done()
   right after ufshcd_hba_stop() completes them before link recovery
   finishes.

To fix this, enforce a strict ownership boundary between LLD hardware
resources and SCSI command completion:

- LLD resources (DMA mappings, crypto PRDT, clk_gating.active_reqs) are
  tied to controller execution and must be released whenever the
  hardware stops executing a command, regardless of whether SCSI EH
  owns the command.
- Command completion (scsi_done()) belongs exclusively to SCSI EH once
  SCMD_STATE_COMPLETE is set. However, for commands where
  SCMD_STATE_COMPLETE is not set, UFS cannot delegate completion to
  SCSI EH because UFS also performs autonomous resets
  (ufshcd_err_handler() on UIC/controller errors) outside of
  scsi_error_handler(). Since ufshcd_hba_stop() (HCE = 0) wipes all
  in-flight hardware transfers while SCSI EH is inactive, the driver
  itself must requeue halted non-EH commands with DID_REQUEUE after
  recovery finishes, or else they stall for the 30s block timeout.

Implement this in three steps:

1. Track controller resource ownership with lrbp->in_flight so
   ufshcd_release_scsi_cmd() is idempotent across normal completion,
   successful task abort, and host reset teardown.
2. At controller stop (ufshcd_release_stopped_reqs()), release LLD
   resources for all halted commands without calling scsi_done(), clear
   any uncompleted reserved dev_cmd, and mark halted non-EH commands
   with lrbp->pending_requeue.
3. After host and link recovery finish (ufshcd_requeue_non_eh_cmds()),
   requeue pending commands that remain non-EH (!SCMD_STATE_COMPLETE)
   with DID_REQUEUE while leaving EH-owned commands on shost->eh_cmd_q.

Fixes: ab248643d3d6 ("scsi: ufs: core: Add error handling for MCQ mode")
Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Stanley Jhu <stanleyjhu@google.com>
---
 drivers/ufs/core/ufshcd.c | 183 ++++++++++++++++++++++++++++++--------
 include/ufs/ufshcd.h      |   6 ++
 2 files changed, 154 insertions(+), 35 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 234e18b5078f..c0772822731e 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2432,6 +2432,7 @@ static inline void ufshcd_send_command(struct ufs_hba *hba,
 		lrbp->compl_time_stamp = ktime_set(0, 0);
 		lrbp->compl_time_stamp_local_clock = 0;
 	}
+	lrbp->in_flight = true;
 	if (ufshcd_is_scsi_cmd(cmd)) {
 		ufshcd_add_command_trace(hba, cmd, UFS_CMD_SEND);
 		ufshcd_clk_scaling_start_busy(hba);
@@ -3116,11 +3117,15 @@ static int ufshcd_init_cmd_priv(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 static enum scsi_qc_status ufshcd_queuecommand(struct Scsi_Host *host,
 					       struct scsi_cmnd *cmd)
 {
+	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
 	struct ufs_hba *hba = shost_priv(host);
 	int tag = scsi_cmd_to_rq(cmd)->tag;
 	int err = 0;
 	struct ufs_hw_queue *hwq = NULL;
 
+	lrbp->in_flight = false;
+	lrbp->pending_requeue = false;
+
 	switch (hba->ufshcd_state) {
 	case UFSHCD_STATE_OPERATIONAL:
 		break;
@@ -3203,6 +3208,8 @@ static enum scsi_qc_status ufshcd_queue_reserved_command(struct Scsi_Host *host,
 	struct ufs_hw_queue *hwq =
 		hba->mcq_enabled ? ufshcd_mcq_req_to_hwq(hba, rq) : NULL;
 
+	lrbp->in_flight = false;
+	lrbp->pending_requeue = false;
 	ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp->ucd_req_ptr);
 	ufshcd_send_command(hba, cmd, hwq);
 	return 0;
@@ -4671,6 +4678,28 @@ int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
 }
 EXPORT_SYMBOL_GPL(ufshcd_uic_change_pwr_mode);
 
+static bool ufshcd_requeue_one_non_eh_cmd(struct request *req, void *data)
+{
+	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
+	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
+
+	if (blk_mq_is_reserved_rq(req) || !lrbp->pending_requeue)
+		return true;
+
+	lrbp->pending_requeue = false;
+	if (!test_bit(SCMD_STATE_COMPLETE, &cmd->state)) {
+		set_host_byte(cmd, DID_REQUEUE);
+		scsi_done(cmd);
+	}
+	return true;
+}
+
+static void ufshcd_requeue_non_eh_cmds(struct ufs_hba *hba)
+{
+	blk_mq_tagset_busy_iter(&hba->host->tag_set,
+				ufshcd_requeue_one_non_eh_cmd, hba);
+}
+
 int ufshcd_link_recovery(struct ufs_hba *hba)
 {
 	int ret;
@@ -4692,6 +4721,8 @@ int ufshcd_link_recovery(struct ufs_hba *hba)
 	ufshcd_clear_eh_in_progress(hba);
 	spin_unlock_irqrestore(&hba->host->host_lock, flags);
 
+	ufshcd_requeue_non_eh_cmds(hba);
+
 	if (ret)
 		dev_err(hba->dev, "%s: link recovery failed, err %d",
 			__func__, ret);
@@ -5937,6 +5968,17 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 /* Release the resources allocated for processing a SCSI command. */
 void ufshcd_release_scsi_cmd(struct ufs_hba *hba, struct scsi_cmnd *cmd)
 {
+	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
+
+	lrbp->pending_requeue = false;
+	if (!lrbp->in_flight)
+		return;
+	lrbp->in_flight = false;
+
+	/* Device management commands do not own any of the below. */
+	if (!ufshcd_is_scsi_cmd(cmd))
+		return;
+
 	scsi_dma_unmap(cmd);
 	ufshcd_crypto_clear_prdt(hba, cmd);
 	ufshcd_release(hba);
@@ -5968,8 +6010,9 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
 		if (unlikely(ufshcd_should_inform_monitor(hba, cmd)))
 			ufshcd_update_monitor(hba, cmd);
 		ufshcd_add_command_trace(hba, cmd, UFS_CMD_COMP);
-		cmd->result = ufshcd_transfer_rsp_status(hba, cmd, cqe);
-		ufshcd_release_scsi_cmd(hba, cmd);
+		if (!test_bit(SCMD_STATE_COMPLETE, &cmd->state))
+			cmd->result =
+				ufshcd_transfer_rsp_status(hba, cmd, cqe);
 	} else {
 		if (cqe) {
 			ocs = cqe->overall_status & MASK_OCS;
@@ -5981,8 +6024,17 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
 			hba,
 			ocs == OCS_SUCCESS ? UFS_QUERY_COMP : UFS_QUERY_ERR,
 			(struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
-		cmd->result = 0;
+		if (!test_bit(SCMD_STATE_COMPLETE, &cmd->state))
+			cmd->result = 0;
 	}
+	ufshcd_release_scsi_cmd(hba, cmd);
+	/*
+	 * Clear lrbp->pending_requeue before the tag is freed so that the
+	 * next user of this tag is not mistaken for a command halted by
+	 * ufshcd_hba_stop().
+	 */
+	lrbp->pending_requeue = false;
+
 	/* Do not touch lrbp after scsi_done() has been called. */
 	scsi_done(cmd);
 }
@@ -7912,6 +7964,8 @@ static bool ufshcd_clear_lu_cmds(struct request *req, void *priv)
 	struct ufs_hba *hba = shost_priv(shost);
 	const u64 lun = *(u64 *)priv;
 	const u32 tag = req->tag;
+	unsigned long flags;
+	bool outstanding;
 
 	if (blk_mq_is_reserved_rq(req) || sdev->lun != lun)
 		return true;
@@ -7930,7 +7984,11 @@ static bool ufshcd_clear_lu_cmds(struct request *req, void *priv)
 		return true;
 	}
 
-	ufshcd_compl_one_cqe(hba, tag, NULL);
+	spin_lock_irqsave(&hba->outstanding_lock, flags);
+	outstanding = __test_and_clear_bit(tag, &hba->outstanding_reqs);
+	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
+	if (outstanding)
+		ufshcd_compl_one_cqe(hba, tag, NULL);
 	return true;
 }
 
@@ -8087,6 +8145,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 	u32 reg;
 
 	ufshcd_hold(hba);
+	lrbp->pending_requeue = false;
 
 	if (!hba->mcq_enabled) {
 		reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
@@ -8153,6 +8212,16 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 	if (hba->mcq_enabled) {
 		/* MCQ mode. Branch off to handle abort for mcq mode */
 		err = ufshcd_mcq_abort(cmd);
+		if (err == SUCCESS) {
+			struct ufs_hw_queue *hwq =
+				ufshcd_mcq_req_to_hwq(hba, rq);
+
+			if (hwq) {
+				spin_lock_irqsave(&hwq->cq_lock, flags);
+				ufshcd_release_scsi_cmd(hba, cmd);
+				spin_unlock_irqrestore(&hwq->cq_lock, flags);
+			}
+		}
 		goto release;
 	}
 
@@ -8216,6 +8285,71 @@ static void ufshcd_process_probe_result(struct ufs_hba *hba,
 			  hba->curr_dev_pwr_mode, hba->uic_link_state);
 }
 
+/*
+ * The caller must own @rq, either by having claimed its bit in
+ * hba->outstanding_reqs (legacy mode) or by holding hwq->cq_lock (MCQ mode).
+ */
+static void __ufshcd_release_stopped_req(struct ufs_hba *hba,
+					 struct request *rq,
+					 struct scsi_cmnd *cmd)
+{
+	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
+
+	if (!lrbp->in_flight)
+		return;
+
+	ufshcd_release_scsi_cmd(hba, cmd);
+	if (test_bit(SCMD_STATE_COMPLETE, &cmd->state))
+		return;
+
+	if (blk_mq_is_reserved_rq(rq)) {
+		/* Device management commands are not requeued. */
+		set_host_byte(cmd, DID_TIME_OUT);
+		scsi_done(cmd);
+	} else {
+		lrbp->pending_requeue = true;
+	}
+}
+
+static bool ufshcd_release_one_stopped_req(struct request *rq, void *priv)
+{
+	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
+	struct ufs_hba *hba = priv;
+	unsigned long flags;
+	bool owned;
+
+	if (hba->mcq_enabled) {
+		struct ufs_hw_queue *hwq = ufshcd_mcq_req_to_hwq(hba, rq);
+
+		if (hwq) {
+			spin_lock_irqsave(&hwq->cq_lock, flags);
+			__ufshcd_release_stopped_req(hba, rq, cmd);
+			spin_unlock_irqrestore(&hwq->cq_lock, flags);
+		}
+		return true;
+	}
+
+	/*
+	 * Keep hba->outstanding_lock a leaf lock, as in ufshcd_poll() and
+	 * ufshcd_abort(): clearing the bit already grants exclusive ownership.
+	 */
+	spin_lock_irqsave(&hba->outstanding_lock, flags);
+	owned = __test_and_clear_bit(rq->tag, &hba->outstanding_reqs);
+	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
+	if (owned)
+		__ufshcd_release_stopped_req(hba, rq, cmd);
+
+	return true;
+}
+
+static void ufshcd_release_stopped_reqs(struct ufs_hba *hba)
+{
+	blk_mq_tagset_busy_iter(&hba->host->tag_set,
+				ufshcd_release_one_stopped_req, hba);
+	/* TMF sweep inherited from the replaced ufshcd_complete_requests(). */
+	ufshcd_tmc_handler(hba);
+}
+
 /**
  * ufshcd_host_reset_and_restore - reset and restore host controller
  * @hba: per-adapter instance
@@ -8231,13 +8365,11 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
 	int err;
 
 	/*
-	 * Stop the host controller and complete the requests
-	 * cleared by h/w
+	 * Stop the host controller and release driver resources for
+	 * requests cleared by h/w
 	 */
 	ufshcd_hba_stop(hba);
-	hba->silence_err_logs = true;
-	ufshcd_complete_requests(hba, true);
-	hba->silence_err_logs = false;
+	ufshcd_release_stopped_reqs(hba);
 
 	/* scale up clocks to max frequency before full reinitialization */
 	if (ufshcd_is_clkscaling_supported(hba))
@@ -8319,6 +8451,8 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba)
 	}
 	spin_unlock_irqrestore(&hba->host->host_lock, flags);
 
+	ufshcd_requeue_non_eh_cmds(hba);
+
 	return err;
 }
 
@@ -9597,30 +9731,6 @@ static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
 	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);
-
-			spin_lock_irqsave(&hba->outstanding_lock, flags);
-			__clear_bit(rq->tag, &hba->outstanding_reqs);
-			spin_unlock_irqrestore(&hba->outstanding_lock, flags);
-		}
-
-		if (ufshcd_is_scsi_cmd(scmd)) {
-			set_host_byte(scmd, DID_REQUEUE);
-			ufshcd_release_scsi_cmd(hba, scmd);
-		} else {
-			set_host_byte(scmd, DID_TIME_OUT);
-		}
-
-		scsi_done(scmd);
-	}
-
 	return SCSI_EH_DONE;
 }
 
@@ -10464,15 +10574,18 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 		ufshcd_device_reset(hba);
 		WARN_ON(!ufshcd_is_link_off(hba));
 	}
-	if (ufshcd_is_link_hibern8(hba) && !ufshcd_uic_hibern8_exit(hba))
+	if (ufshcd_is_link_hibern8(hba) && !ufshcd_uic_hibern8_exit(hba)) {
 		ufshcd_set_link_active(hba);
-	else if (ufshcd_is_link_off(hba))
+	} else if (ufshcd_is_link_off(hba)) {
 		ufshcd_host_reset_and_restore(hba);
+		ufshcd_requeue_non_eh_cmds(hba);
+	}
 set_dev_active:
 	/* Can also get here needing to exit DeepSleep */
 	if (ufshcd_is_ufs_dev_deepsleep(hba)) {
 		ufshcd_device_reset(hba);
 		ufshcd_host_reset_and_restore(hba);
+		ufshcd_requeue_non_eh_cmds(hba);
 	}
 	if (!ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE))
 		ufshcd_disable_auto_bkops(hba);
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index dfd302f2dc7c..cd84f617f95c 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -166,6 +166,10 @@ struct ufs_pm_lvl_states {
  * @lun: LUN of the command
  * @intr_cmd: Interrupt command (doesn't participate in interrupt aggregation)
  * @req_abort_skip: skip request abort task flag
+ * @in_flight: true after the command has been submitted to the controller and
+ *	       before its resources have been released
+ * @pending_requeue: true if command was halted by ufshcd_hba_stop() and awaits
+ *		     post-recovery requeue
  * @issue_time_stamp: time stamp for debug purposes (CLOCK_MONOTONIC)
  * @issue_time_stamp_local_clock: time stamp for debug purposes (local_clock)
  * @compl_time_stamp: time stamp for statistics (CLOCK_MONOTONIC)
@@ -190,6 +194,8 @@ struct ufshcd_lrb {
 	u8 lun; /* UPIU LUN id field is only 8-bit wide */
 	bool intr_cmd;
 	bool req_abort_skip;
+	bool in_flight;
+	bool pending_requeue;
 	ktime_t issue_time_stamp;
 	u64 issue_time_stamp_local_clock;
 	ktime_t compl_time_stamp;
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH 2/2] scsi: ufs: core: Remove unused force_compl parameter and MCQ helper

[Stanley Jhu]

With ufshcd_host_reset_and_restore() converted to call
ufshcd_release_stopped_reqs(), both remaining callers of
ufshcd_complete_requests() (ufshcd_abort_all() and
ufshcd_err_handler()) pass force_compl = false.

Remove the unused force_compl parameter from ufshcd_complete_requests()
and ufshcd_mcq_compl_pending_transfer(), and delete the now-unreachable
ufshcd_mcq_force_compl_one() and ufshcd_mcq_compl_all_cqes_lock()
helpers.

No functional change intended.

Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Stanley Jhu <stanleyjhu@google.com>
---
 drivers/ufs/core/ufs-mcq.c     | 26 -----------------
 drivers/ufs/core/ufshcd-priv.h |  2 --
 drivers/ufs/core/ufshcd.c      | 52 +++++-----------------------------
 3 files changed, 7 insertions(+), 73 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 c0772822731e..b7492a5af616 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -6109,34 +6109,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;
@@ -6151,22 +6123,12 @@ static bool ufshcd_mcq_compl_one(struct request *rq, void *priv)
 }
 
 /**
- * ufshcd_mcq_compl_pending_transfer - MCQ mode function. It is
- * invoked from the error handler context or ufshcd_host_reset_and_restore()
- * to complete the pending transfers and free the resources associated with
- * the scsi command.
- *
+ * ufshcd_mcq_compl_pending_transfer - Complete pending MCQ transfers from EH
  * @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);
 }
 
@@ -6723,10 +6685,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);
 
@@ -7027,7 +6989,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;
 }
@@ -7089,7 +7051,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;
-- 
2.55.0.1082.g2b9226bbc0-goog

Re: [PATCH 1/2] scsi: ufs: core: Release command resources instead of force-completing

[Bart Van Assche]

On 9/20/26 7:33 AM, Stanley Jhu wrote:
> In ufshcd_host_reset_and_restore(), ufshcd_complete_requests(hba, true)
> couples LLD resource release (ufshcd_release_scsi_cmd()) with command
> completion (scsi_done()) right after ufshcd_hba_stop():
> 
> 1. EH-owned SCSI commands (SCMD_STATE_COMPLETE set): skipping
>     scsi_done() in ufshcd_mcq_force_compl_one() also skips
>     ufshcd_release_scsi_cmd(), leaking DMA mappings and
>     clk_gating.active_reqs whenever ufshcd_abort() fails.
> 2. Non-EH SCSI commands (!SCMD_STATE_COMPLETE): calling scsi_done()
>     right after ufshcd_hba_stop() completes them before link recovery
>     finishes.
> 
> To fix this, enforce a strict ownership boundary between LLD hardware
> resources and SCSI command completion:
> 
> - LLD resources (DMA mappings, crypto PRDT, clk_gating.active_reqs) are
>    tied to controller execution and must be released whenever the
>    hardware stops executing a command, regardless of whether SCSI EH
>    owns the command.
> - Command completion (scsi_done()) belongs exclusively to SCSI EH once
>    SCMD_STATE_COMPLETE is set. However, for commands where
>    SCMD_STATE_COMPLETE is not set, UFS cannot delegate completion to
>    SCSI EH because UFS also performs autonomous resets
>    (ufshcd_err_handler() on UIC/controller errors) outside of
>    scsi_error_handler(). Since ufshcd_hba_stop() (HCE = 0) wipes all
>    in-flight hardware transfers while SCSI EH is inactive, the driver
>    itself must requeue halted non-EH commands with DID_REQUEUE after
>    recovery finishes, or else they stall for the 30s block timeout.
> 
> Implement this in three steps:
> 
> 1. Track controller resource ownership with lrbp->in_flight so
>     ufshcd_release_scsi_cmd() is idempotent across normal completion,
>     successful task abort, and host reset teardown.
> 2. At controller stop (ufshcd_release_stopped_reqs()), release LLD
>     resources for all halted commands without calling scsi_done(), clear
>     any uncompleted reserved dev_cmd, and mark halted non-EH commands
>     with lrbp->pending_requeue.
> 3. After host and link recovery finish (ufshcd_requeue_non_eh_cmds()),
>     requeue pending commands that remain non-EH (!SCMD_STATE_COMPLETE)
>     with DID_REQUEUE while leaving EH-owned commands on shost->eh_cmd_q.
> 
> Fixes: ab248643d3d6 ("scsi: ufs: core: Add error handling for MCQ mode")
> Suggested-by: Bart Van Assche <bvanassche@acm.org>

No, this is not what I suggested. Additionally, I see multiple steps in
the wrong direction.

The text above says "implement this in three steps". Hence, this patch
should have been three patches instead of one because of the "one change
per patch" rule in the Linux kernel community.

Regarding (1), why a new member variable? Why isn't rq->state
sufficient?

Regarding (3), the UFS driver is a SCSI LLD (low-level driver) and hence
should only do what is specific to the UFS driver. Completing commands
after .eh_host_reset_handler() has been called is the responsibility of
the SCSI core and should not be done by the UFS driver.

(2) seems wrong to me. The caller of ufshcd_hba_stop() should take care
of pending commands instead of ufshcd_hba_stop() itself.

>   int ufshcd_link_recovery(struct ufs_hba *hba)
>   {
>   	int ret;
> @@ -4692,6 +4721,8 @@ int ufshcd_link_recovery(struct ufs_hba *hba)
>   	ufshcd_clear_eh_in_progress(hba);
>   	spin_unlock_irqrestore(&hba->host->host_lock, flags);
>   
> +	ufshcd_requeue_non_eh_cmds(hba);
> +
>   	if (ret)
>   		dev_err(hba->dev, "%s: link recovery failed, err %d",
>   			__func__, ret);

This is a behavior change that should be in a separate patch and not in
a patch for releasing command resources.

> @@ -5937,6 +5968,17 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
>   /* Release the resources allocated for processing a SCSI command. */
>   void ufshcd_release_scsi_cmd(struct ufs_hba *hba, struct scsi_cmnd *cmd)
>   {
> +	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
> +
> +	lrbp->pending_requeue = false;
> +	if (!lrbp->in_flight)
> +		return;
> +	lrbp->in_flight = false;
> +
> +	/* Device management commands do not own any of the below. */
> +	if (!ufshcd_is_scsi_cmd(cmd))
> +		return;
> +

Did you read this code before you posted it? As its name suggests,
ufshcd_release_scsi_cmd() is only called for SCSI commands. No device
management command code should be added in this function.

>   	scsi_dma_unmap(cmd);
>   	ufshcd_crypto_clear_prdt(hba, cmd);
>   	ufshcd_release(hba);
> @@ -5968,8 +6010,9 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
>   		if (unlikely(ufshcd_should_inform_monitor(hba, cmd)))
>   			ufshcd_update_monitor(hba, cmd);
>   		ufshcd_add_command_trace(hba, cmd, UFS_CMD_COMP);
> -		cmd->result = ufshcd_transfer_rsp_status(hba, cmd, cqe);
> -		ufshcd_release_scsi_cmd(hba, cmd);
> +		if (!test_bit(SCMD_STATE_COMPLETE, &cmd->state))
> +			cmd->result =
> +				ufshcd_transfer_rsp_status(hba, cmd, cqe);

Why has this if-condition been added? ufshcd_compl_one_cqe() should only
be called if SCMD_STATE_COMPLETE has not yet been set.

> @@ -5981,8 +6024,17 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
>   			hba,
>   			ocs == OCS_SUCCESS ? UFS_QUERY_COMP : UFS_QUERY_ERR,
>   			(struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
> -		cmd->result = 0;
> +		if (!test_bit(SCMD_STATE_COMPLETE, &cmd->state))
> +			cmd->result = 0;

Same question for this new if-condition.

> @@ -7930,7 +7984,11 @@ static bool ufshcd_clear_lu_cmds(struct request *req, void *priv)
>   		return true;
>   	}
>   
> -	ufshcd_compl_one_cqe(hba, tag, NULL);
> +	spin_lock_irqsave(&hba->outstanding_lock, flags);
> +	outstanding = __test_and_clear_bit(tag, &hba->outstanding_reqs);
> +	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
> +	if (outstanding)
> +		ufshcd_compl_one_cqe(hba, tag, NULL);
>   	return true;
>   }

Why has ufshcd_clear_lu_cmds() been modified? This function is only
called for pending commands so checking &hba->outstanding_reqs is not
necessary.

> @@ -8087,6 +8145,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>   	u32 reg;
>   
>   	ufshcd_hold(hba);
> +	lrbp->pending_requeue = false;
>   
>   	if (!hba->mcq_enabled) {
>   		reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> @@ -8153,6 +8212,16 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>   	if (hba->mcq_enabled) {
>   		/* MCQ mode. Branch off to handle abort for mcq mode */
>   		err = ufshcd_mcq_abort(cmd);
> +		if (err == SUCCESS) {
> +			struct ufs_hw_queue *hwq =
> +				ufshcd_mcq_req_to_hwq(hba, rq);
> +
> +			if (hwq) {
> +				spin_lock_irqsave(&hwq->cq_lock, flags);
> +				ufshcd_release_scsi_cmd(hba, cmd);
> +				spin_unlock_irqrestore(&hwq->cq_lock, flags);
> +			}
> +		}

This is also wrong because this change will trigger a use-after-free in
scmd_eh_abort_handler() if it decides to retry or finish a SCSI command.

> @@ -8231,13 +8365,11 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
>   	int err;
>   
>   	/*
> -	 * Stop the host controller and complete the requests
> -	 * cleared by h/w
> +	 * Stop the host controller and release driver resources for
> +	 * requests cleared by h/w

This is wrong. The SCSI core error handler is responsible for releasing
resources. The UFS driver must not do this from its host reset handler.

Bart.