Re: [PATCH v8 2/3] ufs: core: fix error handler process for MCQ abort

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

On 9/23/24 1:03 AM, peter.wang@mediatek.com wrote:
> When the error handler successfully aborts a MCQ request,
> it only releases the command and does not notify the SCSI layer.
> This may cause another abort after 30 seconds timeout.
> This patch notifies the SCSI layer to requeue the request.
> 
> Additionally, ignore the OCS: ABORTED CQ slot after MCQ mode
> SQ cleanup. This makes the behavior of MCQ mode consistent with
> that of legacy SDB mode.
> 
> Also, print logs for OCS: ABORTED and OCS_INVALID_COMMAND_STATUS
> for debugging purposes.

Although I like the approach of this patch, two comments below.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index a6f818cdef0e..b5c7bc50a27e 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -5405,9 +5405,15 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
>   		break;
>   	case OCS_ABORTED:
>   		result |= DID_ABORT << 16;
> +		dev_warn(hba->dev,
> +				"OCS aborted from controller = %x for tag %d\n",
> +				ocs, lrbp->task_tag);
>   		break;

Including the OCS status in this message seems redundant to me.

>   	case OCS_INVALID_COMMAND_STATUS:
>   		result |= DID_REQUEUE << 16;
> +		dev_warn(hba->dev,
> +				"OCS invaild from controller = %x for tag %d\n",
> +				ocs, lrbp->task_tag);

Also here, including the OCS status in this message seems redundant to me.

Please change "invaild" into "invalid".

> @@ -5526,6 +5532,18 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
>   			ufshcd_update_monitor(hba, lrbp);
>   		ufshcd_add_command_trace(hba, task_tag, UFS_CMD_COMP);
>   		cmd->result = ufshcd_transfer_rsp_status(hba, lrbp, cqe);
> +
> +		/*
> +		 * Ignore MCQ OCS: ABORTED posted by the host controller.
> +		 * This makes the behavior of MCQ mode consistent with that
> +		 * of legacy SDB mode.
> +		 */
> +		if (hba->mcq_enabled) {
> +			ocs = ufshcd_get_tr_ocs(lrbp, cqe);
> +			if (ocs == OCS_ABORTED)
> +				return;
> +		}

Why only ignore the OCS_ABORTED status in MCQ mode? Is my understanding
correct that MediaTek controllers can also report this status in legacy 
mode?

Thanks,

Bart.