Re: [PATCH] cxl/region: Restore passthrough decoder enable on region re-assembly

On Tue, Jul 28, 2026 at 07:25:09AM +0800, Dave Jiang wrote:
> 
> 
> On 7/27/26 8:03 PM, Richard Cheng wrote:
> > On Mon, Jul 27, 2026 at 01:25:48PM +0800, Dave Jiang wrote:
> >>
> >>
> >> On 7/27/26 3:37 AM, Richard Cheng wrote:
> >>> After a CXL endpoint is PCI hot-removed and the bus rescanned, an
> >>> auto-discovered region fails to rebiuld and is lost, evne though the
> >>
> >> rebuild
> >> even
> >>
> > 
> > Hi Dave,
> > Thanks for the review, I'll fix the typo in v2.
> >  
> >>> device's HDM decoder is still committed and decoding.
> >>
> > 
> > I think this part is talking about physical remove/insert ? sorry maybe I commit message was too vague about the scenario.
> > The case here is a SW-only teardown via sysfs, this is what I did.
> > 
> > """
> > $ echo 1 > /sys/bus/pci/devices/$BDF/remove
> > $ echo 1 > /sys/bus/pci/devices/$BDF/rescan
> > """
> > 
> > No physical removal, no power cycle, no link-down, no reset. Linux drops and re-enumerates the same device, which never stopped running.
> > The endpoint HDM decoder is still committed.
> > 
> > The device is byte-identical before and after, the kernel doesn't clear it either, the decoder is locked and cxl_decoder_reset() returns
> > early for CXL_DECODER_F_LOCK before touching any register. The memory keeps decoding.
> 
> Given this is a single target passthrough, should we check the EP decoder for lock before going forward?
> > 
> > I don't think anything needs re-programming here? the only thing lost is kernel-side bookkeeping on the passthrough decoder, which is
> > freed with port and reallocated with F_ENABLE clear on rescan. The endpoint recovers its state from HW.
> > The passthrough decoder has no HW to recover from, that asymmetry is the bug.
> > 
> >> Also, a complete different device with possibly different size can be inserted. And the device showed up would be unconfigured. Given
> >> there's no BIOS to program the device since the OS has taken over, should it be still considered part of the auto-region?
> > 
> > Agreed that would be wrong, and I don't think this patch allows it. A different or freshly-inserted device fails the existing endpoint
> > checks, its own decoder won't come back COMMITTED with a matching HPA range, so cxl_add_to_region() won't re-assemble the auto-region
> > relardless of what the passthrough decoder's flag says. This patch guards on the decode config still matching (iw, ig, spa_maps_hap()),
> > and on !cxld->commit, so it can only ever touch a SW-only stub.
> > 
> > So the coverage here is SW-only, I should state it more clearly in v2.
> > 
> > Btw, the check "!cxld->commit" is doing a lot of implicit work in the code base, I think that's worth fixing regardless of the patch.
> > "commit == NULL" is not unique to passthrough decoders, cxl_setup_hdm_decoder_from_dvsec() also sets "commit = NULL" for DVSEC-emulated
> > RCD endpoint decoders, and root decoder never set it. My call happens to be safe, but that isn't visible from the condition.
> > 
> > I would suggest an inline helper like the following, do you think it's reasonable ?
> > 
> > """
> > static inline bool cxl_decoder_is_passthrough(struct cxl_decoder *cxld)
> > {
> >     if (cxld->commit)
> >         return false;
> >     if (!is_switch_decoder(&cxld->dev) || is_root_decoder(&cxld->dev))
> >         return false;
> >     return to_cxl_switch_decoder(&cxld->dev)->nr_targets <= 1;
> > }
> > """
> > 
> > There're about 4 call site today, and it distinguishes "passthrough" from "has no commit routine" , which are currently the same check
> > for different things.
> > 
> > Happy to send it as a separate patch and make this one a patch series if you prefer the shape.
> 
> Yeah I think if we can improve the clarity that would be a good thing. Thanks Richard!
> 
> DJ
> 
> > 
> > Best regards,
> > Richard Cheng.
> > 
> >> Any thoughts Jonathan?
> >>
> >>>
> >>> A single-dport host bridge/root port has no HDM decoder capability, so
> >>> its switch decoder is a SW-only passthrough. Its CXL_DECODER_F_ENABLE
> >>> flag is cleared on region teardown and never restored on rescan, so
> >>> cxl_port_setup_targets() fails with -ENXIO.
> >>>
> >>> Re-enable the passthrough decoder when its interleave and HPA config
> >>> still match the region, it holds no HW state.
> >>>
> >>> Signed-off-by: Richard Cheng <[email protected]>
> >>> ---
> >>>  drivers/cxl/core/region.c | 13 +++++++++++++
> >>>  1 file changed, 13 insertions(+)
> >>>
> >>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> >>> index 1e211542b6b6..011ceb5bae79 100644
> >>> --- a/drivers/cxl/core/region.c
> >>> +++ b/drivers/cxl/core/region.c
> >>> @@ -1574,6 +1574,19 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> >>>  	}
> >>>  
> >>>  	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> >>> +		/*
> >>> +		 * A passthrough switch decoder holds no HW decode state.
> >>> +		 * It's CXL_DECODER_F_ENABLE flag is pure software bookkeeping
> >>> +		 * that is cleared when the region is torn down. On auto-discovery
> >>> +		 * re-assembly after a subsequent rescan the decode config still
> >>> +		 * matches the region, so restore the flag rather than fail to
> >>> +		 * rebuild a region that HW is in fact still decoding.
> >>> +		 */
> >>> +		if (!cxld->commit && cxld->interleave_ways == iw &&
> >>> +		    (iw <= 1 || cxld->interleave_granularity == ig) &&
> >>> +		    spa_maps_hpa(p, &cxld->hpa_range))
> >>> +			cxld->flags |= CXL_DECODER_F_ENABLE;
> >>> +
> >>>  		if (cxld->interleave_ways != iw ||
> >>>  		    (iw > 1 && cxld->interleave_granularity != ig) ||
> >>>  		    !spa_maps_hpa(p, &cxld->hpa_range) ||
> >>>
> >>> base-commit: 4539944e515183668109bdf4d0c3d7d228383d88
> >>
>

So I re-tested those operations on v7.0-rc2 kernel without the patch, we find out

0a70b7cd397e ("cxl: Test CXL_DECODER_F_LOCK as a bitmask")

This patch is not needed, and Gregory is right that intermediate decoders should stay locked and
not have F_ENABLE cleared on teardown.

For v2 I'll only send the cxl_decoder_is_passthrough() helper that Dave asked for.
It's just for clarity and no behavior change. This mask v.s. index mistake already caused one bug,
so a small helper that hides the raw flag from callers seems useful.

Best regards,
Richard Cheng.