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

[Dan Williams <dan.j.williams@intel.com>]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Introduce a calculation that determines a targets position in a region
> interleave. Perform a selftest 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
> targets 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_interleave_pos() offers a method to determine a targets position
> by ascending from an endpoint to a root decoder. The calculation starts
> with the endpoints local position and its position in its parents 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, applied iteratively, yields the correct position:
> 
> position = position * parent_ways + parent_pos;
> 
> ...with these rules:
> 
> Rule #1 - When (parent_ways == region_ways), Stop!
> 	  position = parent_position;
> 	  This rule is applied in calc_interleave_pos()
> 
> Rule #2 - Skip over siblings that come before this memdev in
> 	  the decoder list when searching for the parent position.
> 	  This rule is applied in the helper find_pos_and_ways().


I feel like calc_interleave_pos() is missing some kernel-doc about these
rules. It seems fundamental to maintaining this code going forward. This
short summary here is sufficient for the changelog, but
calc_interleave_pos() wants a few sentences on the theory of operation.

> 
> Include a selftest that exercises this new position calculation against
> every successfully configured user-defined region.
> 
> Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")

Again, this change is not a fix, it's a new diagnostic. It is a
dependency for a fix, but that discussion will come out around
backporting patch3.

> Reported-by: Dmytro Adamenko <dmytro.adamenko@intel.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  drivers/cxl/core/region.c | 102 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 102 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 64206fc4d99b..b451d215c3c5 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1500,6 +1500,93 @@ static int match_switch_decoder_by_range(struct device *dev, void *data)
>  	return range_contains(r1, r2);
>  }
>  
> +/* Find the position of a port in it's parent and the parents ways */
> +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;
> +	int child_ways = *ways;
> +	int child_pos = *pos;
> +	struct device *dev;
> +	int skip = 0;
> +	int rc = -1;

On the risk of this error code leaking higher, I think it should be
initialized to -ENXIO directly, and not translated by the caller.

> +
> +	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;
> +
> +	/* Skip over this many siblings in the target list */
> +	if (*ways > child_ways)
> +		skip = child_pos;

Maybe a clarification that "Since the switch target list is by
definition sorted in region position order, siblings to skip are always
at lower indices."

> +
> +	for (int i = 0; i < *ways; i++) {
> +		if (cxlsd->target[i] == port->parent_dport) {
> +			if (skip--)
> +				continue;
> +			*pos = i;
> +			rc = 0;
> +			break;
> +		}
> +	}
> +	put_device(dev);
> +
> +	return rc;
> +}
> +
> +static int calc_interleave_pos(struct cxl_endpoint_decoder *cxled,
> +			       int region_ways)
> +{
> +	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;
> +	int parent_pos = 0;
> +	int rc, pos;
> +
> +	/* Initialize pos to its local position */
> +	rc = find_pos_and_ways(port, range, &parent_pos, &parent_ways);
> +	if (rc)
> +		return -ENXIO; 

per above, this can become "return rc;".

I had wondered if this code becomes easier to read to have separate
inputs and outputs versus dual purpose the @pos and @ways paramters, but
I can't come up with anything simpler. I think a bit of kernel-doc would
help with the next casual reader that comes along and wonders about the
theory of operation.

> +
> +	pos = parent_pos;
> +
> +	if (parent_ways == region_ways)
> +		goto out;
> +
> +	/* Iterate up the ancestral tree refining the position */
> +	for (iter = next_port(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 -ENXIO;
> +
> +		if (parent_ways == region_ways) {
> +			pos = parent_pos;
> +			break;
> +		}
> +		pos = pos * parent_ways + parent_pos;

Nice simplification of the current mess!

> +	}
> +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),
> +		dev_name(&port->dev), range->start, range->end, pos);
> +
> +	return pos;
>  }
>  
>  static void find_positions(const struct cxl_switch_decoder *cxlsd,
> @@ -1765,6 +1852,21 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		.end = p->res->end,
>  	};
>  
> +	if (p->nr_targets != p->interleave_ways)
> +		return 0;
> +
> +	/* Exercise position calculator on user-defined regions */
> +	for (int i = 0; i < p->nr_targets; i++) {
> +		struct cxl_endpoint_decoder *cxled = p->targets[i];
> +		int test_pos;
> +
> +		test_pos = calc_interleave_pos(cxled, p->interleave_ways);
> +		dev_dbg(&cxled->cxld.dev,
> +			"Interleave calc match %s test_pos:%d cxled->pos:%d\n",
> +			(test_pos == cxled->pos) ? "Success" : "Fail",
> +			test_pos, cxled->pos);

Part of me wondered if the failure case should be louder here, but in
the case of autodiscovery there is no position to compare against.