Fri, Oct 27, 2017 at 11:26:52PM CEST, steven.l...@broadcom.com wrote:
>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?
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
