drm/atomic: track bitmask of planes attached to crtc

[Daniel Vetter]

On Thu, Nov 27, 2014 at 06:54:03PM +0300, Dan Carpenter wrote:
> On Thu, Nov 27, 2014 at 10:55:02AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 27, 2014 at 09:41:13AM +0300, Dan Carpenter wrote:
> > > Hello Rob Clark,
> > > 
> > > The patch 1ed2f34b4cc0: "drm/atomic: track bitmask of planes attached
> > > to crtc" from Nov 21, 2014, leads to the following static checker
> > > warning:
> > > 
> > > 	drivers/gpu/drm/drm_atomic.c:368 drm_atomic_set_crtc_for_plane()
> > > 	error: 'plane_state' dereferencing possible ERR_PTR()
> > 
> > Hm yeah that shouldn't ever happen when callers use this correctly. But a
> > WARN_ON would be good I guess. I'll add it.
> > 
> 
> It could fail because of allocation failures.  But maybe this is a boot
> time thing?  Normally dereferencing an ERR_PTR() is easy enough to debug
> and static checkers just ignore WARN_ONs.  I am ambivalent.

Well there rules are that you need to acquire the plane_state first. We're
now respinning the interfaces a bit to again make sure that's done by
requiring callers to directly pass in the plane_state.

btw not sure whether checker should just look through WARN_ON, we have
lots of places where we've historically screwed up and added a WARN_ON +
early return to make sure we'll in the future somewhat recover. This is
really important for gfx since at boot-up (due to fbcon locking bonghits)
the entire intial modeset is run with console_lock held. And that's a few
10k lines of code depending upon platform :(

So we absolutely have to handle failures robustely, but if checkers assume
that it's ok to pass crap caught by WARN_ONs around then that's might
reduce checker usefulness quite a bit.

Just an aside really

> > > 
> > > drivers/gpu/drm/drm_atomic.c
> > >    360  int
> > >    361  drm_atomic_set_crtc_for_plane(struct drm_atomic_state *state,
> > >    362                                struct drm_plane *plane, struct drm_crtc *crtc)
> > >    363  {
> > >    364          struct drm_plane_state *plane_state =
> > >                                         ^^^^^^^^^^^^^
> > >    365                          drm_atomic_get_plane_state(state, plane);
> > >                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >    366          struct drm_crtc_state *crtc_state;
> > >    367  
> > >    368          if (plane_state->crtc) {
> > >                     ^^^^^^^^^^^^^^^^^
> > > Missing IS_ERR() check.
> > > 
> > > Also drm_atomic_get_plane_state() has poor error handling.  In
> > > drm_atomic_get_plane_state(), if the call to drm_atomic_get_plane_state()
> > > fails then it leaks memory.
> > 
> > Where does it leak memory exactly?
> 
> drivers/gpu/drm/drm_atomic.c
>    249  
>    250          plane_state = plane->funcs->atomic_duplicate_state(plane);
> 
> This is a kmemdup().

Another aside: it'll soon be more once a few drivers with atomic support
have merged. But fundamentally they'll all still need to do at least the
kmemdup.

>    251          if (!plane_state)
>    252                  return ERR_PTR(-ENOMEM);
>    253  
>    254          state->plane_states[index] = plane_state;

This statement here should make sure that drm_atomic_state_free cleans
everthing up. So I still don't see a leak ... where does the checker see
one?

>    255          state->planes[index] = plane;
>    256          plane_state->state = state;
>    257  
>    258          DRM_DEBUG_KMS("Added [PLANE:%d] %p state to %p\n",
>    259                        plane->base.id, plane_state, state);
>    260  
>    261          if (plane_state->crtc) {
>    262                  struct drm_crtc_state *crtc_state;
>    263  
>    264                  crtc_state = drm_atomic_get_crtc_state(state,
>    265                                                         plane_state->crtc);
>    266                  if (IS_ERR(crtc_state))
>    267                          return ERR_CAST(crtc_state);
> 
> We leak if we return here.

Note that the atomic stuff is using wait/wound mutexes, so bailing out
with -EDEADLK into the slowpath is an expected path. Hence why we tend to
keep all the allocs around until we eventually get rid of them in one
spot.
-Daniel

> 
>    268          }
>    269  
>    270          return plane_state;
> 
> regards,
> dan carpenter

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch