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

[Kiwoong Kim]

>  > 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:

Let me add it below the timeline described previously.

> 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.

Got it.

> 
>  > +	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.

I didn't get what you meant. The loop just reads updated OCS periodically
and from what I know, it's based on the assumption that when a task is cleaned up by setting SQRTCy.ICU to one
and it's successfully done, the host updates OCS, even if SQ is stopped.
If you feel I get your comment wrong, please let me know.

> 
> 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.

Because a successful SQ cleanup leads to OCS update,
which subsequently leads to raising IS.CQES when MCQ is enabled.
I understand your idea with ufshcd_clear_lu_cmds, then do you intend
Ignoring IS.CQES event in this case?

> 
> 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.

This is the sentence that I referred to:
"The host controller shall free up any resources associated to the nexus,
and shall post a Completion Queue entry with OCS = ABORTED if clean up successful."


> Thanks,
> 
> Bart.

Thanks
Kiwoong Kim
BRs.