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

[Alison Schofield <alison.schofield@intel.com>]

On Tue, Aug 29, 2023 at 02:32:40PM +0100, Jonathan Cameron wrote:
> On Tue, 22 Aug 2023 11:09:28 -0700
> alison.schofield@intel.com 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 encoded interleave granularity
> > value 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
> 
> Arguably doesn't matter what the granularity is for a 1 way
> decode. Probably just drop that or does it matter for the
> calculations somewhere?
> 
>       Switch Decoder_A: 1 way

I agree that the gran for a 1 way decode doesn't matter. The config
offered is an example, so it includes actual gran.

> 
> Feel free to ignore the suggestion below.

see below -

> 
> 
> >         Endpoint_X:	2 way at 256
> >     Switch Decoder_B:	1 way at 512
> >         Endpoint_Y:	2 way at 256
> > 
> > When the Host Bridge interleave is greater than 1 and the root
> > 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 simplification is to apply the logic to the nominal
> > values and use the existing helper function granularity_to_eig() to
> > translate the desired granularity to the encoded form. This means
> > the comment and code regarding setting address bits is discarded.
> > Although that logic is 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 nominal 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>
> > ---
> > 
> > Changes in v2:
> > - No changes to the code. Commit log fixups only.
> > - Correct the commit log example. Endpoints are 2 * 256 (Jonathan)
> > - Use 'encoded' and 'nominal' when referring to the interleave
> >   granularity format (Dan, DaveJ) 
> > - Commit log grammar & spelling fixups (Dan, DaveJ)
> > - Add Fixes Tag (Dan)
> > - v1: https://lore.kernel.org/linux-cxl/20230804232726.1672782-1-alison.schofield@intel.com/
> > 
> >  drivers/cxl/core/region.c | 17 ++++++++---------
> >  1 file changed, 8 insertions(+), 9 deletions(-)
> > 
> > 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;
> 
> This threw me briefly.   eiw isn't used before this patch.
> You 'could' pull that out as a precursor to make this logic a tiny bit
> more obvious but meh it may not be worth doing so.

I expect it will ease the backport of this fix to keep as is.

Alison

> 
> I've not checked the maths (As interleave gives me headaches far too quickly
> so will rely on the you and the other reviewers to ensure that was right :)
> 
> 
> 
> > -		eig = peig;
> > +	rc = granularity_to_eig(parent_ig * parent_iw, &eig);
> > +	if (rc) {
> > +		dev_dbg(&cxlr->dev,
> > +			"%s: invalid granularity calculation (%d * %d)\n",
> > +			dev_name(&parent_port->dev), parent_ig, parent_iw);
> > +		return rc;
> >  	}
> >  
> >  	rc = eig_to_granularity(eig, &ig);
> > 
> > base-commit: fe77cc2e5a6a7c85f5c6ef8a39d7694ffc7f41c9
>