Re: [PATCH 4/7] cxl/region: Factor out cxl_port_parent_interleave()

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

On Fri, Oct 09, 2026 at 03:39:43PM -0700, Dave Jiang wrote:
> 
> 
> On 10/9/26 1:24 PM, Alison Schofield wrote:
> > cxl_port_setup_targets() derives a decoder's interleave from the interleave
> > of the decoder above it. That parent interleave comes from either the root
> > decoder or a switch decoder, and each case is open coded along with the
> > parent validity checks.
> > 
> > Move the parent interleave lookup and validation to a new helper,
> > cxl_port_parent_interleave().
> > 
> > No functional change.
> > 
> > Assisted-by: LLM
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  drivers/cxl/core/region.c | 88 +++++++++++++++++++++++----------------
> >  1 file changed, 52 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 80c40716c368..c1866b7b089b 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1459,45 +1459,13 @@ static int cxl_port_peer_distance(struct cxl_port *port, struct cxl_region *cxlr
> >  	return distance;
> >  }
> >  
> > -static int cxl_port_setup_targets(struct cxl_port *port,
> > -				  struct cxl_region *cxlr,
> > -				  struct cxl_endpoint_decoder *cxled)
> > +static int cxl_port_parent_interleave(struct cxl_port *port, struct cxl_region *cxlr,
> > +				      int *iw, int *ig)
> >  {
> > -	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> > -	int parent_iw, parent_ig, child_iw, child_ig, rc, pos = cxled->pos;
> >  	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_root_decoder *cxlrd = cxlr->cxlrd;
> >  	struct cxl_region_params *p = &cxlr->params;
> > -	struct cxl_decoder *cxld = cxl_rr->decoder;
> > -	struct cxl_switch_decoder *cxlsd;
> > -
> > -	/*
> > -	 * 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)) {
> > -		dev_dbg(&cxlr->dev, "%s:%s: invalid target count %d\n",
> > -			dev_name(port->uport_dev), dev_name(&port->dev),
> > -			cxl_rr->nr_targets);
> > -		return -EINVAL;
> > -	}
> > -
> > -	cxlsd = to_cxl_switch_decoder(&cxld->dev);
> > -	if (cxl_rr->nr_targets_set) {
> > -		int distance = cxl_port_peer_distance(port, cxlr);
> > -
> > -		for (int i = 0; i < cxl_rr->nr_targets_set; i++)
> > -			if (ep->dport == cxlsd->target[i]) {
> > -				rc = check_last_peer(cxled, ep, cxl_rr,
> > -						     distance);
> > -				if (rc)
> > -					return rc;
> > -				goto out_target_set;
> > -			}
> > -		goto add_target;
> > -	}
> > +	int parent_iw, parent_ig;
> >  
> >  	if (is_cxl_root(parent_port)) {
> >  		/*
> > @@ -1539,6 +1507,54 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> >  		return -EINVAL;
> >  	}
> >  
> > +	*iw = parent_iw;
> > +	*ig = parent_ig;
> > +
> > +	return 0;
> > +}
> > +
> > +static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
> 
> Do we want to just rename it to cxl_switch_port_setup_targets() since the function basically is handling switch only?

Yes that makes the scope clearer. I think I shall add a new rename patch
before any of the refactoring patches so the subsequent patches all use
that name.  (Rather than sneaking it in here where it is first touched.)

Thanks for the review!


> 
> DJ
> 
> > +				  struct cxl_endpoint_decoder *cxled)
> > +{
> > +	int parent_iw, parent_ig, child_iw, child_ig, rc, pos = cxled->pos;
> > +	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;
> > +
> > +	/*
> > +	 * 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)) {
> > +		dev_dbg(&cxlr->dev, "%s:%s: invalid target count %d\n",
> > +			dev_name(port->uport_dev), dev_name(&port->dev),
> > +			cxl_rr->nr_targets);
> > +		return -EINVAL;
> > +	}
> > +
> > +	cxlsd = to_cxl_switch_decoder(&cxld->dev);
> > +	if (cxl_rr->nr_targets_set) {
> > +		int distance = cxl_port_peer_distance(port, cxlr);
> > +
> > +		for (int i = 0; i < cxl_rr->nr_targets_set; i++)
> > +			if (ep->dport == cxlsd->target[i]) {
> > +				rc = check_last_peer(cxled, ep, cxl_rr,
> > +						     distance);
> > +				if (rc)
> > +					return rc;
> > +				goto out_target_set;
> > +			}
> > +		goto add_target;
> > +	}
> > +
> > +	rc = cxl_port_parent_interleave(port, cxlr, &parent_iw, &parent_ig);
> > +	if (rc)
> > +		return rc;
> > +
> >  	child_iw = cxl_rr->nr_targets;
> >  	if (!ways_valid(child_iw)) {
> >  		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",
>