Function cxl_calc_interleave_pos() contains code to calculate the
interleaving parameters of a port. Factor out that code for later
reuse. Add function cxl_port_calc_interleave() for this and introduce
struct cxl_interleave_context to collect all interleaving data.

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/core/region.c | 63 ++++++++++++++++++++++++++-------------
 1 file changed, 43 insertions(+), 20 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index c118bda93e86..ad4a6ce37216 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1800,27 +1800,34 @@ static int find_pos_and_ways(struct cxl_port *port, struct range *range,
 	return rc;
 }
 
+struct cxl_interleave_context {
+	struct range *hpa_range;
+	int pos;
+};
+
 /**
- * cxl_calc_interleave_pos() - calculate an endpoint position in a region
- * @cxled: endpoint decoder member of given region
+ * cxl_port_calc_interleave() - calculate interleave config of an endpoint for @port
+ * @port: Port the new position is calculated for.
+ * @ctx: Interleave context
  *
- * The endpoint position is calculated by traversing the topology from
- * the endpoint to the root decoder and iteratively applying this
- * calculation:
+ * The endpoint position for the next port is calculated by applying
+ * this calculation:
  *
  *    position = position * parent_ways + parent_pos;
  *
  * ...where @position is inferred from switch and root decoder target lists.
  *
+ * The endpoint's position in a region can be calculated by traversing
+ * the topology from the endpoint to the root decoder and iteratively
+ * applying the function for each port.
+ *
  * Return: position >= 0 on success
  *	   -ENXIO on failure
  */
-static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
+static int cxl_port_calc_interleave(struct cxl_port *port,
+				    struct cxl_interleave_context *ctx)
 {
-	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 parent_ways = 0, parent_pos = 0;
 	int rc;
 
 	/*
@@ -1852,22 +1859,38 @@ static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
 	 * complex topologies, including those with switches.
 	 */
 
-	/* Iterate from endpoint to root_port refining the position */
-	for (iter = port; iter; iter = parent_port_of(iter)) {
-		if (is_cxl_root(iter))
-			break;
+	if (is_cxl_root(port))
+		return 0;
 
-		rc = find_pos_and_ways(iter, range, &parent_pos, &parent_ways);
-		if (rc)
-			return rc;
+	rc = find_pos_and_ways(port, ctx->hpa_range, &parent_pos, &parent_ways);
+	if (rc)
+		return rc;
 
-		pos = pos * parent_ways + parent_pos;
-	}
+	ctx->pos = ctx->pos * parent_ways + parent_pos;
+
+	return ctx->pos;
+}
+
+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 cxl_interleave_context ctx;
+	int pos = 0;
+
+	ctx = (struct cxl_interleave_context) {
+		.hpa_range = &cxled->cxld.hpa_range,
+	};
+
+	for (iter = cxled_to_port(cxled); pos >= 0 && iter;
+	     iter = parent_port_of(iter))
+		pos = cxl_port_calc_interleave(iter, &ctx);
 
 	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);
+		dev_name(&port->dev), ctx.hpa_range->start, ctx.hpa_range->end,
+		ctx.pos);
 
 	return pos;
 }
-- 
2.39.5

On Tue, 18 Feb 2025 14:23:44 +0100
Robert Richter <rrichter@amd.com> wrote:

> Function cxl_calc_interleave_pos() contains code to calculate the
> interleaving parameters of a port. Factor out that code for later
> reuse. Add function cxl_port_calc_interleave() for this and introduce
> struct cxl_interleave_context to collect all interleaving data.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/core/region.c | 63 ++++++++++++++++++++++++++-------------
>  1 file changed, 43 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index c118bda93e86..ad4a6ce37216 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1800,27 +1800,34 @@ static int find_pos_and_ways(struct cxl_port *port, struct range *range,
>  	return rc;
>  }
>  
> +struct cxl_interleave_context {
> +	struct range *hpa_range;
> +	int pos;

If this isn't going to get bigger later in the series I'd be inclined to just pass
the two separately.  Update the pos based on return value if non negative.

Maybe I'm missing a reason that doesn't work.



> +};
> +
>  /**
> - * cxl_calc_interleave_pos() - calculate an endpoint position in a region
> - * @cxled: endpoint decoder member of given region
> + * cxl_port_calc_interleave() - calculate interleave config of an endpoint for @port
> + * @port: Port the new position is calculated for.
> + * @ctx: Interleave context

>   *
> - * The endpoint position is calculated by traversing the topology from
> - * the endpoint to the root decoder and iteratively applying this
> - * calculation:
> + * The endpoint position for the next port is calculated by applying
> + * this calculation:
>   *
>   *    position = position * parent_ways + parent_pos;
>   *
>   * ...where @position is inferred from switch and root decoder target lists.
>   *
> + * The endpoint's position in a region can be calculated by traversing
> + * the topology from the endpoint to the root decoder and iteratively
> + * applying the function for each port.
> + *
>   * Return: position >= 0 on success
>   *	   -ENXIO on failure
>   */
> -static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
> +static int cxl_port_calc_interleave(struct cxl_port *port,
> +				    struct cxl_interleave_context *ctx)
>  {
> -	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 parent_ways = 0, parent_pos = 0;
>  	int rc;
>  
>  	/*
> @@ -1852,22 +1859,38 @@ static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
>  	 * complex topologies, including those with switches.
>  	 */
>  
> -	/* Iterate from endpoint to root_port refining the position */
> -	for (iter = port; iter; iter = parent_port_of(iter)) {
> -		if (is_cxl_root(iter))
> -			break;
> +	if (is_cxl_root(port))
> +		return 0;
>  
> -		rc = find_pos_and_ways(iter, range, &parent_pos, &parent_ways);
> -		if (rc)
> -			return rc;
> +	rc = find_pos_and_ways(port, ctx->hpa_range, &parent_pos, &parent_ways);
> +	if (rc)
> +		return rc;
>  
> -		pos = pos * parent_ways + parent_pos;
> -	}
> +	ctx->pos = ctx->pos * parent_ways + parent_pos;
> +
> +	return ctx->pos;
> +}
> +
> +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 cxl_interleave_context ctx;
> +	int pos = 0;
> +
> +	ctx = (struct cxl_interleave_context) {
> +		.hpa_range = &cxled->cxld.hpa_range,
> +	};
> +
> +	for (iter = cxled_to_port(cxled); pos >= 0 && iter;
> +	     iter = parent_port_of(iter))
> +		pos = cxl_port_calc_interleave(iter, &ctx);
>  
>  	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);
> +		dev_name(&port->dev), ctx.hpa_range->start, ctx.hpa_range->end,
> +		ctx.pos);
>  
>  	return pos;
>  }

On Tue, Feb 18, 2025 at 02:23:44PM +0100, Robert Richter wrote:
> Function cxl_calc_interleave_pos() contains code to calculate the
> interleaving parameters of a port. Factor out that code for later
> reuse. Add function cxl_port_calc_interleave() for this and introduce
> struct cxl_interleave_context to collect all interleaving data.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/core/region.c | 63 ++++++++++++++++++++++++++-------------
>  1 file changed, 43 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index c118bda93e86..ad4a6ce37216 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1800,27 +1800,34 @@ static int find_pos_and_ways(struct cxl_port *port, struct range *range,
>  	return rc;
>  }
>  
> +struct cxl_interleave_context {
> +	struct range *hpa_range;
> +	int pos;
> +};
> +

I get that this will be used later to pass information back, but this
patch by itself is a little confusing because ctx seems pointless since
the function still returns the position and accesses the hpa_range directly

Looked at in isolation, having the context structure change in this
patch than just adding the hpa_range as an argument and adding the
context later when it's actually relevant.

static int cxl_port_calc_interleave(struct cxl_port *port,
				    struct range *hpa_range);

> +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 cxl_interleave_context ctx;
> +	int pos = 0;
> +
> +	ctx = (struct cxl_interleave_context) {
> +		.hpa_range = &cxled->cxld.hpa_range,
> +	};
> +
> +	for (iter = cxled_to_port(cxled); pos >= 0 && iter;
> +	     iter = parent_port_of(iter))
> +		pos = cxl_port_calc_interleave(iter, &ctx);
>  
>  	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);
> +		dev_name(&port->dev), ctx.hpa_range->start, ctx.hpa_range->end,
> +		ctx.pos);
> 

	context just gets discarded here and hpa_range and pos are
	otherwise still accessible if you just pass hpa_range as an
	argument.  So I would push off adding the context argument to
	the patch that actually needs it.

>  	return pos;
>  }

~Gregory

On Thu, Feb 20, 2025 at 11:28:40AM -0500, Gregory Price wrote:
> On Tue, Feb 18, 2025 at 02:23:44PM +0100, Robert Richter wrote:
> I get that this will be used later to pass information back, but this
> patch by itself is a little confusing because ctx seems pointless since
> the function still returns the position and accesses the hpa_range directly
> 
> Looked at in isolation, having the context structure change in this
> patch than just adding the hpa_range as an argument and adding the
> context later when it's actually relevant.
> 
> static int cxl_port_calc_interleave(struct cxl_port *port,
> 				    struct range *hpa_range);
>

Disregard my note here, I missed that pos has to be carried through.

Didn't notice until I looked at patch 3 and 4 together.

> +	ctx->pos = ctx->pos * parent_ways + parent_pos;
> +
> +	return ctx->pos;

This looks fine

Reviewed-by: Gregory Price <gourry@gourry.net>