[PATCH 0/4] USB: usbtmc: Fix stale status byte ioctl

[PATCH 0/4] USB: usbtmc: Fix stale status byte ioctl

[Dave Penkler]

The ioctl USBTMC488_IOCTL_READ_STB either returns a cached status byte
(STB) sent by the device due to a service request (SRQ) condition or
the STB obtained from a query to the device with a READ_STATUS_BYTE
control message.

When the query is interrupted by an SRQ message on the interrupt pipe,
the ioctl still returns the requested STB while the STB of the
out-of-band SRQ message is cached for the next call of this
ioctl. However the cached SRQ STB represents a state that was previous
to the last returned STB.  Furthermore the cached SRQ STB can be
stale and not reflect the current state of the device.

This set of patches separates out the behaviour into 3 ioctls:

[PATCH 1]
USBTMC488_IOCTL_READ_STB always reads the STB from the device and if the
associated file descriptor has the srq_asserted bit set it ors in the
RQS bit to the returned STB and clears the srq_asserted bit conformant
to subclass USB488 devices.

[PATCH 2]
USBTMC_IOCTL_GET_STB reads the status byte (STB) from the device and
returns the STB unmodified to the application. The srq_asserted bit is
not taken into account and not changed.

[PATCH 3]
USBTMC_IOCTL_GET_SRQ_STB only returns the status byte (STB) that was
originally sent by the device due to a service request (SRQ) condition.

This ioctl checks the srq_asserted bit of the associated file
descriptor. If set, the srq_asserted bit is reset and the cached
STB with original SRQ information is returned. Otherwise the ioctl
returns the error code ENOMSG.

The latter 2 ioctls are useful to support non USBTMC-488 compliant
devices. Time sensitive applications can read the cached STB without
incurring the cost of an urb transaction over the bus.

[PATCH 4]
Increase the API version number

Dave Penkler (4):
  USB: usbtmc: Fix reading stale status byte
  USB: usbtmc: Add USBTMC_IOCTL_GET_STB
  USB: usbtmc: Add separate USBTMC_IOCTL_GET_SRQ_STB
  USB: usbtmc: Bump USBTMC_API_VERSION value

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

-- 
2.29.2

[PATCH 1/4] USB: usbtmc: Fix reading stale status byte

[Dave Penkler]

The ioctl USBTMC488_IOCTL_READ_STB either returns a cached status byte
(STB) sent by the device due to a service request (SRQ) condition or
the STB obtained from a query to the device with a READ_STATUS_BYTE
control message.

When the query is interrupted by an SRQ message on the interrupt pipe,
the ioctl still returns the requested STB while the STB of the
out-of-band SRQ message is cached for the next call of this
ioctl. However the cached SRQ STB represents a state that was previous
to the last returned STB.  Furthermore the cached SRQ STB can be stale
and not reflect the current state of the device.

The fixed ioctl now always reads the STB from the device and if the
associated file descriptor has the srq_asserted bit set it ors in the
RQS bit to the returned STB and clears the srq_asserted bit conformant
to subclass USB488 devices.

Signed-off-by: Dave Penkler <dpenkler@gmail.com>
Reviewed-by: Guido Kiener <guido.kiener@rohde-schwarz.com>
Tested-by: Jian-Wei Wu <jian-wei_wu@keysight.com>
---
 drivers/usb/class/usbtmc.c | 46 +++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index b222b777e6a4..189f06dcb7d3 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -475,33 +475,17 @@ static int usbtmc_ioctl_abort_bulk_out(struct usbtmc_device_data *data)
 	return usbtmc_ioctl_abort_bulk_out_tag(data, data->bTag_last_write);
 }
 
-static int usbtmc488_ioctl_read_stb(struct usbtmc_file_data *file_data,
-				void __user *arg)
+static int usbtmc_get_stb(struct usbtmc_file_data *file_data, __u8 *stb)
 {
 	struct usbtmc_device_data *data = file_data->data;
 	struct device *dev = &data->intf->dev;
-	int srq_asserted = 0;
 	u8 *buffer;
 	u8 tag;
-	__u8 stb;
 	int rv;
 
 	dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
 		data->iin_ep_present);
 
-	spin_lock_irq(&data->dev_lock);
-	srq_asserted = atomic_xchg(&file_data->srq_asserted, srq_asserted);
-	if (srq_asserted) {
-		/* a STB with SRQ is already received */
-		stb = file_data->srq_byte;
-		spin_unlock_irq(&data->dev_lock);
-		rv = put_user(stb, (__u8 __user *)arg);
-		dev_dbg(dev, "stb:0x%02x with srq received %d\n",
-			(unsigned int)stb, rv);
-		return rv;
-	}
-	spin_unlock_irq(&data->dev_lock);
-
 	buffer = kmalloc(8, GFP_KERNEL);
 	if (!buffer)
 		return -ENOMEM;
@@ -548,13 +532,12 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_file_data *file_data,
 				data->iin_bTag, tag);
 		}
 
-		stb = data->bNotify2;
+		*stb = data->bNotify2;
 	} else {
-		stb = buffer[2];
+		*stb = buffer[2];
 	}
 
-	rv = put_user(stb, (__u8 __user *)arg);
-	dev_dbg(dev, "stb:0x%02x received %d\n", (unsigned int)stb, rv);
+	dev_dbg(dev, "stb:0x%02x received %d\n", (unsigned int)*stb, rv);
 
  exit:
 	/* bump interrupt bTag */
@@ -567,6 +550,27 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_file_data *file_data,
 	return rv;
 }
 
+static int usbtmc488_ioctl_read_stb(struct usbtmc_file_data *file_data,
+				void __user *arg)
+{
+	int srq_asserted = 0;
+	__u8 stb;
+	int rv;
+
+	rv = usbtmc_get_stb(file_data, &stb);
+
+	if (rv > 0) {
+		srq_asserted = atomic_xchg(&file_data->srq_asserted,
+					srq_asserted);
+		if (srq_asserted)
+			stb |= 0x40; /* Set RQS bit */
+
+		rv = put_user(stb, (__u8 __user *)arg);
+	}
+	return rv;
+
+}
+
 static int usbtmc488_ioctl_wait_srq(struct usbtmc_file_data *file_data,
 				    __u32 __user *arg)
 {
-- 
2.29.2

[PATCH 2/4] USB: usbtmc: Add USBTMC_IOCTL_GET_STB

[Dave Penkler]

This new ioctl reads the status byte (STB) from the device and returns
the STB unmodified to the application. The srq_asserted bit is not taken
into account and not changed.

This ioctl is useful to support non USBTMC-488 compliant devices.

Signed-off-by: Dave Penkler <dpenkler@gmail.com>
Reviewed-by: Guido Kiener <guido.kiener@rohde-schwarz.com>
Tested-by: Jian-Wei Wu <jian-wei_wu@keysight.com>
---
 drivers/usb/class/usbtmc.c   | 6 ++++++
 include/uapi/linux/usb/tmc.h | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 189f06dcb7d3..8918e2182eca 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -2149,6 +2149,12 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			file_data->auto_abort = !!tmp_byte;
 		break;
 
+	case USBTMC_IOCTL_GET_STB:
+		retval = usbtmc_get_stb(file_data, &tmp_byte);
+		if (retval > 0)
+			retval = put_user(tmp_byte, (__u8 __user *)arg);
+		break;
+
 	case USBTMC_IOCTL_CANCEL_IO:
 		retval = usbtmc_ioctl_cancel_io(file_data);
 		break;
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index fdd4d88a7b95..1e7878fe591f 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -102,6 +102,8 @@ struct usbtmc_message {
 #define USBTMC_IOCTL_MSG_IN_ATTR	_IOR(USBTMC_IOC_NR, 24, __u8)
 #define USBTMC_IOCTL_AUTO_ABORT		_IOW(USBTMC_IOC_NR, 25, __u8)
 
+#define USBTMC_IOCTL_GET_STB            _IOR(USBTMC_IOC_NR, 26, __u8)
+
 /* Cancel and cleanup asynchronous calls */
 #define USBTMC_IOCTL_CANCEL_IO		_IO(USBTMC_IOC_NR, 35)
 #define USBTMC_IOCTL_CLEANUP_IO		_IO(USBTMC_IOC_NR, 36)
-- 
2.29.2

[PATCH 3/4] USB: usbtmc: Add separate USBTMC_IOCTL_GET_SRQ_STB

[Dave Penkler]

This new ioctl only returns the status byte (STB) that was originally
sent by the device due to a service request (SRQ) condition.

This ioctl checks the srq_asserted bit of the associated file
descriptor. If set, the srq_asserted bit is reset and the cached
STB with original SRQ information is returned. Otherwise the ioctl
returns the error code ENOMSG.

This ioctl is useful to support non USBTMC-488 compliant devices.
Time sensitive applications can read the cached STB without incurring
the cost of an urb transaction over the bus.

Signed-off-by: Dave Penkler <dpenkler@gmail.com>
Reviewed-by: Guido Kiener <guido.kiener@rohde-schwarz.com>
Tested-by: Jian-Wei Wu <jian-wei_wu@keysight.com>
---
 drivers/usb/class/usbtmc.c   | 31 +++++++++++++++++++++++++++++++
 include/uapi/linux/usb/tmc.h |  1 +
 2 files changed, 32 insertions(+)

diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 8918e2182eca..d2fcc698c745 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -571,6 +571,32 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_file_data *file_data,
 
 }
 
+static int usbtmc_ioctl_get_srq_stb(struct usbtmc_file_data *file_data,
+				void __user *arg)
+{
+	struct usbtmc_device_data *data = file_data->data;
+	struct device *dev = &data->intf->dev;
+	int srq_asserted = 0;
+	__u8 stb = 0;
+	int rv;
+
+	spin_lock_irq(&data->dev_lock);
+	srq_asserted  = atomic_xchg(&file_data->srq_asserted, srq_asserted);
+
+	if (srq_asserted) {
+		stb = file_data->srq_byte;
+		spin_unlock_irq(&data->dev_lock);
+		rv = put_user(stb, (__u8 __user *)arg);
+	} else {
+		spin_unlock_irq(&data->dev_lock);
+		rv = -ENOMSG;
+	}
+
+	dev_dbg(dev, "stb:0x%02x with srq received %d\n", (unsigned int)stb, rv);
+
+	return rv;
+}
+
 static int usbtmc488_ioctl_wait_srq(struct usbtmc_file_data *file_data,
 				    __u32 __user *arg)
 {
@@ -2155,6 +2181,11 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			retval = put_user(tmp_byte, (__u8 __user *)arg);
 		break;
 
+	case USBTMC_IOCTL_GET_SRQ_STB:
+		retval = usbtmc_ioctl_get_srq_stb(file_data,
+						  (void __user *)arg);
+		break;
+
 	case USBTMC_IOCTL_CANCEL_IO:
 		retval = usbtmc_ioctl_cancel_io(file_data);
 		break;
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index 1e7878fe591f..d791cc58a7f0 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -103,6 +103,7 @@ struct usbtmc_message {
 #define USBTMC_IOCTL_AUTO_ABORT		_IOW(USBTMC_IOC_NR, 25, __u8)
 
 #define USBTMC_IOCTL_GET_STB            _IOR(USBTMC_IOC_NR, 26, __u8)
+#define USBTMC_IOCTL_GET_SRQ_STB        _IOR(USBTMC_IOC_NR, 27, __u8)
 
 /* Cancel and cleanup asynchronous calls */
 #define USBTMC_IOCTL_CANCEL_IO		_IO(USBTMC_IOC_NR, 35)
-- 
2.29.2

[PATCH 4/4] USB: usbtmc: Bump USBTMC_API_VERSION value

[Dave Penkler]

The previous patches in this series have changed the behaviour of the
driver and added new calls.

Signed-off-by: Dave Penkler <dpenkler@gmail.com>
Reviewed-by: Guido Kiener <guido.kiener@rohde-schwarz.com>
Tested-by: Jian-Wei Wu <jian-wei_wu@keysight.com>
---
 drivers/usb/class/usbtmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index d2fcc698c745..74d5a9c5238a 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -25,7 +25,7 @@
 /* Increment API VERSION when changing tmc.h with new flags or ioctls
  * or when changing a significant behavior of the driver.
  */
-#define USBTMC_API_VERSION (2)
+#define USBTMC_API_VERSION (3)
 
 #define USBTMC_HEADER_SIZE	12
 #define USBTMC_MINOR_BASE	176
-- 
2.29.2

Re: [PATCH 4/4] USB: usbtmc: Bump USBTMC_API_VERSION value

[Greg KH]

On Tue, Dec 15, 2020 at 04:56:21PM +0100, Dave Penkler wrote:
> The previous patches in this series have changed the behaviour of the
> driver and added new calls.
> 
> Signed-off-by: Dave Penkler <dpenkler@gmail.com>
> Reviewed-by: Guido Kiener <guido.kiener@rohde-schwarz.com>
> Tested-by: Jian-Wei Wu <jian-wei_wu@keysight.com>
> ---
>  drivers/usb/class/usbtmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> index d2fcc698c745..74d5a9c5238a 100644
> --- a/drivers/usb/class/usbtmc.c
> +++ b/drivers/usb/class/usbtmc.c
> @@ -25,7 +25,7 @@
>  /* Increment API VERSION when changing tmc.h with new flags or ioctls
>   * or when changing a significant behavior of the driver.
>   */
> -#define USBTMC_API_VERSION (2)
> +#define USBTMC_API_VERSION (3)

Why is this needed?  You should be able to detect new calls by just
doing the ioctl and checking right?

I'll take this for now, but versioning apis is not a good thing to do in
general.

thanks,

greg k-h