Re: [PATCH v5 1/1] usb: gadget: add raw-gadget interface

[Andrey Konovalov]

On Fri, Jan 31, 2020 at 2:42 PM Felipe Balbi <balbi@xxxxxxxxxx> wrote:
>
>
> Hi,
>
> Andrey Konovalov <andreyknvl@xxxxxxxxxx> writes:
> > USB Raw Gadget is a kernel module that provides a userspace interface for
> > the USB Gadget subsystem. Essentially it allows to emulate USB devices
> > from userspace. Enabled with CONFIG_USB_RAW_GADGET. Raw Gadget is
> > currently a strictly debugging feature and shouldn't be used in
> > production.
> >
> > Raw Gadget is similar to GadgetFS, but provides a more low-level and
> > direct access to the USB Gadget layer for the userspace. The key
> > differences are:
> >
> > 1. Every USB request is passed to the userspace to get a response, while
> >    GadgetFS responds to some USB requests internally based on the provided
> >    descriptors. However note, that the UDC driver might respond to some
> >    requests on its own and never forward them to the Gadget layer.
> >
> > 2. GadgetFS performs some sanity checks on the provided USB descriptors,
> >    while Raw Gadget allows you to provide arbitrary data as responses to
> >    USB requests.
> >
> > 3. Raw Gadget provides a way to select a UDC device/driver to bind to,
> >    while GadgetFS currently binds to the first available UDC.
> >
> > 4. Raw Gadget uses predictable endpoint names (handles) across different
> >    UDCs (as long as UDCs have enough endpoints of each required transfer
> >    type).
> >
> > 5. Raw Gadget has ioctl-based interface instead of a filesystem-based one.
> >
> > Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Andrey Konovalov <andreyknvl@xxxxxxxxxx>
> > ---
> >
> > Greg, I've assumed your LGTM meant that I can add a Reviewed-by from you.
> >
> > Felipe, looking forward to your review, thanks!
> >
> >  Documentation/usb/index.rst            |    1 +
> >  Documentation/usb/raw-gadget.rst       |   59 ++
> >  drivers/usb/gadget/legacy/Kconfig      |   11 +
> >  drivers/usb/gadget/legacy/Makefile     |    1 +
> >  drivers/usb/gadget/legacy/raw_gadget.c | 1068 ++++++++++++++++++++++++
> >  include/uapi/linux/usb/raw_gadget.h    |  167 ++++
> >  6 files changed, 1307 insertions(+)
> >  create mode 100644 Documentation/usb/raw-gadget.rst
> >  create mode 100644 drivers/usb/gadget/legacy/raw_gadget.c
> >  create mode 100644 include/uapi/linux/usb/raw_gadget.h
> >
> > diff --git a/Documentation/usb/index.rst b/Documentation/usb/index.rst
> > index e55386a4abfb..90310e2a0c1f 100644
> > --- a/Documentation/usb/index.rst
> > +++ b/Documentation/usb/index.rst
> > @@ -22,6 +22,7 @@ USB support
> >      misc_usbsevseg
> >      mtouchusb
> >      ohci
> > +    raw-gadget
> >      rio
> >      usbip_protocol
> >      usbmon
> > diff --git a/Documentation/usb/raw-gadget.rst b/Documentation/usb/raw-gadget.rst
> > new file mode 100644
> > index 000000000000..cbedf5451ed3
> > --- /dev/null
> > +++ b/Documentation/usb/raw-gadget.rst
> > @@ -0,0 +1,59 @@
> > +==============
> > +USB Raw Gadget
> > +==============
> > +
> > +USB Raw Gadget is a kernel module that provides a userspace interface for
> > +the USB Gadget subsystem. Essentially it allows to emulate USB devices
> > +from userspace. Enabled with CONFIG_USB_RAW_GADGET. Raw Gadget is
> > +currently a strictly debugging feature and shouldn't be used in
> > +production, use GadgetFS instead.
> > +
> > +Comparison to GadgetFS
> > +~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +Raw Gadget is similar to GadgetFS, but provides a more low-level and
> > +direct access to the USB Gadget layer for the userspace. The key
> > +differences are:
> > +
> > +1. Every USB request is passed to the userspace to get a response, while
> > +   GadgetFS responds to some USB requests internally based on the provided
> > +   descriptors. However note, that the UDC driver might respond to some
> > +   requests on its own and never forward them to the Gadget layer.
> > +
> > +2. GadgetFS performs some sanity checks on the provided USB descriptors,
> > +   while Raw Gadget allows you to provide arbitrary data as responses to
> > +   USB requests.
> > +
> > +3. Raw Gadget provides a way to select a UDC device/driver to bind to,
> > +   while GadgetFS currently binds to the first available UDC.
> > +
> > +4. Raw Gadget uses predictable endpoint names (handles) across different
> > +   UDCs (as long as UDCs have enough endpoints of each required transfer
> > +   type).
> > +
> > +5. Raw Gadget has ioctl-based interface instead of a filesystem-based one.
> > +
> > +Userspace interface
> > +~~~~~~~~~~~~~~~~~~~
> > +
> > +To create a Raw Gadget instance open /dev/raw-gadget. Multiple raw-gadget
> > +instances (bound to different UDCs) can be used at the same time. The
> > +interaction with the opened file happens through the ioctl() calls, see
> > +comments in include/uapi/linux/usb/raw_gadget.h for details.
> > +
> > +The typical usage of Raw Gadget looks like:
> > +
> > +1. Open Raw Gadget instance via /dev/raw-gadget.
> > +2. Initialize the instance via USB_RAW_IOCTL_INIT.
> > +3. Launch the instance with USB_RAW_IOCTL_RUN.
> > +4. In a loop issue USB_RAW_IOCTL_EVENT_FETCH calls to receive events from
> > +   Raw Gadget and react to those depending on what kind of USB device
> > +   needs to be emulated.
> > +
> > +Potential future improvements
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +- Implement ioctl's for setting/clearing halt status on endpoints.
> > +
> > +- Reporting more events (suspend, resume, etc.) through
> > +  USB_RAW_IOCTL_EVENT_FETCH.
> > diff --git a/drivers/usb/gadget/legacy/Kconfig b/drivers/usb/gadget/legacy/Kconfig
> > index 119a4e47681f..55e495f5d103 100644
> > --- a/drivers/usb/gadget/legacy/Kconfig
> > +++ b/drivers/usb/gadget/legacy/Kconfig
> > @@ -489,3 +489,14 @@ config USB_G_WEBCAM
> >
> >         Say "y" to link the driver statically, or "m" to build a
> >         dynamically linked module called "g_webcam".
> > +
> > +config USB_RAW_GADGET
> > +     tristate "USB Raw Gadget"
> > +     help
> > +       USB Raw Gadget is a kernel module that provides a userspace interface
> > +       for the USB Gadget subsystem. Essentially it allows to emulate USB
> > +       devices from userspace. See Documentation/usb/raw-gadget.rst for
> > +       details.
> > +
> > +       Say "y" to link the driver statically, or "m" to build a
> > +       dynamically linked module called "raw_gadget".
> > diff --git a/drivers/usb/gadget/legacy/Makefile b/drivers/usb/gadget/legacy/Makefile
> > index abd0c3e66a05..4d864bf82799 100644
> > --- a/drivers/usb/gadget/legacy/Makefile
> > +++ b/drivers/usb/gadget/legacy/Makefile
> > @@ -43,3 +43,4 @@ obj-$(CONFIG_USB_G_WEBCAM)  += g_webcam.o
> >  obj-$(CONFIG_USB_G_NCM)              += g_ncm.o
> >  obj-$(CONFIG_USB_G_ACM_MS)   += g_acm_ms.o
> >  obj-$(CONFIG_USB_GADGET_TARGET)      += tcm_usb_gadget.o
> > +obj-$(CONFIG_USB_RAW_GADGET) += raw_gadget.o
> > diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
> > new file mode 100644
> > index 000000000000..51796af48069
> > --- /dev/null
> > +++ b/drivers/usb/gadget/legacy/raw_gadget.c
> > @@ -0,0 +1,1068 @@
> > +// SPDX-License-Identifier: GPL-2.0
>
> V2 only

Like this: SPDX-License-Identifier: GPL-2.0 only ?

>
> > +/*
> > + * USB Raw Gadget driver.
> > + * See Documentation/usb/raw-gadget.rst for more details.
> > + *
> > + * Andrey Konovalov <andreyknvl@xxxxxxxxx>
> > + */
> > +
> > +#include <linux/compiler.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/kref.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/module.h>
> > +#include <linux/semaphore.h>
> > +#include <linux/sched.h>
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/wait.h>
> > +
> > +#include <linux/usb.h>
> > +#include <linux/usb/ch9.h>
> > +#include <linux/usb/ch11.h>
> > +#include <linux/usb/gadget.h>
> > +
> > +#include <uapi/linux/usb/raw_gadget.h>
> > +
> > +#define      DRIVER_DESC "USB Raw Gadget"
> > +#define DRIVER_NAME "raw-gadget"
> > +
> > +MODULE_DESCRIPTION(DRIVER_DESC);
> > +MODULE_AUTHOR("Andrey Konovalov");
> > +MODULE_LICENSE("GPL");
>
> v2+. Care to fix?

MODULE_LICENSE("GPL-v2+")?

>
> > +
> > +/*----------------------------------------------------------------------*/
> > +
> > +#define RAW_EVENT_QUEUE_SIZE 128
> > +
> > +struct raw_event_queue {
> > +     /* See the comment in raw_event_queue_fetch() for locking details. */
> > +     spinlock_t              lock;
> > +     struct semaphore        sema;
> > +     struct usb_raw_event    *events[RAW_EVENT_QUEUE_SIZE];
> > +     int                     size;
> > +};
> > +
> > +static void raw_event_queue_init(struct raw_event_queue *queue)
> > +{
> > +     spin_lock_init(&queue->lock);
> > +     sema_init(&queue->sema, 0);
> > +     queue->size = 0;
> > +}
> > +
> > +static int raw_event_queue_add(struct raw_event_queue *queue,
> > +     enum usb_raw_event_type type, size_t length, const void *data)
> > +{
> > +     unsigned long flags;
> > +     struct usb_raw_event *event;
> > +
> > +     spin_lock_irqsave(&queue->lock, flags);
> > +     if (WARN_ON(queue->size >= RAW_EVENT_QUEUE_SIZE)) {
> > +             spin_unlock_irqrestore(&queue->lock, flags);
> > +             return -ENOMEM;
> > +     }
> > +     event = kmalloc(sizeof(*event) + length, GFP_ATOMIC);
>
> I would very much prefer dropping GFP_ATOMIC here. Must you have this
> allocation under a spinlock?

The issue here is not the spinlock, but that this might be called in
interrupt context. The number of atomic allocations here is restricted
by 128, and we can reduce the limit even further (until some point in
the future when and if we'll report more different events). Another
option would be to preallocate the required number of objects
(although we don't know the required size in advance, so we'll waste
some memory) and use those. What would you prefer?

Thank you for the review!