Message-Id: <200808231601.33052.laurent.pinchart@skynet.be>
Date:	Sat, 23 Aug 2008 16:01:32 +0200
From:	Laurent Pinchart <laurent.pinchart@...net.be>
To:	Hans Verkuil <hverkuil@...all.nl>
Cc:	v4l <video4linux-list@...hat.com>,
	Hans de Goede <j.w.r.degoede@....nl>,
	david@...ntd.dyndns.org,
	Mauro Carvalho Chehab <mchehab@...radead.org>,
	linux-kernel@...r.kernel.org, Mike Isely <isely@...ly.net>
Subject: Re: V4L2: switch to register_chrdev_region: needs testing/review of release() handling

Hi Hans,

On Sunday 17 August 2008, 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.
>
> This is very useful for hotpluggable devices like USB webcams. Currently
> usb video drivers need to do the refcounting themselves, but with this
> patch they can rely on the release callback since it will only be
> called when the last user has closed the device. Since current usb
> drivers do the refcounting in varying degrees of competency (from 'not'
> to 'if you're lucky' to 'buggy' to 'perfect') it would be nice to have
> the v4l framework take care of this.
>
> So on a disconnect the driver can call video_unregister_device() even if
> an application still has the device open. Only when the application
> closes as well will the release be called and the driver can do the
> final cleanup.
>
> In fact, I think with this change it should even be possible to
> reconnect the webcam even while some application is still using the old
> char device. In that case a new minor number will be chosen since the
> old one is still in use, but otherwise the webcam should just work as
> usual. This is untested, though.
>
> Note that right now I basically copy the old release callback as
> installed by cdev_init() and install our own v4l callback instead (to
> be precise, I replace the ktype pointer with our own kobj_type).
>
> It would be much cleaner if chardev.c would allow one to set a callback
> explicitly. It's not difficult to do that, but before doing that I
> first have to know whether my approach is working correctly.
>
> The v4l-dvb repository with my changes is here:
>
> http://linuxtv.org/hg/~hverkuil/v4l-dvb-cdev2/
>
> To see the diff in question:
>
> http://linuxtv.org/hg/~hverkuil/v4l-dvb-cdev2/rev/98acd2c1dea1
>
> I have tested myself with the quickcam_messenger webcam. For this driver
> this change actually fixed a bug: disconnecting while a capture was in
> progress and then trying to use /dev/video0 would lock that second
> application.
>
> I also tested with gspca: I could find no differences here, it all
> worked as before.
>
> There are a lot more USB video devices and it would be great if people
> could test with their devices to see if this doesn't break anything.
> Having a release callback that is called when it is really safe to free
> everything should make life a lot easier I think.

I've given your patch a try on 2.6.27-rc4 with the uvcvideo driver. Nothing 
seems to have been broken so far, and the uvcvideo driver got a bit simpler 
as I've been able to get rid of the refcounting logic. Good job.

Do we really need the double refcounting (class device and character device) ? 
As far as I can tell, the class device is only used for the name and index 
sysfs attributes. Its release callback, video_release, is called as soon as 
video_unregister_device drops its reference to the class device, even when 
sysfs files are still opened.

Best regards,

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

Powered by blists - more mailing lists