| Message ID | 1461662472-2608-1-git-send-email-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > 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>
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
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; return ret; }
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(+)