diff mbox series

[v1,1/3] scsi: ufs: Let UPIU completion trace print RSP UPIU

Message ID 20210523211409.210304-2-huobean@gmail.com
State New
Headers show
Series [v1,1/3] scsi: ufs: Let UPIU completion trace print RSP UPIU | expand

Commit Message

Bean Huo May 23, 2021, 9:14 p.m. UTC 
From: Bean Huo <beanhuo@micron.com>

The current UPIU completion event trace still prints the COMMAND UPIU
header, rather than the RSP UPIU header. This makes UPIU command trace
useless in problem shooting in case we receive a trace log from the
customer/field.

There are two important fields in RSP UPIU:
 1. The response field, which indicates the UFS defined overall success
    or failure of the series of Command, Data and RESPONSE UPIU’s that
    make up the execution of a task.
 2. The Status field, which contains the command set specific status for
    a specific command issued by the initiator device.

Before this patch, the UPIU paired trace events:

ufshcd_upiu: send_req: fe3b0000.ufs: HDR:01 20 00 1c 00 00 00 00 00 00 00 00, CDB:3b e1 00 00 00 00 00 00 30 00 00 00 00 00 00 00
ufshcd_upiu: complete_rsp: fe3b0000.ufs: HDR:01 20 00 1c 00 00 00 00 00 00 00 00, CDB:00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

After the patch:

ufshcd_upiu: send_req: fe3b0000.ufs: HDR:01 20 00 1c 00 00 00 00 00 00 00 00, CDB:3b e1 00 00 00 00 00 00 30 00 00 00 00 00 00 00
ufshcd_upiu: complete_rsp: fe3b0000.ufs: HDR:21 00 00 1c 00 00 00 00 00 00 00 00, CDB:00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshcd.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Bart Van Assche May 24, 2021, 1:24 a.m. UTC | #1 
On 5/23/21 2:14 PM, Bean Huo wrote:
> +		rq_rsp = (struct utp_upiu_req *)hba->lrb[tag].ucd_rsp_ptr;


So a pointer to a response (hba->lrb[tag].ucd_rsp_ptr) is cast to a
pointer to a request (struct utp_upiu_req *)? That seems really odd to
me. Please explain.

Bart.
Bean Huo May 25, 2021, 7:28 p.m. UTC | #2 
On Sun, 2021-05-23 at 18:24 -0700, Bart Van Assche wrote:
> On 5/23/21 2:14 PM, Bean Huo wrote:

> 

> > +             rq_rsp = (struct utp_upiu_req *)hba-

> > >lrb[tag].ucd_rsp_ptr;

> 

> 

> So a pointer to a response (hba->lrb[tag].ucd_rsp_ptr) is cast to a

> 

> pointer to a request (struct utp_upiu_req *)? That seems really odd

> to

> 

> me. Please explain.


Bart,

these two structures have the same size, and inside the structures,
the both unions have the same members(not exactly 100% identical). 

struct utp_upiu_rsp {
        struct utp_upiu_header header;
        union {
                struct utp_cmd_rsp sr;
                struct utp_upiu_query qr;
        };
};


struct utp_upiu_req {
        struct utp_upiu_header header;
        union {
                struct utp_upiu_cmd             sc;
                struct utp_upiu_query           qr;
                struct utp_upiu_query           uc;
        };
};

Use one point for response and request both, no problem here. It is
true that looks very ood, and very difficult to read them.

If this is problem, I can change the code, let them more readable.

how do you think?

Bean
Bart Van Assche May 25, 2021, 8:15 p.m. UTC | #3 
On 5/25/21 12:28 PM, Bean Huo wrote:
> If this is problem, I can change the code, let them more readable.

> 

> how do you think?


A long explanation was needed to show that the patch is correct. I think
this shows that the code is confusing :-) Hence please use the struct
utp_upiu_rsp type when interpreting a pointer as a response.

Thanks,

Bart.
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c382260e0cf7..dc5f13ccf176 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -300,13 +300,18 @@  static void ufshcd_scsi_block_requests(struct ufs_hba *hba)
 static void ufshcd_add_cmd_upiu_trace(struct ufs_hba *hba, unsigned int tag,
 				      enum ufs_trace_str_t str_t)
 {
-	struct utp_upiu_req *rq = hba->lrb[tag].ucd_req_ptr;
+	struct utp_upiu_req *rq_rsp;
 
 	if (!trace_ufshcd_upiu_enabled())
 		return;
 
-	trace_ufshcd_upiu(dev_name(hba->dev), str_t, &rq->header, &rq->sc.cdb,
-			  UFS_TSF_CDB);
+	if (str_t == UFS_CMD_SEND)
+		rq_rsp = hba->lrb[tag].ucd_req_ptr;
+	else
+		rq_rsp = (struct utp_upiu_req *)hba->lrb[tag].ucd_rsp_ptr;
+
+	trace_ufshcd_upiu(dev_name(hba->dev), str_t, &rq_rsp->header,
+			  &rq_rsp->sc.cdb, UFS_TSF_CDB);
 }
 
 static void ufshcd_add_query_upiu_trace(struct ufs_hba *hba,