[RFC PATCH v1 0/2] cxl/cli: HPA-ordered destroy-region teardown

[RFC PATCH v1 0/2] cxl/cli: HPA-ordered destroy-region teardown

[Pawel Mielimonka]

This series addresses an issue in destroy-region, where
region teardown relied on libcxl enumeration order, which is not
guaranteed to match the increasing HPA order exposed by the kernel. 
When a decoder window is fully populated, attempting to destroy a
non-last region causes kernel-side validation to fail (e.g.
set_dpa_size(..., 0) returns an error), and subsequent destroy/create
sequences may become impossible.

The CXL specification requires that decoder programming (and the
implicit teardown path) must preserve continuous HPA coverage and
proceed strictly in order: decoder m before decoder m+1, with each
covering an HPA range below the next one. Practically, this means that
region teardown must follow HPA-descending order and must stop as soon
as a gap in the requested suffix is encountered.

The patch introduces destroy_multiple_regions(), which collects all
regions under a given root decoder, sorts them by HPA, and destroys
only the suffix requested by the user (or all regions in the case of
“all”). The implementation guarantees that only valid teardown
sequences are attempted and prevents decoder state inconsistencies
observed during repeated destroy/create cycles.

Enable/disable paths and all existing bus/port/decoder filtering
remain unchanged.

Pawel Mielimonka (2):
  cxl/cli: add helpers to collect and sort regions by HPA
  cxl/cli: enforce HPA-descending teardown order for destroy-region

 cxl/region.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 111 insertions(+), 3 deletions(-)

-- 
2.45.1.windows.1

[RFC PATCH v1 1/2] cxl/cli: add helpers to collect and sort regions by HPA

[Pawel Mielimonka]

Introduce cmp_region_hpa() and collect_regions_sorted() helpers to
enumerate CXL regions under a given decoder and sort them by their host
physical address.
These helpers will be used by the "cxl destroy-region" command to tear
down regions in HPA-descending order, i.e. in the reverse order of
region creation. This matches the decoder programming requirements from
the CXL specification (8.2.4.20.12 - continuous HPA coverage at all
times when Lock On Commit is used) and avoids teardown sequences that
can leve decoder state inconsistent when the decoder is fully populated
(known problem).
This patch only adds the helpers; no functional changes is intended yet.

Signed-off-by: Pawel Mielimonka <pawel.mielimonka@fujitsu.com>
---
 cxl/region.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/cxl/region.c b/cxl/region.c
index 207cf2d0..58765b3d 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -831,6 +831,61 @@ out:
 	return cxl_region_disable(region);
 }
 
+static int cmp_region_hpa(const void *l, const void *r)
+{
+	const struct cxl_region *const *left = l;
+	const struct cxl_region *const *right = r;
+	u64 left_start = cxl_region_get_resource((struct cxl_region *) *left);
+	u64 right_start = cxl_region_get_resource((struct cxl_region *) *right);
+
+	if (left_start < right_start)
+		return -1;
+	if (left_start > right_start)
+		return 1;
+	return 0;
+}
+
+static int collect_regions_sorted(struct cxl_decoder *root,
+								  const char *filter,
+								  struct cxl_region ***out,
+								  int *out_nr)
+{
+	struct cxl_region *region;
+	struct cxl_region **list = NULL;
+	int nr = 0, alloc = 0;
+
+	cxl_region_foreach(root, region) {
+		if (filter && !util_cxl_region_filter(region, filter))
+			continue;
+		if (nr == alloc) {
+			int new_alloc = alloc ? alloc * 2 : 8;
+			int new_size = (size_t)new_alloc * sizeof(*list);
+			struct cxl_region **tmp;
+
+			tmp = realloc(list, new_size);
+			if (!tmp) {
+				free(list);
+				return -ENOMEM;
+			}
+			list = tmp;
+			alloc = new_alloc;
+		}
+		list[nr++] = region;
+	}
+
+	if (!nr) {
+		free(list);
+		*out = NULL;
+		*out_nr = 0;
+		return 0;
+	}
+
+	qsort(list, nr, sizeof(*list), cmp_region_hpa);
+	*out = list;
+	*out_nr = nr;
+	return 0;
+}
+
 static int destroy_region(struct cxl_region *region)
 {
 	const char *devname = cxl_region_get_devname(region);
-- 
2.45.1.windows.1

[RFC PATCH v1 2/2] cxl/cli: enforce HPA-descending teardown order for destroy-region

[Pawel Mielimonka]

Implement destroy_multiple_regions() and bypass the generic
do_region_xable() path for ACTION_DESTROY. Regions are collected and
sorted by HPA, then destroyed from highest to lowest, stopping at the
first "matching after skipped" to provide user with better error log.
This prevents attempts on non-last regions and aligns destroy-region
with required decoder programming order.

Signed-off-by: Pawel Mielimonka <pawel.mielimonka@fujitsu.com>
---
 cxl/region.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 57 insertions(+), 3 deletions(-)

diff --git a/cxl/region.c b/cxl/region.c
index 58765b3d..1bf1901a 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -950,6 +950,57 @@ static int destroy_region(struct cxl_region *region)
 	return cxl_region_delete(region);
 }
 
+static int destroy_multiple_regions(struct parsed_params *p,
+				 struct cxl_decoder *decoder,
+				 int *count)
+{
+	struct cxl_region **list;
+	int nr, rc, i;
+	bool skipped = false;
+
+	rc = collect_regions_sorted(decoder, NULL, &list, &nr);
+	if (rc) {
+		log_err(&rl, "failed to allocate region list: %s\n", strerror(-rc));
+		return rc;
+	}
+
+	for (i = nr - 1; i >= 0; --i) {
+		struct cxl_region *region = NULL;
+
+		for (int j = 0; j < p->argc; j++) {
+			region = util_cxl_region_filter(list[i], p->argv[j]);
+			if (region)
+				break;
+		}
+
+		if (!region) {
+			skipped = true;
+			continue;
+		}
+
+		/* if current region matches filter, but previous didn't, destroying would
+		 * result in breaking HPA continuity
+		 */
+		if (skipped) {
+			log_err(&rl, "failed to destroy %s: not a valid HPA suffix under %s\n",
+				cxl_region_get_devname(region),
+				cxl_decoder_get_devname(decoder));
+			rc = -EINVAL;
+			break;
+		}
+
+		rc = destroy_region(region);
+		if (rc) {
+			log_err(&rl, "%s: failed: %s\n",
+				cxl_region_get_devname(region), strerror(-rc));
+			break;
+		}
+		++(*count);
+	}
+	free(list);
+	return rc;
+}
+
 static int do_region_xable(struct cxl_region *region, enum region_actions action)
 {
 	switch (action) {
@@ -957,8 +1008,6 @@ static int do_region_xable(struct cxl_region *region, enum region_actions action
 		return cxl_region_enable(region);
 	case ACTION_DISABLE:
 		return disable_region(region);
-	case ACTION_DESTROY:
-		return destroy_region(region);
 	default:
 		return -EINVAL;
 	}
@@ -1026,7 +1075,12 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
 			if (!util_cxl_decoder_filter(decoder,
 						     param.root_decoder))
 				continue;
-			rc = decoder_region_action(p, decoder, action, count);
+
+			if (action == ACTION_DESTROY)
+				rc = destroy_multiple_regions(p, decoder, count);
+			else
+				rc = decoder_region_action(p, decoder, action, count);
+
 			if (rc)
 				err_rc = rc;
 		}
-- 
2.45.1.windows.1

Re: [RFC PATCH v1 0/2] cxl/cli: HPA-ordered destroy-region teardown

[Alison Schofield]

On Tue, Nov 25, 2025 at 11:38:22PM +0900, Pawel Mielimonka wrote:
> This series addresses an issue in destroy-region, where
> region teardown relied on libcxl enumeration order, which is not
> guaranteed to match the increasing HPA order exposed by the kernel. 
> When a decoder window is fully populated, attempting to destroy a
> non-last region causes kernel-side validation to fail (e.g.
> set_dpa_size(..., 0) returns an error), and subsequent destroy/create
> sequences may become impossible.
> 
> The CXL specification requires that decoder programming (and the
> implicit teardown path) must preserve continuous HPA coverage and
> proceed strictly in order: decoder m before decoder m+1, with each
> covering an HPA range below the next one. Practically, this means that
> region teardown must follow HPA-descending order and must stop as soon
> as a gap in the requested suffix is encountered.
> 
> The patch introduces destroy_multiple_regions(), which collects all
> regions under a given root decoder, sorts them by HPA, and destroys
> only the suffix requested by the user (or all regions in the case of
> “all”). The implementation guarantees that only valid teardown
> sequences are attempted and prevents decoder state inconsistencies
> observed during repeated destroy/create cycles.
> 
> Enable/disable paths and all existing bus/port/decoder filtering
> remain unchanged.

Hi Pawel,

Thanks - this is much needed!

It's very valuable that now trying to destroy any region out of 
order will fail 'gracefully'.

This worked for 'all' and needs a small fixup for the decoder option.
That's noted in Patch 2 reply. I didn't test the other filtering yet
but started updating a unit test to cover these cases.

See https://github.com/pmem/ndctl/blob/main/CONTRIBUTING.md for
a few housekeeping tips on submitting to NDCTL.

-- Alison


> 
> Pawel Mielimonka (2):
>   cxl/cli: add helpers to collect and sort regions by HPA
>   cxl/cli: enforce HPA-descending teardown order for destroy-region
> 
>  cxl/region.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 111 insertions(+), 3 deletions(-)
> 
> -- 
> 2.45.1.windows.1
> 
> 

Re: [RFC PATCH v1 1/2] cxl/cli: add helpers to collect and sort regions by HPA

[Alison Schofield]

On Tue, Nov 25, 2025 at 11:38:23PM +0900, Pawel Mielimonka wrote:
> Introduce cmp_region_hpa() and collect_regions_sorted() helpers to
> enumerate CXL regions under a given decoder and sort them by their host
> physical address.
> These helpers will be used by the "cxl destroy-region" command to tear
> down regions in HPA-descending order, i.e. in the reverse order of
> region creation. This matches the decoder programming requirements from
> the CXL specification (8.2.4.20.12 - continuous HPA coverage at all
> times when Lock On Commit is used) and avoids teardown sequences that
> can leve decoder state inconsistent when the decoder is fully populated
> (known problem).
> This patch only adds the helpers; no functional changes is intended yet.
> 
> Signed-off-by: Pawel Mielimonka <pawel.mielimonka@fujitsu.com>
> ---
>  cxl/region.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index 207cf2d0..58765b3d 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -831,6 +831,61 @@ out:
>  	return cxl_region_disable(region);
>  }
>  
> +static int cmp_region_hpa(const void *l, const void *r)
> +{
> +	const struct cxl_region *const *left = l;
> +	const struct cxl_region *const *right = r;
> +	u64 left_start = cxl_region_get_resource((struct cxl_region *) *left);
> +	u64 right_start = cxl_region_get_resource((struct cxl_region *) *right);
> +
> +	if (left_start < right_start)
> +		return -1;
> +	if (left_start > right_start)
> +		return 1;

Suggest calling them hpa's and using this more common kernel compare pattern.
(yeah, in ndctl, cxl/cli, we try to be like kernel)

static int cmp_region_hpa(const void *a, const void *b)
{
        const struct cxl_region *const *r1 = a;
        const struct cxl_region *const *r2 = b;
        u64 hpa1 = cxl_region_get_resource((struct cxl_region *) *r1);
        u64 hpa2 = cxl_region_get_resource((struct cxl_region *) *r2);

        return (hpa1 > hpa2) - (hpa1 < hpa2);
}

> +	return 0;
> +}
> +
> +static int collect_regions_sorted(struct cxl_decoder *root,
> +								  const char *filter,
> +								  struct cxl_region ***out,
> +								  int *out_nr)
> +{

I think there is an alignment issue above.
The filter parameter is always called w NULL in patcht 2.
If it's not going to be used, remove it.

> +	struct cxl_region *region;
> +	struct cxl_region **list = NULL;
> +	int nr = 0, alloc = 0;
> +
> +	cxl_region_foreach(root, region) {
> +		if (filter && !util_cxl_region_filter(region, filter))
> +			continue;
> +		if (nr == alloc) {
> +			int new_alloc = alloc ? alloc * 2 : 8;
> +			int new_size = (size_t)new_alloc * sizeof(*list);

Looks like new_size should be size_t to match what realloc() expects.


> +			struct cxl_region **tmp;
> +
> +			tmp = realloc(list, new_size);
> +			if (!tmp) {
> +				free(list);
> +				return -ENOMEM;
> +			}
> +			list = tmp;
> +			alloc = new_alloc;
> +		}
> +		list[nr++] = region;
> +	}
> +
> +	if (!nr) {
> +		free(list);
> +		*out = NULL;
> +		*out_nr = 0;
> +		return 0;
> +	}
> +
> +	qsort(list, nr, sizeof(*list), cmp_region_hpa);
> +	*out = list;
> +	*out_nr = nr;
> +	return 0;
> +}
> +
>  static int destroy_region(struct cxl_region *region)
>  {
>  	const char *devname = cxl_region_get_devname(region);
> -- 
> 2.45.1.windows.1
> 

Re: [RFC PATCH v1 2/2] cxl/cli: enforce HPA-descending teardown order for destroy-region

[Alison Schofield]

On Tue, Nov 25, 2025 at 11:38:24PM +0900, Pawel Mielimonka wrote:
> Implement destroy_multiple_regions() and bypass the generic
> do_region_xable() path for ACTION_DESTROY. Regions are collected and
> sorted by HPA, then destroyed from highest to lowest, stopping at the
> first "matching after skipped" to provide user with better error log.
> This prevents attempts on non-last regions and aligns destroy-region
> with required decoder programming order.

It would be useful to add a sample bad spew or what happens now
on attempt to destroy out of order.  Folks sometimes search on
those strings.


> 
> Signed-off-by: Pawel Mielimonka <pawel.mielimonka@fujitsu.com>
> ---
>  cxl/region.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index 58765b3d..1bf1901a 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -950,6 +950,57 @@ static int destroy_region(struct cxl_region *region)
>  	return cxl_region_delete(region);
>  }
>  
> +static int destroy_multiple_regions(struct parsed_params *p,
> +				 struct cxl_decoder *decoder,
> +				 int *count)
> +{
> +	struct cxl_region **list;
> +	int nr, rc, i;
> +	bool skipped = false;
> +
> +	rc = collect_regions_sorted(decoder, NULL, &list, &nr);
> +	if (rc) {
> +		log_err(&rl, "failed to allocate region list: %s\n", strerror(-rc));
> +		return rc;
> +	}
> +
> +	for (i = nr - 1; i >= 0; --i) {
> +		struct cxl_region *region = NULL;
> +

Here is where there is a difference btw 'all' and a decoder. 'All' gets
passed as an argument and the filter function recognizes it. But for the
by decoder option: "cxl destroy-region -f -d decoder0.2" argc=0 needs
special handling

Inserting this here worked for me:
+               /* If no region arguments provided, match all regions */
+               if (p->argc == 0)
+                       region = list[i];
+

Then with argc == 0 this next loop is a no-op but that is OK because
region is now assigned.


> +		for (int j = 0; j < p->argc; j++) {
> +			region = util_cxl_region_filter(list[i], p->argv[j]);
> +			if (region)
> +				break;
> +		}
> +
> +		if (!region) {
> +			skipped = true;
> +			continue;
> +		}
> +
> +		/* if current region matches filter, but previous didn't, destroying would
> +		 * result in breaking HPA continuity
> +		 */

Use kernel comment style. See other samples in this file.


> +		if (skipped) {
> +			log_err(&rl, "failed to destroy %s: not a valid HPA suffix under %s\n",

I'm not familiar w the usage of 'suffix' in this context.
How about replace "not a valid HPA suffix under"
with "out of order decoder reset"


> +				cxl_region_get_devname(region),
> +				cxl_decoder_get_devname(decoder));
> +			rc = -EINVAL;
> +			break;
> +		}
> +
> +		rc = destroy_region(region);
> +		if (rc) {
> +			log_err(&rl, "%s: failed: %s\n",
> +				cxl_region_get_devname(region), strerror(-rc));
> +			break;
> +		}
> +		++(*count);
> +	}
> +	free(list);
> +	return rc;
> +}
> +
>  static int do_region_xable(struct cxl_region *region, enum region_actions action)
>  {
>  	switch (action) {
> @@ -957,8 +1008,6 @@ static int do_region_xable(struct cxl_region *region, enum region_actions action
>  		return cxl_region_enable(region);
>  	case ACTION_DISABLE:
>  		return disable_region(region);
> -	case ACTION_DESTROY:
> -		return destroy_region(region);
>  	default:
>  		return -EINVAL;
>  	}
> @@ -1026,7 +1075,12 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
>  			if (!util_cxl_decoder_filter(decoder,
>  						     param.root_decoder))
>  				continue;
> -			rc = decoder_region_action(p, decoder, action, count);
> +
> +			if (action == ACTION_DESTROY)
> +				rc = destroy_multiple_regions(p, decoder, count);
> +			else
> +				rc = decoder_region_action(p, decoder, action, count);
> +
>  			if (rc)
>  				err_rc = rc;
>  		}
> -- 
> 2.45.1.windows.1
> 

[RFC PATCH v1 1/2] cxl/cli: add helpers to collect and sort regions by HPA

[Paweł Mielimonka]

W dniu 3.12.2025 o 04:52, Alison Schofield pisze:
> On Tue, Nov 25, 2025 at 11:38:23PM +0900, Pawel Mielimonka wrote:
>> Introduce cmp_region_hpa() and collect_regions_sorted() helpers to
>> enumerate CXL regions under a given decoder and sort them by their host
>> physical address.
>> These helpers will be used by the "cxl destroy-region" command to tear
>> down regions in HPA-descending order, i.e. in the reverse order of
>> region creation. This matches the decoder programming requirements from
>> the CXL specification (8.2.4.20.12 - continuous HPA coverage at all
>> times when Lock On Commit is used) and avoids teardown sequences that
>> can leve decoder state inconsistent when the decoder is fully populated
>> (known problem).
>> This patch only adds the helpers; no functional changes is intended yet.
>>
>> Signed-off-by: Pawel Mielimonka <pawel.mielimonka@fujitsu.com>
>> ---
>>   cxl/region.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 55 insertions(+)
>>
>> diff --git a/cxl/region.c b/cxl/region.c
>> index 207cf2d0..58765b3d 100644
>> --- a/cxl/region.c
>> +++ b/cxl/region.c
>> @@ -831,6 +831,61 @@ out:
>>   	return cxl_region_disable(region);
>>   }
>>   
>> +static int cmp_region_hpa(const void *l, const void *r)
>> +{
>> +	const struct cxl_region *const *left = l;
>> +	const struct cxl_region *const *right = r;
>> +	u64 left_start = cxl_region_get_resource((struct cxl_region *) *left);
>> +	u64 right_start = cxl_region_get_resource((struct cxl_region *) *right);
>> +
>> +	if (left_start < right_start)
>> +		return -1;
>> +	if (left_start > right_start)
>> +		return 1;
> Suggest calling them hpa's and using this more common kernel compare pattern.
> (yeah, in ndctl, cxl/cli, we try to be like kernel)
>
> static int cmp_region_hpa(const void *a, const void *b)
> {
>          const struct cxl_region *const *r1 = a;
>          const struct cxl_region *const *r2 = b;
>          u64 hpa1 = cxl_region_get_resource((struct cxl_region *) *r1);
>          u64 hpa2 = cxl_region_get_resource((struct cxl_region *) *r2);
>
>          return (hpa1 > hpa2) - (hpa1 < hpa2);
> }
I followed the pattern in region.c but you're right, I'll fix it in v2.
>
>> +	return 0;
>> +}
>> +
>> +static int collect_regions_sorted(struct cxl_decoder *root,
>> +								  const char *filter,
>> +								  struct cxl_region ***out,
>> +								  int *out_nr)
>> +{
> I think there is an alignment issue above.
> The filter parameter is always called w NULL in patcht 2.
> If it's not going to be used, remove it.
>
>> +	struct cxl_region *region;
>> +	struct cxl_region **list = NULL;
>> +	int nr = 0, alloc = 0;
>> +
>> +	cxl_region_foreach(root, region) {
>> +		if (filter && !util_cxl_region_filter(region, filter))
>> +			continue;
>> +		if (nr == alloc) {
>> +			int new_alloc = alloc ? alloc * 2 : 8;
>> +			int new_size = (size_t)new_alloc * sizeof(*list);
> Looks like new_size should be size_t to match what realloc() expects.
Both changes I'll make in v2. Thanks for noticing.
>
>> +			struct cxl_region **tmp;
>> +
>> +			tmp = realloc(list, new_size);
>> +			if (!tmp) {
>> +				free(list);
>> +				return -ENOMEM;
>> +			}
>> +			list = tmp;
>> +			alloc = new_alloc;
>> +		}
>> +		list[nr++] = region;
>> +	}
>> +
>> +	if (!nr) {
>> +		free(list);
>> +		*out = NULL;
>> +		*out_nr = 0;
>> +		return 0;
>> +	}
>> +
>> +	qsort(list, nr, sizeof(*list), cmp_region_hpa);
>> +	*out = list;
>> +	*out_nr = nr;
>> +	return 0;
>> +}
>> +
>>   static int destroy_region(struct cxl_region *region)
>>   {
>>   	const char *devname = cxl_region_get_devname(region);
>> -- 
>> 2.45.1.windows.1
>>

[RFC PATCH v1 2/2] cxl/cli: enforce HPA-descending teardown order for destroy-region

[Paweł Mielimonka]

W dniu 3.12.2025 o 05:15, Alison Schofield pisze:
> On Tue, Nov 25, 2025 at 11:38:24PM +0900, Pawel Mielimonka wrote:
>> Implement destroy_multiple_regions() and bypass the generic
>> do_region_xable() path for ACTION_DESTROY. Regions are collected and
>> sorted by HPA, then destroyed from highest to lowest, stopping at the
>> first "matching after skipped" to provide user with better error log.
>> This prevents attempts on non-last regions and aligns destroy-region
>> with required decoder programming order.
> It would be useful to add a sample bad spew or what happens now
> on attempt to destroy out of order.  Folks sometimes search on
> those strings.
In all cases in which I reproduced this misbehavior, in log i could find:

"write(3</sys/devices/platform/ACPI0017:00/root0/port1/endpoint2/decoder2.0/dpa_size>, 
"0\n\0", 3) = -1 EBUSY (Device or resource busy)"

And the cli returned:

"cxl region: destroy_region: decoder 2.0: set_dpa_size failed: Device or 
resource busy"

>
>> Signed-off-by: Pawel Mielimonka <pawel.mielimonka@fujitsu.com>
>> ---
>>   cxl/region.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 57 insertions(+), 3 deletions(-)
>>
>> diff --git a/cxl/region.c b/cxl/region.c
>> index 58765b3d..1bf1901a 100644
>> --- a/cxl/region.c
>> +++ b/cxl/region.c
>> @@ -950,6 +950,57 @@ static int destroy_region(struct cxl_region *region)
>>   	return cxl_region_delete(region);
>>   }
>>   
>> +static int destroy_multiple_regions(struct parsed_params *p,
>> +				 struct cxl_decoder *decoder,
>> +				 int *count)
>> +{
>> +	struct cxl_region **list;
>> +	int nr, rc, i;
>> +	bool skipped = false;
>> +
>> +	rc = collect_regions_sorted(decoder, NULL, &list, &nr);
>> +	if (rc) {
>> +		log_err(&rl, "failed to allocate region list: %s\n", strerror(-rc));
>> +		return rc;
>> +	}
>> +
>> +	for (i = nr - 1; i >= 0; --i) {
>> +		struct cxl_region *region = NULL;
>> +
> Here is where there is a difference btw 'all' and a decoder. 'All' gets
> passed as an argument and the filter function recognizes it. But for the
> by decoder option: "cxl destroy-region -f -d decoder0.2" argc=0 needs
> special handling
>
> Inserting this here worked for me:
> +               /* If no region arguments provided, match all regions */
> +               if (p->argc == 0)
> +                       region = list[i];
> +
>
> Then with argc == 0 this next loop is a no-op but that is OK because
> region is now assigned.
Thanks for the suggestion.

According to the manual, "cxl destroy-region -f -d decoder0.2" is not a
valid CLI invocation — a region number (or "all") is required:

     SYNOPSIS
         cxl destroy-region <region> [<options>]

So the case you describe would normally be written as
"cxl destroy-region all -f -d decoder0.2". If we want to accept the case
where no region argument is provided, it feels like this should be
handled earlier during parsing and the documentation should explicitly
state that "all" is assumed when <region> is omitted.

Given the current docs, I would lean towards reporting an error when
p->argc == 0, unless we decide to update the documentation to permit the
implicit "all" behavior.

I've also tested various cases with explicit region arguments, e.g.
"cxl destroy-region 1 2 3" with regions 0..7 present, and those paths
behave correctly.

Happy to adjust in whichever direction makes the most sense.
>
>> +		for (int j = 0; j < p->argc; j++) {
>> +			region = util_cxl_region_filter(list[i], p->argv[j]);
>> +			if (region)
>> +				break;
>> +		}
>> +
>> +		if (!region) {
>> +			skipped = true;
>> +			continue;
>> +		}
>> +
>> +		/* if current region matches filter, but previous didn't, destroying would
>> +		 * result in breaking HPA continuity
>> +		 */
> Use kernel comment style. See other samples in this file.
>
>
>> +		if (skipped) {
>> +			log_err(&rl, "failed to destroy %s: not a valid HPA suffix under %s\n",
> I'm not familiar w the usage of 'suffix' in this context.
> How about replace "not a valid HPA suffix under"
> with "out of order decoder reset"
I intended it to mean "the last region in HPA order". Your wording is 
clearer,
so I'll adopt your suggestion in v2.
>
>> +				cxl_region_get_devname(region),
>> +				cxl_decoder_get_devname(decoder));
>> +			rc = -EINVAL;
>> +			break;
>> +		}
>> +
>> +		rc = destroy_region(region);
>> +		if (rc) {
>> +			log_err(&rl, "%s: failed: %s\n",
>> +				cxl_region_get_devname(region), strerror(-rc));
>> +			break;
>> +		}
>> +		++(*count);
>> +	}
>> +	free(list);
>> +	return rc;
>> +}
>> +
>>   static int do_region_xable(struct cxl_region *region, enum region_actions action)
>>   {
>>   	switch (action) {
>> @@ -957,8 +1008,6 @@ static int do_region_xable(struct cxl_region *region, enum region_actions action
>>   		return cxl_region_enable(region);
>>   	case ACTION_DISABLE:
>>   		return disable_region(region);
>> -	case ACTION_DESTROY:
>> -		return destroy_region(region);
>>   	default:
>>   		return -EINVAL;
>>   	}
>> @@ -1026,7 +1075,12 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
>>   			if (!util_cxl_decoder_filter(decoder,
>>   						     param.root_decoder))
>>   				continue;
>> -			rc = decoder_region_action(p, decoder, action, count);
>> +
>> +			if (action == ACTION_DESTROY)
>> +				rc = destroy_multiple_regions(p, decoder, count);
>> +			else
>> +				rc = decoder_region_action(p, decoder, action, count);
>> +
>>   			if (rc)
>>   				err_rc = rc;
>>   		}
>> -- 
>> 2.45.1.windows.1
>>

Re: [RFC PATCH v1 2/2] cxl/cli: enforce HPA-descending teardown order for destroy-region

[Alison Schofield]

On Wed, Dec 03, 2025 at 11:13:10AM +0100, Paweł Mielimonka wrote:
> 
> W dniu 3.12.2025 o 05:15, Alison Schofield pisze:
> > On Tue, Nov 25, 2025 at 11:38:24PM +0900, Pawel Mielimonka wrote:
> > > Implement destroy_multiple_regions() and bypass the generic
> > > do_region_xable() path for ACTION_DESTROY. Regions are collected and
> > > sorted by HPA, then destroyed from highest to lowest, stopping at the
> > > first "matching after skipped" to provide user with better error log.
> > > This prevents attempts on non-last regions and aligns destroy-region
> > > with required decoder programming order.
> > It would be useful to add a sample bad spew or what happens now
> > on attempt to destroy out of order.  Folks sometimes search on
> > those strings.
> In all cases in which I reproduced this misbehavior, in log i could find:
> 
> "write(3</sys/devices/platform/ACPI0017:00/root0/port1/endpoint2/decoder2.0/dpa_size>,
> "0\n\0", 3) = -1 EBUSY (Device or resource busy)"
> 
> And the cli returned:
>

I think this string below is the one to add to the commit log:

> "cxl region: destroy_region: decoder 2.0: set_dpa_size failed: Device or
> resource busy"

snip

> 
> > 
> > > Signed-off-by: Pawel Mielimonka <pawel.mielimonka@fujitsu.com>
> > > ---
> > >   cxl/region.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++---
> > >   1 file changed, 57 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/cxl/region.c b/cxl/region.c
> > > index 58765b3d..1bf1901a 100644
> > > --- a/cxl/region.c
> > > +++ b/cxl/region.c
> > > @@ -950,6 +950,57 @@ static int destroy_region(struct cxl_region *region)
> > >   	return cxl_region_delete(region);
> > >   }
> > > +static int destroy_multiple_regions(struct parsed_params *p,
> > > +				 struct cxl_decoder *decoder,
> > > +				 int *count)
> > > +{
> > > +	struct cxl_region **list;
> > > +	int nr, rc, i;
> > > +	bool skipped = false;
> > > +
> > > +	rc = collect_regions_sorted(decoder, NULL, &list, &nr);
> > > +	if (rc) {
> > > +		log_err(&rl, "failed to allocate region list: %s\n", strerror(-rc));
> > > +		return rc;
> > > +	}
> > > +
> > > +	for (i = nr - 1; i >= 0; --i) {
> > > +		struct cxl_region *region = NULL;
> > > +
> > Here is where there is a difference btw 'all' and a decoder. 'All' gets
> > passed as an argument and the filter function recognizes it. But for the
> > by decoder option: "cxl destroy-region -f -d decoder0.2" argc=0 needs
> > special handling
> > 
> > Inserting this here worked for me:
> > +               /* If no region arguments provided, match all regions */
> > +               if (p->argc == 0)
> > +                       region = list[i];
> > +
> > 
> > Then with argc == 0 this next loop is a no-op but that is OK because
> > region is now assigned.
> Thanks for the suggestion.
> 
> According to the manual, "cxl destroy-region -f -d decoder0.2" is not a
> valid CLI invocation — a region number (or "all") is required:
> 
>     SYNOPSIS
>         cxl destroy-region <region> [<options>]
> 
> So the case you describe would normally be written as
> "cxl destroy-region all -f -d decoder0.2". If we want to accept the case
> where no region argument is provided, it feels like this should be
> handled earlier during parsing and the documentation should explicitly
> state that "all" is assumed when <region> is omitted.
> 
> Given the current docs, I would lean towards reporting an error when
> p->argc == 0, unless we decide to update the documentation to permit the
> implicit "all" behavior.

I thought I was being intuitive, ie specific regions plus a filter seems
redundant. Thanks for clearing that up. I agree w you that it should stay
as defined. It's a bit out of scope, but if you can fix up the parsing to
fail with a clear message when the required param is missing that would be
nice.

> 
> I've also tested various cases with explicit region arguments, e.g.
> "cxl destroy-region 1 2 3" with regions 0..7 present, and those paths
> behave correctly.

Great!

> 
> Happy to adjust in whichever direction makes the most sense.
> > 
> > > +		for (int j = 0; j < p->argc; j++) {
> > > +			region = util_cxl_region_filter(list[i], p->argv[j]);
> > > +			if (region)
> > > +				break;
> > > +		}
> > > +
> > > +		if (!region) {
> > > +			skipped = true;
> > > +			continue;
> > > +		}
> > > +
> > > +		/* if current region matches filter, but previous didn't, destroying would
> > > +		 * result in breaking HPA continuity
> > > +		 */
> > Use kernel comment style. See other samples in this file.
> > 
> > 
> > > +		if (skipped) {
> > > +			log_err(&rl, "failed to destroy %s: not a valid HPA suffix under %s\n",
> > I'm not familiar w the usage of 'suffix' in this context.
> > How about replace "not a valid HPA suffix under"
> > with "out of order decoder reset"
> I intended it to mean "the last region in HPA order". Your wording is
> clearer,
> so I'll adopt your suggestion in v2.
> > 
> > > +				cxl_region_get_devname(region),
> > > +				cxl_decoder_get_devname(decoder));
> > > +			rc = -EINVAL;
> > > +			break;
> > > +		}
> > > +
> > > +		rc = destroy_region(region);
> > > +		if (rc) {
> > > +			log_err(&rl, "%s: failed: %s\n",
> > > +				cxl_region_get_devname(region), strerror(-rc));
> > > +			break;
> > > +		}
> > > +		++(*count);
> > > +	}
> > > +	free(list);
> > > +	return rc;
> > > +}
> > > +
> > >   static int do_region_xable(struct cxl_region *region, enum region_actions action)
> > >   {
> > >   	switch (action) {
> > > @@ -957,8 +1008,6 @@ static int do_region_xable(struct cxl_region *region, enum region_actions action
> > >   		return cxl_region_enable(region);
> > >   	case ACTION_DISABLE:
> > >   		return disable_region(region);
> > > -	case ACTION_DESTROY:
> > > -		return destroy_region(region);
> > >   	default:
> > >   		return -EINVAL;
> > >   	}
> > > @@ -1026,7 +1075,12 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
> > >   			if (!util_cxl_decoder_filter(decoder,
> > >   						     param.root_decoder))
> > >   				continue;
> > > -			rc = decoder_region_action(p, decoder, action, count);
> > > +
> > > +			if (action == ACTION_DESTROY)
> > > +				rc = destroy_multiple_regions(p, decoder, count);
> > > +			else
> > > +				rc = decoder_region_action(p, decoder, action, count);
> > > +
> > >   			if (rc)
> > >   				err_rc = rc;
> > >   		}
> > > -- 
> > > 2.45.1.windows.1
> > > 

Re: [RFC PATCH v1 2/2] cxl/cli: enforce HPA-descending teardown order for destroy-region

[Alison Schofield]

On Tue, Nov 25, 2025 at 11:38:24PM +0900, Pawel Mielimonka wrote:
> Implement destroy_multiple_regions() and bypass the generic
> do_region_xable() path for ACTION_DESTROY. Regions are collected and
> sorted by HPA, then destroyed from highest to lowest, stopping at the
> first "matching after skipped" to provide user with better error log.
> This prevents attempts on non-last regions and aligns destroy-region
> with required decoder programming order.

Hi Pawel,

It looks like teardown needs another sorting layer that accounts for
endpoint decoders.

While updating the unit test, I used a configuration where a memdev is
targeted by regions under multiple root decoders. In that scenario,
the 'all' or 'all -b bus' teardowns fail because we try to teardown
endpoint decoders out of order.

Appending some debug output from the failing case. Regions 5,10,11,12
use endpoint decoder14.4,3,2,1 and need to be torn down before region3
which uses endpoint14.0. 

Also note that with this config, a cxl destroy-region region3 should
fail gracefully with an out of order message.

+ /root/ndctl/build/cxl/cxl destroy-region -f all
cxl region: destroy_multiple_regions: DEBUG: Decoder decoder0.0 - 1 regions collected
cxl region: destroy_multiple_regions: DEBUG: Region[0] = region3
	[348.871669] cxl_core:cxl_dpa_free:567: cxl decoder14.0: expected decoder14.4
cxl region: destroy_region: decoder14.0: set_dpa_size failed: Device or resource busy
cxl region: destroy_multiple_regions: region3: failed: Device or resource busy
cxl region: destroy_multiple_regions: DEBUG: Decoder decoder0.1 - 0 regions collected
cxl region: destroy_multiple_regions: DEBUG: Decoder decoder0.2 - 4 regions collected
cxl region: destroy_multiple_regions: DEBUG: Region[0] = region5
cxl region: destroy_multiple_regions: DEBUG: Region[1] = region10
cxl region: destroy_multiple_regions: DEBUG: Region[2] = region11
cxl region: destroy_multiple_regions: DEBUG: Region[3] = region12
cxl region: destroy_multiple_regions: DEBUG: Decoder decoder0.3 - 0 regions collected
cxl region: destroy_multiple_regions: DEBUG: Decoder decoder0.4 - 0 regions collected
cxl region: destroy_multiple_regions: DEBUG: Decoder decoder0.5 - 0 regions collected
cxl region: region_action: one or more failures, last failure: Device or resource busy
cxl region: cmd_destroy_region: destroyed 4 regions


-- Alison

> 
> Signed-off-by: Pawel Mielimonka <pawel.mielimonka@fujitsu.com>
> ---
>  cxl/region.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index 58765b3d..1bf1901a 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -950,6 +950,57 @@ static int destroy_region(struct cxl_region *region)
>  	return cxl_region_delete(region);
>  }
>  
> +static int destroy_multiple_regions(struct parsed_params *p,
> +				 struct cxl_decoder *decoder,
> +				 int *count)
> +{
> +	struct cxl_region **list;
> +	int nr, rc, i;
> +	bool skipped = false;
> +
> +	rc = collect_regions_sorted(decoder, NULL, &list, &nr);
> +	if (rc) {
> +		log_err(&rl, "failed to allocate region list: %s\n", strerror(-rc));
> +		return rc;
> +	}
> +
> +	for (i = nr - 1; i >= 0; --i) {
> +		struct cxl_region *region = NULL;
> +
> +		for (int j = 0; j < p->argc; j++) {
> +			region = util_cxl_region_filter(list[i], p->argv[j]);
> +			if (region)
> +				break;
> +		}
> +
> +		if (!region) {
> +			skipped = true;
> +			continue;
> +		}
> +
> +		/* if current region matches filter, but previous didn't, destroying would
> +		 * result in breaking HPA continuity
> +		 */
> +		if (skipped) {
> +			log_err(&rl, "failed to destroy %s: not a valid HPA suffix under %s\n",
> +				cxl_region_get_devname(region),
> +				cxl_decoder_get_devname(decoder));
> +			rc = -EINVAL;
> +			break;
> +		}
> +
> +		rc = destroy_region(region);
> +		if (rc) {
> +			log_err(&rl, "%s: failed: %s\n",
> +				cxl_region_get_devname(region), strerror(-rc));
> +			break;
> +		}
> +		++(*count);
> +	}
> +	free(list);
> +	return rc;
> +}
> +
>  static int do_region_xable(struct cxl_region *region, enum region_actions action)
>  {
>  	switch (action) {
> @@ -957,8 +1008,6 @@ static int do_region_xable(struct cxl_region *region, enum region_actions action
>  		return cxl_region_enable(region);
>  	case ACTION_DISABLE:
>  		return disable_region(region);
> -	case ACTION_DESTROY:
> -		return destroy_region(region);
>  	default:
>  		return -EINVAL;
>  	}
> @@ -1026,7 +1075,12 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
>  			if (!util_cxl_decoder_filter(decoder,
>  						     param.root_decoder))
>  				continue;
> -			rc = decoder_region_action(p, decoder, action, count);
> +
> +			if (action == ACTION_DESTROY)
> +				rc = destroy_multiple_regions(p, decoder, count);
> +			else
> +				rc = decoder_region_action(p, decoder, action, count);
> +
>  			if (rc)
>  				err_rc = rc;
>  		}
> -- 
> 2.45.1.windows.1
> 

[PATCH v2 0/1] cxl/cli: HPA-ordered destroy-region teardown

[Pawel Mielimonka]

This is v2 of the series.

Changes since v1:
 - Rework teardown ordering to account endpoint decoders shared across
   regions under the same bus/port
 - This addresses scenario described by Alison when a memdev is
   targeted by regions under multiple root decoders.

Alison, if you have a chance, could you please retest this version
against your multi-root-decoder configuration to confim that the
teardown now behaves correctly in that scenario?

Pawel Mielimonka (1):
  cxl/cli: enforce HPA-descening teardown order for destroy-region

 cxl/region.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 108 insertions(+), 2 deletions(-)

-- 
2.45.1.windows.1

[PATCH v2 1/1] cxl/cli: enforce HPA-descening teardown order for destroy-region

[Pawel Mielimonka]

Note: This revision collapses the previous 2patch series into a single
patch.

Signed-off-by: Pawel Mielimonka <pawel.mielimonka@fujitsu.com>
---
 cxl/region.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 108 insertions(+), 2 deletions(-)

diff --git a/cxl/region.c b/cxl/region.c
index 207cf2d0..2e0f215d 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -831,6 +831,59 @@ out:
 	return cxl_region_disable(region);
 }
 
+static int cmp_region_hpa(const void *l, const void *r)
+{
+	const struct cxl_region *const *left = l;
+	const struct cxl_region *const *right = r;
+	u64 hpa1 = cxl_region_get_resource((struct cxl_region *) *left);
+	u64 hpa2 = cxl_region_get_resource((struct cxl_region *) *right);
+
+	return (hpa1 > hpa2) - (hpa1 < hpa2);
+}
+
+static int collect_regions_sorted(struct cxl_decoder *root,
+	struct cxl_region ***out,
+	int *out_nr)
+{
+	struct cxl_region *region;
+	struct cxl_region **list = NULL;
+	int nr = 0, alloc = 0;
+
+		struct cxl_port *root_port = cxl_decoder_get_port(root)
+
+		cxl_decoder_foreach(root_port, decoder) {
+		if (!cxl_port_is_root(port))
+			continue;
+			cxl_region_foreach(decoder, region) {
+			if (nr == alloc) {
+				int new_alloc = alloc ? alloc * 2 : 8;
+				size_t new_size = (size_t)new_alloc * sizeof(*list);
+				struct cxl_region **tmp;
+
+				tmp = realloc(list, new_size);
+				if (!tmp) {
+					free(list);
+					return -ENOMEM;
+				}
+				list = tmp;
+				alloc = new_alloc;
+			}
+			list[nr++] = region;
+		}
+
+		if (!nr) {
+			free(list);
+			*out = NULL;
+			*out_nr = 0;
+			return 0;
+		}
+	}
+	qsort(list, nr, sizeof(*list), cmp_region_hpa);
+	*out = list;
+	*out_nr = nr;
+	return 0;
+}
+
 static int destroy_region(struct cxl_region *region)
 {
 	const char *devname = cxl_region_get_devname(region);
@@ -895,6 +948,58 @@ static int destroy_region(struct cxl_region *region)
 	return cxl_region_delete(region);
 }
 
+static int destroy_multiple_regions(struct cxl_ctx *ctx,
+	struct parsed_params *p,
+	int *count)
+{
+	struct cxl_region **list;
+	int nr, rc, i;
+	bool skipped = false;
+
+	rc = collect_regions_sorted(ctx, NULL, &list, &nr);
+	if (rc) {
+		log_err(&rl, "failed to allocate region list: %s\n", strerror(-rc));
+		return rc;
+	}
+
+	for (i = nr - 1; i >= 0; --i) {
+		struct cxl_region *region = NULL;
+
+		for (int j = 0; j < p->argc; j++) {
+			region = util_cxl_region_filter(list[i], p->argv[j]);
+			if (region)
+				break;
+		}
+
+		if (!region) {
+			skipped = true;
+			continue;
+		}
+
+		/*
+		 * If current region matches filter, but previous didn't, destroying would
+		 * result in breaking HPA continuity
+		 */
+		if (skipped) {
+			log_err(&rl, "failed to destroy %s: out of order %s reset\n",
+				cxl_region_get_devname(region),
+				cxl_decoder_get_devname(decoder));
+			rc = -EINVAL;
+			break;
+		}
+
+		rc = destroy_region(region);
+		if (rc) {
+			log_err(&rl, "%s: failed: %s\n",
+				cxl_region_get_devname(region), strerror(-rc));
+			break;
+		}
+		++(*count);
+	}
+	free(list);
+	return rc;
+}
+
 static int do_region_xable(struct cxl_region *region, enum region_actions action)
 {
 	switch (action) {
@@ -902,8 +1007,6 @@ static int do_region_xable(struct cxl_region *region, enum region_actions action
 		return cxl_region_enable(region);
 	case ACTION_DISABLE:
 		return disable_region(region);
-	case ACTION_DESTROY:
-		return destroy_region(region);
 	default:
 		return -EINVAL;
 	}
@@ -956,6 +1059,9 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
 	if (action == ACTION_CREATE)
 		return create_region(ctx, count, p);
 
+	if (action == ACTION_DESTROY)
+		return destroy_multiple_regions(ctx, p, count);
+
 	cxl_bus_foreach(ctx, bus) {
 		struct cxl_decoder *decoder;
 		struct cxl_port *port;
-- 
2.45.1.windows.1

Re: [PATCH v2 0/1] cxl/cli: HPA-ordered destroy-region teardown

[Alison Schofield]

On Tue, Jan 20, 2026 at 11:32:11PM +0900, Pawel Mielimonka wrote:
> This is v2 of the series.
> 
> Changes since v1:
>  - Rework teardown ordering to account endpoint decoders shared across
>    regions under the same bus/port
>  - This addresses scenario described by Alison when a memdev is
>    targeted by regions under multiple root decoders.
> 
> Alison, if you have a chance, could you please retest this version
> against your multi-root-decoder configuration to confim that the
> teardown now behaves correctly in that scenario?

Hi Pawel,

Patch isn't compiling - first error:
../cxl/region.c:854:48: error: ‘decoder’ undeclared (first use in this function); did you mean ‘cxl_decoder’?
  854 |                 cxl_decoder_foreach(root_port, decoder) {
      |                                                ^~~~~~~

For v3, please send the patch as a new thread, not in response to
previous. See here [1] for some ndctl patch sending guidelines, like
labelling as [ndctl PATCH v3] and sending to the nvdimm mailing list.

I think rolling into one patch is fine, and would like to see the
entire previous cover letter info in the commit log of the one
patch. It set this work up very well.

[1] https://github.com/pmem/ndctl?tab=contributing-ov-file#readme

Thanks,
Alison

> 
> Pawel Mielimonka (1):
>   cxl/cli: enforce HPA-descening teardown order for destroy-region
> 
>  cxl/region.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 108 insertions(+), 2 deletions(-)
> 
> -- 
> 2.45.1.windows.1
>