Re: [PATCH v3] ufs: core: add hba parameter to trace events

Re: [PATCH v3] ufs: core: add hba parameter to trace events

[Steven Rostedt]

On Tue, 30 Jun 2026 16:56:12 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> >  
> >  	TP_printk("%s: gating state changed to %s",
> > -		__get_str(dev_name),
> > +		dev_name(__entry->hba->dev),  
> 
> NO YOU CAN NOT DO THIS!!!!

This is why you should always Cc linux-trace-kernel@vger.kernel.org on any
trace event updates. We look to catch bugs like this.

The below patch should fix it, and I'll send it as a proper patch soon:

diff --git a/drivers/ufs/core/ufs_trace.h b/drivers/ufs/core/ufs_trace.h
index 309ae51b4906..377a3c54b9f5 100644
--- a/drivers/ufs/core/ufs_trace.h
+++ b/drivers/ufs/core/ufs_trace.h
@@ -89,16 +89,18 @@ TRACE_EVENT(ufshcd_clk_gating,
 
 	TP_STRUCT__entry(
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(hba->dev))
 		__field(int, state)
 	),
 
 	TP_fast_assign(
+		__assign_str(dev_name);
 		__entry->hba = hba;
 		__entry->state = state;
 	),
 
 	TP_printk("%s: gating state changed to %s",
-		dev_name(__entry->hba->dev),
+		__get_str(dev_name),
 		__print_symbolic(__entry->state, UFSCHD_CLK_GATING_STATES))
 );
 
@@ -111,6 +113,7 @@ TRACE_EVENT(ufshcd_clk_scaling,
 
 	TP_STRUCT__entry(
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(hba->dev))
 		__string(state, state)
 		__string(clk, clk)
 		__field(u32, prev_state)
@@ -119,6 +122,7 @@ TRACE_EVENT(ufshcd_clk_scaling,
 
 	TP_fast_assign(
 		__entry->hba = hba;
+		__assign_str(dev_name);
 		__assign_str(state);
 		__assign_str(clk);
 		__entry->prev_state = prev_state;
@@ -126,7 +130,7 @@ TRACE_EVENT(ufshcd_clk_scaling,
 	),
 
 	TP_printk("%s: %s %s from %u to %u Hz",
-		dev_name(__entry->hba->dev), __get_str(state), __get_str(clk),
+		__get_str(dev_name), __get_str(state), __get_str(clk),
 		__entry->prev_state, __entry->curr_state)
 );
 
@@ -138,16 +142,18 @@ TRACE_EVENT(ufshcd_auto_bkops_state,
 
 	TP_STRUCT__entry(
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(hba->dev))
 		__string(state, state)
 	),
 
 	TP_fast_assign(
 		__entry->hba = hba;
+		__assign_str(dev_name);
 		__assign_str(state);
 	),
 
 	TP_printk("%s: auto bkops - %s",
-		dev_name(__entry->hba->dev), __get_str(state))
+		__get_str(dev_name), __get_str(state))
 );
 
 DECLARE_EVENT_CLASS(ufshcd_profiling_template,
@@ -158,6 +164,7 @@ DECLARE_EVENT_CLASS(ufshcd_profiling_template,
 
 	TP_STRUCT__entry(
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(hba->dev))
 		__string(profile_info, profile_info)
 		__field(s64, time_us)
 		__field(int, err)
@@ -165,13 +172,14 @@ DECLARE_EVENT_CLASS(ufshcd_profiling_template,
 
 	TP_fast_assign(
 		__entry->hba = hba;
+		__assign_str(dev_name);
 		__assign_str(profile_info);
 		__entry->time_us = time_us;
 		__entry->err = err;
 	),
 
 	TP_printk("%s: %s: took %lld usecs, err %d",
-		dev_name(__entry->hba->dev), __get_str(profile_info),
+		__get_str(dev_name), __get_str(profile_info),
 		__entry->time_us, __entry->err)
 );
 
@@ -200,6 +208,7 @@ DECLARE_EVENT_CLASS(ufshcd_template,
 		__field(s64, usecs)
 		__field(int, err)
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(hba->dev))
 		__field(int, dev_state)
 		__field(int, link_state)
 	),
@@ -208,13 +217,14 @@ DECLARE_EVENT_CLASS(ufshcd_template,
 		__entry->usecs = usecs;
 		__entry->err = err;
 		__entry->hba = hba;
+		__assign_str(dev_name);
 		__entry->dev_state = dev_state;
 		__entry->link_state = link_state;
 	),
 
 	TP_printk(
 		"%s: took %lld usecs, dev_state: %s, link_state: %s, err %d",
-		dev_name(__entry->hba->dev),
+		__get_str(dev_name),
 		__entry->usecs,
 		__print_symbolic(__entry->dev_state, UFS_PWR_MODES),
 		__print_symbolic(__entry->link_state, UFS_LINK_STATES),
@@ -279,6 +289,7 @@ TRACE_EVENT(ufshcd_command,
 	TP_STRUCT__entry(
 		__field(struct scsi_device *, sdev)
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(&sdev->sdev_dev))
 		__field(enum ufs_trace_str_t, str_t)
 		__field(unsigned int, tag)
 		__field(u32, doorbell)
@@ -291,6 +302,7 @@ TRACE_EVENT(ufshcd_command,
 	),
 
 	TP_fast_assign(
+		__assign_str(dev_name);
 		__entry->sdev = sdev;
 		__entry->hba = hba;
 		__entry->str_t = str_t;
@@ -307,7 +319,7 @@ TRACE_EVENT(ufshcd_command,
 	TP_printk(
 		"%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, opcode: 0x%x (%s), group_id: 0x%x, hwq_id: %d",
 		show_ufs_cmd_trace_str(__entry->str_t),
-		dev_name(&__entry->sdev->sdev_dev), __entry->tag,
+		__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
@@ -322,6 +334,7 @@ TRACE_EVENT(ufshcd_uic_command,
 
 	TP_STRUCT__entry(
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(hba->dev))
 		__field(enum ufs_trace_str_t, str_t)
 		__field(u32, cmd)
 		__field(u32, arg1)
@@ -331,6 +344,7 @@ TRACE_EVENT(ufshcd_uic_command,
 
 	TP_fast_assign(
 		__entry->hba = hba;
+		__assign_str(dev_name);
 		__entry->str_t = str_t;
 		__entry->cmd = cmd;
 		__entry->arg1 = arg1;
@@ -340,7 +354,7 @@ TRACE_EVENT(ufshcd_uic_command,
 
 	TP_printk(
 		"%s: %s: cmd: 0x%x, arg1: 0x%x, arg2: 0x%x, arg3: 0x%x",
-		show_ufs_cmd_trace_str(__entry->str_t), dev_name(__entry->hba->dev),
+		show_ufs_cmd_trace_str(__entry->str_t), __get_str(dev_name),
 		__entry->cmd, __entry->arg1, __entry->arg2, __entry->arg3
 	)
 );
@@ -353,6 +367,7 @@ TRACE_EVENT(ufshcd_upiu,
 
 	TP_STRUCT__entry(
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(hba->dev))
 		__field(enum ufs_trace_str_t, str_t)
 		__array(unsigned char, hdr, 12)
 		__array(unsigned char, tsf, 16)
@@ -361,6 +376,7 @@ TRACE_EVENT(ufshcd_upiu,
 
 	TP_fast_assign(
 		__entry->hba = hba;
+		__assign_str(dev_name);
 		__entry->str_t = str_t;
 		memcpy(__entry->hdr, hdr, sizeof(__entry->hdr));
 		memcpy(__entry->tsf, tsf, sizeof(__entry->tsf));
@@ -369,7 +385,7 @@ TRACE_EVENT(ufshcd_upiu,
 
 	TP_printk(
 		"%s: %s: HDR:%s, %s:%s",
-		show_ufs_cmd_trace_str(__entry->str_t), dev_name(__entry->hba->dev),
+		show_ufs_cmd_trace_str(__entry->str_t), __get_str(dev_name),
 		__print_hex(__entry->hdr, sizeof(__entry->hdr)),
 		show_ufs_cmd_trace_tsf(__entry->tsf_t),
 		__print_hex(__entry->tsf, sizeof(__entry->tsf))
@@ -384,16 +400,18 @@ TRACE_EVENT(ufshcd_exception_event,
 
 	TP_STRUCT__entry(
 		__field(struct ufs_hba *, hba)
+		__string(dev_name, dev_name(hba->dev))
 		__field(u16, status)
 	),
 
 	TP_fast_assign(
 		__entry->hba = hba;
+		__assign_str(dev_name);
 		__entry->status = status;
 	),
 
 	TP_printk("%s: status 0x%x",
-		dev_name(__entry->hba->dev), __entry->status
+		__get_str(dev_name), __entry->status
 	)
 );
 
-- Steve

Re: [PATCH v3] ufs: core: add hba parameter to trace events

[Peter Wang (王信友)]

On Tue, 2026-06-30 at 17:49 -0400, Steven Rostedt wrote:
> On Tue, 30 Jun 2026 16:56:12 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > > 
> > >     TP_printk("%s: gating state changed to %s",
> > > -           __get_str(dev_name),
> > > +           dev_name(__entry->hba->dev),
> > 
> > NO YOU CAN NOT DO THIS!!!!
> 
> This is why you should always Cc
> linux-trace-kernel@vger.kernel.org on any
> trace event updates. We look to catch bugs like this.
> 
> The below patch should fix it, and I'll send it as a proper patch
> soon:

Hi Steven,

Thank you for the reminder and for fixing this bug.
This was indeed not thoroughly considered.

However, I am curious: if the HBA is removed, implying that the 
storage would become unusable, might the system encounter an 
I/O hang or shutdown, potentially preventing its detection? 
Perhaps it's a theoretical issue that would not manifest 
in a real-world situation?

Thanks
Peter

Re: [PATCH v3] ufs: core: add hba parameter to trace events

[Steven Rostedt]

On Wed, 1 Jul 2026 06:11:37 +0000
Peter Wang (王信友) <peter.wang@mediatek.com> wrote:
 
> However, I am curious: if the HBA is removed, implying that the 
> storage would become unusable, might the system encounter an 
> I/O hang or shutdown, potentially preventing its detection? 
> Perhaps it's a theoretical issue that would not manifest 
> in a real-world situation?

Note, it doesn't necessarily mean that the device itself was removed. The
issue is that a pointer to an allocated descriptor is saved in the ring buffer.

Maybe once the device is created it will never go way. But what happens if
for some reason the descriptor is freed and reallocated? Now the old
descriptor pointer is still in the ring buffer.

What in the logic guarantees that the pointer will never be freed?

And lets say there is an issue and the hba is freed and you debug this by
dumping the trace buffer via ftrace_dump_on_oops. Now the dump itself may
crash and you don't have a way to debug what happened.

One other point that causes issues here. It makes user space tracing
useless. Try tracing this with "trace-cmd record". These events will not be
able to be parsed.

-- Steve

Re: [PATCH v3] ufs: core: add hba parameter to trace events

[Peter Wang (王信友)]

On Wed, 2026-07-01 at 08:57 -0400, Steven Rostedt wrote:
> On Wed, 1 Jul 2026 06:11:37 +0000
> Peter Wang (王信友) <peter.wang@mediatek.com> wrote:
> 
> > However, I am curious: if the HBA is removed, implying that the
> > storage would become unusable, might the system encounter an
> > I/O hang or shutdown, potentially preventing its detection?
> > Perhaps it's a theoretical issue that would not manifest
> > in a real-world situation?
> 
> Note, it doesn't necessarily mean that the device itself was removed.
> The
> issue is that a pointer to an allocated descriptor is saved in the
> ring buffer.
> 
> Maybe once the device is created it will never go way. But what
> happens if
> for some reason the descriptor is freed and reallocated? Now the old
> descriptor pointer is still in the ring buffer.
> 
> What in the logic guarantees that the pointer will never be freed?
> 
> And lets say there is an issue and the hba is freed and you debug
> this by
> dumping the trace buffer via ftrace_dump_on_oops. Now the dump itself
> may
> crash and you don't have a way to debug what happened.
> 
> One other point that causes issues here. It makes user space tracing
> useless. Try tracing this with "trace-cmd record". These events will
> not be
> able to be parsed.
> 
> -- Steve
> 

Hi Steven,

Thank you for the detailed explanation. I have no further questions.

Thanks
Peter