| Message ID | 20201206164226.6595-4-huobean@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Three fixes for the UPIU trace | expand |
> > > 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
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
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)) > ) > );
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
> > 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
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
> > 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
> > 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 > > > > >
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
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
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)) ) );