Re: [PATCH v6 2/2] ufs: mcq: poll OCS after sq cleanup for device management commands

[Bart Van Assche <bvanassche@acm.org>]

On 11/4/26 4:43 PM, Kiwoong Kim wrote:
 > Here is the timeline of an actual case for this that happened with 
kernel 6.18.
 > [ ... ]
 > 2. So after the query was timed out, the driver did SQ cleanup.
 > Then the host updated CQ update and raised IS.CQES,
 > Unfortunately, complete() was called the driver's ISR afterward.

This timeline describes code that no longer exists upstream. Since
commit 08b12cda6c44 ("scsi: ufs: core: Switch to
scsi_get_internal_cmd()"), the device management path no longer calls
complete(). Please describe the race in terms of the current code:
the query times out, the SCSI error handler calls ufshcd_abort() ->
ufshcd_mcq_abort() -> ufshcd_mcq_sq_cleanup(), the request is finished,
and the retry reuses the same reserved tag, struct scsi_cmnd and devman
UCD. A late CQE for the first query then maps to hba->dev_cmd.tag via
devman_ucd_dma_addr and completes the retry too early. That is why
cq_lock matters here. Please also keep lines in the patch description
at 75 columns or less.

 > +static enum utp_ocs ufshcd_mcq_read_ocs(struct ufs_hw_queue *hwq,
 > +					struct ufshcd_lrb *lrbp)
 > +{
 > +	guard(spinlock_irqsave)(&hwq->cq_lock);
 > +	return READ_ONCE(lrbp->utr_descriptor_ptr->header.ocs);
 > +}

READ_ONCE() is not necessary since the OCS is read with cq_lock held.

 > +	if (!err && blk_mq_is_reserved_rq(rq)) {
 > +		enum utp_ocs ocs;
 > +
 > +		err = read_poll_timeout(ufshcd_mcq_read_ocs, ocs,
 > +					ocs != OCS_INVALID_COMMAND_STATUS, 20,
 > +					MCQ_POLL_US, false, hwq, lrbp);
 > +		if (err)
 > +			dev_err(hba->dev, "%s: OCS not updated. hwq=%d, tag=%d err=%d\n",
 > +				__func__, id, task_tag, err);
 > +	}
 > +
 >  	if (ufshcd_mcq_sq_start(hba, hwq))
 >  		err = -ETIMEDOUT;

The SQ is still stopped at this point, so this loop can stop fetching
from the hardware queue for up to MCQ_POLL_US (500 ms). Please move the
OCS poll after the ufshcd_mcq_sq_start() call.

This code also waits for the interrupt handler to process the CQE. Why
not reap the CQE from this function? ufshcd_clear_lu_cmds() already
does that by calling ufshcd_mcq_poll_cqe_lock(hba, hwq) after
ufshcd_clear_cmd(). Calling ufshcd_mcq_poll_cqe_lock() before reading
the OCS would remove the dependency on interrupt delivery and match
the existing code.

Additionally, does the UFSHCI specification guarantee that the host
controller posts a CQE (e.g. with OCS = ABORTED) for every command
that has been cleaned up? If not, this patch turns cases that are
harmless today into a host reset. Please cite the relevant section of
the UFSHCI specification in the patch description or in a source code
comment.

Thanks,

Bart.