On Thu, May 14, 2020 at 11:52:49AM +0200, Christian Gromm wrote: > This patch adds the usb driver source file most_usb.c and > modifies the Makefile and Kconfig accordingly. > > Signed-off-by: Christian Gromm <christian.gromm@xxxxxxxxxxxxx> > --- > v2: > Reported-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > - don't remove usb driver from staging area > - don't touch staging/most/Kconfig > - remove subdirectory for USB driver and put source file into > drivers/most > > drivers/most/Kconfig | 12 + > drivers/most/Makefile | 2 + > drivers/most/most_usb.c | 1262 +++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 1276 insertions(+) > create mode 100644 drivers/most/most_usb.c > > diff --git a/drivers/most/Kconfig b/drivers/most/Kconfig > index 58d7999..8650683 100644 > --- a/drivers/most/Kconfig > +++ b/drivers/most/Kconfig > @@ -13,3 +13,15 @@ menuconfig MOST > module will be called most_core. > > If in doubt, say N here. > + > +if MOST > +config MOST_USB > + tristate "USB" > + depends on USB && NET > + help > + Say Y here if you want to connect via USB to network tranceiver. > + This device driver depends on the networking AIM. > + > + To compile this driver as a module, choose M here: the > + module will be called most_usb. > +endif > diff --git a/drivers/most/Makefile b/drivers/most/Makefile > index e810cd3..97ffc06 100644 > --- a/drivers/most/Makefile > +++ b/drivers/most/Makefile > @@ -2,3 +2,5 @@ > obj-$(CONFIG_MOST) += most_core.o > most_core-y := core.o \ > configfs.o > + > +obj-$(CONFIG_MOST_USB) += most_usb.o > diff --git a/drivers/most/most_usb.c b/drivers/most/most_usb.c > new file mode 100644 > index 0000000..daa5e4b > --- /dev/null > +++ b/drivers/most/most_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 > +#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; > + struct usb_device *usb_device; > + u16 reg_addr; > +}; > + > +#define to_dci_obj(p) container_of(p, struct most_dci_obj, dev) > + > +struct most_dev; > + > +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); > +}; > + > +#define to_mdev(d) container_of(d, struct most_dev, iface) > +#define to_mdev_from_dev(d) container_of(d, struct most_dev, dev) > +#define to_mdev_from_work(w) container_of(w, struct most_dev, poll_work_obj) > + > +static void wq_clear_halt(struct work_struct *wq_obj); > +static void wq_netinfo(struct work_struct *wq_obj); > + > +/** > + * drci_rd_reg - read a DCI register > + * @dev: usb device > + * @reg: register address > + * @buf: buffer to store data > + * > + * This is reads data from INIC's direct register communication interface > + */ > +static inline int drci_rd_reg(struct usb_device *dev, u16 reg, u16 *buf) > +{ > + int retval; > + __le16 *dma_buf = kzalloc(sizeof(*dma_buf), GFP_KERNEL); I really hate allocations hidden in the declaration block. :/ > + u8 req_type = USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE; > + > + if (!dma_buf) > + return -ENOMEM; > + > + retval = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), > + DRCI_READ_REQ, req_type, > + 0x0000, > + reg, dma_buf, sizeof(*dma_buf), 5 * HZ); > + *buf = le16_to_cpu(*dma_buf); > + kfree(dma_buf); > + > + return retval; Why bother returning positive values here when none of the callers use that? > +} > + > +/** > + * drci_wr_reg - write a DCI register > + * @dev: usb device > + * @reg: register address > + * @data: data to write > + * > + * This is writes data to INIC's direct register communication interface > + */ > +static inline int drci_wr_reg(struct usb_device *dev, u16 reg, u16 data) > +{ > + return usb_control_msg(dev, > + usb_sndctrlpipe(dev, 0), > + DRCI_WRITE_REQ, > + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > + data, > + reg, > + NULL, > + 0, > + 5 * HZ); > +} > + > +static inline int start_sync_ep(struct usb_device *usb_dev, u16 ep) > +{ > + return drci_wr_reg(usb_dev, DRCI_REG_BASE + DRCI_COMMAND + ep * 16, 1); > +} > + > +/** > + * get_stream_frame_size - calculate frame size of current configuration > + * @cfg: channel configuration > + */ > +static unsigned int get_stream_frame_size(struct most_channel_config *cfg) > +{ > + unsigned int frame_size = 0; > + unsigned int sub_size = cfg->subbuffer_size; > + > + if (!sub_size) { > + pr_warn("Misconfig: Subbuffer size zero.\n"); > + return frame_size; I wish this were a "return 0;" > + } > + switch (cfg->data_type) { > + case MOST_CH_ISOC: > + frame_size = AV_PACKETS_PER_XACT * sub_size; > + break; > + case MOST_CH_SYNC: > + if (cfg->packets_per_xact == 0) { > + pr_warn("Misconfig: Packets per XACT zero\n"); > + frame_size = 0; > + } else if (cfg->packets_per_xact == 0xFF) { > + frame_size = (USB_MTU / sub_size) * sub_size; > + } else { > + frame_size = cfg->packets_per_xact * sub_size; > + } > + break; > + default: > + pr_warn("Query frame size of non-streaming channel\n"); > + break; > + } > + return frame_size; > +} > + > +/** > + * hdm_poison_channel - mark buffers of this channel as invalid > + * @iface: pointer to the interface > + * @channel: channel ID > + * > + * This unlinks all URBs submitted to the HCD, > + * calls the associated completion function of the core and removes > + * them from the list. > + * > + * Returns 0 on success or error code otherwise. > + */ > +static int hdm_poison_channel(struct most_interface *iface, int channel) > +{ > + struct most_dev *mdev = to_mdev(iface); > + unsigned long flags; > + spinlock_t *lock; /* temp. lock */ > + > + if (channel < 0 || channel >= iface->num_channels) { > + dev_warn(&mdev->usb_device->dev, "Channel ID out of range.\n"); > + return -ECHRNG; > + } > + > + lock = mdev->channel_lock + channel; > + spin_lock_irqsave(lock, flags); > + mdev->is_channel_healthy[channel] = false; > + spin_unlock_irqrestore(lock, flags); > + > + cancel_work_sync(&mdev->clear_work[channel].ws); > + > + mutex_lock(&mdev->io_mutex); > + usb_kill_anchored_urbs(&mdev->busy_urbs[channel]); > + if (mdev->padding_active[channel]) > + mdev->padding_active[channel] = false; > + > + if (mdev->conf[channel].data_type == MOST_CH_ASYNC) { > + del_timer_sync(&mdev->link_stat_timer); > + cancel_work_sync(&mdev->poll_work_obj); > + } > + mutex_unlock(&mdev->io_mutex); > + return 0; > +} > + > +/** > + * hdm_add_padding - add padding bytes > + * @mdev: most device > + * @channel: channel ID > + * @mbo: buffer object > + * > + * This inserts the INIC hardware specific padding bytes into a streaming > + * channel's buffer > + */ > +static int hdm_add_padding(struct most_dev *mdev, int channel, struct mbo *mbo) > +{ > + struct most_channel_config *conf = &mdev->conf[channel]; > + unsigned int frame_size = get_stream_frame_size(conf); > + unsigned int j, num_frames; > + > + if (!frame_size) > + return -EINVAL; > + num_frames = mbo->buffer_length / frame_size; > + > + if (num_frames < 1) { > + dev_err(&mdev->usb_device->dev, > + "Missed minimal transfer unit.\n"); > + return -EINVAL; > + } > + > + for (j = num_frames - 1; j > 0; j--) > + memmove(mbo->virt_address + j * USB_MTU, > + mbo->virt_address + j * frame_size, > + frame_size); > + mbo->buffer_length = num_frames * USB_MTU; > + return 0; > +} > + > +/** > + * hdm_remove_padding - remove padding bytes > + * @mdev: most device > + * @channel: channel ID > + * @mbo: buffer object > + * > + * This takes the INIC hardware specific padding bytes off a streaming > + * channel's buffer. > + */ > +static int hdm_remove_padding(struct most_dev *mdev, int channel, > + struct mbo *mbo) > +{ > + struct most_channel_config *const conf = &mdev->conf[channel]; > + unsigned int frame_size = get_stream_frame_size(conf); > + unsigned int j, num_frames; > + > + if (!frame_size) > + return -EINVAL; > + num_frames = mbo->processed_length / USB_MTU; > + > + for (j = 1; j < num_frames; j++) > + memmove(mbo->virt_address + frame_size * j, > + mbo->virt_address + USB_MTU * j, > + frame_size); > + > + mbo->processed_length = frame_size * num_frames; > + return 0; > +} > + > +/** > + * hdm_write_completion - completion function for submitted Tx 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 and calls > + * the completion function. > + * > + * Context: interrupt! > + */ > +static void hdm_write_completion(struct urb *urb) > +{ > + struct mbo *mbo = urb->context; > + struct most_dev *mdev = to_mdev(mbo->ifp); > + unsigned int channel = mbo->hdm_channel_id; > + spinlock_t *lock = mdev->channel_lock + channel; > + unsigned long flags; > + > + spin_lock_irqsave(lock, flags); > + > + mbo->processed_length = 0; > + mbo->status = MBO_E_INVAL; > + if (likely(mdev->is_channel_healthy[channel])) { > + switch (urb->status) { > + case 0: > + case -ESHUTDOWN: > + mbo->processed_length = urb->actual_length; > + mbo->status = MBO_SUCCESS; > + break; > + case -EPIPE: > + dev_warn(&mdev->usb_device->dev, > + "Broken pipe on ep%02x\n", > + mdev->ep_address[channel]); > + mdev->is_channel_healthy[channel] = false; > + mdev->clear_work[channel].pipe = urb->pipe; > + schedule_work(&mdev->clear_work[channel].ws); > + break; > + case -ENODEV: > + case -EPROTO: > + mbo->status = MBO_E_CLOSE; > + break; > + } > + } > + > + spin_unlock_irqrestore(lock, flags); > + > + if (likely(mbo->complete)) > + mbo->complete(mbo); > + usb_free_urb(urb); > +} > + > +/** > + * 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> > + */ > +static void hdm_read_completion(struct urb *urb) > +{ > + struct mbo *mbo = urb->context; > + struct most_dev *mdev = to_mdev(mbo->ifp); > + unsigned int channel = mbo->hdm_channel_id; > + struct device *dev = &mdev->usb_device->dev; > + spinlock_t *lock = mdev->channel_lock + channel; > + unsigned long flags; > + > + spin_lock_irqsave(lock, flags); > + > + mbo->processed_length = 0; > + mbo->status = MBO_E_INVAL; > + if (likely(mdev->is_channel_healthy[channel])) { > + switch (urb->status) { > + case 0: > + case -ESHUTDOWN: > + mbo->processed_length = urb->actual_length; > + mbo->status = MBO_SUCCESS; > + if (mdev->padding_active[channel] && > + hdm_remove_padding(mdev, channel, mbo)) { > + mbo->processed_length = 0; > + mbo->status = MBO_E_INVAL; > + } > + break; > + case -EPIPE: > + dev_warn(dev, "Broken pipe on ep%02x\n", > + mdev->ep_address[channel]); > + mdev->is_channel_healthy[channel] = false; > + mdev->clear_work[channel].pipe = urb->pipe; > + schedule_work(&mdev->clear_work[channel].ws); > + break; > + case -ENODEV: > + case -EPROTO: > + mbo->status = MBO_E_CLOSE; > + break; > + case -EOVERFLOW: > + dev_warn(dev, "Babble on ep%02x\n", > + mdev->ep_address[channel]); > + break; > + } > + } > + > + spin_unlock_irqrestore(lock, flags); > + > + if (likely(mbo->complete)) > + mbo->complete(mbo); > + usb_free_urb(urb); > +} > + > +/** > + * hdm_enqueue - receive a buffer to be used for data transfer > + * @iface: interface to enqueue to > + * @channel: ID of the channel > + * @mbo: pointer to the buffer object > + * > + * This allocates a new URB and fills it according to the channel > + * that is being used for transmission of data. Before the URB is > + * submitted it is stored in the private anchor list. > + * > + * Returns 0 on success. On any error the URB is freed and a error code > + * is returned. > + * > + * Context: Could in _some_ cases be interrupt! > + */ > +static int hdm_enqueue(struct most_interface *iface, int channel, > + struct mbo *mbo) > +{ > + struct most_dev *mdev = to_mdev(iface); > + struct most_channel_config *conf; > + int retval = 0; > + struct urb *urb; > + unsigned long length; > + void *virt_address; > + > + if (!mbo) > + return -EINVAL; > + if (iface->num_channels <= channel || channel < 0) > + return -ECHRNG; > + > + conf = &mdev->conf[channel]; > + > + mutex_lock(&mdev->io_mutex); > + if (!mdev->usb_device) { > + retval = -ENODEV; > + goto unlock_io_mutex; > + } > + > + urb = usb_alloc_urb(NO_ISOCHRONOUS_URB, GFP_ATOMIC); You could move this before the mutex_lock(). Would GFP_ATOMIC still be required? > + if (!urb) { > + retval = -ENOMEM; > + goto unlock_io_mutex; > + } > + > + if ((conf->direction & MOST_CH_TX) && mdev->padding_active[channel] && > + hdm_add_padding(mdev, channel, mbo)) { > + retval = -EINVAL; > + goto err_free_urb; > + } > + > + urb->transfer_dma = mbo->bus_address; > + virt_address = mbo->virt_address; > + length = mbo->buffer_length; > + > + if (conf->direction & MOST_CH_TX) { > + usb_fill_bulk_urb(urb, mdev->usb_device, > + usb_sndbulkpipe(mdev->usb_device, > + mdev->ep_address[channel]), > + virt_address, > + length, > + hdm_write_completion, > + mbo); > + if (conf->data_type != MOST_CH_ISOC && > + conf->data_type != MOST_CH_SYNC) > + urb->transfer_flags |= URB_ZERO_PACKET; > + } else { > + usb_fill_bulk_urb(urb, mdev->usb_device, > + usb_rcvbulkpipe(mdev->usb_device, > + mdev->ep_address[channel]), > + virt_address, > + length + conf->extra_len, > + hdm_read_completion, > + mbo); > + } > + urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; > + > + usb_anchor_urb(urb, &mdev->busy_urbs[channel]); > + > + retval = usb_submit_urb(urb, GFP_KERNEL); > + if (retval) { > + dev_err(&mdev->usb_device->dev, > + "URB submit failed with error %d.\n", retval); > + goto err_unanchor_urb; > + } > + goto unlock_io_mutex; Replace the goto with: mutex_unlock(&mdev->io_mutex); return 0; It lets you move the lock around more easily. > + > +err_unanchor_urb: > + usb_unanchor_urb(urb); > +err_free_urb: > + usb_free_urb(urb); > +unlock_io_mutex: > + mutex_unlock(&mdev->io_mutex); > + return retval; > +} > + > +static void *hdm_dma_alloc(struct mbo *mbo, u32 size) > +{ > + struct most_dev *mdev = to_mdev(mbo->ifp); > + > + return usb_alloc_coherent(mdev->usb_device, size, GFP_KERNEL, > + &mbo->bus_address); > +} > + > +static void hdm_dma_free(struct mbo *mbo, u32 size) > +{ > + struct most_dev *mdev = to_mdev(mbo->ifp); > + > + usb_free_coherent(mdev->usb_device, size, mbo->virt_address, > + mbo->bus_address); > +} > + > +/** > + * hdm_configure_channel - receive channel configuration from core > + * @iface: interface > + * @channel: channel ID > + * @conf: structure that holds the configuration information > + * > + * The attached network interface controller (NIC) supports a padding mode > + * to avoid short packets on USB, hence increasing the performance due to a > + * lower interrupt load. This mode is default for synchronous data and can > + * be switched on for isochronous data. In case padding is active the > + * driver needs to know the frame size of the payload in order to calculate > + * the number of bytes it needs to pad when transmitting or to cut off when > + * receiving data. > + * > + */ > +static int hdm_configure_channel(struct most_interface *iface, int channel, > + struct most_channel_config *conf) > +{ > + unsigned int num_frames; > + unsigned int frame_size; > + struct most_dev *mdev = to_mdev(iface); > + struct device *dev = &mdev->usb_device->dev; > + > + mdev->is_channel_healthy[channel] = true; > + mdev->clear_work[channel].channel = channel; > + mdev->clear_work[channel].mdev = mdev; > + INIT_WORK(&mdev->clear_work[channel].ws, wq_clear_halt); > + > + if (!conf) { > + dev_err(dev, "Bad config pointer.\n"); > + return -EINVAL; > + } > + if (channel < 0 || channel >= iface->num_channels) { > + dev_err(dev, "Channel ID out of range.\n"); > + return -EINVAL; > + } > + if (!conf->num_buffers || !conf->buffer_size) { > + dev_err(dev, "Misconfig: buffer size or #buffers zero.\n"); > + return -EINVAL; > + } > + > + if (conf->data_type != MOST_CH_SYNC && > + !(conf->data_type == MOST_CH_ISOC && > + conf->packets_per_xact != 0xFF)) { > + mdev->padding_active[channel] = false; > + /* > + * Since the NIC's padding mode is not going to be > + * used, we can skip the frame size calculations and > + * move directly on to exit. > + */ I absolutely hate that we're skipping the other checks... At least zero out the frame size information so that we don't save invalid data. > + goto exit; > + } > + > + mdev->padding_active[channel] = true; > + > + frame_size = get_stream_frame_size(conf); > + if (frame_size == 0 || frame_size > USB_MTU) { > + dev_warn(dev, "Misconfig: frame size wrong\n"); > + return -EINVAL; > + } > + > + num_frames = conf->buffer_size / frame_size; > + > + if (conf->buffer_size % frame_size) { > + u16 old_size = conf->buffer_size; > + > + conf->buffer_size = num_frames * frame_size; > + dev_warn(dev, "%s: fixed buffer size (%d -> %d)\n", > + mdev->suffix[channel], old_size, conf->buffer_size); > + } > + > + /* calculate extra length to comply w/ HW padding */ > + conf->extra_len = num_frames * (USB_MTU - frame_size); > + > +exit: > + mdev->conf[channel] = *conf; > + if (conf->data_type == MOST_CH_ASYNC) { > + u16 ep = mdev->ep_address[channel]; > + > + if (start_sync_ep(mdev->usb_device, ep) < 0) > + dev_warn(dev, "sync for ep%02x failed", ep); > + } > + return 0; > +} > + > +/** > + * hdm_request_netinfo - request network information > + * @iface: pointer to interface > + * @channel: channel ID > + * > + * This is used as trigger to set up the link status timer that > + * polls for the NI state of the INIC every 2 seconds. > + * > + */ > +static void hdm_request_netinfo(struct most_interface *iface, int channel, > + void (*on_netinfo)(struct most_interface *, > + unsigned char, > + unsigned char *)) > +{ > + struct most_dev *mdev = to_mdev(iface); > + > + mdev->on_netinfo = on_netinfo; > + if (!on_netinfo) > + return; > + > + mdev->link_stat_timer.expires = jiffies + HZ; > + mod_timer(&mdev->link_stat_timer, mdev->link_stat_timer.expires); > +} > + > +/** > + * link_stat_timer_handler - schedule work obtaining mac address and link status > + * @data: pointer to USB device instance > + * > + * The handler runs in interrupt context. That's why we need to defer the > + * tasks to a work queue. > + */ > +static void link_stat_timer_handler(struct timer_list *t) > +{ > + struct most_dev *mdev = from_timer(mdev, t, link_stat_timer); > + > + schedule_work(&mdev->poll_work_obj); > + mdev->link_stat_timer.expires = jiffies + (2 * HZ); > + add_timer(&mdev->link_stat_timer); > +} > + > +/** > + * wq_netinfo - work queue function to deliver latest networking information > + * @wq_obj: object that holds data for our deferred work to do > + * > + * This retrieves the network interface status of the USB INIC > + */ > +static void wq_netinfo(struct work_struct *wq_obj) > +{ > + struct most_dev *mdev = to_mdev_from_work(wq_obj); > + struct usb_device *usb_device = mdev->usb_device; > + struct device *dev = &usb_device->dev; > + u16 hi, mi, lo, link; > + u8 hw_addr[6]; > + > + if (drci_rd_reg(usb_device, DRCI_REG_HW_ADDR_HI, &hi) < 0) { > + dev_err(dev, "Vendor request 'hw_addr_hi' failed\n"); > + return; > + } > + > + if (drci_rd_reg(usb_device, DRCI_REG_HW_ADDR_MI, &mi) < 0) { > + dev_err(dev, "Vendor request 'hw_addr_mid' failed\n"); > + return; > + } > + > + if (drci_rd_reg(usb_device, DRCI_REG_HW_ADDR_LO, &lo) < 0) { > + dev_err(dev, "Vendor request 'hw_addr_low' failed\n"); > + return; > + } > + > + if (drci_rd_reg(usb_device, DRCI_REG_NI_STATE, &link) < 0) { > + dev_err(dev, "Vendor request 'link status' failed\n"); > + return; > + } > + > + hw_addr[0] = hi >> 8; > + hw_addr[1] = hi; > + hw_addr[2] = mi >> 8; > + hw_addr[3] = mi; > + hw_addr[4] = lo >> 8; > + hw_addr[5] = lo; > + > + if (mdev->on_netinfo) > + mdev->on_netinfo(&mdev->iface, link, hw_addr); > +} > + > +/** > + * wq_clear_halt - work queue function > + * @wq_obj: work_struct object to execute > + * > + * This sends a clear_halt to the given USB pipe. > + */ > +static void wq_clear_halt(struct work_struct *wq_obj) > +{ > + struct clear_hold_work *clear_work = to_clear_hold_work(wq_obj); > + struct most_dev *mdev = clear_work->mdev; > + unsigned int channel = clear_work->channel; > + int pipe = clear_work->pipe; > + > + mutex_lock(&mdev->io_mutex); > + most_stop_enqueue(&mdev->iface, channel); > + usb_kill_anchored_urbs(&mdev->busy_urbs[channel]); > + if (usb_clear_halt(mdev->usb_device, pipe)) > + dev_warn(&mdev->usb_device->dev, "Failed to reset endpoint.\n"); > + > + /* If the functional Stall condition has been set on an > + * asynchronous rx channel, we need to clear the tx channel > + * too, since the hardware runs its clean-up sequence on both > + * channels, as they are physically one on the network. > + * > + * The USB interface that exposes the asynchronous channels > + * contains always two endpoints, and two only. > + */ > + if (mdev->conf[channel].data_type == MOST_CH_ASYNC && > + mdev->conf[channel].direction == MOST_CH_RX) { > + int peer = 1 - channel; ^^^^^^^^^^^^^^^^^^^^^^ This is too tricky. At the start of the function channel seems to be a number between 0-30 so this looks like "peer" can be negative. Presumably, only the first two channels are MOST_CH_ASYNC and MOST_CH_RX? But could we just write it like: if (channel == 0) peer = 1; else peer = 0; > + int snd_pipe = usb_sndbulkpipe(mdev->usb_device, > + mdev->ep_address[peer]); > + usb_clear_halt(mdev->usb_device, snd_pipe); > + } > + mdev->is_channel_healthy[channel] = true; > + most_resume_enqueue(&mdev->iface, channel); > + mutex_unlock(&mdev->io_mutex); > +} > + > +/** > + * hdm_usb_fops - file operation table for USB driver > + */ > +static const struct file_operations hdm_usb_fops = { > + .owner = THIS_MODULE, > +}; > + > +/** > + * usb_device_id - ID table for HCD device probing > + */ > +static const struct usb_device_id usbid[] = { > + { USB_DEVICE(USB_VENDOR_ID_SMSC, USB_DEV_ID_BRDG), }, > + { USB_DEVICE(USB_VENDOR_ID_SMSC, USB_DEV_ID_OS81118), }, > + { USB_DEVICE(USB_VENDOR_ID_SMSC, USB_DEV_ID_OS81119), }, > + { USB_DEVICE(USB_VENDOR_ID_SMSC, USB_DEV_ID_OS81210), }, > + { } /* Terminating entry */ > +}; > + > +struct regs { > + const char *name; > + u16 reg; > +}; > + > +static const struct regs ro_regs[] = { > + { "ni_state", DRCI_REG_NI_STATE }, > + { "packet_bandwidth", DRCI_REG_PACKET_BW }, > + { "node_address", DRCI_REG_NODE_ADDR }, > + { "node_position", DRCI_REG_NODE_POS }, > +}; > + > +static const struct regs rw_regs[] = { > + { "mep_filter", DRCI_REG_MEP_FILTER }, > + { "mep_hash0", DRCI_REG_HASH_TBL0 }, > + { "mep_hash1", DRCI_REG_HASH_TBL1 }, > + { "mep_hash2", DRCI_REG_HASH_TBL2 }, > + { "mep_hash3", DRCI_REG_HASH_TBL3 }, > + { "mep_eui48_hi", DRCI_REG_HW_ADDR_HI }, > + { "mep_eui48_mi", DRCI_REG_HW_ADDR_MI }, > + { "mep_eui48_lo", DRCI_REG_HW_ADDR_LO }, > +}; > + > +static int get_stat_reg_addr(const struct regs *regs, int size, > + const char *name, u16 *reg_addr) > +{ > + int i; > + > + for (i = 0; i < size; i++) { > + if (!strcmp(name, regs[i].name)) { > + *reg_addr = regs[i].reg; > + return 0; > + } > + } > + return -EFAULT; This should be -EINVAL > +} > + > +#define get_static_reg_addr(regs, name, reg_addr) \ > + get_stat_reg_addr(regs, ARRAY_SIZE(regs), name, reg_addr) > + > +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")) I feel like most of these strcmp() should be sysfs_streq(). > + 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, ®_addr) && > + get_static_reg_addr(rw_regs, name, ®_addr)) > + return -EFAULT; -EINVAL > + > + 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); Could we move function calls which can fail out of the declaration block? Otherwise there is a blank or worse between the call and the error check. > + > + if (err) > + return err; > + > + if (!strcmp(name, "arb_address")) { Use sysfs_streq()? > + 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, ®_addr)) > + err = drci_wr_reg(usb_dev, reg_addr, val); > + else > + return -EFAULT; A lot of -EFAULT should be changed to -EINVAL. Search for it everywhere, please. > + > + 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); > + > +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, > +}; > + > +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; The "goto" checks for if err isn't set an defaults to -ENOMEM. Please just set the error code. But actually here the appropriate thing is to just return directly: mdev = kzalloc(sizeof(*mdev), GFP_KERNEL); if (!mdev) return -ENOMEM; > + > + usb_set_intfdata(interface, mdev); > + num_endpoints = usb_iface_desc->desc.bNumEndpoints; > + 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; > + 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); > + > + 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); > + > + ret = most_register_interface(&mdev->iface); > + if (ret) > + goto err_free_busy_urbs; > + > + mutex_lock(&mdev->io_mutex); > + if (le16_to_cpu(usb_dev->descriptor.idProduct) == USB_DEV_ID_OS81118 || > + le16_to_cpu(usb_dev->descriptor.idProduct) == USB_DEV_ID_OS81119 || > + le16_to_cpu(usb_dev->descriptor.idProduct) == USB_DEV_ID_OS81210) { > + mdev->dci = kzalloc(sizeof(*mdev->dci), GFP_KERNEL); > + if (!mdev->dci) { > + mutex_unlock(&mdev->io_mutex); > + most_deregister_interface(&mdev->iface); Free iface after the goto. > + ret = -ENOMEM; > + goto err_free_busy_urbs; > + } > + > + mdev->dci->dev.init_name = "dci"; > + mdev->dci->dev.parent = get_device(mdev->iface.dev); ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Does this get_device() need a matching put_device() somewhere? I'm not totally sure how the ->parent stuff works... > + mdev->dci->dev.groups = dci_attr_groups; > + mdev->dci->dev.release = release_dci; > + if (device_register(&mdev->dci->dev)) { > + mutex_unlock(&mdev->io_mutex); > + most_deregister_interface(&mdev->iface); > + ret = -ENOMEM; > + goto err_free_dci; > + } > + mdev->dci->usb_device = mdev->usb_device; > + } > + mutex_unlock(&mdev->io_mutex); > + return 0; > +err_free_dci: > + put_device(&mdev->dci->dev); > +err_free_busy_urbs: > + kfree(mdev->busy_urbs); > +err_free_ep_address: > + kfree(mdev->ep_address); > +err_free_cap: > + kfree(mdev->cap); > +err_free_conf: > + kfree(mdev->conf); > +err_free_mdev: > + put_device(&mdev->dev); > +err_out_of_memory: > + if (ret == 0 || ret == -ENOMEM) { > + ret = -ENOMEM; > + dev_err(dev, "out of memory\n"); > + } > + return ret; > +} regards, dan carpenter