[PATCH] scsi: ufs: core: Avoid unsafe MMIO reads in ufshcd_mcq_compl_all_cqes_lock()

[PATCH] scsi: ufs: core: Avoid unsafe MMIO reads in ufshcd_mcq_compl_all_cqes_lock()

[Stanley Jhu]

Reading host controller registers (such as CQTP) is unsafe when the
host controller is disabled (HCE = 0), as accessing registers in an
unclocked or reset state can cause bus stalls and system hangs.

In ufshcd_mcq_compl_all_cqes_lock(), all completion queue entries have
already been inspected, processed, and cleared in memory, so the
software queue is logically empty.

Avoid the unsafe MMIO read of CQTP by synchronizing hwq->cq_tail_slot
directly to hwq->cq_head_slot in software. Upon subsequent controller
re-initialization, ufshcd_mcq_make_queues_operational() will
reconfigure and re-zero all queue pointers.

Fixes: ab248643d3d6 ("scsi: ufs: core: Add error handling for MCQ mode")
Cc: stable@vger.kernel.org
Signed-off-by: Stanley Jhu <stanleyjhu@google.com>
---
 drivers/ufs/core/ufs-mcq.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 8106d55f4041..0e1f99c8d77c 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -349,8 +349,12 @@ void ufshcd_mcq_compl_all_cqes_lock(struct ufs_hba *hba,
                entries--;
        }

-       ufshcd_mcq_update_cq_tail_slot(hwq);
-       hwq->cq_head_slot = hwq->cq_tail_slot;
+       /*
+        * All completion entries have been processed and cleared.
+        * Synchronize tail to head in software to mark the queue empty,
+        * avoiding unsafe MMIO reads while the controller is disabled.
+        */
+       hwq->cq_tail_slot = hwq->cq_head_slot;
        spin_unlock_irqrestore(&hwq->cq_lock, flags);
 }

--
2.43.0

[PATCH v2 0/2] scsi: ufs: core: Fix unsafe MMIO reads and redundant CQ sweeps in MCQ reset

[Stanley Jhu]

During Multi-Circular Queue (MCQ) error recovery and host reset,
ufshcd_mcq_compl_pending_transfer() sweeps or polls completion queues to
reap pending transfers. Two bugs exist in this path:

1. Unsafe MMIO read and spurious errors while HCE = 0 (Patch 1/2):
   ufshcd_host_reset_and_restore() stops the controller (HCE = 0) before
   calling ufshcd_mcq_compl_all_cqes_lock(). Calling
   ufshcd_mcq_update_cq_tail_slot() at the end of the sweep reads CQTPy
   over MMIO while HCE = 0, directly contradicting the function's own
   documented contract that reading host controller registers is unsafe
   when the controller is disabled. In addition, passing expected empty
   slots during a full-ring sweep into ufshcd_mcq_process_cqe() prints
   spurious "Abnormal CQ entry!" errors.

2. Redundant per-request CQ sweeps and polls (Patch 2/2):
   ufshcd_mcq_compl_pending_transfer() runs hardware queue completion
   sweeps (force_compl == true) or CQTPy polls (force_compl == false)
   inside blk_mq_tagset_busy_iter() callbacks, repeating whole-queue
   operations once per busy request instead of once per hardware queue.

Patch 1/2 synchronizes hwq->cq_tail_slot = hwq->cq_head_slot in software
and extracts ufshcd_mcq_compl_cqe() so full-ring sweeps skip empty slots
silently. Patch 2/2 sweeps or polls each hardware queue once before
iterating residual requests and removes ufshcd_mcq_compl_one().

Changes since v1:
- Split into a two-patch series separating ring sweep safety from
  per-request tagset iteration.
- Extract ufshcd_mcq_compl_cqe() to skip empty slots without double CQE
  checks (dropped Peter Wang's v1 Reviewed-by due to this change).
- Decouple hardware queue polling/sweeping for both force_compl paths
  and remove ufshcd_mcq_compl_one().

Tested: Verified MCQ host reset, I/O completion, and queue pointer
  integrity on QEMU ARM64 without MMIO aborts or spurious error logs.

Link: https://lore.kernel.org/r/CAE14pdek6ynze+muDZrK+yNX-3ioe3vprxOA4W22qokg352tJQ@mail.gmail.com

Stanley Jhu (2):
  scsi: ufs: core: Avoid unsafe MMIO reads in
    ufshcd_mcq_compl_all_cqes_lock()
  scsi: ufs: core: Decouple CQ sweep from request iterator in MCQ

 drivers/ufs/core/ufs-mcq.c | 35 +++++++++++++++++++++++------------
 drivers/ufs/core/ufshcd.c  | 31 ++++++++++++-------------------
 2 files changed, 35 insertions(+), 31 deletions(-)

-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH v2 1/2] scsi: ufs: core: Avoid unsafe MMIO reads in ufshcd_mcq_compl_all_cqes_lock()

[Stanley Jhu]

During MCQ host reset, ufshcd_host_reset_and_restore() stops the host
controller via ufshcd_hba_stop() (HCE = 0) before calling
ufshcd_complete_requests(hba, true) ->
ufshcd_mcq_compl_pending_transfer(hba, true) ->
ufshcd_mcq_force_compl_one() -> ufshcd_mcq_compl_all_cqes_lock().
Because ufshcd_mcq_force_compl_one() is its sole caller,
ufshcd_mcq_compl_all_cqes_lock() always runs with HCE = 0.

Despite the comment above ufshcd_mcq_compl_all_cqes_lock() stating that
reading CQTPy may not be safe with the controller disabled, the function
still calls ufshcd_mcq_update_cq_tail_slot() at the end of its sweep:

1. Unsafe CQTPy MMIO read:
   Calling ufshcd_mcq_update_cq_tail_slot() at the end of the sweep
   reads CQTPy over MMIO while HCE = 0, directly contradicting the
   function's documented contract (commit 1373df88d535 ("scsi: ufs:
   core: Add a comment block above ufshcd_mcq_compl_all_cqes_lock()"))
   that reading CQTPy may not be safe with the controller disabled.

2. Spurious error logs on empty slots:
   Sweeping all max_entries slots visits empty entries where
   command_desc_base_addr is 0, causing ufshcd_mcq_process_cqe() to log
   unguarded dev_err(hba->dev, "Abnormal CQ entry!\n") messages.

Fix both issues in ufshcd_mcq_compl_all_cqes_lock():
- Synchronize hwq->cq_tail_slot = hwq->cq_head_slot in software after
  sweeping the ring, avoiding CQTPy MMIO reads while HCE = 0.
- Extract ufshcd_mcq_compl_cqe() and invoke it only on non-empty slots
  during full-ring sweeps, keeping "Abnormal CQ entry!" logging strictly
  for unexpected empty entries in ufshcd_mcq_poll_cqe_lock().

Fixes: ab248643d3d6 ("scsi: ufs: core: Add error handling for MCQ mode")
Cc: stable@vger.kernel.org
Signed-off-by: Stanley Jhu <stanleyjhu@google.com>
---
v2:
- Extract ufshcd_mcq_compl_cqe() to skip empty slots inside
  ufshcd_mcq_compl_all_cqes_lock() without double CQE checks (dropped
  Peter Wang's v1 Reviewed-by due to this code change).
- Move hardware queue polling and sweeping deduplication to Patch 2/2.

Link: https://lore.kernel.org/r/CAE14pdek6ynze+muDZrK+yNX-3ioe3vprxOA4W22qokg352tJQ@mail.gmail.com

 drivers/ufs/core/ufs-mcq.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 8106d55f4041..bfc43a6080e7 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -312,20 +312,24 @@ static int ufshcd_mcq_get_tag(struct ufs_hba *hba, struct cq_entry *cqe)
 		UFSHCD_NUM_RESERVED;
 }
 
+static void ufshcd_mcq_compl_cqe(struct ufs_hba *hba, struct cq_entry *cqe)
+{
+	int tag = ufshcd_mcq_get_tag(hba, cqe);
+
+	ufshcd_compl_one_cqe(hba, tag, cqe);
+	/* After processed the cqe, mark it empty (invalid) entry */
+	cqe->command_desc_base_addr = 0;
+}
+
 static void ufshcd_mcq_process_cqe(struct ufs_hba *hba,
 				   struct ufs_hw_queue *hwq)
 {
 	struct cq_entry *cqe = ufshcd_mcq_cur_cqe(hwq);
 
-	if (cqe->command_desc_base_addr) {
-		int tag = ufshcd_mcq_get_tag(hba, cqe);
-
-		ufshcd_compl_one_cqe(hba, tag, cqe);
-		/* After processed the cqe, mark it empty (invalid) entry */
-		cqe->command_desc_base_addr = 0;
-	} else {
+	if (cqe->command_desc_base_addr)
+		ufshcd_mcq_compl_cqe(hba, cqe);
+	else
 		dev_err(hba->dev, "Abnormal CQ entry!\n");
-	}
 }
 
 /*
@@ -333,7 +337,7 @@ static void ufshcd_mcq_process_cqe(struct ufs_hba *hba,
  * 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
+ * double completions because ufshcd_mcq_compl_cqe() clears a CQE after it
  * has been processed.
  */
 void ufshcd_mcq_compl_all_cqes_lock(struct ufs_hba *hba,
@@ -344,13 +348,20 @@ void ufshcd_mcq_compl_all_cqes_lock(struct ufs_hba *hba,
 
 	spin_lock_irqsave(&hwq->cq_lock, flags);
 	while (entries > 0) {
-		ufshcd_mcq_process_cqe(hba, hwq);
+		struct cq_entry *cqe = ufshcd_mcq_cur_cqe(hwq);
+
+		if (cqe->command_desc_base_addr)
+			ufshcd_mcq_compl_cqe(hba, cqe);
 		ufshcd_mcq_inc_cq_head_slot(hwq);
 		entries--;
 	}
 
-	ufshcd_mcq_update_cq_tail_slot(hwq);
-	hwq->cq_head_slot = hwq->cq_tail_slot;
+	/*
+	 * All completion entries have been processed and cleared.
+	 * Synchronize tail to head in software to mark the queue empty,
+	 * avoiding an MMIO read of CQTPy while the controller is disabled.
+	 */
+	hwq->cq_tail_slot = hwq->cq_head_slot;
 	spin_unlock_irqrestore(&hwq->cq_lock, flags);
 }
 
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH v2 2/2] scsi: ufs: core: Decouple CQ sweep from request iterator in MCQ

[Stanley Jhu]

In MCQ mode, ufshcd_mcq_compl_pending_transfer() uses
blk_mq_tagset_busy_iter() to iterate over busy requests during error
recovery and host reset. However, both iterator callbacks perform
whole-queue operations redundantly for each visited request:

- force_compl == true: ufshcd_mcq_force_compl_one() calls
  ufshcd_mcq_compl_all_cqes_lock() on every busy request, sweeping the
  entire completion ring (hwq->max_entries slots) once per active
  request under spin_lock_irqsave even though the first sweep already
  cleared all completion entries.
- force_compl == false: ufshcd_mcq_compl_one() acquires cq_lock and
  polls CQTPy over MMIO via ufshcd_mcq_poll_cqe_lock() for every busy
  request without doing any per-request work.

Sweep or poll each hardware queue (hba->uhq[i]) once at the start of
ufshcd_mcq_compl_pending_transfer(). When force_compl is true, run
blk_mq_tagset_busy_iter() afterward to complete residual in-flight
requests with DID_REQUEUE, and remove the now-unused
ufshcd_mcq_compl_one() callback.

Fixes: ab248643d3d6 ("scsi: ufs: core: Add error handling for MCQ mode")
Cc: stable@vger.kernel.org
Signed-off-by: Stanley Jhu <stanleyjhu@google.com>
---
 drivers/ufs/core/ufshcd.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 2ba244cf40ac..a69dcb04d985 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -6044,8 +6044,6 @@ static bool ufshcd_mcq_force_compl_one(struct request *rq, void *priv)
 	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.
@@ -6061,19 +6059,6 @@ static bool ufshcd_mcq_force_compl_one(struct request *rq, void *priv)
 	return true;
 }
 
-static bool ufshcd_mcq_compl_one(struct request *rq, void *priv)
-{
-	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)
-		ufshcd_mcq_poll_cqe_lock(hba, hwq);
-
-	return true;
-}
-
 /**
  * ufshcd_mcq_compl_pending_transfer - MCQ mode function. It is
  * invoked from the error handler context or ufshcd_host_reset_and_restore()
@@ -6088,10 +6073,18 @@ static bool ufshcd_mcq_compl_one(struct request *rq, void *priv)
 static void ufshcd_mcq_compl_pending_transfer(struct ufs_hba *hba,
 					      bool force_compl)
 {
-	blk_mq_tagset_busy_iter(&hba->host->tag_set,
-				force_compl ? ufshcd_mcq_force_compl_one :
-					      ufshcd_mcq_compl_one,
-				NULL);
+	int i;
+
+	for (i = 0; i < hba->nr_hw_queues; i++) {
+		if (force_compl)
+			ufshcd_mcq_compl_all_cqes_lock(hba, &hba->uhq[i]);
+		else
+			ufshcd_mcq_poll_cqe_lock(hba, &hba->uhq[i]);
+	}
+
+	if (force_compl)
+		blk_mq_tagset_busy_iter(&hba->host->tag_set,
+					ufshcd_mcq_force_compl_one, NULL);
 }
 
 /**
-- 
2.55.0.1082.g2b9226bbc0-goog

Re: [PATCH v2 2/2] scsi: ufs: core: Decouple CQ sweep from request iterator in MCQ

[Bart Van Assche]

On 9/18/26 7:38 AM, Stanley Jhu wrote:
> In MCQ mode, ufshcd_mcq_compl_pending_transfer() uses
> blk_mq_tagset_busy_iter() to iterate over busy requests during error
> recovery and host reset. However, both iterator callbacks perform
> whole-queue operations redundantly for each visited request:
> 
> - force_compl == true: ufshcd_mcq_force_compl_one() calls
>    ufshcd_mcq_compl_all_cqes_lock() on every busy request, sweeping the
>    entire completion ring (hwq->max_entries slots) once per active
>    request under spin_lock_irqsave even though the first sweep already
>    cleared all completion entries.
> - force_compl == false: ufshcd_mcq_compl_one() acquires cq_lock and
>    polls CQTPy over MMIO via ufshcd_mcq_poll_cqe_lock() for every busy
>    request without doing any per-request work.
> 
> Sweep or poll each hardware queue (hba->uhq[i]) once at the start of
> ufshcd_mcq_compl_pending_transfer(). When force_compl is true, run
> blk_mq_tagset_busy_iter() afterward to complete residual in-flight
> requests with DID_REQUEUE, and remove the now-unused
> ufshcd_mcq_compl_one() callback.

Forcibly completing SCSI commands from inside the UFS SCSI host reset
error handling callback is incompatible with the SCSI core error
handler. The "force_compl" behavior should be removed instead of
reworking it. If you take a look at the SDB (single doorbell) code you
will see that forcibly completing requests doesn't happen for SDB mode.

Bart.

Re: [PATCH] scsi: ufs: core: Avoid unsafe MMIO reads in ufshcd_mcq_compl_all_cqes_lock()

[Bart Van Assche]

On 9/1/26 5:23 AM, Stanley Jhu wrote:
> Reading host controller registers (such as CQTP) is unsafe when the
> host controller is disabled (HCE = 0), as accessing registers in an
> unclocked or reset state can cause bus stalls and system hangs.
> 
> In ufshcd_mcq_compl_all_cqes_lock(), all completion queue entries have
> already been inspected, processed, and cleared in memory, so the
> software queue is logically empty.
> 
> Avoid the unsafe MMIO read of CQTP by synchronizing hwq->cq_tail_slot
> directly to hwq->cq_head_slot in software. Upon subsequent controller
> re-initialization, ufshcd_mcq_make_queues_operational() will
> reconfigure and re-zero all queue pointers.
ufshcd_mcq_compl_all_cqes_lock() is only used to forcibly complete UFS
commands. The UFS driver should not forcibly complete UFS commands from
inside its error handler. Please remove the
ufshcd_mcq_compl_all_cqes_lock() function.

Thanks,

Bart.