| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAAd53p7C8UkpEqTEy-WN-uKTSJYOuxPz1kOOcOykYZBvjQX0xg@mail.gmail.com>
Date: Mon, 17 Apr 2023 21:08:52 +0800
From: Kai-Heng Feng <kai.heng.feng@...onical.com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: bhelgaas@...gle.com, sathyanarayanan.kuppuswamy@...ux.intel.com,
linuxppc-dev@...ts.ozlabs.org, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org, koba.ko@...onical.com,
"Oliver O'Halloran" <oohall@...il.com>,
mika.westerberg@...ux.intel.com
Subject: Re: [PATCH 3/3] PCI/DPC: Disable DPC service on suspend when IRQ is
shared with PME
On Thu, Sep 29, 2022 at 5:24 AM Bjorn Helgaas <helgaas@...nel.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@...onical.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
> >
Powered by blists - more mailing lists