mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] cxl/region: Restore passthrough decoder enable on region re-assembly
@ 2026-07-27 10:37 Richard Cheng
  2026-07-27 20:25 ` Dave Jiang
  0 siblings, 1 reply; 8+ messages in thread
From: Richard Cheng @ 2026-07-27 10:37 UTC (permalink / raw)
  To: dave, jic23, dave.jiang, alison.schofield, vishal.l.verma, djbw,
	danwilliams
  Cc: iweiny, ming.li, gourry, rrichter, linux-cxl, linux-kernel,
	sreddym, smadhavan, kaihengf, kobak, newtonl, kristinc, mochs,
	Richard Cheng

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
device's HDM decoder is still committed and decoding.

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 <icheng@nvidia.com>
---
 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
-- 
2.43.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] cxl/region: Restore passthrough decoder enable on region re-assembly
  2026-07-27 10:37 [PATCH] cxl/region: Restore passthrough decoder enable on region re-assembly Richard Cheng
@ 2026-07-27 20:25 ` Dave Jiang
  2026-07-28  3:03   ` Richard Cheng
  0 siblings, 1 reply; 8+ messages in thread
From: Dave Jiang @ 2026-07-27 20:25 UTC (permalink / raw)
  To: Richard Cheng, dave, jic23, alison.schofield, vishal.l.verma,
	djbw, danwilliams
  Cc: iweiny, ming.li, gourry, rrichter, linux-cxl, linux-kernel,
	sreddym, smadhavan, kaihengf, kobak, newtonl, kristinc, mochs



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

> device's HDM decoder is still committed and decoding.

If we truly do a PCI hot-remove and re-insert, the endpoint device would be reset and disabled. Not sure how it would still be decoding. 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?

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 <icheng@nvidia.com>
> ---
>  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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] cxl/region: Restore passthrough decoder enable on region re-assembly
  2026-07-27 20:25 ` Dave Jiang
@ 2026-07-28  3:03   ` Richard Cheng
  2026-07-28  4:14     ` Gregory Price
  2026-07-28 14:25     ` Dave Jiang
  0 siblings, 2 replies; 8+ messages in thread
From: Richard Cheng @ 2026-07-28  3:03 UTC (permalink / raw)
  To: Dave Jiang
  Cc: dave, jic23, alison.schofield, vishal.l.verma, djbw, danwilliams,
	iweiny, ming.li, gourry, rrichter, linux-cxl, linux-kernel,
	sreddym, smadhavan, kaihengf, kobak, newtonl, kristinc, mochs

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.

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.

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 <icheng@nvidia.com>
> > ---
> >  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
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] cxl/region: Restore passthrough decoder enable on region re-assembly
  2026-07-28  3:03   ` Richard Cheng
@ 2026-07-28  4:14     ` Gregory Price
  2026-07-31  9:13       ` Richard Cheng
  2026-07-28 14:25     ` Dave Jiang
  1 sibling, 1 reply; 8+ messages in thread
From: Gregory Price @ 2026-07-28  4:14 UTC (permalink / raw)
  To: Richard Cheng
  Cc: Dave Jiang, dave, jic23, alison.schofield, vishal.l.verma, djbw,
	danwilliams, iweiny, ming.li, rrichter, linux-cxl, linux-kernel,
	sreddym, smadhavan, kaihengf, kobak, newtonl, kristinc, mochs

On Tue, Jul 28, 2026 at 11:03:37AM +0800, Richard Cheng wrote:
> On Mon, Jul 27, 2026 at 01:25:48PM +0800, Dave Jiang wrote:
> 
> 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.
> 
> 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.
> 

Being a passthrough decoder is just a special state of a switch decoder,
it doesn't necessarily imply programmability (Enable, Commit, Lock all
technically still apply, it's just ambiguous what they should be).

Wonder if we're just not setting these intermediate decoders up sanely.

If the endpoint is locked / not reprogrammable (commit / lock) on the
first go around, maybe the intermediate decoders should be force-locked
and not have F_ENABLE cleared on teardown?  It's not like this actually
changes anything on the hardware, it's just bookkeeping.

(although i will say it's been a bit since i looked at the flag state
machine here, so i could be misremembering what the rules around ENABLE
are i this context).

Curious - what happens if you fully unload cxl_pci between
remove/rescan?

~Gregory

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] cxl/region: Restore passthrough decoder enable on region re-assembly
  2026-07-28  3:03   ` Richard Cheng
  2026-07-28  4:14     ` Gregory Price
@ 2026-07-28 14:25     ` Dave Jiang
  2026-08-05  6:21       ` Richard Cheng
  1 sibling, 1 reply; 8+ messages in thread
From: Dave Jiang @ 2026-07-28 14:25 UTC (permalink / raw)
  To: Richard Cheng
  Cc: dave, jic23, alison.schofield, vishal.l.verma, djbw, iweiny,
	ming.li, gourry, rrichter, linux-cxl, linux-kernel, sreddym,
	smadhavan, kaihengf, kobak, newtonl, kristinc, mochs



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 <icheng@nvidia.com>
>>> ---
>>>  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
>>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] cxl/region: Restore passthrough decoder enable on region re-assembly
  2026-07-28  4:14     ` Gregory Price
@ 2026-07-31  9:13       ` Richard Cheng
  0 siblings, 0 replies; 8+ messages in thread
From: Richard Cheng @ 2026-07-31  9:13 UTC (permalink / raw)
  To: Gregory Price
  Cc: Dave Jiang, dave, jic23, alison.schofield, vishal.l.verma, djbw,
	danwilliams, iweiny, ming.li, rrichter, linux-cxl, linux-kernel,
	sreddym, smadhavan, kaihengf, kobak, newtonl, kristinc, mochs

On Tue, Jul 28, 2026 at 12:14:18AM +0800, Gregory Price wrote:
> On Tue, Jul 28, 2026 at 11:03:37AM +0800, Richard Cheng wrote:
> > On Mon, Jul 27, 2026 at 01:25:48PM +0800, Dave Jiang wrote:
> > 
> > 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.
> > 
> > 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.
> > 
> 
> Being a passthrough decoder is just a special state of a switch decoder,
> it doesn't necessarily imply programmability (Enable, Commit, Lock all
> technically still apply, it's just ambiguous what they should be).
> 
> Wonder if we're just not setting these intermediate decoders up sanely.
> 
> If the endpoint is locked / not reprogrammable (commit / lock) on the
> first go around, maybe the intermediate decoders should be force-locked
> and not have F_ENABLE cleared on teardown?  It's not like this actually
> changes anything on the hardware, it's just bookkeeping.
>

Agree on this part, I think what you are suggesting is a better approach
than what I posted, if the EP is locked an not reprogrammable, the intermediate
decoder describing that path has no business begin torn down either.

It also lines up with Dave's question about checking the EP decoder's lock state
before proceeding.

 
> (although i will say it's been a bit since i looked at the flag state
> machine here, so i could be misremembering what the rules around ENABLE
> are i this context).
> 
> Curious - what happens if you fully unload cxl_pci between
> remove/rescan?
> 

Good qeustion, I'll test that as soon as possible before I sent v2.

Thanks for the pointer !

Best regards,
Richard Cheng.

> ~Gregory

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] cxl/region: Restore passthrough decoder enable on region re-assembly
  2026-07-28 14:25     ` Dave Jiang
@ 2026-08-05  6:21       ` Richard Cheng
  2026-08-05  6:55         ` Gregory Price
  0 siblings, 1 reply; 8+ messages in thread
From: Richard Cheng @ 2026-08-05  6:21 UTC (permalink / raw)
  To: Dave Jiang
  Cc: dave, jic23, alison.schofield, vishal.l.verma, djbw, iweiny,
	ming.li, gourry, rrichter, linux-cxl, linux-kernel, sreddym,
	smadhavan, kaihengf, kobak, newtonl, kristinc, mochs

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 <icheng@nvidia.com>
> >>> ---
> >>>  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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] cxl/region: Restore passthrough decoder enable on region re-assembly
  2026-08-05  6:21       ` Richard Cheng
@ 2026-08-05  6:55         ` Gregory Price
  0 siblings, 0 replies; 8+ messages in thread
From: Gregory Price @ 2026-08-05  6:55 UTC (permalink / raw)
  To: Richard Cheng
  Cc: Dave Jiang, dave, jic23, alison.schofield, vishal.l.verma, djbw,
	iweiny, ming.li, rrichter, linux-cxl, linux-kernel, sreddym,
	smadhavan, kaihengf, kobak, newtonl, kristinc, mochs

On Wed, Aug 05, 2026 at 02:21:05PM +0800, Richard Cheng wrote:
> On Tue, Jul 28, 2026 at 07:25:09AM +0800, Dave Jiang wrote:
> 
> 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.
> 

Makes sense to me, it will look a little funky but auto decoders already
got that funk all over them so it is what it is.  :]

~Gregory

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-08-05  6:55 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-07-27 10:37 [PATCH] cxl/region: Restore passthrough decoder enable on region re-assembly Richard Cheng
2026-07-27 20:25 ` Dave Jiang
2026-07-28  3:03   ` Richard Cheng
2026-07-28  4:14     ` Gregory Price
2026-07-31  9:13       ` Richard Cheng
2026-07-28 14:25     ` Dave Jiang
2026-08-05  6:21       ` Richard Cheng
2026-08-05  6:55         ` Gregory Price

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®