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

[Liu Ying <victor.liu@nxp.com>]

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@bootlin.com/
>>>
>>>> (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.

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

Thanks.

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

-- 
Regards,
Liu Ying