Re: [PATCH 3/9] cxl/port: Cleanup dport removal with a devres group

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

On Wed, 21 Jan 2026 19:33:24 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> In preparation for adding more setup actions like RAS register mapping,
> introduce a devres group to collect all the dport creation / registration
> actions. This replaces the maintenance tedium of open coding several
> devm_release_action() calls in del_dport().
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Whilst nice, there is some logic buried deep enough that it might surprise
anyone trying to grasp flow in __devm_cxl_add_dport.

I like the cleanup.h stuff but here I'm wondering if it is appropriate.
Maybe just use a goto in __devm_cxl_add_dport()

Minimum I think is we need a breadcrumb in the function naming that
it has anything to do with devres.  _dr_ is used in a few places where
devres naming is too long. 

Otherwise one minor thing inline around reusing del_dport in
the DEFINE_FREE()
If we can't because the diverge later, maybe add a comment (or just
ignore this feedback).


> ---
>  drivers/cxl/core/port.c | 62 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 56 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 1637e97f6805..f2723bf948e2 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1118,6 +1118,51 @@ static void cxl_dport_unlink(void *data)
>  	sysfs_remove_link(&port->dev.kobj, link_name);
>  }
>  
> +static struct device *dport_to_host(struct cxl_dport *dport)
> +{
> +	if (is_cxl_root(dport->port))
> +		return dport->port->uport_dev;
> +	return &dport->port->dev;
> +}
> +
> +static void free_dport(void *dport)
> +{
> +	kfree(dport);
> +}
> +
> +/*
> + * Upon return either a group is established with one action (free_dport()), or
> + * no group established and @dport is freed.
> + */
> +static void *cxl_dport_open_group_or_free(struct cxl_dport *dport)

Can we put something in the name to hint this is devres stuff?
Group could mean too many things :( Even
cxl_dport_open_dr_group_or_free() avoids sounding too generic.


> +{
> +	int rc;
> +	struct device *host = dport_to_host(dport);
> +	void *group = devres_open_group(host, dport, GFP_KERNEL);
> +
> +	if (!group) {
> +		kfree(dport);
> +		return NULL;
> +	}
> +
> +	rc = devm_add_action_or_reset(host, free_dport, dport);
> +	if (rc) {
> +		devres_release_group(host, group);
> +		return NULL;
> +	}
> +
> +	return group;
> +}
> +
> +static void cxl_dport_close_group(struct cxl_dport *dport, void *group)
> +{
> +	devres_close_group(dport_to_host(dport), group);
> +}
> +
> +/* The dport group id is the dport */
> +DEFINE_FREE(cxl_dport_release_group, void *,
> +	    if (_T) devres_release_group(dport_to_host(_T), _T))

Reorder so this can use the typed del_dport()?

> +
>  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,
> @@ -1143,14 +1188,20 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>  	    CXL_TARGET_STRLEN)
>  		return ERR_PTR(-EINVAL);
>  
> -	dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
> +	dport = kzalloc(sizeof(*dport), GFP_KERNEL);
>  	if (!dport)
>  		return ERR_PTR(-ENOMEM);
>  
> +	/* Just enough init to manage the devres group */
>  	dport->dport_dev = dport_dev;
>  	dport->port_id = port_id;
>  	dport->port = port;
>  
> +	void *dport_group __free(cxl_dport_release_group) =
> +		cxl_dport_open_group_or_free(dport);
> +	if (!dport_group)
> +		return ERR_PTR(-ENOMEM);
> +
>  	if (rcrb == CXL_RESOURCE_NONE) {
>  		rc = cxl_dport_setup_regs(&port->dev, dport,
>  					  component_reg_phys);
> @@ -1203,6 +1254,9 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>  
>  	cxl_debugfs_create_dport_dir(dport);
>  
> +	/* keep the group, and mark the end of devm actions */
> +	cxl_dport_close_group(dport, no_free_ptr(dport_group));
> +
>  	return dport;
>  }
>  
> @@ -1431,11 +1485,7 @@ static void delete_switch_port(struct cxl_port *port)
>  
>  static void del_dport(struct cxl_dport *dport)
>  {
> -	struct cxl_port *port = dport->port;
> -
> -	devm_release_action(&port->dev, cxl_dport_unlink, dport);
> -	devm_release_action(&port->dev, cxl_dport_remove, dport);
> -	devm_kfree(&port->dev, dport);
> +	devres_release_group(dport_to_host(dport), dport);
>  }
>  
>  static void del_dports(struct cxl_port *port)