Re: [PATCH v2 11/13] PCI/DPC: Unbind regions for DPC recovery
From: Richard Cheng
Date: Wed Aug 26 2026 - 04:55:29 EST
On Tue, Aug 25, 2026 at 04:26:26AM +0800, Fabio M. De Francesco wrote:
> CXL r4.0 sec 8.1.5.1 lists Downstream Port Containment among the
> conditions that reset the Downstream Component's bus number. DPC
> recovery needs to wait for the link to come back up, but if Access
> Control Services Source Validation bit is enables PM Initialization will
> fail to complete.
>
> Have dpc_reset_link() reset the secondary bus of a CXL Downstream Port.
> Split pci_bridge_secondary_bus_reset() so the reset takes an action
> saying what to do with the CXL regions routed through the Port, and pass
> the action that unbinds them without offlining their memory. Offlining
> migrates the memory that the memdev back and on a contained link are not
> addressable. Don't release the memdev driver in cxl_error_detected().
>
Hi Fabio,
Do we have any mechanism to prevent CPU or DMA devices from accessing the System RAM
while the link and HDM decoder are unavailable ?
Unbinding the region only tops region managment, it doesn't stop CPU or DMA memory traffic
that's still online.
Unbinding also removes DAX/kmem device while its memory is online.
dev_dax_kmem_remove() can't remove that memory in this state and will leave the resource reserved
until reboot, so the region may not rebind afterward.
Also while DPC link is down, cxl_sbr_save_hdm-state() can't safely read downstream HDM register
through MMIO, I am not sure whether it's correct to save HDM state here.
Best regards,
Richard Cheng.
> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@xxxxxxxxxxxxxxx>
> ---
> drivers/cxl/core/dport_sbr.c | 57 ++++++++++++++++++++++++++++++++-
> drivers/cxl/core/ras.c | 10 ++++++
> drivers/pci/pci.c | 62 ++++++++++++++++++++++++++----------
> drivers/pci/pci.h | 15 +++++++++
> drivers/pci/pcie/dpc.c | 46 +++++++++++++++++++++++++-
> include/linux/aer.h | 9 ++++++
> include/linux/pci.h | 3 ++
> 7 files changed, 183 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/cxl/core/dport_sbr.c b/drivers/cxl/core/dport_sbr.c
> index 2b6f840e22a7..823b63012f45 100644
> --- a/drivers/cxl/core/dport_sbr.c
> +++ b/drivers/cxl/core/dport_sbr.c
> @@ -9,6 +9,30 @@
> #include <cxl.h>
> #include "core.h"
>
> +/*
> + * cxl_region_unbind - take a region out of service ahead of a reset
> + * @cxlr: region routed through the CXL Downstream Port being reset
> + *
> + * Unbind the region driver, which tears down everything built on the region:
> + * the dax region device, its dax device and the driver bound to it. An SBR
> + * zeroes the downstream bus number, so a region left bound would decode to a
> + * device in reset.
> + *
> + * The memory the region hosts is left as it is. A caller that reaches a live
> + * device offlines it first; see cxl_region_disable().
> + *
> + * Context: process context. Driver unbind sleeps, so this cannot run in atomic
> + * context.
> + */
> +static void cxl_region_unbind(struct cxl_region *cxlr)
> +{
> + struct cxl_region_params *p = &cxlr->params;
> +
> + device_release_driver(&cxlr->dev);
> + dev_dbg(&cxlr->dev, "%s: region unbound before reset, HPA %pr\n",
> + __func__, p->res);
> +}
> +
> /*
> * cxl_region_disable - make a region inactive ahead of a Secondary Bus Reset
> * @cxlr: region routed through the CXL Downstream Port being reset
> @@ -58,7 +82,7 @@ static int cxl_region_disable(struct cxl_region *cxlr)
> return rc;
> }
>
> - device_release_driver(&cxlr->dev);
> + cxl_region_unbind(cxlr);
> dev_dbg(&cxlr->dev, "%s: System RAM offline, region disabled before reset, HPA %pr\n",
> __func__, p->res);
>
> @@ -284,6 +308,36 @@ static int cxl_sbr_disable_regions(struct pci_dev *dport_pci)
> return rc;
> }
>
> +/*
> + * Unbind the regions routed through the Downstream Port being reset, leaving
> + * their memory online. Used on the DPC recovery path, where dpc_reset_link()
> + * clears DPC Trigger Status and enters the reset without waiting for the link,
> + * so the device may still be unreachable and the page migration that an offline
> + * performs would have no device to read from.
> + *
> + * Unbinding cannot fail, so unlike cxl_sbr_disable_regions() this never aborts
> + * the reset. The memory stays online across the reset with no region decoding
> + * it; cxl_sbr_enable_regions() reprograms the decoders on the way out.
> + */
> +static void cxl_sbr_unbind_regions(struct pci_dev *dport_pci)
> +{
> + struct cxl_region *cxlr;
> + struct xarray regions;
> + unsigned long index;
> +
> + if (cxl_sbr_save_hdm_state(dport_pci))
> + pci_warn(dport_pci, "HDM state not saved, decode will not be restored\n");
> +
> + xa_init(®ions);
> +
> + cxl_sbr_collect_regions(dport_pci, ®ions);
> +
> + xa_for_each(®ions, index, cxlr)
> + cxl_region_unbind(cxlr);
> +
> + cxl_sbr_put_regions(®ions);
> +}
> +
> /*
> * 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
> @@ -315,5 +369,6 @@ static void cxl_sbr_enable_regions(struct pci_dev *dport_pci)
>
> const struct pci_cxl_sbr_region_ops cxl_sbr_region_ops = {
> .disable_regions = cxl_sbr_disable_regions,
> + .unbind_regions = cxl_sbr_unbind_regions,
> .enable_regions = cxl_sbr_enable_regions,
> };
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index 99fb00949c2f..568a906f3b50 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -323,6 +323,16 @@ pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
> }
> return PCI_ERS_RESULT_CAN_RECOVER;
> case pci_channel_io_frozen:
> + /*
> + * A Port on the path in DPC means dpc_reset_link() is about to
> + * reset the link, and that path takes the CXL regions out of
> + * service and restores the HDM decode itself. Keep the memdev
> + * driver bound so the endpoint and its decoders are still there
> + * to restore.
> + */
> + if (pci_dpc_containment_active(pdev))
> + return PCI_ERS_RESULT_NEED_RESET;
> +
> dev_warn(&pdev->dev,
> "%s: frozen state error detected, disable CXL.mem\n",
> dev_name(dev));
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index f3781d1e8f6e..eedd516f8484 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4864,24 +4864,37 @@ struct cxl_sbr_ctx {
> u16 command;
> };
>
> -static bool is_cxl_dport(struct pci_dev *dev)
> +bool is_cxl_dport(struct pci_dev *dev)
> {
> return pcie_is_cxl(dev) && pcie_downstream_port(dev);
> }
>
> -static u16 cxl_port_dvsec(struct pci_dev *dev)
> +u16 cxl_port_dvsec(struct pci_dev *dev)
> {
> return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
> PCI_DVSEC_CXL_PORT);
> }
>
> static int cxl_sbr_prepare(struct pci_dev *bridge, u16 dvsec,
> - struct cxl_sbr_ctx *ctx)
> + struct cxl_sbr_ctx *ctx,
> + enum cxl_sbr_region_action action)
> {
> int rc;
>
> - /* Abort before touching hardware if the regions cannot be disabled. */
> - if (cxl_sbr_region_ops) {
> + /*
> + * CXL_SBR_UNBIND: the link is already down, so offlining the regions'
> + * memory would take the reads that page migration performs as a machine
> + * check. Per PCIe r7.0 sec 2.9.3 the Port answers a Non-Posted Request
> + * with an Unsupported Request or Completer Abort completion while it is
> + * in DPC. Unbinding never fails, so the reset always goes ahead.
> + *
> + * CXL_SBR_OFFLINE_AND_UNBIND: the device is reachable, so offline the
> + * memory first and abort the reset before touching hardware if that
> + * fails.
> + */
> + if (cxl_sbr_region_ops && action == CXL_SBR_UNBIND) {
> + cxl_sbr_region_ops->unbind_regions(bridge);
> + } else if (cxl_sbr_region_ops) {
> rc = cxl_sbr_region_ops->disable_regions(bridge);
> if (rc)
> return rc;
> @@ -5003,19 +5016,17 @@ static void cxl_sbr_complete(struct pci_dev *bridge, u16 dvsec,
> cxl_sbr_region_ops->enable_regions(bridge);
> }
>
> -/**
> - * pci_bridge_secondary_bus_reset - Reset the secondary bus on a PCI bridge.
> - * @dev: Bridge device
> - *
> - * Use the bridge control register to assert reset on the secondary bus.
> - * Devices on the secondary bus are left in power-on state.
> +/*
> + * __pci_bridge_secondary_bus_reset - assert Secondary Bus Reset on a bridge
> + * @dev: bridge device
> + * @action: what to do with the CXL regions reached through @dev
> *
> - * When @dev is a CXL Downstream Port, clear ACS Source Validation and Bus
> - * Master Enable across the reset, per the workaround in CXL r4.0 sec 8.1.5.1,
> - * so that Port Power Management Initialization completes at link-up. The
> - * bits stay cleared until the secondary bus is back, then are restored.
> + * See pci_bridge_secondary_bus_reset(). Pass CXL_SBR_UNBIND when the link is
> + * already down, which leaves the regions' memory online because offlining it
> + * needs a reachable device.
> */
> -int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
> +int __pci_bridge_secondary_bus_reset(struct pci_dev *dev,
> + enum cxl_sbr_region_action action)
> {
> struct cxl_sbr_ctx ctx = {};
> u16 dvsec = 0;
> @@ -5028,7 +5039,7 @@ int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
> if (is_cxl_dport(dev))
> dvsec = cxl_port_dvsec(dev);
> if (dvsec) {
> - rc = cxl_sbr_prepare(dev, dvsec, &ctx);
> + rc = cxl_sbr_prepare(dev, dvsec, &ctx, action);
> if (rc)
> return rc;
> }
> @@ -5042,6 +5053,23 @@ int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
>
> return rc;
> }
> +
> +/**
> + * pci_bridge_secondary_bus_reset - Reset the secondary bus on a PCI bridge.
> + * @dev: Bridge device
> + *
> + * Use the bridge control register to assert reset on the secondary bus.
> + * Devices on the secondary bus are left in power-on state.
> + *
> + * When @dev is a CXL Downstream Port, clear ACS Source Validation and Bus
> + * Master Enable across the reset, per the workaround in CXL r4.0 sec 8.1.5.1,
> + * so that Port Power Management Initialization completes at link-up. The
> + * bits stay cleared until the secondary bus is back, then are restored.
> + */
> +int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
> +{
> + return __pci_bridge_secondary_bus_reset(dev, CXL_SBR_OFFLINE_AND_UNBIND);
> +}
> EXPORT_SYMBOL_GPL(pci_bridge_secondary_bus_reset);
>
> static int pci_parent_bus_reset(struct pci_dev *dev, bool probe)
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 4469e1a77f3c..b6d873b077ed 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -233,6 +233,21 @@ int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vmai,
> bool pci_reset_supported(struct pci_dev *dev);
> void pci_init_reset_methods(struct pci_dev *dev);
> int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
> +
> +/*
> + * What to do with the CXL regions reached through a Downstream Port before it
> + * is reset. Offlining their memory needs a reachable device, so a Port whose
> + * link is already down only unbinds and leaves the memory online.
> + */
> +enum cxl_sbr_region_action {
> + CXL_SBR_OFFLINE_AND_UNBIND,
> + CXL_SBR_UNBIND,
> +};
> +
> +int __pci_bridge_secondary_bus_reset(struct pci_dev *dev,
> + enum cxl_sbr_region_action action);
> +bool is_cxl_dport(struct pci_dev *dev);
> +u16 cxl_port_dvsec(struct pci_dev *dev);
> int pci_bus_error_reset(struct pci_dev *dev);
> int pci_try_reset_bridge(struct pci_dev *bridge);
>
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 2b779bd1d861..ad74086bf82c 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -127,6 +127,44 @@ bool pci_dpc_recovered(struct pci_dev *pdev)
> }
> #endif /* CONFIG_HOTPLUG_PCI_PCIE */
>
> +/**
> + * pci_dpc_containment_active - whether a Port above @pdev is contained by DPC
> + * @pdev: PCI device below the Port
> + *
> + * Per PCIe r7.0 sec 2.9.3 the Port's LTSSM stays in the Disabled state while
> + * DPC Trigger Status is set, and dpc_reset_link() clears that bit only after
> + * pcie_do_recovery() has broadcast error_detected. A ->error_detected()
> + * callback can therefore use this to tell a DPC containment from any other
> + * frozen-channel error, and to know that the link is about to be reset.
> + *
> + * The Port that triggered is on the path to @pdev, because the broadcast walks
> + * that Port's subordinate bus, so test every bridge above @pdev.
> + *
> + * Return: true if a Port on the path to @pdev has DPC Trigger Status set.
> + */
> +bool pci_dpc_containment_active(struct pci_dev *pdev)
> +{
> + struct pci_dev *bridge;
> +
> + for (bridge = pci_upstream_bridge(pdev); bridge;
> + bridge = pci_upstream_bridge(bridge)) {
> + u16 status;
> +
> + if (!bridge->dpc_cap)
> + continue;
> +
> + pci_read_config_word(bridge,
> + bridge->dpc_cap + PCI_EXP_DPC_STATUS,
> + &status);
> + if (!PCI_POSSIBLE_ERROR(status) &&
> + (status & PCI_EXP_DPC_STATUS_TRIGGER))
> + return true;
> + }
> +
> + return false;
> +}
> +EXPORT_SYMBOL_GPL(pci_dpc_containment_active);
> +
> static int dpc_wait_rp_inactive(struct pci_dev *pdev)
> {
> unsigned long timeout = jiffies + HZ;
> @@ -149,6 +187,7 @@ pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
> {
> pci_ers_result_t ret;
> u16 cap;
> + int rc;
>
> set_bit(PCI_DPC_RECOVERING, &pdev->priv_flags);
>
> @@ -174,7 +213,12 @@ pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
> pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
> PCI_EXP_DPC_STATUS_TRIGGER);
>
> - if (pci_bridge_wait_for_secondary_bus(pdev, "DPC")) {
> + if (is_cxl_dport(pdev) && cxl_port_dvsec(pdev))
> + rc = __pci_bridge_secondary_bus_reset(pdev, CXL_SBR_UNBIND);
> + else
> + rc = pci_bridge_wait_for_secondary_bus(pdev, "DPC");
> +
> + if (rc) {
> clear_bit(PCI_DPC_RECOVERED, &pdev->priv_flags);
> ret = PCI_ERS_RESULT_DISCONNECT;
> } else {
> diff --git a/include/linux/aer.h b/include/linux/aer.h
> index df0f5c382286..7ac029f01c25 100644
> --- a/include/linux/aer.h
> +++ b/include/linux/aer.h
> @@ -66,6 +66,15 @@ static inline int pcie_aer_is_native(struct pci_dev *dev) { return 0; }
> static inline void pci_aer_unmask_internal_errors(struct pci_dev *dev) { }
> #endif
>
> +#if defined(CONFIG_PCIE_DPC)
> +bool pci_dpc_containment_active(struct pci_dev *pdev);
> +#else
> +static inline bool pci_dpc_containment_active(struct pci_dev *pdev)
> +{
> + return false;
> +}
> +#endif
> +
> void pci_print_aer(struct pci_dev *dev, int aer_severity,
> struct aer_capability_regs *aer);
> int cper_severity_to_aer(int cper_severity);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2feb0e355305..0d9832ce6f3d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1609,10 +1609,13 @@ 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
> + * @unbind_regions: unbind the drivers of the regions below @dport, leaving
> + * their memory online, for a link already contained
> * @enable_regions: re-enable the regions below @dport
> */
> struct pci_cxl_sbr_region_ops {
> int (*disable_regions)(struct pci_dev *dport);
> + void (*unbind_regions)(struct pci_dev *dport);
> void (*enable_regions)(struct pci_dev *dport);
> };
>
> --
> 2.55.0
>
>