[PATCH v1] scsi: ufs: core: Add trace event for MCQ

[PATCH v1] scsi: ufs: core: Add trace event for MCQ

[Ziqi Chen]

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,
+				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),
-- 
2.7.4

Re: [PATCH v1] scsi: ufs: core: Add trace event for MCQ

[Steven Rostedt]

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),

Re: [PATCH v1] scsi: ufs: core: Add trace event for MCQ

[Bart Van Assche]

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.

Re: [PATCH v1] scsi: ufs: core: Add trace event for MCQ

[Ziqi Chen]

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),

Re: [PATCH v1] scsi: ufs: core: Add trace event for MCQ

[Ziqi Chen]

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.
>