drm_for_each_bridge_in_chain_scoped() and
drm_for_each_bridge_in_chain_from() currently get/put the bridge at each
iteration. But they don't protect the encoder chain, so it could change
(bridges added/removed) while some code is iterating over the list
itself. To make iterations safe, change the logic of these for_each macros
to lock the encoder chain mutex at the beginning and unlock it at the end
of the loop (be it at the end of the list, or earlier due to a 'break' or
'return' statement).

Also remove the get/put on the current bridge because it is not needed
anymore. In fact all bridges in the encoder chain are refcounted already
thanks to the drm_bridge_get() in drm_bridge_attach() and the
drm_bridge_put() in drm_bridge_detach(). So while iterating with the mutex
held the list cannot change _and_ the refcount of all bridges in the list
cannot drop to zero.

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

---

Changed in v2:
- Fixed infinite loop in drm_for_each_bridge_in_chain_scoped() when
  encoder->bridge_chain is empty, reported here:
  https://lore.kernel.org/lkml/202509301358.38036b85-lkp@intel.com/
- Slightly improved commit message
---
 include/drm/drm_bridge.h | 62 ++++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 0ff7ab4aa8689a022458f935a7ffb23a2b715802..5a72817f04a78f5379f69e72fe519c5eb3ea043e 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1440,26 +1440,29 @@ drm_bridge_chain_get_last_bridge(struct drm_encoder *encoder)
 						      struct drm_bridge, chain_node));
 }
 
-/**
- * drm_bridge_get_next_bridge_and_put - Get the next bridge in the chain
- *                                      and put the previous
- * @bridge: bridge object
- *
- * Same as drm_bridge_get_next_bridge() but additionally puts the @bridge.
- *
- * RETURNS:
- * the next bridge in the chain after @bridge, or NULL if @bridge is the last.
- */
-static inline struct drm_bridge *
-drm_bridge_get_next_bridge_and_put(struct drm_bridge *bridge)
+static inline struct drm_bridge *drm_bridge_encoder_chain_lock(struct drm_bridge *bridge)
 {
-	struct drm_bridge *next = drm_bridge_get_next_bridge(bridge);
+	drm_encoder_chain_lock(bridge->encoder);
+
+	return bridge;
+}
 
-	drm_bridge_put(bridge);
+/* Internal to drm_for_each_bridge_in_chain*() */
+static inline struct drm_bridge *__drm_encoder_bridge_chain_next(struct drm_bridge *bridge)
+{
+	if (list_is_last(&bridge->chain_node, &bridge->encoder->bridge_chain)) {
+		drm_encoder_chain_unlock(bridge->encoder);
 
-	return next;
+		return NULL;
+	}
+
+	return list_next_entry(bridge, chain_node);
 }
 
+/* Internal to drm_for_each_bridge_in_chain*() */
+DEFINE_FREE(drm_bridge_encoder_chain_unlock, struct drm_bridge *,
+	    if (_T) drm_encoder_chain_unlock(_T->encoder);)
+
 /**
  * drm_for_each_bridge_in_chain_scoped - iterate over all bridges attached
  *                                       to an encoder
@@ -1469,14 +1472,15 @@ drm_bridge_get_next_bridge_and_put(struct drm_bridge *bridge)
  *
  * Iterate over all bridges present in the bridge chain attached to @encoder.
  *
- * Automatically gets/puts the bridge reference while iterating, and puts
- * the reference even if returning or breaking in the middle of the loop.
+ * Automatically locks the encoder chain mutex to prevent chain
+ * modifications while iterating.
  */
-#define drm_for_each_bridge_in_chain_scoped(encoder, bridge)		\
-	for (struct drm_bridge *bridge __free(drm_bridge_put) =		\
-	     drm_bridge_chain_get_first_bridge(encoder);		\
-	     bridge;							\
-	     bridge = drm_bridge_get_next_bridge_and_put(bridge))
+#define drm_for_each_bridge_in_chain_scoped(encoder, bridge)					\
+	for (struct drm_bridge *bridge __free(drm_bridge_encoder_chain_unlock) =		\
+		     list_first_entry_or_null(&drm_encoder_chain_lock(encoder)->bridge_chain,	\
+					      struct drm_bridge, chain_node);			\
+	     bridge;										\
+	     bridge = __drm_encoder_bridge_chain_next(bridge))					\
 
 /**
  * drm_for_each_bridge_in_chain_from - iterate over all bridges starting
@@ -1488,14 +1492,14 @@ drm_bridge_get_next_bridge_and_put(struct drm_bridge *bridge)
  * Iterate over all bridges in the encoder chain starting from
  * @first_bridge, included.
  *
- * Automatically gets/puts the bridge reference while iterating, and puts
- * the reference even if returning or breaking in the middle of the loop.
+ * Automatically locks the encoder chain mutex to prevent chain
+ * modifications while iterating.
  */
-#define drm_for_each_bridge_in_chain_from(first_bridge, bridge)		\
-	for (struct drm_bridge *bridge __free(drm_bridge_put) =		\
-		     drm_bridge_get(first_bridge);			\
-	     bridge;							\
-	     bridge = drm_bridge_get_next_bridge_and_put(bridge))
+#define drm_for_each_bridge_in_chain_from(first_bridge, bridge)				\
+	for (struct drm_bridge *bridge __free(drm_bridge_encoder_chain_unlock) =	\
+		     drm_bridge_encoder_chain_lock(first_bridge);			\
+	     bridge;									\
+	     bridge = __drm_encoder_bridge_chain_next(bridge))				\
 
 enum drm_mode_status
 drm_bridge_chain_mode_valid(struct drm_bridge *bridge,

-- 
2.51.0

On Fri, Oct 03, 2025 at 12:39:26PM +0200, Luca Ceresoli wrote:
> drm_for_each_bridge_in_chain_scoped() and
> drm_for_each_bridge_in_chain_from() currently get/put the bridge at each
> iteration. But they don't protect the encoder chain, so it could change
> (bridges added/removed) while some code is iterating over the list
> itself. To make iterations safe, change the logic of these for_each macros
> to lock the encoder chain mutex at the beginning and unlock it at the end
> of the loop (be it at the end of the list, or earlier due to a 'break' or
> 'return' statement).
> 
> Also remove the get/put on the current bridge because it is not needed
> anymore. In fact all bridges in the encoder chain are refcounted already
> thanks to the drm_bridge_get() in drm_bridge_attach() and the
> drm_bridge_put() in drm_bridge_detach(). So while iterating with the mutex
> held the list cannot change _and_ the refcount of all bridges in the list
> cannot drop to zero.

This second paragraph *really* needs to be its own patch. And I'm not
really sure playing games when it comes to refcounting is a good idea.

A strict, simple, rule is way easier to follow than trying to figure out
two years from now why this loop skips the refcounting.

Unless you have a performance issue that is, in which case you should
add a comment (and we will need a meaningful benchmark to back that
claim).

> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> Changed in v2:
> - Fixed infinite loop in drm_for_each_bridge_in_chain_scoped() when
>   encoder->bridge_chain is empty, reported here:
>   https://lore.kernel.org/lkml/202509301358.38036b85-lkp@intel.com/
> - Slightly improved commit message
> ---
>  include/drm/drm_bridge.h | 62 ++++++++++++++++++++++++++----------------------
>  1 file changed, 33 insertions(+), 29 deletions(-)
> 
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 0ff7ab4aa8689a022458f935a7ffb23a2b715802..5a72817f04a78f5379f69e72fe519c5eb3ea043e 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -1440,26 +1440,29 @@ drm_bridge_chain_get_last_bridge(struct drm_encoder *encoder)
>  						      struct drm_bridge, chain_node));
>  }
>  
> -/**
> - * drm_bridge_get_next_bridge_and_put - Get the next bridge in the chain
> - *                                      and put the previous
> - * @bridge: bridge object
> - *
> - * Same as drm_bridge_get_next_bridge() but additionally puts the @bridge.
> - *
> - * RETURNS:
> - * the next bridge in the chain after @bridge, or NULL if @bridge is the last.
> - */
> -static inline struct drm_bridge *
> -drm_bridge_get_next_bridge_and_put(struct drm_bridge *bridge)
> +static inline struct drm_bridge *drm_bridge_encoder_chain_lock(struct drm_bridge *bridge)
>  {
> -	struct drm_bridge *next = drm_bridge_get_next_bridge(bridge);
> +	drm_encoder_chain_lock(bridge->encoder);
> +
> +	return bridge;
> +}

You create a public function, this needs to be documented.

Maxime

Hello Maxime,

On Fri, 3 Oct 2025 16:04:50 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> On Fri, Oct 03, 2025 at 12:39:26PM +0200, Luca Ceresoli wrote:
> > drm_for_each_bridge_in_chain_scoped() and
> > drm_for_each_bridge_in_chain_from() currently get/put the bridge at each
> > iteration. But they don't protect the encoder chain, so it could change
> > (bridges added/removed) while some code is iterating over the list
> > itself. To make iterations safe, change the logic of these for_each macros
> > to lock the encoder chain mutex at the beginning and unlock it at the end
> > of the loop (be it at the end of the list, or earlier due to a 'break' or
> > 'return' statement).
> > 
> > Also remove the get/put on the current bridge because it is not needed
> > anymore. In fact all bridges in the encoder chain are refcounted already
> > thanks to the drm_bridge_get() in drm_bridge_attach() and the
> > drm_bridge_put() in drm_bridge_detach(). So while iterating with the mutex
> > held the list cannot change _and_ the refcount of all bridges in the list
> > cannot drop to zero.  
> 
> This second paragraph *really* needs to be its own patch. And I'm not
> really sure playing games when it comes to refcounting is a good idea.
> 
> A strict, simple, rule is way easier to follow than trying to figure out
> two years from now why this loop skips the refcounting.
> 
> Unless you have a performance issue that is, in which case you should
> add a comment (and we will need a meaningful benchmark to back that
> claim).

Just to give some background, I have realized we need to lock the
encoder chain after drm_for_each_bridge_in_chain_scoped() was added.
Should I had realized it before, I would have sent it in the form you
can see in this patch, without the get/put because it is not necessary.
Not sure whether that would have changed the reception.

But I'm not aware of any performance issue, and the impact of
refcounting should not be small, soI'll try re-adding an explicit
get/put on top of the current version. It will likely make the macro
more complicated but should be reasonably doable. So, expect a v3 with
that change to we can all see how it looks.

Best regards,
Luca

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