Jonathan Cameron wrote: > On Fri, 10 Feb 2023 01:06:39 -0800 > Dan Williams <dan.j.williams@xxxxxxxxx> wrote: > > > Region autodiscovery is an asynchronous state machine advanced by > > cxl_port_probe(). After the decoders on an endpoint port are enumerated > > they are scanned for actively enabled instances. Each active decoder is > > flagged for auto-assembly CXL_DECODER_F_AUTO and attached to a region. > > If a region does not already exist for the address range setting of the > > decoder one is created. That creation process may race with other > > decoders of the same region being discovered since cxl_port_probe() is > > asynchronous. A new 'struct cxl_root_decoder' lock, @range_lock, is > > introduced to mitigate that race. > > > > Once all decoders have arrived, "p->nr_targets == p->interleave_ways", > > they are sorted by their relative decode position. The sort algorithm > > involves finding the point in the cxl_port topology where one leg of the > > decode leads to deviceA and the other deviceB. At that point in the > > topology the target order in the 'struct cxl_switch_decoder' indicates > > the relative position of those endpoint decoders in the region. > > > > >From that point the region goes through the same setup and validation > Why the >? I believe this is auto-added by git send-email or public-inbox to make sure that a sentence that begins with "From" is not misinterpreted as a "From:" header. You can see this throughout the kernel commit history. In this case I pulled the patches back down from lore before editing them to collect review tags. > > steps as user-created regions, but instead of programming the decoders > > it validates that driver would have written the same values to the > > decoders as were already present. > > > > Tested-by: Fan Ni <fan.ni@xxxxxxxxxxx> > > Link: https://lore.kernel.org/r/167564540972.847146.17096178433176097831.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx > > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx> > > A few trivial things inline and this being complex code I'm not > as confident about it as the rest of the series but with that in mind > and the fact I didn't find anything that looked broken... > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > > ... > > > > > + > > +static int cxl_region_sort_targets(struct cxl_region *cxlr) > > +{ > > + struct cxl_region_params *p = &cxlr->params; > > + int i, rc = 0; > > + > > + sort(p->targets, p->nr_targets, sizeof(p->targets[0]), cmp_decode_pos, > > + NULL); > > + > > + for (i = 0; i < p->nr_targets; i++) { > > + struct cxl_endpoint_decoder *cxled = p->targets[i]; > > + > > + if (cxled->pos < 0) > > + rc = -ENXIO; > > If it makes sense to carry on after pos < 0 I'd like to see a comment here > on why. If not, nicer to have a separate dev_dbg() for failed case nad > direct return here. Ok, I'll add: /* * Record that sorting failed, but still continue to restore cxled->pos * with its ->targets[] position so that follow-on code paths can reliably * do p->targets[cxled->pos] to self-reference their entry. */ > > > + cxled->pos = i; > > + } > > + > > + dev_dbg(&cxlr->dev, "region sort %s\n", rc ? "failed" : "successful"); > > + return rc; > > +} > > + > > > + > > +int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled) > > +{ > > + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); > > + struct range *hpa = &cxled->cxld.hpa_range; > > + struct cxl_decoder *cxld = &cxled->cxld; > > + struct cxl_root_decoder *cxlrd; > > + struct cxl_region_params *p; > > + struct cxl_region *cxlr; > > + bool attach = false; > > + struct device *dev; > > + int rc; > > + > > + dev = device_find_child(&root->dev, &cxld->hpa_range, > > + match_decoder_by_range); > > + if (!dev) { > > + dev_err(cxlmd->dev.parent, > > + "%s:%s no CXL window for range %#llx:%#llx\n", > > + dev_name(&cxlmd->dev), dev_name(&cxld->dev), > > + cxld->hpa_range.start, cxld->hpa_range.end); > > + return -ENXIO; > > + } > > + > > + cxlrd = to_cxl_root_decoder(dev); > > + > > + /* > > + * Ensure that if multiple threads race to construct_region() for @hpa > > + * one does the construction and the others add to that. > > + */ > > + mutex_lock(&cxlrd->range_lock); > > + dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa, > > + match_region_by_range); > > + if (!dev) > > + cxlr = construct_region(cxlrd, cxled); > > + else > > + cxlr = to_cxl_region(dev); > > + mutex_unlock(&cxlrd->range_lock); > > + > > + if (IS_ERR(cxlr)) { > > + rc = PTR_ERR(cxlr); > > + goto out; > > + } > > + > > + attach_target(cxlr, cxled, -1, TASK_UNINTERRUPTIBLE); > > + > > + down_read(&cxl_region_rwsem); > > + p = &cxlr->params; > > + attach = p->state == CXL_CONFIG_COMMIT; > > + up_read(&cxl_region_rwsem); > > + > > + if (attach) { > > + int rc = device_attach(&cxlr->dev); > > Shadowing int rc isn't great for readability. Just call it rc2 or something :) > Or given you don't make use of the value... 0day did not like this either... > > /* > * If device_attach() fails the range may still be active via > * the platform-firmware memory map, otherwise the driver for > * regions is local to this file, so driver matching can't fail > + * and hence device_attach() cannot return 1. > > //very much not obvious otherwise to anyone who isn't far too familiar with device_attach() Hence the comment? Not sure what else can be said here about why device_attach() < 0 is a sufficient check. > > */ > if (device_attach(&cxlr->dev) < 0) > dev_err() > > + > > + /* > > + * If device_attach() fails the range may still be active via > > + * the platform-firmware memory map, otherwise the driver for > > + * regions is local to this file, so driver matching can't fail. > > + */ > > + if (rc < 0) > > + dev_err(&cxlr->dev, "failed to enable, range: %pr\n", > > + p->res); > > + } > > + > > + put_device(&cxlr->dev); > > +out: > > + put_device(&cxlrd->cxlsd.cxld.dev); > > Moderately horrible. Maybe just keep an extra local variable around for the first > use of struct device *dev? or maybe add a put_cxl_root_decoder() helper? > > There are lots of other deep structure access like this I guess, so I don't mind > if you just leave this as yet another one. Yeah, it's difficult to have symmetry here, but I think I'll switch to using an @cxlrd_dev variable so better match the get with the put. > > > > + return rc; > > +} > > +EXPORT_SYMBOL_NS_GPL(cxl_add_to_region, CXL); > > ... > > > diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c > > index a8d46a67b45e..d88518836c2d 100644 > > --- a/drivers/cxl/port.c > > +++ b/drivers/cxl/port.c > > @@ -30,6 +30,34 @@ static void schedule_detach(void *cxlmd) > > schedule_cxl_memdev_detach(cxlmd); > > } > > > > +static int discover_region(struct device *dev, void *root) > > +{ > > + struct cxl_endpoint_decoder *cxled; > > + int rc; > > + > > + if (!is_endpoint_decoder(dev)) > > + return 0; > > + > > + cxled = to_cxl_endpoint_decoder(dev); > > + if ((cxled->cxld.flags & CXL_DECODER_F_ENABLE) == 0) > > + return 0; > > + > > + if (cxled->state != CXL_DECODER_STATE_AUTO) > > + return 0; > > + > > + /* > > + * Region enumeration is opportunistic, if this add-event fails, > > + * continue to the next endpoint decoder. > > + */ > > + rc = cxl_add_to_region(root, cxled); > > + if (rc) > > + dev_dbg(dev, "failed to add to region: %#llx-%#llx\n", > > + cxled->cxld.hpa_range.start, cxled->cxld.hpa_range.end); > > + > > + return 0; > > +} > > + > > + > > Two blank lines? Just stashing this here so I can introduce a spurious whitespace removal in the next patch. Will clean up.
