Re: [PATCH v2 09/13] PCI/CXL: Disable and re-enable CXL regions
From: Richard Cheng
Date: Wed Aug 26 2026 - 05:04:41 EST
On Tue, Aug 25, 2026 at 04:26:24AM +0800, Fabio M. De Francesco wrote:
> A Secondary Bus Reset of a CXL Downstream Port needs every CXL region
> routed through that Port disabled for the duration.
>
> Add struct pci_cxl_sbr_region_ops, holding a disable_regions() and an
> enable_regions() callback, and pci_cxl_set_sbr_region_ops() for the CXL
> core to register them. The pointer is NULL whenever the CXL region code
> is absent, either not built or built as a module that is not loaded.
>
> Add cxl_sbr_disable_regions(), which collects the regions with a member
> endpoint below the Port and disables each one. If any fails, re-enable
> the whole collected set and return the error, so the PCI core aborts the
> reset before touching hardware; re-enabling a region left untouched is a
> no-op, which also recovers the one that failed midway.
>
> Add cxl_sbr_enable_regions() for the other side. It restores the HDM
> decoders below the Port before it re-attaches any region driver, since a
> region cannot serve memory through decoders that are not programmed.
>
> cxl_sbr_disable_regions() records the decoder registers before it
> touches anything and cxl_sbr_enable_regions() hands them to the restore
> and releases them. They are held in an xarray indexed by the Downstream
> Port's struct pci_dev, so resets of different Ports do not share an
> entry, and they are released on the abort path too, where
> enable_regions() never runs.
>
> Register both ops from cxl_region_init() and clear the pointer in
> cxl_region_exit().
>
> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@xxxxxxxxxxxxxxx>
> ---
> drivers/cxl/core/core.h | 9 +--
> drivers/cxl/core/dport_sbr.c | 152 +++++++++++++++++++++++++++++++++--
> drivers/cxl/core/region.c | 3 +
> drivers/pci/pci.c | 13 +++
> include/linux/pci.h | 12 +++
> 5 files changed, 175 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 077a2af9cf0c..b250fa346184 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -57,13 +57,8 @@ int devm_cxl_add_dax_region(struct cxl_region *cxlr);
> int devm_cxl_add_pmem_region(struct cxl_region *cxlr);
> void kill_regions(struct cxl_root_decoder *cxlrd);
> int cxl_region_invalidate_memregion(struct cxl_region *cxlr);
> -int cxl_region_disable(struct cxl_region *cxlr);
> -void cxl_region_enable(struct cxl_region *cxlr);
> -struct pci_dev;
> -int cxl_sbr_collect_regions(struct pci_dev *dport_pci, struct xarray *regions);
> -void cxl_sbr_put_regions(struct xarray *regions);
> -void cxl_sbr_recommit_decoders(struct pci_dev *dport_pci,
> - struct xarray *hdm_state);
> +struct pci_cxl_sbr_region_ops;
> +extern const struct pci_cxl_sbr_region_ops cxl_sbr_region_ops;
>
> #else
> static inline u64 cxl_dpa_to_hpa(struct cxl_region *cxlr,
> diff --git a/drivers/cxl/core/dport_sbr.c b/drivers/cxl/core/dport_sbr.c
> index 233499bc1fad..2b6f840e22a7 100644
> --- a/drivers/cxl/core/dport_sbr.c
> +++ b/drivers/cxl/core/dport_sbr.c
> @@ -21,7 +21,7 @@
> * Context: process context. Offlining and driver unbind sleep and take the
> * memory hotplug lock, so this cannot run in atomic context.
> */
> -int cxl_region_disable(struct cxl_region *cxlr)
> +static int cxl_region_disable(struct cxl_region *cxlr)
> {
> struct cxl_region_params *p = &cxlr->params;
> unsigned long block_size;
> @@ -72,7 +72,7 @@ int cxl_region_disable(struct cxl_region *cxlr)
> * Rebind the region driver. The System RAM is left offline; bringing it back
> * online is a separate administrative step.
> */
> -void cxl_region_enable(struct cxl_region *cxlr)
> +static void cxl_region_enable(struct cxl_region *cxlr)
> {
> struct cxl_region_params *p = &cxlr->params;
>
> @@ -93,8 +93,8 @@ void cxl_region_enable(struct cxl_region *cxlr)
> * cxl_region_disable()/cxl_region_enable() run with the rwsem released (they
> * unbind and rebind the region driver). Hence snapshot the set first.
> */
> -int cxl_sbr_collect_regions(struct pci_dev *dport_pci,
> - struct xarray *regions)
> +static int cxl_sbr_collect_regions(struct pci_dev *dport_pci,
> + struct xarray *regions)
> {
> struct cxl_region_ref *cxl_rr;
> struct cxl_dport *dport;
> @@ -141,7 +141,7 @@ int cxl_sbr_collect_regions(struct pci_dev *dport_pci,
> return 0;
> }
>
> -void cxl_sbr_put_regions(struct xarray *regions)
> +static void cxl_sbr_put_regions(struct xarray *regions)
> {
> struct cxl_region *cxlr;
> unsigned long index;
> @@ -159,8 +159,8 @@ void cxl_sbr_put_regions(struct xarray *regions)
> * requires. The caller has already disabled the regions, so nothing reaches the
> * decoders being reprogrammed.
> */
> -void cxl_sbr_recommit_decoders(struct pci_dev *dport_pci,
> - struct xarray *hdm_state)
> +static void cxl_sbr_recommit_decoders(struct pci_dev *dport_pci,
> + struct xarray *hdm_state)
> {
> struct cxl_dport *dport;
> int rc;
> @@ -179,3 +179,141 @@ void cxl_sbr_recommit_decoders(struct pci_dev *dport_pci,
> if (rc)
> pci_warn(dport_pci, "HDM decode restore failed: %d\n", rc);
> }
> +
> +/*
> + * The HDM decoder control registers the reset is about to clear, held from the
> + * disable to the enable of one Downstream Port and indexed by that Port's
> + * struct pci_dev, so resets of different Ports do not share an entry.
> + */
> +static DEFINE_XARRAY(cxl_sbr_hdm_state);
> +
> +static void cxl_sbr_drop_hdm_state(struct pci_dev *dport_pci)
> +{
> + struct xarray *hdm_state;
> +
> + hdm_state = xa_erase(&cxl_sbr_hdm_state, (unsigned long)dport_pci);
> + if (!hdm_state)
> + return;
> +
> + cxl_port_put_hdm_state(hdm_state);
> + kfree(hdm_state);
> +}
> +
> +/*
> + * Record the control registers of every port below @dport_pci before the reset
> + * clears them. cxl_sbr_enable_regions() consumes the set and drops it.
> + */
> +static int cxl_sbr_save_hdm_state(struct pci_dev *dport_pci)
> +{
> + struct xarray *hdm_state;
> + struct cxl_dport *dport;
> + int rc;
> +
> + struct cxl_port *port __free(put_cxl_port) =
> + find_cxl_port(&dport_pci->dev, &dport);
> + if (!port)
> + return 0;
> +
> + hdm_state = kzalloc_obj(*hdm_state);
> + if (!hdm_state)
> + return -ENOMEM;
> +
> + xa_init(hdm_state);
> +
> + scoped_guard(rwsem_read, &cxl_rwsem.region)
> + rc = cxl_port_save_hdm_state(port, hdm_state);
> +
> + if (!rc)
> + rc = xa_insert(&cxl_sbr_hdm_state, (unsigned long)dport_pci,
> + hdm_state, GFP_KERNEL);
> + if (rc) {
> + cxl_port_put_hdm_state(hdm_state);
> + kfree(hdm_state);
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Disable the regions routed through the Downstream Port being reset. On
> + * failure re-enable the regions already disabled and return the error so the
> + * PCI core aborts the reset with the topology unchanged.
> + */
> +static int cxl_sbr_disable_regions(struct pci_dev *dport_pci)
> +{
> + struct cxl_region *cxlr;
> + struct xarray regions;
> + unsigned long index;
> + int rc;
> +
> + rc = cxl_sbr_save_hdm_state(dport_pci);
> + if (rc)
> + return rc;
> +
> + xa_init(®ions);
> +
> + rc = cxl_sbr_collect_regions(dport_pci, ®ions);
> + if (rc)
> + goto out;
> +
> + xa_for_each(®ions, index, cxlr) {
> + rc = cxl_region_disable(cxlr);
> + if (rc)
> + break;
> + }
> +
> + /*
> + * On failure restore every collected region and return the error so the
> + * PCI core aborts the reset before touching the hardware. Re-enabling a
> + * region left untouched is a no-op, so enabling the whole set also
> + * recovers the region whose offline failed midway.
> + */
> + if (rc) {
> + dev_dbg(&dport_pci->dev, "%s: disable failed (%d), re-enabling collected regions and aborting reset\n",
> + __func__, rc);
> + xa_for_each(®ions, index, cxlr)
> + cxl_region_enable(cxlr);
> + }
> +
> +out:
> + cxl_sbr_put_regions(®ions);
> + /* No enable_regions() call follows an aborted reset, so drop the set. */
> + if (rc)
> + cxl_sbr_drop_hdm_state(dport_pci);
> + return rc;
> +}
> +
> +/*
> + * Re-enable the regions disabled by cxl_sbr_disable_regions(). Restore the HDM
> + * decode first: a region cannot serve memory through decoders that are not
> + * programmed, so its driver must not re-attach before they are.
> + */
> +static void cxl_sbr_enable_regions(struct pci_dev *dport_pci)
> +{
> + struct xarray *hdm_state;
> + struct cxl_region *cxlr;
> + struct xarray regions;
> + unsigned long index;
> +
> + xa_init(®ions);
> +
> + cxl_sbr_collect_regions(dport_pci, ®ions);
> +
> + hdm_state = xa_load(&cxl_sbr_hdm_state, (unsigned long)dport_pci);
> + if (hdm_state)
> + cxl_sbr_recommit_decoders(dport_pci, hdm_state);
> + else
> + pci_warn(dport_pci, "no saved HDM state, decode not restored\n");
> +
> + xa_for_each(®ions, index, cxlr)
> + cxl_region_enable(cxlr);
> +
> + cxl_sbr_put_regions(®ions);
> + cxl_sbr_drop_hdm_state(dport_pci);
> +}
> +
> +const struct pci_cxl_sbr_region_ops cxl_sbr_region_ops = {
> + .disable_regions = cxl_sbr_disable_regions,
> + .enable_regions = cxl_sbr_enable_regions,
> +};
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index fc0bec991a69..d1dd4924fba1 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -12,6 +12,7 @@
> #include <linux/idr.h>
> #include <linux/memory-tiers.h>
> #include <linux/string_choices.h>
> +#include <linux/pci.h>
> #include <cxlmem.h>
> #include <cxl.h>
> #include "core.h"
> @@ -4263,12 +4264,14 @@ static struct cxl_driver cxl_region_driver = {
>
> int cxl_region_init(void)
> {
> + pci_cxl_set_sbr_region_ops(&cxl_sbr_region_ops);
> return cxl_driver_register(&cxl_region_driver);
> }
>
> void cxl_region_exit(void)
> {
> cxl_driver_unregister(&cxl_region_driver);
> + pci_cxl_set_sbr_region_ops(NULL);
> }
>
> MODULE_IMPORT_NS("CXL");
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 77b17b13ee61..417b6b44473e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4844,6 +4844,19 @@ void __weak pcibios_reset_secondary_bus(struct pci_dev *dev)
> pci_reset_secondary_bus(dev);
> }
>
> +/*
> + * Registered by the CXL core to disable and re-enable the regions mapped
> + * through a CXL Downstream Port across a Secondary Bus Reset. NULL whenever
> + * the CXL region code is absent: not built, or built as a module not loaded.
> + */
> +static const struct pci_cxl_sbr_region_ops *cxl_sbr_region_ops;
> +
> +void pci_cxl_set_sbr_region_ops(const struct pci_cxl_sbr_region_ops *ops)
> +{
> + cxl_sbr_region_ops = ops;
> +}
> +EXPORT_SYMBOL_GPL(pci_cxl_set_sbr_region_ops);
> +
Is the lifetime of this callback table being protected ?
cxl_core can be a module, PCI reads the pointer without sync or a module ref.
It could see a non-NULL pointer.
I think the callback table registration needs synchronization and module ownership. PCI should pin
the provider for the complete reset, for example with an owner field. Unregistration should first
stop new users and then wait for any reset already using the callbacks.
What do you think about this ?
Best regards,
Richard Cheng.
> /**
> * pci_bridge_secondary_bus_reset - Reset the secondary bus on a PCI bridge.
> * @dev: Bridge device
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 64b308b6e61c..2feb0e355305 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1606,6 +1606,18 @@ int devm_request_pci_bus_resources(struct device *dev,
> /* Temporary until new and working PCI SBR API in place */
> int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
>
> +/**
> + * struct pci_cxl_sbr_region_ops - CXL region callbacks for a bus reset
> + * @disable_regions: disable the regions below @dport, 0 or errno
> + * @enable_regions: re-enable the regions below @dport
> + */
> +struct pci_cxl_sbr_region_ops {
> + int (*disable_regions)(struct pci_dev *dport);
> + void (*enable_regions)(struct pci_dev *dport);
> +};
> +
> +void pci_cxl_set_sbr_region_ops(const struct pci_cxl_sbr_region_ops *ops);
> +
> #define __pci_bus_for_each_res0(bus, res, ...) \
> for (unsigned int __b = 0; \
> (res = pci_bus_resource_n(bus, __b)) || __b < PCI_BRIDGE_RESOURCE_NUM; \
> --
> 2.55.0
>
>