Re: [PATCH v5 05/14] cxl/region: Rename function to cxl_port_pick_region_decoder()

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

On Tue, 29 Apr 2025 16:31:19 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Mon, 28 Apr 2025 23:43:08 +0200
> Robert Richter <rrichter@amd.com> wrote:
> 
> > Current function cxl_region_find_decoder() is used to find a port's
> > decoder during region setup. In the region creation path the function
> > is an allocator to find a free port. In the region assembly path, it
> > is recalling the decoder that platform firmware picked for validation
> > purposes.
> > 
> > Rename function to cxl_port_pick_region_decoder() that better
> > describes its use and update the function's description.
> > 
> > The result of cxl_port_pick_region_decoder() is recorded in a 'struct
> > cxl_region_ref' in @port for later recall when other endpoints might
> > also be targets of the picked decoder.  
> 
> I'm not convinced pick is really any clearer than find as it doesn't to me
> imply 'get the one that was already allocated'.  I'm also not seeing
> a lot of precedence in kernel for this use of pick.
> 
> I don't feel that strongly either way though and I guess I'll
> get used to the term if we go with pick.
> 
> Alternative might just be to make it an or...
> 
> cxl_region_find_or_alloc_decoder()
> 
Just taking a look at where this series stands and feel a clarification
is needed.

Note I don't care enough on this to block the series going forwards!

> 
> > 
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > Reviewed-by: Gregory Price <gourry@gourry.net>
> > Tested-by: Gregory Price <gourry@gourry.net>
> > ---
> >  drivers/cxl/core/region.c | 25 ++++++++++++++++++++-----
> >  1 file changed, 20 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index e35209168c9c..e104035e0855 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -865,10 +865,25 @@ static int match_auto_decoder(struct device *dev, const void *data)
> >  	return 0;
> >  }
> >  
> > +/**
> > + * cxl_port_pick_region_decoder() - assign or lookup a decoder for a region
> > + * @port: a port in the ancestry of the endpoint implied by @cxled
> > + * @cxled: endpoint decoder to be, or currently, mapped by @port
> > + * @cxlr: region to establish, or validate, decode @port
> > + *
> > + * In the region creation path cxl_port_pick_region_decoder() is an
> > + * allocator to find a free port. In the region assembly path, it is
> > + * recalling the decoder that platform firmware picked for validation
> > + * purposes.
> > + *
> > + * The result is recorded in a 'struct cxl_region_ref' in @port for
> > + * later recall when other endpoints might also be targets of the picked
> > + * decoder.
> > + */
> >  static struct cxl_decoder *
> > -cxl_region_find_decoder(struct cxl_port *port,
> > -			struct cxl_endpoint_decoder *cxled,
> > -			struct cxl_region *cxlr)
> > +cxl_port_pick_region_decoder(struct cxl_port *port,
> > +			     struct cxl_endpoint_decoder *cxled,
> > +			     struct cxl_region *cxlr)
> >  {
> >  	struct device *dev;
> >  
> > @@ -932,7 +947,7 @@ alloc_region_ref(struct cxl_port *port, struct cxl_region *cxlr,
> >  		if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> >  			struct cxl_decoder *cxld;
> >  
> > -			cxld = cxl_region_find_decoder(port, cxled, cxlr);
> > +			cxld = cxl_port_pick_region_decoder(port, cxled, cxlr);
> >  			if (auto_order_ok(port, iter->region, cxld))
> >  				continue;
> >  		}
> > @@ -1020,7 +1035,7 @@ static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
> >  {
> >  	struct cxl_decoder *cxld;
> >  
> > -	cxld = cxl_region_find_decoder(port, cxled, cxlr);
> > +	cxld = cxl_port_pick_region_decoder(port, cxled, cxlr);
> >  	if (!cxld) {
> >  		dev_dbg(&cxlr->dev, "%s: no decoder available\n",
> >  			dev_name(&port->dev));  
> 
>