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.ceres...@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
signature.asc
Description: PGP signature
