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

[Bart Van Assche <bvanassche@acm.org>]

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

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

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

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

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

Thanks,

Bart.