[BUG] str not used in cdns3 trace events

[BUG] str not used in cdns3 trace events

[Steven Rostedt]

Hi,

I'm doing updates to add __vstring (dynamically created strings in trace
events) and came across the trace events in:

  drivers/usb/cdns3/trace.h

Where there is several cases of this:

+DECLARE_EVENT_CLASS(cdns3_log_usb_irq,
+       TP_PROTO(struct cdns3_device *priv_dev, u32 usb_ists),
+       TP_ARGS(priv_dev, usb_ists),
+       TP_STRUCT__entry(
+               __field(enum usb_device_speed, speed)
+               __field(u32, usb_ists)
+               __dynamic_array(char, str, CDNS3_MSG_MAX)
+       ),
+       TP_fast_assign(
+               __entry->speed = cdns3_get_speed(priv_dev);
+               __entry->usb_ists = usb_ists;
+       ),
+       TP_printk("%s", cdns3_decode_usb_irq(__get_str(str), __entry->speed,
+                                            __entry->usb_ists))
+);

I see you create a dynamic array for "str" and even reference it in the
print with __get_str(str). But it is never assigned in TP_fast_assign().

This looks to be a bug to me. Can you explain this please?

-- Steve

Re: [BUG] str not used in cdns3 trace events

[Steven Rostedt]

On Fri, 1 Jul 2022 12:13:53 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Hi,
> 
> I'm doing updates to add __vstring (dynamically created strings in trace
> events) and came across the trace events in:
> 
>   drivers/usb/cdns3/trace.h
> 
> Where there is several cases of this:
> 
> +DECLARE_EVENT_CLASS(cdns3_log_usb_irq,
> +       TP_PROTO(struct cdns3_device *priv_dev, u32 usb_ists),
> +       TP_ARGS(priv_dev, usb_ists),
> +       TP_STRUCT__entry(
> +               __field(enum usb_device_speed, speed)
> +               __field(u32, usb_ists)
> +               __dynamic_array(char, str, CDNS3_MSG_MAX)
> +       ),
> +       TP_fast_assign(
> +               __entry->speed = cdns3_get_speed(priv_dev);
> +               __entry->usb_ists = usb_ists;
> +       ),
> +       TP_printk("%s", cdns3_decode_usb_irq(__get_str(str), __entry->speed,
> +                                            __entry->usb_ists))

I see what you are doing now.

You are reserving space in the buffer to use as something to process.

There's a better way to do this. There's a trace_seq handle called "p" that
is reserved for TP_printk()

You can look at the event scsi_dispatch_cmd_start in
include/trace/events/scsi.h that uses it.

  const char *scsi_trace_parse_cdb(struct trace_seq*, unsigned char*, int);
  #define __parse_cdb(cdb, len) scsi_trace_parse_cdb(p, cdb, len)

  __parse_cdb(__get_dynamic_array(cmnd), __entry->cmd_len),

It's better than wasting space on the ring buffer just to use as a buffer
for the event.

-- Steve


> +);
> 
> I see you create a dynamic array for "str" and even reference it in the
> print with __get_str(str). But it is never assigned in TP_fast_assign().
> 
> This looks to be a bug to me. Can you explain this please?
> 
> -- Steve