Re: [PATCH v2 02/10] cxl: Saperate out CXL dport->id vs actual dport hardware id

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

On 5/8/25 5:51 PM, Li Ming wrote:
> On 5/7/2025 8:43 AM, Dave Jiang wrote:
>> In preparation to allow dport to be allocated without being active, make
>> dport->id to be Linux id that enumerates the dport objects per port.
>> Keep the hardware id under dport->port_num to maintain compatibility and
>> introduce a dport->id as the enumeration id.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> v2:
>> - Rename port_id to port_num. (Dan)
>> - Use a dport allocator to deal with ida allocation. (Dan, Jonathan)
>> ---
>>  drivers/cxl/core/cdat.c |  2 +-
>>  drivers/cxl/core/hdm.c  | 18 +++++-----
>>  drivers/cxl/core/port.c | 73 ++++++++++++++++++++++++++++++-----------
>>  drivers/cxl/cxl.h       | 12 ++++---
>>  4 files changed, 71 insertions(+), 34 deletions(-)
>>
> [snip]
>>  static struct cxl_dport *
>>  __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>> -		     int port_id, resource_size_t component_reg_phys,
>> +		     int port_num, resource_size_t component_reg_phys,
>>  		     resource_size_t rcrb)
>>  {
>>  	char link_name[CXL_TARGET_STRLEN];
>> @@ -1139,17 +1170,19 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  		return ERR_PTR(-ENXIO);
>>  	}
>>  
>> -	if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", port_id) >=
>> +	dport = cxl_alloc_dport(port, dport_dev);
>> +	if (!dport)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	rc = devm_add_action_or_reset(&port->dev, free_dport, dport);
>> +	if (rc)
>> +		return ERR_PTR(rc);
>> +
>> +	if (snprintf(link_name, CXL_TARGET_STRLEN, "dport%d", dport->id) >=
>>  	    CXL_TARGET_STRLEN)
>>  		return ERR_PTR(-EINVAL);
> Is it better to call devm_release_action(&port->dev, free_dport, dport) here if snprintf() failed?

Not sure I follow why this error path is different than the others and needs release_action().

> 
>>  
>> -	dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
>> -	if (!dport)
>> -		return ERR_PTR(-ENOMEM);
>> -
>> -	dport->dport_dev = dport_dev;
>> -	dport->port_id = port_id;
>> -	dport->port = port;
>> +	dport->port_num = port_num;
>>  
>>  	if (rcrb == CXL_RESOURCE_NONE) {
>>  		rc = cxl_dport_setup_regs(&port->dev, dport,
>> @@ -1211,7 +1244,7 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>   * devm_cxl_add_dport - append VH downstream port data to a cxl_port
>>   * @port: the cxl_port that references this dport
>>   * @dport_dev: firmware or PCI device representing the dport
>> - * @port_id: identifier for this dport in a decoder's target list
>> + * @port_num: identifier for this dport in a decoder's target list
>>   * @component_reg_phys: optional location of CXL component registers
>>   *
>>   * Note that dports are appended to the devm release action's of the
>> @@ -1219,12 +1252,12 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>   * switch ports)
>>   */
>>  struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
>> -				     struct device *dport_dev, int port_id,
>> +				     struct device *dport_dev, int port_num,
>>  				     resource_size_t component_reg_phys)
>>  {
>>  	struct cxl_dport *dport;
>>  
>> -	dport = __devm_cxl_add_dport(port, dport_dev, port_id,
>> +	dport = __devm_cxl_add_dport(port, dport_dev, port_num,
>>  				     component_reg_phys, CXL_RESOURCE_NONE);
>>  	if (IS_ERR(dport)) {
>>  		dev_dbg(dport_dev, "failed to add dport to %s: %ld\n",
>> @@ -1455,7 +1488,7 @@ static void reap_dports(struct cxl_port *port)
>>  	xa_for_each(&port->dports, index, dport) {
>>  		devm_release_action(&port->dev, cxl_dport_unlink, dport);
>>  		devm_release_action(&port->dev, cxl_dport_remove, dport);
>> -		devm_kfree(&port->dev, dport);
>> +		devm_release_action(&port->dev, free_dport, dport);
>>  	}
>>  }
>>  
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index a9ab46eb0610..f4fe523aaf12 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -583,6 +583,7 @@ struct cxl_dax_region {
>>   * @regions: cxl_region_ref instances, regions mapped by this port
>>   * @parent_dport: dport that points to this port in the parent
>>   * @decoder_ida: allocator for decoder ids
>> + * @dport_ida: allocator for dport ids
>>   * @reg_map: component and ras register mapping parameters
>>   * @nr_dports: number of entries in @dports
>>   * @hdm_end: track last allocated HDM decoder instance for allocation ordering
>> @@ -603,6 +604,7 @@ struct cxl_port {
>>  	struct xarray regions;
>>  	struct cxl_dport *parent_dport;
>>  	struct ida decoder_ida;
>> +	struct ida dport_ida;
>>  	struct cxl_register_map reg_map;
>>  	int nr_dports;
>>  	int hdm_end;
>> @@ -655,7 +657,8 @@ struct cxl_rcrb_info {
>>   * struct cxl_dport - CXL downstream port
>>   * @dport_dev: PCI bridge or firmware device representing the downstream link
>>   * @reg_map: component and ras register mapping parameters
>> - * @port_id: unique hardware identifier for dport in decoder target list
>> + * @id: Linux id to enumerate dport instances per port
>> + * @port_num: unique hardware identifier for dport in decoder target list
>>   * @rcrb: Data about the Root Complex Register Block layout
>>   * @rch: Indicate whether this dport was enumerated in RCH or VH mode
>>   * @port: reference to cxl_port that contains this downstream port
>> @@ -667,7 +670,8 @@ struct cxl_rcrb_info {
>>  struct cxl_dport {
>>  	struct device *dport_dev;
>>  	struct cxl_register_map reg_map;
>> -	int port_id;
>> +	int id;
>> +	int port_num;
>>  	struct cxl_rcrb_info rcrb;
>>  	bool rch;
>>  	struct cxl_port *port;
>> @@ -750,10 +754,10 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
>>  bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);
>>  
>>  struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
>> -				     struct device *dport, int port_id,
>> +				     struct device *dport, int port_num,
>>  				     resource_size_t component_reg_phys);
>>  struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>> -					 struct device *dport_dev, int port_id,
>> +					 struct device *dport_dev, int port_num,
>>  					 resource_size_t rcrb);
>>  
>>  #ifdef CONFIG_PCIEAER_CXL
> 
> 
>