[PATCH v7 00/11] drm/bridge: add devm_drm_bridge_alloc() with bridge refcount

[PATCH v7 00/11] drm/bridge: add devm_drm_bridge_alloc() with bridge refcount

[Luca Ceresoli]

This series improves the way DRM bridges are allocated and initialized and
makes them reference-counted. The goal of reference counting is to avoid
use-after-free by drivers which got a pointer to a bridge and keep it
stored and used even after the bridge has been deallocated.

The overall goal is supporting Linux devices with a DRM pipeline whose
final components can be hot-plugged and hot-unplugged, including one or
more bridges. For more details see the big picture [0].

DRM bridge drivers will have to be adapted to the new API, which is pretty
simple for most cases. Refcounting will have to be adopted on the two
sides: all functions returning a bridge pointer and all code obtaining such
a pointer. This series has just an overview of some of those conversions,
because for now the main goal is to agree on the API.

Series layout:

 1. Add the new API and refcounting:

    drm/bridge: add devm_drm_bridge_alloc()
    drm/bridge: add support for refcounting

 2. get/put the reference in basic operations in the bridge core:

    drm/bridge: get/put the bridge reference in drm_bridge_add/remove()
    drm/bridge: get/put the bridge reference in drm_bridge_attach/detach()

 3. as an example of changes for bridge consumers, get a reference for the
    bridge returned by drm_bridge_chain_get_first_bridge(), have it put by
    all callers (all users will be covered later on separately):

    drm/bridge: add a cleanup action for scope-based drm_bridge_put() invocation
    drm/bridge: get the bridge returned by drm_bridge_chain_get_first_bridge()
    drm/mxsfb: put the bridge returned by drm_bridge_chain_get_first_bridge()
    drm/atomic-helper: put the bridge returned by drm_bridge_chain_get_first_bridge()
    drm/probe-helper: put the bridge returned by drm_bridge_chain_get_first_bridge()

 4. convert a few bridge drivers (bridge providers) to the new API:

    drm/bridge: ti-sn65dsi83: use dynamic lifetime management
    drm/bridge: samsung-dsim: use dynamic lifetime management

This work was formerly a part of my v6 DRM bridge hotplug series[0], now
split as a standalone series with many improvements, hence the "v7" version
number.

[0] https://lore.kernel.org/dri-devel/20250206-hotplug-drm-bridge-v6-0-9d6f2c9c3058@bootlin.com/

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Luca Ceresoli (11):
      drm/bridge: add devm_drm_bridge_alloc()
      drm/bridge: add support for refcounting
      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 a cleanup action for scope-based drm_bridge_put() invocation
      drm/bridge: get the bridge returned by drm_bridge_chain_get_first_bridge()
      drm/mxsfb: put the bridge returned by drm_bridge_chain_get_first_bridge()
      drm/atomic-helper: put the bridge returned by drm_bridge_chain_get_first_bridge()
      drm/probe-helper: put the bridge returned by drm_bridge_chain_get_first_bridge()
      drm/bridge: ti-sn65dsi83: use dynamic lifetime management
      drm/bridge: samsung-dsim: use dynamic lifetime management

 drivers/gpu/drm/bridge/samsung-dsim.c |   7 +-
 drivers/gpu/drm/bridge/ti-sn65dsi83.c |   7 +-
 drivers/gpu/drm/drm_atomic_helper.c   |   5 ++
 drivers/gpu/drm/drm_bridge.c          |  74 +++++++++++++++++++--
 drivers/gpu/drm/drm_probe_helper.c    |   1 +
 drivers/gpu/drm/mxsfb/lcdif_kms.c     |   3 +-
 include/drm/drm_bridge.h              | 119 +++++++++++++++++++++++++++++++++-
 7 files changed, 201 insertions(+), 15 deletions(-)
---
base-commit: 9e6d91c60b0d64a4f945663993b3bbf4f3fb7392
change-id: 20250314-drm-bridge-refcount-58d9503503f6

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

[PATCH v7 01/11] drm/bridge: add devm_drm_bridge_alloc()

[Luca Ceresoli]

Add a macro to allocate and initialize a DRM bridge embedded within a
private driver struct.

Compared to current practice, which is based on [devm_]kzalloc() allocation
followed by open-coded initialization of fields, this allows to have a
common and explicit API to allocate and initialize DRM bridges.

Besides being useful to consolidate bridge driver code, this is a
fundamental step in preparation for adding dynamic lifetime to bridges
based on refcount.

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

---

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 | 22 ++++++++++++++++++++++
 include/drm/drm_bridge.h     | 17 +++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index ea9525ec16b5272d12a21a66913ada38e74e80bc..96df717b2caeb41d45346ded576eaeb2806fd051 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -199,6 +199,28 @@
 static DEFINE_MUTEX(bridge_lock);
 static LIST_HEAD(bridge_list);
 
+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;
+
+	if (!funcs) {
+		dev_warn(dev, "Missing funcs pointer\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	container = devm_kzalloc(dev, size, GFP_KERNEL);
+	if (!container)
+		return ERR_PTR(-ENOMEM);
+
+	bridge = container + offset;
+	bridge->funcs = funcs;
+
+	return container;
+}
+EXPORT_SYMBOL(__devm_drm_bridge_alloc);
+
 /**
  * drm_bridge_add - add the given bridge to the global bridge list
  *
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index b0d86a685a41e3172e0aa15d1c9a5ae8e959255a..dae463b30542d586a595b67f7bdf5a5e898e9572 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -941,6 +941,23 @@ drm_priv_to_bridge(struct drm_private_obj *priv)
 	return container_of(priv, struct drm_bridge, base);
 }
 
+void *__devm_drm_bridge_alloc(struct device *dev, size_t size, size_t offset,
+			      const struct drm_bridge_funcs *funcs);
+
+/**
+ * devm_drm_bridge_alloc - Allocate and initialize a bridge
+ * @dev: struct device of the bridge device
+ * @type: the type of the struct which contains struct &drm_bridge
+ * @member: the name of the &drm_bridge within @type
+ * @funcs: callbacks for this bridge
+ *
+ * Returns:
+ * Pointer to new bridge, or ERR_PTR on failure.
+ */
+#define devm_drm_bridge_alloc(dev, type, member, funcs) \
+	((type *)__devm_drm_bridge_alloc(dev, sizeof(type), \
+					 offsetof(type, member), funcs))
+
 void drm_bridge_add(struct drm_bridge *bridge);
 int devm_drm_bridge_add(struct device *dev, struct drm_bridge *bridge);
 void drm_bridge_remove(struct drm_bridge *bridge);

-- 
2.48.1

Re: [PATCH v7 01/11] drm/bridge: add devm_drm_bridge_alloc()

[Maxime Ripard]

On Fri, 14 Mar 2025 11:31:14 +0100, Luca Ceresoli wrote:
> Add a macro to allocate and initialize a DRM bridge embedded within a
> private driver struct.
> 
> Compared to current practice, which is based on [devm_]kzalloc() allocation
> followed by open-coded initialization of fields, this allows to have a
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH v7 02/11] drm/bridge: add support for refcounting

[Luca Ceresoli]

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;
+	/**
+	 * @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.
  */

-- 
2.48.1

Re: [PATCH v7 02/11] drm/bridge: add support for refcounting

[Maxime Ripard]

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

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

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

Re: [PATCH v7 02/11] drm/bridge: add support for refcounting

[Luca Ceresoli]

Hello Maxime,

thanks for the very prompt feedback!

On Fri, 14 Mar 2025 19:04:40 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> 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?

I don't think we had discussed this before. There is no strong reason,
just a few small ones, and one is related to the DRM_DEBUG() calls that
I have removed from this series, so it's fine to move back
drm_bridge_get/put() in the .c file, and make __drm_bridge_free()
private again.

> > +/**
> > + * 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.

Indeed, that's a leftover I missed when extracting this series for the
branch with the entire work. I'm definitely removing it in the next
iteration.

> >  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?

Indeed, good catch!

Luca

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

[PATCH v7 03/11] 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().

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

---

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 2ba0dac9bfc2dfd709d5e2457d69067c7324972c..72fa51d4dd2337f97c2bd65cfabf9cee05b661b4 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -259,6 +259,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)
@@ -306,6 +308,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.48.1

Re: [PATCH v7 03/11] drm/bridge: get/put the bridge reference in drm_bridge_add/remove()

[Maxime Ripard]

On Fri, 14 Mar 2025 11:31:16 +0100, Luca Ceresoli wrote:
> drm_bridge_add() adds the bridge to the global bridge_list, so take a
> reference for that. Vice versa in drm_bridge_remove().
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH v7 04/11] 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().

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

---

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 72fa51d4dd2337f97c2bd65cfabf9cee05b661b4..da85694e9310f1f910bc8a5aa5d95a91d9254888 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -370,11 +370,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;
@@ -423,6 +429,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);
@@ -443,6 +451,7 @@ void drm_bridge_detach(struct drm_bridge *bridge)
 
 	list_del(&bridge->chain_node);
 	bridge->dev = NULL;
+	drm_bridge_put(bridge);
 }
 
 /**

-- 
2.48.1

Re: [PATCH v7 04/11] drm/bridge: get/put the bridge reference in drm_bridge_attach/detach()

[Maxime Ripard]

On Fri, 14 Mar 2025 11:31:17 +0100, Luca Ceresoli wrote:
> drm_bridge_attach() adds the bridge to the encoder chain, so take a
> reference for that. Vice versa in drm_bridge_detach().
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH v7 05/11] drm/bridge: add a cleanup action for scope-based drm_bridge_put() invocation

[Luca Ceresoli]

Many functions get a drm_bridge pointer, only use it in the function body
(or a smaller scope such as a loop body), and don't store it. In these
cases they always need to drm_bridge_put() it before returning (or exiting
the scope).

Some of those functions have complex code paths with multiple return points
or loop break/continue. This makes adding drm_bridge_put() in the right
places tricky, ugly and error prone in case of future code changes.

Others use the bridge pointer in the return statement and would need to
split the return line to fit the drm_bridge_put, which is a bit annoying:

  -return some_thing(bridge);
  +ret = some_thing(bridge);
  +drm_bridge_put(bridge);
  +return ret;

To make it easier for all of them to put the bridge reference correctly
without complicating code, define a scope-based cleanup action to be used
with __free().

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

---

This patch was added in v7.
---
 include/drm/drm_bridge.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 5c1e2b9cafb12eb429d1f5d3ef312e6cf9b54f47..a5accd64c364ebb57903ae1e7459034ad9ebf4f3 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -23,6 +23,7 @@
 #ifndef __DRM_BRIDGE_H__
 #define __DRM_BRIDGE_H__
 
+#include <linux/cleanup.h>
 #include <linux/ctype.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
@@ -995,6 +996,9 @@ static inline struct drm_bridge *drm_bridge_put(struct drm_bridge *bridge)
 	return bridge;
 }
 
+/* Cleanup action for use with __free() */
+DEFINE_FREE(drm_bridge_put, struct drm_bridge *, if (_T) drm_bridge_put(_T))
+
 void drm_bridge_put_void(void *data);
 
 /**

-- 
2.48.1

Re: [PATCH v7 05/11] drm/bridge: add a cleanup action for scope-based drm_bridge_put() invocation

[Maxime Ripard]

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

On Fri, Mar 14, 2025 at 11:31:18AM +0100, Luca Ceresoli wrote:
> Many functions get a drm_bridge pointer, only use it in the function body
> (or a smaller scope such as a loop body), and don't store it. In these
> cases they always need to drm_bridge_put() it before returning (or exiting
> the scope).
> 
> Some of those functions have complex code paths with multiple return points
> or loop break/continue. This makes adding drm_bridge_put() in the right
> places tricky, ugly and error prone in case of future code changes.
> 
> Others use the bridge pointer in the return statement and would need to
> split the return line to fit the drm_bridge_put, which is a bit annoying:
> 
>   -return some_thing(bridge);
>   +ret = some_thing(bridge);
>   +drm_bridge_put(bridge);
>   +return ret;
> 
> To make it easier for all of them to put the bridge reference correctly
> without complicating code, define a scope-based cleanup action to be used
> with __free().
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> This patch was added in v7.
> ---
>  include/drm/drm_bridge.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 5c1e2b9cafb12eb429d1f5d3ef312e6cf9b54f47..a5accd64c364ebb57903ae1e7459034ad9ebf4f3 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -23,6 +23,7 @@
>  #ifndef __DRM_BRIDGE_H__
>  #define __DRM_BRIDGE_H__
>  
> +#include <linux/cleanup.h>
>  #include <linux/ctype.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
> @@ -995,6 +996,9 @@ static inline struct drm_bridge *drm_bridge_put(struct drm_bridge *bridge)
>  	return bridge;
>  }
>  
> +/* Cleanup action for use with __free() */
> +DEFINE_FREE(drm_bridge_put, struct drm_bridge *, if (_T) drm_bridge_put(_T))
> +

IIRC, drm_bridge_put already checks for the pointer being null before
dereferencing it, right? Then we can probably simplify the macro here.

Either way,

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Maxime

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

Re: [PATCH v7 05/11] drm/bridge: add a cleanup action for scope-based drm_bridge_put() invocation

[Luca Ceresoli]

On Fri, 14 Mar 2025 19:08:22 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> On Fri, Mar 14, 2025 at 11:31:18AM +0100, Luca Ceresoli wrote:
> > Many functions get a drm_bridge pointer, only use it in the function body
> > (or a smaller scope such as a loop body), and don't store it. In these
> > cases they always need to drm_bridge_put() it before returning (or exiting
> > the scope).
> > 
> > Some of those functions have complex code paths with multiple return points
> > or loop break/continue. This makes adding drm_bridge_put() in the right
> > places tricky, ugly and error prone in case of future code changes.
> > 
> > Others use the bridge pointer in the return statement and would need to
> > split the return line to fit the drm_bridge_put, which is a bit annoying:
> > 
> >   -return some_thing(bridge);
> >   +ret = some_thing(bridge);
> >   +drm_bridge_put(bridge);
> >   +return ret;
> > 
> > To make it easier for all of them to put the bridge reference correctly
> > without complicating code, define a scope-based cleanup action to be used
> > with __free().
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > 
> > ---
> > 
> > This patch was added in v7.
> > ---
> >  include/drm/drm_bridge.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 5c1e2b9cafb12eb429d1f5d3ef312e6cf9b54f47..a5accd64c364ebb57903ae1e7459034ad9ebf4f3 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -23,6 +23,7 @@
> >  #ifndef __DRM_BRIDGE_H__
> >  #define __DRM_BRIDGE_H__
> >  
> > +#include <linux/cleanup.h>
> >  #include <linux/ctype.h>
> >  #include <linux/list.h>
> >  #include <linux/mutex.h>
> > @@ -995,6 +996,9 @@ static inline struct drm_bridge *drm_bridge_put(struct drm_bridge *bridge)
> >  	return bridge;
> >  }
> >  
> > +/* Cleanup action for use with __free() */
> > +DEFINE_FREE(drm_bridge_put, struct drm_bridge *, if (_T) drm_bridge_put(_T))
> > +  
> 
> IIRC, drm_bridge_put already checks for the pointer being null before
> dereferencing it, right? Then we can probably simplify the macro here.

drm_bridge_put() does the NULL-check, yes. However I have kept the 'if'
here for consistency with all other DEFINE_FREE()s in the kernel which
also have an 'if' even when the called function does it as well.

Moreover, as per discussion after patch 2, in the next iteration I'm
moving drm_bridge_put() back to a an exported symbol instead of an
inline. So the 'if' here will save a useless function call on NULL.

For the two above reasons I'm rather inclined to keeping this line as
is.

> Either way,
> 
> Reviewed-by: Maxime Ripard <mripard@kernel.org>

Luca

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

[PATCH v7 06/11] drm/bridge: get the bridge returned by drm_bridge_chain_get_first_bridge()

[Luca Ceresoli]

drm_bridge_chain_get_first_bridge() returns a bridge pointer that the
caller could hold for a long time. Increment the refcount of the returned
bridge and document it must be put by the caller.

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

---

This patch was added in v7.
---
 include/drm/drm_bridge.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index a5accd64c364ebb57903ae1e7459034ad9ebf4f3..d9777d5f2e9ef006f0062e4507bce99df4146cd9 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1105,6 +1105,9 @@ drm_bridge_get_prev_bridge(struct drm_bridge *bridge)
  * drm_bridge_chain_get_first_bridge() - Get the first bridge in the chain
  * @encoder: encoder object
  *
+ * The refcount of the returned bridge is incremented. Use drm_bridge_put()
+ * when done with it.
+ *
  * RETURNS:
  * the first bridge in the chain, or NULL if @encoder has no bridge attached
  * to it.
@@ -1112,8 +1115,8 @@ drm_bridge_get_prev_bridge(struct drm_bridge *bridge)
 static inline struct drm_bridge *
 drm_bridge_chain_get_first_bridge(struct drm_encoder *encoder)
 {
-	return list_first_entry_or_null(&encoder->bridge_chain,
-					struct drm_bridge, chain_node);
+	return drm_bridge_get(list_first_entry_or_null(&encoder->bridge_chain,
+						       struct drm_bridge, chain_node));
 }
 
 /**

-- 
2.48.1

Re: [PATCH v7 06/11] drm/bridge: get the bridge returned by drm_bridge_chain_get_first_bridge()

[Maxime Ripard]

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

On Fri, Mar 14, 2025 at 11:31:19AM +0100, Luca Ceresoli wrote:
> drm_bridge_chain_get_first_bridge() returns a bridge pointer that the
> caller could hold for a long time. Increment the refcount of the returned
> bridge and document it must be put by the caller.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> This patch was added in v7.
> ---
>  include/drm/drm_bridge.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index a5accd64c364ebb57903ae1e7459034ad9ebf4f3..d9777d5f2e9ef006f0062e4507bce99df4146cd9 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -1105,6 +1105,9 @@ drm_bridge_get_prev_bridge(struct drm_bridge *bridge)
>   * drm_bridge_chain_get_first_bridge() - Get the first bridge in the chain
>   * @encoder: encoder object
>   *
> + * The refcount of the returned bridge is incremented. Use drm_bridge_put()
> + * when done with it.
> + *
>   * RETURNS:
>   * the first bridge in the chain, or NULL if @encoder has no bridge attached
>   * to it.
> @@ -1112,8 +1115,8 @@ drm_bridge_get_prev_bridge(struct drm_bridge *bridge)
>  static inline struct drm_bridge *
>  drm_bridge_chain_get_first_bridge(struct drm_encoder *encoder)
>  {
> -	return list_first_entry_or_null(&encoder->bridge_chain,
> -					struct drm_bridge, chain_node);
> +	return drm_bridge_get(list_first_entry_or_null(&encoder->bridge_chain,
> +						       struct drm_bridge, chain_node));
>  }

We'll need to modify drm_bridge_get_next_bridge, drm_bridge_get_prev_bridge, and
drm_for_each_bridge_in_chain in a similar manner, but for this one

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Maxime

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

Re: [PATCH v7 06/11] drm/bridge: get the bridge returned by drm_bridge_chain_get_first_bridge()

[Luca Ceresoli]

On Fri, 14 Mar 2025 19:10:41 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> On Fri, Mar 14, 2025 at 11:31:19AM +0100, Luca Ceresoli wrote:
> > drm_bridge_chain_get_first_bridge() returns a bridge pointer that the
> > caller could hold for a long time. Increment the refcount of the returned
> > bridge and document it must be put by the caller.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

...

> > @@ -1112,8 +1115,8 @@ drm_bridge_get_prev_bridge(struct drm_bridge *bridge)
> >  static inline struct drm_bridge *
> >  drm_bridge_chain_get_first_bridge(struct drm_encoder *encoder)
> >  {
> > -	return list_first_entry_or_null(&encoder->bridge_chain,
> > -					struct drm_bridge, chain_node);
> > +	return drm_bridge_get(list_first_entry_or_null(&encoder->bridge_chain,
> > +						       struct drm_bridge, chain_node));
> >  }  
> 
> We'll need to modify drm_bridge_get_next_bridge, drm_bridge_get_prev_bridge, and
> drm_for_each_bridge_in_chain in a similar manner, but for this one

Sure. I'm discussing this in the cover letter.

Luca

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

[PATCH v7 07/11] drm/mxsfb: put the bridge returned by drm_bridge_chain_get_first_bridge()

[Luca Ceresoli]

The bridge returned by drm_bridge_chain_get_first_bridge() is
refcounted. Put it when done. Use a scope-based free action to catch all
the code paths.

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

---

This patch was added in v7.
---
 drivers/gpu/drm/mxsfb/lcdif_kms.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index dbd42cc1da87f82ef9cd4ccc70cdecbe56035174..21311cf5efee7d26c80316bffe80dd2bfed02238 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -433,7 +433,6 @@ static int lcdif_crtc_atomic_check(struct drm_crtc *crtc,
 	struct drm_connector *connector;
 	struct drm_encoder *encoder;
 	struct drm_bridge_state *bridge_state;
-	struct drm_bridge *bridge;
 	u32 bus_format, bus_flags;
 	bool format_set = false, flags_set = false;
 	int ret, i;
@@ -448,6 +447,8 @@ static int lcdif_crtc_atomic_check(struct drm_crtc *crtc,
 
 	/* Try to find consistent bus format and flags across first bridges. */
 	for_each_new_connector_in_state(state, connector, connector_state, i) {
+		struct drm_bridge *bridge __free(drm_bridge_put) = NULL;
+
 		if (!connector_state->crtc)
 			continue;
 

-- 
2.48.1

Re: [PATCH v7 07/11] drm/mxsfb: put the bridge returned by drm_bridge_chain_get_first_bridge()

[Maxime Ripard]

On Fri, 14 Mar 2025 11:31:20 +0100, Luca Ceresoli wrote:
> The bridge returned by drm_bridge_chain_get_first_bridge() is
> refcounted. Put it when done. Use a scope-based free action to catch all
> the code paths.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH v7 08/11] drm/atomic-helper: put the bridge returned by drm_bridge_chain_get_first_bridge()

[Luca Ceresoli]

The bridge returned by drm_bridge_chain_get_first_bridge() is
refcounted. Put it when done.

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

---

Note: I suspect most or all of the involved functions here could just
receive the encoder instead of the first bridge in the chain, and then walk
the encoder chain on their own. For the sake of simplicity and to show how
to put a bridge in general I've kept this patch in this form, for the time
being.

This patch was added in v7.
---
 drivers/gpu/drm/drm_atomic_helper.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 5302ab3248985d3e0a47e40fd3deb7ad0d9f775b..fcd9139ef98e9f57659b7e447cf57702e99a5165 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -455,6 +455,7 @@ mode_fixup(struct drm_atomic_state *state)
 		ret = drm_atomic_bridge_chain_check(bridge,
 						    new_crtc_state,
 						    new_conn_state);
+		drm_bridge_put(bridge);
 		if (ret) {
 			drm_dbg_atomic(encoder->dev, "Bridge atomic check failed\n");
 			return ret;
@@ -526,6 +527,7 @@ static enum drm_mode_status mode_valid_path(struct drm_connector *connector,
 	bridge = drm_bridge_chain_get_first_bridge(encoder);
 	ret = drm_bridge_chain_mode_valid(bridge, &connector->display_info,
 					  mode);
+	drm_bridge_put(bridge);
 	if (ret != MODE_OK) {
 		drm_dbg_atomic(encoder->dev, "[BRIDGE] mode_valid() failed\n");
 		return ret;
@@ -1226,6 +1228,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
 		}
 
 		drm_atomic_bridge_chain_post_disable(bridge, state);
+		drm_bridge_put(bridge);
 	}
 
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
@@ -1433,6 +1436,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *state)
 
 		bridge = drm_bridge_chain_get_first_bridge(encoder);
 		drm_bridge_chain_mode_set(bridge, mode, adjusted_mode);
+		drm_bridge_put(bridge);
 	}
 }
 
@@ -1564,6 +1568,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 		}
 
 		drm_atomic_bridge_chain_enable(bridge, state);
+		drm_bridge_put(bridge);
 	}
 
 	drm_atomic_helper_commit_writebacks(dev, state);

-- 
2.48.1

Re: [PATCH v7 08/11] drm/atomic-helper: put the bridge returned by drm_bridge_chain_get_first_bridge()

[Maxime Ripard]

On Fri, 14 Mar 2025 11:31:21 +0100, Luca Ceresoli wrote:
> The bridge returned by drm_bridge_chain_get_first_bridge() is
> refcounted. Put it when done.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH v7 09/11] drm/probe-helper: put the bridge returned by drm_bridge_chain_get_first_bridge()

[Luca Ceresoli]

The bridge returned by drm_bridge_chain_get_first_bridge() is
refcounted. Put it when done.

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

---

This patch was added in v7.
---
 drivers/gpu/drm/drm_probe_helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 7ba16323e7c2f4bc7ec61f96b01ddfe28461b6a0..15525124ee66b512979e5c4774dd765618bd15d7 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -119,6 +119,7 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode,
 		*status = drm_bridge_chain_mode_valid(bridge,
 						      &connector->display_info,
 						      mode);
+		drm_bridge_put(bridge);
 		if (*status != MODE_OK) {
 			/* There is also no point in continuing for crtc check
 			 * here. */

-- 
2.48.1

Re: [PATCH v7 09/11] drm/probe-helper: put the bridge returned by drm_bridge_chain_get_first_bridge()

[Maxime Ripard]

On Fri, 14 Mar 2025 11:31:22 +0100, Luca Ceresoli wrote:
> The bridge returned by drm_bridge_chain_get_first_bridge() is
> refcounted. Put it when done.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH v7 10/11] drm/bridge: ti-sn65dsi83: use dynamic lifetime management

[Luca Ceresoli]

Allow this bridge to be removable without dangling pointers and
use-after-free, together with proper use of drm_bridge_get() and _put() by
consumers.

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

---

Changes in v7: none

Changed in v6:
 - Update to use devm_drm_bridge_alloc(), remove .destroy

This patch was added in v5.
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 5f8bfeeb553f970671a602fcf2594016243b9db2..bc092fb926563439e316c2cb5a817bd938093df4 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -952,9 +952,9 @@ static int sn65dsi83_probe(struct i2c_client *client)
 	struct sn65dsi83 *ctx;
 	int ret;
 
-	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
-	if (!ctx)
-		return -ENOMEM;
+	ctx = devm_drm_bridge_alloc(dev, struct sn65dsi83, bridge, &sn65dsi83_funcs);
+	if (IS_ERR(ctx))
+		return PTR_ERR(ctx);
 
 	ctx->dev = dev;
 	INIT_WORK(&ctx->reset_work, sn65dsi83_reset_work);
@@ -994,7 +994,6 @@ static int sn65dsi83_probe(struct i2c_client *client)
 	dev_set_drvdata(dev, ctx);
 	i2c_set_clientdata(client, ctx);
 
-	ctx->bridge.funcs = &sn65dsi83_funcs;
 	ctx->bridge.of_node = dev->of_node;
 	ctx->bridge.pre_enable_prev_first = true;
 	ctx->bridge.type = DRM_MODE_CONNECTOR_LVDS;

-- 
2.48.1

Re: [PATCH v7 10/11] drm/bridge: ti-sn65dsi83: use dynamic lifetime management

[Maxime Ripard]

On Fri, 14 Mar 2025 11:31:23 +0100, Luca Ceresoli wrote:
> Allow this bridge to be removable without dangling pointers and
> use-after-free, together with proper use of drm_bridge_get() and _put() by
> consumers.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH v7 11/11] drm/bridge: samsung-dsim: use dynamic lifetime management

[Luca Ceresoli]

Allow this bridge to be removable without dangling pointers and
use-after-free, together with proper use of drm_bridge_get() and _put() by
consumers.

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

---

This patch was added in v7.
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 54de6ed2fae81bc13301a6b1ee8f38183a3118b6..3d41db7a0ceeddccc1a89a2ff1f38fe10ec6acfe 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1935,9 +1935,9 @@ int samsung_dsim_probe(struct platform_device *pdev)
 	struct samsung_dsim *dsi;
 	int ret, i;
 
-	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
-	if (!dsi)
-		return -ENOMEM;
+	dsi = devm_drm_bridge_alloc(dev, struct samsung_dsim, bridge, &samsung_dsim_bridge_funcs);
+	if (IS_ERR(dsi))
+		return PTR_ERR(dsi);
 
 	init_completion(&dsi->completed);
 	spin_lock_init(&dsi->transfer_lock);
@@ -2007,7 +2007,6 @@ int samsung_dsim_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(dev);
 
-	dsi->bridge.funcs = &samsung_dsim_bridge_funcs;
 	dsi->bridge.of_node = dev->of_node;
 	dsi->bridge.type = DRM_MODE_CONNECTOR_DSI;
 

-- 
2.48.1

Re: [PATCH v7 11/11] drm/bridge: samsung-dsim: use dynamic lifetime management

[Maxime Ripard]

On Fri, 14 Mar 2025 11:31:24 +0100, Luca Ceresoli wrote:
> Allow this bridge to be removable without dangling pointers and
> use-after-free, together with proper use of drm_bridge_get() and _put() by
> consumers.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [PATCH v7 00/11] drm/bridge: add devm_drm_bridge_alloc() with bridge refcount

[Maxime Ripard]

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

Hi,

On Fri, Mar 14, 2025 at 11:31:13AM +0100, Luca Ceresoli wrote:
> This series improves the way DRM bridges are allocated and initialized and
> makes them reference-counted. The goal of reference counting is to avoid
> use-after-free by drivers which got a pointer to a bridge and keep it
> stored and used even after the bridge has been deallocated.
> 
> The overall goal is supporting Linux devices with a DRM pipeline whose
> final components can be hot-plugged and hot-unplugged, including one or
> more bridges. For more details see the big picture [0].
> 
> DRM bridge drivers will have to be adapted to the new API, which is pretty
> simple for most cases. Refcounting will have to be adopted on the two
> sides: all functions returning a bridge pointer and all code obtaining such
> a pointer. This series has just an overview of some of those conversions,
> because for now the main goal is to agree on the API.
> 
> Series layout:
> 
>  1. Add the new API and refcounting:
> 
>     drm/bridge: add devm_drm_bridge_alloc()
>     drm/bridge: add support for refcounting
> 
>  2. get/put the reference in basic operations in the bridge core:
> 
>     drm/bridge: get/put the bridge reference in drm_bridge_add/remove()
>     drm/bridge: get/put the bridge reference in drm_bridge_attach/detach()
> 
>  3. as an example of changes for bridge consumers, get a reference for the
>     bridge returned by drm_bridge_chain_get_first_bridge(), have it put by
>     all callers (all users will be covered later on separately):
> 
>     drm/bridge: add a cleanup action for scope-based drm_bridge_put() invocation
>     drm/bridge: get the bridge returned by drm_bridge_chain_get_first_bridge()
>     drm/mxsfb: put the bridge returned by drm_bridge_chain_get_first_bridge()
>     drm/atomic-helper: put the bridge returned by drm_bridge_chain_get_first_bridge()
>     drm/probe-helper: put the bridge returned by drm_bridge_chain_get_first_bridge()
> 
>  4. convert a few bridge drivers (bridge providers) to the new API:
> 
>     drm/bridge: ti-sn65dsi83: use dynamic lifetime management
>     drm/bridge: samsung-dsim: use dynamic lifetime management
> 
> This work was formerly a part of my v6 DRM bridge hotplug series[0], now
> split as a standalone series with many improvements, hence the "v7" version
> number.

Except for one patch where I had comments, I think the series is in
excellent shape. We're still missing a couple of things to close this
topic though:

  - Converting the other bridge iterators/accessors to take / put the references
  - Mass converting the drivers to devm_drm_bridge_alloc
  - Documenting somewhere (possibly in drm_bridge_init?) that it really
    shouldn't be used anymore

Maxime

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

Re: [PATCH v7 00/11] drm/bridge: add devm_drm_bridge_alloc() with bridge refcount

[Luca Ceresoli]

Hello Maxime,

On Fri, 14 Mar 2025 19:21:01 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> Hi,
> 
> On Fri, Mar 14, 2025 at 11:31:13AM +0100, Luca Ceresoli wrote:
> > This series improves the way DRM bridges are allocated and
> > initialized and makes them reference-counted. The goal of reference
> > counting is to avoid use-after-free by drivers which got a pointer
> > to a bridge and keep it stored and used even after the bridge has
> > been deallocated.
> > 
> > The overall goal is supporting Linux devices with a DRM pipeline
> > whose final components can be hot-plugged and hot-unplugged,
> > including one or more bridges. For more details see the big picture
> > [0].
> > 
> > DRM bridge drivers will have to be adapted to the new API, which is
> > pretty simple for most cases. Refcounting will have to be adopted
> > on the two sides: all functions returning a bridge pointer and all
> > code obtaining such a pointer. This series has just an overview of
> > some of those conversions, because for now the main goal is to
> > agree on the API.
> > 
> > Series layout:
> > 
> >  1. Add the new API and refcounting:
> > 
> >     drm/bridge: add devm_drm_bridge_alloc()
> >     drm/bridge: add support for refcounting
> > 
> >  2. get/put the reference in basic operations in the bridge core:
> > 
> >     drm/bridge: get/put the bridge reference in
> > drm_bridge_add/remove() drm/bridge: get/put the bridge reference in
> > drm_bridge_attach/detach()
> > 
> >  3. as an example of changes for bridge consumers, get a reference
> > for the bridge returned by drm_bridge_chain_get_first_bridge(),
> > have it put by all callers (all users will be covered later on
> > separately):
> > 
> >     drm/bridge: add a cleanup action for scope-based
> > drm_bridge_put() invocation drm/bridge: get the bridge returned by
> > drm_bridge_chain_get_first_bridge() drm/mxsfb: put the bridge
> > returned by drm_bridge_chain_get_first_bridge() drm/atomic-helper:
> > put the bridge returned by drm_bridge_chain_get_first_bridge()
> > drm/probe-helper: put the bridge returned by
> > drm_bridge_chain_get_first_bridge()
> > 
> >  4. convert a few bridge drivers (bridge providers) to the new API:
> > 
> >     drm/bridge: ti-sn65dsi83: use dynamic lifetime management
> >     drm/bridge: samsung-dsim: use dynamic lifetime management
> > 
> > This work was formerly a part of my v6 DRM bridge hotplug
> > series[0], now split as a standalone series with many improvements,
> > hence the "v7" version number.  
> 
> Except for one patch where I had comments, I think the series is in
> excellent shape. We're still missing a couple of things to close this
> topic though:
> 
>   - Converting the other bridge iterators/accessors to take / put the
> references

I sent a couple in this series as you had asked, to show how conversion
looks like. But I have a large part of this conversion partially done
already, and it is the largest part of the refcounting work in terms of
touched files due to the large number of drivers using the iterators
and accessors. Here are the functions to convert:

 A) drm_bridge_chain_get_first_bridge
 B) drm_bridge_get_prev_bridge
 C) drm_bridge_get_next_bridge
 D) drm_for_each_bridge_in_chain
 E) drm_bridge_connector_init
 F) of_drm_find_bridge

A) is present in this series as an example but I don't think it should
be applied until all bridge drivers are converted to
drm_bridge_alloc(). Otherwise for not-yet-converted bridge drivers we'd
have drm_bridge_get/put() operating on an uninitialized kref, and
__drm_bridge_free() called on non-refcounted bridges, so I think we'd
see fireworks.

In the previous iteration I used drm_bridge_is_refcounted() in
drm_bridge_get/put() to allow a progressive migration, but if we want
to convert everything in a single run we need to first convert all
bridges to drm_bridge_alloc() and then convert all accessors.

The same reasoning applies to patches 3-4 which add get/put to
drm_bridge_add/remove() and _attach/detach().

B) to E) are ready in my work branch, about 20 patches in total.
Indeed item E) is a special case but it is handled there too.

Item F) is the beast, because of the reverse call graph of
of_drm_find_bridge() which includes drm_of_find_panel_or_bridge() and
then *_of_get_bridge(), each having a few dozen callers, and leading
to the panel_bridge topic. I have converted maybe half of the users of
accessors in F), it's 35 patches but it's the easy half and I still need
to tackle to hardest ones.

>   - Mass converting the drivers to devm_drm_bridge_alloc

Again I sent a couple in this series as you had asked, to show how
conversion looks like for the typical bridge driver. There are ~70
drivers to convert in total and I think most will be easy as the two
examples presented here.

I think this should be merged entirely before merging any accessor
changes, as explained above.

>   - Documenting somewhere (possibly in drm_bridge_init?) that it
> really shouldn't be used anymore

I'm afraid there is no drm_bridge_init(), bridge drivers just do
[devm_]kzalloc and set fields explicitly. So I don't think there is a
place to document this.

However I used to have a documentation patch until v6 [0], and I think
it should be revived and resent at some point, after removing the
"legacy mode" as we are converting all drivers at once. BTW I also have
a kunittest patch that should be revived. Do you still prefer me to
resend these two patches as a separate series, waiting after the API in
this series is applied?

Overall, I think this could be the path forward, let me know if
youthink it should be done differently:

 A. have patches 1 and 2 of this series applied
    (why not, even patches 10-11)
 B. after (A), send series to convert all bridge drivers to new API
    (includes patches 10-11 of this series if not applied already)
 C. after (A), send documentation and kunittest patches
 D. after (B), add get/put to drm_bridge_add/remove() + attach/detech()
    (patches 3-4 in this series)
 E. after (B), send series to convert accessors (including patches 5-9
    in this series which convert drm_bridge_chain_get_first_bridge()
    and its users)

[0] https://lore.kernel.org/lkml/20250206-hotplug-drm-bridge-v6-18-9d6f2c9c3058@bootlin.com/

Luca

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

Re: [PATCH v7 00/11] drm/bridge: add devm_drm_bridge_alloc() with bridge refcount

[Maxime Ripard]

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

On Mon, Mar 17, 2025 at 03:56:07PM +0100, Luca Ceresoli wrote:
> Hello Maxime,
> 
> On Fri, 14 Mar 2025 19:21:01 +0100
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > Hi,
> > 
> > On Fri, Mar 14, 2025 at 11:31:13AM +0100, Luca Ceresoli wrote:
> > > This series improves the way DRM bridges are allocated and
> > > initialized and makes them reference-counted. The goal of reference
> > > counting is to avoid use-after-free by drivers which got a pointer
> > > to a bridge and keep it stored and used even after the bridge has
> > > been deallocated.
> > > 
> > > The overall goal is supporting Linux devices with a DRM pipeline
> > > whose final components can be hot-plugged and hot-unplugged,
> > > including one or more bridges. For more details see the big picture
> > > [0].
> > > 
> > > DRM bridge drivers will have to be adapted to the new API, which is
> > > pretty simple for most cases. Refcounting will have to be adopted
> > > on the two sides: all functions returning a bridge pointer and all
> > > code obtaining such a pointer. This series has just an overview of
> > > some of those conversions, because for now the main goal is to
> > > agree on the API.
> > > 
> > > Series layout:
> > > 
> > >  1. Add the new API and refcounting:
> > > 
> > >     drm/bridge: add devm_drm_bridge_alloc()
> > >     drm/bridge: add support for refcounting
> > > 
> > >  2. get/put the reference in basic operations in the bridge core:
> > > 
> > >     drm/bridge: get/put the bridge reference in
> > > drm_bridge_add/remove() drm/bridge: get/put the bridge reference in
> > > drm_bridge_attach/detach()
> > > 
> > >  3. as an example of changes for bridge consumers, get a reference
> > > for the bridge returned by drm_bridge_chain_get_first_bridge(),
> > > have it put by all callers (all users will be covered later on
> > > separately):
> > > 
> > >     drm/bridge: add a cleanup action for scope-based
> > > drm_bridge_put() invocation drm/bridge: get the bridge returned by
> > > drm_bridge_chain_get_first_bridge() drm/mxsfb: put the bridge
> > > returned by drm_bridge_chain_get_first_bridge() drm/atomic-helper:
> > > put the bridge returned by drm_bridge_chain_get_first_bridge()
> > > drm/probe-helper: put the bridge returned by
> > > drm_bridge_chain_get_first_bridge()
> > > 
> > >  4. convert a few bridge drivers (bridge providers) to the new API:
> > > 
> > >     drm/bridge: ti-sn65dsi83: use dynamic lifetime management
> > >     drm/bridge: samsung-dsim: use dynamic lifetime management
> > > 
> > > This work was formerly a part of my v6 DRM bridge hotplug
> > > series[0], now split as a standalone series with many improvements,
> > > hence the "v7" version number.  
> > 
> > Except for one patch where I had comments, I think the series is in
> > excellent shape. We're still missing a couple of things to close this
> > topic though:
> > 
> >   - Converting the other bridge iterators/accessors to take / put the
> > references
> 
> I sent a couple in this series as you had asked, to show how conversion
> looks like. But I have a large part of this conversion partially done
> already, and it is the largest part of the refcounting work in terms of
> touched files due to the large number of drivers using the iterators
> and accessors. Here are the functions to convert:
> 
>  A) drm_bridge_chain_get_first_bridge
>  B) drm_bridge_get_prev_bridge
>  C) drm_bridge_get_next_bridge
>  D) drm_for_each_bridge_in_chain
>  E) drm_bridge_connector_init
>  F) of_drm_find_bridge
> 
> A) is present in this series as an example but I don't think it should
> be applied until all bridge drivers are converted to
> drm_bridge_alloc(). Otherwise for not-yet-converted bridge drivers we'd
> have drm_bridge_get/put() operating on an uninitialized kref, and
> __drm_bridge_free() called on non-refcounted bridges, so I think we'd
> see fireworks.
> 
> In the previous iteration I used drm_bridge_is_refcounted() in
> drm_bridge_get/put() to allow a progressive migration, but if we want
> to convert everything in a single run we need to first convert all
> bridges to drm_bridge_alloc() and then convert all accessors.
> 
> The same reasoning applies to patches 3-4 which add get/put to
> drm_bridge_add/remove() and _attach/detach().

Agreed.

> B) to E) are ready in my work branch, about 20 patches in total.
> Indeed item E) is a special case but it is handled there too.
> 
> Item F) is the beast, because of the reverse call graph of
> of_drm_find_bridge() which includes drm_of_find_panel_or_bridge() and
> then *_of_get_bridge(), each having a few dozen callers, and leading
> to the panel_bridge topic. I have converted maybe half of the users of
> accessors in F), it's 35 patches but it's the easy half and I still need
> to tackle to hardest ones.

One thing to keep in mind is that, while it's not correct, if the bridge
has been allocated with devm_drm_bridge_alloc, it's not worse either. If
you're not getting a reference to your pointer, the point is buggy,
sure, but it's just as buggy as before, and in the same situations.

So we can make that gradually if it's more convenient.

One way to solve it would be that, for example, of_drm_find_bridge is
oddly named according to our convention and it would make more sense to
be called drm_of_find_bridge().

So maybe we can just create drm_of_find_bridge() that takes a reference,
make of_drm_find_bridge() deprecated in favour of drm_of_find_bridge(),
add a TODO, and call it a day. People will gradually switch to the new
API over time.

> >   - Mass converting the drivers to devm_drm_bridge_alloc
> 
> Again I sent a couple in this series as you had asked, to show how
> conversion looks like for the typical bridge driver. There are ~70
> drivers to convert in total and I think most will be easy as the two
> examples presented here.
> 
> I think this should be merged entirely before merging any accessor
> changes, as explained above.

Agreed.

> >   - Documenting somewhere (possibly in drm_bridge_init?) that it
> > really shouldn't be used anymore
> 
> I'm afraid there is no drm_bridge_init(), bridge drivers just do
> [devm_]kzalloc and set fields explicitly. So I don't think there is a
> place to document this.

Oh, right.

Then, drm_bridge_add() would be a good candidate too to mention that
bridges must be allocated using devm_drm_bridge_alloc().

> However I used to have a documentation patch until v6 [0], and I think
> it should be revived and resent at some point, after removing the
> "legacy mode" as we are converting all drivers at once. BTW I also have
> a kunittest patch that should be revived. Do you still prefer me to
> resend these two patches as a separate series, waiting after the API in
> this series is applied?

Both options work for me.

> Overall, I think this could be the path forward, let me know if
> youthink it should be done differently:
> 
>  A. have patches 1 and 2 of this series applied
>     (why not, even patches 10-11)

I had some comments on patch 2, but it's ok for me on principle.

>  B. after (A), send series to convert all bridge drivers to new API
>     (includes patches 10-11 of this series if not applied already)
>  C. after (A), send documentation and kunittest patches
>  D. after (B), add get/put to drm_bridge_add/remove() + attach/detech()
>     (patches 3-4 in this series)
>  E. after (B), send series to convert accessors (including patches 5-9
>     in this series which convert drm_bridge_chain_get_first_bridge()
>     and its users)

Sounds like a plan.

Maxime

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

Re: [PATCH v7 00/11] drm/bridge: add devm_drm_bridge_alloc() with bridge refcount

[Luca Ceresoli]

Hello Mazime,

On Wed, 19 Mar 2025 17:16:53 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> On Mon, Mar 17, 2025 at 03:56:07PM +0100, Luca Ceresoli wrote:
> > Hello Maxime,
> > 
> > On Fri, 14 Mar 2025 19:21:01 +0100
> > Maxime Ripard <mripard@kernel.org> wrote:
> >   
> > > Hi,
> > > 
> > > On Fri, Mar 14, 2025 at 11:31:13AM +0100, Luca Ceresoli wrote:  
> > > > This series improves the way DRM bridges are allocated and
> > > > initialized and makes them reference-counted. The goal of reference
> > > > counting is to avoid use-after-free by drivers which got a pointer
> > > > to a bridge and keep it stored and used even after the bridge has
> > > > been deallocated.
> > > > 
> > > > The overall goal is supporting Linux devices with a DRM pipeline
> > > > whose final components can be hot-plugged and hot-unplugged,
> > > > including one or more bridges. For more details see the big picture
> > > > [0].
> > > > 
> > > > DRM bridge drivers will have to be adapted to the new API, which is
> > > > pretty simple for most cases. Refcounting will have to be adopted
> > > > on the two sides: all functions returning a bridge pointer and all
> > > > code obtaining such a pointer. This series has just an overview of
> > > > some of those conversions, because for now the main goal is to
> > > > agree on the API.
> > > > 
> > > > Series layout:
> > > > 
> > > >  1. Add the new API and refcounting:
> > > > 
> > > >     drm/bridge: add devm_drm_bridge_alloc()
> > > >     drm/bridge: add support for refcounting
> > > > 
> > > >  2. get/put the reference in basic operations in the bridge core:
> > > > 
> > > >     drm/bridge: get/put the bridge reference in
> > > > drm_bridge_add/remove() drm/bridge: get/put the bridge reference in
> > > > drm_bridge_attach/detach()
> > > > 
> > > >  3. as an example of changes for bridge consumers, get a reference
> > > > for the bridge returned by drm_bridge_chain_get_first_bridge(),
> > > > have it put by all callers (all users will be covered later on
> > > > separately):
> > > > 
> > > >     drm/bridge: add a cleanup action for scope-based
> > > > drm_bridge_put() invocation drm/bridge: get the bridge returned by
> > > > drm_bridge_chain_get_first_bridge() drm/mxsfb: put the bridge
> > > > returned by drm_bridge_chain_get_first_bridge() drm/atomic-helper:
> > > > put the bridge returned by drm_bridge_chain_get_first_bridge()
> > > > drm/probe-helper: put the bridge returned by
> > > > drm_bridge_chain_get_first_bridge()
> > > > 
> > > >  4. convert a few bridge drivers (bridge providers) to the new API:
> > > > 
> > > >     drm/bridge: ti-sn65dsi83: use dynamic lifetime management
> > > >     drm/bridge: samsung-dsim: use dynamic lifetime management
> > > > 
> > > > This work was formerly a part of my v6 DRM bridge hotplug
> > > > series[0], now split as a standalone series with many improvements,
> > > > hence the "v7" version number.    
> > > 
> > > Except for one patch where I had comments, I think the series is in
> > > excellent shape. We're still missing a couple of things to close this
> > > topic though:
> > > 
> > >   - Converting the other bridge iterators/accessors to take / put the
> > > references  
> > 
> > I sent a couple in this series as you had asked, to show how conversion
> > looks like. But I have a large part of this conversion partially done
> > already, and it is the largest part of the refcounting work in terms of
> > touched files due to the large number of drivers using the iterators
> > and accessors. Here are the functions to convert:
> > 
> >  A) drm_bridge_chain_get_first_bridge
> >  B) drm_bridge_get_prev_bridge
> >  C) drm_bridge_get_next_bridge
> >  D) drm_for_each_bridge_in_chain
> >  E) drm_bridge_connector_init
> >  F) of_drm_find_bridge
> > 
> > A) is present in this series as an example but I don't think it should
> > be applied until all bridge drivers are converted to
> > drm_bridge_alloc(). Otherwise for not-yet-converted bridge drivers we'd
> > have drm_bridge_get/put() operating on an uninitialized kref, and
> > __drm_bridge_free() called on non-refcounted bridges, so I think we'd
> > see fireworks.
> > 
> > In the previous iteration I used drm_bridge_is_refcounted() in
> > drm_bridge_get/put() to allow a progressive migration, but if we want
> > to convert everything in a single run we need to first convert all
> > bridges to drm_bridge_alloc() and then convert all accessors.
> > 
> > The same reasoning applies to patches 3-4 which add get/put to
> > drm_bridge_add/remove() and _attach/detach().  
> 
> Agreed.
> 
> > B) to E) are ready in my work branch, about 20 patches in total.
> > Indeed item E) is a special case but it is handled there too.
> > 
> > Item F) is the beast, because of the reverse call graph of
> > of_drm_find_bridge() which includes drm_of_find_panel_or_bridge() and
> > then *_of_get_bridge(), each having a few dozen callers, and leading
> > to the panel_bridge topic. I have converted maybe half of the users of
> > accessors in F), it's 35 patches but it's the easy half and I still need
> > to tackle to hardest ones.  
> 
> One thing to keep in mind is that, while it's not correct, if the bridge
> has been allocated with devm_drm_bridge_alloc, it's not worse either. If
> you're not getting a reference to your pointer, the point is buggy,
> sure, but it's just as buggy as before, and in the same situations.
> 
> So we can make that gradually if it's more convenient.

I see your point, right. And definitely doing it in gradually will help.

> One way to solve it would be that, for example, of_drm_find_bridge is
> oddly named according to our convention and it would make more sense to
> be called drm_of_find_bridge().
> 
> So maybe we can just create drm_of_find_bridge() that takes a reference,
> make of_drm_find_bridge() deprecated in favour of drm_of_find_bridge(),
> add a TODO, and call it a day. People will gradually switch to the new
> API over time.

Thanks for the suggestion, I will certainly try this way when I get
back to the accessor conversion work. While increasing the number of
deprecated functions is not great, I'm starting to suspect a conversion
of _all_ the of_drm_find_bridge() users at once is not realistic.

> > >   - Documenting somewhere (possibly in drm_bridge_init?) that it
> > > really shouldn't be used anymore  
> > 
> > I'm afraid there is no drm_bridge_init(), bridge drivers just do
> > [devm_]kzalloc and set fields explicitly. So I don't think there is a
> > place to document this.  
> 
> Oh, right.
> 
> Then, drm_bridge_add() would be a good candidate too to mention that
> bridges must be allocated using devm_drm_bridge_alloc().

Sure. I'll add such a comment.

> > Overall, I think this could be the path forward, let me know if
> > youthink it should be done differently:
> > 
> >  A. have patches 1 and 2 of this series applied
> >     (why not, even patches 10-11)  
> 
> I had some comments on patch 2, but it's ok for me on principle.

Sorry, my wording was not clear. I really meant "have patches 1 and 2 of
this series applied after the improvements proposed". So hopefully the
v8 I'm sending today-ish. I'm going to send only patches 1, 2, 10 and
11. The other patches are not meant to be merged now, so I'm resending
them when appropriate.

Luca

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