[PATCH v7 0/2] Fix wakeup problems on some AMD platforms

[PATCH v7 0/2] Fix wakeup problems on some AMD platforms

[Mario Limonciello]

Problems have been reported on AMD laptops with suspend/resume
where particular root ports are put into D3 and then the system is unable
to resume properly.

The issue boils down to the currently selected kernel policy for root port
behavior at suspend time:
0) If the machine is from 2015 or later
1) If a PCIe root port is power manageable by the platform then platform
   will be used to determine the power state of the root port at suspend.
2) If the PCIe root is not power manageable by the platform then the kernel
   will check if it was configured to wakeup. 
3) If it was, then it will be put into the deepest state that supports
   wakeup from PME.
4) If it wasn't, then it will be put into D3hot.

This series adjusts it so only root ports that are power manageable by
the platform will be considered for being put into D3.

Mario Limonciello (2):
  PCI: Refactor pci_bridge_d3_possible()
  PCI: Don't put PCIe root ports into D3 unless they are power
    manageable

 drivers/pci/pci.c | 76 ++++++++++++++++++++++++++---------------------
 1 file changed, 42 insertions(+), 34 deletions(-)


base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
-- 
2.34.1

[PATCH v7 1/2] PCI: Refactor pci_bridge_d3_possible()

[Mario Limonciello]

All of the cases handled by pci_bridge_d3_possible() are specific
to these branches:
```
	case PCI_EXP_TYPE_ROOT_PORT:
	case PCI_EXP_TYPE_UPSTREAM:
	case PCI_EXP_TYPE_DOWNSTREAM:
```
Drop a level of indentation by returning false in the default case
instead.  No intended functional changes.

Acked-by: Rafael J. Wysocki <rafael@kernel.org>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/pci.c | 68 +++++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 60230da957e0c..f916fd76eba79 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3004,48 +3004,48 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
 	case PCI_EXP_TYPE_ROOT_PORT:
 	case PCI_EXP_TYPE_UPSTREAM:
 	case PCI_EXP_TYPE_DOWNSTREAM:
-		if (pci_bridge_d3_disable)
-			return false;
+		break;
+	default:
+		return false;
+	}
 
-		/*
-		 * Hotplug ports handled by firmware in System Management Mode
-		 * may not be put into D3 by the OS (Thunderbolt on non-Macs).
-		 */
-		if (bridge->is_hotplug_bridge && !pciehp_is_native(bridge))
-			return false;
+	if (pci_bridge_d3_disable)
+		return false;
 
-		if (pci_bridge_d3_force)
-			return true;
+	/*
+	 * Hotplug ports handled by firmware in System Management Mode
+	 * may not be put into D3 by the OS (Thunderbolt on non-Macs).
+	 */
+	if (bridge->is_hotplug_bridge && !pciehp_is_native(bridge))
+		return false;
 
-		/* Even the oldest 2010 Thunderbolt controller supports D3. */
-		if (bridge->is_thunderbolt)
-			return true;
+	if (pci_bridge_d3_force)
+		return true;
 
-		/* Platform might know better if the bridge supports D3 */
-		if (platform_pci_bridge_d3(bridge))
-			return true;
+	/* Even the oldest 2010 Thunderbolt controller supports D3. */
+	if (bridge->is_thunderbolt)
+		return true;
 
-		/*
-		 * Hotplug ports handled natively by the OS were not validated
-		 * by vendors for runtime D3 at least until 2018 because there
-		 * was no OS support.
-		 */
-		if (bridge->is_hotplug_bridge)
-			return false;
+	/* Platform might know better if the bridge supports D3 */
+	if (platform_pci_bridge_d3(bridge))
+		return true;
 
-		if (dmi_check_system(bridge_d3_blacklist))
-			return false;
+	/*
+	 * Hotplug ports handled natively by the OS were not validated
+	 * by vendors for runtime D3 at least until 2018 because there
+	 * was no OS support.
+	 */
+	if (bridge->is_hotplug_bridge)
+		return false;
 
-		/*
-		 * It should be safe to put PCIe ports from 2015 or newer
-		 * to D3.
-		 */
-		if (dmi_get_bios_year() >= 2015)
-			return true;
-		break;
-	}
+	if (dmi_check_system(bridge_d3_blacklist))
+		return false;
 
-	return false;
+	/*
+	 * It should be safe to put PCIe ports from 2015 or newer
+	 * to D3.
+	 */
+	return dmi_get_bios_year() >= 2015;
 }
 
 static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
-- 
2.34.1

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

[Mario Limonciello]

Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
PCIe ports from modern machines (>2015) are allowed to be put into D3 by
storing a flag in the `struct pci_dev` structure.

pci_power_manageable() uses this flag to indicate a PCIe port can enter D3.
pci_pm_suspend_noirq() uses the return from pci_power_manageable() to
decide whether to try to put a device into its target state for a sleep
cycle via pci_prepare_to_sleep().

For devices that support D3, the target state is selected by this policy:
1. If platform_pci_power_manageable():
   Use platform_pci_choose_state()
2. If the device is armed for wakeup:
   Select the deepest D-state that supports a PME.
3. Else:
   Use D3hot.

Devices are considered power manageable by the platform when they have
one or more objects described in the table in section 7.3 of the ACPI 6.4
specification.

At suspend Linux puts PCIe root ports that are not power manageable by
the platform into D3hot. Windows only puts PCIe root ports into D3 when
they are power manageable by the platform.

The policy selected for Linux to put non-power manageable PCIe root ports
into D3hot at system suspend doesn't match anything in the PCIe or ACPI
specs.

Linux shouldn't assume PCIe root ports support D3 just because
they're on a machine newer than 2015, the ports should also be considered
power manageable by the platform.

Add an extra check for PCIe root ports to ensure D3 isn't selected for
them if they are not power-manageable through platform firmware.
This will avoid pci_pm_suspend_noirq() changing the power state
via pci_prepare_to_sleep().

The check is focused on PCIe root ports because they are part of
the platform.  Other PCIe bridges may be connected externally and thus
cannot impose platform specific limitations.

Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/07_Power_and_Performance_Mgmt/device-power-management-objects.html [1]
Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
Reported-by: Iain Lane <iain@orangesquash.org.uk>
Closes: https://forums.lenovo.com/t5/Ubuntu/Z13-can-t-resume-from-suspend-with-external-USB-keyboard/m-p/5217121
Acked-by: Rafael J. Wysocki <rafael@kernel.org>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v6->v7:
* revert back to v5 code, rewrite commit message to specific examples
  and be more generic
---
 drivers/pci/pci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f916fd76eba79..4be8c6f8f4ebe 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3041,6 +3041,14 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
 	if (dmi_check_system(bridge_d3_blacklist))
 		return false;
 
+	/*
+	 * It's not safe to put root ports that aren't power manageable
+	 * by the platform into D3.
+	 */
+	if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT &&
+	    !platform_pci_power_manageable(bridge))
+		return false;
+
 	/*
 	 * It should be safe to put PCIe ports from 2015 or newer
 	 * to D3.
-- 
2.34.1

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

[Bjorn Helgaas]

[+cc Andy, Intel MID stuff]

On Mon, Jul 10, 2023 at 07:53:25PM -0500, Mario Limonciello wrote:
> Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> PCIe ports from modern machines (>2015) are allowed to be put into D3 by
> storing a flag in the `struct pci_dev` structure.

It looks like >= 2015 (not >2015).  I think "a flag" refers to
"bridge_d3".

> pci_power_manageable() uses this flag to indicate a PCIe port can enter D3.
> pci_pm_suspend_noirq() uses the return from pci_power_manageable() to
> decide whether to try to put a device into its target state for a sleep
> cycle via pci_prepare_to_sleep().
> 
> For devices that support D3, the target state is selected by this policy:
> 1. If platform_pci_power_manageable():
>    Use platform_pci_choose_state()
> 2. If the device is armed for wakeup:
>    Select the deepest D-state that supports a PME.
> 3. Else:
>    Use D3hot.
> 
> Devices are considered power manageable by the platform when they have
> one or more objects described in the table in section 7.3 of the ACPI 6.4
> specification.

No point in citing an old version, so please cite ACPI r6.5, sec 7.3.

The spec claims we only need one object from the table for a device to
be "power-managed", but in reality, it looks like the only things that
actually *control* power are _PRx (the _ON/_OFF methods of Power
Resources) and _PSx (ironically only mentioned parenthically).

This matches up well with acpi_pci_power_manageable(), which returns
true if a device has either _PR0 or _PS0.

  Per ACPI r6.5, sec 7.3, ACPI control of device power states uses
  Power Resources (i.e., the _ON/_OFF methods of _PRx) or _PSx
  methods.  Hence acpi_pci_power_manageable() checks for the presence
  of _PR0 or _PS0.

Tangent unrelated to *this* patch: I don't know how to think about the
pci_use_mid_pm() in platform_pci_power_manageable() because I haven't
seen a MID spec.  pci_use_mid_pm() isn't dependent on "dev", so we
claim *all* PCI devices, even external ones, are power manageable by
the platform, which doesn't seem right.

> At suspend Linux puts PCIe root ports that are not power manageable by
> the platform into D3hot. Windows only puts PCIe root ports into D3 when
> they are power manageable by the platform.
> 
> The policy selected for Linux to put non-power manageable PCIe root ports
> into D3hot at system suspend doesn't match anything in the PCIe or ACPI
> specs.
> 
> Linux shouldn't assume PCIe root ports support D3 just because
> they're on a machine newer than 2015, the ports should also be considered
> power manageable by the platform.
> 
> Add an extra check for PCIe root ports to ensure D3 isn't selected for
> them if they are not power-manageable through platform firmware.
> This will avoid pci_pm_suspend_noirq() changing the power state
> via pci_prepare_to_sleep().
> 
> The check is focused on PCIe root ports because they are part of
> the platform.  Other PCIe bridges may be connected externally and thus
> cannot impose platform specific limitations.
>
> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/07_Power_and_Performance_Mgmt/device-power-management-objects.html [1]
> Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> Reported-by: Iain Lane <iain@orangesquash.org.uk>
> Closes: https://forums.lenovo.com/t5/Ubuntu/Z13-can-t-resume-from-suspend-with-external-USB-keyboard/m-p/5217121
> Acked-by: Rafael J. Wysocki <rafael@kernel.org>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v6->v7:
> * revert back to v5 code, rewrite commit message to specific examples
>   and be more generic
> ---
>  drivers/pci/pci.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index f916fd76eba79..4be8c6f8f4ebe 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3041,6 +3041,14 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>  	if (dmi_check_system(bridge_d3_blacklist))
>  		return false;
>  
> +	/*
> +	 * It's not safe to put root ports that aren't power manageable
> +	 * by the platform into D3.

Does this refer specifically to D3cold?

I assume that if we were talking about D3hot, we wouldn't need to
check for ACPI support because D3hot behavior should be fully covered
by the PCIe spec.

Let's be specific about D3hot vs D3cold whenever possible.

> +	if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT &&
> +	    !platform_pci_power_manageable(bridge))
> +		return false;

If ACPI says a device is not power-manageable, i.e., ACPI doesn't know
how to put it in D0, it makes sense to return "false" here so we don't
try to put it in D3cold.

But I don't understand the ROOT_PORT check.  We may have a Switch
described via ACPI, and the ROOT_PORT check means we can return "true"
(it's OK to use D3cold) even if the Switch Port is not power-manageable
via ACPI.

>  	/*
>  	 * It should be safe to put PCIe ports from 2015 or newer
>  	 * to D3.
> -- 
> 2.34.1
> 

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

[Mario Limonciello]

On 7/11/23 17:14, Bjorn Helgaas wrote:
> [+cc Andy, Intel MID stuff]
> 
> On Mon, Jul 10, 2023 at 07:53:25PM -0500, Mario Limonciello wrote:
>> Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>> PCIe ports from modern machines (>2015) are allowed to be put into D3 by
>> storing a flag in the `struct pci_dev` structure.
> 
> It looks like >= 2015 (not >2015).  I think "a flag" refers to
> "bridge_d3".

Yeah.

> 
>> pci_power_manageable() uses this flag to indicate a PCIe port can enter D3.
>> pci_pm_suspend_noirq() uses the return from pci_power_manageable() to
>> decide whether to try to put a device into its target state for a sleep
>> cycle via pci_prepare_to_sleep().
>>
>> For devices that support D3, the target state is selected by this policy:
>> 1. If platform_pci_power_manageable():
>>     Use platform_pci_choose_state()
>> 2. If the device is armed for wakeup:
>>     Select the deepest D-state that supports a PME.
>> 3. Else:
>>     Use D3hot.
>>
>> Devices are considered power manageable by the platform when they have
>> one or more objects described in the table in section 7.3 of the ACPI 6.4
>> specification.
> 
> No point in citing an old version, so please cite ACPI r6.5, sec 7.3.
> 
> The spec claims we only need one object from the table for a device to
> be "power-managed", but in reality, it looks like the only things that
> actually *control* power are _PRx (the _ON/_OFF methods of Power
> Resources) and _PSx (ironically only mentioned parenthically).
> 

Your point has me actually wondering if I've got this entirely wrong.

Should we perhaps be looking specifically for the presence of _SxW to 
decide if a given PCIe port can go below D0?

IE very similar to what 8133844a8f24 did but for ports that are not 
hotplug bridges.

> This matches up well with acpi_pci_power_manageable(), which returns
> true if a device has either _PR0 or _PS0.
> 
>    Per ACPI r6.5, sec 7.3, ACPI control of device power states uses
>    Power Resources (i.e., the _ON/_OFF methods of _PRx) or _PSx
>    methods.  Hence acpi_pci_power_manageable() checks for the presence
>    of _PR0 or _PS0.
> 
> Tangent unrelated to *this* patch: I don't know how to think about the
> pci_use_mid_pm() in platform_pci_power_manageable() because I haven't
> seen a MID spec.  pci_use_mid_pm() isn't dependent on "dev", so we
> claim *all* PCI devices, even external ones, are power manageable by
> the platform, which doesn't seem right.
> 
>> At suspend Linux puts PCIe root ports that are not power manageable by
>> the platform into D3hot. Windows only puts PCIe root ports into D3 when
>> they are power manageable by the platform.
>>
>> The policy selected for Linux to put non-power manageable PCIe root ports
>> into D3hot at system suspend doesn't match anything in the PCIe or ACPI
>> specs.
>>
>> Linux shouldn't assume PCIe root ports support D3 just because
>> they're on a machine newer than 2015, the ports should also be considered
>> power manageable by the platform.
>>
>> Add an extra check for PCIe root ports to ensure D3 isn't selected for
>> them if they are not power-manageable through platform firmware.
>> This will avoid pci_pm_suspend_noirq() changing the power state
>> via pci_prepare_to_sleep().
>>
>> The check is focused on PCIe root ports because they are part of
>> the platform.  Other PCIe bridges may be connected externally and thus
>> cannot impose platform specific limitations.
>>
>> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/07_Power_and_Performance_Mgmt/device-power-management-objects.html [1]

At least for myself when I respin this, here is the 6.5 link.
https://uefi.org/specs/ACPI/6.5/07_Power_and_Performance_Mgmt.html#device-power-management-objects

>> Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>> Reported-by: Iain Lane <iain@orangesquash.org.uk>
>> Closes: https://forums.lenovo.com/t5/Ubuntu/Z13-can-t-resume-from-suspend-with-external-USB-keyboard/m-p/5217121
>> Acked-by: Rafael J. Wysocki <rafael@kernel.org>
>> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> v6->v7:
>> * revert back to v5 code, rewrite commit message to specific examples
>>    and be more generic
>> ---
>>   drivers/pci/pci.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index f916fd76eba79..4be8c6f8f4ebe 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -3041,6 +3041,14 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>>   	if (dmi_check_system(bridge_d3_blacklist))
>>   		return false;
>>   
>> +	/*
>> +	 * It's not safe to put root ports that aren't power manageable
>> +	 * by the platform into D3.
> 
> Does this refer specifically to D3cold?
> 

No, it's intentionally not saying D3hot or D3cold because it's stored to 
"bridge_d3" which is used for both D3hot and D3cold.

> I assume that if we were talking about D3hot, we wouldn't need to
> check for ACPI support because D3hot behavior should be fully covered
> by the PCIe spec.

Right; the PCIe spec indicates that D3hot should be supported by all 
devices and has rules about when you can go into D3hot like not allowing 
it unless child devices are already in D3.

Naïvely you would think that means pci_bridge_d3_possible() shouldn't 
have any of these checks, but they've all obviously all been grown for a 
reason.

The value from pci_bridge_d3_possible() is used both "at suspend" and 
"runtime", but what we're really talking with this issue is is the 
behavior "at suspend".

> 
> Let's be specific about D3hot vs D3cold whenever possible.
> 
>> +	if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT &&
>> +	    !platform_pci_power_manageable(bridge))
>> +		return false;
> 
> If ACPI says a device is not power-manageable, i.e., ACPI doesn't know
> how to put it in D0, it makes sense to return "false" here so we don't
> try to put it in D3cold.
> 
> But I don't understand the ROOT_PORT check.  We may have a Switch
> described via ACPI, and the ROOT_PORT check means we can return "true"
> (it's OK to use D3cold) even if the Switch Port is not power-manageable
> via ACPI.

This feels a lot more of a potential to cause regressions.

Something we could do is include the value for bridge->untrusted in the 
decision, but I'm not convinced that's correct.

Another option can be to merge a series of changes like this:

1) My v5 patch that adds the quirks for the two known broken machines
2) Patch 1/2 from v7
3) Patch 2/2 from v7
4) Another patch to drop the root port check here

#1 could go to 6.5-rcX as it's riskless.  #2-4 could go to linux-next 
and if they work out not to cause any problems we could revert #1.

If they cause problems we come back to the drawing table to find the
right balance.

> 
>>   	/*
>>   	 * It should be safe to put PCIe ports from 2015 or newer
>>   	 * to D3.
>> -- 
>> 2.34.1
>>

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

[Rafael J. Wysocki]

On Wed, Jul 12, 2023 at 12:14 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Andy, Intel MID stuff]
>
> On Mon, Jul 10, 2023 at 07:53:25PM -0500, Mario Limonciello wrote:
> > Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> > PCIe ports from modern machines (>2015) are allowed to be put into D3 by
> > storing a flag in the `struct pci_dev` structure.
>
> It looks like >= 2015 (not >2015).  I think "a flag" refers to
> "bridge_d3".
>
> > pci_power_manageable() uses this flag to indicate a PCIe port can enter D3.
> > pci_pm_suspend_noirq() uses the return from pci_power_manageable() to
> > decide whether to try to put a device into its target state for a sleep
> > cycle via pci_prepare_to_sleep().
> >
> > For devices that support D3, the target state is selected by this policy:
> > 1. If platform_pci_power_manageable():
> >    Use platform_pci_choose_state()
> > 2. If the device is armed for wakeup:
> >    Select the deepest D-state that supports a PME.
> > 3. Else:
> >    Use D3hot.
> >
> > Devices are considered power manageable by the platform when they have
> > one or more objects described in the table in section 7.3 of the ACPI 6.4
> > specification.
>
> No point in citing an old version, so please cite ACPI r6.5, sec 7.3.
>
> The spec claims we only need one object from the table for a device to
> be "power-managed", but in reality, it looks like the only things that
> actually *control* power are _PRx (the _ON/_OFF methods of Power
> Resources) and _PSx (ironically only mentioned parenthically).
>
> This matches up well with acpi_pci_power_manageable(), which returns
> true if a device has either _PR0 or _PS0.
>
>   Per ACPI r6.5, sec 7.3, ACPI control of device power states uses
>   Power Resources (i.e., the _ON/_OFF methods of _PRx) or _PSx
>   methods.  Hence acpi_pci_power_manageable() checks for the presence
>   of _PR0 or _PS0.
>
> Tangent unrelated to *this* patch: I don't know how to think about the
> pci_use_mid_pm() in platform_pci_power_manageable() because I haven't
> seen a MID spec.  pci_use_mid_pm() isn't dependent on "dev", so we
> claim *all* PCI devices, even external ones, are power manageable by
> the platform, which doesn't seem right.

No, we don't.

This only means that PCI devices may be power manageable by the
platform and so the platform code should be invoked to check that.
AFAICS, intel_mid_pwr_get_lss_id(() will return an error for a device
without platform PM support.

> > At suspend Linux puts PCIe root ports that are not power manageable by
> > the platform into D3hot. Windows only puts PCIe root ports into D3 when
> > they are power manageable by the platform.
> >
> > The policy selected for Linux to put non-power manageable PCIe root ports
> > into D3hot at system suspend doesn't match anything in the PCIe or ACPI
> > specs.
> >
> > Linux shouldn't assume PCIe root ports support D3 just because
> > they're on a machine newer than 2015, the ports should also be considered
> > power manageable by the platform.
> >
> > Add an extra check for PCIe root ports to ensure D3 isn't selected for
> > them if they are not power-manageable through platform firmware.
> > This will avoid pci_pm_suspend_noirq() changing the power state
> > via pci_prepare_to_sleep().
> >
> > The check is focused on PCIe root ports because they are part of
> > the platform.  Other PCIe bridges may be connected externally and thus
> > cannot impose platform specific limitations.
> >
> > Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/07_Power_and_Performance_Mgmt/device-power-management-objects.html [1]
> > Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> > Reported-by: Iain Lane <iain@orangesquash.org.uk>
> > Closes: https://forums.lenovo.com/t5/Ubuntu/Z13-can-t-resume-from-suspend-with-external-USB-keyboard/m-p/5217121
> > Acked-by: Rafael J. Wysocki <rafael@kernel.org>
> > Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > ---
> > v6->v7:
> > * revert back to v5 code, rewrite commit message to specific examples
> >   and be more generic
> > ---
> >  drivers/pci/pci.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index f916fd76eba79..4be8c6f8f4ebe 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -3041,6 +3041,14 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
> >       if (dmi_check_system(bridge_d3_blacklist))
> >               return false;
> >
> > +     /*
> > +      * It's not safe to put root ports that aren't power manageable
> > +      * by the platform into D3.
>
> Does this refer specifically to D3cold?
>
> I assume that if we were talking about D3hot, we wouldn't need to
> check for ACPI support because D3hot behavior should be fully covered
> by the PCIe spec.
>
> Let's be specific about D3hot vs D3cold whenever possible.

Amen.

However, even though by the PCIe spec it should be possible to program
PCIe ports without ACPI PM support into D3hot via PMCSR, I'm not
actually sure how that works in practice, especially as far as PCIe
Root Ports are concerned.

Hardware designs usually don't allow Root Ports to be power managed
individually, so I suppose that programming them into D3hot (or D1 or
D2 for that matter) could be treated by the Host Bridge as dropping
references to them or something similar and I can imagine that this
may not work on some platforms and so maybe it should be avoided in
general.

When there is ACPI PM support, though, it can at least be assumed that
the platform designer has taken Root Port D3hot into account.

> > +     if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT &&
> > +         !platform_pci_power_manageable(bridge))
> > +             return false;
>
> If ACPI says a device is not power-manageable, i.e., ACPI doesn't know
> how to put it in D0, it makes sense to return "false" here so we don't
> try to put it in D3cold.
>
> But I don't understand the ROOT_PORT check.  We may have a Switch
> described via ACPI, and the ROOT_PORT check means we can return "true"
> (it's OK to use D3cold) even if the Switch Port is not power-manageable
> via ACPI.

My understanding is that it is related to the remark above: It is
generally unclear how Root Port power management without ACPI support
is supposed to work, so they are kind of a special case.

> >       /*
> >        * It should be safe to put PCIe ports from 2015 or newer
> >        * to D3.
> > --
> > 2.34.1
> >

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

[Rafael J. Wysocki]

On Wed, Jul 12, 2023 at 12:54 AM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 7/11/23 17:14, Bjorn Helgaas wrote:
> > [+cc Andy, Intel MID stuff]
> >
> > On Mon, Jul 10, 2023 at 07:53:25PM -0500, Mario Limonciello wrote:
> >> Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> >> PCIe ports from modern machines (>2015) are allowed to be put into D3 by
> >> storing a flag in the `struct pci_dev` structure.
> >
> > It looks like >= 2015 (not >2015).  I think "a flag" refers to
> > "bridge_d3".
>
> Yeah.
>
> >
> >> pci_power_manageable() uses this flag to indicate a PCIe port can enter D3.
> >> pci_pm_suspend_noirq() uses the return from pci_power_manageable() to
> >> decide whether to try to put a device into its target state for a sleep
> >> cycle via pci_prepare_to_sleep().
> >>
> >> For devices that support D3, the target state is selected by this policy:
> >> 1. If platform_pci_power_manageable():
> >>     Use platform_pci_choose_state()
> >> 2. If the device is armed for wakeup:
> >>     Select the deepest D-state that supports a PME.
> >> 3. Else:
> >>     Use D3hot.
> >>
> >> Devices are considered power manageable by the platform when they have
> >> one or more objects described in the table in section 7.3 of the ACPI 6.4
> >> specification.
> >
> > No point in citing an old version, so please cite ACPI r6.5, sec 7.3.
> >
> > The spec claims we only need one object from the table for a device to
> > be "power-managed", but in reality, it looks like the only things that
> > actually *control* power are _PRx (the _ON/_OFF methods of Power
> > Resources) and _PSx (ironically only mentioned parenthically).
> >
>
> Your point has me actually wondering if I've got this entirely wrong.
>
> Should we perhaps be looking specifically for the presence of _SxW to
> decide if a given PCIe port can go below D0?

There are two things, _SxW and _SxD, and they shouldn't be confused.

_SxW tells you what the deepest power state from which wakeup can be
signaled by the device (in the given Sx state of the system) is.

_SxD tells you what the deepest power state supported by the device
(in the given Sx state of the system) is.

And note that _SxW is applicable to the device itself, not the
subordinate devices, so I'm not sure how meaningful it is for ports.

pci_target_state() uses both _SxW and _SxD to determine the deepest
state the device can go into and so long as it is used properly, it
shouldn't return a power state that is too deep, so I'm not really
sure why you want this special "should the bridge be allowed to go
into D3hot/cold" routine to double check this.

> IE very similar to what 8133844a8f24 did but for ports that are not
> hotplug bridges.
>
> > This matches up well with acpi_pci_power_manageable(), which returns
> > true if a device has either _PR0 or _PS0.
> >
> >    Per ACPI r6.5, sec 7.3, ACPI control of device power states uses
> >    Power Resources (i.e., the _ON/_OFF methods of _PRx) or _PSx
> >    methods.  Hence acpi_pci_power_manageable() checks for the presence
> >    of _PR0 or _PS0.
> >
> > Tangent unrelated to *this* patch: I don't know how to think about the
> > pci_use_mid_pm() in platform_pci_power_manageable() because I haven't
> > seen a MID spec.  pci_use_mid_pm() isn't dependent on "dev", so we
> > claim *all* PCI devices, even external ones, are power manageable by
> > the platform, which doesn't seem right.
> >
> >> At suspend Linux puts PCIe root ports that are not power manageable by
> >> the platform into D3hot. Windows only puts PCIe root ports into D3 when
> >> they are power manageable by the platform.
> >>
> >> The policy selected for Linux to put non-power manageable PCIe root ports
> >> into D3hot at system suspend doesn't match anything in the PCIe or ACPI
> >> specs.
> >>
> >> Linux shouldn't assume PCIe root ports support D3 just because
> >> they're on a machine newer than 2015, the ports should also be considered
> >> power manageable by the platform.
> >>
> >> Add an extra check for PCIe root ports to ensure D3 isn't selected for
> >> them if they are not power-manageable through platform firmware.
> >> This will avoid pci_pm_suspend_noirq() changing the power state
> >> via pci_prepare_to_sleep().
> >>
> >> The check is focused on PCIe root ports because they are part of
> >> the platform.  Other PCIe bridges may be connected externally and thus
> >> cannot impose platform specific limitations.
> >>
> >> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/07_Power_and_Performance_Mgmt/device-power-management-objects.html [1]
>
> At least for myself when I respin this, here is the 6.5 link.
> https://uefi.org/specs/ACPI/6.5/07_Power_and_Performance_Mgmt.html#device-power-management-objects
>
> >> Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> >> Reported-by: Iain Lane <iain@orangesquash.org.uk>
> >> Closes: https://forums.lenovo.com/t5/Ubuntu/Z13-can-t-resume-from-suspend-with-external-USB-keyboard/m-p/5217121
> >> Acked-by: Rafael J. Wysocki <rafael@kernel.org>
> >> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >> ---
> >> v6->v7:
> >> * revert back to v5 code, rewrite commit message to specific examples
> >>    and be more generic
> >> ---
> >>   drivers/pci/pci.c | 8 ++++++++
> >>   1 file changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >> index f916fd76eba79..4be8c6f8f4ebe 100644
> >> --- a/drivers/pci/pci.c
> >> +++ b/drivers/pci/pci.c
> >> @@ -3041,6 +3041,14 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
> >>      if (dmi_check_system(bridge_d3_blacklist))
> >>              return false;
> >>
> >> +    /*
> >> +     * It's not safe to put root ports that aren't power manageable
> >> +     * by the platform into D3.
> >
> > Does this refer specifically to D3cold?
> >
>
> No, it's intentionally not saying D3hot or D3cold because it's stored to
> "bridge_d3" which is used for both D3hot and D3cold.
>
> > I assume that if we were talking about D3hot, we wouldn't need to
> > check for ACPI support because D3hot behavior should be fully covered
> > by the PCIe spec.
>
> Right; the PCIe spec indicates that D3hot should be supported by all
> devices and has rules about when you can go into D3hot like not allowing
> it unless child devices are already in D3.
>
> Naïvely you would think that means pci_bridge_d3_possible() shouldn't
> have any of these checks, but they've all obviously all been grown for a
> reason.
>
> The value from pci_bridge_d3_possible() is used both "at suspend" and
> "runtime", but what we're really talking with this issue is is the
> behavior "at suspend".

Which is suspend-to-idle AFAICS, so from the ACPI perspective it is
all S0 anyway.

> >
> > Let's be specific about D3hot vs D3cold whenever possible.
> >
> >> +    if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT &&
> >> +        !platform_pci_power_manageable(bridge))
> >> +            return false;
> >
> > If ACPI says a device is not power-manageable, i.e., ACPI doesn't know
> > how to put it in D0, it makes sense to return "false" here so we don't
> > try to put it in D3cold.
> >
> > But I don't understand the ROOT_PORT check.  We may have a Switch
> > described via ACPI, and the ROOT_PORT check means we can return "true"
> > (it's OK to use D3cold) even if the Switch Port is not power-manageable
> > via ACPI.
>
> This feels a lot more of a potential to cause regressions.
>
> Something we could do is include the value for bridge->untrusted in the
> decision, but I'm not convinced that's correct.
>
> Another option can be to merge a series of changes like this:
>
> 1) My v5 patch that adds the quirks for the two known broken machines
> 2) Patch 1/2 from v7
> 3) Patch 2/2 from v7
> 4) Another patch to drop the root port check here
>
> #1 could go to 6.5-rcX as it's riskless.  #2-4 could go to linux-next
> and if they work out not to cause any problems we could revert #1.
>
> If they cause problems we come back to the drawing table to find the
> right balance.

Generally speaking, pci_bridge_d3_possible() is there to prevent
bridges (and PCIe ports in particular) from being put into D3hot/cold
if there are reasons to believe that it may not work.
acpi_pci_bridge_d3() is part of that.

Even if it returns 'true', the _SxD/_SxW check should still be applied
via pci_target_state() to determine whether or not the firmware allows
this particular bridge to go into D3hot/cold.  So arguably, the _SxW
check in acpi_pci_bridge_d3() should not be necessary and if it makes
any functional difference, there is a bug somewhere else.

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

[Andy Shevchenko]

On Wed, Jul 12, 2023 at 01:48:11PM +0200, Rafael J. Wysocki wrote:
> On Wed, Jul 12, 2023 at 12:14 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Mon, Jul 10, 2023 at 07:53:25PM -0500, Mario Limonciello wrote:

...

> > Tangent unrelated to *this* patch: I don't know how to think about the
> > pci_use_mid_pm() in platform_pci_power_manageable() because I haven't
> > seen a MID spec.  pci_use_mid_pm() isn't dependent on "dev", so we
> > claim *all* PCI devices, even external ones, are power manageable by
> > the platform, which doesn't seem right.
> 
> No, we don't.
> 
> This only means that PCI devices may be power manageable by the
> platform and so the platform code should be invoked to check that.
> AFAICS, intel_mid_pwr_get_lss_id(() will return an error for a device
> without platform PM support.

If it's a problem somewhere, we may even harden that by checking
the bus nr to be 0. The devices outside bus 0 for sure have not to
be affected by this code.

-- 
With Best Regards,
Andy Shevchenko

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

[Limonciello, Mario]

On 7/12/2023 07:13, Rafael J. Wysocki wrote:
> On Wed, Jul 12, 2023 at 12:54 AM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> On 7/11/23 17:14, Bjorn Helgaas wrote:
>>> [+cc Andy, Intel MID stuff]
>>>
>>> On Mon, Jul 10, 2023 at 07:53:25PM -0500, Mario Limonciello wrote:
>>>> Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>>>> PCIe ports from modern machines (>2015) are allowed to be put into D3 by
>>>> storing a flag in the `struct pci_dev` structure.
>>>
>>> It looks like >= 2015 (not >2015).  I think "a flag" refers to
>>> "bridge_d3".
>>
>> Yeah.
>>
>>>
>>>> pci_power_manageable() uses this flag to indicate a PCIe port can enter D3.
>>>> pci_pm_suspend_noirq() uses the return from pci_power_manageable() to
>>>> decide whether to try to put a device into its target state for a sleep
>>>> cycle via pci_prepare_to_sleep().
>>>>
>>>> For devices that support D3, the target state is selected by this policy:
>>>> 1. If platform_pci_power_manageable():
>>>>      Use platform_pci_choose_state()
>>>> 2. If the device is armed for wakeup:
>>>>      Select the deepest D-state that supports a PME.
>>>> 3. Else:
>>>>      Use D3hot.
>>>>
>>>> Devices are considered power manageable by the platform when they have
>>>> one or more objects described in the table in section 7.3 of the ACPI 6.4
>>>> specification.
>>>
>>> No point in citing an old version, so please cite ACPI r6.5, sec 7.3.
>>>
>>> The spec claims we only need one object from the table for a device to
>>> be "power-managed", but in reality, it looks like the only things that
>>> actually *control* power are _PRx (the _ON/_OFF methods of Power
>>> Resources) and _PSx (ironically only mentioned parenthically).
>>>
>>
>> Your point has me actually wondering if I've got this entirely wrong.
>>
>> Should we perhaps be looking specifically for the presence of _SxW to
>> decide if a given PCIe port can go below D0?
> 
> There are two things, _SxW and _SxD, and they shouldn't be confused.
> 
> _SxW tells you what the deepest power state from which wakeup can be
> signaled by the device (in the given Sx state of the system) is.
> 
> _SxD tells you what the deepest power state supported by the device
> (in the given Sx state of the system) is.
> 
> And note that _SxW is applicable to the device itself, not the
> subordinate devices, so I'm not sure how meaningful it is for ports.
> 
> pci_target_state() uses both _SxW and _SxD to determine the deepest
> state the device can go into and so long as it is used properly, it
> shouldn't return a power state that is too deep, so I'm not really
> sure why you want this special "should the bridge be allowed to go
> into D3hot/cold" routine to double check this.

Because pci_target_state only looks at _SxW and _SxD "if" the PCI device 
is power manageable by ACPI.  That's why this change is injecting that 
extra check in.

> 
>> IE very similar to what 8133844a8f24 did but for ports that are not
>> hotplug bridges.
>>
>>> This matches up well with acpi_pci_power_manageable(), which returns
>>> true if a device has either _PR0 or _PS0.
>>>
>>>     Per ACPI r6.5, sec 7.3, ACPI control of device power states uses
>>>     Power Resources (i.e., the _ON/_OFF methods of _PRx) or _PSx
>>>     methods.  Hence acpi_pci_power_manageable() checks for the presence
>>>     of _PR0 or _PS0.
>>>
>>> Tangent unrelated to *this* patch: I don't know how to think about the
>>> pci_use_mid_pm() in platform_pci_power_manageable() because I haven't
>>> seen a MID spec.  pci_use_mid_pm() isn't dependent on "dev", so we
>>> claim *all* PCI devices, even external ones, are power manageable by
>>> the platform, which doesn't seem right.
>>>
>>>> At suspend Linux puts PCIe root ports that are not power manageable by
>>>> the platform into D3hot. Windows only puts PCIe root ports into D3 when
>>>> they are power manageable by the platform.
>>>>
>>>> The policy selected for Linux to put non-power manageable PCIe root ports
>>>> into D3hot at system suspend doesn't match anything in the PCIe or ACPI
>>>> specs.
>>>>
>>>> Linux shouldn't assume PCIe root ports support D3 just because
>>>> they're on a machine newer than 2015, the ports should also be considered
>>>> power manageable by the platform.
>>>>
>>>> Add an extra check for PCIe root ports to ensure D3 isn't selected for
>>>> them if they are not power-manageable through platform firmware.
>>>> This will avoid pci_pm_suspend_noirq() changing the power state
>>>> via pci_prepare_to_sleep().
>>>>
>>>> The check is focused on PCIe root ports because they are part of
>>>> the platform.  Other PCIe bridges may be connected externally and thus
>>>> cannot impose platform specific limitations.
>>>>
>>>> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/07_Power_and_Performance_Mgmt/device-power-management-objects.html [1]
>>
>> At least for myself when I respin this, here is the 6.5 link.
>> https://uefi.org/specs/ACPI/6.5/07_Power_and_Performance_Mgmt.html#device-power-management-objects
>>
>>>> Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>>>> Reported-by: Iain Lane <iain@orangesquash.org.uk>
>>>> Closes: https://forums.lenovo.com/t5/Ubuntu/Z13-can-t-resume-from-suspend-with-external-USB-keyboard/m-p/5217121
>>>> Acked-by: Rafael J. Wysocki <rafael@kernel.org>
>>>> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>> v6->v7:
>>>> * revert back to v5 code, rewrite commit message to specific examples
>>>>     and be more generic
>>>> ---
>>>>    drivers/pci/pci.c | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index f916fd76eba79..4be8c6f8f4ebe 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -3041,6 +3041,14 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>>>>       if (dmi_check_system(bridge_d3_blacklist))
>>>>               return false;
>>>>
>>>> +    /*
>>>> +     * It's not safe to put root ports that aren't power manageable
>>>> +     * by the platform into D3.
>>>
>>> Does this refer specifically to D3cold?
>>>
>>
>> No, it's intentionally not saying D3hot or D3cold because it's stored to
>> "bridge_d3" which is used for both D3hot and D3cold.
>>
>>> I assume that if we were talking about D3hot, we wouldn't need to
>>> check for ACPI support because D3hot behavior should be fully covered
>>> by the PCIe spec.
>>
>> Right; the PCIe spec indicates that D3hot should be supported by all
>> devices and has rules about when you can go into D3hot like not allowing
>> it unless child devices are already in D3.
>>
>> Naïvely you would think that means pci_bridge_d3_possible() shouldn't
>> have any of these checks, but they've all obviously all been grown for a
>> reason.
>>
>> The value from pci_bridge_d3_possible() is used both "at suspend" and
>> "runtime", but what we're really talking with this issue is is the
>> behavior "at suspend".
> 
> Which is suspend-to-idle AFAICS, so from the ACPI perspective it is
> all S0 anyway.
> 
>>>
>>> Let's be specific about D3hot vs D3cold whenever possible.
>>>
>>>> +    if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT &&
>>>> +        !platform_pci_power_manageable(bridge))
>>>> +            return false;
>>>
>>> If ACPI says a device is not power-manageable, i.e., ACPI doesn't know
>>> how to put it in D0, it makes sense to return "false" here so we don't
>>> try to put it in D3cold.
>>>
>>> But I don't understand the ROOT_PORT check.  We may have a Switch
>>> described via ACPI, and the ROOT_PORT check means we can return "true"
>>> (it's OK to use D3cold) even if the Switch Port is not power-manageable
>>> via ACPI.
>>
>> This feels a lot more of a potential to cause regressions.
>>
>> Something we could do is include the value for bridge->untrusted in the
>> decision, but I'm not convinced that's correct.
>>
>> Another option can be to merge a series of changes like this:
>>
>> 1) My v5 patch that adds the quirks for the two known broken machines
>> 2) Patch 1/2 from v7
>> 3) Patch 2/2 from v7
>> 4) Another patch to drop the root port check here
>>
>> #1 could go to 6.5-rcX as it's riskless.  #2-4 could go to linux-next
>> and if they work out not to cause any problems we could revert #1.
>>
>> If they cause problems we come back to the drawing table to find the
>> right balance.
> 
> Generally speaking, pci_bridge_d3_possible() is there to prevent
> bridges (and PCIe ports in particular) from being put into D3hot/cold
> if there are reasons to believe that it may not work.
> acpi_pci_bridge_d3() is part of that.
> 
> Even if it returns 'true', the _SxD/_SxW check should still be applied
> via pci_target_state() to determine whether or not the firmware allows
> this particular bridge to go into D3hot/cold.  So arguably, the _SxW
> check in acpi_pci_bridge_d3() should not be necessary and if it makes
> any functional difference, there is a bug somewhere else.

But only if it was power manageable would the _SxD/_SxW check be 
applied.  This issue is around the branch of pci_target_state() where
it's not power manageable and so it uses PME or it falls back to D3hot.

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

[Rafael J. Wysocki]

On Wed, Jul 12, 2023 at 6:09 PM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
> On 7/12/2023 07:13, Rafael J. Wysocki wrote:
> > On Wed, Jul 12, 2023 at 12:54 AM Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> >>
> >> On 7/11/23 17:14, Bjorn Helgaas wrote:
> >>> [+cc Andy, Intel MID stuff]
> >>>
> >>> On Mon, Jul 10, 2023 at 07:53:25PM -0500, Mario Limonciello wrote:
> >>>> Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> >>>> PCIe ports from modern machines (>2015) are allowed to be put into D3 by
> >>>> storing a flag in the `struct pci_dev` structure.
> >>>
> >>> It looks like >= 2015 (not >2015).  I think "a flag" refers to
> >>> "bridge_d3".
> >>
> >> Yeah.
> >>
> >>>
> >>>> pci_power_manageable() uses this flag to indicate a PCIe port can enter D3.
> >>>> pci_pm_suspend_noirq() uses the return from pci_power_manageable() to
> >>>> decide whether to try to put a device into its target state for a sleep
> >>>> cycle via pci_prepare_to_sleep().
> >>>>
> >>>> For devices that support D3, the target state is selected by this policy:
> >>>> 1. If platform_pci_power_manageable():
> >>>>      Use platform_pci_choose_state()
> >>>> 2. If the device is armed for wakeup:
> >>>>      Select the deepest D-state that supports a PME.
> >>>> 3. Else:
> >>>>      Use D3hot.
> >>>>
> >>>> Devices are considered power manageable by the platform when they have
> >>>> one or more objects described in the table in section 7.3 of the ACPI 6.4
> >>>> specification.
> >>>
> >>> No point in citing an old version, so please cite ACPI r6.5, sec 7.3.
> >>>
> >>> The spec claims we only need one object from the table for a device to
> >>> be "power-managed", but in reality, it looks like the only things that
> >>> actually *control* power are _PRx (the _ON/_OFF methods of Power
> >>> Resources) and _PSx (ironically only mentioned parenthically).
> >>>
> >>
> >> Your point has me actually wondering if I've got this entirely wrong.
> >>
> >> Should we perhaps be looking specifically for the presence of _SxW to
> >> decide if a given PCIe port can go below D0?
> >
> > There are two things, _SxW and _SxD, and they shouldn't be confused.
> >
> > _SxW tells you what the deepest power state from which wakeup can be
> > signaled by the device (in the given Sx state of the system) is.
> >
> > _SxD tells you what the deepest power state supported by the device
> > (in the given Sx state of the system) is.
> >
> > And note that _SxW is applicable to the device itself, not the
> > subordinate devices, so I'm not sure how meaningful it is for ports.
> >
> > pci_target_state() uses both _SxW and _SxD to determine the deepest
> > state the device can go into and so long as it is used properly, it
> > shouldn't return a power state that is too deep, so I'm not really
> > sure why you want this special "should the bridge be allowed to go
> > into D3hot/cold" routine to double check this.
>
> Because pci_target_state only looks at _SxW and _SxD "if" the PCI device
> is power manageable by ACPI.  That's why this change is injecting that
> extra check in.

I see.  We seem to be getting to the bottom of the problem.

[cut]

> >
> > Generally speaking, pci_bridge_d3_possible() is there to prevent
> > bridges (and PCIe ports in particular) from being put into D3hot/cold
> > if there are reasons to believe that it may not work.
> > acpi_pci_bridge_d3() is part of that.
> >
> > Even if it returns 'true', the _SxD/_SxW check should still be applied
> > via pci_target_state() to determine whether or not the firmware allows
> > this particular bridge to go into D3hot/cold.  So arguably, the _SxW
> > check in acpi_pci_bridge_d3() should not be necessary and if it makes
> > any functional difference, there is a bug somewhere else.
>
> But only if it was power manageable would the _SxD/_SxW check be
> applied.  This issue is around the branch of pci_target_state() where
> it's not power manageable and so it uses PME or it falls back to D3hot.

Well, this looks like a spec interpretation difference.

We thought that _SxD/_SxW would only be relevant for devices with ACPI
PM support, but the firmware people seem to think that those objects
are also relevant for PCI devices that don't have ACPI PM support
(because those devices are still power-manageable via PMCSR).  If
Windows agrees with that viewpoint, we'll need to adjust, but not
through adding _SxW checks in random places.

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

[Limonciello, Mario]

On 7/14/2023 2:17 PM, Rafael J. Wysocki wrote:
>>> Generally speaking, pci_bridge_d3_possible() is there to prevent
>>> bridges (and PCIe ports in particular) from being put into D3hot/cold
>>> if there are reasons to believe that it may not work.
>>> acpi_pci_bridge_d3() is part of that.
>>>
>>> Even if it returns 'true', the _SxD/_SxW check should still be applied
>>> via pci_target_state() to determine whether or not the firmware allows
>>> this particular bridge to go into D3hot/cold.  So arguably, the _SxW
>>> check in acpi_pci_bridge_d3() should not be necessary and if it makes
>>> any functional difference, there is a bug somewhere else.
>> But only if it was power manageable would the _SxD/_SxW check be
>> applied.  This issue is around the branch of pci_target_state() where
>> it's not power manageable and so it uses PME or it falls back to D3hot.
> Well, this looks like a spec interpretation difference.
>
> We thought that _SxD/_SxW would only be relevant for devices with ACPI
> PM support, but the firmware people seem to think that those objects
> are also relevant for PCI devices that don't have ACPI PM support
> (because those devices are still power-manageable via PMCSR).  If
> Windows agrees with that viewpoint, we'll need to adjust, but not
> through adding _SxW checks in random places.
I think that depends upon how you want to handle the lack of _S0W.

On these problematic devices there is no _S0W under the PCIe
root port.  As I said; Windows puts them into D0 in this case though.

So acpi_dev_power_state_for_wake should return ACPI_STATE_UNKNOWN.

Can you suggest where you think adding a acpi_dev_power_state_for_wake() 
does make sense?

Two areas that I think would work would be in: pci_pm_suspend_noirq() 
(to avoid calling pci_prepare_to_sleep)

or

directly in pci_prepare_to_sleep() to check that value in lieu of 
pci_target_state().

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

[Mario Limonciello]

On 7/14/23 19:46, Limonciello, Mario wrote:
> 
> On 7/14/2023 2:17 PM, Rafael J. Wysocki wrote:
>>>> Generally speaking, pci_bridge_d3_possible() is there to prevent
>>>> bridges (and PCIe ports in particular) from being put into D3hot/cold
>>>> if there are reasons to believe that it may not work.
>>>> acpi_pci_bridge_d3() is part of that.
>>>>
>>>> Even if it returns 'true', the _SxD/_SxW check should still be applied
>>>> via pci_target_state() to determine whether or not the firmware allows
>>>> this particular bridge to go into D3hot/cold.  So arguably, the _SxW
>>>> check in acpi_pci_bridge_d3() should not be necessary and if it makes
>>>> any functional difference, there is a bug somewhere else.
>>> But only if it was power manageable would the _SxD/_SxW check be
>>> applied.  This issue is around the branch of pci_target_state() where
>>> it's not power manageable and so it uses PME or it falls back to D3hot.
>> Well, this looks like a spec interpretation difference.
>>
>> We thought that _SxD/_SxW would only be relevant for devices with ACPI
>> PM support, but the firmware people seem to think that those objects
>> are also relevant for PCI devices that don't have ACPI PM support
>> (because those devices are still power-manageable via PMCSR).  If
>> Windows agrees with that viewpoint, we'll need to adjust, but not
>> through adding _SxW checks in random places.
> I think that depends upon how you want to handle the lack of _S0W.
> 
> On these problematic devices there is no _S0W under the PCIe
> root port.  As I said; Windows puts them into D0 in this case though.
> 
> So acpi_dev_power_state_for_wake should return ACPI_STATE_UNKNOWN.
> 
> Can you suggest where you think adding a acpi_dev_power_state_for_wake() 
> does make sense?
> 
> Two areas that I think would work would be in: pci_pm_suspend_noirq() 
> (to avoid calling pci_prepare_to_sleep)
> 
> or
> 
> directly in pci_prepare_to_sleep() to check that value in lieu of 
> pci_target_state().
> 

Rafael,

Did you have any more thoughts on this?

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

[Rafael J. Wysocki]

On Sat, Jul 15, 2023 at 1:00 AM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
>
> On 7/14/2023 2:17 PM, Rafael J. Wysocki wrote:
>
> Well, this looks like a spec interpretation difference.
>
> We thought that _SxD/_SxW would only be relevant for devices with ACPI
> PM support, but the firmware people seem to think that those objects
> are also relevant for PCI devices that don't have ACPI PM support
> (because those devices are still power-manageable via PMCSR).  If
> Windows agrees with that viewpoint, we'll need to adjust, but not
> through adding _SxW checks in random places.
>
> I think that depends upon how you want to handle the lack of _S0W.

If _S0W is not present, _S0D should return the deepest state that can be used.

If that is not present, it is a matter of OS policy.

> On these problematic devices there is no _S0W under the PCIe
> root port.  As I said; Windows puts them into D0 in this case though.

Do you know what the rationale for that is?  Maybe Windows takes the
lack of _S0W/_S0D as the indication that the device could not go into
low-power states in D0, but do you actually know that this is the
case?

Surely, for non-bridge devices the lack of _S0W/_S0D does not mean
that the device should not be programmed into low-power states via
PMCSR, but maybe Root Ports are an exception?

> So acpi_dev_power_state_for_wake should return ACPI_STATE_UNKNOWN.

And then who'll decide what to do with that return value?

> Can you suggest where you think adding a acpi_dev_power_state_for_wake() does make sense?
>
> Two areas that I think would work would be in: pci_pm_suspend_noirq() (to avoid calling pci_prepare_to_sleep)
>
> or
>
> directly in pci_prepare_to_sleep() to check that value in lieu of pci_target_state().

I'm not sure that this is a core problem TBH.  It looks like this is
an exception made specifically for ports, so this check seems to
belong to where ports are handled, so that would be
acpi_pci_bridge_d3() after all.

However, _S0D needs to be checked too when _S0W is not present.

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

[Rafael J. Wysocki]

On Tue, Aug 1, 2023 at 5:25 AM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 7/14/23 19:46, Limonciello, Mario wrote:
> >
> > On 7/14/2023 2:17 PM, Rafael J. Wysocki wrote:
> >>>> Generally speaking, pci_bridge_d3_possible() is there to prevent
> >>>> bridges (and PCIe ports in particular) from being put into D3hot/cold
> >>>> if there are reasons to believe that it may not work.
> >>>> acpi_pci_bridge_d3() is part of that.
> >>>>
> >>>> Even if it returns 'true', the _SxD/_SxW check should still be applied
> >>>> via pci_target_state() to determine whether or not the firmware allows
> >>>> this particular bridge to go into D3hot/cold.  So arguably, the _SxW
> >>>> check in acpi_pci_bridge_d3() should not be necessary and if it makes
> >>>> any functional difference, there is a bug somewhere else.
> >>> But only if it was power manageable would the _SxD/_SxW check be
> >>> applied.  This issue is around the branch of pci_target_state() where
> >>> it's not power manageable and so it uses PME or it falls back to D3hot.
> >> Well, this looks like a spec interpretation difference.
> >>
> >> We thought that _SxD/_SxW would only be relevant for devices with ACPI
> >> PM support, but the firmware people seem to think that those objects
> >> are also relevant for PCI devices that don't have ACPI PM support
> >> (because those devices are still power-manageable via PMCSR).  If
> >> Windows agrees with that viewpoint, we'll need to adjust, but not
> >> through adding _SxW checks in random places.
> > I think that depends upon how you want to handle the lack of _S0W.
> >
> > On these problematic devices there is no _S0W under the PCIe
> > root port.  As I said; Windows puts them into D0 in this case though.
> >
> > So acpi_dev_power_state_for_wake should return ACPI_STATE_UNKNOWN.
> >
> > Can you suggest where you think adding a acpi_dev_power_state_for_wake()
> > does make sense?
> >
> > Two areas that I think would work would be in: pci_pm_suspend_noirq()
> > (to avoid calling pci_prepare_to_sleep)
> >
> > or
> >
> > directly in pci_prepare_to_sleep() to check that value in lieu of
> > pci_target_state().
> >
>
> Rafael,
>
> Did you have any more thoughts on this?

Reportedly, if there are no ACPI power management objects associated
with a Root Port, Windows will always leave it in D0.

In that case, acpi_pci_bridge_d3() will return false unless the
HotPlugSupportInD3 property is present AFAICS, so the ACPI code will
not allow the port to be put into D3hot.

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?

So it looks like there are some systems on which programming Root
Ports into D3hot is needed to achieve additional power reduction of
the platform and there are systems on which programming Root Ports
into D3hot breaks things and there are no ACPI power management
objects associated with these Root Ports in both cases.

The only way to make progress that I can think about right now is to
limit the dmi_get_bios_year() check at the end of
pci_bridge_d3_possible() to Intel platforms.

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

[Mario Limonciello]

On 8/1/23 05:15, Rafael J. Wysocki wrote:
> On Tue, Aug 1, 2023 at 5:25 AM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> On 7/14/23 19:46, Limonciello, Mario wrote:
>>>
>>> On 7/14/2023 2:17 PM, Rafael J. Wysocki wrote:
>>>>>> Generally speaking, pci_bridge_d3_possible() is there to prevent
>>>>>> bridges (and PCIe ports in particular) from being put into D3hot/cold
>>>>>> if there are reasons to believe that it may not work.
>>>>>> acpi_pci_bridge_d3() is part of that.
>>>>>>
>>>>>> Even if it returns 'true', the _SxD/_SxW check should still be applied
>>>>>> via pci_target_state() to determine whether or not the firmware allows
>>>>>> this particular bridge to go into D3hot/cold.  So arguably, the _SxW
>>>>>> check in acpi_pci_bridge_d3() should not be necessary and if it makes
>>>>>> any functional difference, there is a bug somewhere else.
>>>>> But only if it was power manageable would the _SxD/_SxW check be
>>>>> applied.  This issue is around the branch of pci_target_state() where
>>>>> it's not power manageable and so it uses PME or it falls back to D3hot.
>>>> Well, this looks like a spec interpretation difference.
>>>>
>>>> We thought that _SxD/_SxW would only be relevant for devices with ACPI
>>>> PM support, but the firmware people seem to think that those objects
>>>> are also relevant for PCI devices that don't have ACPI PM support
>>>> (because those devices are still power-manageable via PMCSR).  If
>>>> Windows agrees with that viewpoint, we'll need to adjust, but not
>>>> through adding _SxW checks in random places.
>>> I think that depends upon how you want to handle the lack of _S0W.
>>>
>>> On these problematic devices there is no _S0W under the PCIe
>>> root port.  As I said; Windows puts them into D0 in this case though.
>>>
>>> So acpi_dev_power_state_for_wake should return ACPI_STATE_UNKNOWN.
>>>
>>> Can you suggest where you think adding a acpi_dev_power_state_for_wake()
>>> does make sense?
>>>
>>> Two areas that I think would work would be in: pci_pm_suspend_noirq()
>>> (to avoid calling pci_prepare_to_sleep)
>>>
>>> or
>>>
>>> directly in pci_prepare_to_sleep() to check that value in lieu of
>>> pci_target_state().
>>>
>>
>> Rafael,
>>
>> Did you have any more thoughts on this?
> 
> Reportedly, if there are no ACPI power management objects associated
> with a Root Port, Windows will always leave it in D0.
> 
> In that case, acpi_pci_bridge_d3() will return false unless the
> HotPlugSupportInD3 property is present AFAICS, so the ACPI code will
> not allow the port to be put into D3hot.
> 
> 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?

> So it looks like there are some systems on which programming Root
> Ports into D3hot is needed to achieve additional power reduction of
> the platform and there are systems on which programming Root Ports
> into D3hot breaks things and there are no ACPI power management
> objects associated with these Root Ports in both cases.
> 
> The only way to make progress that I can think about right now is to
> limit the dmi_get_bios_year() check at the end of
> pci_bridge_d3_possible() to Intel platforms.

Yeah if we can't come up with a method that works for both this might be 
the only real option.

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

[Mika Westerberg]

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.

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

[Mario Limonciello]

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.

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?

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

[Mika Westerberg]

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.

> 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.

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

[Mario Limonciello]

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?

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

[Mika Westerberg]

On Wed, Aug 02, 2023 at 09:35:35AM -0500, Mario Limonciello wrote:
> 
> 
> 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?

That's a good question. I would think they have those but I cannot be
sure for all the existing ones. I checked the RPL system I have here and
it does have _S0W and the HotPlugSupportInD3 at least.

> Does my PEP constraints theory hold steam at all?

I think it might be worthile to dig into it futher. Not sure if this
helps at all but the matching PEP constraint for the one of the root
ports mentioned above looks like this:

               Package (0x03)
               {
                    "\\_SB.PC00.RP09",
                    Zero,
                    Package (0x02)
                    {
                        Zero, 
                        Package (0x02)
                        {
                            0xFF, 
                            0x03
                        }
                    } 
                },