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

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

[Kai-Heng Feng]

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(-)

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;
-- 
2.36.1

[PATCH 2/3] PCI/AER: Disable AER service on suspend when IRQ is shared with PME

[Kai-Heng Feng]

PCIe service that shares IRQ with PME may cause spurious wakeup on
system suspend.

PCIe Base Spec 5.0, section 5.2 "Link State Power Management" states
that TLP and DLLP transmission is disabled for a Link in L2/L3 Ready
(D3hot), L2 (D3cold with aux power) and L3 (D3cold), so we don't lose
much here to disable AER during system suspend.

This is very similar to previous attempts to suspend AER and DPC [1],
but with a different reason.

[1] https://lore.kernel.org/linux-pci/20220408153159.106741-1-kai.heng.feng@canonical.com/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216295

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/pci/pcie/aer.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 7952e5efd6cf3..60cc373754af2 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1372,6 +1372,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);
+
+	if (dev->shared_pme_irq)
+		aer_disable_rootport(rpc);
+
+	return 0;
+}
+
+static int aer_resume(struct pcie_device *dev)
+{
+	struct aer_rpc *rpc = get_service_data(dev);
+
+	if (dev->shared_pme_irq)
+		aer_enable_rootport(rpc);
+
+	return 0;
+}
+
 /**
  * aer_root_reset - reset Root Port hierarchy, RCEC, or RCiEP
  * @dev: pointer to Root Port, RCEC, or RCiEP
@@ -1441,8 +1461,9 @@ static struct pcie_port_service_driver aerdriver = {
 	.name		= "aer",
 	.port_type	= PCIE_ANY_PORT,
 	.service	= PCIE_PORT_SERVICE_AER,
-
 	.probe		= aer_probe,
+	.suspend	= aer_suspend,
+	.resume		= aer_resume,
 	.remove		= aer_remove,
 };
 
-- 
2.36.1

[PATCH 3/3] PCI/DPC: Disable DPC service on suspend when IRQ is shared with PME

[Kai-Heng Feng]

PCIe service that shares IRQ with PME may cause spurious wakeup on
system suspend.

Since AER is conditionally disabled in previous patch, also apply the
same condition to disable DPC which depends on AER to work.

PCIe Base Spec 5.0, section 5.2 "Link State Power Management" states
that TLP and DLLP transmission is disabled for a Link in L2/L3 Ready
(D3hot), L2 (D3cold with aux power) and L3 (D3cold), so we don't lose
much here to disable DPC during system suspend.

This is very similar to previous attempts to suspend AER and DPC [1],
but with a different reason.

[1] https://lore.kernel.org/linux-pci/20220408153159.106741-1-kai.heng.feng@canonical.com/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216295

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/pci/pcie/dpc.c | 52 +++++++++++++++++++++++++++++++++---------
 1 file changed, 41 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 3e9afee02e8d1..542f282c43f75 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -343,13 +343,33 @@ void pci_dpc_init(struct pci_dev *pdev)
 	}
 }
 
+static void dpc_enable(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 = (ctl & 0xfff4) | 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);
+}
+
+static void dpc_disable(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);
+}
+
 #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;
@@ -364,10 +384,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 = (ctl & 0xfff4) | 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",
@@ -380,14 +397,25 @@ static int dpc_probe(struct pcie_device *dev)
 	return status;
 }
 
-static void dpc_remove(struct pcie_device *dev)
+static int dpc_suspend(struct pcie_device *dev)
 {
-	struct pci_dev *pdev = dev->port;
-	u16 ctl;
+	if (dev->shared_pme_irq)
+		dpc_disable(dev);
 
-	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);
+	return 0;
+}
+
+static int dpc_resume(struct pcie_device *dev)
+{
+	if (dev->shared_pme_irq)
+		dpc_enable(dev);
+
+	return 0;
+}
+
+static void dpc_remove(struct pcie_device *dev)
+{
+	dpc_disable(dev);
 }
 
 static struct pcie_port_service_driver dpcdriver = {
@@ -395,6 +423,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.36.1

Re: [PATCH 3/3] PCI/DPC: Disable DPC service on suspend when IRQ is shared with PME

[Bjorn Helgaas]

On Wed, Jul 27, 2022 at 09:32:52AM +0800, Kai-Heng Feng wrote:
> PCIe service that shares IRQ with PME may cause spurious wakeup on
> system suspend.
> 
> Since AER is conditionally disabled in previous patch, also apply the
> same condition to disable DPC which depends on AER to work.
> 
> PCIe Base Spec 5.0, section 5.2 "Link State Power Management" states
> that TLP and DLLP transmission is disabled for a Link in L2/L3 Ready
> (D3hot), L2 (D3cold with aux power) and L3 (D3cold), so we don't lose
> much here to disable DPC during system suspend.
> 
> This is very similar to previous attempts to suspend AER and DPC [1],
> but with a different reason.
> 
> [1] https://lore.kernel.org/linux-pci/20220408153159.106741-1-kai.heng.feng@canonical.com/
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216295
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/pci/pcie/dpc.c | 52 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 3e9afee02e8d1..542f282c43f75 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -343,13 +343,33 @@ void pci_dpc_init(struct pci_dev *pdev)
>  	}
>  }
>  
> +static void dpc_enable(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 = (ctl & 0xfff4) | 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);
> +}

I guess the reason we need this is because we disable interupts in
pci_pm_suspend() first, then we call pci_save_dpc_state() from
pci_pm_suspend_noirq(), so we save the *disabled* control register.
Then when we resume, we restore that disabled control register so we
need to enable DPC again.  Right?

I think we should save a "dpc_enabled" bit in the pci_dev and
conditionally set PCI_EXP_DPC_CTL_INT_EN here.  If we unconditionally
set it here, we depend on portdrv *not* calling dpc_resume() if we
didn't enable DPC at enumeration-time for some reason.

And I would leave PCI_EXP_DPC_CTL_EN_FATAL alone; see below.

> +static void dpc_disable(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);

  #define  PCI_EXP_DPC_CTL_EN_FATAL       0x0001
  #define  PCI_EXP_DPC_CTL_INT_EN         0x0008

Clearing PCI_EXP_DPC_CTL_INT_EN makes sense to me, but I don't
understand the PCI_EXP_DPC_CTL_EN_FATAL part.

PCI_EXP_DPC_CTL_EN_FATAL is one of the four values of the two-bit DPC
Trigger Enable, so clearing that bit leaves the field as either 00b
(DPC is disabled) or 10b (DPC enabled and triggered when the port
detects an uncorrectable error or receives an ERR_NONFATAL or
ERR_FATAL message).

I think we should only clear PCI_EXP_DPC_CTL_INT_EN.

> +}
> +
>  #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;
> @@ -364,10 +384,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 = (ctl & 0xfff4) | 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);

I think we should keep the PCI_EXP_DPC_CTL_EN_FATAL part here. That
just sets the desired trigger mode but AFAICT, has nothing to do with
generating interrupts.

> +	dpc_enable(dev);

Then dpc_enable() could be called something like dpc_enable_irq(), and
it would *only* control interupt generation.

>  	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",
> @@ -380,14 +397,25 @@ static int dpc_probe(struct pcie_device *dev)
>  	return status;
>  }
>  
> -static void dpc_remove(struct pcie_device *dev)
> +static int dpc_suspend(struct pcie_device *dev)
>  {
> -	struct pci_dev *pdev = dev->port;
> -	u16 ctl;
> +	if (dev->shared_pme_irq)
> +		dpc_disable(dev);
>  
> -	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);
> +	return 0;
> +}
> +
> +static int dpc_resume(struct pcie_device *dev)
> +{
> +	if (dev->shared_pme_irq)
> +		dpc_enable(dev);
> +
> +	return 0;
> +}
> +
> +static void dpc_remove(struct pcie_device *dev)
> +{
> +	dpc_disable(dev);
>  }
>  
>  static struct pcie_port_service_driver dpcdriver = {
> @@ -395,6 +423,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.36.1
> 

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

[Bjorn Helgaas]

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

Re: [PATCH 2/3] PCI/AER: Disable AER service on suspend when IRQ is shared with PME

[Bjorn Helgaas]

On Wed, Jul 27, 2022 at 09:32:51AM +0800, Kai-Heng Feng wrote:
> PCIe service that shares IRQ with PME may cause spurious wakeup on
> system suspend.
> 
> PCIe Base Spec 5.0, section 5.2 "Link State Power Management" states
> that TLP and DLLP transmission is disabled for a Link in L2/L3 Ready
> (D3hot), L2 (D3cold with aux power) and L3 (D3cold), so we don't lose
> much here to disable AER during system suspend.
> 
> This is very similar to previous attempts to suspend AER and DPC [1],
> but with a different reason.
> 
> [1] https://lore.kernel.org/linux-pci/20220408153159.106741-1-kai.heng.feng@canonical.com/
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216295
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/pci/pcie/aer.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 7952e5efd6cf3..60cc373754af2 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -1372,6 +1372,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);
> +
> +	if (dev->shared_pme_irq)
> +		aer_disable_rootport(rpc);

aer_disable_rootport() seems like it might be overkill.  IIUC, what
we want to do here is disable AER interrupts, which should only
require clearing ROOT_PORT_INTR_ON_MESG_MASK in PCI_ERR_ROOT_COMMAND.

In addition to clearing ROOT_PORT_INTR_ON_MESG_MASK,
aer_disable_rootport() traverses the whole hierarchy, clearing
PCI_EXP_AER_FLAGS (CERE | NFERE | FERE | URRE) in PCI_EXP_DEVCTL.
I don't think these DEVCTL bits control interrupt generation, so I
don't know why we need to touch them.

aer_disable_rootport() also clears PCI_ERR_ROOT_STATUS, which I think
we should not do during suspend either.  We might want to clear it
on resume (which we already do in pci_restore_state()), but I think
generally we should preserve error information as long as it doesn't
cause trouble.

Your thoughts please :)

> +
> +	return 0;
> +}
> +
> +static int aer_resume(struct pcie_device *dev)
> +{
> +	struct aer_rpc *rpc = get_service_data(dev);
> +
> +	if (dev->shared_pme_irq)
> +		aer_enable_rootport(rpc);
> +
> +	return 0;
> +}
> +
>  /**
>   * aer_root_reset - reset Root Port hierarchy, RCEC, or RCiEP
>   * @dev: pointer to Root Port, RCEC, or RCiEP
> @@ -1441,8 +1461,9 @@ static struct pcie_port_service_driver aerdriver = {
>  	.name		= "aer",
>  	.port_type	= PCIE_ANY_PORT,
>  	.service	= PCIE_PORT_SERVICE_AER,
> -
>  	.probe		= aer_probe,
> +	.suspend	= aer_suspend,
> +	.resume		= aer_resume,
>  	.remove		= aer_remove,
>  };
>  
> -- 
> 2.36.1
> 

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

[Kai-Heng Feng]

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

Re: [PATCH 2/3] PCI/AER: Disable AER service on suspend when IRQ is shared with PME

[Kai-Heng Feng]

On Thu, Sep 29, 2022 at 5:46 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Jul 27, 2022 at 09:32:51AM +0800, Kai-Heng Feng wrote:
> > PCIe service that shares IRQ with PME may cause spurious wakeup on
> > system suspend.
> >
> > PCIe Base Spec 5.0, section 5.2 "Link State Power Management" states
> > that TLP and DLLP transmission is disabled for a Link in L2/L3 Ready
> > (D3hot), L2 (D3cold with aux power) and L3 (D3cold), so we don't lose
> > much here to disable AER during system suspend.
> >
> > This is very similar to previous attempts to suspend AER and DPC [1],
> > but with a different reason.
> >
> > [1] https://lore.kernel.org/linux-pci/20220408153159.106741-1-kai.heng.feng@canonical.com/
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216295
> >
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> >  drivers/pci/pcie/aer.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > index 7952e5efd6cf3..60cc373754af2 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -1372,6 +1372,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);
> > +
> > +     if (dev->shared_pme_irq)
> > +             aer_disable_rootport(rpc);
>
> aer_disable_rootport() seems like it might be overkill.  IIUC, what
> we want to do here is disable AER interrupts, which should only
> require clearing ROOT_PORT_INTR_ON_MESG_MASK in PCI_ERR_ROOT_COMMAND.
>
> In addition to clearing ROOT_PORT_INTR_ON_MESG_MASK,
> aer_disable_rootport() traverses the whole hierarchy, clearing
> PCI_EXP_AER_FLAGS (CERE | NFERE | FERE | URRE) in PCI_EXP_DEVCTL.
> I don't think these DEVCTL bits control interrupt generation, so I
> don't know why we need to touch them.
>
> aer_disable_rootport() also clears PCI_ERR_ROOT_STATUS, which I think
> we should not do during suspend either.  We might want to clear it
> on resume (which we already do in pci_restore_state()), but I think
> generally we should preserve error information as long as it doesn't
> cause trouble.
>
> Your thoughts please :)

Sorry for the belated response.

Clearing ROOT_PORT_INTR_ON_MESG_MASK along to disable interrupt can
solve the issue too.
And I agree that the AER information should be preserved too.

Kai-Heng

>
> > +
> > +     return 0;
> > +}
> > +
> > +static int aer_resume(struct pcie_device *dev)
> > +{
> > +     struct aer_rpc *rpc = get_service_data(dev);
> > +
> > +     if (dev->shared_pme_irq)
> > +             aer_enable_rootport(rpc);
> > +
> > +     return 0;
> > +}
> > +
> >  /**
> >   * aer_root_reset - reset Root Port hierarchy, RCEC, or RCiEP
> >   * @dev: pointer to Root Port, RCEC, or RCiEP
> > @@ -1441,8 +1461,9 @@ static struct pcie_port_service_driver aerdriver = {
> >       .name           = "aer",
> >       .port_type      = PCIE_ANY_PORT,
> >       .service        = PCIE_PORT_SERVICE_AER,
> > -
> >       .probe          = aer_probe,
> > +     .suspend        = aer_suspend,
> > +     .resume         = aer_resume,
> >       .remove         = aer_remove,
> >  };
> >
> > --
> > 2.36.1
> >

Re: [PATCH 3/3] PCI/DPC: Disable DPC service on suspend when IRQ is shared with PME

[Kai-Heng Feng]

On Thu, Sep 29, 2022 at 5:24 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Jul 27, 2022 at 09:32:52AM +0800, Kai-Heng Feng wrote:
> > PCIe service that shares IRQ with PME may cause spurious wakeup on
> > system suspend.
> >
> > Since AER is conditionally disabled in previous patch, also apply the
> > same condition to disable DPC which depends on AER to work.
> >
> > PCIe Base Spec 5.0, section 5.2 "Link State Power Management" states
> > that TLP and DLLP transmission is disabled for a Link in L2/L3 Ready
> > (D3hot), L2 (D3cold with aux power) and L3 (D3cold), so we don't lose
> > much here to disable DPC during system suspend.
> >
> > This is very similar to previous attempts to suspend AER and DPC [1],
> > but with a different reason.
> >
> > [1] https://lore.kernel.org/linux-pci/20220408153159.106741-1-kai.heng.feng@canonical.com/
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216295
> >
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> >  drivers/pci/pcie/dpc.c | 52 +++++++++++++++++++++++++++++++++---------
> >  1 file changed, 41 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > index 3e9afee02e8d1..542f282c43f75 100644
> > --- a/drivers/pci/pcie/dpc.c
> > +++ b/drivers/pci/pcie/dpc.c
> > @@ -343,13 +343,33 @@ void pci_dpc_init(struct pci_dev *pdev)
> >       }
> >  }
> >
> > +static void dpc_enable(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 = (ctl & 0xfff4) | 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);
> > +}
>

Sorry for the belated response.

> I guess the reason we need this is because we disable interupts in
> pci_pm_suspend() first, then we call pci_save_dpc_state() from
> pci_pm_suspend_noirq(), so we save the *disabled* control register.
> Then when we resume, we restore that disabled control register so we
> need to enable DPC again.  Right?

Sorry for the belated response.

Yes, and the same logic applies to AER too.

>
> I think we should save a "dpc_enabled" bit in the pci_dev and
> conditionally set PCI_EXP_DPC_CTL_INT_EN here.  If we unconditionally
> set it here, we depend on portdrv *not* calling dpc_resume() if we
> didn't enable DPC at enumeration-time for some reason.

Does this scenario really happen?
Once the port is marked with PCIE_PORT_SERVICE_DPC, DPC will be
enabled by dpc_probe().
So an additional bit seems to be unnecessary.

>
> And I would leave PCI_EXP_DPC_CTL_EN_FATAL alone; see below.
>
> > +static void dpc_disable(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);
>
>   #define  PCI_EXP_DPC_CTL_EN_FATAL       0x0001
>   #define  PCI_EXP_DPC_CTL_INT_EN         0x0008
>
> Clearing PCI_EXP_DPC_CTL_INT_EN makes sense to me, but I don't
> understand the PCI_EXP_DPC_CTL_EN_FATAL part.
>
> PCI_EXP_DPC_CTL_EN_FATAL is one of the four values of the two-bit DPC
> Trigger Enable, so clearing that bit leaves the field as either 00b
> (DPC is disabled) or 10b (DPC enabled and triggered when the port
> detects an uncorrectable error or receives an ERR_NONFATAL or
> ERR_FATAL message).
>
> I think we should only clear PCI_EXP_DPC_CTL_INT_EN.

Yes, clearing PCI_EXP_DPC_CTL_INT_EN should be sufficient.

>
> > +}
> > +
> >  #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;
> > @@ -364,10 +384,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 = (ctl & 0xfff4) | 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);
>
> I think we should keep the PCI_EXP_DPC_CTL_EN_FATAL part here. That
> just sets the desired trigger mode but AFAICT, has nothing to do with
> generating interrupts.

Agree. Will do it in next revision.

>
> > +     dpc_enable(dev);
>
> Then dpc_enable() could be called something like dpc_enable_irq(), and
> it would *only* control interupt generation.

Will do.

Kai-Heng

>
> >       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",
> > @@ -380,14 +397,25 @@ static int dpc_probe(struct pcie_device *dev)
> >       return status;
> >  }
> >
> > -static void dpc_remove(struct pcie_device *dev)
> > +static int dpc_suspend(struct pcie_device *dev)
> >  {
> > -     struct pci_dev *pdev = dev->port;
> > -     u16 ctl;
> > +     if (dev->shared_pme_irq)
> > +             dpc_disable(dev);
> >
> > -     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);
> > +     return 0;
> > +}
> > +
> > +static int dpc_resume(struct pcie_device *dev)
> > +{
> > +     if (dev->shared_pme_irq)
> > +             dpc_enable(dev);
> > +
> > +     return 0;
> > +}
> > +
> > +static void dpc_remove(struct pcie_device *dev)
> > +{
> > +     dpc_disable(dev);
> >  }
> >
> >  static struct pcie_port_service_driver dpcdriver = {
> > @@ -395,6 +423,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.36.1
> >