[PATCH v2 0/3] cxl/port: Cleanup add_port_attach_ep() "cleanup" confusion

[PATCH v2 0/3] cxl/port: Cleanup add_port_attach_ep() "cleanup" confusion

[Dan Williams]

Changes since v1 [1]:
- there is no bug fixed by these changes (Jonathan)
- lets take the time to improve the subtlety (Jonathan)

Dan's recent smatch report [2] highlighted that add_port_attach_ep() is
unreasonably subtle, aka ugly. The recent cleanup conversions did not
improve the situation and instead actively contributed to the poor
readability.

Given that this routine is core to CXL port enumeration it is worth
investing readability improvements. So, first, back out the
scope-based-cleanup helper conversions that were built on an ugly
foundation. Then, refactor add_port_attach_ep() with a new
find_add_dport() helper that clarifies the flow.

This is cxl-next material, not cxl-fixes.

[1]: http://lore.kernel.org/172912104335.1414627.1377616790909088415.stgit@dwillia2-xfh.jf.intel.com
[2]: http://lore.kernel.org/2a19289b-0bcf-42c4-82a9-268a922535f2@stanley.mountain

---

Dan Williams (3):
      cxl/port: Revert usage of scoped_guard()
      cxl/port: Revert __free() conversion of add_port_attach_ep()
      cxl/port: Clarify add_port_attach_ep()


 drivers/cxl/core/port.c   |  121 ++++++++++++++++++++++++++-------------------
 drivers/cxl/core/region.c |   25 ++++-----
 drivers/cxl/cxl.h         |    1 
 drivers/cxl/mem.c         |   22 ++++----
 drivers/cxl/pmem.c        |   16 +++---
 5 files changed, 105 insertions(+), 80 deletions(-)

[PATCH v2 1/3] cxl/port: Revert usage of scoped_guard()

[Dan Williams]

Dan points out via smatch [1] that the conversion of
add_port_attach_ep() to use cleanup helpers gives the appearance of not
correctly handling a pointer reassignment case.

        drivers/cxl/core/port.c:1591 add_port_attach_ep()
        warn: re-assigning __cleanup__ ptr 'port'

The conversion to scoped_guard() complicates rather than simplifies code
readability. It turns out that there is no bug in practice because the
reassignment still results in correct reference accounting, but that
deserves a separate fix.

For now, just revert the scoped_guard() conversions in preparation for
refactoring the subtlety out of this routine.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Link: http://lore.kernel.org/2a19289b-0bcf-42c4-82a9-268a922535f2@stanley.mountain [1]
Cc: Li Ming <ming4.li@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/port.c   |   72 +++++++++++++++++++++++----------------------
 drivers/cxl/core/region.c |   25 ++++++++--------
 drivers/cxl/mem.c         |   22 ++++++++------
 drivers/cxl/pmem.c        |   16 ++++++----
 4 files changed, 70 insertions(+), 65 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index b7828b6c7826..54dd2cd450ca 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1399,14 +1399,14 @@ static void delete_endpoint(void *data)
 	struct cxl_port *endpoint = cxlmd->endpoint;
 	struct device *host = endpoint_host(endpoint);
 
-	scoped_guard(device, host) {
-		if (host->driver && !endpoint->dead) {
-			devm_release_action(host, cxl_unlink_parent_dport, endpoint);
-			devm_release_action(host, cxl_unlink_uport, endpoint);
-			devm_release_action(host, unregister_port, endpoint);
-		}
-		cxlmd->endpoint = NULL;
+	device_lock(host);
+	if (host->driver && !endpoint->dead) {
+		devm_release_action(host, cxl_unlink_parent_dport, endpoint);
+		devm_release_action(host, cxl_unlink_uport, endpoint);
+		devm_release_action(host, unregister_port, endpoint);
 	}
+	cxlmd->endpoint = NULL;
+	device_unlock(host);
 	put_device(&endpoint->dev);
 	put_device(host);
 }
@@ -1571,38 +1571,40 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 	 * dereferencing the device of the port before the parent_port releasing.
 	 */
 	struct cxl_port *port __free(put_cxl_port) = NULL;
-	scoped_guard(device, &parent_port->dev) {
-		if (!parent_port->dev.driver) {
-			dev_warn(&cxlmd->dev,
-				 "port %s:%s disabled, failed to enumerate CXL.mem\n",
-				 dev_name(&parent_port->dev), dev_name(uport_dev));
-			return -ENXIO;
-		}
-
-		port = find_cxl_port_at(parent_port, dport_dev, &dport);
-		if (!port) {
-			component_reg_phys = find_component_registers(uport_dev);
-			port = devm_cxl_add_port(&parent_port->dev, uport_dev,
-						 component_reg_phys, parent_dport);
-			if (IS_ERR(port))
-				return PTR_ERR(port);
+	device_lock(&parent_port->dev);
+	if (!parent_port->dev.driver) {
+		dev_warn(&cxlmd->dev,
+			 "port %s:%s disabled, failed to enumerate CXL.mem\n",
+			 dev_name(&parent_port->dev), dev_name(uport_dev));
+		port = ERR_PTR(-ENXIO);
+		goto out;
+	}
 
-			/* retry find to pick up the new dport information */
+	port = find_cxl_port_at(parent_port, dport_dev, &dport);
+	if (!port) {
+		component_reg_phys = find_component_registers(uport_dev);
+		port = devm_cxl_add_port(&parent_port->dev, uport_dev,
+					 component_reg_phys, parent_dport);
+		/* retry find to pick up the new dport information */
+		if (!IS_ERR(port))
 			port = find_cxl_port_at(parent_port, dport_dev, &dport);
-			if (!port)
-				return -ENXIO;
-		}
 	}
+out:
+	device_unlock(&parent_port->dev);
 
-	dev_dbg(&cxlmd->dev, "add to new port %s:%s\n",
-		dev_name(&port->dev), dev_name(port->uport_dev));
-	rc = cxl_add_ep(dport, &cxlmd->dev);
-	if (rc == -EBUSY) {
-		/*
-		 * "can't" happen, but this error code means
-		 * something to the caller, so translate it.
-		 */
-		rc = -ENXIO;
+	if (IS_ERR(port))
+		rc = PTR_ERR(port);
+	else {
+		dev_dbg(&cxlmd->dev, "add to new port %s:%s\n",
+			dev_name(&port->dev), dev_name(port->uport_dev));
+		rc = cxl_add_ep(dport, &cxlmd->dev);
+		if (rc == -EBUSY) {
+			/*
+			 * "can't" happen, but this error code means
+			 * something to the caller, so translate it.
+			 */
+			rc = -ENXIO;
+		}
 	}
 
 	return rc;
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index dff618c708dc..77b301b0d269 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3083,11 +3083,11 @@ static void cxlr_release_nvdimm(void *_cxlr)
 	struct cxl_region *cxlr = _cxlr;
 	struct cxl_nvdimm_bridge *cxl_nvb = cxlr->cxl_nvb;
 
-	scoped_guard(device, &cxl_nvb->dev) {
-		if (cxlr->cxlr_pmem)
-			devm_release_action(&cxl_nvb->dev, cxlr_pmem_unregister,
-					    cxlr->cxlr_pmem);
-	}
+	device_lock(&cxl_nvb->dev);
+	if (cxlr->cxlr_pmem)
+		devm_release_action(&cxl_nvb->dev, cxlr_pmem_unregister,
+				    cxlr->cxlr_pmem);
+	device_unlock(&cxl_nvb->dev);
 	cxlr->cxl_nvb = NULL;
 	put_device(&cxl_nvb->dev);
 }
@@ -3123,14 +3123,13 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
 	dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent),
 		dev_name(dev));
 
-	scoped_guard(device, &cxl_nvb->dev) {
-		if (cxl_nvb->dev.driver)
-			rc = devm_add_action_or_reset(&cxl_nvb->dev,
-						      cxlr_pmem_unregister,
-						      cxlr_pmem);
-		else
-			rc = -ENXIO;
-	}
+	device_lock(&cxl_nvb->dev);
+	if (cxl_nvb->dev.driver)
+		rc = devm_add_action_or_reset(&cxl_nvb->dev,
+					      cxlr_pmem_unregister, cxlr_pmem);
+	else
+		rc = -ENXIO;
+	device_unlock(&cxl_nvb->dev);
 
 	if (rc)
 		goto err_bridge;
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index a9fd5cd5a0d2..6daca88845ca 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -168,18 +168,20 @@ static int cxl_mem_probe(struct device *dev)
 
 	cxl_dport_init_ras_reporting(dport, dev);
 
-	scoped_guard(device, endpoint_parent) {
-		if (!endpoint_parent->driver) {
-			dev_err(dev, "CXL port topology %s not enabled\n",
-				dev_name(endpoint_parent));
-			return -ENXIO;
-		}
-
-		rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
-		if (rc)
-			return rc;
+	device_lock(endpoint_parent);
+	if (!endpoint_parent->driver) {
+		dev_err(dev, "CXL port topology %s not enabled\n",
+			dev_name(endpoint_parent));
+		rc = -ENXIO;
+		goto unlock;
 	}
 
+	rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
+unlock:
+	device_unlock(endpoint_parent);
+	if (rc)
+		return rc;
+
 	/*
 	 * The kernel may be operating out of CXL memory on this device,
 	 * there is no spec defined way to determine whether this device
diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
index d2d43a4fc053..94ce71952447 100644
--- a/drivers/cxl/pmem.c
+++ b/drivers/cxl/pmem.c
@@ -237,13 +237,15 @@ static int detach_nvdimm(struct device *dev, void *data)
 	if (!is_cxl_nvdimm(dev))
 		return 0;
 
-	scoped_guard(device, dev) {
-		if (dev->driver) {
-			cxl_nvd = to_cxl_nvdimm(dev);
-			if (cxl_nvd->cxlmd && cxl_nvd->cxlmd->cxl_nvb == data)
-				release = true;
-		}
-	}
+	device_lock(dev);
+	if (!dev->driver)
+		goto out;
+
+	cxl_nvd = to_cxl_nvdimm(dev);
+	if (cxl_nvd->cxlmd && cxl_nvd->cxlmd->cxl_nvb == data)
+		release = true;
+out:
+	device_unlock(dev);
 	if (release)
 		device_release_driver(dev);
 	return 0;

Re: [PATCH v2 1/3] cxl/port: Revert usage of scoped_guard()

[Ira Weiny]

Dan Williams wrote:
> Dan points out via smatch [1] that the conversion of
> add_port_attach_ep() to use cleanup helpers gives the appearance of not
> correctly handling a pointer reassignment case.
> 
>         drivers/cxl/core/port.c:1591 add_port_attach_ep()
>         warn: re-assigning __cleanup__ ptr 'port'
> 
> The conversion to scoped_guard() complicates rather than simplifies code
> readability. It turns out that there is no bug in practice because the
> reassignment still results in correct reference accounting, but that
> deserves a separate fix.
> 
> For now, just revert the scoped_guard() conversions in preparation for
> refactoring the subtlety out of this routine.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Link: http://lore.kernel.org/2a19289b-0bcf-42c4-82a9-268a922535f2@stanley.mountain [1]
> Cc: Li Ming <ming4.li@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

[PATCH v2 2/3] cxl/port: Revert __free() conversion of add_port_attach_ep()

[Dan Williams]

Dan points out via smatch [1] that the conversion of
add_port_attach_ep() to use cleanup helpers does appears to not
correctly handle a pointer reassignment case.

    drivers/cxl/core/port.c:1591 add_port_attach_ep()
    warn: re-assigning __cleanup__ ptr 'port'

The reassignment still results in the correct cleanup and reference
counting, but it is not immediately obvious from the code.

Revert the __free() conversion in preparation for refactoring the
subtlety out of this function.

Cc: Dan Carpenter <dan.carpenter@linaro.org>
Link: http://lore.kernel.org/2a19289b-0bcf-42c4-82a9-268a922535f2@stanley.mountain [1]
Cc: Li Ming <ming4.li@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/port.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 54dd2cd450ca..8e6596e147b3 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1544,6 +1544,7 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 			      struct device *dport_dev)
 {
 	struct device *dparent = grandparent(dport_dev);
+	struct cxl_port *port, *parent_port = NULL;
 	struct cxl_dport *dport, *parent_dport;
 	resource_size_t component_reg_phys;
 	int rc;
@@ -1559,18 +1560,12 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 		return -ENXIO;
 	}
 
-	struct cxl_port *parent_port __free(put_cxl_port) =
-		find_cxl_port(dparent, &parent_dport);
+	parent_port = find_cxl_port(dparent, &parent_dport);
 	if (!parent_port) {
 		/* iterate to create this parent_port */
 		return -EAGAIN;
 	}
 
-	/*
-	 * Definition with __free() here to keep the sequence of
-	 * dereferencing the device of the port before the parent_port releasing.
-	 */
-	struct cxl_port *port __free(put_cxl_port) = NULL;
 	device_lock(&parent_port->dev);
 	if (!parent_port->dev.driver) {
 		dev_warn(&cxlmd->dev,
@@ -1605,8 +1600,10 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 			 */
 			rc = -ENXIO;
 		}
+		put_device(&port->dev);
 	}
 
+	put_device(&parent_port->dev);
 	return rc;
 }
 

Re: [PATCH v2 2/3] cxl/port: Revert __free() conversion of add_port_attach_ep()

[Ira Weiny]

Dan Williams wrote:
> Dan points out via smatch [1] that the conversion of
> add_port_attach_ep() to use cleanup helpers does appears to not
> correctly handle a pointer reassignment case.
> 
>     drivers/cxl/core/port.c:1591 add_port_attach_ep()
>     warn: re-assigning __cleanup__ ptr 'port'
> 
> The reassignment still results in the correct cleanup and reference
> counting, but it is not immediately obvious from the code.
> 
> Revert the __free() conversion in preparation for refactoring the
> subtlety out of this function.
> 
> Cc: Dan Carpenter <dan.carpenter@linaro.org>
> Link: http://lore.kernel.org/2a19289b-0bcf-42c4-82a9-268a922535f2@stanley.mountain [1]
> Cc: Li Ming <ming4.li@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

[PATCH v2 3/3] cxl/port: Clarify add_port_attach_ep()

[Dan Williams]

The subtlety in add_port_attach_ep() is too high as even static analysis
checkers had trouble following the error exit logic [1]. Jonathan points
out that at a minimum the 2 acquisitions of @port should use 2 variables
[2]. This patch goes a step further and refactors the code to:

- drop the redundant uport_dev argument
- move all device_lock dependent code to a new find_add_dport() helper
- clarify why the @port reference needs to held
- clarify that @port is not devm released on failure
- create a new __free(put_cxl_dport) helper for this case of passing
  around an implied @port refernce

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: http://lore.kernel.org/2a19289b-0bcf-42c4-82a9-268a922535f2@stanley.mountain [1]
Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Link: http://lore.kernel.org/20241017181244.00003e1f@Huawei.com [2]
Cc: Li Ming <ming4.li@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/port.c |  112 ++++++++++++++++++++++++++++-------------------
 drivers/cxl/cxl.h       |    1 
 2 files changed, 68 insertions(+), 45 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 8e6596e147b3..599b1a4caa70 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1539,14 +1539,63 @@ static resource_size_t find_component_registers(struct device *dev)
 	return map.resource;
 }
 
+/*
+ * Check if @parent_port has a child cxl_port that hosts @dport_dev.
+ * and if not create a new port attached via the @parent_dport
+ * downstream of @parent_port
+ *
+ * Caller is responsible for dropping the port reference after using
+ * @dport
+ */
+static struct cxl_dport *find_add_dport(struct cxl_port *parent_port,
+					struct device *dport_dev,
+					struct cxl_dport *parent_dport)
+{
+	struct device *uport_dev = dport_dev->parent;
+	resource_size_t component_reg_phys;
+	struct cxl_dport *dport;
+	struct cxl_port *port;
+
+	/*
+	 * lock serves 2 purposes:
+	 * - Resolve races to find/create a new port
+	 * - Prevent found / created ports from being freed before a
+	 *   reference can be taken
+	 */
+	guard(device)(&parent_port->dev);
+	if (!parent_port->dev.driver) {
+		dev_warn(&parent_port->dev,
+			 "disabled, failed to enumerate CXL.mem\n");
+		return ERR_PTR(-ENXIO);
+	}
+
+	port = find_cxl_port_at(parent_port, dport_dev, &dport);
+	if (port)
+		return dport;
+
+	/*
+	 * Note that this port is only unregistered when @parent_port
+	 * is unbound / removed, not if this routine returns an error.
+	 * It is a shared object across multiple downstream endpoints.
+	 */
+	component_reg_phys = find_component_registers(uport_dev);
+	port = devm_cxl_add_port(&parent_port->dev, uport_dev,
+				 component_reg_phys, parent_dport);
+	if (IS_ERR(port))
+		return ERR_CAST(port);
+
+	dport = cxl_find_dport_by_dev(port, dport_dev);
+	if (!dport)
+		return ERR_PTR(-ENXIO);
+	get_device(&port->dev);
+	return dport;
+}
+
 static int add_port_attach_ep(struct cxl_memdev *cxlmd,
-			      struct device *uport_dev,
 			      struct device *dport_dev)
 {
 	struct device *dparent = grandparent(dport_dev);
-	struct cxl_port *port, *parent_port = NULL;
-	struct cxl_dport *dport, *parent_dport;
-	resource_size_t component_reg_phys;
+	struct cxl_dport *parent_dport;
 	int rc;
 
 	if (!dparent) {
@@ -1560,50 +1609,23 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 		return -ENXIO;
 	}
 
-	parent_port = find_cxl_port(dparent, &parent_dport);
-	if (!parent_port) {
-		/* iterate to create this parent_port */
-		return -EAGAIN;
-	}
+	struct cxl_port *parent_port __free(put_cxl_port) =
+		find_cxl_port(dparent, &parent_dport);
 
-	device_lock(&parent_port->dev);
-	if (!parent_port->dev.driver) {
-		dev_warn(&cxlmd->dev,
-			 "port %s:%s disabled, failed to enumerate CXL.mem\n",
-			 dev_name(&parent_port->dev), dev_name(uport_dev));
-		port = ERR_PTR(-ENXIO);
-		goto out;
-	}
+	/* iterate to create this parent_port? */
+	if (!parent_port)
+		return -EAGAIN;
 
-	port = find_cxl_port_at(parent_port, dport_dev, &dport);
-	if (!port) {
-		component_reg_phys = find_component_registers(uport_dev);
-		port = devm_cxl_add_port(&parent_port->dev, uport_dev,
-					 component_reg_phys, parent_dport);
-		/* retry find to pick up the new dport information */
-		if (!IS_ERR(port))
-			port = find_cxl_port_at(parent_port, dport_dev, &dport);
-	}
-out:
-	device_unlock(&parent_port->dev);
+	struct cxl_dport *dport __free(put_cxl_dport) =
+		find_add_dport(parent_port, dport_dev, parent_dport);
+	if (IS_ERR(dport))
+		return PTR_ERR(dport);
 
-	if (IS_ERR(port))
-		rc = PTR_ERR(port);
-	else {
-		dev_dbg(&cxlmd->dev, "add to new port %s:%s\n",
-			dev_name(&port->dev), dev_name(port->uport_dev));
-		rc = cxl_add_ep(dport, &cxlmd->dev);
-		if (rc == -EBUSY) {
-			/*
-			 * "can't" happen, but this error code means
-			 * something to the caller, so translate it.
-			 */
-			rc = -ENXIO;
-		}
-		put_device(&port->dev);
-	}
+	rc = cxl_add_ep(dport, &cxlmd->dev);
 
-	put_device(&parent_port->dev);
+	/* translate EBUSY as fatal */
+	if (rc == -EBUSY)
+		rc = -ENXIO;
 	return rc;
 }
 
@@ -1678,7 +1700,7 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
 			return 0;
 		}
 
-		rc = add_port_attach_ep(cxlmd, uport_dev, dport_dev);
+		rc = add_port_attach_ep(cxlmd, dport_dev);
 		/* port missing, try to add parent */
 		if (rc == -EAGAIN)
 			continue;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 5406e3ab3d4a..31dadc128b4e 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -746,6 +746,7 @@ void put_cxl_root(struct cxl_root *cxl_root);
 DEFINE_FREE(put_cxl_root, struct cxl_root *, if (_T) put_cxl_root(_T))
 
 DEFINE_FREE(put_cxl_port, struct cxl_port *, if (!IS_ERR_OR_NULL(_T)) put_device(&_T->dev))
+DEFINE_FREE(put_cxl_dport, struct cxl_dport *, if (!IS_ERR_OR_NULL(_T)) put_device(&_T->port->dev))
 int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
 void cxl_bus_rescan(void);
 void cxl_bus_drain(void);

Re: [PATCH v2 3/3] cxl/port: Clarify add_port_attach_ep()

[Jonathan Cameron]

On Thu, 17 Oct 2024 18:10:33 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> The subtlety in add_port_attach_ep() is too high as even static analysis
> checkers had trouble following the error exit logic [1]. Jonathan points
> out that at a minimum the 2 acquisitions of @port should use 2 variables
> [2]. This patch goes a step further and refactors the code to:
> 
> - drop the redundant uport_dev argument

Feels like an 'and' patch.  Maybe split off this first trivial bit as
a separate patch. Also a move of a comment could be done as a final
patch to simplify the diff of the important stuff.

> - move all device_lock dependent code to a new find_add_dport() helper
> - clarify why the @port reference needs to held
> - clarify that @port is not devm released on failure
> - create a new __free(put_cxl_dport) helper for this case of passing
>   around an implied @port refernce
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: http://lore.kernel.org/2a19289b-0bcf-42c4-82a9-268a922535f2@stanley.mountain [1]
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Link: http://lore.kernel.org/20241017181244.00003e1f@Huawei.com [2]
> Cc: Li Ming <ming4.li@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
A couple of trivial comments inline, but definitely looks much better
to me after this!

Jonathan

> ---
>  drivers/cxl/core/port.c |  112 ++++++++++++++++++++++++++++-------------------
>  drivers/cxl/cxl.h       |    1 
>  2 files changed, 68 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 8e6596e147b3..599b1a4caa70 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1539,14 +1539,63 @@ static resource_size_t find_component_registers(struct device *dev)
>  	return map.resource;
>  }
>  
> +/*
> + * Check if @parent_port has a child cxl_port that hosts @dport_dev.
> + * and if not create a new port attached via the @parent_dport
> + * downstream of @parent_port
> + *
> + * Caller is responsible for dropping the port reference after using
> + * @dport
> + */
> +static struct cxl_dport *find_add_dport(struct cxl_port *parent_port,
> +					struct device *dport_dev,
> +					struct cxl_dport *parent_dport)
> +{
> +	struct device *uport_dev = dport_dev->parent;
> +	resource_size_t component_reg_phys;
> +	struct cxl_dport *dport;
> +	struct cxl_port *port;
> +
> +	/*
> +	 * lock serves 2 purposes:
> +	 * - Resolve races to find/create a new port
> +	 * - Prevent found / created ports from being freed before a
> +	 *   reference can be taken
A breadcrumb on why this second one is true might be helpful
as no immediately obvious why holding parent would do this.
I'm guessing because reap_dports holds the same lock?

> +	 */
> +	guard(device)(&parent_port->dev);
> +	if (!parent_port->dev.driver) {
> +		dev_warn(&parent_port->dev,
> +			 "disabled, failed to enumerate CXL.mem\n");
> +		return ERR_PTR(-ENXIO);
> +	}
> +
> +	port = find_cxl_port_at(parent_port, dport_dev, &dport);
> +	if (port)
> +		return dport;
> +
> +	/*
> +	 * Note that this port is only unregistered when @parent_port
> +	 * is unbound / removed, not if this routine returns an error.
> +	 * It is a shared object across multiple downstream endpoints.
> +	 */
> +	component_reg_phys = find_component_registers(uport_dev);
> +	port = devm_cxl_add_port(&parent_port->dev, uport_dev,
> +				 component_reg_phys, parent_dport);
> +	if (IS_ERR(port))
> +		return ERR_CAST(port);
> +
> +	dport = cxl_find_dport_by_dev(port, dport_dev);
> +	if (!dport)
> +		return ERR_PTR(-ENXIO);
> +	get_device(&port->dev);
> +	return dport;
> +}
> +
>  static int add_port_attach_ep(struct cxl_memdev *cxlmd,
> -			      struct device *uport_dev,
>  			      struct device *dport_dev)
>  {
>  	struct device *dparent = grandparent(dport_dev);
> -	struct cxl_port *port, *parent_port = NULL;
> -	struct cxl_dport *dport, *parent_dport;
> -	resource_size_t component_reg_phys;
> +	struct cxl_dport *parent_dport;
>  	int rc;
>  
>  	if (!dparent) {
> @@ -1560,50 +1609,23 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>  		return -ENXIO;
>  	}
>  
> -	parent_port = find_cxl_port(dparent, &parent_dport);
> -	if (!parent_port) {
> -		/* iterate to create this parent_port */
> -		return -EAGAIN;
> -	}
> +	struct cxl_port *parent_port __free(put_cxl_port) =
> +		find_cxl_port(dparent, &parent_dport);
>  
> -	device_lock(&parent_port->dev);
> -	if (!parent_port->dev.driver) {
> -		dev_warn(&cxlmd->dev,
> -			 "port %s:%s disabled, failed to enumerate CXL.mem\n",
> -			 dev_name(&parent_port->dev), dev_name(uport_dev));
> -		port = ERR_PTR(-ENXIO);
> -		goto out;
> -	}
> +	/* iterate to create this parent_port? */
> +	if (!parent_port)
> +		return -EAGAIN;

Just to reduce diff and hence make this slightly simpler to review,
I'd leave the formatting as above.  If you care trivial follow up patch.


>  
> -	port = find_cxl_port_at(parent_port, dport_dev, &dport);
> -	if (!port) {
> -		component_reg_phys = find_component_registers(uport_dev);
> -		port = devm_cxl_add_port(&parent_port->dev, uport_dev,
> -					 component_reg_phys, parent_dport);
> -		/* retry find to pick up the new dport information */
> -		if (!IS_ERR(port))
> -			port = find_cxl_port_at(parent_port, dport_dev, &dport);
> -	}

Re: [PATCH v2 3/3] cxl/port: Clarify add_port_attach_ep()

[Dan Williams]

Jonathan Cameron wrote:
> On Thu, 17 Oct 2024 18:10:33 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > The subtlety in add_port_attach_ep() is too high as even static analysis
> > checkers had trouble following the error exit logic [1]. Jonathan points
> > out that at a minimum the 2 acquisitions of @port should use 2 variables
> > [2]. This patch goes a step further and refactors the code to:
> > 
> > - drop the redundant uport_dev argument
> 
> Feels like an 'and' patch.  Maybe split off this first trivial bit as
> a separate patch. Also a move of a comment could be done as a final
> patch to simplify the diff of the important stuff.

ok.

> > - move all device_lock dependent code to a new find_add_dport() helper
> > - clarify why the @port reference needs to held
> > - clarify that @port is not devm released on failure
> > - create a new __free(put_cxl_dport) helper for this case of passing
> >   around an implied @port refernce
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: http://lore.kernel.org/2a19289b-0bcf-42c4-82a9-268a922535f2@stanley.mountain [1]
> > Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > Link: http://lore.kernel.org/20241017181244.00003e1f@Huawei.com [2]
> > Cc: Li Ming <ming4.li@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> A couple of trivial comments inline, but definitely looks much better
> to me after this!
> 
> Jonathan
> 
> > ---
> >  drivers/cxl/core/port.c |  112 ++++++++++++++++++++++++++++-------------------
> >  drivers/cxl/cxl.h       |    1 
> >  2 files changed, 68 insertions(+), 45 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index 8e6596e147b3..599b1a4caa70 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -1539,14 +1539,63 @@ static resource_size_t find_component_registers(struct device *dev)
> >  	return map.resource;
> >  }
> >  
> > +/*
> > + * Check if @parent_port has a child cxl_port that hosts @dport_dev.
> > + * and if not create a new port attached via the @parent_dport
> > + * downstream of @parent_port
> > + *
> > + * Caller is responsible for dropping the port reference after using
> > + * @dport
> > + */
> > +static struct cxl_dport *find_add_dport(struct cxl_port *parent_port,
> > +					struct device *dport_dev,
> > +					struct cxl_dport *parent_dport)
> > +{
> > +	struct device *uport_dev = dport_dev->parent;
> > +	resource_size_t component_reg_phys;
> > +	struct cxl_dport *dport;
> > +	struct cxl_port *port;
> > +
> > +	/*
> > +	 * lock serves 2 purposes:
> > +	 * - Resolve races to find/create a new port
> > +	 * - Prevent found / created ports from being freed before a
> > +	 *   reference can be taken
> A breadcrumb on why this second one is true might be helpful
> as no immediately obvious why holding parent would do this.
> I'm guessing because reap_dports holds the same lock?

It does, but the more straightforward answer is that it holds off all
devres release actions. I'll update this comment to:

/*
 * parent_port lock serves 2 purposes:
 * - Resolve races to find/create a new child port
 * - Hold off device_unbind_cleanup() triggering unregister_port()
 *   (via devres_release_all()) for the child port before a reference
 *   can be taken
 */

> 
> > +	 */
> > +	guard(device)(&parent_port->dev);
> > +	if (!parent_port->dev.driver) {
> > +		dev_warn(&parent_port->dev,
> > +			 "disabled, failed to enumerate CXL.mem\n");
> > +		return ERR_PTR(-ENXIO);
> > +	}
> > +
> > +	port = find_cxl_port_at(parent_port, dport_dev, &dport);
> > +	if (port)
> > +		return dport;
> > +
> > +	/*
> > +	 * Note that this port is only unregistered when @parent_port
> > +	 * is unbound / removed, not if this routine returns an error.
> > +	 * It is a shared object across multiple downstream endpoints.
> > +	 */
> > +	component_reg_phys = find_component_registers(uport_dev);
> > +	port = devm_cxl_add_port(&parent_port->dev, uport_dev,
> > +				 component_reg_phys, parent_dport);
> > +	if (IS_ERR(port))
> > +		return ERR_CAST(port);
> > +
> > +	dport = cxl_find_dport_by_dev(port, dport_dev);
> > +	if (!dport)
> > +		return ERR_PTR(-ENXIO);
> > +	get_device(&port->dev);
> > +	return dport;
> > +}
> > +
> >  static int add_port_attach_ep(struct cxl_memdev *cxlmd,
> > -			      struct device *uport_dev,
> >  			      struct device *dport_dev)
> >  {
> >  	struct device *dparent = grandparent(dport_dev);
> > -	struct cxl_port *port, *parent_port = NULL;
> > -	struct cxl_dport *dport, *parent_dport;
> > -	resource_size_t component_reg_phys;
> > +	struct cxl_dport *parent_dport;
> >  	int rc;
> >  
> >  	if (!dparent) {
> > @@ -1560,50 +1609,23 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
> >  		return -ENXIO;
> >  	}
> >  
> > -	parent_port = find_cxl_port(dparent, &parent_dport);
> > -	if (!parent_port) {
> > -		/* iterate to create this parent_port */
> > -		return -EAGAIN;
> > -	}
> > +	struct cxl_port *parent_port __free(put_cxl_port) =
> > +		find_cxl_port(dparent, &parent_dport);
> >  
> > -	device_lock(&parent_port->dev);
> > -	if (!parent_port->dev.driver) {
> > -		dev_warn(&cxlmd->dev,
> > -			 "port %s:%s disabled, failed to enumerate CXL.mem\n",
> > -			 dev_name(&parent_port->dev), dev_name(uport_dev));
> > -		port = ERR_PTR(-ENXIO);
> > -		goto out;
> > -	}
> > +	/* iterate to create this parent_port? */
> > +	if (!parent_port)
> > +		return -EAGAIN;
> 
> Just to reduce diff and hence make this slightly simpler to review,
> I'd leave the formatting as above.  If you care trivial follow up patch.

ok.