Re: [PATCH] drm/msm: Call shutdown conditionally

From: Daniel Vetter <daniel@ffwll.ch>
To: Fabio Estevam <festevam@gmail.com>
Cc: Krishna Manikandan <mkrishn@codeaurora.org>,
	Dave Airlie <airlied@linux.ie>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	stable <stable@vger.kernel.org>,
	Sascha Hauer <kernel@pengutronix.de>, Sean Paul <sean@poorly.run>
Subject: Re: [PATCH] drm/msm: Call shutdown conditionally
Date: Tue, 19 Jan 2021 19:01:01 +0100	[thread overview]
Message-ID: <CAKMK7uE5vo8G8AYTUAndy7Jj2wqmQGR1kcjjRn-ymGsxw-EvQQ@mail.gmail.com> (raw)
In-Reply-To: <20210119174455.2423205-1-festevam@gmail.com>

On Tue, Jan 19, 2021 at 6:47 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Calling the drm_atomic_helper_shutdown() on i.MX5 leads to
> the following flow:
>
> [   24.557742] [<c0769b78>] (msm_atomic_commit_tail) from [<c06db0b4>]
> (commit_tail+0xa4/0x1b0)
> [   24.566349] [<c06db0b4>] (commit_tail) from [<c06dbed0>]
> (drm_atomic_helper_commit+0x154/0x188)
> [   24.575193] [<c06dbed0>] (drm_atomic_helper_commit) from
> [<c06db604>] (drm_atomic_helper_disable_all+0x154/0x1c0)
> [   24.585599] [<c06db604>] (drm_atomic_helper_disable_all) from
> [<c06db704>] (drm_atomic_helper_shutdown+0x94/0x12c)
> [   24.596094] [<c06db704>] (drm_atomic_helper_shutdown) from
>
> In the i.MX5 case, priv->kms is not populated (as i.MX5 does not use any
> of the Qualcomm display controllers), causing a NULL pointer
> dereference in msm_atomic_commit_tail():
>
> [   24.268964] 8<--- cut here ---
> [   24.274602] Unable to handle kernel NULL pointer dereference at
> virtual address 00000000
> [   24.283434] pgd = (ptrval)
> [   24.286387] [00000000] *pgd=ca212831
> [   24.290788] Internal error: Oops: 17 [#1] SMP ARM
> [   24.295609] Modules linked in:
> [   24.298777] CPU: 0 PID: 197 Comm: init Not tainted 5.11.0-rc2-next-20210111 #333
> [   24.306276] Hardware name: Freescale i.MX53 (Device Tree Support)
> [   24.312442] PC is at msm_atomic_commit_tail+0x54/0xb9c
> [   24.317743] LR is at commit_tail+0xa4/0x1b0
>
> Fix the problem by calling drm_atomic_helper_shutdown() conditionally.
>
> Cc: <stable@vger.kernel.org>
> Fixes: 9d5cbf5fe46e ("drm/msm: add shutdown support for display platform_driver")
> Suggested-by: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
>  drivers/gpu/drm/msm/msm_drv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 108c405e03dd..c082b72b9e3b 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -1311,7 +1311,8 @@ static void msm_pdev_shutdown(struct platform_device *pdev)
>  {
>         struct drm_device *drm = platform_get_drvdata(pdev);
>
> -       drm_atomic_helper_shutdown(drm);
> +       if (get_mdp_ver(pdev))
> +               drm_atomic_helper_shutdown(drm);

Don't we need the same treatment for suspend/resume too? Also if you
feel like follow up patch to push this into the helpers with a
DRIVER_MODESET check like I described would be kinda neat.
-Daniel


>  }
>
>  static const struct of_device_id dt_match[] = {
> --
> 2.25.1
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel