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

[Dan Williams <dan.j.williams@intel.com>]

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;