From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S932238AbbJNNeo (ORCPT <rfc822;w@1wt.eu>);
	Wed, 14 Oct 2015 09:34:44 -0400
Received: from mx2.suse.de ([195.135.220.15]:53566 "EHLO mx2.suse.de"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753927AbbJNNem (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Wed, 14 Oct 2015 09:34:42 -0400
Message-ID: <1444829587.1975.13.camel@suse.com>
Subject: Re: [PATCH] USB: usbtmc: Add support for missing USBTMC-USB488 spec
From: Oliver Neukum <oneukum@suse.com>
To: dpenkler@gmail.com
Cc: gregkh@linuxfoundation.org, peter.chen@freescale.com, teuniz@gmail.com,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Date: Wed, 14 Oct 2015 15:33:07 +0200
In-Reply-To: <20151014131309.GB5410@slacky>
References: <20151014131309.GB5410@slacky>
Content-Type: text/plain; charset="UTF-8"
X-Mailer: Evolution 3.12.11 
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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