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

[Alison Schofield <alison.schofield@intel.com>]

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
>