Re: [PATCH v4 3/6] cxl/region: Support mixed-granularity auto regions

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

On Fri, Aug 21, 2026 at 10:57:59PM +0100, Jonathan Cameron wrote:
> On Thu, 20 Aug 2026 16:31:21 -0700
> Alison Schofield <alison.schofield@intel.com> wrote:
> 
> > The CXL Specification permits a region's interleave granularity to
> > differ between levels of the decoder hierarchy. For an auto region,
> > the CXL driver reconstructs that hierarchy from the decoder
> > configuration programmed by platform firmware.

Thanks for the review Jonathan!

The tidy ups and nits are always welcome as I don't want to disturb my
reviewers flow ;) I've reset my rule to 75, not sure why I had that set
at 70 columns.

> 
> With my fussy hat on (I blame Friday!), guidance for commit message wrap is
> 75 chars so bit longer than here.

Fixed up across all commits.

> 
> > 
> > The CXL driver currently assumes the interleaving root has the same
> > granularity as the region, with each successive interleaving level
> > becoming coarser toward the endpoints. This prevents the driver from
> > assembling an auto region where the root granularity is coarser than
> > the region granularity.
> > 
> > Support mixed-granularity auto regions in the CXL driver, restricted
> > to coarse-to-fine layouts. Derive each interleaving decoder's
> > granularity from its parent:
> > 
> >         child_ig = parent_ig / child_iw
> > 
> > The topology determines each decoder's interleave ways, so the parent
> > granularity and child ways determine the child granularity. Use that
> > relationship to validate the decoder geometry while assembling an auto
> > region.
> > 
> > Require the root and region to describe the same interleave span:
> > 
> >         root_iw * root_ig == region_iw * region_ig
> > 
> > The same span relationship covers the CXL Specification's Mod3
> > configurations. For example, a 6-way region at IGB across three host
> > bridges uses a 3-way root interleave at 2 * IGB and a 2-way interleave
> > at IGB below it, as described in CXL 4.0 Section 9.13.1.1.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> 
> This interleave stuff is a rapid path to a headache :( - I vaguely wondered if
> we can use a few local variable names to make it clear which ig and which iw
> each one is.

Done. Thanks for the suggestion, the improvment is nice! The bare iw and ig in
cxl_port_setup_targets() are now child_iw and child_ig, matching the
parent_ prefix already used for the level above, so every arithmetic line
says which side of the boundary it is on.

I did add it in a prep patch though, because doing so here polluted the
work of this patch. But when we get to 'this' patch in v5, it reads so
much better.

> 
> I'll go as far as I saying I ran some "paper tests" and it seems right
> but I've been out of this stuff long enough I'm not feeling that confident.
> Good thing you have tests in a later patch :)

Well, I ramped up the testing too. You'll see new mock topology in the
cxl/test patch, along w more test cases in the ndctl unit test patch.

> 
> > ---
> >  drivers/cxl/core/region.c | 114 ++++++++++++++++++++++++--------------
> >  1 file changed, 73 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 4f367feaf6c8..637d1b60a570 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1434,6 +1434,16 @@ static int check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
> >  	return 0;
> >  }
> >  
> > +/* Mixed granularity has a region IG finer than the interleaving root IG */
> > +static bool cxl_region_is_mixed_gran(struct cxl_region *cxlr)
> > +{
> > +	struct cxl_decoder *cxld = &cxlr->cxlrd->cxlsd.cxld;
> > +
> > +	return cxld->interleave_ways > 1 &&
> > +	       cxld->interleave_granularity >
> > +		       cxlr->params.interleave_granularity;
> 
> 	return cxld->interleave_ways > 1 &&
> 	       cxld->interleave_granularity > cxlr->params.interleave_granularity;
> 
> I'd go a little long to improve readability.

Done.

> 
> > +}
> 
> ...
>   
> > +static int cxl_region_validate_interleave(struct cxl_region *cxlr)
> > +{
> > +	struct cxl_decoder *cxld = &cxlr->cxlrd->cxlsd.cxld;
> > +	struct cxl_region_params *p = &cxlr->params;
> > +	int root_iw = cxld->interleave_ways;
> > +	int root_ig = cxld->interleave_granularity;
> > +
> > +	if (root_iw == 1)
> > +		return 0;
> > +
> > +	if (p->interleave_granularity > root_ig) {
> 
> This took me a while.  It's specific case of more general one that region
> granularity can't be greater than any decoder granularity in the path.
> I was trying to figure out what was special about the root :(
> 

Yes, you're right. There isn't anything special about the root other than
that its granularity is available before walking the topology.

v4 only checked the root. In v5 I kept that early check, but also added the
general check while deriving each decoder's granularity. If the derived
granularity becomes finer than the region granularity, the layout is
rejected.

That closes a hole in v4 where an unbalanced topology could otherwise make
it through this validation.


> > +		dev_dbg(&cxlr->dev,
> > +			"granularity %d exceeds root decoder granularity %d\n",
> > +			p->interleave_granularity, root_ig);
> > +		return -ENXIO;
> > +	}
> > +
> > +	/* Same-gran power-of-two regions may span multiple root targets */
> > +	if (is_power_of_2(root_iw) && p->interleave_granularity == root_ig)
> 
> So this is our original fine to coarse check dropping out nice and early?
> What is this letting through that doesn't get through next check anyway?
> If power of 2 and root_ig == region_ig aren't we guaranteed that
> root_iw == region_iw and next test passes anyway.
> Basically I'm lost on what this test is here for.  What do you mean by
> span multiple root targets that isn't true of the x12?

No, root_iw and region_iw do not have to match in the same-granularity
case. For example, a 2-way root at 4096 can host a 4-way region at 4096.
That is an existing valid configuration, but the span check would reject
it:

root:	2 * 4096
region:	4 * 4096

So the exemption is needed to preserve those power-of-two same-granularity
configurations.

The x12 mixed-granularity case is different. Its root and region spans do
match, like a 3-way at 1024 and 12-way at 256. Mod3 roots therefore go
through the span check rather than taking this exemption.

I also changed the comment in v5 to:

	/*
	 * Same-granularity regions below a power-of-two root may span multiple root
	 * interleaves. Mod3 roots are width-matched instead.
	 */

> 
> > +		return 0;
> > +
> > +	/* Mixed-gran regions must span exactly one root interleave */
> 
> Maths is fine, but I'm getting confused by terms.  This is talking I think
> about complete interleave cycles or maybe strides depending on definitions?

Yes, a complete interleave cycle. I was using "span" somewhat loosely in
v4. The v5 doc patch, now first in series, defines a span as one complete
pass through the interleave pattern, and I changed the comment here to:

/* Span is one complete pass through the interleave pattern. */

The debug message now also reports both the region and root spans directly.

-- Alison
> 
> > +	if (root_iw * root_ig != p->interleave_ways * p->interleave_granularity) {
> > +		dev_dbg(&cxlr->dev,
> > +			"%d ways at %d does not span root decoder %d ways at %d\n",
> > +			p->interleave_ways, p->interleave_granularity, root_iw,
> > +			root_ig);
> > +		return -ENXIO;
> > +	}
> > +
> > +	return 0;
> > +}
>