On Tue, Aug 19, 2025 at 11:42:11AM +0200, Luca Ceresoli wrote: > Between drm_bridge_add() and drm_bridge_remove() bridges are "published" to > the DRM core via the global bridge_list and visible in > /sys/kernel/debug/dri/bridges. However between drm_bridge_remove() and the > last drm_bridge_put() memory is still allocated even though the bridge is > not "published", i.e. not in bridges_list, and also not visible in > debugfs. This prevents debugging refcounted bridges lifetime, especially > leaks due to any missing drm_bridge_put(). > > In order to allow debugfs to also show the removed bridges, move such > bridges into a new ad-hoc list until they are eventually freed. > > Note this requires adding INIT_LIST_HEAD(&bridge->list) in the bridge > initialization code. The lack of such init was not exposing any bug so far, > but it would with the new code, for example when a bridge is allocated and > then freed without calling drm_bridge_add(), which is common on probe > errors. > > Document the new behaviour of drm_bridge_remove() and update the > drm_bridge_add() documentation to stay consistent. > > drm_bridge_add() needs special care for bridges being added after having > been previously added and then removed. This happens for example for many > non-DCS DSI host bridge drivers like samsung-dsim which > drm_bridge_add/remove() themselves every time the DSI device does a DSI > attaches/detach. When the DSI device is hot-pluggable this happens multiple > times in the lifetime of the DSI host bridge. When this happens, the > bridge->list is found in the removed list, not at the initialized state as > drm_bridge_add() currently expects. > > Signed-off-by: Luca Ceresoli <luca.ceres...@bootlin.com> > > --- > > Changes in v7: > - rebase on current drm-misc-next > - remove if (drm_bridge_is_refcounted(bridge)), refcounting is now > mandatory > - add check to detect when re-adding a bridge that is in the removed list > - improve commit message > - fix typo > > This patch was added in v6. > --- > drivers/gpu/drm/drm_bridge.c | 30 +++++++++++++++++++++++++++--- > 1 file changed, 27 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index > 36e0829d25c29457cff5da5fec99646c74b6ad5a..2e688ee14b9efbc810bcdb0ab7ecd4b688be8299 > 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -197,15 +197,22 @@ > * driver. > */ > > +/* Protect bridge_list and bridge_removed_list */ > static DEFINE_MUTEX(bridge_lock); > static LIST_HEAD(bridge_list); > +static LIST_HEAD(bridge_removed_list);
I'm not super fond of "removed" here, it's ambiguous, especially since
the bridge wouldn't be considered as removed after the last put.
lingering maybe?
>
> static void __drm_bridge_free(struct kref *kref)
> {
> struct drm_bridge *bridge = container_of(kref, struct drm_bridge,
> refcount);
>
> + mutex_lock(&bridge_lock);
> + list_del(&bridge->list);
> + mutex_unlock(&bridge_lock);
> +
> if (bridge->funcs->destroy)
> bridge->funcs->destroy(bridge);
> +
> kfree(bridge->container);
> }
>
> @@ -275,6 +282,7 @@ void *__devm_drm_bridge_alloc(struct device *dev, size_t
> size, size_t offset,
> return ERR_PTR(-ENOMEM);
>
> bridge = container + offset;
> + INIT_LIST_HEAD(&bridge->list);
> bridge->container = container;
> bridge->funcs = funcs;
> kref_init(&bridge->refcount);
> @@ -288,10 +296,13 @@ void *__devm_drm_bridge_alloc(struct device *dev,
> size_t size, size_t offset,
> EXPORT_SYMBOL(__devm_drm_bridge_alloc);
>
> /**
> - * drm_bridge_add - add the given bridge to the global bridge list
> + * drm_bridge_add - publish a bridge
> *
> * @bridge: bridge control structure
> *
> + * Add the given bridge to the global list of "published" bridges, where
> + * they can be found by users via of_drm_find_bridge().
It's quite a change in semantics, at least in the doc. I believe it
should be a separate patch, since it's really more about updating the
drm_bridge_add / drm_bridge_remove doc than collecting the
removed-but-not-freed bridges.
Also, I'm not sure if it's more obvious here. The quotes around publish
kind of it to that too. Maybe using register / registration would make
it more obvious?
Maxime
signature.asc
Description: PGP signature
