netdev - Re: [PATCH net-next v4 01/10] 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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171028071631.GA1980@nanopsycho.orion>
Date:   Sat, 28 Oct 2017 09:16:31 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Steve Lin <steven.lin1@...adcom.com>
Cc:     Linux Netdev List <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...lanox.com>,
        "David S . Miller" <davem@...emloft.net>,
        Michael Chan <michael.chan@...adcom.com>,
        John Linville <linville@...driver.com>,
        Andy Gospodarek <gospo@...adcom.com>,
        Yuval Mintz <yuvalm@...lanox.com>
Subject: Re: [PATCH net-next v4 01/10] devlink: Add permanent config
 parameter get/set operations

Fri, Oct 27, 2017 at 11:26:52PM CEST, steven.lin1@...adcom.com wrote:
>On Fri, Oct 27, 2017 at 5:04 PM, Jiri Pirko <jiri@...nulli.us> wrote:
>> Fri, Oct 27, 2017 at 10:54:05PM CEST, steven.lin1@...adcom.com wrote:
>>>Add support for permanent config parameter get/set commands. Used
>>>for persistent device configuration parameters.
>>>
>>>Signed-off-by: Steve Lin <steven.lin1@...adcom.com>
>>>Acked-by: Andy Gospodarek <gospo@...adcom.com>
>>>---
>>> include/net/devlink.h        |   7 ++
>>> include/uapi/linux/devlink.h |  25 ++++
>>> net/core/devlink.c           | 287 +++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 319 insertions(+)
>>>
>>>diff --git a/include/net/devlink.h b/include/net/devlink.h
>>>index b9654e1..c7dd912 100644
>>>--- a/include/net/devlink.h
>>>+++ b/include/net/devlink.h
>>>@@ -270,6 +270,13 @@ 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 (*perm_config_get)(struct devlink *devlink,
>>>+                             enum devlink_perm_config_param param, u8 type,
>>>+                             union devlink_perm_config_value *value);
>>>+      int (*perm_config_set)(struct devlink *devlink,
>>>+                             enum devlink_perm_config_param param, u8 type,
>>>+                             union devlink_perm_config_value *value,
>>>+                             u8 *restart_reqd);
>>
>> type should be enum and restart_reqd should be bool.
>
>Throughout devlink/netlink code, NLA_* types are not a named enum;
>they're defined like this in netlink.h:
>
>enum {
>        NLA_UNSPEC,
>        NLA_U8,
>        NLA_U16,
>        NLA_U32,
>        .... additional types follow ....
>};
>
>and are, I believe referred to as u8 or u16 in the code.  I could
>define a new enum specifically for the config get/set types, but that
>seems duplicative with the NLA_* types already defined - is that what
>you're suggesting?

Yes, and that should be aligned with union devlink_perm_config_value.


>
>I will change restart_reqd to be bool.
>
>>
>> [...]
>>
>>
>>>+static int devlink_nl_single_param_set(struct sk_buff *msg,
>>>+                                     struct devlink *devlink,
>>>+                                     u32 param, u8 type,
>>>+                                     union devlink_perm_config_value *value)
>>>+{
>>>+      const struct devlink_ops *ops = devlink->ops;
>>>+      struct nlattr *cfgparam_attr;
>>>+      u8 need_restart;
>>>+      int err;
>>>+
>>>+      /* Now set parameter */
>>>+      err = ops->perm_config_set(devlink, param, type, value, &need_restart);
>>>+      if (err)
>>>+              return err;
>>>+
>>>+      cfgparam_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIG);
>>>+      /* Update restart reqd - if any param needs restart, should be set */
>>>+      if (need_restart) {
>>>+              err = nla_put_u8(msg,
>>>+                               DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED, 1);
>>
>> Why don't you just put this flag always? Otherwise it could be NLA_FLAG
>
>Ok, I will change to NLA_FLAG.
>
>Thanks,
>Steve