| Message ID | 0b974380436d46ab3d8b7f4988f17e6f822079ac.1590068178.git.gustavo.pimentel@synopsys.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI/PTM: Fix PTM switch capability evaluation | expand |
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@synopsys.com> > Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > Cc: linux-pci@vger.kernel.org > Cc: Joao Pinto <jpinto@synopsys.com> 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? 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@google.com> 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@synopsys.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> 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)
On Thu, May 21, 2020 at 21:50:13, Bjorn Helgaas <helgaas@kernel.org> 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@synopsys.com> > > Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > > Cc: linux-pci@vger.kernel.org > > Cc: Joao Pinto <jpinto@synopsys.com> > > 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@google.com> > 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@synopsys.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > 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)
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@kernel.org> > 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@synopsys.com> > > > Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > > > Cc: linux-pci@vger.kernel.org > > > Cc: Joao Pinto <jpinto@synopsys.com> > > > > 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@google.com> > > 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@synopsys.com> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > > > 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) > >
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;
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@synopsys.com> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> Cc: linux-pci@vger.kernel.org Cc: Joao Pinto <jpinto@synopsys.com> --- drivers/pci/pcie/ptm.c | 8 ++++++++ 1 file changed, 8 insertions(+)