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

[Andy Shevchenko]

On Wed, Nov 18, 2015 at 10:37 AM, Dave Penkler <dpenkler@xxxxxxxxx> 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.
>

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.

[]

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

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

Redundant empty line.

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

Ditto.

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

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

Missed space before asterisk.

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

[]

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

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.

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

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

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

Does it return a proper error code?

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


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


-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/