On Wed, Mar 11, 2026 at 02:37:46PM -0700, Dan Williams wrote:
> Smita Koralahalli wrote:
> > __cxl_decoder_detach() currently resets decoder programming whenever a
> > region is detached if cxl_config_state is beyond CXL_CONFIG_ACTIVE. For
> > autodiscovered regions, this can incorrectly tear down decoder state
> > that may be relied upon by other consumers or by subsequent ownership
> > decisions.
> >
> > Skip cxl_region_decode_reset() during detach when CXL_REGION_F_AUTO is
> > set.
> >
> > Signed-off-by: Smita Koralahalli <[email protected]>
> > Reviewed-by: Jonathan Cameron <[email protected]>
> > Reviewed-by: Dave Jiang <[email protected]>
> > Reviewed-by: Alejandro Lucero <[email protected]>
> > ---
> > drivers/cxl/core/region.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index ae899f68551f..45ee598daf95 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -2178,7 +2178,9 @@ __cxl_decoder_detach(struct cxl_region *cxlr,
> > cxled->part = -1;
> >
> > if (p->state > CXL_CONFIG_ACTIVE) {
> > - cxl_region_decode_reset(cxlr, p->interleave_ways);
> > + if (!test_bit(CXL_REGION_F_AUTO, &cxlr->flags))
> > + cxl_region_decode_reset(cxlr, p->interleave_ways);
> > +
> > p->state = CXL_CONFIG_ACTIVE;
>
Hi Dan,
> tl;dr: I do not think we need this, I do think we need to clarify to
> users what enable/disable and/or hot remove violence is handled and not
> handled by the CXL core.
I'm chiming in here because although this patch is no longer needed for
this series, it has become a dependency for the Type 2 series. So this
follow-up focuses on the hot-remove, endpoint-detach case where
preserving decoders across detach is still needed for later recovery.
Some inline responses to you, and then a diff is appended for a
direction check.
> So this looks deceptively simple, but I think it is incomplete or at
> least adds to the current confusion. A couple points to consider:
>
> 1/ There is no corresponding clear_bit(CXL_REGION_F_AUTO, ...) anywhere in
> the driver. Yes, admin can still force cxl_region_decode_reset() via
> commit_store() path, but admin can not force
> cxl_region_teardown_targets() in the __cxl_decoder_detach() path. I do
> not like that this causes us to end up with 2 separate considerations
> for when __cxl_decoder_detach() skips cleanup actions
> (cxl_region_teardown_targets() and cxl_region_decode_reset()). See
> below, I think the cxl_region_teardown_targets() check is probably
> bogus.
Rather than repurposing CXL_REGION_F_AUTO, this splits decode-reset policy
from AUTO. A new region-scoped CXL_REGION_F_PRESERVE_DECODE flag is introduced
and cleared on explicit decommit in commit_store(). AUTO remains origin/assembly
state.
This does still leave two cleanup decisions:
1) decode reset (now keyed off PRESERVE_DECODE)
2) target teardown (still using existing AUTO behavior)
No change to cxl_region_teardown_targets() in this step.
>
> At a minimum I think commit_store() should clear CXL_REGION_F_AUTO on
> decommit such that cleaning up decoders and targets later proceeds as
> expected.
This point is addressed by clearing CXL_REGION_F_PRESERVE_DECODE instead.
Explicit decommit is treated as destructive and disables decode preservation
before unbind/reset.
>
> 2/ The hard part about CXL region cleanup is that it needs to be prepared
> for:
>
> a/ user manually removes the region via sysfs
>
> b/ user manually disables cxl_port, cxl_mem, or cxl_acpi causing the
> endpoint port to be removed
>
> c/ user physically removes the memdev causing the endpoint port to be
> removed (CXL core can not tell the difference with 2b/ it just sees
> cxl_mem_driver::remove() operation invocation)
>
> d/ setup action fails and region setup is unwound
Agreed. This change targets 2b, 2c.
>
> The cxl_region_decode_reset() is in __cxl_decoder_detach() because of
> 2b/ and 2c/. No other chance to cleanup the decode topology once the
> endpoint decoders are on their way out of the system.
Agreed. The reset remains. Proposed change only makes it conditional on
explicit region policy rather than AUTO.
>
> In this case though the patch was generated back when we were committed
> to cleaning up failed to assemble regions, a new 2d/ case, right?
> However, in that case the decoder is not leaving the system. The
> questions that arrive from that analysis are:
>
> * Is this patch still needed now that there is no auto-cleanup?
Not for this Soft Reserved series, but yes for Type2 hotplug.
>
> * If this patch is still needed is it better to skip
> cxl_region_decode_reset() based on the 'enum cxl_detach_mode' rather
> than the CXL_REGION_F_AUTO flag? I.e. skip reset in the 2d/ case, or
> some other new general flag that says "please preserve hardware
> configuration".
I looked at using and expanding the cxl_detach_mode enum and rejected as
not the right scope. The current detach mode is attached to an individual
detach operation, whereas preserve vs reset decision applies to the region
decode topology as a whole. If we expand detach mode for this region
wide policy, then may risk inconsistent handling across endpoint of the
same region. Just seemed wrong place. I could be missing another reason
why you looked at it.
>
> * Should cxl_region_teardown_targets() also be caring about the
> cxl_detach_mode rather than the auto flag? I actually think the
> CXL_REGION_F_AUTO check in cxl_region_teardown_targets() is misplaced
> and it was confusing "teardown targets" with "decode reset".
Agreed that this is a separate question and didn't touch that here.
>
> All of this wants some documentation to tell users that the rule is
> "Hey, after any endpoint decoder has been seen by the CXL core, if you
> remove that endpoint decoder by removing or disabling any of cxl_acpi,
> cxl_mem, or cxl_port the CXL core *will* violently destroy the decode
> configuration". Then think about whether this needs a way to specify
> "skip decoder teardown" to disambiguate "the decoder is disappearing
> logically, but not physically, keep its configuration". That allows
> turning any manual configuration into an auto-configuration and has an
> explicit rule for all regions rather than the current "auto regions are
> special" policy.
>
> It is helpful that violence has been the default so far. So it allows to
> introduce a decoder shutdown policy toggle where CXL_REGION_F_AUTO flags
> decoders as "preserve" by default. Region decommit clears that flag,
> and/or userspace can toggle that per endpoint decoder flag to determine
> what happens when decoders leave the system. That probably also wants
> some lockdown interaction such that root can not force unplug memory by
> unbinding a driver.
As a step in the direction you suggest, AND aiming to address Type2
need, here is what I'd like a direction check on:
Start separating decode-reset policy rom CXL_REGION_F_AUTO:
- keep CXL_REGION_F_AUTO as origin / assembly semantics
- introduce CXL_REGION_F_PRESERVE_DECODE as a region-scoped policy
- initialize that policy from auto-assembly
- clear it on explicit decommit in commit_store()
- use it to gate cxl_region_decode_reset() in __cxl_decoder_detach()
The decode-reset decision is factored through a small helper,
cxl_region_preserve_decode(), so the policy can be extended independent
of the detach mechanics. Maybe overkill in this simple case, but I
wanted to acknowledge the 'policy' direction.
Compiled but not yet tested, pending a direction check:
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 42874948b589..f99e4aca72f0 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -432,6 +432,12 @@ static ssize_t commit_store(struct device *dev, struct
device_attribute *attr,
if (rc)
return rc;
+ /*
+ * Explicit decommit is destructive. Clear preserve bit before
+ * unbinding so detach paths do not skip decoder reset.
+ */
+ clear_bit(CXL_REGION_F_PRESERVE_DECODE, &cxlr->flags);
+
/*
* Unmap the region and depend the reset-pending state to ensure
* it does not go active again until post reset
@@ -2153,6 +2159,12 @@ static int cxl_region_attach(struct cxl_region *cxlr,
return 0;
}
+/* Region-scoped policy for preserving decoder programming across detach */
+static bool cxl_region_preserve_decode(struct cxl_region *cxlr)
+{
+ return test_bit(CXL_REGION_F_PRESERVE_DECODE, &cxlr->flags);
+}
+
static struct cxl_region *
__cxl_decoder_detach(struct cxl_region *cxlr,
struct cxl_endpoint_decoder *cxled, int pos,
@@ -2185,7 +2197,8 @@ __cxl_decoder_detach(struct cxl_region *cxlr,
cxled->part = -1;
if (p->state > CXL_CONFIG_ACTIVE) {
- cxl_region_decode_reset(cxlr, p->interleave_ways);
+ if (!cxl_region_preserve_decode(cxlr))
+ cxl_region_decode_reset(cxlr, p->interleave_ways);
p->state = CXL_CONFIG_ACTIVE;
}
@@ -3833,6 +3846,7 @@ static int __construct_region(struct cxl_region *cxlr,
}
set_bit(CXL_REGION_F_AUTO, &cxlr->flags);
+ set_bit(CXL_REGION_F_PRESERVE_DECODE, &cxlr->flags);
cxlr->hpa_range = *hpa_range;
res = kmalloc_obj(*res);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 9b947286eb9b..e6fbbee37252 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -532,6 +532,16 @@ enum cxl_partition_mode {
*/
#define CXL_REGION_F_NORMALIZED_ADDRESSING 3
+/*
+ * Indicate that decoder programming should be preserved when endpoint
+ * decoders detach from this region. This allows region decode state to
+ * survive endpoint removal and be recovered by subsequent enumeration.
+ * Automatic assembly may set this flag, and future userspace control
+ * may allow it to be set explicitly. Explicit region decommit should
+ * clear this flag before destructive cleanup.
+ */
+#define CXL_REGION_F_PRESERVE_DECODE 4
+
/**
* struct cxl_region - CXL region
* @dev: This region's device
