diff mbox series

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

Message ID 170449246044.3779673.13035770941393418591.stgit@djiang5-mobl3
State Accepted
Commit 44cd71ef7bacdfcf7c3e8a7b13f11a7eba69533d
Headers show
Series cxl: find_cxl_root() related cleanups | expand

Commit Message

Dave Jiang Jan. 5, 2024, 10:07 p.m. UTC
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(-)

Comments

Robert Richter Jan. 5, 2024, 10:51 p.m. UTC | #1
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
> 
>
Dave Jiang Jan. 5, 2024, 11:08 p.m. UTC | #2
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
>>
>>
Dave Jiang Jan. 5, 2024, 11:34 p.m. UTC | #3
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
>>
>>
Dan Williams Jan. 8, 2024, 8:43 p.m. UTC | #4
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.
Robert Richter Jan. 8, 2024, 10:30 p.m. UTC | #5
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
diff mbox series

Patch

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