[PATCH v9 0/2] PCI: Disable AER & DPC on suspend

[PATCH v9 0/2] PCI: Disable AER & DPC on suspend

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

This is an old series from Kai-Heng that I didn't handle soon enough.  The
intent is to fix several suspend/resume issues:

  - Spurious wakeup from s2idle
    (https://bugzilla.kernel.org/show_bug.cgi?id=216295)

  - Steam Deck doesn't resume after suspend
    (https://bugzilla.kernel.org/show_bug.cgi?id=218090)

  - Unexpected ACS error and DPC event when resuming after suspend
    (https://bugzilla.kernel.org/show_bug.cgi?id=209149)

It seems that a glitch when the link is powered down during suspend causes
errors to be logged by AER.  When AER is enabled, this causes an AER
interrupt, and if that IRQ is shared with PME, it may cause a spurious
wakeup.

Also, errors logged during link power-down and power-up seem to cause
unwanted error reporting during resume.

This series disables AER interrupts, DPC triggering, and DPC interrupts
during suspend.  On resume, it clears AER and DPC error status before
re-enabling their interrupts.

I added a couple cosmetic changes for the v9, but this is essentially all
Kai-Heng's work.  I'm just posting it as a v9 because I failed to act on
this earlier.

Bjorn

v9:
 - Drop pci_ancestor_pr3_present() and pm_suspend_via_firmware; do it
   unconditionally
 - Clear DPC status before re-enabling DPC interrupt

v8: https://lore.kernel.org/r/20240416043225.1462548-1-kai.heng.feng@canonical.com
 - Wording.
 - Add more bug reports.

v7:
 - Wording.
 - Disable AER completely (again) if power will be turned off
 - Disable DPC completely (again) if power will be turned off

v6: https://lore.kernel.org/r/20230512000014.118942-1-kai.heng.feng@canonical.com

v5: https://lore.kernel.org/r/20230511133610.99759-1-kai.heng.feng@canonical.com
 - Wording.

v4: https://lore.kernel.org/r/20230424055249.460381-1-kai.heng.feng@canonical.com
v3: https://lore.kernel.org/r/20230420125941.333675-1-kai.heng.feng@canonical.com
 - Correct subject.

v2: https://lore.kernel.org/r/20230420015830.309845-1-kai.heng.feng@canonical.com
 - Only disable AER IRQ.
 - No more AER check on PME IRQ#.
 - Use AER helper.
 - Only disable DPC IRQ.
 - No more DPC check on PME IRQ#.

v1: https://lore.kernel.org/r/20220727013255.269815-1-kai.heng.feng@canonical.com

Kai-Heng Feng (2):
  PCI/AER: Disable AER service on suspend
  PCI/DPC: Disable DPC service on suspend

 drivers/pci/pcie/aer.c | 18 +++++++++++++
 drivers/pci/pcie/dpc.c | 60 +++++++++++++++++++++++++++++++++---------
 2 files changed, 66 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH v9 1/2] PCI/AER: Disable AER service on suspend

[Bjorn Helgaas]

From: Kai-Heng Feng <kai.heng.feng@canonical.com>

If the link is powered off during suspend, electrical noise may cause
errors that are logged via AER.  If the AER interrupt is enabled and shares
an IRQ with PME, that causes a spurious wakeup during suspend.

Disable the AER interrupt during suspend to prevent this.  Clear error
status before re-enabling IRQ interrupts during resume so we don't get an
interrupt for errors that occurred during the suspend/resume process.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=209149
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216295
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218090
Link: https://lore.kernel.org/r/20240416043225.1462548-2-kai.heng.feng@canonical.com
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
[bhelgaas: drop pci_ancestor_pr3_present() etc, commit log]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aer.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index ac6293c24976..13b8586924ea 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1497,6 +1497,22 @@ 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);
+
+	aer_disable_rootport(rpc);
+	return 0;
+}
+
+static int aer_resume(struct pcie_device *dev)
+{
+	struct aer_rpc *rpc = get_service_data(dev);
+
+	aer_enable_rootport(rpc);
+	return 0;
+}
+
 /**
  * aer_root_reset - reset Root Port hierarchy, RCEC, or RCiEP
  * @dev: pointer to Root Port, RCEC, or RCiEP
@@ -1561,6 +1577,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

[PATCH v9 2/2] PCI/DPC: Disable DPC service on suspend

[Bjorn Helgaas]

From: Kai-Heng Feng <kai.heng.feng@canonical.com>

If the link is powered off during suspend, electrical noise may cause
errors that trigger DPC.  If the DPC interrupt is enabled and shares an IRQ
with PME, that causes a spurious wakeup during suspend.

Disable DPC triggering and the DPC interrupt during suspend to prevent
this.  Clear DPC interrupt status before re-enabling DPC interrupts during
resume so we don't get an interrupt for errors that occurred during the
suspend/resume process.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=209149
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216295
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218090
Link: https://lore.kernel.org/r/20240416043225.1462548-3-kai.heng.feng@canonical.com
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
[bhelgaas: clear status on resume, add comments, commit log]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/dpc.c | 60 +++++++++++++++++++++++++++++++++---------
 1 file changed, 48 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index a668820696dc..2b6ef7efa3c1 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -412,13 +412,44 @@ void pci_dpc_init(struct pci_dev *pdev)
 	}
 }
 
+static void dpc_enable(struct pcie_device *dev)
+{
+	struct pci_dev *pdev = dev->port;
+	int dpc = pdev->dpc_cap;
+	u16 ctl;
+
+	/*
+	 * Clear DPC Interrupt Status so we don't get an interrupt for an
+	 * old event when setting DPC Interrupt Enable.
+	 */
+	pci_write_config_word(pdev, dpc + PCI_EXP_DPC_STATUS,
+			      PCI_EXP_DPC_STATUS_INTERRUPT);
+
+	pci_read_config_word(pdev, dpc + PCI_EXP_DPC_CTL, &ctl);
+	ctl &= ~PCI_EXP_DPC_CTL_EN_MASK;
+	ctl |= PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
+	pci_write_config_word(pdev, dpc + PCI_EXP_DPC_CTL, ctl);
+}
+
+static void dpc_disable(struct pcie_device *dev)
+{
+	struct pci_dev *pdev = dev->port;
+	int dpc = pdev->dpc_cap;
+	u16 ctl;
+
+	/* Disable DPC triggering and DPC interrupts */
+	pci_read_config_word(pdev, dpc + PCI_EXP_DPC_CTL, &ctl);
+	ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN);
+	pci_write_config_word(pdev, dpc + PCI_EXP_DPC_CTL, ctl);
+}
+
 #define FLAG(x, y) (((x) & (y)) ? '+' : '-')
 static int dpc_probe(struct pcie_device *dev)
 {
 	struct pci_dev *pdev = dev->port;
 	struct device *device = &dev->device;
 	int status;
-	u16 ctl, cap;
+	u16 cap;
 
 	if (!pcie_aer_is_native(pdev) && !pcie_ports_dpc_native)
 		return -ENOTSUPP;
@@ -433,11 +464,7 @@ static int dpc_probe(struct pcie_device *dev)
 	}
 
 	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap);
-
-	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
-	ctl &= ~PCI_EXP_DPC_CTL_EN_MASK;
-	ctl |= PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
-	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
+	dpc_enable(dev);
 
 	pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
 	pci_info(pdev, "error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n",
@@ -450,14 +477,21 @@ static int dpc_probe(struct pcie_device *dev)
 	return status;
 }
 
+static int dpc_suspend(struct pcie_device *dev)
+{
+	dpc_disable(dev);
+	return 0;
+}
+
+static int dpc_resume(struct pcie_device *dev)
+{
+	dpc_enable(dev);
+	return 0;
+}
+
 static void dpc_remove(struct pcie_device *dev)
 {
-	struct pci_dev *pdev = dev->port;
-	u16 ctl;
-
-	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
-	ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN);
-	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
+	dpc_disable(dev);
 }
 
 static struct pcie_port_service_driver dpcdriver = {
@@ -465,6 +499,8 @@ static struct pcie_port_service_driver dpcdriver = {
 	.port_type	= PCIE_ANY_PORT,
 	.service	= PCIE_PORT_SERVICE_DPC,
 	.probe		= dpc_probe,
+	.suspend	= dpc_suspend,
+	.resume		= dpc_resume,
 	.remove		= dpc_remove,
 };
 
-- 
2.34.1

Re: [PATCH v9 0/2] PCI: Disable AER & DPC on suspend

[Rafael J. Wysocki]

On Tue, Jun 18, 2024 at 10:49 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> This is an old series from Kai-Heng that I didn't handle soon enough.  The
> intent is to fix several suspend/resume issues:
>
>   - Spurious wakeup from s2idle
>     (https://bugzilla.kernel.org/show_bug.cgi?id=216295)
>
>   - Steam Deck doesn't resume after suspend
>     (https://bugzilla.kernel.org/show_bug.cgi?id=218090)
>
>   - Unexpected ACS error and DPC event when resuming after suspend
>     (https://bugzilla.kernel.org/show_bug.cgi?id=209149)
>
> It seems that a glitch when the link is powered down during suspend causes
> errors to be logged by AER.  When AER is enabled, this causes an AER
> interrupt, and if that IRQ is shared with PME, it may cause a spurious
> wakeup.
>
> Also, errors logged during link power-down and power-up seem to cause
> unwanted error reporting during resume.
>
> This series disables AER interrupts, DPC triggering, and DPC interrupts
> during suspend.  On resume, it clears AER and DPC error status before
> re-enabling their interrupts.
>
> I added a couple cosmetic changes for the v9, but this is essentially all
> Kai-Heng's work.  I'm just posting it as a v9 because I failed to act on
> this earlier.
>
> Bjorn
>
> v9:
>  - Drop pci_ancestor_pr3_present() and pm_suspend_via_firmware; do it
>    unconditionally
>  - Clear DPC status before re-enabling DPC interrupt
>
> v8: https://lore.kernel.org/r/20240416043225.1462548-1-kai.heng.feng@canonical.com
>  - Wording.
>  - Add more bug reports.
>
> v7:
>  - Wording.
>  - Disable AER completely (again) if power will be turned off
>  - Disable DPC completely (again) if power will be turned off
>
> v6: https://lore.kernel.org/r/20230512000014.118942-1-kai.heng.feng@canonical.com
>
> v5: https://lore.kernel.org/r/20230511133610.99759-1-kai.heng.feng@canonical.com
>  - Wording.
>
> v4: https://lore.kernel.org/r/20230424055249.460381-1-kai.heng.feng@canonical.com
> v3: https://lore.kernel.org/r/20230420125941.333675-1-kai.heng.feng@canonical.com
>  - Correct subject.
>
> v2: https://lore.kernel.org/r/20230420015830.309845-1-kai.heng.feng@canonical.com
>  - Only disable AER IRQ.
>  - No more AER check on PME IRQ#.
>  - Use AER helper.
>  - Only disable DPC IRQ.
>  - No more DPC check on PME IRQ#.
>
> v1: https://lore.kernel.org/r/20220727013255.269815-1-kai.heng.feng@canonical.com
>
> Kai-Heng Feng (2):
>   PCI/AER: Disable AER service on suspend
>   PCI/DPC: Disable DPC service on suspend
>
>  drivers/pci/pcie/aer.c | 18 +++++++++++++
>  drivers/pci/pcie/dpc.c | 60 +++++++++++++++++++++++++++++++++---------
>  2 files changed, 66 insertions(+), 12 deletions(-)
>
> --

Please feel free to add

Acked-by: Rafael J. Wysocki <rafael@kernel.org>

to both patches in the series.

Thanks!