netdev - Re: [PATCH 1/7] devlink: Add permanent config parameter get/set operations

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>] [day] [month] [year] [list]

Date:   Wed, 18 Oct 2017 11:46:01 -0400
From:   Steve Lin <steven.lin1@...adcom.com>
To:     Or Gerlitz <gerlitz.or@...il.com>
Cc:     Linux Netdev List <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...lanox.com>,
        David Miller <davem@...emloft.net>,
        Michael Chan <michael.chan@...adcom.com>,
        John Linville <linville@...driver.com>,
        Andy Gospodarek <gospo@...adcom.com>
Subject: Re: [PATCH 1/7] devlink: Add permanent config parameter get/set operations

On Wed, Oct 18, 2017 at 11:22 AM, Or Gerlitz <gerlitz.or@...il.com> wrote:
> On Tue, Oct 17, 2017 at 11:44 PM, Steve Lin <steven.lin1@...adcom.com>
> wrote:
>>
>> Add support for permanent config parameter get/set commands. Used
>> for parameters held in NVRAM, persistent device configuration.
>> The config_get() and config_set() operations operate as expected, but
>> note that the driver implementation of the config_set() operation can
>> indicate whether a restart is necessary for the setting to take
>> effect.  This indication of a necessary restart is passed via the
>> DEVLINK_ATTR_PERM_CFG_RESTART_REQUIRED attribute.
>>
>> First set of parameters defined are PCI SR-IOV and per-VF
>> configuration:
>>
>> DEVLINK_ATTR_PERM_CFG_SRIOV_ENABLED: Enable SR-IOV capability.
>> DEVLINK_ATTR_PERM_CFG_NUM_VF_PER_PF: Maximum number of VFs per PF, in
>> SR-IOV mode.
>> DEVLINK_ATTR_PERM_CFG_MAX_NUM_PF_MSIX_VECT: Maximum number of
>> MSI-X vectors assigned per PF.
>> DEVLINK_ATTR_PERM_CFG_MSIX_VECTORS_PER_VF: Number of MSI-X vectors
>> allocated per VF.
>>
>> Signed-off-by: Steve Lin <steven.lin1@...adcom.com>
>> Acked-by: Andy Gospodarek <gospo@...adcom.com>
>> ---
>>  include/net/devlink.h        |   4 +
>>  include/uapi/linux/devlink.h |  20 ++++
>>  net/core/devlink.c           | 266
>> +++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 290 insertions(+)
>>
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index b9654e1..952966c 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
>> @@ -270,6 +270,10 @@ struct devlink_ops {
>>         int (*eswitch_inline_mode_set)(struct devlink *devlink, u8
>> inline_mode);
>>         int (*eswitch_encap_mode_get)(struct devlink *devlink, u8
>> *p_encap_mode);
>>         int (*eswitch_encap_mode_set)(struct devlink *devlink, u8
>> encap_mode);
>> +       int (*config_get)(struct devlink *devlink, enum devlink_attr attr,
>> +                         u32 *value);
>> +       int (*config_set)(struct devlink *devlink, enum devlink_attr attr,
>> +                         u32 value, u8 *restart_reqd);
>>  };
>>
>>  static inline void *devlink_priv(struct devlink *devlink)
>> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>> index 0cbca96..34de44d 100644
>> --- a/include/uapi/linux/devlink.h
>> +++ b/include/uapi/linux/devlink.h
>> @@ -70,6 +70,9 @@ enum devlink_command {
>>         DEVLINK_CMD_DPIPE_HEADERS_GET,
>>         DEVLINK_CMD_DPIPE_TABLE_COUNTERS_SET,
>>
>> +       DEVLINK_CMD_CONFIG_GET,
>> +       DEVLINK_CMD_CONFIG_SET,
>> +
>>         /* add new commands above here */
>>         __DEVLINK_CMD_MAX,
>>         DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
>> @@ -202,6 +205,23 @@ enum devlink_attr {
>>
>>         DEVLINK_ATTR_ESWITCH_ENCAP_MODE,        /* u8 */
>>
>> +       /* Permanent Configuration Parameters */
>> +       DEVLINK_ATTR_PERM_CFG,                          /* nested */
>> +
>> +       /* When config doesn't take effect until next reboot (config
>> +        * just changed NVM which isn't read until boot, for example),
>> +        * this attribute should be set by the driver.
>> +        */
>> +       DEVLINK_ATTR_PERM_CFG_RESTART_REQUIRED,         /* u8 */
>> +       DEVLINK_ATTR_PERM_CFG_SRIOV_ENABLED,            /* u8 */
>> +       DEVLINK_ATTR_PERM_CFG_FIRST = DEVLINK_ATTR_PERM_CFG_SRIOV_ENABLED,
>
>
> ??? can you explain /   document this one?

I attempted to explain in the patch commit message, but I will try to
add more detail when I resubmit, with each attribute in a separate
patch.

>
> I would join Jiri's request to have patch #1 not defining any attributes,
> review will be much
> easier and robust.

Ok, I'll do that when I resubmit.

>
> Talking on attributes... what is your plan for vendor specific attributes?
>

They will be added just like common attributes, any given driver
doesn't have to support all attributes.

Steve