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

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: <d2d567db-f823-2ee3-48ff-abc9b375d484@amd.com>
Date:   Thu, 13 Apr 2023 17:00:01 -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 02/14] pds_core: add devcmd device interfaces

On 4/13/23 1:33 AM, Leon Romanovsky wrote:
> 
> On Mon, Apr 10, 2023 at 12:05:20PM -0700, Shannon Nelson wrote:
>> 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@....com>

[...]


> 
>>>> +/*
>>>> + * 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.
> 
> You don't really need to create separate enum for that and place
> it in include/linux/pds/pds_common.h like you did.
> 
> You FW returns u8 status, which you can feed to pdsc_err_to_errno().
> In the latter function, you will declare this translation enum.
> Such core reorg will esnure that you still have meaningful FW statuses
> while keeping them in limited code namespace.

Yes, I can see moving it down into the decode function itself can work 
and keep it limited in scope.  I'll take care of that.

Thanks,
sln