| Message ID | 1414756826-21062-1-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 31, 2014 at 12:00:26PM +0000, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > 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@Intel.com> This looks good to me. Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
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@Intel.com wrote: > > From: John Harrison <John.C.Harrison@Intel.com> > > > > 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@Intel.com> > > 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. -Chris
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@Intel.com wrote: > > > From: John Harrison <John.C.Harrison@Intel.com> > > > > > > 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@Intel.com> > > > > 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? John's patch didn't look offensive really, so merged it for now. -Daniel
On 31/10/14 14:52, Damien Lespiau wrote: > On Fri, Oct 31, 2014 at 12:00:26PM +0000, John.C.Harrison@Intel.com wrote: >> From: John Harrison <John.C.Harrison@Intel.com> >> >> 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@Intel.com> > > This looks good to me. > > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> And simpler than the version I previously posted, as that would have had to have another test added for each new ring in future hardware. However I think the description above is slightly misleading, as the problem wasn't dereferencing "ring" but "ring->dev". "ring" is always non-NULL (it's the address of a member of an array inside dev_priv), but the backpointer "ring->dev" is only filled in during ring initialisation. .Dave.
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@Intel.com wrote: > > > > From: John Harrison <John.C.Harrison@Intel.com> > > > > > > > > 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@Intel.com> > > > > > > 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
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index cd74e5c..76776fa 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1214,11 +1214,13 @@ static int gen8_emit_request(struct intel_ringbuffer *ringbuf) */ void intel_logical_ring_cleanup(struct intel_engine_cs *ring) { - struct drm_i915_private *dev_priv = ring->dev->dev_private; + struct drm_i915_private *dev_priv; if (!intel_ring_initialized(ring)) return; + dev_priv = ring->dev->dev_private; + intel_logical_ring_stop(ring); WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0); ring->preallocated_lazy_request = NULL; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index a8f72e8..f457146 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1845,12 +1845,15 @@ error: void intel_cleanup_ring_buffer(struct intel_engine_cs *ring) { - struct drm_i915_private *dev_priv = to_i915(ring->dev); - struct intel_ringbuffer *ringbuf = ring->buffer; + struct drm_i915_private *dev_priv; + struct intel_ringbuffer *ringbuf; if (!intel_ring_initialized(ring)) return; + dev_priv = to_i915(ring->dev); + ringbuf = ring->buffer; + intel_stop_ring_buffer(ring); WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);