lkml.org 
[lkml]   [2012]   [May]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] add new NRP power meter USB device driver
Date
Am Dienstag, 29. Mai 2012, 21:14:18 schrieb stefani@seibold.net:

> The device will be controlled by a SCPI like interface.

Are there other devices using that standard whose interface you might reuse?


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

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

> + }
> + } else
> + n = urb->status;

You need to translate this to standard error codes
> +
> + 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?

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

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

> + case NRPZ_VENDOR_CONTROL_MSG_OUT:
> + {
> + struct nrpz_control_req ncr;

DMA on stack.

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

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

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

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

Regards
Oliver


\
 
 \ /
  Last update: 2012-05-30 11:01    [W:0.475 / U:0.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site