Re: [PATCH v4 1/1] usb: gadget: add raw-gadget interface - Greg Kroah-Hartman

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Andrey Konovalov <andreyknvl@google.com>
Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	Alan Stern <stern@rowland.harvard.edu>,
	Jonathan Corbet <corbet@lwn.net>, Felipe Balbi <balbi@kernel.org>,
	Dmitry Vyukov <dvyukov@google.com>,
	Alexander Potapenko <glider@google.com>,
	Marco Elver <elver@google.com>
Subject: Re: [PATCH v4 1/1] usb: gadget: add raw-gadget interface
Date: Sat, 11 Jan 2020 20:31:56 +0100	[thread overview]
Message-ID: <20200111193156.GC435222@kroah.com> (raw)
In-Reply-To: <6cd46f8512dc12e20667c2b02d487591868cb20f.1576697098.git.andreyknvl@google.com>

On Wed, Dec 18, 2019 at 08:26:57PM +0100, Andrey Konovalov wrote:
> 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.
> 
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

This looks good to me, with the one minor problem below that you can fix
with a follow-on patch.  This should probably go through Felipe's tree
though, so I'll wait for him to review the gadget side and then queue it
up in his tree:

> +/* The type of event fetched with the USB_RAW_IOCTL_EVENT_FETCH ioctl. */
> +enum usb_raw_event_type {
> +	USB_RAW_EVENT_INVALID,
> +
> +	/* This event is queued when the driver has bound to a UDC. */
> +	USB_RAW_EVENT_CONNECT,
> +
> +	/* This event is queued when a new control request arrived to ep0. */
> +	USB_RAW_EVENT_CONTROL,
> +
> +	/* The list might grow in the future. */
> +};

You have to manually specify the enum values in the .h file for all
entries in order to assure that both userspace and the kernel will be in
sync with the same values.  I think that's documented in the "how to
write an ioctl interface" document that is somewhere in
Documentation/...

thanks,

greg k-h