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

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

[Steve Lin]

Changes since v3:

* Using union instead of void * to pass parameter values to/from drivers.
* Updated parameter comments / naming and added enum for enable/disable
  to add clarity.
* Various code cleanup in bnxt:
  - change hwrm call to use mutex protected version
  - use roundup() macro
  - remove unnecessary error messages
  - change mechanism to check for parameter-not-found
  - cleanup merge w/ switchdev eswitch devlink functions
  - rebased with Michael Chan's recent bnxt changes in net-next.

Suggested changes not implemented:

* re: val8 = val32 "Don't you need explicit castings for these kind of
  assignments to prevent warnings?" -- No, I don't think explicit castings
  are necessary here; I don't get any compiler warnings (w/ gcc 5.4.0)

[PATCH net-next v4 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        |   7 ++
 include/uapi/linux/devlink.h |  25 ++++
 net/core/devlink.c           | 287 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 319 insertions(+)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index b9654e1..c7dd912 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -270,6 +270,13 @@ 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, u8 type,
+			       union devlink_perm_config_value *value);
+	int (*perm_config_set)(struct devlink *devlink,
+			       enum devlink_perm_config_param param, u8 type,
+			       union devlink_perm_config_value *value,
+			       u8 *restart_reqd);
 };
 
 static inline void *devlink_priv(struct devlink *devlink)
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 0cbca96..b3a0b2a 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,	/* u8 */
+
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
@@ -244,4 +256,17 @@ 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 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..a7fa7cc 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -1566,6 +1566,277 @@ 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,
+				       u32 param, u8 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 NLA_U8:
+		if (nla_put_u8(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE,
+			       value.value8))
+			goto nest_err;
+		break;
+	case NLA_U16:
+		if (nla_put_u16(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE,
+				value.value16))
+			goto nest_err;
+		break;
+	case NLA_U32:
+		if (nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE,
+				value.value32))
+			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];
+	struct nlattr *cfgparam_attr;
+	struct nlattr *attr;
+	void *hdr;
+	u32 param;
+	int err;
+	int rem;
+	u8 type;
+
+	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,
+				       u32 param, u8 type,
+				       union devlink_perm_config_value *value)
+{
+	const struct devlink_ops *ops = devlink->ops;
+	struct nlattr *cfgparam_attr;
+	u8 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_u8(msg,
+				 DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED, 1);
+		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;
+	struct nlattr *cfgparam_attr;
+	struct sk_buff *msg;
+	struct nlattr *attr;
+	void *hdr;
+	u32 param;
+	u8 type;
+	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 NLA_U8:
+			value.value8 =
+				nla_get_u8(tb[DEVLINK_ATTR_PERM_CONFIG_VALUE]);
+			break;
+		case NLA_U16:
+			value.value16 =
+				nla_get_u16(tb[DEVLINK_ATTR_PERM_CONFIG_VALUE]);
+			break;
+		case NLA_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 +2562,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 +2724,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

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

[Steve Lin]

Adding DEVLINK_PERM_CONFIG_SRIOV_ENABLED permanent config
parameter.  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 | 14 +++++++++++++-
 net/core/devlink.c           |  1 +
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index b3a0b2a..9a41f6e 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -256,8 +256,20 @@ enum devlink_dpipe_header_id {
 	DEVLINK_DPIPE_HEADER_IPV6,
 };
 
-/* Permanent config parameters */
+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
+ * provided by 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 a7fa7cc..395c93c 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 v4 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 9a41f6e..2c714fb 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -266,9 +266,13 @@ enum devlink_perm_config_enabled {
  * provided by device.
  *   DEVLINK_PERM_CONFIG_DISABLE = disable SR-IOV
  *   DEVLINK_PERM_CONFIG_ENABLE  = enable SR-IOV
+ * DEVLINK_PERM_CONFIG_NUM_VF_PER_PF: Configures 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 395c93c..4b1fff3 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 v4 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 2c714fb..2fd0265 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -269,10 +269,14 @@ enum devlink_perm_config_enabled {
  * DEVLINK_PERM_CONFIG_NUM_VF_PER_PF: Configures 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 # 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 4b1fff3..f970d03 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 v4 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 2fd0265..50000a0 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -272,11 +272,15 @@ enum devlink_perm_config_enabled {
  * DEVLINK_PERM_CONFIG_NUM_PF_MSIX_VECT: Configures # 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 # 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 f970d03..226d151 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 v4 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..8a6037f 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,
+				   u8 type,
+				   union devlink_perm_config_value *value,
+				   u8 *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 == NLA_U8) {
+		value_int = value->value8;
+	} else if (type == NLA_U16) {
+		value_int = value->value16;
+	} else if (type == NLA_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,
+				   u8 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 == NLA_U8) {
+		value->value8 = value_int;
+	} else if (type == NLA_U16) {
+		value->value16 = value_int;
+	} else if (type == NLA_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

[PATCH net-next v4 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 8a6037f..9f168a8 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 v4 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 9f168a8..0812221 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 v4 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 0812221..047f09e 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 v4 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 047f09e..555e098 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

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

[Jiri Pirko]

Fri, Oct 27, 2017 at 10:54:05PM CEST, 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>
>---
> include/net/devlink.h        |   7 ++
> include/uapi/linux/devlink.h |  25 ++++
> net/core/devlink.c           | 287 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 319 insertions(+)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index b9654e1..c7dd912 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -270,6 +270,13 @@ 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, u8 type,
>+			       union devlink_perm_config_value *value);
>+	int (*perm_config_set)(struct devlink *devlink,
>+			       enum devlink_perm_config_param param, u8 type,
>+			       union devlink_perm_config_value *value,
>+			       u8 *restart_reqd);

type should be enum and restart_reqd should be bool.

[...]	
	
	
>+static int devlink_nl_single_param_set(struct sk_buff *msg,
>+				       struct devlink *devlink,
>+				       u32 param, u8 type,
>+				       union devlink_perm_config_value *value)
>+{
>+	const struct devlink_ops *ops = devlink->ops;
>+	struct nlattr *cfgparam_attr;
>+	u8 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_u8(msg,
>+				 DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED, 1);

Why don't you just put this flag always? Otherwise it could be NLA_FLAG


>+		if (err)
>+			goto nest_fail;
>+	}
>+

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

[Jiri Pirko]

Fri, Oct 27, 2017 at 10:54:06PM CEST, steven.lin1@broadcom.com wrote:
>Adding DEVLINK_PERM_CONFIG_SRIOV_ENABLED permanent config
>parameter.  Value is permanent, so becomes the new default

Avoid the double space.


>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 | 14 +++++++++++++-
> net/core/devlink.c           |  1 +
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index b3a0b2a..9a41f6e 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -256,8 +256,20 @@ enum devlink_dpipe_header_id {
> 	DEVLINK_DPIPE_HEADER_IPV6,
> };
> 
>-/* Permanent config parameters */
>+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
>+ * provided by device.

I don't understand the sentense :/


>+ *   DEVLINK_PERM_CONFIG_DISABLE = disable SR-IOV
>+ *   DEVLINK_PERM_CONFIG_ENABLE  = enable SR-IOV

These comments should be at the enum values, not here.


>+ */
> 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 a7fa7cc..395c93c 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 v4 01/10] devlink: Add permanent config parameter get/set operations

[Steve Lin]

On Fri, Oct 27, 2017 at 5:04 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Oct 27, 2017 at 10:54:05PM CEST, 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>
>>---
>> include/net/devlink.h        |   7 ++
>> include/uapi/linux/devlink.h |  25 ++++
>> net/core/devlink.c           | 287 +++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 319 insertions(+)
>>
>>diff --git a/include/net/devlink.h b/include/net/devlink.h
>>index b9654e1..c7dd912 100644
>>--- a/include/net/devlink.h
>>+++ b/include/net/devlink.h
>>@@ -270,6 +270,13 @@ 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, u8 type,
>>+                             union devlink_perm_config_value *value);
>>+      int (*perm_config_set)(struct devlink *devlink,
>>+                             enum devlink_perm_config_param param, u8 type,
>>+                             union devlink_perm_config_value *value,
>>+                             u8 *restart_reqd);
>
> type should be enum and restart_reqd should be bool.

Throughout devlink/netlink code, NLA_* types are not a named enum;
they're defined like this in netlink.h:

enum {
        NLA_UNSPEC,
        NLA_U8,
        NLA_U16,
        NLA_U32,
        .... additional types follow ....
};

and are, I believe referred to as u8 or u16 in the code.  I could
define a new enum specifically for the config get/set types, but that
seems duplicative with the NLA_* types already defined - is that what
you're suggesting?

I will change restart_reqd to be bool.

>
> [...]
>
>
>>+static int devlink_nl_single_param_set(struct sk_buff *msg,
>>+                                     struct devlink *devlink,
>>+                                     u32 param, u8 type,
>>+                                     union devlink_perm_config_value *value)
>>+{
>>+      const struct devlink_ops *ops = devlink->ops;
>>+      struct nlattr *cfgparam_attr;
>>+      u8 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_u8(msg,
>>+                               DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED, 1);
>
> Why don't you just put this flag always? Otherwise it could be NLA_FLAG

Ok, I will change to NLA_FLAG.

Thanks,
Steve

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

[Steve Lin]

On Fri, Oct 27, 2017 at 5:06 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Oct 27, 2017 at 10:54:06PM CEST, steven.lin1@broadcom.com wrote:
>>Adding DEVLINK_PERM_CONFIG_SRIOV_ENABLED permanent config
>>parameter.  Value is permanent, so becomes the new default
>
> Avoid the double space.

Ok.

>
>
>>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 | 14 +++++++++++++-
>> net/core/devlink.c           |  1 +
>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>
>>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>>index b3a0b2a..9a41f6e 100644
>>--- a/include/uapi/linux/devlink.h
>>+++ b/include/uapi/linux/devlink.h
>>@@ -256,8 +256,20 @@ enum devlink_dpipe_header_id {
>>       DEVLINK_DPIPE_HEADER_IPV6,
>> };
>>
>>-/* Permanent config parameters */
>>+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
>>+ * provided by device.
>
> I don't understand the sentense :/

It means that this parameter controls whether the device advertises
SR-IOV PCI capability -- perhaps I should say "advertised" rather than
"provided"?  Ok, will do that...

>
>
>>+ *   DEVLINK_PERM_CONFIG_DISABLE = disable SR-IOV
>>+ *   DEVLINK_PERM_CONFIG_ENABLE  = enable SR-IOV
>
> These comments should be at the enum values, not here.

Ok.

>
>
>>+ */
>> 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 a7fa7cc..395c93c 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 v4 01/10] devlink: Add permanent config parameter get/set operations

[Jiri Pirko]

Fri, Oct 27, 2017 at 11:26:52PM CEST, steven.lin1@broadcom.com wrote:
>On Fri, Oct 27, 2017 at 5:04 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Fri, Oct 27, 2017 at 10:54:05PM CEST, 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>
>>>---
>>> include/net/devlink.h        |   7 ++
>>> include/uapi/linux/devlink.h |  25 ++++
>>> net/core/devlink.c           | 287 +++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 319 insertions(+)
>>>
>>>diff --git a/include/net/devlink.h b/include/net/devlink.h
>>>index b9654e1..c7dd912 100644
>>>--- a/include/net/devlink.h
>>>+++ b/include/net/devlink.h
>>>@@ -270,6 +270,13 @@ 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, u8 type,
>>>+                             union devlink_perm_config_value *value);
>>>+      int (*perm_config_set)(struct devlink *devlink,
>>>+                             enum devlink_perm_config_param param, u8 type,
>>>+                             union devlink_perm_config_value *value,
>>>+                             u8 *restart_reqd);
>>
>> type should be enum and restart_reqd should be bool.
>
>Throughout devlink/netlink code, NLA_* types are not a named enum;
>they're defined like this in netlink.h:
>
>enum {
>        NLA_UNSPEC,
>        NLA_U8,
>        NLA_U16,
>        NLA_U32,
>        .... additional types follow ....
>};
>
>and are, I believe referred to as u8 or u16 in the code.  I could
>define a new enum specifically for the config get/set types, but that
>seems duplicative with the NLA_* types already defined - is that what
>you're suggesting?

Yes, and that should be aligned with union devlink_perm_config_value.


>
>I will change restart_reqd to be bool.
>
>>
>> [...]
>>
>>
>>>+static int devlink_nl_single_param_set(struct sk_buff *msg,
>>>+                                     struct devlink *devlink,
>>>+                                     u32 param, u8 type,
>>>+                                     union devlink_perm_config_value *value)
>>>+{
>>>+      const struct devlink_ops *ops = devlink->ops;
>>>+      struct nlattr *cfgparam_attr;
>>>+      u8 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_u8(msg,
>>>+                               DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED, 1);
>>
>> Why don't you just put this flag always? Otherwise it could be NLA_FLAG
>
>Ok, I will change to NLA_FLAG.
>
>Thanks,
>Steve

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

[Jiri Pirko]

Fri, Oct 27, 2017 at 11:30:06PM CEST, steven.lin1@broadcom.com wrote:
>On Fri, Oct 27, 2017 at 5:06 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Fri, Oct 27, 2017 at 10:54:06PM CEST, steven.lin1@broadcom.com wrote:
>>>Adding DEVLINK_PERM_CONFIG_SRIOV_ENABLED permanent config
>>>parameter.  Value is permanent, so becomes the new default
>>
>> Avoid the double space.
>
>Ok.
>
>>
>>
>>>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 | 14 +++++++++++++-
>>> net/core/devlink.c           |  1 +
>>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>>
>>>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>>>index b3a0b2a..9a41f6e 100644
>>>--- a/include/uapi/linux/devlink.h
>>>+++ b/include/uapi/linux/devlink.h
>>>@@ -256,8 +256,20 @@ enum devlink_dpipe_header_id {
>>>       DEVLINK_DPIPE_HEADER_IPV6,
>>> };
>>>
>>>-/* Permanent config parameters */
>>>+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
>>>+ * provided by device.
>>
>> I don't understand the sentense :/
>
>It means that this parameter controls whether the device advertises
>SR-IOV PCI capability -- perhaps I should say "advertised" rather than
>"provided"?  Ok, will do that...

I'm missing "is" most probably.

>
>>
>>
>>>+ *   DEVLINK_PERM_CONFIG_DISABLE = disable SR-IOV
>>>+ *   DEVLINK_PERM_CONFIG_ENABLE  = enable SR-IOV
>>
>> These comments should be at the enum values, not here.
>
>Ok.
>
>>
>>
>>>+ */
>>> 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 a7fa7cc..395c93c 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
>>>