Em Thu, 15 Dec 2016 15:03:36 +0100 Hans Verkuil <hverkuil@xxxxxxxxx> escreveu: > On 15/12/16 13:56, Laurent Pinchart wrote: > > Hi Sakari, > > > > On Thursday 15 Dec 2016 13:30:41 Sakari Ailus wrote: > >> On Tue, Dec 13, 2016 at 10:24:47AM -0200, Mauro Carvalho Chehab wrote: > >>> Em Tue, 13 Dec 2016 12:53:05 +0200 Sakari Ailus escreveu: > >>>> On Tue, Nov 29, 2016 at 09:13:05AM -0200, Mauro Carvalho Chehab wrote: > >>>>> Hi Sakari, > >>>>> > > There's plenty of way to try and work around the problem in drivers, some more > > racy than others, but if we require changes to all platform drivers to fix > > this we need to ensure that we get it right, not as half-baked hacks spread > > around the whole subsystem. > > Why on earth do we want this for the omap3 driver? It is not a hot-pluggable > device and I see no reason whatsoever to start modifying platform drivers just > because you can do an unbind. I know there are real hot-pluggable devices, and > getting this right for those is of course important. That's indeed a very good point. If unbind is not needed by any usecase, the better fix for OMAP3 would be to just prevent it to happen in the first place. > >>> The USB subsystem has a a .disconnect() callback that notifies > >>> the drivers that a device was unbound (likely physically removed). > >>> The way USB media drivers handle it is by returning -ENODEV to any > >>> V4L2 call that would try to touch at the hardware after unbound. > > > > In my view the main problem is that the media core is bound to a struct > device set by the driver that creates the MC. But since the MC gives an > overview of lots of other (sub)devices the refcount of the media device > should be increased for any (sub)device that adds itself to the MC and > decreased for any (sub)device that is removed. Only when the very last > user goes away can the MC memory be released. > > The memory/refcounting associated with device nodes is unrelated to this: > once a devnode is unregistered it will be removed in /dev, and once the > last open fh closes any memory associated with the devnode can be released. > That will also decrease the refcount to its parent device. > > This also means that it is a bad idea to embed devnodes in a larger struct. > They should be allocated and freed when the devnode is unregistered and > the last open filehandle is closed. > > Then the parent's device refcount is decreased, and that may now call its > release callback if the refcount reaches 0. > > For the media controller's device: any other device driver that needs access > to it needs to increase that device's refcount, and only when those devices > are released will they decrease the MC device's refcount. > > And when that refcount goes to 0 can we finally free everything. > > With regards to the opposition to reverting those initial patches, I'm > siding with Greg KH. Just revert the bloody patches. It worked most of the > time before those patches, so reverting really won't cause bisect problems. You're contradicting yourself here ;) The patches that this patch series is reverting are the ones that de-embeeds devnode struct and fixes its lifecycle. Reverting those patches will cause regressions on hot-pluggable drivers, preventing them to be unplugged. So, if we're willing to revert, then we should also revert MC support on them. > Just revert and build up things as they should. > > Note that v4l2-dev.c doesn't do things correctly (it doesn't set the cdev > parent pointer for example) and many drivers (including omap3isp) embed > video_device, which is wrong and can lead to complications. > > I'm to blame for the embedding since I thought that was a good idea at one > time. I now realized that it isn't. Sorry about that... > > And because the cdev of the video_device doesn't know about the parent > device it is (I think) possible that the parent device is released before > the cdev is released. Which can result in major problems. I agree with you here. IMHO, de-embeeding cdev's struct from video_device seems to be the right thing to do at V4L2 side too. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
