Re: [PATCH v6 1/2] PCI: AtomicOps: Do not enable without support in root complex

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Mar 26, 2026 at 10:51:19AM +0100, Gerd Bayer wrote:
> On Wed, 2026-03-25 at 15:08 -0500, Bjorn Helgaas wrote:
> > On Wed, Mar 25, 2026 at 04:16:17PM +0100, Gerd Bayer wrote:
> > > When inspecting the config space of a Connect-X physical function in an
> > > s390 system after it was initialized by the mlx5_core device driver, we
> > > found the function to be enabled to request AtomicOps despite the
> > > system's root-complex lacking support for completing them:
> > > 
> > > 1ed0:00:00.1 Ethernet controller: Mellanox Technologies MT2894 Family [ConnectX-6 Lx]
> > > 	Subsystem: Mellanox Technologies Device 0002
> > >   [...]
> > > 	DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-
> > > 		 AtomicOpsCtl: ReqEn+
> > > 		 IDOReq- IDOCompl- LTR- EmergencyPowerReductionReq-
> > > 		 10BitTagReq- OBFF Disabled, EETLPPrefixBlk-
> > > 
> > > Turns out the device driver calls pci_enable_atomic_ops_to_root() which
> > > defaulted to enable AtomicOps requests even if it had no information
> > > about the root-port that the PCIe device is attached to. Similarly,
> > > AtomicOps requests are enabled for root complex integrated endpoints
> > > (RCiEPs) unconditionally.
> > > 
> > > Change the logic of pci_enable_atomic_ops_to_root() to fully traverse the
> > > PCIe tree upwards, check that the bridge devices support delivering
> > > AtomicOps transactions, and finally check that there is a root port at
> > > the end that does support completing AtomicOps - or that the support for
> > > completing AtomicOps at the root complex is announced through some other
> > > arch specific way.
> > > 
> > > Introduce a new pcibios_connects_to_atomicops_capable_rc() function to
> > > implement the check - and default to always "true". This leaves the
> > > semantics for today's RCiEPs intact. Pass in the device in question and
> > > the requested capabilities for future expansions.
> > > For s390, override pcibios_connects_to_atomicops_capable_rc() to
> > > always return "false".
> > > 
> > > Do not change the enablement of AtomicOps requests if there is no
> > > positive confirmation that the root complex can complete PCIe AtomicOps.
> > > 
> > > Reported-by: Alexander Schmidt <alexs@linux.ibm.com>
> > > Cc: stable@vger.kernel.org
> > > Fixes: 430a23689dea ("PCI: Add pci_enable_atomic_ops_to_root()")
> > > Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> > > ---
> > >  arch/s390/pci/pci.c |  5 +++++
> > >  drivers/pci/pci.c   | 48 +++++++++++++++++++++++++++++++-----------------
> > >  include/linux/pci.h |  1 +
> > >  3 files changed, 37 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> > > index 2a430722cbe415dd56c92fed2e513e524f46481a..a0bef77082a153a258fbe4abb1070b22e020888e 100644
> > > --- a/arch/s390/pci/pci.c
> > > +++ b/arch/s390/pci/pci.c
> > > @@ -265,6 +265,11 @@ static int zpci_cfg_store(struct zpci_dev *zdev, int offset, u32 val, u8 len)
> > >  	return rc;
> > >  }
> > >  
> > > +bool pcibios_connects_to_atomicops_capable_rc(struct pci_dev *dev, u32 cap_mask)
> > > +{
> > > +	return false;
> > > +}
> > > +
> > >  resource_size_t pcibios_align_resource(void *data, const struct resource *res,
> > >  				       resource_size_t size,
> > >  				       resource_size_t align)
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 8479c2e1f74f1044416281aba11bf071ea89488a..006aa589926cb290de43f152100ddaf9961407d1 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -3660,6 +3660,19 @@ void pci_acs_init(struct pci_dev *dev)
> > >  	pci_disable_broken_acs_cap(dev);
> > >  }
> > >  
> > > +static bool pci_is_atomicops_capable_rp(struct pci_dev *dev, u32 cap, u32 cap_mask)
> > > +{
> > > +	if (!dev || !(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT))
> > > +		return false;
> > > +
> > > +	return (cap & cap_mask) == cap_mask;
> > > +}
> > > +
> > > +bool __weak pcibios_connects_to_atomicops_capable_rc(struct pci_dev *dev, u32 cap_mask)
> > > +{
> > > +	return true;
> > > +}
> > > +
> > >  /**
> > >   * pci_enable_atomic_ops_to_root - enable AtomicOp requests to root port
> > >   * @dev: the PCI device
> > > @@ -3676,8 +3689,9 @@ void pci_acs_init(struct pci_dev *dev)
> > >  int pci_enable_atomic_ops_to_root(struct pci_dev *dev, u32 cap_mask)
> > >  {
> > >  	struct pci_bus *bus = dev->bus;
> > > -	struct pci_dev *bridge;
> > > -	u32 cap, ctl2;
> > > +	struct pci_dev *bridge = NULL;
> > > +	u32 cap = 0;
> > > +	u32 ctl2;
> > >  
> > >  	/*
> > >  	 * Per PCIe r5.0, sec 9.3.5.10, the AtomicOp Requester Enable bit
> > > @@ -3714,29 +3728,29 @@ int pci_enable_atomic_ops_to_root(struct pci_dev *dev, u32 cap_mask)
> > >  		switch (pci_pcie_type(bridge)) {
> > >  		/* Ensure switch ports support AtomicOp routing */
> > >  		case PCI_EXP_TYPE_UPSTREAM:
> > > -		case PCI_EXP_TYPE_DOWNSTREAM:
> > > -			if (!(cap & PCI_EXP_DEVCAP2_ATOMIC_ROUTE))
> > > -				return -EINVAL;
> > > -			break;
> > > -
> > > -		/* Ensure root port supports all the sizes we care about */
> > > -		case PCI_EXP_TYPE_ROOT_PORT:
> > > -			if ((cap & cap_mask) != cap_mask)
> > > -				return -EINVAL;
> > > -			break;
> > > -		}
> > > -
> > > -		/* Ensure upstream ports don't block AtomicOps on egress */
> > > -		if (pci_pcie_type(bridge) == PCI_EXP_TYPE_UPSTREAM) {
> > > +			/* Upstream ports must not block AtomicOps on egress */
> > >  			pcie_capability_read_dword(bridge, PCI_EXP_DEVCTL2,
> > >  						   &ctl2);
> > >  			if (ctl2 & PCI_EXP_DEVCTL2_ATOMIC_EGRESS_BLOCK)
> > >  				return -EINVAL;
> > > +			fallthrough;
> > > +		/* All switch ports need to route AtomicOps */
> > > +		case PCI_EXP_TYPE_DOWNSTREAM:
> > > +			if (!(cap & PCI_EXP_DEVCAP2_ATOMIC_ROUTE))
> > > +				return -EINVAL;
> > > +			break;
> > >  		}
> > > -
> > >  		bus = bus->parent;
> > >  	}
> > >  
> > > +	/*
> > > +	 * Finally, last bridge must be root port and support requested sizes
> > > +	 * or firmware asserts support
> > > +	 */
> > > +	if (!(pci_is_atomicops_capable_rp(bridge, cap, cap_mask) ||
> > > +	      pcibios_connects_to_atomicops_capable_rc(dev, cap_mask)))
> > > +		return -EINVAL;
> > 
> > Sashiko says:
> > 
> >   Since the generic weak implementation of
> >   pcibios_connects_to_atomicops_capable_rc() unconditionally returns
> >   true, the logical OR expression pci_is_atomicops_capable_rp(...) ||
> >   true will always evaluate to true. This makes the entire if
> >   condition evaluate to false.
> > 
> >   Because of this, it appears -EINVAL is never returned here, and any
> >   standard endpoint behind a Root Port will successfully be granted
> >   AtomicOps even if the Root Port lacks the capability in its
> >   PCI_EXP_DEVCAP2 register.
> 
> I've made the generic implementation of
> pcibios_connects_to_atomicops_capable_rc() default to return "true" to
> preserve the current code's handling of RCiEPs: Since they are not
> attached to a root port, their dev->bus->parent is NULL and the entire
> while-loop is bypassed - before this patch and after. (Sashiko was
> pointing at that being regressed with v4.)

The v4 patch definitely changed the behavior for RCiEPs: the current
v7.0-rc1 code always enables AtomicOps for RCiEPs, and the v4 patch
never enables AtomicOps for RCiEPs.  But I'm not sure this is a
regression.  It definitely *could* break an RCiEP, but AFAIK we have
no information about whether the RC supports AtomicOps, so enabling
them and telling the driver that AtomicOps work might be a lie.

IIUC, the motivation for this series was to avoid enabling AtomicOps
on s390 where there is no visible Root Port, and you have platform
knowledg that whatever is upstream from the endpoint in fact does not
support them.

I think we should avoid enabling AtomicOps unless we know for certain
that the completer (Root Port or RC) supports them.  To me that sounds
like:

  1) Never enable AtomicOps for RCiEPs.

  2) Only enable AtomicOps for endpoints below a Root Port that
  supports AtomicOps.

This could be two separate patches, where the second would fix the
s390 issue reported by Alexander.

If we come across RCiEPs that need AtomicOps and we somehow know that
the RC supports them, we can add a quirk or something to take
advantage of it.

We are still hand-waving about peer-to-peer transactions; we don't
even try to account for that because we don't know what peer might be
the completer.

> The whole point of pcibios_connects_to_atomicops_capable_rc() is to
> allow different architectures to implement a discriminator outside of
> PCIe's structure - potentially depending on CPU model or more.
> 
> The only point I wonder about: Should
> pcibios_connects_to_atomicops_capable_rc() default to return "false"
> and deliberately change the behavior for today's RCiEP's (if there are
> any...)?
> 
> > 
> > > +
> > >  	pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
> > >  				 PCI_EXP_DEVCTL2_ATOMIC_REQ);
> > >  	return 0;
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index 1c270f1d512301de4d462fe7e5097c32af5c6f8d..ef90604c39859ea8e61e5392d0bdaa1b0e43874b 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -692,6 +692,7 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
> > >  				 void *release_data);
> > >  
> > >  int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge);
> > > +bool pcibios_connects_to_atomicops_capable_rc(struct pci_dev *dev, u32 cap_mask);
> > >  
> > >  #define PCI_REGION_FLAG_MASK	0x0fU	/* These bits of resource flags tell us the PCI region flags */
> > >  
> > > 
> > > -- 
> > > 2.51.0
> > > 
> 
> Thanks,
> Gerd