[PATCH] drm/mxsfb: Disable overlay plane in mxsfb_plane_overlay_atomic_disable()

[Ying Liu]

On Mon, Jun 12, 2023 at 4:04 PM Sam Ravnborg <sam at ravnborg.org> wrote:
>
> Hi Liu Ying.

Hi Sam,

>
> On Mon, Jun 12, 2023 at 03:55:30PM +0800, Liu Ying wrote:
> > When disabling overlay plane in mxsfb_plane_overlay_atomic_update(),
> > overlay plane's framebuffer pointer is NULL.  So, dereferencing it would
> > cause a kernel Oops(NULL pointer dereferencing).  Fix the issue by
> > disabling overlay plane in mxsfb_plane_overlay_atomic_disable() instead.
>
> Reading the above I had expected that some code was dropped from
> mxsfb_plane_overlay_atomic_update().

Yes, the offending commit cb285a5348e7 dropped mxsfb_get_fb_paddr()
which contains an important !fb check to avoid the NULL pointer
dereferencing.

> I do not know the driver code, but was confused so decided to give
> feedback.

drm_fb_{cma, dma}_get_gem_addr() called by
mxsfb_plane_primary_atomic_update() don't do !fb check but directly
dereference fb.  That's why the NULL pointer dereferencing issue
happens.

With this patch, mxsfb_plane_overlay_atomic_disable() is used
to disable overlay plane, not mxsfb_plane_primary_atomic_update().
Please see funcs->atomic_{disable, update}  in
drm_atomic_helper_commit_planes().

Regards,
Liu Ying

>
>         Sam
>
> >
> > Fixes: cb285a5348e7 ("drm: mxsfb: Replace mxsfb_get_fb_paddr() with drm_fb_cma_get_gem_addr()")
> > Signed-off-by: Liu Ying <victor.liu at nxp.com>
> > ---
> >  drivers/gpu/drm/mxsfb/mxsfb_kms.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > index 3bcc9c0f2019..7ed2516b6de0 100644
> > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > @@ -611,6 +611,14 @@ static void mxsfb_plane_overlay_atomic_update(struct drm_plane *plane,
> >       writel(ctrl, mxsfb->base + LCDC_AS_CTRL);
> >  }
> >
> > +static void mxsfb_plane_overlay_atomic_disable(struct drm_plane *plane,
> > +                                            struct drm_atomic_state *state)
> > +{
> > +     struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev);
> > +
> > +     writel(0, mxsfb->base + LCDC_AS_CTRL);
> > +}
> > +
> >  static bool mxsfb_format_mod_supported(struct drm_plane *plane,
> >                                      uint32_t format,
> >                                      uint64_t modifier)
> > @@ -626,6 +634,7 @@ static const struct drm_plane_helper_funcs mxsfb_plane_primary_helper_funcs = {
> >  static const struct drm_plane_helper_funcs mxsfb_plane_overlay_helper_funcs = {
> >       .atomic_check = mxsfb_plane_atomic_check,
> >       .atomic_update = mxsfb_plane_overlay_atomic_update,
> > +     .atomic_disable = mxsfb_plane_overlay_atomic_disable,
> >  };
> >
> >  static const struct drm_plane_funcs mxsfb_plane_funcs = {
> > --
> > 2.37.1