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

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

On Tue, 2024-09-10 at 10:59 -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 12:30 AM, peter.wang@mediatek.com wrote:
> > ufshcd_abort_all froce abort all on-going command and the host
>                     ^^^^^ ^^^^^              ^^^^^^^         ^^^^
>                 forcibly? aborts?            commands?   host
> controller?
> 

Hi Bart,

Sorry, will correct words next version.


> > will automatically fill in the OCS field of the corresponding
> > response with OCS_ABORTED based on different working modes.
>                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> The host controller only sets the OCS field to OCS_ABORTED in MCQ
> mode
> if the host controller successfully aborted the command. If the
> abort TMF is submitted to the UFS device, the OCS field won't be
> changed
> into OCS_ABORTED. In SDB mode, the host controller does not modify
> the 
> OCS field either.
> 

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?


> > SDB mode: aborts a command using UTRLCLR. Task Management response
> > which means a Transfer Request was aborted.
> 
> Hmm ... my understanding is that clearing a bit from UTRLCLR is only
> allowed *after* a command has been aborted and also that clearing a
> bit
> from this register does not abort a command but only frees the
> resources
> in the host controller associated with the command.
> 

Although this specification description does not explicitly 
state the OCS behavior, to my understanding, the specification
for MCQ abort behavior is formulated with reference to the SDB mode.


> > For these two cases, set a flag to notify SCSI to requeue the
> > command after receiving response with OCS_ABORTED.
> 
> I think there is only one case when the SCSI core needs to be
> requested
> to requeue a command, namely when the UFS driver decided to initiate
> the
> abort (ufshcd_abort_all()).
> 
> > @@ -7561,6 +7551,20 @@ int ufshcd_try_to_abort_task(struct ufs_hba
> *hba, int tag)
> >   goto out;
> >   }
> >   
> > +/*
> > + * 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.


> Regarding the word "host" in the above comment block: the host is
> the 
> Android device. I think that in the above comment "host" should be
> changed into "host controller".
> 

It will be changed to 'host controller' to make the comment clearer.


> > diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> > index 0fd2aebac728..15b357672ca5 100644
> > --- a/include/ufs/ufshcd.h
> > +++ b/include/ufs/ufshcd.h
> > @@ -173,6 +173,8 @@ struct ufs_pm_lvl_states {
> >    * @crypto_key_slot: the key slot to use for inline crypto (-1 if
> none)
> >    * @data_unit_num: the data unit number for the first block for
> inline crypto
> >    * @req_abort_skip: skip request abort task flag
> > + * @abort_initiated_by_err: The flag is specifically used to
> handle aborts
> > + *                          caused by errors due to host/device
> communication
> 
> The "abort_initiated_by_err" name still seems confusing to me. Please
> make it more clear that this flag is only set if the UFS error
> handler
> decides to abort a command. How about "abort_initiated_by_eh"?
> 
> Please also make the description of this member variable more clear.
> 

Sure, will change this name and make description clearer.

Thanks.
Peter



> Thanks,
> 
> Bart.