Re: [PATCH] drm/bridge: megachips: Ensure both bridges are probed before registration
From: Robert Foss
Date: Tue Nov 23 2021 - 10:19:09 EST
Hey Martyn,
On Tue, 16 Nov 2021 at 13:28, Martyn Welch <martyn.welch@xxxxxxxxxxxxx> wrote:
>
> In the configuration used by the b850v3, the STDP2690 is used to read EDID
> data whilst it's the STDP4028 which can detect when monitors are connected.
>
> This can result in problems at boot with monitors connected when the
> STDP4028 is probed first, a monitor is detected and an attempt is made to
> read the EDID data before the STDP2690 has probed:
>
> [ 3.795721] Unable to handle kernel NULL pointer dereference at virtual address 00000018
> [ 3.803845] pgd = (ptrval)
> [ 3.806581] [00000018] *pgd=00000000
> [ 3.810180] Internal error: Oops: 5 [#1] SMP ARM
> [ 3.814813] Modules linked in:
> [ 3.817879] CPU: 0 PID: 64 Comm: kworker/u4:1 Not tainted 5.15.0 #1
> [ 3.824161] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> [ 3.830705] Workqueue: events_unbound deferred_probe_work_func
> [ 3.836565] PC is at stdp2690_get_edid+0x44/0x19c
> [ 3.841286] LR is at ge_b850v3_lvds_get_modes+0x2c/0x5c
> [ 3.846526] pc : [<805eae10>] lr : [<805eb138>] psr: 80000013
> [ 3.852802] sp : 81c359d0 ip : 7dbb550b fp : 81c35a1c
> [ 3.858037] r10: 81c73840 r9 : 81c73894 r8 : 816d9800
> [ 3.863270] r7 : 00000000 r6 : 81c34000 r5 : 00000000 r4 : 810c35f0
> [ 3.869808] r3 : 80e3e294 r2 : 00000080 r1 : 00000cc0 r0 : 81401180
> [ 3.876349] Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> [ 3.883499] Control: 10c5387d Table: 1000404a DAC: 00000051
> [ 3.889254] Register r0 information: slab kmem_cache start 81401180 pointer offset 0
> [ 3.897034] Register r1 information: non-paged memory
> [ 3.902097] Register r2 information: non-paged memory
> [ 3.907160] Register r3 information: non-slab/vmalloc memory
> [ 3.912832] Register r4 information: non-slab/vmalloc memory
> [ 3.918503] Register r5 information: NULL pointer
> [ 3.923217] Register r6 information: non-slab/vmalloc memory
> [ 3.928887] Register r7 information: NULL pointer
> [ 3.933601] Register r8 information: slab kmalloc-1k start 816d9800 pointer offset 0 size 1024
> [ 3.942244] Register r9 information: slab kmalloc-2k start 81c73800 pointer offset 148 size 2048
> [ 3.951058] Register r10 information: slab kmalloc-2k start 81c73800 pointer offset 64 size 2048
> [ 3.959873] Register r11 information: non-slab/vmalloc memory
> [ 3.965632] Register r12 information: non-paged memory
> [ 3.970781] Process kworker/u4:1 (pid: 64, stack limit = 0x(ptrval))
> [ 3.977148] Stack: (0x81c359d0 to 0x81c36000)
> [ 3.981517] 59c0: 80b2b668 80b2b5bc 000002e2 0000034e
> [ 3.989712] 59e0: 81c35a8c 816d98e8 81c35a14 7dbb550b 805bfcd0 810c35f0 81c73840 824addc0
> [ 3.997906] 5a00: 00001000 816d9800 81c73894 81c73840 81c35a34 81c35a20 805eb138 805eadd8
> [ 4.006099] 5a20: 810c35f0 00000045 81c35adc 81c35a38 80594188 805eb118 80d7c788 80dd1848
> [ 4.014292] 5a40: 00000000 81c35a50 80dca950 811194d3 80dca7c4 80dca944 80dca91c 816d9800
> [ 4.022485] 5a60: 81c34000 81c760a8 816d9800 80c58c98 810c35f0 816d98e8 00001000 00001000
> [ 4.030678] 5a80: 00000000 00000000 8017712c 81c60000 00000002 00000001 00000000 00000000
> [ 4.038870] 5aa0: 816d9900 816d9900 00000000 7dbb550b 805c700c 00000008 826282c8 826282c8
> [ 4.047062] 5ac0: 00001000 81e1ce40 00001000 00000002 81c35bf4 81c35ae0 805d9694 80593fc0
> [ 4.055255] 5ae0: 8017a970 80179ad8 00000179 00000000 81c35bcc 81c35b00 80177108 8017a950
> [ 4.063447] 5b00: 00000000 81c35b10 81c34000 00000000 81004fd8 81010a38 00000000 00000059
> [ 4.071639] 5b20: 816d98d4 81fbb718 00000013 826282c8 8017a940 81c35b40 81134448 00000400
> [ 4.079831] 5b40: 00000178 00000000 e063b9c1 00000000 c2000049 00000040 00000000 00000008
> [ 4.088024] 5b60: 82628300 82628380 00000000 00000000 81c34000 00000000 81fbb700 82628340
> [ 4.096216] 5b80: 826283c0 00001000 00000000 00000010 816d9800 826282c0 801766f8 00000000
> [ 4.104408] 5ba0: 00000000 81004fd8 00000049 00000000 00000000 00000001 80dcf940 80178de4
> [ 4.112601] 5bc0: 81c35c0c 7dbb550b 80178de4 81fbb700 00000010 00000010 810c35f4 81e1ce40
> [ 4.120793] 5be0: 81c40908 0000000c 81c35c64 81c35bf8 805a7f18 805d94a0 81c35c3c 816d9800
> [ 4.128985] 5c00: 00000010 81c34000 81c35c2c 81c35c18 8012fce0 805be90c 81c35c3c 81c35c28
> [ 4.137178] 5c20: 805be90c 80173210 81fbb600 81fbb6b4 81c35c5c 7dbb550b 81c35c64 81fbb700
> [ 4.145370] 5c40: 816d9800 00000010 810c35f4 81e1ce40 81c40908 0000000c 81c35c84 81c35c68
> [ 4.153565] 5c60: 805a8c78 805a7ed0 816d9800 81fbb700 00000010 00000000 81c35cac 81c35c88
> [ 4.161758] 5c80: 805a8dc4 805a8b68 816d9800 00000000 816d9800 00000000 8179f810 810c42d0
> [ 4.169950] 5ca0: 81c35ccc 81c35cb0 805e47b0 805a8d18 824aa240 81e1ea80 81c40908 81126b60
> [ 4.178144] 5cc0: 81c35d14 81c35cd0 8060db1c 805e46cc 81c35d14 81c35ce0 80dd90f8 810c4d58
> [ 4.186338] 5ce0: 80dd90dc 81fe9740 fffffffe 81fe9740 81e1ea80 00000000 810c4d6c 80c4b95c
> [ 4.194531] 5d00: 80dd9a3c 815c6810 81c35d34 81c35d18 8060dc9c 8060d8fc 8246b440 815c6800
> [ 4.202724] 5d20: 815c6810 eefd8e00 81c35d44 81c35d38 8060dd80 8060dbec 81c35d6c 81c35d48
> [ 4.210918] 5d40: 805e98a4 8060dd70 00000000 815c6810 810c45b0 81126e90 81126e90 80dd9a3c
> [ 4.219112] 5d60: 81c35d8c 81c35d70 80619574 805e9808 815c6810 00000000 810c45b0 81126e90
> [ 4.227305] 5d80: 81c35db4 81c35d90 806168dc 80619514 80625df0 80623c80 815c6810 810c45b0
> [ 4.235498] 5da0: 81c35e6c 815c6810 81c35dec 81c35db8 80616d04 80616800 81c35de4 81c35dc8
> [ 4.243691] 5dc0: 808382b0 80b2f444 8116e310 8116e314 81c35e6c 815c6810 00000003 80dd9a3c
> [ 4.251884] 5de0: 81c35e14 81c35df0 80616ec8 80616c60 00000001 810c45b0 81c35e6c 815c6810
> [ 4.260076] 5e00: 00000001 80dd9a3c 81c35e34 81c35e18 80617338 80616e90 00000000 81c35e6c
> [ 4.268269] 5e20: 80617284 81c34000 81c35e64 81c35e38 80614730 80617290 81c35e64 8171a06c
> [ 4.276461] 5e40: 81e220b8 7dbb550b 815c6810 81c34000 815c6854 81126e90 81c35e9c 81c35e68
> [ 4.284654] 5e60: 8061673c 806146a8 8060f5e0 815c6810 00000001 7dbb550b 00000000 810c5080
> [ 4.292847] 5e80: 810c5320 815c6810 81126e90 00000000 81c35eac 81c35ea0 80617554 80616650
> [ 4.301040] 5ea0: 81c35ecc 81c35eb0 80615694 80617544 810c5080 810c5080 810c5094 81126e90
> [ 4.309233] 5ec0: 81c35efc 81c35ed0 80615c6c 8061560c 80615bc0 810c50c0 817eeb00 81412800
> [ 4.317425] 5ee0: 814c3000 00000000 814c300d 81119a60 81c35f3c 81c35f00 80141488 80615bcc
> [ 4.325618] 5f00: 81c60000 81c34000 81c35f24 81c35f18 80143078 817eeb00 81412800 817eeb18
> [ 4.333811] 5f20: 81412818 81003d00 00000088 81412800 81c35f74 81c35f40 80141a48 80141298
> [ 4.342005] 5f40: 81c35f74 81c34000 801481ac 817efa40 817efc00 801417d8 817eeb00 00000000
> [ 4.350199] 5f60: 815a7e7c 81c34000 81c35fac 81c35f78 80149b1c 801417e4 817efc20 817efc20
> [ 4.358391] 5f80: ffffe000 817efa40 801499a8 00000000 00000000 00000000 00000000 00000000
> [ 4.366583] 5fa0: 00000000 81c35fb0 80100130 801499b4 00000000 00000000 00000000 00000000
> [ 4.374774] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [ 4.382966] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
> [ 4.391155] Backtrace:
> [ 4.393613] [<805eadcc>] (stdp2690_get_edid) from [<805eb138>] (ge_b850v3_lvds_get_modes+0x2c/0x5c)
> [ 4.402691] r10:81c73840 r9:81c73894 r8:816d9800 r7:00001000 r6:824addc0 r5:81c73840
> [ 4.410534] r4:810c35f0
> [ 4.413073] [<805eb10c>] (ge_b850v3_lvds_get_modes) from [<80594188>] (drm_helper_probe_single_connector_modes+0x1d4/0x84c)
> [ 4.424240] r5:00000045 r4:810c35f0
> [ 4.427822] [<80593fb4>] (drm_helper_probe_single_connector_modes) from [<805d9694>] (drm_client_modeset_probe+0x200/0x1384)
> [ 4.439074] r10:00000002 r9:00001000 r8:81e1ce40 r7:00001000 r6:826282c8 r5:826282c8
> [ 4.446917] r4:00000008
> [ 4.449455] [<805d9494>] (drm_client_modeset_probe) from [<805a7f18>] (__drm_fb_helper_initial_config_and_unlock+0x54/0x5b4)
> [ 4.460713] r10:0000000c r9:81c40908 r8:81e1ce40 r7:810c35f4 r6:00000010 r5:00000010
> [ 4.468556] r4:81fbb700
> [ 4.471095] [<805a7ec4>] (__drm_fb_helper_initial_config_and_unlock) from [<805a8c78>] (drm_fbdev_client_hotplug+0x11c/0x1b0)
> [ 4.482434] r10:0000000c r9:81c40908 r8:81e1ce40 r7:810c35f4 r6:00000010 r5:816d9800
> [ 4.490276] r4:81fbb700
> [ 4.492814] [<805a8b5c>] (drm_fbdev_client_hotplug) from [<805a8dc4>] (drm_fbdev_generic_setup+0xb8/0x1a4)
> [ 4.502494] r7:00000000 r6:00000010 r5:81fbb700 r4:816d9800
> [ 4.508160] [<805a8d0c>] (drm_fbdev_generic_setup) from [<805e47b0>] (imx_drm_bind+0xf0/0x130)
> [ 4.516805] r7:810c42d0 r6:8179f810 r5:00000000 r4:816d9800
> [ 4.522474] [<805e46c0>] (imx_drm_bind) from [<8060db1c>] (try_to_bring_up_master+0x22c/0x2f0)
> [ 4.531116] r7:81126b60 r6:81c40908 r5:81e1ea80 r4:824aa240
> [ 4.536783] [<8060d8f0>] (try_to_bring_up_master) from [<8060dc9c>] (__component_add+0xbc/0x184)
> [ 4.545597] r10:815c6810 r9:80dd9a3c r8:80c4b95c r7:810c4d6c r6:00000000 r5:81e1ea80
> [ 4.553440] r4:81fe9740
> [ 4.555980] [<8060dbe0>] (__component_add) from [<8060dd80>] (component_add+0x1c/0x20)
> [ 4.563921] r7:eefd8e00 r6:815c6810 r5:815c6800 r4:8246b440
> [ 4.569589] [<8060dd64>] (component_add) from [<805e98a4>] (dw_hdmi_imx_probe+0xa8/0xe8)
> [ 4.577702] [<805e97fc>] (dw_hdmi_imx_probe) from [<80619574>] (platform_probe+0x6c/0xc8)
> [ 4.585908] r9:80dd9a3c r8:81126e90 r7:81126e90 r6:810c45b0 r5:815c6810 r4:00000000
> [ 4.593662] [<80619508>] (platform_probe) from [<806168dc>] (really_probe+0xe8/0x460)
> [ 4.601524] r7:81126e90 r6:810c45b0 r5:00000000 r4:815c6810
> [ 4.607191] [<806167f4>] (really_probe) from [<80616d04>] (__driver_probe_device+0xb0/0x230)
> [ 4.615658] r7:815c6810 r6:81c35e6c r5:810c45b0 r4:815c6810
> [ 4.621326] [<80616c54>] (__driver_probe_device) from [<80616ec8>] (driver_probe_device+0x44/0xe0)
> [ 4.630313] r9:80dd9a3c r8:00000003 r7:815c6810 r6:81c35e6c r5:8116e314 r4:8116e310
> [ 4.638068] [<80616e84>] (driver_probe_device) from [<80617338>] (__device_attach_driver+0xb4/0x12c)
> [ 4.647227] r9:80dd9a3c r8:00000001 r7:815c6810 r6:81c35e6c r5:810c45b0 r4:00000001
> [ 4.654981] [<80617284>] (__device_attach_driver) from [<80614730>] (bus_for_each_drv+0x94/0xd8)
> [ 4.663794] r7:81c34000 r6:80617284 r5:81c35e6c r4:00000000
> [ 4.669461] [<8061469c>] (bus_for_each_drv) from [<8061673c>] (__device_attach+0xf8/0x190)
> [ 4.677753] r7:81126e90 r6:815c6854 r5:81c34000 r4:815c6810
> [ 4.683419] [<80616644>] (__device_attach) from [<80617554>] (device_initial_probe+0x1c/0x20)
> [ 4.691971] r8:00000000 r7:81126e90 r6:815c6810 r5:810c5320 r4:810c5080
> [ 4.698681] [<80617538>] (device_initial_probe) from [<80615694>] (bus_probe_device+0x94/0x9c)
> [ 4.707318] [<80615600>] (bus_probe_device) from [<80615c6c>] (deferred_probe_work_func+0xac/0xf0)
> [ 4.716305] r7:81126e90 r6:810c5094 r5:810c5080 r4:810c5080
> [ 4.721973] [<80615bc0>] (deferred_probe_work_func) from [<80141488>] (process_one_work+0x1fc/0x54c)
> [ 4.731139] r10:81119a60 r9:814c300d r8:00000000 r7:814c3000 r6:81412800 r5:817eeb00
> [ 4.738981] r4:810c50c0 r3:80615bc0
> [ 4.742563] [<8014128c>] (process_one_work) from [<80141a48>] (worker_thread+0x270/0x570)
> [ 4.750765] r10:81412800 r9:00000088 r8:81003d00 r7:81412818 r6:817eeb18 r5:81412800
> [ 4.758608] r4:817eeb00
> [ 4.761147] [<801417d8>] (worker_thread) from [<80149b1c>] (kthread+0x174/0x190)
> [ 4.768574] r10:81c34000 r9:815a7e7c r8:00000000 r7:817eeb00 r6:801417d8 r5:817efc00
> [ 4.776417] r4:817efa40
> [ 4.778955] [<801499a8>] (kthread) from [<80100130>] (ret_from_fork+0x14/0x24)
> [ 4.786201] Exception stack(0x81c35fb0 to 0x81c35ff8)
> [ 4.791266] 5fa0: 00000000 00000000 00000000 00000000
> [ 4.799459] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [ 4.807651] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [ 4.814279] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:801499a8
> [ 4.822120] r4:817efa40
> [ 4.824664] Code: e3a02080 e593001c e3a01d33 e3a05000 (e5979018)
>
> Split the registration from the STDP4028 probe routine and only perform
> registration once both the STDP4028 and STDP2690 have probed.
>
> Signed-off-by: Martyn Welch <martyn.welch@xxxxxxxxxxxxx>
> CC: Peter Senna Tschudin <peter.senna@xxxxxxxxx>
> CC: Martin Donnelly <martin.donnelly@xxxxxx>
> CC: Martyn Welch <martyn.welch@xxxxxxxxxxxxxxx>
> CC: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> CC: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> CC: Robert Foss <robert.foss@xxxxxxxxxx>
> CC: Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>
> CC: Jonas Karlman <jonas@xxxxxxxxx>
> CC: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
> ---
> .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 40 +++++++++++++------
> 1 file changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> index d2808c4a6fb1..72f134849822 100644
> --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> @@ -306,19 +306,10 @@ static void ge_b850v3_lvds_remove(void)
> mutex_unlock(&ge_b850v3_lvds_dev_mutex);
> }
>
> -static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
> - const struct i2c_device_id *id)
> +static int ge_b850v3_register(void)
> {
> + struct i2c_client *stdp4028_i2c = ge_b850v3_lvds_ptr->stdp4028_i2c;
> struct device *dev = &stdp4028_i2c->dev;
> - int ret;
> -
> - ret = ge_b850v3_lvds_init(dev);
> -
> - if (ret)
> - return ret;
> -
> - ge_b850v3_lvds_ptr->stdp4028_i2c = stdp4028_i2c;
> - i2c_set_clientdata(stdp4028_i2c, ge_b850v3_lvds_ptr);
>
> /* drm bridge initialization */
> ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs;
> @@ -343,6 +334,27 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
> "ge-b850v3-lvds-dp", ge_b850v3_lvds_ptr);
> }
>
> +static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
> + const struct i2c_device_id *id)
> +{
> + struct device *dev = &stdp4028_i2c->dev;
> + int ret;
> +
> + ret = ge_b850v3_lvds_init(dev);
> +
> + if (ret)
> + return ret;
> +
> + ge_b850v3_lvds_ptr->stdp4028_i2c = stdp4028_i2c;
> + i2c_set_clientdata(stdp4028_i2c, ge_b850v3_lvds_ptr);
> +
> + /* Only register after both bridges are probed */
> + if (!(ge_b850v3_lvds_ptr->stdp2690_i2c && ge_b850v3_lvds_ptr->stdp4028_i2c))
ge_b850v3_lvds_ptr->stdp4028_i2c was just assigned, is there any
situation it wouldn't be assigned here?
stdp2690_ge_b850v3_fw_probe() seems to contain an identical snippet,
so I assume symmetry is why both stdp2690_i2c and stdp4028_i2c are
checked in both locations. Unless you think otherwise, I would suggest
removing the stdp4028_i2c check from stdp4028_ge_b850v3_fw_probe() and
the stdp2690_i2c check from stdp2690_ge_b850v3_fw_probe().
> + return 0;
My reading of this code is that stdp4028_ge_b850v3_fw_probe() and
stdp2690_ge_b850v3_fw_probe() are racing, and the 2nd driver to probe
will go ahead and call ge_b850v3_register(). Is there any reason why
EPROBE_DEFER isn't used here?
> +
> + return ge_b850v3_register();
> +}
> +
> static int stdp4028_ge_b850v3_fw_remove(struct i2c_client *stdp4028_i2c)
> {
> ge_b850v3_lvds_remove();
> @@ -386,7 +398,11 @@ static int stdp2690_ge_b850v3_fw_probe(struct i2c_client *stdp2690_i2c,
> ge_b850v3_lvds_ptr->stdp2690_i2c = stdp2690_i2c;
> i2c_set_clientdata(stdp2690_i2c, ge_b850v3_lvds_ptr);
>
> - return 0;
> + /* Only register after both bridges are probed */
> + if (!(ge_b850v3_lvds_ptr->stdp2690_i2c && ge_b850v3_lvds_ptr->stdp4028_i2c))
> + return 0;
> +
> + return ge_b850v3_register();
> }
>
> static int stdp2690_ge_b850v3_fw_remove(struct i2c_client *stdp2690_i2c)
> --
> 2.33.0
>