[PATCH v19 0/2] Add quirk for PCIe root port on AMD systems

[PATCH v19 0/2] Add quirk for PCIe root port on AMD systems

[Mario Limonciello]

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 D3hot and AMD's platform can't handle USB devices waking the
platform from a hardware sleep state in this case.

The firmware doesn't express this limitation in a way that Linux
recognizes so this series introduces a quirk for this problem.

Previous submissions:
* v18
  https://lore.kernel.org/linux-usb/20230913040832.114610-1-mario.limonciello@amd.com/T/#m92a2654129d5f6be5439d8a94fb36734804763c1
* v17
  https://lore.kernel.org/platform-driver-x86/20230906184354.45846-1-mario.limonciello@amd.com/
  This version implemented constraints for the amd-pmc driver and introduced
  a veto/optin system for the PCI core as suggested by Hans.
  Rafael suggested not to use the veto/optin system and instead use a quirk.
* v16
  https://lore.kernel.org/platform-driver-x86/20230829171212.156688-1-mario.limonciello@amd.com/
  This version implemented constraints for the amd-pmc driver.
* v15
  https://lore.kernel.org/platform-driver-x86/20230828042819.47013-1-mario.limonciello@amd.com/#t
  This version hardcoded the quirk into amd-pmc driver as part of suspend
  callback.
* v14
  https://lore.kernel.org/linux-pci/20230818193932.27187-1-mario.limonciello@amd.com/
  https://lore.kernel.org/linux-pci/20230818194007.27410-1-mario.limonciello@amd.com/
  https://lore.kernel.org/linux-pci/20230818194027.27559-1-mario.limonciello@amd.com/
  This version implemented constraints exporting and limited the policy for
  >= 2015 to Intel only. It also added support for constraints optin.
  V13 was split into multiple parts to make it easier to land. 14.b was
  merged.
* v13
  https://lore.kernel.org/linux-pci/20230818051319.551-1-mario.limonciello@amd.com/
* v12
  https://lore.kernel.org/linux-pci/20230816204143.66281-1-mario.limonciello@amd.com/
* v11
  https://lore.kernel.org/linux-pci/20230809185453.40916-1-mario.limonciello@amd.com/
* v10
  https://lore.kernel.org/linux-pci/20230804210129.5356-1-mario.limonciello@amd.com/
* v9
  https://lore.kernel.org/linux-pci/20230804010229.3664-1-mario.limonciello@amd.com/
* v8
  https://lore.kernel.org/linux-pci/20230802201013.910-1-mario.limonciello@amd.com/
* v7
  https://lore.kernel.org/linux-pci/20230711005325.1499-1-mario.limonciello@amd.com/
* v6
  https://lore.kernel.org/linux-pci/20230708214457.1229-1-mario.limonciello@amd.com/
* v5
  https://lore.kernel.org/linux-pci/20230530163947.230418-1-mario.limonciello@amd.com/
* v4
  https://lore.kernel.org/linux-pci/20230524190726.17012-1-mario.limonciello@amd.com/
* v3
  https://lore.kernel.org/linux-pci/20230524152136.1033-1-mario.limonciello@amd.com/
* v2
  https://lore.kernel.org/linux-pci/20230517150827.89819-1-mario.limonciello@amd.com/
* v1
  https://lore.kernel.org/linux-pci/20230515231515.1440-1-mario.limonciello@amd.com/

Mario Limonciello (2):
  PCI: Move the `PCI_CLASS_SERIAL_USB_USB4` definition to common header
  PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers

 drivers/pci/quirks.c      | 61 +++++++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/nhi.h |  2 --
 include/linux/pci_ids.h   |  1 +
 3 files changed, 62 insertions(+), 2 deletions(-)


base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
-- 
2.34.1

[PATCH v19 1/2] PCI: Move the `PCI_CLASS_SERIAL_USB_USB4` definition to common header

[Mario Limonciello]

`PCI_CLASS_SERIAL_USB_USB4` may be used by code outside of thunderbolt.
Move the declaration into the common pci_ids.h header.

Cc: stable@vger.kernel.org
Acked-by: Mika Westerberberg <mika.westerberg@linux.intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/thunderbolt/nhi.h | 2 --
 include/linux/pci_ids.h   | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h
index 0f029ce75882..675ddefe283c 100644
--- a/drivers/thunderbolt/nhi.h
+++ b/drivers/thunderbolt/nhi.h
@@ -91,6 +91,4 @@ extern const struct tb_nhi_ops icl_nhi_ops;
 #define PCI_DEVICE_ID_INTEL_RPL_NHI0			0xa73e
 #define PCI_DEVICE_ID_INTEL_RPL_NHI1			0xa76d
 
-#define PCI_CLASS_SERIAL_USB_USB4			0x0c0340
-
 #endif
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 5fb3d4c393a9..29aeac53dc41 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -121,6 +121,7 @@
 #define PCI_CLASS_SERIAL_USB_OHCI	0x0c0310
 #define PCI_CLASS_SERIAL_USB_EHCI	0x0c0320
 #define PCI_CLASS_SERIAL_USB_XHCI	0x0c0330
+#define PCI_CLASS_SERIAL_USB_USB4	0x0c0340
 #define PCI_CLASS_SERIAL_USB_DEVICE	0x0c03fe
 #define PCI_CLASS_SERIAL_FIBER		0x0c04
 #define PCI_CLASS_SERIAL_SMBUS		0x0c05
-- 
2.34.1

[PATCH v19 2/2] PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers

[Mario Limonciello]

Iain reports that USB devices can't be used to wake a Lenovo Z13
from suspend. This problem occurs because the PCIe root port has been put
into D3hot and AMD's platform can't handle USB devices waking the platform
from a hardware sleep state in this case. The platform is put into
a hardware sleep state by the actions of the amd-pmc driver.

Although the issue is initially reported on a single model it actually
affects all Yellow Carp (Rembrandt) and Pink Sardine (Phoenix) SoCs.
This problem only occurs on Linux specifically when attempting to
wake the platform from a hardware sleep state.
Comparing the behavior on Windows and Linux, Windows doesn't put
the root ports into D3 at this time.

Linux decides the target state to put the device into at suspend by
this policy:
1. If platform_pci_power_manageable():
   Use platform_pci_choose_state()
2. If the device is armed for wakeup:
   Select the deepest D-state that supports a PME.
3. Else:
   Use D3hot.

Devices are considered power manageable by the platform when they have
one or more objects described in the table in section 7.3 of the ACPI 6.5
specification [1]. In this case the root ports are not power manageable.

If devices are not considered power manageable; specs are ambiguous as
to what should happen.  In this situation Windows 11 puts PCIe ports
in D0 ostensibly due the policy from the "uPEP driver" which is a
complimentary driver the Linux "amd-pmc" driver.

Linux chooses to allow D3 for these root ports due to the policy
introduced by commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during
suspend").

The Windows uPEP driver expresses the desired state that should be
selected for suspend but Linux doesn't, so introduce a quirk for the
problematic root ports.

The quirk removes PME support for D3hot and D3cold at suspend time if the
system will be using s2idle. When the port is configured for wakeup this
will prevent these states from being selected in pci_target_state().

After the system is resumes the PME support is re-read from the PM
capabilities register to allow opportunistic power savings at runtime by
letting the root port go into D3hot or D3cold.

Cc: stable@vger.kernel.org
Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/platform-design-for-modern-standby#low-power-core-silicon-cpu-soc-dram [1]
Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
Reported-by: Iain Lane <iain@orangesquash.org.uk>
Closes: https://forums.lenovo.com/t5/Ubuntu/Z13-can-t-resume-from-suspend-with-external-USB-keyboard/m-p/5217121
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/quirks.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index eeec1d6f9023..ebc0afbc814e 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6188,3 +6188,64 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a31, dpc_log_size);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5020, of_pci_make_dev_node);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5021, of_pci_make_dev_node);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REDHAT, 0x0005, of_pci_make_dev_node);
+
+/*
+ * When AMD PCIe root ports with AMD USB4 controllers attached to them are put
+ * into D3hot or D3cold downstream USB devices may fail to wakeup the system.
+ * This manifests as a missing wakeup interrupt.
+ *
+ * Prevent the associated root port from using PME to wake from D3hot or
+ * D3cold power states during s2idle.
+ * This will effectively put the root port into D0 power state over s2idle.
+ */
+static bool child_has_amd_usb4(struct pci_dev *pdev)
+{
+	struct pci_dev *child = NULL;
+
+	while ((child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child))) {
+		if (child->vendor != PCI_VENDOR_ID_AMD)
+			continue;
+		if (pcie_find_root_port(child) != pdev)
+			continue;
+		return true;
+	}
+
+	return false;
+}
+
+static void quirk_reenable_pme(struct pci_dev *dev)
+{
+	u16 pmc;
+
+	if (!dev->pm_cap)
+		return;
+
+	if (!child_has_amd_usb4(dev))
+		return;
+
+	pci_read_config_word(dev, dev->pm_cap + PCI_PM_PMC, &pmc);
+	pmc &= PCI_PM_CAP_PME_MASK;
+	dev->pme_support = pmc >> PCI_PM_CAP_PME_SHIFT;
+}
+
+static void quirk_disable_pme_suspend(struct pci_dev *dev)
+{
+	int mask;
+
+	if (pm_suspend_via_firmware())
+		return;
+
+	if (!child_has_amd_usb4(dev))
+		return;
+
+	mask = (PCI_PM_CAP_PME_D3hot|PCI_PM_CAP_PME_D3cold) >> PCI_PM_CAP_PME_SHIFT;
+	if (!(dev->pme_support & mask))
+		return;
+	dev->pme_support &= ~mask;
+	dev_info_once(&dev->dev, "quirk: disabling PME from D3hot and D3cold at suspend\n");
+}
+
+DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x14b9, quirk_disable_pme_suspend);
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x14b9, quirk_reenable_pme);
+DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x14eb, quirk_disable_pme_suspend);
+DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x14eb, quirk_reenable_pme);
-- 
2.34.1

Re: [PATCH v19 2/2] PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers

[Lukas Wunner]

On Thu, Sep 14, 2023 at 09:33:54PM -0500, Mario Limonciello wrote:
> +static bool child_has_amd_usb4(struct pci_dev *pdev)
> +{
> +	struct pci_dev *child = NULL;
> +
> +	while ((child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child))) {
> +		if (child->vendor != PCI_VENDOR_ID_AMD)
> +			continue;
> +		if (pcie_find_root_port(child) != pdev)
> +			continue;
> +		return true;
> +	}
> +
> +	return false;
> +}

What's the purpose of the pcie_find_root_port() check?  PCI is a hierarchy,
not a graph, so a device cannot have any other Root Port but the one below
which you're searching.

If the purpose is to check that the port is a Root Port (if the PCI IDs
you're using in the DECLARE_PCI_FIXUP_* clauses match non-Root Ports),
check for pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT.  (No need to
check for that in every loop iteration obviously, just check once in
the fixup.)

Thanks,

Lukas

Re: [PATCH v19 2/2] PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers

[Mario Limonciello]

On 9/15/2023 02:08, Lukas Wunner wrote:
> On Thu, Sep 14, 2023 at 09:33:54PM -0500, Mario Limonciello wrote:
>> +static bool child_has_amd_usb4(struct pci_dev *pdev)
>> +{
>> +	struct pci_dev *child = NULL;
>> +
>> +	while ((child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child))) {
>> +		if (child->vendor != PCI_VENDOR_ID_AMD)
>> +			continue;
>> +		if (pcie_find_root_port(child) != pdev)
>> +			continue;
>> +		return true;
>> +	}
>> +
>> +	return false;
>> +}
> 
> What's the purpose of the pcie_find_root_port() check?  PCI is a hierarchy,
> not a graph, so a device cannot have any other Root Port but the one below
> which you're searching.
> 
> If the purpose is to check that the port is a Root Port (if the PCI IDs
> you're using in the DECLARE_PCI_FIXUP_* clauses match non-Root Ports),
> check for pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT.  (No need to
> check for that in every loop iteration obviously, just check once in
> the fixup.)
> 
> Thanks,
> 
> Lukas

The reason to look for it the way that I did was that there are multiple 
root ports with the exact same PCI ID.

The problem only occurs on the root port that happens to have an AMD 
USB4 controller connected.

Re: [PATCH v19 2/2] PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers

[Lukas Wunner]

On Fri, Sep 15, 2023 at 07:04:11AM -0500, Mario Limonciello wrote:
> On 9/15/2023 02:08, Lukas Wunner wrote:
> > On Thu, Sep 14, 2023 at 09:33:54PM -0500, Mario Limonciello wrote:
> > > +static bool child_has_amd_usb4(struct pci_dev *pdev)
> > > +{
> > > +	struct pci_dev *child = NULL;
> > > +
> > > +	while ((child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child))) {
> > > +		if (child->vendor != PCI_VENDOR_ID_AMD)
> > > +			continue;
> > > +		if (pcie_find_root_port(child) != pdev)
> > > +			continue;
> > > +		return true;
> > > +	}
> > > +
> > > +	return false;
> > > +}
> > 
> > What's the purpose of the pcie_find_root_port() check?  PCI is a hierarchy,
> > not a graph, so a device cannot have any other Root Port but the one below
> > which you're searching.
> > 
> > If the purpose is to check that the port is a Root Port (if the PCI IDs
> > you're using in the DECLARE_PCI_FIXUP_* clauses match non-Root Ports),
> > check for pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT.  (No need to
> > check for that in every loop iteration obviously, just check once in
> > the fixup.)
> > 
> > Thanks,
> > 
> > Lukas
> 
> The reason to look for it the way that I did was that there are multiple
> root ports with the exact same PCI ID.
> 
> The problem only occurs on the root port that happens to have an AMD USB4
> controller connected.

Yes but what's the purpose of the pcie_find_root_port(child) check
quoted above?

Thanks,

Lukas

Re: [PATCH v19 2/2] PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers

[Mario Limonciello]

On 9/15/2023 23:48, Lukas Wunner wrote:
> On Fri, Sep 15, 2023 at 07:04:11AM -0500, Mario Limonciello wrote:
>> On 9/15/2023 02:08, Lukas Wunner wrote:
>>> On Thu, Sep 14, 2023 at 09:33:54PM -0500, Mario Limonciello wrote:
>>>> +static bool child_has_amd_usb4(struct pci_dev *pdev)
>>>> +{
>>>> +	struct pci_dev *child = NULL;
>>>> +
>>>> +	while ((child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child))) {
>>>> +		if (child->vendor != PCI_VENDOR_ID_AMD)
>>>> +			continue;
>>>> +		if (pcie_find_root_port(child) != pdev)
>>>> +			continue;
>>>> +		return true;
>>>> +	}
>>>> +
>>>> +	return false;
>>>> +}
>>>
>>> What's the purpose of the pcie_find_root_port() check?  PCI is a hierarchy,
>>> not a graph, so a device cannot have any other Root Port but the one below
>>> which you're searching.
>>>
>>> If the purpose is to check that the port is a Root Port (if the PCI IDs
>>> you're using in the DECLARE_PCI_FIXUP_* clauses match non-Root Ports),
>>> check for pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT.  (No need to
>>> check for that in every loop iteration obviously, just check once in
>>> the fixup.)
>>>
>>> Thanks,
>>>
>>> Lukas
>>
>> The reason to look for it the way that I did was that there are multiple
>> root ports with the exact same PCI ID.
>>
>> The problem only occurs on the root port that happens to have an AMD USB4
>> controller connected.
> 
> Yes but what's the purpose of the pcie_find_root_port(child) check
> quoted above?
> 
> Thanks,
> 
> Lukas

You're right that if you look at this system alone that the check isn't 
strictly necessary.  It's to future proof the quirk.  If a discrete USB4 
controller was connected to the system it would be connected to a 
different root port (the one that is used for PCI tunneling).

AMD doesn't have any of these devices, but if some day one was created 
it could trip this codepath.

If you feel it's better to remove the check unless such a device is 
created sure I can drop it.

Re: [PATCH v19 2/2] PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers

[Lukas Wunner]

On Sat, Sep 16, 2023 at 08:09:19AM -0500, Mario Limonciello wrote:
> On 9/15/2023 23:48, Lukas Wunner wrote:
> > On Fri, Sep 15, 2023 at 07:04:11AM -0500, Mario Limonciello wrote:
> > > On 9/15/2023 02:08, Lukas Wunner wrote:
> > > > On Thu, Sep 14, 2023 at 09:33:54PM -0500, Mario Limonciello wrote:
> > > > > +static bool child_has_amd_usb4(struct pci_dev *pdev)
> > > > > +{
> > > > > +	struct pci_dev *child = NULL;
> > > > > +
> > > > > +	while ((child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child))) {
> > > > > +		if (child->vendor != PCI_VENDOR_ID_AMD)
> > > > > +			continue;
> > > > > +		if (pcie_find_root_port(child) != pdev)
> > > > > +			continue;
> > > > > +		return true;
> > > > > +	}
> > > > > +
> > > > > +	return false;
> > > > > +}
> > > > 
> > > > What's the purpose of the pcie_find_root_port() check?  PCI is a hierarchy,
> > > > not a graph, so a device cannot have any other Root Port but the one below
> > > > which you're searching.
> > > > 
> > > > If the purpose is to check that the port is a Root Port (if the PCI IDs
> > > > you're using in the DECLARE_PCI_FIXUP_* clauses match non-Root Ports),
> > > > check for pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT.  (No need to
> > > > check for that in every loop iteration obviously, just check once in
> > > > the fixup.)
> > > > 
> > > > Thanks,
> > > > 
> > > > Lukas
> > > 
> > > The reason to look for it the way that I did was that there are multiple
> > > root ports with the exact same PCI ID.
> > > 
> > > The problem only occurs on the root port that happens to have an AMD USB4
> > > controller connected.
> > 
> > Yes but what's the purpose of the pcie_find_root_port(child) check
> > quoted above?
> 
> You're right that if you look at this system alone that the check isn't
> strictly necessary.  It's to future proof the quirk.  If a discrete USB4
> controller was connected to the system it would be connected to a different
> root port (the one that is used for PCI tunneling).
> 
> AMD doesn't have any of these devices, but if some day one was created it
> could trip this codepath.
> 
> If you feel it's better to remove the check unless such a device is created
> sure I can drop it.

PCIe ports used for Thunderbolt tunneling are Downstream Ports or
Upstream Ports (depending on which of the two ends of the tunnel
you're looking at).

The "pcie_find_root_port(child) != pdev" check is always false:

You're searching for a USB controller below a Root Port and
check whether the Root Port in the USB controller's ancestry
is the Root Port below which you're searching.  That's a
tautology.

I'm guessing what you really mean is:

		if (pci_upstream_bridge(child)) != pdev)
			continue;

Thanks,

Lukas

Re: [PATCH v19 2/2] PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers

[Mario Limonciello]

On 9/16/2023 08:36, Lukas Wunner wrote:
> On Sat, Sep 16, 2023 at 08:09:19AM -0500, Mario Limonciello wrote:
>> On 9/15/2023 23:48, Lukas Wunner wrote:
>>> On Fri, Sep 15, 2023 at 07:04:11AM -0500, Mario Limonciello wrote:
>>>> On 9/15/2023 02:08, Lukas Wunner wrote:
>>>>> On Thu, Sep 14, 2023 at 09:33:54PM -0500, Mario Limonciello wrote:
>>>>>> +static bool child_has_amd_usb4(struct pci_dev *pdev)
>>>>>> +{
>>>>>> +	struct pci_dev *child = NULL;
>>>>>> +
>>>>>> +	while ((child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child))) {
>>>>>> +		if (child->vendor != PCI_VENDOR_ID_AMD)
>>>>>> +			continue;
>>>>>> +		if (pcie_find_root_port(child) != pdev)
>>>>>> +			continue;
>>>>>> +		return true;
>>>>>> +	}
>>>>>> +
>>>>>> +	return false;
>>>>>> +}
>>>>>
>>>>> What's the purpose of the pcie_find_root_port() check?  PCI is a hierarchy,
>>>>> not a graph, so a device cannot have any other Root Port but the one below
>>>>> which you're searching.
>>>>>
>>>>> If the purpose is to check that the port is a Root Port (if the PCI IDs
>>>>> you're using in the DECLARE_PCI_FIXUP_* clauses match non-Root Ports),
>>>>> check for pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT.  (No need to
>>>>> check for that in every loop iteration obviously, just check once in
>>>>> the fixup.)
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Lukas
>>>>
>>>> The reason to look for it the way that I did was that there are multiple
>>>> root ports with the exact same PCI ID.
>>>>
>>>> The problem only occurs on the root port that happens to have an AMD USB4
>>>> controller connected.
>>>
>>> Yes but what's the purpose of the pcie_find_root_port(child) check
>>> quoted above?
>>
>> You're right that if you look at this system alone that the check isn't
>> strictly necessary.  It's to future proof the quirk.  If a discrete USB4
>> controller was connected to the system it would be connected to a different
>> root port (the one that is used for PCI tunneling).
>>
>> AMD doesn't have any of these devices, but if some day one was created it
>> could trip this codepath.
>>
>> If you feel it's better to remove the check unless such a device is created
>> sure I can drop it.
> 
> PCIe ports used for Thunderbolt tunneling are Downstream Ports or
> Upstream Ports (depending on which of the two ends of the tunnel
> you're looking at).

Let me explain the topology from an actual system with PCIe tunneling 
active and a dock connected downstream.

-[0000:00]-+-00.0
            +-04.1-[33-62]----00.0-[34-36]--+-02.0-[35]----00.0
            |                               \-04.0-[36]--
            +-08.3-[64]--+-00.0
            |            +-00.3
            |            +-00.4
            |            +-00.5
            |            \-00.6


This is the root port that tunneled devices will connect to:
$ sudo lspci -s 0000:00:04.1 -v | grep "Capabilities: \[58\]"
         Capabilities: [58] Express Root Port (Slot+), MSI 00

This is the root port that the USB4 controllers are connected to that is 
problematic:
$ sudo lspci -s 0000:00:08.3 -v | grep "Capabilities: \[58\]"
         Capabilities: [58] Express Root Port (Slot-), MSI 00

Here's a downstream connected dock from tunneling:
33:00.0 PCI bridge [0604]: Intel Corporation JHL7540 Thunderbolt 3 
Bridge [Titan Ridge DD 2018] [8086:15ef] (rev 06)
34:02.0 PCI bridge [0604]: Intel Corporation JHL7540 Thunderbolt 3 
Bridge [Titan Ridge DD 2018] [8086:15ef] (rev 06)
34:04.0 PCI bridge [0604]: Intel Corporation JHL7540 Thunderbolt 3 
Bridge [Titan Ridge DD 2018] [8086:15ef] (rev 06)

Here's the USB4 controllers connected to 08.3:
64:00.0 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] Device 
[1022:161f]
64:00.3 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] Device 
[1022:15d6]
64:00.4 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] Device 
[1022:15d7]
64:00.5 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] Device 
[1022:162e]
64:00.6 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] Device 
[1022:162f]

If an AMD USB4 controller happened to be connected, it would have shown 
up connected to 04.1 as it's root port.

> 
> The "pcie_find_root_port(child) != pdev" check is always false:
> 
> You're searching for a USB controller below a Root Port and
> check whether the Root Port in the USB controller's ancestry
> is the Root Port below which you're searching.  That's a
> tautology.

The search doesn't start at pdev, it starts at NULL.

struct pci_dev *child = NULL;

> 
> I'm guessing what you really mean is:
> 
> 		if (pci_upstream_bridge(child)) != pdev)
> 			continue;
> 
That's exactly what I had before and Rafael suggested [1]
to change it to pcie_find_root_port().

[1] 
https://lore.kernel.org/linux-usb/20230913040832.114610-1-mario.limonciello@amd.com/T/#m66acb79a13d314b5e868993b1611266a968cf064
> Thanks,
> 
> Lukas

Re: [PATCH v19 2/2] PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers

[Bjorn Helgaas]

On Sat, Sep 16, 2023 at 03:36:50PM +0200, Lukas Wunner wrote:
> On Sat, Sep 16, 2023 at 08:09:19AM -0500, Mario Limonciello wrote:
> > On 9/15/2023 23:48, Lukas Wunner wrote:
> > > On Fri, Sep 15, 2023 at 07:04:11AM -0500, Mario Limonciello wrote:
> > > > On 9/15/2023 02:08, Lukas Wunner wrote:
> > > > > On Thu, Sep 14, 2023 at 09:33:54PM -0500, Mario Limonciello wrote:
> > > > > > +static bool child_has_amd_usb4(struct pci_dev *pdev)
> > > > > > +{
> > > > > > +	struct pci_dev *child = NULL;
> > > > > > +
> > > > > > +	while ((child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child))) {
> > > > > > +		if (child->vendor != PCI_VENDOR_ID_AMD)
> > > > > > +			continue;
> > > > > > +		if (pcie_find_root_port(child) != pdev)
> > > > > > +			continue;
> > > > > > +		return true;
> > > > > > +	}
> > > > > > +
> > > > > > +	return false;
> > > > > > +}
> ...

> The "pcie_find_root_port(child) != pdev" check is always false:

If we were using pci_walk_bus() and only looking at devices below
pdev, I would agree, but since we're using pci_get_class(), which
searches all PCI devices in the system, I'm confused about why it
would always be false.

I don't really see the point of checking for USB4, because the commit
log doesn't say anything about why this would be specific to USB4.

I know Mario has mentioned something about how "internal interrupt
routing works with the USB4 controller connected to this root port,"
but I don't understand what that means.

Is the USB4 controller integrated into the Root Port?  Or is this
interrupt routed via some non-PCIe mechanism?  If the USB4 controller
is connected via standard PCIe, I don't see why the issue sould be
specific to USB4.

I could believe that BIOS configures the Root Port differently based
on whether the downstream device is USB4, but I haven't heard anything
about that.

Bjorn

Re: [PATCH v19 2/2] PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers

[Bjorn Helgaas]

On Thu, Sep 14, 2023 at 09:33:54PM -0500, Mario Limonciello wrote:
> Iain reports that USB devices can't be used to wake a Lenovo Z13
> from suspend. This problem occurs because the PCIe root port has been put
> into D3hot and AMD's platform can't handle USB devices waking the platform
> from a hardware sleep state in this case. The platform is put into
> a hardware sleep state by the actions of the amd-pmc driver.
> 
> Although the issue is initially reported on a single model it actually
> affects all Yellow Carp (Rembrandt) and Pink Sardine (Phoenix) SoCs.
> This problem only occurs on Linux specifically when attempting to
> wake the platform from a hardware sleep state.
> Comparing the behavior on Windows and Linux, Windows doesn't put
> the root ports into D3 at this time.
> 
> Linux decides the target state to put the device into at suspend by
> this policy:
> 1. If platform_pci_power_manageable():
>    Use platform_pci_choose_state()
> 2. If the device is armed for wakeup:
>    Select the deepest D-state that supports a PME.
> 3. Else:
>    Use D3hot.
> 
> Devices are considered power manageable by the platform when they have
> one or more objects described in the table in section 7.3 of the ACPI 6.5
> specification [1]. In this case the root ports are not power manageable.
> 
> If devices are not considered power manageable; specs are ambiguous as
> to what should happen.  In this situation Windows 11 puts PCIe ports
> in D0 ostensibly due the policy from the "uPEP driver" which is a
> complimentary driver the Linux "amd-pmc" driver.
> 
> Linux chooses to allow D3 for these root ports due to the policy
> introduced by commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during
> suspend").
> 
> The Windows uPEP driver expresses the desired state that should be
> selected for suspend but Linux doesn't, so introduce a quirk for the
> problematic root ports.
> 
> The quirk removes PME support for D3hot and D3cold at suspend time if the
> system will be using s2idle. When the port is configured for wakeup this
> will prevent these states from being selected in pci_target_state().
> 
> After the system is resumes the PME support is re-read from the PM
> capabilities register to allow opportunistic power savings at runtime by
> letting the root port go into D3hot or D3cold.

There's a lot of text here, but I think the essential thing is:

  These Root Ports advertise D3hot and D3cold in the PME_Support
  register, but PMEs do not work in those states when the amd-pmc
  driver has put the platform in a sleep state.

> Cc: stable@vger.kernel.org
> Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/platform-design-for-modern-standby#low-power-core-silicon-cpu-soc-dram [1]
> Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> Reported-by: Iain Lane <iain@orangesquash.org.uk>
> Closes: https://forums.lenovo.com/t5/Ubuntu/Z13-can-t-resume-from-suspend-with-external-USB-keyboard/m-p/5217121
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/pci/quirks.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index eeec1d6f9023..ebc0afbc814e 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -6188,3 +6188,64 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a31, dpc_log_size);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5020, of_pci_make_dev_node);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5021, of_pci_make_dev_node);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REDHAT, 0x0005, of_pci_make_dev_node);
> +
> +/*
> + * When AMD PCIe root ports with AMD USB4 controllers attached to them are put
> + * into D3hot or D3cold downstream USB devices may fail to wakeup the system.
> + * This manifests as a missing wakeup interrupt.
> + *
> + * Prevent the associated root port from using PME to wake from D3hot or
> + * D3cold power states during s2idle.
> + * This will effectively put the root port into D0 power state over s2idle.
> + */
> +static bool child_has_amd_usb4(struct pci_dev *pdev)
> +{
> +	struct pci_dev *child = NULL;
> +
> +	while ((child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child))) {
> +		if (child->vendor != PCI_VENDOR_ID_AMD)
> +			continue;
> +		if (pcie_find_root_port(child) != pdev)
> +			continue;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static void quirk_reenable_pme(struct pci_dev *dev)
> +{
> +	u16 pmc;
> +
> +	if (!dev->pm_cap)
> +		return;
> +
> +	if (!child_has_amd_usb4(dev))
> +		return;
> +
> +	pci_read_config_word(dev, dev->pm_cap + PCI_PM_PMC, &pmc);
> +	pmc &= PCI_PM_CAP_PME_MASK;
> +	dev->pme_support = pmc >> PCI_PM_CAP_PME_SHIFT;
> +}
> +
> +static void quirk_disable_pme_suspend(struct pci_dev *dev)
> +{
> +	int mask;
> +
> +	if (pm_suspend_via_firmware())
> +		return;

There's always something more to confuse me.  Why does
pm_suspend_via_firmware() matter?  I can sort of see that Linux
platform power management, which uses the PMC, is not the same
as platform firmware being invoked at the end of a system-wide power
management transition to a sleep state.

I guess this must have something to do with acpi_suspend_begin() and
acpi_hibernation_begin() (the callers of
pm_set_suspend_via_firmware())?

> +	if (!child_has_amd_usb4(dev))
> +		return;
> +
> +	mask = (PCI_PM_CAP_PME_D3hot|PCI_PM_CAP_PME_D3cold) >> PCI_PM_CAP_PME_SHIFT;
> +	if (!(dev->pme_support & mask))
> +		return;
> +	dev->pme_support &= ~mask;
> +	dev_info_once(&dev->dev, "quirk: disabling PME from D3hot and D3cold at suspend\n");
> +}
> +
> +DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x14b9, quirk_disable_pme_suspend);
> +DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x14b9, quirk_reenable_pme);
> +DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x14eb, quirk_disable_pme_suspend);
> +DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x14eb, quirk_reenable_pme);
> -- 
> 2.34.1
> 

Re: [PATCH v19 2/2] PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers

[Mario Limonciello]

On 9/17/2023 16:40, Bjorn Helgaas wrote:
>> The "pcie_find_root_port(child) != pdev" check is always false:
> 
> If we were using pci_walk_bus() and only looking at devices below
> pdev, I would agree, but since we're using pci_get_class(), which
> searches all PCI devices in the system, I'm confused about why it
> would always be false.
> 
> I don't really see the point of checking for USB4, because the commit
> log doesn't say anything about why this would be specific to USB4.
> 
> I know Mario has mentioned something about how "internal interrupt
> routing works with the USB4 controller connected to this root port,"
> but I don't understand what that means.
> 
> Is the USB4 controller integrated into the Root Port?  Or is this
> interrupt routed via some non-PCIe mechanism?  If the USB4 controller
> is connected via standard PCIe, I don't see why the issue sould be
> specific to USB4.

It's the latter.  When the PMC has the SoC in hardware sleep the 
interrupt routing works differently.

That's where this bug stems from.

> 
> I could believe that BIOS configures the Root Port differently based
> on whether the downstream device is USB4, but I haven't heard anything
> about that.
> 
> Bjorn
Nothing along these lines.

Re: [PATCH v19 2/2] PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers

[Mario Limonciello]

On 9/17/2023 16:56, Bjorn Helgaas wrote:
> On Thu, Sep 14, 2023 at 09:33:54PM -0500, Mario Limonciello wrote:
>> Iain reports that USB devices can't be used to wake a Lenovo Z13
>> from suspend. This problem occurs because the PCIe root port has been put
>> into D3hot and AMD's platform can't handle USB devices waking the platform
>> from a hardware sleep state in this case. The platform is put into
>> a hardware sleep state by the actions of the amd-pmc driver.
>>
>> Although the issue is initially reported on a single model it actually
>> affects all Yellow Carp (Rembrandt) and Pink Sardine (Phoenix) SoCs.
>> This problem only occurs on Linux specifically when attempting to
>> wake the platform from a hardware sleep state.
>> Comparing the behavior on Windows and Linux, Windows doesn't put
>> the root ports into D3 at this time.
>>
>> Linux decides the target state to put the device into at suspend by
>> this policy:
>> 1. If platform_pci_power_manageable():
>>     Use platform_pci_choose_state()
>> 2. If the device is armed for wakeup:
>>     Select the deepest D-state that supports a PME.
>> 3. Else:
>>     Use D3hot.
>>
>> Devices are considered power manageable by the platform when they have
>> one or more objects described in the table in section 7.3 of the ACPI 6.5
>> specification [1]. In this case the root ports are not power manageable.
>>
>> If devices are not considered power manageable; specs are ambiguous as
>> to what should happen.  In this situation Windows 11 puts PCIe ports
>> in D0 ostensibly due the policy from the "uPEP driver" which is a
>> complimentary driver the Linux "amd-pmc" driver.
>>
>> Linux chooses to allow D3 for these root ports due to the policy
>> introduced by commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during
>> suspend").
>>
>> The Windows uPEP driver expresses the desired state that should be
>> selected for suspend but Linux doesn't, so introduce a quirk for the
>> problematic root ports.
>>
>> The quirk removes PME support for D3hot and D3cold at suspend time if the
>> system will be using s2idle. When the port is configured for wakeup this
>> will prevent these states from being selected in pci_target_state().
>>
>> After the system is resumes the PME support is re-read from the PM
>> capabilities register to allow opportunistic power savings at runtime by
>> letting the root port go into D3hot or D3cold.
> 
> There's a lot of text here, but I think the essential thing is:
> 
>    These Root Ports advertise D3hot and D3cold in the PME_Support
>    register, but PMEs do not work in those states when the amd-pmc
>    driver has put the platform in a sleep state.
> 

It's specific to the PMEs for root ports with USB4 controller connected, 
but yes otherwise correct.

I've confirmed that XHCI controllers connected to other root ports work 
fine.

>> Cc: stable@vger.kernel.org
>> Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/platform-design-for-modern-standby#low-power-core-silicon-cpu-soc-dram [1]
>> Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>> Reported-by: Iain Lane <iain@orangesquash.org.uk>
>> Closes: https://forums.lenovo.com/t5/Ubuntu/Z13-can-t-resume-from-suspend-with-external-USB-keyboard/m-p/5217121
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/pci/quirks.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 61 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index eeec1d6f9023..ebc0afbc814e 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -6188,3 +6188,64 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a31, dpc_log_size);
>>   DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5020, of_pci_make_dev_node);
>>   DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5021, of_pci_make_dev_node);
>>   DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REDHAT, 0x0005, of_pci_make_dev_node);
>> +
>> +/*
>> + * When AMD PCIe root ports with AMD USB4 controllers attached to them are put
>> + * into D3hot or D3cold downstream USB devices may fail to wakeup the system.
>> + * This manifests as a missing wakeup interrupt.
>> + *
>> + * Prevent the associated root port from using PME to wake from D3hot or
>> + * D3cold power states during s2idle.
>> + * This will effectively put the root port into D0 power state over s2idle.
>> + */
>> +static bool child_has_amd_usb4(struct pci_dev *pdev)
>> +{
>> +	struct pci_dev *child = NULL;
>> +
>> +	while ((child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child))) {
>> +		if (child->vendor != PCI_VENDOR_ID_AMD)
>> +			continue;
>> +		if (pcie_find_root_port(child) != pdev)
>> +			continue;
>> +		return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> +static void quirk_reenable_pme(struct pci_dev *dev)
>> +{
>> +	u16 pmc;
>> +
>> +	if (!dev->pm_cap)
>> +		return;
>> +
>> +	if (!child_has_amd_usb4(dev))
>> +		return;
>> +
>> +	pci_read_config_word(dev, dev->pm_cap + PCI_PM_PMC, &pmc);
>> +	pmc &= PCI_PM_CAP_PME_MASK;
>> +	dev->pme_support = pmc >> PCI_PM_CAP_PME_SHIFT;
>> +}
>> +
>> +static void quirk_disable_pme_suspend(struct pci_dev *dev)
>> +{
>> +	int mask;
>> +
>> +	if (pm_suspend_via_firmware())
>> +		return;
> 
> There's always something more to confuse me.  Why does
> pm_suspend_via_firmware() matter?  I can sort of see that Linux
> platform power management, which uses the PMC, is not the same
> as platform firmware being invoked at the end of a system-wide power
> management transition to a sleep state.
> 
> I guess this must have something to do with acpi_suspend_begin() and
> acpi_hibernation_begin() (the callers of
> pm_set_suspend_via_firmware())?
> 

The "why" has to do with the implementation details of how the platform 
enters and exits hardware sleep and what happens.
It's much different than how ACPI S3 works.

Most OEM platforms don't support S3, so if it distracts from the issue 
and quirk I'm fine to drop this check.

>> +	if (!child_has_amd_usb4(dev))
>> +		return;
>> +
>> +	mask = (PCI_PM_CAP_PME_D3hot|PCI_PM_CAP_PME_D3cold) >> PCI_PM_CAP_PME_SHIFT;
>> +	if (!(dev->pme_support & mask))
>> +		return;
>> +	dev->pme_support &= ~mask;
>> +	dev_info_once(&dev->dev, "quirk: disabling PME from D3hot and D3cold at suspend\n");
>> +}
>> +
>> +DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x14b9, quirk_disable_pme_suspend);
>> +DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x14b9, quirk_reenable_pme);
>> +DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x14eb, quirk_disable_pme_suspend);
>> +DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x14eb, quirk_reenable_pme);
>> -- 
>> 2.34.1
>>

Re: [PATCH v19 2/2] PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers

[Lukas Wunner]

On Sun, Sep 17, 2023 at 04:40:15PM -0500, Bjorn Helgaas wrote:
> On Sat, Sep 16, 2023 at 03:36:50PM +0200, Lukas Wunner wrote:
> > The "pcie_find_root_port(child) != pdev" check is always false:
> 
> If we were using pci_walk_bus() and only looking at devices below
> pdev, I would agree, but since we're using pci_get_class(), which
> searches all PCI devices in the system, I'm confused about why it
> would always be false.

Right, I had misread the patch, I indeed thought it was using
pci_walk_bus().  My apologies for the noise.

(I guess I assumed that because a global search (as done here)
is more expensive than just searching below the Root Port.)

Thanks,

Lukas

Re: [PATCH v19 2/2] PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers

[Ilpo Järvinen]

On Thu, 14 Sep 2023, Mario Limonciello wrote:

> Iain reports that USB devices can't be used to wake a Lenovo Z13
> from suspend. This problem occurs because the PCIe root port has been put
> into D3hot and AMD's platform can't handle USB devices waking the platform
> from a hardware sleep state in this case. The platform is put into
> a hardware sleep state by the actions of the amd-pmc driver.
> 
> Although the issue is initially reported on a single model it actually
> affects all Yellow Carp (Rembrandt) and Pink Sardine (Phoenix) SoCs.
> This problem only occurs on Linux specifically when attempting to
> wake the platform from a hardware sleep state.
> Comparing the behavior on Windows and Linux, Windows doesn't put
> the root ports into D3 at this time.
> 
> Linux decides the target state to put the device into at suspend by
> this policy:
> 1. If platform_pci_power_manageable():
>    Use platform_pci_choose_state()
> 2. If the device is armed for wakeup:
>    Select the deepest D-state that supports a PME.
> 3. Else:
>    Use D3hot.
> 
> Devices are considered power manageable by the platform when they have
> one or more objects described in the table in section 7.3 of the ACPI 6.5
> specification [1]. In this case the root ports are not power manageable.
> 
> If devices are not considered power manageable; specs are ambiguous as
> to what should happen.  In this situation Windows 11 puts PCIe ports
> in D0 ostensibly due the policy from the "uPEP driver" which is a
> complimentary driver the Linux "amd-pmc" driver.
> 
> Linux chooses to allow D3 for these root ports due to the policy
> introduced by commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during
> suspend").
> 
> The Windows uPEP driver expresses the desired state that should be
> selected for suspend but Linux doesn't, so introduce a quirk for the
> problematic root ports.
> 
> The quirk removes PME support for D3hot and D3cold at suspend time if the
> system will be using s2idle. When the port is configured for wakeup this
> will prevent these states from being selected in pci_target_state().
> 
> After the system is resumes the PME support is re-read from the PM
> capabilities register to allow opportunistic power savings at runtime by
> letting the root port go into D3hot or D3cold.
> 
> Cc: stable@vger.kernel.org
> Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/platform-design-for-modern-standby#low-power-core-silicon-cpu-soc-dram [1]
> Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> Reported-by: Iain Lane <iain@orangesquash.org.uk>
> Closes: https://forums.lenovo.com/t5/Ubuntu/Z13-can-t-resume-from-suspend-with-external-USB-keyboard/m-p/5217121
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/pci/quirks.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index eeec1d6f9023..ebc0afbc814e 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -6188,3 +6188,64 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a31, dpc_log_size);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5020, of_pci_make_dev_node);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5021, of_pci_make_dev_node);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REDHAT, 0x0005, of_pci_make_dev_node);
> +
> +/*
> + * When AMD PCIe root ports with AMD USB4 controllers attached to them are put
> + * into D3hot or D3cold downstream USB devices may fail to wakeup the system.
> + * This manifests as a missing wakeup interrupt.
> + *
> + * Prevent the associated root port from using PME to wake from D3hot or
> + * D3cold power states during s2idle.
> + * This will effectively put the root port into D0 power state over s2idle.
> + */
> +static bool child_has_amd_usb4(struct pci_dev *pdev)
> +{
> +	struct pci_dev *child = NULL;
> +
> +	while ((child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child))) {
> +		if (child->vendor != PCI_VENDOR_ID_AMD)
> +			continue;
> +		if (pcie_find_root_port(child) != pdev)
> +			continue;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static void quirk_reenable_pme(struct pci_dev *dev)
> +{
> +	u16 pmc;
> +
> +	if (!dev->pm_cap)
> +		return;
> +
> +	if (!child_has_amd_usb4(dev))
> +		return;
> +
> +	pci_read_config_word(dev, dev->pm_cap + PCI_PM_PMC, &pmc);
> +	pmc &= PCI_PM_CAP_PME_MASK;
> +	dev->pme_support = pmc >> PCI_PM_CAP_PME_SHIFT;

FIELD_GET(PCI_PM_CAP_PME_MASK, pmc);

-- 
 i.

> +}
> +
> +static void quirk_disable_pme_suspend(struct pci_dev *dev)
> +{
> +	int mask;
> +
> +	if (pm_suspend_via_firmware())
> +		return;
> +
> +	if (!child_has_amd_usb4(dev))
> +		return;
> +
> +	mask = (PCI_PM_CAP_PME_D3hot|PCI_PM_CAP_PME_D3cold) >> PCI_PM_CAP_PME_SHIFT;
> +	if (!(dev->pme_support & mask))
> +		return;
> +	dev->pme_support &= ~mask;
> +	dev_info_once(&dev->dev, "quirk: disabling PME from D3hot and D3cold at suspend\n");
> +}
> +
> +DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x14b9, quirk_disable_pme_suspend);
> +DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x14b9, quirk_reenable_pme);
> +DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x14eb, quirk_disable_pme_suspend);
> +DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, 0x14eb, quirk_reenable_pme);
>