On Fri, Oct 27, 2017 at 5:04 PM, Jiri Pirko <j...@resnulli.us> wrote:
> Fri, Oct 27, 2017 at 10:54:05PM CEST, steven.l...@broadcom.com wrote:
>>Add support for permanent config parameter get/set commands. Used
>>for persistent device configuration parameters.
>>
>>Signed-off-by: Steve Lin <steven.l...@broadcom.com>
>>Acked-by: Andy Gospodarek <go...@broadcom.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?
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
