Re: [PATCH v2 2/6] cxl/region: Derive port granularity from selector bits

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

On Fri, Jun 26, 2026 at 02:09:22PM +0200, Robert Richter wrote:
> On 11.06.26 10:47:26, Alison Schofield wrote:
> > Selector-bit validation uses a single same-granularity formula
> > for user-created regions. Mixed-granularity layouts need the
> > programmed granularity to depend on the type of decoder being
> > configured.
> > 
> > Derive port granularity from the selector state:
> > 
> >   AUTO:    keep the firmware-programmed value; the walk validated it.
> >   iw == 1: passthrough; consume no selector bits and span the parent
> >            stride.
> >   iw > 1:  claim the lowest selector bit still available to this
> >            level.
> 
> If we change the implementation to use selector bits, this patch may
> change again. Anyway, see below.
> 

The selector-model question is answered in my reply on Patch 3/6.
v3 keeps position/granularity derivation in value space and keeps the
selector bits as the validation representation. This patch is updated
per your comments below, but not rewritten around selector arithmetic.


> > 
> > This lets an interleaving switch beneath a mixed-granularity root
> > claim an inner selector bit instead of colliding with the outer bit
> > already owned by the root.
> > 
> > Add a final check that the derived selector fits within the
> > remaining selector bits.
> > 
> > Remove the auto-region granularity equality check that duplicated
> > the old ordering rule. The selector walk now validates auto region
> > granularities.
> > 
> > Originally-by: Robert Richter <rrichter@amd.com>
> > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  drivers/cxl/core/region.c | 30 ++++++++++++++++++++++++++++--
> >  1 file changed, 28 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index c859c8661281..d5d19ed62171 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1533,6 +1533,9 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> >  		return -ENXIO;
> >  	}
> >  
> > +	/* Selector bits still available to this port. */
> > +	selector = cxlr_sel & ~selector;
> 
> This is only used to calculate ig for the non-auto case. Can be moved
> there.


Done in v3. Calc'd at the point of use in the non-auto path.


> 
> > +
> >  	if (cxl_rr->nr_targets_set) {
> >  		for (int i = 0; i < cxl_rr->nr_targets_set; i++)
> >  			if (ep->dport == cxlsd->target[i]) {
> > @@ -1545,7 +1548,23 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> >  		goto add_target;
> >  	}
> >  
> > -	ig = p->interleave_granularity * parent_distance;
> > +	/*
> > +	 * Auto regions keep the firmware value, passthrough decoders consume
> > +	 * no selector bits, interleaving decoders claim the lowest available
> > +	 * selector bit.
> > +	 */
> > +	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> > +		ig = cxld->interleave_granularity;
> 
> Move that to the CXL_REGION_F_AUTO section later below.

Done in v3.

> 
> > +	} else if (iw == 1) {
> > +		ig = p->interleave_granularity * parent_distance;
> > +	} else if (selector) {
> > +		ig = 1ULL << __ffs64(selector);
> > +	} else {
> > +		dev_dbg(&cxlr->dev,
> > +			"%s:%s: no selector bits available for iw %d\n",
> > +			dev_name(port->uport_dev), dev_name(&port->dev), iw);
> > +		return -ENXIO;
> > +	}
> 
> That can be moved to non-auto only part.
> 

Done in v3.


> >  
> >  	rc = ways_to_eiw(iw, &eiw);
> >  	if (!rc)
> > @@ -1557,9 +1576,16 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> >  		return rc;
> >  	}
> >  
> > +	if (iw > 1 && (~selector & get_selector(iw, ig))) {
> > +		dev_dbg(&cxlr->dev,
> > +			"%s:%s: derived selector %#llx exceeds remaining %#llx (iw %d ig %d)\n",
> > +			dev_name(port->uport_dev), dev_name(&port->dev),
> > +			get_selector(iw, ig), selector, iw, ig);
> > +		return -ENXIO;
> > +	}
> > +
> 
> Same with this check, should go to non-auto.
> 

Done in v3. With these moves the AUTO path keeps its checks in
one block and all derivation is visibly non-auto-only. That
reads much better, thanks!

-- Alison


> >  	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> >  		if (cxld->interleave_ways != iw ||
> > -		    (iw > 1 && cxld->interleave_granularity != ig) ||
> >  		    !spa_maps_hpa(p, &cxld->hpa_range) ||
> >  		    ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)) {
> >  			dev_err(&cxlr->dev,
> > -- 
> > 2.37.3
> >