MIME-Version: 1.0
References: <20211116122831.278576-1-martyn.welch@collabora.com>
In-Reply-To: <20211116122831.278576-1-martyn.welch@collabora.com>
From:   Robert Foss <robert.foss@linaro.org>
Date:   Tue, 23 Nov 2021 16:18:49 +0100
Message-ID: <CAG3jFytjX1_X1dzJAm_yqTx49CVpnKttRgH-dpUHEJK6GqqTuw@mail.gmail.com>
Subject: Re: [PATCH] drm/bridge: megachips: Ensure both bridges are probed
 before registration
To:     Martyn Welch <martyn.welch@collabora.com>
Cc:     dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        kernel@collabora.com, Peter Senna Tschudin <peter.senna@gmail.com>,
        Martin Donnelly <martin.donnelly@ge.com>,
        Martyn Welch <martyn.welch@collabora.co.uk>,
        Andrzej Hajda <a.hajda@samsung.com>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hey Martyn,

On Tue, 16 Nov 2021 at 13:28, Martyn Welch <martyn.welch@collabora.com> 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@collabora.com>
> CC: Peter Senna Tschudin <peter.senna@gmail.com>
> CC: Martin Donnelly <martin.donnelly@ge.com>
> CC: Martyn Welch <martyn.welch@collabora.co.uk>
> CC: Andrzej Hajda <a.hajda@samsung.com>
> CC: Neil Armstrong <narmstrong@baylibre.com>
> CC: Robert Foss <robert.foss@linaro.org>
> CC: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> CC: Jonas Karlman <jonas@kwiboo.se>
> CC: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  .../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
>