> On Thu, Oct 19, 2017 at 4:21 PM, Yuval Mintz <yuv...@mellanox.com> > wrote: > >> Subject: [PATCH net-next v2 1/6] devlink: Add permanent config > parameter > >> get/set operations > >> > >> Add support for permanent config parameter get/set commands. Used > >> for parameters held in NVRAM, persistent device configuration. > > > > Given some of the attributes aren't Boolean, what about an API that > > allows the user to learn of supported values per option? > > Otherwise only way for configuring some of them would be trial & error. > > Interesting suggestion. There's a couple of places where this could > be a factor. (1) When a user wants to know what values are > defined/available in the API, and (2) When the user wants to know what > values are supported by a specific driver/device. > > The intention for (1) is to push that into userspace. The userspace > devlink tool patches I am working on (not yet submitted) essentially > mirror the config parameters and their options, with string "keywords" > associated with each parameter and option, since it's the userspace > app that will be parsing the command line strings and converting to > API enums. So the userspace app can provide the list of > parameters/options it supports, which could be a subset of what's > available in the API. > > For (2), currently there is no mechanism other than trial/error as you > suggest (up to driver to either return an error or else make use of > the value specified by the user). We could contemplate adding such a > mechanism, but it's a little complicated as some options take a range > (i.e. # of VFs per PF for example), and others may take one of a set > of enumerated values (pre-boot link speed for example). > > To clarify, are you suggesting some mechanism to allow a driver to > report which parameters and options it supports (case (2))? Or are > you suggesting something in the kernel API to handle case (1) above?
I was thinking of (2). And I agree it would take some effort. > > > > >> > >> 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, > >> + 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..47cc584 100644 > >> --- a/include/uapi/linux/devlink.h > >> +++ b/include/uapi/linux/devlink.h > >> @@ -70,6 +70,10 @@ enum devlink_command { > >> DEVLINK_CMD_DPIPE_HEADERS_GET, > >> DEVLINK_CMD_DPIPE_TABLE_COUNTERS_SET, > >> > >> + /* Permanent (NVRAM) device config get/set */ > >> + DEVLINK_CMD_PERM_CONFIG_GET, > >> + DEVLINK_CMD_PERM_CONFIG_SET, > >> + > >> /* add new commands above here */ > >> __DEVLINK_CMD_MAX, > >> DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1 > >> @@ -202,6 +206,13 @@ enum devlink_attr { > >> > >> DEVLINK_ATTR_ESWITCH_ENCAP_MODE, /* u8 */ > >> > >> + /* Permanent Configuration Parameters */ > >> + DEVLINK_ATTR_PERM_CONFIGS, /* nested */ > >> + DEVLINK_ATTR_PERM_CONFIG, /* nested */ > >> + DEVLINK_ATTR_PERM_CONFIG_PARAMETER, /* u32 */ > >> + DEVLINK_ATTR_PERM_CONFIG_VALUE, /* > >> u32 */ > >> + DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED, /* u8 */ > >> + > >> /* add new attributes above here, update the policy in devlink.c */ > >> > >> __DEVLINK_ATTR_MAX, > >> diff --git a/net/core/devlink.c b/net/core/devlink.c > >> index 7d430c1..c2cc7c6 100644 > >> --- a/net/core/devlink.c > >> +++ b/net/core/devlink.c > >> @@ -1566,6 +1566,224 @@ static int > >> devlink_nl_cmd_eswitch_set_doit(struct sk_buff *skb, > >> return 0; > >> } > >> > >> +static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + > 1]; > >> + > >> +static int devlink_nl_single_param_get(struct sk_buff *msg, > >> + struct devlink *devlink, > >> + uint32_t param) > >> +{ > >> + u32 value; > >> + int err; > >> + const struct devlink_ops *ops = devlink->ops; > >> + struct nlattr *param_attr; > >> + > >> + err = ops->perm_config_get(devlink, param, &value); > >> + if (err) > >> + return err; > >> + > >> + param_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIG); > >> + nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_PARAMETER, > >> param); > >> + nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE, value); > >> + nla_nest_end(msg, param_attr); > >> + > >> + return 0; > >> +} > >> + > >> +static int devlink_nl_config_get_fill(struct sk_buff *msg, > >> + struct devlink *devlink, > >> + enum devlink_command cmd, > >> + struct genl_info *info) > >> +{ > >> + void *hdr; > >> + int err; > >> + struct nlattr *attr; > >> + int param_count = 0; > >> + struct nlattr *cfgparam_attr; > >> + int rem; > >> + struct nlattr *tb[DEVLINK_ATTR_MAX + 1]; > >> + u32 param; > >> + > >> + hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq, > >> + &devlink_nl_family, 0, cmd); > >> + if (!hdr) { > >> + err = -EMSGSIZE; > >> + goto nla_msg_failure; > >> + } > >> + > >> + err = devlink_nl_put_handle(msg, devlink); > >> + if (err) > >> + goto nla_put_failure; > >> + > >> + if (!info->attrs[DEVLINK_ATTR_PERM_CONFIGS]) { > >> + /* No configuration parameters */ > >> + 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) { > > Isn't it possible that a response for a single request for multiple ATTRs > > wouldn't fit in a single message? > > > > Hmm... Given the small size and relatively small total number of these > attributes, even when additional drivers add their own, I think it's We probably have a different idea about 'small'. Didn’t your *initial* series attempt to add 35 attributes at once? > unlikely that a single request would require a multipart message, even > in the event it's requesting all of the parameters for a given device. > I guess we could support NLM_F_MULTI type messages, but it didn't seem > necessary to me for this application? Thoughts? > > >> + if (err) > >> + goto nla_nest_failure; > >> + param_count++; > > > > Looks like an unused parameter > >> + > >> + if (restart_reqd) { > > > > Doesn't seem like you're ever setting it. > > A leftover from when this was an attribute of the configs instead > > of per-config perhaps? > > Thanks for catching these two issues - you're right, they are leftover > from the v1 implementation; apologies for not catching them in v2. > Will fix in v3.