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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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;
>  }
>  
> 





[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]
  Powered by Linux