Re: [PATCH v9 net-next 01/14] pds_core: initial framework for pds_core PF driver

[Leon Romanovsky <leon@kernel.org>]

On Thu, Apr 06, 2023 at 04:41:30PM -0700, Shannon Nelson wrote:
> This is the initial PCI driver framework for the new pds_core device
> driver and its family of devices.  This does the very basics of
> registering for the new PF PCI device 1dd8:100c, setting up debugfs
> entries, and registering with devlink.
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
>  .../device_drivers/ethernet/amd/pds_core.rst  |  40 ++
>  .../device_drivers/ethernet/index.rst         |   1 +
>  drivers/net/ethernet/amd/pds_core/Makefile    |   8 +
>  drivers/net/ethernet/amd/pds_core/core.h      |  63 ++
>  drivers/net/ethernet/amd/pds_core/debugfs.c   |  34 ++
>  drivers/net/ethernet/amd/pds_core/main.c      | 285 +++++++++
>  include/linux/pds/pds_common.h                |  14 +
>  include/linux/pds/pds_core_if.h               | 540 ++++++++++++++++++
>  8 files changed, 985 insertions(+)
>  create mode 100644 Documentation/networking/device_drivers/ethernet/amd/pds_core.rst
>  create mode 100644 drivers/net/ethernet/amd/pds_core/Makefile
>  create mode 100644 drivers/net/ethernet/amd/pds_core/core.h
>  create mode 100644 drivers/net/ethernet/amd/pds_core/debugfs.c
>  create mode 100644 drivers/net/ethernet/amd/pds_core/main.c
>  create mode 100644 include/linux/pds/pds_common.h
>  create mode 100644 include/linux/pds/pds_core_if.h

<...>

> +#ifdef CONFIG_DEBUG_FS
> +void pdsc_debugfs_create(void);
> +void pdsc_debugfs_destroy(void);
> +void pdsc_debugfs_add_dev(struct pdsc *pdsc);
> +void pdsc_debugfs_del_dev(struct pdsc *pdsc);
> +#else
> +static inline void pdsc_debugfs_create(void) { }
> +static inline void pdsc_debugfs_destroy(void) { }
> +static inline void pdsc_debugfs_add_dev(struct pdsc *pdsc) { }
> +static inline void pdsc_debugfs_del_dev(struct pdsc *pdsc) { }
> +#endif

I don't think that you need CONFIG_DEBUG_FS guard as debugfs code is
built to handle this case, so you can call to internal debugfs_*() calls
without completed initialization of debugfs.

> +
> +#endif /* _PDSC_H_ */
> diff --git a/drivers/net/ethernet/amd/pds_core/debugfs.c b/drivers/net/ethernet/amd/pds_core/debugfs.c
> new file mode 100644
> index 000000000000..9b2385c19c41
> --- /dev/null
> +++ b/drivers/net/ethernet/amd/pds_core/debugfs.c
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2023 Advanced Micro Devices, Inc */
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +#include <linux/pci.h>
> +
> +#include "core.h"
> +
> +static struct dentry *pdsc_dir;
> +
> +void pdsc_debugfs_create(void)
> +{
> +	pdsc_dir = debugfs_create_dir(PDS_CORE_DRV_NAME, NULL);
> +}
> +
> +void pdsc_debugfs_destroy(void)
> +{
> +	debugfs_remove_recursive(pdsc_dir);
> +}
> +
> +void pdsc_debugfs_add_dev(struct pdsc *pdsc)
> +{
> +	pdsc->dentry = debugfs_create_dir(pci_name(pdsc->pdev), pdsc_dir);
> +
> +	debugfs_create_ulong("state", 0400, pdsc->dentry, &pdsc->state);
> +}
> +
> +void pdsc_debugfs_del_dev(struct pdsc *pdsc)
> +{
> +	debugfs_remove_recursive(pdsc->dentry);
> +	pdsc->dentry = NULL;
> +}
> +#endif /* CONFIG_DEBUG_FS */
> diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
> new file mode 100644
> index 000000000000..1c2f3fbaa27c
> --- /dev/null
> +++ b/drivers/net/ethernet/amd/pds_core/main.c
> @@ -0,0 +1,285 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2023 Advanced Micro Devices, Inc */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/pci.h>
> +
> +#include <linux/pds/pds_common.h>
> +
> +#include "core.h"
> +
> +MODULE_DESCRIPTION(PDSC_DRV_DESCRIPTION);
> +MODULE_AUTHOR("Advanced Micro Devices, Inc");
> +MODULE_LICENSE("GPL");
> +
> +/* Supported devices */
> +static const struct pci_device_id pdsc_id_table[] = {
> +	{ PCI_VDEVICE(PENSANDO, PCI_DEVICE_ID_PENSANDO_CORE_PF) },
> +	{ 0, }	/* end of table */
> +};
> +MODULE_DEVICE_TABLE(pci, pdsc_id_table);
> +
> +static void pdsc_unmap_bars(struct pdsc *pdsc)
> +{
> +	struct pdsc_dev_bar *bars = pdsc->bars;
> +	unsigned int i;
> +
> +	for (i = 0; i < PDS_CORE_BARS_MAX; i++) {
> +		if (bars[i].vaddr) {
> +			pci_iounmap(pdsc->pdev, bars[i].vaddr);
> +			bars[i].vaddr = NULL;
> +		}
> +
> +		bars[i].len = 0;
> +		bars[i].bus_addr = 0;
> +		bars[i].res_index = 0;

Why are you clearing it? You are going to release bars[] anyway.
It will be great to remove this zeroing pattern from whole driver
as it hides use-after-free bugs.

> +	}
> +}
> +
> +static int pdsc_map_bars(struct pdsc *pdsc)
> +{
> +	struct pdsc_dev_bar *bar = pdsc->bars;
> +	struct pci_dev *pdev = pdsc->pdev;
> +	struct device *dev = pdsc->dev;
> +	struct pdsc_dev_bar *bars;
> +	unsigned int i, j;
> +	int num_bars = 0;
> +	int err;
> +	u32 sig;
> +
> +	bars = pdsc->bars;
> +	num_bars = 0;

You set it to zero 4 lines above.

> +

<...>

> +module_init(pdsc_init_module);
> +module_exit(pdsc_cleanup_module);
> diff --git a/include/linux/pds/pds_common.h b/include/linux/pds/pds_common.h
> new file mode 100644
> index 000000000000..bd041a5170a6
> --- /dev/null
> +++ b/include/linux/pds/pds_common.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR Linux-OpenIB) OR BSD-2-Clause */
> +/* Copyright(c) 2023 Advanced Micro Devices, Inc. */
> +
> +#ifndef _PDS_COMMON_H_
> +#define _PDS_COMMON_H_
> +
> +#define PDS_CORE_DRV_NAME			"pds_core"

It is KBUILD_MODNAME.

> +
> +/* the device's internal addressing uses up to 52 bits */
> +#define PDS_CORE_ADDR_LEN	52
> +#define PDS_CORE_ADDR_MASK	(BIT_ULL(PDS_ADDR_LEN) - 1)
> +#define PDS_PAGE_SIZE		4096

Thanks