[PATCH v6 1/3] PCI/AER: Factor out interrupt toggling into helpers

[PATCH v6 1/3] PCI/AER: Factor out interrupt toggling into helpers

[Kai-Heng Feng]

There are many places that enable and disable AER interrupt, so move
them into helpers.

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v6:
 - No change.

v5:
 - Fix misspelling.

v4:
 - No change.

v3:
 - Correct subject.

v2:
 - New patch.

 drivers/pci/pcie/aer.c | 45 +++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index f6c24ded134c..1420e1f27105 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1227,6 +1227,28 @@ static irqreturn_t aer_irq(int irq, void *context)
 	return IRQ_WAKE_THREAD;
 }
 
+static void aer_enable_irq(struct pci_dev *pdev)
+{
+	int aer = pdev->aer_cap;
+	u32 reg32;
+
+	/* Enable Root Port's interrupt in response to error messages */
+	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
+	reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
+	pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
+}
+
+static void aer_disable_irq(struct pci_dev *pdev)
+{
+	int aer = pdev->aer_cap;
+	u32 reg32;
+
+	/* Disable Root's interrupt in response to error messages */
+	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
+	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
+	pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
+}
+
 /**
  * aer_enable_rootport - enable Root Port's interrupts when receiving messages
  * @rpc: pointer to a Root Port data structure
@@ -1256,10 +1278,7 @@ static void aer_enable_rootport(struct aer_rpc *rpc)
 	pci_read_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, &reg32);
 	pci_write_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, reg32);
 
-	/* Enable Root Port's interrupt in response to error messages */
-	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
-	reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
-	pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
+	aer_enable_irq(pdev);
 }
 
 /**
@@ -1274,10 +1293,7 @@ static void aer_disable_rootport(struct aer_rpc *rpc)
 	int aer = pdev->aer_cap;
 	u32 reg32;
 
-	/* Disable Root's interrupt in response to error messages */
-	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
-	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
-	pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
+	aer_disable_irq(pdev);
 
 	/* Clear Root's error status reg */
 	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, &reg32);
@@ -1372,12 +1388,8 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
 	 */
 	aer = root ? root->aer_cap : 0;
 
-	if ((host->native_aer || pcie_ports_native) && aer) {
-		/* Disable Root's interrupt in response to error messages */
-		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
-		reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
-		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
-	}
+	if ((host->native_aer || pcie_ports_native) && aer)
+		aer_disable_irq(root);
 
 	if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
 		rc = pcie_reset_flr(dev, PCI_RESET_DO_RESET);
@@ -1396,10 +1408,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
 		pci_read_config_dword(root, aer + PCI_ERR_ROOT_STATUS, &reg32);
 		pci_write_config_dword(root, aer + PCI_ERR_ROOT_STATUS, reg32);
 
-		/* Enable Root Port's interrupt in response to error messages */
-		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
-		reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
-		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
+		aer_enable_irq(root);
 	}
 
 	return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
-- 
2.34.1

[PATCH v6 1/3] PCI/AER: Factor out interrupt toggling into helpers

[Kai-Heng Feng]

There are many places that enable and disable AER interrupt, so move
them into helpers.

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v6:
 - No change.

v5:
 - Fix misspelling.

v4:
 - No change.

v3:
 - Correct subject.

v2:
 - New patch.

 drivers/pci/pcie/aer.c | 45 +++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index f6c24ded134c..1420e1f27105 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1227,6 +1227,28 @@ static irqreturn_t aer_irq(int irq, void *context)
 	return IRQ_WAKE_THREAD;
 }
 
+static void aer_enable_irq(struct pci_dev *pdev)
+{
+	int aer = pdev->aer_cap;
+	u32 reg32;
+
+	/* Enable Root Port's interrupt in response to error messages */
+	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
+	reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
+	pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
+}
+
+static void aer_disable_irq(struct pci_dev *pdev)
+{
+	int aer = pdev->aer_cap;
+	u32 reg32;
+
+	/* Disable Root's interrupt in response to error messages */
+	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
+	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
+	pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
+}
+
 /**
  * aer_enable_rootport - enable Root Port's interrupts when receiving messages
  * @rpc: pointer to a Root Port data structure
@@ -1256,10 +1278,7 @@ static void aer_enable_rootport(struct aer_rpc *rpc)
 	pci_read_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, &reg32);
 	pci_write_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, reg32);
 
-	/* Enable Root Port's interrupt in response to error messages */
-	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
-	reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
-	pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
+	aer_enable_irq(pdev);
 }
 
 /**
@@ -1274,10 +1293,7 @@ static void aer_disable_rootport(struct aer_rpc *rpc)
 	int aer = pdev->aer_cap;
 	u32 reg32;
 
-	/* Disable Root's interrupt in response to error messages */
-	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
-	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
-	pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
+	aer_disable_irq(pdev);
 
 	/* Clear Root's error status reg */
 	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, &reg32);
@@ -1372,12 +1388,8 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
 	 */
 	aer = root ? root->aer_cap : 0;
 
-	if ((host->native_aer || pcie_ports_native) && aer) {
-		/* Disable Root's interrupt in response to error messages */
-		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
-		reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
-		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
-	}
+	if ((host->native_aer || pcie_ports_native) && aer)
+		aer_disable_irq(root);
 
 	if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
 		rc = pcie_reset_flr(dev, PCI_RESET_DO_RESET);
@@ -1396,10 +1408,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
 		pci_read_config_dword(root, aer + PCI_ERR_ROOT_STATUS, &reg32);
 		pci_write_config_dword(root, aer + PCI_ERR_ROOT_STATUS, reg32);
 
-		/* Enable Root Port's interrupt in response to error messages */
-		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
-		reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
-		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
+		aer_enable_irq(root);
 	}
 
 	return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
-- 
2.34.1

[PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Kai-Heng Feng]

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.

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 <kai.heng.feng@canonical.com>
---
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

[PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Kai-Heng Feng]

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.

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 <kai.heng.feng@canonical.com>
---
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

[PATCH v6 3/3] PCI/DPC: Disable DPC interrupt during suspend

[Kai-Heng Feng]

PCIe services that share an IRQ with PME, such as AER or DPC, may cause a
spurious wakeup on system suspend. Since DPC depends on AER to work, disable
DPC interrupt notification during the system suspend process as AER interrupt
notification is already disabled by previous patch.

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 DPC
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 <kai.heng.feng@canonical.com>
---
v6:
v5:
 - Wording.

v4:
v3:
 - No change.

v2:
 - Only disable DPC IRQ.
 - No more check on PME IRQ#.

 drivers/pci/pcie/dpc.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 3ceed8e3de41..d2d845c20438 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -384,6 +384,30 @@ static int dpc_probe(struct pcie_device *dev)
 	return status;
 }
 
+static int dpc_suspend(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_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)
+{
+	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_INT_EN;
+	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
+
+	return 0;
+}
+
 static void dpc_remove(struct pcie_device *dev)
 {
 	struct pci_dev *pdev = dev->port;
@@ -399,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.34.1

[PATCH v6 3/3] PCI/DPC: Disable DPC interrupt during suspend

[Kai-Heng Feng]

PCIe services that share an IRQ with PME, such as AER or DPC, may cause a
spurious wakeup on system suspend. Since DPC depends on AER to work, disable
DPC interrupt notification during the system suspend process as AER interrupt
notification is already disabled by previous patch.

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 DPC
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 <kai.heng.feng@canonical.com>
---
v6:
v5:
 - Wording.

v4:
v3:
 - No change.

v2:
 - Only disable DPC IRQ.
 - No more check on PME IRQ#.

 drivers/pci/pcie/dpc.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 3ceed8e3de41..d2d845c20438 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -384,6 +384,30 @@ static int dpc_probe(struct pcie_device *dev)
 	return status;
 }
 
+static int dpc_suspend(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_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)
+{
+	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_INT_EN;
+	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
+
+	return 0;
+}
+
 static void dpc_remove(struct pcie_device *dev)
 {
 	struct pci_dev *pdev = dev->port;
@@ -399,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.34.1

Re: [PATCH v6 1/3] PCI/AER: Factor out interrupt toggling into helpers

[Kai-Heng Feng]

Hi Bjorn,

On Fri, May 12, 2023 at 8:01 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> There are many places that enable and disable AER interrupt, so move
> them into helpers.

Do you think the series is good to be be merged now?

Kai-Heng

>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v6:
>  - No change.
>
> v5:
>  - Fix misspelling.
>
> v4:
>  - No change.
>
> v3:
>  - Correct subject.
>
> v2:
>  - New patch.
>
>  drivers/pci/pcie/aer.c | 45 +++++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index f6c24ded134c..1420e1f27105 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -1227,6 +1227,28 @@ static irqreturn_t aer_irq(int irq, void *context)
>         return IRQ_WAKE_THREAD;
>  }
>
> +static void aer_enable_irq(struct pci_dev *pdev)
> +{
> +       int aer = pdev->aer_cap;
> +       u32 reg32;
> +
> +       /* Enable Root Port's interrupt in response to error messages */
> +       pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> +       reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> +       pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +}
> +
> +static void aer_disable_irq(struct pci_dev *pdev)
> +{
> +       int aer = pdev->aer_cap;
> +       u32 reg32;
> +
> +       /* Disable Root's interrupt in response to error messages */
> +       pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> +       reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> +       pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +}
> +
>  /**
>   * aer_enable_rootport - enable Root Port's interrupts when receiving messages
>   * @rpc: pointer to a Root Port data structure
> @@ -1256,10 +1278,7 @@ static void aer_enable_rootport(struct aer_rpc *rpc)
>         pci_read_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, &reg32);
>         pci_write_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, reg32);
>
> -       /* Enable Root Port's interrupt in response to error messages */
> -       pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -       reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> -       pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +       aer_enable_irq(pdev);
>  }
>
>  /**
> @@ -1274,10 +1293,7 @@ static void aer_disable_rootport(struct aer_rpc *rpc)
>         int aer = pdev->aer_cap;
>         u32 reg32;
>
> -       /* Disable Root's interrupt in response to error messages */
> -       pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -       reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> -       pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +       aer_disable_irq(pdev);
>
>         /* Clear Root's error status reg */
>         pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, &reg32);
> @@ -1372,12 +1388,8 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
>          */
>         aer = root ? root->aer_cap : 0;
>
> -       if ((host->native_aer || pcie_ports_native) && aer) {
> -               /* Disable Root's interrupt in response to error messages */
> -               pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -               reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> -               pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
> -       }
> +       if ((host->native_aer || pcie_ports_native) && aer)
> +               aer_disable_irq(root);
>
>         if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
>                 rc = pcie_reset_flr(dev, PCI_RESET_DO_RESET);
> @@ -1396,10 +1408,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
>                 pci_read_config_dword(root, aer + PCI_ERR_ROOT_STATUS, &reg32);
>                 pci_write_config_dword(root, aer + PCI_ERR_ROOT_STATUS, reg32);
>
> -               /* Enable Root Port's interrupt in response to error messages */
> -               pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -               reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> -               pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +               aer_enable_irq(root);
>         }
>
>         return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
> --
> 2.34.1
>

Re: [PATCH v6 1/3] PCI/AER: Factor out interrupt toggling into helpers

[Kai-Heng Feng]

Hi Bjorn,

On Fri, May 12, 2023 at 8:01 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> There are many places that enable and disable AER interrupt, so move
> them into helpers.

Do you think the series is good to be be merged now?

Kai-Heng

>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v6:
>  - No change.
>
> v5:
>  - Fix misspelling.
>
> v4:
>  - No change.
>
> v3:
>  - Correct subject.
>
> v2:
>  - New patch.
>
>  drivers/pci/pcie/aer.c | 45 +++++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index f6c24ded134c..1420e1f27105 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -1227,6 +1227,28 @@ static irqreturn_t aer_irq(int irq, void *context)
>         return IRQ_WAKE_THREAD;
>  }
>
> +static void aer_enable_irq(struct pci_dev *pdev)
> +{
> +       int aer = pdev->aer_cap;
> +       u32 reg32;
> +
> +       /* Enable Root Port's interrupt in response to error messages */
> +       pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> +       reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> +       pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +}
> +
> +static void aer_disable_irq(struct pci_dev *pdev)
> +{
> +       int aer = pdev->aer_cap;
> +       u32 reg32;
> +
> +       /* Disable Root's interrupt in response to error messages */
> +       pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> +       reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> +       pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +}
> +
>  /**
>   * aer_enable_rootport - enable Root Port's interrupts when receiving messages
>   * @rpc: pointer to a Root Port data structure
> @@ -1256,10 +1278,7 @@ static void aer_enable_rootport(struct aer_rpc *rpc)
>         pci_read_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, &reg32);
>         pci_write_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, reg32);
>
> -       /* Enable Root Port's interrupt in response to error messages */
> -       pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -       reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> -       pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +       aer_enable_irq(pdev);
>  }
>
>  /**
> @@ -1274,10 +1293,7 @@ static void aer_disable_rootport(struct aer_rpc *rpc)
>         int aer = pdev->aer_cap;
>         u32 reg32;
>
> -       /* Disable Root's interrupt in response to error messages */
> -       pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -       reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> -       pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +       aer_disable_irq(pdev);
>
>         /* Clear Root's error status reg */
>         pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, &reg32);
> @@ -1372,12 +1388,8 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
>          */
>         aer = root ? root->aer_cap : 0;
>
> -       if ((host->native_aer || pcie_ports_native) && aer) {
> -               /* Disable Root's interrupt in response to error messages */
> -               pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -               reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> -               pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
> -       }
> +       if ((host->native_aer || pcie_ports_native) && aer)
> +               aer_disable_irq(root);
>
>         if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
>                 rc = pcie_reset_flr(dev, PCI_RESET_DO_RESET);
> @@ -1396,10 +1408,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
>                 pci_read_config_dword(root, aer + PCI_ERR_ROOT_STATUS, &reg32);
>                 pci_write_config_dword(root, aer + PCI_ERR_ROOT_STATUS, reg32);
>
> -               /* Enable Root Port's interrupt in response to error messages */
> -               pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -               reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> -               pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +               aer_enable_irq(root);
>         }
>
>         return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
> --
> 2.34.1
>

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Kai-Heng Feng]

On Fri, May 12, 2023 at 8:01 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> 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.
>
> 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 <kai.heng.feng@canonical.com>

A gentle ping...

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

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Kai-Heng Feng]

On Fri, May 12, 2023 at 8:01 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> 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.
>
> 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 <kai.heng.feng@canonical.com>

A gentle ping...

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

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Bjorn Helgaas]

[+cc Rafael]

On Fri, May 12, 2023 at 08:00:13AM +0800, Kai-Heng Feng wrote:
> 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.

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.

> 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 <kai.heng.feng@canonical.com>
> ---
> 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
> 

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Bjorn Helgaas]

[+cc Rafael]

On Fri, May 12, 2023 at 08:00:13AM +0800, Kai-Heng Feng wrote:
> 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.

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.

> 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 <kai.heng.feng@canonical.com>
> ---
> 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
> 

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Kai-Heng Feng]

On Tue, Jul 18, 2023 at 7:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Rafael]
>
> On Fri, May 12, 2023 at 08:00:13AM +0800, Kai-Heng Feng wrote:
> > 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?

>
> 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.

Kai-Heng

>
> > 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 <kai.heng.feng@canonical.com>
> > ---
> > 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
> >

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Kai-Heng Feng]

On Tue, Jul 18, 2023 at 7:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Rafael]
>
> On Fri, May 12, 2023 at 08:00:13AM +0800, Kai-Heng Feng wrote:
> > 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?

>
> 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.

Kai-Heng

>
> > 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 <kai.heng.feng@canonical.com>
> > ---
> > 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
> >

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Kai-Heng Feng]

On Fri, Jul 21, 2023 at 11:58 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> On Tue, Jul 18, 2023 at 7:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > [+cc Rafael]
> >
> > On Fri, May 12, 2023 at 08:00:13AM +0800, Kai-Heng Feng wrote:
> > > 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?
>
> >
> > 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
>
> >
> > > 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 <kai.heng.feng@canonical.com>
> > > ---
> > > 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
> > >

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Kai-Heng Feng]

On Fri, Jul 21, 2023 at 11:58 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> On Tue, Jul 18, 2023 at 7:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > [+cc Rafael]
> >
> > On Fri, May 12, 2023 at 08:00:13AM +0800, Kai-Heng Feng wrote:
> > > 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?
>
> >
> > 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
>
> >
> > > 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 <kai.heng.feng@canonical.com>
> > > ---
> > > 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
> > >

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Bjorn Helgaas]

On Fri, Jul 21, 2023 at 11:58:24AM +0800, Kai-Heng Feng wrote:
> On Tue, Jul 18, 2023 at 7:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, May 12, 2023 at 08:00:13AM +0800, Kai-Heng Feng wrote:
> > > 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?

Rafael could answer this better than I can, but
Documentation/power/suspend-and-interrupts.rst says device interrupts
are generally disabled during suspend after the "late" phase of
suspending devices, i.e.,

  dpm_suspend_noirq
    suspend_device_irqs           <-- disable non-wakeup IRQs
    dpm_noirq_suspend_devices
      ...
        pci_pm_suspend_noirq      # (I assume)
	  pci_prepare_to_sleep

I think the downstream component would be put in D3cold by
pci_prepare_to_sleep(), so non-wakeup interrupts should be disabled by
then.

I assume PME would generally *not* be disabled since it's needed for
wakeup, so I think any interrupt that shares the PME IRQ and occurs
during suspend may cause a spurious wakeup.

If so, it's exactly as you said at the beginning: AER/DPC/etc sharing
the PME IRQ may cause spurious wakeups, and we would have to disable
those other interrupts at the source, e.g., by clearing
PCI_ERR_ROOT_CMD_FATAL_EN etc (exactly as your series does).

> > 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.
> 
> Kai-Heng
> 
> >
> > > 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 <kai.heng.feng@canonical.com>
> > > ---
> > > 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
> > >

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Bjorn Helgaas]

On Fri, Jul 21, 2023 at 11:58:24AM +0800, Kai-Heng Feng wrote:
> On Tue, Jul 18, 2023 at 7:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, May 12, 2023 at 08:00:13AM +0800, Kai-Heng Feng wrote:
> > > 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?

Rafael could answer this better than I can, but
Documentation/power/suspend-and-interrupts.rst says device interrupts
are generally disabled during suspend after the "late" phase of
suspending devices, i.e.,

  dpm_suspend_noirq
    suspend_device_irqs           <-- disable non-wakeup IRQs
    dpm_noirq_suspend_devices
      ...
        pci_pm_suspend_noirq      # (I assume)
	  pci_prepare_to_sleep

I think the downstream component would be put in D3cold by
pci_prepare_to_sleep(), so non-wakeup interrupts should be disabled by
then.

I assume PME would generally *not* be disabled since it's needed for
wakeup, so I think any interrupt that shares the PME IRQ and occurs
during suspend may cause a spurious wakeup.

If so, it's exactly as you said at the beginning: AER/DPC/etc sharing
the PME IRQ may cause spurious wakeups, and we would have to disable
those other interrupts at the source, e.g., by clearing
PCI_ERR_ROOT_CMD_FATAL_EN etc (exactly as your series does).

> > 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.
> 
> Kai-Heng
> 
> >
> > > 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 <kai.heng.feng@canonical.com>
> > > ---
> > > 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
> > >

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Kai-Heng Feng]

On Thu, Aug 10, 2023 at 2:52 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jul 21, 2023 at 11:58:24AM +0800, Kai-Heng Feng wrote:
> > On Tue, Jul 18, 2023 at 7:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Fri, May 12, 2023 at 08:00:13AM +0800, Kai-Heng Feng wrote:
> > > > 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?
>
> Rafael could answer this better than I can, but
> Documentation/power/suspend-and-interrupts.rst says device interrupts
> are generally disabled during suspend after the "late" phase of
> suspending devices, i.e.,
>
>   dpm_suspend_noirq
>     suspend_device_irqs           <-- disable non-wakeup IRQs
>     dpm_noirq_suspend_devices
>       ...
>         pci_pm_suspend_noirq      # (I assume)
>           pci_prepare_to_sleep
>
> I think the downstream component would be put in D3cold by
> pci_prepare_to_sleep(), so non-wakeup interrupts should be disabled by
> then.
>
> I assume PME would generally *not* be disabled since it's needed for
> wakeup, so I think any interrupt that shares the PME IRQ and occurs
> during suspend may cause a spurious wakeup.

Yes, that's the case here.

>
> If so, it's exactly as you said at the beginning: AER/DPC/etc sharing
> the PME IRQ may cause spurious wakeups, and we would have to disable
> those other interrupts at the source, e.g., by clearing
> PCI_ERR_ROOT_CMD_FATAL_EN etc (exactly as your series does).

So is the series good to be merged now?

Kai-Heng

>
> > > 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.
> >
> > Kai-Heng
> >
> > >
> > > > 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 <kai.heng.feng@canonical.com>
> > > > ---
> > > > 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
> > > >

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Kai-Heng Feng]

On Thu, Aug 10, 2023 at 2:52 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jul 21, 2023 at 11:58:24AM +0800, Kai-Heng Feng wrote:
> > On Tue, Jul 18, 2023 at 7:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Fri, May 12, 2023 at 08:00:13AM +0800, Kai-Heng Feng wrote:
> > > > 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?
>
> Rafael could answer this better than I can, but
> Documentation/power/suspend-and-interrupts.rst says device interrupts
> are generally disabled during suspend after the "late" phase of
> suspending devices, i.e.,
>
>   dpm_suspend_noirq
>     suspend_device_irqs           <-- disable non-wakeup IRQs
>     dpm_noirq_suspend_devices
>       ...
>         pci_pm_suspend_noirq      # (I assume)
>           pci_prepare_to_sleep
>
> I think the downstream component would be put in D3cold by
> pci_prepare_to_sleep(), so non-wakeup interrupts should be disabled by
> then.
>
> I assume PME would generally *not* be disabled since it's needed for
> wakeup, so I think any interrupt that shares the PME IRQ and occurs
> during suspend may cause a spurious wakeup.

Yes, that's the case here.

>
> If so, it's exactly as you said at the beginning: AER/DPC/etc sharing
> the PME IRQ may cause spurious wakeups, and we would have to disable
> those other interrupts at the source, e.g., by clearing
> PCI_ERR_ROOT_CMD_FATAL_EN etc (exactly as your series does).

So is the series good to be merged now?

Kai-Heng

>
> > > 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.
> >
> > Kai-Heng
> >
> > >
> > > > 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 <kai.heng.feng@canonical.com>
> > > > ---
> > > > 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
> > > >

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Bjorn Helgaas]

On Thu, Aug 10, 2023 at 04:17:21PM +0800, Kai-Heng Feng wrote:
> On Thu, Aug 10, 2023 at 2:52 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Jul 21, 2023 at 11:58:24AM +0800, Kai-Heng Feng wrote:
> > > On Tue, Jul 18, 2023 at 7:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Fri, May 12, 2023 at 08:00:13AM +0800, Kai-Heng Feng wrote:
> > > > > 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?
> >
> > Rafael could answer this better than I can, but
> > Documentation/power/suspend-and-interrupts.rst says device interrupts
> > are generally disabled during suspend after the "late" phase of
> > suspending devices, i.e.,
> >
> >   dpm_suspend_noirq
> >     suspend_device_irqs           <-- disable non-wakeup IRQs
> >     dpm_noirq_suspend_devices
> >       ...
> >         pci_pm_suspend_noirq      # (I assume)
> >           pci_prepare_to_sleep
> >
> > I think the downstream component would be put in D3cold by
> > pci_prepare_to_sleep(), so non-wakeup interrupts should be disabled by
> > then.
> >
> > I assume PME would generally *not* be disabled since it's needed for
> > wakeup, so I think any interrupt that shares the PME IRQ and occurs
> > during suspend may cause a spurious wakeup.
> 
> Yes, that's the case here.
> 
> > If so, it's exactly as you said at the beginning: AER/DPC/etc sharing
> > the PME IRQ may cause spurious wakeups, and we would have to disable
> > those other interrupts at the source, e.g., by clearing
> > PCI_ERR_ROOT_CMD_FATAL_EN etc (exactly as your series does).
> 
> So is the series good to be merged now?

If we merge as-is, won't we disable AER & DPC interrupts unnecessarily
in the case where the link goes to D3hot?  In that case, there's no
reason to expect interrupts related to the link going down, but things
like PTM messages still work, and they may cause errors that we should
know about.

> > > > 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.
> > >
> > > Kai-Heng
> > >
> > > >
> > > > > 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 <kai.heng.feng@canonical.com>
> > > > > ---
> > > > > 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
> > > > >

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Bjorn Helgaas]

On Thu, Aug 10, 2023 at 04:17:21PM +0800, Kai-Heng Feng wrote:
> On Thu, Aug 10, 2023 at 2:52 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Jul 21, 2023 at 11:58:24AM +0800, Kai-Heng Feng wrote:
> > > On Tue, Jul 18, 2023 at 7:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Fri, May 12, 2023 at 08:00:13AM +0800, Kai-Heng Feng wrote:
> > > > > 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?
> >
> > Rafael could answer this better than I can, but
> > Documentation/power/suspend-and-interrupts.rst says device interrupts
> > are generally disabled during suspend after the "late" phase of
> > suspending devices, i.e.,
> >
> >   dpm_suspend_noirq
> >     suspend_device_irqs           <-- disable non-wakeup IRQs
> >     dpm_noirq_suspend_devices
> >       ...
> >         pci_pm_suspend_noirq      # (I assume)
> >           pci_prepare_to_sleep
> >
> > I think the downstream component would be put in D3cold by
> > pci_prepare_to_sleep(), so non-wakeup interrupts should be disabled by
> > then.
> >
> > I assume PME would generally *not* be disabled since it's needed for
> > wakeup, so I think any interrupt that shares the PME IRQ and occurs
> > during suspend may cause a spurious wakeup.
> 
> Yes, that's the case here.
> 
> > If so, it's exactly as you said at the beginning: AER/DPC/etc sharing
> > the PME IRQ may cause spurious wakeups, and we would have to disable
> > those other interrupts at the source, e.g., by clearing
> > PCI_ERR_ROOT_CMD_FATAL_EN etc (exactly as your series does).
> 
> So is the series good to be merged now?

If we merge as-is, won't we disable AER & DPC interrupts unnecessarily
in the case where the link goes to D3hot?  In that case, there's no
reason to expect interrupts related to the link going down, but things
like PTM messages still work, and they may cause errors that we should
know about.

> > > > 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.
> > >
> > > Kai-Heng
> > >
> > > >
> > > > > 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 <kai.heng.feng@canonical.com>
> > > > > ---
> > > > > 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
> > > > >

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Kai-Heng Feng]

On Thu, Aug 10, 2023 at 6:51 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Aug 10, 2023 at 04:17:21PM +0800, Kai-Heng Feng wrote:
> > On Thu, Aug 10, 2023 at 2:52 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Fri, Jul 21, 2023 at 11:58:24AM +0800, Kai-Heng Feng wrote:
> > > > On Tue, Jul 18, 2023 at 7:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > On Fri, May 12, 2023 at 08:00:13AM +0800, Kai-Heng Feng wrote:
> > > > > > 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?
> > >
> > > Rafael could answer this better than I can, but
> > > Documentation/power/suspend-and-interrupts.rst says device interrupts
> > > are generally disabled during suspend after the "late" phase of
> > > suspending devices, i.e.,
> > >
> > >   dpm_suspend_noirq
> > >     suspend_device_irqs           <-- disable non-wakeup IRQs
> > >     dpm_noirq_suspend_devices
> > >       ...
> > >         pci_pm_suspend_noirq      # (I assume)
> > >           pci_prepare_to_sleep
> > >
> > > I think the downstream component would be put in D3cold by
> > > pci_prepare_to_sleep(), so non-wakeup interrupts should be disabled by
> > > then.
> > >
> > > I assume PME would generally *not* be disabled since it's needed for
> > > wakeup, so I think any interrupt that shares the PME IRQ and occurs
> > > during suspend may cause a spurious wakeup.
> >
> > Yes, that's the case here.
> >
> > > If so, it's exactly as you said at the beginning: AER/DPC/etc sharing
> > > the PME IRQ may cause spurious wakeups, and we would have to disable
> > > those other interrupts at the source, e.g., by clearing
> > > PCI_ERR_ROOT_CMD_FATAL_EN etc (exactly as your series does).
> >
> > So is the series good to be merged now?
>
> If we merge as-is, won't we disable AER & DPC interrupts unnecessarily
> in the case where the link goes to D3hot?  In that case, there's no
> reason to expect interrupts related to the link going down, but things
> like PTM messages still work, and they may cause errors that we should
> know about.

Because the issue can be observed on D3hot as well [0].
The root port device [0] is power managed by ACPI, so I wonder if it's
reasonable to disable AER & DPC for devices that power managed by
firmware?

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

Kai-Heng

>
> > > > > 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.
> > > >
> > > > Kai-Heng
> > > >
> > > > >
> > > > > > 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 <kai.heng.feng@canonical.com>
> > > > > > ---
> > > > > > 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
> > > > > >

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Kai-Heng Feng]

On Thu, Aug 10, 2023 at 6:51 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Aug 10, 2023 at 04:17:21PM +0800, Kai-Heng Feng wrote:
> > On Thu, Aug 10, 2023 at 2:52 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Fri, Jul 21, 2023 at 11:58:24AM +0800, Kai-Heng Feng wrote:
> > > > On Tue, Jul 18, 2023 at 7:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > On Fri, May 12, 2023 at 08:00:13AM +0800, Kai-Heng Feng wrote:
> > > > > > 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?
> > >
> > > Rafael could answer this better than I can, but
> > > Documentation/power/suspend-and-interrupts.rst says device interrupts
> > > are generally disabled during suspend after the "late" phase of
> > > suspending devices, i.e.,
> > >
> > >   dpm_suspend_noirq
> > >     suspend_device_irqs           <-- disable non-wakeup IRQs
> > >     dpm_noirq_suspend_devices
> > >       ...
> > >         pci_pm_suspend_noirq      # (I assume)
> > >           pci_prepare_to_sleep
> > >
> > > I think the downstream component would be put in D3cold by
> > > pci_prepare_to_sleep(), so non-wakeup interrupts should be disabled by
> > > then.
> > >
> > > I assume PME would generally *not* be disabled since it's needed for
> > > wakeup, so I think any interrupt that shares the PME IRQ and occurs
> > > during suspend may cause a spurious wakeup.
> >
> > Yes, that's the case here.
> >
> > > If so, it's exactly as you said at the beginning: AER/DPC/etc sharing
> > > the PME IRQ may cause spurious wakeups, and we would have to disable
> > > those other interrupts at the source, e.g., by clearing
> > > PCI_ERR_ROOT_CMD_FATAL_EN etc (exactly as your series does).
> >
> > So is the series good to be merged now?
>
> If we merge as-is, won't we disable AER & DPC interrupts unnecessarily
> in the case where the link goes to D3hot?  In that case, there's no
> reason to expect interrupts related to the link going down, but things
> like PTM messages still work, and they may cause errors that we should
> know about.

Because the issue can be observed on D3hot as well [0].
The root port device [0] is power managed by ACPI, so I wonder if it's
reasonable to disable AER & DPC for devices that power managed by
firmware?

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

Kai-Heng

>
> > > > > 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.
> > > >
> > > > Kai-Heng
> > > >
> > > > >
> > > > > > 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 <kai.heng.feng@canonical.com>
> > > > > > ---
> > > > > > 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
> > > > > >

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Kai-Heng Feng]

On Fri, Aug 11, 2023 at 4:00 PM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> On Thu, Aug 10, 2023 at 6:51 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Thu, Aug 10, 2023 at 04:17:21PM +0800, Kai-Heng Feng wrote:
> > > On Thu, Aug 10, 2023 at 2:52 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Fri, Jul 21, 2023 at 11:58:24AM +0800, Kai-Heng Feng wrote:
> > > > > On Tue, Jul 18, 2023 at 7:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > On Fri, May 12, 2023 at 08:00:13AM +0800, Kai-Heng Feng wrote:
> > > > > > > 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?
> > > >
> > > > Rafael could answer this better than I can, but
> > > > Documentation/power/suspend-and-interrupts.rst says device interrupts
> > > > are generally disabled during suspend after the "late" phase of
> > > > suspending devices, i.e.,
> > > >
> > > >   dpm_suspend_noirq
> > > >     suspend_device_irqs           <-- disable non-wakeup IRQs
> > > >     dpm_noirq_suspend_devices
> > > >       ...
> > > >         pci_pm_suspend_noirq      # (I assume)
> > > >           pci_prepare_to_sleep
> > > >
> > > > I think the downstream component would be put in D3cold by
> > > > pci_prepare_to_sleep(), so non-wakeup interrupts should be disabled by
> > > > then.
> > > >
> > > > I assume PME would generally *not* be disabled since it's needed for
> > > > wakeup, so I think any interrupt that shares the PME IRQ and occurs
> > > > during suspend may cause a spurious wakeup.
> > >
> > > Yes, that's the case here.
> > >
> > > > If so, it's exactly as you said at the beginning: AER/DPC/etc sharing
> > > > the PME IRQ may cause spurious wakeups, and we would have to disable
> > > > those other interrupts at the source, e.g., by clearing
> > > > PCI_ERR_ROOT_CMD_FATAL_EN etc (exactly as your series does).
> > >
> > > So is the series good to be merged now?
> >
> > If we merge as-is, won't we disable AER & DPC interrupts unnecessarily
> > in the case where the link goes to D3hot?  In that case, there's no
> > reason to expect interrupts related to the link going down, but things
> > like PTM messages still work, and they may cause errors that we should
> > know about.
>
> Because the issue can be observed on D3hot as well [0].
> The root port device [0] is power managed by ACPI, so I wonder if it's
> reasonable to disable AER & DPC for devices that power managed by
> firmware?

OK, I think the D3hot case is different to this one, so I'll work on
next revision that only disable AER/DPC when power is really off.

In additional to disabling interrupt, is it reasonable to disable AER
and DPC service completely, so unwanted electric noise wont trigger a
DPC reset?

Kai-Heng

> [0] https://bugzilla.kernel.org/show_bug.cgi?id=216295#c3
>
> Kai-Heng
>
> >
> > > > > > 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.
> > > > >
> > > > > Kai-Heng
> > > > >
> > > > > >
> > > > > > > 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 <kai.heng.feng@canonical.com>
> > > > > > > ---
> > > > > > > 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
> > > > > > >

Re: [PATCH v6 2/3] PCI/AER: Disable AER interrupt on suspend

[Kai-Heng Feng]

On Fri, Aug 11, 2023 at 4:00 PM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> On Thu, Aug 10, 2023 at 6:51 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Thu, Aug 10, 2023 at 04:17:21PM +0800, Kai-Heng Feng wrote:
> > > On Thu, Aug 10, 2023 at 2:52 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Fri, Jul 21, 2023 at 11:58:24AM +0800, Kai-Heng Feng wrote:
> > > > > On Tue, Jul 18, 2023 at 7:17 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > On Fri, May 12, 2023 at 08:00:13AM +0800, Kai-Heng Feng wrote:
> > > > > > > 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?
> > > >
> > > > Rafael could answer this better than I can, but
> > > > Documentation/power/suspend-and-interrupts.rst says device interrupts
> > > > are generally disabled during suspend after the "late" phase of
> > > > suspending devices, i.e.,
> > > >
> > > >   dpm_suspend_noirq
> > > >     suspend_device_irqs           <-- disable non-wakeup IRQs
> > > >     dpm_noirq_suspend_devices
> > > >       ...
> > > >         pci_pm_suspend_noirq      # (I assume)
> > > >           pci_prepare_to_sleep
> > > >
> > > > I think the downstream component would be put in D3cold by
> > > > pci_prepare_to_sleep(), so non-wakeup interrupts should be disabled by
> > > > then.
> > > >
> > > > I assume PME would generally *not* be disabled since it's needed for
> > > > wakeup, so I think any interrupt that shares the PME IRQ and occurs
> > > > during suspend may cause a spurious wakeup.
> > >
> > > Yes, that's the case here.
> > >
> > > > If so, it's exactly as you said at the beginning: AER/DPC/etc sharing
> > > > the PME IRQ may cause spurious wakeups, and we would have to disable
> > > > those other interrupts at the source, e.g., by clearing
> > > > PCI_ERR_ROOT_CMD_FATAL_EN etc (exactly as your series does).
> > >
> > > So is the series good to be merged now?
> >
> > If we merge as-is, won't we disable AER & DPC interrupts unnecessarily
> > in the case where the link goes to D3hot?  In that case, there's no
> > reason to expect interrupts related to the link going down, but things
> > like PTM messages still work, and they may cause errors that we should
> > know about.
>
> Because the issue can be observed on D3hot as well [0].
> The root port device [0] is power managed by ACPI, so I wonder if it's
> reasonable to disable AER & DPC for devices that power managed by
> firmware?

OK, I think the D3hot case is different to this one, so I'll work on
next revision that only disable AER/DPC when power is really off.

In additional to disabling interrupt, is it reasonable to disable AER
and DPC service completely, so unwanted electric noise wont trigger a
DPC reset?

Kai-Heng

> [0] https://bugzilla.kernel.org/show_bug.cgi?id=216295#c3
>
> Kai-Heng
>
> >
> > > > > > 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.
> > > > >
> > > > > Kai-Heng
> > > > >
> > > > > >
> > > > > > > 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 <kai.heng.feng@canonical.com>
> > > > > > > ---
> > > > > > > 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
> > > > > > >

Re: [PATCH v6 1/3] PCI/AER: Factor out interrupt toggling into helpers

[Bjorn Helgaas]

On Fri, May 12, 2023 at 08:00:12AM +0800, Kai-Heng Feng wrote:
> There are many places that enable and disable AER interrupt, so move
> them into helpers.
> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

I applied this patch (only 1/3) to pci/aer for v6.7.

I'm not clear on the others yet, so let's look at those again after
v6.7-rc1.  It seemed like there's still a question about disabling
interrupts when we're going to D3hot.

>  drivers/pci/pcie/aer.c | 45 +++++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index f6c24ded134c..1420e1f27105 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -1227,6 +1227,28 @@ static irqreturn_t aer_irq(int irq, void *context)
>  	return IRQ_WAKE_THREAD;
>  }
>  
> +static void aer_enable_irq(struct pci_dev *pdev)
> +{
> +	int aer = pdev->aer_cap;
> +	u32 reg32;
> +
> +	/* Enable Root Port's interrupt in response to error messages */
> +	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> +	reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> +	pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +}
> +
> +static void aer_disable_irq(struct pci_dev *pdev)
> +{
> +	int aer = pdev->aer_cap;
> +	u32 reg32;
> +
> +	/* Disable Root's interrupt in response to error messages */
> +	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> +	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> +	pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +}
> +
>  /**
>   * aer_enable_rootport - enable Root Port's interrupts when receiving messages
>   * @rpc: pointer to a Root Port data structure
> @@ -1256,10 +1278,7 @@ static void aer_enable_rootport(struct aer_rpc *rpc)
>  	pci_read_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, &reg32);
>  	pci_write_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, reg32);
>  
> -	/* Enable Root Port's interrupt in response to error messages */
> -	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -	reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> -	pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +	aer_enable_irq(pdev);
>  }
>  
>  /**
> @@ -1274,10 +1293,7 @@ static void aer_disable_rootport(struct aer_rpc *rpc)
>  	int aer = pdev->aer_cap;
>  	u32 reg32;
>  
> -	/* Disable Root's interrupt in response to error messages */
> -	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> -	pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +	aer_disable_irq(pdev);
>  
>  	/* Clear Root's error status reg */
>  	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, &reg32);
> @@ -1372,12 +1388,8 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
>  	 */
>  	aer = root ? root->aer_cap : 0;
>  
> -	if ((host->native_aer || pcie_ports_native) && aer) {
> -		/* Disable Root's interrupt in response to error messages */
> -		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -		reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> -		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
> -	}
> +	if ((host->native_aer || pcie_ports_native) && aer)
> +		aer_disable_irq(root);
>  
>  	if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
>  		rc = pcie_reset_flr(dev, PCI_RESET_DO_RESET);
> @@ -1396,10 +1408,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
>  		pci_read_config_dword(root, aer + PCI_ERR_ROOT_STATUS, &reg32);
>  		pci_write_config_dword(root, aer + PCI_ERR_ROOT_STATUS, reg32);
>  
> -		/* Enable Root Port's interrupt in response to error messages */
> -		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -		reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> -		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +		aer_enable_irq(root);
>  	}
>  
>  	return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
> -- 
> 2.34.1
> 

Re: [PATCH v6 1/3] PCI/AER: Factor out interrupt toggling into helpers

[Bjorn Helgaas]

On Fri, May 12, 2023 at 08:00:12AM +0800, Kai-Heng Feng wrote:
> There are many places that enable and disable AER interrupt, so move
> them into helpers.
> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

I applied this patch (only 1/3) to pci/aer for v6.7.

I'm not clear on the others yet, so let's look at those again after
v6.7-rc1.  It seemed like there's still a question about disabling
interrupts when we're going to D3hot.

>  drivers/pci/pcie/aer.c | 45 +++++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index f6c24ded134c..1420e1f27105 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -1227,6 +1227,28 @@ static irqreturn_t aer_irq(int irq, void *context)
>  	return IRQ_WAKE_THREAD;
>  }
>  
> +static void aer_enable_irq(struct pci_dev *pdev)
> +{
> +	int aer = pdev->aer_cap;
> +	u32 reg32;
> +
> +	/* Enable Root Port's interrupt in response to error messages */
> +	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> +	reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> +	pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +}
> +
> +static void aer_disable_irq(struct pci_dev *pdev)
> +{
> +	int aer = pdev->aer_cap;
> +	u32 reg32;
> +
> +	/* Disable Root's interrupt in response to error messages */
> +	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> +	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> +	pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +}
> +
>  /**
>   * aer_enable_rootport - enable Root Port's interrupts when receiving messages
>   * @rpc: pointer to a Root Port data structure
> @@ -1256,10 +1278,7 @@ static void aer_enable_rootport(struct aer_rpc *rpc)
>  	pci_read_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, &reg32);
>  	pci_write_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, reg32);
>  
> -	/* Enable Root Port's interrupt in response to error messages */
> -	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -	reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> -	pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +	aer_enable_irq(pdev);
>  }
>  
>  /**
> @@ -1274,10 +1293,7 @@ static void aer_disable_rootport(struct aer_rpc *rpc)
>  	int aer = pdev->aer_cap;
>  	u32 reg32;
>  
> -	/* Disable Root's interrupt in response to error messages */
> -	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> -	pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +	aer_disable_irq(pdev);
>  
>  	/* Clear Root's error status reg */
>  	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, &reg32);
> @@ -1372,12 +1388,8 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
>  	 */
>  	aer = root ? root->aer_cap : 0;
>  
> -	if ((host->native_aer || pcie_ports_native) && aer) {
> -		/* Disable Root's interrupt in response to error messages */
> -		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -		reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> -		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
> -	}
> +	if ((host->native_aer || pcie_ports_native) && aer)
> +		aer_disable_irq(root);
>  
>  	if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) {
>  		rc = pcie_reset_flr(dev, PCI_RESET_DO_RESET);
> @@ -1396,10 +1408,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
>  		pci_read_config_dword(root, aer + PCI_ERR_ROOT_STATUS, &reg32);
>  		pci_write_config_dword(root, aer + PCI_ERR_ROOT_STATUS, reg32);
>  
> -		/* Enable Root Port's interrupt in response to error messages */
> -		pci_read_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -		reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> -		pci_write_config_dword(root, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +		aer_enable_irq(root);
>  	}
>  
>  	return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
> -- 
> 2.34.1
>