[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

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

[Peter Wang]

On Fri, 2026-09-18 at 22:38 +0800, Stanley Jhu wrote:
> 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>
> ---

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

[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 v2 2/2] scsi: ufs: core: Decouple CQ sweep from request iterator in MCQ

[Stanley Jhu]

On 9/18/26 9:06 AM, Bart Van Assche wrote:
> 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.

Note that ufshcd_mcq_force_compl_one() already checks
!test_bit(SCMD_STATE_COMPLETE, &cmd->state), so it only completes non-EH
commands (e.g. during an autonomous ufshcd_err_handler() reset); in SDB
mode, ufshcd_hba_stop() (HCE = 0) clears UTRLDBR to 0, so ufshcd_poll()
similarly treats all outstanding_reqs as completed right after
ufshcd_hba_stop().

That said, doing this inside ufshcd_host_reset_and_restore() right after
ufshcd_hba_stop() is indeed the wrong place: at controller stop time we
should only release LLD resources (ufshcd_release_scsi_cmd()), and
requeue any remaining non-EH (!SCMD_STATE_COMPLETE) commands with
DID_REQUEUE only after host/link recovery finishes (so autonomous resets
neither wake callers mid-reset nor leave in-flight I/O stalled for the
30s block layer timeout, similar to autonomous reset handling in
hisi_sas, megaraid_sas, and smartpqi).

Let's drop this v2 series. I will send a separate 2-patch series shortly
to replace this and remove force_compl and
ufshcd_mcq_compl_all_cqes_lock().

Thanks,

Stanley

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

[Bart Van Assche]

On 9/20/26 6:49 AM, Stanley Jhu wrote:
> On 9/18/26 9:06 AM, Bart Van Assche wrote:
>> 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.
> 
> Note that ufshcd_mcq_force_compl_one() already checks
> !test_bit(SCMD_STATE_COMPLETE, &cmd->state), so it only completes non-EH
> commands (e.g. during an autonomous ufshcd_err_handler() reset);

What are "non-EH" commands? The SCSI error handler only starts its
error handling strategy after all pending commands have either timed
out or completed.

I think that you are misunderstanding the code. The purpose of the
SCMD_STATE_COMPLETE check is to prevent double completions of SCSI
commands.

> in SDB mode, ufshcd_hba_stop() (HCE = 0) clears UTRLDBR to 0, so
> ufshcd_poll() similarly treats all outstanding_reqs as completed
> right after ufshcd_hba_stop().
> 
> That said, doing this inside ufshcd_host_reset_and_restore() right after
> ufshcd_hba_stop() is indeed the wrong place: at controller stop time we
> should only release LLD resources (ufshcd_release_scsi_cmd()), and
> requeue any remaining non-EH (!SCMD_STATE_COMPLETE) commands with
> DID_REQUEUE only after host/link recovery finishes (so autonomous resets
> neither wake callers mid-reset nor leave in-flight I/O stalled for the
> 30s block layer timeout, similar to autonomous reset handling in
> hisi_sas, megaraid_sas, and smartpqi).

In SDB mode, clearing UTRLDBR will cause all pending commands to be
requeued because the OCS member is initialized to 
OCS_INVALID_COMMAND_STATUS and because ufshcd_transfer_rsp_status()
translates this status value into DID_REQUEUE << 16. I'm concerned
that this approach may cause the deadlines for SCSI commands to be
exceeded. Hence my proposal for MCQ mode not to requeue pending SCSI
commands but instead to let the SCSI error handler decide what to do
with these commands.

Thanks,

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.