| 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: <20230409114608.GA182481@unreal>
Date: Sun, 9 Apr 2023 14:46:08 +0300
From: Leon Romanovsky <leon@...nel.org>
To: Shannon Nelson <shannon.nelson@....com>
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 02/14] pds_core: add devcmd device interfaces
On Thu, Apr 06, 2023 at 04:41:31PM -0700, Shannon Nelson wrote:
> The devcmd interface is the basic connection to the device through the
> PCI BAR for low level identification and command services. This does
> the early device initialization and finds the identity data, and adds
> devcmd routines to be used by later driver bits.
>
> Signed-off-by: Shannon Nelson <shannon.nelson@....com>
> ---
> drivers/net/ethernet/amd/pds_core/Makefile | 4 +-
> drivers/net/ethernet/amd/pds_core/core.c | 36 ++
> drivers/net/ethernet/amd/pds_core/core.h | 52 +++
> drivers/net/ethernet/amd/pds_core/debugfs.c | 68 ++++
> drivers/net/ethernet/amd/pds_core/dev.c | 349 ++++++++++++++++++++
> drivers/net/ethernet/amd/pds_core/main.c | 33 +-
> include/linux/pds/pds_common.h | 61 ++++
> include/linux/pds/pds_intr.h | 163 +++++++++
> 8 files changed, 763 insertions(+), 3 deletions(-)
> create mode 100644 drivers/net/ethernet/amd/pds_core/core.c
> create mode 100644 drivers/net/ethernet/amd/pds_core/dev.c
> create mode 100644 include/linux/pds/pds_intr.h
<...>
> +int pdsc_setup(struct pdsc *pdsc, bool init)
> +{
> + int err = 0;
You don't need to set value as you overwrite it immediately.
> +
> + if (init)
> + err = pdsc_dev_init(pdsc);
> + else
> + err = pdsc_dev_reinit(pdsc);
> + if (err)
> + return err;
> +
> + clear_bit(PDSC_S_FW_DEAD, &pdsc->state);
> + return 0;
> +}
<...>
> +static int irqs_show(struct seq_file *seq, void *v)
> +{
> + struct pdsc *pdsc = seq->private;
> + struct pdsc_intr_info *intr_info;
> + int i;
> +
> + seq_printf(seq, "index vector name (nintrs %d)\n", pdsc->nintrs);
> +
> + if (!pdsc->intr_info)
> + return 0;
> +
> + for (i = 0; i < pdsc->nintrs; i++) {
> + intr_info = &pdsc->intr_info[i];
> + if (!intr_info->vector)
> + continue;
> +
> + seq_printf(seq, "% 3d % 3d %s\n",
> + i, intr_info->vector, intr_info->name);
> + }
> +
> + return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(irqs);
I'm curious why existing IRQ core support is not enough?
> +
> +void pdsc_debugfs_add_irqs(struct pdsc *pdsc)
> +{
> + debugfs_create_file("irqs", 0400, pdsc->dentry, pdsc, &irqs_fops);
> +}
> +
> #endif /* CONFIG_DEBUG_FS */
> diff --git a/drivers/net/ethernet/amd/pds_core/dev.c b/drivers/net/ethernet/amd/pds_core/dev.c
> new file mode 100644
> index 000000000000..52385a72246d
> --- /dev/null
> +++ b/drivers/net/ethernet/amd/pds_core/dev.c
> @@ -0,0 +1,349 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2023 Advanced Micro Devices, Inc */
> +
> +#include <linux/errno.h>
> +#include <linux/pci.h>
> +#include <linux/utsname.h>
> +
> +#include "core.h"
> +
> +int pdsc_err_to_errno(enum pds_core_status_code code)
All users of this function, call to pdsc_devcmd_status() first. Probably
they need to be combined.
> +{
> + switch (code) {
> + case PDS_RC_SUCCESS:
> + return 0;
> + case PDS_RC_EVERSION:
> + case PDS_RC_EQTYPE:
> + case PDS_RC_EQID:
> + case PDS_RC_EINVAL:
> + case PDS_RC_ENOSUPP:
> + return -EINVAL;
> + case PDS_RC_EPERM:
> + return -EPERM;
> + case PDS_RC_ENOENT:
> + return -ENOENT;
> + case PDS_RC_EAGAIN:
> + return -EAGAIN;
> + case PDS_RC_ENOMEM:
> + return -ENOMEM;
> + case PDS_RC_EFAULT:
> + return -EFAULT;
> + case PDS_RC_EBUSY:
> + return -EBUSY;
> + case PDS_RC_EEXIST:
> + return -EEXIST;
> + case PDS_RC_EVFID:
> + return -ENODEV;
> + case PDS_RC_ECLIENT:
> + return -ECHILD;
> + case PDS_RC_ENOSPC:
> + return -ENOSPC;
> + case PDS_RC_ERANGE:
> + return -ERANGE;
> + case PDS_RC_BAD_ADDR:
> + return -EFAULT;
> + case PDS_RC_EOPCODE:
> + case PDS_RC_EINTR:
> + case PDS_RC_DEV_CMD:
> + case PDS_RC_ERROR:
> + case PDS_RC_ERDMA:
> + case PDS_RC_EIO:
> + default:
> + return -EIO;
> + }
> +}
<...>
> +static u8 pdsc_devcmd_status(struct pdsc *pdsc)
> +{
> + return ioread8(&pdsc->cmd_regs->comp.status);
> +}
<...>
> +int pdsc_devcmd_init(struct pdsc *pdsc)
> +{
> + union pds_core_dev_comp comp = { 0 };
There is no need to put 0 while using {} initialization.
> + union pds_core_dev_cmd cmd = {
> + .opcode = PDS_CORE_CMD_INIT,
> + };
> +
> + return pdsc_devcmd(pdsc, &cmd, &comp, pdsc->devcmd_timeout);
> +}
<...>
> + /* Get intr_info struct array for tracking */
> + pdsc->intr_info = kcalloc(nintrs, sizeof(*pdsc->intr_info), GFP_KERNEL);
> + if (!pdsc->intr_info) {
> + err = -ENOSPC;
The general convention is to return -ENOMEM for memorly allocation failures.
> + goto err_out;
> + }
<...>
> +/*
> + * enum pds_core_status_code - Device command return codes
> + */
> +enum pds_core_status_code {
> + PDS_RC_SUCCESS = 0, /* Success */
> + PDS_RC_EVERSION = 1, /* Incorrect version for request */
> + PDS_RC_EOPCODE = 2, /* Invalid cmd opcode */
> + PDS_RC_EIO = 3, /* I/O error */
> + PDS_RC_EPERM = 4, /* Permission denied */
> + PDS_RC_EQID = 5, /* Bad qid */
> + PDS_RC_EQTYPE = 6, /* Bad qtype */
> + PDS_RC_ENOENT = 7, /* No such element */
> + PDS_RC_EINTR = 8, /* operation interrupted */
> + PDS_RC_EAGAIN = 9, /* Try again */
> + PDS_RC_ENOMEM = 10, /* Out of memory */
> + PDS_RC_EFAULT = 11, /* Bad address */
> + PDS_RC_EBUSY = 12, /* Device or resource busy */
> + PDS_RC_EEXIST = 13, /* object already exists */
> + PDS_RC_EINVAL = 14, /* Invalid argument */
> + PDS_RC_ENOSPC = 15, /* No space left or alloc failure */
> + PDS_RC_ERANGE = 16, /* Parameter out of range */
> + PDS_RC_BAD_ADDR = 17, /* Descriptor contains a bad ptr */
> + PDS_RC_DEV_CMD = 18, /* Device cmd attempted on AdminQ */
> + PDS_RC_ENOSUPP = 19, /* Operation not supported */
> + PDS_RC_ERROR = 29, /* Generic error */
> + PDS_RC_ERDMA = 30, /* Generic RDMA error */
> + PDS_RC_EVFID = 31, /* VF ID does not exist */
> + PDS_RC_BAD_FW = 32, /* FW file is invalid or corrupted */
> + PDS_RC_ECLIENT = 33, /* No such client id */
> +};
We asked from Intel to remove custom error codes and we would like to
ask it here too. Please use standard in-kernel errors.
> +
> +enum pds_core_driver_type {
> + PDS_DRIVER_LINUX = 1,
This is only relevant here, everything else is not applicable.
> + PDS_DRIVER_WIN = 2,
> + PDS_DRIVER_DPDK = 3,
> + PDS_DRIVER_FREEBSD = 4,
> + PDS_DRIVER_IPXE = 5,
> + PDS_DRIVER_ESXI = 6,
> +};
> +
Thanks
Powered by blists - more mailing lists