| Message ID | 20220909152856.149291-1-fabien.parent@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm/msm/mdp5: fix kernel panic during shutdown | expand |
Hi Fabien Thanks for the patch. I believe this issue should get resolved with https://patchwork.freedesktop.org/patch/490326/ as this avoids the override. I have acked that change and will pick it up for the next fixes. Thanks Abhinav On 9/9/2022 8:28 AM, Fabien Parent wrote: > The kernel is panicking when rebooting on MSM8939: > > # reboot -f > [ 87.280853] Unable to handle kernel write to read-only memory at virtual address ffff800008ed5810 > ... > snip > ... > [ 87.445142] Call trace: > [ 87.452253] mutex_lock+0x1c/0x50 > [ 87.454511] msm_drv_shutdown+0x28/0x40 > [ 87.457984] platform_shutdown+0x28/0x40 > [ 87.461629] device_shutdown+0x14c/0x240 > [ 87.465796] __do_sys_reboot+0x180/0x274 > [ 87.469703] __arm64_sys_reboot+0x28/0x3c > [ 87.473608] invoke_syscall+0x54/0x124 > [ 87.477515] el0_svc_common.constprop.0+0x44/0xec > [ 87.481163] do_el0_svc+0x90/0xe0 > [ 87.485934] el0_svc+0x50/0xa4 > [ 87.489232] el0t_64_sync_handler+0x11c/0x150 > [ 87.492185] el0t_64_sync+0x190/0x194 > [ 87.496618] Code: f9800011 c85ffc03 ca010064 b5000064 (c8047c02) > [ 87.500264] ---[ end trace 0000000000000000 ]--- > Segmentation fault > > The issue comes from the fact that mdp5_init() is calling > platform_set_drvdata() and consequently overwriting the driver data > previously set by msm_drv_probe. > msm_drv_shutdown was casting the driver data as "struct msm_drm_private" > while it was actually a "struct mdp5_kms". > > This commit fixes the issue by having mdp5_init() not override the > platform driver data, and instead use a series of > to_mdp5_kms(to_mdp_kms(priv->kms)) to retrieve the mdp5_kms from the > pdata. > > Fixes: 54199009958f ("drm/msm: Fix shutdown") > Signed-off-by: Fabien Parent <fabien.parent@linaro.org> > --- > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > index d2a48caf9d27..17aeabeedfeb 100644 > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c > @@ -634,7 +634,8 @@ static int mdp5_kms_init(struct drm_device *dev) > > static void mdp5_destroy(struct platform_device *pdev) > { > - struct mdp5_kms *mdp5_kms = platform_get_drvdata(pdev); > + struct msm_drm_private *priv = platform_get_drvdata(pdev); > + struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); > int i; > > if (mdp5_kms->ctlm) > @@ -797,7 +798,8 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) > goto fail; > } > > - platform_set_drvdata(pdev, mdp5_kms); > + /* set uninit-ed kms */ > + priv->kms = &mdp5_kms->base.base; > > spin_lock_init(&mdp5_kms->resource_lock); > > @@ -890,13 +892,13 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) > if (ret) > goto fail; > > - /* set uninit-ed kms */ > - priv->kms = &mdp5_kms->base.base; > - > return 0; > fail: > if (mdp5_kms) > mdp5_destroy(pdev); > + > + priv->kms = NULL; > + > return ret; > } > > @@ -956,7 +958,8 @@ static int mdp5_dev_remove(struct platform_device *pdev) > static __maybe_unused int mdp5_runtime_suspend(struct device *dev) > { > struct platform_device *pdev = to_platform_device(dev); > - struct mdp5_kms *mdp5_kms = platform_get_drvdata(pdev); > + struct msm_drm_private *priv = platform_get_drvdata(pdev); > + struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); > > DBG(""); > > @@ -966,7 +969,8 @@ static __maybe_unused int mdp5_runtime_suspend(struct device *dev) > static __maybe_unused int mdp5_runtime_resume(struct device *dev) > { > struct platform_device *pdev = to_platform_device(dev); > - struct mdp5_kms *mdp5_kms = platform_get_drvdata(pdev); > + struct msm_drm_private *priv = platform_get_drvdata(pdev); > + struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); > > DBG(""); >
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c index d2a48caf9d27..17aeabeedfeb 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c @@ -634,7 +634,8 @@ static int mdp5_kms_init(struct drm_device *dev) static void mdp5_destroy(struct platform_device *pdev) { - struct mdp5_kms *mdp5_kms = platform_get_drvdata(pdev); + struct msm_drm_private *priv = platform_get_drvdata(pdev); + struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); int i; if (mdp5_kms->ctlm) @@ -797,7 +798,8 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) goto fail; } - platform_set_drvdata(pdev, mdp5_kms); + /* set uninit-ed kms */ + priv->kms = &mdp5_kms->base.base; spin_lock_init(&mdp5_kms->resource_lock); @@ -890,13 +892,13 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) if (ret) goto fail; - /* set uninit-ed kms */ - priv->kms = &mdp5_kms->base.base; - return 0; fail: if (mdp5_kms) mdp5_destroy(pdev); + + priv->kms = NULL; + return ret; } @@ -956,7 +958,8 @@ static int mdp5_dev_remove(struct platform_device *pdev) static __maybe_unused int mdp5_runtime_suspend(struct device *dev) { struct platform_device *pdev = to_platform_device(dev); - struct mdp5_kms *mdp5_kms = platform_get_drvdata(pdev); + struct msm_drm_private *priv = platform_get_drvdata(pdev); + struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); DBG(""); @@ -966,7 +969,8 @@ static __maybe_unused int mdp5_runtime_suspend(struct device *dev) static __maybe_unused int mdp5_runtime_resume(struct device *dev) { struct platform_device *pdev = to_platform_device(dev); - struct mdp5_kms *mdp5_kms = platform_get_drvdata(pdev); + struct msm_drm_private *priv = platform_get_drvdata(pdev); + struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms)); DBG("");
The kernel is panicking when rebooting on MSM8939: # reboot -f [ 87.280853] Unable to handle kernel write to read-only memory at virtual address ffff800008ed5810 ... snip ... [ 87.445142] Call trace: [ 87.452253] mutex_lock+0x1c/0x50 [ 87.454511] msm_drv_shutdown+0x28/0x40 [ 87.457984] platform_shutdown+0x28/0x40 [ 87.461629] device_shutdown+0x14c/0x240 [ 87.465796] __do_sys_reboot+0x180/0x274 [ 87.469703] __arm64_sys_reboot+0x28/0x3c [ 87.473608] invoke_syscall+0x54/0x124 [ 87.477515] el0_svc_common.constprop.0+0x44/0xec [ 87.481163] do_el0_svc+0x90/0xe0 [ 87.485934] el0_svc+0x50/0xa4 [ 87.489232] el0t_64_sync_handler+0x11c/0x150 [ 87.492185] el0t_64_sync+0x190/0x194 [ 87.496618] Code: f9800011 c85ffc03 ca010064 b5000064 (c8047c02) [ 87.500264] ---[ end trace 0000000000000000 ]--- Segmentation fault The issue comes from the fact that mdp5_init() is calling platform_set_drvdata() and consequently overwriting the driver data previously set by msm_drv_probe. msm_drv_shutdown was casting the driver data as "struct msm_drm_private" while it was actually a "struct mdp5_kms". This commit fixes the issue by having mdp5_init() not override the platform driver data, and instead use a series of to_mdp5_kms(to_mdp_kms(priv->kms)) to retrieve the mdp5_kms from the pdata. Fixes: 54199009958f ("drm/msm: Fix shutdown") Signed-off-by: Fabien Parent <fabien.parent@linaro.org> --- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)