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

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

[Liu Ying]

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@nxp.com>
---
 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,
 		   lingering ? " [lingering]" : "");
 
 	drm_printf(p, "\ttype: [%d] %s\n",
@@ -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);
+}
+
 static int allbridges_show(struct seq_file *m, void *data)
 {
 	struct drm_printer p = drm_seq_file_printer(m);
@@ -1626,7 +1646,7 @@ static int encoder_bridges_show(struct seq_file *m, void *data)
 	unsigned int idx = 0;
 
 	drm_for_each_bridge_in_chain_scoped(encoder, bridge)
-		drm_bridge_debugfs_show_bridge(&p, bridge, idx++, false);
+		drm_bridge_debugfs_show_bridge_scoped(&p, bridge, idx++, false);
 
 	return 0;
 }

---
base-commit: d2e20c8951e4bb5f4a828aed39813599980353b6
change-id: 20260312-drm-misc-next-2026-03-05-fix-encoder-bridges-refcount-8f8ee3f58339

Best regards,
-- 
Liu Ying <victor.liu@nxp.com>

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

[Luca Ceresoli]

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.

[v2] https://lore.kernel.org/all/20251003-drm-bridge-alloc-encoder-chain-mutex-v2-4-78bf61580a06@bootlin.com/
[v4] https://lore.kernel.org/all/20260113-drm-bridge-alloc-encoder-chain-mutex-v4-4-60f3135adc45@bootlin.com/

Luca

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

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@bootlin.com/
> [v4] https://lore.kernel.org/all/20260113-drm-bridge-alloc-encoder-chain-mutex-v4-4-60f3135adc45@bootlin.com/
> 
> Luca
> 
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com/

-- 
Regards,
Liu Ying

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

[Luca Ceresoli]

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

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

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

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

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

[Luca Ceresoli]

On Fri Mar 13, 2026 at 11:22 AM CET, Liu Ying wrote:
> 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

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

Luca

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

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

[Liu Ying]

On Fri, Mar 13, 2026 at 06:32:46PM +0100, Luca Ceresoli wrote:
> On Fri Mar 13, 2026 at 11:22 AM CET, Liu Ying wrote:
>> 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
> 
> 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.  Even if
we end up with the first option, the refcount is supposed to be correct
anyway.

Luca, do you think this patch deserves at least an A-b tag from you if not
a R-b tag?

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

-- 
Regards,
Liu Ying

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

[Luca Ceresoli]

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.

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

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

Luca

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

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

[Liu Ying]

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

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

[Luca Ceresoli]

Hello Liu,

On Tue Mar 17, 2026 at 3:04 AM CET, Liu Ying wrote:
> 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.

Ah, yes, sure.

Luca

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

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

[Luca Ceresoli]

Hello Liu,

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

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

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

Best regards,
Luca

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

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

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