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

Commit Message

Alison Schofield Aug. 22, 2023, 6:09 p.m. UTC

From: Alison Schofield <[e-mail]>

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
        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 <[e-mail]>
---

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/[e-mail]/

 drivers/cxl/core/region.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)


base-commit: fe77cc2e5a6a7c85f5c6ef8a39d7694ffc7f41c9

Comments

Dave Jiang Aug. 25, 2023, 8:19 p.m. UTC | #1

On 8/22/23 11:09, [e-mail] wrote:
> From: Alison Schofield <[e-mail]>
> 
> 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
>          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 <[e-mail]>

Reviewed-by: Dave Jiang <[e-mail]>
> ---
> 
> 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/[e-mail]/
> 
>   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;
> -		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

Jonathan Cameron Aug. 29, 2023, 1:32 p.m. UTC | #2

On Tue, 22 Aug 2023 11:09:28 -0700
[e-mail] wrote:

> From: Alison Schofield <[e-mail]>
> 
> 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

Feel free to ignore the suggestion 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 <[e-mail]>
> ---
> 
> 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/[e-mail]/
> 
>  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'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

Alison Schofield Sept. 5, 2023, 5:29 p.m. UTC | #3

On Tue, Aug 29, 2023 at 02:32:40PM +0100, Jonathan Cameron wrote:
> On Tue, 22 Aug 2023 11:09:28 -0700
> [e-mail] wrote:
> 
> > From: Alison Schofield <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> > 
> > 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/[e-mail]/
> > 
> >  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
>

Patch

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);
+	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);