Re: [PATCH v4 3/6] cxl/region: Support mixed-granularity auto regions

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect distance calculation for non-mixed-granularity regions with non-power-of-2 root interleaves causes a NULL pointer dereference.
--

--- Patch [3]: [PATCH v4 3/6] cxl/region: Support mixed-granularity auto regions ---
commit 13bc48fe2efef1744b5bbf29c756bd07f400ebe3
Author: Alison Schofield <[email protected]>

cxl/region: Support mixed-granularity auto regions

The CXL driver currently assumes the interleaving root has the same
granularity as the region, with each successive interleaving level
becoming coarser toward the endpoints. This patch allows the CXL driver
to assemble an auto region where the root granularity is coarser than
the region granularity.

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 4f367feaf6c84..637d1b60a570b 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
[ ... ]
> @@ -1463,26 +1472,21 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  	}
>  
>  	cxlsd = to_cxl_switch_decoder(&cxld->dev);
> +	iw = cxl_rr->nr_targets;
> +
>  	if (cxl_rr->nr_targets_set) {
> -		int i, distance = 1;
> -		struct cxl_region_ref *cxl_rr_iter;
> +		int i, distance;
>  
>  		/*
> -		 * The "distance" between peer downstream ports represents which
> -		 * endpoint positions in the region interleave a given port can
> -		 * host.
> -		 *
> -		 * For example, at the root of a hierarchy the distance is
> -		 * always 1 as every index targets a different host-bridge. At
> -		 * each subsequent switch level those ports map every Nth region
> -		 * position where N is the width of the switch == distance.
> +		 * @distance is the spacing between region positions sharing
> +		 * this dport. Mixed-granularity regions place those positions
> +		 * contiguously.
>  		 */
> -		do {
> -			cxl_rr_iter = cxl_rr_load(iter, cxlr);
> -			distance *= cxl_rr_iter->nr_targets;
> -			iter = to_cxl_port(iter->dev.parent);
> -		} while (!is_cxl_root(iter));
> -		distance *= cxlrd->cxlsd.cxld.interleave_ways;
> +		if (cxl_region_is_mixed_gran(cxlr))
> +			distance = 1;
> +		else
> +			distance = cxld->interleave_granularity * iw /
> +				   p->interleave_granularity;

[Severity: High]
Does this mathematical distance calculation correctly handle non-power-of-2
root interleaves?

When configuring a CXL region with a non-power-of-2 root interleave (like
3-way or 6-way) and multiple endpoints sharing a downstream port, this
formula seems to calculate a distance 3 times smaller than the true
topological stride. This appears to happen because parent_iw is divided
by 3 for address routing, which scales down cxld->interleave_granularity.

If the distance is undersized, check_last_peer() will check an endpoint on
a completely different host bridge:

drivers/cxl/core/region.c:check_last_peer() {
        ...
        cxled_peer = p->targets[pos - distance];
        cxlmd_peer = cxled_to_memdev(cxled_peer);
        ep_peer = cxl_ep_load(port, cxlmd_peer);
        if (ep->dport != ep_peer->dport) {
        ...
}

Since cxl_ep_load() returns NULL when selecting a peer on a different host
bridge, will ep_peer->dport trigger a NULL pointer dereference here?

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