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

[Andrzej Hajda]

On 30.11.2016 10:39, Laurent Pinchart wrote:
> 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.

Could you be more precise. What exactly prevents DSI or component
framework from being safe, or where do you see big complexity?
It is of course more complex than current 'solution' but as we know
current 'solution' is broken.

On the other side, why do you think refcounting solution is better? Even
if refcounting is done properly, it will generate problems
with handling 'ghost' objects detached from the bridge, what if the
bridge re-appears, etc... Handling all this properly will complicate
code much more.
But most importantly lack of notification to upstream about broken
pipeline looks like broken design.

Regards
Andrzej