On Mon, 2020-12-07 at 10:34 -0500, Steven Rostedt wrote:
> On Sun, 6 Dec 2020 17:42:26 +0100
> Bean Huo <huobean@xxxxxxxxx> wrote:
>
> > From: Bean Huo <beanhuo@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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
