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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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