Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 16 May 2023 08:59:18 +0300
From:   Mika Westerberg <mika.westerberg@linux.intel.com>
To:     Mario Limonciello <mario.limonciello@amd.com>
Cc:     Bjorn Helgaas <bhelgaas@google.com>, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        S-k Shyam-sundar <Shyam-sundar.S-k@amd.com>,
        Natikar Basavaraj <Basavaraj.Natikar@amd.com>,
        Deucher Alexander <Alexander.Deucher@amd.com>,
        Iain Lane <iain@orangesquash.org.uk>,
        "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Subject: Re: [PATCH] PCI: Only put >= 2015 root ports into D3 on Intel
Message-ID: <20230516055918.GS66750@black.fi.intel.com>
References: <20230515231515.1440-1-mario.limonciello@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230515231515.1440-1-mario.limonciello@amd.com>
Precedence: bulk

+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