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

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

[Dave Jiang]

This series attempts to delay the setup of dports and Host Bridge (HB) register
until when the endpoint device (memdev) is 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: Saperate out CXL dport->id vs actual dport hardware id
  cxl: Rename find_dport() to provide better function intent
  cxl: Remove adding of port_num via devm_cxl_add_dport()
  cxl: Defer hardware dport->port_id assignment and registers probing
  cxl/test: Add workaround for cxl_test for cxl_core calling mocked functions
  cxl: Change sslbis handler to only handle single dport
  cxl: Add helper to detect top of CXL device topology
  cxl: Create an xarray to tie a host bridge to the cxl_root
  cxl: Move enumeration of hostbridge ports to the memdev probe path

 drivers/cxl/acpi.c                   | 143 ++++++-----
 drivers/cxl/core/cdat.c              |  23 +-
 drivers/cxl/core/core.h              |   4 +
 drivers/cxl/core/hdm.c               |  45 ++--
 drivers/cxl/core/pci.c               |  60 ++++-
 drivers/cxl/core/port.c              | 353 +++++++++++++++++++++++----
 drivers/cxl/core/region.c            |   7 +-
 drivers/cxl/cxl.h                    |  50 +++-
 drivers/cxl/port.c                   |  20 +-
 tools/testing/cxl/Kbuild             |   4 +-
 tools/testing/cxl/cxl_core_exports.c |  31 +++
 tools/testing/cxl/exports.h          |  17 ++
 tools/testing/cxl/test/cxl.c         |   5 +-
 tools/testing/cxl/test/mock.c        |  40 +--
 14 files changed, 611 insertions(+), 191 deletions(-)
 create mode 100644 tools/testing/cxl/exports.h


base-commit: b4432656b36e5cc1d50a1f2dc15357543add530e
-- 
2.49.0

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

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 c3f4dc244df7..a91d4eb061e4 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -788,7 +788,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

base-commit: 3c746d4821f507304b08789e3c7c151554fc2356
-- 
2.49.0

[PATCH v2 02/10] cxl: Saperate out CXL dport->id vs actual dport hardware id

[Dave Jiang]

In preparation to allow dport to be allocated without being active, make
dport->id to be Linux id that enumerates the dport objects per port.
Keep the hardware id under dport->port_num to maintain compatibility and
introduce a dport->id as the enumeration id.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v2:
- Rename port_id to port_num. (Dan)
- Use a dport allocator to deal with ida allocation. (Dan, Jonathan)
---
 drivers/cxl/core/cdat.c |  2 +-
 drivers/cxl/core/hdm.c  | 18 +++++-----
 drivers/cxl/core/port.c | 73 ++++++++++++++++++++++++++++++-----------
 drivers/cxl/cxl.h       | 12 ++++---
 4 files changed, 71 insertions(+), 34 deletions(-)

diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index edb4f41eeacc..f637e3631d88 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -501,7 +501,7 @@ static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg,
 
 		xa_for_each(&port->dports, index, dport) {
 			if (dsp_id == ACPI_CDAT_SSLBIS_ANY_PORT ||
-			    dsp_id == dport->port_id) {
+			    dsp_id == dport->port_num) {
 				cxl_access_coordinate_set(dport->coord,
 							  sslbis->data_type,
 							  val);
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 70cae4ebf8a4..d7ad92e191ba 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -69,7 +69,7 @@ int devm_cxl_add_passthrough_decoder(struct cxl_port *port)
 
 	xa_for_each(&port->dports, index, dport)
 		break;
-	single_port_map[0] = dport->port_id;
+	single_port_map[0] = dport->port_num;
 
 	return add_hdm_decoder(port, &cxlsd->cxld, single_port_map);
 }
@@ -720,21 +720,21 @@ static void cxlsd_set_targets(struct cxl_switch_decoder *cxlsd, u64 *tgt)
 	struct cxl_dport **t = &cxlsd->target[0];
 	int ways = cxlsd->cxld.interleave_ways;
 
-	*tgt = FIELD_PREP(GENMASK(7, 0), t[0]->port_id);
+	*tgt = FIELD_PREP(GENMASK(7, 0), t[0]->port_num);
 	if (ways > 1)
-		*tgt |= FIELD_PREP(GENMASK(15, 8), t[1]->port_id);
+		*tgt |= FIELD_PREP(GENMASK(15, 8), t[1]->port_num);
 	if (ways > 2)
-		*tgt |= FIELD_PREP(GENMASK(23, 16), t[2]->port_id);
+		*tgt |= FIELD_PREP(GENMASK(23, 16), t[2]->port_num);
 	if (ways > 3)
-		*tgt |= FIELD_PREP(GENMASK(31, 24), t[3]->port_id);
+		*tgt |= FIELD_PREP(GENMASK(31, 24), t[3]->port_num);
 	if (ways > 4)
-		*tgt |= FIELD_PREP(GENMASK_ULL(39, 32), t[4]->port_id);
+		*tgt |= FIELD_PREP(GENMASK_ULL(39, 32), t[4]->port_num);
 	if (ways > 5)
-		*tgt |= FIELD_PREP(GENMASK_ULL(47, 40), t[5]->port_id);
+		*tgt |= FIELD_PREP(GENMASK_ULL(47, 40), t[5]->port_num);
 	if (ways > 6)
-		*tgt |= FIELD_PREP(GENMASK_ULL(55, 48), t[6]->port_id);
+		*tgt |= FIELD_PREP(GENMASK_ULL(55, 48), t[6]->port_num);
 	if (ways > 7)
-		*tgt |= FIELD_PREP(GENMASK_ULL(63, 56), t[7]->port_id);
+		*tgt |= FIELD_PREP(GENMASK_ULL(63, 56), t[7]->port_num);
 }
 
 /*
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 726bd4a7de27..e9c02e4d0d4c 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -159,7 +159,7 @@ static ssize_t emit_target_list(struct cxl_switch_decoder *cxlsd, char *buf)
 
 		if (i + 1 < cxld->interleave_ways)
 			next = cxlsd->target[i + 1];
-		rc = sysfs_emit_at(buf, offset, "%d%s", dport->port_id,
+		rc = sysfs_emit_at(buf, offset, "%d%s", dport->id,
 				   next ? "," : "");
 		if (rc < 0)
 			return rc;
@@ -739,6 +739,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev,
 		dev->parent = uport_dev;
 
 	ida_init(&port->decoder_ida);
+	ida_init(&port->dport_ida);
 	port->hdm_end = -1;
 	port->commit_end = -1;
 	xa_init(&port->dports);
@@ -1044,14 +1045,14 @@ void put_cxl_root(struct cxl_root *cxl_root)
 }
 EXPORT_SYMBOL_NS_GPL(put_cxl_root, "CXL");
 
-static struct cxl_dport *find_dport(struct cxl_port *port, int id)
+static struct cxl_dport *find_dport(struct cxl_port *port, int port_num)
 {
 	struct cxl_dport *dport;
 	unsigned long index;
 
 	device_lock_assert(&port->dev);
 	xa_for_each(&port->dports, index, dport)
-		if (dport->port_id == id)
+		if (dport->port_num == port_num)
 			return dport;
 	return NULL;
 }
@@ -1062,11 +1063,11 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *dport)
 	int rc;
 
 	device_lock_assert(&port->dev);
-	dup = find_dport(port, dport->port_id);
+	dup = find_dport(port, dport->port_num);
 	if (dup) {
 		dev_err(&port->dev,
-			"unable to add dport%d-%s non-unique port id (%s)\n",
-			dport->port_id, dev_name(dport->dport_dev),
+			"unable to add dport%d-%s non-unique port num (%s)\n",
+			dport->port_num, dev_name(dport->dport_dev),
 			dev_name(dup->dport_dev));
 		return -EBUSY;
 	}
@@ -1114,13 +1115,43 @@ static void cxl_dport_unlink(void *data)
 	struct cxl_port *port = dport->port;
 	char link_name[CXL_TARGET_STRLEN];
 
-	sprintf(link_name, "dport%d", dport->port_id);
+	sprintf(link_name, "dport%d", dport->id);
 	sysfs_remove_link(&port->dev.kobj, link_name);
 }
 
+static void free_dport(void *data)
+{
+	struct cxl_dport *dport = data;
+	struct cxl_port *port = dport->port;
+
+	ida_free(&port->dport_ida, dport->id);
+	kfree(dport);
+}
+
+static struct cxl_dport *cxl_alloc_dport(struct cxl_port *port,
+					 struct device *dport_dev)
+{
+	int id;
+
+	struct cxl_dport *dport __free(kfree) =
+		kzalloc(sizeof(*dport), GFP_KERNEL);
+	if (!dport)
+		return NULL;
+
+	id = ida_alloc(&port->dport_ida, GFP_KERNEL);
+	if (id < 0)
+		return NULL;
+
+	dport->dport_dev = dport_dev;
+	dport->port = port;
+	dport->id = id;
+
+	return no_free_ptr(dport);
+}
+
 static struct cxl_dport *
 __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
-		     int port_id, resource_size_t component_reg_phys,
+		     int port_num, resource_size_t component_reg_phys,
 		     resource_size_t rcrb)
 {
 	char link_name[CXL_TARGET_STRLEN];
@@ -1139,17 +1170,19 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
 		return ERR_PTR(-ENXIO);
 	}
 
-	if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >=
+	dport = cxl_alloc_dport(port, dport_dev);
+	if (!dport)
+		return ERR_PTR(-ENOMEM);
+
+	rc = devm_add_action_or_reset(&port->dev, free_dport, dport);
+	if (rc)
+		return ERR_PTR(rc);
+
+	if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", dport->id) >=
 	    CXL_TARGET_STRLEN)
 		return ERR_PTR(-EINVAL);
 
-	dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
-	if (!dport)
-		return ERR_PTR(-ENOMEM);
-
-	dport->dport_dev = dport_dev;
-	dport->port_id = port_id;
-	dport->port = port;
+	dport->port_num = port_num;
 
 	if (rcrb == CXL_RESOURCE_NONE) {
 		rc = cxl_dport_setup_regs(&port->dev, dport,
@@ -1211,7 +1244,7 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
  * devm_cxl_add_dport - append VH downstream port data to a cxl_port
  * @port: the cxl_port that references this dport
  * @dport_dev: firmware or PCI device representing the dport
- * @port_id: identifier for this dport in a decoder's target list
+ * @port_num: identifier for this dport in a decoder's target list
  * @component_reg_phys: optional location of CXL component registers
  *
  * Note that dports are appended to the devm release action's of the
@@ -1219,12 +1252,12 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
  * switch ports)
  */
 struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
-				     struct device *dport_dev, int port_id,
+				     struct device *dport_dev, int port_num,
 				     resource_size_t component_reg_phys)
 {
 	struct cxl_dport *dport;
 
-	dport = __devm_cxl_add_dport(port, dport_dev, port_id,
+	dport = __devm_cxl_add_dport(port, dport_dev, port_num,
 				     component_reg_phys, CXL_RESOURCE_NONE);
 	if (IS_ERR(dport)) {
 		dev_dbg(dport_dev, "failed to add dport to %s: %ld\n",
@@ -1455,7 +1488,7 @@ static void reap_dports(struct cxl_port *port)
 	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);
+		devm_release_action(&port->dev, free_dport, dport);
 	}
 }
 
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index a9ab46eb0610..f4fe523aaf12 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -583,6 +583,7 @@ struct cxl_dax_region {
  * @regions: cxl_region_ref instances, regions mapped by this port
  * @parent_dport: dport that points to this port in the parent
  * @decoder_ida: allocator for decoder ids
+ * @dport_ida: allocator for dport ids
  * @reg_map: component and ras register mapping parameters
  * @nr_dports: number of entries in @dports
  * @hdm_end: track last allocated HDM decoder instance for allocation ordering
@@ -603,6 +604,7 @@ struct cxl_port {
 	struct xarray regions;
 	struct cxl_dport *parent_dport;
 	struct ida decoder_ida;
+	struct ida dport_ida;
 	struct cxl_register_map reg_map;
 	int nr_dports;
 	int hdm_end;
@@ -655,7 +657,8 @@ struct cxl_rcrb_info {
  * struct cxl_dport - CXL downstream port
  * @dport_dev: PCI bridge or firmware device representing the downstream link
  * @reg_map: component and ras register mapping parameters
- * @port_id: unique hardware identifier for dport in decoder target list
+ * @id: Linux id to enumerate dport instances per port
+ * @port_num: unique hardware identifier for dport in decoder target list
  * @rcrb: Data about the Root Complex Register Block layout
  * @rch: Indicate whether this dport was enumerated in RCH or VH mode
  * @port: reference to cxl_port that contains this downstream port
@@ -667,7 +670,8 @@ struct cxl_rcrb_info {
 struct cxl_dport {
 	struct device *dport_dev;
 	struct cxl_register_map reg_map;
-	int port_id;
+	int id;
+	int port_num;
 	struct cxl_rcrb_info rcrb;
 	bool rch;
 	struct cxl_port *port;
@@ -750,10 +754,10 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
 bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);
 
 struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
-				     struct device *dport, int port_id,
+				     struct device *dport, int port_num,
 				     resource_size_t component_reg_phys);
 struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
-					 struct device *dport_dev, int port_id,
+					 struct device *dport_dev, int port_num,
 					 resource_size_t rcrb);
 
 #ifdef CONFIG_PCIEAER_CXL
-- 
2.49.0

[PATCH v2 03/10] cxl: Rename find_dport() to provide better function intent

[Dave Jiang]

Rename find_dport() to find_dport_by_num() to indicate that the function
is trying to match a dport by its hardware number index.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/port.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index e9c02e4d0d4c..1d7a4a2ef6ad 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1045,7 +1045,7 @@ void put_cxl_root(struct cxl_root *cxl_root)
 }
 EXPORT_SYMBOL_NS_GPL(put_cxl_root, "CXL");
 
-static struct cxl_dport *find_dport(struct cxl_port *port, int port_num)
+static struct cxl_dport *find_dport_by_num(struct cxl_port *port, int port_num)
 {
 	struct cxl_dport *dport;
 	unsigned long index;
@@ -1063,7 +1063,7 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *dport)
 	int rc;
 
 	device_lock_assert(&port->dev);
-	dup = find_dport(port, dport->port_num);
+	dup = find_dport_by_num(port, dport->port_num);
 	if (dup) {
 		dev_err(&port->dev,
 			"unable to add dport%d-%s non-unique port num (%s)\n",
@@ -1275,13 +1275,13 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, "CXL");
  * devm_cxl_add_rch_dport - append RCH downstream port data to a cxl_port
  * @port: the cxl_port that references this dport
  * @dport_dev: firmware or PCI device representing the dport
- * @port_id: identifier for this dport in a decoder's target list
+ * @port_num: identifier for this dport in a decoder's target list
  * @rcrb: mandatory location of a Root Complex Register Block
  *
  * See CXL 3.0 9.11.8 CXL Devices Attached to an RCH
  */
 struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
-					 struct device *dport_dev, int port_id,
+					 struct device *dport_dev, int port_num,
 					 resource_size_t rcrb)
 {
 	struct cxl_dport *dport;
@@ -1291,7 +1291,7 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
 		return ERR_PTR(-EINVAL);
 	}
 
-	dport = __devm_cxl_add_dport(port, dport_dev, port_id,
+	dport = __devm_cxl_add_dport(port, dport_dev, port_num,
 				     CXL_RESOURCE_NONE, rcrb);
 	if (IS_ERR(dport)) {
 		dev_dbg(dport_dev, "failed to add RCH dport to %s: %ld\n",
@@ -1764,7 +1764,7 @@ static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
 
 	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]);
+		struct cxl_dport *dport = find_dport_by_num(port, target_map[i]);
 
 		if (!dport)
 			return -ENXIO;
-- 
2.49.0

[PATCH v2 04/10] cxl: Remove adding of port_num via devm_cxl_add_dport()

[Dave Jiang]

In preparation for delayed dport probing, remove setting of port_num
through devm_cxl_add_dport(). Will temporarily set the port_num after
dport is added for now.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/acpi.c            |  7 ++++---
 drivers/cxl/core/pci.c        |  4 +++-
 drivers/cxl/core/port.c       | 18 +++++++-----------
 drivers/cxl/cxl.h             |  5 +++--
 tools/testing/cxl/test/cxl.c  |  5 +++--
 tools/testing/cxl/test/mock.c |  5 ++---
 6 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index cb14829bb9be..6f8630e50800 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -593,16 +593,17 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 	if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) {
 		dev_dbg(match, "RCRB found for UID %lld: %pa\n", ctx.uid,
 			&ctx.base);
-		dport = devm_cxl_add_rch_dport(root_port, bridge, ctx.uid,
-					       ctx.base);
+		dport = devm_cxl_add_rch_dport(root_port, bridge, ctx.base);
 	} else {
-		dport = devm_cxl_add_dport(root_port, bridge, ctx.uid,
+		dport = devm_cxl_add_dport(root_port, bridge,
 					   CXL_RESOURCE_NONE);
 	}
 
 	if (IS_ERR(dport))
 		return PTR_ERR(dport);
 
+	dport->port_num = ctx.uid;
+
 	ret = get_genport_coordinates(match, dport);
 	if (ret)
 		dev_dbg(match, "Failed to get generic port perf coordinates.\n");
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 3b80e9a76ba8..3b84b43ab194 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -57,11 +57,13 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
 		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);
+	dport = devm_cxl_add_dport(port, &pdev->dev, map.resource);
 	if (IS_ERR(dport)) {
 		ctx->error = PTR_ERR(dport);
 		return PTR_ERR(dport);
 	}
+
+	dport->port_num = port_num;
 	ctx->count++;
 
 	return 0;
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 1d7a4a2ef6ad..70173d23139c 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1145,14 +1145,14 @@ static struct cxl_dport *cxl_alloc_dport(struct cxl_port *port,
 	dport->dport_dev = dport_dev;
 	dport->port = port;
 	dport->id = id;
+	dport->port_num = CXL_DPORT_NUM_INVALID;
 
 	return no_free_ptr(dport);
 }
 
 static struct cxl_dport *
 __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
-		     int port_num, resource_size_t component_reg_phys,
-		     resource_size_t rcrb)
+		     resource_size_t component_reg_phys, resource_size_t rcrb)
 {
 	char link_name[CXL_TARGET_STRLEN];
 	struct cxl_dport *dport;
@@ -1182,8 +1182,6 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
 	    CXL_TARGET_STRLEN)
 		return ERR_PTR(-EINVAL);
 
-	dport->port_num = port_num;
-
 	if (rcrb == CXL_RESOURCE_NONE) {
 		rc = cxl_dport_setup_regs(&port->dev, dport,
 					  component_reg_phys);
@@ -1244,7 +1242,6 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
  * devm_cxl_add_dport - append VH downstream port data to a cxl_port
  * @port: the cxl_port that references this dport
  * @dport_dev: firmware or PCI device representing the dport
- * @port_num: identifier for this dport in a decoder's target list
  * @component_reg_phys: optional location of CXL component registers
  *
  * Note that dports are appended to the devm release action's of the
@@ -1252,13 +1249,13 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
  * switch ports)
  */
 struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
-				     struct device *dport_dev, int port_num,
+				     struct device *dport_dev,
 				     resource_size_t component_reg_phys)
 {
 	struct cxl_dport *dport;
 
-	dport = __devm_cxl_add_dport(port, dport_dev, port_num,
-				     component_reg_phys, CXL_RESOURCE_NONE);
+	dport = __devm_cxl_add_dport(port, dport_dev, component_reg_phys,
+				     CXL_RESOURCE_NONE);
 	if (IS_ERR(dport)) {
 		dev_dbg(dport_dev, "failed to add dport to %s: %ld\n",
 			dev_name(&port->dev), PTR_ERR(dport));
@@ -1275,13 +1272,12 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, "CXL");
  * devm_cxl_add_rch_dport - append RCH downstream port data to a cxl_port
  * @port: the cxl_port that references this dport
  * @dport_dev: firmware or PCI device representing the dport
- * @port_num: identifier for this dport in a decoder's target list
  * @rcrb: mandatory location of a Root Complex Register Block
  *
  * See CXL 3.0 9.11.8 CXL Devices Attached to an RCH
  */
 struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
-					 struct device *dport_dev, int port_num,
+					 struct device *dport_dev,
 					 resource_size_t rcrb)
 {
 	struct cxl_dport *dport;
@@ -1291,7 +1287,7 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
 		return ERR_PTR(-EINVAL);
 	}
 
-	dport = __devm_cxl_add_dport(port, dport_dev, port_num,
+	dport = __devm_cxl_add_dport(port, dport_dev,
 				     CXL_RESOURCE_NONE, rcrb);
 	if (IS_ERR(dport)) {
 		dev_dbg(dport_dev, "failed to add RCH dport to %s: %ld\n",
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index f4fe523aaf12..4ba3bbe9600b 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -345,6 +345,7 @@ enum cxl_decoder_type {
 #define CXL_DECODER_MAX_INTERLEAVE 16
 
 #define CXL_QOS_CLASS_INVALID -1
+#define CXL_DPORT_NUM_INVALID -1
 
 /**
  * struct cxl_decoder - Common CXL HDM Decoder Attributes
@@ -754,10 +755,10 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
 bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);
 
 struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
-				     struct device *dport, int port_num,
+				     struct device *dport,
 				     resource_size_t component_reg_phys);
 struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
-					 struct device *dport_dev, int port_num,
+					 struct device *dport_dev,
 					 resource_size_t rcrb);
 
 #ifdef CONFIG_PCIEAER_CXL
diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index eb36408d287a..4846082127c4 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -968,11 +968,12 @@ static int mock_cxl_port_enumerate_dports(struct cxl_port *port)
 			continue;
 		}
 
-		dport = devm_cxl_add_dport(port, &pdev->dev, pdev->id,
-					   CXL_RESOURCE_NONE);
+		dport = devm_cxl_add_dport(port, &pdev->dev, CXL_RESOURCE_NONE);
 
 		if (IS_ERR(dport))
 			return PTR_ERR(dport);
+
+		dport->port_num = pdev->id;
 	}
 
 	return 0;
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index 6147f0966ffd..d9e8940d9328 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -246,7 +246,6 @@ EXPORT_SYMBOL_NS_GPL(__wrap_cxl_dvsec_rr_decode, "CXL");
 
 struct cxl_dport *__wrap_devm_cxl_add_rch_dport(struct cxl_port *port,
 						struct device *dport_dev,
-						int port_id,
 						resource_size_t rcrb)
 {
 	int index;
@@ -254,14 +253,14 @@ struct cxl_dport *__wrap_devm_cxl_add_rch_dport(struct cxl_port *port,
 	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
 
 	if (ops && ops->is_mock_port(dport_dev)) {
-		dport = devm_cxl_add_dport(port, dport_dev, port_id,
+		dport = devm_cxl_add_dport(port, dport_dev,
 					   CXL_RESOURCE_NONE);
 		if (!IS_ERR(dport)) {
 			dport->rcrb.base = rcrb;
 			dport->rch = true;
 		}
 	} else
-		dport = devm_cxl_add_rch_dport(port, dport_dev, port_id, rcrb);
+		dport = devm_cxl_add_rch_dport(port, dport_dev, rcrb);
 	put_cxl_mock_ops(index);
 
 	return dport;
-- 
2.49.0

[PATCH v2 05/10] cxl: Defer hardware dport->port_id assignment and registers probing

[Dave Jiang]

Current implementation only enumerates the dports during the port 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 hw
dport id assignment and the register probing to behind memdev probe so the
endpoint is guaranteed to be connected.

The detection of duplicate dport for add_dport() is removed. The port_id
is not read from the hw at this point any longer. The port->id will always
be unique since it's retrieved from an ida. The dup detection thus become
irrelevant.

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.

Link: https://lore.kernel.org/linux-cxl/20250305100123.3077031-1-rrichter@amd.com/
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v2:
- Fix spelling error in commit message. (Ming)
- Move probing to the specific dport. (Ming)
- Add decoder update.
- Return errors when setup dports. (Jonathan)
- Add driver check before setup dport. (Dan)
- Add comment on why location to do dport update. (Dan)
---
 drivers/cxl/core/core.h |   4 ++
 drivers/cxl/core/pci.c  |  60 ++++++++++++----
 drivers/cxl/core/port.c | 152 +++++++++++++++++++++++++++++++++++-----
 drivers/cxl/cxl.h       |   4 ++
 drivers/cxl/port.c      |  20 +-----
 5 files changed, 192 insertions(+), 48 deletions(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 17b692eb3257..8b1d9a8ab852 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -134,4 +134,8 @@ int cxl_set_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
 		    u16 *return_code);
 #endif
 
+int cxl_dport_setup(struct cxl_dport *dport);
+int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport,
+			 resource_size_t component_reg_phys);
+
 #endif /* __CXL_CORE_H__ */
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 3b84b43ab194..16bd842904e5 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -24,6 +24,52 @@ 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");
 
+int cxl_dport_setup(struct cxl_dport *dport)
+{
+	struct device *dport_dev = dport->dport_dev;
+	struct cxl_port *port = dport->port;
+	struct cxl_register_map map;
+	struct pci_dev *pdev;
+	u32 lnkcap, port_num;
+	int rc;
+
+	if (!dev_is_pci(dport_dev))
+		return 0;
+
+	/*
+	 * dport->port_id is valid means that dport has been probed and is
+	 * setup.
+	 */
+	if (dport->port_num != CXL_DPORT_NUM_INVALID)
+		return 0;
+
+	pdev = to_pci_dev(dport_dev);
+	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
+				  &lnkcap))
+		return -ENODEV;
+
+	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
+	if (rc) {
+		dev_dbg(&port->dev, "failed to find component registers\n");
+		return -ENODEV;
+	}
+
+	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
+	rc = cxl_dport_setup_regs(&port->dev, dport, map.resource);
+	if (rc)
+		return rc;
+
+	if (map.resource != CXL_RESOURCE_NONE) {
+		dev_dbg(dport->dport_dev,
+			"Component Register found for dport: %pa\n",
+			&map.resource);
+	}
+
+	dport->port_num = port_num;
+
+	return 0;
+}
+
 struct cxl_walk_context {
 	struct pci_bus *bus;
 	struct cxl_port *port;
@@ -37,10 +83,7 @@ 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;
@@ -48,22 +91,13 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
 		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, map.resource);
+	dport = devm_cxl_add_dport(port, &pdev->dev, CXL_RESOURCE_NONE);
 	if (IS_ERR(dport)) {
 		ctx->error = PTR_ERR(dport);
 		return PTR_ERR(dport);
 	}
 
-	dport->port_num = port_num;
 	ctx->count++;
 
 	return 0;
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 70173d23139c..04e18a102d26 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -782,8 +782,8 @@ static int cxl_port_setup_regs(struct cxl_port *port,
 				   component_reg_phys);
 }
 
-static int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport,
-				resource_size_t component_reg_phys)
+int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport,
+			 resource_size_t component_reg_phys)
 {
 	int rc;
 
@@ -1004,7 +1004,7 @@ int devm_cxl_register_pci_bus(struct device *host, struct device *uport_dev,
 }
 EXPORT_SYMBOL_NS_GPL(devm_cxl_register_pci_bus, "CXL");
 
-static bool dev_is_cxl_root_child(struct device *dev)
+bool dev_is_cxl_root_child(struct device *dev)
 {
 	struct cxl_port *port, *parent;
 
@@ -1021,6 +1021,7 @@ static bool dev_is_cxl_root_child(struct device *dev)
 
 	return false;
 }
+EXPORT_SYMBOL_NS_GPL(dev_is_cxl_root_child, "CXL");
 
 struct cxl_root *find_cxl_root(struct cxl_port *port)
 {
@@ -1059,19 +1060,9 @@ static struct cxl_dport *find_dport_by_num(struct cxl_port *port, int port_num)
 
 static int add_dport(struct cxl_port *port, struct cxl_dport *dport)
 {
-	struct cxl_dport *dup;
 	int rc;
 
 	device_lock_assert(&port->dev);
-	dup = find_dport_by_num(port, dport->port_num);
-	if (dup) {
-		dev_err(&port->dev,
-			"unable to add dport%d-%s non-unique port num (%s)\n",
-			dport->port_num, dev_name(dport->dport_dev),
-			dev_name(dup->dport_dev));
-		return -EBUSY;
-	}
-
 	rc = xa_insert(&port->dports, (unsigned long)dport->dport_dev, dport,
 		       GFP_KERNEL);
 	if (rc)
@@ -1643,6 +1634,118 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 	return rc;
 }
 
+static int port_probe_dport(struct cxl_port *port, struct device *dport_dev,
+			    struct cxl_dport **probed_dport)
+{
+	struct cxl_dport *dport;
+	unsigned long index;
+	int rc;
+
+	device_lock_assert(&port->dev);
+	xa_for_each(&port->dports, index, dport) {
+		if (dport->dport_dev == dport_dev) {
+			rc = cxl_dport_setup(dport);
+			if (rc)
+				return rc;
+			*probed_dport = dport;
+			return 0;
+		}
+	}
+
+	*probed_dport = NULL;
+
+	return -ENODEV;
+}
+
+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_num) {
+			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);
+}
+
+int devm_cxl_port_setup_decoders(struct cxl_port *port)
+{
+	struct cxl_dport *dport;
+	struct cxl_hdm *cxlhdm;
+	unsigned long index;
+	int dports = 0;
+
+	cxlhdm = devm_cxl_setup_hdm(port, NULL);
+	if (!IS_ERR(cxlhdm))
+		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
+
+	if (PTR_ERR(cxlhdm) != -ENODEV) {
+		dev_err(&port->dev, "Failed to map HDM decoder capability\n");
+		return PTR_ERR(cxlhdm);
+	}
+
+	xa_for_each(&port->dports, index, dport)
+		dports++;
+
+	if (dports == 1) {
+		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
+		return devm_cxl_add_passthrough_decoder(port);
+	}
+
+	dev_err(&port->dev, "HDM decoder capability not found\n");
+	return -ENXIO;
+}
+EXPORT_SYMBOL_NS_GPL(devm_cxl_port_setup_decoders, "CXL");
+
+static int cxl_switch_port_dport_setup(struct cxl_port *port,
+				       struct device *dport_dev)
+{
+	struct cxl_dport *dport;
+	int rc;
+
+	device_lock_assert(&port->dev);
+
+	if (!port->dev.driver)
+		return -ENODEV;
+
+	rc = port_probe_dport(port, dport_dev, &dport);
+	if (rc)
+		return rc;
+
+	cxl_switch_parse_cdat(port);
+
+	/* Make sure that no decoders have been allocated before proceeding. */
+	if (ida_is_empty(&port->decoder_ida))
+		rc = devm_cxl_port_setup_decoders(port);
+	else
+		rc = update_decoders_with_dport(port, dport);
+	if (rc)
+		return rc;
+
+	return 0;
+}
+
 int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 {
 	struct device *dev = &cxlmd->dev;
@@ -1695,6 +1798,19 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 				"found already registered port %s:%s\n",
 				dev_name(&port->dev),
 				dev_name(port->uport_dev));
+
+			/*
+			 * Attempt to do single pass dport setup by checking here
+			 * instead of doing it during port creation. Otherwise
+			 * it still needs to check here for dports that are
+			 * being probed with a port already created.
+			 */
+			scoped_guard(device, &port->dev) {
+				rc = cxl_switch_port_dport_setup(port, dport_dev);
+				if (rc)
+					return rc;
+			}
+
 			rc = cxl_add_ep(dport, &cxlmd->dev);
 
 			/*
@@ -1760,10 +1876,14 @@ static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
 
 	guard(rwsem_write)(&cxl_region_rwsem);
 	for (i = 0; i < cxlsd->cxld.interleave_ways; i++) {
-		struct cxl_dport *dport = find_dport_by_num(port, target_map[i]);
+		struct cxl_dport *dport;
 
-		if (!dport)
-			return -ENXIO;
+		cxlsd->target_map[i] = target_map[i];
+		dport = find_dport_by_num(port, target_map[i]);
+		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 4ba3bbe9600b..6b85d5f23be0 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -404,6 +404,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: array of expected dport port_id mirror the target
  * @target: active ordered target list in current decoder configuration
  *
  * The 'switch' decoder type represents the decoder instances of cxl_port's that
@@ -415,6 +416,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[];
 };
 
@@ -906,6 +908,8 @@ void cxl_coordinates_combine(struct access_coordinate *out,
 			     struct access_coordinate *c2);
 
 bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port);
+int devm_cxl_port_setup_decoders(struct cxl_port *port);
+bool dev_is_cxl_root_child(struct device *dev);
 
 /*
  * Unit test builds overrides this to __weak, find the 'strong' version
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index a35fc5552845..4d840a6ef802 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -59,7 +59,6 @@ static int discover_region(struct device *dev, void *root)
 
 static int cxl_switch_port_probe(struct cxl_port *port)
 {
-	struct cxl_hdm *cxlhdm;
 	int rc;
 
 	/* Cache the data early to ensure is_visible() works */
@@ -69,24 +68,7 @@ static int cxl_switch_port_probe(struct cxl_port *port)
 	if (rc < 0)
 		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);
-
-	if (PTR_ERR(cxlhdm) != -ENODEV) {
-		dev_err(&port->dev, "Failed to map HDM decoder capability\n");
-		return PTR_ERR(cxlhdm);
-	}
-
-	if (rc == 1) {
-		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
-		return devm_cxl_add_passthrough_decoder(port);
-	}
-
-	dev_err(&port->dev, "HDM decoder capability not found\n");
-	return -ENXIO;
+	return 0;
 }
 
 static int cxl_endpoint_port_probe(struct cxl_port *port)
-- 
2.49.0

[PATCH v2 06/10] cxl/test: Add workaround for cxl_test for cxl_core calling mocked functions

[Dave Jiang]

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 host bridge uport mapping update changes in order to enable
cxl-test.

The following functions are being modified:
devm_cxl_add_passthrough_decoder()
devm_cxl_setup_hdm()
devm_cxl_enumerate_decoders()

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.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
- Reverse unregister order. (Jonathan)
- Move the bits only needed by cxl_test to cxl_test. (Dan)
- Add more comments in the commit log. (Jonathan)
---
 drivers/cxl/core/hdm.c               | 27 ++++++++++++---------
 drivers/cxl/cxl.h                    | 25 ++++++++++++++++++++
 tools/testing/cxl/Kbuild             |  4 +---
 tools/testing/cxl/cxl_core_exports.c | 31 ++++++++++++++++++++++++
 tools/testing/cxl/exports.h          | 17 ++++++++++++++
 tools/testing/cxl/test/mock.c        | 35 +++++++++++++++++++---------
 6 files changed, 114 insertions(+), 25 deletions(-)
 create mode 100644 tools/testing/cxl/exports.h

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index d7ad92e191ba..76cb6cf22c62 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -39,14 +39,19 @@ static int add_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 	return 0;
 }
 
-/*
+/**
+ * __devm_cxl_add_passthrough_decoder - Add passthrough decoder
+ * @port: The cxl_port context
+ *
+ * Return 0 on success or errno on failure.
+ *
  * Per the CXL specification (8.2.5.12 CXL HDM Decoder Capability Structure)
  * single ported host-bridges need not publish a decoder capability when a
  * passthrough decode can be assumed, i.e. all transactions that the uport sees
  * are claimed and passed to the single dport. Disable the range until the first
  * CXL region is enumerated / activated.
  */
-int devm_cxl_add_passthrough_decoder(struct cxl_port *port)
+int __devm_cxl_add_passthrough_decoder(struct cxl_port *port)
 {
 	struct cxl_switch_decoder *cxlsd;
 	struct cxl_dport *dport = NULL;
@@ -73,7 +78,7 @@ int devm_cxl_add_passthrough_decoder(struct cxl_port *port)
 
 	return add_hdm_decoder(port, &cxlsd->cxld, single_port_map);
 }
-EXPORT_SYMBOL_NS_GPL(devm_cxl_add_passthrough_decoder, "CXL");
+EXPORT_SYMBOL_NS_GPL(__devm_cxl_add_passthrough_decoder, "CXL");
 
 static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
 {
@@ -139,12 +144,12 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
 }
 
 /**
- * devm_cxl_setup_hdm - map HDM decoder component registers
+ * __devm_cxl_setup_hdm - map HDM decoder component registers
  * @port: cxl_port to map
  * @info: cached DVSEC range register info
  */
-struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
-				   struct cxl_endpoint_dvsec_info *info)
+struct cxl_hdm *__devm_cxl_setup_hdm(struct cxl_port *port,
+				     struct cxl_endpoint_dvsec_info *info)
 {
 	struct cxl_register_map *reg_map = &port->reg_map;
 	struct device *dev = &port->dev;
@@ -199,7 +204,7 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
 
 	return cxlhdm;
 }
-EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_hdm, "CXL");
+EXPORT_SYMBOL_NS_GPL(__devm_cxl_setup_hdm, "CXL");
 
 static void __cxl_dpa_debug(struct seq_file *file, struct resource *r, int depth)
 {
@@ -1150,12 +1155,12 @@ static void cxl_settle_decoders(struct cxl_hdm *cxlhdm)
 }
 
 /**
- * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
+ * __devm_cxl_enumerate_decoders - add decoder objects per HDM register set
  * @cxlhdm: Structure to populate with HDM capabilities
  * @info: cached DVSEC range register info
  */
-int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
-				struct cxl_endpoint_dvsec_info *info)
+int __devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
+				  struct cxl_endpoint_dvsec_info *info)
 {
 	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
 	struct cxl_port *port = cxlhdm->port;
@@ -1212,4 +1217,4 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 
 	return 0;
 }
-EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_decoders, "CXL");
+EXPORT_SYMBOL_NS_GPL(__devm_cxl_enumerate_decoders, "CXL");
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 6b85d5f23be0..4d0ddb506ec5 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -810,9 +810,17 @@ struct cxl_endpoint_dvsec_info {
 struct cxl_hdm;
 struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
 				   struct cxl_endpoint_dvsec_info *info);
+struct cxl_hdm *__devm_cxl_setup_hdm(struct cxl_port *port,
+				     struct cxl_endpoint_dvsec_info *info);
+
 int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 				struct cxl_endpoint_dvsec_info *info);
+int __devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
+				  struct cxl_endpoint_dvsec_info *info);
+
 int devm_cxl_add_passthrough_decoder(struct cxl_port *port);
+int __devm_cxl_add_passthrough_decoder(struct cxl_port *port);
+
 struct cxl_dev_state;
 int cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds,
 			struct cxl_endpoint_dvsec_info *info);
@@ -921,4 +929,21 @@ bool dev_is_cxl_root_child(struct device *dev);
 
 u16 cxl_gpf_get_dvsec(struct device *dev);
 
+/*
+ * 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_enumerate_decoders DECLARE_TESTABLE(devm_cxl_enumerate_decoders)
+#define devm_cxl_setup_hdm DECLARE_TESTABLE(devm_cxl_setup_hdm)
+#define devm_cxl_add_passthrough_decoder DECLARE_TESTABLE(devm_cxl_add_passthrough_decoder)
+#endif
+
 #endif /* __CXL_H__ */
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index 387f3df8b988..8bf786eef9f1 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -5,9 +5,6 @@ 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=devm_cxl_setup_hdm
-ldflags-y += --wrap=devm_cxl_add_passthrough_decoder
-ldflags-y += --wrap=devm_cxl_enumerate_decoders
 ldflags-y += --wrap=cxl_await_media_ready
 ldflags-y += --wrap=cxl_hdm_decode_init
 ldflags-y += --wrap=cxl_dvsec_rr_decode
@@ -21,6 +18,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..9a5f2e641897 100644
--- a/tools/testing/cxl/cxl_core_exports.c
+++ b/tools/testing/cxl/cxl_core_exports.c
@@ -2,6 +2,37 @@
 /* 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_pt_decoder_fn _devm_cxl_add_passthrough_decoder =
+	__devm_cxl_add_passthrough_decoder;
+EXPORT_SYMBOL_NS_GPL(_devm_cxl_add_passthrough_decoder, "CXL");
+
+int devm_cxl_add_passthrough_decoder(struct cxl_port *port)
+{
+	return _devm_cxl_add_passthrough_decoder(port);
+}
+EXPORT_SYMBOL_NS_GPL(devm_cxl_add_passthrough_decoder, "CXL");
+
+cxl_setup_hdm_fn _devm_cxl_setup_hdm = __devm_cxl_setup_hdm;
+EXPORT_SYMBOL_NS_GPL(_devm_cxl_setup_hdm, "CXL");
+
+struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
+				   struct cxl_endpoint_dvsec_info *info)
+{
+	return _devm_cxl_setup_hdm(port, info);
+}
+EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_hdm, "CXL");
+
+cxl_enum_decoders_fn _devm_cxl_enumerate_decoders = __devm_cxl_enumerate_decoders;
+EXPORT_SYMBOL_NS_GPL(_devm_cxl_enumerate_decoders, "CXL");
+
+int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
+				struct cxl_endpoint_dvsec_info *info)
+{
+	return _devm_cxl_enumerate_decoders(cxlhdm, info);
+}
+EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_decoders, "CXL");
diff --git a/tools/testing/cxl/exports.h b/tools/testing/cxl/exports.h
new file mode 100644
index 000000000000..036fe85e1ad9
--- /dev/null
+++ b/tools/testing/cxl/exports.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2025 Intel Corporation */
+#ifndef __MOCK_CXL_EXPORTS_H_
+#define __MOCK_CXL_EXPORTS_H_
+
+typedef struct cxl_hdm *(*cxl_setup_hdm_fn)(struct cxl_port *port,
+					    struct cxl_endpoint_dvsec_info *info);
+extern cxl_setup_hdm_fn _devm_cxl_setup_hdm;
+
+typedef int (*cxl_enum_decoders_fn)(struct cxl_hdm *cxlhdm,
+				    struct cxl_endpoint_dvsec_info *info);
+extern cxl_enum_decoders_fn _devm_cxl_enumerate_decoders;
+
+typedef int (*cxl_add_pt_decoder_fn)(struct cxl_port *port);
+extern cxl_add_pt_decoder_fn _devm_cxl_add_passthrough_decoder;
+
+#endif
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index d9e8940d9328..c4367c4b5278 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -10,12 +10,25 @@
 #include <cxlmem.h>
 #include <cxlpci.h>
 #include "mock.h"
+#include "../exports.h"
 
 static LIST_HEAD(mock);
 
+static struct cxl_hdm *
+redirect_devm_cxl_setup_hdm(struct cxl_port *port,
+			    struct cxl_endpoint_dvsec_info *info);
+static int
+redirect_devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
+				     struct cxl_endpoint_dvsec_info *info);
+static int redirect_devm_cxl_add_passthrough_decoder(struct cxl_port *port);
+
 void register_cxl_mock_ops(struct cxl_mock_ops *ops)
 {
 	list_add_rcu(&ops->list, &mock);
+	_devm_cxl_setup_hdm = redirect_devm_cxl_setup_hdm;
+	_devm_cxl_enumerate_decoders = redirect_devm_cxl_enumerate_decoders;
+	_devm_cxl_add_passthrough_decoder =
+		redirect_devm_cxl_add_passthrough_decoder;
 }
 EXPORT_SYMBOL_GPL(register_cxl_mock_ops);
 
@@ -23,6 +36,9 @@ DEFINE_STATIC_SRCU(cxl_mock_srcu);
 
 void unregister_cxl_mock_ops(struct cxl_mock_ops *ops)
 {
+	_devm_cxl_add_passthrough_decoder = __devm_cxl_add_passthrough_decoder;
+	_devm_cxl_enumerate_decoders = __devm_cxl_enumerate_decoders;
+	_devm_cxl_setup_hdm = __devm_cxl_setup_hdm;
 	list_del_rcu(&ops->list);
 	synchronize_srcu(&cxl_mock_srcu);
 }
@@ -131,8 +147,8 @@ __wrap_nvdimm_bus_register(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(__wrap_nvdimm_bus_register);
 
-struct cxl_hdm *__wrap_devm_cxl_setup_hdm(struct cxl_port *port,
-					  struct cxl_endpoint_dvsec_info *info)
+struct cxl_hdm *redirect_devm_cxl_setup_hdm(struct cxl_port *port,
+					    struct cxl_endpoint_dvsec_info *info)
 
 {
 	int index;
@@ -142,14 +158,13 @@ struct cxl_hdm *__wrap_devm_cxl_setup_hdm(struct cxl_port *port,
 	if (ops && ops->is_mock_port(port->uport_dev))
 		cxlhdm = ops->devm_cxl_setup_hdm(port, info);
 	else
-		cxlhdm = devm_cxl_setup_hdm(port, info);
+		cxlhdm = __devm_cxl_setup_hdm(port, info);
 	put_cxl_mock_ops(index);
 
 	return cxlhdm;
 }
-EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_setup_hdm, "CXL");
 
-int __wrap_devm_cxl_add_passthrough_decoder(struct cxl_port *port)
+int redirect_devm_cxl_add_passthrough_decoder(struct cxl_port *port)
 {
 	int rc, index;
 	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
@@ -157,15 +172,14 @@ int __wrap_devm_cxl_add_passthrough_decoder(struct cxl_port *port)
 	if (ops && ops->is_mock_port(port->uport_dev))
 		rc = ops->devm_cxl_add_passthrough_decoder(port);
 	else
-		rc = devm_cxl_add_passthrough_decoder(port);
+		rc = __devm_cxl_add_passthrough_decoder(port);
 	put_cxl_mock_ops(index);
 
 	return rc;
 }
-EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_add_passthrough_decoder, "CXL");
 
-int __wrap_devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
-				       struct cxl_endpoint_dvsec_info *info)
+int redirect_devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
+					 struct cxl_endpoint_dvsec_info *info)
 {
 	int rc, index;
 	struct cxl_port *port = cxlhdm->port;
@@ -174,12 +188,11 @@ int __wrap_devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 	if (ops && ops->is_mock_port(port->uport_dev))
 		rc = ops->devm_cxl_enumerate_decoders(cxlhdm, info);
 	else
-		rc = devm_cxl_enumerate_decoders(cxlhdm, info);
+		rc = __devm_cxl_enumerate_decoders(cxlhdm, info);
 	put_cxl_mock_ops(index);
 
 	return rc;
 }
-EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_enumerate_decoders, "CXL");
 
 int __wrap_devm_cxl_port_enumerate_dports(struct cxl_port *port)
 {
-- 
2.49.0

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

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 f637e3631d88..f750a3afa8f3 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_num) {
-				cxl_access_coordinate_set(dport->coord,
-							  sslbis->data_type,
-							  val);
-			}
+		if (dsp_id == ACPI_CDAT_SSLBIS_ANY_PORT ||
+		    dsp_id == dport->port_num) {
+			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 04e18a102d26..e212bc2faada 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1733,7 +1733,7 @@ static int cxl_switch_port_dport_setup(struct cxl_port *port,
 	if (rc)
 		return rc;
 
-	cxl_switch_parse_cdat(port);
+	cxl_switch_parse_cdat(dport);
 
 	/* Make sure that no decoders have been allocated before proceeding. */
 	if (ida_is_empty(&port->decoder_ida))
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 4d0ddb506ec5..22f1a9542077 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -901,7 +901,7 @@ static inline u64 cxl_port_get_spa_cache_alias(struct cxl_port *endpoint,
 #endif
 
 void cxl_endpoint_parse_cdat(struct cxl_port *port);
-void cxl_switch_parse_cdat(struct cxl_port *port);
+void cxl_switch_parse_cdat(struct cxl_dport *dport);
 
 int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 				      struct access_coordinate *coord);
-- 
2.49.0

[PATCH v2 08/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.

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 e212bc2faada..259b217e812f 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);
@@ -1774,11 +1783,7 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 		struct device *uport_dev;
 		struct cxl_dport *dport;
 
-		/*
-		 * The terminal "grandparent" in PCI is NULL and @platform_bus
-		 * for platform devices
-		 */
-		if (!dport_dev || dport_dev == &platform_bus)
+		if (is_cxl_hierarchy_head(dport_dev))
 			return 0;
 
 		uport_dev = dport_dev->parent;
-- 
2.49.0

[PATCH v2 09/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.

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 259b217e812f..a5a673d789f3 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
@@ -1013,6 +1014,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_udev_to_root - Retrieve cxl_root tied to the host bridge device
+ * @uport_dev: upstream port device that is the host bridge
+ *
+ * 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_udev_to_root(struct device *uport_dev)
+{
+	struct cxl_root *root;
+
+	root = xa_load(&cxl_root_ports, (unsigned long)uport_dev);
+	if (!root)
+		return NULL;
+
+	get_device(&root->port.dev);
+
+	return root;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_udev_to_root, "CXL");
+
+static void unregister_udev_root_ports(void *uport_dev)
+{
+	xa_erase(&cxl_root_ports, (unsigned long)uport_dev);
+}
+
+/**
+ * devm_cxl_register_udev_root_port - Tie a hostbridge device to a root port
+ * @host: device that hosts the memory for the xarray entries
+ * @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_udev_root_port(struct device *host,
+				     struct device *uport_dev,
+				     struct cxl_root *root)
+{
+	int rc;
+
+	rc = xa_insert(&cxl_root_ports, (unsigned long)uport_dev, root,
+		       GFP_KERNEL);
+	if (rc)
+		return rc;
+
+	return devm_add_action_or_reset(host, unregister_udev_root_ports,
+					uport_dev);
+}
+EXPORT_SYMBOL_NS_GPL(devm_cxl_register_udev_root_port, "CXL");
+
 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 22f1a9542077..e0cba91803cc 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -736,6 +736,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_udev_root_port(struct device *host,
+				     struct device *uport_dev,
+				     struct cxl_root *root);
+struct cxl_root *cxl_udev_to_root(struct device *uport_dev);
 struct cxl_port *devm_cxl_add_port(struct device *host,
 				   struct device *uport_dev,
 				   resource_size_t component_reg_phys,
-- 
2.49.0

[PATCH v2 10/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.

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

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/acpi.c      | 136 ++++++++++++++++++++++++----------------
 drivers/cxl/core/port.c |  58 +++++++++++++++++
 drivers/cxl/cxl.h       |   2 +
 3 files changed, 141 insertions(+), 55 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 6f8630e50800..1db4d308b4b7 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -298,8 +298,79 @@ 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 decode 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;
+
+	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)
@@ -460,16 +531,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)
 {
@@ -588,7 +649,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,
@@ -611,22 +672,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;
@@ -639,37 +692,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_udev_root_port(host, bridge,
+						to_cxl_root(root_port));
 }
 
 static int add_root_nvdimm_bridge(struct device *match, void *data)
@@ -843,6 +871,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)
@@ -871,12 +900,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 a5a673d789f3..bbecbb04b6be 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1808,6 +1808,60 @@ static int cxl_switch_port_dport_setup(struct cxl_port *port,
 	return 0;
 }
 
+static int get_hostbridge_port_devices(struct cxl_memdev *cxlmd,
+				       struct device **uport_dev,
+				       struct device **dport_dev)
+{
+	struct device *dev = &cxlmd->dev;
+	struct device *iter;
+
+	for (iter = dev; iter; iter = grandparent(iter)) {
+		struct device *ddev = grandparent(iter);
+		struct device *udev;
+
+		udev = ddev->parent;
+		if (is_cxl_hierarchy_head(udev->parent)) {
+			*uport_dev = udev;
+			*dport_dev = ddev;
+			return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+
+static int cxl_hostbridge_port_setup(struct cxl_memdev *cxlmd)
+{
+	struct device *uport_dev, *dport_dev;
+	struct cxl_dport *dport;
+	struct cxl_port *port;
+	int rc;
+
+	rc = get_hostbridge_port_devices(cxlmd, &uport_dev, &dport_dev);
+	if (rc)
+		return -ENODEV;
+
+	struct cxl_root *cxl_root __free(put_cxl_root) = cxl_udev_to_root(uport_dev);
+	if (!cxl_root)
+		return -ENODEV;
+
+	guard(device)(&cxl_root->port.dev);
+	port = find_cxl_port(dport_dev, &dport);
+	if (port) {
+		put_device(&port->dev);
+		return 0;
+	}
+
+	if (!cxl_root->ops || !cxl_root->ops->setup_hostbridge_uport)
+		return -EOPNOTSUPP;
+
+	rc = cxl_root->ops->setup_hostbridge_uport(cxl_root, uport_dev);
+	if (rc)
+		return rc;
+
+	return 0;
+}
+
 int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 {
 	struct device *dev = &cxlmd->dev;
@@ -1821,6 +1875,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 e0cba91803cc..e4017328b05f 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -643,6 +643,8 @@ struct cxl_root_ops {
 	int (*qos_class)(struct cxl_root *cxl_root,
 			 struct access_coordinate *coord, int entries,
 			 int *qos_class);
+	int (*setup_hostbridge_uport)(struct cxl_root *cxl_root,
+				      struct device *bridge_dev);
 };
 
 static inline struct cxl_dport *
-- 
2.49.0

Re: [PATCH v2 05/10] cxl: Defer hardware dport->port_id assignment and registers probing

[Li Ming]

On 5/7/2025 8:43 AM, Dave Jiang wrote:
> Current implementation only enumerates the dports during the port 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 hw
> dport id assignment and the register probing to behind memdev probe so the
> endpoint is guaranteed to be connected.
>
> The detection of duplicate dport for add_dport() is removed. The port_id
> is not read from the hw at this point any longer. The port->id will always
> be unique since it's retrieved from an ida. The dup detection thus become
> irrelevant.
>
> 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.
>
> Link: https://lore.kernel.org/linux-cxl/20250305100123.3077031-1-rrichter@amd.com/
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v2:
> - Fix spelling error in commit message. (Ming)
> - Move probing to the specific dport. (Ming)
> - Add decoder update.
> - Return errors when setup dports. (Jonathan)
> - Add driver check before setup dport. (Dan)
> - Add comment on why location to do dport update. (Dan)
> ---
>  drivers/cxl/core/core.h |   4 ++
>  drivers/cxl/core/pci.c  |  60 ++++++++++++----
>  drivers/cxl/core/port.c | 152 +++++++++++++++++++++++++++++++++++-----
>  drivers/cxl/cxl.h       |   4 ++
>  drivers/cxl/port.c      |  20 +-----
>  5 files changed, 192 insertions(+), 48 deletions(-)
>
[snip]
>  
> +static int port_probe_dport(struct cxl_port *port, struct device *dport_dev,
> +			    struct cxl_dport **probed_dport)
> +{
> +	struct cxl_dport *dport;
> +	unsigned long index;
> +	int rc;
> +
> +	device_lock_assert(&port->dev);
> +	xa_for_each(&port->dports, index, dport) {
> +		if (dport->dport_dev == dport_dev) {
> +			rc = cxl_dport_setup(dport);
> +			if (rc)
> +				return rc;
> +			*probed_dport = dport;
> +			return 0;
> +		}
> +	}
> +
> +	*probed_dport = NULL;
> +
> +	return -ENODEV;
> +}

feels like the parameter struct cxl_dport **probed_dport is not needed, the function can return the struct cxl_dport on success, return errno on failure.


> +
> +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_num) {
> +			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);
> +}
> +
> +int devm_cxl_port_setup_decoders(struct cxl_port *port)
> +{
> +	struct cxl_dport *dport;
> +	struct cxl_hdm *cxlhdm;
> +	unsigned long index;
> +	int dports = 0;
> +
> +	cxlhdm = devm_cxl_setup_hdm(port, NULL);
> +	if (!IS_ERR(cxlhdm))
> +		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
> +
> +	if (PTR_ERR(cxlhdm) != -ENODEV) {
> +		dev_err(&port->dev, "Failed to map HDM decoder capability\n");
> +		return PTR_ERR(cxlhdm);
> +	}
> +
> +	xa_for_each(&port->dports, index, dport)
> +		dports++;
> +
> +	if (dports == 1) {
> +		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
> +		return devm_cxl_add_passthrough_decoder(port);
> +	}
> +
> +	dev_err(&port->dev, "HDM decoder capability not found\n");
> +	return -ENXIO;
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_port_setup_decoders, "CXL");
> +
> +static int cxl_switch_port_dport_setup(struct cxl_port *port,
> +				       struct device *dport_dev)
> +{
> +	struct cxl_dport *dport;
> +	int rc;
> +
> +	device_lock_assert(&port->dev);
> +
> +	if (!port->dev.driver)
> +		return -ENODEV;
> +
> +	rc = port_probe_dport(port, dport_dev, &dport);
> +	if (rc)
> +		return rc;
> +
> +	cxl_switch_parse_cdat(port);
> +
> +	/* Make sure that no decoders have been allocated before proceeding. */
> +	if (ida_is_empty(&port->decoder_ida))
> +		rc = devm_cxl_port_setup_decoders(port);
> +	else
> +		rc = update_decoders_with_dport(port, dport);
> +	if (rc)
> +		return rc;
> +
> +	return 0;

Can return rc directly here, no need to check rc before return 0.


Ming

[snip]

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

[Alison Schofield]

On Tue, May 06, 2025 at 05:43:01PM -0700, 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.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

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

> 

Re: [PATCH v2 02/10] cxl: Saperate out CXL dport->id vs actual dport hardware id

[Alison Schofield]

On Tue, May 06, 2025 at 05:43:02PM -0700, Dave Jiang wrote:

s/saperate/separate

> In preparation to allow dport to be allocated without being active, make
> dport->id to be Linux id that enumerates the dport objects per port.
> Keep the hardware id under dport->port_num to maintain compatibility and
> introduce a dport->id as the enumeration id.

I think the 'and introduce...' end of the second sentence is repeating
what the first sentence declared an can be omitted.

With that-

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

Re: [PATCH v2 02/10] cxl: Saperate out CXL dport->id vs actual dport hardware id

[Li Ming]

On 5/7/2025 8:43 AM, Dave Jiang wrote:
> In preparation to allow dport to be allocated without being active, make
> dport->id to be Linux id that enumerates the dport objects per port.
> Keep the hardware id under dport->port_num to maintain compatibility and
> introduce a dport->id as the enumeration id.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v2:
> - Rename port_id to port_num. (Dan)
> - Use a dport allocator to deal with ida allocation. (Dan, Jonathan)
> ---
>  drivers/cxl/core/cdat.c |  2 +-
>  drivers/cxl/core/hdm.c  | 18 +++++-----
>  drivers/cxl/core/port.c | 73 ++++++++++++++++++++++++++++++-----------
>  drivers/cxl/cxl.h       | 12 ++++---
>  4 files changed, 71 insertions(+), 34 deletions(-)
>
[snip]
>  static struct cxl_dport *
>  __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> -		     int port_id, resource_size_t component_reg_phys,
> +		     int port_num, resource_size_t component_reg_phys,
>  		     resource_size_t rcrb)
>  {
>  	char link_name[CXL_TARGET_STRLEN];
> @@ -1139,17 +1170,19 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>  		return ERR_PTR(-ENXIO);
>  	}
>  
> -	if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >=
> +	dport = cxl_alloc_dport(port, dport_dev);
> +	if (!dport)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rc = devm_add_action_or_reset(&port->dev, free_dport, dport);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", dport->id) >=
>  	    CXL_TARGET_STRLEN)
>  		return ERR_PTR(-EINVAL);
Is it better to call devm_release_action(&port->dev, free_dport, dport) here if snprintf() failed?

>  
> -	dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
> -	if (!dport)
> -		return ERR_PTR(-ENOMEM);
> -
> -	dport->dport_dev = dport_dev;
> -	dport->port_id = port_id;
> -	dport->port = port;
> +	dport->port_num = port_num;
>  
>  	if (rcrb == CXL_RESOURCE_NONE) {
>  		rc = cxl_dport_setup_regs(&port->dev, dport,
> @@ -1211,7 +1244,7 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>   * devm_cxl_add_dport - append VH downstream port data to a cxl_port
>   * @port: the cxl_port that references this dport
>   * @dport_dev: firmware or PCI device representing the dport
> - * @port_id: identifier for this dport in a decoder's target list
> + * @port_num: identifier for this dport in a decoder's target list
>   * @component_reg_phys: optional location of CXL component registers
>   *
>   * Note that dports are appended to the devm release action's of the
> @@ -1219,12 +1252,12 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>   * switch ports)
>   */
>  struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
> -				     struct device *dport_dev, int port_id,
> +				     struct device *dport_dev, int port_num,
>  				     resource_size_t component_reg_phys)
>  {
>  	struct cxl_dport *dport;
>  
> -	dport = __devm_cxl_add_dport(port, dport_dev, port_id,
> +	dport = __devm_cxl_add_dport(port, dport_dev, port_num,
>  				     component_reg_phys, CXL_RESOURCE_NONE);
>  	if (IS_ERR(dport)) {
>  		dev_dbg(dport_dev, "failed to add dport to %s: %ld\n",
> @@ -1455,7 +1488,7 @@ static void reap_dports(struct cxl_port *port)
>  	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);
> +		devm_release_action(&port->dev, free_dport, dport);
>  	}
>  }
>  
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index a9ab46eb0610..f4fe523aaf12 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -583,6 +583,7 @@ struct cxl_dax_region {
>   * @regions: cxl_region_ref instances, regions mapped by this port
>   * @parent_dport: dport that points to this port in the parent
>   * @decoder_ida: allocator for decoder ids
> + * @dport_ida: allocator for dport ids
>   * @reg_map: component and ras register mapping parameters
>   * @nr_dports: number of entries in @dports
>   * @hdm_end: track last allocated HDM decoder instance for allocation ordering
> @@ -603,6 +604,7 @@ struct cxl_port {
>  	struct xarray regions;
>  	struct cxl_dport *parent_dport;
>  	struct ida decoder_ida;
> +	struct ida dport_ida;
>  	struct cxl_register_map reg_map;
>  	int nr_dports;
>  	int hdm_end;
> @@ -655,7 +657,8 @@ struct cxl_rcrb_info {
>   * struct cxl_dport - CXL downstream port
>   * @dport_dev: PCI bridge or firmware device representing the downstream link
>   * @reg_map: component and ras register mapping parameters
> - * @port_id: unique hardware identifier for dport in decoder target list
> + * @id: Linux id to enumerate dport instances per port
> + * @port_num: unique hardware identifier for dport in decoder target list
>   * @rcrb: Data about the Root Complex Register Block layout
>   * @rch: Indicate whether this dport was enumerated in RCH or VH mode
>   * @port: reference to cxl_port that contains this downstream port
> @@ -667,7 +670,8 @@ struct cxl_rcrb_info {
>  struct cxl_dport {
>  	struct device *dport_dev;
>  	struct cxl_register_map reg_map;
> -	int port_id;
> +	int id;
> +	int port_num;
>  	struct cxl_rcrb_info rcrb;
>  	bool rch;
>  	struct cxl_port *port;
> @@ -750,10 +754,10 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
>  bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);
>  
>  struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
> -				     struct device *dport, int port_id,
> +				     struct device *dport, int port_num,
>  				     resource_size_t component_reg_phys);
>  struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
> -					 struct device *dport_dev, int port_id,
> +					 struct device *dport_dev, int port_num,
>  					 resource_size_t rcrb);
>  
>  #ifdef CONFIG_PCIEAER_CXL

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

[Li Ming]

On 5/7/2025 8:43 AM, 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.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Li Ming <ming.li@zohomail.com>

Re: [PATCH v2 03/10] cxl: Rename find_dport() to provide better function intent

[Li Ming]

On 5/7/2025 8:43 AM, Dave Jiang wrote:
> Rename find_dport() to find_dport_by_num() to indicate that the function
> is trying to match a dport by its hardware number index.
>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Li Ming <ming.li@zohomail.com>

Re: [PATCH v2 04/10] cxl: Remove adding of port_num via devm_cxl_add_dport()

[Li Ming]

On 5/7/2025 8:43 AM, Dave Jiang wrote:
> In preparation for delayed dport probing, remove setting of port_num
> through devm_cxl_add_dport(). Will temporarily set the port_num after
> dport is added for now.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Li Ming <ming.li@zohomail.com>

Re: [PATCH v2 02/10] cxl: Saperate out CXL dport->id vs actual dport hardware id

[Alejandro Lucero Palau]

Hi Dave,

swapped vowels in the commit: Saperate -> Separate


With that:

Reviewed-by: Alejandro Lucero <alucerop@amd.com>


On 5/7/25 01:43, Dave Jiang wrote:
> In preparation to allow dport to be allocated without being active, make
> dport->id to be Linux id that enumerates the dport objects per port.
> Keep the hardware id under dport->port_num to maintain compatibility and
> introduce a dport->id as the enumeration id.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v2:
> - Rename port_id to port_num. (Dan)
> - Use a dport allocator to deal with ida allocation. (Dan, Jonathan)
> ---
>   drivers/cxl/core/cdat.c |  2 +-
>   drivers/cxl/core/hdm.c  | 18 +++++-----
>   drivers/cxl/core/port.c | 73 ++++++++++++++++++++++++++++++-----------
>   drivers/cxl/cxl.h       | 12 ++++---
>   4 files changed, 71 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index edb4f41eeacc..f637e3631d88 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -501,7 +501,7 @@ static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg,
>   
>   		xa_for_each(&port->dports, index, dport) {
>   			if (dsp_id == ACPI_CDAT_SSLBIS_ANY_PORT ||
> -			    dsp_id == dport->port_id) {
> +			    dsp_id == dport->port_num) {
>   				cxl_access_coordinate_set(dport->coord,
>   							  sslbis->data_type,
>   							  val);
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 70cae4ebf8a4..d7ad92e191ba 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -69,7 +69,7 @@ int devm_cxl_add_passthrough_decoder(struct cxl_port *port)
>   
>   	xa_for_each(&port->dports, index, dport)
>   		break;
> -	single_port_map[0] = dport->port_id;
> +	single_port_map[0] = dport->port_num;
>   
>   	return add_hdm_decoder(port, &cxlsd->cxld, single_port_map);
>   }
> @@ -720,21 +720,21 @@ static void cxlsd_set_targets(struct cxl_switch_decoder *cxlsd, u64 *tgt)
>   	struct cxl_dport **t = &cxlsd->target[0];
>   	int ways = cxlsd->cxld.interleave_ways;
>   
> -	*tgt = FIELD_PREP(GENMASK(7, 0), t[0]->port_id);
> +	*tgt = FIELD_PREP(GENMASK(7, 0), t[0]->port_num);
>   	if (ways > 1)
> -		*tgt |= FIELD_PREP(GENMASK(15, 8), t[1]->port_id);
> +		*tgt |= FIELD_PREP(GENMASK(15, 8), t[1]->port_num);
>   	if (ways > 2)
> -		*tgt |= FIELD_PREP(GENMASK(23, 16), t[2]->port_id);
> +		*tgt |= FIELD_PREP(GENMASK(23, 16), t[2]->port_num);
>   	if (ways > 3)
> -		*tgt |= FIELD_PREP(GENMASK(31, 24), t[3]->port_id);
> +		*tgt |= FIELD_PREP(GENMASK(31, 24), t[3]->port_num);
>   	if (ways > 4)
> -		*tgt |= FIELD_PREP(GENMASK_ULL(39, 32), t[4]->port_id);
> +		*tgt |= FIELD_PREP(GENMASK_ULL(39, 32), t[4]->port_num);
>   	if (ways > 5)
> -		*tgt |= FIELD_PREP(GENMASK_ULL(47, 40), t[5]->port_id);
> +		*tgt |= FIELD_PREP(GENMASK_ULL(47, 40), t[5]->port_num);
>   	if (ways > 6)
> -		*tgt |= FIELD_PREP(GENMASK_ULL(55, 48), t[6]->port_id);
> +		*tgt |= FIELD_PREP(GENMASK_ULL(55, 48), t[6]->port_num);
>   	if (ways > 7)
> -		*tgt |= FIELD_PREP(GENMASK_ULL(63, 56), t[7]->port_id);
> +		*tgt |= FIELD_PREP(GENMASK_ULL(63, 56), t[7]->port_num);
>   }
>   
>   /*
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 726bd4a7de27..e9c02e4d0d4c 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -159,7 +159,7 @@ static ssize_t emit_target_list(struct cxl_switch_decoder *cxlsd, char *buf)
>   
>   		if (i + 1 < cxld->interleave_ways)
>   			next = cxlsd->target[i + 1];
> -		rc = sysfs_emit_at(buf, offset, "%d%s", dport->port_id,
> +		rc = sysfs_emit_at(buf, offset, "%d%s", dport->id,
>   				   next ? "," : "");
>   		if (rc < 0)
>   			return rc;
> @@ -739,6 +739,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev,
>   		dev->parent = uport_dev;
>   
>   	ida_init(&port->decoder_ida);
> +	ida_init(&port->dport_ida);
>   	port->hdm_end = -1;
>   	port->commit_end = -1;
>   	xa_init(&port->dports);
> @@ -1044,14 +1045,14 @@ void put_cxl_root(struct cxl_root *cxl_root)
>   }
>   EXPORT_SYMBOL_NS_GPL(put_cxl_root, "CXL");
>   
> -static struct cxl_dport *find_dport(struct cxl_port *port, int id)
> +static struct cxl_dport *find_dport(struct cxl_port *port, int port_num)
>   {
>   	struct cxl_dport *dport;
>   	unsigned long index;
>   
>   	device_lock_assert(&port->dev);
>   	xa_for_each(&port->dports, index, dport)
> -		if (dport->port_id == id)
> +		if (dport->port_num == port_num)
>   			return dport;
>   	return NULL;
>   }
> @@ -1062,11 +1063,11 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *dport)
>   	int rc;
>   
>   	device_lock_assert(&port->dev);
> -	dup = find_dport(port, dport->port_id);
> +	dup = find_dport(port, dport->port_num);
>   	if (dup) {
>   		dev_err(&port->dev,
> -			"unable to add dport%d-%s non-unique port id (%s)\n",
> -			dport->port_id, dev_name(dport->dport_dev),
> +			"unable to add dport%d-%s non-unique port num (%s)\n",
> +			dport->port_num, dev_name(dport->dport_dev),
>   			dev_name(dup->dport_dev));
>   		return -EBUSY;
>   	}
> @@ -1114,13 +1115,43 @@ static void cxl_dport_unlink(void *data)
>   	struct cxl_port *port = dport->port;
>   	char link_name[CXL_TARGET_STRLEN];
>   
> -	sprintf(link_name, "dport%d", dport->port_id);
> +	sprintf(link_name, "dport%d", dport->id);
>   	sysfs_remove_link(&port->dev.kobj, link_name);
>   }
>   
> +static void free_dport(void *data)
> +{
> +	struct cxl_dport *dport = data;
> +	struct cxl_port *port = dport->port;
> +
> +	ida_free(&port->dport_ida, dport->id);
> +	kfree(dport);
> +}
> +
> +static struct cxl_dport *cxl_alloc_dport(struct cxl_port *port,
> +					 struct device *dport_dev)
> +{
> +	int id;
> +
> +	struct cxl_dport *dport __free(kfree) =
> +		kzalloc(sizeof(*dport), GFP_KERNEL);
> +	if (!dport)
> +		return NULL;
> +
> +	id = ida_alloc(&port->dport_ida, GFP_KERNEL);
> +	if (id < 0)
> +		return NULL;
> +
> +	dport->dport_dev = dport_dev;
> +	dport->port = port;
> +	dport->id = id;
> +
> +	return no_free_ptr(dport);
> +}
> +
>   static struct cxl_dport *
>   __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> -		     int port_id, resource_size_t component_reg_phys,
> +		     int port_num, resource_size_t component_reg_phys,
>   		     resource_size_t rcrb)
>   {
>   	char link_name[CXL_TARGET_STRLEN];
> @@ -1139,17 +1170,19 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>   		return ERR_PTR(-ENXIO);
>   	}
>   
> -	if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >=
> +	dport = cxl_alloc_dport(port, dport_dev);
> +	if (!dport)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rc = devm_add_action_or_reset(&port->dev, free_dport, dport);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", dport->id) >=
>   	    CXL_TARGET_STRLEN)
>   		return ERR_PTR(-EINVAL);
>   
> -	dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
> -	if (!dport)
> -		return ERR_PTR(-ENOMEM);
> -
> -	dport->dport_dev = dport_dev;
> -	dport->port_id = port_id;
> -	dport->port = port;
> +	dport->port_num = port_num;
>   
>   	if (rcrb == CXL_RESOURCE_NONE) {
>   		rc = cxl_dport_setup_regs(&port->dev, dport,
> @@ -1211,7 +1244,7 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>    * devm_cxl_add_dport - append VH downstream port data to a cxl_port
>    * @port: the cxl_port that references this dport
>    * @dport_dev: firmware or PCI device representing the dport
> - * @port_id: identifier for this dport in a decoder's target list
> + * @port_num: identifier for this dport in a decoder's target list
>    * @component_reg_phys: optional location of CXL component registers
>    *
>    * Note that dports are appended to the devm release action's of the
> @@ -1219,12 +1252,12 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>    * switch ports)
>    */
>   struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
> -				     struct device *dport_dev, int port_id,
> +				     struct device *dport_dev, int port_num,
>   				     resource_size_t component_reg_phys)
>   {
>   	struct cxl_dport *dport;
>   
> -	dport = __devm_cxl_add_dport(port, dport_dev, port_id,
> +	dport = __devm_cxl_add_dport(port, dport_dev, port_num,
>   				     component_reg_phys, CXL_RESOURCE_NONE);
>   	if (IS_ERR(dport)) {
>   		dev_dbg(dport_dev, "failed to add dport to %s: %ld\n",
> @@ -1455,7 +1488,7 @@ static void reap_dports(struct cxl_port *port)
>   	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);
> +		devm_release_action(&port->dev, free_dport, dport);
>   	}
>   }
>   
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index a9ab46eb0610..f4fe523aaf12 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -583,6 +583,7 @@ struct cxl_dax_region {
>    * @regions: cxl_region_ref instances, regions mapped by this port
>    * @parent_dport: dport that points to this port in the parent
>    * @decoder_ida: allocator for decoder ids
> + * @dport_ida: allocator for dport ids
>    * @reg_map: component and ras register mapping parameters
>    * @nr_dports: number of entries in @dports
>    * @hdm_end: track last allocated HDM decoder instance for allocation ordering
> @@ -603,6 +604,7 @@ struct cxl_port {
>   	struct xarray regions;
>   	struct cxl_dport *parent_dport;
>   	struct ida decoder_ida;
> +	struct ida dport_ida;
>   	struct cxl_register_map reg_map;
>   	int nr_dports;
>   	int hdm_end;
> @@ -655,7 +657,8 @@ struct cxl_rcrb_info {
>    * struct cxl_dport - CXL downstream port
>    * @dport_dev: PCI bridge or firmware device representing the downstream link
>    * @reg_map: component and ras register mapping parameters
> - * @port_id: unique hardware identifier for dport in decoder target list
> + * @id: Linux id to enumerate dport instances per port
> + * @port_num: unique hardware identifier for dport in decoder target list
>    * @rcrb: Data about the Root Complex Register Block layout
>    * @rch: Indicate whether this dport was enumerated in RCH or VH mode
>    * @port: reference to cxl_port that contains this downstream port
> @@ -667,7 +670,8 @@ struct cxl_rcrb_info {
>   struct cxl_dport {
>   	struct device *dport_dev;
>   	struct cxl_register_map reg_map;
> -	int port_id;
> +	int id;
> +	int port_num;
>   	struct cxl_rcrb_info rcrb;
>   	bool rch;
>   	struct cxl_port *port;
> @@ -750,10 +754,10 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
>   bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);
>   
>   struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
> -				     struct device *dport, int port_id,
> +				     struct device *dport, int port_num,
>   				     resource_size_t component_reg_phys);
>   struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
> -					 struct device *dport_dev, int port_id,
> +					 struct device *dport_dev, int port_num,
>   					 resource_size_t rcrb);
>   
>   #ifdef CONFIG_PCIEAER_CXL

Re: [PATCH v2 03/10] cxl: Rename find_dport() to provide better function intent

[Alejandro Lucero Palau]

On 5/7/25 01:43, Dave Jiang wrote:
> Rename find_dport() to find_dport_by_num() to indicate that the function
> is trying to match a dport by its hardware number index.
>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   drivers/cxl/core/port.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index e9c02e4d0d4c..1d7a4a2ef6ad 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1045,7 +1045,7 @@ void put_cxl_root(struct cxl_root *cxl_root)
>   }
>   EXPORT_SYMBOL_NS_GPL(put_cxl_root, "CXL");
>   
> -static struct cxl_dport *find_dport(struct cxl_port *port, int port_num)
> +static struct cxl_dport *find_dport_by_num(struct cxl_port *port, int port_num)
>   {
>   	struct cxl_dport *dport;
>   	unsigned long index;
> @@ -1063,7 +1063,7 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *dport)
>   	int rc;
>   
>   	device_lock_assert(&port->dev);
> -	dup = find_dport(port, dport->port_num);
> +	dup = find_dport_by_num(port, dport->port_num);
>   	if (dup) {
>   		dev_err(&port->dev,
>   			"unable to add dport%d-%s non-unique port num (%s)\n",
> @@ -1275,13 +1275,13 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, "CXL");
>    * devm_cxl_add_rch_dport - append RCH downstream port data to a cxl_port
>    * @port: the cxl_port that references this dport
>    * @dport_dev: firmware or PCI device representing the dport
> - * @port_id: identifier for this dport in a decoder's target list
> + * @port_num: identifier for this dport in a decoder's target list


Not sure this change should be in this patch. It makes more sense to me 
in the previous one.


Also, maybe adding some reference for easily seeing where the identifier 
comes from:

+ * @port_num: hardware identifier for this dport in a decoder's target list


>    * @rcrb: mandatory location of a Root Complex Register Block
>    *
>    * See CXL 3.0 9.11.8 CXL Devices Attached to an RCH
>    */
>   struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
> -					 struct device *dport_dev, int port_id,
> +					 struct device *dport_dev, int port_num,
>   					 resource_size_t rcrb)
>   {
>   	struct cxl_dport *dport;
> @@ -1291,7 +1291,7 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>   		return ERR_PTR(-EINVAL);
>   	}
>   
> -	dport = __devm_cxl_add_dport(port, dport_dev, port_id,
> +	dport = __devm_cxl_add_dport(port, dport_dev, port_num,
>   				     CXL_RESOURCE_NONE, rcrb);
>   	if (IS_ERR(dport)) {
>   		dev_dbg(dport_dev, "failed to add RCH dport to %s: %ld\n",
> @@ -1764,7 +1764,7 @@ static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
>   
>   	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]);
> +		struct cxl_dport *dport = find_dport_by_num(port, target_map[i]);
>   
>   		if (!dport)
>   			return -ENXIO;

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

[Gregory Price]

On Tue, May 06, 2025 at 05:43:01PM -0700, 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.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Reviewed-by: Gregory Price <gourry@gourry.net>

Re: [PATCH v2 02/10] cxl: Saperate out CXL dport->id vs actual dport hardware id

[Gregory Price]

On Tue, May 06, 2025 at 05:43:02PM -0700, Dave Jiang wrote:
> In preparation to allow dport to be allocated without being active, make
> dport->id to be Linux id that enumerates the dport objects per port.
> Keep the hardware id under dport->port_num to maintain compatibility and
> introduce a dport->id as the enumeration id.
>

This is more for the sake of clarity/documentation than anything else.

dport->port_num is the `hardware id` - i.e. what we'd find in an ACPI
table or something.  This number may not necessarily be unique

dport->port_id is now a Linux ID that is unique to the device.

For example, on my existing test system, i have this

[/sys/bus/cxl/devices]# ls port1/dport*
dport0  dport113  dport2 
[/sys/bus/cxl/devices]# ls port2/dport*
dport113  dport2
[/sys/bus/cxl/devices]# ls port3/dport*
dport0

I should now expect the following:

[/sys/bus/cxl/devices]# ls port1/dport*
dport0  dport1  dport2 
[/sys/bus/cxl/devices]# ls port2/dport*
dport0  dport1
[/sys/bus/cxl/devices]# ls port3/dport*
dport0


This should also apply to the root, whose dports were previously
dictated by the host bridge numbers defined in the CHBS/DSDT.

[/sys/bus/cxl/devices/root0]# ls
dport0  dport1  dport4  dport5

turns into

[/sys/bus/cxl/devices/root0]# ls
dport0  dport1  dport2  dport3


and my decoder0.0 target list ends up similarly changed

from
[/sys/bus/cxl/devices/root0/decoder0.0]# cat target_list
5,4

to

[/sys/bus/cxl/devices/root0/decoder0.0]# cat target_list
3,2

> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

If this is all correct, I like this.

Reviewed-by: Gregory Price <gourry@gourry.net>

~Gregory

Re: [PATCH v2 03/10] cxl: Rename find_dport() to provide better function intent

[Gregory Price]

On Tue, May 06, 2025 at 05:43:03PM -0700, Dave Jiang wrote:
> Rename find_dport() to find_dport_by_num() to indicate that the function
> is trying to match a dport by its hardware number index.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

With the change about the follwoing lines recommended by Alejandro:

> - * @port_id: identifier for this dport in a decoder's target list
> + * @port_num: identifier for this dport in a decoder's target list

Reviewed-by: Gregory Price <gourry@gourry.net>

Re: [PATCH v2 04/10] cxl: Remove adding of port_num via devm_cxl_add_dport()

[Gregory Price]

On Tue, May 06, 2025 at 05:43:04PM -0700, Dave Jiang wrote:
> In preparation for delayed dport probing, remove setting of port_num
> through devm_cxl_add_dport(). Will temporarily set the port_num after
> dport is added for now.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>


Worth noting in the changelog that the port_num is set to
CXL_DPORT_NUM_INVALID in the interim to detect any race
conditions that might rise from this.

with that

Reviewed-by: Gregory Price <gourry@gourry.net>

Re: [PATCH v2 05/10] cxl: Defer hardware dport->port_id assignment and registers probing

[Gregory Price]

On Tue, May 06, 2025 at 05:43:05PM -0700, Dave Jiang wrote:
> Current implementation only enumerates the dports during the port 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 hw
> dport id assignment and the register probing to behind memdev probe so the
> endpoint is guaranteed to be connected.
> 
> The detection of duplicate dport for add_dport() is removed. The port_id
> is not read from the hw at this point any longer. The port->id will always
> be unique since it's retrieved from an ida. The dup detection thus become
> irrelevant.
> 
> 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.
> 

I'm finding the changes a little difficult to follow, can you lay out
the expected order of operations before and after?

Specifically there's two new guard() calls, can you explain under what
conditions those can be contended?

~Gregory

> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 70173d23139c..04e18a102d26 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
... snip ...
> +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_num) {
> +			cxlsd->target[i] = dport;
> +			return 0;
> +		}
> +	}
... snip ...
> @@ -1695,6 +1798,19 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>  				"found already registered port %s:%s\n",
>  				dev_name(&port->dev),
>  				dev_name(port->uport_dev));
> +
> +			/*
> +			 * Attempt to do single pass dport setup by checking here
> +			 * instead of doing it during port creation. Otherwise
> +			 * it still needs to check here for dports that are
> +			 * being probed with a port already created.
> +			 */
> +			scoped_guard(device, &port->dev) {
> +				rc = cxl_switch_port_dport_setup(port, dport_dev);
> +				if (rc)
> +					return rc;
> +			}
> +
... snip ...
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index a35fc5552845..4d840a6ef802 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
... snip ...
>  	/* Cache the data early to ensure is_visible() works */
> @@ -69,24 +68,7 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>  	if (rc < 0)
>  		return rc;
>  
... snip ...
> -	return -ENXIO;
> +	return 0;
>  }

	return devm_cxl_port_enumerate_dports(port);


~Gregory

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

[Gregory Price]

On Tue, May 06, 2025 at 05:43:07PM -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.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Reviewed-by: Gregory Price <gourry@gourry.net>

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

[Gregory Price]

On Tue, May 06, 2025 at 05:43:08PM -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.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

ignorant terminology question: what's the different between the
"cxl hierarchy head" and "cxl root"?

~Gregory

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

[Gregory Price]

On Tue, May 06, 2025 at 05:43:09PM -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.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

mostly wording/naming nits.

I think "udev" is a bad naming choice, but i don't know what to replace
it with.  Most of the scenarios below are host-bridge-uport specifical,
so maybe like "hbup" (actually no, hbup is ugly, but you get the point).

> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 259b217e812f..a5a673d789f3 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
>  
> +/**
> + * cxl_udev_to_root - Retrieve cxl_root tied to the host bridge device
> + * @uport_dev: upstream port device that is the host bridge
                  "The host bridge's uport device connect to the root" ?

maybe instead of uport_dev, maybe hb_uport_dev? If this interface is
only intended to be used specifically with hb_uport.  This is a nit,
ignore it if you want.

> + *
> + * 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_udev_to_root(struct device *uport_dev)

udev_to_root implies you can call this with any uport_dev.
hb_uport_to_root?

Functionality is fine.

> +{
> +	struct cxl_root *root;
> +
> +	root = xa_load(&cxl_root_ports, (unsigned long)uport_dev);
> +	if (!root)
> +		return NULL;
> +
> +	get_device(&root->port.dev);
> +
> +	return root;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_udev_to_root, "CXL");
> +
> +/**
> + * devm_cxl_register_udev_root_port - Tie a hostbridge device to a root port

Similar naming questions as above.

At this point i'm a bit convinced "udev" is not what we want.

> + * @host: device that hosts the memory for the xarray entries
> + * @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_udev_root_port(struct device *host,
> +				     struct device *uport_dev,
> +				     struct cxl_root *root)
> +{
> +	int rc;
> +
> +	rc = xa_insert(&cxl_root_ports, (unsigned long)uport_dev, root,
> +		       GFP_KERNEL);
> +	if (rc)
> +		return rc;
> +
> +	return devm_add_action_or_reset(host, unregister_udev_root_ports,
> +					uport_dev);
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_register_udev_root_port, "CXL");
> +
>  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 22f1a9542077..e0cba91803cc 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -736,6 +736,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_udev_root_port(struct device *host,
> +				     struct device *uport_dev,
> +				     struct cxl_root *root);
> +struct cxl_root *cxl_udev_to_root(struct device *uport_dev);
>  struct cxl_port *devm_cxl_add_port(struct device *host,
>  				   struct device *uport_dev,
>  				   resource_size_t component_reg_phys,
> -- 
> 2.49.0
> 

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

[Dave Jiang]

On 5/13/25 8:49 AM, Gregory Price wrote:
> On Tue, May 06, 2025 at 05:43:08PM -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.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> ignorant terminology question: what's the different between the
> "cxl hierarchy head" and "cxl root"?

I struggle to find the correct terminology between the head of the PCI (or platform device for cxl_test) hierarchy vs the CXL one we constructed for the 'cxl_port' hierarchy. I'm open to suggestions to use better words to distinguish that. 
> 
> ~Gregory
> 

Re: [PATCH v2 02/10] cxl: Saperate out CXL dport->id vs actual dport hardware id

[Dave Jiang]

On 5/8/25 5:51 PM, Li Ming wrote:
> On 5/7/2025 8:43 AM, Dave Jiang wrote:
>> In preparation to allow dport to be allocated without being active, make
>> dport->id to be Linux id that enumerates the dport objects per port.
>> Keep the hardware id under dport->port_num to maintain compatibility and
>> introduce a dport->id as the enumeration id.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> v2:
>> - Rename port_id to port_num. (Dan)
>> - Use a dport allocator to deal with ida allocation. (Dan, Jonathan)
>> ---
>>  drivers/cxl/core/cdat.c |  2 +-
>>  drivers/cxl/core/hdm.c  | 18 +++++-----
>>  drivers/cxl/core/port.c | 73 ++++++++++++++++++++++++++++++-----------
>>  drivers/cxl/cxl.h       | 12 ++++---
>>  4 files changed, 71 insertions(+), 34 deletions(-)
>>
> [snip]
>>  static struct cxl_dport *
>>  __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>> -		     int port_id, resource_size_t component_reg_phys,
>> +		     int port_num, resource_size_t component_reg_phys,
>>  		     resource_size_t rcrb)
>>  {
>>  	char link_name[CXL_TARGET_STRLEN];
>> @@ -1139,17 +1170,19 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  		return ERR_PTR(-ENXIO);
>>  	}
>>  
>> -	if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >=
>> +	dport = cxl_alloc_dport(port, dport_dev);
>> +	if (!dport)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	rc = devm_add_action_or_reset(&port->dev, free_dport, dport);
>> +	if (rc)
>> +		return ERR_PTR(rc);
>> +
>> +	if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", dport->id) >=
>>  	    CXL_TARGET_STRLEN)
>>  		return ERR_PTR(-EINVAL);
> Is it better to call devm_release_action(&port->dev, free_dport, dport) here if snprintf() failed?

Not sure I follow why this error path is different than the others and needs release_action().

> 
>>  
>> -	dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
>> -	if (!dport)
>> -		return ERR_PTR(-ENOMEM);
>> -
>> -	dport->dport_dev = dport_dev;
>> -	dport->port_id = port_id;
>> -	dport->port = port;
>> +	dport->port_num = port_num;
>>  
>>  	if (rcrb == CXL_RESOURCE_NONE) {
>>  		rc = cxl_dport_setup_regs(&port->dev, dport,
>> @@ -1211,7 +1244,7 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>   * devm_cxl_add_dport - append VH downstream port data to a cxl_port
>>   * @port: the cxl_port that references this dport
>>   * @dport_dev: firmware or PCI device representing the dport
>> - * @port_id: identifier for this dport in a decoder's target list
>> + * @port_num: identifier for this dport in a decoder's target list
>>   * @component_reg_phys: optional location of CXL component registers
>>   *
>>   * Note that dports are appended to the devm release action's of the
>> @@ -1219,12 +1252,12 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>   * switch ports)
>>   */
>>  struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
>> -				     struct device *dport_dev, int port_id,
>> +				     struct device *dport_dev, int port_num,
>>  				     resource_size_t component_reg_phys)
>>  {
>>  	struct cxl_dport *dport;
>>  
>> -	dport = __devm_cxl_add_dport(port, dport_dev, port_id,
>> +	dport = __devm_cxl_add_dport(port, dport_dev, port_num,
>>  				     component_reg_phys, CXL_RESOURCE_NONE);
>>  	if (IS_ERR(dport)) {
>>  		dev_dbg(dport_dev, "failed to add dport to %s: %ld\n",
>> @@ -1455,7 +1488,7 @@ static void reap_dports(struct cxl_port *port)
>>  	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);
>> +		devm_release_action(&port->dev, free_dport, dport);
>>  	}
>>  }
>>  
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index a9ab46eb0610..f4fe523aaf12 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -583,6 +583,7 @@ struct cxl_dax_region {
>>   * @regions: cxl_region_ref instances, regions mapped by this port
>>   * @parent_dport: dport that points to this port in the parent
>>   * @decoder_ida: allocator for decoder ids
>> + * @dport_ida: allocator for dport ids
>>   * @reg_map: component and ras register mapping parameters
>>   * @nr_dports: number of entries in @dports
>>   * @hdm_end: track last allocated HDM decoder instance for allocation ordering
>> @@ -603,6 +604,7 @@ struct cxl_port {
>>  	struct xarray regions;
>>  	struct cxl_dport *parent_dport;
>>  	struct ida decoder_ida;
>> +	struct ida dport_ida;
>>  	struct cxl_register_map reg_map;
>>  	int nr_dports;
>>  	int hdm_end;
>> @@ -655,7 +657,8 @@ struct cxl_rcrb_info {
>>   * struct cxl_dport - CXL downstream port
>>   * @dport_dev: PCI bridge or firmware device representing the downstream link
>>   * @reg_map: component and ras register mapping parameters
>> - * @port_id: unique hardware identifier for dport in decoder target list
>> + * @id: Linux id to enumerate dport instances per port
>> + * @port_num: unique hardware identifier for dport in decoder target list
>>   * @rcrb: Data about the Root Complex Register Block layout
>>   * @rch: Indicate whether this dport was enumerated in RCH or VH mode
>>   * @port: reference to cxl_port that contains this downstream port
>> @@ -667,7 +670,8 @@ struct cxl_rcrb_info {
>>  struct cxl_dport {
>>  	struct device *dport_dev;
>>  	struct cxl_register_map reg_map;
>> -	int port_id;
>> +	int id;
>> +	int port_num;
>>  	struct cxl_rcrb_info rcrb;
>>  	bool rch;
>>  	struct cxl_port *port;
>> @@ -750,10 +754,10 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
>>  bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);
>>  
>>  struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
>> -				     struct device *dport, int port_id,
>> +				     struct device *dport, int port_num,
>>  				     resource_size_t component_reg_phys);
>>  struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>> -					 struct device *dport_dev, int port_id,
>> +					 struct device *dport_dev, int port_num,
>>  					 resource_size_t rcrb);
>>  
>>  #ifdef CONFIG_PCIEAER_CXL
> 
> 
> 

Re: [PATCH v2 02/10] cxl: Saperate out CXL dport->id vs actual dport hardware id

[Dave Jiang]

On 5/8/25 1:08 PM, Alison Schofield wrote:
> On Tue, May 06, 2025 at 05:43:02PM -0700, Dave Jiang wrote:
> 
> s/saperate/separate
> 
>> In preparation to allow dport to be allocated without being active, make
>> dport->id to be Linux id that enumerates the dport objects per port.
>> Keep the hardware id under dport->port_num to maintain compatibility and
>> introduce a dport->id as the enumeration id.
> 
> I think the 'and introduce...' end of the second sentence is repeating
> what the first sentence declared an can be omitted.
> 

Thank you. Will fix.

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

Re: [PATCH v2 02/10] cxl: Saperate out CXL dport->id vs actual dport hardware id

[Dave Jiang]

On 5/9/25 2:14 AM, Alejandro Lucero Palau wrote:
> Hi Dave,
> 
> swapped vowels in the commit: Saperate -> Separate

Thanks for the review. Will fix. 
> 
> 
> With that:
> 
> Reviewed-by: Alejandro Lucero <alucerop@amd.com>
> 
> 
> On 5/7/25 01:43, Dave Jiang wrote:
>> In preparation to allow dport to be allocated without being active, make
>> dport->id to be Linux id that enumerates the dport objects per port.
>> Keep the hardware id under dport->port_num to maintain compatibility and
>> introduce a dport->id as the enumeration id.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> v2:
>> - Rename port_id to port_num. (Dan)
>> - Use a dport allocator to deal with ida allocation. (Dan, Jonathan)
>> ---
>>   drivers/cxl/core/cdat.c |  2 +-
>>   drivers/cxl/core/hdm.c  | 18 +++++-----
>>   drivers/cxl/core/port.c | 73 ++++++++++++++++++++++++++++++-----------
>>   drivers/cxl/cxl.h       | 12 ++++---
>>   4 files changed, 71 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>> index edb4f41eeacc..f637e3631d88 100644
>> --- a/drivers/cxl/core/cdat.c
>> +++ b/drivers/cxl/core/cdat.c
>> @@ -501,7 +501,7 @@ static int cdat_sslbis_handler(union acpi_subtable_headers *header, void *arg,
>>             xa_for_each(&port->dports, index, dport) {
>>               if (dsp_id == ACPI_CDAT_SSLBIS_ANY_PORT ||
>> -                dsp_id == dport->port_id) {
>> +                dsp_id == dport->port_num) {
>>                   cxl_access_coordinate_set(dport->coord,
>>                                 sslbis->data_type,
>>                                 val);
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index 70cae4ebf8a4..d7ad92e191ba 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -69,7 +69,7 @@ int devm_cxl_add_passthrough_decoder(struct cxl_port *port)
>>         xa_for_each(&port->dports, index, dport)
>>           break;
>> -    single_port_map[0] = dport->port_id;
>> +    single_port_map[0] = dport->port_num;
>>         return add_hdm_decoder(port, &cxlsd->cxld, single_port_map);
>>   }
>> @@ -720,21 +720,21 @@ static void cxlsd_set_targets(struct cxl_switch_decoder *cxlsd, u64 *tgt)
>>       struct cxl_dport **t = &cxlsd->target[0];
>>       int ways = cxlsd->cxld.interleave_ways;
>>   -    *tgt = FIELD_PREP(GENMASK(7, 0), t[0]->port_id);
>> +    *tgt = FIELD_PREP(GENMASK(7, 0), t[0]->port_num);
>>       if (ways > 1)
>> -        *tgt |= FIELD_PREP(GENMASK(15, 8), t[1]->port_id);
>> +        *tgt |= FIELD_PREP(GENMASK(15, 8), t[1]->port_num);
>>       if (ways > 2)
>> -        *tgt |= FIELD_PREP(GENMASK(23, 16), t[2]->port_id);
>> +        *tgt |= FIELD_PREP(GENMASK(23, 16), t[2]->port_num);
>>       if (ways > 3)
>> -        *tgt |= FIELD_PREP(GENMASK(31, 24), t[3]->port_id);
>> +        *tgt |= FIELD_PREP(GENMASK(31, 24), t[3]->port_num);
>>       if (ways > 4)
>> -        *tgt |= FIELD_PREP(GENMASK_ULL(39, 32), t[4]->port_id);
>> +        *tgt |= FIELD_PREP(GENMASK_ULL(39, 32), t[4]->port_num);
>>       if (ways > 5)
>> -        *tgt |= FIELD_PREP(GENMASK_ULL(47, 40), t[5]->port_id);
>> +        *tgt |= FIELD_PREP(GENMASK_ULL(47, 40), t[5]->port_num);
>>       if (ways > 6)
>> -        *tgt |= FIELD_PREP(GENMASK_ULL(55, 48), t[6]->port_id);
>> +        *tgt |= FIELD_PREP(GENMASK_ULL(55, 48), t[6]->port_num);
>>       if (ways > 7)
>> -        *tgt |= FIELD_PREP(GENMASK_ULL(63, 56), t[7]->port_id);
>> +        *tgt |= FIELD_PREP(GENMASK_ULL(63, 56), t[7]->port_num);
>>   }
>>     /*
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 726bd4a7de27..e9c02e4d0d4c 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -159,7 +159,7 @@ static ssize_t emit_target_list(struct cxl_switch_decoder *cxlsd, char *buf)
>>             if (i + 1 < cxld->interleave_ways)
>>               next = cxlsd->target[i + 1];
>> -        rc = sysfs_emit_at(buf, offset, "%d%s", dport->port_id,
>> +        rc = sysfs_emit_at(buf, offset, "%d%s", dport->id,
>>                      next ? "," : "");
>>           if (rc < 0)
>>               return rc;
>> @@ -739,6 +739,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev,
>>           dev->parent = uport_dev;
>>         ida_init(&port->decoder_ida);
>> +    ida_init(&port->dport_ida);
>>       port->hdm_end = -1;
>>       port->commit_end = -1;
>>       xa_init(&port->dports);
>> @@ -1044,14 +1045,14 @@ void put_cxl_root(struct cxl_root *cxl_root)
>>   }
>>   EXPORT_SYMBOL_NS_GPL(put_cxl_root, "CXL");
>>   -static struct cxl_dport *find_dport(struct cxl_port *port, int id)
>> +static struct cxl_dport *find_dport(struct cxl_port *port, int port_num)
>>   {
>>       struct cxl_dport *dport;
>>       unsigned long index;
>>         device_lock_assert(&port->dev);
>>       xa_for_each(&port->dports, index, dport)
>> -        if (dport->port_id == id)
>> +        if (dport->port_num == port_num)
>>               return dport;
>>       return NULL;
>>   }
>> @@ -1062,11 +1063,11 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *dport)
>>       int rc;
>>         device_lock_assert(&port->dev);
>> -    dup = find_dport(port, dport->port_id);
>> +    dup = find_dport(port, dport->port_num);
>>       if (dup) {
>>           dev_err(&port->dev,
>> -            "unable to add dport%d-%s non-unique port id (%s)\n",
>> -            dport->port_id, dev_name(dport->dport_dev),
>> +            "unable to add dport%d-%s non-unique port num (%s)\n",
>> +            dport->port_num, dev_name(dport->dport_dev),
>>               dev_name(dup->dport_dev));
>>           return -EBUSY;
>>       }
>> @@ -1114,13 +1115,43 @@ static void cxl_dport_unlink(void *data)
>>       struct cxl_port *port = dport->port;
>>       char link_name[CXL_TARGET_STRLEN];
>>   -    sprintf(link_name, "dport%d", dport->port_id);
>> +    sprintf(link_name, "dport%d", dport->id);
>>       sysfs_remove_link(&port->dev.kobj, link_name);
>>   }
>>   +static void free_dport(void *data)
>> +{
>> +    struct cxl_dport *dport = data;
>> +    struct cxl_port *port = dport->port;
>> +
>> +    ida_free(&port->dport_ida, dport->id);
>> +    kfree(dport);
>> +}
>> +
>> +static struct cxl_dport *cxl_alloc_dport(struct cxl_port *port,
>> +                     struct device *dport_dev)
>> +{
>> +    int id;
>> +
>> +    struct cxl_dport *dport __free(kfree) =
>> +        kzalloc(sizeof(*dport), GFP_KERNEL);
>> +    if (!dport)
>> +        return NULL;
>> +
>> +    id = ida_alloc(&port->dport_ida, GFP_KERNEL);
>> +    if (id < 0)
>> +        return NULL;
>> +
>> +    dport->dport_dev = dport_dev;
>> +    dport->port = port;
>> +    dport->id = id;
>> +
>> +    return no_free_ptr(dport);
>> +}
>> +
>>   static struct cxl_dport *
>>   __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>> -             int port_id, resource_size_t component_reg_phys,
>> +             int port_num, resource_size_t component_reg_phys,
>>                resource_size_t rcrb)
>>   {
>>       char link_name[CXL_TARGET_STRLEN];
>> @@ -1139,17 +1170,19 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>           return ERR_PTR(-ENXIO);
>>       }
>>   -    if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >=
>> +    dport = cxl_alloc_dport(port, dport_dev);
>> +    if (!dport)
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    rc = devm_add_action_or_reset(&port->dev, free_dport, dport);
>> +    if (rc)
>> +        return ERR_PTR(rc);
>> +
>> +    if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", dport->id) >=
>>           CXL_TARGET_STRLEN)
>>           return ERR_PTR(-EINVAL);
>>   -    dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
>> -    if (!dport)
>> -        return ERR_PTR(-ENOMEM);
>> -
>> -    dport->dport_dev = dport_dev;
>> -    dport->port_id = port_id;
>> -    dport->port = port;
>> +    dport->port_num = port_num;
>>         if (rcrb == CXL_RESOURCE_NONE) {
>>           rc = cxl_dport_setup_regs(&port->dev, dport,
>> @@ -1211,7 +1244,7 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>    * devm_cxl_add_dport - append VH downstream port data to a cxl_port
>>    * @port: the cxl_port that references this dport
>>    * @dport_dev: firmware or PCI device representing the dport
>> - * @port_id: identifier for this dport in a decoder's target list
>> + * @port_num: identifier for this dport in a decoder's target list
>>    * @component_reg_phys: optional location of CXL component registers
>>    *
>>    * Note that dports are appended to the devm release action's of the
>> @@ -1219,12 +1252,12 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>    * switch ports)
>>    */
>>   struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
>> -                     struct device *dport_dev, int port_id,
>> +                     struct device *dport_dev, int port_num,
>>                        resource_size_t component_reg_phys)
>>   {
>>       struct cxl_dport *dport;
>>   -    dport = __devm_cxl_add_dport(port, dport_dev, port_id,
>> +    dport = __devm_cxl_add_dport(port, dport_dev, port_num,
>>                        component_reg_phys, CXL_RESOURCE_NONE);
>>       if (IS_ERR(dport)) {
>>           dev_dbg(dport_dev, "failed to add dport to %s: %ld\n",
>> @@ -1455,7 +1488,7 @@ static void reap_dports(struct cxl_port *port)
>>       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);
>> +        devm_release_action(&port->dev, free_dport, dport);
>>       }
>>   }
>>   diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index a9ab46eb0610..f4fe523aaf12 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -583,6 +583,7 @@ struct cxl_dax_region {
>>    * @regions: cxl_region_ref instances, regions mapped by this port
>>    * @parent_dport: dport that points to this port in the parent
>>    * @decoder_ida: allocator for decoder ids
>> + * @dport_ida: allocator for dport ids
>>    * @reg_map: component and ras register mapping parameters
>>    * @nr_dports: number of entries in @dports
>>    * @hdm_end: track last allocated HDM decoder instance for allocation ordering
>> @@ -603,6 +604,7 @@ struct cxl_port {
>>       struct xarray regions;
>>       struct cxl_dport *parent_dport;
>>       struct ida decoder_ida;
>> +    struct ida dport_ida;
>>       struct cxl_register_map reg_map;
>>       int nr_dports;
>>       int hdm_end;
>> @@ -655,7 +657,8 @@ struct cxl_rcrb_info {
>>    * struct cxl_dport - CXL downstream port
>>    * @dport_dev: PCI bridge or firmware device representing the downstream link
>>    * @reg_map: component and ras register mapping parameters
>> - * @port_id: unique hardware identifier for dport in decoder target list
>> + * @id: Linux id to enumerate dport instances per port
>> + * @port_num: unique hardware identifier for dport in decoder target list
>>    * @rcrb: Data about the Root Complex Register Block layout
>>    * @rch: Indicate whether this dport was enumerated in RCH or VH mode
>>    * @port: reference to cxl_port that contains this downstream port
>> @@ -667,7 +670,8 @@ struct cxl_rcrb_info {
>>   struct cxl_dport {
>>       struct device *dport_dev;
>>       struct cxl_register_map reg_map;
>> -    int port_id;
>> +    int id;
>> +    int port_num;
>>       struct cxl_rcrb_info rcrb;
>>       bool rch;
>>       struct cxl_port *port;
>> @@ -750,10 +754,10 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
>>   bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);
>>     struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
>> -                     struct device *dport, int port_id,
>> +                     struct device *dport, int port_num,
>>                        resource_size_t component_reg_phys);
>>   struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>> -                     struct device *dport_dev, int port_id,
>> +                     struct device *dport_dev, int port_num,
>>                        resource_size_t rcrb);
>>     #ifdef CONFIG_PCIEAER_CXL

Re: [PATCH v2 02/10] cxl: Saperate out CXL dport->id vs actual dport hardware id

[Dave Jiang]

On 5/12/25 10:04 PM, Gregory Price wrote:
> On Tue, May 06, 2025 at 05:43:02PM -0700, Dave Jiang wrote:
>> In preparation to allow dport to be allocated without being active, make
>> dport->id to be Linux id that enumerates the dport objects per port.
>> Keep the hardware id under dport->port_num to maintain compatibility and
>> introduce a dport->id as the enumeration id.
>>
> 
> This is more for the sake of clarity/documentation than anything else.
> 
> dport->port_num is the `hardware id` - i.e. what we'd find in an ACPI
> table or something.  This number may not necessarily be unique
> 
> dport->port_id is now a Linux ID that is unique to the device.

Yes. dport->id is guaranteed to be unique. While dport->port_num theoretically should be, but if a port is not active, it may have some bogus number in the register that clash with existing active port_num. I think Robert may have ran into this issue.

DJ


> 
> For example, on my existing test system, i have this
> 
> [/sys/bus/cxl/devices]# ls port1/dport*
> dport0  dport113  dport2 
> [/sys/bus/cxl/devices]# ls port2/dport*
> dport113  dport2
> [/sys/bus/cxl/devices]# ls port3/dport*
> dport0
> 
> I should now expect the following:
> 
> [/sys/bus/cxl/devices]# ls port1/dport*
> dport0  dport1  dport2 
> [/sys/bus/cxl/devices]# ls port2/dport*
> dport0  dport1
> [/sys/bus/cxl/devices]# ls port3/dport*
> dport0
> 
> 
> This should also apply to the root, whose dports were previously
> dictated by the host bridge numbers defined in the CHBS/DSDT.
> 
> [/sys/bus/cxl/devices/root0]# ls
> dport0  dport1  dport4  dport5
> 
> turns into
> 
> [/sys/bus/cxl/devices/root0]# ls
> dport0  dport1  dport2  dport3
> 
> 
> and my decoder0.0 target list ends up similarly changed
> 
> from
> [/sys/bus/cxl/devices/root0/decoder0.0]# cat target_list
> 5,4
> 
> to
> 
> [/sys/bus/cxl/devices/root0/decoder0.0]# cat target_list
> 3,2
> 
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> If this is all correct, I like this.
> 
> Reviewed-by: Gregory Price <gourry@gourry.net>
> 
> ~Gregory

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

[Gregory Price]

On Tue, May 13, 2025 at 09:12:00AM -0700, Dave Jiang wrote:
> 
> 
> On 5/13/25 8:49 AM, Gregory Price wrote:
> > On Tue, May 06, 2025 at 05:43:08PM -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.
> >>
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > 
> > ignorant terminology question: what's the different between the
> > "cxl hierarchy head" and "cxl root"?
> 
> I struggle to find the correct terminology between the head of the PCI (or platform device for cxl_test) hierarchy vs the CXL one we constructed for the 'cxl_port' hierarchy. I'm open to suggestions to use better words to distinguish that. 

Is there an actual difference or they actually the same "device" - just
abstracted differently?

~Gregory

Re: [PATCH v2 03/10] cxl: Rename find_dport() to provide better function intent

[Dave Jiang]

On 5/9/25 2:20 AM, Alejandro Lucero Palau wrote:
> 
> On 5/7/25 01:43, Dave Jiang wrote:
>> Rename find_dport() to find_dport_by_num() to indicate that the function
>> is trying to match a dport by its hardware number index.
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/cxl/core/port.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index e9c02e4d0d4c..1d7a4a2ef6ad 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -1045,7 +1045,7 @@ void put_cxl_root(struct cxl_root *cxl_root)
>>   }
>>   EXPORT_SYMBOL_NS_GPL(put_cxl_root, "CXL");
>>   -static struct cxl_dport *find_dport(struct cxl_port *port, int port_num)
>> +static struct cxl_dport *find_dport_by_num(struct cxl_port *port, int port_num)
>>   {
>>       struct cxl_dport *dport;
>>       unsigned long index;
>> @@ -1063,7 +1063,7 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *dport)
>>       int rc;
>>         device_lock_assert(&port->dev);
>> -    dup = find_dport(port, dport->port_num);
>> +    dup = find_dport_by_num(port, dport->port_num);
>>       if (dup) {
>>           dev_err(&port->dev,
>>               "unable to add dport%d-%s non-unique port num (%s)\n",
>> @@ -1275,13 +1275,13 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, "CXL");
>>    * devm_cxl_add_rch_dport - append RCH downstream port data to a cxl_port
>>    * @port: the cxl_port that references this dport
>>    * @dport_dev: firmware or PCI device representing the dport
>> - * @port_id: identifier for this dport in a decoder's target list
>> + * @port_num: identifier for this dport in a decoder's target list
> 
> 
> Not sure this change should be in this patch. It makes more sense to me in the previous one.

Ok I'll fold it into previous patch.
> 
> 
> Also, maybe adding some reference for easily seeing where the identifier comes from:
> 
> + * @port_num: hardware identifier for this dport in a decoder's target list

ok 


DJ

> 
> 
>>    * @rcrb: mandatory location of a Root Complex Register Block
>>    *
>>    * See CXL 3.0 9.11.8 CXL Devices Attached to an RCH
>>    */
>>   struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>> -                     struct device *dport_dev, int port_id,
>> +                     struct device *dport_dev, int port_num,
>>                        resource_size_t rcrb)
>>   {
>>       struct cxl_dport *dport;
>> @@ -1291,7 +1291,7 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>>           return ERR_PTR(-EINVAL);
>>       }
>>   -    dport = __devm_cxl_add_dport(port, dport_dev, port_id,
>> +    dport = __devm_cxl_add_dport(port, dport_dev, port_num,
>>                        CXL_RESOURCE_NONE, rcrb);
>>       if (IS_ERR(dport)) {
>>           dev_dbg(dport_dev, "failed to add RCH dport to %s: %ld\n",
>> @@ -1764,7 +1764,7 @@ static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
>>         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]);
>> +        struct cxl_dport *dport = find_dport_by_num(port, target_map[i]);
>>             if (!dport)
>>               return -ENXIO;

Re: [PATCH v2 05/10] cxl: Defer hardware dport->port_id assignment and registers probing

[Dave Jiang]

On 5/13/25 8:43 AM, Gregory Price wrote:
> On Tue, May 06, 2025 at 05:43:05PM -0700, Dave Jiang wrote:
>> Current implementation only enumerates the dports during the port 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 hw
>> dport id assignment and the register probing to behind memdev probe so the
>> endpoint is guaranteed to be connected.
>>
>> The detection of duplicate dport for add_dport() is removed. The port_id
>> is not read from the hw at this point any longer. The port->id will always
>> be unique since it's retrieved from an ida. The dup detection thus become
>> irrelevant.
>>
>> 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.
>>
> 
> I'm finding the changes a little difficult to follow, can you lay out
> the expected order of operations before and after?
> 
> Specifically there's two new guard() calls, can you explain under what
> conditions those can be contended?

I'll add more to the commit log and explain.
> 
> ~Gregory
> 
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 70173d23139c..04e18a102d26 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
> ... snip ...
>> +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_num) {
>> +			cxlsd->target[i] = dport;
>> +			return 0;
>> +		}
>> +	}
> ... snip ...
>> @@ -1695,6 +1798,19 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>>  				"found already registered port %s:%s\n",
>>  				dev_name(&port->dev),
>>  				dev_name(port->uport_dev));
>> +
>> +			/*
>> +			 * Attempt to do single pass dport setup by checking here
>> +			 * instead of doing it during port creation. Otherwise
>> +			 * it still needs to check here for dports that are
>> +			 * being probed with a port already created.
>> +			 */
>> +			scoped_guard(device, &port->dev) {
>> +				rc = cxl_switch_port_dport_setup(port, dport_dev);
>> +				if (rc)
>> +					return rc;
>> +			}
>> +
> ... snip ...
>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>> index a35fc5552845..4d840a6ef802 100644
>> --- a/drivers/cxl/port.c
>> +++ b/drivers/cxl/port.c
> ... snip ...
>>  	/* Cache the data early to ensure is_visible() works */
>> @@ -69,24 +68,7 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>>  	if (rc < 0)
>>  		return rc;
>>  
> ... snip ...
>> -	return -ENXIO;
>> +	return 0;
>>  }
> 
> 	return devm_cxl_port_enumerate_dports(port);

This was actually done on purpose. devm_cxl_port_enumerate_dports() returns the number of dports enumerated. So usually the return value is greater than 0. in drivers/base/dd.c, call_driver_probe() throws the return value into a switch() where any value not 0 are errors. So the probe() call would fail. Here we are intercepting the return value and return a 0 if it's positive. I got bitten here during this series's debug. I should add a comment and explain why.  


> 
> 
> ~Gregory

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

[Dave Jiang]

On 5/15/25 10:03 AM, Gregory Price wrote:
> On Tue, May 13, 2025 at 09:12:00AM -0700, Dave Jiang wrote:
>>
>>
>> On 5/13/25 8:49 AM, Gregory Price wrote:
>>> On Tue, May 06, 2025 at 05:43:08PM -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.
>>>>
>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>
>>> ignorant terminology question: what's the different between the
>>> "cxl hierarchy head" and "cxl root"?
>>
>> I struggle to find the correct terminology between the head of the PCI (or platform device for cxl_test) hierarchy vs the CXL one we constructed for the 'cxl_port' hierarchy. I'm open to suggestions to use better words to distinguish that. 
> 
> Is there an actual difference or they actually the same "device" - just
> abstracted differently?

One is ACPI1700:nn which is an ACPI device. The other is the PCIe root port and considered a pci_bridge device. So top of two different hierarchies. But what trips me up is cxl_test gets in the mix and this is the top of the platform device hierarchy. So finding a proper word to describe the top of that side is difficult. Otherwise I could just say pci_root or something. But I'm avoiding using the keyword "pci" in core/port.c. 

> 
> ~Gregory

Re: [PATCH v2 03/10] cxl: Rename find_dport() to provide better function intent

[Dave Jiang]

On 5/9/25 2:20 AM, Alejandro Lucero Palau wrote:
> 
> On 5/7/25 01:43, Dave Jiang wrote:
>> Rename find_dport() to find_dport_by_num() to indicate that the function
>> is trying to match a dport by its hardware number index.
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/cxl/core/port.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index e9c02e4d0d4c..1d7a4a2ef6ad 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -1045,7 +1045,7 @@ void put_cxl_root(struct cxl_root *cxl_root)
>>   }
>>   EXPORT_SYMBOL_NS_GPL(put_cxl_root, "CXL");
>>   -static struct cxl_dport *find_dport(struct cxl_port *port, int port_num)
>> +static struct cxl_dport *find_dport_by_num(struct cxl_port *port, int port_num)
>>   {
>>       struct cxl_dport *dport;
>>       unsigned long index;
>> @@ -1063,7 +1063,7 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *dport)
>>       int rc;
>>         device_lock_assert(&port->dev);
>> -    dup = find_dport(port, dport->port_num);
>> +    dup = find_dport_by_num(port, dport->port_num);
>>       if (dup) {
>>           dev_err(&port->dev,
>>               "unable to add dport%d-%s non-unique port num (%s)\n",
>> @@ -1275,13 +1275,13 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, "CXL");
>>    * devm_cxl_add_rch_dport - append RCH downstream port data to a cxl_port
>>    * @port: the cxl_port that references this dport
>>    * @dport_dev: firmware or PCI device representing the dport
>> - * @port_id: identifier for this dport in a decoder's target list
>> + * @port_num: identifier for this dport in a decoder's target list
> 
> 
> Not sure this change should be in this patch. It makes more sense to me in the previous one.
> 
> 
> Also, maybe adding some reference for easily seeing where the identifier comes from:
> 
> + * @port_num: hardware identifier for this dport in a decoder's target list

In the process of doing that, I realized that this entire comment line gets dropped in a later patch because we remove the port_num parameter. So probably no need to change since it goes away entirely.

DJ

> 
> 
>>    * @rcrb: mandatory location of a Root Complex Register Block
>>    *
>>    * See CXL 3.0 9.11.8 CXL Devices Attached to an RCH
>>    */
>>   struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>> -                     struct device *dport_dev, int port_id,
>> +                     struct device *dport_dev, int port_num,
>>                        resource_size_t rcrb)
>>   {
>>       struct cxl_dport *dport;
>> @@ -1291,7 +1291,7 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>>           return ERR_PTR(-EINVAL);
>>       }
>>   -    dport = __devm_cxl_add_dport(port, dport_dev, port_id,
>> +    dport = __devm_cxl_add_dport(port, dport_dev, port_num,
>>                        CXL_RESOURCE_NONE, rcrb);
>>       if (IS_ERR(dport)) {
>>           dev_dbg(dport_dev, "failed to add RCH dport to %s: %ld\n",
>> @@ -1764,7 +1764,7 @@ static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
>>         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]);
>> +        struct cxl_dport *dport = find_dport_by_num(port, target_map[i]);
>>             if (!dport)
>>               return -ENXIO;

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

[Jonathan Cameron]

On Tue, 6 May 2025 17:43:01 -0700
Dave Jiang <dave.jiang@intel.com> 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.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Stands on it's own so maybe queue this up even if later patches
are still under discussion?

I'm keen to reduce what is floating around in a good state!

Jonathan

> ---
>  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 c3f4dc244df7..a91d4eb061e4 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -788,7 +788,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
> 
> base-commit: 3c746d4821f507304b08789e3c7c151554fc2356

Re: [PATCH v2 02/10] cxl: Saperate out CXL dport->id vs actual dport hardware id

[Jonathan Cameron]

On Tue, 6 May 2025 17:43:02 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> In preparation to allow dport to be allocated without being active, make
> dport->id to be Linux id that enumerates the dport objects per port.
> Keep the hardware id under dport->port_num to maintain compatibility and
> introduce a dport->id as the enumeration id.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Given you already fixed the typo in title.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v2 03/10] cxl: Rename find_dport() to provide better function intent

[Jonathan Cameron]

On Mon, 19 May 2025 09:33:26 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 5/9/25 2:20 AM, Alejandro Lucero Palau wrote:
> > 
> > On 5/7/25 01:43, Dave Jiang wrote:  
> >> Rename find_dport() to find_dport_by_num() to indicate that the function
> >> is trying to match a dport by its hardware number index.
> >>
> >> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >> ---
> >>   drivers/cxl/core/port.c | 12 ++++++------
> >>   1 file changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> >> index e9c02e4d0d4c..1d7a4a2ef6ad 100644
> >> --- a/drivers/cxl/core/port.c
> >> +++ b/drivers/cxl/core/port.c
> >> @@ -1045,7 +1045,7 @@ void put_cxl_root(struct cxl_root *cxl_root)
> >>   }
> >>   EXPORT_SYMBOL_NS_GPL(put_cxl_root, "CXL");
> >>   -static struct cxl_dport *find_dport(struct cxl_port *port, int port_num)
> >> +static struct cxl_dport *find_dport_by_num(struct cxl_port *port, int port_num)
> >>   {
> >>       struct cxl_dport *dport;
> >>       unsigned long index;
> >> @@ -1063,7 +1063,7 @@ static int add_dport(struct cxl_port *port, struct cxl_dport *dport)
> >>       int rc;
> >>         device_lock_assert(&port->dev);
> >> -    dup = find_dport(port, dport->port_num);
> >> +    dup = find_dport_by_num(port, dport->port_num);
> >>       if (dup) {
> >>           dev_err(&port->dev,
> >>               "unable to add dport%d-%s non-unique port num (%s)\n",
> >> @@ -1275,13 +1275,13 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, "CXL");
> >>    * devm_cxl_add_rch_dport - append RCH downstream port data to a cxl_port
> >>    * @port: the cxl_port that references this dport
> >>    * @dport_dev: firmware or PCI device representing the dport
> >> - * @port_id: identifier for this dport in a decoder's target list
> >> + * @port_num: identifier for this dport in a decoder's target list  
> > 
> > 
> > Not sure this change should be in this patch. It makes more sense to me in the previous one.
> > 
> > 
> > Also, maybe adding some reference for easily seeing where the identifier comes from:
> > 
> > + * @port_num: hardware identifier for this dport in a decoder's target list  
> 
> In the process of doing that, I realized that this entire comment line gets dropped in a later patch because we remove the port_num parameter. So probably no need to change since it goes away entirely.
>
Even though it's only a comment, if you are changing the name in
the function let's avoid the trivial issue of the kernel-doc
warning that will show up.  Make sure those are kept inline.

I'll assume you'll tidy this up one way or another - rest of patch is fine

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

> DJ
> 
> > 
> >   
> >>    * @rcrb: mandatory location of a Root Complex Register Block
> >>    *
> >>    * See CXL 3.0 9.11.8 CXL Devices Attached to an RCH
> >>    */
> >>   struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
> >> -                     struct device *dport_dev, int port_id,
> >> +                     struct device *dport_dev, int port_num,
> >>                        resource_size_t rcrb)
> >>   {
> >>       struct cxl_dport *dport;
> >> @@ -1291,7 +1291,7 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
> >>           return ERR_PTR(-EINVAL);
> >>       }
> >>   -    dport = __devm_cxl_add_dport(port, dport_dev, port_id,
> >> +    dport = __devm_cxl_add_dport(port, dport_dev, port_num,
> >>                        CXL_RESOURCE_NONE, rcrb);
> >>       if (IS_ERR(dport)) {
> >>           dev_dbg(dport_dev, "failed to add RCH dport to %s: %ld\n",
> >> @@ -1764,7 +1764,7 @@ static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
> >>         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]);
> >> +        struct cxl_dport *dport = find_dport_by_num(port, target_map[i]);
> >>             if (!dport)
> >>               return -ENXIO;  
> 
> 

Re: [PATCH v2 04/10] cxl: Remove adding of port_num via devm_cxl_add_dport()

[Jonathan Cameron]

On Tue, 6 May 2025 17:43:04 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> In preparation for delayed dport probing, remove setting of port_num
> through devm_cxl_add_dport(). Will temporarily set the port_num after
> dport is added for now.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v2 05/10] cxl: Defer hardware dport->port_id assignment and registers probing

[Jonathan Cameron]

  
> >> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> >> index a35fc5552845..4d840a6ef802 100644
> >> --- a/drivers/cxl/port.c
> >> +++ b/drivers/cxl/port.c  
> > ... snip ...  
> >>  	/* Cache the data early to ensure is_visible() works */
> >> @@ -69,24 +68,7 @@ static int cxl_switch_port_probe(struct cxl_port *port)
> >>  	if (rc < 0)
> >>  		return rc;
> >>    
> > ... snip ...  
> >> -	return -ENXIO;
> >> +	return 0;
> >>  }  
> > 
> > 	return devm_cxl_port_enumerate_dports(port);  
> 
> This was actually done on purpose. devm_cxl_port_enumerate_dports() returns the number of dports enumerated. So usually the return value is greater than 0. in drivers/base/dd.c, call_driver_probe() throws the return value into a switch() where any value not 0 are errors. So the probe() call would fail. Here we are intercepting the return value and return a 0 if it's positive. I got bitten here during this series's debug. I should add a comment and explain why.  

I'll ask a 'silly' follow up.  Why does devm_cxl_port_enumerate_dports()
return the number.  From a quick look does anyone use it?  If not
just change that as a precursor patch and allow this
tiny bit of code improvement.

> 
> 
> > 
> > 
> > ~Gregory  
> 
> 

Re: [PATCH v2 05/10] cxl: Defer hardware dport->port_id assignment and registers probing

[Jonathan Cameron]

On Tue, 6 May 2025 17:43:05 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Current implementation only enumerates the dports during the port 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 hw
> dport id assignment and the register probing to behind memdev probe so the
> endpoint is guaranteed to be connected.
> 
> The detection of duplicate dport for add_dport() is removed. The port_id
> is not read from the hw at this point any longer. The port->id will always
> be unique since it's retrieved from an ida. The dup detection thus become
> irrelevant.
> 
> 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.
> 
> Link: https://lore.kernel.org/linux-cxl/20250305100123.3077031-1-rrichter@amd.com/
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
A few trivial things inline.

> ---
> v2:
> - Fix spelling error in commit message. (Ming)
> - Move probing to the specific dport. (Ming)
> - Add decoder update.
> - Return errors when setup dports. (Jonathan)
> - Add driver check before setup dport. (Dan)
> - Add comment on why location to do dport update. (Dan)
> ---
>  drivers/cxl/core/core.h |   4 ++
>  drivers/cxl/core/pci.c  |  60 ++++++++++++----
>  drivers/cxl/core/port.c | 152 +++++++++++++++++++++++++++++++++++-----
>  drivers/cxl/cxl.h       |   4 ++
>  drivers/cxl/port.c      |  20 +-----
>  5 files changed, 192 insertions(+), 48 deletions(-)
> 

> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 3b84b43ab194..16bd842904e5 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -24,6 +24,52 @@ 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");
>  
> +int cxl_dport_setup(struct cxl_dport *dport)
> +{
> +	struct device *dport_dev = dport->dport_dev;
> +	struct cxl_port *port = dport->port;
> +	struct cxl_register_map map;
> +	struct pci_dev *pdev;
> +	u32 lnkcap, port_num;
> +	int rc;
> +

I assume this is a cxl test thing?  If so maybe throw a comment in
as I at least sometimes forget that exists and wonder how on earth
we got here with something that wasn't a pci device.


> +	if (!dev_is_pci(dport_dev))
> +		return 0;
> +
> +	/*
> +	 * dport->port_id is valid means that dport has been probed and is
> +	 * setup.
> +	 */
> +	if (dport->port_num != CXL_DPORT_NUM_INVALID)
> +		return 0;
> +
> +	pdev = to_pci_dev(dport_dev);
> +	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
> +				  &lnkcap))
> +		return -ENODEV;
> +
> +	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
> +	if (rc) {
> +		dev_dbg(&port->dev, "failed to find component registers\n");
> +		return -ENODEV;
> +	}
> +
> +	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);

I'd grab this where you read link cap above, or just before use below.
Here it's not associated with anything in particular so just feels
odd. I'd guess this is down to some code reorg leaving it somewhat
stranded (or a change in a later patch that makes this location an obviou
choice?)


> +	rc = cxl_dport_setup_regs(&port->dev, dport, map.resource);
> +	if (rc)
> +		return rc;
> +
> +	if (map.resource != CXL_RESOURCE_NONE) {
> +		dev_dbg(dport->dport_dev,
> +			"Component Register found for dport: %pa\n",
> +			&map.resource);
> +	}
> +
> +	dport->port_num = port_num;
> +
> +	return 0;
> +}

> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 70173d23139c..04e18a102d26 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c


> +static int port_probe_dport(struct cxl_port *port, struct device *dport_dev,
> +			    struct cxl_dport **probed_dport)
> +{
> +	struct cxl_dport *dport;
> +	unsigned long index;
> +	int rc;
> +
> +	device_lock_assert(&port->dev);
> +	xa_for_each(&port->dports, index, dport) {
> +		if (dport->dport_dev == dport_dev) {

Maybe flip?  The code indent isn't huge though so up to you.

		if (!dport->dport_dev != dport_dev)
			continue;

> +			rc = cxl_dport_setup(dport);
> +			if (rc)
> +				return rc;

I'd throw in a blank line here.

> +			*probed_dport = dport;

and here in the interests of minor readability improvement

> +			return 0;
> +		}
> +	}
> +
> +	*probed_dport = NULL;
> +
> +	return -ENODEV;
> +}


> +
> +static int cxl_switch_port_dport_setup(struct cxl_port *port,
> +				       struct device *dport_dev)
> +{
> +	struct cxl_dport *dport;
> +	int rc;
> +
> +	device_lock_assert(&port->dev);
> +
> +	if (!port->dev.driver)
> +		return -ENODEV;
> +
> +	rc = port_probe_dport(port, dport_dev, &dport);
> +	if (rc)
> +		return rc;
> +
> +	cxl_switch_parse_cdat(port);
> +
> +	/* Make sure that no decoders have been allocated before proceeding. */
> +	if (ida_is_empty(&port->decoder_ida))
> +		rc = devm_cxl_port_setup_decoders(port);
> +	else
> +		rc = update_decoders_with_dport(port, dport);
> +	if (rc)
> +		return rc;
> +
> +	return 0;

return rc?  or just return in each branch perhaps.

> +}
> +

Re: [PATCH v2 06/10] cxl/test: Add workaround for cxl_test for cxl_core calling mocked functions

[Jonathan Cameron]

On Tue, 6 May 2025 17:43:06 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> 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 host bridge uport mapping update changes in order to enable
> cxl-test.
> 
> The following functions are being modified:
> devm_cxl_add_passthrough_decoder()
> devm_cxl_setup_hdm()
> devm_cxl_enumerate_decoders()
> 
> 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.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Hmm. I think I follow this and it seems ok + I assume you tested it extensively ;)


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

> ---
> - Reverse unregister order. (Jonathan)
> - Move the bits only needed by cxl_test to cxl_test. (Dan)
> - Add more comments in the commit log. (Jonathan)
> ---
>  drivers/cxl/core/hdm.c               | 27 ++++++++++++---------
>  drivers/cxl/cxl.h                    | 25 ++++++++++++++++++++
>  tools/testing/cxl/Kbuild             |  4 +---
>  tools/testing/cxl/cxl_core_exports.c | 31 ++++++++++++++++++++++++
>  tools/testing/cxl/exports.h          | 17 ++++++++++++++
>  tools/testing/cxl/test/mock.c        | 35 +++++++++++++++++++---------
>  6 files changed, 114 insertions(+), 25 deletions(-)
>  create mode 100644 tools/testing/cxl/exports.h
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index d7ad92e191ba..76cb6cf22c62 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -39,14 +39,19 @@ static int add_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  	return 0;
>  }
>  
> -/*
> +/**
> + * __devm_cxl_add_passthrough_decoder - Add passthrough decoder
> + * @port: The cxl_port context
> + *
> + * Return 0 on success or errno on failure.
> + *
>   * Per the CXL specification (8.2.5.12 CXL HDM Decoder Capability Structure)
>   * single ported host-bridges need not publish a decoder capability when a
>   * passthrough decode can be assumed, i.e. all transactions that the uport sees
>   * are claimed and passed to the single dport. Disable the range until the first
>   * CXL region is enumerated / activated.
>   */
> -int devm_cxl_add_passthrough_decoder(struct cxl_port *port)
> +int __devm_cxl_add_passthrough_decoder(struct cxl_port *port)
>  {
>  	struct cxl_switch_decoder *cxlsd;
>  	struct cxl_dport *dport = NULL;
> @@ -73,7 +78,7 @@ int devm_cxl_add_passthrough_decoder(struct cxl_port *port)
>  
>  	return add_hdm_decoder(port, &cxlsd->cxld, single_port_map);
>  }
> -EXPORT_SYMBOL_NS_GPL(devm_cxl_add_passthrough_decoder, "CXL");
> +EXPORT_SYMBOL_NS_GPL(__devm_cxl_add_passthrough_decoder, "CXL");
>  
>  static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>  {
> @@ -139,12 +144,12 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
>  }
>  
>  /**
> - * devm_cxl_setup_hdm - map HDM decoder component registers
> + * __devm_cxl_setup_hdm - map HDM decoder component registers
>   * @port: cxl_port to map
>   * @info: cached DVSEC range register info
>   */
> -struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
> -				   struct cxl_endpoint_dvsec_info *info)
> +struct cxl_hdm *__devm_cxl_setup_hdm(struct cxl_port *port,
> +				     struct cxl_endpoint_dvsec_info *info)
>  {
>  	struct cxl_register_map *reg_map = &port->reg_map;
>  	struct device *dev = &port->dev;
> @@ -199,7 +204,7 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>  
>  	return cxlhdm;
>  }
> -EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_hdm, "CXL");
> +EXPORT_SYMBOL_NS_GPL(__devm_cxl_setup_hdm, "CXL");
>  
>  static void __cxl_dpa_debug(struct seq_file *file, struct resource *r, int depth)
>  {
> @@ -1150,12 +1155,12 @@ static void cxl_settle_decoders(struct cxl_hdm *cxlhdm)
>  }
>  
>  /**
> - * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
> + * __devm_cxl_enumerate_decoders - add decoder objects per HDM register set
>   * @cxlhdm: Structure to populate with HDM capabilities
>   * @info: cached DVSEC range register info
>   */
> -int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> -				struct cxl_endpoint_dvsec_info *info)
> +int __devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> +				  struct cxl_endpoint_dvsec_info *info)
>  {
>  	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
>  	struct cxl_port *port = cxlhdm->port;
> @@ -1212,4 +1217,4 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_decoders, "CXL");
> +EXPORT_SYMBOL_NS_GPL(__devm_cxl_enumerate_decoders, "CXL");
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 6b85d5f23be0..4d0ddb506ec5 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -810,9 +810,17 @@ struct cxl_endpoint_dvsec_info {
>  struct cxl_hdm;
>  struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>  				   struct cxl_endpoint_dvsec_info *info);
> +struct cxl_hdm *__devm_cxl_setup_hdm(struct cxl_port *port,
> +				     struct cxl_endpoint_dvsec_info *info);
> +
>  int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>  				struct cxl_endpoint_dvsec_info *info);
> +int __devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> +				  struct cxl_endpoint_dvsec_info *info);
> +
>  int devm_cxl_add_passthrough_decoder(struct cxl_port *port);
> +int __devm_cxl_add_passthrough_decoder(struct cxl_port *port);
> +
>  struct cxl_dev_state;
>  int cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds,
>  			struct cxl_endpoint_dvsec_info *info);
> @@ -921,4 +929,21 @@ bool dev_is_cxl_root_child(struct device *dev);
>  
>  u16 cxl_gpf_get_dvsec(struct device *dev);
>  
> +/*
> + * 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_enumerate_decoders DECLARE_TESTABLE(devm_cxl_enumerate_decoders)
> +#define devm_cxl_setup_hdm DECLARE_TESTABLE(devm_cxl_setup_hdm)
> +#define devm_cxl_add_passthrough_decoder DECLARE_TESTABLE(devm_cxl_add_passthrough_decoder)
> +#endif
> +
>  #endif /* __CXL_H__ */
> diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
> index 387f3df8b988..8bf786eef9f1 100644
> --- a/tools/testing/cxl/Kbuild
> +++ b/tools/testing/cxl/Kbuild
> @@ -5,9 +5,6 @@ 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=devm_cxl_setup_hdm
> -ldflags-y += --wrap=devm_cxl_add_passthrough_decoder
> -ldflags-y += --wrap=devm_cxl_enumerate_decoders
>  ldflags-y += --wrap=cxl_await_media_ready
>  ldflags-y += --wrap=cxl_hdm_decode_init
>  ldflags-y += --wrap=cxl_dvsec_rr_decode
> @@ -21,6 +18,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..9a5f2e641897 100644
> --- a/tools/testing/cxl/cxl_core_exports.c
> +++ b/tools/testing/cxl/cxl_core_exports.c
> @@ -2,6 +2,37 @@
>  /* 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_pt_decoder_fn _devm_cxl_add_passthrough_decoder =
> +	__devm_cxl_add_passthrough_decoder;
> +EXPORT_SYMBOL_NS_GPL(_devm_cxl_add_passthrough_decoder, "CXL");
> +
> +int devm_cxl_add_passthrough_decoder(struct cxl_port *port)
> +{
> +	return _devm_cxl_add_passthrough_decoder(port);
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_add_passthrough_decoder, "CXL");
> +
> +cxl_setup_hdm_fn _devm_cxl_setup_hdm = __devm_cxl_setup_hdm;
> +EXPORT_SYMBOL_NS_GPL(_devm_cxl_setup_hdm, "CXL");
> +
> +struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
> +				   struct cxl_endpoint_dvsec_info *info)
> +{
> +	return _devm_cxl_setup_hdm(port, info);
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_hdm, "CXL");
> +
> +cxl_enum_decoders_fn _devm_cxl_enumerate_decoders = __devm_cxl_enumerate_decoders;
> +EXPORT_SYMBOL_NS_GPL(_devm_cxl_enumerate_decoders, "CXL");
> +
> +int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> +				struct cxl_endpoint_dvsec_info *info)
> +{
> +	return _devm_cxl_enumerate_decoders(cxlhdm, info);
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_decoders, "CXL");
> diff --git a/tools/testing/cxl/exports.h b/tools/testing/cxl/exports.h
> new file mode 100644
> index 000000000000..036fe85e1ad9
> --- /dev/null
> +++ b/tools/testing/cxl/exports.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright(c) 2025 Intel Corporation */
> +#ifndef __MOCK_CXL_EXPORTS_H_
> +#define __MOCK_CXL_EXPORTS_H_
> +
> +typedef struct cxl_hdm *(*cxl_setup_hdm_fn)(struct cxl_port *port,
> +					    struct cxl_endpoint_dvsec_info *info);
> +extern cxl_setup_hdm_fn _devm_cxl_setup_hdm;
> +
> +typedef int (*cxl_enum_decoders_fn)(struct cxl_hdm *cxlhdm,
> +				    struct cxl_endpoint_dvsec_info *info);
> +extern cxl_enum_decoders_fn _devm_cxl_enumerate_decoders;
> +
> +typedef int (*cxl_add_pt_decoder_fn)(struct cxl_port *port);
> +extern cxl_add_pt_decoder_fn _devm_cxl_add_passthrough_decoder;
> +
> +#endif
> diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
> index d9e8940d9328..c4367c4b5278 100644
> --- a/tools/testing/cxl/test/mock.c
> +++ b/tools/testing/cxl/test/mock.c
> @@ -10,12 +10,25 @@
>  #include <cxlmem.h>
>  #include <cxlpci.h>
>  #include "mock.h"
> +#include "../exports.h"
>  
>  static LIST_HEAD(mock);
>  
> +static struct cxl_hdm *
> +redirect_devm_cxl_setup_hdm(struct cxl_port *port,
> +			    struct cxl_endpoint_dvsec_info *info);
> +static int
> +redirect_devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> +				     struct cxl_endpoint_dvsec_info *info);
> +static int redirect_devm_cxl_add_passthrough_decoder(struct cxl_port *port);
> +
>  void register_cxl_mock_ops(struct cxl_mock_ops *ops)
>  {
>  	list_add_rcu(&ops->list, &mock);
> +	_devm_cxl_setup_hdm = redirect_devm_cxl_setup_hdm;
> +	_devm_cxl_enumerate_decoders = redirect_devm_cxl_enumerate_decoders;
> +	_devm_cxl_add_passthrough_decoder =
> +		redirect_devm_cxl_add_passthrough_decoder;
>  }
>  EXPORT_SYMBOL_GPL(register_cxl_mock_ops);
>  
> @@ -23,6 +36,9 @@ DEFINE_STATIC_SRCU(cxl_mock_srcu);
>  
>  void unregister_cxl_mock_ops(struct cxl_mock_ops *ops)
>  {
> +	_devm_cxl_add_passthrough_decoder = __devm_cxl_add_passthrough_decoder;
> +	_devm_cxl_enumerate_decoders = __devm_cxl_enumerate_decoders;
> +	_devm_cxl_setup_hdm = __devm_cxl_setup_hdm;
>  	list_del_rcu(&ops->list);
>  	synchronize_srcu(&cxl_mock_srcu);
>  }
> @@ -131,8 +147,8 @@ __wrap_nvdimm_bus_register(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(__wrap_nvdimm_bus_register);
>  
> -struct cxl_hdm *__wrap_devm_cxl_setup_hdm(struct cxl_port *port,
> -					  struct cxl_endpoint_dvsec_info *info)
> +struct cxl_hdm *redirect_devm_cxl_setup_hdm(struct cxl_port *port,
> +					    struct cxl_endpoint_dvsec_info *info)
>  
>  {
>  	int index;
> @@ -142,14 +158,13 @@ struct cxl_hdm *__wrap_devm_cxl_setup_hdm(struct cxl_port *port,
>  	if (ops && ops->is_mock_port(port->uport_dev))
>  		cxlhdm = ops->devm_cxl_setup_hdm(port, info);
>  	else
> -		cxlhdm = devm_cxl_setup_hdm(port, info);
> +		cxlhdm = __devm_cxl_setup_hdm(port, info);
>  	put_cxl_mock_ops(index);
>  
>  	return cxlhdm;
>  }
> -EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_setup_hdm, "CXL");
>  
> -int __wrap_devm_cxl_add_passthrough_decoder(struct cxl_port *port)
> +int redirect_devm_cxl_add_passthrough_decoder(struct cxl_port *port)
>  {
>  	int rc, index;
>  	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
> @@ -157,15 +172,14 @@ int __wrap_devm_cxl_add_passthrough_decoder(struct cxl_port *port)
>  	if (ops && ops->is_mock_port(port->uport_dev))
>  		rc = ops->devm_cxl_add_passthrough_decoder(port);
>  	else
> -		rc = devm_cxl_add_passthrough_decoder(port);
> +		rc = __devm_cxl_add_passthrough_decoder(port);
>  	put_cxl_mock_ops(index);
>  
>  	return rc;
>  }
> -EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_add_passthrough_decoder, "CXL");
>  
> -int __wrap_devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> -				       struct cxl_endpoint_dvsec_info *info)
> +int redirect_devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> +					 struct cxl_endpoint_dvsec_info *info)
>  {
>  	int rc, index;
>  	struct cxl_port *port = cxlhdm->port;
> @@ -174,12 +188,11 @@ int __wrap_devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>  	if (ops && ops->is_mock_port(port->uport_dev))
>  		rc = ops->devm_cxl_enumerate_decoders(cxlhdm, info);
>  	else
> -		rc = devm_cxl_enumerate_decoders(cxlhdm, info);
> +		rc = __devm_cxl_enumerate_decoders(cxlhdm, info);
>  	put_cxl_mock_ops(index);
>  
>  	return rc;
>  }
> -EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_enumerate_decoders, "CXL");
>  
>  int __wrap_devm_cxl_port_enumerate_dports(struct cxl_port *port)
>  {

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

[Jonathan Cameron]

On Tue, 6 May 2025 17:43:07 -0700
Dave Jiang <dave.jiang@intel.com> 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.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Seems reasonable.  Any reason not to yank this one out before
the rest of the series as a bit of cleanup?

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

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

[Jonathan Cameron]

On Tue, 6 May 2025 17:43:08 -0700
Dave Jiang <dave.jiang@intel.com> 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.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Another one that I think can be yanked out ahead of the
main series so we can focus on the more substantial stuff.

Reviewed-by: Jonathan Cameron <Jonsathan.Cameron@huawei.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 e212bc2faada..259b217e812f 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);
> @@ -1774,11 +1783,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;

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

[Jonathan Cameron]

On Tue, 6 May 2025 17:43:09 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add helper functions to setup association of a host bridge device to a
> related cxl_root. Functions are in preparation to support the moving
> of host bridge ports creation from cxl_acpi to cxl_memdev probe path.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
I've nothing to add to Gregory's naming comments beyond agreeing that
it could do to be a little more obvious what is what here!

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

[Jonathan Cameron]

On Tue, 6 May 2025 17:43:10 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

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

I think we should call out that this bit (unlike port numbers) is valid
under the CXL spec.  Whilst I think that statement in the spec is something
I'd rather wasn't there we should reflect this one isn't a hardware bug
work around (unlike port number which I think is :)


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

This is a fairly fiddly change but it looks reasonable.

Just trivial style comments inline.

J
> ---
>  drivers/cxl/acpi.c      | 136 ++++++++++++++++++++++++----------------
>  drivers/cxl/core/port.c |  58 +++++++++++++++++
>  drivers/cxl/cxl.h       |   2 +
>  3 files changed, 141 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 6f8630e50800..1db4d308b4b7 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -298,8 +298,79 @@ 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 decode and it is a uport to the

decoder maybe?

> + * dport (PCIe Root Ports) in the host bridge.
> + */
> +static int cxl_acpi_setup_hostbridge_uport(struct cxl_root *cxl_root,

Pity this doesn't sit in similar place to original add_host_bridge_uport
as we'd get a much nicer diff if it could.
I suppose it would be a bit too ugly to preceded this patch with
a code move patch just for that diff.  Ah well. I'll cope ;)

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

> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index a5a673d789f3..bbecbb04b6be 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1808,6 +1808,60 @@ static int cxl_switch_port_dport_setup(struct cxl_port *port,
>  	return 0;
>  }
>  
> +static int get_hostbridge_port_devices(struct cxl_memdev *cxlmd,
> +				       struct device **uport_dev,
> +				       struct device **dport_dev)
> +{
> +	struct device *dev = &cxlmd->dev;
> +	struct device *iter;
> +
> +	for (iter = dev; iter; iter = grandparent(iter)) {
> +		struct device *ddev = grandparent(iter);
> +		struct device *udev;
> +
> +		udev = ddev->parent;

Odd to have ddev set at declaration and udev set here.
Pick a style - either is fine.

> +		if (is_cxl_hierarchy_head(udev->parent)) {
> +			*uport_dev = udev;
> +			*dport_dev = ddev;
> +			return 0;
> +		}
> +	}
> +
> +	return -ENODEV;
> +}
> +
> +static int cxl_hostbridge_port_setup(struct cxl_memdev *cxlmd)
> +{
> +	struct device *uport_dev, *dport_dev;
> +	struct cxl_dport *dport;
> +	struct cxl_port *port;
> +	int rc;
> +
> +	rc = get_hostbridge_port_devices(cxlmd, &uport_dev, &dport_dev);
> +	if (rc)
> +		return -ENODEV;
> +
> +	struct cxl_root *cxl_root __free(put_cxl_root) = cxl_udev_to_root(uport_dev);
> +	if (!cxl_root)
> +		return -ENODEV;
> +
> +	guard(device)(&cxl_root->port.dev);

> +	port = find_cxl_port(dport_dev, &dport);

I vaguely wonder if a __free() make sense on this. It'll autofree the NULL
much later than needed but maybe it's cleaner code?

> +	if (port) {
> +		put_device(&port->dev);
> +		return 0;
> +	}
> +
> +	if (!cxl_root->ops || !cxl_root->ops->setup_hostbridge_uport)
> +		return -EOPNOTSUPP;
> +
> +	rc = cxl_root->ops->setup_hostbridge_uport(cxl_root, uport_dev);
> +	if (rc)
> +		return rc;
> +
> +	return 0;

return cxl_root->ops....

> +}

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

[Dave Jiang]

On 5/20/25 4:14 AM, Jonathan Cameron wrote:
> On Tue, 6 May 2025 17:43:01 -0700
> Dave Jiang <dave.jiang@intel.com> 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.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks Jonathan
> 
> Stands on it's own so maybe queue this up even if later patches
> are still under discussion?
> 
> I'm keen to reduce what is floating around in a good state!

I originally submitted this stand alone and Dan said to include it with the series. The issue does not show up until we change the behavior of port enumeration.

DJ

> 
> Jonathan
> 
>> ---
>>  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 c3f4dc244df7..a91d4eb061e4 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -788,7 +788,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
>>
>> base-commit: 3c746d4821f507304b08789e3c7c151554fc2356
> 

Re: [PATCH v2 05/10] cxl: Defer hardware dport->port_id assignment and registers probing

[Dave Jiang]

On 5/20/25 4:26 AM, Jonathan Cameron wrote:
>   
>>>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>>>> index a35fc5552845..4d840a6ef802 100644
>>>> --- a/drivers/cxl/port.c
>>>> +++ b/drivers/cxl/port.c  
>>> ... snip ...  
>>>>  	/* Cache the data early to ensure is_visible() works */
>>>> @@ -69,24 +68,7 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>>>>  	if (rc < 0)
>>>>  		return rc;
>>>>    
>>> ... snip ...  
>>>> -	return -ENXIO;
>>>> +	return 0;
>>>>  }  
>>>
>>> 	return devm_cxl_port_enumerate_dports(port);  
>>
>> This was actually done on purpose. devm_cxl_port_enumerate_dports() returns the number of dports enumerated. So usually the return value is greater than 0. in drivers/base/dd.c, call_driver_probe() throws the return value into a switch() where any value not 0 are errors. So the probe() call would fail. Here we are intercepting the return value and return a 0 if it's positive. I got bitten here during this series's debug. I should add a comment and explain why.  
> 
> I'll ask a 'silly' follow up.  Why does devm_cxl_port_enumerate_dports()
> return the number.  From a quick look does anyone use it?  If not
> just change that as a precursor patch and allow this
> tiny bit of code improvement.

I think originally the return value was used to determine of the decoder is passthrough or not. I guess with the new behavior, I don't have access to that and use the dports_ida to do the same thing. We can have it return 0 going forward.

DJ

> 
>>
>>
>>>
>>>
>>> ~Gregory  
>>
>>
> 

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

[Dave Jiang]

On 5/20/25 5:32 AM, Jonathan Cameron wrote:
> On Tue, 6 May 2025 17:43:07 -0700
> Dave Jiang <dave.jiang@intel.com> 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.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Seems reasonable.  Any reason not to yank this one out before
> the rest of the series as a bit of cleanup?

Well given it's only necessary because of the new behavior, probably should leave with the series. Previously it made sense because all the dports have arrived already.

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

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

[Dave Jiang]

On 5/20/25 5:34 AM, Jonathan Cameron wrote:
> On Tue, 6 May 2025 17:43:08 -0700
> Dave Jiang <dave.jiang@intel.com> 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.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> Another one that I think can be yanked out ahead of the
> main series so we can focus on the more substantial stuff.

Yeah I'll move this to the beginning.
> 
> Reviewed-by: Jonathan Cameron <Jonsathan.Cameron@huawei.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 e212bc2faada..259b217e812f 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);
>> @@ -1774,11 +1783,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;
> 
> 

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

[Dave Jiang]

On 5/20/25 6:11 AM, Jonathan Cameron wrote:
> On Tue, 6 May 2025 17:43:10 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Current enuemration scheme in cxl_acpi module creates the ports under the
>> root port by enumerating the hostbridges after the dports under the root
>> port is created. However error messages "cxl portN: Couldn't locate the
>> CXL.cache and CXL.mem capability array header" is observed when certain
>> platform has PCIe hotplug option turned on in BIOS. If the cxl_acpi module
>> probe is running before the CXL link between the endpoint device and the
>> RP is established, then the platform may not have exposed DVSEC ID 3 and/or
>> DVSEC ID 7 blocks which will trigger the error message.
> 
> I think we should call out that this bit (unlike port numbers) is valid
> under the CXL spec.  Whilst I think that statement in the spec is something
> I'd rather wasn't there we should reflect this one isn't a hardware bug
> work around (unlike port number which I think is :)

ok
> 
> 
>>
>> Setup an association in cxl_port to tie the host bridge device to the
>> associated cxl_root. The cxl_root provides a callback that's setup
>> by the cxl_acpi probe function in order to create a port per host bridge
>> that was previously done during cxl_acpi probe. Add the calling of the
>> callback in devm_cxl_enumerate_ports(). The observed behavior is that
>> ports that are not connected to endpoint device(s) are no longer
>> enumerated. This should also remove any excessive noise of port probe
>> failing on those inactive ports.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> This is a fairly fiddly change but it looks reasonable.
> 
> Just trivial style comments inline.
> 
> J
>> ---
>>  drivers/cxl/acpi.c      | 136 ++++++++++++++++++++++++----------------
>>  drivers/cxl/core/port.c |  58 +++++++++++++++++
>>  drivers/cxl/cxl.h       |   2 +
>>  3 files changed, 141 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
>> index 6f8630e50800..1db4d308b4b7 100644
>> --- a/drivers/cxl/acpi.c
>> +++ b/drivers/cxl/acpi.c
>> @@ -298,8 +298,79 @@ 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 decode and it is a uport to the
> 
> decoder maybe?

yes

> 
>> + * dport (PCIe Root Ports) in the host bridge.
>> + */
>> +static int cxl_acpi_setup_hostbridge_uport(struct cxl_root *cxl_root,
> 
> Pity this doesn't sit in similar place to original add_host_bridge_uport
> as we'd get a much nicer diff if it could.
> I suppose it would be a bit too ugly to preceded this patch with
> a code move patch just for that diff.  Ah well. I'll cope ;)
> 
>> +					   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;
>> +
>> +	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;
>> +}
> 
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index a5a673d789f3..bbecbb04b6be 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -1808,6 +1808,60 @@ static int cxl_switch_port_dport_setup(struct cxl_port *port,
>>  	return 0;
>>  }
>>  
>> +static int get_hostbridge_port_devices(struct cxl_memdev *cxlmd,
>> +				       struct device **uport_dev,
>> +				       struct device **dport_dev)
>> +{
>> +	struct device *dev = &cxlmd->dev;
>> +	struct device *iter;
>> +
>> +	for (iter = dev; iter; iter = grandparent(iter)) {
>> +		struct device *ddev = grandparent(iter);
>> +		struct device *udev;
>> +
>> +		udev = ddev->parent;
> 
> Odd to have ddev set at declaration and udev set here.
> Pick a style - either is fine.

ok

> 
>> +		if (is_cxl_hierarchy_head(udev->parent)) {
>> +			*uport_dev = udev;
>> +			*dport_dev = ddev;
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	return -ENODEV;
>> +}
>> +
>> +static int cxl_hostbridge_port_setup(struct cxl_memdev *cxlmd)
>> +{
>> +	struct device *uport_dev, *dport_dev;
>> +	struct cxl_dport *dport;
>> +	struct cxl_port *port;
>> +	int rc;
>> +
>> +	rc = get_hostbridge_port_devices(cxlmd, &uport_dev, &dport_dev);
>> +	if (rc)
>> +		return -ENODEV;
>> +
>> +	struct cxl_root *cxl_root __free(put_cxl_root) = cxl_udev_to_root(uport_dev);
>> +	if (!cxl_root)
>> +		return -ENODEV;
>> +
>> +	guard(device)(&cxl_root->port.dev);
> 
>> +	port = find_cxl_port(dport_dev, &dport);
> 
> I vaguely wonder if a __free() make sense on this. It'll autofree the NULL
> much later than needed but maybe it's cleaner code?

Given we are intentionally looking for a port and will return immediate if found, and later on port isn't used, having a __free() is probably overkill.

> 
>> +	if (port) {
>> +		put_device(&port->dev);
>> +		return 0;
>> +	}
>> +
>> +	if (!cxl_root->ops || !cxl_root->ops->setup_hostbridge_uport)
>> +		return -EOPNOTSUPP;
>> +
>> +	rc = cxl_root->ops->setup_hostbridge_uport(cxl_root, uport_dev);
>> +	if (rc)
>> +		return rc;
>> +
>> +	return 0;
> 
> return cxl_root->ops....

ok

> 
>> +}
>