| Message ID | 20220727013255.269815-2-kai.heng.feng@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] PCI/portdrv: Flag services when IRQ is shared with PME | expand |
On Wed, Jul 27, 2022 at 09:32:51AM +0800, Kai-Heng Feng wrote: > PCIe service that shares IRQ with PME may cause spurious wakeup on > system suspend. > > PCIe Base Spec 5.0, section 5.2 "Link State Power Management" states > that TLP and DLLP transmission is disabled for a Link in L2/L3 Ready > (D3hot), L2 (D3cold with aux power) and L3 (D3cold), so we don't lose > much here to disable AER during system suspend. > > This is very similar to previous attempts to suspend AER and DPC [1], > but with a different reason. > > [1] https://lore.kernel.org/linux-pci/20220408153159.106741-1-kai.heng.feng@canonical.com/ > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216295 > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/pci/pcie/aer.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index 7952e5efd6cf3..60cc373754af2 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -1372,6 +1372,26 @@ static int aer_probe(struct pcie_device *dev) > return 0; > } > > +static int aer_suspend(struct pcie_device *dev) > +{ > + struct aer_rpc *rpc = get_service_data(dev); > + > + if (dev->shared_pme_irq) > + aer_disable_rootport(rpc); aer_disable_rootport() seems like it might be overkill. IIUC, what we want to do here is disable AER interrupts, which should only require clearing ROOT_PORT_INTR_ON_MESG_MASK in PCI_ERR_ROOT_COMMAND. In addition to clearing ROOT_PORT_INTR_ON_MESG_MASK, aer_disable_rootport() traverses the whole hierarchy, clearing PCI_EXP_AER_FLAGS (CERE | NFERE | FERE | URRE) in PCI_EXP_DEVCTL. I don't think these DEVCTL bits control interrupt generation, so I don't know why we need to touch them. aer_disable_rootport() also clears PCI_ERR_ROOT_STATUS, which I think we should not do during suspend either. We might want to clear it on resume (which we already do in pci_restore_state()), but I think generally we should preserve error information as long as it doesn't cause trouble. Your thoughts please :) > + > + return 0; > +} > + > +static int aer_resume(struct pcie_device *dev) > +{ > + struct aer_rpc *rpc = get_service_data(dev); > + > + if (dev->shared_pme_irq) > + aer_enable_rootport(rpc); > + > + return 0; > +} > + > /** > * aer_root_reset - reset Root Port hierarchy, RCEC, or RCiEP > * @dev: pointer to Root Port, RCEC, or RCiEP > @@ -1441,8 +1461,9 @@ static struct pcie_port_service_driver aerdriver = { > .name = "aer", > .port_type = PCIE_ANY_PORT, > .service = PCIE_PORT_SERVICE_AER, > - > .probe = aer_probe, > + .suspend = aer_suspend, > + .resume = aer_resume, > .remove = aer_remove, > }; > > -- > 2.36.1 >
On Thu, Sep 29, 2022 at 5:46 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Jul 27, 2022 at 09:32:51AM +0800, Kai-Heng Feng wrote: > > PCIe service that shares IRQ with PME may cause spurious wakeup on > > system suspend. > > > > PCIe Base Spec 5.0, section 5.2 "Link State Power Management" states > > that TLP and DLLP transmission is disabled for a Link in L2/L3 Ready > > (D3hot), L2 (D3cold with aux power) and L3 (D3cold), so we don't lose > > much here to disable AER during system suspend. > > > > This is very similar to previous attempts to suspend AER and DPC [1], > > but with a different reason. > > > > [1] https://lore.kernel.org/linux-pci/20220408153159.106741-1-kai.heng.feng@canonical.com/ > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216295 > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > drivers/pci/pcie/aer.c | 23 ++++++++++++++++++++++- > > 1 file changed, 22 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > > index 7952e5efd6cf3..60cc373754af2 100644 > > --- a/drivers/pci/pcie/aer.c > > +++ b/drivers/pci/pcie/aer.c > > @@ -1372,6 +1372,26 @@ static int aer_probe(struct pcie_device *dev) > > return 0; > > } > > > > +static int aer_suspend(struct pcie_device *dev) > > +{ > > + struct aer_rpc *rpc = get_service_data(dev); > > + > > + if (dev->shared_pme_irq) > > + aer_disable_rootport(rpc); > > aer_disable_rootport() seems like it might be overkill. IIUC, what > we want to do here is disable AER interrupts, which should only > require clearing ROOT_PORT_INTR_ON_MESG_MASK in PCI_ERR_ROOT_COMMAND. > > In addition to clearing ROOT_PORT_INTR_ON_MESG_MASK, > aer_disable_rootport() traverses the whole hierarchy, clearing > PCI_EXP_AER_FLAGS (CERE | NFERE | FERE | URRE) in PCI_EXP_DEVCTL. > I don't think these DEVCTL bits control interrupt generation, so I > don't know why we need to touch them. > > aer_disable_rootport() also clears PCI_ERR_ROOT_STATUS, which I think > we should not do during suspend either. We might want to clear it > on resume (which we already do in pci_restore_state()), but I think > generally we should preserve error information as long as it doesn't > cause trouble. > > Your thoughts please :) Sorry for the belated response. Clearing ROOT_PORT_INTR_ON_MESG_MASK along to disable interrupt can solve the issue too. And I agree that the AER information should be preserved too. Kai-Heng > > > + > > + return 0; > > +} > > + > > +static int aer_resume(struct pcie_device *dev) > > +{ > > + struct aer_rpc *rpc = get_service_data(dev); > > + > > + if (dev->shared_pme_irq) > > + aer_enable_rootport(rpc); > > + > > + return 0; > > +} > > + > > /** > > * aer_root_reset - reset Root Port hierarchy, RCEC, or RCiEP > > * @dev: pointer to Root Port, RCEC, or RCiEP > > @@ -1441,8 +1461,9 @@ static struct pcie_port_service_driver aerdriver = { > > .name = "aer", > > .port_type = PCIE_ANY_PORT, > > .service = PCIE_PORT_SERVICE_AER, > > - > > .probe = aer_probe, > > + .suspend = aer_suspend, > > + .resume = aer_resume, > > .remove = aer_remove, > > }; > > > > -- > > 2.36.1 > >
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index 7952e5efd6cf3..60cc373754af2 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -1372,6 +1372,26 @@ static int aer_probe(struct pcie_device *dev) return 0; } +static int aer_suspend(struct pcie_device *dev) +{ + struct aer_rpc *rpc = get_service_data(dev); + + if (dev->shared_pme_irq) + aer_disable_rootport(rpc); + + return 0; +} + +static int aer_resume(struct pcie_device *dev) +{ + struct aer_rpc *rpc = get_service_data(dev); + + if (dev->shared_pme_irq) + aer_enable_rootport(rpc); + + return 0; +} + /** * aer_root_reset - reset Root Port hierarchy, RCEC, or RCiEP * @dev: pointer to Root Port, RCEC, or RCiEP @@ -1441,8 +1461,9 @@ static struct pcie_port_service_driver aerdriver = { .name = "aer", .port_type = PCIE_ANY_PORT, .service = PCIE_PORT_SERVICE_AER, - .probe = aer_probe, + .suspend = aer_suspend, + .resume = aer_resume, .remove = aer_remove, };
PCIe service that shares IRQ with PME may cause spurious wakeup on system suspend. PCIe Base Spec 5.0, section 5.2 "Link State Power Management" states that TLP and DLLP transmission is disabled for a Link in L2/L3 Ready (D3hot), L2 (D3cold with aux power) and L3 (D3cold), so we don't lose much here to disable AER during system suspend. This is very similar to previous attempts to suspend AER and DPC [1], but with a different reason. [1] https://lore.kernel.org/linux-pci/20220408153159.106741-1-kai.heng.feng@canonical.com/ Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216295 Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/pci/pcie/aer.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)