Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1272066
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. |
| Date | 2015-11-18 11:00 +0100 |
| Message-ID | <qw7GW-1aT-29@gated-at.bofh.it> (permalink) |
| References | <qw6rv-qW-5@gated-at.bofh.it> <qw6rv-qW-11@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
On Wed, Nov 18, 2015 at 10:37 AM, Dave Penkler <dpenkler@gmail.com> 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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH v5 0/5] usb: usbtmc: Add support for missing functions in USBTMC-USB488 spec Dave Penkler <dpenkler@gmail.com> - 2015-11-18 09:40 +0100
[PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. Dave Penkler <dpenkler@gmail.com> - 2015-11-18 09:40 +0100
Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-18 11:00 +0100
Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. Dave Penkler <dpenkler@gmail.com> - 2015-11-22 10:30 +0100
Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-22 11:40 +0100
Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. Dave Penkler <dpenkler@gmail.com> - 2015-11-25 10:20 +0100
Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-25 21:40 +0100
Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. Dave Penkler <dpenkler@gmail.com> - 2015-11-28 13:00 +0100
Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-28 16:00 +0100
Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. Dave Penkler <dpenkler@gmail.com> - 2015-11-28 18:50 +0100
Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-28 20:20 +0100
[PATCH v5 2/5] Add support for USBTMC USB488 SRQ notification with fasync Dave Penkler <dpenkler@gmail.com> - 2015-11-18 09:40 +0100
[PATCH v5 5/5] Add ioctls to enable and disable local controls on an instrument Dave Penkler <dpenkler@gmail.com> - 2015-11-18 09:40 +0100
Re: [PATCH v5 5/5] Add ioctls to enable and disable local controls on an instrument Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-18 10:50 +0100
Re: [PATCH v5 5/5] Add ioctls to enable and disable local controls on an instrument Dave Penkler <dpenkler@gmail.com> - 2015-11-22 10:00 +0100
Re: [PATCH v5 5/5] Add ioctls to enable and disable local controls on an instrument Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-22 11:40 +0100
Re: [PATCH v5 5/5] Add ioctls to enable and disable local controls on an instrument Dave Penkler <dpenkler@gmail.com> - 2015-11-25 10:20 +0100
Re: [PATCH v5 5/5] Add ioctls to enable and disable local controls on an instrument Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-25 21:50 +0100
[PATCH v5 4/5] Add ioctl to retrieve USBTMC-USB488 capabilities Dave Penkler <dpenkler@gmail.com> - 2015-11-18 09:40 +0100
csiph-web