Re: [PATCH v4] PCI/PTM: Enable PTM only if it advertises a role

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Nov 11, 2025 at 07:10:48AM +0100, Mika Westerberg wrote:
> We have a Upstream Port (2b:00.0) that has following in the PTM capability:
> 
>   Capabilities: [220 v1] Precision Time Measurement
> 		PTMCap: Requester- Responder- Root-
> 
> Linux enables PTM for this without looking into what roles it actually
> supports. Immediately after enabling PTM we start getting these:
> 
>   pci 0000:2b:00.0: [8086:5786] type 01 class 0x060400 PCIe Switch Upstream Port
>   ...
>   pci 0000:2b:00.0: PTM enabled, 4ns granularity
>   ...
>   pcieport 0000:00:07.1: AER: Multiple Uncorrectable (Non-Fatal) error message received from 0000:00:07.1
>   pcieport 0000:00:07.1: PCIe Bus Error: severity=Uncorrectable (Non-Fatal), type=Transaction Layer, (Receiver ID)
>   pcieport 0000:00:07.1:   device [8086:e44f] error status/mask=00200000/00000000
>   pcieport 0000:00:07.1:    [21] ACSViol                (First)
>   pcieport 0000:00:07.1: AER:   TLP Header: 0x34000000 0x00000052 0x00000000 0x00000000
> 
> Fix this by enabling PTM only if any of the following conditions are
> true (see more in PCIe r7.0 sec 6.21.1 figure 6-21):
> 
>   - 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.
> 
> While there make the enabling happen for all in __pci_enable_ptm() instead
> of enabling some in pci_ptm_init() and some in __pci_enable_ptm().
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> Previous versions can be seen:
> 
>   v3: https://lore.kernel.org/linux-pci/20251030134606.3782352-1-mika.westerberg@linux.intel.com/
>   v2: https://lore.kernel.org/linux-pci/20251028060427.2163115-1-mika.westerberg@linux.intel.com/
>   v1: https://lore.kernel.org/linux-pci/20251021104833.3729120-1-mika.westerberg@linux.intel.com/
> 
> Changes from v3:
> 
>   - Cache the responder and requester capability bits.
>   - Enable PTM only in __pci_enable_ptm().
>   - Update $subject and commit message.
>   - Since this is changed quite a lot, I dropped the Reviewed-by from Lukas
>     and also stable tag.
> 
> Changes from v2:
> 
>   - Limit the check in __pci_enable_ptm() to Endpoints and Legacy
>     Endpoints.
>   - Added stable tags suggested by Lukas, and PCIe spec reference.
>   - Added Reviewed-by tag from Lukas (hope it is okay to keep).
> 
> Changes from v1:
> 
>   - Limit Switch Upstream Port only to Responder, not both Requester and
>     Responder.
> 
>  drivers/pci/pcie/ptm.c | 41 ++++++++++++++++++++++++++++++++++++++---
>  include/linux/pci.h    |  2 ++
>  2 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
> index 65e4b008be00..30e25f1ad28e 100644
> --- a/drivers/pci/pcie/ptm.c
> +++ b/drivers/pci/pcie/ptm.c
> @@ -81,9 +81,12 @@ 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);
> +	if (cap & PCI_PTM_CAP_RES)
> +		dev->ptm_responder = 1;
> +	if (cap & PCI_PTM_CAP_REQ)
> +		dev->ptm_requester = 1;
> +
> +	pci_enable_ptm(dev, NULL);

This seems nice and clean overall.

I do wonder about the fact that previously we automatically enabled
PTM only for Root Ports and Switch Upstream Ports, but we didn't
enable it for Endpoints until a driver called pci_enable_ptm().

With this change, it looks like we automatically enable PTM for every
device that supports it.  Worth a mention in the commit log, and we
might also want to revisit the drivers (ice, idpf, igc, mlx5) that
explicitly enable it to remove the enable and disable calls there.

PTM consumes some link bandwidth, so the idea was to avoid paying that
cost unless a driver actually wanted to use PTM.  PTM Messages are
local, so they terminate at the Switch Downstream Port or Root Port
that receives them.  I assumed that Switches would only send PTM
Requests upstream if they received a PTM Request from a downstream
device, so I thought it would be free to enable PTM on the Switch.

But apparently that isn't true; these errors happen immediately when
enabling PTM on the Switch, before it's enabled on any downstream
device.  And it makes sense that a Switch could provide better service
if it kept its local Time Source updated so it could generate PTM
Responses directly instead of sending a request upstream, waiting for
a response, and passing it along downstream.

I still feel like it's worth avoiding the bandwidth cost by leaving
PTM disabled unless a driver wants it.  The cost is probably small,
but why pay it if we're not using PTM?  What are your thoughts?

I could imagine leaving everything disabled until an endpoint driver
enables it, and then enabling it in the whole path up to the root.  I
don't really *like* modifying things outside the actual device owned
by the driver, because then we have to worry about concurrency and
locking for the rest of the path.  But maybe that's the only way?

The errors seem pretty important to fix, so maybe we should apply this
for v6.19 even if we want to do more work to avoid the bandwidth cost
later?

>  }
>  
>  void pci_save_ptm_state(struct pci_dev *dev)
> @@ -144,6 +147,38 @@ static int __pci_enable_ptm(struct pci_dev *dev)
>  			return -EINVAL;
>  	}
>  
> +	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)
> +			return -EINVAL;
> +		break;
> +	case PCI_EXP_TYPE_UPSTREAM:
> +		/*
> +		 * Switch Upstream Ports must at least declare Responder
> +		 * Capable if we want to enable PTM for it.
> +		 */
> +		if (!dev->ptm_responder)
> +			return -EINVAL;
> +		break;
> +
> +	case PCI_EXP_TYPE_ENDPOINT:
> +	case PCI_EXP_TYPE_LEG_END:
> +		/*
> +		 * PCIe Endpoint must declare Requester Capable before we
> +		 * can enable PTM for it.
> +		 */
> +		if (!dev->ptm_requester)
> +			return -EINVAL;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
>  	pci_read_config_dword(dev, ptm + PCI_PTM_CTRL, &ctrl);
>  
>  	ctrl |= PCI_PTM_CTRL_ENABLE;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d1fdf81fbe1e..d5018cb5c331 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -500,6 +500,8 @@ struct pci_dev {
>  #ifdef CONFIG_PCIE_PTM
>  	u16		ptm_cap;		/* PTM Capability */
>  	unsigned int	ptm_root:1;
> +	unsigned int	ptm_responder:1;
> +	unsigned int	ptm_requester:1;
>  	unsigned int	ptm_enabled:1;
>  	u8		ptm_granularity;
>  #endif
> -- 
> 2.50.1
>