Re: [PATCH v3 1/2] cxl/region: Allow passthrough decoders with >16K granularity

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] During region auto-discovery, a passthrough decoder's interleave granularity is not updated to its correct topological value, causing valid interleaving descendants to fail auto-discovery.
--

--- Patch [1]: [PATCH v3 1/2] cxl/region: Allow passthrough decoders with >16K granularity ---
commit d213bad3e4cb8647e64aafb5eacaa82258aa533b
Author: Alison Schofield <[email protected]>

cxl/region: Allow passthrough decoders with >16K granularity

This commit allows passthrough decoders to have computed granularities larger
than 16K by treating the interleave granularity as a don't-care value for
non-interleaving decoders. It updates the region configuration to skip
validation and hardware encodability checks for these granularities, while
keeping the computed value to seed descendant decoder setup.

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
[ ... ]
> @@ -1549,20 +1557,21 @@ static int cxl_port_setup_targets(struct cxl_port *port,
[ ... ]
> +	ig = parent_ig * parent_iw;
>  
> -	rc = eig_to_granularity(eig, &ig);
> -	if (rc) {
> -		dev_dbg(&cxlr->dev, "%s:%s: invalid interleave: %d\n",
> -			dev_name(port->uport_dev), dev_name(&port->dev),
> -			256 << eig);
> -		return rc;
> +	/*
> +	 * Keep the computed granularity for descendant setup. Only
> +	 * interleaving decoders require an encodable granularity.
> +	 */
> +	if (iw > 1) {
> +		rc = granularity_to_eig(ig, &eig);
> +		if (rc) {
> +			dev_dbg(&cxlr->dev,
> +				"%s: invalid granularity calculation (%d * %d)\n",
> +				dev_name(&parent_port->dev), parent_ig,
> +				parent_iw);
> +			return rc;
> +		}
>  	}
>  
>  	if (iw > 8 || iw > cxlsd->nr_targets) {

[Severity: High]
When auto-discovering a region with a passthrough decoder (iw == 1), does this
skip updating the decoder's interleave granularity to the computed 'ig'?

Looking at cxl_port_setup_targets(), the auto-discovery path handles
non-interleaving decoders by bypassing the granularity mismatch check:

    if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
            if (cxld->interleave_ways != iw ||
                (iw > 1 && cxld->interleave_granularity != ig) || ...

However, because cxld->interleave_granularity is never updated to 'ig' for
auto-discovered passthrough decoders, won't it retain its stale hardware
value (which could be a don't-care value like 0)?

If so, when descendant decoders load their parent's interleave granularity
earlier in cxl_port_setup_targets():

    parent_rr = cxl_rr_load(parent_port, cxlr);
    parent_cxld = parent_rr->decoder;
    parent_ig = parent_cxld->interleave_granularity;

won't they inherit this stale hardware don't-care value? This appears to
cause their own expected 'ig' calculation to fail validation during
auto-discovery, resulting in the region being rejected.

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