2008-08-27 17:29:17

by Greg KH

[permalink] [raw]
Subject: [PATCH] USB: add USB test and measurement class driver - round 2

Here's an updated version of the usbtmc driver, with all of the
different issues that have been raised, hopefully addressed.

thanks,

greg k-h

----------
From: Greg Kroah-Hartman <[email protected]>
Subject: USB: add USB test and measurement class driver

This driver was originaly written by Stefan Kopp, but massively
reworked by Greg for submission.

Thanks to Felipe Balbi <[email protected]> for lots of work in cleaning
up this driver.

Thanks to Oliver Neukum <[email protected]> for reviewing previous
versions and pointing out problems.


Cc: Stefan Kopp <[email protected]>
Cc: Marcel Janssen <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>

---
Documentation/ABI/stable/sysfs-driver-usb-usbtmc | 62 +
Documentation/devices.txt | 3
Documentation/ioctl-number.txt | 2
drivers/usb/class/Kconfig | 10
drivers/usb/class/Makefile | 1
drivers/usb/class/usbtmc.c | 1095 +++++++++++++++++++++++
include/linux/usb/Kbuild | 2
include/linux/usb/tmc.h | 43
8 files changed, 1217 insertions(+), 1 deletion(-)

--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-driver-usb-usbtmc
@@ -0,0 +1,62 @@
+What: /sys/bus/usb/drivers/usbtmc/devices/*/interface_capabilities
+What: /sys/bus/usb/drivers/usbtmc/devices/*/device_capabilities
+Date: August 2008
+Contact: Greg Kroah-Hartman <[email protected]>
+Description:
+ These files show the various USB TMC capabilities as described
+ by the device itself. The full description of the bitfields
+ can be found in the USB TMC documents from the USB-IF entitled
+ "Universal Serial Bus Test and Measurement Class Specification
+ (USBTMC) Revision 1.0" section 4.2.1.8.
+
+ The files are read only.
+
+
+What: /sys/bus/usb/drivers/usbtmc/devices/*/usb488_interface_capabilities
+What: /sys/bus/usb/drivers/usbtmc/devices/*/usb488_device_capabilities
+Date: August 2008
+Contact: Greg Kroah-Hartman <[email protected]>
+Description:
+ These files show the various USB TMC capabilities as described
+ by the device itself. The full description of the bitfields
+ can be found in the USB TMC documents from the USB-IF entitled
+ "Universal Serial Bus Test and Measurement Class, Subclass
+ USB488 Specification (USBTMC-USB488) Revision 1.0" section
+ 4.2.2.
+
+ The files are read only.
+
+
+What: /sys/bus/usb/drivers/usbtmc/devices/*/TermChar
+Date: August 2008
+Contact: Greg Kroah-Hartman <[email protected]>
+Description:
+ This file is the TermChar value to be sent to the USB TMC
+ device as described by the document, "Universal Serial Bus Test
+ and Measurement Class Specification
+ (USBTMC) Revision 1.0" as published by the USB-IF.
+
+ Note that the TermCharEnabled file determines if this value is
+ sent to the device or not.
+
+
+What: /sys/bus/usb/drivers/usbtmc/devices/*/TermCharEnabled
+Date: August 2008
+Contact: Greg Kroah-Hartman <[email protected]>
+Description:
+ This file determines if the TermChar is to be sent to the
+ device on every transaction or not. For more details about
+ this, please see the document, "Universal Serial Bus Test and
+ Measurement Class Specification (USBTMC) Revision 1.0" as
+ published by the USB-IF.
+
+
+What: /sys/bus/usb/drivers/usbtmc/devices/*/auto_abort
+Date: August 2008
+Contact: Greg Kroah-Hartman <[email protected]>
+Description:
+ This file determines if the the transaction of the USB TMC
+ device is to be automatically aborted if there is any error.
+ For more details about this, please see the document,
+ "Universal Serial Bus Test and Measurement Class Specification
+ (USBTMC) Revision 1.0" as published by the USB-IF.
--- a/Documentation/devices.txt
+++ b/Documentation/devices.txt
@@ -2571,6 +2571,9 @@ Your cooperation is appreciated.
160 = /dev/usb/legousbtower0 1st USB Legotower device
...
175 = /dev/usb/legousbtower15 16th USB Legotower device
+ 176 = /dev/usb/usbtmc1 First USB TMC device
+ ...
+ 192 = /dev/usb/usbtmc16 16th USB TMC device
240 = /dev/usb/dabusb0 First daubusb device
...
243 = /dev/usb/dabusb3 Fourth dabusb device
--- a/Documentation/ioctl-number.txt
+++ b/Documentation/ioctl-number.txt
@@ -110,6 +110,8 @@ Code Seq# Include File Comments
'W' 00-1F linux/wanrouter.h conflict!
'X' all linux/xfs_fs.h
'Y' all linux/cyclades.h
+'[' 00-07 linux/usb/usbtmc.h USB Test and Measurement Devices
+ <mailto:[email protected]>
'a' all ATM on linux
<http://lrcwww.epfl.ch/linux-atm/magic.html>
'b' 00-FF bit3 vme host bridge
--- 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,1095 @@
+/**
+ * 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>
+
+
+#define USBTMC_MINOR_BASE 176
+
+/*
+ * 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 2048
+
+/* 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;
+
+ struct usbtmc_dev_capabilities capabilities;
+ struct kref kref;
+ struct mutex io_mutex; /* only one i/o function running at a time */
+};
+#define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
+
+/* Forward declarations */
+static struct usb_driver usbtmc_driver;
+
+static struct mutex usbtmc_mutex;
+
+static void usbtmc_delete(struct kref *kref)
+{
+ struct usbtmc_device_data *data = to_usbtmc_data(kref);
+
+ usb_put_dev(data->usb_dev);
+ kfree(data);
+}
+
+static int usbtmc_open(struct inode *inode, struct file *filp)
+{
+ struct usb_interface *intf;
+ struct usbtmc_device_data *data;
+ int retval = -ENODEV;
+
+ mutex_lock(&usbtmc_mutex);
+
+ intf = usb_find_interface(&usbtmc_driver, iminor(inode));
+ if (!intf) {
+ printk(KERN_ERR KBUILD_MODNAME
+ ": can not find device for minor %d", iminor(inode));
+ goto exit;
+ }
+
+ data = usb_get_intfdata(intf);
+ kref_get(&data->kref);
+
+ /* Store pointer in file structure's private data field */
+ filp->private_data = data;
+
+exit:
+ mutex_unlock(&usbtmc_mutex);
+ return retval;
+}
+
+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 usbtmc_device_data *data)
+{
+ char *buffer;
+ struct device *dev;
+ int rv;
+ int n;
+ int actual;
+ struct usb_host_interface *current_setting;
+ int max_size;
+
+ 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", 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 usbtmc_device_data *data)
+{
+ struct device *dev;
+ u8 *buffer;
+ int rv;
+ int n;
+
+ 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 = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ mutex_lock(&data->io_mutex);
+
+ 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(data);
+ goto exit;
+ }
+
+ /* 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(data);
+ goto exit;
+ }
+
+ /* 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 */
+ retval = -EFAULT;
+ goto exit;
+ }
+
+ done += n_characters;
+ if (n_characters < USBTMC_SIZE_IOBUFFER)
+ remaining = 0;
+ }
+
+ /* Update file position value */
+ *f_pos = *f_pos + done;
+ retval = done;
+
+exit:
+ mutex_unlock(&data->io_mutex);
+ kfree(buffer);
+ return retval;
+}
+
+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 = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ mutex_lock(&data->io_mutex);
+
+ 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)) {
+ retval = -EFAULT;
+ goto exit;
+ }
+
+ 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(data);
+ goto exit;
+ }
+
+ remaining -= this_part;
+ done += this_part;
+ }
+
+ retval = count;
+exit:
+ mutex_unlock(&data->io_mutex);
+ kfree(buffer);
+ return retval;
+}
+
+static int usbtmc_ioctl_clear(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;
+
+ 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 = &current_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 usbtmc_device_data *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 usbtmc_device_data *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];
+
+ kfree(buffer);
+ 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,
+};
+
+static ssize_t show_TermChar(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, "%c\n", data->TermChar);
+}
+
+static ssize_t store_TermChar(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);
+
+ if (count < 1)
+ return -EINVAL;
+ data->TermChar = buf[0];
+ return count;
+}
+static DEVICE_ATTR(TermChar, S_IRUGO, show_TermChar, store_TermChar);
+
+#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(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 usbtmc_device_data *data)
+{
+ struct device *dev;
+ u8 *buffer;
+ int rv;
+
+ 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 long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ struct usbtmc_device_data *data;
+ int retval = -EBADRQC;
+
+ data = file->private_data;
+ mutex_lock(&data->io_mutex);
+
+ switch (cmd) {
+ case USBTMC_IOCTL_CLEAR_OUT_HALT:
+ retval = usbtmc_ioctl_clear_out_halt(data);
+
+ case USBTMC_IOCTL_CLEAR_IN_HALT:
+ retval = usbtmc_ioctl_clear_in_halt(data);
+
+ case USBTMC_IOCTL_INDICATOR_PULSE:
+ retval = usbtmc_ioctl_indicator_pulse(data);
+
+ case USBTMC_IOCTL_CLEAR:
+ retval = usbtmc_ioctl_clear(data);
+
+ case USBTMC_IOCTL_ABORT_BULK_OUT:
+ retval = usbtmc_ioctl_abort_bulk_out(data);
+
+ case USBTMC_IOCTL_ABORT_BULK_IN:
+ retval = usbtmc_ioctl_abort_bulk_in(data);
+ }
+
+ mutex_unlock(&data->io_mutex);
+ return retval;
+}
+
+static struct file_operations fops = {
+ .owner = THIS_MODULE,
+ .read = usbtmc_read,
+ .write = usbtmc_write,
+ .open = usbtmc_open,
+ .release = usbtmc_release,
+ .unlocked_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)
+{
+ struct usbtmc_device_data *data;
+ struct usb_host_interface *iface_desc;
+ struct usb_endpoint_descriptor *endpoint;
+ int n;
+ int retcode;
+
+ dev_dbg(&intf->dev, "%s called\n", __func__);
+
+ data = kmalloc(sizeof(struct usbtmc_device_data), GFP_KERNEL);
+ if (!data) {
+ dev_err(&intf->dev, "Unable to allocate kernel memory\n");
+ 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);
+ mutex_init(&data->io_mutex);
+
+ /* Initialize USBTMC bTag and other fields */
+ data->bTag = 1;
+ data->TermCharEnabled = 0;
+ data->TermChar = '\n';
+
+ /* 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(&intf->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(&intf->dev, "Found Bulk out endpoint at %u\n",
+ data->bulk_out);
+ break;
+ }
+ }
+
+ 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);
+ goto error_register;
+ }
+ dev_dbg(&intf->dev, "Using minor number %d\n", intf->minor);
+
+ return 0;
+
+error_register:
+ sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
+ sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
+ kref_put(&data->kref, usbtmc_delete);
+ return retcode;
+}
+
+static void usbtmc_disconnect(struct usb_interface *intf)
+{
+ struct usbtmc_device_data *data;
+
+ dev_dbg(&intf->dev, "usbtmc_disconnect called\n");
+
+ mutex_lock(&usbtmc_mutex);
+ data = usb_get_intfdata(intf);
+ sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
+ sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
+ kref_put(&data->kref, usbtmc_delete);
+ mutex_unlock(&usbtmc_mutex);
+}
+
+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;
+
+ mutex_init(&usbtmc_mutex);
+
+ 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");
--- a/include/linux/usb/Kbuild
+++ b/include/linux/usb/Kbuild
@@ -4,4 +4,4 @@ header-y += ch9.h
header-y += gadgetfs.h
header-y += midi.h
header-y += g_printer.h
-
+header-y += tmc.h
--- /dev/null
+++ b/include/linux/usb/tmc.h
@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2007 Stefan Kopp, Gechingen, Germany
+ * Copyright (C) 2008 Novell, Inc.
+ * Copyright (C) 2008 Greg Kroah-Hartman <[email protected]>
+ *
+ * This file holds USB constants defined by the USB Device Class
+ * Definition for Test and Measurement devices published by the USB-IF.
+ *
+ * It also has the ioctl definitions for the usbtmc kernel driver that
+ * userspace needs to know about.
+ */
+
+#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
+
+/* 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


2008-08-27 18:28:36

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

On Wed, 27 Aug 2008, Greg KH wrote:

> Here's an updated version of the usbtmc driver, with all of the
> different issues that have been raised, hopefully addressed.

This is an example of what I was discussing with Oliver. In all
likelihood you simply don't need usbtmc_mutex, and using it will cause
a lockdep violation.

That's why so many of the other USB class drivers don't have an
analogous static mutex.

> +static int usbtmc_open(struct inode *inode, struct file *filp)
> +{
> + struct usb_interface *intf;
> + struct usbtmc_device_data *data;
> + int retval = -ENODEV;
> +
> + mutex_lock(&usbtmc_mutex);

You must never acquire a lock in your open method if it will be held
by your disconnect method while unregistering the minor.

> +static int usbtmc_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
...
> + 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);
> + goto error_register;
> + }
> + dev_dbg(&intf->dev, "Using minor number %d\n", intf->minor);

You do call usb_register_dev() during probe...

> +
> + return 0;
> +
> +error_register:
> + sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
> + sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
> + kref_put(&data->kref, usbtmc_delete);
> + return retcode;
> +}
> +
> +static void usbtmc_disconnect(struct usb_interface *intf)
> +{
> + struct usbtmc_device_data *data;
> +
> + dev_dbg(&intf->dev, "usbtmc_disconnect called\n");
> +
> + mutex_lock(&usbtmc_mutex);
> + data = usb_get_intfdata(intf);
> + sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
> + sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
> + kref_put(&data->kref, usbtmc_delete);
> + mutex_unlock(&usbtmc_mutex);
> +}

But you don't call usb_unregister_dev() during disconnect. An
oversight?

Alan Stern

2008-08-27 18:36:46

by Oliver Neukum

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

Am Mittwoch 27 August 2008 20:28:22 schrieb Alan Stern:
> On Wed, 27 Aug 2008, Greg KH wrote:
>
> > Here's an updated version of the usbtmc driver, with all of the
> > different issues that have been raised, hopefully addressed.
>
> This is an example of what I was discussing with Oliver. ?In all
> likelihood you simply don't need usbtmc_mutex, and using it will cause
> a lockdep violation.
>
> That's why so many of the other USB class drivers don't have an
> analogous static mutex.
>
> > +static int usbtmc_open(struct inode *inode, struct file *filp)
> > +{
> > +?????struct usb_interface *intf;
> > +?????struct usbtmc_device_data *data;
> > +?????int retval = -ENODEV;
> > +
> > +?????mutex_lock(&usbtmc_mutex);
>
> You must never acquire a lock in your open method if it will be held
> by your disconnect method while unregistering the minor.

And this rule depends on sharing the USB major or not. This needs
a big fat mention in Documentation.

Regards
Oliver

2008-08-27 18:42:17

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

On Wed, Aug 27, 2008 at 02:28:22PM -0400, Alan Stern wrote:
> On Wed, 27 Aug 2008, Greg KH wrote:
>
> > Here's an updated version of the usbtmc driver, with all of the
> > different issues that have been raised, hopefully addressed.
>
> This is an example of what I was discussing with Oliver. In all
> likelihood you simply don't need usbtmc_mutex, and using it will cause
> a lockdep violation.
>
> That's why so many of the other USB class drivers don't have an
> analogous static mutex.

Ok, then it's just safe to drop this static mutex entirely, right?

> > +static int usbtmc_open(struct inode *inode, struct file *filp)
> > +{
> > + struct usb_interface *intf;
> > + struct usbtmc_device_data *data;
> > + int retval = -ENODEV;
> > +
> > + mutex_lock(&usbtmc_mutex);
>
> You must never acquire a lock in your open method if it will be held
> by your disconnect method while unregistering the minor.
>
> > +static int usbtmc_probe(struct usb_interface *intf,
> > + const struct usb_device_id *id)
> > +{
> ...
> > + 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);
> > + goto error_register;
> > + }
> > + dev_dbg(&intf->dev, "Using minor number %d\n", intf->minor);
>
> You do call usb_register_dev() during probe...
>
> > +
> > + return 0;
> > +
> > +error_register:
> > + sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
> > + sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
> > + kref_put(&data->kref, usbtmc_delete);
> > + return retcode;
> > +}
> > +
> > +static void usbtmc_disconnect(struct usb_interface *intf)
> > +{
> > + struct usbtmc_device_data *data;
> > +
> > + dev_dbg(&intf->dev, "usbtmc_disconnect called\n");
> > +
> > + mutex_lock(&usbtmc_mutex);
> > + data = usb_get_intfdata(intf);
> > + sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
> > + sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
> > + kref_put(&data->kref, usbtmc_delete);
> > + mutex_unlock(&usbtmc_mutex);
> > +}
>
> But you don't call usb_unregister_dev() during disconnect. An
> oversight?

Ah, yes, my fault, I converted this from using a private cdev to using
the usb_register_dev() call and forgot about this. Thanks for catching
it, I'll go fix this.

Oh, it's usb_deregister_dev(), stupid programmers with inconsitant
interfaces :)

thanks,

greg k-h

2008-08-27 18:58:20

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

On Wed, 27 Aug 2008, Greg KH wrote:

> On Wed, Aug 27, 2008 at 02:28:22PM -0400, Alan Stern wrote:
> > On Wed, 27 Aug 2008, Greg KH wrote:
> >
> > > Here's an updated version of the usbtmc driver, with all of the
> > > different issues that have been raised, hopefully addressed.
> >
> > This is an example of what I was discussing with Oliver. In all
> > likelihood you simply don't need usbtmc_mutex, and using it will cause
> > a lockdep violation.
> >
> > That's why so many of the other USB class drivers don't have an
> > analogous static mutex.
>
> Ok, then it's just safe to drop this static mutex entirely, right?

Yes, once you add the call to usb_deregister_dev.


On Wed, 27 Aug 2008, Oliver Neukum wrote:

> And this rule depends on sharing the USB major or not. This needs
> a big fat mention in Documentation.

You mean that the open/disconnect locking rule applies only to drivers
that call usb_register_dev, i.e, drivers using the USB major. Right?

I agree that it deserves to be mentioned in the documentation
somewhere. Where would be a good place? None of the existing files in
Documentation/usb seem appropriate.

Alan Stern

2008-08-27 19:04:46

by Oliver Neukum

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

Am Mittwoch 27 August 2008 20:58:08 schrieb Alan Stern:
> > And this rule depends on sharing the USB major or not. This needs
> > a big fat mention in Documentation.
>
> You mean that the open/disconnect locking rule applies only to drivers
> that call usb_register_dev, i.e, drivers using the USB major. ?Right?

Yes. In fact drivers not using the USB major but their own char devices
will need such a lock. This is tricky.

> I agree that it deserves to be mentioned in the documentation
> somewhere. ?Where would be a good place? ?None of the existing files in
> Documentation/usb seem appropriate.

The USB major merits a file of its own.

Regards
Oliver

2008-08-27 19:16:29

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

On Wed, 27 Aug 2008, Oliver Neukum wrote:

> Am Mittwoch 27 August 2008 20:58:08 schrieb Alan Stern:
> > > And this rule depends on sharing the USB major or not. This needs
> > > a big fat mention in Documentation.
> >
> > You mean that the open/disconnect locking rule applies only to drivers
> > that call usb_register_dev, i.e, drivers using the USB major. ?Right?
>
> Yes. In fact drivers not using the USB major but their own char devices
> will need such a lock. This is tricky.

IMO all char-device registration/deregistration routines should use a
similar rwsem. Then device drivers wouldn't need to worry about it.

> > I agree that it deserves to be mentioned in the documentation
> > somewhere. ?Where would be a good place? ?None of the existing files in
> > Documentation/usb seem appropriate.
>
> The USB major merits a file of its own.

I don't feel competent to start writing such a file. Greg, what do you
think? Maybe all that's needed to get going is to "borrow" some of the
material in LDD3. :-)

Alan Stern

2008-08-28 00:01:32

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

On Wed, Aug 27, 2008 at 02:58:08PM -0400, Alan Stern wrote:
> On Wed, 27 Aug 2008, Greg KH wrote:
>
> > On Wed, Aug 27, 2008 at 02:28:22PM -0400, Alan Stern wrote:
> > > On Wed, 27 Aug 2008, Greg KH wrote:
> > >
> > > > Here's an updated version of the usbtmc driver, with all of the
> > > > different issues that have been raised, hopefully addressed.
> > >
> > > This is an example of what I was discussing with Oliver. In all
> > > likelihood you simply don't need usbtmc_mutex, and using it will cause
> > > a lockdep violation.
> > >
> > > That's why so many of the other USB class drivers don't have an
> > > analogous static mutex.
> >
> > Ok, then it's just safe to drop this static mutex entirely, right?
>
> Yes, once you add the call to usb_deregister_dev.

Great, all done now.

Here's the updated version.

thanks,

greg k-h
----------
From: Greg Kroah-Hartman <[email protected]>
Subject: USB: add USB test and measurement class driver

This driver was originaly written by Stefan Kopp, but massively
reworked by Greg for submission.

Thanks to Felipe Balbi <[email protected]> for lots of work in cleaning
up this driver.

Thanks to Oliver Neukum <[email protected]> for reviewing previous
versions and pointing out problems.


Cc: Stefan Kopp <[email protected]>
Cc: Marcel Janssen <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Oliver Neukum <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>

---
Documentation/ABI/stable/sysfs-driver-usb-usbtmc | 62 +
Documentation/devices.txt | 3
Documentation/ioctl-number.txt | 2
drivers/usb/class/Kconfig | 10
drivers/usb/class/Makefile | 1
drivers/usb/class/usbtmc.c | 1087 +++++++++++++++++++++++
include/linux/usb/Kbuild | 2
include/linux/usb/tmc.h | 43
8 files changed, 1209 insertions(+), 1 deletion(-)

--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-driver-usb-usbtmc
@@ -0,0 +1,62 @@
+What: /sys/bus/usb/drivers/usbtmc/devices/*/interface_capabilities
+What: /sys/bus/usb/drivers/usbtmc/devices/*/device_capabilities
+Date: August 2008
+Contact: Greg Kroah-Hartman <[email protected]>
+Description:
+ These files show the various USB TMC capabilities as described
+ by the device itself. The full description of the bitfields
+ can be found in the USB TMC documents from the USB-IF entitled
+ "Universal Serial Bus Test and Measurement Class Specification
+ (USBTMC) Revision 1.0" section 4.2.1.8.
+
+ The files are read only.
+
+
+What: /sys/bus/usb/drivers/usbtmc/devices/*/usb488_interface_capabilities
+What: /sys/bus/usb/drivers/usbtmc/devices/*/usb488_device_capabilities
+Date: August 2008
+Contact: Greg Kroah-Hartman <[email protected]>
+Description:
+ These files show the various USB TMC capabilities as described
+ by the device itself. The full description of the bitfields
+ can be found in the USB TMC documents from the USB-IF entitled
+ "Universal Serial Bus Test and Measurement Class, Subclass
+ USB488 Specification (USBTMC-USB488) Revision 1.0" section
+ 4.2.2.
+
+ The files are read only.
+
+
+What: /sys/bus/usb/drivers/usbtmc/devices/*/TermChar
+Date: August 2008
+Contact: Greg Kroah-Hartman <[email protected]>
+Description:
+ This file is the TermChar value to be sent to the USB TMC
+ device as described by the document, "Universal Serial Bus Test
+ and Measurement Class Specification
+ (USBTMC) Revision 1.0" as published by the USB-IF.
+
+ Note that the TermCharEnabled file determines if this value is
+ sent to the device or not.
+
+
+What: /sys/bus/usb/drivers/usbtmc/devices/*/TermCharEnabled
+Date: August 2008
+Contact: Greg Kroah-Hartman <[email protected]>
+Description:
+ This file determines if the TermChar is to be sent to the
+ device on every transaction or not. For more details about
+ this, please see the document, "Universal Serial Bus Test and
+ Measurement Class Specification (USBTMC) Revision 1.0" as
+ published by the USB-IF.
+
+
+What: /sys/bus/usb/drivers/usbtmc/devices/*/auto_abort
+Date: August 2008
+Contact: Greg Kroah-Hartman <[email protected]>
+Description:
+ This file determines if the the transaction of the USB TMC
+ device is to be automatically aborted if there is any error.
+ For more details about this, please see the document,
+ "Universal Serial Bus Test and Measurement Class Specification
+ (USBTMC) Revision 1.0" as published by the USB-IF.
--- a/Documentation/devices.txt
+++ b/Documentation/devices.txt
@@ -2571,6 +2571,9 @@ Your cooperation is appreciated.
160 = /dev/usb/legousbtower0 1st USB Legotower device
...
175 = /dev/usb/legousbtower15 16th USB Legotower device
+ 176 = /dev/usb/usbtmc1 First USB TMC device
+ ...
+ 192 = /dev/usb/usbtmc16 16th USB TMC device
240 = /dev/usb/dabusb0 First daubusb device
...
243 = /dev/usb/dabusb3 Fourth dabusb device
--- a/Documentation/ioctl-number.txt
+++ b/Documentation/ioctl-number.txt
@@ -110,6 +110,8 @@ Code Seq# Include File Comments
'W' 00-1F linux/wanrouter.h conflict!
'X' all linux/xfs_fs.h
'Y' all linux/cyclades.h
+'[' 00-07 linux/usb/usbtmc.h USB Test and Measurement Devices
+ <mailto:[email protected]>
'a' all ATM on linux
<http://lrcwww.epfl.ch/linux-atm/magic.html>
'b' 00-FF bit3 vme host bridge
--- 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,1087 @@
+/**
+ * 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>
+
+
+#define USBTMC_MINOR_BASE 176
+
+/*
+ * 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 2048
+
+/* 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;
+
+ struct usbtmc_dev_capabilities capabilities;
+ struct kref kref;
+ struct mutex io_mutex; /* only one i/o function running at a time */
+};
+#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);
+}
+
+static int usbtmc_open(struct inode *inode, struct file *filp)
+{
+ struct usb_interface *intf;
+ struct usbtmc_device_data *data;
+ int retval = -ENODEV;
+
+ intf = usb_find_interface(&usbtmc_driver, iminor(inode));
+ if (!intf) {
+ printk(KERN_ERR KBUILD_MODNAME
+ ": can not find device for minor %d", iminor(inode));
+ goto exit;
+ }
+
+ data = usb_get_intfdata(intf);
+ kref_get(&data->kref);
+
+ /* Store pointer in file structure's private data field */
+ filp->private_data = data;
+
+exit:
+ return retval;
+}
+
+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 usbtmc_device_data *data)
+{
+ char *buffer;
+ struct device *dev;
+ int rv;
+ int n;
+ int actual;
+ struct usb_host_interface *current_setting;
+ int max_size;
+
+ 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", 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 usbtmc_device_data *data)
+{
+ struct device *dev;
+ u8 *buffer;
+ int rv;
+ int n;
+
+ 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 = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ mutex_lock(&data->io_mutex);
+
+ 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(data);
+ goto exit;
+ }
+
+ /* 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(data);
+ goto exit;
+ }
+
+ /* 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 */
+ retval = -EFAULT;
+ goto exit;
+ }
+
+ done += n_characters;
+ if (n_characters < USBTMC_SIZE_IOBUFFER)
+ remaining = 0;
+ }
+
+ /* Update file position value */
+ *f_pos = *f_pos + done;
+ retval = done;
+
+exit:
+ mutex_unlock(&data->io_mutex);
+ kfree(buffer);
+ return retval;
+}
+
+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 = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ mutex_lock(&data->io_mutex);
+
+ 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)) {
+ retval = -EFAULT;
+ goto exit;
+ }
+
+ 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(data);
+ goto exit;
+ }
+
+ remaining -= this_part;
+ done += this_part;
+ }
+
+ retval = count;
+exit:
+ mutex_unlock(&data->io_mutex);
+ kfree(buffer);
+ return retval;
+}
+
+static int usbtmc_ioctl_clear(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;
+
+ 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 = &current_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 usbtmc_device_data *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 usbtmc_device_data *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];
+
+ kfree(buffer);
+ 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,
+};
+
+static ssize_t show_TermChar(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, "%c\n", data->TermChar);
+}
+
+static ssize_t store_TermChar(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);
+
+ if (count < 1)
+ return -EINVAL;
+ data->TermChar = buf[0];
+ return count;
+}
+static DEVICE_ATTR(TermChar, S_IRUGO, show_TermChar, store_TermChar);
+
+#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(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 usbtmc_device_data *data)
+{
+ struct device *dev;
+ u8 *buffer;
+ int rv;
+
+ 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 long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ struct usbtmc_device_data *data;
+ int retval = -EBADRQC;
+
+ data = file->private_data;
+ mutex_lock(&data->io_mutex);
+
+ switch (cmd) {
+ case USBTMC_IOCTL_CLEAR_OUT_HALT:
+ retval = usbtmc_ioctl_clear_out_halt(data);
+
+ case USBTMC_IOCTL_CLEAR_IN_HALT:
+ retval = usbtmc_ioctl_clear_in_halt(data);
+
+ case USBTMC_IOCTL_INDICATOR_PULSE:
+ retval = usbtmc_ioctl_indicator_pulse(data);
+
+ case USBTMC_IOCTL_CLEAR:
+ retval = usbtmc_ioctl_clear(data);
+
+ case USBTMC_IOCTL_ABORT_BULK_OUT:
+ retval = usbtmc_ioctl_abort_bulk_out(data);
+
+ case USBTMC_IOCTL_ABORT_BULK_IN:
+ retval = usbtmc_ioctl_abort_bulk_in(data);
+ }
+
+ mutex_unlock(&data->io_mutex);
+ return retval;
+}
+
+static struct file_operations fops = {
+ .owner = THIS_MODULE,
+ .read = usbtmc_read,
+ .write = usbtmc_write,
+ .open = usbtmc_open,
+ .release = usbtmc_release,
+ .unlocked_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)
+{
+ struct usbtmc_device_data *data;
+ struct usb_host_interface *iface_desc;
+ struct usb_endpoint_descriptor *endpoint;
+ int n;
+ int retcode;
+
+ dev_dbg(&intf->dev, "%s called\n", __func__);
+
+ data = kmalloc(sizeof(struct usbtmc_device_data), GFP_KERNEL);
+ if (!data) {
+ dev_err(&intf->dev, "Unable to allocate kernel memory\n");
+ 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);
+ mutex_init(&data->io_mutex);
+
+ /* Initialize USBTMC bTag and other fields */
+ data->bTag = 1;
+ data->TermCharEnabled = 0;
+ data->TermChar = '\n';
+
+ /* 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(&intf->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(&intf->dev, "Found Bulk out endpoint at %u\n",
+ data->bulk_out);
+ break;
+ }
+ }
+
+ 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);
+ goto error_register;
+ }
+ dev_dbg(&intf->dev, "Using minor number %d\n", intf->minor);
+
+ return 0;
+
+error_register:
+ sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
+ sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
+ kref_put(&data->kref, usbtmc_delete);
+ return retcode;
+}
+
+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);
+ usb_deregister_dev(intf, &usbtmc_class);
+ sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
+ sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
+ 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");
--- a/include/linux/usb/Kbuild
+++ b/include/linux/usb/Kbuild
@@ -4,4 +4,4 @@ header-y += ch9.h
header-y += gadgetfs.h
header-y += midi.h
header-y += g_printer.h
-
+header-y += tmc.h
--- /dev/null
+++ b/include/linux/usb/tmc.h
@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2007 Stefan Kopp, Gechingen, Germany
+ * Copyright (C) 2008 Novell, Inc.
+ * Copyright (C) 2008 Greg Kroah-Hartman <[email protected]>
+ *
+ * This file holds USB constants defined by the USB Device Class
+ * Definition for Test and Measurement devices published by the USB-IF.
+ *
+ * It also has the ioctl definitions for the usbtmc kernel driver that
+ * userspace needs to know about.
+ */
+
+#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
+
+/* 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

2008-08-28 00:04:40

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

On Wed, Aug 27, 2008 at 03:16:18PM -0400, Alan Stern wrote:
> On Wed, 27 Aug 2008, Oliver Neukum wrote:
>
> > Am Mittwoch 27 August 2008 20:58:08 schrieb Alan Stern:
> > > > And this rule depends on sharing the USB major or not. This needs
> > > > a big fat mention in Documentation.
> > >
> > > You mean that the open/disconnect locking rule applies only to drivers
> > > that call usb_register_dev, i.e, drivers using the USB major. ?Right?
> >
> > Yes. In fact drivers not using the USB major but their own char devices
> > will need such a lock. This is tricky.
>
> IMO all char-device registration/deregistration routines should use a
> similar rwsem. Then device drivers wouldn't need to worry about it.
>
> > > I agree that it deserves to be mentioned in the documentation
> > > somewhere. ?Where would be a good place? ?None of the existing files in
> > > Documentation/usb seem appropriate.
> >
> > The USB major merits a file of its own.
>
> I don't feel competent to start writing such a file. Greg, what do you
> think? Maybe all that's needed to get going is to "borrow" some of the
> material in LDD3. :-)

Well, as the USB chapter in LDD3 "borrowed" heavily from the comments in
the usb code itself, that would only be fair :)

That chapter needs to be reworked. The authors and Oreilly and I are
currently talking about how to do this all in a framework that makes
sense (the current one of a book every few years that quickly gets out
of date doesn't make sense.)

thanks,

greg k-h

2008-08-28 10:09:28

by Oliver Neukum

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

Am Donnerstag 28 August 2008 01:47:20 schrieb Greg KH:
> On Wed, Aug 27, 2008 at 02:58:08PM -0400, Alan Stern wrote:
> > On Wed, 27 Aug 2008, Greg KH wrote:
> >
> > > On Wed, Aug 27, 2008 at 02:28:22PM -0400, Alan Stern wrote:
> > > > On Wed, 27 Aug 2008, Greg KH wrote:
> > > >
> > > > > Here's an updated version of the usbtmc driver, with all of the
> > > > > different issues that have been raised, hopefully addressed.
> > > >
> > > > This is an example of what I was discussing with Oliver. In all
> > > > likelihood you simply don't need usbtmc_mutex, and using it will cause
> > > > a lockdep violation.
> > > >
> > > > That's why so many of the other USB class drivers don't have an
> > > > analogous static mutex.
> > >
> > > Ok, then it's just safe to drop this static mutex entirely, right?
> >
> > Yes, once you add the call to usb_deregister_dev.
>
> Great, all done now.
>
> Here's the updated version.

OK, here we go again.

> +static ssize_t usbtmc_read(struct file *filp, char __user *buf,
> + size_t count, loff_t *f_pos)
> +{

[..]

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

We have excellent endianness conversion macros.

> + buffer[8] = data->TermCharEnabled * 2;
> + /* Use term character? */

smp_rmb(); /* we must make sure we don't read a stale terminator */

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

even if usb_bulk_msg() failed?

> +
> + /* 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(data);
> + goto exit;
> + }
> +
> + /* 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(data);
> + goto exit;
> + }
> +
> + /* How many characters did the instrument send? */
> + n_characters = buffer[4] +
> + (buffer[5] << 8) +
> + (buffer[6] << 16) +
> + (buffer[7] << 24);

endianness macro

> +
> + /* Copy buffer to user space */
> + if (copy_to_user(buf + done, &buffer[12], n_characters)) {
> + /* There must have been an addressing problem */
> + retval = -EFAULT;
> + goto exit;
> + }
> +
> + done += n_characters;
> + if (n_characters < USBTMC_SIZE_IOBUFFER)
> + remaining = 0;
> + }
> +
> + /* Update file position value */
> + *f_pos = *f_pos + done;

That'll get out of sync if -EFAULT is returned

> + retval = done;
> +
> +exit:
> + mutex_unlock(&data->io_mutex);
> + kfree(buffer);
> + return retval;
> +}
> +
> +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 = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL);
> + if (!buffer)
> + return -ENOMEM;
> +
> + mutex_lock(&data->io_mutex);

This and usbtmc_read() need a test for disconnection. Open() and disconnect
are guarded in usbcore, read & write are not. By reference count you've made
sure you have a valid device descriptor, but the device may have been reprobed.

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

endianness macro

> + /* 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)) {
> + retval = -EFAULT;
> + goto exit;
> + }
> +
> + 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;

error case?

[..]
> +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;

memory leak

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

memory leak

[..]
> +static ssize_t store_TermChar(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);
> +
> + if (count < 1)
> + return -EINVAL;
> + data->TermChar = buf[0];

smp_wmb(); /* must hit RAM before enablement */

> + return count;
> +}
> +static DEVICE_ATTR(TermChar, S_IRUGO, show_TermChar, store_TermChar);
> +
> +#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; \

smp_rmb(); /* must hit RAM after the terminator */ \

There are subtle penalties to avoiding ioctl(). Among them is the loss
of atomicity.
[..]
> +static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + struct usbtmc_device_data *data;
> + int retval = -EBADRQC;
> +
> + data = file->private_data;
> + mutex_lock(&data->io_mutex);

check for disconnect as in read & write

> +
> + switch (cmd) {
> + case USBTMC_IOCTL_CLEAR_OUT_HALT:
> + retval = usbtmc_ioctl_clear_out_halt(data);
> +
> + case USBTMC_IOCTL_CLEAR_IN_HALT:
> + retval = usbtmc_ioctl_clear_in_halt(data);
> +
> + case USBTMC_IOCTL_INDICATOR_PULSE:
> + retval = usbtmc_ioctl_indicator_pulse(data);
> +
> + case USBTMC_IOCTL_CLEAR:
> + retval = usbtmc_ioctl_clear(data);
> +
> + case USBTMC_IOCTL_ABORT_BULK_OUT:
> + retval = usbtmc_ioctl_abort_bulk_out(data);
> +
> + case USBTMC_IOCTL_ABORT_BULK_IN:
> + retval = usbtmc_ioctl_abort_bulk_in(data);
> + }

This is missing a whole lot of "break;"

> +static int usbtmc_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
> + struct usbtmc_device_data *data;
> + struct usb_host_interface *iface_desc;
> + struct usb_endpoint_descriptor *endpoint;
> + int n;
> + int retcode;
> +
> + dev_dbg(&intf->dev, "%s called\n", __func__);
> +
> + data = kmalloc(sizeof(struct usbtmc_device_data), GFP_KERNEL);
> + if (!data) {
> + dev_err(&intf->dev, "Unable to allocate kernel memory\n");
> + 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);
> + mutex_init(&data->io_mutex);
> +
> + /* Initialize USBTMC bTag and other fields */
> + data->bTag = 1;
> + data->TermCharEnabled = 0;
> + data->TermChar = '\n';
> +
> + /* 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(&intf->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(&intf->dev, "Found Bulk out endpoint at %u\n",
> + data->bulk_out);
> + break;
> + }
> + }
> +
> + 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);

If you ignore an error return, be open about it.

> + 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);
> + goto error_register;
> + }
> + dev_dbg(&intf->dev, "Using minor number %d\n", intf->minor);
> +
> + return 0;
> +
> +error_register:
> + sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
> + sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);

The groups might never have been created.

> + kref_put(&data->kref, usbtmc_delete);
> + return retcode;
> +}
> +
> +static void usbtmc_disconnect(struct usb_interface *intf)
> +{
> + struct usbtmc_device_data *data;
> +
> + dev_dbg(&intf->dev, "usbtmc_disconnect called\n");
> +

You must set a flag for read, write and ioctl.

Regards
Oliver

2008-08-28 16:28:22

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

On Thu, Aug 28, 2008 at 12:10:26PM +0200, Oliver Neukum wrote:
> Am Donnerstag 28 August 2008 01:47:20 schrieb Greg KH:
> > On Wed, Aug 27, 2008 at 02:58:08PM -0400, Alan Stern wrote:
> > > On Wed, 27 Aug 2008, Greg KH wrote:
> > >
> > > > On Wed, Aug 27, 2008 at 02:28:22PM -0400, Alan Stern wrote:
> > > > > On Wed, 27 Aug 2008, Greg KH wrote:
> > > > >
> > > > > > Here's an updated version of the usbtmc driver, with all of the
> > > > > > different issues that have been raised, hopefully addressed.
> > > > >
> > > > > This is an example of what I was discussing with Oliver. In all
> > > > > likelihood you simply don't need usbtmc_mutex, and using it will cause
> > > > > a lockdep violation.
> > > > >
> > > > > That's why so many of the other USB class drivers don't have an
> > > > > analogous static mutex.
> > > >
> > > > Ok, then it's just safe to drop this static mutex entirely, right?
> > >
> > > Yes, once you add the call to usb_deregister_dev.
> >
> > Great, all done now.
> >
> > Here's the updated version.
>
> OK, here we go again.
>
> > +static ssize_t usbtmc_read(struct file *filp, char __user *buf,
> > + size_t count, loff_t *f_pos)
> > +{
>
> [..]
>
> > + /* 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;
>
> We have excellent endianness conversion macros.

For splitting values up into the individual byte portions? I think this
is far more obvious as to exactly what is going on, don't you?

> > + buffer[8] = data->TermCharEnabled * 2;
> > + /* Use term character? */
>
> smp_rmb(); /* we must make sure we don't read a stale terminator */

I'm not going to worry about races here, that's not a real issue.

> > + 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;
>
> even if usb_bulk_msg() failed?

Good point, will fix.

> > + /* How many characters did the instrument send? */
> > + n_characters = buffer[4] +
> > + (buffer[5] << 8) +
> > + (buffer[6] << 16) +
> > + (buffer[7] << 24);
>
> endianness macro

I'll leave it (casting is just a big of a mess, this obviously shows
what is going on here.)

> > + /* Copy buffer to user space */
> > + if (copy_to_user(buf + done, &buffer[12], n_characters)) {
> > + /* There must have been an addressing problem */
> > + retval = -EFAULT;
> > + goto exit;
> > + }
> > +
> > + done += n_characters;
> > + if (n_characters < USBTMC_SIZE_IOBUFFER)
> > + remaining = 0;
> > + }
> > +
> > + /* Update file position value */
> > + *f_pos = *f_pos + done;
>
> That'll get out of sync if -EFAULT is returned

Hm, I wonder if this is an issue, I'll poke at it.

> > +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 = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL);
> > + if (!buffer)
> > + return -ENOMEM;
> > +
> > + mutex_lock(&data->io_mutex);
>
> This and usbtmc_read() need a test for disconnection. Open() and disconnect
> are guarded in usbcore, read & write are not. By reference count you've made
> sure you have a valid device descriptor, but the device may have been reprobed.

If so, then struct usb_device would be different, right? Oh, I see,
disconnect() using usbfs/sysfs. Bah, is it really something that
happens in the real world? Oh well, I'll go fix this...

> > + 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;
>
> error case?

Will do.

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

Good find, will fix.

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

will fix.

> [..]
> > +static ssize_t store_TermChar(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);
> > +
> > + if (count < 1)
> > + return -EINVAL;
> > + data->TermChar = buf[0];
>
> smp_wmb(); /* must hit RAM before enablement */

Not going to be an issue, it's ok to leave as-is.

> > +{ \
> > + 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; \
>
> smp_rmb(); /* must hit RAM after the terminator */ \

Same as above.

> There are subtle penalties to avoiding ioctl(). Among them is the loss
> of atomicity.

Sure, but again, this isn't a real issue here.

> [..]
> > +static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > +{
> > + struct usbtmc_device_data *data;
> > + int retval = -EBADRQC;
> > +
> > + data = file->private_data;
> > + mutex_lock(&data->io_mutex);
>
> check for disconnect as in read & write
>
> > +
> > + switch (cmd) {
> > + case USBTMC_IOCTL_CLEAR_OUT_HALT:
> > + retval = usbtmc_ioctl_clear_out_halt(data);
> > +
> > + case USBTMC_IOCTL_CLEAR_IN_HALT:
> > + retval = usbtmc_ioctl_clear_in_halt(data);
> > +
> > + case USBTMC_IOCTL_INDICATOR_PULSE:
> > + retval = usbtmc_ioctl_indicator_pulse(data);
> > +
> > + case USBTMC_IOCTL_CLEAR:
> > + retval = usbtmc_ioctl_clear(data);
> > +
> > + case USBTMC_IOCTL_ABORT_BULK_OUT:
> > + retval = usbtmc_ioctl_abort_bulk_out(data);
> > +
> > + case USBTMC_IOCTL_ABORT_BULK_IN:
> > + retval = usbtmc_ioctl_abort_bulk_in(data);
> > + }
>
> This is missing a whole lot of "break;"

Bah, good catch, my fault.

> > + 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);
>
> If you ignore an error return, be open about it.

I'm not? Should I print an error and then just continue on? Would that
be sufficient?

> > +error_register:
> > + sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
> > + sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
>
> The groups might never have been created.

That's ok, the sysfs core should handle this safely.

> > +static void usbtmc_disconnect(struct usb_interface *intf)
> > +{
> > + struct usbtmc_device_data *data;
> > +
> > + dev_dbg(&intf->dev, "usbtmc_disconnect called\n");
> > +
>
> You must set a flag for read, write and ioctl.

Will do. Then I need to lock the flag with a mutex, right?

thanks,

greg k-h

2008-08-28 16:59:19

by Marcel Janssen

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

On Thursday 28 August 2008 01:47:20 Greg KH wrote:
> Great, all done now.
>
> Here's the updated version.

I've just installed this version.

Here's what I see so far :

The driver inserts well and when I connect my device it shows /dev/usbtmc0
with major 180 and minor 176.
It only creates one device (Stefan's driver created two) but I'm not sure if
that has changed for a reason so just let you know.

When I disconnect my device, usbtmc0 will not be destroyed. After connecting
the device a couple of times I have a lot of /dev/usbtmc.. files.

I would expect the following to work :
echo :*IDN?>/dev/usbtmc0
But it returns : No such device

Using echo and cat to test the device is quite convenient, but is this
supposed to work yet ?

I checked /sys a bit and found that the endpoints are correctly found.

I also had an issue applying the patch (documentation and makefile). Not sure
if that's related to my system though, I'll check that later.

regards,
Marcel

2008-08-28 21:28:33

by Oliver Neukum

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2


> > > + 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;
> >
> > We have excellent endianness conversion macros.
>
> For splitting values up into the individual byte portions? I think this
> is far more obvious as to exactly what is going on, don't you?

No. Kernel code does not exist to show how to do endianness conversion.
We have clearly labeled functions.

> > > + buffer[8] = data->TermCharEnabled * 2;
> > > + /* Use term character? */
> >
> > smp_rmb(); /* we must make sure we don't read a stale terminator */
>
> I'm not going to worry about races here, that's not a real issue.

There is no such thing as an ignorable race. On second thought
you can take the mutex in the sysfs handlers. That will also do the job.


> > This and usbtmc_read() need a test for disconnection. Open() and disconnect
> > are guarded in usbcore, read & write are not. By reference count you've made
> > sure you have a valid device descriptor, but the device may have been reprobed.
>
> If so, then struct usb_device would be different, right? Oh, I see,
> disconnect() using usbfs/sysfs. Bah, is it really something that
> happens in the real world? Oh well, I'll go fix this...

This is oopsable from user space.

> > If you ignore an error return, be open about it.
>
> I'm not? Should I print an error and then just continue on? Would that
> be sufficient?

Yes.

> > > +static void usbtmc_disconnect(struct usb_interface *intf)
> > > +{
> > > + struct usbtmc_device_data *data;
> > > +
> > > + dev_dbg(&intf->dev, "usbtmc_disconnect called\n");
> > > +
> >
> > You must set a flag for read, write and ioctl.
>
> Will do. Then I need to lock the flag with a mutex, right?

Yes. You can set the intf pointer to NULL. That's sort of idiomatic.
And you should NULL intfdata.


Regards
Oliver

2008-08-28 22:07:49

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

On Thu, Aug 28, 2008 at 06:58:41PM +0200, Marcel Janssen wrote:
> On Thursday 28 August 2008 01:47:20 Greg KH wrote:
> > Great, all done now.
> >
> > Here's the updated version.
>
> I've just installed this version.
>
> Here's what I see so far :
>
> The driver inserts well and when I connect my device it shows /dev/usbtmc0
> with major 180 and minor 176.
> It only creates one device (Stefan's driver created two) but I'm not sure if
> that has changed for a reason so just let you know.

Yes, the old driver had a "control and debug " channel at minor 0, which
isn't needed anymore.

> When I disconnect my device, usbtmc0 will not be destroyed. After connecting
> the device a couple of times I have a lot of /dev/usbtmc.. files.

Hm, that's not good, something's wrong here.

Can you enable CONFIG_USB_DEBUG and rebuild the driver and send me the
kernel log messages when you plug the device in and then remove it?

> I would expect the following to work :
> echo :*IDN?>/dev/usbtmc0
> But it returns : No such device

Yeah, I would expect that to work as well. The debug information might
be helpful here also.

> Using echo and cat to test the device is quite convenient, but is this
> supposed to work yet ?

Yes it should.

> I checked /sys a bit and found that the endpoints are correctly found.

Good, how about the capability information? That should be in sysfs as
well, does that show the proper values?

thanks,

greg k-h

2008-08-29 07:05:21

by stefan_kopp

[permalink] [raw]
Subject: RE: [PATCH] USB: add USB test and measurement class driver - round 2

Hi all,

I haven't been able to follow all the enhancements you did to the driver, but I can comment on the ratioale behind the orignal driver.

The original driver used minor number 0 for "driver communication" (in contrast to "device communication"). The main purpose was to get a list of the USBTMC devices attached by doing a "cat /dev/usbtmc0" (which would point to minor number 0). The first device found would then use the next free minor number, usually 1. I believe the concept of "driver communication" has been removed from the driver, so a single minor number for a single device should be it.

The issue with using cat on the shell level is that it uses fread which has the (in this case) ugly behaviour of recalling the driver's read method until the full number of characters requested has been accumulated (or until zero characters are returned, indicating the end of file). With USBTMC instruments, this behavour is bad because the retry will not just return zero characters, it will cause a timeout with the associated error condition in the device. So, to enable the use of echo/cat, I added some fread handling to the driver (which catches the retries). I believe this also has been removed, so I assume cat/fread will not work (?).

BTW, removing these "features" is not a problem IMHO -- actually I have been working on a "scaled down" version of the driver myself. The reason is that I have been adding a user space library which hides the details of the USB driver and adds portability/exchangability between USB/LAN/serial devices. I expect most users to use this level and it is unlikely they will still want to use echo/cat.

Best regards,
Stefan

-----Original Message-----
From: Marcel Janssen [mailto:[email protected]]
Sent: Donnerstag, 28. August 2008 18:59
To: Greg KH
Cc: Alan Stern; Oliver Neukum; USB list; KOPP,STEFAN (A-Germany,ex1); Felipe Balbi; Kernel development list
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

On Thursday 28 August 2008 01:47:20 Greg KH wrote:
> Great, all done now.
>
> Here's the updated version.

I've just installed this version.

Here's what I see so far :

The driver inserts well and when I connect my device it shows /dev/usbtmc0 with major 180 and minor 176.
It only creates one device (Stefan's driver created two) but I'm not sure if that has changed for a reason so just let you know.

When I disconnect my device, usbtmc0 will not be destroyed. After connecting the device a couple of times I have a lot of /dev/usbtmc.. files.

I would expect the following to work :
echo :*IDN?>/dev/usbtmc0
But it returns : No such device

Using echo and cat to test the device is quite convenient, but is this supposed to work yet ?

I checked /sys a bit and found that the endpoints are correctly found.

I also had an issue applying the patch (documentation and makefile). Not sure if that's related to my system though, I'll check that later.

regards,
Marcel



2008-08-29 07:45:34

by Oliver Neukum

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

Am Freitag 29 August 2008 08:57:54 schrieb [email protected]:
> The issue with using cat on the shell level is that it uses fread which has the (in this case) ugly behaviour of recalling the driver's read method until the full number of characters requested has been accumulated (or until zero characters are returned, indicating the end of file). With USBTMC instruments, this behavour is bad because the retry

This may be problematic. The driver is throwing away EOF in other words.
Generally this is not a good idea. User space can no longer tell how long the
reply was.

Regards
Oliver

2008-08-29 08:25:27

by stefan_kopp

[permalink] [raw]
Subject: RE: [PATCH] USB: add USB test and measurement class driver - round 2

Hi Oliver,

USBTMC has a different concept, the "termination character". If this is enabled and set to the appropriate character (usually /n), the read operation will be terminated by the device automatically. The last character in the response will be the termination character. Note, however, this capability is optional (not every vendor supports it) and even if it is supported, is might be deactivated (for good reason, e.g. for transfer of binary data).

If you are not using the termination character, the device will typically send its full output buffer contents. Usually, this works just fine because communication with the device is transactional, one query followed by a response, then another query, followed by another response, and so on.

The driver does return the number of characters received in both cases (with or without use of the term character feature).

The issue is more the habit of fread of "insisting" on receiving the full number of characters requested in the call to fread. Usually, you don't know how many characters will be returned by a USBTMC device, so you just ask for plenty and see how many you get. This works just fine with read(2). fread, unfortunately, retries the read by calling the driver's read entry point again. If you don't catch this retry and ask the USBTMC device for data, you will receive a timeout (unless there happens to be additional data in the device's output buffer).

In the original driver, I "simulated" an EOF by just returning 0 in the retry call to the read method. Not sure there's a better way of handling this issue because you can't rely on the termination character... However, I'm very open to ideas... As I said earlier, I avoided this whole issue by using read in my next layer.

Best regards
Stefan


-----Original Message-----
From: Oliver Neukum [mailto:[email protected]]
Sent: Freitag, 29. August 2008 09:47
To: KOPP,STEFAN (A-Germany,ex1)
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

Am Freitag 29 August 2008 08:57:54 schrieb [email protected]:
> The issue with using cat on the shell level is that it uses fread
> which has the (in this case) ugly behaviour of recalling the driver's
> read method until the full number of characters requested has been
> accumulated (or until zero characters are returned, indicating the end
> of file). With USBTMC instruments, this behavour is bad because the
> retry

This may be problematic. The driver is throwing away EOF in other words.
Generally this is not a good idea. User space can no longer tell how long the reply was.

Regards
Oliver


2008-08-29 08:33:19

by Oliver Neukum

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

Am Freitag 29 August 2008 10:14:46 schrieb [email protected]:
> USBTMC has a different concept, the "termination character". If this is enabled and set to the appropriate character (usually /n), the read operation will be terminated by the device automatically. The last character in the response will be the termination character. Note, however, this capability is optional (not every vendor supports it) and even if it is supported, is might be deactivated (for good reason, e.g. for transfer of binary data).
>
> If you are not using the termination character, the device will typically send its full output buffer contents. Usually, this works just fine because communication with the device is transactional, one query followed by a response, then another query, followed by another response, and so on.
>
> The driver does return the number of characters received in both cases (with or without use of the term character feature).

But how do you learn in user space how many characters the device actually
did send? Using read() you'll only learn that the answer was shorter than
you asked for. If you get the amount you expected the answer may have
been as long as you expected or longer. How do you find out whether the answer
was longer?

Regards
Oliver

2008-08-29 09:13:21

by stefan_kopp

[permalink] [raw]
Subject: RE: [PATCH] USB: add USB test and measurement class driver - round 2

I may be misinterpreting your message, but read(2) does return the number of bytes read, so if you ask for plenty of data, you can be sure you will get less than that. A device's response should only appear in the output buffer when it is complete. I have never seen a partial response being made available by an instrument.

USBTMC instruments are mostly controlled by short commands such as "CONF:FREQ 1.3E3\n", and they usually return short strings such as a measurement result or setting, again as a human-readable string (according to a standard called "SCPI"). It is common practice to ask for, let's say, 1000 bytes, what you really expect is maybe 10...30 bytes. And you can activate the term char handling if the device supports it.

For large amounts of data (e.g. when downloading a waveform), USBTMC instruments often use a "binblock", a concept taken from the good old GPIB (IEEE488) standard, where the data is preceeded by a header and the header indicates how many bytes are following. So you first read and interpret the header and then you know exactly how many bytes to expect in the body of the message.

Best regards,
Stefan

-----Original Message-----
From: Oliver Neukum [mailto:[email protected]]
Sent: Freitag, 29. August 2008 10:34
To: KOPP,STEFAN (A-Germany,ex1)
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

Am Freitag 29 August 2008 10:14:46 schrieb [email protected]:
> USBTMC has a different concept, the "termination character". If this is enabled and set to the appropriate character (usually /n), the read operation will be terminated by the device automatically. The last character in the response will be the termination character. Note, however, this capability is optional (not every vendor supports it) and even if it is supported, is might be deactivated (for good reason, e.g. for transfer of binary data).
>
> If you are not using the termination character, the device will typically send its full output buffer contents. Usually, this works just fine because communication with the device is transactional, one query followed by a response, then another query, followed by another response, and so on.
>
> The driver does return the number of characters received in both cases (with or without use of the term character feature).

But how do you learn in user space how many characters the device actually did send? Using read() you'll only learn that the answer was shorter than you asked for. If you get the amount you expected the answer may have been as long as you expected or longer. How do you find out whether the answer was longer?

Regards
Oliver

2008-08-29 11:31:57

by Oliver Neukum

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

Am Freitag 29 August 2008 11:13:04 schrieb [email protected]:
> For large amounts of data (e.g. when downloading a waveform), USBTMC instruments often use a "binblock", a concept taken from the good old GPIB (IEEE488) standard, where the data is preceeded by a header and the header indicates how many bytes are following. So you first read and interpret the header and then you know exactly how many bytes to expect in the body of the message.

Not really ideal. We should tell user space how much really was sent.

Regards
Oliver

2008-08-29 14:39:43

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

On Fri, Aug 29, 2008 at 12:57:54AM -0600, [email protected] wrote:
> Hi all,
>
> I haven't been able to follow all the enhancements you did to the
> driver, but I can comment on the ratioale behind the orignal driver.
>
> The original driver used minor number 0 for "driver communication" (in
> contrast to "device communication"). The main purpose was to get a
> list of the USBTMC devices attached by doing a "cat /dev/usbtmc0"
> (which would point to minor number 0). The first device found would
> then use the next free minor number, usually 1. I believe the concept
> of "driver communication" has been removed from the driver, so a
> single minor number for a single device should be it.

cating that file was sending out a text file to userspace that needed to
be parsed, like a /proc file. Now you can just look at all of the
devices in /sys/class/usb/usbtmc* to see all of the different devices in
the system.

> The issue with using cat on the shell level is that it uses fread
> which has the (in this case) ugly behaviour of recalling the driver's
> read method until the full number of characters requested has been
> accumulated (or until zero characters are returned, indicating the end
> of file). With USBTMC instruments, this behavour is bad because the
> retry will not just return zero characters, it will cause a timeout
> with the associated error condition in the device. So, to enable the
> use of echo/cat, I added some fread handling to the driver (which
> catches the retries). I believe this also has been removed, so I
> assume cat/fread will not work (?).

I do not know, but we do not do wierd things in the kernel just because
of broken userspace programs. This logic should be done in userspace,
and programs should look at the return value of read() and handle it
properly. Otherwise it is a bug.

thanks,

greg k-h

2008-08-29 16:41:46

by Marcel Janssen

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

On Friday 29 August 2008 16:39:02 Greg KH wrote:
> > The issue with using cat on the shell level is that it uses fread
> > which has the (in this case) ugly behaviour of recalling the driver's
> > read method until the full number of characters requested has been
> > accumulated (or until zero characters are returned, indicating the end
> > of file). With USBTMC instruments, this behavour is bad because the
> > retry will not just return zero characters, it will cause a timeout
> > with the associated error condition in the device. So, to enable the
> > use of echo/cat, I added some fread handling to the driver (which
> > catches the retries). I believe this also has been removed, so I
> > assume cat/fread will not work (?).
>
> I do not know, but we do not do wierd things in the kernel just because
> of broken userspace programs. This logic should be done in userspace,
> and programs should look at the return value of read() and handle it
> properly. Otherwise it is a bug.

I don't think this is broken in user space. The problem is that when you issue
a measurement command it is not known how many bytes it will return. This is
probably due to ASCII output being very common in T&M devices instead of raw
data (int, float etc). The ASCII formatting is done in the device and this
returns just a string which may or may not be terminated by the term char.
This is of course not true for all T&M devices, but the majority works this
way.

I admit that the above produces a lot of overhead, but it's just a fact that
T&M devices work this way, including ours for most of their data processing
(not all though).

I think the USBTMC spec is quite clear on how it should be implemented on
messaging level. Basically when you issue the command "*IDN?" the device
will process this and return the device ID string. The length of this string
is set in the TransferSize of the 12 byte header that the device returns. The
problem when you issue a read command is that the read command does not yet
know how much data to expect. It should issue the REQUEST_DEV_DEP_MSG_IN
first and set the TransferSize value high enough.
In the USBTMC_488 extension you find an example (chapter 3.3.1 page 7) that
shows the REQUEST_DEV_DEP_MSG_IN TransferSize being set to 64 although the
actual data being returned from the device is less (only 36 bytes).

What you do see in practice is that when someone would issue a read command
and asking for less bytes than are available is that the user program may
handle this as a warning telling the user that he did not request all
available data.

Stefan's driver works exactly the way I would expect from a users point of
view. Whether the implementation can be improved is another issue, but the
behaviour is correct and compliant with other usbtmc drivers on other
platforms.

Regards,
Marcel


2008-08-30 03:50:38

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2

On Fri, Aug 29, 2008 at 06:41:15PM +0200, Marcel Janssen wrote:
> On Friday 29 August 2008 16:39:02 Greg KH wrote:
> > > The issue with using cat on the shell level is that it uses fread
> > > which has the (in this case) ugly behaviour of recalling the driver's
> > > read method until the full number of characters requested has been
> > > accumulated (or until zero characters are returned, indicating the end
> > > of file). With USBTMC instruments, this behavour is bad because the
> > > retry will not just return zero characters, it will cause a timeout
> > > with the associated error condition in the device. So, to enable the
> > > use of echo/cat, I added some fread handling to the driver (which
> > > catches the retries). I believe this also has been removed, so I
> > > assume cat/fread will not work (?).
> >
> > I do not know, but we do not do wierd things in the kernel just because
> > of broken userspace programs. This logic should be done in userspace,
> > and programs should look at the return value of read() and handle it
> > properly. Otherwise it is a bug.
>
> I don't think this is broken in user space. The problem is that when you issue
> a measurement command it is not known how many bytes it will return. This is
> probably due to ASCII output being very common in T&M devices instead of raw
> data (int, float etc). The ASCII formatting is done in the device and this
> returns just a string which may or may not be terminated by the term char.
> This is of course not true for all T&M devices, but the majority works this
> way.
>
> I admit that the above produces a lot of overhead, but it's just a fact that
> T&M devices work this way, including ours for most of their data processing
> (not all though).

How is this overhead in userspace, just do something like the following:
char big_buffer[16000]; /* bigger than any possible request */
size = read(file_desc, &big_buffer[0], sizeof(big_buffer));

and size is the amount of data you actually read from the device, i.e.
one request.

> I think the USBTMC spec is quite clear on how it should be implemented on
> messaging level. Basically when you issue the command "*IDN?" the device
> will process this and return the device ID string. The length of this string
> is set in the TransferSize of the 12 byte header that the device returns. The
> problem when you issue a read command is that the read command does not yet
> know how much data to expect. It should issue the REQUEST_DEV_DEP_MSG_IN
> first and set the TransferSize value high enough.
> In the USBTMC_488 extension you find an example (chapter 3.3.1 page 7) that
> shows the REQUEST_DEV_DEP_MSG_IN TransferSize being set to 64 although the
> actual data being returned from the device is less (only 36 bytes).
>
> What you do see in practice is that when someone would issue a read command
> and asking for less bytes than are available is that the user program may
> handle this as a warning telling the user that he did not request all
> available data.

Then that's a userspace bug, don't do that in your program that reads
from these types of devices :)

> Stefan's driver works exactly the way I would expect from a users point of
> view. Whether the implementation can be improved is another issue, but the
> behaviour is correct and compliant with other usbtmc drivers on other
> platforms.

But it's not compliant with the "standard" way of using a file
descriptor in unix, and might break some POSIX requirements as well (I'm
not a good POSIX follower, so I don't know for sure...)

As this is trivial to handle in userspace, and requires no additional
wierd code in the kernel driver, I don't see this as something we should
change the driver for.

thanks,

greg k-h