| Message ID | 20201005223635.2922805-4-jaegeuk@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/4] scsi: ufs: atomic update for clkgating_enable | expand |
On 2020-10-06 06:36, Jaegeuk Kim wrote: > From: Jaegeuk Kim <jaegeuk@google.com> > > This adds user-friendly tracepoints with group id. > > Cc: Alim Akhtar <alim.akhtar@samsung.com> > Cc: Avri Altman <avri.altman@wdc.com> > Cc: Can Guo <cang@codeaurora.org> > Signed-off-by: Jaegeuk Kim <jaegeuk@google.com> Reviewed-by: Can Guo <cang@codeaurora.org> > --- > drivers/scsi/ufs/ufshcd.c | 6 ++++-- > include/trace/events/ufs.h | 21 +++++++++++++++++---- > 2 files changed, 21 insertions(+), 6 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 76e95963887be..a2db8182663da 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -336,7 +336,7 @@ static void ufshcd_add_command_trace(struct ufs_hba > *hba, > unsigned int tag, const char *str) > { > sector_t lba = -1; > - u8 opcode = 0; > + u8 opcode = 0, group_id = 0; > u32 intr, doorbell; > struct ufshcd_lrb *lrbp = &hba->lrb[tag]; > struct scsi_cmnd *cmd = lrbp->cmd; > @@ -362,13 +362,15 @@ static void ufshcd_add_command_trace(struct > ufs_hba *hba, > lba = cmd->request->bio->bi_iter.bi_sector; > transfer_len = be32_to_cpu( > lrbp->ucd_req_ptr->sc.exp_data_transfer_len); > + if (opcode == WRITE_10) > + group_id = lrbp->cmd->cmnd[6]; > } > } > > intr = ufshcd_readl(hba, REG_INTERRUPT_STATUS); > doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL); > trace_ufshcd_command(dev_name(hba->dev), str, tag, > - doorbell, transfer_len, intr, lba, opcode); > + doorbell, transfer_len, intr, lba, opcode, group_id); > } > > static void ufshcd_print_clk_freqs(struct ufs_hba *hba) > diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h > index 84841b3a7ffd5..50654f3526392 100644 > --- a/include/trace/events/ufs.h > +++ b/include/trace/events/ufs.h > @@ -11,6 +11,15 @@ > > #include <linux/tracepoint.h> > > +#define str_opcode(opcode) \ > + __print_symbolic(opcode, \ > + { WRITE_16, "WRITE_16" }, \ > + { WRITE_10, "WRITE_10" }, \ > + { READ_16, "READ_16" }, \ > + { READ_10, "READ_10" }, \ > + { SYNCHRONIZE_CACHE, "SYNC" }, \ > + { UNMAP, "UNMAP" }) > + > #define UFS_LINK_STATES \ > EM(UIC_LINK_OFF_STATE) \ > EM(UIC_LINK_ACTIVE_STATE) \ > @@ -215,9 +224,10 @@ DEFINE_EVENT(ufshcd_template, ufshcd_init, > TRACE_EVENT(ufshcd_command, > TP_PROTO(const char *dev_name, const char *str, unsigned int tag, > u32 doorbell, int transfer_len, u32 intr, u64 lba, > - u8 opcode), > + u8 opcode, u8 group_id), > > - TP_ARGS(dev_name, str, tag, doorbell, transfer_len, intr, lba, > opcode), > + TP_ARGS(dev_name, str, tag, doorbell, transfer_len, > + intr, lba, opcode, group_id), > > TP_STRUCT__entry( > __string(dev_name, dev_name) > @@ -228,6 +238,7 @@ TRACE_EVENT(ufshcd_command, > __field(u32, intr) > __field(u64, lba) > __field(u8, opcode) > + __field(u8, group_id) > ), > > TP_fast_assign( > @@ -239,13 +250,15 @@ TRACE_EVENT(ufshcd_command, > __entry->intr = intr; > __entry->lba = lba; > __entry->opcode = opcode; > + __entry->group_id = group_id; > ), > > TP_printk( > - "%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, opcode: > 0x%x", > + "%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, opcode: > 0x%x (%s), group_id: 0x%x", > __get_str(str), __get_str(dev_name), __entry->tag, > __entry->doorbell, __entry->transfer_len, > - __entry->intr, __entry->lba, (u32)__entry->opcode > + __entry->intr, __entry->lba, (u32)__entry->opcode, > + str_opcode(__entry->opcode), (u32)__entry->group_id > ) > );
> > On 2020-10-06 06:36, Jaegeuk Kim wrote: > > From: Jaegeuk Kim <jaegeuk@google.com> > > > > This adds user-friendly tracepoints with group id. You have the entire cdb as part of the upiu trace, Can't you parse what you need from there? Thanks, Avri
On 2020-10-20 18:51, Avri Altman wrote: >> >> On 2020-10-06 06:36, Jaegeuk Kim wrote: >> > From: Jaegeuk Kim <jaegeuk@google.com> >> > >> > This adds user-friendly tracepoints with group id. > You have the entire cdb as part of the upiu trace, > Can't you parse what you need from there? > > Thanks, > Avri Yes, but assume we have a large trace log file, having a groud id allows us to filter the data by it easily, right? Thanks, Can Guo.
> > On 2020-10-20 18:51, Avri Altman wrote: > >> > >> On 2020-10-06 06:36, Jaegeuk Kim wrote: > >> > From: Jaegeuk Kim <jaegeuk@google.com> > >> > > >> > This adds user-friendly tracepoints with group id. > > You have the entire cdb as part of the upiu trace, > > Can't you parse what you need from there? > > > > Thanks, > > Avri > > Yes, but assume we have a large trace log file, having a > groud id allows us to filter the data by it easily, right? > Ahha, ok. Thanks, Avri
On 2020-10-20 19:02, Can Guo wrote: > On 2020-10-20 18:51, Avri Altman wrote: >>> >>> On 2020-10-06 06:36, Jaegeuk Kim wrote: >>> > From: Jaegeuk Kim <jaegeuk@google.com> >>> > >>> > This adds user-friendly tracepoints with group id. >> You have the entire cdb as part of the upiu trace, >> Can't you parse what you need from there? >> >> Thanks, >> Avri > > Yes, but assume we have a large trace log file, having a > groud id allows us to filter the data by it easily, right? > > Thanks, > > Can Guo. I just dobule checked WRITE(10)'s CDB, byte 6 has group ID ONLY. So Avri is right, we don't even need to parse it, we can easily filter a ftrace log file by byte 6 to get the WRITE(10) cmds with specific group ID - we don't need this change. Thanks, Can Guo.
On 2020-10-20 19:57, Can Guo wrote: > On 2020-10-20 19:02, Can Guo wrote: >> On 2020-10-20 18:51, Avri Altman wrote: >>>> >>>> On 2020-10-06 06:36, Jaegeuk Kim wrote: >>>> > From: Jaegeuk Kim <jaegeuk@google.com> >>>> > >>>> > This adds user-friendly tracepoints with group id. >>> You have the entire cdb as part of the upiu trace, >>> Can't you parse what you need from there? >>> >>> Thanks, >>> Avri >> >> Yes, but assume we have a large trace log file, having a >> groud id allows us to filter the data by it easily, right? >> >> Thanks, >> >> Can Guo. > > I just dobule checked WRITE(10)'s CDB, byte 6 has group > ID ONLY. So Avri is right, we don't even need to parse it, > we can easily filter a ftrace log file by byte 6 to get the > WRITE(10) cmds with specific group ID - we don't need this > change. > > Thanks, > > Can Guo. Please ignore my previous mail, I misunderstood the change. :( You have my reivewed-by tag for this change. Regards, Can Guo.
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 76e95963887be..a2db8182663da 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -336,7 +336,7 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag, const char *str) { sector_t lba = -1; - u8 opcode = 0; + u8 opcode = 0, group_id = 0; u32 intr, doorbell; struct ufshcd_lrb *lrbp = &hba->lrb[tag]; struct scsi_cmnd *cmd = lrbp->cmd; @@ -362,13 +362,15 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, lba = cmd->request->bio->bi_iter.bi_sector; transfer_len = be32_to_cpu( lrbp->ucd_req_ptr->sc.exp_data_transfer_len); + if (opcode == WRITE_10) + group_id = lrbp->cmd->cmnd[6]; } } intr = ufshcd_readl(hba, REG_INTERRUPT_STATUS); doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL); trace_ufshcd_command(dev_name(hba->dev), str, tag, - doorbell, transfer_len, intr, lba, opcode); + doorbell, transfer_len, intr, lba, opcode, group_id); } static void ufshcd_print_clk_freqs(struct ufs_hba *hba) diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h index 84841b3a7ffd5..50654f3526392 100644 --- a/include/trace/events/ufs.h +++ b/include/trace/events/ufs.h @@ -11,6 +11,15 @@ #include <linux/tracepoint.h> +#define str_opcode(opcode) \ + __print_symbolic(opcode, \ + { WRITE_16, "WRITE_16" }, \ + { WRITE_10, "WRITE_10" }, \ + { READ_16, "READ_16" }, \ + { READ_10, "READ_10" }, \ + { SYNCHRONIZE_CACHE, "SYNC" }, \ + { UNMAP, "UNMAP" }) + #define UFS_LINK_STATES \ EM(UIC_LINK_OFF_STATE) \ EM(UIC_LINK_ACTIVE_STATE) \ @@ -215,9 +224,10 @@ DEFINE_EVENT(ufshcd_template, ufshcd_init, TRACE_EVENT(ufshcd_command, TP_PROTO(const char *dev_name, const char *str, unsigned int tag, u32 doorbell, int transfer_len, u32 intr, u64 lba, - u8 opcode), + u8 opcode, u8 group_id), - TP_ARGS(dev_name, str, tag, doorbell, transfer_len, intr, lba, opcode), + TP_ARGS(dev_name, str, tag, doorbell, transfer_len, + intr, lba, opcode, group_id), TP_STRUCT__entry( __string(dev_name, dev_name) @@ -228,6 +238,7 @@ TRACE_EVENT(ufshcd_command, __field(u32, intr) __field(u64, lba) __field(u8, opcode) + __field(u8, group_id) ), TP_fast_assign( @@ -239,13 +250,15 @@ TRACE_EVENT(ufshcd_command, __entry->intr = intr; __entry->lba = lba; __entry->opcode = opcode; + __entry->group_id = group_id; ), TP_printk( - "%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, opcode: 0x%x", + "%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, opcode: 0x%x (%s), group_id: 0x%x", __get_str(str), __get_str(dev_name), __entry->tag, __entry->doorbell, __entry->transfer_len, - __entry->intr, __entry->lba, (u32)__entry->opcode + __entry->intr, __entry->lba, (u32)__entry->opcode, + str_opcode(__entry->opcode), (u32)__entry->group_id ) );