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

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

alison.schofield@ 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.

I would say:

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

...because there are lots of valid configurations that may not be
"correct". I.e. outside of pmem where there are labels to validate the
intended position, label-less autodiscovery can only interpret the
current state of the decode topology.

> 
> cxl_region_sort_targets() uses the kernel sort() function to put the
> targets in relative order.

This can more agressively throw the current implementation under the
bus.

"The current implementation fails to parse valid decode topologies, as
it does not consider the child offset into a parent port. The sort puts
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().

Maybe a comment on the testing this passes, or the plan for keeping this
algorithm regression tested?

> 
> 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>

[..]

Code looks good.