| Message ID | 20210430082744.3638743-1-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/meson: fix shutdown crash when component not probed | expand |
On Fri, Apr 30, 2021 at 10:28 AM Neil Armstrong <narmstrong@baylibre.com> wrote: > > When main component is not probed, by example when the dw-hdmi module is > not loaded yet or in probe defer, the following crash appears on shutdown: > > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000038 > ... > pc : meson_drv_shutdown+0x24/0x50 > lr : platform_drv_shutdown+0x20/0x30 > ... > Call trace: > meson_drv_shutdown+0x24/0x50 > platform_drv_shutdown+0x20/0x30 > device_shutdown+0x158/0x360 > kernel_restart_prepare+0x38/0x48 > kernel_restart+0x18/0x68 > __do_sys_reboot+0x224/0x250 > __arm64_sys_reboot+0x24/0x30 > ... > > Simply check if the priv struct has been allocated before using it. > > Fixes: fa0c16caf3d7 ("drm: meson_drv add shutdown function") > Reported-by: Stefan Agner <stefan@agner.ch> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Hi Neil, since this has not received any Reviewed-by yet I tried my best to review it myself On Fri, Apr 30, 2021 at 10:28 AM Neil Armstrong <narmstrong@baylibre.com> wrote: [...] > --- a/drivers/gpu/drm/meson/meson_drv.c > +++ b/drivers/gpu/drm/meson/meson_drv.c > @@ -485,11 +485,12 @@ static int meson_probe_remote(struct platform_device *pdev, > static void meson_drv_shutdown(struct platform_device *pdev) > { > struct meson_drm *priv = dev_get_drvdata(&pdev->dev); this part made it hard for me because I was wondering where the matching dev_set_drvdata call is it turns out platform_set_drvdata is used instead, meaning for me it would have been easier to understand if platform_get_drvdata was used here that's however nothing which has changed with this patch > - struct drm_device *drm = priv->drm; > > - DRM_DEBUG_DRIVER("\n"); > - drm_kms_helper_poll_fini(drm); > - drm_atomic_helper_shutdown(drm); > + if (!priv) > + return; > + > + drm_kms_helper_poll_fini(priv->drm); > + drm_atomic_helper_shutdown(priv->drm); > } then this part finally made sense to me (as non-drm person), as platform_set_drvdata comes near the end of meson_drv_bind_master (so any errors would cause the drvdata to be NULL). with this I can also give me: Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> in addition to my: Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Can you please queue this up for -fixes or do we need to ask someone to do it? Best regards, Martin
Hi Martin, On 20/05/2021 22:25, Martin Blumenstingl wrote: > Hi Neil, > > since this has not received any Reviewed-by yet I tried my best to > review it myself > > On Fri, Apr 30, 2021 at 10:28 AM Neil Armstrong <narmstrong@baylibre.com> wrote: > [...] >> --- a/drivers/gpu/drm/meson/meson_drv.c >> +++ b/drivers/gpu/drm/meson/meson_drv.c >> @@ -485,11 +485,12 @@ static int meson_probe_remote(struct platform_device *pdev, >> static void meson_drv_shutdown(struct platform_device *pdev) >> { >> struct meson_drm *priv = dev_get_drvdata(&pdev->dev); > this part made it hard for me because I was wondering where the > matching dev_set_drvdata call is > it turns out platform_set_drvdata is used instead, meaning for me it > would have been easier to understand if platform_get_drvdata was used > here > that's however nothing which has changed with this patch OK sure, indeed, but since it's the same... but yeah it may be an issue if platform_set_drvdata changes. > >> - struct drm_device *drm = priv->drm; >> >> - DRM_DEBUG_DRIVER("\n"); >> - drm_kms_helper_poll_fini(drm); >> - drm_atomic_helper_shutdown(drm); >> + if (!priv) >> + return; >> + >> + drm_kms_helper_poll_fini(priv->drm); >> + drm_atomic_helper_shutdown(priv->drm); >> } > then this part finally made sense to me (as non-drm person), as > platform_set_drvdata comes near the end of meson_drv_bind_master (so > any errors would cause the drvdata to be NULL). > > with this I can also give me: > Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > in addition to my: > Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Thanks ! > > Can you please queue this up for -fixes or do we need to ask someone to do it? Yes, the drm-misc-next & drm-misc-fixes are aleays opened and merged each week in the corresponding drm-next and drm-fixes branches, so we can push patches at anytime without thinking about the merge window (except drm-misc-fixes merged back with -rc1). Neil > > > Best regards, > Martin >
On 30/04/2021 10:27, Neil Armstrong wrote: > When main component is not probed, by example when the dw-hdmi module is > not loaded yet or in probe defer, the following crash appears on shutdown: > > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000038 > ... > pc : meson_drv_shutdown+0x24/0x50 > lr : platform_drv_shutdown+0x20/0x30 > ... > Call trace: > meson_drv_shutdown+0x24/0x50 > platform_drv_shutdown+0x20/0x30 > device_shutdown+0x158/0x360 > kernel_restart_prepare+0x38/0x48 > kernel_restart+0x18/0x68 > __do_sys_reboot+0x224/0x250 > __arm64_sys_reboot+0x24/0x30 > ... > > Simply check if the priv struct has been allocated before using it. > > Fixes: fa0c16caf3d7 ("drm: meson_drv add shutdown function") > Reported-by: Stefan Agner <stefan@agner.ch> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/gpu/drm/meson/meson_drv.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c > index 453d8b4c5763..07fcd12dca16 100644 > --- a/drivers/gpu/drm/meson/meson_drv.c > +++ b/drivers/gpu/drm/meson/meson_drv.c > @@ -485,11 +485,12 @@ static int meson_probe_remote(struct platform_device *pdev, > static void meson_drv_shutdown(struct platform_device *pdev) > { > struct meson_drm *priv = dev_get_drvdata(&pdev->dev); > - struct drm_device *drm = priv->drm; > > - DRM_DEBUG_DRIVER("\n"); > - drm_kms_helper_poll_fini(drm); > - drm_atomic_helper_shutdown(drm); > + if (!priv) > + return; > + > + drm_kms_helper_poll_fini(priv->drm); > + drm_atomic_helper_shutdown(priv->drm); > } > > static int meson_drv_probe(struct platform_device *pdev) > Applied to drm-misc-fixes Neil
diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c index 453d8b4c5763..07fcd12dca16 100644 --- a/drivers/gpu/drm/meson/meson_drv.c +++ b/drivers/gpu/drm/meson/meson_drv.c @@ -485,11 +485,12 @@ static int meson_probe_remote(struct platform_device *pdev, static void meson_drv_shutdown(struct platform_device *pdev) { struct meson_drm *priv = dev_get_drvdata(&pdev->dev); - struct drm_device *drm = priv->drm; - DRM_DEBUG_DRIVER("\n"); - drm_kms_helper_poll_fini(drm); - drm_atomic_helper_shutdown(drm); + if (!priv) + return; + + drm_kms_helper_poll_fini(priv->drm); + drm_atomic_helper_shutdown(priv->drm); } static int meson_drv_probe(struct platform_device *pdev)
When main component is not probed, by example when the dw-hdmi module is not loaded yet or in probe defer, the following crash appears on shutdown: Unable to handle kernel NULL pointer dereference at virtual address 0000000000000038 ... pc : meson_drv_shutdown+0x24/0x50 lr : platform_drv_shutdown+0x20/0x30 ... Call trace: meson_drv_shutdown+0x24/0x50 platform_drv_shutdown+0x20/0x30 device_shutdown+0x158/0x360 kernel_restart_prepare+0x38/0x48 kernel_restart+0x18/0x68 __do_sys_reboot+0x224/0x250 __arm64_sys_reboot+0x24/0x30 ... Simply check if the priv struct has been allocated before using it. Fixes: fa0c16caf3d7 ("drm: meson_drv add shutdown function") Reported-by: Stefan Agner <stefan@agner.ch> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/gpu/drm/meson/meson_drv.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)