Message-ID: <20240804181045.000009dc@Huawei.com>
Date: Sun, 4 Aug 2024 18:10:45 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: <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>,
	Alejandro Lucero <alucerop@....com>
Subject: Re: [PATCH v2 01/15] cxl: add type2 device basic support

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>
Hi Alejandro,

Various comments inline. Mostly minor detail as I need to get my head
around the whole thing which will take a while yet!

Some will seem very fussy given the stage we are at (and fairly
long way to go), but cleaner code will generally be easier to read
so may help move the bigger stuff forwards quicker.
+ I had my review brain in gear so couldn't ignore things.

Jonathan

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

> @@ -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;

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.


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

> +}
> +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.

> +	}
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_accel_set_resource, CXL);
> +
>  static int cxl_memdev_release_file(struct inode *inode, struct file *file)
>  {
>  	struct cxl_memdev *cxlmd =

> diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
> index e9d9de8e648a..cb3f74d30852 100644
> --- a/drivers/net/ethernet/sfc/efx.c
> +++ b/drivers/net/ethernet/sfc/efx.c
> @@ -33,6 +33,7 @@
>  #include "selftest.h"
>  #include "sriov.h"
>  #include "efx_devlink.h"
> +#include "efx_cxl.h"
>  
>  #include "mcdi_port_common.h"
>  #include "mcdi_pcol.h"
> @@ -899,6 +900,7 @@ static void efx_pci_remove(struct pci_dev *pci_dev)
>  	efx_pci_remove_main(efx);
>  
>  	efx_fini_io(efx);
> +

Make sure you don't add noisy whitespace changes in v3. Slows down
review and makes a patch set look bigger than it is.

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

>  	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();  

> +
> +	cxl->cxlds = cxl_accel_state_create(&pci_dev->dev);
> +	if (IS_ERR(cxl->cxlds)) {
> +		pci_info(pci_dev, "CXL accel device state failed");
> +		return;

Return an error.  A driver calling CXL stuff that fails is going to
want to know

> +	}
> +
> +	cxl_accel_set_dvsec(cxl->cxlds, dvsec);
> +	cxl_accel_set_serial(cxl->cxlds, pci_dev->dev.id);
> +
> +	res = DEFINE_RES_MEM(0, EFX_CTPIO_BUFFER_SIZE);
> +	cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_DPA);
> +
> +	res = DEFINE_RES_MEM_NAMED(0, EFX_CTPIO_BUFFER_SIZE, "ram");
> +	cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_RAM);
> +}
> +
> +
> +MODULE_IMPORT_NS(CXL);
> diff --git a/drivers/net/ethernet/sfc/efx_cxl.h b/drivers/net/ethernet/sfc/efx_cxl.h
> new file mode 100644
> index 000000000000..76c6794c20d8
> --- /dev/null
> +++ b/drivers/net/ethernet/sfc/efx_cxl.h
> @@ -0,0 +1,29 @@
> +// 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.
> + */
> +
> +#ifndef EFX_CXL_H
> +#define EFX_CLX_H
> +
> +#include <linux/cxl_accel_mem.h>

Maybe, or maybe just some more forward defines to keep all the header
nice an separate.

> +
> +struct efx_nic;
> +
> +struct efx_cxl {
> +	cxl_accel_state *cxlds;
There are other ways to keep this opaque that let you embed the structure
into one you do know about.  Usually involve allocating a
cxl_device_state + your structure and some cxl_devstate_private()
accessors to get to the data placed after the cxlds part.

May not be worth bothering here though, particularly as the CXL-ness
of the device may not be the most important part and you may well be
doing similar tricks anyway to hid some other subsystem specific driver.

So for now this looks like a sensible approach to me.

> +	struct cxl_memdev *cxlmd;
> +	struct cxl_root_decoder *cxlrd;
> +	struct cxl_port *endpoint;
> +	struct cxl_endpoint_decoder *cxled;
> +	struct cxl_region *efx_region;
> +	void __iomem *ctpio_cxl;
> +};
> +
> +void efx_cxl_init(struct efx_nic *efx);
> +#endif

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

A forwards def would work like you do for struct efx_cxl
above. Keeps the structure opaque unless code actually needs
to know what is in it. That code can including the header
that defines it.  In many cases it will be an opaque pointer
passed to code in the CXL core.

struct cxl_dev_state;

Then use pointers to that in these functions.

> +cxl_accel_state *cxl_accel_state_create(struct device *dev);
> +
> +void cxl_accel_set_dvsec(cxl_accel_state *cxlds, u16 dvsec);
> +void cxl_accel_set_serial(cxl_accel_state *cxlds, u64 serial);
> +void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res,
> +			    enum accel_resource);
> +#endif
> 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.


> +
> +#endif

Powered by blists - more mailing lists