Fri, Oct 20, 2017 at 05:13:39PM CEST, steven.l...@broadcom.com wrote:
>On Fri, Oct 20, 2017 at 10:39 AM, Jiri Pirko <j...@resnulli.us> wrote:
>> Thu, Oct 19, 2017 at 09:17:05PM CEST, steven.l...@broadcom.com wrote:
>>>Add support for permanent config parameter get/set commands. Used
>>>for parameters held in NVRAM, persistent device configuration.
>>>
>>>Signed-off-by: Steve Lin <steven.l...@broadcom.com>
>>>Acked-by: Andy Gospodarek <go...@broadcom.com>
>>>---
>>> include/net/devlink.h | 3 +
>>> include/uapi/linux/devlink.h | 11 ++
>>> net/core/devlink.c | 234
>>> +++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 248 insertions(+)
>>>
>>>diff --git a/include/net/devlink.h b/include/net/devlink.h
>>>index b9654e1..bd64623 100644
>>>--- a/include/net/devlink.h
>>>+++ b/include/net/devlink.h
>>>@@ -270,6 +270,9 @@ 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, u32 param, u32
>>>*value);
>>>+ int (*perm_config_set)(struct devlink *devlink, u32 param, u32 value,
>>
>> Please use enum instead of "u32 param". Also, what would happen if the
>> value is >u32, like string for example? I believe we need to take it into
>> the consideration for the UAPI sake.
>>
>>
>
>Using enum instead of u32 param: ok, will do in v3, thanks.
>
>Value > u32: In the RFC and v1 versions of the patch, each parameter
>was its own attribute, so could have its own type (u32, string,
>whatever). In v2, trying to move to nested parameters w/ parameter
>being an enum, as requested, it seems to mean that the parameter value
>now must be defined as a specific type, so I went with u32.
Why? I have to be missing something. In the nest all is same as outside
of the nest.
Also, please see team_nl_cmd_options_set() where something similar is
done, for multiple option types.
>
>If we need to support strings or other types > u32, then the
>perm_config_value attribute will not be a fixed type, so can't be
>policy checked. Or, I could go back to non-nested as in RFC/v1 case
>and have each parameter with its own type.
>
>> [...]
>>
>>
>>>+
>>>+static int devlink_nl_single_param_set(struct sk_buff *msg,
>>>+ struct devlink *devlink,
>>>+ u32 param, u32 value)
>>>+{
>>>+ u32 orig_value;
>>>+ u8 need_restart;
>>>+ int err;
>>>+ const struct devlink_ops *ops = devlink->ops;
>>>+ struct nlattr *cfgparam_attr;
>>
>> Reverse christmas tree please (this applies to all functions)
>
>Will do in v3, thanks.
>
>>
>>
>>>+
>>>+ /* First get current value of parameter */
>>>+ err = ops->perm_config_get(devlink, param, &orig_value);
>>
>> I'm missing why this is needed.
>>
>>
>>>+ if (err)
>>>+ return err;
>>>+
>>>+ /* Now set parameter */
>>>+ err = ops->perm_config_set(devlink, param, 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);
>>>+
>>>+ /* Since set was successful, write attr back to msg with orig val */
>>>+ err = nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_PARAMETER, param);
>>>+ err = nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE, orig_value);
>>
>> Why to write it back?
>
>In a response to the "RFC" version of this patch, you wrote: "Also,
>we need to expose to the user the original value (currently being
>used) and the new one (to be used after driver re-instatiation)".
>
>I understood that to mean that we need to return the current/original
>value of the parameter (and the user knows the new value, since they
>are setting it).
>
>If I mis-interpreted that comment, then I'm happy to remove returning
>the original value to the user; it wasn't in there originally.
>
>>
>>
>>>+
>>>+ nla_nest_end(msg, cfgparam_attr);
>>>+
>>>+ return 0;
>>>+}
>>>+
>>>+static int devlink_nl_cmd_perm_config_set_doit(struct sk_buff *skb,
>>>+ struct genl_info *info)
>>>+{
>>>+ struct devlink *devlink = info->user_ptr[0];
>>>+ struct sk_buff *msg;
>>>+ void *hdr;
>>>+ struct nlattr *attr;
>>>+ int rem;
>>>+ int err;
>>>+ u8 restart_reqd = 0;
>>>+ struct nlattr *cfgparam_attr;
>>>+ struct nlattr *tb[DEVLINK_ATTR_MAX + 1];
>>>+ u32 param;
>>>+ u32 value;
>>>+
>>>+ if (!devlink->ops || !devlink->ops->perm_config_get ||
>>>+ !devlink->ops->perm_config_set)
>>>+ return -EOPNOTSUPP;
>>>+
>>>+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>>+ if (!msg)
>>>+ return -ENOMEM;
>>>+
>>>+ hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
>>>+ &devlink_nl_family, 0, DEVLINK_CMD_PERM_CONFIG_SET);
>>>+ if (!hdr) {
>>>+ err = -EMSGSIZE;
>>>+ goto nla_msg_failure;
>>>+ }
>>>+
>>>+ err = devlink_nl_put_handle(msg, devlink);
>>>+ if (err)
>>>+ goto nla_put_failure;
>>>+
>>>+ cfgparam_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIGS);
>>>+
>>>+ nla_for_each_nested(attr, info->attrs[DEVLINK_ATTR_PERM_CONFIGS],
>>>rem) {
>>>+ err = nla_parse_nested(tb, DEVLINK_ATTR_MAX, attr,
>>>+ devlink_nl_policy, NULL);
>>>+ if (err)
>>>+ goto nla_nest_failure;
>>>+
>>>+ if (!tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER] ||
>>>+ !tb[DEVLINK_ATTR_PERM_CONFIG_VALUE])
>>>+ continue;
>>>+
>>>+ param = nla_get_u32(tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER]);
>>
>> You should check it the "param" value is withing the enum boundary.
>
>Will do in v3, I'll add a DEVLINK_PERM_CONFIG_MAX enum and check against that.
>
>>
>>
>>>+ value = nla_get_u32(tb[DEVLINK_ATTR_PERM_CONFIG_VALUE]);
>>>+ err = devlink_nl_single_param_set(msg, devlink, param,
>>>+ value);
>>>+ if (err)
>>>+ goto nla_nest_failure;
>>
>> Wouldn't it make sense to rollback to old values if any of the config
>> parameters set would fail?
>
>Hmmmm.... We could do a rollback on failure of any of the set cmds,
>but I feel like that's more useful when the user doesn't know which
>sets failed. (i.e. if the response was just a boolean ok/not-ok
>response).
>
>If the user knows which sets worked and which didn't, then maybe no
>rollback is necessary. So perhaps I change the code so that rather
>than falling out on error, it continues to try all the set cmds, but
>just returns the parameters which were successful.
>
>Is there a convention for this? I'm fine either way.
>
>>
>>
>>>+ }
>>>+
>>>+ nla_nest_end(msg, cfgparam_attr);
>>>+
>>>+ if (restart_reqd) {
>>>+ err = nla_put_u8(msg,
>>>DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED,
>>>+ restart_reqd);
>>>+ if (err)
>>>+ goto nla_put_failure;
>>>+ }
>>>+
>>>+ genlmsg_end(msg, hdr);
>>>+ return genlmsg_reply(msg, info);
>>>+
>>>+nla_nest_failure:
>>>+ nla_nest_cancel(msg, cfgparam_attr);
>>>+nla_put_failure:
>>>+ genlmsg_cancel(msg, hdr);
>>>+nla_msg_failure:
>>>+ return err;
>>>+}
>>>+
>>> int devlink_dpipe_match_put(struct sk_buff *skb,
>>> struct devlink_dpipe_match *match)
>>> {
>>>@@ -2291,6 +2509,8 @@ static const struct nla_policy
>>>devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
>>> [DEVLINK_ATTR_ESWITCH_ENCAP_MODE] = { .type = NLA_U8 },
>>> [DEVLINK_ATTR_DPIPE_TABLE_NAME] = { .type = NLA_NUL_STRING },
>>> [DEVLINK_ATTR_DPIPE_TABLE_COUNTERS_ENABLED] = { .type = NLA_U8 },
>>>+ [DEVLINK_ATTR_PERM_CONFIG_PARAMETER] = { .type = NLA_U32 },
>>>+ [DEVLINK_ATTR_PERM_CONFIG_VALUE] = { .type = NLA_U32 },
>>> };
>>>
>>> static const struct genl_ops devlink_nl_ops[] = {
>>>@@ -2451,6 +2671,20 @@ static const struct genl_ops devlink_nl_ops[] = {
>>> .flags = GENL_ADMIN_PERM,
>>> .internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
>>> },
>>>+ {
>>>+ .cmd = DEVLINK_CMD_PERM_CONFIG_GET,
>>>+ .doit = devlink_nl_cmd_perm_config_get_doit,
>>>+ .policy = devlink_nl_policy,
>>>+ .flags = GENL_ADMIN_PERM,
>>>+ .internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
>>>+ },
>>>+ {
>>>+ .cmd = DEVLINK_CMD_PERM_CONFIG_SET,
>>>+ .doit = devlink_nl_cmd_perm_config_set_doit,
>>>+ .policy = devlink_nl_policy,
>>>+ .flags = GENL_ADMIN_PERM,
>>>+ .internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
>>>+ },
>>> };
>>>
>>> static struct genl_family devlink_nl_family __ro_after_init = {
>>>--
>>>2.7.4
>>>
