[PATCH v2 00/13] Make SBR work for CXL Downstream Ports

[PATCH v2 00/13] Make SBR work for CXL Downstream Ports

[Fabio M. De Francesco]

A Secondary Bus Reset (SBR), a Link Down or a Downstream Port
Containment (DPC) event on a CXL Downstream Port clears the Downstream
Component's captured Bus Number. CXL r4.0 sec 8.1.5.1 notes that, if the
Component then sources its Power Management (PM) Initialization traffic
with Requester Bus = 0, a Port with Access Control Services (ACS) Source
Validation (SV) enabled may reject the Component's CREDIT_RTN IP2PM
message, PM Initialization may fail to complete, and that "another
Secondary Bus Reset alone will not facilitate recovery". PCIe r7.0 sec
6.12.1.1 makes the rejection an error that is reported as ACS Violation.

The Implementation Note under sec 8.1.5.1 gives the sequence that avoids
it: save and clear Bus Master Enable and ACS SV, generate the SBR, wait
for Port PM Initialization Complete, restore both bits, continue device
re-initialization. This series takes a step further to a preventive
measure that avoids the ACS Violation in the first place, not after
failure.

Patches 1-5 restore HDM Decoder registers below a reset CXL Downstream
Port.

Patches 6-8 disable and re-enable the CXL Regions around the reset:
offline memory, invalidate CPU caches for its range, collect the set of
regions the CXL DPort which is SBR andunbind the driver.

Patch 9 disable and re-enable CXL regions across a Downstream Port reset
and registers the CXL region callbacks for a Downstream Port bus. We
want the reset to be called only when the CXL driver is loaded.

Patch 10 calls the CXL operations from pci_bridge_secondary_bus_reset().

Patch 11 issues the same sequence from dpc_reset_link().

Patches 12 and 13 gives root a sysfs attribute that conrols whether the
SBR is allowed to be unmasked and prevents kernel from unmasking SBR on
its own.

What user space finds after an SBR of a CXL Downstream Port: the dax
device returns under the same name, the region's HPA range, interleave
geometry and decoder programming are restored, and the media content is
unchanged for PMEM. System RAM comes back registered but offline.

Fabio M. De Francesco (13):
  cxl/pci: Make the HDM and Mem_Enable writes callable from cxl_core
  cxl/hdm: Add cxl_decoder_recommit() to restore one HDM decoder
  cxl/hdm: Add cxl_endpoint_enable_hdm_decode() to restore CXL.mem
    decode
  cxl/hdm: Reprogram the HDM Decoders below a CXL Port
  cxl/core: Restore the HDM decoders below a reset Downstream Port
  drivers/base/memory: Add cxl_offline_memory() to offline a physical
    range
  cxl/core: Add region disable and enable for a Downstream Port reset
  cxl/core: Collect the CXL regions routed through a Downstream Port
  PCI/CXL: Register CXL region callbacks for a Downstream Port bus reset
  PCI/CXL: Clear ACS SV across an SBR of a CXL Downstream Port
  PCI/DPC: Issue the CXL recovery SBR from DPC without offlining memory
  PCI/CXL: Add cxl_unmask_sbr to permit an SBR of a CXL Downstream Port
  PCI/CXL: Refuse an SBR of a CXL Downstream Port without the unmask
    consent

 Documentation/ABI/testing/sysfs-bus-pci |  16 +
 drivers/base/memory.c                   |  36 +++
 drivers/cxl/core/Makefile               |   2 +-
 drivers/cxl/core/core.h                 |  28 ++
 drivers/cxl/core/dport_sbr.c            | 374 ++++++++++++++++++++++++
 drivers/cxl/core/hdm.c                  | 341 +++++++++++++++++++++
 drivers/cxl/core/pci.c                  |  19 +-
 drivers/cxl/core/port.c                 |   4 +-
 drivers/cxl/core/ras.c                  |  10 +
 drivers/cxl/core/region.c               |   6 +-
 drivers/pci/pci-sysfs.c                 |  31 ++
 drivers/pci/pci.c                       | 254 +++++++++++++++-
 drivers/pci/pci.h                       |  15 +
 drivers/pci/pcie/dpc.c                  |  46 ++-
 include/linux/aer.h                     |   9 +
 include/linux/memory.h                  |   5 +
 include/linux/pci.h                     |  16 +
 include/uapi/linux/pci_regs.h           |   2 +
 tools/testing/cxl/Kbuild                |   2 +-
 19 files changed, 1193 insertions(+), 23 deletions(-)
 create mode 100644 drivers/cxl/core/dport_sbr.c

-- 
2.55.0

[PATCH v2 01/13] cxl/pci: Make the HDM and Mem_Enable writes callable from CXL

[Fabio M. De Francesco]

Split the Global Control write out of devm_cxl_enable_hdm() as
cxl_enable_hdm() and drop the static from cxl_set_mem_enable(), so both
are reachable from the rest of cxl_core. Declare them in core.h.

Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
---
 drivers/cxl/core/core.h |  2 ++
 drivers/cxl/core/pci.c  | 19 ++++++++++++++-----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 07555ae63859..4dc324f019ab 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -213,6 +213,8 @@ int cxl_gpf_port_setup(struct cxl_dport *dport);
 struct cxl_hdm;
 int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 			struct cxl_endpoint_dvsec_info *info);
+void cxl_enable_hdm(struct cxl_hdm *cxlhdm, u32 global_ctrl);
+int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val);
 int cxl_port_get_possible_dports(struct cxl_port *port);
 
 #ifdef CONFIG_CXL_FEATURES
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index e4338fd7e01b..a7a2b84293e9 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -179,7 +179,7 @@ int cxl_await_media_ready(struct cxl_dev_state *cxlds)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_await_media_ready, "CXL");
 
-static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val)
+int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val)
 {
 	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
 	int d = cxlds->cxl_dvsec;
@@ -247,14 +247,23 @@ static void disable_hdm(void *_cxlhdm)
 	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
 }
 
-static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm)
+/*
+ * @global_ctrl is the CXL HDM Decoder Global Control value to enable decode in.
+ * A caller restoring decode after a reset passes the value it saved, so the
+ * fields the driver does not model are not left at their reset defaults.
+ */
+void cxl_enable_hdm(struct cxl_hdm *cxlhdm, u32 global_ctrl)
 {
 	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
-	u32 global_ctrl;
 
-	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
 	writel(global_ctrl | CXL_HDM_DECODER_ENABLE,
 	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
+}
+
+static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm,
+			       u32 global_ctrl)
+{
+	cxl_enable_hdm(cxlhdm, global_ctrl);
 
 	return devm_add_action_or_reset(host, disable_hdm, cxlhdm);
 }
@@ -398,7 +407,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	 * enable and use the HDM Decoder Capability registers.
 	 */
 	if (!info->mem_enabled) {
-		rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
+		rc = devm_cxl_enable_hdm(&port->dev, cxlhdm, global_ctrl);
 		if (rc)
 			return rc;
 
-- 
2.55.0

[PATCH v2 02/13] cxl/hdm: Add function to restore one HDM decoder

[Fabio M. De Francesco]

Add cxl_decoder_recommit() to restore one HDM decoder that the driver
holds as committed but hardware no longer reports Committed, rewriting the
cached settings and committing it again.

Restoring the decoders below a reset Port means reprogramming each one
along the path to every affected endpoint. cxl_decoder_recommit() does that
for a single decoder, so it will be reused later for the full path.

Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
---
 drivers/cxl/core/core.h |  1 +
 drivers/cxl/core/hdm.c  | 71 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 4dc324f019ab..6d536fe7b446 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -215,6 +215,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 			struct cxl_endpoint_dvsec_info *info);
 void cxl_enable_hdm(struct cxl_hdm *cxlhdm, u32 global_ctrl);
 int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val);
+int cxl_decoder_recommit(struct cxl_decoder *cxld, u32 ctrl);
 int cxl_port_get_possible_dports(struct cxl_port *port);
 
 #ifdef CONFIG_CXL_FEATURES
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 0c80b76a5f9b..d55d764895f8 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -849,6 +849,77 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
 	return 0;
 }
 
+/**
+ * cxl_decoder_recommit - reprogram @cxld's HDM decoder registers and commit
+ * @cxld: decoder to reprogram from its cached settings
+ * @ctrl: CXL HDM Decoder n Control value to restore under the cached settings
+ *
+ * A reset of an upstream link clears the HDM decoder registers of every
+ * component below it, dropping Committed while the driver still holds the
+ * settings that were in effect. Restore those settings and commit.
+ *
+ * setup_hw_decoder() rewrites only Interleave Granularity, Interleave Ways and
+ * Target Range Type, so the rest of the control register would come from a read
+ * of the reset defaults. Per CXL r4.0 sec 8.2.4.20.7 Table 8-123 that register
+ * also holds BI, UIO, Upstream Interleave Granularity, Upstream Interleave Ways
+ * and Lock On Commit, none of which the driver models, and sec 8.2.4.20.12 makes
+ * device operation undefined if a device that requires BI is committed without
+ * it. Write @ctrl first so those fields are in place, with Commit masked off
+ * until setup_hw_decoder() has written the range.
+ *
+ * A decoder that hardware still reports Committed kept its programming across
+ * the reset and needs no work. A decoder with no ->commit is driven through the
+ * DVSEC ranges or is a passthrough decoder, and has no registers to program.
+ *
+ * Section 8.2.4.20.13 requires the traffic targeting @cxld to be quiesced while
+ * it is reprogrammed, which the caller owns. @cxld decodes nothing until the
+ * commit completes.
+ *
+ * Return: 0 on success or if @cxld needs no reprogramming, negative errno if the
+ * commit times out or if the hardware reports a commit error.
+ */
+int cxl_decoder_recommit(struct cxl_decoder *cxld, u32 ctrl)
+{
+	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
+	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
+	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
+	u32 hw_ctrl;
+	int rc;
+
+	if ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)
+		return 0;
+
+	if (!cxld->commit)
+		return 0;
+
+	hw_ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
+	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, hw_ctrl)) {
+		dev_dbg(&cxld->dev, "%s: still committed, no reprogram needed\n",
+			__func__);
+		return 0;
+	}
+
+	writel(ctrl & ~(CXL_HDM_DECODER0_CTRL_COMMIT |
+			CXL_HDM_DECODER0_CTRL_COMMITTED |
+			CXL_HDM_DECODER0_CTRL_COMMIT_ERROR),
+	       hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
+
+	scoped_guard(rwsem_read, &cxl_rwsem.dpa)
+		setup_hw_decoder(cxld, hdm);
+
+	rc = cxld_await_commit(hdm, cxld->id);
+	if (rc) {
+		dev_warn(&cxld->dev, "%s: failed to commit decoder: %d\n",
+			 __func__, rc);
+		return rc;
+	}
+
+	dev_dbg(&cxld->dev, "%s: reprogrammed HPA %#llx-%#llx\n",
+		__func__, cxld->hpa_range.start, cxld->hpa_range.end);
+
+	return 0;
+}
+
 static int commit_reap(struct device *dev, void *data)
 {
 	struct cxl_port *port = to_cxl_port(dev->parent);
-- 
2.55.0

[PATCH v2 03/13] cxl/hdm: Add function to restore CXL.mem decode

[Fabio M. De Francesco]

A reset of a CXL Downstream Port clears HDM Decoder Enable in the CXL HDM
Decoder Global Control register of every endpoint below it, and Mem_Enable
in their DVSEC CXL Control register. Per CXL r4.0 sec 8.2.4.20.2
Table 8-118 a device decodes CXL.mem through the DVSEC range registers
while HDM Decoder Enable is clear, so a decoder committed in that state
does not establish the route, and per sec 8.1.3.2 Table 8-6 a component
with Mem_Enable clear may silently drop all CXL.mem transactions.

Add cxl_endpoint_enable_hdm_decode() to put an endpoint back in HDM decode
before its decoders are committed again. It takes the Global Control value
to enable decode in, since that register also holds Poison On Decode Error
Enable, which the driver does not model and which a read taken after the
reset returns at its default.

Leave alone an endpoint that decodes through the DVSEC ranges rather than
the HDM decoders, either because it has no HDM decoder registers or
because its decoders are emulated from those ranges. Setting HDM Decoder
Enable on such a device moves it off the ranges it is decoding through and
onto decoders that are locked against reprogramming, so it would decode
nothing.

Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
---
 drivers/cxl/core/core.h |  1 +
 drivers/cxl/core/hdm.c  | 52 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 6d536fe7b446..918d48f4422e 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -216,6 +216,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 void cxl_enable_hdm(struct cxl_hdm *cxlhdm, u32 global_ctrl);
 int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val);
 int cxl_decoder_recommit(struct cxl_decoder *cxld, u32 ctrl);
+int cxl_endpoint_enable_hdm_decode(struct cxl_memdev *cxlmd, u32 global_ctrl);
 int cxl_port_get_possible_dports(struct cxl_port *port);
 
 #ifdef CONFIG_CXL_FEATURES
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index d55d764895f8..7c898c60f60f 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -920,6 +920,58 @@ int cxl_decoder_recommit(struct cxl_decoder *cxld, u32 ctrl)
 	return 0;
 }
 
+static int __cxl_endpoint_decoder_is_emulated(struct device *dev, void *data)
+{
+	if (!is_endpoint_decoder(dev))
+		return 0;
+
+	return !to_cxl_decoder(dev)->commit;
+}
+
+/* Only the DVSEC setup path leaves ->commit NULL. */
+static bool cxl_endpoint_decoders_are_emulated(struct cxl_port *endpoint)
+{
+	return device_for_each_child(&endpoint->dev, NULL,
+				     __cxl_endpoint_decoder_is_emulated);
+}
+
+/*
+ * Restore CXL.mem decode on @cxlmd before any of its decoders is committed. A
+ * reset clears the endpoint's HDM Decoder Global Control and the DVSEC CXL
+ * Control, and per CXL r4.0 sec 8.2.4.20.2 Table 8-118 a device decodes CXL.mem
+ * with the DVSEC range registers while HDM Decoder Enable is clear. Committing
+ * a decoder in that state does not establish the route. An endpoint with no HDM
+ * decoder registers is driven through the DVSEC ranges and has nothing to
+ * enable. So is an endpoint whose decoders are emulated from those ranges, and
+ * setting HDM Decoder Enable there would switch it to decoders locked against
+ * reprogramming.
+ *
+ * @global_ctrl is the Global Control value to enable decode in. That register
+ * also holds Poison On Decode Error Enable, which the driver does not model, so
+ * the caller supplies the value it saved rather than one read back after the
+ * reset.
+ */
+int cxl_endpoint_enable_hdm_decode(struct cxl_memdev *cxlmd, u32 global_ctrl)
+{
+	struct cxl_port *endpoint = cxlmd->endpoint;
+	struct cxl_hdm *cxlhdm = dev_get_drvdata(&endpoint->dev);
+	int rc;
+
+	if (!cxlhdm || !cxlhdm->regs.hdm_decoder)
+		return 0;
+
+	if (cxl_endpoint_decoders_are_emulated(endpoint))
+		return 0;
+
+	cxl_enable_hdm(cxlhdm, global_ctrl);
+
+	rc = cxl_set_mem_enable(cxlmd->cxlds, PCI_DVSEC_CXL_MEM_ENABLE);
+	if (rc < 0)
+		return rc;
+
+	return 0;
+}
+
 static int commit_reap(struct device *dev, void *data)
 {
 	struct cxl_port *port = to_cxl_port(dev->parent);
-- 
2.55.0

[PATCH v2 04/13] cxl/hdm: Reprogram the HDM Decoders below a CXL Port

[Fabio M. De Francesco]

A reset of a CXL Port clears the HDM Decoders of every component below it,
so the decode has to be put back for every endpoint the Port routes to.

setup_hw_decoder() writes Interleave Granularity, Interleave Ways and
Target Range Type. Restoring a decoder needs the rest of its Control
register too, and the port's HDM Decoder Global Control register, so read
both before the reset and write them back after it.

Add cxl_port_recommit_decoders() to restore the decode of every endpoint
below a CXL Port, and cxl_port_save_hdm_state() to record those registers
beforehand, one struct cxl_hdm_state per port.

Commit each path from the endpoint upward, so a decoder starts claiming its
range only after the path below it is programmed. Within a port, commit the
decoders in instance order, which CXL r4.0 sec 8.2.4.20.13 requires during
reprogramming.

Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
---
 drivers/cxl/core/core.h |  22 +++-
 drivers/cxl/core/hdm.c  | 222 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 240 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 918d48f4422e..d23f0bbd7bf0 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -215,8 +215,26 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 			struct cxl_endpoint_dvsec_info *info);
 void cxl_enable_hdm(struct cxl_hdm *cxlhdm, u32 global_ctrl);
 int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val);
-int cxl_decoder_recommit(struct cxl_decoder *cxld, u32 ctrl);
-int cxl_endpoint_enable_hdm_decode(struct cxl_memdev *cxlmd, u32 global_ctrl);
+/**
+ * struct cxl_hdm_state - one CXL port's HDM decoder programming, saved
+ * @global_ctrl: CXL HDM Decoder Global Control
+ * @nr_ctrl: number of entries in @ctrl
+ * @ctrl: CXL HDM Decoder n Control, indexed by decoder id
+ *
+ * Holds the fields of those two registers that the driver does not model, read
+ * before a reset and written back after it. Instances are held in an xarray
+ * keyed by the &struct cxl_port they were read from.
+ */
+struct cxl_hdm_state {
+	u32 global_ctrl;
+	int nr_ctrl;
+	u32 ctrl[];
+};
+
+int cxl_port_save_hdm_state(struct cxl_port *port, struct xarray *hdm_state);
+void cxl_port_put_hdm_state(struct xarray *hdm_state);
+int cxl_port_recommit_decoders(struct cxl_port *port,
+			       struct xarray *hdm_state);
 int cxl_port_get_possible_dports(struct cxl_port *port);
 
 #ifdef CONFIG_CXL_FEATURES
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 7c898c60f60f..9839d2592d20 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -878,7 +878,7 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
  * Return: 0 on success or if @cxld needs no reprogramming, negative errno if the
  * commit times out or if the hardware reports a commit error.
  */
-int cxl_decoder_recommit(struct cxl_decoder *cxld, u32 ctrl)
+static int cxl_decoder_recommit(struct cxl_decoder *cxld, u32 ctrl)
 {
 	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
 	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
@@ -935,6 +935,42 @@ static bool cxl_endpoint_decoders_are_emulated(struct cxl_port *endpoint)
 				     __cxl_endpoint_decoder_is_emulated);
 }
 
+struct cxl_recommit_ctx {
+	const struct cxl_hdm_state *state;
+	int *first_rc;
+};
+
+static int __cxl_port_recommit_decoder(struct device *dev, void *data)
+{
+	struct cxl_recommit_ctx *ctx = data;
+	struct cxl_decoder *cxld;
+	int rc;
+
+	if (!(is_switch_decoder(dev) || is_endpoint_decoder(dev)))
+		return 0;
+
+	cxld = to_cxl_decoder(dev);
+
+	if (cxld->id >= ctx->state->nr_ctrl) {
+		dev_warn(&cxld->dev, "%s: no saved control register\n",
+			 __func__);
+		if (!*ctx->first_rc)
+			*ctx->first_rc = -ENODATA;
+		return 0;
+	}
+
+	/*
+	 * Reprogram every decoder the walk reaches. Stopping at the first
+	 * failure would leave the rest of the path decoding nothing, so record
+	 * the first error and continue.
+	 */
+	rc = cxl_decoder_recommit(cxld, ctx->state->ctrl[cxld->id]);
+	if (rc && !*ctx->first_rc)
+		*ctx->first_rc = rc;
+
+	return 0;
+}
+
 /*
  * Restore CXL.mem decode on @cxlmd before any of its decoders is committed. A
  * reset clears the endpoint's HDM Decoder Global Control and the DVSEC CXL
@@ -951,7 +987,8 @@ static bool cxl_endpoint_decoders_are_emulated(struct cxl_port *endpoint)
  * the caller supplies the value it saved rather than one read back after the
  * reset.
  */
-int cxl_endpoint_enable_hdm_decode(struct cxl_memdev *cxlmd, u32 global_ctrl)
+static int cxl_endpoint_enable_hdm_decode(struct cxl_memdev *cxlmd,
+					  u32 global_ctrl)
 {
 	struct cxl_port *endpoint = cxlmd->endpoint;
 	struct cxl_hdm *cxlhdm = dev_get_drvdata(&endpoint->dev);
@@ -972,6 +1009,187 @@ int cxl_endpoint_enable_hdm_decode(struct cxl_memdev *cxlmd, u32 global_ctrl)
 	return 0;
 }
 
+/**
+ * cxl_port_recommit_decoders - reprogram the HDM decoders below @port
+ * @port: CXL port whose downstream decoders to reprogram
+ * @hdm_state: saved &struct cxl_hdm_state per port, keyed by &struct cxl_port
+ *
+ * Reprogram the HDM decoders below @port that lost their programming. Every
+ * endpoint beneath @port is restored along its whole path, from the endpoint up
+ * to the last port below @port. A decoder that hardware still reports committed
+ * is left untouched.
+ *
+ * Per CXL r4.0 sec 8.2.4.20.13 decoder m must be committed before decoder m+1
+ * while reprogramming, so let device_for_each_child() visit each port's decoders
+ * in instance order. Each path is walked from the endpoint upward, the order
+ * cxl_region_decode_commit() uses.
+ *
+ * The endpoints are reprogrammed one after another, so an interleaved HPA range
+ * decodes through only part of its interleave set until the last member is
+ * done. Per CXL r4.0 sec 8.2.4.20.13 software owns quiescing the traffic that
+ * targets a decoder being reprogrammed: a read that no decoder positively
+ * decodes returns all 1s or poison, and per Table 8-118 such a write is
+ * dropped. Nothing here can detect a stray access, so the caller carries that
+ * duty.
+ *
+ * Context: caller must hold @cxl_rwsem.region to keep the topology and the
+ * switch decoder target lists stable across the walk, and must have quiesced
+ * every access to the HPA ranges decoded below @port.
+ *
+ * A port with no entry in @hdm_state was not saved, so its decoders are left
+ * alone rather than committed with whatever the reset left in the fields the
+ * driver does not model.
+ *
+ * Return: 0 on success, negative errno of the first decoder that failed or
+ * -ENODATA if a port on the path has no saved state.
+ */
+int cxl_port_recommit_decoders(struct cxl_port *port, struct xarray *hdm_state)
+{
+	struct cxl_ep *port_ep;
+	unsigned long index;
+	int first_rc = 0;
+
+	lockdep_assert_held(&cxl_rwsem.region);
+
+	xa_for_each(&port->endpoints, index, port_ep) {
+		struct cxl_memdev *cxlmd = to_cxl_memdev(port_ep->ep);
+		struct cxl_hdm_state *state;
+		struct cxl_port *iter;
+		int rc;
+
+		if (IS_ERR_OR_NULL(cxlmd->endpoint))
+			continue;
+
+		state = xa_load(hdm_state, (unsigned long)cxlmd->endpoint);
+		if (!state) {
+			dev_warn(&cxlmd->dev, "%s: no saved HDM state\n",
+				 __func__);
+			if (!first_rc)
+				first_rc = -ENODATA;
+			continue;
+		}
+
+		rc = cxl_endpoint_enable_hdm_decode(cxlmd, state->global_ctrl);
+		if (rc) {
+			dev_warn(&cxlmd->dev,
+				 "%s: failed to enable HDM decode: %d\n",
+				 __func__, rc);
+			if (!first_rc)
+				first_rc = rc;
+			continue;
+		}
+
+		/*
+		 * Walk from the endpoint up to @port so a decoder is committed
+		 * only after the decoder it routes to. @port is the last parent
+		 * visited by the walk, and it is excluded.
+		 */
+		for (iter = cxlmd->endpoint; iter && iter != port;
+		     iter = parent_port_of(iter)) {
+			struct cxl_recommit_ctx ctx = {
+				.state = xa_load(hdm_state, (unsigned long)iter),
+				.first_rc = &first_rc,
+			};
+
+			if (!ctx.state) {
+				dev_warn(&iter->dev, "%s: no saved HDM state\n",
+					 __func__);
+				if (!first_rc)
+					first_rc = -ENODATA;
+				continue;
+			}
+
+			device_for_each_child(&iter->dev, &ctx,
+					      __cxl_port_recommit_decoder);
+		}
+	}
+
+	return first_rc;
+}
+
+/**
+ * cxl_port_save_hdm_state - record the HDM decoder control registers below @port
+ * @port: CXL port whose downstream decoders to record
+ * @hdm_state: xarray to fill, one entry per port, keyed by &struct cxl_port
+ *
+ * Read the CXL HDM Decoder Global Control and every CXL HDM Decoder n Control
+ * register of the ports below @port. Those hold the fields
+ * cxl_port_recommit_decoders() cannot rebuild from the driver's cached settings,
+ * so they have to be read while the registers still hold them.
+ *
+ * The set of ports is the same one cxl_port_recommit_decoders() walks. A port
+ * with no HDM decoder registers has nothing to record and gets no entry.
+ *
+ * Context: caller must hold @cxl_rwsem.region.
+ *
+ * Return: 0 on success, negative errno if an entry cannot be allocated or
+ * inserted.
+ */
+int cxl_port_save_hdm_state(struct cxl_port *port, struct xarray *hdm_state)
+{
+	struct cxl_ep *port_ep;
+	unsigned long index;
+
+	lockdep_assert_held(&cxl_rwsem.region);
+
+	xa_for_each(&port->endpoints, index, port_ep) {
+		struct cxl_memdev *cxlmd = to_cxl_memdev(port_ep->ep);
+		struct cxl_port *iter;
+
+		if (IS_ERR_OR_NULL(cxlmd->endpoint))
+			continue;
+
+		for (iter = cxlmd->endpoint; iter && iter != port;
+		     iter = parent_port_of(iter)) {
+			struct cxl_hdm *cxlhdm = dev_get_drvdata(&iter->dev);
+			struct cxl_hdm_state *state;
+			void __iomem *hdm;
+			int rc;
+
+			if (xa_load(hdm_state, (unsigned long)iter))
+				continue;
+
+			if (!cxlhdm || !cxlhdm->regs.hdm_decoder)
+				continue;
+
+			hdm = cxlhdm->regs.hdm_decoder;
+			state = kzalloc_flex(*state, ctrl,
+					     cxlhdm->decoder_count);
+			if (!state)
+				return -ENOMEM;
+
+			state->global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
+			state->nr_ctrl = cxlhdm->decoder_count;
+			for (int i = 0; i < state->nr_ctrl; i++)
+				state->ctrl[i] =
+					readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(i));
+
+			rc = xa_insert(hdm_state, (unsigned long)iter, state,
+				       GFP_KERNEL);
+			if (rc) {
+				kfree(state);
+				return rc;
+			}
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * cxl_port_put_hdm_state - release a set filled by cxl_port_save_hdm_state()
+ * @hdm_state: xarray to empty
+ */
+void cxl_port_put_hdm_state(struct xarray *hdm_state)
+{
+	struct cxl_hdm_state *state;
+	unsigned long index;
+
+	xa_for_each(hdm_state, index, state)
+		kfree(state);
+	xa_destroy(hdm_state);
+}
+
 static int commit_reap(struct device *dev, void *data)
 {
 	struct cxl_port *port = to_cxl_port(dev->parent);
-- 
2.55.0

[PATCH v2 05/13] cxl/core: Restore the HDM decoders below DPort

[Fabio M. De Francesco]

A Secondary Bus Reset clears the HDM Decoder registers of every CXL
component below the Port, so the decoders have to be reprogrammed from the
settings the driver holds before anything reaches the region again.

Add cxl_sbr_recommit_decoders() to find the CXL port that owns the
Downstream Port and reprogram the decoders below it. It takes
cxl_rwsem.region for read across the walk: the walk reads the endpoint set
below the Port, each endpoint's parent chain and the switch decoder target
lists, all of which change only under that lock held for write.

The driver's cached settings do not cover the whole of either control
register, so the saved state that cxl_port_recommit_decoders() needs is
passed through as well, an xarray of struct cxl_hdm_state keyed by
struct cxl_port and read before the reset.

Put it in a new dport_sbr.c, which collects what a Secondary Bus Reset of a
CXL Downstream Port needs from the CXL core. find_cxl_port() loses its
static since the lookup now has a caller outside port.c.

It has no caller yet. It is used on the way out of the reset, once the
regions routed through the Port are back.

Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
---
 drivers/cxl/core/Makefile    |  2 +-
 drivers/cxl/core/core.h      |  6 ++++++
 drivers/cxl/core/dport_sbr.c | 36 ++++++++++++++++++++++++++++++++++++
 drivers/cxl/core/port.c      |  4 ++--
 tools/testing/cxl/Kbuild     |  2 +-
 5 files changed, 46 insertions(+), 4 deletions(-)
 create mode 100644 drivers/cxl/core/dport_sbr.c

diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
index ce7213818d3c..4d4538182d81 100644
--- a/drivers/cxl/core/Makefile
+++ b/drivers/cxl/core/Makefile
@@ -15,7 +15,7 @@ cxl_core-y += hdm.o
 cxl_core-y += pmu.o
 cxl_core-y += cdat.o
 cxl_core-$(CONFIG_TRACING) += trace.o
-cxl_core-$(CONFIG_CXL_REGION) += region.o region_pmem.o region_dax.o
+cxl_core-$(CONFIG_CXL_REGION) += region.o region_pmem.o region_dax.o dport_sbr.o
 cxl_core-$(CONFIG_CXL_MCE) += mce.o
 cxl_core-$(CONFIG_CXL_FEATURES) += features.o
 cxl_core-$(CONFIG_CXL_EDAC_MEM_FEATURES) += edac.o
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index d23f0bbd7bf0..a8b9d7990d32 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -13,6 +13,9 @@ extern const struct device_type cxl_pmu_type;
 
 extern struct attribute_group cxl_base_attribute_group;
 
+struct cxl_port *find_cxl_port(struct device *dport_dev,
+			       struct cxl_dport **dport);
+
 enum cxl_detach_mode {
 	DETACH_ONLY,
 	DETACH_INVALIDATE,
@@ -53,6 +56,9 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
 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);
+struct pci_dev;
+void cxl_sbr_recommit_decoders(struct pci_dev *dport_pci,
+			       struct xarray *hdm_state);
 
 #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
new file mode 100644
index 000000000000..55c35386d0cb
--- /dev/null
+++ b/drivers/cxl/core/dport_sbr.c
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2026 Intel Corporation. */
+
+#include <linux/device.h>
+#include <linux/pci.h>
+#include <cxl.h>
+#include "core.h"
+
+/*
+ * The reset cleared the HDM Decoder registers of every CXL component below
+ * @dport_pci, so restore them from the settings the driver holds and from
+ * @hdm_state, the register fields the driver does not model, saved before the
+ * reset. Takes cxl_rwsem.region for read, which cxl_port_recommit_decoders()
+ * 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)
+{
+	struct cxl_dport *dport;
+	int rc;
+
+	struct cxl_port *port __free(put_cxl_port) =
+		find_cxl_port(&dport_pci->dev, &dport);
+	if (!port) {
+		pci_dbg(dport_pci, "no CXL port owns this Downstream Port\n");
+		return;
+	}
+
+	pci_dbg(dport_pci, "restoring HDM decode below %s\n", dev_name(&port->dev));
+
+	guard(rwsem_read)(&cxl_rwsem.region);
+	rc = cxl_port_recommit_decoders(port, hdm_state);
+	if (rc)
+		pci_warn(dport_pci, "HDM decode restore failed: %d\n", rc);
+}
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 1215ee4f4035..66a6d513843e 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1392,8 +1392,8 @@ static struct cxl_port *__find_cxl_port(struct cxl_find_port_ctx *ctx)
 	return NULL;
 }
 
-static struct cxl_port *find_cxl_port(struct device *dport_dev,
-				      struct cxl_dport **dport)
+struct cxl_port *find_cxl_port(struct device *dport_dev,
+			       struct cxl_dport **dport)
 {
 	struct cxl_find_port_ctx ctx = {
 		.dport_dev = dport_dev,
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index 2be1df80fcc9..f18d41b299fe 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -63,7 +63,7 @@ cxl_core-y += $(CXL_CORE_SRC)/hdm.o
 cxl_core-y += $(CXL_CORE_SRC)/pmu.o
 cxl_core-y += $(CXL_CORE_SRC)/cdat.o
 cxl_core-$(CONFIG_TRACING) += $(CXL_CORE_SRC)/trace.o
-cxl_core-$(CONFIG_CXL_REGION) += $(CXL_CORE_SRC)/region.o $(CXL_CORE_SRC)/region_pmem.o $(CXL_CORE_SRC)/region_dax.o
+cxl_core-$(CONFIG_CXL_REGION) += $(CXL_CORE_SRC)/region.o $(CXL_CORE_SRC)/region_pmem.o $(CXL_CORE_SRC)/region_dax.o $(CXL_CORE_SRC)/dport_sbr.o
 cxl_core-$(CONFIG_CXL_MCE) += $(CXL_CORE_SRC)/mce.o
 cxl_core-$(CONFIG_CXL_FEATURES) += $(CXL_CORE_SRC)/features.o
 cxl_core-$(CONFIG_CXL_EDAC_MEM_FEATURES) += $(CXL_CORE_SRC)/edac.o
-- 
2.55.0

[PATCH v2 06/13] drivers/base/memory: Add cxl_offline_memory() to offline a physical range

[Fabio M. De Francesco]

A CXL region whose capacity is online as System RAM must be taken
offline before a Secondary Bus Reset of the CXL Downstream Port it is
reached through.

Add cxl_offline_memory() to offline the memory blocks spanning a
physical range, exported in the CXL_MHP namespace since the CXL core is
its only caller.

Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
---
 drivers/base/memory.c  | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/memory.h |  5 +++++
 2 files changed, 41 insertions(+)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index bcfe2d9f4adb..ce89b251d34a 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -1018,6 +1018,42 @@ int walk_memory_blocks(unsigned long start, unsigned long size,
 	return ret;
 }
 
+static int cxl_offline_memory_block(struct memory_block *mem, void *arg)
+{
+	int rc = device_offline(&mem->dev);
+
+	/* device_offline() returns a positive value when already offline. */
+	if (rc > 0)
+		return 0;
+
+	return rc;
+}
+
+/**
+ * cxl_offline_memory - offline the memory blocks spanning a physical range
+ * @start: start of the range, memory-block aligned
+ * @size: size of the range, a multiple of the memory block size
+ *
+ * Offline every memory block in [start, start + size). The blocks are offlined
+ * but not removed, so the range can be brought back online afterward. The
+ * caller owns the range and this performs no validation on it.
+ *
+ * Return: 0 on success, negative errno if a block cannot be offlined.
+ *
+ * Context: process context. Sleeps and takes the memory hotplug lock.
+ */
+int cxl_offline_memory(u64 start, u64 size)
+{
+	int rc;
+
+	lock_device_hotplug();
+	rc = walk_memory_blocks(start, size, NULL, cxl_offline_memory_block);
+	unlock_device_hotplug();
+
+	return rc;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_offline_memory, "CXL_MHP");
+
 struct for_each_memory_block_cb_data {
 	walk_memory_blocks_func_t func;
 	void *arg;
diff --git a/include/linux/memory.h b/include/linux/memory.h
index 463dc02f6cff..5dc0b96fb004 100644
--- a/include/linux/memory.h
+++ b/include/linux/memory.h
@@ -149,6 +149,10 @@ static inline unsigned long memory_block_advised_max_size(void)
 {
 	return 0;
 }
+static inline int cxl_offline_memory(u64 start, u64 size)
+{
+	return 0;
+}
 #else /* CONFIG_MEMORY_HOTPLUG */
 extern int register_memory_notifier(struct notifier_block *nb);
 extern void unregister_memory_notifier(struct notifier_block *nb);
@@ -167,6 +171,7 @@ typedef int (*walk_memory_blocks_func_t)(struct memory_block *, void *);
 extern int walk_memory_blocks(unsigned long start, unsigned long size,
 			      void *arg, walk_memory_blocks_func_t func);
 extern int for_each_memory_block(void *arg, walk_memory_blocks_func_t func);
+int cxl_offline_memory(u64 start, u64 size);
 
 extern int memory_group_register_static(int nid, unsigned long max_pages);
 extern int memory_group_register_dynamic(int nid, unsigned long unit_pages);
-- 
2.55.0

[PATCH v2 07/13] cxl/core: Add region disable and enable for a DPort SBR

[Fabio M. De Francesco]

A Secondary Bus Reset of a CXL Downstream Port removes the downstream
component from the bus, so a CXL region reached through that Port has to
be disabled while the reset runs.

Add cxl_region_disable() to offline memory, invalidate the CPU caches
for its range and release its driver, and cxl_region_enable() to
re-attach the driver afterwards.

Neither function has a caller yet. Both are driven by the reset once the
set of regions routed through the Port is known.

Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
---
 drivers/cxl/core/core.h      |  3 ++
 drivers/cxl/core/dport_sbr.c | 79 ++++++++++++++++++++++++++++++++++++
 drivers/cxl/core/region.c    |  3 +-
 3 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index a8b9d7990d32..ac55f9e8160e 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -56,6 +56,9 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
 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;
 void cxl_sbr_recommit_decoders(struct pci_dev *dport_pci,
 			       struct xarray *hdm_state);
diff --git a/drivers/cxl/core/dport_sbr.c b/drivers/cxl/core/dport_sbr.c
index 55c35386d0cb..0f398ab0d3a4 100644
--- a/drivers/cxl/core/dport_sbr.c
+++ b/drivers/cxl/core/dport_sbr.c
@@ -1,11 +1,90 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2026 Intel Corporation. */
 
+#include <linux/memregion.h>
+#include <linux/memory_hotplug.h>
+#include <linux/memory.h>
 #include <linux/device.h>
 #include <linux/pci.h>
 #include <cxl.h>
 #include "core.h"
 
+/*
+ * cxl_region_disable - make a region inactive ahead of a Secondary Bus Reset
+ * @cxlr: region routed through the CXL Downstream Port being reset
+ *
+ * Offline the memory blocks the region owns and unbind its driver. An SBR
+ * zeroes the downstream bus number, so a region left live as System RAM would
+ * be accessed while the device is in reset. On offline failure return the error
+ * so the caller aborts the reset; the memory is never force-removed.
+ *
+ * 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)
+{
+	struct cxl_region_params *p = &cxlr->params;
+	unsigned long block_size;
+	u64 start, end;
+	int rc;
+
+	/*
+	 * Per CXL r4.0 sec 9.13.1 an Interleave Set has a Base HPA and a Size
+	 * that are multiples of 256 MB, while a memory block spans up to 2 GB.
+	 * A block overlapping either end of the range therefore also covers
+	 * memory outside this region, so round the range inward to block
+	 * granularity as dax_kmem did when it onlined the range. Offlining a
+	 * straddling block would migrate pages that the reset does not affect.
+	 */
+	block_size = memory_block_size_bytes();
+	start = ALIGN(p->res->start, block_size);
+	end = ALIGN_DOWN(p->res->end + 1, block_size);
+	if (start >= end) {
+		dev_dbg(&cxlr->dev, "%s: HPA %pr spans no whole memory block, no System RAM to offline\n",
+			__func__, p->res);
+	} else {
+		rc = cxl_offline_memory(start, end - start);
+		if (rc) {
+			dev_warn(&cxlr->dev, "offline System RAM failed before reset: %d\n",
+				 rc);
+			return rc;
+		}
+	}
+
+	rc = cxl_region_invalidate_memregion(cxlr);
+	if (rc) {
+		dev_warn(&cxlr->dev, "CPU cache invalidate failed before reset: %d\n",
+			 rc);
+		return rc;
+	}
+
+	device_release_driver(&cxlr->dev);
+	dev_dbg(&cxlr->dev, "%s: System RAM offline, region disabled before reset, HPA %pr\n",
+		__func__, p->res);
+
+	return 0;
+}
+
+/*
+ * cxl_region_enable - restore a region after a Secondary Bus Reset
+ * @cxlr: region disabled by cxl_region_disable() before the reset
+ *
+ * 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)
+{
+	struct cxl_region_params *p = &cxlr->params;
+
+	if (device_attach(&cxlr->dev) < 0) {
+		dev_dbg(&cxlr->dev, "driver re-attach failed after reset\n");
+		return;
+	}
+
+	dev_dbg(&cxlr->dev, "%s: region re-enabled after reset, HPA %pr, IW %d, IG %d\n",
+		__func__, p->res, p->interleave_ways, p->interleave_granularity);
+}
+
 /*
  * The reset cleared the HDM Decoder registers of every CXL component below
  * @dport_pci, so restore them from the settings the driver holds and from
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 1e211542b6b6..fc0bec991a69 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -222,7 +222,7 @@ static struct cxl_region_ref *cxl_rr_load(struct cxl_port *port,
 	return xa_load(&port->regions, (unsigned long)cxlr);
 }
 
-static int cxl_region_invalidate_memregion(struct cxl_region *cxlr)
+int cxl_region_invalidate_memregion(struct cxl_region *cxlr)
 {
 	if (!cpu_cache_has_invalidate_memregion()) {
 		if (IS_ENABLED(CONFIG_CXL_REGION_INVALIDATION_TEST)) {
@@ -4273,4 +4273,5 @@ void cxl_region_exit(void)
 
 MODULE_IMPORT_NS("CXL");
 MODULE_IMPORT_NS("DEVMEM");
+MODULE_IMPORT_NS("CXL_MHP");
 MODULE_ALIAS_CXL(CXL_DEVICE_REGION);
-- 
2.55.0

[PATCH v2 08/13] cxl/core: Collect the regions routed through a DPort

[Fabio M. De Francesco]

Disabling the CXL regions ahead of a Secondary Bus Reset needs the set of
regions with a member endpoint below the Port being reset.

Add cxl_sbr_collect_regions() to walk the CXL port that owns the Downstream
Port and return every region with an endpoint below it, and
cxl_sbr_put_regions() to release the set.

Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
---
 drivers/cxl/core/core.h      |  2 ++
 drivers/cxl/core/dport_sbr.c | 66 ++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index ac55f9e8160e..077a2af9cf0c 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -60,6 +60,8 @@ 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);
 
diff --git a/drivers/cxl/core/dport_sbr.c b/drivers/cxl/core/dport_sbr.c
index 0f398ab0d3a4..233499bc1fad 100644
--- a/drivers/cxl/core/dport_sbr.c
+++ b/drivers/cxl/core/dport_sbr.c
@@ -85,6 +85,72 @@ void cxl_region_enable(struct cxl_region *cxlr)
 		__func__, p->res, p->interleave_ways, p->interleave_granularity);
 }
 
+/*
+ * Collect the regions with a member endpoint routed through @dport_pci, the
+ * CXL Downstream Port about to be reset. cxl_rwsem.region keeps the topology
+ * stable for the duration of the walk only. Each collected region is pinned
+ * with get_device() so the object survives after the lock is dropped, since
+ * 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)
+{
+	struct cxl_region_ref *cxl_rr;
+	struct cxl_dport *dport;
+	unsigned long index;
+	int count = 0;
+	int rc;
+
+	struct cxl_port *port __free(put_cxl_port) =
+		find_cxl_port(&dport_pci->dev, &dport);
+	if (!port) {
+		pci_dbg(dport_pci, "no CXL port found for reset dport\n");
+		return 0;
+	}
+
+	guard(rwsem_read)(&cxl_rwsem.region);
+	xa_for_each(&port->regions, index, cxl_rr) {
+		struct cxl_region *cxlr = cxl_rr->region;
+		struct cxl_ep *ep;
+		unsigned long ep_index;
+
+		/* Skip unless a region endpoint sits below the reset dport. */
+		xa_for_each(&cxl_rr->endpoints, ep_index, ep)
+			if (ep->dport == dport)
+				break;
+		if (!ep) {
+			dev_dbg(&cxlr->dev, "%s: no endpoint below %s, region excluded\n",
+				__func__, dev_name(dport->dport_dev));
+			continue;
+		}
+
+		get_device(&cxlr->dev);
+		rc = xa_insert(regions, (unsigned long)cxlr, cxlr, GFP_KERNEL);
+		if (rc) {
+			put_device(&cxlr->dev);
+			return rc;
+		}
+		dev_dbg(&cxlr->dev, "%s: endpoint below %s, region collected\n",
+			__func__, dev_name(dport->dport_dev));
+		count++;
+	}
+
+	dev_dbg(&port->dev, "%d region(s) routed through %s\n", count,
+		dev_name(dport->dport_dev));
+	return 0;
+}
+
+void cxl_sbr_put_regions(struct xarray *regions)
+{
+	struct cxl_region *cxlr;
+	unsigned long index;
+
+	xa_for_each(regions, index, cxlr)
+		put_device(&cxlr->dev);
+	xa_destroy(regions);
+}
+
 /*
  * The reset cleared the HDM Decoder registers of every CXL component below
  * @dport_pci, so restore them from the settings the driver holds and from
-- 
2.55.0

[PATCH v2 09/13] PCI/CXL: Disable and re-enable CXL regions

[Fabio M. De Francesco]

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@linux.intel.com>
---
 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(&regions);
+
+	rc = cxl_sbr_collect_regions(dport_pci, &regions);
+	if (rc)
+		goto out;
+
+	xa_for_each(&regions, 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(&regions, index, cxlr)
+			cxl_region_enable(cxlr);
+	}
+
+out:
+	cxl_sbr_put_regions(&regions);
+	/* 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(&regions);
+
+	cxl_sbr_collect_regions(dport_pci, &regions);
+
+	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(&regions, index, cxlr)
+		cxl_region_enable(cxlr);
+
+	cxl_sbr_put_regions(&regions);
+	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);
+
 /**
  * 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

[PATCH v2 10/13] PCI/CXL: Clear ACS SV across an SBR of a CXL DPort

[Fabio M. De Francesco]

A Secondary Bus Reset (SBR) of a CXL Downstream Port clears the
Downstream Component's (DPort) captured Bus Number. CXL r4.0 sec 8.1.5.1
notes that if a Downstream Component issues PM Init messages to a DPort
with Access Control Services (ACS) Source Validation (SV) bit enabled,
PM Initialization may then fail to complete, and that a further SBR
alone does not recover it. PCIe r7.0 sec 6.12.1.1 makes that rejection
an error that is a reported as ACS Violation.

Reuse the recovery sequence described in sec 8.1.5.1 but make it
preventive, so that PM Init don't fail and don't need to be recovered.

Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
---
 drivers/pci/pci.c             | 177 ++++++++++++++++++++++++++++++++--
 include/uapi/linux/pci_regs.h |   2 +
 2 files changed, 172 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 417b6b44473e..f3781d1e8f6e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -23,6 +23,7 @@
 #include <linux/module.h>
 #include <linux/spinlock.h>
 #include <linux/string.h>
+#include <linux/string_choices.h>
 #include <linux/log2.h>
 #include <linux/logic_pio.h>
 #include <linux/device.h>
@@ -4857,21 +4858,189 @@ void pci_cxl_set_sbr_region_ops(const struct pci_cxl_sbr_region_ops *ops)
 }
 EXPORT_SYMBOL_GPL(pci_cxl_set_sbr_region_ops);
 
+struct cxl_sbr_ctx {
+	u16 port_ctl;
+	u16 acs_ctrl;
+	u16 command;
+};
+
+static 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)
+{
+	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)
+{
+	int rc;
+
+	/* Abort before touching hardware if the regions cannot be disabled. */
+	if (cxl_sbr_region_ops) {
+		rc = cxl_sbr_region_ops->disable_regions(bridge);
+		if (rc)
+			return rc;
+	}
+
+	/* CXL r4.0 sec 8.1.5.2, Table 8-32: set Unmask SBR so the Port issues Hot Reset. */
+	pci_read_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL, &ctx->port_ctl);
+	pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL,
+			      ctx->port_ctl | PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR);
+
+	pci_read_config_word(bridge, PCI_COMMAND, &ctx->command);
+	pci_clear_master(bridge);
+
+	/* CXL r4.0 sec 8.1.5.1: Disable ACS SV bit before SBR */
+	if (bridge->acs_cap) {
+		pci_read_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL, &ctx->acs_ctrl);
+		pci_dbg(bridge, "%s: ACS SV %s\n", __func__,
+			str_enabled_disabled(ctx->acs_ctrl & PCI_ACS_SV));
+		pci_write_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL,
+				      ctx->acs_ctrl & ~PCI_ACS_SV);
+	}
+
+	return 0;
+}
+
+/*
+ * CXL r4.0 sec 8.1.5.1, Table 8-31: the Port sets PM Init Complete within
+ * 100 ms of link-up. Restoring ACS Source Validation before then makes the
+ * Port reject the downstream Component's Requester-Bus-0 IP2PM message, so
+ * poll for completion before restoring config.
+ */
+static bool cxl_port_pm_init_is_complete(struct pci_dev *bridge, u16 dvsec)
+{
+	unsigned long start = jiffies;
+	unsigned long timeout = start + msecs_to_jiffies(100);
+	u16 status;
+
+	do {
+		pci_read_config_word(bridge,
+				     dvsec + PCI_DVSEC_CXL_PORT_EXT_STATUS,
+				     &status);
+		if (!PCI_POSSIBLE_ERROR(status) &&
+		    (status & PCI_DVSEC_CXL_PORT_EXT_STATUS_PM_INIT_COMP)) {
+			pci_dbg(bridge, "%s: PM Init Complete set after %u ms, ext status %#06x\n",
+				__func__, jiffies_to_msecs(jiffies - start), status);
+			return true;
+		}
+		msleep(10);
+	} while (time_before(jiffies, timeout));
+
+	pci_warn(bridge, "%s: PM Init Complete not set after %u ms, ext status %#06x\n",
+		 __func__, jiffies_to_msecs(jiffies - start), status);
+
+	return false;
+}
+
+static int cxl_sbr_restore_config_space(struct pci_dev *dev, void *userdata)
+{
+	pci_restore_config_space(dev);
+	pci_dbg(dev, "%s: config space restored\n", __func__);
+
+	return 0;
+}
+
+/*
+ * The CXL region ops that run next read the HDM Decoders through a Base Address
+ * Register the reset returned to its initialization value, so restore the
+ * header of every device below @bridge first. Restoring also re-captures each
+ * Bus Number before the Port's ACS Source Validation comes back: a device that
+ * has completed no Type 0 Configuration Write since the reset sources Requests
+ * with Bus 0, which the Port rejects as an ACS Violation.
+ *
+ * Only the header is restored. The capability state each caller saved is its own
+ * to replay, and the ->reset_done() callbacks pci_dev_restore() invokes must
+ * fire once, from the caller that owns the reset.
+ */
+static void cxl_sbr_restore_subordinate(struct pci_dev *bridge)
+{
+	if (!bridge->subordinate)
+		return;
+
+	/* Parents before children: a child answers once its parent forwards. */
+	pci_walk_bus(bridge->subordinate, cxl_sbr_restore_config_space, NULL);
+}
+
+static void cxl_sbr_complete(struct pci_dev *bridge, u16 dvsec,
+			     const struct cxl_sbr_ctx *ctx)
+{
+	u16 val;
+
+	/* CXL r4.0 sec 8.1.5.1: wait for PM Init before restoring ACS SV. */
+	if (!cxl_port_pm_init_is_complete(bridge, dvsec))
+		pci_warn(bridge,
+			 "restoring ACS Source Validation before PM Init complete; Port may reject the Component's bus 0 traffic\n");
+
+	cxl_sbr_restore_subordinate(bridge);
+
+	/* CXL r4.0 sec 8.1.5.1: Re-enable ACS SV bit after SBR if it was enabled before */
+	if (bridge->acs_cap && (ctx->acs_ctrl & PCI_ACS_SV)) {
+		pci_read_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL, &val);
+		pci_write_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL,
+				      val | PCI_ACS_SV);
+		pci_dbg(bridge, "%s: ACS SV bit set\n", __func__);
+	} else {
+		pci_dbg(bridge, "%s: ACS SV bit not set (was not enabled before the SBR)\n",
+			__func__);
+	}
+
+	if (ctx->command & PCI_COMMAND_MASTER)
+		pci_set_master(bridge);
+
+	if (!(ctx->port_ctl & PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR)) {
+		pci_read_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL, &val);
+		pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL,
+				      val & ~PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR);
+	}
+
+	if (cxl_sbr_region_ops)
+		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.
+ *
+ * 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)
 {
+	struct cxl_sbr_ctx ctx = {};
+	u16 dvsec = 0;
+	int rc;
+
 	if (!dev->block_cfg_access)
 		pci_warn_once(dev, "unlocked secondary bus reset via: %pS\n",
 			      __builtin_return_address(0));
+
+	if (is_cxl_dport(dev))
+		dvsec = cxl_port_dvsec(dev);
+	if (dvsec) {
+		rc = cxl_sbr_prepare(dev, dvsec, &ctx);
+		if (rc)
+			return rc;
+	}
+
 	pcibios_reset_secondary_bus(dev);
 
-	return pci_bridge_wait_for_secondary_bus(dev, "bus reset");
+	rc = pci_bridge_wait_for_secondary_bus(dev, "bus reset");
+
+	if (dvsec)
+		cxl_sbr_complete(dev, dvsec, &ctx);
+
+	return rc;
 }
 EXPORT_SYMBOL_GPL(pci_bridge_secondary_bus_reset);
 
@@ -4917,12 +5086,6 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe)
 	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
 }
 
-static u16 cxl_port_dvsec(struct pci_dev *dev)
-{
-	return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
-					 PCI_DVSEC_CXL_PORT);
-}
-
 static bool cxl_sbr_masked(struct pci_dev *dev)
 {
 	u16 dvsec, reg;
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index facaa324bd86..0eaa34db93ce 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1371,6 +1371,8 @@
 
 /* CXL r4.0, 8.1.5: Extensions DVSEC for Ports */
 #define PCI_DVSEC_CXL_PORT				3
+#define  PCI_DVSEC_CXL_PORT_EXT_STATUS			0x0a
+#define   PCI_DVSEC_CXL_PORT_EXT_STATUS_PM_INIT_COMP	0x00000001
 #define  PCI_DVSEC_CXL_PORT_CTL				0x0c
 #define   PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR		0x00000001
 
-- 
2.55.0

[PATCH v2 11/13] PCI/DPC: Unbind regions for DPC recovery

[Fabio M. De Francesco]

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

Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
---
 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(&regions);
+
+	cxl_sbr_collect_regions(dport_pci, &regions);
+
+	xa_for_each(&regions, index, cxlr)
+		cxl_region_unbind(cxlr);
+
+	cxl_sbr_put_regions(&regions);
+}
+
 /*
  * 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

[PATCH v2 12/13] PCI/CXL: Add a sysfs entry to unmask SBR

[Fabio M. De Francesco]

A Secondary Bus Reset of a CXL Downstream Port only reaches the component
below it when the Port's Unmask SBR bit is set. CXL r4.0 sec 8.1.5.2
Table 8-32: "When cleared to 0 (default), the SBR bit in this Port's
Bridge Control register has no effect." A kernel that unmasks it on its own initiative overrules
that for every path reaching pci_bridge_secondary_bus_reset() SBR.

Add a cxl_unmask_sbr attribute beside reset_subordinate in the Port's
sysfs directory to allow kernel triggered SBR of CXL DPorts.

Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
---
 Documentation/ABI/testing/sysfs-bus-pci | 16 +++++++++++++
 drivers/pci/pci-sysfs.c                 | 31 +++++++++++++++++++++++++
 include/linux/pci.h                     |  1 +
 3 files changed, 48 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index b767db2c52cb..7b0314da6f4d 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -174,6 +174,22 @@ Description:
 		similiar to writing 1 to their individual "reset" file, so use
 		with caution.
 
+What:		/sys/bus/pci/devices/.../cxl_unmask_sbr
+Date:		August 2026
+Contact:	linux-pci@vger.kernel.org
+Description:
+		This is visible only for a CXL Downstream Port, that is a Root
+		Port or a Downstream Switch Port that publishes the CXL
+		Extensions DVSEC for Ports.  Such a Port ignores the Secondary
+		Bus Reset bit in its Bridge Control register unless its
+		"Unmask SBR" bit is set, and system firmware leaves that bit
+		clear by default.
+
+		Writing 1 allows the kernel to set "Unmask SBR" while it
+		resets the Port's secondary bus, so that the reset reaches the
+		component below the Port.  While this reads 0, a bus reset of
+		the Port fails instead.
+
 What:		/sys/bus/pci/devices/.../vpd
 Date:		February 2008
 Contact:	Ben Hutchings <bwh@kernel.org>
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 5ec0b245a69b..ba1cae6ab4fa 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -573,6 +573,33 @@ static ssize_t reset_subordinate_store(struct device *dev,
 }
 static DEVICE_ATTR_WO(reset_subordinate);
 
+static ssize_t cxl_unmask_sbr_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sysfs_emit(buf, "%u\n", pdev->cxl_unmask_sbr);
+}
+
+static ssize_t cxl_unmask_sbr_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	unsigned long val;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (kstrtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	pdev->cxl_unmask_sbr = !!val;
+
+	return count;
+}
+static DEVICE_ATTR_RW(cxl_unmask_sbr);
+
 #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
 static ssize_t d3cold_allowed_store(struct device *dev,
 				    struct device_attribute *attr,
@@ -650,6 +677,7 @@ static struct attribute *pci_bridge_attrs[] = {
 	&dev_attr_subordinate_bus_number.attr,
 	&dev_attr_secondary_bus_number.attr,
 	&dev_attr_reset_subordinate.attr,
+	&dev_attr_cxl_unmask_sbr.attr,
 	NULL,
 };
 
@@ -1824,6 +1852,9 @@ static umode_t pci_bridge_attrs_are_visible(struct kobject *kobj,
 	struct device *dev = kobj_to_dev(kobj);
 	struct pci_dev *pdev = to_pci_dev(dev);
 
+	if (a == &dev_attr_cxl_unmask_sbr.attr && !is_cxl_dport(pdev))
+		return 0;
+
 	if (pci_is_bridge(pdev))
 		return a->mode;
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0d9832ce6f3d..bc0f652eff80 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -485,6 +485,7 @@ struct pci_dev {
 	unsigned int	shpc_managed:1;		/* SHPC owned by shpchp */
 	unsigned int	is_thunderbolt:1;	/* Thunderbolt controller */
 	unsigned int	is_cxl:1;               /* Compute Express Link (CXL) */
+	unsigned int	cxl_unmask_sbr:1;	/* SBR unmask allowed by user */
 	/*
 	 * Devices marked being untrusted are the ones that can potentially
 	 * execute DMA attacks and similar. They are typically connected
-- 
2.55.0

[PATCH v2 13/13] PCI/CXL: Refuse an SBR of a CXL DPort unless authorized

[Fabio M. De Francesco]

CXL r4.0 sec 8.1.5.2 Table 8-32 describes a bit that makes the SBR bit
in Bridge Control take effect at all, so a Port left at the firmware
default is one the reset must not silently unmask.

Add cxl_sbr_allowed() to answer if SBR is allowed: check if a Port whose
Unmask SBR is already set needs no consent, as it happens with cxl_bus
reset.

Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
---
 drivers/pci/pci.c | 26 ++++++++++++++++++++++++++
 drivers/pci/pci.h |  1 +
 2 files changed, 27 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index eedd516f8484..08873ea3957b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4881,6 +4881,11 @@ static int cxl_sbr_prepare(struct pci_dev *bridge, u16 dvsec,
 {
 	int rc;
 
+	if (action == CXL_SBR_OFFLINE_AND_UNBIND && !cxl_sbr_allowed(bridge)) {
+		pci_info(bridge, "SBR masked, write 1 to cxl_unmask_sbr to allow a bus reset\n");
+		return -ENOTTY;
+	}
+
 	/*
 	 * CXL_SBR_UNBIND: the link is already down, so offlining the regions'
 	 * memory would take the reads that page migration performs as a machine
@@ -5138,6 +5143,27 @@ static bool cxl_sbr_masked(struct pci_dev *dev)
 	return true;
 }
 
+/*
+ * cxl_sbr_allowed - whether an SBR of a CXL Downstream Port may go ahead
+ * @dev: Downstream Port to test
+ *
+ * Per CXL r4.0 sec 8.1.5.2 Table 8-32 the SBR bit in a CXL Port's Bridge
+ * Control register has no effect while the Port's Unmask SBR bit is clear, and
+ * sec 9.12.3 says System Firmware may leave it clear "to prevent CXL-unaware
+ * PCIe software from resetting the device and the link". A Port that already
+ * has it set needs no further permission; otherwise unmasking it takes the
+ * administrator's consent, given by writing 1 to the Port's cxl_unmask_sbr.
+ *
+ * Return: true if the reset paths may unmask and generate an SBR of @dev.
+ */
+bool cxl_sbr_allowed(struct pci_dev *dev)
+{
+	if (!cxl_port_dvsec(dev))
+		return false;
+
+	return dev->cxl_unmask_sbr || !cxl_sbr_masked(dev);
+}
+
 static int pci_reset_bus_function(struct pci_dev *dev, bool probe)
 {
 	struct pci_dev *bridge = pci_upstream_bridge(dev);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index b6d873b077ed..bea05acc58f0 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -248,6 +248,7 @@ 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);
+bool cxl_sbr_allowed(struct pci_dev *dev);
 int pci_bus_error_reset(struct pci_dev *dev);
 int pci_try_reset_bridge(struct pci_dev *bridge);
 
-- 
2.55.0

Re: [PATCH v2 01/13] cxl/pci: Make the HDM and Mem_Enable writes callable from CXL

[sashiko-bot]

> Split the Global Control write out of devm_cxl_enable_hdm() as
> cxl_enable_hdm() and drop the static from cxl_set_mem_enable(), so both
> are reachable from the rest of cxl_core. Declare them in core.h.
> 
> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825022628.3651434-1-fabio.m.de.francesco@linux.intel.com?part=1

Re: [PATCH v2 09/13] PCI/CXL: Disable and re-enable CXL regions

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] `cxl_sbr_enable_regions()` silently ignores the return value of `cxl_sbr_collect_regions()`, leading to permanently disabled regions on allocation failure.
- [High] The global `cxl_sbr_region_ops` pointer can dangle if `cxl_driver_register()` fails, and lacks module pinning to protect against concurrent unloads.
- [High] `cxl_sbr_enable_regions()` unconditionally re-enables regions even if HDM decoder restoration fails.
- [Medium] The recommit path incorrectly treats legitimately missing HDM state (e.g., ports without devices, or emulated decoders) as a failure or logs spurious warnings.
--

--- Patch [9]: [PATCH v2 09/13] PCI/CXL: Disable and re-enable CXL regions ---
commit cfc12109c423de14f05ed1c759bffe4684c3373e
Author: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

PCI/CXL: Disable and re-enable CXL regions

A Secondary Bus Reset of a CXL Downstream Port needs every CXL region
routed through that Port disabled for the duration. This patch adds 
callbacks to collect and disable those regions before the reset, and 
subsequently restore HDM decoders and re-enable the regions afterward.

> diff --git a/drivers/cxl/core/dport_sbr.c b/drivers/cxl/core/dport_sbr.c
> index 233499bc1fad9..2b6f840e22a7d 100644
> --- a/drivers/cxl/core/dport_sbr.c
> +++ b/drivers/cxl/core/dport_sbr.c
[ ... ]
> +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(&regions);
> +
> +	cxl_sbr_collect_regions(dport_pci, &regions);

[Severity: High]
Does this code need to check the return value of cxl_sbr_collect_regions()?

If xa_insert() fails inside cxl_sbr_collect_regions():

	rc = xa_insert(regions, (unsigned long)cxlr, cxlr, GFP_KERNEL);
	if (rc) {
		put_device(&cxlr->dev);
		return rc;
	}

it returns an error and stops collecting. By discarding the return value here,
could this silently skip some regions and leave them permanently offline and
unbound?

> +
> +	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");

[Severity: Medium]
Is it possible for ports to legitimately have no HDM state (like ports without
devices or with emulated decoders)?

The save path in cxl_port_save_hdm_state() appears to correctly skip such
ports without saving state. If that happens, wouldn't this path log a spurious
warning? Furthermore, wouldn't the recommit path in
cxl_port_recommit_decoders() incorrectly treat missing states as an -ENODATA
failure?

[Severity: High]
Since cxl_sbr_recommit_decoders() returns void, how does this code handle a
hardware programming failure?

If cxl_sbr_recommit_decoders() fails to reprogram the HDM decoders (e.g., due
to device timeout), won't the subsequent loop unconditionally call
cxl_region_enable() and rebind the region driver while the hardware is still
misconfigured? Could this lead to invalid memory accesses?

> +
> +	xa_for_each(&regions, index, cxlr)
> +		cxl_region_enable(cxlr);
> +
> +	cxl_sbr_put_regions(&regions);
> +	cxl_sbr_drop_hdm_state(dport_pci);
> +}
[ ... ]
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index fc0bec991a693..d1dd4924fba17 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
[ ... ]
> @@ -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);
>  }

[Severity: High]
If cxl_driver_register() fails here, does the PCI subsystem retain a dangling
pointer to cxl_sbr_region_ops?

Also, since the PCI core doesn't hold a module reference, is there a risk of
the module being concurrently unloaded while a Secondary Bus Reset is in
progress?

>  
>  void cxl_region_exit(void)
>  {
>  	cxl_driver_unregister(&cxl_region_driver);
> +	pci_cxl_set_sbr_region_ops(NULL);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825022628.3651434-1-fabio.m.de.francesco@linux.intel.com?part=9

Re: [PATCH v2 06/13] drivers/base/memory: Add cxl_offline_memory() to offline a physical range

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] cxl_offline_memory() leaves memory blocks partially offlined and permanently stranded if device_offline() fails midway, lacking a state-tracking and rollback mechanism.
--

--- Patch [6]: [PATCH v2 06/13] drivers/base/memory: Add cxl_offline_memory() to offline a physical range ---
commit 00648cd16fe5b1311f9ee6c83c16afccf56f6b3d
Author: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

drivers/base/memory: Add cxl_offline_memory() to offline a physical range

A CXL region whose capacity is online as System RAM must be taken
offline before a Secondary Bus Reset of the CXL Downstream Port it is
reached through.

> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index bcfe2d9f4adbd..ce89b251d34ac 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c

[ ... ]

> @@ -1018,6 +1018,42 @@ int walk_memory_blocks(unsigned long start, unsigned long size,
>  	return ret;
>  }
>  
> +static int cxl_offline_memory_block(struct memory_block *mem, void *arg)
> +{
> +	int rc = device_offline(&mem->dev);
> +
> +	/* device_offline() returns a positive value when already offline. */
> +	if (rc > 0)
> +		return 0;
> +
> +	return rc;
> +}
> +
> +/**
> + * cxl_offline_memory - offline the memory blocks spanning a physical range

[ ... ]

> +int cxl_offline_memory(u64 start, u64 size)
> +{
> +	int rc;
> +
> +	lock_device_hotplug();
> +	rc = walk_memory_blocks(start, size, NULL, cxl_offline_memory_block);

[Severity: High]
If device_offline() fails midway through a range (for example, due to
unmovable or pinned pages), will this leave the system in an inconsistent
state with partially offlined memory ranges and silent capacity loss?

When walk_memory_blocks() aborts on an error from cxl_offline_memory_block(),
there doesn't appear to be a rollback mechanism for blocks that were already
successfully offlined.

Additionally, since cxl_offline_memory_block() treats an already-offline
block as a success but doesn't record that state, how could a caller
safely roll back without accidentally onlining a block that was originally
offline before cxl_offline_memory() was called? Core memory hotplug APIs
typically allocate an array to track original states to handle these failures.

> +	unlock_device_hotplug();
> +
> +	return rc;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825022628.3651434-1-fabio.m.de.francesco@linux.intel.com?part=6

Re: [PATCH v2 10/13] PCI/CXL: Clear ACS SV across an SBR of a CXL DPort

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unsynchronized access to the global `cxl_sbr_region_ops` struct pointer can result in a NULL pointer dereference or a Use-After-Free crash if the CXL core module unloads concurrently.
- [High] Missing link training wait for nested bridges during subordinate configuration space restoration.
- [High] Restoring the standard configuration space of subordinate devices re-enables Bus Master and Memory Space before their PCIe capabilities are restored, risking Malformed TLP fatal errors.
- [High] The completion function attempts to access the configuration space and memory regions of downstream devices even if the secondary bus reset fails.
- [Medium] The polling loop in `cxl_port_pm_init_is_complete()` can miss a successful completion if scheduling delays cause `msleep()` to overshoot the timeout.
--

--- Patch [10]: [PATCH v2 10/13] PCI/CXL: Clear ACS SV across an SBR of a CXL DPort ---
commit fae09a9aaf20d6e342f823e3752512b2c39cdd95
Author: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

PCI/CXL: Clear ACS SV across an SBR of a CXL DPort

A Secondary Bus Reset (SBR) of a CXL Downstream Port clears the
Downstream Component's (DPort) captured Bus Number. This patch
clears ACS Source Validation and Bus Master Enable across the reset
so that Port Power Management Initialization completes at link-up.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 417b6b44473e5..f3781d1e8f6e1 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
[ ... ]
> +static int cxl_sbr_prepare(struct pci_dev *bridge, u16 dvsec,
> +			   struct cxl_sbr_ctx *ctx)
> +{
> +	int rc;
> +
> +	/* Abort before touching hardware if the regions cannot be disabled. */
> +	if (cxl_sbr_region_ops) {
> +		rc = cxl_sbr_region_ops->disable_regions(bridge);
> +		if (rc)
> +			return rc;
> +	}

[Severity: High]
Could accessing cxl_sbr_region_ops here result in a use-after-free or NULL
pointer dereference?

If a secondary bus reset is initiated via sysfs concurrently with the
cxl_core module unloading, the pointer might be cleared or freed before
disable_regions() is called. There does not appear to be any lock or RCU
protection here.

[ ... ]
> +static bool cxl_port_pm_init_is_complete(struct pci_dev *bridge, u16 dvsec)
> +{
> +	unsigned long start = jiffies;
> +	unsigned long timeout = start + msecs_to_jiffies(100);
> +	u16 status;
> +
> +	do {
> +		pci_read_config_word(bridge,
> +				     dvsec + PCI_DVSEC_CXL_PORT_EXT_STATUS,
> +				     &status);
> +		if (!PCI_POSSIBLE_ERROR(status) &&
> +		    (status & PCI_DVSEC_CXL_PORT_EXT_STATUS_PM_INIT_COMP)) {
> +			pci_dbg(bridge, "%s: PM Init Complete set after %u ms, ext status %#06x\n",
> +				__func__, jiffies_to_msecs(jiffies - start), status);
> +			return true;
> +		}
> +		msleep(10);
> +	} while (time_before(jiffies, timeout));
> +
> +	pci_warn(bridge, "%s: PM Init Complete not set after %u ms, ext status %#06x\n",

[Severity: Medium]
Will this loop miss successful completions if scheduling delays cause
msleep() to overshoot the timeout?

If the sleep significantly exceeds 10ms and crosses the 100ms timeout
boundary, the loop terminates and returns false without a final status
read. This might lead to spurious warnings and premature restoration of
ACS Source Validation, causing the Port to reject bus 0 traffic.

[ ... ]
> +static int cxl_sbr_restore_config_space(struct pci_dev *dev, void *userdata)
> +{
> +	pci_restore_config_space(dev);
> +	pci_dbg(dev, "%s: config space restored\n", __func__);
> +
> +	return 0;
> +}

[Severity: High]
Does this unconditionally re-enable Bus Master and Memory Space before
PCIe capabilities are restored?

When pci_restore_config_space() writes the first 64 bytes, it restores
PCI_COMMAND while the Max Payload Size (MPS) remains at its hardware
reset default of 128 bytes. This might trigger Malformed TLP fatal errors
if the device interacts with a host configured for a larger MPS during
this window.

[ ... ]
> +static void cxl_sbr_restore_subordinate(struct pci_dev *bridge)
> +{
> +	if (!bridge->subordinate)
> +		return;
> +
> +	/* Parents before children: a child answers once its parent forwards. */
> +	pci_walk_bus(bridge->subordinate, cxl_sbr_restore_config_space, NULL);
> +}

[Severity: High]
Does pci_walk_bus() safely handle nested PCI-to-PCI bridges on the
subordinate bus?

It seems to restore the nested bridge's standard configuration and then
immediately traverse to the grandchildren without waiting for the nested
bridge's secondary link to retrain. This means configuration writes might
be sent to deeper endpoints while their link is still down.

[ ... ]
>  int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
>  {
> +	struct cxl_sbr_ctx ctx = {};
> +	u16 dvsec = 0;
> +	int rc;
> +
>  	if (!dev->block_cfg_access)
>  		pci_warn_once(dev, "unlocked secondary bus reset via: %pS\n",
>  			      __builtin_return_address(0));
> +
> +	if (is_cxl_dport(dev))
> +		dvsec = cxl_port_dvsec(dev);
> +	if (dvsec) {
> +		rc = cxl_sbr_prepare(dev, dvsec, &ctx);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	pcibios_reset_secondary_bus(dev);
>  
> -	return pci_bridge_wait_for_secondary_bus(dev, "bus reset");
> +	rc = pci_bridge_wait_for_secondary_bus(dev, "bus reset");
> +
> +	if (dvsec)
> +		cxl_sbr_complete(dev, dvsec, &ctx);
> +
> +	return rc;
>  }

[Severity: High]
Is it safe to call cxl_sbr_complete() when
pci_bridge_wait_for_secondary_bus() times out or returns a non-zero error?

Accessing the configuration space and memory regions of downstream devices
while the secondary link is physically down might generate uncorrectable
PCIe AER errors like Unsupported Requests or Completion Timeouts.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825022628.3651434-1-fabio.m.de.francesco@linux.intel.com?part=10

Re: [PATCH v2 08/13] cxl/core: Collect the regions routed through a DPort

[sashiko-bot]

> Disabling the CXL regions ahead of a Secondary Bus Reset needs the set of
> regions with a member endpoint below the Port being reset.
> 
> Add cxl_sbr_collect_regions() to walk the CXL port that owns the Downstream
> Port and return every region with an endpoint below it, and
> cxl_sbr_put_regions() to release the set.
> 
> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825022628.3651434-1-fabio.m.de.francesco@linux.intel.com?part=8

Re: [PATCH v2 02/13] cxl/hdm: Add function to restore one HDM decoder

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] `cxl_decoder_recommit()` falsely reports success if the hardware is disconnected and the MMIO read returns `~0`.
--

--- Patch [2]: [PATCH v2 02/13] cxl/hdm: Add function to restore one HDM decoder ---
commit 15afab37836901527b4313972b91d202d46a3f32
Author: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

cxl/hdm: Add function to restore one HDM decoder

This commit adds cxl_decoder_recommit() to restore a single HDM decoder's
settings if hardware no longer reports it as committed after a reset. The
function reprograms the registers and commits the changes, which is a step
toward restoring decoders below a reset port.

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 0c80b76a5f9b4..d55d764895f88 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
[ ... ]
> @@ -849,6 +849,77 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  	return 0;
>  }
>  
> +/**
> + * cxl_decoder_recommit - reprogram @cxld's HDM decoder registers and commit
[ ... ]
> +int cxl_decoder_recommit(struct cxl_decoder *cxld, u32 ctrl)
> +{
> +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> +	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> +	u32 hw_ctrl;
> +	int rc;
> +
> +	if ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)
> +		return 0;
> +
> +	if (!cxld->commit)
> +		return 0;
> +
> +	hw_ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> +	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, hw_ctrl)) {

[Severity: High]
If the hardware is disconnected or failed to recover from reset, readl() could
return ~0 (all 1s). In that case, FIELD_GET() will evaluate the COMMITTED bit
as true, causing the function to immediately return 0 (success) without
reprogramming the decoder or checking for COMMIT_ERROR.

Could this lead to falsely reporting successful decoder restoration during a
hardware failure, potentially routing traffic to a dead path?

> +		dev_dbg(&cxld->dev, "%s: still committed, no reprogram needed\n",
> +			__func__);
> +		return 0;
> +	}
> +
> +	writel(ctrl & ~(CXL_HDM_DECODER0_CTRL_COMMIT |
> +			CXL_HDM_DECODER0_CTRL_COMMITTED |
> +			CXL_HDM_DECODER0_CTRL_COMMIT_ERROR),
> +	       hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825022628.3651434-1-fabio.m.de.francesco@linux.intel.com?part=2

Re: [PATCH v2 05/13] cxl/core: Restore the HDM decoders below DPort

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Lockless iteration of `port->endpoints` using `xa_for_each` in `cxl_port_recommit_decoders()` lacks protection from concurrent endpoint removal, leading to Use-After-Free.
- [Low] Unused local variable 'dport' whose address is passed to a function but never read.

Pre-existing issues:
- [High] Returning cxl_dport from find_cxl_port() leads to use-after-free because its lifecycle is managed by devres and is not tied to the port's reference count.
--

--- Patch [5]: [PATCH v2 05/13] cxl/core: Restore the HDM decoders below DPort ---
Due to system constraints regarding recitation of copyrighted or proprietary
material, this review is provided as a free-form summary without direct
inline quotes of the patch code.

commit c3d66df5869718f0ad0a7763a605cb85c574d23c
Author: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

cxl/core: Restore the HDM decoders below DPort

This commit adds cxl_sbr_recommit_decoders() to restore HDM decoder settings
for CXL components below a Downstream Port after a Secondary Bus Reset. It
reads the endpoint set and switch decoder target lists while holding the
cxl_rwsem.region lock. It also drops the static declaration of find_cxl_port()
so it can be used outside of port.c.

[Severity: Low]
In drivers/cxl/core/dport_sbr.c, inside cxl_sbr_recommit_decoders(), the local
variable dport is declared and its address is passed to find_cxl_port(). Since
dport is never read or accessed again after this call, is this variable
necessary, or can we pass NULL if the downstream port is not needed?

[Severity: High]
In drivers/cxl/core/hdm.c, cxl_port_recommit_decoders() uses xa_for_each() to
iterate over port->endpoints. Does this lack protection from concurrent
endpoint removal?

The lock held during this walk is cxl_rwsem.region. Concurrently, a sysfs
unbind could trigger cxl_detach_ep(), which holds device_lock(&port->dev)
and frees the port_ep structure via kfree(). Because xa_for_each() internally
acquires and drops the RCU read lock per iteration, the port_ep pointer can
become dangling before the loop body dereferences port_ep->ep, resulting in a
use-after-free.

[Severity: High]
In drivers/cxl/core/port.c, find_cxl_port() populates and returns the dport
pointer via the cxl_find_port_ctx struct.

This is a pre-existing issue, but does retrieving the cxl_dport pointer this
way lead to a potential use-after-free? The cxl_dport memory is managed by
devres and is freed when the port's host device unbinds. It is not pinned by
the returned cxl_port's reference count. If a caller holds the port reference
but the port's host unbinds concurrently, accessing the dport pointer will
result in a use-after-free. Exporting find_cxl_port() to the rest of the
subsystem expands this vulnerable API surface.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825022628.3651434-1-fabio.m.de.francesco@linux.intel.com?part=5

Re: [PATCH v2 07/13] cxl/core: Add region disable and enable for a DPort SBR

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] CPU caches are invalidated before the DAX region is unbound, leaving a TOCTOU window where caches can be re-dirtied, leading to fatal Machine Check Exceptions.
- [High] `cxl_region_disable()` blindly dereferences `p->res` before verifying that the region has an assigned resource, causing a guaranteed kernel panic on unconfigured regions.
- [Medium] `cxl_region_disable` offlines System RAM before checking if CPU cache invalidation is supported, leading to unnecessary and permanent memory loss on unsupported architectures.
- [Low] `cxl_region_enable()` misinterprets the return value of `device_attach()`, treating a failure to bind as a success.
--

--- Patch [7]: [PATCH v2 07/13] cxl/core: Add region disable and enable for a DPort SBR ---
commit d35a413a3760382cbad793ba438fa85c779b3677
Author: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

cxl/core: Add region disable and enable for a DPort SBR

A Secondary Bus Reset of a CXL Downstream Port removes the downstream
component from the bus, so a CXL region reached through that Port has to
be disabled while the reset runs.

> diff --git a/drivers/cxl/core/dport_sbr.c b/drivers/cxl/core/dport_sbr.c
> index 55c35386d0cb1..0f398ab0d3a41 100644
> --- a/drivers/cxl/core/dport_sbr.c
> +++ b/drivers/cxl/core/dport_sbr.c
> @@ -1,11 +1,90 @@

[ ... ]

> +int cxl_region_disable(struct cxl_region *cxlr)
> +{
> +	struct cxl_region_params *p = &cxlr->params;
> +	unsigned long block_size;
> +	u64 start, end;
> +	int rc;
> +
> +	/*
> +	 * Per CXL r4.0 sec 9.13.1 an Interleave Set has a Base HPA and a Size
> +	 * that are multiples of 256 MB, while a memory block spans up to 2 GB.
> +	 * A block overlapping either end of the range therefore also covers
> +	 * memory outside this region, so round the range inward to block
> +	 * granularity as dax_kmem did when it onlined the range. Offlining a
> +	 * straddling block would migrate pages that the reset does not affect.
> +	 */
> +	block_size = memory_block_size_bytes();
> +	start = ALIGN(p->res->start, block_size);
> +	end = ALIGN_DOWN(p->res->end + 1, block_size);

[Severity: High]
If a region is partially configured, such as when an administrator clears its
size via sysfs (free_hpa()), p->res can be NULL. 

SBR can still collect this region if it has endpoints attached.

Would dereferencing p->res->start and p->res->end here unconditionally cause a
kernel panic for unconfigured regions?

> +	if (start >= end) {
> +		dev_dbg(&cxlr->dev, "%s: HPA %pr spans no whole memory block, no System RAM to offline\n",
> +			__func__, p->res);
> +	} else {
> +		rc = cxl_offline_memory(start, end - start);
> +		if (rc) {
> +			dev_warn(&cxlr->dev, "offline System RAM failed before reset: %d\n",
> +				 rc);
> +			return rc;
> +		}
> +	}
> +
> +	rc = cxl_region_invalidate_memregion(cxlr);

[Severity: Medium]
Could this sequence permanently strand System RAM?

If the system lacks CPU cache invalidation support,
cxl_region_invalidate_memregion() will fail and return -ENXIO.

Because cxl_offline_memory() has already succeeded at this point, the memory
is left offline indefinitely. When the SBR is aborted, the subsequent
cxl_region_enable() will only rebind the driver and won't bring the memory
back online.

> +	if (rc) {
> +		dev_warn(&cxlr->dev, "CPU cache invalidate failed before reset: %d\n",
> +			 rc);
> +		return rc;
> +	}
> +
> +	device_release_driver(&cxlr->dev);

[Severity: Critical]
Is it safe to invalidate the CPU cache before unbinding the DAX driver?

Since cxl_offline_memory() is a no-op for DAX regions, userspace PTE mappings
remain fully active. By invalidating the cache before calling
device_release_driver(), a TOCTOU window is opened where an unprivileged
process could dirty cachelines again. 

If this happens, when the downstream port is reset via SBR, any CPU eviction
of those dirty cachelines will trigger a PCIe Unsupported Request and escalate
into a fatal Machine Check Exception, crashing the host.

> +	dev_dbg(&cxlr->dev, "%s: System RAM offline, region disabled before reset, HPA %pr\n",
> +		__func__, p->res);
> +
> +	return 0;
> +}
> +

[ ... ]

> +void cxl_region_enable(struct cxl_region *cxlr)
> +{
> +	struct cxl_region_params *p = &cxlr->params;
> +
> +	if (device_attach(&cxlr->dev) < 0) {

[Severity: Low]
Does this correctly handle the case when no driver was found? 

device_attach() returns 0 when no driver was found, 1 on success, and < 0 on
error. Checking for < 0 here effectively treats 0 as a success.

This could lead to a misleading debug log that says the region was successfully
re-enabled even when it failed to bind a driver.

> +		dev_dbg(&cxlr->dev, "driver re-attach failed after reset\n");
> +		return;
> +	}
> +
> +	dev_dbg(&cxlr->dev, "%s: region re-enabled after reset, HPA %pr, IW %d, IG %d\n",
> +		__func__, p->res, p->interleave_ways, p->interleave_granularity);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825022628.3651434-1-fabio.m.de.francesco@linux.intel.com?part=7

Re: [PATCH v2 03/13] cxl/hdm: Add function to restore CXL.mem decode

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] `cxl_trigger_poison_list()` can trigger a kernel panic by dereferencing an ERR_PTR when checking `is_cxl_endpoint(port)`.
--

--- Patch [3]: [PATCH v2 03/13] cxl/hdm: Add function to restore CXL.mem decode ---
commit 3dc5dc9495c42e77bea8b303acf10562a2ca0f7a
Author: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

cxl/hdm: Add function to restore CXL.mem decode

This commit adds a function to put an endpoint back in HDM decode before
its decoders are committed again. It takes the Global Control value to
enable decode and skips endpoints that decode through DVSEC ranges.

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index d55d764895f88..7c898c60f60f6 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c

[ ... ]

> +int cxl_endpoint_enable_hdm_decode(struct cxl_memdev *cxlmd, u32 global_ctrl)
> +{
> +	struct cxl_port *endpoint = cxlmd->endpoint;

[Severity: High]
This is a pre-existing issue, but since we are accessing cxlmd->endpoint
here, can this endpoint be an error pointer that causes a panic elsewhere?

Looking at cxl_trigger_poison_list() in drivers/cxl/core/memdev.c, if a
privileged user writes to the sysfs attribute trigger_poison_list early:

    port = cxlmd->endpoint;
    if (!port || !is_cxl_endpoint(port))
        return -EINVAL;

If the sysfs attribute is accessed before bus enumeration completes, or if
enumeration fails on a device lacking DVSEC capabilities, cxlmd->endpoint
remains set to ERR_PTR(-ENXIO).

The !port check evaluates to false for an ERR_PTR, and the error pointer is
then passed to is_cxl_endpoint(), which blindly dereferences it causing an
invalid memory access and a kernel panic.

Could this be prevented by using IS_ERR_OR_NULL() instead when validating
the port?

> +	struct cxl_hdm *cxlhdm = dev_get_drvdata(&endpoint->dev);
> +	int rc;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825022628.3651434-1-fabio.m.de.francesco@linux.intel.com?part=3

Re: [PATCH v2 11/13] PCI/DPC: Unbind regions for DPC recovery

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] DPC Trigger Status is cleared before relying on DPC containment, allowing uncontained fatal MMIO reads.
- [Critical] Bypassing driver unbind in `cxl_error_detected` exposes the system to MCE from concurrent MMIO accesses during SBR.
- [High] Ignoring `cxl_sbr_collect_regions()` return value leaves CXL regions silently bound or permanently detached.
- [High] Unguarded secondary bus reset and config space data race due to missing PCI config access lock.
- [High] Unbinding the CXL region driver without offlining its memory causes `dax_kmem` to leak physical memory reservations, preventing reprobe.
- [High] Regions are unconditionally re-enabled even if HDM decoder state restoration fails.
--

--- Patch [11]: [PATCH v2 11/13] PCI/DPC: Unbind regions for DPC recovery ---
commit e04d0da5a53882402af54024d29f830953f6e9bc
Author: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

PCI/DPC: Unbind regions for DPC recovery

This commit splits pci_bridge_secondary_bus_reset() to allow unbinding
CXL regions routed through a port during DPC recovery without offlining
their memory. It keeps the memdev driver bound during containment so the
endpoint and decoders can be restored once the link comes back up.

> diff --git a/drivers/cxl/core/dport_sbr.c b/drivers/cxl/core/dport_sbr.c
> index 2b6f840e22a7d..823b63012f45b 100644
> --- a/drivers/cxl/core/dport_sbr.c
> +++ b/drivers/cxl/core/dport_sbr.c
[ ... ]
> @@ -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");

[Severity: High]
What happens during recovery if cxl_sbr_save_hdm_state() fails here?

If the state fails to save (e.g., due to a memory allocation failure), we log
a warning but proceed. Later, during recovery in cxl_sbr_enable_regions(),
it appears the code will unconditionally call cxl_region_enable() on all
regions even if no HDM state was restored. Does this risk OS memory mappings
pointing to unprogrammed hardware, which could lead to Machine Check Exceptions
on access?

> +
> +	xa_init(&regions);
> +
> +	cxl_sbr_collect_regions(dport_pci, &regions);

[Severity: High]
Does ignoring the return value of cxl_sbr_collect_regions() mean we might
operate on an incomplete list of regions?

If collection encounters an error (such as an allocation failure in xa_insert),
it aborts early and returns a partial list. Uncollected regions would then
be left bound during the reset, meaning any memory accesses to them might
generate uncontained errors. Similarly, when re-enabling, any regions that
failed to collect wouldn't get re-enabled.

> +
> +	xa_for_each(&regions, index, cxlr)
> +		cxl_region_unbind(cxlr);

[Severity: High]
Could unbinding the region driver here without offlining its memory cause
dax_kmem to leak physical memory reservations?

If cxl_region_unbind() tears down the DAX devices while the memory is still
online, remove_memory() will fail. The driver intentionally leaks the
request_mem_region() allocation when this happens. Would this prevent
us from successfully reprobing the region after recovery completes?

> +
> +	cxl_sbr_put_regions(&regions);
> +}
[ ... ]
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index 99fb00949c2fa..568a906f3b50f 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;

[Severity: Critical]
Is it safe to bypass device_release_driver() here?

By returning PCI_ERS_RESULT_NEED_RESET without unbinding the memdev driver,
the device remains fully exposed to userspace. During the Secondary Bus Reset,
the device is unresponsive. If a user issues an ioctl (like CXL_MEM_SEND_COMMAND)
or reads sysfs attributes during this window, the driver will perform MMIO reads
to the mailbox registers.

Could these non-posted reads hit uncontained Unsupported Requests while the
link is in reset and crash the system?

> +
>  		dev_warn(&pdev->dev,
>  			 "%s: frozen state error detected, disable CXL.mem\n",
>  			 dev_name(dev));
[ ... ]
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 2b779bd1d861b..ad74086bf82c7 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
[ ... ]
> @@ -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);

[Severity: Critical]
Could clearing the DPC Trigger Status here lead to fatal MMIO reads?

Right after this clears PCI_EXP_DPC_STATUS_TRIGGER (which pulls the port
out of DPC containment), __pci_bridge_secondary_bus_reset() is called.
This eventually invokes cxl_sbr_save_hdm_state(), which performs non-posted
MMIO reads to downstream endpoints:

drivers/cxl/core/dport_sbr.c:cxl_sbr_save_hdm_state() {
    ...
    scoped_guard(rwsem_read, &cxl_rwsem.region)
        rc = cxl_port_save_hdm_state(port, hdm_state);
}

Since the link is still down but DPC containment has just been lifted,
would the Root Port treat these reads as uncontained Completer Aborts
or Unsupported Requests and trigger a system crash?

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

[Severity: High]
Are we missing a lock for the PCI config space here?

Calling __pci_bridge_secondary_bus_reset() without holding
pci_cfg_access_lock(pdev) triggers a lock assertion warning:

drivers/pci/pci.c:__pci_bridge_secondary_bus_reset() {
    if (!dev->block_cfg_access)
        pci_warn_once(dev, "unlocked secondary bus reset via: %pS\n", ...);
}

Could running this unlocked allow concurrent operations (like sysfs resets
or hotplug) to corrupt the PCI config space during a DPC event?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825022628.3651434-1-fabio.m.de.francesco@linux.intel.com?part=11

Re: [PATCH v2 12/13] PCI/CXL: Add a sysfs entry to unmask SBR

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Concurrent writes to the `cxl_unmask_sbr` bitfield via the sysfs store function can corrupt adjacent bitfields in `struct pci_dev` due to non-atomic read-modify-write operations.
--

--- Patch [12]: [PATCH v2 12/13] PCI/CXL: Add a sysfs entry to unmask SBR ---
commit d915ae90bb0b4f81c466e6e9be4cc5b40f737fbb
Author: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

PCI/CXL: Add a sysfs entry to unmask SBR

A Secondary Bus Reset of a CXL Downstream Port only reaches the component
below it when the Port's Unmask SBR bit is set. This commit adds a
cxl_unmask_sbr sysfs attribute to allow the kernel to trigger an SBR of
CXL DPorts.

> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
[ ... ]
> @@ -573,6 +573,33 @@ static ssize_t reset_subordinate_store(struct device *dev,
>  }
>  static DEVICE_ATTR_WO(reset_subordinate);
>  
> +static ssize_t cxl_unmask_sbr_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	return sysfs_emit(buf, "%u\n", pdev->cxl_unmask_sbr);
> +}
> +
> +static ssize_t cxl_unmask_sbr_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	unsigned long val;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	if (kstrtoul(buf, 0, &val) < 0)
> +		return -EINVAL;
> +
> +	pdev->cxl_unmask_sbr = !!val;

[Severity: High]
Does writing to this bitfield without synchronization risk corrupting adjacent
state flags in struct pci_dev?

Modifying a C bitfield is a non-atomic read-modify-write operation on the
containing 32-bit word.

If a user writes to this sysfs file while the kernel concurrently modifies
other flags sharing this word (like is_busmaster, msi_enabled, or
state_saved), could this silently overwrite those updates?

> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(cxl_unmask_sbr);

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
[ ... ]
> @@ -485,6 +485,7 @@ struct pci_dev {
>  	unsigned int	shpc_managed:1;		/* SHPC owned by shpchp */
>  	unsigned int	is_thunderbolt:1;	/* Thunderbolt controller */
>  	unsigned int	is_cxl:1;               /* Compute Express Link (CXL) */
> +	unsigned int	cxl_unmask_sbr:1;	/* SBR unmask allowed by user */
>  	/*
>  	 * Devices marked being untrusted are the ones that can potentially
>  	 * execute DMA attacks and similar. They are typically connected

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825022628.3651434-1-fabio.m.de.francesco@linux.intel.com?part=12

Re: [PATCH v2 04/13] cxl/hdm: Reprogram the HDM Decoders below a CXL Port

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Valid ports intentionally lacking HDM decoder state (e.g., DVSEC endpoints or passthrough switches) incorrectly trigger an error that aborts topology restoration.
- [High] The HDM Decoder Global Control register is saved for all ports but is only written back for endpoints, leaving switch HDM decoding globally disabled.
- [High] Lockless iteration over `port->endpoints` using `xa_for_each()` leads to a Use-After-Free of `struct cxl_ep` when endpoints are concurrently removed.
- [High] Lockless access to `iter->dev` driver data leads to Use-After-Free of `struct cxl_hdm` if the port driver is concurrently unbound.
- [Medium] Recommitting decoders on shared switches during a per-endpoint path walk prematurely commits sibling decoders, violating the bottom-up programming requirement.
--

--- Patch [4]: [PATCH v2 04/13] cxl/hdm: Reprogram the HDM Decoders below a CXL Port ---
commit 8ab001ee7d558b0995b7f0cff3266fb8cd0949bb
Author: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

cxl/hdm: Reprogram the HDM Decoders below a CXL Port

A reset of a CXL Port clears the HDM Decoders of every component below it,
so the decode has to be put back for every endpoint the Port routes to.
This patch adds cxl_port_recommit_decoders() and cxl_port_save_hdm_state()
to save and restore HDM decoder states across a reset.

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 7c898c60f60f6..9839d2592d206 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
[ ... ]
> @@ -935,6 +935,42 @@ static bool cxl_endpoint_decoders_are_emulated(struct cxl_port *endpoint)
>  				     __cxl_endpoint_decoder_is_emulated);
>  }
>  
> +struct cxl_recommit_ctx {
> +	const struct cxl_hdm_state *state;
> +	int *first_rc;
> +};
> +
> +static int __cxl_port_recommit_decoder(struct device *dev, void *data)
> +{
> +	struct cxl_recommit_ctx *ctx = data;
> +	struct cxl_decoder *cxld;
> +	int rc;
> +
> +	if (!(is_switch_decoder(dev) || is_endpoint_decoder(dev)))
> +		return 0;
> +
> +	cxld = to_cxl_decoder(dev);
> +
> +	if (cxld->id >= ctx->state->nr_ctrl) {
> +		dev_warn(&cxld->dev, "%s: no saved control register\n",
> +			 __func__);
> +		if (!*ctx->first_rc)
> +			*ctx->first_rc = -ENODATA;
> +		return 0;
> +	}
> +
> +	/*
> +	 * Reprogram every decoder the walk reaches. Stopping at the first
> +	 * failure would leave the rest of the path decoding nothing, so record
> +	 * the first error and continue.
> +	 */
> +	rc = cxl_decoder_recommit(cxld, ctx->state->ctrl[cxld->id]);

[Severity: Medium]
Since this function is called for every child decoder of a switch port,
could it unconditionally recommit sibling decoders before their downstream
path is reprogrammed?

If multiple endpoints route through a shared switch, reprogramming the
switch during the first endpoint's traversal might commit decoders for
other endpoints whose downstream paths haven't been restored yet,
violating bottom-up programming requirements.

> +	if (rc && !*ctx->first_rc)
> +		*ctx->first_rc = rc;
> +
> +	return 0;
> +}
[ ... ]
> +int cxl_port_recommit_decoders(struct cxl_port *port, struct xarray *hdm_state)
> +{
> +	struct cxl_ep *port_ep;
> +	unsigned long index;
> +	int first_rc = 0;
> +
> +	lockdep_assert_held(&cxl_rwsem.region);
> +
> +	xa_for_each(&port->endpoints, index, port_ep) {

[Severity: High]
Does this lockless iteration risk a use-after-free of port_ep?

If a concurrent unbind of a cxl_mem driver invokes cxl_detach_ep(), the
endpoint is removed from the xarray and the struct cxl_ep is freed without
an RCU grace period. Since xa_for_each() doesn't hold the RCU read lock
across the loop body, port_ep could be freed before dereferencing
port_ep->ep.

> +		struct cxl_memdev *cxlmd = to_cxl_memdev(port_ep->ep);
> +		struct cxl_hdm_state *state;
> +		struct cxl_port *iter;
> +		int rc;
> +
> +		if (IS_ERR_OR_NULL(cxlmd->endpoint))
> +			continue;
> +
> +		state = xa_load(hdm_state, (unsigned long)cxlmd->endpoint);
> +		if (!state) {
> +			dev_warn(&cxlmd->dev, "%s: no saved HDM state\n",
> +				 __func__);
> +			if (!first_rc)
> +				first_rc = -ENODATA;
> +			continue;

[Severity: High]
If a valid port intentionally lacks HDM decoder state (e.g., a DVSEC endpoint
or passthrough switch), will this skip processing its upstream path?

This skip appears to completely abort topology restoration for the upstream
path when legitimately missing HDM state is encountered.

> +		}
> +
> +		rc = cxl_endpoint_enable_hdm_decode(cxlmd, state->global_ctrl);
> +		if (rc) {
> +			dev_warn(&cxlmd->dev,
> +				 "%s: failed to enable HDM decode: %d\n",
> +				 __func__, rc);
> +			if (!first_rc)
> +				first_rc = rc;
> +			continue;
> +		}
> +
> +		/*
> +		 * Walk from the endpoint up to @port so a decoder is committed
> +		 * only after the decoder it routes to. @port is the last parent
> +		 * visited by the walk, and it is excluded.
> +		 */
> +		for (iter = cxlmd->endpoint; iter && iter != port;
> +		     iter = parent_port_of(iter)) {

[Severity: High]
Does this loop miss restoring the HDM Decoder Global Control register for
switch ports?

While cxl_endpoint_enable_hdm_decode() writes state->global_ctrl for endpoints,
this upstream walk only restores Decoder N Control registers for switch ports
via __cxl_port_recommit_decoder(). The Global Control register for switches
is saved but never seems to be written back, leaving switch HDM decoding
globally disabled.

> +			struct cxl_recommit_ctx ctx = {
> +				.state = xa_load(hdm_state, (unsigned long)iter),
> +				.first_rc = &first_rc,
> +			};
> +
> +			if (!ctx.state) {
> +				dev_warn(&iter->dev, "%s: no saved HDM state\n",
> +					 __func__);
> +				if (!first_rc)
> +					first_rc = -ENODATA;
> +				continue;

[Severity: High]
Similar to the endpoint check above, could this erroneously throw -ENODATA
and skip reprogramming if a valid passthrough switch legitimately lacks
HDM state?

> +			}
> +
> +			device_for_each_child(&iter->dev, &ctx,
> +					      __cxl_port_recommit_decoder);

[Severity: Medium]
As mentioned earlier, does this loop inherently break bottom-up ordering
rules on shared switches by recommitting all of the switch's decoders during
the first endpoint's traversal?

> +		}
> +	}
> +
> +	return first_rc;
> +}
[ ... ]
> +int cxl_port_save_hdm_state(struct cxl_port *port, struct xarray *hdm_state)
> +{
> +	struct cxl_ep *port_ep;
> +	unsigned long index;
> +
> +	lockdep_assert_held(&cxl_rwsem.region);
> +
> +	xa_for_each(&port->endpoints, index, port_ep) {

[Severity: High]
Does this have the same use-after-free risk for port_ep as the loop in
cxl_port_recommit_decoders()?

> +		struct cxl_memdev *cxlmd = to_cxl_memdev(port_ep->ep);
> +		struct cxl_port *iter;
> +
> +		if (IS_ERR_OR_NULL(cxlmd->endpoint))
> +			continue;
> +
> +		for (iter = cxlmd->endpoint; iter && iter != port;
> +		     iter = parent_port_of(iter)) {
> +			struct cxl_hdm *cxlhdm = dev_get_drvdata(&iter->dev);

[Severity: High]
Could this result in a use-after-free of cxlhdm if the port driver is
concurrently unbound?

Accessing driver data via dev_get_drvdata() while holding only
cxl_rwsem.region doesn't block driver unbind. A concurrent unbind
could release the devres allocations, freeing struct cxl_hdm before its
members are accessed below.

> +			struct cxl_hdm_state *state;
> +			void __iomem *hdm;
> +			int rc;
> +
> +			if (xa_load(hdm_state, (unsigned long)iter))
> +				continue;
> +
> +			if (!cxlhdm || !cxlhdm->regs.hdm_decoder)
> +				continue;
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825022628.3651434-1-fabio.m.de.francesco@linux.intel.com?part=4

Re: [PATCH v2 13/13] PCI/CXL: Refuse an SBR of a CXL DPort unless authorized

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] cxl_sbr_unbind_regions() and cxl_sbr_enable_regions() ignore the return value of cxl_sbr_collect_regions().
--

--- Patch [13]: [PATCH v2 13/13] PCI/CXL: Refuse an SBR of a CXL DPort unless authorized ---
commit 8a167a59c2870bad236e079ff1641933532c8a30
Author: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

PCI/CXL: Refuse an SBR of a CXL DPort unless authorized

This commit prevents the Secondary Bus Reset (SBR) bit from being
silently unmasked if it is left at the firmware default. It adds
cxl_sbr_allowed() to verify whether a CXL Downstream Port has its
Unmask SBR bit set, requiring explicit authorization otherwise.

> diff --git a/drivers/cxl/core/dport_sbr.c b/drivers/cxl/core/dport_sbr.c
> new file mode 100644
> index 0000000000000..823b63012f45b
> --- /dev/null
> +++ b/drivers/cxl/core/dport_sbr.c

[ ... ]

> +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(&regions);
> +
> +	cxl_sbr_collect_regions(dport_pci, &regions);

[Severity: High]
Does this code silently ignore memory allocation failures during region
collection?

If xa_insert() in cxl_sbr_collect_regions() returns -ENOMEM under memory
pressure, the error appears to be ignored, resulting in a partial unbind
before a bus reset.

Could this leave uncollected regions bound while the link is reset, leading
to Machine Check Exceptions if the memory is accessed while still online?

> +
> +	xa_for_each(&regions, index, cxlr)
> +		cxl_region_unbind(cxlr);
> +
> +	cxl_sbr_put_regions(&regions);
> +}

[ ... ]

> +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(&regions);
> +
> +	cxl_sbr_collect_regions(dport_pci, &regions);

[Severity: High]
Is the return value of cxl_sbr_collect_regions() intentionally ignored here
as well?

If region collection fails, it seems the uncollected regions would skip the
enable step and permanently leak their disabled state.

> +
> +	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(&regions, index, cxlr)
> +		cxl_region_enable(cxlr);
> +
> +	cxl_sbr_put_regions(&regions);
> +	cxl_sbr_drop_hdm_state(dport_pci);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825022628.3651434-1-fabio.m.de.francesco@linux.intel.com?part=13

Re: [PATCH v2 10/13] PCI/CXL: Clear ACS SV across an SBR of a CXL DPort

[Richard Cheng]

On Tue, Aug 25, 2026 at 04:26:25AM +0800, Fabio M. De Francesco wrote:
> A Secondary Bus Reset (SBR) of a CXL Downstream Port clears the
> Downstream Component's (DPort) captured Bus Number. CXL r4.0 sec 8.1.5.1
> notes that if a Downstream Component issues PM Init messages to a DPort
> with Access Control Services (ACS) Source Validation (SV) bit enabled,
> PM Initialization may then fail to complete, and that a further SBR
> alone does not recover it. PCIe r7.0 sec 6.12.1.1 makes that rejection
> an error that is a reported as ACS Violation.
> 
> Reuse the recovery sequence described in sec 8.1.5.1 but make it
> preventive, so that PM Init don't fail and don't need to be recovered.
> 
> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> ---
>  drivers/pci/pci.c             | 177 ++++++++++++++++++++++++++++++++--
>  include/uapi/linux/pci_regs.h |   2 +
>  2 files changed, 172 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 417b6b44473e..f3781d1e8f6e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -23,6 +23,7 @@
>  #include <linux/module.h>
>  #include <linux/spinlock.h>
>  #include <linux/string.h>
> +#include <linux/string_choices.h>
>  #include <linux/log2.h>
>  #include <linux/logic_pio.h>
>  #include <linux/device.h>
> @@ -4857,21 +4858,189 @@ void pci_cxl_set_sbr_region_ops(const struct pci_cxl_sbr_region_ops *ops)
>  }
>  EXPORT_SYMBOL_GPL(pci_cxl_set_sbr_region_ops);
>  
> +struct cxl_sbr_ctx {
> +	u16 port_ctl;
> +	u16 acs_ctrl;
> +	u16 command;
> +};
> +
> +static 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)
> +{
> +	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)
> +{
> +	int rc;
> +
> +	/* Abort before touching hardware if the regions cannot be disabled. */
> +	if (cxl_sbr_region_ops) {
> +		rc = cxl_sbr_region_ops->disable_regions(bridge);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	/* CXL r4.0 sec 8.1.5.2, Table 8-32: set Unmask SBR so the Port issues Hot Reset. */
> +	pci_read_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL, &ctx->port_ctl);
> +	pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL,
> +			      ctx->port_ctl | PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR);
> +
> +	pci_read_config_word(bridge, PCI_COMMAND, &ctx->command);
> +	pci_clear_master(bridge);
> +
> +	/* CXL r4.0 sec 8.1.5.1: Disable ACS SV bit before SBR */
> +	if (bridge->acs_cap) {
> +		pci_read_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL, &ctx->acs_ctrl);
> +		pci_dbg(bridge, "%s: ACS SV %s\n", __func__,
> +			str_enabled_disabled(ctx->acs_ctrl & PCI_ACS_SV));
> +		pci_write_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL,
> +				      ctx->acs_ctrl & ~PCI_ACS_SV);
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * CXL r4.0 sec 8.1.5.1, Table 8-31: the Port sets PM Init Complete within
> + * 100 ms of link-up. Restoring ACS Source Validation before then makes the
> + * Port reject the downstream Component's Requester-Bus-0 IP2PM message, so
> + * poll for completion before restoring config.
> + */
> +static bool cxl_port_pm_init_is_complete(struct pci_dev *bridge, u16 dvsec)
> +{
> +	unsigned long start = jiffies;
> +	unsigned long timeout = start + msecs_to_jiffies(100);
> +	u16 status;
> +
> +	do {
> +		pci_read_config_word(bridge,
> +				     dvsec + PCI_DVSEC_CXL_PORT_EXT_STATUS,
> +				     &status);
> +		if (!PCI_POSSIBLE_ERROR(status) &&
> +		    (status & PCI_DVSEC_CXL_PORT_EXT_STATUS_PM_INIT_COMP)) {
> +			pci_dbg(bridge, "%s: PM Init Complete set after %u ms, ext status %#06x\n",
> +				__func__, jiffies_to_msecs(jiffies - start), status);
> +			return true;
> +		}
> +		msleep(10);
> +	} while (time_before(jiffies, timeout));
> +
> +	pci_warn(bridge, "%s: PM Init Complete not set after %u ms, ext status %#06x\n",
> +		 __func__, jiffies_to_msecs(jiffies - start), status);
> +
> +	return false;
> +}
> +
> +static int cxl_sbr_restore_config_space(struct pci_dev *dev, void *userdata)
> +{
> +	pci_restore_config_space(dev);
> +	pci_dbg(dev, "%s: config space restored\n", __func__);
> +
> +	return 0;
> +}
> +
> +/*
> + * The CXL region ops that run next read the HDM Decoders through a Base Address
> + * Register the reset returned to its initialization value, so restore the
> + * header of every device below @bridge first. Restoring also re-captures each
> + * Bus Number before the Port's ACS Source Validation comes back: a device that
> + * has completed no Type 0 Configuration Write since the reset sources Requests
> + * with Bus 0, which the Port rejects as an ACS Violation.
> + *
> + * Only the header is restored. The capability state each caller saved is its own
> + * to replay, and the ->reset_done() callbacks pci_dev_restore() invokes must
> + * fire once, from the caller that owns the reset.
> + */
> +static void cxl_sbr_restore_subordinate(struct pci_dev *bridge)
> +{
> +	if (!bridge->subordinate)
> +		return;
> +
> +	/* Parents before children: a child answers once its parent forwards. */
> +	pci_walk_bus(bridge->subordinate, cxl_sbr_restore_config_space, NULL);
> +}
> +
> +static void cxl_sbr_complete(struct pci_dev *bridge, u16 dvsec,
> +			     const struct cxl_sbr_ctx *ctx)
> +{
> +	u16 val;
> +
> +	/* CXL r4.0 sec 8.1.5.1: wait for PM Init before restoring ACS SV. */
> +	if (!cxl_port_pm_init_is_complete(bridge, dvsec))
> +		pci_warn(bridge,
> +			 "restoring ACS Source Validation before PM Init complete; Port may reject the Component's bus 0 traffic\n");
> +
> +	cxl_sbr_restore_subordinate(bridge);
> +
> +	/* CXL r4.0 sec 8.1.5.1: Re-enable ACS SV bit after SBR if it was enabled before */
> +	if (bridge->acs_cap && (ctx->acs_ctrl & PCI_ACS_SV)) {
> +		pci_read_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL, &val);
> +		pci_write_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL,
> +				      val | PCI_ACS_SV);
> +		pci_dbg(bridge, "%s: ACS SV bit set\n", __func__);
> +	} else {
> +		pci_dbg(bridge, "%s: ACS SV bit not set (was not enabled before the SBR)\n",
> +			__func__);
> +	}
> +
> +	if (ctx->command & PCI_COMMAND_MASTER)
> +		pci_set_master(bridge);
> +
> +	if (!(ctx->port_ctl & PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR)) {
> +		pci_read_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL, &val);
> +		pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL,
> +				      val & ~PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR);
> +	}
> +
> +	if (cxl_sbr_region_ops)
> +		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.
> + *
> + * 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)
>  {
> +	struct cxl_sbr_ctx ctx = {};
> +	u16 dvsec = 0;
> +	int rc;
> +
>  	if (!dev->block_cfg_access)
>  		pci_warn_once(dev, "unlocked secondary bus reset via: %pS\n",
>  			      __builtin_return_address(0));
> +
> +	if (is_cxl_dport(dev))
> +		dvsec = cxl_port_dvsec(dev);
> +	if (dvsec) {
> +		rc = cxl_sbr_prepare(dev, dvsec, &ctx);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	pcibios_reset_secondary_bus(dev);
>  
> -	return pci_bridge_wait_for_secondary_bus(dev, "bus reset");
> +	rc = pci_bridge_wait_for_secondary_bus(dev, "bus reset");
> +
> +	if (dvsec)
> +		cxl_sbr_complete(dev, dvsec, &ctx);

Hi Fabio,

I wonder what happens if pci_bridge_wait_for_secondary_bus() fails here?

Maybe failures before/after SBR should be handled differenly ?

Before SBR, it's safe to undo preparation. After SBR, the HW state may be unknown,
in that case, enable_regions() must not reattach regions unless PCI state and all HDM
decoders were restored successfully.

Best regards,
Richard Cheng

> +
> +	return rc;
>  }
>  EXPORT_SYMBOL_GPL(pci_bridge_secondary_bus_reset);
>  
> @@ -4917,12 +5086,6 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe)
>  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
>  }
>  
> -static u16 cxl_port_dvsec(struct pci_dev *dev)
> -{
> -	return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
> -					 PCI_DVSEC_CXL_PORT);
> -}
> -
>  static bool cxl_sbr_masked(struct pci_dev *dev)
>  {
>  	u16 dvsec, reg;
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index facaa324bd86..0eaa34db93ce 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1371,6 +1371,8 @@
>  
>  /* CXL r4.0, 8.1.5: Extensions DVSEC for Ports */
>  #define PCI_DVSEC_CXL_PORT				3
> +#define  PCI_DVSEC_CXL_PORT_EXT_STATUS			0x0a
> +#define   PCI_DVSEC_CXL_PORT_EXT_STATUS_PM_INIT_COMP	0x00000001
>  #define  PCI_DVSEC_CXL_PORT_CTL				0x0c
>  #define   PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR		0x00000001
>  
> -- 
> 2.55.0
> 
> 

Re: [PATCH v2 11/13] PCI/DPC: Unbind regions for DPC recovery

[Richard Cheng]

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@linux.intel.com>
> ---
>  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(&regions);
> +
> +	cxl_sbr_collect_regions(dport_pci, &regions);
> +
> +	xa_for_each(&regions, index, cxlr)
> +		cxl_region_unbind(cxlr);
> +
> +	cxl_sbr_put_regions(&regions);
> +}
> +
>  /*
>   * 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
> 
> 

Re: [PATCH v2 09/13] PCI/CXL: Disable and re-enable CXL regions

[Richard Cheng]

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@linux.intel.com>
> ---
>  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(&regions);
> +
> +	rc = cxl_sbr_collect_regions(dport_pci, &regions);
> +	if (rc)
> +		goto out;
> +
> +	xa_for_each(&regions, 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(&regions, index, cxlr)
> +			cxl_region_enable(cxlr);
> +	}
> +
> +out:
> +	cxl_sbr_put_regions(&regions);
> +	/* 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(&regions);
> +
> +	cxl_sbr_collect_regions(dport_pci, &regions);
> +
> +	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(&regions, index, cxlr)
> +		cxl_region_enable(cxlr);
> +
> +	cxl_sbr_put_regions(&regions);
> +	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
> 
> 

Re: [PATCH v2 00/13] Make SBR work for CXL Downstream Ports

[Richard Cheng]

On Tue, Aug 25, 2026 at 04:26:15AM +0800, Fabio M. De Francesco wrote:
> A Secondary Bus Reset (SBR), a Link Down or a Downstream Port
> Containment (DPC) event on a CXL Downstream Port clears the Downstream
> Component's captured Bus Number. CXL r4.0 sec 8.1.5.1 notes that, if the
> Component then sources its Power Management (PM) Initialization traffic
> with Requester Bus = 0, a Port with Access Control Services (ACS) Source
> Validation (SV) enabled may reject the Component's CREDIT_RTN IP2PM
> message, PM Initialization may fail to complete, and that "another
> Secondary Bus Reset alone will not facilitate recovery". PCIe r7.0 sec
> 6.12.1.1 makes the rejection an error that is reported as ACS Violation.
> 
> The Implementation Note under sec 8.1.5.1 gives the sequence that avoids
> it: save and clear Bus Master Enable and ACS SV, generate the SBR, wait
> for Port PM Initialization Complete, restore both bits, continue device
> re-initialization. This series takes a step further to a preventive
> measure that avoids the ACS Violation in the first place, not after
> failure.
> 
> Patches 1-5 restore HDM Decoder registers below a reset CXL Downstream
> Port.
> 
> Patches 6-8 disable and re-enable the CXL Regions around the reset:
> offline memory, invalidate CPU caches for its range, collect the set of
> regions the CXL DPort which is SBR andunbind the driver.
> 
> Patch 9 disable and re-enable CXL regions across a Downstream Port reset
> and registers the CXL region callbacks for a Downstream Port bus. We
> want the reset to be called only when the CXL driver is loaded.
> 
> Patch 10 calls the CXL operations from pci_bridge_secondary_bus_reset().
> 
> Patch 11 issues the same sequence from dpc_reset_link().
> 
> Patches 12 and 13 gives root a sysfs attribute that conrols whether the
> SBR is allowed to be unmasked and prevents kernel from unmasking SBR on
> its own.
> 
> What user space finds after an SBR of a CXL Downstream Port: the dax
> device returns under the same name, the region's HPA range, interleave
> geometry and decoder programming are restored, and the media content is
> unchanged for PMEM. System RAM comes back registered but offline.
>

Hi Fabio,

I think overall the recovery sequence makes sense, but I have some concerns
about how the state of one reset is represented.

In your current design, if I'm not misunderstanding, the state of one reset is split
across several places:

- PCI keeps ACS, BME, and Port Control in its local cxl_sbr_ctx
- CXL keeps HDM state in a global xarray keyed by the Downstream Port
- disable_regions() collects a region set, operates on it, and then discards it
- enable_regions() collects the region set again after the reset

There's no single owner that remembers exactly which regions and memory blocks were
changed by the corresponded reset. The set collected after the reset should be the
same set that was disabled, but I don't get where does this guaranteed ?
Collection or decoder restoration may fail, but enable_regions() can't report failure
and may still reattach regions.

IMHO, maybe have an opaque per-reset context, something like

"""
prepare(dport, mode) -> context
reset
restore(context)
"""

The context should hold
- Ref to the exact affected reegions, ports, EP
- The original bound state of each region
- The original online type of each memory block changed by preparation
- Cached HDM Global Control and decoder state
- The current phase and which preparation steps completed

I think PCI core doesn't need to know the contents of this context, it only pass it
to CXL core. This can allow the recovery path to dinstinguish 3 outcomes
- Preparation failed before SBR
- SBR and restoration succeeded
- SBR was issued but restoration failed

This can also avoid the need to recollect topology after reset or store operation state
in a global xarray.
Separate resets would naturally have separate state.

Does this model fit the indended flow ?

Best regards,
Richard Cheng.

 
> Fabio M. De Francesco (13):
>   cxl/pci: Make the HDM and Mem_Enable writes callable from cxl_core
>   cxl/hdm: Add cxl_decoder_recommit() to restore one HDM decoder
>   cxl/hdm: Add cxl_endpoint_enable_hdm_decode() to restore CXL.mem
>     decode
>   cxl/hdm: Reprogram the HDM Decoders below a CXL Port
>   cxl/core: Restore the HDM decoders below a reset Downstream Port
>   drivers/base/memory: Add cxl_offline_memory() to offline a physical
>     range
>   cxl/core: Add region disable and enable for a Downstream Port reset
>   cxl/core: Collect the CXL regions routed through a Downstream Port
>   PCI/CXL: Register CXL region callbacks for a Downstream Port bus reset
>   PCI/CXL: Clear ACS SV across an SBR of a CXL Downstream Port
>   PCI/DPC: Issue the CXL recovery SBR from DPC without offlining memory
>   PCI/CXL: Add cxl_unmask_sbr to permit an SBR of a CXL Downstream Port
>   PCI/CXL: Refuse an SBR of a CXL Downstream Port without the unmask
>     consent
> 
>  Documentation/ABI/testing/sysfs-bus-pci |  16 +
>  drivers/base/memory.c                   |  36 +++
>  drivers/cxl/core/Makefile               |   2 +-
>  drivers/cxl/core/core.h                 |  28 ++
>  drivers/cxl/core/dport_sbr.c            | 374 ++++++++++++++++++++++++
>  drivers/cxl/core/hdm.c                  | 341 +++++++++++++++++++++
>  drivers/cxl/core/pci.c                  |  19 +-
>  drivers/cxl/core/port.c                 |   4 +-
>  drivers/cxl/core/ras.c                  |  10 +
>  drivers/cxl/core/region.c               |   6 +-
>  drivers/pci/pci-sysfs.c                 |  31 ++
>  drivers/pci/pci.c                       | 254 +++++++++++++++-
>  drivers/pci/pci.h                       |  15 +
>  drivers/pci/pcie/dpc.c                  |  46 ++-
>  include/linux/aer.h                     |   9 +
>  include/linux/memory.h                  |   5 +
>  include/linux/pci.h                     |  16 +
>  include/uapi/linux/pci_regs.h           |   2 +
>  tools/testing/cxl/Kbuild                |   2 +-
>  19 files changed, 1193 insertions(+), 23 deletions(-)
>  create mode 100644 drivers/cxl/core/dport_sbr.c
> 
> -- 
> 2.55.0
> 
>