| Message ID | 167564538779.847146.8356062886811511706.stgit@dwillia2-xfh.jf.intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | CXL RAM and the 'Soft Reserved' => 'System RAM' default | expand |
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? Ira
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.
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. > @@ -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; > } > >
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.
Jonathan Cameron wrote: [..] > > @@ -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. I played with this a bit and the only way I could see to make the diff come out significantly nicer would be to use a forward declaration to move the new helpers below cxl_region_attach().
On Wed, 8 Feb 2023 20:26:26 -0800 Dan Williams <dan.j.williams@intel.com> wrote: > Jonathan Cameron wrote: > [..] > > > @@ -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. > > I played with this a bit and the only way I could see to make the diff > come out significantly nicer would be to use a forward declaration to > move the new helpers below cxl_region_attach(). Don't bother then! Unless you've already done it. In the ideal world diff would magically present everything in the most human readable form :) What are all these AI folk working on that we don't already have this! Jonathan
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@intel.com> > --- > drivers/cxl/core/region.c | 119 +++++++++++++++++++++++++++++---------------- > 1 file changed, 76 insertions(+), 43 deletions(-) Looks good, Reviewed-by: Vishal Verma <vishal.l.verma@intel.com> > > 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; > } > >
Jonathan Cameron wrote: > On Wed, 8 Feb 2023 20:26:26 -0800 > Dan Williams <dan.j.williams@intel.com> wrote: > > > Jonathan Cameron wrote: > > [..] > > > > @@ -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. > > > > I played with this a bit and the only way I could see to make the diff > > come out significantly nicer would be to use a forward declaration to > > move the new helpers below cxl_region_attach(). > > Don't bother then! Unless you've already done it. No worries, abandoned. > In the ideal world diff would magically present everything in the most > human readable form :) What are all these AI folk working on that we > don't already have this! +1 to this.
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; }
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(-)