lkml.org 
[lkml]   [2008]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: V4L2: switch to register_chrdev_region: needs testing/review of release() handling
Hans Verkuil wrote:
> Hi all,
>
> As part of my ongoing cleanup of the v4l subsystem I've been looking
> into converting v4l from register_chrdev to register_chrdev_region. The
> latter is more flexible and allows for a larger range of minor numbers.
> In addition it allows us to intercept the release callback when the
> char device's refcount reaches 0.
>

Hans,

Thanks for doing this! You rock! I've been planning on cleaning up gspca's
somewhat archaic disconnect handling for a while now and I was sorta waiting
for something like this :) But I guess that that cleanup might be 2.6.28 material.

Anyways I've reviewed your patch and in general I like it, I only see one problem:

@@ -99,7 +130,8 @@ static void video_release(struct device
{
struct video_device *vfd = container_of(cd, struct video_device, dev);
-#if 1 /* keep */
+ return;
+#if 1 /* keep */
/* needed until all drivers are fixed */
if (!vfd->release)
return;
@@ -107,6 +139,7 @@ static void video_release(struct device
vfd->release(vfd);
}
+
static struct class video_class = {
.name = VIDEO_NAME,
#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 19)


Here you basicly make the release callback of the video class device a no-op.
First of all I think it would be better to just delete it then to add a return,
which sort of hides its an empty function now.

More importantly, its wrong to make this a no-op. When a driver unregisters a
v4l device, and all cdev usage has stopped there can still be open references
to sysfs files of the video class device, but in this case currently the
video_unregister_device call will lead to the vfd->release callback getting
called freeing the vfd struct, which contains the class device.

I believe the way to fix this is todo a get on the kobj contained in the cdev
in video_register_device before registering the class device, and then in the
class device release callback do a put on this kobj.

Other then that it looks good!

Regards,
Hans



\
 
 \ /
  Last update: 2008-08-17 20:23    [W:0.056 / U:1.488 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site