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

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

On Tue, Jun 03, 2025 at 01:36:45PM +0800, Li Zhijian 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.
> 

While the intent to clean up and consolidate the auto-attach logic
is understandable, I think it's best to avoid refactoring purely for
structural clarity unless it's in support of a bug fix, new feature,
or needed to resolve a real maintenance burden.

If we accept pure cleanup patches and move things around for no real
reason, it just makes it harder to read diffs, find bugs, and maintain
code. It also consumes reviewer time.

NAK
-- Alison

> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>  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];
> +		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)
> -- 
> 2.41.0
>