Re: [RFC v3 00/21] Make use of kref in media device, grab references as needed

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Thursday 15 Dec 2016 13:45:52 Mauro Carvalho Chehab wrote:
> Em Thu, 15 Dec 2016 15:45:22 +0100
> 
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > On 15/12/16 15:32, Mauro Carvalho Chehab wrote:
> > > Em Thu, 15 Dec 2016 15:03:36 +0100
> > > 
> > > Hans Verkuil <hverkuil@xs4all.nl> 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.
> > 
> > Two options:
> > 
> > 1) Revert, then build up a proper solution.
> 
> Reverting is a regression, as we'll strip off the MC support from the
> existing devices. We would also need to revert a lot more than just those
> 3 patches.

It's not a regression for all the drivers that were already broken before. It 
can be considered as a regression for the drivers that have been broken 
afterwards (as far as I understand that's several USB drivers that you and 
Shuah have migrated to MC in the past few months, but I haven't followed 
driver work closely enough to name them), and I would certainly not oppose to 
additional patches being reverted for those drivers.

> > 2) Do a big-bang patch switching directly over to the new solution, but
> > that's very hard to review.
> > 2a) Post the patch series in small chunks on the mailinglist (starting
> > with the reverts), but once we're all happy merge that patch series into
> > a single big-bang patch and apply that.
> 
> We could do that, but so far, what has been submitted are incomplete,
> as they only touch on a single driver (with doesn't require hot-plugging),
> breaking all the other ones.
> 
> > As far as I am concerned the whole hotplugging code is broken and has been
> > for a very long time. We (or at least I :-) ) understand the underlying
> > concepts a lot better, so we can do a better job. But the transition may
> > well be painful.
> 
> It is not broken currently on the devices that require hotplugging.

It is. The problem is in the core as Sakari as proven multiple times. The race 
window might be smaller than it used to be, but the base on top of which our 
drivers are built have degraded in a pretty terrible way over the last year. 
We need to fix it before it collapses, which requires solving the problem 
correctly before building anything else on top of it.

-- 
Regards,

Laurent Pinchart