Re: [PATCH 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:

Pre-existing issues:
- [High] Passthrough decoders in CXL_REGION_F_AUTO mode do not update their `interleave_granularity` to the logically computed `ig`, breaking auto-discovery for downstream interleaving decoders.
--

commit e8de72986281000607b505c71ee58ce7b4899cb6
Author: Alison Schofield <alison.schofield@intel.com>
Subject: cxl/region: Allow passthrough decoders with >16K granularity

This commit relaxes granularity validation for non-interleaving passthrough
decoders. It ensures the computed granularity is kept to seed descendant
decoder setups while programming a don't-care encoding when committing
non-interleaving decoders.

> 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

[ ... ]

> @@ -1538,20 +1546,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]
This is a pre-existing issue, but since the commit message mentions keeping
the computed value for passthrough decoders to seed descendant setup, does
the auto-discovery path also need to be updated to ensure this?

During region auto-discovery (CXL_REGION_F_AUTO), cxl_port_setup_targets()
checks the configuration but does not appear to update the decoder's
interleave_granularity for passthrough decoders:

	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)) {
			// ... validation fails ...
			return -ENXIO;
		}
		// cxld->interleave_granularity is not updated to ig here
	} else {
		// ... manual config path updates it:
		cxld->interleave_granularity = ig;
	}

Since cxld->interleave_granularity is never updated to the computed ig in
the auto-discovery block, it retains the hardware don't-care value (such as
256 bytes or 0) read from the register.

When a downstream switch decoder is subsequently evaluated, it loads this
unencodable hardware value as parent_ig. If the downstream decoder is
interleaving (iw > 1), it will calculate a logically incorrect ig from the
garbage parent_ig. This would cause it to fail its own validation and abort
region assembly with -ENXIO.

Should cxld->interleave_granularity be explicitly updated to ig in the
CXL_REGION_F_AUTO path to prevent downstream decoders from inheriting
unencodable don't-care values?

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