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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] During auto-discovery (CXL_REGION_F_AUTO), passthrough decoders fail to update their `interleave_granularity` software state to the computed value, causing downstream decoders to inherit incorrect granularity and fail validation upon reboot.
--

--- Patch [1]: [PATCH v2 1/2] cxl/region: Allow passthrough decoders with >16K granularity ---
commit 9e2526342188a97e338bfc63d454c253f1e2b3ef
Author: Alison Schofield <alison.schofield@intel.com>

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

This commit allows passthrough decoders beneath wide parent interleaves
to have a granularity exceeding 16K by programming a don't-care value
in hardware for non-interleaving decoders.

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 0c80b76a5f9b4..d164662668c19 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -691,9 +691,16 @@ static void cxld_set_interleave(struct cxl_decoder *cxld, u32 *ctrl)
>  	if (WARN_ONCE(ways_to_eiw(cxld->interleave_ways, &eiw),
>  		      "invalid interleave_ways: %d\n", cxld->interleave_ways))
>  		return;
> -	if (WARN_ONCE(granularity_to_eig(cxld->interleave_granularity, &eig),
> -		      "invalid interleave_granularity: %d\n",
> -		      cxld->interleave_granularity))
> +
> +	/*
> +	 * A non-interleaving decoder ignores the IG field. Encode a
> +	 * don't-care value instead of validating the stored granularity.
> +	 */
> +	if (cxld->interleave_ways == 1)
> +		eig = 0;
> +	else if (WARN_ONCE(granularity_to_eig(cxld->interleave_granularity, &eig),
> +			   "invalid interleave_granularity: %d\n",
> +			   cxld->interleave_granularity))
>  		return;
>  
>  	u32p_replace_bits(ctrl, eig, CXL_HDM_DECODER0_CTRL_IG_MASK);

[ ... ]

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 1e211542b6b64..5a443551288c4 100644
> --- 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,
>  	 * Interleave granularity is a multiple of @parent_port granularity.
>  	 * Multiplier is the parent port interleave ways.
>  	 */
> -	rc = granularity_to_eig(parent_ig * parent_iw, &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;
> -	}
> +	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]
Does this code break region auto-discovery on reboot for topologies
containing a passthrough decoder?

When cxld_set_interleave() configures a passthrough decoder, it now sets
eig = 0 (a don't-care value) in the hardware.

Upon reboot, reading this hardware state will initialize the decoder's
software interleave_granularity to a stale value.

During auto-discovery in cxl_port_setup_targets(), the CXL_REGION_F_AUTO
path skips validating the granularity for passthrough decoders (iw == 1),
but it appears to be missing an update to correct the software state:

	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
		if (cxld->interleave_ways != iw ||
		    (iw > 1 && cxld->interleave_granularity != ig) ||
		    !spa_maps_hpa(p, &cxld->hpa_range) ||
		    ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)) {
			...
			return -ENXIO;
		}
		/* missing update: cxld->interleave_granularity = ig; */
	}

If cxld->interleave_granularity is not updated to the computed ig here, won't
any interleaving descendants read the stale parent_ig, compute a mismatched
expected granularity, and fail validation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783795720.git.alison.schofield@intel.com?part=1