[PATCH] drm/bridge: analogix_dp: Use devm_drm_bridge_alloc() API

[PATCH] drm/bridge: analogix_dp: Use devm_drm_bridge_alloc() API

[Marek Szyprowski]

devm_drm_bridge_alloc() is the new API to be used for allocating
(and partially initializing) a private driver struct embedding
a struct drm_bridge.

Analogix DP driver somehow missed the automated conversion in commit
9c399719cfb9 ("drm: convert many bridge drivers from devm_kzalloc() to
devm_drm_bridge_alloc() API"), what causes the following warning:

------------[ cut here ]------------
WARNING: lib/refcount.c:25 at drm_bridge_attach+0x2c/0x248, CPU#1: kworker/u8:1/34
refcount_t: addition on 0; use-after-free.
Modules linked in:
CPU: 1 UID: 0 PID: 34 Comm: kworker/u8:1 Not tainted 6.16.0-rc3-next-20250627-dirty #15839 PREEMPT
Hardware name: Samsung Exynos (Flattened Device Tree)
Workqueue: events_unbound deferred_probe_work_func
Call trace:
 unwind_backtrace from show_stack+0x10/0x14
 show_stack from dump_stack_lvl+0x68/0x88
 dump_stack_lvl from __warn+0x94/0x1f0
 __warn from warn_slowpath_fmt+0x124/0x1bc
 warn_slowpath_fmt from drm_bridge_attach+0x2c/0x248
 drm_bridge_attach from analogix_dp_bind+0x70/0xc8
 analogix_dp_bind from exynos_dp_bind+0x58/0xc4
 exynos_dp_bind from component_bind_all+0x11c/0x27c
 component_bind_all from exynos_drm_bind+0xe8/0x198
 exynos_drm_bind from try_to_bring_up_aggregate_device+0x200/0x2d8
 try_to_bring_up_aggregate_device from __component_add+0xb0/0x170
 __component_add from exynos_dp_probe+0xc0/0x164
 exynos_dp_probe from platform_probe+0x5c/0xb8
 platform_probe from really_probe+0xe0/0x3d8
 really_probe from __driver_probe_device+0x9c/0x1e0
 __driver_probe_device from driver_probe_device+0x30/0xc0
 driver_probe_device from __device_attach_driver+0xa8/0x120
 __device_attach_driver from bus_for_each_drv+0x84/0xdc
 bus_for_each_drv from __device_attach+0xb0/0x20c
 __device_attach from bus_probe_device+0x8c/0x90
 bus_probe_device from deferred_probe_work_func+0x98/0xe0
 deferred_probe_work_func from process_one_work+0x24c/0x70c
 process_one_work from worker_thread+0x1b8/0x3bc
 worker_thread from kthread+0x13c/0x264
 kthread from ret_from_fork+0x14/0x28
...
---[ end trace 0000000000000000 ]---

Fix this by switching the driver to the new API.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 .../drm/bridge/analogix/analogix_dp_core.c    | 40 +++++--------------
 .../drm/bridge/analogix/analogix_dp_core.h    |  3 +-
 2 files changed, 13 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index a1bc3e96dd35..ed35e567d117 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1041,7 +1041,7 @@ static int analogix_dp_bridge_attach(struct drm_bridge *bridge,
 				     struct drm_encoder *encoder,
 				     enum drm_bridge_attach_flags flags)
 {
-	struct analogix_dp_device *dp = bridge->driver_private;
+	struct analogix_dp_device *dp = to_dp(bridge);
 	struct drm_connector *connector = NULL;
 	int ret = 0;
 
@@ -1125,7 +1125,7 @@ struct drm_crtc *analogix_dp_get_new_crtc(struct analogix_dp_device *dp,
 static void analogix_dp_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 						 struct drm_atomic_state *old_state)
 {
-	struct analogix_dp_device *dp = bridge->driver_private;
+	struct analogix_dp_device *dp = to_dp(bridge);
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
 
@@ -1180,7 +1180,7 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
 static void analogix_dp_bridge_atomic_enable(struct drm_bridge *bridge,
 					     struct drm_atomic_state *old_state)
 {
-	struct analogix_dp_device *dp = bridge->driver_private;
+	struct analogix_dp_device *dp = to_dp(bridge);
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
 	int timeout_loop = 0;
@@ -1217,7 +1217,7 @@ static void analogix_dp_bridge_atomic_enable(struct drm_bridge *bridge,
 
 static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
 {
-	struct analogix_dp_device *dp = bridge->driver_private;
+	struct analogix_dp_device *dp = to_dp(bridge);
 
 	if (dp->dpms_mode != DRM_MODE_DPMS_ON)
 		return;
@@ -1240,7 +1240,7 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
 static void analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge,
 					      struct drm_atomic_state *old_state)
 {
-	struct analogix_dp_device *dp = bridge->driver_private;
+	struct analogix_dp_device *dp = to_dp(bridge);
 	struct drm_crtc *old_crtc, *new_crtc;
 	struct drm_crtc_state *old_crtc_state = NULL;
 	struct drm_crtc_state *new_crtc_state = NULL;
@@ -1278,7 +1278,7 @@ static void analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge,
 static void analogix_dp_bridge_atomic_post_disable(struct drm_bridge *bridge,
 						   struct drm_atomic_state *old_state)
 {
-	struct analogix_dp_device *dp = bridge->driver_private;
+	struct analogix_dp_device *dp = to_dp(bridge);
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *new_crtc_state;
 	int ret;
@@ -1300,7 +1300,7 @@ static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
 				const struct drm_display_mode *orig_mode,
 				const struct drm_display_mode *mode)
 {
-	struct analogix_dp_device *dp = bridge->driver_private;
+	struct analogix_dp_device *dp = to_dp(bridge);
 	struct drm_display_info *display_info = &dp->connector.display_info;
 	struct video_info *video = &dp->video_info;
 	struct device_node *dp_node = dp->dev->of_node;
@@ -1385,25 +1385,6 @@ static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
 	.attach = analogix_dp_bridge_attach,
 };
 
-static int analogix_dp_create_bridge(struct drm_device *drm_dev,
-				     struct analogix_dp_device *dp)
-{
-	struct drm_bridge *bridge;
-
-	bridge = devm_kzalloc(drm_dev->dev, sizeof(*bridge), GFP_KERNEL);
-	if (!bridge) {
-		DRM_ERROR("failed to allocate for drm bridge\n");
-		return -ENOMEM;
-	}
-
-	dp->bridge = bridge;
-
-	bridge->driver_private = dp;
-	bridge->funcs = &analogix_dp_bridge_funcs;
-
-	return drm_bridge_attach(dp->encoder, bridge, NULL, 0);
-}
-
 static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp)
 {
 	struct device_node *dp_node = dp->dev->of_node;
@@ -1491,7 +1472,8 @@ analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data)
 		return ERR_PTR(-EINVAL);
 	}
 
-	dp = devm_kzalloc(dev, sizeof(struct analogix_dp_device), GFP_KERNEL);
+	dp = devm_drm_bridge_alloc(dev, struct analogix_dp_device, bridge,
+				   &analogix_dp_bridge_funcs);
 	if (!dp)
 		return ERR_PTR(-ENOMEM);
 
@@ -1643,7 +1625,7 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
 		return ret;
 	}
 
-	ret = analogix_dp_create_bridge(drm_dev, dp);
+	ret = drm_bridge_attach(dp->encoder, &dp->bridge, NULL, 0);
 	if (ret) {
 		DRM_ERROR("failed to create bridge (%d)\n", ret);
 		goto err_unregister_aux;
@@ -1660,7 +1642,7 @@ EXPORT_SYMBOL_GPL(analogix_dp_bind);
 
 void analogix_dp_unbind(struct analogix_dp_device *dp)
 {
-	analogix_dp_bridge_disable(dp->bridge);
+	analogix_dp_bridge_disable(&dp->bridge);
 	dp->connector.funcs->destroy(&dp->connector);
 
 	drm_panel_unprepare(dp->plat_data->panel);
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index 2b54120ba4a3..b86e93f30ed6 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -11,6 +11,7 @@
 
 #include <drm/display/drm_dp_helper.h>
 #include <drm/drm_crtc.h>
+#include <drm/drm_bridge.h>
 
 #define DP_TIMEOUT_LOOP_COUNT 100
 #define MAX_CR_LOOP 5
@@ -154,7 +155,7 @@ struct analogix_dp_device {
 	struct device		*dev;
 	struct drm_device	*drm_dev;
 	struct drm_connector	connector;
-	struct drm_bridge	*bridge;
+	struct drm_bridge	bridge;
 	struct drm_dp_aux       aux;
 	struct clk		*clock;
 	unsigned int		irq;
-- 
2.34.1

Re: [PATCH] drm/bridge: analogix_dp: Use devm_drm_bridge_alloc() API

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1562 bytes --]

Hi,

On Fri, Jun 27, 2025 at 06:56:52PM +0200, Marek Szyprowski wrote:
> -static int analogix_dp_create_bridge(struct drm_device *drm_dev,
> -				     struct analogix_dp_device *dp)
> -{
> -	struct drm_bridge *bridge;
> -
> -	bridge = devm_kzalloc(drm_dev->dev, sizeof(*bridge), GFP_KERNEL);
> -	if (!bridge) {
> -		DRM_ERROR("failed to allocate for drm bridge\n");
> -		return -ENOMEM;
> -	}
> -
> -	dp->bridge = bridge;
> -
> -	bridge->driver_private = dp;
> -	bridge->funcs = &analogix_dp_bridge_funcs;
> -
> -	return drm_bridge_attach(dp->encoder, bridge, NULL, 0);
> -}
> -
>  static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp)
>  {
>  	struct device_node *dp_node = dp->dev->of_node;
> @@ -1491,7 +1472,8 @@ analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data)
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> -	dp = devm_kzalloc(dev, sizeof(struct analogix_dp_device), GFP_KERNEL);
> +	dp = devm_drm_bridge_alloc(dev, struct analogix_dp_device, bridge,
> +				   &analogix_dp_bridge_funcs);
>  	if (!dp)
>  		return ERR_PTR(-ENOMEM);
>  
> @@ -1643,7 +1625,7 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
>  		return ret;
>  	}
>  
> -	ret = analogix_dp_create_bridge(drm_dev, dp);
> +	ret = drm_bridge_attach(dp->encoder, &dp->bridge, NULL, 0);
>  	if (ret) {
>  		DRM_ERROR("failed to create bridge (%d)\n", ret);
>  		goto err_unregister_aux;

It looks like you don't set bridge->driver_private anymore. Is it on purpose?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH] drm/bridge: analogix_dp: Use devm_drm_bridge_alloc() API

[Luca Ceresoli]

Hello Marek, Maxime,

thanks Marek for spotting the issue and sending a patch!

On Mon, 30 Jun 2025 18:44:24 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> > @@ -1643,7 +1625,7 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
> >  		return ret;
> >  	}
> >  
> > -	ret = analogix_dp_create_bridge(drm_dev, dp);
> > +	ret = drm_bridge_attach(dp->encoder, &dp->bridge, NULL, 0);
> >  	if (ret) {
> >  		DRM_ERROR("failed to create bridge (%d)\n", ret);
> >  		goto err_unregister_aux;  
> 
> It looks like you don't set bridge->driver_private anymore. Is it on purpose?

This looks correct to me. In current code, driver_private is used to
hold a pointer to the driver private struct (struct
analogix_dp_device). With devm_drm_bridge_alloc() container_of() is now
enough, no pointer is needed. With the patch applied, driver_private
becomes unused.

Thus I think the patch is correct:

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

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

Re: [PATCH] drm/bridge: analogix_dp: Use devm_drm_bridge_alloc() API

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

On Tue, Jul 01, 2025 at 04:02:19PM +0200, Luca Ceresoli wrote:
> Hello Marek, Maxime,
> 
> thanks Marek for spotting the issue and sending a patch!
> 
> On Mon, 30 Jun 2025 18:44:24 +0200
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > > @@ -1643,7 +1625,7 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
> > >  		return ret;
> > >  	}
> > >  
> > > -	ret = analogix_dp_create_bridge(drm_dev, dp);
> > > +	ret = drm_bridge_attach(dp->encoder, &dp->bridge, NULL, 0);
> > >  	if (ret) {
> > >  		DRM_ERROR("failed to create bridge (%d)\n", ret);
> > >  		goto err_unregister_aux;  
> > 
> > It looks like you don't set bridge->driver_private anymore. Is it on purpose?
> 
> This looks correct to me. In current code, driver_private is used to
> hold a pointer to the driver private struct (struct
> analogix_dp_device). With devm_drm_bridge_alloc() container_of() is now
> enough, no pointer is needed. With the patch applied, driver_private
> becomes unused.

Then we should remove it from the structure if it's unused.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH] drm/bridge: analogix_dp: Use devm_drm_bridge_alloc() API

[Luca Ceresoli]

Hi Maxime,

On Tue, 1 Jul 2025 16:27:54 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> On Tue, Jul 01, 2025 at 04:02:19PM +0200, Luca Ceresoli wrote:
> > Hello Marek, Maxime,
> > 
> > thanks Marek for spotting the issue and sending a patch!
> > 
> > On Mon, 30 Jun 2025 18:44:24 +0200
> > Maxime Ripard <mripard@kernel.org> wrote:
> >   
> > > > @@ -1643,7 +1625,7 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
> > > >  		return ret;
> > > >  	}
> > > >  
> > > > -	ret = analogix_dp_create_bridge(drm_dev, dp);
> > > > +	ret = drm_bridge_attach(dp->encoder, &dp->bridge, NULL, 0);
> > > >  	if (ret) {
> > > >  		DRM_ERROR("failed to create bridge (%d)\n", ret);
> > > >  		goto err_unregister_aux;    
> > > 
> > > It looks like you don't set bridge->driver_private anymore. Is it on purpose?  
> > 
> > This looks correct to me. In current code, driver_private is used to
> > hold a pointer to the driver private struct (struct
> > analogix_dp_device). With devm_drm_bridge_alloc() container_of() is now
> > enough, no pointer is needed. With the patch applied, driver_private
> > becomes unused.  
> 
> Then we should remove it from the structure if it's unused.

Makes sense now that struct drm_bridge is meant to be always embedded
in a driver-private struct. But several drivers are still using it, so
those would need to be updated beforehand:

$ git grep  -l driver_private -- drivers/gpu/drm/ | wc -l
23
$

So I think this patch should be taken as it fixes a regression. Do you
agree on this?

Luca

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

Re: [PATCH] drm/bridge: analogix_dp: Use devm_drm_bridge_alloc() API

[Marek Szyprowski]

On 03.07.2025 17:50, Luca Ceresoli wrote:
> On Tue, 1 Jul 2025 16:27:54 +0200
> Maxime Ripard <mripard@kernel.org> wrote:
>
>> On Tue, Jul 01, 2025 at 04:02:19PM +0200, Luca Ceresoli wrote:
>>> Hello Marek, Maxime,
>>>
>>> thanks Marek for spotting the issue and sending a patch!
>>>
>>> On Mon, 30 Jun 2025 18:44:24 +0200
>>> Maxime Ripard <mripard@kernel.org> wrote:
>>>    
>>>>> @@ -1643,7 +1625,7 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
>>>>>   		return ret;
>>>>>   	}
>>>>>   
>>>>> -	ret = analogix_dp_create_bridge(drm_dev, dp);
>>>>> +	ret = drm_bridge_attach(dp->encoder, &dp->bridge, NULL, 0);
>>>>>   	if (ret) {
>>>>>   		DRM_ERROR("failed to create bridge (%d)\n", ret);
>>>>>   		goto err_unregister_aux;
>>>> It looks like you don't set bridge->driver_private anymore. Is it on purpose?
>>> This looks correct to me. In current code, driver_private is used to
>>> hold a pointer to the driver private struct (struct
>>> analogix_dp_device). With devm_drm_bridge_alloc() container_of() is now
>>> enough, no pointer is needed. With the patch applied, driver_private
>>> becomes unused.
>> Then we should remove it from the structure if it's unused.
> Makes sense now that struct drm_bridge is meant to be always embedded
> in a driver-private struct. But several drivers are still using it, so
> those would need to be updated beforehand:
>
> $ git grep  -l driver_private -- drivers/gpu/drm/ | wc -l
> 23
> $
>
> So I think this patch should be taken as it fixes a regression. Do you
> agree on this?

Yes, please apply it as a fix :)


BTW, there are 2 more bridge drivers that need a fix similar to the 
$subject patch:

$ git grep "bridge = devm_kzalloc" drivers/gpu
drivers/gpu/drm/sti/sti_hda.c:  bridge = devm_kzalloc(dev, 
sizeof(*bridge), GFP_KERNEL);
drivers/gpu/drm/sti/sti_hdmi.c: bridge = devm_kzalloc(dev, 
sizeof(*bridge), GFP_KERNEL);

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] drm/bridge: analogix_dp: Use devm_drm_bridge_alloc() API

[Luca Ceresoli]

On Mon, 7 Jul 2025 11:07:26 +0200
Marek Szyprowski <m.szyprowski@samsung.com> wrote:

> On 03.07.2025 17:50, Luca Ceresoli wrote:
> > On Tue, 1 Jul 2025 16:27:54 +0200
> > Maxime Ripard <mripard@kernel.org> wrote:
> >  
> >> On Tue, Jul 01, 2025 at 04:02:19PM +0200, Luca Ceresoli wrote:  
> >>> Hello Marek, Maxime,
> >>>
> >>> thanks Marek for spotting the issue and sending a patch!
> >>>
> >>> On Mon, 30 Jun 2025 18:44:24 +0200
> >>> Maxime Ripard <mripard@kernel.org> wrote:
> >>>      
> >>>>> @@ -1643,7 +1625,7 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
> >>>>>   		return ret;
> >>>>>   	}
> >>>>>   
> >>>>> -	ret = analogix_dp_create_bridge(drm_dev, dp);
> >>>>> +	ret = drm_bridge_attach(dp->encoder, &dp->bridge, NULL, 0);
> >>>>>   	if (ret) {
> >>>>>   		DRM_ERROR("failed to create bridge (%d)\n", ret);
> >>>>>   		goto err_unregister_aux;  
> >>>> It looks like you don't set bridge->driver_private anymore. Is it on purpose?  
> >>> This looks correct to me. In current code, driver_private is used to
> >>> hold a pointer to the driver private struct (struct
> >>> analogix_dp_device). With devm_drm_bridge_alloc() container_of() is now
> >>> enough, no pointer is needed. With the patch applied, driver_private
> >>> becomes unused.  
> >> Then we should remove it from the structure if it's unused.  
> > Makes sense now that struct drm_bridge is meant to be always embedded
> > in a driver-private struct. But several drivers are still using it, so
> > those would need to be updated beforehand:
> >
> > $ git grep  -l driver_private -- drivers/gpu/drm/ | wc -l
> > 23
> > $
> >
> > So I think this patch should be taken as it fixes a regression. Do you
> > agree on this?  
> 
> Yes, please apply it as a fix :)
> 
> 
> BTW, there are 2 more bridge drivers that need a fix similar to the 
> $subject patch:
> 
> $ git grep "bridge = devm_kzalloc" drivers/gpu
> drivers/gpu/drm/sti/sti_hda.c:  bridge = devm_kzalloc(dev, 
> sizeof(*bridge), GFP_KERNEL);
> drivers/gpu/drm/sti/sti_hdmi.c: bridge = devm_kzalloc(dev, 
> sizeof(*bridge), GFP_KERNEL);

Ouch. My grep logic was probably too clever and missed these obvious
ones. I'm taking care of converting these ones later this week as time
permits, unless patches are sent before.

Thanks for reporting!

Luca

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

Re: [PATCH] drm/bridge: analogix_dp: Use devm_drm_bridge_alloc() API

[Luca Ceresoli]

Hello Maxime, all,

On Mon, 7 Jul 2025 12:59:54 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> On Mon, 7 Jul 2025 11:07:26 +0200
> Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> 
> > On 03.07.2025 17:50, Luca Ceresoli wrote:  
> > > On Tue, 1 Jul 2025 16:27:54 +0200
> > > Maxime Ripard <mripard@kernel.org> wrote:
> > >    
> > >> On Tue, Jul 01, 2025 at 04:02:19PM +0200, Luca Ceresoli wrote:    
> > >>> Hello Marek, Maxime,
> > >>>
> > >>> thanks Marek for spotting the issue and sending a patch!
> > >>>
> > >>> On Mon, 30 Jun 2025 18:44:24 +0200
> > >>> Maxime Ripard <mripard@kernel.org> wrote:
> > >>>        
> > >>>>> @@ -1643,7 +1625,7 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
> > >>>>>   		return ret;
> > >>>>>   	}
> > >>>>>   
> > >>>>> -	ret = analogix_dp_create_bridge(drm_dev, dp);
> > >>>>> +	ret = drm_bridge_attach(dp->encoder, &dp->bridge, NULL, 0);
> > >>>>>   	if (ret) {
> > >>>>>   		DRM_ERROR("failed to create bridge (%d)\n", ret);
> > >>>>>   		goto err_unregister_aux;    
> > >>>> It looks like you don't set bridge->driver_private anymore. Is it on purpose?    
> > >>> This looks correct to me. In current code, driver_private is used to
> > >>> hold a pointer to the driver private struct (struct
> > >>> analogix_dp_device). With devm_drm_bridge_alloc() container_of() is now
> > >>> enough, no pointer is needed. With the patch applied, driver_private
> > >>> becomes unused.    
> > >> Then we should remove it from the structure if it's unused.    
> > > Makes sense now that struct drm_bridge is meant to be always embedded
> > > in a driver-private struct. But several drivers are still using it, so
> > > those would need to be updated beforehand:
> > >
> > > $ git grep  -l driver_private -- drivers/gpu/drm/ | wc -l
> > > 23
> > > $
> > >
> > > So I think this patch should be taken as it fixes a regression. Do you
> > > agree on this?    
> > 
> > Yes, please apply it as a fix :)
> > 
> > 
> > BTW, there are 2 more bridge drivers that need a fix similar to the 
> > $subject patch:
> > 
> > $ git grep "bridge = devm_kzalloc" drivers/gpu
> > drivers/gpu/drm/sti/sti_hda.c:  bridge = devm_kzalloc(dev, 
> > sizeof(*bridge), GFP_KERNEL);
> > drivers/gpu/drm/sti/sti_hdmi.c: bridge = devm_kzalloc(dev, 
> > sizeof(*bridge), GFP_KERNEL);  
> 
> Ouch. My grep logic was probably too clever and missed these obvious
> ones. I'm taking care of converting these ones later this week as time
> permits, unless patches are sent before.

I think I missed those two because I searched for all calls to
drm_bridge_add(), and converted matching drivers, but these two do now
call drm_bridge_add() at all.

I understand this probably works just fine, for non-DT hardware at
least. However, doesn't this look like wrong, from a DRM bridge API
point of view?

Right now I'm preparing patches to convert to devm_drm_bridge_alloc(),
but what about the following two additions:

 * add calls to drm_bridge_add/remove() in those drivers
 * add a WARN in drm_bridge_attach() if the bridge was not added

?

Luca

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

Re: [PATCH] drm/bridge: analogix_dp: Use devm_drm_bridge_alloc() API

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3752 bytes --]

Hi,

On Tue, Jul 08, 2025 at 04:19:15PM +0200, Luca Ceresoli wrote:
> Hello Maxime, all,
> 
> On Mon, 7 Jul 2025 12:59:54 +0200
> Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> 
> > On Mon, 7 Jul 2025 11:07:26 +0200
> > Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> > 
> > > On 03.07.2025 17:50, Luca Ceresoli wrote:  
> > > > On Tue, 1 Jul 2025 16:27:54 +0200
> > > > Maxime Ripard <mripard@kernel.org> wrote:
> > > >    
> > > >> On Tue, Jul 01, 2025 at 04:02:19PM +0200, Luca Ceresoli wrote:    
> > > >>> Hello Marek, Maxime,
> > > >>>
> > > >>> thanks Marek for spotting the issue and sending a patch!
> > > >>>
> > > >>> On Mon, 30 Jun 2025 18:44:24 +0200
> > > >>> Maxime Ripard <mripard@kernel.org> wrote:
> > > >>>        
> > > >>>>> @@ -1643,7 +1625,7 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
> > > >>>>>   		return ret;
> > > >>>>>   	}
> > > >>>>>   
> > > >>>>> -	ret = analogix_dp_create_bridge(drm_dev, dp);
> > > >>>>> +	ret = drm_bridge_attach(dp->encoder, &dp->bridge, NULL, 0);
> > > >>>>>   	if (ret) {
> > > >>>>>   		DRM_ERROR("failed to create bridge (%d)\n", ret);
> > > >>>>>   		goto err_unregister_aux;    
> > > >>>> It looks like you don't set bridge->driver_private anymore. Is it on purpose?    
> > > >>> This looks correct to me. In current code, driver_private is used to
> > > >>> hold a pointer to the driver private struct (struct
> > > >>> analogix_dp_device). With devm_drm_bridge_alloc() container_of() is now
> > > >>> enough, no pointer is needed. With the patch applied, driver_private
> > > >>> becomes unused.    
> > > >> Then we should remove it from the structure if it's unused.    
> > > > Makes sense now that struct drm_bridge is meant to be always embedded
> > > > in a driver-private struct. But several drivers are still using it, so
> > > > those would need to be updated beforehand:
> > > >
> > > > $ git grep  -l driver_private -- drivers/gpu/drm/ | wc -l
> > > > 23
> > > > $
> > > >
> > > > So I think this patch should be taken as it fixes a regression. Do you
> > > > agree on this?    
> > > 
> > > Yes, please apply it as a fix :)
> > > 
> > > 
> > > BTW, there are 2 more bridge drivers that need a fix similar to the 
> > > $subject patch:
> > > 
> > > $ git grep "bridge = devm_kzalloc" drivers/gpu
> > > drivers/gpu/drm/sti/sti_hda.c:  bridge = devm_kzalloc(dev, 
> > > sizeof(*bridge), GFP_KERNEL);
> > > drivers/gpu/drm/sti/sti_hdmi.c: bridge = devm_kzalloc(dev, 
> > > sizeof(*bridge), GFP_KERNEL);  
> > 
> > Ouch. My grep logic was probably too clever and missed these obvious
> > ones. I'm taking care of converting these ones later this week as time
> > permits, unless patches are sent before.
> 
> I think I missed those two because I searched for all calls to
> drm_bridge_add(), and converted matching drivers, but these two do now
> call drm_bridge_add() at all.
> 
> I understand this probably works just fine, for non-DT hardware at
> least. However, doesn't this look like wrong, from a DRM bridge API
> point of view?
> 
> Right now I'm preparing patches to convert to devm_drm_bridge_alloc(),
> but what about the following two additions:
> 
>  * add calls to drm_bridge_add/remove() in those drivers

Yep, we definitely need to do that

>  * add a WARN in drm_bridge_attach() if the bridge was not added

We also need to update the documentation to make it more obvious,
because at the moment it's not documented that you need to add a bridge
before attaching it.

And for displaying the warning, I guess we could but I'd like to see the
implementation. If it's anything but trivial, I don't think it's worth it.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH] drm/bridge: analogix_dp: Use devm_drm_bridge_alloc() API

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1885 bytes --]

On Thu, Jul 03, 2025 at 05:50:32PM +0200, Luca Ceresoli wrote:
> Hi Maxime,
> 
> On Tue, 1 Jul 2025 16:27:54 +0200
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > On Tue, Jul 01, 2025 at 04:02:19PM +0200, Luca Ceresoli wrote:
> > > Hello Marek, Maxime,
> > > 
> > > thanks Marek for spotting the issue and sending a patch!
> > > 
> > > On Mon, 30 Jun 2025 18:44:24 +0200
> > > Maxime Ripard <mripard@kernel.org> wrote:
> > >   
> > > > > @@ -1643,7 +1625,7 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
> > > > >  		return ret;
> > > > >  	}
> > > > >  
> > > > > -	ret = analogix_dp_create_bridge(drm_dev, dp);
> > > > > +	ret = drm_bridge_attach(dp->encoder, &dp->bridge, NULL, 0);
> > > > >  	if (ret) {
> > > > >  		DRM_ERROR("failed to create bridge (%d)\n", ret);
> > > > >  		goto err_unregister_aux;    
> > > > 
> > > > It looks like you don't set bridge->driver_private anymore. Is it on purpose?  
> > > 
> > > This looks correct to me. In current code, driver_private is used to
> > > hold a pointer to the driver private struct (struct
> > > analogix_dp_device). With devm_drm_bridge_alloc() container_of() is now
> > > enough, no pointer is needed. With the patch applied, driver_private
> > > becomes unused.  
> > 
> > Then we should remove it from the structure if it's unused.
> 
> Makes sense now that struct drm_bridge is meant to be always embedded
> in a driver-private struct. But several drivers are still using it, so
> those would need to be updated beforehand:
> 
> $ git grep  -l driver_private -- drivers/gpu/drm/ | wc -l
> 23
> $

Ah, you're right, sorry for the noise.

> So I think this patch should be taken as it fixes a regression. Do you
> agree on this?

As far as I know, that commit only exists in drm-misc-next. Also, it
should have a Fixes tag.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH] drm/bridge: analogix_dp: Use devm_drm_bridge_alloc() API

[Marek Szyprowski]

On 07.07.2025 11:21, Maxime Ripard wrote:
> On Thu, Jul 03, 2025 at 05:50:32PM +0200, Luca Ceresoli wrote:
>> On Tue, 1 Jul 2025 16:27:54 +0200
>> Maxime Ripard <mripard@kernel.org> wrote:
>>
>>> On Tue, Jul 01, 2025 at 04:02:19PM +0200, Luca Ceresoli wrote:
>>>> Hello Marek, Maxime,
>>>>
>>>> thanks Marek for spotting the issue and sending a patch!
>>>>
>>>> On Mon, 30 Jun 2025 18:44:24 +0200
>>>> Maxime Ripard <mripard@kernel.org> wrote:
>>>>    
>>>>>> @@ -1643,7 +1625,7 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
>>>>>>   		return ret;
>>>>>>   	}
>>>>>>   
>>>>>> -	ret = analogix_dp_create_bridge(drm_dev, dp);
>>>>>> +	ret = drm_bridge_attach(dp->encoder, &dp->bridge, NULL, 0);
>>>>>>   	if (ret) {
>>>>>>   		DRM_ERROR("failed to create bridge (%d)\n", ret);
>>>>>>   		goto err_unregister_aux;
>>>>> It looks like you don't set bridge->driver_private anymore. Is it on purpose?
>>>> This looks correct to me. In current code, driver_private is used to
>>>> hold a pointer to the driver private struct (struct
>>>> analogix_dp_device). With devm_drm_bridge_alloc() container_of() is now
>>>> enough, no pointer is needed. With the patch applied, driver_private
>>>> becomes unused.
>>> Then we should remove it from the structure if it's unused.
>> Makes sense now that struct drm_bridge is meant to be always embedded
>> in a driver-private struct. But several drivers are still using it, so
>> those would need to be updated beforehand:
>>
>> $ git grep  -l driver_private -- drivers/gpu/drm/ | wc -l
>> 23
>> $
> Ah, you're right, sorry for the noise.
>
>> So I think this patch should be taken as it fixes a regression. Do you
>> agree on this?
> As far as I know, that commit only exists in drm-misc-next. Also, it
> should have a Fixes tag.

I wasn't sure which commit should be listed as Fixes tag in this case. 
The mentioned 9c399719cfb9 ("drm: convert many bridge drivers from 
devm_kzalloc() to devm_drm_bridge_alloc() API")?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] drm/bridge: analogix_dp: Use devm_drm_bridge_alloc() API

[Luca Ceresoli]

Hi Marek, Maxime,

On Mon, 7 Jul 2025 12:12:51 +0200
Marek Szyprowski <m.szyprowski@samsung.com> wrote:

> On 07.07.2025 11:21, Maxime Ripard wrote:
> > On Thu, Jul 03, 2025 at 05:50:32PM +0200, Luca Ceresoli wrote:  
> >> On Tue, 1 Jul 2025 16:27:54 +0200
> >> Maxime Ripard <mripard@kernel.org> wrote:
> >>  
> >>> On Tue, Jul 01, 2025 at 04:02:19PM +0200, Luca Ceresoli wrote:  
> >>>> Hello Marek, Maxime,
> >>>>
> >>>> thanks Marek for spotting the issue and sending a patch!
> >>>>
> >>>> On Mon, 30 Jun 2025 18:44:24 +0200
> >>>> Maxime Ripard <mripard@kernel.org> wrote:
> >>>>      
> >>>>>> @@ -1643,7 +1625,7 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
> >>>>>>   		return ret;
> >>>>>>   	}
> >>>>>>   
> >>>>>> -	ret = analogix_dp_create_bridge(drm_dev, dp);
> >>>>>> +	ret = drm_bridge_attach(dp->encoder, &dp->bridge, NULL, 0);
> >>>>>>   	if (ret) {
> >>>>>>   		DRM_ERROR("failed to create bridge (%d)\n", ret);
> >>>>>>   		goto err_unregister_aux;  
> >>>>> It looks like you don't set bridge->driver_private anymore. Is it on purpose?  
> >>>> This looks correct to me. In current code, driver_private is used to
> >>>> hold a pointer to the driver private struct (struct
> >>>> analogix_dp_device). With devm_drm_bridge_alloc() container_of() is now
> >>>> enough, no pointer is needed. With the patch applied, driver_private
> >>>> becomes unused.  
> >>> Then we should remove it from the structure if it's unused.  
> >> Makes sense now that struct drm_bridge is meant to be always embedded
> >> in a driver-private struct. But several drivers are still using it, so
> >> those would need to be updated beforehand:
> >>
> >> $ git grep  -l driver_private -- drivers/gpu/drm/ | wc -l
> >> 23
> >> $  
> > Ah, you're right, sorry for the noise.
> >  
> >> So I think this patch should be taken as it fixes a regression. Do you
> >> agree on this?  
> > As far as I know, that commit only exists in drm-misc-next. Also, it
> > should have a Fixes tag.  
> 
> I wasn't sure which commit should be listed as Fixes tag in this case. 
> The mentioned 9c399719cfb9 ("drm: convert many bridge drivers from 
> devm_kzalloc() to devm_drm_bridge_alloc() API")?

Despite being somewhat orthogonal, I think it should be commit
a7748dd127ea ("drm/bridge: get/put the bridge reference in
drm_bridge_add/remove()") just because it is the very first commit in
the drm-misc-next history introducing a get/put pair, and thus
triggering the refcount warning.

I'm applying with that added.

Thanks both for the discussion.

Luca

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

Re: [PATCH] drm/bridge: analogix_dp: Use devm_drm_bridge_alloc() API

[Luca Ceresoli]

On Fri, 27 Jun 2025 18:56:52 +0200, Marek Szyprowski wrote:
> devm_drm_bridge_alloc() is the new API to be used for allocating
> (and partially initializing) a private driver struct embedding
> a struct drm_bridge.
> 
> Analogix DP driver somehow missed the automated conversion in commit
> 9c399719cfb9 ("drm: convert many bridge drivers from devm_kzalloc() to
> devm_drm_bridge_alloc() API"), what causes the following warning:
> 
> [...]

Applied, thanks!

[1/1] drm/bridge: analogix_dp: Use devm_drm_bridge_alloc() API
      commit: 48f05c3b4b701ae7687fd44d462c88b7ac67e952

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>