[PATCH v8 0/5] drm/bridge: get/put the bridge returned by drm_bridge_chain_get_first_bridge()

[PATCH v8 0/5] drm/bridge: get/put the bridge returned by drm_bridge_chain_get_first_bridge()

[Luca Ceresoli]

This series adds drm_bridge_get/put() calls for DRM bridges returned by
drm_bridge_chain_get_first_bridge().

This is part of the work towards removal of bridges from a still existing
DRM pipeline without use-after-free. The grand plan as discussed in [1].
Here's the work breakdown (➜ marks the current series):

 1. ➜ add refcounting to DRM bridges (struct drm_bridge)
    (based on devm_drm_bridge_alloc() [0])
    A. ✔ add new alloc API and refcounting (in v6.16-rc1)
    B. ✔ convert all bridge drivers to new API (now in drm-misc-next)
    C. ✔ kunit tests (now in drm-misc-next)
    D. … add get/put to drm_bridge_add/remove() + attach/detach()
         and warn on old allocation pattern (under review)
    E. ➜ add get/put on drm_bridge accessors
       1. ➜ drm_bridge_chain_get_first_bridge() + add a cleanup action (this series)
       2. drm_bridge_chain_get_last_bridge()
       3. drm_bridge_get_prev_bridge()
       4. drm_bridge_get_next_bridge()
       5. drm_for_each_bridge_in_chain()
       6. drm_bridge_connector_init
       7. of_drm_find_bridge
       8. drm_of_find_panel_or_bridge, *_of_get_bridge
    F. debugfs improvements
 2. handle gracefully atomic updates during bridge removal
 3. avoid DSI host drivers to have dangling pointers to DSI devices
 4. finish the hotplug bridge work, removing the "always-disconnected"
    connector, moving code to the core and potentially removing the
    hotplug-bridge itself (this needs to be clarified as points 1-3 are
    developed)

All the patches in this series have already been sent as part of the larger
"[PATCH v7 00/11] drm/bridge: add devm_drm_bridge_alloc() with bridge
refcount" series [2], hence the v8 number. They have all been Reviewed-by
Maxime too, however they could bnot be applied at that time, awaiting the
conversion of all bridge drivers to devm_drm_bridge_alloc(), now done (item
1.A).

I'm resending all patches to give them visibility now that they are ready
to be applied. I have removed the R-by tag from patch 4 which had to be
reworked on current code. All other patches are diff-identical.

[0] https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/0cc6aadd7fc1e629b715ea3d1ba537ef2da95eec
[1] https://lore.kernel.org/lkml/20250206-hotplug-drm-bridge-v6-0-9d6f2c9c3058@bootlin.com/t/#u
[2] https://lore.kernel.org/all/20250314-drm-bridge-refcount-v7-0-152571f8c694@bootlin.com/

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Changes in v8:
- rebased on current drm-misc-next
- Patch 4: reworked based on current code
- Link to v7: https://lore.kernel.org/all/20250314-drm-bridge-refcount-v7-0-152571f8c694@bootlin.com/

---
Luca Ceresoli (5):
      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()

 drivers/gpu/drm/drm_atomic_helper.c |  7 +++++++
 drivers/gpu/drm/drm_probe_helper.c  |  1 +
 drivers/gpu/drm/mxsfb/lcdif_kms.c   |  3 ++-
 include/drm/drm_bridge.h            | 11 +++++++++--
 4 files changed, 19 insertions(+), 3 deletions(-)
---
base-commit: a59a271769149f0b8258507276f3d2a24370cbdb
change-id: 20250620-drm-bridge-alloc-getput-drm_bridge_chain_get_first_bridge-be39c442dcd6

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

[PATCH v8 1/5] 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().

Reviewed-by: Maxime Ripard <mripard@kernel.org>
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 7f66f9018c1090642876ff948bbf06ca66a46bfe..b110c5bba8c0612a71f749ad51345e7a8ccdc910 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>
@@ -1227,6 +1228,9 @@ drm_priv_to_bridge(struct drm_private_obj *priv)
 struct drm_bridge *drm_bridge_get(struct drm_bridge *bridge);
 void drm_bridge_put(struct drm_bridge *bridge);
 
+/* Cleanup action for use with __free() */
+DEFINE_FREE(drm_bridge_put, struct drm_bridge *, if (_T) drm_bridge_put(_T))
+
 void *__devm_drm_bridge_alloc(struct device *dev, size_t size, size_t offset,
 			      const struct drm_bridge_funcs *funcs);
 

-- 
2.49.0

[PATCH v8 2/5] 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.

Reviewed-by: Maxime Ripard <mripard@kernel.org>
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 b110c5bba8c0612a71f749ad51345e7a8ccdc910..f98044581d67c380c3bc3a1943bd6ab09b764ec3 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1336,6 +1336,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.
@@ -1343,8 +1346,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.49.0

[PATCH v8 3/5] 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.

Reviewed-by: Maxime Ripard <mripard@kernel.org>
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.49.0

[PATCH v8 4/5] 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>
---

Changes in v8:
- reworked after the changes in pre_enable/post_disable order:
  f6ee26f58870 ("drm/atomic-helper: Refactor crtc & encoder-bridge op loops into separate functions")
  d5bef6430c85 ("drm/atomic-helper: Separate out bridge pre_enable/post_disable from enable/disable")
  c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable and post-disable")
- Removed R-by Maxime as the diff is not identical, even though it is in
  principle the same (there are 2 new drm_bridge_chain_get_first_bridge
  calls, thus added the corresponding drm_bridge_put calls)

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

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index ef56b474acf599bb9cd341674dc83b04ae247eb7..d5ebe6ea0acbc5a08aef7fa41ecb9ed5d8fa8e80 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -456,6 +456,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;
@@ -527,6 +528,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;
@@ -1212,6 +1214,7 @@ encoder_bridge_disable(struct drm_device *dev, struct drm_atomic_state *state)
 		 */
 		bridge = drm_bridge_chain_get_first_bridge(encoder);
 		drm_atomic_bridge_chain_disable(bridge, state);
+		drm_bridge_put(bridge);
 
 		/* Right function depends upon target state. */
 		if (funcs) {
@@ -1329,6 +1332,7 @@ encoder_bridge_post_disable(struct drm_device *dev, struct drm_atomic_state *sta
 		 */
 		bridge = drm_bridge_chain_get_first_bridge(encoder);
 		drm_atomic_bridge_chain_post_disable(bridge, state);
+		drm_bridge_put(bridge);
 	}
 }
 
@@ -1501,6 +1505,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);
 	}
 }
 
@@ -1580,6 +1585,7 @@ encoder_bridge_pre_enable(struct drm_device *dev, struct drm_atomic_state *state
 		 */
 		bridge = drm_bridge_chain_get_first_bridge(encoder);
 		drm_atomic_bridge_chain_pre_enable(bridge, state);
+		drm_bridge_put(bridge);
 	}
 }
 
@@ -1655,6 +1661,7 @@ encoder_bridge_enable(struct drm_device *dev, struct drm_atomic_state *state)
 		}
 
 		drm_atomic_bridge_chain_enable(bridge, state);
+		drm_bridge_put(bridge);
 	}
 }
 

-- 
2.49.0

[PATCH v8 5/5] 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.

Reviewed-by: Maxime Ripard <mripard@kernel.org>
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 6b3541159c0ffdd6dc05b6a2324ec0de9d1c5474..09b12c30df69d44f099f119739b8d5d2f77907d5 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.49.0

Re: [PATCH v8 2/5] drm/bridge: get the bridge returned by drm_bridge_chain_get_first_bridge()

[Liu Ying]

On 06/21/2025, 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.

To make sure the incremented refcount is decremented once this patch is
applied, does it make sense to squash patch 3, 4 and 5 into this one?

> 
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> 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 b110c5bba8c0612a71f749ad51345e7a8ccdc910..f98044581d67c380c3bc3a1943bd6ab09b764ec3 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -1336,6 +1336,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.
> @@ -1343,8 +1346,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));
>  }
>  
>  /**
> 

-- 
Regards,
Liu Ying

Re: [PATCH v8 3/5] drm/mxsfb: put the bridge returned by drm_bridge_chain_get_first_bridge()

[Liu Ying]

On 06/21/2025, 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.
> 
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> 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;

To avoid the unnecessary cleanup for !connector_state->crtc, I would write:

-8<-
--- 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;
@@ -453,7 +452,8 @@ static int lcdif_crtc_atomic_check(struct drm_crtc *crtc,
 
                encoder = connector_state->best_encoder;
 
-               bridge = drm_bridge_chain_get_first_bridge(encoder);
+               struct drm_bridge *bridge __free(drm_bridge_put) =
+                               drm_bridge_chain_get_first_bridge(encoder);
                if (!bridge)
                        continue;
-8<-

>  
> 

-- 
Regards,
Liu Ying

Re: [PATCH v8 2/5] drm/bridge: get the bridge returned by drm_bridge_chain_get_first_bridge()

[Luca Ceresoli]

Hello Liu,

On Mon, 23 Jun 2025 10:56:13 +0800
Liu Ying <victor.liu@nxp.com> wrote:

> On 06/21/2025, 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.  
> 
> To make sure the incremented refcount is decremented once this patch is
> applied, does it make sense to squash patch 3, 4 and 5 into this one?

I see there is a trade off here between bisectability and patch
readability.

However about bisectability the problem is limited for this series. To
get an actual get/put imbalance you'd have to be able to remove the
bridge, but removing (part of) the bridge chain is not at all supported
right now, and it won't be until after chapter 4 of this work (see
cover letter).

However I realize there is an issue if:
* patch 2 is applied but patches 3/4/5 are not
  (it does not make sense to apply this series partially, but this
  might happen when cherry-picking?)
* an entire DRM card is removed where
  drm_bridge_chain_get_first_bridge() is used by some components

If both happen we'd have a get without put, thus a missing free and a
memory leak for the container struct.

Note that, besides drm_bridge_chain_get_first_bridge() that this
series covers, there are various other accessors: see items 1.E.{2..8}
in cover letter. For some of those there are many more changes to apply
because they are called in more places. Squashing them would result in
a really large patch that is likely hard to review and manage.

So I'll leave the decision to DRM subsystem maintainers. For the time
being I'm keeping the current approach given that Maxime already
reviewed these patches in the past, not squashed.

Best regards,
Luca

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

Re: [PATCH v8 3/5] drm/mxsfb: put the bridge returned by drm_bridge_chain_get_first_bridge()

[Luca Ceresoli]

On Mon, 23 Jun 2025 11:06:37 +0800
Liu Ying <victor.liu@nxp.com> wrote:

[...]

> > 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;  
> 
> To avoid the unnecessary cleanup for !connector_state->crtc, I would write:
> 
> -8<-
> --- 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;
> @@ -453,7 +452,8 @@ static int lcdif_crtc_atomic_check(struct drm_crtc *crtc,
>  
>                 encoder = connector_state->best_encoder;
>  
> -               bridge = drm_bridge_chain_get_first_bridge(encoder);
> +               struct drm_bridge *bridge __free(drm_bridge_put) =
> +                               drm_bridge_chain_get_first_bridge(encoder);

Good idea, I probably didn't think about it because I was grown up in a
world where all declarations must before the code. :-)

Changing this in v9.

Luca

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

Re: [PATCH v8 2/5] drm/bridge: get the bridge returned by drm_bridge_chain_get_first_bridge()

[Liu Ying]

On 06/23/2025, Luca Ceresoli wrote:
> Hello Liu,

Luca,

> 
> On Mon, 23 Jun 2025 10:56:13 +0800
> Liu Ying <victor.liu@nxp.com> wrote:
> 
>> On 06/21/2025, 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.  
>>
>> To make sure the incremented refcount is decremented once this patch is
>> applied, does it make sense to squash patch 3, 4 and 5 into this one?
> 
> I see there is a trade off here between bisectability and patch
> readability.
> 
> However about bisectability the problem is limited for this series. To
> get an actual get/put imbalance you'd have to be able to remove the
> bridge, but removing (part of) the bridge chain is not at all supported
> right now, and it won't be until after chapter 4 of this work (see
> cover letter).
> 
> However I realize there is an issue if:
> * patch 2 is applied but patches 3/4/5 are not
>   (it does not make sense to apply this series partially, but this
>   might happen when cherry-picking?)

Yes for cherry-picking and bisecting.

> * an entire DRM card is removed where
>   drm_bridge_chain_get_first_bridge() is used by some components
> 
> If both happen we'd have a get without put, thus a missing free and a
> memory leak for the container struct.

Yes, that's a memory leak.

> 
> Note that, besides drm_bridge_chain_get_first_bridge() that this
> series covers, there are various other accessors: see items 1.E.{2..8}

IIUC, without those items addressed, the issue we have is use-after-free,
but not the memory leak this patch introduces(without squash).

> in cover letter. For some of those there are many more changes to apply
> because they are called in more places. Squashing them would result in
> a really large patch that is likely hard to review and manage.

If the squash is done for each item separately and properly, I'd say
the memory leak issue can be avoided.  I assume patches for each item
would not be large.

> 
> So I'll leave the decision to DRM subsystem maintainers. For the time
> being I'm keeping the current approach given that Maxime already
> reviewed these patches in the past, not squashed.
> 
> Best regards,
> Luca
> 

-- 
Regards,
Liu Ying

Re: [PATCH v8 2/5] drm/bridge: get the bridge returned by drm_bridge_chain_get_first_bridge()

[Maxime Ripard]

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

On Tue, Jun 24, 2025 at 10:44:03AM +0800, Liu Ying wrote:
> On 06/23/2025, Luca Ceresoli wrote:
> > On Mon, 23 Jun 2025 10:56:13 +0800
> > Liu Ying <victor.liu@nxp.com> wrote:
> > 
> >> On 06/21/2025, 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.  
> >>
> >> To make sure the incremented refcount is decremented once this patch is
> >> applied, does it make sense to squash patch 3, 4 and 5 into this one?
> > 
> > I see there is a trade off here between bisectability and patch
> > readability.
> > 
> > However about bisectability the problem is limited for this series. To
> > get an actual get/put imbalance you'd have to be able to remove the
> > bridge, but removing (part of) the bridge chain is not at all supported
> > right now, and it won't be until after chapter 4 of this work (see
> > cover letter).
> > 
> > However I realize there is an issue if:
> > * patch 2 is applied but patches 3/4/5 are not
> >   (it does not make sense to apply this series partially, but this
> >   might happen when cherry-picking?)
> 
> Yes for cherry-picking and bisecting.
> 
> > * an entire DRM card is removed where
> >   drm_bridge_chain_get_first_bridge() is used by some components
> > 
> > If both happen we'd have a get without put, thus a missing free and a
> > memory leak for the container struct.
> 
> Yes, that's a memory leak.
> 
> > Note that, besides drm_bridge_chain_get_first_bridge() that this
> > series covers, there are various other accessors: see items 1.E.{2..8}
> 
> IIUC, without those items addressed, the issue we have is use-after-free,
> but not the memory leak this patch introduces(without squash).

Given that this structure is going to be allocated a couple of times in
the system life at best, and that the situation prior to the work Luca
has been doing was a use-after-free, I'm not really concerned about a
transient memory leak in a situation that cannot happen.

If people want to come and backport random patches without looking at
the whole thing, that's their problem.

Maxime

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

Re: [PATCH v8 4/5] drm/atomic-helper: put the bridge returned by drm_bridge_chain_get_first_bridge()

[Maxime Ripard]

On Fri, 20 Jun 2025 19:26:16 +0200, 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