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

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

[Dave Jiang]

This series attempts to delay the setup of dports and Host Bridge (HB) register
probing 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 (4):
      cxl: Saperate out CXL dport->id vs actual dport hardware id
      cxl: Defer hardware dport->port_id assignment and registers probing
      cxl: Add late host bridge uport mapping update
      cxl/test: Add workaround for cxl_test for cxl_core calling mocked functions

 drivers/cxl/acpi.c            |  17 +++++-
 drivers/cxl/core/core.h       |   4 ++
 drivers/cxl/core/hdm.c        |  51 ++++++++++++++---
 drivers/cxl/core/pci.c        |  74 +++++++++++++++++++++----
 drivers/cxl/core/port.c       | 186 ++++++++++++++++++++++++++++++++++++++++++++++----------------
 drivers/cxl/cxl.h             |  25 +++++++++
 drivers/cxl/port.c            |  21 +------
 tools/testing/cxl/Kbuild      |   3 -
 tools/testing/cxl/test/mock.c |  34 ++++++++----
 9 files changed, 315 insertions(+), 100 deletions(-)      

base-commit: a0ba0d4ec5e79ce96317a057ce4a60d8aaf0af6e

[PATCH 1/4] cxl: Saperate out CXL dport->id vs actual dport hardware id

[Dave Jiang]

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

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

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 0fd6646c1a2e..e90e55bc11ac 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -159,7 +159,7 @@ static ssize_t emit_target_list(struct cxl_switch_decoder *cxlsd, char *buf)
 
 		if (i + 1 < cxld->interleave_ways)
 			next = cxlsd->target[i + 1];
-		rc = sysfs_emit_at(buf, offset, "%d%s", dport->port_id,
+		rc = sysfs_emit_at(buf, offset, "%d%s", dport->id,
 				   next ? "," : "");
 		if (rc < 0)
 			return rc;
@@ -739,6 +739,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev,
 		dev->parent = uport_dev;
 
 	ida_init(&port->decoder_ida);
+	ida_init(&port->dport_ida);
 	port->hdm_end = -1;
 	port->commit_end = -1;
 	xa_init(&port->dports);
@@ -1044,14 +1045,14 @@ void put_cxl_root(struct cxl_root *cxl_root)
 }
 EXPORT_SYMBOL_NS_GPL(put_cxl_root, "CXL");
 
-static struct cxl_dport *find_dport(struct cxl_port *port, int id)
+static struct cxl_dport *find_dport(struct cxl_port *port, int port_id)
 {
 	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_id == port_id)
 			return dport;
 	return NULL;
 }
@@ -1105,6 +1106,7 @@ static void cxl_dport_remove(void *data)
 	struct cxl_port *port = dport->port;
 
 	xa_erase(&port->dports, (unsigned long) dport->dport_dev);
+	ida_free(&port->dport_ida, dport->id);
 	put_device(dport->dport_dev);
 }
 
@@ -1114,7 +1116,7 @@ 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);
 }
 
@@ -1126,7 +1128,7 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
 	char link_name[CXL_TARGET_STRLEN];
 	struct cxl_dport *dport;
 	struct device *host;
-	int rc;
+	int id, rc;
 
 	if (is_cxl_root(port))
 		host = port->uport_dev;
@@ -1139,29 +1141,41 @@ __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) >=
-	    CXL_TARGET_STRLEN)
+	id = ida_alloc(&port->dport_ida, GFP_KERNEL);
+	if (id < 0)
+		return ERR_PTR(id);
+
+	if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", id) >=
+	    CXL_TARGET_STRLEN) {
+		ida_free(&port->dport_ida, id);
 		return ERR_PTR(-EINVAL);
+	}
 
 	dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
-	if (!dport)
+	if (!dport) {
+		ida_free(&port->dport_ida, id);
 		return ERR_PTR(-ENOMEM);
+	}
 
 	dport->dport_dev = dport_dev;
 	dport->port_id = port_id;
 	dport->port = port;
+	dport->id = id;
 
 	if (rcrb == CXL_RESOURCE_NONE) {
 		rc = cxl_dport_setup_regs(&port->dev, dport,
 					  component_reg_phys);
-		if (rc)
+		if (rc) {
+			ida_free(&port->dport_ida, id);
 			return ERR_PTR(rc);
+		}
 	} else {
 		dport->rcrb.base = rcrb;
 		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
 							 CXL_RCRB_DOWNSTREAM);
 		if (component_reg_phys == CXL_RESOURCE_NONE) {
 			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
+			ida_free(&port->dport_ida, id);
 			return ERR_PTR(-ENXIO);
 		}
 
@@ -1170,8 +1184,10 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
 		 * memdev
 		 */
 		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
-		if (rc)
+		if (rc) {
+			ida_free(&port->dport_ida, id);
 			return ERR_PTR(rc);
+		}
 
 		dport->rch = true;
 	}
@@ -1183,8 +1199,10 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
 	cond_cxl_root_lock(port);
 	rc = add_dport(port, dport);
 	cond_cxl_root_unlock(port);
-	if (rc)
+	if (rc) {
+		ida_free(&port->dport_ida, id);
 		return ERR_PTR(rc);
+	}
 
 	get_device(dport_dev);
 	rc = devm_add_action_or_reset(host, cxl_dport_remove, dport);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index be8a7dc77719..c942fa40c869 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
@@ -604,6 +605,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;
@@ -657,6 +659,7 @@ 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
+ * @id: Linux id to enumerate dport instances per port
  * @port_id: 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
@@ -668,6 +671,7 @@ struct cxl_rcrb_info {
 struct cxl_dport {
 	struct device *dport_dev;
 	struct cxl_register_map reg_map;
+	int id;
 	int port_id;
 	struct cxl_rcrb_info rcrb;
 	bool rch;
-- 
2.49.0

Re: [PATCH 1/4] cxl: Saperate out CXL dport->id vs actual dport hardware id

[Jonathan Cameron]

On Fri, 4 Apr 2025 15:57:33 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

Typo in patch title. Separate


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

I'm getting my head around this still but just based on refactor in here
we have a bit where lifetimes weren't quite what I expected to see.

> ---
>  drivers/cxl/core/port.c | 40 +++++++++++++++++++++++++++++-----------
>  drivers/cxl/cxl.h       |  4 ++++
>  2 files changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 0fd6646c1a2e..e90e55bc11ac 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -159,7 +159,7 @@ static ssize_t emit_target_list(struct cxl_switch_decoder *cxlsd, char *buf)
>  
>  		if (i + 1 < cxld->interleave_ways)
>  			next = cxlsd->target[i + 1];
> -		rc = sysfs_emit_at(buf, offset, "%d%s", dport->port_id,
> +		rc = sysfs_emit_at(buf, offset, "%d%s", dport->id,
>  				   next ? "," : "");
>  		if (rc < 0)
>  			return rc;
> @@ -739,6 +739,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev,
>  		dev->parent = uport_dev;
>  
>  	ida_init(&port->decoder_ida);
> +	ida_init(&port->dport_ida);
>  	port->hdm_end = -1;
>  	port->commit_end = -1;
>  	xa_init(&port->dports);
> @@ -1044,14 +1045,14 @@ void put_cxl_root(struct cxl_root *cxl_root)
>  }
>  EXPORT_SYMBOL_NS_GPL(put_cxl_root, "CXL");
>  
> -static struct cxl_dport *find_dport(struct cxl_port *port, int id)
> +static struct cxl_dport *find_dport(struct cxl_port *port, int port_id)
>  {
>  	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_id == port_id)
>  			return dport;
>  	return NULL;
>  }
> @@ -1105,6 +1106,7 @@ static void cxl_dport_remove(void *data)
>  	struct cxl_port *port = dport->port;
>  
>  	xa_erase(&port->dports, (unsigned long) dport->dport_dev);
> +	ida_free(&port->dport_ida, dport->id);

Hmm. This ordering is effectively different to that in __devm_cxl_add_dport(reversed).
If we were to match that we'd free the ida after the
put_device(dport->dport_dev) which I think is unwinding the reference from
get_device(dport_dev)

This makes me suspicious of the ownership of dport->id.

Maybe just spin another devm_add_action_or_reset() to free the id?

If we need this ordering add a comment here on why.


>  	put_device(dport->dport_dev);
>  }
>  
> @@ -1114,7 +1116,7 @@ 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);
>  }
>  
> @@ -1126,7 +1128,7 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>  	char link_name[CXL_TARGET_STRLEN];
>  	struct cxl_dport *dport;
>  	struct device *host;
> -	int rc;
> +	int id, rc;
>  
>  	if (is_cxl_root(port))
>  		host = port->uport_dev;
> @@ -1139,29 +1141,41 @@ __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) >=
> -	    CXL_TARGET_STRLEN)
> +	id = ida_alloc(&port->dport_ida, GFP_KERNEL);
> +	if (id < 0)
> +		return ERR_PTR(id);
> +
> +	if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", id) >=
> +	    CXL_TARGET_STRLEN) {
> +		ida_free(&port->dport_ida, id);

I'm not that keen on the number of places we have to call ida_free() manually
in error paths.  I think that's because we should have another devm
handler...

>  		return ERR_PTR(-EINVAL);
> +	}
>  
>  	dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
> -	if (!dport)
> +	if (!dport) {
> +		ida_free(&port->dport_ida, id);
>  		return ERR_PTR(-ENOMEM);
> +	}
>  
>  	dport->dport_dev = dport_dev;
>  	dport->port_id = port_id;
>  	dport->port = port;
> +	dport->id = id;
>  
>  	if (rcrb == CXL_RESOURCE_NONE) {
>  		rc = cxl_dport_setup_regs(&port->dev, dport,
>  					  component_reg_phys);
> -		if (rc)
> +		if (rc) {
> +			ida_free(&port->dport_ida, id);
>  			return ERR_PTR(rc);
> +		}
>  	} else {
>  		dport->rcrb.base = rcrb;
>  		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
>  							 CXL_RCRB_DOWNSTREAM);
>  		if (component_reg_phys == CXL_RESOURCE_NONE) {
>  			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
> +			ida_free(&port->dport_ida, id);
>  			return ERR_PTR(-ENXIO);
>  		}
>  
> @@ -1170,8 +1184,10 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>  		 * memdev
>  		 */
>  		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
> -		if (rc)
> +		if (rc) {
> +			ida_free(&port->dport_ida, id);
>  			return ERR_PTR(rc);
> +		}
>  
>  		dport->rch = true;
>  	}
> @@ -1183,8 +1199,10 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>  	cond_cxl_root_lock(port);
>  	rc = add_dport(port, dport);
>  	cond_cxl_root_unlock(port);
> -	if (rc)
> +	if (rc) {
> +		ida_free(&port->dport_ida, id);
>  		return ERR_PTR(rc);
> +	}
>  
>  	get_device(dport_dev);
>  	rc = devm_add_action_or_reset(host, cxl_dport_remove, dport);

Re: [PATCH 1/4] cxl: Saperate out CXL dport->id vs actual dport hardware id

[Dave Jiang]

On 4/22/25 9:54 AM, Jonathan Cameron wrote:
> On Fri, 4 Apr 2025 15:57:33 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
> Typo in patch title. Separate
> 
> 
>> In preparation to allow dport to be allocated without being active, make
>> dport->id to be Linux id that enumerates the dport objects per port.
>> Keep the hardware id under dport->port_id to maintain compatibility and
>> introduce a dport->id as the enumeration id.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> I'm getting my head around this still but just based on refactor in here
> we have a bit where lifetimes weren't quite what I expected to see.
> 
>> ---
>>  drivers/cxl/core/port.c | 40 +++++++++++++++++++++++++++++-----------
>>  drivers/cxl/cxl.h       |  4 ++++
>>  2 files changed, 33 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 0fd6646c1a2e..e90e55bc11ac 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -159,7 +159,7 @@ static ssize_t emit_target_list(struct cxl_switch_decoder *cxlsd, char *buf)
>>  
>>  		if (i + 1 < cxld->interleave_ways)
>>  			next = cxlsd->target[i + 1];
>> -		rc = sysfs_emit_at(buf, offset, "%d%s", dport->port_id,
>> +		rc = sysfs_emit_at(buf, offset, "%d%s", dport->id,
>>  				   next ? "," : "");
>>  		if (rc < 0)
>>  			return rc;
>> @@ -739,6 +739,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev,
>>  		dev->parent = uport_dev;
>>  
>>  	ida_init(&port->decoder_ida);
>> +	ida_init(&port->dport_ida);
>>  	port->hdm_end = -1;
>>  	port->commit_end = -1;
>>  	xa_init(&port->dports);
>> @@ -1044,14 +1045,14 @@ void put_cxl_root(struct cxl_root *cxl_root)
>>  }
>>  EXPORT_SYMBOL_NS_GPL(put_cxl_root, "CXL");
>>  
>> -static struct cxl_dport *find_dport(struct cxl_port *port, int id)
>> +static struct cxl_dport *find_dport(struct cxl_port *port, int port_id)
>>  {
>>  	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_id == port_id)
>>  			return dport;
>>  	return NULL;
>>  }
>> @@ -1105,6 +1106,7 @@ static void cxl_dport_remove(void *data)
>>  	struct cxl_port *port = dport->port;
>>  
>>  	xa_erase(&port->dports, (unsigned long) dport->dport_dev);
>> +	ida_free(&port->dport_ida, dport->id);
> 
> Hmm. This ordering is effectively different to that in __devm_cxl_add_dport(reversed).
> If we were to match that we'd free the ida after the
> put_device(dport->dport_dev) which I think is unwinding the reference from
> get_device(dport_dev)
> 
> This makes me suspicious of the ownership of dport->id.
> 
> Maybe just spin another devm_add_action_or_reset() to free the id?

Yes I'll do that. Same thing Dan suggested as well.

> 
> If we need this ordering add a comment here on why.
> 
> 
>>  	put_device(dport->dport_dev);
>>  }
>>  
>> @@ -1114,7 +1116,7 @@ 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);
>>  }
>>  
>> @@ -1126,7 +1128,7 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  	char link_name[CXL_TARGET_STRLEN];
>>  	struct cxl_dport *dport;
>>  	struct device *host;
>> -	int rc;
>> +	int id, rc;
>>  
>>  	if (is_cxl_root(port))
>>  		host = port->uport_dev;
>> @@ -1139,29 +1141,41 @@ __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) >=
>> -	    CXL_TARGET_STRLEN)
>> +	id = ida_alloc(&port->dport_ida, GFP_KERNEL);
>> +	if (id < 0)
>> +		return ERR_PTR(id);
>> +
>> +	if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", id) >=
>> +	    CXL_TARGET_STRLEN) {
>> +		ida_free(&port->dport_ida, id);
> 
> I'm not that keen on the number of places we have to call ida_free() manually
> in error paths.  I think that's because we should have another devm
> handler...

With the new change this should go away.

DJ

> 
>>  		return ERR_PTR(-EINVAL);
>> +	}
>>  
>>  	dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
>> -	if (!dport)
>> +	if (!dport) {
>> +		ida_free(&port->dport_ida, id);
>>  		return ERR_PTR(-ENOMEM);
>> +	}
>>  
>>  	dport->dport_dev = dport_dev;
>>  	dport->port_id = port_id;
>>  	dport->port = port;
>> +	dport->id = id;
>>  
>>  	if (rcrb == CXL_RESOURCE_NONE) {
>>  		rc = cxl_dport_setup_regs(&port->dev, dport,
>>  					  component_reg_phys);
>> -		if (rc)
>> +		if (rc) {
>> +			ida_free(&port->dport_ida, id);
>>  			return ERR_PTR(rc);
>> +		}
>>  	} else {
>>  		dport->rcrb.base = rcrb;
>>  		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
>>  							 CXL_RCRB_DOWNSTREAM);
>>  		if (component_reg_phys == CXL_RESOURCE_NONE) {
>>  			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
>> +			ida_free(&port->dport_ida, id);
>>  			return ERR_PTR(-ENXIO);
>>  		}
>>  
>> @@ -1170,8 +1184,10 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  		 * memdev
>>  		 */
>>  		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
>> -		if (rc)
>> +		if (rc) {
>> +			ida_free(&port->dport_ida, id);
>>  			return ERR_PTR(rc);
>> +		}
>>  
>>  		dport->rch = true;
>>  	}
>> @@ -1183,8 +1199,10 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  	cond_cxl_root_lock(port);
>>  	rc = add_dport(port, dport);
>>  	cond_cxl_root_unlock(port);
>> -	if (rc)
>> +	if (rc) {
>> +		ida_free(&port->dport_ida, id);
>>  		return ERR_PTR(rc);
>> +	}
>>  
>>  	get_device(dport_dev);
>>  	rc = devm_add_action_or_reset(host, cxl_dport_remove, dport);
> 
> 

Re: [PATCH 1/4] cxl: Saperate out CXL dport->id vs actual dport hardware id

[Dan Williams]

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_id to maintain compatibility and
> introduce a dport->id as the enumeration id.

I think ->port_id is now a poor choice for a name given the potential
confusion of "which one of these ->id ->port_id is the one read from the
hardware?". So I like ->id is the name for the logical id, but lets do a
lead-in patch renaming ->port_id to ->port_num since that is the
hardware name for this i.e.:

#define  PCI_EXP_LNKCAP_PN      0xff000000 /* Port Number */

> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/port.c | 40 +++++++++++++++++++++++++++++-----------
>  drivers/cxl/cxl.h       |  4 ++++
>  2 files changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 0fd6646c1a2e..e90e55bc11ac 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -159,7 +159,7 @@ static ssize_t emit_target_list(struct cxl_switch_decoder *cxlsd, char *buf)
>  
>  		if (i + 1 < cxld->interleave_ways)
>  			next = cxlsd->target[i + 1];
> -		rc = sysfs_emit_at(buf, offset, "%d%s", dport->port_id,
> +		rc = sysfs_emit_at(buf, offset, "%d%s", dport->id,
>  				   next ? "," : "");
>  		if (rc < 0)
>  			return rc;
> @@ -739,6 +739,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev,
>  		dev->parent = uport_dev;
>  
>  	ida_init(&port->decoder_ida);
> +	ida_init(&port->dport_ida);
>  	port->hdm_end = -1;
>  	port->commit_end = -1;
>  	xa_init(&port->dports);
> @@ -1044,14 +1045,14 @@ void put_cxl_root(struct cxl_root *cxl_root)
>  }
>  EXPORT_SYMBOL_NS_GPL(put_cxl_root, "CXL");
>  
> -static struct cxl_dport *find_dport(struct cxl_port *port, int id)
> +static struct cxl_dport *find_dport(struct cxl_port *port, int port_id)

This also feels like it needs a rename (find_dport_{by_num,by_id}) to
avoid future confusion where someone gets confused about whether a dport
is looked up logically or physically. Even looking at this now in
isolation I do not have the context to say whether logical or physical
lookup is required.

>  {
>  	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_id == port_id)
>  			return dport;
>  	return NULL;
>  }
> @@ -1105,6 +1106,7 @@ static void cxl_dport_remove(void *data)
>  	struct cxl_port *port = dport->port;
>  
>  	xa_erase(&port->dports, (unsigned long) dport->dport_dev);
> +	ida_free(&port->dport_ida, dport->id);
>  	put_device(dport->dport_dev);
>  }
>  
> @@ -1114,7 +1116,7 @@ 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);
>  }
>  
> @@ -1126,7 +1128,7 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>  	char link_name[CXL_TARGET_STRLEN];
>  	struct cxl_dport *dport;
>  	struct device *host;
> -	int rc;
> +	int id, rc;
>  
>  	if (is_cxl_root(port))
>  		host = port->uport_dev;
> @@ -1139,29 +1141,41 @@ __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) >=
> -	    CXL_TARGET_STRLEN)
> +	id = ida_alloc(&port->dport_ida, GFP_KERNEL);
> +	if (id < 0)
> +		return ERR_PTR(id);
> +
> +	if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", id) >=
> +	    CXL_TARGET_STRLEN) {
> +		ida_free(&port->dport_ida, id);
>  		return ERR_PTR(-EINVAL);
> +	}
>  
>  	dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
> -	if (!dport)
> +	if (!dport) {
> +		ida_free(&port->dport_ida, id);

Rather than sprinkle ida_free() throughout this function, just make
allocation do all the allocations in one go, something like:

static struct cxl_dport *cxl_alloc_dport(struct cxl_port *port, ...)
{
	struct cxl_dport *dport __free(kfree) = kzalloc(&port->dev, 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_id = port_id;
	dport->port = port;
	dport->id = id;

	return no_free_ptr(dport);
}

dport = cxl_alloc_dport(...)
if (!dport)
	return -ENOMEM;
rc = devm_add_action_or_reset(&port->dev, free_dport, dport);
if (rc)
	return rc;

Re: [PATCH 1/4] cxl: Saperate out CXL dport->id vs actual dport hardware id

[Dave Jiang]

On 4/22/25 12:37 PM, Dan Williams wrote:
> 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_id to maintain compatibility and
>> introduce a dport->id as the enumeration id.
> 
> I think ->port_id is now a poor choice for a name given the potential
> confusion of "which one of these ->id ->port_id is the one read from the
> hardware?". So I like ->id is the name for the logical id, but lets do a
> lead-in patch renaming ->port_id to ->port_num since that is the
> hardware name for this i.e.:
> 
> #define  PCI_EXP_LNKCAP_PN      0xff000000 /* Port Number */

Will do

> 
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  drivers/cxl/core/port.c | 40 +++++++++++++++++++++++++++++-----------
>>  drivers/cxl/cxl.h       |  4 ++++
>>  2 files changed, 33 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 0fd6646c1a2e..e90e55bc11ac 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -159,7 +159,7 @@ static ssize_t emit_target_list(struct cxl_switch_decoder *cxlsd, char *buf)
>>  
>>  		if (i + 1 < cxld->interleave_ways)
>>  			next = cxlsd->target[i + 1];
>> -		rc = sysfs_emit_at(buf, offset, "%d%s", dport->port_id,
>> +		rc = sysfs_emit_at(buf, offset, "%d%s", dport->id,
>>  				   next ? "," : "");
>>  		if (rc < 0)
>>  			return rc;
>> @@ -739,6 +739,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev,
>>  		dev->parent = uport_dev;
>>  
>>  	ida_init(&port->decoder_ida);
>> +	ida_init(&port->dport_ida);
>>  	port->hdm_end = -1;
>>  	port->commit_end = -1;
>>  	xa_init(&port->dports);
>> @@ -1044,14 +1045,14 @@ void put_cxl_root(struct cxl_root *cxl_root)
>>  }
>>  EXPORT_SYMBOL_NS_GPL(put_cxl_root, "CXL");
>>  
>> -static struct cxl_dport *find_dport(struct cxl_port *port, int id)
>> +static struct cxl_dport *find_dport(struct cxl_port *port, int port_id)
> 
> This also feels like it needs a rename (find_dport_{by_num,by_id}) to
> avoid future confusion where someone gets confused about whether a dport
> is looked up logically or physically. Even looking at this now in
> isolation I do not have the context to say whether logical or physical
> lookup is required.
> 

Yes will introduce a prep patch to change that. I agree about the confusion.

>>  {
>>  	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_id == port_id)
>>  			return dport;
>>  	return NULL;
>>  }
>> @@ -1105,6 +1106,7 @@ static void cxl_dport_remove(void *data)
>>  	struct cxl_port *port = dport->port;
>>  
>>  	xa_erase(&port->dports, (unsigned long) dport->dport_dev);
>> +	ida_free(&port->dport_ida, dport->id);
>>  	put_device(dport->dport_dev);
>>  }
>>  
>> @@ -1114,7 +1116,7 @@ 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);
>>  }
>>  
>> @@ -1126,7 +1128,7 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  	char link_name[CXL_TARGET_STRLEN];
>>  	struct cxl_dport *dport;
>>  	struct device *host;
>> -	int rc;
>> +	int id, rc;
>>  
>>  	if (is_cxl_root(port))
>>  		host = port->uport_dev;
>> @@ -1139,29 +1141,41 @@ __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) >=
>> -	    CXL_TARGET_STRLEN)
>> +	id = ida_alloc(&port->dport_ida, GFP_KERNEL);
>> +	if (id < 0)
>> +		return ERR_PTR(id);
>> +
>> +	if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", id) >=
>> +	    CXL_TARGET_STRLEN) {
>> +		ida_free(&port->dport_ida, id);
>>  		return ERR_PTR(-EINVAL);
>> +	}
>>  
>>  	dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
>> -	if (!dport)
>> +	if (!dport) {
>> +		ida_free(&port->dport_ida, id);
> 
> Rather than sprinkle ida_free() throughout this function, just make
> allocation do all the allocations in one go, something like:
> 
> static struct cxl_dport *cxl_alloc_dport(struct cxl_port *port, ...)
> {
> 	struct cxl_dport *dport __free(kfree) = kzalloc(&port->dev, 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_id = port_id;
> 	dport->port = port;
> 	dport->id = id;
> 
> 	return no_free_ptr(dport);
> }
> 
> dport = cxl_alloc_dport(...)
> if (!dport)
> 	return -ENOMEM;
> rc = devm_add_action_or_reset(&port->dev, free_dport, dport);
> if (rc)
> 	return rc;


Thanks. Will do that.

[PATCH 2/4] cxl: Defer hardware dport->port_id assignment and registers probing

[Dave Jiang]

Current implementation only enuemrates the dports dupring the port probe.
Without an endpoint connected, the dport may not be active during port
probe. This scheme may prevent a valid hardware dport id to be retrieved
and MMIO registers to be read when an endpoint is hot-plugged. Move the hw
dport id assignment and the register probing to behind memdev probe so the
endpoint is guaranteed to be connected.

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

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/core.h |  4 ++
 drivers/cxl/core/pci.c  | 74 ++++++++++++++++++++++++++++------
 drivers/cxl/core/port.c | 88 ++++++++++++++++++++++-------------------
 drivers/cxl/cxl.h       |  1 +
 drivers/cxl/port.c      |  2 -
 5 files changed, 114 insertions(+), 55 deletions(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 15699299dc11..e2822ead6a67 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_probe(struct cxl_dport *dport, resource_size_t component_reg_phys,
+		    resource_size_t rcrb);
+void cxl_port_probe_dports(struct cxl_port *port);
+
 #endif /* __CXL_CORE_H__ */
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 96fecb799cbc..a47dd032abd7 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -24,6 +24,66 @@ 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");
 
+static int probe_dports(struct cxl_dport *dport)
+{
+	struct device *dport_dev = dport->dport_dev;
+	struct cxl_port *port = dport->port;
+	struct cxl_register_map map;
+	struct pci_dev *pdev;
+	u32 lnkcap, port_num;
+	int rc;
+
+	if (!dev_is_pci(dport_dev))
+		return 0;
+
+	/*
+	 * dport->port_id is valid means that dport has been probed and is
+	 * setup.
+	 */
+	if (dport->port_id != CXL_PORT_ID_INVALID)
+		return 0;
+
+	pdev = to_pci_dev(dport_dev);
+	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");
+		return 0;
+	}
+
+	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
+	rc = cxl_dport_probe(dport, map.resource, CXL_RESOURCE_NONE);
+	if (rc)
+		return rc;
+
+	/*
+	 * port_id is only set if the register block is also probed
+	 * successfully.
+	 */
+	dport->port_id = port_num;
+	cxl_switch_parse_cdat(port);
+
+	return 0;
+}
+
+/**
+ * cxl_port_probe_dports - probe downstream ports of the upstream port
+ * @port: cxl_port whose ->uport_dev is the upstream of dports to be probed
+ *
+ */
+void cxl_port_probe_dports(struct cxl_port *port)
+{
+	struct cxl_dport *dport;
+	unsigned long index;
+
+	device_lock_assert(&port->dev);
+	xa_for_each(&port->dports, index, dport)
+		probe_dports(dport);
+}
+
 struct cxl_walk_context {
 	struct pci_bus *bus;
 	struct cxl_port *port;
@@ -37,10 +97,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,16 +105,9 @@ 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, port_num, map.resource);
+	dport = devm_cxl_add_dport(port, &pdev->dev, CXL_PORT_ID_INVALID,
+				   CXL_RESOURCE_NONE);
 	if (IS_ERR(dport)) {
 		ctx->error = PTR_ERR(dport);
 		return PTR_ERR(dport);
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index e90e55bc11ac..1c772c516dbe 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1059,18 +1059,9 @@ static struct cxl_dport *find_dport(struct cxl_port *port, int port_id)
 
 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(port, dport->port_id);
-	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),
-			dev_name(dup->dport_dev));
-		return -EBUSY;
-	}
 
 	rc = xa_insert(&port->dports, (unsigned long)dport->dport_dev, dport,
 		       GFP_KERNEL);
@@ -1120,6 +1111,45 @@ static void cxl_dport_unlink(void *data)
 	sysfs_remove_link(&port->dev.kobj, link_name);
 }
 
+int cxl_dport_probe(struct cxl_dport *dport, resource_size_t component_reg_phys,
+		    resource_size_t rcrb)
+{
+	struct device *dport_dev = dport->dport_dev;
+	struct cxl_port *port = dport->port;
+	int rc;
+
+	if (rcrb == CXL_RESOURCE_NONE) {
+		rc = cxl_dport_setup_regs(&port->dev, dport,
+					  component_reg_phys);
+		if (rc)
+			return rc;
+	} else {
+		dport->rcrb.base = rcrb;
+		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
+							 CXL_RCRB_DOWNSTREAM);
+		if (component_reg_phys == CXL_RESOURCE_NONE) {
+			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
+			return -ENXIO;
+		}
+
+		/*
+		 * RCH @dport is not ready to map until associated with its
+		 * memdev
+		 */
+		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
+		if (rc)
+			return rc;
+
+		dport->rch = true;
+	}
+
+	if (component_reg_phys != CXL_RESOURCE_NONE)
+		dev_dbg(dport_dev, "Component Registers found for dport: %pa\n",
+			&component_reg_phys);
+
+	return 0;
+}
+
 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,
@@ -1162,40 +1192,12 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
 	dport->port = port;
 	dport->id = id;
 
-	if (rcrb == CXL_RESOURCE_NONE) {
-		rc = cxl_dport_setup_regs(&port->dev, dport,
-					  component_reg_phys);
-		if (rc) {
-			ida_free(&port->dport_ida, id);
-			return ERR_PTR(rc);
-		}
-	} else {
-		dport->rcrb.base = rcrb;
-		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
-							 CXL_RCRB_DOWNSTREAM);
-		if (component_reg_phys == CXL_RESOURCE_NONE) {
-			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
-			ida_free(&port->dport_ida, id);
-			return ERR_PTR(-ENXIO);
-		}
-
-		/*
-		 * RCH @dport is not ready to map until associated with its
-		 * memdev
-		 */
-		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
-		if (rc) {
-			ida_free(&port->dport_ida, id);
-			return ERR_PTR(rc);
-		}
-
-		dport->rch = true;
+	rc = cxl_dport_probe(dport, component_reg_phys, rcrb);
+	if (rc) {
+		ida_free(&port->dport_ida, id);
+		return ERR_PTR(rc);
 	}
 
-	if (component_reg_phys != CXL_RESOURCE_NONE)
-		dev_dbg(dport_dev, "Component Registers found for dport: %pa\n",
-			&component_reg_phys);
-
 	cond_cxl_root_lock(port);
 	rc = add_dport(port, dport);
 	cond_cxl_root_unlock(port);
@@ -1684,6 +1686,10 @@ 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));
+
+			scoped_guard(device, &port->dev)
+				cxl_port_probe_dports(port);
+
 			rc = cxl_add_ep(dport, &cxlmd->dev);
 
 			/*
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index c942fa40c869..0e61b76f5c13 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_PORT_ID_INVALID -1
 
 /**
  * struct cxl_decoder - Common CXL HDM Decoder Attributes
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index a35fc5552845..30c0335089b9 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -69,8 +69,6 @@ static int cxl_switch_port_probe(struct cxl_port *port)
 	if (rc < 0)
 		return rc;
 
-	cxl_switch_parse_cdat(port);
-
 	cxlhdm = devm_cxl_setup_hdm(port, NULL);
 	if (!IS_ERR(cxlhdm))
 		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
-- 
2.49.0

Re: [PATCH 2/4] cxl: Defer hardware dport->port_id assignment and registers probing

[Li Ming]

On 4/5/2025 6:57 AM, Dave Jiang wrote:
> Current implementation only enuemrates the dports dupring the port probe.
s/dupring/during
> Without an endpoint connected, the dport may not be active during port
> probe. This scheme may prevent a valid hardware dport id to be retrieved
> and MMIO registers to be read when an endpoint is hot-plugged. Move the hw
> dport id assignment and the register probing to behind memdev probe so the
> endpoint is guaranteed to be connected.
>
> The detection of duplicate dport for add_dport() is removed. The port_id
> is not read from the hw at this point any longer. The port->id will always
> be unique since it's retrieved from an ida. The dup detection thus become
> irrelevant.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/core.h |  4 ++
>  drivers/cxl/core/pci.c  | 74 ++++++++++++++++++++++++++++------
>  drivers/cxl/core/port.c | 88 ++++++++++++++++++++++-------------------
>  drivers/cxl/cxl.h       |  1 +
>  drivers/cxl/port.c      |  2 -
>  5 files changed, 114 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 15699299dc11..e2822ead6a67 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_probe(struct cxl_dport *dport, resource_size_t component_reg_phys,
> +		    resource_size_t rcrb);
> +void cxl_port_probe_dports(struct cxl_port *port);
> +
>  #endif /* __CXL_CORE_H__ */
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 96fecb799cbc..a47dd032abd7 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -24,6 +24,66 @@ 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");
>  
> +static int probe_dports(struct cxl_dport *dport)
maybe rename it to probe_dport(), because it just probes one dport.
> +{
> +	struct device *dport_dev = dport->dport_dev;
> +	struct cxl_port *port = dport->port;
> +	struct cxl_register_map map;
> +	struct pci_dev *pdev;
> +	u32 lnkcap, port_num;
> +	int rc;
> +
> +	if (!dev_is_pci(dport_dev))
> +		return 0;
> +
> +	/*
> +	 * dport->port_id is valid means that dport has been probed and is
> +	 * setup.
> +	 */
> +	if (dport->port_id != CXL_PORT_ID_INVALID)
> +		return 0;
> +
> +	pdev = to_pci_dev(dport_dev);
> +	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");
> +		return 0;
> +	}
> +
> +	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
> +	rc = cxl_dport_probe(dport, map.resource, CXL_RESOURCE_NONE);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * port_id is only set if the register block is also probed
> +	 * successfully.
> +	 */
> +	dport->port_id = port_num;
> +	cxl_switch_parse_cdat(port);
> +
> +	return 0;
> +}
> +
> +/**
> + * cxl_port_probe_dports - probe downstream ports of the upstream port
> + * @port: cxl_port whose ->uport_dev is the upstream of dports to be probed
> + *
> + */
> +void cxl_port_probe_dports(struct cxl_port *port)
> +{
> +	struct cxl_dport *dport;
> +	unsigned long index;
> +
> +	device_lock_assert(&port->dev);
> +	xa_for_each(&port->dports, index, dport)
> +		probe_dports(dport);
> +}
> +

This function is called during each endpoint attaching, I am a little confused that it probes all dports under the port at the same time during a new endpoint attaching to the port.

Per commit log, hardware dport id and MMIO registers are available only when an endpoint connects, seems like probing other dports which no endpoint connects is redundant. is it possible to just probe the dport which the attaching endpoint is under?

Maybe I miss some details, please correct me if I am wrong.

>  struct cxl_walk_context {
>  	struct pci_bus *bus;
>  	struct cxl_port *port;
> @@ -37,10 +97,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,16 +105,9 @@ 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, port_num, map.resource);
> +	dport = devm_cxl_add_dport(port, &pdev->dev, CXL_PORT_ID_INVALID,
> +				   CXL_RESOURCE_NONE);
>  	if (IS_ERR(dport)) {
>  		ctx->error = PTR_ERR(dport);
>  		return PTR_ERR(dport);
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index e90e55bc11ac..1c772c516dbe 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1059,18 +1059,9 @@ static struct cxl_dport *find_dport(struct cxl_port *port, int port_id)
>  
>  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(port, dport->port_id);
> -	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),
> -			dev_name(dup->dport_dev));
> -		return -EBUSY;
> -	}
>  
>  	rc = xa_insert(&port->dports, (unsigned long)dport->dport_dev, dport,
>  		       GFP_KERNEL);
> @@ -1120,6 +1111,45 @@ static void cxl_dport_unlink(void *data)
>  	sysfs_remove_link(&port->dev.kobj, link_name);
>  }
>  
> +int cxl_dport_probe(struct cxl_dport *dport, resource_size_t component_reg_phys,
> +		    resource_size_t rcrb)
> +{
> +	struct device *dport_dev = dport->dport_dev;
> +	struct cxl_port *port = dport->port;
> +	int rc;
> +
> +	if (rcrb == CXL_RESOURCE_NONE) {
> +		rc = cxl_dport_setup_regs(&port->dev, dport,
> +					  component_reg_phys);
> +		if (rc)
> +			return rc;
> +	} else {
> +		dport->rcrb.base = rcrb;
> +		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
> +							 CXL_RCRB_DOWNSTREAM);
> +		if (component_reg_phys == CXL_RESOURCE_NONE) {
> +			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
> +			return -ENXIO;
> +		}
> +
> +		/*
> +		 * RCH @dport is not ready to map until associated with its
> +		 * memdev
> +		 */
> +		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
> +		if (rc)
> +			return rc;
> +
> +		dport->rch = true;
> +	}
> +
> +	if (component_reg_phys != CXL_RESOURCE_NONE)
> +		dev_dbg(dport_dev, "Component Registers found for dport: %pa\n",
> +			&component_reg_phys);
> +
> +	return 0;
> +}
> +
>  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,
> @@ -1162,40 +1192,12 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>  	dport->port = port;
>  	dport->id = id;
>  
> -	if (rcrb == CXL_RESOURCE_NONE) {
> -		rc = cxl_dport_setup_regs(&port->dev, dport,
> -					  component_reg_phys);
> -		if (rc) {
> -			ida_free(&port->dport_ida, id);
> -			return ERR_PTR(rc);
> -		}
> -	} else {
> -		dport->rcrb.base = rcrb;
> -		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
> -							 CXL_RCRB_DOWNSTREAM);
> -		if (component_reg_phys == CXL_RESOURCE_NONE) {
> -			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
> -			ida_free(&port->dport_ida, id);
> -			return ERR_PTR(-ENXIO);
> -		}
> -
> -		/*
> -		 * RCH @dport is not ready to map until associated with its
> -		 * memdev
> -		 */
> -		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
> -		if (rc) {
> -			ida_free(&port->dport_ida, id);
> -			return ERR_PTR(rc);
> -		}
> -
> -		dport->rch = true;
> +	rc = cxl_dport_probe(dport, component_reg_phys, rcrb);
> +	if (rc) {
> +		ida_free(&port->dport_ida, id);
> +		return ERR_PTR(rc);
>  	}
>  
> -	if (component_reg_phys != CXL_RESOURCE_NONE)
> -		dev_dbg(dport_dev, "Component Registers found for dport: %pa\n",
> -			&component_reg_phys);
> -
>  	cond_cxl_root_lock(port);
>  	rc = add_dport(port, dport);
>  	cond_cxl_root_unlock(port);
> @@ -1684,6 +1686,10 @@ 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));
> +
> +			scoped_guard(device, &port->dev)
> +				cxl_port_probe_dports(port);
Just like above comment, is it possible to be cxl_port_probe_dport(port, dport) here?
> +
>  			rc = cxl_add_ep(dport, &cxlmd->dev);
>  
>  			/*
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index c942fa40c869..0e61b76f5c13 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_PORT_ID_INVALID -1
>  
>  /**
>   * struct cxl_decoder - Common CXL HDM Decoder Attributes
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index a35fc5552845..30c0335089b9 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -69,8 +69,6 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>  	if (rc < 0)
>  		return rc;
>  
> -	cxl_switch_parse_cdat(port);
> -
>  	cxlhdm = devm_cxl_setup_hdm(port, NULL);
>  	if (!IS_ERR(cxlhdm))
>  		return devm_cxl_enumerate_decoders(cxlhdm, NULL);

Re: [PATCH 2/4] cxl: Defer hardware dport->port_id assignment and registers probing

[Dave Jiang]

On 4/10/25 7:20 PM, Li Ming wrote:
> On 4/5/2025 6:57 AM, Dave Jiang wrote:
>> Current implementation only enuemrates the dports dupring the port probe.
> s/dupring/during
>> Without an endpoint connected, the dport may not be active during port
>> probe. This scheme may prevent a valid hardware dport id to be retrieved
>> and MMIO registers to be read when an endpoint is hot-plugged. Move the hw
>> dport id assignment and the register probing to behind memdev probe so the
>> endpoint is guaranteed to be connected.
>>
>> The detection of duplicate dport for add_dport() is removed. The port_id
>> is not read from the hw at this point any longer. The port->id will always
>> be unique since it's retrieved from an ida. The dup detection thus become
>> irrelevant.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  drivers/cxl/core/core.h |  4 ++
>>  drivers/cxl/core/pci.c  | 74 ++++++++++++++++++++++++++++------
>>  drivers/cxl/core/port.c | 88 ++++++++++++++++++++++-------------------
>>  drivers/cxl/cxl.h       |  1 +
>>  drivers/cxl/port.c      |  2 -
>>  5 files changed, 114 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
>> index 15699299dc11..e2822ead6a67 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_probe(struct cxl_dport *dport, resource_size_t component_reg_phys,
>> +		    resource_size_t rcrb);
>> +void cxl_port_probe_dports(struct cxl_port *port);
>> +
>>  #endif /* __CXL_CORE_H__ */
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 96fecb799cbc..a47dd032abd7 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -24,6 +24,66 @@ 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");
>>  
>> +static int probe_dports(struct cxl_dport *dport)
> maybe rename it to probe_dport(), because it just probes one dport.

ok

>> +{
>> +	struct device *dport_dev = dport->dport_dev;
>> +	struct cxl_port *port = dport->port;
>> +	struct cxl_register_map map;
>> +	struct pci_dev *pdev;
>> +	u32 lnkcap, port_num;
>> +	int rc;
>> +
>> +	if (!dev_is_pci(dport_dev))
>> +		return 0;
>> +
>> +	/*
>> +	 * dport->port_id is valid means that dport has been probed and is
>> +	 * setup.
>> +	 */
>> +	if (dport->port_id != CXL_PORT_ID_INVALID)
>> +		return 0;
>> +
>> +	pdev = to_pci_dev(dport_dev);
>> +	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");
>> +		return 0;
>> +	}
>> +
>> +	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
>> +	rc = cxl_dport_probe(dport, map.resource, CXL_RESOURCE_NONE);
>> +	if (rc)
>> +		return rc;
>> +
>> +	/*
>> +	 * port_id is only set if the register block is also probed
>> +	 * successfully.
>> +	 */
>> +	dport->port_id = port_num;
>> +	cxl_switch_parse_cdat(port);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * cxl_port_probe_dports - probe downstream ports of the upstream port
>> + * @port: cxl_port whose ->uport_dev is the upstream of dports to be probed
>> + *
>> + */
>> +void cxl_port_probe_dports(struct cxl_port *port)
>> +{
>> +	struct cxl_dport *dport;
>> +	unsigned long index;
>> +
>> +	device_lock_assert(&port->dev);
>> +	xa_for_each(&port->dports, index, dport)
>> +		probe_dports(dport);
>> +}
>> +
> 
> This function is called during each endpoint attaching, I am a little confused that it probes all dports under the port at the same time during a new endpoint attaching to the port.
> 
> Per commit log, hardware dport id and MMIO registers are available only when an endpoint connects, seems like probing other dports which no endpoint connects is redundant. is it possible to just probe the dport which the attaching endpoint is under?
> 
> Maybe I miss some details, please correct me if I am wrong.

No it was a mistake. I'll correct that and only update the dport at hand.

DJ

> 
>>  struct cxl_walk_context {
>>  	struct pci_bus *bus;
>>  	struct cxl_port *port;
>> @@ -37,10 +97,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,16 +105,9 @@ 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, port_num, map.resource);
>> +	dport = devm_cxl_add_dport(port, &pdev->dev, CXL_PORT_ID_INVALID,
>> +				   CXL_RESOURCE_NONE);
>>  	if (IS_ERR(dport)) {
>>  		ctx->error = PTR_ERR(dport);
>>  		return PTR_ERR(dport);
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index e90e55bc11ac..1c772c516dbe 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -1059,18 +1059,9 @@ static struct cxl_dport *find_dport(struct cxl_port *port, int port_id)
>>  
>>  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(port, dport->port_id);
>> -	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),
>> -			dev_name(dup->dport_dev));
>> -		return -EBUSY;
>> -	}
>>  
>>  	rc = xa_insert(&port->dports, (unsigned long)dport->dport_dev, dport,
>>  		       GFP_KERNEL);
>> @@ -1120,6 +1111,45 @@ static void cxl_dport_unlink(void *data)
>>  	sysfs_remove_link(&port->dev.kobj, link_name);
>>  }
>>  
>> +int cxl_dport_probe(struct cxl_dport *dport, resource_size_t component_reg_phys,
>> +		    resource_size_t rcrb)
>> +{
>> +	struct device *dport_dev = dport->dport_dev;
>> +	struct cxl_port *port = dport->port;
>> +	int rc;
>> +
>> +	if (rcrb == CXL_RESOURCE_NONE) {
>> +		rc = cxl_dport_setup_regs(&port->dev, dport,
>> +					  component_reg_phys);
>> +		if (rc)
>> +			return rc;
>> +	} else {
>> +		dport->rcrb.base = rcrb;
>> +		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
>> +							 CXL_RCRB_DOWNSTREAM);
>> +		if (component_reg_phys == CXL_RESOURCE_NONE) {
>> +			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
>> +			return -ENXIO;
>> +		}
>> +
>> +		/*
>> +		 * RCH @dport is not ready to map until associated with its
>> +		 * memdev
>> +		 */
>> +		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
>> +		if (rc)
>> +			return rc;
>> +
>> +		dport->rch = true;
>> +	}
>> +
>> +	if (component_reg_phys != CXL_RESOURCE_NONE)
>> +		dev_dbg(dport_dev, "Component Registers found for dport: %pa\n",
>> +			&component_reg_phys);
>> +
>> +	return 0;
>> +}
>> +
>>  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,
>> @@ -1162,40 +1192,12 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  	dport->port = port;
>>  	dport->id = id;
>>  
>> -	if (rcrb == CXL_RESOURCE_NONE) {
>> -		rc = cxl_dport_setup_regs(&port->dev, dport,
>> -					  component_reg_phys);
>> -		if (rc) {
>> -			ida_free(&port->dport_ida, id);
>> -			return ERR_PTR(rc);
>> -		}
>> -	} else {
>> -		dport->rcrb.base = rcrb;
>> -		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
>> -							 CXL_RCRB_DOWNSTREAM);
>> -		if (component_reg_phys == CXL_RESOURCE_NONE) {
>> -			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
>> -			ida_free(&port->dport_ida, id);
>> -			return ERR_PTR(-ENXIO);
>> -		}
>> -
>> -		/*
>> -		 * RCH @dport is not ready to map until associated with its
>> -		 * memdev
>> -		 */
>> -		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
>> -		if (rc) {
>> -			ida_free(&port->dport_ida, id);
>> -			return ERR_PTR(rc);
>> -		}
>> -
>> -		dport->rch = true;
>> +	rc = cxl_dport_probe(dport, component_reg_phys, rcrb);
>> +	if (rc) {
>> +		ida_free(&port->dport_ida, id);
>> +		return ERR_PTR(rc);
>>  	}
>>  
>> -	if (component_reg_phys != CXL_RESOURCE_NONE)
>> -		dev_dbg(dport_dev, "Component Registers found for dport: %pa\n",
>> -			&component_reg_phys);
>> -
>>  	cond_cxl_root_lock(port);
>>  	rc = add_dport(port, dport);
>>  	cond_cxl_root_unlock(port);
>> @@ -1684,6 +1686,10 @@ 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));
>> +
>> +			scoped_guard(device, &port->dev)
>> +				cxl_port_probe_dports(port);
> Just like above comment, is it possible to be cxl_port_probe_dport(port, dport) here?
>> +
>>  			rc = cxl_add_ep(dport, &cxlmd->dev);
>>  
>>  			/*
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index c942fa40c869..0e61b76f5c13 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_PORT_ID_INVALID -1
>>  
>>  /**
>>   * struct cxl_decoder - Common CXL HDM Decoder Attributes
>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>> index a35fc5552845..30c0335089b9 100644
>> --- a/drivers/cxl/port.c
>> +++ b/drivers/cxl/port.c
>> @@ -69,8 +69,6 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>>  	if (rc < 0)
>>  		return rc;
>>  
>> -	cxl_switch_parse_cdat(port);
>> -
>>  	cxlhdm = devm_cxl_setup_hdm(port, NULL);
>>  	if (!IS_ERR(cxlhdm))
>>  		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
> 
> 

Re: [PATCH 2/4] cxl: Defer hardware dport->port_id assignment and registers probing

[Jonathan Cameron]

On Fri, 4 Apr 2025 15:57:34 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Current implementation only enuemrates the dports dupring 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.
A few additional comments from me.


Can we point at anything in the PCI spec to say whether those IDs not being
valid is allowed or not?  I always like to know whether to grump at hardware
folk or not if it they manage to duplicate something unexpected :)


> 
> 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.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/core.h |  4 ++
>  drivers/cxl/core/pci.c  | 74 ++++++++++++++++++++++++++++------
>  drivers/cxl/core/port.c | 88 ++++++++++++++++++++++-------------------
>  drivers/cxl/cxl.h       |  1 +
>  drivers/cxl/port.c      |  2 -
>  5 files changed, 114 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 15699299dc11..e2822ead6a67 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_probe(struct cxl_dport *dport, resource_size_t component_reg_phys,
> +		    resource_size_t rcrb);
> +void cxl_port_probe_dports(struct cxl_port *port);
> +
>  #endif /* __CXL_CORE_H__ */
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 96fecb799cbc..a47dd032abd7 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -24,6 +24,66 @@ 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");
>  
> +static int probe_dports(struct cxl_dport *dport)

Given return value is always ignored, maybe don't return anything?

> +{
> +	struct device *dport_dev = dport->dport_dev;
> +	struct cxl_port *port = dport->port;
> +	struct cxl_register_map map;
> +	struct pci_dev *pdev;
> +	u32 lnkcap, port_num;
> +	int rc;
> +
> +	if (!dev_is_pci(dport_dev))
> +		return 0;
> +
> +	/*
> +	 * dport->port_id is valid means that dport has been probed and is
> +	 * setup.
> +	 */
> +	if (dport->port_id != CXL_PORT_ID_INVALID)
> +		return 0;
> +
> +	pdev = to_pci_dev(dport_dev);
> +	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
> +				  &lnkcap))
> +		return 0;

I'd be tempted to return an error that means come back later and eat it
at next level up if possible (though currently you eat all errors so
that is fine ;)

> +
> +	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
> +	if (rc) {
> +		dev_dbg(&port->dev, "failed to find component registers\n");
> +		return 0;

Likewise. Something called probe_dports() to me should be returning an erorr
if this happens and caller be responsible for ignoring that or not rather
than errors being eaten down here.

> +	}
> +
> +	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
> +	rc = cxl_dport_probe(dport, map.resource, CXL_RESOURCE_NONE);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * port_id is only set if the register block is also probed
> +	 * successfully.
> +	 */
> +	dport->port_id = port_num;
> +	cxl_switch_parse_cdat(port);
> +
> +	return 0;
> +}
> +
> +/**
> + * cxl_port_probe_dports - probe downstream ports of the upstream port
> + * @port: cxl_port whose ->uport_dev is the upstream of dports to be probed
> + *

Bonus blank line doesn't add anything.

> + */
> +void cxl_port_probe_dports(struct cxl_port *port)
> +{
> +	struct cxl_dport *dport;
> +	unsigned long index;
> +
> +	device_lock_assert(&port->dev);
> +	xa_for_each(&port->dports, index, dport)
> +		probe_dports(dport);
> +}

> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index e90e55bc11ac..1c772c516dbe 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c

> @@ -1120,6 +1111,45 @@ static void cxl_dport_unlink(void *data)
>  	sysfs_remove_link(&port->dev.kobj, link_name);
>  }
>  
> +int cxl_dport_probe(struct cxl_dport *dport, resource_size_t component_reg_phys,
> +		    resource_size_t rcrb)
> +{
> +	struct device *dport_dev = dport->dport_dev;
> +	struct cxl_port *port = dport->port;
> +	int rc;
> +
> +	if (rcrb == CXL_RESOURCE_NONE) {
> +		rc = cxl_dport_setup_regs(&port->dev, dport,
> +					  component_reg_phys);
> +		if (rc)
> +			return rc;
> +	} else {
> +		dport->rcrb.base = rcrb;
> +		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
> +							 CXL_RCRB_DOWNSTREAM);
> +		if (component_reg_phys == CXL_RESOURCE_NONE) {
> +			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
> +			return -ENXIO;
> +		}
> +
> +		/*
> +		 * RCH @dport is not ready to map until associated with its
> +		 * memdev
> +		 */
> +		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
> +		if (rc)
> +			return rc;
> +
> +		dport->rch = true;
> +	}
> +
> +	if (component_reg_phys != CXL_RESOURCE_NONE)
> +		dev_dbg(dport_dev, "Component Registers found for dport: %pa\n",
> +			&component_reg_phys);
> +
> +	return 0;
> +}
> +
>  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,
> @@ -1162,40 +1192,12 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>  	dport->port = port;
>  	dport->id = id;
> 

Maybe do this factoring out as a precursor patch?

> -	if (rcrb == CXL_RESOURCE_NONE) {
> -		rc = cxl_dport_setup_regs(&port->dev, dport,
> -					  component_reg_phys);
> -		if (rc) {
> -			ida_free(&port->dport_ida, id);
> -			return ERR_PTR(rc);
> -		}
> -	} else {
> -		dport->rcrb.base = rcrb;
> -		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
> -							 CXL_RCRB_DOWNSTREAM);
> -		if (component_reg_phys == CXL_RESOURCE_NONE) {
> -			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
> -			ida_free(&port->dport_ida, id);
> -			return ERR_PTR(-ENXIO);
> -		}
> -
> -		/*
> -		 * RCH @dport is not ready to map until associated with its
> -		 * memdev
> -		 */
> -		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
> -		if (rc) {
> -			ida_free(&port->dport_ida, id);
> -			return ERR_PTR(rc);
> -		}
> -
> -		dport->rch = true;
> +	rc = cxl_dport_probe(dport, component_reg_phys, rcrb);
> +	if (rc) {
> +		ida_free(&port->dport_ida, id);
> +		return ERR_PTR(rc);
>  	}
>

Re: [PATCH 2/4] cxl: Defer hardware dport->port_id assignment and registers probing

[Dave Jiang]

On 4/22/25 10:05 AM, Jonathan Cameron wrote:
> On Fri, 4 Apr 2025 15:57:34 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Current implementation only enuemrates the dports dupring 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.
> A few additional comments from me.
> 
> 
> Can we point at anything in the PCI spec to say whether those IDs not being
> valid is allowed or not?  I always like to know whether to grump at hardware
> folk or not if it they manage to duplicate something unexpected :)

Nothing I can find. The thinking was stemmed from this series [1] by Robert.

[1]: https://lore.kernel.org/linux-cxl/20250305100123.3077031-1-rrichter@amd.com/

Mainly the intention of the current series is to enumerate ports triggered by memdev probe(). At that point we know there's an endpoint device and the link is established from EP to RP.

> 
> 
>>
>> 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.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  drivers/cxl/core/core.h |  4 ++
>>  drivers/cxl/core/pci.c  | 74 ++++++++++++++++++++++++++++------
>>  drivers/cxl/core/port.c | 88 ++++++++++++++++++++++-------------------
>>  drivers/cxl/cxl.h       |  1 +
>>  drivers/cxl/port.c      |  2 -
>>  5 files changed, 114 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
>> index 15699299dc11..e2822ead6a67 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_probe(struct cxl_dport *dport, resource_size_t component_reg_phys,
>> +		    resource_size_t rcrb);
>> +void cxl_port_probe_dports(struct cxl_port *port);
>> +
>>  #endif /* __CXL_CORE_H__ */
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 96fecb799cbc..a47dd032abd7 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -24,6 +24,66 @@ 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");
>>  
>> +static int probe_dports(struct cxl_dport *dport)
> 
> Given return value is always ignored, maybe don't return anything?

ok

> 
>> +{
>> +	struct device *dport_dev = dport->dport_dev;
>> +	struct cxl_port *port = dport->port;
>> +	struct cxl_register_map map;
>> +	struct pci_dev *pdev;
>> +	u32 lnkcap, port_num;
>> +	int rc;
>> +
>> +	if (!dev_is_pci(dport_dev))
>> +		return 0;
>> +
>> +	/*
>> +	 * dport->port_id is valid means that dport has been probed and is
>> +	 * setup.
>> +	 */
>> +	if (dport->port_id != CXL_PORT_ID_INVALID)
>> +		return 0;
>> +
>> +	pdev = to_pci_dev(dport_dev);
>> +	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
>> +				  &lnkcap))
>> +		return 0;
> 
> I'd be tempted to return an error that means come back later and eat it
> at next level up if possible (though currently you eat all errors so
> that is fine ;)
> >> +
>> +	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
>> +	if (rc) {
>> +		dev_dbg(&port->dev, "failed to find component registers\n");
>> +		return 0;
> 
> Likewise. Something called probe_dports() to me should be returning an erorr
> if this happens and caller be responsible for ignoring that or not rather
> than errors being eaten down here.
> 
>> +	}
>> +
>> +	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
>> +	rc = cxl_dport_probe(dport, map.resource, CXL_RESOURCE_NONE);
>> +	if (rc)
>> +		return rc;
>> +
>> +	/*
>> +	 * port_id is only set if the register block is also probed
>> +	 * successfully.
>> +	 */
>> +	dport->port_id = port_num;
>> +	cxl_switch_parse_cdat(port);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * cxl_port_probe_dports - probe downstream ports of the upstream port
>> + * @port: cxl_port whose ->uport_dev is the upstream of dports to be probed
>> + *
> 
> Bonus blank line doesn't add anything.

will remove

> 
>> + */
>> +void cxl_port_probe_dports(struct cxl_port *port)
>> +{
>> +	struct cxl_dport *dport;
>> +	unsigned long index;
>> +
>> +	device_lock_assert(&port->dev);
>> +	xa_for_each(&port->dports, index, dport)
>> +		probe_dports(dport);
>> +}
> 
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index e90e55bc11ac..1c772c516dbe 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
> 
>> @@ -1120,6 +1111,45 @@ static void cxl_dport_unlink(void *data)
>>  	sysfs_remove_link(&port->dev.kobj, link_name);
>>  }
>>  
>> +int cxl_dport_probe(struct cxl_dport *dport, resource_size_t component_reg_phys,
>> +		    resource_size_t rcrb)
>> +{
>> +	struct device *dport_dev = dport->dport_dev;
>> +	struct cxl_port *port = dport->port;
>> +	int rc;
>> +
>> +	if (rcrb == CXL_RESOURCE_NONE) {
>> +		rc = cxl_dport_setup_regs(&port->dev, dport,
>> +					  component_reg_phys);
>> +		if (rc)
>> +			return rc;
>> +	} else {
>> +		dport->rcrb.base = rcrb;
>> +		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
>> +							 CXL_RCRB_DOWNSTREAM);
>> +		if (component_reg_phys == CXL_RESOURCE_NONE) {
>> +			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
>> +			return -ENXIO;
>> +		}
>> +
>> +		/*
>> +		 * RCH @dport is not ready to map until associated with its
>> +		 * memdev
>> +		 */
>> +		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
>> +		if (rc)
>> +			return rc;
>> +
>> +		dport->rch = true;
>> +	}
>> +
>> +	if (component_reg_phys != CXL_RESOURCE_NONE)
>> +		dev_dbg(dport_dev, "Component Registers found for dport: %pa\n",
>> +			&component_reg_phys);
>> +
>> +	return 0;
>> +}
>> +
>>  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,
>> @@ -1162,40 +1192,12 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  	dport->port = port;
>>  	dport->id = id;
>>
> 
> Maybe do this factoring out as a precursor patch?

ok will do
> 
>> -	if (rcrb == CXL_RESOURCE_NONE) {
>> -		rc = cxl_dport_setup_regs(&port->dev, dport,
>> -					  component_reg_phys);
>> -		if (rc) {
>> -			ida_free(&port->dport_ida, id);
>> -			return ERR_PTR(rc);
>> -		}
>> -	} else {
>> -		dport->rcrb.base = rcrb;
>> -		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
>> -							 CXL_RCRB_DOWNSTREAM);
>> -		if (component_reg_phys == CXL_RESOURCE_NONE) {
>> -			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
>> -			ida_free(&port->dport_ida, id);
>> -			return ERR_PTR(-ENXIO);
>> -		}
>> -
>> -		/*
>> -		 * RCH @dport is not ready to map until associated with its
>> -		 * memdev
>> -		 */
>> -		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
>> -		if (rc) {
>> -			ida_free(&port->dport_ida, id);
>> -			return ERR_PTR(rc);
>> -		}
>> -
>> -		dport->rch = true;
>> +	rc = cxl_dport_probe(dport, component_reg_phys, rcrb);
>> +	if (rc) {
>> +		ida_free(&port->dport_ida, id);
>> +		return ERR_PTR(rc);
>>  	}
>>

Re: [PATCH 2/4] cxl: Defer hardware dport->port_id assignment and registers probing

[Dan Williams]

Dave Jiang wrote:
> Current implementation only enuemrates the dports dupring the port probe.
> Without an endpoint connected, the dport may not be active during port
> probe. This scheme may prevent a valid hardware dport id to be retrieved
> and MMIO registers to be read when an endpoint is hot-plugged. Move the hw
> dport id assignment and the register probing to behind memdev probe so the
> endpoint is guaranteed to be connected.
> 
> The detection of duplicate dport for add_dport() is removed. The port_id
> is not read from the hw at this point any longer. The port->id will always
> be unique since it's retrieved from an ida. The dup detection thus become
> irrelevant.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/core.h |  4 ++
>  drivers/cxl/core/pci.c  | 74 ++++++++++++++++++++++++++++------
>  drivers/cxl/core/port.c | 88 ++++++++++++++++++++++-------------------
>  drivers/cxl/cxl.h       |  1 +
>  drivers/cxl/port.c      |  2 -
>  5 files changed, 114 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 15699299dc11..e2822ead6a67 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_probe(struct cxl_dport *dport, resource_size_t component_reg_phys,
> +		    resource_size_t rcrb);
> +void cxl_port_probe_dports(struct cxl_port *port);
> +
>  #endif /* __CXL_CORE_H__ */
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 96fecb799cbc..a47dd032abd7 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -24,6 +24,66 @@ 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");
>  
> +static int probe_dports(struct cxl_dport *dport)
> +{
> +	struct device *dport_dev = dport->dport_dev;
> +	struct cxl_port *port = dport->port;
> +	struct cxl_register_map map;
> +	struct pci_dev *pdev;
> +	u32 lnkcap, port_num;
> +	int rc;
> +
> +	if (!dev_is_pci(dport_dev))
> +		return 0;
> +
> +	/*
> +	 * dport->port_id is valid means that dport has been probed and is
> +	 * setup.
> +	 */
> +	if (dport->port_id != CXL_PORT_ID_INVALID)
> +		return 0;
> +
> +	pdev = to_pci_dev(dport_dev);
> +	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");
> +		return 0;
> +	}
> +
> +	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
> +	rc = cxl_dport_probe(dport, map.resource, CXL_RESOURCE_NONE);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * port_id is only set if the register block is also probed
> +	 * successfully.
> +	 */
> +	dport->port_id = port_num;
> +	cxl_switch_parse_cdat(port);

Some commentary on why it is safe to re-run this over and over again for
each found port might help future readers. For example might this
disturb already published data for this port?

That comment likely belongs as cxl_switch_parse_cdat() kdoc to clarify
how it is used and re-entered for a given port multiple times.

> +
> +	return 0;
> +}
> +
> +/**
> + * cxl_port_probe_dports - probe downstream ports of the upstream port
> + * @port: cxl_port whose ->uport_dev is the upstream of dports to be probed
> + *
> + */
> +void cxl_port_probe_dports(struct cxl_port *port)
> +{
> +	struct cxl_dport *dport;
> +	unsigned long index;
> +
> +	device_lock_assert(&port->dev);

This also needs to check if port->dev.driver is non-null otherwise it
could be mapping resources onto an idle port.

> +	xa_for_each(&port->dports, index, dport)
> +		probe_dports(dport);
> +}
> +
>  struct cxl_walk_context {
>  	struct pci_bus *bus;
>  	struct cxl_port *port;
> @@ -37,10 +97,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,16 +105,9 @@ 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, port_num, map.resource);
> +	dport = devm_cxl_add_dport(port, &pdev->dev, CXL_PORT_ID_INVALID,
> +				   CXL_RESOURCE_NONE);

Hm, why pass in an invalid id for the common case vs make the static
case just manually "probe" after add_dport?

>  	if (IS_ERR(dport)) {
>  		ctx->error = PTR_ERR(dport);
>  		return PTR_ERR(dport);
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index e90e55bc11ac..1c772c516dbe 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1059,18 +1059,9 @@ static struct cxl_dport *find_dport(struct cxl_port *port, int port_id)
>  
>  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(port, dport->port_id);
> -	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),
> -			dev_name(dup->dport_dev));
> -		return -EBUSY;
> -	}
>  
>  	rc = xa_insert(&port->dports, (unsigned long)dport->dport_dev, dport,
>  		       GFP_KERNEL);
> @@ -1120,6 +1111,45 @@ static void cxl_dport_unlink(void *data)
>  	sysfs_remove_link(&port->dev.kobj, link_name);
>  }
>  
> +int cxl_dport_probe(struct cxl_dport *dport, resource_size_t component_reg_phys,
> +		    resource_size_t rcrb)
> +{
> +	struct device *dport_dev = dport->dport_dev;
> +	struct cxl_port *port = dport->port;
> +	int rc;
> +
> +	if (rcrb == CXL_RESOURCE_NONE) {
> +		rc = cxl_dport_setup_regs(&port->dev, dport,
> +					  component_reg_phys);
> +		if (rc)
> +			return rc;
> +	} else {
> +		dport->rcrb.base = rcrb;
> +		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
> +							 CXL_RCRB_DOWNSTREAM);
> +		if (component_reg_phys == CXL_RESOURCE_NONE) {
> +			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
> +			return -ENXIO;
> +		}
> +
> +		/*
> +		 * RCH @dport is not ready to map until associated with its
> +		 * memdev
> +		 */
> +		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
> +		if (rc)
> +			return rc;
> +
> +		dport->rch = true;
> +	}

It seems a little bit awkward to maintain the RCRB code in this path.
The whole dport mapping problem is a CXL 2.0 complexity. The CXL 1.1 path
should probably be separated from all this deferral logic to keep it
cleaner.

Keep in mind that devm_cxl_enumerate_ports() early exits in the RCD
case, so I do not expect this code ever runs if cxl_port_probe_dports()
late in devm_cxl_enumerate_ports() is the only caller.

> +
> +	if (component_reg_phys != CXL_RESOURCE_NONE)
> +		dev_dbg(dport_dev, "Component Registers found for dport: %pa\n",
> +			&component_reg_phys);
> +
> +	return 0;
> +}
> +
>  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,
> @@ -1162,40 +1192,12 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>  	dport->port = port;
>  	dport->id = id;
>  
> -	if (rcrb == CXL_RESOURCE_NONE) {
> -		rc = cxl_dport_setup_regs(&port->dev, dport,
> -					  component_reg_phys);
> -		if (rc) {
> -			ida_free(&port->dport_ida, id);
> -			return ERR_PTR(rc);
> -		}
> -	} else {
> -		dport->rcrb.base = rcrb;
> -		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
> -							 CXL_RCRB_DOWNSTREAM);
> -		if (component_reg_phys == CXL_RESOURCE_NONE) {
> -			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
> -			ida_free(&port->dport_ida, id);
> -			return ERR_PTR(-ENXIO);
> -		}
> -
> -		/*
> -		 * RCH @dport is not ready to map until associated with its
> -		 * memdev
> -		 */
> -		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
> -		if (rc) {
> -			ida_free(&port->dport_ida, id);
> -			return ERR_PTR(rc);
> -		}
> -
> -		dport->rch = true;
> +	rc = cxl_dport_probe(dport, component_reg_phys, rcrb);
> +	if (rc) {
> +		ida_free(&port->dport_ida, id);
> +		return ERR_PTR(rc);
>  	}
>  
> -	if (component_reg_phys != CXL_RESOURCE_NONE)
> -		dev_dbg(dport_dev, "Component Registers found for dport: %pa\n",
> -			&component_reg_phys);
> -
>  	cond_cxl_root_lock(port);
>  	rc = add_dport(port, dport);
>  	cond_cxl_root_unlock(port);
> @@ -1684,6 +1686,10 @@ 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));
> +
> +			scoped_guard(device, &port->dev)
> +				cxl_port_probe_dports(port);

A comment here would be nice to indicate why this port did not probe
dports before.

The comment would likely also answer why this is only called in the
"already registered" case, and not the initial port creation case in
add_port_attach_ep().

> +
>  			rc = cxl_add_ep(dport, &cxlmd->dev);
>  
>  			/*
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index c942fa40c869..0e61b76f5c13 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_PORT_ID_INVALID -1
>  
>  /**
>   * struct cxl_decoder - Common CXL HDM Decoder Attributes
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index a35fc5552845..30c0335089b9 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -69,8 +69,6 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>  	if (rc < 0)
>  		return rc;
>  
> -	cxl_switch_parse_cdat(port);
> -
>  	cxlhdm = devm_cxl_setup_hdm(port, NULL);
>  	if (!IS_ERR(cxlhdm))
>  		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
> -- 
> 2.49.0
> 

Re: [PATCH 2/4] cxl: Defer hardware dport->port_id assignment and registers probing

[Dave Jiang]

On 4/22/25 1:12 PM, Dan Williams wrote:
> Dave Jiang wrote:
>> Current implementation only enuemrates the dports dupring the port probe.
>> Without an endpoint connected, the dport may not be active during port
>> probe. This scheme may prevent a valid hardware dport id to be retrieved
>> and MMIO registers to be read when an endpoint is hot-plugged. Move the hw
>> dport id assignment and the register probing to behind memdev probe so the
>> endpoint is guaranteed to be connected.
>>
>> The detection of duplicate dport for add_dport() is removed. The port_id
>> is not read from the hw at this point any longer. The port->id will always
>> be unique since it's retrieved from an ida. The dup detection thus become
>> irrelevant.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  drivers/cxl/core/core.h |  4 ++
>>  drivers/cxl/core/pci.c  | 74 ++++++++++++++++++++++++++++------
>>  drivers/cxl/core/port.c | 88 ++++++++++++++++++++++-------------------
>>  drivers/cxl/cxl.h       |  1 +
>>  drivers/cxl/port.c      |  2 -
>>  5 files changed, 114 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
>> index 15699299dc11..e2822ead6a67 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_probe(struct cxl_dport *dport, resource_size_t component_reg_phys,
>> +		    resource_size_t rcrb);
>> +void cxl_port_probe_dports(struct cxl_port *port);
>> +
>>  #endif /* __CXL_CORE_H__ */
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 96fecb799cbc..a47dd032abd7 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -24,6 +24,66 @@ 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");
>>  
>> +static int probe_dports(struct cxl_dport *dport)
>> +{
>> +	struct device *dport_dev = dport->dport_dev;
>> +	struct cxl_port *port = dport->port;
>> +	struct cxl_register_map map;
>> +	struct pci_dev *pdev;
>> +	u32 lnkcap, port_num;
>> +	int rc;
>> +
>> +	if (!dev_is_pci(dport_dev))
>> +		return 0;
>> +
>> +	/*
>> +	 * dport->port_id is valid means that dport has been probed and is
>> +	 * setup.
>> +	 */
>> +	if (dport->port_id != CXL_PORT_ID_INVALID)
>> +		return 0;
>> +
>> +	pdev = to_pci_dev(dport_dev);
>> +	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");
>> +		return 0;
>> +	}
>> +
>> +	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
>> +	rc = cxl_dport_probe(dport, map.resource, CXL_RESOURCE_NONE);
>> +	if (rc)
>> +		return rc;
>> +
>> +	/*
>> +	 * port_id is only set if the register block is also probed
>> +	 * successfully.
>> +	 */
>> +	dport->port_id = port_num;
>> +	cxl_switch_parse_cdat(port);
> 
> Some commentary on why it is safe to re-run this over and over again for
> each found port might help future readers. For example might this
> disturb already published data for this port?

Rerun this multiple times is harmless as it only updates the switch perf numbers for all the dports. The dport perf numbers from CDAT are static and does not change. I will create a follow on patch to only update the specific dport we are probing to make the code more efficient. 

> 
> That comment likely belongs as cxl_switch_parse_cdat() kdoc to clarify
> how it is used and re-entered for a given port multiple times.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * cxl_port_probe_dports - probe downstream ports of the upstream port
>> + * @port: cxl_port whose ->uport_dev is the upstream of dports to be probed
>> + *
>> + */
>> +void cxl_port_probe_dports(struct cxl_port *port)
>> +{
>> +	struct cxl_dport *dport;
>> +	unsigned long index;
>> +
>> +	device_lock_assert(&port->dev);
> 
> This also needs to check if port->dev.driver is non-null otherwise it
> could be mapping resources onto an idle port.

ok

> 
>> +	xa_for_each(&port->dports, index, dport)
>> +		probe_dports(dport);
>> +}
>> +
>>  struct cxl_walk_context {
>>  	struct pci_bus *bus;
>>  	struct cxl_port *port;
>> @@ -37,10 +97,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,16 +105,9 @@ 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, port_num, map.resource);
>> +	dport = devm_cxl_add_dport(port, &pdev->dev, CXL_PORT_ID_INVALID,
>> +				   CXL_RESOURCE_NONE);
> 
> Hm, why pass in an invalid id for the common case vs make the static
> case just manually "probe" after add_dport?

ok

> 
>>  	if (IS_ERR(dport)) {
>>  		ctx->error = PTR_ERR(dport);
>>  		return PTR_ERR(dport);
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index e90e55bc11ac..1c772c516dbe 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -1059,18 +1059,9 @@ static struct cxl_dport *find_dport(struct cxl_port *port, int port_id)
>>  
>>  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(port, dport->port_id);
>> -	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),
>> -			dev_name(dup->dport_dev));
>> -		return -EBUSY;
>> -	}
>>  
>>  	rc = xa_insert(&port->dports, (unsigned long)dport->dport_dev, dport,
>>  		       GFP_KERNEL);
>> @@ -1120,6 +1111,45 @@ static void cxl_dport_unlink(void *data)
>>  	sysfs_remove_link(&port->dev.kobj, link_name);
>>  }
>>  
>> +int cxl_dport_probe(struct cxl_dport *dport, resource_size_t component_reg_phys,
>> +		    resource_size_t rcrb)
>> +{
>> +	struct device *dport_dev = dport->dport_dev;
>> +	struct cxl_port *port = dport->port;
>> +	int rc;
>> +
>> +	if (rcrb == CXL_RESOURCE_NONE) {
>> +		rc = cxl_dport_setup_regs(&port->dev, dport,
>> +					  component_reg_phys);
>> +		if (rc)
>> +			return rc;
>> +	} else {
>> +		dport->rcrb.base = rcrb;
>> +		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
>> +							 CXL_RCRB_DOWNSTREAM);
>> +		if (component_reg_phys == CXL_RESOURCE_NONE) {
>> +			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
>> +			return -ENXIO;
>> +		}
>> +
>> +		/*
>> +		 * RCH @dport is not ready to map until associated with its
>> +		 * memdev
>> +		 */
>> +		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
>> +		if (rc)
>> +			return rc;
>> +
>> +		dport->rch = true;
>> +	}
> 
> It seems a little bit awkward to maintain the RCRB code in this path.
> The whole dport mapping problem is a CXL 2.0 complexity. The CXL 1.1 path
> should probably be separated from all this deferral logic to keep it
> cleaner.
> 
> Keep in mind that devm_cxl_enumerate_ports() early exits in the RCD
> case, so I do not expect this code ever runs if cxl_port_probe_dports()
> late in devm_cxl_enumerate_ports() is the only caller.

ok

> 
>> +
>> +	if (component_reg_phys != CXL_RESOURCE_NONE)
>> +		dev_dbg(dport_dev, "Component Registers found for dport: %pa\n",
>> +			&component_reg_phys);
>> +
>> +	return 0;
>> +}
>> +
>>  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,
>> @@ -1162,40 +1192,12 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  	dport->port = port;
>>  	dport->id = id;
>>  
>> -	if (rcrb == CXL_RESOURCE_NONE) {
>> -		rc = cxl_dport_setup_regs(&port->dev, dport,
>> -					  component_reg_phys);
>> -		if (rc) {
>> -			ida_free(&port->dport_ida, id);
>> -			return ERR_PTR(rc);
>> -		}
>> -	} else {
>> -		dport->rcrb.base = rcrb;
>> -		component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
>> -							 CXL_RCRB_DOWNSTREAM);
>> -		if (component_reg_phys == CXL_RESOURCE_NONE) {
>> -			dev_warn(dport_dev, "Invalid Component Registers in RCRB");
>> -			ida_free(&port->dport_ida, id);
>> -			return ERR_PTR(-ENXIO);
>> -		}
>> -
>> -		/*
>> -		 * RCH @dport is not ready to map until associated with its
>> -		 * memdev
>> -		 */
>> -		rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
>> -		if (rc) {
>> -			ida_free(&port->dport_ida, id);
>> -			return ERR_PTR(rc);
>> -		}
>> -
>> -		dport->rch = true;
>> +	rc = cxl_dport_probe(dport, component_reg_phys, rcrb);
>> +	if (rc) {
>> +		ida_free(&port->dport_ida, id);
>> +		return ERR_PTR(rc);
>>  	}
>>  
>> -	if (component_reg_phys != CXL_RESOURCE_NONE)
>> -		dev_dbg(dport_dev, "Component Registers found for dport: %pa\n",
>> -			&component_reg_phys);
>> -
>>  	cond_cxl_root_lock(port);
>>  	rc = add_dport(port, dport);
>>  	cond_cxl_root_unlock(port);
>> @@ -1684,6 +1686,10 @@ 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));
>> +
>> +			scoped_guard(device, &port->dev)
>> +				cxl_port_probe_dports(port);
> 
> A comment here would be nice to indicate why this port did not probe
> dports before.
> 
> The comment would likely also answer why this is only called in the
> "already registered" case, and not the initial port creation case in
> add_port_attach_ep().

I think that if we do it at port creation, we also would need to do it here anyways for a port that's already created for the other dports. I think we can just do all of the dports here in a single location. Did I miss something?

DJ

> 
>> +
>>  			rc = cxl_add_ep(dport, &cxlmd->dev);
>>  
>>  			/*
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index c942fa40c869..0e61b76f5c13 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_PORT_ID_INVALID -1
>>  
>>  /**
>>   * struct cxl_decoder - Common CXL HDM Decoder Attributes
>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>> index a35fc5552845..30c0335089b9 100644
>> --- a/drivers/cxl/port.c
>> +++ b/drivers/cxl/port.c
>> @@ -69,8 +69,6 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>>  	if (rc < 0)
>>  		return rc;
>>  
>> -	cxl_switch_parse_cdat(port);
>> -
>>  	cxlhdm = devm_cxl_setup_hdm(port, NULL);
>>  	if (!IS_ERR(cxlhdm))
>>  		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
>> -- 
>> 2.49.0
>>
> 
> 

[PATCH 3/4] cxl: Add late host bridge uport mapping update

[Dave Jiang]

Error message "cxl portN: Couldn't locate the CXL.cache and CXL.mem
capability array header" is reported through testing when a platform is
enabled with PCIe hotplug. The cxl_acpi module is responsible for
enumerating the host bridges through ACPI objects. During the enumeration
of the host bridge upstream ports (uports), the root port (RP) registers
are mapped. The enumeration can happen as soon as the cxl_acpi module
probe() function is called. However if the CXL link between the endpoint
device and the RP is not established before the enumeration happens,
the platform may not expose DVSEC ID 3 and/or DVSEC ID 7 blocks which
triggers the error message.

Add an attempt to map the register block under the memdev probe() port
enumeration. When the PCI probe of the device endpoint is called, the
driver is now communicating with the CXL device and the CXL link is
considered established. Doing the register block mapping at that point
guarantees that the mandatory DVSEC blocks are present.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/acpi.c      | 17 +++++++++-
 drivers/cxl/core/port.c | 72 +++++++++++++++++++++++++++++++++++++++--
 drivers/cxl/cxl.h       |  4 +++
 drivers/cxl/port.c      | 19 ++---------
 4 files changed, 93 insertions(+), 19 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index cb14829bb9be..3c8f04bee9a3 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -662,9 +662,24 @@ static int add_host_bridge_uport(struct device *match, void *arg)
 	if (rc)
 		return rc;
 
-	port = devm_cxl_add_port(host, bridge, component_reg_phys, dport);
+	/*
+	 * While there is a chance the uport gets mapped when the probe
+	 * function gets called, it is not a guarantee due to acpi driver
+	 * can be probed before the root port has established the CXL
+	 * connection to the endpoint device. Bypass mapping during
+	 * port creation by pass in CXL_RESOURCE_NONE for the
+	 * component_reg_phys parameter. After, set the 'resource'
+	 * parameter of port->map to allow a setup via the endpoint
+	 * memdev probe.
+	 */
+	port = devm_cxl_add_port(host, bridge, CXL_RESOURCE_NONE, dport);
 	if (IS_ERR(port))
 		return PTR_ERR(port);
+	port->reg_map = (struct cxl_register_map) {
+		.host = host,
+		.reg_type = CXL_REGLOC_RBI_EMPTY,
+		.resource = component_reg_phys,
+	};
 
 	dev_info(bridge, "host supports CXL\n");
 
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 1c772c516dbe..8c29db214d60 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -758,6 +758,10 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev,
 static int cxl_setup_comp_regs(struct device *host, struct cxl_register_map *map,
 			       resource_size_t component_reg_phys)
 {
+	/* Skip the setup if the map has been setup previously. */
+	if (map->reg_type != CXL_REGLOC_RBI_EMPTY)
+		return 0;
+
 	*map = (struct cxl_register_map) {
 		.host = host,
 		.reg_type = CXL_REGLOC_RBI_EMPTY,
@@ -773,7 +777,7 @@ static int cxl_setup_comp_regs(struct device *host, struct cxl_register_map *map
 	return cxl_setup_regs(map);
 }
 
-static int cxl_port_setup_regs(struct cxl_port *port,
+int cxl_port_setup_regs(struct cxl_port *port,
 			resource_size_t component_reg_phys)
 {
 	if (dev_is_platform(port->uport_dev))
@@ -781,6 +785,7 @@ static int cxl_port_setup_regs(struct cxl_port *port,
 	return cxl_setup_comp_regs(&port->dev, &port->reg_map,
 				   component_reg_phys);
 }
+EXPORT_SYMBOL_NS_GPL(cxl_port_setup_regs, "CXL");
 
 static int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport,
 				resource_size_t component_reg_phys)
@@ -1004,7 +1009,7 @@ int devm_cxl_register_pci_bus(struct device *host, struct device *uport_dev,
 }
 EXPORT_SYMBOL_NS_GPL(devm_cxl_register_pci_bus, "CXL");
 
-static bool dev_is_cxl_root_child(struct device *dev)
+bool dev_is_cxl_root_child(struct device *dev)
 {
 	struct cxl_port *port, *parent;
 
@@ -1021,6 +1026,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)
 {
@@ -1565,6 +1571,57 @@ static resource_size_t find_component_registers(struct device *dev)
 	return map.resource;
 }
 
+int devm_cxl_decoders_setup(struct cxl_port *port)
+{
+	struct cxl_dport *dport;
+	struct cxl_hdm *cxlhdm;
+	unsigned long index;
+	int dports = 0;
+
+	/* Make sure that no decoders have been allocated before proceeding. */
+	if (!ida_is_empty(&port->decoder_ida))
+		return 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_decoders_setup, "CXL");
+
+static int cxl_hb_port_setup(struct cxl_port *port)
+{
+	int rc;
+
+	device_lock_assert(&port->dev);
+
+	if (!dev_is_cxl_root_child(&port->dev))
+		return 0;
+
+	cxl_port_probe_dports(port);
+
+	rc = cxl_port_setup_regs(port, port->reg_map.resource);
+	if (rc)
+		return rc;
+
+	return devm_cxl_decoders_setup(port);
+}
+
 static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 			      struct device *uport_dev,
 			      struct device *dport_dev)
@@ -1605,6 +1662,17 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 			return -ENXIO;
 		}
 
+		/*
+		 * Initiate delayed host bridge setup here in the path of memdev probe
+		 * to ensure that the CXL link is established.
+		 */
+		rc = cxl_hb_port_setup(parent_port);
+		if (rc) {
+			dev_warn(&cxlmd->dev, "Failed HB port setup of %s.\n",
+				 dev_name(&parent_port->dev));
+			return rc;
+		}
+
 		port = find_cxl_port_at(parent_port, dport_dev, &dport);
 		if (!port) {
 			component_reg_phys = find_component_registers(uport_dev);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 0e61b76f5c13..b27e9d3306fe 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -906,6 +906,10 @@ 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_decoders_setup(struct cxl_port *port);
+bool dev_is_cxl_root_child(struct device *dev);
+int cxl_port_setup_regs(struct cxl_port *port,
+			resource_size_t component_reg_phys);
 
 /*
  * Unit test builds overrides this to __weak, find the 'strong' version
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index 30c0335089b9..dc532ee9065f 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -59,7 +59,6 @@ static int discover_region(struct device *dev, void *root)
 
 static int cxl_switch_port_probe(struct cxl_port *port)
 {
-	struct cxl_hdm *cxlhdm;
 	int rc;
 
 	/* Cache the data early to ensure is_visible() works */
@@ -69,22 +68,10 @@ static int cxl_switch_port_probe(struct cxl_port *port)
 	if (rc < 0)
 		return rc;
 
-	cxlhdm = devm_cxl_setup_hdm(port, NULL);
-	if (!IS_ERR(cxlhdm))
-		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
+	if (dev_is_cxl_root_child(&port->dev))
+		return 0;
 
-	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_decoders_setup(port);
 }
 
 static int cxl_endpoint_port_probe(struct cxl_port *port)
-- 
2.49.0

Re: [PATCH 3/4] cxl: Add late host bridge uport mapping update

[Li Ming]

On 4/5/2025 6:57 AM, Dave Jiang wrote:
> Error message "cxl portN: Couldn't locate the CXL.cache and CXL.mem
> capability array header" is reported through testing when a platform is
> enabled with PCIe hotplug. The cxl_acpi module is responsible for
> enumerating the host bridges through ACPI objects. During the enumeration
> of the host bridge upstream ports (uports), the root port (RP) registers
> are mapped. The enumeration can happen as soon as the cxl_acpi module
> probe() function is called. However if the CXL link between the endpoint
> device and the RP is not established before the enumeration happens,
> the platform may not expose DVSEC ID 3 and/or DVSEC ID 7 blocks which
> triggers the error message.
>
> Add an attempt to map the register block under the memdev probe() port
> enumeration. When the PCI probe of the device endpoint is called, the
> driver is now communicating with the CXL device and the CXL link is
> considered established. Doing the register block mapping at that point
> guarantees that the mandatory DVSEC blocks are present.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/acpi.c      | 17 +++++++++-
>  drivers/cxl/core/port.c | 72 +++++++++++++++++++++++++++++++++++++++--
>  drivers/cxl/cxl.h       |  4 +++
>  drivers/cxl/port.c      | 19 ++---------
>  4 files changed, 93 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index cb14829bb9be..3c8f04bee9a3 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -662,9 +662,24 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  	if (rc)
>  		return rc;
>  
> -	port = devm_cxl_add_port(host, bridge, component_reg_phys, dport);
> +	/*
> +	 * While there is a chance the uport gets mapped when the probe
> +	 * function gets called, it is not a guarantee due to acpi driver
> +	 * can be probed before the root port has established the CXL
> +	 * connection to the endpoint device. Bypass mapping during
> +	 * port creation by pass in CXL_RESOURCE_NONE for the
> +	 * component_reg_phys parameter. After, set the 'resource'
> +	 * parameter of port->map to allow a setup via the endpoint
> +	 * memdev probe.
> +	 */
> +	port = devm_cxl_add_port(host, bridge, CXL_RESOURCE_NONE, dport);
>  	if (IS_ERR(port))
>  		return PTR_ERR(port);
> +	port->reg_map = (struct cxl_register_map) {
> +		.host = host,
> +		.reg_type = CXL_REGLOC_RBI_EMPTY,
> +		.resource = component_reg_phys,
> +	};
>  
>  	dev_info(bridge, "host supports CXL\n");
>  
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 1c772c516dbe..8c29db214d60 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -758,6 +758,10 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev,
>  static int cxl_setup_comp_regs(struct device *host, struct cxl_register_map *map,
>  			       resource_size_t component_reg_phys)
>  {
> +	/* Skip the setup if the map has been setup previously. */
> +	if (map->reg_type != CXL_REGLOC_RBI_EMPTY)
> +		return 0;
> +
>  	*map = (struct cxl_register_map) {
>  		.host = host,
>  		.reg_type = CXL_REGLOC_RBI_EMPTY,
> @@ -773,7 +777,7 @@ static int cxl_setup_comp_regs(struct device *host, struct cxl_register_map *map
>  	return cxl_setup_regs(map);
>  }
>  
> -static int cxl_port_setup_regs(struct cxl_port *port,
> +int cxl_port_setup_regs(struct cxl_port *port,
>  			resource_size_t component_reg_phys)
>  {
>  	if (dev_is_platform(port->uport_dev))
> @@ -781,6 +785,7 @@ static int cxl_port_setup_regs(struct cxl_port *port,
>  	return cxl_setup_comp_regs(&port->dev, &port->reg_map,
>  				   component_reg_phys);
>  }
> +EXPORT_SYMBOL_NS_GPL(cxl_port_setup_regs, "CXL");
>  
>  static int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport,
>  				resource_size_t component_reg_phys)
> @@ -1004,7 +1009,7 @@ int devm_cxl_register_pci_bus(struct device *host, struct device *uport_dev,
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_cxl_register_pci_bus, "CXL");
>  
> -static bool dev_is_cxl_root_child(struct device *dev)
> +bool dev_is_cxl_root_child(struct device *dev)
>  {
>  	struct cxl_port *port, *parent;
>  
> @@ -1021,6 +1026,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)
>  {
> @@ -1565,6 +1571,57 @@ static resource_size_t find_component_registers(struct device *dev)
>  	return map.resource;
>  }
>  
> +int devm_cxl_decoders_setup(struct cxl_port *port)
> +{
> +	struct cxl_dport *dport;
> +	struct cxl_hdm *cxlhdm;
> +	unsigned long index;
> +	int dports = 0;
> +
> +	/* Make sure that no decoders have been allocated before proceeding. */
> +	if (!ida_is_empty(&port->decoder_ida))
> +		return 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_decoders_setup, "CXL");
> +
> +static int cxl_hb_port_setup(struct cxl_port *port)
> +{
> +	int rc;
> +
> +	device_lock_assert(&port->dev);
> +
> +	if (!dev_is_cxl_root_child(&port->dev))
> +		return 0;
> +
> +	cxl_port_probe_dports(port);
> +
> +	rc = cxl_port_setup_regs(port, port->reg_map.resource);
> +	if (rc)
> +		return rc;
> +
> +	return devm_cxl_decoders_setup(port);
> +}
> +
>  static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>  			      struct device *uport_dev,
>  			      struct device *dport_dev)
> @@ -1605,6 +1662,17 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>  			return -ENXIO;
>  		}
>  
> +		/*
> +		 * Initiate delayed host bridge setup here in the path of memdev probe
> +		 * to ensure that the CXL link is established.
> +		 */
> +		rc = cxl_hb_port_setup(parent_port);
> +		if (rc) {
> +			dev_warn(&cxlmd->dev, "Failed HB port setup of %s.\n",
> +				 dev_name(&parent_port->dev));
> +			return rc;
> +		}
> +

My understanding is that add_port_attach_ep() is only called for cxl switch port adding. cxl hb ports are already added during cxl acpi probing, they can be found in devm_cxl_enumerate_ports().

So if an endpoint directly connects to a root port, seems like no chance to call cxl_hb_port_setup() in add_port_attach_ep().


>  		port = find_cxl_port_at(parent_port, dport_dev, &dport);
>  		if (!port) {
>  			component_reg_phys = find_component_registers(uport_dev);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 0e61b76f5c13..b27e9d3306fe 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -906,6 +906,10 @@ 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_decoders_setup(struct cxl_port *port);
> +bool dev_is_cxl_root_child(struct device *dev);
> +int cxl_port_setup_regs(struct cxl_port *port,
> +			resource_size_t component_reg_phys);
>  
>  /*
>   * Unit test builds overrides this to __weak, find the 'strong' version
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index 30c0335089b9..dc532ee9065f 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -59,7 +59,6 @@ static int discover_region(struct device *dev, void *root)
>  
>  static int cxl_switch_port_probe(struct cxl_port *port)
>  {
> -	struct cxl_hdm *cxlhdm;
>  	int rc;
>  
>  	/* Cache the data early to ensure is_visible() works */
> @@ -69,22 +68,10 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>  	if (rc < 0)
>  		return rc;
>  
> -	cxlhdm = devm_cxl_setup_hdm(port, NULL);
> -	if (!IS_ERR(cxlhdm))
> -		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
> +	if (dev_is_cxl_root_child(&port->dev))
> +		return 0;
>  
> -	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_decoders_setup(port);
>  }
>  
>  static int cxl_endpoint_port_probe(struct cxl_port *port)

Re: [PATCH 3/4] cxl: Add late host bridge uport mapping update

[Dave Jiang]

On 4/10/25 7:32 PM, Li Ming wrote:
> On 4/5/2025 6:57 AM, Dave Jiang wrote:
>> Error message "cxl portN: Couldn't locate the CXL.cache and CXL.mem
>> capability array header" is reported through testing when a platform is
>> enabled with PCIe hotplug. The cxl_acpi module is responsible for
>> enumerating the host bridges through ACPI objects. During the enumeration
>> of the host bridge upstream ports (uports), the root port (RP) registers
>> are mapped. The enumeration can happen as soon as the cxl_acpi module
>> probe() function is called. However if the CXL link between the endpoint
>> device and the RP is not established before the enumeration happens,
>> the platform may not expose DVSEC ID 3 and/or DVSEC ID 7 blocks which
>> triggers the error message.
>>
>> Add an attempt to map the register block under the memdev probe() port
>> enumeration. When the PCI probe of the device endpoint is called, the
>> driver is now communicating with the CXL device and the CXL link is
>> considered established. Doing the register block mapping at that point
>> guarantees that the mandatory DVSEC blocks are present.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  drivers/cxl/acpi.c      | 17 +++++++++-
>>  drivers/cxl/core/port.c | 72 +++++++++++++++++++++++++++++++++++++++--
>>  drivers/cxl/cxl.h       |  4 +++
>>  drivers/cxl/port.c      | 19 ++---------
>>  4 files changed, 93 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
>> index cb14829bb9be..3c8f04bee9a3 100644
>> --- a/drivers/cxl/acpi.c
>> +++ b/drivers/cxl/acpi.c
>> @@ -662,9 +662,24 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>>  	if (rc)
>>  		return rc;
>>  
>> -	port = devm_cxl_add_port(host, bridge, component_reg_phys, dport);
>> +	/*
>> +	 * While there is a chance the uport gets mapped when the probe
>> +	 * function gets called, it is not a guarantee due to acpi driver
>> +	 * can be probed before the root port has established the CXL
>> +	 * connection to the endpoint device. Bypass mapping during
>> +	 * port creation by pass in CXL_RESOURCE_NONE for the
>> +	 * component_reg_phys parameter. After, set the 'resource'
>> +	 * parameter of port->map to allow a setup via the endpoint
>> +	 * memdev probe.
>> +	 */
>> +	port = devm_cxl_add_port(host, bridge, CXL_RESOURCE_NONE, dport);
>>  	if (IS_ERR(port))
>>  		return PTR_ERR(port);
>> +	port->reg_map = (struct cxl_register_map) {
>> +		.host = host,
>> +		.reg_type = CXL_REGLOC_RBI_EMPTY,
>> +		.resource = component_reg_phys,
>> +	};
>>  
>>  	dev_info(bridge, "host supports CXL\n");
>>  
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 1c772c516dbe..8c29db214d60 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -758,6 +758,10 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev,
>>  static int cxl_setup_comp_regs(struct device *host, struct cxl_register_map *map,
>>  			       resource_size_t component_reg_phys)
>>  {
>> +	/* Skip the setup if the map has been setup previously. */
>> +	if (map->reg_type != CXL_REGLOC_RBI_EMPTY)
>> +		return 0;
>> +
>>  	*map = (struct cxl_register_map) {
>>  		.host = host,
>>  		.reg_type = CXL_REGLOC_RBI_EMPTY,
>> @@ -773,7 +777,7 @@ static int cxl_setup_comp_regs(struct device *host, struct cxl_register_map *map
>>  	return cxl_setup_regs(map);
>>  }
>>  
>> -static int cxl_port_setup_regs(struct cxl_port *port,
>> +int cxl_port_setup_regs(struct cxl_port *port,
>>  			resource_size_t component_reg_phys)
>>  {
>>  	if (dev_is_platform(port->uport_dev))
>> @@ -781,6 +785,7 @@ static int cxl_port_setup_regs(struct cxl_port *port,
>>  	return cxl_setup_comp_regs(&port->dev, &port->reg_map,
>>  				   component_reg_phys);
>>  }
>> +EXPORT_SYMBOL_NS_GPL(cxl_port_setup_regs, "CXL");
>>  
>>  static int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport,
>>  				resource_size_t component_reg_phys)
>> @@ -1004,7 +1009,7 @@ int devm_cxl_register_pci_bus(struct device *host, struct device *uport_dev,
>>  }
>>  EXPORT_SYMBOL_NS_GPL(devm_cxl_register_pci_bus, "CXL");
>>  
>> -static bool dev_is_cxl_root_child(struct device *dev)
>> +bool dev_is_cxl_root_child(struct device *dev)
>>  {
>>  	struct cxl_port *port, *parent;
>>  
>> @@ -1021,6 +1026,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)
>>  {
>> @@ -1565,6 +1571,57 @@ static resource_size_t find_component_registers(struct device *dev)
>>  	return map.resource;
>>  }
>>  
>> +int devm_cxl_decoders_setup(struct cxl_port *port)
>> +{
>> +	struct cxl_dport *dport;
>> +	struct cxl_hdm *cxlhdm;
>> +	unsigned long index;
>> +	int dports = 0;
>> +
>> +	/* Make sure that no decoders have been allocated before proceeding. */
>> +	if (!ida_is_empty(&port->decoder_ida))
>> +		return 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_decoders_setup, "CXL");
>> +
>> +static int cxl_hb_port_setup(struct cxl_port *port)
>> +{
>> +	int rc;
>> +
>> +	device_lock_assert(&port->dev);
>> +
>> +	if (!dev_is_cxl_root_child(&port->dev))
>> +		return 0;
>> +
>> +	cxl_port_probe_dports(port);
>> +
>> +	rc = cxl_port_setup_regs(port, port->reg_map.resource);
>> +	if (rc)
>> +		return rc;
>> +
>> +	return devm_cxl_decoders_setup(port);
>> +}
>> +
>>  static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>>  			      struct device *uport_dev,
>>  			      struct device *dport_dev)
>> @@ -1605,6 +1662,17 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>>  			return -ENXIO;
>>  		}
>>  
>> +		/*
>> +		 * Initiate delayed host bridge setup here in the path of memdev probe
>> +		 * to ensure that the CXL link is established.
>> +		 */
>> +		rc = cxl_hb_port_setup(parent_port);
>> +		if (rc) {
>> +			dev_warn(&cxlmd->dev, "Failed HB port setup of %s.\n",
>> +				 dev_name(&parent_port->dev));
>> +			return rc;
>> +		}
>> +
> 
> My understanding is that add_port_attach_ep() is only called for cxl switch port adding. cxl hb ports are already added during cxl acpi probing, they can be found in devm_cxl_enumerate_ports().
> 
> So if an endpoint directly connects to a root port, seems like no chance to call cxl_hb_port_setup() in add_port_attach_ep().

Thanks for pointing that out. I need to modify my qemu setup to test this scenario.

DJ

> 
> 
>>  		port = find_cxl_port_at(parent_port, dport_dev, &dport);
>>  		if (!port) {
>>  			component_reg_phys = find_component_registers(uport_dev);
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 0e61b76f5c13..b27e9d3306fe 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -906,6 +906,10 @@ 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_decoders_setup(struct cxl_port *port);
>> +bool dev_is_cxl_root_child(struct device *dev);
>> +int cxl_port_setup_regs(struct cxl_port *port,
>> +			resource_size_t component_reg_phys);
>>  
>>  /*
>>   * Unit test builds overrides this to __weak, find the 'strong' version
>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>> index 30c0335089b9..dc532ee9065f 100644
>> --- a/drivers/cxl/port.c
>> +++ b/drivers/cxl/port.c
>> @@ -59,7 +59,6 @@ static int discover_region(struct device *dev, void *root)
>>  
>>  static int cxl_switch_port_probe(struct cxl_port *port)
>>  {
>> -	struct cxl_hdm *cxlhdm;
>>  	int rc;
>>  
>>  	/* Cache the data early to ensure is_visible() works */
>> @@ -69,22 +68,10 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>>  	if (rc < 0)
>>  		return rc;
>>  
>> -	cxlhdm = devm_cxl_setup_hdm(port, NULL);
>> -	if (!IS_ERR(cxlhdm))
>> -		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
>> +	if (dev_is_cxl_root_child(&port->dev))
>> +		return 0;
>>  
>> -	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_decoders_setup(port);
>>  }
>>  
>>  static int cxl_endpoint_port_probe(struct cxl_port *port)
> 
> 

Re: [PATCH 3/4] cxl: Add late host bridge uport mapping update

[Jonathan Cameron]

On Fri, 4 Apr 2025 15:57:35 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Error message "cxl portN: Couldn't locate the CXL.cache and CXL.mem
> capability array header" is reported through testing when a platform is

Maybe call out this is 'a specific' platform rather than necessarily being
a general thing.

> enabled with PCIe hotplug. The cxl_acpi module is responsible for
> enumerating the host bridges through ACPI objects. During the enumeration
> of the host bridge upstream ports (uports), the root port (RP) registers
> are mapped. The enumeration can happen as soon as the cxl_acpi module
> probe() function is called. However if the CXL link between the endpoint
> device and the RP is not established before the enumeration happens,
> the platform may not expose DVSEC ID 3 and/or DVSEC ID 7 blocks which
> triggers the error message.
> 
> Add an attempt to map the register block under the memdev probe() port
> enumeration. When the PCI probe of the device endpoint is called, the
> driver is now communicating with the CXL device and the CXL link is
> considered established. Doing the register block mapping at that point
> guarantees that the mandatory DVSEC blocks are present.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

It's always challenging to get the balance right but I think it might
be worth a precursor patch to factor out some of the code you then reuse
here so that we can see how that is changed more easily.

> ---
>  drivers/cxl/acpi.c      | 17 +++++++++-
>  drivers/cxl/core/port.c | 72 +++++++++++++++++++++++++++++++++++++++--
>  drivers/cxl/cxl.h       |  4 +++
>  drivers/cxl/port.c      | 19 ++---------
>  4 files changed, 93 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index cb14829bb9be..3c8f04bee9a3 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -662,9 +662,24 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  	if (rc)
>  		return rc;
>  
> -	port = devm_cxl_add_port(host, bridge, component_reg_phys, dport);
> +	/*
> +	 * While there is a chance the uport gets mapped when the probe
> +	 * function gets called, it is not a guarantee due to acpi driver
> +	 * can be probed before the root port has established the CXL
> +	 * connection to the endpoint device. Bypass mapping during
> +	 * port creation by pass in CXL_RESOURCE_NONE for the

by passing in

> +	 * component_reg_phys parameter. After, set the 'resource'
> +	 * parameter of port->map to allow a setup via the endpoint
> +	 * memdev probe.
> +	 */
> +	port = devm_cxl_add_port(host, bridge, CXL_RESOURCE_NONE, dport);
>  	if (IS_ERR(port))
>  		return PTR_ERR(port);
> +	port->reg_map = (struct cxl_register_map) {
> +		.host = host,
> +		.reg_type = CXL_REGLOC_RBI_EMPTY,
> +		.resource = component_reg_phys,
> +	};
>  
>  	dev_info(bridge, "host supports CXL\n");
>  
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 1c772c516dbe..8c29db214d60 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c

> @@ -1565,6 +1571,57 @@ static resource_size_t find_component_registers(struct device *dev)
>  	return map.resource;
>  }
>  
> +int devm_cxl_decoders_setup(struct cxl_port *port)
> +{
> +	struct cxl_dport *dport;
> +	struct cxl_hdm *cxlhdm;
> +	unsigned long index;
> +	int dports = 0;
> +
> +	/* Make sure that no decoders have been allocated before proceeding. */
> +	if (!ida_is_empty(&port->decoder_ida))
> +		return 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_decoders_setup, "CXL");

> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 0e61b76f5c13..b27e9d3306fe 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -906,6 +906,10 @@ 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_decoders_setup(struct cxl_port *port);
> +bool dev_is_cxl_root_child(struct device *dev);
> +int cxl_port_setup_regs(struct cxl_port *port,
> +			resource_size_t component_reg_phys);
>  
>  /*
>   * Unit test builds overrides this to __weak, find the 'strong' version
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index 30c0335089b9..dc532ee9065f 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -59,7 +59,6 @@ static int discover_region(struct device *dev, void *root)
>  
>  static int cxl_switch_port_probe(struct cxl_port *port)
>  {
> -	struct cxl_hdm *cxlhdm;
>  	int rc;
>  
>  	/* Cache the data early to ensure is_visible() works */
> @@ -69,22 +68,10 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>  	if (rc < 0)
>  		return rc;
>  
I'd consider factoring out the bulk of this as a precursor patch so we
can clearly see the functional changes in this one as well as the
reuse.

Maybe pass in the dports for that refactor and switch to the xa_for_each()
in this patch.


> -	cxlhdm = devm_cxl_setup_hdm(port, NULL);
> -	if (!IS_ERR(cxlhdm))
> -		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
> +	if (dev_is_cxl_root_child(&port->dev))
> +		return 0;
>  
> -	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_decoders_setup(port);
>  }
>  
>  static int cxl_endpoint_port_probe(struct cxl_port *port)

Re: [PATCH 3/4] cxl: Add late host bridge uport mapping update

[Dan Williams]

Dave Jiang wrote:
> Error message "cxl portN: Couldn't locate the CXL.cache and CXL.mem
> capability array header" is reported through testing when a platform is
> enabled with PCIe hotplug. The cxl_acpi module is responsible for
> enumerating the host bridges through ACPI objects. During the enumeration
> of the host bridge upstream ports (uports), the root port (RP) registers
> are mapped. The enumeration can happen as soon as the cxl_acpi module
> probe() function is called. However if the CXL link between the endpoint
> device and the RP is not established before the enumeration happens,
> the platform may not expose DVSEC ID 3 and/or DVSEC ID 7 blocks which
> triggers the error message.
> 
> Add an attempt to map the register block under the memdev probe() port
> enumeration. When the PCI probe of the device endpoint is called, the
> driver is now communicating with the CXL device and the CXL link is
> considered established. Doing the register block mapping at that point
> guarantees that the mandatory DVSEC blocks are present.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/acpi.c      | 17 +++++++++-
>  drivers/cxl/core/port.c | 72 +++++++++++++++++++++++++++++++++++++++--
>  drivers/cxl/cxl.h       |  4 +++
>  drivers/cxl/port.c      | 19 ++---------
>  4 files changed, 93 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index cb14829bb9be..3c8f04bee9a3 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -662,9 +662,24 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  	if (rc)
>  		return rc;
>  
> -	port = devm_cxl_add_port(host, bridge, component_reg_phys, dport);
> +	/*
> +	 * While there is a chance the uport gets mapped when the probe
> +	 * function gets called, it is not a guarantee due to acpi driver
> +	 * can be probed before the root port has established the CXL
> +	 * connection to the endpoint device. Bypass mapping during
> +	 * port creation by pass in CXL_RESOURCE_NONE for the
> +	 * component_reg_phys parameter. After, set the 'resource'
> +	 * parameter of port->map to allow a setup via the endpoint
> +	 * memdev probe.
> +	 */
> +	port = devm_cxl_add_port(host, bridge, CXL_RESOURCE_NONE, dport);
>  	if (IS_ERR(port))
>  		return PTR_ERR(port);
> +	port->reg_map = (struct cxl_register_map) {
> +		.host = host,
> +		.reg_type = CXL_REGLOC_RBI_EMPTY,
> +		.resource = component_reg_phys,
> +	};

This looks racy. How do you know the cxl_mem_probe() path is not messing
with the reg_map here?

I feel like the rules about when component_reg_phys is valid to pass to
devm_cxl_add_port() are getting complicated pass the point of
maintainability.

I would be happier if the rule is always "component registers are only
probed and cxl_ports are only created when a memdev is present".

What is currently missing for that is that devm_cxl_enumerate_ports()
does not know how to create the cxl_port instances beneath the cxl_root.
I think the time has come for that. So cxl_acpi only creates the
cxl_root and the host-bridges dports, and devm_cxl_enumerate_ports()
does the rest.

I do not see anything fundamentally difficult about doing one more level
of iteration in devm_cxl_enumerate_ports() to check that a host-bridge
is registered as a dport of the cxl_root and then register a cxl_port
(if not already created).

Re: [PATCH 3/4] cxl: Add late host bridge uport mapping update

[Dave Jiang]

On 4/22/25 11:10 PM, Dan Williams wrote:
> Dave Jiang wrote:
>> Error message "cxl portN: Couldn't locate the CXL.cache and CXL.mem
>> capability array header" is reported through testing when a platform is
>> enabled with PCIe hotplug. The cxl_acpi module is responsible for
>> enumerating the host bridges through ACPI objects. During the enumeration
>> of the host bridge upstream ports (uports), the root port (RP) registers
>> are mapped. The enumeration can happen as soon as the cxl_acpi module
>> probe() function is called. However if the CXL link between the endpoint
>> device and the RP is not established before the enumeration happens,
>> the platform may not expose DVSEC ID 3 and/or DVSEC ID 7 blocks which
>> triggers the error message.
>>
>> Add an attempt to map the register block under the memdev probe() port
>> enumeration. When the PCI probe of the device endpoint is called, the
>> driver is now communicating with the CXL device and the CXL link is
>> considered established. Doing the register block mapping at that point
>> guarantees that the mandatory DVSEC blocks are present.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  drivers/cxl/acpi.c      | 17 +++++++++-
>>  drivers/cxl/core/port.c | 72 +++++++++++++++++++++++++++++++++++++++--
>>  drivers/cxl/cxl.h       |  4 +++
>>  drivers/cxl/port.c      | 19 ++---------
>>  4 files changed, 93 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
>> index cb14829bb9be..3c8f04bee9a3 100644
>> --- a/drivers/cxl/acpi.c
>> +++ b/drivers/cxl/acpi.c
>> @@ -662,9 +662,24 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>>  	if (rc)
>>  		return rc;
>>  
>> -	port = devm_cxl_add_port(host, bridge, component_reg_phys, dport);
>> +	/*
>> +	 * While there is a chance the uport gets mapped when the probe
>> +	 * function gets called, it is not a guarantee due to acpi driver
>> +	 * can be probed before the root port has established the CXL
>> +	 * connection to the endpoint device. Bypass mapping during
>> +	 * port creation by pass in CXL_RESOURCE_NONE for the
>> +	 * component_reg_phys parameter. After, set the 'resource'
>> +	 * parameter of port->map to allow a setup via the endpoint
>> +	 * memdev probe.
>> +	 */
>> +	port = devm_cxl_add_port(host, bridge, CXL_RESOURCE_NONE, dport);
>>  	if (IS_ERR(port))
>>  		return PTR_ERR(port);
>> +	port->reg_map = (struct cxl_register_map) {
>> +		.host = host,
>> +		.reg_type = CXL_REGLOC_RBI_EMPTY,
>> +		.resource = component_reg_phys,
>> +	};
> 
> This looks racy. How do you know the cxl_mem_probe() path is not messing
> with the reg_map here?
> 
> I feel like the rules about when component_reg_phys is valid to pass to
> devm_cxl_add_port() are getting complicated pass the point of
> maintainability.
> 
> I would be happier if the rule is always "component registers are only
> probed and cxl_ports are only created when a memdev is present".
> 
> What is currently missing for that is that devm_cxl_enumerate_ports()
> does not know how to create the cxl_port instances beneath the cxl_root.
> I think the time has come for that. So cxl_acpi only creates the
> cxl_root and the host-bridges dports, and devm_cxl_enumerate_ports()
> does the rest.
> 
> I do not see anything fundamentally difficult about doing one more level
> of iteration in devm_cxl_enumerate_ports() to check that a host-bridge
> is registered as a dport of the cxl_root and then register a cxl_port
> (if not already created).

Essentially move add_host_bridge_uport() to cxl_core? It looks like we'll have to move some ACPI bits to core then in order to retrieve the CHBS. 

[PATCH 4/4] 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.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/hdm.c        | 51 +++++++++++++++++++++++++++++------
 drivers/cxl/cxl.h             | 16 +++++++++++
 tools/testing/cxl/Kbuild      |  3 ---
 tools/testing/cxl/test/mock.c | 34 +++++++++++++++--------
 4 files changed, 82 insertions(+), 22 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 70cae4ebf8a4..ed6bdbd6b452 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,6 +78,16 @@ 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");
+
+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");
 
 static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
@@ -139,12 +154,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,6 +214,16 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
 
 	return cxlhdm;
 }
+EXPORT_SYMBOL_NS_GPL(__devm_cxl_setup_hdm, "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");
 
 static void __cxl_dpa_debug(struct seq_file *file, struct resource *r, int depth)
@@ -1150,12 +1175,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 +1237,14 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 
 	return 0;
 }
+EXPORT_SYMBOL_NS_GPL(__devm_cxl_enumerate_decoders, "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/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index b27e9d3306fe..ffcd6de18e20 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -911,6 +911,22 @@ bool dev_is_cxl_root_child(struct device *dev);
 int cxl_port_setup_regs(struct cxl_port *port,
 			resource_size_t component_reg_phys);
 
+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;
+struct cxl_hdm *__devm_cxl_setup_hdm(struct cxl_port *port,
+				     struct cxl_endpoint_dvsec_info *info);
+
+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;
+int __devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
+				  struct cxl_endpoint_dvsec_info *info);
+
+typedef int (*cxl_add_pt_decoder_fn)(struct cxl_port *port);
+extern cxl_add_pt_decoder_fn _devm_cxl_add_passthrough_decoder;
+int __devm_cxl_add_passthrough_decoder(struct cxl_port *port);
+
 /*
  * Unit test builds overrides this to __weak, find the 'strong' version
  * of these symbols in tools/testing/cxl/.
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index 387f3df8b988..34ea6e6f773b 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
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index af2594e4f35d..f3d97652b9ee 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -13,9 +13,21 @@
 
 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 +35,9 @@ DEFINE_STATIC_SRCU(cxl_mock_srcu);
 
 void unregister_cxl_mock_ops(struct cxl_mock_ops *ops)
 {
+	_devm_cxl_setup_hdm = __devm_cxl_setup_hdm;
+	_devm_cxl_enumerate_decoders = __devm_cxl_enumerate_decoders;
+	_devm_cxl_add_passthrough_decoder = __devm_cxl_add_passthrough_decoder;
 	list_del_rcu(&ops->list);
 	synchronize_srcu(&cxl_mock_srcu);
 }
@@ -131,8 +146,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 +157,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 +171,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 +187,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

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

[Jonathan Cameron]

On Fri, 4 Apr 2025 15:57:36 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> When cxl_core calls a cxl_core exported function and that function is
> mocked by cxl_test, the call chain causes a circular dependency issue. Dan
> provided a workaround to avoid this issue. Apply the method to changes from
> the late host bridge uport mapping update changes in order to enable
> cxl-test.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Perhaps some more breadcrumbs (e.g. comments in the code) so we remember why this
dance is used?

Otherwise I think this is fine though I'm not entirely sure what happens
wrt to races and the  register_cxl_mock_ops() / unregister paths.

> ---
>  drivers/cxl/core/hdm.c        | 51 +++++++++++++++++++++++++++++------
>  drivers/cxl/cxl.h             | 16 +++++++++++
>  tools/testing/cxl/Kbuild      |  3 ---
>  tools/testing/cxl/test/mock.c | 34 +++++++++++++++--------
>  4 files changed, 82 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 70cae4ebf8a4..ed6bdbd6b452 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,6 +78,16 @@ 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");
> +
> +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");

>  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 +35,9 @@ DEFINE_STATIC_SRCU(cxl_mock_srcu);
>  
>  void unregister_cxl_mock_ops(struct cxl_mock_ops *ops)
>  {
> +	_devm_cxl_setup_hdm = __devm_cxl_setup_hdm;
> +	_devm_cxl_enumerate_decoders = __devm_cxl_enumerate_decoders;
> +	_devm_cxl_add_passthrough_decoder = __devm_cxl_add_passthrough_decoder;

I doubt we actually care but should we ever need to reason about this sequence
for some reason, it would be better if it reversed that in register_cxl_mock_ops.

>  	list_del_rcu(&ops->list);
>  	synchronize_srcu(&cxl_mock_srcu);
>  }
> @@ -131,8 +146,8 @@ __wrap_nvdimm_bus_register(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(__wrap_nvdimm_bus_register);

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

[Dan Williams]

Dave Jiang wrote:
> When cxl_core calls a cxl_core exported function and that function is
> mocked by cxl_test, the call chain causes a circular dependency issue. Dan
> provided a workaround to avoid this issue. Apply the method to changes from
> the late host bridge uport mapping update changes in order to enable
> cxl-test.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/hdm.c        | 51 +++++++++++++++++++++++++++++------
>  drivers/cxl/cxl.h             | 16 +++++++++++
>  tools/testing/cxl/Kbuild      |  3 ---
>  tools/testing/cxl/test/mock.c | 34 +++++++++++++++--------
>  4 files changed, 82 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 70cae4ebf8a4..ed6bdbd6b452 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,6 +78,16 @@ 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");
> +
> +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");

I was hoping that this could be more polite and hide most of the the
mess in the common case. Something like this:

#ifdef CXL_TEST_ENABLE
#define DECLARE_TESTABLE(x)
#define x __##x
#else
#define x x
#endif

...i.e. append "__" to all testable functions in the core.

Then move the function pointer declarations, functions that call through
the function pointer, and extra symbol exports to a .c file that only
gets build into the cxl_core by tools/testing/cxl/Kbuild. I.e. like
tools/testing/cxl/cxl_core_exports.c.

That way the core only sees a few extra DECLARE_TESTABLE() lines, and
all the rest of the "magic" (burden) is hidden away in
tools/testing/cxl/.

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

[Li Ming]

On 4/5/2025 6:57 AM, Dave Jiang wrote:
> This series attempts to delay the setup of dports and Host Bridge (HB) register
> probing 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/

Hi Dave,

Thanks for that, you can also mention the patchset is also a long term solution for the issue[2]

[2]: https://lore.kernel.org/linux-cxl/6712b7bf2c1cd_10a03294b3@dwillia2-mobl3.amr.corp.intel.com.notmuch/


After going through the implementation, Setup component registers is triggered on root port and HB when the first endpoint under them attaching, then component registers information are cached. But the information will not be reset after the last endpoint under the root port and HB removed. So the information will be resued when an new endpoint is re-attached to the root port and HB.

I am not sure if component registers information on hardware side is possible to be changed in above case. Just want to know if we need to consider about it?


Ming

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

[Dave Jiang]

On 4/10/25 8:05 PM, Li Ming wrote:
> On 4/5/2025 6:57 AM, Dave Jiang wrote:
>> This series attempts to delay the setup of dports and Host Bridge (HB) register
>> probing 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/
> 
> Hi Dave,
> 
> Thanks for that, you can also mention the patchset is also a long term solution for the issue[2]
> 
> [2]: https://lore.kernel.org/linux-cxl/6712b7bf2c1cd_10a03294b3@dwillia2-mobl3.amr.corp.intel.com.notmuch/
> 
> 
> After going through the implementation, Setup component registers is triggered on root port and HB when the first endpoint under them attaching, then component registers information are cached. But the information will not be reset after the last endpoint under the root port and HB removed. So the information will be resued when an new endpoint is re-attached to the root port and HB.
> 
> I am not sure if component registers information on hardware side is possible to be changed in above case. Just want to know if we need to consider about it?

I do not know either and I don't have the hotplug hardware to see. Unless someone can report the behavior, it may be one of those we cross that bridge when we get there situation.

> 
> 
> Ming
>