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 >? > > 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> Folded the following: diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 3f6453da2c51..1580170d5bdb 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -2479,16 +2479,16 @@ 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 device *cxlrd_dev, *region_dev; 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) { + cxlrd_dev = device_find_child(&root->dev, &cxld->hpa_range, + match_decoder_by_range); + if (!cxlrd_dev) { dev_err(cxlmd->dev.parent, "%s:%s no CXL window for range %#llx:%#llx\n", dev_name(&cxlmd->dev), dev_name(&cxld->dev), @@ -2496,19 +2496,20 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled) return -ENXIO; } - cxlrd = to_cxl_root_decoder(dev); + cxlrd = to_cxl_root_decoder(cxlrd_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) + region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa, + match_region_by_range); + if (!region_dev) { cxlr = construct_region(cxlrd, cxled); - else - cxlr = to_cxl_region(dev); + region_dev = &cxlr->dev; + } else + cxlr = to_cxl_region(region_dev); mutex_unlock(&cxlrd->range_lock); if (IS_ERR(cxlr)) { @@ -2524,21 +2525,19 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled) up_read(&cxl_region_rwsem); if (attach) { - int rc = device_attach(&cxlr->dev); - /* * 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) + if (device_attach(&cxlr->dev) < 0) dev_err(&cxlr->dev, "failed to enable, range: %pr\n", p->res); } - put_device(&cxlr->dev); + put_device(region_dev); out: - put_device(&cxlrd->cxlsd.cxld.dev); + put_device(cxlrd_dev); return rc; } EXPORT_SYMBOL_NS_GPL(cxl_add_to_region, CXL); diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c index d88518836c2d..d6c151dabaa7 100644 --- a/drivers/cxl/port.c +++ b/drivers/cxl/port.c @@ -57,7 +57,6 @@ static int discover_region(struct device *dev, void *root) return 0; } - static int cxl_switch_port_probe(struct cxl_port *port) { struct cxl_hdm *cxlhdm;
