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

[Dan Williams <dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> 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.

I'll clean that up with a lead-in fixup.