Re: [PATCH v4] PCI/ASPM: Add back L1 PM Substate save and restore

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Oct 05, 2023 at 12:02:58PM +0300, Mika Westerberg wrote:
> On Wed, Oct 04, 2023 at 05:23:24PM -0500, Bjorn Helgaas wrote:
> ...

> The thing with TUXEDO is that on Thomas's system with mem_sleep=deep
> this patch (without denylist) breaks the resume as he describes here:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=216877
> 
> However, on exact same TUXEDO system with the same firmwares Werner is
> not able to reproduce the issue with or without the patch. So I'm not
> sure what to do and that's why I added denylist that should take effect
> on Thomas' system when mem_sleep=deep is set but also work on the same
> system without it.
> 
> And since we have the denylist, I decided to add the ASUS there to avoid
> accidentally breaking those too.
> ...

> > I think there's still something we're missing.
> > 
> > We restore the LTR config before restoring DEVCTL2 (including the LTR
> > enable bit) and L1SS state.  I don't think we know the state of ASPM
> > and L1SS at that point, do we?  Do you think there could be an issue
> > there, too?
> 
> AFAICT LTR does not affect until it is explicitly enabled and I don't
> think many drivers actually program it (although we have some sort of
> API for it at least for Intel LPSS devices).

I couldn't find anything in the spec that suggests LTR should be an
issue.  I'm just grasping at straws here.

There's obviously *something* we're doing wrong because ASPM worked
before suspend, so we should be able to get it to work after resume.

Could we learn anything by dumping config space of the problem devices
before/after the suspend/resume and comparing them?

If we could figure out a difference between Werner's working TUXEDO
and Thomas' non-working TUXEDO, that might be a hint, too.

> If you have suggestions, I'm all open. If I understand you would like
> this to be done like:
> 
>   - Drop the denylist
>   - Add back the suspend/restore of L1SS
>   - Ask everyone in this thread to try it out
> 
> I can do that no problem but I guess that for the TUXEDO one (Thomas')
> it probably is going to fail still.

Right, without the denylist, I expect Thomas' TUXEDO to fail, but I
still hope we can figure out why.  If we just keep it on the denylist,
that system will suffer from more power consumption than necessary,
but only after suspend/resume.

A denylist seems like the absolute last resort.  In this case we don't
know about anything *wrong* with those platforms; all we know is that
our resume path doesn't work.  It's likely that it fails on other
platforms we haven't heard about, too.

Bjorn