Re: [PATCH v3 1/3] cxl: Change 'struct cxl_memdev_state' *_perf_list to single 'struct cxl_dpa_perf'

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 1 Feb 2024 14:47:29 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> In order to address the issue with being able to expose qos_class sysfs
> attributes under 'ram' and 'pmem' sub-directories, the attributes must
> be defined as static attributes rather than under driver->dev_groups.
> To avoid implementing locking for accessing the 'struct cxl_dpa_perf`
> lists, convert the list to a single 'struct cxl_dpa_perf' entry in
> preparation to move the attributes to statically defined.
> 
> While theoretically a partition may have multiple qos_class via CDAT, this
> has not been encountered with testing on available hardware. The code is
> simplified for now to not support the complex case until a use case is
> needed to support that.
> 
> Link: https://lore.kernel.org/linux-cxl/65b200ba228f_2d43c29468@dwillia2-mobl3.amr.corp.intel.com.notmuch/
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
A few comments inline.

Jonathan

> ---
> v3:
> - Add to commit log about simplification (Dan)
> - Remove check for dev->driver (Dan)
> - Remove check for invalid qos_class (Dan)
> ---
>  drivers/cxl/core/cdat.c | 81 ++++++++++++-----------------------------
>  drivers/cxl/core/mbox.c |  4 +-
>  drivers/cxl/cxlmem.h    | 10 ++---
>  drivers/cxl/mem.c       | 28 ++------------
>  4 files changed, 33 insertions(+), 90 deletions(-)
> 
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index 6fe11546889f..55b82dfd794b 100644
> --- a/drivers/cxl/core/cdat.c

>  static void cxl_qos_match(struct cxl_port *root_port,
> -			  struct list_head *work_list,
> -			  struct list_head *discard_list)
> +			  struct cxl_dpa_perf *dpa_perf)
>  {
> -	struct cxl_dpa_perf *dpa_perf, *n;
> +	int rc;
>  
> -	list_for_each_entry_safe(dpa_perf, n, work_list, list) {
> -		int rc;
> +	if (dpa_perf->qos_class == CXL_QOS_CLASS_INVALID)
> +		return;
>  
> -		if (dpa_perf->qos_class == CXL_QOS_CLASS_INVALID)
> -			return;
> -
> -		rc = device_for_each_child(&root_port->dev,
> -					   (void *)&dpa_perf->qos_class,
> -					   match_cxlrd_qos_class);
> -		if (!rc)
> -			list_move_tail(&dpa_perf->list, discard_list);
> -	}
> +	rc = device_for_each_child(&root_port->dev,

Over aggressive wrap.

> +				   &dpa_perf->qos_class,
> +				   match_cxlrd_qos_class);
> +	if (!rc)
> +		reset_dpa_perf(dpa_perf);

I'm not particularly keen on a function that on failure to match resets
some internal state in one of it's inputs.
Would prefer to see this return a bool then the caller decide to reset it.

>  }
>  
>  static int match_cxlrd_hb(struct device *dev, void *data)
> @@ -334,23 +312,10 @@ static int match_cxlrd_hb(struct device *dev, void *data)
>  	return 0;
>  }
>  

>  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);
> -	LIST_HEAD(__discard);
> -	struct list_head *discard __free(dpa_perf) = &__discard;
>  	struct cxl_port *root_port;
>  	int rc;
>  
> @@ -363,16 +328,16 @@ static int cxl_qos_class_verify(struct cxl_memdev *cxlmd)
>  	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(root_port, &mds->ram_perf);
> +	cxl_qos_match(root_port, &mds->pmem_perf);
>  
>  	/* Check to make sure that the device's host bridge is under a root decoder */
>  	rc = device_for_each_child(&root_port->dev,
>  				   (void *)cxlmd->endpoint->host_bridge,

Why is the explicit void * cast needed?  It's not removing const or anything like
that so usual c rules on it being fine to implicitly cast to void * should apply.


>  				   match_cxlrd_hb);
>  	if (!rc) {
> -		list_splice_tail_init(&mds->ram_perf_list, discard);
> -		list_splice_tail_init(&mds->pmem_perf_list, discard);
> +		reset_dpa_perf(&mds->ram_perf);
> +		reset_dpa_perf(&mds->pmem_perf);
>  	}
>  
>  	return rc;