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

[Bean Huo <beanhuo@iokpp.de>]

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