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