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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ufs: data race on completion.done via reinit_completion()
--

commit bd0e388ed86b6eb0eec312ed2150d925d6dc5c8a
Author: Alice Chao <alice.chao@mediatek.com>
scsi: ufs: core: Re-arm the device command completion before submitting

This patch re-arms the hba->dev_cmd.complete completion immediately before
submitting a device command. This aims to prevent using a stale completion
from a previously timed-out command, particularly in MCQ mode.

> --- 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);

[Severity: Medium]
Could this introduce a data race with concurrent interrupt-driven completions?

Looking at ufshcd_compl_one_cqe(), if the completion queue entry for a
previously timed-out command arrives right as we submit a new one, the IRQ
handler will call complete() on this shared completion.

The complete() function takes x->wait.lock and performs a read-modify-write
on x->done. However, reinit_completion() performs an unlocked plain write
setting x->done to 0. 

If these run concurrently:

CPU1 (CQE handler)
ufshcd_compl_one_cqe()
  complete(&hba->dev_cmd.complete)
    spin_lock_irqsave(&x->wait.lock, flags);
    x->done++;
    spin_unlock_irqrestore(&x->wait.lock, flags);

CPU2 (Submitter)
ufshcd_issue_dev_cmd()
  reinit_completion(&hba->dev_cmd.complete)
    x->done = 0;

Since ufshcd_issue_dev_cmd() only holds hba->dev_cmd.lock to serialize
submitters but doesn't synchronize with the asynchronous CQE handler, this
concurrent access to x->done violates the memory model and could lead to
lost or spurious completions.

Would it be safer to drain the completion instead, perhaps by looping
with try_wait_for_completion() rather than performing a plain write?

> +
>  	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);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915052638.459390-1-alice.chao@mediatek.com?part=1