diff mbox series

[1/3] PCI/portdrv: Flag services when IRQ is shared with PME

Message ID 20220727013255.269815-1-kai.heng.feng@canonical.com (mailing list archive)
State Changes Requested
Delegated to: Bjorn Helgaas
Headers show
Series [1/3] PCI/portdrv: Flag services when IRQ is shared with PME | expand

Commit Message

Kai-Heng Feng July 27, 2022, 1:32 a.m. UTC
After commit cb1f65c1e142 ("PM: s2idle: ACPI: Fix wakeup interrupts
handling"), there's a system that always gets woken up by spurious PME
event when one of the root port is put to D3cold.

'/sys/power/pm_wakeup_irq' shows 122, which is an IRQ shared between
PME, AER and DPC:
pcieport 0000:00:01.0: PME: Signaling with IRQ 122
pcieport 0000:00:01.0: AER: enabled with IRQ 122
pcieport 0000:00:01.0: DPC: enabled with IRQ 122

Disabling services one by one and the issue goes away when
PCIE_PORT_SERVICE_AER is not enabled. Following the lead, more info can
be found on resume when pci_aer_clear_status() is removed from
pci_restore_state() to print out what happened:
pcieport 0000:00:01.0: AER: Corrected error received: 0000:00:01.0
pcieport 0000:00:01.0: PCIe Bus Error: severity=Corrected, type=Physical Layer, (Receiver ID)
pcieport 0000:00:01.0:   device [8086:4c01] error status/mask=00000001/00002000
pcieport 0000:00:01.0:    [ 0] RxErr

Since the corrected AER error happens at physical layer when the root
port is transitioning to D3cold, making system be able to suspend is
more important than reporting issues like this.

So introduce a new flag to indicate when IRQ is shared with PME,
therefore AER and DPC can be suspended to prevent any spurious wakeup.
HP already has its own suspend routine so it doesn't need to use this
flag.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216295
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/pci/pcie/portdrv.h      | 11 ++++++-----
 drivers/pci/pcie/portdrv_core.c | 21 +++++++++++++++------
 2 files changed, 21 insertions(+), 11 deletions(-)

Comments

Bjorn Helgaas Sept. 28, 2022, 9:32 p.m. UTC | #1
On Wed, Jul 27, 2022 at 09:32:50AM +0800, Kai-Heng Feng wrote:
> After commit cb1f65c1e142 ("PM: s2idle: ACPI: Fix wakeup interrupts
> handling"), there's a system that always gets woken up by spurious PME
> event when one of the root port is put to D3cold.
> 
> '/sys/power/pm_wakeup_irq' shows 122, which is an IRQ shared between
> PME, AER and DPC:
> pcieport 0000:00:01.0: PME: Signaling with IRQ 122
> pcieport 0000:00:01.0: AER: enabled with IRQ 122
> pcieport 0000:00:01.0: DPC: enabled with IRQ 122
> 
> Disabling services one by one and the issue goes away when
> PCIE_PORT_SERVICE_AER is not enabled. Following the lead, more info can
> be found on resume when pci_aer_clear_status() is removed from
> pci_restore_state() to print out what happened:
> pcieport 0000:00:01.0: AER: Corrected error received: 0000:00:01.0
> pcieport 0000:00:01.0: PCIe Bus Error: severity=Corrected, type=Physical Layer, (Receiver ID)
> pcieport 0000:00:01.0:   device [8086:4c01] error status/mask=00000001/00002000
> pcieport 0000:00:01.0:    [ 0] RxErr
> 
> Since the corrected AER error happens at physical layer when the root
> port is transitioning to D3cold, making system be able to suspend is
> more important than reporting issues like this.
> 
> So introduce a new flag to indicate when IRQ is shared with PME,
> therefore AER and DPC can be suspended to prevent any spurious wakeup.
> HP already has its own suspend routine so it doesn't need to use this
> flag.

I think it probably does make sense to disable AER and DPC interrupts
during suspend.  I'm not sure it makes sense to do that conditionally
based on whether the interrupt is shared.  I think I'd rather disable
them always, whether the interrupt is shared or not, because then we
would do the same thing on all machines.  What do you think?

Bjorn
Kai-Heng Feng April 17, 2023, 9:37 a.m. UTC | #2
On Thu, Sep 29, 2022 at 5:32 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Jul 27, 2022 at 09:32:50AM +0800, Kai-Heng Feng wrote:
> > After commit cb1f65c1e142 ("PM: s2idle: ACPI: Fix wakeup interrupts
> > handling"), there's a system that always gets woken up by spurious PME
> > event when one of the root port is put to D3cold.
> >
> > '/sys/power/pm_wakeup_irq' shows 122, which is an IRQ shared between
> > PME, AER and DPC:
> > pcieport 0000:00:01.0: PME: Signaling with IRQ 122
> > pcieport 0000:00:01.0: AER: enabled with IRQ 122
> > pcieport 0000:00:01.0: DPC: enabled with IRQ 122
> >
> > Disabling services one by one and the issue goes away when
> > PCIE_PORT_SERVICE_AER is not enabled. Following the lead, more info can
> > be found on resume when pci_aer_clear_status() is removed from
> > pci_restore_state() to print out what happened:
> > pcieport 0000:00:01.0: AER: Corrected error received: 0000:00:01.0
> > pcieport 0000:00:01.0: PCIe Bus Error: severity=Corrected, type=Physical Layer, (Receiver ID)
> > pcieport 0000:00:01.0:   device [8086:4c01] error status/mask=00000001/00002000
> > pcieport 0000:00:01.0:    [ 0] RxErr
> >
> > Since the corrected AER error happens at physical layer when the root
> > port is transitioning to D3cold, making system be able to suspend is
> > more important than reporting issues like this.
> >
> > So introduce a new flag to indicate when IRQ is shared with PME,
> > therefore AER and DPC can be suspended to prevent any spurious wakeup.
> > HP already has its own suspend routine so it doesn't need to use this
> > flag.
>
> I think it probably does make sense to disable AER and DPC interrupts
> during suspend.  I'm not sure it makes sense to do that conditionally
> based on whether the interrupt is shared.  I think I'd rather disable
> them always, whether the interrupt is shared or not, because then we
> would do the same thing on all machines.  What do you think?

Sorry for the belated response.

I think that sounds reasonable.
I'll send a new version that unconditionally disable AER and DPC IRQ on suspend.

Kai-Heng

>
> Bjorn
diff mbox series

Patch

diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index 0ef4bf5f811d9..97a9d15638cc8 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -57,11 +57,12 @@  static inline int pcie_dpc_init(void) { return 0; }
 #define PCIE_ANY_PORT			(~0)
 
 struct pcie_device {
-	int		irq;	    /* Service IRQ/MSI/MSI-X Vector */
-	struct pci_dev *port;	    /* Root/Upstream/Downstream Port */
-	u32		service;    /* Port service this device represents */
-	void		*priv_data; /* Service Private Data */
-	struct device	device;     /* Generic Device Interface */
+	int		irq;	        /* Service IRQ/MSI/MSI-X Vector */
+	struct pci_dev *port;	        /* Root/Upstream/Downstream Port */
+	u32		service;        /* Port service this device represents */
+	bool		shared_pme_irq; /* Service IRQ shared with PME */
+	void		*priv_data;     /* Service Private Data */
+	struct device	device;         /* Generic Device Interface */
 };
 #define to_pcie_device(d) container_of(d, struct pcie_device, device)
 
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 604feeb84ee40..ddc6854cdde2d 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -274,7 +274,7 @@  static int get_port_device_capability(struct pci_dev *dev)
  * @service: Type of service to associate with the service device
  * @irq: Interrupt vector to associate with the service device
  */
-static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
+static struct pcie_device *pcie_device_init(struct pci_dev *pdev, int service, int irq)
 {
 	int retval;
 	struct pcie_device *pcie;
@@ -282,7 +282,7 @@  static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
 
 	pcie = kzalloc(sizeof(*pcie), GFP_KERNEL);
 	if (!pcie)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 	pcie->port = pdev;
 	pcie->irq = irq;
 	pcie->service = service;
@@ -300,12 +300,12 @@  static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
 	retval = device_register(device);
 	if (retval) {
 		put_device(device);
-		return retval;
+		return ERR_PTR(retval);
 	}
 
 	pm_runtime_no_callbacks(device);
 
-	return 0;
+	return pcie;
 }
 
 /**
@@ -350,10 +350,19 @@  int pcie_port_device_register(struct pci_dev *dev)
 	nr_service = 0;
 	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
 		int service = 1 << i;
+		struct pcie_device *pcie;
 		if (!(capabilities & service))
 			continue;
-		if (!pcie_device_init(dev, service, irqs[i]))
-			nr_service++;
+
+		pcie = pcie_device_init(dev, service, irqs[i]);
+		if (IS_ERR(pcie))
+			continue;
+
+		nr_service++;
+
+		if (i != PCIE_PORT_SERVICE_PME_SHIFT &&
+		    irqs[i] == irqs[PCIE_PORT_SERVICE_PME_SHIFT])
+			pcie->shared_pme_irq = true;
 	}
 	if (!nr_service)
 		goto error_cleanup_irqs;