[PATCH v2] usb: gadget: Address review comments and include missing files

[PATCH v2] usb: gadget: Address review comments and include missing files

[Olivier Tuchon]

This patch addresses feedback on v1 from greg k-h and Alan Stern. It
also includes files that were missing from the initial submission.

The following changes have been made:
  - correct initial coding style issue (spaces for tabs)
  - add missing file required for the feature to build and run
  - improve FIFO full handling by truncating event data instead of
    dropping the event entirely
  - avoid capturing redundant data for IN submissions

Signed-off-by: Olivier Tuchon <tcn@google.com>
---
 drivers/usb/Kconfig                     |  2 +
 drivers/usb/Makefile                    |  1 +
 drivers/usb/gadget/udc/core.c           | 78 +++++++++++++++++++++++++
 drivers/usb/gadget_mon/Kconfig          | 20 +++----
 drivers/usb/gadget_mon/gadgetmon_main.c | 41 ++++++++-----
 include/linux/usb/gadget.h              | 23 ++++++++
 6 files changed, 141 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index abf8c6cdea9e..b615035cc7c6 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -107,6 +107,8 @@ source "drivers/usb/core/Kconfig"

 source "drivers/usb/mon/Kconfig"

+source "drivers/usb/gadget_mon/Kconfig"
+
 source "drivers/usb/host/Kconfig"

 source "drivers/usb/renesas_usbhs/Kconfig"
diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index 949eca0adebe..2539151a5366 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -62,6 +62,7 @@ obj-$(CONFIG_USB_MUSB_HDRC) += musb/
 obj-$(CONFIG_USB_CHIPIDEA) += chipidea/
 obj-$(CONFIG_USB_RENESAS_USBHS) += renesas_usbhs/
 obj-$(CONFIG_USB_GADGET) += gadget/
+obj-$(CONFIG_USB_GADGET_MON) += gadget_mon/

 obj-$(CONFIG_USBIP_CORE) += usbip/

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index d709e24c1fd4..a7d22ef3ad74 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -28,6 +28,16 @@ static DEFINE_IDA(gadget_id_numbers);

 static const struct bus_type gadget_bus_type;

+/* ------------------------------------------------------------------------- */
+
+/* USB Gadget monitoring */
+#if IS_ENABLED(CONFIG_USB_GADGET_MON)
+static DEFINE_MUTEX(gadget_mon_lock);
+static const struct usb_gadget_mon_operations *gadget_mon_ops;
+#endif
+
+/* ------------------------------------------------------------------------- */
+
 /**
  * struct usb_udc - describes one usb device controller
  * @driver: the gadget driver pointer. For use by the class code
@@ -302,6 +312,18 @@ int usb_ep_queue(struct usb_ep *ep,
 out:
  trace_usb_ep_queue(ep, req, ret);

+#if IS_ENABLED(CONFIG_USB_GADGET_MON)
+ if (unlikely(rcu_access_pointer(gadget_mon_ops))) {
+ const struct usb_gadget_mon_operations *ops;
+
+ rcu_read_lock();
+ ops = rcu_dereference(gadget_mon_ops);
+ if (ops)
+ ops->request_queue(ep, req, ret);
+ rcu_read_unlock();
+ }
+#endif /* CONFIG_USB_GADGET_MON */
+
  return ret;
 }
 EXPORT_SYMBOL_GPL(usb_ep_queue);
@@ -996,6 +1018,18 @@ void usb_gadget_giveback_request(struct usb_ep *ep,

  trace_usb_gadget_giveback_request(ep, req, 0);

+#if IS_ENABLED(CONFIG_USB_GADGET_MON)
+ if (unlikely(rcu_access_pointer(gadget_mon_ops))) {
+ const struct usb_gadget_mon_operations *ops;
+
+ rcu_read_lock();
+ ops = rcu_dereference(gadget_mon_ops);
+ if (ops)
+ ops->request_giveback(ep, req);
+ rcu_read_unlock();
+ }
+#endif /* CONFIG_USB_GADGET_MON */
+
  req->complete(ep, req);
 }
 EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
@@ -1925,6 +1959,50 @@ static void __exit usb_udc_exit(void)
 }
 module_exit(usb_udc_exit);

+/* ------------------------------------------------------------------------- */
+
+/* USB Gadget monitoring */
+#if IS_ENABLED(CONFIG_USB_GADGET_MON)
+/*
+ * register_gadget_monitor - Register a monitoring module.
+ * @ops: the monitoring operations to register
+ *
+ * Allows a monitoring module to register its callbacks. Returns -EBUSY
+ * if a monitor is already registered
+ */
+int register_gadget_monitor(const struct usb_gadget_mon_operations *ops)
+{
+ int ret = 0;
+
+ mutex_lock(&gadget_mon_lock);
+ if (gadget_mon_ops)
+ ret = -EBUSY;
+ else
+ rcu_assign_pointer(gadget_mon_ops, ops);
+ mutex_unlock(&gadget_mon_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(register_gadget_monitor);
+
+/*
+ * unregister_gadget_monitor - Unregister a monitoring module.
+ * @ops: the monitoring operations to unregister
+ *
+ * A module must call this to unregister its callbacks before exiting
+ */
+void unregister_gadget_monitor(const struct usb_gadget_mon_operations *ops)
+{
+ mutex_lock(&gadget_mon_lock);
+ if (gadget_mon_ops == ops)
+ rcu_assign_pointer(gadget_mon_ops, NULL);
+ mutex_unlock(&gadget_mon_lock);
+
+ synchronize_rcu();
+}
+EXPORT_SYMBOL_GPL(unregister_gadget_monitor);
+#endif
+
 MODULE_DESCRIPTION("UDC Framework");
 MODULE_AUTHOR("Felipe Balbi <balbi@ti.com>");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/usb/gadget_mon/Kconfig b/drivers/usb/gadget_mon/Kconfig
index 113423a2a96f..bfda5c006909 100644
--- a/drivers/usb/gadget_mon/Kconfig
+++ b/drivers/usb/gadget_mon/Kconfig
@@ -4,18 +4,18 @@
 #

 config USB_GADGET_MON
-  tristate "Gadget-side USB monitor"
-  depends on USB_GADGET
-  help
-    This option enables a low-level monitor for the USB gadget
-    subsystem, similar to what usbmon provides for the host side.
+ tristate "Gadget-side USB monitor"
+ depends on USB_GADGET
+ help
+   This option enables a low-level monitor for the USB gadget
+   subsystem, similar to what usbmon provides for the host side.

-    It creates a character device (/dev/gadgetmon0) that outputs a
-    stream of all USB request submissions and completions, allowing
-    for detailed debugging and performance analysis of gadget drivers.
+   It creates a character device (/dev/gadgetmon0) that outputs a
+   stream of all USB request submissions and completions, allowing
+   for detailed debugging and performance analysis of gadget drivers.

-    This is primarily a tool for developers. If you are not developing
-    or debugging a USB gadget function driver, say N.
+   This is primarily a tool for developers. If you are not developing
+   or debugging a USB gadget function driver, say N.

 config GADGETMON_BUFFER_SIZE_MB
  int "Buffer size for gadget monitor (in MiB)"
diff --git a/drivers/usb/gadget_mon/gadgetmon_main.c
b/drivers/usb/gadget_mon/gadgetmon_main.c
index 9017f91808ae..4597f10abc5e 100644
--- a/drivers/usb/gadget_mon/gadgetmon_main.c
+++ b/drivers/usb/gadget_mon/gadgetmon_main.c
@@ -50,7 +50,8 @@ static void gadgetmon_event(enum
gadgetmon_event_type event_type,
  unsigned long flags;
  struct gadgetmon_hdr hdr;
  u32 payload_len;
- u32 total_len;
+ u32 available_space;
+ u32 payload_to_copy;
  struct timespec64 ts;

  if (!req || !ep)
@@ -78,28 +79,32 @@ static void gadgetmon_event(enum
gadgetmon_event_type event_type,
  if (payload_len > GADGETMON_DATA_MAX)
  payload_len = GADGETMON_DATA_MAX;
  /*
- * optimization: for an OUT submission (host-to-device), the data
- * has not yet arrived from the host. The buffer is an empty
- * placeholder, so its content is not captured to save space.
+ * optimization: for all submission, the buffer data is not yet
+ * relevant. Capture no payload to save significant buffer space.
  */
- if (event_type == GADGETMON_EVENT_SUBMIT && hdr.dir == USB_DIR_OUT)
+ if (event_type == GADGETMON_EVENT_SUBMIT)
  payload_len = 0;

  hdr.data_len = payload_len;
- total_len = sizeof(hdr) + payload_len;

  /* lock and queue the event into the FIFO */
  spin_lock_irqsave(&mon_lock, flags);

- if (kfifo_avail(&mon_fifo) < total_len) {
- /* not enough space, drop the event silently */
+ available_space = kfifo_avail(&mon_fifo);
+
+ /* if the header itself doesn't fit, we must drop the even */
+ if (available_space < sizeof(hdr)) {
  spin_unlock_irqrestore(&mon_lock, flags);
  return;
  }

+ payload_to_copy = min(payload_len, available_space - sizeof(hdr));
+ if (payload_to_copy != payload_len)
+ hdr.data_len = payload_to_copy;
+
  kfifo_in(&mon_fifo, &hdr, sizeof(hdr));
- if (payload_len > 0)
- kfifo_in(&mon_fifo, req->buf, payload_len);
+ if (payload_to_copy > 0)
+ kfifo_in(&mon_fifo, req->buf, payload_to_copy);

  spin_unlock_irqrestore(&mon_lock, flags);

@@ -251,13 +256,20 @@ static int __init gadgetmon_init(void)
  goto err_del_cdev;
  }

- /* Atomically publish our monitoring functions to the UDC core */
- rcu_assign_pointer(gadget_mon_ops, &gadget_mon_ops_impl);
+ /* register our monitoring functions with the UDC core */
+ ret = register_gadget_monitor(&gadget_mon_ops_impl);
+ if (ret) {
+ pr_err("gadgetmon: Failed to register monitor, is another one loaded? (%d)\n",
+        ret);
+ goto err_destroy_device;
+ }

  pr_info("gadgetmon: Gadget Monitoring driver loaded\n");

  return 0;

+err_destroy_device:
+ device_destroy(mon_class, mon_dev_t);
 err_del_cdev:
  cdev_del(&mon_cdev);
 err_destroy_class:
@@ -278,8 +290,9 @@ static int __init gadgetmon_init(void)
  */
 static void __exit gadgetmon_exit(void)
 {
- rcu_assign_pointer(gadget_mon_ops, NULL);
- synchronize_rcu();
+ pr_info("Gadget Monitoring driver unloading\n");
+
+ unregister_gadget_monitor(&gadget_mon_ops_impl);

  device_destroy(mon_class, mon_dev_t);
  cdev_del(&mon_cdev);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index df33333650a0..a263b8ea968f 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -971,4 +971,27 @@ extern void usb_ep_autoconfig_release(struct usb_ep *);

 extern void usb_ep_autoconfig_reset(struct usb_gadget *);

+/*-------------------------------------------------------------------------*/
+
+/* USB Gadget monitoring */
+#if IS_ENABLED(CONFIG_USB_GADGET_MON)
+/**
+ * struct usb_gadget_mon_operations - operations for gadget monitoring
+ * @request_queue: Called when a gadget driver queues a request.
+ * @request_giveback: Called just before a request is given back.
+ */
+struct usb_gadget_mon_operations {
+ void (*request_queue)(struct usb_ep *ep, const struct usb_request *req,
+ int status);
+ void (*request_giveback)(struct usb_ep *ep,
+ const struct usb_request *req);
+};
+
+int register_gadget_monitor(const struct usb_gadget_mon_operations *ops);
+void unregister_gadget_monitor(const struct usb_gadget_mon_operations *ops);
+#else
+static inline int register_gadget_monitor(const void *ops) { return 0; }
+static inline void unregister_gadget_monitor(const void *ops) { }
+#endif /* CONFIG_USB_GADGET_MON */
+
 #endif /* __LINUX_USB_GADGET_H */
-- 
2.50.1.487.gc89ff58d15-goog

Changes in v2:
  - Add optimization to skip capturing IN submissions
  - Truncate event payload on FIFO full instead of dropping (as
    suggested)
  - Added forgotten files (include/linux/usb/gadget.h,
    drivers/usb/core/Kconfig, drivers/usb/Makefile,
    drivers/usb/gadget/udc/core.c)
  - Fixed initial indentation issues

Re: [PATCH v2] usb: gadget: Address review comments and include missing files

[Greg KH]

On Fri, Jul 25, 2025 at 11:09:26PM +0200, Olivier Tuchon wrote:
> This patch addresses feedback on v1 from greg k-h and Alan Stern. It
> also includes files that were missing from the initial submission.
> 
> The following changes have been made:
>   - correct initial coding style issue (spaces for tabs)
>   - add missing file required for the feature to build and run
>   - improve FIFO full handling by truncating event data instead of
>     dropping the event entirely
>   - avoid capturing redundant data for IN submissions

This is not a proper way to write a subject line or comments at all.

Look at patches on the list and in the tree, nothing starts with
"address review comments", right?

Please go back and start again...


> 
> Signed-off-by: Olivier Tuchon <tcn@google.com>
> ---
>  drivers/usb/Kconfig                     |  2 +
>  drivers/usb/Makefile                    |  1 +
>  drivers/usb/gadget/udc/core.c           | 78 +++++++++++++++++++++++++
>  drivers/usb/gadget_mon/Kconfig          | 20 +++----
>  drivers/usb/gadget_mon/gadgetmon_main.c | 41 ++++++++-----
>  include/linux/usb/gadget.h              | 23 ++++++++
>  6 files changed, 141 insertions(+), 24 deletions(-)

Please read the in-kernel documentation about how to submit a kernel
patch, it should explain all of this, right?  If not, please work with
someone in your group that knows how to create kernel patches, don't
require the community to do that kind of basic review/training for you.

thanks,

greg k-h