diff mbox series

[29/34] drm: zynqmp_dp: convert to devm_drm_bridge_alloc() API

Message ID 20250407-drm-bridge-convert-to-alloc-api-v1-29-42113ff8d9c0@bootlin.com (mailing list archive)
State New
Delegated to: Kieran Bingham
Headers show
Series drm: convert all bridges to devm_drm_bridge_alloc() | expand

Commit Message

Luca Ceresoli April 7, 2025, 2:23 p.m. UTC 
This is the new API for allocating DRM bridges.

This driver has a peculiar structure. zynqmp_dpsub.c is the actual driver,
which delegates to a submodule (zynqmp_dp.c) the allocation of a
sub-structure embedding the drm_bridge and its initialization, however it
does not delegate the drm_bridge_add(). Hence, following carefully the code
flow, it is correct to change the allocation function and .funcs assignment
in the submodule, while the drm_bridge_add() is not in that submodule.

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

---

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Michal Simek <michal.simek@amd.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/xlnx/zynqmp_dp.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Tomi Valkeinen April 16, 2025, 12:26 p.m. UTC | #1 
Hi,

On 07/04/2025 17:23, Luca Ceresoli wrote:
> This is the new API for allocating DRM bridges.
> 
> This driver has a peculiar structure. zynqmp_dpsub.c is the actual driver,
> which delegates to a submodule (zynqmp_dp.c) the allocation of a
> sub-structure embedding the drm_bridge and its initialization, however it
> does not delegate the drm_bridge_add(). Hence, following carefully the code
> flow, it is correct to change the allocation function and .funcs assignment
> in the submodule, while the drm_bridge_add() is not in that submodule.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Michal Simek <michal.simek@amd.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>   drivers/gpu/drm/xlnx/zynqmp_dp.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 11d2415fb5a1f7fad03421898331289f2295d68b..de22b6457a78a7a2110f9f308d0b5a8700544010 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -2439,9 +2439,9 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
>   	struct zynqmp_dp *dp;
>   	int ret;
>   
> -	dp = kzalloc(sizeof(*dp), GFP_KERNEL);
> -	if (!dp)
> -		return -ENOMEM;
> +	dp = devm_drm_bridge_alloc(&pdev->dev, struct zynqmp_dp, bridge, &zynqmp_dp_bridge_funcs);
> +	if (IS_ERR(dp))
> +		return PTR_ERR(dp);
>   
>   	dp->dev = &pdev->dev;
>   	dp->dpsub = dpsub;
> @@ -2488,7 +2488,6 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
>   
>   	/* Initialize the bridge. */
>   	bridge = &dp->bridge;
> -	bridge->funcs = &zynqmp_dp_bridge_funcs;
>   	bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID
>   		    | DRM_BRIDGE_OP_HPD;
>   	bridge->type = DRM_MODE_CONNECTOR_DisplayPort;
> 

I haven't had time to look at this more, but jfyi: I got this when 
unloading modules, but it doesn't seem to happen every time:

[  103.010533] ------------[ cut here ]------------
[  103.015415] refcount_t: underflow; use-after-free.
[  103.020657] WARNING: CPU: 2 PID: 392 at lib/refcount.c:28 
refcount_warn_saturate+0xf4/0x148
[  103.029056] Modules linked in: zynqmp_dpsub(-) display_connector 
drm_display_helper drm_dma_helper drm_kms_helper drm drm_p
anel_orientation_quirks
[  103.042437] CPU: 2 UID: 0 PID: 392 Comm: rmmod Not tainted 
6.15.0-rc2+ #3 PREEMPT
[  103.050035] Hardware name: ZynqMP ZCU106 RevA (DT)
[  103.054836] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS 
BTYPE=--)
[  103.061814] pc : refcount_warn_saturate+0xf4/0x148
[  103.066632] lr : refcount_warn_saturate+0xf4/0x148
[  103.071441] sp : ffff800083b5bbb0
[  103.074766] x29: ffff800083b5bbb0 x28: ffff000806b23780 x27: 
0000000000000000
[  103.081953] x26: 0000000000000000 x25: 0000000000000000 x24: 
ffff000801a68400
[  103.089141] x23: ffff800081311a20 x22: ffff800083b5bc38 x21: 
ffff000801a68010
[  103.096329] x20: ffff0008040676c0 x19: ffff000804067240 x18: 
0000000000000006
[  103.103517] x17: 2e30303030303464 x16: 662d7968703a7968 x15: 
ffff800083b5b5a0
[  103.110705] x14: 0000000000000000 x13: 00000000000c0000 x12: 
0000000000000000
[  103.117892] x11: ffff80008163d6bc x10: 0000000000000028 x9 : 
ffff800080ead38c
[  103.125080] x8 : ffff800083b5b908 x7 : 0000000000000000 x6 : 
ffff800083b5b9c0
[  103.132268] x5 : ffff800083b5b948 x4 : 0000000000000001 x3 : 
00000000000000db
[  103.139455] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 
ffff000806b23780
[  103.146644] Call trace:
[  103.149102]  refcount_warn_saturate+0xf4/0x148 (P)
[  103.153918]  drm_bridge_put.part.0+0x88/0xa0 [drm]
[  103.159188]  drm_bridge_put_void+0x1c/0x38 [drm]
[  103.164231]  devm_action_release+0x1c/0x30
[  103.168354]  release_nodes+0x68/0xa8
[  103.171957]  devres_release_all+0x98/0xf0
[  103.175993]  device_unbind_cleanup+0x20/0x70
[  103.180291]  device_release_driver_internal+0x208/0x250
[  103.185542]  driver_detach+0x54/0xa8
[  103.189145]  bus_remove_driver+0x78/0x108
[  103.193181]  driver_unregister+0x38/0x70
[  103.197131]  platform_driver_unregister+0x1c/0x30
[  103.201862]  zynqmp_dpsub_driver_exit+0x18/0x1100 [zynqmp_dpsub]
[  103.207931]  __arm64_sys_delete_module+0x1a8/0x2d0
[  103.212748]  invoke_syscall+0x50/0x120
[  103.216524]  el0_svc_common.constprop.0+0x48/0xf0
[  103.221256]  do_el0_svc+0x24/0x38
[  103.224598]  el0_svc+0x48/0x128
[  103.227766]  el0t_64_sync_handler+0x10c/0x138
[  103.232150]  el0t_64_sync+0x1a4/0x1a8
[  103.235841] irq event stamp: 7936
[  103.239173] hardirqs last  enabled at (7935): [<ffff8000800aaf78>] 
finish_task_switch.isra.0+0xb0/0x2a0
[  103.248600] hardirqs last disabled at (7936): [<ffff800080eaac74>] 
el1_dbg+0x24/0x90
[  103.256369] softirqs last  enabled at (7930): [<ffff800080066f98>] 
handle_softirqs+0x4a0/0x4c0
[  103.265007] softirqs last disabled at (7905): [<ffff800080010224>] 
__do_softirq+0x1c/0x28
[  103.273211] ---[ end trace 0000000000000000 ]---

  Tomi
Tomi Valkeinen April 16, 2025, 12:31 p.m. UTC | #2 
Hi,

On 07/04/2025 17:23, Luca Ceresoli wrote:
> This is the new API for allocating DRM bridges.
> 
> This driver has a peculiar structure. zynqmp_dpsub.c is the actual driver,
> which delegates to a submodule (zynqmp_dp.c) the allocation of a
> sub-structure embedding the drm_bridge and its initialization, however it
> does not delegate the drm_bridge_add(). Hence, following carefully the code
> flow, it is correct to change the allocation function and .funcs assignment
> in the submodule, while the drm_bridge_add() is not in that submodule.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Michal Simek <michal.simek@amd.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>   drivers/gpu/drm/xlnx/zynqmp_dp.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 11d2415fb5a1f7fad03421898331289f2295d68b..de22b6457a78a7a2110f9f308d0b5a8700544010 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -2439,9 +2439,9 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
>   	struct zynqmp_dp *dp;
>   	int ret;
>   
> -	dp = kzalloc(sizeof(*dp), GFP_KERNEL);
> -	if (!dp)
> -		return -ENOMEM;
> +	dp = devm_drm_bridge_alloc(&pdev->dev, struct zynqmp_dp, bridge, &zynqmp_dp_bridge_funcs);
> +	if (IS_ERR(dp))
> +		return PTR_ERR(dp);
>   
>   	dp->dev = &pdev->dev;
>   	dp->dpsub = dpsub;
> @@ -2488,7 +2488,6 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
>   
>   	/* Initialize the bridge. */
>   	bridge = &dp->bridge;
> -	bridge->funcs = &zynqmp_dp_bridge_funcs;
>   	bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID
>   		    | DRM_BRIDGE_OP_HPD;
>   	bridge->type = DRM_MODE_CONNECTOR_DisplayPort;
> 

To add to my last mail, this clearly cannot be right, as it changes 
kzalloc call to devm_* call, without removing the kfree()s...

  Tomi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 11d2415fb5a1f7fad03421898331289f2295d68b..de22b6457a78a7a2110f9f308d0b5a8700544010 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -2439,9 +2439,9 @@  int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
 	struct zynqmp_dp *dp;
 	int ret;
 
-	dp = kzalloc(sizeof(*dp), GFP_KERNEL);
-	if (!dp)
-		return -ENOMEM;
+	dp = devm_drm_bridge_alloc(&pdev->dev, struct zynqmp_dp, bridge, &zynqmp_dp_bridge_funcs);
+	if (IS_ERR(dp))
+		return PTR_ERR(dp);
 
 	dp->dev = &pdev->dev;
 	dp->dpsub = dpsub;
@@ -2488,7 +2488,6 @@  int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
 
 	/* Initialize the bridge. */
 	bridge = &dp->bridge;
-	bridge->funcs = &zynqmp_dp_bridge_funcs;
 	bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID
 		    | DRM_BRIDGE_OP_HPD;
 	bridge->type = DRM_MODE_CONNECTOR_DisplayPort;