| Message ID | 1676515562-55805-1-git-send-email-quic_ziqichen@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] scsi: ufs: core: Add trace event for MCQ | expand |
On Thu, 16 Feb 2023 10:45:55 +0800 Ziqi Chen <quic_ziqichen@quicinc.com> wrote: > Added a new trace event to record MCQ relevant information > for each request in MCQ mode, include hardware queue ID, > SQ tail slot, CQ head slot and CQ tail slot. > > Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com> > --- > drivers/ufs/core/ufshcd.c | 15 ++++++++++++--- > include/trace/events/ufs.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 60 insertions(+), 3 deletions(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 3b3cf78..0037f4f 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -426,6 +426,8 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag, > struct ufshcd_lrb *lrbp = &hba->lrb[tag]; > struct scsi_cmnd *cmd = lrbp->cmd; > struct request *rq = scsi_cmd_to_rq(cmd); > + struct ufs_hw_queue *hwq; > + > int transfer_len = -1; > > if (!cmd) > @@ -456,9 +458,16 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag, > } > > 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_t, tag, > - doorbell, transfer_len, intr, lba, opcode, group_id); > + > + if (is_mcq_enabled(hba)) { > + hwq = ufshcd_mcq_req_to_hwq(hba, rq); > + trace_ufshcd_command_mcq(dev_name(hba->dev), str_t, tag, It's better to move the processing of the "dev_name(hba->dev) into the trace event code. Maybe even just pass in hba. > + hwq, transfer_len, intr, lba, opcode, group_id); > + } else { > + doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL); > + trace_ufshcd_command(dev_name(hba->dev), str_t, tag, Here too. > + 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 599739e..a406404e 100644 > --- a/include/trace/events/ufs.h > +++ b/include/trace/events/ufs.h > @@ -10,6 +10,7 @@ > #define _TRACE_UFS_H > > #include <linux/tracepoint.h> > +#include <ufs/ufshcd.h> > > #define str_opcode(opcode) \ > __print_symbolic(opcode, \ > @@ -307,6 +308,53 @@ TRACE_EVENT(ufshcd_command, > ) > ); > > +TRACE_EVENT(ufshcd_command_mcq, > + TP_PROTO(const char *dev_name, enum ufs_trace_str_t str_t, So you would pass in the type of hba here. > + unsigned int tag, struct ufs_hw_queue *hwq, int transfer_len, > + u32 intr, u64 lba, u8 opcode, u8 group_id), > + > + TP_ARGS(dev_name, str_t, tag, hwq, transfer_len, intr, lba, opcode, group_id), > + > + TP_STRUCT__entry( > + __string(dev_name, dev_name) __string(dname, dev_name(hba->dev)) > + __field(enum ufs_trace_str_t, str_t) > + __field(unsigned int, tag) > + __field(u32, hwq_id) > + __field(int, transfer_len) > + __field(u32, intr) > + __field(u64, lba) > + __field(u8, opcode) > + __field(u8, group_id) > + __field(u32, sq_tail) > + __field(u32, cq_head) > + __field(u32, cq_tail) I bet the above has a lot of holes in it. The above is not packed, so make sure you count the size of each field and try to keep them aligned. > + ), > + > + TP_fast_assign( > + __assign_str(dev_name, dev_name); __assign_str(dname, dev_name(hba->dev)); > + __entry->str_t = str_t; > + __entry->tag = tag; > + __entry->hwq_id = hwq->id; > + __entry->transfer_len = transfer_len; > + __entry->intr = intr; > + __entry->lba = lba; > + __entry->opcode = opcode; > + __entry->group_id = group_id; > + __entry->sq_tail = hwq->sq_tail_slot; > + __entry->cq_head = hwq->cq_head_slot; > + __entry->cq_tail = hwq->cq_tail_slot; > + ), > + > + TP_printk( > + "%s: %s: tag: %u, hwq_id: %d, size: %d, IS: %u, LBA: %llu, opcode: 0x%x (%s), group_id: 0x%x, sq_tail_slot: %d, cq_head_slot: %d, cq_tail_slot: %d", > + show_ufs_cmd_trace_str(__entry->str_t), __get_str(dev_name), __get_str(dname), -- Steve > + __entry->tag, __entry->hwq_id, __entry->transfer_len, > + __entry->intr, __entry->lba, (u32)__entry->opcode, > + str_opcode(__entry->opcode), (u32)__entry->group_id, > + __entry->sq_tail, __entry->cq_head, __entry->cq_tail > + ) > +); > + > TRACE_EVENT(ufshcd_uic_command, > TP_PROTO(const char *dev_name, enum ufs_trace_str_t str_t, u32 cmd, > u32 arg1, u32 arg2, u32 arg3),
On 2/15/23 18:45, Ziqi Chen wrote: > diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h > index 599739e..a406404e 100644 > --- a/include/trace/events/ufs.h > +++ b/include/trace/events/ufs.h > @@ -10,6 +10,7 @@ > #define _TRACE_UFS_H > > #include <linux/tracepoint.h> > +#include <ufs/ufshcd.h> Would including <ufs/ufs.h> here be sufficient? Thanks, Bart.
On 2/17/2023 1:22 AM, Steven Rostedt wrote: > On Thu, 16 Feb 2023 10:45:55 +0800 > Ziqi Chen <quic_ziqichen@quicinc.com> wrote: > >> Added a new trace event to record MCQ relevant information >> for each request in MCQ mode, include hardware queue ID, >> SQ tail slot, CQ head slot and CQ tail slot. >> >> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com> >> --- >> drivers/ufs/core/ufshcd.c | 15 ++++++++++++--- >> include/trace/events/ufs.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 60 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c >> index 3b3cf78..0037f4f 100644 >> --- a/drivers/ufs/core/ufshcd.c >> +++ b/drivers/ufs/core/ufshcd.c >> @@ -426,6 +426,8 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag, >> struct ufshcd_lrb *lrbp = &hba->lrb[tag]; >> struct scsi_cmnd *cmd = lrbp->cmd; >> struct request *rq = scsi_cmd_to_rq(cmd); >> + struct ufs_hw_queue *hwq; >> + >> int transfer_len = -1; >> >> if (!cmd) >> @@ -456,9 +458,16 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag, >> } >> >> 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_t, tag, >> - doorbell, transfer_len, intr, lba, opcode, group_id); >> + >> + if (is_mcq_enabled(hba)) { >> + hwq = ufshcd_mcq_req_to_hwq(hba, rq); >> + trace_ufshcd_command_mcq(dev_name(hba->dev), str_t, tag, > It's better to move the processing of the "dev_name(hba->dev) into the > trace event code. Maybe even just pass in hba. Hi Steven, it is a good idea, but if so do .we need to make same change to all other UFS trace events to stay the same style. that would be a big change. > >> + hwq, transfer_len, intr, lba, opcode, group_id); >> + } else { >> + doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL); >> + trace_ufshcd_command(dev_name(hba->dev), str_t, tag, > Here too. > >> + 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 599739e..a406404e 100644 >> --- a/include/trace/events/ufs.h >> +++ b/include/trace/events/ufs.h >> @@ -10,6 +10,7 @@ >> #define _TRACE_UFS_H >> >> #include <linux/tracepoint.h> >> +#include <ufs/ufshcd.h> >> >> #define str_opcode(opcode) \ >> __print_symbolic(opcode, \ >> @@ -307,6 +308,53 @@ TRACE_EVENT(ufshcd_command, >> ) >> ); >> >> +TRACE_EVENT(ufshcd_command_mcq, >> + TP_PROTO(const char *dev_name, enum ufs_trace_str_t str_t, > So you would pass in the type of hba here. > >> + unsigned int tag, struct ufs_hw_queue *hwq, int transfer_len, >> + u32 intr, u64 lba, u8 opcode, u8 group_id), >> + >> + TP_ARGS(dev_name, str_t, tag, hwq, transfer_len, intr, lba, opcode, group_id), >> + >> + TP_STRUCT__entry( >> + __string(dev_name, dev_name) > __string(dname, dev_name(hba->dev)) > >> + __field(enum ufs_trace_str_t, str_t) >> + __field(unsigned int, tag) >> + __field(u32, hwq_id) >> + __field(int, transfer_len) >> + __field(u32, intr) >> + __field(u64, lba) >> + __field(u8, opcode) >> + __field(u8, group_id) >> + __field(u32, sq_tail) >> + __field(u32, cq_head) >> + __field(u32, cq_tail) > I bet the above has a lot of holes in it. The above is not packed, so > make sure you count the size of each field and try to keep them aligned. you are right, I will update in next version. Thanks， Ziqi > > > >> + ), >> + >> + TP_fast_assign( >> + __assign_str(dev_name, dev_name); > __assign_str(dname, dev_name(hba->dev)); > >> + __entry->str_t = str_t; >> + __entry->tag = tag; >> + __entry->hwq_id = hwq->id; >> + __entry->transfer_len = transfer_len; >> + __entry->intr = intr; >> + __entry->lba = lba; >> + __entry->opcode = opcode; >> + __entry->group_id = group_id; >> + __entry->sq_tail = hwq->sq_tail_slot; >> + __entry->cq_head = hwq->cq_head_slot; >> + __entry->cq_tail = hwq->cq_tail_slot; >> + ), >> + >> + TP_printk( >> + "%s: %s: tag: %u, hwq_id: %d, size: %d, IS: %u, LBA: %llu, opcode: 0x%x (%s), group_id: 0x%x, sq_tail_slot: %d, cq_head_slot: %d, cq_tail_slot: %d", >> + show_ufs_cmd_trace_str(__entry->str_t), __get_str(dev_name), > __get_str(dname), > > -- Steve > >> + __entry->tag, __entry->hwq_id, __entry->transfer_len, >> + __entry->intr, __entry->lba, (u32)__entry->opcode, >> + str_opcode(__entry->opcode), (u32)__entry->group_id, >> + __entry->sq_tail, __entry->cq_head, __entry->cq_tail >> + ) >> +); >> + >> TRACE_EVENT(ufshcd_uic_command, >> TP_PROTO(const char *dev_name, enum ufs_trace_str_t str_t, u32 cmd, >> u32 arg1, u32 arg2, u32 arg3),
On 2/17/2023 1:44 AM, Bart Van Assche wrote: > On 2/15/23 18:45, Ziqi Chen wrote: >> diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h >> index 599739e..a406404e 100644 >> --- a/include/trace/events/ufs.h >> +++ b/include/trace/events/ufs.h >> @@ -10,6 +10,7 @@ >> #define _TRACE_UFS_H >> #include <linux/tracepoint.h> >> +#include <ufs/ufshcd.h> > > Would including <ufs/ufs.h> here be sufficient? Hi Bart. The struct ufs_hw_queue which be used by this change not be defined in ufs.h. Best Regards, Ziqi > > Thanks, > > Bart. >
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 3b3cf78..0037f4f 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -426,6 +426,8 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag, struct ufshcd_lrb *lrbp = &hba->lrb[tag]; struct scsi_cmnd *cmd = lrbp->cmd; struct request *rq = scsi_cmd_to_rq(cmd); + struct ufs_hw_queue *hwq; + int transfer_len = -1; if (!cmd) @@ -456,9 +458,16 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag, } 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_t, tag, - doorbell, transfer_len, intr, lba, opcode, group_id); + + if (is_mcq_enabled(hba)) { + hwq = ufshcd_mcq_req_to_hwq(hba, rq); + trace_ufshcd_command_mcq(dev_name(hba->dev), str_t, tag, + hwq, transfer_len, intr, lba, opcode, group_id); + } else { + doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL); + trace_ufshcd_command(dev_name(hba->dev), str_t, tag, + 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 599739e..a406404e 100644 --- a/include/trace/events/ufs.h +++ b/include/trace/events/ufs.h @@ -10,6 +10,7 @@ #define _TRACE_UFS_H #include <linux/tracepoint.h> +#include <ufs/ufshcd.h> #define str_opcode(opcode) \ __print_symbolic(opcode, \ @@ -307,6 +308,53 @@ TRACE_EVENT(ufshcd_command, ) ); +TRACE_EVENT(ufshcd_command_mcq, + TP_PROTO(const char *dev_name, enum ufs_trace_str_t str_t, + unsigned int tag, struct ufs_hw_queue *hwq, int transfer_len, + u32 intr, u64 lba, u8 opcode, u8 group_id), + + TP_ARGS(dev_name, str_t, tag, hwq, transfer_len, intr, lba, opcode, group_id), + + TP_STRUCT__entry( + __string(dev_name, dev_name) + __field(enum ufs_trace_str_t, str_t) + __field(unsigned int, tag) + __field(u32, hwq_id) + __field(int, transfer_len) + __field(u32, intr) + __field(u64, lba) + __field(u8, opcode) + __field(u8, group_id) + __field(u32, sq_tail) + __field(u32, cq_head) + __field(u32, cq_tail) + ), + + TP_fast_assign( + __assign_str(dev_name, dev_name); + __entry->str_t = str_t; + __entry->tag = tag; + __entry->hwq_id = hwq->id; + __entry->transfer_len = transfer_len; + __entry->intr = intr; + __entry->lba = lba; + __entry->opcode = opcode; + __entry->group_id = group_id; + __entry->sq_tail = hwq->sq_tail_slot; + __entry->cq_head = hwq->cq_head_slot; + __entry->cq_tail = hwq->cq_tail_slot; + ), + + TP_printk( + "%s: %s: tag: %u, hwq_id: %d, size: %d, IS: %u, LBA: %llu, opcode: 0x%x (%s), group_id: 0x%x, sq_tail_slot: %d, cq_head_slot: %d, cq_tail_slot: %d", + show_ufs_cmd_trace_str(__entry->str_t), __get_str(dev_name), + __entry->tag, __entry->hwq_id, __entry->transfer_len, + __entry->intr, __entry->lba, (u32)__entry->opcode, + str_opcode(__entry->opcode), (u32)__entry->group_id, + __entry->sq_tail, __entry->cq_head, __entry->cq_tail + ) +); + TRACE_EVENT(ufshcd_uic_command, TP_PROTO(const char *dev_name, enum ufs_trace_str_t str_t, u32 cmd, u32 arg1, u32 arg2, u32 arg3),
Added a new trace event to record MCQ relevant information for each request in MCQ mode, include hardware queue ID, SQ tail slot, CQ head slot and CQ tail slot. Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com> --- drivers/ufs/core/ufshcd.c | 15 ++++++++++++--- include/trace/events/ufs.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 3 deletions(-)