lkml.org 
[lkml]   [2008]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: V4L2: switch to register_chrdev_region: needs testing/review of release() handling
Date
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,
> #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.

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:
http://linuxtv.org/hg/~hverkuil/v4l-dvb-cdev2/rev/575997018499

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

Regards,

Hans


\
 
 \ /
  Last update: 2008-08-17 21:51    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site