lkml.org 
[lkml]   [2012]   [Jun]   [2]   [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 Samstag, 2. Juni 2012, 18:18:59 schrieb stefani@seibold.net:
    > From: Stefani Seibold <stefani@seibold.net>
    >
    > This driver supports all of the Rohde&Schwarz RF Power Meter NRP Sensors. These
    > sensors are intelligent standalone instruments that communicate via USB.

    Hi,

    I am commenting here only on the code as such, not on whether it should be
    included.


    > +static int bulks_init(struct usb_nrpz *dev,
    > + struct usb_device *udev,
    > + struct urb *urb,
    > + unsigned n,
    > + unsigned int pipe,
    > + int buf_size,
    > + usb_complete_t complete_fn)
    > +{
    > + void *buffer;
    > +
    > + while (n--) {
    > + usb_init_urb(urb);
    > +
    > + buffer = usb_alloc_coherent(udev,
    > + buf_size,
    > + GFP_KERNEL,
    > + &urb->transfer_dma);
    > + if (!buffer)
    > + return -ENOMEM;
    > +
    > + /* set up our read urb */
    > + usb_fill_bulk_urb(urb,
    > + udev,
    > + pipe,
    > + buffer,
    > + buf_size,
    > + complete_fn,
    > + dev);
    > +
    > + urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;

    I'd prefer |= in case the fill macros set a flag.

    > +
    > + ++urb;
    > + }
    > + return 0;
    > +}
    > +
    > +static int bulks_in_submit(struct usb_nrpz *dev)
    > +{
    > + int ret;
    > + unsigned i;
    > +
    > + 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);
    > + if (ret) {
    > + usb_kill_anchored_urbs(&dev->in_running);
    > + return ret;
    > + }
    > + }
    > + return 0;
    > +}

    This is used in the resume code path. During resume you
    cannot swap, as the swap device may still be asleep.
    Therefore GFP_NOIO should be used. It's considered
    best practice to pass the gfp parameter to such methods.

    > +static ssize_t nrpz_read(struct file *file, char __user *buffer, size_t count,
    > + loff_t *ppos)
    > +{
    > + struct usb_nrpz *dev = file->private_data;
    > + int ret;
    > + struct urb *urb;
    > + size_t n;
    > +
    > + /* verify that we actually have some data to read */
    > + if (!count)
    > + return 0;
    > +
    > + /* lock the read data */
    > + if (file->f_flags & O_NONBLOCK) {
    > + if (!mutex_trylock(&dev->read_mutex))
    > + return -EAGAIN;

    Congratulations. I overlooked this. Does skeleton do it right?

    > + } else {
    > + 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;
    > +
    > + /* verify that the device wasn't unplugged */
    > + if (!dev->connected) {
    > + ret = -ENODEV;
    > + goto exit;
    > + }
    > +
    > + if (file->f_flags & O_NONBLOCK) {
    > + ret = -EAGAIN;
    > + goto exit;
    > + }
    > +
    > + ret = wait_event_interruptible(dev->wq,
    > + !list_empty(&dev->in_avail) || !dev->connected);
    > + if (ret) {
    > + ret = -ERESTARTSYS;
    > + 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;
    > + }
    > + } else {
    > + n = -EPIPE;
    > + }
    > +
    > + 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);
    > + urb->status = ret;

    That is a bug. You will report that error the next time the URB comes up.
    That's wrong. I think you need a special error code that will cause you
    to just resubmit the URB and go for the next URB.

    > + dev_err(&urb->dev->dev,
    > + "Failed submitting read urb (error %d)", ret);
    > + }
    > +
    > + ret = n;
    > +exit:
    > + mutex_unlock(&dev->read_mutex);
    > + return ret;
    > +}

    > +static long nrpz_compat_ioctl(struct file *file, unsigned int cmd,
    > + unsigned long arg)
    > +{
    > + struct usb_nrpz *dev = file->private_data;
    > + struct usb_interface *intf = dev->intf;
    > + struct usb_device *udev;
    > + int ret;
    > +
    > + /* verify that the device wasn't unplugged */
    > + if (!dev->connected)
    > + return -ENODEV;
    > +
    > + udev = interface_to_usbdev(intf);
    > +
    > + 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(udev->descriptor.bcdDevice,
    > + &sensor_info->bcdDevice);
    > + __put_user(udev->descriptor.bcdUSB,
    > + &sensor_info->bcdUSB);
    > + __put_user(udev->descriptor.bDescriptorType,
    > + &sensor_info->bDescriptorType);
    > + __put_user(udev->descriptor.bDeviceClass,
    > + &sensor_info->bDeviceClass);
    > + __put_user(udev->descriptor.bDeviceSubClass,
    > + &sensor_info->bDeviceSubClass);
    > + __put_user(udev->descriptor.bDeviceProtocol,
    > + &sensor_info->bDeviceProtocol);
    > + __put_user(udev->descriptor.bMaxPacketSize0,
    > + &sensor_info->bMaxPacketSize0);
    > + __put_user(udev->descriptor.bNumConfigurations,
    > + &sensor_info->bNumConfigurations);
    > + __put_user(udev->descriptor.iManufacturer,
    > + &sensor_info->iManufacturer);
    > + __put_user(udev->descriptor.iProduct,
    > + &sensor_info->iProduct);
    > + __put_user(udev->descriptor.iSerialNumber,
    > + &sensor_info->iSerialNumber);
    > + __put_user(udev->descriptor.idVendor,
    > + &sensor_info->vendorId);
    > + __put_user(udev->descriptor.idProduct,
    > + &sensor_info->productId);
    > + usb_string(udev, udev->descriptor.iManufacturer,
    > + (char __force *)sensor_info->manufacturer,
    > + sizeof(sensor_info->manufacturer));
    > + usb_string(udev, udev->descriptor.iProduct,
    > + (char __force *)sensor_info->productName,
    > + sizeof(sensor_info->productName));
    > + usb_string(udev, udev->descriptor.iSerialNumber,
    > + (char __force *)sensor_info->serialNumber,
    > + sizeof(sensor_info->serialNumber));
    > +
    > + return 0;
    > + }
    > + case NRPZ_START:
    > + {
    > + u8 *device_state;
    > +
    > + device_state = kzalloc(DEVICE_STATE_SIZE, GFP_KERNEL | GFP_DMA);

    I apologize for misleading naming in the MM code. GFP_DMA goes back
    to the time the ISA bus was alive and kicking. GFP_DMA means memory
    that you can do DMA with with the old PC_AT ISA DMA controller. For USB
    just GFP_KERNEL will do.

    > + ret = usb_control_msg(udev,
    > + usb_rcvctrlpipe(udev, 0),
    > + VRT_GET_DEVICE_INFO,
    > + USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
    > + 0,
    > + VRI_DEVICE_NAME,
    > + device_state,
    > + DEVICE_STATE_SIZE,
    > + 5000);
    > +
    > + if (ret < 0)
    > + goto done;
    > +
    > + dev_dbg(&intf->dev,
    > + "device state:%s", device_state);
    > +
    > + if (strncmp(device_state, "Boot ", 5)) {
    > + ret = 0;
    > + goto done;
    > + }
    > +
    > + ret = usb_control_msg(udev,
    > + usb_sndctrlpipe(udev, 0),
    > + VRT_RESET_ALL,
    > + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
    > + 1,
    > + 1,
    > + device_state,
    > + sizeof(device_state),
    > + 5000);
    > +done:
    > + kfree(device_state);
    > + return ret;
    > + }
    > + case NRPZ_WRITE_DONE:
    > + 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;
    > + case NRPZ_VENDOR_CONTROL_MSG_OUT:
    > + {
    > + struct nrpz_control_req ncr;
    > + u8 *data;
    > + 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;
    > +
    > + data = kzalloc(size, GFP_KERNEL | GFP_DMA);
    > + if (!data)
    > + return -ENOMEM;
    > +
    > + memcpy(data, ncr.data, size);
    > + } else {
    > + size = 0;
    > + data = NULL;
    > + }
    > +
    > + ret = usb_control_msg(udev,
    > + usb_sndctrlpipe(udev, 0),
    > + ncr.request,
    > + ncr.type,
    > + ncr.value,
    > + ncr.index,
    > + data,
    > + size,
    > + 0);
    > +
    > +
    > + kfree(data);
    > +
    > + return ret;
    > + }
    > + case NRPZ_VENDOR_CONTROL_MSG_IN:
    > + {
    > + struct nrpz_control_req ncr;
    > + u8 *data;
    > + 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;
    > +
    > + data = kzalloc(size, GFP_KERNEL | GFP_DMA);
    > + if (!data)
    > + return -ENOMEM;
    > + } else {
    > + size = 0;
    > + data = NULL;
    > + }
    > +
    > + ret = usb_control_msg(udev,
    > + usb_rcvctrlpipe(udev, 0),
    > + ncr.request,
    > + ncr.type,
    > + ncr.value,
    > + ncr.index,
    > + data,
    > + size,
    > + 0);
    > +
    > + if (data) {
    > + memcpy(ncr.data, data, size);
    > + kfree(data);
    > + }
    > +
    > + return ret;
    > + }
    > + default:
    > + dev_dbg(&intf->dev,
    > + "Invalid ioctl call (%08x)", cmd);
    > + return -ENOTTY;
    > + }
    > +
    > + return ret;
    > +}

    > +static int nrpz_release(struct inode *inode, struct file *file)
    > +{
    > + struct usb_nrpz *dev = file->private_data;
    > +
    > + if (dev == NULL)
    > + return -ENODEV;
    > +
    > + usb_kill_anchored_urbs(&dev->in_running);
    > + usb_kill_anchored_urbs(&dev->out_running);

    Isn't this a noop as you've implemented flush() ?

    > +
    > + bulks_release(dev->out_urbs, ARRAY_SIZE(dev->out_urbs), dev->out_size);
    > + bulks_release(dev->in_urbs, ARRAY_SIZE(dev->in_urbs), dev->in_size);
    > +
    > + /* decrement the count on our device */
    > + kref_put(&dev->kref, nrpz_delete);
    > +
    > + spin_lock_irq(&dev->read_lock);
    > + dev->in_use = false;
    > + spin_unlock_irq(&dev->read_lock);

    Looks like a use after free. You should drop the reference as the very
    last thing.

    > +
    > + return 0;
    > +}
    > +

    HTH
    Oliver


    \
     
     \ /
      Last update: 2012-06-02 22:41    [W:0.048 / U:1.888 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site