[PATCH 01/26] drm/bridge: add drm_of_find_bridge()

Luca Ceresoli posted 26 patches 1 week, 5 days ago
There is a newer version of this series
[PATCH 01/26] drm/bridge: add drm_of_find_bridge()
Posted by Luca Ceresoli 1 week, 5 days ago
of_drm_find_bridge() does not increment the refcount for the returned
bridge, but that is required now. However converting it and all its users
is not realistically doable at once given the large amount of (direct and
indirect) callers and the complexity of some. Also, "of_drm_find_bridge is
oddly named according to our convention and it would make more sense to be
called drm_of_find_bridge()" (quoted from Link: below).

Solve both issues by creating a new drm_of_find_bridge() that is identical
to of_drm_find_bridge() except it takes a reference. Then
of_drm_find_bridge() will be deprecated to be eventually removed.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Link: https://lore.kernel.org/dri-devel/20250319-stylish-lime-mongoose-0a18ad@houat/
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Note: a simple implementation would just be
  { return drm_bridge_get(of_drm_find_bridge(np)); }
but it would release the mutex before getting the reference, so it is
not safe. Make things simple by duplicating the code. A later patch will
make instead the (to be deprecated) of_drm_find_bridge() become a wrapper
of the new drm_of_find_bridge()
---
 drivers/gpu/drm/drm_bridge.c | 29 +++++++++++++++++++++++++++++
 include/drm/drm_bridge.h     |  5 +++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 8f355df883d8..d98a7b4a83c0 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -1417,6 +1417,35 @@ void drm_bridge_hpd_notify(struct drm_bridge *bridge,
 EXPORT_SYMBOL_GPL(drm_bridge_hpd_notify);
 
 #ifdef CONFIG_OF
+/**
+ * drm_of_find_bridge - find the bridge corresponding to the device node in
+ *			the global bridge list
+ * @np: device node
+ *
+ * The refcount of the returned bridge is incremented. Use drm_bridge_put()
+ * when done with it.
+ *
+ * RETURNS:
+ * drm_bridge control struct on success, NULL on failure
+ */
+struct drm_bridge *drm_of_find_bridge(struct device_node *np)
+{
+	struct drm_bridge *bridge;
+
+	mutex_lock(&bridge_lock);
+
+	list_for_each_entry(bridge, &bridge_list, list) {
+		if (bridge->of_node == np) {
+			mutex_unlock(&bridge_lock);
+			return drm_bridge_get(bridge);
+		}
+	}
+
+	mutex_unlock(&bridge_lock);
+	return NULL;
+}
+EXPORT_SYMBOL(drm_of_find_bridge);
+
 /**
  * of_drm_find_bridge - find the bridge corresponding to the device node in
  *			the global bridge list
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 0ff7ab4aa868..e74e91004c48 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1313,8 +1313,13 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 		      enum drm_bridge_attach_flags flags);
 
 #ifdef CONFIG_OF
+struct drm_bridge *drm_of_find_bridge(struct device_node *np);
 struct drm_bridge *of_drm_find_bridge(struct device_node *np);
 #else
+static inline struct drm_bridge *drm_of_find_bridge(struct device_node *np)
+{
+	return NULL;
+}
 static inline struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 {
 	return NULL;

-- 
2.51.1
Re: [PATCH 01/26] drm/bridge: add drm_of_find_bridge()
Posted by Maxime Ripard 1 week ago
Hi,

On Wed, Nov 19, 2025 at 02:05:32PM +0100, Luca Ceresoli wrote:
>  #ifdef CONFIG_OF
> +/**
> + * drm_of_find_bridge - find the bridge corresponding to the device node in
> + *			the global bridge list
> + * @np: device node
> + *
> + * The refcount of the returned bridge is incremented. Use drm_bridge_put()
> + * when done with it.
> + *
> + * RETURNS:
> + * drm_bridge control struct on success, NULL on failure
> + */
> +struct drm_bridge *drm_of_find_bridge(struct device_node *np)

So the convention we've mostly had was that the first argument would
define the prefix, ie. if we pass a drm_* pointer, the prefix is drm, if
we pass a device_node pointer, then the prefix is of.

Considering that convention, of_drm_find_bridge would be the ideal
candidate, but we can't use that obviously. What about
of_drm_find_and_get_bridge, or of_drm_get_bridge?

Maxime
Re: [PATCH 01/26] drm/bridge: add drm_of_find_bridge()
Posted by Luca Ceresoli 1 week ago
Hello Maxime,

On Mon Nov 24, 2025 at 11:15 AM CET, Maxime Ripard wrote:
> Hi,
>
> On Wed, Nov 19, 2025 at 02:05:32PM +0100, Luca Ceresoli wrote:
>>  #ifdef CONFIG_OF
>> +/**
>> + * drm_of_find_bridge - find the bridge corresponding to the device node in
>> + *			the global bridge list
>> + * @np: device node
>> + *
>> + * The refcount of the returned bridge is incremented. Use drm_bridge_put()
>> + * when done with it.
>> + *
>> + * RETURNS:
>> + * drm_bridge control struct on success, NULL on failure
>> + */
>> +struct drm_bridge *drm_of_find_bridge(struct device_node *np)
>
> So the convention we've mostly had was that the first argument would
> define the prefix, ie. if we pass a drm_* pointer, the prefix is drm, if
> we pass a device_node pointer, then the prefix is of.
>
> Considering that convention, of_drm_find_bridge would be the ideal
> candidate, but we can't use that obviously. What about
> of_drm_find_and_get_bridge, or of_drm_get_bridge?

Ah, it sounded the other way around during the old discussion [0]. :-) But
no problem in using a different name of course. of_drm_get_bridge() looks
like the best to me, so I'll rename that way in v2.

[0] https://lore.kernel.org/dri-devel/20250319-stylish-lime-mongoose-0a18ad@houat/

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [PATCH 01/26] drm/bridge: add drm_of_find_bridge()
Posted by Louis Chauvet 1 week, 5 days ago

On 11/19/25 13:05, Luca Ceresoli wrote:
> of_drm_find_bridge() does not increment the refcount for the returned
> bridge, but that is required now. However converting it and all its users
> is not realistically doable at once given the large amount of (direct and
> indirect) callers and the complexity of some. Also, "of_drm_find_bridge is
> oddly named according to our convention and it would make more sense to be
> called drm_of_find_bridge()" (quoted from Link: below).
> 
> Solve both issues by creating a new drm_of_find_bridge() that is identical
> to of_drm_find_bridge() except it takes a reference. Then
> of_drm_find_bridge() will be deprecated to be eventually removed.
> 
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Link: https://lore.kernel.org/dri-devel/20250319-stylish-lime-mongoose-0a18ad@houat/
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> Note: a simple implementation would just be
>    { return drm_bridge_get(of_drm_find_bridge(np)); }
> but it would release the mutex before getting the reference, so it is
> not safe. Make things simple by duplicating the code. A later patch will
> make instead the (to be deprecated) of_drm_find_bridge() become a wrapper
> of the new drm_of_find_bridge()
> ---
>   drivers/gpu/drm/drm_bridge.c | 29 +++++++++++++++++++++++++++++
>   include/drm/drm_bridge.h     |  5 +++++
>   2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 8f355df883d8..d98a7b4a83c0 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -1417,6 +1417,35 @@ void drm_bridge_hpd_notify(struct drm_bridge *bridge,
>   EXPORT_SYMBOL_GPL(drm_bridge_hpd_notify);
>   
>   #ifdef CONFIG_OF
> +/**
> + * drm_of_find_bridge - find the bridge corresponding to the device node in
> + *			the global bridge list
> + * @np: device node
> + *
> + * The refcount of the returned bridge is incremented. Use drm_bridge_put()
> + * when done with it.
> + *
> + * RETURNS:
> + * drm_bridge control struct on success, NULL on failure
> + */
> +struct drm_bridge *drm_of_find_bridge(struct device_node *np)
> +{
> +	struct drm_bridge *bridge;
> +
> +	mutex_lock(&bridge_lock);
> +
> +	list_for_each_entry(bridge, &bridge_list, list) {
> +		if (bridge->of_node == np) {
> +			mutex_unlock(&bridge_lock);

It seems a bit strange to unlock the mutex just before the 
drm_bridge_get, is it expected?

If no, I think you can use scoped_guard(mutex, &bridge_lock) to avoid 
messing with mutex_unlock, IIRC, scoped_guard will unlock the mutex just 
after the return, so in your case, just after the drm_bridge_get.

> +			return drm_bridge_get(bridge);
> +		}
> +	}
> +
> +	mutex_unlock(&bridge_lock);
> +	return NULL;
> +}
> +EXPORT_SYMBOL(drm_of_find_bridge);
> +
>   /**
>    * of_drm_find_bridge - find the bridge corresponding to the device node in
>    *			the global bridge list
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 0ff7ab4aa868..e74e91004c48 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -1313,8 +1313,13 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>   		      enum drm_bridge_attach_flags flags);
>   
>   #ifdef CONFIG_OF
> +struct drm_bridge *drm_of_find_bridge(struct device_node *np);
>   struct drm_bridge *of_drm_find_bridge(struct device_node *np);
>   #else
> +static inline struct drm_bridge *drm_of_find_bridge(struct device_node *np)
> +{
> +	return NULL;
> +}
>   static inline struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>   {
>   	return NULL;
>
Re: [PATCH 01/26] drm/bridge: add drm_of_find_bridge()
Posted by Luca Ceresoli 1 week, 5 days ago
Hi Louis,

On Wed Nov 19, 2025 at 3:22 PM CET, Louis Chauvet wrote:
>
>
> On 11/19/25 13:05, Luca Ceresoli wrote:
>> of_drm_find_bridge() does not increment the refcount for the returned
>> bridge, but that is required now. However converting it and all its users
>> is not realistically doable at once given the large amount of (direct and
>> indirect) callers and the complexity of some. Also, "of_drm_find_bridge is
>> oddly named according to our convention and it would make more sense to be
>> called drm_of_find_bridge()" (quoted from Link: below).
>>
>> Solve both issues by creating a new drm_of_find_bridge() that is identical
>> to of_drm_find_bridge() except it takes a reference. Then
>> of_drm_find_bridge() will be deprecated to be eventually removed.
>>
>> Suggested-by: Maxime Ripard <mripard@kernel.org>
>> Link: https://lore.kernel.org/dri-devel/20250319-stylish-lime-mongoose-0a18ad@houat/
>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

...

>> +struct drm_bridge *drm_of_find_bridge(struct device_node *np)
>> +{
>> +	struct drm_bridge *bridge;
>> +
>> +	mutex_lock(&bridge_lock);
>> +
>> +	list_for_each_entry(bridge, &bridge_list, list) {
>> +		if (bridge->of_node == np) {
>> +			mutex_unlock(&bridge_lock);
>
> It seems a bit strange to unlock the mutex just before the
> drm_bridge_get, is it expected?

Ouch. No, it's not expected, it is a very silly mistake. Thanks for
noticing.

> If no, I think you can use scoped_guard(mutex, &bridge_lock) to avoid
> messing with mutex_unlock, IIRC, scoped_guard will unlock the mutex just
> after the return, so in your case, just after the drm_bridge_get.
>
>> +			return drm_bridge_get(bridge);
>> +		}
>> +	}

My intent was to keep the function as similar as possible to the original
one, thus I just added a drm_bridge_get(), but that is of course wrong.

So these lines should instead have been:

	if (bridge->of_node == np) {
		drm_bridge_get(bridge);
		mutex_unlock(&bridge_lock);
		return bridge;
	}

But indeed scoped_guard() is much cleaner and less error-prone, so I'm
probably going to use it in v2.

Luca

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