Re: [PATCH] drm/msm/mdp5: fix kernel panic during shutdown
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 88ed5810
...
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 b564 (c8047c02)
[ 87.500264] ---[ end trace ]---
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
---
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 = _kms->base.base;
spin_lock_init(_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 = _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("");
[PATCH] drm/msm/mdp5: fix kernel panic during shutdown
The kernel is panicking when rebooting on MSM8939:
# reboot -f
[ 87.280853] Unable to handle kernel write to read-only memory at
virtual address 88ed5810
...
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 b564 (c8047c02)
[ 87.500264] ---[ end trace ]---
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
---
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 = _kms->base.base;
spin_lock_init(_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 = _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("");
--
2.37.2
