[Intel-gfx] [PATCH 2/2] drm/i915/pxp: Trigger the global teardown for before suspending

[Juston Li]

On Mon, 2022-11-21 at 19:21 +0000, Teres Alexis, Alan Previn wrote:
> 
> 
> On Mon, 2022-11-21 at 10:39 -0800, Juston Li wrote:
> > On Thu, 2022-11-17 at 16:36 -0800, Alan Previn wrote:
> > > A driver bug was recently discovered where the security firmware
> > > was
> > > receiving internal HW signals indicating that session key
> > > expirations
> > > had occurred. Architecturally, the firmware was expecting a
> > > response
> > > from the GuC to acknowledge the event with the firmware side.
> > > However the OS was in a suspended state and GuC had been reset.
> > > Internal specifications actually required the driver to ensure
> > > that all active sessions be properly cleaned up in such cases
> > > where
> > > the system is suspended and the GuC potentially unable to
> > > respond.
> > > 
> > > This patch adds the global teardown code in i915's
> > > suspend_prepare
> > > code path.
> > > 
> Alan:[snip]
> 
> > > +void intel_pxp_end(struct intel_pxp *pxp)
> > > +{
> > > +       struct drm_i915_private *i915 = pxp_to_gt(pxp)->i915;
> > > +       intel_wakeref_t wakeref;
> > > +
> > > +       if (!intel_pxp_is_enabled(pxp))
> > > +               return;
> > > +
> > > +       wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> > > +
> > > +       mutex_lock(&pxp->arb_mutex);
> > > +
> > > +       if (__pxp_global_teardown_locked(pxp, true))
> > > +               drm_dbg(&(pxp_to_gt(pxp))->i915->drm, "PXP end
> > > timed
> > > out\n");
> > > +
> > > +       mutex_unlock(&pxp->arb_mutex);
> > > +
> > > +       intel_pxp_fini_hw(pxp);
> > 
> > Is intel_pxp_suspend() still needed then if we already fini_hw()
> > here
> > and mark invalidation in intel_pxp_terminate()?
> > 
> 
> Good catch - looks like we might not need intel_pxp_suspend. But I'll
> verify that for you.

Actually, might need to be careful here. If system aborts suspend or
fails to suspend for any reason, suspend_prepare()->intel_pxp_fini_hw()
might have been called but not suspend().

Correct me if I'm wrong, but in that case I don't think resume() will
be called and thus intel_pxp_init_hw().

For some background, there were some issues with PXP ending up in a bad
state when some other driver caused suspend to fail or user
closed/opened lid quickly and aborted suspend.

> Also, looks like i forgot to include a non-CONFIG_DRM_I915_PXP
> version of intel_pxp_end which was causing the build
> failure. Will resend.
> 
> Btw, thanks for reviewing this Juston, i had cc'd you because of the
> impact to suspend-resume flows and I believe you
> have had prior experience debugging issues with that and runtime-
> suspend/resume. Do you any other issues with this
> change?

Np, thanks for the patches!

The only other concern I had that's more of a downstream issue is we
ended up using hw_state_invalidated to block PXP ioctl ops during
teardown to prevent further PXP ioctls triggering pxp_start and another
termination queued.
I don't recall if I sent you this patch on our tree:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3207105
I think this could happen in suspend now too, if app sends PXP ops
while suspend termination is in progress.

Juston