[PATCH v2 0/9] drm/bridge: get/put the bridge when looping over the encoder chain

[PATCH v2 0/9] drm/bridge: get/put the bridge when looping over the encoder chain

[Luca Ceresoli]

This series adds drm_bridge_get/put() calls for DRM bridges used when
looping over bridges in an encoder chain.

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)

Most loops on the encoder bridge chain are based on
drm_for_each_bridge_in_chain(), which does not get/put the bridge pointed
to by the loop iterator.

Add a scoped variant that ensures ta reference to the bridge pointed to by
the iterator is taken/released before/after every iteration (and when
breaking out of the loop). Using the scoped version, the bridge being
iterated on will always have a reference taken during the shole
iteration. This is similar to what for_each_child_of_node() and similar
for_each_* macros do.

All conversions are trivial except for drm_bridge_connector_init() which
needs some preliminary cleanups (patches 1-2).

omapdrm/omap_encoder.c is the only driver iterating over the encoder bridge
chain starting from a specific bridge, instead of iterating over the whole
list. For this use case, add a drm_for_each_bridge_in_chain_from() variant.

This series depends on:

 * commit 103578241512 ("drm/bridge-connector: Fix bridge in
   drm_connector_hdmi_audio_init()"), currently in drm-misc-fixes, not yet
   on drm-misc-next

[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>
---
Changes in v2:
- Prune series dependency list
- Clarify wording in cover letter and patch 3
- Link to v1: https://lore.kernel.org/r/20250723-drm-bridge-alloc-getput-for_each_bridge-v1-0-be8f4ae006e9@bootlin.com

---
Luca Ceresoli (9):
      drm/display: bridge-connector: use scope-specific variable for the bridge pointer
      drm/display: bridge-connector: remove unused variable assignment
      drm/bridge: add drm_for_each_bridge_in_chain_scoped()
      drm/display: bridge-connector: use drm_for_each_bridge_in_chain_scoped()
      drm/atomic: use drm_for_each_bridge_in_chain_scoped()
      drm/bridge: use drm_for_each_bridge_in_chain_scoped()
      drm/bridge: remove drm_for_each_bridge_in_chain()
      drm/bridge: add drm_for_each_bridge_in_chain_from()
      drm/omap: use drm_for_each_bridge_in_chain_from()

 .clang-format                                  |  2 +-
 drivers/gpu/drm/display/drm_bridge_connector.c | 13 +++----
 drivers/gpu/drm/drm_atomic.c                   |  3 +-
 drivers/gpu/drm/drm_bridge.c                   |  3 +-
 drivers/gpu/drm/omapdrm/omap_encoder.c         |  4 +-
 include/drm/drm_bridge.h                       | 52 ++++++++++++++++++++++++--
 6 files changed, 58 insertions(+), 19 deletions(-)
---
base-commit: d2b48f2b30f25997a1ae1ad0cefac68c25f8c330
change-id: 20250718-drm-bridge-alloc-getput-for_each_bridge-f141ae17b65a
prerequisite-message-id: <20250620011616.118-1-kernel@airkyi.com>
prerequisite-patch-id: ba5a6a15ea02bcee387db0e92ffb4cd0e1fbf816

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

[PATCH v2 1/9] drm/display: bridge-connector: use scope-specific variable for the bridge pointer

[Luca Ceresoli]

Currently drm_bridge_connector_init() reuses the 'bridge' variable, first
as a loop variable in the long drm_for_each_bridge_in_chain() and then in
the HDMI-specific code as a shortcut to bridge_connector->bridge_cec.

We are about to remove the 'bridge' loop variable for
drm_for_each_bridge_in_chain() by moving to a scoped version of the same
macro, which implies removing the 'bridge' variable from the main function
scope. Additionally reusing the variable can make such long function less
readable.

Similarly to what commit 6f727c838ea8 ("drm/bridge-connector: Fix bridge in
drm_connector_hdmi_audio_init()") already did for the audio HDMI bridge,
use n local variable inside the scopes where it is needed as a
bridge_connector->bridge_hdmi_cec shortcut to make its scope clearer as
well as to allow removing the 'bridge' variable in an upcoming commit.

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

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 8c915427d0538435661d771940efe38b462027a1..3ddde53b28131c3ce026413eb5518e9c8ed08b4d 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -816,7 +816,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 
 	if (bridge_connector->bridge_hdmi_cec &&
 	    bridge_connector->bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
-		bridge = bridge_connector->bridge_hdmi_cec;
+		struct drm_bridge *bridge = bridge_connector->bridge_hdmi_cec;
 
 		ret = drmm_connector_hdmi_cec_notifier_register(connector,
 								NULL,
@@ -827,7 +827,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 
 	if (bridge_connector->bridge_hdmi_cec &&
 	    bridge_connector->bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
-		bridge = bridge_connector->bridge_hdmi_cec;
+		struct drm_bridge *bridge = bridge_connector->bridge_hdmi_cec;
 
 		ret = drmm_connector_hdmi_cec_register(connector,
 						       &drm_bridge_connector_hdmi_cec_funcs,

-- 
2.50.1

[PATCH v2 2/9] drm/display: bridge-connector: remove unused variable assignment

[Luca Ceresoli]

The 'bridge' pointer started being assigned and used within this 'if' scope
in commit 0beba3f9d366 ("drm/bridge: connector: add support for HDMI codec
framework").

After that, commit 5d04b4188959 ("drm/bridge: split HDMI Audio from
DRM_BRIDGE_OP_HDMI") removed the code dereferencing it from the same 'if'
scope, but did not remove the assignment.

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

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 3ddde53b28131c3ce026413eb5518e9c8ed08b4d..65d54019e5eab323ecd3dc568541ed1e1793a683 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -772,8 +772,6 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 		if (!connector->ycbcr_420_allowed)
 			supported_formats &= ~BIT(HDMI_COLORSPACE_YUV420);
 
-		bridge = bridge_connector->bridge_hdmi;
-
 		ret = drmm_connector_hdmi_init(drm, connector,
 					       bridge_connector->bridge_hdmi->vendor,
 					       bridge_connector->bridge_hdmi->product,

-- 
2.50.1

[PATCH v2 3/9] drm/bridge: add drm_for_each_bridge_in_chain_scoped()

[Luca Ceresoli]

drm_for_each_bridge_in_chain() iterates ofer the bridges in an encoder
chain without protecting the lifetime of the bridges using
drm_bridge_get/put(). This creates a risk window where the bridge could be
freed while iterating on it. Users of drm_for_each_bridge_in_chain() cannot
solve this reliably.

Add variant of drm_for_each_bridge_in_chain() that gets/puts the bridge
reference at the beginning/end of each iteration, and puts it if breaking
ot of the loop.

Note that this requires adding a new drm_bridge_get_next_bridge_and_put()
function because, unlike similar functions as __of_get_next_child(),
drm_bridge_get_next_bridge() gets the "next" pointer but does not put the
"prev" pointer. Unfortunately drm_bridge_get_next_bridge() cannot be
modified to put the "prev" pointer because some of its users rely on
this, such as drm_atomic_bridge_propagate_bus_flags().

Also deprecate drm_for_each_bridge_in_chain(), in preparation for removing
it after converting all users to the scoped version.

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

---

Changes in v2:
- clarified commit message and mention an example where the current
  behaviour of drm_bridge_get_next_bridge() is wanted

Note 1: drm_for_each_bridge_in_chain_scoped() could be renamed removing the
        _scoped suffix after removing all the users of the current macro
        and eventually the current macro itself. Even though this series is
        converting all users, I'd at least wait one kernel release before
        renaming, to minimize issues with existing patches which would fail
        building.

Note 2: Yes, the drm_bridge_get_next_bridge_and_put() name is ugly, but we
        do need a "next_bridge" function that does not put the "prev"
        bridge and one that does. Any proposal for a better name or a
        different implementation is welcome.
---
 .clang-format            |  1 +
 include/drm/drm_bridge.h | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/.clang-format b/.clang-format
index 48405c54ef271e9546da08893d200a4cf48f3a55..1cac7d4976644c8f083f801e98f619782c2e23cc 100644
--- a/.clang-format
+++ b/.clang-format
@@ -168,6 +168,7 @@ ForEachMacros:
   - 'drm_exec_for_each_locked_object'
   - 'drm_exec_for_each_locked_object_reverse'
   - 'drm_for_each_bridge_in_chain'
+  - 'drm_for_each_bridge_in_chain_scoped'
   - 'drm_for_each_connector_iter'
   - 'drm_for_each_crtc'
   - 'drm_for_each_crtc_reverse'
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 620e119cc24c3491c2be5f08efaf51dfa8f708b3..a8e2f599aea764c705da3582df0ca428bb32f19c 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1365,10 +1365,51 @@ drm_bridge_chain_get_first_bridge(struct drm_encoder *encoder)
  *	    iteration
  *
  * Iterate over all bridges present in the bridge chain attached to @encoder.
+ *
+ * This is deprecated, do not use!
+ * New drivers shall use drm_for_each_bridge_in_chain_scoped().
  */
 #define drm_for_each_bridge_in_chain(encoder, bridge)			\
 	list_for_each_entry(bridge, &(encoder)->bridge_chain, 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)
+{
+	struct drm_bridge *next = drm_bridge_get_next_bridge(bridge);
+
+	drm_bridge_put(bridge);
+
+	return next;
+}
+
+/**
+ * drm_for_each_bridge_in_chain_scoped - iterate over all bridges attached
+ *                                       to an encoder
+ * @encoder: the encoder to iterate bridges on
+ * @bridge: a bridge pointer updated to point to the current bridge at each
+ *	    iteration
+ *
+ * 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.
+ */
+#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))
+
 enum drm_mode_status
 drm_bridge_chain_mode_valid(struct drm_bridge *bridge,
 			    const struct drm_display_info *info,

-- 
2.50.1

[PATCH v2 4/9] drm/display: bridge-connector: use drm_for_each_bridge_in_chain_scoped()

[Luca Ceresoli]

Use drm_for_each_bridge_in_chain_scoped() instead of
drm_for_each_bridge_in_chain() to ensure the bridge being looped on is
refcounted.

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

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 65d54019e5eab323ecd3dc568541ed1e1793a683..760ee00b6b02192dc1e59c213ef043a6bb9e3b60 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -136,10 +136,9 @@ static void drm_bridge_connector_hpd_notify(struct drm_connector *connector,
 {
 	struct drm_bridge_connector *bridge_connector =
 		to_drm_bridge_connector(connector);
-	struct drm_bridge *bridge;
 
 	/* Notify all bridges in the pipeline of hotplug events. */
-	drm_for_each_bridge_in_chain(bridge_connector->encoder, bridge) {
+	drm_for_each_bridge_in_chain_scoped(bridge_connector->encoder, bridge) {
 		if (bridge->funcs->hpd_notify)
 			bridge->funcs->hpd_notify(bridge, status);
 	}
@@ -638,7 +637,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 	struct drm_bridge_connector *bridge_connector;
 	struct drm_connector *connector;
 	struct i2c_adapter *ddc = NULL;
-	struct drm_bridge *bridge, *panel_bridge = NULL;
+	struct drm_bridge *panel_bridge = NULL;
 	unsigned int supported_formats = BIT(HDMI_COLORSPACE_RGB);
 	unsigned int max_bpc = 8;
 	int connector_type;
@@ -665,7 +664,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 	 * detection are available, we don't support hotplug detection at all.
 	 */
 	connector_type = DRM_MODE_CONNECTOR_Unknown;
-	drm_for_each_bridge_in_chain(encoder, bridge) {
+	drm_for_each_bridge_in_chain_scoped(encoder, bridge) {
 		if (!bridge->interlace_allowed)
 			connector->interlace_allowed = false;
 		if (!bridge->ycbcr_420_allowed)

-- 
2.50.1

[PATCH v2 5/9] drm/atomic: use drm_for_each_bridge_in_chain_scoped()

[Luca Ceresoli]

Use drm_for_each_bridge_in_chain_scoped() instead of
drm_for_each_bridge_in_chain() to ensure the bridge being looped on is
refcounted.

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

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index cd15cf52f0c9144711da5879da57884674aea9e4..ed5359a71f7e2cd8fa52b993e62ee65f8fed4537 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1308,7 +1308,6 @@ drm_atomic_add_encoder_bridges(struct drm_atomic_state *state,
 			       struct drm_encoder *encoder)
 {
 	struct drm_bridge_state *bridge_state;
-	struct drm_bridge *bridge;
 
 	if (!encoder)
 		return 0;
@@ -1317,7 +1316,7 @@ drm_atomic_add_encoder_bridges(struct drm_atomic_state *state,
 		       "Adding all bridges for [encoder:%d:%s] to %p\n",
 		       encoder->base.id, encoder->name, state);
 
-	drm_for_each_bridge_in_chain(encoder, bridge) {
+	drm_for_each_bridge_in_chain_scoped(encoder, bridge) {
 		/* Skip bridges that don't implement the atomic state hooks. */
 		if (!bridge->funcs->atomic_duplicate_state)
 			continue;

-- 
2.50.1

[PATCH v2 6/9] drm/bridge: use drm_for_each_bridge_in_chain_scoped()

[Luca Ceresoli]

Use drm_for_each_bridge_in_chain_scoped() instead of
drm_for_each_bridge_in_chain() to ensure the bridge being looped on is
refcounted.

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

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c3bfcd735a3c426a147bf0a7427b3d2cd0df3524..c91a99b7eb1b9b1525e2d95888952f733ca6b9e0 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -1476,10 +1476,9 @@ static int encoder_bridges_show(struct seq_file *m, void *data)
 {
 	struct drm_encoder *encoder = m->private;
 	struct drm_printer p = drm_seq_file_printer(m);
-	struct drm_bridge *bridge;
 	unsigned int idx = 0;
 
-	drm_for_each_bridge_in_chain(encoder, bridge)
+	drm_for_each_bridge_in_chain_scoped(encoder, bridge)
 		drm_bridge_debugfs_show_bridge(&p, bridge, idx++);
 
 	return 0;

-- 
2.50.1

[PATCH v2 7/9] drm/bridge: remove drm_for_each_bridge_in_chain()

[Luca Ceresoli]

All users have been replaced by drm_for_each_bridge_in_chain_scoped().

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 .clang-format            |  1 -
 include/drm/drm_bridge.h | 14 --------------
 2 files changed, 15 deletions(-)

diff --git a/.clang-format b/.clang-format
index 1cac7d4976644c8f083f801e98f619782c2e23cc..d5c05db1a0d96476b711b95912d2b82b2e780397 100644
--- a/.clang-format
+++ b/.clang-format
@@ -167,7 +167,6 @@ ForEachMacros:
   - 'drm_connector_for_each_possible_encoder'
   - 'drm_exec_for_each_locked_object'
   - 'drm_exec_for_each_locked_object_reverse'
-  - 'drm_for_each_bridge_in_chain'
   - 'drm_for_each_bridge_in_chain_scoped'
   - 'drm_for_each_connector_iter'
   - 'drm_for_each_crtc'
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index a8e2f599aea764c705da3582df0ca428bb32f19c..6adf9221c2d462ec8e0e4e281c97b39081b3da24 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1358,20 +1358,6 @@ drm_bridge_chain_get_first_bridge(struct drm_encoder *encoder)
 						       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
- * @bridge: a bridge pointer updated to point to the current bridge at each
- *	    iteration
- *
- * Iterate over all bridges present in the bridge chain attached to @encoder.
- *
- * This is deprecated, do not use!
- * New drivers shall use drm_for_each_bridge_in_chain_scoped().
- */
-#define drm_for_each_bridge_in_chain(encoder, bridge)			\
-	list_for_each_entry(bridge, &(encoder)->bridge_chain, chain_node)
-
 /**
  * drm_bridge_get_next_bridge_and_put - Get the next bridge in the chain
  *                                      and put the previous

-- 
2.50.1

[PATCH v2 8/9] drm/bridge: add drm_for_each_bridge_in_chain_from()

[Luca Ceresoli]

Add variant of drm_for_each_bridge_in_chain_scoped() that iterates on the
encoder bridge from a given bridge until the end of the chain.

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

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 6adf9221c2d462ec8e0e4e281c97b39081b3da24..6a79edcb4a8476a8eefa6ff00771b0f919de0f6b 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1396,6 +1396,25 @@ drm_bridge_get_next_bridge_and_put(struct drm_bridge *bridge)
 	     bridge;							\
 	     bridge = drm_bridge_get_next_bridge_and_put(bridge))
 
+/**
+ * drm_for_each_bridge_in_chain_from - iterate over all bridges starting
+ *                                     from the given bridge
+ * @first_bridge: the bridge to start from
+ * @bridge: a bridge pointer updated to point to the current bridge at each
+ *	    iteration
+ *
+ * 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.
+ */
+#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))
+
 enum drm_mode_status
 drm_bridge_chain_mode_valid(struct drm_bridge *bridge,
 			    const struct drm_display_info *info,

-- 
2.50.1

[PATCH v2 9/9] drm/omap: use drm_for_each_bridge_in_chain_from()

[Luca Ceresoli]

Use drm_for_each_bridge_in_chain_from _scoped() instead of an open-coded
loop based on drm_bridge_get_next_bridge() to ensure the bridge being
looped on is refcounted and simplify the driver code.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c
index 4dd05bc732daebcedbfcbbc9ba7dffee7415bdfc..195715b162e38ea0cd0870f3dd79342a8cbf218b 100644
--- a/drivers/gpu/drm/omapdrm/omap_encoder.c
+++ b/drivers/gpu/drm/omapdrm/omap_encoder.c
@@ -77,7 +77,6 @@ static void omap_encoder_mode_set(struct drm_encoder *encoder,
 	struct omap_dss_device *output = omap_encoder->output;
 	struct drm_device *dev = encoder->dev;
 	struct drm_connector *connector;
-	struct drm_bridge *bridge;
 	struct videomode vm = { 0 };
 	u32 bus_flags;
 
@@ -97,8 +96,7 @@ static void omap_encoder_mode_set(struct drm_encoder *encoder,
 	 *
 	 * A better solution is to use DRM's bus-flags through the whole driver.
 	 */
-	for (bridge = output->bridge; bridge;
-	     bridge = drm_bridge_get_next_bridge(bridge)) {
+	drm_for_each_bridge_in_chain_from(output->bridge, bridge) {
 		if (!bridge->timings)
 			continue;
 

-- 
2.50.1

Re: [PATCH v2 0/9] drm/bridge: get/put the bridge when looping over the encoder chain

[Luca Ceresoli]

On Fri, 08 Aug 2025 16:49:07 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> This series depends on:
> 
>  * commit 103578241512 ("drm/bridge-connector: Fix bridge in
>    drm_connector_hdmi_audio_init()"), currently in drm-misc-fixes, not yet
>    on drm-misc-next

Just a quick update: this commit is now in drm-misc-next, so this patch
has zero dependencies now.

Luca

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

Re: [PATCH v2 1/9] drm/display: bridge-connector: use scope-specific variable for the bridge pointer

[Maxime Ripard]

On Fri, 8 Aug 2025 16:49:08 +0200, Luca Ceresoli wrote:
> Currently drm_bridge_connector_init() reuses the 'bridge' variable, first
> as a loop variable in the long drm_for_each_bridge_in_chain() and then in
> the HDMI-specific code as a shortcut to bridge_connector->bridge_cec.
> 
> We are about to remove the 'bridge' loop variable for
> 
> [ ... ]

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

Thanks!
Maxime

Re: [PATCH v2 2/9] drm/display: bridge-connector: remove unused variable assignment

[Maxime Ripard]

On Fri, 8 Aug 2025 16:49:09 +0200, Luca Ceresoli wrote:
> The 'bridge' pointer started being assigned and used within this 'if' scope
> in commit 0beba3f9d366 ("drm/bridge: connector: add support for HDMI codec
> framework").
> 
> After that, commit 5d04b4188959 ("drm/bridge: split HDMI Audio from
> 
> [ ... ]

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

Thanks!
Maxime

Re: [PATCH v2 3/9] drm/bridge: add drm_for_each_bridge_in_chain_scoped()

[Maxime Ripard]

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

On Fri, Aug 08, 2025 at 04:49:10PM +0200, Luca Ceresoli wrote:
> drm_for_each_bridge_in_chain() iterates ofer the bridges in an encoder
> chain without protecting the lifetime of the bridges using
> drm_bridge_get/put(). This creates a risk window where the bridge could be
> freed while iterating on it. Users of drm_for_each_bridge_in_chain() cannot
> solve this reliably.
> 
> Add variant of drm_for_each_bridge_in_chain() that gets/puts the bridge
> reference at the beginning/end of each iteration, and puts it if breaking
> ot of the loop.
> 
> Note that this requires adding a new drm_bridge_get_next_bridge_and_put()
> function because, unlike similar functions as __of_get_next_child(),
> drm_bridge_get_next_bridge() gets the "next" pointer but does not put the
> "prev" pointer. Unfortunately drm_bridge_get_next_bridge() cannot be
> modified to put the "prev" pointer because some of its users rely on
> this, such as drm_atomic_bridge_propagate_bus_flags().
> 
> Also deprecate drm_for_each_bridge_in_chain(), in preparation for removing
> it after converting all users to the scoped version.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> Changes in v2:
> - clarified commit message and mention an example where the current
>   behaviour of drm_bridge_get_next_bridge() is wanted
> 
> Note 1: drm_for_each_bridge_in_chain_scoped() could be renamed removing the
>         _scoped suffix after removing all the users of the current macro
>         and eventually the current macro itself. Even though this series is
>         converting all users, I'd at least wait one kernel release before
>         renaming, to minimize issues with existing patches which would fail
>         building.
> 
> Note 2: Yes, the drm_bridge_get_next_bridge_and_put() name is ugly, but we
>         do need a "next_bridge" function that does not put the "prev"
>         bridge and one that does. Any proposal for a better name or a
>         different implementation is welcome.
> ---
>  .clang-format            |  1 +
>  include/drm/drm_bridge.h | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/.clang-format b/.clang-format
> index 48405c54ef271e9546da08893d200a4cf48f3a55..1cac7d4976644c8f083f801e98f619782c2e23cc 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -168,6 +168,7 @@ ForEachMacros:
>    - 'drm_exec_for_each_locked_object'
>    - 'drm_exec_for_each_locked_object_reverse'
>    - 'drm_for_each_bridge_in_chain'
> +  - 'drm_for_each_bridge_in_chain_scoped'
>    - 'drm_for_each_connector_iter'
>    - 'drm_for_each_crtc'
>    - 'drm_for_each_crtc_reverse'
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 620e119cc24c3491c2be5f08efaf51dfa8f708b3..a8e2f599aea764c705da3582df0ca428bb32f19c 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -1365,10 +1365,51 @@ drm_bridge_chain_get_first_bridge(struct drm_encoder *encoder)
>   *	    iteration
>   *
>   * Iterate over all bridges present in the bridge chain attached to @encoder.
> + *
> + * This is deprecated, do not use!
> + * New drivers shall use drm_for_each_bridge_in_chain_scoped().
>   */
>  #define drm_for_each_bridge_in_chain(encoder, bridge)			\
>  	list_for_each_entry(bridge, &(encoder)->bridge_chain, 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)
> +{
> +	struct drm_bridge *next = drm_bridge_get_next_bridge(bridge);
> +
> +	drm_bridge_put(bridge);
> +
> +	return next;
> +}
> +
> +/**
> + * drm_for_each_bridge_in_chain_scoped - iterate over all bridges attached
> + *                                       to an encoder
> + * @encoder: the encoder to iterate bridges on
> + * @bridge: a bridge pointer updated to point to the current bridge at each
> + *	    iteration
> + *
> + * 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.
> + */
> +#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);		\

So my understanding is that the initial value of bridge would be cleaned
up with drm_bridge_put...

> +	     bridge;							\
> +	     bridge = drm_bridge_get_next_bridge_and_put(bridge))

... but also when iterating?

So if we have more than 0 values, we put two references?

Maxime

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

Re: [PATCH v2 3/9] drm/bridge: add drm_for_each_bridge_in_chain_scoped()

[Luca Ceresoli]

Hi Maxime,

On Tue, 19 Aug 2025 15:47:06 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> > +/**
> > + * drm_for_each_bridge_in_chain_scoped - iterate over all bridges attached
> > + *                                       to an encoder
> > + * @encoder: the encoder to iterate bridges on
> > + * @bridge: a bridge pointer updated to point to the current bridge at each
> > + *	    iteration
> > + *
> > + * 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.
> > + */
> > +#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);		\  
> 
> So my understanding is that the initial value of bridge would be cleaned
> up with drm_bridge_put...
> 
> > +	     bridge;							\
> > +	     bridge = drm_bridge_get_next_bridge_and_put(bridge))  
> 
> ... but also when iterating?
> 
> So if we have more than 0 values, we put two references?

No, this is not the case. The __free action is executed only when
exiting the entire for loop, not a single iteration.

This is consistent with the fact that the loop variable is persistent
across iterations.

I tested this macro in both cases:

 * looping over the entire chain the final value of @bridge will be
   NULL and the cleanup action won't call drm_bridge_put()
 * breaking before the last element, @bridge is non-NULL and the
   cleanup action does call drm_bridge_put()

See examples such as for_each_child_of_node_scoped() and other OF
iterators which work in the same way (which is no coincidence, I used
them as starting point for writing this patch).

Best regards,
Luca

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

Re: [PATCH v2 3/9] drm/bridge: add drm_for_each_bridge_in_chain_scoped()

[Luca Ceresoli]

Hi Maxime,

On Tue, 19 Aug 2025 18:01:37 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> Hi Maxime,
> 
> On Tue, 19 Aug 2025 15:47:06 +0200
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > > +/**
> > > + * drm_for_each_bridge_in_chain_scoped - iterate over all bridges attached
> > > + *                                       to an encoder
> > > + * @encoder: the encoder to iterate bridges on
> > > + * @bridge: a bridge pointer updated to point to the current bridge at each
> > > + *	    iteration
> > > + *
> > > + * 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.
> > > + */
> > > +#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);		\    
> > 
> > So my understanding is that the initial value of bridge would be cleaned
> > up with drm_bridge_put...
> >   
> > > +	     bridge;							\
> > > +	     bridge = drm_bridge_get_next_bridge_and_put(bridge))    
> > 
> > ... but also when iterating?
> > 
> > So if we have more than 0 values, we put two references?  
> 
> No, this is not the case. The __free action is executed only when
> exiting the entire for loop, not a single iteration.
> 
> This is consistent with the fact that the loop variable is persistent
> across iterations.

PS: here's the C language spec reference:

> 6.8.5.3 The for statement
> The statement
> for ( clause-1 ; expression-2 ; expression-3 ) statement
> behaves as follows:
> [...]
> If clause-1 is a declaration, the scope of any identifiers it declares
> is the remainder of the declaration and the entire loop 

https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
https://rgambord.github.io/c99-doc/sections/6/8/5/3/index.html

Best regards,
Luca

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

Re: [PATCH v2 3/9] drm/bridge: add drm_for_each_bridge_in_chain_scoped()

[Luca Ceresoli]

Hi Maxime,

On Wed, 20 Aug 2025 11:40:30 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> Hi Maxime,
> 
> On Tue, 19 Aug 2025 18:01:37 +0200
> Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> 
> > Hi Maxime,
> > 
> > On Tue, 19 Aug 2025 15:47:06 +0200
> > Maxime Ripard <mripard@kernel.org> wrote:
> >   
> > > > +/**
> > > > + * drm_for_each_bridge_in_chain_scoped - iterate over all bridges attached
> > > > + *                                       to an encoder
> > > > + * @encoder: the encoder to iterate bridges on
> > > > + * @bridge: a bridge pointer updated to point to the current bridge at each
> > > > + *	    iteration
> > > > + *
> > > > + * 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.
> > > > + */
> > > > +#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);		\      
> > > 
> > > So my understanding is that the initial value of bridge would be cleaned
> > > up with drm_bridge_put...
> > >     
> > > > +	     bridge;							\
> > > > +	     bridge = drm_bridge_get_next_bridge_and_put(bridge))      
> > > 
> > > ... but also when iterating?
> > > 
> > > So if we have more than 0 values, we put two references?    
> > 
> > No, this is not the case. The __free action is executed only when
> > exiting the entire for loop, not a single iteration.
> > 
> > This is consistent with the fact that the loop variable is persistent
> > across iterations.  
> 
> PS: here's the C language spec reference:
> 
> > 6.8.5.3 The for statement
> > The statement
> > for ( clause-1 ; expression-2 ; expression-3 ) statement
> > behaves as follows:
> > [...]
> > If clause-1 is a declaration, the scope of any identifiers it declares
> > is the remainder of the declaration and the entire loop   
> 
> https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
> https://rgambord.github.io/c99-doc/sections/6/8/5/3/index.html

I think my replies have proven the correctness of the bridge cleanup in
this patch. Based on my arguments, do you agree this patch is correct?

If it is, I think most of the remainder of this series is trivial to
review, and it would be a good step forward for dynamic bridge lifetime
implementation.

Otherwise, don't hesitate to let me know your concerns.

Best regards,
Luca

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