Re: [Intel-gfx] [PATCH] drm/i915/psr: Chase psr.enabled only under the psr.lock

From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"R, Durgadoss" <durgadoss.r@intel.com>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: Re: [Intel-gfx] [PATCH] drm/i915/psr: Chase psr.enabled only under the psr.lock
Date: Tue, 10 Apr 2018 11:02:55 -0700	[thread overview]
Message-ID: <20180410180255.GB2303@intel.com> (raw)
In-Reply-To: <152335802656.3167.11689504042884395588@mail.alporthouse.com>

On Tue, Apr 10, 2018 at 12:00:26PM +0100, Chris Wilson wrote:
> Quoting Rodrigo Vivi (2018-04-09 20:14:32)
> > On Sat, Apr 07, 2018 at 10:05:25AM +0100, Chris Wilson wrote:
> > > Quoting Rodrigo Vivi (2018-04-06 23:18:16)
> > > > On Fri, Apr 06, 2018 at 11:12:27AM -0700, Souza, Jose wrote:
> > > > > On Thu, 2018-04-05 at 12:49 +0100, Chris Wilson wrote:
> > > > > > +           struct drm_crtc *crtc =
> > > > > > +                   dp_to_dig_port(intel_dp)->base.base.crtc;
> > > > 
> > > > I'm afraid that the issue is this pointer here. So this will only mask
> > > > the issue.
> > > > 
> > > > Should we maybe stash the pipe? :/
> > > 
> > > It's not that bad. pipe cannot change until after psr_disable is called,
> > > right? And psr_disable ensures that this worker is flushed. The current
> > > problem is just the coordination of cancelling the worker, where we may
> > > set psr.enabled to NULL right before the worker grabs it and
> > > dereferences it.
> > > 
> > > So if we lock until we have the pipe, we know that dereference chain is
> > > valid, and we know that psr_disable() cannot complete until we complete
> > > the wait. So the pipe remains valid until we return (so long as the pipe
> > > exists when we start).
> > 
> > hmm... it makes sense and I have no better suggestion actually.
> > So, as long it really fixes the regression we introduced:
> > 
> > Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> It does fix the abstract race, but I have no evidence of this being hit
> in practice. Pushed, but up to you if you care about this being
> backported.
> 
> Note this race is different from the GPF CI reported. Hmm, I think
> https://bugs.freedesktop.org/show_bug.cgi?id=105959 is the same one as
> hit on the kasan run earlier.

Ouch, thanks for the clarification... I was really considering that this
was the case... but I should have noticed that there was no bugzilla
referenced here...

> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx