Re: [PATCH v2] cxl/region: Refactor logic around check_last_peer()

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

On Wed, Nov 01, 2023 at 05:02:18AM +0000, Jim Harris wrote:
> 'distance' is equivalent to the product of the interleave_ways of the
> "ancestor" decoders of the port's decoder we are setting up in
> cxl_port_setup_targets().
> 
> So use the term "ancestral_ways" instead of "distance" to better
> clarify the meaning of this value. Also move all logic around this
> value directly into check_last_peer() so that all necessary calculations
> are in one place. It also allows eliminating a parameter to that
> function.

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

A couple of housekeeping notes:
- Use get_maintainers script to find the To: List
- It's customary to send revised patches as a new thread, not as a
  reply to previous version.

Thanks,
Alison


> 
> Signed-off-by: Jim Harris <jim.harris@samsung.com>
> ---
>  drivers/cxl/core/region.c |   33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index a1eac592c66a..2edfc02bb766 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1037,8 +1037,7 @@ static void cxl_port_detach_region(struct cxl_port *port,
>  }
>  
>  static int check_last_peer(struct cxl_endpoint_decoder *cxled,
> -			   struct cxl_ep *ep, struct cxl_region_ref *cxl_rr,
> -			   int distance)
> +			   struct cxl_ep *ep, struct cxl_region_ref *cxl_rr)
>  {
>  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>  	struct cxl_region *cxlr = cxl_rr->region;
> @@ -1048,19 +1047,30 @@ static int check_last_peer(struct cxl_endpoint_decoder *cxled,
>  	struct cxl_memdev *cxlmd_peer;
>  	struct cxl_ep *ep_peer;
>  	int pos = cxled->pos;
> +	int ancestral_ways;
> +
> +	if (cxl_rr->nr_targets == 1) {
> +		/*
> +		 * Passthrough decoders impose no positioning requirements
> +		 * between peers.
> +		 */
> +		return 0;
> +	}
> +
> +	ancestral_ways = p->nr_targets / cxl_rr->nr_targets;
>  
>  	/*
>  	 * If this position wants to share a dport with the last endpoint mapped
> -	 * then that endpoint, at index 'position - distance', must also be
> +	 * then that endpoint, at index 'position - ancestral_ways', must also be
>  	 * mapped by this dport.
>  	 */
> -	if (pos < distance) {
> +	if (pos < ancestral_ways) {
>  		dev_dbg(&cxlr->dev, "%s:%s: cannot host %s:%s at %d\n",
>  			dev_name(port->uport_dev), dev_name(&port->dev),
>  			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), pos);
>  		return -ENXIO;
>  	}
> -	cxled_peer = p->targets[pos - distance];
> +	cxled_peer = p->targets[pos - ancestral_ways];
>  	cxlmd_peer = cxled_to_memdev(cxled_peer);
>  	ep_peer = cxl_ep_load(port, cxlmd_peer);
>  	if (ep->dport != ep_peer->dport) {
> @@ -1105,20 +1115,15 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  
>  	cxlsd = to_cxl_switch_decoder(&cxld->dev);
>  	if (cxl_rr->nr_targets_set) {
> -		int i, distance;
> +		int i;
>  
>  		/*
> -		 * Passthrough decoders impose no distance requirements between
> -		 * peers
> +		 * Check if this ep is already in the switch target list and
> +		 * if so ensure it meets relative positioning requirements.
>  		 */
> -		if (cxl_rr->nr_targets == 1)
> -			distance = 0;
> -		else
> -			distance = p->nr_targets / cxl_rr->nr_targets;
>  		for (i = 0; i < cxl_rr->nr_targets_set; i++)
>  			if (ep->dport == cxlsd->target[i]) {
> -				rc = check_last_peer(cxled, ep, cxl_rr,
> -						     distance);
> +				rc = check_last_peer(cxled, ep, cxl_rr);
>  				if (rc)
>  					return rc;
>  				goto out_target_set;
>