[PATCH v4 0/9] cxl: Delay HB port and switch dport probing until endpoint dev probe

[PATCH v4 0/9] cxl: Delay HB port and switch dport probing until endpoint dev probe

[Dave Jiang]

v4:
- Push dport allocation to when they are discovered. (Robert)
- Drop linux id for dport with above changes.

v3:
- Main changes revolve around improving naming of hostbridge uport and dport (Gregory)
- See specific patches for detailed change log

This series attempts to delay the allocation of dports until when the endpoint device
(memdev) are being probed. At this point, the CXL link is established and all the
devices along the CXL link path up to the Root Port (RP) should be active.

And hopefully this help a bit with Robert's issue raised in the "Inactive
downstream port handling" series [1]. Testing would be appreicated. Thank you!

[1]: https://lore.kernel.org/linux-cxl/67c8a0cc23ec_24b64294f6@dwillia2-xfh.jf.intel.com.notmuch/

Dave Jiang (9):
  cxl/region: Add decoder check to check_commit_order()
  cxl: Add helper to detect top of CXL device topology
  cxl: Add helper to reap dport
  cxl: Defer dport allocation for switch ports
  cxl/test: Add cxl_test support for new dport allocation scheme
  cxl/test: Add mock version of devm_cxl_add_dport_by_dev()
  cxl: Change sslbis handler to only handle single dport
  cxl: Create an xarray to tie a host bridge to the cxl_root
  cxl: Move enumeration of hostbridge ports to the memdev probe path

 drivers/cxl/acpi.c                   | 137 ++++++-----
 drivers/cxl/core/cdat.c              |  23 +-
 drivers/cxl/core/pci.c               |  89 ++++++++
 drivers/cxl/core/port.c              | 329 +++++++++++++++++++++++++--
 drivers/cxl/core/region.c            |   7 +-
 drivers/cxl/cxl.h                    |  32 ++-
 drivers/cxl/port.c                   |   8 +-
 tools/testing/cxl/Kbuild             |   2 +
 tools/testing/cxl/cxl_core_exports.c |  12 +
 tools/testing/cxl/exports.h          |  10 +
 tools/testing/cxl/test/cxl.c         | 119 ++++++++++
 tools/testing/cxl/test/mock.c        |  38 ++++
 tools/testing/cxl/test/mock.h        |   3 +
 13 files changed, 715 insertions(+), 94 deletions(-)
 create mode 100644 tools/testing/cxl/exports.h


base-commit: 86731a2a651e58953fc949573895f2fa6d456841
-- 
2.49.0

[PATCH v4 1/9] cxl/region: Add decoder check to check_commit_order()

[Dave Jiang]

check_commit_order() attempts to convert a device to a decoder without
making sure the device is a decoder. So far this has been working due
to pure luck. Issue discovered while doing deferred dport probing when
child ports are now in the midst of decoders due to ordering change
of child port additions. Add a check before attempting to do decoder
conversion.

Reviewed-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Li Ming <ming.li@zohomail.com>
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/region.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 6e5e1460068d..b94fda6f2e4c 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -787,7 +787,12 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
 
 static int check_commit_order(struct device *dev, void *data)
 {
-	struct cxl_decoder *cxld = to_cxl_decoder(dev);
+	struct cxl_decoder *cxld;
+
+	if (!is_switch_decoder(dev))
+		return 0;
+
+	cxld = to_cxl_decoder(dev);
 
 	/*
 	 * if port->commit_end is not the only free decoder, then out of
-- 
2.49.0

[PATCH v4 2/9] cxl: Add helper to detect top of CXL device topology

[Dave Jiang]

Add a helper to replace the open code detection of CXL device hierarchy
root. The helper will be used for delayed hostbridge port creation later
on.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Li Ming <ming.li@zohomail.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/port.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index eb46c6764d20..a49491c14d19 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -39,6 +39,15 @@ DECLARE_RWSEM(cxl_region_rwsem);
 static DEFINE_IDA(cxl_port_ida);
 static DEFINE_XARRAY(cxl_root_buses);
 
+/*
+ * The terminal device in PCI is NULL and @platform_bus
+ * for platform devices (for cxl_test)
+ */
+static bool is_cxl_hierarchy_head(struct device *dev)
+{
+	return (!dev || dev == &platform_bus);
+}
+
 int cxl_num_decoders_committed(struct cxl_port *port)
 {
 	lockdep_assert_held(&cxl_region_rwsem);
@@ -1635,11 +1644,7 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 		struct device *uport_dev;
 		struct cxl_dport *dport;
 
-		/*
-		 * The terminal "grandparent" in PCI is NULL and @platform_bus
-		 * for platform devices
-		 */
-		if (!dport_dev || dport_dev == &platform_bus)
+		if (is_cxl_hierarchy_head(dport_dev))
 			return 0;
 
 		uport_dev = dport_dev->parent;
-- 
2.49.0

[PATCH v4 3/9] cxl: Add helper to reap dport

[Dave Jiang]

Refactor the code in reap_dports() out to provide a helper function that
reaps a single dport. This will be used later in the cleanup path for
allocating a dport.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/port.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index a49491c14d19..9691da831224 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1447,6 +1447,13 @@ static void delete_switch_port(struct cxl_port *port)
 	devm_release_action(port->dev.parent, unregister_port, port);
 }
 
+static void reap_dport(struct cxl_port *port, struct cxl_dport *dport)
+{
+	devm_release_action(&port->dev, cxl_dport_unlink, dport);
+	devm_release_action(&port->dev, cxl_dport_remove, dport);
+	devm_kfree(&port->dev, dport);
+}
+
 static void reap_dports(struct cxl_port *port)
 {
 	struct cxl_dport *dport;
@@ -1454,11 +1461,8 @@ static void reap_dports(struct cxl_port *port)
 
 	device_lock_assert(&port->dev);
 
-	xa_for_each(&port->dports, index, dport) {
-		devm_release_action(&port->dev, cxl_dport_unlink, dport);
-		devm_release_action(&port->dev, cxl_dport_remove, dport);
-		devm_kfree(&port->dev, dport);
-	}
+	xa_for_each(&port->dports, index, dport)
+		reap_dport(port, dport);
 }
 
 struct detach_ctx {
-- 
2.49.0

Re: [PATCH v4 3/9] cxl: Add helper to reap dport

[Jonathan Cameron]

On Tue, 24 Jun 2025 14:39:10 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Refactor the code in reap_dports() out to provide a helper function that
> reaps a single dport. This will be used later in the cleanup path for
> allocating a dport.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

[PATCH v4 4/9] cxl: Defer dport allocation for switch ports

[Dave Jiang]

The current implementation enumerates the dports during the cxl_port
driver probe. Without an endpoint connected, the dport may not be
active during port probe. This scheme may prevent a valid hardware
dport id to be retrieved and MMIO registers to be read when an endpoint
is hot-plugged. Move the dport allocation and setup to behind memdev
probe so the endpoint is guaranteed to be connected.

In the original enumeration behavior, there are 3 phases (or 2 if no CXL
switches) for port creation. cxl_acpi() creates a Root Port (RP) from the
ACPI0017.N device. Through that it enumerate downstream ports composed
of ACPI0016.N devices through add_host_bridge_dport(). Once done, it
use add_host_bridge_uport() to create the ports that enumerates the PCI
RPs as the dports of these ports. Every time a port is created, the port
driver is attached and drv->probe() is called and
devm_cxl_port_enumerate_dports() is envoked to enumerate and probe
the dports.

The second phase is if there are any CXL switches. When the pci endpoint
device driver (cxl_pci) calls probe, it will add a mem device and triggers
the cxl_mem->probe(). cxl_mem->probe() calls devm_cxl_enumerate_ports()
and attempts to discovery and create all the ports represent CXL switches.
During this phase, a port is created per switch and the attached dports
are also enumerated and probed.

The last phase is creating endpoint port which happens for all endpoint
devices.

In this commit, the port create and its dport probing in cxl_acpi is not
changed. That will be handled in a different patch later on. The behavior
change is only for CXL switch ports. Only the dport that is part of the
path for an endpoint device to the RP will be probed. This happens
naturally by the code walking up the device hierarchy and identifying the
upstream device and the downstream device.

There are two points where the interception of dport creation happens
during the devm_cxl_enumerate_ports() path. The first location is right
before the function calls add_port_attch_ep() where it does the dport
allocation for the RP. Once the dport is allocated, the iteration path
is reset to the beginning to try again. The second location happens
in add_port_attach_ep() after the location where either the port is
discovered or allocated new if it does not exist.

Locking of port device during __cxl_port_add_dport() protects modifications
against the port and its dports while multiple endpoints can be probing at
the same time and the same port is being modified concurrently.

While the decoders are allocated during the port driver probe,
The decoders must also be updated since previously it's all done when all
the dports are setup and now every time a dport is setup per endpoint, the
switch target listing need to be updated with new dport. A
guard(rwsem_write) is used to update decoder targets. This is similar to
when decoder_populate_target() is called and the decoder programming
must be protected.

Link: https://lore.kernel.org/linux-cxl/20250305100123.3077031-1-rrichter@amd.com/
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v4:
- Allocate dport when they are found via endpoint probe. (Robert)
---
 drivers/cxl/core/core.h |   2 +
 drivers/cxl/core/pci.c  |  88 ++++++++++++++++++++
 drivers/cxl/core/port.c | 180 ++++++++++++++++++++++++++++++++++++++--
 drivers/cxl/cxl.h       |   5 ++
 drivers/cxl/port.c      |   8 +-
 5 files changed, 269 insertions(+), 14 deletions(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 29b61828a847..8cfead6f3b08 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -122,6 +122,8 @@ void cxl_ras_exit(void);
 int cxl_gpf_port_setup(struct cxl_dport *dport);
 int cxl_acpi_get_extended_linear_cache_size(struct resource *backing_res,
 					    int nid, resource_size_t *size);
+struct cxl_dport *devm_cxl_add_dport_by_dev(struct cxl_port *port,
+					    struct device *dport_dev);
 
 #ifdef CONFIG_CXL_FEATURES
 struct cxl_feat_entry *
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index b50551601c2e..6e9b5ab69de0 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -24,6 +24,44 @@ static unsigned short media_ready_timeout = 60;
 module_param(media_ready_timeout, ushort, 0644);
 MODULE_PARM_DESC(media_ready_timeout, "seconds to wait for media ready");
 
+/**
+ * devm_cxl_add_dport_by_dev - allocate a dport by dport device
+ * @port: cxl_port that hosts the dport
+ * @dport_dev: 'struct device' of the dport
+ *
+ * Returns the allocate dport on success or ERR_PTR() of -errno on error
+ */
+struct cxl_dport *devm_cxl_add_dport_by_dev(struct cxl_port *port,
+					    struct device *dport_dev)
+{
+	struct cxl_register_map map;
+	struct pci_dev *pdev;
+	u32 lnkcap, port_num;
+	int type;
+	int rc;
+
+	if (!dev_is_pci(dport_dev))
+		return ERR_PTR(-EINVAL);
+
+	device_lock_assert(&port->dev);
+
+	pdev = to_pci_dev(dport_dev);
+	type = pci_pcie_type(pdev);
+	if (type != PCI_EXP_TYPE_DOWNSTREAM && type != PCI_EXP_TYPE_ROOT_PORT)
+		return ERR_PTR(-EINVAL);
+
+	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
+				  &lnkcap))
+		return ERR_PTR(-ENXIO);
+
+	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
+	if (rc)
+		dev_dbg(&port->dev, "failed to find component registers\n");
+
+	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
+	return devm_cxl_add_dport(port, &pdev->dev, port_num, map.resource);
+}
+
 struct cxl_walk_context {
 	struct pci_bus *bus;
 	struct cxl_port *port;
@@ -1169,3 +1207,53 @@ int cxl_gpf_port_setup(struct cxl_dport *dport)
 
 	return 0;
 }
+
+static int match_dport(struct pci_dev *pdev, void *data)
+{
+	struct cxl_walk_context *ctx = data;
+	int type = pci_pcie_type(pdev);
+
+	if (pdev->bus != ctx->bus)
+		return 0;
+	if (!pci_is_pcie(pdev))
+		return 0;
+	if (type != ctx->type)
+		return 0;
+
+	ctx->count++;
+	return 0;
+}
+
+int cxl_port_get_total_dports(struct cxl_port *port)
+{
+	struct pci_bus *bus = cxl_port_to_pci_bus(port);
+	struct cxl_walk_context ctx;
+	int type;
+
+	if (!bus) {
+		dev_err(&port->dev, "No PCI bus found for port %s\n",
+			dev_name(&port->dev));
+		return -ENXIO;
+	}
+
+	if (pci_is_root_bus(bus))
+		type = PCI_EXP_TYPE_ROOT_PORT;
+	else
+		type = PCI_EXP_TYPE_DOWNSTREAM;
+
+	ctx = (struct cxl_walk_context) {
+		.bus = bus,
+		.type = type,
+	};
+	pci_walk_bus(bus, match_dport, &ctx);
+
+	port->total_dports = ctx.count;
+	if (port->total_dports == 0) {
+		dev_warn(&port->dev, "No dports found for port %s on bus %s\n",
+			 dev_name(&port->dev), bus->name);
+		return -ENXIO;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_port_get_total_dports, "CXL");
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 9691da831224..3872638c439b 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1551,6 +1551,134 @@ static resource_size_t find_component_registers(struct device *dev)
 	return map.resource;
 }
 
+static int match_port_by_uport(struct device *dev, const void *data)
+{
+	const struct device *uport_dev = data;
+	struct cxl_port *port;
+
+	if (!is_cxl_port(dev))
+		return 0;
+
+	port = to_cxl_port(dev);
+	return uport_dev == port->uport_dev;
+}
+
+/*
+ * Function takes a device reference on the port device. Caller should do a
+ * put_device() when done.
+ */
+static struct cxl_port *find_cxl_port_by_uport(struct device *uport_dev)
+{
+	struct device *dev;
+
+	dev = bus_find_device(&cxl_bus_type, NULL, uport_dev, match_port_by_uport);
+	if (dev)
+		return to_cxl_port(dev);
+	return NULL;
+}
+
+static int update_switch_decoder(struct device *dev, void *data)
+{
+	struct cxl_dport *dport = data;
+	struct cxl_switch_decoder *cxlsd;
+	struct cxl_decoder *cxld;
+	int i;
+
+	if (!is_switch_decoder(dev))
+		return 0;
+
+	cxlsd = to_cxl_switch_decoder(dev);
+	cxld = &cxlsd->cxld;
+	guard(rwsem_write)(&cxl_region_rwsem);
+	for (i = 0; i < cxld->interleave_ways; i++) {
+		if (cxlsd->target_map[i] == dport->port_id) {
+			cxlsd->target[i] = dport;
+			return 0;
+		}
+	}
+
+	dev_dbg(dev, "Updating decoder target_map with %s and none found\n",
+		dev_name(dport->dport_dev));
+
+	return 0;
+}
+
+static int update_decoders_with_dport(struct cxl_port *port, struct cxl_dport *dport)
+{
+	device_lock_assert(&port->dev);
+	return device_for_each_child(&port->dev, dport, update_switch_decoder);
+}
+
+static int cxl_port_setup_with_dport(struct cxl_port *port,
+				     struct cxl_dport *dport)
+{
+	device_lock_assert(&port->dev);
+
+	cxl_switch_parse_cdat(port);
+
+	return update_decoders_with_dport(port, dport);
+}
+
+static int devm_cxl_port_add_dport(struct cxl_port *port,
+				   struct device *dport_dev,
+				   struct cxl_dport **saved_dport)
+{
+	struct cxl_dport *dport;
+	int rc;
+
+	*saved_dport = NULL;
+	device_lock_assert(&port->dev);
+
+	/* Port driver not attached yet, wait for cxl_acpi reprobe */
+	if (!port->dev.driver)
+		return -ENODEV;
+
+	dport = cxl_find_dport_by_dev(port, dport_dev);
+	if (dport)
+		return 0;
+
+	dport = devm_cxl_add_dport_by_dev(port, dport_dev);
+	if (IS_ERR(dport))
+		return PTR_ERR(dport);
+
+	rc = cxl_port_setup_with_dport(port, dport);
+	if (rc) {
+		reap_dport(port, dport);
+		return rc;
+	}
+
+	*saved_dport = dport;
+	return 0;
+}
+
+static int devm_cxl_add_dport_by_uport(struct device *uport_dev,
+				       struct device *dport_dev,
+				       struct cxl_dport **dport)
+{
+	*dport = NULL;
+	struct cxl_port *port __free(put_cxl_port) =
+		find_cxl_port_by_uport(uport_dev);
+	if (!port)
+		return -ENODEV;
+
+	guard(device)(&port->dev);
+	return devm_cxl_port_add_dport(port, dport_dev, dport);
+}
+
+static int devm_cxl_add_dport_for_rp(struct device *uport_dev,
+				     struct device *dport_dev,
+				     struct cxl_dport **dport)
+{
+	struct device *dparent = grandparent(dport_dev);
+
+	*dport = NULL;
+	/* Only go down this path if we are at the root port */
+	if (!is_cxl_hierarchy_head(dparent))
+		return 0;
+
+	return devm_cxl_add_dport_by_uport(uport_dev, dport_dev, dport);
+}
+
 static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 			      struct device *uport_dev,
 			      struct device *dport_dev)
@@ -1584,6 +1712,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 	 */
 	struct cxl_port *port __free(put_cxl_port) = NULL;
 	scoped_guard(device, &parent_port->dev) {
+		struct cxl_dport *new_dport;
+
 		if (!parent_port->dev.driver) {
 			dev_warn(&cxlmd->dev,
 				 "port %s:%s disabled, failed to enumerate CXL.mem\n",
@@ -1592,6 +1722,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 		}
 
 		port = find_cxl_port_at(parent_port, dport_dev, &dport);
+		if (!port)
+			port = find_cxl_port_by_uport(uport_dev);
 		if (!port) {
 			component_reg_phys = find_component_registers(uport_dev);
 			port = devm_cxl_add_port(&parent_port->dev, uport_dev,
@@ -1599,11 +1731,17 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 			if (IS_ERR(port))
 				return PTR_ERR(port);
 
-			/* retry find to pick up the new dport information */
-			port = find_cxl_port_at(parent_port, dport_dev, &dport);
-			if (!port)
-				return -ENXIO;
+			get_device(&port->dev);
 		}
+
+		guard(device)(&port->dev);
+		/* Existing dport is checked with no action done if exists */
+		rc = devm_cxl_port_add_dport(port, dport_dev, &new_dport);
+		if (rc)
+			return rc;
+
+		if (new_dport)
+			dport = new_dport;
 	}
 
 	dev_dbg(&cxlmd->dev, "add to new port %s:%s\n",
@@ -1620,11 +1758,13 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 	return rc;
 }
 
+#define CXL_ITER_LEVEL_SWITCH	1
+
 int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 {
 	struct device *dev = &cxlmd->dev;
 	struct device *iter;
-	int rc;
+	int rc, i;
 
 	/*
 	 * Skip intermediate port enumeration in the RCH case, there
@@ -1643,7 +1783,7 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 	 * attempt fails.
 	 */
 retry:
-	for (iter = dev; iter; iter = grandparent(iter)) {
+	for (i = 0, iter = dev; iter; i++, iter = grandparent(iter)) {
 		struct device *dport_dev = grandparent(iter);
 		struct device *uport_dev;
 		struct cxl_dport *dport;
@@ -1686,9 +1826,28 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 			if (!dev_is_cxl_root_child(&port->dev))
 				continue;
 
+			/*
+			 * This is a corner case where the rootport is setup but
+			 * the switch dport is not. It needs to go back to the
+			 * beginning to setup the switch port.
+			 */
+			if (i >= CXL_ITER_LEVEL_SWITCH) {
+				struct cxl_port *pport __free(put_cxl_port) =
+					cxl_mem_find_port(cxlmd, &dport);
+				if (!port)
+					goto retry;
+			}
+
 			return 0;
 		}
 
+		rc = devm_cxl_add_dport_for_rp(uport_dev, dport_dev, &dport);
+		if (rc)
+			return rc;
+		/* Added a dport, restart enumeration */
+		if (dport)
+			goto retry;
+
 		rc = add_port_attach_ep(cxlmd, uport_dev, dport_dev);
 		/* port missing, try to add parent */
 		if (rc == -EAGAIN)
@@ -1727,16 +1886,19 @@ static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
 		return 0;
 
 	device_lock_assert(&port->dev);
+	memcpy(cxlsd->target_map, target_map, sizeof(cxlsd->target_map));
 
 	if (xa_empty(&port->dports))
-		return -EINVAL;
+		return 0;
 
 	guard(rwsem_write)(&cxl_region_rwsem);
 	for (i = 0; i < cxlsd->cxld.interleave_ways; i++) {
 		struct cxl_dport *dport = find_dport(port, target_map[i]);
 
-		if (!dport)
-			return -ENXIO;
+		if (!dport) {
+			/* dport may be activated later */
+			continue;
+		}
 		cxlsd->target[i] = dport;
 	}
 
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 3f1695c96abc..5f598fb542e5 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -403,6 +403,7 @@ struct cxl_endpoint_decoder {
  * struct cxl_switch_decoder - Switch specific CXL HDM Decoder
  * @cxld: base cxl_decoder object
  * @nr_targets: number of elements in @target
+ * @target_map: map of target dport ids to interleave positions
  * @target: active ordered target list in current decoder configuration
  *
  * The 'switch' decoder type represents the decoder instances of cxl_port's that
@@ -414,6 +415,7 @@ struct cxl_endpoint_decoder {
 struct cxl_switch_decoder {
 	struct cxl_decoder cxld;
 	int nr_targets;
+	int target_map[CXL_DECODER_MAX_INTERLEAVE];
 	struct cxl_dport *target[];
 };
 
@@ -584,6 +586,7 @@ struct cxl_dax_region {
  * @parent_dport: dport that points to this port in the parent
  * @decoder_ida: allocator for decoder ids
  * @reg_map: component and ras register mapping parameters
+ * @total_dports: total possible dports in this port
  * @nr_dports: number of entries in @dports
  * @hdm_end: track last allocated HDM decoder instance for allocation ordering
  * @commit_end: cursor to track highest committed decoder for commit ordering
@@ -604,6 +607,7 @@ struct cxl_port {
 	struct cxl_dport *parent_dport;
 	struct ida decoder_ida;
 	struct cxl_register_map reg_map;
+	int total_dports;
 	int nr_dports;
 	int hdm_end;
 	int commit_end;
@@ -902,6 +906,7 @@ void cxl_coordinates_combine(struct access_coordinate *out,
 			     struct access_coordinate *c2);
 
 bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port);
+int cxl_port_get_total_dports(struct cxl_port *port);
 
 /*
  * Unit test builds overrides this to __weak, find the 'strong' version
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index fe4b593331da..354e134793c3 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -65,12 +65,10 @@ static int cxl_switch_port_probe(struct cxl_port *port)
 	/* Cache the data early to ensure is_visible() works */
 	read_cdat_data(port);
 
-	rc = devm_cxl_port_enumerate_dports(port);
-	if (rc < 0)
+	rc = cxl_port_get_total_dports(port);
+	if (rc)
 		return rc;
 
-	cxl_switch_parse_cdat(port);
-
 	cxlhdm = devm_cxl_setup_hdm(port, NULL);
 	if (!IS_ERR(cxlhdm))
 		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
@@ -80,7 +78,7 @@ static int cxl_switch_port_probe(struct cxl_port *port)
 		return PTR_ERR(cxlhdm);
 	}
 
-	if (rc == 1) {
+	if (port->total_dports == 1) {
 		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
 		return devm_cxl_add_passthrough_decoder(port);
 	}
-- 
2.49.0

Re: [PATCH v4 4/9] cxl: Defer dport allocation for switch ports

[Jonathan Cameron]

On Tue, 24 Jun 2025 14:39:11 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> The current implementation enumerates the dports during the cxl_port
> driver probe. Without an endpoint connected, the dport may not be
> active during port probe. This scheme may prevent a valid hardware
> dport id to be retrieved and MMIO registers to be read when an endpoint
> is hot-plugged. Move the dport allocation and setup to behind memdev
> probe so the endpoint is guaranteed to be connected.
> 
> In the original enumeration behavior, there are 3 phases (or 2 if no CXL
> switches) for port creation. cxl_acpi() creates a Root Port (RP) from the
> ACPI0017.N device. Through that it enumerate downstream ports composed
> of ACPI0016.N devices through add_host_bridge_dport(). Once done, it
> use add_host_bridge_uport() to create the ports that enumerates the PCI
> RPs as the dports of these ports. Every time a port is created, the port
> driver is attached and drv->probe() is called and
> devm_cxl_port_enumerate_dports() is envoked to enumerate and probe
> the dports.
> 
> The second phase is if there are any CXL switches. When the pci endpoint
> device driver (cxl_pci) calls probe, it will add a mem device and triggers
> the cxl_mem->probe(). cxl_mem->probe() calls devm_cxl_enumerate_ports()
> and attempts to discovery and create all the ports represent CXL switches.
> During this phase, a port is created per switch and the attached dports
> are also enumerated and probed.
> 
> The last phase is creating endpoint port which happens for all endpoint
> devices.
> 
> In this commit, the port create and its dport probing in cxl_acpi is not
> changed. That will be handled in a different patch later on. The behavior
> change is only for CXL switch ports. Only the dport that is part of the
> path for an endpoint device to the RP will be probed. This happens
> naturally by the code walking up the device hierarchy and identifying the
> upstream device and the downstream device.
> 
> There are two points where the interception of dport creation happens
> during the devm_cxl_enumerate_ports() path. The first location is right
> before the function calls add_port_attch_ep() where it does the dport

attach_ep()

> allocation for the RP. Once the dport is allocated, the iteration path
> is reset to the beginning to try again. The second location happens
> in add_port_attach_ep() after the location where either the port is
> discovered or allocated new if it does not exist.
> 
> Locking of port device during __cxl_port_add_dport() protects modifications
> against the port and its dports while multiple endpoints can be probing at
> the same time and the same port is being modified concurrently.
> 
> While the decoders are allocated during the port driver probe,
> The decoders must also be updated since previously it's all done when all
> the dports are setup and now every time a dport is setup per endpoint, the
> switch target listing need to be updated with new dport. A
> guard(rwsem_write) is used to update decoder targets. This is similar to
> when decoder_populate_target() is called and the decoder programming
> must be protected.
> 
> Link: https://lore.kernel.org/linux-cxl/20250305100123.3077031-1-rrichter@amd.com/
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v4:
> - Allocate dport when they are found via endpoint probe. (Robert)

A few comments inline. 

> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index b50551601c2e..6e9b5ab69de0 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -24,6 +24,44 @@ static unsigned short media_ready_timeout = 60;
>  module_param(media_ready_timeout, ushort, 0644);
>  MODULE_PARM_DESC(media_ready_timeout, "seconds to wait for media ready");

>  struct cxl_walk_context {
>  	struct pci_bus *bus;
>  	struct cxl_port *port;
> @@ -1169,3 +1207,53 @@ int cxl_gpf_port_setup(struct cxl_dport *dport)
>  
>  	return 0;
>  }
> +
> +static int match_dport(struct pci_dev *pdev, void *data)
> +{
> +	struct cxl_walk_context *ctx = data;
> +	int type = pci_pcie_type(pdev);
> +
> +	if (pdev->bus != ctx->bus)
> +		return 0;
> +	if (!pci_is_pcie(pdev))
> +		return 0;
> +	if (type != ctx->type)

I'm lost.  How would bus have matched, but type not?
Add a comment.

> +		return 0;
> +
> +	ctx->count++;
> +	return 0;
> +}
> +
> +int cxl_port_get_total_dports(struct cxl_port *port)

See comment below on renaming this given it doesn't return
the number.

> +{
> +	struct pci_bus *bus = cxl_port_to_pci_bus(port);
> +	struct cxl_walk_context ctx;
> +	int type;
> +
> +	if (!bus) {
> +		dev_err(&port->dev, "No PCI bus found for port %s\n",
> +			dev_name(&port->dev));
> +		return -ENXIO;
> +	}
> +
> +	if (pci_is_root_bus(bus))
> +		type = PCI_EXP_TYPE_ROOT_PORT;
> +	else
> +		type = PCI_EXP_TYPE_DOWNSTREAM;
> +
> +	ctx = (struct cxl_walk_context) {
> +		.bus = bus,
> +		.type = type,
> +	};
> +	pci_walk_bus(bus, match_dport, &ctx);
> +
> +	port->total_dports = ctx.count;
> +	if (port->total_dports == 0) {
> +		dev_warn(&port->dev, "No dports found for port %s on bus %s\n",
> +			 dev_name(&port->dev), bus->name);
> +		return -ENXIO;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_port_get_total_dports, "CXL");
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 9691da831224..3872638c439b 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1551,6 +1551,134 @@ static resource_size_t find_component_registers(struct device *dev)
>  	return map.resource;
>  }



> +
> +static int devm_cxl_add_dport_by_uport(struct device *uport_dev,
> +				       struct device *dport_dev,
> +				       struct cxl_dport **dport)
> +{
> +	*dport = NULL;
> +	struct cxl_port *port __free(put_cxl_port) =
> +		find_cxl_port_by_uport(uport_dev);
> +	if (!port)
> +		return -ENODEV;
> +
> +	guard(device)(&port->dev);
> +	return devm_cxl_port_add_dport(port, dport_dev, dport);
> +}
> +
> +static int devm_cxl_add_dport_for_rp(struct device *uport_dev,
> +				     struct device *dport_dev,
> +				     struct cxl_dport **dport)

That this sets *dport = NULL lead me to get confused at the caller.
Perhaps instead these could return a dport *, NULL or ERR_PTR()
instead?
 
> +{
> +	struct device *dparent = grandparent(dport_dev);
> +
> +	*dport = NULL;
> +	/* Only go down this path if we are at the root port */
> +	if (!is_cxl_hierarchy_head(dparent))
> +		return 0;
> +
> +	return devm_cxl_add_dport_by_uport(uport_dev, dport_dev, dport);
> +}
> +
>  static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>  			      struct device *uport_dev,
>  			      struct device *dport_dev)
> @@ -1584,6 +1712,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>  	 */
>  	struct cxl_port *port __free(put_cxl_port) = NULL;
>  	scoped_guard(device, &parent_port->dev) {
> +		struct cxl_dport *new_dport;
> +
>  		if (!parent_port->dev.driver) {
>  			dev_warn(&cxlmd->dev,
>  				 "port %s:%s disabled, failed to enumerate CXL.mem\n",
> @@ -1592,6 +1722,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>  		}
>  
>  		port = find_cxl_port_at(parent_port, dport_dev, &dport);
> +		if (!port)
> +			port = find_cxl_port_by_uport(uport_dev);
>  		if (!port) {
>  			component_reg_phys = find_component_registers(uport_dev);
>  			port = devm_cxl_add_port(&parent_port->dev, uport_dev,
> @@ -1599,11 +1731,17 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>  			if (IS_ERR(port))
>  				return PTR_ERR(port);
>  
> -			/* retry find to pick up the new dport information */
> -			port = find_cxl_port_at(parent_port, dport_dev, &dport);
> -			if (!port)
> -				return -ENXIO;
> +			get_device(&port->dev);

Not immediately obvious to me why a reference needs to be grabbed here. PErhaps
a comment?

>  		}
> +
> +		guard(device)(&port->dev);
> +		/* Existing dport is checked with no action done if exists */
> +		rc = devm_cxl_port_add_dport(port, dport_dev, &new_dport);

Similar comment to below. Would prefer

		new_dport = devm_cxl_port_add_dport(port, dprot_dev);
		if (IS_ERR(new_dport))
			return PTR_ERR(new_dport);


Similar approach already used for devm_cxl_add_port()


> +		if (rc)
> +			return rc;
> +
> +		if (new_dport)
> +			dport = new_dport;
>  	}
>  
>  	dev_dbg(&cxlmd->dev, "add to new port %s:%s\n",
> @@ -1620,11 +1758,13 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>  	return rc;
>  }
>  
> +#define CXL_ITER_LEVEL_SWITCH	1
> +
>  int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>  {
>  	struct device *dev = &cxlmd->dev;
>  	struct device *iter;
> -	int rc;
> +	int rc, i;
>  
>  	/*
>  	 * Skip intermediate port enumeration in the RCH case, there
> @@ -1643,7 +1783,7 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>  	 * attempt fails.
>  	 */
>  retry:
> -	for (iter = dev; iter; iter = grandparent(iter)) {
> +	for (i = 0, iter = dev; iter; i++, iter = grandparent(iter)) {
>  		struct device *dport_dev = grandparent(iter);
>  		struct device *uport_dev;
>  		struct cxl_dport *dport;
> @@ -1686,9 +1826,28 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>  			if (!dev_is_cxl_root_child(&port->dev))
>  				continue;
>  
> +			/*
> +			 * This is a corner case where the rootport is setup but
> +			 * the switch dport is not. It needs to go back to the
> +			 * beginning to setup the switch port.
> +			 */
> +			if (i >= CXL_ITER_LEVEL_SWITCH) {
> +				struct cxl_port *pport __free(put_cxl_port) =
> +					cxl_mem_find_port(cxlmd, &dport);
> +				if (!port)
> +					goto retry;
> +			}
> +
>  			return 0;
>  		}
>  
> +		rc = devm_cxl_add_dport_for_rp(uport_dev, dport_dev, &dport);

As above, I'd prefer to see
		dport = devm_cxl_add_dport_for_rp(uport_dev, dport_dev);
		if (IS_ERR(dport))
			return PTR_ERR(dport);

		if (dport)
			goto retry;

That way it's explicit that dport is going to be updated in all cases within
devm_cxl_add_dport_for_rp()

> +		if (rc)
> +			return rc;
> +		/* Added a dport, restart enumeration */
> +		if (dport)
> +			goto retry;
> +
>  		rc = add_port_attach_ep(cxlmd, uport_dev, dport_dev);
>  		/* port missing, try to add parent */
>  		if (rc == -EAGAIN)
> @@ -1727,16 +1886,19 @@ static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
>  		return 0;
>  
>  	device_lock_assert(&port->dev);
> +	memcpy(cxlsd->target_map, target_map, sizeof(cxlsd->target_map));
>  
>  	if (xa_empty(&port->dports))
> -		return -EINVAL;
> +		return 0;
>  
>  	guard(rwsem_write)(&cxl_region_rwsem);
>  	for (i = 0; i < cxlsd->cxld.interleave_ways; i++) {
>  		struct cxl_dport *dport = find_dport(port, target_map[i]);
>  
> -		if (!dport)
> -			return -ENXIO;
> +		if (!dport) {
> +			/* dport may be activated later */
> +			continue;
> +		}
>  		cxlsd->target[i] = dport;
>  	}



> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index fe4b593331da..354e134793c3 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -65,12 +65,10 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>  	/* Cache the data early to ensure is_visible() works */
>  	read_cdat_data(port);
>  
> -	rc = devm_cxl_port_enumerate_dports(port);
> -	if (rc < 0)
> +	rc = cxl_port_get_total_dports(port);

A function called cxl_port_get_total_dports() to my thinking should return
the number of them not fill in a value embedded in port.

cxl_port_update_total_dports() perhaps?

> +	if (rc)
>  		return rc;
>  
> -	cxl_switch_parse_cdat(port);
> -
>  	cxlhdm = devm_cxl_setup_hdm(port, NULL);
>  	if (!IS_ERR(cxlhdm))
>  		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
> @@ -80,7 +78,7 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>  		return PTR_ERR(cxlhdm);
>  	}
>  
> -	if (rc == 1) {
> +	if (port->total_dports == 1) {
>  		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
>  		return devm_cxl_add_passthrough_decoder(port);
>  	}

Re: [PATCH v4 4/9] cxl: Defer dport allocation for switch ports

[Dave Jiang]

On 7/1/25 4:10 AM, Jonathan Cameron wrote:
> On Tue, 24 Jun 2025 14:39:11 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> The current implementation enumerates the dports during the cxl_port
>> driver probe. Without an endpoint connected, the dport may not be
>> active during port probe. This scheme may prevent a valid hardware
>> dport id to be retrieved and MMIO registers to be read when an endpoint
>> is hot-plugged. Move the dport allocation and setup to behind memdev
>> probe so the endpoint is guaranteed to be connected.
>>
>> In the original enumeration behavior, there are 3 phases (or 2 if no CXL
>> switches) for port creation. cxl_acpi() creates a Root Port (RP) from the
>> ACPI0017.N device. Through that it enumerate downstream ports composed
>> of ACPI0016.N devices through add_host_bridge_dport(). Once done, it
>> use add_host_bridge_uport() to create the ports that enumerates the PCI
>> RPs as the dports of these ports. Every time a port is created, the port
>> driver is attached and drv->probe() is called and
>> devm_cxl_port_enumerate_dports() is envoked to enumerate and probe
>> the dports.
>>
>> The second phase is if there are any CXL switches. When the pci endpoint
>> device driver (cxl_pci) calls probe, it will add a mem device and triggers
>> the cxl_mem->probe(). cxl_mem->probe() calls devm_cxl_enumerate_ports()
>> and attempts to discovery and create all the ports represent CXL switches.
>> During this phase, a port is created per switch and the attached dports
>> are also enumerated and probed.
>>
>> The last phase is creating endpoint port which happens for all endpoint
>> devices.
>>
>> In this commit, the port create and its dport probing in cxl_acpi is not
>> changed. That will be handled in a different patch later on. The behavior
>> change is only for CXL switch ports. Only the dport that is part of the
>> path for an endpoint device to the RP will be probed. This happens
>> naturally by the code walking up the device hierarchy and identifying the
>> upstream device and the downstream device.
>>
>> There are two points where the interception of dport creation happens
>> during the devm_cxl_enumerate_ports() path. The first location is right
>> before the function calls add_port_attch_ep() where it does the dport
> 
> attach_ep()
> 
>> allocation for the RP. Once the dport is allocated, the iteration path
>> is reset to the beginning to try again. The second location happens
>> in add_port_attach_ep() after the location where either the port is
>> discovered or allocated new if it does not exist.
>>
>> Locking of port device during __cxl_port_add_dport() protects modifications
>> against the port and its dports while multiple endpoints can be probing at
>> the same time and the same port is being modified concurrently.
>>
>> While the decoders are allocated during the port driver probe,
>> The decoders must also be updated since previously it's all done when all
>> the dports are setup and now every time a dport is setup per endpoint, the
>> switch target listing need to be updated with new dport. A
>> guard(rwsem_write) is used to update decoder targets. This is similar to
>> when decoder_populate_target() is called and the decoder programming
>> must be protected.
>>
>> Link: https://lore.kernel.org/linux-cxl/20250305100123.3077031-1-rrichter@amd.com/
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> v4:
>> - Allocate dport when they are found via endpoint probe. (Robert)
> 
> A few comments inline. 
> 
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index b50551601c2e..6e9b5ab69de0 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -24,6 +24,44 @@ static unsigned short media_ready_timeout = 60;
>>  module_param(media_ready_timeout, ushort, 0644);
>>  MODULE_PARM_DESC(media_ready_timeout, "seconds to wait for media ready");
> 
>>  struct cxl_walk_context {
>>  	struct pci_bus *bus;
>>  	struct cxl_port *port;
>> @@ -1169,3 +1207,53 @@ int cxl_gpf_port_setup(struct cxl_dport *dport)
>>  
>>  	return 0;
>>  }
>> +
>> +static int match_dport(struct pci_dev *pdev, void *data)
>> +{
>> +	struct cxl_walk_context *ctx = data;
>> +	int type = pci_pcie_type(pdev);
>> +
>> +	if (pdev->bus != ctx->bus)
>> +		return 0;
>> +	if (!pci_is_pcie(pdev))
>> +		return 0;
>> +	if (type != ctx->type)
> 
> I'm lost.  How would bus have matched, but type not?
> Add a comment.

I don't follow. It could be PCI bus but not the right device type (i.e. not a downstream port or root port). Or do you mean explain that? Sure. 

> 
>> +		return 0;
>> +
>> +	ctx->count++;
>> +	return 0;
>> +}
>> +
>> +int cxl_port_get_total_dports(struct cxl_port *port)
> 
> See comment below on renaming this given it doesn't return
> the number.
> 
>> +{
>> +	struct pci_bus *bus = cxl_port_to_pci_bus(port);
>> +	struct cxl_walk_context ctx;
>> +	int type;
>> +
>> +	if (!bus) {
>> +		dev_err(&port->dev, "No PCI bus found for port %s\n",
>> +			dev_name(&port->dev));
>> +		return -ENXIO;
>> +	}
>> +
>> +	if (pci_is_root_bus(bus))
>> +		type = PCI_EXP_TYPE_ROOT_PORT;
>> +	else
>> +		type = PCI_EXP_TYPE_DOWNSTREAM;
>> +
>> +	ctx = (struct cxl_walk_context) {
>> +		.bus = bus,
>> +		.type = type,
>> +	};
>> +	pci_walk_bus(bus, match_dport, &ctx);
>> +
>> +	port->total_dports = ctx.count;
>> +	if (port->total_dports == 0) {
>> +		dev_warn(&port->dev, "No dports found for port %s on bus %s\n",
>> +			 dev_name(&port->dev), bus->name);
>> +		return -ENXIO;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_port_get_total_dports, "CXL");
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 9691da831224..3872638c439b 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -1551,6 +1551,134 @@ static resource_size_t find_component_registers(struct device *dev)
>>  	return map.resource;
>>  }
> 
> 
> 
>> +
>> +static int devm_cxl_add_dport_by_uport(struct device *uport_dev,
>> +				       struct device *dport_dev,
>> +				       struct cxl_dport **dport)
>> +{
>> +	*dport = NULL;
>> +	struct cxl_port *port __free(put_cxl_port) =
>> +		find_cxl_port_by_uport(uport_dev);
>> +	if (!port)
>> +		return -ENODEV;
>> +
>> +	guard(device)(&port->dev);
>> +	return devm_cxl_port_add_dport(port, dport_dev, dport);
>> +}
>> +
>> +static int devm_cxl_add_dport_for_rp(struct device *uport_dev,
>> +				     struct device *dport_dev,
>> +				     struct cxl_dport **dport)
> 
> That this sets *dport = NULL lead me to get confused at the caller.
> Perhaps instead these could return a dport *, NULL or ERR_PTR()
> instead?

Yeah I went back and forth a bit on this one. I needed a way to distinguish between returning an existing dport vs a newly allocated dport. I suppose I could return PTR_ERR(-EEXIST) and then try to retrieve that dport again. Thoughts?

DJ
>  
>> +{
>> +	struct device *dparent = grandparent(dport_dev);
>> +
>> +	*dport = NULL;
>> +	/* Only go down this path if we are at the root port */
>> +	if (!is_cxl_hierarchy_head(dparent))
>> +		return 0;
>> +
>> +	return devm_cxl_add_dport_by_uport(uport_dev, dport_dev, dport);
>> +}
>> +
>>  static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>>  			      struct device *uport_dev,
>>  			      struct device *dport_dev)
>> @@ -1584,6 +1712,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>>  	 */
>>  	struct cxl_port *port __free(put_cxl_port) = NULL;
>>  	scoped_guard(device, &parent_port->dev) {
>> +		struct cxl_dport *new_dport;
>> +
>>  		if (!parent_port->dev.driver) {
>>  			dev_warn(&cxlmd->dev,
>>  				 "port %s:%s disabled, failed to enumerate CXL.mem\n",
>> @@ -1592,6 +1722,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>>  		}
>>  
>>  		port = find_cxl_port_at(parent_port, dport_dev, &dport);
>> +		if (!port)
>> +			port = find_cxl_port_by_uport(uport_dev);
>>  		if (!port) {
>>  			component_reg_phys = find_component_registers(uport_dev);
>>  			port = devm_cxl_add_port(&parent_port->dev, uport_dev,
>> @@ -1599,11 +1731,17 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>>  			if (IS_ERR(port))
>>  				return PTR_ERR(port);
>>  
>> -			/* retry find to pick up the new dport information */
>> -			port = find_cxl_port_at(parent_port, dport_dev, &dport);
>> -			if (!port)
>> -				return -ENXIO;
>> +			get_device(&port->dev);
> 
> Not immediately obvious to me why a reference needs to be grabbed here. PErhaps
> a comment?
> 
>>  		}
>> +
>> +		guard(device)(&port->dev);
>> +		/* Existing dport is checked with no action done if exists */
>> +		rc = devm_cxl_port_add_dport(port, dport_dev, &new_dport);
> 
> Similar comment to below. Would prefer
> 
> 		new_dport = devm_cxl_port_add_dport(port, dprot_dev);
> 		if (IS_ERR(new_dport))
> 			return PTR_ERR(new_dport);
> 
> 
> Similar approach already used for devm_cxl_add_port()
> 
> 
>> +		if (rc)
>> +			return rc;
>> +
>> +		if (new_dport)
>> +			dport = new_dport;
>>  	}
>>  
>>  	dev_dbg(&cxlmd->dev, "add to new port %s:%s\n",
>> @@ -1620,11 +1758,13 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>>  	return rc;
>>  }
>>  
>> +#define CXL_ITER_LEVEL_SWITCH	1
>> +
>>  int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>>  {
>>  	struct device *dev = &cxlmd->dev;
>>  	struct device *iter;
>> -	int rc;
>> +	int rc, i;
>>  
>>  	/*
>>  	 * Skip intermediate port enumeration in the RCH case, there
>> @@ -1643,7 +1783,7 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>>  	 * attempt fails.
>>  	 */
>>  retry:
>> -	for (iter = dev; iter; iter = grandparent(iter)) {
>> +	for (i = 0, iter = dev; iter; i++, iter = grandparent(iter)) {
>>  		struct device *dport_dev = grandparent(iter);
>>  		struct device *uport_dev;
>>  		struct cxl_dport *dport;
>> @@ -1686,9 +1826,28 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>>  			if (!dev_is_cxl_root_child(&port->dev))
>>  				continue;
>>  
>> +			/*
>> +			 * This is a corner case where the rootport is setup but
>> +			 * the switch dport is not. It needs to go back to the
>> +			 * beginning to setup the switch port.
>> +			 */
>> +			if (i >= CXL_ITER_LEVEL_SWITCH) {
>> +				struct cxl_port *pport __free(put_cxl_port) =
>> +					cxl_mem_find_port(cxlmd, &dport);
>> +				if (!port)
>> +					goto retry;
>> +			}
>> +
>>  			return 0;
>>  		}
>>  
>> +		rc = devm_cxl_add_dport_for_rp(uport_dev, dport_dev, &dport);
> 
> As above, I'd prefer to see
> 		dport = devm_cxl_add_dport_for_rp(uport_dev, dport_dev);
> 		if (IS_ERR(dport))
> 			return PTR_ERR(dport);
> 
> 		if (dport)
> 			goto retry;
> 
> That way it's explicit that dport is going to be updated in all cases within
> devm_cxl_add_dport_for_rp()
> 
>> +		if (rc)
>> +			return rc;
>> +		/* Added a dport, restart enumeration */
>> +		if (dport)
>> +			goto retry;
>> +
>>  		rc = add_port_attach_ep(cxlmd, uport_dev, dport_dev);
>>  		/* port missing, try to add parent */
>>  		if (rc == -EAGAIN)
>> @@ -1727,16 +1886,19 @@ static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
>>  		return 0;
>>  
>>  	device_lock_assert(&port->dev);
>> +	memcpy(cxlsd->target_map, target_map, sizeof(cxlsd->target_map));
>>  
>>  	if (xa_empty(&port->dports))
>> -		return -EINVAL;
>> +		return 0;
>>  
>>  	guard(rwsem_write)(&cxl_region_rwsem);
>>  	for (i = 0; i < cxlsd->cxld.interleave_ways; i++) {
>>  		struct cxl_dport *dport = find_dport(port, target_map[i]);
>>  
>> -		if (!dport)
>> -			return -ENXIO;
>> +		if (!dport) {
>> +			/* dport may be activated later */
>> +			continue;
>> +		}
>>  		cxlsd->target[i] = dport;
>>  	}
> 
> 
> 
>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>> index fe4b593331da..354e134793c3 100644
>> --- a/drivers/cxl/port.c
>> +++ b/drivers/cxl/port.c
>> @@ -65,12 +65,10 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>>  	/* Cache the data early to ensure is_visible() works */
>>  	read_cdat_data(port);
>>  
>> -	rc = devm_cxl_port_enumerate_dports(port);
>> -	if (rc < 0)
>> +	rc = cxl_port_get_total_dports(port);
> 
> A function called cxl_port_get_total_dports() to my thinking should return
> the number of them not fill in a value embedded in port.
> 
> cxl_port_update_total_dports() perhaps?
> 
>> +	if (rc)
>>  		return rc;
>>  
>> -	cxl_switch_parse_cdat(port);
>> -
>>  	cxlhdm = devm_cxl_setup_hdm(port, NULL);
>>  	if (!IS_ERR(cxlhdm))
>>  		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
>> @@ -80,7 +78,7 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>>  		return PTR_ERR(cxlhdm);
>>  	}
>>  
>> -	if (rc == 1) {
>> +	if (port->total_dports == 1) {
>>  		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
>>  		return devm_cxl_add_passthrough_decoder(port);
>>  	}
> 

Re: [PATCH v4 4/9] cxl: Defer dport allocation for switch ports

[Jonathan Cameron]

On Tue, 1 Jul 2025 13:18:16 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 7/1/25 4:10 AM, Jonathan Cameron wrote:
> > On Tue, 24 Jun 2025 14:39:11 -0700
> > Dave Jiang <dave.jiang@intel.com> wrote:
> >   
> >> The current implementation enumerates the dports during the cxl_port
> >> driver probe. Without an endpoint connected, the dport may not be
> >> active during port probe. This scheme may prevent a valid hardware
> >> dport id to be retrieved and MMIO registers to be read when an endpoint
> >> is hot-plugged. Move the dport allocation and setup to behind memdev
> >> probe so the endpoint is guaranteed to be connected.
> >>
> >> In the original enumeration behavior, there are 3 phases (or 2 if no CXL
> >> switches) for port creation. cxl_acpi() creates a Root Port (RP) from the
> >> ACPI0017.N device. Through that it enumerate downstream ports composed
> >> of ACPI0016.N devices through add_host_bridge_dport(). Once done, it
> >> use add_host_bridge_uport() to create the ports that enumerates the PCI
> >> RPs as the dports of these ports. Every time a port is created, the port
> >> driver is attached and drv->probe() is called and
> >> devm_cxl_port_enumerate_dports() is envoked to enumerate and probe
> >> the dports.
> >>
> >> The second phase is if there are any CXL switches. When the pci endpoint
> >> device driver (cxl_pci) calls probe, it will add a mem device and triggers
> >> the cxl_mem->probe(). cxl_mem->probe() calls devm_cxl_enumerate_ports()
> >> and attempts to discovery and create all the ports represent CXL switches.
> >> During this phase, a port is created per switch and the attached dports
> >> are also enumerated and probed.
> >>
> >> The last phase is creating endpoint port which happens for all endpoint
> >> devices.
> >>
> >> In this commit, the port create and its dport probing in cxl_acpi is not
> >> changed. That will be handled in a different patch later on. The behavior
> >> change is only for CXL switch ports. Only the dport that is part of the
> >> path for an endpoint device to the RP will be probed. This happens
> >> naturally by the code walking up the device hierarchy and identifying the
> >> upstream device and the downstream device.
> >>
> >> There are two points where the interception of dport creation happens
> >> during the devm_cxl_enumerate_ports() path. The first location is right
> >> before the function calls add_port_attch_ep() where it does the dport  
> > 
> > attach_ep()
> >   
> >> allocation for the RP. Once the dport is allocated, the iteration path
> >> is reset to the beginning to try again. The second location happens
> >> in add_port_attach_ep() after the location where either the port is
> >> discovered or allocated new if it does not exist.
> >>
> >> Locking of port device during __cxl_port_add_dport() protects modifications
> >> against the port and its dports while multiple endpoints can be probing at
> >> the same time and the same port is being modified concurrently.
> >>
> >> While the decoders are allocated during the port driver probe,
> >> The decoders must also be updated since previously it's all done when all
> >> the dports are setup and now every time a dport is setup per endpoint, the
> >> switch target listing need to be updated with new dport. A
> >> guard(rwsem_write) is used to update decoder targets. This is similar to
> >> when decoder_populate_target() is called and the decoder programming
> >> must be protected.
> >>
> >> Link: https://lore.kernel.org/linux-cxl/20250305100123.3077031-1-rrichter@amd.com/
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >> ---
> >> v4:
> >> - Allocate dport when they are found via endpoint probe. (Robert)  
> > 
> > A few comments inline. 
> >   
> >> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> >> index b50551601c2e..6e9b5ab69de0 100644
> >> --- a/drivers/cxl/core/pci.c
> >> +++ b/drivers/cxl/core/pci.c
> >> @@ -24,6 +24,44 @@ static unsigned short media_ready_timeout = 60;
> >>  module_param(media_ready_timeout, ushort, 0644);
> >>  MODULE_PARM_DESC(media_ready_timeout, "seconds to wait for media ready");  
> >   
> >>  struct cxl_walk_context {
> >>  	struct pci_bus *bus;
> >>  	struct cxl_port *port;
> >> @@ -1169,3 +1207,53 @@ int cxl_gpf_port_setup(struct cxl_dport *dport)
> >>  
> >>  	return 0;
> >>  }
> >> +
> >> +static int match_dport(struct pci_dev *pdev, void *data)
> >> +{
> >> +	struct cxl_walk_context *ctx = data;
> >> +	int type = pci_pcie_type(pdev);
> >> +
> >> +	if (pdev->bus != ctx->bus)
> >> +		return 0;
> >> +	if (!pci_is_pcie(pdev))
> >> +		return 0;
> >> +	if (type != ctx->type)  
> > 
> > I'm lost.  How would bus have matched, but type not?
> > Add a comment.  
> 
> I don't follow. It could be PCI bus but not the right device type (i.e. not a downstream port or root port). Or do you mean explain that? Sure. 

Miss read. I'd confused bus and subordinate though
even that would not be enough as might be a usp.

So this is fine, ignore me :)




> 
> >   
> >> +		return 0;
> >> +
> >> +	ctx->count++;
> >> +	return 0;
> >> +}
> >> +
> >> +int cxl_port_get_total_dports(struct cxl_port *port)  
> > 
> > See comment below on renaming this given it doesn't return
> > the number.
> >   
> >> +{
> >> +	struct pci_bus *bus = cxl_port_to_pci_bus(port);
> >> +	struct cxl_walk_context ctx;
> >> +	int type;
> >> +
> >> +	if (!bus) {
> >> +		dev_err(&port->dev, "No PCI bus found for port %s\n",
> >> +			dev_name(&port->dev));
> >> +		return -ENXIO;
> >> +	}
> >> +
> >> +	if (pci_is_root_bus(bus))
> >> +		type = PCI_EXP_TYPE_ROOT_PORT;
> >> +	else
> >> +		type = PCI_EXP_TYPE_DOWNSTREAM;
> >> +
> >> +	ctx = (struct cxl_walk_context) {
> >> +		.bus = bus,
> >> +		.type = type,
> >> +	};
> >> +	pci_walk_bus(bus, match_dport, &ctx);
> >> +
> >> +	port->total_dports = ctx.count;
> >> +	if (port->total_dports == 0) {
> >> +		dev_warn(&port->dev, "No dports found for port %s on bus %s\n",
> >> +			 dev_name(&port->dev), bus->name);
> >> +		return -ENXIO;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +EXPORT_SYMBOL_NS_GPL(cxl_port_get_total_dports, "CXL");
> >> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> >> index 9691da831224..3872638c439b 100644
> >> --- a/drivers/cxl/core/port.c
> >> +++ b/drivers/cxl/core/port.c
> >> @@ -1551,6 +1551,134 @@ static resource_size_t find_component_registers(struct device *dev)
> >>  	return map.resource;
> >>  }  
> > 
> > 
> >   
> >> +
> >> +static int devm_cxl_add_dport_by_uport(struct device *uport_dev,
> >> +				       struct device *dport_dev,
> >> +				       struct cxl_dport **dport)
> >> +{
> >> +	*dport = NULL;
> >> +	struct cxl_port *port __free(put_cxl_port) =
> >> +		find_cxl_port_by_uport(uport_dev);
> >> +	if (!port)
> >> +		return -ENODEV;
> >> +
> >> +	guard(device)(&port->dev);
> >> +	return devm_cxl_port_add_dport(port, dport_dev, dport);
> >> +}
> >> +
> >> +static int devm_cxl_add_dport_for_rp(struct device *uport_dev,
> >> +				     struct device *dport_dev,
> >> +				     struct cxl_dport **dport)  
> > 
> > That this sets *dport = NULL lead me to get confused at the caller.
> > Perhaps instead these could return a dport *, NULL or ERR_PTR()
> > instead?  
> 
> Yeah I went back and forth a bit on this one. I needed a way to
distinguish between returning an existing dport vs a newly allocated dport.
I suppose I could return PTR_ERR(-EEXIST) and then try to retrieve that dport again. Thoughts?

Using assignment or not after reporting success on something called 'add' is a rather non intuitive.
I was thinking NULL but -EEXIST is more explicit so agreed that
would work and doesn't look like the handling will be too bad

	dport = devm_cxl_add_dport_for_rp(uport_dev, dport_dev);
	if (!IS_ERR(dport)) /* Added dport so restart enumeration */
		goto retry;
	if (PTR_ERR(dport != -EEXIST))
		return PTR_ERR(dport);

or something along those lines.

[PATCH v4 5/9] cxl/test: Add cxl_test support for new dport allocation scheme

[Dave Jiang]

In the delayed dport allocation scheme, the total number of dports
need to be discovered during port probe. Add the mock function
that does it for cxl_test.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 tools/testing/cxl/Kbuild      |  1 +
 tools/testing/cxl/test/cxl.c  | 60 +++++++++++++++++++++++++++++++++++
 tools/testing/cxl/test/mock.c | 15 +++++++++
 tools/testing/cxl/test/mock.h |  1 +
 4 files changed, 77 insertions(+)

diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index 31a2d73c963f..1f05497c44e1 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -5,6 +5,7 @@ ldflags-y += --wrap=acpi_evaluate_integer
 ldflags-y += --wrap=acpi_pci_find_root
 ldflags-y += --wrap=nvdimm_bus_register
 ldflags-y += --wrap=devm_cxl_port_enumerate_dports
+ldflags-y += --wrap=cxl_port_get_total_dports
 ldflags-y += --wrap=devm_cxl_setup_hdm
 ldflags-y += --wrap=devm_cxl_add_passthrough_decoder
 ldflags-y += --wrap=devm_cxl_enumerate_decoders
diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index 8a5815ca870d..5d6585d4b15e 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -920,6 +920,65 @@ static int mock_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 	return 0;
 }
 
+static int mock_cxl_port_get_total_dports(struct cxl_port *port)
+{
+	struct platform_device **array;
+	int i, array_size;
+	int dports = 0;
+
+	if (port->depth == 1) {
+		if (is_multi_bridge(port->uport_dev)) {
+			array_size = ARRAY_SIZE(cxl_root_port);
+			array = cxl_root_port;
+		} else if (is_single_bridge(port->uport_dev)) {
+			array_size = ARRAY_SIZE(cxl_root_single);
+			array = cxl_root_single;
+		} else {
+			dev_dbg(&port->dev, "%s: unknown bridge type\n",
+				dev_name(port->uport_dev));
+			return -ENXIO;
+		}
+	} else if (port->depth == 2) {
+		struct cxl_port *parent = to_cxl_port(port->dev.parent);
+
+		if (is_multi_bridge(parent->uport_dev)) {
+			array_size = ARRAY_SIZE(cxl_switch_dport);
+			array = cxl_switch_dport;
+		} else if (is_single_bridge(parent->uport_dev)) {
+			array_size = ARRAY_SIZE(cxl_swd_single);
+			array = cxl_swd_single;
+		} else {
+			dev_dbg(&port->dev, "%s: unknown bridge type\n",
+				dev_name(port->uport_dev));
+			return -ENXIO;
+		}
+	} else {
+		dev_WARN_ONCE(&port->dev, 1, "unexpected depth %d\n",
+			      port->depth);
+		return -ENXIO;
+	}
+
+	for (i = 0; i < array_size; i++) {
+		struct platform_device *pdev = array[i];
+
+		if (pdev->dev.parent != port->uport_dev) {
+			dev_dbg(&port->dev, "%s: mismatch parent %s\n",
+				dev_name(port->uport_dev),
+				dev_name(pdev->dev.parent));
+			continue;
+		}
+
+		dports++;
+	}
+
+	if (!dports)
+		return -ENXIO;
+
+	port->total_dports = dports;
+
+	return 0;
+}
+
 static int mock_cxl_port_enumerate_dports(struct cxl_port *port)
 {
 	struct platform_device **array;
@@ -1035,6 +1094,7 @@ static struct cxl_mock_ops cxl_mock_ops = {
 	.acpi_evaluate_integer = mock_acpi_evaluate_integer,
 	.acpi_pci_find_root = mock_acpi_pci_find_root,
 	.devm_cxl_port_enumerate_dports = mock_cxl_port_enumerate_dports,
+	.cxl_port_get_total_dports = mock_cxl_port_get_total_dports,
 	.devm_cxl_setup_hdm = mock_cxl_setup_hdm,
 	.devm_cxl_add_passthrough_decoder = mock_cxl_add_passthrough_decoder,
 	.devm_cxl_enumerate_decoders = mock_cxl_enumerate_decoders,
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index 1989ae020df3..c92cc3a4f0b9 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -196,6 +196,21 @@ int __wrap_devm_cxl_port_enumerate_dports(struct cxl_port *port)
 }
 EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_port_enumerate_dports, "CXL");
 
+int __wrap_cxl_port_get_total_dports(struct cxl_port *port)
+{
+	int rc, index;
+	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
+
+	if (ops && ops->is_mock_port(port->uport_dev))
+		rc = ops->cxl_port_get_total_dports(port);
+	else
+		rc = cxl_port_get_total_dports(port);
+	put_cxl_mock_ops(index);
+
+	return rc;
+}
+EXPORT_SYMBOL_NS_GPL(__wrap_cxl_port_get_total_dports, "CXL");
+
 int __wrap_cxl_await_media_ready(struct cxl_dev_state *cxlds)
 {
 	int rc, index;
diff --git a/tools/testing/cxl/test/mock.h b/tools/testing/cxl/test/mock.h
index d1b0271d2822..9f9685d0f00c 100644
--- a/tools/testing/cxl/test/mock.h
+++ b/tools/testing/cxl/test/mock.h
@@ -20,6 +20,7 @@ struct cxl_mock_ops {
 	bool (*is_mock_port)(struct device *dev);
 	bool (*is_mock_dev)(struct device *dev);
 	int (*devm_cxl_port_enumerate_dports)(struct cxl_port *port);
+	int (*cxl_port_get_total_dports)(struct cxl_port *port);
 	struct cxl_hdm *(*devm_cxl_setup_hdm)(
 		struct cxl_port *port, struct cxl_endpoint_dvsec_info *info);
 	int (*devm_cxl_add_passthrough_decoder)(struct cxl_port *port);
-- 
2.49.0

Re: [PATCH v4 5/9] cxl/test: Add cxl_test support for new dport allocation scheme

[Jonathan Cameron]

On Tue, 24 Jun 2025 14:39:12 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> In the delayed dport allocation scheme, the total number of dports
> need to be discovered during port probe. Add the mock function
> that does it for cxl_test.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

I'm far from an expert on cxl_test but this looks sensible to me
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
 

[PATCH v4 6/9] cxl/test: Add mock version of devm_cxl_add_dport_by_dev()

[Dave Jiang]

devm_cxl_add_dport_by_dev() outside of cxl_test is done through PCI
hierarchy. However with cxl_test, it needs to be done through the
platform device hierarchy. Add the mock function for
devm_cxl_add_dport_by_dev().

When cxl_core calls a cxl_core exported function and that function is
mocked by cxl_test, the call chain causes a circular dependency issue. Dan
provided a workaround to avoid this issue. Apply the method to changes from
the late dport allocation changes in order to enable cxl-test.

In cxl_core they are defined with "__" added in front of the function. A
macro is used to define the original function names for when non-test
version of the kernel is built. A bit of macros and typedefs are used to
allow mocking of those functions in cxl_test.

Co-developed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/core.h              |  2 -
 drivers/cxl/core/pci.c               |  7 ++--
 drivers/cxl/cxl.h                    | 19 +++++++++
 tools/testing/cxl/Kbuild             |  1 +
 tools/testing/cxl/cxl_core_exports.c | 12 ++++++
 tools/testing/cxl/exports.h          | 10 +++++
 tools/testing/cxl/test/cxl.c         | 59 ++++++++++++++++++++++++++++
 tools/testing/cxl/test/mock.c        | 23 +++++++++++
 tools/testing/cxl/test/mock.h        |  2 +
 9 files changed, 130 insertions(+), 5 deletions(-)
 create mode 100644 tools/testing/cxl/exports.h

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 8cfead6f3b08..29b61828a847 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -122,8 +122,6 @@ void cxl_ras_exit(void);
 int cxl_gpf_port_setup(struct cxl_dport *dport);
 int cxl_acpi_get_extended_linear_cache_size(struct resource *backing_res,
 					    int nid, resource_size_t *size);
-struct cxl_dport *devm_cxl_add_dport_by_dev(struct cxl_port *port,
-					    struct device *dport_dev);
 
 #ifdef CONFIG_CXL_FEATURES
 struct cxl_feat_entry *
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 6e9b5ab69de0..cf3cbd674b1c 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -25,14 +25,14 @@ module_param(media_ready_timeout, ushort, 0644);
 MODULE_PARM_DESC(media_ready_timeout, "seconds to wait for media ready");
 
 /**
- * devm_cxl_add_dport_by_dev - allocate a dport by dport device
+ * __devm_cxl_add_dport_by_dev - allocate a dport by dport device
  * @port: cxl_port that hosts the dport
  * @dport_dev: 'struct device' of the dport
  *
  * Returns the allocate dport on success or ERR_PTR() of -errno on error
  */
-struct cxl_dport *devm_cxl_add_dport_by_dev(struct cxl_port *port,
-					    struct device *dport_dev)
+struct cxl_dport *__devm_cxl_add_dport_by_dev(struct cxl_port *port,
+					      struct device *dport_dev)
 {
 	struct cxl_register_map map;
 	struct pci_dev *pdev;
@@ -61,6 +61,7 @@ struct cxl_dport *devm_cxl_add_dport_by_dev(struct cxl_port *port,
 	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
 	return devm_cxl_add_dport(port, &pdev->dev, port_num, map.resource);
 }
+EXPORT_SYMBOL_NS_GPL(__devm_cxl_add_dport_by_dev, "CXL");
 
 struct cxl_walk_context {
 	struct pci_bus *bus;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 5f598fb542e5..a1596c9561a9 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -907,6 +907,10 @@ void cxl_coordinates_combine(struct access_coordinate *out,
 
 bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port);
 int cxl_port_get_total_dports(struct cxl_port *port);
+struct cxl_dport *devm_cxl_add_dport_by_dev(struct cxl_port *port,
+					    struct device *dport_dev);
+struct cxl_dport *__devm_cxl_add_dport_by_dev(struct cxl_port *port,
+					      struct device *dport_dev);
 
 /*
  * Unit test builds overrides this to __weak, find the 'strong' version
@@ -928,4 +932,19 @@ static inline struct rw_semaphore *rwsem_read_intr_acquire(struct rw_semaphore *
 
 DEFINE_FREE(rwsem_read_release, struct rw_semaphore *, if (_T) up_read(_T))
 
+/*
+ * Declaration for functions that are mocked by cxl_test that are called by
+ * cxl_core. The respective functions are defined as __foo() and called by
+ * cxl_core as foo(). The macros below ensures that those functions would
+ * exist as foo(). See tools/testing/cxl/cxl_core_exports.c and
+ * tools/testing/cxl/exports.h for setting up the mock functions. The dance
+ * is done to avoid a circular dependency where cxl_core calls a function that
+ * ends up being a mock function and goes to * cxl_test where it calls a
+ * cxl_core function.
+ */
+#ifndef CXL_TEST_ENABLE
+#define DECLARE_TESTABLE(x) __##x
+#define devm_cxl_add_dport_by_dev DECLARE_TESTABLE(devm_cxl_add_dport_by_dev)
+#endif
+
 #endif /* __CXL_H__ */
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index 1f05497c44e1..d77717645ac7 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -22,6 +22,7 @@ CXL_SRC := $(DRIVERS)/cxl
 CXL_CORE_SRC := $(DRIVERS)/cxl/core
 ccflags-y := -I$(srctree)/drivers/cxl/
 ccflags-y += -D__mock=__weak
+ccflags-y += -DCXL_TEST_ENABLE=1
 ccflags-y += -DTRACE_INCLUDE_PATH=$(CXL_CORE_SRC) -I$(srctree)/drivers/cxl/core/
 
 obj-m += cxl_acpi.o
diff --git a/tools/testing/cxl/cxl_core_exports.c b/tools/testing/cxl/cxl_core_exports.c
index f088792a8925..0d18abc1f5a3 100644
--- a/tools/testing/cxl/cxl_core_exports.c
+++ b/tools/testing/cxl/cxl_core_exports.c
@@ -2,6 +2,18 @@
 /* Copyright(c) 2022 Intel Corporation. All rights reserved. */
 
 #include "cxl.h"
+#include "exports.h"
 
 /* Exporting of cxl_core symbols that are only used by cxl_test */
 EXPORT_SYMBOL_NS_GPL(cxl_num_decoders_committed, "CXL");
+
+cxl_add_dport_by_dev_fn _devm_cxl_add_dport_by_dev =
+	__devm_cxl_add_dport_by_dev;
+EXPORT_SYMBOL_NS_GPL(_devm_cxl_add_dport_by_dev, "CXL");
+
+struct cxl_dport *devm_cxl_add_dport_by_dev(struct cxl_port *port,
+					    struct device *dport_dev)
+{
+	return _devm_cxl_add_dport_by_dev(port, dport_dev);
+}
+EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport_by_dev, "CXL");
diff --git a/tools/testing/cxl/exports.h b/tools/testing/cxl/exports.h
new file mode 100644
index 000000000000..9261ce6f1197
--- /dev/null
+++ b/tools/testing/cxl/exports.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2025 Intel Corporation */
+#ifndef __MOCK_CXL_EXPORTS_H_
+#define __MOCK_CXL_EXPORTS_H_
+
+typedef struct cxl_dport *(*cxl_add_dport_by_dev_fn)(struct cxl_port *port,
+							  struct device *dport_dev);
+extern cxl_add_dport_by_dev_fn _devm_cxl_add_dport_by_dev;
+
+#endif
diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index 5d6585d4b15e..b13f9796687c 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -1037,6 +1037,64 @@ static int mock_cxl_port_enumerate_dports(struct cxl_port *port)
 	return 0;
 }
 
+static struct cxl_dport *mock_cxl_add_dport_by_dev(struct cxl_port *port,
+						   struct device *dport_dev)
+{
+	struct platform_device **array;
+	int i, array_size;
+
+	if (port->depth == 1) {
+		if (is_multi_bridge(port->uport_dev)) {
+			array_size = ARRAY_SIZE(cxl_root_port);
+			array = cxl_root_port;
+		} else if (is_single_bridge(port->uport_dev)) {
+			array_size = ARRAY_SIZE(cxl_root_single);
+			array = cxl_root_single;
+		} else {
+			dev_dbg(&port->dev, "%s: unknown bridge type\n",
+				dev_name(port->uport_dev));
+			return ERR_PTR(-ENXIO);
+		}
+	} else if (port->depth == 2) {
+		struct cxl_port *parent = to_cxl_port(port->dev.parent);
+
+		if (is_multi_bridge(parent->uport_dev)) {
+			array_size = ARRAY_SIZE(cxl_switch_dport);
+			array = cxl_switch_dport;
+		} else if (is_single_bridge(parent->uport_dev)) {
+			array_size = ARRAY_SIZE(cxl_swd_single);
+			array = cxl_swd_single;
+		} else {
+			dev_dbg(&port->dev, "%s: unknown bridge type\n",
+				dev_name(port->uport_dev));
+			return ERR_PTR(-ENXIO);
+		}
+	} else {
+		dev_WARN_ONCE(&port->dev, 1, "unexpected depth %d\n",
+			      port->depth);
+		return ERR_PTR(-ENXIO);
+	}
+
+	for (i = 0; i < array_size; i++) {
+		struct platform_device *pdev = array[i];
+
+		if (pdev->dev.parent != port->uport_dev) {
+			dev_dbg(&port->dev, "%s: mismatch parent %s\n",
+				dev_name(port->uport_dev),
+				dev_name(pdev->dev.parent));
+			continue;
+		}
+
+		if (&pdev->dev != dport_dev)
+			continue;
+
+		return devm_cxl_add_dport(port, &pdev->dev, pdev->id,
+					  CXL_RESOURCE_NONE);
+	}
+
+	return ERR_PTR(-ENODEV);
+}
+
 /*
  * Faking the cxl_dpa_perf for the memdev when appropriate.
  */
@@ -1099,6 +1157,7 @@ static struct cxl_mock_ops cxl_mock_ops = {
 	.devm_cxl_add_passthrough_decoder = mock_cxl_add_passthrough_decoder,
 	.devm_cxl_enumerate_decoders = mock_cxl_enumerate_decoders,
 	.cxl_endpoint_parse_cdat = mock_cxl_endpoint_parse_cdat,
+	.devm_cxl_add_dport_by_dev = mock_cxl_add_dport_by_dev,
 	.list = LIST_HEAD_INIT(cxl_mock_ops.list),
 };
 
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index c92cc3a4f0b9..d9fdaaf311cd 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -10,12 +10,18 @@
 #include <cxlmem.h>
 #include <cxlpci.h>
 #include "mock.h"
+#include "../exports.h"
 
 static LIST_HEAD(mock);
 
+static struct cxl_dport *
+redirect_devm_cxl_add_dport_by_dev(struct cxl_port *port,
+				   struct device *dport_dev);
+
 void register_cxl_mock_ops(struct cxl_mock_ops *ops)
 {
 	list_add_rcu(&ops->list, &mock);
+	_devm_cxl_add_dport_by_dev = redirect_devm_cxl_add_dport_by_dev;
 }
 EXPORT_SYMBOL_GPL(register_cxl_mock_ops);
 
@@ -23,6 +29,7 @@ DEFINE_STATIC_SRCU(cxl_mock_srcu);
 
 void unregister_cxl_mock_ops(struct cxl_mock_ops *ops)
 {
+	_devm_cxl_add_dport_by_dev = __devm_cxl_add_dport_by_dev;
 	list_del_rcu(&ops->list);
 	synchronize_srcu(&cxl_mock_srcu);
 }
@@ -326,6 +333,22 @@ void __wrap_cxl_dport_init_ras_reporting(struct cxl_dport *dport, struct device
 }
 EXPORT_SYMBOL_NS_GPL(__wrap_cxl_dport_init_ras_reporting, "CXL");
 
+struct cxl_dport *redirect_devm_cxl_add_dport_by_dev(struct cxl_port *port,
+						     struct device *dport_dev)
+{
+	int index;
+	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
+	struct cxl_dport *dport;
+
+	if (ops && ops->is_mock_port(port->uport_dev))
+		dport = ops->devm_cxl_add_dport_by_dev(port, dport_dev);
+	else
+		dport = devm_cxl_add_dport_by_dev(port, dport_dev);
+	put_cxl_mock_ops(index);
+
+	return dport;
+}
+
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("cxl_test: emulation module");
 MODULE_IMPORT_NS("ACPI");
diff --git a/tools/testing/cxl/test/mock.h b/tools/testing/cxl/test/mock.h
index 9f9685d0f00c..60aa7ea5a0f3 100644
--- a/tools/testing/cxl/test/mock.h
+++ b/tools/testing/cxl/test/mock.h
@@ -27,6 +27,8 @@ struct cxl_mock_ops {
 	int (*devm_cxl_enumerate_decoders)(
 		struct cxl_hdm *hdm, struct cxl_endpoint_dvsec_info *info);
 	void (*cxl_endpoint_parse_cdat)(struct cxl_port *port);
+	struct cxl_dport *(*devm_cxl_add_dport_by_dev)(
+		struct cxl_port *port, struct device *dport_dev);
 };
 
 void register_cxl_mock_ops(struct cxl_mock_ops *ops);
-- 
2.49.0

Re: [PATCH v4 6/9] cxl/test: Add mock version of devm_cxl_add_dport_by_dev()

[Jonathan Cameron]

On Tue, 24 Jun 2025 14:39:13 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> devm_cxl_add_dport_by_dev() outside of cxl_test is done through PCI
> hierarchy. However with cxl_test, it needs to be done through the
> platform device hierarchy. Add the mock function for
> devm_cxl_add_dport_by_dev().
> 
> When cxl_core calls a cxl_core exported function and that function is
> mocked by cxl_test, the call chain causes a circular dependency issue. Dan
> provided a workaround to avoid this issue. Apply the method to changes from
> the late dport allocation changes in order to enable cxl-test.
> 
> In cxl_core they are defined with "__" added in front of the function. A
> macro is used to define the original function names for when non-test
> version of the kernel is built. A bit of macros and typedefs are used to
> allow mocking of those functions in cxl_test.
> 
> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

One comment inline.  I'm not fond of the macro dance in here but don't have
a better suggestion, so fair enough.

> index 5d6585d4b15e..b13f9796687c 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -1037,6 +1037,64 @@ static int mock_cxl_port_enumerate_dports(struct cxl_port *port)
>  	return 0;
>  }
>  
> +static struct cxl_dport *mock_cxl_add_dport_by_dev(struct cxl_port *port,
> +						   struct device *dport_dev)
> +{
> +	struct platform_device **array;
> +	int i, array_size;
> +
> +	if (port->depth == 1) {
> +		if (is_multi_bridge(port->uport_dev)) {
> +			array_size = ARRAY_SIZE(cxl_root_port);
> +			array = cxl_root_port;
> +		} else if (is_single_bridge(port->uport_dev)) {
> +			array_size = ARRAY_SIZE(cxl_root_single);
> +			array = cxl_root_single;
> +		} else {
> +			dev_dbg(&port->dev, "%s: unknown bridge type\n",
> +				dev_name(port->uport_dev));
> +			return ERR_PTR(-ENXIO);
> +		}

This is all duplicating code in mock_cxl_port_get_total_dports.  
Helper to at least get the array_size and array?  Probably not
worth combining the final loops.

> +	} else if (port->depth == 2) {
> +		struct cxl_port *parent = to_cxl_port(port->dev.parent);
> +
> +		if (is_multi_bridge(parent->uport_dev)) {
> +			array_size = ARRAY_SIZE(cxl_switch_dport);
> +			array = cxl_switch_dport;
> +		} else if (is_single_bridge(parent->uport_dev)) {
> +			array_size = ARRAY_SIZE(cxl_swd_single);
> +			array = cxl_swd_single;
> +		} else {
> +			dev_dbg(&port->dev, "%s: unknown bridge type\n",
> +				dev_name(port->uport_dev));
> +			return ERR_PTR(-ENXIO);
> +		}
> +	} else {
> +		dev_WARN_ONCE(&port->dev, 1, "unexpected depth %d\n",
> +			      port->depth);
> +		return ERR_PTR(-ENXIO);
> +	}
> +
> +	for (i = 0; i < array_size; i++) {
> +		struct platform_device *pdev = array[i];
> +
> +		if (pdev->dev.parent != port->uport_dev) {
> +			dev_dbg(&port->dev, "%s: mismatch parent %s\n",
> +				dev_name(port->uport_dev),
> +				dev_name(pdev->dev.parent));
> +			continue;
> +		}
> +
> +		if (&pdev->dev != dport_dev)
> +			continue;
> +
> +		return devm_cxl_add_dport(port, &pdev->dev, pdev->id,
> +					  CXL_RESOURCE_NONE);
> +	}
> +
> +	return ERR_PTR(-ENODEV);
> +}

[PATCH v4 7/9] cxl: Change sslbis handler to only handle single dport

[Dave Jiang]

While cxl_switch_parse_cdat() is harmless to be run multiple times, it is
not efficient in the current scheme where one dport is being updated at
a time by the memdev probe path. Change the input parameter to the
specific dport being updated to pick up the SSLBIS information for just
that dport.

Reviewed-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Li Ming <ming.li@zohomail.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/cdat.c | 23 ++++++++++-------------
 drivers/cxl/core/port.c |  2 +-
 drivers/cxl/cxl.h       |  2 +-
 3 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index 0ccef2f2a26a..58c21a2f22c1 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -440,8 +440,8 @@ static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg,
 	} *tbl = (struct acpi_cdat_sslbis_table *)header;
 	int size = sizeof(header->cdat) + sizeof(tbl->sslbis_header);
 	struct acpi_cdat_sslbis *sslbis;
-	struct cxl_port *port = arg;
-	struct device *dev = &port->dev;
+	struct cxl_dport *dport = arg;
+	struct device *dev = &dport->port->dev;
 	int remain, entries, i;
 	u16 len;
 
@@ -467,8 +467,6 @@ static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg,
 		u16 y = le16_to_cpu((__force __le16)tbl->entries[i].porty_id);
 		__le64 le_base;
 		__le16 le_val;
-		struct cxl_dport *dport;
-		unsigned long index;
 		u16 dsp_id;
 		u64 val;
 
@@ -499,28 +497,27 @@ static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg,
 		val = cdat_normalize(le16_to_cpu(le_val), le64_to_cpu(le_base),
 				     sslbis->data_type);
 
-		xa_for_each(&port->dports, index, dport) {
-			if (dsp_id == ACPI_CDAT_SSLBIS_ANY_PORT ||
-			    dsp_id == dport->port_id) {
-				cxl_access_coordinate_set(dport->coord,
-							  sslbis->data_type,
-							  val);
-			}
+		if (dsp_id == ACPI_CDAT_SSLBIS_ANY_PORT ||
+		    dsp_id == dport->port_id) {
+			cxl_access_coordinate_set(dport->coord,
+						  sslbis->data_type, val);
+			return 0;
 		}
 	}
 
 	return 0;
 }
 
-void cxl_switch_parse_cdat(struct cxl_port *port)
+void cxl_switch_parse_cdat(struct cxl_dport *dport)
 {
+	struct cxl_port *port = dport->port;
 	int rc;
 
 	if (!port->cdat.table)
 		return;
 
 	rc = cdat_table_parse(ACPI_CDAT_TYPE_SSLBIS, cdat_sslbis_handler,
-			      port, port->cdat.table, port->cdat.length);
+			      dport, port->cdat.table, port->cdat.length);
 	rc = cdat_table_parse_output(rc);
 	if (rc)
 		dev_dbg(&port->dev, "Failed to parse SSLBIS: %d\n", rc);
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 3872638c439b..4f7ac4f0d3cf 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1614,7 +1614,7 @@ static int cxl_port_setup_with_dport(struct cxl_port *port,
 {
 	device_lock_assert(&port->dev);
 
-	cxl_switch_parse_cdat(port);
+	cxl_switch_parse_cdat(dport);
 
 	return update_decoders_with_dport(port, dport);
 }
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index a1596c9561a9..2b06f6099959 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -891,7 +891,7 @@ static inline u64 cxl_port_get_spa_cache_alias(struct cxl_port *endpoint,
 #endif
 
 void cxl_endpoint_parse_cdat(struct cxl_port *port);
-void cxl_switch_parse_cdat(struct cxl_port *port);
+void cxl_switch_parse_cdat(struct cxl_dport *dport);
 
 int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 				      struct access_coordinate *coord);
-- 
2.49.0

[PATCH v4 8/9] cxl: Create an xarray to tie a host bridge to the cxl_root

[Dave Jiang]

Add helper functions to setup association of a host bridge device to a
related cxl_root. Functions are in preparation to support the moving
of host bridge ports creation from cxl_acpi to cxl_memdev probe path.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/port.c | 53 +++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxl.h       |  4 ++++
 2 files changed, 57 insertions(+)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 4f7ac4f0d3cf..20d2f834bf94 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -38,6 +38,7 @@ DECLARE_RWSEM(cxl_region_rwsem);
 
 static DEFINE_IDA(cxl_port_ida);
 static DEFINE_XARRAY(cxl_root_buses);
+static DEFINE_XARRAY(cxl_root_ports);
 
 /*
  * The terminal device in PCI is NULL and @platform_bus
@@ -1014,6 +1015,58 @@ int devm_cxl_register_pci_bus(struct device *host, struct device *uport_dev,
 }
 EXPORT_SYMBOL_NS_GPL(devm_cxl_register_pci_bus, "CXL");
 
+/**
+ * cxl_hb_uport_dev_to_root - Retrieve cxl_root tied to the host bridge device
+ * @hb_uport_dev: host bridge upstream port device
+ *
+ * Return cxl_root on success or NULL on failure
+ *
+ * A reference is taken on the port device. Caller needs to call put_device()
+ * when done.
+ */
+struct cxl_root *cxl_hb_uport_dev_to_root(struct device *hb_uport_dev)
+{
+	struct cxl_root *root;
+
+	root = xa_load(&cxl_root_ports, (unsigned long)hb_uport_dev);
+	if (!root)
+		return NULL;
+
+	get_device(&root->port.dev);
+
+	return root;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_hb_uport_dev_to_root, "CXL");
+
+static void unregister_hb_uport_root_ports(void *hb_uport_dev)
+{
+	xa_erase(&cxl_root_ports, (unsigned long)hb_uport_dev);
+}
+
+/**
+ * devm_cxl_register_hb_uport_root_port - Tie a hostbridge device to a root port
+ * @host: device that hosts the memory for the xarray entries
+ * @hb_uport_dev: host bridge device that serves as the xarray index
+ * @root: cxl_root that serves as the xarray entry data
+ *
+ * Return 0 on success or -errno on failure.
+ */
+int devm_cxl_register_hb_uport_root_port(struct device *host,
+					 struct device *hb_uport_dev,
+					 struct cxl_root *root)
+{
+	int rc;
+
+	rc = xa_insert(&cxl_root_ports, (unsigned long)hb_uport_dev, root,
+		       GFP_KERNEL);
+	if (rc)
+		return rc;
+
+	return devm_add_action_or_reset(host, unregister_hb_uport_root_ports,
+					hb_uport_dev);
+}
+EXPORT_SYMBOL_NS_GPL(devm_cxl_register_hb_uport_root_port, "CXL");
+
 static bool dev_is_cxl_root_child(struct device *dev)
 {
 	struct cxl_port *port, *parent;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 2b06f6099959..3973bf11043f 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -734,6 +734,10 @@ struct pci_bus;
 int devm_cxl_register_pci_bus(struct device *host, struct device *uport_dev,
 			      struct pci_bus *bus);
 struct pci_bus *cxl_port_to_pci_bus(struct cxl_port *port);
+int devm_cxl_register_hb_uport_root_port(struct device *host,
+					 struct device *hb_uport_dev,
+					 struct cxl_root *root);
+struct cxl_root *cxl_hb_uport_dev_to_root(struct device *uport_dev);
 struct cxl_port *devm_cxl_add_port(struct device *host,
 				   struct device *uport_dev,
 				   resource_size_t component_reg_phys,
-- 
2.49.0

Re: [PATCH v4 8/9] cxl: Create an xarray to tie a host bridge to the cxl_root

[Jonathan Cameron]

On Tue, 24 Jun 2025 14:39:15 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add helper functions to setup association of a host bridge device to a
> related cxl_root. Functions are in preparation to support the moving
> of host bridge ports creation from cxl_acpi to cxl_memdev probe path.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

[PATCH v4 9/9] cxl: Move enumeration of hostbridge ports to the memdev probe path

[Dave Jiang]

Current enuemration scheme in cxl_acpi module creates the ports under the
root port by enumerating the hostbridges after the dports under the root
port is created. However error messages "cxl portN: Couldn't locate the
CXL.cache and CXL.mem capability array header" is observed when certain
platform has PCIe hotplug option turned on in BIOS. If the cxl_acpi module
probe is running before the CXL link between the endpoint device and the
RP is established, then the platform may not have exposed DVSEC ID 3 and/or
DVSEC ID 7 blocks which will trigger the error message. This behavior
is defined by the spec and not a hardware quirk.

Setup an association in cxl_port to tie the host bridge device to the
associated cxl_root. The cxl_root provides a callback that's setup
by the cxl_acpi probe function in order to create a port per host bridge
that was previously done during cxl_acpi probe. Add the calling of the
callback in devm_cxl_enumerate_ports(). The observed behavior is that
ports that are not connected to endpoint device(s) are no longer
enumerated. This should also remove any excessive noise of port probe
failing on those inactive ports.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v4:
- Reworked against new delayed dport allocation mechanism
---
 drivers/cxl/acpi.c      | 137 ++++++++++++++++++++++++----------------
 drivers/cxl/core/port.c |  67 ++++++++++++++++++++
 drivers/cxl/cxl.h       |   2 +
 3 files changed, 151 insertions(+), 55 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index a1a99ec3f12c..badaa99ab33a 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -296,8 +296,80 @@ static int cxl_acpi_qos_class(struct cxl_root *cxl_root,
 	return cxl_acpi_evaluate_qtg_dsm(handle, coord, entries, qos_class);
 }
 
+/* Note, @dev is used by mock_acpi_table_parse_cedt() */
+struct cxl_chbs_context {
+	struct device *dev;
+	unsigned long long uid;
+	resource_size_t base;
+	u32 cxl_version;
+	int nr_versions;
+	u32 saved_version;
+};
+
+static int cxl_get_chbs(struct device *dev, struct acpi_device *hb,
+			struct cxl_chbs_context *ctx);
+
+/*
+ * A host bridge is a dport to a CFMWS decoder and it is a uport to the
+ * dport (PCIe Root Ports) in the host bridge.
+ */
+static int cxl_acpi_setup_hostbridge_uport(struct cxl_root *cxl_root,
+					   struct device *bridge_dev)
+{
+	struct cxl_port *root_port = &cxl_root->port;
+	struct device *host = root_port->dev.parent;
+	struct acpi_device *hb = ACPI_COMPANION(bridge_dev);
+	resource_size_t component_reg_phys;
+	struct acpi_pci_root *pci_root;
+	struct cxl_chbs_context ctx;
+	struct cxl_dport *dport;
+	struct cxl_port *port;
+	int rc;
+
+	device_lock_assert(&cxl_root->port.dev);
+	pci_root = acpi_pci_find_root(hb->handle);
+	dport = cxl_find_dport_by_dev(root_port, bridge_dev);
+	if (!dport) {
+		dev_dbg(host, "Host bridge expected and not found\n");
+		return -ENODEV;
+	}
+
+	if (dport->rch) {
+		dev_info(bridge_dev, "host supports CXL (restricted)\n");
+		return 0;
+	}
+
+	rc = cxl_get_chbs(&hb->dev, hb, &ctx);
+	if (rc)
+		return rc;
+
+	if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) {
+		dev_warn(bridge_dev,
+			 "CXL CHBS version mismatch, skip port registration\n");
+		return 0;
+	}
+
+	component_reg_phys = ctx.base;
+	if (component_reg_phys != CXL_RESOURCE_NONE)
+		dev_dbg(&hb->dev, "CHBRC found for UID %lld: %pa\n",
+			ctx.uid, &component_reg_phys);
+
+	rc = devm_cxl_register_pci_bus(host, bridge_dev, pci_root->bus);
+	if (rc && rc != -EBUSY)
+		return rc;
+
+	port = devm_cxl_add_port(host, bridge_dev, component_reg_phys, dport);
+	if (IS_ERR(port))
+		return PTR_ERR(port);
+
+	dev_info(bridge_dev, "host supports CXL\n");
+
+	return 0;
+}
+
 static const struct cxl_root_ops acpi_root_ops = {
 	.qos_class = cxl_acpi_qos_class,
+	.setup_hostbridge_uport = cxl_acpi_setup_hostbridge_uport,
 };
 
 static void del_cxl_resource(struct resource *res)
@@ -466,16 +538,6 @@ __mock struct acpi_device *to_cxl_host_bridge(struct device *host,
 	return NULL;
 }
 
-/* Note, @dev is used by mock_acpi_table_parse_cedt() */
-struct cxl_chbs_context {
-	struct device *dev;
-	unsigned long long uid;
-	resource_size_t base;
-	u32 cxl_version;
-	int nr_versions;
-	u32 saved_version;
-};
-
 static int cxl_get_chbs_iter(union acpi_subtable_headers *header, void *arg,
 			     const unsigned long end)
 {
@@ -598,7 +660,7 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 	/*
 	 * In RCH mode, bind the component regs base to the dport. In
 	 * VH mode it will be bound to the CXL host bridge's port
-	 * object later in add_host_bridge_uport().
+	 * object later in cxl_acpi_setup_hostbridge_uport().
 	 */
 	if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) {
 		dev_dbg(match, "RCRB found for UID %lld: %pa\n", ctx.uid,
@@ -620,22 +682,14 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 	return 0;
 }
 
-/*
- * A host bridge is a dport to a CFMWS decode and it is a uport to the
- * dport (PCIe Root Ports) in the host bridge.
- */
-static int add_host_bridge_uport(struct device *match, void *arg)
+static int set_cxl_root_to_hostbridge(struct device *match, void *arg)
 {
 	struct cxl_port *root_port = arg;
 	struct device *host = root_port->dev.parent;
 	struct acpi_device *hb = to_cxl_host_bridge(host, match);
-	struct acpi_pci_root *pci_root;
 	struct cxl_dport *dport;
-	struct cxl_port *port;
+	struct acpi_pci_root *pci_root;
 	struct device *bridge;
-	struct cxl_chbs_context ctx;
-	resource_size_t component_reg_phys;
-	int rc;
 
 	if (!hb)
 		return 0;
@@ -648,37 +702,12 @@ static int add_host_bridge_uport(struct device *match, void *arg)
 		return 0;
 	}
 
-	if (dport->rch) {
-		dev_info(bridge, "host supports CXL (restricted)\n");
+	if (dport->rch)
 		return 0;
-	}
 
-	rc = cxl_get_chbs(match, hb, &ctx);
-	if (rc)
-		return rc;
-
-	if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) {
-		dev_warn(bridge,
-			 "CXL CHBS version mismatch, skip port registration\n");
-		return 0;
-	}
-
-	component_reg_phys = ctx.base;
-	if (component_reg_phys != CXL_RESOURCE_NONE)
-		dev_dbg(match, "CHBCR found for UID %lld: %pa\n",
-			ctx.uid, &component_reg_phys);
-
-	rc = devm_cxl_register_pci_bus(host, bridge, pci_root->bus);
-	if (rc)
-		return rc;
-
-	port = devm_cxl_add_port(host, bridge, component_reg_phys, dport);
-	if (IS_ERR(port))
-		return PTR_ERR(port);
-
-	dev_info(bridge, "host supports CXL\n");
-
-	return 0;
+	bridge = pci_root->bus->bridge;
+	return devm_cxl_register_hb_uport_root_port(host, bridge,
+						    to_cxl_root(root_port));
 }
 
 static int add_root_nvdimm_bridge(struct device *match, void *data)
@@ -852,6 +881,7 @@ static int cxl_acpi_probe(struct platform_device *pdev)
 		return PTR_ERR(cxl_root);
 	root_port = &cxl_root->port;
 
+	 /* Root level scanned with host-bridge as dports */
 	rc = bus_for_each_dev(adev->dev.bus, NULL, root_port,
 			      add_host_bridge_dport);
 	if (rc < 0)
@@ -880,12 +910,9 @@ static int cxl_acpi_probe(struct platform_device *pdev)
 	 */
 	device_for_each_child(&root_port->dev, cxl_res, pair_cxl_resource);
 
-	/*
-	 * Root level scanned with host-bridge as dports, now scan host-bridges
-	 * for their role as CXL uports to their CXL-capable PCIe Root Ports.
-	 */
+	/* Scan host-bridges and point the cxl root port struct to it */
 	rc = bus_for_each_dev(adev->dev.bus, NULL, root_port,
-			      add_host_bridge_uport);
+			      set_cxl_root_to_hostbridge);
 	if (rc < 0)
 		return rc;
 
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 20d2f834bf94..b1d19c609dde 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1813,6 +1813,69 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 
 #define CXL_ITER_LEVEL_SWITCH	1
 
+static int get_hostbridge_port_devices(struct cxl_memdev *cxlmd,
+				       struct device **hb_uport_dev,
+				       struct device **hb_dport_dev)
+{
+	struct device *dev = &cxlmd->dev;
+	struct device *iter;
+
+	for (iter = dev; iter; iter = grandparent(iter)) {
+		struct device *dport_dev = grandparent(iter);
+		struct device *uport_dev = dport_dev->parent;
+
+		if (is_cxl_hierarchy_head(uport_dev->parent)) {
+			*hb_uport_dev = uport_dev;
+			*hb_dport_dev = dport_dev;
+			return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+
+static int cxl_hostbridge_port_setup(struct cxl_memdev *cxlmd)
+{
+	struct device *hb_uport_dev, *hb_dport_dev;
+	struct cxl_dport *dport;
+	int rc;
+
+	rc = get_hostbridge_port_devices(cxlmd, &hb_uport_dev, &hb_dport_dev);
+	if (rc)
+		return -ENODEV;
+
+	struct cxl_root *cxl_root __free(put_cxl_root) =
+		cxl_hb_uport_dev_to_root(hb_uport_dev);
+	if (!cxl_root)
+		return -ENODEV;
+
+	guard(device)(&cxl_root->port.dev);
+	struct cxl_port *port __free(put_cxl_port) =
+		find_cxl_port(hb_dport_dev, &dport);
+	if (!port)
+		port = find_cxl_port_by_uport(hb_uport_dev);
+
+	/* Port already established, add the associated dport if needed. */
+	if (port) {
+		if (dport)
+			return 0;
+
+		guard(device)(&port->dev);
+		return devm_cxl_port_add_dport(port, hb_dport_dev, &dport);
+	}
+
+	/* No port found, setup a port via the root port ops */
+	if (!cxl_root->ops || !cxl_root->ops->setup_hostbridge_uport)
+		return -EOPNOTSUPP;
+
+	rc = cxl_root->ops->setup_hostbridge_uport(cxl_root, hb_uport_dev);
+	if (rc)
+		return rc;
+
+	/* Add the dport that goes with the newly created port */
+	return devm_cxl_add_dport_by_uport(hb_uport_dev, hb_dport_dev, &dport);
+}
+
 int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 {
 	struct device *dev = &cxlmd->dev;
@@ -1826,6 +1889,10 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 	if (cxlmd->cxlds->rcd)
 		return 0;
 
+	rc = cxl_hostbridge_port_setup(cxlmd);
+	if (rc)
+		return rc;
+
 	rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd);
 	if (rc)
 		return rc;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 3973bf11043f..e7a3727dd86d 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -642,6 +642,8 @@ struct cxl_root_ops {
 	int (*qos_class)(struct cxl_root *cxl_root,
 			 struct access_coordinate *coord, int entries,
 			 int *qos_class);
+	int (*setup_hostbridge_uport)(struct cxl_root *cxl_root,
+				      struct device *bridge_dev);
 };
 
 static inline struct cxl_dport *
-- 
2.49.0

Re: [PATCH v4 9/9] cxl: Move enumeration of hostbridge ports to the memdev probe path

[Jonathan Cameron]

On Tue, 24 Jun 2025 14:39:16 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Current enuemration scheme in cxl_acpi module creates the ports under the
> root port by enumerating the hostbridges after the dports under the root
> port is created. However error messages "cxl portN: Couldn't locate the
> CXL.cache and CXL.mem capability array header" is observed when certain
> platform has PCIe hotplug option turned on in BIOS. If the cxl_acpi module
> probe is running before the CXL link between the endpoint device and the
> RP is established, then the platform may not have exposed DVSEC ID 3 and/or
> DVSEC ID 7 blocks which will trigger the error message. This behavior
> is defined by the spec and not a hardware quirk.
> 
> Setup an association in cxl_port to tie the host bridge device to the
> associated cxl_root. The cxl_root provides a callback that's setup
> by the cxl_acpi probe function in order to create a port per host bridge
> that was previously done during cxl_acpi probe. Add the calling of the
> callback in devm_cxl_enumerate_ports(). The observed behavior is that
> ports that are not connected to endpoint device(s) are no longer
> enumerated. This should also remove any excessive noise of port probe
> failing on those inactive ports.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v4:
> - Reworked against new delayed dport allocation mechanism

One comment inline.

This whole things is complex enough I'm not feeling particularly confident
in my reviewing - so definitely needs a lot more eyes.  With that in mind.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 20d2f834bf94..b1d19c609dde 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c

> +
> +static int cxl_hostbridge_port_setup(struct cxl_memdev *cxlmd)
> +{
> +	struct device *hb_uport_dev, *hb_dport_dev;
> +	struct cxl_dport *dport;
> +	int rc;
> +
> +	rc = get_hostbridge_port_devices(cxlmd, &hb_uport_dev, &hb_dport_dev);
> +	if (rc)
> +		return -ENODEV;
> +
> +	struct cxl_root *cxl_root __free(put_cxl_root) =
> +		cxl_hb_uport_dev_to_root(hb_uport_dev);
> +	if (!cxl_root)
> +		return -ENODEV;
> +
> +	guard(device)(&cxl_root->port.dev);
> +	struct cxl_port *port __free(put_cxl_port) =
> +		find_cxl_port(hb_dport_dev, &dport);

I'm not certain that there isn't a path to dport being uninitialized
if !port.  Maybe we always get passed the early checks in match_port_by_dport()
at least once.  Event if it is safe, I suspect we might get false positive
reports as a compiler or static analysis tool is going to struggle to figure
that out.


> +	if (!port)
> +		port = find_cxl_port_by_uport(hb_uport_dev);
> +
> +	/* Port already established, add the associated dport if needed. */
> +	if (port) {
> +		if (dport)
> +			return 0;
> +
> +		guard(device)(&port->dev);
> +		return devm_cxl_port_add_dport(port, hb_dport_dev, &dport);
> +	}
> +
> +	/* No port found, setup a port via the root port ops */
> +	if (!cxl_root->ops || !cxl_root->ops->setup_hostbridge_uport)
> +		return -EOPNOTSUPP;
> +
> +	rc = cxl_root->ops->setup_hostbridge_uport(cxl_root, hb_uport_dev);
> +	if (rc)
> +		return rc;
> +
> +	/* Add the dport that goes with the newly created port */
> +	return devm_cxl_add_dport_by_uport(hb_uport_dev, hb_dport_dev, &dport);
> +}
> +
>  int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>  {
>  	struct device *dev = &cxlmd->dev;
> @@ -1826,6 +1889,10 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>  	if (cxlmd->cxlds->rcd)
>  		return 0;
>  
> +	rc = cxl_hostbridge_port_setup(cxlmd);
> +	if (rc)
> +		return rc;
> +
>  	rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd);
>  	if (rc)
>  		return rc;

Re: [PATCH v4 9/9] cxl: Move enumeration of hostbridge ports to the memdev probe path

[Dave Jiang]

On 7/1/25 4:32 AM, Jonathan Cameron wrote:
> On Tue, 24 Jun 2025 14:39:16 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Current enuemration scheme in cxl_acpi module creates the ports under the
>> root port by enumerating the hostbridges after the dports under the root
>> port is created. However error messages "cxl portN: Couldn't locate the
>> CXL.cache and CXL.mem capability array header" is observed when certain
>> platform has PCIe hotplug option turned on in BIOS. If the cxl_acpi module
>> probe is running before the CXL link between the endpoint device and the
>> RP is established, then the platform may not have exposed DVSEC ID 3 and/or
>> DVSEC ID 7 blocks which will trigger the error message. This behavior
>> is defined by the spec and not a hardware quirk.
>>
>> Setup an association in cxl_port to tie the host bridge device to the
>> associated cxl_root. The cxl_root provides a callback that's setup
>> by the cxl_acpi probe function in order to create a port per host bridge
>> that was previously done during cxl_acpi probe. Add the calling of the
>> callback in devm_cxl_enumerate_ports(). The observed behavior is that
>> ports that are not connected to endpoint device(s) are no longer
>> enumerated. This should also remove any excessive noise of port probe
>> failing on those inactive ports.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> v4:
>> - Reworked against new delayed dport allocation mechanism
> 
> One comment inline.
> 
> This whole things is complex enough I'm not feeling particularly confident
> in my reviewing - so definitely needs a lot more eyes.  With that in mind.
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> 
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 20d2f834bf94..b1d19c609dde 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
> 
>> +
>> +static int cxl_hostbridge_port_setup(struct cxl_memdev *cxlmd)
>> +{
>> +	struct device *hb_uport_dev, *hb_dport_dev;
>> +	struct cxl_dport *dport;
>> +	int rc;
>> +
>> +	rc = get_hostbridge_port_devices(cxlmd, &hb_uport_dev, &hb_dport_dev);
>> +	if (rc)
>> +		return -ENODEV;
>> +
>> +	struct cxl_root *cxl_root __free(put_cxl_root) =
>> +		cxl_hb_uport_dev_to_root(hb_uport_dev);
>> +	if (!cxl_root)
>> +		return -ENODEV;
>> +
>> +	guard(device)(&cxl_root->port.dev);
>> +	struct cxl_port *port __free(put_cxl_port) =
>> +		find_cxl_port(hb_dport_dev, &dport);
> 
> I'm not certain that there isn't a path to dport being uninitialized
> if !port.  Maybe we always get passed the early checks in match_port_by_dport()
> at least once.  Event if it is safe, I suspect we might get false positive
> reports as a compiler or static analysis tool is going to struggle to figure
> that out.

I'm not quite sure I understand what you are saying here.

DJ

> 
> 
>> +	if (!port)
>> +		port = find_cxl_port_by_uport(hb_uport_dev);
>> +
>> +	/* Port already established, add the associated dport if needed. */
>> +	if (port) {
>> +		if (dport)
>> +			return 0;
>> +
>> +		guard(device)(&port->dev);
>> +		return devm_cxl_port_add_dport(port, hb_dport_dev, &dport);
>> +	}
>> +
>> +	/* No port found, setup a port via the root port ops */
>> +	if (!cxl_root->ops || !cxl_root->ops->setup_hostbridge_uport)
>> +		return -EOPNOTSUPP;
>> +
>> +	rc = cxl_root->ops->setup_hostbridge_uport(cxl_root, hb_uport_dev);
>> +	if (rc)
>> +		return rc;
>> +
>> +	/* Add the dport that goes with the newly created port */
>> +	return devm_cxl_add_dport_by_uport(hb_uport_dev, hb_dport_dev, &dport);
>> +}
>> +
>>  int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>>  {
>>  	struct device *dev = &cxlmd->dev;
>> @@ -1826,6 +1889,10 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>>  	if (cxlmd->cxlds->rcd)
>>  		return 0;
>>  
>> +	rc = cxl_hostbridge_port_setup(cxlmd);
>> +	if (rc)
>> +		return rc;
>> +
>>  	rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd);
>>  	if (rc)
>>  		return rc;
> 
> 

Re: [PATCH v4 9/9] cxl: Move enumeration of hostbridge ports to the memdev probe path

[Jonathan Cameron]

On Thu, 3 Jul 2025 16:22:38 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 7/1/25 4:32 AM, Jonathan Cameron wrote:
> > On Tue, 24 Jun 2025 14:39:16 -0700
> > Dave Jiang <dave.jiang@intel.com> wrote:
> >   
> >> Current enuemration scheme in cxl_acpi module creates the ports under the
> >> root port by enumerating the hostbridges after the dports under the root
> >> port is created. However error messages "cxl portN: Couldn't locate the
> >> CXL.cache and CXL.mem capability array header" is observed when certain
> >> platform has PCIe hotplug option turned on in BIOS. If the cxl_acpi module
> >> probe is running before the CXL link between the endpoint device and the
> >> RP is established, then the platform may not have exposed DVSEC ID 3 and/or
> >> DVSEC ID 7 blocks which will trigger the error message. This behavior
> >> is defined by the spec and not a hardware quirk.
> >>
> >> Setup an association in cxl_port to tie the host bridge device to the
> >> associated cxl_root. The cxl_root provides a callback that's setup
> >> by the cxl_acpi probe function in order to create a port per host bridge
> >> that was previously done during cxl_acpi probe. Add the calling of the
> >> callback in devm_cxl_enumerate_ports(). The observed behavior is that
> >> ports that are not connected to endpoint device(s) are no longer
> >> enumerated. This should also remove any excessive noise of port probe
> >> failing on those inactive ports.
> >>
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >> ---
> >> v4:
> >> - Reworked against new delayed dport allocation mechanism  
> > 
> > One comment inline.
> > 
> > This whole things is complex enough I'm not feeling particularly confident
> > in my reviewing - so definitely needs a lot more eyes.  With that in mind.
> > 
> > Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> >   
> >> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> >> index 20d2f834bf94..b1d19c609dde 100644
> >> --- a/drivers/cxl/core/port.c
> >> +++ b/drivers/cxl/core/port.c  
> >   
> >> +
> >> +static int cxl_hostbridge_port_setup(struct cxl_memdev *cxlmd)
> >> +{
> >> +	struct device *hb_uport_dev, *hb_dport_dev;
> >> +	struct cxl_dport *dport;
> >> +	int rc;
> >> +
> >> +	rc = get_hostbridge_port_devices(cxlmd, &hb_uport_dev, &hb_dport_dev);
> >> +	if (rc)
> >> +		return -ENODEV;
> >> +
> >> +	struct cxl_root *cxl_root __free(put_cxl_root) =
> >> +		cxl_hb_uport_dev_to_root(hb_uport_dev);
> >> +	if (!cxl_root)
> >> +		return -ENODEV;
> >> +
> >> +	guard(device)(&cxl_root->port.dev);
> >> +	struct cxl_port *port __free(put_cxl_port) =
> >> +		find_cxl_port(hb_dport_dev, &dport);  
> > 
> > I'm not certain that there isn't a path to dport being uninitialized
> > if !port.  Maybe we always get passed the early checks in match_port_by_dport()
> > at least once.  Event if it is safe, I suspect we might get false positive
> > reports as a compiler or static analysis tool is going to struggle to figure
> > that out.  
> 
> I'm not quite sure I understand what you are saying here.
Need more caffeine and a spell checker that day it seems.

My question was whether we can have (or a static analysis tool thinks we can
have)
1. port == NULL here, dport not initialized.
> 
> DJ
> 
> > 
> >   
> >> +	if (!port)
> >> +		port = find_cxl_port_by_uport(hb_uport_dev);

2. Then this successfully sets port.

> >> +
> >> +	/* Port already established, add the associated dport if needed. */
> >> +	if (port) {
> >> +		if (dport)

3. At which point this is using dport uninitialized.

> >> +			return 0;
> >> +
> >> +		guard(device)(&port->dev);
> >> +		return devm_cxl_port_add_dport(port, hb_dport_dev, &dport);
> >> +	}
> >> +
> >> +	/* No port found, setup a port via the root port ops */
> >> +	if (!cxl_root->ops || !cxl_root->ops->setup_hostbridge_uport)
> >> +		return -EOPNOTSUPP;
> >> +
> >> +	rc = cxl_root->ops->setup_hostbridge_uport(cxl_root, hb_uport_dev);
> >> +	if (rc)
> >> +		return rc;
> >> +
> >> +	/* Add the dport that goes with the newly created port */
> >> +	return devm_cxl_add_dport_by_uport(hb_uport_dev, hb_dport_dev, &dport);
> >> +}
> >> +
> >>  int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
> >>  {
> >>  	struct device *dev = &cxlmd->dev;
> >> @@ -1826,6 +1889,10 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
> >>  	if (cxlmd->cxlds->rcd)
> >>  		return 0;
> >>  
> >> +	rc = cxl_hostbridge_port_setup(cxlmd);
> >> +	if (rc)
> >> +		return rc;
> >> +
> >>  	rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd);
> >>  	if (rc)
> >>  		return rc;  
> > 
> >   
> 
> 

Re: [PATCH v4 9/9] cxl: Move enumeration of hostbridge ports to the memdev probe path

[Dave Jiang]

On 7/4/25 2:39 AM, Jonathan Cameron wrote:
> On Thu, 3 Jul 2025 16:22:38 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> On 7/1/25 4:32 AM, Jonathan Cameron wrote:
>>> On Tue, 24 Jun 2025 14:39:16 -0700
>>> Dave Jiang <dave.jiang@intel.com> wrote:
>>>   
>>>> Current enuemration scheme in cxl_acpi module creates the ports under the
>>>> root port by enumerating the hostbridges after the dports under the root
>>>> port is created. However error messages "cxl portN: Couldn't locate the
>>>> CXL.cache and CXL.mem capability array header" is observed when certain
>>>> platform has PCIe hotplug option turned on in BIOS. If the cxl_acpi module
>>>> probe is running before the CXL link between the endpoint device and the
>>>> RP is established, then the platform may not have exposed DVSEC ID 3 and/or
>>>> DVSEC ID 7 blocks which will trigger the error message. This behavior
>>>> is defined by the spec and not a hardware quirk.
>>>>
>>>> Setup an association in cxl_port to tie the host bridge device to the
>>>> associated cxl_root. The cxl_root provides a callback that's setup
>>>> by the cxl_acpi probe function in order to create a port per host bridge
>>>> that was previously done during cxl_acpi probe. Add the calling of the
>>>> callback in devm_cxl_enumerate_ports(). The observed behavior is that
>>>> ports that are not connected to endpoint device(s) are no longer
>>>> enumerated. This should also remove any excessive noise of port probe
>>>> failing on those inactive ports.
>>>>
>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>> ---
>>>> v4:
>>>> - Reworked against new delayed dport allocation mechanism  
>>>
>>> One comment inline.
>>>
>>> This whole things is complex enough I'm not feeling particularly confident
>>> in my reviewing - so definitely needs a lot more eyes.  With that in mind.
>>>
>>> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>>>   
>>>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>>>> index 20d2f834bf94..b1d19c609dde 100644
>>>> --- a/drivers/cxl/core/port.c
>>>> +++ b/drivers/cxl/core/port.c  
>>>   
>>>> +
>>>> +static int cxl_hostbridge_port_setup(struct cxl_memdev *cxlmd)
>>>> +{
>>>> +	struct device *hb_uport_dev, *hb_dport_dev;
>>>> +	struct cxl_dport *dport;
>>>> +	int rc;
>>>> +
>>>> +	rc = get_hostbridge_port_devices(cxlmd, &hb_uport_dev, &hb_dport_dev);
>>>> +	if (rc)
>>>> +		return -ENODEV;
>>>> +
>>>> +	struct cxl_root *cxl_root __free(put_cxl_root) =
>>>> +		cxl_hb_uport_dev_to_root(hb_uport_dev);
>>>> +	if (!cxl_root)
>>>> +		return -ENODEV;
>>>> +
>>>> +	guard(device)(&cxl_root->port.dev);
>>>> +	struct cxl_port *port __free(put_cxl_port) =
>>>> +		find_cxl_port(hb_dport_dev, &dport);  
>>>
>>> I'm not certain that there isn't a path to dport being uninitialized
>>> if !port.  Maybe we always get passed the early checks in match_port_by_dport()
>>> at least once.  Event if it is safe, I suspect we might get false positive
>>> reports as a compiler or static analysis tool is going to struggle to figure
>>> that out.  
>>
>> I'm not quite sure I understand what you are saying here.
> Need more caffeine and a spell checker that day it seems.
> 
> My question was whether we can have (or a static analysis tool thinks we can
> have)
> 1. port == NULL here, dport not initialized.
>>
>> DJ
>>
>>>
>>>   
>>>> +	if (!port)
>>>> +		port = find_cxl_port_by_uport(hb_uport_dev);
> 
> 2. Then this successfully sets port.
> 
>>>> +
>>>> +	/* Port already established, add the associated dport if needed. */
>>>> +	if (port) {
>>>> +		if (dport)
> 
> 3. At which point this is using dport uninitialized.

find_cxl_port() should assign the dport to NULL if we don't find a port. Although I can set dport to NULL at declaration just to be sure.


DJ

> 
>>>> +			return 0;
>>>> +
>>>> +		guard(device)(&port->dev);
>>>> +		return devm_cxl_port_add_dport(port, hb_dport_dev, &dport);
>>>> +	}
>>>> +
>>>> +	/* No port found, setup a port via the root port ops */
>>>> +	if (!cxl_root->ops || !cxl_root->ops->setup_hostbridge_uport)
>>>> +		return -EOPNOTSUPP;
>>>> +
>>>> +	rc = cxl_root->ops->setup_hostbridge_uport(cxl_root, hb_uport_dev);
>>>> +	if (rc)
>>>> +		return rc;
>>>> +
>>>> +	/* Add the dport that goes with the newly created port */
>>>> +	return devm_cxl_add_dport_by_uport(hb_uport_dev, hb_dport_dev, &dport);
>>>> +}
>>>> +
>>>>  int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>>>>  {
>>>>  	struct device *dev = &cxlmd->dev;
>>>> @@ -1826,6 +1889,10 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>>>>  	if (cxlmd->cxlds->rcd)
>>>>  		return 0;
>>>>  
>>>> +	rc = cxl_hostbridge_port_setup(cxlmd);
>>>> +	if (rc)
>>>> +		return rc;
>>>> +
>>>>  	rc = devm_add_action_or_reset(&cxlmd->dev, cxl_detach_ep, cxlmd);
>>>>  	if (rc)
>>>>  		return rc;  
>>>
>>>   
>>
>>
> 
>