[lkml]   [2008]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: V4L2: switch to register_chrdev_region: needs testing/review of release() handling
Hi Hans,

On Sunday 17 August 2008 20:30:19 Hans de Goede wrote:
> 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!

Thanks! :-)

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

I thought so as well, but without a release function the low-level class
code in the kernel starts complaining about the missing release.

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

You might have gotten confused here: video_release() didn't call the
main release callback: there was a return in front. I'd forgotten to
remove that test code.

I've also tested what happens when you keep a sysfs file open: it seems
to work OK in that video_release is called even though the sysfs file
is still open. That said, I've moved the cdev_del call from
video_unregister_device() to the video_release() function. It makes
sense not to delete the char device until that callback is called.

This patch is here:

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

There is no need to do an additional get: cdev_init does that already,
so the char dev stays alive at least until the cdev_del (longer if apps
still keep it open).

> Other then that it looks good!

Thanks, I've been wanting to do this for some time now and I finally had
the time today to go in and dig through all the refcounting and how
chardev handles things so that I could come up with a proper solution.
What's nice is that this approach works also fine in older kernels
(well, it compiles, I guess I need to do a real test on an older kernel
before I can commit it in v4l-dvb). And that is very nice since the
v4l-dvb repository is supposed to support any kernel >= 2.6.16.

I would be very curious to hear how well it works with the gspca driver.
In particular if you can indeed reconnect while an application still
has a char device open from before the disconnect. Currently the gspca
own locking seems to disallow that (the new device doesn't appear until
all applications stopped using the old one).



 \ /
  Last update: 2008-08-17 21:51    [from the cache]
©2003-2014 Jasper Spaans. hosted at Digital Ocean