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.
-Gustavo
>
> Bjorn
>
> > ---
> > 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)
