On Thu, Sep 29, 2022 at 5:24 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@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); > > +} > 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 > >
