[Intel-gfx] [PATCH 15/17] drm/atomic-helpers: functions for state duplicate/destroy/reset

[Daniel Vetter]

On Mon, Nov 03, 2014 at 02:45:28PM +0000, Daniel Thompson wrote:
> > index 70bd67cf86e3..bd38df3cbe55 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1429,7 +1429,7 @@ EXPORT_SYMBOL(drm_atomic_helper_set_config);
> >  /**
> >   * drm_atomic_helper_crtc_set_property - helper for crtc prorties
> >   * @crtc: DRM crtc
> > - * @prorty: DRM property
> > + * @property: DRM property
> 
> This looks like a bad fixup (should be in patch 11).

Indeed, will shuffle around.

> > +void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc)
> > +{
> > +	kfree(crtc->state);
> > +	crtc->state = kzalloc(sizeof(*crtc->state), GFP_KERNEL);
> 
> This code looks semantically equivalent to a memset() although it may
> result in a change to the pointer value. Is this code trying to flush
> out uses-after-free?
> 
> I can't find this free/alloc pattern in delivered code anywhere else in
> the drm code base. Should this need to be replaced with memset() before
> merging (or at least commenting)?

kfree is a nop when the argument is NULL, which is a crucial property of
this - memset would oops on driver load.

Even neglecting this a memset imo doesn't blow up loudly enough if the
driver subclasses the state structs (by adding more of it's driver private
state at the end). Whereas underallocating tends to anger the slab
poisoning code badly.

Finally it's really not just a memset, but a free + realloc. See the plane
state, which also needs to drop a potential fb reference. Imo the explicit
kfree+realloc makes that more obvious.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch