Messages in this thread Patch in this message | | | Date | Sun, 17 Aug 2008 20:30:19 +0200 | From | Hans de Goede <> | Subject | Re: 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
| |