[PATCH net-next v5 00/10] Adding permanent config get/set to devlink

[PATCH net-next v5 00/10] Adding permanent config get/set to devlink

[Steve Lin]

Changes since v4:

* Created / used enum for permanent config param / types.
* Use bool and NLA_FLAG for restart_required indication.
* Move and reworded comments and git commit messagess for
  clarity and removed double-spaces after sentences.

[PATCH net-next v5 01/10] devlink: Add permanent config parameter get/set operations

[Steve Lin]

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

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 include/net/devlink.h        |   9 ++
 include/uapi/linux/devlink.h |  33 +++++
 net/core/devlink.c           | 293 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 335 insertions(+)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index b9654e1..437b6ab 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -270,6 +270,15 @@ struct devlink_ops {
 	int (*eswitch_inline_mode_set)(struct devlink *devlink, u8 inline_mode);
 	int (*eswitch_encap_mode_get)(struct devlink *devlink, u8 *p_encap_mode);
 	int (*eswitch_encap_mode_set)(struct devlink *devlink, u8 encap_mode);
+	int (*perm_config_get)(struct devlink *devlink,
+			       enum devlink_perm_config_param param,
+			       enum devlink_perm_config_type type,
+			       union devlink_perm_config_value *value);
+	int (*perm_config_set)(struct devlink *devlink,
+			       enum devlink_perm_config_param param,
+			       enum devlink_perm_config_type type,
+			       union devlink_perm_config_value *value,
+			       bool *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..55e35f2 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 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,14 @@ 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_TYPE,			/* u8 */
+	DEVLINK_ATTR_PERM_CONFIG_VALUE,			/* dynamic */
+	DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED,	/* flag */
+
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
@@ -244,4 +256,25 @@ enum devlink_dpipe_header_id {
 	DEVLINK_DPIPE_HEADER_IPV6,
 };
 
+/* Permanent config parameters */
+enum devlink_perm_config_param {
+	__DEVLINK_PERM_CONFIG_MAX,
+	DEVLINK_PERM_CONFIG_MAX = __DEVLINK_PERM_CONFIG_MAX - 1
+};
+
+/* Permanent config types */
+enum devlink_perm_config_type {
+	DEVLINK_PERM_CONFIG_TYPE_UNSPEC,
+	DEVLINK_PERM_CONFIG_TYPE_U8,
+	DEVLINK_PERM_CONFIG_TYPE_U16,
+	DEVLINK_PERM_CONFIG_TYPE_U32,
+};
+
+/* Permanent config value */
+union devlink_perm_config_value {
+	__u8	value8;
+	__u16	value16;
+	__u32	value32;
+};
+
 #endif /* _UAPI_LINUX_DEVLINK_H_ */
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 7d430c1..5e77408 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -1566,6 +1566,283 @@ 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 const u8 devlink_perm_cfg_param_types[DEVLINK_PERM_CONFIG_MAX + 1] = {
+};
+
+static int devlink_nl_single_param_get(struct sk_buff *msg,
+				       struct devlink *devlink,
+				       enum devlink_perm_config_param param,
+				       enum devlink_perm_config_type type)
+{
+	const struct devlink_ops *ops = devlink->ops;
+	union devlink_perm_config_value value;
+	struct nlattr *param_attr;
+	int err;
+
+	err = ops->perm_config_get(devlink, param, type, &value);
+	if (err)
+		return err;
+
+	param_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIG);
+	if (!param_attr)
+		return -EMSGSIZE;
+
+	if (nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_PARAMETER, param) ||
+	    nla_put_u8(msg, DEVLINK_ATTR_PERM_CONFIG_TYPE, type))
+		goto nest_err;
+
+	switch (type) {
+	case DEVLINK_PERM_CONFIG_TYPE_U8:
+		if (nla_put_u8(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE,
+			       value.value8))
+			goto nest_err;
+		break;
+	case DEVLINK_PERM_CONFIG_TYPE_U16:
+		if (nla_put_u16(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE,
+				value.value16))
+			goto nest_err;
+		break;
+	case DEVLINK_PERM_CONFIG_TYPE_U32:
+		if (nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE,
+				value.value32))
+			goto nest_err;
+		break;
+	default:
+		err = -EINVAL;
+		goto nest_err;
+		break;
+	}
+	nla_nest_end(msg, param_attr);
+
+	return err;
+
+nest_err:
+	nla_nest_cancel(msg, param_attr);
+	return -EMSGSIZE;
+}
+
+static int devlink_nl_config_get_fill(struct sk_buff *msg,
+				      struct devlink *devlink,
+				      enum devlink_command cmd,
+				      struct genl_info *info)
+{
+	struct nlattr *tb[DEVLINK_ATTR_MAX + 1];
+	enum devlink_perm_config_param param;
+	enum devlink_perm_config_type type;
+	struct nlattr *cfgparam_attr;
+	struct nlattr *attr;
+	void *hdr;
+	int err;
+	int rem;
+
+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
+			  &devlink_nl_family, 0, cmd);
+	if (!hdr)
+		return -EMSGSIZE;
+
+	err = devlink_nl_put_handle(msg, devlink);
+	if (err)
+		goto nla_put_failure;
+
+	if (!info->attrs[DEVLINK_ATTR_PERM_CONFIGS]) {
+		err = -EINVAL;
+		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_TYPE])
+			continue;
+
+		param = nla_get_u32(tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER]);
+		type = nla_get_u8(tb[DEVLINK_ATTR_PERM_CONFIG_TYPE]);
+		if (param > DEVLINK_PERM_CONFIG_MAX ||
+		    type > NLA_TYPE_MAX) {
+			continue;
+		}
+		/* Note if single_param_get fails, that param won't be in
+		 * response msg, so caller will know which param(s) failed to
+		 * get.
+		 */
+		devlink_nl_single_param_get(msg, devlink, param, type);
+	}
+
+	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);
+	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,
+				       enum devlink_perm_config_param param,
+				       enum devlink_perm_config_type type,
+				       union devlink_perm_config_value *value)
+{
+	const struct devlink_ops *ops = devlink->ops;
+	struct nlattr *cfgparam_attr;
+	bool need_restart;
+	int err;
+
+	/* Now set parameter */
+	err = ops->perm_config_set(devlink, param, type, value, &need_restart);
+	if (err)
+		return err;
+
+	cfgparam_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIG);
+	/* Update restart reqd - if any param needs restart, should be set */
+	if (need_restart) {
+		err = nla_put_flag(msg,
+				 DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED);
+		if (err)
+			goto nest_fail;
+	}
+
+	/* Since set was successful, write attr back to msg */
+	err = nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_PARAMETER, param);
+	if (err)
+		goto nest_fail;
+
+	nla_nest_end(msg, cfgparam_attr);
+
+	return 0;
+
+nest_fail:
+	nla_nest_cancel(msg, cfgparam_attr);
+	return err;
+}
+
+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 nlattr *tb[DEVLINK_ATTR_MAX + 1];
+	union devlink_perm_config_value value;
+	enum devlink_perm_config_param param;
+	enum devlink_perm_config_type type;
+	struct nlattr *cfgparam_attr;
+	struct sk_buff *msg;
+	struct nlattr *attr;
+	void *hdr;
+	int rem;
+	int err;
+
+	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_TYPE] ||
+		    !tb[DEVLINK_ATTR_PERM_CONFIG_VALUE])
+			continue;
+
+		param = nla_get_u32(tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER]);
+		type = nla_get_u8(tb[DEVLINK_ATTR_PERM_CONFIG_TYPE]);
+		if (param > DEVLINK_PERM_CONFIG_MAX ||
+		    type > NLA_TYPE_MAX) {
+			continue;
+		}
+
+		switch (type) {
+		case DEVLINK_PERM_CONFIG_TYPE_U8:
+			value.value8 =
+				nla_get_u8(tb[DEVLINK_ATTR_PERM_CONFIG_VALUE]);
+			break;
+		case DEVLINK_PERM_CONFIG_TYPE_U16:
+			value.value16 =
+				nla_get_u16(tb[DEVLINK_ATTR_PERM_CONFIG_VALUE]);
+			break;
+		case DEVLINK_PERM_CONFIG_TYPE_U32:
+			value.value32 =
+				nla_get_u32(tb[DEVLINK_ATTR_PERM_CONFIG_VALUE]);
+			break;
+		default:
+			continue;
+		}
+
+		/* Note if single_param_set fails, that param won't be in
+		 * response msg, so caller will know which param(s) failed to
+		 * set.
+		 */
+		devlink_nl_single_param_set(msg, devlink, param, type, &value);
+	}
+
+	nla_nest_end(msg, cfgparam_attr);
+
+	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 +2568,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_TYPE] = { .type = NLA_U8 },
 };
 
 static const struct genl_ops devlink_nl_ops[] = {
@@ -2451,6 +2730,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 v5 01/10] devlink: Add permanent config parameter get/set operations

[Jiri Pirko]

Mon, Oct 30, 2017 at 03:46:07PM CET, steven.lin1@broadcom.com wrote:
>Add support for permanent config parameter get/set commands. Used
>for persistent device configuration parameters.
>
>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>Acked-by: Andy Gospodarek <gospo@broadcom.com>

In general, I don't like how you use netlink. Please see the rest of
this code. We should do things in the common way. More info inlined.


[...]

> 
>+/* Permanent config parameters */
>+enum devlink_perm_config_param {
>+	__DEVLINK_PERM_CONFIG_MAX,
>+	DEVLINK_PERM_CONFIG_MAX = __DEVLINK_PERM_CONFIG_MAX - 1
>+};
>+
>+/* Permanent config types */
>+enum devlink_perm_config_type {
>+	DEVLINK_PERM_CONFIG_TYPE_UNSPEC,

You should not need this. Type should be always specified.


>+	DEVLINK_PERM_CONFIG_TYPE_U8,
>+	DEVLINK_PERM_CONFIG_TYPE_U16,
>+	DEVLINK_PERM_CONFIG_TYPE_U32,
>+};

This definitelly should not be in uapi header.

	
>+
>+/* Permanent config value */
>+union devlink_perm_config_value {
>+	__u8	value8;
>+	__u16	value16;
>+	__u32	value32;
>+};

This definitelly should not be in uapi header.



>+
> #endif /* _UAPI_LINUX_DEVLINK_H_ */

[...]


>+static int devlink_nl_config_get_fill(struct sk_buff *msg,
>+				      struct devlink *devlink,
>+				      enum devlink_command cmd,
>+				      struct genl_info *info)
>+{
>+	struct nlattr *tb[DEVLINK_ATTR_MAX + 1];
>+	enum devlink_perm_config_param param;
>+	enum devlink_perm_config_type type;
>+	struct nlattr *cfgparam_attr;
>+	struct nlattr *attr;
>+	void *hdr;
>+	int err;
>+	int rem;
>+
>+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
>+			  &devlink_nl_family, 0, cmd);
>+	if (!hdr)
>+		return -EMSGSIZE;
>+
>+	err = devlink_nl_put_handle(msg, devlink);
>+	if (err)
>+		goto nla_put_failure;
>+
>+	if (!info->attrs[DEVLINK_ATTR_PERM_CONFIGS]) {
>+		err = -EINVAL;
>+		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],


Okay. So I have to know what is in kernel in order to get the value.

We need a dump operation. In fact, why can't we just have dump operation?



>+			    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_TYPE])
>+			continue;
>+
>+		param = nla_get_u32(tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER]);
>+		type = nla_get_u8(tb[DEVLINK_ATTR_PERM_CONFIG_TYPE]);

So to get parameter, I have to specify a type? What if I specify wrong
type? That is wrong. The type should be read attr for get.

You should have a param -> type table and always use that.


>+		if (param > DEVLINK_PERM_CONFIG_MAX ||
>+		    type > NLA_TYPE_MAX) {
>+			continue;
>+		}
>+		/* Note if single_param_get fails, that param won't be in
>+		 * response msg, so caller will know which param(s) failed to
>+		 * get.
>+		 */
>+		devlink_nl_single_param_get(msg, devlink, param, type);
>+	}
>+
>+	nla_nest_end(msg, cfgparam_attr);
>+
>+	genlmsg_end(msg, hdr);
>+	return 0;
>+
>+nla_nest_failure:
>+	nla_nest_cancel(msg, cfgparam_attr);
>+nla_put_failure:


You should make this multipart aware. If config parameters won't fit
into a single skb, you have to allocate another one. See devlink_dpipe_tables_fill
as an example how to do it. It is important to have this from the
beginning as the userspace knows to expect multipart message.



>+	genlmsg_cancel(msg, hdr);
>+	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,
>+				       enum devlink_perm_config_param param,
>+				       enum devlink_perm_config_type type,
>+				       union devlink_perm_config_value *value)
>+{
>+	const struct devlink_ops *ops = devlink->ops;
>+	struct nlattr *cfgparam_attr;
>+	bool need_restart;
>+	int err;
>+
>+	/* Now set parameter */
>+	err = ops->perm_config_set(devlink, param, type, value, &need_restart);
>+	if (err)
>+		return err;
>+
>+	cfgparam_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIG);
>+	/* Update restart reqd - if any param needs restart, should be set */
>+	if (need_restart) {

You should propagate this out so the caller would fill it to the
message. This is a global thing, not per-param, shoult not be nested.



>+		err = nla_put_flag(msg,
>+				 DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED);
>+		if (err)
>+			goto nest_fail;
>+	}
>+
>+	/* Since set was successful, write attr back to msg */
>+	err = nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_PARAMETER, param);
>+	if (err)
>+		goto nest_fail;
>+
>+	nla_nest_end(msg, cfgparam_attr);
>+
>+	return 0;
>+
>+nest_fail:
>+	nla_nest_cancel(msg, cfgparam_attr);
>+	return err;
>+}
>+
>+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 nlattr *tb[DEVLINK_ATTR_MAX + 1];
>+	union devlink_perm_config_value value;
>+	enum devlink_perm_config_param param;
>+	enum devlink_perm_config_type type;
>+	struct nlattr *cfgparam_attr;
>+	struct sk_buff *msg;
>+	struct nlattr *attr;
>+	void *hdr;
>+	int rem;
>+	int err;
>+
>+	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_TYPE] ||
>+		    !tb[DEVLINK_ATTR_PERM_CONFIG_VALUE])
>+			continue;
>+
>+		param = nla_get_u32(tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER]);
>+		type = nla_get_u8(tb[DEVLINK_ATTR_PERM_CONFIG_TYPE]);
>+		if (param > DEVLINK_PERM_CONFIG_MAX ||
>+		    type > NLA_TYPE_MAX) {
>+			continue;
>+		}
>+
>+		switch (type) {
>+		case DEVLINK_PERM_CONFIG_TYPE_U8:
>+			value.value8 =
>+				nla_get_u8(tb[DEVLINK_ATTR_PERM_CONFIG_VALUE]);
>+			break;
>+		case DEVLINK_PERM_CONFIG_TYPE_U16:
>+			value.value16 =
>+				nla_get_u16(tb[DEVLINK_ATTR_PERM_CONFIG_VALUE]);
>+			break;
>+		case DEVLINK_PERM_CONFIG_TYPE_U32:
>+			value.value32 =
>+				nla_get_u32(tb[DEVLINK_ATTR_PERM_CONFIG_VALUE]);
>+			break;
>+		default:
>+			continue;
>+		}
>+
>+		/* Note if single_param_set fails, that param won't be in
>+		 * response msg, so caller will know which param(s) failed to
>+		 * set.

No. You should not send the params back on set operation. That is wrong.
You should use extact in order to tell anything useful to the user.


>+		 */
>+		devlink_nl_single_param_set(msg, devlink, param, type, &value);
>+	}
>+
>+	nla_nest_end(msg, cfgparam_attr);
>+
>+	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 +2568,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_TYPE] = { .type = NLA_U8 },
> };
> 
> static const struct genl_ops devlink_nl_ops[] = {
>@@ -2451,6 +2730,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,

As I said, you need dumpit.



>+		.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 v5 01/10] devlink: Add permanent config parameter get/set operations

[Steve Lin]

On Mon, Oct 30, 2017 at 1:03 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Mon, Oct 30, 2017 at 03:46:07PM CET, steven.lin1@broadcom.com wrote:
>>Add support for permanent config parameter get/set commands. Used
>>for persistent device configuration parameters.
>>
>>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>>Acked-by: Andy Gospodarek <gospo@broadcom.com>
>
> In general, I don't like how you use netlink. Please see the rest of
> this code. We should do things in the common way. More info inlined.
>
>
> [...]
>
>>
>>+/* Permanent config parameters */
>>+enum devlink_perm_config_param {
>>+      __DEVLINK_PERM_CONFIG_MAX,
>>+      DEVLINK_PERM_CONFIG_MAX = __DEVLINK_PERM_CONFIG_MAX - 1
>>+};
>>+
>>+/* Permanent config types */
>>+enum devlink_perm_config_type {
>>+      DEVLINK_PERM_CONFIG_TYPE_UNSPEC,
>
> You should not need this. Type should be always specified.

This was an attempt to align enum devlink_perm_config_type with the
NLA_* enums in netlink.h, which include the unspecified type.  I will
remove the unspecified enum, but it seems like having multiple type
enums that are not aligned with each other could be problematic.

>
>
>>+      DEVLINK_PERM_CONFIG_TYPE_U8,
>>+      DEVLINK_PERM_CONFIG_TYPE_U16,
>>+      DEVLINK_PERM_CONFIG_TYPE_U32,
>>+};
>
> This definitelly should not be in uapi header.
>
>
>>+
>>+/* Permanent config value */
>>+union devlink_perm_config_value {
>>+      __u8    value8;
>>+      __u16   value16;
>>+      __u32   value32;
>>+};
>
> This definitelly should not be in uapi header.

Ok, will move these.

>
>
>
>>+
>> #endif /* _UAPI_LINUX_DEVLINK_H_ */
>
> [...]
>
>
>>+static int devlink_nl_config_get_fill(struct sk_buff *msg,
>>+                                    struct devlink *devlink,
>>+                                    enum devlink_command cmd,
>>+                                    struct genl_info *info)
>>+{
>>+      struct nlattr *tb[DEVLINK_ATTR_MAX + 1];
>>+      enum devlink_perm_config_param param;
>>+      enum devlink_perm_config_type type;
>>+      struct nlattr *cfgparam_attr;
>>+      struct nlattr *attr;
>>+      void *hdr;
>>+      int err;
>>+      int rem;
>>+
>>+      hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
>>+                        &devlink_nl_family, 0, cmd);
>>+      if (!hdr)
>>+              return -EMSGSIZE;
>>+
>>+      err = devlink_nl_put_handle(msg, devlink);
>>+      if (err)
>>+              goto nla_put_failure;
>>+
>>+      if (!info->attrs[DEVLINK_ATTR_PERM_CONFIGS]) {
>>+              err = -EINVAL;
>>+              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],
>
>
> Okay. So I have to know what is in kernel in order to get the value.
>
> We need a dump operation. In fact, why can't we just have dump operation?

Ok, I can add a dump operation.

>
>
>
>>+                          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_TYPE])
>>+                      continue;
>>+
>>+              param = nla_get_u32(tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER]);
>>+              type = nla_get_u8(tb[DEVLINK_ATTR_PERM_CONFIG_TYPE]);
>
> So to get parameter, I have to specify a type? What if I specify wrong
> type? That is wrong. The type should be read attr for get.

For set, seems like the user would need to know and set the type.  So,
I was just doing the same for get().  But I can make the type a read
attribute for get.

>
> You should have a param -> type table and always use that.
>
>
>>+              if (param > DEVLINK_PERM_CONFIG_MAX ||
>>+                  type > NLA_TYPE_MAX) {
>>+                      continue;
>>+              }
>>+              /* Note if single_param_get fails, that param won't be in
>>+               * response msg, so caller will know which param(s) failed to
>>+               * get.
>>+               */
>>+              devlink_nl_single_param_get(msg, devlink, param, type);
>>+      }
>>+
>>+      nla_nest_end(msg, cfgparam_attr);
>>+
>>+      genlmsg_end(msg, hdr);
>>+      return 0;
>>+
>>+nla_nest_failure:
>>+      nla_nest_cancel(msg, cfgparam_attr);
>>+nla_put_failure:
>
>
> You should make this multipart aware. If config parameters won't fit
> into a single skb, you have to allocate another one. See devlink_dpipe_tables_fill
> as an example how to do it. It is important to have this from the
> beginning as the userspace knows to expect multipart message.

In a response to a similar comment from v2, I tried to describe why I
felt multipart messages wouldn't be required.  There was no response
to my reply, so I thought that my argument was accepted.  Apparently
not, so I will add multipart support.

>
>
>
>>+      genlmsg_cancel(msg, hdr);
>>+      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,
>>+                                     enum devlink_perm_config_param param,
>>+                                     enum devlink_perm_config_type type,
>>+                                     union devlink_perm_config_value *value)
>>+{
>>+      const struct devlink_ops *ops = devlink->ops;
>>+      struct nlattr *cfgparam_attr;
>>+      bool need_restart;
>>+      int err;
>>+
>>+      /* Now set parameter */
>>+      err = ops->perm_config_set(devlink, param, type, value, &need_restart);
>>+      if (err)
>>+              return err;
>>+
>>+      cfgparam_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIG);
>>+      /* Update restart reqd - if any param needs restart, should be set */
>>+      if (need_restart) {
>
> You should propagate this out so the caller would fill it to the
> message. This is a global thing, not per-param, shoult not be nested.

The idea here was that some parameters may require a restart and some
may not.  By reporting it per param, the caller knows which parameter
have taken effect immediately.

But, per your comment below, I will no longer report back which
parameters were successfully set (and thus also not return the
restart_required information), and instead just put any extra needed
information into extack.

>
>
>
>>+              err = nla_put_flag(msg,
>>+                               DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED);
>>+              if (err)
>>+                      goto nest_fail;
>>+      }
>>+
>>+      /* Since set was successful, write attr back to msg */
>>+      err = nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_PARAMETER, param);
>>+      if (err)
>>+              goto nest_fail;
>>+
>>+      nla_nest_end(msg, cfgparam_attr);
>>+
>>+      return 0;
>>+
>>+nest_fail:
>>+      nla_nest_cancel(msg, cfgparam_attr);
>>+      return err;
>>+}
>>+
>>+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 nlattr *tb[DEVLINK_ATTR_MAX + 1];
>>+      union devlink_perm_config_value value;
>>+      enum devlink_perm_config_param param;
>>+      enum devlink_perm_config_type type;
>>+      struct nlattr *cfgparam_attr;
>>+      struct sk_buff *msg;
>>+      struct nlattr *attr;
>>+      void *hdr;
>>+      int rem;
>>+      int err;
>>+
>>+      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_TYPE] ||
>>+                  !tb[DEVLINK_ATTR_PERM_CONFIG_VALUE])
>>+                      continue;
>>+
>>+              param = nla_get_u32(tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER]);
>>+              type = nla_get_u8(tb[DEVLINK_ATTR_PERM_CONFIG_TYPE]);
>>+              if (param > DEVLINK_PERM_CONFIG_MAX ||
>>+                  type > NLA_TYPE_MAX) {
>>+                      continue;
>>+              }
>>+
>>+              switch (type) {
>>+              case DEVLINK_PERM_CONFIG_TYPE_U8:
>>+                      value.value8 =
>>+                              nla_get_u8(tb[DEVLINK_ATTR_PERM_CONFIG_VALUE]);
>>+                      break;
>>+              case DEVLINK_PERM_CONFIG_TYPE_U16:
>>+                      value.value16 =
>>+                              nla_get_u16(tb[DEVLINK_ATTR_PERM_CONFIG_VALUE]);
>>+                      break;
>>+              case DEVLINK_PERM_CONFIG_TYPE_U32:
>>+                      value.value32 =
>>+                              nla_get_u32(tb[DEVLINK_ATTR_PERM_CONFIG_VALUE]);
>>+                      break;
>>+              default:
>>+                      continue;
>>+              }
>>+
>>+              /* Note if single_param_set fails, that param won't be in
>>+               * response msg, so caller will know which param(s) failed to
>>+               * set.
>
> No. You should not send the params back on set operation. That is wrong.
> You should use extact in order to tell anything useful to the user.

Ok, see above.

>
>
>>+               */
>>+              devlink_nl_single_param_set(msg, devlink, param, type, &value);
>>+      }
>>+
>>+      nla_nest_end(msg, cfgparam_attr);
>>+
>>+      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 +2568,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_TYPE] = { .type = NLA_U8 },
>> };
>>
>> static const struct genl_ops devlink_nl_ops[] = {
>>@@ -2451,6 +2730,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,
>
> As I said, you need dumpit.
>
>
>
>>+              .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 v5 01/10] devlink: Add permanent config parameter get/set operations

[Jakub Kicinski]

On Mon, 30 Oct 2017 18:03:01 +0100, Jiri Pirko wrote:
> >+	cfgparam_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIG);
> >+	/* Update restart reqd - if any param needs restart, should be set */
> >+	if (need_restart) {  
> 
> You should propagate this out so the caller would fill it to the
> message. This is a global thing, not per-param, shoult not be nested.

Right, I think Jiri has already asked for this but I feel like we
should provide the ability to query running/pending configurations
independently.  I don't see it in this patch set?

I'm not sure what the status of the reconfig trigger patches for mlxsw
is, but we actually may need 3 config sets:
 - current/runtime configurable, 
 - requiring soft reset of the device/driver reinit;
 - requiring hard reset/set on boot.

Secondly, IMHO calling set/get parameters "permanent" is a bit
backwards.  One device may not be able to change max VF counts or MSIX
allocation without full reinit of PCIe blocks, but for others soft
reset is more than enough.  Port splitting is another example.  For 
NICs port splitting at runtime is usually not a priority in HW/FW
development, so some form of reset is generally required, while
switches can split a port at runtime.  IOW we should define parameters
without assigning them to config sets in the ABI itself.  And also we
should make it in a way which will allow existing parameters to be
reused in permanent/sort reset required/runtime modes.

Does that make sense?

Re: [PATCH net-next v5 01/10] devlink: Add permanent config parameter get/set operations

[Jiri Pirko]

Mon, Oct 30, 2017 at 11:12:13PM CET, kubakici@wp.pl wrote:
>On Mon, 30 Oct 2017 18:03:01 +0100, Jiri Pirko wrote:
>> >+	cfgparam_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIG);
>> >+	/* Update restart reqd - if any param needs restart, should be set */
>> >+	if (need_restart) {  
>> 
>> You should propagate this out so the caller would fill it to the
>> message. This is a global thing, not per-param, shoult not be nested.
>
>Right, I think Jiri has already asked for this but I feel like we
>should provide the ability to query running/pending configurations
>independently.  I don't see it in this patch set?

I don't see it either :/


>
>I'm not sure what the status of the reconfig trigger patches for mlxsw
>is, but we actually may need 3 config sets:
> - current/runtime configurable, 
> - requiring soft reset of the device/driver reinit;
> - requiring hard reset/set on boot.
>
>Secondly, IMHO calling set/get parameters "permanent" is a bit
>backwards.  One device may not be able to change max VF counts or MSIX
>allocation without full reinit of PCIe blocks, but for others soft
>reset is more than enough.  Port splitting is another example.  For 
>NICs port splitting at runtime is usually not a priority in HW/FW
>development, so some form of reset is generally required, while
>switches can split a port at runtime.  IOW we should define parameters
>without assigning them to config sets in the ABI itself.  And also we
>should make it in a way which will allow existing parameters to be
>reused in permanent/sort reset required/runtime modes.
>
>Does that make sense?

"IOW we should define parameters without assigning them to config sets
in the ABI itself" - I don't understand what do you mean by this.

Re: [PATCH net-next v5 01/10] devlink: Add permanent config parameter get/set operations

[Jakub Kicinski]

On Tue, 31 Oct 2017 08:17:30 +0100, Jiri Pirko wrote:
> Mon, Oct 30, 2017 at 11:12:13PM CET, kubakici@wp.pl wrote:
> >On Mon, 30 Oct 2017 18:03:01 +0100, Jiri Pirko wrote:  
> >I'm not sure what the status of the reconfig trigger patches for mlxsw
> >is, but we actually may need 3 config sets:
> > - current/runtime configurable, 
> > - requiring soft reset of the device/driver reinit;
> > - requiring hard reset/set on boot.
> >
> >Secondly, IMHO calling set/get parameters "permanent" is a bit
> >backwards.  One device may not be able to change max VF counts or MSIX
> >allocation without full reinit of PCIe blocks, but for others soft
> >reset is more than enough.  Port splitting is another example.  For 
> >NICs port splitting at runtime is usually not a priority in HW/FW
> >development, so some form of reset is generally required, while
> >switches can split a port at runtime.  IOW we should define parameters
> >without assigning them to config sets in the ABI itself.  And also we
> >should make it in a way which will allow existing parameters to be
> >reused in permanent/sort reset required/runtime modes.
> >
> >Does that make sense?  
> 
> "IOW we should define parameters without assigning them to config sets
> in the ABI itself" - I don't understand what do you mean by this.

OK, whether the setting is permanent or not - is device specific.

I'm basically asking to remove the "PERM" from the names and indicate
which config set (of the 3 enumerated above) user wants it applied in a
separate attribute.

Re: [PATCH net-next v5 01/10] devlink: Add permanent config parameter get/set operations

[Jiri Pirko]

Tue, Oct 31, 2017 at 09:04:18AM CET, kubakici@wp.pl wrote:
>On Tue, 31 Oct 2017 08:17:30 +0100, Jiri Pirko wrote:
>> Mon, Oct 30, 2017 at 11:12:13PM CET, kubakici@wp.pl wrote:
>> >On Mon, 30 Oct 2017 18:03:01 +0100, Jiri Pirko wrote:  
>> >I'm not sure what the status of the reconfig trigger patches for mlxsw
>> >is, but we actually may need 3 config sets:
>> > - current/runtime configurable, 
>> > - requiring soft reset of the device/driver reinit;
>> > - requiring hard reset/set on boot.
>> >
>> >Secondly, IMHO calling set/get parameters "permanent" is a bit
>> >backwards.  One device may not be able to change max VF counts or MSIX
>> >allocation without full reinit of PCIe blocks, but for others soft
>> >reset is more than enough.  Port splitting is another example.  For 
>> >NICs port splitting at runtime is usually not a priority in HW/FW
>> >development, so some form of reset is generally required, while
>> >switches can split a port at runtime.  IOW we should define parameters
>> >without assigning them to config sets in the ABI itself.  And also we
>> >should make it in a way which will allow existing parameters to be
>> >reused in permanent/sort reset required/runtime modes.
>> >
>> >Does that make sense?  
>> 
>> "IOW we should define parameters without assigning them to config sets
>> in the ABI itself" - I don't understand what do you mean by this.
>
>OK, whether the setting is permanent or not - is device specific.
>
>I'm basically asking to remove the "PERM" from the names and indicate
>which config set (of the 3 enumerated above) user wants it applied in a
>separate attribute.

Yesh, would be good if driver could indicate which option is of what
type and devlink should expose this information to the user.

But it probably should be flags. Driver may provide possibility to
configure for example VF count as runtime and also permanent option.
User should pass the correct flags along the set message so the driver
knows what to set. Makes sense?

Re: [PATCH net-next v5 01/10] devlink: Add permanent config parameter get/set operations

[Jakub Kicinski]

On Tue, 31 Oct 2017 10:00:24 +0100, Jiri Pirko wrote:
> Tue, Oct 31, 2017 at 09:04:18AM CET, kubakici@wp.pl wrote:
> >On Tue, 31 Oct 2017 08:17:30 +0100, Jiri Pirko wrote:  
> >> Mon, Oct 30, 2017 at 11:12:13PM CET, kubakici@wp.pl wrote:  
> >> >On Mon, 30 Oct 2017 18:03:01 +0100, Jiri Pirko wrote:  
> >> >I'm not sure what the status of the reconfig trigger patches for mlxsw
> >> >is, but we actually may need 3 config sets:
> >> > - current/runtime configurable, 
> >> > - requiring soft reset of the device/driver reinit;
> >> > - requiring hard reset/set on boot.
> >> >
> >> >Secondly, IMHO calling set/get parameters "permanent" is a bit
> >> >backwards.  One device may not be able to change max VF counts or MSIX
> >> >allocation without full reinit of PCIe blocks, but for others soft
> >> >reset is more than enough.  Port splitting is another example.  For 
> >> >NICs port splitting at runtime is usually not a priority in HW/FW
> >> >development, so some form of reset is generally required, while
> >> >switches can split a port at runtime.  IOW we should define parameters
> >> >without assigning them to config sets in the ABI itself.  And also we
> >> >should make it in a way which will allow existing parameters to be
> >> >reused in permanent/sort reset required/runtime modes.
> >> >
> >> >Does that make sense?    
> >> 
> >> "IOW we should define parameters without assigning them to config sets
> >> in the ABI itself" - I don't understand what do you mean by this.  
> >
> >OK, whether the setting is permanent or not - is device specific.
> >
> >I'm basically asking to remove the "PERM" from the names and indicate
> >which config set (of the 3 enumerated above) user wants it applied in a
> >separate attribute.  
> 
> Yesh, would be good if driver could indicate which option is of what
> type and devlink should expose this information to the user.

Sounds like there would be one dump with all settings each marked with
a flag indicating if it's permanent/init time or run time.  How would
users check the current vs pending configurations?

> But it probably should be flags. Driver may provide possibility to
> configure for example VF count as runtime and also permanent option.
> User should pass the correct flags along the set message so the driver
> knows what to set. Makes sense?

SGTM.  And we can also extend port splitting to take the same flags? :)

[PATCH net-next v5 02/10] devlink: Adding SR-IOV enablement perm config param

[Steve Lin]

Adding DEVLINK_PERM_CONFIG_SRIOV_ENABLED permanent config
parameter; this parameter controls whether the device
advertises the SR-IOV PCI capability. Value is permanent, so
becomes the new default value for this device.

  DEVLINK_PERM_CONFIG_DISABLE = Disable SR-IOV
  DEVLINK_PERM_CONFIG_ENABLE  = Enable SR-IOV

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 include/uapi/linux/devlink.h | 18 +++++++++++++++++-
 net/core/devlink.c           |  1 +
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 55e35f2..1c5d4ae 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -256,8 +256,24 @@ enum devlink_dpipe_header_id {
 	DEVLINK_DPIPE_HEADER_IPV6,
 };
 
-/* Permanent config parameters */
+/* Permanent config parameter enable/disable
+ * DEVLINK_PERM_CONFIG_DISABLE = disable a permanent config parameter
+ * DEVLINK_PERM_CONFIG_ENBALE  = enable a permanent config parameter
+ */
+enum devlink_perm_config_enabled {
+	DEVLINK_PERM_CONFIG_DISABLE,
+	DEVLINK_PERM_CONFIG_ENABLE,
+};
+
+/* Permanent config parameters:
+ * DEVLINK_PERM_CONFIG_SRIOV_ENABLED: Configures whether SR-IOV PCI capability
+ * is advertised by the device.
+ *   DEVLINK_PERM_CONFIG_DISABLE = disable SR-IOV
+ *   DEVLINK_PERM_CONFIG_ENABLE  = enable SR-IOV
+ */
 enum devlink_perm_config_param {
+	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
+
 	__DEVLINK_PERM_CONFIG_MAX,
 	DEVLINK_PERM_CONFIG_MAX = __DEVLINK_PERM_CONFIG_MAX - 1
 };
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 5e77408..b4d43c29 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -1569,6 +1569,7 @@ static int devlink_nl_cmd_eswitch_set_doit(struct sk_buff *skb,
 static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1];
 
 static const u8 devlink_perm_cfg_param_types[DEVLINK_PERM_CONFIG_MAX + 1] = {
+	[DEVLINK_PERM_CONFIG_SRIOV_ENABLED] = NLA_U8,
 };
 
 static int devlink_nl_single_param_get(struct sk_buff *msg,
-- 
2.7.4

Re: [PATCH net-next v5 02/10] devlink: Adding SR-IOV enablement perm config param

[Jiri Pirko]

Mon, Oct 30, 2017 at 03:46:08PM CET, steven.lin1@broadcom.com wrote:
>Adding DEVLINK_PERM_CONFIG_SRIOV_ENABLED permanent config
>parameter; this parameter controls whether the device
>advertises the SR-IOV PCI capability. Value is permanent, so
>becomes the new default value for this device.
>
>  DEVLINK_PERM_CONFIG_DISABLE = Disable SR-IOV
>  DEVLINK_PERM_CONFIG_ENABLE  = Enable SR-IOV
>
>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>Acked-by: Andy Gospodarek <gospo@broadcom.com>
>---
> include/uapi/linux/devlink.h | 18 +++++++++++++++++-
> net/core/devlink.c           |  1 +
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index 55e35f2..1c5d4ae 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -256,8 +256,24 @@ enum devlink_dpipe_header_id {
> 	DEVLINK_DPIPE_HEADER_IPV6,
> };
> 
>-/* Permanent config parameters */
>+/* Permanent config parameter enable/disable
>+ * DEVLINK_PERM_CONFIG_DISABLE = disable a permanent config parameter
>+ * DEVLINK_PERM_CONFIG_ENBALE  = enable a permanent config parameter
>+ */
>+enum devlink_perm_config_enabled {
>+	DEVLINK_PERM_CONFIG_DISABLE,
>+	DEVLINK_PERM_CONFIG_ENABLE,
>+};


Basically, this is "bool"

Why don't you use some own enum instead of NLA_U*. Like team does for
example:

enum team_option_type {
        TEAM_OPTION_TYPE_U32,
        TEAM_OPTION_TYPE_STRING,
        TEAM_OPTION_TYPE_BINARY,
        TEAM_OPTION_TYPE_BOOL,
        TEAM_OPTION_TYPE_S32,
};



>+
>+/* Permanent config parameters:
>+ * DEVLINK_PERM_CONFIG_SRIOV_ENABLED: Configures whether SR-IOV PCI capability
>+ * is advertised by the device.
>+ *   DEVLINK_PERM_CONFIG_DISABLE = disable SR-IOV
>+ *   DEVLINK_PERM_CONFIG_ENABLE  = enable SR-IOV
>+ */
> enum devlink_perm_config_param {
>+	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,


Please align "ENABLED" vs "ENABLE".
The rest of devlink code uses "ENABLED"


>+
> 	__DEVLINK_PERM_CONFIG_MAX,
> 	DEVLINK_PERM_CONFIG_MAX = __DEVLINK_PERM_CONFIG_MAX - 1
> };
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 5e77408..b4d43c29 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -1569,6 +1569,7 @@ static int devlink_nl_cmd_eswitch_set_doit(struct sk_buff *skb,
> static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1];
> 
> static const u8 devlink_perm_cfg_param_types[DEVLINK_PERM_CONFIG_MAX + 1] = {
>+	[DEVLINK_PERM_CONFIG_SRIOV_ENABLED] = NLA_U8,
> };
> 
> static int devlink_nl_single_param_get(struct sk_buff *msg,
>-- 
>2.7.4
>

Re: [PATCH net-next v5 02/10] devlink: Adding SR-IOV enablement perm config param

[Steve Lin]

On Mon, Oct 30, 2017 at 1:20 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Mon, Oct 30, 2017 at 03:46:08PM CET, steven.lin1@broadcom.com wrote:
>>Adding DEVLINK_PERM_CONFIG_SRIOV_ENABLED permanent config
>>parameter; this parameter controls whether the device
>>advertises the SR-IOV PCI capability. Value is permanent, so
>>becomes the new default value for this device.
>>
>>  DEVLINK_PERM_CONFIG_DISABLE = Disable SR-IOV
>>  DEVLINK_PERM_CONFIG_ENABLE  = Enable SR-IOV
>>
>>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>>Acked-by: Andy Gospodarek <gospo@broadcom.com>
>>---
>> include/uapi/linux/devlink.h | 18 +++++++++++++++++-
>> net/core/devlink.c           |  1 +
>> 2 files changed, 18 insertions(+), 1 deletion(-)
>>
>>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>>index 55e35f2..1c5d4ae 100644
>>--- a/include/uapi/linux/devlink.h
>>+++ b/include/uapi/linux/devlink.h
>>@@ -256,8 +256,24 @@ enum devlink_dpipe_header_id {
>>       DEVLINK_DPIPE_HEADER_IPV6,
>> };
>>
>>-/* Permanent config parameters */
>>+/* Permanent config parameter enable/disable
>>+ * DEVLINK_PERM_CONFIG_DISABLE = disable a permanent config parameter
>>+ * DEVLINK_PERM_CONFIG_ENBALE  = enable a permanent config parameter
>>+ */
>>+enum devlink_perm_config_enabled {
>>+      DEVLINK_PERM_CONFIG_DISABLE,
>>+      DEVLINK_PERM_CONFIG_ENABLE,
>>+};
>
>
> Basically, this is "bool"
>
> Why don't you use some own enum instead of NLA_U*. Like team does for
> example:
>
> enum team_option_type {
>         TEAM_OPTION_TYPE_U32,
>         TEAM_OPTION_TYPE_STRING,
>         TEAM_OPTION_TYPE_BINARY,
>         TEAM_OPTION_TYPE_BOOL,
>         TEAM_OPTION_TYPE_S32,
> };
>
>

I had added enum devlink_perm_config_type in v5 based on your comments
in v4 - I will add BOOL if it helps clarity.

>
>>+
>>+/* Permanent config parameters:
>>+ * DEVLINK_PERM_CONFIG_SRIOV_ENABLED: Configures whether SR-IOV PCI capability
>>+ * is advertised by the device.
>>+ *   DEVLINK_PERM_CONFIG_DISABLE = disable SR-IOV
>>+ *   DEVLINK_PERM_CONFIG_ENABLE  = enable SR-IOV
>>+ */
>> enum devlink_perm_config_param {
>>+      DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
>
>
> Please align "ENABLED" vs "ENABLE".
> The rest of devlink code uses "ENABLED"
>

Ok.

>
>>+
>>       __DEVLINK_PERM_CONFIG_MAX,
>>       DEVLINK_PERM_CONFIG_MAX = __DEVLINK_PERM_CONFIG_MAX - 1
>> };
>>diff --git a/net/core/devlink.c b/net/core/devlink.c
>>index 5e77408..b4d43c29 100644
>>--- a/net/core/devlink.c
>>+++ b/net/core/devlink.c
>>@@ -1569,6 +1569,7 @@ static int devlink_nl_cmd_eswitch_set_doit(struct sk_buff *skb,
>> static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1];
>>
>> static const u8 devlink_perm_cfg_param_types[DEVLINK_PERM_CONFIG_MAX + 1] = {
>>+      [DEVLINK_PERM_CONFIG_SRIOV_ENABLED] = NLA_U8,
>> };
>>
>> static int devlink_nl_single_param_get(struct sk_buff *msg,
>>--
>>2.7.4
>>

Re: [PATCH net-next v5 02/10] devlink: Adding SR-IOV enablement perm config param

[Jiri Pirko]

Mon, Oct 30, 2017 at 09:17:16PM CET, steven.lin1@broadcom.com wrote:
>On Mon, Oct 30, 2017 at 1:20 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Mon, Oct 30, 2017 at 03:46:08PM CET, steven.lin1@broadcom.com wrote:
>>>Adding DEVLINK_PERM_CONFIG_SRIOV_ENABLED permanent config
>>>parameter; this parameter controls whether the device
>>>advertises the SR-IOV PCI capability. Value is permanent, so
>>>becomes the new default value for this device.
>>>
>>>  DEVLINK_PERM_CONFIG_DISABLE = Disable SR-IOV
>>>  DEVLINK_PERM_CONFIG_ENABLE  = Enable SR-IOV
>>>
>>>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>>>Acked-by: Andy Gospodarek <gospo@broadcom.com>
>>>---
>>> include/uapi/linux/devlink.h | 18 +++++++++++++++++-
>>> net/core/devlink.c           |  1 +
>>> 2 files changed, 18 insertions(+), 1 deletion(-)
>>>
>>>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>>>index 55e35f2..1c5d4ae 100644
>>>--- a/include/uapi/linux/devlink.h
>>>+++ b/include/uapi/linux/devlink.h
>>>@@ -256,8 +256,24 @@ enum devlink_dpipe_header_id {
>>>       DEVLINK_DPIPE_HEADER_IPV6,
>>> };
>>>
>>>-/* Permanent config parameters */
>>>+/* Permanent config parameter enable/disable
>>>+ * DEVLINK_PERM_CONFIG_DISABLE = disable a permanent config parameter
>>>+ * DEVLINK_PERM_CONFIG_ENBALE  = enable a permanent config parameter
>>>+ */
>>>+enum devlink_perm_config_enabled {
>>>+      DEVLINK_PERM_CONFIG_DISABLE,
>>>+      DEVLINK_PERM_CONFIG_ENABLE,
>>>+};
>>
>>
>> Basically, this is "bool"
>>
>> Why don't you use some own enum instead of NLA_U*. Like team does for
>> example:
>>
>> enum team_option_type {
>>         TEAM_OPTION_TYPE_U32,
>>         TEAM_OPTION_TYPE_STRING,
>>         TEAM_OPTION_TYPE_BINARY,
>>         TEAM_OPTION_TYPE_BOOL,
>>         TEAM_OPTION_TYPE_S32,
>> };
>>
>>
>
>I had added enum devlink_perm_config_type in v5 based on your comments
>in v4 - I will add BOOL if it helps clarity.

But you did not use it in uapi for some reason. That is what I suggest.


>
>>
>>>+
>>>+/* Permanent config parameters:
>>>+ * DEVLINK_PERM_CONFIG_SRIOV_ENABLED: Configures whether SR-IOV PCI capability
>>>+ * is advertised by the device.
>>>+ *   DEVLINK_PERM_CONFIG_DISABLE = disable SR-IOV
>>>+ *   DEVLINK_PERM_CONFIG_ENABLE  = enable SR-IOV
>>>+ */
>>> enum devlink_perm_config_param {
>>>+      DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
>>
>>
>> Please align "ENABLED" vs "ENABLE".
>> The rest of devlink code uses "ENABLED"
>>
>
>Ok.
>
>>
>>>+
>>>       __DEVLINK_PERM_CONFIG_MAX,
>>>       DEVLINK_PERM_CONFIG_MAX = __DEVLINK_PERM_CONFIG_MAX - 1
>>> };
>>>diff --git a/net/core/devlink.c b/net/core/devlink.c
>>>index 5e77408..b4d43c29 100644
>>>--- a/net/core/devlink.c
>>>+++ b/net/core/devlink.c
>>>@@ -1569,6 +1569,7 @@ static int devlink_nl_cmd_eswitch_set_doit(struct sk_buff *skb,
>>> static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1];
>>>
>>> static const u8 devlink_perm_cfg_param_types[DEVLINK_PERM_CONFIG_MAX + 1] = {
>>>+      [DEVLINK_PERM_CONFIG_SRIOV_ENABLED] = NLA_U8,
>>> };
>>>
>>> static int devlink_nl_single_param_get(struct sk_buff *msg,
>>>--
>>>2.7.4
>>>

[PATCH net-next v5 03/10] devlink: Adding num VFs per PF permanent config param

[Steve Lin]

Adding DEVLINK_PERM_CONFIG_NUM_VF_PER_PF permanent config
parameter. Value is permanent, so becomes the new default
value for this device.

The value sets the number of VFs per PF in SR-IOV mode.

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

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 1c5d4ae..4e0b8b7 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -270,9 +270,13 @@ enum devlink_perm_config_enabled {
  * is advertised by the device.
  *   DEVLINK_PERM_CONFIG_DISABLE = disable SR-IOV
  *   DEVLINK_PERM_CONFIG_ENABLE  = enable SR-IOV
+ * DEVLINK_PERM_CONFIG_NUM_VF_PER_PF: Configures the number of total/maximum
+ * VFs per PF available on device.
+ *   Value: # of VFs per PF in SR-IOV mode
  */
 enum devlink_perm_config_param {
 	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
+	DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
 
 	__DEVLINK_PERM_CONFIG_MAX,
 	DEVLINK_PERM_CONFIG_MAX = __DEVLINK_PERM_CONFIG_MAX - 1
diff --git a/net/core/devlink.c b/net/core/devlink.c
index b4d43c29..f421cfb 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -1570,6 +1570,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1];
 
 static const u8 devlink_perm_cfg_param_types[DEVLINK_PERM_CONFIG_MAX + 1] = {
 	[DEVLINK_PERM_CONFIG_SRIOV_ENABLED] = NLA_U8,
+	[DEVLINK_PERM_CONFIG_NUM_VF_PER_PF] = NLA_U32,
 };
 
 static int devlink_nl_single_param_get(struct sk_buff *msg,
-- 
2.7.4

[PATCH net-next v5 04/10] devlink: Adding max PF MSI-X vectors perm config param

[Steve Lin]

Adding DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT permanent config
parameter. Value is permanent, so becomes the new default value
for this device.

Value sets the maximum number of PF MSI-X vectors.

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

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 4e0b8b7..6936c0e 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -273,10 +273,14 @@ enum devlink_perm_config_enabled {
  * DEVLINK_PERM_CONFIG_NUM_VF_PER_PF: Configures the number of total/maximum
  * VFs per PF available on device.
  *   Value: # of VFs per PF in SR-IOV mode
+ * DEVLINK_PERM_CONFIG_NUM_PF_MSIX_VECT: Configures the # of MSI-X vectors
+ * advertised via PCI capability for this device.
+ *   Value = Max # of MSI-X vectors per PF
  */
 enum devlink_perm_config_param {
 	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
 	DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
+	DEVLINK_PERM_CONFIG_NUM_PF_MSIX_VECT,
 
 	__DEVLINK_PERM_CONFIG_MAX,
 	DEVLINK_PERM_CONFIG_MAX = __DEVLINK_PERM_CONFIG_MAX - 1
diff --git a/net/core/devlink.c b/net/core/devlink.c
index f421cfb..78bb949 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -1571,6 +1571,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1];
 static const u8 devlink_perm_cfg_param_types[DEVLINK_PERM_CONFIG_MAX + 1] = {
 	[DEVLINK_PERM_CONFIG_SRIOV_ENABLED] = NLA_U8,
 	[DEVLINK_PERM_CONFIG_NUM_VF_PER_PF] = NLA_U32,
+	[DEVLINK_PERM_CONFIG_NUM_PF_MSIX_VECT] = NLA_U32,
 };
 
 static int devlink_nl_single_param_get(struct sk_buff *msg,
-- 
2.7.4

[PATCH net-next v5 05/10] devlink: Adding num MSI-X vectors per VF perm config param

[Steve Lin]

Adding DEVLINK_PERM_CONFIG_NUM_MSIX_VECT_PER_VF permanent config
parameter. Value is permanent, so becomes the new default value
for this device.

Value defines number of MSI-X vectors allocated per VF.

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

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 6936c0e..fe56a02 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -276,11 +276,15 @@ enum devlink_perm_config_enabled {
  * DEVLINK_PERM_CONFIG_NUM_PF_MSIX_VECT: Configures the # of MSI-X vectors
  * advertised via PCI capability for this device.
  *   Value = Max # of MSI-X vectors per PF
+ * DEVLINK_PERM_CONFIG_NUM_MSIX_VECT_PER_VF: Configures the # of MSI-X vectors
+ * advertised via PCI capability for each VF on this device.
+ *   # of MSI-X vectors per VF
  */
 enum devlink_perm_config_param {
 	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
 	DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
 	DEVLINK_PERM_CONFIG_NUM_PF_MSIX_VECT,
+	DEVLINK_PERM_CONFIG_NUM_MSIX_VECT_PER_VF,
 
 	__DEVLINK_PERM_CONFIG_MAX,
 	DEVLINK_PERM_CONFIG_MAX = __DEVLINK_PERM_CONFIG_MAX - 1
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 78bb949..f1f16c2 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -1572,6 +1572,7 @@ static const u8 devlink_perm_cfg_param_types[DEVLINK_PERM_CONFIG_MAX + 1] = {
 	[DEVLINK_PERM_CONFIG_SRIOV_ENABLED] = NLA_U8,
 	[DEVLINK_PERM_CONFIG_NUM_VF_PER_PF] = NLA_U32,
 	[DEVLINK_PERM_CONFIG_NUM_PF_MSIX_VECT] = NLA_U32,
+	[DEVLINK_PERM_CONFIG_NUM_MSIX_VECT_PER_VF] = NLA_U32,
 };
 
 static int devlink_nl_single_param_get(struct sk_buff *msg,
-- 
2.7.4

[PATCH net-next v5 06/10] bnxt: Add devlink support for config get/set

[Steve Lin]

Implements get and set of configuration parameters using new devlink
config get/set API. Parameters themselves defined in later patches.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 258 +++++++++++++++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  17 ++
 2 files changed, 263 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 402fa32..deb24e0 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -14,26 +14,260 @@
 #include "bnxt_vfr.h"
 #include "bnxt_devlink.h"
 
-static const 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 */
+struct bnxt_drv_cfgparam bnxt_drv_cfgparam_list[] = {
 };
 
-int bnxt_dl_register(struct bnxt *bp)
+#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 devlink *dl;
+	struct hwrm_nvm_get_variable_input req = {0};
+	dma_addr_t dest_data_dma_addr;
+	void *dest_data_addr = NULL;
+	int bytesize;
 	int rc;
 
-	if (!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV))
-		return 0;
+	bytesize = roundup(size, BITS_PER_BYTE) / BITS_PER_BYTE;
+	dest_data_addr = dma_alloc_coherent(&bp->pdev->dev, bytesize,
+					    &dest_data_dma_addr, GFP_KERNEL);
+	if (!dest_data_addr)
+		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};
+	dma_addr_t src_data_dma_addr;
+	void *src_data_addr = NULL;
+	int bytesize;
+	int rc;
+
+	bytesize = roundup(size, BITS_PER_BYTE) / BITS_PER_BYTE;
+
+	src_data_addr = dma_alloc_coherent(&bp->pdev->dev, bytesize,
+					   &src_data_dma_addr, GFP_KERNEL);
+	if (!src_data_addr)
+		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);
 
-	if (bp->hwrm_spec_code < 0x10803) {
-		netdev_warn(bp->dev, "Firmware does not support SR-IOV E-Switch SWITCHDEV mode.\n");
-		return -ENOTSUPP;
+	return 0;
+}
+
+static int bnxt_dl_perm_config_set(struct devlink *devlink,
+				   enum devlink_perm_config_param param,
+				   enum devlink_perm_config_type type,
+				   union devlink_perm_config_value *value,
+				   bool *restart_reqd)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
+	struct bnxt_drv_cfgparam *entry = NULL;
+	u32 value_int;
+	u32 bytesize;
+	int idx = 0;
+	int ret = 0;
+	u16 val16;
+	u8 val8;
+	int i;
+
+	*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 (!entry)
+		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;
 	}
 
+	bytesize = roundup(entry->bitlength, BITS_PER_BYTE) / BITS_PER_BYTE;
+
+	/* Type expected by device may or may not be the same as type passed
+	 * in from devlink API.  So first convert to an interval u32 value,
+	 * then to type needed by device.
+	 */
+	if (type == DEVLINK_PERM_CONFIG_TYPE_U8) {
+		value_int = value->value8;
+	} else if (type == DEVLINK_PERM_CONFIG_TYPE_U16) {
+		value_int = value->value16;
+	} else if (type == DEVLINK_PERM_CONFIG_TYPE_U32) {
+		value_int = value->value32;
+	} else {
+		/* Unsupported type */
+		return -EINVAL;
+	}
+
+	if (bytesize == 1) {
+		val8 = value_int;
+		ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val8,
+				     entry->bitlength);
+	} else if (bytesize == 2) {
+		val16 = value_int;
+		ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val16,
+				     entry->bitlength);
+	} else {
+		ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &value_int,
+				     entry->bitlength);
+	}
+
+	/* Restart required for all nvm parameter writes */
+	*restart_reqd = 1;
+
+	return ret;
+}
+
+static int bnxt_dl_perm_config_get(struct devlink *devlink,
+				   enum devlink_perm_config_param param,
+				   enum devlink_perm_config_type type,
+				   union devlink_perm_config_value *value)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
+	struct bnxt_drv_cfgparam *entry = NULL;
+	u32 value_int;
+	u32 bytesize;
+	int idx = 0;
+	int err = 0;
+	void *data;
+	u16 val16;
+	u8 val8;
+	int i;
+
+	/* 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 (!entry)
+		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;
+	}
+
+	/* Allocate space, retrieve value, and copy to result */
+	bytesize = roundup(entry->bitlength, BITS_PER_BYTE) / BITS_PER_BYTE;
+	data = kmalloc(bytesize, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	err = bnxt_nvm_read(bp, entry->nvm_param, idx, data, entry->bitlength);
+	if (err)
+		goto err_data;
+
+	/* Type returned by device may or may not be the same as type expected
+	 * by devlink API.  So first convert to an interval u32 value, then to
+	 * type expected by devlink.
+	 */
+	if (bytesize == 1) {
+		memcpy(&val8, data, sizeof(val8));
+		value_int = val8;
+	} else if (bytesize == 2) {
+		memcpy(&val16, data, sizeof(val16));
+		value_int = val16;
+	} else {
+		memcpy(&value_int, data, sizeof(value_int));
+	}
+
+	if (type == DEVLINK_PERM_CONFIG_TYPE_U8) {
+		value->value8 = value_int;
+	} else if (type == DEVLINK_PERM_CONFIG_TYPE_U16) {
+		value->value16 = value_int;
+	} else if (type == DEVLINK_PERM_CONFIG_TYPE_U32) {
+		value->value32 = value_int;
+	} else {
+		/* Unsupported type */
+		err = -EINVAL;
+	}
+
+err_data:
+	kfree(data);
+
+	return err;
+}
+
+static struct devlink_ops bnxt_dl_ops = {
+	.perm_config_get = bnxt_dl_perm_config_get,
+	.perm_config_set = bnxt_dl_perm_config_set,
+};
+
+int bnxt_dl_register(struct bnxt *bp)
+{
+	struct devlink *dl;
+	int rc;
+
+#ifdef CONFIG_BNXT_SRIOV
+	/* Add switchdev eswitch mode setting if SRIOV supported */
+	if (pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV) &&
+	    bp->hwrm_spec_code >= 0x10800) {
+		bnxt_dl_ops.eswitch_mode_set = bnxt_dl_eswitch_mode_set;
+		bnxt_dl_ops.eswitch_mode_get = bnxt_dl_eswitch_mode_get;
+	} else
+#endif /* CONFIG_BNXT_SRIOV */
+		netdev_warn(bp->dev, "SR-IOV E-Switch SWITCHDEV mode not supported.\n");
+
 	dl = devlink_alloc(&bnxt_dl_ops, sizeof(struct bnxt_dl));
 	if (!dl) {
 		netdev_warn(bp->dev, "devlink_alloc failed");
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 */
-- 
2.7.4

Re: [PATCH net-next v5 06/10] bnxt: Add devlink support for config get/set

[Jiri Pirko]

Mon, Oct 30, 2017 at 03:46:12PM CET, steven.lin1@broadcom.com wrote:
>Implements get and set of configuration parameters using new devlink
>config get/set API. Parameters themselves defined in later patches.
>
>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>Acked-by: Andy Gospodarek <gospo@broadcom.com>
>---
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 258 +++++++++++++++++++++-
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  17 ++
> 2 files changed, 263 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>index 402fa32..deb24e0 100644

[...]

>+static int bnxt_dl_perm_config_set(struct devlink *devlink,
>+				   enum devlink_perm_config_param param,
>+				   enum devlink_perm_config_type type,
>+				   union devlink_perm_config_value *value,
>+				   bool *restart_reqd)
>+{
>+	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
>+	struct bnxt_drv_cfgparam *entry = NULL;
>+	u32 value_int;
>+	u32 bytesize;
>+	int idx = 0;
>+	int ret = 0;
>+	u16 val16;
>+	u8 val8;
>+	int i;
>+
>+	*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 (!entry)
>+		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;
> 	}
> 
>+	bytesize = roundup(entry->bitlength, BITS_PER_BYTE) / BITS_PER_BYTE;
>+
>+	/* Type expected by device may or may not be the same as type passed
>+	 * in from devlink API.  So first convert to an interval u32 value,
>+	 * then to type needed by device.
>+	 */
>+	if (type == DEVLINK_PERM_CONFIG_TYPE_U8) {
>+		value_int = value->value8;
>+	} else if (type == DEVLINK_PERM_CONFIG_TYPE_U16) {
>+		value_int = value->value16;
>+	} else if (type == DEVLINK_PERM_CONFIG_TYPE_U32) {
>+		value_int = value->value32;
>+	} else {
>+		/* Unsupported type */
>+		return -EINVAL;
>+	}
>+
>+	if (bytesize == 1) {
>+		val8 = value_int;

>+		ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val8,
>+				     entry->bitlength);
>+	} else if (bytesize == 2) {
>+		val16 = value_int;
>+		ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val16,
>+				     entry->bitlength);
>+	} else {
>+		ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &value_int,
>+				     entry->bitlength);
>+	}
>+
>+	/* Restart required for all nvm parameter writes */
>+	*restart_reqd = 1;
>+
>+	return ret;
>+}

I don't like this swissknife approach for setters and getters. It's
messy, and hard to read. There should be clean get/set pair function
per parameter defined in array.
Something like:

static int bnxt_param_sriov_enabled_get(struct devlink *devlink, bool *p_value)
{
	...
}

static int bnxt_param_sriov_enabled_set(struct devlink *devlink, bool value,
					bool *restart_reqd)
{
	...
}

static int bnxt_param_num_vf_per_pf_get(struct devlink *devlink, u32 *p_value)
{
	...
}

static int bnxt_param_num_vf_per_pf_set(struct devlink *devlink, u32 value,
					bool *restart_reqd)
{
	...
}

static const struct devlink_config_param bnxt_params[] = {
	DEVLINK_CONFIG_PARAM_BOOL(DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
				  bnxt_param_sriov_enabled_get,
				  bnxt_param_sriov_enabled_set),
	DEVLINK_CONFIG_PARAM_U32(DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
				 bnxt_param_num_vf_per_pf_get,
				 bnxt_param_num_vf_per_pf_set),
};

and then in init:

err = devlink_config_param_register(devlink, bnxt_params,
				    ARRAY_SIZE(bnxt_params));

The register function will check types against the internal devlink
param->type table.

Also, the restart_reqd could be signalized by some pre-defined positive
setter return value.

Re: [PATCH net-next v5 06/10] bnxt: Add devlink support for config get/set

[Steve Lin]

On Mon, Oct 30, 2017 at 1:35 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Mon, Oct 30, 2017 at 03:46:12PM CET, steven.lin1@broadcom.com wrote:
>>Implements get and set of configuration parameters using new devlink
>>config get/set API. Parameters themselves defined in later patches.
>>
>>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>>Acked-by: Andy Gospodarek <gospo@broadcom.com>
>>---
>> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 258 +++++++++++++++++++++-
>> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  17 ++
>> 2 files changed, 263 insertions(+), 12 deletions(-)
>>
>>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>>index 402fa32..deb24e0 100644
>
> [...]
>
>>+static int bnxt_dl_perm_config_set(struct devlink *devlink,
>>+                                 enum devlink_perm_config_param param,
>>+                                 enum devlink_perm_config_type type,
>>+                                 union devlink_perm_config_value *value,
>>+                                 bool *restart_reqd)
>>+{
>>+      struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
>>+      struct bnxt_drv_cfgparam *entry = NULL;
>>+      u32 value_int;
>>+      u32 bytesize;
>>+      int idx = 0;
>>+      int ret = 0;
>>+      u16 val16;
>>+      u8 val8;
>>+      int i;
>>+
>>+      *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 (!entry)
>>+              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;
>>       }
>>
>>+      bytesize = roundup(entry->bitlength, BITS_PER_BYTE) / BITS_PER_BYTE;
>>+
>>+      /* Type expected by device may or may not be the same as type passed
>>+       * in from devlink API.  So first convert to an interval u32 value,
>>+       * then to type needed by device.
>>+       */
>>+      if (type == DEVLINK_PERM_CONFIG_TYPE_U8) {
>>+              value_int = value->value8;
>>+      } else if (type == DEVLINK_PERM_CONFIG_TYPE_U16) {
>>+              value_int = value->value16;
>>+      } else if (type == DEVLINK_PERM_CONFIG_TYPE_U32) {
>>+              value_int = value->value32;
>>+      } else {
>>+              /* Unsupported type */
>>+              return -EINVAL;
>>+      }
>>+
>>+      if (bytesize == 1) {
>>+              val8 = value_int;
>
>>+              ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val8,
>>+                                   entry->bitlength);
>>+      } else if (bytesize == 2) {
>>+              val16 = value_int;
>>+              ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val16,
>>+                                   entry->bitlength);
>>+      } else {
>>+              ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &value_int,
>>+                                   entry->bitlength);
>>+      }
>>+
>>+      /* Restart required for all nvm parameter writes */
>>+      *restart_reqd = 1;
>>+
>>+      return ret;
>>+}
>
> I don't like this swissknife approach for setters and getters. It's
> messy, and hard to read. There should be clean get/set pair function
> per parameter defined in array.

The advantage of the swiss kinfe approach is that you don't have a
large number of almost-identical functions (i.e. any "set" that
operates on a u32 will be doing essentially the exact same thing) in
the driver, which could lead to code bloat and also make it harder to
catch errors (since any bug that may creep in during the copy/paste to
make so many functions will only be caught when that specific
parameter is set, rather than when any parameter is set).

In general, I very much appreciate the thorough review.  I do feel,
though, like some of this basic architectural stuff could have been
addressed more efficiently in v1 or v2, rather than v5.  I admit that
when the comments to v4 included suggestions like removing extra
spaces in the commit message, I was thinking that we were close to
reaching consensus. :)

I just want to make sure that you feel that the overall goal of this
effort is worthwhile - I will make the changes you've suggested here
and in the other replies to v5, but I am hopeful that v6 does not
result in another set of big architectural changes.

> Something like:
>
> static int bnxt_param_sriov_enabled_get(struct devlink *devlink, bool *p_value)
> {
>         ...
> }
>
> static int bnxt_param_sriov_enabled_set(struct devlink *devlink, bool value,
>                                         bool *restart_reqd)
> {
>         ...
> }
>
> static int bnxt_param_num_vf_per_pf_get(struct devlink *devlink, u32 *p_value)
> {
>         ...
> }
>
> static int bnxt_param_num_vf_per_pf_set(struct devlink *devlink, u32 value,
>                                         bool *restart_reqd)
> {
>         ...
> }
>
> static const struct devlink_config_param bnxt_params[] = {
>         DEVLINK_CONFIG_PARAM_BOOL(DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
>                                   bnxt_param_sriov_enabled_get,
>                                   bnxt_param_sriov_enabled_set),
>         DEVLINK_CONFIG_PARAM_U32(DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
>                                  bnxt_param_num_vf_per_pf_get,
>                                  bnxt_param_num_vf_per_pf_set),
> };
>
> and then in init:
>
> err = devlink_config_param_register(devlink, bnxt_params,
>                                     ARRAY_SIZE(bnxt_params));
>
> The register function will check types against the internal devlink
> param->type table.
>
> Also, the restart_reqd could be signalized by some pre-defined positive
> setter return value.

Understood - like I said, I thought it preferable to use a global
getter/setter rather than one for each individual parameter, but I
will make the requested change, in hopes of reaching a conclusion
soon.

Re: [PATCH net-next v5 06/10] bnxt: Add devlink support for config get/set

[Jiri Pirko]

Mon, Oct 30, 2017 at 09:20:17PM CET, steven.lin1@broadcom.com wrote:
>On Mon, Oct 30, 2017 at 1:35 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Mon, Oct 30, 2017 at 03:46:12PM CET, steven.lin1@broadcom.com wrote:
>>>Implements get and set of configuration parameters using new devlink
>>>config get/set API. Parameters themselves defined in later patches.
>>>
>>>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>>>Acked-by: Andy Gospodarek <gospo@broadcom.com>
>>>---
>>> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 258 +++++++++++++++++++++-
>>> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  17 ++
>>> 2 files changed, 263 insertions(+), 12 deletions(-)
>>>
>>>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>>>index 402fa32..deb24e0 100644
>>
>> [...]
>>
>>>+static int bnxt_dl_perm_config_set(struct devlink *devlink,
>>>+                                 enum devlink_perm_config_param param,
>>>+                                 enum devlink_perm_config_type type,
>>>+                                 union devlink_perm_config_value *value,
>>>+                                 bool *restart_reqd)
>>>+{
>>>+      struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
>>>+      struct bnxt_drv_cfgparam *entry = NULL;
>>>+      u32 value_int;
>>>+      u32 bytesize;
>>>+      int idx = 0;
>>>+      int ret = 0;
>>>+      u16 val16;
>>>+      u8 val8;
>>>+      int i;
>>>+
>>>+      *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 (!entry)
>>>+              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;
>>>       }
>>>
>>>+      bytesize = roundup(entry->bitlength, BITS_PER_BYTE) / BITS_PER_BYTE;
>>>+
>>>+      /* Type expected by device may or may not be the same as type passed
>>>+       * in from devlink API.  So first convert to an interval u32 value,
>>>+       * then to type needed by device.
>>>+       */
>>>+      if (type == DEVLINK_PERM_CONFIG_TYPE_U8) {
>>>+              value_int = value->value8;
>>>+      } else if (type == DEVLINK_PERM_CONFIG_TYPE_U16) {
>>>+              value_int = value->value16;
>>>+      } else if (type == DEVLINK_PERM_CONFIG_TYPE_U32) {
>>>+              value_int = value->value32;
>>>+      } else {
>>>+              /* Unsupported type */
>>>+              return -EINVAL;
>>>+      }
>>>+
>>>+      if (bytesize == 1) {
>>>+              val8 = value_int;
>>
>>>+              ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val8,
>>>+                                   entry->bitlength);
>>>+      } else if (bytesize == 2) {
>>>+              val16 = value_int;
>>>+              ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val16,
>>>+                                   entry->bitlength);
>>>+      } else {
>>>+              ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &value_int,
>>>+                                   entry->bitlength);
>>>+      }
>>>+
>>>+      /* Restart required for all nvm parameter writes */
>>>+      *restart_reqd = 1;
>>>+
>>>+      return ret;
>>>+}
>>
>> I don't like this swissknife approach for setters and getters. It's
>> messy, and hard to read. There should be clean get/set pair function
>> per parameter defined in array.
>
>The advantage of the swiss kinfe approach is that you don't have a
>large number of almost-identical functions (i.e. any "set" that
>operates on a u32 will be doing essentially the exact same thing) in
>the driver, which could lead to code bloat and also make it harder to
>catch errors (since any bug that may creep in during the copy/paste to
>make so many functions will only be caught when that specific
>parameter is set, rather than when any parameter is set).

You can always do some common functions to do the work. But if gives the
driver freedom, unlike the swissknife approach.


>
>In general, I very much appreciate the thorough review.  I do feel,
>though, like some of this basic architectural stuff could have been
>addressed more efficiently in v1 or v2, rather than v5.  I admit that
>when the comments to v4 included suggestions like removing extra
>spaces in the commit message, I was thinking that we were close to
>reaching consensus. :)

Yeah, I noticed this just now.

>
>I just want to make sure that you feel that the overall goal of this
>effort is worthwhile - I will make the changes you've suggested here
>and in the other replies to v5, but I am hopeful that v6 does not
>result in another set of big architectural changes.

Hopefully.


>
>> Something like:
>>
>> static int bnxt_param_sriov_enabled_get(struct devlink *devlink, bool *p_value)
>> {
>>         ...
>> }
>>
>> static int bnxt_param_sriov_enabled_set(struct devlink *devlink, bool value,
>>                                         bool *restart_reqd)
>> {
>>         ...
>> }
>>
>> static int bnxt_param_num_vf_per_pf_get(struct devlink *devlink, u32 *p_value)
>> {
>>         ...
>> }
>>
>> static int bnxt_param_num_vf_per_pf_set(struct devlink *devlink, u32 value,
>>                                         bool *restart_reqd)
>> {
>>         ...
>> }
>>
>> static const struct devlink_config_param bnxt_params[] = {
>>         DEVLINK_CONFIG_PARAM_BOOL(DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
>>                                   bnxt_param_sriov_enabled_get,
>>                                   bnxt_param_sriov_enabled_set),
>>         DEVLINK_CONFIG_PARAM_U32(DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
>>                                  bnxt_param_num_vf_per_pf_get,
>>                                  bnxt_param_num_vf_per_pf_set),
>> };
>>
>> and then in init:
>>
>> err = devlink_config_param_register(devlink, bnxt_params,
>>                                     ARRAY_SIZE(bnxt_params));
>>
>> The register function will check types against the internal devlink
>> param->type table.
>>
>> Also, the restart_reqd could be signalized by some pre-defined positive
>> setter return value.
>
>Understood - like I said, I thought it preferable to use a global
>getter/setter rather than one for each individual parameter, but I
>will make the requested change, in hopes of reaching a conclusion
>soon.

[PATCH net-next v5 07/10] bnxt: Adding SR-IOV enablement permanent cfg param

[Steve Lin]

Adding permanent config parameter for SR-IOV enablement, using
devlink API for get/set operation.

DEVLINK_PERM_CONFIG_DISABLE = SR-IOV disabled
DEVLINK_PERM_CONFIG_ENABLE  = SR-IOV enabled

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index deb24e0..a2a4973 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -14,7 +14,14 @@
 #include "bnxt_vfr.h"
 #include "bnxt_devlink.h"
 
+/* Permanent config parameters from devlink.h:
+ * DEVLINK_PERM_CONFIG_SRIOV_ENABLED:
+ *   DEVLINK_PERM_CONFIG_DISABLE = disable SR-IOV
+ *   DEVLINK_PERM_CONFIG_ENABLE  = enable SR-IOV
+ */
 struct bnxt_drv_cfgparam bnxt_drv_cfgparam_list[] = {
+	{DEVLINK_PERM_CONFIG_SRIOV_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 1, 401},
 };
 
 #define BNXT_NUM_DRV_CFGPARAM ARRAY_SIZE(bnxt_drv_cfgparam_list)
-- 
2.7.4

[PATCH net-next v5 08/10] bnxt: Adding num VFs per PF perm config param

[Steve Lin]

Adding permanent config parameter for number of VFs per PF,
using devlink API for get/set operation.

Value sets the number of VFs per PF in SR-IOV mode.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index a2a4973..218d18d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -18,10 +18,14 @@
  * DEVLINK_PERM_CONFIG_SRIOV_ENABLED:
  *   DEVLINK_PERM_CONFIG_DISABLE = disable SR-IOV
  *   DEVLINK_PERM_CONFIG_ENABLE  = enable SR-IOV
+ * DEVLINK_PERM_CONFIG_NUM_VF_PER_PF:
+ *   # of VFs per PF in SR-IOV mode
  */
 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},
 };
 
 #define BNXT_NUM_DRV_CFGPARAM ARRAY_SIZE(bnxt_drv_cfgparam_list)
-- 
2.7.4

[PATCH net-next v5 09/10] bnxt: Adding max PF MSI-X vectors perm config param

[Steve Lin]

Adding permanent config parameter for maximum number of PF
MSI-X vectors.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 218d18d..ec0f57f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -20,12 +20,16 @@
  *   DEVLINK_PERM_CONFIG_ENABLE  = enable SR-IOV
  * DEVLINK_PERM_CONFIG_NUM_VF_PER_PF:
  *   # of VFs per PF in SR-IOV mode
+ * DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT:
+ *   Max # of MSI-X vectors per PF
  */
 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_NUM_PF_MSIX_VECT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 10, 108},
 };
 
 #define BNXT_NUM_DRV_CFGPARAM ARRAY_SIZE(bnxt_drv_cfgparam_list)
-- 
2.7.4

[PATCH net-next v5 10/10] bnxt: Adding num MSI-X vectors per VF perm config param

[Steve Lin]

Adding permanent config parameter for number MSI-X vectors
per VF, using devlink API for get/set operation.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index ec0f57f..0ea8d95 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -22,6 +22,8 @@
  *   # of VFs per PF in SR-IOV mode
  * DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT:
  *   Max # of MSI-X vectors per PF
+ * DEVLINK_PERM_CONFIG_NUM_MSIX_VECT_PER_VF:
+ *   # of MSI-X vectors per VF
  */
 struct bnxt_drv_cfgparam bnxt_drv_cfgparam_list[] = {
 	{DEVLINK_PERM_CONFIG_SRIOV_ENABLED, BNXT_DRV_PF,
@@ -30,6 +32,8 @@ struct bnxt_drv_cfgparam bnxt_drv_cfgparam_list[] = {
 		BNXT_DRV_APPL_FUNCTION, 8, 404},
 	{DEVLINK_PERM_CONFIG_NUM_PF_MSIX_VECT, BNXT_DRV_PF,
 		BNXT_DRV_APPL_SHARED, 10, 108},
+	{DEVLINK_PERM_CONFIG_NUM_MSIX_VECT_PER_VF, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 10, 406},
 };
 
 #define BNXT_NUM_DRV_CFGPARAM ARRAY_SIZE(bnxt_drv_cfgparam_list)
-- 
2.7.4