Message-ID: <87blh7l9lo.fsf@kernel.org>
Date:   Mon, 12 Oct 2020 18:58:27 +0300
From:   Felipe Balbi <balbi@...nel.org>
To:     rickyniu <rickyniu@...gle.com>, gregkh@...uxfoundation.org,
        astrachan@...gle.com, rickyniu@...gle.com, amit.pundir@...aro.org,
        lockwood@...roid.com, benoit@...roid.com, jackp@...eaurora.org,
        vvreddy@...eaurora.org
Cc:     linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
        kyletso@...gle.com
Subject: Re: [PATCH 1/3] ANDROID: usb: gadget: f_accessory: Add Android
 Accessory function


Hi,

rickyniu <rickyniu@...gle.com> writes:
> From: Benoit Goby <benoit@...roid.com>

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@...roid.com>
> [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@...aro.org>
> [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@...gle.com>
> Signed-off-by: rickyniu <rickyniu@...gle.com>

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@...roid.com>
> + *
> + * 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

Download attachment "signature.asc" of type "application/pgp-signature" (858 bytes)

Powered by blists - more mailing lists