Re: V4L2: switch to register_chrdev_region: needs testing/reviewof release() handling

[Hans de Goede]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/