On Fri, May 22, 2020 at 02:46:32PM +0000, Gustavo Pimentel wrote:
> On Thu, May 21, 2020 at 21:50:13, Bjorn Helgaas <helgaas@xxxxxxxxxx>
> wrote:
>
> > On Thu, May 21, 2020 at 03:37:30PM +0200, Gustavo Pimentel wrote:
> > > In order to enable PTM feature in a PCIe Endpoint, it is required to
> > > enable this feature as well in all devices capable (if present) in the
> > > datapath between the Root complex and the referred Endpoint e.g. PCIe
> > > switches.
> > >
> > > RC <--> Switch (USP) <-> Switch (DSP) <-> EP
> > >
> > > According to PCIe specification Rev5 (6.22.3.2) and (7.9.16), in order
> > > to enable this feature on a PTM capable switch, it's required to write a
> > > enable bit in the switch upstream port (USP) control register, which after
> > > that must respond to all PTM request messages received at any of its
> > > downstream ports (DSP).
> > >
> > > The previous implementation verifies if the PCIe switch has the PTM
> > > feature enabled on both streams ports (USP and DSP). Since the DSP
> > > doesn't support PTM capability, the previous implementation doesn't
> > > allow the PTM feature to be enabled in the Endpoint, the current patch
> > > fixes this.
> > >
> > > Fixes: eec097d43100 ("PCI: Add pci_enable_ptm() for drivers to enable PTM on endpoints")
> > > Signed-off-by: Aditya Paluri <venkata.adityapaluri@xxxxxxxxxxxx>
> > > Signed-off-by: Gustavo Pimentel <gustavo.pimentel@xxxxxxxxxxxx>
> > > Cc: linux-pci@xxxxxxxxxxxxxxx
> > > Cc: Joao Pinto <jpinto@xxxxxxxxxxxx>
> >
> > The existing code is definitely broken. I would prefer to fix this on
> > the enumeration side, as opposed to walking the tree at enable-time.
> > Can you try the alternate patch below?
>
> Ok, we have tested your patch and it's working.
Thanks! I applied this to pci/enumeration for v5.8.
> > > ---
> > > drivers/pci/pcie/ptm.c | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
> > > index 9361f3a..cd85d44 100644
> > > --- a/drivers/pci/pcie/ptm.c
> > > +++ b/drivers/pci/pcie/ptm.c
> > > @@ -111,6 +111,14 @@ int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
> > > */
> > > if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT) {
> > > ups = pci_upstream_bridge(dev);
> > > + /*
> > > + * Per PCIe r5.0, sec 7.9.16, the PTM capability is not
> > > + * permitted in Switch Downstream Ports
> > > + */
> > > + if (ups && ups->hdr_type == PCI_HEADER_TYPE_BRIDGE &&
> > > + pci_pcie_type(ups) == PCI_EXP_TYPE_DOWNSTREAM)
> > > + ups = pci_upstream_bridge(ups);
> > > +
> > > if (!ups || !ups->ptm_enabled)
> > > return -EINVAL;
> > >
> >
> > commit b9e92258d486 ("PCI/PTM: Inherit Switch Downstream Port PTM settings from Upstream Port")
> > Author: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > Date: Thu May 21 15:40:07 2020 -0500
> >
> > PCI/PTM: Inherit Switch Downstream Port PTM settings from Upstream Port
> >
> > Except for Endpoints, we enable PTM at enumeration-time. Previously we did
> > not account for the fact that Switch Downstream Ports may not have a PTM
> > capability; their PTM behavior is controlled by the Upstream Port (PCIe
> > r5.0, sec 7.9.16). Since Downstream Ports don't have a PTM capability, we
> > did not mark them as "ptm_enabled", which meant that pci_enable_ptm() on an
> > Endpoint failed because there was no PTM path to it.
> >
> > Mark Downstream Ports as "ptm_enabled" if their Upstream Port has PTM
> > enabled.
> >
> > Fixes: eec097d43100 ("PCI: Add pci_enable_ptm() for drivers to enable PTM on endpoints")
> > Reported-by: Aditya Paluri <Venkata.AdityaPaluri@xxxxxxxxxxxx>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> >
> > diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
> > index 9361f3aa26ab..0c42573a66d8 100644
> > --- a/drivers/pci/pcie/ptm.c
> > +++ b/drivers/pci/pcie/ptm.c
> > @@ -39,10 +39,6 @@ void pci_ptm_init(struct pci_dev *dev)
> > if (!pci_is_pcie(dev))
> > return;
> >
> > - pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
> > - if (!pos)
> > - return;
> > -
> > /*
> > * Enable PTM only on interior devices (root ports, switch ports,
> > * etc.) on the assumption that it causes no link traffic until an
> > @@ -52,6 +48,23 @@ void pci_ptm_init(struct pci_dev *dev)
> > pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END))
> > return;
> >
> > + /*
> > + * Switch Downstream Ports may not have a PTM capability; their PTM
> > + * behavior is controlled by the Upstream Port (PCIe r5.0, sec
> > + * 7.9.16).
> > + */
> > + ups = pci_upstream_bridge(dev);
> > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM &&
> > + ups && ups->ptm_enabled) {
> > + dev->ptm_granularity = ups->ptm_granularity;
> > + dev->ptm_enabled = 1;
> > + return;
> > + }
> > +
> > + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
> > + if (!pos)
> > + return;
> > +
> > pci_read_config_dword(dev, pos + PCI_PTM_CAP, &cap);
> > local_clock = (cap & PCI_PTM_GRANULARITY_MASK) >> 8;
> >
> > @@ -61,7 +74,6 @@ void pci_ptm_init(struct pci_dev *dev)
> > * the spec recommendation (PCIe r3.1, sec 7.32.3), select the
> > * furthest upstream Time Source as the PTM Root.
> > */
> > - ups = pci_upstream_bridge(dev);
> > if (ups && ups->ptm_enabled) {
> > ctrl = PCI_PTM_CTRL_ENABLE;
> > if (ups->ptm_granularity == 0)
>
>
