Re: [PATCH v2 13/20] cxl/region: Add region autodiscovery

[Dan Williams <dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> On Fri, 10 Feb 2023 01:06:39 -0800
> Dan Williams <dan.j.williams@intel.com> 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@samsung.com>
> > Link: https://lore.kernel.org/r/167564540972.847146.17096178433176097831.stgit@dwillia2-xfh.jf.intel.com
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> 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@huawei.com>

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;