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

[Dave Penkler]

Hi Andy,
On Sun, Nov 15, 2015 at 10:04:10PM +0200, Andy Shevchenko wrote:
> On Sun, Nov 15, 2015 at 8:39 PM, Dave Penkler  wrote:

snip

> > +
> 
> Redundant empty line.
> 

ok

> 
> > +   data->iin_bTag = 2;
> 
> Hmm??? Why 2?
> A-ha, below I found a comment. Something might be good to have here as well.
> 

Added comment

> > +
> 
> Redundant empty line.
> 

ok

> > +
> > +   if (data->iin_buffer[0] & 0x80) {
> > +   /* check for valid STB notification */
> > +   if ((data->iin_buffer[0] & 0x7f) > 1) {
> 
> It's the same as
>  if (data->iin_buffer[0] & 0x7e) {
> 

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

> > +   dev_dbg(>intf->dev,
> > +   "%s - urb terminated, status: %d\n",
> 
> I heard that dynamic debug adds function name.
> 
> > +   __func__, status);

I checked in device.h and could not find any trace of it. I'm on 4.4.0-rc1.

> > +static void usbtmc_free_int(struct usbtmc_device_data *data)
> > +{
> > +   if (data->iin_ep_present) {
> > +   if (data->iin_urb) {
> 
> Why not
> 
> if (!data->iin_ep_present || !data->iin_urb)
>   return;
> 
> ?
> 

OK

Thanks,
-Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andy Shevchenko]

On Sun, Nov 15, 2015 at 8:39 PM, Dave Penkler  wrote:
> 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.
>

Few nitpicks below.

> Signed-off-by: Dave Penkler 
> ---
>  drivers/usb/class/usbtmc.c   | 219 
> +++
>  include/uapi/linux/usb/tmc.h |   2 +
>  2 files changed, 221 insertions(+)
>
> diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> index 7a11a82..72ef7f0 100644
> --- a/drivers/usb/class/usbtmc.c
> +++ b/drivers/usb/class/usbtmc.c
> @@ -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;
> +   u16ifnum;
> +   u8 iin_bTag;
> +   u8*iin_buffer;
> +   atomic_t   iin_data_valid;
> +   unsigned int   iin_ep;
> +   intiin_ep_present;
> +   intiin_interval;
> +   struct urb*iin_urb;
> +   u16iin_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,88 @@ exit:
> return rv;
>  }
>
> +static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
> +   unsigned long arg)
> +{
> +   u8 *buffer;
> +   struct device *dev;
> +   int rv;
> +   u8 tag, stb;
> +
> +   dev = >intf->dev;

it could be struct device *dev = …;

> +
> +   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(>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) {
> +

Redundant empty line.

> +   rv = wait_event_interruptible_timeout(
> +   data->waitq,
> +   atomic_read(>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, , sizeof(stb));
> +   if (rv)
> +   rv = -EFAULT;
> +
> + exit:
> +   /* bump interrupt bTag */
> +   data->iin_bTag += 1;
> +   if (data->iin_bTag > 127)

> +   data->iin_bTag = 2;

Hmm… Why 2?
A-ha, below I found a comment. Something might be good to have here as well.

> +
> +   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 +1165,11 @@ static long usbtmc_ioctl(struct file *file, unsigned 
> int cmd, unsigned long arg)
> 

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

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

Signed-off-by: Dave Penkler 
---
 drivers/usb/class/usbtmc.c   | 219 +++
 include/uapi/linux/usb/tmc.h |   2 +
 2 files changed, 221 insertions(+)

diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 7a11a82..72ef7f0 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -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;
+   u16ifnum;
+   u8 iin_bTag;
+   u8*iin_buffer;
+   atomic_t   iin_data_valid;
+   unsigned int   iin_ep;
+   intiin_ep_present;
+   intiin_interval;
+   struct urb*iin_urb;
+   u16iin_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,88 @@ exit:
return rv;
 }
 
+static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
+   unsigned long arg)
+{
+   u8 *buffer;
+   struct device *dev;
+   int rv;
+   u8 tag, stb;
+
+   dev = >intf->dev;
+
+   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(>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(>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, , sizeof(stb));
+   if (rv)
+   rv = -EFAULT;
+
+ exit:
+   /* bump interrupt bTag */
+   data->iin_bTag += 1;
+   if (data->iin_bTag > 127)
+   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 +1165,11 @@ 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 +1193,69 @@ 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