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

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

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()


> 
> 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));