On Fri, Jul 21, 2023 at 11:58 AM Kai-Heng Feng
[off-list ref] wrote:

On Tue, Jul 18, 2023 at 7:17 PM Bjorn Helgaas [off-list ref] wrote:

quoted

[+cc Rafael]

On Fri, May 12, 2023 at 08:00:13AM +0800, Kai-Heng Feng wrote:

quoted

PCIe services that share an IRQ with PME, such as AER or DPC, may cause a
spurious wakeup on system suspend. To prevent this, disable the AER interrupt
notification during the system suspend process.

I see that in this particular BZ dmesg log, PME, AER, and DPC do share
the same IRQ, but I don't think this is true in general.

Root Ports usually use MSI or MSI-X.  PME and hotplug events use the
Interrupt Message Number in the PCIe Capability, but AER uses the one
in the AER Root Error Status register, and DPC uses the one in the DPC
Capability register.  Those potentially correspond to three distinct
MSI/MSI-X vectors.

I think this probably has nothing to do with the IRQ being *shared*,
but just that putting the downstream component into D3cold, where the
link state is L3, may cause the upstream component to log and signal a
link-related error as the link goes completely down.

That's quite likely a better explanation than my wording.
Assuming AER IRQ and PME IRQ are not shared, does system get woken up
by AER IRQ?

quoted

I don't think D0-D3hot should be relevant here because in all those
states, the link should be active because the downstream config space
remains accessible.  So I'm not sure if it's possible, but I wonder if
there's a more targeted place we could do this, e.g., in the path that
puts downstream devices in D3cold.

Let me try to work on this.

We are seeing another case where the issue happens on D3hot [0].
So I wonder if it's possible to disable AER unconditionally?

[0] https://bugzilla.kernel.org/show_bug.cgi?id=216295#c3

Kai-Heng

quoted

quoted

As Per PCIe Base Spec 5.0, section 5.2, titled "Link State Power Management",
TLP and DLLP transmission are disabled for a Link in L2/L3 Ready (D3hot), L2
(D3cold with aux power) and L3 (D3cold) states. So disabling the AER
notification during suspend and re-enabling them during the resume process
should not affect the basic functionality.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=216295
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Kai-Heng Feng <redacted>
---
v6:
v5:
 - Wording.

v4:
v3:
 - No change.

v2:
 - Only disable AER IRQ.
 - No more check on PME IRQ#.
 - Use helper.

 drivers/pci/pcie/aer.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 1420e1f27105..9c07fdbeb52d 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c

@@ -1356,6 +1356,26 @@ static int aer_probe(struct pcie_device *dev)
      return 0;
 }

+static int aer_suspend(struct pcie_device *dev)
+{
+     struct aer_rpc *rpc = get_service_data(dev);
+     struct pci_dev *pdev = rpc->rpd;
+
+     aer_disable_irq(pdev);
+
+     return 0;
+}
+
+static int aer_resume(struct pcie_device *dev)
+{
+     struct aer_rpc *rpc = get_service_data(dev);
+     struct pci_dev *pdev = rpc->rpd;
+
+     aer_enable_irq(pdev);
+
+     return 0;
+}
+
 /**
  * aer_root_reset - reset Root Port hierarchy, RCEC, or RCiEP
  * @dev: pointer to Root Port, RCEC, or RCiEP

@@ -1420,6 +1440,8 @@ static struct pcie_port_service_driver aerdriver = {
      .service        = PCIE_PORT_SERVICE_AER,

      .probe          = aer_probe,
+     .suspend        = aer_suspend,
+     .resume         = aer_resume,
      .remove         = aer_remove,
 };

--

2.34.1