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

[Luca Ceresoli]

On Fri Mar 13, 2026 at 9:33 AM CET, Liu Ying wrote:
> On Thu, Mar 12, 2026 at 06:30:22PM +0100, Luca Ceresoli wrote:
>> Hello Liu, Maxime,
>>
>> On Thu Mar 12, 2026 at 7:05 AM CET, Liu Ying wrote:
>>> A typical bridge refcount value is 3 after a bridge chain is formed:
>>> - devm_drm_bridge_alloc() initializes the refcount value to be 1.
>>> - drm_bridge_add() gets an additional reference hence 2.
>>> - drm_bridge_attach() gets the third reference hence 3.
>>>
>>> This typical refcount value aligns with allbridges_show()'s behaviour.
>>> However, since encoder_bridges_show() uses
>>> drm_for_each_bridge_in_chain_scoped() to automatically get/put the
>>> bridge reference while iterating, a bogus reference is accidentally
>>> got when showing the wrong typical refcount value as 4 to users via
>>> debugfs.  Fix this by caching the refcount value returned from
>>> kref_read() while iterating and explicitly decreasing the cached
>>> refcount value by 1 before showing it to users.
>>
>> Good point, indeed the refcount shown by
>> <debugfs>/dri/<card>/encoder-0/bridges is by one unit higher than the one
>> shown in <debugfs>/dri/bridges. I understand it's puzzling from a debugfs
>> user point of view.
>>
>> As you noticed, this is because the _scoped loop holds an extra ref on the
>> current bridge.
>>
>> For other reasons I proposed a mutex for stronger protection around the
>> bridge chain [v2]. With the mutex the extra ref is redundant, so in [v2]
>> the extra ref is removed, thus making your patch unneeded. However Maxime
>> asked to keep the extra ref, and so my latest iteration [v4] still has the
>> extra ref.
>>
>> That series is still on the mailing list, we are still in time to rediscuss
>> it.
>>
>> @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).

(*) by "necessary" I mean it's necessary to support a bridge chain that
changes after the card is populated, i.e. for bridge hotplug.


Luca

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