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

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

On Thu, 22 Jan 2026 12:43:36 -0800
dan.j.williams@intel.com wrote:

> 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.

I'm an enthusiast for the cleanup.h stuff. This was very much specific
to this case. I thought I wrote more on this in original mail, but seems
I deleted the comments before sending!  Sorry about that.

Main thing I was a bit dubious about in this very specific case was about
overlapping semantic meaning of the group and the the dport (which are
the same address, but we only pretend that in some paths).

That is necessary so there is 'one' thing for:

DEFINE_FREE(cxl_dport_release_group, void *,
	    if (_T) devres_release_group(dport_to_host(_T), _T))

Which is fine but then the meaning is broken out in
static void cxl_dport_close_group(struct cxl_dport *dport, void *group)

+ I'd have preferred we were explicit in the group being temporary and
hence passed NULL as ID which we can't do if group and dport need
to be the same pointer.

That in combination made me think perhaps it wasn't worth applying here.

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