Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <94e0ae94-c8f8-8aa5-606d-68e3abca5a9e@quicinc.com>
Date:   Mon, 12 Sep 2022 16:42:53 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.2
Subject: Re: [PATCH] drm/msm/mdp5: fix kernel panic during shutdown
Content-Language: en-US
To:     Fabien Parent <fabien.parent@linaro.org>, <robdclark@gmail.com>,
        <dmitry.baryshkov@linaro.org>
CC:     <airlied@linux.ie>, <daniel@ffwll.ch>, <dianders@chromium.org>,
        <linux-arm-msm@vger.kernel.org>, <dri-devel@lists.freedesktop.org>,
        <freedreno@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>
References: <20220909152856.149291-1-fabien.parent@linaro.org>
From:   Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <20220909152856.149291-1-fabien.parent@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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("");
>