Re: [PATCH for-next] cxl/core: Consolidate auto region attachment logic

[Jonathan Cameron]

On Tue,  3 Jun 2025 13:36:45 +0800
Li Zhijian <lizhijian@xxxxxxxxxxx> wrote:

> Move all auto-attach handling from cxl_region_attach() into the
> cxl_region_attach_auto() function. This combines the partial handling
> previously in cxl_region_attach_auto() with the remaining logic that
> was directly implemented in cxl_region_attach().
> 
> Specifically, cxl_region_attach_auto() now handles:
> - Adding new targets when in auto-discovery mode
> - Waiting for all required targets
> - Sorting and validating targets when ready
> 
> This improves code organization by:
> - Keeping all auto-attach logic in a single function
> - Reducing complexity in the main attach function
> - Making the control flow easier to follow
> 
> No functional change intended.
> 
> Signed-off-by: Li Zhijian <lizhijian@xxxxxxxxxxx>
Hi.

One minor comment inline.  More generally I don't feel that strongly
either way on this one.  Lets see if others think the improvements
are worth the churn.

Jonathan

> ---
>  drivers/cxl/core/region.c | 164 +++++++++++++++++++-------------------
>  1 file changed, 82 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index c3f4dc244df7..e7618d59b548 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1702,44 +1702,6 @@ static int cxl_region_attach_position(struct cxl_region *cxlr,
>  	return rc;
>  }
>  
> -static int cxl_region_attach_auto(struct cxl_region *cxlr,
> -				  struct cxl_endpoint_decoder *cxled, int pos)
> -{
> -	struct cxl_region_params *p = &cxlr->params;
> -
> -	if (cxled->state != CXL_DECODER_STATE_AUTO) {
> -		dev_err(&cxlr->dev,
> -			"%s: unable to add decoder to autodetected region\n",
> -			dev_name(&cxled->cxld.dev));
> -		return -EINVAL;
> -	}
> -
> -	if (pos >= 0) {
> -		dev_dbg(&cxlr->dev, "%s: expected auto position, not %d\n",
> -			dev_name(&cxled->cxld.dev), pos);
> -		return -EINVAL;
> -	}
> -
> -	if (p->nr_targets >= p->interleave_ways) {
> -		dev_err(&cxlr->dev, "%s: no more target slots available\n",
> -			dev_name(&cxled->cxld.dev));
> -		return -ENXIO;
> -	}
> -
> -	/*
> -	 * Temporarily record the endpoint decoder into the target array. Yes,
> -	 * this means that userspace can view devices in the wrong position
> -	 * before the region activates, and must be careful to understand when
> -	 * it might be racing region autodiscovery.
> -	 */
> -	pos = p->nr_targets;
> -	p->targets[pos] = cxled;
> -	cxled->pos = pos;
> -	p->nr_targets++;
> -
> -	return 0;
> -}
> -
>  static int cmp_interleave_pos(const void *a, const void *b)
>  {
>  	struct cxl_endpoint_decoder *cxled_a = *(typeof(cxled_a) *)a;
> @@ -1905,6 +1867,86 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
>  	return rc;
>  }
>  
> +static int cxl_region_attach_auto(struct cxl_region *cxlr,
> +				  struct cxl_endpoint_decoder *cxled, int pos)
> +{
> +	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> +	struct cxl_region_params *p = &cxlr->params;
> +	struct cxl_port *root_port;
> +	int i, rc;
> +
> +	if (cxled->state != CXL_DECODER_STATE_AUTO) {
> +		dev_err(&cxlr->dev,
> +			"%s: unable to add decoder to autodetected region\n",
> +			dev_name(&cxled->cxld.dev));
> +		return -EINVAL;
> +	}
> +
> +	if (pos >= 0) {
> +		dev_dbg(&cxlr->dev, "%s: expected auto position, not %d\n",
> +			dev_name(&cxled->cxld.dev), pos);
> +		return -EINVAL;
> +	}
> +
> +	if (p->nr_targets >= p->interleave_ways) {
> +		dev_err(&cxlr->dev, "%s: no more target slots available\n",
> +			dev_name(&cxled->cxld.dev));
> +		return -ENXIO;
> +	}
> +
> +	/*
> +	 * Temporarily record the endpoint decoder into the target array. Yes,
> +	 * this means that userspace can view devices in the wrong position
> +	 * before the region activates, and must be careful to understand when
> +	 * it might be racing region autodiscovery.
> +	 */
> +	pos = p->nr_targets;
> +	p->targets[pos] = cxled;
> +	cxled->pos = pos;
> +	p->nr_targets++;
> +
> +	/* await more targets to arrive... */
> +	if (p->nr_targets < p->interleave_ways)
> +		return 0;
> +
> +	/*
> +	 * All targets are here, which implies all PCI enumeration that
> +	 * affects this region has been completed. Walk the topology to
> +	 * sort the devices into their relative region decode position.
> +	 */
> +	rc = cxl_region_sort_targets(cxlr);
> +	if (rc)
> +		return rc;
> +
> +	root_port = cxlrd_to_port(cxlrd);
> +	for (i = 0; i < p->nr_targets; i++) {
> +		struct cxl_port *ep_port;
> +		struct cxl_dport *dport;
> +
> +		cxled = p->targets[i];

I don't much like this reuse of the cxled here given it's a totally different
one to the code above (which was the parameter passed in).

Maybe just use
		ep_port = cxled_to_port(p->targets[i]);
...
		rc = cxl_region_attach_popsition(cxlr, cxlrd, p->targets[i],
						 dport, i);

or call it target_cxled or similar.

 
> +		ep_port = cxled_to_port(cxled);
> +		dport = cxl_find_dport_by_dev(root_port,
> +					      ep_port->host_bridge);
> +		rc = cxl_region_attach_position(cxlr, cxlrd, cxled,
> +						dport, i);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	rc = cxl_region_setup_targets(cxlr);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * If target setup succeeds in the autodiscovery case
> +	 * then the region is already committed.
> +	 */
> +	p->state = CXL_CONFIG_COMMIT;
> +	cxl_region_shared_upstream_bandwidth_update(cxlr);
> +
> +	return 0;
> +}
> +
>  static int cxl_region_attach(struct cxl_region *cxlr,
>  			     struct cxl_endpoint_decoder *cxled, int pos)
>  {
> @@ -1986,50 +2028,8 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  
>  	cxl_region_perf_data_calculate(cxlr, cxled);
>  
> -	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> -		int i;
> -
> -		rc = cxl_region_attach_auto(cxlr, cxled, pos);
> -		if (rc)
> -			return rc;
> -
> -		/* await more targets to arrive... */
> -		if (p->nr_targets < p->interleave_ways)
> -			return 0;
> -
> -		/*
> -		 * All targets are here, which implies all PCI enumeration that
> -		 * affects this region has been completed. Walk the topology to
> -		 * sort the devices into their relative region decode position.
> -		 */
> -		rc = cxl_region_sort_targets(cxlr);
> -		if (rc)
> -			return rc;
> -
> -		for (i = 0; i < p->nr_targets; i++) {
> -			cxled = p->targets[i];
> -			ep_port = cxled_to_port(cxled);
> -			dport = cxl_find_dport_by_dev(root_port,
> -						      ep_port->host_bridge);
> -			rc = cxl_region_attach_position(cxlr, cxlrd, cxled,
> -							dport, i);
> -			if (rc)
> -				return rc;
> -		}
> -
> -		rc = cxl_region_setup_targets(cxlr);
> -		if (rc)
> -			return rc;
> -
> -		/*
> -		 * If target setup succeeds in the autodiscovery case
> -		 * then the region is already committed.
> -		 */
> -		p->state = CXL_CONFIG_COMMIT;
> -		cxl_region_shared_upstream_bandwidth_update(cxlr);
> -
> -		return 0;
> -	}
> +	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags))
> +		return cxl_region_attach_auto(cxlr, cxled, pos);
>  
>  	rc = cxl_region_validate_position(cxlr, cxled, pos);
>  	if (rc)