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

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d2d0b09d-4c16-ccf6-2cc5-00f371db0c58@amd.com>
Date:   Mon, 10 Apr 2023 11:41:22 -0700
From:   Shannon Nelson <shannon.nelson@....com>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     brett.creeley@....com, davem@...emloft.net, netdev@...r.kernel.org,
        kuba@...nel.org, drivers@...sando.io, jiri@...nulli.us
Subject: Re: [PATCH v9 net-next 01/14] pds_core: initial framework for
 pds_core PF driver

On 4/9/23 4:26 AM, Leon Romanovsky wrote:
> 

Thanks for the time you put into these review notes, I appreciate it.

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

Old habits... sure, we can pull that out.

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

These are from old habits of cleaning up when done with something.  Some 
of these kinds of zeroing are useful for checks later to see if 
something is still valid, but you are right, not all of it is really 
necessary.


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

Will fix

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

yep, will fix

> 
>> +
>> +/* 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