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.

drivers/usb/class/usbtmc.c new file +3 -0

+static int usbtmc_probe(struct usb_interface *intf,
+			const struct usb_device_id *id)
+{

drivers/usb/class/usbtmc.c new file +13 -0

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

Quoted 3 lines

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.

drivers/usb/class/usbtmc.c line 111

Quoted 7 lines

drivers/usb/class/usbtmc.c new file +7 -0

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

Quoted 2 lines · Greg KH

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.

drivers/usb/class/usbtmc.c line 111

Quoted 7 lines

drivers/usb/class/usbtmc.c new file +7 -0

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

Quoted 3 lines

drivers/usb/class/usbtmc.c new file +3 -0

+static int usbtmc_probe(struct usb_interface *intf,
+			const struct usb_device_id *id)
+{

...

drivers/usb/class/usbtmc.c line 1044

Quoted 8 lines

drivers/usb/class/usbtmc.c new file +8 -0

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

Quoted 18 lines

drivers/usb/class/usbtmc.c new file +18 -0

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

drivers/usb/class/usbtmc.c line 1067

Quoted 5 lines

drivers/usb/class/usbtmc.c new file +5 -0

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

Quoted 5 lines · Alan Stern

Quoted 2 lines · Greg KH

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

Quoted 6 lines

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?

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

Quoted 2 lines

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.

Am Mittwoch 27 August 2008 20:58:08 schrieb Alan Stern:

Quoted 5 lines

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 9 lines · Oliver Neukum

Am Mittwoch 27 August 2008 20:58:08 schrieb Alan Stern:

Quoted 5 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 3 lines

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.

Quoted 1 line

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. :-)

Quoted 10 lines · Greg KH

Quoted 9 lines · Alan Stern

Quoted 2 lines · Greg KH

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 15 lines · Greg KH

Quoted 14 lines · Alan Stern

Quoted 11 lines · Greg KH

Quoted 3 lines

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.

Quoted 3 lines

Quoted 1 line

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.

drivers/usb/class/usbtmc.c new file +3 -0

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

drivers/usb/class/usbtmc.c new file +25 -0

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

drivers/usb/class/usbtmc.c new file +5 -0

+
+		/* Copy buffer to user space */
+		if (copy_to_user(buf + done, &buffer[12], n_characters)) {
+			/* There must have been an addressing problem */
+			retval = -EFAULT;

drivers/usb/class/usbtmc.c new file +18 -0

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

drivers/usb/class/usbtmc.c new file +12 -0

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

drivers/usb/class/usbtmc.c new file +12 -0

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

drivers/usb/class/usbtmc.c new file +7 -0

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

drivers/usb/class/usbtmc.c new file +15 -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,		\

drivers/usb/class/usbtmc.c new file +10 -0

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

drivers/usb/class/usbtmc.c new file +54 -0

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

drivers/usb/class/usbtmc.c new file +4 -0

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

Am Donnerstag 28 August 2008 01:47:20 schrieb Greg KH:

Quoted 16 lines · Alan Stern

Quoted 15 lines · Greg KH

Quoted 14 lines · Alan Stern

Quoted 11 lines · Greg KH

Quoted 3 lines

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.

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

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.

Quoted 3 lines

drivers/usb/class/usbtmc.c new file +3 -0

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

[..]

drivers/usb/class/usbtmc.c line 403

Quoted 11 lines

drivers/usb/class/usbtmc.c new file +11 -0

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

drivers/usb/class/usbtmc.c line 405

Quoted 2 lines

drivers/usb/class/usbtmc.c new file +2 -0

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

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

Quoted 6 lines

drivers/usb/class/usbtmc.c new file +6 -0

+		buffer[9] = data->TermChar;
+		buffer[10] = 0; /* Reserved */
+		buffer[11] = 0; /* Reserved */
+
+		/* Send bulk URB */
+		retval = usb_bulk_msg(data->usb_dev,

drivers/usb/class/usbtmc.c line 417

Quoted 6 lines

drivers/usb/class/usbtmc.c new file +6 -0

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

drivers/usb/class/usbtmc.c line 452

Quoted 5 lines

drivers/usb/class/usbtmc.c new file +5 -0

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

endianness macro

Quoted 8 lines

drivers/usb/class/usbtmc.c new file +8 -0

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

drivers/usb/class/usbtmc.c line 467

Quoted 6 lines

drivers/usb/class/usbtmc.c new file +6 -0

+		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

Quoted 16 lines

drivers/usb/class/usbtmc.c new file +16 -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;
+
+	data = filp->private_data;
+
+	buffer = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL);

drivers/usb/class/usbtmc.c line 382

Quoted 4 lines

drivers/usb/class/usbtmc.c new file +4 -0

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

drivers/usb/class/usbtmc.c line 539

Quoted 6 lines

drivers/usb/class/usbtmc.c new file +6 -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?

Quoted 11 lines

drivers/usb/class/usbtmc.c new file +11 -0

+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),

drivers/usb/class/usbtmc.c line 766

Quoted 6 lines

drivers/usb/class/usbtmc.c new file +6 -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

drivers/usb/class/usbtmc.c line 776

Quoted 8 lines

drivers/usb/class/usbtmc.c new file +8 -0

+	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

[..]

drivers/usb/class/usbtmc.c line 834

Quoted 10 lines

drivers/usb/class/usbtmc.c new file +10 -0

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

drivers/usb/class/usbtmc.c line 859

Quoted 9 lines

drivers/usb/class/usbtmc.c new file +9 -0

+{									\
+	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.

[..]

drivers/usb/class/usbtmc.c line 925

Quoted 7 lines

drivers/usb/class/usbtmc.c new file +7 -0

+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

Quoted 14 lines

drivers/usb/class/usbtmc.c new file +14 -0

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

drivers/usb/class/usbtmc.c line 945

Quoted 6 lines

drivers/usb/class/usbtmc.c new file +6 -0

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

drivers/usb/class/usbtmc.c line 1030

Quoted 8 lines

drivers/usb/class/usbtmc.c new file +8 -0

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

drivers/usb/class/usbtmc.c line 1045

Quoted 3 lines

drivers/usb/class/usbtmc.c new file +3 -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.

drivers/usb/class/usbtmc.c line 1055

Quoted 6 lines

drivers/usb/class/usbtmc.c new file +6 -0

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

Quoted 7 lines

drivers/usb/class/usbtmc.c line 403

Quoted 5 lines

drivers/usb/class/usbtmc.c new file +4 -0

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

Quoted 4 lines

drivers/usb/class/usbtmc.c line 405

Quoted 2 lines

drivers/usb/class/usbtmc.c new file +2 -0

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

Quoted 3 lines

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...

Quoted 1 line

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?

Quoted 8 lines

drivers/usb/class/usbtmc.c line 1055

Quoted 6 lines

drivers/usb/class/usbtmc.c new file +6 -0

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

Great, all done now.

Here's the updated version.

Quoted 3 lines · 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.

Great, all done now.

Here's the updated version.

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

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

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).

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).

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.

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 (?).

Quoted 10 lines

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.

Quoted 11 lines · On Friday 29 August 2008 16:39:02 Greg KH wrote:

Quoted 10 lines

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 (?).

Quoted 4 lines

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.