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

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

Ira Weiny wrote:
> Dan Williams 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>
> > ---
> >  drivers/cxl/core/region.c |  119 +++++++++++++++++++++++++++++----------------
> >  1 file changed, 76 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 97eafdd75675..c82d3b6f3d1f 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1207,35 +1207,13 @@ static int cxl_region_setup_targets(struct cxl_region *cxlr)
> >  	return 0;
> >  }
> >  
> 
> [snip]
> 
> > @@ -1274,6 +1252,71 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> >  		}
> >  	}
> >  
> > +	return 0;
> > +}
> > +
> > +static int cxl_region_attach_position(struct cxl_region *cxlr,
> > +				      struct cxl_root_decoder *cxlrd,
> > +				      struct cxl_endpoint_decoder *cxled,
> > +				      const struct cxl_dport *dport, int pos)
> > +{
> > +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > +	struct cxl_port *iter;
> > +	int rc;
> > +
> > +	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;
> > +	}
> 
> I think I know the answer but I'm curious why this check is not part of
> validating the position?  Is it because this is validating the position
> relative to the hostbridge which is not strictly part of the region
> validation?

cxl_region_validate_position() is just doing the basic checks like
preventing assigning 2 decoders to the same position, or assigning a
device twice to the same region.

The checks in cxl_region_attach_position() and cxl_port_attach_region()
are more about whether that device can be in that position relative to
the hardware topology.

I think this split makes more sense in the follow on patch where you see
that cxl_region_attach_position() is done after sorting while
cxl_region_attach_auto() replaces cxl_region_validate_position() since
the latter is validating user input and the former is reacting to what
platform-firmware already successfully programmed.