Re: [PATCH v8 01/27] cxl: add type2 device basic support

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

alejandro.lucero-palau@ wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> Differentiate CXL memory expanders (type 3) from CXL device accelerators
> (type 2) with a new function for initializing cxl_dev_state.
> 
> Create accessors to cxl_dev_state to be used by accel drivers.
> 
> Based on previous work by Dan Williams [1]
> 
> Link: [1] https://lore.kernel.org/linux-cxl/168592160379.1948938.12863272903570476312.stgit@dwillia2-xfh.jf.intel.com/
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Reviewed-by: Fan Ni <fan.ni@samsung.com>

This patch causes 
> ---
>  drivers/cxl/core/memdev.c | 51 +++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/core/pci.c    |  1 +
>  drivers/cxl/cxlpci.h      | 16 ------------
>  drivers/cxl/pci.c         | 13 +++++++---
>  include/cxl/cxl.h         | 21 ++++++++++++++++
>  include/cxl/pci.h         | 23 ++++++++++++++++++
>  6 files changed, 105 insertions(+), 20 deletions(-)
>  create mode 100644 include/cxl/cxl.h
>  create mode 100644 include/cxl/pci.h
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index ae3dfcbe8938..99f533caae1e 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -7,6 +7,7 @@
>  #include <linux/slab.h>
>  #include <linux/idr.h>
>  #include <linux/pci.h>
> +#include <cxl/cxl.h>
>  #include <cxlmem.h>
>  #include "trace.h"
>  #include "core.h"
> @@ -616,6 +617,25 @@ static void detach_memdev(struct work_struct *work)
>  
>  static struct lock_class_key cxl_memdev_key;
>  
> +struct cxl_dev_state *cxl_accel_state_create(struct device *dev)

Lets just call this cxl_dev_state_create and have cxl_memdev_state use
it internally for the truly common init functionality.

Move the cxlds->type setting to a passed in parameter as that appears to
be the only common init piece that needs to change to make this usable
by cxl_memdev_state_create().

That would also fix the missing initialization of these values the
cxl_memdev_state_create() currently handles:

        mds->cxlds.reg_map.resource = CXL_RESOURCE_NONE;
        mds->ram_perf.qos_class = CXL_QOS_CLASS_INVALID;
        mds->pmem_perf.qos_class = CXL_QOS_CLASS_INVALID;

> +{
> +	struct cxl_dev_state *cxlds;
> +
> +	cxlds = kzalloc(sizeof(*cxlds), GFP_KERNEL);
> +	if (!cxlds)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cxlds->dev = dev;
> +	cxlds->type = CXL_DEVTYPE_DEVMEM;
> +
> +	cxlds->dpa_res = DEFINE_RES_MEM_NAMED(0, 0, "dpa");
> +	cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, 0, "ram");
> +	cxlds->pmem_res = DEFINE_RES_MEM_NAMED(0, 0, "pmem");
> +
> +	return cxlds;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_accel_state_create, "CXL");

So, this is the only new function I would expect in this patch based on
the changelog...

> +
>  static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>  					   const struct file_operations *fops)
>  {
> @@ -693,6 +713,37 @@ static int cxl_memdev_open(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> +void cxl_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec)
> +{
> +	cxlds->cxl_dvsec = dvsec;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_set_dvsec, "CXL");
> +
> +void cxl_set_serial(struct cxl_dev_state *cxlds, u64 serial)
> +{
> +	cxlds->serial = serial;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_set_serial, "CXL");

What are these doing in this patch? Why are new exports needed for such
trivial functions? If they are common values to move to init time I would
just make them common argument to cxl_dev_state_create().

> +
> +int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res,

Additionally, why does this take a 'struct resource' rather than a 'struct resource *'?

> +		     enum cxl_resource type)
> +{
> +	switch (type) {
> +	case CXL_RES_DPA:
> +		cxlds->dpa_res = res;
> +		return 0;
> +	case CXL_RES_RAM:
> +		cxlds->ram_res = res;
> +		return 0;
> +	case CXL_RES_PMEM:
> +		cxlds->pmem_res = res;
> +		return 0;

This appears to misunderstand the relationship between these resources.
dpa_res is the overall device internal DPA address space resource tree.
ram_res and pmem_res are shortcuts to get to the volatile and pmem
partitions of the dpa space. I can imagine it would ever be desirable to
trust the caller to fully initialize all the values of the resource,
especially 'parent', 'sibling', and 'child' which should only be touched
under the resource lock in the common case.