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@xxxxxxxxxxxxx/ > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216295 > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> > --- > 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 >
