[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

Claude review: drm/bridge: Fix refcount shown via debugfs for encoder_bridges_show()

[Claude Code Review Bot]

Overall Series Review

Subject: drm/bridge: Fix refcount shown via debugfs for encoder_bridges_show()
Author: Liu Ying <victor.liu@nxp.com>
Patches: 2
Reviewed: 2026-03-13T14:28:00.934943

---

This is a single-patch fix for a debugfs display issue where `encoder_bridges_show()` reports an inflated refcount (4 instead of the expected 3) because `drm_for_each_bridge_in_chain_scoped()` holds an extra reference during iteration. The bug analysis is correct and the fix works, but the implementation is over-engineered — it adds two wrapper functions and a `bool scoped` parameter when a simpler approach would suffice.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: Fix refcount shown via debugfs for encoder_bridges_show()

[Claude Code Review Bot]

Patch Review

**Correctness: The fix is correct.** The `drm_for_each_bridge_in_chain_scoped()` macro (defined at `include/drm/drm_bridge.h:1490`) calls `drm_bridge_chain_get_first_bridge()` and `drm_bridge_get_next_bridge_and_put()`, holding an extra reference on the current bridge during each iteration. This inflates the `kref_read()` value by 1 in the debugfs output.

**Style nit — prefer `refcount - 1` over `--refcount`:**

```c
drm_printf(p, "\trefcount: %u%s\n",
           scoped ? --refcount : refcount,
           lingering ? " [lingering]" : "");
```

Using pre-decrement (`--refcount`) on a local variable is unnecessarily side-effectful. Since `refcount` is never used again, it works, but `refcount - 1` would be clearer and more idiomatic for a display-only adjustment.

**Over-engineering concern:** The patch adds 20+ lines (a `__` prefixed inner function, two wrappers, and a new `bool scoped` parameter) to fix a one-line display issue. A simpler approach would avoid the extra abstraction entirely. For example, `encoder_bridges_show()` could just call the existing function and the refcount adjustment could be handled more locally, or the original function could simply take the offset as a parameter:

```c
static void drm_bridge_debugfs_show_bridge(struct drm_printer *p,
                                           struct drm_bridge *bridge,
                                           unsigned int idx,
                                           bool lingering,
                                           unsigned int refcount_adj)
{
    unsigned int refcount = kref_read(&bridge->refcount) - refcount_adj;
    ...
```

This avoids needing both a `__` function and two thin wrappers, and it's more transparent about what's happening (subtracting an adjustment rather than a boolean that means "subtract 1").

Alternatively, the simplest possible fix would be to just adjust the refcount inline in `encoder_bridges_show()` without changing the helper at all — though that would mean duplicating the format string.

**Conceptual question:** Debugfs is a debugging interface. Arguably, showing the *actual* refcount (including the iterator's reference) is the most truthful thing to do, and the "expected" value of 3 is a convention, not a contract. A comment in the output or in the code explaining the +1 from the scoped iterator might be more appropriate than silently adjusting the value. That said, consistency between `allbridges_show` and `encoder_bridges_show` for the same bridge is a reasonable goal, so the fix is defensible.

**Summary:** The fix is correct and addresses a real inconsistency. Recommend simplifying the implementation (drop the double-wrapper pattern, use `refcount - 1` instead of `--refcount`) before merging.

---
Generated by Claude Code Patch Reviewer