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

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

Jonathan Cameron 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)

It is a fair point, but not sure how to square it outside of the comment
in the one place that needs the alias. These things are separate, but
for the convenience of DEFINE_FREE(), aliasing the pointer saves a bunch
of other boilerplate.

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

This group is the one that needs a long lived ID. The port group is
temporary. It was not immediately clear to me that NULL could be used
for that ID given the need to also create the dport group, but that is a
potential follow-on cleanup.