linux-kernel - Re: [PATCH v1 08/29] cxl/region: Split region registration into an initialization and adding part

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: <7a1a8340-a393-4781-957b-9da026b03bb5@zohomail.com>
Date: Thu, 9 Jan 2025 09:08:32 +0800
From: Li Ming <ming.li@...omail.com>
To: Robert Richter <rrichter@....com>,
 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>,
 Jonathan Cameron <jonathan.cameron@...wei.com>,
 Dave Jiang <dave.jiang@...el.com>, Davidlohr Bueso <dave@...olabs.net>
Cc: 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 v1 08/29] cxl/region: Split region registration into an
 initialization and adding part

On 1/7/2025 10:09 PM, Robert Richter wrote:
> Before adding an endpoint to a region, the endpoint is initialized
> first. Move that part to a new function cxl_endpoint_initialize().
> The function is in preparation of adding more parameters that need to
> be determined in a setup.
>
> The split also helps better separating the code. After initialization
> the addition of an endpoint may fail with an error code and all the
> data would need to be reverted to not leave the endpoint in an
> undefined state. With separate functions the init part can succeed
> even if the endpoint cannot be added.
>
> Function naming follows the style of device_register() etc. Thus,
> rename function cxl_add_to_region() to cxl_endpoint_register().
>
> Signed-off-by: Robert Richter <rrichter@....com>
> ---
>  drivers/cxl/core/region.c | 36 ++++++++++++++++++++++++++++--------
>  drivers/cxl/cxl.h         |  5 +++--
>  drivers/cxl/port.c        |  9 +++++----
>  3 files changed, 36 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index d5dcc94df0a5..5132c689b1f2 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3340,7 +3340,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>  		dev_name(&cxlr->dev), p->res, p->interleave_ways,
>  		p->interleave_granularity);
>  
> -	/* ...to match put_device() in cxl_add_to_region() */
> +	/* ...to match put_device() in cxl_endpoint_add() */
>  	get_device(&cxlr->dev);
>  	up_write(&cxl_region_rwsem);
>  
> @@ -3352,19 +3352,28 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>  	return ERR_PTR(rc);
>  }
>  
> -int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
> +static int cxl_endpoint_initialize(struct cxl_endpoint_decoder *cxled)
>  {
> -	struct range *hpa = &cxled->cxld.hpa_range;
>  	struct cxl_root_decoder *cxlrd;
> -	struct cxl_region_params *p;
> -	struct cxl_region *cxlr;
> -	bool attach = false;
> -	int rc;
>  
>  	cxlrd = cxl_find_root_decoder(cxled);
>  	if (!cxlrd)
>  		return -ENXIO;
>  
> +	cxled->cxlrd = cxlrd;
> +
> +	return 0;
> +}
> +
> +static int cxl_endpoint_add(struct cxl_endpoint_decoder *cxled)
> +{
> +	struct range *hpa = &cxled->cxld.hpa_range;
> +	struct cxl_root_decoder *cxlrd = cxled->cxlrd;
> +	struct cxl_region_params *p;
> +	struct cxl_region *cxlr;
> +	bool attach = false;
> +	int rc;
> +
>  	/*
>  	 * Ensure that if multiple threads race to construct_region() for @hpa
>  	 * one does the construction and the others add to that.
> @@ -3401,7 +3410,18 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
>  
>  	return rc;
>  }
> -EXPORT_SYMBOL_NS_GPL(cxl_add_to_region, "CXL");
> +
> +int cxl_endpoint_register(struct cxl_endpoint_decoder *cxled)
> +{
> +	int rc;
> +
> +	rc = cxl_endpoint_initialize(cxled);
> +	if (rc)
> +		return rc;
> +
> +	return cxl_endpoint_add(cxled);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_endpoint_register, "CXL");

Hi Robert,


cxl_endpoint_initialize(), cxl_endpoint_add(), cxl_endpoint_register() feels like some functions related to an endpoint, but I think they are for an endpoint decoder enabling, maybe rename them to cxl_endpoint_decoder_initialize()/add()/register()?


Ming