diff mbox series

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

Message ID aa43b687f652cdb648dfba967dae9c1fecef3155.1696550786.git.alison.schofield@intel.com
State Superseded
Headers show
Series cxl/region: Autodiscovery position repair | expand

Commit Message

Alison Schofield Oct. 6, 2023, 12:43 a.m. UTC 
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>
---
 drivers/cxl/core/region.c | 127 +++++---------------------------------
 1 file changed, 15 insertions(+), 112 deletions(-)

Comments

Dave Jiang Oct. 13, 2023, 4:48 p.m. UTC | #1 
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;
diff mbox series

Patch

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;