On ti, 2016-04-26 at 11:04 +0100, Dave Gordon wrote: > On 26/04/16 10:21, Matthew Auld wrote: > > > > The teardown path in render_state_init leaves so->obj != NULL. > > > > Suggested-by: Joonas Lahtinen <[email protected]> > > Signed-off-by: Matthew Auld <[email protected]> > > --- > > drivers/gpu/drm/i915/i915_gem_render_state.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c > > b/drivers/gpu/drm/i915/i915_gem_render_state.c > > index 71611bf..10f3cf0 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_render_state.c > > +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c > > @@ -70,6 +70,7 @@ static int render_state_init(struct render_state *so, > > struct drm_device *dev) > > > > free_gem: > > drm_gem_object_unreference(&so->obj->base); > > + so->obj = NULL;
This is object_init style function, if it fails, the contents of "so" is expected to be uninitialized, and should only be freed or attempted to re-initialize by caller, never inspected, so no need for this. See gen8_ppgtt_init for an example, it would be rather cubersome to undo all assignments on error. In short, I do not see this as a necessary step. Regards, Joonas > > return ret; > > } > It doesn't actually matter, because 'so' is pointing to a local object a > few frames up the callstack. But I don't think this function is entitled > to assume that; it should leave everything in a consistent state so > there aren't any dangling pointers to objects that have been freed lying > around - someday the argument may turn into a per-engine static or some > other long-lived thing. So, > > Reviewed-by: Dave Gordon <[email protected]> > _______________________________________________ > Intel-gfx mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Joonas Lahtinen Open Source Technology Center Intel Corporation _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
