| 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: <508e796c-64f1-f90a-3860-827eaab2c672@amd.com>
Date: Mon, 12 Aug 2024 12:16:02 +0100
From: Alejandro Lucero Palau <alucerop@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.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
Subject: Re: [PATCH v2 01/15] cxl: add type2 device basic support
On 8/4/24 18:10, Jonathan Cameron 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>
>
>> +
>> +void cxl_accel_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec)
>> +{
>> + cxlds->cxl_dvsec = dvsec;
> Nothing to do with accel. If these make sense promote to cxl
> core and a linux/cxl/ header. Also we may want the type3 driver to
> switch to them long term. If nothing else, making that handle the
> cxl_dev_state as more opaque will show up what is still directly
> accessed and may need to be wrapped up for a future accelerator driver
> to use.
>
I will change the function name then, but not sure I follow the comment
about more opaque ...
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_dvsec, CXL);
>> +
>> +void cxl_accel_set_serial(struct cxl_dev_state *cxlds, u64 serial)
>> +{
>> + cxlds->serial= serial;
> Run checkpatch over this series before v3 with --strict and fix the
> warnings. Probably would have spotted missing space before =
>
> Sure it's a series that is kind of RFC ish at the moment but clean
> code means you don't get nitpickers like me pointing this stuff out!
>
Sure. Thanks.
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_serial, CXL);
>> +
>> +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);
> typo. Plus I'd let this return an error as we may well have more types
> in future and not handle them all.
>
OK.
>> pci_dbg(efx->pci_dev, "shutdown successful\n");
>>
>> efx_fini_devlink_and_unlock(efx);
>> @@ -1109,6 +1111,8 @@ static int efx_pci_probe(struct pci_dev *pci_dev,
>> if (rc)
>> goto fail2;
>>
>> + efx_cxl_init(efx);
>> +
> As below, have an error code. This is not something we want to fail
> and have the driver carry on.
As you have seen in another patch when CXL initialization is taken into
account, the driver can keep going if this fails.
Those pci_warn/err inside CXL core should be enough.
>> rc = efx_pci_probe_post_io(efx);
>> if (rc) {
>> /* On failure, retry once immediately.
>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
>> new file mode 100644
>> index 000000000000..4554dd7cca76
>> --- /dev/null
>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>> @@ -0,0 +1,53 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/****************************************************************************
>> + * Driver for AMD network controllers and boards
>> + * Copyright (C) 2024, Advanced Micro Devices, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published
>> + * by the Free Software Foundation, incorporated herein by reference.
>> + */
>> +
>> +
>> +#include <linux/pci.h>
>> +#include <linux/cxl_accel_mem.h>
>> +#include <linux/cxl_accel_pci.h>
>> +
>> +#include "net_driver.h"
>> +#include "efx_cxl.h"
>> +
>> +#define EFX_CTPIO_BUFFER_SIZE (1024*1024*256)
>> +
>> +void efx_cxl_init(struct efx_nic *efx)
>> +{
>> + struct pci_dev *pci_dev = efx->pci_dev;
>> + struct efx_cxl *cxl = efx->cxl;
>> + struct resource res;
>> + u16 dvsec;
>> +
>> + dvsec = pci_find_dvsec_capability(pci_dev, PCI_VENDOR_ID_CXL,
>> + CXL_DVSEC_PCIE_DEVICE);
>> +
>> + if (!dvsec)
>> + return;
>> +
>> + pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability found");
> pci_dbg();
Right.
>
>> diff --git a/include/linux/cxl_accel_pci.h b/include/linux/cxl_accel_pci.h
>> new file mode 100644
>> index 000000000000..c337ae8797e6
>> --- /dev/null
>> +++ b/include/linux/cxl_accel_pci.h
>> @@ -0,0 +1,23 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */
>> +
>> +#ifndef __CXL_ACCEL_PCI_H
>> +#define __CXL_ACCEL_PCI_H
>> +
>> +/* CXL 2.0 8.1.3: PCIe DVSEC for CXL Device */
>> +#define CXL_DVSEC_PCIE_DEVICE 0
>> +#define CXL_DVSEC_CAP_OFFSET 0xA
>> +#define CXL_DVSEC_MEM_CAPABLE BIT(2)
>> +#define CXL_DVSEC_HDM_COUNT_MASK GENMASK(5, 4)
>> +#define CXL_DVSEC_CTRL_OFFSET 0xC
>> +#define CXL_DVSEC_MEM_ENABLE BIT(2)
>> +#define CXL_DVSEC_RANGE_SIZE_HIGH(i) (0x18 + (i * 0x10))
>> +#define CXL_DVSEC_RANGE_SIZE_LOW(i) (0x1C + (i * 0x10))
>> +#define CXL_DVSEC_MEM_INFO_VALID BIT(0)
>> +#define CXL_DVSEC_MEM_ACTIVE BIT(1)
>> +#define CXL_DVSEC_MEM_SIZE_LOW_MASK GENMASK(31, 28)
>> +#define CXL_DVSEC_RANGE_BASE_HIGH(i) (0x20 + (i * 0x10))
>> +#define CXL_DVSEC_RANGE_BASE_LOW(i) (0x24 + (i * 0x10))
>> +#define CXL_DVSEC_MEM_BASE_LOW_MASK GENMASK(31, 28)
> As I think Dave suggested, pull any defs you need to linux/cxl/pci.h or whatever
> makes sense and make the exiting code look for them there.
>
> Ideally do that in a patch that does nothing else as simple
> moves are easier to review quickly than ones mixed with real changes.
I'll do.
Thanks
>
>
>> +
>> +#endif
Powered by blists - more mailing lists