Re: [PATCH] drm/bridge: Fix refcount shown via debugfs for encoder_bridges_show()

[Luca Ceresoli]

Hello Liu,

On Tue Mar 17, 2026 at 3:04 AM CET, Liu Ying wrote:
> On Mon, Mar 16, 2026 at 12:14:23PM +0100, Luca Ceresoli wrote:
>> Hello Liu, Maxime,
>>
>> On Mon Mar 16, 2026 at 10:47 AM CET, Liu Ying wrote:
>>>>>>>> @Maxime: based on the issue Liu is trying to work around, do you think it
>>>>>>>> would make sense to go back to the initial approach for that series?
>>>>>>>> I.e. drm_for_each_bridge_in_chain_scoped() grabs the chain lock, which is a
>>>>>>>> superset of the per-bridge refcount, and thus the refcount can be dropped?
>>>>>>>> This would remove the debugfs issue, slightly simplify
>>>>>>>> drm_for_each_bridge_in_chain_scoped(), and introduce no new issues AFAIK.
>>>>>>>
>>>>>>> Just my take on the chain lock approach - I agree Maxime's comment on [v2]
>>>>>>> that keeping the get/put is a better than using the chain lock to ensure
>>>>>>> the refcount is correct.  The chain lock could be added later on if needed.
>>>>>>
>>>>>> Well, no, adding the chain mutex is necessary(*), otherwise Thread A could
>>>>>> iterate over the chain while thread B is adding/removing bridges to/from
>>>>>> the chain.
>>>>>>
>>>>>> And the chain mutex is a superset of the per-bridge refcount, so when
>>>>>> adding the mutex the refcount inside drm_for_each_bridge_in_chain_scoped()
>>>>>> becomes useless (and slightly hurting as it makes the refcount shown in
>>>>>> debugfs inconsistent, as you noticed).
>>>>>
>>>>> For better code readability, I think keeping the get/put is fine even if
>>>>> you add a lock
>>>>
>>>> The [v4] code with the removal of the extra refcount would not be more
>>>> complex. It would be a bit less code (no need for the DEFINE_FREE and
>>>> __free()). Maybe it'd need an extra comment to clarify when the
>>>> drm_bridge_put() is called.
>>>>
>>>> [v4] https://lore.kernel.org/all/20260113-drm-bridge-alloc-encoder-chain-mutex-v4-4-60f3135adc45@xxxxxxxxxxx/
>>>>
>>>>> (maybe RCU list is better than mutex, since the chain is
>>>>> read often).  That follows the idea that you mentioned in [1]: "every
>>>>> pointer to a drm_bridge stored somewhere is a reference to a bridge".
>>>>
>>>> That's true. However while it's an important pointer hygiene rule for
>>>> device drivers, for core code it's OK to deviate when there is a reason.
>>>>
>>>>> Plus, seems no performance issue with the get/put, as discussed in [v2].
>>>>
>>>> I confirm performance is surely not an issue here.
>>>>
>>>> All that said, I'm OK with either option:
>>>>
>>>>  * no ref taken when the mutex is added
>>>>  * ref taken when the mutex is added (as v4) + your patch to fix debugfs
>>>
>>> Maybe consider to take this patch first, since it doesn't hurt.
>>
>> Yes, especially as the current debugfs output is non-intuitive.
>
> Agreed.
>
>>
>>> Even if
>>> we end up with the first option, the refcount is supposed to be correct
>>> anyway.
>>
>> Well, if we apply this patch and then go for option 1 then this patch shall
>> be removed, or the refcount shown would be one-less than the expected
>> value, instead of one-more as it is now.
>
> I meant that if we go for option 1, then a single patch may introduce the
> protection for the chain with the mutex/RCU list(whatever), plus remove
> the change done by this patch.  This way, the refcount would be consistent
> over time.

Ah, yes, sure.

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com