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

[Rafael J. Wysocki]

On Tue, Aug 30, 2022 at 6:25 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > ---
> >  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
> >