Messages in this thread Patch in this message |  | | Date | Tue, 2 Dec 2025 20:15:34 -0800 | | From | Alison Schofield <> | | Subject | Re: [RFC PATCH v1 2/2] cxl/cli: enforce HPA-descending teardown order for destroy-region |
| |
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 >
|  |