Message-Id: <DDJ623AGI83R.ESC0V9XXWXFN@bootlin.com>
Date: Wed, 15 Oct 2025 22:08:59 +0200
From: "Luca Ceresoli" <luca.ceresoli@...tlin.com>
To: "Marek Szyprowski" <m.szyprowski@...sung.com>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "David Airlie"
 <airlied@...il.com>, "Simona Vetter" <simona@...ll.ch>, "Andrzej Hajda"
 <andrzej.hajda@...el.com>, "Neil Armstrong" <neil.armstrong@...aro.org>,
 "Robert Foss" <rfoss@...nel.org>, "Laurent Pinchart"
 <Laurent.pinchart@...asonboard.com>, "Jonas Karlman" <jonas@...boo.se>,
 "Jernej Skrabec" <jernej.skrabec@...il.com>, "Dmitry Baryshkov"
 <dmitry.baryshkov@....qualcomm.com>
Cc: "Hui Pu" <Hui.Pu@...ealthcare.com>, "Thomas Petazzoni"
 <thomas.petazzoni@...tlin.com>, <dri-devel@...ts.freedesktop.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] drm/display: bridge_connector: get/put the stored
 bridges

Hello Marek,

On Wed Oct 15, 2025 at 10:22 AM CEST, Marek Szyprowski wrote:
> Hi Luca,
>
> On 26.09.2025 16:59, Luca Ceresoli wrote:
>> drm_bridge_connector_init() takes eight pointers to various bridges, some
>> of which can be identical, and stores them in pointers inside struct
>> drm_bridge_connector. Get a reference to each of the taken bridges and put
>> it on cleanup.
>>
>> This is tricky because the pointers are currently stored directly in the
>> drm_bridge_connector in the loop, but there is no nice and clean way to put
>> those pointers on error return paths. To overcome this, store all pointers
>> in temporary local variables with a cleanup action, and only on success
>> copy them into struct drm_bridge_connector (getting another ref while
>> copying).
>>
>> Additionally four of these pointers (edid, hpd, detect and modes) can be
>> written in multiple loop iterations, in order to eventually store the last
>> matching bridge. However, when one of those pointers is overwritten, we
>> need to put the reference that we got during the previous assignment. Add a
>> drm_bridge_put() before writing them to handle this.
>>
>> Finally, there is also a function-local panel_bridge pointer taken inside
>> the loop and used after the loop. Use a cleanup action as well to ensure it
>> is put on return.
>>
>> Signed-off-by: Luca Ceresoli <luca.ceresoli@...tlin.com>
>
> This patch landed recently in linux-next as commit 2be300f9a0b6
> ("drm/display: bridge_connector: get/put the stored bridges"). In my
> tests I found that it causes the following NULL pointer dereference on
> DragonBoard410c (arch/arm64/boot/dts/qcom/apq8016-sbc.dts):
>
> --->8---
>
>   msm_mdp 1a01000.display-controller: bound 1c00000.gpu (ops a3xx_ops [msm])
>   msm_mdp 1a01000.display-controller: [drm:mdp5_kms_init [msm]] MDP5
> version v1.6
>   msm_mdp 1a01000.display-controller: fall back to the other CTL
> category for INTF 1!
>   Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000110
>   ...
>   Internal error: Oops: 0000000096000004 [#1]  SMP
>   Modules linked in: qcom_wcnss_pil(+) cpufreq_powersave
> cpufreq_conservative coresight_stm stm_core coresight_replicator
> coresight_cpu_debug coresight_funnel coresight_tmc coresight_cti
> coresight_tpiu adv7511 nfc coresight rfkill msm snd_soc_lpass_apq8016
> snd_soc_lpass_cpu snd_soc_apq8016_sbc snd_soc_msm8916_digital
> snd_soc_msm8916_analog snd_soc_lpass_platform snd_soc_qcom_common
> snd_soc_core qrtr snd_compress snd_pcm_dmaengine qcom_camss
> qcom_spmi_temp_alarm snd_pcm qcom_q6v5_mss qcom_pil_info ubwc_config
> qcom_q6v5 videobuf2_dma_sg llcc_qcom v4l2_fwnode rtc_pm8xxx ocmem
> venus_core v4l2_async qcom_sysmon qcom_common qcom_spmi_vadc drm_gpuvm
> qcom_vadc_common qcom_pon v4l2_mem2mem snd_timer drm_exec
> videobuf2_memops gpu_sched videobuf2_v4l2 snd qcom_glink_smem
> drm_dp_aux_bus videodev soundcore mdt_loader qmi_helpers
> drm_display_helper qnoc_msm8916 videobuf2_common qcom_stats mc qcom_rng
> rpmsg_ctrl rpmsg_char ramoops reed_solomon display_connector socinfo
> rmtfs_mem ax88796b asix usbnet phy_qcom_usb_hs ipv6
>   CPU: 2 UID: 0 PID: 42 Comm: kworker/u16:1 Tainted: G W
> 6.17.0-rc6+ #16051 PREEMPT
>   Tainted: [W]=WARN
>   Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
>   Workqueue: events_unbound deferred_probe_work_func
>   pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>   pc : drm_bridge_connector_hdmi_cec_init+0x8/0x38 [drm_display_helper]
>   lr : drmm_connector_hdmi_cec_register+0xf8/0x1c8 [drm_display_helper]
>   sp : ffff8000843d3560
>   x29: ffff8000843d3560 x28: 0000000000000000 x27: 0000000000000000
>   x26: ffff000010c14400 x25: ffff00000c034b68 x24: ffff00003b82a020
>   x23: ffff00000c030000 x22: ffff0000097bf7d0 x21: ffff0000042e6a60
>   x20: ffff80007c162af8 x19: ffff00000c034080 x18: 00000000ffffffff
>   x5 : 00000000000007ff x4 : ffff800083cc5c50 x3 : 0000000000000000
>   x2 : 0000000000000000 x1 : ffff00000c034080 x0 : 0000000000000000
>   Call trace:
>    drm_bridge_connector_hdmi_cec_init+0x8/0x38 [drm_display_helper] (P)
>    drm_bridge_connector_init+0x6b4/0x6d4 [drm_display_helper]
>    msm_dsi_manager_connector_init+0x9c/0xf0 [msm]
>    msm_dsi_modeset_init+0x60/0xe8 [msm]
>    modeset_init+0x3c4/0x4c0 [msm]
>    mdp5_kms_init+0x3cc/0x670 [msm]
>    msm_drm_kms_init+0x40/0x33c [msm]
>    msm_drm_init+0x1c4/0x284 [msm]
>    msm_drm_bind+0x30/0x3c [msm]
>    try_to_bring_up_aggregate_device+0x168/0x1d4
>    __component_add+0xa8/0x170
>    component_add+0x14/0x20
>    dsi_dev_attach+0x20/0x2c [msm]
>    dsi_host_attach+0x58/0x98 [msm]
>    devm_mipi_dsi_attach+0x34/0x90
>    adv7533_attach_dsi+0x8c/0x104 [adv7511]
>    adv7511_probe+0x764/0x988 [adv7511]
>    i2c_device_probe+0x154/0x350
>    really_probe+0xbc/0x298
>    __driver_probe_device+0x78/0x12c
>    driver_probe_device+0xdc/0x164
>    __device_attach_driver+0xb8/0x138
>    bus_for_each_drv+0x80/0xdc
>    __device_attach+0xa8/0x1b0
>    device_initial_probe+0x14/0x20
>    bus_probe_device+0xb0/0xb4
>    deferred_probe_work_func+0x8c/0xc8
>    process_one_work+0x208/0x60c
>    worker_thread+0x244/0x388
>    kthread+0x150/0x228
>    ret_from_fork+0x10/0x20
>   Code: d50323bf d65f03c0 aa0003e1 f945e400 (f9408802)
>   ---[ end trace 0000000000000000 ]---

Thanks for testing and reporting.

I'm afraid I have no hardware where the same bug can be reproduced, but by
code inspection the root cause is clear given the call chain:

  drm_bridge_connector_init() [1]
   -> drmm_connector_hdmi_cec_register() [2]
       -> funcs->init() = drm_bridge_connector_hdmi_cec_init() [3]

[1] used to set bridge_connector->bridge_hdmi_cec before calling [2], now
it does it afterwards. But [3] expects it to be set already.

I have overlooked this when writing the patch. My apologies.

> This can be easily fixed by adding the following check:
>
> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c
> b/drivers/gpu/drm/display/drm_bridge_connector.c
> index 7b18be3ff9a3..222ecbc98155 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> @@ -601,7 +601,7 @@ static int drm_bridge_connector_hdmi_cec_init(struct
> drm_connector *connector)
>
>          bridge = bridge_connector->bridge_hdmi_cec;
>
> -       if (!bridge->funcs->hdmi_cec_init)
> +       if (!bridge || !bridge->funcs->hdmi_cec_init)
>                  return 0;
>
>          return bridge->funcs->hdmi_cec_init(bridge, connector);
>
>
> However I don't know the internals of the related code to judge that
> such check is the proper way to fix this issue.

I'm afraid this is not a correct solution. When this function is called,
the bridge_hdmi_cec is already known, but (after my patch) not yet set
where it is expected to be. So skipping the bridge->funcs->hdmi_cec_init()
call would make the code behave differently for no good reason.

I'm looking at how to properly fix this bug ASAP.

Best regards,
Luca

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

Powered by blists - more mailing lists