subject:"RE\: \[PATCH net\-next v2 1\/6\] devlink\: Add permanent config parameter get\/set operations"

Re: [PATCH net-next v2 1/6] devlink: Add permanent config parameter get/set operations

2017-10-23 Thread Steve Lin

On Sat, Oct 21, 2017 at 10:12 AM, Yuval Mintz  wrote:
>> On Thu, Oct 19, 2017 at 4:21 PM, Yuval Mintz 
>> 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.

I don't disagree that this could be a useful addition.  But, it seems
like this could be added as a follow on patch.  I think many/most
users of this permanent device config API probably already know what
capabilities their device offers, and if they are wrong, the driver
can indicate invalid config options.  Nothing in the patchset prevents
future work to add a new devlink operation to query the driver for
supported options.  Thoughts?

>> > 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?
>

Yes, and I'd still like to get those ~35 parameters in eventually! :)
But even if there were 100 parameters, and all 100 were being get or
set at once, if you assume 20 bytes per parameter (4 bytes each for
DEVLINK_ATTR_CONFIG, DEVLINK_ATTR_CONFIG_PARAMETER,
DEVLINK_ATTR_CONFIG_VALUE, 8 for nesting), that's ~2000 bytes in the
message.  A little overhead, too, of course, but still less than half
a typical netlink message size of 4KB.

So, at some point - if a device with 250 parameters comes along, say,
all of which are set/get at once - this could be a problem, but it's
not clear if this is a realistic scenario to prepare for?

Re: [PATCH net-next v2 1/6] devlink: Add permanent config parameter get/set operations

2017-10-23 Thread Steve Lin

On Sat, Oct 21, 2017 at 5:24 AM, Jiri Pirko  wrote:
> 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  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 
Acked-by: Andy Gospodarek 
---
 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.
>
>

Okay, I can implement similar to that driver.  In the current
devlink.c/devlink.h, all attributes have specific types, and aren't
dynamic types like the "data" attribute in the team driver, so I had
thought having specific types defined for each attribute was required
in devlink.

RE: [PATCH net-next v2 1/6] devlink: Add permanent config parameter get/set operations

2017-10-21 Thread Yuval Mintz

> On Thu, Oct 19, 2017 at 4:21 PM, Yuval Mintz 
> 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 
> >> Acked-by: Andy Gospodarek 
> >> ---
> >>  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

Re: [PATCH net-next v2 1/6] devlink: Add permanent config parameter get/set operations

2017-10-21 Thread Jiri Pirko

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  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 
>>>Acked-by: Andy Gospodarek 
>>>---
>>> 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, _value);
>>
>> I'm missing why this is needed.
>>
>>
>>>+  if (err)
>>>+  return err;
>>>+
>>>+  /* Now set parameter */
>>>+  err = ops->perm_config_set(devlink, param, value, _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)
>>>+

Re: [PATCH net-next v2 1/6] devlink: Add permanent config parameter get/set operations

2017-10-20 Thread Steve Lin

On Fri, Oct 20, 2017 at 10:39 AM, Jiri Pirko  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 
>>Acked-by: Andy Gospodarek 
>>---
>> 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.

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, _value);
>
> I'm missing why this is needed.
>
>
>>+  if (err)
>>+  return err;
>>+
>>+  /* Now set parameter */
>>+  err = ops->perm_config_set(devlink, param, value, _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,
>>+_nl_family, 0, DEVLINK_CMD_PERM_CONFIG_SET);
>>+  if (!hdr) {
>>+  err = -EMSGSIZE;
>>+  goto nla_msg_failure;
>>+  }
>>+
>>+

Re: [PATCH net-next v2 1/6] devlink: Add permanent config parameter get/set operations

2017-10-20 Thread Jiri Pirko

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 
>Acked-by: Andy Gospodarek 
>---
> 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.


[...]


>+
>+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)


>+
>+  /* First get current value of parameter */
>+  err = ops->perm_config_get(devlink, param, _value);

I'm missing why this is needed.


>+  if (err)
>+  return err;
>+
>+  /* Now set parameter */
>+  err = ops->perm_config_set(devlink, param, value, _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?


>+
>+  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,
>+_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.


>+  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?


>+  }
>+
>+  nla_nest_end(msg, cfgparam_attr);
>+
>+  if (restart_reqd) {
>+  err = nla_put_u8(msg, DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED,
>+

Re: [PATCH net-next v2 1/6] devlink: Add permanent config parameter get/set operations

2017-10-20 Thread Steve Lin

On Thu, Oct 19, 2017 at 4:21 PM, Yuval Mintz  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?

>
>>
>> Signed-off-by: Steve Lin 
>> Acked-by: Andy Gospodarek 
>> ---
>>  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, );
>> + if (err)
>> + return err;
>> +
>> + param_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIG);
>> +

RE: [PATCH net-next v2 1/6] devlink: Add permanent config parameter get/set operations

2017-10-19 Thread Yuval Mintz

> 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.

> 
> Signed-off-by: Steve Lin 
> Acked-by: Andy Gospodarek 
> ---
>  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, );
> + 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,
> +   _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?

> + err = nla_parse_nested(tb, DEVLINK_ATTR_MAX, attr,
>

Re: [PATCH net-next v2 1/6] devlink: Add permanent config parameter get/set operations

Re: [PATCH net-next v2 1/6] devlink: Add permanent config parameter get/set operations

RE: [PATCH net-next v2 1/6] devlink: Add permanent config parameter get/set operations

Re: [PATCH net-next v2 1/6] devlink: Add permanent config parameter get/set operations

Re: [PATCH net-next v2 1/6] devlink: Add permanent config parameter get/set operations

Re: [PATCH net-next v2 1/6] devlink: Add permanent config parameter get/set operations

Re: [PATCH net-next v2 1/6] devlink: Add permanent config parameter get/set operations

RE: [PATCH net-next v2 1/6] devlink: Add permanent config parameter get/set operations

8 matches

Site Navigation

Mail list logo

Footer information