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.ceres...@bootlin.com>
>
> ---
>
> Cc: Liu Ying <victor....@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
