[PATCH 1/2] nvme: add set feature tracing support

[Hou Pu]

On Mon, Jul 5, 2021 at 3:16 PM Max Gurtovoy <mgurtovoy at nvidia.com> wrote:
>
>
> On 7/5/2021 6:15 AM, Hou Pu wrote:
> > A nvme connect command produces following trace.
> >
> > Before:
> > /sys/kernel/debug/tracing# cat trace | grep feature
> >      kworker/5:1H-98      [005] ....  3221.294844: nvme_setup_cmd: nvme0: qid=0, cmdid=25, nsid=0, flags=0x0, meta=0x0, cmd=(nvme_admin_set_features cdw10=07 00 00 00 07 00 07 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
> >      kworker/4:1H-124     [004] ....  3222.009186: nvme_setup_cmd: nvme0: qid=0, cmdid=17, nsid=0, flags=0x0, meta=0x0, cmd=(nvme_admin_set_features cdw10=0b 00 00 00 00 09 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
> >
> > After:
> > /sys/kernel/debug/tracing# cat trace | grep feature
> >      kworker/0:1H-253     [000] ....   196.060509: nvme_setup_cmd: nvme0: qid=0, cmdid=29, nsid=0, flags=0x0, meta=0x0, cmd=(nvme_admin_set_features fid=0x7, sv=0x0, cdw11=0x70007)
> >      kworker/0:1H-253     [000] ....   196.763947: nvme_setup_cmd: nvme0: qid=0, cmdid=29, nsid=0, flags=0x0, meta=0x0, cmd=(nvme_admin_set_features fid=0xb, sv=0x0, cdw11=0x900)
> >
> > Using ',' to separate different field like others in
> > nvmet_trace_admin_get_features.
> >
> > Signed-off-by: Hou Pu <houpu.main at gmail.com>
> > ---
> >   drivers/nvme/host/trace.c | 18 +++++++++++++++++-
> >   1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/nvme/host/trace.c b/drivers/nvme/host/trace.c
> > index 6543015b6121..2a89c5aa0790 100644
> > --- a/drivers/nvme/host/trace.c
> > +++ b/drivers/nvme/host/trace.c
> > @@ -72,6 +72,20 @@ static const char *nvme_trace_admin_identify(struct trace_seq *p, u8 *cdw10)
> >       return ret;
> >   }
> >
> > +static const char *nvme_trace_admin_set_features(struct trace_seq *p,
> > +                                              u8 *cdw10)
> > +{
> > +     const char *ret = trace_seq_buffer_ptr(p);
> > +     u8 fid = cdw10[0];
> > +     u8 sv = cdw10[3] & 0x8;
> > +     u32 cdw11 = get_unaligned_le32(cdw10 + 4);
>
> By the spec cdw10, 11, 12, 13, 14, 15 are used by set features command.
>
> At least lets print cdw11-cdw15 as raw data and not only cdw11.
>
> We don't want to loose information.

Yes, it's better to keep these although currently only cdw11 is used.
Will change it in v2. Thanks.

>
> > +
> > +     trace_seq_printf(p, "fid=0x%x, sv=0x%x, cdw11=0x%x", fid, sv, cdw11);
> > +     trace_seq_putc(p, 0);
> > +
> > +     return ret;
> > +}
> > +
> >   static const char *nvme_trace_admin_get_features(struct trace_seq *p,
> >                                                u8 *cdw10)
> >   {
> > @@ -80,7 +94,7 @@ static const char *nvme_trace_admin_get_features(struct trace_seq *p,
> >       u8 sel = cdw10[1] & 0x7;
> >       u32 cdw11 = get_unaligned_le32(cdw10 + 4);
> >
> > -     trace_seq_printf(p, "fid=0x%x sel=0x%x cdw11=0x%x", fid, sel, cdw11);
> > +     trace_seq_printf(p, "fid=0x%x, sel=0x%x, cdw11=0x%x", fid, sel, cdw11);
>
> This can be done in separate commit.
>
> Also if you can fix the current status and print also cdw14 since it's
> used by get features by the spec (in separate commit).

OK. Thanks for the comment.

>
>
> >       trace_seq_putc(p, 0);
> >
> >       return ret;
> > @@ -201,6 +215,8 @@ const char *nvme_trace_parse_admin_cmd(struct trace_seq *p,
> >               return nvme_trace_create_cq(p, cdw10);
> >       case nvme_admin_identify:
> >               return nvme_trace_admin_identify(p, cdw10);
> > +     case nvme_admin_set_features:
> > +             return nvme_trace_admin_set_features(p, cdw10);
> >       case nvme_admin_get_features:
> >               return nvme_trace_admin_get_features(p, cdw10);
> >       case nvme_admin_get_lba_status: