Re: [PATCH 3/3] PCI/DPC: Disable DPC service on suspend when IRQ is shared with PME

[Kai-Heng Feng]

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
> >