Re: [PATCH 1/3] cxl/hdm: Reject switch decoder interleave ways that overflow targets

On Mon, Jul 13, 2026 at 09:00:19PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [High] Validation of `interleave_ways` returns a hard error for uncommitted decoders, causing a fatal probe failure if an inactive decoder contains leftover or uninitialized configuration.

Fixed in v2.


> - [High] Due to a preexisting off-by-one error in `nr_targets`, the new check `cxld->interleave_ways > cxlsd->nr_targets` will falsely reject valid max-width interleaved configurations.
> 

Disagree on this one. 
Target Count is not 0-based. Per CXL r4.0, Table 8-116 (CXL HDM Decoder
Capability Register), the Target Count field is a direct count

But...given the confusion w the adjacent decoder_count field I have
added a clarifying comment in v2.


> Pre-existing issues:
> - [Critical] Target list array accesses are endian-dependent, leading to corrupted target mapping on Big-Endian systems.

This is addressed in Patch 2 of the series


> --
> 
> --- Patch [1]: [PATCH 1/3] cxl/hdm: Reject switch decoder interleave ways that overflow targets ---
> commit 0f855df703f711546a94bba79b4742022fb3bae7
> Author: Alison Schofield <[email protected]>
> 
> cxl/hdm: Reject switch decoder interleave ways that overflow targets
> 
> This commit adds validation during CXL switch decoder enumeration to ensure
> that the number of interleave ways does not exceed either the hardware
> target list capacity or the reported target count. If the ways exceed
> these values, enumeration fails with an error to prevent array overflows.
> 
> Link: https://sashiko.dev/#/patchset/[email protected]?part=1
> 
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -1084,6 +1084,24 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >  		cxld->interleave_ways, cxld->interleave_granularity);
> >  
> >  	if (!cxled) {
> > +		struct cxl_switch_decoder *cxlsd =
> > +			to_cxl_switch_decoder(&cxld->dev);
> > +
> > +		if (cxld->interleave_ways > CXL_HDM_DECODER0_TL_TARGETS) {
> 
> [Severity: High]
> Will this return a hard error for uncommitted decoders?
> 
> If an inactive or uncommitted decoder contains uninitialized or leftover
> state (such as from BIOS) in its control register, this bounds check will
> fail and return -ENXIO. 
> 
> Because this validation happens before checking if the decoder is actually
> committed, does this abort the initialization of subsequent decoders and
> prevent the CXL switch port from probing?
> 
> devm_cxl_switch_port_decoders_setup()
>   -> devm_cxl_enumerate_decoders()
>     -> init_hdm_decoder()
> 
> > +			dev_err(&port->dev,
> > +				"decoder%d.%d: interleave ways: %d exceeds target list capacity: %d\n",
> > +				port->id, cxld->id, cxld->interleave_ways,
> > +				CXL_HDM_DECODER0_TL_TARGETS);
> > +			return -ENXIO;
> > +		}
> > +		if (cxld->interleave_ways > cxlsd->nr_targets) {
> 
> [Severity: High]
> Is cxlsd->nr_targets a 0-based count?
> 
> In parse_hdm_decoder_caps(), the 0-based hardware capability "Target Count"
> is assigned to cxlhdm->target_count without adjustment, making
> cxlsd->nr_targets 0-based (where 0 means 1 target, 1 means 2 targets, etc.).
> 
> Since interleave_ways is a 1-based exact count, won't this check incorrectly
> reject valid configurations? 
> 
> For example, a 2-way interleave across 2 targets evaluates as 2 > 1,
> incorrectly returning -ENXIO and preventing CXL memory enumeration on valid
> hardware topologies.
> 
> > +			dev_err(&port->dev,
> > +				"decoder%d.%d: interleave ways: %d exceeds targets: %d\n",
> > +				port->id, cxld->id, cxld->interleave_ways,
> > +				cxlsd->nr_targets);
> > +			return -ENXIO;
> > +		}
> > +
> >  		lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which));
> >  		hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));
> >  		target_list.value = (hi << 32) + lo;
> 
> [Severity: Critical]
> This isn't a bug introduced by this patch, but does assigning
> target_list.value this way cause endian-dependent target mapping on
> Big-Endian systems?
> 
> By reading the lo and hi registers and combining them into a host-endian
> 64-bit integer, accessing it as a byte array (e.g., via
> target_list.target_id[i]) will read the most significant byte on Big-Endian
> architectures instead of the expected byte.
> 
> Could this lead to memory traffic being routed to the wrong CXL target on
> architectures like PowerPC or s390x?
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1