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 <[email protected]> > > --- > > 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/[email protected]/ > - 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
signature.asc
Description: PGP signature
