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

From: Bjorn Helgaas <helgaas@kernel.org>
To: Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>
Cc: Aditya Paluri <Venkata.AdityaPaluri@synopsys.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	Joao Pinto <Joao.Pinto@synopsys.com>
Subject: Re: [PATCH] PCI/PTM: Fix PTM switch capability evaluation
Date: Wed, 27 May 2020 16:40:34 -0500	[thread overview]
Message-ID: <20200527214034.GA266668@bjorn-Precision-5520> (raw)
In-Reply-To: <DM5PR12MB1276AA9D6ED3B737DD6C9F70DAB40@DM5PR12MB1276.namprd12.prod.outlook.com>

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