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? hostcontroller?
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/devicecommunication
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.