Re: [PATCH v9 net-next 02/14] pds_core: add devcmd device interfaces

[Shannon Nelson <shannon.nelson@amd.com>]

On 4/9/23 4:46 AM, Leon Romanovsky wrote:
> 
> 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@amd.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.

will fix

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

Yes, that one is not necessary, will remove.

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

This is also called from pdsc_adminq_post() which doesn't use 
pdsc_devcmd_status().

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

will fix.

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

will fix

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

These are part of the device interface defined by the device firmware 
and include some that aren't in the errno set.  This is why we use 
pdsc_err_to_errno() in pdsc_devcmd_wait() and pdsc_adminq_post(), so 
that we can change these status codes that we get from the device into 
standard kernel error codes.  We try to report both in error messages, 
but only return the kernel errno.

However, I see in one place in pdsc_devcmd_wait() we're using the status 
codes where we could use the errno, so I'll fix that up.


> 
>> +
>> +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,
>> +};

Yes, they are rather pointless for the Linux kernel, but it is part of 
documenting the device interface.

>> +
> 
> Thanks