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

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Nov 11, 2025 at 04:46:53PM +0100, Mika Westerberg wrote:
> 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.

Makes sense.  We can consider whether and how to avoid enabling it for
Switches later.

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

Sounds good.