Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220830104913.1620539-2-rajvi.jingar@linux.intel.com> <20220830162529.GA106073@bhelgaas>
In-Reply-To: <20220830162529.GA106073@bhelgaas>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 30 Aug 2022 18:58:20 +0200
Message-ID: <CAJZ5v0jL812FKOJsmijE7gx5GEYp0hQ9+3UtQ_WUr-Uf5pgxAg@mail.gmail.com>
Subject: Re: [RESEND PATCH v3 2/2] PCI/PTM: fix to maintain pci_dev->ptm_enabled
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Rajvi Jingar <rajvi.jingar@linux.intel.com>,
        Rafael Wysocki <rafael.j.wysocki@intel.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        David Box <david.e.box@linux.intel.com>,
        Linux PCI <linux-pci@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        Kai-Heng Feng <kai.heng.feng@canonical.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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