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

[Maxime Ripard <mripard@kernel.org>]

[-- 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 --]