Re: [PATCH] PCI/PTM: Fix PTM switch capability evaluation

[Bjorn Helgaas <helgaas@kernel.org>]

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)