diff mbox series

PCI: Only put >= 2015 root ports into D3 on Intel

Message ID 20230515231515.1440-1-mario.limonciello@amd.com
State New
Headers
Series PCI: Only put >= 2015 root ports into D3 on Intel |

Commit Message

Mario Limonciello May 15, 2023, 11:15 p.m. UTC 
  Using an XHCI device to wakeup the system from s2idle fails when
that XHCI device is connected to a USB-C port for an AMD USB4
router.

Due to commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during
suspend") all root port go into D3 during s2idle.
When the root ports are in D3 over s2idle it's not possible for the
platform firmware to properly identify the wakeup source.

As a user presses a key on a keyboard the APU will exit
hardware sleep, but no wake source will be active so the kernel will
let the APU enter back into a hardware sleep state.

Here is an example of that sequence of events.  The USB keyboard was
pressed after 11.9 seconds, and then a GPIO was triggered after
another 12 seconds.
```
PM: suspend-to-idle
ACPI: EC: ACPI EC GPE status set
ACPI: PM: Rearming ACPI SCI for wakeup
amd_pmc AMDI0007:00: SMU idlemask s0i3: 0x8fff9eb5
Timekeeping suspended for 11.985 seconds
PM: Triggering wakeup from IRQ 9
ACPI: EC: ACPI EC GPE status set
ACPI: EC: ACPI EC GPE dispatched
ACPI: EC: ACPI EC work flushed
ACPI: EC: ACPI EC work flushed
ACPI: PM: Rearming ACPI SCI for wakeup
amd_pmc AMDI0007:00: SMU idlemask s0i3: 0x8fff9eb5
PM: Triggering wakeup from IRQ 9
ACPI: EC: ACPI EC GPE status set
ACPI: PM: Rearming ACPI SCI for wakeup
amd_pmc AMDI0007:00: SMU idlemask s0i3: 0x8fff9eb5
Timekeeping suspended for 12.916 seconds
PM: Triggering wakeup from IRQ 9
PM: Triggering wakeup from IRQ 7
ACPI: EC: ACPI EC GPE status set
ACPI: EC: ACPI EC GPE dispatched
ACPI: EC: ACPI EC work flushed
ACPI: PM: Wakeup after ACPI Notify sync
PM: resume from suspend-to-idle
```

If the root ports are in D0 during s2idle, then the wake source is
properly identified and an IRQ is active for the root port, waking
the system up.

Here is the same sequence with root ports in D0.  The USB keyboard
was pressed after 11 seconds.
```
PM: suspend-to-idle
ACPI: EC: ACPI EC GPE status set
ACPI: PM: Rearming ACPI SCI for wakeup
amd_pmc AMDI0007:00: SMU idlemask s0i3: 0x8fff9eb5
Timekeeping suspended for 11.138 seconds
PM: Triggering wakeup from IRQ 9
ACPI: PM: ACPI non-EC GPE wakeup
PM: resume from suspend-to-idle
PM: Triggering wakeup from IRQ 40
```

Comparing registers between Linux and Windows 11 this behavior to put root
ports into D3 at suspend is unique to Linux. Windows does not put the
root ports into D3 over Modern Standby.

As this policy change to put root ports into D3 if they're manufactured
after 2015 was originally introduced for Intel systems narrow it down to
only apply there.

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
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/pci.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Mika Westerberg May 16, 2023, 5:59 a.m. UTC | #1 
+Rafael

Hi Mario,

On Mon, May 15, 2023 at 06:15:15PM -0500, Mario Limonciello wrote:
> Using an XHCI device to wakeup the system from s2idle fails when
> that XHCI device is connected to a USB-C port for an AMD USB4
> router.
> 
> Due to commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during
> suspend") all root port go into D3 during s2idle.
> When the root ports are in D3 over s2idle it's not possible for the
> platform firmware to properly identify the wakeup source.
> 
> As a user presses a key on a keyboard the APU will exit
> hardware sleep, but no wake source will be active so the kernel will
> let the APU enter back into a hardware sleep state.
> 
> Here is an example of that sequence of events.  The USB keyboard was
> pressed after 11.9 seconds, and then a GPIO was triggered after
> another 12 seconds.
> ```
> PM: suspend-to-idle
> ACPI: EC: ACPI EC GPE status set
> ACPI: PM: Rearming ACPI SCI for wakeup
> amd_pmc AMDI0007:00: SMU idlemask s0i3: 0x8fff9eb5
> Timekeeping suspended for 11.985 seconds
> PM: Triggering wakeup from IRQ 9
> ACPI: EC: ACPI EC GPE status set
> ACPI: EC: ACPI EC GPE dispatched
> ACPI: EC: ACPI EC work flushed
> ACPI: EC: ACPI EC work flushed
> ACPI: PM: Rearming ACPI SCI for wakeup
> amd_pmc AMDI0007:00: SMU idlemask s0i3: 0x8fff9eb5
> PM: Triggering wakeup from IRQ 9
> ACPI: EC: ACPI EC GPE status set
> ACPI: PM: Rearming ACPI SCI for wakeup
> amd_pmc AMDI0007:00: SMU idlemask s0i3: 0x8fff9eb5
> Timekeeping suspended for 12.916 seconds
> PM: Triggering wakeup from IRQ 9
> PM: Triggering wakeup from IRQ 7
> ACPI: EC: ACPI EC GPE status set
> ACPI: EC: ACPI EC GPE dispatched
> ACPI: EC: ACPI EC work flushed
> ACPI: PM: Wakeup after ACPI Notify sync
> PM: resume from suspend-to-idle
> ```
> 
> If the root ports are in D0 during s2idle, then the wake source is
> properly identified and an IRQ is active for the root port, waking
> the system up.
> 
> Here is the same sequence with root ports in D0.  The USB keyboard
> was pressed after 11 seconds.
> ```
> PM: suspend-to-idle
> ACPI: EC: ACPI EC GPE status set
> ACPI: PM: Rearming ACPI SCI for wakeup
> amd_pmc AMDI0007:00: SMU idlemask s0i3: 0x8fff9eb5
> Timekeeping suspended for 11.138 seconds
> PM: Triggering wakeup from IRQ 9
> ACPI: PM: ACPI non-EC GPE wakeup
> PM: resume from suspend-to-idle
> PM: Triggering wakeup from IRQ 40
> ```
> 
> Comparing registers between Linux and Windows 11 this behavior to put root
> ports into D3 at suspend is unique to Linux. Windows does not put the
> root ports into D3 over Modern Standby.

Are you sure this is the case? Leaving the root port in D0 would mean
its power resource is left on and that would leave the whole USB4 domain
on consuming quite a lot of power. The root ports involved are supposed
to support PME from the D3hot/cold (this is what they advertise in their
config spaces) which should allow the OS to move the port into low power
states. Of course the PME from D3cold in general needs some ACPI support
(typically a GPE) in the BIOS.

> As this policy change to put root ports into D3 if they're manufactured
> after 2015 was originally introduced for Intel systems narrow it down to
> only apply there.
> 
> 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
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/pci/pci.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 5ede93222bc1..7d1b078b8d40 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3010,12 +3010,17 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>  		if (dmi_check_system(bridge_d3_blacklist))
>  			return false;
>  
> +#ifdef CONFIG_X86
>  		/*
> -		 * It should be safe to put PCIe ports from 2015 or newer
> -		 * to D3.
> +		 * It should be safe to put PCIe ports from Intel systems
> +		 * from 2015 or newer to D3.
> +		 * Windows 11 does not do this over Modern Standby and this is
> +		 * known to cause problems with s2idle on some AMD systems.
>  		 */
> -		if (dmi_get_bios_year() >= 2015)
> +		if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&

Can't we then check here AMD case instead? Then generic case would be to
support according what the port announces.

> +		    dmi_get_bios_year() >= 2015)
>  			return true;
> +#endif
>  		break;
>  	}
>  
> -- 
> 2.34.1
Limonciello, Mario May 16, 2023, 2:29 p.m. UTC | #2 
On 5/16/2023 12:59 AM, Mika Westerberg wrote:

> +Rafael
>
> Hi Mario,
>
> On Mon, May 15, 2023 at 06:15:15PM -0500, Mario Limonciello wrote:
>> Using an XHCI device to wakeup the system from s2idle fails when
>> that XHCI device is connected to a USB-C port for an AMD USB4
>> router.
>>
>> Due to commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during
>> suspend") all root port go into D3 during s2idle.
>> When the root ports are in D3 over s2idle it's not possible for the
>> platform firmware to properly identify the wakeup source.
>>
>> As a user presses a key on a keyboard the APU will exit
>> hardware sleep, but no wake source will be active so the kernel will
>> let the APU enter back into a hardware sleep state.
>>
>> Here is an example of that sequence of events.  The USB keyboard was
>> pressed after 11.9 seconds, and then a GPIO was triggered after
>> another 12 seconds.
>> ```
>> PM: suspend-to-idle
>> ACPI: EC: ACPI EC GPE status set
>> ACPI: PM: Rearming ACPI SCI for wakeup
>> amd_pmc AMDI0007:00: SMU idlemask s0i3: 0x8fff9eb5
>> Timekeeping suspended for 11.985 seconds
>> PM: Triggering wakeup from IRQ 9
>> ACPI: EC: ACPI EC GPE status set
>> ACPI: EC: ACPI EC GPE dispatched
>> ACPI: EC: ACPI EC work flushed
>> ACPI: EC: ACPI EC work flushed
>> ACPI: PM: Rearming ACPI SCI for wakeup
>> amd_pmc AMDI0007:00: SMU idlemask s0i3: 0x8fff9eb5
>> PM: Triggering wakeup from IRQ 9
>> ACPI: EC: ACPI EC GPE status set
>> ACPI: PM: Rearming ACPI SCI for wakeup
>> amd_pmc AMDI0007:00: SMU idlemask s0i3: 0x8fff9eb5
>> Timekeeping suspended for 12.916 seconds
>> PM: Triggering wakeup from IRQ 9
>> PM: Triggering wakeup from IRQ 7
>> ACPI: EC: ACPI EC GPE status set
>> ACPI: EC: ACPI EC GPE dispatched
>> ACPI: EC: ACPI EC work flushed
>> ACPI: PM: Wakeup after ACPI Notify sync
>> PM: resume from suspend-to-idle
>> ```
>>
>> If the root ports are in D0 during s2idle, then the wake source is
>> properly identified and an IRQ is active for the root port, waking
>> the system up.
>>
>> Here is the same sequence with root ports in D0.  The USB keyboard
>> was pressed after 11 seconds.
>> ```
>> PM: suspend-to-idle
>> ACPI: EC: ACPI EC GPE status set
>> ACPI: PM: Rearming ACPI SCI for wakeup
>> amd_pmc AMDI0007:00: SMU idlemask s0i3: 0x8fff9eb5
>> Timekeeping suspended for 11.138 seconds
>> PM: Triggering wakeup from IRQ 9
>> ACPI: PM: ACPI non-EC GPE wakeup
>> PM: resume from suspend-to-idle
>> PM: Triggering wakeup from IRQ 40
>> ```
>>
>> Comparing registers between Linux and Windows 11 this behavior to put root
>> ports into D3 at suspend is unique to Linux. Windows does not put the
>> root ports into D3 over Modern Standby.
> Are you sure this is the case? Leaving the root port in D0 would mean
> its power resource is left on and that would leave the whole USB4 domain
> on consuming quite a lot of power.

Right - I had the same thought, but at least for AMD after the hardware 
enters into s2idle
the platform does some internal management of power domains.

> The root ports involved are supposed
> to support PME from the D3hot/cold (this is what they advertise in their
> config spaces) which should allow the OS to move the port into low power
> states. Of course the PME from D3cold in general needs some ACPI support
> (typically a GPE) in the BIOS.

AFAICT the actual issue is entirely a wakeup platform firmware 
sequencing issue
while in a hardware sleep state and not PMEs.

It's only exposed by putting the root ports into D3 over s2idle.

As an experiment on an unpatched kernel if I avoid letting amd-pmc bind 
then the
hardware will never enter a hardware sleep state over Linux s2idle and 
this issue
doesn't occur.

That shows that PMEs *do* work from D3cold.

With all of this I have to wonder if the Windows behavior of what to do 
with the root
ports is tied to the uPEP requirements specified in the firmware.

Linux doesn't do any enforcement or adjustments from what uPEP indicates.

The uPEP constraints for the root port in question in an affected AMD 
system has:

                     Package (0x04)
                     {
                         Zero,
                         "\\_SB.PCI0.GP19",
                         Zero,
                         Zero
                     },

AMD's parsing is through 'lpi_device_get_constraints_amd' so that 
structure shows
as not enabled and doesn't specify any D-state requirements.

What do they specify for Intel on a matching root port?

>> As this policy change to put root ports into D3 if they're manufactured
>> after 2015 was originally introduced for Intel systems narrow it down to
>> only apply there.
>>
>> 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
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/pci/pci.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 5ede93222bc1..7d1b078b8d40 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -3010,12 +3010,17 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>>   		if (dmi_check_system(bridge_d3_blacklist))
>>   			return false;
>>   
>> +#ifdef CONFIG_X86
>>   		/*
>> -		 * It should be safe to put PCIe ports from 2015 or newer
>> -		 * to D3.
>> +		 * It should be safe to put PCIe ports from Intel systems
>> +		 * from 2015 or newer to D3.
>> +		 * Windows 11 does not do this over Modern Standby and this is
>> +		 * known to cause problems with s2idle on some AMD systems.
>>   		 */
>> -		if (dmi_get_bios_year() >= 2015)
>> +		if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
> Can't we then check here AMD case instead? Then generic case would be to
> support according what the port announces.
Right; that was my original patch, but given the Windows behavior I 
figured it was better

to discuss if the original patch was actually correct or not.
Mika Westerberg May 17, 2023, 7:15 a.m. UTC | #3 
Hi Mario,

On Tue, May 16, 2023 at 09:29:38AM -0500, Limonciello, Mario wrote:
> On 5/16/2023 12:59 AM, Mika Westerberg wrote:
> 
> > +Rafael
> > 
> > Hi Mario,
> > 
> > On Mon, May 15, 2023 at 06:15:15PM -0500, Mario Limonciello wrote:
> > > Using an XHCI device to wakeup the system from s2idle fails when
> > > that XHCI device is connected to a USB-C port for an AMD USB4
> > > router.
> > > 
> > > Due to commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during
> > > suspend") all root port go into D3 during s2idle.
> > > When the root ports are in D3 over s2idle it's not possible for the
> > > platform firmware to properly identify the wakeup source.
> > > 
> > > As a user presses a key on a keyboard the APU will exit
> > > hardware sleep, but no wake source will be active so the kernel will
> > > let the APU enter back into a hardware sleep state.
> > > 
> > > Here is an example of that sequence of events.  The USB keyboard was
> > > pressed after 11.9 seconds, and then a GPIO was triggered after
> > > another 12 seconds.
> > > ```
> > > PM: suspend-to-idle
> > > ACPI: EC: ACPI EC GPE status set
> > > ACPI: PM: Rearming ACPI SCI for wakeup
> > > amd_pmc AMDI0007:00: SMU idlemask s0i3: 0x8fff9eb5
> > > Timekeeping suspended for 11.985 seconds
> > > PM: Triggering wakeup from IRQ 9
> > > ACPI: EC: ACPI EC GPE status set
> > > ACPI: EC: ACPI EC GPE dispatched
> > > ACPI: EC: ACPI EC work flushed
> > > ACPI: EC: ACPI EC work flushed
> > > ACPI: PM: Rearming ACPI SCI for wakeup
> > > amd_pmc AMDI0007:00: SMU idlemask s0i3: 0x8fff9eb5
> > > PM: Triggering wakeup from IRQ 9
> > > ACPI: EC: ACPI EC GPE status set
> > > ACPI: PM: Rearming ACPI SCI for wakeup
> > > amd_pmc AMDI0007:00: SMU idlemask s0i3: 0x8fff9eb5
> > > Timekeeping suspended for 12.916 seconds
> > > PM: Triggering wakeup from IRQ 9
> > > PM: Triggering wakeup from IRQ 7
> > > ACPI: EC: ACPI EC GPE status set
> > > ACPI: EC: ACPI EC GPE dispatched
> > > ACPI: EC: ACPI EC work flushed
> > > ACPI: PM: Wakeup after ACPI Notify sync
> > > PM: resume from suspend-to-idle
> > > ```
> > > 
> > > If the root ports are in D0 during s2idle, then the wake source is
> > > properly identified and an IRQ is active for the root port, waking
> > > the system up.
> > > 
> > > Here is the same sequence with root ports in D0.  The USB keyboard
> > > was pressed after 11 seconds.
> > > ```
> > > PM: suspend-to-idle
> > > ACPI: EC: ACPI EC GPE status set
> > > ACPI: PM: Rearming ACPI SCI for wakeup
> > > amd_pmc AMDI0007:00: SMU idlemask s0i3: 0x8fff9eb5
> > > Timekeeping suspended for 11.138 seconds
> > > PM: Triggering wakeup from IRQ 9
> > > ACPI: PM: ACPI non-EC GPE wakeup
> > > PM: resume from suspend-to-idle
> > > PM: Triggering wakeup from IRQ 40
> > > ```
> > > 
> > > Comparing registers between Linux and Windows 11 this behavior to put root
> > > ports into D3 at suspend is unique to Linux. Windows does not put the
> > > root ports into D3 over Modern Standby.
> > Are you sure this is the case? Leaving the root port in D0 would mean
> > its power resource is left on and that would leave the whole USB4 domain
> > on consuming quite a lot of power.
> 
> Right - I had the same thought, but at least for AMD after the hardware
> enters into s2idle
> the platform does some internal management of power domains.
> 
> > The root ports involved are supposed
> > to support PME from the D3hot/cold (this is what they advertise in their
> > config spaces) which should allow the OS to move the port into low power
> > states. Of course the PME from D3cold in general needs some ACPI support
> > (typically a GPE) in the BIOS.
> 
> AFAICT the actual issue is entirely a wakeup platform firmware sequencing
> issue
> while in a hardware sleep state and not PMEs.
> 
> It's only exposed by putting the root ports into D3 over s2idle.

But there are two ways to enter s2idle (well or the S0ix whatever is the
AMD term for that). Either through system sleep or simply waiting that
all the needed devices runtime suspend. There should be no difference
from device perspective AFAICT.

> As an experiment on an unpatched kernel if I avoid letting amd-pmc bind then
> the
> hardware will never enter a hardware sleep state over Linux s2idle and this
> issue
> doesn't occur.
> 
> That shows that PMEs *do* work from D3cold.
> 
> With all of this I have to wonder if the Windows behavior of what to do with
> the root
> ports is tied to the uPEP requirements specified in the firmware.
> 
> Linux doesn't do any enforcement or adjustments from what uPEP indicates.
> 
> The uPEP constraints for the root port in question in an affected AMD system
> has:
> 
>                     Package (0x04)
>                     {
>                         Zero,
>                         "\\_SB.PCI0.GP19",
>                         Zero,
>                         Zero
>                     },
> 
> AMD's parsing is through 'lpi_device_get_constraints_amd' so that structure
> shows
> as not enabled and doesn't specify any D-state requirements.

AFAIK this object does not exist in ChromeOS so Linux cannot use it
there.

> What do they specify for Intel on a matching root port?

I think the corresponding entry in ADL-P system for TBT PCIe root port 0
looks like this:

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

I'm not entirely sure what does it tell? ;-)
Mario Limonciello May 17, 2023, 12:33 p.m. UTC | #4 
>> AFAICT the actual issue is entirely a wakeup platform firmware sequencing
>> issue
>> while in a hardware sleep state and not PMEs.
>>
>> It's only exposed by putting the root ports into D3 over s2idle.
> But there are two ways to enter s2idle (well or the S0ix whatever is the
> AMD term for that). Either through system sleep or simply waiting that
> all the needed devices runtime suspend. There should be no difference
> from device perspective AFAICT.
On AMD all devices in runtime suspend and SoC entering system
suspend aren't the same state.
>> As an experiment on an unpatched kernel if I avoid letting amd-pmc bind then
>> the
>> hardware will never enter a hardware sleep state over Linux s2idle and this
>> issue
>> doesn't occur.
>>
>> That shows that PMEs *do* work from D3cold.
>>
>> With all of this I have to wonder if the Windows behavior of what to do with
>> the root
>> ports is tied to the uPEP requirements specified in the firmware.
>>
>> Linux doesn't do any enforcement or adjustments from what uPEP indicates.
>>
>> The uPEP constraints for the root port in question in an affected AMD system
>> has:
>>
>>                      Package (0x04)
>>                      {
>>                          Zero,
>>                          "\\_SB.PCI0.GP19",
>>                          Zero,
>>                          Zero
>>                      },
>>
>> AMD's parsing is through 'lpi_device_get_constraints_amd' so that structure
>> shows
>> as not enabled and doesn't specify any D-state requirements.
> AFAIK this object does not exist in ChromeOS so Linux cannot use it
> there.
OK that means that if we came up with a solution that utilized
uPEP that it would have to remain optional.
>
>> What do they specify for Intel on a matching root port?
> I think the corresponding entry in ADL-P system for TBT PCIe root port 0
> looks like this:
>
> 	Package (0x03)
> 	{
> 	    "\\_SB.PC00.TRP0",
> 	    Zero,
> 	    Package (0x02)
> 	    {
> 		Zero,
> 		Package (0x02)
> 		{
> 		    0xFF,
> 		    0x03
> 		}
> 	    }
> 	},
>
> I'm not entirely sure what does it tell? ;-)

It's parsed using `lpi_device_get_constraints`.

So should I follow it right this means for ACPI device
\\_SB.PC00.TRP0 the constraint is disabled.

It's described as
Version 0, UID 0xFF has a minimum D-state of 3.

That means my idea to try to only change D-states at
suspend for enabled constraints won't help.
Mika Westerberg May 17, 2023, 12:58 p.m. UTC | #5 
On Wed, May 17, 2023 at 07:33:17AM -0500, Limonciello, Mario wrote:
> 
> > > AFAICT the actual issue is entirely a wakeup platform firmware sequencing
> > > issue
> > > while in a hardware sleep state and not PMEs.
> > > 
> > > It's only exposed by putting the root ports into D3 over s2idle.
> > But there are two ways to enter s2idle (well or the S0ix whatever is the
> > AMD term for that). Either through system sleep or simply waiting that
> > all the needed devices runtime suspend. There should be no difference
> > from device perspective AFAICT.

I should correct that the wakes may be configured differently, though.

> On AMD all devices in runtime suspend and SoC entering system
> suspend aren't the same state.

Okay.

> > > As an experiment on an unpatched kernel if I avoid letting amd-pmc bind then
> > > the
> > > hardware will never enter a hardware sleep state over Linux s2idle and this
> > > issue
> > > doesn't occur.
> > > 
> > > That shows that PMEs *do* work from D3cold.
> > > 
> > > With all of this I have to wonder if the Windows behavior of what to do with
> > > the root
> > > ports is tied to the uPEP requirements specified in the firmware.
> > > 
> > > Linux doesn't do any enforcement or adjustments from what uPEP indicates.
> > > 
> > > The uPEP constraints for the root port in question in an affected AMD system
> > > has:
> > > 
> > >                      Package (0x04)
> > >                      {
> > >                          Zero,
> > >                          "\\_SB.PCI0.GP19",
> > >                          Zero,
> > >                          Zero
> > >                      },
> > > 
> > > AMD's parsing is through 'lpi_device_get_constraints_amd' so that structure
> > > shows
> > > as not enabled and doesn't specify any D-state requirements.
> > AFAIK this object does not exist in ChromeOS so Linux cannot use it
> > there.
> OK that means that if we came up with a solution that utilized
> uPEP that it would have to remain optional.

Right.

> > > What do they specify for Intel on a matching root port?
> > I think the corresponding entry in ADL-P system for TBT PCIe root port 0
> > looks like this:
> > 
> > 	Package (0x03)
> > 	{
> > 	    "\\_SB.PC00.TRP0",
> > 	    Zero,
> > 	    Package (0x02)
> > 	    {
> > 		Zero,
> > 		Package (0x02)
> > 		{
> > 		    0xFF,
> > 		    0x03
> > 		}
> > 	    }
> > 	},
> > 
> > I'm not entirely sure what does it tell? ;-)
> 
> It's parsed using `lpi_device_get_constraints`.
> 
> So should I follow it right this means for ACPI device
> \\_SB.PC00.TRP0 the constraint is disabled.
> 
> It's described as
> Version 0, UID 0xFF has a minimum D-state of 3.

I see, so it needs to be in D3 for this "constraint" to be valid.

> That means my idea to try to only change D-states at
> suspend for enabled constraints won't help.

:(
Mario Limonciello May 18, 2023, 1:48 a.m. UTC | #6 
On 5/17/2023 7:58 AM, Mika Westerberg wrote:
> On Wed, May 17, 2023 at 07:33:17AM -0500, Limonciello, Mario wrote:
>>>> AFAICT the actual issue is entirely a wakeup platform firmware sequencing
>>>> issue
>>>> while in a hardware sleep state and not PMEs.
>>>>
>>>> It's only exposed by putting the root ports into D3 over s2idle.
>>> But there are two ways to enter s2idle (well or the S0ix whatever is the
>>> AMD term for that). Either through system sleep or simply waiting that
>>> all the needed devices runtime suspend. There should be no difference
>>> from device perspective AFAICT.
> I should correct that the wakes may be configured differently, though.
>
>> On AMD all devices in runtime suspend and SoC entering system
>> suspend aren't the same state.
> Okay.
>
>>>> As an experiment on an unpatched kernel if I avoid letting amd-pmc bind then
>>>> the
>>>> hardware will never enter a hardware sleep state over Linux s2idle and this
>>>> issue
>>>> doesn't occur.
>>>>
>>>> That shows that PMEs *do* work from D3cold.
>>>>
>>>> With all of this I have to wonder if the Windows behavior of what to do with
>>>> the root
>>>> ports is tied to the uPEP requirements specified in the firmware.
>>>>
>>>> Linux doesn't do any enforcement or adjustments from what uPEP indicates.
>>>>
>>>> The uPEP constraints for the root port in question in an affected AMD system
>>>> has:
>>>>
>>>>                       Package (0x04)
>>>>                       {
>>>>                           Zero,
>>>>                           "\\_SB.PCI0.GP19",
>>>>                           Zero,
>>>>                           Zero
>>>>                       },
>>>>
>>>> AMD's parsing is through 'lpi_device_get_constraints_amd' so that structure
>>>> shows
>>>> as not enabled and doesn't specify any D-state requirements.
>>> AFAIK this object does not exist in ChromeOS so Linux cannot use it
>>> there.
>> OK that means that if we came up with a solution that utilized
>> uPEP that it would have to remain optional.
> Right.
>
>>>> What do they specify for Intel on a matching root port?
>>> I think the corresponding entry in ADL-P system for TBT PCIe root port 0
>>> looks like this:
>>>
>>> 	Package (0x03)
>>> 	{
>>> 	    "\\_SB.PC00.TRP0",
>>> 	    Zero,
>>> 	    Package (0x02)
>>> 	    {
>>> 		Zero,
>>> 		Package (0x02)
>>> 		{
>>> 		    0xFF,
>>> 		    0x03
>>> 		}
>>> 	    }
>>> 	},
>>>
>>> I'm not entirely sure what does it tell? ;-)
>> It's parsed using `lpi_device_get_constraints`.
>>
>> So should I follow it right this means for ACPI device
>> \\_SB.PC00.TRP0 the constraint is disabled.
>>
>> It's described as
>> Version 0, UID 0xFF has a minimum D-state of 3.
> I see, so it needs to be in D3 for this "constraint" to be valid.
>
>> That means my idea to try to only change D-states at
>> suspend for enabled constraints won't help.
> :(

At least on an Alder Lake P system can you check
whether your root ports actually respond
affirmatively to acpi_pci_bridge_d3() or they need
to fall back to that logic?

In my case the problematic ones don't have _PRW or
_S0W, which might explain why Windows doesn't try
to use D3 (hot or cold) for them either.

If the root ports on your current systems do respond
through acpi_pci_bridge_d3() a possible solution here
might be an allow list for systems from 2015-2018 rather
than everything > 2015.
Mario Limonciello May 18, 2023, 2 a.m. UTC | #7 
On 5/17/2023 8:48 PM, Limonciello, Mario wrote:
>
> On 5/17/2023 7:58 AM, Mika Westerberg wrote:
>> On Wed, May 17, 2023 at 07:33:17AM -0500, Limonciello, Mario wrote:
>>>>> AFAICT the actual issue is entirely a wakeup platform firmware 
>>>>> sequencing
>>>>> issue
>>>>> while in a hardware sleep state and not PMEs.
>>>>>
>>>>> It's only exposed by putting the root ports into D3 over s2idle.
>>>> But there are two ways to enter s2idle (well or the S0ix whatever 
>>>> is the
>>>> AMD term for that). Either through system sleep or simply waiting that
>>>> all the needed devices runtime suspend. There should be no difference
>>>> from device perspective AFAICT.
>> I should correct that the wakes may be configured differently, though.
>>
>>> On AMD all devices in runtime suspend and SoC entering system
>>> suspend aren't the same state.
>> Okay.
>>
>>>>> As an experiment on an unpatched kernel if I avoid letting amd-pmc 
>>>>> bind then
>>>>> the
>>>>> hardware will never enter a hardware sleep state over Linux s2idle 
>>>>> and this
>>>>> issue
>>>>> doesn't occur.
>>>>>
>>>>> That shows that PMEs *do* work from D3cold.
>>>>>
>>>>> With all of this I have to wonder if the Windows behavior of what 
>>>>> to do with
>>>>> the root
>>>>> ports is tied to the uPEP requirements specified in the firmware.
>>>>>
>>>>> Linux doesn't do any enforcement or adjustments from what uPEP 
>>>>> indicates.
>>>>>
>>>>> The uPEP constraints for the root port in question in an affected 
>>>>> AMD system
>>>>> has:
>>>>>
>>>>>                       Package (0x04)
>>>>>                       {
>>>>>                           Zero,
>>>>>                           "\\_SB.PCI0.GP19",
>>>>>                           Zero,
>>>>>                           Zero
>>>>>                       },
>>>>>
>>>>> AMD's parsing is through 'lpi_device_get_constraints_amd' so that 
>>>>> structure
>>>>> shows
>>>>> as not enabled and doesn't specify any D-state requirements.
>>>> AFAIK this object does not exist in ChromeOS so Linux cannot use it
>>>> there.
>>> OK that means that if we came up with a solution that utilized
>>> uPEP that it would have to remain optional.
>> Right.
>>
>>>>> What do they specify for Intel on a matching root port?
>>>> I think the corresponding entry in ADL-P system for TBT PCIe root 
>>>> port 0
>>>> looks like this:
>>>>
>>>>     Package (0x03)
>>>>     {
>>>>         "\\_SB.PC00.TRP0",
>>>>         Zero,
>>>>         Package (0x02)
>>>>         {
>>>>         Zero,
>>>>         Package (0x02)
>>>>         {
>>>>             0xFF,
>>>>             0x03
>>>>         }
>>>>         }
>>>>     },
>>>>
>>>> I'm not entirely sure what does it tell? ;-)
>>> It's parsed using `lpi_device_get_constraints`.
>>>
>>> So should I follow it right this means for ACPI device
>>> \\_SB.PC00.TRP0 the constraint is disabled.
>>>
>>> It's described as
>>> Version 0, UID 0xFF has a minimum D-state of 3.
>> I see, so it needs to be in D3 for this "constraint" to be valid.
>>
>>> That means my idea to try to only change D-states at
>>> suspend for enabled constraints won't help.
>> :(
>
> At least on an Alder Lake P system can you check
> whether your root ports actually respond
> affirmatively to acpi_pci_bridge_d3() or they need
> to fall back to that logic?
>
> In my case the problematic ones don't have _PRW or
> _S0W, which might explain why Windows doesn't try
> to use D3 (hot or cold) for them either.
>
> If the root ports on your current systems do respond
> through acpi_pci_bridge_d3() a possible solution here
> might be an allow list for systems from 2015-2018 rather
> than everything > 2015.
>
Actually this is exactly it.  The function is missing a call

for platform_pci_power_manageable().

I'll send out a v2.
diff mbox series

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 5ede93222bc1..7d1b078b8d40 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3010,12 +3010,17 @@  bool pci_bridge_d3_possible(struct pci_dev *bridge)
 		if (dmi_check_system(bridge_d3_blacklist))
 			return false;
 
+#ifdef CONFIG_X86
 		/*
-		 * It should be safe to put PCIe ports from 2015 or newer
-		 * to D3.
+		 * It should be safe to put PCIe ports from Intel systems
+		 * from 2015 or newer to D3.
+		 * Windows 11 does not do this over Modern Standby and this is
+		 * known to cause problems with s2idle on some AMD systems.
 		 */
-		if (dmi_get_bios_year() >= 2015)
+		if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
+		    dmi_get_bios_year() >= 2015)
 			return true;
+#endif
 		break;
 	}