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