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

[Dave Jiang <dave.jiang@intel.com>]

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.