Re: [PATCH] USB: usbtmc: Add support for missing USBTMC-USB488 spec

[dave penkler]

Hi Oliver,

On Wed, Oct 14, 2015 at 3:33 PM, Oliver Neukum <oneukum@xxxxxxxx> wrote:
> On Wed, 2015-10-14 at 15:13 +0200, dave penkler wrote:
>
> Hi,
>
> just a few remarks.

thank you.

>
>>
>> +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 = &data->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(&data->iin_data_valid, 0);
>> +
>> +     /* must issue read_stb before using poll or select */
>> +     atomic_set(&data->srq_asserted, 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(&data->iin_data_valid) != 0),
>> +                     USBTMC_TIMEOUT
>> +                     );
>> +
>> +             if (rv < 0) {
>> +                     dev_dbg(dev, "wait interrupted %d\n", rv);
>> +                     goto exit;
>> +             }
>
> If you do this, yet the transfer succeeds, how do you keep the tag
> between host and device consistent?
>

The next message will just use the same tag value as before if rv <= 0
which is not a problem - one could do it either way. I'll move the bump
after exit: for V2

>> +
>> +             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];
>> +     }
>> +
>> +     /* bump interrupt bTag */
>> +     data->iin_bTag += 1;
>> +     if (data->iin_bTag > 127)
>> +             data->iin_bTag = 2;
>> +
>> +     rv = copy_to_user((void *)arg, &stb, sizeof(stb));
>> +     if (rv)
>> +             rv = -EFAULT;
>> +
>> + exit:
>> +     kfree(buffer);
>> +     return rv;
>> +
>> +}
>> +
>> +static unsigned int usbtmc_poll(struct file *file, poll_table *wait)
>> +{
>> +     struct usbtmc_device_data *data = file->private_data;
>> +     unsigned int mask = 0;
>> +
>> +     mutex_lock(&data->io_mutex);
>> +
>> +     if (data->zombie)
>> +             goto no_poll;
>> +
>> +     poll_wait(file, &data->waitq, wait);
>
> Presumably the waiters should be woken when the device is disconnected.
>

Yes the mask should be set to POLLHUP etc on the first zombie test above.
After the poll_wait it should simply read
          mask = (atomic_read(&data->srq_asserted)) ? POLLIN | POLLRDNORM : 0;
Will revise for V2
thank you

>> +
>> +     mask = data->zombie ? POLLHUP | POLLERR :
>> +             (atomic_read(&data->srq_asserted)) ? POLLIN | POLLRDNORM : 0;
>> +
>> +no_poll:
>> +     mutex_unlock(&data->io_mutex);
>> +     return mask;
>> +}
>
>         Regards
>                 Oliver
>
>
--
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/