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@xxxxxxxxx>
> > ---
> > 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.
