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

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

Jonathan Cameron wrote:
> 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()
> 

It is several gotos, I have a hard time ever writing goto again.

Maybe if you can clarify your "inappropriate" feeling. To be clear I
have heard this from other maintainers that are not ready to let go of
goto, but I feel this is rapidly approaching the reverse-xmas-tree level
of local maintainer preferences.

[..]
> > + * 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.

I was on the fence with making it more clear it was devres, was just
waiting for a tie breaking shove. Shove received, "dr_group" it is.

> > +{
> > +	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()?

Yeah, that is cleaner.