Re: [PATCH 4/9] cxl/port: Move decoder setup before dport creation

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

Jonathan Cameron wrote:
> On Wed, 21 Jan 2026 19:33:25 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > There are port setup actions that run on first dport arrival, and there are
> > setup actions that run per dport.
> > 
> > RAS register setup is a future additional setup action to run per-port
> > (once the first dport arrives), and each dport also has RAS registers to
> > map.
> > 
> > Before adding that, flip the order of "first dport" and "per-dport"
> > actions. This makes allocation symmetric with teardown, "first dport"
> > actions unwind after last dport removed. It also allows for using a devres
> > group to collect the unrelated decoder, RAS, and dport setup actions into
> > one group release action.
> > 
> > The new cxl_port_open_group() collects "first dport" and "per-dport" into
> > one group that can be released on any failure. This group's lifetime only
> > needs to span the short duration of cxl_port_add_dport() to cleanup all
> > potential damage from failing to add a dport. Contrast that to the "dport"
> > devres group that is called upon to destruct fully formed dport objects.
> > 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> Trivial stuff only.  Took me a while to get my head around the temporary
> group usage, but having done so it seems correct to me.  I poked the
> various paths fairly heavily to be sure they all worked out after
> thinking there was a bug due to a misread :(
> 
> Either way on suggestions below.
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> > ---
> >  drivers/cxl/core/port.c | 43 +++++++++++++++++++++++++++++------------
> >  1 file changed, 31 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index f2723bf948e2..f69395ea0c14 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -1650,10 +1650,24 @@ static bool dport_exists(struct cxl_port *port, struct device *dport_dev)
> >  	return false;
> >  }
> >  
> > -DEFINE_FREE(del_cxl_dport, struct cxl_dport *, if (!IS_ERR_OR_NULL(_T)) del_dport(_T))
> > +static void *cxl_port_open_group(struct cxl_port *port)
> > +{
> > +	return devres_open_group(&port->dev, port, GFP_KERNEL);
> So only reason you are using port as the ID is so there is just one thing
> to pass to the DEFINE_FREE() callback. Fair enough, but...

Right, and it avoids needing to wade through the cleverness of defining
a new CLASS() with a 'fat' pointer that contains the devm host and the
group. I.e. similar to DEFINE_LOCK_GUARD_0.

> > +}
> > +
> > +/* note this implicitly casts @port_group back to its @port */
> > +DEFINE_FREE(cxl_port_release_group, struct cxl_port *,
> > +	    if (_T) devres_release_group(&_T->dev, _T))
> > +
> > +static void cxl_port_remove_group(struct cxl_port *port, void *port_group)
> > +{
> > +	devres_remove_group(&port->dev, port_group);
> 
> To keep this inline with the DEFINE_FREE(), I'd pass in only one parameter.
> Can in theory be either of them but to me port_group is more
> consistent. Then cast that to get the struct cxl_port *

I would sooner go the other way and skip the open/remove wrappers
altogether. Because it just adds to the confusion of what is happening.
The DEFINE_FREE() needs a comment for its cleverness, but unlike the
dport case this port group can just be idiomatic
devres_{open,remove}_group().

I will do the "dr_group" rename of the local pointer though which also
fits better with not wrapping devres_{open,remove}_group() for this case.