USB: add USB test and measurement class driver
PATCH 1/1 USB: add USB test and measurement class driver
I took the time today to fix up the usbtmc driver that was in the -staging tree and here's the results. I only have 3 todo items left:
- get assigned minor number
- reserve ioctl range
- add autosuspend support
The minor number is easy, I can give that :)
The ioctl range is also simple, I really can't see getting rid of the remaining ioctls as they are useful and unless we want to just us sysfs files for these items, I'll leave them as is. I moved the majority of the ioctls in the old driver to be sysfs files, as they are simple values.
The autosuspend support isn't that hard either, but I would like to find someone with one of these devices to help me in testing that before I add it, so I imagine the driver can go into the tree without that being added at this time.
Many thanks to Felipe Balbi <[email protected]> for lots of work in cleaning up this driver.
I've now added this to my tree, and any review comments are greatly welcome.
--- drivers/usb/class/Kconfig | 10 drivers/usb/class/Makefile | 1 drivers/usb/class/usbtmc.c | 1058 +++++++++++++++++++++++++++++++++++++++++++++ drivers/usb/class/usbtmc.h | 24 + include/linux/usb/tmc.h | 27 + 5 files changed, 1120 insertions(+)
drivers/usb/class/Kconfig +10 -0
@@ -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.
drivers/usb/class/Makefile +1 -0
@@ -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
drivers/usb/class/usbtmc.c new file +1058 -0
1,058 lines not on this page · raw message
drivers/usb/class/usbtmc.h new file +24 -0
@@ -0,0 +1,24 @@ +/* +* usbtmc.h +* This file is part of a Linux kernel module for USBTMC (USB Test and +* Measurement Class) devices +* +* Copyright (C) 2007 Stefan Kopp, Gechingen, Germany +* +* See usbtmc.c source file for license details +*/ +#ifndef __USB_TMC_H +#define __USB_TMC_H + +#include <linux/ioctl.h> + +/* Request values for USBTMC driver's ioctl entry point */ +#define USBTMC_IOC_NR 91 +#define USBTMC_IOCTL_INDICATOR_PULSE _IO(USBTMC_IOC_NR, 1) +#define USBTMC_IOCTL_CLEAR _IO(USBTMC_IOC_NR, 2) +#define USBTMC_IOCTL_ABORT_BULK_OUT _IO(USBTMC_IOC_NR, 3) +#define USBTMC_IOCTL_ABORT_BULK_IN _IO(USBTMC_IOC_NR, 4) +#define USBTMC_IOCTL_CLEAR_OUT_HALT _IO(USBTMC_IOC_NR, 6) +#define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7) + +#endif /* __USB_TMC_H */
include/linux/usb/tmc.h new file +27 -0
@@ -0,0 +1,27 @@ +/* +* This file holds USB constants defined by the USB Device Class +* Definition for Test and Measurement devices published by the USB-IF. +*/ + +#ifndef __LINUX_USB_TMC_H +#define __LINUX_USB_TMC_H + +/* USB TMC status values */ +#define USBTMC_STATUS_SUCCESS 0x01 +#define USBTMC_STATUS_PENDING 0x02 +#define USBTMC_STATUS_FAILED 0x80 +#define USBTMC_STATUS_TRANSFER_NOT_IN_PROGRESS 0x81 +#define USBTMC_STATUS_SPLIT_NOT_IN_PROGRESS 0x82 +#define USBTMC_STATUS_SPLIT_IN_PROGRESS 0x83 + +/* USB TMC requests values */ +#define USBTMC_REQUEST_INITIATE_ABORT_BULK_OUT 1 +#define USBTMC_REQUEST_CHECK_ABORT_BULK_OUT_STATUS 2 +#define USBTMC_REQUEST_INITIATE_ABORT_BULK_IN 3 +#define USBTMC_REQUEST_CHECK_ABORT_BULK_IN_STATUS 4 +#define USBTMC_REQUEST_INITIATE_CLEAR 5 +#define USBTMC_REQUEST_CHECK_CLEAR_STATUS 6 +#define USBTMC_REQUEST_GET_CAPABILITIES 7 +#define USBTMC_REQUEST_INDICATOR_PULSE 64 + +#endif
Quoted 2 lines · Greg KH
I've now added this to my tree, and any review comments are greatly welcome.
Heck, I'll review my own patch, as I noticed I forgot some things:
Quoted 7 lines
--- drivers/usb/class/Kconfig | 10 drivers/usb/class/Makefile | 1 drivers/usb/class/usbtmc.c | 1058 +++++++++++++++++++++++++++++++++++++++++++++ drivers/usb/class/usbtmc.h | 24 + include/linux/usb/tmc.h | 27 + 5 files changed, 1120 insertions(+)
needs Documentation/ABI description of new sysfs files
drivers/usb/class/usbtmc.c line 66
Quoted 10 lines
drivers/usb/class/usbtmc.c new file +10
-0+/* +* This structure is the capabilities for the device +* See section 4.2.1.8 of the USBTMC specification for details. +*/ +struct usbtmc_dev_capabilities { +__u8 interface_capabilities; +__u8 device_capabilities; +__u8 usb488_interface_capabilities; +__u8 usb488_device_capabilities; +};
These should match the usbif description names to make it easier to understand.
Quoted 16 lines
drivers/usb/class/usbtmc.c new file +16
-0+/* USBTMC devices have only one setting, so use that */ + iface_desc = data->intf->cur_altsetting; + +/* Find bulk in endpoint */ +for (n =0 ; n < iface_desc->desc.bNumEndpoints; n++) { + endpoint = &iface_desc->endpoint[n].desc; + +if (usb_endpoint_is_bulk_in (endpoint)) { + data->bulk_in = endpoint->bEndpointAddress; +dev_dbg (dev,"Found bulk in endpoint at %u\n" , + data->bulk_in); +break ; + } + } + +/* Find bulk out endpoint */
drivers/usb/class/usbtmc.c line 156
Quoted 10 lines
drivers/usb/class/usbtmc.c new file +10
-0+for (n =0 ; n < iface_desc->desc.bNumEndpoints; n++) { + endpoint = &iface_desc->endpoint[n].desc; + +if (usb_endpoint_is_bulk_out (endpoint)) { + data->bulk_out = endpoint->bEndpointAddress; +dev_dbg (dev,"Found Bulk out endpoint at %u\n" , + data->bulk_out); +break ; + } + }
This can all be done at probe() time, no need to wait until open().
Quoted 5 lines
drivers/usb/class/usbtmc.c new file +5
-0+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;
drivers/usb/class/usbtmc.c line 522
Quoted 10 lines
drivers/usb/class/usbtmc.c new file +10
-0+int retval; +int actual; +unsigned long int n_bytes; +int n; +int remaining; +int done; +int this_part; + + data = filp->private_data; + buffer = data->buffer;
This buffer should be local to the function, and not the device as we could overlap reads and writes. It should be removed from the device itself to make sure everything is correct. The ioctls already have their own local buffers, fixing a bug in the original driver which used 1! buffer for all devices and all commands...
thanks,
greg k-h
Quoted 14 lines · Greg KH
Quoted 2 lines · Greg KH
I've now added this to my tree, and any review comments are greatly welcome.
Heck, I'll review my own patch, as I noticed I forgot some things:
Quoted 7 lines
--- drivers/usb/class/Kconfig | 10 drivers/usb/class/Makefile | 1 drivers/usb/class/usbtmc.c | 1058 +++++++++++++++++++++++++++++++++++++++++++++ drivers/usb/class/usbtmc.h | 24 + include/linux/usb/tmc.h | 27 + 5 files changed, 1120 insertions(+)
Quoted 1 line
needs
Documentation/ABIdescription of new sysfs files
Now added.
Quoted 5 lines
Quoted 5 lines
drivers/usb/class/usbtmc.c new file +5
-0+/* +* This structure is the capabilities for the device +* See section 4.2.1.8 of the USBTMC specification for details. +*/ +struct usbtmc_dev_capabilities {
Quoted 8 lines
drivers/usb/class/usbtmc.c line 66
Quoted 5 lines
drivers/usb/class/usbtmc.c new file +5
-0+__u8 interface_capabilities; +__u8 device_capabilities; +__u8 usb488_interface_capabilities; +__u8 usb488_device_capabilities; +};These should match the usbif description names to make it easier to understand.
Nevermind, they match the spec description just fine, these will stay.
Quoted 20 lines
Quoted 20 lines
drivers/usb/class/usbtmc.c new file +20
-0+/* USBTMC devices have only one setting, so use that */ + iface_desc = data->intf->cur_altsetting; + +/* Find bulk in endpoint */ +for (n =0 ; n < iface_desc->desc.bNumEndpoints; n++) { + endpoint = &iface_desc->endpoint[n].desc; + +if (usb_endpoint_is_bulk_in (endpoint)) { + data->bulk_in = endpoint->bEndpointAddress; +dev_dbg (dev,"Found bulk in endpoint at %u\n" , + data->bulk_in); +break ; + } + } + +/* Find bulk out endpoint */ +for (n =0 ; n < iface_desc->desc.bNumEndpoints; n++) { + endpoint = &iface_desc->endpoint[n].desc; + +if (usb_endpoint_is_bulk_out (endpoint)) {
Quoted 8 lines
drivers/usb/class/usbtmc.c line 156
Quoted 6 lines
drivers/usb/class/usbtmc.c new file +6
-0+ data->bulk_out = endpoint->bEndpointAddress; +dev_dbg (dev,"Found Bulk out endpoint at %u\n" , + data->bulk_out); +break ; + } + }This can all be done at
probe()time, no need to wait untilopen().
Now moved.
Quoted 13 lines
Quoted 13 lines
drivers/usb/class/usbtmc.c new file +13
-0+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; +
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ data = filp->private_data; + buffer = data->buffer;This buffer should be local to the function, and not the device as we could overlap reads and writes. It should be removed from the device itself to make sure everything is correct. The ioctls already have their own local buffers, fixing a bug in the original driver which used 1! buffer for all devices and all commands...
Now fixed.
It's fun talking to yourself...
greg k-h
Hello Greg,
Quoted 4 lines · On Wednesday 27 August 2008 02:05:01 Greg KH wrote:
The autosuspend support isn't that hard either, but I would like to find someone with one of these devices to help me in testing that before I add it, so I imagine the driver can go into the tree without that being added at this time.
I think I can be of some help here. We have usbtmc devices that have this functionality included. The only difficulty I currently have is time. I'll be on a business trip for the next 3 weeks but have our units with me and may be able to test it.
Best regards,
Marcel Janssen
Quoted 8 lines · Marcel Janssen
Hello Greg,
Quoted 4 lines · On Wednesday 27 August 2008 02:05:01 Greg KH wrote:
The autosuspend support isn't that hard either, but I would like to find someone with one of these devices to help me in testing that before I add it, so I imagine the driver can go into the tree without that being added at this time.
Quoted 4 lines
I think I can be of some help here. We have usbtmc devices that have this functionality included. The only difficulty I currently have is time. I'll be on a business trip for the next 3 weeks but have our units with me and may be able to test it.
That's great. How about a basic "the driver works or not" type testing of what we have already?
After that, we can work on autosuspend support.
thanks,
greg k-h
Am Mittwoch 27 August 2008 02:05:01 schrieb Greg KH:
Quoted 14 lines
+ intf =
usb_find_interface(&usbtmc_driver, iminor(inode)); + if (!intf) {
+ printk(KERN_ERRKBUILD_MODNAME
+ ": can not find device for minor %d", iminor(inode)); + return-ENODEV;
+ }drivers/usb/class/usbtmc.c new file +8
-0+ + data = usb_get_intfdata(intf); + kref_get(&data->kref); + + /* Store pointer in file structure's private data field */ + filp->private_data = data; + + dev = &data->intf->dev;
drivers/usb/class/usbtmc.c line 168
Quoted 10 lines
drivers/usb/class/usbtmc.c new file +9
-0+static void usbtmc_delete (struct kref *kref) +{ +struct usbtmc_device_data *data =to_usbtmc_data (kref); + +usb_put_dev (data->usb_dev); +kfree (data->buffer); +kfree (data); +} +
This is a race condition.
CPU A CPU Bopen()usb_find_interface()
disconnect() kref_put() usbtmc_delete() kfree()
kref_get()
You can write to free memory. You must use a static mutex for mutual exclusion between open() and disconnect()
Regards
Oliver
Quoted 31 lines · Oliver Neukum
Am Mittwoch 27 August 2008 02:05:01 schrieb Greg KH:
Quoted 14 lines
+ intf =
usb_find_interface(&usbtmc_driver, iminor(inode)); + if (!intf) {
+ printk(KERN_ERRKBUILD_MODNAME
+ ": can not find device for minor %d", iminor(inode)); + return-ENODEV;
+ }drivers/usb/class/usbtmc.c new file +8
-0+ + data = usb_get_intfdata(intf); + kref_get(&data->kref); + + /* Store pointer in file structure's private data field */ + filp->private_data = data; + + dev = &data->intf->dev;drivers/usb/class/usbtmc.c line 168
Quoted 10 lines
drivers/usb/class/usbtmc.c new file +9
-0+static void usbtmc_delete (struct kref *kref) +{ +struct usbtmc_device_data *data =to_usbtmc_data (kref); + +usb_put_dev (data->usb_dev); +kfree (data->buffer); +kfree (data); +} +This is a race condition.
CPU A CPU B
open()usb_find_interface()
Quoted 8 lines
disconnect() kref_put() usbtmc_delete() kfree()
kref_get()You can write to free memory. You must use a static mutex for mutual exclusion between
open()anddisconnect()
Ah, thanks, for some reason I thought that this wasn't needed with the kref code, but you are correct.
Hm, in looking at the usb driver tree, there are lots of drivers in there with that problem :)
Now fixed up in this driver.
thanks,
greg k-h
Quoted 5 lines · Oliver Neukum
This is a race condition.
CPU A CPU B
open()usb_find_interface()
Quoted 8 lines
disconnect() kref_put() usbtmc_delete() kfree()
kref_get()You can write to free memory. You must use a static mutex for mutual exclusion between
open()anddisconnect()
Is that necessary? usbcore includes its own mutual exclusion now. Look in file.c at how minor_rwsem is used.
Alan Stern
Am Mittwoch 27 August 2008 18:06:23 schrieb Alan Stern:
Quoted 13 lines · Oliver Neukum
Quoted 12 lines
This is a race condition.
CPU A CPU B
open()usb_find_interface()disconnect() kref_put() usbtmc_delete() kfree()
kref_get()
Quoted 5 lines
Quoted 2 lines
You can write to free memory. You must use a static mutex for mutual exclusion between
open()anddisconnect()Is that necessary? usbcore includes its own mutual exclusion now. Look in
file.cat howminor_rwsemis used.
This is interesting, the driver simply doesn't unregister the device. So it would be needed if the code could be left as it is. As not unregistering the device is wrong, this will fix the bug.
Thanks
Oliver
Am Mittwoch 27 August 2008 02:05:01 schrieb Greg KH:> +static ssize_t usbtmc_read(struct file *filp, char __user *buf,> + size_t count, loff_t *f_pos)> +{> + struct usbtmc_device_data *data;> + struct device *dev;> + unsigned long int n_characters;> + u8 *buffer;> + int actual;> + int done;> + int remaining;> + int retval;> + int this_part;> +> + /* Get pointer to private data structure */> + data = filp->private_data;> + dev = &data->intf->dev;> + buffer = data->buffer;> +> + remaining = count;> + done = 0;> +> + while (remaining > 0) {> + if (remaining > USBTMC_SIZE_IOBUFFER - 12 - 3)> + this_part = USBTMC_SIZE_IOBUFFER - 12 - 3;> + else> + this_part = remaining;> +> + /* Setup IO buffer for DEV_DEP_MSG_IN message> + * Refer to class specs for details> + */> + buffer[0] = 2;> + buffer[1] = data->bTag;> + buffer[2] = ~(data->bTag);> + buffer[3] = 0; /* Reserved */> + buffer[4] = (this_part - 12 - 3) & 255;> + buffer[5] = ((this_part - 12 - 3) >> 8) & 255;> + buffer[6] = ((this_part - 12 - 3) >> 16) & 255;> + buffer[7] = ((this_part - 12 - 3) >> 24) & 255;> + buffer[8] = data->TermCharEnabled * 2;> + /* Use term character? */> + buffer[9] = data->TermChar;> + buffer[10] = 0; /* Reserved */> + buffer[11] = 0; /* Reserved */ This function will go bad if concurrent readers enter, yet it has no locking.
Regards Oliver
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ¥Šwÿº{.nÇ+‰·¥Š{±þG«�éÿŠ{ayº ʇڙë,j¢f£¢·hš�ï�êÿ‘êçz_è®(階ŠÝ¢j"�ú¶m§ÿÿ¾«þG«�éÿ¢¸?™¨èÚ&£ø§~�á¶iO•æ¬z·švØ^¶m§ÿÿà ÿ¶ìÿ¢¸?–I¥
Quoted 38 lines · Oliver Neukum
Am Mittwoch 27 August 2008 02:05:01 schrieb Greg KH:
Quoted 37 lines
drivers/usb/class/usbtmc.c new file +37
-0+static ssize_t usbtmc_read (struct file *filp,char __user *buf, + size_t count, loff_t *f_pos) +{ + struct usbtmc_device_data *data; + struct device *dev; + unsigned long int n_characters; + u8 *buffer; + int actual; + int done; + int remaining; + int retval; + int this_part; + + /* Get pointer to private data structure */ + data = filp->private_data; + dev = &data->intf->dev; + buffer = data->buffer; + + remaining = count; + done = 0; + + while (remaining > 0) { + if (remaining > USBTMC_SIZE_IOBUFFER - 12 - 3) + this_part = USBTMC_SIZE_IOBUFFER - 12 - 3; + else + this_part = remaining; + + /* Setup IO buffer for DEV_DEP_MSG_IN message + * Refer to class specs for details + */ + buffer[0] = 2; + buffer[1] = data->bTag; + buffer[2] = ~(data->bTag); + buffer[3] = 0; /* Reserved */ + buffer[4] = (this_part - 12 - 3) & 255; + buffer[5] = ((this_part - 12 - 3) >> 8) & 255; + buffer[6] = ((this_part - 12 - 3) >> 16) & 255;
Quoted 8 lines
Quoted 6 lines
+ buffer[7] = ((
this_part- 12 - 3) >> 24) & 255; + buffer[8] =data->TermCharEnabled* 2; + /* Use term character? */
+ buffer[9] =data->TermChar;
+ buffer[10] = 0; /* Reserved */
+ buffer[11] = 0; /* Reserved */This function will go bad if concurrent readers enter, yet it has no locking.
Now fixed with an i/o mutex.
thanks,
greg k-h
Am Mittwoch 27 August 2008 02:05:01 schrieb Greg KH:
Quoted 2 lines
I've now added this to my tree, and any review comments are greatly welcome.
Unfortunately the issues are not trivial.
1. USBTMC_SIZE_IOBUFFER
use 2048. 4096 means a multipage allocation
2. usbtmc_open race with disconnect
already mailed about
3. read/write and, if you convert to unlocked_ioctl, need protection against reentry
4. read, write & ioctl need protection against using a rebound interface you add a mutex, that you need for (3) anyway to the descriptor and protected by this mutex you set the interface and device pointers to null in disconnect and check them in the affected methods
5.get_capabilities
kmalloced buffer is never freed
6. disconnect fails to remove sysfs attributes
7. probe will report success even if usb_register_dev fails
Regards
Oliver
Quoted 6 lines · Oliver Neukum
Am Mittwoch 27 August 2008 02:05:01 schrieb Greg KH:
Quoted 2 lines
I've now added this to my tree, and any review comments are greatly welcome.
Unfortunately the issues are not trivial.
Nah, they are all easily fixable :)
Quoted 2 lines
1.
USBTMC_SIZE_IOBUFFER
use 2048. 4096 means a multipage allocation
So? What difference does this really matter at normal USB speeds?
thanks,
greg k-h
Am Mittwoch 27 August 2008 17:28:48 schrieb Greg KH:
Quoted 3 lines · Oliver Neukum
So? What difference does this really matter at normal USB speeds?
It doesn't matter in terms of speed for you, but it's nice to be nice to the rest of the system.
Regards
Oliver
Quoted 7 lines · Oliver Neukum
Am Mittwoch 27 August 2008 17:28:48 schrieb Greg KH:
Quoted 3 lines · Oliver Neukum
So? What difference does this really matter at normal USB speeds?
It doesn't matter in terms of speed for you, but it's nice to be nice to the rest of the system.
Sure, it's not a real big deal, if this driver doesn't use the kmalloc-4096 slab cache, someone else will :)
I'll go change it.
thanks,
greg k-h
Quoted 2 lines · Oliver Neukum
5.get_capabilities
kmalloced buffer is never freed
Now fixed.
Quoted 1 line
6. disconnect fails to remove sysfs attributes
Ick, good catch, now fixed.
Quoted 1 line
7. probe will report success even if
usb_register_devfails
now fixed.
thanks a lot for the review.
greg k-h
Ok, as I've responded to this multiple times, now summarizing so I make sure I got everything...
Quoted 9 lines · Oliver Neukum
Am Mittwoch 27 August 2008 02:05:01 schrieb Greg KH:
Quoted 2 lines
I've now added this to my tree, and any review comments are greatly welcome.
Unfortunately the issues are not trivial.
1.
USBTMC_SIZE_IOBUFFER
use 2048. 4096 means a multipage allocation
changed.
Quoted 2 lines
2.
usbtmc_openrace with disconnect
already mailed about
fixed.
Quoted 2 lines
3. read/write and, if you convert to
unlocked_ioctl, need protection against reentry
fixed and changed to unlocked_ioctl.
Quoted 4 lines
4. read, write & ioctl need protection against using a rebound interface you add a mutex, that you need for (3) anyway to the descriptor and protected by this mutex you set the interface and device pointers to null in disconnect and check them in the affected methods
Should now be fixed.
Quoted 2 lines
5.get_capabilities
kmalloced buffer is never freed
fixed.
Quoted 1 line
6. disconnect fails to remove sysfs attributes
fixed.
Quoted 1 line
7. probe will report success even if
usb_register_devfails
fixed.
I'll post a new version now.
thanks a lot for the review, I really appreciate it.
greg k-h