Messages in this thread | | | From | Oliver Neukum <> | Subject | Re: [PATCH] USB: add Sensoray 2255 v4l driver | Date | Thu, 15 May 2008 13:38:37 +0200 |
| |
Hi,
1. how about a *.h file?
2. You can inline these.
+static int norm_maxw(struct video_device *vdev) +{ + return (vdev->current_norm != V4L2_STD_PAL_B) ? + LINE_SZ_4CIFS_NTSC : LINE_SZ_4CIFS_PAL; +}
3. The firmware stuff. That's an interesting solution. However:
a - if you don't need that delay, use a work queue
b - that's mean, use interruptible sleep
+ /* give 1 second for firmware to load in case + driver loaded and then device immediately opened */ + msleep(1000);
particularly you'd stall khubd processing an early unplug
c - obviously loading the firmware might have failed after waiting
+ if (dev->fw_data->fw_state == FWSTATE_NOTLOADED) { + err("2255 firmware loading stalled\n"); + mutex_unlock(&usb_s2255_open_mutex); + return -EAGAIN; + } + }
you need a check for failure in an else branch
d - so you'll never release firmware in the error case unless you unplug
+ /* if first open after firmware loaded, release the firmware */ + if (dev->fw_data->fw) { + release_firmware(dev->fw_data->fw); + dev->fw_data->fw = NULL; + }
e - you need to report errors
+static void s2255_timer(unsigned long user_data) +{ + struct complete_data *data = (struct complete_data *)user_data; + dprintk(100, "s2255 timer\n"); + if (usb_submit_urb(data->fw_urb, GFP_ATOMIC) < 0) { + printk(KERN_ERR "s2255: can't submit urb\n"); + if (data->fw) { + release_firmware(data->fw); + data->fw = NULL; + } + return; + } +}
f - also here
+static void s2255_fwchunk_complete(struct urb *urb) +{ + struct complete_data *data = urb->context; + struct usb_device *udev = urb->dev; + int len; + dprintk(100, "udev %p urb %p", udev, urb); + + if (urb->status) { + dev_err(&udev->dev, "URB failed with status %d", urb->status); + return; + }
4. as a rule, init all locks before you start a timer
+ mod_timer(&dev->timer, jiffies + HZ); + spin_lock_init(&dev->slock);
5. Unnecessary init
+static void s2255_disconnect(struct usb_interface *interface) +{ + struct s2255_dev *dev = NULL;
6. The initial firmware timer may still be ticking
+ if (dev->fw_data->fw_urb) { + dprintk(2, "kill URB\n"); + usb_kill_urb(dev->fw_data->fw_urb); + usb_free_urb(dev->fw_data->fw_urb);
You need to delete that timer and kill the firmware urb after that.
7. That's not an error you want to return in that case. It may livelock
+ if (dev->users[cur_channel] > 1) { + dev->users[cur_channel]--; + dev_err(&dev->udev->dev, "one user at a time\n"); + mutex_unlock(&usb_s2255_open_mutex); + return -EAGAIN; + }
8. Bogus check
+ if (data->fw_urb == NULL) { + dev_err(&udev->dev, "early disconncect\n"); + return; + }
9. This can be computed directly
+ while (*size * *count > vid_limit * 1024 * 1024) + (*count)--;
10. This is fishy.
+static int res_locked(struct s2255_dev *dev, struct s2255_fh *fh) +{ + return (dev->resources[fh->channel]); +} + +static void res_free(struct s2255_dev *dev, struct s2255_fh *fh) +{ + dev->resources[fh->channel] = 0; + dprintk(1, "res: put\n"); +}
In theory out of order memory access might return false positives. Better use memory barriers or take the mutex.
11. Coding style
+ return (0);
12. This is close to obfuscated code
+ is_ntsc = + (dev->vdev[fh->channel]->current_norm != V4L2_STD_PAL_B) ? 1 : 0;
13. Coding style
+ if (ret < 0) + return (ret);
14. GFP_KERNEL in interrupt context
+ + if (pipe_info->state != 0) { + if (usb_submit_urb(pipe_info->stream_urb, GFP_KERNEL)) {
15. Error handling in s2255_probe() is a gigantic resource leak
+ if (!dev->fw_data->fw_urb) { + dev_err(&interface->dev, "out of memory!\n"); + goto error; + }
You must free what you allocate
Regards Oliver
| |