Re: [PATCH 3/3] ufs: core: Do not forcibly complete SCSI commands

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Polling the MCQ Completion Queue by reading hardware registers while the host controller is disabled (HCE=0) causes a hard CPU lockup due to an infinite loop.
--

--- Patch [3]: [PATCH 3/3] ufs: core: Do not forcibly complete SCSI commands ---
commit af070348e9f00f73ade393f3d9421e41e0afb4fb
Author: Bart Van Assche <bvanassche@acm.org>

ufs: core: Do not forcibly complete SCSI commands

This patch removes the forced completion of SCSI commands from the UFS error
handler for MCQ mode, delegating abort and requeue decisions to the SCSI core.
It makes MCQ behavior consistent with legacy mode by removing
ufshcd_mcq_compl_all_cqes_lock and the force_compl flag.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index cdfcd3d4c4c97..26bc2b0c08124 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
[ ... ]
> @@ -8197,7 +8163,7 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
>  	 */
>  	ufshcd_hba_stop(hba);
>  	hba->silence_err_logs = true;
> -	ufshcd_complete_requests(hba, true);
> +	ufshcd_complete_requests(hba);

[Severity: Critical]
Can this lead to an infinite loop and hard lockup during error recovery?

After ufshcd_hba_stop() disables the host controller,
ufshcd_complete_requests() now eventually calls ufshcd_mcq_poll_cqe_lock():

ufshcd_host_reset_and_restore()
  ufshcd_hba_stop()
  ufshcd_complete_requests()
    ufshcd_mcq_compl_pending_transfer()
      blk_mq_tagset_busy_iter(..., ufshcd_mcq_compl_one, ...)
        ufshcd_mcq_compl_one()
          ufshcd_mcq_poll_cqe_lock()

In ufshcd_mcq_poll_cqe_lock():

    ufshcd_mcq_update_cq_tail_slot(hwq);
    while (!ufshcd_mcq_is_cq_empty(hwq)) {
        ...
        ufshcd_mcq_inc_cq_head_slot(hwq);
    }

If reading the disabled MMIO register in ufshcd_mcq_update_cq_tail_slot()
returns 0xFFFFFFFF, the tail slot becomes out-of-bounds. Since the head slot
wraps around at hwq->max_entries, ufshcd_mcq_is_cq_empty() will never evaluate
to true.

Because this loop runs with the hwq->cq_lock spinlock held and interrupts
disabled, will this permanently hang the CPU?

>  	hba->silence_err_logs = false;
>  
>  	/* scale up clocks to max frequency before full reinitialization */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786558726.git.bvanassche@acm.org?part=3