RE: [PATCH] cxl/region: Refactor granularity select in cxl_port_setup_targets()

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

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> In cxl_port_setup_targets() the region driver validates the
> configuration of auto-discovered region decoders, as well
> as decoders the driver is preparing to program.
> 
> The existing calculations use the encrypted interleave granularity

s/encrypted/encoded/

> value, the eig, to create an interleave granularity that properly
> fans out when routing an x1 interleave to a greater than x1 interleave.
> 
> That all worked well, until this config came along:
> Host Bridge: 2 way at 256 granularity
>     Switch Decoder_A:	1 way at 512
>         Endpoint_X:	2 way at 512
>     Switch Decoder_B:	1 way at 512
>         Endpoint_Y:	2 way at 512
> 
> When the Host Bridge interleave is greater that 1, and the root

s/that/than/

> decoder interleave is exactly 1, the region driver needs to
> consider the number of targets in the region when calculating
> the expected granularity.
> 
> While examining the existing logic, and trying to cover the case
> above, a couple of simplifications appeared, hence this proposed
> refactoring.
> 
> The first simplicfication is to apply the logic to the unencrypted

s/unencrypted/nominal/

> values and use the existing helper function granularity_to_eig() to
> translate the desired granularity to the encrypted form. This means
> the comment and code regarding setting address bits is discarded.
> Although that logic was not wrong, it adds a level of complexity that
> is not required in the granularity selection. The eig and eiw are
> indeed part of the routing instructions programmed into the decoders.
> Up-level the discussion to plain ways and granularity for clearer
> analysis.
> 
> The second simplification reduces the logic to a single granularity
> calculation that works for all cases. The new calculation doesn't
> care if parent_iw => 1,  because parent_iw is used as a multiplier.
> 
> The refactor cleans up a useless assignment of eiw, made after the iw
> is already calculated.
> 
> Regression testing included an examination of all of the ways and
> granularity selections made during a run of the cxl_test unit tests.
> There were no differences in selections before and after this patch.

Fixes: ("27b3f8d13830 cxl/region: Program target lists")

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

Fix *and* less code!? Yes, please!

> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index e115ba382e04..5a1cc59cca99 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1154,16 +1154,15 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  	}
>  
>  	/*
> -	 * If @parent_port is masking address bits, pick the next unused address
> -	 * bit to route @port's targets.
> +	 * Interleave granularity is a multiple of @parent_port granularity.
> +	 * Multiplier is the parent port interleave ways.
>  	 */
> -	if (parent_iw > 1 && cxl_rr->nr_targets > 1) {
> -		u32 address_bit = max(peig + peiw, eiw + peig);
> -
> -		eig = address_bit - eiw + 1;
> -	} else {
> -		eiw = peiw;
> -		eig = peig;
> +	rc = granularity_to_eig(parent_ig * parent_iw, &eig);

I was going to say "wait, does this work with x3 parent_iw", and I
believe it does because this:

                /*
                 * For purposes of address bit routing, use power-of-2 math for
                 * switch ports.
                 */
                if (!is_power_of_2(parent_iw))
                        parent_iw /= 3;

...is handled in the is_cxl_root(parent_port) case.

A thing of beauty this patch. Ship it! (modulo those minor nits above).