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

[Luca Ceresoli]

On Tue Mar 17, 2026 at 3:35 AM CET, Liu Ying wrote:
> On Mon, Mar 16, 2026 at 12:15:29PM +0100, Luca Ceresoli wrote:
>> Hello Liu,
>
> Hello Luca,
>
>>
>> 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.
>>>
>>> Fixes: bd57048e4576 ("drm/bridge: use drm_for_each_bridge_in_chain_scoped()")
>>> Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
>>> ---
>>>  drivers/gpu/drm/drm_bridge.c | 32 ++++++++++++++++++++++++++------
>>>  1 file changed, 26 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>> index f8b0333a0a3b..84fc3cfd17e0 100644
>>> --- a/drivers/gpu/drm/drm_bridge.c
>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>> @@ -1567,14 +1567,18 @@ void devm_drm_put_bridge(struct device *dev, struct drm_bridge *bridge)
>>>  }
>>>  EXPORT_SYMBOL(devm_drm_put_bridge);
>>>
>>> -static void drm_bridge_debugfs_show_bridge(struct drm_printer *p,
>>> -					   struct drm_bridge *bridge,
>>> -					   unsigned int idx,
>>> -					   bool lingering)
>>> +static void __drm_bridge_debugfs_show_bridge(struct drm_printer *p,
>>> +					     struct drm_bridge *bridge,
>>> +					     unsigned int idx,
>>> +					     bool lingering,
>>> +					     bool scoped)
>>>  {
>>> +	unsigned int refcount = kref_read(&bridge->refcount);
>>> +
>>>  	drm_printf(p, "bridge[%u]: %ps\n", idx, bridge->funcs);
>>>
>>> -	drm_printf(p, "\trefcount: %u%s\n", kref_read(&bridge->refcount),
>>> +	drm_printf(p, "\trefcount: %u%s\n",
>>> +		   scoped ? --refcount : refcount,
>>
>> I'd s/--refcount/refcount - 1/ here, no point in modifying the value while
>> printing it.
>
> Well, maybe there is a point if we consider 'scoped == true', which means
> one reference should be dropped from the refcount.  In the future, if the
> refcount is used in this function multiple times, then we don't need to
> do 'refcount - 1' for each time with '--refcount'.  But, for now, since
> the refcount is just used for one time in this function, I'm fine with
> either '--refcount' or 'refcount - 1', please let me know your preference.

My preference is to not use the '--' operator. I tend to avoid it in
function/macros parameters because it can be tricky with macros, and I
admit I had to double check to find out drm_printf() is not a macro (but it
could become at some point).

So my preference is for 'refcount - 1'. Or, if you prefer, decrement just
after the assignment:

	unsigned int refcount = kref_read(&bridge->refcount);
+	refcount = scoped ? refcount - 1 : refcount;

But anyway this is a minor detail, go for whatever seems best to you.

>>> @@ -1599,6 +1603,22 @@ static void drm_bridge_debugfs_show_bridge(struct drm_printer *p,
>>>  	drm_puts(p, "\n");
>>>  }
>>>
>>> +static void drm_bridge_debugfs_show_bridge(struct drm_printer *p,
>>> +					   struct drm_bridge *bridge,
>>> +					   unsigned int idx,
>>> +					   bool lingering)
>>> +{
>>> +	__drm_bridge_debugfs_show_bridge(p, bridge, idx, lingering, false);
>>> +}
>>> +
>>> +static void drm_bridge_debugfs_show_bridge_scoped(struct drm_printer *p,
>>> +						  struct drm_bridge *bridge,
>>> +						  unsigned int idx,
>>> +						  bool lingering)
>>> +{
>>> +	__drm_bridge_debugfs_show_bridge(p, bridge, idx, lingering, true);
>>> +}
>>
>> I think this should be much simpler and avoid a lot of the boilerplate
>> code: just add a 'bool scoped' argument to drm_bridge_debugfs_show_bridge()
>> and pass true/false as applicable.
>
> Hm, I was thinking how to avoid the two bool arguments(lingering and
> scoped) for drm_bridge_debugfs_show_bridge(), because they make a function
> call look ugly - people have to go back to the function declaration to
> check which bool argument is which.  So, I came up with the boilerplate
> code, at least any function call has just one 'true' or 'false'.  I'm open
> to any better idea.  If you insist on adding a 'bool scoped' argument to
> drm_bridge_debugfs_show_bridge() is a good way to go, then I accept that
> and would follow - let me know your thoughts.

Here I really think adding a dozen lines of boilerplate code for such a
simple think is bad for maintainability/readability. The code is simple
enough that two bools (or 1 bool + an int offset) will be readable. If/when
the needs will become more complex, code can be made more sophisticated
accordingly.

Luca

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