On Sun, 2023-02-05 at 17:03 -0800, 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(-)
Looks good,
Reviewed-by: Vishal Verma <vishal.l.verma@xxxxxxxxx>
>
> 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;
> }
>
> -static int cxl_region_attach(struct cxl_region *cxlr,
> - struct cxl_endpoint_decoder *cxled, int pos)
> +static int cxl_region_validate_position(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_port *ep_port, *root_port, *iter;
> struct cxl_region_params *p = &cxlr->params;
> - struct cxl_dport *dport;
> - int i, 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;
> - }
> + int i;
>
> if (pos < 0 || pos >= p->interleave_ways) {
> dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos,
> @@ -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;
> + }
> +
> + for (iter = cxled_to_port(cxled); !is_cxl_root(iter);
> + iter = to_cxl_port(iter->dev.parent)) {
> + rc = cxl_port_attach_region(iter, cxlr, cxled, pos);
> + if (rc)
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + for (iter = cxled_to_port(cxled); !is_cxl_root(iter);
> + iter = to_cxl_port(iter->dev.parent))
> + cxl_port_detach_region(iter, cxlr, cxled);
> + return rc;
> +}
> +
> +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;
> - }
> -
> 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;
> @@ -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;
> }
>
>
