Re: [PATCH RFC] drivers: most: add USB adapter driver

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, May 11, 2020 at 11:51:15AM +0200, Christian Gromm wrote:
> This patch adds the MOST USB adapter driver to the stable branch. This is
> a follow-up to commit <b276527>.

I do not understand the "a follow-up..." sentance.  Always use the
format of:
	b27652753918 ("staging: most: move core files out of the staging area")
when writing kernel commits in changelogs.

Also, that commit doesn't really mean anything here, this is a
stand-alone driver for the most subsystem.  This changelog needs work.

> 
> Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> ---
>  drivers/most/Kconfig          |    6 +
>  drivers/most/Makefile         |    2 +
>  drivers/most/usb/Kconfig      |   14 +
>  drivers/most/usb/Makefile     |    4 +
>  drivers/most/usb/usb.c        | 1262 +++++++++++++++++++++++++++++++++++++++++

Why not just call this file most-usb.c so you don't have to do the
2-step Makefile work.  Also, why a whole subdir for a single .c file?

>  drivers/staging/most/Kconfig  |    2 -
>  drivers/staging/most/Makefile |    1 -

Why touch the staging directory for this patch?  We can delete the old
driver after this one is merged, no need for that here.

> diff --git a/drivers/most/usb/usb.c b/drivers/most/usb/usb.c
> new file mode 100644
> index 0000000..daa5e4b
> --- /dev/null
> +++ b/drivers/most/usb/usb.c
> @@ -0,0 +1,1262 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * usb.c - Hardware dependent module for USB
> + *
> + * Copyright (C) 2013-2015 Microchip Technology Germany II GmbH & Co. KG
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

You shouldn't need any pr_*() calls because this is a driver and you
always have access to the struct device * it controls.  So drop this and
fix up the remaining pr_*() calls to be dev_*() instead.

> +#include <linux/module.h>
> +#include <linux/fs.h>
> +#include <linux/usb.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/completion.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock.h>
> +#include <linux/interrupt.h>
> +#include <linux/workqueue.h>
> +#include <linux/sysfs.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/etherdevice.h>
> +#include <linux/uaccess.h>
> +#include <linux/most.h>
> +
> +#define USB_MTU			512
> +#define NO_ISOCHRONOUS_URB	0
> +#define AV_PACKETS_PER_XACT	2
> +#define BUF_CHAIN_SIZE		0xFFFF
> +#define MAX_NUM_ENDPOINTS	30
> +#define MAX_SUFFIX_LEN		10
> +#define MAX_STRING_LEN		80
> +#define MAX_BUF_SIZE		0xFFFF
> +
> +#define USB_VENDOR_ID_SMSC	0x0424  /* VID: SMSC */
> +#define USB_DEV_ID_BRDG		0xC001  /* PID: USB Bridge */
> +#define USB_DEV_ID_OS81118	0xCF18  /* PID: USB OS81118 */
> +#define USB_DEV_ID_OS81119	0xCF19  /* PID: USB OS81119 */
> +#define USB_DEV_ID_OS81210	0xCF30  /* PID: USB OS81210 */
> +/* DRCI Addresses */
> +#define DRCI_REG_NI_STATE	0x0100
> +#define DRCI_REG_PACKET_BW	0x0101
> +#define DRCI_REG_NODE_ADDR	0x0102
> +#define DRCI_REG_NODE_POS	0x0103
> +#define DRCI_REG_MEP_FILTER	0x0140
> +#define DRCI_REG_HASH_TBL0	0x0141
> +#define DRCI_REG_HASH_TBL1	0x0142
> +#define DRCI_REG_HASH_TBL2	0x0143
> +#define DRCI_REG_HASH_TBL3	0x0144
> +#define DRCI_REG_HW_ADDR_HI	0x0145
> +#define DRCI_REG_HW_ADDR_MI	0x0146
> +#define DRCI_REG_HW_ADDR_LO	0x0147
> +#define DRCI_REG_BASE		0x1100
> +#define DRCI_COMMAND		0x02
> +#define DRCI_READ_REQ		0xA0
> +#define DRCI_WRITE_REQ		0xA1
> +
> +/**
> + * struct most_dci_obj - Direct Communication Interface
> + * @kobj:position in sysfs
> + * @usb_device: pointer to the usb device
> + * @reg_addr: register address for arbitrary DCI access
> + */
> +struct most_dci_obj {
> +	struct device dev;

Wait, why is a USB driver creating something with a separate struct
device embedded in it?  Shouldn't the most core handle stuff like this?

> +	struct usb_device *usb_device;
> +	u16 reg_addr;
> +};
> +
> +#define to_dci_obj(p) container_of(p, struct most_dci_obj, dev)
> +
> +struct most_dev;

Don't you already have this in the most.h file?

> +
> +struct clear_hold_work {
> +	struct work_struct ws;
> +	struct most_dev *mdev;
> +	unsigned int channel;
> +	int pipe;
> +};
> +
> +#define to_clear_hold_work(w) container_of(w, struct clear_hold_work, ws)
> +
> +/**
> + * struct most_dev - holds all usb interface specific stuff
> + * @usb_device: pointer to usb device
> + * @iface: hardware interface
> + * @cap: channel capabilities
> + * @conf: channel configuration
> + * @dci: direct communication interface of hardware
> + * @ep_address: endpoint address table
> + * @description: device description
> + * @suffix: suffix for channel name
> + * @channel_lock: synchronize channel access
> + * @padding_active: indicates channel uses padding
> + * @is_channel_healthy: health status table of each channel
> + * @busy_urbs: list of anchored items
> + * @io_mutex: synchronize I/O with disconnect
> + * @link_stat_timer: timer for link status reports
> + * @poll_work_obj: work for polling link status
> + */
> +struct most_dev {
> +	struct device dev;
> +	struct usb_device *usb_device;
> +	struct most_interface iface;
> +	struct most_channel_capability *cap;
> +	struct most_channel_config *conf;
> +	struct most_dci_obj *dci;
> +	u8 *ep_address;
> +	char description[MAX_STRING_LEN];
> +	char suffix[MAX_NUM_ENDPOINTS][MAX_SUFFIX_LEN];
> +	spinlock_t channel_lock[MAX_NUM_ENDPOINTS]; /* sync channel access */
> +	bool padding_active[MAX_NUM_ENDPOINTS];
> +	bool is_channel_healthy[MAX_NUM_ENDPOINTS];
> +	struct clear_hold_work clear_work[MAX_NUM_ENDPOINTS];
> +	struct usb_anchor *busy_urbs;
> +	struct mutex io_mutex;
> +	struct timer_list link_stat_timer;
> +	struct work_struct poll_work_obj;
> +	void (*on_netinfo)(struct most_interface *most_iface,
> +			   unsigned char link_state, unsigned char *addrs);
> +};

Why isn't this all in the most core?  Why does the USB adapter need to
do this?

> +/**
> + * hdm_read_completion - completion function for submitted Rx URBs
> + * @urb: the URB that has been completed
> + *
> + * This checks the status of the completed URB. In case the URB has been
> + * unlinked before it is immediately freed. On any other error the MBO transfer
> + * flag is set. On success it frees allocated resources, removes
> + * padding bytes -if necessary- and calls the completion function.
> + *
> + * Context: interrupt!
> + *
> + * **************************************************************************
> + *                   Error codes returned by in urb->status
> + *                   or in iso_frame_desc[n].status (for ISO)
> + * *************************************************************************
> + *
> + * USB device drivers may only test urb status values in completion handlers.
> + * This is because otherwise there would be a race between HCDs updating
> + * these values on one CPU, and device drivers testing them on another CPU.
> + *
> + * A transfer's actual_length may be positive even when an error has been
> + * reported.  That's because transfers often involve several packets, so that
> + * one or more packets could finish before an error stops further endpoint I/O.
> + *
> + * For isochronous URBs, the urb status value is non-zero only if the URB is
> + * unlinked, the device is removed, the host controller is disabled or the total
> + * transferred length is less than the requested length and the URB_SHORT_NOT_OK
> + * flag is set.  Completion handlers for isochronous URBs should only see
> + * urb->status set to zero, -ENOENT, -ECONNRESET, -ESHUTDOWN, or -EREMOTEIO.
> + * Individual frame descriptor status fields may report more status codes.
> + *
> + *
> + * 0			Transfer completed successfully
> + *
> + * -ENOENT		URB was synchronously unlinked by usb_unlink_urb
> + *
> + * -EINPROGRESS		URB still pending, no results yet
> + *			(That is, if drivers see this it's a bug.)
> + *
> + * -EPROTO (*, **)	a) bitstuff error
> + *			b) no response packet received within the
> + *			   prescribed bus turn-around time
> + *			c) unknown USB error
> + *
> + * -EILSEQ (*, **)	a) CRC mismatch
> + *			b) no response packet received within the
> + *			   prescribed bus turn-around time
> + *			c) unknown USB error
> + *
> + *			Note that often the controller hardware does not
> + *			distinguish among cases a), b), and c), so a
> + *			driver cannot tell whether there was a protocol
> + *			error, a failure to respond (often caused by
> + *			device disconnect), or some other fault.
> + *
> + * -ETIME (**)		No response packet received within the prescribed
> + *			bus turn-around time.  This error may instead be
> + *			reported as -EPROTO or -EILSEQ.
> + *
> + * -ETIMEDOUT		Synchronous USB message functions use this code
> + *			to indicate timeout expired before the transfer
> + *			completed, and no other error was reported by HC.
> + *
> + * -EPIPE (**)		Endpoint stalled.  For non-control endpoints,
> + *			reset this status with usb_clear_halt().
> + *
> + * -ECOMM		During an IN transfer, the host controller
> + *			received data from an endpoint faster than it
> + *			could be written to system memory
> + *
> + * -ENOSR		During an OUT transfer, the host controller
> + *			could not retrieve data from system memory fast
> + *			enough to keep up with the USB data rate
> + *
> + * -EOVERFLOW (*)	The amount of data returned by the endpoint was
> + *			greater than either the max packet size of the
> + *			endpoint or the remaining buffer size.  "Babble".
> + *
> + * -EREMOTEIO		The data read from the endpoint did not fill the
> + *			specified buffer, and URB_SHORT_NOT_OK was set in
> + *			urb->transfer_flags.
> + *
> + * -ENODEV		Device was removed.  Often preceded by a burst of
> + *			other errors, since the hub driver doesn't detect
> + *			device removal events immediately.
> + *
> + * -EXDEV		ISO transfer only partially completed
> + *			(only set in iso_frame_desc[n].status, not urb->status)
> + *
> + * -EINVAL		ISO madness, if this happens: Log off and go home
> + *
> + * -ECONNRESET		URB was asynchronously unlinked by usb_unlink_urb
> + *
> + * -ESHUTDOWN		The device or host controller has been disabled due
> + *			to some problem that could not be worked around,
> + *			such as a physical disconnect.
> + *
> + *
> + * (*) Error codes like -EPROTO, -EILSEQ and -EOVERFLOW normally indicate
> + * hardware problems such as bad devices (including firmware) or cables.
> + *
> + * (**) This is also one of several codes that different kinds of host
> + * controller use to indicate a transfer has failed because of device
> + * disconnect.  In the interval before the hub driver starts disconnect
> + * processing, devices may receive such fault reports for every request.
> + *
> + * See <https://www.kernel.org/doc/Documentation/driver-api/usb/error-codes.rst>

No need to provide a whole copy of this existing kernel file here in a
comment, right?

> +static ssize_t value_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	const char *name = attr->attr.name;
> +	struct most_dci_obj *dci_obj = to_dci_obj(dev);
> +	u16 val;
> +	u16 reg_addr;
> +	int err;
> +
> +	if (!strcmp(name, "arb_address"))
> +		return snprintf(buf, PAGE_SIZE, "%04x\n", dci_obj->reg_addr);
> +
> +	if (!strcmp(name, "arb_value"))
> +		reg_addr = dci_obj->reg_addr;
> +	else if (get_static_reg_addr(ro_regs, name, &reg_addr) &&
> +		 get_static_reg_addr(rw_regs, name, &reg_addr))
> +		return -EFAULT;
> +
> +	err = drci_rd_reg(dci_obj->usb_device, reg_addr, &val);
> +	if (err < 0)
> +		return err;
> +
> +	return snprintf(buf, PAGE_SIZE, "%04x\n", val);
> +}
> +
> +static ssize_t value_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	u16 val;
> +	u16 reg_addr;
> +	const char *name = attr->attr.name;
> +	struct most_dci_obj *dci_obj = to_dci_obj(dev);
> +	struct usb_device *usb_dev = dci_obj->usb_device;
> +	int err = kstrtou16(buf, 16, &val);
> +
> +	if (err)
> +		return err;
> +
> +	if (!strcmp(name, "arb_address")) {
> +		dci_obj->reg_addr = val;
> +		return count;
> +	}
> +
> +	if (!strcmp(name, "arb_value"))
> +		err = drci_wr_reg(usb_dev, dci_obj->reg_addr, val);
> +	else if (!strcmp(name, "sync_ep"))
> +		err = start_sync_ep(usb_dev, val);
> +	else if (!get_static_reg_addr(rw_regs, name, &reg_addr))
> +		err = drci_wr_reg(usb_dev, reg_addr, val);
> +	else
> +		return -EFAULT;
> +
> +	if (err < 0)
> +		return err;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(ni_state, 0444, value_show, NULL);
> +static DEVICE_ATTR(packet_bandwidth, 0444, value_show, NULL);
> +static DEVICE_ATTR(node_address, 0444, value_show, NULL);
> +static DEVICE_ATTR(node_position, 0444, value_show, NULL);
> +static DEVICE_ATTR(sync_ep, 0200, NULL, value_store);
> +static DEVICE_ATTR(mep_filter, 0644, value_show, value_store);
> +static DEVICE_ATTR(mep_hash0, 0644, value_show, value_store);
> +static DEVICE_ATTR(mep_hash1, 0644, value_show, value_store);
> +static DEVICE_ATTR(mep_hash2, 0644, value_show, value_store);
> +static DEVICE_ATTR(mep_hash3, 0644, value_show, value_store);
> +static DEVICE_ATTR(mep_eui48_hi, 0644, value_show, value_store);
> +static DEVICE_ATTR(mep_eui48_mi, 0644, value_show, value_store);
> +static DEVICE_ATTR(mep_eui48_lo, 0644, value_show, value_store);
> +static DEVICE_ATTR(arb_address, 0644, value_show, value_store);
> +static DEVICE_ATTR(arb_value, 0644, value_show, value_store);

Loads of sysfs files with no documentation for them?


> +
> +static struct attribute *dci_attrs[] = {
> +	&dev_attr_ni_state.attr,
> +	&dev_attr_packet_bandwidth.attr,
> +	&dev_attr_node_address.attr,
> +	&dev_attr_node_position.attr,
> +	&dev_attr_sync_ep.attr,
> +	&dev_attr_mep_filter.attr,
> +	&dev_attr_mep_hash0.attr,
> +	&dev_attr_mep_hash1.attr,
> +	&dev_attr_mep_hash2.attr,
> +	&dev_attr_mep_hash3.attr,
> +	&dev_attr_mep_eui48_hi.attr,
> +	&dev_attr_mep_eui48_mi.attr,
> +	&dev_attr_mep_eui48_lo.attr,
> +	&dev_attr_arb_address.attr,
> +	&dev_attr_arb_value.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group dci_attr_group = {
> +	.attrs = dci_attrs,
> +};
> +
> +static const struct attribute_group *dci_attr_groups[] = {
> +	&dci_attr_group,
> +	NULL,
> +};

ATTRIBUTE_GROUPS()?

> +
> +static void release_dci(struct device *dev)
> +{
> +	struct most_dci_obj *dci = to_dci_obj(dev);
> +
> +	kfree(dci);
> +}
> +
> +static void release_mdev(struct device *dev)
> +{
> +	struct most_dev *mdev = to_mdev_from_dev(dev);
> +
> +	kfree(mdev);
> +}
> +/**
> + * hdm_probe - probe function of USB device driver
> + * @interface: Interface of the attached USB device
> + * @id: Pointer to the USB ID table.
> + *
> + * This allocates and initializes the device instance, adds the new
> + * entry to the internal list, scans the USB descriptors and registers
> + * the interface with the core.
> + * Additionally, the DCI objects are created and the hardware is sync'd.
> + *
> + * Return 0 on success. In case of an error a negative number is returned.
> + */
> +static int
> +hdm_probe(struct usb_interface *interface, const struct usb_device_id *id)
> +{
> +	struct usb_host_interface *usb_iface_desc = interface->cur_altsetting;
> +	struct usb_device *usb_dev = interface_to_usbdev(interface);
> +	struct device *dev = &usb_dev->dev;
> +	struct most_dev *mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> +	unsigned int i;
> +	unsigned int num_endpoints;
> +	struct most_channel_capability *tmp_cap;
> +	struct usb_endpoint_descriptor *ep_desc;
> +	int ret = 0;
> +
> +	if (!mdev)
> +		goto err_out_of_memory;
> +
> +	usb_set_intfdata(interface, mdev);
> +	num_endpoints = usb_iface_desc->desc.bNumEndpoints;

No error checking if you have the wrong number of expected endpoints
because you have a "fake" device plugged in?  The syzbot fuzzer will
test this eventually...


> +	mutex_init(&mdev->io_mutex);
> +	INIT_WORK(&mdev->poll_work_obj, wq_netinfo);
> +	timer_setup(&mdev->link_stat_timer, link_stat_timer_handler, 0);
> +
> +	mdev->usb_device = usb_dev;
> +	mdev->link_stat_timer.expires = jiffies + (2 * HZ);
> +
> +	mdev->iface.mod = hdm_usb_fops.owner;
> +	mdev->iface.dev = &mdev->dev;
> +	mdev->iface.driver_dev = &interface->dev;
> +	mdev->iface.interface = ITYPE_USB;
> +	mdev->iface.configure = hdm_configure_channel;
> +	mdev->iface.request_netinfo = hdm_request_netinfo;
> +	mdev->iface.enqueue = hdm_enqueue;
> +	mdev->iface.poison_channel = hdm_poison_channel;
> +	mdev->iface.dma_alloc = hdm_dma_alloc;
> +	mdev->iface.dma_free = hdm_dma_free;
> +	mdev->iface.description = mdev->description;
> +	mdev->iface.num_channels = num_endpoints;
> +
> +	snprintf(mdev->description, sizeof(mdev->description),
> +		 "%d-%s:%d.%d",
> +		 usb_dev->bus->busnum,
> +		 usb_dev->devpath,
> +		 usb_dev->config->desc.bConfigurationValue,
> +		 usb_iface_desc->desc.bInterfaceNumber);
> +
> +	mdev->dev.init_name = mdev->description;
> +	mdev->dev.parent = &interface->dev;
> +	mdev->dev.release = release_mdev;

I find it odd a USB driver creates another device, I thought the most
core handled this.

> +	mdev->conf = kcalloc(num_endpoints, sizeof(*mdev->conf), GFP_KERNEL);
> +	if (!mdev->conf)
> +		goto err_free_mdev;
> +
> +	mdev->cap = kcalloc(num_endpoints, sizeof(*mdev->cap), GFP_KERNEL);
> +	if (!mdev->cap)
> +		goto err_free_conf;
> +
> +	mdev->iface.channel_vector = mdev->cap;
> +	mdev->ep_address =
> +		kcalloc(num_endpoints, sizeof(*mdev->ep_address), GFP_KERNEL);
> +	if (!mdev->ep_address)
> +		goto err_free_cap;
> +
> +	mdev->busy_urbs =
> +		kcalloc(num_endpoints, sizeof(*mdev->busy_urbs), GFP_KERNEL);
> +	if (!mdev->busy_urbs)
> +		goto err_free_ep_address;
> +
> +	tmp_cap = mdev->cap;
> +	for (i = 0; i < num_endpoints; i++) {
> +		ep_desc = &usb_iface_desc->endpoint[i].desc;
> +		mdev->ep_address[i] = ep_desc->bEndpointAddress;
> +		mdev->padding_active[i] = false;
> +		mdev->is_channel_healthy[i] = true;
> +
> +		snprintf(&mdev->suffix[i][0], MAX_SUFFIX_LEN, "ep%02x",
> +			 mdev->ep_address[i]);
> +
> +		tmp_cap->name_suffix = &mdev->suffix[i][0];
> +		tmp_cap->buffer_size_packet = MAX_BUF_SIZE;
> +		tmp_cap->buffer_size_streaming = MAX_BUF_SIZE;
> +		tmp_cap->num_buffers_packet = BUF_CHAIN_SIZE;
> +		tmp_cap->num_buffers_streaming = BUF_CHAIN_SIZE;
> +		tmp_cap->data_type = MOST_CH_CONTROL | MOST_CH_ASYNC |
> +				     MOST_CH_ISOC | MOST_CH_SYNC;
> +		if (usb_endpoint_dir_in(ep_desc))
> +			tmp_cap->direction = MOST_CH_RX;
> +		else
> +			tmp_cap->direction = MOST_CH_TX;
> +		tmp_cap++;
> +		init_usb_anchor(&mdev->busy_urbs[i]);
> +		spin_lock_init(&mdev->channel_lock[i]);
> +	}
> +	dev_notice(dev, "claimed gadget: Vendor=%4.4x ProdID=%4.4x Bus=%02x Device=%02x\n",
> +		   le16_to_cpu(usb_dev->descriptor.idVendor),
> +		   le16_to_cpu(usb_dev->descriptor.idProduct),
> +		   usb_dev->bus->busnum,
> +		   usb_dev->devnum);

That's really loud, why?  If drivers work properly, they should be
quiet, make this a debug call please.

> +
> +	dev_notice(dev, "device path: /sys/bus/usb/devices/%d-%s:%d.%d\n",
> +		   usb_dev->bus->busnum,
> +		   usb_dev->devpath,
> +		   usb_dev->config->desc.bConfigurationValue,
> +		   usb_iface_desc->desc.bInterfaceNumber);

Same here.

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel