[PATCH v1 00/20] cxl: Address translation support, part 2: Region code rework

[PATCH v1 00/20] cxl: Address translation support, part 2: Region code rework

[Robert Richter]

This series is the second part of adding support for CXL address
translation. It adds another rework of region code to address
implementation changes or conflicts of current address translation
code with cxl/next, esp. the introduction of support of extended
linear caching.

Following parts are currently planned, worked on or finished:

Part 1: Cleanups and refactoring
Upstream: 68d8b4f399e7 ("Merge branch 'for-6.16/cxl-cleanups' into cxl-for-next")

Part 2: Region code rework
This initial patch series.

Part 3: Extended linear cache rework
Not yet posted.

Part 4: Generic support and AMD Zen5 platform enablement.
Not yet posted. (Earlier version posted as part 2, v2: Generic support
and AMD Zen5 platform enablement. [1])

The general changes in the implementation compared to [1] are in
particular to use the attached region of an endpoint decoder to host
the HPA range and interleaving configuration parameters. That is, the
region's root decoder and HPA range are added as members @cxlrd and
@hpa_range to struct cxl_region. Both are introduced to keep track of
the region's SPA address range and the interleaving configuration.
Those parameters are the same for all endpoint decoders that share the
same interleaving setup.

The implementation must ensure that the endpoint decoder's region
parameters are always valid. All parameters must be determined first
and then a check must be performed if a region with identical
parameters already exists. A split of region creation and registration
is required as the region may not become active and may need to be
replaced by an already existing region. Several high-level functions
are introduced (create_region(), setup_region(), register_region(),
cxl_endpoint_get_region(), cxl_region_find_duplicate()). Most of it is
implemented in cxl_add_to_region().

Finally, this series adds a lot of simplification and improves error
handling and code readability.

[1] https://lore.kernel.org/all/20250218132356.1809075-1-rrichter@amd.com/

Robert Richter (20):
  cxl/region: Move helper functions closer to their users
  cxl/region: Store root decoder in struct cxl_region
  cxl/region: Remove region id handling from cxl_region_alloc()
  cxl/region: Add region registration code to new function
    register_region()
  cxl/region: Separate cxl_region_alloc() from devm_cxl_add_region()
  cxl/region: Remove dev_err() from cxl_find_root_decoder()
  cxl/region: Add new function cxl_endpoint_get_region() to simplify
    cxl_add_to_region()
  cxl/region: Rework memregion id allocation and move it to
    register_region()
  cxl/region: Change __construct_region() to use it as a tail function
    call
  cxl/region: Remove __construct_region()
  cxl/region: Separate auto-generated region cration code path
  cxl/region: Remove region creation code from construct_region()
  cxl/region: Move devm_cxl_add_region() out of create_region()
  cxl/region: Prepare removal of @cxlrd argument from create_region()
  cxl/region: Prepare removal of @cxled argument from construct_region()
  cxl/region: Introduce @hpa_range to struct cxl_region
  cxl/region: Remove create_region() call from construct_region()
  cxl/region: Determine root decoder in create_region()
  cxl/region: Add function to find a region's duplicate
  cxl/region: Early check region's interleaving configuration

 drivers/cxl/core/region.c | 514 +++++++++++++++++++++++---------------
 drivers/cxl/cxl.h         |   4 +
 2 files changed, 315 insertions(+), 203 deletions(-)


base-commit: 12b3d697c812aaf356e82d9e1f351fbb2ea97500
-- 
2.39.5

[PATCH v1 01/20] cxl/region: Move helper functions closer to their users

[Robert Richter]

Just moving code around without any changes. Move helper functions
closer to their users. This simplifies code and a rework and reduces
the diffstat of follow-on patches.

In particular, do the following:

Place cxl_region_alloc() and unregister_region() before
devm_cxl_add_region(). Have cxl_region_alloc() first as it is used in
devm_cxl_add_region() first.

Group match_region_by_range() and cxl_find_region_by_range() as both
belong together.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 08ac7f483562..f22ad20b0db9 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2365,56 +2365,6 @@ static struct cxl_region *to_cxl_region(struct device *dev)
 	return container_of(dev, struct cxl_region, dev);
 }
 
-static void unregister_region(void *_cxlr)
-{
-	struct cxl_region *cxlr = _cxlr;
-	struct cxl_region_params *p = &cxlr->params;
-	int i;
-
-	device_del(&cxlr->dev);
-
-	/*
-	 * Now that region sysfs is shutdown, the parameter block is now
-	 * read-only, so no need to hold the region rwsem to access the
-	 * region parameters.
-	 */
-	for (i = 0; i < p->interleave_ways; i++)
-		detach_target(cxlr, i);
-
-	cxl_region_iomem_release(cxlr);
-	put_device(&cxlr->dev);
-}
-
-static struct lock_class_key cxl_region_key;
-
-static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int id)
-{
-	struct cxl_region *cxlr;
-	struct device *dev;
-
-	cxlr = kzalloc(sizeof(*cxlr), GFP_KERNEL);
-	if (!cxlr) {
-		memregion_free(id);
-		return ERR_PTR(-ENOMEM);
-	}
-
-	dev = &cxlr->dev;
-	device_initialize(dev);
-	lockdep_set_class(&dev->mutex, &cxl_region_key);
-	dev->parent = &cxlrd->cxlsd.cxld.dev;
-	/*
-	 * Keep root decoder pinned through cxl_region_release to fixup
-	 * region id allocations
-	 */
-	get_device(dev->parent);
-	device_set_pm_not_required(dev);
-	dev->bus = &cxl_bus_type;
-	dev->type = &cxl_region_type;
-	cxlr->id = id;
-
-	return cxlr;
-}
-
 static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
 {
 	int cset = 0;
@@ -2498,6 +2448,56 @@ static int cxl_region_calculate_adistance(struct notifier_block *nb,
 	return NOTIFY_STOP;
 }
 
+static struct lock_class_key cxl_region_key;
+
+static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int id)
+{
+	struct cxl_region *cxlr;
+	struct device *dev;
+
+	cxlr = kzalloc(sizeof(*cxlr), GFP_KERNEL);
+	if (!cxlr) {
+		memregion_free(id);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	dev = &cxlr->dev;
+	device_initialize(dev);
+	lockdep_set_class(&dev->mutex, &cxl_region_key);
+	dev->parent = &cxlrd->cxlsd.cxld.dev;
+	/*
+	 * Keep root decoder pinned through cxl_region_release to fixup
+	 * region id allocations
+	 */
+	get_device(dev->parent);
+	device_set_pm_not_required(dev);
+	dev->bus = &cxl_bus_type;
+	dev->type = &cxl_region_type;
+	cxlr->id = id;
+
+	return cxlr;
+}
+
+static void unregister_region(void *_cxlr)
+{
+	struct cxl_region *cxlr = _cxlr;
+	struct cxl_region_params *p = &cxlr->params;
+	int i;
+
+	device_del(&cxlr->dev);
+
+	/*
+	 * Now that region sysfs is shutdown, the parameter block is now
+	 * read-only, so no need to hold the region rwsem to access the
+	 * region parameters.
+	 */
+	for (i = 0; i < p->interleave_ways; i++)
+		detach_target(cxlr, i);
+
+	cxl_region_iomem_release(cxlr);
+	put_device(&cxlr->dev);
+}
+
 /**
  * devm_cxl_add_region - Adds a region to a decoder
  * @cxlrd: root decoder
@@ -3277,6 +3277,19 @@ static int match_region_by_range(struct device *dev, const void *data)
 	return 0;
 }
 
+static struct cxl_region *
+cxl_find_region_by_range(struct cxl_root_decoder *cxlrd, struct range *hpa)
+{
+	struct device *region_dev;
+
+	region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
+				       match_region_by_range);
+	if (!region_dev)
+		return NULL;
+
+	return to_cxl_region(region_dev);
+}
+
 static int cxl_extended_linear_cache_resize(struct cxl_region *cxlr,
 					    struct resource *res)
 {
@@ -3419,19 +3432,6 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 	return cxlr;
 }
 
-static struct cxl_region *
-cxl_find_region_by_range(struct cxl_root_decoder *cxlrd, struct range *hpa)
-{
-	struct device *region_dev;
-
-	region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
-				       match_region_by_range);
-	if (!region_dev)
-		return NULL;
-
-	return to_cxl_region(region_dev);
-}
-
 int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
 {
 	struct range *hpa = &cxled->cxld.hpa_range;
-- 
2.39.5

[PATCH v1 02/20] cxl/region: Store root decoder in struct cxl_region

[Robert Richter]

A region is always bound to a root decoder. The region's associated
root decoder is often needed. Add it to struct cxl_region.

This simpifies code by removing dynamic lookups and removing the root
decoder argument from the function argument list where possible.

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/core/region.c | 33 +++++++++++++++++----------------
 drivers/cxl/cxl.h         |  2 ++
 2 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index f22ad20b0db9..a18ab5e30138 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -467,9 +467,9 @@ static ssize_t interleave_ways_store(struct device *dev,
 				     struct device_attribute *attr,
 				     const char *buf, size_t len)
 {
-	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev->parent);
-	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
 	struct cxl_region *cxlr = to_cxl_region(dev);
+	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
+	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
 	struct cxl_region_params *p = &cxlr->params;
 	unsigned int val, save;
 	int rc;
@@ -535,9 +535,9 @@ static ssize_t interleave_granularity_store(struct device *dev,
 					    struct device_attribute *attr,
 					    const char *buf, size_t len)
 {
-	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev->parent);
-	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
 	struct cxl_region *cxlr = to_cxl_region(dev);
+	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
+	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
 	struct cxl_region_params *p = &cxlr->params;
 	int rc, val;
 	u16 ig;
@@ -617,7 +617,7 @@ static DEVICE_ATTR_RO(mode);
 
 static int alloc_hpa(struct cxl_region *cxlr, resource_size_t size)
 {
-	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
+	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
 	struct cxl_region_params *p = &cxlr->params;
 	struct resource *res;
 	u64 remainder = 0;
@@ -1320,7 +1320,7 @@ static int cxl_port_setup_targets(struct cxl_port *port,
 				  struct cxl_region *cxlr,
 				  struct cxl_endpoint_decoder *cxled)
 {
-	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
+	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
 	int parent_iw, parent_ig, ig, iw, rc, inc = 0, pos = cxled->pos;
 	struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
 	struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
@@ -1679,10 +1679,10 @@ static int cxl_region_validate_position(struct cxl_region *cxlr,
 }
 
 static int cxl_region_attach_position(struct cxl_region *cxlr,
-				      struct cxl_root_decoder *cxlrd,
 				      struct cxl_endpoint_decoder *cxled,
 				      const struct cxl_dport *dport, int pos)
 {
+	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	struct cxl_switch_decoder *cxlsd = &cxlrd->cxlsd;
 	struct cxl_decoder *cxld = &cxlsd->cxld;
@@ -1919,7 +1919,7 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
 static int cxl_region_attach(struct cxl_region *cxlr,
 			     struct cxl_endpoint_decoder *cxled, int pos)
 {
-	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
+	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_region_params *p = &cxlr->params;
@@ -2024,8 +2024,7 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 			ep_port = cxled_to_port(cxled);
 			dport = cxl_find_dport_by_dev(root_port,
 						      ep_port->host_bridge);
-			rc = cxl_region_attach_position(cxlr, cxlrd, cxled,
-							dport, i);
+			rc = cxl_region_attach_position(cxlr, cxled, dport, i);
 			if (rc)
 				return rc;
 		}
@@ -2048,7 +2047,7 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 	if (rc)
 		return rc;
 
-	rc = cxl_region_attach_position(cxlr, cxlrd, cxled, dport, pos);
+	rc = cxl_region_attach_position(cxlr, cxled, dport, pos);
 	if (rc)
 		return rc;
 
@@ -2323,8 +2322,8 @@ static const struct attribute_group *region_groups[] = {
 
 static void cxl_region_release(struct device *dev)
 {
-	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev->parent);
 	struct cxl_region *cxlr = to_cxl_region(dev);
+	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
 	int id = atomic_read(&cxlrd->region_id);
 
 	/*
@@ -2470,6 +2469,8 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i
 	 * region id allocations
 	 */
 	get_device(dev->parent);
+	cxlr->cxlrd = cxlrd;
+
 	device_set_pm_not_required(dev);
 	dev->bus = &cxl_bus_type;
 	dev->type = &cxl_region_type;
@@ -2905,7 +2906,7 @@ static bool cxl_is_hpa_in_chunk(u64 hpa, struct cxl_region *cxlr, int pos)
 u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
 		   u64 dpa)
 {
-	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
+	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
 	u64 dpa_offset, hpa_offset, bits_upper, mask_upper, hpa;
 	struct cxl_region_params *p = &cxlr->params;
 	struct cxl_endpoint_decoder *cxled = NULL;
@@ -3293,7 +3294,7 @@ cxl_find_region_by_range(struct cxl_root_decoder *cxlrd, struct range *hpa)
 static int cxl_extended_linear_cache_resize(struct cxl_region *cxlr,
 					    struct resource *res)
 {
-	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
+	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
 	struct cxl_region_params *p = &cxlr->params;
 	resource_size_t size = resource_size(res);
 	resource_size_t cache_size, start;
@@ -3329,9 +3330,9 @@ static int cxl_extended_linear_cache_resize(struct cxl_region *cxlr,
 }
 
 static int __construct_region(struct cxl_region *cxlr,
-			      struct cxl_root_decoder *cxlrd,
 			      struct cxl_endpoint_decoder *cxled)
 {
+	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	struct range *hpa = &cxled->cxld.hpa_range;
 	struct cxl_region_params *p;
@@ -3423,7 +3424,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 		return cxlr;
 	}
 
-	rc = __construct_region(cxlr, cxlrd, cxled);
+	rc = __construct_region(cxlr, cxled);
 	if (rc) {
 		devm_release_action(port->uport_dev, unregister_region, cxlr);
 		return ERR_PTR(rc);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 0730f92df038..58c9db0bfb93 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -508,6 +508,7 @@ enum cxl_partition_mode {
  * struct cxl_region - CXL region
  * @dev: This region's device
  * @id: This region's id. Id is globally unique across all regions
+ * @cxlrd: Region's root decoder
  * @mode: Operational mode of the mapped capacity
  * @type: Endpoint decoder target type
  * @cxl_nvb: nvdimm bridge for coordinating @cxlr_pmem setup / shutdown
@@ -521,6 +522,7 @@ enum cxl_partition_mode {
 struct cxl_region {
 	struct device dev;
 	int id;
+	struct cxl_root_decoder *cxlrd;
 	enum cxl_partition_mode mode;
 	enum cxl_decoder_type type;
 	struct cxl_nvdimm_bridge *cxl_nvb;
-- 
2.39.5

[PATCH v1 03/20] cxl/region: Remove region id handling from cxl_region_alloc()

[Robert Richter]

The region's id is not needed to allocate the region. Move the region
id handling out of cxl_region_alloc(). This simplifies the function
interface and allows the implementation of an early region allocation
when an id is not yet assigned.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index a18ab5e30138..b968050ad3d7 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2449,16 +2449,14 @@ static int cxl_region_calculate_adistance(struct notifier_block *nb,
 
 static struct lock_class_key cxl_region_key;
 
-static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int id)
+static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd)
 {
 	struct cxl_region *cxlr;
 	struct device *dev;
 
 	cxlr = kzalloc(sizeof(*cxlr), GFP_KERNEL);
-	if (!cxlr) {
-		memregion_free(id);
+	if (!cxlr)
 		return ERR_PTR(-ENOMEM);
-	}
 
 	dev = &cxlr->dev;
 	device_initialize(dev);
@@ -2474,7 +2472,6 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i
 	device_set_pm_not_required(dev);
 	dev->bus = &cxl_bus_type;
 	dev->type = &cxl_region_type;
-	cxlr->id = id;
 
 	return cxlr;
 }
@@ -2522,13 +2519,18 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
 	struct device *dev;
 	int rc;
 
-	cxlr = cxl_region_alloc(cxlrd, id);
-	if (IS_ERR(cxlr))
+	cxlr = cxl_region_alloc(cxlrd);
+	if (IS_ERR(cxlr)) {
+		memregion_free(id);
 		return cxlr;
+	}
+
 	cxlr->mode = mode;
 	cxlr->type = type;
 
 	dev = &cxlr->dev;
+	cxlr->id = id;
+
 	rc = dev_set_name(dev, "region%d", id);
 	if (rc)
 		goto err;
-- 
2.39.5

[PATCH v1 04/20] cxl/region: Add region registration code to new function register_region()

[Robert Richter]

Separate code to register a region from its creation. This splits the
region setup into a creation and registration part. This helps
grouping the code, adds a counterpart to unregister_region() and esp.
simplifies the error handling paths as there is a single exit for a
put_device() on errors.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index b968050ad3d7..8e2521c6c845 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2326,6 +2326,9 @@ static void cxl_region_release(struct device *dev)
 	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
 	int id = atomic_read(&cxlrd->region_id);
 
+	if (cxlr->id < 0)
+		goto out;
+
 	/*
 	 * Try to reuse the recently idled id rather than the cached
 	 * next id to prevent the region id space from increasing
@@ -2468,6 +2471,7 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd)
 	 */
 	get_device(dev->parent);
 	cxlr->cxlrd = cxlrd;
+	cxlr->id = -1;
 
 	device_set_pm_not_required(dev);
 	dev->bus = &cxl_bus_type;
@@ -2496,6 +2500,30 @@ static void unregister_region(void *_cxlr)
 	put_device(&cxlr->dev);
 }
 
+static int register_region(struct cxl_region *cxlr, int id)
+{
+	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
+	struct device *dev = &cxlr->dev;
+	int rc;
+
+	rc = memregion_alloc(GFP_KERNEL);
+	if (rc < 0)
+		return rc;
+
+	if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) {
+		memregion_free(rc);
+		return -EBUSY;
+	}
+
+	cxlr->id = id;
+
+	rc = dev_set_name(dev, "region%d", cxlr->id);
+	if (rc)
+		return rc;
+
+	return device_add(dev);
+}
+
 /**
  * devm_cxl_add_region - Adds a region to a decoder
  * @cxlrd: root decoder
@@ -2516,40 +2544,29 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
 {
 	struct cxl_port *port = to_cxl_port(cxlrd->cxlsd.cxld.dev.parent);
 	struct cxl_region *cxlr;
-	struct device *dev;
 	int rc;
 
 	cxlr = cxl_region_alloc(cxlrd);
-	if (IS_ERR(cxlr)) {
-		memregion_free(id);
+	if (IS_ERR(cxlr))
 		return cxlr;
-	}
 
 	cxlr->mode = mode;
 	cxlr->type = type;
 
-	dev = &cxlr->dev;
-	cxlr->id = id;
-
-	rc = dev_set_name(dev, "region%d", id);
-	if (rc)
-		goto err;
-
-	rc = device_add(dev);
-	if (rc)
-		goto err;
+	rc = register_region(cxlr, id);
+	if (rc) {
+		put_device(&cxlr->dev);
+		return ERR_PTR(rc);
+	}
 
 	rc = devm_add_action_or_reset(port->uport_dev, unregister_region, cxlr);
 	if (rc)
 		return ERR_PTR(rc);
 
 	dev_dbg(port->uport_dev, "%s: created %s\n",
-		dev_name(&cxlrd->cxlsd.cxld.dev), dev_name(dev));
-	return cxlr;
+		dev_name(cxlr->dev.parent), dev_name(&cxlr->dev));
 
-err:
-	put_device(dev);
-	return ERR_PTR(rc);
+	return cxlr;
 }
 
 static ssize_t __create_region_show(struct cxl_root_decoder *cxlrd, char *buf)
@@ -2572,8 +2589,6 @@ 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)
 {
-	int rc;
-
 	switch (mode) {
 	case CXL_PARTMODE_RAM:
 	case CXL_PARTMODE_PMEM:
@@ -2583,15 +2598,6 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd,
 		return ERR_PTR(-EINVAL);
 	}
 
-	rc = memregion_alloc(GFP_KERNEL);
-	if (rc < 0)
-		return ERR_PTR(rc);
-
-	if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) {
-		memregion_free(rc);
-		return ERR_PTR(-EBUSY);
-	}
-
 	return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_HOSTONLYMEM);
 }
 
-- 
2.39.5

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

[Robert Richter]

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

[Joshua Hahn]

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)

[PATCH v1 06/20] cxl/region: Remove dev_err() from cxl_find_root_decoder()

[Robert Richter]

cxl_find_root_decoder() is a small helper function. Remove dev_err()
from there and instead move it to the next higher level where other
messages are generated too and all parameters are available to
generate the message. This simplifies code in cxl_find_root_decoder().

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index cfcd286251d5..760455c760e8 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3250,22 +3250,14 @@ cxl_port_find_switch_decoder(struct cxl_port *port, struct range *hpa)
 static struct cxl_root_decoder *
 cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled)
 {
-	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	struct cxl_port *port = cxled_to_port(cxled);
 	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
-	struct cxl_decoder *root, *cxld = &cxled->cxld;
-	struct range *hpa = &cxld->hpa_range;
+	struct range *hpa = &cxled->cxld.hpa_range;
+	struct cxl_decoder *root;
 
 	root = cxl_port_find_switch_decoder(&cxl_root->port, hpa);
-	if (!root) {
-		dev_err(cxlmd->dev.parent,
-			"%s:%s no CXL window for range %#llx:%#llx\n",
-			dev_name(&cxlmd->dev), dev_name(&cxld->dev),
-			cxld->hpa_range.start, cxld->hpa_range.end);
-		return NULL;
-	}
 
-	return to_cxl_root_decoder(&root->dev);
+	return root ? to_cxl_root_decoder(&root->dev) : NULL;
 }
 
 static int match_region_by_range(struct device *dev, const void *data)
@@ -3444,6 +3436,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 
 int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
 {
+	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	struct range *hpa = &cxled->cxld.hpa_range;
 	struct cxl_region_params *p;
 	bool attach = false;
@@ -3451,8 +3444,14 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
 
 	struct cxl_root_decoder *cxlrd __free(put_cxl_root_decoder) =
 		cxl_find_root_decoder(cxled);
-	if (!cxlrd)
+
+	if (!cxlrd) {
+		dev_err(cxlmd->dev.parent,
+			"%s:%s no CXL window for range %#llx:%#llx\n",
+			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
+			hpa->start, hpa->end);
 		return -ENXIO;
+	}
 
 	/*
 	 * Ensure that if multiple threads race to construct_region() for @hpa
-- 
2.39.5

[PATCH v1 07/20] cxl/region: Add new function cxl_endpoint_get_region() to simplify cxl_add_to_region()

[Robert Richter]

Reduce complexity of cxl_add_to_region() by adding the new function
cxl_endpoint_get_region(). The function handles the creation and
construction of a region, if a region already exists, that will be
used instead.

The split in two functions helps grouping the code, simplifies the
error handlers and further reworks, and reduces both functions'
complexity and number of local variables.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 760455c760e8..57ee758bdece 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3396,7 +3396,7 @@ static int __construct_region(struct cxl_region *cxlr,
 		dev_name(&cxlr->dev), p->res, p->interleave_ways,
 		p->interleave_granularity);
 
-	/* ...to match put_device() in cxl_add_to_region() */
+	/* Pair with cxl_find_region_by_range() in cxl_endpoint_get_region(). */
 	get_device(&cxlr->dev);
 
 	return 0;
@@ -3434,13 +3434,12 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 	return cxlr;
 }
 
-int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
+static struct cxl_region *
+cxl_endpoint_get_region(struct cxl_endpoint_decoder *cxled)
 {
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	struct range *hpa = &cxled->cxld.hpa_range;
-	struct cxl_region_params *p;
-	bool attach = false;
-	int rc;
+	struct cxl_region *cxlr;
 
 	struct cxl_root_decoder *cxlrd __free(put_cxl_root_decoder) =
 		cxl_find_root_decoder(cxled);
@@ -3450,23 +3449,31 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
 			"%s:%s no CXL window for range %#llx:%#llx\n",
 			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
 			hpa->start, hpa->end);
-		return -ENXIO;
+		return ERR_PTR(-ENXIO);
 	}
 
 	/*
 	 * Ensure that if multiple threads race to construct_region() for @hpa
 	 * one does the construction and the others add to that.
 	 */
-	mutex_lock(&cxlrd->range_lock);
-	struct cxl_region *cxlr __free(put_cxl_region) =
-		cxl_find_region_by_range(cxlrd, hpa);
+	guard(mutex)(&cxlrd->range_lock);
+
+	cxlr = cxl_find_region_by_range(cxlrd, hpa);
 	if (!cxlr)
-		cxlr = construct_region(cxlrd, cxled);
-	mutex_unlock(&cxlrd->range_lock);
+		return construct_region(cxlrd, cxled);
 
-	rc = PTR_ERR_OR_ZERO(cxlr);
-	if (rc)
-		return rc;
+	return cxlr;
+}
+
+int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
+{
+	struct cxl_region *cxlr;
+	struct cxl_region_params *p;
+	bool attach = false;
+
+	cxlr = cxl_endpoint_get_region(cxled);
+	if (IS_ERR(cxlr))
+		return PTR_ERR(cxlr);
 
 	attach_target(cxlr, cxled, -1, TASK_UNINTERRUPTIBLE);
 
@@ -3486,7 +3493,7 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
 				p->res);
 	}
 
-	return rc;
+	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_add_to_region, "CXL");
 
-- 
2.39.5

[PATCH v1 08/20] cxl/region: Rework memregion id allocation and move it to register_region()

[Robert Richter]

Make the atomic_cmpxchg() loop an inner loop of register_region().
Simplify calling of __create_region() by modifying the @port_id
function argument to accept a value of -1 to indicates that an
available memregion id should be assigned to the region.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 57ee758bdece..34ffd726859e 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2504,18 +2504,34 @@ static int register_region(struct cxl_region *cxlr, int id)
 {
 	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
 	struct device *dev = &cxlr->dev;
-	int rc;
+	int old, match, rc;
 
 	rc = memregion_alloc(GFP_KERNEL);
 	if (rc < 0)
 		return rc;
 
-	if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) {
+	if (id < 0)
+		match = atomic_read(&cxlrd->region_id);
+	else
+		match = id;
+
+	for (; match >= 0;) {
+		old = atomic_cmpxchg(&cxlrd->region_id, match, rc);
+		if (old == match)
+			break;
+		if (id >= 0)
+			break;
+		match = old;
+	}
+
+	if (match < 0 || match != old) {
 		memregion_free(rc);
+		if (match < 0)
+			return -ENXIO;
 		return -EBUSY;
 	}
 
-	cxlr->id = id;
+	cxlr->id = old;
 
 	rc = dev_set_name(dev, "region%d", cxlr->id);
 	if (rc)
@@ -2528,7 +2544,8 @@ static int register_region(struct cxl_region *cxlr, int id)
  * 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
+ *      region's @cxlrd. A negative value indicates that an available
+ *      memregion id should be assigned to the region.
  *
  * This is the second step of region initialization. Regions exist within an
  * address space which is mapped by a @cxlrd.
@@ -3412,11 +3429,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 	int rc, part = READ_ONCE(cxled->part);
 	struct cxl_region *cxlr;
 
-	do {
-		cxlr = __create_region(cxlrd, cxlds->part[part].mode,
-				       atomic_read(&cxlrd->region_id));
-	} while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY);
-
+	cxlr = __create_region(cxlrd, cxlds->part[part].mode, -1);
 	if (IS_ERR(cxlr)) {
 		dev_err(cxlmd->dev.parent,
 			"%s:%s: %s failed assign region: %ld\n",
-- 
2.39.5

Re: [PATCH v1 08/20] cxl/region: Rework memregion id allocation and move it to register_region()

[Joshua Hahn]

On Tue, 15 Jul 2025 21:11:31 +0200 Robert Richter <rrichter@amd.com> wrote:

> Make the atomic_cmpxchg() loop an inner loop of register_region().
> Simplify calling of __create_region() by modifying the @port_id
> function argument to accept a value of -1 to indicates that an
> available memregion id should be assigned to the region.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---

Hello Robert,

Thank you again for this patchset!

>  drivers/cxl/core/region.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 57ee758bdece..34ffd726859e 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2504,18 +2504,34 @@ static int register_region(struct cxl_region *cxlr, int id)
>  {
>  	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
>  	struct device *dev = &cxlr->dev;
> -	int rc;
> +	int old, match, rc;
>  
>  	rc = memregion_alloc(GFP_KERNEL);
>  	if (rc < 0)
>  		return rc;
>  
> -	if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) {
> +	if (id < 0)
> +		match = atomic_read(&cxlrd->region_id);
> +	else
> +		match = id;
> +
> +	for (; match >= 0;) {

Is there a reason we use a for loop with no initialization or update step?
(would a while loop suffice?)

> +		old = atomic_cmpxchg(&cxlrd->region_id, match, rc);
> +		if (old == match)
> +			break;
> +		if (id >= 0)
> +			break;
> +		match = old;
> +	}

Also, if I understand correctly, there seem to be 2 ways this loop is used.
There's the loop for when id < 0, in which case the loop will iterate at most
twice, and the loop for when id >= 0, in which case the loop will always
iterate once. The two break statements also seem to be unique to each use case.

Would it make sense to avoid using the while loop? I think that the following
code achieves the same functionality as the code above (untested), but
does not create an illusion of being able to be stuck in an infinite loop.

match = id < 0 ? atomic_read(&cxlrd->region_id) : id;

old = atomic_cmpxchg(&cxlrd->region_id, match, rc);
if (id < 0 && match != old)  {
	match = atomic_cmpxchg(&cxlrd->region_id, old, rc);
}


Of course, please let me know if this is incorrect, or you feel that this is
even more difficult to read : -) I think that the error handling below
should also behave the same way as before.

> +
> +	if (match < 0 || match != old) {
>  		memregion_free(rc);
> +		if (match < 0)
> +			return -ENXIO;
>  		return -EBUSY;
>  	}
>  
> -	cxlr->id = id;
> +	cxlr->id = old;
>  
>  	rc = dev_set_name(dev, "region%d", cxlr->id);
>  	if (rc)

[...snip...]

Thank you again for the patch! Have a great day!
Joshua

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

[PATCH v1 09/20] cxl/region: Change __construct_region() to use it as a tail function call

[Robert Richter]

Change the function interface of __construct_region() to use it as a
tail function call. Use the new cleanup helper early_region_unregister
to unregister the region in case of errors directly in
__construct_region(). This avoids additional error handling after
calling the function.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 34ffd726859e..4743421c6db5 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2540,6 +2540,15 @@ static int register_region(struct cxl_region *cxlr, int id)
 	return device_add(dev);
 }
 
+static void early_region_unregister(struct cxl_region *cxlr)
+{
+	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
+	struct cxl_port *port = to_cxl_port(cxlrd->cxlsd.cxld.dev.parent);
+
+	devm_release_action(port->uport_dev, unregister_region, cxlr);
+}
+DEFINE_FREE(early_region_unregister, struct cxl_region *, if (!IS_ERR_OR_NULL(_T)) early_region_unregister(_T))
+
 /**
  * devm_cxl_add_region - Adds a region to the CXL hierarchy.
  * @cxlr: region to be added
@@ -3347,9 +3356,10 @@ static int cxl_extended_linear_cache_resize(struct cxl_region *cxlr,
 	return 0;
 }
 
-static int __construct_region(struct cxl_region *cxlr,
-			      struct cxl_endpoint_decoder *cxled)
+static struct cxl_region *__construct_region(struct cxl_region *__cxlr,
+					     struct cxl_endpoint_decoder *cxled)
 {
+	struct cxl_region *cxlr __free(early_region_unregister) = __cxlr;
 	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	struct range *hpa = &cxled->cxld.hpa_range;
@@ -3364,14 +3374,14 @@ static int __construct_region(struct cxl_region *cxlr,
 			"%s:%s: %s autodiscovery interrupted\n",
 			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
 			__func__);
-		return -EBUSY;
+		return ERR_PTR(-EBUSY);
 	}
 
 	set_bit(CXL_REGION_F_AUTO, &cxlr->flags);
 
 	res = kmalloc(sizeof(*res), GFP_KERNEL);
 	if (!res)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	*res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa),
 				    dev_name(&cxlr->dev));
@@ -3406,7 +3416,7 @@ static int __construct_region(struct cxl_region *cxlr,
 
 	rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_target_group());
 	if (rc)
-		return rc;
+		return ERR_PTR(rc);
 
 	dev_dbg(cxlmd->dev.parent, "%s:%s: %s %s res: %pr iw: %d ig: %d\n",
 		dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), __func__,
@@ -3416,7 +3426,7 @@ static int __construct_region(struct cxl_region *cxlr,
 	/* Pair with cxl_find_region_by_range() in cxl_endpoint_get_region(). */
 	get_device(&cxlr->dev);
 
-	return 0;
+	return no_free_ptr(cxlr);
 }
 
 /* Establish an empty region covering the given HPA range */
@@ -3424,9 +3434,8 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 					   struct cxl_endpoint_decoder *cxled)
 {
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
-	struct cxl_port *port = cxlrd_to_port(cxlrd);
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
-	int rc, part = READ_ONCE(cxled->part);
+	int part = READ_ONCE(cxled->part);
 	struct cxl_region *cxlr;
 
 	cxlr = __create_region(cxlrd, cxlds->part[part].mode, -1);
@@ -3438,13 +3447,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 		return cxlr;
 	}
 
-	rc = __construct_region(cxlr, cxled);
-	if (rc) {
-		devm_release_action(port->uport_dev, unregister_region, cxlr);
-		return ERR_PTR(rc);
-	}
-
-	return cxlr;
+	return __construct_region(cxlr, cxled);
 }
 
 static struct cxl_region *
-- 
2.39.5

[PATCH v1 10/20] cxl/region: Remove __construct_region()

[Robert Richter]

There is nothing left the function does in addition to
construct_region(). Remove __construct_region() entirely.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 4743421c6db5..edd1cd9ef022 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3356,18 +3356,31 @@ static int cxl_extended_linear_cache_resize(struct cxl_region *cxlr,
 	return 0;
 }
 
-static struct cxl_region *__construct_region(struct cxl_region *__cxlr,
-					     struct cxl_endpoint_decoder *cxled)
+/* Establish an empty region covering the given HPA range */
+static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
+					   struct cxl_endpoint_decoder *cxled)
 {
-	struct cxl_region *cxlr __free(early_region_unregister) = __cxlr;
-	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	int part = READ_ONCE(cxled->part);
 	struct range *hpa = &cxled->cxld.hpa_range;
 	struct cxl_region_params *p;
 	struct resource *res;
 	int rc;
 
+	struct cxl_region *cxlr __free(early_region_unregister) =
+		__create_region(cxlrd, cxlds->part[part].mode, -1);
+
+	if (IS_ERR(cxlr)) {
+		dev_err(cxlmd->dev.parent,
+			"%s:%s: %s failed assign region: %ld\n",
+			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
+			__func__, PTR_ERR(cxlr));
+		return cxlr;
+	}
+
 	guard(rwsem_write)(&cxl_region_rwsem);
+
 	p = &cxlr->params;
 	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE) {
 		dev_err(cxlmd->dev.parent,
@@ -3429,27 +3442,6 @@ static struct cxl_region *__construct_region(struct cxl_region *__cxlr,
 	return no_free_ptr(cxlr);
 }
 
-/* Establish an empty region covering the given HPA range */
-static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
-					   struct cxl_endpoint_decoder *cxled)
-{
-	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
-	struct cxl_dev_state *cxlds = cxlmd->cxlds;
-	int part = READ_ONCE(cxled->part);
-	struct cxl_region *cxlr;
-
-	cxlr = __create_region(cxlrd, cxlds->part[part].mode, -1);
-	if (IS_ERR(cxlr)) {
-		dev_err(cxlmd->dev.parent,
-			"%s:%s: %s failed assign region: %ld\n",
-			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
-			__func__, PTR_ERR(cxlr));
-		return cxlr;
-	}
-
-	return __construct_region(cxlr, cxled);
-}
-
 static struct cxl_region *
 cxl_endpoint_get_region(struct cxl_endpoint_decoder *cxled)
 {
-- 
2.39.5

[PATCH v1 11/20] cxl/region: Separate auto-generated region cration code path

[Robert Richter]

For further optimizations, separate auto-generated region cration code
path from the one used by sysfs. Introduce the new function
create_region() that is used to auto-create a region.
__create_region() will be used as a sysfs helper function. The
separation helps to simplify both code paths as implementation
of both largely diverged already.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index edd1cd9ef022..ba59fa726cf3 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2609,15 +2609,6 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd,
 {
 	struct cxl_region *cxlr;
 
-	switch (mode) {
-	case CXL_PARTMODE_RAM:
-	case CXL_PARTMODE_PMEM:
-		break;
-	default:
-		dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode);
-		return ERR_PTR(-EINVAL);
-	}
-
 	cxlr = cxl_region_alloc(cxlrd);
 	if (IS_ERR(cxlr))
 		return cxlr;
@@ -3356,6 +3347,30 @@ static int cxl_extended_linear_cache_resize(struct cxl_region *cxlr,
 	return 0;
 }
 
+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:
+		break;
+	default:
+		dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode);
+		return ERR_PTR(-EINVAL);
+	}
+
+	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);
+}
+
 /* Establish an empty region covering the given HPA range */
 static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 					   struct cxl_endpoint_decoder *cxled)
@@ -3369,7 +3384,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 	int rc;
 
 	struct cxl_region *cxlr __free(early_region_unregister) =
-		__create_region(cxlrd, cxlds->part[part].mode, -1);
+		create_region(cxlrd, cxlds->part[part].mode, -1);
 
 	if (IS_ERR(cxlr)) {
 		dev_err(cxlmd->dev.parent,
-- 
2.39.5

[PATCH v1 12/20] cxl/region: Remove region creation code from construct_region()

[Robert Richter]

Remove region creation code from construct_region() to clearly split
the code. This simplifies the function interface of construct_region()
and reduces the number of local variables there.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index ba59fa726cf3..f88b7f86f0f5 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3348,9 +3348,14 @@ static int cxl_extended_linear_cache_resize(struct cxl_region *cxlr,
 }
 
 static struct cxl_region *create_region(struct cxl_root_decoder *cxlrd,
-					enum cxl_partition_mode mode, int id)
+					struct cxl_endpoint_decoder *cxled)
 {
+	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	int part = READ_ONCE(cxled->part);
+	enum cxl_partition_mode mode = cxlds->part[part].mode;
 	struct cxl_region *cxlr;
+	struct cxl_region_params *p;
 
 	switch (mode) {
 	case CXL_PARTMODE_RAM:
@@ -3367,8 +3372,13 @@ static struct cxl_region *create_region(struct cxl_root_decoder *cxlrd,
 
 	cxlr->mode = mode;
 	cxlr->type = CXL_DECODER_HOSTONLYMEM;
+	set_bit(CXL_REGION_F_AUTO, &cxlr->flags);
 
-	return devm_cxl_add_region(cxlr, id);
+	p = &cxlr->params;
+	p->interleave_ways = cxled->cxld.interleave_ways;
+	p->interleave_granularity = cxled->cxld.interleave_granularity;
+
+	return devm_cxl_add_region(cxlr, -1);
 }
 
 /* Establish an empty region covering the given HPA range */
@@ -3376,15 +3386,13 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 					   struct cxl_endpoint_decoder *cxled)
 {
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
-	struct cxl_dev_state *cxlds = cxlmd->cxlds;
-	int part = READ_ONCE(cxled->part);
 	struct range *hpa = &cxled->cxld.hpa_range;
 	struct cxl_region_params *p;
 	struct resource *res;
 	int rc;
 
 	struct cxl_region *cxlr __free(early_region_unregister) =
-		create_region(cxlrd, cxlds->part[part].mode, -1);
+		create_region(cxlrd, cxled);
 
 	if (IS_ERR(cxlr)) {
 		dev_err(cxlmd->dev.parent,
@@ -3405,8 +3413,6 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 		return ERR_PTR(-EBUSY);
 	}
 
-	set_bit(CXL_REGION_F_AUTO, &cxlr->flags);
-
 	res = kmalloc(sizeof(*res), GFP_KERNEL);
 	if (!res)
 		return ERR_PTR(-ENOMEM);
@@ -3438,8 +3444,6 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 	}
 
 	p->res = res;
-	p->interleave_ways = cxled->cxld.interleave_ways;
-	p->interleave_granularity = cxled->cxld.interleave_granularity;
 	p->state = CXL_CONFIG_INTERLEAVE_ACTIVE;
 
 	rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_target_group());
-- 
2.39.5

[PATCH v1 13/20] cxl/region: Move devm_cxl_add_region() out of create_region()

[Robert Richter]

To further separate creation and registration of a region, move
devm_cxl_add_region() out of create_region(). Also, adjust error
messages to still cover all errors in case create_region() fails.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index f88b7f86f0f5..e06cc92ad3e2 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3367,8 +3367,13 @@ static struct cxl_region *create_region(struct cxl_root_decoder *cxlrd,
 	}
 
 	cxlr = cxl_region_alloc(cxlrd);
-	if (IS_ERR(cxlr))
+	if (IS_ERR(cxlr)) {
+		dev_err(cxlmd->dev.parent,
+			"%s:%s: %s failed to allocate region: %ld\n",
+			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
+			__func__, PTR_ERR(cxlr));
 		return cxlr;
+	}
 
 	cxlr->mode = mode;
 	cxlr->type = CXL_DECODER_HOSTONLYMEM;
@@ -3378,7 +3383,7 @@ static struct cxl_region *create_region(struct cxl_root_decoder *cxlrd,
 	p->interleave_ways = cxled->cxld.interleave_ways;
 	p->interleave_granularity = cxled->cxld.interleave_granularity;
 
-	return devm_cxl_add_region(cxlr, -1);
+	return cxlr;
 }
 
 /* Establish an empty region covering the given HPA range */
@@ -3394,9 +3399,13 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 	struct cxl_region *cxlr __free(early_region_unregister) =
 		create_region(cxlrd, cxled);
 
+	if (IS_ERR(cxlr))
+		return cxlr;
+
+	cxlr = devm_cxl_add_region(cxlr, -1);
 	if (IS_ERR(cxlr)) {
 		dev_err(cxlmd->dev.parent,
-			"%s:%s: %s failed assign region: %ld\n",
+			"%s:%s: %s failed to add region: %ld\n",
 			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
 			__func__, PTR_ERR(cxlr));
 		return cxlr;
-- 
2.39.5

[PATCH v1 14/20] cxl/region: Prepare removal of @cxlrd argument from create_region()

[Robert Richter]

The @cxlrd function argument will be removed. Rework the error message
and use the memdev's parent as reference device, which is typically
the associated pci_dev.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index e06cc92ad3e2..9adec670432a 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3362,7 +3362,10 @@ static struct cxl_region *create_region(struct cxl_root_decoder *cxlrd,
 	case CXL_PARTMODE_PMEM:
 		break;
 	default:
-		dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode);
+		dev_err(cxlmd->dev.parent,
+			"%s:%s: %s unsupported mode %d\n",
+			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
+			__func__, mode);
 		return ERR_PTR(-EINVAL);
 	}
 
-- 
2.39.5

[PATCH v1 15/20] cxl/region: Prepare removal of @cxled argument from construct_region()

[Robert Richter]

The @cxled function argument will be removed. Rework all error
messages and use the root decoder as reference device.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 9adec670432a..81ff9956a128 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3393,7 +3393,7 @@ static struct cxl_region *create_region(struct cxl_root_decoder *cxlrd,
 static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 					   struct cxl_endpoint_decoder *cxled)
 {
-	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	struct device *cxlrd_dev = &cxlrd->cxlsd.cxld.dev;
 	struct range *hpa = &cxled->cxld.hpa_range;
 	struct cxl_region_params *p;
 	struct resource *res;
@@ -3407,10 +3407,9 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 
 	cxlr = devm_cxl_add_region(cxlr, -1);
 	if (IS_ERR(cxlr)) {
-		dev_err(cxlmd->dev.parent,
-			"%s:%s: %s failed to add region: %ld\n",
-			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
-			__func__, PTR_ERR(cxlr));
+		dev_err(cxlrd_dev->parent,
+			"%s: %s failed to add region: %ld\n",
+			dev_name(cxlrd_dev), __func__, PTR_ERR(cxlr));
 		return cxlr;
 	}
 
@@ -3418,10 +3417,8 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 
 	p = &cxlr->params;
 	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE) {
-		dev_err(cxlmd->dev.parent,
-			"%s:%s: %s autodiscovery interrupted\n",
-			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
-			__func__);
+		dev_err(cxlr->dev.parent, "%s: %s autodiscovery interrupted\n",
+			dev_name(&cxlr->dev), __func__);
 		return ERR_PTR(-EBUSY);
 	}
 
@@ -3439,8 +3436,9 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 		 * prevent the region from functioning. Only causes cxl list showing
 		 * incorrect region size.
 		 */
-		dev_warn(cxlmd->dev.parent,
-			 "Extended linear cache calculation failed rc:%d\n", rc);
+		dev_warn(cxlr->dev.parent,
+			"%s: %s Extended linear cache calculation failed rc:%d\n",
+			dev_name(&cxlr->dev), __func__, rc);
 	}
 
 	rc = insert_resource(cxlrd->res, res);
@@ -3449,10 +3447,8 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 		 * Platform-firmware may not have split resources like "System
 		 * RAM" on CXL window boundaries see cxl_region_iomem_release()
 		 */
-		dev_warn(cxlmd->dev.parent,
-			 "%s:%s: %s %s cannot insert resource\n",
-			 dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
-			 __func__, dev_name(&cxlr->dev));
+		dev_warn(cxlr->dev.parent, "%s: %s cannot insert resource\n",
+			dev_name(&cxlr->dev), __func__);
 	}
 
 	p->res = res;
@@ -3462,9 +3458,8 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 	if (rc)
 		return ERR_PTR(rc);
 
-	dev_dbg(cxlmd->dev.parent, "%s:%s: %s %s res: %pr iw: %d ig: %d\n",
-		dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), __func__,
-		dev_name(&cxlr->dev), p->res, p->interleave_ways,
+	dev_dbg(cxlr->dev.parent, "%s: %s res: %pr iw: %d ig: %d\n",
+		dev_name(&cxlr->dev), __func__, p->res, p->interleave_ways,
 		p->interleave_granularity);
 
 	/* Pair with cxl_find_region_by_range() in cxl_endpoint_get_region(). */
-- 
2.39.5

[PATCH v1 16/20] cxl/region: Introduce @hpa_range to struct cxl_region

[Robert Richter]

Each region has a known host physical address (HPA) range it is
assigned to. All assigned ports to that region share the same hpa
range. The region's address range is the system's physical address
(SPA) range. That is, for regions there is hpa == spa.

As systems with address translation have hpa != spa, track the
region's spa range and introduce @hpa_range to struct cxl_region.

The introduction of @hpa_range also helps further reworking
construct_region() in order to move the create_region() call out of
the function and remove @cxled from the parameter list.

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/core/region.c | 18 ++++++++++++++++++
 drivers/cxl/cxl.h         |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 81ff9956a128..7e21946072a5 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -653,6 +653,11 @@ static int alloc_hpa(struct cxl_region *cxlr, resource_size_t size)
 		return PTR_ERR(res);
 	}
 
+	cxlr->hpa_range = (struct range) {
+		.start = res->start,
+		.end = res->end,
+	};
+
 	p->res = res;
 	p->state = CXL_CONFIG_INTERLEAVE_ACTIVE;
 
@@ -689,8 +694,14 @@ static int free_hpa(struct cxl_region *cxlr)
 	if (p->state >= CXL_CONFIG_ACTIVE)
 		return -EBUSY;
 
+	cxlr->hpa_range = (struct range) {
+		.start = 0,
+		.end = -1,
+	};
+
 	cxl_region_iomem_release(cxlr);
 	p->state = CXL_CONFIG_IDLE;
+
 	return 0;
 }
 
@@ -2496,6 +2507,11 @@ static void unregister_region(void *_cxlr)
 	for (i = 0; i < p->interleave_ways; i++)
 		detach_target(cxlr, i);
 
+	cxlr->hpa_range = (struct range) {
+		.start = 0,
+		.end = -1,
+	};
+
 	cxl_region_iomem_release(cxlr);
 	put_device(&cxlr->dev);
 }
@@ -3354,6 +3370,7 @@ static struct cxl_region *create_region(struct cxl_root_decoder *cxlrd,
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	int part = READ_ONCE(cxled->part);
 	enum cxl_partition_mode mode = cxlds->part[part].mode;
+	struct range *hpa = &cxled->cxld.hpa_range;
 	struct cxl_region *cxlr;
 	struct cxl_region_params *p;
 
@@ -3381,6 +3398,7 @@ static struct cxl_region *create_region(struct cxl_root_decoder *cxlrd,
 	cxlr->mode = mode;
 	cxlr->type = CXL_DECODER_HOSTONLYMEM;
 	set_bit(CXL_REGION_F_AUTO, &cxlr->flags);
+	cxlr->hpa_range = *hpa;
 
 	p = &cxlr->params;
 	p->interleave_ways = cxled->cxld.interleave_ways;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 58c9db0bfb93..cdca44556039 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -509,6 +509,7 @@ enum cxl_partition_mode {
  * @dev: This region's device
  * @id: This region's id. Id is globally unique across all regions
  * @cxlrd: Region's root decoder
+ * @hpa_range: Address range occupied by the region
  * @mode: Operational mode of the mapped capacity
  * @type: Endpoint decoder target type
  * @cxl_nvb: nvdimm bridge for coordinating @cxlr_pmem setup / shutdown
@@ -523,6 +524,7 @@ struct cxl_region {
 	struct device dev;
 	int id;
 	struct cxl_root_decoder *cxlrd;
+	struct range hpa_range;
 	enum cxl_partition_mode mode;
 	enum cxl_decoder_type type;
 	struct cxl_nvdimm_bridge *cxl_nvb;
-- 
2.39.5

[PATCH v1 17/20] cxl/region: Remove create_region() call from construct_region()

[Robert Richter]

To separate the creation and construction of a region, remove the
create_region() call from construct_region().

This makes the @cxled function argument in construct_region()
obsolete, remove it and instead start using @hpa_range of struct
cxl_region, which is initialized using the endpoint decoders hpa
range.

The region creation is moved to cxl_endpoint_get_region(). Use
put_device() to remove the created region if another region is found
for the same hpa range.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 7e21946072a5..a81278fbb0ab 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3363,6 +3363,7 @@ static int cxl_extended_linear_cache_resize(struct cxl_region *cxlr,
 	return 0;
 }
 
+/* Establish a region for the endpoint decoder */
 static struct cxl_region *create_region(struct cxl_root_decoder *cxlrd,
 					struct cxl_endpoint_decoder *cxled)
 {
@@ -3407,23 +3408,17 @@ static struct cxl_region *create_region(struct cxl_root_decoder *cxlrd,
 	return cxlr;
 }
 
-/* Establish an empty region covering the given HPA range */
-static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
-					   struct cxl_endpoint_decoder *cxled)
+static struct cxl_region *construct_region(struct cxl_region *__cxlr)
 {
-	struct device *cxlrd_dev = &cxlrd->cxlsd.cxld.dev;
-	struct range *hpa = &cxled->cxld.hpa_range;
+	struct device *cxlrd_dev = &__cxlr->cxlrd->cxlsd.cxld.dev;
 	struct cxl_region_params *p;
+	struct range *hpa;
 	struct resource *res;
 	int rc;
 
 	struct cxl_region *cxlr __free(early_region_unregister) =
-		create_region(cxlrd, cxled);
-
-	if (IS_ERR(cxlr))
-		return cxlr;
+		devm_cxl_add_region(__cxlr, -1);
 
-	cxlr = devm_cxl_add_region(cxlr, -1);
 	if (IS_ERR(cxlr)) {
 		dev_err(cxlrd_dev->parent,
 			"%s: %s failed to add region: %ld\n",
@@ -3444,6 +3439,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 	if (!res)
 		return ERR_PTR(-ENOMEM);
 
+	hpa = &cxlr->hpa_range;
 	*res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa),
 				    dev_name(&cxlr->dev));
 
@@ -3459,7 +3455,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 			dev_name(&cxlr->dev), __func__, rc);
 	}
 
-	rc = insert_resource(cxlrd->res, res);
+	rc = insert_resource(cxlr->cxlrd->res, res);
 	if (rc) {
 		/*
 		 * Platform-firmware may not have split resources like "System
@@ -3491,7 +3487,7 @@ cxl_endpoint_get_region(struct cxl_endpoint_decoder *cxled)
 {
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	struct range *hpa = &cxled->cxld.hpa_range;
-	struct cxl_region *cxlr;
+	struct cxl_region *cxlr, *new;
 
 	struct cxl_root_decoder *cxlrd __free(put_cxl_root_decoder) =
 		cxl_find_root_decoder(cxled);
@@ -3504,15 +3500,21 @@ cxl_endpoint_get_region(struct cxl_endpoint_decoder *cxled)
 		return ERR_PTR(-ENXIO);
 	}
 
+	new = create_region(cxlrd, cxled);
+	if (IS_ERR(new))
+		return new;
+
 	/*
 	 * Ensure that if multiple threads race to construct_region() for @hpa
 	 * one does the construction and the others add to that.
 	 */
-	guard(mutex)(&cxlrd->range_lock);
+	guard(mutex)(&new->cxlrd->range_lock);
 
-	cxlr = cxl_find_region_by_range(cxlrd, hpa);
+	cxlr = cxl_find_region_by_range(new->cxlrd, &new->hpa_range);
 	if (!cxlr)
-		return construct_region(cxlrd, cxled);
+		return construct_region(new);
+
+	put_device(&new->dev);
 
 	return cxlr;
 }
-- 
2.39.5

[PATCH v1 18/20] cxl/region: Determine root decoder in create_region()

[Robert Richter]

Each endpoint decoder is a root decoder assigned to. It is determined
using the endpoint decoders HPA range. Move function
cxl_find_root_decoder() that determines the root decoder to
create_region(). Remove the @cxlrd argument of create_region(), it is
no longer needed.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index a81278fbb0ab..8faef2b2ee05 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3364,8 +3364,7 @@ static int cxl_extended_linear_cache_resize(struct cxl_region *cxlr,
 }
 
 /* Establish a region for the endpoint decoder */
-static struct cxl_region *create_region(struct cxl_root_decoder *cxlrd,
-					struct cxl_endpoint_decoder *cxled)
+static struct cxl_region *create_region(struct cxl_endpoint_decoder *cxled)
 {
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
@@ -3375,6 +3374,17 @@ static struct cxl_region *create_region(struct cxl_root_decoder *cxlrd,
 	struct cxl_region *cxlr;
 	struct cxl_region_params *p;
 
+	struct cxl_root_decoder *cxlrd __free(put_cxl_root_decoder) =
+		cxl_find_root_decoder(cxled);
+
+	if (!cxlrd) {
+		dev_err(cxlmd->dev.parent,
+			"%s:%s no CXL window for range %#llx:%#llx\n",
+			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
+			hpa->start, hpa->end);
+		return ERR_PTR(-ENXIO);
+	}
+
 	switch (mode) {
 	case CXL_PARTMODE_RAM:
 	case CXL_PARTMODE_PMEM:
@@ -3485,22 +3495,9 @@ static struct cxl_region *construct_region(struct cxl_region *__cxlr)
 static struct cxl_region *
 cxl_endpoint_get_region(struct cxl_endpoint_decoder *cxled)
 {
-	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
-	struct range *hpa = &cxled->cxld.hpa_range;
 	struct cxl_region *cxlr, *new;
 
-	struct cxl_root_decoder *cxlrd __free(put_cxl_root_decoder) =
-		cxl_find_root_decoder(cxled);
-
-	if (!cxlrd) {
-		dev_err(cxlmd->dev.parent,
-			"%s:%s no CXL window for range %#llx:%#llx\n",
-			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
-			hpa->start, hpa->end);
-		return ERR_PTR(-ENXIO);
-	}
-
-	new = create_region(cxlrd, cxled);
+	new = create_region(cxled);
 	if (IS_ERR(new))
 		return new;
 
-- 
2.39.5

[PATCH v1 19/20] cxl/region: Add function to find a region's duplicate

[Robert Richter]

Before registering a region, there is a check if a region with the
same parameters already exists. Rename the existing function
cxl_find_region_by_range() to cxl_region_find_duplicate() and modify
the argument list to only pass the @cxlr argument, which also
simplifies the function's interface.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 8faef2b2ee05..5c30c417de1a 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3312,9 +3312,10 @@ static int match_region_by_range(struct device *dev, const void *data)
 	return 0;
 }
 
-static struct cxl_region *
-cxl_find_region_by_range(struct cxl_root_decoder *cxlrd, struct range *hpa)
+static struct cxl_region *cxl_region_find_duplicate(struct cxl_region *cxlr)
 {
+	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
+	struct range *hpa = &cxlr->hpa_range;
 	struct device *region_dev;
 
 	region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
@@ -3486,7 +3487,7 @@ static struct cxl_region *construct_region(struct cxl_region *__cxlr)
 		dev_name(&cxlr->dev), __func__, p->res, p->interleave_ways,
 		p->interleave_granularity);
 
-	/* Pair with cxl_find_region_by_range() in cxl_endpoint_get_region(). */
+	/* Pair with cxl_region_find_duplicate() in cxl_endpoint_get_region(). */
 	get_device(&cxlr->dev);
 
 	return no_free_ptr(cxlr);
@@ -3502,12 +3503,13 @@ cxl_endpoint_get_region(struct cxl_endpoint_decoder *cxled)
 		return new;
 
 	/*
-	 * Ensure that if multiple threads race to construct_region() for @hpa
-	 * one does the construction and the others add to that.
+	 * Ensure that if multiple threads race to construct_region()
+	 * for the hpa range one does the construction and the others
+	 * add to that.
 	 */
 	guard(mutex)(&new->cxlrd->range_lock);
 
-	cxlr = cxl_find_region_by_range(new->cxlrd, &new->hpa_range);
+	cxlr = cxl_region_find_duplicate(new);
 	if (!cxlr)
 		return construct_region(new);
 
-- 
2.39.5

[PATCH v1 20/20] cxl/region: Early check region's interleaving configuration

[Robert Richter]

In interleaving configurations multiple endpoint decoders share the
same region. All endpoint decoders must be configured with the same
region parameters. Right now a check is done late during
initialization in cxl_region_attach(), and an already existing region
for the same hpa range could be bound to the endpoint decoder. That
region may have a different interleaving configuration.

To reuse the region's interleaving configuration parameters for
endpoint bringup with address translation, make sure the interleaving
parameters are always correct and add an early check for this. Add a
function cxl_region_check() to check memory mode, memory type and the
interleaving parameters. This ensures an endpoint decoder will only be
attached to a region with matching parameters. The region's parameters
can be used during endpoint decoder enumeration. Also, a config
mismatch is detected early before trying to attach the region. This
helps to identify and handle errors at an early stage.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 5c30c417de1a..aca02d011c57 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3493,10 +3493,42 @@ static struct cxl_region *construct_region(struct cxl_region *__cxlr)
 	return no_free_ptr(cxlr);
 }
 
+static int cxl_region_check(struct device *dev,
+			    struct cxl_region *cxlr,
+			    struct cxl_region *new)
+{
+	struct cxl_region_params *p = &cxlr->params;
+	struct cxl_region_params *np = &new->params;
+
+	if (cxlr->mode != new->mode) {
+		dev_dbg(dev, "%s: region mode mismatch: %d vs %d\n",
+			dev_name(&cxlr->dev), cxlr->mode, new->mode);
+		return -EINVAL;
+	}
+
+	if (cxlr->type != new->type) {
+		dev_dbg(dev, "%s: region type mismatch: %d vs %d\n",
+			dev_name(&cxlr->dev), cxlr->type, new->type);
+		return -ENXIO;
+	}
+
+	if (p->interleave_ways != np->interleave_ways ||
+	    p->interleave_granularity != np->interleave_granularity) {
+		dev_dbg(dev, "%s: interleaving config mismatch: %dx%d vs %dx%d\n",
+			dev_name(&cxlr->dev),
+			p->interleave_ways, p->interleave_granularity,
+			np->interleave_ways, np->interleave_granularity);
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
 static struct cxl_region *
 cxl_endpoint_get_region(struct cxl_endpoint_decoder *cxled)
 {
 	struct cxl_region *cxlr, *new;
+	int rc;
 
 	new = create_region(cxled);
 	if (IS_ERR(new))
@@ -3513,8 +3545,15 @@ cxl_endpoint_get_region(struct cxl_endpoint_decoder *cxled)
 	if (!cxlr)
 		return construct_region(new);
 
+	rc = cxl_region_check(&cxled->cxld.dev, cxlr, new);
+
 	put_device(&new->dev);
 
+	if (rc) {
+		put_device(&cxlr->dev);
+		return ERR_PTR(rc);
+	}
+
 	return cxlr;
 }
 
-- 
2.39.5

Re: [PATCH v1 00/20] cxl: Address translation support, part 2: Region code rework

[Gregory Price]

On Tue, Jul 15, 2025 at 09:11:23PM +0200, Robert Richter wrote:
> This series is the second part of adding support for CXL address
> translation. It adds another rework of region code to address
> implementation changes or conflicts of current address translation
> code with cxl/next, esp. the introduction of support of extended
> linear caching.
> 

Apologies in advance for delays on review and testing.  I am unavailable
until ~September.  Joshua Hahn may pick this up for testing in my
absense. I will make myself available to him for questions as needed.

+CC: Joshua

~Gregory

Re: [PATCH v1 00/20] cxl: Address translation support, part 2: Region code rework

[Dave Jiang]

On 7/15/25 12:11 PM, Robert Richter wrote:
> This series is the second part of adding support for CXL address
> translation. It adds another rework of region code to address
> implementation changes or conflicts of current address translation
> code with cxl/next, esp. the introduction of support of extended
> linear caching.
> 
> Following parts are currently planned, worked on or finished:
> 
> Part 1: Cleanups and refactoring
> Upstream: 68d8b4f399e7 ("Merge branch 'for-6.16/cxl-cleanups' into cxl-for-next")
> 
> Part 2: Region code rework
> This initial patch series.
> 
> Part 3: Extended linear cache rework
> Not yet posted.
> 
> Part 4: Generic support and AMD Zen5 platform enablement.
> Not yet posted. (Earlier version posted as part 2, v2: Generic support
> and AMD Zen5 platform enablement. [1])

Hi Robert, this is A LOT of refactoring and we are not anywhere near the actual translation implementation. Before we proceed further, can you please send out a documentation patch and describe the ZEN5 translation needs for docs/driver-api/cxl/conventions.rst similar to what Fabio is doing for LMH [1]? Thank you!

[1]: https://lore.kernel.org/linux-cxl/687ea20d2e508_34e0f2941@iweiny-mobl.notmuch/T/#t

DJ

> 
> The general changes in the implementation compared to [1] are in
> particular to use the attached region of an endpoint decoder to host
> the HPA range and interleaving configuration parameters. That is, the
> region's root decoder and HPA range are added as members @cxlrd and
> @hpa_range to struct cxl_region. Both are introduced to keep track of
> the region's SPA address range and the interleaving configuration.
> Those parameters are the same for all endpoint decoders that share the
> same interleaving setup.
> 
> The implementation must ensure that the endpoint decoder's region
> parameters are always valid. All parameters must be determined first
> and then a check must be performed if a region with identical
> parameters already exists. A split of region creation and registration
> is required as the region may not become active and may need to be
> replaced by an already existing region. Several high-level functions
> are introduced (create_region(), setup_region(), register_region(),
> cxl_endpoint_get_region(), cxl_region_find_duplicate()). Most of it is
> implemented in cxl_add_to_region().
> 
> Finally, this series adds a lot of simplification and improves error
> handling and code readability.
> 
> [1] https://lore.kernel.org/all/20250218132356.1809075-1-rrichter@amd.com/
> 
> Robert Richter (20):
>   cxl/region: Move helper functions closer to their users
>   cxl/region: Store root decoder in struct cxl_region
>   cxl/region: Remove region id handling from cxl_region_alloc()
>   cxl/region: Add region registration code to new function
>     register_region()
>   cxl/region: Separate cxl_region_alloc() from devm_cxl_add_region()
>   cxl/region: Remove dev_err() from cxl_find_root_decoder()
>   cxl/region: Add new function cxl_endpoint_get_region() to simplify
>     cxl_add_to_region()
>   cxl/region: Rework memregion id allocation and move it to
>     register_region()
>   cxl/region: Change __construct_region() to use it as a tail function
>     call
>   cxl/region: Remove __construct_region()
>   cxl/region: Separate auto-generated region cration code path
>   cxl/region: Remove region creation code from construct_region()
>   cxl/region: Move devm_cxl_add_region() out of create_region()
>   cxl/region: Prepare removal of @cxlrd argument from create_region()
>   cxl/region: Prepare removal of @cxled argument from construct_region()
>   cxl/region: Introduce @hpa_range to struct cxl_region
>   cxl/region: Remove create_region() call from construct_region()
>   cxl/region: Determine root decoder in create_region()
>   cxl/region: Add function to find a region's duplicate
>   cxl/region: Early check region's interleaving configuration
> 
>  drivers/cxl/core/region.c | 514 +++++++++++++++++++++++---------------
>  drivers/cxl/cxl.h         |   4 +
>  2 files changed, 315 insertions(+), 203 deletions(-)
> 
> 
> base-commit: 12b3d697c812aaf356e82d9e1f351fbb2ea97500