[PATCH v1 0/2] update CQ entry and dump CQE in MCQ mode

[PATCH v1 0/2] update CQ entry and dump CQE in MCQ mode

[peter.wang]

From: Peter Wang <peter.wang@mediatek.com>

Update the CQ entry format for UFS 4.1 compatibility and
add support for logging CQ entries in MCQ mode, enhancing
debugging capabilities.

Peter Wang (2):
  ufs: core: update CQ Entry to UFS 4.1 format
  ufs: core: support dumping CQ entry in MCQ Mode

 drivers/ufs/core/ufshcd.c | 17 ++++++++++-------
 include/ufs/ufshci.h      | 17 +++++++++++++----
 2 files changed, 23 insertions(+), 11 deletions(-)

-- 
2.45.2

[PATCH v1 1/2] ufs: core: update CQ Entry to UFS 4.1 format

[peter.wang]

From: Peter Wang <peter.wang@mediatek.com>

Update the completion queue (CQ) entry format with UFS 4.1
specifications. UFS 4.1 introduces new members in reserved
record DW5. This patch also refines the DW4 with detailed
members defined in UFS 4.0. Modify the code to
incorporate these changes by updating the overall_status
in the CQ entry structure.

Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/core/ufshcd.c |  4 ++--
 include/ufs/ufshci.h      | 17 +++++++++++++----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index ff7a3d60b11d..d779cc777a17 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -858,7 +858,7 @@ static enum utp_ocs ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp,
 				      struct cq_entry *cqe)
 {
 	if (cqe)
-		return le32_to_cpu(cqe->status) & MASK_OCS;
+		return cqe->overall_status & MASK_OCS;
 
 	return lrbp->utr_descriptor_ptr->header.ocs & MASK_OCS;
 }
@@ -5643,7 +5643,7 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
 		scsi_done(cmd);
 	} else {
 		if (cqe) {
-			ocs = le32_to_cpu(cqe->status) & MASK_OCS;
+			ocs = cqe->overall_status & MASK_OCS;
 			lrbp->utr_descriptor_ptr->header.ocs = ocs;
 		}
 		complete(&hba->dev_cmd.complete);
diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
index 612500a7088f..8b14f6e5e6f5 100644
--- a/include/ufs/ufshci.h
+++ b/include/ufs/ufshci.h
@@ -567,10 +567,19 @@ struct cq_entry {
 	__le16  prd_table_offset;
 
 	/* DW 4 */
-	__le32 status;
-
-	/* DW 5-7 */
-	__le32 reserved[3];
+	u8 overall_status;
+	u8 extended_error_code;
+	__le16 reserved_1;
+
+	/* DW 5 */
+	u8 task_tag;
+	u8 lun;
+	u8 iid:4;
+	u8 ext_iid:4;
+	u8 reserved_2;
+
+	/* DW 6-7 */
+	__le32 reserved_3[2];
 };
 
 static_assert(sizeof(struct cq_entry) == 32);
-- 
2.45.2

[PATCH v1 2/2] ufs: core: support dumping CQ entry in MCQ Mode

[peter.wang]

From: Peter Wang <peter.wang@mediatek.com>

Enhance the ufshcd_print_tr function to support dumping
completion queue (CQ) entries in MCQ mode when an error occurs.
This addition provides more detailed debugging information
by including the CQ entry data in the error logs, aiding
in the diagnosis of issues in MCQ mode.

Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/core/ufshcd.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index d779cc777a17..b90500126b35 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -599,7 +599,8 @@ static void ufshcd_print_evt_hist(struct ufs_hba *hba)
 }
 
 static
-void ufshcd_print_tr(struct ufs_hba *hba, int tag, bool pr_prdt)
+void ufshcd_print_tr(struct ufs_hba *hba, struct cq_entry *cqe,
+		     int tag, bool pr_prdt)
 {
 	const struct ufshcd_lrb *lrbp;
 	int prdt_length;
@@ -618,6 +619,8 @@ void ufshcd_print_tr(struct ufs_hba *hba, int tag, bool pr_prdt)
 
 	ufshcd_hex_dump("UPIU TRD: ", lrbp->utr_descriptor_ptr,
 			sizeof(struct utp_transfer_req_desc));
+	if (cqe)
+		ufshcd_hex_dump("UPIU CQE: ", cqe, sizeof(struct cq_entry));
 	dev_err(hba->dev, "UPIU[%d] - Request UPIU phys@0x%llx\n", tag,
 		(u64)lrbp->ucd_req_dma_addr);
 	ufshcd_hex_dump("UPIU REQ: ", lrbp->ucd_req_ptr,
@@ -648,7 +651,7 @@ static bool ufshcd_print_tr_iter(struct request *req, void *priv)
 	struct Scsi_Host *shost = sdev->host;
 	struct ufs_hba *hba = shost_priv(shost);
 
-	ufshcd_print_tr(hba, req->tag, *(bool *)priv);
+	ufshcd_print_tr(hba, NULL, req->tag, *(bool *)priv);
 
 	return true;
 }
@@ -5536,7 +5539,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
 
 	if ((host_byte(result) != DID_OK) &&
 	    (host_byte(result) != DID_REQUEUE) && !hba->silence_err_logs)
-		ufshcd_print_tr(hba, lrbp->task_tag, true);
+		ufshcd_print_tr(hba, cqe, lrbp->task_tag, true);
 	return result;
 }
 
@@ -7763,9 +7766,9 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 		ufshcd_print_evt_hist(hba);
 		ufshcd_print_host_state(hba);
 		ufshcd_print_pwr_info(hba);
-		ufshcd_print_tr(hba, tag, true);
+		ufshcd_print_tr(hba, NULL, tag, true);
 	} else {
-		ufshcd_print_tr(hba, tag, false);
+		ufshcd_print_tr(hba, NULL, tag, false);
 	}
 	hba->req_abort_count++;
 
-- 
2.45.2

Re: [PATCH v1 1/2] ufs: core: update CQ Entry to UFS 4.1 format

[Bart Van Assche]

On 10/14/25 6:15 AM, peter.wang@mediatek.com wrote:
> diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
> index 612500a7088f..8b14f6e5e6f5 100644
> --- a/include/ufs/ufshci.h
> +++ b/include/ufs/ufshci.h
> @@ -567,10 +567,19 @@ struct cq_entry {
>   	__le16  prd_table_offset;
>   
>   	/* DW 4 */
> -	__le32 status;
> -
> -	/* DW 5-7 */
> -	__le32 reserved[3];
> +	u8 overall_status;
> +	u8 extended_error_code;
> +	__le16 reserved_1;
> +
> +	/* DW 5 */
> +	u8 task_tag;
> +	u8 lun;
> +	u8 iid:4;
> +	u8 ext_iid:4;
> +	u8 reserved_2;
> +
> +	/* DW 6-7 */
> +	__le32 reserved_3[2];
>   };

The above definition is only correct for little endian CPUs. If you 
really want to use bitfields, please take a look at struct
request_desc_header for making bitfield definitions work for both little
and big endian CPUs.

Bart.

Re: [PATCH v1 2/2] ufs: core: support dumping CQ entry in MCQ Mode

[Bart Van Assche]

On 10/14/25 6:15 AM, peter.wang@mediatek.com wrote:
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index d779cc777a17..b90500126b35 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -599,7 +599,8 @@ static void ufshcd_print_evt_hist(struct ufs_hba *hba)
>   }
>   
>   static
> -void ufshcd_print_tr(struct ufs_hba *hba, int tag, bool pr_prdt)
> +void ufshcd_print_tr(struct ufs_hba *hba, struct cq_entry *cqe,
> +		     int tag, bool pr_prdt)
>   {
>   	const struct ufshcd_lrb *lrbp;
>   	int prdt_length;
> @@ -618,6 +619,8 @@ void ufshcd_print_tr(struct ufs_hba *hba, int tag, bool pr_prdt)
>   
>   	ufshcd_hex_dump("UPIU TRD: ", lrbp->utr_descriptor_ptr,
>   			sizeof(struct utp_transfer_req_desc));
> +	if (cqe)
> +		ufshcd_hex_dump("UPIU CQE: ", cqe, sizeof(struct cq_entry));
>   	dev_err(hba->dev, "UPIU[%d] - Request UPIU phys@0x%llx\n", tag,
>   		(u64)lrbp->ucd_req_dma_addr);
>   	ufshcd_hex_dump("UPIU REQ: ", lrbp->ucd_req_ptr,
> @@ -648,7 +651,7 @@ static bool ufshcd_print_tr_iter(struct request *req, void *priv)
>   	struct Scsi_Host *shost = sdev->host;
>   	struct ufs_hba *hba = shost_priv(shost);
>   
> -	ufshcd_print_tr(hba, req->tag, *(bool *)priv);
> +	ufshcd_print_tr(hba, NULL, req->tag, *(bool *)priv);
>   
>   	return true;
>   }
> @@ -5536,7 +5539,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
>   
>   	if ((host_byte(result) != DID_OK) &&
>   	    (host_byte(result) != DID_REQUEUE) && !hba->silence_err_logs)
> -		ufshcd_print_tr(hba, lrbp->task_tag, true);
> +		ufshcd_print_tr(hba, cqe, lrbp->task_tag, true);
>   	return result;
>   }
>   
> @@ -7763,9 +7766,9 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>   		ufshcd_print_evt_hist(hba);
>   		ufshcd_print_host_state(hba);
>   		ufshcd_print_pwr_info(hba);
> -		ufshcd_print_tr(hba, tag, true);
> +		ufshcd_print_tr(hba, NULL, tag, true);
>   	} else {
> -		ufshcd_print_tr(hba, tag, false);
> +		ufshcd_print_tr(hba, NULL, tag, false);
>   	}
>   	hba->req_abort_count++;

So there are four callers of ufshcd_print_tr() and only one caller dumps
the CQE? Wouldn't it be better not to add any arguments to
ufshcd_print_tr() and instead add the code for dumping the CQE directly
in the only function that needs this functionality?

Thanks,

Bart.

Re: [PATCH v1 1/2] ufs: core: update CQ Entry to UFS 4.1 format

[Peter Wang (王信友)]

On Tue, 2025-10-14 at 08:44 -0700, Bart Van Assche wrote:
> The above definition is only correct for little endian CPUs. If you
> really want to use bitfields, please take a look at struct
> request_desc_header for making bitfield definitions work for both
> little
> and big endian CPUs.
> 
> Bart.

Hi Bart,

Okay, I will add big-endian support in the next version.

Thanks
Peter

Re: [PATCH v1 2/2] ufs: core: support dumping CQ entry in MCQ Mode

[Peter Wang (王信友)]

On Tue, 2025-10-14 at 08:46 -0700, Bart Van Assche wrote:
> So there are four callers of ufshcd_print_tr() and only one caller
> dumps
> the CQE? Wouldn't it be better not to add any arguments to
> ufshcd_print_tr() and instead add the code for dumping the CQE
> directly
> in the only function that needs this functionality?
> 
> Thanks,
> 
> Bart.
> 
> 

Hi Bart,

The CQE pointer is generated by the hardware after receiving
the UFS device response. So, if we don't add the CQE argument,
I'm not sure how we could obtain this pointer in ufshcd_print_tr.
Or are you suggesting that we dump the CQE directly in 
ufshcd_transfer_rsp_status?

Thanks
Peter

Re: [PATCH v1 2/2] ufs: core: support dumping CQ entry in MCQ Mode

[Bart Van Assche]

On 10/14/25 11:14 PM, Peter Wang (王信友) wrote:
> Or are you suggesting that we dump the CQE directly in
> ufshcd_transfer_rsp_status()?

Yes, that's what I'm suggesting.

Thanks,

Bart.

Re: [PATCH v1 2/2] ufs: core: support dumping CQ entry in MCQ Mode

[Peter Wang (王信友)]

On Wed, 2025-10-15 at 09:00 -0700, Bart Van Assche wrote:
> On 10/14/25 11:14 PM, Peter Wang (王信友) wrote:
> > Or are you suggesting that we dump the CQE directly in
> > ufshcd_transfer_rsp_status()?
> 
> Yes, that's what I'm suggesting.
> 
> Thanks,
> 
> Bart.

Hi Bart,

I'm not sure if putting the dump code outside the dump
function is a reasonable approach.
But if there is this concern, I’ll make a version 
with it moved outside first.

Thanks
Peter