linux-kernel - Re: [PATCH 2/2] usb: core: Add tracepoints for device allocation and state changes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKzKK0pzfSFJ15esnGB9gY_HMrgubZ1QtSkLOUo2FvsNGtCi6g@mail.gmail.com>
Date: Tue, 14 Oct 2025 08:05:25 +0800
From: Kuen-Han Tsai <khtsai@...gle.com>
To: Alan Stern <stern@...land.harvard.edu>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Mathias Nyman <mathias.nyman@...ux.intel.com>, linux-usb@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] usb: core: Add tracepoints for device allocation and
 state changes

Hi Alan,

On Mon, Oct 13, 2025 at 9:20 PM Alan Stern <stern@...land.harvard.edu> wrote:
>
> On Mon, Oct 13, 2025 at 10:01:23AM +0800, Kuen-Han Tsai wrote:
> > Introduce new tracepoints to the USB core to improve debuggability of
> > USB device lifecycle events.
> >
> > The following tracepoints are added:
> >
> > - usb_alloc_dev: Triggered when a new USB device structure is allocated,
> > providing insights into early device setup.
> > - usb_set_device_state: Triggered when the USB device state changes,
> > allowing observation of the device's state transitions.
> >
> > These tracepoints capture detailed information about the USB device,
> > including its name, speed, state, bus current value, and authorized
> > flag. This will aid developers in diagnosing issues related to device
> > enumeration within the USB subsystem.
> >
> > Examples:
> >  usb_alloc_dev: usb 1-1 speed 0 state 1 0mA [authorized]
> >  usb_set_device_state: usb 1-1 speed 0 state 2 0mA [authorized]
> >  usb_set_device_state: usb 1-1 speed 2 state 5 500mA [authorized]
> >  usb_set_device_state: usb 1-1 speed 2 state 5 500mA [authorized]
> >  usb_set_device_state: usb 1-1 speed 2 state 6 500mA [authorized]
> >  usb_set_device_state: usb 1-1 speed 2 state 7 500mA [authorized]
> >  usb_set_device_state: usb 1-1 speed 2 state 8 500mA [authorized]
> >  usb_set_device_state: usb 1-1 speed 2 state 0 500mA [authorized]
> >
> > Signed-off-by: Kuen-Han Tsai <khtsai@...gle.com>
> > ---
>
> > diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
> > index 766000b4939ef937a04848aa9cc45d8bb8860fe5..11647942ff3ae6c688dac043218f7d886a3e2f88 100644
> > --- a/drivers/usb/core/Makefile
> > +++ b/drivers/usb/core/Makefile
> > @@ -3,10 +3,14 @@
> >  # Makefile for USB Core files and filesystem
> >  #
> >
> > +# define_trace.h needs to know how to find our header
> > +CFLAGS_trace.o                  := -I$(src)
> > +
> >  usbcore-y := usb.o hub.o hcd.o urb.o message.o driver.o
> >  usbcore-y += config.o file.o buffer.o sysfs.o endpoint.o
> >  usbcore-y += devio.o notify.o generic.o quirks.o devices.o
> >  usbcore-y += phy.o port.o
> > +usbcore-y += trace.o
>
> This looks a little odd.  Why not put trace.o at the end of the
> preceding line?
>

Thanks for the review. I will move trace.o to the preceding line.

> > diff --git a/drivers/usb/core/trace.h b/drivers/usb/core/trace.h
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..db6edf570640e7af0598ccf2c7bd71b187605a42
> > --- /dev/null
> > +++ b/drivers/usb/core/trace.h
> > @@ -0,0 +1,61 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2025 Google LLC
> > + */
> > +#undef TRACE_SYSTEM
> > +#define TRACE_SYSTEM usbcore
> > +
> > +#if !defined(_USB_CORE_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
> > +#define _USB_CORE_TRACE_H
> > +
> > +#include <linux/types.h>
> > +#include <linux/tracepoint.h>
> > +#include <linux/usb.h>
> > +
> > +DECLARE_EVENT_CLASS(usb_core_log_usb_device,
> > +     TP_PROTO(struct usb_device *udev),
> > +     TP_ARGS(udev),
> > +     TP_STRUCT__entry(
> > +             __string(name, dev_name(&udev->dev))
> > +             __field(enum usb_device_speed, speed)
> > +             __field(enum usb_device_state, state)
> > +             __field(unsigned short, bus_mA)
> > +             __field(unsigned, authorized)
> > +     ),
> > +     TP_fast_assign(
> > +             __assign_str(name);
> > +             __entry->speed = udev->speed;
> > +             __entry->state = udev->state;
> > +             __entry->bus_mA = udev->bus_mA;
> > +             __entry->authorized = udev->authorized;
> > +     ),
> > +     TP_printk("usb %s speed %d state %d %dmA [%s]",
> > +             __get_str(name),
> > +             __entry->speed,
> > +             __entry->state,
>
> Suggestion: Rather than printing the meaningless numerical value of
> __entry->state, print the string value returned by
> usb_state_string(__entry->state).

I kept it consistent with the udc_log_gadget tracepoint, which also
uses the numerical value for the USB state.

If we change the state to a string, should we convert the speed field
to a string using usb_speed_string()?

I lean toward keeping both as numerical values, but I am happy to
switch both to strings if you prefer. Please let me know what you
think is best.

Regards,
Kuen-Han

>
> Alan Stern