[PATCH v16 0/3] Avoid PCIe D3 for AMD PCIe root ports

[PATCH v16 0/3] Avoid PCIe D3 for AMD PCIe root ports

[Mario Limonciello]

D3 on PCIe root ports isn't used on Windows systems in Modern Standby.
This series adjusts the amd-pmc driver to choose the same strategy
for Rembrandt and Phoenix platforms in Linux with s2idle.

LPS0 constraints are the basis for it; which if they are added for
Windows would also apply for Linux as well.

This version doesn't incorporate a callback, as it's pending feedback
from Bjorn if that approach is amenable.

NOTE:
This series relies upon changes that are both in linux-pm.git and
platform-x86.git. So it won't be able to apply to either maintainer's
tree until later.

Mario Limonciello (3):
  ACPI: x86: s2idle: Export symbol for fetching constraints for module
    use
  platform/x86/amd: pmc: Adjust workarounds to be part of a switch/case
  platform/x86/amd: pmc: Don't let PCIe root ports go into D3

 drivers/acpi/x86/s2idle.c          |  1 +
 drivers/platform/x86/amd/pmc/pmc.c | 56 ++++++++++++++++++++++++++----
 2 files changed, 50 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH v16 1/3] ACPI: x86: s2idle: Export symbol for fetching constraints for module use

[Mario Limonciello]

The amd-pmc driver will be fetching constraints to make decisions at
suspend time. This driver can be compiled as a module, so export the
symbol for when it is a module.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v15->v16:
 * new patch
---
 drivers/acpi/x86/s2idle.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 08f7c6708206..de9c313c21fa 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -322,6 +322,7 @@ int acpi_get_lps0_constraint(struct acpi_device *adev)
 
 	return ACPI_STATE_UNKNOWN;
 }
+EXPORT_SYMBOL_GPL(acpi_get_lps0_constraint);
 
 static void lpi_check_constraints(void)
 {
-- 
2.34.1

[PATCH v16 2/3] platform/x86/amd: pmc: Adjust workarounds to be part of a switch/case

[Mario Limonciello]

To allow introducing additional workarounds more cleanly for other
platforms change the if block into a switch/case.
No intended functional changes.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd/pmc/pmc.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index c1e788b67a74..eb2a4263814c 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -884,17 +884,20 @@ static struct acpi_s2idle_dev_ops amd_pmc_s2idle_dev_ops = {
 static int amd_pmc_suspend_handler(struct device *dev)
 {
 	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
+	int rc = 0;
 
-	if (pdev->cpu_id == AMD_CPU_ID_CZN && !disable_workarounds) {
-		int rc = amd_pmc_czn_wa_irq1(pdev);
+	if (disable_workarounds)
+		return 0;
 
-		if (rc) {
-			dev_err(pdev->dev, "failed to adjust keyboard wakeup: %d\n", rc);
-			return rc;
-		}
+	switch (pdev->cpu_id) {
+	case AMD_CPU_ID_CZN:
+		rc = amd_pmc_czn_wa_irq1(pdev);
+		break;
+	default:
+		break;
 	}
 
-	return 0;
+	return rc;
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(amd_pmc_pm, amd_pmc_suspend_handler, NULL);
-- 
2.34.1

[PATCH v16 3/3] platform/x86/amd: pmc: Don't let PCIe root ports go into D3

[Mario Limonciello]

commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
changed pci_bridge_d3_possible() so that any vendor's PCIe ports
from modern machines (>=2015) are allowed to be put into D3.

Iain reports that USB devices can't be used to wake a Lenovo Z13
from suspend. This is because the PCIe root port has been put
into D3 and AMD's platform can't handle USB devices waking from
a hardware sleep state in this case.

This problem only occurs on Linux, and only when the AMD PMC driver
is utilized to put the device into a hardware sleep state. Comparing
the behavior on Windows and Linux, Windows doesn't put the root ports
into D3.

A variety of approaches were discussed to change PCI core to handle this
case generically but no consensus was reached. To limit the scope of
effect only to the affected machines introduce a workaround into the
amd-pmc driver to only apply to the PCI root ports in affected machines
when going into hardware sleep.

Link: https://lore.kernel.org/linux-pci/20230818193932.27187-1-mario.limonciello@amd.com/
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
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v15->v16:
 * Only match PCIe root ports with ACPI companions
 * Use constraints when workaround activated
---
 drivers/platform/x86/amd/pmc/pmc.c | 39 ++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index eb2a4263814c..6a037447ec5a 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -741,6 +741,41 @@ static int amd_pmc_czn_wa_irq1(struct amd_pmc_dev *pdev)
 	return 0;
 }
 
+/* only allow PCIe root ports with a LPS0 constraint configured to go to D3 */
+static int amd_pmc_rp_wa(struct amd_pmc_dev *pdev)
+{
+	struct pci_dev *pci_dev = NULL;
+
+	while ((pci_dev = pci_get_device(PCI_VENDOR_ID_AMD, PCI_ANY_ID, pci_dev))) {
+		struct acpi_device *adev;
+		int constraint;
+
+		if (!pci_is_pcie(pci_dev) ||
+		    !(pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT))
+			continue;
+
+		if (pci_dev->current_state == PCI_D3hot ||
+		    pci_dev->current_state == PCI_D3cold)
+			continue;
+
+		adev = ACPI_COMPANION(&pci_dev->dev);
+		if (!adev)
+			continue;
+
+		constraint = acpi_get_lps0_constraint(adev);
+		if (constraint != ACPI_STATE_UNKNOWN &&
+		    constraint >= ACPI_STATE_S3)
+			continue;
+
+		if (pci_dev->bridge_d3 == 0)
+			continue;
+		pci_dev->bridge_d3 = 0;
+		dev_info(&pci_dev->dev, "Disabling D3 on PCIe root port due lack of constraint\n");
+	}
+
+	return 0;
+}
+
 static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
 {
 	struct rtc_device *rtc_device;
@@ -893,6 +928,10 @@ static int amd_pmc_suspend_handler(struct device *dev)
 	case AMD_CPU_ID_CZN:
 		rc = amd_pmc_czn_wa_irq1(pdev);
 		break;
+	case AMD_CPU_ID_YC:
+	case AMD_CPU_ID_PS:
+		rc = amd_pmc_rp_wa(pdev);
+		break;
 	default:
 		break;
 	}
-- 
2.34.1

Re: [PATCH v16 1/3] ACPI: x86: s2idle: Export symbol for fetching constraints for module use

[Rafael J. Wysocki]

On Tue, Aug 29, 2023 at 9:28 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> The amd-pmc driver will be fetching constraints to make decisions at
> suspend time. This driver can be compiled as a module, so export the
> symbol for when it is a module.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

and please route this along with the rest of the series.

> ---
> v15->v16:
>  * new patch
> ---
>  drivers/acpi/x86/s2idle.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> index 08f7c6708206..de9c313c21fa 100644
> --- a/drivers/acpi/x86/s2idle.c
> +++ b/drivers/acpi/x86/s2idle.c
> @@ -322,6 +322,7 @@ int acpi_get_lps0_constraint(struct acpi_device *adev)
>
>         return ACPI_STATE_UNKNOWN;
>  }
> +EXPORT_SYMBOL_GPL(acpi_get_lps0_constraint);
>
>  static void lpi_check_constraints(void)
>  {
> --
> 2.34.1
>

Re: [PATCH v16 3/3] platform/x86/amd: pmc: Don't let PCIe root ports go into D3

[Shyam Sundar S K]

On 8/29/2023 10:42 PM, Mario Limonciello wrote:
> commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> changed pci_bridge_d3_possible() so that any vendor's PCIe ports
> from modern machines (>=2015) are allowed to be put into D3.
> 
> Iain reports that USB devices can't be used to wake a Lenovo Z13
> from suspend. This is because the PCIe root port has been put
> into D3 and AMD's platform can't handle USB devices waking from
> a hardware sleep state in this case.
> 
> This problem only occurs on Linux, and only when the AMD PMC driver
> is utilized to put the device into a hardware sleep state. Comparing
> the behavior on Windows and Linux, Windows doesn't put the root ports
> into D3.
> 
> A variety of approaches were discussed to change PCI core to handle this
> case generically but no consensus was reached. To limit the scope of
> effect only to the affected machines introduce a workaround into the
> amd-pmc driver to only apply to the PCI root ports in affected machines
> when going into hardware sleep.
> 
> Link: https://lore.kernel.org/linux-pci/20230818193932.27187-1-mario.limonciello@amd.com/
> 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
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

See if this change can be moved to pmc-quirks.c, besides that change
looks good to me. Thank you.

Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

> ---
> v15->v16:
>  * Only match PCIe root ports with ACPI companions
>  * Use constraints when workaround activated
> ---
>  drivers/platform/x86/amd/pmc/pmc.c | 39 ++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index eb2a4263814c..6a037447ec5a 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -741,6 +741,41 @@ static int amd_pmc_czn_wa_irq1(struct amd_pmc_dev *pdev)
>  	return 0;
>  }
>  
> +/* only allow PCIe root ports with a LPS0 constraint configured to go to D3 */
> +static int amd_pmc_rp_wa(struct amd_pmc_dev *pdev)
> +{
> +	struct pci_dev *pci_dev = NULL;
> +
> +	while ((pci_dev = pci_get_device(PCI_VENDOR_ID_AMD, PCI_ANY_ID, pci_dev))) {
> +		struct acpi_device *adev;
> +		int constraint;
> +
> +		if (!pci_is_pcie(pci_dev) ||
> +		    !(pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT))
> +			continue;
> +
> +		if (pci_dev->current_state == PCI_D3hot ||
> +		    pci_dev->current_state == PCI_D3cold)
> +			continue;
> +
> +		adev = ACPI_COMPANION(&pci_dev->dev);
> +		if (!adev)
> +			continue;
> +
> +		constraint = acpi_get_lps0_constraint(adev);
> +		if (constraint != ACPI_STATE_UNKNOWN &&
> +		    constraint >= ACPI_STATE_S3)
> +			continue;
> +
> +		if (pci_dev->bridge_d3 == 0)
> +			continue;
> +		pci_dev->bridge_d3 = 0;
> +		dev_info(&pci_dev->dev, "Disabling D3 on PCIe root port due lack of constraint\n");
> +	}
> +
> +	return 0;
> +}
> +
>  static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
>  {
>  	struct rtc_device *rtc_device;
> @@ -893,6 +928,10 @@ static int amd_pmc_suspend_handler(struct device *dev)
>  	case AMD_CPU_ID_CZN:
>  		rc = amd_pmc_czn_wa_irq1(pdev);
>  		break;
> +	case AMD_CPU_ID_YC:
> +	case AMD_CPU_ID_PS:
> +		rc = amd_pmc_rp_wa(pdev);
> +		break;
>  	default:
>  		break;
>  	}
> 

Re: [PATCH v16 0/3] Avoid PCIe D3 for AMD PCIe root ports

[Hans de Goede]

Hi Mario,

On 8/29/23 19:12, Mario Limonciello wrote:
> D3 on PCIe root ports isn't used on Windows systems in Modern Standby.
> This series adjusts the amd-pmc driver to choose the same strategy
> for Rembrandt and Phoenix platforms in Linux with s2idle.
> 
> LPS0 constraints are the basis for it; which if they are added for
> Windows would also apply for Linux as well.
> 
> This version doesn't incorporate a callback, as it's pending feedback
> from Bjorn if that approach is amenable.
> 
> NOTE:
> This series relies upon changes that are both in linux-pm.git and
> platform-x86.git. So it won't be able to apply to either maintainer's
> tree until later.
> 
> Mario Limonciello (3):
>   ACPI: x86: s2idle: Export symbol for fetching constraints for module
>     use
>   platform/x86/amd: pmc: Adjust workarounds to be part of a switch/case
>   platform/x86/amd: pmc: Don't let PCIe root ports go into D3

Thank you for the new version.

I understand you wanted to get this new approach "out there" but
this does not address my remarks on v15:

https://lore.kernel.org/platform-driver-x86/53d26a63-64f3-e736-99f5-32bf4b5ba31d@redhat.com/

Bjorn, I suggest to allow platform code to register a callback
to influence pci_bridge_d3_possible() results there. Can you
take a look at this and let us know what you think of this
suggestion ?

Looking at this problem again and rereading the commit message
of "platform/x86/amd: pmc: Don't let PCIe root ports go into D3"

I see that the problem is that the PCIe root ports to which
the USB controllers connect should not be allowed to go
into D3 when an USB child of them is configured to wakeup
the system.

It seems to me that given that problem description,
we should not be directly messing with the bridge_d3
setting at all.

Instead the XHCI code should have an AMD specific quirk
where it either unconditionally calls pci_d3cold_disable()
on the XHCI PCIe device; or it could even try to be smart
and call pci_d3cold_enable() / pci_d3cold_disable()
from its (runtime)suspend handler depending on if any
USB child is configured as a system wakeup source.

Note that it is safe to repeatedly call pci_d3cold_enable()
/ _disable() there is no need to balance the calls.

Regards,

Hans

Re: [PATCH v16 3/3] platform/x86/amd: pmc: Don't let PCIe root ports go into D3

[Hans de Goede]

Hi Shyam,

On 9/5/23 12:08, Shyam Sundar S K wrote:
> 
> 
> On 8/29/2023 10:42 PM, Mario Limonciello wrote:
>> commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>> changed pci_bridge_d3_possible() so that any vendor's PCIe ports
>> from modern machines (>=2015) are allowed to be put into D3.
>>
>> Iain reports that USB devices can't be used to wake a Lenovo Z13
>> from suspend. This is because the PCIe root port has been put
>> into D3 and AMD's platform can't handle USB devices waking from
>> a hardware sleep state in this case.
>>
>> This problem only occurs on Linux, and only when the AMD PMC driver
>> is utilized to put the device into a hardware sleep state. Comparing
>> the behavior on Windows and Linux, Windows doesn't put the root ports
>> into D3.
>>
>> A variety of approaches were discussed to change PCI core to handle this
>> case generically but no consensus was reached. To limit the scope of
>> effect only to the affected machines introduce a workaround into the
>> amd-pmc driver to only apply to the PCI root ports in affected machines
>> when going into hardware sleep.
>>
>> Link: https://lore.kernel.org/linux-pci/20230818193932.27187-1-mario.limonciello@amd.com/
>> 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
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> See if this change can be moved to pmc-quirks.c, besides that change
> looks good to me. Thank you.
> 
> Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

Thank you for the review.

I also just replied to this series (to the cover-letter)
with an alternative approach based on making the
XHCI driver call pci_d3cold_disable() on the XHCI
PCIe-device on affected AMD chipsets.

That seems like a cleaner approach to me. I wonder
if you have any remarks about that approach ?

Regards,

Hans


> 
>> ---
>> v15->v16:
>>  * Only match PCIe root ports with ACPI companions
>>  * Use constraints when workaround activated
>> ---
>>  drivers/platform/x86/amd/pmc/pmc.c | 39 ++++++++++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
>> index eb2a4263814c..6a037447ec5a 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>> @@ -741,6 +741,41 @@ static int amd_pmc_czn_wa_irq1(struct amd_pmc_dev *pdev)
>>  	return 0;
>>  }
>>  
>> +/* only allow PCIe root ports with a LPS0 constraint configured to go to D3 */
>> +static int amd_pmc_rp_wa(struct amd_pmc_dev *pdev)
>> +{
>> +	struct pci_dev *pci_dev = NULL;
>> +
>> +	while ((pci_dev = pci_get_device(PCI_VENDOR_ID_AMD, PCI_ANY_ID, pci_dev))) {
>> +		struct acpi_device *adev;
>> +		int constraint;
>> +
>> +		if (!pci_is_pcie(pci_dev) ||
>> +		    !(pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT))
>> +			continue;
>> +
>> +		if (pci_dev->current_state == PCI_D3hot ||
>> +		    pci_dev->current_state == PCI_D3cold)
>> +			continue;
>> +
>> +		adev = ACPI_COMPANION(&pci_dev->dev);
>> +		if (!adev)
>> +			continue;
>> +
>> +		constraint = acpi_get_lps0_constraint(adev);
>> +		if (constraint != ACPI_STATE_UNKNOWN &&
>> +		    constraint >= ACPI_STATE_S3)
>> +			continue;
>> +
>> +		if (pci_dev->bridge_d3 == 0)
>> +			continue;
>> +		pci_dev->bridge_d3 = 0;
>> +		dev_info(&pci_dev->dev, "Disabling D3 on PCIe root port due lack of constraint\n");
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
>>  {
>>  	struct rtc_device *rtc_device;
>> @@ -893,6 +928,10 @@ static int amd_pmc_suspend_handler(struct device *dev)
>>  	case AMD_CPU_ID_CZN:
>>  		rc = amd_pmc_czn_wa_irq1(pdev);
>>  		break;
>> +	case AMD_CPU_ID_YC:
>> +	case AMD_CPU_ID_PS:
>> +		rc = amd_pmc_rp_wa(pdev);
>> +		break;
>>  	default:
>>  		break;
>>  	}
>>
> 

Re: [PATCH v16 0/3] Avoid PCIe D3 for AMD PCIe root ports

[Mario Limonciello]

On 9/5/2023 05:13, Hans de Goede wrote:
> Hi Mario,
> 
> On 8/29/23 19:12, Mario Limonciello wrote:
>> D3 on PCIe root ports isn't used on Windows systems in Modern Standby.
>> This series adjusts the amd-pmc driver to choose the same strategy
>> for Rembrandt and Phoenix platforms in Linux with s2idle.
>>
>> LPS0 constraints are the basis for it; which if they are added for
>> Windows would also apply for Linux as well.
>>
>> This version doesn't incorporate a callback, as it's pending feedback
>> from Bjorn if that approach is amenable.
>>
>> NOTE:
>> This series relies upon changes that are both in linux-pm.git and
>> platform-x86.git. So it won't be able to apply to either maintainer's
>> tree until later.
>>
>> Mario Limonciello (3):
>>    ACPI: x86: s2idle: Export symbol for fetching constraints for module
>>      use
>>    platform/x86/amd: pmc: Adjust workarounds to be part of a switch/case
>>    platform/x86/amd: pmc: Don't let PCIe root ports go into D3
> 
> Thank you for the new version.
> 
> I understand you wanted to get this new approach "out there" but
> this does not address my remarks on v15:
> 
> https://lore.kernel.org/platform-driver-x86/53d26a63-64f3-e736-99f5-32bf4b5ba31d@redhat.com/
> 

Right; I called out in the cover letter this is pending feedback from Bjorn.

> Bjorn, I suggest to allow platform code to register a callback
> to influence pci_bridge_d3_possible() results there. Can you
> take a look at this and let us know what you think of this
> suggestion ?
> 
> Looking at this problem again and rereading the commit message
> of "platform/x86/amd: pmc: Don't let PCIe root ports go into D3"
> 
> I see that the problem is that the PCIe root ports to which
> the USB controllers connect should not be allowed to go
> into D3 when an USB child of them is configured to wakeup
> the system.
> 
> It seems to me that given that problem description,
> we should not be directly messing with the bridge_d3
> setting at all.
> 
> Instead the XHCI code should have an AMD specific quirk
> where it either unconditionally calls pci_d3cold_disable()
> on the XHCI PCIe device; or it could even try to be smart
> and call pci_d3cold_enable() / pci_d3cold_disable()
> from its (runtime)suspend handler depending on if any
> USB child is configured as a system wakeup source.
> 
> Note that it is safe to repeatedly call pci_d3cold_enable()
> / _disable() there is no need to balance the calls.
> 

It's only the PCIe root port that is used for XHCI tunneling that has 
this issue.  This specific problem is NOT for the root port of "any" AMD 
XHCI controllers.  There is no problem with any of the XHCI controllers
going into D3hot.

So if a quirk was used in the XHCI driver, it's going to mean examining 
the topology of the PCI devices to find the right one.  I really don't 
think this is a scalable way to do it.

The big advantage of the way this quirking is done now is that it should 
mirror how Windows makes the decision.  On Windows the uPEP ACPI driver 
uses the constraints to orchestrate the desired ACPI states for Modern 
Standby.  In Linux we can then use the matching driver (amd-pmc) to make 
the decision.

Re: [PATCH v16 3/3] platform/x86/amd: pmc: Don't let PCIe root ports go into D3

[Mario Limonciello]

On 9/5/2023 05:15, Hans de Goede wrote:
> Hi Shyam,
> 
> On 9/5/23 12:08, Shyam Sundar S K wrote:
>>
>>
>> On 8/29/2023 10:42 PM, Mario Limonciello wrote:
>>> commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>>> changed pci_bridge_d3_possible() so that any vendor's PCIe ports
>>> from modern machines (>=2015) are allowed to be put into D3.
>>>
>>> Iain reports that USB devices can't be used to wake a Lenovo Z13
>>> from suspend. This is because the PCIe root port has been put
>>> into D3 and AMD's platform can't handle USB devices waking from
>>> a hardware sleep state in this case.
>>>
>>> This problem only occurs on Linux, and only when the AMD PMC driver
>>> is utilized to put the device into a hardware sleep state. Comparing
>>> the behavior on Windows and Linux, Windows doesn't put the root ports
>>> into D3.
>>>
>>> A variety of approaches were discussed to change PCI core to handle this
>>> case generically but no consensus was reached. To limit the scope of
>>> effect only to the affected machines introduce a workaround into the
>>> amd-pmc driver to only apply to the PCI root ports in affected machines
>>> when going into hardware sleep.
>>>
>>> Link: https://lore.kernel.org/linux-pci/20230818193932.27187-1-mario.limonciello@amd.com/
>>> 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
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>
>> See if this change can be moved to pmc-quirks.c, besides that change
>> looks good to me. Thank you.
>>
>> Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> 
> Thank you for the review.
> 
> I also just replied to this series (to the cover-letter)
> with an alternative approach based on making the
> XHCI driver call pci_d3cold_disable() on the XHCI
> PCIe-device on affected AMD chipsets.
> 
> That seems like a cleaner approach to me. I wonder
> if you have any remarks about that approach ?
> 

I was thinking more about Hans' comments to the cover letter as well as 
Shyam's comments to move it into pmc-quirks.c.

Perhaps it would better be conveying what's going on by having a 
dedicated step that amd-pmc calls pci_choose_state() for each PCIe 
device and checks the value against the constraints.  If "any" of them 
are mismatched it could emit a message.  This is a little bit of 
duplication though because drivers/acpi/x86/s2idle.c already also emits 
a similar message for some devices when pm_debug_messages is enabled.

Then the special case would be for PCIe root ports that are mismatched 
the driver overrides it.  If this logic change is wouldn't make sense 
for it to be moved into pmc-quirks.c.

I don't think using pci_d3cold_disable() / pci_d3cold_enable() is 
correct though.  If PCI core stays the same it should still be setting 
pdev->bridge_d3 to zero.  The problem isn't with D3cold on the PCIe RP 
at s2didle, it's with D3hot.


> Regards,
> 
> Hans
> 
> 
>>
>>> ---
>>> v15->v16:
>>>   * Only match PCIe root ports with ACPI companions
>>>   * Use constraints when workaround activated
>>> ---
>>>   drivers/platform/x86/amd/pmc/pmc.c | 39 ++++++++++++++++++++++++++++++
>>>   1 file changed, 39 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
>>> index eb2a4263814c..6a037447ec5a 100644
>>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>>> @@ -741,6 +741,41 @@ static int amd_pmc_czn_wa_irq1(struct amd_pmc_dev *pdev)
>>>   	return 0;
>>>   }
>>>   
>>> +/* only allow PCIe root ports with a LPS0 constraint configured to go to D3 */
>>> +static int amd_pmc_rp_wa(struct amd_pmc_dev *pdev)
>>> +{
>>> +	struct pci_dev *pci_dev = NULL;
>>> +
>>> +	while ((pci_dev = pci_get_device(PCI_VENDOR_ID_AMD, PCI_ANY_ID, pci_dev))) {
>>> +		struct acpi_device *adev;
>>> +		int constraint;
>>> +
>>> +		if (!pci_is_pcie(pci_dev) ||
>>> +		    !(pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT))
>>> +			continue;
>>> +
>>> +		if (pci_dev->current_state == PCI_D3hot ||
>>> +		    pci_dev->current_state == PCI_D3cold)
>>> +			continue;
>>> +
>>> +		adev = ACPI_COMPANION(&pci_dev->dev);
>>> +		if (!adev)
>>> +			continue;
>>> +
>>> +		constraint = acpi_get_lps0_constraint(adev);
>>> +		if (constraint != ACPI_STATE_UNKNOWN &&
>>> +		    constraint >= ACPI_STATE_S3)
>>> +			continue;
>>> +
>>> +		if (pci_dev->bridge_d3 == 0)
>>> +			continue;
>>> +		pci_dev->bridge_d3 = 0;
>>> +		dev_info(&pci_dev->dev, "Disabling D3 on PCIe root port due lack of constraint\n");
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
>>>   {
>>>   	struct rtc_device *rtc_device;
>>> @@ -893,6 +928,10 @@ static int amd_pmc_suspend_handler(struct device *dev)
>>>   	case AMD_CPU_ID_CZN:
>>>   		rc = amd_pmc_czn_wa_irq1(pdev);
>>>   		break;
>>> +	case AMD_CPU_ID_YC:
>>> +	case AMD_CPU_ID_PS:
>>> +		rc = amd_pmc_rp_wa(pdev);
>>> +		break;
>>>   	default:
>>>   		break;
>>>   	}
>>>
>>
> 

Re: [PATCH v16 3/3] platform/x86/amd: pmc: Don't let PCIe root ports go into D3

[Rafael J. Wysocki]

On Tue, Sep 5, 2023 at 9:57 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 9/5/2023 05:15, Hans de Goede wrote:
> > Hi Shyam,
> >
> > On 9/5/23 12:08, Shyam Sundar S K wrote:
> >>
> >>
> >> On 8/29/2023 10:42 PM, Mario Limonciello wrote:
> >>> commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> >>> changed pci_bridge_d3_possible() so that any vendor's PCIe ports
> >>> from modern machines (>=2015) are allowed to be put into D3.
> >>>
> >>> Iain reports that USB devices can't be used to wake a Lenovo Z13
> >>> from suspend. This is because the PCIe root port has been put
> >>> into D3 and AMD's platform can't handle USB devices waking from
> >>> a hardware sleep state in this case.
> >>>
> >>> This problem only occurs on Linux, and only when the AMD PMC driver
> >>> is utilized to put the device into a hardware sleep state. Comparing
> >>> the behavior on Windows and Linux, Windows doesn't put the root ports
> >>> into D3.
> >>>
> >>> A variety of approaches were discussed to change PCI core to handle this
> >>> case generically but no consensus was reached. To limit the scope of
> >>> effect only to the affected machines introduce a workaround into the
> >>> amd-pmc driver to only apply to the PCI root ports in affected machines
> >>> when going into hardware sleep.
> >>>
> >>> Link: https://lore.kernel.org/linux-pci/20230818193932.27187-1-mario.limonciello@amd.com/
> >>> 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
> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >>
> >> See if this change can be moved to pmc-quirks.c, besides that change
> >> looks good to me. Thank you.
> >>
> >> Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >
> > Thank you for the review.
> >
> > I also just replied to this series (to the cover-letter)
> > with an alternative approach based on making the
> > XHCI driver call pci_d3cold_disable() on the XHCI
> > PCIe-device on affected AMD chipsets.
> >
> > That seems like a cleaner approach to me. I wonder
> > if you have any remarks about that approach ?
> >
>
> I was thinking more about Hans' comments to the cover letter as well as
> Shyam's comments to move it into pmc-quirks.c.
>
> Perhaps it would better be conveying what's going on by having a
> dedicated step that amd-pmc calls pci_choose_state() for each PCIe
> device and checks the value against the constraints.  If "any" of them
> are mismatched it could emit a message.  This is a little bit of
> duplication though because drivers/acpi/x86/s2idle.c already also emits
> a similar message for some devices when pm_debug_messages is enabled.
>
> Then the special case would be for PCIe root ports that are mismatched
> the driver overrides it.  If this logic change is wouldn't make sense
> for it to be moved into pmc-quirks.c.
>
> I don't think using pci_d3cold_disable() / pci_d3cold_enable() is
> correct though.  If PCI core stays the same it should still be setting
> pdev->bridge_d3 to zero.  The problem isn't with D3cold on the PCIe RP
> at s2didle, it's with D3hot.

Well, it's not even that.

If there were no devices expected to wake up the system from sleep
under the given Root Port, it might very well go into D3hot IIUC, so
the wakeup capability seems to be the key property here.

I need to think a bit more about this.

Re: [PATCH v16 3/3] platform/x86/amd: pmc: Don't let PCIe root ports go into D3

[Bjorn Helgaas]

[+cc Hans]

On Tue, Aug 29, 2023 at 12:12:12PM -0500, Mario Limonciello wrote:
> commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> changed pci_bridge_d3_possible() so that any vendor's PCIe ports
> from modern machines (>=2015) are allowed to be put into D3.
> 
> Iain reports that USB devices can't be used to wake a Lenovo Z13
> from suspend. This is because the PCIe root port has been put
> into D3 and AMD's platform can't handle USB devices waking from
> a hardware sleep state in this case.

Can you be specific in the subject and commit log about whether "D3"
refers to "D3hot", "D3cold", or both?  It's probably obvious to PM
folks, but it's always a stumbling block for me.

I assume "can't handle USB devices waking" does not refer to a problem
with the USB adapter and whatever mechanism it uses to send a wakeup
event to request that power be turned on, but rather it means that the
wakeup event doesn't get propagated through the Root Port?

Is this actually specific to USB devices?  Or could a NIC below the
Root Port suffer the same problem when a wake-on-lan packet causes it
to send a wakeup event?  It seems like we've had this conversation
before; sorry to ask the same questions again.

If it's not specific to USB, I would say something like "when the Root
Port is in D3cold, wakeup events from devices below it are lost" (or
whatever the actual problem is).

> This problem only occurs on Linux, and only when the AMD PMC driver
> is utilized to put the device into a hardware sleep state.

Is the AMD PMC driver doing something magic that can't be done via
other power management paths?  That's what "only when the AMD PMC
driver is utilized" suggests.  But if the problem occurs when the Root
Port is put into D3cold via *any* means, just say that.

And if you can say a specific PCI power state instead of "hardware
sleep state", that would be good, too.

> Comparing
> the behavior on Windows and Linux, Windows doesn't put the root ports
> into D3.
> 
> A variety of approaches were discussed to change PCI core to handle this
> case generically but no consensus was reached. To limit the scope of
> effect only to the affected machines introduce a workaround into the
> amd-pmc driver to only apply to the PCI root ports in affected machines
> when going into hardware sleep.

> +/* only allow PCIe root ports with a LPS0 constraint configured to go to D3 */
> +static int amd_pmc_rp_wa(struct amd_pmc_dev *pdev)
> +{
> +	struct pci_dev *pci_dev = NULL;
> +
> +	while ((pci_dev = pci_get_device(PCI_VENDOR_ID_AMD, PCI_ANY_ID, pci_dev))) {

I hate to add more uses of pci_get_device() because it doesn't account
for hot-added devices.  Maybe there's no need to support hot-add of
AMD Root Ports, but that's not obvious to readers here.

One mechanism to avoid pci_get_device() is to use quirks, although it
might be hard to deal with PCI/ACPI ordering issues.

> +		struct acpi_device *adev;
> +		int constraint;
> +
> +		if (!pci_is_pcie(pci_dev) ||
> +		    !(pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT))
> +			continue;
> +
> +		if (pci_dev->current_state == PCI_D3hot ||
> +		    pci_dev->current_state == PCI_D3cold)
> +			continue;

If we're trying to determine a property of the device, why does the
current power state make a difference?

It looks like this loop runs every time we suspend (from
amd_pmc_suspend_handler()), even though this is something we should
know at boot-time, so we only need it once.

> +		adev = ACPI_COMPANION(&pci_dev->dev);
> +		if (!adev)
> +			continue;
> +
> +		constraint = acpi_get_lps0_constraint(adev);
> +		if (constraint != ACPI_STATE_UNKNOWN &&
> +		    constraint >= ACPI_STATE_S3)
> +			continue;
> +
> +		if (pci_dev->bridge_d3 == 0)
> +			continue;
> +		pci_dev->bridge_d3 = 0;
> +		dev_info(&pci_dev->dev, "Disabling D3 on PCIe root port due lack of constraint\n");

D3hot?  D3cold?  Both?  "lack of constraint"?

> +	}
> +
> +	return 0;
> +}
> +
>  static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
>  {
>  	struct rtc_device *rtc_device;
> @@ -893,6 +928,10 @@ static int amd_pmc_suspend_handler(struct device *dev)
>  	case AMD_CPU_ID_CZN:
>  		rc = amd_pmc_czn_wa_irq1(pdev);
>  		break;
> +	case AMD_CPU_ID_YC:
> +	case AMD_CPU_ID_PS:
> +		rc = amd_pmc_rp_wa(pdev);
> +		break;
>  	default:
>  		break;
>  	}
> -- 
> 2.34.1
> 

Re: [PATCH v16 3/3] platform/x86/amd: pmc: Don't let PCIe root ports go into D3

[Mario Limonciello]

On 9/5/2023 15:51, Bjorn Helgaas wrote:
> [+cc Hans]
> 
> On Tue, Aug 29, 2023 at 12:12:12PM -0500, Mario Limonciello wrote:
>> commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>> changed pci_bridge_d3_possible() so that any vendor's PCIe ports
>> from modern machines (>=2015) are allowed to be put into D3.
>>
>> Iain reports that USB devices can't be used to wake a Lenovo Z13
>> from suspend. This is because the PCIe root port has been put
>> into D3 and AMD's platform can't handle USB devices waking from
>> a hardware sleep state in this case.
> 
> Can you be specific in the subject and commit log about whether "D3"
> refers to "D3hot", "D3cold", or both?  It's probably obvious to PM
> folks, but it's always a stumbling block for me.
> 
> I assume "can't handle USB devices waking" does not refer to a problem
> with the USB adapter and whatever mechanism it uses to send a wakeup
> event to request that power be turned on, but rather it means that the
> wakeup event doesn't get propagated through the Root Port?
> 
> Is this actually specific to USB devices?  Or could a NIC below the
> Root Port suffer the same problem when a wake-on-lan packet causes it
> to send a wakeup event?  It seems like we've had this conversation
> before; sorry to ask the same questions again.
> 
> If it's not specific to USB, I would say something like "when the Root
> Port is in D3cold, wakeup events from devices below it are lost" (or
> whatever the actual problem is).

The problem is specific to the root port in D3hot over s2idle after the 
hardware has entered the deepest state.

It's fine any other time.

This particular root port only connects to the XHCI controllers and USB4 
controllers, so I can't confirm whether anything else is affected.

> 
>> This problem only occurs on Linux, and only when the AMD PMC driver
>> is utilized to put the device into a hardware sleep state.
> 
> Is the AMD PMC driver doing something magic that can't be done via
> other power management paths?  That's what "only when the AMD PMC
> driver is utilized" suggests.  But if the problem occurs when the Root
> Port is put into D3cold via *any* means, just say that.
> 
> And if you can say a specific PCI power state instead of "hardware
> sleep state", that would be good, too.

Yes; the AMD PMC driver does a notification to the platform that the OS 
is ready for it to go into a hardware sleep state [1].

If the AMD PMC driver isn't used, the platform is not notified that the 
OS is ready for it to go into hardware sleep state, and this issue will 
not occur.

So the PCI root port being in D3 while the hardware is in a sleep state 
is very accurate.

[1] 
https://github.com/torvalds/linux/blob/v6.5/drivers/platform/x86/amd/pmc.c#L816

> 
>> Comparing
>> the behavior on Windows and Linux, Windows doesn't put the root ports
>> into D3.
>>
>> A variety of approaches were discussed to change PCI core to handle this
>> case generically but no consensus was reached. To limit the scope of
>> effect only to the affected machines introduce a workaround into the
>> amd-pmc driver to only apply to the PCI root ports in affected machines
>> when going into hardware sleep.
> 
>> +/* only allow PCIe root ports with a LPS0 constraint configured to go to D3 */
>> +static int amd_pmc_rp_wa(struct amd_pmc_dev *pdev)
>> +{
>> +	struct pci_dev *pci_dev = NULL;
>> +
>> +	while ((pci_dev = pci_get_device(PCI_VENDOR_ID_AMD, PCI_ANY_ID, pci_dev))) {
> 
> I hate to add more uses of pci_get_device() because it doesn't account
> for hot-added devices.  Maybe there's no need to support hot-add of
> AMD Root Ports, but that's not obvious to readers here.

This function is only called during suspend, so it should cover hot 
added / hot removed devices.

If this ends up staying for v17 as is I'll add more verbose comments.

> 
> One mechanism to avoid pci_get_device() is to use quirks, although it
> might be hard to deal with PCI/ACPI ordering issues

I did quirks in an earlier version of this series, but you had feedback 
that the solution isn't scalable.  That's why it's morphed into this 
approach, which I'd like to think is more scalable as it looks at the 
constraints advertised by the platform in an AMD specific driver.

> 
>> +		struct acpi_device *adev;
>> +		int constraint;
>> +
>> +		if (!pci_is_pcie(pci_dev) ||
>> +		    !(pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT))
>> +			continue;
>> +
>> +		if (pci_dev->current_state == PCI_D3hot ||
>> +		    pci_dev->current_state == PCI_D3cold)
>> +			continue;
> 
> If we're trying to determine a property of the device, why does the
> current power state make a difference?

Hans left feedback in v15 that if the device was already in D3 at the 
time of this function it wouldn't work properly.  So I excluded those 
devices.

> 
> It looks like this loop runs every time we suspend (from
> amd_pmc_suspend_handler()), even though this is something we should
> know at boot-time, so we only need it once.

It's was because pci_bridge_d3_update() can be called and change it 
again in other places.

I think if we want to optimize it to only run a single time we need a 
new variable or bit in the pci_dev structure that can be used to mark 
such an exclusion which pci_bridge_d3_update() could take into account.

This could fit in well with Hans' idea of drivers could register a 
callback to "veto" D3 support.  It could be something like 
pci_bridge_d3_update() is called whenever a new driver 
registers/unregisters the callback.  It might also fit in well with your 
previous comments about how you want to separate "spec compliant" things 
and "quirk" things in pci_bridge_d3_possible().

Could you comment on that?  He suggested it in the cover letter responses.

> 
>> +		adev = ACPI_COMPANION(&pci_dev->dev);
>> +		if (!adev)
>> +			continue;
>> +
>> +		constraint = acpi_get_lps0_constraint(adev);
>> +		if (constraint != ACPI_STATE_UNKNOWN &&
>> +		    constraint >= ACPI_STATE_S3)
>> +			continue;
>> +
>> +		if (pci_dev->bridge_d3 == 0)
>> +			continue;
>> +		pci_dev->bridge_d3 = 0;
>> +		dev_info(&pci_dev->dev, "Disabling D3 on PCIe root port due lack of constraint\n");
> 
> D3hot?  D3cold?  Both?  "lack of constraint"?

It's disabling both, which is why I left it as D3 to cover both.  The 
lack of constraint can't be explained in a single line message.  If this 
is too noisy for a user and you think would cause more questions than 
help I'm fine to downgrade it to debug.

> 
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
>>   {
>>   	struct rtc_device *rtc_device;
>> @@ -893,6 +928,10 @@ static int amd_pmc_suspend_handler(struct device *dev)
>>   	case AMD_CPU_ID_CZN:
>>   		rc = amd_pmc_czn_wa_irq1(pdev);
>>   		break;
>> +	case AMD_CPU_ID_YC:
>> +	case AMD_CPU_ID_PS:
>> +		rc = amd_pmc_rp_wa(pdev);
>> +		break;
>>   	default:
>>   		break;
>>   	}
>> -- 
>> 2.34.1
>>

Re: [PATCH v16 0/3] Avoid PCIe D3 for AMD PCIe root ports

[Hans de Goede]

Hi Mario,

On 9/5/23 14:45, Mario Limonciello wrote:
> On 9/5/2023 05:13, Hans de Goede wrote:
>> Hi Mario,
>>
>> On 8/29/23 19:12, Mario Limonciello wrote:
>>> D3 on PCIe root ports isn't used on Windows systems in Modern Standby.
>>> This series adjusts the amd-pmc driver to choose the same strategy
>>> for Rembrandt and Phoenix platforms in Linux with s2idle.
>>>
>>> LPS0 constraints are the basis for it; which if they are added for
>>> Windows would also apply for Linux as well.
>>>
>>> This version doesn't incorporate a callback, as it's pending feedback
>>> from Bjorn if that approach is amenable.
>>>
>>> NOTE:
>>> This series relies upon changes that are both in linux-pm.git and
>>> platform-x86.git. So it won't be able to apply to either maintainer's
>>> tree until later.
>>>
>>> Mario Limonciello (3):
>>>    ACPI: x86: s2idle: Export symbol for fetching constraints for module
>>>      use
>>>    platform/x86/amd: pmc: Adjust workarounds to be part of a switch/case
>>>    platform/x86/amd: pmc: Don't let PCIe root ports go into D3
>>
>> Thank you for the new version.
>>
>> I understand you wanted to get this new approach "out there" but
>> this does not address my remarks on v15:
>>
>> https://lore.kernel.org/platform-driver-x86/53d26a63-64f3-e736-99f5-32bf4b5ba31d@redhat.com/
>>
> 
> Right; I called out in the cover letter this is pending feedback from Bjorn.
> 
>> Bjorn, I suggest to allow platform code to register a callback
>> to influence pci_bridge_d3_possible() results there. Can you
>> take a look at this and let us know what you think of this
>> suggestion ?
>>
>> Looking at this problem again and rereading the commit message
>> of "platform/x86/amd: pmc: Don't let PCIe root ports go into D3"
>>
>> I see that the problem is that the PCIe root ports to which
>> the USB controllers connect should not be allowed to go
>> into D3 when an USB child of them is configured to wakeup
>> the system.
>>
>> It seems to me that given that problem description,
>> we should not be directly messing with the bridge_d3
>> setting at all.
>>
>> Instead the XHCI code should have an AMD specific quirk
>> where it either unconditionally calls pci_d3cold_disable()
>> on the XHCI PCIe device; or it could even try to be smart
>> and call pci_d3cold_enable() / pci_d3cold_disable()
>> from its (runtime)suspend handler depending on if any
>> USB child is configured as a system wakeup source.
>>
>> Note that it is safe to repeatedly call pci_d3cold_enable()
>> / _disable() there is no need to balance the calls.
>>
> 
> It's only the PCIe root port that is used for XHCI tunneling that has this issue.  This specific problem is NOT for the root port of "any" AMD XHCI controllers.  There is no problem with any of the XHCI controllers
> going into D3hot.

"XHCI tunneling" is an unfamiliar term for me. Are we talking about a XHCI controller inside a USB4/thunderbold dock here which is connected to the laptop over PCIe tunneling over thunderbolt ?

Or do you mean the XHCI controller inside the laptop which is connected to a USB4/thunderbolt capable Type-C port which is used when that port is in USB3/USB2 mode ?

As long as the XHCI controller is inside the laptop (and not in the dock), presumably you can identify it by say a set of PCI device-ids of the "tunneling" XHCI controllers on affected AMD platforms. So you could then still call pci_d3cold_disable() from the XHCI driver on only those controllers.

Note I'm not saying this is the best solution. I'm just trying to understand what you mean with " the PCIe root port that is used for XHCI tunneling" .

I also see that Rafael has said elsewhere in the thread that he needs to think a bit about how to best handle this ...

Regards,

Hans

Re: [PATCH v16 0/3] Avoid PCIe D3 for AMD PCIe root ports

[Mario Limonciello]

On 9/6/2023 07:24, Hans de Goede wrote:
> 
> "XHCI tunneling" is an unfamiliar term for me. Are we talking about a XHCI controller inside a USB4/thunderbold dock here which is connected to the laptop over PCIe tunneling over thunderbolt ?
> 
> Or do you mean the XHCI controller inside the laptop which is connected to a USB4/thunderbolt capable Type-C port which is used when that port is in USB3/USB2 mode ?
> 
> As long as the XHCI controller is inside the laptop (and not in the dock), presumably you can identify it by say a set of PCI device-ids of the "tunneling" XHCI controllers on affected AMD platforms. So you could then still call pci_d3cold_disable() from the XHCI driver on only those controllers.

XHCI tunneling refers to XHCI over USB4 fabric.   The problem isn't with 
the XHCI controllers going to D3 - it's with the root ports they are 
connected to.  And the issue occurs with D3hot.

An earlier version of the series did do something like this where it was 
quirks for the PCI IDs for the root ports but it has two problems:

1) It covers too many things.  The same PCI ID is used for a second root 
port that is unaffected by the issue.  So this means the quirk needs to 
look at the topology to make sure the right device combination is quirked.

2) It doesn't scale.  I don't have any reason to believe the constraints 
requirements change which means we'll be adding new quirks with every 
single new CPU.

> 
> Note I'm not saying this is the best solution. I'm just trying to understand what you mean with " the PCIe root port that is used for XHCI tunneling" .
> 
> I also see that Rafael has said elsewhere in the thread that he needs to think a bit about how to best handle this ...
> 

I have done a prototype for your callback proposal, and I've got 
something working at least for amd-pmc.  It only calls the callback one 
time rather than at suspend.

Unless I get some feedback from Bjorn that the callback proposal is a 
bad idea I'll post something later today.

Re: [PATCH v16 0/3] Avoid PCIe D3 for AMD PCIe root ports

[Rafael J. Wysocki]

On Wed, Sep 6, 2023 at 2:24 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Mario,
>
> On 9/5/23 14:45, Mario Limonciello wrote:
> > On 9/5/2023 05:13, Hans de Goede wrote:
> >> Hi Mario,
> >>
> >> On 8/29/23 19:12, Mario Limonciello wrote:
> >>> D3 on PCIe root ports isn't used on Windows systems in Modern Standby.
> >>> This series adjusts the amd-pmc driver to choose the same strategy
> >>> for Rembrandt and Phoenix platforms in Linux with s2idle.
> >>>
> >>> LPS0 constraints are the basis for it; which if they are added for
> >>> Windows would also apply for Linux as well.
> >>>
> >>> This version doesn't incorporate a callback, as it's pending feedback
> >>> from Bjorn if that approach is amenable.
> >>>
> >>> NOTE:
> >>> This series relies upon changes that are both in linux-pm.git and
> >>> platform-x86.git. So it won't be able to apply to either maintainer's
> >>> tree until later.
> >>>
> >>> Mario Limonciello (3):
> >>>    ACPI: x86: s2idle: Export symbol for fetching constraints for module
> >>>      use
> >>>    platform/x86/amd: pmc: Adjust workarounds to be part of a switch/case
> >>>    platform/x86/amd: pmc: Don't let PCIe root ports go into D3
> >>
> >> Thank you for the new version.
> >>
> >> I understand you wanted to get this new approach "out there" but
> >> this does not address my remarks on v15:
> >>
> >> https://lore.kernel.org/platform-driver-x86/53d26a63-64f3-e736-99f5-32bf4b5ba31d@redhat.com/
> >>
> >
> > Right; I called out in the cover letter this is pending feedback from Bjorn.
> >
> >> Bjorn, I suggest to allow platform code to register a callback
> >> to influence pci_bridge_d3_possible() results there. Can you
> >> take a look at this and let us know what you think of this
> >> suggestion ?
> >>
> >> Looking at this problem again and rereading the commit message
> >> of "platform/x86/amd: pmc: Don't let PCIe root ports go into D3"
> >>
> >> I see that the problem is that the PCIe root ports to which
> >> the USB controllers connect should not be allowed to go
> >> into D3 when an USB child of them is configured to wakeup
> >> the system.
> >>
> >> It seems to me that given that problem description,
> >> we should not be directly messing with the bridge_d3
> >> setting at all.
> >>
> >> Instead the XHCI code should have an AMD specific quirk
> >> where it either unconditionally calls pci_d3cold_disable()
> >> on the XHCI PCIe device; or it could even try to be smart
> >> and call pci_d3cold_enable() / pci_d3cold_disable()
> >> from its (runtime)suspend handler depending on if any
> >> USB child is configured as a system wakeup source.
> >>
> >> Note that it is safe to repeatedly call pci_d3cold_enable()
> >> / _disable() there is no need to balance the calls.
> >>
> >
> > It's only the PCIe root port that is used for XHCI tunneling that has this issue.  This specific problem is NOT for the root port of "any" AMD XHCI controllers.  There is no problem with any of the XHCI controllers
> > going into D3hot.
>
> "XHCI tunneling" is an unfamiliar term for me. Are we talking about a XHCI controller inside a USB4/thunderbold dock here which is connected to the laptop over PCIe tunneling over thunderbolt ?
>
> Or do you mean the XHCI controller inside the laptop which is connected to a USB4/thunderbolt capable Type-C port which is used when that port is in USB3/USB2 mode ?
>
> As long as the XHCI controller is inside the laptop (and not in the dock), presumably you can identify it by say a set of PCI device-ids of the "tunneling" XHCI controllers on affected AMD platforms. So you could then still call pci_d3cold_disable() from the XHCI driver on only those controllers.
>
> Note I'm not saying this is the best solution. I'm just trying to understand what you mean with " the PCIe root port that is used for XHCI tunneling" .
>
> I also see that Rafael has said elsewhere in the thread that he needs to think a bit about how to best handle this ...

Yes, I have, and that's because of the realization that the
requirements may differ depending on whether or not there is a device
(USB or other) enabled to wake up the system from sleep under the Root
Port in question.

Essentially, the problem is that wakeup doesn't work and the
investigation led to the Root Port's power state when suspended, but
that power state only appears to be too deep for the wakeup to work
and not in general.

IIUC, the port can be safely programmed into D3hot and then back to D0
and that works as long as there are no wakeup devices under it (Mario,
please correct me if that's not the case).

Now, when a USB device on the bus segment under the port is configured
for system wakeup, it needs to be able to trigger a wake interrupt
when the system is in the sleep state.  That wake interrupt is not
generated by the USB wakeup device itself, but by the USB controller
handling it.  The USB controller is a PCIe device, so in order to
generate a wake interrupt it needs the link to its parent port to be
up unless it is capable of generating PMEs from D3cold (which only is
the case when it is connected to a separate wake power source and that
is indicated by setting the corresponding bit in its PM Capabilities
Register).  If that is not the case, and its parent port is programmed
into D3hot, that may cause the link between them to go down and so the
wake interrupt cannot be generated.  This means that the port which is
generally allowed to go into D3hot (because why not), may not be
allowed to do so if system wakeup devices are present under it and
that appears to be the missing piece to me.

Re: [PATCH v16 0/3] Avoid PCIe D3 for AMD PCIe root ports

[Mario Limonciello]

On 9/6/2023 13:56, Rafael J. Wysocki wrote:
> On Wed, Sep 6, 2023 at 2:24 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Mario,
>>
>> On 9/5/23 14:45, Mario Limonciello wrote:
>>> On 9/5/2023 05:13, Hans de Goede wrote:
>>>> Hi Mario,
>>>>
>>>> On 8/29/23 19:12, Mario Limonciello wrote:
>>>>> D3 on PCIe root ports isn't used on Windows systems in Modern Standby.
>>>>> This series adjusts the amd-pmc driver to choose the same strategy
>>>>> for Rembrandt and Phoenix platforms in Linux with s2idle.
>>>>>
>>>>> LPS0 constraints are the basis for it; which if they are added for
>>>>> Windows would also apply for Linux as well.
>>>>>
>>>>> This version doesn't incorporate a callback, as it's pending feedback
>>>>> from Bjorn if that approach is amenable.
>>>>>
>>>>> NOTE:
>>>>> This series relies upon changes that are both in linux-pm.git and
>>>>> platform-x86.git. So it won't be able to apply to either maintainer's
>>>>> tree until later.
>>>>>
>>>>> Mario Limonciello (3):
>>>>>     ACPI: x86: s2idle: Export symbol for fetching constraints for module
>>>>>       use
>>>>>     platform/x86/amd: pmc: Adjust workarounds to be part of a switch/case
>>>>>     platform/x86/amd: pmc: Don't let PCIe root ports go into D3
>>>>
>>>> Thank you for the new version.
>>>>
>>>> I understand you wanted to get this new approach "out there" but
>>>> this does not address my remarks on v15:
>>>>
>>>> https://lore.kernel.org/platform-driver-x86/53d26a63-64f3-e736-99f5-32bf4b5ba31d@redhat.com/
>>>>
>>>
>>> Right; I called out in the cover letter this is pending feedback from Bjorn.
>>>
>>>> Bjorn, I suggest to allow platform code to register a callback
>>>> to influence pci_bridge_d3_possible() results there. Can you
>>>> take a look at this and let us know what you think of this
>>>> suggestion ?
>>>>
>>>> Looking at this problem again and rereading the commit message
>>>> of "platform/x86/amd: pmc: Don't let PCIe root ports go into D3"
>>>>
>>>> I see that the problem is that the PCIe root ports to which
>>>> the USB controllers connect should not be allowed to go
>>>> into D3 when an USB child of them is configured to wakeup
>>>> the system.
>>>>
>>>> It seems to me that given that problem description,
>>>> we should not be directly messing with the bridge_d3
>>>> setting at all.
>>>>
>>>> Instead the XHCI code should have an AMD specific quirk
>>>> where it either unconditionally calls pci_d3cold_disable()
>>>> on the XHCI PCIe device; or it could even try to be smart
>>>> and call pci_d3cold_enable() / pci_d3cold_disable()
>>>> from its (runtime)suspend handler depending on if any
>>>> USB child is configured as a system wakeup source.
>>>>
>>>> Note that it is safe to repeatedly call pci_d3cold_enable()
>>>> / _disable() there is no need to balance the calls.
>>>>
>>>
>>> It's only the PCIe root port that is used for XHCI tunneling that has this issue.  This specific problem is NOT for the root port of "any" AMD XHCI controllers.  There is no problem with any of the XHCI controllers
>>> going into D3hot.
>>
>> "XHCI tunneling" is an unfamiliar term for me. Are we talking about a XHCI controller inside a USB4/thunderbold dock here which is connected to the laptop over PCIe tunneling over thunderbolt ?
>>
>> Or do you mean the XHCI controller inside the laptop which is connected to a USB4/thunderbolt capable Type-C port which is used when that port is in USB3/USB2 mode ?
>>
>> As long as the XHCI controller is inside the laptop (and not in the dock), presumably you can identify it by say a set of PCI device-ids of the "tunneling" XHCI controllers on affected AMD platforms. So you could then still call pci_d3cold_disable() from the XHCI driver on only those controllers.
>>
>> Note I'm not saying this is the best solution. I'm just trying to understand what you mean with " the PCIe root port that is used for XHCI tunneling" .
>>
>> I also see that Rafael has said elsewhere in the thread that he needs to think a bit about how to best handle this ...
> 
> Yes, I have, and that's because of the realization that the
> requirements may differ depending on whether or not there is a device
> (USB or other) enabled to wake up the system from sleep under the Root
> Port in question.
> 
> Essentially, the problem is that wakeup doesn't work and the
> investigation led to the Root Port's power state when suspended, but
> that power state only appears to be too deep for the wakeup to work
> and not in general.
> 
> IIUC, the port can be safely programmed into D3hot and then back to D0
> and that works as long as there are no wakeup devices under it (Mario,
> please correct me if that's not the case).

This is correct.

> 
> Now, when a USB device on the bus segment under the port is configured
> for system wakeup, it needs to be able to trigger a wake interrupt
> when the system is in the sleep state.  That wake interrupt is not
> generated by the USB wakeup device itself, but by the USB controller
> handling it.  The USB controller is a PCIe device, so in order to
> generate a wake interrupt it needs the link to its parent port to be
> up unless it is capable of generating PMEs from D3cold (which only is
> the case when it is connected to a separate wake power source and that
> is indicated by setting the corresponding bit in its PM Capabilities
> Register).  If that is not the case, and its parent port is programmed
> into D3hot, that may cause the link between them to go down and so the
> wake interrupt cannot be generated.  This means that the port which is
> generally allowed to go into D3hot (because why not), may not be
> allowed to do so if system wakeup devices are present under it and
> that appears to be the missing piece to me.

At a glance this description makes sense, but the logic in 
pci_target_state() already accounts for finding the matching states that 
PME can be generated from.

 From one of these systems this is the problematic root port:

pci 0000:00:08.3: PME# supported from D0 D3hot D3cold

It's accurate while the system isn't in the deepest state too.
If I rmmod amd-pmc or unbind it then the platform will never be notified 
that the OS said it can go to the deepest state at suspend.

With no changes to the kernel pressing a key on the keyboard works and 
you can see the IRQ that woke the system matches the PCIe PME.

I think what we're at here is likely an unspecified platform behavior 
that is "masked" in Windows due to the way that policy works in Windows 
(constraints will influence what states are selected for integrated 
devices by the uPEP driver).

I'm going to drop my updated series to the mailing list that adds the 
ability for a driver to register a vote like Hans suggested and we can 
see what everyone thinks.

Re: [PATCH v16 0/3] Avoid PCIe D3 for AMD PCIe root ports

[Bjorn Helgaas]

On Wed, Sep 06, 2023 at 08:56:29PM +0200, Rafael J. Wysocki wrote:
> On Wed, Sep 6, 2023 at 2:24 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > On 9/5/23 14:45, Mario Limonciello wrote:
> > > On 9/5/2023 05:13, Hans de Goede wrote:
> > >> On 8/29/23 19:12, Mario Limonciello wrote:
> > >>> D3 on PCIe root ports isn't used on Windows systems in Modern Standby.
> > >>> This series adjusts the amd-pmc driver to choose the same strategy
> > >>> for Rembrandt and Phoenix platforms in Linux with s2idle.
> > >>>
> > >>> LPS0 constraints are the basis for it; which if they are added for
> > >>> Windows would also apply for Linux as well.
> > >>>
> > >>> This version doesn't incorporate a callback, as it's pending feedback
> > >>> from Bjorn if that approach is amenable.
> > >>>
> > >>> NOTE:
> > >>> This series relies upon changes that are both in linux-pm.git and
> > >>> platform-x86.git. So it won't be able to apply to either maintainer's
> > >>> tree until later.
> > >>>
> > >>> Mario Limonciello (3):
> > >>>    ACPI: x86: s2idle: Export symbol for fetching constraints for module
> > >>>      use
> > >>>    platform/x86/amd: pmc: Adjust workarounds to be part of a switch/case
> > >>>    platform/x86/amd: pmc: Don't let PCIe root ports go into D3
> > >>
> > >> Thank you for the new version.
> > >>
> > >> I understand you wanted to get this new approach "out there" but
> > >> this does not address my remarks on v15:
> > >>
> > >> https://lore.kernel.org/platform-driver-x86/53d26a63-64f3-e736-99f5-32bf4b5ba31d@redhat.com/
> > >>
> > >
> > > Right; I called out in the cover letter this is pending feedback from Bjorn.
> > >
> > >> Bjorn, I suggest to allow platform code to register a callback
> > >> to influence pci_bridge_d3_possible() results there. Can you
> > >> take a look at this and let us know what you think of this
> > >> suggestion ?
> > >>
> > >> Looking at this problem again and rereading the commit message
> > >> of "platform/x86/amd: pmc: Don't let PCIe root ports go into D3"
> > >>
> > >> I see that the problem is that the PCIe root ports to which
> > >> the USB controllers connect should not be allowed to go
> > >> into D3 when an USB child of them is configured to wakeup
> > >> the system.
> > >>
> > >> It seems to me that given that problem description,
> > >> we should not be directly messing with the bridge_d3
> > >> setting at all.
> > >>
> > >> Instead the XHCI code should have an AMD specific quirk
> > >> where it either unconditionally calls pci_d3cold_disable()
> > >> on the XHCI PCIe device; or it could even try to be smart
> > >> and call pci_d3cold_enable() / pci_d3cold_disable()
> > >> from its (runtime)suspend handler depending on if any
> > >> USB child is configured as a system wakeup source.
> > >>
> > >> Note that it is safe to repeatedly call pci_d3cold_enable()
> > >> / _disable() there is no need to balance the calls.
> > >>
> > >
> > > It's only the PCIe root port that is used for XHCI tunneling that has this issue.  This specific problem is NOT for the root port of "any" AMD XHCI controllers.  There is no problem with any of the XHCI controllers
> > > going into D3hot.
> >
> > "XHCI tunneling" is an unfamiliar term for me. Are we talking about a XHCI controller inside a USB4/thunderbold dock here which is connected to the laptop over PCIe tunneling over thunderbolt ?
> >
> > Or do you mean the XHCI controller inside the laptop which is connected to a USB4/thunderbolt capable Type-C port which is used when that port is in USB3/USB2 mode ?
> >
> > As long as the XHCI controller is inside the laptop (and not in the dock), presumably you can identify it by say a set of PCI device-ids of the "tunneling" XHCI controllers on affected AMD platforms. So you could then still call pci_d3cold_disable() from the XHCI driver on only those controllers.
> >
> > Note I'm not saying this is the best solution. I'm just trying to understand what you mean with " the PCIe root port that is used for XHCI tunneling" .
> >
> > I also see that Rafael has said elsewhere in the thread that he needs to think a bit about how to best handle this ...
> 
> Yes, I have, and that's because of the realization that the
> requirements may differ depending on whether or not there is a device
> (USB or other) enabled to wake up the system from sleep under the Root
> Port in question.
> 
> Essentially, the problem is that wakeup doesn't work and the
> investigation led to the Root Port's power state when suspended, but
> that power state only appears to be too deep for the wakeup to work
> and not in general.
> 
> IIUC, the port can be safely programmed into D3hot and then back to D0
> and that works as long as there are no wakeup devices under it (Mario,
> please correct me if that's not the case).
> 
> Now, when a USB device on the bus segment under the port is configured
> for system wakeup, it needs to be able to trigger a wake interrupt
> when the system is in the sleep state.  That wake interrupt is not
> generated by the USB wakeup device itself, but by the USB controller
> handling it.  The USB controller is a PCIe device, so in order to
> generate a wake interrupt it needs the link to its parent port to be
> up unless it is capable of generating PMEs from D3cold (which only is
> the case when it is connected to a separate wake power source and that
> is indicated by setting the corresponding bit in its PM Capabilities
> Register).

> If that is not the case, and its parent port is programmed
> into D3hot, that may cause the link between them to go down and so the
> wake interrupt cannot be generated.

If both the USB adapter and the parent port are in D3hot, the link
should stay up, shouldn't it?  It should go to L1 (if enabled), but
still be usable for wake interrupts.  I'm looking at PCIe r6.0, sec
5.2, Table 5-1.

> This means that the port which is generally allowed to go into D3hot
> (because why not), may not be allowed to do so if system wakeup
> devices are present under it and that appears to be the missing
> piece to me.

Re: [PATCH v16 0/3] Avoid PCIe D3 for AMD PCIe root ports

[Rafael J. Wysocki]

On Wed, Sep 6, 2023 at 9:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Sep 06, 2023 at 08:56:29PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Sep 6, 2023 at 2:24 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > > On 9/5/23 14:45, Mario Limonciello wrote:
> > > > On 9/5/2023 05:13, Hans de Goede wrote:
> > > >> On 8/29/23 19:12, Mario Limonciello wrote:
> > > >>> D3 on PCIe root ports isn't used on Windows systems in Modern Standby.
> > > >>> This series adjusts the amd-pmc driver to choose the same strategy
> > > >>> for Rembrandt and Phoenix platforms in Linux with s2idle.
> > > >>>
> > > >>> LPS0 constraints are the basis for it; which if they are added for
> > > >>> Windows would also apply for Linux as well.
> > > >>>
> > > >>> This version doesn't incorporate a callback, as it's pending feedback
> > > >>> from Bjorn if that approach is amenable.
> > > >>>
> > > >>> NOTE:
> > > >>> This series relies upon changes that are both in linux-pm.git and
> > > >>> platform-x86.git. So it won't be able to apply to either maintainer's
> > > >>> tree until later.
> > > >>>
> > > >>> Mario Limonciello (3):
> > > >>>    ACPI: x86: s2idle: Export symbol for fetching constraints for module
> > > >>>      use
> > > >>>    platform/x86/amd: pmc: Adjust workarounds to be part of a switch/case
> > > >>>    platform/x86/amd: pmc: Don't let PCIe root ports go into D3
> > > >>
> > > >> Thank you for the new version.
> > > >>
> > > >> I understand you wanted to get this new approach "out there" but
> > > >> this does not address my remarks on v15:
> > > >>
> > > >> https://lore.kernel.org/platform-driver-x86/53d26a63-64f3-e736-99f5-32bf4b5ba31d@redhat.com/
> > > >>
> > > >
> > > > Right; I called out in the cover letter this is pending feedback from Bjorn.
> > > >
> > > >> Bjorn, I suggest to allow platform code to register a callback
> > > >> to influence pci_bridge_d3_possible() results there. Can you
> > > >> take a look at this and let us know what you think of this
> > > >> suggestion ?
> > > >>
> > > >> Looking at this problem again and rereading the commit message
> > > >> of "platform/x86/amd: pmc: Don't let PCIe root ports go into D3"
> > > >>
> > > >> I see that the problem is that the PCIe root ports to which
> > > >> the USB controllers connect should not be allowed to go
> > > >> into D3 when an USB child of them is configured to wakeup
> > > >> the system.
> > > >>
> > > >> It seems to me that given that problem description,
> > > >> we should not be directly messing with the bridge_d3
> > > >> setting at all.
> > > >>
> > > >> Instead the XHCI code should have an AMD specific quirk
> > > >> where it either unconditionally calls pci_d3cold_disable()
> > > >> on the XHCI PCIe device; or it could even try to be smart
> > > >> and call pci_d3cold_enable() / pci_d3cold_disable()
> > > >> from its (runtime)suspend handler depending on if any
> > > >> USB child is configured as a system wakeup source.
> > > >>
> > > >> Note that it is safe to repeatedly call pci_d3cold_enable()
> > > >> / _disable() there is no need to balance the calls.
> > > >>
> > > >
> > > > It's only the PCIe root port that is used for XHCI tunneling that has this issue.  This specific problem is NOT for the root port of "any" AMD XHCI controllers.  There is no problem with any of the XHCI controllers
> > > > going into D3hot.
> > >
> > > "XHCI tunneling" is an unfamiliar term for me. Are we talking about a XHCI controller inside a USB4/thunderbold dock here which is connected to the laptop over PCIe tunneling over thunderbolt ?
> > >
> > > Or do you mean the XHCI controller inside the laptop which is connected to a USB4/thunderbolt capable Type-C port which is used when that port is in USB3/USB2 mode ?
> > >
> > > As long as the XHCI controller is inside the laptop (and not in the dock), presumably you can identify it by say a set of PCI device-ids of the "tunneling" XHCI controllers on affected AMD platforms. So you could then still call pci_d3cold_disable() from the XHCI driver on only those controllers.
> > >
> > > Note I'm not saying this is the best solution. I'm just trying to understand what you mean with " the PCIe root port that is used for XHCI tunneling" .
> > >
> > > I also see that Rafael has said elsewhere in the thread that he needs to think a bit about how to best handle this ...
> >
> > Yes, I have, and that's because of the realization that the
> > requirements may differ depending on whether or not there is a device
> > (USB or other) enabled to wake up the system from sleep under the Root
> > Port in question.
> >
> > Essentially, the problem is that wakeup doesn't work and the
> > investigation led to the Root Port's power state when suspended, but
> > that power state only appears to be too deep for the wakeup to work
> > and not in general.
> >
> > IIUC, the port can be safely programmed into D3hot and then back to D0
> > and that works as long as there are no wakeup devices under it (Mario,
> > please correct me if that's not the case).
> >
> > Now, when a USB device on the bus segment under the port is configured
> > for system wakeup, it needs to be able to trigger a wake interrupt
> > when the system is in the sleep state.  That wake interrupt is not
> > generated by the USB wakeup device itself, but by the USB controller
> > handling it.  The USB controller is a PCIe device, so in order to
> > generate a wake interrupt it needs the link to its parent port to be
> > up unless it is capable of generating PMEs from D3cold (which only is
> > the case when it is connected to a separate wake power source and that
> > is indicated by setting the corresponding bit in its PM Capabilities
> > Register).
>
> > If that is not the case, and its parent port is programmed
> > into D3hot, that may cause the link between them to go down and so the
> > wake interrupt cannot be generated.
>
> If both the USB adapter and the parent port are in D3hot, the link
> should stay up, shouldn't it?  It should go to L1 (if enabled), but
> still be usable for wake interrupts.  I'm looking at PCIe r6.0, sec
> 5.2, Table 5-1.

As per the spec, you're right, so this would be a departure from it.

> > This means that the port which is generally allowed to go into D3hot
> > (because why not), may not be allowed to do so if system wakeup
> > devices are present under it and that appears to be the missing
> > piece to me.

Re: [PATCH v16 0/3] Avoid PCIe D3 for AMD PCIe root ports

[Rafael J. Wysocki]

On Wed, Sep 6, 2023 at 9:11 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 9/6/2023 13:56, Rafael J. Wysocki wrote:
> > On Wed, Sep 6, 2023 at 2:24 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi Mario,
> >>
> >> On 9/5/23 14:45, Mario Limonciello wrote:
> >>> On 9/5/2023 05:13, Hans de Goede wrote:
> >>>> Hi Mario,
> >>>>
> >>>> On 8/29/23 19:12, Mario Limonciello wrote:
> >>>>> D3 on PCIe root ports isn't used on Windows systems in Modern Standby.
> >>>>> This series adjusts the amd-pmc driver to choose the same strategy
> >>>>> for Rembrandt and Phoenix platforms in Linux with s2idle.
> >>>>>
> >>>>> LPS0 constraints are the basis for it; which if they are added for
> >>>>> Windows would also apply for Linux as well.
> >>>>>
> >>>>> This version doesn't incorporate a callback, as it's pending feedback
> >>>>> from Bjorn if that approach is amenable.
> >>>>>
> >>>>> NOTE:
> >>>>> This series relies upon changes that are both in linux-pm.git and
> >>>>> platform-x86.git. So it won't be able to apply to either maintainer's
> >>>>> tree until later.
> >>>>>
> >>>>> Mario Limonciello (3):
> >>>>>     ACPI: x86: s2idle: Export symbol for fetching constraints for module
> >>>>>       use
> >>>>>     platform/x86/amd: pmc: Adjust workarounds to be part of a switch/case
> >>>>>     platform/x86/amd: pmc: Don't let PCIe root ports go into D3
> >>>>
> >>>> Thank you for the new version.
> >>>>
> >>>> I understand you wanted to get this new approach "out there" but
> >>>> this does not address my remarks on v15:
> >>>>
> >>>> https://lore.kernel.org/platform-driver-x86/53d26a63-64f3-e736-99f5-32bf4b5ba31d@redhat.com/
> >>>>
> >>>
> >>> Right; I called out in the cover letter this is pending feedback from Bjorn.
> >>>
> >>>> Bjorn, I suggest to allow platform code to register a callback
> >>>> to influence pci_bridge_d3_possible() results there. Can you
> >>>> take a look at this and let us know what you think of this
> >>>> suggestion ?
> >>>>
> >>>> Looking at this problem again and rereading the commit message
> >>>> of "platform/x86/amd: pmc: Don't let PCIe root ports go into D3"
> >>>>
> >>>> I see that the problem is that the PCIe root ports to which
> >>>> the USB controllers connect should not be allowed to go
> >>>> into D3 when an USB child of them is configured to wakeup
> >>>> the system.
> >>>>
> >>>> It seems to me that given that problem description,
> >>>> we should not be directly messing with the bridge_d3
> >>>> setting at all.
> >>>>
> >>>> Instead the XHCI code should have an AMD specific quirk
> >>>> where it either unconditionally calls pci_d3cold_disable()
> >>>> on the XHCI PCIe device; or it could even try to be smart
> >>>> and call pci_d3cold_enable() / pci_d3cold_disable()
> >>>> from its (runtime)suspend handler depending on if any
> >>>> USB child is configured as a system wakeup source.
> >>>>
> >>>> Note that it is safe to repeatedly call pci_d3cold_enable()
> >>>> / _disable() there is no need to balance the calls.
> >>>>
> >>>
> >>> It's only the PCIe root port that is used for XHCI tunneling that has this issue.  This specific problem is NOT for the root port of "any" AMD XHCI controllers.  There is no problem with any of the XHCI controllers
> >>> going into D3hot.
> >>
> >> "XHCI tunneling" is an unfamiliar term for me. Are we talking about a XHCI controller inside a USB4/thunderbold dock here which is connected to the laptop over PCIe tunneling over thunderbolt ?
> >>
> >> Or do you mean the XHCI controller inside the laptop which is connected to a USB4/thunderbolt capable Type-C port which is used when that port is in USB3/USB2 mode ?
> >>
> >> As long as the XHCI controller is inside the laptop (and not in the dock), presumably you can identify it by say a set of PCI device-ids of the "tunneling" XHCI controllers on affected AMD platforms. So you could then still call pci_d3cold_disable() from the XHCI driver on only those controllers.
> >>
> >> Note I'm not saying this is the best solution. I'm just trying to understand what you mean with " the PCIe root port that is used for XHCI tunneling" .
> >>
> >> I also see that Rafael has said elsewhere in the thread that he needs to think a bit about how to best handle this ...
> >
> > Yes, I have, and that's because of the realization that the
> > requirements may differ depending on whether or not there is a device
> > (USB or other) enabled to wake up the system from sleep under the Root
> > Port in question.
> >
> > Essentially, the problem is that wakeup doesn't work and the
> > investigation led to the Root Port's power state when suspended, but
> > that power state only appears to be too deep for the wakeup to work
> > and not in general.
> >
> > IIUC, the port can be safely programmed into D3hot and then back to D0
> > and that works as long as there are no wakeup devices under it (Mario,
> > please correct me if that's not the case).
>
> This is correct.
>
> >
> > Now, when a USB device on the bus segment under the port is configured
> > for system wakeup, it needs to be able to trigger a wake interrupt
> > when the system is in the sleep state.  That wake interrupt is not
> > generated by the USB wakeup device itself, but by the USB controller
> > handling it.  The USB controller is a PCIe device, so in order to
> > generate a wake interrupt it needs the link to its parent port to be
> > up unless it is capable of generating PMEs from D3cold (which only is
> > the case when it is connected to a separate wake power source and that
> > is indicated by setting the corresponding bit in its PM Capabilities
> > Register).  If that is not the case, and its parent port is programmed
> > into D3hot, that may cause the link between them to go down and so the
> > wake interrupt cannot be generated.  This means that the port which is
> > generally allowed to go into D3hot (because why not), may not be
> > allowed to do so if system wakeup devices are present under it and
> > that appears to be the missing piece to me.
>
> At a glance this description makes sense, but the logic in
> pci_target_state() already accounts for finding the matching states that
> PME can be generated from.
>
>  From one of these systems this is the problematic root port:
>
> pci 0000:00:08.3: PME# supported from D0 D3hot D3cold
>
> It's accurate while the system isn't in the deepest state too.
> If I rmmod amd-pmc or unbind it then the platform will never be notified
> that the OS said it can go to the deepest state at suspend.
>
> With no changes to the kernel pressing a key on the keyboard works and
> you can see the IRQ that woke the system matches the PCIe PME.
>
> I think what we're at here is likely an unspecified platform behavior
> that is "masked" in Windows due to the way that policy works in Windows
> (constraints will influence what states are selected for integrated
> devices by the uPEP driver).

This appears to be correct.

> I'm going to drop my updated series to the mailing list that adds the
> ability for a driver to register a vote like Hans suggested and we can
> see what everyone thinks.

Works for me.