* [PATCH 2/3] PCI/AER: Disable AER service on suspend when IRQ is shared with PME [not found] <20220727013255.269815-1-kai.heng.feng@canonical.com> @ 2022-07-27 1:32 ` Kai-Heng Feng 2022-09-28 21:45 ` Bjorn Helgaas 2022-07-27 1:32 ` [PATCH 3/3] PCI/DPC: Disable DPC " Kai-Heng Feng 1 sibling, 1 reply; 6+ messages in thread From: Kai-Heng Feng @ 2022-07-27 1:32 UTC (permalink / raw) To: bhelgaas Cc: sathyanarayanan.kuppuswamy, linuxppc-dev, linux-pci, linux-kernel, koba.ko, Kai-Heng Feng, Oliver O'Halloran, mika.westerberg PCIe service that shares IRQ with PME may cause spurious wakeup on system suspend. PCIe Base Spec 5.0, section 5.2 "Link State Power Management" states that TLP and DLLP transmission is disabled for a Link in L2/L3 Ready (D3hot), L2 (D3cold with aux power) and L3 (D3cold), so we don't lose much here to disable AER during system suspend. This is very similar to previous attempts to suspend AER and DPC [1], but with a different reason. [1] https://lore.kernel.org/linux-pci/20220408153159.106741-1-kai.heng.feng@canonical.com/ Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216295 Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/pci/pcie/aer.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index 7952e5efd6cf3..60cc373754af2 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -1372,6 +1372,26 @@ static int aer_probe(struct pcie_device *dev) return 0; } +static int aer_suspend(struct pcie_device *dev) +{ + struct aer_rpc *rpc = get_service_data(dev); + + if (dev->shared_pme_irq) + aer_disable_rootport(rpc); + + return 0; +} + +static int aer_resume(struct pcie_device *dev) +{ + struct aer_rpc *rpc = get_service_data(dev); + + if (dev->shared_pme_irq) + aer_enable_rootport(rpc); + + return 0; +} + /** * aer_root_reset - reset Root Port hierarchy, RCEC, or RCiEP * @dev: pointer to Root Port, RCEC, or RCiEP @@ -1441,8 +1461,9 @@ static struct pcie_port_service_driver aerdriver = { .name = "aer", .port_type = PCIE_ANY_PORT, .service = PCIE_PORT_SERVICE_AER, - .probe = aer_probe, + .suspend = aer_suspend, + .resume = aer_resume, .remove = aer_remove, }; -- 2.36.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] PCI/AER: Disable AER service on suspend when IRQ is shared with PME 2022-07-27 1:32 ` [PATCH 2/3] PCI/AER: Disable AER service on suspend when IRQ is shared with PME Kai-Heng Feng @ 2022-09-28 21:45 ` Bjorn Helgaas 2023-04-17 11:50 ` Kai-Heng Feng 0 siblings, 1 reply; 6+ messages in thread From: Bjorn Helgaas @ 2022-09-28 21:45 UTC (permalink / raw) To: Kai-Heng Feng Cc: sathyanarayanan.kuppuswamy, mika.westerberg, linux-pci, linux-kernel, koba.ko, Oliver O'Halloran, bhelgaas, linuxppc-dev 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] PCI/AER: Disable AER service on suspend when IRQ is shared with PME 2022-09-28 21:45 ` Bjorn Helgaas @ 2023-04-17 11:50 ` Kai-Heng Feng 0 siblings, 0 replies; 6+ messages in thread From: Kai-Heng Feng @ 2023-04-17 11:50 UTC (permalink / raw) To: Bjorn Helgaas Cc: sathyanarayanan.kuppuswamy, mika.westerberg, linux-pci, linux-kernel, koba.ko, Oliver O'Halloran, bhelgaas, linuxppc-dev 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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] PCI/DPC: Disable DPC service on suspend when IRQ is shared with PME [not found] <20220727013255.269815-1-kai.heng.feng@canonical.com> 2022-07-27 1:32 ` [PATCH 2/3] PCI/AER: Disable AER service on suspend when IRQ is shared with PME Kai-Heng Feng @ 2022-07-27 1:32 ` Kai-Heng Feng 2022-09-28 21:24 ` Bjorn Helgaas 1 sibling, 1 reply; 6+ messages in thread From: Kai-Heng Feng @ 2022-07-27 1:32 UTC (permalink / raw) To: bhelgaas Cc: sathyanarayanan.kuppuswamy, linuxppc-dev, linux-pci, linux-kernel, koba.ko, Kai-Heng Feng, Oliver O'Halloran, mika.westerberg PCIe service that shares IRQ with PME may cause spurious wakeup on system suspend. Since AER is conditionally disabled in previous patch, also apply the same condition to disable DPC which depends on AER to work. PCIe Base Spec 5.0, section 5.2 "Link State Power Management" states that TLP and DLLP transmission is disabled for a Link in L2/L3 Ready (D3hot), L2 (D3cold with aux power) and L3 (D3cold), so we don't lose much here to disable DPC during system suspend. This is very similar to previous attempts to suspend AER and DPC [1], but with a different reason. [1] https://lore.kernel.org/linux-pci/20220408153159.106741-1-kai.heng.feng@canonical.com/ Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216295 Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/pci/pcie/dpc.c | 52 +++++++++++++++++++++++++++++++++--------- 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c index 3e9afee02e8d1..542f282c43f75 100644 --- a/drivers/pci/pcie/dpc.c +++ b/drivers/pci/pcie/dpc.c @@ -343,13 +343,33 @@ void pci_dpc_init(struct pci_dev *pdev) } } +static void dpc_enable(struct pcie_device *dev) +{ + struct pci_dev *pdev = dev->port; + u16 ctl; + + pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl); + ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN; + pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl); +} + +static void dpc_disable(struct pcie_device *dev) +{ + struct pci_dev *pdev = dev->port; + u16 ctl; + + pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl); + ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN); + pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl); +} + #define FLAG(x, y) (((x) & (y)) ? '+' : '-') static int dpc_probe(struct pcie_device *dev) { struct pci_dev *pdev = dev->port; struct device *device = &dev->device; int status; - u16 ctl, cap; + u16 cap; if (!pcie_aer_is_native(pdev) && !pcie_ports_dpc_native) return -ENOTSUPP; @@ -364,10 +384,7 @@ static int dpc_probe(struct pcie_device *dev) } pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap); - pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl); - - ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN; - pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl); + dpc_enable(dev); pci_info(pdev, "enabled with IRQ %d\n", dev->irq); pci_info(pdev, "error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n", @@ -380,14 +397,25 @@ static int dpc_probe(struct pcie_device *dev) return status; } -static void dpc_remove(struct pcie_device *dev) +static int dpc_suspend(struct pcie_device *dev) { - struct pci_dev *pdev = dev->port; - u16 ctl; + if (dev->shared_pme_irq) + dpc_disable(dev); - pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl); - ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN); - pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl); + return 0; +} + +static int dpc_resume(struct pcie_device *dev) +{ + if (dev->shared_pme_irq) + dpc_enable(dev); + + return 0; +} + +static void dpc_remove(struct pcie_device *dev) +{ + dpc_disable(dev); } static struct pcie_port_service_driver dpcdriver = { @@ -395,6 +423,8 @@ static struct pcie_port_service_driver dpcdriver = { .port_type = PCIE_ANY_PORT, .service = PCIE_PORT_SERVICE_DPC, .probe = dpc_probe, + .suspend = dpc_suspend, + .resume = dpc_resume, .remove = dpc_remove, }; -- 2.36.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] PCI/DPC: Disable DPC service on suspend when IRQ is shared with PME 2022-07-27 1:32 ` [PATCH 3/3] PCI/DPC: Disable DPC " Kai-Heng Feng @ 2022-09-28 21:24 ` Bjorn Helgaas 2023-04-17 13:08 ` Kai-Heng Feng 0 siblings, 1 reply; 6+ messages in thread From: Bjorn Helgaas @ 2022-09-28 21:24 UTC (permalink / raw) To: Kai-Heng Feng Cc: sathyanarayanan.kuppuswamy, mika.westerberg, linux-pci, linux-kernel, koba.ko, Oliver O'Halloran, bhelgaas, linuxppc-dev On Wed, Jul 27, 2022 at 09:32:52AM +0800, Kai-Heng Feng wrote: > PCIe service that shares IRQ with PME may cause spurious wakeup on > system suspend. > > Since AER is conditionally disabled in previous patch, also apply the > same condition to disable DPC which depends on AER to work. > > PCIe Base Spec 5.0, section 5.2 "Link State Power Management" states > that TLP and DLLP transmission is disabled for a Link in L2/L3 Ready > (D3hot), L2 (D3cold with aux power) and L3 (D3cold), so we don't lose > much here to disable DPC during system suspend. > > This is very similar to previous attempts to suspend AER and DPC [1], > but with a different reason. > > [1] https://lore.kernel.org/linux-pci/20220408153159.106741-1-kai.heng.feng@canonical.com/ > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216295 > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/pci/pcie/dpc.c | 52 +++++++++++++++++++++++++++++++++--------- > 1 file changed, 41 insertions(+), 11 deletions(-) > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c > index 3e9afee02e8d1..542f282c43f75 100644 > --- a/drivers/pci/pcie/dpc.c > +++ b/drivers/pci/pcie/dpc.c > @@ -343,13 +343,33 @@ void pci_dpc_init(struct pci_dev *pdev) > } > } > > +static void dpc_enable(struct pcie_device *dev) > +{ > + struct pci_dev *pdev = dev->port; > + u16 ctl; > + > + pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl); > + ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN; > + pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl); > +} I guess the reason we need this is because we disable interupts in pci_pm_suspend() first, then we call pci_save_dpc_state() from pci_pm_suspend_noirq(), so we save the *disabled* control register. Then when we resume, we restore that disabled control register so we need to enable DPC again. Right? I think we should save a "dpc_enabled" bit in the pci_dev and conditionally set PCI_EXP_DPC_CTL_INT_EN here. If we unconditionally set it here, we depend on portdrv *not* calling dpc_resume() if we didn't enable DPC at enumeration-time for some reason. And I would leave PCI_EXP_DPC_CTL_EN_FATAL alone; see below. > +static void dpc_disable(struct pcie_device *dev) > +{ > + struct pci_dev *pdev = dev->port; > + u16 ctl; > + > + pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl); > + ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN); > + pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl); #define PCI_EXP_DPC_CTL_EN_FATAL 0x0001 #define PCI_EXP_DPC_CTL_INT_EN 0x0008 Clearing PCI_EXP_DPC_CTL_INT_EN makes sense to me, but I don't understand the PCI_EXP_DPC_CTL_EN_FATAL part. PCI_EXP_DPC_CTL_EN_FATAL is one of the four values of the two-bit DPC Trigger Enable, so clearing that bit leaves the field as either 00b (DPC is disabled) or 10b (DPC enabled and triggered when the port detects an uncorrectable error or receives an ERR_NONFATAL or ERR_FATAL message). I think we should only clear PCI_EXP_DPC_CTL_INT_EN. > +} > + > #define FLAG(x, y) (((x) & (y)) ? '+' : '-') > static int dpc_probe(struct pcie_device *dev) > { > struct pci_dev *pdev = dev->port; > struct device *device = &dev->device; > int status; > - u16 ctl, cap; > + u16 cap; > > if (!pcie_aer_is_native(pdev) && !pcie_ports_dpc_native) > return -ENOTSUPP; > @@ -364,10 +384,7 @@ static int dpc_probe(struct pcie_device *dev) > } > > pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap); > - pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl); > - > - ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN; > - pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl); I think we should keep the PCI_EXP_DPC_CTL_EN_FATAL part here. That just sets the desired trigger mode but AFAICT, has nothing to do with generating interrupts. > + dpc_enable(dev); Then dpc_enable() could be called something like dpc_enable_irq(), and it would *only* control interupt generation. > pci_info(pdev, "enabled with IRQ %d\n", dev->irq); > > pci_info(pdev, "error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n", > @@ -380,14 +397,25 @@ static int dpc_probe(struct pcie_device *dev) > return status; > } > > -static void dpc_remove(struct pcie_device *dev) > +static int dpc_suspend(struct pcie_device *dev) > { > - struct pci_dev *pdev = dev->port; > - u16 ctl; > + if (dev->shared_pme_irq) > + dpc_disable(dev); > > - pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl); > - ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN); > - pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl); > + return 0; > +} > + > +static int dpc_resume(struct pcie_device *dev) > +{ > + if (dev->shared_pme_irq) > + dpc_enable(dev); > + > + return 0; > +} > + > +static void dpc_remove(struct pcie_device *dev) > +{ > + dpc_disable(dev); > } > > static struct pcie_port_service_driver dpcdriver = { > @@ -395,6 +423,8 @@ static struct pcie_port_service_driver dpcdriver = { > .port_type = PCIE_ANY_PORT, > .service = PCIE_PORT_SERVICE_DPC, > .probe = dpc_probe, > + .suspend = dpc_suspend, > + .resume = dpc_resume, > .remove = dpc_remove, > }; > > -- > 2.36.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] PCI/DPC: Disable DPC service on suspend when IRQ is shared with PME 2022-09-28 21:24 ` Bjorn Helgaas @ 2023-04-17 13:08 ` Kai-Heng Feng 0 siblings, 0 replies; 6+ messages in thread From: Kai-Heng Feng @ 2023-04-17 13:08 UTC (permalink / raw) To: Bjorn Helgaas Cc: sathyanarayanan.kuppuswamy, mika.westerberg, linux-pci, linux-kernel, koba.ko, Oliver O'Halloran, bhelgaas, linuxppc-dev On Thu, Sep 29, 2022 at 5:24 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Jul 27, 2022 at 09:32:52AM +0800, Kai-Heng Feng wrote: > > PCIe service that shares IRQ with PME may cause spurious wakeup on > > system suspend. > > > > Since AER is conditionally disabled in previous patch, also apply the > > same condition to disable DPC which depends on AER to work. > > > > PCIe Base Spec 5.0, section 5.2 "Link State Power Management" states > > that TLP and DLLP transmission is disabled for a Link in L2/L3 Ready > > (D3hot), L2 (D3cold with aux power) and L3 (D3cold), so we don't lose > > much here to disable DPC during system suspend. > > > > This is very similar to previous attempts to suspend AER and DPC [1], > > but with a different reason. > > > > [1] https://lore.kernel.org/linux-pci/20220408153159.106741-1-kai.heng.feng@canonical.com/ > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216295 > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > drivers/pci/pcie/dpc.c | 52 +++++++++++++++++++++++++++++++++--------- > > 1 file changed, 41 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c > > index 3e9afee02e8d1..542f282c43f75 100644 > > --- a/drivers/pci/pcie/dpc.c > > +++ b/drivers/pci/pcie/dpc.c > > @@ -343,13 +343,33 @@ void pci_dpc_init(struct pci_dev *pdev) > > } > > } > > > > +static void dpc_enable(struct pcie_device *dev) > > +{ > > + struct pci_dev *pdev = dev->port; > > + u16 ctl; > > + > > + pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl); > > + ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN; > > + pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl); > > +} > Sorry for the belated response. > I guess the reason we need this is because we disable interupts in > pci_pm_suspend() first, then we call pci_save_dpc_state() from > pci_pm_suspend_noirq(), so we save the *disabled* control register. > Then when we resume, we restore that disabled control register so we > need to enable DPC again. Right? Sorry for the belated response. Yes, and the same logic applies to AER too. > > I think we should save a "dpc_enabled" bit in the pci_dev and > conditionally set PCI_EXP_DPC_CTL_INT_EN here. If we unconditionally > set it here, we depend on portdrv *not* calling dpc_resume() if we > didn't enable DPC at enumeration-time for some reason. Does this scenario really happen? Once the port is marked with PCIE_PORT_SERVICE_DPC, DPC will be enabled by dpc_probe(). So an additional bit seems to be unnecessary. > > And I would leave PCI_EXP_DPC_CTL_EN_FATAL alone; see below. > > > +static void dpc_disable(struct pcie_device *dev) > > +{ > > + struct pci_dev *pdev = dev->port; > > + u16 ctl; > > + > > + pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl); > > + ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN); > > + pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl); > > #define PCI_EXP_DPC_CTL_EN_FATAL 0x0001 > #define PCI_EXP_DPC_CTL_INT_EN 0x0008 > > Clearing PCI_EXP_DPC_CTL_INT_EN makes sense to me, but I don't > understand the PCI_EXP_DPC_CTL_EN_FATAL part. > > PCI_EXP_DPC_CTL_EN_FATAL is one of the four values of the two-bit DPC > Trigger Enable, so clearing that bit leaves the field as either 00b > (DPC is disabled) or 10b (DPC enabled and triggered when the port > detects an uncorrectable error or receives an ERR_NONFATAL or > ERR_FATAL message). > > I think we should only clear PCI_EXP_DPC_CTL_INT_EN. Yes, clearing PCI_EXP_DPC_CTL_INT_EN should be sufficient. > > > +} > > + > > #define FLAG(x, y) (((x) & (y)) ? '+' : '-') > > static int dpc_probe(struct pcie_device *dev) > > { > > struct pci_dev *pdev = dev->port; > > struct device *device = &dev->device; > > int status; > > - u16 ctl, cap; > > + u16 cap; > > > > if (!pcie_aer_is_native(pdev) && !pcie_ports_dpc_native) > > return -ENOTSUPP; > > @@ -364,10 +384,7 @@ static int dpc_probe(struct pcie_device *dev) > > } > > > > pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap); > > - pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl); > > - > > - ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN; > > - pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl); > > I think we should keep the PCI_EXP_DPC_CTL_EN_FATAL part here. That > just sets the desired trigger mode but AFAICT, has nothing to do with > generating interrupts. Agree. Will do it in next revision. > > > + dpc_enable(dev); > > Then dpc_enable() could be called something like dpc_enable_irq(), and > it would *only* control interupt generation. Will do. Kai-Heng > > > pci_info(pdev, "enabled with IRQ %d\n", dev->irq); > > > > pci_info(pdev, "error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n", > > @@ -380,14 +397,25 @@ static int dpc_probe(struct pcie_device *dev) > > return status; > > } > > > > -static void dpc_remove(struct pcie_device *dev) > > +static int dpc_suspend(struct pcie_device *dev) > > { > > - struct pci_dev *pdev = dev->port; > > - u16 ctl; > > + if (dev->shared_pme_irq) > > + dpc_disable(dev); > > > > - pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl); > > - ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN); > > - pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl); > > + return 0; > > +} > > + > > +static int dpc_resume(struct pcie_device *dev) > > +{ > > + if (dev->shared_pme_irq) > > + dpc_enable(dev); > > + > > + return 0; > > +} > > + > > +static void dpc_remove(struct pcie_device *dev) > > +{ > > + dpc_disable(dev); > > } > > > > static struct pcie_port_service_driver dpcdriver = { > > @@ -395,6 +423,8 @@ static struct pcie_port_service_driver dpcdriver = { > > .port_type = PCIE_ANY_PORT, > > .service = PCIE_PORT_SERVICE_DPC, > > .probe = dpc_probe, > > + .suspend = dpc_suspend, > > + .resume = dpc_resume, > > .remove = dpc_remove, > > }; > > > > -- > > 2.36.1 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-04-17 13:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20220727013255.269815-1-kai.heng.feng@canonical.com>
2022-07-27 1:32 ` [PATCH 2/3] PCI/AER: Disable AER service on suspend when IRQ is shared with PME Kai-Heng Feng
2022-09-28 21:45 ` Bjorn Helgaas
2023-04-17 11:50 ` Kai-Heng Feng
2022-07-27 1:32 ` [PATCH 3/3] PCI/DPC: Disable DPC " Kai-Heng Feng
2022-09-28 21:24 ` Bjorn Helgaas
2023-04-17 13:08 ` Kai-Heng Feng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).