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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Wed, Dec 11, 2019 at 07:02:41PM +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.

Looks good to me, only minor comments below.

> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
>  Documentation/usb/index.rst         |    1 +
>  Documentation/usb/raw-gadget.rst    |   59 ++
>  drivers/usb/gadget/legacy/Kconfig   |    8 +
>  drivers/usb/gadget/legacy/Makefile  |    1 +
>  drivers/usb/gadget/legacy/raw.c     | 1070 +++++++++++++++++++++++++++
>  include/uapi/linux/usb/raw_gadget.h |  167 +++++
>  6 files changed, 1306 insertions(+)
>  create mode 100644 Documentation/usb/raw-gadget.rst
>  create mode 100644 drivers/usb/gadget/legacy/raw.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..8c947edf7515 100644
> --- a/drivers/usb/gadget/legacy/Kconfig
> +++ b/drivers/usb/gadget/legacy/Kconfig
> @@ -489,3 +489,11 @@ 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.

No help text saying what the module name would be if selected as one?


> diff --git a/drivers/usb/gadget/legacy/Makefile b/drivers/usb/gadget/legacy/Makefile
> index abd0c3e66a05..799feefeee58 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.o

raw.ko?  I think we already have a raw.ko in the kernel, shouldn't this
be usb_raw_gadget.ko or something?

Yeah, we do, look at drivers/char/raw.c.  This needs to be changed,
I can't take it as-is otherwise the builders will croak on it.

> diff --git a/drivers/usb/gadget/legacy/raw.c b/drivers/usb/gadget/legacy/raw.c
> new file mode 100644
> index 000000000000..fcfa2ebc103e
> --- /dev/null
> +++ b/drivers/usb/gadget/legacy/raw.c
> @@ -0,0 +1,1070 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * USB Raw Gadget driver.
> + * See Documentation/usb/raw-gadget.rst for more details.
> + *
> + * Andrey Konovalov <andreyknvl@gmail.com>
> + */
> +
> +#define pr_fmt(fmt) "raw: %s: " fmt, __func__

"raw gadget"?

This a driver, why do you ever need to do a pr_* call and not a dev_*
instead?

> +
> +#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"

You only use this in one place.  You can use it in the definition of the
misc driver name also, right?


> +
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_AUTHOR("Andrey Konovalov");
> +MODULE_LICENSE("GPL");
> +
> +/*----------------------------------------------------------------------*/
> +
> +#define RAW_EVENT_QUEUE_SIZE	128
> +
> +struct raw_event_queue {
> +	spinlock_t		lock;
> +	struct semaphore	sema;

Two locks for the same structure?  That feels "wrong".  Why is this?
document the heck out of it if it is required please.

> +	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 (queue->size >= RAW_EVENT_QUEUE_SIZE) {
> +		pr_err("too many events\n");

What can someone do with this error?

dev_err() to show exactly where it came from?

> +		spin_unlock_irqrestore(&queue->lock, flags);
> +		return -ENOMEM;
> +	}
> +	event = kmalloc(sizeof(*event) + length, GFP_ATOMIC);
> +	if (!event) {
> +		spin_unlock_irqrestore(&queue->lock, flags);
> +		return -ENOMEM;
> +	}
> +	event->type = type;
> +	event->length = length;
> +	if (event->length)
> +		memcpy(&event->data[0], data, length);
> +	queue->events[queue->size] = event;
> +	queue->size++;
> +	up(&queue->sema);
> +	spin_unlock_irqrestore(&queue->lock, flags);
> +	return 0;
> +}
> +
> +static struct usb_raw_event *raw_event_queue_fetch(
> +				struct raw_event_queue *queue)
> +{
> +	unsigned long flags;
> +	struct usb_raw_event *event;
> +
> +	if (down_interruptible(&queue->sema))
> +		return NULL;
> +	spin_lock_irqsave(&queue->lock, flags);

You just locked twice?

> +	if (WARN_ON(!queue->size))
> +		return NULL;

It's as if you want to trigger syzbot :)

Just log it and return an error.

> +	event = queue->events[0];
> +	queue->size--;
> +	memmove(&queue->events[0], &queue->events[1],
> +			queue->size * sizeof(queue->events[0]));
> +	spin_unlock_irqrestore(&queue->lock, flags);
> +	return event;
> +}
> +
> +static void raw_event_queue_destroy(struct raw_event_queue *queue)
> +{
> +	int i;
> +
> +	for (i = 0; i < queue->size; i++)
> +		kfree(queue->events[i]);
> +	queue->size = 0;

No locking needed?

> +}
> +
> +/*----------------------------------------------------------------------*/
> +
> +struct raw_dev;
> +
> +#define USB_RAW_MAX_ENDPOINTS 32
> +
> +enum ep_state {
> +	STATE_EP_DISABLED,
> +	STATE_EP_ENABLED,
> +};
> +
> +struct raw_ep {
> +	struct raw_dev		*dev;
> +	enum ep_state		state;
> +	struct usb_ep		*ep;
> +	struct usb_request	*req;
> +	bool			urb_queued;
> +	bool			disabling;
> +	ssize_t			status;
> +};
> +
> +enum dev_state {
> +	STATE_DEV_INVALID = 0,
> +	STATE_DEV_OPENED,
> +	STATE_DEV_INITIALIZED,
> +	STATE_DEV_RUNNING,
> +	STATE_DEV_CLOSED,
> +	STATE_DEV_FAILED
> +};
> +
> +struct raw_dev {
> +	struct kref			count;
> +	spinlock_t			lock;
> +
> +	const char			*udc_name;
> +	struct usb_gadget_driver	driver;

A dev embeds a driver?

Not a pointer?

But you have a kref, so the reference count of this object is there,
right?

> +
> +	/* Protected by lock: */
> +	enum dev_state			state;
> +	bool				gadget_registered;
> +	struct usb_gadget		*gadget;
> +	struct usb_request		*req;
> +	bool				ep0_in_pending;
> +	bool				ep0_out_pending;
> +	bool				ep0_urb_queued;
> +	ssize_t				ep0_status;
> +	struct raw_ep			eps[USB_RAW_MAX_ENDPOINTS];
> +
> +	struct completion		ep0_done;
> +	struct raw_event_queue		queue;

No pointer to your misc device?  I think you need that for logging.

> +};
> +
> +static struct raw_dev *dev_new(void)
> +{
> +	struct raw_dev *dev;
> +
> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return NULL;
> +	/* Matches kref_put() in raw_release(). */
> +	kref_init(&dev->count);
> +	spin_lock_init(&dev->lock);
> +	init_completion(&dev->ep0_done);
> +	raw_event_queue_init(&dev->queue);
> +	return dev;
> +}
> +
> +static void dev_free(struct kref *kref)
> +{
> +	struct raw_dev *dev = container_of(kref, struct raw_dev, count);
> +	int i;
> +
> +	kfree(dev->udc_name);
> +	kfree(dev->driver.udc_name);
> +	if (dev->req) {
> +		if (dev->ep0_urb_queued)
> +			usb_ep_dequeue(dev->gadget->ep0, dev->req);
> +		usb_ep_free_request(dev->gadget->ep0, dev->req);
> +	}
> +	raw_event_queue_destroy(&dev->queue);
> +	for (i = 0; i < USB_RAW_MAX_ENDPOINTS; i++) {
> +		if (dev->eps[i].state != STATE_EP_ENABLED)
> +			continue;
> +		usb_ep_disable(dev->eps[i].ep);
> +		usb_ep_free_request(dev->eps[i].ep, dev->eps[i].req);
> +		kfree(dev->eps[i].ep->desc);
> +		dev->eps[i].state = STATE_EP_DISABLED;
> +	}
> +	kfree(dev);
> +}
> +
> +/*----------------------------------------------------------------------*/
> +
> +static int raw_queue_event(struct raw_dev *dev,
> +	enum usb_raw_event_type type, size_t length, const void *data)
> +{
> +	int ret = 0;
> +	unsigned long flags;
> +
> +	ret = raw_event_queue_add(&dev->queue, type, length, data);
> +	if (ret < 0) {
> +		spin_lock_irqsave(&dev->lock, flags);
> +		dev->state = STATE_DEV_FAILED;
> +		spin_unlock_irqrestore(&dev->lock, flags);
> +	}
> +	return ret;
> +}
> +
> +static void gadget_ep0_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct raw_dev *dev = req->context;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	if (req->status)
> +		dev->ep0_status = req->status;
> +	else
> +		dev->ep0_status = req->actual;
> +	if (dev->ep0_in_pending)
> +		dev->ep0_in_pending = false;
> +	else
> +		dev->ep0_out_pending = false;
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +
> +	complete(&dev->ep0_done);
> +}
> +
> +static int gadget_bind(struct usb_gadget *gadget,
> +			struct usb_gadget_driver *driver)
> +{
> +	int ret = 0;
> +	struct raw_dev *dev = container_of(driver, struct raw_dev, driver);
> +	struct usb_request *req;
> +	unsigned long flags;
> +
> +	if (strcmp(gadget->name, dev->udc_name) != 0)
> +		return -ENODEV;
> +
> +	set_gadget_data(gadget, dev);
> +	req = usb_ep_alloc_request(gadget->ep0, GFP_KERNEL);
> +	if (!req) {
> +		pr_err("usb_ep_alloc_request failed\n");
> +		set_gadget_data(gadget, NULL);
> +		return -ENOMEM;
> +	}
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	dev->req = req;
> +	dev->req->context = dev;
> +	dev->req->complete = gadget_ep0_complete;
> +	dev->gadget = gadget;
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +
> +	/* Matches kref_put() in gadget_unbind(). */
> +	kref_get(&dev->count);
> +
> +	ret = raw_queue_event(dev, USB_RAW_EVENT_CONNECT, 0, NULL);
> +	if (ret < 0)
> +		pr_err("failed to queue event\n");
> +
> +	return ret;
> +}
> +
> +static void gadget_unbind(struct usb_gadget *gadget)
> +{
> +	struct raw_dev *dev = get_gadget_data(gadget);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	set_gadget_data(gadget, NULL);
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +	/* Matches kref_get() in gadget_bind(). */
> +	kref_put(&dev->count, dev_free);

What protects the kref from being called 'put' twice on the same
pointer at the same time?  There should be some lock somewhere, right?


> +}
> +
> +static int gadget_setup(struct usb_gadget *gadget,
> +			const struct usb_ctrlrequest *ctrl)
> +{
> +	int ret = 0;
> +	struct raw_dev *dev = get_gadget_data(gadget);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	if (dev->state != STATE_DEV_RUNNING) {
> +		pr_err("ignoring, device is not running\n");
> +		ret = -ENODEV;
> +		goto out_unlock;
> +	}
> +	if (dev->ep0_in_pending || dev->ep0_out_pending) {
> +		pr_debug("stalling, already have pending request\n");
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +	if ((ctrl->bRequestType & USB_DIR_IN) && ctrl->wLength)
> +		dev->ep0_in_pending = true;
> +	else
> +		dev->ep0_out_pending = true;
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +
> +	ret = raw_queue_event(dev, USB_RAW_EVENT_CONTROL, sizeof(*ctrl), ctrl);
> +	if (ret < 0)
> +		pr_err("failed to queue event\n");
> +	goto out;
> +
> +out_unlock:
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +out:
> +	return ret;
> +}
> +
> +/* These are currently ununsed but present in case UDC driver requires them. */
> +static void gadget_disconnect(struct usb_gadget *gadget) { }
> +static void gadget_suspend(struct usb_gadget *gadget) { }
> +static void gadget_resume(struct usb_gadget *gadget) { }
> +static void gadget_reset(struct usb_gadget *gadget) { }
> +
> +/*----------------------------------------------------------------------*/
> +
> +static int raw_open(struct inode *inode, struct file *fd)
> +{
> +	struct raw_dev *dev;
> +
> +	dev = dev_new();
> +	if (!dev) {
> +		pr_err("failed to created device");

You should have access to your misc device here, so dev_err()?

> +		return -ENOMEM;
> +	}
> +	fd->private_data = dev;
> +	dev->state = STATE_DEV_OPENED;
> +	return 0;
> +}
> +
> +static int raw_release(struct inode *inode, struct file *fd)
> +{
> +	int ret = 0;
> +	struct raw_dev *dev = fd->private_data;
> +	unsigned long flags;
> +	bool unregister = false;
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	dev->state = STATE_DEV_CLOSED;
> +	if (!dev->gadget) {
> +		spin_unlock_irqrestore(&dev->lock, flags);
> +		goto out_put;
> +	}
> +	if (dev->gadget_registered)
> +		unregister = true;
> +	dev->gadget_registered = false;
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +
> +	if (unregister) {
> +		ret = usb_gadget_unregister_driver(&dev->driver);
> +		WARN_ON(ret != 0);

you want to crash?  :)

> +		/* Matches kref_get() in raw_ioctl_run(). */
> +		kref_put(&dev->count, dev_free);
> +	}
> +
> +out_put:
> +	/* Matches dev_new() in raw_open(). */
> +	kref_put(&dev->count, dev_free);
> +	return ret;
> +}
> +
> +/*----------------------------------------------------------------------*/
> +
> +static int raw_ioctl_init(struct raw_dev *dev, unsigned long value)
> +{
> +	int ret = 0;
> +	struct usb_raw_init arg;
> +	char *udc_driver_name;
> +	char *udc_device_name;
> +	unsigned long flags;
> +
> +	ret = copy_from_user(&arg, (void __user *)value, sizeof(arg));
> +	if (ret)
> +		return ret;
> +
> +	switch (arg.speed) {
> +	case USB_SPEED_UNKNOWN:
> +		arg.speed = USB_SPEED_HIGH;
> +		break;
> +	case USB_SPEED_LOW:
> +	case USB_SPEED_FULL:
> +	case USB_SPEED_HIGH:
> +	case USB_SPEED_SUPER:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	udc_driver_name = kmalloc(UDC_NAME_LENGTH_MAX, GFP_KERNEL);
> +	if (!udc_driver_name)
> +		return -ENOMEM;
> +	ret = strscpy(udc_driver_name, &arg.driver_name[0],
> +				UDC_NAME_LENGTH_MAX);
> +	if (ret < 0) {
> +		kfree(udc_driver_name);
> +		return ret;
> +	}
> +	ret = 0;
> +
> +	udc_device_name = kmalloc(UDC_NAME_LENGTH_MAX, GFP_KERNEL);
> +	if (!udc_device_name) {
> +		kfree(udc_driver_name);
> +		return -ENOMEM;
> +	}
> +	ret = strscpy(udc_device_name, &arg.device_name[0],
> +				UDC_NAME_LENGTH_MAX);
> +	if (ret < 0) {
> +		kfree(udc_driver_name);
> +		kfree(udc_device_name);
> +		return ret;
> +	}
> +	ret = 0;
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	if (dev->state != STATE_DEV_OPENED) {
> +		pr_debug("fail, device is not opened\n");

dev_dbg().

> +		kfree(udc_driver_name);
> +		kfree(udc_device_name);
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +	dev->udc_name = udc_driver_name;
> +
> +	dev->driver.function = DRIVER_DESC;
> +	dev->driver.max_speed = arg.speed;
> +	dev->driver.setup = gadget_setup;
> +	dev->driver.disconnect = gadget_disconnect;
> +	dev->driver.bind = gadget_bind;
> +	dev->driver.unbind = gadget_unbind;
> +	dev->driver.suspend = gadget_suspend;
> +	dev->driver.resume = gadget_resume;
> +	dev->driver.reset = gadget_reset;
> +	dev->driver.driver.name = DRIVER_NAME;
> +	dev->driver.udc_name = udc_device_name;
> +	dev->driver.match_existing_only = 1;
> +
> +	dev->state = STATE_DEV_INITIALIZED;
> +
> +out_unlock:
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +	return ret;
> +}
> +
> +static int raw_ioctl_run(struct raw_dev *dev, unsigned long value)
> +{
> +	int ret = 0;
> +	unsigned long flags;
> +
> +	if (value)
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	if (dev->state != STATE_DEV_INITIALIZED) {
> +		pr_debug("fail, device is not initialized\n");

dev_dbg()

> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +
> +	ret = usb_gadget_probe_driver(&dev->driver);
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	if (ret) {
> +		pr_err("fail, usb_gadget_probe_driver returned %d\n", ret);

dev_err()

same goes for everywhere, no need for any pr_* calls in this driver.

> +		dev->state = STATE_DEV_FAILED;
> +		goto out_unlock;
> +	}
> +	dev->gadget_registered = true;
> +	dev->state = STATE_DEV_RUNNING;
> +	/* Matches kref_put() in raw_release(). */
> +	kref_get(&dev->count);
> +
> +out_unlock:
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +	return ret;
> +}
> +
> +static int raw_ioctl_event_fetch(struct raw_dev *dev, unsigned long value)
> +{
> +	int ret = 0;
> +	struct usb_raw_event arg;
> +	unsigned long flags;
> +	struct usb_raw_event *event;
> +	uint32_t length;
> +
> +	ret = copy_from_user(&arg, (void __user *)value, sizeof(arg));
> +	if (ret)
> +		return ret;
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	if (dev->state != STATE_DEV_RUNNING) {
> +		pr_debug("fail, device is not running\n");
> +		spin_unlock_irqrestore(&dev->lock, flags);
> +		return -EINVAL;
> +	}
> +	if (!dev->gadget) {
> +		pr_debug("fail, gadget is not bound\n");
> +		spin_unlock_irqrestore(&dev->lock, flags);
> +		return -EBUSY;
> +	}
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +
> +	event = raw_event_queue_fetch(&dev->queue);
> +	if (!event) {
> +		pr_debug("event fetching interrupted\n");
> +		return -EINTR;
> +	}
> +	length = min(arg.length, event->length);
> +	ret = copy_to_user((void __user *)value, event,
> +				sizeof(*event) + length);
> +	return ret;
> +}
> +
> +static void *raw_alloc_io_data(struct usb_raw_ep_io *io, void __user *ptr,
> +				bool get_from_user)
> +{
> +	int ret;
> +	void *data;
> +
> +	ret = copy_from_user(io, ptr, sizeof(*io));
> +	if (ret)
> +		return ERR_PTR(ret);
> +	if (io->ep >= USB_RAW_MAX_ENDPOINTS)
> +		return ERR_PTR(-EINVAL);
> +	if (!usb_raw_io_flags_valid(io->flags))
> +		return ERR_PTR(-EINVAL);
> +	if (io->length > PAGE_SIZE)
> +		return ERR_PTR(-EINVAL);
> +	if (get_from_user)
> +		data = memdup_user(ptr + sizeof(*io), io->length);
> +	else {
> +		data = kmalloc(io->length, GFP_KERNEL);
> +		if (!data)
> +			data = ERR_PTR(-ENOMEM);
> +	}
> +	return data;
> +}
> +
> +static int raw_process_ep0_io(struct raw_dev *dev, struct usb_raw_ep_io *io,
> +				void *data, bool in)
> +{
> +	int ret = 0;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	if (dev->state != STATE_DEV_RUNNING) {
> +		pr_debug("fail, device is not running\n");
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +	if (!dev->gadget) {
> +		pr_debug("fail, gadget is not bound\n");
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +	if (dev->ep0_urb_queued) {
> +		pr_debug("fail, urb already queued\n");
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +	if ((in && !dev->ep0_in_pending) ||
> +			(!in && !dev->ep0_out_pending)) {
> +		pr_debug("fail, wrong direction\n");
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +	if (WARN_ON(in && dev->ep0_out_pending)) {
> +		ret = -ENODEV;
> +		dev->state = STATE_DEV_FAILED;
> +		goto out_done;
> +	}
> +	if (WARN_ON(!in && dev->ep0_in_pending)) {
> +		ret = -ENODEV;
> +		dev->state = STATE_DEV_FAILED;
> +		goto out_done;
> +	}
> +
> +	dev->req->buf = data;
> +	dev->req->length = io->length;
> +	dev->req->zero = usb_raw_io_flags_zero(io->flags);
> +	dev->ep0_urb_queued = true;
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +
> +	ret = usb_ep_queue(dev->gadget->ep0, dev->req, GFP_ATOMIC);
> +	if (ret) {
> +		pr_err("fail, usb_ep_queue returned %d\n", ret);
> +		spin_lock_irqsave(&dev->lock, flags);
> +		dev->state = STATE_DEV_FAILED;
> +		goto out_done;
> +	}
> +
> +	ret = wait_for_completion_interruptible(&dev->ep0_done);
> +	if (ret) {
> +		pr_debug("wait interrupted\n");
> +		usb_ep_dequeue(dev->gadget->ep0, dev->req);
> +		wait_for_completion(&dev->ep0_done);
> +		spin_lock_irqsave(&dev->lock, flags);
> +		goto out_done;
> +	}
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	ret = dev->ep0_status;
> +
> +out_done:
> +	dev->ep0_urb_queued = false;
> +out_unlock:
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +	return ret;
> +}
> +
> +static int raw_ioctl_ep0_write(struct raw_dev *dev, unsigned long value)
> +{
> +	int ret = 0;
> +	void *data;
> +	struct usb_raw_ep_io io;
> +
> +	data = raw_alloc_io_data(&io, (void __user *)value, true);
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +	ret = raw_process_ep0_io(dev, &io, data, true);
> +	kfree(data);
> +	return ret;
> +}
> +
> +static int raw_ioctl_ep0_read(struct raw_dev *dev, unsigned long value)
> +{
> +	int ret = 0;
> +	void *data;
> +	struct usb_raw_ep_io io;
> +	unsigned int length;
> +
> +	data = raw_alloc_io_data(&io, (void __user *)value, false);
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +	ret = raw_process_ep0_io(dev, &io, data, false);
> +	if (ret < 0) {
> +		kfree(data);
> +		return ret;
> +	}
> +	length = min(io.length, (unsigned int)ret);
> +	ret = copy_to_user((void __user *)(value + sizeof(io)), data, length);
> +	kfree(data);
> +	return ret;
> +}
> +
> +static bool check_ep_caps(struct usb_ep *ep,
> +				struct usb_endpoint_descriptor *desc)
> +{
> +	switch (usb_endpoint_type(desc)) {
> +	case USB_ENDPOINT_XFER_ISOC:
> +		if (!ep->caps.type_iso)
> +			return false;
> +		break;
> +	case USB_ENDPOINT_XFER_BULK:
> +		if (!ep->caps.type_bulk)
> +			return false;
> +		break;
> +	case USB_ENDPOINT_XFER_INT:
> +		if (!ep->caps.type_int)
> +			return false;
> +		break;
> +	default:
> +		return false;
> +	}
> +
> +	if (usb_endpoint_dir_in(desc) && !ep->caps.dir_in)
> +		return false;
> +	if (usb_endpoint_dir_out(desc) && !ep->caps.dir_out)
> +		return false;
> +
> +	return true;
> +}
> +
> +static int raw_ioctl_ep_enable(struct raw_dev *dev, unsigned long value)
> +{
> +	int ret = 0, i;
> +	unsigned long flags;
> +	struct usb_endpoint_descriptor *desc;
> +	struct usb_ep *ep = NULL;
> +
> +	desc = memdup_user((void __user *)value, sizeof(*desc));
> +	if (IS_ERR(desc))
> +		return PTR_ERR(desc);
> +
> +	/*
> +	 * Endpoints with a maxpacket length of 0 can cause crashes in UDC
> +	 * drivers.
> +	 */
> +	if (usb_endpoint_maxp(desc) == 0) {
> +		kfree(desc);
> +		return -EINVAL;
> +	}
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	if (dev->state != STATE_DEV_RUNNING) {
> +		pr_debug("fail, device is not running\n");
> +		ret = -EINVAL;
> +		goto out_free;
> +	}
> +	if (!dev->gadget) {
> +		pr_debug("fail, gadget is not bound\n");
> +		ret = -EBUSY;
> +		goto out_free;
> +	}
> +
> +	for (i = 0; i < USB_RAW_MAX_ENDPOINTS; i++) {
> +		if (dev->eps[i].state == STATE_EP_ENABLED)
> +			continue;
> +		break;
> +	}
> +	if (i == USB_RAW_MAX_ENDPOINTS) {
> +		pr_debug("fail, no device endpoints available\n");
> +		ret = -EBUSY;
> +		goto out_free;
> +	}
> +
> +	gadget_for_each_ep(ep, dev->gadget) {
> +		if (ep->enabled)
> +			continue;
> +		if (!check_ep_caps(ep, desc))
> +			continue;
> +		ep->desc = desc;
> +		ret = usb_ep_enable(ep);
> +		if (ret < 0) {
> +			pr_err("fail, usb_ep_enable returned %d\n", ret);
> +			goto out_free;
> +		}
> +		dev->eps[i].req = usb_ep_alloc_request(ep, GFP_ATOMIC);
> +		if (!dev->eps[i].req) {
> +			pr_err("fail, usb_ep_alloc_request failed\n");
> +			usb_ep_disable(ep);
> +			ret = -ENOMEM;
> +			goto out_free;
> +		}
> +		dev->eps[i].ep = ep;
> +		dev->eps[i].state = STATE_EP_ENABLED;
> +		ep->driver_data = &dev->eps[i];
> +		ret = i;
> +		goto out_unlock;
> +	}
> +
> +	pr_debug("fail, no gadget endpoints available\n");
> +	ret = -EBUSY;
> +
> +out_free:
> +	kfree(desc);
> +out_unlock:
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +	return ret;
> +}
> +
> +static int raw_ioctl_ep_disable(struct raw_dev *dev, unsigned long value)
> +{
> +	int ret = 0, i = value;
> +	unsigned long flags;
> +	const void *desc;
> +
> +	if (i < 0 || i >= USB_RAW_MAX_ENDPOINTS)
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	if (dev->state != STATE_DEV_RUNNING) {
> +		pr_debug("fail, device is not running\n");
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +	if (!dev->gadget) {
> +		pr_debug("fail, gadget is not bound\n");
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +	if (dev->eps[i].state != STATE_EP_ENABLED) {
> +		pr_debug("fail, endpoint is not enabled\n");
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +	if (dev->eps[i].disabling) {
> +		pr_debug("fail, disable already in progress\n");
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +	dev->eps[i].disabling = true;
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +
> +	usb_ep_disable(dev->eps[i].ep);
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	usb_ep_free_request(dev->eps[i].ep, dev->eps[i].req);
> +	desc = dev->eps[i].ep->desc;
> +	dev->eps[i].ep = NULL;
> +	dev->eps[i].state = STATE_EP_DISABLED;
> +	kfree(desc);
> +	dev->eps[i].disabling = false;
> +
> +out_unlock:
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +	return ret;
> +}
> +
> +static void gadget_ep_complete(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct raw_ep *r_ep = (struct raw_ep *)ep->driver_data;
> +	struct raw_dev *dev = r_ep->dev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	if (req->status)
> +		r_ep->status = req->status;
> +	else
> +		r_ep->status = req->actual;
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +
> +	complete((struct completion *)req->context);
> +}
> +
> +static int raw_process_ep_io(struct raw_dev *dev, struct usb_raw_ep_io *io,
> +				void *data, bool in)
> +{
> +	int ret = 0;
> +	unsigned long flags;
> +	struct raw_ep *ep = &dev->eps[io->ep];
> +	DECLARE_COMPLETION_ONSTACK(done);
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	if (dev->state != STATE_DEV_RUNNING) {
> +		pr_debug("fail, device is not running\n");
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +	if (!dev->gadget) {
> +		pr_debug("fail, gadget is not bound\n");
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +	if (ep->state != STATE_EP_ENABLED) {
> +		pr_debug("fail, endpoint is not enabled\n");
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +	if (ep->disabling) {
> +		pr_debug("fail, endpoint is being disabled\n");
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +	if (ep->urb_queued) {
> +		pr_debug("fail, urb already queued\n");
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +	if ((in && !ep->ep->caps.dir_in) || (!in && ep->ep->caps.dir_in)) {
> +		pr_debug("fail, wrong direction\n");
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	ep->dev = dev;
> +	ep->req->context = &done;
> +	ep->req->complete = gadget_ep_complete;
> +	ep->req->buf = data;
> +	ep->req->length = io->length;
> +	ep->req->zero = usb_raw_io_flags_zero(io->flags);
> +	ep->urb_queued = true;
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +
> +	ret = usb_ep_queue(ep->ep, ep->req, GFP_ATOMIC);
> +	if (ret) {
> +		pr_err("fail, usb_ep_queue returned %d\n", ret);
> +		spin_lock_irqsave(&dev->lock, flags);
> +		dev->state = STATE_DEV_FAILED;
> +		goto out_done;
> +	}
> +
> +	ret = wait_for_completion_interruptible(&done);
> +	if (ret) {
> +		pr_debug("wait interrupted\n");
> +		usb_ep_dequeue(ep->ep, ep->req);
> +		wait_for_completion(&done);
> +		spin_lock_irqsave(&dev->lock, flags);
> +		goto out_done;
> +	}
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	ret = ep->status;
> +
> +out_done:
> +	ep->urb_queued = false;
> +out_unlock:
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +	return ret;
> +}
> +
> +static int raw_ioctl_ep_write(struct raw_dev *dev, unsigned long value)
> +{
> +	int ret = 0;
> +	char *data;
> +	struct usb_raw_ep_io io;
> +
> +	data = raw_alloc_io_data(&io, (void __user *)value, true);
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +	ret = raw_process_ep_io(dev, &io, data, true);
> +	kfree(data);
> +	return ret;
> +}
> +
> +static int raw_ioctl_ep_read(struct raw_dev *dev, unsigned long value)
> +{
> +	int ret = 0;
> +	char *data;
> +	struct usb_raw_ep_io io;
> +	unsigned int length;
> +
> +	data = raw_alloc_io_data(&io, (void __user *)value, false);
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +	ret = raw_process_ep_io(dev, &io, data, false);
> +	if (ret < 0) {
> +		kfree(data);
> +		return ret;
> +	}
> +	length = min(io.length, (unsigned int)ret);
> +	ret = copy_to_user((void __user *)(value + sizeof(io)), data, length);
> +	kfree(data);
> +	return ret;
> +}
> +
> +static int raw_ioctl_configure(struct raw_dev *dev, unsigned long value)
> +{
> +	int ret = 0;
> +	unsigned long flags;
> +
> +	if (value)
> +		return -EINVAL;
> +	spin_lock_irqsave(&dev->lock, flags);
> +	if (dev->state != STATE_DEV_RUNNING) {
> +		pr_debug("fail, device is not running\n");
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +	if (!dev->gadget) {
> +		pr_debug("fail, gadget is not bound\n");
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +	usb_gadget_set_state(dev->gadget, USB_STATE_CONFIGURED);
> +
> +out_unlock:
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +	return ret;
> +}
> +
> +static int raw_ioctl_vbus_draw(struct raw_dev *dev, unsigned long value)
> +{
> +	int ret = 0;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	if (dev->state != STATE_DEV_RUNNING) {
> +		pr_debug("fail, device is not running\n");
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +	if (!dev->gadget) {
> +		pr_debug("fail, gadget is not bound\n");
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +	usb_gadget_vbus_draw(dev->gadget, 2 * value);
> +
> +out_unlock:
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +	return ret;
> +}
> +
> +static long raw_ioctl(struct file *fd, unsigned int cmd, unsigned long value)
> +{
> +	struct raw_dev *dev = fd->private_data;
> +	int ret = 0;
> +
> +	if (!dev)
> +		return -EBUSY;
> +
> +	switch (cmd) {
> +	case USB_RAW_IOCTL_INIT:
> +		ret = raw_ioctl_init(dev, value);
> +		break;
> +	case USB_RAW_IOCTL_RUN:
> +		ret = raw_ioctl_run(dev, value);
> +		break;
> +	case USB_RAW_IOCTL_EVENT_FETCH:
> +		ret = raw_ioctl_event_fetch(dev, value);
> +		break;
> +	case USB_RAW_IOCTL_EP0_WRITE:
> +		ret = raw_ioctl_ep0_write(dev, value);
> +		break;
> +	case USB_RAW_IOCTL_EP0_READ:
> +		ret = raw_ioctl_ep0_read(dev, value);
> +		break;
> +	case USB_RAW_IOCTL_EP_ENABLE:
> +		ret = raw_ioctl_ep_enable(dev, value);
> +		break;
> +	case USB_RAW_IOCTL_EP_DISABLE:
> +		ret = raw_ioctl_ep_disable(dev, value);
> +		break;
> +	case USB_RAW_IOCTL_EP_WRITE:
> +		ret = raw_ioctl_ep_write(dev, value);
> +		break;
> +	case USB_RAW_IOCTL_EP_READ:
> +		ret = raw_ioctl_ep_read(dev, value);
> +		break;
> +	case USB_RAW_IOCTL_CONFIGURE:
> +		ret = raw_ioctl_configure(dev, value);
> +		break;
> +	case USB_RAW_IOCTL_VBUS_DRAW:
> +		ret = raw_ioctl_vbus_draw(dev, value);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +/*----------------------------------------------------------------------*/
> +
> +static const struct file_operations raw_fops = {
> +	.open =			raw_open,
> +	.unlocked_ioctl =	raw_ioctl,
> +	.compat_ioctl =		raw_ioctl,
> +	.release =		raw_release,
> +	.llseek =		no_llseek,
> +};
> +
> +static struct miscdevice raw_device = {
> +	.minor = MISC_DYNAMIC_MINOR,
> +	.name = "raw-gadget",

DRIVER_NAME?

> +	.fops = &raw_fops,
> +};
> +
> +static int __init raw_init(void)
> +{
> +	int rv;
> +
> +	rv = misc_register(&raw_device);
> +	if (rv) {
> +		pr_err("failed to register raw-gadget device with %d\n", rv);
> +		return rv;
> +	}
> +	return 0;
> +}
> +
> +static void __exit raw_exit(void)
> +{
> +	misc_deregister(&raw_device);
> +}
> +
> +module_init(raw_init);
> +module_exit(raw_exit);

module_misc_device()?  :)

Looks good.

thanks,

greg k-h