Re: [PATCH 1/3] ANDROID: usb: gadget: f_accessory: Add Android Accessory function

From: Felipe Balbi
Date: Mon Oct 12 2020 - 11:58:39 EST

Next message: Alan Stern: "Re: [linux-safety] [PATCH] usb: host: ehci-sched: add comment about find_tt() not returning error"
Previous message: Ingo Molnar: "[GIT PULL] Static calls for v5.10"
In reply to: Greg KH: "Re: [PATCH 1/3] ANDROID: usb: gadget: f_accessory: Add Android Accessory function"
Next in thread: rickyniu: "[PATCH 2/3] ANDROID: USB: f_accessory: Check dev pointer before decoding ctrl request"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

rickyniu <rickyniu@xxxxxxxxxx> writes:
> From: Benoit Goby <benoit@xxxxxxxxxxx>

missing Signed-off-by for author

> USB accessory mode allows users to connect USB host hardware
> specifically designed for Android-powered devices. The accessories
> must adhere to the Android accessory protocol outlined in the
> http://accessories.android.com documentation. This allows
> Android devices that cannot act as a USB host to still interact with
> USB hardware. When an Android device is in USB accessory mode, the
> attached Android USB accessory acts as the host, provides power
> to the USB bus, and enumerates connected devices.

The protocol is still a HID protocol, is it? Why couldn't you use f_hid.c?

> Signed-off-by: Mike Lockwood <lockwood@xxxxxxxxxxx>
> [AmitP: Folded following android-4.9 commit changes into this patch
> ceb2f0aac624 ("ANDROID: usb: gadget: accessory: Fix section mismatch")
> Parts of e27543931009 ("ANDROID: usb: gadget: Fixes and hacks to make android usb gadget compile on 3.8")
> 1b07ec751563 ("ANDROID: drivers: usb: gadget: 64-bit related type fixes")]
> Signed-off-by: Amit Pundir <amit.pundir@xxxxxxxxxx>
> [astrachan: Folded the following changes into this patch:
> 9d5891d516e2 ("ANDROID: usb: gadget: f_accessory: Add ACCESSORY_SET_AUDIO_MODE control request and ioctl")
> dc66cfce9622 ("ANDROID: usb: gadget: f_accessory: Add support for HID input devices")
> 5f1ac9c2871b ("ANDROID: usb: gadget: f_accessory: move userspace interface to uapi")
> 9a6241722cd8 ("ANDROID: usb: gadget: f_accessory: Enabled Zero Length Packet (ZLP) for acc_write")
> 31a0ecd5a825 ("ANDROID: usb: gadget: f_accessory: check for accessory device before disconnecting HIDs")
> 580721fa6cbc ("ANDROID: usb: gadget: f_accessory: Migrate to USB_FUNCTION API")
> 7f407172fb28 ("ANDROID: usb: gadget: f_accessory: Fix for UsbAccessory clean unbind.")
> ebc98ac5a22f ("ANDROID: usb: gadget: f_accessory: fix false disconnect due to a signal sent to the reading process")
> 71c6dc5ffdab ("ANDROID: usb: gadget: f_accessory: assign no-op request complete callbacks")
> 675047ee68e9 ("ANDROID: usb: gadget: f_accessory: Move gadget functions code")
> b2bedaa5c7df ("CHROMIUM: usb: gadget: f_accessory: add .raw_request callback")]
> Signed-off-by: Alistair Strachan <astrachan@xxxxxxxxxx>
> Signed-off-by: rickyniu <rickyniu@xxxxxxxxxx>

We need an actual name here, IIRC.

> 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 @@

missing SPDX license identifier comment

> +/*
> + * Gadget Function Driver for Android USB accessories
> + *
> + * Copyright (C) 2011 Google, Inc.
> + * 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.
> + *
> + */
> +
> +/* #define DEBUG */
> +/* #define VERBOSE_DEBUG */

these shouldn't be necessary

> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/poll.h>
> +#include <linux/delay.h>
> +#include <linux/wait.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/kthread.h>
> +#include <linux/freezer.h>
> +
> +#include <linux/types.h>
> +#include <linux/file.h>
> +#include <linux/device.h>
> +#include <linux/miscdevice.h>
> +
> +#include <linux/hid.h>
> +#include <linux/hiddev.h>
>
> +#include <linux/usb.h>
> +#include <linux/usb/ch9.h>
> +#include <linux/usb/f_accessory.h>
> +
> +#include <linux/configfs.h>
> +#include <linux/usb/composite.h>
> +
> +#define MAX_INST_NAME_LEN 40
> +#define BULK_BUFFER_SIZE 16384
> +#define ACC_STRING_SIZE 256
> +
> +#define PROTOCOL_VERSION 2
> +
> +/* String IDs */
> +#define INTERFACE_STRING_INDEX 0
> +
> +/* number of tx and rx requests to allocate */
> +#define TX_REQ_MAX 4
> +#define RX_REQ_MAX 2
> +
> +struct acc_hid_dev {
> + struct list_head list;
> + struct hid_device *hid;
> + struct acc_dev *dev;
> + /* accessory defined ID */
> + int id;
> + /* HID report descriptor */
> + u8 *report_desc;
> + /* length of HID report descriptor */
> + int report_desc_len;
> + /* number of bytes of report_desc we have received so far */
> + int report_desc_offset;
> +};
> +
> +struct acc_dev {
> + struct usb_function function;
> + struct usb_composite_dev *cdev;
> + spinlock_t lock;
> +
> + struct usb_ep *ep_in;
> + struct usb_ep *ep_out;
> +
> + /* online indicates state of function_set_alt & function_unbind
> + * set to 1 when we connect
> + */

wrong multi-line comment style

> + int online:1;
> +
> + /* disconnected indicates state of open & release
> + * Set to 1 when we disconnect.
> + * Not cleared until our file is closed.
> + */
> + int disconnected:1;
> +
> + /* strings sent by the host */
> + char manufacturer[ACC_STRING_SIZE];
> + char model[ACC_STRING_SIZE];
> + char description[ACC_STRING_SIZE];
> + char version[ACC_STRING_SIZE];
> + char uri[ACC_STRING_SIZE];
> + char serial[ACC_STRING_SIZE];
> +
> + /* for acc_complete_set_string */
> + int string_index;
> +
> + /* set to 1 if we have a pending start request */
> + int start_requested;
> +
> + int audio_mode;
> +
> + /* synchronize access to our device file */
> + atomic_t open_excl;
> +
> + struct list_head tx_idle;
> +
> + wait_queue_head_t read_wq;
> + wait_queue_head_t write_wq;
> + struct usb_request *rx_req[RX_REQ_MAX];
> + int rx_done;
> +
> + /* delayed work for handling ACCESSORY_START */
> + struct delayed_work start_work;
> +
> + /* worker for registering and unregistering hid devices */
> + struct work_struct hid_work;

why are these workers needed?

> +static struct usb_interface_descriptor acc_interface_desc = {
> + .bLength = USB_DT_INTERFACE_SIZE,
> + .bDescriptorType = USB_DT_INTERFACE,
> + .bInterfaceNumber = 0,
> + .bNumEndpoints = 2,
> + .bInterfaceClass = USB_CLASS_VENDOR_SPEC,
> + .bInterfaceSubClass = USB_SUBCLASS_VENDOR_SPEC,
> + .bInterfaceProtocol = 0,
> +};

const?

> +static struct usb_endpoint_descriptor acc_highspeed_in_desc = {
> + .bLength = USB_DT_ENDPOINT_SIZE,
> + .bDescriptorType = USB_DT_ENDPOINT,
> + .bEndpointAddress = USB_DIR_IN,
> + .bmAttributes = USB_ENDPOINT_XFER_BULK,
> + .wMaxPacketSize = __constant_cpu_to_le16(512),
> +};

const?

> +static struct usb_endpoint_descriptor acc_highspeed_out_desc = {
> + .bLength = USB_DT_ENDPOINT_SIZE,
> + .bDescriptorType = USB_DT_ENDPOINT,
> + .bEndpointAddress = USB_DIR_OUT,
> + .bmAttributes = USB_ENDPOINT_XFER_BULK,
> + .wMaxPacketSize = __constant_cpu_to_le16(512),
> +};

const?

> +static struct usb_endpoint_descriptor acc_fullspeed_in_desc = {
> + .bLength = USB_DT_ENDPOINT_SIZE,
> + .bDescriptorType = USB_DT_ENDPOINT,
> + .bEndpointAddress = USB_DIR_IN,
> + .bmAttributes = USB_ENDPOINT_XFER_BULK,
> +};

const?

> +static struct usb_endpoint_descriptor acc_fullspeed_out_desc = {
> + .bLength = USB_DT_ENDPOINT_SIZE,
> + .bDescriptorType = USB_DT_ENDPOINT,
> + .bEndpointAddress = USB_DIR_OUT,
> + .bmAttributes = USB_ENDPOINT_XFER_BULK,
> +};

const?

> +static struct usb_descriptor_header *fs_acc_descs[] = {
> + (struct usb_descriptor_header *) &acc_interface_desc,
> + (struct usb_descriptor_header *) &acc_fullspeed_in_desc,
> + (struct usb_descriptor_header *) &acc_fullspeed_out_desc,
> + NULL,
> +};

const?

> +static struct usb_descriptor_header *hs_acc_descs[] = {
> + (struct usb_descriptor_header *) &acc_interface_desc,
> + (struct usb_descriptor_header *) &acc_highspeed_in_desc,
> + (struct usb_descriptor_header *) &acc_highspeed_out_desc,
> + NULL,
> +};

const?

> +/* temporary variable used between acc_open() and acc_gadget_bind() */
> +static struct acc_dev *_acc_dev;

why?

> +struct acc_instance {
> + struct usb_function_instance func_inst;
> + const char *name;
> +};
> +
> +static inline struct acc_dev *func_to_dev(struct usb_function *f)
> +{
> + return container_of(f, struct acc_dev, function);
> +}

this can be a define, but fine :-)

> +static void acc_request_free(struct usb_request *req, struct usb_ep *ep)
> +{
> + if (req) {

why would req ever be NULL here?

> +static void req_put(struct acc_dev *dev, struct list_head *head,

how about we use a more descriptive name? Usually this would be called $prefix_enqueue().

> + struct usb_request *req)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dev->lock, flags);
> + list_add_tail(&req->list, head);
> + spin_unlock_irqrestore(&dev->lock, flags);
> +}
> +
> +/* remove a request from the head of a list */
> +static struct usb_request *req_get(struct acc_dev *dev, struct list_head *head)
> +{
> + unsigned long flags;
> + struct usb_request *req;
> +
> + spin_lock_irqsave(&dev->lock, flags);
> + if (list_empty(head)) {
> + req = 0;
> + } else {
> + req = list_first_entry(head, struct usb_request, list);
> + list_del(&req->list);
> + }

list_first_entry_or_null()?

> +static void acc_complete_in(struct usb_ep *ep, struct usb_request *req)
> +{
> + struct acc_dev *dev = _acc_dev;
> +
> + if (req->status == -ESHUTDOWN) {
> + pr_debug("acc_complete_in set disconnected");

yeah, we need something with a dev* for these prints.

> +static int acc_hid_start(struct hid_device *hid)
> +{
> + return 0;
> +}
> +
> +static void acc_hid_stop(struct hid_device *hid)
> +{
> +}
> +
> +static int acc_hid_open(struct hid_device *hid)
> +{
> + return 0;
> +}
> +
> +static void acc_hid_close(struct hid_device *hid)
> +{
> +}
> +
> +static int acc_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
> + __u8 *buf, size_t len, unsigned char rtype, int reqtype)
> +{
> + return 0;
> +}

what's with all the unimplemented methods?

> +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;

add this part as a default on the switch above?

> +
> + 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");

no printk() calls!

why couldn't you get your acc_dev from fp->private_data?

> + if (atomic_xchg(&_acc_dev->open_excl, 1))

do you really need this to be an atomic_t?

> +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;
> +
> +/*
> + printk(KERN_INFO "acc_ctrlrequest "
> + "%02x.%02x v%04x i%04x l%u\n",
> + b_requestType, b_request,
> + w_value, w_index, w_length);
> +*/

commented out code? Please remove.

> + if (b_requestType == (USB_DIR_OUT | USB_TYPE_VENDOR)) {
> + if (b_request == ACCESSORY_START) {

looks like a switch statement is fitting here?

> +EXPORT_SYMBOL_GPL(acc_ctrlrequest);

why do you export this for any caller? Who is going to call this?

> +void acc_disconnect(void)
> +{
> + /* unregister all HID devices if USB is disconnected */
> + kill_all_hid_devices(_acc_dev);
> +}
> +EXPORT_SYMBOL_GPL(acc_disconnect);

why exported?

--
balbi

Attachment: signature.asc
Description: PGP signature

Next message: Alan Stern: "Re: [linux-safety] [PATCH] usb: host: ehci-sched: add comment about find_tt() not returning error"
Previous message: Ingo Molnar: "[GIT PULL] Static calls for v5.10"
In reply to: Greg KH: "Re: [PATCH 1/3] ANDROID: usb: gadget: f_accessory: Add Android Accessory function"
Next in thread: rickyniu: "[PATCH 2/3] ANDROID: USB: f_accessory: Check dev pointer before decoding ctrl request"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]