Re: [PATCH v3 1/9] cxl/region: Factor port target calculations

On Tue, Aug 18, 2026 at 10:20:34AM +0200, Robert Richter wrote:
> On 30.07.26 15:20:21, Alison Schofield wrote:
> > cxl_port_setup_targets() calculates the interleave fan-out above a
> > port, the port decoder granularity, and the distance between
> > endpoints routed through the same downstream port.
> > 
> > Factor those calculations into helpers so the target setup path can
> > be extended. Validation of parent decoder values is dropped since
> > those values are validated where they are set, before this port's
> > setup runs. A configuration that is invalid in more than one way may
> > report a different error first.
> > 
> > Suggested-by: Originally-by: Robert Richter <[email protected]>
> > Signed-off-by: Alison Schofield <[email protected]>
> > ---
> >  drivers/cxl/core/region.c | 197 +++++++++++++++++++-------------------
> 
> Please split patch and move out changes in error handling, see also
> below.

Thanks for the reviews Robert!

I took another pass at this, including the broader complexity concern
you raised here and in the collab mtg.

Rather than further splitting the refactoring here, I reworked the
the series around the existing region setup flow. The selector walk
and the helper machinery introduced here are gone in v4, along with
the unrelated error-handling movement.

more below

> 
> >  1 file changed, 96 insertions(+), 101 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 1e211542b6b6..1082db7b2cca 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1350,6 +1350,19 @@ static void cxl_port_detach_region(struct cxl_port *port,
> >  		free_region_ref(cxl_rr);
> >  }
> >  
> > +/**
> > + * check_last_peer() - Verify the previous endpoint routed to this dport
> > + * @cxled: endpoint decoder being placed
> > + * @ep: this endpoint's entry for the port
> > + * @cxl_rr: region reference for the port
> > + * @distance: distance to the previous endpoint routed to this dport
> > + *
> > + * Endpoints routed through the same dport recur at @distance intervals in
> > + * region-position order. Verify that the endpoint at ``pos - distance`` used
> > + * the same dport.
> > + *
> > + * Return: 0 on success, -ENXIO on a routing mismatch.
> > + */
> >  static int check_last_peer(struct cxl_endpoint_decoder *cxled,
> >  			   struct cxl_ep *ep, struct cxl_region_ref *cxl_rr,
> >  			   int distance)
> > @@ -1434,60 +1447,106 @@ static int check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
> >  	return 0;
> >  }
> >  
> > +/**
> > + * get_parent_fanout() - Calculate the switch fan-out above a port
> > + * @parent_port: first ancestor port
> > + * @cxlr: region under construction
> > + * @fanout: filled with the product of the ancestor switch ways
> > + *
> > + * Walk from @parent_port to the root and multiply the interleave ways of
> > + * each switch decoder. Root decoder ways are not included.
> > + *
> > + * Return: 0 on success.
> > + */
> 
> I don't think that static functions should be documented. They do not
> describe an interface, are short and also called only once. Instead,
> the function's purpose should be understandable from reading the code
> with small comments only where really helpful.
> 

These helpers are gone in v4. I also took your larger point here and
tried to make the resulting code readable without needing comments
to explain the implementation.


> > +static int get_parent_fanout(struct cxl_port *parent_port,
> > +			     struct cxl_region *cxlr, int *fanout)
> > +{
> > +	int distance = 1;
> > +	struct cxl_port *iter;
> > +
> > +	for (iter = parent_port; !is_cxl_root(iter);
> > +	     iter = to_cxl_port(iter->dev.parent)) {
> > +		struct cxl_region_ref *cxl_rr_iter = cxl_rr_load(iter, cxlr);
> > +
> > +		distance *= cxl_rr_iter->nr_targets;
> > +	}
> > +
> > +	*fanout = distance;
> > +	return 0;
> > +}
> > +
> > +/**
> > + * derive_port_granularity() - Calculate the granularity for a port decoder
> > + * @cxlr: region under construction
> > + * @fanout: product of the ancestor switch ways
> > + * @ig: filled with the port decoder granularity
> > + *
> > + * Preserve the existing parent-granularity times parent-ways recurrence in
> > + * terms of the region granularity and the fan-out above this port.
> > + *
> > + * Return: 0 on success.
> > + */
> > +static int derive_port_granularity(struct cxl_region *cxlr, int fanout,
> > +				   int *ig)
> > +{
> > +	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> > +	int root_iw = cxlrd->cxlsd.cxld.interleave_ways;
> > +	struct cxl_region_params *p = &cxlr->params;
> > +	int sel_distance;
> > +
> > +	sel_distance = is_power_of_2(root_iw) ? root_iw : root_iw / 3;
> > +	sel_distance *= fanout;
> > +	*ig = p->interleave_granularity * sel_distance;
> > +
> > +	return 0;
> 
> Always succeeds. Should directly return ig.

Agree. This helper is gone in v4.

> 
> > +}
> 
> I will review both functions again after reading the rest of the
> series.
> 
> > +
> >  static int cxl_port_setup_targets(struct cxl_port *port,
> >  				  struct cxl_region *cxlr,
> >  				  struct cxl_endpoint_decoder *cxled)
> >  {
> >  	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> > -	int parent_iw, parent_ig, ig, iw, rc, pos = cxled->pos;
> > +	int root_iw = cxlrd->cxlsd.cxld.interleave_ways;
> >  	struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
> >  	struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
> >  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> >  	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
> >  	struct cxl_region_params *p = &cxlr->params;
> >  	struct cxl_decoder *cxld = cxl_rr->decoder;
> > -	struct cxl_switch_decoder *cxlsd;
> > -	struct cxl_port *iter = port;
> > -	u16 eig, peig;
> > -	u8 eiw, peiw;
> > +	struct cxl_switch_decoder *cxlsd = to_cxl_switch_decoder(&cxld->dev);
> > +	int ig, iw = cxl_rr->nr_targets;
> 
> The changes around here should be a separate patch only containing
> error handling changes and other related reworks.
> 

Agree. I dropped this unrelated rework rather than carrying as
part of this series.


> > +	int fanout, rc;
> > +	int pos = cxled->pos;
> > +	u16 eig;
> > +	u8 eiw;
> >  
> >  	/*
> >  	 * While root level decoders support x3, x6, x12, switch level
> >  	 * decoders only support powers of 2 up to x16.
> >  	 */
> > -	if (!is_power_of_2(cxl_rr->nr_targets)) {
> > +	if (!is_power_of_2(iw)) {
> >  		dev_dbg(&cxlr->dev, "%s:%s: invalid target count %d\n",
> > -			dev_name(port->uport_dev), dev_name(&port->dev),
> > -			cxl_rr->nr_targets);
> > +			dev_name(port->uport_dev), dev_name(&port->dev), iw);
> >  		return -EINVAL;
> >  	}
> >  
> > -	cxlsd = to_cxl_switch_decoder(&cxld->dev);
> > +	if (iw > 8 || iw > cxlsd->nr_targets) {
> > +		dev_dbg(&cxlr->dev,
> > +			"%s:%s:%s: ways: %d overflows targets: %d\n",
> > +			dev_name(port->uport_dev), dev_name(&port->dev),
> > +			dev_name(&cxld->dev), iw, cxlsd->nr_targets);
> > +		return -ENXIO;
> > +	}
> 
> Split patch: Moving of this check and other changes above should be in
> a separate patch.

Agree. This movement is gone in v4.

> 
> > +
> > +	rc = get_parent_fanout(parent_port, cxlr, &fanout);
> 
> I am not a "fan" of that term. :-) IMO, target_count or target_total
> would fit better here.

:) No more fanout in v4.

> 
> > +	if (rc)
> > +		return rc;
> > +
> >  	if (cxl_rr->nr_targets_set) {
> 
> The check can be dropped now as it is done with the for loop already.

Agree. This rework is gone in v4 as well.

snip

> > @@ -1495,84 +1554,20 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> >  		goto add_target;
> >  	}
> >  
> > -	if (is_cxl_root(parent_port)) {
> > -		/*
> > -		 * Root decoder IG is always set to value in CFMWS which
> > -		 * may be different than this region's IG.  We can use the
> > -		 * region's IG here since interleave_granularity_store()
> > -		 * does not allow interleaved host-bridges with
> > -		 * root IG != region IG.
> > -		 */
> > -		parent_ig = p->interleave_granularity;
> > -		parent_iw = cxlrd->cxlsd.cxld.interleave_ways;
> > -		/*
> > -		 * For purposes of address bit routing, use power-of-2 math for
> > -		 * switch ports.
> > -		 */
> > -		if (!is_power_of_2(parent_iw))
> > -			parent_iw /= 3;
> > -	} else {
> > -		struct cxl_region_ref *parent_rr;
> > -		struct cxl_decoder *parent_cxld;
> > -
> > -		parent_rr = cxl_rr_load(parent_port, cxlr);
> > -		parent_cxld = parent_rr->decoder;
> > -		parent_ig = parent_cxld->interleave_granularity;
> > -		parent_iw = parent_cxld->interleave_ways;
> > -	}
> > -
> > -	rc = granularity_to_eig(parent_ig, &peig);
> > -	if (rc) {
> > -		dev_dbg(&cxlr->dev, "%s:%s: invalid parent granularity: %d\n",
> > -			dev_name(parent_port->uport_dev),
> > -			dev_name(&parent_port->dev), parent_ig);
> > -		return rc;
> > -	}
> > -
> > -	rc = ways_to_eiw(parent_iw, &peiw);
> > -	if (rc) {
> > -		dev_dbg(&cxlr->dev, "%s:%s: invalid parent interleave: %d\n",
> > -			dev_name(parent_port->uport_dev),
> > -			dev_name(&parent_port->dev), parent_iw);
> > +	rc = derive_port_granularity(cxlr, fanout, &ig);
> 
> I still think, the granularity should be determined just by
> calculating the bit position of the ways bit within the HPA. But let's
> see next patches.


Agree with the direction here. I ended up dropping the selector walk as
well, though, and deriving the decoder granularity directly from the parent
interleave geometry. More on that in the following patches.


> 
> > +	if (rc)
> >  		return rc;
> > -	}
> >  
> > -	iw = cxl_rr->nr_targets;
> >  	rc = ways_to_eiw(iw, &eiw);
> > -	if (rc) {
> > -		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",
> > -			dev_name(port->uport_dev), dev_name(&port->dev), iw);
> > -		return rc;
> > -	}
> > -
> > -	/*
> > -	 * Interleave granularity is a multiple of @parent_port granularity.
> > -	 * Multiplier is the parent port interleave ways.
> > -	 */
> > -	rc = granularity_to_eig(parent_ig * parent_iw, &eig);
> > +	if (!rc)
> > +		rc = granularity_to_eig(ig, &eig);
> 
> Same here, separate the combination of those two checks in a separate
> patch.
> 
> With a patch split the actual change will be much better readable.


Agree. Rather than splitting this version further, I dropped this rework
and substantially simplified the target setup in v4.


> 
> Thanks,
> 
> -Robert
> 

snip