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

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)