[PATCH] drm/drm_bridge: adjust bridge module's refcount

[Laurent Pinchart]

Hi Andrzej,

On Wednesday 30 Nov 2016 10:26:24 Andrzej Hajda wrote:
> On 30.11.2016 09:16, Laurent Pinchart wrote:
> > On Wednesday 30 Nov 2016 09:11:56 Andrzej Hajda wrote:
> >> On 29.11.2016 22:12, Jyri Sarha wrote:
> >>> Store the module that provides the bridge and adjust its refcount
> >>> accordingly. The bridge module unload should not be allowed while the
> >>> bridge is attached.
> >> 
> >> Module refcounting as a way of preventing driver from unbinding
> >> is quite popular, but as other developers said already is incorrect
> >> solution - it does not really prevent from unbinding and is a hack.
> > 
> > Absolutely, module refcounting must not be used as a way to prevent
> > unbinding, but it's still necessary to prevent functions from
> > disappearing. The unbinding race has to be fixed through reference
> > counting to prevent objects from being freed, but if an object contains
> > function pointers that refer to code part of a module, object refcounting
> > won't prevent the code from being removed. Only module refcounting helps
> > there. The two techniques are thus complementary.
>
> It is not true. There at least two existing and proper solutions, which
> do not use refcounting:
>
> 1. components - if you put the bridge into component framework, it will
> bring down drm device before detaching the bridge.

Don't get me started on that one. The component concept is fine, its 
implementation less so. It's on my (long) to-do list of things to fix.

> 2. proper callbacks from the provider (bridge in this case) to the
> consumer (encoder or drm device). Such callbacks exists for example in
> case of MIPI-DSI panels attached to encoders. On removal
> panel calls mipi_dsi_detach, which calls .detach ops in associated
> encoder, encoder safely disables the video pipeline and the panel
> continue its removal.

*safely* is the keyword here. I have yet to see a solution based on a removal 
notification callback that is both race-free and easy to use in drivers.

> Of course both solutions have some pitfalls, the first one removes whole
> drmdev instead of disabling one pipeline,

The DRM subsystem doesn't have hotplug support (except for displayed connected 
to connectors of course), let alone hot-unplug support. That should be fixed, 
but will be a very long term goal.

Regardless of that, the component framework also relies on a removal 
notification callback, which has the same drawback as the DSI one. Handling 
this in a race-free way is complex. Seeing how drivers fail at simple things 
(such as calling to drm_bridge_detach(), which all but one driver fail to do), 
I'd be surprised if even a single driver got the component unbind handling 
right.

> the second one is specific for DSI bus, but they clearly shows refcounting
> is not the only option.

Sorry, I meant the only working and (more or less) simple option ;-)

-- 
Regards,

Laurent Pinchart