usb: usbtmc: Add support for missing functions in USBTMC-USB488 spec

PATCH Superseded Dave Penkler 2 participants 20 messages Nov 18, 2015 – Nov 28, 2015 USB

Share & tools

Dave Penkler

author

2015-11-18 08:32 UTC #0 lore raw

v5 cover usb: usbtmc: Add support for missing functions in USBTMC-USB488 spec

Implement support for the USB488 defined READ_STATUS_BYTE ioctl (1/5) and SRQ notifications with fasync (2/5) and poll/select (3/5) in order to be able to synchronize with variable duration instrument operations.

Add ioctls for other USB488 requests: REN_CONTROL, GOTO_LOCAL and LOCAL_LOCKOUT. (4/5)

Add convenience ioctl to return all device capabilities (5/5)

PATCH Changelog:
v5 - Remove excess newlines and parens
- Move dev variable initialisations to declaration
- Add comment on interrupt btag wrap
- simplify test in usbtmc_free_int()

v4 - Remove excess newlines and parens
- simplify some expressions

v3 - Split into multiple patches as per gregkh request

V2 - Fix V1 bug: not waking sleepers on disconnect.
- Correct sparse warnings.

V1 - Original patch

Testing:
All functions tested ok on an USBTMC-USB488 compliant oscilloscope

Dave Penkler (5):
Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE
operation.
Add support for USBTMC USB488 SRQ notification with fasync Add support for receiving USBTMC USB488 SRQ notifications via
poll/select
Add ioctl to retrieve USBTMC-USB488 capabilities
Add ioctls to enable and disable local controls on an instrument

 drivers/usb/class/usbtmc.c   | 342 +++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/usb/tmc.h |  29 +++-
 2 files changed, 368 insertions(+), 3 deletions(-)

Signature · 1 line

2.5.1

Dave Penkler

author

2015-11-18 08:38 UTC #1 lore raw

PATCH v5 1/5 Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.

Background:
When performing a read on an instrument that is executing a function that runs longer than the USB timeout the instrument may hang and require a device reset to recover. The READ_STATUS_BYTE operation always returns even when the instrument is busy permitting to poll for the appropriate condition. This capability is referred to in instrument application notes on synchronizing acquisitions for other platforms.

Signed-off-by Dave Penkler <[email protected]>

---
 drivers/usb/class/usbtmc.c   | 212 +++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/usb/tmc.h |   2 +
 2 files changed, 214 insertions(+)

drivers/usb/class/usbtmc.c +212 -0

index 7a11a82..60a71cb 100644
@@ -87,6 +87,19 @@ struct usbtmc_device_data {
 	u8 bTag_last_write;	/* needed for abort */
 	u8 bTag_last_read;	/* needed for abort */
 
+	/* data for interrupt in endpoint handling */
+	u8             bNotify1;
+	u8             bNotify2;
+	u16            ifnum;
+	u8             iin_bTag;
+	u8            *iin_buffer;
+	atomic_t       iin_data_valid;
+	unsigned int   iin_ep;
+	int            iin_ep_present;
+	int            iin_interval;
+	struct urb    *iin_urb;
+	u16            iin_wMaxPacketSize;
+
 	u8 rigol_quirk;
 
 	/* attributes from the USB TMC spec for this device */

@@ -99,6 +112,7 @@ struct usbtmc_device_data {
 	struct usbtmc_dev_capabilities	capabilities;
 	struct kref kref;
 	struct mutex io_mutex;	/* only one i/o function running at a time */
+	wait_queue_head_t waitq;
 };
 #define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)

@@ -373,6 +387,86 @@ exit:
 	return rv;
 }
 
+static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
+				unsigned long arg)
+{
+	u8 *buffer;
+	struct device *dev = &data->intf->dev;
+	int rv;
+	u8 tag, stb;
+
+	dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
+		data->iin_ep_present);
+
+	buffer = kmalloc(8, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	atomic_set(&data->iin_data_valid, 0);
+
+	rv = usb_control_msg(data->usb_dev,
+			usb_rcvctrlpipe(data->usb_dev, 0),
+			USBTMC488_REQUEST_READ_STATUS_BYTE,
+			USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+			data->iin_bTag,
+			data->ifnum,
+			buffer, 0x03, USBTMC_TIMEOUT);
+
+	if (rv < 0) {
+		dev_err(dev, "stb usb_control_msg returned %d\n", rv);
+		goto exit;
+	}
+
+	if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+		dev_err(dev, "control status returned %x\n", buffer[0]);
+		rv = -EIO;
+		goto exit;
+	}
+
+	if (data->iin_ep_present) {
+		rv = wait_event_interruptible_timeout(
+			data->waitq,
+			atomic_read(&data->iin_data_valid) != 0,
+			USBTMC_TIMEOUT);
+
+		if (rv < 0) {
+			dev_dbg(dev, "wait interrupted %d\n", rv);
+			goto exit;
+		}
+
+		if (rv == 0) {
+			dev_dbg(dev, "wait timed out\n");
+			rv = -ETIME;
+			goto exit;
+		}
+
+		tag = data->bNotify1 & 0x7f;
+
+		if (tag != data->iin_bTag) {
+			dev_err(dev, "expected bTag %x got %x\n",
+				data->iin_bTag, tag);
+		}
+
+		stb = data->bNotify2;
+	} else {
+		stb = buffer[2];
+	}
+
+	rv = copy_to_user((void __user *)arg, &stb, sizeof(stb));
+	if (rv)
+		rv = -EFAULT;
+
+ exit:
+	/* bump interrupt bTag */
+	data->iin_bTag += 1;
+	if (data->iin_bTag > 127)
+		/* 1 is for SRQ see USBTMC-USB488 subclass specification 4.3.1*/
+		data->iin_bTag = 2;
+
+	kfree(buffer);
+	return rv;
+}
+
 /*
  * Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
  * @transfer_size: number of bytes to request from the device.

@@ -1069,6 +1163,10 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case USBTMC_IOCTL_ABORT_BULK_IN:
 		retval = usbtmc_ioctl_abort_bulk_in(data);
 		break;
+
+	case USBTMC488_IOCTL_READ_STB:
+		retval = usbtmc488_ioctl_read_stb(data, arg);
+		break;
 	}
 
 skip_io_on_zombie:

@@ -1092,6 +1190,66 @@ static struct usb_class_driver usbtmc_class = {
 	.minor_base =	USBTMC_MINOR_BASE,
 };
 
+static void usbtmc_interrupt(struct urb *urb)
+{
+	struct usbtmc_device_data *data = urb->context;
+	int status = urb->status;
+	int rv;
+
+	dev_dbg(&data->intf->dev, "int status: %d len %d\n",
+		status, urb->actual_length);
+
+	switch (status) {
+	case 0: /* SUCCESS */
+		if (data->iin_buffer[0] & 0x80) {
+			/* check for valid STB notification */
+			if ((data->iin_buffer[0] & 0x7f) > 1) {
+				data->bNotify1 = data->iin_buffer[0];
+				data->bNotify2 = data->iin_buffer[1];
+				atomic_set(&data->iin_data_valid, 1);
+				wake_up_interruptible(&data->waitq);
+				goto exit;
+			}
+		}
+		dev_warn(&data->intf->dev, "invalid notification: %x\n",
+			data->iin_buffer[0]);
+		break;
+	case -EOVERFLOW:
+		dev_err(&data->intf->dev,
+			"%s - overflow with length %d, actual length is %d\n",
+			__func__, data->iin_wMaxPacketSize, urb->actual_length);
+	case -ECONNRESET:
+	case -ENOENT:
+	case -ESHUTDOWN:
+	case -EILSEQ:
+	case -ETIME:
+		/* urb terminated, clean up */
+		dev_dbg(&data->intf->dev,
+			"%s - urb terminated, status: %d\n",
+			__func__, status);
+		return;
+	default:
+		dev_err(&data->intf->dev,
+			"%s - unknown status received: %d\n",
+			__func__, status);
+	}
+exit:
+	rv = usb_submit_urb(urb, GFP_ATOMIC);
+	if (rv) {
+		dev_err(&data->intf->dev, "%s - usb_submit_urb failed: %d\n",
+			__func__, rv);
+	}
+}
+
+static void usbtmc_free_int(struct usbtmc_device_data *data)
+{
+	if (!data->iin_ep_present || !data->iin_urb)
+		return;
+	usb_kill_urb(data->iin_urb);
+	kfree(data->iin_buffer);
+	usb_free_urb(data->iin_urb);
+	kref_put(&data->kref, usbtmc_delete);
+}
 
 static int usbtmc_probe(struct usb_interface *intf,
 			const struct usb_device_id *id)

@@ -1114,6 +1272,8 @@ static int usbtmc_probe(struct usb_interface *intf,
 	usb_set_intfdata(intf, data);
 	kref_init(&data->kref);
 	mutex_init(&data->io_mutex);
+	init_waitqueue_head(&data->waitq);
+	atomic_set(&data->iin_data_valid, 0);
 	data->zombie = 0;
 
 	/* Determine if it is a Rigol or not */

@@ -1134,9 +1294,12 @@ static int usbtmc_probe(struct usb_interface *intf,
 	data->bTag	= 1;
 	data->TermCharEnabled = 0;
 	data->TermChar = '\n';
+	/*  2 <= bTag <= 127   USBTMC-USB488 subclass specification 4.3.1 */
+	data->iin_bTag = 2;
 
 	/* USBTMC devices have only one setting, so use that */
 	iface_desc = data->intf->cur_altsetting;
+	data->ifnum = iface_desc->desc.bInterfaceNumber;
 
 	/* Find bulk in endpoint */
 	for (n = 0; n < iface_desc->desc.bNumEndpoints; n++) {

@@ -1161,6 +1324,20 @@ static int usbtmc_probe(struct usb_interface *intf,
 			break;
 		}
 	}
+	/* Find int endpoint */
+	for (n = 0; n < iface_desc->desc.bNumEndpoints; n++) {
+		endpoint = &iface_desc->endpoint[n].desc;
+
+		if (usb_endpoint_is_int_in(endpoint)) {
+			data->iin_ep_present = 1;
+			data->iin_ep = endpoint->bEndpointAddress;
+			data->iin_wMaxPacketSize = usb_endpoint_maxp(endpoint);
+			data->iin_interval = endpoint->bInterval;
+			dev_dbg(&intf->dev, "Found Int in endpoint at %u\n",
+				data->iin_ep);
+			break;
+		}
+	}
 
 	retcode = get_capabilities(data);
 	if (retcode)

@@ -1169,6 +1346,39 @@ static int usbtmc_probe(struct usb_interface *intf,
 		retcode = sysfs_create_group(&intf->dev.kobj,
 					     &capability_attr_grp);
 
+	if (data->iin_ep_present) {
+		/* allocate int urb */
+		data->iin_urb = usb_alloc_urb(0, GFP_KERNEL);
+		if (!data->iin_urb) {
+			dev_err(&intf->dev, "Failed to allocate int urb\n");
+			goto error_register;
+		}
+
+		/* will reference data in int urb */
+		kref_get(&data->kref);
+
+		/* allocate buffer for interrupt in */
+		data->iin_buffer = kmalloc(data->iin_wMaxPacketSize,
+					GFP_KERNEL);
+		if (!data->iin_buffer) {
+			dev_err(&intf->dev, "Failed to allocate int buf\n");
+			goto error_register;
+		}
+
+		/* fill interrupt urb */
+		usb_fill_int_urb(data->iin_urb, data->usb_dev,
+				usb_rcvintpipe(data->usb_dev, data->iin_ep),
+				data->iin_buffer, data->iin_wMaxPacketSize,
+				usbtmc_interrupt,
+				data, data->iin_interval);
+
+		if (usb_submit_urb(data->iin_urb, GFP_KERNEL)) {
+			retcode = -EIO;
+			dev_err(&intf->dev, "Failed to submit iin_urb\n");
+			goto error_register;
+		}
+	}
+
 	retcode = sysfs_create_group(&intf->dev.kobj, &data_attr_grp);
 
 	retcode = usb_register_dev(intf, &usbtmc_class);

@@ -1185,6 +1395,7 @@ static int usbtmc_probe(struct usb_interface *intf,
 error_register:
 	sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
 	sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
+	usbtmc_free_int(data);
 	kref_put(&data->kref, usbtmc_delete);
 	return retcode;
 }

@@ -1196,6 +1407,7 @@ static void usbtmc_disconnect(struct usb_interface *intf)
 	dev_dbg(&intf->dev, "usbtmc_disconnect called\n");
 
 	data = usb_get_intfdata(intf);
+	usbtmc_free_int(data);
 	usb_deregister_dev(intf, &usbtmc_class);
 	sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
 	sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);

include/uapi/linux/usb/tmc.h +2 -0

index c045ae1..7e5ced8 100644
@@ -30,6 +30,7 @@
 #define USBTMC_REQUEST_CHECK_CLEAR_STATUS		6
 #define USBTMC_REQUEST_GET_CAPABILITIES			7
 #define USBTMC_REQUEST_INDICATOR_PULSE			64
+#define USBTMC488_REQUEST_READ_STATUS_BYTE		128
 
 /* Request values for USBTMC driver's ioctl entry point */
 #define USBTMC_IOC_NR			91

@@ -39,5 +40,6 @@
 #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)
+#define USBTMC488_IOCTL_READ_STB	_IOR(USBTMC_IOC_NR, 18, unsigned char)
 
 #endif

Signature · 1 line

2.5.1

Dave Penkler

author

2015-11-18 08:38 UTC #11 lore raw

PATCH v5 2/5 Add support for USBTMC USB488 SRQ notification with fasync

In reply to Dave Penkler

Background:
By configuring an instrument's event status register various conditions can be reported via an SRQ notification. This complements the synchronous polling approach using the READ_STATUS_BYTE ioctl with an asynchronous notification.

Signed-off-by Dave Penkler <[email protected]>

---
 drivers/usb/class/usbtmc.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

drivers/usb/class/usbtmc.c +24 -0

index 60a71cb..a95a63d 100644
@@ -99,6 +99,7 @@ struct usbtmc_device_data {
 	int            iin_interval;
 	struct urb    *iin_urb;
 	u16            iin_wMaxPacketSize;
+	atomic_t       srq_asserted;
 
 	u8 rigol_quirk;

@@ -113,6 +114,7 @@ struct usbtmc_device_data {
 	struct kref kref;
 	struct mutex io_mutex;	/* only one i/o function running at a time */
 	wait_queue_head_t waitq;
+	struct fasync_struct *fasync;
 };
 #define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)

@@ -404,6 +406,9 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
 
 	atomic_set(&data->iin_data_valid, 0);
 
+	/* must issue read_stb before using poll or select */
+	atomic_set(&data->srq_asserted, 0);
+
 	rv = usb_control_msg(data->usb_dev,
 			usb_rcvctrlpipe(data->usb_dev, 0),
 			USBTMC488_REQUEST_READ_STATUS_BYTE,

@@ -1174,6 +1179,13 @@ skip_io_on_zombie:
 	return retval;
 }
 
+static int usbtmc_fasync(int fd, struct file *file, int on)
+{
+	struct usbtmc_device_data *data = file->private_data;
+
+	return fasync_helper(fd, file, on, &data->fasync);
+}
+
 static const struct file_operations fops = {
 	.owner		= THIS_MODULE,
 	.read		= usbtmc_read,

@@ -1181,6 +1193,7 @@ static const struct file_operations fops = {
 	.open		= usbtmc_open,
 	.release	= usbtmc_release,
 	.unlocked_ioctl	= usbtmc_ioctl,
+	.fasync         = usbtmc_fasync,
 	.llseek		= default_llseek,
 };

@@ -1210,6 +1223,16 @@ static void usbtmc_interrupt(struct urb *urb)
 				wake_up_interruptible(&data->waitq);
 				goto exit;
 			}
+			/* check for SRQ notification */
+			if ((data->iin_buffer[0] & 0x7f) == 1) {
+				if (data->fasync)
+					kill_fasync(&data->fasync,
+						SIGIO, POLL_IN);
+
+				atomic_set(&data->srq_asserted, 1);
+				wake_up_interruptible(&data->waitq);
+				goto exit;
+			}
 		}
 		dev_warn(&data->intf->dev, "invalid notification: %x\n",
 			data->iin_buffer[0]);

@@ -1274,6 +1297,7 @@ static int usbtmc_probe(struct usb_interface *intf,
 	mutex_init(&data->io_mutex);
 	init_waitqueue_head(&data->waitq);
 	atomic_set(&data->iin_data_valid, 0);
+	atomic_set(&data->srq_asserted, 0);
 	data->zombie = 0;
 
 	/* Determine if it is a Rigol or not */

Signature · 1 line

2.5.1

Dave Penkler

author

2015-11-18 08:38 UTC #12 lore raw

PATCH v5 3/5 Add support for receiving USBTMC USB488 SRQ notifications via poll/select

In reply to Dave Penkler

Background:
In many situations operations on multiple instruments need to be synchronized. poll/select provide a convenient way of waiting on a number of different instruments and other peripherals simultaneously.

Signed-off-by Dave Penkler <[email protected]>

---
 drivers/usb/class/usbtmc.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

drivers/usb/class/usbtmc.c +23 -0

index a95a63d..6294a3c 100644
@@ -27,6 +27,7 @@
 #include <linux/uaccess.h>
 #include <linux/kref.h>
 #include <linux/slab.h>
+#include <linux/poll.h>
 #include <linux/mutex.h>
 #include <linux/usb.h>
 #include <linux/usb/tmc.h>

@@ -1186,6 +1187,27 @@ static int usbtmc_fasync(int fd, struct file *file, int on)
 	return fasync_helper(fd, file, on, &data->fasync);
 }
 
+static unsigned int usbtmc_poll(struct file *file, poll_table *wait)
+{
+	struct usbtmc_device_data *data = file->private_data;
+	unsigned int mask;
+
+	mutex_lock(&data->io_mutex);
+
+	if (data->zombie) {
+		mask = POLLHUP | POLLERR;
+		goto no_poll;
+	}
+
+	poll_wait(file, &data->waitq, wait);
+
+	mask = (atomic_read(&data->srq_asserted)) ? POLLIN | POLLRDNORM : 0;
+
+no_poll:
+	mutex_unlock(&data->io_mutex);
+	return mask;
+}
+
 static const struct file_operations fops = {
 	.owner		= THIS_MODULE,
 	.read		= usbtmc_read,

@@ -1194,6 +1216,7 @@ static const struct file_operations fops = {
 	.release	= usbtmc_release,
 	.unlocked_ioctl	= usbtmc_ioctl,
 	.fasync         = usbtmc_fasync,
+	.poll           = usbtmc_poll,
 	.llseek		= default_llseek,
 };

Signature · 1 line

2.5.1

Dave Penkler

author

2015-11-18 08:38 UTC #13 lore raw

PATCH v5 4/5 Add ioctl to retrieve USBTMC-USB488 capabilities

In reply to Dave Penkler

This is a convenience function to obtain an instrument's capabilities from its file descriptor without having to access sysfs from the user program.

Signed-off-by Dave Penkler <[email protected]>

---
 drivers/usb/class/usbtmc.c   | 12 ++++++++++++
 include/uapi/linux/usb/tmc.h | 21 ++++++++++++++++++---
 2 files changed, 30 insertions(+), 3 deletions(-)

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

index 6294a3c..2358991 100644
@@ -102,6 +102,9 @@ struct usbtmc_device_data {
 	u16            iin_wMaxPacketSize;
 	atomic_t       srq_asserted;
 
+	/* coalesced usb488_caps from usbtmc_dev_capabilities */
+	u8 usb488_caps;
+
 	u8 rigol_quirk;
 
 	/* attributes from the USB TMC spec for this device */

@@ -995,6 +998,7 @@ static int get_capabilities(struct usbtmc_device_data *data)
 	data->capabilities.device_capabilities = buffer[5];
 	data->capabilities.usb488_interface_capabilities = buffer[14];
 	data->capabilities.usb488_device_capabilities = buffer[15];
+	data->usb488_caps = (buffer[14] & 0x07) | ((buffer[15] & 0x0f) << 4);
 	rv = 0;
 
 err_out:

@@ -1170,6 +1174,14 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		retval = usbtmc_ioctl_abort_bulk_in(data);
 		break;
 
+	case USBTMC488_IOCTL_GET_CAPS:
+		retval = copy_to_user((void __user *)arg,
+				&data->usb488_caps,
+				sizeof(data->usb488_caps));
+		if (retval)
+			retval = -EFAULT;
+		break;
+
 	case USBTMC488_IOCTL_READ_STB:
 		retval = usbtmc488_ioctl_read_stb(data, arg);
 		break;

include/uapi/linux/usb/tmc.h +18 -3

index 7e5ced8..1dc3af1 100644
@@ -2,12 +2,14 @@
  * Copyright (C) 2007 Stefan Kopp, Gechingen, Germany
  * Copyright (C) 2008 Novell, Inc.
  * Copyright (C) 2008 Greg Kroah-Hartman <[email protected]>
+ * Copyright (C) 2015 Dave Penkler <[email protected]>
  *
  * This file holds USB constants defined by the USB Device Class
- * Definition for Test and Measurement devices published by the USB-IF.
+ * and USB488 Subclass Definitions 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.
+ * It also has the ioctl and capability definitions for the
+ * usbtmc kernel driver that userspace needs to know about.
  */
 
 #ifndef __LINUX_USB_TMC_H

@@ -40,6 +42,19 @@
 #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)
+#define USBTMC488_IOCTL_GET_CAPS	_IO(USBTMC_IOC_NR, 17)
 #define USBTMC488_IOCTL_READ_STB	_IOR(USBTMC_IOC_NR, 18, unsigned char)
 
+/* Driver encoded usb488 capabilities */
+#define USBTMC488_CAPABILITY_TRIGGER         1
+#define USBTMC488_CAPABILITY_SIMPLE          2
+#define USBTMC488_CAPABILITY_REN_CONTROL     2
+#define USBTMC488_CAPABILITY_GOTO_LOCAL      2
+#define USBTMC488_CAPABILITY_LOCAL_LOCKOUT   2
+#define USBTMC488_CAPABILITY_488_DOT_2       4
+#define USBTMC488_CAPABILITY_DT1             16
+#define USBTMC488_CAPABILITY_RL1             32
+#define USBTMC488_CAPABILITY_SR1             64
+#define USBTMC488_CAPABILITY_FULL_SCPI       128
+
 #endif

Signature · 1 line

2.5.1

Dave Penkler

author

2015-11-18 08:38 UTC #14 lore raw

PATCH v5 5/5 Add ioctls to enable and disable local controls on an instrument

In reply to Dave Penkler

These ioctls provide support for the USBTMC-USB488 control requests for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT

Signed-off-by Dave Penkler <[email protected]>

---
 drivers/usb/class/usbtmc.c   | 71 ++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/usb/tmc.h |  6 ++++
 2 files changed, 77 insertions(+)

drivers/usb/class/usbtmc.c +71 -0

index 2358991..d416a5f 100644
@@ -476,6 +476,62 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
 	return rv;
 }
 
+static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data,
+				unsigned long arg,
+				unsigned int cmd)
+{
+	u8 *buffer;
+	struct device *dev = &data->intf->dev;
+	int rv;
+	unsigned int val;
+	u16 wValue;
+
+	if (!(data->usb488_caps & USBTMC488_CAPABILITY_SIMPLE))
+		return -EINVAL;
+
+	buffer = kmalloc(8, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	if (cmd == USBTMC488_REQUEST_REN_CONTROL) {
+		rv = copy_from_user(&val, (void __user *)arg, sizeof(val));
+		if (rv) {
+			rv = -EFAULT;
+			goto exit;
+		}
+		wValue = val ? 1 : 0;
+	} else {
+		wValue = 0;
+	}
+
+	rv = usb_control_msg(data->usb_dev,
+			usb_rcvctrlpipe(data->usb_dev, 0),
+			cmd,
+			USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+			wValue,
+			data->ifnum,
+			buffer, 0x01, USBTMC_TIMEOUT);
+
+	if (rv < 0) {
+		dev_err(dev, "simple usb_control_msg failed %d\n", rv);
+		goto exit;
+	} else if (rv != 1) {
+		dev_warn(dev, "simple usb_control_msg returned %d\n", rv);
+		goto exit;
+	}
+
+	if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+		dev_err(dev, "simple control status returned %x\n", buffer[0]);
+		rv = -EIO;
+		goto exit;
+	}
+	rv = 0;
+
+ exit:
+	kfree(buffer);
+	return rv;
+}
+
 /*
  * Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
  * @transfer_size: number of bytes to request from the device.

@@ -1185,6 +1241,21 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case USBTMC488_IOCTL_READ_STB:
 		retval = usbtmc488_ioctl_read_stb(data, arg);
 		break;
+
+	case USBTMC488_IOCTL_REN_CONTROL:
+		retval = usbtmc488_ioctl_simple(data, arg,
+						USBTMC488_REQUEST_REN_CONTROL);
+		break;
+
+	case USBTMC488_IOCTL_GOTO_LOCAL:
+		retval = usbtmc488_ioctl_simple(data, arg,
+						USBTMC488_REQUEST_GOTO_LOCAL);
+		break;
+
+	case USBTMC488_IOCTL_LOCAL_LOCKOUT:
+		retval = usbtmc488_ioctl_simple(data, arg,
+						USBTMC488_REQUEST_LOCAL_LOCKOUT);
+		break;
 	}
 
 skip_io_on_zombie:

include/uapi/linux/usb/tmc.h +6 -0

index 1dc3af1..0d852c9 100644
@@ -33,6 +33,9 @@
 #define USBTMC_REQUEST_GET_CAPABILITIES			7
 #define USBTMC_REQUEST_INDICATOR_PULSE			64
 #define USBTMC488_REQUEST_READ_STATUS_BYTE		128
+#define USBTMC488_REQUEST_REN_CONTROL			160
+#define USBTMC488_REQUEST_GOTO_LOCAL			161
+#define USBTMC488_REQUEST_LOCAL_LOCKOUT			162
 
 /* Request values for USBTMC driver's ioctl entry point */
 #define USBTMC_IOC_NR			91

@@ -44,6 +47,9 @@
 #define USBTMC_IOCTL_CLEAR_IN_HALT	_IO(USBTMC_IOC_NR, 7)
 #define USBTMC488_IOCTL_GET_CAPS	_IO(USBTMC_IOC_NR, 17)
 #define USBTMC488_IOCTL_READ_STB	_IOR(USBTMC_IOC_NR, 18, unsigned char)
+#define USBTMC488_IOCTL_REN_CONTROL	_IOW(USBTMC_IOC_NR, 19, unsigned char)
+#define USBTMC488_IOCTL_GOTO_LOCAL	_IO(USBTMC_IOC_NR, 20)
+#define USBTMC488_IOCTL_LOCAL_LOCKOUT	_IO(USBTMC_IOC_NR, 21)
 
 /* Driver encoded usb488 capabilities */
 #define USBTMC488_CAPABILITY_TRIGGER         1

Signature · 1 line

2.5.1

Andy Shevchenko

2015-11-18 09:41 UTC #15 lore raw

Re: [PATCH v5 5/5] Add ioctls to enable and disable local controls on an instrument

Quoted 2 lines · Dave Penkler

These ioctls provide support for the USBTMC-USB488 control requests for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT

Couple of comments below.

Quoted 34 lines

drivers/usb/class/usbtmc.c +26 -0

index 2358991..d416a5f 100644
@@ -476,6 +476,62 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
        return rv;
 }
 
+static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data,
+                               unsigned long arg,
+                               unsigned int cmd)
+{
+       u8 *buffer;
+       struct device *dev = &data->intf->dev;
+       int rv;
+       unsigned int val;
+       u16 wValue;
+
+       if (!(data->usb488_caps & USBTMC488_CAPABILITY_SIMPLE))
+               return -EINVAL;
+
+       buffer = kmalloc(8, GFP_KERNEL);
+       if (!buffer)
+               return -ENOMEM;
+
+       if (cmd == USBTMC488_REQUEST_REN_CONTROL) {
+               rv = copy_from_user(&val, (void __user *)arg, sizeof(val));
+               if (rv) {
+                       rv = -EFAULT;
+                       goto exit;
+               }
+               wValue = val ? 1 : 0;
+       } else {
+               wValue = 0;

drivers/usb/class/usbtmc.c static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data, · line 514

Quoted 10 lines

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

+       }
+
+       rv = usb_control_msg(data->usb_dev,
+                       usb_rcvctrlpipe(data->usb_dev, 0),
+                       cmd,
+                       USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+                       wValue,
+                       data->ifnum,
+                       buffer, 0x01, USBTMC_TIMEOUT);
+

Redundant empty line

drivers/usb/class/usbtmc.c static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data, · line 519

Quoted 5 lines

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

+       if (rv < 0) {
+               dev_err(dev, "simple usb_control_msg failed %d\n", rv);
+               goto exit;
+       } else if (rv != 1) {
+               dev_warn(dev, "simple usb_control_msg returned %d\n", rv);

Actually here what king of results could be? 0? 2+? In all cases of error you have to provide an error code.

Quoted 15 lines

drivers/usb/class/usbtmc.c +15 -0

+               goto exit;
+       }
+
+       if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+               dev_err(dev, "simple control status returned %x\n", buffer[0]);
+               rv = -EIO;
+               goto exit;
+       }
+       rv = 0;
+
+ exit:
+       kfree(buffer);
+       return rv;
+}
+

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2015-11-18 09:55 UTC #2 lore raw

Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.

In reply to Dave Penkler

Quoted 9 lines · Dave Penkler

Background:
When performing a read on an instrument that is executing a function that runs longer than the USB timeout the instrument may hang and require a device reset to recover. The READ_STATUS_BYTE operation always returns even when the instrument is busy permitting to poll for the appropriate condition. This capability is referred to in instrument application notes on synchronizing acquisitions for other platforms.

First of all, please be patient and do not send patches immediately when you answered to someone's review. It increases redundant noise and burden on reviewer. Wait at least for 24h especially if there are topics still to discuss.

[]

drivers/usb/class/usbtmc.c exit: · line 396

Quoted 7 lines

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

+static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
+                               unsigned long arg)
+{
+       u8 *buffer;
+       struct device *dev = &data->intf->dev;
+       int rv;
+       u8 tag, stb;

If you rearrange this like

+       struct device *dev = &data->intf->dev;
+       u8 *buffer;
+       u8 tag, stb;
+       int rv;

it will look better. Similar to check in all your patches.

Quoted 8 lines

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

+
+       dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
+               data->iin_ep_present);
+
+       buffer = kmalloc(8, GFP_KERNEL);
+       if (!buffer)
+               return -ENOMEM;
+

drivers/usb/class/usbtmc.c exit: · line 414

Quoted 10 lines

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

+       atomic_set(&data->iin_data_valid, 0);
+
+       rv = usb_control_msg(data->usb_dev,
+                       usb_rcvctrlpipe(data->usb_dev, 0),
+                       USBTMC488_REQUEST_READ_STATUS_BYTE,
+                       USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+                       data->iin_bTag,
+                       data->ifnum,
+                       buffer, 0x03, USBTMC_TIMEOUT);
+

Redundant empty line.

Quoted 7 lines

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

+       if (rv < 0) {
+               dev_err(dev, "stb usb_control_msg returned %d\n", rv);
+               goto exit;
+       }
+
+       if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+               dev_err(dev, "control status returned %x\n", buffer[0]);

drivers/usb/class/usbtmc.c exit: · line 431

Quoted 10 lines

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

+               rv = -EIO;
+               goto exit;
+       }
+
+       if (data->iin_ep_present) {
+               rv = wait_event_interruptible_timeout(
+                       data->waitq,
+                       atomic_read(&data->iin_data_valid) != 0,
+                       USBTMC_TIMEOUT);
+

Ditto.

drivers/usb/class/usbtmc.c exit: · line 444

Quoted 13 lines

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

+               if (rv < 0) {
+                       dev_dbg(dev, "wait interrupted %d\n", rv);
+                       goto exit;
+               }
+
+               if (rv == 0) {
+                       dev_dbg(dev, "wait timed out\n");
+                       rv = -ETIME;
+                       goto exit;
+               }
+
+               tag = data->bNotify1 & 0x7f;
+

You may remove empty line, though it's minor nitpick here.

Quoted 9 lines

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

+               if (tag != data->iin_bTag) {
+                       dev_err(dev, "expected bTag %x got %x\n",
+                               data->iin_bTag, tag);
+               }
+
+               stb = data->bNotify2;
+       } else {
+               stb = buffer[2];
+       }

drivers/usb/class/usbtmc.c exit: · line 463

Quoted 10 lines

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

+
+       rv = copy_to_user((void __user *)arg, &stb, sizeof(stb));
+       if (rv)
+               rv = -EFAULT;
+
+ exit:
+       /* bump interrupt bTag */
+       data->iin_bTag += 1;
+       if (data->iin_bTag > 127)
+               /* 1 is for SRQ see USBTMC-USB488 subclass specification 4.3.1*/

Missed space before asterisk.

drivers/usb/class/usbtmc.c exit: · line 468

Quoted 5 lines

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

+               data->iin_bTag = 2;
+
+       kfree(buffer);
+       return rv;
+}

[]

Quoted 4 lines

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

+static void usbtmc_interrupt(struct urb *urb)
+{
+       struct usbtmc_device_data *data = urb->context;
+       int status = urb->status;

drivers/usb/class/usbtmc.c static struct usb_class_driver usbtmc_class = { · line 1206

Quoted 10 lines

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

+       int rv;
+
+       dev_dbg(&data->intf->dev, "int status: %d len %d\n",
+               status, urb->actual_length);
+
+       switch (status) {
+       case 0: /* SUCCESS */
+               if (data->iin_buffer[0] & 0x80) {
+                       /* check for valid STB notification */
+                       if ((data->iin_buffer[0] & 0x7f) > 1) {

Despite your answer to my comment code is quite understandable even with & 0x7e. You already put comment line about this, you may add that you validate the value to be 127 >= value >= 2.

Quoted 13 lines

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

+                               data->bNotify1 = data->iin_buffer[0];
+                               data->bNotify2 = data->iin_buffer[1];
+                               atomic_set(&data->iin_data_valid, 1);
+                               wake_up_interruptible(&data->waitq);
+                               goto exit;
+                       }
+               }
+               dev_warn(&data->intf->dev, "invalid notification: %x\n",
+                       data->iin_buffer[0]);
+               break;
+       case -EOVERFLOW:
+               dev_err(&data->intf->dev,
+                       "%s - overflow with length %d, actual length is %d\n",

drivers/usb/class/usbtmc.c static struct usb_class_driver usbtmc_class = { · line 1229

Quoted 10 lines

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

+                       __func__, data->iin_wMaxPacketSize, urb->actual_length);
+       case -ECONNRESET:
+       case -ENOENT:
+       case -ESHUTDOWN:
+       case -EILSEQ:
+       case -ETIME:
+               /* urb terminated, clean up */
+               dev_dbg(&data->intf->dev,
+                       "%s - urb terminated, status: %d\n",
+                       __func__, status);

No need to print function here explicitly. Check Dynamic Debug framework.

Quoted 90 lines

drivers/usb/class/usbtmc.c +59 -0

+               return;
+       default:
+               dev_err(&data->intf->dev,
+                       "%s - unknown status received: %d\n",
+                       __func__, status);
+       }
+exit:
+       rv = usb_submit_urb(urb, GFP_ATOMIC);
+       if (rv) {
+               dev_err(&data->intf->dev, "%s - usb_submit_urb failed: %d\n",
+                       __func__, rv);
+       }
+}
+
+static void usbtmc_free_int(struct usbtmc_device_data *data)
+{
+       if (!data->iin_ep_present || !data->iin_urb)
+               return;
+       usb_kill_urb(data->iin_urb);
+       kfree(data->iin_buffer);
+       usb_free_urb(data->iin_urb);
+       kref_put(&data->kref, usbtmc_delete);
+}
 
 static int usbtmc_probe(struct usb_interface *intf,
                        const struct usb_device_id *id)

@@ -1114,6 +1272,8 @@ static int usbtmc_probe(struct usb_interface *intf,
        usb_set_intfdata(intf, data);
        kref_init(&data->kref);
        mutex_init(&data->io_mutex);
+       init_waitqueue_head(&data->waitq);
+       atomic_set(&data->iin_data_valid, 0);
        data->zombie = 0;
 
        /* Determine if it is a Rigol or not */

@@ -1134,9 +1294,12 @@ static int usbtmc_probe(struct usb_interface *intf,
        data->bTag      = 1;
        data->TermCharEnabled = 0;
        data->TermChar = '\n';
+       /*  2 <= bTag <= 127   USBTMC-USB488 subclass specification 4.3.1 */
+       data->iin_bTag = 2;
 
        /* USBTMC devices have only one setting, so use that */
        iface_desc = data->intf->cur_altsetting;
+       data->ifnum = iface_desc->desc.bInterfaceNumber;
 
        /* Find bulk in endpoint */
        for (n = 0; n < iface_desc->desc.bNumEndpoints; n++) {

@@ -1161,6 +1324,20 @@ static int usbtmc_probe(struct usb_interface *intf,
                        break;
                }
        }
+       /* Find int endpoint */
+       for (n = 0; n < iface_desc->desc.bNumEndpoints; n++) {
+               endpoint = &iface_desc->endpoint[n].desc;
+
+               if (usb_endpoint_is_int_in(endpoint)) {
+                       data->iin_ep_present = 1;
+                       data->iin_ep = endpoint->bEndpointAddress;
+                       data->iin_wMaxPacketSize = usb_endpoint_maxp(endpoint);
+                       data->iin_interval = endpoint->bInterval;
+                       dev_dbg(&intf->dev, "Found Int in endpoint at %u\n",
+                               data->iin_ep);
+                       break;
+               }
+       }
 
        retcode = get_capabilities(data);
        if (retcode)

@@ -1169,6 +1346,39 @@ static int usbtmc_probe(struct usb_interface *intf,
                retcode = sysfs_create_group(&intf->dev.kobj,
                                             &capability_attr_grp);
 
+       if (data->iin_ep_present) {
+               /* allocate int urb */
+               data->iin_urb = usb_alloc_urb(0, GFP_KERNEL);
+               if (!data->iin_urb) {
+                       dev_err(&intf->dev, "Failed to allocate int urb\n");
+                       goto error_register;
+               }
+
+               /* will reference data in int urb */
+               kref_get(&data->kref);
+
+               /* allocate buffer for interrupt in */
+               data->iin_buffer = kmalloc(data->iin_wMaxPacketSize,
+                                       GFP_KERNEL);
+               if (!data->iin_buffer) {
+                       dev_err(&intf->dev, "Failed to allocate int buf\n");
+                       goto error_register;

drivers/usb/class/usbtmc.c static int usbtmc_probe(struct usb_interface *intf, · line 1375

Quoted 10 lines

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

+               }
+
+               /* fill interrupt urb */
+               usb_fill_int_urb(data->iin_urb, data->usb_dev,
+                               usb_rcvintpipe(data->usb_dev, data->iin_ep),
+                               data->iin_buffer, data->iin_wMaxPacketSize,
+                               usbtmc_interrupt,
+                               data, data->iin_interval);
+
+               if (usb_submit_urb(data->iin_urb, GFP_KERNEL)) {

Does it return a proper error code?

Quoted 10 lines

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

+                       retcode = -EIO;
+                       dev_err(&intf->dev, "Failed to submit iin_urb\n");
+                       goto error_register;
+               }
+       }
+
 
        retcode = sysfs_create_group(&intf->dev.kobj, &data_attr_grp);

        retcode = usb_register_dev(intf, &usbtmc_class);

Hmm… Unrelated to this patch, but notice that retcode is overridden here.

Quoted 8 lines

drivers/usb/class/usbtmc.c +1 -0

@@ -1185,6 +1395,7 @@ static int usbtmc_probe(struct usb_interface *intf,
 error_register:
        sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
        sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
+       usbtmc_free_int(data);
        kref_put(&data->kref, usbtmc_delete);
        return retcode;
 }

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Dave Penkler

author

2015-11-22 08:52 UTC #16 lore raw

Re: [PATCH v5 5/5] Add ioctls to enable and disable local controls on an instrument

In reply to Andy Shevchenko

Quoted 11 lines · Andy Shevchenko

Quoted 2 lines · Dave Penkler
These ioctls provide support for the USBTMC-USB488 control requests for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT
Couple of comments below.
Quoted 5 lines
drivers/usb/class/usbtmc.c +0 -0
index 2358991..d416a5f 100644
@@ -476,6 +476,62 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,

snip

Quoted 10 lines

drivers/usb/class/usbtmc.c static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data, · line 519
Quoted 6 lines
drivers/usb/class/usbtmc.c +5 -0
+       if (rv < 0) {
+               dev_err(dev, "simple usb_control_msg failed %d\n", rv);
+               goto exit;
+       } else if (rv != 1) {
+               dev_warn(dev, "simple usb_control_msg returned %d\n", rv);
Actually here what king of results could be? 0? 2+? In all cases of error you have to provide an error code.

We seem to be going round in circles here, last time you suggested to propagate the return value. The non-negative return is the number of bytes transferred which should be 1 unless there is some usb implementation flakiness happening. So I will go back to returning -EIO.

Quoted 7 lines

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

+               goto exit;
+       }
+
+       if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+               dev_err(dev, "simple control status returned %x\n", buffer[0]);
+               rv = -EIO;
+               goto exit;

Quoted 12 lines

Quoted 8 lines

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

+       }
+       rv = 0;
+
+ exit:
+       kfree(buffer);
+       return rv;
+}
+

Signature · 2 lines

With Best Regards,
Andy Shevchenko

cheers,
-Dave

Dave Penkler

author

2015-11-22 09:20 UTC #3 lore raw

Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.

In reply to Andy Shevchenko

Quoted 11 lines · Andy Shevchenko

Quoted 9 lines · Dave Penkler
Background:
When performing a read on an instrument that is executing a function that runs longer than the USB timeout the instrument may hang and require a device reset to recover. The READ_STATUS_BYTE operation always returns even when the instrument is busy permitting to poll for the appropriate condition. This capability is referred to in instrument application notes on synchronizing acquisitions for other platforms.

Quoted 5 lines

First of all, please be patient and do not send patches immediately when you answered to someone's review. It increases redundant noise and burden on reviewer. Wait at least for 24h especially if there are topics still to discuss.

OK, apologies.

snip

Quoted 11 lines

drivers/usb/class/usbtmc.c static struct usb_class_driver usbtmc_class = { · line 1206
Quoted 6 lines
drivers/usb/class/usbtmc.c +6 -0
+
+       switch (status) {
+       case 0: /* SUCCESS */
+               if (data->iin_buffer[0] & 0x80) {
+                       /* check for valid STB notification */
+                       if ((data->iin_buffer[0] & 0x7f) > 1) {
Despite your answer to my comment code is quite understandable even with & 0x7e. You already put comment line about this, you may add that you validate the value to be 127 >= value >= 2.

Yes it is quite understandable but it is less clear. I repeat my comment here:
When reading the spec and the code it is more obvious that here we are testing for the value in bits D6..D0 to be a valid iin_bTag return. (See Table 7 in the USBTMC-USB488 spec.)

What is your motivation for

if (data->iin_buffer[0] & 0x7e)

Quoted 7 lines

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

+                               data->bNotify1 = data->iin_buffer[0];
+                               data->bNotify2 = data->iin_buffer[1];
+                               atomic_set(&data->iin_data_valid, 1);
+                               wake_up_interruptible(&data->waitq);
+                               goto exit;
+                       }
+               }

snip

Quoted 6 lines

drivers/usb/class/usbtmc.c static struct usb_class_driver usbtmc_class = { · line 1229
Quoted 4 lines
drivers/usb/class/usbtmc.c +4 -0
+               /* urb terminated, clean up */
+               dev_dbg(&data->intf->dev,
+                       "%s - urb terminated, status: %d\n",
+                       __func__, status);
No need to print function here explicitly. Check Dynamic Debug framework.

I am not using dynamic debug but when I enable static debug I get:

[ 1438.562458] usbtmc 1-1:1.0: Enter ioctl_read_stb iin_ep_present: 1

on the console log for

dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",

		data->iin_ep_present);

So if I don't print the function it does not appear on the log.

Quoted 14 lines

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

+               return;
+       default:
+               dev_err(&data->intf->dev,
+                       "%s - unknown status received: %d\n",
+                       __func__, status);
+       }
+exit:
+       rv = usb_submit_urb(urb, GFP_ATOMIC);
+       if (rv) {
+               dev_err(&data->intf->dev, "%s - usb_submit_urb failed: %d\n",
+                       __func__, rv);
+       }
+}

snip

Quoted 12 lines

drivers/usb/class/usbtmc.c static int usbtmc_probe(struct usb_interface *intf, · line 1375

Quoted 9 lines

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

+
+               /* fill interrupt urb */
+               usb_fill_int_urb(data->iin_urb, data->usb_dev,
+                               usb_rcvintpipe(data->usb_dev, data->iin_ep),
+                               data->iin_buffer, data->iin_wMaxPacketSize,
+                               usbtmc_interrupt,
+                               data, data->iin_interval);
+
+               if (usb_submit_urb(data->iin_urb, GFP_KERNEL)) {

Does it return a proper error code?

Yes, will propagate it.

Quoted 14 lines

drivers/usb/class/usbtmc.c static int usbtmc_probe(struct usb_interface *intf, · line 1384
Quoted 10 lines
drivers/usb/class/usbtmc.c +6 -0
+                       retcode = -EIO;
+                       dev_err(&intf->dev, "Failed to submit iin_urb\n");
+                       goto error_register;
+               }
+       }
+
 
        retcode = sysfs_create_group(&intf->dev.kobj, &data_attr_grp);
 
        retcode = usb_register_dev(intf, &usbtmc_class);
Hmm??? Unrelated to this patch, but notice that retcode is overridden here.

Quoted 13 lines

Quoted 8 lines

drivers/usb/class/usbtmc.c +1 -0

@@ -1185,6 +1395,7 @@ static int usbtmc_probe(struct usb_interface *intf,
 error_register:
        sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
        sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
+       usbtmc_free_int(data);
        kref_put(&data->kref, usbtmc_delete);
        return retcode;
 }

Signature · 2 lines

With Best Regards,
Andy Shevchenko

cheers,
-Dave

Andy Shevchenko

2015-11-22 10:32 UTC #4 lore raw

Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.

Quoted 19 lines · Dave Penkler

Quoted 8 lines · Andy Shevchenko
drivers/usb/class/usbtmc.c static struct usb_class_driver usbtmc_class = { · line 1206
Quoted 5 lines · Dave Penkler
drivers/usb/class/usbtmc.c +5 -0
+       switch (status) {
+       case 0: /* SUCCESS */
+               if (data->iin_buffer[0] & 0x80) {
+                       /* check for valid STB notification */
+                       if ((data->iin_buffer[0] & 0x7f) > 1) {
Quoted 4 lines
Despite your answer to my comment code is quite understandable even with & 0x7e. You already put comment line about this, you may add that you validate the value to be 127 >= value >= 2.
Yes it is quite understandable but it is less clear. I repeat my comment here:
When reading the spec and the code it is more obvious that here we are testing for the value in bits D6..D0 to be a valid iin_bTag return. (See Table 7 in the USBTMC-USB488 spec.)

Quoted 5 lines

What is your motivation for
if (data->iin_buffer[0] & 0x7e)
?

In non-optimized variant it will certainly generate less code. You may have check assembly code with -O2 and compare. I don't know if compiler is clever enough to do the same by itself.

Quoted 9 lines

Quoted 6 lines
drivers/usb/class/usbtmc.c static struct usb_class_driver usbtmc_class = { · line 1229
Quoted 4 lines
drivers/usb/class/usbtmc.c +4 -0
+               /* urb terminated, clean up */
+               dev_dbg(&data->intf->dev,
+                       "%s - urb terminated, status: %d\n",
+                       __func__, status);
No need to print function here explicitly. Check Dynamic Debug framework.
I am not using dynamic debug but when I enable static debug I get:

Quoted 8 lines

[ 1438.562458] usbtmc 1-1:1.0: Enter ioctl_read_stb iin_ep_present: 1
on the console log for
dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
                data->iin_ep_present);
So if I don't print the function it does not appear on the log.

Whatever maintainers prefer, though I think there are quite rare cases in USB when someone needs static debug. I'm pretty sure most of the developers all in favour of dynamic debug.

Quoted 5 lines

Quoted 5 lines
Quoted 3 lines
        retcode = sysfs_create_group(&intf->dev.kobj, &data_attr_grp);

        retcode = usb_register_dev(intf, &usbtmc_class);
Hmm??? Unrelated to this patch, but notice that retcode is overridden here.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2015-11-22 10:36 UTC #17 lore raw

Re: [PATCH v5 5/5] Add ioctls to enable and disable local controls on an instrument

In reply to Dave Penkler

Quoted 10 lines · Dave Penkler

Quoted 9 lines · Andy Shevchenko

Quoted 5 lines · Dave Penkler

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

+       if (rv < 0) {
+               dev_err(dev, "simple usb_control_msg failed %d\n", rv);
+               goto exit;
+       } else if (rv != 1) {
+               dev_warn(dev, "simple usb_control_msg returned %d\n", rv);

Quoted 6 lines

Quoted 3 lines
Actually here what king of results could be? 0? 2+? In all cases of error you have to provide an error code.
We seem to be going round in circles here, last time you suggested to propagate the return value.

You didn't pay much attention to where I put my comment. You have few branches depending on return value

1) negative, apparently an error code, should be propagated if nothing specific to framework;
2) zero, what does it means?
3) one, seems the expected result when success, so, error code should be 0;
4) two, three, … non-negative numbers,see 2).

For my understanding 2) and 4) have to return what you initially had -EIO.

Quoted 3 lines

The non-negative return is the number of bytes
transferred which should be 1 unless there is some usb implementation flakiness happening. So I will go back to returning -EIO.

Yes, in *this* branch.

Quoted 15 lines

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

+               goto exit;
+       }
+
+       if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+               dev_err(dev, "simple control status returned %x\n", buffer[0]);
+               rv = -EIO;
+               goto exit;
+       }
+       rv = 0;
+
+ exit:
+       kfree(buffer);
+       return rv;
+}

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Dave Penkler

author

2015-11-25 09:13 UTC #18 lore raw

Re: [PATCH v5 5/5] Add ioctls to enable and disable local controls on an instrument

Quoted 18 lines · Andy Shevchenko

Quoted 10 lines · Dave Penkler
Quoted 9 lines · Andy Shevchenko
Quoted 5 lines · Dave Penkler
drivers/usb/class/usbtmc.c +5 -0
+       if (rv < 0) {
+               dev_err(dev, "simple usb_control_msg failed %d\n", rv);
+               goto exit;
+       } else if (rv != 1) {
+               dev_warn(dev, "simple usb_control_msg returned %d\n", rv);
Quoted 6 lines
Quoted 3 lines
Actually here what king of results could be? 0? 2+? In all cases of error you have to provide an error code.
We seem to be going round in circles here, last time you suggested to propagate the return value.

Quoted 6 lines

You didn't pay much attention to where I put my comment. You have few branches depending on return value
1) negative, apparently an error code, should be propagated if nothing specific to framework;
2) zero, what does it means?

Zero bytes transferred -> error

Quoted 1 line

3) one, seems the expected result when success, so, error code should be 0;

Which it is when we drop through

Quoted 10 lines

4) two, three, ??? non-negative numbers,see 2).
For my understanding 2) and 4) have to return what you initially had -EIO.
Quoted 3 lines
The non-negative return is the number of bytes
transferred which should be 1 unless there is some usb implementation flakiness happening. So I will go back to returning -EIO.
Yes, in *this* branch.

OK so now I have the same as in v4 again:

	if (rv < 0) {
		dev_err(dev, "simple usb_control_msg failed %d\n", rv);
		goto exit;
	} else if (rv != 1) {
		dev_warn(dev, "simple usb_control_msg returned %d\n", rv);
		rv = -EIO;
		goto exit;
	}

Quoted 19 lines

Quoted 15 lines

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

+               goto exit;
+       }
+
+       if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+               dev_err(dev, "simple control status returned %x\n", buffer[0]);
+               rv = -EIO;
+               goto exit;
+       }
+       rv = 0;
+
+ exit:
+       kfree(buffer);
+       return rv;
+}

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Dave Penkler

author

2015-11-25 09:18 UTC #5 lore raw

Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.

In reply to Andy Shevchenko

Quoted 22 lines · Andy Shevchenko

Quoted 21 lines · Dave Penkler
Quoted 8 lines · Andy Shevchenko
drivers/usb/class/usbtmc.c static struct usb_class_driver usbtmc_class = { · line 1206
Quoted 5 lines · Dave Penkler
drivers/usb/class/usbtmc.c +5 -0
+       switch (status) {
+       case 0: /* SUCCESS */
+               if (data->iin_buffer[0] & 0x80) {
+                       /* check for valid STB notification */
+                       if ((data->iin_buffer[0] & 0x7f) > 1) {
Quoted 4 lines
Despite your answer to my comment code is quite understandable even with & 0x7e. You already put comment line about this, you may add that you validate the value to be 127 >= value >= 2.
Yes it is quite understandable but it is less clear. I repeat my comment here:
When reading the spec and the code it is more obvious that here we are testing for the value in bits D6..D0 to be a valid iin_bTag return. (See Table 7 in the USBTMC-USB488 spec.)
What is your motivation for

Quoted 8 lines

Quoted 3 lines
if (data->iin_buffer[0] & 0x7e)
?
In non-optimized variant it will certainly generate less code. You may have check assembly code with -O2 and compare. I don't know if compiler is clever enough to do the same by itself.

I tested out both variants, and the explicit test is actually faster on by box:

$ cat tp.c
#include <stdlib.h>
#include <stdio.h>
#define xstr(s) str(s)
#define str(s) #s
main() {
        unsigned int v,s=0;
        struct recs {
                unsigned char *iin_buffer;
        } rec;
        struct recs *data = &rec;
        data->iin_buffer = (unsigned char *) malloc(8);
        for (v=1;v;v++) {
                data->iin_buffer[0] = v & 0x7f;
                if (TEST)
                        s++;
        }
        printf("%s %x\n",xstr(TEST),s);
}
$ cc -O2 tp.c -DTEST='data->iin_buffer[0] & 0x7e'
$ time ./a.out
data->iin_buffer[0] & 0x7e fc000000

real    0m3.927s
user    0m3.920s
sys     0m0.000s
$ time ./a.out
data->iin_buffer[0] & 0x7e fc000000

real    0m3.925s
user    0m3.920s
sys     0m0.000s
$ cc -O2 tp.c -DTEST='(data->iin_buffer[0] & 0x7f) > 1'
$ time ./a.out
(data->iin_buffer[0] & 0x7f) > 1 fc000000

real    0m2.638s
user    0m2.610s
sys     0m0.000s
$ time ./a.out
(data->iin_buffer[0] & 0x7f) > 1 fc000000

real    0m2.648s
user    0m2.620s
sys     0m0.000s

Quoted 13 lines

Quoted 13 lines
Quoted 6 lines
drivers/usb/class/usbtmc.c static struct usb_class_driver usbtmc_class = { · line 1229
Quoted 4 lines
drivers/usb/class/usbtmc.c +4 -0
+               /* urb terminated, clean up */
+               dev_dbg(&data->intf->dev,
+                       "%s - urb terminated, status: %d\n",
+                       __func__, status);
No need to print function here explicitly. Check Dynamic Debug framework.
I am not using dynamic debug but when I enable static debug I get:
[ 1438.562458] usbtmc 1-1:1.0: Enter ioctl_read_stb iin_ep_present: 1
on the console log for

Quoted 9 lines

Quoted 4 lines
dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
                data->iin_ep_present);
So if I don't print the function it does not appear on the log.
Whatever maintainers prefer, though I think there are quite rare cases in USB when someone needs static debug. I'm pretty sure most of the developers all in favour of dynamic debug.

I am happy to remove the func

Quoted 11 lines

Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
        retcode = sysfs_create_group(&intf->dev.kobj, &data_attr_grp);

        retcode = usb_register_dev(intf, &usbtmc_class);
Hmm??? Unrelated to this patch, but notice that retcode is overridden here.
Signature · 2 lines
With Best Regards,
Andy Shevchenko

Andy Shevchenko

2015-11-25 20:38 UTC #6 lore raw

Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.

Quoted 48 lines · Dave Penkler

Quoted 22 lines · Andy Shevchenko
Quoted 21 lines · Dave Penkler
Quoted 8 lines · Andy Shevchenko
drivers/usb/class/usbtmc.c static struct usb_class_driver usbtmc_class = { · line 1206
Quoted 5 lines · Dave Penkler
drivers/usb/class/usbtmc.c +5 -0
+       switch (status) {
+       case 0: /* SUCCESS */
+               if (data->iin_buffer[0] & 0x80) {
+                       /* check for valid STB notification */
+                       if ((data->iin_buffer[0] & 0x7f) > 1) {
Quoted 4 lines
Despite your answer to my comment code is quite understandable even with & 0x7e. You already put comment line about this, you may add that you validate the value to be 127 >= value >= 2.
Yes it is quite understandable but it is less clear. I repeat my comment here:
When reading the spec and the code it is more obvious that here we are testing for the value in bits D6..D0 to be a valid iin_bTag return. (See Table 7 in the USBTMC-USB488 spec.)
What is your motivation for
Quoted 8 lines
Quoted 3 lines
if (data->iin_buffer[0] & 0x7e)
?
In non-optimized variant it will certainly generate less code. You may have check assembly code with -O2 and compare. I don't know if compiler is clever enough to do the same by itself.
I tested out both variants, and the explicit test is actually faster on by box:
$ cat tp.c
#include <stdlib.h>
#include <stdio.h>
#define xstr(s) str(s)
#define str(s) #s
main() {
        unsigned int v,s=0;
        struct recs {
                unsigned char *iin_buffer;
        } rec;
        struct recs *data = &rec;
        data->iin_buffer = (unsigned char *) malloc(8);
        for (v=1;v;v++) {

Quoted 1 line

                data->iin_buffer[0] = v & 0x7f;

This line makes test fragile.

Quoted 28 lines

                if (TEST)
                        s++;
        }
        printf("%s %x\n",xstr(TEST),s);
}
$ cc -O2 tp.c -DTEST='data->iin_buffer[0] & 0x7e'
$ time ./a.out
data->iin_buffer[0] & 0x7e fc000000

real    0m3.927s
user    0m3.920s
sys     0m0.000s
$ time ./a.out
data->iin_buffer[0] & 0x7e fc000000

real    0m3.925s
user    0m3.920s
sys     0m0.000s
$ cc -O2 tp.c -DTEST='(data->iin_buffer[0] & 0x7f) > 1'
$ time ./a.out
(data->iin_buffer[0] & 0x7f) > 1 fc000000

real    0m2.638s
user    0m2.610s
sys     0m0.000s
$ time ./a.out
(data->iin_buffer[0] & 0x7f) > 1 fc000000

Quoted 3 lines

real    0m2.648s
user    0m2.620s
sys     0m0.000s

Can you, please, check the assembly code in the real driver?
I can't do this right now, maybe tomorrow I will have few minutes to check that.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2015-11-25 20:46 UTC #19 lore raw

Re: [PATCH v5 5/5] Add ioctls to enable and disable local controls on an instrument

In reply to Dave Penkler

Quoted 5 lines · Dave Penkler

On Sun, Nov 22, 2015 at 12:36:53PM +0200, Andy Shevchenko wrote:
OK so now I have the same as in v4 again:

Quoted 8 lines

        if (rv < 0) {
                dev_err(dev, "simple usb_control_msg failed %d\n", rv);
                goto exit;
        } else if (rv != 1) {
                dev_warn(dev, "simple usb_control_msg returned %d\n", rv);
                rv = -EIO;
                goto exit;
        }

This looks correct. Sorry if I misled you, but I really asked about error code in the first place.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Dave Penkler

author

2015-11-28 11:55 UTC #7 lore raw

Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.

In reply to Andy Shevchenko

Quoted 49 lines · Andy Shevchenko

Quoted 47 lines · Dave Penkler
Quoted 22 lines · Andy Shevchenko
Quoted 21 lines · Dave Penkler
Quoted 8 lines · Andy Shevchenko
drivers/usb/class/usbtmc.c static struct usb_class_driver usbtmc_class = { · line 1206
Quoted 5 lines · Dave Penkler
drivers/usb/class/usbtmc.c +5 -0
+       switch (status) {
+       case 0: /* SUCCESS */
+               if (data->iin_buffer[0] & 0x80) {
+                       /* check for valid STB notification */
+                       if ((data->iin_buffer[0] & 0x7f) > 1) {
Quoted 4 lines
Despite your answer to my comment code is quite understandable even with & 0x7e. You already put comment line about this, you may add that you validate the value to be 127 >= value >= 2.
Yes it is quite understandable but it is less clear. I repeat my comment here:
When reading the spec and the code it is more obvious that here we are testing for the value in bits D6..D0 to be a valid iin_bTag return. (See Table 7 in the USBTMC-USB488 spec.)
What is your motivation for
Quoted 8 lines
Quoted 3 lines
if (data->iin_buffer[0] & 0x7e)
?
In non-optimized variant it will certainly generate less code. You may have check assembly code with -O2 and compare. I don't know if compiler is clever enough to do the same by itself.
I tested out both variants, and the explicit test is actually faster on by box:
$ cat tp.c
#include <stdlib.h>
#include <stdio.h>
#define xstr(s) str(s)
#define str(s) #s
main() {
        unsigned int v,s=0;
        struct recs {
                unsigned char *iin_buffer;
        } rec;
        struct recs *data = &rec;
        data->iin_buffer = (unsigned char *) malloc(8);
        for (v=1;v;v++) {

Quoted 3 lines

Quoted 1 line
                data->iin_buffer[0] = v & 0x7f;
This line makes test fragile.

You are right, ignore this test.

snip

Quoted 1 line

Can you, please, check the assembly code in the real driver?

Below are the generated assembly code fragments using the standard kernel makefile flags. The opcodes for the relevant instructions are in capital letters. Comments added to show correspondence with C code. Note that it is the combination of the two tests that must be considered:
6 instructions for the 0x7e version and 5 for the original.
Performance is the same so I guess we can stick with the original ?

#### Assembly for (data->iin_buffer[0] & 0x7e) version .L258:

	TESTB	$126, %dl  # if (data->iin_buffer[0] & 0x7e)  goto moveit
	JNE	.L260      
	MOVL	%edx, %eax # if ((data->iin_buffer[0] & 0x7f) == 1) goto tfasync
	ANDL	$127, %eax 
	CMPB	$1, %al
	JNE	.L234      # else goto .L234
tfasync	cmpq	$0, 168(%r12)
	je	.L237
	leaq	168(%r12), %rdi
	movl	$131073, %edx
	movl	$29, %esi
	call	kill_fasync
.L237:
	movl	$1, 84(%r12)

.L255:

[snip]

.L260:
moveit movb %dl, 35(%r12)

	movzbl	1(%rax), %eax
	movl	$1, 56(%r12)
	movb	%al, 36(%r12)
	jmp	.L255

#### Assembly for ((data->iin_buffer[0] & 0x7f) > 1) version .L258:

	MOVL	%edx, %ecx   # if ((data->iin_buffer[0] & 0x7f) > 1) goto moveit
	ANDL	$127, %ecx
	CMPB	$1, %cl
	JBE	.L235        # else goto .L235
moveit	movb	%dl, 35(%r12)
	movzbl	1(%rax), %eax
	movl	$1, 56(%r12)
	movb	%al, 36(%r12)

.L255:

[snip]

.L235:

	JNE	.L234    # if ((data->iin_buffer[0] & 0x7f) == 1) goto tfasync
                         # else goto .L234
tfasync	cmpq	$0, 168(%r12)
	je	.L237
	leaq	168(%r12), %rdi
	movl	$131073, %edx
	movl	$29, %esi
	call	kill_fasync
.L237:
	movl	$1, 84(%r12)
	jmp	.L255

Quoted 1 line

I can't do this right now, maybe tomorrow I will have few minutes to check that.

cheers,
-Dave

Andy Shevchenko

2015-11-28 14:57 UTC #8 lore raw

Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.

Quoted 6 lines · Dave Penkler

Quoted 5 lines · Andy Shevchenko
Quoted 4 lines · Dave Penkler
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Dave Penkler
Quoted 1 line · Andy Shevchenko
On Wed, Nov 18, 2015 at 10:37 AM, Dave Penkler <[email protected]> wrote:

Thank you for an update!

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Review
Quoted 5 lines
(quoted diff) +5 -0
+       switch (status) {
+       case 0: /* SUCCESS */
+               if (data->iin_buffer[0] & 0x80) {
+                       /* check for valid STB notification */
+                       if ((data->iin_buffer[0] & 0x7f) > 1) {

How can I miss that there are two conditionals in a sequence and moreover for the same data?!
That might explain the optimization done by compiler.

So, could it be transformed to simple one condition
if (data->iin_buffer[0] > 0x81 /* 129 */) {
?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Dave Penkler

author

2015-11-28 17:41 UTC #9 lore raw

Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.

Quoted 10 lines · Andy Shevchenko

Quoted 6 lines · Dave Penkler
Quoted 5 lines · Andy Shevchenko
Quoted 4 lines · Dave Penkler
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Dave Penkler
Quoted 1 line · Andy Shevchenko
On Wed, Nov 18, 2015 at 10:37 AM, Dave Penkler <[email protected]> wrote:
Thank you for an update!

Quoted 8 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Review
Quoted 5 lines
(quoted diff) +5 -0
+       switch (status) {
+       case 0: /* SUCCESS */
+               if (data->iin_buffer[0] & 0x80) {
+                       /* check for valid STB notification */
+                       if ((data->iin_buffer[0] & 0x7f) > 1) {
How can I miss that there are two conditionals in a sequence and moreover for the same data?!

Sorry, my fault, it is the combination of patch 1 and 2

Quoted 5 lines

That might explain the optimization done by compiler.
So, could it be transformed to simple one condition
if (data->iin_buffer[0] > 0x81 /* 129 */) {
?

OK so now for patch 1 and 2 we have:

	switch (status) {
	case 0: /* SUCCESS */
		/* PATCH 1 check for valid STB notification */
		if (data->iin_buffer[0] > 0x81) {
			data->bNotify1 = data->iin_buffer[0];
			data->bNotify2 = data->iin_buffer[1];
			atomic_set(&data->iin_data_valid, 1);
			wake_up_interruptible(&data->waitq);
			goto exit;
		}
		/* PATCH 2 check for SRQ notification */
		if (data->iin_buffer[0] == 0x81) {
			if (data->fasync)
				kill_fasync(&data->fasync,
					SIGIO, POLL_IN);

			atomic_set(&data->srq_asserted, 1);
			wake_up_interruptible(&data->waitq);
			goto exit;
		}

I'll push a new set if you are OK with this.
cheers,
-Dave

Andy Shevchenko

2015-11-28 19:18 UTC #10 lore raw

Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.

Quoted 52 lines · Dave Penkler

Quoted 10 lines · Andy Shevchenko
Quoted 6 lines · Dave Penkler
Quoted 5 lines · Andy Shevchenko
Quoted 4 lines · Dave Penkler
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Dave Penkler
Quoted 1 line · Andy Shevchenko
On Wed, Nov 18, 2015 at 10:37 AM, Dave Penkler <[email protected]> wrote:
Thank you for an update!
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Review
Quoted 5 lines
(quoted diff) +5 -0
+       switch (status) {
+       case 0: /* SUCCESS */
+               if (data->iin_buffer[0] & 0x80) {
+                       /* check for valid STB notification */
+                       if ((data->iin_buffer[0] & 0x7f) > 1) {
How can I miss that there are two conditionals in a sequence and moreover for the same data?!
Sorry, my fault, it is the combination of patch 1 and 2
Quoted 5 lines
That might explain the optimization done by compiler.
So, could it be transformed to simple one condition
if (data->iin_buffer[0] > 0x81 /* 129 */) {
?
OK so now for patch 1 and 2 we have:
        switch (status) {
        case 0: /* SUCCESS */
                /* PATCH 1 check for valid STB notification */
                if (data->iin_buffer[0] > 0x81) {
                        data->bNotify1 = data->iin_buffer[0];
                        data->bNotify2 = data->iin_buffer[1];
                        atomic_set(&data->iin_data_valid, 1);
                        wake_up_interruptible(&data->waitq);
                        goto exit;
                }
                /* PATCH 2 check for SRQ notification */
                if (data->iin_buffer[0] == 0x81) {
                        if (data->fasync)
                                kill_fasync(&data->fasync,
                                        SIGIO, POLL_IN);

                        atomic_set(&data->srq_asserted, 1);
                        wake_up_interruptible(&data->waitq);
                        goto exit;
                }

Quoted 1 line

I'll push a new set if you are OK with this.

Yes, fine by me, thanks for patience.

Signature · 2 lines

With Best Regards,
Andy Shevchenko