Several drivers (about 20) follow the same pattern:

 1. get a pointer to a bridge (typically the next bridge in the chain) by
    calling of_drm_find_bridge()
 2. store the returned pointer in the private driver data, keep it until
    driver .remove
 3. dereference the pointer at attach time and possibly at other times

of_drm_find_bridge() is now deprecated because it does not increment the
refcount and should be replaced with drm_of_find_bridge() +
drm_bridge_put().

However some of those drivers have a complex code flow and adding a
drm_bridge_put() call in all the appropriate locations is error-prone,
leads to ugly and more complex code, and can lead to errors over time with
code flow changes.

To handle all those drivers in a straightforward way, add a devm variant of
drm_of_find_bridge() that adds a devm action to invoke drm_bridge_put()
when the said driver is removed. This allows all those drivers to put the
reference automatically and safely with a one line change:

  - priv->next_bridge = of_drm_find_bridge(remote_np);
  + priv->next_bridge = devm_drm_of_find_bridge(dev, remote_np);

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h     |  5 +++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 09ad825f9cb8..c7baafbe5695 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -1446,6 +1446,36 @@ struct drm_bridge *drm_of_find_bridge(struct device_node *np)
 }
 EXPORT_SYMBOL(drm_of_find_bridge);
 
+/**
+ * devm_drm_of_find_bridge - find the bridge corresponding to the device
+ *			     node in the global bridge list and add a devm
+ *			     action to put it
+ *
+ * @dev: device requesting the bridge
+ * @np: device node
+ *
+ * On success the returned bridge refcount is incremented, and a devm
+ * action is added to call drm_bridge_put() when @dev is removed. So the
+ * caller does not have to put the returned bridge explicitly.
+ *
+ * RETURNS:
+ * drm_bridge control struct on success, NULL on failure
+ */
+struct drm_bridge *devm_drm_of_find_bridge(struct device *dev, struct device_node *np)
+{
+	struct drm_bridge *bridge = drm_of_find_bridge(np);
+
+	if (bridge) {
+		int err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge);
+
+		if (err)
+			return ERR_PTR(err);
+	}
+
+	return bridge;
+}
+EXPORT_SYMBOL(devm_drm_of_find_bridge);
+
 /**
  * of_drm_find_bridge - find the bridge corresponding to the device node in
  *			the global bridge list
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index e74e91004c48..98d5433f7d35 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1314,12 +1314,17 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 
 #ifdef CONFIG_OF
 struct drm_bridge *drm_of_find_bridge(struct device_node *np);
+struct drm_bridge *devm_drm_of_find_bridge(struct device *dev, struct device_node *np);
 struct drm_bridge *of_drm_find_bridge(struct device_node *np);
 #else
 static inline struct drm_bridge *drm_of_find_bridge(struct device_node *np)
 {
 	return NULL;
 }
+static inline struct drm_bridge *devm_drm_of_find_bridge(struct device *dev, struct device_node *np)
+{
+	return NULL;
+}
 static inline struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 {
 	return NULL;

-- 
2.51.1

On Wed, Nov 19, 2025 at 02:05:37PM +0100, Luca Ceresoli wrote:
> Several drivers (about 20) follow the same pattern:
> 
>  1. get a pointer to a bridge (typically the next bridge in the chain) by
>     calling of_drm_find_bridge()
>  2. store the returned pointer in the private driver data, keep it until
>     driver .remove
>  3. dereference the pointer at attach time and possibly at other times
> 
> of_drm_find_bridge() is now deprecated because it does not increment the
> refcount and should be replaced with drm_of_find_bridge() +
> drm_bridge_put().
> 
> However some of those drivers have a complex code flow and adding a
> drm_bridge_put() call in all the appropriate locations is error-prone,
> leads to ugly and more complex code, and can lead to errors over time with
> code flow changes.
> 
> To handle all those drivers in a straightforward way, add a devm variant of
> drm_of_find_bridge() that adds a devm action to invoke drm_bridge_put()
> when the said driver is removed. This allows all those drivers to put the
> reference automatically and safely with a one line change:
> 
>   - priv->next_bridge = of_drm_find_bridge(remote_np);
>   + priv->next_bridge = devm_drm_of_find_bridge(dev, remote_np);
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>
> ---
>  drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++
>  include/drm/drm_bridge.h     |  5 +++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 09ad825f9cb8..c7baafbe5695 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -1446,6 +1446,36 @@ struct drm_bridge *drm_of_find_bridge(struct device_node *np)
>  }
>  EXPORT_SYMBOL(drm_of_find_bridge);
>  
> +/**
> + * devm_drm_of_find_bridge - find the bridge corresponding to the device
> + *			     node in the global bridge list and add a devm
> + *			     action to put it
> + *
> + * @dev: device requesting the bridge
> + * @np: device node
> + *
> + * On success the returned bridge refcount is incremented, and a devm
> + * action is added to call drm_bridge_put() when @dev is removed. So the
> + * caller does not have to put the returned bridge explicitly.
> + *
> + * RETURNS:
> + * drm_bridge control struct on success, NULL on failure
> + */
> +struct drm_bridge *devm_drm_of_find_bridge(struct device *dev, struct device_node *np)
> +{
> +	struct drm_bridge *bridge = drm_of_find_bridge(np);
> +
> +	if (bridge) {
> +		int err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge);
> +
> +		if (err)
> +			return ERR_PTR(err);
> +	}
> +
> +	return bridge;
> +}
> +EXPORT_SYMBOL(devm_drm_of_find_bridge);

That's inherently unsafe though, because even if the bridge is removed
other parts of DRM might still have a reference to it and could call
into it.

We'd then have dropped our reference to the next bridge, which could
have been freed, and it's a use-after-free.

It's more complicated than it sounds, because we only have access to the
drm_device when the bridge is attached, so later than probe.

I wonder if we shouldn't tie the lifetime of that reference to the
lifetime of the bridge itself, and we would give up the next_bridge
reference only when we're destroyed ourselves.

Storing a list of all the references we need to drop is going to be
intrusive though, so maybe the easiest way to do it would be to create a
next_bridge field in drm_bridge, and only drop the reference stored
there?

And possibly tie the whole thing together using a helper?

Anyway, I'm not sure it should be a prerequisite to this series. I we do
want to go the devm_drm_of_find_bridge route however, we should at least
document that it's unsafe, and add a TODO entry to clean up the mess
later on.

Maxime

Hi Maxime,

On Mon Nov 24, 2025 at 11:39 AM CET, Maxime Ripard wrote:
> On Wed, Nov 19, 2025 at 02:05:37PM +0100, Luca Ceresoli wrote:
>> Several drivers (about 20) follow the same pattern:
>>
>>  1. get a pointer to a bridge (typically the next bridge in the chain) by
>>     calling of_drm_find_bridge()
>>  2. store the returned pointer in the private driver data, keep it until
>>     driver .remove
>>  3. dereference the pointer at attach time and possibly at other times
>>
>> of_drm_find_bridge() is now deprecated because it does not increment the
>> refcount and should be replaced with drm_of_find_bridge() +
>> drm_bridge_put().
>>
>> However some of those drivers have a complex code flow and adding a
>> drm_bridge_put() call in all the appropriate locations is error-prone,
>> leads to ugly and more complex code, and can lead to errors over time with
>> code flow changes.
>>
>> To handle all those drivers in a straightforward way, add a devm variant of
>> drm_of_find_bridge() that adds a devm action to invoke drm_bridge_put()
>> when the said driver is removed. This allows all those drivers to put the
>> reference automatically and safely with a one line change:
>>
>>   - priv->next_bridge = of_drm_find_bridge(remote_np);
>>   + priv->next_bridge = devm_drm_of_find_bridge(dev, remote_np);
>>
>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++
>>  include/drm/drm_bridge.h     |  5 +++++
>>  2 files changed, 35 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 09ad825f9cb8..c7baafbe5695 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -1446,6 +1446,36 @@ struct drm_bridge *drm_of_find_bridge(struct device_node *np)
>>  }
>>  EXPORT_SYMBOL(drm_of_find_bridge);
>>
>> +/**
>> + * devm_drm_of_find_bridge - find the bridge corresponding to the device
>> + *			     node in the global bridge list and add a devm
>> + *			     action to put it
>> + *
>> + * @dev: device requesting the bridge
>> + * @np: device node
>> + *
>> + * On success the returned bridge refcount is incremented, and a devm
>> + * action is added to call drm_bridge_put() when @dev is removed. So the
>> + * caller does not have to put the returned bridge explicitly.
>> + *
>> + * RETURNS:
>> + * drm_bridge control struct on success, NULL on failure
>> + */
>> +struct drm_bridge *devm_drm_of_find_bridge(struct device *dev, struct device_node *np)
>> +{
>> +	struct drm_bridge *bridge = drm_of_find_bridge(np);
>> +
>> +	if (bridge) {
>> +		int err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge);
>> +
>> +		if (err)
>> +			return ERR_PTR(err);
>> +	}
>> +
>> +	return bridge;
>> +}
>> +EXPORT_SYMBOL(devm_drm_of_find_bridge);
>
> That's inherently unsafe though, because even if the bridge is removed
> other parts of DRM might still have a reference to it and could call
> into it.
>
> We'd then have dropped our reference to the next bridge, which could
> have been freed, and it's a use-after-free.

I think you refer to this scenario:

  1. pipeline: encoder --> bridge A --> bridge B --> bridge C
  2. encoder takes a reference to bridge B
     using devm_drm_of_find_bridge() or other means
  3. bridge B takes a next_bridge reference to bridge C
     using devm_drm_of_find_bridge()
  4. encoder calls (bridge B)->foo(), which in turns references
     next_bridge, e.g.:

       b_foo() {
           bar(b->next_bridge);
       }

If bridges B and C are removed, bridge C can be freed but B is still
allocated because the encoder holds a ref. So when step 4 happens, 'b->c'
would be a use-after-free (or NULL deref if b.remove cleared it, which is
just as bad).

If I got you correctly, then I'm a bit surprised by your comment. This
series is part of the first chapter of the hotplug work, which does not aim
at fixing everything but rather at fixing one part: handle dynamic
_allocation_ lifetime of drm_bridges by adding a refcount and
drm_bridge_get/put().

Chapter 2 of the work is adding drm_bridge_enter/exit/unplug() [1] and
other changes in order to avoid code of drivers of removed bridges to
access fields they shouldn't. So the above example at point 4 would become:

       b_foo() {
           if (!drm_bridge_enter())
               return;
           bar(b->c);
           drm_bridge_exit();
       }

And that avoids 'b->c' after bridge B is removed.

Does that answer your remark?

> It's more complicated than it sounds, because we only have access to the
> drm_device when the bridge is attached, so later than probe.
>
> I wonder if we shouldn't tie the lifetime of that reference to the
> lifetime of the bridge itself, and we would give up the next_bridge
> reference only when we're destroyed ourselves.

I'm afraid I'm not following you, sorry. Do you refer to the time between
the bridge removal (driver .remove) and the last bridge put (when
deallocation happens)?

In that time frame the struct drm_bridge is still allocated along with any
next_bridge pointer it may contain, but the following bridge could have
been deallocated.

What do you mean by "give up the next_bridge"?

> Storing a list of all the references we need to drop is going to be
> intrusive though, so maybe the easiest way to do it would be to create a
> next_bridge field in drm_bridge, and only drop the reference stored
> there?
>
> And possibly tie the whole thing together using a helper?
>
> Anyway, I'm not sure it should be a prerequisite to this series. I we do
> want to go the devm_drm_of_find_bridge route however, we should at least
> document that it's unsafe, and add a TODO entry to clean up the mess
> later on.

Do you mean the drm variant is unsafe while the original
(drm_of_find_bridge() in this series, might be renamed) is not? I don't see
how that can happen.  If the driver for bridge B were to use
drm_of_find_bridge(), that driver would be responsible to
drm_bridge_put(b->next_bridge) in its .remove() function or earlier. So the
next_bridge pointing to bridge C would equally become subject to
use-after-free. devm does not make it worse, on the opposite it postpones
the drm_bridge_put(next_bridge) as late as possible: just after b.remove().

One final, high-level thought about the various 'next_bridge' pointers that
many bridge drivers have. Most of them do:

 0. have a 'struct drm_bridge next_bridge *' in their private struct
 1. take the next_bridge reference during probe or another startup phase
 2. store it in their private driver struct
 3. use it to call drm_bridge_attach
 4. (pending) put the reference to it in their .remove or earlier

I'm wondering whether we could let the DRM bridge core do it all, by
removing items 0, 1, 2 and 4, and change 3 as:

-     drm_bridge_attach(encoder, me->next_bridge, &me->bridge, flags);
+  drm_of_bridge_attach(encoder, &me->bridge, dev->of_node, 1, -1, flags);

where dev->of_node and the following integers are the same flags passed to
devm_drm_of_get_bridge() and the like, i.e. the endpoint info needed to
walk the DT graph and reach the next bridge.

This would allow the core to take care of all locking and lifetime of the
next bridge, and most (all?) bridges would never access any pointers to the
next bridge. The idea is to let the core do the right thing in a single
place instead of trying to make all drivers do the right thing (and
touching dozen files when needing to touch the logic).

That is more a long-term ideal than something I'd do right now, but having
opinions would be very interesting.

[1] https://lore.kernel.org/lkml/20251112-drm-bridge-atomic-vs-remove-v3-0-85db717ce094@bootlin.com/

Best regards,
Luca

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

On Mon, Nov 24, 2025 at 05:25:39PM +0100, Luca Ceresoli wrote:
> Hi Maxime,
> 
> On Mon Nov 24, 2025 at 11:39 AM CET, Maxime Ripard wrote:
> > On Wed, Nov 19, 2025 at 02:05:37PM +0100, Luca Ceresoli wrote:
> >> Several drivers (about 20) follow the same pattern:
> >>
> >>  1. get a pointer to a bridge (typically the next bridge in the chain) by
> >>     calling of_drm_find_bridge()
> >>  2. store the returned pointer in the private driver data, keep it until
> >>     driver .remove
> >>  3. dereference the pointer at attach time and possibly at other times
> >>
> >> of_drm_find_bridge() is now deprecated because it does not increment the
> >> refcount and should be replaced with drm_of_find_bridge() +
> >> drm_bridge_put().
> >>
> >> However some of those drivers have a complex code flow and adding a
> >> drm_bridge_put() call in all the appropriate locations is error-prone,
> >> leads to ugly and more complex code, and can lead to errors over time with
> >> code flow changes.
> >>
> >> To handle all those drivers in a straightforward way, add a devm variant of
> >> drm_of_find_bridge() that adds a devm action to invoke drm_bridge_put()
> >> when the said driver is removed. This allows all those drivers to put the
> >> reference automatically and safely with a one line change:
> >>
> >>   - priv->next_bridge = of_drm_find_bridge(remote_np);
> >>   + priv->next_bridge = devm_drm_of_find_bridge(dev, remote_np);
> >>
> >> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> >>
> >> ---
> >>  drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++
> >>  include/drm/drm_bridge.h     |  5 +++++
> >>  2 files changed, 35 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> >> index 09ad825f9cb8..c7baafbe5695 100644
> >> --- a/drivers/gpu/drm/drm_bridge.c
> >> +++ b/drivers/gpu/drm/drm_bridge.c
> >> @@ -1446,6 +1446,36 @@ struct drm_bridge *drm_of_find_bridge(struct device_node *np)
> >>  }
> >>  EXPORT_SYMBOL(drm_of_find_bridge);
> >>
> >> +/**
> >> + * devm_drm_of_find_bridge - find the bridge corresponding to the device
> >> + *			     node in the global bridge list and add a devm
> >> + *			     action to put it
> >> + *
> >> + * @dev: device requesting the bridge
> >> + * @np: device node
> >> + *
> >> + * On success the returned bridge refcount is incremented, and a devm
> >> + * action is added to call drm_bridge_put() when @dev is removed. So the
> >> + * caller does not have to put the returned bridge explicitly.
> >> + *
> >> + * RETURNS:
> >> + * drm_bridge control struct on success, NULL on failure
> >> + */
> >> +struct drm_bridge *devm_drm_of_find_bridge(struct device *dev, struct device_node *np)
> >> +{
> >> +	struct drm_bridge *bridge = drm_of_find_bridge(np);
> >> +
> >> +	if (bridge) {
> >> +		int err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge);
> >> +
> >> +		if (err)
> >> +			return ERR_PTR(err);
> >> +	}
> >> +
> >> +	return bridge;
> >> +}
> >> +EXPORT_SYMBOL(devm_drm_of_find_bridge);
> >
> > That's inherently unsafe though, because even if the bridge is removed
> > other parts of DRM might still have a reference to it and could call
> > into it.
> >
> > We'd then have dropped our reference to the next bridge, which could
> > have been freed, and it's a use-after-free.
> 
> I think you refer to this scenario:
> 
>   1. pipeline: encoder --> bridge A --> bridge B --> bridge C
>   2. encoder takes a reference to bridge B
>      using devm_drm_of_find_bridge() or other means
>   3. bridge B takes a next_bridge reference to bridge C
>      using devm_drm_of_find_bridge()
>   4. encoder calls (bridge B)->foo(), which in turns references
>      next_bridge, e.g.:
> 
>        b_foo() {
>            bar(b->next_bridge);
>        }
> 
> If bridges B and C are removed, bridge C can be freed but B is still
> allocated because the encoder holds a ref. So when step 4 happens, 'b->c'
> would be a use-after-free (or NULL deref if b.remove cleared it, which is
> just as bad).

Yep.

> If I got you correctly, then I'm a bit surprised by your comment. This
> series is part of the first chapter of the hotplug work, which does not aim
> at fixing everything but rather at fixing one part: handle dynamic
> _allocation_ lifetime of drm_bridges by adding a refcount and
> drm_bridge_get/put().
> 
> Chapter 2 of the work is adding drm_bridge_enter/exit/unplug() [1] and
> other changes in order to avoid code of drivers of removed bridges to
> access fields they shouldn't. So the above example at point 4 would become:
> 
>        b_foo() {
>            if (!drm_bridge_enter())
>                return;
>            bar(b->c);
>            drm_bridge_exit();
>        }
> 
> And that avoids 'b->c' after bridge B is removed.
> 
> Does that answer your remark?

Not really. I wasn't really questionning your current focus, or the way
you laid out the current agenda or whatever.

What I am questionning though is whether or not we want to introduce
something we will have to untangle soon, and even more so when we're not
mentioning it anywhere.

> > It's more complicated than it sounds, because we only have access to the
> > drm_device when the bridge is attached, so later than probe.
> >
> > I wonder if we shouldn't tie the lifetime of that reference to the
> > lifetime of the bridge itself, and we would give up the next_bridge
> > reference only when we're destroyed ourselves.
> 
> I'm afraid I'm not following you, sorry. Do you refer to the time between
> the bridge removal (driver .remove) and the last bridge put (when
> deallocation happens)?
> 
> In that time frame the struct drm_bridge is still allocated along with any
> next_bridge pointer it may contain, but the following bridge could have
> been deallocated.
> 
> What do you mean by "give up the next_bridge"?

What I was trying to say was that if we want to fix the problem you
illustrated about, we need to give up the reference at __drm_bridge_free
time. So each bridge having a reference to a bridge would need to do so
in its destroy hook.

Since it's quite a common pattern, it would make sense to add a
next_bridge field to drm_bridge itself, so the core can do it
automatically in __drm_bridge_free if that pointer is !NULL.

But...

> > Storing a list of all the references we need to drop is going to be
> > intrusive though, so maybe the easiest way to do it would be to create a
> > next_bridge field in drm_bridge, and only drop the reference stored
> > there?
> >
> > And possibly tie the whole thing together using a helper?
> >
> > Anyway, I'm not sure it should be a prerequisite to this series. I we do
> > want to go the devm_drm_of_find_bridge route however, we should at least
> > document that it's unsafe, and add a TODO entry to clean up the mess
> > later on.

... I *really* don't consider it something you need to work on right now.

> Do you mean the drm variant is unsafe while the original
> (drm_of_find_bridge() in this series, might be renamed) is not? I
> don't see how that can happen. If the driver for bridge B were to use
> drm_of_find_bridge(), that driver would be responsible to
> drm_bridge_put(b->next_bridge) in its .remove() function or earlier.
> So the next_bridge pointing to bridge C would equally become subject
> to use-after-free.

No, I was saying that both are equally unsafe. But we're adding a new,
broken, helper, and we don't mention anywhere that it is. So what I was
saying is mostly do we really want to introduce some more broken code
when we know it is. And if we do, we should be really clear about it.

> devm does not make it worse, on the opposite it postpones the
> drm_bridge_put(next_bridge) as late as possible: just after
> b.remove().

Which doesn't really change anything, does it? I'd expect the window
between the remove and final drm_bridge_put to be much wider than the
execution time of remove itself.

> One final, high-level thought about the various 'next_bridge' pointers that
> many bridge drivers have. Most of them do:
> 
>  0. have a 'struct drm_bridge next_bridge *' in their private struct
>  1. take the next_bridge reference during probe or another startup phase
>  2. store it in their private driver struct
>  3. use it to call drm_bridge_attach
>  4. (pending) put the reference to it in their .remove or earlier
> 
> I'm wondering whether we could let the DRM bridge core do it all, by
> removing items 0, 1, 2 and 4, and change 3 as:
> 
> -     drm_bridge_attach(encoder, me->next_bridge, &me->bridge, flags);
> +  drm_of_bridge_attach(encoder, &me->bridge, dev->of_node, 1, -1, flags);
> 
> where dev->of_node and the following integers are the same flags passed to
> devm_drm_of_get_bridge() and the like, i.e. the endpoint info needed to
> walk the DT graph and reach the next bridge.
> 
> This would allow the core to take care of all locking and lifetime of the
> next bridge, and most (all?) bridges would never access any pointers to the
> next bridge. The idea is to let the core do the right thing in a single
> place instead of trying to make all drivers do the right thing (and
> touching dozen files when needing to touch the logic).
> 
> That is more a long-term ideal than something I'd do right now, but having
> opinions would be very interesting.

That was pretty much my point, yeah.

Maxime

On 11/19/25 13:05, Luca Ceresoli wrote:
> Several drivers (about 20) follow the same pattern:
> 
>   1. get a pointer to a bridge (typically the next bridge in the chain) by
>      calling of_drm_find_bridge()
>   2. store the returned pointer in the private driver data, keep it until
>      driver .remove
>   3. dereference the pointer at attach time and possibly at other times
> 
> of_drm_find_bridge() is now deprecated because it does not increment the
> refcount and should be replaced with drm_of_find_bridge() +
> drm_bridge_put().
> 
> However some of those drivers have a complex code flow and adding a
> drm_bridge_put() call in all the appropriate locations is error-prone,
> leads to ugly and more complex code, and can lead to errors over time with
> code flow changes.
> 
> To handle all those drivers in a straightforward way, add a devm variant of
> drm_of_find_bridge() that adds a devm action to invoke drm_bridge_put()
> when the said driver is removed. This allows all those drivers to put the
> reference automatically and safely with a one line change:
> 
>    - priv->next_bridge = of_drm_find_bridge(remote_np);
>    + priv->next_bridge = devm_drm_of_find_bridge(dev, remote_np);
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
>   drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++
>   include/drm/drm_bridge.h     |  5 +++++
>   2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 09ad825f9cb8..c7baafbe5695 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -1446,6 +1446,36 @@ struct drm_bridge *drm_of_find_bridge(struct device_node *np)
>   }
>   EXPORT_SYMBOL(drm_of_find_bridge);
>   
> +/**
> + * devm_drm_of_find_bridge - find the bridge corresponding to the device
> + *			     node in the global bridge list and add a devm
> + *			     action to put it
> + *
> + * @dev: device requesting the bridge
> + * @np: device node
> + *
> + * On success the returned bridge refcount is incremented, and a devm
> + * action is added to call drm_bridge_put() when @dev is removed. So the
> + * caller does not have to put the returned bridge explicitly.
> + *
> + * RETURNS:
> + * drm_bridge control struct on success, NULL on failure

I am not sure for the "NULL on failure", you return ERR_PTR(err), which 
is probably not NULL but an error code.

> + */
> +struct drm_bridge *devm_drm_of_find_bridge(struct device *dev, struct device_node *np)
> +{
> +	struct drm_bridge *bridge = drm_of_find_bridge(np);
> +
> +	if (bridge) {
> +		int err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge);
> +
> +		if (err)
> +			return ERR_PTR(err);
> +	}
> +
> +	return bridge;
> +}
> +EXPORT_SYMBOL(devm_drm_of_find_bridge);
> +
>   /**
>    * of_drm_find_bridge - find the bridge corresponding to the device node in
>    *			the global bridge list
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index e74e91004c48..98d5433f7d35 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -1314,12 +1314,17 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>   
>   #ifdef CONFIG_OF
>   struct drm_bridge *drm_of_find_bridge(struct device_node *np);
> +struct drm_bridge *devm_drm_of_find_bridge(struct device *dev, struct device_node *np);
>   struct drm_bridge *of_drm_find_bridge(struct device_node *np);
>   #else
>   static inline struct drm_bridge *drm_of_find_bridge(struct device_node *np)
>   {
>   	return NULL;
>   }
> +static inline struct drm_bridge *devm_drm_of_find_bridge(struct device *dev, struct device_node *np)
> +{
> +	return NULL;
> +}
>   static inline struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>   {
>   	return NULL;
>

Hi Louis,

On Wed Nov 19, 2025 at 3:33 PM CET, Louis Chauvet wrote:
>
>
> On 11/19/25 13:05, Luca Ceresoli wrote:
>> Several drivers (about 20) follow the same pattern:
>>
>>   1. get a pointer to a bridge (typically the next bridge in the chain) by
>>      calling of_drm_find_bridge()
>>   2. store the returned pointer in the private driver data, keep it until
>>      driver .remove
>>   3. dereference the pointer at attach time and possibly at other times
>>
>> of_drm_find_bridge() is now deprecated because it does not increment the
>> refcount and should be replaced with drm_of_find_bridge() +
>> drm_bridge_put().
>>
>> However some of those drivers have a complex code flow and adding a
>> drm_bridge_put() call in all the appropriate locations is error-prone,
>> leads to ugly and more complex code, and can lead to errors over time with
>> code flow changes.
>>
>> To handle all those drivers in a straightforward way, add a devm variant of
>> drm_of_find_bridge() that adds a devm action to invoke drm_bridge_put()
>> when the said driver is removed. This allows all those drivers to put the
>> reference automatically and safely with a one line change:
>>
>>    - priv->next_bridge = of_drm_find_bridge(remote_np);
>>    + priv->next_bridge = devm_drm_of_find_bridge(dev, remote_np);
>>
>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>> ---
>>   drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++
>>   include/drm/drm_bridge.h     |  5 +++++
>>   2 files changed, 35 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 09ad825f9cb8..c7baafbe5695 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -1446,6 +1446,36 @@ struct drm_bridge *drm_of_find_bridge(struct device_node *np)
>>   }
>>   EXPORT_SYMBOL(drm_of_find_bridge);
>>
>> +/**
>> + * devm_drm_of_find_bridge - find the bridge corresponding to the device
>> + *			     node in the global bridge list and add a devm
>> + *			     action to put it
>> + *
>> + * @dev: device requesting the bridge
>> + * @np: device node
>> + *
>> + * On success the returned bridge refcount is incremented, and a devm
>> + * action is added to call drm_bridge_put() when @dev is removed. So the
>> + * caller does not have to put the returned bridge explicitly.
>> + *
>> + * RETURNS:
>> + * drm_bridge control struct on success, NULL on failure
>
> I am not sure for the "NULL on failure", you return ERR_PTR(err), which
> is probably not NULL but an error code.

Indeed.

Apologies for the mess in this series: it was adapted from an old one using
a different approach, so I had to adapt lots of details, and missed a few
along the way. :(

About the value to return, maybe it's better to use the same semantics as
drm_of_find_bridge(), i.e. NULL on error. I don't think a caller would have
anything clever to do with an error return value other tan bailing out. And
the only error path for devm_add_action_or_reset() is on a small
allocation, so it basically cannot happen.

>> +struct drm_bridge *devm_drm_of_find_bridge(struct device *dev, struct device_node *np)
>> +{
>> +	struct drm_bridge *bridge = drm_of_find_bridge(np);
>> +
>> +	if (bridge) {
>> +		int err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge);
>> +
>> +		if (err)
>> +			return ERR_PTR(err);
>> +	}

So this would become:

	if (bridge) {
		if (devm_add_action_or_reset(dev, drm_bridge_put_void, bridge))
			return NULL;
	}

>> +
>> +	return bridge;
>> +}

Luca

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