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

[Liu Ying]

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.

> 
> [v2] https://lore.kernel.org/all/20251003-drm-bridge-alloc-encoder-chain-mutex-v2-4-78bf61580a06@xxxxxxxxxxx/
> [v4] https://lore.kernel.org/all/20260113-drm-bridge-alloc-encoder-chain-mutex-v4-4-60f3135adc45@xxxxxxxxxxx/
> 
> Luca
> 
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com/

-- 
Regards,
Liu Ying