[RESEND,V2] drm: fsl-dcu: Fix no fb check bug

[Meng Yi]

> >>         switch (fb->pixel_format) {
> >>         case DRM_FORMAT_RGB565:
> >>         case DRM_FORMAT_RGB888:
> >> @@ -85,9 +88,6 @@ static void fsl_dcu_drm_plane_atomic_update(struct
> drm_plane *plane,
> >>         unsigned int alpha, bpp;
> >>         int index, ret;
> >>
> >> -       if (!fb)
> >> -               return;
> >> -
> > ... which no longer has the !fb check, and we'll crash with null deref
> > a few lines below ?
> 
> 
> If there is a legitimate situation where fb is null which also ultimately leads to a
> atomic_commit, I guess we should keep the return here...

I think I made a mistake here, fb check should not be removed . As Stefan mentioned, if fb check in fsl_dcu_drm_plane_atomic_check return 0, fsl_dcu_drm_plane_atomic_update will ultimately called, and we'll crash since plane->state->fb is NULL.


> -----Original Message-----
> From: Stefan Agner [mailto:stefan at agner.ch]
> Sent: Thursday, January 14, 2016 1:54 PM
> To: Emil Velikov <emil.l.velikov at gmail.com>
> Cc: Meng Yi <meng.yi at nxp.com>; ML dri-devel <dri-
> devel at lists.freedesktop.org>
> Subject: Re: [RESEND,V2] drm: fsl-dcu: Fix no fb check bug
> 
> On 2016-01-08 01:20, Emil Velikov wrote:
> > Hi guys,
> >
> > Am I loosing the plot here or something feels amiss here ?
> >
> > On 6 January 2016 at 06:12, Meng Yi <meng.yi at nxp.com> wrote:
> >> For state->fb or state->crtc may be NULL in
> >> fsl_dcu_drm_plane_atomic_check function, if so, return 0.
> >>
> >> Signed-off-by: Meng Yi <meng.yi at nxp.com>
> >> Signed-off-by: Jianwei Wang <jianwei.wang.chn at gmail.com>
> >>
> >> ---
> >>
> >> change in v2:
> >> -Add state->crtc check
> >> -return 0 when state->fb or state->crtc is NULL, instead of -EINVAL
> >> Adviced by Daniel Stone
> >>
> >>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> >> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> >> index 4b13cf9..8965580 100644
> >> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> >> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
> >> @@ -41,6 +41,9 @@ static int fsl_dcu_drm_plane_atomic_check(struct
> >> drm_plane *plane,  {
> >>         struct drm_framebuffer *fb = state->fb;
> >>
> >> +       if (!state->fb || !state->crtc)
> >> +               return 0;
> >> +
> > Namely: if we return success here core drm will end up calling the
> > atomic_update...
> >
> 
> After atomic_check atomic_disable could be called too. However, this seem
> not directly depend on state'>fb, but more on plane->state->crtc.
> 
> 
> 
> >>         switch (fb->pixel_format) {
> >>         case DRM_FORMAT_RGB565:
> >>         case DRM_FORMAT_RGB888:
> >> @@ -85,9 +88,6 @@ static void fsl_dcu_drm_plane_atomic_update(struct
> drm_plane *plane,
> >>         unsigned int alpha, bpp;
> >>         int index, ret;
> >>
> >> -       if (!fb)
> >> -               return;
> >> -
> > ... which no longer has the !fb check, and we'll crash with null deref
> > a few lines below ?
> 
> 
> If there is a legitimate situation where fb is null which also ultimately leads to a
> atomic_commit, I guess we should keep the return here...
> 
> --
> Stefan