On Wed, Nov 18, 2015 at 11:55:27AM +0200, Andy Shevchenko wrote:
> 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.
>
OK, apologies.
snip
> > +
> > + 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)
?
> > + 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
> > + /* 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.
>
> > + 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
> > +
> > + /* 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.
> > + 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
cheers,
-Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
