[PATCH 0/9] drm/bridge: get/put the bridge returned by drm_bridge_get_last_bridge()

[PATCH 0/9] drm/bridge: get/put the bridge returned by drm_bridge_get_last_bridge()

[Luca Ceresoli]

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

This is part of the work towards removal of bridges from a still existing
DRM pipeline without use-after-free. The grand plan was 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 (now in drm-misc-next)
    E. ➜ add get/put on drm_bridge accessors
       1. … drm_bridge_chain_get_first_bridge() + add a cleanup action
       2. … drm_bridge_get_prev_bridge()
       3. ➜ drm_bridge_get_next_bridge()
       4. drm_for_each_bridge_in_chain()
       5. drm_bridge_connector_init
       6. of_drm_find_bridge
       7. 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)

There are various users of drm_bridge_get_last_bridge() which cannot be
converted easily. Luckily they are not really looking for the next bridge,
but for something else, such as getting the last bridge in the encoder
chain or checking whether a bridge is the last in the encoder chain. So
introduce better functions for those users and use them instead of
drm_bridge_get_last_bridge(), making the code cleaner at the same time.

Finally add a drm_bridge_get() ti drm_bridge_get_next_bridge() and
drm_bridge_put() to the remaining, and legitimate, calls.

[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

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Luca Ceresoli (9):
      list: add list_last_entry_or_null()
      drm/bridge: add drm_bridge_chain_get_last_bridge()
      drm/bridge: imx93-mipi-dsi: use drm_bridge_chain_get_last_bridge()
      drm/omapdrm: use drm_bridge_chain_get_last_bridge()
      drm/bridge: add drm_bridge_is_last()
      drm/display: bridge_connector: use drm_bridge_is_last()
      drm/bridge: get the bridge returned by drm_bridge_get_next_bridge()
      drm/bridge: put the bridge returned by drm_bridge_get_next_bridge()
      drm/imx: parallel-display: put the bridge returned by drm_bridge_get_next_bridge()

 drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c    | 12 ++++------
 drivers/gpu/drm/display/drm_bridge_connector.c |  5 ++--
 drivers/gpu/drm/drm_bridge.c                   |  2 ++
 drivers/gpu/drm/imx/ipuv3/parallel-display.c   |  4 +++-
 drivers/gpu/drm/omapdrm/omap_drv.c             |  8 +++----
 include/drm/drm_bridge.h                       | 32 +++++++++++++++++++++++++-
 include/linux/list.h                           | 14 +++++++++++
 7 files changed, 61 insertions(+), 16 deletions(-)
---
base-commit: 0f168e7be696a17487e83d1d47e5a408a181080f
change-id: 20250709-drm-bridge-alloc-getput-drm_bridge_get_next_bridge-1df6aaf62136

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

[PATCH 1/9] list: add list_last_entry_or_null()

[Luca Ceresoli]

Add an equivalent of list_first_entry_or_null() to obtain the last element
of a list.

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

---

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Zijun Hu <quic_zijuhu@quicinc.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/linux/list.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/linux/list.h b/include/linux/list.h
index e7e28afd28f8eef94ab6baec77e69ea104ba0391..7f7657e416209a2941b3f292b1334e9e0f2a3ca5 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -636,6 +636,20 @@ static inline void list_splice_tail_init(struct list_head *list,
 	pos__ != head__ ? list_entry(pos__, type, member) : NULL; \
 })
 
+/**
+ * list_last_entry_or_null - get the last element from a list
+ * @ptr:	the list head to take the element from.
+ * @type:	the type of the struct this is embedded in.
+ * @member:	the name of the list_head within the struct.
+ *
+ * Note that if the list is empty, it returns NULL.
+ */
+#define list_last_entry_or_null(ptr, type, member) ({ \
+	struct list_head *head__ = (ptr); \
+	struct list_head *pos__ = READ_ONCE(head__->prev); \
+	pos__ != head__ ? list_entry(pos__, type, member) : NULL; \
+})
+
 /**
  * list_next_entry - get the next element in list
  * @pos:	the type * to cursor

-- 
2.50.0

[PATCH 2/9] drm/bridge: add drm_bridge_chain_get_last_bridge()

[Luca Ceresoli]

Add an equivalent of drm_bridge_chain_get_first_bridge() to get the last
bridge.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 include/drm/drm_bridge.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index d2454ba83db36f8f0d475b0b37468c2ebe7e921d..9a7e9dd8cbfa54eedb82981032a25009e845a037 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1343,6 +1343,24 @@ drm_bridge_chain_get_first_bridge(struct drm_encoder *encoder)
 					struct drm_bridge, chain_node);
 }
 
+/**
+ * drm_bridge_chain_get_last_bridge() - Get the last 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 last bridge in the chain, or NULL if @encoder has no bridge attached
+ * to it.
+ */
+static inline struct drm_bridge *
+drm_bridge_chain_get_last_bridge(struct drm_encoder *encoder)
+{
+	return drm_bridge_get(list_last_entry_or_null(&encoder->bridge_chain,
+						      struct drm_bridge, chain_node));
+}
+
 /**
  * drm_for_each_bridge_in_chain() - Iterate over all bridges present in a chain
  * @encoder: the encoder to iterate bridges on

-- 
2.50.0

[PATCH 3/9] drm/bridge: imx93-mipi-dsi: use drm_bridge_chain_get_last_bridge()

[Luca Ceresoli]

Use drm_bridge_chain_get_last_bridge() instead of open coding a loop with
two invocations of drm_bridge_get_next_bridge() per iteration.

Besides being cleaner and more efficient, this change is necessary in
preparation for drm_bridge_get_next_bridge() to get a reference to the
returned bridge.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
index bea8346515b8c8ce150040f58d288ac564eeb563..8f7a0d46601a41e1bfc04587398b0f1536a6a16c 100644
--- a/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
@@ -492,14 +492,12 @@ static int imx93_dsi_get_phy_configure_opts(struct imx93_dsi *dsi,
 static enum drm_mode_status
 imx93_dsi_validate_mode(struct imx93_dsi *dsi, const struct drm_display_mode *mode)
 {
-	struct drm_bridge *bridge = dw_mipi_dsi_get_bridge(dsi->dmd);
+	struct drm_bridge *dmd_bridge = dw_mipi_dsi_get_bridge(dsi->dmd);
+	struct drm_bridge *last_bridge __free(drm_bridge_put) =
+		drm_bridge_chain_get_last_bridge(dmd_bridge->encoder);
 
-	/* Get the last bridge */
-	while (drm_bridge_get_next_bridge(bridge))
-		bridge = drm_bridge_get_next_bridge(bridge);
-
-	if ((bridge->ops & DRM_BRIDGE_OP_DETECT) &&
-	    (bridge->ops & DRM_BRIDGE_OP_EDID)) {
+	if ((last_bridge->ops & DRM_BRIDGE_OP_DETECT) &&
+	    (last_bridge->ops & DRM_BRIDGE_OP_EDID)) {
 		unsigned long pixel_clock_rate = mode->clock * 1000;
 		unsigned long rounded_rate;
 

-- 
2.50.0

[PATCH 4/9] drm/omapdrm: use drm_bridge_chain_get_last_bridge()

[Luca Ceresoli]

Use drm_bridge_chain_get_last_bridge() instead of open coding a loop with
two invocations of drm_bridge_get_next_bridge() per iteration.

Besides being cleaner and more efficient, this change is necessary in
preparation for drm_bridge_get_next_bridge() to get a reference to the
returned bridge.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 054b71dba6a75b8c42198c4b102a093f43a675a2..3bbcec01428a6f290afdfa40ef6f79629539a584 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -378,12 +378,12 @@ static int omap_display_id(struct omap_dss_device *output)
 	struct device_node *node = NULL;
 
 	if (output->bridge) {
-		struct drm_bridge *bridge = output->bridge;
-
-		while (drm_bridge_get_next_bridge(bridge))
-			bridge = drm_bridge_get_next_bridge(bridge);
+		struct drm_bridge *bridge =
+			drm_bridge_chain_get_last_bridge(output->bridge->encoder);
 
 		node = bridge->of_node;
+
+		drm_bridge_put(bridge);
 	}
 
 	return node ? of_alias_get_id(node, "display") : -ENODEV;

-- 
2.50.0

[PATCH 5/9] drm/bridge: add drm_bridge_is_last()

[Luca Ceresoli]

Some code needing to know whether a bridge is the last in a chain currently
call drm_bridge_get_next_bridge(). However drm_bridge_get_next_bridge()
will soon increment the refcount of the returned bridge, which would make
such code more annoying to write.

In preparation for drm_bridge_get_next_bridge() to increment the refcount,
as well as to simplify such code, introduce a simple bool function to tell
whether a bridge is the last in the chain.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 include/drm/drm_bridge.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 9a7e9dd8cbfa54eedb82981032a25009e845a037..c2a7a7d2dfc420e9dcf7ea4c093ce1f1b939c820 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1264,6 +1264,11 @@ static inline struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 }
 #endif
 
+static inline bool drm_bridge_is_last(struct drm_bridge *bridge)
+{
+	return list_is_last(&bridge->chain_node, &bridge->encoder->bridge_chain);
+}
+
 /**
  * drm_bridge_get_current_state() - Get the current bridge state
  * @bridge: bridge object

-- 
2.50.0

[PATCH 6/9] drm/display: bridge_connector: use drm_bridge_is_last()

[Luca Ceresoli]

Simplify code to know whether a bridge is the last in the chain by using
drm_bridge_is_last().

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/display/drm_bridge_connector.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 6cdb432dbc3004f88988883a37c2486c409ea932..842561d7b3fd5b833b97edc299c4a6d81b2a7168 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -749,12 +749,11 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 				return ERR_PTR(-EINVAL);
 		}
 
-		if (!drm_bridge_get_next_bridge(bridge))
+		if (drm_bridge_is_last(bridge))
 			connector_type = bridge->type;
 
 #ifdef CONFIG_OF
-		if (!drm_bridge_get_next_bridge(bridge) &&
-		    bridge->of_node)
+		if (drm_bridge_is_last(bridge) && bridge->of_node)
 			connector->fwnode = fwnode_handle_get(of_fwnode_handle(bridge->of_node));
 #endif
 

-- 
2.50.0

[PATCH 7/9] drm/bridge: get the bridge returned by drm_bridge_get_next_bridge()

[Luca Ceresoli]

drm_bridge_get_next_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>
---
 include/drm/drm_bridge.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index c2a7a7d2dfc420e9dcf7ea4c093ce1f1b939c820..158d22892bf3ddb469d510735818f14a1c23d7a1 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1305,6 +1305,13 @@ drm_bridge_get_current_state(struct drm_bridge *bridge)
  * drm_bridge_get_next_bridge() - Get the next bridge in the chain
  * @bridge: bridge object
  *
+ * The caller is responsible of having a reference to @bridge via
+ * drm_bridge_get() or equivalent. This function leaves the refcount of
+ * @bridge unmodified.
+ *
+ * The refcount of the returned bridge is incremented. Use drm_bridge_put()
+ * when done with it.
+ *
  * RETURNS:
  * the next bridge in the chain after @bridge, or NULL if @bridge is the last.
  */
@@ -1314,7 +1321,7 @@ drm_bridge_get_next_bridge(struct drm_bridge *bridge)
 	if (list_is_last(&bridge->chain_node, &bridge->encoder->bridge_chain))
 		return NULL;
 
-	return list_next_entry(bridge, chain_node);
+	return drm_bridge_get(list_next_entry(bridge, chain_node));
 }
 
 /**

-- 
2.50.0

[PATCH 8/9] drm/bridge: put the bridge returned by drm_bridge_get_next_bridge()

[Luca Ceresoli]

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

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/drm_bridge.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 0b450b334afd82e0460f18fdd248f79d0a2b153d..05e85457099ab1e0a23ea7842c9654c9a6881dfb 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -1147,6 +1147,8 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
 	} else {
 		next_bridge_state = drm_atomic_get_new_bridge_state(state,
 								next_bridge);
+		drm_bridge_put(next_bridge);
+
 		/*
 		 * No bridge state attached to the next bridge, just leave the
 		 * flags to 0.

-- 
2.50.0

[PATCH 9/9] drm/imx: parallel-display: put the bridge returned by drm_bridge_get_next_bridge()

[Luca Ceresoli]

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

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/imx/ipuv3/parallel-display.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/ipuv3/parallel-display.c b/drivers/gpu/drm/imx/ipuv3/parallel-display.c
index 6d8325c766979aa3ba98970f00806e99c139d3c3..44b2ce3c2a3a1641c4483a610607555dfbedff9e 100644
--- a/drivers/gpu/drm/imx/ipuv3/parallel-display.c
+++ b/drivers/gpu/drm/imx/ipuv3/parallel-display.c
@@ -138,9 +138,11 @@ static int imx_pd_bridge_atomic_check(struct drm_bridge *bridge,
 	u32 bus_flags, bus_fmt;
 
 	next_bridge = drm_bridge_get_next_bridge(bridge);
-	if (next_bridge)
+	if (next_bridge) {
 		next_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
 								    next_bridge);
+		drm_bridge_put(next_bridge);
+	}
 
 	if (next_bridge_state)
 		bus_flags = next_bridge_state->input_bus_cfg.flags;

-- 
2.50.0

Re: [PATCH 2/9] drm/bridge: add drm_bridge_chain_get_last_bridge()

[Maxime Ripard]

On Wed, 9 Jul 2025 18:48:01 +0200, Luca Ceresoli wrote:
> Add an equivalent of drm_bridge_chain_get_first_bridge() to get the last
> bridge.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

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

Thanks!
Maxime

Re: [PATCH 3/9] drm/bridge: imx93-mipi-dsi: use drm_bridge_chain_get_last_bridge()

[Maxime Ripard]

On Wed, 9 Jul 2025 18:48:02 +0200, Luca Ceresoli wrote:
> Use drm_bridge_chain_get_last_bridge() instead of open coding a loop with
> two invocations of drm_bridge_get_next_bridge() per iteration.
> 
> Besides being cleaner and more efficient, this change is necessary in
> preparation for drm_bridge_get_next_bridge() to get a reference to the
> 
> [ ... ]

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

Thanks!
Maxime

Re: [PATCH 4/9] drm/omapdrm: use drm_bridge_chain_get_last_bridge()

[Maxime Ripard]

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

On Wed, Jul 09, 2025 at 06:48:03PM +0200, Luca Ceresoli wrote:
> Use drm_bridge_chain_get_last_bridge() instead of open coding a loop with
> two invocations of drm_bridge_get_next_bridge() per iteration.
> 
> Besides being cleaner and more efficient, this change is necessary in
> preparation for drm_bridge_get_next_bridge() to get a reference to the
> returned bridge.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 054b71dba6a75b8c42198c4b102a093f43a675a2..3bbcec01428a6f290afdfa40ef6f79629539a584 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -378,12 +378,12 @@ static int omap_display_id(struct omap_dss_device *output)
>  	struct device_node *node = NULL;
>  
>  	if (output->bridge) {
> -		struct drm_bridge *bridge = output->bridge;
> -
> -		while (drm_bridge_get_next_bridge(bridge))
> -			bridge = drm_bridge_get_next_bridge(bridge);
> +		struct drm_bridge *bridge =
> +			drm_bridge_chain_get_last_bridge(output->bridge->encoder);
>  
>  		node = bridge->of_node;
> +
> +		drm_bridge_put(bridge);

Any reason you're not using __free(drm_bridge_put) here?

Maxime

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

Re: [PATCH 5/9] drm/bridge: add drm_bridge_is_last()

[Maxime Ripard]

On Wed, 9 Jul 2025 18:48:04 +0200, Luca Ceresoli wrote:
> Some code needing to know whether a bridge is the last in a chain currently
> call drm_bridge_get_next_bridge(). However drm_bridge_get_next_bridge()
> will soon increment the refcount of the returned bridge, which would make
> such code more annoying to write.
> 
> 
> [ ... ]

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

Thanks!
Maxime

Re: [PATCH 6/9] drm/display: bridge_connector: use drm_bridge_is_last()

[Maxime Ripard]

On Wed, 9 Jul 2025 18:48:05 +0200, Luca Ceresoli wrote:
> Simplify code to know whether a bridge is the last in the chain by using
> drm_bridge_is_last().
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

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

Thanks!
Maxime

Re: [PATCH 7/9] drm/bridge: get the bridge returned by drm_bridge_get_next_bridge()

[Maxime Ripard]

On Wed, 9 Jul 2025 18:48:06 +0200, Luca Ceresoli wrote:
> drm_bridge_get_next_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>
> 
> [ ... ]

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

Thanks!
Maxime

Re: [PATCH 8/9] drm/bridge: put the bridge returned by drm_bridge_get_next_bridge()

[Maxime Ripard]

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

Hi,

On Wed, Jul 09, 2025 at 06:48:07PM +0200, Luca Ceresoli wrote:
> The bridge returned by drm_bridge_get_next_bridge() is refcounted. Put it
> when done.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

You should really expand a bit more your commit logs, and provide the
context of why you think putting drm_bridge_put where you do is a good idea.

> ---
>  drivers/gpu/drm/drm_bridge.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 0b450b334afd82e0460f18fdd248f79d0a2b153d..05e85457099ab1e0a23ea7842c9654c9a6881dfb 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -1147,6 +1147,8 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
>  	} else {
>  		next_bridge_state = drm_atomic_get_new_bridge_state(state,
>  								next_bridge);
> +		drm_bridge_put(next_bridge);
> +
>  		/*
>  		 * No bridge state attached to the next bridge, just leave the
>  		 * flags to 0.

In particular, I don't think it is here.

You still have a variable in scope after that branch that you would have
given up the reference for, which is pretty dangerous.

Also, the bridge state lifetime is shorter than the bridge lifetime
itself, so we probably want to have the drm_bridge_put after we're done
with next_bridge_state too.

Overall, I think using __free here is probably the most robust solution.

Maxime

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

Re: [PATCH 9/9] drm/imx: parallel-display: put the bridge returned by drm_bridge_get_next_bridge()

[Maxime Ripard]

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

On Wed, Jul 09, 2025 at 06:48:08PM +0200, Luca Ceresoli wrote:
> The bridge returned by drm_bridge_get_next_bridge() is refcounted. Put it
> when done.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Same comments than on the previous patch here.

Maxime

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

Re: [PATCH 8/9] drm/bridge: put the bridge returned by drm_bridge_get_next_bridge()

[Luca Ceresoli]

Hi Maxime,

On Thu, 10 Jul 2025 09:27:09 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> Hi,
> 
> On Wed, Jul 09, 2025 at 06:48:07PM +0200, Luca Ceresoli wrote:
> > The bridge returned by drm_bridge_get_next_bridge() is refcounted. Put it
> > when done.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>  
> 
> You should really expand a bit more your commit logs, and provide the
> context of why you think putting drm_bridge_put where you do is a good idea.
> 
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 0b450b334afd82e0460f18fdd248f79d0a2b153d..05e85457099ab1e0a23ea7842c9654c9a6881dfb 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -1147,6 +1147,8 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
> >  	} else {
> >  		next_bridge_state = drm_atomic_get_new_bridge_state(state,
> >  								next_bridge);
> > +		drm_bridge_put(next_bridge);
> > +
> >  		/*
> >  		 * No bridge state attached to the next bridge, just leave the
> >  		 * flags to 0.  
> 
> In particular, I don't think it is here.
> 
> You still have a variable in scope after that branch that you would have
> given up the reference for, which is pretty dangerous.
> 
> Also, the bridge state lifetime is shorter than the bridge lifetime
> itself, so we probably want to have the drm_bridge_put after we're done
> with next_bridge_state too.

Totally agree about this.

I theory moving the _put just after the last usage of next_bridge_state
would be enough. However...

> Overall, I think using __free here is probably the most robust solution.

...I'm OK with using use __free here, even though it doesn't look
strictly necessary. However for patch 9 the code path is slightly more
complex, so I'll use __free for both.

With this change, this patch would become:

@@ -1121,7 +1121,6 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
                                      struct drm_atomic_state *state)
 {
        struct drm_bridge_state *bridge_state, *next_bridge_state;
-       struct drm_bridge *next_bridge;
        u32 output_flags = 0;
 
        bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
@@ -1130,7 +1129,7 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
        if (!bridge_state)
                return;
 
-       next_bridge = drm_bridge_get_next_bridge(bridge);
+       struct drm_bridge *next_bridge __free(drm_bridge_put) = drm_bridge_get_next_bridge(bridge);
 
        /*
         * Let's try to apply the most common case here, that is, propagate

And a tentative commit message body is:

  The bridge returned by drm_bridge_get_next_bridge() is refcounted. Put
  it when done. We need to ensure it is not put before either
  next_bridge or next_bridge_state is in use, thus for simplicity use a
  cleanup action.

I'll resend with the above changes (unless you have more improvements to
suggest) in a few days, to wait for any feedback on patch 1.

Luca

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

Re: [PATCH 4/9] drm/omapdrm: use drm_bridge_chain_get_last_bridge()

[Luca Ceresoli]

Hi Maxime,

On Thu, 10 Jul 2025 09:13:46 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> On Wed, Jul 09, 2025 at 06:48:03PM +0200, Luca Ceresoli wrote:
> > Use drm_bridge_chain_get_last_bridge() instead of open coding a loop with
> > two invocations of drm_bridge_get_next_bridge() per iteration.
> > 
> > Besides being cleaner and more efficient, this change is necessary in
> > preparation for drm_bridge_get_next_bridge() to get a reference to the
> > returned bridge.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > ---
> >  drivers/gpu/drm/omapdrm/omap_drv.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> > index 054b71dba6a75b8c42198c4b102a093f43a675a2..3bbcec01428a6f290afdfa40ef6f79629539a584 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> > @@ -378,12 +378,12 @@ static int omap_display_id(struct omap_dss_device *output)
> >  	struct device_node *node = NULL;
> >  
> >  	if (output->bridge) {
> > -		struct drm_bridge *bridge = output->bridge;
> > -
> > -		while (drm_bridge_get_next_bridge(bridge))
> > -			bridge = drm_bridge_get_next_bridge(bridge);
> > +		struct drm_bridge *bridge =
> > +			drm_bridge_chain_get_last_bridge(output->bridge->encoder);
> >  
> >  		node = bridge->of_node;
> > +
> > +		drm_bridge_put(bridge);  
> 
> Any reason you're not using __free(drm_bridge_put) here?

Just because the code is simple enough that an explicit
drm_bridge_put() is clearly sufficient.

Do you think __free() should be used even in such trivial cases?

Luca

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

Re: [PATCH 4/9] drm/omapdrm: use drm_bridge_chain_get_last_bridge()

[Maxime Ripard]

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

On Mon, Jul 14, 2025 at 12:32:40PM +0200, Luca Ceresoli wrote:
> Hi Maxime,
> 
> On Thu, 10 Jul 2025 09:13:46 +0200
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > On Wed, Jul 09, 2025 at 06:48:03PM +0200, Luca Ceresoli wrote:
> > > Use drm_bridge_chain_get_last_bridge() instead of open coding a loop with
> > > two invocations of drm_bridge_get_next_bridge() per iteration.
> > > 
> > > Besides being cleaner and more efficient, this change is necessary in
> > > preparation for drm_bridge_get_next_bridge() to get a reference to the
> > > returned bridge.
> > > 
> > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > > ---
> > >  drivers/gpu/drm/omapdrm/omap_drv.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> > > index 054b71dba6a75b8c42198c4b102a093f43a675a2..3bbcec01428a6f290afdfa40ef6f79629539a584 100644
> > > --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> > > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> > > @@ -378,12 +378,12 @@ static int omap_display_id(struct omap_dss_device *output)
> > >  	struct device_node *node = NULL;
> > >  
> > >  	if (output->bridge) {
> > > -		struct drm_bridge *bridge = output->bridge;
> > > -
> > > -		while (drm_bridge_get_next_bridge(bridge))
> > > -			bridge = drm_bridge_get_next_bridge(bridge);
> > > +		struct drm_bridge *bridge =
> > > +			drm_bridge_chain_get_last_bridge(output->bridge->encoder);
> > >  
> > >  		node = bridge->of_node;
> > > +
> > > +		drm_bridge_put(bridge);  
> > 
> > Any reason you're not using __free(drm_bridge_put) here?
> 
> Just because the code is simple enough that an explicit
> drm_bridge_put() is clearly sufficient.
> 
> Do you think __free() should be used even in such trivial cases?

It's a matter of opinion at this point :)

It' makes it a bit easier and consistent so that's why I raised it, but
if you feel like it's too much, that's fine by me as well.

Maxime

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

Re: [PATCH 4/9] drm/omapdrm: use drm_bridge_chain_get_last_bridge()

[Luca Ceresoli]

Hi Maxime,

On Fri, 25 Jul 2025 16:15:23 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> On Mon, Jul 14, 2025 at 12:32:40PM +0200, Luca Ceresoli wrote:
> > Hi Maxime,
> > 
> > On Thu, 10 Jul 2025 09:13:46 +0200
> > Maxime Ripard <mripard@kernel.org> wrote:
> >   
> > > On Wed, Jul 09, 2025 at 06:48:03PM +0200, Luca Ceresoli wrote:  
> > > > Use drm_bridge_chain_get_last_bridge() instead of open coding a loop with
> > > > two invocations of drm_bridge_get_next_bridge() per iteration.
> > > > 
> > > > Besides being cleaner and more efficient, this change is necessary in
> > > > preparation for drm_bridge_get_next_bridge() to get a reference to the
> > > > returned bridge.
> > > > 
> > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > > > ---
> > > >  drivers/gpu/drm/omapdrm/omap_drv.c | 8 ++++----
> > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> > > > index 054b71dba6a75b8c42198c4b102a093f43a675a2..3bbcec01428a6f290afdfa40ef6f79629539a584 100644
> > > > --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> > > > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> > > > @@ -378,12 +378,12 @@ static int omap_display_id(struct omap_dss_device *output)
> > > >  	struct device_node *node = NULL;
> > > >  
> > > >  	if (output->bridge) {
> > > > -		struct drm_bridge *bridge = output->bridge;
> > > > -
> > > > -		while (drm_bridge_get_next_bridge(bridge))
> > > > -			bridge = drm_bridge_get_next_bridge(bridge);
> > > > +		struct drm_bridge *bridge =
> > > > +			drm_bridge_chain_get_last_bridge(output->bridge->encoder);
> > > >  
> > > >  		node = bridge->of_node;
> > > > +
> > > > +		drm_bridge_put(bridge);    
> > > 
> > > Any reason you're not using __free(drm_bridge_put) here?  
> > 
> > Just because the code is simple enough that an explicit
> > drm_bridge_put() is clearly sufficient.
> > 
> > Do you think __free() should be used even in such trivial cases?  
> 
> It's a matter of opinion at this point :)
> 
> It' makes it a bit easier and consistent so that's why I raised it, but
> if you feel like it's too much, that's fine by me as well.

In the end I chose to use __free here as well for v2, for consistency
over the series.

Luca

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