linux-kernel - Re: [PATCH v5 05/14] cxl/region: Rename function to cxl_port_pick_region

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250429163119.00001eba@huawei.com>
Date: Tue, 29 Apr 2025 16:31:19 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Robert Richter <rrichter@....com>
CC: Alison Schofield <alison.schofield@...el.com>, Vishal Verma
	<vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>, Dan Williams
	<dan.j.williams@...el.com>, Dave Jiang <dave.jiang@...el.com>, "Davidlohr
 Bueso" <dave@...olabs.net>, <linux-cxl@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, Gregory Price <gourry@...rry.net>, "Fabio M.
 De Francesco" <fabio.m.de.francesco@...ux.intel.com>, Terry Bowman
	<terry.bowman@....com>
Subject: Re: [PATCH v5 05/14] cxl/region: Rename function to
 cxl_port_pick_region_decoder()

On Mon, 28 Apr 2025 23:43:08 +0200
Robert Richter <rrichter@....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@....com>
> Reviewed-by: Gregory Price <gourry@...rry.net>
> Tested-by: Gregory Price <gourry@...rry.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));