[PATCH v6 0/5] cxl: find_cxl_root() related cleanups

[PATCH v6 0/5] cxl: find_cxl_root() related cleanups

[Dave Jiang]

v6:
- Fix breakage in previous series for device_find_child() (Dan)
- Move DEFINE_FREE() for put_cxl_root() to first patch (Dan)
- Don't touch put_device() until final cleanup. (Dan)
- Add review tag from Ira.

This series provides a number of small cleanups to make fix_cxl_root()
and related code more readable and safer.

---

Dave Jiang (5):
      cxl: Introduce put_cxl_root() helper
      cxl: Convert find_cxl_root() to return a 'struct cxl_root *'
      cxl: Fix device reference leak in cxl_port_perf_data_calculate()
      cxl: Refactor to use __free() for cxl_root allocation in cxl_find_nvdimm_bridge()
      cxl: Refactor to use __free() for cxl_root allocation in cxl_endpoint_port_probe()


 drivers/cxl/acpi.c      |  6 ++----
 drivers/cxl/core/cdat.c | 22 ++++++++++++++--------
 drivers/cxl/core/pmem.c |  8 ++++----
 drivers/cxl/core/port.c | 13 +++++++++++--
 drivers/cxl/cxl.h       | 17 ++++++++++-------
 drivers/cxl/port.c      |  5 +++--
 6 files changed, 44 insertions(+), 27 deletions(-)

[PATCH v6 1/5] cxl: Introduce put_cxl_root() helper

[Dave Jiang]

Add a helper function put_cxl_root() to maintain symmetry for
find_cxl_root() function instead of relying on open coding of the
put_device() in order to dereference the 'struct device' that happens via
get_device() in find_cxl_root().

Suggested-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v6:
- Move DEFINE_FREE() of put_cxl_root to current patch (Dan)
v5:
- Move out usages for follow on cleanup. (Dan)
v4:
- Adjust ordering of this patch to front. (Dan)
v3:
- Adjust for cxl_root as parameter for find_cxl_root()
- Add NULL ptr check fore __free(). (Dan)
- Fix DEFINE_FREE() macro to name it put_cxl_root (Dan)
- Cleanup all functions calling put_cxl_root() and related calls. (Dan)
v2:
- Make put_cxl_root() an exported function to be symmetric to
  find_cxl_root(). (Robert)
---
 drivers/cxl/core/port.c |    9 +++++++++
 drivers/cxl/cxl.h       |    3 +++
 2 files changed, 12 insertions(+)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 8c00fd6be730..64f30d5fe1f6 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -986,6 +986,15 @@ struct cxl_port *find_cxl_root(struct cxl_port *port)
 }
 EXPORT_SYMBOL_NS_GPL(find_cxl_root, CXL);
 
+void put_cxl_root(struct cxl_root *cxl_root)
+{
+	if (!cxl_root)
+		return;
+
+	put_device(&cxl_root->port.dev);
+}
+EXPORT_SYMBOL_NS_GPL(put_cxl_root, CXL);
+
 static struct cxl_dport *find_dport(struct cxl_port *port, int id)
 {
 	struct cxl_dport *dport;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 492dbf63935f..df3db3e43913 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -735,6 +735,9 @@ struct cxl_port *devm_cxl_add_port(struct device *host,
 struct cxl_root *devm_cxl_add_root(struct device *host,
 				   const struct cxl_root_ops *ops);
 struct cxl_port *find_cxl_root(struct cxl_port *port);
+void put_cxl_root(struct cxl_root *cxl_root);
+DEFINE_FREE(put_cxl_root, struct cxl_root *, if (_T) put_cxl_root(_T))
+
 int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
 void cxl_bus_rescan(void);
 void cxl_bus_drain(void);

[PATCH v6 2/5] cxl: Convert find_cxl_root() to return a 'struct cxl_root *'

[Dave Jiang]

Commit 790815902ec6 ("cxl: Add support for _DSM Function for retrieving QTG ID")
introduced 'struct cxl_root', however all usages have been worked
indirectly through cxl_port. Refactor code such as find_cxl_root()
function to use 'struct cxl_root' directly.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v6:
- Revert code that broke device_for_each_child() (Dan)
- Leave put_device() calls in this patch. (Dan)
v5:
- Squash in v4 3/6 to update the qos_class to cxl_root. (Dan)
- Moved the introduction of __free() for cxl_root_put from v4 1/6 to
  here. (Dan)
v4:
- Adjust ordering of patches to move this to 2nd place. (Dan)
---
 drivers/cxl/acpi.c      |    6 ++----
 drivers/cxl/core/cdat.c |   17 ++++++++++-------
 drivers/cxl/core/pmem.c |    6 ++++--
 drivers/cxl/core/port.c |    4 ++--
 drivers/cxl/cxl.h       |   14 +++++++-------
 drivers/cxl/port.c      |    4 +++-
 6 files changed, 28 insertions(+), 23 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index afc712264d1c..dcf2b39e1048 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -295,14 +295,12 @@ cxl_acpi_evaluate_qtg_dsm(acpi_handle handle, struct access_coordinate *coord,
 	return rc;
 }
 
-static int cxl_acpi_qos_class(struct cxl_port *root_port,
+static int cxl_acpi_qos_class(struct cxl_root *cxl_root,
 			      struct access_coordinate *coord, int entries,
 			      int *qos_class)
 {
+	struct device *dev = cxl_root->port.uport_dev;
 	acpi_handle handle;
-	struct device *dev;
-
-	dev = root_port->uport_dev;
 
 	if (!dev_is_platform(dev))
 		return -ENODEV;
diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index cd84d87f597a..0df5379cf02f 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -162,7 +162,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
 					struct xarray *dsmas_xa)
 {
 	struct access_coordinate c;
-	struct cxl_port *root_port;
 	struct cxl_root *cxl_root;
 	struct dsmas_entry *dent;
 	int valid_entries = 0;
@@ -175,8 +174,7 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
 		return rc;
 	}
 
-	root_port = find_cxl_root(port);
-	cxl_root = to_cxl_root(root_port);
+	cxl_root = find_cxl_root(port);
 	if (!cxl_root->ops || !cxl_root->ops->qos_class)
 		return -EOPNOTSUPP;
 
@@ -193,7 +191,8 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
 						    dent->coord.write_bandwidth);
 
 		dent->entries = 1;
-		rc = cxl_root->ops->qos_class(root_port, &dent->coord, 1, &qos_class);
+		rc = cxl_root->ops->qos_class(cxl_root, &dent->coord, 1,
+					      &qos_class);
 		if (rc != 1)
 			continue;
 
@@ -349,15 +348,19 @@ static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
 {
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
-	struct cxl_port *root_port __free(put_device) = NULL;
 	LIST_HEAD(__discard);
 	struct list_head *discard __free(dpa_perf) = &__discard;
+	struct cxl_port *root_port;
 	int rc;
 
-	root_port = find_cxl_root(cxlmd->endpoint);
-	if (!root_port)
+	struct cxl_root *cxl_root __free(put_cxl_root) =
+		find_cxl_root(cxlmd->endpoint);
+
+	if (!cxl_root)
 		return -ENODEV;
 
+	root_port = &cxl_root->port;
+
 	/* Check that the QTG IDs are all sane between end device and root decoders */
 	cxl_qos_match(root_port, &mds->ram_perf_list, discard);
 	cxl_qos_match(root_port, &mds->pmem_perf_list, discard);
diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
index fc94f5240327..da92a901b9e8 100644
--- a/drivers/cxl/core/pmem.c
+++ b/drivers/cxl/core/pmem.c
@@ -64,12 +64,14 @@ static int match_nvdimm_bridge(struct device *dev, void *data)
 
 struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_memdev *cxlmd)
 {
-	struct cxl_port *port = find_cxl_root(cxlmd->endpoint);
+	struct cxl_root *cxl_root = find_cxl_root(cxlmd->endpoint);
+	struct cxl_port *port;
 	struct device *dev;
 
-	if (!port)
+	if (!cxl_root)
 		return NULL;
 
+	port = &cxl_root->port;
 	dev = device_find_child(&port->dev, NULL, match_nvdimm_bridge);
 	put_device(&port->dev);
 
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 64f30d5fe1f6..63a4e3c2baed 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -972,7 +972,7 @@ static bool dev_is_cxl_root_child(struct device *dev)
 	return false;
 }
 
-struct cxl_port *find_cxl_root(struct cxl_port *port)
+struct cxl_root *find_cxl_root(struct cxl_port *port)
 {
 	struct cxl_port *iter = port;
 
@@ -982,7 +982,7 @@ struct cxl_port *find_cxl_root(struct cxl_port *port)
 	if (!iter)
 		return NULL;
 	get_device(&iter->dev);
-	return iter;
+	return to_cxl_root(iter);
 }
 EXPORT_SYMBOL_NS_GPL(find_cxl_root, CXL);
 
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index df3db3e43913..3a5004aab97a 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -617,12 +617,6 @@ struct cxl_port {
 	long pci_latency;
 };
 
-struct cxl_root_ops {
-	int (*qos_class)(struct cxl_port *root_port,
-			 struct access_coordinate *coord, int entries,
-			 int *qos_class);
-};
-
 /**
  * struct cxl_root - logical collection of root cxl_port items
  *
@@ -640,6 +634,12 @@ to_cxl_root(const struct cxl_port *port)
 	return container_of(port, struct cxl_root, port);
 }
 
+struct cxl_root_ops {
+	int (*qos_class)(struct cxl_root *cxl_root,
+			 struct access_coordinate *coord, int entries,
+			 int *qos_class);
+};
+
 static inline struct cxl_dport *
 cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
 {
@@ -734,7 +734,7 @@ struct cxl_port *devm_cxl_add_port(struct device *host,
 				   struct cxl_dport *parent_dport);
 struct cxl_root *devm_cxl_add_root(struct device *host,
 				   const struct cxl_root_ops *ops);
-struct cxl_port *find_cxl_root(struct cxl_port *port);
+struct cxl_root *find_cxl_root(struct cxl_port *port);
 void put_cxl_root(struct cxl_root *cxl_root);
 DEFINE_FREE(put_cxl_root, struct cxl_root *, if (_T) put_cxl_root(_T))
 
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index da3c3a08bd62..4f3a08fdc9e9 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -94,6 +94,7 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
 	struct cxl_endpoint_dvsec_info info = { .port = port };
 	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	struct cxl_root *cxl_root;
 	struct cxl_hdm *cxlhdm;
 	struct cxl_port *root;
 	int rc;
@@ -130,7 +131,8 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
 	 * This can't fail in practice as CXL root exit unregisters all
 	 * descendant ports and that in turn synchronizes with cxl_port_probe()
 	 */
-	root = find_cxl_root(port);
+	cxl_root = find_cxl_root(port);
+	root = &cxl_root->port;
 
 	/*
 	 * Now that all endpoint decoders are successfully enumerated, try to

[PATCH v6 3/5] cxl: Fix device reference leak in cxl_port_perf_data_calculate()

[Dave Jiang]

cxl_port_perf_data_calculate() calls find_cxl_root() and does not
dereference the 'struct device' in the cxl_root->port. find_cxl_root()
calls get_device() and takes a reference on the port 'struct device'
member. Use the __free() macro to ensure the dereference happens.

Fixes: 7a4f148dd8d5 ("cxl: Compute the entire CXL path latency and bandwidth data")
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v5:
- Update patch title (Dan)
---
 drivers/cxl/core/cdat.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index 0df5379cf02f..c6208aab452f 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -162,7 +162,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
 					struct xarray *dsmas_xa)
 {
 	struct access_coordinate c;
-	struct cxl_root *cxl_root;
 	struct dsmas_entry *dent;
 	int valid_entries = 0;
 	unsigned long index;
@@ -174,7 +173,11 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
 		return rc;
 	}
 
-	cxl_root = find_cxl_root(port);
+	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
+
+	if (!cxl_root)
+		return -ENODEV;
+
 	if (!cxl_root->ops || !cxl_root->ops->qos_class)
 		return -EOPNOTSUPP;
 

[PATCH v6 4/5] cxl: Refactor to use __free() for cxl_root allocation in cxl_find_nvdimm_bridge()

[Dave Jiang]

Use scope-based resource management __free() macro to drop the open coded
put_device() in cxl_find_nvdimm_bridge().

Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v6:
- Cleanup put_device()
v5
- Update commit log (Dan)
---
 drivers/cxl/core/pmem.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
index da92a901b9e8..e69625a8d6a1 100644
--- a/drivers/cxl/core/pmem.c
+++ b/drivers/cxl/core/pmem.c
@@ -64,16 +64,14 @@ static int match_nvdimm_bridge(struct device *dev, void *data)
 
 struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_memdev *cxlmd)
 {
-	struct cxl_root *cxl_root = find_cxl_root(cxlmd->endpoint);
-	struct cxl_port *port;
+	struct cxl_root *cxl_root __free(put_cxl_root) =
+		find_cxl_root(cxlmd->endpoint);
 	struct device *dev;
 
 	if (!cxl_root)
 		return NULL;
 
-	port = &cxl_root->port;
-	dev = device_find_child(&port->dev, NULL, match_nvdimm_bridge);
-	put_device(&port->dev);
+	dev = device_find_child(&cxl_root->port.dev, NULL, match_nvdimm_bridge);
 
 	if (!dev)
 		return NULL;

[PATCH v6 5/5] cxl: Refactor to use __free() for cxl_root allocation in cxl_endpoint_port_probe()

[Dave Jiang]

Use scope-based resource management __free() macro to drop the open coded
put_device() in cxl_endpoint_port_probe().

Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v6:
- Cleanup put_device()
v5:
- Update commit log (Dan)
v4:
- Don't check return value of find_cxl_root() per comment. (Dan)
---
 drivers/cxl/port.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index 4f3a08fdc9e9..97c21566677a 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -94,7 +94,6 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
 	struct cxl_endpoint_dvsec_info info = { .port = port };
 	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
-	struct cxl_root *cxl_root;
 	struct cxl_hdm *cxlhdm;
 	struct cxl_port *root;
 	int rc;
@@ -131,7 +130,8 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
 	 * This can't fail in practice as CXL root exit unregisters all
 	 * descendant ports and that in turn synchronizes with cxl_port_probe()
 	 */
-	cxl_root = find_cxl_root(port);
+	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
+
 	root = &cxl_root->port;
 
 	/*
@@ -139,7 +139,6 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
 	 * assemble regions from committed decoders
 	 */
 	device_for_each_child(&port->dev, root, discover_region);
-	put_device(&root->dev);
 
 	return 0;
 }

Re: [PATCH v6 1/5] cxl: Introduce put_cxl_root() helper

[Robert Richter]

On 05.01.24 15:07:34, Dave Jiang wrote:
> Add a helper function put_cxl_root() to maintain symmetry for
> find_cxl_root() function instead of relying on open coding of the
> put_device() in order to dereference the 'struct device' that happens via
> get_device() in find_cxl_root().
> 
> Suggested-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Reviewed-by: Robert Richter <rrichter@amd.com>

RE: [PATCH v6 0/5] cxl: find_cxl_root() related cleanups

[Dan Williams]

Dave Jiang wrote:
> v6:
> - Fix breakage in previous series for device_find_child() (Dan)
> - Move DEFINE_FREE() for put_cxl_root() to first patch (Dan)
> - Don't touch put_device() until final cleanup. (Dan)
> - Add review tag from Ira.
> 
> This series provides a number of small cleanups to make fix_cxl_root()
> and related code more readable and safer.

Looks good, Dave! Thanks for the spins.

Re: [PATCH v6 2/5] cxl: Convert find_cxl_root() to return a 'struct cxl_root *'

[Robert Richter]

See comments below.

On 05.01.24 15:07:40, Dave Jiang wrote:
> Commit 790815902ec6 ("cxl: Add support for _DSM Function for retrieving QTG ID")
> introduced 'struct cxl_root', however all usages have been worked
> indirectly through cxl_port. Refactor code such as find_cxl_root()
> function to use 'struct cxl_root' directly.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v6:
> - Revert code that broke device_for_each_child() (Dan)
> - Leave put_device() calls in this patch. (Dan)
> v5:
> - Squash in v4 3/6 to update the qos_class to cxl_root. (Dan)
> - Moved the introduction of __free() for cxl_root_put from v4 1/6 to
>   here. (Dan)
> v4:
> - Adjust ordering of patches to move this to 2nd place. (Dan)
> ---
>  drivers/cxl/acpi.c      |    6 ++----
>  drivers/cxl/core/cdat.c |   17 ++++++++++-------
>  drivers/cxl/core/pmem.c |    6 ++++--
>  drivers/cxl/core/port.c |    4 ++--
>  drivers/cxl/cxl.h       |   14 +++++++-------
>  drivers/cxl/port.c      |    4 +++-
>  6 files changed, 28 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index afc712264d1c..dcf2b39e1048 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -295,14 +295,12 @@ cxl_acpi_evaluate_qtg_dsm(acpi_handle handle, struct access_coordinate *coord,
>  	return rc;
>  }
>  
> -static int cxl_acpi_qos_class(struct cxl_port *root_port,
> +static int cxl_acpi_qos_class(struct cxl_root *cxl_root,
>  			      struct access_coordinate *coord, int entries,
>  			      int *qos_class)
>  {
> +	struct device *dev = cxl_root->port.uport_dev;
>  	acpi_handle handle;
> -	struct device *dev;
> -
> -	dev = root_port->uport_dev;
>  
>  	if (!dev_is_platform(dev))
>  		return -ENODEV;
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index cd84d87f597a..0df5379cf02f 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -162,7 +162,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>  					struct xarray *dsmas_xa)
>  {
>  	struct access_coordinate c;
> -	struct cxl_port *root_port;
>  	struct cxl_root *cxl_root;
>  	struct dsmas_entry *dent;
>  	int valid_entries = 0;
> @@ -175,8 +174,7 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>  		return rc;
>  	}
>  
> -	root_port = find_cxl_root(port);
> -	cxl_root = to_cxl_root(root_port);
> +	cxl_root = find_cxl_root(port);
>  	if (!cxl_root->ops || !cxl_root->ops->qos_class)
>  		return -EOPNOTSUPP;
>  
> @@ -193,7 +191,8 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>  						    dent->coord.write_bandwidth);
>  
>  		dent->entries = 1;
> -		rc = cxl_root->ops->qos_class(root_port, &dent->coord, 1, &qos_class);
> +		rc = cxl_root->ops->qos_class(cxl_root, &dent->coord, 1,
> +					      &qos_class);
>  		if (rc != 1)
>  			continue;
>  
> @@ -349,15 +348,19 @@ static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
>  {
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> -	struct cxl_port *root_port __free(put_device) = NULL;
>  	LIST_HEAD(__discard);
>  	struct list_head *discard __free(dpa_perf) = &__discard;
> +	struct cxl_port *root_port;
>  	int rc;
>  
> -	root_port = find_cxl_root(cxlmd->endpoint);
> -	if (!root_port)
> +	struct cxl_root *cxl_root __free(put_cxl_root) =
> +		find_cxl_root(cxlmd->endpoint);

That's the drawback that lines get very long. Not sure if that was
discussed before, maybe just assign NULL in the definition and then
have the first assignment right after?

Should be moved to the definitions above without a newline.

> +
> +	if (!cxl_root)
>  		return -ENODEV;
>  
> +	root_port = &cxl_root->port;
> +
>  	/* Check that the QTG IDs are all sane between end device and root decoders */
>  	cxl_qos_match(root_port, &mds->ram_perf_list, discard);
>  	cxl_qos_match(root_port, &mds->pmem_perf_list, discard);

It looks like cxl_qos_match() consumes a root port, so we could pass
cxl_root here directly and get rid of root_port.

> diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
> index fc94f5240327..da92a901b9e8 100644
> --- a/drivers/cxl/core/pmem.c
> +++ b/drivers/cxl/core/pmem.c
> @@ -64,12 +64,14 @@ static int match_nvdimm_bridge(struct device *dev, void *data)
>  
>  struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_memdev *cxlmd)
>  {
> -	struct cxl_port *port = find_cxl_root(cxlmd->endpoint);
> +	struct cxl_root *cxl_root = find_cxl_root(cxlmd->endpoint);
> +	struct cxl_port *port;
>  	struct device *dev;
>  
> -	if (!port)
> +	if (!cxl_root)
>  		return NULL;
>  
> +	port = &cxl_root->port;
>  	dev = device_find_child(&port->dev, NULL, match_nvdimm_bridge);

No need for port variable here.

>  	put_device(&port->dev);
>  
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 64f30d5fe1f6..63a4e3c2baed 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -972,7 +972,7 @@ static bool dev_is_cxl_root_child(struct device *dev)
>  	return false;
>  }
>  
> -struct cxl_port *find_cxl_root(struct cxl_port *port)
> +struct cxl_root *find_cxl_root(struct cxl_port *port)
>  {
>  	struct cxl_port *iter = port;
>  
> @@ -982,7 +982,7 @@ struct cxl_port *find_cxl_root(struct cxl_port *port)
>  	if (!iter)
>  		return NULL;
>  	get_device(&iter->dev);
> -	return iter;
> +	return to_cxl_root(iter);

I guess this is the last user of to_cxl_root() now so we can remove
the macro entirely. Unlikely we will ever need it again and a
container_of() call is also fairly easy.

>  }
>  EXPORT_SYMBOL_NS_GPL(find_cxl_root, CXL);
>  
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index df3db3e43913..3a5004aab97a 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -617,12 +617,6 @@ struct cxl_port {
>  	long pci_latency;
>  };
>  
> -struct cxl_root_ops {
> -	int (*qos_class)(struct cxl_port *root_port,
> -			 struct access_coordinate *coord, int entries,
> -			 int *qos_class);
> -};
> -
>  /**
>   * struct cxl_root - logical collection of root cxl_port items
>   *
> @@ -640,6 +634,12 @@ to_cxl_root(const struct cxl_port *port)
>  	return container_of(port, struct cxl_root, port);
>  }
>  
> +struct cxl_root_ops {
> +	int (*qos_class)(struct cxl_root *cxl_root,
> +			 struct access_coordinate *coord, int entries,
> +			 int *qos_class);
> +};
> +

Any intention to move code here in addition?

>  static inline struct cxl_dport *
>  cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
>  {
> @@ -734,7 +734,7 @@ struct cxl_port *devm_cxl_add_port(struct device *host,
>  				   struct cxl_dport *parent_dport);
>  struct cxl_root *devm_cxl_add_root(struct device *host,
>  				   const struct cxl_root_ops *ops);
> -struct cxl_port *find_cxl_root(struct cxl_port *port);
> +struct cxl_root *find_cxl_root(struct cxl_port *port);
>  void put_cxl_root(struct cxl_root *cxl_root);
>  DEFINE_FREE(put_cxl_root, struct cxl_root *, if (_T) put_cxl_root(_T))
>  
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index da3c3a08bd62..4f3a08fdc9e9 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -94,6 +94,7 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>  	struct cxl_endpoint_dvsec_info info = { .port = port };
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_root *cxl_root;
>  	struct cxl_hdm *cxlhdm;
>  	struct cxl_port *root;
>  	int rc;
> @@ -130,7 +131,8 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>  	 * This can't fail in practice as CXL root exit unregisters all
>  	 * descendant ports and that in turn synchronizes with cxl_port_probe()
>  	 */
> -	root = find_cxl_root(port);
> +	cxl_root = find_cxl_root(port);
> +	root = &cxl_root->port;

Same here, no need for the root var.

-Robert


>  
>  	/*
>  	 * Now that all endpoint decoders are successfully enumerated, try to
> 
> 

Re: [PATCH v6 3/5] cxl: Fix device reference leak in cxl_port_perf_data_calculate()

[Robert Richter]

See inline.

On 05.01.24 15:07:46, Dave Jiang wrote:
> cxl_port_perf_data_calculate() calls find_cxl_root() and does not
> dereference the 'struct device' in the cxl_root->port. find_cxl_root()
> calls get_device() and takes a reference on the port 'struct device'
> member. Use the __free() macro to ensure the dereference happens.
> 
> Fixes: 7a4f148dd8d5 ("cxl: Compute the entire CXL path latency and bandwidth data")
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v5:
> - Update patch title (Dan)
> ---
>  drivers/cxl/core/cdat.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index 0df5379cf02f..c6208aab452f 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -162,7 +162,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>  					struct xarray *dsmas_xa)
>  {
>  	struct access_coordinate c;
> -	struct cxl_root *cxl_root;
>  	struct dsmas_entry *dent;
>  	int valid_entries = 0;
>  	unsigned long index;
> @@ -174,7 +173,11 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>  		return rc;
>  	}
>  
> -	cxl_root = find_cxl_root(port);
> +	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);

I am not quite sure here, can a definition in the middle of a block?
At least this is uncommon.

> +
> +	if (!cxl_root)
> +		return -ENODEV;

As Dan pointed out earlier, isn't there always a root port in the
hierarchy? So the check could be dropped?

> +
>  	if (!cxl_root->ops || !cxl_root->ops->qos_class)
>  		return -EOPNOTSUPP;
>  
> 
> 

Re: [PATCH v6 4/5] cxl: Refactor to use __free() for cxl_root allocation in cxl_find_nvdimm_bridge()

[Robert Richter]

On 05.01.24 15:07:53, Dave Jiang wrote:
> Use scope-based resource management __free() macro to drop the open coded
> put_device() in cxl_find_nvdimm_bridge().
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v6:
> - Cleanup put_device()
> v5
> - Update commit log (Dan)
> ---
>  drivers/cxl/core/pmem.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
> index da92a901b9e8..e69625a8d6a1 100644
> --- a/drivers/cxl/core/pmem.c
> +++ b/drivers/cxl/core/pmem.c
> @@ -64,16 +64,14 @@ static int match_nvdimm_bridge(struct device *dev, void *data)
>  
>  struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_memdev *cxlmd)
>  {
> -	struct cxl_root *cxl_root = find_cxl_root(cxlmd->endpoint);
> -	struct cxl_port *port;
> +	struct cxl_root *cxl_root __free(put_cxl_root) =
> +		find_cxl_root(cxlmd->endpoint);
>  	struct device *dev;
>  
>  	if (!cxl_root)
>  		return NULL;

Can the check be removed here too?

-Robert

>  
> -	port = &cxl_root->port;
> -	dev = device_find_child(&port->dev, NULL, match_nvdimm_bridge);
> -	put_device(&port->dev);
> +	dev = device_find_child(&cxl_root->port.dev, NULL, match_nvdimm_bridge);
>  
>  	if (!dev)
>  		return NULL;
> 
> 

Re: [PATCH v6 2/5] cxl: Convert find_cxl_root() to return a 'struct cxl_root *'

[Dave Jiang]

On 1/5/24 15:51, Robert Richter wrote:
> See comments below.
> 
> On 05.01.24 15:07:40, Dave Jiang wrote:
>> Commit 790815902ec6 ("cxl: Add support for _DSM Function for retrieving QTG ID")
>> introduced 'struct cxl_root', however all usages have been worked
>> indirectly through cxl_port. Refactor code such as find_cxl_root()
>> function to use 'struct cxl_root' directly.
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> v6:
>> - Revert code that broke device_for_each_child() (Dan)
>> - Leave put_device() calls in this patch. (Dan)
>> v5:
>> - Squash in v4 3/6 to update the qos_class to cxl_root. (Dan)
>> - Moved the introduction of __free() for cxl_root_put from v4 1/6 to
>>   here. (Dan)
>> v4:
>> - Adjust ordering of patches to move this to 2nd place. (Dan)
>> ---
>>  drivers/cxl/acpi.c      |    6 ++----
>>  drivers/cxl/core/cdat.c |   17 ++++++++++-------
>>  drivers/cxl/core/pmem.c |    6 ++++--
>>  drivers/cxl/core/port.c |    4 ++--
>>  drivers/cxl/cxl.h       |   14 +++++++-------
>>  drivers/cxl/port.c      |    4 +++-
>>  6 files changed, 28 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
>> index afc712264d1c..dcf2b39e1048 100644
>> --- a/drivers/cxl/acpi.c
>> +++ b/drivers/cxl/acpi.c
>> @@ -295,14 +295,12 @@ cxl_acpi_evaluate_qtg_dsm(acpi_handle handle, struct access_coordinate *coord,
>>  	return rc;
>>  }
>>  
>> -static int cxl_acpi_qos_class(struct cxl_port *root_port,
>> +static int cxl_acpi_qos_class(struct cxl_root *cxl_root,
>>  			      struct access_coordinate *coord, int entries,
>>  			      int *qos_class)
>>  {
>> +	struct device *dev = cxl_root->port.uport_dev;
>>  	acpi_handle handle;
>> -	struct device *dev;
>> -
>> -	dev = root_port->uport_dev;
>>  
>>  	if (!dev_is_platform(dev))
>>  		return -ENODEV;
>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>> index cd84d87f597a..0df5379cf02f 100644
>> --- a/drivers/cxl/core/cdat.c
>> +++ b/drivers/cxl/core/cdat.c
>> @@ -162,7 +162,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>>  					struct xarray *dsmas_xa)
>>  {
>>  	struct access_coordinate c;
>> -	struct cxl_port *root_port;
>>  	struct cxl_root *cxl_root;
>>  	struct dsmas_entry *dent;
>>  	int valid_entries = 0;
>> @@ -175,8 +174,7 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>>  		return rc;
>>  	}
>>  
>> -	root_port = find_cxl_root(port);
>> -	cxl_root = to_cxl_root(root_port);
>> +	cxl_root = find_cxl_root(port);
>>  	if (!cxl_root->ops || !cxl_root->ops->qos_class)
>>  		return -EOPNOTSUPP;
>>  
>> @@ -193,7 +191,8 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>>  						    dent->coord.write_bandwidth);
>>  
>>  		dent->entries = 1;
>> -		rc = cxl_root->ops->qos_class(root_port, &dent->coord, 1, &qos_class);
>> +		rc = cxl_root->ops->qos_class(cxl_root, &dent->coord, 1,
>> +					      &qos_class);
>>  		if (rc != 1)
>>  			continue;
>>  
>> @@ -349,15 +348,19 @@ static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
>>  {
>>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
>> -	struct cxl_port *root_port __free(put_device) = NULL;
>>  	LIST_HEAD(__discard);
>>  	struct list_head *discard __free(dpa_perf) = &__discard;
>> +	struct cxl_port *root_port;
>>  	int rc;
>>  
>> -	root_port = find_cxl_root(cxlmd->endpoint);
>> -	if (!root_port)
>> +	struct cxl_root *cxl_root __free(put_cxl_root) =
>> +		find_cxl_root(cxlmd->endpoint);
> 
> That's the drawback that lines get very long. Not sure if that was
> discussed before, maybe just assign NULL in the definition and then
> have the first assignment right after?
> 
> Should be moved to the definitions above without a newline.

Here Dan says to avoid the NULL assign pattern:
https://lore.kernel.org/linux-cxl/65970c97b2b6c_8dc68294ca@dwillia2-xfh.jf.intel.com.notmuch/T/#m6462902836f98c35f0b0faa70467314998b22185

> 
>> +
>> +	if (!cxl_root)
>>  		return -ENODEV;
>>  
>> +	root_port = &cxl_root->port;
>> +
>>  	/* Check that the QTG IDs are all sane between end device and root decoders */
>>  	cxl_qos_match(root_port, &mds->ram_perf_list, discard);
>>  	cxl_qos_match(root_port, &mds->pmem_perf_list, discard);
> 
> It looks like cxl_qos_match() consumes a root port, so we could pass
> cxl_root here directly and get rid of root_port.

ok I can fix that.

> 
>> diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
>> index fc94f5240327..da92a901b9e8 100644
>> --- a/drivers/cxl/core/pmem.c
>> +++ b/drivers/cxl/core/pmem.c
>> @@ -64,12 +64,14 @@ static int match_nvdimm_bridge(struct device *dev, void *data)
>>  
>>  struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_memdev *cxlmd)
>>  {
>> -	struct cxl_port *port = find_cxl_root(cxlmd->endpoint);
>> +	struct cxl_root *cxl_root = find_cxl_root(cxlmd->endpoint);
>> +	struct cxl_port *port;
>>  	struct device *dev;
>>  
>> -	if (!port)
>> +	if (!cxl_root)
>>  		return NULL;
>>  
>> +	port = &cxl_root->port;
>>  	dev = device_find_child(&port->dev, NULL, match_nvdimm_bridge);
> 
> No need for port variable here.

Ok

> 
>>  	put_device(&port->dev);
>>  
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 64f30d5fe1f6..63a4e3c2baed 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -972,7 +972,7 @@ static bool dev_is_cxl_root_child(struct device *dev)
>>  	return false;
>>  }
>>  
>> -struct cxl_port *find_cxl_root(struct cxl_port *port)
>> +struct cxl_root *find_cxl_root(struct cxl_port *port)
>>  {
>>  	struct cxl_port *iter = port;
>>  
>> @@ -982,7 +982,7 @@ struct cxl_port *find_cxl_root(struct cxl_port *port)
>>  	if (!iter)
>>  		return NULL;
>>  	get_device(&iter->dev);
>> -	return iter;
>> +	return to_cxl_root(iter);
> 
> I guess this is the last user of to_cxl_root() now so we can remove
> the macro entirely. Unlikely we will ever need it again and a
> container_of() call is also fairly easy.

ok

> 
>>  }
>>  EXPORT_SYMBOL_NS_GPL(find_cxl_root, CXL);
>>  
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index df3db3e43913..3a5004aab97a 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -617,12 +617,6 @@ struct cxl_port {
>>  	long pci_latency;
>>  };
>>  
>> -struct cxl_root_ops {
>> -	int (*qos_class)(struct cxl_port *root_port,
>> -			 struct access_coordinate *coord, int entries,
>> -			 int *qos_class);
>> -};
>> -
>>  /**
>>   * struct cxl_root - logical collection of root cxl_port items
>>   *
>> @@ -640,6 +634,12 @@ to_cxl_root(const struct cxl_port *port)
>>  	return container_of(port, struct cxl_root, port);
>>  }
>>  
>> +struct cxl_root_ops {
>> +	int (*qos_class)(struct cxl_root *cxl_root,
>> +			 struct access_coordinate *coord, int entries,
>> +			 int *qos_class);
>> +};
>> +
> 
> Any intention to move code here in addition?

Yes. Needed to move that under the 'struct cxl_root' declaration. Either that or we add a 'struct cxl_root;' above it. But since 'struct cxl_root' is declared a few lines down, I figure I just move it. 
> 
>>  static inline struct cxl_dport *
>>  cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
>>  {
>> @@ -734,7 +734,7 @@ struct cxl_port *devm_cxl_add_port(struct device *host,
>>  				   struct cxl_dport *parent_dport);
>>  struct cxl_root *devm_cxl_add_root(struct device *host,
>>  				   const struct cxl_root_ops *ops);
>> -struct cxl_port *find_cxl_root(struct cxl_port *port);
>> +struct cxl_root *find_cxl_root(struct cxl_port *port);
>>  void put_cxl_root(struct cxl_root *cxl_root);
>>  DEFINE_FREE(put_cxl_root, struct cxl_root *, if (_T) put_cxl_root(_T))
>>  
>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>> index da3c3a08bd62..4f3a08fdc9e9 100644
>> --- a/drivers/cxl/port.c
>> +++ b/drivers/cxl/port.c
>> @@ -94,6 +94,7 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>>  	struct cxl_endpoint_dvsec_info info = { .port = port };
>>  	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
>>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> +	struct cxl_root *cxl_root;
>>  	struct cxl_hdm *cxlhdm;
>>  	struct cxl_port *root;
>>  	int rc;
>> @@ -130,7 +131,8 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>>  	 * This can't fail in practice as CXL root exit unregisters all
>>  	 * descendant ports and that in turn synchronizes with cxl_port_probe()
>>  	 */
>> -	root = find_cxl_root(port);
>> +	cxl_root = find_cxl_root(port);
>> +	root = &cxl_root->port;
> 
> Same here, no need for the root var.

ok

> 
> -Robert
> 
> 
>>  
>>  	/*
>>  	 * Now that all endpoint decoders are successfully enumerated, try to
>>
>>

Re: [PATCH v6 5/5] cxl: Refactor to use __free() for cxl_root allocation in cxl_endpoint_port_probe()

[Robert Richter]

On 05.01.24 15:07:59, Dave Jiang wrote:
> Use scope-based resource management __free() macro to drop the open coded
> put_device() in cxl_endpoint_port_probe().
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v6:
> - Cleanup put_device()
> v5:
> - Update commit log (Dan)
> v4:
> - Don't check return value of find_cxl_root() per comment. (Dan)
> ---
>  drivers/cxl/port.c |    5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index 4f3a08fdc9e9..97c21566677a 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -94,7 +94,6 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>  	struct cxl_endpoint_dvsec_info info = { .port = port };
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> -	struct cxl_root *cxl_root;
>  	struct cxl_hdm *cxlhdm;
>  	struct cxl_port *root;
>  	int rc;
> @@ -131,7 +130,8 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>  	 * This can't fail in practice as CXL root exit unregisters all
>  	 * descendant ports and that in turn synchronizes with cxl_port_probe()
>  	 */
> -	cxl_root = find_cxl_root(port);
> +	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);

Same here, definition is not at the beginning of the block.

> +
>  	root = &cxl_root->port;

root var can be dropped.

-Robert

>  
>  	/*
> @@ -139,7 +139,6 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>  	 * assemble regions from committed decoders
>  	 */
>  	device_for_each_child(&port->dev, root, discover_region);
> -	put_device(&root->dev);
>  
>  	return 0;
>  }
> 
> 

Re: [PATCH v6 2/5] cxl: Convert find_cxl_root() to return a 'struct cxl_root *'

[Dave Jiang]

On 1/5/24 15:51, Robert Richter wrote:
> See comments below.
> 
> On 05.01.24 15:07:40, Dave Jiang wrote:
>> Commit 790815902ec6 ("cxl: Add support for _DSM Function for retrieving QTG ID")
>> introduced 'struct cxl_root', however all usages have been worked
>> indirectly through cxl_port. Refactor code such as find_cxl_root()
>> function to use 'struct cxl_root' directly.
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> v6:
>> - Revert code that broke device_for_each_child() (Dan)
>> - Leave put_device() calls in this patch. (Dan)
>> v5:
>> - Squash in v4 3/6 to update the qos_class to cxl_root. (Dan)
>> - Moved the introduction of __free() for cxl_root_put from v4 1/6 to
>>   here. (Dan)
>> v4:
>> - Adjust ordering of patches to move this to 2nd place. (Dan)
>> ---
>>  drivers/cxl/acpi.c      |    6 ++----
>>  drivers/cxl/core/cdat.c |   17 ++++++++++-------
>>  drivers/cxl/core/pmem.c |    6 ++++--
>>  drivers/cxl/core/port.c |    4 ++--
>>  drivers/cxl/cxl.h       |   14 +++++++-------
>>  drivers/cxl/port.c      |    4 +++-
>>  6 files changed, 28 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
>> index afc712264d1c..dcf2b39e1048 100644
>> --- a/drivers/cxl/acpi.c
>> +++ b/drivers/cxl/acpi.c
>> @@ -295,14 +295,12 @@ cxl_acpi_evaluate_qtg_dsm(acpi_handle handle, struct access_coordinate *coord,
>>  	return rc;
>>  }
>>  
>> -static int cxl_acpi_qos_class(struct cxl_port *root_port,
>> +static int cxl_acpi_qos_class(struct cxl_root *cxl_root,
>>  			      struct access_coordinate *coord, int entries,
>>  			      int *qos_class)
>>  {
>> +	struct device *dev = cxl_root->port.uport_dev;
>>  	acpi_handle handle;
>> -	struct device *dev;
>> -
>> -	dev = root_port->uport_dev;
>>  
>>  	if (!dev_is_platform(dev))
>>  		return -ENODEV;
>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>> index cd84d87f597a..0df5379cf02f 100644
>> --- a/drivers/cxl/core/cdat.c
>> +++ b/drivers/cxl/core/cdat.c
>> @@ -162,7 +162,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>>  					struct xarray *dsmas_xa)
>>  {
>>  	struct access_coordinate c;
>> -	struct cxl_port *root_port;
>>  	struct cxl_root *cxl_root;
>>  	struct dsmas_entry *dent;
>>  	int valid_entries = 0;
>> @@ -175,8 +174,7 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>>  		return rc;
>>  	}
>>  
>> -	root_port = find_cxl_root(port);
>> -	cxl_root = to_cxl_root(root_port);
>> +	cxl_root = find_cxl_root(port);
>>  	if (!cxl_root->ops || !cxl_root->ops->qos_class)
>>  		return -EOPNOTSUPP;
>>  
>> @@ -193,7 +191,8 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>>  						    dent->coord.write_bandwidth);
>>  
>>  		dent->entries = 1;
>> -		rc = cxl_root->ops->qos_class(root_port, &dent->coord, 1, &qos_class);
>> +		rc = cxl_root->ops->qos_class(cxl_root, &dent->coord, 1,
>> +					      &qos_class);
>>  		if (rc != 1)
>>  			continue;
>>  
>> @@ -349,15 +348,19 @@ static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
>>  {
>>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
>> -	struct cxl_port *root_port __free(put_device) = NULL;
>>  	LIST_HEAD(__discard);
>>  	struct list_head *discard __free(dpa_perf) = &__discard;
>> +	struct cxl_port *root_port;
>>  	int rc;
>>  
>> -	root_port = find_cxl_root(cxlmd->endpoint);
>> -	if (!root_port)
>> +	struct cxl_root *cxl_root __free(put_cxl_root) =
>> +		find_cxl_root(cxlmd->endpoint);
> 
> That's the drawback that lines get very long. Not sure if that was
> discussed before, maybe just assign NULL in the definition and then
> have the first assignment right after?
> 
> Should be moved to the definitions above without a newline.
> 
>> +
>> +	if (!cxl_root)
>>  		return -ENODEV;
>>  
>> +	root_port = &cxl_root->port;
>> +
>>  	/* Check that the QTG IDs are all sane between end device and root decoders */
>>  	cxl_qos_match(root_port, &mds->ram_perf_list, discard);
>>  	cxl_qos_match(root_port, &mds->pmem_perf_list, discard);
> 
> It looks like cxl_qos_match() consumes a root port, so we could pass
> cxl_root here directly and get rid of root_port.
> 
>> diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
>> index fc94f5240327..da92a901b9e8 100644
>> --- a/drivers/cxl/core/pmem.c
>> +++ b/drivers/cxl/core/pmem.c
>> @@ -64,12 +64,14 @@ static int match_nvdimm_bridge(struct device *dev, void *data)
>>  
>>  struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_memdev *cxlmd)
>>  {
>> -	struct cxl_port *port = find_cxl_root(cxlmd->endpoint);
>> +	struct cxl_root *cxl_root = find_cxl_root(cxlmd->endpoint);
>> +	struct cxl_port *port;
>>  	struct device *dev;
>>  
>> -	if (!port)
>> +	if (!cxl_root)
>>  		return NULL;
>>  
>> +	port = &cxl_root->port;
>>  	dev = device_find_child(&port->dev, NULL, match_nvdimm_bridge);
> 
> No need for port variable here.
> 
>>  	put_device(&port->dev);
>>  
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 64f30d5fe1f6..63a4e3c2baed 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -972,7 +972,7 @@ static bool dev_is_cxl_root_child(struct device *dev)
>>  	return false;
>>  }
>>  
>> -struct cxl_port *find_cxl_root(struct cxl_port *port)
>> +struct cxl_root *find_cxl_root(struct cxl_port *port)
>>  {
>>  	struct cxl_port *iter = port;
>>  
>> @@ -982,7 +982,7 @@ struct cxl_port *find_cxl_root(struct cxl_port *port)
>>  	if (!iter)
>>  		return NULL;
>>  	get_device(&iter->dev);
>> -	return iter;
>> +	return to_cxl_root(iter);
> 
> I guess this is the last user of to_cxl_root() now so we can remove
> the macro entirely. Unlikely we will ever need it again and a
> container_of() call is also fairly easy.

Looks like there are two other locations using it still:

$ git grep -n "to_cxl_root("
core/port.c:546:                kfree(to_cxl_root(port));
core/port.c:914:        cxl_root = to_cxl_root(port);
core/port.c:985:        return to_cxl_root(iter);
cxl.h:632:to_cxl_root(const struct cxl_port *port)

So we can keep this macro for now.

DJ

> 
>>  }
>>  EXPORT_SYMBOL_NS_GPL(find_cxl_root, CXL);
>>  
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index df3db3e43913..3a5004aab97a 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -617,12 +617,6 @@ struct cxl_port {
>>  	long pci_latency;
>>  };
>>  
>> -struct cxl_root_ops {
>> -	int (*qos_class)(struct cxl_port *root_port,
>> -			 struct access_coordinate *coord, int entries,
>> -			 int *qos_class);
>> -};
>> -
>>  /**
>>   * struct cxl_root - logical collection of root cxl_port items
>>   *
>> @@ -640,6 +634,12 @@ to_cxl_root(const struct cxl_port *port)
>>  	return container_of(port, struct cxl_root, port);
>>  }
>>  
>> +struct cxl_root_ops {
>> +	int (*qos_class)(struct cxl_root *cxl_root,
>> +			 struct access_coordinate *coord, int entries,
>> +			 int *qos_class);
>> +};
>> +
> 
> Any intention to move code here in addition?
> 
>>  static inline struct cxl_dport *
>>  cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
>>  {
>> @@ -734,7 +734,7 @@ struct cxl_port *devm_cxl_add_port(struct device *host,
>>  				   struct cxl_dport *parent_dport);
>>  struct cxl_root *devm_cxl_add_root(struct device *host,
>>  				   const struct cxl_root_ops *ops);
>> -struct cxl_port *find_cxl_root(struct cxl_port *port);
>> +struct cxl_root *find_cxl_root(struct cxl_port *port);
>>  void put_cxl_root(struct cxl_root *cxl_root);
>>  DEFINE_FREE(put_cxl_root, struct cxl_root *, if (_T) put_cxl_root(_T))
>>  
>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>> index da3c3a08bd62..4f3a08fdc9e9 100644
>> --- a/drivers/cxl/port.c
>> +++ b/drivers/cxl/port.c
>> @@ -94,6 +94,7 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>>  	struct cxl_endpoint_dvsec_info info = { .port = port };
>>  	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
>>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> +	struct cxl_root *cxl_root;
>>  	struct cxl_hdm *cxlhdm;
>>  	struct cxl_port *root;
>>  	int rc;
>> @@ -130,7 +131,8 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>>  	 * This can't fail in practice as CXL root exit unregisters all
>>  	 * descendant ports and that in turn synchronizes with cxl_port_probe()
>>  	 */
>> -	root = find_cxl_root(port);
>> +	cxl_root = find_cxl_root(port);
>> +	root = &cxl_root->port;
> 
> Same here, no need for the root var.
> 
> -Robert
> 
> 
>>  
>>  	/*
>>  	 * Now that all endpoint decoders are successfully enumerated, try to
>>
>>

Re: [PATCH v6 3/5] cxl: Fix device reference leak in cxl_port_perf_data_calculate()

[Dave Jiang]

On 1/5/24 15:56, Robert Richter wrote:
> See inline.
> 
> On 05.01.24 15:07:46, Dave Jiang wrote:
>> cxl_port_perf_data_calculate() calls find_cxl_root() and does not
>> dereference the 'struct device' in the cxl_root->port. find_cxl_root()
>> calls get_device() and takes a reference on the port 'struct device'
>> member. Use the __free() macro to ensure the dereference happens.
>>
>> Fixes: 7a4f148dd8d5 ("cxl: Compute the entire CXL path latency and bandwidth data")
>> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> v5:
>> - Update patch title (Dan)
>> ---
>>  drivers/cxl/core/cdat.c |    7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>> index 0df5379cf02f..c6208aab452f 100644
>> --- a/drivers/cxl/core/cdat.c
>> +++ b/drivers/cxl/core/cdat.c
>> @@ -162,7 +162,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>>  					struct xarray *dsmas_xa)
>>  {
>>  	struct access_coordinate c;
>> -	struct cxl_root *cxl_root;
>>  	struct dsmas_entry *dent;
>>  	int valid_entries = 0;
>>  	unsigned long index;
>> @@ -174,7 +173,11 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>>  		return rc;
>>  	}
>>  
>> -	cxl_root = find_cxl_root(port);
>> +	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
> 
> I am not quite sure here, can a definition in the middle of a block?
> At least this is uncommon.

From what I understand with the introduction of the scope-based resource management, this kind of declaration is ok for that purpose? Dan?

> 
>> +
>> +	if (!cxl_root)
>> +		return -ENODEV;
> 
> As Dan pointed out earlier, isn't there always a root port in the
> hierarchy? So the check could be dropped?
> 
>> +
>>  	if (!cxl_root->ops || !cxl_root->ops->qos_class)
>>  		return -EOPNOTSUPP;
>>  
>>
>>

Re: [PATCH v6 0/5] cxl: find_cxl_root() related cleanups

[Jonathan Cameron]

On Fri, 5 Jan 2024 14:35:14 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Dave Jiang wrote:
> > v6:
> > - Fix breakage in previous series for device_find_child() (Dan)
> > - Move DEFINE_FREE() for put_cxl_root() to first patch (Dan)
> > - Don't touch put_device() until final cleanup. (Dan)
> > - Add review tag from Ira.
> > 
> > This series provides a number of small cleanups to make fix_cxl_root()
> > and related code more readable and safer.  
> 
> Looks good, Dave! Thanks for the spins.
> 
Subject to Robert's note on removing the root variable in patch 5 as it
now appears unused, this series looks fine to me (and I guess Dan can
probably just fix that up in the tree)

FWIW given this is already queued in in cxl/next

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v6 2/5] cxl: Convert find_cxl_root() to return a 'struct cxl_root *'

[Dan Williams]

Robert Richter wrote:
> See comments below.
> 
> On 05.01.24 15:07:40, Dave Jiang wrote:
> > Commit 790815902ec6 ("cxl: Add support for _DSM Function for retrieving QTG ID")
> > introduced 'struct cxl_root', however all usages have been worked
> > indirectly through cxl_port. Refactor code such as find_cxl_root()
> > function to use 'struct cxl_root' directly.
> > 
> > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > ---
> > v6:
> > - Revert code that broke device_for_each_child() (Dan)
> > - Leave put_device() calls in this patch. (Dan)
> > v5:
> > - Squash in v4 3/6 to update the qos_class to cxl_root. (Dan)
> > - Moved the introduction of __free() for cxl_root_put from v4 1/6 to
> >   here. (Dan)
> > v4:
> > - Adjust ordering of patches to move this to 2nd place. (Dan)
> > ---
> >  drivers/cxl/acpi.c      |    6 ++----
> >  drivers/cxl/core/cdat.c |   17 ++++++++++-------
> >  drivers/cxl/core/pmem.c |    6 ++++--
> >  drivers/cxl/core/port.c |    4 ++--
> >  drivers/cxl/cxl.h       |   14 +++++++-------
> >  drivers/cxl/port.c      |    4 +++-
> >  6 files changed, 28 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index afc712264d1c..dcf2b39e1048 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -295,14 +295,12 @@ cxl_acpi_evaluate_qtg_dsm(acpi_handle handle, struct access_coordinate *coord,
> >  	return rc;
> >  }
> >  
> > -static int cxl_acpi_qos_class(struct cxl_port *root_port,
> > +static int cxl_acpi_qos_class(struct cxl_root *cxl_root,
> >  			      struct access_coordinate *coord, int entries,
> >  			      int *qos_class)
> >  {
> > +	struct device *dev = cxl_root->port.uport_dev;
> >  	acpi_handle handle;
> > -	struct device *dev;
> > -
> > -	dev = root_port->uport_dev;
> >  
> >  	if (!dev_is_platform(dev))
> >  		return -ENODEV;
> > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> > index cd84d87f597a..0df5379cf02f 100644
> > --- a/drivers/cxl/core/cdat.c
> > +++ b/drivers/cxl/core/cdat.c
> > @@ -162,7 +162,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
> >  					struct xarray *dsmas_xa)
> >  {
> >  	struct access_coordinate c;
> > -	struct cxl_port *root_port;
> >  	struct cxl_root *cxl_root;
> >  	struct dsmas_entry *dent;
> >  	int valid_entries = 0;
> > @@ -175,8 +174,7 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
> >  		return rc;
> >  	}
> >  
> > -	root_port = find_cxl_root(port);
> > -	cxl_root = to_cxl_root(root_port);
> > +	cxl_root = find_cxl_root(port);
> >  	if (!cxl_root->ops || !cxl_root->ops->qos_class)
> >  		return -EOPNOTSUPP;
> >  
> > @@ -193,7 +191,8 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
> >  						    dent->coord.write_bandwidth);
> >  
> >  		dent->entries = 1;
> > -		rc = cxl_root->ops->qos_class(root_port, &dent->coord, 1, &qos_class);
> > +		rc = cxl_root->ops->qos_class(cxl_root, &dent->coord, 1,
> > +					      &qos_class);
> >  		if (rc != 1)
> >  			continue;
> >  
> > @@ -349,15 +348,19 @@ static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
> >  {
> >  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> >  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> > -	struct cxl_port *root_port __free(put_device) = NULL;
> >  	LIST_HEAD(__discard);
> >  	struct list_head *discard __free(dpa_perf) = &__discard;
> > +	struct cxl_port *root_port;
> >  	int rc;
> >  
> > -	root_port = find_cxl_root(cxlmd->endpoint);
> > -	if (!root_port)
> > +	struct cxl_root *cxl_root __free(put_cxl_root) =
> > +		find_cxl_root(cxlmd->endpoint);
> 
> That's the drawback that lines get very long. Not sure if that was
> discussed before, maybe just assign NULL in the definition and then
> have the first assignment right after?

That style recommendation was something we were discussing here:

http://lore.kernel.org/r/20240104223725.GA1829769@bhelgaas

...where the outcome I took away was "try to keep the get/put in the
same statement, especially if the function has multiple usages of
__free()".

> 
> Should be moved to the definitions above without a newline.
> 
> > +
> > +	if (!cxl_root)
> >  		return -ENODEV;
> >  
> > +	root_port = &cxl_root->port;
> > +
> >  	/* Check that the QTG IDs are all sane between end device and root decoders */
> >  	cxl_qos_match(root_port, &mds->ram_perf_list, discard);
> >  	cxl_qos_match(root_port, &mds->pmem_perf_list, discard);
> 
> It looks like cxl_qos_match() consumes a root port, so we could pass
> cxl_root here directly and get rid of root_port.

Agree, that would be a useful incremental cleanup.

> 
> > diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
> > index fc94f5240327..da92a901b9e8 100644
> > --- a/drivers/cxl/core/pmem.c
> > +++ b/drivers/cxl/core/pmem.c
> > @@ -64,12 +64,14 @@ static int match_nvdimm_bridge(struct device *dev, void *data)
> >  
> >  struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_memdev *cxlmd)
> >  {
> > -	struct cxl_port *port = find_cxl_root(cxlmd->endpoint);
> > +	struct cxl_root *cxl_root = find_cxl_root(cxlmd->endpoint);
> > +	struct cxl_port *port;
> >  	struct device *dev;
> >  
> > -	if (!port)
> > +	if (!cxl_root)
> >  		return NULL;
> >  
> > +	port = &cxl_root->port;
> >  	dev = device_find_child(&port->dev, NULL, match_nvdimm_bridge);
> 
> No need for port variable here.

Agree, but to me that's a follow on cleanup. It's nice not to need to
review the changes to the device_find_child() line. In fact, there was
already one breakage of mixing up the arguments to
device_for_each_child() in this set, so minimizing the collateral
changes with a local variable is what I asked for.

> 
> >  	put_device(&port->dev);
> >  
> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index 64f30d5fe1f6..63a4e3c2baed 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -972,7 +972,7 @@ static bool dev_is_cxl_root_child(struct device *dev)
> >  	return false;
> >  }
> >  
> > -struct cxl_port *find_cxl_root(struct cxl_port *port)
> > +struct cxl_root *find_cxl_root(struct cxl_port *port)
> >  {
> >  	struct cxl_port *iter = port;
> >  
> > @@ -982,7 +982,7 @@ struct cxl_port *find_cxl_root(struct cxl_port *port)
> >  	if (!iter)
> >  		return NULL;
> >  	get_device(&iter->dev);
> > -	return iter;
> > +	return to_cxl_root(iter);
> 
> I guess this is the last user of to_cxl_root() now so we can remove
> the macro entirely. Unlikely we will ever need it again and a
> container_of() call is also fairly easy.
> 
> >  }
> >  EXPORT_SYMBOL_NS_GPL(find_cxl_root, CXL);
> >  
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index df3db3e43913..3a5004aab97a 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -617,12 +617,6 @@ struct cxl_port {
> >  	long pci_latency;
> >  };
> >  
> > -struct cxl_root_ops {
> > -	int (*qos_class)(struct cxl_port *root_port,
> > -			 struct access_coordinate *coord, int entries,
> > -			 int *qos_class);
> > -};
> > -
> >  /**
> >   * struct cxl_root - logical collection of root cxl_port items
> >   *
> > @@ -640,6 +634,12 @@ to_cxl_root(const struct cxl_port *port)
> >  	return container_of(port, struct cxl_root, port);
> >  }
> >  
> > +struct cxl_root_ops {
> > +	int (*qos_class)(struct cxl_root *cxl_root,
> > +			 struct access_coordinate *coord, int entries,
> > +			 int *qos_class);
> > +};
> > +
> 
> Any intention to move code here in addition?
> 
> >  static inline struct cxl_dport *
> >  cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
> >  {
> > @@ -734,7 +734,7 @@ struct cxl_port *devm_cxl_add_port(struct device *host,
> >  				   struct cxl_dport *parent_dport);
> >  struct cxl_root *devm_cxl_add_root(struct device *host,
> >  				   const struct cxl_root_ops *ops);
> > -struct cxl_port *find_cxl_root(struct cxl_port *port);
> > +struct cxl_root *find_cxl_root(struct cxl_port *port);
> >  void put_cxl_root(struct cxl_root *cxl_root);
> >  DEFINE_FREE(put_cxl_root, struct cxl_root *, if (_T) put_cxl_root(_T))
> >  
> > diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> > index da3c3a08bd62..4f3a08fdc9e9 100644
> > --- a/drivers/cxl/port.c
> > +++ b/drivers/cxl/port.c
> > @@ -94,6 +94,7 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
> >  	struct cxl_endpoint_dvsec_info info = { .port = port };
> >  	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
> >  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> > +	struct cxl_root *cxl_root;
> >  	struct cxl_hdm *cxlhdm;
> >  	struct cxl_port *root;
> >  	int rc;
> > @@ -130,7 +131,8 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
> >  	 * This can't fail in practice as CXL root exit unregisters all
> >  	 * descendant ports and that in turn synchronizes with cxl_port_probe()
> >  	 */
> > -	root = find_cxl_root(port);
> > +	cxl_root = find_cxl_root(port);
> > +	root = &cxl_root->port;
> 
> Same here, no need for the root var.

Again, this one I like here because it keeps from needing to change:

    device_for_each_child(&port->dev, root, discover_region);

...in the same patch, and avoided a bug in one version of this series
that did, something like:

    device_for_each_child(&cxl_root->port.dev, &cxl_root->port, discover_region);

...i.e. inadvertantly replaced the first argument.

Re: [PATCH v6 3/5] cxl: Fix device reference leak in cxl_port_perf_data_calculate()

[Dan Williams]

Dave Jiang wrote:
> 
> 
> On 1/5/24 15:56, Robert Richter wrote:
> > See inline.
> > 
> > On 05.01.24 15:07:46, Dave Jiang wrote:
> >> cxl_port_perf_data_calculate() calls find_cxl_root() and does not
> >> dereference the 'struct device' in the cxl_root->port. find_cxl_root()
> >> calls get_device() and takes a reference on the port 'struct device'
> >> member. Use the __free() macro to ensure the dereference happens.
> >>
> >> Fixes: 7a4f148dd8d5 ("cxl: Compute the entire CXL path latency and bandwidth data")
> >> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
[..]
> >> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> >> index 0df5379cf02f..c6208aab452f 100644
> >> --- a/drivers/cxl/core/cdat.c
> >> +++ b/drivers/cxl/core/cdat.c
> >> @@ -162,7 +162,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
> >>  					struct xarray *dsmas_xa)
> >>  {
> >>  	struct access_coordinate c;
> >> -	struct cxl_root *cxl_root;
> >>  	struct dsmas_entry *dent;
> >>  	int valid_entries = 0;
> >>  	unsigned long index;
> >> @@ -174,7 +173,11 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
> >>  		return rc;
> >>  	}
> >>  
> >> -	cxl_root = find_cxl_root(port);
> >> +	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
> > 
> > I am not quite sure here, can a definition in the middle of a block?
> > At least this is uncommon.
> 
> From what I understand with the introduction of the scope-based
> resource management, this kind of declaration is ok for that purpose?
> Dan?

The relaxation of the "variables declared in the middle of a block" came
with the move to compiling with -std=gnu11:

e8c07082a810 Kbuild: move to -std=gnu11

...back in v5.18, and Peter took it further in his examples of using the
cleanup.h helpers to keep "get/put" in the same statement, which means
combining declaration and intialization, and means initialization may
need to happen after the initial variable declaration block.

I agree that it is a bit jarring to see given the legacy, but I think
this a good reason for an exception, along with some of the others like
declaring iterator variables in for () loops.

Re: [PATCH v6 0/5] cxl: find_cxl_root() related cleanups

[Dan Williams]

Jonathan Cameron wrote:
> On Fri, 5 Jan 2024 14:35:14 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Dave Jiang wrote:
> > > v6:
> > > - Fix breakage in previous series for device_find_child() (Dan)
> > > - Move DEFINE_FREE() for put_cxl_root() to first patch (Dan)
> > > - Don't touch put_device() until final cleanup. (Dan)
> > > - Add review tag from Ira.
> > > 
> > > This series provides a number of small cleanups to make fix_cxl_root()
> > > and related code more readable and safer.  
> > 
> > Looks good, Dave! Thanks for the spins.
> > 
> Subject to Robert's note on removing the root variable in patch 5 as it
> now appears unused, this series looks fine to me (and I guess Dan can
> probably just fix that up in the tree)
> 
> FWIW given this is already queued in in cxl/next
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks Jonathan, will add that to the pull request.

Re: [PATCH v6 2/5] cxl: Convert find_cxl_root() to return a 'struct cxl_root *'

[Robert Richter]

On 08.01.24 12:43:50, Dan Williams wrote:
> Robert Richter wrote:
> > On 05.01.24 15:07:40, Dave Jiang wrote:

> > > @@ -349,15 +348,19 @@ static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
> > >  {
> > >  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> > >  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> > > -	struct cxl_port *root_port __free(put_device) = NULL;
> > >  	LIST_HEAD(__discard);
> > >  	struct list_head *discard __free(dpa_perf) = &__discard;
> > > +	struct cxl_port *root_port;
> > >  	int rc;
> > >  
> > > -	root_port = find_cxl_root(cxlmd->endpoint);
> > > -	if (!root_port)
> > > +	struct cxl_root *cxl_root __free(put_cxl_root) =
> > > +		find_cxl_root(cxlmd->endpoint);
> > 
> > That's the drawback that lines get very long. Not sure if that was
> > discussed before, maybe just assign NULL in the definition and then
> > have the first assignment right after?
> 
> That style recommendation was something we were discussing here:
> 
> http://lore.kernel.org/r/20240104223725.GA1829769@bhelgaas
> 
> ...where the outcome I took away was "try to keep the get/put in the
> same statement, especially if the function has multiple usages of
> __free()".

I see the point here. An alternative would be to have one __free per
block, but additional blocks would introduce indentation. Maybe worth
to consider anyway as that addresses the style issue too.

I am fine that my other comments are addressed with top patches too.

Thanks,

-Robert

[PATCH 1/3] cxl: Remove cxl_root check after find_cxl_root

[Dave Jiang]

find_cxl_root() can't fail in practice as CXL root exit unregisters all
descendant ports and that in turn synchronizes with cxl_port_probe().
Remove unnecessary check after calling find_cxl_root().

Suggested-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/cdat.c |    6 ------
 drivers/cxl/core/pmem.c |    3 ---
 2 files changed, 9 deletions(-)

diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index 6fe11546889f..f7ba7bd2e459 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -176,9 +176,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
 
 	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
 
-	if (!cxl_root)
-		return -ENODEV;
-
 	if (!cxl_root->ops || !cxl_root->ops->qos_class)
 		return -EOPNOTSUPP;
 
@@ -357,9 +354,6 @@ static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
 	struct cxl_root *cxl_root __free(put_cxl_root) =
 		find_cxl_root(cxlmd->endpoint);
 
-	if (!cxl_root)
-		return -ENODEV;
-
 	root_port = &cxl_root->port;
 
 	/* Check that the QTG IDs are all sane between end device and root decoders */
diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
index e69625a8d6a1..ed76d37e4fd9 100644
--- a/drivers/cxl/core/pmem.c
+++ b/drivers/cxl/core/pmem.c
@@ -68,9 +68,6 @@ struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_memdev *cxlmd)
 		find_cxl_root(cxlmd->endpoint);
 	struct device *dev;
 
-	if (!cxl_root)
-		return NULL;
-
 	dev = device_find_child(&cxl_root->port.dev, NULL, match_nvdimm_bridge);
 
 	if (!dev)

[PATCH 2/3] cxl: Cleanup unnecessary uages of cxl_port local vars from cxl_root

[Dave Jiang]

Remove the local vars that points to the 'struct cxl_port' withint 'struct
cxl_root' and refer to the port directly.

Suggested-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/cdat.c |   13 +++++--------
 drivers/cxl/port.c      |    5 +----
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index f7ba7bd2e459..140935511bab 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -290,7 +290,7 @@ static int match_cxlrd_qos_class(struct device *dev, void *data)
 	return 0;
 }
 
-static void cxl_qos_match(struct cxl_port *root_port,
+static void cxl_qos_match(struct cxl_root *cxl_root,
 			  struct list_head *work_list,
 			  struct list_head *discard_list)
 {
@@ -302,7 +302,7 @@ static void cxl_qos_match(struct cxl_port *root_port,
 		if (dpa_perf->qos_class == CXL_QOS_CLASS_INVALID)
 			return;
 
-		rc = device_for_each_child(&root_port->dev,
+		rc = device_for_each_child(&cxl_root->port.dev,
 					   (void *)&dpa_perf->qos_class,
 					   match_cxlrd_qos_class);
 		if (!rc)
@@ -348,20 +348,17 @@ static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
 	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
 	LIST_HEAD(__discard);
 	struct list_head *discard __free(dpa_perf) = &__discard;
-	struct cxl_port *root_port;
 	int rc;
 
 	struct cxl_root *cxl_root __free(put_cxl_root) =
 		find_cxl_root(cxlmd->endpoint);
 
-	root_port = &cxl_root->port;
-
 	/* Check that the QTG IDs are all sane between end device and root decoders */
-	cxl_qos_match(root_port, &mds->ram_perf_list, discard);
-	cxl_qos_match(root_port, &mds->pmem_perf_list, discard);
+	cxl_qos_match(cxl_root, &mds->ram_perf_list, discard);
+	cxl_qos_match(cxl_root, &mds->pmem_perf_list, discard);
 
 	/* Check to make sure that the device's host bridge is under a root decoder */
-	rc = device_for_each_child(&root_port->dev,
+	rc = device_for_each_child(&cxl_root->port.dev,
 				   (void *)cxlmd->endpoint->host_bridge,
 				   match_cxlrd_hb);
 	if (!rc) {
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index 97c21566677a..c054e7b13bdd 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -95,7 +95,6 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
 	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_hdm *cxlhdm;
-	struct cxl_port *root;
 	int rc;
 
 	rc = cxl_dvsec_rr_decode(cxlds->dev, cxlds->cxl_dvsec, &info);
@@ -132,13 +131,11 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
 	 */
 	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
 
-	root = &cxl_root->port;
-
 	/*
 	 * Now that all endpoint decoders are successfully enumerated, try to
 	 * assemble regions from committed decoders
 	 */
-	device_for_each_child(&port->dev, root, discover_region);
+	device_for_each_child(&port->dev, &cxl_root->port, discover_region);
 
 	return 0;
 }

[PATCH 3/3] cxl: Make calling of find_cxl_root() declaration uniform

[Dave Jiang]

Move the calling find_cxl_port() and the variable declaration into the
var declaration block and remove line breaks in the declaration. There are
no ordering issues that would require the declaration to be in middle of
the code.

Suggested-by: Robert Richter <rrichter@amd.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/cdat.c |    7 ++-----
 drivers/cxl/core/pmem.c |    3 +--
 drivers/cxl/port.c      |   11 +++++------
 3 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index 140935511bab..1d830d088389 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -162,6 +162,7 @@ static int cxl_cdat_endpoint_process(struct cxl_port *port,
 static int cxl_port_perf_data_calculate(struct cxl_port *port,
 					struct xarray *dsmas_xa)
 {
+	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
 	struct access_coordinate c;
 	struct dsmas_entry *dent;
 	int valid_entries = 0;
@@ -174,8 +175,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
 		return rc;
 	}
 
-	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
-
 	if (!cxl_root->ops || !cxl_root->ops->qos_class)
 		return -EOPNOTSUPP;
 
@@ -344,15 +343,13 @@ DEFINE_FREE(dpa_perf, struct list_head *, if (!list_empty(_T)) discard_dpa_perf(
 
 static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
 {
+	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(cxlmd->endpoint);
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
 	LIST_HEAD(__discard);
 	struct list_head *discard __free(dpa_perf) = &__discard;
 	int rc;
 
-	struct cxl_root *cxl_root __free(put_cxl_root) =
-		find_cxl_root(cxlmd->endpoint);
-
 	/* Check that the QTG IDs are all sane between end device and root decoders */
 	cxl_qos_match(cxl_root, &mds->ram_perf_list, discard);
 	cxl_qos_match(cxl_root, &mds->pmem_perf_list, discard);
diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
index ed76d37e4fd9..976212e588bc 100644
--- a/drivers/cxl/core/pmem.c
+++ b/drivers/cxl/core/pmem.c
@@ -64,8 +64,7 @@ static int match_nvdimm_bridge(struct device *dev, void *data)
 
 struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_memdev *cxlmd)
 {
-	struct cxl_root *cxl_root __free(put_cxl_root) =
-		find_cxl_root(cxlmd->endpoint);
+	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(cxlmd->endpoint);
 	struct device *dev;
 
 	dev = device_find_child(&cxl_root->port.dev, NULL, match_nvdimm_bridge);
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index c054e7b13bdd..2bacf48593e6 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -91,6 +91,11 @@ static int cxl_switch_port_probe(struct cxl_port *port)
 
 static int cxl_endpoint_port_probe(struct cxl_port *port)
 {
+	/*
+	 * This can't fail in practice as CXL root exit unregisters all
+	 * descendant ports and that in turn synchronizes with cxl_port_probe()
+	 */
+	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
 	struct cxl_endpoint_dvsec_info info = { .port = port };
 	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
@@ -125,12 +130,6 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
 	if (rc)
 		return rc;
 
-	/*
-	 * This can't fail in practice as CXL root exit unregisters all
-	 * descendant ports and that in turn synchronizes with cxl_port_probe()
-	 */
-	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
-
 	/*
 	 * Now that all endpoint decoders are successfully enumerated, try to
 	 * assemble regions from committed decoders

RE: [PATCH 1/3] cxl: Remove cxl_root check after find_cxl_root

[Dan Williams]

Dave Jiang wrote:
> find_cxl_root() can't fail in practice as CXL root exit unregisters all
> descendant ports and that in turn synchronizes with cxl_port_probe().
> Remove unnecessary check after calling find_cxl_root().
> 
> Suggested-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/cdat.c |    6 ------
>  drivers/cxl/core/pmem.c |    3 ---
>  2 files changed, 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index 6fe11546889f..f7ba7bd2e459 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -176,9 +176,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>  
>  	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
>  
> -	if (!cxl_root)
> -		return -ENODEV;
> -
>  	if (!cxl_root->ops || !cxl_root->ops->qos_class)
>  		return -EOPNOTSUPP;
>  
> @@ -357,9 +354,6 @@ static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
>  	struct cxl_root *cxl_root __free(put_cxl_root) =
>  		find_cxl_root(cxlmd->endpoint);
>  
> -	if (!cxl_root)
> -		return -ENODEV;
> -

These are kind of ok because cxl_port_perf_data_calculate() is called
from cxl_endpoint_port_probe() which has the exact comment you reference
in the changelog. However it is not clear a couple of calls away that
cxl_port_perf_data_calculate() can / should be assuming its calling
context.

I would be inclined to keep them.

>  	root_port = &cxl_root->port;
>  
>  	/* Check that the QTG IDs are all sane between end device and root decoders */
> diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
> index e69625a8d6a1..ed76d37e4fd9 100644
> --- a/drivers/cxl/core/pmem.c
> +++ b/drivers/cxl/core/pmem.c
> @@ -68,9 +68,6 @@ struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_memdev *cxlmd)
>  		find_cxl_root(cxlmd->endpoint);
>  	struct device *dev;
>  
> -	if (!cxl_root)
> -		return NULL;
> -

This one is definitely needed since it is not called from
cxl_endpoint_port_probe() where it knows the linkage exists.

RE: [PATCH 2/3] cxl: Cleanup unnecessary uages of cxl_port local vars from cxl_root

[Dan Williams]

Dave Jiang wrote:
> Remove the local vars that points to the 'struct cxl_port' withint 'struct

s/withint/within/

> cxl_root' and refer to the port directly.
> 
> Suggested-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/cdat.c |   13 +++++--------
>  drivers/cxl/port.c      |    5 +----
>  2 files changed, 6 insertions(+), 12 deletions(-)

This one looks ok.

RE: [PATCH 3/3] cxl: Make calling of find_cxl_root() declaration uniform

[Dan Williams]

Dave Jiang wrote:
> Move the calling find_cxl_port() and the variable declaration into the
> var declaration block and remove line breaks in the declaration. There are
> no ordering issues that would require the declaration to be in middle of
> the code.
> 
> Suggested-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/cdat.c |    7 ++-----
>  drivers/cxl/core/pmem.c |    3 +--
>  drivers/cxl/port.c      |   11 +++++------
>  3 files changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index 140935511bab..1d830d088389 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -162,6 +162,7 @@ static int cxl_cdat_endpoint_process(struct cxl_port *port,
>  static int cxl_port_perf_data_calculate(struct cxl_port *port,
>  					struct xarray *dsmas_xa)
>  {
> +	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
>  	struct access_coordinate c;
>  	struct dsmas_entry *dent;
>  	int valid_entries = 0;
> @@ -174,8 +175,6 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>  		return rc;
>  	}
>  
> -	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
> -

This looks ok, but given the comment on patch1 where it looks like a
layering violation for this to assume that it is being called from
cxl_port_probe() context then keeping the:

        if (!cxl_root)
                return -ENODEV;


...check means this one should stay as well.

>  	if (!cxl_root->ops || !cxl_root->ops->qos_class)
>  		return -EOPNOTSUPP;
>  
> @@ -344,15 +343,13 @@ DEFINE_FREE(dpa_perf, struct list_head *, if (!list_empty(_T)) discard_dpa_perf(
>  
>  static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
>  {
> +	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(cxlmd->endpoint);
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
>  	LIST_HEAD(__discard);
>  	struct list_head *discard __free(dpa_perf) = &__discard;
>  	int rc;
>  
> -	struct cxl_root *cxl_root __free(put_cxl_root) =
> -		find_cxl_root(cxlmd->endpoint);
> -

Similar comment as above.

>  	/* Check that the QTG IDs are all sane between end device and root decoders */
>  	cxl_qos_match(cxl_root, &mds->ram_perf_list, discard);
>  	cxl_qos_match(cxl_root, &mds->pmem_perf_list, discard);
> diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
> index ed76d37e4fd9..976212e588bc 100644
> --- a/drivers/cxl/core/pmem.c
> +++ b/drivers/cxl/core/pmem.c
> @@ -64,8 +64,7 @@ static int match_nvdimm_bridge(struct device *dev, void *data)
>  
>  struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_memdev *cxlmd)
>  {
> -	struct cxl_root *cxl_root __free(put_cxl_root) =
> -		find_cxl_root(cxlmd->endpoint);
> +	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(cxlmd->endpoint);

clang-format picks the other way, I don't like futzing with line breaks
if I don't have to.

>  	struct device *dev;
>  
>  	dev = device_find_child(&cxl_root->port.dev, NULL, match_nvdimm_bridge);
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index c054e7b13bdd..2bacf48593e6 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -91,6 +91,11 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>  
>  static int cxl_endpoint_port_probe(struct cxl_port *port)
>  {
> +	/*
> +	 * This can't fail in practice as CXL root exit unregisters all
> +	 * descendant ports and that in turn synchronizes with cxl_port_probe()
> +	 */
> +	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
>  	struct cxl_endpoint_dvsec_info info = { .port = port };
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> @@ -125,12 +130,6 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>  	if (rc)
>  		return rc;
>  
> -	/*
> -	 * This can't fail in practice as CXL root exit unregisters all
> -	 * descendant ports and that in turn synchronizes with cxl_port_probe()
> -	 */
> -	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
> -

Given the "not a slam dunk" justification for the other changes I'm
inclined to just drop this patch altogether.

Re: [PATCH 2/3] cxl: Cleanup unnecessary uages of cxl_port local vars from cxl_root

[Jonathan Cameron]

On Mon, 08 Jan 2024 18:07:14 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Remove the local vars that points to the 'struct cxl_port' withint 'struct
> cxl_root' and refer to the port directly.
> 
> Suggested-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
A comment on existing code inline.  Otherwise LGTM

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/cdat.c |   13 +++++--------
>  drivers/cxl/port.c      |    5 +----
>  2 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index f7ba7bd2e459..140935511bab 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -290,7 +290,7 @@ static int match_cxlrd_qos_class(struct device *dev, void *data)
>  	return 0;
>  }
>  
> -static void cxl_qos_match(struct cxl_port *root_port,
> +static void cxl_qos_match(struct cxl_root *cxl_root,
>  			  struct list_head *work_list,
>  			  struct list_head *discard_list)
>  {
> @@ -302,7 +302,7 @@ static void cxl_qos_match(struct cxl_port *root_port,
>  		if (dpa_perf->qos_class == CXL_QOS_CLASS_INVALID)
>  			return;
>  
> -		rc = device_for_each_child(&root_port->dev,
> +		rc = device_for_each_child(&cxl_root->port.dev,
>  					   (void *)&dpa_perf->qos_class,

Whilst we are here: Why is the (void *) needed or useful?

>  					   match_cxlrd_qos_class);
>  		if (!rc)

> 

Re: [PATCH 2/3] cxl: Cleanup unnecessary uages of cxl_port local vars from cxl_root

[Dave Jiang]

On 1/9/24 08:29, Jonathan Cameron wrote:
> On Mon, 08 Jan 2024 18:07:14 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Remove the local vars that points to the 'struct cxl_port' withint 'struct
>> cxl_root' and refer to the port directly.
>>
>> Suggested-by: Robert Richter <rrichter@amd.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> A comment on existing code inline.  Otherwise LGTM
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
>> ---
>>  drivers/cxl/core/cdat.c |   13 +++++--------
>>  drivers/cxl/port.c      |    5 +----
>>  2 files changed, 6 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>> index f7ba7bd2e459..140935511bab 100644
>> --- a/drivers/cxl/core/cdat.c
>> +++ b/drivers/cxl/core/cdat.c
>> @@ -290,7 +290,7 @@ static int match_cxlrd_qos_class(struct device *dev, void *data)
>>  	return 0;
>>  }
>>  
>> -static void cxl_qos_match(struct cxl_port *root_port,
>> +static void cxl_qos_match(struct cxl_root *cxl_root,
>>  			  struct list_head *work_list,
>>  			  struct list_head *discard_list)
>>  {
>> @@ -302,7 +302,7 @@ static void cxl_qos_match(struct cxl_port *root_port,
>>  		if (dpa_perf->qos_class == CXL_QOS_CLASS_INVALID)
>>  			return;
>>  
>> -		rc = device_for_each_child(&root_port->dev,
>> +		rc = device_for_each_child(&cxl_root->port.dev,
>>  					   (void *)&dpa_perf->qos_class,
> 
> Whilst we are here: Why is the (void *) needed or useful?

It's not. We can remove it. 
> 
>>  					   match_cxlrd_qos_class);
>>  		if (!rc)
> 
>>
> 
>