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

I wasn't clear with delete I only meant the body.

>> 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'm not talking about video_release, I'm talking about the following call chain:
video_device_unregister
cdev_del
kobj_put
v4l2_chardev_release
vfd->release

Which could happen in your old version (before the cdev_del was moved) even
when a class device sysfs file was still open, and thus sysfs read / write
callbacks which may use the (now released) vfd could still be made after the
release.

> 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 should not happen, if a process holds a sysfs file open the release
callback for the device which owns the sysfs file should not get called, are
you sure this is happening, if the device then does a read / write on this file
mayhem could happen, or does the kernel catch this now a days and just returns
-ENODEV?

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

Yes, that will fix the problem I was trying to describe too.

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

Well since the code was registering a class device which shared the same in
memory struct, we needed an additional put on the cdev kobj, as once the
refcount for that got to 0 the entire vfd struct including the class device
would get released.

But now that you've moved the cdev_del this is moot, as now the ref_count won't
reach zero until all users of the class device are done with it.

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

This is on my todo, but not very high atm.

Regards,

Hans


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