| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8498f6bd-7ad0-5f24-826c-50956f4d9769@amd.com>
Date: Mon, 12 Aug 2024 12:34:55 +0100
From: Alejandro Lucero Palau <alucerop@....com>
To: Zhi Wang <zhiw@...dia.com>, alejandro.lucero-palau@....com
Cc: linux-cxl@...r.kernel.org, netdev@...r.kernel.org,
dan.j.williams@...el.com, martin.habets@...inx.com, edward.cree@....com,
davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
edumazet@...gle.com, richard.hughes@....com, targupta@...dia.com
Subject: Re: [PATCH v2 01/15] cxl: add type2 device basic support
On 8/9/24 09:34, Zhi Wang wrote:
> On Mon, 15 Jul 2024 18:28:21 +0100
> <alejandro.lucero-palau@....com> wrote:
>
>> From: Alejandro Lucero <alucerop@....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@....com>
>> Co-developed-by: Dan Williams <dan.j.williams@...el.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?
>> #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!
Powered by blists - more mailing lists