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

Gerd Bayer
2026-03-30T08:59:44Z

On Thu, 2026-03-26 at 11:40 -0500, Bjorn Helgaas wrote:
> 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.

That sounds like a plan. Working on a v7 - keeping the reference
updates as a third patch of the series.

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

Leave that as an execise for later.

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

Anyway, I'm wondering what the adoption of AtomicOps looks like. Not
much turned up in my searches.

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

Thanks,
Gerd