[PATCH v1 05/20] cxl/region: Separate cxl_region_alloc() from devm_cxl_add_region()

Robert Richter posted 20 patches 2 months, 3 weeks ago
[PATCH v1 05/20] cxl/region: Separate cxl_region_alloc() from devm_cxl_add_region()
Posted by Robert Richter 2 months, 3 weeks ago
In interleaving configs multiple endpoint decoders connect to the same
region. The region's parameters must be the same for all endpoint
decoders that share the interleaving setup. During initialization, the
region's parameters are determined for each endpoint decoder.
If a region for the same hpa range already exists, no new region is
created and the existing one is reused.

To simplify region setup and the collection of the region parameters,
separate region allocation from its registration. This allows it to
allocate and setup a region before checking the parameters with
existing other regions and adding it to the cxl tree or releasing it
and instead reusing an existing region.

Here, only separate cxl_region_alloc() from devm_cxl_add_region().

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/core/region.c | 41 ++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 8e2521c6c845..cfcd286251d5 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2525,34 +2525,26 @@ static int register_region(struct cxl_region *cxlr, int id)
 }
 
 /**
- * devm_cxl_add_region - Adds a region to a decoder
- * @cxlrd: root decoder
- * @id: memregion id to create, or memregion_free() on failure
- * @mode: mode for the endpoint decoders of this region
- * @type: select whether this is an expander or accelerator (type-2 or type-3)
+ * devm_cxl_add_region - Adds a region to the CXL hierarchy.
+ * @cxlr: region to be added
+ * @id: memregion id to create must match current @port_id of the
+ *      region's @cxlrd
  *
  * This is the second step of region initialization. Regions exist within an
  * address space which is mapped by a @cxlrd.
  *
- * Return: 0 if the region was added to the @cxlrd, else returns negative error
- * code. The region will be named "regionZ" where Z is the unique region number.
+ * Return: Pointer to the region if the region could be registered
+ * (for use in a tail call). The region will be named "regionZ" where
+ * Z is the unique region number. On errors, devm_cxl_add_region()
+ * returns an encoded negative error code and releases or unregisters
+ * @cxlr.
  */
-static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
-					      int id,
-					      enum cxl_partition_mode mode,
-					      enum cxl_decoder_type type)
+static struct cxl_region *devm_cxl_add_region(struct cxl_region *cxlr, int id)
 {
+	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
 	struct cxl_port *port = to_cxl_port(cxlrd->cxlsd.cxld.dev.parent);
-	struct cxl_region *cxlr;
 	int rc;
 
-	cxlr = cxl_region_alloc(cxlrd);
-	if (IS_ERR(cxlr))
-		return cxlr;
-
-	cxlr->mode = mode;
-	cxlr->type = type;
-
 	rc = register_region(cxlr, id);
 	if (rc) {
 		put_device(&cxlr->dev);
@@ -2589,6 +2581,8 @@ static ssize_t create_ram_region_show(struct device *dev,
 static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd,
 					  enum cxl_partition_mode mode, int id)
 {
+	struct cxl_region *cxlr;
+
 	switch (mode) {
 	case CXL_PARTMODE_RAM:
 	case CXL_PARTMODE_PMEM:
@@ -2598,7 +2592,14 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd,
 		return ERR_PTR(-EINVAL);
 	}
 
-	return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_HOSTONLYMEM);
+	cxlr = cxl_region_alloc(cxlrd);
+	if (IS_ERR(cxlr))
+		return cxlr;
+
+	cxlr->mode = mode;
+	cxlr->type = CXL_DECODER_HOSTONLYMEM;
+
+	return devm_cxl_add_region(cxlr, id);
 }
 
 static ssize_t create_region_store(struct device *dev, const char *buf,
-- 
2.39.5
Re: [PATCH v1 05/20] cxl/region: Separate cxl_region_alloc() from devm_cxl_add_region()
Posted by Joshua Hahn 2 months, 2 weeks ago
On Tue, 15 Jul 2025 21:11:28 +0200 Robert Richter <rrichter@amd.com> wrote:

> In interleaving configs multiple endpoint decoders connect to the same
> region. The region's parameters must be the same for all endpoint
> decoders that share the interleaving setup. During initialization, the
> region's parameters are determined for each endpoint decoder.
> If a region for the same hpa range already exists, no new region is
> created and the existing one is reused.
> 
> To simplify region setup and the collection of the region parameters,
> separate region allocation from its registration. This allows it to
> allocate and setup a region before checking the parameters with
> existing other regions and adding it to the cxl tree or releasing it
> and instead reusing an existing region.
> 
> Here, only separate cxl_region_alloc() from devm_cxl_add_region().
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---

Hello Robert,

Thank you for this great patch! I have one small nit:

>  /**
> - * devm_cxl_add_region - Adds a region to a decoder
> - * @cxlrd: root decoder
> - * @id: memregion id to create, or memregion_free() on failure
> - * @mode: mode for the endpoint decoders of this region
> - * @type: select whether this is an expander or accelerator (type-2 or type-3)
> + * devm_cxl_add_region - Adds a region to the CXL hierarchy.
> + * @cxlr: region to be added
> + * @id: memregion id to create must match current @port_id of the
> + *      region's @cxlrd
>   *
>   * This is the second step of region initialization. Regions exist within an
>   * address space which is mapped by a @cxlrd.
>   *
> - * Return: 0 if the region was added to the @cxlrd, else returns negative error
> - * code. The region will be named "regionZ" where Z is the unique region number.
> + * Return: Pointer to the region if the region could be registered
> + * (for use in a tail call). The region will be named "regionZ" where
> + * Z is the unique region number. On errors, devm_cxl_add_region()
> + * returns an encoded negative error code and releases or unregisters
> + * @cxlr.
>   */

It seems like the changes that this new return description corresponds to
were actually made in the previous patch. Would it make sense to move this
new description to the previous patch?

> -static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
> -					      int id,
> -					      enum cxl_partition_mode mode,
> -					      enum cxl_decoder_type type)
> +static struct cxl_region *devm_cxl_add_region(struct cxl_region *cxlr, int id)

Otherwise, LGTM!

Reviewed-by: Joshua Hahn <joshua.hahnjy@gmail.com>

Sent using hkml (https://github.com/sjp38/hackermail)