[PATCH v2] PCI/PTM: Do not enable PTM solely based on the capability existense

[PATCH v2] PCI/PTM: Do not enable PTM solely based on the capability existense

[Mika Westerberg]

It is not advisable to enable PTM solely based on the fact that the
capability exists. Instead there are separate bits in the capability
register that need to be set for the feature to be enabled for a given
component (this is suggestion from Intel PCIe folks):

  - PCIe Endpoint that has PTM capability must to declare requester
    capable
  - PCIe Switch Upstream Port that has PTM capability must declare
    at least responder capable
  - PCIe Root Port must declare root port capable.

Currently we see following:

  pci 0000:01:00.0: [8086:5786] type 01 class 0x060400 PCIe Switch Upstream Port
  pci 0000:01:00.0: PCI bridge to [bus 00]
  pci 0000:01:00.0:   bridge window [io  0x0000-0x0fff]
  pci 0000:01:00.0:   bridge window [mem 0x00000000-0x000fffff]
  pci 0000:01:00.0:   bridge window [mem 0x00000000-0x000fffff 64bit pref]
  ...
  pci 0000:01:00.0: PTM enabled, 4ns granularity
  ...
  pcieport 0000:00:07.0: AER: Multiple Uncorrectable (Non-Fatal) error message received from 0000:00:07.0
  pcieport 0000:00:07.0: PCIe Bus Error: severity=Uncorrectable (Non-Fatal), type=Transaction Layer, (Receiver ID)
  pcieport 0000:00:07.0:   device [8086:e44e] error status/mask=00200000/00000000
  pcieport 0000:00:07.0:    [21] ACSViol                (First)

The 01:00.0 PCIe Upstream Port has this:

  Capabilities: [220 v1] Precision Time Measurement
		PTMCap: Requester- Responder- Root-

This happens because Linux sees the PTM capability and blindly enables
PTM which then causes the AER error to trigger.

Fix this by enabling PTM only if the above described criteria is met.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
Previous version can be seen:

  https://lore.kernel.org/linux-pci/20251021104833.3729120-1-mika.westerberg@linux.intel.com/

Changes from the previous version:

  - Limit Switch Upstream Port only to Responder, not both Requester and
    Responder.

 drivers/pci/pcie/ptm.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
index 65e4b008be00..5ebb2edb4dec 100644
--- a/drivers/pci/pcie/ptm.c
+++ b/drivers/pci/pcie/ptm.c
@@ -81,9 +81,24 @@ void pci_ptm_init(struct pci_dev *dev)
 		dev->ptm_granularity = 0;
 	}
 
-	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
-	    pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM)
-		pci_enable_ptm(dev, NULL);
+	switch (pci_pcie_type(dev)) {
+	case PCI_EXP_TYPE_ROOT_PORT:
+		/*
+		 * Root Port must declare Root Capable if we want to
+		 * enable PTM for it.
+		 */
+		if (dev->ptm_root)
+			pci_enable_ptm(dev, NULL);
+		break;
+	case PCI_EXP_TYPE_UPSTREAM:
+		/*
+		 * Switch Upstream Ports must at least declare Responder
+		 * Capable if we want to enable PTM for it.
+		 */
+		if (cap & PCI_PTM_CAP_RES)
+			pci_enable_ptm(dev, NULL);
+		break;
+	}
 }
 
 void pci_save_ptm_state(struct pci_dev *dev)
@@ -125,7 +140,7 @@ static int __pci_enable_ptm(struct pci_dev *dev)
 {
 	u16 ptm = dev->ptm_cap;
 	struct pci_dev *ups;
-	u32 ctrl;
+	u32 cap, ctrl;
 
 	if (!ptm)
 		return -EINVAL;
@@ -144,6 +159,14 @@ static int __pci_enable_ptm(struct pci_dev *dev)
 			return -EINVAL;
 	}
 
+	/*
+	 * PCIe Endpoint must declare Requester Capable before we can
+	 * enable PTM for it.
+	 */
+	pci_read_config_dword(dev, ptm + PCI_PTM_CAP, &cap);
+	if (!(cap & PCI_PTM_CAP_REQ))
+		return -EINVAL;
+
 	pci_read_config_dword(dev, ptm + PCI_PTM_CTRL, &ctrl);
 
 	ctrl |= PCI_PTM_CTRL_ENABLE;
-- 
2.50.1

Re: [PATCH v2] PCI/PTM: Do not enable PTM solely based on the capability existense

[Lukas Wunner]

On Tue, Oct 28, 2025 at 07:04:27AM +0100, Mika Westerberg wrote:
> It is not advisable to enable PTM solely based on the fact that the
> capability exists. Instead there are separate bits in the capability
> register that need to be set for the feature to be enabled for a given
> component (this is suggestion from Intel PCIe folks):
> 
>   - PCIe Endpoint that has PTM capability must to declare requester
>     capable
>   - PCIe Switch Upstream Port that has PTM capability must declare
>     at least responder capable
>   - PCIe Root Port must declare root port capable.
[...]
> This happens because Linux sees the PTM capability and blindly enables
> PTM which then causes the AER error to trigger.
> 
> Fix this by enabling PTM only if the above described criteria is met.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

A stable designation might be merited, it looks like we've been doing
this wrong since forever:

Fixes: 9bb04a0c4e26 ("PCI: Add Precision Time Measurement (PTM) support")
Cc: stable@vger.kernel.org  # v4.9+

A spec reference in the commit message may also be helpful:

PCIe r7.0 sec 6.21.1 figure 6-21

I guess Bjorn could add those when applying if he deems them necessary,
so probably no reason to respin just for that.

Thanks,

Lukas

Re: [PATCH v2] PCI/PTM: Do not enable PTM solely based on the capability existense

[Bjorn Helgaas]

On Tue, Oct 28, 2025 at 07:04:27AM +0100, Mika Westerberg wrote:
> It is not advisable to enable PTM solely based on the fact that the
> capability exists. Instead there are separate bits in the capability
> register that need to be set for the feature to be enabled for a given
> component (this is suggestion from Intel PCIe folks):
> 
>   - PCIe Endpoint that has PTM capability must to declare requester
>     capable
>   - PCIe Switch Upstream Port that has PTM capability must declare
>     at least responder capable
>   - PCIe Root Port must declare root port capable.
> 
> Currently we see following:
> 
>   pci 0000:01:00.0: [8086:5786] type 01 class 0x060400 PCIe Switch Upstream Port
>   pci 0000:01:00.0: PCI bridge to [bus 00]
>   pci 0000:01:00.0:   bridge window [io  0x0000-0x0fff]
>   pci 0000:01:00.0:   bridge window [mem 0x00000000-0x000fffff]
>   pci 0000:01:00.0:   bridge window [mem 0x00000000-0x000fffff 64bit pref]
>   ...
>   pci 0000:01:00.0: PTM enabled, 4ns granularity
>   ...
>   pcieport 0000:00:07.0: AER: Multiple Uncorrectable (Non-Fatal) error message received from 0000:00:07.0
>   pcieport 0000:00:07.0: PCIe Bus Error: severity=Uncorrectable (Non-Fatal), type=Transaction Layer, (Receiver ID)
>   pcieport 0000:00:07.0:   device [8086:e44e] error status/mask=00200000/00000000
>   pcieport 0000:00:07.0:    [21] ACSViol                (First)
> 
> The 01:00.0 PCIe Upstream Port has this:
> 
>   Capabilities: [220 v1] Precision Time Measurement
> 		PTMCap: Requester- Responder- Root-
> 
> This happens because Linux sees the PTM capability and blindly enables
> PTM which then causes the AER error to trigger.
> 
> Fix this by enabling PTM only if the above described criteria is met.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> Previous version can be seen:
> 
>   https://lore.kernel.org/linux-pci/20251021104833.3729120-1-mika.westerberg@linux.intel.com/
> 
> Changes from the previous version:
> 
>   - Limit Switch Upstream Port only to Responder, not both Requester and
>     Responder.
> 
>  drivers/pci/pcie/ptm.c | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
> index 65e4b008be00..5ebb2edb4dec 100644
> --- a/drivers/pci/pcie/ptm.c
> +++ b/drivers/pci/pcie/ptm.c
> @@ -81,9 +81,24 @@ void pci_ptm_init(struct pci_dev *dev)
>  		dev->ptm_granularity = 0;
>  	}
>  
> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> -	    pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM)
> -		pci_enable_ptm(dev, NULL);
> +	switch (pci_pcie_type(dev)) {
> +	case PCI_EXP_TYPE_ROOT_PORT:
> +		/*
> +		 * Root Port must declare Root Capable if we want to
> +		 * enable PTM for it.
> +		 */
> +		if (dev->ptm_root)
> +			pci_enable_ptm(dev, NULL);
> +		break;
> +	case PCI_EXP_TYPE_UPSTREAM:
> +		/*
> +		 * Switch Upstream Ports must at least declare Responder
> +		 * Capable if we want to enable PTM for it.
> +		 */
> +		if (cap & PCI_PTM_CAP_RES)
> +			pci_enable_ptm(dev, NULL);
> +		break;
> +	}
>  }
>  
>  void pci_save_ptm_state(struct pci_dev *dev)
> @@ -125,7 +140,7 @@ static int __pci_enable_ptm(struct pci_dev *dev)
>  {
>  	u16 ptm = dev->ptm_cap;
>  	struct pci_dev *ups;
> -	u32 ctrl;
> +	u32 cap, ctrl;
>  
>  	if (!ptm)
>  		return -EINVAL;
> @@ -144,6 +159,14 @@ static int __pci_enable_ptm(struct pci_dev *dev)
>  			return -EINVAL;
>  	}
>  
> +	/*
> +	 * PCIe Endpoint must declare Requester Capable before we can
> +	 * enable PTM for it.
> +	 */
> +	pci_read_config_dword(dev, ptm + PCI_PTM_CAP, &cap);
> +	if (!(cap & PCI_PTM_CAP_REQ))
> +		return -EINVAL;

Isn't this going to prevent enabling PTM on Root Ports?

>  	pci_read_config_dword(dev, ptm + PCI_PTM_CTRL, &ctrl);
>  
>  	ctrl |= PCI_PTM_CTRL_ENABLE;
> -- 
> 2.50.1
> 

Re: [PATCH v2] PCI/PTM: Do not enable PTM solely based on the capability existense

[Mika Westerberg]

On Tue, Oct 28, 2025 at 12:06:39PM -0500, Bjorn Helgaas wrote:
> On Tue, Oct 28, 2025 at 07:04:27AM +0100, Mika Westerberg wrote:
> > It is not advisable to enable PTM solely based on the fact that the
> > capability exists. Instead there are separate bits in the capability
> > register that need to be set for the feature to be enabled for a given
> > component (this is suggestion from Intel PCIe folks):
> > 
> >   - PCIe Endpoint that has PTM capability must to declare requester
> >     capable
> >   - PCIe Switch Upstream Port that has PTM capability must declare
> >     at least responder capable
> >   - PCIe Root Port must declare root port capable.
> > 
> > Currently we see following:
> > 
> >   pci 0000:01:00.0: [8086:5786] type 01 class 0x060400 PCIe Switch Upstream Port
> >   pci 0000:01:00.0: PCI bridge to [bus 00]
> >   pci 0000:01:00.0:   bridge window [io  0x0000-0x0fff]
> >   pci 0000:01:00.0:   bridge window [mem 0x00000000-0x000fffff]
> >   pci 0000:01:00.0:   bridge window [mem 0x00000000-0x000fffff 64bit pref]
> >   ...
> >   pci 0000:01:00.0: PTM enabled, 4ns granularity
> >   ...
> >   pcieport 0000:00:07.0: AER: Multiple Uncorrectable (Non-Fatal) error message received from 0000:00:07.0
> >   pcieport 0000:00:07.0: PCIe Bus Error: severity=Uncorrectable (Non-Fatal), type=Transaction Layer, (Receiver ID)
> >   pcieport 0000:00:07.0:   device [8086:e44e] error status/mask=00200000/00000000
> >   pcieport 0000:00:07.0:    [21] ACSViol                (First)
> > 
> > The 01:00.0 PCIe Upstream Port has this:
> > 
> >   Capabilities: [220 v1] Precision Time Measurement
> > 		PTMCap: Requester- Responder- Root-
> > 
> > This happens because Linux sees the PTM capability and blindly enables
> > PTM which then causes the AER error to trigger.
> > 
> > Fix this by enabling PTM only if the above described criteria is met.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> > Previous version can be seen:
> > 
> >   https://lore.kernel.org/linux-pci/20251021104833.3729120-1-mika.westerberg@linux.intel.com/
> > 
> > Changes from the previous version:
> > 
> >   - Limit Switch Upstream Port only to Responder, not both Requester and
> >     Responder.
> > 
> >  drivers/pci/pcie/ptm.c | 31 +++++++++++++++++++++++++++----
> >  1 file changed, 27 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
> > index 65e4b008be00..5ebb2edb4dec 100644
> > --- a/drivers/pci/pcie/ptm.c
> > +++ b/drivers/pci/pcie/ptm.c
> > @@ -81,9 +81,24 @@ void pci_ptm_init(struct pci_dev *dev)
> >  		dev->ptm_granularity = 0;
> >  	}
> >  
> > -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> > -	    pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM)
> > -		pci_enable_ptm(dev, NULL);
> > +	switch (pci_pcie_type(dev)) {
> > +	case PCI_EXP_TYPE_ROOT_PORT:
> > +		/*
> > +		 * Root Port must declare Root Capable if we want to
> > +		 * enable PTM for it.
> > +		 */
> > +		if (dev->ptm_root)
> > +			pci_enable_ptm(dev, NULL);
> > +		break;
> > +	case PCI_EXP_TYPE_UPSTREAM:
> > +		/*
> > +		 * Switch Upstream Ports must at least declare Responder
> > +		 * Capable if we want to enable PTM for it.
> > +		 */
> > +		if (cap & PCI_PTM_CAP_RES)
> > +			pci_enable_ptm(dev, NULL);
> > +		break;
> > +	}
> >  }
> >  
> >  void pci_save_ptm_state(struct pci_dev *dev)
> > @@ -125,7 +140,7 @@ static int __pci_enable_ptm(struct pci_dev *dev)
> >  {
> >  	u16 ptm = dev->ptm_cap;
> >  	struct pci_dev *ups;
> > -	u32 ctrl;
> > +	u32 cap, ctrl;
> >  
> >  	if (!ptm)
> >  		return -EINVAL;
> > @@ -144,6 +159,14 @@ static int __pci_enable_ptm(struct pci_dev *dev)
> >  			return -EINVAL;
> >  	}
> >  
> > +	/*
> > +	 * PCIe Endpoint must declare Requester Capable before we can
> > +	 * enable PTM for it.
> > +	 */
> > +	pci_read_config_dword(dev, ptm + PCI_PTM_CAP, &cap);
> > +	if (!(cap & PCI_PTM_CAP_REQ))
> > +		return -EINVAL;
> 
> Isn't this going to prevent enabling PTM on Root Ports?

Isn't this function called only for Endpoints? Root Ports and Switch Ports
are enabled in pci_ptm_init() instead.

> >  	pci_read_config_dword(dev, ptm + PCI_PTM_CTRL, &ctrl);
> >  
> >  	ctrl |= PCI_PTM_CTRL_ENABLE;
> > -- 
> > 2.50.1
> > 

Re: [PATCH v2] PCI/PTM: Do not enable PTM solely based on the capability existense

[Lukas Wunner]

On Wed, Oct 29, 2025 at 06:33:54AM +0100, Mika Westerberg wrote:
> On Tue, Oct 28, 2025 at 12:06:39PM -0500, Bjorn Helgaas wrote:
> > On Tue, Oct 28, 2025 at 07:04:27AM +0100, Mika Westerberg wrote:
> > > @@ -125,7 +140,7 @@ static int __pci_enable_ptm(struct pci_dev *dev)
> > >  {
> > >  	u16 ptm = dev->ptm_cap;
> > >  	struct pci_dev *ups;
> > > -	u32 ctrl;
> > > +	u32 cap, ctrl;
> > >  
> > >  	if (!ptm)
> > >  		return -EINVAL;
> > > @@ -144,6 +159,14 @@ static int __pci_enable_ptm(struct pci_dev *dev)
> > >  			return -EINVAL;
> > >  	}
> > >  
> > > +	/*
> > > +	 * PCIe Endpoint must declare Requester Capable before we can
> > > +	 * enable PTM for it.
> > > +	 */
> > > +	pci_read_config_dword(dev, ptm + PCI_PTM_CAP, &cap);
> > > +	if (!(cap & PCI_PTM_CAP_REQ))
> > > +		return -EINVAL;
> > 
> > Isn't this going to prevent enabling PTM on Root Ports?
> 
> Isn't this function called only for Endpoints? Root Ports and Switch Ports
> are enabled in pci_ptm_init() instead.

The function is also called for Root Ports:

  pci_ptm_init()
    pci_enable_ptm()
      __pci_enable_ptm()

So I guess you need to constrain this to:

  PCI_EXP_TYPE_ENDPOINT
  PCI_EXP_TYPE_LEG_END

Sorry for missing that during review!

Lukas

Re: [PATCH v2] PCI/PTM: Do not enable PTM solely based on the capability existense

[Mika Westerberg]

On Wed, Oct 29, 2025 at 11:53:49AM +0100, Lukas Wunner wrote:
> On Wed, Oct 29, 2025 at 06:33:54AM +0100, Mika Westerberg wrote:
> > On Tue, Oct 28, 2025 at 12:06:39PM -0500, Bjorn Helgaas wrote:
> > > On Tue, Oct 28, 2025 at 07:04:27AM +0100, Mika Westerberg wrote:
> > > > @@ -125,7 +140,7 @@ static int __pci_enable_ptm(struct pci_dev *dev)
> > > >  {
> > > >  	u16 ptm = dev->ptm_cap;
> > > >  	struct pci_dev *ups;
> > > > -	u32 ctrl;
> > > > +	u32 cap, ctrl;
> > > >  
> > > >  	if (!ptm)
> > > >  		return -EINVAL;
> > > > @@ -144,6 +159,14 @@ static int __pci_enable_ptm(struct pci_dev *dev)
> > > >  			return -EINVAL;
> > > >  	}
> > > >  
> > > > +	/*
> > > > +	 * PCIe Endpoint must declare Requester Capable before we can
> > > > +	 * enable PTM for it.
> > > > +	 */
> > > > +	pci_read_config_dword(dev, ptm + PCI_PTM_CAP, &cap);
> > > > +	if (!(cap & PCI_PTM_CAP_REQ))
> > > > +		return -EINVAL;
> > > 
> > > Isn't this going to prevent enabling PTM on Root Ports?
> > 
> > Isn't this function called only for Endpoints? Root Ports and Switch Ports
> > are enabled in pci_ptm_init() instead.
> 
> The function is also called for Root Ports:
> 
>   pci_ptm_init()
>     pci_enable_ptm()
>       __pci_enable_ptm()

Ah, indeed. How did I miss that? ;-)

> So I guess you need to constrain this to:
> 
>   PCI_EXP_TYPE_ENDPOINT
>   PCI_EXP_TYPE_LEG_END

Sure I'll do that in v3.

Thanks!