Re: [PATCH 06/26] drm/bridge: add devm_drm_of_find_bridge

Maxime Ripard
2025-11-24T10:39:40Z

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

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

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