Date:	Thu, 31 May 2012 09:43:41 +0200
From:	Stefani Seibold <stefani@...bold.net>
To:	Oliver Neukum <oneukum@...e.de>
Cc:	linux-kernel@...r.kernel.org, greg@...ah.com,
	gregkh@...uxfoundation.org,
	thomas.braunstorfinger@...de-schwarz.com
Subject: Re: [PATCH] add new NRP power meter USB device driver

Hi Oliver,

Thanks for the review.

On Wed, 2012-05-30 at 10:10 +0200, Oliver Neukum wrote:
> Am Dienstag, 29. Mai 2012, 21:14:18 schrieb stefani@...bold.net:
>  
> > The device will be controlled by a SCPI like interface.
> 
> Are there other devices using that standard whose interface you might reuse?
>  
> 

There are a lot of instruments from many different manufacturer which
use the SCPI standard. But there is now way to bring them together. The
commands are well defined, but there are a lot of vendor specific
commands. And the communication path are differing, which can be
IEEE488, USB, Socket, Serial and more. 

For more information see
https://en.wikipedia.org/wiki/Standard_Commands_for_Programmable_Instruments

> > +static ssize_t nrpz_read(struct file *file, char __user *buffer, size_t count,
> > +				loff_t *ppos)
> > +{
> > +	struct usb_nrpz *dev;
> > +	int ret;
> > +	struct urb *urb;
> > +	size_t n;
> > +
> > +	dev = (struct usb_nrpz *)file->private_data;
> > +
> > +	/* verify that we actually have some data to read */
> > +	if (!count)
> > +		return 0;
> > +
> > +	/* lock the read data */
> > +	ret = mutex_lock_interruptible(&dev->read_mutex);
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (;;) {
> > +		urb = urb_list_get(&dev->read_lock, &dev->in_avail);
> > +		if (urb)
> > +			break;
> > +
> > +		if (file->f_flags & O_NONBLOCK) {
> > +			ret = -EAGAIN;
> > +			goto exit;
> > +		}
> 
> You should test for device disconnect before that. Otherwise
> a nonblocking reader would never learn of the problem. IIRC
> the skeleton has been fixed.
> 

I did this 10 lines later, i will move this check before the O_NONBLOCK
check.

> > +
> > +		ret = wait_event_interruptible(dev->wq,
> > +			!list_empty(&dev->in_avail) || !dev->intf);
> > +		if (ret) {
> > +			ret = -ERESTARTSYS;
> > +			goto exit;
> > +		}
> > +
> > +		/* verify that the device wasn't unplugged */
> > +		if (!dev->intf) {
> > +			ret = -ENODEV;
> > +			goto exit;
> > +		}
> > +	}
> > +
> > +	if (!urb->status) {
> > +		n = min(count, urb->actual_length);
> > +
> > +		if (copy_to_user(buffer, urb->transfer_buffer, n)) {
> > +			urb_list_add(&dev->read_lock, urb, &dev->in_avail);
> > +			ret = -EFAULT;
> > +			goto exit;
> 
> Is there a good reason you don't resubmit in this case?
> 

The data was not successfully transfered to the user space, so it will
stay again on the top of the input urb list.
 
> > +		}
> > +	} else
> > +		n = urb->status;
> 
> You need to translate this to standard error codes

I will report -EPIPE in the next version of the driver.

> > +
> > +	usb_anchor_urb(urb, &dev->in_running);
> > +
> > +	ret = usb_submit_urb(urb, GFP_KERNEL);
> > +	if (ret) {
> > +		usb_unanchor_urb(urb);
> > +		urb_list_add_tail(&dev->read_lock, urb, &dev->in_avail);
> > +		nrpz_err("Failed submitting read urb (error %d)", ret);
> > +	}
> > +
> > +	ret = n;
> > +exit:
> > +	mutex_unlock(&dev->read_mutex);
> > +	return ret;
> > +}
> 
> > +#define	VRT_RESET_ALL		1
> > +#define	VRT_GET_DEVICE_INFO	6
> > +#define	VRI_DEVICE_NAME		5
> > +
> > +static long nrpz_compat_ioctl(struct file *file, unsigned int cmd,
> > +		unsigned long arg)
> > +{
> > +	struct usb_nrpz *dev;
> > +	int ret;
> > +
> > +	dev = (struct usb_nrpz *)file->private_data;
> > +
> > +	/* verify that the device wasn't unplugged */
> > +	if (!dev->intf)
> > +		return -ENODEV;
> 
> Locking?
> 

I see no reason why. But i will change it in the next version.

> > +
> > +	switch (cmd) {
> > +	case NRPZ_GETSENSORINFO:
> > +	 {
> > +		struct nrpz_sensor_info __user *sensor_info =
> > +			(struct nrpz_sensor_info __user *)arg;
> > +
> > +		if (!access_ok(VERIFY_WRITE, sensor_info, sizeof(*sensor_info)))
> > +			return -EFAULT;
> > +
> > +		__put_user(dev->udev->descriptor.bcdDevice,
> > +				&sensor_info->bcdDevice);
> > +		__put_user(dev->udev->descriptor.bcdUSB,
> > +				&sensor_info->bcdUSB);
> > +		__put_user(dev->udev->descriptor.bDescriptorType,
> > +				&sensor_info->bDescriptorType);
> > +		__put_user(dev->udev->descriptor.bDeviceClass,
> > +				&sensor_info->bDeviceClass);
> > +		__put_user(dev->udev->descriptor.bDeviceSubClass,
> > +				&sensor_info->bDeviceSubClass);
> > +		__put_user(dev->udev->descriptor.bDeviceProtocol,
> > +				&sensor_info->bDeviceProtocol);
> > +		__put_user(dev->udev->descriptor.bMaxPacketSize0,
> > +				&sensor_info->bMaxPacketSize0);
> > +		__put_user(dev->udev->descriptor.bNumConfigurations,
> > +				&sensor_info->bNumConfigurations);
> > +		__put_user(dev->udev->descriptor.iManufacturer,
> > +				&sensor_info->iManufacturer);
> > +		__put_user(dev->udev->descriptor.iProduct,
> > +				&sensor_info->iProduct);
> > +		__put_user(dev->udev->descriptor.iSerialNumber,
> > +				&sensor_info->iSerialNumber);
> > +		__put_user(dev->udev->descriptor.idVendor,
> > +				&sensor_info->vendorId);
> > +		__put_user(dev->udev->descriptor.idProduct,
> > +				&sensor_info->productId);
> > +		usb_string(dev->udev, dev->udev->descriptor.iManufacturer,
> > +				(char __force *)sensor_info->manufacturer,
> > +				sizeof(sensor_info->manufacturer));
> > +		usb_string(dev->udev, dev->udev->descriptor.iProduct,
> > +				(char __force *)sensor_info->productName,
> > +				sizeof(sensor_info->productName));
> > +		usb_string(dev->udev, dev->udev->descriptor.iSerialNumber,
> > +				(char __force *)sensor_info->serialNumber,
> > +				sizeof(sensor_info->serialNumber));
> > +
> > +		return 0;
> > +	 }
> > +	case NRPZ_START:
> > +	 {
> > +		u8 device_state[128];
> 
> DMA on stack. This may ail on some architectures. You need to use
> kmalloc() or better kzalloc().
> 

Never heard about this problem. The driver will work since years on X86,
PPC and ARM platforms. But it will be fixes in the next version.

> > +		memset(device_state, 0, sizeof(device_state));
> > +
> > +		ret = usb_control_msg(dev->udev,
> > +			usb_rcvctrlpipe(dev->udev, 0),
> > +			VRT_GET_DEVICE_INFO,
> > +			USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> > +			0,
> > +			VRI_DEVICE_NAME,
> > +			device_state,
> > +			sizeof(device_state),
> > +			5000);
> > +
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		nrpz_dbg("device state:%s", device_state);
> > +
> > +		if (strncmp(device_state, "Boot ", 5))
> > +			return 0;
> > +
> > +		return usb_control_msg(dev->udev,
> > +			usb_sndctrlpipe(dev->udev, 0),
> > +			VRT_RESET_ALL,
> > +			USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> > +			1,
> > +			1,
> > +			device_state,
> > +			sizeof(device_state),
> > +			5000);
> > +	 }
> > +	case NRPZ_WRITE_DONE:
> 
> EEEEEEK!
> 
> > +		if (arg) {
> > +			ret = wait_event_interruptible_timeout(
> > +				dev->out_running.wait,
> > +				list_empty(&dev->out_running.urb_list),
> > +				msecs_to_jiffies(arg));
> > +			if (!ret)
> > +				return -ETIMEDOUT;
> > +			if (ret < 0)
> > +				return ret;
> > +			return 0;
> > +		} else {
> > +			return wait_event_interruptible(
> > +				dev->out_running.wait,
> > +				list_empty(&dev->out_running.urb_list));
> > +		}
> > +		break;
> 
> This is very ugly. If you need fsync(), then implement it.
> 

fsync() did not meat the requirements, since i need in some case a
timeout for the device. poll() will also not help, since it signals only
that there is space to write.

> > +	case NRPZ_VENDOR_CONTROL_MSG_OUT:
> > +	 {
> > +		struct nrpz_control_req ncr;
> 
> DMA on stack.
> 

Fixed in the next version.

> > +		u16 size;
> > +
> > +		if (copy_from_user(&ncr, (struct nrpz_control_req __user *)arg, sizeof(ncr)))
> > +			return -EFAULT;
> > +
> > +		if (ncr.data) {
> > +			size = ncr.size;
> > +
> > +			if (!access_ok(VERIFY_WRITE, (void __user *)ncr.data, size))
> > +				return -EFAULT;
> > +		} else {
> > +			size = 0;
> > +		}
> > +
> > +		ret = usb_control_msg(dev->udev,
> > +				usb_sndctrlpipe(dev->udev, 0),
> > +				ncr.request,
> > +				ncr.type,
> > +				ncr.value,
> > +				ncr.index,
> > +				ncr.data,
> > +				size,
> > +				0);
> > +
> > +		return ret;
> > +	 }
> > +	case NRPZ_VENDOR_CONTROL_MSG_IN:
> > +	 {
> > +		struct nrpz_control_req ncr;
> 
> DMA on stack
> 

Fixed in the next version.

> > +		u16 size;
> > +
> > +		if (copy_from_user(&ncr, (struct nrpz_control_req __user *)arg, sizeof(ncr)))
> > +			return -EFAULT;
> > +
> > +		if (ncr.data) {
> > +			size = ncr.size;
> > +
> > +			if (!access_ok(VERIFY_READ, (void __user *)ncr.data, size))
> > +				return -EFAULT;
> > +		} else {
> > +			size = 0;
> > +		}
> > +
> > +		ret = usb_control_msg(dev->udev,
> > +				usb_rcvctrlpipe(dev->udev, 0),
> > +				ncr.request,
> > +				ncr.type,
> > +				ncr.value,
> > +				ncr.index,
> > +				ncr.data,
> > +				size,
> > +				0);
> > +
> > +		return ret;
> > +	 }
> > +	default:
> > +		nrpz_err("Invalid ioctl call (%08x)", cmd);
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return ret;
> > +}
> 
> > +static int nrpz_open(struct inode *inode, struct file *file)
> > +{
> > +	struct usb_nrpz *dev;
> > +	int minor;
> > +	struct usb_interface *intf;
> > +	int ret;
> > +	unsigned i;
> > +
> > +	minor = iminor(inode);
> > +
> > +	intf = usb_find_interface(&nrpz_driver, minor);
> > +	if (!intf) {
> > +		nrpz_err("Can not find device for minor %d", minor);
> > +		return -ENODEV;
> > +	}
> > +
> > +	dev = usb_get_intfdata(intf);
> > +	if (!dev)
> > +		return -ENODEV;
> > +
> > +	if (test_and_set_bit(0, &dev->in_use))
> > +		return -EBUSY;
> > +
> > +	/* increment our usage count for the device */
> > +	kref_get(&dev->kref);
> > +
> > +	/* save our object in the file's private structure */
> > +	file->private_data = dev;
> > +
> > +	INIT_LIST_HEAD(&dev->in_avail);
> > +	INIT_LIST_HEAD(&dev->out_avail);
> > +
> > +	ret = bulks_init(dev,
> > +			 dev->in_urbs,
> > +			 ARRAY_SIZE(dev->in_urbs),
> > +			 usb_rcvbulkpipe(dev->udev, dev->in_epAddr),
> > +			 dev->in_size,
> > +			 nrpz_read_callback);
> > +	if (ret)
> > +		goto error;
> > +
> > +	ret = bulks_init(dev,
> > +			 dev->out_urbs,
> > +			 ARRAY_SIZE(dev->out_urbs),
> > +			 usb_sndbulkpipe(dev->udev, dev->out_epAddr),
> > +			 dev->out_size,
> > +			 nrpz_write_callback);
> > +	if (ret)
> > +		goto error;
> > +
> > +	for (i = 0; i != ARRAY_SIZE(dev->in_urbs); ++i) {
> > +		usb_anchor_urb(&dev->in_urbs[i], &dev->in_running);
> > +
> > +		ret = usb_submit_urb(&dev->in_urbs[i], GFP_KERNEL);
> 
> You do this here and only here. So this will see a slow attrition if you don't resubmit for som reason.
> 

No, i resubmit the urb in the nrpz_read() function.

> > +		if (ret) {
> > +			usb_kill_anchored_urbs(&dev->in_running);
> > +			goto error;
> > +		}
> > +	}
> > +
> > +	for (i = 0; i != ARRAY_SIZE(dev->out_urbs); ++i)
> > +		list_add(&dev->out_urbs[i].urb_list, &dev->out_avail);
> > +
> > +	return 0;
> > +error:
> > +	clear_bit(0, &dev->in_use);
> > +
> > +	bulks_release(dev, dev->out_urbs, ARRAY_SIZE(dev->out_urbs),
> > +			dev->out_size);
> > +	bulks_release(dev, dev->in_urbs, ARRAY_SIZE(dev->in_urbs),
> > +			dev->in_size);
> > +
> > +	return 0;
> > +}
> > +
> > +static int nrpz_release(struct inode *inode, struct file *file)
> > +{
> > +	struct usb_nrpz *dev;
> > +
> > +	dev = (struct usb_nrpz *)file->private_data;
> > +	if (dev == NULL)
> > +		return -ENODEV;
> > +
> > +	usb_kill_anchored_urbs(&dev->in_running);
> > +	usb_kill_anchored_urbs(&dev->out_running);
> > +
> > +	bulks_release(dev, dev->out_urbs, ARRAY_SIZE(dev->out_urbs),
> > +			dev->out_size);
> > +	bulks_release(dev, dev->in_urbs, ARRAY_SIZE(dev->in_urbs),
> > +			dev->in_size);
> > +
> > +	/* decrement the count on our device */
> > +	kref_put(&dev->kref, nrpz_delete);
> > +
> > +	clear_bit(0, &dev->in_use);
> > +
> > +	return 0;
> > +}
> > +
> > +static int nrpz_flush(struct file *file, fl_owner_t id)
> > +{
> > +	struct usb_nrpz *dev;
> > +	int ret;
> > +
> > +	dev = (struct usb_nrpz *)file->private_data;
> > +	if (dev == NULL)
> > +		return -ENODEV;
> > +
> > +	/* lock the write data */
> > +	ret = mutex_lock_interruptible(&dev->write_mutex);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* verify that the device wasn't unplugged */
> > +	if (!dev->intf) {
> > +		ret = -ENODEV;
> > +		goto exit;
> > +	}
> > +
> > +	ret = wait_event_interruptible(dev->out_running.wait,
> > +				list_empty(&dev->out_running.urb_list));
> > +	if (ret) {
> > +		ret = -ERESTARTSYS;
> > +		goto exit;
> > +	}
> > +exit:
> > +	mutex_unlock(&dev->write_mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +static unsigned int nrpz_poll(struct file *file, poll_table *wait)
> > +{
> > +	struct usb_nrpz *dev;
> > +	int ret = 0;
> > +
> > +	dev = (struct usb_nrpz *)file->private_data;
> > +
> > +	poll_wait(file, &dev->wq, wait);
> > +
> > +	if (!dev->intf)
> > +		ret = POLLIN | POLLOUT | POLLPRI | POLLERR | POLLHUP;
> > +	else {
> > +		if (!list_empty(&dev->in_avail))
> > +			ret |= POLLIN;
> > +
> > +		if (!list_empty(&dev->out_avail))
> > +			ret |= POLLOUT;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct file_operations nrpz_fops = {
> > +	.owner = THIS_MODULE,
> > +	.read = nrpz_read,
> > +	.write = nrpz_write,
> > +	.unlocked_ioctl = nrpz_ioctl,
> > +	.compat_ioctl = nrpz_compat_ioctl,
> > +	.open = nrpz_open,
> > +	.release = nrpz_release,
> > +	.flush = nrpz_flush,
> > +	.poll = nrpz_poll,
> > +	.llseek = noop_llseek,
> > +};
> > +
> > +static struct usb_class_driver nrpz_class = {
> > +	.name = "nrpz%d",
> > +	.fops = &nrpz_fops,
> > +	.minor_base = NRPZ_MINOR_BASE,
> > +};
> > +
> > +static int nrpz_probe(struct usb_interface *intf,
> > +			const struct usb_device_id *id)
> > +{
> > +	int i;
> > +	int ret;
> > +	struct usb_endpoint_descriptor *endpoint;
> > +	struct usb_host_interface *iface_desc;
> > +	struct usb_nrpz *dev;
> > +
> > +	/* allocate memory for our device state and intialize it */
> > +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > +	if (!dev) {
> > +		nrpz_err("Out of memory");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	ret = -EIO;
> > +
> > +	init_waitqueue_head(&dev->wq);
> > +	kref_init(&dev->kref);
> > +
> > +	mutex_init(&dev->read_mutex);
> > +	mutex_init(&dev->write_mutex);
> > +
> > +	spin_lock_init(&dev->read_lock);
> > +	spin_lock_init(&dev->write_lock);
> > +
> > +	init_usb_anchor(&dev->in_running);
> > +	init_usb_anchor(&dev->out_running);
> > +
> > +	dev->in_use = 0;
> > +	dev->udev = usb_get_dev(interface_to_usbdev(intf));
> > +	dev->intf = intf;
> > +
> > +	/* set up the endpoint information */
> > +	/* check out the endpoints */
> > +	iface_desc = intf->cur_altsetting;
> > +	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
> > +		endpoint = &iface_desc->endpoint[i].desc;
> > +
> > +		if (!dev->in_epAddr && usb_endpoint_is_bulk_in(endpoint)) {
> > +			/* we found a bulk in endpoint */
> > +			dev->in_size = le16_to_cpu(endpoint->wMaxPacketSize);
> > +			dev->in_epAddr = endpoint->bEndpointAddress;
> > +		} else
> > +		if (!dev->out_epAddr && usb_endpoint_is_bulk_out(endpoint)) {
> > +			/* we found a bulk out endpoint */
> > +			dev->out_size = le16_to_cpu(endpoint->wMaxPacketSize);
> > +			dev->out_epAddr = endpoint->bEndpointAddress;
> > +		}
> > +	}
> > +	if (!(dev->in_epAddr && dev->out_epAddr)) {
> > +		nrpz_err("Could not find both bulk in and out endpoints");
> > +		goto error;
> > +	}
> > +
> > +	usb_set_intfdata(intf, dev);
> > +
> > +	ret = usb_register_dev(intf, &nrpz_class);
> > +	if (ret) {
> > +		nrpz_err("Not able to get a minor for this device\n");
> > +		goto error;
> > +	}
> > +
> > +	dev->minor = intf->minor - NRPZ_MINOR_BASE;
> > +
> > +	/* let the user know what node this device is now attached to */
> > +	nrpz_info("Device now attached to USB nrpz%u", dev->minor);
> > +
> > +	return 0;
> > +error:
> > +	usb_set_intfdata(intf, NULL);
> > +	nrpz_delete(&dev->kref);
> > +	return ret;
> > +}
> > +
> > +static void nrpz_disconnect(struct usb_interface *intf)
> > +{
> > +	struct usb_nrpz *dev;
> > +	unsigned minor;
> > +
> > +	dev = usb_get_intfdata(intf);
> > +	usb_set_intfdata(intf, NULL);
> > +
> > +	minor = dev->minor;
> > +
> > +	/* give back our minor */
> > +	usb_deregister_dev(intf, &nrpz_class);
> > +
> > +	/* prevent more I/O from starting */
> > +	dev->intf = NULL;
> > +
> > +	usb_kill_anchored_urbs(&dev->in_running);
> > +	usb_kill_anchored_urbs(&dev->out_running);
> > +
> > +	wake_up_all(&dev->wq);
> > +
> > +	/* decrement our usage count */
> > +	kref_put(&dev->kref, nrpz_delete);
> > +
> > +	nrpz_info("nrpz%u now disconnected", minor);
> > +}
> > +
> > +static void nrpz_draw_down(struct usb_nrpz *dev)
> > +{
> > +	int time;
> > +
> > +	time = usb_wait_anchor_empty_timeout(&dev->out_running, 1000);
> > +	if (!time)
> > +		usb_kill_anchored_urbs(&dev->out_running);
> > +
> > +	usb_kill_anchored_urbs(&dev->in_running);
> > +}
> > +
> > +static int nrpz_suspend(struct usb_interface *intf, pm_message_t message)
> > +{
> > +	struct usb_nrpz *dev = usb_get_intfdata(intf);
> > +
> > +	if (dev)
> > +		nrpz_draw_down(dev);
> > +	return 0;
> > +}
> > +
> > +static int nrpz_resume(struct usb_interface *intf)
> > +{
> > +	return 0;
> > +}
> 
> And what restarts reading?
> 

Fixed in the next version.

> > +
> > +static int nrpz_pre_reset(struct usb_interface *intf)
> > +{
> > +	struct usb_nrpz *dev = usb_get_intfdata(intf);
> > +
> > +	if (dev)
> > +		nrpz_draw_down(dev);
> > +	return 0;
> > +}
> > +
> > +static int nrpz_post_reset(struct usb_interface *intf)
> > +{
> > +	return 0;
> > +}
> 
> And you don't tell user space that the device has been reset?
> And what restarts reading?
> 

I have no idea how to do this. But i will have a look in the kernel
source and figure it out.

Thanks again and best regards,
Stefani


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists