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

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi,

On Tue, Nov 11, 2025 at 09:39:42AM -0600, Bjorn Helgaas wrote:
> 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().

Oh, that's good point actually.

Yeah it should be like this still:

if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
    pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM)
	pci_enable_ptm(dev, NULL);

To keep the behaviour same.

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

Fully agree. It was my mistake. If no objections I'll submit v5 with the
above added back (+ the newline inconsistency thing Lukas mentioned).