[PATCH v1 0/3] Three fixes for the UPIU trace

[PATCH v1 0/3] Three fixes for the UPIU trace

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>



Bean Huo (3):
  scsi: ufs: Distinguish between query REQ and query RSP in query trace
  scsi: ufs: Distinguish between TM request UPIU and response UPIU in TM
    UPIU trace
  scsi: ufs: Make UPIU trace easier differentiate among CDB, OSF, and TM

 drivers/scsi/ufs/ufshcd.c  | 21 ++++++++++++++++-----
 include/trace/events/ufs.h | 10 +++++++---
 2 files changed, 23 insertions(+), 8 deletions(-)

-- 
2.17.1

[PATCH v1 1/3] scsi: ufs: Distinguish between query REQ and query RSP in query trace

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Currently, in the query completion trace print,  since we use
hba->lrb[tag].ucd_req_ptr and didn't differentiate UPIU between
request and response, thus header and transaction-specific field
in UPIU printed by query trace are identical. This is not very
practical. As below:

query_send: HDR:16 00 00 0e 00 81 00 00 00 00 00 00, CDB:06 0e 03 00 00 00 00 00 00 00 00 00 00 00 00 00
query_complete: HDR:16 00 00 0e 00 81 00 00 00 00 00 00, CDB:06 0e 03 00 00 00 00 00 00 00 00 00 00 00 00 00

For the failure analysis, we want to understand the real response
reported by the UFS device, however, the current query trace tells
us nothing. After this patch, the query trace on the query_send, and
the above a pair of query_send and query_complete will be:

query_send: HDR:16 00 00 0e 00 81 00 00 00 00 00 00, CDB:06 0e 03 00 00 00 00 00 00 00 00 00 00 00 00 00
ufshcd_upiu: HDR:36 00 00 0e 00 81 00 00 00 00 00 00, CDB:06 0e 03 00 00 00 00 00 00 00 00 01 00 00 00 00

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index ceb562accc39..e10de94adb3f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -321,9 +321,15 @@ static void ufshcd_add_cmd_upiu_trace(struct ufs_hba *hba, unsigned int tag,
 static void ufshcd_add_query_upiu_trace(struct ufs_hba *hba, unsigned int tag,
 		const char *str)
 {
-	struct utp_upiu_req *rq = hba->lrb[tag].ucd_req_ptr;
+	struct utp_upiu_req *rq_rsp;
+
+	if (!strcmp("query_send", str))
+		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, &rq->header, &rq->qr);
+	trace_ufshcd_upiu(dev_name(hba->dev), str, &rq_rsp->header,
+			  &rq_rsp->qr);
 }
 
 static void ufshcd_add_tm_upiu_trace(struct ufs_hba *hba, unsigned int tag,
-- 
2.17.1

[PATCH v1 2/3] scsi: ufs: Distinguish between TM request UPIU and response UPIU in TM UPIU trace

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Distinguish between TM request UPIU and response UPIU in TM UPIU trace,
for the TM response, let TM UPIU trace print its TM response UPIU.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e10de94adb3f..29d7240a61bf 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -338,8 +338,12 @@ static void ufshcd_add_tm_upiu_trace(struct ufs_hba *hba, unsigned int tag,
 	int off = (int)tag - hba->nutrs;
 	struct utp_task_req_desc *descp = &hba->utmrdl_base_addr[off];
 
-	trace_ufshcd_upiu(dev_name(hba->dev), str, &descp->req_header,
-			&descp->input_param1);
+	if (!strcmp("tm_send", str))
+		trace_ufshcd_upiu(dev_name(hba->dev), str, &descp->req_header,
+				  &descp->input_param1);
+	else
+		trace_ufshcd_upiu(dev_name(hba->dev), str, &descp->rsp_header,
+				  &descp->output_param1);
 }
 
 static void ufshcd_add_uic_command_trace(struct ufs_hba *hba,
-- 
2.17.1

[PATCH v1 3/3] scsi: ufs: Make UPIU trace easier differentiate among CDB, OSF, and TM

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Transaction Specific Fields (TSF) in the UPIU package could be CDB
(SCSI/UFS Command Descriptor Block), OSF (Opcode Specific Field), and
TM I/O parameter (Task Management Input/Output Parameter). But, currently,
we take all of these as CDB  in the UPIU trace. Thus makes user confuse
among CDB, OSF, and TM message. So fix it with this patch.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshcd.c  |  9 +++++----
 include/trace/events/ufs.h | 10 +++++++---
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 29d7240a61bf..5b2219e44743 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -315,7 +315,8 @@ static void ufshcd_add_cmd_upiu_trace(struct ufs_hba *hba, unsigned int tag,
 {
 	struct utp_upiu_req *rq = hba->lrb[tag].ucd_req_ptr;
 
-	trace_ufshcd_upiu(dev_name(hba->dev), str, &rq->header, &rq->sc.cdb);
+	trace_ufshcd_upiu(dev_name(hba->dev), str, &rq->header, &rq->sc.cdb,
+			  "CDB");
 }
 
 static void ufshcd_add_query_upiu_trace(struct ufs_hba *hba, unsigned int tag,
@@ -329,7 +330,7 @@ static void ufshcd_add_query_upiu_trace(struct ufs_hba *hba, unsigned int tag,
 		rq_rsp = (struct utp_upiu_req *)hba->lrb[tag].ucd_rsp_ptr;
 
 	trace_ufshcd_upiu(dev_name(hba->dev), str, &rq_rsp->header,
-			  &rq_rsp->qr);
+			  &rq_rsp->qr, "OSF");
 }
 
 static void ufshcd_add_tm_upiu_trace(struct ufs_hba *hba, unsigned int tag,
@@ -340,10 +341,10 @@ static void ufshcd_add_tm_upiu_trace(struct ufs_hba *hba, unsigned int tag,
 
 	if (!strcmp("tm_send", str))
 		trace_ufshcd_upiu(dev_name(hba->dev), str, &descp->req_header,
-				  &descp->input_param1);
+				  &descp->input_param1, "TM_INPUT");
 	else
 		trace_ufshcd_upiu(dev_name(hba->dev), str, &descp->rsp_header,
-				  &descp->output_param1);
+				  &descp->output_param1, "TM_OUTPUT");
 }
 
 static void ufshcd_add_uic_command_trace(struct ufs_hba *hba,
diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
index 0bd54a184391..68e8e97a9b47 100644
--- a/include/trace/events/ufs.h
+++ b/include/trace/events/ufs.h
@@ -295,15 +295,17 @@ TRACE_EVENT(ufshcd_uic_command,
 );
 
 TRACE_EVENT(ufshcd_upiu,
-	TP_PROTO(const char *dev_name, const char *str, void *hdr, void *tsf),
+	TP_PROTO(const char *dev_name, const char *str, void *hdr, void *tsf,
+		 const char *tsf_type),
 
-	TP_ARGS(dev_name, str, hdr, tsf),
+	TP_ARGS(dev_name, str, hdr, tsf, tsf_type),
 
 	TP_STRUCT__entry(
 		__string(dev_name, dev_name)
 		__string(str, str)
 		__array(unsigned char, hdr, 12)
 		__array(unsigned char, tsf, 16)
+		__string(tsf_type, tsf_type)
 	),
 
 	TP_fast_assign(
@@ -311,12 +313,14 @@ TRACE_EVENT(ufshcd_upiu,
 		__assign_str(str, str);
 		memcpy(__entry->hdr, hdr, sizeof(__entry->hdr));
 		memcpy(__entry->tsf, tsf, sizeof(__entry->tsf));
+		__assign_str(tsf_type, tsf_type);
 	),
 
 	TP_printk(
-		"%s: %s: HDR:%s, CDB:%s",
+		"%s: %s: HDR:%s, %s:%s",
 		__get_str(str), __get_str(dev_name),
 		__print_hex(__entry->hdr, sizeof(__entry->hdr)),
+		__get_str(tsf_type),
 		__print_hex(__entry->tsf, sizeof(__entry->tsf))
 	)
 );
-- 
2.17.1

Re: [PATCH v1 1/3] scsi: ufs: Distinguish between query REQ and query RSP in query trace

[Bart Van Assche]

On 12/6/20 8:42 AM, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> Currently, in the query completion trace print,  since we use
> hba->lrb[tag].ucd_req_ptr and didn't differentiate UPIU between
> request and response, thus header and transaction-specific field
> in UPIU printed by query trace are identical. This is not very
> practical. As below:
> 
> query_send: HDR:16 00 00 0e 00 81 00 00 00 00 00 00, CDB:06 0e 03 00 00 00 00 00 00 00 00 00 00 00 00 00
> query_complete: HDR:16 00 00 0e 00 81 00 00 00 00 00 00, CDB:06 0e 03 00 00 00 00 00 00 00 00 00 00 00 00 00
> 
> For the failure analysis, we want to understand the real response
> reported by the UFS device, however, the current query trace tells
> us nothing. After this patch, the query trace on the query_send, and
> the above a pair of query_send and query_complete will be:
> 
> query_send: HDR:16 00 00 0e 00 81 00 00 00 00 00 00, CDB:06 0e 03 00 00 00 00 00 00 00 00 00 00 00 00 00
> ufshcd_upiu: HDR:36 00 00 0e 00 81 00 00 00 00 00 00, CDB:06 0e 03 00 00 00 00 00 00 00 00 01 00 00 00 00
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index ceb562accc39..e10de94adb3f 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -321,9 +321,15 @@ static void ufshcd_add_cmd_upiu_trace(struct ufs_hba *hba, unsigned int tag,
>  static void ufshcd_add_query_upiu_trace(struct ufs_hba *hba, unsigned int tag,
>  		const char *str)
>  {
> -	struct utp_upiu_req *rq = hba->lrb[tag].ucd_req_ptr;
> +	struct utp_upiu_req *rq_rsp;
> +
> +	if (!strcmp("query_send", str))
> +		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, &rq->header, &rq->qr);
> +	trace_ufshcd_upiu(dev_name(hba->dev), str, &rq_rsp->header,
> +			  &rq_rsp->qr);
>  }

Please change the 'str' argument into an enum and let
ufshcd_add_query_upiu_trace() do the enum-to-string conversion. That
will allow to convert the strcmp() call into an integer comparison.

Thanks,

Bart.

Re: [PATCH v1 2/3] scsi: ufs: Distinguish between TM request UPIU and response UPIU in TM UPIU trace

[Bart Van Assche]

On 12/6/20 8:42 AM, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> Distinguish between TM request UPIU and response UPIU in TM UPIU trace,
> for the TM response, let TM UPIU trace print its TM response UPIU.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index e10de94adb3f..29d7240a61bf 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -338,8 +338,12 @@ static void ufshcd_add_tm_upiu_trace(struct ufs_hba *hba, unsigned int tag,
>  	int off = (int)tag - hba->nutrs;
>  	struct utp_task_req_desc *descp = &hba->utmrdl_base_addr[off];
>  
> -	trace_ufshcd_upiu(dev_name(hba->dev), str, &descp->req_header,
> -			&descp->input_param1);
> +	if (!strcmp("tm_send", str))
> +		trace_ufshcd_upiu(dev_name(hba->dev), str, &descp->req_header,
> +				  &descp->input_param1);
> +	else
> +		trace_ufshcd_upiu(dev_name(hba->dev), str, &descp->rsp_header,
> +				  &descp->output_param1);
>  }

Same comment here: please change the type of the 'str' argument in an
enum such that the strcmp() call can be changed into an integer comparison.

Thanks,

Bart.

RE: [PATCH v1 1/3] scsi: ufs: Distinguish between query REQ and query RSP in query trace

[Avri Altman]

> 
> From: Bean Huo <beanhuo@micron.com>
> 
> Currently, in the query completion trace print,  since we use
> hba->lrb[tag].ucd_req_ptr and didn't differentiate UPIU between
> request and response, thus header and transaction-specific field
> in UPIU printed by query trace are identical. This is not very
> practical. As below:
> 
> query_send: HDR:16 00 00 0e 00 81 00 00 00 00 00 00, CDB:06 0e 03 00 00 00 00
> 00 00 00 00 00 00 00 00 00
> query_complete: HDR:16 00 00 0e 00 81 00 00 00 00 00 00, CDB:06 0e 03 00 00
> 00 00 00 00 00 00 00 00 00 00 00
> 
> For the failure analysis, we want to understand the real response
> reported by the UFS device, however, the current query trace tells
> us nothing. After this patch, the query trace on the query_send, and
> the above a pair of query_send and query_complete will be:
> 
> query_send: HDR:16 00 00 0e 00 81 00 00 00 00 00 00, CDB:06 0e 03 00 00 00 00
> 00 00 00 00 00 00 00 00 00
> ufshcd_upiu: HDR:36 00 00 0e 00 81 00 00 00 00 00 00, CDB:06 0e 03 00 00 00 00
> 00 00 00 00 01 00 00 00 00
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
Acked-by: Avri Altman <avri.altman@wdc.com>

But you need to change the complete string so not to break the current parsers.
I would also pass to ufshcd_add_query_upiu_trace the struct utp_upiu_req *, 
so no comparison is needed.

Thanks,
Avri

> ---
>  drivers/scsi/ufs/ufshcd.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index ceb562accc39..e10de94adb3f 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -321,9 +321,15 @@ static void ufshcd_add_cmd_upiu_trace(struct
> ufs_hba *hba, unsigned int tag,
>  static void ufshcd_add_query_upiu_trace(struct ufs_hba *hba, unsigned int
> tag,
>                 const char *str)
>  {
> -       struct utp_upiu_req *rq = hba->lrb[tag].ucd_req_ptr;
> +       struct utp_upiu_req *rq_rsp;
> +
> +       if (!strcmp("query_send", str))
> +               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, &rq->header, &rq->qr);
> +       trace_ufshcd_upiu(dev_name(hba->dev), str, &rq_rsp->header,
> +                         &rq_rsp->qr);
>  }
> 
>  static void ufshcd_add_tm_upiu_trace(struct ufs_hba *hba, unsigned int
> tag,
> --
> 2.17.1

RE: [PATCH v1 2/3] scsi: ufs: Distinguish between TM request UPIU and response UPIU in TM UPIU trace

[Avri Altman]

> 
> 
> From: Bean Huo <beanhuo@micron.com>
> 
> Distinguish between TM request UPIU and response UPIU in TM UPIU trace,
> for the TM response, let TM UPIU trace print its TM response UPIU.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
Acked-by: Avri Altman <avri.altman@wdc.com>

Again - same comment:
But you need to change the complete string so not to break the current parsers.
I would also pass to the  struct utp_upiu_header *,
so no comparison is needed.

Thanks,
Avri

> ---
>  drivers/scsi/ufs/ufshcd.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index e10de94adb3f..29d7240a61bf 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -338,8 +338,12 @@ static void ufshcd_add_tm_upiu_trace(struct
> ufs_hba *hba, unsigned int tag,
>         int off = (int)tag - hba->nutrs;
>         struct utp_task_req_desc *descp = &hba->utmrdl_base_addr[off];
> 
> -       trace_ufshcd_upiu(dev_name(hba->dev), str, &descp->req_header,
> -                       &descp->input_param1);
> +       if (!strcmp("tm_send", str))
> +               trace_ufshcd_upiu(dev_name(hba->dev), str, &descp-
> >req_header,
> +                                 &descp->input_param1);
> +       else
> +               trace_ufshcd_upiu(dev_name(hba->dev), str, &descp-
> >rsp_header,
> +                                 &descp->output_param1);
>  }
> 
>  static void ufshcd_add_uic_command_trace(struct ufs_hba *hba,
> --
> 2.17.1

RE: [PATCH v1 3/3] scsi: ufs: Make UPIU trace easier differentiate among CDB, OSF, and TM

[Avri Altman]

> 
> 
> From: Bean Huo <beanhuo@micron.com>
> 
> Transaction Specific Fields (TSF) in the UPIU package could be CDB
> (SCSI/UFS Command Descriptor Block), OSF (Opcode Specific Field), and
> TM I/O parameter (Task Management Input/Output Parameter). But,
> currently,
> we take all of these as CDB  in the UPIU trace. Thus makes user confuse
> among CDB, OSF, and TM message. So fix it with this patch.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufshcd.c  |  9 +++++----
>  include/trace/events/ufs.h | 10 +++++++---
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 29d7240a61bf..5b2219e44743 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -315,7 +315,8 @@ static void ufshcd_add_cmd_upiu_trace(struct
> ufs_hba *hba, unsigned int tag,
>  {
>         struct utp_upiu_req *rq = hba->lrb[tag].ucd_req_ptr;
> 
> -       trace_ufshcd_upiu(dev_name(hba->dev), str, &rq->header, &rq-
> >sc.cdb);
> +       trace_ufshcd_upiu(dev_name(hba->dev), str, &rq->header, &rq-
> >sc.cdb,
> +                         "CDB");
>  }
> 
>  static void ufshcd_add_query_upiu_trace(struct ufs_hba *hba, unsigned int
> tag,
> @@ -329,7 +330,7 @@ static void ufshcd_add_query_upiu_trace(struct
> ufs_hba *hba, unsigned int tag,
>                 rq_rsp = (struct utp_upiu_req *)hba->lrb[tag].ucd_rsp_ptr;
> 
>         trace_ufshcd_upiu(dev_name(hba->dev), str, &rq_rsp->header,
> -                         &rq_rsp->qr);
> +                         &rq_rsp->qr, "OSF");
>  }
> 
>  static void ufshcd_add_tm_upiu_trace(struct ufs_hba *hba, unsigned int
> tag,
> @@ -340,10 +341,10 @@ static void ufshcd_add_tm_upiu_trace(struct
> ufs_hba *hba, unsigned int tag,
> 
>         if (!strcmp("tm_send", str))
>                 trace_ufshcd_upiu(dev_name(hba->dev), str, &descp->req_header,
> -                                 &descp->input_param1);
> +                                 &descp->input_param1, "TM_INPUT");
>         else
>                 trace_ufshcd_upiu(dev_name(hba->dev), str, &descp->rsp_header,
> -                                 &descp->output_param1);
> +                                 &descp->output_param1, "TM_OUTPUT");
>  }
> 
>  static void ufshcd_add_uic_command_trace(struct ufs_hba *hba,
> diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
> index 0bd54a184391..68e8e97a9b47 100644
> --- a/include/trace/events/ufs.h
> +++ b/include/trace/events/ufs.h
> @@ -295,15 +295,17 @@ TRACE_EVENT(ufshcd_uic_command,
>  );
> 
>  TRACE_EVENT(ufshcd_upiu,
> -       TP_PROTO(const char *dev_name, const char *str, void *hdr, void *tsf),
> +       TP_PROTO(const char *dev_name, const char *str, void *hdr, void *tsf,
> +                const char *tsf_type),
> 
> -       TP_ARGS(dev_name, str, hdr, tsf),
> +       TP_ARGS(dev_name, str, hdr, tsf, tsf_type),
> 
>         TP_STRUCT__entry(
>                 __string(dev_name, dev_name)
>                 __string(str, str)
>                 __array(unsigned char, hdr, 12)
>                 __array(unsigned char, tsf, 16)
> +               __string(tsf_type, tsf_type)
>         ),
> 
>         TP_fast_assign(
> @@ -311,12 +313,14 @@ TRACE_EVENT(ufshcd_upiu,
>                 __assign_str(str, str);
>                 memcpy(__entry->hdr, hdr, sizeof(__entry->hdr));
>                 memcpy(__entry->tsf, tsf, sizeof(__entry->tsf));
> +               __assign_str(tsf_type, tsf_type);
>         ),
> 
>         TP_printk(
> -               "%s: %s: HDR:%s, CDB:%s",
> +               "%s: %s: HDR:%s, %s:%s",
>                 __get_str(str), __get_str(dev_name),
>                 __print_hex(__entry->hdr, sizeof(__entry->hdr)),
> +               __get_str(tsf_type),
This breaks what current parsers expects.
Why str is not enough to distinguish between the command?

Thanks,
Avri

>                 __print_hex(__entry->tsf, sizeof(__entry->tsf))
>         )
>  );
> --
> 2.17.1

Re: [PATCH v1 3/3] scsi: ufs: Make UPIU trace easier differentiate among CDB, OSF, and TM

[Bean Huo]

On Mon, 2020-12-07 at 07:57 +0000, Avri Altman wrote:
> >          TP_printk(
> > -               "%s: %s: HDR:%s, CDB:%s",
> > +               "%s: %s: HDR:%s, %s:%s",
> >                  __get_str(str), __get_str(dev_name),
> >                  __print_hex(__entry->hdr, sizeof(__entry->hdr)),
> > +               __get_str(tsf_type),
> 
> This breaks what current parsers expects.
> Why str is not enough to distinguish between the command?
> 
> Thanks,
> Avri

Hi Avri
Tt donesn't break original CDB parser. for the CDB, it is still the
same as before. Here just make Transaction Specific Fields in the UPIU
package much clearer.

I mentioned in the commits message: 

Transaction Specific Fields (TSF) in the UPIU package could be CDB
(SCSI/UFS Command Descriptor Block), OSF (Opcode Specific Field), and
TM I/O parameter (Task Management Input/Output Parameter). But we
didn't differenciate them. we take all of these as CDB. This is wrong.

I want to make it clearer and make UPIU trace in line with the Spec.
what's more,  how do you filter OSF, TM parameters with current UPIU
trace? you take all of them as CDB? if so, I think, it's better to
change parser.

Thanks,
Bean

Re: [PATCH v1 1/3] scsi: ufs: Distinguish between query REQ and query RSP in query trace

[Steven Rostedt]

On Sun,  6 Dec 2020 17:42:24 +0100
Bean Huo <huobean@gmail.com> wrote:

> From: Bean Huo <beanhuo@micron.com>
> 
> Currently, in the query completion trace print,  since we use
> hba->lrb[tag].ucd_req_ptr and didn't differentiate UPIU between
> request and response, thus header and transaction-specific field
> in UPIU printed by query trace are identical. This is not very
> practical. As below:
> 
> query_send: HDR:16 00 00 0e 00 81 00 00 00 00 00 00, CDB:06 0e 03 00 00 00 00 00 00 00 00 00 00 00 00 00
> query_complete: HDR:16 00 00 0e 00 81 00 00 00 00 00 00, CDB:06 0e 03 00 00 00 00 00 00 00 00 00 00 00 00 00
> 
> For the failure analysis, we want to understand the real response
> reported by the UFS device, however, the current query trace tells
> us nothing. After this patch, the query trace on the query_send, and
> the above a pair of query_send and query_complete will be:
> 
> query_send: HDR:16 00 00 0e 00 81 00 00 00 00 00 00, CDB:06 0e 03 00 00 00 00 00 00 00 00 00 00 00 00 00
> ufshcd_upiu: HDR:36 00 00 0e 00 81 00 00 00 00 00 00, CDB:06 0e 03 00 00 00 00 00 00 00 00 01 00 00 00 00
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index ceb562accc39..e10de94adb3f 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -321,9 +321,15 @@ static void ufshcd_add_cmd_upiu_trace(struct ufs_hba *hba, unsigned int tag,
>  static void ufshcd_add_query_upiu_trace(struct ufs_hba *hba, unsigned int tag,
>  		const char *str)
>  {
> -	struct utp_upiu_req *rq = hba->lrb[tag].ucd_req_ptr;
> +	struct utp_upiu_req *rq_rsp;
> +

I would add:

	if (!trace_ufshcd_upiu_enabled())
		return;

Why do the work if the trace point is not enabled?

-- Steve


> +	if (!strcmp("query_send", str))
> +		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, &rq->header, &rq->qr);
> +	trace_ufshcd_upiu(dev_name(hba->dev), str, &rq_rsp->header,
> +			  &rq_rsp->qr);
>  }
>  
>  static void ufshcd_add_tm_upiu_trace(struct ufs_hba *hba, unsigned int tag,

Re: [PATCH v1 3/3] scsi: ufs: Make UPIU trace easier differentiate among CDB, OSF, and TM

[Steven Rostedt]

On Sun,  6 Dec 2020 17:42:26 +0100
Bean Huo <huobean@gmail.com> wrote:

> From: Bean Huo <beanhuo@micron.com>
> 
> Transaction Specific Fields (TSF) in the UPIU package could be CDB
> (SCSI/UFS Command Descriptor Block), OSF (Opcode Specific Field), and
> TM I/O parameter (Task Management Input/Output Parameter). But, currently,
> we take all of these as CDB  in the UPIU trace. Thus makes user confuse
> among CDB, OSF, and TM message. So fix it with this patch.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufshcd.c  |  9 +++++----
>  include/trace/events/ufs.h | 10 +++++++---
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 29d7240a61bf..5b2219e44743 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -315,7 +315,8 @@ static void ufshcd_add_cmd_upiu_trace(struct ufs_hba *hba, unsigned int tag,
>  {
>  	struct utp_upiu_req *rq = hba->lrb[tag].ucd_req_ptr;
>  
> -	trace_ufshcd_upiu(dev_name(hba->dev), str, &rq->header, &rq->sc.cdb);
> +	trace_ufshcd_upiu(dev_name(hba->dev), str, &rq->header, &rq->sc.cdb,
> +			  "CDB");
>  }
>  
>  static void ufshcd_add_query_upiu_trace(struct ufs_hba *hba, unsigned int tag,
> @@ -329,7 +330,7 @@ static void ufshcd_add_query_upiu_trace(struct ufs_hba *hba, unsigned int tag,
>  		rq_rsp = (struct utp_upiu_req *)hba->lrb[tag].ucd_rsp_ptr;
>  
>  	trace_ufshcd_upiu(dev_name(hba->dev), str, &rq_rsp->header,
> -			  &rq_rsp->qr);
> +			  &rq_rsp->qr, "OSF");
>  }
>  
>  static void ufshcd_add_tm_upiu_trace(struct ufs_hba *hba, unsigned int tag,
> @@ -340,10 +341,10 @@ static void ufshcd_add_tm_upiu_trace(struct ufs_hba *hba, unsigned int tag,
>  
>  	if (!strcmp("tm_send", str))
>  		trace_ufshcd_upiu(dev_name(hba->dev), str, &descp->req_header,
> -				  &descp->input_param1);
> +				  &descp->input_param1, "TM_INPUT");
>  	else
>  		trace_ufshcd_upiu(dev_name(hba->dev), str, &descp->rsp_header,
> -				  &descp->output_param1);
> +				  &descp->output_param1, "TM_OUTPUT");

You could save some space on the ring buffer, if you made the above into an
enum, and then used print_symbolic().

>  }
>  
>  static void ufshcd_add_uic_command_trace(struct ufs_hba *hba,
> diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
> index 0bd54a184391..68e8e97a9b47 100644
> --- a/include/trace/events/ufs.h
> +++ b/include/trace/events/ufs.h
> @@ -295,15 +295,17 @@ TRACE_EVENT(ufshcd_uic_command,
>  );


You could make this:

#define TRACE_TSF_TYPES		\
	EM(CDB)			\
	EM(OSF)			\
	EM(TM_INPUT)		\
	EMe(TM_OUTPUT)

#ifndef TRACE_TSF_TYPES_ENUMS
#define TRACE_TSF_TYPES_ENUMS
#undef EM
#undef EMe

#define EM(x)	TRACE_TSF_##x,
#define EMe(x)	TRACE_TSF_##x

enum {
	TRACE_TSF_TYPES
}
#endif /* TRACE_TSF_TYPES_ENUMS */

#undef EM
#undef EMe

/* These export the enum names to user space */
#define EM(x)	TRACE_DEFINE_ENUM(TRACE_TSF_##x)
#define EMe(x)	TRACE_DEFINE_ENUM(TRACE_TSF_##x)

TRACE_TSF_TYPES

#undef EM
#undef EMe

/* These are used in the print_symbolic */
#define EM(x) { TRACE_TSF_##x, #x },
#define EMe(x) { TRACE_TSF_##x, #x }


>  
>  TRACE_EVENT(ufshcd_upiu,
> -	TP_PROTO(const char *dev_name, const char *str, void *hdr, void *tsf),
> +	TP_PROTO(const char *dev_name, const char *str, void *hdr, void *tsf,
> +		 const char *tsf_type),

		int tsf_type;

>  
> -	TP_ARGS(dev_name, str, hdr, tsf),
> +	TP_ARGS(dev_name, str, hdr, tsf, tsf_type),
>  
>  	TP_STRUCT__entry(
>  		__string(dev_name, dev_name)
>  		__string(str, str)
>  		__array(unsigned char, hdr, 12)
>  		__array(unsigned char, tsf, 16)
> +		__string(tsf_type, tsf_type)

		__field(int, tsf_type)

>  	),
>  
>  	TP_fast_assign(
> @@ -311,12 +313,14 @@ TRACE_EVENT(ufshcd_upiu,
>  		__assign_str(str, str);
>  		memcpy(__entry->hdr, hdr, sizeof(__entry->hdr));
>  		memcpy(__entry->tsf, tsf, sizeof(__entry->tsf));
> +		__assign_str(tsf_type, tsf_type);


		__entry->tsf_type = tsf_type;


>  	),
>  
>  	TP_printk(
> -		"%s: %s: HDR:%s, CDB:%s",
> +		"%s: %s: HDR:%s, %s:%s",
>  		__get_str(str), __get_str(dev_name),
>  		__print_hex(__entry->hdr, sizeof(__entry->hdr)),
> +		__get_str(tsf_type),

		print_symbolic(tsf_type, TRACE_TSF_TYPES),

-- Steve


>  		__print_hex(__entry->tsf, sizeof(__entry->tsf))
>  	)
>  );

Re: [PATCH v1 3/3] scsi: ufs: Make UPIU trace easier differentiate among CDB, OSF, and TM

[Steven Rostedt]

On Mon, 7 Dec 2020 07:57:27 +0000
Avri Altman <Avri.Altman@wdc.com> wrote:

> > 
> >         TP_printk(
> > -               "%s: %s: HDR:%s, CDB:%s",
> > +               "%s: %s: HDR:%s, %s:%s",
> >                 __get_str(str), __get_str(dev_name),
> >                 __print_hex(__entry->hdr, sizeof(__entry->hdr)),
> > +               __get_str(tsf_type),  
> This breaks what current parsers expects.
> Why str is not enough to distinguish between the command?

Hopefully it shouldn't. Reading from user space should use the
libtraceevent library, that reads the format files and extracts the raw
data to find the fields. As long as the field exists, it should not break
user space parsers. If it does, please let me know, and I'll gladly help
change the user space code to use libtraceevent :-)

-- Steve

RE: [PATCH v1 3/3] scsi: ufs: Make UPIU trace easier differentiate among CDB, OSF, and TM

[Avri Altman]

> 
> On Mon, 7 Dec 2020 07:57:27 +0000
> Avri Altman <Avri.Altman@wdc.com> wrote:
> 
> > >
> > >         TP_printk(
> > > -               "%s: %s: HDR:%s, CDB:%s",
> > > +               "%s: %s: HDR:%s, %s:%s",
> > >                 __get_str(str), __get_str(dev_name),
> > >                 __print_hex(__entry->hdr, sizeof(__entry->hdr)),
> > > +               __get_str(tsf_type),
> > This breaks what current parsers expects.
> > Why str is not enough to distinguish between the command?
> 
> Hopefully it shouldn't. Reading from user space should use the
> libtraceevent library, that reads the format files and extracts the raw
> data to find the fields. As long as the field exists, it should not break
> user space parsers. If it does, please let me know, and I'll gladly help
> change the user space code to use libtraceevent :-)
Hi Steve,
Thanks. I wasn't aware of libtraceevent - is this a new thing?

We have a relatively sophisticated analysis platform that utilizes raw traces,
Among which the upiu trace is the most important and informative.

This tool has evolved over the years, adding more and more parsers per need,
and the users are picking the appropriate parser per the trace they used.

We will surely be glad to adopt new tracing capabilities,
But we would prefer not to break anything.

Thanks,
Avri

Re: [PATCH v1 3/3] scsi: ufs: Make UPIU trace easier differentiate among CDB, OSF, and TM

[Steven Rostedt]

On Mon, 7 Dec 2020 16:40:51 +0000
Avri Altman <Avri.Altman@wdc.com> wrote:

> > 
> > On Mon, 7 Dec 2020 07:57:27 +0000
> > Avri Altman <Avri.Altman@wdc.com> wrote:
> >   
> > > >
> > > >         TP_printk(
> > > > -               "%s: %s: HDR:%s, CDB:%s",
> > > > +               "%s: %s: HDR:%s, %s:%s",
> > > >                 __get_str(str), __get_str(dev_name),
> > > >                 __print_hex(__entry->hdr, sizeof(__entry->hdr)),
> > > > +               __get_str(tsf_type),  
> > > This breaks what current parsers expects.
> > > Why str is not enough to distinguish between the command?  
> > 
> > Hopefully it shouldn't. Reading from user space should use the
> > libtraceevent library, that reads the format files and extracts the raw
> > data to find the fields. As long as the field exists, it should not break
> > user space parsers. If it does, please let me know, and I'll gladly help
> > change the user space code to use libtraceevent :-)  
> Hi Steve,
> Thanks. I wasn't aware of libtraceevent - is this a new thing?

Actually, it's been around almost as long as ftrace. But unfortunately,
it's just now becoming a separate library. It was originally developed for
trace-cmd, but has been copied into perf, power-top and rasdaemon. But this
copying is inefficient and a maintenance nightmare, and we finally have the
library as a stand alone, and hopefully will be delivered by distributions
(I believe they are packaging it).

   https://git.kernel.org/pub/scm/libs/libtrace/libtraceevent.git/

Looks like distros are starting to catch on.

  https://packages.debian.org/unstable/libtraceevent-dev


We are currently working on libtracefs

  https://git.kernel.org/pub/scm/libs/libtrace/libtracefs.git/

Which will make it a lot easier for applications to interact with the
tracefs file system. I'm hoping to have this ready for distros by the end
of the year. We have applications coming that depend on these.

> 
> We have a relatively sophisticated analysis platform that utilizes raw traces,
> Among which the upiu trace is the most important and informative.
> 
> This tool has evolved over the years, adding more and more parsers per need,
> and the users are picking the appropriate parser per the trace they used.
> 
> We will surely be glad to adopt new tracing capabilities,

I think libtraceevent and libtracefs would be a much welcome addition for
upiu trace as it would be reading raw data (very fast), and have an API
that makes doing so much simpler. For example, I just wrote a quick program
that checks what files an application opens (this is not in anyway
production ready):

  http://rostedt.org/code/show-open-files.c

> But we would prefer not to break anything.

Of course!

And again, I would be happy to help out in converting to this libraries. It
will make your applications more robust, as they make it so that you do not
need to rely on the order of fields.

Note, there's plans on making these libraries python modules as well (to
have python scripts enable and read ftrace data).

-- Steve

RE: [PATCH v1 3/3] scsi: ufs: Make UPIU trace easier differentiate among CDB, OSF, and TM

[Avri Altman]

> 
> On Mon, 7 Dec 2020 16:40:51 +0000
> Avri Altman <Avri.Altman@wdc.com> wrote:
> 
> > >
> > > On Mon, 7 Dec 2020 07:57:27 +0000
> > > Avri Altman <Avri.Altman@wdc.com> wrote:
> > >
> > > > >
> > > > >         TP_printk(
> > > > > -               "%s: %s: HDR:%s, CDB:%s",
> > > > > +               "%s: %s: HDR:%s, %s:%s",
> > > > >                 __get_str(str), __get_str(dev_name),
> > > > >                 __print_hex(__entry->hdr, sizeof(__entry->hdr)),
> > > > > +               __get_str(tsf_type),
> > > > This breaks what current parsers expects.
> > > > Why str is not enough to distinguish between the command?
> > >
> > > Hopefully it shouldn't. Reading from user space should use the
> > > libtraceevent library, that reads the format files and extracts the raw
> > > data to find the fields. As long as the field exists, it should not break
> > > user space parsers. If it does, please let me know, and I'll gladly help
> > > change the user space code to use libtraceevent :-)
> > Hi Steve,
> > Thanks. I wasn't aware of libtraceevent - is this a new thing?
> 
> Actually, it's been around almost as long as ftrace. But unfortunately,
> it's just now becoming a separate library. It was originally developed for
> trace-cmd, but has been copied into perf, power-top and rasdaemon. But
> this
> copying is inefficient and a maintenance nightmare, and we finally have the
> library as a stand alone, and hopefully will be delivered by distributions
> (I believe they are packaging it).
> 
>    https://git.kernel.org/pub/scm/libs/libtrace/libtraceevent.git/
> 
> Looks like distros are starting to catch on.
> 
>   https://packages.debian.org/unstable/libtraceevent-dev
> 
> 
> We are currently working on libtracefs
> 
>   https://git.kernel.org/pub/scm/libs/libtrace/libtracefs.git/
> 
> Which will make it a lot easier for applications to interact with the
> tracefs file system. I'm hoping to have this ready for distros by the end
> of the year. We have applications coming that depend on these.
> 
> >
> > We have a relatively sophisticated analysis platform that utilizes raw traces,
> > Among which the upiu trace is the most important and informative.
> >
> > This tool has evolved over the years, adding more and more parsers per
> need,
> > and the users are picking the appropriate parser per the trace they used.
> >
> > We will surely be glad to adopt new tracing capabilities,
> 
> I think libtraceevent and libtracefs would be a much welcome addition for
> upiu trace as it would be reading raw data (very fast), and have an API
> that makes doing so much simpler. For example, I just wrote a quick program
> that checks what files an application opens (this is not in anyway
> production ready):
> 
>   http://rostedt.org/code/show-open-files.c
> 
> > But we would prefer not to break anything.
> 
> Of course!
> 
> And again, I would be happy to help out in converting to this libraries. It
> will make your applications more robust, as they make it so that you do not
> need to rely on the order of fields.
> 
> Note, there's plans on making these libraries python modules as well (to
> have python scripts enable and read ftrace data).
Thanks a lot for your insightful comments.
We will surely look into libtraceevent and libtracefs

Thanks,
Avri
> 
> -- Steve

RE: [PATCH v1 3/3] scsi: ufs: Make UPIU trace easier differentiate among CDB, OSF, and TM

[Avri Altman]

> 
> On Mon, 2020-12-07 at 07:57 +0000, Avri Altman wrote:
> > >          TP_printk(
> > > -               "%s: %s: HDR:%s, CDB:%s",
> > > +               "%s: %s: HDR:%s, %s:%s",
> > >                  __get_str(str), __get_str(dev_name),
> > >                  __print_hex(__entry->hdr, sizeof(__entry->hdr)),
> > > +               __get_str(tsf_type),
> >
> > This breaks what current parsers expects.
> > Why str is not enough to distinguish between the command?
> >
> > Thanks,
> > Avri
> 
> Hi Avri
> Tt donesn't break original CDB parser. for the CDB, it is still the
> same as before. Here just make Transaction Specific Fields in the UPIU
> package much clearer.
It does breaks our current parser that expects "CDB:" for all upiu types

> 
> I mentioned in the commits message:
> 
> Transaction Specific Fields (TSF) in the UPIU package could be CDB
> (SCSI/UFS Command Descriptor Block), OSF (Opcode Specific Field), and
> TM I/O parameter (Task Management Input/Output Parameter). But we
> didn't differenciate them. we take all of these as CDB. This is wrong.
> 
> I want to make it clearer and make UPIU trace in line with the Spec.
> what's more,  how do you filter OSF, TM parameters with current UPIU
> trace? you take all of them as CDB? if so, I think, it's better to
> change parser.
Indeed, it is just a small change, but breaking user-space is not an acceptable approach.
Also, the upiu tracer was never meant to be human-readable: it just dump the upiu,
Which contains all the info required to parse it anyway,
So breaking user-space just to making it more readable doesn't really make sense?

Looking at the previous 2 patches of this series, I was hoping that you will do the same for
Command upiu, as well?
Again - same comment: if you are doing that need to change the str not to break current parsers.

Thanks,
Avri


> 
> Thanks,
> Bean
> 
> 
> 
> 
> 

Re: [PATCH v1 1/3] scsi: ufs: Distinguish between query REQ and query RSP in query trace

[Bean Huo]

On Mon, 2020-12-07 at 10:21 -0500, Steven Rostedt wrote:
> > @@ -321,9 +321,15 @@ static void ufshcd_add_cmd_upiu_trace(struct
> > ufs_hba *hba, unsigned int tag,
> >   static void ufshcd_add_query_upiu_trace(struct ufs_hba *hba,
> > unsigned int tag,
> >                const char *str)
> >   {
> > -     struct utp_upiu_req *rq = hba->lrb[tag].ucd_req_ptr;
> > +     struct utp_upiu_req *rq_rsp;
> > +
> 
> I would add:
> 
>         if (!trace_ufshcd_upiu_enabled())
>                 return;
> 
> Why do the work if the trace point is not enabled?
> 
> -- Steve

Steve,

Thanks a lot, I will fix it in the next version.


Thanks,
Bean

Re: [PATCH v1 3/3] scsi: ufs: Make UPIU trace easier differentiate among CDB, OSF, and TM

[Bean Huo]

On Mon, 2020-12-07 at 10:34 -0500, Steven Rostedt wrote:
> On Sun,  6 Dec 2020 17:42:26 +0100
> Bean Huo <huobean@gmail.com> wrote:
> 
> > From: Bean Huo <beanhuo@micron.com>
> > 
> > Transaction Specific Fields (TSF) in the UPIU package could be CDB
> > (SCSI/UFS Command Descriptor Block), OSF (Opcode Specific Field),
> > and
> > TM I/O parameter (Task Management Input/Output Parameter). But,
> > currently,
> > we take all of these as CDB  in the UPIU trace. Thus makes user
> > confuse
> > among CDB, OSF, and TM message. So fix it with this patch.
> > 
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
> > ---
> >  drivers/scsi/ufs/ufshcd.c  |  9 +++++----
> >  include/trace/events/ufs.h | 10 +++++++---
> >  2 files changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 29d7240a61bf..5b2219e44743 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -315,7 +315,8 @@ static void ufshcd_add_cmd_upiu_trace(struct
> > ufs_hba *hba, unsigned int tag,
> >  {
> >  	struct utp_upiu_req *rq = hba->lrb[tag].ucd_req_ptr;
> >  
> > -	trace_ufshcd_upiu(dev_name(hba->dev), str, &rq->header, &rq-
> > >sc.cdb);
> > +	trace_ufshcd_upiu(dev_name(hba->dev), str, &rq->header, &rq-
> > >sc.cdb,
> > +			  "CDB");
> >  }
> >  
> >  static void ufshcd_add_query_upiu_trace(struct ufs_hba *hba,
> > unsigned int tag,
> > @@ -329,7 +330,7 @@ static void ufshcd_add_query_upiu_trace(struct
> > ufs_hba *hba, unsigned int tag,
> >  		rq_rsp = (struct utp_upiu_req *)hba-
> > >lrb[tag].ucd_rsp_ptr;
> >  
> >  	trace_ufshcd_upiu(dev_name(hba->dev), str, &rq_rsp->header,
> > -			  &rq_rsp->qr);
> > +			  &rq_rsp->qr, "OSF");
> >  }
> >  
> >  static void ufshcd_add_tm_upiu_trace(struct ufs_hba *hba, unsigned
> > int tag,
> > @@ -340,10 +341,10 @@ static void ufshcd_add_tm_upiu_trace(struct
> > ufs_hba *hba, unsigned int tag,
> >  
> >  	if (!strcmp("tm_send", str))
> >  		trace_ufshcd_upiu(dev_name(hba->dev), str, &descp-
> > >req_header,
> > -				  &descp->input_param1);
> > +				  &descp->input_param1, "TM_INPUT");
> >  	else
> >  		trace_ufshcd_upiu(dev_name(hba->dev), str, &descp-
> > >rsp_header,
> > -				  &descp->output_param1);
> > +				  &descp->output_param1, "TM_OUTPUT");
> 
> You could save some space on the ring buffer, if you made the above
> into an
> enum, and then used print_symbolic().
> 
> >  }
> >  
> >  static void ufshcd_add_uic_command_trace(struct ufs_hba *hba,
> > diff --git a/include/trace/events/ufs.h
> > b/include/trace/events/ufs.h
> > index 0bd54a184391..68e8e97a9b47 100644
> > --- a/include/trace/events/ufs.h
> > +++ b/include/trace/events/ufs.h
> > @@ -295,15 +295,17 @@ TRACE_EVENT(ufshcd_uic_command,
> >  );
> 
> 
> You could make this:
> 
> #define TRACE_TSF_TYPES		\
> 	EM(CDB)			\
> 	EM(OSF)			\
> 	EM(TM_INPUT)		\
> 	EMe(TM_OUTPUT)
> 
> #ifndef TRACE_TSF_TYPES_ENUMS
> #define TRACE_TSF_TYPES_ENUMS
> #undef EM
> #undef EMe
> 
> #define EM(x)	TRACE_TSF_##x,
> #define EMe(x)	TRACE_TSF_##x
> 
> enum {
> 	TRACE_TSF_TYPES
> }
> #endif /* TRACE_TSF_TYPES_ENUMS */
> 
> #undef EM
> #undef EMe
> 
> /* These export the enum names to user space */
> #define EM(x)	TRACE_DEFINE_ENUM(TRACE_TSF_##x)
> #define EMe(x)	TRACE_DEFINE_ENUM(TRACE_TSF_##x)
> 
> TRACE_TSF_TYPES
> 
> #undef EM
> #undef EMe
> 
> /* These are used in the print_symbolic */
> #define EM(x) { TRACE_TSF_##x, #x },
> #define EMe(x) { TRACE_TSF_##x, #x }
> 
> 
> >  
> >  TRACE_EVENT(ufshcd_upiu,
> > -	TP_PROTO(const char *dev_name, const char *str, void *hdr, void
> > *tsf),
> > +	TP_PROTO(const char *dev_name, const char *str, void *hdr, void
> > *tsf,
> > +		 const char *tsf_type),
> 
> 		int tsf_type;
> 
> >  
> > -	TP_ARGS(dev_name, str, hdr, tsf),
> > +	TP_ARGS(dev_name, str, hdr, tsf, tsf_type),
> >  
> >  	TP_STRUCT__entry(
> >  		__string(dev_name, dev_name)
> >  		__string(str, str)
> >  		__array(unsigned char, hdr, 12)
> >  		__array(unsigned char, tsf, 16)
> > +		__string(tsf_type, tsf_type)
> 
> 		__field(int, tsf_type)
> 
> >  	),
> >  
> >  	TP_fast_assign(
> > @@ -311,12 +313,14 @@ TRACE_EVENT(ufshcd_upiu,
> >  		__assign_str(str, str);
> >  		memcpy(__entry->hdr, hdr, sizeof(__entry->hdr));
> >  		memcpy(__entry->tsf, tsf, sizeof(__entry->tsf));
> > +		__assign_str(tsf_type, tsf_type);
> 
> 
> 		__entry->tsf_type = tsf_type;
> 
> 
> >  	),
> >  
> >  	TP_printk(
> > -		"%s: %s: HDR:%s, CDB:%s",
> > +		"%s: %s: HDR:%s, %s:%s",
> >  		__get_str(str), __get_str(dev_name),
> >  		__print_hex(__entry->hdr, sizeof(__entry->hdr)),
> > +		__get_str(tsf_type),
> 
> 		print_symbolic(tsf_type, TRACE_TSF_TYPES),
> 
> -- Steve
> 
> 
> >  		__print_hex(__entry->tsf, sizeof(__entry->tsf))
> >  	)
> >  );
> 
> 

Thanks Bart and Steve,
That's nice, I will change them in the next version.

Bean

Re: [PATCH v1 3/3] scsi: ufs: Make UPIU trace easier differentiate among CDB, OSF, and TM

[Bean Huo]

On Tue, 2020-12-08 at 08:35 +0000, Avri Altman wrote:
> > didn't differenciate them. we take all of these as CDB. This is
> > wrong.
> > 
> > I want to make it clearer and make UPIU trace in line with the
> > Spec.
> > what's more,  how do you filter OSF, TM parameters with current
> > UPIU
> > trace? you take all of them as CDB? if so, I think, it's better to
> > change parser.
> 
> Indeed, it is just a small change, but breaking user-space is not an
> acceptable approach.
> Also, the upiu tracer was never meant to be human-readable: it just
> dump the upiu,
> Which contains all the info required to parse it anyway,
> So breaking user-space just to making it more readable doesn't really
> make sense?
> 
> Looking at the previous 2 patches of this series, I was hoping that
> you will do the same for
> Command upiu, as well?
> Again - same comment: if you are doing that need to change the str
> not to break current parsers.
> 
> Thanks,
> Avri

will not change original CDB format, just add new OSF, TM.
the string format will not be change. The current the HDR and CDB in
the send and complete trace are the same, I guess, you even didn't
trace CDB in your parser, they cannot tell you the request execution
result.

Bean