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

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

[Gustavo Pimentel]

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(+)

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;
 
-- 
2.7.4

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

[Bjorn Helgaas]

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)

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

[Gustavo Pimentel]

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)

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

[Bjorn Helgaas]

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)
> 
>