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

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

[Dave Jiang]

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

This series attempts to delay the 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 (9):
  cxl/region: Add decoder check to check_commit_order()
  cxl: Add helper to detect top of CXL device topology
  cxl: Separate out CXL dport->id vs actual dport hardware id
  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: 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               |  66 +++--
 drivers/cxl/core/port.c              | 344 +++++++++++++++++++++++----
 drivers/cxl/core/region.c            |   7 +-
 drivers/cxl/cxl.h                    |  50 +++-
 drivers/cxl/port.c                   |  26 +-
 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, 605 insertions(+), 200 deletions(-)
 create mode 100644 tools/testing/cxl/exports.h


base-commit: efda449f9119a954359a9c2928a61a99c79d7b41
-- 
2.49.0

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

[Dave Jiang]

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

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 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
-- 
2.49.0

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

[Dave Jiang]

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

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

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 726bd4a7de27..cafb1b13cba1 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);
@@ -1642,11 +1651,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

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

[Dave Jiang]

On 5/21/25 11:34 AM, Dave Jiang wrote:
> Add a helper to replace the open code detection of CXL device hierarchy
> root. The helper will be used for delayed hostbridge port creation later
> on.
> 
> Reviewed-by: Jonathan Cameron <Jonsathan.Cameron@huawei.com>

I'll fix the email address. :)
DJ

> 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 726bd4a7de27..cafb1b13cba1 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);
> @@ -1642,11 +1651,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 v3 2/9] cxl: Add helper to detect top of CXL device topology

[Jonathan Cameron]

On Wed, 21 May 2025 11:39:29 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 5/21/25 11:34 AM, Dave Jiang wrote:
> > Add a helper to replace the open code detection of CXL device hierarchy
> > root. The helper will be used for delayed hostbridge port creation later
> > on.
> > 
> > Reviewed-by: Jonathan Cameron <Jonsathan.Cameron@huawei.com>  
> 
> I'll fix the email address. :)

One day I'll grow up and learn to spell my own name ;)
Thanks!

J
(playing is safe :)

> DJ
> 
> > 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 726bd4a7de27..cafb1b13cba1 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);
> > @@ -1642,11 +1651,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 v3 2/9] cxl: Add helper to detect top of CXL device topology

[Li Ming]

On 5/22/2025 2:34 AM, Dave Jiang wrote:
> Add a helper to replace the open code detection of CXL device hierarchy
> root. The helper will be used for delayed hostbridge port creation later
> on.
>
> Reviewed-by: Jonathan Cameron <Jonsathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Li Ming <ming.li@zohomail.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 726bd4a7de27..cafb1b13cba1 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);
> @@ -1642,11 +1651,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;

[PATCH v3 3/9] cxl: Separate 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.

Reviewed-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Alejandro Lucero <alucerop@amd.com>
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v3:
- Fix typo in subject. (Alison, Alejandro)
- Remove repetitive language in commit log. (Alison)
- Squash in renaming of find_dport(). (Alejandro)
---
 drivers/cxl/core/cdat.c |  2 +-
 drivers/cxl/core/hdm.c  | 18 ++++-----
 drivers/cxl/core/port.c | 81 +++++++++++++++++++++++++++++------------
 drivers/cxl/cxl.h       | 12 ++++--
 4 files changed, 75 insertions(+), 38 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 cafb1b13cba1..30a79276b489 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -168,7 +168,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;
@@ -748,6 +748,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);
@@ -1053,14 +1054,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_by_num(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;
 }
@@ -1071,11 +1072,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_by_num(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;
 	}
@@ -1123,13 +1124,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];
@@ -1148,17 +1179,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,
@@ -1220,7 +1253,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: hardware 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
@@ -1228,12 +1261,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",
@@ -1251,13 +1284,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: 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;
@@ -1267,7 +1300,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",
@@ -1464,7 +1497,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);
 	}
 }
 
@@ -1736,7 +1769,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;
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

Re: [PATCH v3 3/9] cxl: Separate out CXL dport->id vs actual dport hardware id

[Li Ming]

On 5/22/2025 2:34 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.
>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Reviewed-by: Alejandro Lucero <alucerop@amd.com>
> Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Li Ming <ming.li@zohomail.com>

Re: [PATCH v3 3/9] cxl: Separate out CXL dport->id vs actual dport hardware id

[Robert Richter]

On 21.05.25 11:34:37, Dave Jiang wrote:

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

I have noticed some leftovers of the port_id string in 2 comments
(based on cxl/next):

drivers/cxl/core/pci.c:  * dport->port_id is valid means that dport has been probed and is
drivers/cxl/cxl.h: * @target_map: array of expected dport port_id mirror the target

-Robert

>  	struct cxl_rcrb_info rcrb;
>  	bool rch;
>  	struct cxl_port *port;

[PATCH v3 4/9] 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. dport->port_num is set to CXL_DPORT_NUM_INVALID
until the port_num is set in the dport->port_num field.

Reviewed-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Li Ming <ming.li@zohomail.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v3:
- Make note that port_num is set to default invalid value. (Alejandro)
---
 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 30a79276b489..d62008583da2 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1154,14 +1154,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;
@@ -1191,8 +1191,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);
@@ -1253,7 +1251,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: hardware 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
@@ -1261,13 +1258,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));
@@ -1284,13 +1281,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: 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_num,
+					 struct device *dport_dev,
 					 resource_size_t rcrb)
 {
 	struct cxl_dport *dport;
@@ -1300,7 +1296,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 v3 5/9] 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.

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

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

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

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

Locking of port device wraps the cxl_switch_port_dport_setup() to
protect modifications against the port and its dports while multiple
endpoints can be probing at the same time and the same port is being
modified concurrently.

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. With this behavior, only a dport that points to a valid
endpoint path would be probed. Of course the decoder enumeration and
SSLBIS parsing is also delayed until the dport is probed since it needs to
know the port_num and also be able to read the registers.

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

The success return value of devm_cxl_port_enumerate_dports() has been
changed to always 0 since no caller is using the return value of dports
enumerated for anything in the current code now.

Link: https://lore.kernel.org/linux-cxl/20250305100123.3077031-1-rrichter@amd.com/
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v3:
- port_probe_dport() return dport() directly (Ming)
- cxl_switch_port_dport_setup() return rc directly. (Ming)
- Add more explanation in the commit log. (Gregory)
- Return 0 on success for devm_cxl_port_enumerate_dports. (Jonathan)
- comment on return !dev_is_pci() for cxl_dport_setup(). (Jonathan)
- Move retrieving port_num after reading lnkcap. (Jonathan)
- Optimize port_probe_dport code flow. (Jonathan)
- Optimize return of decoder setup. (Jonathan)
---
 drivers/cxl/core/core.h |   4 ++
 drivers/cxl/core/pci.c  |  66 +++++++++++++-----
 drivers/cxl/core/port.c | 146 +++++++++++++++++++++++++++++++++++-----
 drivers/cxl/cxl.h       |   4 ++
 drivers/cxl/port.c      |  26 +------
 5 files changed, 189 insertions(+), 57 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..dd204b6f7032 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -24,6 +24,53 @@ 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;
+
+	/* Skipping for cxl_test since it's platform 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;
+	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
+
+	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
+	if (rc) {
+		dev_dbg(&port->dev, "failed to find component registers\n");
+		return -ENODEV;
+	}
+
+	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 +84,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 +92,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;
@@ -73,8 +108,7 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
  * devm_cxl_port_enumerate_dports - enumerate downstream ports of the upstream port
  * @port: cxl_port whose ->uport_dev is the upstream of dports to be enumerated
  *
- * Returns a positive number of dports enumerated or a negative error
- * code.
+ * Returns 0 on success or a negative error code.
  */
 int devm_cxl_port_enumerate_dports(struct cxl_port *port)
 {
@@ -101,7 +135,7 @@ int devm_cxl_port_enumerate_dports(struct cxl_port *port)
 		return -ENODEV;
 	if (ctx.error)
 		return ctx.error;
-	return ctx.count;
+	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(devm_cxl_port_enumerate_dports, "CXL");
 
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index d62008583da2..2b3ec8436d3d 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -791,8 +791,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;
 
@@ -1013,7 +1013,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;
 
@@ -1030,6 +1030,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)
 {
@@ -1068,19 +1069,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)
@@ -1652,6 +1643,112 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 	return rc;
 }
 
+static struct cxl_dport *port_probe_dport(struct cxl_port *port,
+					  struct device *dport_dev)
+{
+	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)
+			continue;
+
+		rc = cxl_dport_setup(dport);
+		if (rc)
+			return ERR_PTR(rc);
+
+		return dport;
+	}
+
+	return ERR_PTR(-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;
+
+	device_lock_assert(&port->dev);
+
+	if (!port->dev.driver)
+		return -ENODEV;
+
+	dport = port_probe_dport(port, dport_dev);
+	if (IS_ERR(dport))
+		return PTR_ERR(dport);
+
+	cxl_switch_parse_cdat(port);
+
+	/* Make sure that no decoders have been allocated before proceeding. */
+	if (ida_is_empty(&port->decoder_ida))
+		return devm_cxl_port_setup_decoders(port);
+
+	return update_decoders_with_dport(port, dport);
+}
+
 int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 {
 	struct device *dev = &cxlmd->dev;
@@ -1700,6 +1797,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);
 
 			/*
@@ -1765,10 +1875,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..12394f280288 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -59,34 +59,10 @@ 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 */
 	read_cdat_data(port);
 
-	rc = devm_cxl_port_enumerate_dports(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 devm_cxl_port_enumerate_dports(port);
 }
 
 static int cxl_endpoint_port_probe(struct cxl_port *port)
-- 
2.49.0

Re: [PATCH v3 5/9] cxl: Defer hardware dport->port_id assignment and registers probing

[Li Ming]

On 5/22/2025 2:34 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.
>
> In the original enumeration behavior, there are 3 phases (or 2 if no CXL
> switches) for port creation. cxl_acpi() creates a root port from the
> ACPI0017.N device. Through that it enumerate downstream ports composed
> of ACPI0016.N devices through add_host_bridge_dport(). Once done, it
> use add_host_bridge_uport() to create the ports that enumerates the PCI
> root ports as the dports of these ports. Every time a port is created,
> the port driver is attached and drv->probe() is called and
> devm_cxl_port_enumerate_dports() is envoked to enumerate and probe
> the dports.
>
> The second phase is if there are any CXL switches. When the pci endpoint
> device driver (cxl_pci) calls probe, it will add a mem device and triggers
> the cxl_mem->probe(). cxl_mem->probe() calls devm_cxl_enumerate_ports()
> and attempts to discovery and create all the ports represent CXL switches.
> During this phase, a port is created per switch and the attached dports
> are also enumerated and probed.
>
> The last phase is creating endpoint port which happens for all endpoint
> devices.
>
> In this commit, the port create and its dport probing in cxl_acpi is not
> changed. That will be handled in a different patch later on. The behavior
> change is only for CXL switch ports. Only the dport that is part of the
> path for an endpoint device to the RP will be probed and marked as active
> by having a valid dport->port_num. This happens naturally by the code
> walking up the device hierarchy and identifying the upstream device and
> the downstream device.
>
> Locking of port device wraps the cxl_switch_port_dport_setup() to
> protect modifications against the port and its dports while multiple
> endpoints can be probing at the same time and the same port is being
> modified concurrently.
>
> 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. With this behavior, only a dport that points to a valid
> endpoint path would be probed. Of course the decoder enumeration and
> SSLBIS parsing is also delayed until the dport is probed since it needs to
> know the port_num and also be able to read the registers.
>
> The decoders must also be updated since previously it's all done when all
> the dports are setup and now every time a dport is setup per endpoint, the
> switch target listing need to be updated with new dport. A
> guard(rwsem_write) is used to update decoder targets. This is similar to
> when decoder_populate_target() is called and the decoder programming
> must be protected.
>
> The success return value of devm_cxl_port_enumerate_dports() has been
> changed to always 0 since no caller is using the return value of dports
> enumerated for anything in the current code now.
>
> Link: https://lore.kernel.org/linux-cxl/20250305100123.3077031-1-rrichter@amd.com/
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
[snip]
> +
> +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++;

There is a nr_dports in cxl_port, maybe we can use it instead of this xa_for_each().

Other looks good to me.

Reviewed-by: Li Ming <ming.li@zohomail.com>


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

Re: [PATCH v3 5/9] cxl: Defer hardware dport->port_id assignment and registers probing

[Robert Richter]

Dave, see inline.

On 21.05.25 11:34:39, 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.
> 
> In the original enumeration behavior, there are 3 phases (or 2 if no CXL
> switches) for port creation. cxl_acpi() creates a root port from the
> ACPI0017.N device. Through that it enumerate downstream ports composed
> of ACPI0016.N devices through add_host_bridge_dport(). Once done, it
> use add_host_bridge_uport() to create the ports that enumerates the PCI
> root ports as the dports of these ports. Every time a port is created,
> the port driver is attached and drv->probe() is called and
> devm_cxl_port_enumerate_dports() is envoked to enumerate and probe
> the dports.
> 
> The second phase is if there are any CXL switches. When the pci endpoint
> device driver (cxl_pci) calls probe, it will add a mem device and triggers
> the cxl_mem->probe(). cxl_mem->probe() calls devm_cxl_enumerate_ports()
> and attempts to discovery and create all the ports represent CXL switches.
> During this phase, a port is created per switch and the attached dports
> are also enumerated and probed.
> 
> The last phase is creating endpoint port which happens for all endpoint
> devices.
> 
> In this commit, the port create and its dport probing in cxl_acpi is not
> changed. That will be handled in a different patch later on. The behavior
> change is only for CXL switch ports. Only the dport that is part of the
> path for an endpoint device to the RP will be probed and marked as active
> by having a valid dport->port_num. This happens naturally by the code
> walking up the device hierarchy and identifying the upstream device and
> the downstream device.
> 
> Locking of port device wraps the cxl_switch_port_dport_setup() to
> protect modifications against the port and its dports while multiple
> endpoints can be probing at the same time and the same port is being
> modified concurrently.
> 
> 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. With this behavior, only a dport that points to a valid
> endpoint path would be probed. Of course the decoder enumeration and
> SSLBIS parsing is also delayed until the dport is probed since it needs to
> know the port_num and also be able to read the registers.

The decoders could be still enumerated early, just the target list
needs to be updated later. See also below.

> 
> The decoders must also be updated since previously it's all done when all
> the dports are setup and now every time a dport is setup per endpoint, the
> switch target listing need to be updated with new dport. A
> guard(rwsem_write) is used to update decoder targets. This is similar to
> when decoder_populate_target() is called and the decoder programming
> must be protected.
> 
> The success return value of devm_cxl_port_enumerate_dports() has been
> changed to always 0 since no caller is using the return value of dports
> enumerated for anything in the current code now.
> 
> Link: https://lore.kernel.org/linux-cxl/20250305100123.3077031-1-rrichter@amd.com/
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v3:
> - port_probe_dport() return dport() directly (Ming)
> - cxl_switch_port_dport_setup() return rc directly. (Ming)
> - Add more explanation in the commit log. (Gregory)
> - Return 0 on success for devm_cxl_port_enumerate_dports. (Jonathan)
> - comment on return !dev_is_pci() for cxl_dport_setup(). (Jonathan)
> - Move retrieving port_num after reading lnkcap. (Jonathan)
> - Optimize port_probe_dport code flow. (Jonathan)
> - Optimize return of decoder setup. (Jonathan)
> ---
>  drivers/cxl/core/core.h |   4 ++
>  drivers/cxl/core/pci.c  |  66 +++++++++++++-----
>  drivers/cxl/core/port.c | 146 +++++++++++++++++++++++++++++++++++-----
>  drivers/cxl/cxl.h       |   4 ++
>  drivers/cxl/port.c      |  26 +------
>  5 files changed, 189 insertions(+), 57 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..dd204b6f7032 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -24,6 +24,53 @@ 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)

Could you factor that out in a separate patch before this one? That
makes the whole patch smaller and changes easier to review.

> +{
> +	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;
> +
> +	/* Skipping for cxl_test since it's platform device */
> +	if (!dev_is_pci(dport_dev))

The check can be moved in front of port_probe_dport().

Maybe initialize it: dport->port_num = dport->id?

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

Later I am suggesting to create the dport late together with its
activation. port_num would be always valid then.

> +
> +	pdev = to_pci_dev(dport_dev);
> +	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
> +				  &lnkcap))
> +		return -ENODEV;
> +	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
> +
> +	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
> +	if (rc) {
> +		dev_dbg(&port->dev, "failed to find component registers\n");
> +		return -ENODEV;
> +	}
> +
> +	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;

We still need to check for dups of dport->port_num for all dports
connected to dport->port.

> +
> +	return 0;
> +}
> +
>  struct cxl_walk_context {
>  	struct pci_bus *bus;
>  	struct cxl_port *port;
> @@ -37,10 +84,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 +92,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);

All users of devm_cxl_add_dport use CXL_RESOURCE_NONE now, so better
move cxl_dport_setup_regs() out of devm_cxl_add_dport. That also
implies to add a separate code path for the rch/rcrb case to the dport
setup.

>  	if (IS_ERR(dport)) {
>  		ctx->error = PTR_ERR(dport);
>  		return PTR_ERR(dport);
>  	}
>  
> -	dport->port_num = port_num;
>  	ctx->count++;
>  
>  	return 0;
> @@ -73,8 +108,7 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
>   * devm_cxl_port_enumerate_dports - enumerate downstream ports of the upstream port
>   * @port: cxl_port whose ->uport_dev is the upstream of dports to be enumerated
>   *
> - * Returns a positive number of dports enumerated or a negative error
> - * code.
> + * Returns 0 on success or a negative error code.
>   */
>  int devm_cxl_port_enumerate_dports(struct cxl_port *port)
>  {
> @@ -101,7 +135,7 @@ int devm_cxl_port_enumerate_dports(struct cxl_port *port)
>  		return -ENODEV;
>  	if (ctx.error)
>  		return ctx.error;
> -	return ctx.count;
> +	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_cxl_port_enumerate_dports, "CXL");
>  
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index d62008583da2..2b3ec8436d3d 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -791,8 +791,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;
>  
> @@ -1013,7 +1013,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)

Keep static.

>  {
>  	struct cxl_port *port, *parent;
>  
> @@ -1030,6 +1030,7 @@ static bool dev_is_cxl_root_child(struct device *dev)
>  
>  	return false;
>  }
> +EXPORT_SYMBOL_NS_GPL(dev_is_cxl_root_child, "CXL");

... and export not needed.

>  
>  struct cxl_root *find_cxl_root(struct cxl_port *port)
>  {
> @@ -1068,19 +1069,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;

I think we still need to check for dups of dport->port_num for the
port. That might screw up the target list entries otherwise.

> -	}
> -
>  	rc = xa_insert(&port->dports, (unsigned long)dport->dport_dev, dport,
>  		       GFP_KERNEL);
>  	if (rc)
> @@ -1652,6 +1643,112 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>  	return rc;
>  }
>  
> +static struct cxl_dport *port_probe_dport(struct cxl_port *port,
> +					  struct device *dport_dev)

If you move that code back to cxl/pci.c and/or cxl/port.c I think the
whole mock dance in the next patch could become obsolete.

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

Just look that up with xa_load aka cxl_find_dport_by_dev.

> +			continue;
> +
> +		rc = cxl_dport_setup(dport);

How about moving dport creation here too and only collect dport_dev
entry in &port->dports with a reference to NULL first meaning it is
not yet initialized. We can then only expose linked dports with real
port_nums in the reference (dportN where N is the port_num). I really
like to see port_num in sysfs.

> +		if (rc)
> +			return ERR_PTR(rc);
> +
> +		return dport;
> +	}
> +
> +	return ERR_PTR(-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));

That message is misleading as there could be multiple decoders
connected to a port and not all dports are used by that one.

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

Please add a separate patch for creating
devm_cxl_port_setup_decoders().

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

port->nr_dports?

Is the number of dports constant for each port or may it change
depending on hotplug events or enumeration order?

> +
> +	if (dports == 1) {
> +		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
> +		return devm_cxl_add_passthrough_decoder(port);

The dport port_num is not yet available for setting up the passthrough
decoder's target list.

We should ignore cxlsd->target_map[i] and just pre-init
cxlsd->target[i] with the only existing dport for the passthrough
case.

> +	}
> +
> +	dev_err(&port->dev, "HDM decoder capability not found\n");
> +	return -ENXIO;
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_port_setup_decoders, "CXL");

Export not needed.

> +
> +static int cxl_switch_port_dport_setup(struct cxl_port *port,
> +				       struct device *dport_dev)
> +{
> +	struct cxl_dport *dport;
> +
> +	device_lock_assert(&port->dev);
> +
> +	if (!port->dev.driver)
> +		return -ENODEV;

This looks fragile and introduces order dependencies. It would be
better to make cxl_switch_port_dport_setup part of the port probe.

> +
> +	dport = port_probe_dport(port, dport_dev);
> +	if (IS_ERR(dport))
> +		return PTR_ERR(dport);
> +
> +	cxl_switch_parse_cdat(port);
> +

This decoder setup looks odd:

> +	/* Make sure that no decoders have been allocated before proceeding. */
> +	if (ida_is_empty(&port->decoder_ida))
> +		return devm_cxl_port_setup_decoders(port);
> +
> +	return update_decoders_with_dport(port, dport);

Why do you combine decoder and dport setup? To me it looks straight to
just allocate dports, then decoders, and later update the dports and
switch decoder target list with the dport activation.

That is, move the update/activation part to endpoint probe, while the
decoder setup is part of the memdev probe here and/or switch port
probe.

> +}
> +
>  int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>  {
>  	struct device *dev = &cxlmd->dev;
> @@ -1700,6 +1797,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);

I suggest to move the dport creation to the port probe and make this
iterator work without dports (which should work as the next port is
just the grandparent).

>  
>  			/*
> @@ -1765,10 +1875,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;

Just copy the target_map here, the dport is updated later.

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

Both seem to be a leftovers, can be kept static.

>  
>  /*
>   * 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..12394f280288 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -59,34 +59,10 @@ 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 */
>  	read_cdat_data(port);
>  
> -	rc = devm_cxl_port_enumerate_dports(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);

Why do you move cdat and hdm setup out here? It's unrelated to dport
setup.

Ah, cdat depends on it and cdat_sslbis_handler needs it. Maybe
decouple it same way with cxl_cxims_data and update it with the dport
activation. Then, add it to an *_update_port_num() function.

> -
> -	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 devm_cxl_port_enumerate_dports(port);
>  }
>  
>  static int cxl_endpoint_port_probe(struct cxl_port *port)
> -- 
> 2.49.0
> 

Thanks,

-Robert

[PATCH v3 6/9] 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>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 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 v3 7/9] cxl: Change sslbis handler to only handle single dport

[Dave Jiang]

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

Reviewed-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
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 2b3ec8436d3d..5c05cc70787e 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1740,7 +1740,7 @@ static int cxl_switch_port_dport_setup(struct cxl_port *port,
 	if (IS_ERR(dport))
 		return PTR_ERR(dport);
 
-	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

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

[Li Ming]

On 5/22/2025 2:34 AM, Dave Jiang wrote:
> While cxl_switch_parse_cdat() is harmless to be run multiple times, it is
> not efficient in the current scheme where one dport is being updated at
> a time by the memdev probe path. Change the input parameter to the
> specific dport being updated to pick up the SSLBIS information for just
> that dport.
>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Li Ming <ming.li@zohomail.com>

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

[Dave Jiang]

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

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v3:
- Rename udev to hb_uport_dev (Gregory)
---
 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 5c05cc70787e..7650254fdcb4 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_hb_uport_dev_to_root - Retrieve cxl_root tied to the host bridge device
+ * @hb_uport_dev: host bridge upstream port device
+ *
+ * Return cxl_root on success or NULL on failure
+ *
+ * A reference is taken on the port device. Caller needs to call put_device()
+ * when done.
+ */
+struct cxl_root *cxl_hb_uport_dev_to_root(struct device *hb_uport_dev)
+{
+	struct cxl_root *root;
+
+	root = xa_load(&cxl_root_ports, (unsigned long)hb_uport_dev);
+	if (!root)
+		return NULL;
+
+	get_device(&root->port.dev);
+
+	return root;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_hb_uport_dev_to_root, "CXL");
+
+static void unregister_hb_uport_root_ports(void *hb_uport_dev)
+{
+	xa_erase(&cxl_root_ports, (unsigned long)hb_uport_dev);
+}
+
+/**
+ * devm_cxl_register_hb_uport_root_port - Tie a hostbridge device to a root port
+ * @host: device that hosts the memory for the xarray entries
+ * @hb_uport_dev: host bridge device that serves as the xarray index
+ * @root: cxl_root that serves as the xarray entry data
+ *
+ * Return 0 on success or -errno on failure.
+ */
+int devm_cxl_register_hb_uport_root_port(struct device *host,
+					 struct device *hb_uport_dev,
+					 struct cxl_root *root)
+{
+	int rc;
+
+	rc = xa_insert(&cxl_root_ports, (unsigned long)hb_uport_dev, root,
+		       GFP_KERNEL);
+	if (rc)
+		return rc;
+
+	return devm_add_action_or_reset(host, unregister_hb_uport_root_ports,
+					hb_uport_dev);
+}
+EXPORT_SYMBOL_NS_GPL(devm_cxl_register_hb_uport_root_port, "CXL");
+
 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..443182146076 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_hb_uport_root_port(struct device *host,
+					 struct device *hb_uport_dev,
+					 struct cxl_root *root);
+struct cxl_root *cxl_hb_uport_dev_to_root(struct device *uport_dev);
 struct cxl_port *devm_cxl_add_port(struct device *host,
 				   struct device *uport_dev,
 				   resource_size_t component_reg_phys,
-- 
2.49.0

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

[Dave Jiang]

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

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

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v3:
- Add comment in commit log WRT missing DVSEC is spec compliant. (Jonathan)
- Fix spelling mistake in cxl_acpi comment (Jonathan)
- Fix coding style for get_hostbridge_port_devices(). (Jonathan)
- Directly return ->setup_hostbridge_uport(). (Jonathan)
---
 drivers/cxl/acpi.c      | 136 ++++++++++++++++++++++++----------------
 drivers/cxl/core/port.c |  55 ++++++++++++++++
 drivers/cxl/cxl.h       |   2 +
 3 files changed, 138 insertions(+), 55 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 6f8630e50800..f193b62b744b 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 decoder and it is a uport to the
+ * dport (PCIe Root Ports) in the host bridge.
+ */
+static int cxl_acpi_setup_hostbridge_uport(struct cxl_root *cxl_root,
+					   struct device *bridge_dev)
+{
+	struct cxl_port *root_port = &cxl_root->port;
+	struct device *host = root_port->dev.parent;
+	struct acpi_device *hb = ACPI_COMPANION(bridge_dev);
+	resource_size_t component_reg_phys;
+	struct acpi_pci_root *pci_root;
+	struct cxl_chbs_context ctx;
+	struct cxl_dport *dport;
+	struct cxl_port *port;
+	int rc;
+
+	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_hb_uport_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 7650254fdcb4..05f5c235934e 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1802,6 +1802,57 @@ static int cxl_switch_port_dport_setup(struct cxl_port *port,
 	return update_decoders_with_dport(port, dport);
 }
 
+static int get_hostbridge_port_devices(struct cxl_memdev *cxlmd,
+				       struct device **hb_uport_dev,
+				       struct device **hb_dport_dev)
+{
+	struct device *dev = &cxlmd->dev;
+	struct device *iter;
+
+	for (iter = dev; iter; iter = grandparent(iter)) {
+		struct device *dport_dev = grandparent(iter);
+		struct device *uport_dev = dport_dev->parent;
+
+		if (is_cxl_hierarchy_head(uport_dev->parent)) {
+			*hb_uport_dev = uport_dev;
+			*hb_dport_dev = dport_dev;
+			return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+
+static int cxl_hostbridge_port_setup(struct cxl_memdev *cxlmd)
+{
+	struct device *hb_uport_dev, *hb_dport_dev;
+	struct cxl_dport *dport;
+	struct cxl_port *port;
+	int rc;
+
+	rc = get_hostbridge_port_devices(cxlmd, &hb_uport_dev, &hb_dport_dev);
+	if (rc)
+		return -ENODEV;
+
+	struct cxl_root *cxl_root __free(put_cxl_root) =
+		cxl_hb_uport_dev_to_root(hb_uport_dev);
+	if (!cxl_root)
+		return -ENODEV;
+
+	guard(device)(&cxl_root->port.dev);
+	port = find_cxl_port(hb_dport_dev, &dport);
+	if (port) {
+		/* Nothing to do if a port has been created already */
+		put_device(&port->dev);
+		return 0;
+	}
+
+	if (!cxl_root->ops || !cxl_root->ops->setup_hostbridge_uport)
+		return -EOPNOTSUPP;
+
+	return cxl_root->ops->setup_hostbridge_uport(cxl_root, hb_uport_dev);
+}
+
 int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 {
 	struct device *dev = &cxlmd->dev;
@@ -1815,6 +1866,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 443182146076..b813b54f13c9 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 v3 0/9] cxl: Delay HB port and switch dport probing until endpoint dev probe

[Robert Richter]

Dave,

thanks for your series.

On 21.05.25 11:34:34, Dave Jiang wrote:
> v3:
> - Main changes revolve around improving naming of hostbridge uport and dport (Gregory)
> - See specific patches for detailed change log
> 
> This series attempts to delay the 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 (9):
>   cxl/region: Add decoder check to check_commit_order()
>   cxl: Add helper to detect top of CXL device topology
>   cxl: Separate out CXL dport->id vs actual dport hardware id
>   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: Create an xarray to tie a host bridge to the cxl_root
>   cxl: Move enumeration of hostbridge ports to the memdev probe path

I have tested your series and this solves the enumeration issue with
ports where the link of its downstream ports is down and that have
duplicate port ids. For the whole series you can add:

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

However, some observations:

The port_num can no longer retrieved using sysfs. Previously, the X in
dportX could be used to identify the port number (former port_id)
which was identical to the numbers in the sysfs target_list entries.
This is esp. useful to reconstruct the decoder tree and map pci child
devices and its decoders to a parent decoder including its positions
in the target list. Maybe create a targetX symlink from the decoder
device to the child decoders of it?

Due to the different initialization order there is an odd port
numbering now showing up with unexpected, out-of-order sequence
numbers, e.g.:

/sys/bus/cxl/devices/port1/endpoint2
/sys/bus/cxl/devices/port1/endpoint3
/sys/bus/cxl/devices/port1/endpoint6
/sys/bus/cxl/devices/port4/endpoint5
/sys/bus/cxl/devices/port4/endpoint7
/sys/bus/cxl/devices/port4/endpoint10
/sys/bus/cxl/devices/port4/endpoint11
/sys/bus/cxl/devices/port8/endpoint9
/sys/bus/cxl/devices/port12/endpoint13
/sys/bus/cxl/devices/port12/endpoint14

Still, this is correct, but maybe we could force a specific order
during initialization, such as per-port initialization which should
result in a defined order? Note the shared numbering of ports and
endpoints is also confusing, maybe that could be changed here too?

Going to review your patches.

Thanks for your patches.

-Robert

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

[Dave Jiang]

On 5/30/25 6:51 AM, Robert Richter wrote:
> Dave,
> 
> thanks for your series.
> 
> On 21.05.25 11:34:34, Dave Jiang wrote:
>> v3:
>> - Main changes revolve around improving naming of hostbridge uport and dport (Gregory)
>> - See specific patches for detailed change log
>>
>> This series attempts to delay the 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 (9):
>>   cxl/region: Add decoder check to check_commit_order()
>>   cxl: Add helper to detect top of CXL device topology
>>   cxl: Separate out CXL dport->id vs actual dport hardware id
>>   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: Create an xarray to tie a host bridge to the cxl_root
>>   cxl: Move enumeration of hostbridge ports to the memdev probe path
> 
> I have tested your series and this solves the enumeration issue with
> ports where the link of its downstream ports is down and that have
> duplicate port ids. For the whole series you can add:
> 
> Tested-by: Robert Richter <rrichter@amd.com>
> 
> However, some observations:
> 
> The port_num can no longer retrieved using sysfs. Previously, the X in
> dportX could be used to identify the port number (former port_id)
> which was identical to the numbers in the sysfs target_list entries.
> This is esp. useful to reconstruct the decoder tree and map pci child
> devices and its decoders to a parent decoder including its positions
> in the target list. Maybe create a targetX symlink from the decoder
> device to the child decoders of it?

The numbers in the target_list are dport->id and should reflect the dportX. Is this not what you are seeing?

> 
> Due to the different initialization order there is an odd port
> numbering now showing up with unexpected, out-of-order sequence
> numbers, e.g.:
> 
> /sys/bus/cxl/devices/port1/endpoint2
> /sys/bus/cxl/devices/port1/endpoint3
> /sys/bus/cxl/devices/port1/endpoint6
> /sys/bus/cxl/devices/port4/endpoint5
> /sys/bus/cxl/devices/port4/endpoint7
> /sys/bus/cxl/devices/port4/endpoint10
> /sys/bus/cxl/devices/port4/endpoint11
> /sys/bus/cxl/devices/port8/endpoint9
> /sys/bus/cxl/devices/port12/endpoint13
> /sys/bus/cxl/devices/port12/endpoint14
> 
> Still, this is correct, but maybe we could force a specific order
> during initialization, such as per-port initialization which should
> result in a defined order? Note the shared numbering of ports and
> endpoints is also confusing, maybe that could be changed here too?

I think the upstream expectation WRT sysfs device number enumeration (for all Linux kernel devices) is that no ordering should be expected as the devices are initialized async. And previous ordering appearance is merely coincidence and not the expectation. And the expectation for user tools is that they should not depend on device numbering order and identify the devices by other stable means.  

> 
> Going to review your patches.

Thank you!
> 
> Thanks for your patches.
> 
> -Robert

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

[Robert Richter]

On 03.06.25 06:55:15, Dave Jiang wrote:
> On 5/30/25 6:51 AM, Robert Richter wrote:

> > The port_num can no longer retrieved using sysfs. Previously, the X in
> > dportX could be used to identify the port number (former port_id)
> > which was identical to the numbers in the sysfs target_list entries.
> > This is esp. useful to reconstruct the decoder tree and map pci child
> > devices and its decoders to a parent decoder including its positions
> > in the target list. Maybe create a targetX symlink from the decoder
> > device to the child decoders of it?
> 

> The numbers in the target_list are dport->id and should reflect the
> dportX. Is this not what you are seeing?

Exactly, but I would prefer real numbers for the target list, that is,
uid and port_num.

> > 
> > Due to the different initialization order there is an odd port
> > numbering now showing up with unexpected, out-of-order sequence
> > numbers, e.g.:
> > 
> > /sys/bus/cxl/devices/port1/endpoint2
> > /sys/bus/cxl/devices/port1/endpoint3
> > /sys/bus/cxl/devices/port1/endpoint6
> > /sys/bus/cxl/devices/port4/endpoint5
> > /sys/bus/cxl/devices/port4/endpoint7
> > /sys/bus/cxl/devices/port4/endpoint10
> > /sys/bus/cxl/devices/port4/endpoint11
> > /sys/bus/cxl/devices/port8/endpoint9
> > /sys/bus/cxl/devices/port12/endpoint13
> > /sys/bus/cxl/devices/port12/endpoint14
> > 
> > Still, this is correct, but maybe we could force a specific order
> > during initialization, such as per-port initialization which should
> > result in a defined order? Note the shared numbering of ports and
> > endpoints is also confusing, maybe that could be changed here too?
> 

> I think the upstream expectation WRT sysfs device number enumeration
> (for all Linux kernel devices) is that no ordering should be
> expected as the devices are initialized async. And previous ordering
> appearance is merely coincidence and not the expectation. And the
> expectation for user tools is that they should not depend on device
> numbering order and identify the devices by other stable means.

sysfs still is human readable and for good reasons not some binary
blob you need a viewer for. Avoiding un-ordered enumeration where
possible helps understanding the topology. So let's think about it and
if possible find a way to get a defined numbering.

Thanks,

-Robert

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

[Dave Jiang]

On 6/4/25 8:44 AM, Robert Richter wrote:
> On 03.06.25 06:55:15, Dave Jiang wrote:
>> On 5/30/25 6:51 AM, Robert Richter wrote:
> 
>>> The port_num can no longer retrieved using sysfs. Previously, the X in
>>> dportX could be used to identify the port number (former port_id)
>>> which was identical to the numbers in the sysfs target_list entries.
>>> This is esp. useful to reconstruct the decoder tree and map pci child
>>> devices and its decoders to a parent decoder including its positions
>>> in the target list. Maybe create a targetX symlink from the decoder
>>> device to the child decoders of it?
>>
> 
>> The numbers in the target_list are dport->id and should reflect the
>> dportX. Is this not what you are seeing?
> 
> Exactly, but I would prefer real numbers for the target list, that is,
> uid and port_num.

Does it help if we export the port_num sysfs attribute? Is there any specific usage that you need the actual hardware port_num?

> 
>>>
>>> Due to the different initialization order there is an odd port
>>> numbering now showing up with unexpected, out-of-order sequence
>>> numbers, e.g.:
>>>
>>> /sys/bus/cxl/devices/port1/endpoint2
>>> /sys/bus/cxl/devices/port1/endpoint3
>>> /sys/bus/cxl/devices/port1/endpoint6
>>> /sys/bus/cxl/devices/port4/endpoint5
>>> /sys/bus/cxl/devices/port4/endpoint7
>>> /sys/bus/cxl/devices/port4/endpoint10
>>> /sys/bus/cxl/devices/port4/endpoint11
>>> /sys/bus/cxl/devices/port8/endpoint9
>>> /sys/bus/cxl/devices/port12/endpoint13
>>> /sys/bus/cxl/devices/port12/endpoint14
>>>
>>> Still, this is correct, but maybe we could force a specific order
>>> during initialization, such as per-port initialization which should
>>> result in a defined order? Note the shared numbering of ports and
>>> endpoints is also confusing, maybe that could be changed here too?
>>
> 
>> I think the upstream expectation WRT sysfs device number enumeration
>> (for all Linux kernel devices) is that no ordering should be
>> expected as the devices are initialized async. And previous ordering
>> appearance is merely coincidence and not the expectation. And the
>> expectation for user tools is that they should not depend on device
>> numbering order and identify the devices by other stable means.
> 
> sysfs still is human readable and for good reasons not some binary
> blob you need a viewer for. Avoiding un-ordered enumeration where
> possible helps understanding the topology. So let's think about it and
> if possible find a way to get a defined numbering.

I'm trying to understand your pain point WRT to why ordering is necessary. Is it somehow breaking your user tooling? I'm not sure I agree with the thinking that we should try to design the enumeration to make sure that the device id number should be in a certain predictable order, especially with the async nature of multiple device enumeration. 'cxl list' should provide the human readable hierarchy view for user understanding, not sysfs.   

> 
> Thanks,
> 
> -Robert

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

[Robert Richter]

On 05.06.25 08:17:17, Dave Jiang wrote:
> 
> 
> On 6/4/25 8:44 AM, Robert Richter wrote:
> > On 03.06.25 06:55:15, Dave Jiang wrote:
> >> On 5/30/25 6:51 AM, Robert Richter wrote:
> > 
> >>> The port_num can no longer retrieved using sysfs. Previously, the X in
> >>> dportX could be used to identify the port number (former port_id)
> >>> which was identical to the numbers in the sysfs target_list entries.
> >>> This is esp. useful to reconstruct the decoder tree and map pci child
> >>> devices and its decoders to a parent decoder including its positions
> >>> in the target list. Maybe create a targetX symlink from the decoder
> >>> device to the child decoders of it?
> >>
> > 
> >> The numbers in the target_list are dport->id and should reflect the
> >> dportX. Is this not what you are seeing?
> > 
> > Exactly, but I would prefer real numbers for the target list, that is,
> > uid and port_num.

> Does it help if we export the port_num sysfs attribute? Is there any
> specific usage that you need the actual hardware port_num?

In the target_list attribute I would expect the actual register
content of the target list. I assume tools rely on dport%d being the
same number as in the target_list attribute?

My first preference would be to delay the creation of the dports. We
can then work with port_num in dport%d. That is, only create it if the
link is up and a port or endpoint is connected to it. Everything
remains the same then, except for when the dport is created and seen
in sysfs. There are other advantages this has, e.g. for the driver
implemention.

port_num would be an alternative, but where to place it in sysfs as
dport%d links to the pci_dev already and dport%d/port_num does not
work? port%d/port_num does not help either as this would acutally the
port num of the parent port for the link to it.

I thought of reusing the target%d scheme, but that is already taken
for the regions and provides the decoder device string.

So my 2nd preference would be to introduce a link%d attribute with
port_num as parameter that links to the child port or endpoint. E.g.

 ls -l /sys/bus/cxl/devices/root0/link*/link*
 ls -ld /sys/bus/cxl/devices/root0/link*/endpoint*

would give a quick inside into the port hierarchy. That also helps to
easier find port children.

> 
> > 
> >>>
> >>> Due to the different initialization order there is an odd port
> >>> numbering now showing up with unexpected, out-of-order sequence
> >>> numbers, e.g.:
> >>>
> >>> /sys/bus/cxl/devices/port1/endpoint2
> >>> /sys/bus/cxl/devices/port1/endpoint3
> >>> /sys/bus/cxl/devices/port1/endpoint6
> >>> /sys/bus/cxl/devices/port4/endpoint5
> >>> /sys/bus/cxl/devices/port4/endpoint7
> >>> /sys/bus/cxl/devices/port4/endpoint10
> >>> /sys/bus/cxl/devices/port4/endpoint11
> >>> /sys/bus/cxl/devices/port8/endpoint9
> >>> /sys/bus/cxl/devices/port12/endpoint13
> >>> /sys/bus/cxl/devices/port12/endpoint14
> >>>
> >>> Still, this is correct, but maybe we could force a specific order
> >>> during initialization, such as per-port initialization which should
> >>> result in a defined order? Note the shared numbering of ports and
> >>> endpoints is also confusing, maybe that could be changed here too?
> >>
> > 
> >> I think the upstream expectation WRT sysfs device number enumeration
> >> (for all Linux kernel devices) is that no ordering should be
> >> expected as the devices are initialized async. And previous ordering
> >> appearance is merely coincidence and not the expectation. And the
> >> expectation for user tools is that they should not depend on device
> >> numbering order and identify the devices by other stable means.
> > 
> > sysfs still is human readable and for good reasons not some binary
> > blob you need a viewer for. Avoiding un-ordered enumeration where
> > possible helps understanding the topology. So let's think about it and
> > if possible find a way to get a defined numbering.
> 

> I'm trying to understand your pain point WRT to why ordering is
> necessary. Is it somehow breaking your user tooling? I'm not sure I

The expectation is broken in a way that Port in the LnkCap no longer
matches with numbers in the target_list attribute and with dport%d.
But I don't know of tools that break here.

> agree with the thinking that we should try to design the enumeration
> to make sure that the device id number should be in a certain
> predictable order, especially with the async nature of multiple
> device enumeration. 'cxl list' should provide the human readable
> hierarchy view for user understanding, not sysfs.

cxl list only shows enumerated devices. Will check again if that can
be used if the setup is incomplete. In general we should keep the
sysfs structure simple. I am not saying the must grant a defined order
in sysfs, but should keep it straight when possible. That helps
debugging and typically improves also the software design of the
driver.

-Robert

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

[Gregory Price]

On Fri, Jun 06, 2025 at 11:44:39AM +0200, Robert Richter wrote:
> > I'm trying to understand your pain point WRT to why ordering is
> > necessary. Is it somehow breaking your user tooling? I'm not sure I
> 
> The expectation is broken in a way that Port in the LnkCap no longer
> matches with numbers in the target_list attribute and with dport%d.
> But I don't know of tools that break here.
> 

Maybe just expose a given port's "real" port ID as portN/target_id?

The issue is that async probe breaks sysfs object numbering in general,
and so "N" and "ID" don't have an intrinsic relationship.  I imagine we
should fix anything that assume this relationship exists - rather than
try to retain that relationship.

That gives a more formal search mechanism for ports based on a target_id
and better naming.  (I don't actually care what we call it)

~Gregory

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

[Dave Jiang]

On 6/13/25 8:15 AM, Gregory Price wrote:
> On Fri, Jun 06, 2025 at 11:44:39AM +0200, Robert Richter wrote:
>>> I'm trying to understand your pain point WRT to why ordering is
>>> necessary. Is it somehow breaking your user tooling? I'm not sure I
>>
>> The expectation is broken in a way that Port in the LnkCap no longer
>> matches with numbers in the target_list attribute and with dport%d.
>> But I don't know of tools that break here.
>>
> 
> Maybe just expose a given port's "real" port ID as portN/target_id?
> 
> The issue is that async probe breaks sysfs object numbering in general,
> and so "N" and "ID" don't have an intrinsic relationship.  I imagine we
> should fix anything that assume this relationship exists - rather than
> try to retain that relationship.
> 
> That gives a more formal search mechanism for ports based on a target_id
> and better naming.  (I don't actually care what we call it)

I'm currently doing testing. But so far it seems possible to do dport allocation at a later point as Robert suggested when the downstream port should be valid and we don't need a separate virtual id. We'll see if I hit anything weird. It's working for me on qemu, but cxl-test needs some massaging still.

> 
> ~Gregory

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

[Robert Richter]

On 13.06.25 08:43:30, Dave Jiang wrote:

> I'm currently doing testing. But so far it seems possible to do
> dport allocation at a later point as Robert suggested when the
> downstream port should be valid and we don't need a separate virtual
> id. We'll see if I hit anything weird. It's working for me on qemu,
> but cxl-test needs some massaging still.

Great, goint to test this once out.

Thanks,

-Robert