Re: [PATCH v7 2/2] PCI: Don't put non-power manageable PCIe root ports into D3

From: Mario Limonciello
Date: Wed Aug 02 2023 - 10:35:48 EST




On 8/2/23 09:31, Mika Westerberg wrote:
Hi,

On Wed, Aug 02, 2023 at 09:10:38AM -0500, Mario Limonciello wrote:


On 8/2/23 00:26, Mika Westerberg wrote:
Hi Mario,

On Tue, Aug 01, 2023 at 10:17:11PM -0500, Mario Limonciello wrote:
Consequently, platform_pci_bridge_d3() will return false and the only
thing that may allow the port to go into D0 is the dmi_get_bios_year()
check at the end of pci_bridge_d3_possible().

However, that was added, because there are Intel platforms on which
Root Ports need to be programmed into D3hot on suspend (which allows
the whole platform to reduce power significantly) and there are no
ACPI device power management objects associated with them (Mika should
know the gory details related to this). It looks like under Windows
the additional power reduction would not be possible on those systems,
but that would be a problem, wouldn't it?


I've been thinking on this today, and I at least have a hypothesis about
this behavior. Perhaps Windows is actually utilizing enabled PEP
constraints to enforce what state device should be put into over Modern
Standby cycles in the absence of ACPI objects.

In the case of one of my problematic system the PEP constraints for the root
port are:

Package (0x04)
{
0x00,
"\\_SB.PCI0.GP17",
0x00,
0x00
},

That first 0x00 means the constraint isn't actually enabled for the root
port.

Mika,

Could you get an acpidump from one of these problematic Intel systems so we
can check the PEP constraints to see if this theory works? Or maybe you have
some other ideas why this is different?

The patch adding this was merged in 2016 and unfortunately I don't have
any of the ACPI dumps from them available anymore (and do not recall the
details either). I think these were Apollo Lake-P based systems with the
initial runtime D3cold and S0ix support at the time.


I scoured the web looking for acpidumps a bit an Apollo Lake system and came
across this random bug report:

https://bugzilla.redhat.com/show_bug.cgi?id=1591307

"Intel(R) Celeron(R) CPU N3450 @ 1.10GHz (family: 0x6, model: 0x5c,
stepping: 0x9)"

I looked at the acpidump, and I notice:

Low Power S0 Idle (V5) : 0

That means that Windows wouldn't actually be putting it into Modern Standby
at suspend but would rather use S3.

Same goes for Linux AFAICT. The ones needed this actually used S0ix so
the bit should definitely be set.

OK.


Considering that result, could we perhaps adjust the check to:

if ((c->x86_vendor == X86_VENDOR_INTEL) && !(acpi_gbl_FADT.flags &
ACPI_FADT_LOW_POWER_S0))

Or could we quirk the PCI root ports from Apollo Lake to opt into D3?

It is not just Apollo Lake, but all "modern" systems as well (sorry if
this was unclear). Apollo Lake just was the first one that needed this.
We also have the Low Power S0 Idle bit set in recent systems too.

Ah got it; I misunderstood it as Apollo Lake was the only one that needed it.

So modern systems that set the bit in the FADT, do they also lack _S0W and _S0D on the root ports?

Does my PEP constraints theory hold steam at all?