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

[Liu Ying]

On Fri, Mar 13, 2026 at 10:57:04AM +0100, Luca Ceresoli wrote:
> 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).

For better code readability, I think keeping the get/put is fine even if
you add a lock(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".
Plus, seems no performance issue with the get/put, as discussed in [v2].

[1] https://lore.kernel.org/all/DH0YC7M9YFU7.31DSHCPV381WC@xxxxxxxxxxx/

> 
> (*) 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/

-- 
Regards,
Liu Ying