Re: [PATCH v2 01/15] cxl: add type2 device basic support

[Zhi Wang <zhiw@nvidia.com>]

On Mon, 12 Aug 2024 12:34:55 +0100
Alejandro Lucero Palau <alucerop@amd.com> wrote:

> 
> On 8/9/24 09:34, Zhi Wang wrote:
> > On Mon, 15 Jul 2024 18:28:21 +0100
> > <alejandro.lucero-palau@amd.com> wrote:
> >
> >> From: Alejandro Lucero <alucerop@amd.com>
> >>
> >> Differientiate Type3, aka memory expanders, from Type2, aka device
> >> accelerators, with a new function for initializing cxl_dev_state.
> >>
> >> Create opaque struct to be used by accelerators relying on new
> >> access functions in following patches.
> >>
> >> Add SFC ethernet network driver as the client.
> >>
> >> Based on
> >> https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@dwillia2-xfh.jf.intel.com/T/#m52543f85d0e41ff7b3063fdb9caa7e845b446d0e
> >>
> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> >> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> >> ---
> >>   drivers/cxl/core/memdev.c             | 52
> >> ++++++++++++++++++++++++++ drivers/net/ethernet/sfc/Makefile     |
> >>  2 +- drivers/net/ethernet/sfc/efx.c        |  4 ++
> >>   drivers/net/ethernet/sfc/efx_cxl.c    | 53
> >> +++++++++++++++++++++++++++ drivers/net/ethernet/sfc/efx_cxl.h    |
> >> 29 +++++++++++++++ drivers/net/ethernet/sfc/net_driver.h |  4 ++
> >>   include/linux/cxl_accel_mem.h         | 22 +++++++++++
> >>   include/linux/cxl_accel_pci.h         | 23 ++++++++++++
> >>   8 files changed, 188 insertions(+), 1 deletion(-)
> >>   create mode 100644 drivers/net/ethernet/sfc/efx_cxl.c
> >>   create mode 100644 drivers/net/ethernet/sfc/efx_cxl.h
> >>   create mode 100644 include/linux/cxl_accel_mem.h
> >>   create mode 100644 include/linux/cxl_accel_pci.h
> >>
> >> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> >> index 0277726afd04..61b5d35b49e7 100644
> >> --- a/drivers/cxl/core/memdev.c
> >> +++ b/drivers/cxl/core/memdev.c
> >> @@ -8,6 +8,7 @@
> >>   #include <linux/idr.h>
> >>   #include <linux/pci.h>
> >>   #include <cxlmem.h>
> >> +#include <linux/cxl_accel_mem.h>
> > Let's keep the header inclusion in an alphabetical order. The same
> > in efx_cxl.c
> 
> 
> The headers seem to follow a reverse Christmas tree order here rather 
> than an alphabetical one.
> 
> Should I rearrange them all?
> 

Let's fix them.

> 
> >>   #include "trace.h"
> >>   #include "core.h"
> >>   
> >> @@ -615,6 +616,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)
> >> +{
> >> +	struct cxl_dev_state *cxlds;
> >> +
> >> +	cxlds = devm_kzalloc(dev, 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);
> >> +
> >>   static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state
> >> *cxlds, const struct file_operations *fops)
> >>   {
> >> @@ -692,6 +712,38 @@ static int cxl_memdev_open(struct inode
> >> *inode, struct file *file) return 0;
> >>   }
> >>
> >> +
> >> +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec)
> >> +{
> >> +	cxlds->cxl_dvsec = dvsec;
> >> +}
> >> +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_dvsec, CXL);
> >> +
> >> +void cxl_accel_set_serial(struct cxl_dev_state *cxlds, u64 serial)
> >> +{
> >> +	cxlds->serial= serial;
> >> +}
> >> +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_serial, CXL);
> >> +
> > It would be nice to explain about how the cxl core is using these in
> > the patch comments, as we just saw the stuff got promoted into the
> > core.
> 
> 
> As far as I can see, it is for info/debugging purposes. I will add
> such explanation in next version.
> 
> 
> >
> >> +void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct
> >> resource res,
> >> +			    enum accel_resource type)
> >> +{
> >> +	switch (type) {
> >> +	case CXL_ACCEL_RES_DPA:
> >> +		cxlds->dpa_res = res;
> >> +		return;
> >> +	case CXL_ACCEL_RES_RAM:
> >> +		cxlds->ram_res = res;
> >> +		return;
> >> +	case CXL_ACCEL_RES_PMEM:
> >> +		cxlds->pmem_res = res;
> >> +		return;
> >> +	default:
> >> +		dev_err(cxlds->dev, "unkown resource type (%u)\n",
> >> type);
> >> +	}
> >> +}
> >> +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_resource, CXL);
> >> +
> > I wonder in which situation this error can be triggered.
> > One can be a newer out-of-tree type-2 driver tries to work on an
> > older kernel. Other situations should be the coding problem of an
> > in-tree driver.
> 
> 
> I guess that would point to an extension not updating this function.
> 
> 
> > I prefer to WARN_ONCE() here.
> 
> 
> I agree after your previous concern.
> 
> 
> >
> >>   
> >> diff --git a/include/linux/cxl_accel_mem.h
> >> b/include/linux/cxl_accel_mem.h new file mode 100644
> >> index 000000000000..daf46d41f59c
> >> --- /dev/null
> >> +++ b/include/linux/cxl_accel_mem.h
> >> @@ -0,0 +1,22 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */
> >> +
> >> +#include <linux/cdev.h>
> >> +
> >> +#ifndef __CXL_ACCEL_MEM_H
> >> +#define __CXL_ACCEL_MEM_H
> >> +
> >> +enum accel_resource{
> >> +	CXL_ACCEL_RES_DPA,
> >> +	CXL_ACCEL_RES_RAM,
> >> +	CXL_ACCEL_RES_PMEM,
> >> +};
> >> +
> >> +typedef struct cxl_dev_state cxl_accel_state;
> > The case of using typedef in kernel coding is very rare (quite many
> > of them are still there due to history reason, you can also spot
> > that there is only one typedef in driver/cxl). Be sure to double
> > check the coding style bible [1] when deciding to use one. :)
> >
> > [1] https://www.kernel.org/doc/html/v4.14/process/coding-style.html
> 
> 
> Right.
> 
> I think there is an agreement now in not using typedef but struct 
> cxl_dev_state so problem solved.
> 
> 
> Thanks!
> 
> 
>