The per-encoder bridge chain is currently assumed to be static once it is
fully initialized. Work is in progress to add hot-pluggable bridges,
breaking that assumption.

With bridge removal, the encoder chain can change without notice, removing
tail bridges. This can be problematic while iterating over the chain.

Add a mutex to be taken whenever looping or changing the encoder chain.

Also add two APIs to lock/unlock the mutex without the need to manipulate
internal struct drm_encoder fields.

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

---

Changes in v2:
- Added documentation to new APIs
---
 drivers/gpu/drm/drm_encoder.c |  2 ++
 include/drm/drm_encoder.h     | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index 8f2bc6a28482229fd0b030a1958f87753ad7885f..3261f142baea30c516499d23dbf8d0acf5952cd6 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -129,6 +129,7 @@ static int __drm_encoder_init(struct drm_device *dev,
 	}
 
 	INIT_LIST_HEAD(&encoder->bridge_chain);
+	mutex_init(&encoder->bridge_chain_mutex);
 	list_add_tail(&encoder->head, &dev->mode_config.encoder_list);
 	encoder->index = dev->mode_config.num_encoder++;
 
@@ -202,6 +203,7 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
 	kfree(encoder->name);
 	list_del(&encoder->head);
 	dev->mode_config.num_encoder--;
+	mutex_destroy(&encoder->bridge_chain_mutex);
 
 	memset(encoder, 0, sizeof(*encoder));
 }
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index 977a9381c8ba943b4d3e021635ea14856df8a17d..449281c37e39f67a0037603762f347f5086df983 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -25,6 +25,7 @@
 
 #include <linux/list.h>
 #include <linux/ctype.h>
+#include <linux/mutex.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_mode.h>
 #include <drm/drm_mode_object.h>
@@ -189,6 +190,9 @@ struct drm_encoder {
 	 */
 	struct list_head bridge_chain;
 
+	/** @bridge_chain_mutex: protect bridge_chain from changes while iterating */
+	struct mutex bridge_chain_mutex;
+
 	const struct drm_encoder_funcs *funcs;
 	const struct drm_encoder_helper_funcs *helper_private;
 
@@ -319,6 +323,41 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
 	return mo ? obj_to_encoder(mo) : NULL;
 }
 
+/**
+ * drm_encoder_chain_lock - lock the encoder bridge chain
+ * @encoder: encoder whose bridge chain must be locked
+ *
+ * Locks the mutex protecting the bridge chain from concurrent access.
+ * To be called by code modifying ot iterating over the bridge chain to
+ * prevent the list from changing while iterating over it.
+ * Call drm_encoder_chain_unlock() when done to unlock the mutex.
+ *
+ * Returns:
+ * Pointer to @encoder. Useful to lock the chain and then operate on the
+ * in the same statement, e.g.:
+ * list_first_entry_or_null(&drm_encoder_chain_lock(encoder)->bridge_chain)
+ */
+static inline struct drm_encoder *drm_encoder_chain_lock(struct drm_encoder *encoder)
+{
+	if (!WARN_ON_ONCE(!encoder))
+		mutex_lock(&encoder->bridge_chain_mutex);
+
+	return encoder;
+}
+
+/**
+ * drm_encoder_chain_unlock - unlock the encoder bridge chain
+ * @encoder: encoder whose bridge chain must be unlocked
+ *
+ * Unlocks the mutex protecting the bridge chain from concurrent access,
+ * matching drm_encoder_chain_lock().
+ */
+static inline void drm_encoder_chain_unlock(struct drm_encoder *encoder)
+{
+	if (!WARN_ON_ONCE(!encoder))
+		mutex_unlock(&encoder->bridge_chain_mutex);
+}
+
 void drm_encoder_cleanup(struct drm_encoder *encoder);
 
 /**

-- 
2.51.0

On Fri, Oct 03, 2025 at 12:39:23PM +0200, Luca Ceresoli wrote:
> The per-encoder bridge chain is currently assumed to be static once it is
> fully initialized. Work is in progress to add hot-pluggable bridges,
> breaking that assumption.
> 
> With bridge removal, the encoder chain can change without notice, removing
> tail bridges. This can be problematic while iterating over the chain.
> 
> Add a mutex to be taken whenever looping or changing the encoder chain.
> 
> Also add two APIs to lock/unlock the mutex without the need to manipulate
> internal struct drm_encoder fields.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> Changes in v2:
> - Added documentation to new APIs
> ---
>  drivers/gpu/drm/drm_encoder.c |  2 ++
>  include/drm/drm_encoder.h     | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index 8f2bc6a28482229fd0b030a1958f87753ad7885f..3261f142baea30c516499d23dbf8d0acf5952cd6 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -129,6 +129,7 @@ static int __drm_encoder_init(struct drm_device *dev,
>  	}
>  
>  	INIT_LIST_HEAD(&encoder->bridge_chain);
> +	mutex_init(&encoder->bridge_chain_mutex);
>  	list_add_tail(&encoder->head, &dev->mode_config.encoder_list);
>  	encoder->index = dev->mode_config.num_encoder++;
>  
> @@ -202,6 +203,7 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
>  	kfree(encoder->name);
>  	list_del(&encoder->head);
>  	dev->mode_config.num_encoder--;
> +	mutex_destroy(&encoder->bridge_chain_mutex);
>  
>  	memset(encoder, 0, sizeof(*encoder));
>  }
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index 977a9381c8ba943b4d3e021635ea14856df8a17d..449281c37e39f67a0037603762f347f5086df983 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -25,6 +25,7 @@
>  
>  #include <linux/list.h>
>  #include <linux/ctype.h>
> +#include <linux/mutex.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_mode.h>
>  #include <drm/drm_mode_object.h>
> @@ -189,6 +190,9 @@ struct drm_encoder {
>  	 */
>  	struct list_head bridge_chain;
>  
> +	/** @bridge_chain_mutex: protect bridge_chain from changes while iterating */
> +	struct mutex bridge_chain_mutex;
> +
>  	const struct drm_encoder_funcs *funcs;
>  	const struct drm_encoder_helper_funcs *helper_private;
>  
> @@ -319,6 +323,41 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
>  	return mo ? obj_to_encoder(mo) : NULL;
>  }
>  
> +/**
> + * drm_encoder_chain_lock - lock the encoder bridge chain
> + * @encoder: encoder whose bridge chain must be locked
> + *
> + * Locks the mutex protecting the bridge chain from concurrent access.
> + * To be called by code modifying ot iterating over the bridge chain to
> + * prevent the list from changing while iterating over it.
> + * Call drm_encoder_chain_unlock() when done to unlock the mutex.
> + *
> + * Returns:
> + * Pointer to @encoder. Useful to lock the chain and then operate on the
> + * in the same statement, e.g.:
> + * list_first_entry_or_null(&drm_encoder_chain_lock(encoder)->bridge_chain)
> + */
> +static inline struct drm_encoder *drm_encoder_chain_lock(struct drm_encoder *encoder)

What is the use case for these wrappers? I'm asking especially since
you almost never use the return value of the _lock() one. I think with
scoped_guard you can get the same kind of code without needing extra API
or extra wrappers.

> +{
> +	if (!WARN_ON_ONCE(!encoder))
> +		mutex_lock(&encoder->bridge_chain_mutex);
> +
> +	return encoder;
> +}
> +
> +/**
> + * drm_encoder_chain_unlock - unlock the encoder bridge chain
> + * @encoder: encoder whose bridge chain must be unlocked
> + *
> + * Unlocks the mutex protecting the bridge chain from concurrent access,
> + * matching drm_encoder_chain_lock().
> + */
> +static inline void drm_encoder_chain_unlock(struct drm_encoder *encoder)
> +{
> +	if (!WARN_ON_ONCE(!encoder))
> +		mutex_unlock(&encoder->bridge_chain_mutex);
> +}
> +
>  void drm_encoder_cleanup(struct drm_encoder *encoder);
>  
>  /**
> 
> -- 
> 2.51.0
> 

-- 
With best wishes
Dmitry

Hi Dmitry,

On Sat Oct 4, 2025 at 11:47 AM CEST, Dmitry Baryshkov wrote:

>> @@ -319,6 +323,41 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
>>  	return mo ? obj_to_encoder(mo) : NULL;
>>  }
>>  
>> +/**
>> + * drm_encoder_chain_lock - lock the encoder bridge chain
>> + * @encoder: encoder whose bridge chain must be locked
>> + *
>> + * Locks the mutex protecting the bridge chain from concurrent access.
>> + * To be called by code modifying ot iterating over the bridge chain to
>> + * prevent the list from changing while iterating over it.
>> + * Call drm_encoder_chain_unlock() when done to unlock the mutex.
>> + *
>> + * Returns:
>> + * Pointer to @encoder. Useful to lock the chain and then operate on the
>> + * in the same statement, e.g.:
>> + * list_first_entry_or_null(&drm_encoder_chain_lock(encoder)->bridge_chain)
>> + */
>> +static inline struct drm_encoder *drm_encoder_chain_lock(struct drm_encoder *encoder)
>
> What is the use case for these wrappers? I'm asking especially since
> you almost never use the return value of the _lock() one. I think with
> scoped_guard you can get the same kind of code without needing extra API
> or extra wrappers.

For two reasons.

One is to avoid drm_encoder users to need to access internal fields
(encapsulation, in object-oriented jargon). But if I read correctly between
the lines of your question, it is not worth because drm_bridge and
drm_encoder are already interdependent?

The second is that the C language spec sets tight constraints to the
drm_for_each_bridge_in_chain_scoped(). The macro must look like:

  #define drm_for_each_bridge_in_chain_scoped(encoder, bridge) \
          for (struct drm_bridge *bridge = <FOO>; clause-2; clause-3)
	       '----------- clause-1 ----------'

clause-1 must:

 * declare a 'struct drm_bridge *' variable (the loop cursor)
 * initialize it via <FOO> which thus must be a rvalue of type
   'struct drm_bridge *' (<FOO> must be a function or a macro, as a
   variable with the correct value is not available)
 * use the struct drm_encoder * as its sole input
 * lock the encoder chain mutex
 * get a reference to the bridge (as Maxime requested)
 * ensure the bridge reference is put and the mutex is released on break
   and return (clause-3 can't do that)

Given the above, we still need a function that locks the encoder chain
mutex and returns the encoder (bullets 3 and 4), like
drm_encoder_chain_lock(). I'm OK with removing drm_encoder_chain_lock() and
replace it with an internal macro or function in drm_bridge.h though.

However I'm not sure how to use scoped_guard here because it doesn't return
a pointer that can then be passed further. Basically we are constrained to
have a chain of function or macro calls, each eating the result of the
inner one, with the outer one returning a bridge pointer for the loop
cursor variable. There might be some macro magic I'm missing, in such case
don't hesitate to mention that.

Best regards,
Luca

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

Hi Dmitry,

On Tue Oct 7, 2025 at 11:45 AM CEST, Luca Ceresoli wrote:
> Hi Dmitry,
>
> On Sat Oct 4, 2025 at 11:47 AM CEST, Dmitry Baryshkov wrote:
>
>>> @@ -319,6 +323,41 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
>>>  	return mo ? obj_to_encoder(mo) : NULL;
>>>  }
>>>  
>>> +/**
>>> + * drm_encoder_chain_lock - lock the encoder bridge chain
>>> + * @encoder: encoder whose bridge chain must be locked
>>> + *
>>> + * Locks the mutex protecting the bridge chain from concurrent access.
>>> + * To be called by code modifying ot iterating over the bridge chain to
>>> + * prevent the list from changing while iterating over it.
>>> + * Call drm_encoder_chain_unlock() when done to unlock the mutex.
>>> + *
>>> + * Returns:
>>> + * Pointer to @encoder. Useful to lock the chain and then operate on the
>>> + * in the same statement, e.g.:
>>> + * list_first_entry_or_null(&drm_encoder_chain_lock(encoder)->bridge_chain)
>>> + */
>>> +static inline struct drm_encoder *drm_encoder_chain_lock(struct drm_encoder *encoder)
>>
>> What is the use case for these wrappers? I'm asking especially since
>> you almost never use the return value of the _lock() one. I think with
>> scoped_guard you can get the same kind of code without needing extra API
>> or extra wrappers.
>
> For two reasons.
>
> One is to avoid drm_encoder users to need to access internal fields
> (encapsulation, in object-oriented jargon). But if I read correctly between
> the lines of your question, it is not worth because drm_bridge and
> drm_encoder are already interdependent?
>
> The second is that the C language spec sets tight constraints to the
> drm_for_each_bridge_in_chain_scoped(). The macro must look like:
>
>   #define drm_for_each_bridge_in_chain_scoped(encoder, bridge) \
>           for (struct drm_bridge *bridge = <FOO>; clause-2; clause-3)
> 	       '----------- clause-1 ----------'
>
> clause-1 must:
>
>  * declare a 'struct drm_bridge *' variable (the loop cursor)
>  * initialize it via <FOO> which thus must be a rvalue of type
>    'struct drm_bridge *' (<FOO> must be a function or a macro, as a
>    variable with the correct value is not available)
>  * use the struct drm_encoder * as its sole input
>  * lock the encoder chain mutex
>  * get a reference to the bridge (as Maxime requested)
>  * ensure the bridge reference is put and the mutex is released on break
>    and return (clause-3 can't do that)
>
> Given the above, we still need a function that locks the encoder chain
> mutex and returns the encoder (bullets 3 and 4), like
> drm_encoder_chain_lock(). I'm OK with removing drm_encoder_chain_lock() and
> replace it with an internal macro or function in drm_bridge.h though.
>
> However I'm not sure how to use scoped_guard here because it doesn't return
> a pointer that can then be passed further. Basically we are constrained to
> have a chain of function or macro calls, each eating the result of the
> inner one, with the outer one returning a bridge pointer for the loop
> cursor variable. There might be some macro magic I'm missing, in such case
> don't hesitate to mention that.

I realized I was not fully clear, sorry about that. The inability to use
scoped_guard refers to the drm_for_each_bridge_in_chain_scoped()
implementation, being a macro defining a for loop. However scoped_guard can
be used in normal locking code such as the changes in patches 3, 6 and 7.

Luca

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