I took the time today to fix up the usbtmc driver that was in the
-staging tree and here's the results. I only have 3 todo items left:
- get assigned minor number
- reserve ioctl range
- add autosuspend support
The minor number is easy, I can give that :)
The ioctl range is also simple, I really can't see getting rid of the
remaining ioctls as they are useful and unless we want to just us sysfs
files for these items, I'll leave them as is. I moved the majority of
the ioctls in the old driver to be sysfs files, as they are simple
values.
The autosuspend support isn't that hard either, but I would like to find
someone with one of these devices to help me in testing that before I
add it, so I imagine the driver can go into the tree without that being
added at this time.
Many thanks to Felipe Balbi <[email protected]> for lots of work in
cleaning up this driver.
I've now added this to my tree, and any review comments are greatly
welcome.
Cc: Stefan Kopp <[email protected]>
Cc: Marcel Janssen <[email protected]>
Cc: Felipe Balbi <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
drivers/usb/class/Kconfig | 10
drivers/usb/class/Makefile | 1
drivers/usb/class/usbtmc.c | 1058 +++++++++++++++++++++++++++++++++++++++++++++
drivers/usb/class/usbtmc.h | 24 +
include/linux/usb/tmc.h | 27 +
5 files changed, 1120 insertions(+)
--- a/drivers/usb/class/Kconfig
+++ b/drivers/usb/class/Kconfig
@@ -40,3 +40,13 @@ config USB_WDM
To compile this driver as a module, choose M here: the
module will be called cdc-wdm.
+config USB_TMC
+ tristate "USB Test and Measurement Class support"
+ depends on USB
+ help
+ Say Y here if you want to connect a USB device that follows
+ the USB.org specification for USB Test and Measurement devices
+ to your computer's USB port.
+
+ To compile this driver as a module, choose M here: the
+ module will be called usbtmc.
--- a/drivers/usb/class/Makefile
+++ b/drivers/usb/class/Makefile
@@ -6,3 +6,4 @@
obj-$(CONFIG_USB_ACM) += cdc-acm.o
obj-$(CONFIG_USB_PRINTER) += usblp.o
obj-$(CONFIG_USB_WDM) += cdc-wdm.o
+obj-$(CONFIG_USB_TMC) += usbtmc.o
--- /dev/null
+++ b/drivers/usb/class/usbtmc.c
@@ -0,0 +1,1058 @@
+/**
+ * drivers/usb/class/usbtmc.c - USB Test & Measurment class driver
+ *
+ * Copyright (C) 2007 Stefan Kopp, Gechingen, Germany
+ * Copyright (C) 2008 Novell, Inc.
+ * Copyright (C) 2008 Greg Kroah-Hartman <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * 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.
+ *
+ * The GNU General Public License is available at
+ * http://www.gnu.org/copyleft/gpl.html.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/uaccess.h>
+#include <linux/kref.h>
+#include <linux/mutex.h>
+#include <linux/usb.h>
+#include <linux/usb/tmc.h>
+#include "usbtmc.h"
+
+
+/* FIXME get a real minor base */
+#define USBTMC_MINOR_BASE 192
+
+/*
+ * Size of driver internal IO buffer. Must be multiple of 4 and at least as
+ * large as wMaxPacketSize (which is usually 512 bytes).
+ */
+#define USBTMC_SIZE_IOBUFFER 4096
+
+/* Default USB timeout (in milliseconds) */
+#define USBTMC_TIMEOUT 10
+
+/*
+ * Maximum number of read cycles to empty bulk in endpoint during CLEAR and
+ * ABORT_BULK_IN requests. Ends the loop if (for whatever reason) a short
+ * packet is never read.
+ */
+#define USBTMC_MAX_READS_TO_CLEAR_BULK_IN 100
+
+static struct usb_device_id usbtmc_devices[] = {
+ { USB_INTERFACE_INFO(USB_CLASS_APP_SPEC, 3, 0), },
+ { 0, } /* terminating entry */
+};
+
+/*
+ * This structure is the capabilities for the device
+ * See section 4.2.1.8 of the USBTMC specification for details.
+ */
+struct usbtmc_dev_capabilities {
+ __u8 interface_capabilities;
+ __u8 device_capabilities;
+ __u8 usb488_interface_capabilities;
+ __u8 usb488_device_capabilities;
+};
+
+/* This structure holds private data for each USBTMC device. One copy is
+ * allocated for each USBTMC device in the driver's probe function.
+ */
+struct usbtmc_device_data {
+ const struct usb_device_id *id;
+ struct usb_device *usb_dev;
+ struct usb_interface *intf;
+
+ unsigned int bulk_in;
+ unsigned int bulk_out;
+
+ u8 bTag;
+ u8 bTag_last_write; /* needed for abort */
+ u8 bTag_last_read; /* needed for abort */
+
+ /* attributes from the USB TMC spec for this device */
+ u8 TermChar;
+ bool TermCharEnabled;
+ bool auto_abort;
+
+ u8 *buffer; /* i/o buffer */
+
+ struct usbtmc_dev_capabilities capabilities;
+ struct kref kref;
+};
+#define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
+
+/* Forward declarations */
+static struct usb_driver usbtmc_driver;
+
+static void usbtmc_delete(struct kref *kref)
+{
+ struct usbtmc_device_data *data = to_usbtmc_data(kref);
+
+ usb_put_dev(data->usb_dev);
+ kfree(data->buffer);
+ kfree(data);
+}
+
+static int usbtmc_open(struct inode *inode, struct file *filp)
+{
+ struct usb_endpoint_descriptor *endpoint;
+ struct usb_interface *intf;
+ struct usb_host_interface *iface_desc;
+ struct usbtmc_device_data *data;
+ struct device *dev;
+ u8 n;
+
+ intf = usb_find_interface(&usbtmc_driver, iminor(inode));
+ if (!intf) {
+ printk(KERN_ERR KBUILD_MODNAME
+ ": can not find device for minor %d", iminor(inode));
+ return -ENODEV;
+ }
+
+ data = usb_get_intfdata(intf);
+ kref_get(&data->kref);
+
+ /* Store pointer in file structure's private data field */
+ filp->private_data = data;
+
+ dev = &data->intf->dev;
+
+ /* USBTMC devices have only one setting, so use that */
+ iface_desc = data->intf->cur_altsetting;
+
+ /* Find bulk in endpoint */
+ for (n = 0; n < iface_desc->desc.bNumEndpoints; n++) {
+ endpoint = &iface_desc->endpoint[n].desc;
+
+ if (usb_endpoint_is_bulk_in(endpoint)) {
+ data->bulk_in = endpoint->bEndpointAddress;
+ dev_dbg(dev, "Found bulk in endpoint at %u\n",
+ data->bulk_in);
+ break;
+ }
+ }
+
+ /* Find bulk out endpoint */
+ for (n = 0; n < iface_desc->desc.bNumEndpoints; n++) {
+ endpoint = &iface_desc->endpoint[n].desc;
+
+ if (usb_endpoint_is_bulk_out(endpoint)) {
+ data->bulk_out = endpoint->bEndpointAddress;
+ dev_dbg(dev, "Found Bulk out endpoint at %u\n",
+ data->bulk_out);
+ break;
+ }
+ }
+
+ return 0;
+}
+
+static int usbtmc_release(struct inode *inode, struct file *file)
+{
+ struct usbtmc_device_data *data = file->private_data;
+
+ kref_put(&data->kref, usbtmc_delete);
+ return 0;
+}
+
+static int usbtmc_ioctl_abort_bulk_in(struct inode *inode, struct file *filp,
+ unsigned int cmd, unsigned long arg)
+{
+ struct usbtmc_device_data *data;
+ char *buffer;
+ struct device *dev;
+ int rv;
+ int n;
+ int actual;
+ struct usb_host_interface *current_setting;
+ int max_size;
+
+ data = filp->private_data;
+ dev = &data->intf->dev;
+ buffer = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ USBTMC_REQUEST_INITIATE_ABORT_BULK_IN,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_ENDPOINT,
+ data->bTag_last_read, data->bulk_in,
+ buffer, 2, USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_err(dev, "usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+
+ dev_dbg(dev, "INITIATE_ABORT_BULK_IN returned %x\n", data->buffer[0]);
+
+ if (buffer[0] == USBTMC_STATUS_FAILED) {
+ rv = 0;
+ goto exit;
+ }
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "INITIATE_ABORT_BULK_IN returned %x\n",
+ buffer[0]);
+ rv = -EPERM;
+ goto exit;
+ }
+
+ max_size = 0;
+ current_setting = data->intf->cur_altsetting;
+ for (n = 0; n < current_setting->desc.bNumEndpoints; n++)
+ if (current_setting->endpoint[n].desc.bEndpointAddress ==
+ data->bulk_in)
+ max_size = le16_to_cpu(current_setting->endpoint[n].
+ desc.wMaxPacketSize);
+
+ if (max_size == 0) {
+ dev_err(dev, "Couldn't get wMaxPacketSize\n");
+ rv = -EPERM;
+ goto exit;
+ }
+
+ dev_dbg(&data->intf->dev, "wMaxPacketSize is %d\n", max_size);
+
+ n = 0;
+
+ do {
+ dev_dbg(dev, "Reading from bulk in EP\n");
+
+ rv = usb_bulk_msg(data->usb_dev,
+ usb_rcvbulkpipe(data->usb_dev,
+ data->bulk_in),
+ buffer, USBTMC_SIZE_IOBUFFER,
+ &actual, USBTMC_TIMEOUT);
+
+ n++;
+
+ if (rv < 0) {
+ dev_err(dev, "usb_bulk_msg returned %d\n", rv);
+ goto exit;
+ }
+ } while ((actual == max_size) &&
+ (n < USBTMC_MAX_READS_TO_CLEAR_BULK_IN));
+
+ if (actual == max_size) {
+ dev_err(dev, "Couldn't clear device buffer within %d cycles\n",
+ USBTMC_MAX_READS_TO_CLEAR_BULK_IN);
+ rv = -EPERM;
+ goto exit;
+ }
+
+ n = 0;
+
+usbtmc_abort_bulk_in_status:
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ USBTMC_REQUEST_CHECK_ABORT_BULK_IN_STATUS,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_ENDPOINT,
+ 0, data->bulk_in, buffer, 0x08,
+ USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_err(dev, "usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+
+ dev_dbg(dev, "INITIATE_ABORT_BULK_IN returned %x\n", buffer[0]);
+
+ if (buffer[0] == USBTMC_STATUS_SUCCESS) {
+ rv = 0;
+ goto exit;
+ }
+
+ if (buffer[0] != USBTMC_STATUS_PENDING) {
+ dev_err(dev, "INITIATE_ABORT_BULK_IN returned %x\n", buffer[0]);
+ rv = -EPERM;
+ goto exit;
+ }
+
+ if (buffer[1] == 1)
+ do {
+ dev_dbg(dev, "Reading from bulk in EP\n");
+
+ rv = usb_bulk_msg(data->usb_dev,
+ usb_rcvbulkpipe(data->usb_dev,
+ data->bulk_in),
+ buffer, USBTMC_SIZE_IOBUFFER,
+ &actual, USBTMC_TIMEOUT);
+
+ n++;
+
+ if (rv < 0) {
+ dev_err(dev, "usb_bulk_msg returned %d\n", rv);
+ goto exit;
+ }
+ } while ((actual = max_size) &&
+ (n < USBTMC_MAX_READS_TO_CLEAR_BULK_IN));
+
+ if (actual == max_size) {
+ dev_err(dev, "Couldn't clear device buffer within %d cycles\n",
+ USBTMC_MAX_READS_TO_CLEAR_BULK_IN);
+ rv = -EPERM;
+ goto exit;
+ }
+
+ goto usbtmc_abort_bulk_in_status;
+
+exit:
+ kfree(buffer);
+ return rv;
+
+}
+
+static int usbtmc_ioctl_abort_bulk_out(struct inode *inode, struct file *filp,
+ unsigned int cmd, unsigned long arg)
+{
+ struct usbtmc_device_data *data;
+ struct device *dev;
+ u8 *buffer;
+ int rv;
+ int n;
+
+ data = filp->private_data;
+ dev = &data->intf->dev;
+
+ buffer = kmalloc(8, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ USBTMC_REQUEST_INITIATE_ABORT_BULK_OUT,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_ENDPOINT,
+ data->bTag_last_write, data->bulk_out,
+ buffer, 2, USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_err(dev, "usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+
+ dev_dbg(dev, "INITIATE_ABORT_BULK_OUT returned %x\n", buffer[0]);
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "INITIATE_ABORT_BULK_OUT returned %x\n",
+ buffer[0]);
+ rv = -EPERM;
+ goto exit;
+ }
+
+ n = 0;
+
+usbtmc_abort_bulk_out_check_status:
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ USBTMC_REQUEST_CHECK_ABORT_BULK_OUT_STATUS,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_ENDPOINT,
+ 0, data->bulk_out, buffer, 0x08,
+ USBTMC_TIMEOUT);
+ n++;
+ if (rv < 0) {
+ dev_err(dev, "usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+
+ dev_dbg(dev, "CHECK_ABORT_BULK_OUT returned %x\n", buffer[0]);
+
+ if (buffer[0] == USBTMC_STATUS_SUCCESS)
+ goto usbtmc_abort_bulk_out_clear_halt;
+
+ if ((buffer[0] == USBTMC_STATUS_PENDING) &&
+ (n < USBTMC_MAX_READS_TO_CLEAR_BULK_IN))
+ goto usbtmc_abort_bulk_out_check_status;
+
+ rv = -EPERM;
+ goto exit;
+
+usbtmc_abort_bulk_out_clear_halt:
+ rv = usb_control_msg(data->usb_dev,
+ usb_sndctrlpipe(data->usb_dev, 0),
+ USB_REQ_CLEAR_FEATURE,
+ USB_DIR_OUT | USB_TYPE_STANDARD |
+ USB_RECIP_ENDPOINT,
+ USB_ENDPOINT_HALT, data->bulk_out, buffer,
+ 0, USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_err(dev, "usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+ rv = 0;
+
+exit:
+ kfree(buffer);
+ return rv;
+}
+
+static ssize_t usbtmc_read(struct file *filp, char __user *buf,
+ size_t count, loff_t *f_pos)
+{
+ struct usbtmc_device_data *data;
+ struct device *dev;
+ unsigned long int n_characters;
+ u8 *buffer;
+ int actual;
+ int done;
+ int remaining;
+ int retval;
+ int this_part;
+
+ /* Get pointer to private data structure */
+ data = filp->private_data;
+ dev = &data->intf->dev;
+ buffer = data->buffer;
+
+ remaining = count;
+ done = 0;
+
+ while (remaining > 0) {
+ if (remaining > USBTMC_SIZE_IOBUFFER - 12 - 3)
+ this_part = USBTMC_SIZE_IOBUFFER - 12 - 3;
+ else
+ this_part = remaining;
+
+ /* Setup IO buffer for DEV_DEP_MSG_IN message
+ * Refer to class specs for details
+ */
+ buffer[0] = 2;
+ buffer[1] = data->bTag;
+ buffer[2] = ~(data->bTag);
+ buffer[3] = 0; /* Reserved */
+ buffer[4] = (this_part - 12 - 3) & 255;
+ buffer[5] = ((this_part - 12 - 3) >> 8) & 255;
+ buffer[6] = ((this_part - 12 - 3) >> 16) & 255;
+ buffer[7] = ((this_part - 12 - 3) >> 24) & 255;
+ buffer[8] = data->TermCharEnabled * 2;
+ /* Use term character? */
+ buffer[9] = data->TermChar;
+ buffer[10] = 0; /* Reserved */
+ buffer[11] = 0; /* Reserved */
+
+ /* Send bulk URB */
+ retval = usb_bulk_msg(data->usb_dev,
+ usb_sndbulkpipe(data->usb_dev,
+ data->bulk_out),
+ buffer, 12, &actual, USBTMC_TIMEOUT);
+
+ /* Store bTag (in case we need to abort) */
+ data->bTag_last_write = data->bTag;
+
+ /* Increment bTag -- and increment again if zero */
+ data->bTag++;
+ if (!data->bTag)
+ (data->bTag)++;
+
+ if (retval < 0) {
+ dev_err(dev, "usb_bulk_msg returned %d\n", retval);
+ if (data->auto_abort)
+ usbtmc_ioctl_abort_bulk_out(NULL, filp,
+ USBTMC_IOCTL_ABORT_BULK_OUT, 0);
+ return retval;
+ }
+
+ /* Send bulk URB */
+ retval = usb_bulk_msg(data->usb_dev,
+ usb_rcvbulkpipe(data->usb_dev,
+ data->bulk_in),
+ buffer, USBTMC_SIZE_IOBUFFER, &actual,
+ USBTMC_TIMEOUT);
+
+ /* Store bTag (in case we need to abort) */
+ data->bTag_last_read = data->bTag;
+
+ if (retval < 0) {
+ dev_err(dev, "Unable to read data, error %d\n", retval);
+ if (data->auto_abort)
+ usbtmc_ioctl_abort_bulk_in(NULL, filp,
+ USBTMC_IOCTL_ABORT_BULK_IN, 0);
+ return retval;
+ }
+
+ /* How many characters did the instrument send? */
+ n_characters = buffer[4] +
+ (buffer[5] << 8) +
+ (buffer[6] << 16) +
+ (buffer[7] << 24);
+
+ /* Copy buffer to user space */
+ if (copy_to_user(buf + done, &buffer[12], n_characters))
+ /* There must have been an addressing problem */
+ return -EFAULT;
+
+ done += n_characters;
+ if (n_characters < USBTMC_SIZE_IOBUFFER)
+ remaining = 0;
+ }
+
+ /* Update file position value */
+ *f_pos = *f_pos + done;
+
+ return done;
+}
+
+static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
+ size_t count, loff_t *f_pos)
+{
+ struct usbtmc_device_data *data;
+ u8 *buffer;
+ int retval;
+ int actual;
+ unsigned long int n_bytes;
+ int n;
+ int remaining;
+ int done;
+ int this_part;
+
+ data = filp->private_data;
+ buffer = data->buffer;
+
+ remaining = count;
+ done = 0;
+
+ while (remaining > 0) {
+ if (remaining > USBTMC_SIZE_IOBUFFER - 12) {
+ this_part = USBTMC_SIZE_IOBUFFER - 12;
+ buffer[8] = 0;
+ } else {
+ this_part = remaining;
+ buffer[8] = 1;
+ }
+
+ /* Setup IO buffer for DEV_DEP_MSG_OUT message */
+ buffer[0] = 1;
+ buffer[1] = data->bTag;
+ buffer[2] = ~(data->bTag);
+ buffer[3] = 0; /* Reserved */
+ buffer[4] = this_part & 255;
+ buffer[5] = (this_part >> 8) & 255;
+ buffer[6] = (this_part >> 16) & 255;
+ buffer[7] = (this_part >> 24) & 255;
+ /* buffer[8] is set above... */
+ buffer[9] = 0; /* Reserved */
+ buffer[10] = 0; /* Reserved */
+ buffer[11] = 0; /* Reserved */
+
+ if (copy_from_user(&buffer[12], buf + done, this_part))
+ return -EFAULT;
+
+ n_bytes = 12 + this_part;
+ if (this_part % 4)
+ n_bytes += 4 - this_part % 4;
+ for (n = 12 + this_part; n < n_bytes; n++)
+ buffer[n] = 0;
+
+ retval = usb_bulk_msg(data->usb_dev,
+ usb_sndbulkpipe(data->usb_dev,
+ data->bulk_out),
+ buffer, n_bytes, &actual, USBTMC_TIMEOUT);
+
+ data->bTag_last_write = data->bTag;
+ data->bTag++;
+
+ if (!data->bTag)
+ data->bTag++;
+
+ if (retval < 0) {
+ dev_err(&data->intf->dev,
+ "Unable to send data, error %d\n", retval);
+ if (data->auto_abort)
+ usbtmc_ioctl_abort_bulk_out(NULL, filp,
+ USBTMC_IOCTL_ABORT_BULK_OUT,
+ 0);
+ return retval;
+ }
+
+ remaining -= this_part;
+ done += this_part;
+ }
+
+ return count;
+}
+
+static int usbtmc_ioctl_clear(struct inode *inode, struct file *filp,
+ unsigned int cmd, unsigned long arg)
+{
+ struct usbtmc_device_data *data;
+ struct usb_host_interface *current_setting;
+ struct usb_endpoint_descriptor *desc;
+ struct device *dev;
+ u8 *buffer;
+ int rv;
+ int n;
+ int actual;
+ int max_size;
+
+ data = filp->private_data;
+ dev = &data->intf->dev;
+
+ dev_dbg(dev, "Sending INITIATE_CLEAR request\n");
+
+ buffer = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ USBTMC_REQUEST_INITIATE_CLEAR,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ 0, 0, buffer, 1, USBTMC_TIMEOUT);
+ if (rv < 0) {
+ dev_err(dev, "usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+
+ dev_dbg(dev, "INITIATE_CLEAR returned %x\n", buffer[0]);
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "INITIATE_CLEAR returned %x\n", buffer[0]);
+ rv = -EPERM;
+ goto exit;
+ }
+
+ max_size = 0;
+ current_setting = data->intf->cur_altsetting;
+ for (n = 0; n < current_setting->desc.bNumEndpoints; n++) {
+ desc = ¤t_setting->endpoint[n].desc;
+ if (desc->bEndpointAddress == data->bulk_in)
+ max_size = le16_to_cpu(desc->wMaxPacketSize);
+ }
+
+ if (max_size == 0) {
+ dev_err(dev, "Couldn't get wMaxPacketSize\n");
+ rv = -EPERM;
+ goto exit;
+ }
+
+ dev_dbg(dev, "wMaxPacketSize is %d\n", max_size);
+
+ n = 0;
+
+usbtmc_clear_check_status:
+
+ dev_dbg(dev, "Sending CHECK_CLEAR_STATUS request\n");
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ USBTMC_REQUEST_CHECK_CLEAR_STATUS,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ 0, 0, buffer, 2, USBTMC_TIMEOUT);
+ if (rv < 0) {
+ dev_err(dev, "usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+
+ dev_dbg(dev, "CHECK_CLEAR_STATUS returned %x\n", buffer[0]);
+
+ if (buffer[0] == USBTMC_STATUS_SUCCESS)
+ goto usbtmc_clear_bulk_out_halt;
+
+ if (buffer[0] != USBTMC_STATUS_PENDING) {
+ dev_err(dev, "CHECK_CLEAR_STATUS returned %x\n", buffer[0]);
+ rv = -EPERM;
+ goto exit;
+ }
+
+ if (buffer[1] == 1)
+ do {
+ dev_dbg(dev, "Reading from bulk in EP\n");
+
+ rv = usb_bulk_msg(data->usb_dev,
+ usb_rcvbulkpipe(data->usb_dev,
+ data->bulk_in),
+ buffer, USBTMC_SIZE_IOBUFFER,
+ &actual, USBTMC_TIMEOUT);
+ n++;
+
+ if (rv < 0) {
+ dev_err(dev, "usb_control_msg returned %d\n",
+ rv);
+ goto exit;
+ }
+ } while ((actual == max_size) &&
+ (n < USBTMC_MAX_READS_TO_CLEAR_BULK_IN));
+
+ if (actual == max_size) {
+ dev_err(dev, "Couldn't clear device buffer within %d cycles\n",
+ USBTMC_MAX_READS_TO_CLEAR_BULK_IN);
+ rv = -EPERM;
+ goto exit;
+ }
+
+ goto usbtmc_clear_check_status;
+
+usbtmc_clear_bulk_out_halt:
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_sndctrlpipe(data->usb_dev, 0),
+ USB_REQ_CLEAR_FEATURE,
+ USB_DIR_OUT | USB_TYPE_STANDARD |
+ USB_RECIP_ENDPOINT,
+ USB_ENDPOINT_HALT,
+ data->bulk_out, buffer, 0,
+ USBTMC_TIMEOUT);
+ if (rv < 0) {
+ dev_err(dev, "usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+ rv = 0;
+
+exit:
+ kfree(buffer);
+ return rv;
+}
+
+static int usbtmc_ioctl_clear_out_halt(struct inode *inode, struct file *file,
+ unsigned int cmd, unsigned long arg)
+{
+ struct usbtmc_device_data *data = file->private_data;
+ u8 *buffer;
+ int rv;
+
+ buffer = kmalloc(2, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_sndctrlpipe(data->usb_dev, 0),
+ USB_REQ_CLEAR_FEATURE,
+ USB_DIR_OUT | USB_TYPE_STANDARD |
+ USB_RECIP_ENDPOINT,
+ USB_ENDPOINT_HALT, data->bulk_out,
+ buffer, 0, USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_err(&data->usb_dev->dev, "usb_control_msg returned %d\n",
+ rv);
+ goto exit;
+ }
+ rv = 0;
+
+exit:
+ kfree(buffer);
+ return rv;
+}
+
+static int usbtmc_ioctl_clear_in_halt(struct inode *inode, struct file *file,
+ unsigned int cmd, unsigned long arg)
+{
+ struct usbtmc_device_data *data = file->private_data;
+ u8 *buffer;
+ int rv;
+
+ buffer = kmalloc(2, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ rv = usb_control_msg(data->usb_dev, usb_sndctrlpipe(data->usb_dev, 0),
+ USB_REQ_CLEAR_FEATURE,
+ USB_DIR_OUT | USB_TYPE_STANDARD |
+ USB_RECIP_ENDPOINT,
+ USB_ENDPOINT_HALT, data->bulk_in, buffer, 0,
+ USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_err(&data->usb_dev->dev, "usb_control_msg returned %d\n",
+ rv);
+ goto exit;
+ }
+ rv = 0;
+
+exit:
+ kfree(buffer);
+ return rv;
+}
+
+static int get_capabilities(struct usbtmc_device_data *data)
+{
+ struct device *dev = &data->usb_dev->dev;
+ char *buffer;
+ int rv;
+
+ buffer = kmalloc(0x18, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ rv = usb_control_msg(data->usb_dev, usb_rcvctrlpipe(data->usb_dev, 0),
+ USBTMC_REQUEST_GET_CAPABILITIES,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ 0, 0, buffer, 0x18, USBTMC_TIMEOUT);
+ if (rv < 0) {
+ dev_err(dev, "usb_control_msg returned %d\n", rv);
+ return rv;
+ }
+
+ dev_dbg(dev, "GET_CAPABILITIES returned %x\n", buffer[0]);
+ dev_dbg(dev, "Interface capabilities are %x\n", buffer[4]);
+ dev_dbg(dev, "Device capabilities are %x\n", buffer[5]);
+ dev_dbg(dev, "USB488 interface capabilities are %x\n", buffer[14]);
+ dev_dbg(dev, "USB488 device capabilities are %x\n", buffer[15]);
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "GET_CAPABILITIES returned %x\n", buffer[0]);
+ return -EPERM;
+ }
+
+ data->capabilities.interface_capabilities = buffer[4];
+ data->capabilities.device_capabilities = buffer[5];
+ data->capabilities.usb488_interface_capabilities = buffer[14];
+ data->capabilities.usb488_device_capabilities = buffer[15];
+ return 0;
+}
+
+#define capability_attribute(name) \
+static ssize_t show_##name(struct device *dev, \
+ struct device_attribute *attr, char *buf) \
+{ \
+ struct usb_interface *intf = to_usb_interface(dev); \
+ struct usbtmc_device_data *data = usb_get_intfdata(intf); \
+ \
+ return sprintf(buf, "%d\n", data->capabilities.name); \
+} \
+static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)
+
+capability_attribute(interface_capabilities);
+capability_attribute(device_capabilities);
+capability_attribute(usb488_interface_capabilities);
+capability_attribute(usb488_device_capabilities);
+
+static struct attribute *capability_attrs[] = {
+ &dev_attr_interface_capabilities.attr,
+ &dev_attr_device_capabilities.attr,
+ &dev_attr_usb488_interface_capabilities.attr,
+ &dev_attr_usb488_device_capabilities.attr,
+ NULL,
+};
+
+static struct attribute_group capability_attr_grp = {
+ .attrs = capability_attrs,
+};
+
+#define data_attribute(name) \
+static ssize_t show_##name(struct device *dev, \
+ struct device_attribute *attr, char *buf) \
+{ \
+ struct usb_interface *intf = to_usb_interface(dev); \
+ struct usbtmc_device_data *data = usb_get_intfdata(intf); \
+ \
+ return sprintf(buf, "%d\n", data->name); \
+} \
+static ssize_t store_##name(struct device *dev, \
+ struct device_attribute *attr, \
+ const char *buf, size_t count) \
+{ \
+ struct usb_interface *intf = to_usb_interface(dev); \
+ struct usbtmc_device_data *data = usb_get_intfdata(intf); \
+ ssize_t result; \
+ unsigned val; \
+ \
+ result = sscanf(buf, "%u\n", &val); \
+ if (result != 1) \
+ result = -EINVAL; \
+ data->name = val; \
+ if (result < 0) \
+ return result; \
+ else \
+ return count; \
+} \
+static DEVICE_ATTR(name, S_IRUGO, show_##name, store_##name)
+
+data_attribute(TermChar);
+data_attribute(TermCharEnabled);
+data_attribute(auto_abort);
+
+static struct attribute *data_attrs[] = {
+ &dev_attr_TermChar.attr,
+ &dev_attr_TermCharEnabled.attr,
+ &dev_attr_auto_abort.attr,
+ NULL,
+};
+
+static struct attribute_group data_attr_grp = {
+ .attrs = data_attrs,
+};
+
+
+
+static int usbtmc_ioctl_indicator_pulse(struct inode *inode, struct file *file,
+ unsigned int cmd, unsigned long arg)
+{
+ struct usbtmc_device_data *data;
+ struct device *dev;
+ u8 *buffer;
+ int rv;
+
+ data = file->private_data;
+ dev = &data->intf->dev;
+
+ buffer = kmalloc(2, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ USBTMC_REQUEST_INDICATOR_PULSE,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ 0, 0, buffer, 0x01, USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_err(dev, "usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+
+ dev_dbg(dev, "INDICATOR_PULSE returned %x\n", buffer[0]);
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "INDICATOR_PULSE returned %x\n", buffer[0]);
+ rv = -EPERM;
+ goto exit;
+ }
+ rv = 0;
+
+exit:
+ kfree(buffer);
+ return rv;
+}
+
+static int usbtmc_ioctl(struct inode *inode, struct file *filp,
+ unsigned int cmd, unsigned long arg)
+{
+ switch (cmd) {
+ case USBTMC_IOCTL_CLEAR_OUT_HALT:
+ return usbtmc_ioctl_clear_out_halt(inode, filp, cmd, arg);
+
+ case USBTMC_IOCTL_CLEAR_IN_HALT:
+ return usbtmc_ioctl_clear_in_halt(inode, filp, cmd, arg);
+
+ case USBTMC_IOCTL_INDICATOR_PULSE:
+ return usbtmc_ioctl_indicator_pulse(inode, filp, cmd, arg);
+
+ case USBTMC_IOCTL_CLEAR:
+ return usbtmc_ioctl_clear(inode, filp, cmd, arg);
+
+ case USBTMC_IOCTL_ABORT_BULK_OUT:
+ return usbtmc_ioctl_abort_bulk_out(inode, filp, cmd, arg);
+
+ case USBTMC_IOCTL_ABORT_BULK_IN:
+ return usbtmc_ioctl_abort_bulk_in(inode, filp, cmd, arg);
+
+ default:
+ return -EBADRQC;
+ }
+}
+
+static struct file_operations fops = {
+ .owner = THIS_MODULE,
+ .read = usbtmc_read,
+ .write = usbtmc_write,
+ .open = usbtmc_open,
+ .release = usbtmc_release,
+ .ioctl = usbtmc_ioctl,
+};
+
+static struct usb_class_driver usbtmc_class = {
+ .name = "usbtmc%d",
+ .fops = &fops,
+ .minor_base = USBTMC_MINOR_BASE,
+};
+
+
+static int usbtmc_probe(struct usb_interface *intf,
+ const struct usb_device_id *id)
+{
+ int retcode;
+ struct usbtmc_device_data *data;
+
+ dev_dbg(&intf->dev, "usbtmc_probe called\n");
+
+ data = kmalloc(sizeof(struct usbtmc_device_data), GFP_KERNEL);
+ if (!data) {
+ dev_err(&intf->dev, "Unable to allocate kernel memory\n");
+ return -ENOMEM;
+ }
+ data->buffer = kzalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL);
+ if (!data->buffer) {
+ dev_err(&intf->dev, "Unable to allocate kernel memory\n");
+ kfree(data);
+ return -ENOMEM;
+ }
+
+ data->intf = intf;
+ data->id = id;
+ data->usb_dev = usb_get_dev(interface_to_usbdev(intf));
+ usb_set_intfdata(intf, data);
+ kref_init(&data->kref);
+
+ /* Initialize USBTMC bTag and other fields */
+ data->bTag = 1;
+ data->TermCharEnabled = 0;
+ data->TermChar = '\n';
+
+ retcode = get_capabilities(data);
+ if (retcode)
+ dev_err(&intf->dev, "can't read capabilities\n");
+ else
+ retcode = sysfs_create_group(&intf->dev.kobj,
+ &capability_attr_grp);
+
+ retcode = sysfs_create_group(&intf->dev.kobj, &data_attr_grp);
+
+ retcode = usb_register_dev(intf, &usbtmc_class);
+ if (retcode) {
+ dev_err(&intf->dev, "Not able to get a minor"
+ " (base %u, slice default): %d\n", USBTMC_MINOR_BASE,
+ retcode);
+ }
+ dev_dbg(&intf->dev, "Using minor number %d\n", intf->minor);
+
+ return 0;
+}
+
+static void usbtmc_disconnect(struct usb_interface *intf)
+{
+ struct usbtmc_device_data *data;
+
+ dev_dbg(&intf->dev, "usbtmc_disconnect called\n");
+
+ data = usb_get_intfdata(intf);
+ kref_put(&data->kref, usbtmc_delete);
+}
+
+static struct usb_driver usbtmc_driver = {
+ .name = "usbtmc",
+ .id_table = usbtmc_devices,
+ .probe = usbtmc_probe,
+ .disconnect = usbtmc_disconnect
+};
+
+static int __init usbtmc_init(void)
+{
+ int retcode;
+
+ retcode = usb_register(&usbtmc_driver);
+ if (retcode)
+ printk(KERN_ERR KBUILD_MODNAME": Unable to register driver\n");
+ return retcode;
+}
+module_init(usbtmc_init);
+
+static void __exit usbtmc_exit(void)
+{
+ usb_deregister(&usbtmc_driver);
+}
+module_exit(usbtmc_exit);
+
+MODULE_LICENSE("GPL");
--- /dev/null
+++ b/drivers/usb/class/usbtmc.h
@@ -0,0 +1,24 @@
+/*
+ * usbtmc.h
+ * This file is part of a Linux kernel module for USBTMC (USB Test and
+ * Measurement Class) devices
+ *
+ * Copyright (C) 2007 Stefan Kopp, Gechingen, Germany
+ *
+ * See usbtmc.c source file for license details
+ */
+#ifndef __USB_TMC_H
+#define __USB_TMC_H
+
+#include <linux/ioctl.h>
+
+/* Request values for USBTMC driver's ioctl entry point */
+#define USBTMC_IOC_NR 91
+#define USBTMC_IOCTL_INDICATOR_PULSE _IO(USBTMC_IOC_NR, 1)
+#define USBTMC_IOCTL_CLEAR _IO(USBTMC_IOC_NR, 2)
+#define USBTMC_IOCTL_ABORT_BULK_OUT _IO(USBTMC_IOC_NR, 3)
+#define USBTMC_IOCTL_ABORT_BULK_IN _IO(USBTMC_IOC_NR, 4)
+#define USBTMC_IOCTL_CLEAR_OUT_HALT _IO(USBTMC_IOC_NR, 6)
+#define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7)
+
+#endif /* __USB_TMC_H */
--- /dev/null
+++ b/include/linux/usb/tmc.h
@@ -0,0 +1,27 @@
+/*
+ * This file holds USB constants defined by the USB Device Class
+ * Definition for Test and Measurement devices published by the USB-IF.
+ */
+
+#ifndef __LINUX_USB_TMC_H
+#define __LINUX_USB_TMC_H
+
+/* USB TMC status values */
+#define USBTMC_STATUS_SUCCESS 0x01
+#define USBTMC_STATUS_PENDING 0x02
+#define USBTMC_STATUS_FAILED 0x80
+#define USBTMC_STATUS_TRANSFER_NOT_IN_PROGRESS 0x81
+#define USBTMC_STATUS_SPLIT_NOT_IN_PROGRESS 0x82
+#define USBTMC_STATUS_SPLIT_IN_PROGRESS 0x83
+
+/* USB TMC requests values */
+#define USBTMC_REQUEST_INITIATE_ABORT_BULK_OUT 1
+#define USBTMC_REQUEST_CHECK_ABORT_BULK_OUT_STATUS 2
+#define USBTMC_REQUEST_INITIATE_ABORT_BULK_IN 3
+#define USBTMC_REQUEST_CHECK_ABORT_BULK_IN_STATUS 4
+#define USBTMC_REQUEST_INITIATE_CLEAR 5
+#define USBTMC_REQUEST_CHECK_CLEAR_STATUS 6
+#define USBTMC_REQUEST_GET_CAPABILITIES 7
+#define USBTMC_REQUEST_INDICATOR_PULSE 64
+
+#endif
On Tue, Aug 26, 2008 at 05:05:01PM -0700, Greg KH wrote:
> I've now added this to my tree, and any review comments are greatly
> welcome.
Heck, I'll review my own patch, as I noticed I forgot some things:
> ---
> drivers/usb/class/Kconfig | 10
> drivers/usb/class/Makefile | 1
> drivers/usb/class/usbtmc.c | 1058 +++++++++++++++++++++++++++++++++++++++++++++
> drivers/usb/class/usbtmc.h | 24 +
> include/linux/usb/tmc.h | 27 +
> 5 files changed, 1120 insertions(+)
needs Documentation/ABI description of new sysfs files
> +/*
> + * This structure is the capabilities for the device
> + * See section 4.2.1.8 of the USBTMC specification for details.
> + */
> +struct usbtmc_dev_capabilities {
> + __u8 interface_capabilities;
> + __u8 device_capabilities;
> + __u8 usb488_interface_capabilities;
> + __u8 usb488_device_capabilities;
> +};
These should match the usbif description names to make it easier to
understand.
> + /* USBTMC devices have only one setting, so use that */
> + iface_desc = data->intf->cur_altsetting;
> +
> + /* Find bulk in endpoint */
> + for (n = 0; n < iface_desc->desc.bNumEndpoints; n++) {
> + endpoint = &iface_desc->endpoint[n].desc;
> +
> + if (usb_endpoint_is_bulk_in(endpoint)) {
> + data->bulk_in = endpoint->bEndpointAddress;
> + dev_dbg(dev, "Found bulk in endpoint at %u\n",
> + data->bulk_in);
> + break;
> + }
> + }
> +
> + /* Find bulk out endpoint */
> + for (n = 0; n < iface_desc->desc.bNumEndpoints; n++) {
> + endpoint = &iface_desc->endpoint[n].desc;
> +
> + if (usb_endpoint_is_bulk_out(endpoint)) {
> + data->bulk_out = endpoint->bEndpointAddress;
> + dev_dbg(dev, "Found Bulk out endpoint at %u\n",
> + data->bulk_out);
> + break;
> + }
> + }
This can all be done at probe() time, no need to wait until open().
> +static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
> + size_t count, loff_t *f_pos)
> +{
> + struct usbtmc_device_data *data;
> + u8 *buffer;
> + int retval;
> + int actual;
> + unsigned long int n_bytes;
> + int n;
> + int remaining;
> + int done;
> + int this_part;
> +
> + data = filp->private_data;
> + buffer = data->buffer;
This buffer should be local to the function, and not the device as we
could overlap reads and writes. It should be removed from the device
itself to make sure everything is correct. The ioctls already have
their own local buffers, fixing a bug in the original driver which used
1! buffer for all devices and all commands...
thanks,
greg k-h
Hello Greg,
On Wednesday 27 August 2008 02:05:01 Greg KH wrote:
> The autosuspend support isn't that hard either, but I would like to find
> someone with one of these devices to help me in testing that before I
> add it, so I imagine the driver can go into the tree without that being
> added at this time.
I think I can be of some help here. We have usbtmc devices that have this
functionality included. The only difficulty I currently have is time. I'll be
on a business trip for the next 3 weeks but have our units with me and may be
able to test it.
Best regards,
Marcel Janssen
Am Mittwoch 27 August 2008 02:05:01 schrieb Greg KH:
> +???????intf = usb_find_interface(&usbtmc_driver, iminor(inode));
> +???????if (!intf) {
> +???????????????printk(KERN_ERR KBUILD_MODNAME
> +??????????????? ? ? ? ": can not find device for minor %d", iminor(inode));
> +???????????????return -ENODEV;
> +???????}
> +
> +???????data = usb_get_intfdata(intf);
> +???????kref_get(&data->kref);
> +
> +???????/* Store pointer in file structure's private data field */
> +???????filp->private_data = data;
> +
> +???????dev = &data->intf->dev;
> +static void usbtmc_delete(struct kref *kref)
> +{
> + struct usbtmc_device_data *data = to_usbtmc_data(kref);
> +
> + usb_put_dev(data->usb_dev);
> + kfree(data->buffer);
> + kfree(data);
> +}
> +
This is a race condition.
CPU A CPU B
open()
usb_find_interface()
disconnect()
kref_put()
usbtmc_delete()
kfree()
kref_get()
You can write to free memory. You must use a static mutex for
mutual exclusion between open() and disconnect()
Regards
Oliver
Am Mittwoch 27 August 2008 02:05:01 schrieb Greg KH:> +static ssize_t usbtmc_read(struct file *filp, char __user *buf,> +??????????????????????? ? size_t count, loff_t *f_pos)> +{> +???????struct usbtmc_device_data *data;> +???????struct device *dev;> +???????unsigned long int n_characters;> +???????u8 *buffer;> +???????int actual;> +???????int done;> +???????int remaining;> +???????int retval;> +???????int this_part;> +> +???????/* Get pointer to private data structure */> +???????data = filp->private_data;> +???????dev = &data->intf->dev;> +???????buffer = data->buffer;> +> +???????remaining = count;> +???????done = 0;> +> +???????while (remaining > 0) {> +???????????????if (remaining > USBTMC_SIZE_IOBUFFER - 12 - 3)> +???????????????????????this_part = USBTMC_SIZE_IOBUFFER - 12 - 3;> +???????????????else> +???????????????????????this_part = remaining;> +> +???????????????/* Setup IO buffer for DEV_DEP_MSG_IN message> +??????????????? * Refer to class specs for details> +??????????????? */> +???????????????buffer[0] = 2;> +???????????????buffer[1] = data->bTag;> +???????????????buffer[2] = ~(data->bTag);> +???????????????buffer[3] = 0; /* Reserved */> +???????????????buffer[4] = (this_part - 12 - 3) & 255;> +???????????????buffer[5] = ((this_part - 12 - 3) >> 8) & 255;> +???????????????buffer[6] = ((this_part - 12 - 3) >> 16) & 255;> +???????????????buffer[7] = ((this_part - 12 - 3) >> 24) & 255;> +???????????????buffer[8] = data->TermCharEnabled * 2;> +???????????????/* Use term character? */> +???????????????buffer[9] = data->TermChar;> +???????????????buffer[10] = 0; /* Reserved */> +???????????????buffer[11] = 0; /* Reserved */
This function will go bad if concurrent readers enter, yet it has no locking.
Regards Oliver
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
Am Mittwoch 27 August 2008 02:05:01 schrieb Greg KH:
> I've now added this to my tree, and any review comments are greatly
> welcome.
Unfortunately the issues are not trivial.
1. USBTMC_SIZE_IOBUFFER
use 2048. 4096 means a multipage allocation
2. usbtmc_open race with disconnect
already mailed about
3. read/write and, if you convert to unlocked_ioctl, need protection
against reentry
4. read, write & ioctl need protection against using a rebound interface
you add a mutex, that you need for (3) anyway to the descriptor and protected
by this mutex you set the interface and device pointers to null in disconnect
and check them in the affected methods
5.get_capabilities
kmalloced buffer is never freed
6. disconnect fails to remove sysfs attributes
7. probe will report success even if usb_register_dev fails
Regards
Oliver
On Wed, Aug 27, 2008 at 10:56:36AM +0200, Oliver Neukum wrote:
> Am Mittwoch 27 August 2008 02:05:01 schrieb Greg KH:
>
> > I've now added this to my tree, and any review comments are greatly
> > welcome.
>
> Unfortunately the issues are not trivial.
Nah, they are all easily fixable :)
> 1. USBTMC_SIZE_IOBUFFER
> use 2048. 4096 means a multipage allocation
So? What difference does this really matter at normal USB speeds?
thanks,
greg k-h
Am Mittwoch 27 August 2008 17:28:48 schrieb Greg KH:
> On Wed, Aug 27, 2008 at 10:56:36AM +0200, Oliver Neukum wrote:
> So? What difference does this really matter at normal USB speeds?
It doesn't matter in terms of speed for you, but it's nice to be nice
to the rest of the system.
Regards
Oliver
On Wed, Aug 27, 2008 at 10:08:24AM +0200, Oliver Neukum wrote:
> Am Mittwoch 27 August 2008 02:05:01 schrieb Greg KH:
> > +???????intf = usb_find_interface(&usbtmc_driver, iminor(inode));
> > +???????if (!intf) {
> > +???????????????printk(KERN_ERR KBUILD_MODNAME
> > +??????????????? ? ? ? ": can not find device for minor %d", iminor(inode));
> > +???????????????return -ENODEV;
> > +???????}
> > +
> > +???????data = usb_get_intfdata(intf);
> > +???????kref_get(&data->kref);
> > +
> > +???????/* Store pointer in file structure's private data field */
> > +???????filp->private_data = data;
> > +
> > +???????dev = &data->intf->dev;
>
> > +static void usbtmc_delete(struct kref *kref)
> > +{
> > + struct usbtmc_device_data *data = to_usbtmc_data(kref);
> > +
> > + usb_put_dev(data->usb_dev);
> > + kfree(data->buffer);
> > + kfree(data);
> > +}
> > +
>
> This is a race condition.
>
> CPU A CPU B
> open()
> usb_find_interface()
> disconnect()
> kref_put()
> usbtmc_delete()
> kfree()
> kref_get()
>
> You can write to free memory. You must use a static mutex for
> mutual exclusion between open() and disconnect()
Ah, thanks, for some reason I thought that this wasn't needed with the
kref code, but you are correct.
Hm, in looking at the usb driver tree, there are lots of drivers in
there with that problem :)
Now fixed up in this driver.
thanks,
greg k-h
On Wed, Aug 27, 2008 at 05:44:26PM +0200, Oliver Neukum wrote:
> Am Mittwoch 27 August 2008 17:28:48 schrieb Greg KH:
> > On Wed, Aug 27, 2008 at 10:56:36AM +0200, Oliver Neukum wrote:
>
> > So? What difference does this really matter at normal USB speeds?
>
> It doesn't matter in terms of speed for you, but it's nice to be nice
> to the rest of the system.
Sure, it's not a real big deal, if this driver doesn't use the
kmalloc-4096 slab cache, someone else will :)
I'll go change it.
thanks,
greg k-h
On Wed, 27 Aug 2008, Oliver Neukum wrote:
> This is a race condition.
>
> CPU A CPU B
> open()
> usb_find_interface()
> disconnect()
> kref_put()
> usbtmc_delete()
> kfree()
> kref_get()
>
> You can write to free memory. You must use a static mutex for
> mutual exclusion between open() and disconnect()
Is that necessary? usbcore includes its own mutual exclusion now.
Look in file.c at how minor_rwsem is used.
Alan Stern
On Wed, Aug 27, 2008 at 10:13:06AM +0200, Oliver Neukum wrote:
> Am Mittwoch 27 August 2008 02:05:01 schrieb Greg KH:
> > +static ssize_t usbtmc_read(struct file *filp, char __user *buf,
> > +??????????????????????? ? size_t count, loff_t *f_pos)
> > +{
> > +???????struct usbtmc_device_data *data;
> > +???????struct device *dev;
> > +???????unsigned long int n_characters;
> > +???????u8 *buffer;
> > +???????int actual;
> > +???????int done;
> > +???????int remaining;
> > +???????int retval;
> > +???????int this_part;
> > +
> > +???????/* Get pointer to private data structure */
> > +???????data = filp->private_data;
> > +???????dev = &data->intf->dev;
> > +???????buffer = data->buffer;
> > +
> > +???????remaining = count;
> > +???????done = 0;
> > +
> > +???????while (remaining > 0) {
> > +???????????????if (remaining > USBTMC_SIZE_IOBUFFER - 12 - 3)
> > +???????????????????????this_part = USBTMC_SIZE_IOBUFFER - 12 - 3;
> > +???????????????else
> > +???????????????????????this_part = remaining;
> > +
> > +???????????????/* Setup IO buffer for DEV_DEP_MSG_IN message
> > +??????????????? * Refer to class specs for details
> > +??????????????? */
> > +???????????????buffer[0] = 2;
> > +???????????????buffer[1] = data->bTag;
> > +???????????????buffer[2] = ~(data->bTag);
> > +???????????????buffer[3] = 0; /* Reserved */
> > +???????????????buffer[4] = (this_part - 12 - 3) & 255;
> > +???????????????buffer[5] = ((this_part - 12 - 3) >> 8) & 255;
> > +???????????????buffer[6] = ((this_part - 12 - 3) >> 16) & 255;
> > +???????????????buffer[7] = ((this_part - 12 - 3) >> 24) & 255;
> > +???????????????buffer[8] = data->TermCharEnabled * 2;
> > +???????????????/* Use term character? */
> > +???????????????buffer[9] = data->TermChar;
> > +???????????????buffer[10] = 0; /* Reserved */
> > +???????????????buffer[11] = 0; /* Reserved */
>
> This function will go bad if concurrent readers enter, yet it has no locking.
Now fixed with an i/o mutex.
thanks,
greg k-h
On Wed, Aug 27, 2008 at 10:56:36AM +0200, Oliver Neukum wrote:
> 5.get_capabilities
> kmalloced buffer is never freed
Now fixed.
> 6. disconnect fails to remove sysfs attributes
Ick, good catch, now fixed.
> 7. probe will report success even if usb_register_dev fails
now fixed.
thanks a lot for the review.
greg k-h
Ok, as I've responded to this multiple times, now summarizing so I make
sure I got everything...
On Wed, Aug 27, 2008 at 10:56:36AM +0200, Oliver Neukum wrote:
> Am Mittwoch 27 August 2008 02:05:01 schrieb Greg KH:
>
> > I've now added this to my tree, and any review comments are greatly
> > welcome.
>
> Unfortunately the issues are not trivial.
>
> 1. USBTMC_SIZE_IOBUFFER
> use 2048. 4096 means a multipage allocation
changed.
> 2. usbtmc_open race with disconnect
> already mailed about
fixed.
> 3. read/write and, if you convert to unlocked_ioctl, need protection
> against reentry
fixed and changed to unlocked_ioctl.
> 4. read, write & ioctl need protection against using a rebound interface
> you add a mutex, that you need for (3) anyway to the descriptor and protected
> by this mutex you set the interface and device pointers to null in disconnect
> and check them in the affected methods
Should now be fixed.
> 5.get_capabilities
> kmalloced buffer is never freed
fixed.
> 6. disconnect fails to remove sysfs attributes
fixed.
> 7. probe will report success even if usb_register_dev fails
fixed.
I'll post a new version now.
thanks a lot for the review, I really appreciate it.
greg k-h
On Tue, Aug 26, 2008 at 08:12:05PM -0700, Greg KH wrote:
> On Tue, Aug 26, 2008 at 05:05:01PM -0700, Greg KH wrote:
> > I've now added this to my tree, and any review comments are greatly
> > welcome.
>
> Heck, I'll review my own patch, as I noticed I forgot some things:
>
> > ---
> > drivers/usb/class/Kconfig | 10
> > drivers/usb/class/Makefile | 1
> > drivers/usb/class/usbtmc.c | 1058 +++++++++++++++++++++++++++++++++++++++++++++
> > drivers/usb/class/usbtmc.h | 24 +
> > include/linux/usb/tmc.h | 27 +
> > 5 files changed, 1120 insertions(+)
>
> needs Documentation/ABI description of new sysfs files
Now added.
> > +/*
> > + * This structure is the capabilities for the device
> > + * See section 4.2.1.8 of the USBTMC specification for details.
> > + */
> > +struct usbtmc_dev_capabilities {
> > + __u8 interface_capabilities;
> > + __u8 device_capabilities;
> > + __u8 usb488_interface_capabilities;
> > + __u8 usb488_device_capabilities;
> > +};
>
> These should match the usbif description names to make it easier to
> understand.
Nevermind, they match the spec description just fine, these will stay.
> > + /* USBTMC devices have only one setting, so use that */
> > + iface_desc = data->intf->cur_altsetting;
> > +
> > + /* Find bulk in endpoint */
> > + for (n = 0; n < iface_desc->desc.bNumEndpoints; n++) {
> > + endpoint = &iface_desc->endpoint[n].desc;
> > +
> > + if (usb_endpoint_is_bulk_in(endpoint)) {
> > + data->bulk_in = endpoint->bEndpointAddress;
> > + dev_dbg(dev, "Found bulk in endpoint at %u\n",
> > + data->bulk_in);
> > + break;
> > + }
> > + }
> > +
> > + /* Find bulk out endpoint */
> > + for (n = 0; n < iface_desc->desc.bNumEndpoints; n++) {
> > + endpoint = &iface_desc->endpoint[n].desc;
> > +
> > + if (usb_endpoint_is_bulk_out(endpoint)) {
> > + data->bulk_out = endpoint->bEndpointAddress;
> > + dev_dbg(dev, "Found Bulk out endpoint at %u\n",
> > + data->bulk_out);
> > + break;
> > + }
> > + }
>
> This can all be done at probe() time, no need to wait until open().
Now moved.
> > +static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
> > + size_t count, loff_t *f_pos)
> > +{
> > + struct usbtmc_device_data *data;
> > + u8 *buffer;
> > + int retval;
> > + int actual;
> > + unsigned long int n_bytes;
> > + int n;
> > + int remaining;
> > + int done;
> > + int this_part;
> > +
> > + data = filp->private_data;
> > + buffer = data->buffer;
>
> This buffer should be local to the function, and not the device as we
> could overlap reads and writes. It should be removed from the device
> itself to make sure everything is correct. The ioctls already have
> their own local buffers, fixing a bug in the original driver which used
> 1! buffer for all devices and all commands...
Now fixed.
It's fun talking to yourself...
greg k-h
Am Mittwoch 27 August 2008 18:06:23 schrieb Alan Stern:
> On Wed, 27 Aug 2008, Oliver Neukum wrote:
>
> > This is a race condition.
> >
> > CPU A CPU B
> > open()
> > usb_find_interface()
> > disconnect()
> > kref_put()
> > usbtmc_delete()
> > kfree()
> > kref_get()
> >
> > You can write to free memory. You must use a static mutex for
> > mutual exclusion between open() and disconnect()
>
> Is that necessary? usbcore includes its own mutual exclusion now.
> Look in file.c at how minor_rwsem is used.
This is interesting, the driver simply doesn't unregister the device.
So it would be needed if the code could be left as it is. As not unregistering
the device is wrong, this will fix the bug.
Thanks
Oliver
On Wed, Aug 27, 2008 at 08:22:58AM +0200, Marcel Janssen wrote:
> Hello Greg,
>
> On Wednesday 27 August 2008 02:05:01 Greg KH wrote:
> > The autosuspend support isn't that hard either, but I would like to find
> > someone with one of these devices to help me in testing that before I
> > add it, so I imagine the driver can go into the tree without that being
> > added at this time.
>
> I think I can be of some help here. We have usbtmc devices that have this
> functionality included. The only difficulty I currently have is time. I'll be
> on a business trip for the next 3 weeks but have our units with me and may be
> able to test it.
That's great. How about a basic "the driver works or not" type testing
of what we have already?
After that, we can work on autosuspend support.
thanks,
greg k-h