RE: [PATCH v3 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 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 | 142 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 142 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index eea8e89a6860..bea43da7e8f2 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1501,6 +1501,128 @@ static int match_switch_decoder_by_range(struct device *dev, void *data)
>  	return (r1->start == r2->start && r1->end == r2->end);
>  }
>  
> +/**
> + * cxl_find_pos_and_ways() - find the position of a port in a parent
> + *			     and the parent interleave ways

Given that this function is local to this file I do not think the cxl_
prefix is needed, and it does not match the below which is still
find_pos_and_ways().

> + *
> + * @port: the port in search of parent info
> + * @range: the hpa range that the parent must map
> + * @pos: the position of @port in the parent decoder target list
> + * @ways: the interleave ways of the parent decoder
> + *
> + * This helper function for cxl_calc_interleave_pos() provides the
> + * @pos and @ways used to calculate the endpoint position in a region
> + * interleave.
> + *
> + * Use @range to find @port's parent port and follow that to the
> + * parent switch decoder. Extract @ways from the switch decoder and
> + * lookup the @port @pos in the switch decoder target list.

Hmm, per your concern in the cover letter, yes, this
find_pos_and_ways()() commentary is not adding much for describing the
theory of operation. Lets just go with the calc_interleave_pos() comment
block which is answering all my concerns about documenting what is going
on here.

> + *
> + * Returns:	0: success, @pos and @ways are updated
> + *	   -ENXIO: failed to find @pos or @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;
> +	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.

Looks good.

Just delete the find_pos_and_ways() kdoc and we can call this one good
to go.