From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751732AbbKVJUF (ORCPT ); Sun, 22 Nov 2015 04:20:05 -0500 Received: from mail-wm0-f48.google.com ([74.125.82.48]:37896 "EHLO mail-wm0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751165AbbKVJT7 (ORCPT ); Sun, 22 Nov 2015 04:19:59 -0500 Date: Sun, 22 Nov 2015 10:19:52 +0100 From: Dave Penkler To: Andy Shevchenko Cc: Greg Kroah-Hartman , peter.chen@freescale.com, teuniz@gmail.com, USB , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. Message-ID: <20151122091952.GB4581@slacky> References: <20151118083216.GA1845@slacky> <20151118083750.GA1945@slacky> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Nov 18, 2015 at 11:55:27AM +0200, Andy Shevchenko wrote: > On Wed, Nov 18, 2015 at 10:37 AM, 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. > > > > 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