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

[Luca Ceresoli]

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.

> 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.

> Luca, do you think this patch deserves at least an A-b tag from you if not
> a R-b tag?

I've been waiting a bit in case Maxime or someone else wanted to chime
in. I'm reviewing it now.

Luca

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