Re: [PATCH v4 2/2] ufs: core: requeue aborted request

["Peter Wang (王信友)" <peter.wang@mediatek.com>]

On Wed, 2024-09-11 at 12:11 -0700, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 9/10/24 11:03 PM, Peter Wang (王信友) wrote:
> > This statement is not quite accurate becasue in UFSHIC2.1, SDB mode
> > specification already have OCS: ABORTED (0x6) define.
> > And it is used in below UTRLCLR description:
> > 'which means a Transfer Request was "aborted"'
> > Therefore, the host controller should follow the
> > specification and fill the OCS field with OCS: ABORTED.
> > If not so, at what point does your host controller use the
> > OCS: ABORTED status?
> 
> Hmm ... I have not been able to find any explanation in the UFSHCI
> 2.1
> specification that says when the OCS status is set to aborted. Did I
> perhaps overlook something?
> 
> This is what I found in the UTRLCLR description: "The host software 
> shall use this field only when a UTP Transfer Request is expected to
> not be completed, e.g., when the host software receives a “FUNCTION
> COMPLETE” Task Management response which means a Transfer Request was
> aborted." This does not mean that the host controller is expected to
> set the OCS status to "ABORTED". I will send an email to the JC-64
> mailing list to request clarification.
> 

Hi Bart,


Yes, you're right, just as I mentioned earlier, I also think
the spec does not explicitly state that UTRLC should have 
corresponding behavior for OCS. This might lead to inconsistencies
in how host controllers operate. 


> >>> +/*
> >>> + * When the host software receives a "FUNCTION COMPLETE", set
> flag
> >>> + * to requeue command after receive response with OCS_ABORTED
> >>> + * SDB mode: UTRLCLR Task Management response which means a
> >> Transfer
> >>> + *           Request was aborted.
> >>> + * MCQ mode: Host will post to CQ with OCS_ABORTED after SQ
> >> cleanup
> >>> + * This flag is set because ufshcd_abort_all forcibly aborts all
> >>> + * commands, and the host will automatically fill in the OCS
> field
> >>> + * of the corresponding response with OCS_ABORTED.
> >>> + * Therefore, upon receiving this response, it needs to be
> >> requeued.
> >>> + */
> >>> +if (!err)
> >>> +lrbp->abort_initiated_by_err = true;
> >>> +
> >>>    err = ufshcd_clear_cmd(hba, tag);
> >>>    if (err)
> >>>    dev_err(hba->dev, "%s: Failed clearing cmd at tag %d, err
> %d\n",
> >>
> >> The above change is misplaced. ufshcd_try_to_abort_task() can be
> >> called
> >> when the SCSI core decides to abort a command while
> >> abort_initiated_by_err must not be set in that case. Please move
> the
> >> above code block into ufshcd_abort_one().
> > 
> > But move to ufshcd_abort_one may have race condition, beacause we
> > need set this flag before ufshcd_clear_cmd host controller fill
> > OCS_ABORTED to response. I will add check ufshcd_eh_in_progress.
> 
> Calling ufshcd_clear_cmd() does not affect the OCS status as far as I
> know. Did I perhaps overlook something?
> 

Because after ufshcd_clear_cmd,

in MCQ mode: 
ufshcd_mcq_sq_cleanup, host controller will post CQ response with OCS
ABORTED.

in SDB mode: 
ufshcd_utrl_clear set UTRLC, Mediatek host controller 
(may not all host controller) will post response with OCS ABORTED.

In both cases, we have an interrupt sent to the host, and there 
may be a race condition before we set this flag for requeue.
So I need to set this flag before ufshcd_clear_cmd.


Thanks.
Peter


> Thanks,
> 
> Bart.