Re: [PATCH 07/18] cxl/region: Move region-position validation to a helper

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Sun, 05 Feb 2023 17:03:07 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> In preparation for region autodiscovery, that needs all devices
> discovered before their relative position in the region can be
> determined, consolidate all position dependent validation in a helper.
> 
> Recall that in the on-demand region creation flow the end-user picks the
> position of a given endpoint decoder in a region. In the autodiscovery
> case the position of an endpoint decoder can only be determined after
> all other endpoint decoders that claim to decode the region's address
> range have been enumerated and attached. So, in the autodiscovery case
> endpoint decoders may be attached before their relative position is
> known. Once all decoders arrive, then positions can be determined and
> validated with cxl_region_validate_position() the same as user initiated
> on-demand creation.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Hi Dan,

A few comments inline, but mostly reflect the original code rather than
the refactoring you have done in this patch.

Jonathan


> +static int cxl_region_attach(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_memdev *cxlmd = cxled_to_memdev(cxled);
> +	struct cxl_region_params *p = &cxlr->params;
> +	struct cxl_port *ep_port, *root_port;
> +	struct cxl_dport *dport;
> +	int rc = -ENXIO;
> +
> +	if (cxled->mode != cxlr->mode) {
> +		dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n",
> +			dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode);
> +		return -EINVAL;
> +	}
> +
> +	if (cxled->mode == CXL_DECODER_DEAD) {
> +		dev_dbg(&cxlr->dev, "%s dead\n", dev_name(&cxled->cxld.dev));
> +		return -ENODEV;
> +	}
> +
> +	/* all full of members, or interleave config not established? */
> +	if (p->state > CXL_CONFIG_INTERLEAVE_ACTIVE) {
> +		dev_dbg(&cxlr->dev, "region already active\n");
> +		return -EBUSY;
> +	} else if (p->state < CXL_CONFIG_INTERLEAVE_ACTIVE) {
> +		dev_dbg(&cxlr->dev, "interleave config missing\n");
> +		return -ENXIO;
> +	}
> +
>  	ep_port = cxled_to_port(cxled);
>  	root_port = cxlrd_to_port(cxlrd);
>  	dport = cxl_find_dport_by_dev(root_port, ep_port->host_bridge);
> @@ -1284,13 +1327,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		return -ENXIO;
>  	}
>  
> -	if (cxlrd->calc_hb(cxlrd, pos) != dport) {
> -		dev_dbg(&cxlr->dev, "%s:%s invalid target position for %s\n",
> -			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> -			dev_name(&cxlrd->cxlsd.cxld.dev));
> -		return -ENXIO;
> -	}
> -

In an ideal world, this would have been nice as two patches.
One that reorders the various checks so that they are in the order
after you have factored things out (easy to review for correctness)
then one that factored it out.

>  	if (cxled->cxld.target_type != cxlr->type) {
>  		dev_dbg(&cxlr->dev, "%s:%s type mismatch: %d vs %d\n",
>  			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> @@ -1314,12 +1350,13 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		return -EINVAL;
>  	}
>  
> -	for (iter = ep_port; !is_cxl_root(iter);
> -	     iter = to_cxl_port(iter->dev.parent)) {
> -		rc = cxl_port_attach_region(iter, cxlr, cxled, pos);
> -		if (rc)
> -			goto err;
> -	}
> +	rc = cxl_region_validate_position(cxlr, cxled, pos);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_region_attach_position(cxlr, cxlrd, cxled, dport, pos);
> +	if (rc)
> +		return rc;
>  
>  	p->targets[pos] = cxled;
>  	cxled->pos = pos;

More something about original code than the refactoring...

I'm not keen on the side effects that aren't unwound in the error paths.

p->targets[pos] and cxled->pos are left set.  Probably never matters
but not elegant or as easy to reason about as it would be if they
were cleared in error cases.  In particular there is a check on
whether p->targets[pos] is set that will result in a dev_dbg even
though setting it up actually failed.


> @@ -1343,10 +1380,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  
>  err_decrement:
>  	p->nr_targets--;
> -err:
> -	for (iter = ep_port; !is_cxl_root(iter);
> -	     iter = to_cxl_port(iter->dev.parent))
> -		cxl_port_detach_region(iter, cxlr, cxled);
>  	return rc;
>  }
>  
>