On Mon, 2020-12-07 at 10:34 -0500, Steven Rostedt wrote: > On Sun, 6 Dec 2020 17:42:26 +0100 > Bean Huo <[email protected]> wrote: > > > From: Bean Huo <[email protected]> > > > > 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 <[email protected]> > > --- > > 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
