On Mon, Oct 12, 2020 at 07:10:22PM +0800, rickyniu wrote:
> @@ -369,6 +372,13 @@ config USB_CONFIGFS_F_FS
> implemented in kernel space (for instance Ethernet, serial or
> mass storage) and other are implemented in user space.
>
> +config USB_CONFIGFS_F_ACC
> + bool "Accessory gadget"
> + depends on USB_CONFIGFS
> + select USB_F_ACC
> + help
> + USB gadget Accessory support
> +
We need a real help text here please.
> config USB_CONFIGFS_F_UAC1
> bool "Audio Class 1.0"
> depends on USB_CONFIGFS
> diff --git a/drivers/usb/gadget/function/Makefile b/drivers/usb/gadget/function/Makefile
> index 5d3a6cf02218..2305360e5f22 100644
> --- a/drivers/usb/gadget/function/Makefile
> +++ b/drivers/usb/gadget/function/Makefile
> @@ -50,3 +50,5 @@ usb_f_printer-y := f_printer.o
> obj-$(CONFIG_USB_F_PRINTER) += usb_f_printer.o
> usb_f_tcm-y := f_tcm.o
> obj-$(CONFIG_USB_F_TCM) += usb_f_tcm.o
> +usb_f_accessory-y := f_accessory.o
> +obj-$(CONFIG_USB_F_ACC) += usb_f_accessory.o
> diff --git a/drivers/usb/gadget/function/f_accessory.c b/drivers/usb/gadget/function/f_accessory.c
> new file mode 100644
> index 000000000000..514eadee1793
> --- /dev/null
> +++ b/drivers/usb/gadget/function/f_accessory.c
> @@ -0,0 +1,1352 @@
> +/*
> + * Gadget Function Driver for Android USB accessories
You didn't run this through checkpatch, did you :(
You need a spdx line here.
> + *
> + * Copyright (C) 2011 Google, Inc.
No one has touched this since 2011?
> + * Author: Mike Lockwood <lockwood@xxxxxxxxxxx>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
license boiler-plate text can be removed once you add the correct spdx
line.
> + *
> + */
> +
> +/* #define DEBUG */
> +/* #define VERBOSE_DEBUG */
Why are these here?
> +static int create_bulk_endpoints(struct acc_dev *dev,
> + struct usb_endpoint_descriptor *in_desc,
> + struct usb_endpoint_descriptor *out_desc)
> +{
> + struct usb_composite_dev *cdev = dev->cdev;
> + struct usb_request *req;
> + struct usb_ep *ep;
> + int i;
> +
> + DBG(cdev, "create_bulk_endpoints dev: %p\n", dev);
ftrace is your friend, this is not needed.
> +
> + ep = usb_ep_autoconfig(cdev->gadget, in_desc);
> + if (!ep) {
> + DBG(cdev, "usb_ep_autoconfig for ep_in failed\n");
dev_err()?
> + return -ENODEV;
> + }
> + DBG(cdev, "usb_ep_autoconfig for ep_in got %s\n", ep->name);
dev_dbg()?
> + ep->driver_data = dev; /* claim the endpoint */
> + dev->ep_in = ep;
> +
> + ep = usb_ep_autoconfig(cdev->gadget, out_desc);
> + if (!ep) {
> + DBG(cdev, "usb_ep_autoconfig for ep_out failed\n");
> + return -ENODEV;
> + }
> + DBG(cdev, "usb_ep_autoconfig for ep_out got %s\n", ep->name);
> + ep->driver_data = dev; /* claim the endpoint */
> + dev->ep_out = ep;
> +
> + /* now allocate requests for our endpoints */
> + for (i = 0; i < TX_REQ_MAX; i++) {
> + req = acc_request_new(dev->ep_in, BULK_BUFFER_SIZE);
> + if (!req)
> + goto fail;
> + req->complete = acc_complete_in;
> + req_put(dev, &dev->tx_idle, req);
> + }
> + for (i = 0; i < RX_REQ_MAX; i++) {
> + req = acc_request_new(dev->ep_out, BULK_BUFFER_SIZE);
> + if (!req)
> + goto fail;
> + req->complete = acc_complete_out;
> + dev->rx_req[i] = req;
> + }
> +
> + return 0;
> +
> +fail:
> + pr_err("acc_bind() could not allocate requests\n");
dev_err()?
Same goes for all other pr_* calls in this driver.
> + while ((req = req_get(dev, &dev->tx_idle)))
> + acc_request_free(req, dev->ep_in);
> + for (i = 0; i < RX_REQ_MAX; i++)
> + acc_request_free(dev->rx_req[i], dev->ep_out);
> + return -1;
> +}
> +
> +static ssize_t acc_read(struct file *fp, char __user *buf,
> + size_t count, loff_t *pos)
> +{
> + struct acc_dev *dev = fp->private_data;
> + struct usb_request *req;
> + ssize_t r = count;
> + unsigned xfer;
> + int ret = 0;
> +
> + pr_debug("acc_read(%zu)\n", count);
Again, ftrace is there, please use it and remove all of this type of
debugging lines.
> +
> + if (dev->disconnected) {
> + pr_debug("acc_read disconnected");
> + return -ENODEV;
> + }
> +
> + if (count > BULK_BUFFER_SIZE)
> + count = BULK_BUFFER_SIZE;
> +
> + /* we will block until we're online */
> + pr_debug("acc_read: waiting for online\n");
> + ret = wait_event_interruptible(dev->read_wq, dev->online);
> + if (ret < 0) {
> + r = ret;
> + goto done;
> + }
> +
> + if (dev->rx_done) {
> + // last req cancelled. try to get it.
> + req = dev->rx_req[0];
> + goto copy_data;
> + }
> +
> +requeue_req:
> + /* queue a request */
> + req = dev->rx_req[0];
> + req->length = count;
> + dev->rx_done = 0;
> + ret = usb_ep_queue(dev->ep_out, req, GFP_KERNEL);
> + if (ret < 0) {
> + r = -EIO;
> + goto done;
> + } else {
> + pr_debug("rx %p queue\n", req);
> + }
> +
> + /* wait for a request to complete */
> + ret = wait_event_interruptible(dev->read_wq, dev->rx_done);
> + if (ret < 0) {
> + r = ret;
> + ret = usb_ep_dequeue(dev->ep_out, req);
> + if (ret != 0) {
> + // cancel failed. There can be a data already received.
> + // it will be retrieved in the next read.
> + pr_debug("acc_read: cancelling failed %d", ret);
> + }
> + goto done;
> + }
> +
> +copy_data:
> + dev->rx_done = 0;
> + if (dev->online) {
> + /* If we got a 0-len packet, throw it back and try again. */
> + if (req->actual == 0)
> + goto requeue_req;
> +
> + pr_debug("rx %p %u\n", req, req->actual);
> + xfer = (req->actual < count) ? req->actual : count;
> + r = xfer;
> + if (copy_to_user(buf, req->buf, xfer))
> + r = -EFAULT;
> + } else
> + r = -EIO;
> +
> +done:
> + pr_debug("acc_read returning %zd\n", r);
> + return r;
> +}
> +
> +static ssize_t acc_write(struct file *fp, const char __user *buf,
> + size_t count, loff_t *pos)
> +{
> + struct acc_dev *dev = fp->private_data;
> + struct usb_request *req = 0;
> + ssize_t r = count;
> + unsigned xfer;
> + int ret;
> +
> + pr_debug("acc_write(%zu)\n", count);
> +
> + if (!dev->online || dev->disconnected) {
> + pr_debug("acc_write disconnected or not online");
> + return -ENODEV;
> + }
> +
> + while (count > 0) {
> + if (!dev->online) {
> + pr_debug("acc_write dev->error\n");
> + r = -EIO;
> + break;
> + }
> +
> + /* get an idle tx request to use */
> + req = 0;
> + ret = wait_event_interruptible(dev->write_wq,
> + ((req = req_get(dev, &dev->tx_idle)) || !dev->online));
> + if (!req) {
> + r = ret;
> + break;
> + }
> +
> + if (count > BULK_BUFFER_SIZE) {
> + xfer = BULK_BUFFER_SIZE;
> + /* ZLP, They will be more TX requests so not yet. */
> + req->zero = 0;
> + } else {
> + xfer = count;
> + /* If the data length is a multple of the
> + * maxpacket size then send a zero length packet(ZLP).
> + */
> + req->zero = ((xfer % dev->ep_in->maxpacket) == 0);
> + }
> + if (copy_from_user(req->buf, buf, xfer)) {
> + r = -EFAULT;
> + break;
> + }
> +
> + req->length = xfer;
> + ret = usb_ep_queue(dev->ep_in, req, GFP_KERNEL);
> + if (ret < 0) {
> + pr_debug("acc_write: xfer error %d\n", ret);
> + r = -EIO;
> + break;
> + }
> +
> + buf += xfer;
> + count -= xfer;
> +
> + /* zero this so we don't try to free it on error exit */
> + req = 0;
> + }
> +
> + if (req)
> + req_put(dev, &dev->tx_idle, req);
> +
> + pr_debug("acc_write returning %zd\n", r);
> + return r;
> +}
> +
> +static long acc_ioctl(struct file *fp, unsigned code, unsigned long value)
> +{
> + struct acc_dev *dev = fp->private_data;
> + char *src = NULL;
> + int ret;
> +
> + switch (code) {
> + case ACCESSORY_GET_STRING_MANUFACTURER:
> + src = dev->manufacturer;
> + break;
> + case ACCESSORY_GET_STRING_MODEL:
> + src = dev->model;
> + break;
> + case ACCESSORY_GET_STRING_DESCRIPTION:
> + src = dev->description;
> + break;
> + case ACCESSORY_GET_STRING_VERSION:
> + src = dev->version;
> + break;
> + case ACCESSORY_GET_STRING_URI:
> + src = dev->uri;
> + break;
> + case ACCESSORY_GET_STRING_SERIAL:
> + src = dev->serial;
> + break;
> + case ACCESSORY_IS_START_REQUESTED:
> + return dev->start_requested;
> + case ACCESSORY_GET_AUDIO_MODE:
> + return dev->audio_mode;
> + }
> + if (!src)
> + return -EINVAL;
> +
> + ret = strlen(src) + 1;
> + if (copy_to_user((void __user *)value, src, ret))
> + ret = -EFAULT;
> + return ret;
> +}
> +
> +static int acc_open(struct inode *ip, struct file *fp)
> +{
> + printk(KERN_INFO "acc_open\n");
That's just noisy, did you test this???
> + if (atomic_xchg(&_acc_dev->open_excl, 1))
> + return -EBUSY;
> +
> + _acc_dev->disconnected = 0;
> + fp->private_data = _acc_dev;
> + return 0;
> +}
> +
> +static int acc_release(struct inode *ip, struct file *fp)
> +{
> + printk(KERN_INFO "acc_release\n");
Again, this is wrong.
> +
> + WARN_ON(!atomic_xchg(&_acc_dev->open_excl, 0));
> + /* indicate that we are disconnected
> + * still could be online so don't touch online flag
> + */
> + _acc_dev->disconnected = 1;
> + return 0;
> +}
> +
> +/* file operations for /dev/usb_accessory */
> +static const struct file_operations acc_fops = {
> + .owner = THIS_MODULE,
> + .read = acc_read,
> + .write = acc_write,
> + .unlocked_ioctl = acc_ioctl,
> + .open = acc_open,
> + .release = acc_release,
> +};
> +
> +static int acc_hid_probe(struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
> + int ret;
> +
> + ret = hid_parse(hdev);
> + if (ret)
> + return ret;
> + return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> +}
> +
> +static struct miscdevice acc_device = {
> + .minor = MISC_DYNAMIC_MINOR,
> + .name = "usb_accessory",
> + .fops = &acc_fops,
> +};
> +
> +static const struct hid_device_id acc_hid_table[] = {
> + { HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID) },
> + { }
> +};
> +
> +static struct hid_driver acc_hid_driver = {
> + .name = "USB accessory",
> + .id_table = acc_hid_table,
> + .probe = acc_hid_probe,
> +};
> +
> +static void acc_complete_setup_noop(struct usb_ep *ep, struct usb_request *req)
> +{
> + /*
> + * Default no-op function when nothing needs to be done for the
> + * setup request
> + */
> +}
> +
> +int acc_ctrlrequest(struct usb_composite_dev *cdev,
> + const struct usb_ctrlrequest *ctrl)
> +{
> + struct acc_dev *dev = _acc_dev;
> + int value = -EOPNOTSUPP;
> + struct acc_hid_dev *hid;
> + int offset;
> + u8 b_requestType = ctrl->bRequestType;
> + u8 b_request = ctrl->bRequest;
> + u16 w_index = le16_to_cpu(ctrl->wIndex);
> + u16 w_value = le16_to_cpu(ctrl->wValue);
> + u16 w_length = le16_to_cpu(ctrl->wLength);
> + unsigned long flags;
Odd alignment issues :(
> +
> +/*
> + printk(KERN_INFO "acc_ctrlrequest "
> + "%02x.%02x v%04x i%04x l%u\n",
> + b_requestType, b_request,
> + w_value, w_index, w_length);
> +*/
Please remove all debugging code from the driver when you resend this.
thanks,
greg k-h
