When AER is using the same IRQ as PME, AER interrupt is treated as a
wakeup event and it can disrupt system suspend process.
If that happens, the system will report it's woken up by PME IRQ without
indicating any AER error since AER status is cleared on resume.
So keep the AER status so users can know the system is woken up by AER
instead of PME.
Reviewed-by: Mika Westerberg <[email protected]>
Signed-off-by: Kai-Heng Feng <[email protected]>
---
v3:
- No change.
v2:
- New patch.
drivers/pci/pci.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7a67611dc5f4..71aead00fc20 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1778,7 +1778,6 @@ void pci_restore_state(struct pci_dev *dev)
pci_restore_dpc_state(dev);
pci_restore_ptm_state(dev);
- pci_aer_clear_status(dev);
pci_restore_aer_state(dev);
pci_restore_config_space(dev);
--
2.34.1
There are many places that enable and disable AER interrput, so move
them into helpers.
Signed-off-by: Kai-Heng Feng <[email protected]>
---
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, ®32);
+ 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, ®32);
+ 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, ®32);
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, ®32);
- 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, ®32);
- 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, ®32);
@@ -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, ®32);
- 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, ®32);
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, ®32);
- 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
PCIe service that shares IRQ with PME may cause spurious wakeup on
system suspend.
Since AER is conditionally disabled in previous patch, also apply the
same logic to disable DPC which depends on AER to work.
PCIe Base Spec 5.0, section 5.2 "Link State Power Management" states
that TLP and DLLP transmission is disabled for a Link in L2/L3 Ready
(D3hot), L2 (D3cold with aux power) and L3 (D3cold), so we don't lose
much here to disable DPC during system suspend.
This is very similar to previous attempts to suspend AER and DPC [1],
but with a different reason.
[1] https://lore.kernel.org/linux-pci/[email protected]/
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216295
Reviewed-by: Mika Westerberg <[email protected]>
Signed-off-by: Kai-Heng Feng <[email protected]>
---
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 a5d7c69b764e..98bdefde6df1 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -385,6 +385,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;
@@ -400,6 +424,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
PCIe service that shares IRQ with PME may cause spurious wakeup on
system suspend.
PCIe Base Spec 5.0, section 5.2 "Link State Power Management" states
that TLP and DLLP transmission is disabled for a Link in L2/L3 Ready
(D3hot), L2 (D3cold with aux power) and L3 (D3cold), so we don't lose
much here to disable AER during system suspend.
This is very similar to previous attempts to suspend AER and DPC [1],
but with a different reason.
[1] https://lore.kernel.org/linux-pci/[email protected]/
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216295
Reviewed-by: Mika Westerberg <[email protected]>
Signed-off-by: Kai-Heng Feng <[email protected]>
---
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
On Thu, Apr 20, 2023 at 08:59:38PM +0800, Kai-Heng Feng wrote:
> There are many places that enable and disable AER interrput, so move
> them into helpers.
>
> Signed-off-by: Kai-Heng Feng <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Hi Kai,
On 4/20/23 5:59 AM, Kai-Heng Feng wrote:
> When AER is using the same IRQ as PME, AER interrupt is treated as a
> wakeup event and it can disrupt system suspend process.
>
> If that happens, the system will report it's woken up by PME IRQ without
> indicating any AER error since AER status is cleared on resume.
>
> So keep the AER status so users can know the system is woken up by AER
> instead of PME.
>
> Reviewed-by: Mika Westerberg <[email protected]>
> Signed-off-by: Kai-Heng Feng <[email protected]>
> ---
Any history on why it is cleared before? Is it done to hide some resume
issues?
> v3:
> - No change.
>
> v2:
> - New patch.
>
> drivers/pci/pci.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7a67611dc5f4..71aead00fc20 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1778,7 +1778,6 @@ void pci_restore_state(struct pci_dev *dev)
> pci_restore_dpc_state(dev);
> pci_restore_ptm_state(dev);
>
> - pci_aer_clear_status(dev);
> pci_restore_aer_state(dev);
>
> pci_restore_config_space(dev);
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On 4/20/23 5:59 AM, Kai-Heng Feng wrote:
> There are many places that enable and disable AER interrput, so move
> them into helpers.
Add "No functional changes intended"
>
> Signed-off-by: Kai-Heng Feng <[email protected]>
> ---
Looks good to me.
Reviewed-by: Kuppuswamy Sathyanarayanan <[email protected]>
> 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, ®32);
> + 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, ®32);
> + 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, ®32);
> 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, ®32);
> - 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, ®32);
> - 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, ®32);
> @@ -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, ®32);
> - 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, ®32);
> 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, ®32);
> - 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;
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On 4/20/23 5:59 AM, Kai-Heng Feng wrote:
> PCIe service that shares IRQ with PME may cause spurious wakeup on
> system suspend.
>
> PCIe Base Spec 5.0, section 5.2 "Link State Power Management" states
> that TLP and DLLP transmission is disabled for a Link in L2/L3 Ready
> (D3hot), L2 (D3cold with aux power) and L3 (D3cold), so we don't lose
> much here to disable AER during system suspend.
>
> This is very similar to previous attempts to suspend AER and DPC [1],
> but with a different reason.
>
> [1] https://lore.kernel.org/linux-pci/[email protected]/
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216295
>
> Reviewed-by: Mika Westerberg <[email protected]>
> Signed-off-by: Kai-Heng Feng <[email protected]>
> ---
In Patch #1, you skip clearing AER errors in the resume path, right? So if we disable
interrupts here, will AER driver not be notified on resume path error?
> 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,
> };
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
Hi Sathyanarayanan,
On Thu, Apr 20, 2023 at 10:39 PM Sathyanarayanan Kuppuswamy
<[email protected]> wrote:
>
> Hi Kai,
It's Kai-Heng :)
>
> On 4/20/23 5:59 AM, Kai-Heng Feng wrote:
> > When AER is using the same IRQ as PME, AER interrupt is treated as a
> > wakeup event and it can disrupt system suspend process.
> >
> > If that happens, the system will report it's woken up by PME IRQ without
> > indicating any AER error since AER status is cleared on resume.
> >
> > So keep the AER status so users can know the system is woken up by AER
> > instead of PME.
> >
> > Reviewed-by: Mika Westerberg <[email protected]>
> > Signed-off-by: Kai-Heng Feng <[email protected]>
> > ---
>
> Any history on why it is cleared before? Is it done to hide some resume
> issues?
It was introduced by commit b07461a8e45b ("PCI/AER: Clear error status
registers during enumeration and restore").
The justification is quite reasonable so I think maybe we should keep it as is.
Kai-Heng
>
> > v3:
> > - No change.
> >
> > v2:
> > - New patch.
> >
> > drivers/pci/pci.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 7a67611dc5f4..71aead00fc20 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1778,7 +1778,6 @@ void pci_restore_state(struct pci_dev *dev)
> > pci_restore_dpc_state(dev);
> > pci_restore_ptm_state(dev);
> >
> > - pci_aer_clear_status(dev);
> > pci_restore_aer_state(dev);
> >
> > pci_restore_config_space(dev);
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
On 4/20/23 6:35 PM, Kai-Heng Feng wrote:
> Hi Sathyanarayanan,
>
> On Thu, Apr 20, 2023 at 10:39 PM Sathyanarayanan Kuppuswamy
> <[email protected]> wrote:
>>
>> Hi Kai,
>
> It's Kai-Heng :)
>
>>
>> On 4/20/23 5:59 AM, Kai-Heng Feng wrote:
>>> When AER is using the same IRQ as PME, AER interrupt is treated as a
>>> wakeup event and it can disrupt system suspend process.
>>>
>>> If that happens, the system will report it's woken up by PME IRQ without
>>> indicating any AER error since AER status is cleared on resume.
>>>
>>> So keep the AER status so users can know the system is woken up by AER
>>> instead of PME.
>>>
>>> Reviewed-by: Mika Westerberg <[email protected]>
>>> Signed-off-by: Kai-Heng Feng <[email protected]>
>>> ---
>>
>> Any history on why it is cleared before? Is it done to hide some resume
>> issues?
>
> It was introduced by commit b07461a8e45b ("PCI/AER: Clear error status
> registers during enumeration and restore").
> The justification is quite reasonable so I think maybe we should keep it as is.
Yes. It looks like it is better to leave it as it is.
>
> Kai-Heng
>
>>
>>> v3:
>>> - No change.
>>>
>>> v2:
>>> - New patch.
>>>
>>> drivers/pci/pci.c | 1 -
>>> 1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index 7a67611dc5f4..71aead00fc20 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -1778,7 +1778,6 @@ void pci_restore_state(struct pci_dev *dev)
>>> pci_restore_dpc_state(dev);
>>> pci_restore_ptm_state(dev);
>>>
>>> - pci_aer_clear_status(dev);
>>> pci_restore_aer_state(dev);
>>>
>>> pci_restore_config_space(dev);
>>
>> --
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Thu, Apr 20, 2023 at 10:53 PM Sathyanarayanan Kuppuswamy
<[email protected]> wrote:
>
>
>
> On 4/20/23 5:59 AM, Kai-Heng Feng wrote:
> > PCIe service that shares IRQ with PME may cause spurious wakeup on
> > system suspend.
> >
> > PCIe Base Spec 5.0, section 5.2 "Link State Power Management" states
> > that TLP and DLLP transmission is disabled for a Link in L2/L3 Ready
> > (D3hot), L2 (D3cold with aux power) and L3 (D3cold), so we don't lose
> > much here to disable AER during system suspend.
> >
> > This is very similar to previous attempts to suspend AER and DPC [1],
> > but with a different reason.
> >
> > [1] https://lore.kernel.org/linux-pci/[email protected]/
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216295
> >
> > Reviewed-by: Mika Westerberg <[email protected]>
> > Signed-off-by: Kai-Heng Feng <[email protected]>
> > ---
>
> In Patch #1, you skip clearing AER errors in the resume path, right? So if we disable
> interrupts here, will AER driver not be notified on resume path error?
I agree the driver should report the error via aer_isr_one_error() on
resume path.
But on the system I am using (Intel ADL PCH), once the interrupt is
disabled, PCI_ERR_ROOT_STATUS doesn't record error anymore.
Not sure if it's intended though.
Kai-Heng
>
> > 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,
> > };
> >
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer