Re: [PATCH 3/3] cxl/region: Use calc_interleave_pos() with autodiscovered regions

[Dave Jiang <dave.jiang@intel.com>]

On 10/5/23 17:43, alison.schofield@intel.com wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> For auto-discovered regions, the driver must assign each target to
> the correct position in the region interleave set.
> 
> cxl_region_sort_targets() uses the kernel sort() function to put the
> targets in relative order. Once the relative ordering is complete,
> positions are assigned based on each targets index in that sorted list.
> 
> The sort() compare function does not consider the child offset into a
> parent port. The sort put all targets of one port ahead of another
> port when an interleave was expected, causing the region assembly to
> fail.
> 
> Replace the relative sort, with calc_interleave_pos() on each target
> in the region target list. That will find the exact position for each
> target based on a walk up the ancestral tree from endpoint to root
> decoder.
> 
> calc_interleave_pos() was introduced in a prior patch, so the work
> here is to use in cxl_region_sort_targets().
> 
> Cleanup the obsolete helper functions from the prior sort().
> 
> Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> Reported-by: Dmytro Adamenko <dmytro.adamenko@intel.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/region.c | 127 +++++---------------------------------
>  1 file changed, 15 insertions(+), 112 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 297b9132d5b3..5a4a70ceb4ce 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1480,6 +1480,14 @@ static int cxl_region_attach_auto(struct cxl_region *cxlr,
>  	return 0;
>  }
>  
> +static int cmp_interleave_pos(const void *a, const void *b)
> +{
> +	struct cxl_endpoint_decoder *cxled_a = *(typeof(cxled_a) *)a;
> +	struct cxl_endpoint_decoder *cxled_b = *(typeof(cxled_b) *)b;
> +
> +	return cxled_a->pos - cxled_b->pos;
> +}
> +
>  static struct cxl_port *next_port(struct cxl_port *port)
>  {
>  	if (!port->parent_dport)
> @@ -1587,131 +1595,26 @@ static int calc_interleave_pos(struct cxl_endpoint_decoder *cxled,
>  	return pos;
>  }
>  
> -static void find_positions(const struct cxl_switch_decoder *cxlsd,
> -			   const struct cxl_port *iter_a,
> -			   const struct cxl_port *iter_b, int *a_pos,
> -			   int *b_pos)
> -{
> -	int i;
> -
> -	for (i = 0, *a_pos = -1, *b_pos = -1; i < cxlsd->nr_targets; i++) {
> -		if (cxlsd->target[i] == iter_a->parent_dport)
> -			*a_pos = i;
> -		else if (cxlsd->target[i] == iter_b->parent_dport)
> -			*b_pos = i;
> -		if (*a_pos >= 0 && *b_pos >= 0)
> -			break;
> -	}
> -}
> -
> -static int cmp_decode_pos(const void *a, const void *b)
> -{
> -	struct cxl_endpoint_decoder *cxled_a = *(typeof(cxled_a) *)a;
> -	struct cxl_endpoint_decoder *cxled_b = *(typeof(cxled_b) *)b;
> -	struct cxl_memdev *cxlmd_a = cxled_to_memdev(cxled_a);
> -	struct cxl_memdev *cxlmd_b = cxled_to_memdev(cxled_b);
> -	struct cxl_port *port_a = cxled_to_port(cxled_a);
> -	struct cxl_port *port_b = cxled_to_port(cxled_b);
> -	struct cxl_port *iter_a, *iter_b, *port = NULL;
> -	struct cxl_switch_decoder *cxlsd;
> -	struct device *dev;
> -	int a_pos, b_pos;
> -	unsigned int seq;
> -
> -	/* Exit early if any prior sorting failed */
> -	if (cxled_a->pos < 0 || cxled_b->pos < 0)
> -		return 0;
> -
> -	/*
> -	 * Walk up the hierarchy to find a shared port, find the decoder that
> -	 * maps the range, compare the relative position of those dport
> -	 * mappings.
> -	 */
> -	for (iter_a = port_a; iter_a; iter_a = next_port(iter_a)) {
> -		struct cxl_port *next_a, *next_b;
> -
> -		next_a = next_port(iter_a);
> -		if (!next_a)
> -			break;
> -
> -		for (iter_b = port_b; iter_b; iter_b = next_port(iter_b)) {
> -			next_b = next_port(iter_b);
> -			if (next_a != next_b)
> -				continue;
> -			port = next_a;
> -			break;
> -		}
> -
> -		if (port)
> -			break;
> -	}
> -
> -	if (!port) {
> -		dev_err(cxlmd_a->dev.parent,
> -			"failed to find shared port with %s\n",
> -			dev_name(cxlmd_b->dev.parent));
> -		goto err;
> -	}
> -
> -	dev = device_find_child(&port->dev, &cxled_a->cxld.hpa_range,
> -				match_switch_decoder_by_range);
> -	if (!dev) {
> -		struct range *range = &cxled_a->cxld.hpa_range;
> -
> -		dev_err(port->uport_dev,
> -			"failed to find decoder that maps %#llx-%#llx\n",
> -			range->start, range->end);
> -		goto err;
> -	}
> -
> -	cxlsd = to_cxl_switch_decoder(dev);
> -	do {
> -		seq = read_seqbegin(&cxlsd->target_lock);
> -		find_positions(cxlsd, iter_a, iter_b, &a_pos, &b_pos);
> -	} while (read_seqretry(&cxlsd->target_lock, seq));
> -
> -	put_device(dev);
> -
> -	if (a_pos < 0 || b_pos < 0) {
> -		dev_err(port->uport_dev,
> -			"failed to find shared decoder for %s and %s\n",
> -			dev_name(cxlmd_a->dev.parent),
> -			dev_name(cxlmd_b->dev.parent));
> -		goto err;
> -	}
> -
> -	dev_dbg(port->uport_dev, "%s comes %s %s\n",
> -		dev_name(cxlmd_a->dev.parent),
> -		a_pos - b_pos < 0 ? "before" : "after",
> -		dev_name(cxlmd_b->dev.parent));
> -
> -	return a_pos - b_pos;
> -err:
> -	cxled_a->pos = -1;
> -	return 0;
> -}
> -
>  static int cxl_region_sort_targets(struct cxl_region *cxlr)
>  {
>  	struct cxl_region_params *p = &cxlr->params;
>  	int i, rc = 0;
>  
> -	sort(p->targets, p->nr_targets, sizeof(p->targets[0]), cmp_decode_pos,
> -	     NULL);
> -
>  	for (i = 0; i < p->nr_targets; i++) {
>  		struct cxl_endpoint_decoder *cxled = p->targets[i];
>  
> +		cxled->pos = calc_interleave_pos(cxled, p->interleave_ways);
>  		/*
> -		 * Record that sorting failed, but still continue to restore
> -		 * cxled->pos with its ->targets[] position so that follow-on
> -		 * code paths can reliably do p->targets[cxled->pos] to
> -		 * self-reference their entry.
> +		 * Record that sorting failed, but still continue to calc
> +		 * cxled->pos so that follow-on code paths can reliably
> +		 * do p->targets[cxled->pos] to self-reference their entry.
>  		 */
>  		if (cxled->pos < 0)
>  			rc = -ENXIO;
> -		cxled->pos = i;
>  	}
> +	/* Keep the cxlr target list in interleave position order */
> +	sort(p->targets, p->nr_targets, sizeof(p->targets[0]),
> +	     cmp_interleave_pos, NULL);
>  
>  	dev_dbg(&cxlr->dev, "region sort %s\n", rc ? "failed" : "successful");
>  	return rc;