[PATCH 6.18.y] scsi: ufs: core: Re-arm the device command completion before submitting

[PATCH 6.18.y] scsi: ufs: core: Re-arm the device command completion before submitting

[alice.chao]

From: Alice Chao <alice.chao@mediatek.com>

Commit 20b97acc4caf ("scsi: ufs: core: Fix a race condition related to
device commands") moved the device management command completion into
struct ufs_hba, initialized once by ufshcd_init(), and dropped the

	hba->dev_cmd.complete = NULL;

assignments that used to make ufshcd_compl_one_cqe() discard completions
the submitter had already given up on. Nothing replaced them, so the
completion is never reset between two device commands:

  Task A (device command submitter) IRQ (tag == hba->reserved_slot)
  --------------------------------- ------------------------------
  ufshcd_read_desc_param()
   ufshcd_query_descriptor_retry()
    __ufshcd_query_descriptor()
     ufshcd_exec_dev_cmd()
      ufshcd_issue_dev_cmd()
       ufshcd_send_command()
       ufshcd_wait_for_dev_cmd()
        wait_for_completion_timeout()
        /* times out, done == 0 */
        ufshcd_clear_cmd()
        /* returns 0, no effect */
       return -EAGAIN
                                    ufs_mtk_mcq_intr()
                                     ufshcd_mcq_poll_cqe_lock()
                                      ufshcd_mcq_process_cqe()
                                       ufshcd_compl_one_cqe()
                                        /* lrbp->cmd == NULL */
                                        complete(&hba->dev_cmd.complete)
                                        /* done: 0 -> 1 */
  ufshcd_query_attr_retry()
   ufshcd_query_attr()
    ufshcd_exec_dev_cmd()
     ufshcd_issue_dev_cmd()
      ufshcd_send_command()
      ufshcd_wait_for_dev_cmd()
       wait_for_completion_timeout()
       /* returns at once, done: 1 -> 0 */
       ufshcd_dev_cmd_completion()
       /* response UPIU not written yet */
       return -EINVAL

Task A then rejects what it reads out of the response UPIU:

  ufshcd_dev_cmd_completion: Invalid device management cmd response: 0
  ufshcd_dev_cmd_completion: unexpected response in Query RSP: ff

The skew does not self-correct. On a UFS 4.0 controller in MCQ mode it
persisted across more than a thousand consecutive device commands,
failing every descriptor and attribute read until the link was reset.

The controller can still complete the timed-out command because in MCQ
mode ufshcd_clear_cmd() only issues an SQ cleanup (SQRTC.ICU), which
shows the command left the submission queue but not that a CQE is not
already posted. The MCQ path also skips the hba->outstanding_reqs
re-check that the SDB path does, so it returns -EAGAIN with the
completion still armed.

Re-arm the completion in ufshcd_issue_dev_cmd(), immediately before
submitting. All submitters - ufshcd_exec_dev_cmd(),
ufshcd_issue_devman_upiu_cmd() and ufshcd_advanced_rpmb_op() - reach it
holding hba->dev_cmd.lock, so no extra serialization is needed.

This narrows the window rather than closing it: the CQE only carries the
tag and every device command uses hba->reserved_slot, so a late
completion is still indistinguishable from the expected one. It no
longer spans the idle time between two commands.

No mainline commit: commit 08b12cda6c44 ("scsi: ufs: core: Switch to
scsi_get_internal_cmd()") moved this path onto the block layer and
removed struct ufs_dev_cmd::complete and ufshcd_wait_for_dev_cmd(). Each
device command now waits on its own request via blk_execute_rq(), so
mainline has no shared completion to skew. That refactor is not a
reasonable stable backport; this is the minimal alternative.

Affected versions: v6.15 through v6.18, i.e. the kernels that carry the
commit named in the Fixes: tag but not the mainline rewrite above.

Fixes: 20b97acc4caf ("scsi: ufs: core: Fix a race condition related to device commands")
Cc: stable@vger.kernel.org
Signed-off-by: Alice Chao <alice.chao@mediatek.com>
---
 drivers/ufs/core/ufshcd.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 87578e8824d2..003fa8af4f4d 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3312,6 +3312,15 @@ static int ufshcd_issue_dev_cmd(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
 {
 	int err;
 
+	/*
+	 * A device command that timed out may still be completed by the
+	 * controller later on. hba->dev_cmd.complete is shared by all device
+	 * commands, so re-arm it here, immediately before submitting, to keep
+	 * such a late completion from being mistaken for the completion of
+	 * this command.
+	 */
+	reinit_completion(&hba->dev_cmd.complete);
+
 	ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp->ucd_req_ptr);
 	ufshcd_send_command(hba, tag, hba->dev_cmd_queue);
 	err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
-- 
2.45.2

Re: [PATCH 6.18.y] scsi: ufs: core: Re-arm the device command completion before submitting

[Sasha Levin]

> Re-arm the completion in ufshcd_issue_dev_cmd(), immediately before
> submitting. All submitters - ufshcd_exec_dev_cmd(),
> ufshcd_issue_devman_upiu_cmd() and ufshcd_advanced_rpmb_op() - reach it
> holding hba->dev_cmd.lock, so no extra serialization is needed.

The analysis reads correct to me and the patch is clean against 6.18, but
this is a stable only change in the UFS core completion path with no
mainline counterpart, since 08b12cda6c44 ("scsi: ufs: core: Switch to
scsi_get_internal_cmd()") deleted this code outright. I would like a
subsystem ack before queueing it.

-- 
Thanks,
Sasha

Re: [PATCH 6.18.y] scsi: ufs: core: Re-arm the device command completion before submitting

[Alice Chao]

On Tue, 2026-09-15 at 21:52 -0400, Sasha Levin wrote:
> > Re-arm the completion in ufshcd_issue_dev_cmd(), immediately before
> > submitting. All submitters - ufshcd_exec_dev_cmd(),
> > ufshcd_issue_devman_upiu_cmd() and ufshcd_advanced_rpmb_op() -
> > reach it
> > holding hba->dev_cmd.lock, so no extra serialization is needed.
> 
> The analysis reads correct to me and the patch is clean against 6.18,
> but
> this is a stable only change in the UFS core completion path with no
> mainline counterpart, since 08b12cda6c44 ("scsi: ufs: core: Switch to
> scsi_get_internal_cmd()") deleted this code outright. I would like a
> subsystem ack before queueing it.
> 


Gentle Ping.

Hi Martin and Bart, any thoughts on this one?

Thanks.

Re: [PATCH 6.18.y] scsi: ufs: core: Re-arm the device command completion before submitting

[Bean Huo]

On Tue, 2026-09-15 at 13:26 +0800, alice.chao@mediatek.com wrote:
> From: Alice Chao <alice.chao@mediatek.com>
> 
> Commit 20b97acc4caf ("scsi: ufs: core: Fix a race condition related to
> device commands") moved the device management command completion into
> struct ufs_hba, initialized once by ufshcd_init(), and dropped the
> 
>         hba->dev_cmd.complete = NULL;
> 
> assignments that used to make ufshcd_compl_one_cqe() discard completions
> the submitter had already given up on. Nothing replaced them, so the
> completion is never reset between two device commands:
> 
>   Task A (device command submitter) IRQ (tag == hba->reserved_slot)
>   --------------------------------- ------------------------------
>   ufshcd_read_desc_param()
>    ufshcd_query_descriptor_retry()
>     __ufshcd_query_descriptor()
>      ufshcd_exec_dev_cmd()
>       ufshcd_issue_dev_cmd()
>        ufshcd_send_command()
>        ufshcd_wait_for_dev_cmd()
>         wait_for_completion_timeout()
>         /* times out, done == 0 */
>         ufshcd_clear_cmd()
>         /* returns 0, no effect */
>        return -EAGAIN
>                                     ufs_mtk_mcq_intr()
>                                      ufshcd_mcq_poll_cqe_lock()
>                                       ufshcd_mcq_process_cqe()
>                                        ufshcd_compl_one_cqe()
>                                         /* lrbp->cmd == NULL */
>                                         complete(&hba->dev_cmd.complete)
>                                         /* done: 0 -> 1 */
>   ufshcd_query_attr_retry()
>    ufshcd_query_attr()
>     ufshcd_exec_dev_cmd()
>      ufshcd_issue_dev_cmd()
>       ufshcd_send_command()
>       ufshcd_wait_for_dev_cmd()
>        wait_for_completion_timeout()
>        /* returns at once, done: 1 -> 0 */
>        ufshcd_dev_cmd_completion()
>        /* response UPIU not written yet */
>        return -EINVAL
> 
> Task A then rejects what it reads out of the response UPIU:
> 
>   ufshcd_dev_cmd_completion: Invalid device management cmd response: 0
>   ufshcd_dev_cmd_completion: unexpected response in Query RSP: ff
> 
> The skew does not self-correct. On a UFS 4.0 controller in MCQ mode it
> persisted across more than a thousand consecutive device commands,
> failing every descriptor and attribute read until the link was reset.
> 
> The controller can still complete the timed-out command because in MCQ
> mode ufshcd_clear_cmd() only issues an SQ cleanup (SQRTC.ICU), which
> shows the command left the submission queue but not that a CQE is not
> already posted. The MCQ path also skips the hba->outstanding_reqs
> re-check that the SDB path does, so it returns -EAGAIN with the
> completion still armed.
> 
> Re-arm the completion in ufshcd_issue_dev_cmd(), immediately before
> submitting. All submitters - ufshcd_exec_dev_cmd(),
> ufshcd_issue_devman_upiu_cmd() and ufshcd_advanced_rpmb_op() - reach it
> holding hba->dev_cmd.lock, so no extra serialization is needed.
> 
> This narrows the window rather than closing it: the CQE only carries the
> tag and every device command uses hba->reserved_slot, so a late
> completion is still indistinguishable from the expected one. It no
> longer spans the idle time between two commands.
> 
> No mainline commit: commit 08b12cda6c44 ("scsi: ufs: core: Switch to
> scsi_get_internal_cmd()") moved this path onto the block layer and
> removed struct ufs_dev_cmd::complete and ufshcd_wait_for_dev_cmd(). Each
> device command now waits on its own request via blk_execute_rq(), so
> mainline has no shared completion to skew. That refactor is not a
> reasonable stable backport; this is the minimal alternative.
> 
> Affected versions: v6.15 through v6.18, i.e. the kernels that carry the
> commit named in the Fixes: tag but not the mainline rewrite above.
> 
> Fixes: 20b97acc4caf ("scsi: ufs: core: Fix a race condition related to device
> commands")
> Cc: stable@vger.kernel.org
> Signed-off-by: Alice Chao <alice.chao@mediatek.com>
> ---
>  drivers/ufs/core/ufshcd.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 87578e8824d2..003fa8af4f4d 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -3312,6 +3312,15 @@ static int ufshcd_issue_dev_cmd(struct ufs_hba *hba,
> struct ufshcd_lrb *lrbp,
>  {
>         int err;
>  
> +       /*
> +        * A device command that timed out may still be completed by the
> +        * controller later on. hba->dev_cmd.complete is shared by all device
> +        * commands, so re-arm it here, immediately before submitting, to keep
> +        * such a late completion from being mistaken for the completion of
> +        * this command.
> +        */
> +       reinit_completion(&hba->dev_cmd.complete);
> +
>         ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp->ucd_req_ptr);
>         ufshcd_send_command(hba, tag, hba->dev_cmd_queue);
>         err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
> --


I have one question:

The late CQE can come after the re-arm:

timeout -> cleanup -> retry B -> re-arm -> send B -> A's CQE arrives, and then B
is woken up early and fails, if B's own CQE in turn comes after C's re-arm, C
fails the same way, and C may even read B's response as its own. 

Whether this stops depends on device latency against our retry path, not on the
patch. 

The root cause is that all device commands share hba->reserved_slot and the CQE
carries only the tag. Since a successful SQ cleanup must post an ABORTED CQE,
could the MCQ timeout path wait for and consume that CQE before returning -
EAGAIN? 

does this patch only covers a late CQE arriving while no device command is in
flight. Did you test it with injected timeouts to see whether it recovers?


Kind regards,
Bean

Re: [PATCH 6.18.y] scsi: ufs: core: Re-arm the device command completion before submitting

[Alice Chao]

On Mon, 2026-10-05 at 13:54 +0200, Bean Huo wrote:
> The late CQE can come after the re-arm:
> 
> timeout -> cleanup -> retry B -> re-arm -> send B -> A's CQE arrives,
> and then B
> is woken up early and fails, if B's own CQE in turn comes after C's
> re-arm, C
> fails the same way, and C may even read B's response as its own.
> 
> Whether this stops depends on device latency against our retry path,
> not on the
> patch.
> 

You are right. Re-arming only covers a late CQE that arrives while no
device command is in flight. The query retry wrappers resubmit right
away, so the next re-arm will usually happen before the previous CQE
lands, and the skew can cascade exactly as you describe. It is also
worse than an early wakeup: every device command shares the reserved
slot's response UPIU, so C can parse B's response as its own and
return a wrong value rather than an error.

> The root cause is that all device commands share hba->reserved_slot
> and the CQE
> carries only the tag. Since a successful SQ cleanup must post an
> ABORTED CQE,
> could the MCQ timeout path wait for and consume that CQE before
> returning -
> EAGAIN?
> 

Agreed, that closes the window instead of narrowing it. In v2 the MCQ
timeout path will:

 - after the SQ cleanup, wait (bounded) for the reserved slot's CQE -
   the ABORTED one, or the regular one if the command completed
   anyway - before returning;
 - if no CQE shows up in time (e.g. cleanup failed, or
   UFSHCD_QUIRK_MCQ_BROKEN_RTC), force a host reset and refuse device
   commands outside the error handler until it has happened, so the
   slot is not reused while that CQE can still arrive;
 - keep reinit_completion() at submission time as a safety net.

> does this patch only covers a late CQE arriving while no device
> command is in
> flight. Did you test it with injected timeouts to see whether it
> recovers?
> 

Yes, v1 only covers that case. And no, I have not tested it with
injected timeouts yet.

Before posting v2 I will run it with injected device command timeouts
in MCQ mode: periodic fake timeouts under a descriptor/attribute read
loop, with the values checked against known-good ones, and the case
where the CQE never arrives, to check that the host gets reset and
device commands recover. This will also show whether our controller
actually posts the ABORTED CQE after the SQ cleanup. I will run the
same injection on v1 and on the unpatched kernel for comparison and
put the results in the v2 changelog.

Thanks for the careful review.

Alice