[PATCH] usbmon: expose the usbmon structures and constants as an UAPI header.

[PATCH] usbmon: expose the usbmon structures and constants as an UAPI header.

[Diego Elio Pettenò]

Previously any application wanting to implement the usbmon binary
interfaces needed to re-declare the structures and constants, leading to
structure duplication and confusion over whether these structures fall into
the system call exception or not.

Also update Paolo's address to match `net/core/dst_cache.c` as the previous
one bounces.

Cc: linux-usb@vger.kernel.org
Cc: Pete Zaitcev <zaitcev@redhat.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Kris Katterjohn <katterjohn@gmail.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Signed-off-by: Diego Elio Pettenò <flameeyes@flameeyes.com>
---
 Documentation/usb/usbmon.rst |  12 ++---
 drivers/usb/mon/mon_bin.c    |  94 ++------------------------------
 include/uapi/linux/usb/mon.h | 102 +++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+), 97 deletions(-)
 create mode 100644 include/uapi/linux/usb/mon.h

diff --git a/Documentation/usb/usbmon.rst b/Documentation/usb/usbmon.rst
index b0bd51080799..cf98ea553ba1 100644
--- a/Documentation/usb/usbmon.rst
+++ b/Documentation/usb/usbmon.rst
@@ -211,11 +211,13 @@ Bulk wrapper to a storage device at address 5::
 Raw binary format and API
 =========================
 
-The overall architecture of the API is about the same as the one above,
-only the events are delivered in binary format. Each event is sent in
-the following structure (its name is made up, so that we can refer to it)::
+The overall architecture of the API is about the same as the one above, only the
+events are delivered in binary format. The structures and constants are defined
+in linux/usb/mon.h.
 
-  struct usbmon_packet {
+Each event is sent in the following structure::
+
+  struct mon_bin_hdr {
 	u64 id;			/*  0: URB ID - from submission to callback */
 	unsigned char type;	/*  8: Same as text; extensible. */
 	unsigned char xfer_type; /*    ISO (0), Intr, Control, Bulk (3) */
@@ -346,8 +348,6 @@ be polled with select(2) and poll(2). But lseek(2) does not work.
 
 * Memory-mapped access of the kernel buffer for the binary API
 
-The basic idea is simple:
-
 To prepare, map the buffer by getting the current size, then using mmap(2).
 Then, execute a loop similar to the one written in pseudo-code below::
 
diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index f48a23adbc35..e914fd8d039e 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -4,8 +4,8 @@
  *
  * This is a binary format reader.
  *
- * Copyright (C) 2006 Paolo Abeni (paolo.abeni@email.it)
- * Copyright (C) 2006,2007 Pete Zaitcev (zaitcev@redhat.com)
+ * Copyright (C) 2006 Paolo Abeni <pabeni@redhat.com>
+ * Copyright (C) 2006,2007 Pete Zaitcev <zaitcev@redhat.com>
  */
 
 #include <linux/kernel.h>
@@ -23,34 +23,10 @@
 #include <linux/time64.h>
 
 #include <linux/uaccess.h>
+#include <linux/usb/mon.h>
 
 #include "usb_mon.h"
 
-/*
- * Defined by USB 2.0 clause 9.3, table 9.2.
- */
-#define SETUP_LEN  8
-
-/* ioctl macros */
-#define MON_IOC_MAGIC 0x92
-
-#define MON_IOCQ_URB_LEN _IO(MON_IOC_MAGIC, 1)
-/* #2 used to be MON_IOCX_URB, removed before it got into Linus tree */
-#define MON_IOCG_STATS _IOR(MON_IOC_MAGIC, 3, struct mon_bin_stats)
-#define MON_IOCT_RING_SIZE _IO(MON_IOC_MAGIC, 4)
-#define MON_IOCQ_RING_SIZE _IO(MON_IOC_MAGIC, 5)
-#define MON_IOCX_GET   _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get)
-#define MON_IOCX_MFETCH _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch)
-#define MON_IOCH_MFLUSH _IO(MON_IOC_MAGIC, 8)
-/* #9 was MON_IOCT_SETAPI */
-#define MON_IOCX_GETX   _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get)
-
-#ifdef CONFIG_COMPAT
-#define MON_IOCX_GET32 _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get32)
-#define MON_IOCX_MFETCH32 _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch32)
-#define MON_IOCX_GETX32   _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get32)
-#endif
-
 /*
  * Some architectures have enormous basic pages (16KB for ia64, 64KB for ppc).
  * But it's all right. Just use a simple way to make sure the chunk is never
@@ -81,38 +57,6 @@
 #define BUFF_DFL   CHUNK_ALIGN(300*1024)
 #define BUFF_MIN     CHUNK_ALIGN(8*1024)
 
-/*
- * The per-event API header (2 per URB).
- *
- * This structure is seen in userland as defined by the documentation.
- */
-struct mon_bin_hdr {
-	u64 id;			/* URB ID - from submission to callback */
-	unsigned char type;	/* Same as in text API; extensible. */
-	unsigned char xfer_type;	/* ISO, Intr, Control, Bulk */
-	unsigned char epnum;	/* Endpoint number and transfer direction */
-	unsigned char devnum;	/* Device address */
-	unsigned short busnum;	/* Bus number */
-	char flag_setup;
-	char flag_data;
-	s64 ts_sec;		/* ktime_get_real_ts64 */
-	s32 ts_usec;		/* ktime_get_real_ts64 */
-	int status;
-	unsigned int len_urb;	/* Length of data (submitted or actual) */
-	unsigned int len_cap;	/* Delivered length */
-	union {
-		unsigned char setup[SETUP_LEN];	/* Only for Control S-type */
-		struct iso_rec {
-			int error_count;
-			int numdesc;
-		} iso;
-	} s;
-	int interval;
-	int start_frame;
-	unsigned int xfer_flags;
-	unsigned int ndesc;	/* Actual number of ISO descriptors */
-};
-
 /*
  * ISO vector, packed into the head of data stream.
  * This has to take 16 bytes to make sure that the end of buffer
@@ -125,38 +69,6 @@ struct mon_bin_isodesc {
 	u32 _pad;
 };
 
-/* per file statistic */
-struct mon_bin_stats {
-	u32 queued;
-	u32 dropped;
-};
-
-struct mon_bin_get {
-	struct mon_bin_hdr __user *hdr;	/* Can be 48 bytes or 64. */
-	void __user *data;
-	size_t alloc;		/* Length of data (can be zero) */
-};
-
-struct mon_bin_mfetch {
-	u32 __user *offvec;	/* Vector of events fetched */
-	u32 nfetch;		/* Number of events to fetch (out: fetched) */
-	u32 nflush;		/* Number of events to flush */
-};
-
-#ifdef CONFIG_COMPAT
-struct mon_bin_get32 {
-	u32 hdr32;
-	u32 data32;
-	u32 alloc32;
-};
-
-struct mon_bin_mfetch32 {
-        u32 offvec32;
-        u32 nfetch32;
-        u32 nflush32;
-};
-#endif
-
 /* Having these two values same prevents wrapping of the mon_bin_hdr */
 #define PKT_ALIGN   64
 #define PKT_SIZE    64
diff --git a/include/uapi/linux/usb/mon.h b/include/uapi/linux/usb/mon.h
new file mode 100644
index 000000000000..265e0169e2ef
--- /dev/null
+++ b/include/uapi/linux/usb/mon.h
@@ -0,0 +1,102 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * USB Monitoring (usbmon) definitions
+ */
+
+#ifndef __UAPI_LINUX_USB_MON_H
+#define __UAPI_LINUX_USB_MON_H
+
+/* ioctl macros */
+#define MON_IOC_MAGIC 0x92
+
+#define MON_IOCQ_URB_LEN _IO(MON_IOC_MAGIC, 1)
+/* #2 used to be MON_IOCX_URB, removed before it got into Linus tree */
+#define MON_IOCG_STATS _IOR(MON_IOC_MAGIC, 3, struct mon_bin_stats)
+#define MON_IOCT_RING_SIZE _IO(MON_IOC_MAGIC, 4)
+#define MON_IOCQ_RING_SIZE _IO(MON_IOC_MAGIC, 5)
+#define MON_IOCX_GET   _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get)
+#define MON_IOCX_MFETCH _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch)
+#define MON_IOCH_MFLUSH _IO(MON_IOC_MAGIC, 8)
+/* #9 was MON_IOCT_SETAPI */
+#define MON_IOCX_GETX   _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get)
+
+#ifdef CONFIG_COMPAT
+#define MON_IOCX_GET32 _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get32)
+#define MON_IOCX_MFETCH32 _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch32)
+#define MON_IOCX_GETX32   _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get32)
+#endif
+
+/* ioctl structures */
+
+/* per file statistic */
+struct mon_bin_stats {
+	u32 queued;
+	u32 dropped;
+};
+
+struct mon_bin_get {
+	struct mon_bin_hdr __user *hdr;	/* Can be 48 bytes or 64. */
+	void __user *data;
+	size_t alloc;		/* Length of data (can be zero) */
+};
+
+struct mon_bin_mfetch {
+	u32 __user *offvec;	/* Vector of events fetched */
+	u32 nfetch;		/* Number of events to fetch (out: fetched) */
+	u32 nflush;		/* Number of events to flush */
+};
+
+#ifdef CONFIG_COMPAT
+struct mon_bin_get32 {
+	u32 hdr32;
+	u32 data32;
+	u32 alloc32;
+};
+
+struct mon_bin_mfetch32 {
+	u32 offvec32;
+	u32 nfetch32;
+	u32 nflush32;
+};
+#endif
+
+/* Data format */
+
+/*
+ * Defined by USB 2.0 clause 9.3, table 9.2.
+ */
+#define SETUP_LEN  8
+
+/*
+ * The per-event API header (2 per URB).
+ *
+ * This structure is seen in userland as defined by the documentation.
+ */
+struct mon_bin_hdr {
+	u64 id;			/* URB ID - from submission to callback */
+	unsigned char type;	/* Same as in text API; extensible. */
+	unsigned char xfer_type;	/* ISO, Intr, Control, Bulk */
+	unsigned char epnum;	/* Endpoint number and transfer direction */
+	unsigned char devnum;	/* Device address */
+	unsigned short busnum;	/* Bus number */
+	char flag_setup;
+	char flag_data;
+	s64 ts_sec;		/* ktime_get_real_ts64 */
+	s32 ts_usec;		/* ktime_get_real_ts64 */
+	int status;
+	unsigned int len_urb;	/* Length of data (submitted or actual) */
+	unsigned int len_cap;	/* Delivered length */
+	union {
+		unsigned char setup[SETUP_LEN];	/* Only for Control S-type */
+		struct iso_rec {
+			int error_count;
+			int numdesc;
+		} iso;
+	} s;
+	int interval;
+	int start_frame;
+	unsigned int xfer_flags;
+	unsigned int ndesc;	/* Actual number of ISO descriptors */
+};
+
+#endif /* __UAPI_LINUX_USB_MON_H */
-- 
2.27.0

Re: [PATCH] usbmon: expose the usbmon structures and constants as an UAPI header.

[Pete Zaitcev]

On Sun,  5 Jul 2020 16:02:25 +0100
Diego Elio Pettenò <flameeyes@flameeyes.com> wrote:

> Previously any application wanting to implement the usbmon binary
> interfaces needed to re-declare the structures and constants, leading to
> structure duplication and confusion over whether these structures fall into
> the system call exception or not.

I am not sure I follow the logic in the confusion claim above. Since the
applications define their own headers, they do not use the kernel source
at all. Therefore, there can be no question about any exceptions, and no
confusion. But perhaps I'm missing a turn here. Feel free to educate me.

As for the "duplication", I do not see it as harmful in any way.
And they do not re-declare, they declare. You're trying to create a
centralized language definition that did not exist previously.

> +++ b/Documentation/usb/usbmon.rst
> -  struct usbmon_packet {
> +  struct mon_bin_hdr {
>  	u64 id;			/*  0: URB ID - from submission to callback */

It was named that specifically in order to underscore that it's not
the actual code.

> +++ b/include/uapi/linux/usb/mon.h
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */

Yeah, that. You created the confusion first, and then resolved it
with licensing amendments.

> +/* ioctl macros */
> +#define MON_IOC_MAGIC 0x92
> +
> +#define MON_IOCQ_URB_LEN _IO(MON_IOC_MAGIC, 1)

Okay. Our documentation refers to _IO(), which is a system macro,
so we're not entirely self-containing. I can see an opening for
some sophistry here. 

Fortunately, include/uapi/asm-generic/ioctl.h already includes
that "GPL-2.0 WITH Linux-syscall-note" thing, so I think we're
clear in usbmon.

-- Pete

Re: [PATCH] usbmon: expose the usbmon structures and constants as an UAPI header.

[Diego Elio Pettenò]

On Mon, Jul 6, 2020, at 04:51, Pete Zaitcev wrote:
> I am not sure I follow the logic in the confusion claim above. Since the
> applications define their own headers, they do not use the kernel source
> at all. Therefore, there can be no question about any exceptions, and no
> confusion. But perhaps I'm missing a turn here. Feel free to educate me.

Most applications rely on one or another kernel header — this has been the norm for Linux kernel interface users for many years. The UAPI headers are installed on systems and wrapped explicitly for this reason.

> As for the "duplication", I do not see it as harmful in any way.
> And they do not re-declare, they declare. You're trying to create a
> centralized language definition that did not exist previously.

It is harmful, because these structures *need* to be exactly synchronised or the calls will fail. Or the data will be garbage. Not exposing them on UAPI just doesn't make it as obvious, but it is an issue needing to be addressed.

Furthermore there's the problem of whether having to pick up the structures that are defined in a piece of source code that is purported to not be covered by the syscall exception — that would cause require making the userspace implementations of usbmon a derived work of the kernel, which in turn would make them GPL-2.

> > +++ b/include/uapi/linux/usb/mon.h
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> 
> Yeah, that. You created the confusion first, and then resolved it
> with licensing amendments.

I didn't create the confusion. I stumbled across the confusion.

Either the structure definition is not covered by the syscall exception, or it isn't. If it is covered, then we're all good: anyone can include the header, or copy it, or copy part of it, or redefine the structures in whichever language they prefer, and it's not a derived work of the kernel.

If it is, then we have a bit of an issue, as libpcap is not considering itself a derived work of Linux and contains a copy of the same structures. So either we say that Paolo had the ability to release it under both licenses, or we're going to give a headache to libpcap. If we accept that Paolo released these structures under both licenses, then the syscall exception can be applied, if nothing else by sleight-of-hand by taking the MIT licensed version and restricting it.

> Okay. Our documentation refers to _IO(), which is a system macro,
> so we're not entirely self-containing. I can see an opening for
> some sophistry here. 
> 
> Fortunately, include/uapi/asm-generic/ioctl.h already includes
> that "GPL-2.0 WITH Linux-syscall-note" thing, so I think we're
> clear in usbmon.

You _also_ need the structure definition, or at least its size, for the _IO macro to be usable.
Which is why it's important that syscall-level interfaces are made available in UAPI headers with exceptions.

Now that does mean that mon_bin_hdr doesn't strictly need to be there…. but what's the point of not defining the structure that you _need_ to have to be able to read the data you're requesting?

So in short as for "Why all this?"

Because without this change, making a new userspace implementation of usbmon requires straight-out copying code from the kernel in not-quite-clearcut licensing environment. Exposing this on an UAPI header should address most of that, without tying your hands any more than already present for compatibility, and without diluting the licensing of the structures more than it is right now.

Unless you plan to argue that libpcap is violating the kernel license, but I think that'd be making usbmon toxic enough that I wouldn't be the only one walking away.

Anything else, beside not feeling that we need this?

Diego

Re: [PATCH] usbmon: expose the usbmon structures and constants as an UAPI header.

[Pete Zaitcev]

On Mon, 06 Jul 2020 09:32:09 +0100
Diego Elio Pettenò <flameeyes@flameeyes.com> wrote:

> Most applications rely on one or another kernel header — this has been
> the norm for Linux kernel interface users for many years.

Fine, I never said what you're proposing is without precedent.

> > As for the "duplication", I do not see it as harmful in any way.
> > And they do not re-declare, they declare. You're trying to create a
> > centralized language definition that did not exist previously.  
> 
> It is harmful, because these structures *need* to be exactly
> synchronised or the calls will fail. Or the data will be garbage.

They are already synchronized today. No calls are failing and data
isn't garbage. So you're not proving anything here.

> Either the structure definition is not covered by the syscall exception,
> or it isn't.
> If it is, then we have a bit of an issue, as libpcap is not considering
> itself a derived work of Linux and contains a copy of the same structures.
> [...]

You obviously mised "not", but hey. The important part is, do not see
Paolo doing anything wrong. He has his own definitions, and if he chose
to use the same field names, it's complately fine by me, as a copyright
holder. Your attempts to drag libpcap into this are laughable.

> You _also_ need the structure definition, or at least its size,
> for the _IO macro to be usable.

The question of _IO is a fine point. However, since the handcrafted
application level structs are guaranteed to be the same size, it
seems moot to me.

> Now that does mean that mon_bin_hdr doesn't strictly need to be
> there…. but what's the point of not defining the structure that
> you _need_ to have to be able to read the data you're requesting?

The point is to avoid licensing complications. Usbmon is much older
than "syscall exception".
 
> So in short as for "Why all this?"
> 
> Because without this change, making a new userspace implementation
> of usbmon requires straight-out copying code from the kernel in
> not-quite-clearcut licensing environment.

False. Making a new application for usbmon requires reading the
documentation. It does not requires copying any code from the
kernel. It is your choice to copy that code.

I see that you got Greg to agree to your change in principle, so
I am okay with providing a fixed binary struct for your convenience.
Post a patch that satisfies him and I'll ack it. But please
leave the falsehoods about structs being necessary out of it.
Just don't put them into comments or the commit message.
Can you do at least that much?

-- Pete

Re: [PATCH] usbmon: expose the usbmon structures and constants as an UAPI header.

[-- Attachment #1: Type: text/plain, Size: 1867 bytes --]

Hi "Diego,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v5.8-rc4 next-20200703]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Diego-Elio-Petten/usbmon-expose-the-usbmon-structures-and-constants-as-an-UAPI-header/20200705-230320
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: powerpc-randconfig-r026-20200706 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project a378c0449507e00e96534ff9ce9034e185425182)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> error: include/uapi/linux/usb/mon.h: leak CONFIG_COMPAT to user-space
   make[2]: *** [scripts/Makefile.headersinst:63: usr/include/linux/usb/mon.h] Error 1
   make[2]: Target '__headers' not remade because of errors.
   make[1]: *** [Makefile:1258: headers] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:185: __sub-make] Error 2
   make: Target 'prepare' not remade because of errors.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31949 bytes --]

Re: [PATCH] usbmon: expose the usbmon structures and constants as an UAPI header.

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1907 bytes --]

Hi "Diego,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v5.8-rc4 next-20200703]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Diego-Elio-Petten/usbmon-expose-the-usbmon-structures-and-constants-as-an-UAPI-header/20200705-230320
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: powerpc-randconfig-r026-20200706 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project a378c0449507e00e96534ff9ce9034e185425182)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> error: include/uapi/linux/usb/mon.h: leak CONFIG_COMPAT to user-space
   make[2]: *** [scripts/Makefile.headersinst:63: usr/include/linux/usb/mon.h] Error 1
   make[2]: Target '__headers' not remade because of errors.
   make[1]: *** [Makefile:1258: headers] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:185: __sub-make] Error 2
   make: Target 'prepare' not remade because of errors.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31949 bytes --]

Re: [PATCH] usbmon: expose the usbmon structures and constants as an UAPI header.

[Greg KH]

On Sun, Jul 05, 2020 at 04:02:25PM +0100, Diego Elio Pettenò wrote:
> Previously any application wanting to implement the usbmon binary
> interfaces needed to re-declare the structures and constants, leading to
> structure duplication and confusion over whether these structures fall into
> the system call exception or not.
> 
> Also update Paolo's address to match `net/core/dst_cache.c` as the previous
> one bounces.

When you say "also" that means it should be a separate patch.  Please do
that for this one.

> 
> Cc: linux-usb@vger.kernel.org
> Cc: Pete Zaitcev <zaitcev@redhat.com>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Kris Katterjohn <katterjohn@gmail.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Signed-off-by: Diego Elio Pettenò <flameeyes@flameeyes.com>
> ---
>  Documentation/usb/usbmon.rst |  12 ++---
>  drivers/usb/mon/mon_bin.c    |  94 ++------------------------------
>  include/uapi/linux/usb/mon.h | 102 +++++++++++++++++++++++++++++++++++
>  3 files changed, 111 insertions(+), 97 deletions(-)
>  create mode 100644 include/uapi/linux/usb/mon.h
> 
> diff --git a/Documentation/usb/usbmon.rst b/Documentation/usb/usbmon.rst
> index b0bd51080799..cf98ea553ba1 100644
> --- a/Documentation/usb/usbmon.rst
> +++ b/Documentation/usb/usbmon.rst
> @@ -211,11 +211,13 @@ Bulk wrapper to a storage device at address 5::
>  Raw binary format and API
>  =========================
>  
> -The overall architecture of the API is about the same as the one above,
> -only the events are delivered in binary format. Each event is sent in
> -the following structure (its name is made up, so that we can refer to it)::
> +The overall architecture of the API is about the same as the one above, only the
> +events are delivered in binary format. The structures and constants are defined
> +in linux/usb/mon.h.
>  
> -  struct usbmon_packet {
> +Each event is sent in the following structure::
> +
> +  struct mon_bin_hdr {
>  	u64 id;			/*  0: URB ID - from submission to callback */
>  	unsigned char type;	/*  8: Same as text; extensible. */
>  	unsigned char xfer_type; /*    ISO (0), Intr, Control, Bulk (3) */
> @@ -346,8 +348,6 @@ be polled with select(2) and poll(2). But lseek(2) does not work.
>  
>  * Memory-mapped access of the kernel buffer for the binary API
>  
> -The basic idea is simple:
> -
>  To prepare, map the buffer by getting the current size, then using mmap(2).
>  Then, execute a loop similar to the one written in pseudo-code below::
>  
> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index f48a23adbc35..e914fd8d039e 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -4,8 +4,8 @@
>   *
>   * This is a binary format reader.
>   *
> - * Copyright (C) 2006 Paolo Abeni (paolo.abeni@email.it)
> - * Copyright (C) 2006,2007 Pete Zaitcev (zaitcev@redhat.com)
> + * Copyright (C) 2006 Paolo Abeni <pabeni@redhat.com>

You can't change someone's copyright lines, sorry.

> + * Copyright (C) 2006,2007 Pete Zaitcev <zaitcev@redhat.com>

Why this change?


>   */
>  
>  #include <linux/kernel.h>
> @@ -23,34 +23,10 @@
>  #include <linux/time64.h>
>  
>  #include <linux/uaccess.h>
> +#include <linux/usb/mon.h>
>  
>  #include "usb_mon.h"
>  
> -/*
> - * Defined by USB 2.0 clause 9.3, table 9.2.
> - */
> -#define SETUP_LEN  8
> -
> -/* ioctl macros */
> -#define MON_IOC_MAGIC 0x92
> -
> -#define MON_IOCQ_URB_LEN _IO(MON_IOC_MAGIC, 1)
> -/* #2 used to be MON_IOCX_URB, removed before it got into Linus tree */
> -#define MON_IOCG_STATS _IOR(MON_IOC_MAGIC, 3, struct mon_bin_stats)
> -#define MON_IOCT_RING_SIZE _IO(MON_IOC_MAGIC, 4)
> -#define MON_IOCQ_RING_SIZE _IO(MON_IOC_MAGIC, 5)
> -#define MON_IOCX_GET   _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get)
> -#define MON_IOCX_MFETCH _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch)
> -#define MON_IOCH_MFLUSH _IO(MON_IOC_MAGIC, 8)
> -/* #9 was MON_IOCT_SETAPI */
> -#define MON_IOCX_GETX   _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get)
> -
> -#ifdef CONFIG_COMPAT
> -#define MON_IOCX_GET32 _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get32)
> -#define MON_IOCX_MFETCH32 _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch32)
> -#define MON_IOCX_GETX32   _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get32)
> -#endif
> -
>  /*
>   * Some architectures have enormous basic pages (16KB for ia64, 64KB for ppc).
>   * But it's all right. Just use a simple way to make sure the chunk is never
> @@ -81,38 +57,6 @@
>  #define BUFF_DFL   CHUNK_ALIGN(300*1024)
>  #define BUFF_MIN     CHUNK_ALIGN(8*1024)
>  
> -/*
> - * The per-event API header (2 per URB).
> - *
> - * This structure is seen in userland as defined by the documentation.
> - */
> -struct mon_bin_hdr {
> -	u64 id;			/* URB ID - from submission to callback */
> -	unsigned char type;	/* Same as in text API; extensible. */
> -	unsigned char xfer_type;	/* ISO, Intr, Control, Bulk */
> -	unsigned char epnum;	/* Endpoint number and transfer direction */
> -	unsigned char devnum;	/* Device address */
> -	unsigned short busnum;	/* Bus number */
> -	char flag_setup;
> -	char flag_data;
> -	s64 ts_sec;		/* ktime_get_real_ts64 */
> -	s32 ts_usec;		/* ktime_get_real_ts64 */
> -	int status;
> -	unsigned int len_urb;	/* Length of data (submitted or actual) */
> -	unsigned int len_cap;	/* Delivered length */
> -	union {
> -		unsigned char setup[SETUP_LEN];	/* Only for Control S-type */
> -		struct iso_rec {
> -			int error_count;
> -			int numdesc;
> -		} iso;
> -	} s;
> -	int interval;
> -	int start_frame;
> -	unsigned int xfer_flags;
> -	unsigned int ndesc;	/* Actual number of ISO descriptors */
> -};
> -
>  /*
>   * ISO vector, packed into the head of data stream.
>   * This has to take 16 bytes to make sure that the end of buffer
> @@ -125,38 +69,6 @@ struct mon_bin_isodesc {
>  	u32 _pad;
>  };
>  
> -/* per file statistic */
> -struct mon_bin_stats {
> -	u32 queued;
> -	u32 dropped;
> -};
> -
> -struct mon_bin_get {
> -	struct mon_bin_hdr __user *hdr;	/* Can be 48 bytes or 64. */
> -	void __user *data;
> -	size_t alloc;		/* Length of data (can be zero) */
> -};
> -
> -struct mon_bin_mfetch {
> -	u32 __user *offvec;	/* Vector of events fetched */
> -	u32 nfetch;		/* Number of events to fetch (out: fetched) */
> -	u32 nflush;		/* Number of events to flush */
> -};
> -
> -#ifdef CONFIG_COMPAT
> -struct mon_bin_get32 {
> -	u32 hdr32;
> -	u32 data32;
> -	u32 alloc32;
> -};
> -
> -struct mon_bin_mfetch32 {
> -        u32 offvec32;
> -        u32 nfetch32;
> -        u32 nflush32;
> -};
> -#endif
> -
>  /* Having these two values same prevents wrapping of the mon_bin_hdr */
>  #define PKT_ALIGN   64
>  #define PKT_SIZE    64
> diff --git a/include/uapi/linux/usb/mon.h b/include/uapi/linux/usb/mon.h
> new file mode 100644
> index 000000000000..265e0169e2ef
> --- /dev/null
> +++ b/include/uapi/linux/usb/mon.h
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * USB Monitoring (usbmon) definitions

Put the copyright notices back in here.

> + */
> +
> +#ifndef __UAPI_LINUX_USB_MON_H
> +#define __UAPI_LINUX_USB_MON_H
> +
> +/* ioctl macros */
> +#define MON_IOC_MAGIC 0x92
> +
> +#define MON_IOCQ_URB_LEN _IO(MON_IOC_MAGIC, 1)
> +/* #2 used to be MON_IOCX_URB, removed before it got into Linus tree */
> +#define MON_IOCG_STATS _IOR(MON_IOC_MAGIC, 3, struct mon_bin_stats)
> +#define MON_IOCT_RING_SIZE _IO(MON_IOC_MAGIC, 4)
> +#define MON_IOCQ_RING_SIZE _IO(MON_IOC_MAGIC, 5)
> +#define MON_IOCX_GET   _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get)
> +#define MON_IOCX_MFETCH _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch)
> +#define MON_IOCH_MFLUSH _IO(MON_IOC_MAGIC, 8)
> +/* #9 was MON_IOCT_SETAPI */
> +#define MON_IOCX_GETX   _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get)
> +
> +#ifdef CONFIG_COMPAT
> +#define MON_IOCX_GET32 _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get32)
> +#define MON_IOCX_MFETCH32 _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch32)
> +#define MON_IOCX_GETX32   _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get32)
> +#endif

Alignment?

And you see the kbuild error for this, please fix that up.

> +
> +/* ioctl structures */
> +
> +/* per file statistic */
> +struct mon_bin_stats {
> +	u32 queued;
> +	u32 dropped;
> +};

As you are making this a "real" uapi .h file, you have to use the "real"
data types as well.  That means using __u32 and not u32.  Same for all
others.

> +
> +struct mon_bin_get {
> +	struct mon_bin_hdr __user *hdr;	/* Can be 48 bytes or 64. */
> +	void __user *data;

Does this cross the user/kernel boundry?  Ick, no wonder we need compat
fixups :(

> +	size_t alloc;		/* Length of data (can be zero) */
> +};
> +
> +struct mon_bin_mfetch {
> +	u32 __user *offvec;	/* Vector of events fetched */
> +	u32 nfetch;		/* Number of events to fetch (out: fetched) */
> +	u32 nflush;		/* Number of events to flush */
> +};
> +
> +#ifdef CONFIG_COMPAT
> +struct mon_bin_get32 {
> +	u32 hdr32;
> +	u32 data32;
> +	u32 alloc32;
> +};
> +
> +struct mon_bin_mfetch32 {
> +	u32 offvec32;
> +	u32 nfetch32;
> +	u32 nflush32;
> +};
> +#endif
> +
> +/* Data format */
> +
> +/*
> + * Defined by USB 2.0 clause 9.3, table 9.2.
> + */
> +#define SETUP_LEN  8

Horrible global define, please use USB_MON_SETUP_LEN or something like
that.


> +
> +/*
> + * The per-event API header (2 per URB).
> + *
> + * This structure is seen in userland as defined by the documentation.
> + */
> +struct mon_bin_hdr {
> +	u64 id;			/* URB ID - from submission to callback */
> +	unsigned char type;	/* Same as in text API; extensible. */
> +	unsigned char xfer_type;	/* ISO, Intr, Control, Bulk */
> +	unsigned char epnum;	/* Endpoint number and transfer direction */
> +	unsigned char devnum;	/* Device address */
> +	unsigned short busnum;	/* Bus number */
> +	char flag_setup;
> +	char flag_data;
> +	s64 ts_sec;		/* ktime_get_real_ts64 */
> +	s32 ts_usec;		/* ktime_get_real_ts64 */
> +	int status;
> +	unsigned int len_urb;	/* Length of data (submitted or actual) */
> +	unsigned int len_cap;	/* Delivered length */

Again, correct types please.

thanks,

greg k-h

Re: [PATCH] usbmon: expose the usbmon structures and constants as an UAPI header.

[Diego Elio Pettenò]

On Mon, Jul 6, 2020, at 11:30, Greg KH wrote:
> You can't change someone's copyright lines, sorry.

Then I hope Paolo can send a follow-up patch updating his email address — email.it doesn't seem to exist anymore/doesn't seem to have his address anymore.

> Put the copyright notices back in here.

Done.

> And you see the kbuild error for this, please fix that up.

This should be fixed now by dropping the conditional, which AIUI is the right thing to do in this case? I couldn't find any documentation referencing what to do with CONFIG_COMPAT for headers, but I assume that anything optionally available should be exposed in UAPI and worst case fail at runtime.

> As you are making this a "real" uapi .h file, you have to use the "real"
> data types as well.  That means using __u32 and not u32.  Same for all
> others.

Done, and in the documentation.

> Does this cross the user/kernel boundry?  Ick, no wonder we need compat
> fixups :(

I fear so. The usbmon interface is fairly long-living, and I don't think it matches most of the best practices of today.

I keep telling myself I should spend more time trying to figure out what I can improve overall, because I fear this is better served by having a good thought on the design, and be replaced. But that's a topic for another thread.

Although if someone is interested in whiteboarding an alternative design, I'd be happy to start such a thread or have an offline chat. My current castle-in-the-sky would involve integrating BPF so that capture filters can be applied — and I think that the modern way to stream data from the kernel to userspace would be netlink?

[PATCH v2] usbmon: expose the usbmon structures and constants as an UAPI header.

[Diego Elio Pettenò]

Previously any application wanting to implement the usbmon binary
interfaces needed to re-declare the structures and constants, leading to
structure duplication and confusion over whether these structures fall into
the system call exception or not.

Cc: linux-usb@vger.kernel.org
Cc: Pete Zaitcev <zaitcev@redhat.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Kris Katterjohn <katterjohn@gmail.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Signed-off-by: Diego Elio Pettenò <flameeyes@flameeyes.com>
---
 Documentation/usb/usbmon.rst |  64 ++++++++++-----------
 drivers/usb/mon/mon_bin.c    |  92 +-----------------------------
 include/uapi/linux/usb/mon.h | 105 +++++++++++++++++++++++++++++++++++
 3 files changed, 139 insertions(+), 122 deletions(-)
 create mode 100644 include/uapi/linux/usb/mon.h

diff --git a/Documentation/usb/usbmon.rst b/Documentation/usb/usbmon.rst
index b0bd51080799..da18c5543b2f 100644
--- a/Documentation/usb/usbmon.rst
+++ b/Documentation/usb/usbmon.rst
@@ -211,36 +211,38 @@ Bulk wrapper to a storage device at address 5::
 Raw binary format and API
 =========================
 
-The overall architecture of the API is about the same as the one above,
-only the events are delivered in binary format. Each event is sent in
-the following structure (its name is made up, so that we can refer to it)::
-
-  struct usbmon_packet {
-	u64 id;			/*  0: URB ID - from submission to callback */
-	unsigned char type;	/*  8: Same as text; extensible. */
-	unsigned char xfer_type; /*    ISO (0), Intr, Control, Bulk (3) */
-	unsigned char epnum;	/*     Endpoint number and transfer direction */
-	unsigned char devnum;	/*     Device address */
-	u16 busnum;		/* 12: Bus number */
-	char flag_setup;	/* 14: Same as text */
-	char flag_data;		/* 15: Same as text; Binary zero is OK. */
-	s64 ts_sec;		/* 16: gettimeofday */
-	s32 ts_usec;		/* 24: gettimeofday */
-	int status;		/* 28: */
-	unsigned int length;	/* 32: Length of data (submitted or actual) */
-	unsigned int len_cap;	/* 36: Delivered length */
-	union {			/* 40: */
-		unsigned char setup[SETUP_LEN];	/* Only for Control S-type */
+The overall architecture of the API is about the same as the one above, only the
+events are delivered in binary format. The structures and constants are defined
+in linux/usb/mon.h.
+
+Each event is sent in the following structure::
+
+  struct mon_bin_hdr {
+	u64 id;		/*  0: URB ID - from submission to callback */
+	u8 type;	/*  8: Same as text; extensible. */
+	u8 xfer_type;	/*    ISO (0), Intr, Control, Bulk (3) */
+	u8 epnum;	/*     Endpoint number and transfer direction */
+	u8 devnum;	/*     Device address */
+	u16 busnum;	/* 12: Bus number */
+	s8 flag_setup;	/* 14: Same as text */
+	s8 flag_data;	/* 15: Same as text; Binary zero is OK. */
+	s64 ts_sec;	/* 16: gettimeofday */
+	s32 ts_usec;	/* 24: gettimeofday */
+	s32 status;		/* 28: */
+	u32 length;	/* 32: Length of data (submitted or actual) */
+	u32 len_cap;	/* 36: Delivered length */
+	union {		/* 40: */
+		u8 setup[MON_USB_SETUP_LEN];	/* Only for Control S-type */
 		struct iso_rec {		/* Only for ISO */
-			int error_count;
-			int numdesc;
+			s32 error_count;
+			s32 numdesc;
 		} iso;
 	} s;
-	int interval;		/* 48: Only for Interrupt and ISO */
-	int start_frame;	/* 52: For ISO */
-	unsigned int xfer_flags; /* 56: copy of URB's transfer_flags */
-	unsigned int ndesc;	/* 60: Actual number of ISO descriptors */
-  };				/* 64 total length */
+	s32 interval;	/* 48: Only for Interrupt and ISO */
+	s32 start_frame;/* 52: For ISO */
+	u32 xfer_flags; /* 56: copy of URB's transfer_flags */
+	u32 ndesc;	/* 60: Actual number of ISO descriptors */
+  };			/* 64 total length */
 
 These events can be received from a character device by reading with read(2),
 with an ioctl(2), or by accessing the buffer with mmap. However, read(2)
@@ -313,9 +315,9 @@ This ioctl is primarily used when the application accesses the buffer
 with mmap(2). Its argument is a pointer to the following structure::
 
   struct mon_mfetch_arg {
-	uint32_t *offvec;	/* Vector of events fetched */
-	uint32_t nfetch;	/* Number of events to fetch (out: fetched) */
-	uint32_t nflush;	/* Number of events to flush */
+	u32 *offvec;	/* Vector of events fetched */
+	u32 nfetch;	/* Number of events to fetch (out: fetched) */
+	u32 nflush;	/* Number of events to flush */
   };
 
 The ioctl operates in 3 stages.
@@ -346,8 +348,6 @@ be polled with select(2) and poll(2). But lseek(2) does not work.
 
 * Memory-mapped access of the kernel buffer for the binary API
 
-The basic idea is simple:
-
 To prepare, map the buffer by getting the current size, then using mmap(2).
 Then, execute a loop similar to the one written in pseudo-code below::
 
diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index f48a23adbc35..e1a7e69a3b0c 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -23,34 +23,10 @@
 #include <linux/time64.h>
 
 #include <linux/uaccess.h>
+#include <linux/usb/mon.h>
 
 #include "usb_mon.h"
 
-/*
- * Defined by USB 2.0 clause 9.3, table 9.2.
- */
-#define SETUP_LEN  8
-
-/* ioctl macros */
-#define MON_IOC_MAGIC 0x92
-
-#define MON_IOCQ_URB_LEN _IO(MON_IOC_MAGIC, 1)
-/* #2 used to be MON_IOCX_URB, removed before it got into Linus tree */
-#define MON_IOCG_STATS _IOR(MON_IOC_MAGIC, 3, struct mon_bin_stats)
-#define MON_IOCT_RING_SIZE _IO(MON_IOC_MAGIC, 4)
-#define MON_IOCQ_RING_SIZE _IO(MON_IOC_MAGIC, 5)
-#define MON_IOCX_GET   _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get)
-#define MON_IOCX_MFETCH _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch)
-#define MON_IOCH_MFLUSH _IO(MON_IOC_MAGIC, 8)
-/* #9 was MON_IOCT_SETAPI */
-#define MON_IOCX_GETX   _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get)
-
-#ifdef CONFIG_COMPAT
-#define MON_IOCX_GET32 _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get32)
-#define MON_IOCX_MFETCH32 _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch32)
-#define MON_IOCX_GETX32   _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get32)
-#endif
-
 /*
  * Some architectures have enormous basic pages (16KB for ia64, 64KB for ppc).
  * But it's all right. Just use a simple way to make sure the chunk is never
@@ -81,38 +57,6 @@
 #define BUFF_DFL   CHUNK_ALIGN(300*1024)
 #define BUFF_MIN     CHUNK_ALIGN(8*1024)
 
-/*
- * The per-event API header (2 per URB).
- *
- * This structure is seen in userland as defined by the documentation.
- */
-struct mon_bin_hdr {
-	u64 id;			/* URB ID - from submission to callback */
-	unsigned char type;	/* Same as in text API; extensible. */
-	unsigned char xfer_type;	/* ISO, Intr, Control, Bulk */
-	unsigned char epnum;	/* Endpoint number and transfer direction */
-	unsigned char devnum;	/* Device address */
-	unsigned short busnum;	/* Bus number */
-	char flag_setup;
-	char flag_data;
-	s64 ts_sec;		/* ktime_get_real_ts64 */
-	s32 ts_usec;		/* ktime_get_real_ts64 */
-	int status;
-	unsigned int len_urb;	/* Length of data (submitted or actual) */
-	unsigned int len_cap;	/* Delivered length */
-	union {
-		unsigned char setup[SETUP_LEN];	/* Only for Control S-type */
-		struct iso_rec {
-			int error_count;
-			int numdesc;
-		} iso;
-	} s;
-	int interval;
-	int start_frame;
-	unsigned int xfer_flags;
-	unsigned int ndesc;	/* Actual number of ISO descriptors */
-};
-
 /*
  * ISO vector, packed into the head of data stream.
  * This has to take 16 bytes to make sure that the end of buffer
@@ -125,38 +69,6 @@ struct mon_bin_isodesc {
 	u32 _pad;
 };
 
-/* per file statistic */
-struct mon_bin_stats {
-	u32 queued;
-	u32 dropped;
-};
-
-struct mon_bin_get {
-	struct mon_bin_hdr __user *hdr;	/* Can be 48 bytes or 64. */
-	void __user *data;
-	size_t alloc;		/* Length of data (can be zero) */
-};
-
-struct mon_bin_mfetch {
-	u32 __user *offvec;	/* Vector of events fetched */
-	u32 nfetch;		/* Number of events to fetch (out: fetched) */
-	u32 nflush;		/* Number of events to flush */
-};
-
-#ifdef CONFIG_COMPAT
-struct mon_bin_get32 {
-	u32 hdr32;
-	u32 data32;
-	u32 alloc32;
-};
-
-struct mon_bin_mfetch32 {
-        u32 offvec32;
-        u32 nfetch32;
-        u32 nflush32;
-};
-#endif
-
 /* Having these two values same prevents wrapping of the mon_bin_hdr */
 #define PKT_ALIGN   64
 #define PKT_SIZE    64
@@ -396,7 +308,7 @@ static inline char mon_bin_get_setup(unsigned char *setupb,
 
 	if (urb->setup_packet == NULL)
 		return 'Z';
-	memcpy(setupb, urb->setup_packet, SETUP_LEN);
+	memcpy(setupb, urb->setup_packet, MON_USB_SETUP_LEN);
 	return 0;
 }
 
diff --git a/include/uapi/linux/usb/mon.h b/include/uapi/linux/usb/mon.h
new file mode 100644
index 000000000000..376f93acdde2
--- /dev/null
+++ b/include/uapi/linux/usb/mon.h
@@ -0,0 +1,105 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * USB Monitoring (usbmon) definitions
+ *
+ * See documentation in Documentation/usb/usbmon.rst.
+ *
+ * Copyright (C) 2006 Paolo Abeni (paolo.abeni@email.it)
+ * Copyright (C) 2006,2007 Pete Zaitcev (zaitcev@redhat.com)
+ */
+
+#ifndef __UAPI_LINUX_USB_MON_H
+#define __UAPI_LINUX_USB_MON_H
+
+/* ioctl macros */
+#define MON_IOC_MAGIC 0x92
+
+#define MON_IOCQ_URB_LEN	_IO(MON_IOC_MAGIC, 1)
+/* #2 used to be MON_IOCX_URB, removed before it got into Linus tree */
+#define MON_IOCG_STATS		_IOR(MON_IOC_MAGIC, 3, struct mon_bin_stats)
+#define MON_IOCT_RING_SIZE	_IO(MON_IOC_MAGIC, 4)
+#define MON_IOCQ_RING_SIZE	_IO(MON_IOC_MAGIC, 5)
+#define MON_IOCX_GET		_IOW(MON_IOC_MAGIC, 6, struct mon_bin_get)
+#define MON_IOCX_MFETCH		_IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch)
+#define MON_IOCH_MFLUSH		_IO(MON_IOC_MAGIC, 8)
+/* #9 was MON_IOCT_SETAPI */
+#define MON_IOCX_GETX		_IOW(MON_IOC_MAGIC, 10, struct mon_bin_get)
+
+/* ioctl structures */
+
+/* per file statistic */
+struct mon_bin_stats {
+	__u32 queued;
+	__u32 dropped;
+};
+
+struct mon_bin_get {
+	struct mon_bin_hdr __user *hdr;	/* Can be 48 bytes or 64. */
+	void __user *data;
+	size_t alloc;		/* Length of data (can be zero) */
+};
+
+struct mon_bin_mfetch {
+	__u32 __user *offvec;	/* Vector of events fetched */
+	__u32 nfetch;		/* Number of events to fetch (out: fetched) */
+	__u32 nflush;		/* Number of events to flush */
+};
+
+
+/* Only defined with CONFIG_COMPAT */
+#define MON_IOCX_GET32		_IOW(MON_IOC_MAGIC, 6, struct mon_bin_get32)
+#define MON_IOCX_MFETCH32	_IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch32)
+#define MON_IOCX_GETX32		_IOW(MON_IOC_MAGIC, 10, struct mon_bin_get32)
+
+struct mon_bin_get32 {
+	__u32 hdr32;
+	__u32 data32;
+	__u32 alloc32;
+};
+
+struct mon_bin_mfetch32 {
+	__u32 offvec32;
+	__u32 nfetch32;
+	__u32 nflush32;
+};
+
+/* Data format */
+
+/*
+ * Defined by USB 2.0 clause 9.3, table 9.2.
+ */
+#define MON_USB_SETUP_LEN  8
+
+/*
+ * The per-event API header (2 per URB).
+ *
+ * This structure is seen in userland as defined by the documentation.
+ */
+struct mon_bin_hdr {
+	__u64 id;	/* URB ID - from submission to callback */
+	__u8 type;	/* Same as in text API; extensible. */
+	__u8 xfer_type;	/* ISO, Intr, Control, Bulk */
+	__u8 epnum;	/* Endpoint number and transfer direction */
+	__u8 devnum;	/* Device address */
+	__u16 busnum;	/* Bus number */
+	__s8 flag_setup;
+	__s8 flag_data;
+	__s64 ts_sec;	/* ktime_get_real_ts64 */
+	__s32 ts_usec;	/* ktime_get_real_ts64 */
+	__s32 status;
+	__u32 len_urb;	/* Length of data (submitted or actual) */
+	__u32 len_cap;	/* Delivered length */
+	union {
+		__u8 setup[MON_USB_SETUP_LEN];	/* Only for Control S-type */
+		struct iso_rec {
+			__s32 error_count;
+			__s32 numdesc;
+		} iso;
+	} s;
+	__s32 interval;
+	__s32 start_frame;
+	__u32 xfer_flags;
+	__u32 ndesc;	/* Actual number of ISO descriptors */
+};
+
+#endif /* __UAPI_LINUX_USB_MON_H */
-- 
2.27.0

Re: [PATCH v2] usbmon: expose the usbmon structures and constants as an UAPI header.

[Greg KH]

On Mon, Jul 06, 2020 at 01:15:09PM +0100, Diego Elio Pettenò wrote:
> Previously any application wanting to implement the usbmon binary
> interfaces needed to re-declare the structures and constants, leading to
> structure duplication and confusion over whether these structures fall into
> the system call exception or not.
> 
> Cc: linux-usb@vger.kernel.org
> Cc: Pete Zaitcev <zaitcev@redhat.com>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Kris Katterjohn <katterjohn@gmail.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Signed-off-by: Diego Elio Pettenò <flameeyes@flameeyes.com>
> ---
>  Documentation/usb/usbmon.rst |  64 ++++++++++-----------
>  drivers/usb/mon/mon_bin.c    |  92 +-----------------------------
>  include/uapi/linux/usb/mon.h | 105 +++++++++++++++++++++++++++++++++++
>  3 files changed, 139 insertions(+), 122 deletions(-)
>  create mode 100644 include/uapi/linux/usb/mon.h
> 
> diff --git a/Documentation/usb/usbmon.rst b/Documentation/usb/usbmon.rst
> index b0bd51080799..da18c5543b2f 100644
> --- a/Documentation/usb/usbmon.rst
> +++ b/Documentation/usb/usbmon.rst
> @@ -211,36 +211,38 @@ Bulk wrapper to a storage device at address 5::
>  Raw binary format and API
>  =========================
>  
> -The overall architecture of the API is about the same as the one above,
> -only the events are delivered in binary format. Each event is sent in
> -the following structure (its name is made up, so that we can refer to it)::
> -
> -  struct usbmon_packet {
> -	u64 id;			/*  0: URB ID - from submission to callback */
> -	unsigned char type;	/*  8: Same as text; extensible. */
> -	unsigned char xfer_type; /*    ISO (0), Intr, Control, Bulk (3) */
> -	unsigned char epnum;	/*     Endpoint number and transfer direction */
> -	unsigned char devnum;	/*     Device address */
> -	u16 busnum;		/* 12: Bus number */
> -	char flag_setup;	/* 14: Same as text */
> -	char flag_data;		/* 15: Same as text; Binary zero is OK. */
> -	s64 ts_sec;		/* 16: gettimeofday */
> -	s32 ts_usec;		/* 24: gettimeofday */
> -	int status;		/* 28: */
> -	unsigned int length;	/* 32: Length of data (submitted or actual) */
> -	unsigned int len_cap;	/* 36: Delivered length */
> -	union {			/* 40: */
> -		unsigned char setup[SETUP_LEN];	/* Only for Control S-type */
> +The overall architecture of the API is about the same as the one above, only the

Why change that line?

> +events are delivered in binary format. The structures and constants are defined
> +in linux/usb/mon.h.

Not your new uapi file location?

> +
> +Each event is sent in the following structure::
> +
> +  struct mon_bin_hdr {
> +	u64 id;		/*  0: URB ID - from submission to callback */

__u64, right?  Same for the rest of this file...

> +	u8 type;	/*  8: Same as text; extensible. */
> +	u8 xfer_type;	/*    ISO (0), Intr, Control, Bulk (3) */
> +	u8 epnum;	/*     Endpoint number and transfer direction */
> +	u8 devnum;	/*     Device address */
> +	u16 busnum;	/* 12: Bus number */
> +	s8 flag_setup;	/* 14: Same as text */
> +	s8 flag_data;	/* 15: Same as text; Binary zero is OK. */
> +	s64 ts_sec;	/* 16: gettimeofday */
> +	s32 ts_usec;	/* 24: gettimeofday */
> +	s32 status;		/* 28: */
> +	u32 length;	/* 32: Length of data (submitted or actual) */
> +	u32 len_cap;	/* 36: Delivered length */
> +	union {		/* 40: */
> +		u8 setup[MON_USB_SETUP_LEN];	/* Only for Control S-type */
>  		struct iso_rec {		/* Only for ISO */
> -			int error_count;
> -			int numdesc;
> +			s32 error_count;
> +			s32 numdesc;
>  		} iso;
>  	} s;
> -	int interval;		/* 48: Only for Interrupt and ISO */
> -	int start_frame;	/* 52: For ISO */
> -	unsigned int xfer_flags; /* 56: copy of URB's transfer_flags */
> -	unsigned int ndesc;	/* 60: Actual number of ISO descriptors */
> -  };				/* 64 total length */
> +	s32 interval;	/* 48: Only for Interrupt and ISO */
> +	s32 start_frame;/* 52: For ISO */
> +	u32 xfer_flags; /* 56: copy of URB's transfer_flags */
> +	u32 ndesc;	/* 60: Actual number of ISO descriptors */
> +  };			/* 64 total length */
>  
>  These events can be received from a character device by reading with read(2),
>  with an ioctl(2), or by accessing the buffer with mmap. However, read(2)
> @@ -313,9 +315,9 @@ This ioctl is primarily used when the application accesses the buffer
>  with mmap(2). Its argument is a pointer to the following structure::
>  
>    struct mon_mfetch_arg {
> -	uint32_t *offvec;	/* Vector of events fetched */
> -	uint32_t nfetch;	/* Number of events to fetch (out: fetched) */
> -	uint32_t nflush;	/* Number of events to flush */
> +	u32 *offvec;	/* Vector of events fetched */
> +	u32 nfetch;	/* Number of events to fetch (out: fetched) */
> +	u32 nflush;	/* Number of events to flush */

Again, __u32

>    };
>  
>  The ioctl operates in 3 stages.
> @@ -346,8 +348,6 @@ be polled with select(2) and poll(2). But lseek(2) does not work.
>  
>  * Memory-mapped access of the kernel buffer for the binary API
>  
> -The basic idea is simple:
> -

Why drop this?  If you want to clean up the documentation, wonderful,
but again, don't do that in the same patch.

Remember, each patch can only do ONE thing.


>  To prepare, map the buffer by getting the current size, then using mmap(2).
>  Then, execute a loop similar to the one written in pseudo-code below::
>  
> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index f48a23adbc35..e1a7e69a3b0c 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -23,34 +23,10 @@
>  #include <linux/time64.h>
>  
>  #include <linux/uaccess.h>
> +#include <linux/usb/mon.h>
>  
>  #include "usb_mon.h"
>  
> -/*
> - * Defined by USB 2.0 clause 9.3, table 9.2.
> - */
> -#define SETUP_LEN  8
> -
> -/* ioctl macros */
> -#define MON_IOC_MAGIC 0x92
> -
> -#define MON_IOCQ_URB_LEN _IO(MON_IOC_MAGIC, 1)
> -/* #2 used to be MON_IOCX_URB, removed before it got into Linus tree */
> -#define MON_IOCG_STATS _IOR(MON_IOC_MAGIC, 3, struct mon_bin_stats)
> -#define MON_IOCT_RING_SIZE _IO(MON_IOC_MAGIC, 4)
> -#define MON_IOCQ_RING_SIZE _IO(MON_IOC_MAGIC, 5)
> -#define MON_IOCX_GET   _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get)
> -#define MON_IOCX_MFETCH _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch)
> -#define MON_IOCH_MFLUSH _IO(MON_IOC_MAGIC, 8)
> -/* #9 was MON_IOCT_SETAPI */
> -#define MON_IOCX_GETX   _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get)
> -
> -#ifdef CONFIG_COMPAT
> -#define MON_IOCX_GET32 _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get32)
> -#define MON_IOCX_MFETCH32 _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch32)
> -#define MON_IOCX_GETX32   _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get32)
> -#endif
> -
>  /*
>   * Some architectures have enormous basic pages (16KB for ia64, 64KB for ppc).
>   * But it's all right. Just use a simple way to make sure the chunk is never
> @@ -81,38 +57,6 @@
>  #define BUFF_DFL   CHUNK_ALIGN(300*1024)
>  #define BUFF_MIN     CHUNK_ALIGN(8*1024)
>  
> -/*
> - * The per-event API header (2 per URB).
> - *
> - * This structure is seen in userland as defined by the documentation.
> - */
> -struct mon_bin_hdr {
> -	u64 id;			/* URB ID - from submission to callback */
> -	unsigned char type;	/* Same as in text API; extensible. */
> -	unsigned char xfer_type;	/* ISO, Intr, Control, Bulk */
> -	unsigned char epnum;	/* Endpoint number and transfer direction */
> -	unsigned char devnum;	/* Device address */
> -	unsigned short busnum;	/* Bus number */
> -	char flag_setup;
> -	char flag_data;
> -	s64 ts_sec;		/* ktime_get_real_ts64 */
> -	s32 ts_usec;		/* ktime_get_real_ts64 */
> -	int status;
> -	unsigned int len_urb;	/* Length of data (submitted or actual) */
> -	unsigned int len_cap;	/* Delivered length */
> -	union {
> -		unsigned char setup[SETUP_LEN];	/* Only for Control S-type */
> -		struct iso_rec {
> -			int error_count;
> -			int numdesc;
> -		} iso;
> -	} s;
> -	int interval;
> -	int start_frame;
> -	unsigned int xfer_flags;
> -	unsigned int ndesc;	/* Actual number of ISO descriptors */
> -};
> -
>  /*
>   * ISO vector, packed into the head of data stream.
>   * This has to take 16 bytes to make sure that the end of buffer
> @@ -125,38 +69,6 @@ struct mon_bin_isodesc {
>  	u32 _pad;
>  };
>  
> -/* per file statistic */
> -struct mon_bin_stats {
> -	u32 queued;
> -	u32 dropped;
> -};
> -
> -struct mon_bin_get {
> -	struct mon_bin_hdr __user *hdr;	/* Can be 48 bytes or 64. */
> -	void __user *data;
> -	size_t alloc;		/* Length of data (can be zero) */
> -};
> -
> -struct mon_bin_mfetch {
> -	u32 __user *offvec;	/* Vector of events fetched */
> -	u32 nfetch;		/* Number of events to fetch (out: fetched) */
> -	u32 nflush;		/* Number of events to flush */
> -};
> -
> -#ifdef CONFIG_COMPAT
> -struct mon_bin_get32 {
> -	u32 hdr32;
> -	u32 data32;
> -	u32 alloc32;
> -};
> -
> -struct mon_bin_mfetch32 {
> -        u32 offvec32;
> -        u32 nfetch32;
> -        u32 nflush32;
> -};
> -#endif
> -
>  /* Having these two values same prevents wrapping of the mon_bin_hdr */
>  #define PKT_ALIGN   64
>  #define PKT_SIZE    64
> @@ -396,7 +308,7 @@ static inline char mon_bin_get_setup(unsigned char *setupb,
>  
>  	if (urb->setup_packet == NULL)
>  		return 'Z';
> -	memcpy(setupb, urb->setup_packet, SETUP_LEN);
> +	memcpy(setupb, urb->setup_packet, MON_USB_SETUP_LEN);
>  	return 0;
>  }
>  
> diff --git a/include/uapi/linux/usb/mon.h b/include/uapi/linux/usb/mon.h
> new file mode 100644
> index 000000000000..376f93acdde2
> --- /dev/null
> +++ b/include/uapi/linux/usb/mon.h
> @@ -0,0 +1,105 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * USB Monitoring (usbmon) definitions
> + *
> + * See documentation in Documentation/usb/usbmon.rst.
> + *
> + * Copyright (C) 2006 Paolo Abeni (paolo.abeni@email.it)
> + * Copyright (C) 2006,2007 Pete Zaitcev (zaitcev@redhat.com)
> + */
> +
> +#ifndef __UAPI_LINUX_USB_MON_H
> +#define __UAPI_LINUX_USB_MON_H
> +
> +/* ioctl macros */
> +#define MON_IOC_MAGIC 0x92
> +
> +#define MON_IOCQ_URB_LEN	_IO(MON_IOC_MAGIC, 1)
> +/* #2 used to be MON_IOCX_URB, removed before it got into Linus tree */
> +#define MON_IOCG_STATS		_IOR(MON_IOC_MAGIC, 3, struct mon_bin_stats)
> +#define MON_IOCT_RING_SIZE	_IO(MON_IOC_MAGIC, 4)
> +#define MON_IOCQ_RING_SIZE	_IO(MON_IOC_MAGIC, 5)
> +#define MON_IOCX_GET		_IOW(MON_IOC_MAGIC, 6, struct mon_bin_get)
> +#define MON_IOCX_MFETCH		_IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch)
> +#define MON_IOCH_MFLUSH		_IO(MON_IOC_MAGIC, 8)
> +/* #9 was MON_IOCT_SETAPI */
> +#define MON_IOCX_GETX		_IOW(MON_IOC_MAGIC, 10, struct mon_bin_get)
> +
> +/* ioctl structures */
> +
> +/* per file statistic */
> +struct mon_bin_stats {
> +	__u32 queued;
> +	__u32 dropped;
> +};
> +
> +struct mon_bin_get {
> +	struct mon_bin_hdr __user *hdr;	/* Can be 48 bytes or 64. */
> +	void __user *data;
> +	size_t alloc;		/* Length of data (can be zero) */

is size_t a value we can pass across user/kernel boundry?  Are you sure
this isn't __kernel_size_t?

thanks,

greg k-h

Re: [PATCH v2] usbmon: expose the usbmon structures and constants as an UAPI header.

[Diego Elio Pettenò]

On Mon, Jul 6, 2020, at 13:49, Greg KH wrote:
> > -The overall architecture of the API is about the same as the one above,
> > -only the events are delivered in binary format. Each event is sent in
> > -the following structure (its name is made up, so that we can refer to it)::
[snip]
> > +The overall architecture of the API is about the same as the one above, only the
> 
> Why change that line?

The original text says that the structure had a made up name — since there was no exposed structures to match. Now there is, it's no longer made up.

> 
> > +events are delivered in binary format. The structures and constants are defined
> > +in linux/usb/mon.h.
> 
> Not your new uapi file location?

That is the new uapi header — that'd be the location on the installed system, updated with the location within the repository instead.

> __u64, right?  Same for the rest of this file...

Ack.

> Why drop this?  If you want to clean up the documentation, wonderful,
> but again, don't do that in the same patch.

Okay split that one line change to a separate change. I really should try to sit down and improve the docs overall, but that's a much bigger undertaking.

> is size_t a value we can pass across user/kernel boundry?  Are you sure
> this isn't __kernel_size_t?

No I'm not sure and I was pondering myself. I'll trust your instinct because you know this much better than me.

But… I'm not sure how to use that from an uapi header? The __kernel_size_t does not appear to be defined by including linux/types.h. There's only three users of it in include/uapi/ and the only one that does not appear to re-define it as a typedef is include/uapi/linux/uio.h, but using the same include doesn't seem to work for me.

So I expect I'll need a v3 for this one ;)

Re: [PATCH v2] usbmon: expose the usbmon structures and constants as an UAPI header.

[Pete Zaitcev]

On Mon, 6 Jul 2020 14:49:43 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> > +struct mon_bin_get {
> > +	struct mon_bin_hdr __user *hdr;	/* Can be 48 bytes or 64. */
> > +	void __user *data;
> > +	size_t alloc;		/* Length of data (can be zero) */  
> 
> is size_t a value we can pass across user/kernel boundry?  Are you sure
> this isn't __kernel_size_t?

Sorry, it was my fault letting that one through.

Since currently the definitions are separate, userland uses their size_t,
and kernel uses in-kernel size_t. We have a set of MON_IOCX_GET and
MON_IOCX_GET32 with the same base number 6, but using mon_bin_get32,
so the resulting ioctl number magically matched what 32- and 64-bit
applications used. We don't even need an adaptation layer that
re-encodes the argument structure.

Not sure how to resolve this properly once we attempt to export the
structures. Something for the patch submitter to work out, I suppose.

-- Pere

[PATCH v3 1/2] Remove documentation line that adds nothing and sounds condescending.

[Diego Elio Pettenò]

Signed-off-by: Diego Elio Pettenò <flameeyes@flameeyes.com>
---
 Documentation/usb/usbmon.rst | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/usb/usbmon.rst b/Documentation/usb/usbmon.rst
index b0bd51080799..e9ec7e40b3bf 100644
--- a/Documentation/usb/usbmon.rst
+++ b/Documentation/usb/usbmon.rst
@@ -346,8 +346,6 @@ be polled with select(2) and poll(2). But lseek(2) does not work.
 
 * Memory-mapped access of the kernel buffer for the binary API
 
-The basic idea is simple:
-
 To prepare, map the buffer by getting the current size, then using mmap(2).
 Then, execute a loop similar to the one written in pseudo-code below::
 
-- 
2.27.0

[PATCH v3 2/2] usbmon: expose the usbmon structures and constants as an UAPI header.

[Diego Elio Pettenò]

Previously any application wanting to implement the usbmon binary
interfaces needed to re-declare the structures and constants, leading to
structure duplication and confusion over whether these structures fall into
the system call exception or not.

Cc: linux-usb@vger.kernel.org
Cc: Pete Zaitcev <zaitcev@redhat.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Kris Katterjohn <katterjohn@gmail.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Signed-off-by: Diego Elio Pettenò <flameeyes@flameeyes.com>
---
 Documentation/usb/usbmon.rst |  70 ++++++++++++-----------
 drivers/usb/mon/mon_bin.c    |  92 +-----------------------------
 include/uapi/linux/usb/mon.h | 107 +++++++++++++++++++++++++++++++++++
 3 files changed, 145 insertions(+), 124 deletions(-)
 create mode 100644 include/uapi/linux/usb/mon.h

diff --git a/Documentation/usb/usbmon.rst b/Documentation/usb/usbmon.rst
index e9ec7e40b3bf..31c14b38fd03 100644
--- a/Documentation/usb/usbmon.rst
+++ b/Documentation/usb/usbmon.rst
@@ -211,35 +211,37 @@ Bulk wrapper to a storage device at address 5::
 Raw binary format and API
 =========================
 
-The overall architecture of the API is about the same as the one above,
-only the events are delivered in binary format. Each event is sent in
-the following structure (its name is made up, so that we can refer to it)::
-
-  struct usbmon_packet {
-	u64 id;			/*  0: URB ID - from submission to callback */
-	unsigned char type;	/*  8: Same as text; extensible. */
-	unsigned char xfer_type; /*    ISO (0), Intr, Control, Bulk (3) */
-	unsigned char epnum;	/*     Endpoint number and transfer direction */
-	unsigned char devnum;	/*     Device address */
-	u16 busnum;		/* 12: Bus number */
-	char flag_setup;	/* 14: Same as text */
-	char flag_data;		/* 15: Same as text; Binary zero is OK. */
-	s64 ts_sec;		/* 16: gettimeofday */
-	s32 ts_usec;		/* 24: gettimeofday */
-	int status;		/* 28: */
-	unsigned int length;	/* 32: Length of data (submitted or actual) */
-	unsigned int len_cap;	/* 36: Delivered length */
+The overall architecture of the API is about the same as the one above, only the
+events are delivered in binary format. The structures and constants are defined
+in include/uapi/linux/usb/mon.h.
+
+Each event is sent in the following structure::
+
+  struct mon_bin_hdr {
+	__u64 id;		/*  0: URB ID - from submission to callback */
+	__u8 type;		/*  8: Same as text; extensible. */
+	__u8 xfer_type;		/*    ISO (0), Intr, Control, Bulk (3) */
+	__u8 epnum;		/*     Endpoint number and transfer direction */
+	__u8 devnum;		/*     Device address */
+	__u16 busnum;		/* 12: Bus number */
+	__s8 flag_setup;	/* 14: Same as text */
+	__s8 flag_data;		/* 15: Same as text; Binary zero is OK. */
+	__s64 ts_sec;		/* 16: gettimeofday */
+	__s32 ts_usec;		/* 24: gettimeofday */
+	__s32 status;		/* 28: */
+	__u32 length;		/* 32: Length of data (submitted or actual) */
+	__u32 len_cap;		/* 36: Delivered length */
 	union {			/* 40: */
-		unsigned char setup[SETUP_LEN];	/* Only for Control S-type */
+		__u8 setup[MON_USB_SETUP_LEN];	/* Only for Control S-type */
 		struct iso_rec {		/* Only for ISO */
-			int error_count;
-			int numdesc;
+			__s32 error_count;
+			__s32 numdesc;
 		} iso;
 	} s;
-	int interval;		/* 48: Only for Interrupt and ISO */
-	int start_frame;	/* 52: For ISO */
-	unsigned int xfer_flags; /* 56: copy of URB's transfer_flags */
-	unsigned int ndesc;	/* 60: Actual number of ISO descriptors */
+	__s32 interval;		/* 48: Only for Interrupt and ISO */
+	__s32 start_frame;	/* 52: For ISO */
+	__u32 xfer_flags;	/* 56: copy of URB's transfer_flags */
+	__u32 ndesc;		/* 60: Actual number of ISO descriptors */
   };				/* 64 total length */
 
 These events can be received from a character device by reading with read(2),
@@ -267,8 +269,8 @@ no events are available.
 The argument is a pointer to the following structure::
 
   struct mon_bin_stats {
-	u32 queued;
-	u32 dropped;
+	__u32 queued;
+	__u32 dropped;
   };
 
 The member "queued" refers to the number of events currently queued in the
@@ -296,9 +298,9 @@ then return the first event. The argument is a pointer to the following
 structure::
 
   struct mon_get_arg {
-	struct usbmon_packet *hdr;
+	struct mon_bin_hdr *hdr;
 	void *data;
-	size_t alloc;		/* Length of data (can be zero) */
+	__kernel_size_t alloc;		/* Length of data (can be zero) */
   };
 
 Before the call, hdr, data, and alloc should be filled. Upon return, the area
@@ -313,9 +315,9 @@ This ioctl is primarily used when the application accesses the buffer
 with mmap(2). Its argument is a pointer to the following structure::
 
   struct mon_mfetch_arg {
-	uint32_t *offvec;	/* Vector of events fetched */
-	uint32_t nfetch;	/* Number of events to fetch (out: fetched) */
-	uint32_t nflush;	/* Number of events to flush */
+	__u32 *offvec;	/* Vector of events fetched */
+	__u32 nfetch;	/* Number of events to fetch (out: fetched) */
+	__u32 nflush;	/* Number of events to flush */
   };
 
 The ioctl operates in 3 stages.
@@ -350,7 +352,7 @@ To prepare, map the buffer by getting the current size, then using mmap(2).
 Then, execute a loop similar to the one written in pseudo-code below::
 
    struct mon_mfetch_arg fetch;
-   struct usbmon_packet *hdr;
+   struct mon_bin_hdr *hdr;
    int nflush = 0;
    for (;;) {
       fetch.offvec = vec; // Has N 32-bit words
@@ -359,7 +361,7 @@ Then, execute a loop similar to the one written in pseudo-code below::
       ioctl(fd, MON_IOCX_MFETCH, &fetch);   // Process errors, too
       nflush = fetch.nfetch;       // This many packets to flush when done
       for (i = 0; i < nflush; i++) {
-         hdr = (struct ubsmon_packet *) &mmap_area[vec[i]];
+         hdr = (struct mon_bin_hdr *) &mmap_area[vec[i]];
          if (hdr->type == '@')     // Filler packet
             continue;
          caddr_t data = &mmap_area[vec[i]] + 64;
diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index f48a23adbc35..e1a7e69a3b0c 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -23,34 +23,10 @@
 #include <linux/time64.h>
 
 #include <linux/uaccess.h>
+#include <linux/usb/mon.h>
 
 #include "usb_mon.h"
 
-/*
- * Defined by USB 2.0 clause 9.3, table 9.2.
- */
-#define SETUP_LEN  8
-
-/* ioctl macros */
-#define MON_IOC_MAGIC 0x92
-
-#define MON_IOCQ_URB_LEN _IO(MON_IOC_MAGIC, 1)
-/* #2 used to be MON_IOCX_URB, removed before it got into Linus tree */
-#define MON_IOCG_STATS _IOR(MON_IOC_MAGIC, 3, struct mon_bin_stats)
-#define MON_IOCT_RING_SIZE _IO(MON_IOC_MAGIC, 4)
-#define MON_IOCQ_RING_SIZE _IO(MON_IOC_MAGIC, 5)
-#define MON_IOCX_GET   _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get)
-#define MON_IOCX_MFETCH _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch)
-#define MON_IOCH_MFLUSH _IO(MON_IOC_MAGIC, 8)
-/* #9 was MON_IOCT_SETAPI */
-#define MON_IOCX_GETX   _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get)
-
-#ifdef CONFIG_COMPAT
-#define MON_IOCX_GET32 _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get32)
-#define MON_IOCX_MFETCH32 _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch32)
-#define MON_IOCX_GETX32   _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get32)
-#endif
-
 /*
  * Some architectures have enormous basic pages (16KB for ia64, 64KB for ppc).
  * But it's all right. Just use a simple way to make sure the chunk is never
@@ -81,38 +57,6 @@
 #define BUFF_DFL   CHUNK_ALIGN(300*1024)
 #define BUFF_MIN     CHUNK_ALIGN(8*1024)
 
-/*
- * The per-event API header (2 per URB).
- *
- * This structure is seen in userland as defined by the documentation.
- */
-struct mon_bin_hdr {
-	u64 id;			/* URB ID - from submission to callback */
-	unsigned char type;	/* Same as in text API; extensible. */
-	unsigned char xfer_type;	/* ISO, Intr, Control, Bulk */
-	unsigned char epnum;	/* Endpoint number and transfer direction */
-	unsigned char devnum;	/* Device address */
-	unsigned short busnum;	/* Bus number */
-	char flag_setup;
-	char flag_data;
-	s64 ts_sec;		/* ktime_get_real_ts64 */
-	s32 ts_usec;		/* ktime_get_real_ts64 */
-	int status;
-	unsigned int len_urb;	/* Length of data (submitted or actual) */
-	unsigned int len_cap;	/* Delivered length */
-	union {
-		unsigned char setup[SETUP_LEN];	/* Only for Control S-type */
-		struct iso_rec {
-			int error_count;
-			int numdesc;
-		} iso;
-	} s;
-	int interval;
-	int start_frame;
-	unsigned int xfer_flags;
-	unsigned int ndesc;	/* Actual number of ISO descriptors */
-};
-
 /*
  * ISO vector, packed into the head of data stream.
  * This has to take 16 bytes to make sure that the end of buffer
@@ -125,38 +69,6 @@ struct mon_bin_isodesc {
 	u32 _pad;
 };
 
-/* per file statistic */
-struct mon_bin_stats {
-	u32 queued;
-	u32 dropped;
-};
-
-struct mon_bin_get {
-	struct mon_bin_hdr __user *hdr;	/* Can be 48 bytes or 64. */
-	void __user *data;
-	size_t alloc;		/* Length of data (can be zero) */
-};
-
-struct mon_bin_mfetch {
-	u32 __user *offvec;	/* Vector of events fetched */
-	u32 nfetch;		/* Number of events to fetch (out: fetched) */
-	u32 nflush;		/* Number of events to flush */
-};
-
-#ifdef CONFIG_COMPAT
-struct mon_bin_get32 {
-	u32 hdr32;
-	u32 data32;
-	u32 alloc32;
-};
-
-struct mon_bin_mfetch32 {
-        u32 offvec32;
-        u32 nfetch32;
-        u32 nflush32;
-};
-#endif
-
 /* Having these two values same prevents wrapping of the mon_bin_hdr */
 #define PKT_ALIGN   64
 #define PKT_SIZE    64
@@ -396,7 +308,7 @@ static inline char mon_bin_get_setup(unsigned char *setupb,
 
 	if (urb->setup_packet == NULL)
 		return 'Z';
-	memcpy(setupb, urb->setup_packet, SETUP_LEN);
+	memcpy(setupb, urb->setup_packet, MON_USB_SETUP_LEN);
 	return 0;
 }
 
diff --git a/include/uapi/linux/usb/mon.h b/include/uapi/linux/usb/mon.h
new file mode 100644
index 000000000000..3557fe6a93e6
--- /dev/null
+++ b/include/uapi/linux/usb/mon.h
@@ -0,0 +1,107 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * USB Monitoring (usbmon) definitions
+ *
+ * See documentation in Documentation/usb/usbmon.rst.
+ *
+ * Copyright (C) 2006 Paolo Abeni (paolo.abeni@email.it)
+ * Copyright (C) 2006,2007 Pete Zaitcev (zaitcev@redhat.com)
+ */
+
+#ifndef __UAPI_LINUX_USB_MON_H
+#define __UAPI_LINUX_USB_MON_H
+
+#include <linux/types.h>
+
+/* ioctl macros */
+#define MON_IOC_MAGIC 0x92
+
+#define MON_IOCQ_URB_LEN	_IO(MON_IOC_MAGIC, 1)
+/* #2 used to be MON_IOCX_URB, removed before it got into Linus tree */
+#define MON_IOCG_STATS		_IOR(MON_IOC_MAGIC, 3, struct mon_bin_stats)
+#define MON_IOCT_RING_SIZE	_IO(MON_IOC_MAGIC, 4)
+#define MON_IOCQ_RING_SIZE	_IO(MON_IOC_MAGIC, 5)
+#define MON_IOCX_GET		_IOW(MON_IOC_MAGIC, 6, struct mon_bin_get)
+#define MON_IOCX_MFETCH		_IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch)
+#define MON_IOCH_MFLUSH		_IO(MON_IOC_MAGIC, 8)
+/* #9 was MON_IOCT_SETAPI */
+#define MON_IOCX_GETX		_IOW(MON_IOC_MAGIC, 10, struct mon_bin_get)
+
+/* ioctl structures */
+
+/* per file statistic */
+struct mon_bin_stats {
+	__u32 queued;
+	__u32 dropped;
+};
+
+struct mon_bin_get {
+	struct mon_bin_hdr __user *hdr;	/* Can be 48 bytes or 64. */
+	void __user *data;
+	size_t alloc;		/* Length of data (can be zero) */
+};
+
+struct mon_bin_mfetch {
+	__u32 __user *offvec;	/* Vector of events fetched */
+	__u32 nfetch;		/* Number of events to fetch (out: fetched) */
+	__u32 nflush;		/* Number of events to flush */
+};
+
+
+/* Only defined with CONFIG_COMPAT */
+#define MON_IOCX_GET32		_IOW(MON_IOC_MAGIC, 6, struct mon_bin_get32)
+#define MON_IOCX_MFETCH32	_IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch32)
+#define MON_IOCX_GETX32		_IOW(MON_IOC_MAGIC, 10, struct mon_bin_get32)
+
+struct mon_bin_get32 {
+	__u32 hdr32;
+	__u32 data32;
+	__u32 alloc32;
+};
+
+struct mon_bin_mfetch32 {
+	__u32 offvec32;
+	__u32 nfetch32;
+	__u32 nflush32;
+};
+
+/* Data format */
+
+/*
+ * Defined by USB 2.0 clause 9.3, table 9.2.
+ */
+#define MON_USB_SETUP_LEN  8
+
+/*
+ * The per-event API header (2 per URB).
+ *
+ * This structure is seen in userland as defined by the documentation.
+ */
+struct mon_bin_hdr {
+	__u64 id;	/* URB ID - from submission to callback */
+	__u8 type;	/* Same as in text API; extensible. */
+	__u8 xfer_type;	/* ISO, Intr, Control, Bulk */
+	__u8 epnum;	/* Endpoint number and transfer direction */
+	__u8 devnum;	/* Device address */
+	__u16 busnum;	/* Bus number */
+	__s8 flag_setup;
+	__s8 flag_data;
+	__s64 ts_sec;	/* ktime_get_real_ts64 */
+	__s32 ts_usec;	/* ktime_get_real_ts64 */
+	__s32 status;
+	__u32 len_urb;	/* Length of data (submitted or actual) */
+	__u32 len_cap;	/* Delivered length */
+	union {
+		__u8 setup[MON_USB_SETUP_LEN];	/* Only for Control S-type */
+		struct iso_rec {
+			__s32 error_count;
+			__s32 numdesc;
+		} iso;
+	} s;
+	__s32 interval;
+	__s32 start_frame;
+	__u32 xfer_flags;
+	__u32 ndesc;	/* Actual number of ISO descriptors */
+};
+
+#endif /* __UAPI_LINUX_USB_MON_H */
-- 
2.27.0

Re: [PATCH v3 2/2] usbmon: expose the usbmon structures and constants as an UAPI header.

[Greg KH]

On Mon, Jul 06, 2020 at 02:10:14PM +0100, Diego Elio Pettenò wrote:
> Previously any application wanting to implement the usbmon binary
> interfaces needed to re-declare the structures and constants, leading to
> structure duplication and confusion over whether these structures fall into
> the system call exception or not.

There is no "confusion", like Pete said, just make the text here say
something like "add proper uapi file for the usbmon structures" and
expand on that.

thanks,

greg k-h

Re: [PATCH v3 1/2] Remove documentation line that adds nothing and sounds condescending.

[Greg KH]

On Mon, Jul 06, 2020 at 02:10:13PM +0100, Diego Elio Pettenò wrote:
> Signed-off-by: Diego Elio Pettenò <flameeyes@flameeyes.com>

I can't take patches with no changelog text in them at all, sorry.

[PATCH v4 1/2] Remove documentation line that adds nothing and sounds condescending.

[Diego Elio Pettenò]

"Easy" is a subjective term, and while not adding more context, it can push
away some of the readers.

Signed-off-by: Diego Elio Pettenò <flameeyes@flameeyes.com>
---
 Documentation/usb/usbmon.rst | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/usb/usbmon.rst b/Documentation/usb/usbmon.rst
index b0bd51080799..e9ec7e40b3bf 100644
--- a/Documentation/usb/usbmon.rst
+++ b/Documentation/usb/usbmon.rst
@@ -346,8 +346,6 @@ be polled with select(2) and poll(2). But lseek(2) does not work.
 
 * Memory-mapped access of the kernel buffer for the binary API
 
-The basic idea is simple:
-
 To prepare, map the buffer by getting the current size, then using mmap(2).
 Then, execute a loop similar to the one written in pseudo-code below::
 
-- 
2.27.0

[PATCH v4 2/2] usbmon: expose the usbmon structures and constants as an UAPI header.

[Diego Elio Pettenò]

This allows applications to use the usbmon ioctl() without declaring the
constants.

Update the documentation to reflect the new header.

Cc: linux-usb@vger.kernel.org
Cc: Pete Zaitcev <zaitcev@redhat.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Kris Katterjohn <katterjohn@gmail.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Signed-off-by: Diego Elio Pettenò <flameeyes@flameeyes.com>
---
 Documentation/usb/usbmon.rst |  70 ++++++++++++-----------
 drivers/usb/mon/mon_bin.c    |  92 +-----------------------------
 include/uapi/linux/usb/mon.h | 107 +++++++++++++++++++++++++++++++++++
 3 files changed, 145 insertions(+), 124 deletions(-)
 create mode 100644 include/uapi/linux/usb/mon.h

diff --git a/Documentation/usb/usbmon.rst b/Documentation/usb/usbmon.rst
index e9ec7e40b3bf..31c14b38fd03 100644
--- a/Documentation/usb/usbmon.rst
+++ b/Documentation/usb/usbmon.rst
@@ -211,35 +211,37 @@ Bulk wrapper to a storage device at address 5::
 Raw binary format and API
 =========================
 
-The overall architecture of the API is about the same as the one above,
-only the events are delivered in binary format. Each event is sent in
-the following structure (its name is made up, so that we can refer to it)::
-
-  struct usbmon_packet {
-	u64 id;			/*  0: URB ID - from submission to callback */
-	unsigned char type;	/*  8: Same as text; extensible. */
-	unsigned char xfer_type; /*    ISO (0), Intr, Control, Bulk (3) */
-	unsigned char epnum;	/*     Endpoint number and transfer direction */
-	unsigned char devnum;	/*     Device address */
-	u16 busnum;		/* 12: Bus number */
-	char flag_setup;	/* 14: Same as text */
-	char flag_data;		/* 15: Same as text; Binary zero is OK. */
-	s64 ts_sec;		/* 16: gettimeofday */
-	s32 ts_usec;		/* 24: gettimeofday */
-	int status;		/* 28: */
-	unsigned int length;	/* 32: Length of data (submitted or actual) */
-	unsigned int len_cap;	/* 36: Delivered length */
+The overall architecture of the API is about the same as the one above, only the
+events are delivered in binary format. The structures and constants are defined
+in include/uapi/linux/usb/mon.h.
+
+Each event is sent in the following structure::
+
+  struct mon_bin_hdr {
+	__u64 id;		/*  0: URB ID - from submission to callback */
+	__u8 type;		/*  8: Same as text; extensible. */
+	__u8 xfer_type;		/*    ISO (0), Intr, Control, Bulk (3) */
+	__u8 epnum;		/*     Endpoint number and transfer direction */
+	__u8 devnum;		/*     Device address */
+	__u16 busnum;		/* 12: Bus number */
+	__s8 flag_setup;	/* 14: Same as text */
+	__s8 flag_data;		/* 15: Same as text; Binary zero is OK. */
+	__s64 ts_sec;		/* 16: gettimeofday */
+	__s32 ts_usec;		/* 24: gettimeofday */
+	__s32 status;		/* 28: */
+	__u32 length;		/* 32: Length of data (submitted or actual) */
+	__u32 len_cap;		/* 36: Delivered length */
 	union {			/* 40: */
-		unsigned char setup[SETUP_LEN];	/* Only for Control S-type */
+		__u8 setup[MON_USB_SETUP_LEN];	/* Only for Control S-type */
 		struct iso_rec {		/* Only for ISO */
-			int error_count;
-			int numdesc;
+			__s32 error_count;
+			__s32 numdesc;
 		} iso;
 	} s;
-	int interval;		/* 48: Only for Interrupt and ISO */
-	int start_frame;	/* 52: For ISO */
-	unsigned int xfer_flags; /* 56: copy of URB's transfer_flags */
-	unsigned int ndesc;	/* 60: Actual number of ISO descriptors */
+	__s32 interval;		/* 48: Only for Interrupt and ISO */
+	__s32 start_frame;	/* 52: For ISO */
+	__u32 xfer_flags;	/* 56: copy of URB's transfer_flags */
+	__u32 ndesc;		/* 60: Actual number of ISO descriptors */
   };				/* 64 total length */
 
 These events can be received from a character device by reading with read(2),
@@ -267,8 +269,8 @@ no events are available.
 The argument is a pointer to the following structure::
 
   struct mon_bin_stats {
-	u32 queued;
-	u32 dropped;
+	__u32 queued;
+	__u32 dropped;
   };
 
 The member "queued" refers to the number of events currently queued in the
@@ -296,9 +298,9 @@ then return the first event. The argument is a pointer to the following
 structure::
 
   struct mon_get_arg {
-	struct usbmon_packet *hdr;
+	struct mon_bin_hdr *hdr;
 	void *data;
-	size_t alloc;		/* Length of data (can be zero) */
+	__kernel_size_t alloc;		/* Length of data (can be zero) */
   };
 
 Before the call, hdr, data, and alloc should be filled. Upon return, the area
@@ -313,9 +315,9 @@ This ioctl is primarily used when the application accesses the buffer
 with mmap(2). Its argument is a pointer to the following structure::
 
   struct mon_mfetch_arg {
-	uint32_t *offvec;	/* Vector of events fetched */
-	uint32_t nfetch;	/* Number of events to fetch (out: fetched) */
-	uint32_t nflush;	/* Number of events to flush */
+	__u32 *offvec;	/* Vector of events fetched */
+	__u32 nfetch;	/* Number of events to fetch (out: fetched) */
+	__u32 nflush;	/* Number of events to flush */
   };
 
 The ioctl operates in 3 stages.
@@ -350,7 +352,7 @@ To prepare, map the buffer by getting the current size, then using mmap(2).
 Then, execute a loop similar to the one written in pseudo-code below::
 
    struct mon_mfetch_arg fetch;
-   struct usbmon_packet *hdr;
+   struct mon_bin_hdr *hdr;
    int nflush = 0;
    for (;;) {
       fetch.offvec = vec; // Has N 32-bit words
@@ -359,7 +361,7 @@ Then, execute a loop similar to the one written in pseudo-code below::
       ioctl(fd, MON_IOCX_MFETCH, &fetch);   // Process errors, too
       nflush = fetch.nfetch;       // This many packets to flush when done
       for (i = 0; i < nflush; i++) {
-         hdr = (struct ubsmon_packet *) &mmap_area[vec[i]];
+         hdr = (struct mon_bin_hdr *) &mmap_area[vec[i]];
          if (hdr->type == '@')     // Filler packet
             continue;
          caddr_t data = &mmap_area[vec[i]] + 64;
diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index f48a23adbc35..e1a7e69a3b0c 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -23,34 +23,10 @@
 #include <linux/time64.h>
 
 #include <linux/uaccess.h>
+#include <linux/usb/mon.h>
 
 #include "usb_mon.h"
 
-/*
- * Defined by USB 2.0 clause 9.3, table 9.2.
- */
-#define SETUP_LEN  8
-
-/* ioctl macros */
-#define MON_IOC_MAGIC 0x92
-
-#define MON_IOCQ_URB_LEN _IO(MON_IOC_MAGIC, 1)
-/* #2 used to be MON_IOCX_URB, removed before it got into Linus tree */
-#define MON_IOCG_STATS _IOR(MON_IOC_MAGIC, 3, struct mon_bin_stats)
-#define MON_IOCT_RING_SIZE _IO(MON_IOC_MAGIC, 4)
-#define MON_IOCQ_RING_SIZE _IO(MON_IOC_MAGIC, 5)
-#define MON_IOCX_GET   _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get)
-#define MON_IOCX_MFETCH _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch)
-#define MON_IOCH_MFLUSH _IO(MON_IOC_MAGIC, 8)
-/* #9 was MON_IOCT_SETAPI */
-#define MON_IOCX_GETX   _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get)
-
-#ifdef CONFIG_COMPAT
-#define MON_IOCX_GET32 _IOW(MON_IOC_MAGIC, 6, struct mon_bin_get32)
-#define MON_IOCX_MFETCH32 _IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch32)
-#define MON_IOCX_GETX32   _IOW(MON_IOC_MAGIC, 10, struct mon_bin_get32)
-#endif
-
 /*
  * Some architectures have enormous basic pages (16KB for ia64, 64KB for ppc).
  * But it's all right. Just use a simple way to make sure the chunk is never
@@ -81,38 +57,6 @@
 #define BUFF_DFL   CHUNK_ALIGN(300*1024)
 #define BUFF_MIN     CHUNK_ALIGN(8*1024)
 
-/*
- * The per-event API header (2 per URB).
- *
- * This structure is seen in userland as defined by the documentation.
- */
-struct mon_bin_hdr {
-	u64 id;			/* URB ID - from submission to callback */
-	unsigned char type;	/* Same as in text API; extensible. */
-	unsigned char xfer_type;	/* ISO, Intr, Control, Bulk */
-	unsigned char epnum;	/* Endpoint number and transfer direction */
-	unsigned char devnum;	/* Device address */
-	unsigned short busnum;	/* Bus number */
-	char flag_setup;
-	char flag_data;
-	s64 ts_sec;		/* ktime_get_real_ts64 */
-	s32 ts_usec;		/* ktime_get_real_ts64 */
-	int status;
-	unsigned int len_urb;	/* Length of data (submitted or actual) */
-	unsigned int len_cap;	/* Delivered length */
-	union {
-		unsigned char setup[SETUP_LEN];	/* Only for Control S-type */
-		struct iso_rec {
-			int error_count;
-			int numdesc;
-		} iso;
-	} s;
-	int interval;
-	int start_frame;
-	unsigned int xfer_flags;
-	unsigned int ndesc;	/* Actual number of ISO descriptors */
-};
-
 /*
  * ISO vector, packed into the head of data stream.
  * This has to take 16 bytes to make sure that the end of buffer
@@ -125,38 +69,6 @@ struct mon_bin_isodesc {
 	u32 _pad;
 };
 
-/* per file statistic */
-struct mon_bin_stats {
-	u32 queued;
-	u32 dropped;
-};
-
-struct mon_bin_get {
-	struct mon_bin_hdr __user *hdr;	/* Can be 48 bytes or 64. */
-	void __user *data;
-	size_t alloc;		/* Length of data (can be zero) */
-};
-
-struct mon_bin_mfetch {
-	u32 __user *offvec;	/* Vector of events fetched */
-	u32 nfetch;		/* Number of events to fetch (out: fetched) */
-	u32 nflush;		/* Number of events to flush */
-};
-
-#ifdef CONFIG_COMPAT
-struct mon_bin_get32 {
-	u32 hdr32;
-	u32 data32;
-	u32 alloc32;
-};
-
-struct mon_bin_mfetch32 {
-        u32 offvec32;
-        u32 nfetch32;
-        u32 nflush32;
-};
-#endif
-
 /* Having these two values same prevents wrapping of the mon_bin_hdr */
 #define PKT_ALIGN   64
 #define PKT_SIZE    64
@@ -396,7 +308,7 @@ static inline char mon_bin_get_setup(unsigned char *setupb,
 
 	if (urb->setup_packet == NULL)
 		return 'Z';
-	memcpy(setupb, urb->setup_packet, SETUP_LEN);
+	memcpy(setupb, urb->setup_packet, MON_USB_SETUP_LEN);
 	return 0;
 }
 
diff --git a/include/uapi/linux/usb/mon.h b/include/uapi/linux/usb/mon.h
new file mode 100644
index 000000000000..3557fe6a93e6
--- /dev/null
+++ b/include/uapi/linux/usb/mon.h
@@ -0,0 +1,107 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * USB Monitoring (usbmon) definitions
+ *
+ * See documentation in Documentation/usb/usbmon.rst.
+ *
+ * Copyright (C) 2006 Paolo Abeni (paolo.abeni@email.it)
+ * Copyright (C) 2006,2007 Pete Zaitcev (zaitcev@redhat.com)
+ */
+
+#ifndef __UAPI_LINUX_USB_MON_H
+#define __UAPI_LINUX_USB_MON_H
+
+#include <linux/types.h>
+
+/* ioctl macros */
+#define MON_IOC_MAGIC 0x92
+
+#define MON_IOCQ_URB_LEN	_IO(MON_IOC_MAGIC, 1)
+/* #2 used to be MON_IOCX_URB, removed before it got into Linus tree */
+#define MON_IOCG_STATS		_IOR(MON_IOC_MAGIC, 3, struct mon_bin_stats)
+#define MON_IOCT_RING_SIZE	_IO(MON_IOC_MAGIC, 4)
+#define MON_IOCQ_RING_SIZE	_IO(MON_IOC_MAGIC, 5)
+#define MON_IOCX_GET		_IOW(MON_IOC_MAGIC, 6, struct mon_bin_get)
+#define MON_IOCX_MFETCH		_IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch)
+#define MON_IOCH_MFLUSH		_IO(MON_IOC_MAGIC, 8)
+/* #9 was MON_IOCT_SETAPI */
+#define MON_IOCX_GETX		_IOW(MON_IOC_MAGIC, 10, struct mon_bin_get)
+
+/* ioctl structures */
+
+/* per file statistic */
+struct mon_bin_stats {
+	__u32 queued;
+	__u32 dropped;
+};
+
+struct mon_bin_get {
+	struct mon_bin_hdr __user *hdr;	/* Can be 48 bytes or 64. */
+	void __user *data;
+	size_t alloc;		/* Length of data (can be zero) */
+};
+
+struct mon_bin_mfetch {
+	__u32 __user *offvec;	/* Vector of events fetched */
+	__u32 nfetch;		/* Number of events to fetch (out: fetched) */
+	__u32 nflush;		/* Number of events to flush */
+};
+
+
+/* Only defined with CONFIG_COMPAT */
+#define MON_IOCX_GET32		_IOW(MON_IOC_MAGIC, 6, struct mon_bin_get32)
+#define MON_IOCX_MFETCH32	_IOWR(MON_IOC_MAGIC, 7, struct mon_bin_mfetch32)
+#define MON_IOCX_GETX32		_IOW(MON_IOC_MAGIC, 10, struct mon_bin_get32)
+
+struct mon_bin_get32 {
+	__u32 hdr32;
+	__u32 data32;
+	__u32 alloc32;
+};
+
+struct mon_bin_mfetch32 {
+	__u32 offvec32;
+	__u32 nfetch32;
+	__u32 nflush32;
+};
+
+/* Data format */
+
+/*
+ * Defined by USB 2.0 clause 9.3, table 9.2.
+ */
+#define MON_USB_SETUP_LEN  8
+
+/*
+ * The per-event API header (2 per URB).
+ *
+ * This structure is seen in userland as defined by the documentation.
+ */
+struct mon_bin_hdr {
+	__u64 id;	/* URB ID - from submission to callback */
+	__u8 type;	/* Same as in text API; extensible. */
+	__u8 xfer_type;	/* ISO, Intr, Control, Bulk */
+	__u8 epnum;	/* Endpoint number and transfer direction */
+	__u8 devnum;	/* Device address */
+	__u16 busnum;	/* Bus number */
+	__s8 flag_setup;
+	__s8 flag_data;
+	__s64 ts_sec;	/* ktime_get_real_ts64 */
+	__s32 ts_usec;	/* ktime_get_real_ts64 */
+	__s32 status;
+	__u32 len_urb;	/* Length of data (submitted or actual) */
+	__u32 len_cap;	/* Delivered length */
+	union {
+		__u8 setup[MON_USB_SETUP_LEN];	/* Only for Control S-type */
+		struct iso_rec {
+			__s32 error_count;
+			__s32 numdesc;
+		} iso;
+	} s;
+	__s32 interval;
+	__s32 start_frame;
+	__u32 xfer_flags;
+	__u32 ndesc;	/* Actual number of ISO descriptors */
+};
+
+#endif /* __UAPI_LINUX_USB_MON_H */
-- 
2.27.0

Re: [PATCH v4 2/2] usbmon: expose the usbmon structures and constants as an UAPI header.

[Greg KH]

On Mon, Jul 06, 2020 at 11:44:15PM +0100, Diego Elio Pettenò wrote:
> This allows applications to use the usbmon ioctl() without declaring the
> constants.
> 
> Update the documentation to reflect the new header.
> 
> Cc: linux-usb@vger.kernel.org
> Cc: Pete Zaitcev <zaitcev@redhat.com>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Kris Katterjohn <katterjohn@gmail.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Signed-off-by: Diego Elio Pettenò <flameeyes@flameeyes.com>
> ---
>  Documentation/usb/usbmon.rst |  70 ++++++++++++-----------
>  drivers/usb/mon/mon_bin.c    |  92 +-----------------------------
>  include/uapi/linux/usb/mon.h | 107 +++++++++++++++++++++++++++++++++++
>  3 files changed, 145 insertions(+), 124 deletions(-)
>  create mode 100644 include/uapi/linux/usb/mon.h

What changed from previous versions?  Always put that below the --- line
like the documentation asks you to.   Please do that when you resend
this series.

thanks,

greg k-h

Re: [PATCH v4 2/2] usbmon: expose the usbmon structures and constants as an UAPI header.

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

Hi "Diego,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v5.8-rc4 next-20200710]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Diego-Elio-Petten/Remove-documentation-line-that-adds-nothing-and-sounds-condescending/20200707-074450
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-randconfig-a011-20200710 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from <command-line>:32:
>> ./usr/include/linux/usb/mon.h:41:2: error: unknown type name 'size_t'
      41 |  size_t alloc;  /* Length of data (can be zero) */
         |  ^~~~~~

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34303 bytes --]

Re: [PATCH v4 2/2] usbmon: expose the usbmon structures and constants as an UAPI header.

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1287 bytes --]

Hi "Diego,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v5.8-rc4 next-20200710]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Diego-Elio-Petten/Remove-documentation-line-that-adds-nothing-and-sounds-condescending/20200707-074450
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-randconfig-a011-20200710 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from <command-line>:32:
>> ./usr/include/linux/usb/mon.h:41:2: error: unknown type name 'size_t'
      41 |  size_t alloc;  /* Length of data (can be zero) */
         |  ^~~~~~

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 34303 bytes --]

Re: [PATCH v4 1/2] Remove documentation line that adds nothing and sounds condescending.

[Greg KH]

On Mon, Jul 06, 2020 at 11:44:14PM +0100, Diego Elio Pettenò wrote:
> "Easy" is a subjective term, and while not adding more context, it can push
> away some of the readers.
> 
> Signed-off-by: Diego Elio Pettenò <flameeyes@flameeyes.com>

Nit, your subject line needs "USB: usbmon:" in it, and please cc: the
proper people that get_maintainer.pl asks you to.

thanks,

greg k-h