From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com ([192.55.52.43]:29401 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751845AbeDJSC4 (ORCPT ); Tue, 10 Apr 2018 14:02:56 -0400 Date: Tue, 10 Apr 2018 11:02:55 -0700 From: Rodrigo Vivi To: Chris Wilson Cc: "intel-gfx@lists.freedesktop.org" , "R, Durgadoss" , "stable@vger.kernel.org" Subject: Re: [Intel-gfx] [PATCH] drm/i915/psr: Chase psr.enabled only under the psr.lock Message-ID: <20180410180255.GB2303@intel.com> References: <20180405114915.29609-1-chris@chris-wilson.co.uk> <7e7b4336aefd6e6cf993150c1a83d214c161a210.camel@intel.com> <20180406221816.GG8964@intel.com> <152309192577.26482.17726023366871782940@mail.alporthouse.com> <20180409191432.GN8964@intel.com> <152335802656.3167.11689504042884395588@mail.alporthouse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <152335802656.3167.11689504042884395588@mail.alporthouse.com> Sender: stable-owner@vger.kernel.org List-ID: 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 > > 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