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

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

On Fri, 23 Jan 2026 12:14:41 +0000
Jonathan Cameron <jonathan.cameron@huawei.com> wrote:

> 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.
Ah. That discussion was in the next patch.  Temporal slip ;)

Anyhow, it was a bit of musing rather than a strong request to not use
cleanup stuff here.

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