| Message ID | 20250314-drm-bridge-refcount-v7-2-152571f8c694@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/bridge: add devm_drm_bridge_alloc() with bridge refcount | expand |
On Fri, Mar 14, 2025 at 11:31:15AM +0100, Luca Ceresoli wrote: > DRM bridges are currently considered as a fixed element of a DRM card, and > thus their lifetime is assumed to extend for as long as the card > exists. New use cases, such as hot-pluggable hardware with video bridges, > require DRM bridges to be added to and removed from a DRM card without > tearing the card down. This is possible for connectors already (used by DP > MST), it is now needed for DRM bridges as well. > > As a first preliminary step, make bridges reference-counted to allow a > struct drm_bridge (along with the private driver structure embedding it) to > stay allocated even after the driver has been removed, until the last > reference is put. > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > --- > > Changes in v7: > - export drm_bridge_put_void > - struct drm_bridge: use container pointer instead of container_offset > - remove drm_bridge_is_refcounted() > - remove all DRM_DEBUG()s > - drm_bridge_get/put: accept NULL pointer and return the bridge pointer to > allow pass-through calls > - extract to separate patches: > - the addition of drm_bridge_alloc > - the addition of drm_bridge_get/put() to drm_bridge_add/remove() > - the addition of drm_bridge_get/put() to drm_bridge_attach/detach() > - fix a typo, slightly improve kerneldoc > > Changes in v6: > - use drm_warn, not WARN_ON (Jani Nikula) > - Add devm_drm_bridge_alloc() to replace drm_bridge_init() (similar to > drmm_encoder_alloc) > - Remove .destroy func: deallocation is done via the struct offset > computed by the devm_drm_bridge_alloc() macro > - use fixed free callback, as the same callback is used in all cases > anyway (remove free_cb, add bool is_refcounted) > - add drm_bridge_get/put() to drm_bridge_attach/detach() (add the bridge > to a list) > - make some DRM_DEBUG() strings more informative > > This patch was added in v5. > --- > drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++- > include/drm/drm_bridge.h | 91 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 123 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 96df717b2caeb41d45346ded576eaeb2806fd051..2ba0dac9bfc2dfd709d5e2457d69067c7324972c 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -199,23 +199,54 @@ > static DEFINE_MUTEX(bridge_lock); > static LIST_HEAD(bridge_list); > > +void __drm_bridge_free(struct kref *kref) > +{ > + struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount); > + > + kfree(bridge->container); > +} > +EXPORT_SYMBOL(__drm_bridge_free); > + > +/** > + * drm_bridge_put_void - wrapper to drm_bridge_put() taking a void pointer > + * > + * @data: pointer to @struct drm_bridge, cast to a void pointer > + * > + * Wrapper of drm_bridge_put() to be used when a function taking a void > + * pointer is needed, for example as a devm action. > + */ > +void drm_bridge_put_void(void *data) > +{ > + struct drm_bridge *bridge = (struct drm_bridge *)data; > + > + drm_bridge_put(bridge); > +} > +EXPORT_SYMBOL(drm_bridge_put_void); > + > void *__devm_drm_bridge_alloc(struct device *dev, size_t size, size_t offset, > const struct drm_bridge_funcs *funcs) > { > void *container; > struct drm_bridge *bridge; > + int err; > > if (!funcs) { > dev_warn(dev, "Missing funcs pointer\n"); > return ERR_PTR(-EINVAL); > } > > - container = devm_kzalloc(dev, size, GFP_KERNEL); > + container = kzalloc(size, GFP_KERNEL); > if (!container) > return ERR_PTR(-ENOMEM); > > bridge = container + offset; > + bridge->container = container; > bridge->funcs = funcs; > + kref_init(&bridge->refcount); > + > + err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge); > + if (err) > + return ERR_PTR(err); > > return container; > } > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index dae463b30542d586a595b67f7bdf5a5e898e9572..5c1e2b9cafb12eb429d1f5d3ef312e6cf9b54f47 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -840,6 +840,17 @@ struct drm_bridge { > const struct drm_bridge_timings *timings; > /** @funcs: control functions */ > const struct drm_bridge_funcs *funcs; > + > + /** > + * @container: Pointer to the private driver struct embedding this > + * @struct drm_bridge. > + */ > + void *container; newline here > + /** > + * @refcount: reference count of users referencing this bridge. > + */ > + struct kref refcount; > + > /** @driver_private: pointer to the bridge driver's internal context */ > void *driver_private; > /** @ops: bitmask of operations supported by the bridge */ > @@ -941,6 +952,82 @@ drm_priv_to_bridge(struct drm_private_obj *priv) > return container_of(priv, struct drm_bridge, base); > } > > +void __drm_bridge_free(struct kref *kref); > + > +/** > + * drm_bridge_get - Acquire a bridge reference > + * @bridge: DRM bridge > + * > + * This function increments the bridge's refcount. > + * > + * Returns: > + * Pointer to @bridge. > + */ > +static inline struct drm_bridge *drm_bridge_get(struct drm_bridge *bridge) > +{ > + if (!bridge) > + return bridge; > + > + kref_get(&bridge->refcount); > + > + return bridge; > +} > + > +/** > + * drm_bridge_put - Release a bridge reference > + * @bridge: DRM bridge > + * > + * This function decrements the bridge's reference count and frees the > + * object if the reference count drops to zero. > + * > + * See also drm_bridge_put_and_clear() which is more handy in many cases. > + * > + * Returns: > + * Pointer to @bridge. > + */ > +static inline struct drm_bridge *drm_bridge_put(struct drm_bridge *bridge) > +{ > + if (!bridge) > + return bridge; > + > + kref_put(&bridge->refcount, __drm_bridge_free); > + > + return bridge; > +} I'm not sure we discussed it already, but why do you need to put both drm_bridge_get and drm_bridge_put into the header, and thus export __drm_bridge_free? I'd expect both to be in drm_bridge.c? > + > +void drm_bridge_put_void(void *data); > + > +/** > + * drm_bridge_put_and_clear - Given a bridge pointer, clear the pointer > + * then put the bridge > + * > + * @bridge_pp: pointer to pointer to a struct drm_bridge > + * > + * Helper to put a DRM bridge (whose pointer is passed), but only after > + * setting its pointer to NULL. Useful for drivers having struct drm_bridge > + * pointers they need to dispose of, without leaving a use-after-free > + * window where the pointed bridge might have been freed while still > + * holding a pointer to it. > + * > + * For example a driver having this private struct:: > + * > + * struct my_bridge { > + * struct drm_bridge *remote_bridge; > + * ... > + * }; > + * > + * can dispose of remote_bridge using:: > + * > + * drm_bridge_put_and_clear(&my_bridge->remote_bridge); > + */ > +static inline void drm_bridge_put_and_clear(struct drm_bridge **bridge_pp) > +{ > + struct drm_bridge *bridge = *bridge_pp; > + > + *bridge_pp = NULL; > + drm_bridge_put(bridge); > +} I'm not convinced we need that one for now, and it doesn't look like there's a user for it in your series, so I'd rather introduce it once we actually need it. > void *__devm_drm_bridge_alloc(struct device *dev, size_t size, size_t offset, > const struct drm_bridge_funcs *funcs); > > @@ -951,6 +1038,10 @@ void *__devm_drm_bridge_alloc(struct device *dev, size_t size, size_t offset, > * @member: the name of the &drm_bridge within @type > * @funcs: callbacks for this bridge > * > + * The returned refcount is initialized to 1. I'm confused, there's no returned refcount here? Or did you mean the returned bridge refcount? Maxime
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 96df717b2caeb41d45346ded576eaeb2806fd051..2ba0dac9bfc2dfd709d5e2457d69067c7324972c 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -199,23 +199,54 @@ static DEFINE_MUTEX(bridge_lock); static LIST_HEAD(bridge_list); +void __drm_bridge_free(struct kref *kref) +{ + struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount); + + kfree(bridge->container); +} +EXPORT_SYMBOL(__drm_bridge_free); + +/** + * drm_bridge_put_void - wrapper to drm_bridge_put() taking a void pointer + * + * @data: pointer to @struct drm_bridge, cast to a void pointer + * + * Wrapper of drm_bridge_put() to be used when a function taking a void + * pointer is needed, for example as a devm action. + */ +void drm_bridge_put_void(void *data) +{ + struct drm_bridge *bridge = (struct drm_bridge *)data; + + drm_bridge_put(bridge); +} +EXPORT_SYMBOL(drm_bridge_put_void); + void *__devm_drm_bridge_alloc(struct device *dev, size_t size, size_t offset, const struct drm_bridge_funcs *funcs) { void *container; struct drm_bridge *bridge; + int err; if (!funcs) { dev_warn(dev, "Missing funcs pointer\n"); return ERR_PTR(-EINVAL); } - container = devm_kzalloc(dev, size, GFP_KERNEL); + container = kzalloc(size, GFP_KERNEL); if (!container) return ERR_PTR(-ENOMEM); bridge = container + offset; + bridge->container = container; bridge->funcs = funcs; + kref_init(&bridge->refcount); + + err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge); + if (err) + return ERR_PTR(err); return container; } diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index dae463b30542d586a595b67f7bdf5a5e898e9572..5c1e2b9cafb12eb429d1f5d3ef312e6cf9b54f47 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -840,6 +840,17 @@ struct drm_bridge { const struct drm_bridge_timings *timings; /** @funcs: control functions */ const struct drm_bridge_funcs *funcs; + + /** + * @container: Pointer to the private driver struct embedding this + * @struct drm_bridge. + */ + void *container; + /** + * @refcount: reference count of users referencing this bridge. + */ + struct kref refcount; + /** @driver_private: pointer to the bridge driver's internal context */ void *driver_private; /** @ops: bitmask of operations supported by the bridge */ @@ -941,6 +952,82 @@ drm_priv_to_bridge(struct drm_private_obj *priv) return container_of(priv, struct drm_bridge, base); } +void __drm_bridge_free(struct kref *kref); + +/** + * drm_bridge_get - Acquire a bridge reference + * @bridge: DRM bridge + * + * This function increments the bridge's refcount. + * + * Returns: + * Pointer to @bridge. + */ +static inline struct drm_bridge *drm_bridge_get(struct drm_bridge *bridge) +{ + if (!bridge) + return bridge; + + kref_get(&bridge->refcount); + + return bridge; +} + +/** + * drm_bridge_put - Release a bridge reference + * @bridge: DRM bridge + * + * This function decrements the bridge's reference count and frees the + * object if the reference count drops to zero. + * + * See also drm_bridge_put_and_clear() which is more handy in many cases. + * + * Returns: + * Pointer to @bridge. + */ +static inline struct drm_bridge *drm_bridge_put(struct drm_bridge *bridge) +{ + if (!bridge) + return bridge; + + kref_put(&bridge->refcount, __drm_bridge_free); + + return bridge; +} + +void drm_bridge_put_void(void *data); + +/** + * drm_bridge_put_and_clear - Given a bridge pointer, clear the pointer + * then put the bridge + * + * @bridge_pp: pointer to pointer to a struct drm_bridge + * + * Helper to put a DRM bridge (whose pointer is passed), but only after + * setting its pointer to NULL. Useful for drivers having struct drm_bridge + * pointers they need to dispose of, without leaving a use-after-free + * window where the pointed bridge might have been freed while still + * holding a pointer to it. + * + * For example a driver having this private struct:: + * + * struct my_bridge { + * struct drm_bridge *remote_bridge; + * ... + * }; + * + * can dispose of remote_bridge using:: + * + * drm_bridge_put_and_clear(&my_bridge->remote_bridge); + */ +static inline void drm_bridge_put_and_clear(struct drm_bridge **bridge_pp) +{ + struct drm_bridge *bridge = *bridge_pp; + + *bridge_pp = NULL; + drm_bridge_put(bridge); +} + void *__devm_drm_bridge_alloc(struct device *dev, size_t size, size_t offset, const struct drm_bridge_funcs *funcs); @@ -951,6 +1038,10 @@ void *__devm_drm_bridge_alloc(struct device *dev, size_t size, size_t offset, * @member: the name of the &drm_bridge within @type * @funcs: callbacks for this bridge * + * The returned refcount is initialized to 1. This reference will be + * automatically dropped via devm (by calling drm_bridge_put()) when @dev + * is removed. + * * Returns: * Pointer to new bridge, or ERR_PTR on failure. */
DRM bridges are currently considered as a fixed element of a DRM card, and thus their lifetime is assumed to extend for as long as the card exists. New use cases, such as hot-pluggable hardware with video bridges, require DRM bridges to be added to and removed from a DRM card without tearing the card down. This is possible for connectors already (used by DP MST), it is now needed for DRM bridges as well. As a first preliminary step, make bridges reference-counted to allow a struct drm_bridge (along with the private driver structure embedding it) to stay allocated even after the driver has been removed, until the last reference is put. Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> --- Changes in v7: - export drm_bridge_put_void - struct drm_bridge: use container pointer instead of container_offset - remove drm_bridge_is_refcounted() - remove all DRM_DEBUG()s - drm_bridge_get/put: accept NULL pointer and return the bridge pointer to allow pass-through calls - extract to separate patches: - the addition of drm_bridge_alloc - the addition of drm_bridge_get/put() to drm_bridge_add/remove() - the addition of drm_bridge_get/put() to drm_bridge_attach/detach() - fix a typo, slightly improve kerneldoc Changes in v6: - use drm_warn, not WARN_ON (Jani Nikula) - Add devm_drm_bridge_alloc() to replace drm_bridge_init() (similar to drmm_encoder_alloc) - Remove .destroy func: deallocation is done via the struct offset computed by the devm_drm_bridge_alloc() macro - use fixed free callback, as the same callback is used in all cases anyway (remove free_cb, add bool is_refcounted) - add drm_bridge_get/put() to drm_bridge_attach/detach() (add the bridge to a list) - make some DRM_DEBUG() strings more informative This patch was added in v5. --- drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++- include/drm/drm_bridge.h | 91 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 1 deletion(-)