Please test: using the device release() callback instead of the cdev release

Please test: using the device release() callback instead of the cdev release

[Hans Verkuil]

[-- Attachment #1: Type: text/plain, Size: 2687 bytes --]

Hi all,

My tree http://linuxtv.org/hg/~hverkuil/v4l-dvb drops the cdev release code 
in favor of using the refcounting and release callback from the device 
struct. Based on the discussion on the kernel list regarding the use of 
cdev refcounting it became clear that that was not the right solution, 
hence this change.

In combination with the cdev fix posted earlier in the cdev thread I am 
unable to force any races or oopses. I've tested with the uvc driver 
(bought a uvc webcam :-) ) in combination with Laurent's UVC patch 
(attached to this mail).

What's particularly nice is that if an app has /dev/video0 open and you 
disconnect followed by a reconnect, then this newly probed device ends up 
at /dev/video1 since internally /dev/video0 is still in use even though the 
actual device node has been removed already. That's the way it should work, 
so this looks very promising.

The only disadvantage of my change is that struct video_device gets a copy 
of the file_operations struct since I need to override the open and release 
to do the refcounting. In the future we can do this much more elegant but I 
don't want to make changes to drivers right now.

I'd appreciate it people can test it and if the change I made in v4l2-dev.c 
can be reviewed. It's not a big patch, but it's tricky code at the core of 
v4l. Basically you should see no difference at all in the behavior of 
drivers. It would be very nice as well if someone can test it on a 2.6.18 
kernel or older. It compiles, but I've no idea whether it also works...

Regards,

	Hans

PS: For those who missed it, this is my patch to fix a race condition in 
fs/char_dev.c:

--- fs/char_dev.c.orig  2008-12-17 20:28:40.000000000 +0100
+++ fs/char_dev.c       2008-12-17 20:28:49.000000000 +0100
@@ -345,7 +345,9 @@
 {
        if (p) {
                struct module *owner = p->owner;
+               spin_lock(&cdev_lock);
                kobject_put(&p->kobj);
+               spin_unlock(&cdev_lock);
                module_put(owner);
        }
 }
@@ -415,14 +417,12 @@
 
 static void cdev_purge(struct cdev *cdev)
 {
-       spin_lock(&cdev_lock);
        while (!list_empty(&cdev->list)) {
                struct inode *inode;
                inode = container_of(cdev->list.next, struct inode, 
i_devices);
                list_del_init(&inode->i_devices);
                inode->i_cdev = NULL;
        }
-       spin_unlock(&cdev_lock);
 }
 
 /*
@@ -478,7 +478,9 @@
 void cdev_del(struct cdev *p)
 {
        cdev_unmap(p->dev, p->count);
+       spin_lock(&cdev_lock);
        kobject_put(&p->kobj);
+       spin_unlock(&cdev_lock);
 }


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

[-- Attachment #2: uvc.diff --]
[-- Type: text/x-diff, Size: 6770 bytes --]

diff -r c81af7a1a20a linux/drivers/media/video/uvc/uvc_driver.c
--- a/linux/drivers/media/video/uvc/uvc_driver.c	Tue Dec 16 12:32:37 2008 +0100
+++ b/linux/drivers/media/video/uvc/uvc_driver.c	Thu Dec 18 00:00:52 2008 +0100
@@ -1169,20 +1169,6 @@
  */
 
 /*
- * Unregister the video devices.
- */
-static void uvc_unregister_video(struct uvc_device *dev)
-{
-	if (dev->video.vdev) {
-		if (dev->video.vdev->minor == -1)
-			video_device_release(dev->video.vdev);
-		else
-			video_unregister_device(dev->video.vdev);
-		dev->video.vdev = NULL;
-	}
-}
-
-/*
  * Scan the UVC descriptors to locate a chain starting at an Output Terminal
  * and containing the following units:
  *
@@ -1398,6 +1384,64 @@
 }
 
 /*
+ * Delete the UVC device.
+ *
+ * Called by the kernel when the last reference to the V4L2 device (and thus
+ * the uvc_device structure) is released.
+ *
+ * As this function is called after or during disconnect(), all URBs have
+ * already been canceled by the USB core. There is no need to kill the
+ * interrupt URB manually.
+ */
+static void uvc_delete(struct uvc_device *dev)
+{
+	struct list_head *p, *n;
+
+	usb_put_intf(dev->intf);
+	usb_put_dev(dev->udev);
+
+	uvc_status_cleanup(dev);
+	uvc_ctrl_cleanup_device(dev);
+
+	list_for_each_safe(p, n, &dev->entities) {
+		struct uvc_entity *entity;
+		entity = list_entry(p, struct uvc_entity, list);
+		kfree(entity);
+	}
+
+	list_for_each_safe(p, n, &dev->streaming) {
+		struct uvc_streaming *streaming;
+		streaming = list_entry(p, struct uvc_streaming, list);
+		usb_driver_release_interface(&uvc_driver.driver,
+			streaming->intf);
+		usb_put_intf(streaming->intf);
+		kfree(streaming->format);
+		kfree(streaming->header.bmaControls);
+		kfree(streaming);
+	}
+
+	kfree(dev);
+}
+
+static void uvc_release(struct video_device *vdev)
+{
+	struct uvc_video_device *video = video_get_drvdata(vdev);
+	struct uvc_device *dev = video->dev;
+
+	video_device_release(vdev);
+	uvc_delete(dev);
+}
+
+/*
+ * Unregister the video devices.
+ */
+static void uvc_unregister_video(struct uvc_device *dev)
+{
+	video_unregister_device(dev->video.vdev);
+	dev->video.vdev = NULL;
+}
+
+/*
  * Register the video devices.
  *
  * The driver currently supports a single video device per control interface
@@ -1480,7 +1524,7 @@
 	vdev->parent = &dev->intf->dev;
 	vdev->minor = -1;
 	vdev->fops = &uvc_fops;
-	vdev->release = video_device_release;
+	vdev->release = uvc_release;
 	strncpy(vdev->name, dev->name, sizeof vdev->name);
 
 	/* Set the driver data before calling video_register_device, otherwise
@@ -1500,55 +1544,6 @@
 	}
 
 	return 0;
-}
-
-/*
- * Delete the UVC device.
- *
- * Called by the kernel when the last reference to the uvc_device structure
- * is released.
- *
- * Unregistering the video devices is done here because every opened instance
- * must be closed before the device can be unregistered. An alternative would
- * have been to use another reference count for uvc_v4l2_open/uvc_release, and
- * unregister the video devices on disconnect when that reference count drops
- * to zero.
- *
- * As this function is called after or during disconnect(), all URBs have
- * already been canceled by the USB core. There is no need to kill the
- * interrupt URB manually.
- */
-void uvc_delete(struct kref *kref)
-{
-	struct uvc_device *dev = container_of(kref, struct uvc_device, kref);
-	struct list_head *p, *n;
-
-	/* Unregister the video device */
-	uvc_unregister_video(dev);
-	usb_put_intf(dev->intf);
-	usb_put_dev(dev->udev);
-
-	uvc_status_cleanup(dev);
-	uvc_ctrl_cleanup_device(dev);
-
-	list_for_each_safe(p, n, &dev->entities) {
-		struct uvc_entity *entity;
-		entity = list_entry(p, struct uvc_entity, list);
-		kfree(entity);
-	}
-
-	list_for_each_safe(p, n, &dev->streaming) {
-		struct uvc_streaming *streaming;
-		streaming = list_entry(p, struct uvc_streaming, list);
-		usb_driver_release_interface(&uvc_driver.driver,
-			streaming->intf);
-		usb_put_intf(streaming->intf);
-		kfree(streaming->format);
-		kfree(streaming->header.bmaControls);
-		kfree(streaming);
-	}
-
-	kfree(dev);
 }
 
 static int uvc_probe(struct usb_interface *intf,
@@ -1572,7 +1567,6 @@
 
 	INIT_LIST_HEAD(&dev->entities);
 	INIT_LIST_HEAD(&dev->streaming);
-	kref_init(&dev->kref);
 
 	dev->udev = usb_get_dev(udev);
 	dev->intf = usb_get_intf(intf);
@@ -1629,7 +1623,7 @@
 	return 0;
 
 error:
-	kref_put(&dev->kref, uvc_delete);
+	uvc_delete(dev);
 	return -ENODEV;
 }
 
@@ -1645,21 +1639,9 @@
 	if (intf->cur_altsetting->desc.bInterfaceSubClass == SC_VIDEOSTREAMING)
 		return;
 
-	/* uvc_v4l2_open() might race uvc_disconnect(). A static driver-wide
-	 * lock is needed to prevent uvc_disconnect from releasing its
-	 * reference to the uvc_device instance after uvc_v4l2_open() received
-	 * the pointer to the device (video_devdata) but before it got the
-	 * chance to increase the reference count (kref_get).
-	 *
-	 * Note that the reference can't be released with the lock held,
-	 * otherwise a AB-BA deadlock can occur with videodev_lock that
-	 * videodev acquires in videodev_open() and video_unregister_device().
-	 */
-	mutex_lock(&uvc_driver.open_mutex);
 	dev->state |= UVC_DEV_DISCONNECTED;
-	mutex_unlock(&uvc_driver.open_mutex);
 
-	kref_put(&dev->kref, uvc_delete);
+	uvc_unregister_video(dev);
 }
 
 static int uvc_suspend(struct usb_interface *intf, pm_message_t message)
diff -r c81af7a1a20a linux/drivers/media/video/uvc/uvc_v4l2.c
--- a/linux/drivers/media/video/uvc/uvc_v4l2.c	Tue Dec 16 12:32:37 2008 +0100
+++ b/linux/drivers/media/video/uvc/uvc_v4l2.c	Thu Dec 18 00:00:52 2008 +0100
@@ -398,7 +398,6 @@
 	int ret = 0;
 
 	uvc_trace(UVC_TRACE_CALLS, "uvc_v4l2_open\n");
-	mutex_lock(&uvc_driver.open_mutex);
 	video = video_drvdata(file);
 
 	if (video->dev->state & UVC_DEV_DISCONNECTED) {
@@ -426,10 +425,7 @@
 	handle->state = UVC_HANDLE_PASSIVE;
 	file->private_data = handle;
 
-	kref_get(&video->dev->kref);
-
 done:
-	mutex_unlock(&uvc_driver.open_mutex);
 	return ret;
 }
 
@@ -459,7 +455,6 @@
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 19)
 	usb_autopm_put_interface(video->dev->intf);
 #endif
-	kref_put(&video->dev->kref, uvc_delete);
 	return 0;
 }
 
diff -r c81af7a1a20a linux/drivers/media/video/uvc/uvcvideo.h
--- a/linux/drivers/media/video/uvc/uvcvideo.h	Tue Dec 16 12:32:37 2008 +0100
+++ b/linux/drivers/media/video/uvc/uvcvideo.h	Thu Dec 18 00:00:52 2008 +0100
@@ -624,7 +624,6 @@
 	char name[32];
 
 	enum uvc_device_state state;
-	struct kref kref;
 	struct list_head list;
 
 	/* Video control interface */
@@ -718,7 +717,6 @@
 
 /* Core driver */
 extern struct uvc_driver uvc_driver;
-extern void uvc_delete(struct kref *kref);
 
 /* Video buffers queue management. */
 extern void uvc_queue_init(struct uvc_video_queue *queue);

[-- Attachment #3: Type: text/plain, Size: 164 bytes --]

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Please test: using the device release() callback instead of the cdev release

[Hans de Goede]

<resend with reply to all>

Hans Verkuil wrote:
> Hi all,
> 
> My tree http://linuxtv.org/hg/~hverkuil/v4l-dvb drops the cdev release code 
> in favor of using the refcounting and release callback from the device 
> struct. Based on the discussion on the kernel list regarding the use of 
> cdev refcounting it became clear that that was not the right solution, 
> hence this change.
> 

I haven't tested it, but I have reviewed it. In general it looks ok, but:

I do not like the VFL_FL_REGISTERED trickery. Why not just hold the
videodev_lock in video_register_device_index until completely done? It is not
like these are functions which will get called many times a second. This will
also lead to cleaner code.

The correct return code in v4l2_open when cfd == NULL, so the device has been
removed underneath the open call is -ENODEV, not -EBUSY.

last, device_* seem to have the same problem as cdev_*, when
video_unregister_device and v4l2_release race, we can still end up with a
kref_put race. I see you've fixed this by taking videodev_lock around
device_unregister() and device_put(), but IMHO this really should happen in
drivers/base/core.c, other drivers might vary well hit the same issue. Seems
you need to hit gkh a bit more with that clue stick of yours :) (note this last
one is not a blocker, but would be nice to get fixed eventually).

Regards,

Hans (the other Hans)


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Please test: using the device release() callback instead of the cdev release

[Hans Verkuil]

On Thursday 18 December 2008 11:23:14 Hans de Goede wrote:
> <resend with reply to all>
>
> Hans Verkuil wrote:
> > Hi all,
> >
> > My tree http://linuxtv.org/hg/~hverkuil/v4l-dvb drops the cdev release
> > code in favor of using the refcounting and release callback from the
> > device struct. Based on the discussion on the kernel list regarding the
> > use of cdev refcounting it became clear that that was not the right
> > solution, hence this change.
>
> I haven't tested it, but I have reviewed it. In general it looks ok, but:
>
> I do not like the VFL_FL_REGISTERED trickery. Why not just hold the
> videodev_lock in video_register_device_index until completely done? It is
> not like these are functions which will get called many times a second.
> This will also lead to cleaner code.

This flag is meant to handle the case where a USB device is disconnected 
while an application is still using the video device. In that case the 
disconnect routine unregisters the video device, but it is still possible 
to open the device if the device node was made with mknod instead of 
handled by udev. So it is still possible to call open. Currently drivers 
need to check for this, but it is much easier to catch this in the v4l core 
directly.

Note that eventually all the file_operations that v4l drivers use will go 
through similar code as is now done for open and release. So all those 
operations will do the same test and hopefully drivers don't need to be 
bothered about it. There are some other neat things you can do if all ops 
go through some standard function first (e.g. proper priority handling), 
but that's for the future.

> The correct return code in v4l2_open when cfd == NULL, so the device has
> been removed underneath the open call is -ENODEV, not -EBUSY.

Oops, you are correct. Stupid of me.

> last, device_* seem to have the same problem as cdev_*, when
> video_unregister_device and v4l2_release race, we can still end up with a
> kref_put race. I see you've fixed this by taking videodev_lock around
> device_unregister() and device_put(), but IMHO this really should happen
> in drivers/base/core.c, other drivers might vary well hit the same issue.
> Seems you need to hit gkh a bit more with that clue stick of yours :)
> (note this last one is not a blocker, but would be nice to get fixed
> eventually).

It seems that the rule is that drivers need to take care of their own 
locking. Personally I suspect that there are no doubt a lot of drivers that 
don't do that properly. I don't think it is a terribly good idea to start 
messing with this, though. I prefer to concentrate on doing the right thing 
in the v4l framework, that's already difficult enough.

One change I made is that the video_device release() callback is now called 
without holding the global mutex. Since the release() can take some time 
depending on what it is doing it's much better not to hold that lock. It 
takes a bit of extra code, but it's well worth it.

Both changes are now pushed to my tree for review.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Please test: using the device release() callback instead of the cdev release

[Laurent Pinchart]

Hi Hans,

On Thursday 18 December 2008, Hans Verkuil wrote:
> On Thursday 18 December 2008 11:23:14 Hans de Goede wrote:
> > <resend with reply to all>
> >
> > Hans Verkuil wrote:
> > > Hi all,
> > >
> > > My tree http://linuxtv.org/hg/~hverkuil/v4l-dvb drops the cdev release
> > > code in favor of using the refcounting and release callback from the
> > > device struct. Based on the discussion on the kernel list regarding the
> > > use of cdev refcounting it became clear that that was not the right
> > > solution, hence this change.
> >
> > I haven't tested it, but I have reviewed it. In general it looks ok, but:

Ditto.

Could you use a V4L2_FLAG_ prefix instead of VFL_FL_ ?

$ find include/ -type f -exec grep VFL_ {} \; | wc
     12      36     413
$ find include/ -type f -exec grep V4L_ {} \; | wc
      2       4      58
$ find include/ -type f -exec grep V4L2_ {} \; | wc
   1178    5096   59839

> > I do not like the VFL_FL_REGISTERED trickery. Why not just hold the
> > videodev_lock in video_register_device_index until completely done? It is
> > not like these are functions which will get called many times a second.
> > This will also lead to cleaner code.
>
> This flag is meant to handle the case where a USB device is disconnected
> while an application is still using the video device. In that case the
> disconnect routine unregisters the video device, but it is still possible
> to open the device if the device node was made with mknod instead of
> handled by udev. So it is still possible to call open. Currently drivers
> need to check for this, but it is much easier to catch this in the v4l core
> directly.

The flag is indeed needed, but I'd use a V4L2_FLAG_UNREGISTERED (or 
V4L2_FLAG_UNREGISTER_PENDING) instead. You wouldn't have to explicitly clear 
it when registering the device.

> Note that eventually all the file_operations that v4l drivers use will go
> through similar code as is now done for open and release. So all those
> operations will do the same test and hopefully drivers don't need to be
> bothered about it.

I don't completely agree with that. While an open() calls should obviously 
fails when the device has been or is being unregistered, ioctls might make 
sense, for instance to dequeue remaining buffers.

> There are some other neat things you can do if all ops 
> go through some standard function first (e.g. proper priority handling),
> but that's for the future.
>
> > last, device_* seem to have the same problem as cdev_*, when
> > video_unregister_device and v4l2_release race, we can still end up with a
> > kref_put race. I see you've fixed this by taking videodev_lock around
> > device_unregister() and device_put(), but IMHO this really should happen
> > in drivers/base/core.c, other drivers might vary well hit the same issue.
> > Seems you need to hit gkh a bit more with that clue stick of yours :)
> > (note this last one is not a blocker, but would be nice to get fixed
> > eventually).
>
> It seems that the rule is that drivers need to take care of their own
> locking. Personally I suspect that there are no doubt a lot of drivers that
> don't do that properly. I don't think it is a terribly good idea to start
> messing with this, though. I prefer to concentrate on doing the right thing
> in the v4l framework, that's already difficult enough.
>
> One change I made is that the video_device release() callback is now called
> without holding the global mutex. Since the release() can take some time
> depending on what it is doing it's much better not to hold that lock. It
> takes a bit of extra code, but it's well worth it.

This will only work if put_device() is never called from outside of v4l2-dev. 
Is that guaranteed ? Could something else take a reference to the device ? In 
case of doubt we should probably remove the optimisation for now and call 
vfd->release with the lock held.

[Patch inlined for easier review]

> diff -r 7ec490a64a56 linux/drivers/media/video/v4l2-dev.c
> --- a/linux/drivers/media/video/v4l2-dev.c	Tue Dec 16 14:41:57 2008 +0100
> +++ b/linux/drivers/media/video/v4l2-dev.c	Thu Dec 18 13:28:00 2008 +0100
> @@ -106,58 +106,40 @@
>  }
>  EXPORT_SYMBOL(video_device_release_empty);
>
> -/* Called when the last user of the character device is gone. */
> -static void v4l2_chardev_release(struct kobject *kobj)
> +
> +/* Called when the last user of the video device exits.
> +   Note that the videodev_lock mutex is locked when this
> +   function is called. */

Is this always true ? put_device() is called by v4l2-dev with the video_lock 
mutex held, but could it be called through other code paths ? sysfs maybe ?

[snip]

> @@ -170,6 +152,66 @@
>  #endif
>  }
>  EXPORT_SYMBOL(video_devdata);
> +
> +/* Override for the open function */
> +static int v4l2_open(struct inode *inode, struct file *filp)
> +{
> +	struct video_device *vfd;
> +	int ret;
> +
> +	/* Check if the video device is available */
> +	mutex_lock(&videodev_lock);
> +	vfd = video_devdata(filp);
> +	/* return ENODEV if the video device has been removed
> +	   already or if it is not registered. */
> +	if (vfd == NULL || !test_bit(VFL_FL_REGISTERED, &vfd->flags)) {
> +		mutex_unlock(&videodev_lock);
> +		return -ENODEV;
> +	}
> +	/* and increase the device refcount */
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 19)
> +	class_device_get(&vfd->dev);
> +#else
> +	get_device(&vfd->dev);
> +#endif
> +	mutex_unlock(&videodev_lock);
> +	ret = vfd->fops->open(inode, filp);
> +	if (ret) {
> +		mutex_lock(&videodev_lock);
> +		/* decrease the refcount in case of an error */
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 19)
> +		class_device_put(&vfd->dev);
> +#else
> +		put_device(&vfd->dev);
> +#endif
> +		mutex_unlock(&videodev_lock);
> +		/* Check if the video_device can be released */
> +		if (test_bit(VFL_FL_RELEASE, &vfd->flags) && vfd->release)

Can vfd->release be NULL ? There's a BUG_ON in video_register_device_index. 
The check should be removed here.

> +			vfd->release(vfd);
> +	}
> +	return ret;
> +}
> +
> +/* Override for the release function */
> +static int v4l2_release(struct inode *inode, struct file *filp)
> +{
> +	struct video_device *vfd = video_devdata(filp);
> +	int ret = vfd->fops->release(inode, filp);
> +
> +	mutex_lock(&videodev_lock);
> +	/* decrease the refcount unconditionally since the release()
> +	   return value is ignored. */
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 19)
> +	class_device_put(&vfd->dev);
> +#else
> +	put_device(&vfd->dev);
> +#endif
> +	mutex_unlock(&videodev_lock);
> +	/* Check if the video_device can be released */
> +	if (test_bit(VFL_FL_RELEASE, &vfd->flags) && vfd->release)

Same as above, vfd->release shouldn't be NULL.

> +		vfd->release(vfd);
> +	return ret;
> +}
>
>  /**
>   * get_index - assign stream number based on parent device
> @@ -212,8 +254,6 @@
>  	return i > max_index ? -ENFILE : i;
>  }
>
> -static const struct file_operations video_fops;
> -
>  int video_register_device(struct video_device *vfd, int type, int nr)
>  {
>  	return video_register_device_index(vfd, type, nr, -1);
> @@ -246,7 +286,6 @@
>   *
>   *	%VFL_TYPE_RADIO - A radio card
>   */
> -
>  int video_register_device_index(struct video_device *vfd, int type, int
> nr, int index)
>  {
> @@ -314,13 +353,12 @@
>  	}
>  #endif
>
> -	/* Initialize the character device */
> -#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 17)
> -	cdev_init(&vfd->cdev, vfd->fops);
> -#else
> -	cdev_init(&vfd->cdev, (struct file_operations *)vfd->fops);
> -#endif
> -	vfd->cdev.owner = vfd->fops->owner;
> +	/* Make a copy of the fops and override open and release.
> +	   We take care of proper refcounting. */
> +	vfd->video_fops = *vfd->fops;
> +	vfd->video_fops.open = v4l2_open;
> +	vfd->video_fops.release = v4l2_release;
> +
>  	/* pick a minor number */
>  	mutex_lock(&videodev_lock);
>  	nr = find_next_zero_bit(video_nums[type], minor_cnt, nr == -1 ? 0 : nr);
> @@ -348,7 +386,7 @@
>  	vfd->minor = i + minor_offset;
>  	vfd->num = nr;
>  	set_bit(nr, video_nums[type]);
> -	BUG_ON(video_device[vfd->minor]);
> +	WARN_ON(video_device[vfd->minor] != NULL);
>  	video_device[vfd->minor] = vfd;
>
>  	ret = get_index(vfd, index);
> @@ -361,9 +399,18 @@
>  		goto fail_minor;
>  	}
>
> -	ret = cdev_add(&vfd->cdev, MKDEV(VIDEO_MAJOR, vfd->minor), 1);
> +	/* Initialize the character device */
> +	vfd->cdev = cdev_alloc();
> +	if (vfd->cdev == NULL) {
> +		ret = -ENOMEM;
> +		goto fail_minor;
> +	}
> +	vfd->cdev->ops = &vfd->video_fops;
> +	vfd->cdev->owner = vfd->fops->owner;
> +	ret = cdev_add(vfd->cdev, MKDEV(VIDEO_MAJOR, vfd->minor), 1);
>  	if (ret < 0) {
>  		printk(KERN_ERR "%s: cdev_add failed\n", __func__);
> +		kfree(vfd->cdev);
>  		goto fail_minor;
>  	}
>  	/* sysfs class */
> @@ -404,14 +451,14 @@
>  		goto del_cdev;
>  	}
>  #endif
> -	/* Remember the cdev's release function */
> -	vfd->cdev_release = vfd->cdev.kobj.ktype->release;
> -	/* Install our own */
> -	vfd->cdev.kobj.ktype = &v4l2_ktype_cdev_default;
> +	/* Register the release callback that will be called when the last
> +	   reference to the device goes away. */
> +	vfd->dev.release = v4l2_device_release;
> +	set_bit(VFL_FL_REGISTERED, &vfd->flags);
>  	return 0;
>
>  del_cdev:
> -	cdev_del(&vfd->cdev);
> +	cdev_del(vfd->cdev);
>
>  fail_minor:
>  	mutex_lock(&videodev_lock);
> @@ -427,17 +474,22 @@
>   *	video_unregister_device - unregister a video4linux device
>   *	@vfd: the device to unregister
>   *
> - *	This unregisters the passed device and deassigns the minor
> - *	number. Future open calls will be met with errors.
> + *	This unregisters the passed device. Future open calls will
> + *	be met with errors.
>   */
> -
>  void video_unregister_device(struct video_device *vfd)
>  {
> +	mutex_lock(&videodev_lock);
> +	clear_bit(VFL_FL_REGISTERED, &vfd->flags);
>  #if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 19)
>  	class_device_unregister(&vfd->dev);
>  #else
>  	device_unregister(&vfd->dev);
>  #endif
> +	mutex_unlock(&videodev_lock);
> +	/* Check if the video_device can be released */
> +	if (test_bit(VFL_FL_RELEASE, &vfd->flags) && vfd->release)
> +		vfd->release(vfd);
>  }
>  EXPORT_SYMBOL(video_unregister_device);
>
> diff -r 7ec490a64a56 linux/include/media/v4l2-dev.h
> --- a/linux/include/media/v4l2-dev.h	Tue Dec 16 14:41:57 2008 +0100
> +++ b/linux/include/media/v4l2-dev.h	Thu Dec 18 13:28:00 2008 +0100
> @@ -26,6 +26,17 @@
>
>  struct v4l2_ioctl_callbacks;
>
> +/* Flag to mark the video_device struct as registered.
> +   Drivers can clear this flag if they want to block all future
> +   device access. It is set by video_register_device and
> +   video_register_device_index on success and cleared by
> +   video_unregister_device. */
> +#define VFL_FL_REGISTERED	(0)
> +/* Flag to mark that this video device needs to be released.
> +   It is set when refcount of the device went to 0.
> +   Used internally only, drivers should never touch it. */
> +#define VFL_FL_RELEASE		(1)

See my comment above about flag names.

> +
>  /*
>   * Newer version of video_device, handled by videodev2.c
>   * 	This version moves redundant code from video device code to
> @@ -36,6 +47,7 @@
>  {
>  	/* device ops */
>  	const struct file_operations *fops;
> +	struct file_operations video_fops;
>
>  	/* sysfs */
>  #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 19)
> @@ -43,8 +55,7 @@
>  #else
>  	struct class_device dev;
>  #endif
> -	struct cdev cdev;		/* character device */
> -	void (*cdev_release)(struct kobject *kobj);
> +	struct cdev *cdev;		/* character device */
>  	struct device *parent;		/* device parent */
>
>  	/* device info */
> @@ -52,6 +63,8 @@
>  	int vfl_type;
>  	int minor;
>  	u16 num;
> +	/* use bitops to set/clear/test flags */
> +	u16 flags;

clear_bit/set_bit/ macros use unsigned long pointers.

>  	/* attribute to differentiate multiple indices on one physical device */
>  	int index;

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Please test: using the device release() callback instead of the cdev release

[Hans Verkuil]

On Thursday 18 December 2008 15:29:35 Laurent Pinchart wrote:
> Hi Hans,
>
> On Thursday 18 December 2008, Hans Verkuil wrote:
> > On Thursday 18 December 2008 11:23:14 Hans de Goede wrote:
> > > <resend with reply to all>
> > >
> > > Hans Verkuil wrote:
> > > > Hi all,
> > > >
> > > > My tree http://linuxtv.org/hg/~hverkuil/v4l-dvb drops the cdev
> > > > release code in favor of using the refcounting and release callback
> > > > from the device struct. Based on the discussion on the kernel list
> > > > regarding the use of cdev refcounting it became clear that that was
> > > > not the right solution, hence this change.
> > >
> > > I haven't tested it, but I have reviewed it. In general it looks ok,
> > > but:
>
> Ditto.
>
> Could you use a V4L2_FLAG_ prefix instead of VFL_FL_ ?
>
> $ find include/ -type f -exec grep VFL_ {} \; | wc
>      12      36     413
> $ find include/ -type f -exec grep V4L_ {} \; | wc
>       2       4      58
> $ find include/ -type f -exec grep V4L2_ {} \; | wc
>    1178    5096   59839

I've switched to V4L2_FL_.

> > > I do not like the VFL_FL_REGISTERED trickery. Why not just hold the
> > > videodev_lock in video_register_device_index until completely done?
> > > It is not like these are functions which will get called many times a
> > > second. This will also lead to cleaner code.
> >
> > This flag is meant to handle the case where a USB device is
> > disconnected while an application is still using the video device. In
> > that case the disconnect routine unregisters the video device, but it
> > is still possible to open the device if the device node was made with
> > mknod instead of handled by udev. So it is still possible to call open.
> > Currently drivers need to check for this, but it is much easier to
> > catch this in the v4l core directly.
>
> The flag is indeed needed, but I'd use a V4L2_FLAG_UNREGISTERED (or
> V4L2_FLAG_UNREGISTER_PENDING) instead. You wouldn't have to explicitly
> clear it when registering the device.

Done.

> > Note that eventually all the file_operations that v4l drivers use will
> > go through similar code as is now done for open and release. So all
> > those operations will do the same test and hopefully drivers don't need
> > to be bothered about it.
>
> I don't completely agree with that. While an open() calls should
> obviously fails when the device has been or is being unregistered, ioctls
> might make sense, for instance to dequeue remaining buffers.

True. Something to keep in mind.

> > There are some other neat things you can do if all ops
> > go through some standard function first (e.g. proper priority
> > handling), but that's for the future.
> >
> > > last, device_* seem to have the same problem as cdev_*, when
> > > video_unregister_device and v4l2_release race, we can still end up
> > > with a kref_put race. I see you've fixed this by taking videodev_lock
> > > around device_unregister() and device_put(), but IMHO this really
> > > should happen in drivers/base/core.c, other drivers might vary well
> > > hit the same issue. Seems you need to hit gkh a bit more with that
> > > clue stick of yours :) (note this last one is not a blocker, but
> > > would be nice to get fixed eventually).
> >
> > It seems that the rule is that drivers need to take care of their own
> > locking. Personally I suspect that there are no doubt a lot of drivers
> > that don't do that properly. I don't think it is a terribly good idea
> > to start messing with this, though. I prefer to concentrate on doing
> > the right thing in the v4l framework, that's already difficult enough.
> >
> > One change I made is that the video_device release() callback is now
> > called without holding the global mutex. Since the release() can take
> > some time depending on what it is doing it's much better not to hold
> > that lock. It takes a bit of extra code, but it's well worth it.
>
> This will only work if put_device() is never called from outside of
> v4l2-dev. Is that guaranteed ? Could something else take a reference to
> the device ? In case of doubt we should probably remove the optimisation
> for now and call vfd->release with the lock held.

Good point. I've reverted that part of the patch. I need to dig deeper into 
the device code in the kernel to figure out what the best approach is.

> [Patch inlined for easier review]
>
> > diff -r 7ec490a64a56 linux/drivers/media/video/v4l2-dev.c
> > --- a/linux/drivers/media/video/v4l2-dev.c	Tue Dec 16 14:41:57 2008
> > +0100 +++ b/linux/drivers/media/video/v4l2-dev.c	Thu Dec 18 13:28:00
> > 2008 +0100 @@ -106,58 +106,40 @@
> >  }
> >  EXPORT_SYMBOL(video_device_release_empty);
> >
> > -/* Called when the last user of the character device is gone. */
> > -static void v4l2_chardev_release(struct kobject *kobj)
> > +
> > +/* Called when the last user of the video device exits.
> > +   Note that the videodev_lock mutex is locked when this
> > +   function is called. */
>
> Is this always true ? put_device() is called by v4l2-dev with the
> video_lock mutex held, but could it be called through other code paths ?
> sysfs maybe ?

I think you are right again. Grr, why is this so hard to do?

>
> [snip]
>
> > @@ -170,6 +152,66 @@
> >  #endif
> >  }
> >  EXPORT_SYMBOL(video_devdata);
> > +
> > +/* Override for the open function */
> > +static int v4l2_open(struct inode *inode, struct file *filp)
> > +{
> > +	struct video_device *vfd;
> > +	int ret;
> > +
> > +	/* Check if the video device is available */
> > +	mutex_lock(&videodev_lock);
> > +	vfd = video_devdata(filp);
> > +	/* return ENODEV if the video device has been removed
> > +	   already or if it is not registered. */
> > +	if (vfd == NULL || !test_bit(VFL_FL_REGISTERED, &vfd->flags)) {
> > +		mutex_unlock(&videodev_lock);
> > +		return -ENODEV;
> > +	}
> > +	/* and increase the device refcount */
> > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 19)
> > +	class_device_get(&vfd->dev);
> > +#else
> > +	get_device(&vfd->dev);
> > +#endif
> > +	mutex_unlock(&videodev_lock);
> > +	ret = vfd->fops->open(inode, filp);
> > +	if (ret) {
> > +		mutex_lock(&videodev_lock);
> > +		/* decrease the refcount in case of an error */
> > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 19)
> > +		class_device_put(&vfd->dev);
> > +#else
> > +		put_device(&vfd->dev);
> > +#endif
> > +		mutex_unlock(&videodev_lock);
> > +		/* Check if the video_device can be released */
> > +		if (test_bit(VFL_FL_RELEASE, &vfd->flags) && vfd->release)
>
> Can vfd->release be NULL ? There's a BUG_ON in
> video_register_device_index. The check should be removed here.

Correct.

...

> > +	/* use bitops to set/clear/test flags */
> > +	u16 flags;
>
> clear_bit/set_bit/ macros use unsigned long pointers.

Correct.

I've fixed all the smaller points in my tree. Tonight or tomorrow I'll look 
closer on how to safely find the moment that vfd->release can be called.

Thanks for the good review!

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Please test: using the device release() callback instead of the cdev release

[Hans Verkuil]

On Friday 19 December 2008 14:24:42 Hans de Goede wrote:
> Hans Verkuil wrote:
> > On Thursday 18 December 2008 11:23:14 Hans de Goede wrote:
> >> <resend with reply to all>
> >>
> >> Hans Verkuil wrote:
> >>> Hi all,
> >>>
> >>> My tree http://linuxtv.org/hg/~hverkuil/v4l-dvb drops the cdev
> >>> release code in favor of using the refcounting and release callback
> >>> from the device struct. Based on the discussion on the kernel list
> >>> regarding the use of cdev refcounting it became clear that that was
> >>> not the right solution, hence this change.
> >>
> >> I haven't tested it, but I have reviewed it. In general it looks ok,
> >> but:
> >>
> >> I do not like the VFL_FL_REGISTERED trickery. Why not just hold the
> >> videodev_lock in video_register_device_index until completely done? It
> >> is not like these are functions which will get called many times a
> >> second. This will also lead to cleaner code.
> >
> > This flag is meant to handle the case where a USB device is
> > disconnected while an application is still using the video device. In
> > that case the disconnect routine unregisters the video device, but it
> > is still possible to open the device if the device node was made with
> > mknod instead of handled by udev. So it is still possible to call open.
> > Currently drivers need to check for this, but it is much easier to
> > catch this in the v4l core directly.
>
> I see, ok.
>
> > Note that eventually all the file_operations that v4l drivers use will
> > go through similar code as is now done for open and release. So all
> > those operations will do the same test and hopefully drivers don't need
> > to be bothered about it. There are some other neat things you can do if
> > all ops go through some standard function first (e.g. proper priority
> > handling), but that's for the future.
>
> +1
>
> <snip>
>
> > One change I made is that the video_device release() callback is now
> > called without holding the global mutex. Since the release() can take
> > some time depending on what it is doing it's much better not to hold
> > that lock. It takes a bit of extra code, but it's well worth it.
>
> +1 (also see my other mail)
>
>
> I do still have one issue with your new code, there still is a open /
> release(through unregister) race.

Hmm, I think I should have posted my latest changes as a reply to this 
thread, rather than starting a new one.

Please look at my posting with subject "[PATCH] Please review V3 of 
v4l2-dev.c".

The version you are looking at is wrong and will crash. But I have high 
hopes of the V3 version.

Regards,

	Hans

>
> Take the following:
>
> (the device is open 0 times)
> unregister gets called
> release gets called
> release gets done till the "mutex_unlock(&videodev_lock);"
> register gets called (camera replugged, usb glitch), problem:
> This new device will get the same minor as the just unregistered one!
> So cdev_add will get called for already used minor, cdev_add does not
> check for this! Then cdev_del will get called on the minor used for the
> new device.
>
> I believe this can be fixed by calling cdev_del inside the piece which
> has the videodev_lock.
>
> Regards,
>
> Hans



-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Please test: using the device release() callback instead of the cdev release

[Laurent Pinchart]

On Thursday 18 December 2008, Hans de Goede wrote:
> <resend with reply to all>
>
> Hans Verkuil wrote:
> > Hi all,
> >
> > My tree http://linuxtv.org/hg/~hverkuil/v4l-dvb drops the cdev release
> > code in favor of using the refcounting and release callback from the
> > device struct. Based on the discussion on the kernel list regarding the
> > use of cdev refcounting it became clear that that was not the right
> > solution, hence this change.
>
> I haven't tested it, but I have reviewed it. In general it looks ok, but:
>
> I do not like the VFL_FL_REGISTERED trickery. Why not just hold the
> videodev_lock in video_register_device_index until completely done? It is
> not like these are functions which will get called many times a second.
> This will also lead to cleaner code.
>
> The correct return code in v4l2_open when cfd == NULL, so the device has
> been removed underneath the open call is -ENODEV, not -EBUSY.
>
> last, device_* seem to have the same problem as cdev_*, when
> video_unregister_device and v4l2_release race, we can still end up with a
> kref_put race. I see you've fixed this by taking videodev_lock around
> device_unregister() and device_put(), but IMHO this really should happen in
> drivers/base/core.c, other drivers might vary well hit the same issue.
> Seems you need to hit gkh a bit more with that clue stick of yours :) (note
> this last one is not a blocker, but would be nice to get fixed eventually).

That would be a performance killer. We need a global lock to prevent races, 
and I'm pretty sure a single global lock across all devices will be rejected 
(especially if it's a mutex). Handling locking at the subsystem level still 
requires a global lock, but we will have one per subsystem.

Regards,

Laurent Pinchart

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list