Re: [PATCH] drm/i915: In render_state_init reset obj in teardown path

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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 <joonas.lahtinen@linux.intel.com>
> > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > ---
> >   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 <david.s.gordon@intel.com>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx