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

[Liu Ying]

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.

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

> 
> Or maybe an 'int offset' with an integer to be subtracted from the refcount
> for diplaying, but that's probably overkill.

Yes, that's overkill at least for now.  Dropping just one reference from
the refcount is currently enough.

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

-- 
Regards,
Liu Ying