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

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

Pre-existing issues:
- [High] Extracting target IDs from the target list register assumes little-endian byte ordering, causing incorrect target mapping on big-endian architectures.
- [High] Failure during decoder initialization leaves `port->commit_end` in a dirty state, permanently breaking subsequent device probe attempts.
--

--- Patch [1]: [PATCH v2 1/3] cxl/hdm: Reject switch decoder interleave ways that overflow targets ---
commit 4660ad9cd44e7b73b9a24a494a4fb73e7c2f2eaa
Author: Alison Schofield <[email protected]>

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

Switch decoder enumeration validates that the interleave ways encoding
is legal, but not that the resulting number of ways fits the available
targets. This can overrun the target arrays during enumeration.

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..9f005f3193e2f 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c

[ ... ]

> @@ -1084,6 +1086,30 @@ 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 (!committed) {
> +			/* Ignore interleave ways until commit */
> +			cxld->interleave_ways = 1;
> +			return 0;
> +		}
> +
> +		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;
               ^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but will this early return leave
port->commit_end permanently modified?

Looking at init_hdm_decoder() earlier in the function, port->commit_end is
updated when the decoder is committed:

	if (committed) {
		...
		port->commit_end = cxld->id;
	}

If the initialization fails here due to exceeding hardware capacity, it returns
-ENXIO and aborts the port probe. However, port->commit_end is never reverted.

If the driver is re-probed, the next initialization attempt will falsely reject
the first decoder as 'Committed out of order'. This happens because
cxl_num_decoders_committed() uses the leaked commit_end state.

Should this error path revert port->commit_end before returning?

> +		}
> +
>  		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 extracting target IDs from the target
list register using a union byte array overlay cause incorrect target mapping
on big-endian architectures?

When init_hdm_decoder() reads hardware registers TL_LOW and TL_HIGH, it
converts them to native CPU endianness via readl() and stores them in a native
64-bit integer.

The function then extracts target IDs using target_list.target_id[i]. On
big-endian systems, byte 0 is the most significant byte of the 64-bit value,
rather than the least significant byte.

This could route CXL transactions to the wrong switch ports.

Does this logic need to be adjusted to avoid endianness issues?

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