[PATCH 0/3] drm/bridge: get/put the bridge reference in drm_bridge.c, warn on old alloc pattern

[PATCH 0/3] drm/bridge: get/put the bridge reference in drm_bridge.c, warn on old alloc pattern

[Luca Ceresoli]

This small series adds drm_bridge_get/put() calls where applicable in the
bridge core code, and adds a warning when the old allocation patter is
used.

This is part of the work towards removal of bridges from a still existing
DRM pipeline without use-after-free.

The steps in the grand plan [1] are:

 1. ➜ add refcounting to DRM bridges (struct drm_bridge)
 2. handle gracefully atomic updates during bridge removal
 3. avoid DSI host drivers to have dangling pointers to DSI devices
 4. finish the hotplug bridge work, removing the "always-disconnected"
    connector, moving code to the core and potentially removing the
    hotplug-bridge itself (this needs to be clarified as points 1-3 are
    developed)

This series is part of step 1 of the grand plan.

Current tasks in step 1 of the grand plan:

 A. ✔ add new alloc API and refcounting (in v6.16-rc1)
 B. ✔ convert all bridge drivers to new API (now in drm-misc-next)
 C. ✔ kunit tests (now in drm-misc-next)
 D. ➜ after (B), add get/put to drm_bridge_add/remove() + attach/detech()
      and warn on old allocation pattern (this series)
 E. after (B), convert accessors; this is a large work and can be done
    in chunks
 F. debugfs improvements

Specifically, this series does two things related to the recently
introduced devm_drm_bridge_alloc() [0]:

 A) add drm_bridge_get/put() in the two places within drm_bridge.c where a
    bridge reference is taken/relased
 B) add a warning on bridges using the old allocation pattern instead of
    devm_drm_bridge_alloc()

These are loosely related topics, but I'm sending them in a single series
to avoid trivial patch conflicts, as they touch the same lines. This will
holefully help getting the patches applied more efficiently and quickly. I
hope this is fine being 3 small patches.

Part A) is made of patches 1 and 2. Those have been previously sent in
"[PATCH v7 00/11] drm/bridge: add devm_drm_bridge_alloc() with bridge
refcount" [2] (as patches 3 and 4), and already Reviewed-by Maxime at that
time. However they could not be applied until the conversion of all bridge
drivers to devm_drm_bridge_alloc(), which got completed a few days
ago. Hence the reason this is v8.

[0] https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/0cc6aadd7fc1e629b715ea3d1ba537ef2da95eec
[1] https://lore.kernel.org/lkml/20250206-hotplug-drm-bridge-v6-0-9d6f2c9c3058@bootlin.com/t/#u
[2] https://lore.kernel.org/all/20250314-drm-bridge-refcount-v7-0-152571f8c694@bootlin.com/

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Luca Ceresoli (3):
      drm/bridge: get/put the bridge reference in drm_bridge_add/remove()
      drm/bridge: get/put the bridge reference in drm_bridge_attach/detach()
      drm/bridge: add warning for bridges not using devm_drm_bridge_alloc()

 drivers/gpu/drm/drm_bridge.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)
---
base-commit: a59a271769149f0b8258507276f3d2a24370cbdb
change-id: 20250620-drm-bridge-alloc-getput-drm-bridge-c-0ab2ca481c37

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>

[PATCH 1/3] drm/bridge: get/put the bridge reference in drm_bridge_add/remove()

[Luca Ceresoli]

drm_bridge_add() adds the bridge to the global bridge_list, so take a
reference for that. Vice versa in drm_bridge_remove().

Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---

Changes in v8: none

Changes in v7:
- in v6 this was part of "drm/bridge: add support for refcounted DRM
  bridges", now split to a separate patch
---
 drivers/gpu/drm/drm_bridge.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index d6ce7b4c019f415400bab8aa3d032638cba6cdc5..fa2b2457b16e145e3ace70c53984937096d310d0 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -295,6 +295,8 @@ EXPORT_SYMBOL(__devm_drm_bridge_alloc);
  */
 void drm_bridge_add(struct drm_bridge *bridge)
 {
+	drm_bridge_get(bridge);
+
 	mutex_init(&bridge->hpd_mutex);
 
 	if (bridge->ops & DRM_BRIDGE_OP_HDMI)
@@ -342,6 +344,8 @@ void drm_bridge_remove(struct drm_bridge *bridge)
 	mutex_unlock(&bridge_lock);
 
 	mutex_destroy(&bridge->hpd_mutex);
+
+	drm_bridge_put(bridge);
 }
 EXPORT_SYMBOL(drm_bridge_remove);
 

-- 
2.49.0

[PATCH 2/3] drm/bridge: get/put the bridge reference in drm_bridge_attach/detach()

[Luca Ceresoli]

drm_bridge_attach() adds the bridge to the encoder chain, so take a
reference for that. Vice versa in drm_bridge_detach().

Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---

Changes in v8: none

Changes in v7:
- in v6 this was part of "drm/bridge: add support for refcounted DRM
  bridges", now split to a separate patch
---
 drivers/gpu/drm/drm_bridge.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index fa2b2457b16e145e3ace70c53984937096d310d0..f001bbe95559aabf0aac9f25f89250ad4e1ad9c8 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -411,11 +411,17 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 	if (!encoder || !bridge)
 		return -EINVAL;
 
-	if (previous && (!previous->dev || previous->encoder != encoder))
-		return -EINVAL;
+	drm_bridge_get(bridge);
 
-	if (bridge->dev)
-		return -EBUSY;
+	if (previous && (!previous->dev || previous->encoder != encoder)) {
+		ret = -EINVAL;
+		goto err_put_bridge;
+	}
+
+	if (bridge->dev) {
+		ret = -EBUSY;
+		goto err_put_bridge;
+	}
 
 	bridge->dev = encoder->dev;
 	bridge->encoder = encoder;
@@ -464,6 +470,8 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 			      "failed to attach bridge %pOF to encoder %s\n",
 			      bridge->of_node, encoder->name);
 
+err_put_bridge:
+	drm_bridge_put(bridge);
 	return ret;
 }
 EXPORT_SYMBOL(drm_bridge_attach);
@@ -484,6 +492,7 @@ void drm_bridge_detach(struct drm_bridge *bridge)
 
 	list_del(&bridge->chain_node);
 	bridge->dev = NULL;
+	drm_bridge_put(bridge);
 }
 
 /**

-- 
2.49.0

[PATCH 3/3] drm/bridge: add warning for bridges not using devm_drm_bridge_alloc()

[Luca Ceresoli]

To the best of my knowledge, all drivers in the mainline kernel adding a
DRM bridge are now converted to using devm_drm_bridge_alloc() for
allocation and initialization. Among others this ensures initialization of
the bridge refcount, allowing dynamic allocation lifetime.

devm_drm_bridge_alloc() is now mandatory for all new bridges. Code using
the old pattern ([devm_]kzalloc + filling the struct fields +
drm_bridge_add) is not allowed anymore.

Any drivers that might have been missed during the conversion, patches in
flight towards mainline and out-of-tre drivers still using the old pattern
will already be caught by a warning looking like:

  ------------[ cut here ]------------
  refcount_t: addition on 0; use-after-free.
  WARNING: CPU: 2 PID: 83 at lib/refcount.c:25 refcount_warn_saturate+0x120/0x148
  [...]
  Call trace:
   refcount_warn_saturate+0x120/0x148 (P)
   drm_bridge_get.part.0+0x70/0x98 [drm]
   drm_bridge_add+0x34/0x108 [drm]
   sn65dsi83_probe+0x200/0x480 [ti_sn65dsi83]
   [...]

This warning comes from the refcount code and happens because
drm_bridge_add() is increasing the refcount, which is uninitialized and
thus initially zero.

Having a warning and the corresponding stack trace is surely useful, but
the warning text does not clarify the root problem nor how to fix it.

Add a DRM_WARN() just before increasing the refcount, so the log will be
much more readable:

  [drm] DRM bridge corrupted or not allocated by devm_drm_bridge_alloc()
  ------------[ cut here ]------------
  refcount_t: addition on 0; use-after-free.
  [...etc...]

A DRM_WARN is used because drm_warn and drm_WARN require a struct
drm_device pointer which is not yet available when adding a bridge.

Do not print the dev_name() in the warning because struct drm_bridge has no
pointer to the struct device. The affected driver should be easy to catch
based on the following stack trace however.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

This patch was added in v8
---
 drivers/gpu/drm/drm_bridge.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index f001bbe95559aabf0aac9f25f89250ad4e1ad9c8..7d511c28608f1d8ea8fbb81d00efa9e227b02a13 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -295,6 +295,9 @@ EXPORT_SYMBOL(__devm_drm_bridge_alloc);
  */
 void drm_bridge_add(struct drm_bridge *bridge)
 {
+	if (kref_read(&bridge->refcount) == 0)
+		DRM_WARN("DRM bridge corrupted or not allocated by devm_drm_bridge_alloc()\n");
+
 	drm_bridge_get(bridge);
 
 	mutex_init(&bridge->hpd_mutex);

-- 
2.49.0

Re: [PATCH 3/3] drm/bridge: add warning for bridges not using devm_drm_bridge_alloc()

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2965 bytes --]

Hi Luca,

On Fri, Jun 20, 2025 at 11:32:08AM +0200, Luca Ceresoli wrote:
> To the best of my knowledge, all drivers in the mainline kernel adding a
> DRM bridge are now converted to using devm_drm_bridge_alloc() for
> allocation and initialization. Among others this ensures initialization of
> the bridge refcount, allowing dynamic allocation lifetime.
> 
> devm_drm_bridge_alloc() is now mandatory for all new bridges. Code using
> the old pattern ([devm_]kzalloc + filling the struct fields +
> drm_bridge_add) is not allowed anymore.
> 
> Any drivers that might have been missed during the conversion, patches in
> flight towards mainline and out-of-tre drivers still using the old pattern
> will already be caught by a warning looking like:
> 
>   ------------[ cut here ]------------
>   refcount_t: addition on 0; use-after-free.
>   WARNING: CPU: 2 PID: 83 at lib/refcount.c:25 refcount_warn_saturate+0x120/0x148
>   [...]
>   Call trace:
>    refcount_warn_saturate+0x120/0x148 (P)
>    drm_bridge_get.part.0+0x70/0x98 [drm]
>    drm_bridge_add+0x34/0x108 [drm]
>    sn65dsi83_probe+0x200/0x480 [ti_sn65dsi83]
>    [...]
> 
> This warning comes from the refcount code and happens because
> drm_bridge_add() is increasing the refcount, which is uninitialized and
> thus initially zero.
> 
> Having a warning and the corresponding stack trace is surely useful, but
> the warning text does not clarify the root problem nor how to fix it.
> 
> Add a DRM_WARN() just before increasing the refcount, so the log will be
> much more readable:
> 
>   [drm] DRM bridge corrupted or not allocated by devm_drm_bridge_alloc()
>   ------------[ cut here ]------------
>   refcount_t: addition on 0; use-after-free.
>   [...etc...]
> 
> A DRM_WARN is used because drm_warn and drm_WARN require a struct
> drm_device pointer which is not yet available when adding a bridge.
> 
> Do not print the dev_name() in the warning because struct drm_bridge has no
> pointer to the struct device. The affected driver should be easy to catch
> based on the following stack trace however.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> This patch was added in v8
> ---
>  drivers/gpu/drm/drm_bridge.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index f001bbe95559aabf0aac9f25f89250ad4e1ad9c8..7d511c28608f1d8ea8fbb81d00efa9e227b02a13 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -295,6 +295,9 @@ EXPORT_SYMBOL(__devm_drm_bridge_alloc);
>   */
>  void drm_bridge_add(struct drm_bridge *bridge)
>  {
> +	if (kref_read(&bridge->refcount) == 0)
> +		DRM_WARN("DRM bridge corrupted or not allocated by devm_drm_bridge_alloc()\n");
> +

I'm fine with it on principle, but I wonder if using bridge->container
is set wouldn't be a more obvious way to check it?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 3/3] drm/bridge: add warning for bridges not using devm_drm_bridge_alloc()

[Luca Ceresoli]

Hi Maxime,

On Fri, 20 Jun 2025 13:41:48 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> Hi Luca,
> 
> On Fri, Jun 20, 2025 at 11:32:08AM +0200, Luca Ceresoli wrote:
> > To the best of my knowledge, all drivers in the mainline kernel adding a
> > DRM bridge are now converted to using devm_drm_bridge_alloc() for
> > allocation and initialization. Among others this ensures initialization of
> > the bridge refcount, allowing dynamic allocation lifetime.
> > 
> > devm_drm_bridge_alloc() is now mandatory for all new bridges. Code using
> > the old pattern ([devm_]kzalloc + filling the struct fields +
> > drm_bridge_add) is not allowed anymore.
> > 
> > Any drivers that might have been missed during the conversion, patches in
> > flight towards mainline and out-of-tre drivers still using the old pattern
> > will already be caught by a warning looking like:
> > 
> >   ------------[ cut here ]------------
> >   refcount_t: addition on 0; use-after-free.
> >   WARNING: CPU: 2 PID: 83 at lib/refcount.c:25 refcount_warn_saturate+0x120/0x148
> >   [...]
> >   Call trace:
> >    refcount_warn_saturate+0x120/0x148 (P)
> >    drm_bridge_get.part.0+0x70/0x98 [drm]
> >    drm_bridge_add+0x34/0x108 [drm]
> >    sn65dsi83_probe+0x200/0x480 [ti_sn65dsi83]
> >    [...]
> > 
> > This warning comes from the refcount code and happens because
> > drm_bridge_add() is increasing the refcount, which is uninitialized and
> > thus initially zero.
> > 
> > Having a warning and the corresponding stack trace is surely useful, but
> > the warning text does not clarify the root problem nor how to fix it.
> > 
> > Add a DRM_WARN() just before increasing the refcount, so the log will be
> > much more readable:
> > 
> >   [drm] DRM bridge corrupted or not allocated by devm_drm_bridge_alloc()
> >   ------------[ cut here ]------------
> >   refcount_t: addition on 0; use-after-free.
> >   [...etc...]
> > 
> > A DRM_WARN is used because drm_warn and drm_WARN require a struct
> > drm_device pointer which is not yet available when adding a bridge.
> > 
> > Do not print the dev_name() in the warning because struct drm_bridge has no
> > pointer to the struct device. The affected driver should be easy to catch
> > based on the following stack trace however.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > 
> > ---
> > 
> > This patch was added in v8
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index f001bbe95559aabf0aac9f25f89250ad4e1ad9c8..7d511c28608f1d8ea8fbb81d00efa9e227b02a13 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -295,6 +295,9 @@ EXPORT_SYMBOL(__devm_drm_bridge_alloc);
> >   */
> >  void drm_bridge_add(struct drm_bridge *bridge)
> >  {
> > +	if (kref_read(&bridge->refcount) == 0)
> > +		DRM_WARN("DRM bridge corrupted or not allocated by devm_drm_bridge_alloc()\n");
> > +  
> 
> I'm fine with it on principle, but I wonder if using bridge->container
> is set wouldn't be a more obvious way to check it?

Good point. Indeed the refcount check is potentially fragile because by
its own nature the refcount gets changed at various moments by various
parts of the kernel, unlike the container which is supposed to be never
modified after allocation.

I'll change this for v9.

Ouch, I just realized this does not show as v8 as it should (see cover
letter). Sorry about that, I messed up with my b4 branches, fixing both
things in, well, v9.

Luca

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