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

[PATCH v5] 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>

---
Changes to v4:
- Merged MCQ and SDB trace event as one.

Changes to v3:
- Free trace_ufshcd_command_mcq() from dependency on trace_ufshcd_command().

Changes to v2:
- Shorten printing strings.

Changes to v1:
- Adjust the order of fileds to keep them aligned.
---
 drivers/ufs/core/ufshcd.c  | 15 ++++++++++++---
 include/trace/events/ufs.h | 24 +++++++++++++-----------
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 3b3cf78..53a7287 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -422,10 +422,12 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,
 {
 	u64 lba = 0;
 	u8 opcode = 0, group_id = 0;
-	u32 intr, doorbell;
+	u32 doorbell = 0, hwq_id = 0;
+	u32 intr;
 	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 = NULL;
 	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);
+
+	if (is_mcq_enabled(hba)) {
+		hwq = ufshcd_mcq_req_to_hwq(hba, rq);
+		hwq_id = hwq->id;
+	} else {
+		doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
+		hwq_id = -1;
+	}
 	trace_ufshcd_command(dev_name(hba->dev), str_t, tag,
-			doorbell, transfer_len, intr, lba, opcode, group_id);
+			doorbell, hwq_id, 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..37637ff 100644
--- a/include/trace/events/ufs.h
+++ b/include/trace/events/ufs.h
@@ -268,20 +268,21 @@ DEFINE_EVENT(ufshcd_template, ufshcd_wl_runtime_resume,
 
 TRACE_EVENT(ufshcd_command,
 	TP_PROTO(const char *dev_name, enum ufs_trace_str_t str_t,
-		 unsigned int tag, u32 doorbell, int transfer_len, u32 intr,
-		 u64 lba, u8 opcode, u8 group_id),
+		 unsigned int tag, u32 doorbell, u32 hwq_id, int transfer_len,
+		 u32 intr, u64 lba, u8 opcode, u8 group_id),
 
-	TP_ARGS(dev_name, str_t, tag, doorbell, transfer_len,
-				intr, lba, opcode, group_id),
+	TP_ARGS(dev_name, str_t, tag, doorbell, hwq_id, 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, doorbell)
-		__field(int, transfer_len)
+		__field(u32, hwq_id)
 		__field(u32, intr)
 		__field(u64, lba)
+		__field(int, transfer_len)
 		__field(u8, opcode)
 		__field(u8, group_id)
 	),
@@ -291,19 +292,20 @@ TRACE_EVENT(ufshcd_command,
 		__entry->str_t = str_t;
 		__entry->tag = tag;
 		__entry->doorbell = doorbell;
-		__entry->transfer_len = transfer_len;
+		__entry->hwq_id = hwq_id;
 		__entry->intr = intr;
 		__entry->lba = lba;
+		__entry->transfer_len = transfer_len;
 		__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), group_id: 0x%x",
-		show_ufs_cmd_trace_str(__entry->str_t), __get_str(dev_name),
-		__entry->tag, __entry->doorbell, __entry->transfer_len,
-		__entry->intr, __entry->lba, (u32)__entry->opcode,
-		str_opcode(__entry->opcode), (u32)__entry->group_id
+		"%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, opcode: 0x%x (%s), group_id: 0x%x, hqid: %d",
+		 show_ufs_cmd_trace_str(__entry->str_t), __get_str(dev_name),
+		 __entry->tag, __entry->doorbell, __entry->transfer_len, __entry->intr,
+		 __entry->lba, (u32)__entry->opcode, str_opcode(__entry->opcode),
+		 (u32)__entry->group_id, __entry->hwq_id
 	)
 );
 
-- 
2.7.4

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

[Bart Van Assche]

On 3/13/23 02:16, Ziqi Chen 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.

Added a new trace event ... -> Add the MCQ information in the existing 
trace event ...

 From Documentation/process/submitting-patches.rst: "Describe your 
changes in imperative mood, e.g. "make xyzzy do frotz"".

> Changes to v1:
> - Adjust the order of fileds to keep them aligned.
                         ^^^^^^
                         fields?

> -	u32 intr, doorbell;
> +	u32 doorbell = 0, hwq_id = 0;
> +	u32 intr;

Since hwq_id can be assigned the value -1, shouldn't its type be changed 
from u32 into int, int16_t or int32_t?

> +	struct ufs_hw_queue *hwq = NULL;

Is this pointer only used inside one branch of the if-statement below? 
If so, can it be moved under "if (is_mcq_enabled(hba)) {"?

> +		"%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, opcode: 0x%x (%s), group_id: 0x%x, hqid: %d",

Consider changing "hqid" into "hwq_id" to make the trace information 
easier to understand.

Thanks,

Bart.

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

[Ziqi Chen]

Thanks Bart, will address your comments and update new version V6 soon.

Best Regards,
Ziqi

On 3/14/2023 5:09 AM, Bart Van Assche wrote:
> On 3/13/23 02:16, Ziqi Chen 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.
> 
> Added a new trace event ... -> Add the MCQ information in the existing 
> trace event ...
> 
>  From Documentation/process/submitting-patches.rst: "Describe your 
> changes in imperative mood, e.g. "make xyzzy do frotz"".
> 
>> Changes to v1:
>> - Adjust the order of fileds to keep them aligned.
>                          ^^^^^^
>                          fields?
> 
>> -    u32 intr, doorbell;
>> +    u32 doorbell = 0, hwq_id = 0;
>> +    u32 intr;
> 
> Since hwq_id can be assigned the value -1, shouldn't its type be changed 
> from u32 into int, int16_t or int32_t?
> 
>> +    struct ufs_hw_queue *hwq = NULL;
> 
> Is this pointer only used inside one branch of the if-statement below? 
> If so, can it be moved under "if (is_mcq_enabled(hba)) {"?
> 
>> +        "%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, 
>> opcode: 0x%x (%s), group_id: 0x%x, hqid: %d",
> 
> Consider changing "hqid" into "hwq_id" to make the trace information 
> easier to understand.
> 
> Thanks,
> 
> Bart.