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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The patch fails to reset `interleave_ways` for uncommitted endpoint decoders, contradicting its stated intent.

Pre-existing issues:
- [High] Reading the switch decoder target list via a union results in incorrect target IDs on big-endian architectures.
- [High] Unconditional parsing of interleave properties aborts initialization for uncommitted decoders with uninitialized registers.
--

--- Patch [1]: [PATCH v4 1/4] cxl/hdm: Reject switch decoder interleave ways that overflow targets ---
commit 6b69d832bd669e734938484bbf51cd067e822152
Author: Alison Schofield <[email protected]>

cxl/hdm: Reject switch decoder interleave ways that overflow targets

This commit validates that a switch decoder's interleave ways fit
within available targets and the hardware capacity. Uncommitted
decoders have their interleave ways ignored and reset to one,
and decoders reporting zero targets are rejected.

Link: https://sashiko.dev/#/patchset/[email protected]?part=1

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 0c80b76a5f9b4..d81df45d8005d 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c

[ ... ]

> @@ -1084,6 +1086,37 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  		cxld->interleave_ways, cxld->interleave_granularity);

[Severity: High]
This is a pre-existing issue, but does unconditional parsing of interleave
properties abort initialization for uncommitted decoders?

If the control register of an uncommitted decoder contains an invalid
interleave value, eiw_to_ways() or eig_to_granularity() returns -EINVAL.
This fails init_hdm_decoder() and aborts port enumeration before we can
check if the decoder is committed.

>  	if (!cxled) {
> +		struct cxl_switch_decoder *cxlsd =
> +			to_cxl_switch_decoder(&cxld->dev);
> +
> +		if (cxlsd->nr_targets < 1) {
> +			dev_err(&port->dev,
> +				"decoder%d.%d: reports zero targets\n",
> +				port->id, cxld->id);
> +			return -ENXIO;
> +		}
> +
> +		if (!committed) {
> +			/* Ignore interleave ways until commit */
> +			cxld->interleave_ways = 1;
> +			return 0;
> +		}

[Severity: Medium]
Since this reset logic is inside the if (!cxled) block, does it incorrectly
skip endpoint decoders?

For endpoint decoders, cxled is non-NULL, so the execution bypasses the
if (!cxled) block and won't reset the value. This seems to contradict the
commit message's intent to reset interleave ways for uncommitted decoders.

> +
> +		if (cxld->interleave_ways > CXL_HDM_DECODER0_TL_TARGETS) {
> +			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) {
> +			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: High]
This is a pre-existing issue, but does reading the switch decoder target
list via a union result in incorrect target IDs on big-endian architectures?

The logic combines 32-bit CPU-endian integers into a 64-bit CPU-endian value:

    target_list.value = (hi << 32) + lo;

On big-endian systems, the highest byte of this integer is stored at index 0.
A subsequent loop reading target_id[0] and assigning it to target_map[0]
would entirely reverse the CXL target map.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1