[PATCH net-next v2 0/6] Adding permanent config get/set to devlink

[PATCH net-next v2 0/6] Adding permanent config get/set to devlink

[Steve Lin]

Changes since v1, based on the excellent feedback received:

* Implemented nested parameters correctly this time, I think.
* Submitting config get/set infrastructure separately from the
  parameters themselves, and then submitting just the first four
  parameters as separate patches.  Once this approach is
  accepted, I will add additional parameters, taking into
  account comments received on them.
* Changed devlink_nl_sing_param_get/set to use _single_.
* Tried to make clear that all params using this command are
  permanent / NVRAM settings, not transient.
* Split out the reorganization of bnxt driver to separate patch,
  submitted to net-next earlier today.
* One non-change: The devices this change affects don't typically
  have a separate 'asic' pci b/d/f versus per-port b/d/f; they
  just have (typically multiple) b/d/f entities for each function
  on the device.  So, doesn't seem to me like splitting these
  parameters into port vs.  device params works here.

Adds a devlink command for getting & setting permanent
(persistent / NVRAM) device configuration parameters, and
enumerates the parameters as nested devlink attributes.

bnxt driver patch makes use of these new devlink cmds.

Steve Lin (6):
  devlink: Add permanent config parameter get/set operations
  devlink: Adding SR-IOV enablement NVRAM config param
  devlink: Adding num VFs per PF NVRAM config param
  devlink: Adding max PF MSI-X vectors NVRAM config param
  devlink: Adding num MSI-X vectors per VF NVRAM config param
  bnxt: Add devlink support for config get/set

 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 245 +++++++++++++++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  17 ++
 drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h     | 100 +++++++++
 include/net/devlink.h                             |   3 +
 include/uapi/linux/devlink.h                      |  19 ++
 net/core/devlink.c                                | 234 +++++++++++++++++++++
 6 files changed, 612 insertions(+), 6 deletions(-)

-- 
2.7.4

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

[Steve Lin]

Add support for permanent config parameter get/set commands. Used
for parameters held in NVRAM, persistent device configuration.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@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) {
+		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])
+			continue;
+
+		param = nla_get_u32(tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER]);
+		err = devlink_nl_single_param_get(msg, devlink, param);
+		if (err)
+			goto nla_nest_failure;
+		param_count++;
+	}
+
+	nla_nest_end(msg, cfgparam_attr);
+
+	genlmsg_end(msg, hdr);
+	return 0;
+
+nla_nest_failure:
+	nla_nest_cancel(msg, cfgparam_attr);
+nla_put_failure:
+	genlmsg_cancel(msg, hdr);
+nla_msg_failure:
+	return err;
+}
+
+static int devlink_nl_cmd_perm_config_get_doit(struct sk_buff *skb,
+					       struct genl_info *info)
+{
+	struct devlink *devlink = info->user_ptr[0];
+	struct sk_buff *msg;
+	int err;
+
+	if (!devlink->ops || !devlink->ops->perm_config_get)
+		return -EOPNOTSUPP;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	err = devlink_nl_config_get_fill(msg, devlink,
+					 DEVLINK_CMD_PERM_CONFIG_GET, info);
+
+	if (err) {
+		nlmsg_free(msg);
+		return err;
+	}
+
+	return genlmsg_reply(msg, info);
+}
+
+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;
+
+	/* First get current value of parameter */
+	err = ops->perm_config_get(devlink, param, &orig_value);
+	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);
+
+	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]);
+		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;
+	}
+
+	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

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

[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 <steven.lin1@broadcom.com>
> Acked-by: Andy Gospodarek <gospo@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?

> +		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])
> +			continue;
> +
> +		param =
> nla_get_u32(tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER]);
> +		err = devlink_nl_single_param_get(msg, devlink, param);
> +		if (err)
> +			goto nla_nest_failure;
> +		param_count++;

Looks like an unused parameter

> +	}
> +
> +	nla_nest_end(msg, cfgparam_attr);
> +
> +	genlmsg_end(msg, hdr);
> +	return 0;
> +
> +nla_nest_failure:
> +	nla_nest_cancel(msg, cfgparam_attr);
> +nla_put_failure:
> +	genlmsg_cancel(msg, hdr);
> +nla_msg_failure:
> +	return err;
> +}
> +
> +static int devlink_nl_cmd_perm_config_get_doit(struct sk_buff *skb,
> +					       struct genl_info *info)
> +{
> +	struct devlink *devlink = info->user_ptr[0];
> +	struct sk_buff *msg;
> +	int err;
> +
> +	if (!devlink->ops || !devlink->ops->perm_config_get)
> +		return -EOPNOTSUPP;
> +
> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	err = devlink_nl_config_get_fill(msg, devlink,
> +
> DEVLINK_CMD_PERM_CONFIG_GET, info);
> +
> +	if (err) {
> +		nlmsg_free(msg);
> +		return err;
> +	}
> +
> +	return genlmsg_reply(msg, info);
> +}
> +
> +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;
> +
> +	/* First get current value of parameter */
> +	err = ops->perm_config_get(devlink, param, &orig_value);
> +	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);
> +
> +	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]);
> +		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;
> +	}
> +
> +	nla_nest_end(msg, cfgparam_attr);
> +
> +	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?

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

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

[Steve Lin]

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

>
>>
>> Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>> Acked-by: Andy Gospodarek <gospo@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
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.

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

[Yuval Mintz]

> On Thu, Oct 19, 2017 at 4:21 PM, Yuval Mintz <yuvalm@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.lin1@broadcom.com>
> >> Acked-by: Andy Gospodarek <gospo@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.

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

[Steve Lin]

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

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

[Jiri Pirko]

Thu, Oct 19, 2017 at 09:17:05PM CEST, steven.lin1@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.lin1@broadcom.com>
>Acked-by: Andy Gospodarek <gospo@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.


[...]


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


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


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

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

[Steve Lin]

On Fri, Oct 20, 2017 at 10:39 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 19, 2017 at 09:17:05PM CEST, steven.lin1@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.lin1@broadcom.com>
>>Acked-by: Andy Gospodarek <gospo@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.

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

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

[Jiri Pirko]

Fri, Oct 20, 2017 at 05:13:39PM CEST, steven.lin1@broadcom.com wrote:
>On Fri, Oct 20, 2017 at 10:39 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Thu, Oct 19, 2017 at 09:17:05PM CEST, steven.lin1@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.lin1@broadcom.com>
>>>Acked-by: Andy Gospodarek <gospo@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
>>>

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

[Steve Lin]

On Sat, Oct 21, 2017 at 5:24 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Oct 20, 2017 at 05:13:39PM CEST, steven.lin1@broadcom.com wrote:
>>On Fri, Oct 20, 2017 at 10:39 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Thu, Oct 19, 2017 at 09:17:05PM CEST, steven.lin1@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.lin1@broadcom.com>
>>>>Acked-by: Andy Gospodarek <gospo@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.
>
>

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.

[PATCH net-next v2 2/6] devlink: Adding SR-IOV enablement NVRAM config param

[Steve Lin]

Adding DEVLINK_PERM_CONFIG_SRIOV_ENABLED permanent config
parameter.  If value is 1, SR-IOV is enabled.  If value is 0,
SR-IOV is disabled on this device.  Value is permanent (stored
in NVRAM), so becomes the new default value for this device.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 include/uapi/linux/devlink.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 47cc584..2640203 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -255,4 +255,9 @@ enum devlink_dpipe_header_id {
 	DEVLINK_DPIPE_HEADER_IPV6,
 };
 
+/* Permanent (NVRAM) config parameters */
+enum devlink_perm_config_param {
+	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
+};
+
 #endif /* _UAPI_LINUX_DEVLINK_H_ */
-- 
2.7.4

Re: [PATCH net-next v2 2/6] devlink: Adding SR-IOV enablement NVRAM config param

[Jiri Pirko]

Thu, Oct 19, 2017 at 09:17:06PM CEST, steven.lin1@broadcom.com wrote:
>Adding DEVLINK_PERM_CONFIG_SRIOV_ENABLED permanent config
>parameter.  If value is 1, SR-IOV is enabled.  If value is 0,
>SR-IOV is disabled on this device.  Value is permanent (stored
>in NVRAM), so becomes the new default value for this device.
>
>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>Acked-by: Andy Gospodarek <gospo@broadcom.com>
>---
> include/uapi/linux/devlink.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index 47cc584..2640203 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -255,4 +255,9 @@ enum devlink_dpipe_header_id {
> 	DEVLINK_DPIPE_HEADER_IPV6,
> };
> 
>+/* Permanent (NVRAM) config parameters */

We need the decription here in the header as well. Commit message alone
is no good for this.

Also, there should not be mention of "NVRAM". It is up to the device
implementation where is stores the value.


>+enum devlink_perm_config_param {
>+	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
>+};
>+
> #endif /* _UAPI_LINUX_DEVLINK_H_ */
>-- 
>2.7.4
>

Re: [PATCH net-next v2 2/6] devlink: Adding SR-IOV enablement NVRAM config param

[Steve Lin]

On Thu, Oct 19, 2017 at 3:33 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 19, 2017 at 09:17:06PM CEST, steven.lin1@broadcom.com wrote:
>>Adding DEVLINK_PERM_CONFIG_SRIOV_ENABLED permanent config
>>parameter.  If value is 1, SR-IOV is enabled.  If value is 0,
>>SR-IOV is disabled on this device.  Value is permanent (stored
>>in NVRAM), so becomes the new default value for this device.
>>
>>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>>Acked-by: Andy Gospodarek <gospo@broadcom.com>
>>---
>> include/uapi/linux/devlink.h | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>>index 47cc584..2640203 100644
>>--- a/include/uapi/linux/devlink.h
>>+++ b/include/uapi/linux/devlink.h
>>@@ -255,4 +255,9 @@ enum devlink_dpipe_header_id {
>>       DEVLINK_DPIPE_HEADER_IPV6,
>> };
>>
>>+/* Permanent (NVRAM) config parameters */
>
> We need the decription here in the header as well. Commit message alone
> is no good for this.
>
> Also, there should not be mention of "NVRAM". It is up to the device
> implementation where is stores the value.
>
>

Will do, I'll add descriptions in the header file and reword as
requested.  Thanks!

[PATCH net-next v2 3/6] devlink: Adding num VFs per PF NVRAM config param

[Steve Lin]

Adding DEVLINK_PERM_CONFIG_NUM_VF_PER_PF permanent config
parameter, which sets the number of VFs per PF in SR-IOV
mode.  Value is permanent (stored in NVRAM), so becomes the
new default value for this device.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 include/uapi/linux/devlink.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 2640203..09231e1 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -258,6 +258,7 @@ enum devlink_dpipe_header_id {
 /* Permanent (NVRAM) config parameters */
 enum devlink_perm_config_param {
 	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
+	DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
 };
 
 #endif /* _UAPI_LINUX_DEVLINK_H_ */
-- 
2.7.4

[PATCH net-next v2 4/6] devlink: Adding max PF MSI-X vectors NVRAM config param

[Steve Lin]

Adding DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT permanent config
parameter.  Sets the maximum number of PF MSI-X (Message
Signaled Interrupts) vectors.  Value is permanent (stored in
NVRAM), so becomes the new default value for this device.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 include/uapi/linux/devlink.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 09231e1..8ad6c63 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -259,6 +259,7 @@ enum devlink_dpipe_header_id {
 enum devlink_perm_config_param {
 	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
 	DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
+	DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT,
 };
 
 #endif /* _UAPI_LINUX_DEVLINK_H_ */
-- 
2.7.4

[PATCH net-next v2 5/6] devlink: Adding num MSI-X vectors per VF NVRAM config param

[Steve Lin]

Adding DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF permanent config
parameter.  Defines number of MSI-X vectors allocated per VF.
Value is permanent (stored in NVRAM), so becomes the new default
value for this device.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 include/uapi/linux/devlink.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 8ad6c63..ef163b6 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -260,6 +260,7 @@ enum devlink_perm_config_param {
 	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
 	DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
 	DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT,
+	DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF,
 };
 
 #endif /* _UAPI_LINUX_DEVLINK_H_ */
-- 
2.7.4

RE: [PATCH net-next v2 5/6] devlink: Adding num MSI-X vectors per VF NVRAM config param

[Yuval Mintz]

> Adding DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF permanent
> config
> parameter.  Defines number of MSI-X vectors allocated per VF.
> Value is permanent (stored in NVRAM), so becomes the new default
> value for this device.

Sounds like you're having this enforce the same configuration for all child VFs.

> 
> Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
> Acked-by: Andy Gospodarek <gospo@broadcom.com>
> ---
>  include/uapi/linux/devlink.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> index 8ad6c63..ef163b6 100644
> --- a/include/uapi/linux/devlink.h
> +++ b/include/uapi/linux/devlink.h
> @@ -260,6 +260,7 @@ enum devlink_perm_config_param {
>  	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
>  	DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
>  	DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT,
> +	DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF,
>  };
> 
>  #endif /* _UAPI_LINUX_DEVLINK_H_ */
> --
> 2.7.4

Re: [PATCH net-next v2 5/6] devlink: Adding num MSI-X vectors per VF NVRAM config param

[Jiri Pirko]

Thu, Oct 19, 2017 at 10:32:21PM CEST, yuvalm@mellanox.com wrote:
>> Adding DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF permanent
>> config
>> parameter.  Defines number of MSI-X vectors allocated per VF.
>> Value is permanent (stored in NVRAM), so becomes the new default
>> value for this device.
>
>Sounds like you're having this enforce the same configuration for all child VFs.

Yeah, this sounds like per-port config.


>
>> 
>> Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>> Acked-by: Andy Gospodarek <gospo@broadcom.com>
>> ---
>>  include/uapi/linux/devlink.h | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>> index 8ad6c63..ef163b6 100644
>> --- a/include/uapi/linux/devlink.h
>> +++ b/include/uapi/linux/devlink.h
>> @@ -260,6 +260,7 @@ enum devlink_perm_config_param {
>>  	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
>>  	DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
>>  	DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT,
>> +	DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF,
>>  };
>> 
>>  #endif /* _UAPI_LINUX_DEVLINK_H_ */
>> --
>> 2.7.4
>

Re: [PATCH net-next v2 5/6] devlink: Adding num MSI-X vectors per VF NVRAM config param

[Jiri Pirko]

Thu, Oct 19, 2017 at 11:39:55PM CEST, jiri@resnulli.us wrote:
>Thu, Oct 19, 2017 at 10:32:21PM CEST, yuvalm@mellanox.com wrote:
>>> Adding DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF permanent
>>> config
>>> parameter.  Defines number of MSI-X vectors allocated per VF.
>>> Value is permanent (stored in NVRAM), so becomes the new default
>>> value for this device.
>>
>>Sounds like you're having this enforce the same configuration for all child VFs.
>
>Yeah, this sounds like per-port config.

This opens old but lately silent discussion about introducing new port
types for different things. Like VF, dsa CPU port or dsa inter-chip
ports.

>
>
>>
>>> 
>>> Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>>> Acked-by: Andy Gospodarek <gospo@broadcom.com>
>>> ---
>>>  include/uapi/linux/devlink.h | 1 +
>>>  1 file changed, 1 insertion(+)
>>> 
>>> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>>> index 8ad6c63..ef163b6 100644
>>> --- a/include/uapi/linux/devlink.h
>>> +++ b/include/uapi/linux/devlink.h
>>> @@ -260,6 +260,7 @@ enum devlink_perm_config_param {
>>>  	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
>>>  	DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
>>>  	DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT,
>>> +	DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF,
>>>  };
>>> 
>>>  #endif /* _UAPI_LINUX_DEVLINK_H_ */
>>> --
>>> 2.7.4
>>

Re: [PATCH net-next v2 5/6] devlink: Adding num MSI-X vectors per VF NVRAM config param

[Florian Fainelli]

On 10/19/2017 02:43 PM, Jiri Pirko wrote:
> Thu, Oct 19, 2017 at 11:39:55PM CEST, jiri@resnulli.us wrote:
>> Thu, Oct 19, 2017 at 10:32:21PM CEST, yuvalm@mellanox.com wrote:
>>>> Adding DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF permanent
>>>> config
>>>> parameter.  Defines number of MSI-X vectors allocated per VF.
>>>> Value is permanent (stored in NVRAM), so becomes the new default
>>>> value for this device.
>>>
>>> Sounds like you're having this enforce the same configuration for all child VFs.
>>
>> Yeah, this sounds like per-port config.
> 
> This opens old but lately silent discussion about introducing new port
> types for different things. Like VF, dsa CPU port or dsa inter-chip
> ports.

FWIW, the "issue" with representing VF, DSA CPU port or DSA inter-chip
port is that you would be representing a pipe, so there is obviously a
question of whether your represent one end or both ends of that pipe,
and how do you make sure both stay in sync if you represent those.

For instance, for an inter-switch connection, I could decide to
configure VLANs 1-3 tagged on one end of the connection, and forget to
that on the other end of the connection, and that's just one example
where things can go seriously wrong.

> 
>>
>>
>>>
>>>>
>>>> Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>>>> Acked-by: Andy Gospodarek <gospo@broadcom.com>
>>>> ---
>>>>  include/uapi/linux/devlink.h | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>>>> index 8ad6c63..ef163b6 100644
>>>> --- a/include/uapi/linux/devlink.h
>>>> +++ b/include/uapi/linux/devlink.h
>>>> @@ -260,6 +260,7 @@ enum devlink_perm_config_param {
>>>>  	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
>>>>  	DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
>>>>  	DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT,
>>>> +	DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF,
>>>>  };
>>>>
>>>>  #endif /* _UAPI_LINUX_DEVLINK_H_ */
>>>> --
>>>> 2.7.4
>>>


-- 
Florian

Re: [PATCH net-next v2 5/6] devlink: Adding num MSI-X vectors per VF NVRAM config param

[Jiri Pirko]

Fri, Oct 20, 2017 at 03:01:35AM CEST, f.fainelli@gmail.com wrote:
>On 10/19/2017 02:43 PM, Jiri Pirko wrote:
>> Thu, Oct 19, 2017 at 11:39:55PM CEST, jiri@resnulli.us wrote:
>>> Thu, Oct 19, 2017 at 10:32:21PM CEST, yuvalm@mellanox.com wrote:
>>>>> Adding DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF permanent
>>>>> config
>>>>> parameter.  Defines number of MSI-X vectors allocated per VF.
>>>>> Value is permanent (stored in NVRAM), so becomes the new default
>>>>> value for this device.
>>>>
>>>> Sounds like you're having this enforce the same configuration for all child VFs.
>>>
>>> Yeah, this sounds like per-port config.
>> 
>> This opens old but lately silent discussion about introducing new port
>> types for different things. Like VF, dsa CPU port or dsa inter-chip
>> ports.
>
>FWIW, the "issue" with representing VF, DSA CPU port or DSA inter-chip
>port is that you would be representing a pipe, so there is obviously a
>question of whether your represent one end or both ends of that pipe,
>and how do you make sure both stay in sync if you represent those.
>
>For instance, for an inter-switch connection, I could decide to
>configure VLANs 1-3 tagged on one end of the connection, and forget to
>that on the other end of the connection, and that's just one example
>where things can go seriously wrong.

Certainly you have to represent both ends. So in your dsa inter-port
example, there should be onle devlink port instance for both dsa chips.


>
>> 
>>>
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>>>>> Acked-by: Andy Gospodarek <gospo@broadcom.com>
>>>>> ---
>>>>>  include/uapi/linux/devlink.h | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>>>>> index 8ad6c63..ef163b6 100644
>>>>> --- a/include/uapi/linux/devlink.h
>>>>> +++ b/include/uapi/linux/devlink.h
>>>>> @@ -260,6 +260,7 @@ enum devlink_perm_config_param {
>>>>>  	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
>>>>>  	DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
>>>>>  	DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT,
>>>>> +	DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF,
>>>>>  };
>>>>>
>>>>>  #endif /* _UAPI_LINUX_DEVLINK_H_ */
>>>>> --
>>>>> 2.7.4
>>>>
>
>
>-- 
>Florian

Re: [PATCH net-next v2 5/6] devlink: Adding num MSI-X vectors per VF NVRAM config param

[Steve Lin]

On Thu, Oct 19, 2017 at 5:39 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 19, 2017 at 10:32:21PM CEST, yuvalm@mellanox.com wrote:
>>> Adding DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF permanent
>>> config
>>> parameter.  Defines number of MSI-X vectors allocated per VF.
>>> Value is permanent (stored in NVRAM), so becomes the new default
>>> value for this device.
>>
>>Sounds like you're having this enforce the same configuration for all child VFs.
>
> Yeah, this sounds like per-port config.
>

Well, it gets a little tricky here.  I assume some cards handle this
per-port.  Other cards might handle this per PF, where PF may not
always correspond 1:1 with a port.  And some cards maybe just allow a
single value for this parameter for the entire card, covering all
ports/PFs.

To keep things simple and as general as possible, it made sense to set
all parameters on a per-PCI device level.  As I mentioned in my
cover-letter, the devices most likely to use these proposed commands
do not have a single "whole asic" PCI b/d/f with internal mechanism
for accessing ports - most expose each port (and each function on each
port) as a separate PCI b/d/f, with no separate "whole asic" PCI
b/d/f.  That's how the BCM cards work, and I think that's how the MLNX
cards work, and others that would be likely to use these cmds.

So, to summarize, you direct the command to the PCI b/d/f you want to
target.  Does this make sense?

Re: [PATCH net-next v2 5/6] devlink: Adding num MSI-X vectors per VF NVRAM config param

[Jiri Pirko]

Fri, Oct 20, 2017 at 04:03:55PM CEST, steven.lin1@broadcom.com wrote:
>On Thu, Oct 19, 2017 at 5:39 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Thu, Oct 19, 2017 at 10:32:21PM CEST, yuvalm@mellanox.com wrote:
>>>> Adding DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF permanent
>>>> config
>>>> parameter.  Defines number of MSI-X vectors allocated per VF.
>>>> Value is permanent (stored in NVRAM), so becomes the new default
>>>> value for this device.
>>>
>>>Sounds like you're having this enforce the same configuration for all child VFs.
>>
>> Yeah, this sounds like per-port config.
>>
>
>Well, it gets a little tricky here.  I assume some cards handle this
>per-port.  Other cards might handle this per PF, where PF may not
>always correspond 1:1 with a port.  And some cards maybe just allow a
>single value for this parameter for the entire card, covering all
>ports/PFs.
>
>To keep things simple and as general as possible, it made sense to set
>all parameters on a per-PCI device level.  As I mentioned in my
>cover-letter, the devices most likely to use these proposed commands
>do not have a single "whole asic" PCI b/d/f with internal mechanism
>for accessing ports - most expose each port (and each function on each
>port) as a separate PCI b/d/f, with no separate "whole asic" PCI
>b/d/f.  That's how the BCM cards work, and I think that's how the MLNX
>cards work, and others that would be likely to use these cmds.
>
>So, to summarize, you direct the command to the PCI b/d/f you want to
>target.  Does this make sense?

So you plan to have 1 devlink instance for each vf? Not sure that
does sound right to me :/

Re: [PATCH net-next v2 5/6] devlink: Adding num MSI-X vectors per VF NVRAM config param

[Steve Lin]

On Fri, Oct 20, 2017 at 10:10 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Oct 20, 2017 at 04:03:55PM CEST, steven.lin1@broadcom.com wrote:
>>On Thu, Oct 19, 2017 at 5:39 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Thu, Oct 19, 2017 at 10:32:21PM CEST, yuvalm@mellanox.com wrote:
>>>>> Adding DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF permanent
>>>>> config
>>>>> parameter.  Defines number of MSI-X vectors allocated per VF.
>>>>> Value is permanent (stored in NVRAM), so becomes the new default
>>>>> value for this device.
>>>>
>>>>Sounds like you're having this enforce the same configuration for all child VFs.
>>>
>>> Yeah, this sounds like per-port config.
>>>
>>
>>Well, it gets a little tricky here.  I assume some cards handle this
>>per-port.  Other cards might handle this per PF, where PF may not
>>always correspond 1:1 with a port.  And some cards maybe just allow a
>>single value for this parameter for the entire card, covering all
>>ports/PFs.
>>
>>To keep things simple and as general as possible, it made sense to set
>>all parameters on a per-PCI device level.  As I mentioned in my
>>cover-letter, the devices most likely to use these proposed commands
>>do not have a single "whole asic" PCI b/d/f with internal mechanism
>>for accessing ports - most expose each port (and each function on each
>>port) as a separate PCI b/d/f, with no separate "whole asic" PCI
>>b/d/f.  That's how the BCM cards work, and I think that's how the MLNX
>>cards work, and others that would be likely to use these cmds.
>>
>>So, to summarize, you direct the command to the PCI b/d/f you want to
>>target.  Does this make sense?
>
> So you plan to have 1 devlink instance for each vf? Not sure that
> does sound right to me :/
>

For the commands proposed in this patchset, AFAIK they all apply on a
per-PF or broader, i.e. per-port or whole-card, granularity, since
they affect permanent config that applies at boot-up.  So, no, the VFs
don't really come into play here.

RE: [PATCH net-next v2 5/6] devlink: Adding num MSI-X vectors per VF NVRAM config param

[Yuval Mintz]

> On Fri, Oct 20, 2017 at 10:10 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> > Fri, Oct 20, 2017 at 04:03:55PM CEST, steven.lin1@broadcom.com wrote:
> >>On Thu, Oct 19, 2017 at 5:39 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> >>> Thu, Oct 19, 2017 at 10:32:21PM CEST, yuvalm@mellanox.com wrote:
> >>>>> Adding DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF
> permanent
> >>>>> config
> >>>>> parameter.  Defines number of MSI-X vectors allocated per VF.
> >>>>> Value is permanent (stored in NVRAM), so becomes the new default
> >>>>> value for this device.
> >>>>
> >>>>Sounds like you're having this enforce the same configuration for all
> child VFs.
> >>>
> >>> Yeah, this sounds like per-port config.
> >>>
> >>
> >>Well, it gets a little tricky here.  I assume some cards handle this
> >>per-port.  Other cards might handle this per PF, where PF may not
> >>always correspond 1:1 with a port.  And some cards maybe just allow a
> >>single value for this parameter for the entire card, covering all
> >>ports/PFs.
> >>
> >>To keep things simple and as general as possible, it made sense to set
> >>all parameters on a per-PCI device level.  As I mentioned in my
> >>cover-letter, the devices most likely to use these proposed commands
> >>do not have a single "whole asic" PCI b/d/f with internal mechanism
> >>for accessing ports - most expose each port (and each function on each
> >>port) as a separate PCI b/d/f, with no separate "whole asic" PCI
> >>b/d/f.  That's how the BCM cards work, and I think that's how the MLNX
> >>cards work, and others that would be likely to use these cmds.
> >>
> >>So, to summarize, you direct the command to the PCI b/d/f you want to
> >>target.  Does this make sense?
> >
> > So you plan to have 1 devlink instance for each vf? Not sure that
> > does sound right to me :/
> >
> 
> For the commands proposed in this patchset, AFAIK they all apply on a
> per-PF or broader, i.e. per-port or whole-card, granularity, since
> they affect permanent config that applies at boot-up.  So, no, the VFs
> don't really come into play here.

Regardless of whether you're planning on having VFs as devlink instances,
the actual attribute question remains -
you're proposing an attribute that forces all VFs to have the same value.
This probably suits your PCI core limitations but other vendors might have
a different capability set, and accepting this design limitation now would
muck all future extension attempts of such attributes.

I think VF configurations should be planned in advance for supporting a
per-VF Configuration whenever it's possible - even if not required [/possible]
by the one pushing the new attribute.

Re: [PATCH net-next v2 5/6] devlink: Adding num MSI-X vectors per VF NVRAM config param

[Steve Lin]

On Sat, Oct 21, 2017 at 9:59 AM, Yuval Mintz <yuvalm@mellanox.com> wrote:
>> On Fri, Oct 20, 2017 at 10:10 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> > Fri, Oct 20, 2017 at 04:03:55PM CEST, steven.lin1@broadcom.com wrote:
>> >>On Thu, Oct 19, 2017 at 5:39 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> >>> Thu, Oct 19, 2017 at 10:32:21PM CEST, yuvalm@mellanox.com wrote:
>> >>>>> Adding DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF
>> permanent
>> >>>>> config
>> >>>>> parameter.  Defines number of MSI-X vectors allocated per VF.
>> >>>>> Value is permanent (stored in NVRAM), so becomes the new default
>> >>>>> value for this device.
>> >>>>
>> >>>>Sounds like you're having this enforce the same configuration for all
>> child VFs.
>> >>>
>> >>> Yeah, this sounds like per-port config.
>> >>>
>> >>
>> >>Well, it gets a little tricky here.  I assume some cards handle this
>> >>per-port.  Other cards might handle this per PF, where PF may not
>> >>always correspond 1:1 with a port.  And some cards maybe just allow a
>> >>single value for this parameter for the entire card, covering all
>> >>ports/PFs.
>> >>
>> >>To keep things simple and as general as possible, it made sense to set
>> >>all parameters on a per-PCI device level.  As I mentioned in my
>> >>cover-letter, the devices most likely to use these proposed commands
>> >>do not have a single "whole asic" PCI b/d/f with internal mechanism
>> >>for accessing ports - most expose each port (and each function on each
>> >>port) as a separate PCI b/d/f, with no separate "whole asic" PCI
>> >>b/d/f.  That's how the BCM cards work, and I think that's how the MLNX
>> >>cards work, and others that would be likely to use these cmds.
>> >>
>> >>So, to summarize, you direct the command to the PCI b/d/f you want to
>> >>target.  Does this make sense?
>> >
>> > So you plan to have 1 devlink instance for each vf? Not sure that
>> > does sound right to me :/
>> >
>>
>> For the commands proposed in this patchset, AFAIK they all apply on a
>> per-PF or broader, i.e. per-port or whole-card, granularity, since
>> they affect permanent config that applies at boot-up.  So, no, the VFs
>> don't really come into play here.
>
> Regardless of whether you're planning on having VFs as devlink instances,
> the actual attribute question remains -
> you're proposing an attribute that forces all VFs to have the same value.
> This probably suits your PCI core limitations but other vendors might have
> a different capability set, and accepting this design limitation now would
> muck all future extension attempts of such attributes.
>
> I think VF configurations should be planned in advance for supporting a
> per-VF Configuration whenever it's possible - even if not required [/possible]
> by the one pushing the new attribute.
>

The commands being added in this patch are for permanent (i.e. NVRAM)
config - essentially setting the new default values for various
features of the device at boot-up.  At that initialization time, no
VFs are yet instantiated.

So my perspective was, in general (not just for our specific device /
design), it doesn't seem like permanent config parameters would be set
on individual VFs.  That was what my previous comment was trying to
convey.

If that assumption is wrong, though, and there is some device that has
NVRAM config that is set per-VF, I assume the user would instantiate
the VF and then call the devlink API on the pci device corresponding
to the VF they with to affect, and I think the model proposed still
works.

Are you suggesting adding a mechanism to set NVRAM parameters on a
per-VF basis, without instantiating the VF first?  I would prefer not
adding such a mechanism unless/until there's a use case for it.

RE: [PATCH net-next v2 5/6] devlink: Adding num MSI-X vectors per VF NVRAM config param

[Yuval Mintz]

> >> On Fri, Oct 20, 2017 at 10:10 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> >> > Fri, Oct 20, 2017 at 04:03:55PM CEST, steven.lin1@broadcom.com wrote:
> >> >>On Thu, Oct 19, 2017 at 5:39 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> >> >>> Thu, Oct 19, 2017 at 10:32:21PM CEST, yuvalm@mellanox.com wrote:
> >> >>>>> Adding DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF
> >> permanent
> >> >>>>> config
> >> >>>>> parameter.  Defines number of MSI-X vectors allocated per VF.
> >> >>>>> Value is permanent (stored in NVRAM), so becomes the new
> default
> >> >>>>> value for this device.
> >> >>>>
> >> >>>>Sounds like you're having this enforce the same configuration for all
> >> child VFs.
> >> >>>
> >> >>> Yeah, this sounds like per-port config.
> >> >>>
> >> >>
> >> >>Well, it gets a little tricky here.  I assume some cards handle this
> >> >>per-port.  Other cards might handle this per PF, where PF may not
> >> >>always correspond 1:1 with a port.  And some cards maybe just allow a
> >> >>single value for this parameter for the entire card, covering all
> >> >>ports/PFs.
> >> >>
> >> >>To keep things simple and as general as possible, it made sense to set
> >> >>all parameters on a per-PCI device level.  As I mentioned in my
> >> >>cover-letter, the devices most likely to use these proposed commands
> >> >>do not have a single "whole asic" PCI b/d/f with internal mechanism
> >> >>for accessing ports - most expose each port (and each function on each
> >> >>port) as a separate PCI b/d/f, with no separate "whole asic" PCI
> >> >>b/d/f.  That's how the BCM cards work, and I think that's how the
> MLNX
> >> >>cards work, and others that would be likely to use these cmds.
> >> >>
> >> >>So, to summarize, you direct the command to the PCI b/d/f you want
> to
> >> >>target.  Does this make sense?
> >> >
> >> > So you plan to have 1 devlink instance for each vf? Not sure that
> >> > does sound right to me :/
> >> >
> >>
> >> For the commands proposed in this patchset, AFAIK they all apply on a
> >> per-PF or broader, i.e. per-port or whole-card, granularity, since
> >> they affect permanent config that applies at boot-up.  So, no, the VFs
> >> don't really come into play here.
> >
> > Regardless of whether you're planning on having VFs as devlink instances,
> > the actual attribute question remains -
> > you're proposing an attribute that forces all VFs to have the same value.
> > This probably suits your PCI core limitations but other vendors might have
> > a different capability set, and accepting this design limitation now would
> > muck all future extension attempts of such attributes.
> >
> > I think VF configurations should be planned in advance for supporting a
> > per-VF Configuration whenever it's possible - even if not required
> [/possible]
> > by the one pushing the new attribute.
> >
> 
> The commands being added in this patch are for permanent (i.e. NVRAM)
> config - essentially setting the new default values for various
> features of the device at boot-up.  At that initialization time, no
> VFs are yet instantiated.
> 
> So my perspective was, in general (not just for our specific device /
> design), it doesn't seem like permanent config parameters would be set
> on individual VFs.  That was what my previous comment was trying to
> convey.

That's an odd assumption; Why should you assume there's some device
that allows configuring persistent behavior for all VFs but think no other
would set the same on a per-VF basis?

> If that assumption is wrong, though, and there is some device that has
> NVRAM config that is set per-VF, I assume the user would instantiate
> the VF and then call the devlink API on the pci device corresponding
> to the VF they with to affect, and I think the model proposed still
> works.

What would be the purpose of re-configuring a value reflected in the
PCI device for an already instantiated VF?

> Are you suggesting adding a mechanism to set NVRAM parameters on a
> per-VF basis, without instantiating the VF first?  I would prefer not
> adding such a mechanism unless/until there's a use case for it.

The thing is that you're suggesting a new UAPI; We don't have the leisure
of pushing a partial implementation and changing it later on.

Re: [PATCH net-next v2 5/6] devlink: Adding num MSI-X vectors per VF NVRAM config param

[Steve Lin]

On Mon, Oct 23, 2017 at 10:37 AM, Yuval Mintz <yuvalm@mellanox.com> wrote:
>> >> On Fri, Oct 20, 2017 at 10:10 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> >> > Fri, Oct 20, 2017 at 04:03:55PM CEST, steven.lin1@broadcom.com wrote:
>> >> >>On Thu, Oct 19, 2017 at 5:39 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >>> Thu, Oct 19, 2017 at 10:32:21PM CEST, yuvalm@mellanox.com wrote:
>> >> >>>>> Adding DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF
>> >> permanent
>> >> >>>>> config
>> >> >>>>> parameter.  Defines number of MSI-X vectors allocated per VF.
>> >> >>>>> Value is permanent (stored in NVRAM), so becomes the new
>> default
>> >> >>>>> value for this device.
>> >> >>>>
>> >> >>>>Sounds like you're having this enforce the same configuration for all
>> >> child VFs.
>> >> >>>
>> >> >>> Yeah, this sounds like per-port config.
>> >> >>>
>> >> >>
>> >> >>Well, it gets a little tricky here.  I assume some cards handle this
>> >> >>per-port.  Other cards might handle this per PF, where PF may not
>> >> >>always correspond 1:1 with a port.  And some cards maybe just allow a
>> >> >>single value for this parameter for the entire card, covering all
>> >> >>ports/PFs.
>> >> >>
>> >> >>To keep things simple and as general as possible, it made sense to set
>> >> >>all parameters on a per-PCI device level.  As I mentioned in my
>> >> >>cover-letter, the devices most likely to use these proposed commands
>> >> >>do not have a single "whole asic" PCI b/d/f with internal mechanism
>> >> >>for accessing ports - most expose each port (and each function on each
>> >> >>port) as a separate PCI b/d/f, with no separate "whole asic" PCI
>> >> >>b/d/f.  That's how the BCM cards work, and I think that's how the
>> MLNX
>> >> >>cards work, and others that would be likely to use these cmds.
>> >> >>
>> >> >>So, to summarize, you direct the command to the PCI b/d/f you want
>> to
>> >> >>target.  Does this make sense?
>> >> >
>> >> > So you plan to have 1 devlink instance for each vf? Not sure that
>> >> > does sound right to me :/
>> >> >
>> >>
>> >> For the commands proposed in this patchset, AFAIK they all apply on a
>> >> per-PF or broader, i.e. per-port or whole-card, granularity, since
>> >> they affect permanent config that applies at boot-up.  So, no, the VFs
>> >> don't really come into play here.
>> >
>> > Regardless of whether you're planning on having VFs as devlink instances,
>> > the actual attribute question remains -
>> > you're proposing an attribute that forces all VFs to have the same value.
>> > This probably suits your PCI core limitations but other vendors might have
>> > a different capability set, and accepting this design limitation now would
>> > muck all future extension attempts of such attributes.
>> >
>> > I think VF configurations should be planned in advance for supporting a
>> > per-VF Configuration whenever it's possible - even if not required
>> [/possible]
>> > by the one pushing the new attribute.
>> >
>>
>> The commands being added in this patch are for permanent (i.e. NVRAM)
>> config - essentially setting the new default values for various
>> features of the device at boot-up.  At that initialization time, no
>> VFs are yet instantiated.
>>
>> So my perspective was, in general (not just for our specific device /
>> design), it doesn't seem like permanent config parameters would be set
>> on individual VFs.  That was what my previous comment was trying to
>> convey.
>
> That's an odd assumption; Why should you assume there's some device
> that allows configuring persistent behavior for all VFs but think no other
> would set the same on a per-VF basis?
>
>> If that assumption is wrong, though, and there is some device that has
>> NVRAM config that is set per-VF, I assume the user would instantiate
>> the VF and then call the devlink API on the pci device corresponding
>> to the VF they with to affect, and I think the model proposed still
>> works.
>
> What would be the purpose of re-configuring a value reflected in the
> PCI device for an already instantiated VF?
>
>> Are you suggesting adding a mechanism to set NVRAM parameters on a
>> per-VF basis, without instantiating the VF first?  I would prefer not
>> adding such a mechanism unless/until there's a use case for it.
>
> The thing is that you're suggesting a new UAPI; We don't have the leisure
> of pushing a partial implementation and changing it later on.

I hope we're not talking past each other because I'm not sure we're
saying the same thing.  But if you have a device which has NVRAM
config on an individual VF basis, and you want to be able to get/set
that configuration without instantiating the VF first (i.e. without a
PCI device to operate on), then one way to handle this is with a new
attribute, DEVLINK_ATTR_PERM_CONFIG_VF_INDEX, for example.

It could be sent in the nested DEVLINK_ATTR_PERM_CONFIG attribute,
along with the existing DEVLINK_ATTR_PERM_CONFIG_PARAMETER and _VALUE,
to indicate a specific VF within the PF that you are targeting.

That seems like the type of thing that could be added later, if/when
such a device needed support, without breaking the UAPI, couldn't it?

RE: [PATCH net-next v2 5/6] devlink: Adding num MSI-X vectors per VF NVRAM config param

[Yuval Mintz]

> >> >> On Fri, Oct 20, 2017 at 10:10 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> >> >> > Fri, Oct 20, 2017 at 04:03:55PM CEST, steven.lin1@broadcom.com
> wrote:
> >> >> >>On Thu, Oct 19, 2017 at 5:39 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> >> >> >>> Thu, Oct 19, 2017 at 10:32:21PM CEST, yuvalm@mellanox.com
> wrote:
> >> >> >>>>> Adding DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF
> >> >> permanent
> >> >> >>>>> config
> >> >> >>>>> parameter.  Defines number of MSI-X vectors allocated per VF.
> >> >> >>>>> Value is permanent (stored in NVRAM), so becomes the new
> >> default
> >> >> >>>>> value for this device.
> >> >> >>>>
> >> >> >>>>Sounds like you're having this enforce the same configuration for
> all
> >> >> child VFs.
> >> >> >>>
> >> >> >>> Yeah, this sounds like per-port config.
> >> >> >>>
> >> >> >>
> >> >> >>Well, it gets a little tricky here.  I assume some cards handle this
> >> >> >>per-port.  Other cards might handle this per PF, where PF may not
> >> >> >>always correspond 1:1 with a port.  And some cards maybe just
> allow a
> >> >> >>single value for this parameter for the entire card, covering all
> >> >> >>ports/PFs.
> >> >> >>
> >> >> >>To keep things simple and as general as possible, it made sense to
> set
> >> >> >>all parameters on a per-PCI device level.  As I mentioned in my
> >> >> >>cover-letter, the devices most likely to use these proposed
> commands
> >> >> >>do not have a single "whole asic" PCI b/d/f with internal mechanism
> >> >> >>for accessing ports - most expose each port (and each function on
> each
> >> >> >>port) as a separate PCI b/d/f, with no separate "whole asic" PCI
> >> >> >>b/d/f.  That's how the BCM cards work, and I think that's how the
> >> MLNX
> >> >> >>cards work, and others that would be likely to use these cmds.
> >> >> >>
> >> >> >>So, to summarize, you direct the command to the PCI b/d/f you
> want
> >> to
> >> >> >>target.  Does this make sense?
> >> >> >
> >> >> > So you plan to have 1 devlink instance for each vf? Not sure that
> >> >> > does sound right to me :/
> >> >> >
> >> >>
> >> >> For the commands proposed in this patchset, AFAIK they all apply on a
> >> >> per-PF or broader, i.e. per-port or whole-card, granularity, since
> >> >> they affect permanent config that applies at boot-up.  So, no, the VFs
> >> >> don't really come into play here.
> >> >
> >> > Regardless of whether you're planning on having VFs as devlink
> instances,
> >> > the actual attribute question remains -
> >> > you're proposing an attribute that forces all VFs to have the same value.
> >> > This probably suits your PCI core limitations but other vendors might
> have
> >> > a different capability set, and accepting this design limitation now would
> >> > muck all future extension attempts of such attributes.
> >> >
> >> > I think VF configurations should be planned in advance for supporting a
> >> > per-VF Configuration whenever it's possible - even if not required
> >> [/possible]
> >> > by the one pushing the new attribute.
> >> >
> >>
> >> The commands being added in this patch are for permanent (i.e. NVRAM)
> >> config - essentially setting the new default values for various
> >> features of the device at boot-up.  At that initialization time, no
> >> VFs are yet instantiated.
> >>
> >> So my perspective was, in general (not just for our specific device /
> >> design), it doesn't seem like permanent config parameters would be set
> >> on individual VFs.  That was what my previous comment was trying to
> >> convey.
> >
> > That's an odd assumption; Why should you assume there's some device
> > that allows configuring persistent behavior for all VFs but think no other
> > would set the same on a per-VF basis?
> >
> >> If that assumption is wrong, though, and there is some device that has
> >> NVRAM config that is set per-VF, I assume the user would instantiate
> >> the VF and then call the devlink API on the pci device corresponding
> >> to the VF they with to affect, and I think the model proposed still
> >> works.
> >
> > What would be the purpose of re-configuring a value reflected in the
> > PCI device for an already instantiated VF?
> >
> >> Are you suggesting adding a mechanism to set NVRAM parameters on a
> >> per-VF basis, without instantiating the VF first?  I would prefer not
> >> adding such a mechanism unless/until there's a use case for it.
> >
> > The thing is that you're suggesting a new UAPI; We don't have the leisure
> > of pushing a partial implementation and changing it later on.
> 
> I hope we're not talking past each other because I'm not sure we're
> saying the same thing.  But if you have a device which has NVRAM
> config on an individual VF basis, and you want to be able to get/set
> that configuration without instantiating the VF first (i.e. without a
> PCI device to operate on), then one way to handle this is with a new
> attribute, DEVLINK_ATTR_PERM_CONFIG_VF_INDEX, for example.
> 
> It could be sent in the nested DEVLINK_ATTR_PERM_CONFIG attribute,
> along with the existing DEVLINK_ATTR_PERM_CONFIG_PARAMETER and
> _VALUE,
> to indicate a specific VF within the PF that you are targeting.
> 
> That seems like the type of thing that could be added later, if/when
> such a device needed support, without breaking the UAPI, couldn't it?

Sounds feasible to me - as well as UAPI-conserving [albeit a bit un-conservative]

[PATCH net-next v2 6/6] bnxt: Add devlink support for config get/set

[Steve Lin]

Implements get and set of configuration parameters using new devlink
config get/set API.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 245 +++++++++++++++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  17 ++
 drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h     | 100 +++++++++
 3 files changed, 356 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index f3f6aa8..88a1f1d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -14,11 +14,244 @@
 #include "bnxt_vfr.h"
 #include "bnxt_devlink.h"
 
-static const struct devlink_ops bnxt_dl_ops = {
+struct bnxt_drv_cfgparam bnxt_drv_cfgparam_list[] = {
+	{DEVLINK_PERM_CONFIG_SRIOV_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 1, 401},
+	{DEVLINK_PERM_CONFIG_NUM_VF_PER_PF, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 8, 404},
+	{DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 10, 108},
+	{DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 10, 406},
+};
+
+#define BNXT_NUM_DRV_CFGPARAM ARRAY_SIZE(bnxt_drv_cfgparam_list)
+
+static int bnxt_nvm_read(struct bnxt *bp, int nvm_param, int idx,
+			 void *buf, int size)
+{
+	struct hwrm_nvm_get_variable_input req = {0};
+	void *dest_data_addr = NULL;
+	dma_addr_t dest_data_dma_addr;
+	int rc;
+	int bytesize;
+
+	bytesize = (size + 7) / 8;
+	dest_data_addr = dma_alloc_coherent(&bp->pdev->dev, bytesize,
+					    &dest_data_dma_addr, GFP_KERNEL);
+	if (!dest_data_addr) {
+		netdev_err(bp->dev, "dma_alloc_coherent failure\n");
+		return -ENOMEM;
+	}
+
+	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_NVM_GET_VARIABLE, -1, -1);
+	req.dest_data_addr = cpu_to_le64(dest_data_dma_addr);
+	req.data_len = cpu_to_le16(size);
+	req.option_num = cpu_to_le16(nvm_param);
+	req.index_0 = cpu_to_le16(idx);
+	if (idx != 0)
+		req.dimensions = cpu_to_le16(1);
+
+	rc = _hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);
+
+	memcpy(buf, dest_data_addr, bytesize);
+
+	dma_free_coherent(&bp->pdev->dev, bytesize, dest_data_addr,
+			  dest_data_dma_addr);
+
+	return rc;
+}
+
+static int bnxt_nvm_write(struct bnxt *bp, int nvm_param, int idx,
+			  const void *buf, int size)
+{
+	struct hwrm_nvm_set_variable_input req = {0};
+	void *src_data_addr = NULL;
+	dma_addr_t src_data_dma_addr;
+	int rc;
+	int bytesize;
+
+	bytesize = (size + 7) / 8;
+
+	src_data_addr = dma_alloc_coherent(&bp->pdev->dev, bytesize,
+					   &src_data_dma_addr, GFP_KERNEL);
+	if (!src_data_addr) {
+		netdev_err(bp->dev, "dma_alloc_coherent failure\n");
+		return -ENOMEM;
+	}
+
+	memcpy(src_data_addr, buf, bytesize);
+
+	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_NVM_SET_VARIABLE, -1, -1);
+	req.src_data_addr = cpu_to_le64(src_data_dma_addr);
+	req.data_len = cpu_to_le16(size);
+	req.option_num = cpu_to_le16(nvm_param);
+	req.index_0 = cpu_to_le16(idx);
+	if (idx != 0)
+		req.dimensions = cpu_to_le16(1);
+
+	rc = _hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);
+
+	dma_free_coherent(&bp->pdev->dev, bytesize, src_data_addr,
+			  src_data_dma_addr);
+
+	return 0;
+}
+
+static int bnxt_dl_perm_config_set(struct devlink *devlink,
+				   u32 param, u32 value, u8 *restart_reqd)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
+	int i;
+	int idx = 0;
+	void *data;
+	int ret = 0;
+	u32 bytesize;
+	struct bnxt_drv_cfgparam *entry;
+
+	*restart_reqd = 0;
+
+	/* Find parameter in table */
+	for (i = 0; i < BNXT_NUM_DRV_CFGPARAM; i++) {
+		if (param == bnxt_drv_cfgparam_list[i].param) {
+			entry = &bnxt_drv_cfgparam_list[i];
+			break;
+		}
+	}
+
+	/* Not found */
+	if (i == BNXT_NUM_DRV_CFGPARAM)
+		return -EINVAL;
+
+	/* Check to see if this func type can access variable */
+	if (BNXT_PF(bp) && !(entry->func & BNXT_DRV_PF))
+		return -EOPNOTSUPP;
+	if (BNXT_VF(bp) && !(entry->func & BNXT_DRV_VF))
+		return -EOPNOTSUPP;
+
+	/* If parameter is per port or function, compute index */
+	if (entry->appl == BNXT_DRV_APPL_PORT) {
+		idx = bp->pf.port_id;
+	} else if (entry->appl == BNXT_DRV_APPL_FUNCTION) {
+		if (BNXT_PF(bp))
+			idx = bp->pf.fw_fid - BNXT_FIRST_PF_FID;
+#ifdef CONFIG_BNXT_SRIOV
+		else
+			idx = bp->vf.fw_fid - BNXT_FIRST_VF_FID;
+#endif /* CONFIG_BNXT_SRIOV */
+	}
+
+	bytesize = (entry->bitlength + 7) / 8;
+	data = kmalloc(bytesize, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	if (bytesize == 1) {
+		u8 val8 = (value & 0xff);
+
+		memcpy(data, &val8, sizeof(u8));
+	} else if (bytesize == 2) {
+		u16 val16 = (value & 0xffff);
+
+		memcpy(data, &val16, sizeof(u16));
+	} else {
+		memcpy(data, &value, sizeof(u32));
+	}
+
+	ret = bnxt_nvm_write(bp, entry->nvm_param, idx, data,
+			     entry->bitlength);
+
+	/* Restart required for all nvm parameter writes */
+	*restart_reqd = 1;
+
+	kfree(data);
+
+	return ret;
+}
+
+static int bnxt_dl_perm_config_get(struct devlink *devlink, u32 param,
+				   u32 *value)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
+
+	int i;
+	int idx = 0;
+	void *data;
+	int ret = 0;
+	u32 bytesize;
+	struct bnxt_drv_cfgparam *entry;
+
+	/* Find parameter in table */
+	for (i = 0; i < BNXT_NUM_DRV_CFGPARAM; i++) {
+		if (param == bnxt_drv_cfgparam_list[i].param) {
+			entry = &bnxt_drv_cfgparam_list[i];
+			break;
+		}
+	}
+
+	/* Not found */
+	if (i == BNXT_NUM_DRV_CFGPARAM)
+		return -EINVAL;
+
+	/* Check to see if this func type can access variable */
+	if (BNXT_PF(bp) && !(entry->func & BNXT_DRV_PF))
+		return -EOPNOTSUPP;
+	if (BNXT_VF(bp) && !(entry->func & BNXT_DRV_VF))
+		return -EOPNOTSUPP;
+
+	/* If parameter is per port or function, compute index */
+	if (entry->appl == BNXT_DRV_APPL_PORT) {
+		idx = bp->pf.port_id;
+	} else if (entry->appl == BNXT_DRV_APPL_FUNCTION) {
+		if (BNXT_PF(bp))
+			idx = bp->pf.fw_fid - BNXT_FIRST_PF_FID;
+#ifdef CONFIG_BNXT_SRIOV
+		else
+			idx = bp->vf.fw_fid - BNXT_FIRST_VF_FID;
+#endif /* CONFIG_BNXT_SRIOV */
+	}
+
+	/* Allocate space, retrieve value, and copy to result */
+	bytesize = (entry->bitlength + 7) / 8;
+	data = kmalloc(bytesize, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	ret = bnxt_nvm_read(bp, entry->nvm_param, idx, data, entry->bitlength);
+
+	if (ret) {
+		kfree(data);
+		return ret;
+	}
+
+	if (bytesize == 1) {
+		u8 val;
+
+		memcpy(&val, data, sizeof(u8));
+		*value = val;
+	} else if (bytesize == 2) {
+		u16 val;
+
+		memcpy(&val, data, sizeof(u16));
+		*value = val;
+	} else {
+		u32 val;
+
+		memcpy(&val, data, sizeof(u32));
+		*value = val;
+	}
+
+	kfree(data);
+
+	return 0;
+}
+
+static struct devlink_ops bnxt_dl_ops = {
 #ifdef CONFIG_BNXT_SRIOV
 	.eswitch_mode_set = bnxt_dl_eswitch_mode_set,
 	.eswitch_mode_get = bnxt_dl_eswitch_mode_get,
 #endif /* CONFIG_BNXT_SRIOV */
+	.perm_config_get = bnxt_dl_perm_config_get,
+	.perm_config_set = bnxt_dl_perm_config_set,
 };
 
 int bnxt_dl_register(struct bnxt *bp)
@@ -26,12 +259,12 @@ int bnxt_dl_register(struct bnxt *bp)
 	struct devlink *dl;
 	int rc;
 
-	if (!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV))
-		return 0;
-
-	if (bp->hwrm_spec_code < 0x10800) {
+	if ((!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV)) ||
+	    bp->hwrm_spec_code < 0x10800) {
+		/* eswitch switchdev mode not supported */
+		bnxt_dl_ops.eswitch_mode_set = NULL;
+		bnxt_dl_ops.eswitch_mode_get = NULL;
 		netdev_warn(bp->dev, "Firmware does not support SR-IOV E-Switch SWITCHDEV mode.\n");
-		return -ENOTSUPP;
 	}
 
 	dl = devlink_alloc(&bnxt_dl_ops, sizeof(struct bnxt_dl));
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
index e92a35d..d843a81 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
@@ -10,6 +10,23 @@
 #ifndef BNXT_DEVLINK_H
 #define BNXT_DEVLINK_H
 
+#define BNXT_DRV_PF 1
+#define BNXT_DRV_VF 2
+
+enum bnxt_drv_appl {
+	BNXT_DRV_APPL_SHARED,
+	BNXT_DRV_APPL_PORT,
+	BNXT_DRV_APPL_FUNCTION
+};
+
+struct bnxt_drv_cfgparam {
+	enum devlink_perm_config_param	param;
+	u8			func; /* BNXT_DRV_PF | BNXT_DRV_VF */
+	enum bnxt_drv_appl	appl; /* applicability (shared, func, port) */
+	u32			bitlength; /* length, in bits */
+	u32			nvm_param;
+};
+
 /* Struct to hold housekeeping info needed by devlink interface */
 struct bnxt_dl {
 	struct bnxt *bp;	/* back ptr to the controlling dev */
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
index cb04cc7..8c25731 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
@@ -5676,6 +5676,106 @@ struct hwrm_nvm_install_update_cmd_err {
 	u8 unused_0[7];
 };
 
+/* hwrm_nvm_get_variable */
+/* Input (40 bytes) */
+struct hwrm_nvm_get_variable_input {
+	__le16 req_type;
+	__le16 cmpl_ring;
+	__le16 seq_id;
+	__le16 target_id;
+	__le64 resp_addr;
+	__le64 dest_data_addr;
+	__le16 data_len;
+	__le16 option_num;
+	#define NVM_GET_VARIABLE_REQ_OPTION_NUM_RSVD_0		   0x0UL
+	#define NVM_GET_VARIABLE_REQ_OPTION_NUM_RSVD_FFFF	   0xffffUL
+	__le16 dimensions;
+	__le16 index_0;
+	__le16 index_1;
+	__le16 index_2;
+	__le16 index_3;
+	u8 flags;
+	#define NVM_GET_VARIABLE_REQ_FLAGS_FACTORY_DFLT	    0x1UL
+	u8 unused_0;
+};
+
+/* Output (16 bytes) */
+struct hwrm_nvm_get_variable_output {
+	__le16 error_code;
+	__le16 req_type;
+	__le16 seq_id;
+	__le16 resp_len;
+	__le16 data_len;
+	__le16 option_num;
+	#define NVM_GET_VARIABLE_RESP_OPTION_NUM_RSVD_0	   0x0UL
+	#define NVM_GET_VARIABLE_RESP_OPTION_NUM_RSVD_FFFF	   0xffffUL
+	u8 unused_0;
+	u8 unused_1;
+	u8 unused_2;
+	u8 valid;
+};
+
+/* Command specific Error Codes (8 bytes) */
+struct hwrm_nvm_get_variable_cmd_err {
+	u8 code;
+	#define NVM_GET_VARIABLE_CMD_ERR_CODE_UNKNOWN		   0x0UL
+	#define NVM_GET_VARIABLE_CMD_ERR_CODE_VAR_NOT_EXIST       0x1UL
+	#define NVM_GET_VARIABLE_CMD_ERR_CODE_CORRUPT_VAR	   0x2UL
+	#define NVM_GET_VARIABLE_CMD_ERR_CODE_LEN_TOO_SHORT       0x3UL
+	u8 unused_0[7];
+};
+
+/* hwrm_nvm_set_variable */
+/* Input (40 bytes) */
+struct hwrm_nvm_set_variable_input {
+	__le16 req_type;
+	__le16 cmpl_ring;
+	__le16 seq_id;
+	__le16 target_id;
+	__le64 resp_addr;
+	__le64 src_data_addr;
+	__le16 data_len;
+	__le16 option_num;
+	#define NVM_SET_VARIABLE_REQ_OPTION_NUM_RSVD_0		   0x0UL
+	#define NVM_SET_VARIABLE_REQ_OPTION_NUM_RSVD_FFFF	   0xffffUL
+	__le16 dimensions;
+	__le16 index_0;
+	__le16 index_1;
+	__le16 index_2;
+	__le16 index_3;
+	u8 flags;
+	#define NVM_SET_VARIABLE_REQ_FLAGS_FORCE_FLUSH		    0x1UL
+	#define NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_MASK       0xeUL
+	#define NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_SFT	    1
+	#define NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_NONE      (0x0UL << 1)
+	#define NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_HMAC_SHA1 (0x1UL << 1)
+	#define NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_LAST \
+		NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_HMAC_SHA1
+	u8 unused_0;
+};
+
+/* Output (16 bytes) */
+struct hwrm_nvm_set_variable_output {
+	__le16 error_code;
+	__le16 req_type;
+	__le16 seq_id;
+	__le16 resp_len;
+	__le32 unused_0;
+	u8 unused_1;
+	u8 unused_2;
+	u8 unused_3;
+	u8 valid;
+};
+
+/* Command specific Error Codes (8 bytes) */
+struct hwrm_nvm_set_variable_cmd_err {
+	u8 code;
+	#define NVM_SET_VARIABLE_CMD_ERR_CODE_UNKNOWN		   0x0UL
+	#define NVM_SET_VARIABLE_CMD_ERR_CODE_VAR_NOT_EXIST       0x1UL
+	#define NVM_SET_VARIABLE_CMD_ERR_CODE_CORRUPT_VAR	   0x2UL
+	u8 unused_0[7];
+};
+
 /* hwrm_selftest_qlist */
 /* Input (16 bytes) */
 struct hwrm_selftest_qlist_input {
-- 
2.7.4

Re: [PATCH net-next v2 6/6] bnxt: Add devlink support for config get/set

[Jiri Pirko]

Thu, Oct 19, 2017 at 09:17:10PM CEST, steven.lin1@broadcom.com wrote:
>Implements get and set of configuration parameters using new devlink
>config get/set API.

Please split this patch too. One to introduce the infra, one per each
config option.

Re: [PATCH net-next v2 6/6] bnxt: Add devlink support for config get/set

[Steve Lin]

On Thu, Oct 19, 2017 at 3:35 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 19, 2017 at 09:17:10PM CEST, steven.lin1@broadcom.com wrote:
>>Implements get and set of configuration parameters using new devlink
>>config get/set API.
>
> Please split this patch too. One to introduce the infra, one per each
> config option.

Ok, will do in v3, thanks.