Jonathan Cameron wrote:
> On Sun, 05 Feb 2023 17:03:07 -0800
> Dan Williams <dan.j.williams@xxxxxxxxx> 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>
> 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.
