[PATCH v7 00/10] cxl: Delay HB port and switch dport probing until endpoint dev probe

[PATCH v7 00/10] cxl: Delay HB port and switch dport probing until endpoint dev probe

[Dave Jiang]

v7:
- Remove -EEXIST to simplify error flow. (Ming)
- Set dport to NULL during declare. (Jonathan)

v6:
- Return -EEXIST when a dport already exists. (Jonathan)
- Fix checking wrong port for NULL. (Ming)
- Check host_bridge and call devm_cxl_add_dport_by_uport() directly vs add_port_attach_ep(). (Ming)
- Set dport to NULL during declaration. (Jonathan)

v5:
- Return dport instead of errno with dport pointer as output param. (Jonathan)
- Consolidate common code in cxl_test. (Jonathan)
- Rename cxl_port_get_total_dports() to cxl_port_update_total_dports(). (Jonathan)

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 (10):
  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 cxl_port_update_total_ports()
  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
  cxl: Remove devm_cxl_port_enumerate_dports() that is no longer used

 drivers/cxl/acpi.c                   | 137 ++++++-----
 drivers/cxl/core/cdat.c              |  23 +-
 drivers/cxl/core/pci.c               | 146 ++++++-----
 drivers/cxl/core/port.c              | 351 +++++++++++++++++++++++++--
 drivers/cxl/core/region.c            |   7 +-
 drivers/cxl/cxl.h                    |  32 ++-
 drivers/cxl/cxlpci.h                 |   1 -
 drivers/cxl/port.c                   |   8 +-
 tools/testing/cxl/Kbuild             |   3 +-
 tools/testing/cxl/cxl_core_exports.c |  12 +
 tools/testing/cxl/exports.h          |  10 +
 tools/testing/cxl/test/cxl.c         |  67 ++++-
 tools/testing/cxl/test/mock.c        |  31 ++-
 tools/testing/cxl/test/mock.h        |   4 +-
 14 files changed, 651 insertions(+), 181 deletions(-)
 create mode 100644 tools/testing/cxl/exports.h


base-commit: 347e9f5043c89695b01e66b3ed111755afcf1911
-- 
2.50.0

[PATCH v7 01/10] 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.50.0

[PATCH v7 02/10] 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.50.0

[PATCH v7 03/10] 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.

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

[PATCH v7 04/10] 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_attach_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/
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v7:
- Return dport instead of -EEXIST (Ming)
- Remove extra goto retry (Ming)
---
 drivers/cxl/core/core.h |   2 +
 drivers/cxl/core/pci.c  |  88 +++++++++++++++++++
 drivers/cxl/core/port.c | 185 ++++++++++++++++++++++++++++++++++++----
 drivers/cxl/cxl.h       |   5 ++
 drivers/cxl/port.c      |   8 +-
 5 files changed, 267 insertions(+), 21 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..336451b9144a 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_update_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_update_total_dports, "CXL");
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 9691da831224..c1de6872e57f 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1551,6 +1551,116 @@ 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 struct cxl_dport *devm_cxl_port_add_dport(struct cxl_port *port,
+						 struct device *dport_dev)
+{
+	struct cxl_dport *dport;
+	int rc;
+
+	device_lock_assert(&port->dev);
+
+	/* Port driver not attached yet, wait for cxl_acpi reprobe */
+	if (!port->dev.driver)
+		return ERR_PTR(-ENODEV);
+
+	dport = cxl_find_dport_by_dev(port, dport_dev);
+	if (dport)
+		return dport;
+
+	dport = devm_cxl_add_dport_by_dev(port, dport_dev);
+	if (IS_ERR(dport))
+		return dport;
+
+	rc = cxl_port_setup_with_dport(port, dport);
+	if (rc) {
+		reap_dport(port, dport);
+		return ERR_PTR(rc);
+	}
+
+	return dport;
+}
+
+static struct cxl_dport *devm_cxl_add_dport_by_uport(struct device *uport_dev,
+						     struct device *dport_dev)
+{
+	struct cxl_port *port __free(put_cxl_port) =
+		find_cxl_port_by_uport(uport_dev);
+
+	if (!port)
+		return ERR_PTR(-ENODEV);
+
+	guard(device)(&port->dev);
+	return devm_cxl_port_add_dport(port, dport_dev);
+}
+
 static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 			      struct device *uport_dev,
 			      struct device *dport_dev)
@@ -1584,6 +1694,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 +1704,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 +1713,21 @@ 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;
+			/*
+			 * The port holds a device reference via find_cxl_port_at()
+			 * if the port is valid. But if the port is newly created
+			 * via devm_cxl_add_port(), no reference is held. Therefore
+			 * the driver needs to get a device reference here.
+			 */
+			get_device(&port->dev);
 		}
+
+		guard(device)(&port->dev);
+		new_dport = devm_cxl_port_add_dport(port, dport_dev);
+		if (IS_ERR(new_dport))
+			return PTR_ERR(new_dport);
+
+		dport = new_dport;
 	}
 
 	dev_dbg(&cxlmd->dev, "add to new port %s:%s\n",
@@ -1620,11 +1744,14 @@ 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 *dgparent;
 	struct device *iter;
-	int rc;
+	int rc, i;
 
 	/*
 	 * Skip intermediate port enumeration in the RCH case, there
@@ -1643,7 +1770,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,16 +1813,39 @@ 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 (!pport)
+					goto retry;
+			}
+
 			return 0;
 		}
 
-		rc = add_port_attach_ep(cxlmd, uport_dev, dport_dev);
-		/* port missing, try to add parent */
-		if (rc == -EAGAIN)
-			continue;
-		/* failed to add ep or port */
-		if (rc)
-			return rc;
+		dgparent = grandparent(dport_dev);
+		/* Only go down this path if we are at the root port */
+		if (is_cxl_hierarchy_head(dgparent)) {
+			dport = devm_cxl_add_dport_by_uport(uport_dev,
+							    dport_dev);
+			/* Added a dport, restart enumeration */
+			if (IS_ERR(dport))
+				return PTR_ERR(dport);
+		} else {
+			rc = add_port_attach_ep(cxlmd, uport_dev, dport_dev);
+			/* port missing, try to add parent */
+			if (rc == -EAGAIN)
+				continue;
+			/* failed to add ep or port */
+			if (rc)
+				return rc;
+		}
+
 		/* port added, new descendants possible, start over */
 		goto retry;
 	}
@@ -1727,16 +1877,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..de7883747555 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_update_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..ee7dcd7c4f24 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_update_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.50.0

[PATCH v7 05/10] cxl/test: Add cxl_test support for cxl_port_update_total_ports()

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

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>
---
 tools/testing/cxl/Kbuild      |  1 +
 tools/testing/cxl/test/cxl.c  | 53 +++++++++++++++++++++++++++++++++--
 tools/testing/cxl/test/mock.c | 15 ++++++++++
 tools/testing/cxl/test/mock.h |  1 +
 4 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index 31a2d73c963f..b9fbaaca999e 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_update_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..6fd1eaca6cbe 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -920,10 +920,12 @@ static int mock_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 	return 0;
 }
 
-static int mock_cxl_port_enumerate_dports(struct cxl_port *port)
+static int get_port_array(struct cxl_port *port,
+			  struct platform_device ***port_array,
+			  int *port_array_size)
 {
 	struct platform_device **array;
-	int i, array_size;
+	int array_size;
 
 	if (port->depth == 1) {
 		if (is_multi_bridge(port->uport_dev)) {
@@ -957,6 +959,52 @@ static int mock_cxl_port_enumerate_dports(struct cxl_port *port)
 		return -ENXIO;
 	}
 
+	*port_array = array;
+	*port_array_size = array_size;
+
+	return 0;
+}
+
+static int mock_cxl_port_update_total_dports(struct cxl_port *port)
+{
+	struct platform_device **array;
+	int rc, i, array_size;
+	int dports = 0;
+
+	rc = get_port_array(port, &array, &array_size);
+	if (rc)
+		return rc;
+
+	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;
+	int rc, i, array_size;
+
+	rc = get_port_array(port, &array, &array_size);
+	if (rc)
+		return rc;
+
 	for (i = 0; i < array_size; i++) {
 		struct platform_device *pdev = array[i];
 		struct cxl_dport *dport;
@@ -1035,6 +1083,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_update_total_dports = mock_cxl_port_update_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..6272c0691275 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_update_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_update_total_dports(port);
+	else
+		rc = cxl_port_update_total_dports(port);
+	put_cxl_mock_ops(index);
+
+	return rc;
+}
+EXPORT_SYMBOL_NS_GPL(__wrap_cxl_port_update_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..bf2a3e5581ba 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_update_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.50.0

[PATCH v7 06/10] 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>
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/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         | 31 ++++++++++++++++++++++++++++
 tools/testing/cxl/test/mock.c        | 23 +++++++++++++++++++++
 tools/testing/cxl/test/mock.h        |  2 ++
 9 files changed, 102 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 336451b9144a..5594b0fcddb0 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 de7883747555..340073265657 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_update_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 b9fbaaca999e..d8c5f7df4866 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 6fd1eaca6cbe..3e9c877fcff2 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -1026,6 +1026,36 @@ 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 rc, i, array_size;
+
+	rc = get_port_array(port, &array, &array_size);
+	if (rc)
+		return ERR_PTR(rc);
+
+	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.
  */
@@ -1088,6 +1118,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 6272c0691275..3c7190b7d5a4 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 bf2a3e5581ba..0a440b9f544c 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.50.0

[PATCH v7 07/10] 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 c1de6872e57f..afeebb0ce4ab 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 340073265657..cc5e0858824b 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.50.0

[PATCH v7 08/10] 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.

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/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 afeebb0ce4ab..ef6e1c63f652 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 cc5e0858824b..f88a7ab8d94b 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.50.0

[PATCH v7 09/10] 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.

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>
---
v6:
- Deal with -EEXIST output for devm_cxl_port_add_dport()
- Set dport to NULL during declare. (Jonathan)
---
 drivers/cxl/acpi.c      | 137 ++++++++++++++++++++++++----------------
 drivers/cxl/core/port.c |  84 ++++++++++++++++++++++++
 drivers/cxl/cxl.h       |   2 +
 3 files changed, 168 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 ef6e1c63f652..183cb5190d5c 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1799,6 +1799,86 @@ 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 = NULL;
+	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);
+		dport = devm_cxl_port_add_dport(port, hb_dport_dev);
+		if (IS_ERR(dport)) {
+			dev_dbg(&cxlmd->dev,
+				"failed to add dport %s to port %s: %ld\n",
+				dev_name(hb_dport_dev), dev_name(&port->dev),
+				PTR_ERR(dport));
+			return PTR_ERR(dport);
+		}
+		return 0;
+	}
+
+	/* 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 */
+	dport = devm_cxl_add_dport_by_uport(hb_uport_dev, hb_dport_dev);
+	if (IS_ERR(dport)) {
+		dev_dbg(&cxlmd->dev,
+			"failed to add dport %s to port %s: %ld\n",
+			dev_name(hb_dport_dev), dev_name(&cxl_root->port.dev),
+			PTR_ERR(dport));
+		return PTR_ERR(dport);
+	}
+
+	return 0;
+}
+
 int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 {
 	struct device *dev = &cxlmd->dev;
@@ -1813,6 +1893,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 f88a7ab8d94b..f9972fb4990f 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.50.0

[PATCH v7 10/10] cxl: Remove devm_cxl_port_enumerate_dports() that is no longer used

[Dave Jiang]

Delete function devm_cxl_port_enumerate_dports() which is no longer being
used anywhere. Also remove cxl_test support.

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/pci.c        | 87 ++++-------------------------------
 drivers/cxl/cxlpci.h          |  1 -
 tools/testing/cxl/Kbuild      |  1 -
 tools/testing/cxl/test/cxl.c  | 31 -------------
 tools/testing/cxl/test/mock.c | 15 ------
 tools/testing/cxl/test/mock.h |  1 -
 6 files changed, 8 insertions(+), 128 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 5594b0fcddb0..717e25131db7 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -63,85 +63,6 @@ struct cxl_dport *__devm_cxl_add_dport_by_dev(struct cxl_port *port,
 }
 EXPORT_SYMBOL_NS_GPL(__devm_cxl_add_dport_by_dev, "CXL");
 
-struct cxl_walk_context {
-	struct pci_bus *bus;
-	struct cxl_port *port;
-	int type;
-	int error;
-	int count;
-};
-
-static int match_add_dports(struct pci_dev *pdev, void *data)
-{
-	struct cxl_walk_context *ctx = data;
-	struct cxl_port *port = ctx->port;
-	int type = pci_pcie_type(pdev);
-	struct cxl_register_map map;
-	struct cxl_dport *dport;
-	u32 lnkcap, port_num;
-	int rc;
-
-	if (pdev->bus != ctx->bus)
-		return 0;
-	if (!pci_is_pcie(pdev))
-		return 0;
-	if (type != ctx->type)
-		return 0;
-	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
-				  &lnkcap))
-		return 0;
-
-	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);
-	dport = devm_cxl_add_dport(port, &pdev->dev, port_num, map.resource);
-	if (IS_ERR(dport)) {
-		ctx->error = PTR_ERR(dport);
-		return PTR_ERR(dport);
-	}
-	ctx->count++;
-
-	return 0;
-}
-
-/**
- * devm_cxl_port_enumerate_dports - enumerate downstream ports of the upstream port
- * @port: cxl_port whose ->uport_dev is the upstream of dports to be enumerated
- *
- * Returns a positive number of dports enumerated or a negative error
- * code.
- */
-int devm_cxl_port_enumerate_dports(struct cxl_port *port)
-{
-	struct pci_bus *bus = cxl_port_to_pci_bus(port);
-	struct cxl_walk_context ctx;
-	int type;
-
-	if (!bus)
-		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) {
-		.port = port,
-		.bus = bus,
-		.type = type,
-	};
-	pci_walk_bus(bus, match_add_dports, &ctx);
-
-	if (ctx.count == 0)
-		return -ENODEV;
-	if (ctx.error)
-		return ctx.error;
-	return ctx.count;
-}
-EXPORT_SYMBOL_NS_GPL(devm_cxl_port_enumerate_dports, "CXL");
-
 static int cxl_dvsec_mem_range_valid(struct cxl_dev_state *cxlds, int id)
 {
 	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
@@ -1209,6 +1130,14 @@ int cxl_gpf_port_setup(struct cxl_dport *dport)
 	return 0;
 }
 
+struct cxl_walk_context {
+	struct pci_bus *bus;
+	struct cxl_port *port;
+	int type;
+	int error;
+	int count;
+};
+
 static int match_dport(struct pci_dev *pdev, void *data)
 {
 	struct cxl_walk_context *ctx = data;
diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
index 54e219b0049e..7827a183128d 100644
--- a/drivers/cxl/cxlpci.h
+++ b/drivers/cxl/cxlpci.h
@@ -127,7 +127,6 @@ static inline bool cxl_pci_flit_256(struct pci_dev *pdev)
 	return lnksta2 & PCI_EXP_LNKSTA2_FLIT;
 }
 
-int devm_cxl_port_enumerate_dports(struct cxl_port *port);
 struct cxl_dev_state;
 int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 			struct cxl_endpoint_dvsec_info *info);
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index d8c5f7df4866..4e3597d4ee9c 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -4,7 +4,6 @@ ldflags-y += --wrap=is_acpi_device_node
 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_update_total_dports
 ldflags-y += --wrap=devm_cxl_setup_hdm
 ldflags-y += --wrap=devm_cxl_add_passthrough_decoder
diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index 3e9c877fcff2..d52ae0be4941 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -996,36 +996,6 @@ static int mock_cxl_port_update_total_dports(struct cxl_port *port)
 	return 0;
 }
 
-static int mock_cxl_port_enumerate_dports(struct cxl_port *port)
-{
-	struct platform_device **array;
-	int rc, i, array_size;
-
-	rc = get_port_array(port, &array, &array_size);
-	if (rc)
-		return rc;
-
-	for (i = 0; i < array_size; i++) {
-		struct platform_device *pdev = array[i];
-		struct cxl_dport *dport;
-
-		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;
-		}
-
-		dport = devm_cxl_add_dport(port, &pdev->dev, pdev->id,
-					   CXL_RESOURCE_NONE);
-
-		if (IS_ERR(dport))
-			return PTR_ERR(dport);
-	}
-
-	return 0;
-}
-
 static struct cxl_dport *mock_cxl_add_dport_by_dev(struct cxl_port *port,
 						   struct device *dport_dev)
 {
@@ -1112,7 +1082,6 @@ static struct cxl_mock_ops cxl_mock_ops = {
 	.acpi_table_parse_cedt = mock_acpi_table_parse_cedt,
 	.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_update_total_dports = mock_cxl_port_update_total_dports,
 	.devm_cxl_setup_hdm = mock_cxl_setup_hdm,
 	.devm_cxl_add_passthrough_decoder = mock_cxl_add_passthrough_decoder,
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index 3c7190b7d5a4..8bb90d732332 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -188,21 +188,6 @@ int __wrap_devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 }
 EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_enumerate_decoders, "CXL");
 
-int __wrap_devm_cxl_port_enumerate_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->devm_cxl_port_enumerate_dports(port);
-	else
-		rc = devm_cxl_port_enumerate_dports(port);
-	put_cxl_mock_ops(index);
-
-	return rc;
-}
-EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_port_enumerate_dports, "CXL");
-
 int __wrap_cxl_port_update_total_dports(struct cxl_port *port)
 {
 	int rc, index;
diff --git a/tools/testing/cxl/test/mock.h b/tools/testing/cxl/test/mock.h
index 0a440b9f544c..4d262a38dd51 100644
--- a/tools/testing/cxl/test/mock.h
+++ b/tools/testing/cxl/test/mock.h
@@ -19,7 +19,6 @@ struct cxl_mock_ops {
 	bool (*is_mock_bus)(struct pci_bus *bus);
 	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_update_total_dports)(struct cxl_port *port);
 	struct cxl_hdm *(*devm_cxl_setup_hdm)(
 		struct cxl_port *port, struct cxl_endpoint_dvsec_info *info);
-- 
2.50.0

Re: [PATCH v7 04/10] cxl: Defer dport allocation for switch ports

[Li Ming]

On 7/15/2025 6:35 AM, Dave Jiang 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_attach_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/
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Li Ming <ming.li@zohomail.com>

Re: [PATCH v7 02/10] cxl: Add helper to detect top of CXL device topology

[Alison Schofield]

On Mon, Jul 14, 2025 at 03:35:19PM -0700, Dave Jiang wrote:
> 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: Alison Schofield <alison.schofield@intel.com>

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

Re: [PATCH v7 03/10] cxl: Add helper to reap dport

[Alison Schofield]

On Mon, Jul 14, 2025 at 03:35:20PM -0700, Dave Jiang 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.
> 

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

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

Re: [PATCH v7 04/10] cxl: Defer dport allocation for switch ports

[Alison Schofield]

On Mon, Jul 14, 2025 at 03:35:21PM -0700, Dave Jiang 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_attach_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.

Appreciate the explanation in commit log!

Reviewed-by: Alison Schofield <alison.schofield@intel.com>


> 
> Link: https://lore.kernel.org/linux-cxl/20250305100123.3077031-1-rrichter@amd.com/
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 

Re: [PATCH v7 05/10] cxl/test: Add cxl_test support for cxl_port_update_total_ports()

[Alison Schofield]

On Mon, Jul 14, 2025 at 03:35:22PM -0700, Dave Jiang 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.


Tested-by: Alison Schofield <alison.schofield@intel.com>

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

Re: [PATCH v7 06/10] cxl/test: Add mock version of devm_cxl_add_dport_by_dev()

[Alison Schofield]

On Mon, Jul 14, 2025 at 03:35:23PM -0700, Dave Jiang 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.

This is a mock masterpiece!
Tested-by: Alison Schofield <alison.schofield@intel.com>

> 
> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> 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>
> ---

Re: [PATCH v7 07/10] cxl: Change sslbis handler to only handle single dport

[Alison Schofield]

On Mon, Jul 14, 2025 at 03:35:24PM -0700, Dave Jiang wrote:
> 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: Alison Schofield <alison.schofield@intel.com>

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

Re: [PATCH v7 08/10] cxl: Create an xarray to tie a host bridge to the cxl_root

[Alison Schofield]

On Mon, Jul 14, 2025 at 03:35:25PM -0700, Dave Jiang 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.
> 

Reviewed-by: Alison Schofield <alison.schofield@intel.com>


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

Re: [PATCH v7 09/10] cxl: Move enumeration of hostbridge ports to the memdev probe path

[Alison Schofield]

On Mon, Jul 14, 2025 at 03:35:26PM -0700, Dave Jiang 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.

Another needed and much appreciated commit log!
Reviewed-by: Alison Schofield <alison.schofield@intel.com>

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

Re: [PATCH v7 10/10] cxl: Remove devm_cxl_port_enumerate_dports() that is no longer used

[Alison Schofield]

On Mon, Jul 14, 2025 at 03:35:27PM -0700, Dave Jiang wrote:
> Delete function devm_cxl_port_enumerate_dports() which is no longer being
> used anywhere. Also remove cxl_test support.


Reviewed-by: Alison Schofield <alison.schofield@intel.com>


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

Re: [PATCH v7 01/10] cxl/region: Add decoder check to check_commit_order()

[Robert Richter]

On 14.07.25 15:35:18, Dave Jiang wrote:
> 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>

Reviewed-by: Robert Richter <rrichter@amd.com>

> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/region.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Re: [PATCH v7 01/10] cxl/region: Add decoder check to check_commit_order()

[dan.j.williams]

Dave Jiang wrote:
> 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

s/pure luck/cxl_port_attach_region() only handling switch port decoder
assignment/

>. Issue discovered while doing deferred dport probing when
> child ports are now in the midst of decoders due to ordering change

s/midst/mix/?

So I went looking to see if this change actually makes a difference in
the end, or if this was just the result of debugging an interim state of
the code that ended up passing non-switch decoders through this path.

The disinction matters for understanding code-flow and history here.
Like something is wrong if this expectation is violated in some future
refactoring and may even be worth putting a dev_WARN_ONCE() in this
path.

In fact I put a dev_WARN_ONCE in this path with all the patches applied
and it never triggers with a cxl test suite run, so I think it is safe
to do something like the below.

...but before going there, can you confirm the expectation that the
safety is not strictly needed?

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 6e5e1460068d..63c3dc978797 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -787,7 +787,10 @@ 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 = to_cxl_switch_decoder(dev);
+
+	if (!cxld)
+		return 0;
 
 	/*
 	 * if port->commit_end is not the only free decoder, then out of

Re: [PATCH v7 02/10] cxl: Add helper to detect top of CXL device topology

[dan.j.williams]

Dave Jiang wrote:
> 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;

I quibble with a new term "cxl_hierarchy_head" and the fact that this is
a PCI topology property not a "CXL" property per se. ...but not a large
enough quibble to ask for a change.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH v7 03/10] cxl: Add helper to reap dport

[dan.j.williams]

Dave Jiang 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.
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Reviewed-by: Li Ming <ming.li@zohomail.com>

Simple enough:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH v7 01/10] cxl/region: Add decoder check to check_commit_order()

[dan.j.williams]

dan.j.williams@ wrote:
> Dave Jiang wrote:
> > 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
> 
> s/pure luck/cxl_port_attach_region() only handling switch port decoder
> assignment/
> 
> >. Issue discovered while doing deferred dport probing when
> > child ports are now in the midst of decoders due to ordering change
> 
> s/midst/mix/?
> 
> So I went looking to see if this change actually makes a difference in
> the end, or if this was just the result of debugging an interim state of
> the code that ended up passing non-switch decoders through this path.
> 
> The disinction matters for understanding code-flow and history here.
> Like something is wrong if this expectation is violated in some future
> refactoring and may even be worth putting a dev_WARN_ONCE() in this
> path.
> 
> In fact I put a dev_WARN_ONCE in this path with all the patches applied
> and it never triggers with a cxl test suite run, so I think it is safe
> to do something like the below.
> 
> ...but before going there, can you confirm the expectation that the
> safety is not strictly needed?

Ok, so I finally remembered the previous discussion here and switch
ports have both endpoint-port child devices and decoder child devices.

However, endpoint-ports are always added *after* decoders. That is
enforced by the fact that decoders are added in cxl_switch_port_probe()
and endpoint-ports serialize against that before adding new ports.

The device_for_each_child_reverse_from() staring from a decoder makes
sure to only see decoder children. So check_commit_order() seeing
endpoint ports is a bug not something that should be silently allowed.

Re: [PATCH v7 04/10] cxl: Defer dport allocation for switch ports

[Robert Richter]

Hi Dave,

see inline.

On 14.07.25 15:35:21, Dave Jiang 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_attach_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/
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v7:
> - Return dport instead of -EEXIST (Ming)
> - Remove extra goto retry (Ming)
> ---
>  drivers/cxl/core/core.h |   2 +
>  drivers/cxl/core/pci.c  |  88 +++++++++++++++++++
>  drivers/cxl/core/port.c | 185 ++++++++++++++++++++++++++++++++++++----
>  drivers/cxl/cxl.h       |   5 ++
>  drivers/cxl/port.c      |   8 +-
>  5 files changed, 267 insertions(+), 21 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..336451b9144a 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)

count_dports()?

> +{
> +	struct cxl_walk_context *ctx = data;
> +	int type = pci_pcie_type(pdev);

The compiler might optimize this, but better place this after
pci_is_pcie(). The local variable could be omitted.

> +
> +	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_update_total_dports(struct cxl_port *port)

I would prefer to name this cxl_pci_count_dports() or so (if the need
this at all, see below).

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

This is not an error, the switch port could be non-pci. I think you
should add a pci check in front of this and early exit with 0 then.
This emphasizes also the cxl_pci_*() naming.

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

match_dport() should be named count_dports().

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

This isn't an error either, there just could be no dports online.

Why do you count this at all? An empty number of dports is possible
and not an error.

I see, you are using total_dports only to setup passthrough
decoders. I think we should allow to setup the first dport with a
passthrough decoder if cxlhdm->decoder_count of the port is zero, but
later fail if a 2nd dport is found.

> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_port_update_total_dports, "CXL");
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 9691da831224..c1de6872e57f 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1551,6 +1551,116 @@ 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));

I that message really needed for debugging? It does not seem an error
to me. The success path is much more interesting, e.g.:

   	dev_dbg(dev, "dport%d found in target list, index %d\n", ...);

> +
> +	return 0;
> +}
> +
> +static int update_decoders_with_dport(struct cxl_port *port, struct cxl_dport *dport)
> +{
> +	device_lock_assert(&port->dev);

Already checked in cxl_port_setup_with_dport().

> +	return device_for_each_child(&port->dev, dport, update_switch_decoder);

Can be squashed into cxl_port_setup_with_dport().

update_target_map() 

> +}
> +
> +static int cxl_port_setup_with_dport(struct cxl_port *port,
> +				     struct cxl_dport *dport)
> +{

Argument port can be removed, it is dport->port.

> +	device_lock_assert(&port->dev);
> +
> +	cxl_switch_parse_cdat(port);
> +
> +	return update_decoders_with_dport(port, dport);
> +}
> +
> +static struct cxl_dport *devm_cxl_port_add_dport(struct cxl_port *port,

The name is misleading as an existing dport could be reused. Name it
cxl_port_get_dport()?

> +						 struct device *dport_dev)
> +{
> +	struct cxl_dport *dport;
> +	int rc;
> +
> +	device_lock_assert(&port->dev);

Why don't you move guard() here?

> +
> +	/* Port driver not attached yet, wait for cxl_acpi reprobe */
> +	if (!port->dev.driver)
> +		return ERR_PTR(-ENODEV);
> +
> +	dport = cxl_find_dport_by_dev(port, dport_dev);
> +	if (dport)
> +		return dport;
> +
> +	dport = devm_cxl_add_dport_by_dev(port, dport_dev);
> +	if (IS_ERR(dport))
> +		return dport;
> +
> +	rc = cxl_port_setup_with_dport(port, dport);

With the removal of the port arg this could be:

	rc = cxl_dport_update_target_maps(dport);


That could be moved to devm_cxl_add_dport() along with the cleanup
below.

> +	if (rc) {
> +		reap_dport(port, dport);

Same here port, could be removed from the arg list.

> +		return ERR_PTR(rc);
> +	}
> +
> +	return dport;
> +}
> +
> +static struct cxl_dport *devm_cxl_add_dport_by_uport(struct device *uport_dev,
> +						     struct device *dport_dev)
> +{
> +	struct cxl_port *port __free(put_cxl_port) =
> +		find_cxl_port_by_uport(uport_dev);
> +
> +	if (!port)
> +		return ERR_PTR(-ENODEV);
> +
> +	guard(device)(&port->dev);
> +	return devm_cxl_port_add_dport(port, dport_dev);
> +}
> +
>  static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>  			      struct device *uport_dev,
>  			      struct device *dport_dev)
> @@ -1584,6 +1694,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;

Why don't you use dport directly?

> +
>  		if (!parent_port->dev.driver) {
>  			dev_warn(&cxlmd->dev,
>  				 "port %s:%s disabled, failed to enumerate CXL.mem\n",
> @@ -1592,6 +1704,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>  		}
>  
>  		port = find_cxl_port_at(parent_port, dport_dev, &dport);

That call could be removed as the find_cxl_port_by_uport() will find
it.

> +		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 +1713,21 @@ 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;
> +			/*
> +			 * The port holds a device reference via find_cxl_port_at()
> +			 * if the port is valid. But if the port is newly created
> +			 * via devm_cxl_add_port(), no reference is held. Therefore
> +			 * the driver needs to get a device reference here.
> +			 */
> +			get_device(&port->dev);

I would prefer the retry approach that would use
find_cxl_port_by_uport() here.

>  		}
> +
> +		guard(device)(&port->dev);
> +		new_dport = devm_cxl_port_add_dport(port, dport_dev);

I first thought, how do you ensure that dport was not yet added to
port already. But then saw the function that does something else. It
should be renamed.

> +		if (IS_ERR(new_dport))
> +			return PTR_ERR(new_dport);
> +
> +		dport = new_dport;
>  	}
>  
>  	dev_dbg(&cxlmd->dev, "add to new port %s:%s\n",
> @@ -1620,11 +1744,14 @@ 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 *dgparent;
>  	struct device *iter;
> -	int rc;
> +	int rc, i;
>  
>  	/*
>  	 * Skip intermediate port enumeration in the RCH case, there
> @@ -1643,7 +1770,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,16 +1813,39 @@ 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

"root port"

> +			 * the switch dport is not. It needs to go back to the
> +			 * beginning to setup the switch port.
> +			 */
> +			if (i >= CXL_ITER_LEVEL_SWITCH) {

Even after reading your comment I still don't know what
"CXL_ITER_LEVEL_SWITCH" means nor why this is a magic 1 ...

> +				struct cxl_port *pport __free(put_cxl_port) =
> +					cxl_mem_find_port(cxlmd, &dport);
> +				if (!pport)
> +					goto retry;

... and why retrying all this until dport is found for cxlmd all
solves this. Could this end up in an infinite loop?

I think that means if i > 0 (the root port is not found with the first
attempt) then there must be a switch port between EP and root port.

A better check would be:

	if (dev != &cxlmd->dev)
		...

It looks like there is no dport yet for cxlmd. Could you explain this?


> +			}
> +
>  			return 0;
>  		}
>  
> -		rc = add_port_attach_ep(cxlmd, uport_dev, dport_dev);
> -		/* port missing, try to add parent */
> -		if (rc == -EAGAIN)
> -			continue;
> -		/* failed to add ep or port */
> -		if (rc)
> -			return rc;
> +		dgparent = grandparent(dport_dev);
> +		/* Only go down this path if we are at the root port */
> +		if (is_cxl_hierarchy_head(dgparent)) {
> +			dport = devm_cxl_add_dport_by_uport(uport_dev,
> +							    dport_dev);
> +			/* Added a dport, restart enumeration */
> +			if (IS_ERR(dport))
> +				return PTR_ERR(dport);

Move that check to add_port_attach_ep() and return early.

But, you could move that block to the check above at the beginning of
the loop before returning 0.

I don't see much sense in the is_cxl_hierarchy_head() helper. It is
used only in this function and possibly can reduced to the one already
existing check.

Please explain why that path is different? Isn't uport_dev attached to
a root port here and can't the dports just be added on creation?

> +		} else {
> +			rc = add_port_attach_ep(cxlmd, uport_dev, dport_dev);
> +			/* port missing, try to add parent */
> +			if (rc == -EAGAIN)
> +				continue;
> +			/* failed to add ep or port */
> +			if (rc)
> +				return rc;
> +		}
> +
>  		/* port added, new descendants possible, start over */
>  		goto retry;
>  	}
> @@ -1727,16 +1877,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;

Could the target map update here be removed at all as that is also
done in the new function update_switch_decoder()?

>  	}
>  
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 3f1695c96abc..de7883747555 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_update_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..ee7dcd7c4f24 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_update_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) {

See my comment above on enabling a passthrough decoder.

Removing total_dports would simplify the implementation.

>  		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
>  		return devm_cxl_add_passthrough_decoder(port);
>  	}
> -- 
> 2.50.0
> 

-Robert

Re: [PATCH v7 07/10] cxl: Change sslbis handler to only handle single dport

[Robert Richter]

On 14.07.25 15:35:24, Dave Jiang wrote:
> 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.

I more like the approach to read the table once that collects all the
data in a struct as it is done for the xormaps. Have you considered
this?

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

Re: [PATCH v7 01/10] cxl/region: Add decoder check to check_commit_order()

[Dave Jiang]

On 7/21/25 3:12 PM, dan.j.williams@intel.com wrote:
> dan.j.williams@ wrote:
>> Dave Jiang wrote:
>>> 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
>>
>> s/pure luck/cxl_port_attach_region() only handling switch port decoder
>> assignment/
>>
>>> . Issue discovered while doing deferred dport probing when
>>> child ports are now in the midst of decoders due to ordering change
>>
>> s/midst/mix/?
>>
>> So I went looking to see if this change actually makes a difference in
>> the end, or if this was just the result of debugging an interim state of
>> the code that ended up passing non-switch decoders through this path.
>>
>> The disinction matters for understanding code-flow and history here.
>> Like something is wrong if this expectation is violated in some future
>> refactoring and may even be worth putting a dev_WARN_ONCE() in this
>> path.
>>
>> In fact I put a dev_WARN_ONCE in this path with all the patches applied
>> and it never triggers with a cxl test suite run, so I think it is safe
>> to do something like the below.
>>
>> ...but before going there, can you confirm the expectation that the
>> safety is not strictly needed?
> 
> Ok, so I finally remembered the previous discussion here and switch
> ports have both endpoint-port child devices and decoder child devices.
> 
> However, endpoint-ports are always added *after* decoders. That is
> enforced by the fact that decoders are added in cxl_switch_port_probe()
> and endpoint-ports serialize against that before adding new ports.
> 
> The device_for_each_child_reverse_from() staring from a decoder makes
> sure to only see decoder children. So check_commit_order() seeing
> endpoint ports is a bug not something that should be silently allowed.

I think at one point the decoders were pushed to be allocated at a later time. However, the current implementation allocates all the decoders at the time that it was done originally and we just end up updating the targets for the decoders when the dports show up. So I'm thinking maybe this patch can just be dropped. 

Re: [PATCH v7 07/10] cxl: Change sslbis handler to only handle single dport

[Dave Jiang]

On 7/21/25 4:18 PM, Robert Richter wrote:
> On 14.07.25 15:35:24, Dave Jiang wrote:
>> 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.
> 
> I more like the approach to read the table once that collects all the
> data in a struct as it is done for the xormaps. Have you considered
> this?
>

Ok let me take a look.

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

Re: [PATCH v7 04/10] cxl: Defer dport allocation for switch ports

[Dave Jiang]

On 7/21/25 4:14 PM, Robert Richter wrote:
> Hi Dave,
> 
> see inline.
> 
> On 14.07.25 15:35:21, Dave Jiang 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_attach_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/
>> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> v7:
>> - Return dport instead of -EEXIST (Ming)
>> - Remove extra goto retry (Ming)
>> ---
>>  drivers/cxl/core/core.h |   2 +
>>  drivers/cxl/core/pci.c  |  88 +++++++++++++++++++
>>  drivers/cxl/core/port.c | 185 ++++++++++++++++++++++++++++++++++++----
>>  drivers/cxl/cxl.h       |   5 ++
>>  drivers/cxl/port.c      |   8 +-
>>  5 files changed, 267 insertions(+), 21 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..336451b9144a 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)
> 
> count_dports()?

ok
> 
>> +{
>> +	struct cxl_walk_context *ctx = data;
>> +	int type = pci_pcie_type(pdev);
> 
> The compiler might optimize this, but better place this after
> pci_is_pcie(). The local variable could be omitted.

ok

> 
>> +
>> +	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_update_total_dports(struct cxl_port *port)
> 
> I would prefer to name this cxl_pci_count_dports() or so (if the need
> this at all, see below).

ok
> 
>> +{
>> +	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;
> 
> This is not an error, the switch port could be non-pci. I think you
> should add a pci check in front of this and early exit with 0 then.
> This emphasizes also the cxl_pci_*() naming.

cxl_test replaces this function with its own mock function. So unless we are dealing with some other topology, it should be PCI.

> 
>> +	}
>> +
>> +	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);
> 
> match_dport() should be named count_dports().

ok
> 
>> +
>> +	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;
> 
> This isn't an error either, there just could be no dports online.

The dports should exist even if it's not online. This counts total possible dports and not total online dports. We don't try to access it. Just checking if the dport has be enumerated by the PCI subsystem. If we encounter a port with no dports at all, this should be an error. 

> 
> Why do you count this at all? An empty number of dports is possible
> and not an error.
> 
> I see, you are using total_dports only to setup passthrough
> decoders. I think we should allow to setup the first dport with a
> passthrough decoder if cxlhdm->decoder_count of the port is zero, but
> later fail if a 2nd dport is found.

So we should know how many possible dports there can be, just some may not be online yet. But it is easier to just get that count and create the passthrough decoder if the count is 1 rather than do the dance of seeing if a second dport shows up later and try to kill the passthrough decoder. Maybe it's not necessary to store a total_dports in cxl_port. 

DJ
  
> 
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_port_update_total_dports, "CXL");
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 9691da831224..c1de6872e57f 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -1551,6 +1551,116 @@ 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));
> 
> I that message really needed for debugging? It does not seem an error
> to me. The success path is much more interesting, e.g.:
> 
>    	dev_dbg(dev, "dport%d found in target list, index %d\n", ...);
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int update_decoders_with_dport(struct cxl_port *port, struct cxl_dport *dport)
>> +{
>> +	device_lock_assert(&port->dev);
> 
> Already checked in cxl_port_setup_with_dport().
> 
>> +	return device_for_each_child(&port->dev, dport, update_switch_decoder);
> 
> Can be squashed into cxl_port_setup_with_dport().
> 
> update_target_map() 
> 
>> +}
>> +
>> +static int cxl_port_setup_with_dport(struct cxl_port *port,
>> +				     struct cxl_dport *dport)
>> +{
> 
> Argument port can be removed, it is dport->port.
> 
>> +	device_lock_assert(&port->dev);
>> +
>> +	cxl_switch_parse_cdat(port);
>> +
>> +	return update_decoders_with_dport(port, dport);
>> +}
>> +
>> +static struct cxl_dport *devm_cxl_port_add_dport(struct cxl_port *port,
> 
> The name is misleading as an existing dport could be reused. Name it
> cxl_port_get_dport()?
> 
>> +						 struct device *dport_dev)
>> +{
>> +	struct cxl_dport *dport;
>> +	int rc;
>> +
>> +	device_lock_assert(&port->dev);
> 
> Why don't you move guard() here?
> 
>> +
>> +	/* Port driver not attached yet, wait for cxl_acpi reprobe */
>> +	if (!port->dev.driver)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	dport = cxl_find_dport_by_dev(port, dport_dev);
>> +	if (dport)
>> +		return dport;
>> +
>> +	dport = devm_cxl_add_dport_by_dev(port, dport_dev);
>> +	if (IS_ERR(dport))
>> +		return dport;
>> +
>> +	rc = cxl_port_setup_with_dport(port, dport);
> 
> With the removal of the port arg this could be:
> 
> 	rc = cxl_dport_update_target_maps(dport);
> 
> 
> That could be moved to devm_cxl_add_dport() along with the cleanup
> below.
> 
>> +	if (rc) {
>> +		reap_dport(port, dport);
> 
> Same here port, could be removed from the arg list.
> 
>> +		return ERR_PTR(rc);
>> +	}
>> +
>> +	return dport;
>> +}
>> +
>> +static struct cxl_dport *devm_cxl_add_dport_by_uport(struct device *uport_dev,
>> +						     struct device *dport_dev)
>> +{
>> +	struct cxl_port *port __free(put_cxl_port) =
>> +		find_cxl_port_by_uport(uport_dev);
>> +
>> +	if (!port)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	guard(device)(&port->dev);
>> +	return devm_cxl_port_add_dport(port, dport_dev);
>> +}
>> +
>>  static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>>  			      struct device *uport_dev,
>>  			      struct device *dport_dev)
>> @@ -1584,6 +1694,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;
> 
> Why don't you use dport directly?
> 
>> +
>>  		if (!parent_port->dev.driver) {
>>  			dev_warn(&cxlmd->dev,
>>  				 "port %s:%s disabled, failed to enumerate CXL.mem\n",
>> @@ -1592,6 +1704,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>>  		}
>>  
>>  		port = find_cxl_port_at(parent_port, dport_dev, &dport);
> 
> That call could be removed as the find_cxl_port_by_uport() will find
> it.
> 
>> +		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 +1713,21 @@ 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;
>> +			/*
>> +			 * The port holds a device reference via find_cxl_port_at()
>> +			 * if the port is valid. But if the port is newly created
>> +			 * via devm_cxl_add_port(), no reference is held. Therefore
>> +			 * the driver needs to get a device reference here.
>> +			 */
>> +			get_device(&port->dev);
> 
> I would prefer the retry approach that would use
> find_cxl_port_by_uport() here.
> 
>>  		}
>> +
>> +		guard(device)(&port->dev);
>> +		new_dport = devm_cxl_port_add_dport(port, dport_dev);
> 
> I first thought, how do you ensure that dport was not yet added to
> port already. But then saw the function that does something else. It
> should be renamed.
> 
>> +		if (IS_ERR(new_dport))
>> +			return PTR_ERR(new_dport);
>> +
>> +		dport = new_dport;
>>  	}
>>  
>>  	dev_dbg(&cxlmd->dev, "add to new port %s:%s\n",
>> @@ -1620,11 +1744,14 @@ 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 *dgparent;
>>  	struct device *iter;
>> -	int rc;
>> +	int rc, i;
>>  
>>  	/*
>>  	 * Skip intermediate port enumeration in the RCH case, there
>> @@ -1643,7 +1770,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,16 +1813,39 @@ 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
> 
> "root port"
> 
>> +			 * the switch dport is not. It needs to go back to the
>> +			 * beginning to setup the switch port.
>> +			 */
>> +			if (i >= CXL_ITER_LEVEL_SWITCH) {
> 
> Even after reading your comment I still don't know what
> "CXL_ITER_LEVEL_SWITCH" means nor why this is a magic 1 ...
> 
>> +				struct cxl_port *pport __free(put_cxl_port) =
>> +					cxl_mem_find_port(cxlmd, &dport);
>> +				if (!pport)
>> +					goto retry;
> 
> ... and why retrying all this until dport is found for cxlmd all
> solves this. Could this end up in an infinite loop?
> 
> I think that means if i > 0 (the root port is not found with the first
> attempt) then there must be a switch port between EP and root port.
> 
> A better check would be:
> 
> 	if (dev != &cxlmd->dev)
> 		...
> 
> It looks like there is no dport yet for cxlmd. Could you explain this?
> 
> 
>> +			}
>> +
>>  			return 0;
>>  		}
>>  
>> -		rc = add_port_attach_ep(cxlmd, uport_dev, dport_dev);
>> -		/* port missing, try to add parent */
>> -		if (rc == -EAGAIN)
>> -			continue;
>> -		/* failed to add ep or port */
>> -		if (rc)
>> -			return rc;
>> +		dgparent = grandparent(dport_dev);
>> +		/* Only go down this path if we are at the root port */
>> +		if (is_cxl_hierarchy_head(dgparent)) {
>> +			dport = devm_cxl_add_dport_by_uport(uport_dev,
>> +							    dport_dev);
>> +			/* Added a dport, restart enumeration */
>> +			if (IS_ERR(dport))
>> +				return PTR_ERR(dport);
> 
> Move that check to add_port_attach_ep() and return early.
> 
> But, you could move that block to the check above at the beginning of
> the loop before returning 0.
> 
> I don't see much sense in the is_cxl_hierarchy_head() helper. It is
> used only in this function and possibly can reduced to the one already
> existing check.
> 
> Please explain why that path is different? Isn't uport_dev attached to
> a root port here and can't the dports just be added on creation?
> 
>> +		} else {
>> +			rc = add_port_attach_ep(cxlmd, uport_dev, dport_dev);
>> +			/* port missing, try to add parent */
>> +			if (rc == -EAGAIN)
>> +				continue;
>> +			/* failed to add ep or port */
>> +			if (rc)
>> +				return rc;
>> +		}
>> +
>>  		/* port added, new descendants possible, start over */
>>  		goto retry;
>>  	}
>> @@ -1727,16 +1877,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;
> 
> Could the target map update here be removed at all as that is also
> done in the new function update_switch_decoder()?
> 
>>  	}
>>  
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 3f1695c96abc..de7883747555 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_update_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..ee7dcd7c4f24 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_update_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) {
> 
> See my comment above on enabling a passthrough decoder.
> 
> Removing total_dports would simplify the implementation.
> 
>>  		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
>>  		return devm_cxl_add_passthrough_decoder(port);
>>  	}
>> -- 
>> 2.50.0
>>
> 
> -Robert

Re: [PATCH v7 04/10] cxl: Defer dport allocation for switch ports

[dan.j.williams]

I ended up with more comments than I was expecting. The logic looks
sound, but the organization and naming threw me in several places. Jump
to the commentary around the devm_cxl_create_or_extend_port() for the
meat of the feedback.

I did pause and think, "should I be giving this much feedback this late
on a patch that has passed other review and looks functionally viable".
It comes down to whether I expect to be contributing to drivers/cxl/
longterm and would I send follow-on patches to clean this up, the answer
is yes to both.

Thankfully, I also found a crash with the current code in
devm_cxl_add_passthrough_decoder(), so it needs to be respun anyway.
That "too much feedback" question still lingers such that I am open to
pushback on some of the arbitrary naming and organization choices.

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

Some minor grammar to fix up below, but I do like that this changelog
attempts to tell the before and after story.

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

s/enumerate/enumerates/

> 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

s/use/uses/
s/enumerates/enumerate/

> RPs as the dports of these ports. Every time a port is created, the port
> driver is attached and drv->probe() is called and

s/attached and drv->probe()/attached, cxl_switch_port_probe() is called

> devm_cxl_port_enumerate_dports() is envoked to enumerate and probe

s/envoked/invoked/

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

Might as well spell these out as cxl_mem_probe() rather than make the
read lookup that cxl_mem->probe() points to cxl_mem_probe().

> 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

s/different patch later on/later/

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

The story of what this patch does in the end gets muddy in this
paragraph, and the paragraphs below just look like narration of the
mechanical code changes rather than the story.

The story as I understand it is:

"The new sequence is instead of creating all possible dports at initial
port creation, defer dport instantiation until a memdev beneath that
dport arrives. Introduce devm_cxl_create_or_extend_port() to centralize
the creation and extension of ports with new dports as memory devices
arrive. As part of this rework @target_map moves to 'struct
cxl_switch_decoder' so that hardware port-id lookups can be amended at
runtime."

> 
> 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_attach_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/
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v7:
> - Return dport instead of -EEXIST (Ming)
> - Remove extra goto retry (Ming)
> ---
>  drivers/cxl/core/core.h |   2 +
>  drivers/cxl/core/pci.c  |  88 +++++++++++++++++++
>  drivers/cxl/core/port.c | 185 ++++++++++++++++++++++++++++++++++++----
>  drivers/cxl/cxl.h       |   5 ++
>  drivers/cxl/port.c      |   8 +-
>  5 files changed, 267 insertions(+), 21 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..336451b9144a 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_update_total_dports(struct cxl_port *port)

More name quibbling, but this quibbling is not about personal preference
it is about setting expectations with the reader. "Update" to me implies
"may already be established, and may be changed at runtime".  This is a
one-time initialization at the beginning of the device's driver-attached
lifetime.

So, something like cxl_probe_possible_dports()?

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

Why warn? Driver warnings cause customer tickets in distro bug trackers.
They should be something actionable.

> +		return -ENXIO;

Why fail?

If the driver enumerates a port with no dports then naturally no
endpoint can appear underneath. This just looks like a violation of the
Robustness Principle (be liberal in what you accept) with no gain that
immediately comes to mind. If hardware wants to produce vestigial ports,
let them.

> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_port_update_total_dports, "CXL");

In the course of looking up whether this "update" routine is ever called
more than once, it made me realize that this is yet another pure helper
function that should live in cxl/port.o. It lives in cxl/core/port.o
only so testing/cxl/test/cxl_test.o can override it.

There is no reason that on production distro builds of the CXL driver
that this function needs to live in cxl_core.ko and be exported.

A cleanup project for another time would be move functions like this out
of the core and into something like cxl/port_lib.o. Where cxl/port_lib.o is
builtin to cxl/port.o in the common case, but a config symbol like
CXL_TEST_ENABLE moves port_lib.o to its own .ko object and allows for
the cxl_test magic to intercept at that point.

> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 9691da831224..c1de6872e57f 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1551,6 +1551,116 @@ 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)

A more precise name might be "update_decoder_targets".

> +{
> +	struct cxl_dport *dport = data;
> +	struct cxl_switch_decoder *cxlsd;
> +	struct cxl_decoder *cxld;
> +	int i;
> +
> +	if (!is_switch_decoder(dev))

Looks good, unlike patch1 this really *is* a case where all possible
children of a switch port might pass through this helper.

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

Why is this assert here?

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

Why is this assert here?

Just assert at leafs where the locking has actual impact.

This might need a devm_cxl_add_dport_unlocked() to distinguish paths
that add dports while already known to be holding the port device-lock
from paths that do it outside of that context like
devm_cxl_enumerate_ports().

> +
> +	cxl_switch_parse_cdat(port);

Don't you want to do this *after* new dports are added to setup their
coordinates?

> +
> +	return update_decoders_with_dport(port, dport);
> +}
> +
> +static struct cxl_dport *devm_cxl_port_add_dport(struct cxl_port *port,
> +						 struct device *dport_dev)
> +{
> +	struct cxl_dport *dport;
> +	int rc;
> +
> +	device_lock_assert(&port->dev);
> +
> +	/* Port driver not attached yet, wait for cxl_acpi reprobe */

The reason may not be cxl_acpi, and may not be solved by waiting. I
would just drop the comment. In general "ACPI" should not appear in
drivers/cxl/core/, outside of cdat.c which just happens to reuse ACPI
defined values.

> +	if (!port->dev.driver)
> +		return ERR_PTR(-ENODEV);

I would make this ENXIO because the device is found, just not ready to
take on new activations.

> +
> +	dport = cxl_find_dport_by_dev(port, dport_dev);
> +	if (dport)
> +		return dport;
> +
> +	dport = devm_cxl_add_dport_by_dev(port, dport_dev);
> +	if (IS_ERR(dport))
> +		return dport;
> +
> +	rc = cxl_port_setup_with_dport(port, dport);
> +	if (rc) {
> +		reap_dport(port, dport);
> +		return ERR_PTR(rc);
> +	}
> +
> +	return dport;
> +}
> +
> +static struct cxl_dport *devm_cxl_add_dport_by_uport(struct device *uport_dev,
> +						     struct device *dport_dev)
> +{
> +	struct cxl_port *port __free(put_cxl_port) =
> +		find_cxl_port_by_uport(uport_dev);
> +
> +	if (!port)
> +		return ERR_PTR(-ENODEV);
> +
> +	guard(device)(&port->dev);

This guard feels too far removed from where it is actually needed. If
all dport creation is moving outside of the cxl_switch_port_probe() path
then does that mean that devm_cxl_add_dport() can be replaced by
devm_cxl_port_add_dport()?

> +	return devm_cxl_port_add_dport(port, dport_dev);
> +}
> +
>  static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>  			      struct device *uport_dev,
>  			      struct device *dport_dev)
> @@ -1584,6 +1694,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) {

Not this patch's problem, but this pattern is loudly asking for a helper
function that does all of this:

struct cxl_port *port __free(put_cxl_port) = find_or_add_port(..., &dport);

...because this "... __free(...) = NULL" pattern is almost always the
wrong the answer.

...however I think a devm_cxl_create_or_extend_port() reorganization
makes all port reference management disappear in this path. See below.

> +		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 +1704,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 +1713,21 @@ 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;
> +			/*
> +			 * The port holds a device reference via find_cxl_port_at()
> +			 * if the port is valid. But if the port is newly created
> +			 * via devm_cxl_add_port(), no reference is held. Therefore
> +			 * the driver needs to get a device reference here.
> +			 */
> +			get_device(&port->dev);
>  		}
> +
> +		guard(device)(&port->dev);

This seems too far removed from where the lock is actually needed.

> +		new_dport = devm_cxl_port_add_dport(port, dport_dev);

I can follow this, it looks correct, but I do not like that it redoes
the port lookup potentially 3 times. 4 times if you count the original
lookup before dropping into add_port_attach_ep().

So that find_or_add() suggestion I said above probably wants to be more
like an API likes this:

struct cxl_dport *devm_cxl_create_or_extend_port(parent_port, port, uport_dev, dport_dev)

...because nothing in add_port_attach_ep() actually needs the created
port. It only needs the dport which implies a port exists, then you only
need to do the lookup 

> +		if (IS_ERR(new_dport))
> +			return PTR_ERR(new_dport);
> +
> +		dport = new_dport;
>  	}
>  
>  	dev_dbg(&cxlmd->dev, "add to new port %s:%s\n",
> @@ -1620,11 +1744,14 @@ 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 *dgparent;
>  	struct device *iter;
> -	int rc;
> +	int rc, i;
>  
>  	/*
>  	 * Skip intermediate port enumeration in the RCH case, there
> @@ -1643,7 +1770,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,16 +1813,39 @@ 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.
> +			 */

This feels like a shortcut to avoid reworking the main loop for the new
constraint of adding dports to all paths in the ancestry.

> +			if (i >= CXL_ITER_LEVEL_SWITCH) {
> +				struct cxl_port *pport __free(put_cxl_port) =
> +					cxl_mem_find_port(cxlmd, &dport);
> +				if (!pport)
> +					goto retry;
> +			}
> +
>  			return 0;
>  		}
>  
> -		rc = add_port_attach_ep(cxlmd, uport_dev, dport_dev);
> -		/* port missing, try to add parent */
> -		if (rc == -EAGAIN)
> -			continue;
> -		/* failed to add ep or port */
> -		if (rc)
> -			return rc;
> +		dgparent = grandparent(dport_dev);
> +		/* Only go down this path if we are at the root port */
> +		if (is_cxl_hierarchy_head(dgparent)) {

It turns out "hierarchy_head" is just a "host bridge". Lets just call it
that and not invent a new term. However, I do not understand why
devm_cxl_add_dport_by_uport is done separately from the
add_port_attach_ep() path? Are they not doing the same thing?

> +			dport = devm_cxl_add_dport_by_uport(uport_dev,
> +							    dport_dev);
> +			/* Added a dport, restart enumeration */
> +			if (IS_ERR(dport))
> +				return PTR_ERR(dport);
> +		} else {
> +			rc = add_port_attach_ep(cxlmd, uport_dev, dport_dev);
> +			/* port missing, try to add parent */
> +			if (rc == -EAGAIN)
> +				continue;
> +			/* failed to add ep or port */
> +			if (rc)
> +				return rc;
> +		}
> +
>  		/* port added, new descendants possible, start over */
>  		goto retry;
>  	}
> @@ -1727,16 +1877,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));

No need for a copy if the target_map is in the cxl_switch_decoder
object.

>  
>  	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..de7883747555 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];

This can save space by being a u8 since hardware port ids are 8-bits.

I would do a lead-in patch to introduce this and drop the @target_map
argument to cxl_decoder_add() and its helpers.

It might help to clarify somewhere that this is a cached copy of the
hardware port-id list and that it is available at init even before all
@dport objects have been discovered / instantiated.

>  	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

I was confused by the "total_dports" name choice until I read this
comment. Yes, "possible" is a well known concept for a count of things
that can pop into existing later, like "possible CPUs" and "possible
Nodes".  Just s/total_dports/possible_dports/ throughout this patch to
fix up the confusion.

>   * @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_update_total_dports(struct cxl_port *port);

...and per above, this is never an "update" it is always static similar
to possible CPUs" and "possible Nodes".

Re: [PATCH v7 05/10] cxl/test: Add cxl_test support for cxl_port_update_total_ports()

[dan.j.williams]

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

Looks ok, but this coming after the core code adds the call means that
there is a cxl_test bisect break? I guess there is no clean way to avoid
that.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH v7 06/10] cxl/test: Add mock version of devm_cxl_add_dport_by_dev()

[dan.j.williams]

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

Co-developed-by always needs to come with Signed-off-by. Go ahead and
add that in lieu of my Reviewed-by.

Re: [PATCH v7 07/10] cxl: Change sslbis handler to only handle single dport

[dan.j.williams]

Dave Jiang wrote:
> 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 c1de6872e57f..afeebb0ce4ab 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);

Ah, I now realize I misread patch4 where I asked about the ordering of
this function relative to the update. The dport is already added at this
point.

>  
>  	return update_decoders_with_dport(port, dport);

More delayed realization... cxl_port_setup_with_dport() should be
"void", it never fails.

As for this patch, looks good.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH v7 09/10] cxl: Move enumeration of hostbridge ports to the memdev probe path

[dan.j.williams]

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

I do not understand the story here. Why is it necessary to hide host
bridge ports that do not have endpoints attached? I think that is
valuable information for hotplug to identify available ports.

Now, if the concern is that host bridge CXL component register
enumeration happens too early then the solution there is delay port
register setup until the first dport arrival.

Otherwise this feels like too large of a change for just that small
ordering constraint and I think this makes some of the hierarchy walking
redundant. I suspect that moving register init lets the
devm_cxl_enumerate_ports() walk handle everything without new callbacks
and new lookup infrastructure (patch8).

I have long wanted to move cxl_port_setup_regs() out of cxl_port_add().
It has always been an awkward fit their because register init is usually
something that belongs in a ->probe() path.

Re: [PATCH v7 10/10] cxl: Remove devm_cxl_port_enumerate_dports() that is no longer used

[dan.j.williams]

Dave Jiang wrote:
> Delete function devm_cxl_port_enumerate_dports() which is no longer being
> used anywhere. Also remove cxl_test support.

Looks good.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH v7 09/10] cxl: Move enumeration of hostbridge ports to the memdev probe path

[Dave Jiang]

On 7/22/25 11:31 AM, dan.j.williams@intel.com wrote:
> Dave Jiang 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.
> 
> I do not understand the story here. Why is it necessary to hide host
> bridge ports that do not have endpoints attached? I think that is
> valuable information for hotplug to identify available ports.

It's not going out of the way to hide on purpose. It's more that this is the resulting behavior when we are setting up the hierarchy via the mem probe.

> 
> Now, if the concern is that host bridge CXL component register
> enumeration happens too early then the solution there is delay port
> register setup until the first dport arrival.

Are you saying to just delay the RP register setup until when dports show up, leave the enumeration as is?
> 
> Otherwise this feels like too large of a change for just that small
> ordering constraint and I think this makes some of the hierarchy walking
> redundant. I suspect that moving register init lets the
> devm_cxl_enumerate_ports() walk handle everything without new callbacks
> and new lookup infrastructure (patch8).
> 
> I have long wanted to move cxl_port_setup_regs() out of cxl_port_add().
> It has always been an awkward fit their because register init is usually
> something that belongs in a ->probe() path.

Re: [PATCH v7 09/10] cxl: Move enumeration of hostbridge ports to the memdev probe path

[dan.j.williams]

Dave Jiang wrote:
> 
> 
> On 7/22/25 11:31 AM, dan.j.williams@intel.com wrote:
> > Dave Jiang 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.
> > 
> > I do not understand the story here. Why is it necessary to hide host
> > bridge ports that do not have endpoints attached? I think that is
> > valuable information for hotplug to identify available ports.
> 
> It's not going out of the way to hide on purpose. It's more that this
> is the resulting behavior when we are setting up the hierarchy via the
> mem probe.

Right, but that feels like a step backwards in system observability.

> > 
> > Now, if the concern is that host bridge CXL component register
> > enumeration happens too early then the solution there is delay port
> > register setup until the first dport arrival.
> 
> Are you saying to just delay the RP register setup until when dports
> show up, leave the enumeration as is?

Right, cxl_port_setup_regs() moves out of cxl_port_add() and gets
triggered by first dport assignment to a port.

Re: [PATCH v7 04/10] cxl: Defer dport allocation for switch ports

[Dave Jiang]

On 7/22/25 8:50 AM, dan.j.williams@intel.com wrote:

<--snip-->

>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 3f1695c96abc..de7883747555 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];
> 
> This can save space by being a u8 since hardware port ids are 8-bits.
> 
> I would do a lead-in patch to introduce this and drop the @target_map
> argument to cxl_decoder_add() and its helpers.
> 
> It might help to clarify somewhere that this is a cached copy of the
> hardware port-id list and that it is available at init even before all
> @dport objects have been discovered / instantiated.
> 

This actually needs to be u32 and cannot be u8. Mainly because CFMWS interleave targets is 4 bytes. And if we set this to u8, it'll break things later on when we do region decoder matching for the root port (took couple days for me to chase that on hardware).