Re: [PATCH v2 30/34] drm/bridge: imx8qxp-pixel-combiner: convert to devm_drm_bridge_alloc() API

[Liu Ying <victor.liu@nxp.com>]

Hi,

On 04/25/2025, Luca Ceresoli wrote:
> This is the new API for allocating DRM bridges.
> 
> This driver embeds an array of channels in the main struct, and each
> channel embeds a drm_bridge. This prevents dynamic, refcount-based
> deallocation of the bridges.
> 
> To make the new, dynamic bridge allocation possible:
> 
>  * change the array of channels into an array of channel pointers
>  * allocate each channel using devm_drm_bridge_alloc()
>  * adapt the code wherever using the channels
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> Cc: Liu Ying <victor.liu@nxp.com>
> ---
>  drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> index 1f6fd488e7039e943351006d3373009f0c15cb08..40a8a5a53a781137e722309ff91692cf90d881da 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> @@ -68,7 +68,7 @@ struct imx8qxp_pc_channel {
>  
>  struct imx8qxp_pc {
>  	struct device *dev;
> -	struct imx8qxp_pc_channel ch[2];
> +	struct imx8qxp_pc_channel *ch[2];
>  	struct clk *clk_apb;
>  	void __iomem *base;
>  };
> @@ -307,7 +307,14 @@ static int imx8qxp_pc_bridge_probe(struct platform_device *pdev)
>  			goto free_child;
>  		}
>  
> -		ch = &pc->ch[i];
> +		ch = devm_drm_bridge_alloc(dev, struct imx8qxp_pc_channel, bridge,
> +					   &imx8qxp_pc_bridge_funcs);
> +		if (IS_ERR(ch)) {
> +			ret = PTR_ERR(ch);
> +			goto free_child;
> +		}
> +
> +		pc->ch[i] = ch;
>  		ch->pc = pc;
>  		ch->stream_id = i;
>  
> @@ -333,7 +340,6 @@ static int imx8qxp_pc_bridge_probe(struct platform_device *pdev)
>  		of_node_put(remote);
>  
>  		ch->bridge.driver_private = ch;
> -		ch->bridge.funcs = &imx8qxp_pc_bridge_funcs;
>  		ch->bridge.of_node = child;
>  		ch->is_available = true;
>  
> @@ -345,8 +351,8 @@ static int imx8qxp_pc_bridge_probe(struct platform_device *pdev)
>  free_child:
>  	of_node_put(child);
>  
> -	if (i == 1 && pc->ch[0].next_bridge)
> -		drm_bridge_remove(&pc->ch[0].bridge);
> +	if (i == 1 && pc->ch[0]->next_bridge)

Since this patch makes pc->ch[0] and pc->ch[1] be allocated separately,
pc->ch[0] could be NULL if channel0 is not available, hence a NULL pointer
dereference here...

> +		drm_bridge_remove(&pc->ch[0]->bridge);
>  
>  	pm_runtime_disable(dev);
>  	return ret;
> @@ -359,7 +365,7 @@ static void imx8qxp_pc_bridge_remove(struct platform_device *pdev)
>  	int i;
>  
>  	for (i = 0; i < 2; i++) {
> -		ch = &pc->ch[i];
> +		ch = pc->ch[i];
>  
>  		if (!ch->is_available)

...and here too.

This is what I get when removing the imx8qxp_pixel_combiner module.
-8<-
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000144
Mem abort info:
  ESR = 0x0000000096000004
  EC = 0x25: DABT (current EL), IL = 32 bits
  SET = 0, FnV = 0
  EA = 0, S1PTW = 0
  FSC = 0x04: level 0 translation fault
Data abort info:
  ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
  CM = 0, WnR = 0, TnD = 0, TagAccess = 0
  GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
user pgtable: 4k pages, 48-bit VAs, pgdp=000000089e613000
[0000000000000144] pgd=0000000000000000, p4d=0000000000000000
Internal error: Oops: 0000000096000004 [#1]  SMP
Modules linked in: mpl3115 isl29018 industrialio_triggered_buffer kfifo_buf cdns3 cdns_usb_common snd_soc_imx_audmix rtc_imx_sc imx_sc_wdt imx_sc_thermal imx_sc_key imx8qxp_pixel_link6
CPU: 1 UID: 0 PID: 528 Comm: modprobe Not tainted 6.15.0-rc3-next-20250424-00059-gee51752c256e #217 PREEMPT 
Hardware name: Freescale i.MX8QXP MEK (DT)
pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : imx8qxp_pc_bridge_remove+0x38/0x6c [imx8qxp_pixel_combiner]
lr : imx8qxp_pc_bridge_remove+0x30/0x6c [imx8qxp_pixel_combiner]
sp : ffff8000840f3c40
x29: ffff8000840f3c40 x28: ffff00081e75d780 x27: 0000000000000000
x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000
x23: ffff0008100fc090 x22: ffff0008100fe490 x21: ffff00081e69de00
x20: 0000000000000000 x19: ffff0008100fe410 x18: 0000000000000000
x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
x14: 0000000000000013 x13: ffff000810049010 x12: 0000000000000000
x11: ffff0008182bc550 x10: ffff0008182bc490 x9 : ffff000810049010
x8 : 0101010101010101 x7 : 7f7f7f7f7f7f7f7f x6 : fefefeff6364626d
x5 : 8080808000000000 x4 : 0000000000000000 x3 : 0000000000000000
x2 : ffff00081e75d780 x1 : ffff00081e75d780 x0 : ffff80007af9bdc0
Call trace:
 imx8qxp_pc_bridge_remove+0x38/0x6c [imx8qxp_pixel_combiner] (P)
 platform_remove+0x28/0x44
 device_remove+0x4c/0x80
 device_release_driver_internal+0x1c8/0x224
 driver_detach+0x50/0x98
 bus_remove_driver+0x6c/0xbc
 driver_unregister+0x30/0x60
 platform_driver_unregister+0x14/0x20
 imx8qxp_pc_bridge_driver_exit+0x18/0x814 [imx8qxp_pixel_combiner]
 __arm64_sys_delete_module+0x184/0x264
 invoke_syscall+0x48/0x110
 el0_svc_common.constprop.0+0xc8/0xe8
 do_el0_svc+0x20/0x2c
 el0_svc+0x30/0xd0
 el0t_64_sync_handler+0x144/0x168
 el0t_64_sync+0x198/0x19c
Code: aa1503e0 97f547f8 390512bf f9400a94 (39451280) 
---[ end trace 0000000000000000 ]---
-8<-

On top of this patch series, this issue doesn't happen if I apply the below
change:
diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
index 40a8a5a53a78..2eb0ade65d89 100644
--- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
+++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
@@ -63,7 +63,6 @@ struct imx8qxp_pc_channel {
        struct drm_bridge *next_bridge;
        struct imx8qxp_pc *pc;
        unsigned int stream_id;
-       bool is_available;
 };
 
 struct imx8qxp_pc {
@@ -341,7 +340,6 @@ static int imx8qxp_pc_bridge_probe(struct platform_device *pdev)
 
                ch->bridge.driver_private = ch;
                ch->bridge.of_node = child;
-               ch->is_available = true;
 
                drm_bridge_add(&ch->bridge);
        }
@@ -351,7 +349,7 @@ static int imx8qxp_pc_bridge_probe(struct platform_device *pdev)
 free_child:
        of_node_put(child);
 
-       if (i == 1 && pc->ch[0]->next_bridge)
+       if (i == 1 && pc->ch[0])
                drm_bridge_remove(&pc->ch[0]->bridge);
 
        pm_runtime_disable(dev);
@@ -367,11 +365,8 @@ static void imx8qxp_pc_bridge_remove(struct platform_device *pdev)
        for (i = 0; i < 2; i++) {
                ch = pc->ch[i];
 
-               if (!ch->is_available)
-                       continue;
-
-               drm_bridge_remove(&ch->bridge);
-               ch->is_available = false;
+               if (ch)
+                       drm_bridge_remove(&ch->bridge);
        }
 
        pm_runtime_disable(&pdev->dev);

>  			continue;
> 

-- 
Regards,
Liu Ying