lkml.org 
[lkml]   [2008]   [May]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] USB: add Sensoray 2255 v4l driver
Date
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


\
 
 \ /
  Last update: 2008-05-15 13:41    [from the cache]
©2003-2011 Jasper Spaans