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-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
