[PATCH v4 0/3] cxl/region: Autodiscovery position repair

[PATCH v4 0/3] cxl/region: Autodiscovery position repair

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Changes in v4:
- Remove useless kernel doc comments on find_pos_and_ways() (Dan)
  That also resolves the missing prototype warning from 0-day.
- v3: https://lore.kernel.org/linux-cxl/cover.1698254338.git.alison.schofield@intel.com/

Changes in v3:
- Collapse the local position initialization into the iterative loop. (Jim)
- Match on exact resource range when looking up a switch decoder. (Jim, Dan)
- Update changelogs comments & remove extra fixes & reported-by tags (Dan)
- Add kernel doc to calc_interleave_pos() and find_pos_and_ways() (Dan)
- Add cxl_ prefix to calc_interleave_pos() and find_pos_and_ways()
- Expand in code comments at the dev_dbg test of cxl_calc_interleave_pos()
- Reword the dev_dbg message at test of cxl_calc_interleave_pos()
- Remove stray brace in Patch 1/3 (Jim)
- Init rc to -ENXIO, not -1 (Dan)
- Remove the skip logic (was index logic in v1)
  The skip logic was based on my misunderstanding of valid configs.
  Once I understood that that targets cannot repeat in a decoder list,
  ie a CFMWS of {0 1 0 1} allowing two hosts bridges to appear twice
  in a root decoder was garbage, the prior skip logic and related
  shenanigans were removed.
- v2: https://lore.kernel.org/linux-cxl/cover.1697433770.git.alison.schofield@intel.com/

Changes in v2:
- Use a 'skip', which is a number of siblings to skip over, rather than
  an 'index' when finding a child's position in a parent interleave.
- Tidy up commit messages for clarity and grammar. (DaveJ)
- Update this cover letter with added testing configs that led to the
  'skip' change in the calculation.
- v1: https://lore.kernel.org/linux-cxl/cover.1696550786.git.alison.schofield@intel.com/


Begin original cover letter:

Some region configurations fail to assemble through the auto-discovered
region path. These are valid region configurations that can be assembled
correctly if presented as user defined regions.

The difference being that user defined regions arrive at the driver
with their targets in interleave order, whereas with autodiscovered
regions, the driver needs to assign each target in the interleave
set a correct position. And, in some cases, that fails.

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 the sorted list.

That relative sort doesn't consider the offset of a port into its
parent port. In the failure case, a 2 + 2 config (2 host bridges each
with 2 endpoints), this causes the sort to put all targets of one port
ahead of another port, when they were expected to be interleaved.

While examining the problem and weighing the option of repairing the
existing sort algorithm with assigning positions another way, I chose
the latter. Each endpoint can be examined individually to discover its
position in the region interleave.

The presentation of this patchset was a challenge. While the changes
are essentially a replacement, the resulting diff is horrible. (I did 
try multiple git diff algs). So after a small preparation patch (Patch 1),
it's presented like this:

Patch 2:The new method, cxl_calc_interleave_pos(), is introduced and used
	in a dev_dbg() exercise on user defined regions.

Patch 3:cxl_calc_interleave_pos() replaces the relative sort() in
	cxl_region_sort_targets() for auto-discoverd regions
	and the now obsolete sort helpers are removed.

The only function that seems useful for a side by side diff viewing
is cxl_region_sort_targets() and it is visible in Patch 3.

Testing passes on pre-production hardware with BIOS defined regions
that natively trigger this autodiscovery path of the region driver.
Testing passes a CXL unit test using the dev_dbg() calculation test
(see cxl_region_attach()) across an expanded set of region configs:
1, 1, 1+1, 1+1+1, 2, 2+2, 2+2+2, 2+2+2+2, 4, 4+4, where each number
represents the count of endpoints per host bridge.

Alison Schofield (3):
  cxl/region: Prepare the decoder match range helper for reuse
  cxl/region: Calculate a target position in a region interleave
  cxl/region: Use cxl_calc_interleave_pos() for auto-discovery

 drivers/cxl/core/region.c | 222 +++++++++++++++++++++-----------------
 1 file changed, 125 insertions(+), 97 deletions(-)


base-commit: 8a749fd1a8720d4619c91c8b6e7528c0a355c0aa
-- 
2.37.3

[PATCH v4 1/3] cxl/region: Prepare the decoder match range helper for reuse

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

match_decoder_by_range() and decoder_match_range() both determine
if an HPA range matches a decoder. The first does it for root
decoders and the second one operates on switch decoders.

Tidy these up with clear naming and make the switch helper more
like the root decoder helper in style and functionality. Make it
take the actual range, rather than an endpoint decoder from which
it extracts the range. Require an exact match on switch decoders,
because unlike a root decoder that maps an entire region, Linux
only supports 1:1 mapping of switch to endpoint decoders.

Aside from aesthetics and maintainability, this is in preparation
for reuse.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Jim Harris <jim.harris@samsung.com>
---
 drivers/cxl/core/region.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 6d63b8798c29..eea8e89a6860 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1487,16 +1487,18 @@ static struct cxl_port *next_port(struct cxl_port *port)
 	return port->parent_dport->port;
 }
 
-static int decoder_match_range(struct device *dev, void *data)
+static int match_switch_decoder_by_range(struct device *dev, void *data)
 {
-	struct cxl_endpoint_decoder *cxled = data;
 	struct cxl_switch_decoder *cxlsd;
+	struct range *r1, *r2 = data;
 
 	if (!is_switch_decoder(dev))
 		return 0;
 
 	cxlsd = to_cxl_switch_decoder(dev);
-	return range_contains(&cxlsd->cxld.hpa_range, &cxled->cxld.hpa_range);
+	r1 = &cxlsd->cxld.hpa_range;
+
+	return (r1->start == r2->start && r1->end == r2->end);
 }
 
 static void find_positions(const struct cxl_switch_decoder *cxlsd,
@@ -1565,7 +1567,8 @@ static int cmp_decode_pos(const void *a, const void *b)
 		goto err;
 	}
 
-	dev = device_find_child(&port->dev, cxled_a, decoder_match_range);
+	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;
 
@@ -2696,7 +2699,7 @@ static int devm_cxl_add_dax_region(struct cxl_region *cxlr)
 	return rc;
 }
 
-static int match_decoder_by_range(struct device *dev, void *data)
+static int match_root_decoder_by_range(struct device *dev, void *data)
 {
 	struct range *r1, *r2 = data;
 	struct cxl_root_decoder *cxlrd;
@@ -2827,7 +2830,7 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
 	int rc;
 
 	cxlrd_dev = device_find_child(&root->dev, &cxld->hpa_range,
-				      match_decoder_by_range);
+				      match_root_decoder_by_range);
 	if (!cxlrd_dev) {
 		dev_err(cxlmd->dev.parent,
 			"%s:%s no CXL window for range %#llx:%#llx\n",
-- 
2.37.3

[PATCH v4 2/3] cxl/region: Calculate a target position in a region interleave

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Introduce a calculation to find a target's position in a region
interleave. Perform a self-test of the calculation on user-defined
regions.

The region driver uses the kernel sort() function to put region
targets in relative order. Positions are assigned based on each
target's index in that sorted list. That relative sort doesn't
consider the offset of a port into its parent port which causes
some auto-discovered regions to fail creation. In one failure case,
a 2 + 2 config (2 host bridges each with 2 endpoints), the sort
puts all the targets of one port ahead of another port when they
were expected to be interleaved.

In preparation for repairing the autodiscovery region assembly,
introduce a new method for discovering a target position in the
region interleave.

cxl_calc_interleave_pos() adds a method to find the target position by
ascending from an endpoint to a root decoder. The calculation starts
with the endpoint's local position and position in the parent port. It
traverses towards the root decoder and examines both position and ways
in order to allow the position to be refined all the way to the root
decoder.

This calculation: position = position * parent_ways + parent_pos;
applied iteratively yields the correct position.

Include a self-test that exercises this new position calculation against
every successfully configured user-defined region.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/region.c | 122 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index eea8e89a6860..481aab3234bf 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1501,6 +1501,108 @@ static int match_switch_decoder_by_range(struct device *dev, void *data)
 	return (r1->start == r2->start && r1->end == r2->end);
 }
 
+static int find_pos_and_ways(struct cxl_port *port, struct range *range,
+			     int *pos, int *ways)
+{
+	struct cxl_switch_decoder *cxlsd;
+	struct cxl_port *parent;
+	struct device *dev;
+	int rc = -ENXIO;
+
+	parent = next_port(port);
+	if (!parent)
+		return rc;
+
+	dev = device_find_child(&parent->dev, range,
+				match_switch_decoder_by_range);
+	if (!dev) {
+		dev_err(port->uport_dev,
+			"failed to find decoder mapping %#llx-%#llx\n",
+			range->start, range->end);
+		return rc;
+	}
+	cxlsd = to_cxl_switch_decoder(dev);
+	*ways = cxlsd->cxld.interleave_ways;
+
+	for (int i = 0; i < *ways; i++) {
+		if (cxlsd->target[i] == port->parent_dport) {
+			*pos = i;
+			rc = 0;
+			break;
+		}
+	}
+	put_device(dev);
+
+	return rc;
+}
+
+/**
+ * cxl_calc_interleave_pos() - calculate an endpoint position in a region
+ * @cxled: the endpoint decoder
+ *
+ * The endpoint position is calculated by traversing from the endpoint to
+ * the root decoder and iteratively applying this calculation:
+ *	position = position * parent_ways + parent_pos;
+ *
+ * For example, the expected interleave order of the 4-way region shown
+ * below is: mem0, mem2, mem1, mem3
+ *
+ *		  root_port
+ *                 /      \
+ *      host_bridge_0    host_bridge_1
+ *        |    |           |    |
+ *       mem0 mem1        mem2 mem3
+ *
+ * In the example the calculator will iterate twice. The first iteration
+ * uses the mem position in the host-bridge and the ways of the host-
+ * bridge to generate the first, or local, position. The second iteration
+ * uses the host-bridge position in the root_port and the ways of the
+ * root_port to refine the position.
+ *
+ * A trace of the calculation per endpoint looks like this:
+ * mem0:	pos = 0 * 2 + 0		mem2:	pos = 0 * 2 + 0
+ *		pos = 0 * 2 + 0			pos = 0 * 2 + 1
+ *		pos: 0				pos: 1
+ *
+ * mem1:	pos = 0 * 2 + 1		mem3:	pos = 0 * 2 + 1
+ *		pos = 1 * 2 + 0			pos = 1 * 2 + 1
+ *		pos: 2				pos = 3
+ *
+ * Note that while this example is simple, the method applies to more
+ * complex topologies, including those with switches.
+ *
+ * Return: position >= 0 on success
+ *	   -ENXIO on failure
+ */
+
+static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
+{
+	struct cxl_port *iter, *port = cxled_to_port(cxled);
+	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	struct range *range = &cxled->cxld.hpa_range;
+	int parent_ways = 0, parent_pos = 0, pos = 0;
+	int rc;
+
+	/* Iterate from endpoint to root_port refining the position */
+	for (iter = port; iter; iter = next_port(iter)) {
+		if (is_cxl_root(iter))
+			break;
+
+		rc = find_pos_and_ways(iter, range, &parent_pos, &parent_ways);
+		if (rc)
+			return rc;
+
+		pos = pos * parent_ways + parent_pos;
+	}
+
+	dev_dbg(&cxlmd->dev,
+		"decoder:%s parent:%s port:%s range:%#llx-%#llx pos:%d\n",
+		dev_name(&cxled->cxld.dev), dev_name(cxlmd->dev.parent),
+		dev_name(&port->dev), range->start, range->end, pos);
+
+	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,
@@ -1764,6 +1866,26 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 		.end = p->res->end,
 	};
 
+	if (p->nr_targets != p->interleave_ways)
+		return 0;
+
+	/*
+	 * Test the auto-discovery position calculator function
+	 * against this successfully created user-defined region.
+	 * A fail message here means that this interleave config
+	 * will fail when presented as CXL_REGION_F_AUTO.
+	 */
+	for (int i = 0; i < p->nr_targets; i++) {
+		struct cxl_endpoint_decoder *cxled = p->targets[i];
+		int test_pos;
+
+		test_pos = cxl_calc_interleave_pos(cxled);
+		dev_dbg(&cxled->cxld.dev,
+			"Test cxl_calc_interleave_pos(): %s test_pos:%d cxled->pos:%d\n",
+			(test_pos == cxled->pos) ? "success" : "fail",
+			test_pos, cxled->pos);
+	}
+
 	return 0;
 
 err_decrement:
-- 
2.37.3

[PATCH v4 3/3] cxl/region: Use cxl_calc_interleave_pos() for auto-discovery

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

For auto-discovered regions the driver must assign each target to
a valid position in the region interleave set based on the decoder
topology.

The current implementation fails to parse valid decode topologies,
as it 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 existing relative sort with cxl_calc_interleave_pos() that
finds the exact position in a region interleave for an endpoint based
on a walk up the ancestral tree from endpoint to root decoder.

cxl_calc_interleave_pos() was introduced in a prior patch, so the work
here is to use it in cxl_region_sort_targets().

Remove the obsoleted helper functions from the prior sort.

Testing passes on pre-production hardware with BIOS defined regions
that natively trigger this autodiscovery path of the region driver.
Testing passes a CXL unit test using the dev_dbg() calculation test
(see cxl_region_attach()) across an expanded set of region configs:
1, 1, 1+1, 1+1+1, 2, 2+2, 2+2+2, 2+2+2+2, 4, 4+4, where each number
represents the count of endpoints per host bridge.

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>
Reviewed-by: Jim Harris <jim.harris@samsung.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 481aab3234bf..ce421b242715 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)
@@ -1603,131 +1611,26 @@ static int cxl_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 = cxl_calc_interleave_pos(cxled);
 		/*
-		 * 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;
-- 
2.37.3

Re: [PATCH v4 2/3] cxl/region: Calculate a target position in a region interleave

[Dan Williams]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Introduce a calculation to find a target's position in a region
> interleave. Perform a self-test of the calculation on user-defined
> regions.
> 
> The region driver uses the kernel sort() function to put region
> targets in relative order. Positions are assigned based on each
> target's index in that sorted list. That relative sort doesn't
> consider the offset of a port into its parent port which causes
> some auto-discovered regions to fail creation. In one failure case,
> a 2 + 2 config (2 host bridges each with 2 endpoints), the sort
> puts all the targets of one port ahead of another port when they
> were expected to be interleaved.
> 
> In preparation for repairing the autodiscovery region assembly,
> introduce a new method for discovering a target position in the
> region interleave.
> 
> cxl_calc_interleave_pos() adds a method to find the target position by
> ascending from an endpoint to a root decoder. The calculation starts
> with the endpoint's local position and position in the parent port. It
> traverses towards the root decoder and examines both position and ways
> in order to allow the position to be refined all the way to the root
> decoder.
> 
> This calculation: position = position * parent_ways + parent_pos;
> applied iteratively yields the correct position.
> 
> Include a self-test that exercises this new position calculation against
> every successfully configured user-defined region.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
[..]
> +/**
> + * cxl_calc_interleave_pos() - calculate an endpoint position in a region
> + * @cxled: the endpoint decoder
> + *
> + * The endpoint position is calculated by traversing from the endpoint to
> + * the root decoder and iteratively applying this calculation:
> + *	position = position * parent_ways + parent_pos;
> + *
> + * For example, the expected interleave order of the 4-way region shown
> + * below is: mem0, mem2, mem1, mem3
> + *
> + *		  root_port
> + *                 /      \
> + *      host_bridge_0    host_bridge_1
> + *        |    |           |    |
> + *       mem0 mem1        mem2 mem3
> + *
> + * In the example the calculator will iterate twice. The first iteration
> + * uses the mem position in the host-bridge and the ways of the host-
> + * bridge to generate the first, or local, position. The second iteration
> + * uses the host-bridge position in the root_port and the ways of the
> + * root_port to refine the position.
> + *
> + * A trace of the calculation per endpoint looks like this:
> + * mem0:	pos = 0 * 2 + 0		mem2:	pos = 0 * 2 + 0
> + *		pos = 0 * 2 + 0			pos = 0 * 2 + 1
> + *		pos: 0				pos: 1
> + *
> + * mem1:	pos = 0 * 2 + 1		mem3:	pos = 0 * 2 + 1
> + *		pos = 1 * 2 + 0			pos = 1 * 2 + 1
> + *		pos: 2				pos = 3
> + *
> + * Note that while this example is simple, the method applies to more
> + * complex topologies, including those with switches.
> + *
> + * Return: position >= 0 on success
> + *	   -ENXIO on failure
> + */
> +
> +static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
> +{

This needed a minor fixup for the RCD case, since RCDs are directly
integrated into the CXL-root with no intervening ports. I went ahead and
rolled this hunk:

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 0fee06660c04..62f1feb1781f 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1575,8 +1575,12 @@ static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	struct range *range = &cxled->cxld.hpa_range;
 	int parent_ways = 0, parent_pos = 0, pos = 0;
+	struct cxl_memdev_state = cxlmd->cxlds;
 	int rc;
 
+	if (cxlds->rcd)
+		goto out;
+
 	/* Iterate from endpoint to root_port refining the position */
 	for (iter = port; iter; iter = next_port(iter)) {
 		if (is_cxl_root(iter))
@@ -1589,6 +1593,7 @@ static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
 		pos = pos * parent_ways + parent_pos;
 	}
 
+out:
 	dev_dbg(&cxlmd->dev,
 		"decoder:%s parent:%s port:%s range:%#llx-%#llx pos:%d\n",
 		dev_name(&cxled->cxld.dev), dev_name(cxlmd->dev.parent),

Re: [PATCH v4 2/3] cxl/region: Calculate a target position in a region interleave

[Dan Williams]

Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Introduce a calculation to find a target's position in a region
> > interleave. Perform a self-test of the calculation on user-defined
> > regions.
> > 
> > The region driver uses the kernel sort() function to put region
> > targets in relative order. Positions are assigned based on each
> > target's index in that sorted list. That relative sort doesn't
> > consider the offset of a port into its parent port which causes
> > some auto-discovered regions to fail creation. In one failure case,
> > a 2 + 2 config (2 host bridges each with 2 endpoints), the sort
> > puts all the targets of one port ahead of another port when they
> > were expected to be interleaved.
> > 
> > In preparation for repairing the autodiscovery region assembly,
> > introduce a new method for discovering a target position in the
> > region interleave.
> > 
> > cxl_calc_interleave_pos() adds a method to find the target position by
> > ascending from an endpoint to a root decoder. The calculation starts
> > with the endpoint's local position and position in the parent port. It
> > traverses towards the root decoder and examines both position and ways
> > in order to allow the position to be refined all the way to the root
> > decoder.
> > 
> > This calculation: position = position * parent_ways + parent_pos;
> > applied iteratively yields the correct position.
> > 
> > Include a self-test that exercises this new position calculation against
> > every successfully configured user-defined region.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> [..]
> > +/**
> > + * cxl_calc_interleave_pos() - calculate an endpoint position in a region
> > + * @cxled: the endpoint decoder
> > + *
> > + * The endpoint position is calculated by traversing from the endpoint to
> > + * the root decoder and iteratively applying this calculation:
> > + *	position = position * parent_ways + parent_pos;
> > + *
> > + * For example, the expected interleave order of the 4-way region shown
> > + * below is: mem0, mem2, mem1, mem3
> > + *
> > + *		  root_port
> > + *                 /      \
> > + *      host_bridge_0    host_bridge_1
> > + *        |    |           |    |
> > + *       mem0 mem1        mem2 mem3
> > + *
> > + * In the example the calculator will iterate twice. The first iteration
> > + * uses the mem position in the host-bridge and the ways of the host-
> > + * bridge to generate the first, or local, position. The second iteration
> > + * uses the host-bridge position in the root_port and the ways of the
> > + * root_port to refine the position.
> > + *
> > + * A trace of the calculation per endpoint looks like this:
> > + * mem0:	pos = 0 * 2 + 0		mem2:	pos = 0 * 2 + 0
> > + *		pos = 0 * 2 + 0			pos = 0 * 2 + 1
> > + *		pos: 0				pos: 1
> > + *
> > + * mem1:	pos = 0 * 2 + 1		mem3:	pos = 0 * 2 + 1
> > + *		pos = 1 * 2 + 0			pos = 1 * 2 + 1
> > + *		pos: 2				pos = 3
> > + *
> > + * Note that while this example is simple, the method applies to more
> > + * complex topologies, including those with switches.
> > + *
> > + * Return: position >= 0 on success
> > + *	   -ENXIO on failure
> > + */
> > +
> > +static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
> > +{
> 
> This needed a minor fixup for the RCD case, since RCDs are directly
> integrated into the CXL-root with no intervening ports. I went ahead and
> rolled this hunk:

Nope, that hunk was broken and did not fix the issue, but the below
does. The reason this was triggering on the RCH region test was due to
the fact that cxl_test defines a sub-window size region to
auto-assemble.

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index dbaea89dfa4d..6f8a50bf6201 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1500,6 +1500,8 @@ static int match_switch_decoder_by_range(struct device *dev, void *data)
 	cxlsd = to_cxl_switch_decoder(dev);
 	r1 = &cxlsd->cxld.hpa_range;
 
+	if (is_root_decoder(dev))
+		return range_contains(r1, r2);
 	return (r1->start == r2->start && r1->end == r2->end);
 }


I'll fold this into patch1. The root-decoders are a super-set of
switch-decoders and the range they cover is a super-set of the region
range.

The failure mode could intermittently trigger a crash which I need to
debug, but I am fairly certain it was due to causing auto-assembly to
fail in an awkward place:

 general protection fault, probably for non-canonical address 0x5a5a5a5a5a5a5ab2: 0000 [#1] PREEMPT SMP PTI
 CPU: 23 PID: 1468 Comm: cxl Tainted: G           OE    N 6.6.0-rc3+ #1120
 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20230524-3.fc38 05/24/2023
 RIP: 0010:to_cxl_port+0x8/0x60 [cxl_core]
 Call Trace:
  <TASK>
  ? die_addr+0x32/0x80
  ? exc_general_protection+0x19b/0x450
  ? asm_exc_general_protection+0x22/0x30
  ? to_cxl_port+0x8/0x60 [cxl_core]
  cxl_region_detach+0x19/0x210 [cxl_core]
  detach_target.part.0+0x29/0x80 [cxl_core]
  unregister_region+0x42/0x70 [cxl_core]
  devres_release_all+0xb8/0x110
  device_unbind_cleanup+0xe/0x70
  device_release_driver_internal+0x1d2/0x210
  unbind_store+0x9d/0xb0

RE: [PATCH v4 2/3] cxl/region: Calculate a target position in a region interleave

[Dan Williams]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Introduce a calculation to find a target's position in a region
> interleave. Perform a self-test of the calculation on user-defined
> regions.
> 
> The region driver uses the kernel sort() function to put region
> targets in relative order. Positions are assigned based on each
> target's index in that sorted list. That relative sort doesn't
> consider the offset of a port into its parent port which causes
> some auto-discovered regions to fail creation. In one failure case,
> a 2 + 2 config (2 host bridges each with 2 endpoints), the sort
> puts all the targets of one port ahead of another port when they
> were expected to be interleaved.
> 
> In preparation for repairing the autodiscovery region assembly,
> introduce a new method for discovering a target position in the
> region interleave.
> 
> cxl_calc_interleave_pos() adds a method to find the target position by
> ascending from an endpoint to a root decoder. The calculation starts
> with the endpoint's local position and position in the parent port. It
> traverses towards the root decoder and examines both position and ways
> in order to allow the position to be refined all the way to the root
> decoder.
> 
> This calculation: position = position * parent_ways + parent_pos;
> applied iteratively yields the correct position.
> 
> Include a self-test that exercises this new position calculation against
> every successfully configured user-defined region.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
[..]
> +/**
> + * cxl_calc_interleave_pos() - calculate an endpoint position in a region
> + * @cxled: the endpoint decoder
> + *
> + * The endpoint position is calculated by traversing from the endpoint to
> + * the root decoder and iteratively applying this calculation:
> + *	position = position * parent_ways + parent_pos;
> + *
> + * For example, the expected interleave order of the 4-way region shown
> + * below is: mem0, mem2, mem1, mem3
> + *
> + *		  root_port
> + *                 /      \
> + *      host_bridge_0    host_bridge_1
> + *        |    |           |    |
> + *       mem0 mem1        mem2 mem3
> + *
> + * In the example the calculator will iterate twice. The first iteration
> + * uses the mem position in the host-bridge and the ways of the host-
> + * bridge to generate the first, or local, position. The second iteration
> + * uses the host-bridge position in the root_port and the ways of the
> + * root_port to refine the position.
> + *
> + * A trace of the calculation per endpoint looks like this:
> + * mem0:	pos = 0 * 2 + 0		mem2:	pos = 0 * 2 + 0
> + *		pos = 0 * 2 + 0			pos = 0 * 2 + 1
> + *		pos: 0				pos: 1
> + *
> + * mem1:	pos = 0 * 2 + 1		mem3:	pos = 0 * 2 + 1
> + *		pos = 1 * 2 + 0			pos = 1 * 2 + 1
> + *		pos: 2				pos = 3

As 0day reports, kdoc does not like formatted text, so I will fold in
this fixup:

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 9c714b53908d..f6be4164dfbe 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1534,43 +1534,19 @@ static int find_pos_and_ways(struct cxl_port *port, struct range *range,
 
 /**
  * cxl_calc_interleave_pos() - calculate an endpoint position in a region
- * @cxled: the endpoint decoder
+ * @cxled: endpoint decoder member of given region
  *
- * The endpoint position is calculated by traversing from the endpoint to
- * the root decoder and iteratively applying this calculation:
- *	position = position * parent_ways + parent_pos;
+ * The endpoint position is calculated by traversing the topology from
+ * the endpoint to the root decoder and iteratively applying this
+ * calculation:
  *
- * For example, the expected interleave order of the 4-way region shown
- * below is: mem0, mem2, mem1, mem3
+ *    position = position * parent_ways + parent_pos;
  *
- *		  root_port
- *                 /      \
- *      host_bridge_0    host_bridge_1
- *        |    |           |    |
- *       mem0 mem1        mem2 mem3
- *
- * In the example the calculator will iterate twice. The first iteration
- * uses the mem position in the host-bridge and the ways of the host-
- * bridge to generate the first, or local, position. The second iteration
- * uses the host-bridge position in the root_port and the ways of the
- * root_port to refine the position.
- *
- * A trace of the calculation per endpoint looks like this:
- * mem0:	pos = 0 * 2 + 0		mem2:	pos = 0 * 2 + 0
- *		pos = 0 * 2 + 0			pos = 0 * 2 + 1
- *		pos: 0				pos: 1
- *
- * mem1:	pos = 0 * 2 + 1		mem3:	pos = 0 * 2 + 1
- *		pos = 1 * 2 + 0			pos = 1 * 2 + 1
- *		pos: 2				pos = 3
- *
- * Note that while this example is simple, the method applies to more
- * complex topologies, including those with switches.
+ * ...where @position is inferred from switch and root decoder target lists.
  *
  * Return: position >= 0 on success
  *	   -ENXIO on failure
  */
-
 static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
 {
 	struct cxl_port *iter, *port = cxled_to_port(cxled);
@@ -1579,6 +1555,35 @@ static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
 	int parent_ways = 0, parent_pos = 0, pos = 0;
 	int rc;
 
+	/*
+	 * Example: the expected interleave order of the 4-way region shown
+	 * below is: mem0, mem2, mem1, mem3
+	 *
+	 *		  root_port
+	 *                 /      \
+	 *      host_bridge_0    host_bridge_1
+	 *        |    |           |    |
+	 *       mem0 mem1        mem2 mem3
+	 *
+	 * In the example the calculator will iterate twice. The first iteration
+	 * uses the mem position in the host-bridge and the ways of the host-
+	 * bridge to generate the first, or local, position. The second
+	 * iteration uses the host-bridge position in the root_port and the ways
+	 * of the root_port to refine the position.
+	 *
+	 * A trace of the calculation per endpoint looks like this:
+	 * mem0: pos = 0 * 2 + 0    mem2: pos = 0 * 2 + 0
+	 *       pos = 0 * 2 + 0          pos = 0 * 2 + 1
+	 *       pos: 0                   pos: 1
+	 *
+	 * mem1: pos = 0 * 2 + 1    mem3: pos = 0 * 2 + 1
+	 *       pos = 1 * 2 + 0          pos = 1 * 2 + 1
+	 *       pos: 2                   pos = 3
+	 *
+	 * Note that while this example is simple, the method applies to more
+	 * complex topologies, including those with switches.
+	 */
+
 	/* Iterate from endpoint to root_port refining the position */
 	for (iter = port; iter; iter = next_port(iter)) {
 		if (is_cxl_root(iter))