On Mon, Nov 03, 2014 at 01:54:00PM +0100, Daniel Vetter wrote:
> On Fri, Oct 31, 2014 at 04:07:35PM +0000, Chris Wilson wrote:
> > On Fri, Oct 31, 2014 at 02:52:40PM +0000, Damien Lespiau wrote:
> > > On Fri, Oct 31, 2014 at 12:00:26PM +0000, John.C.Harrison@xxxxxxxxx wrote:
> > > > From: John Harrison <John.C.Harrison@xxxxxxxxx>
> > > >
> > > > If a ring failed to initialise for any reason then the error path would try to
> > > > clean up all rings including those that had not yet been allocated. The ring
> > > > clean up code did a check that the ring was valid before starting its work.
> > > > Unfortunately, that was after it had already dereferenced the ring to obtain a
> > > > dev_private pointer.
> > > >
> > > > Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> > >
> > > This looks good to me.
> >
> > Really? These functions(!!!) are only called under controlled conditions...
> > I would have been happy to see this follow my suggestion I made to fix
> > this bug months ago.
>
> Hm, do you mean to shuffle the ring_initialized checks into callers? Or
> something else?
i915_gem_cleanup_engines()
{
/* Not the regular for_each_engine so we can cleanup a failed setup */
for (int i =0 ; i < I915_NUM_ENGINES; i++)
intel_engine_cleanup(&to_i915(dev)->engine[i]);
}
And that is callable then from an incomplete setup as well as normal
teardown.
And intel_engine_cleanup() need just be:
void intel_engine_cleanup(struct intel_engine_cs *engine)
{
WARN_ON(engine->last_request);
if (engine->cleanup)
engine->cleanup(engine);
}
Remove bugs by removing code, win.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
