linux-kernel - Re: [RESEND PATCH v3 2/2] PCI/PTM: fix to maintain pci_dev->ptm

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 30 Aug 2022 18:58:20 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Rajvi Jingar <rajvi.jingar@...ux.intel.com>,
        Rafael Wysocki <rafael.j.wysocki@...el.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        David Box <david.e.box@...ux.intel.com>,
        Linux PCI <linux-pci@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Kai-Heng Feng <kai.heng.feng@...onical.com>
Subject: Re: [RESEND PATCH v3 2/2] PCI/PTM: fix to maintain pci_dev->ptm_enabled

On Tue, Aug 30, 2022 at 6:25 PM Bjorn Helgaas <helgaas@...nel.org> wrote:
>
> [+cc Kai-Heng]
>
> On Tue, Aug 30, 2022 at 03:49:13AM -0700, Rajvi Jingar wrote:
> > pci_dev->ptm_enabled needs to be maintained to reflect the current PTM
> > state of the device. In pci_ptm_disable(), clear ptm_enabled from
> > 'struct pci_dev' on disabling PTM state for the device.
> > In pci_restore_ptm_state(), set dev->ptm_enabled based on the restored
> > PTM state of the device.
> >
> > In pci_ptm_disable(), perform ptm_enabled check to avoid config space
> > access in case if PTM is already disabled for the device. ptm_enabled
> > won't be set for non-PCIe devices so pci_is_pcie(dev) check is not
> > needed anymore.
>
> This one sounds like it's supposed to fix something, but I'm not clear
> exactly what.
>
> I have a vague memory of config accesses messing up a low power state.
> But this is still completely magical and unmaintainable since AFAIK
> there is nothing in the PCIe spec about avoiding config accesses when
> PTM is disabled.

Because ptm_enabled is expected to always reflect the hardware state,
pci_disable_ptm() needs to be amended to clear it.  Also it is prudent
to explicitly make it reflect the new hardware state in
pci_restore_ptm_state().

Then, pci_disable_ptm() can be made bail out if ptm_enabled is clear,
because it has nothing to do then and the pci_is_pcie() check in there
is not necessary, because ptm_enabled will never be set for devices
that are not PCIe.

> At the very least, we would need more details in the commit log and
> a hint in the code about this.
>
> > Signed-off-by: Rajvi Jingar <rajvi.jingar@...ux.intel.com>
> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > ---
> >  v1->v2:
> >    - add ptm_enabled check in pci_ptm_disable().
> >    - set the dev->ptm_enabled value in pci_restore_ptm_state().
> >  v2->v3:
> >    - remove pci_is_pcie(dev) check in pci_ptm_disable().
> >    - add Reviewed-by tag in commit message
> > ---
> >  drivers/pci/pcie/ptm.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
> > index 368a254e3124..1ce241d4538f 100644
> > --- a/drivers/pci/pcie/ptm.c
> > +++ b/drivers/pci/pcie/ptm.c
> > @@ -34,7 +34,7 @@ void pci_disable_ptm(struct pci_dev *dev)
> >       int ptm;
> >       u16 ctrl;
> >
> > -     if (!pci_is_pcie(dev))
> > +     if (!dev->ptm_enabled)
> >               return;
>
> This will conflict with a change Kai-Heng Feng and I have been working
> on, but I can resolve it when applying.
>
> >       ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
> > @@ -44,6 +44,7 @@ void pci_disable_ptm(struct pci_dev *dev)
> >       pci_read_config_word(dev, ptm + PCI_PTM_CTRL, &ctrl);
> >       ctrl &= ~(PCI_PTM_CTRL_ENABLE | PCI_PTM_CTRL_ROOT);
> >       pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl);
> > +     dev->ptm_enabled = 0;
> >  }
> >
> >  void pci_save_ptm_state(struct pci_dev *dev)
> > @@ -83,6 +84,7 @@ void pci_restore_ptm_state(struct pci_dev *dev)
> >
> >       cap = (u16 *)&save_state->cap.data[0];
> >       pci_write_config_word(dev, ptm + PCI_PTM_CTRL, *cap);
> > +     dev->ptm_enabled = !!(*cap & PCI_PTM_CTRL_ENABLE);
> >  }
> >
> >  void pci_ptm_init(struct pci_dev *dev)
> > --
> > 2.25.1
> >