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