| Message ID | 20220727013255.269815-3-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [1/3] PCI/portdrv: Flag services when IRQ is shared with PME | expand |
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 >
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 > >
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, };
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(-)