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

Zhi Wang
2024-08-17T20:33:02Z

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!
>
>
>