Message-Id: <200805151338.38801.oliver@neukum.org>
Date:	Thu, 15 May 2008 13:38:37 +0200
From:	Oliver Neukum <oliver@...kum.org>
To:	Greg KH <greg@...ah.com>
Cc:	mchehab@...radead.org, v4l-dvb-maintainer@...uxtv.org,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	video4linux-list@...hat.com
Subject: Re: [PATCH] USB: add Sensoray 2255 v4l driver

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