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

[Oliver Neukum <oneukum@suse.com>]

On Wed, 2015-10-14 at 15:13 +0200, dave penkler wrote:

Hi,

just a few remarks.

>  
> +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?

> +
> +		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.

> +
> +	mask = data->zombie ? POLLHUP | POLLERR :
> +		(atomic_read(&data->srq_asserted)) ? POLLIN | POLLRDNORM : 0;
> +
> +no_poll:
> +	mutex_unlock(&data->io_mutex);
> +	return mask;
> +}

	Regards
		Oliver