Re: [PATCH v7 2/3] PCI: AtomicOps: Do not enable without support in root port

[Gerd Bayer]

On Wed, 2026-04-01 at 12:27 -0500, Bjorn Helgaas wrote:
> On Mon, Mar 30, 2026 at 03:09:45PM +0200, 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.
> > 
> > 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.
> > 
> > Reported-by: Alexander Schmidt <alexs@xxxxxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: 430a23689dea ("PCI: Add pci_enable_atomic_ops_to_root()")
> > Signed-off-by: Gerd Bayer <gbayer@xxxxxxxxxxxxx>
> 
> OK, I think this is set to go.  It sounds like there are no RCiEPs
> that we need to worry about.
> 
> I think pci_enable_atomic_ops_to_root() will end up more readable if
> we check for the Root Port first and explicitly as in the modified
> version.  I *think* it's equivalent but can't easily test it.  What do
> you think?

At first sight it appears counter-intuitive to test the root-port's
capabilities before traversing the hierarchy - but with the explicit
read of the root port's DEVCAP2, we avoid the dependency to work on the
cap read within the while-loop.

My testing is somewhat limited, too - but I've verified that the
results with your patch (+ a small nit - see below) are the same as
with my version:

- ConnectX-5 Ex on s390: AtomicsOpsCtl: ReqEn-
- ConnectX-6 Dc on x86_64: AtomicsOpsCtl: ReqEn+

> 
> commit 2f3f32f2c180 ("PCI: Enable AtomicOps only if Root Port supports them")
> Author: Gerd Bayer <gbayer@xxxxxxxxxxxxx>
> Date:   Mon Mar 30 15:09:45 2026 +0200
> 
>     PCI: Enable AtomicOps only if Root Port supports them
>     
>     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 Root Port
>     lacking support for completing them:
>     
>       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+
>     
>     On s390 and many virtualized guests, the Endpoint is visible but the Root
>     Port is not.  In this case, pci_enable_atomic_ops_to_root() previously
>     enabled AtomicOps in the Endpoint even though it couldn't tell whether
>     the Root Port supports them as a completer.
>     
>     Change pci_enable_atomic_ops_to_root() to fail if there's no Root Port or
>     the Root Port doesn't support AtomicOps.
>     
>     Fixes: 430a23689dea ("PCI: Add pci_enable_atomic_ops_to_root()")
>     Reported-by: Alexander Schmidt <alexs@xxxxxxxxxxxxx>
>     Signed-off-by: Gerd Bayer <gbayer@xxxxxxxxxxxxx>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>     Cc: stable@xxxxxxxxxxxxxxx
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 135e5b591df4..515f565a4a70 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3675,8 +3675,7 @@ 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;
> +	struct pci_dev *root, *bridge;
>  	u32 cap, ctl2;
>  
>  	/*
> @@ -3705,35 +3704,35 @@ int pci_enable_atomic_ops_to_root(struct pci_dev *dev, u32 cap_mask)
>  		return -EINVAL;
>  	}
>  
> -	while (bus->parent) {
> -		bridge = bus->self;
> +	root = pcie_find_root_port(dev);
> +	if (!root)
> +		return -EINVAL;
>  
> -		pcie_capability_read_dword(bridge, PCI_EXP_DEVCAP2, &cap);
> +	pcie_capability_read_dword(bridge, PCI_EXP_DEVCAP2, &cap);

You want to read DEVCAP2 on root here, bridge is still unitialized.

> +	if ((cap & cap_mask) != cap_mask)
> +		return -EINVAL;
>  
> +	bridge = pci_upstream_bridge(dev);
> +	while (bridge != root) {
>  		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:
> +			pcie_capability_read_dword(bridge, PCI_EXP_DEVCAP2,
> +						   &cap);
> +			if (!(cap & PCI_EXP_DEVCAP2_ATOMIC_ROUTE))
> +				return -EINVAL;
> +			break;
>  		}
>  
> -		bus = bus->parent;
> +		bridge = pci_upstream_bridge(bridge);
>  	}
>  
>  	pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,

Thanks,
Gerd