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

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi Bjorn,

On Tue, Jun 27, 2023 at 03:41:24PM -0500, Bjorn Helgaas wrote:
> On Tue, Jun 27, 2023 at 01:04:47PM +0300, Mika Westerberg wrote:
> > On Tue, Jun 27, 2023 at 11:53:33AM +0200, Thomas Witt wrote:
> > > On 27/06/2023 08:24, Mika Westerberg wrote:
> > > > Commit a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM Substates Capability
> > > > for suspend/resume"") reverted saving and restoring of ASPM L1 Substates
> > > > due to a regression that caused resume from suspend to fail on certain
> > > > systems. However, we never added this capability back and this is now
> > > > causing systems fail to enter low power CPU states, drawing more power
> > > > from the battery.
> > > 
> > > Hello Mika,
> > > 
> > > I am sorry, but your patch (applied on top of master) triggers the exact
> > > same behaviour I described in
> > > <https://bugzilla.kernel.org/show_bug.cgi?id=216877> (nvme and wifi become
> > > unavailable during suspend/resume)
> > 
> > Thanks for testing! Can you provide the output of dmidecode from that
> > system? We can add it to the denylist as well to avoid the issue on your
> > system.
> 
> To me this says we don't completely understand the mechanism of the
> failure.  If BIOS made L1SS work initially, Linux should be able to
> make it work again after suspend/resume.
> 
> If we can identify an actual hardware or firmware defect, it makes
> good sense to add a quirk or denylist.  But I'll push back a little if
> it's just "there's some problem we don't understand on this system, so
> avoid it."

Fair enough.

I've looked at the Thomas' system dmesg that he attached to the bugzilla
and he has mem_sleep_default=deep in the kernel command line. There is
no such option in the mainline kernel but I suppose this makes systemd
(or some initscript) to write "deep" to /sys/power/mem_sleep, thus
forcing S3 instead of the default the ACPI tables suggest, which
probably is S0ix (Intel low power state which does not involve BIOS).

So when forcing S3 this means the system suspend and resume is done
through the BIOS who is supposed to enable wakes and program the devices
accordingly during and after S3 before the OS is given back the control,
it might very well be that it already did something for the L1
configuration here before Linux (with this patch) reconfigured them and
this is causing the problem.

@Thomas, is there any particular reason you have this option in the
command line? There is possibility that S3 is not even fully validated
if the system advertises S0 low power sleep instead.