[PATCH 0/7] Adding permanent config get/set to devlink

[PATCH 0/7] Adding permanent config get/set to devlink

[Steve Lin]

DIFFERENCES FROM RFC:
Implemented most of the changes suggested by Jiri and others.
Thanks for the valuable feedback!

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

bnxt driver patches make use of these new devlink cmds/
attributes.

Steve Lin (7):
  devlink: Add permanent config parameter get/set operations
  devlink: Adding NPAR permanent config parameters
  devlink: Adding high level dev perm config params
  devlink: Adding perm config of link settings
  devlink: Adding pre-boot permanent config parameters
  bnxt: Move generic devlink code to new file
  bnxt: Add devlink support for config get/set

 drivers/net/ethernet/broadcom/bnxt/Makefile       |   2 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt.c         |   1 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 363 ++++++++++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  56 ++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h     | 100 ++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c     |  53 +---
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h     |  37 +--
 include/net/devlink.h                             |   4 +
 include/uapi/linux/devlink.h                      | 113 +++++++
 net/core/devlink.c                                | 300 ++++++++++++++++++
 10 files changed, 944 insertions(+), 85 deletions(-)
 create mode 100644 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
 create mode 100644 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h

-- 
2.7.4

[PATCH 1/7] devlink: Add permanent config parameter get/set operations

[Steve Lin]

Add support for permanent config parameter get/set commands. Used
for parameters held in NVRAM, persistent device configuration.
The config_get() and config_set() operations operate as expected, but
note that the driver implementation of the config_set() operation can
indicate whether a restart is necessary for the setting to take
effect.  This indication of a necessary restart is passed via the
DEVLINK_ATTR_PERM_CFG_RESTART_REQUIRED attribute.

First set of parameters defined are PCI SR-IOV and per-VF
configuration:

DEVLINK_ATTR_PERM_CFG_SRIOV_ENABLED: Enable SR-IOV capability.
DEVLINK_ATTR_PERM_CFG_NUM_VF_PER_PF: Maximum number of VFs per PF, in
SR-IOV mode.
DEVLINK_ATTR_PERM_CFG_MAX_NUM_PF_MSIX_VECT: Maximum number of
MSI-X vectors assigned per PF.
DEVLINK_ATTR_PERM_CFG_MSIX_VECTORS_PER_VF: 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/net/devlink.h        |   4 +
 include/uapi/linux/devlink.h |  20 ++++
 net/core/devlink.c           | 266 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 290 insertions(+)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index b9654e1..952966c 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -270,6 +270,10 @@ 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 (*config_get)(struct devlink *devlink, enum devlink_attr attr,
+			  u32 *value);
+	int (*config_set)(struct devlink *devlink, enum devlink_attr attr,
+			  u32 value, u8 *restart_reqd);
 };
 
 static inline void *devlink_priv(struct devlink *devlink)
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 0cbca96..34de44d 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -70,6 +70,9 @@ enum devlink_command {
 	DEVLINK_CMD_DPIPE_HEADERS_GET,
 	DEVLINK_CMD_DPIPE_TABLE_COUNTERS_SET,
 
+	DEVLINK_CMD_CONFIG_GET,
+	DEVLINK_CMD_CONFIG_SET,
+
 	/* add new commands above here */
 	__DEVLINK_CMD_MAX,
 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
@@ -202,6 +205,23 @@ enum devlink_attr {
 
 	DEVLINK_ATTR_ESWITCH_ENCAP_MODE,	/* u8 */
 
+	/* Permanent Configuration Parameters */
+	DEVLINK_ATTR_PERM_CFG,				/* nested */
+
+	/* When config doesn't take effect until next reboot (config
+	 * just changed NVM which isn't read until boot, for example),
+	 * this attribute should be set by the driver.
+	 */
+	DEVLINK_ATTR_PERM_CFG_RESTART_REQUIRED,		/* u8 */
+	DEVLINK_ATTR_PERM_CFG_SRIOV_ENABLED,		/* u8 */
+	DEVLINK_ATTR_PERM_CFG_FIRST = DEVLINK_ATTR_PERM_CFG_SRIOV_ENABLED,
+	DEVLINK_ATTR_PERM_CFG_NUM_VF_PER_PF,		/* u32 */
+	DEVLINK_ATTR_PERM_CFG_MAX_NUM_PF_MSIX_VECT,	/* u32 */
+	DEVLINK_ATTR_PERM_CFG_MSIX_VECTORS_PER_VF,	/* u32 */
+
+	/* Add new permanent config parameters above here */
+	DEVLINK_ATTR_PERM_CFG_LAST = DEVLINK_ATTR_PERM_CFG_MSIX_VECTORS_PER_VF,
+
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 7d430c1..427a65e 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -1566,6 +1566,254 @@ static int devlink_nl_cmd_eswitch_set_doit(struct sk_buff *skb,
 	return 0;
 }
 
+static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1];
+
+static int devlink_nl_sing_param_get(struct sk_buff *msg,
+				     struct devlink *devlink,
+				     enum devlink_attr attr)
+{
+	struct nla_policy policy;
+	u32 value;
+	int err;
+	const struct devlink_ops *ops = devlink->ops;
+
+	policy = devlink_nl_policy[attr];
+	err = ops->config_get(devlink, attr, &value);
+	if (err)
+		return err;
+
+	switch (policy.type) {
+	case NLA_U8:
+		err = nla_put_u8(msg, attr, value);
+		break;
+	case NLA_U16:
+		err = nla_put_u16(msg, attr, value);
+		break;
+	case NLA_U32:
+		err = nla_put_u32(msg, attr, value);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int devlink_nl_config_get_fill(struct sk_buff *msg,
+				      struct devlink *devlink,
+				      enum devlink_command cmd,
+				      struct genl_info *info)
+{
+	const struct devlink_ops *ops = devlink->ops;
+	void *hdr;
+	int err;
+	enum devlink_attr attr;
+	int param_count = 0;
+	struct nlattr *cfgparam_attr;
+
+	if (!ops->config_get)
+		return -EOPNOTSUPP;
+
+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
+			  &devlink_nl_family, 0, cmd);
+	if (!hdr) {
+		err = -EMSGSIZE;
+		goto nla_msg_failure;
+	}
+
+	err = devlink_nl_put_handle(msg, devlink);
+	if (err)
+		goto nla_put_failure;
+
+	cfgparam_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CFG);
+
+	/* Were specific parameter(s) requested? */
+	for (attr = DEVLINK_ATTR_PERM_CFG_FIRST;
+	     attr <= DEVLINK_ATTR_PERM_CFG_LAST; attr++) {
+		if (info->attrs[attr]) {
+			err = devlink_nl_sing_param_get(msg, devlink, attr);
+			if (err)
+				goto nla_nest_failure;
+			param_count++;
+			break;
+		}
+	}
+
+	if (param_count == 0) {
+		/* return all parameter values */
+		for (attr = DEVLINK_ATTR_PERM_CFG_FIRST;
+		     attr <= DEVLINK_ATTR_PERM_CFG_LAST; attr++) {
+			err = devlink_nl_sing_param_get(msg, devlink, attr);
+			if (err)
+				goto nla_nest_failure;
+		}
+	}
+	nla_nest_end(msg, cfgparam_attr);
+
+	genlmsg_end(msg, hdr);
+	return 0;
+
+nla_nest_failure:
+	nla_nest_cancel(msg, cfgparam_attr);
+nla_put_failure:
+	genlmsg_cancel(msg, hdr);
+nla_msg_failure:
+	return err;
+}
+
+static int devlink_nl_cmd_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)
+		return -EOPNOTSUPP;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	err = devlink_nl_config_get_fill(msg, devlink, DEVLINK_CMD_CONFIG_GET,
+					 info);
+
+	if (err) {
+		nlmsg_free(msg);
+		return err;
+	}
+
+	return genlmsg_reply(msg, info);
+}
+
+static int devlink_nl_sing_param_set(struct sk_buff *msg,
+				     struct devlink *devlink,
+				     struct genl_info *info,
+				     enum devlink_attr attr,
+				     u8 *restart_reqd)
+{
+	struct nla_policy policy;
+	u32 orig_value;
+	u32 new_value;
+	u8 need_restart;
+	u8 *val8;
+	u16 *val16;
+	u32 *val32;
+	int err;
+	const struct devlink_ops *ops = devlink->ops;
+
+	policy = devlink_nl_policy[attr];
+
+	/* First get current value of parameter */
+	err = ops->config_get(devlink, attr, &orig_value);
+	if (err)
+		return err;
+
+	/* Now set parameter */
+	switch (policy.type) {
+	case NLA_U8:
+		val8 = nla_data(info->attrs[attr]);
+		new_value = *val8;
+		break;
+	case NLA_U16:
+		val16 = nla_data(info->attrs[attr]);
+		new_value = *val16;
+		break;
+	case NLA_U32:
+		val32 = nla_data(info->attrs[attr]);
+		new_value = *val32;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	err = ops->config_set(devlink, attr, new_value, &need_restart);
+	if (err)
+		return err;
+
+	/* Update restart reqd - if any param needs restart, should be set */
+	*restart_reqd |= need_restart;
+
+	/* Since set was successful, write attr back to msg with orig val */
+	switch (policy.type) {
+	case NLA_U8:
+		err = nla_put_u8(msg, attr, orig_value);
+		break;
+	case NLA_U16:
+		err = nla_put_u16(msg, attr, orig_value);
+		break;
+	case NLA_U32:
+		err = nla_put_u32(msg, attr, orig_value);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int devlink_nl_cmd_config_set_doit(struct sk_buff *skb,
+					  struct genl_info *info)
+{
+	struct devlink *devlink = info->user_ptr[0];
+	struct sk_buff *msg;
+	void *hdr;
+	enum devlink_attr attr;
+	int err;
+	u8 restart_reqd = 0;
+	struct nlattr *cfgparam_attr;
+
+	if (!devlink->ops || !devlink->ops->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_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_CFG);
+
+	/* Find attribute(s) being set */
+	for (attr = DEVLINK_ATTR_PERM_CFG_FIRST;
+	     attr <= DEVLINK_ATTR_PERM_CFG_LAST; attr++) {
+		if (info->attrs[attr]) {
+			err = devlink_nl_sing_param_set(msg, devlink, info,
+							attr, &restart_reqd);
+			if (err)
+				goto nla_nest_failure;
+		}
+	}
+
+	nla_nest_end(msg, cfgparam_attr);
+
+	if (restart_reqd) {
+		err = nla_put_u8(msg, DEVLINK_ATTR_PERM_CFG_RESTART_REQUIRED,
+				 restart_reqd);
+		if (err)
+			goto nla_put_failure;
+	}
+
+	genlmsg_end(msg, hdr);
+	return genlmsg_reply(msg, info);
+
+nla_nest_failure:
+	nla_nest_cancel(msg, cfgparam_attr);
+nla_put_failure:
+	genlmsg_cancel(msg, hdr);
+nla_msg_failure:
+	return err;
+}
+
 int devlink_dpipe_match_put(struct sk_buff *skb,
 			    struct devlink_dpipe_match *match)
 {
@@ -2291,6 +2539,10 @@ 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_CFG_SRIOV_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PERM_CFG_NUM_VF_PER_PF] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CFG_MAX_NUM_PF_MSIX_VECT] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CFG_MSIX_VECTORS_PER_VF] = { .type = NLA_U32 },
 };
 
 static const struct genl_ops devlink_nl_ops[] = {
@@ -2451,6 +2703,20 @@ static const struct genl_ops devlink_nl_ops[] = {
 		.flags = GENL_ADMIN_PERM,
 		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
 	},
+	{
+		.cmd = DEVLINK_CMD_CONFIG_GET,
+		.doit = devlink_nl_cmd_config_get_doit,
+		.policy = devlink_nl_policy,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
+	},
+	{
+		.cmd = DEVLINK_CMD_CONFIG_SET,
+		.doit = devlink_nl_cmd_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 1/7] devlink: Add permanent config parameter get/set operations

[Jiri Pirko]

Tue, Oct 17, 2017 at 10:44:23PM CEST, steven.lin1@broadcom.com wrote:
>Add support for permanent config parameter get/set commands. Used
>for parameters held in NVRAM, persistent device configuration.
>The config_get() and config_set() operations operate as expected, but
>note that the driver implementation of the config_set() operation can
>indicate whether a restart is necessary for the setting to take
>effect.  This indication of a necessary restart is passed via the
>DEVLINK_ATTR_PERM_CFG_RESTART_REQUIRED attribute.
>
>First set of parameters defined are PCI SR-IOV and per-VF
>configuration:
>
>DEVLINK_ATTR_PERM_CFG_SRIOV_ENABLED: Enable SR-IOV capability.
>DEVLINK_ATTR_PERM_CFG_NUM_VF_PER_PF: Maximum number of VFs per PF, in
>SR-IOV mode.
>DEVLINK_ATTR_PERM_CFG_MAX_NUM_PF_MSIX_VECT: Maximum number of
>MSI-X vectors assigned per PF.
>DEVLINK_ATTR_PERM_CFG_MSIX_VECTORS_PER_VF: 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/net/devlink.h        |   4 +
> include/uapi/linux/devlink.h |  20 ++++
> net/core/devlink.c           | 266 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 290 insertions(+)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index b9654e1..952966c 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -270,6 +270,10 @@ 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 (*config_get)(struct devlink *devlink, enum devlink_attr attr,
>+			  u32 *value);
>+	int (*config_set)(struct devlink *devlink, enum devlink_attr attr,
>+			  u32 value, u8 *restart_reqd);
> };
> 
> static inline void *devlink_priv(struct devlink *devlink)
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index 0cbca96..34de44d 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -70,6 +70,9 @@ enum devlink_command {
> 	DEVLINK_CMD_DPIPE_HEADERS_GET,
> 	DEVLINK_CMD_DPIPE_TABLE_COUNTERS_SET,
> 
>+	DEVLINK_CMD_CONFIG_GET,
>+	DEVLINK_CMD_CONFIG_SET,
>+
> 	/* add new commands above here */
> 	__DEVLINK_CMD_MAX,
> 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
>@@ -202,6 +205,23 @@ enum devlink_attr {
> 
> 	DEVLINK_ATTR_ESWITCH_ENCAP_MODE,	/* u8 */
> 
>+	/* Permanent Configuration Parameters */
>+	DEVLINK_ATTR_PERM_CFG,				/* nested */
>+
>+	/* When config doesn't take effect until next reboot (config
>+	 * just changed NVM which isn't read until boot, for example),
>+	 * this attribute should be set by the driver.
>+	 */
>+	DEVLINK_ATTR_PERM_CFG_RESTART_REQUIRED,		/* u8 */
>+	DEVLINK_ATTR_PERM_CFG_SRIOV_ENABLED,		/* u8 */
>+	DEVLINK_ATTR_PERM_CFG_FIRST = DEVLINK_ATTR_PERM_CFG_SRIOV_ENABLED,
>+	DEVLINK_ATTR_PERM_CFG_NUM_VF_PER_PF,		/* u32 */
>+	DEVLINK_ATTR_PERM_CFG_MAX_NUM_PF_MSIX_VECT,	/* u32 */
>+	DEVLINK_ATTR_PERM_CFG_MSIX_VECTORS_PER_VF,	/* u32 */

Steve. As I originally requested, could you please split this to:
1) single patch adding config get/set commands, without any config attributes
2) single patch per config attribute - please don't add them in bulk.
   We also need very strict description for every single attribute so
   other vendors know what it is and can re-use it. There is need to
   avoid duplication here. Also, please send just few attribites in the
   first run, not like 40 you are sending now. Impossible to review.

Also, why didn't you put it into nested attribute we were discussing?


>+
>+	/* Add new permanent config parameters above here */
>+	DEVLINK_ATTR_PERM_CFG_LAST = DEVLINK_ATTR_PERM_CFG_MSIX_VECTORS_PER_VF,

Yeah, this is odd, it replaces nested attribute aproach.


>+
> 	/* add new attributes above here, update the policy in devlink.c */
> 
> 	__DEVLINK_ATTR_MAX,
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 7d430c1..427a65e 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -1566,6 +1566,254 @@ static int devlink_nl_cmd_eswitch_set_doit(struct sk_buff *skb,
> 	return 0;
> }
> 
>+static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1];
>+
>+static int devlink_nl_sing_param_get(struct sk_buff *msg,

I was wondering what song it will sing :) Just add "le", it's just 2
chars :)

Re: [PATCH 1/7] devlink: Add permanent config parameter get/set operations

[Steve Lin]

On Wed, Oct 18, 2017 at 3:11 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Tue, Oct 17, 2017 at 10:44:23PM CEST, steven.lin1@broadcom.com wrote:
> Steve. As I originally requested, could you please split this to:
> 1) single patch adding config get/set commands, without any config attributes
> 2) single patch per config attribute - please don't add them in bulk.
>    We also need very strict description for every single attribute so
>    other vendors know what it is and can re-use it. There is need to
>    avoid duplication here. Also, please send just few attribites in the
>    first run, not like 40 you are sending now. Impossible to review.

I broke the patch set up into functional blocks of attributes, in
order to avoid having ~40 patches of just a couple lines each.  But, I
will split further for each individual attribute, and just submit a
few initially, per your request.

>
> Also, why didn't you put it into nested attribute we were discussing?
>

I thought I did :) , using the DPIPE_HEADERS nested attribute as an
example.  I'll reach out to you off-list to understand what I'm
missing.

>>
>>+static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1];
>>+
>>+static int devlink_nl_sing_param_get(struct sk_buff *msg,
>
> I was wondering what song it will sing :) Just add "le", it's just 2
> chars :)
>

Will do, thanks. ;)

Steve

Re: [PATCH 1/7] devlink: Add permanent config parameter get/set operations

[Jiri Pirko]

Wed, Oct 18, 2017 at 02:39:09PM CEST, steven.lin1@broadcom.com wrote:
>On Wed, Oct 18, 2017 at 3:11 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Tue, Oct 17, 2017 at 10:44:23PM CEST, steven.lin1@broadcom.com wrote:
>> Steve. As I originally requested, could you please split this to:
>> 1) single patch adding config get/set commands, without any config attributes
>> 2) single patch per config attribute - please don't add them in bulk.
>>    We also need very strict description for every single attribute so
>>    other vendors know what it is and can re-use it. There is need to
>>    avoid duplication here. Also, please send just few attribites in the
>>    first run, not like 40 you are sending now. Impossible to review.
>
>I broke the patch set up into functional blocks of attributes, in
>order to avoid having ~40 patches of just a couple lines each.  But, I
>will split further for each individual attribute, and just submit a
>few initially, per your request.
>
>>
>> Also, why didn't you put it into nested attribute we were discussing?
>>
>
>I thought I did :) , using the DPIPE_HEADERS nested attribute as an
>example.  I'll reach out to you off-list to understand what I'm
>missing.

I missed that. But you need a separate attr enum as well.

Re: [PATCH 1/7] devlink: Add permanent config parameter get/set operations

[Steve Lin]

On Wed, Oct 18, 2017 at 8:58 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Wed, Oct 18, 2017 at 02:39:09PM CEST, steven.lin1@broadcom.com wrote:
>>On Wed, Oct 18, 2017 at 3:11 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Tue, Oct 17, 2017 at 10:44:23PM CEST, steven.lin1@broadcom.com wrote:
>>> Steve. As I originally requested, could you please split this to:
>>> 1) single patch adding config get/set commands, without any config attributes
>>> 2) single patch per config attribute - please don't add them in bulk.
>>>    We also need very strict description for every single attribute so
>>>    other vendors know what it is and can re-use it. There is need to
>>>    avoid duplication here. Also, please send just few attribites in the
>>>    first run, not like 40 you are sending now. Impossible to review.
>>
>>I broke the patch set up into functional blocks of attributes, in
>>order to avoid having ~40 patches of just a couple lines each.  But, I
>>will split further for each individual attribute, and just submit a
>>few initially, per your request.
>>
>>>
>>> Also, why didn't you put it into nested attribute we were discussing?
>>>
>>
>>I thought I did :) , using the DPIPE_HEADERS nested attribute as an
>>example.  I'll reach out to you off-list to understand what I'm
>>missing.
>
> I missed that. But you need a separate attr enum as well.
>

I did have this as the nested attr enum in the original patch:

        /* Permanent Configuration Parameters */
        DEVLINK_ATTR_PERM_CFG,                          /* nested */

However, I only used the nested construct in the response from kernel
to userspace, not in the request from userspace to kernel.  (This was
based on looking at the various DPIPE_* nested attributes as
examples.)

Thinking about it after seeing your comment, I'm thinking I should
also use the nested attribute construct in the original request from
userspace to kernel as well, although I didn't see any previous
examples of this in devlink.

So I'll plan to use nesting in that direction as well.

Thanks,
Steve

Re: [PATCH 1/7] devlink: Add permanent config parameter get/set operations

[Jiri Pirko]

Wed, Oct 18, 2017 at 03:14:35PM CEST, steven.lin1@broadcom.com wrote:
>On Wed, Oct 18, 2017 at 8:58 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Wed, Oct 18, 2017 at 02:39:09PM CEST, steven.lin1@broadcom.com wrote:
>>>On Wed, Oct 18, 2017 at 3:11 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> Tue, Oct 17, 2017 at 10:44:23PM CEST, steven.lin1@broadcom.com wrote:
>>>> Steve. As I originally requested, could you please split this to:
>>>> 1) single patch adding config get/set commands, without any config attributes
>>>> 2) single patch per config attribute - please don't add them in bulk.
>>>>    We also need very strict description for every single attribute so
>>>>    other vendors know what it is and can re-use it. There is need to
>>>>    avoid duplication here. Also, please send just few attribites in the
>>>>    first run, not like 40 you are sending now. Impossible to review.
>>>
>>>I broke the patch set up into functional blocks of attributes, in
>>>order to avoid having ~40 patches of just a couple lines each.  But, I
>>>will split further for each individual attribute, and just submit a
>>>few initially, per your request.
>>>
>>>>
>>>> Also, why didn't you put it into nested attribute we were discussing?
>>>>
>>>
>>>I thought I did :) , using the DPIPE_HEADERS nested attribute as an
>>>example.  I'll reach out to you off-list to understand what I'm
>>>missing.
>>
>> I missed that. But you need a separate attr enum as well.
>>
>
>I did have this as the nested attr enum in the original patch:

No, I mean separate "enum your_new_enum"


>
>        /* Permanent Configuration Parameters */
>        DEVLINK_ATTR_PERM_CFG,                          /* nested */
>
>However, I only used the nested construct in the response from kernel
>to userspace, not in the request from userspace to kernel.  (This was
>based on looking at the various DPIPE_* nested attributes as
>examples.)
>
>Thinking about it after seeing your comment, I'm thinking I should
>also use the nested attribute construct in the original request from
>userspace to kernel as well, although I didn't see any previous
>examples of this in devlink.
>
>So I'll plan to use nesting in that direction as well.
>
>Thanks,
>Steve

Re: [PATCH 1/7] devlink: Add permanent config parameter get/set operations

[Steve Lin]

On Wed, Oct 18, 2017 at 11:22 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Tue, Oct 17, 2017 at 11:44 PM, Steve Lin <steven.lin1@broadcom.com>
> wrote:
>>
>> Add support for permanent config parameter get/set commands. Used
>> for parameters held in NVRAM, persistent device configuration.
>> The config_get() and config_set() operations operate as expected, but
>> note that the driver implementation of the config_set() operation can
>> indicate whether a restart is necessary for the setting to take
>> effect.  This indication of a necessary restart is passed via the
>> DEVLINK_ATTR_PERM_CFG_RESTART_REQUIRED attribute.
>>
>> First set of parameters defined are PCI SR-IOV and per-VF
>> configuration:
>>
>> DEVLINK_ATTR_PERM_CFG_SRIOV_ENABLED: Enable SR-IOV capability.
>> DEVLINK_ATTR_PERM_CFG_NUM_VF_PER_PF: Maximum number of VFs per PF, in
>> SR-IOV mode.
>> DEVLINK_ATTR_PERM_CFG_MAX_NUM_PF_MSIX_VECT: Maximum number of
>> MSI-X vectors assigned per PF.
>> DEVLINK_ATTR_PERM_CFG_MSIX_VECTORS_PER_VF: 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/net/devlink.h        |   4 +
>>  include/uapi/linux/devlink.h |  20 ++++
>>  net/core/devlink.c           | 266
>> +++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 290 insertions(+)
>>
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index b9654e1..952966c 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
>> @@ -270,6 +270,10 @@ 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 (*config_get)(struct devlink *devlink, enum devlink_attr attr,
>> +                         u32 *value);
>> +       int (*config_set)(struct devlink *devlink, enum devlink_attr attr,
>> +                         u32 value, u8 *restart_reqd);
>>  };
>>
>>  static inline void *devlink_priv(struct devlink *devlink)
>> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>> index 0cbca96..34de44d 100644
>> --- a/include/uapi/linux/devlink.h
>> +++ b/include/uapi/linux/devlink.h
>> @@ -70,6 +70,9 @@ enum devlink_command {
>>         DEVLINK_CMD_DPIPE_HEADERS_GET,
>>         DEVLINK_CMD_DPIPE_TABLE_COUNTERS_SET,
>>
>> +       DEVLINK_CMD_CONFIG_GET,
>> +       DEVLINK_CMD_CONFIG_SET,
>> +
>>         /* add new commands above here */
>>         __DEVLINK_CMD_MAX,
>>         DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
>> @@ -202,6 +205,23 @@ enum devlink_attr {
>>
>>         DEVLINK_ATTR_ESWITCH_ENCAP_MODE,        /* u8 */
>>
>> +       /* Permanent Configuration Parameters */
>> +       DEVLINK_ATTR_PERM_CFG,                          /* nested */
>> +
>> +       /* When config doesn't take effect until next reboot (config
>> +        * just changed NVM which isn't read until boot, for example),
>> +        * this attribute should be set by the driver.
>> +        */
>> +       DEVLINK_ATTR_PERM_CFG_RESTART_REQUIRED,         /* u8 */
>> +       DEVLINK_ATTR_PERM_CFG_SRIOV_ENABLED,            /* u8 */
>> +       DEVLINK_ATTR_PERM_CFG_FIRST = DEVLINK_ATTR_PERM_CFG_SRIOV_ENABLED,
>
>
> ??? can you explain /   document this one?

I attempted to explain in the patch commit message, but I will try to
add more detail when I resubmit, with each attribute in a separate
patch.

>
> I would join Jiri's request to have patch #1 not defining any attributes,
> review will be much
> easier and robust.

Ok, I'll do that when I resubmit.

>
> Talking on attributes... what is your plan for vendor specific attributes?
>

They will be added just like common attributes, any given driver
doesn't have to support all attributes.

Steve

[PATCH 2/7] devlink: Adding NPAR permanent config parameters

[Steve Lin]

Extending DEVLINK_ATTR_PERM_CFG (permanent/NVRAM device configuration)
to include NPAR settings:

DEVLINK_ATTR_PERM_CFG_NPAR_NUM_PARTITIONS_PER_PORT: Number of NIC
Partitions (NPAR) per port.

DEVLINK_ATTR_PERM_CFG_NPAR_BW_IN_PERCENT: 1 if BW_RESERVATION and
BW_LIMIT is in percent; /0 if BW_RESERVATION and BW_LIMIT is in
100 Mbps units.

DEVLINK_ATTR_PERM_CFG_NPAR_BW_RESERVATION: Configures NPAR bandwidth
or weight reservation, in percent or 100 Mbps units, depending on
BW_IN_PERCENT.

DEVLINK_ATTR_PERM_CFG_NPAR_BW_RESERVATION_VALID: 1 to use
BW_RESERVATION setting, 0 to ignore.

DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT: Configures NPAR bandwidth or
weight limit, in percent or 100 Mbps units, depending on
BW_IN_PERCENT.

DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT_VALID: 1 to use BW_LIMIT
setting, 0 to ignore.

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

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 34de44d..21cfb37 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -218,9 +218,15 @@ enum devlink_attr {
 	DEVLINK_ATTR_PERM_CFG_NUM_VF_PER_PF,		/* u32 */
 	DEVLINK_ATTR_PERM_CFG_MAX_NUM_PF_MSIX_VECT,	/* u32 */
 	DEVLINK_ATTR_PERM_CFG_MSIX_VECTORS_PER_VF,	/* u32 */
+	DEVLINK_ATTR_PERM_CFG_NPAR_NUM_PARTITIONS_PER_PORT,	/* u32 */
+	DEVLINK_ATTR_PERM_CFG_NPAR_BW_IN_PERCENT,	/* u32 */
+	DEVLINK_ATTR_PERM_CFG_NPAR_BW_RESERVATION,	/* u32 */
+	DEVLINK_ATTR_PERM_CFG_NPAR_BW_RESERVATION_VALID,/* u8 */
+	DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT,		/* u32 */
+	DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT_VALID,	/* u8 */
 
 	/* Add new permanent config parameters above here */
-	DEVLINK_ATTR_PERM_CFG_LAST = DEVLINK_ATTR_PERM_CFG_MSIX_VECTORS_PER_VF,
+	DEVLINK_ATTR_PERM_CFG_LAST = DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT_VALID,
 
 	/* add new attributes above here, update the policy in devlink.c */
 
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 427a65e..76bb6d4 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2543,6 +2543,13 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_PERM_CFG_NUM_VF_PER_PF] = { .type = NLA_U32 },
 	[DEVLINK_ATTR_PERM_CFG_MAX_NUM_PF_MSIX_VECT] = { .type = NLA_U32 },
 	[DEVLINK_ATTR_PERM_CFG_MSIX_VECTORS_PER_VF] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CFG_NPAR_NUM_PARTITIONS_PER_PORT] = {
+		.type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CFG_NPAR_BW_IN_PERCENT] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CFG_NPAR_BW_RESERVATION] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CFG_NPAR_BW_RESERVATION_VALID] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT_VALID] = { .type = NLA_U8 },
 };
 
 static const struct genl_ops devlink_nl_ops[] = {
-- 
2.7.4

RE: [PATCH 2/7] devlink: Adding NPAR permanent config parameters

[Yuval Mintz]

> DEVLINK_ATTR_PERM_CFG_NPAR_BW_RESERVATION_VALID: 1 to use
> BW_RESERVATION setting, 0 to ignore.
> 
...
> DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT_VALID: 1 to use BW_LIMIT
> setting, 0 to ignore.

While it probably ties to different fields in your NVM layout why would the user
require specific attributes for these? Why not have values in the actual
attributes indicating of this status?

Re: [PATCH 2/7] devlink: Adding NPAR permanent config parameters

[Steve Lin]

On Thu, Oct 19, 2017 at 6:39 AM, Yuval Mintz <yuvalm@mellanox.com> wrote:
>> DEVLINK_ATTR_PERM_CFG_NPAR_BW_RESERVATION_VALID: 1 to use
>> BW_RESERVATION setting, 0 to ignore.
>>
> ...
>> DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT_VALID: 1 to use BW_LIMIT
>> setting, 0 to ignore.
>
> While it probably ties to different fields in your NVM layout why would the user
> require specific attributes for these? Why not have values in the actual
> attributes indicating of this status?

Hi Yuval,

Does having the separate valid flag present any difficulties?  There
are lots of implementation options here (a limit or reservation value
of 0 could mean invalid, or we could define (1 << 31) to be a valid
flag when setting the value, etc.), and I'm not necessarily tied to
doing it this way, but it seemed a straightforward way to represent
the validity of the other field.

Thanks again,
Steve

RE: [PATCH 2/7] devlink: Adding NPAR permanent config parameters

[Yuval Mintz]

> >> DEVLINK_ATTR_PERM_CFG_NPAR_BW_RESERVATION_VALID: 1 to use
> >> BW_RESERVATION setting, 0 to ignore.
> >>
> > ...
> >> DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT_VALID: 1 to use BW_LIMIT
> >> setting, 0 to ignore.
> >
> > While it probably ties to different fields in your NVM layout why would the
> user
> > require specific attributes for these? Why not have values in the actual
> > attributes indicating of this status?
> 
> Hi Yuval,
> 
> Does having the separate valid flag present any difficulties?  There
> are lots of implementation options here (a limit or reservation value
> of 0 could mean invalid, or we could define (1 << 31) to be a valid
> flag when setting the value, etc.), and I'm not necessarily tied to
> doing it this way, but it seemed a straightforward way to represent
> the validity of the other field.

You're pushing a LOT of new attributes, every one of which is going
to have to be documented for future generations.
I think whenever it's possible to drop an unnecessary attribute, that
would be the better option.
 

[PATCH 3/7] devlink: Adding high level dev perm config params

[Steve Lin]

Extending DEVLINK_ATTR_PERM_CFG (permanent/NVRAM device configuration)
to include some high level device configuration settings:

DEVLINK_ATTR_PERM_CFG_DCBX_MODE: Data Center Bridging Exchange
(DCBX) protocol mode; use enum devlink_dcbx_mode.

DEVLINK_ATTR_PERM_CFG_RDMA_ENABLED: 1 to enable RDMA, 0 to disable.

DEVLINK_ATTR_PERM_CFG_MULTIFUNC_MODE: Configure multi-function
mode; use devlink_multifunc_mode.

DEVLINK_ATTR_PERM_CFG_SECURE_NIC_ENABLED: 1 to enable Secure NIC
functionality, 0 to disable.

DEVLINK_ATTR_PERM_CFG_IGNORE_ARI_CAPABILITY: 1 to ignore ARI
(Alternate Routing ID) interpretation, 0 to honor ARI.

DEVLINK_ATTR_PERM_CFG_LLDP_NEAREST_BRIDGE_ENABLED: 1 to enable
Link Layer Data Protocol (LLDP) on Nearest Bridge, 0 to
disable.

DEVLINK_ATTR_PERM_CFG_LLDP_NEAREST_NONTPMR_BRIDGE_ENABLED: 1 to
enable Link Layer Data Protocol (LLDP) on Non Two Port MAC
Relay (non-TPMR) Bridge, 0 to disable.

DEVLINK_ATTR_PERM_CFG_PME_CAPABILITY_ENABLED: 1 to enable Power
Management Events (PME) functionality, 0 to disable.

DEVLINK_ATTR_PERM_CFG_MAGIC_PACKET_WOL_ENABLED: 1 to enable
Magic Packet Wake on Lan using ACPI pattern, 0 to disable.

DEVLINK_ATTR_PERM_CFG_EEE_PWR_SAVE_ENABLED: 1 to enable Energy
Efficient Ethernet (EEE), 0 to disable.

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

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 21cfb37..4a9eafd 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -127,6 +127,21 @@ enum devlink_eswitch_encap_mode {
 	DEVLINK_ESWITCH_ENCAP_MODE_BASIC,
 };
 
+enum devlink_dcbx_mode {
+	DEVLINK_DCBX_MODE_DISABLED,
+	DEVLINK_DCBX_MODE_IEEE,
+	DEVLINK_DCBX_MODE_CEE,
+	DEVLINK_DCBX_MODE_IEEE_CEE,
+};
+
+enum devlink_multifunc_mode {
+	DEVLINK_MULTIFUNC_MODE_ALLOWED,		/* Ext switch activates MF */
+	DEVLINK_MULTIFUNC_MODE_FORCE_SINGFUNC,
+	DEVLINK_MULTIFUNC_MODE_NPAR10,		/* NPAR 1.0 */
+	DEVLINK_MULTIFUNC_MODE_NPAR15,		/* NPAR 1.5 */
+	DEVLINK_MULTIFUNC_MODE_NPAR20,		/* NPAR 2.0 */
+};
+
 enum devlink_attr {
 	/* don't change the order or add anything between, this is ABI! */
 	DEVLINK_ATTR_UNSPEC,
@@ -224,9 +239,19 @@ enum devlink_attr {
 	DEVLINK_ATTR_PERM_CFG_NPAR_BW_RESERVATION_VALID,/* u8 */
 	DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT,		/* u32 */
 	DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT_VALID,	/* u8 */
+	DEVLINK_ATTR_PERM_CFG_DCBX_MODE,		/* u32 */
+	DEVLINK_ATTR_PERM_CFG_RDMA_ENABLED,		/* u8 */
+	DEVLINK_ATTR_PERM_CFG_MULTIFUNC_MODE,		/* u32 */
+	DEVLINK_ATTR_PERM_CFG_SECURE_NIC_ENABLED,	/* u8 */
+	DEVLINK_ATTR_PERM_CFG_IGNORE_ARI_CAPABILITY,	/* u8 */
+	DEVLINK_ATTR_PERM_CFG_LLDP_NEAREST_BRIDGE_ENABLED,	/* u8 */
+	DEVLINK_ATTR_PERM_CFG_LLDP_NEAREST_NONTPMR_BRIDGE_ENABLED,	/* u8 */
+	DEVLINK_ATTR_PERM_CFG_PME_CAPABILITY_ENABLED,	/* u8 */
+	DEVLINK_ATTR_PERM_CFG_MAGIC_PACKET_WOL_ENABLED,	/* u8 */
+	DEVLINK_ATTR_PERM_CFG_EEE_PWR_SAVE_ENABLED,	/* u8 */
 
 	/* Add new permanent config parameters above here */
-	DEVLINK_ATTR_PERM_CFG_LAST = DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT_VALID,
+	DEVLINK_ATTR_PERM_CFG_LAST = DEVLINK_ATTR_PERM_CFG_EEE_PWR_SAVE_ENABLED,
 
 	/* add new attributes above here, update the policy in devlink.c */
 
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 76bb6d4..d611154 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2550,6 +2550,18 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_PERM_CFG_NPAR_BW_RESERVATION_VALID] = { .type = NLA_U8 },
 	[DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT] = { .type = NLA_U32 },
 	[DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT_VALID] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PERM_CFG_DCBX_MODE] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CFG_RDMA_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PERM_CFG_MULTIFUNC_MODE] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CFG_SECURE_NIC_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PERM_CFG_IGNORE_ARI_CAPABILITY] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PERM_CFG_LLDP_NEAREST_BRIDGE_ENABLED] = {
+		.type = NLA_U8 },
+	[DEVLINK_ATTR_PERM_CFG_LLDP_NEAREST_NONTPMR_BRIDGE_ENABLED] = {
+		.type = NLA_U8 },
+	[DEVLINK_ATTR_PERM_CFG_PME_CAPABILITY_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PERM_CFG_MAGIC_PACKET_WOL_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PERM_CFG_EEE_PWR_SAVE_ENABLED] = { .type = NLA_U8 },
 };
 
 static const struct genl_ops devlink_nl_ops[] = {
-- 
2.7.4

RE: [PATCH 3/7] devlink: Adding high level dev perm config params

[Yuval Mintz]

> DEVLINK_ATTR_PERM_CFG_MULTIFUNC_MODE: Configure multi-function
> mode; use devlink_multifunc_mode.
...
> +enum devlink_multifunc_mode {
> +	DEVLINK_MULTIFUNC_MODE_ALLOWED,		/* Ext switch
> activates MF */
> +	DEVLINK_MULTIFUNC_MODE_FORCE_SINGFUNC,
> +	DEVLINK_MULTIFUNC_MODE_NPAR10,		/* NPAR 1.0
> */
> +	DEVLINK_MULTIFUNC_MODE_NPAR15,		/* NPAR 1.5
> */
> +	DEVLINK_MULTIFUNC_MODE_NPAR20,		/* NPAR 2.0
> */
> +};

... And when someone would invent a new partitioning scheme, what then?
You'd extend the 'generic' modes?
It's not very generic.

> DEVLINK_ATTR_PERM_CFG_SECURE_NIC_ENABLED: 1 to enable Secure NIC
> functionality, 0 to disable.

What does it mean? Is there some spec explaining it?

[PATCH 4/7] devlink: Adding perm config of link settings

[Steve Lin]

Extending DEVLINK_ATTR_PERM_CFG (permanent/NVRAM device configuration)
to include persistent configuration of device link settings:

DEVLINK_ATTR_PERM_CFG_AUTONEG_PROTOCOL: Configure default autoneg
protocol; use enum devlink_autoneg_protocol.

DEVLINK_ATTR_PERM_CFG_MEDIA_AUTO_DETECT: Configure default
auto-detection of attached media connector (1 = enable, 0 =
disable).

DEVLINK_ATTR_PERM_CFG_PHY_SELECT: Configure default external PHY
selection (0 = PHY 0, 1 = PHY 1).

DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D0: Configure default
pre-OS link speed in full power (D0) state; use enum
devlink_pre_os_link_speed.

DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D3: Configure default
pre-OS link speed in sleep (D3) state; use enum
devlink_pre_os_link_speed.

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

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 4a9eafd..2e1c006 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -142,6 +142,26 @@ enum devlink_multifunc_mode {
 	DEVLINK_MULTIFUNC_MODE_NPAR20,		/* NPAR 2.0 */
 };
 
+enum devlink_autoneg_protocol {
+	DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY_BAM,
+	DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY_CONSORTIUM,
+	DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY,
+	DEVLINK_AUTONEG_PROTOCOL_BAM,		/* Broadcom Autoneg Mode */
+	DEVLINK_AUTONEG_PROTOCOL_CONSORTIUM,	/* Consortium Autoneg Mode */
+};
+
+enum devlink_pre_os_link_speed {
+	DEVLINK_PRE_OS_LINK_SPEED_AUTONEG,
+	DEVLINK_PRE_OS_LINK_SPEED_1G,
+	DEVLINK_PRE_OS_LINK_SPEED_10G,
+	DEVLINK_PRE_OS_LINK_SPEED_25G,
+	DEVLINK_PRE_OS_LINK_SPEED_40G,
+	DEVLINK_PRE_OS_LINK_SPEED_50G,
+	DEVLINK_PRE_OS_LINK_SPEED_100G,
+	DEVLINK_PRE_OS_LINK_SPEED_5G = 0xe,
+	DEVLINK_PRE_OS_LINK_SPEED_100M = 0xf,
+};
+
 enum devlink_attr {
 	/* don't change the order or add anything between, this is ABI! */
 	DEVLINK_ATTR_UNSPEC,
@@ -249,9 +269,14 @@ enum devlink_attr {
 	DEVLINK_ATTR_PERM_CFG_PME_CAPABILITY_ENABLED,	/* u8 */
 	DEVLINK_ATTR_PERM_CFG_MAGIC_PACKET_WOL_ENABLED,	/* u8 */
 	DEVLINK_ATTR_PERM_CFG_EEE_PWR_SAVE_ENABLED,	/* u8 */
+	DEVLINK_ATTR_PERM_CFG_AUTONEG_PROTOCOL,		/* u32 */
+	DEVLINK_ATTR_PERM_CFG_MEDIA_AUTO_DETECT,	/* u8 */
+	DEVLINK_ATTR_PERM_CFG_PHY_SELECT,		/* u8 */
+	DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D0,	/* u32 */
+	DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D3,	/* u32 */
 
 	/* Add new permanent config parameters above here */
-	DEVLINK_ATTR_PERM_CFG_LAST = DEVLINK_ATTR_PERM_CFG_EEE_PWR_SAVE_ENABLED,
+	DEVLINK_ATTR_PERM_CFG_LAST = DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D3,
 
 	/* add new attributes above here, update the policy in devlink.c */
 
diff --git a/net/core/devlink.c b/net/core/devlink.c
index d611154..80a2a50 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2562,6 +2562,11 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_PERM_CFG_PME_CAPABILITY_ENABLED] = { .type = NLA_U8 },
 	[DEVLINK_ATTR_PERM_CFG_MAGIC_PACKET_WOL_ENABLED] = { .type = NLA_U8 },
 	[DEVLINK_ATTR_PERM_CFG_EEE_PWR_SAVE_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PERM_CFG_AUTONEG_PROTOCOL] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CFG_MEDIA_AUTO_DETECT] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PERM_CFG_PHY_SELECT] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D0] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D3] = { .type = NLA_U32 },
 };
 
 static const struct genl_ops devlink_nl_ops[] = {
-- 
2.7.4

Re: [PATCH 4/7] devlink: Adding perm config of link settings

[Jiri Pirko]

Tue, Oct 17, 2017 at 10:44:26PM CEST, steven.lin1@broadcom.com wrote:
>Extending DEVLINK_ATTR_PERM_CFG (permanent/NVRAM device configuration)
>to include persistent configuration of device link settings:
>
>DEVLINK_ATTR_PERM_CFG_AUTONEG_PROTOCOL: Configure default autoneg
>protocol; use enum devlink_autoneg_protocol.
>
>DEVLINK_ATTR_PERM_CFG_MEDIA_AUTO_DETECT: Configure default
>auto-detection of attached media connector (1 = enable, 0 =
>disable).
>
>DEVLINK_ATTR_PERM_CFG_PHY_SELECT: Configure default external PHY
>selection (0 = PHY 0, 1 = PHY 1).
>
>DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D0: Configure default
>pre-OS link speed in full power (D0) state; use enum
>devlink_pre_os_link_speed.
>
>DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D3: Configure default
>pre-OS link speed in sleep (D3) state; use enum
>devlink_pre_os_link_speed.
>
>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>Acked-by: Andy Gospodarek <gospo@broadcom.com>
>---
> include/uapi/linux/devlink.h | 27 ++++++++++++++++++++++++++-
> net/core/devlink.c           |  5 +++++
> 2 files changed, 31 insertions(+), 1 deletion(-)
>
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index 4a9eafd..2e1c006 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -142,6 +142,26 @@ enum devlink_multifunc_mode {
> 	DEVLINK_MULTIFUNC_MODE_NPAR20,		/* NPAR 2.0 */
> };
> 
>+enum devlink_autoneg_protocol {
>+	DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY_BAM,
>+	DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY_CONSORTIUM,
>+	DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY,
>+	DEVLINK_AUTONEG_PROTOCOL_BAM,		/* Broadcom Autoneg Mode */
>+	DEVLINK_AUTONEG_PROTOCOL_CONSORTIUM,	/* Consortium Autoneg Mode */
>+};
>+
>+enum devlink_pre_os_link_speed {
>+	DEVLINK_PRE_OS_LINK_SPEED_AUTONEG,
>+	DEVLINK_PRE_OS_LINK_SPEED_1G,
>+	DEVLINK_PRE_OS_LINK_SPEED_10G,
>+	DEVLINK_PRE_OS_LINK_SPEED_25G,
>+	DEVLINK_PRE_OS_LINK_SPEED_40G,
>+	DEVLINK_PRE_OS_LINK_SPEED_50G,
>+	DEVLINK_PRE_OS_LINK_SPEED_100G,
>+	DEVLINK_PRE_OS_LINK_SPEED_5G = 0xe,
>+	DEVLINK_PRE_OS_LINK_SPEED_100M = 0xf,
>+};
>+
> enum devlink_attr {
> 	/* don't change the order or add anything between, this is ABI! */
> 	DEVLINK_ATTR_UNSPEC,
>@@ -249,9 +269,14 @@ enum devlink_attr {
> 	DEVLINK_ATTR_PERM_CFG_PME_CAPABILITY_ENABLED,	/* u8 */
> 	DEVLINK_ATTR_PERM_CFG_MAGIC_PACKET_WOL_ENABLED,	/* u8 */
> 	DEVLINK_ATTR_PERM_CFG_EEE_PWR_SAVE_ENABLED,	/* u8 */
>+	DEVLINK_ATTR_PERM_CFG_AUTONEG_PROTOCOL,		/* u32 */
>+	DEVLINK_ATTR_PERM_CFG_MEDIA_AUTO_DETECT,	/* u8 */
>+	DEVLINK_ATTR_PERM_CFG_PHY_SELECT,		/* u8 */
>+	DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D0,	/* u32 */
>+	DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D3,	/* u32 */


You need to split the config option to those that are per-port and to
those that are per-asic. For each family, you have to use ither
devlink_port of devlink handle. Also, you need to split into those that are
permanent and to those who are teporary (until reset). I think you might
need some flags for that.

Re: [PATCH 4/7] devlink: Adding perm config of link settings

[Steve Lin]

On Wed, Oct 18, 2017 at 3:31 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>
> You need to split the config option to those that are per-port and to
> those that are per-asic. For each family, you have to use ither
> devlink_port of devlink handle. Also, you need to split into those that are
> permanent and to those who are teporary (until reset). I think you might
> need some flags for that.
>

All these are permanent; none are temporary - that's (partially) why
we consider these to be devlink/device parameters rather than a
netdev/ethtool thing, since they take effect after the next reset and
before any drivers are loaded (i.e. the card uses these parameters for
its default/startup configuration).

I will take a closer look at splitting these between per-port and per-asic.

Thanks,
Steve

Re: [PATCH 4/7] devlink: Adding perm config of link settings

[Jiri Pirko]

Wed, Oct 18, 2017 at 02:39:42PM CEST, steven.lin1@broadcom.com wrote:
>On Wed, Oct 18, 2017 at 3:31 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> You need to split the config option to those that are per-port and to
>> those that are per-asic. For each family, you have to use ither
>> devlink_port of devlink handle. Also, you need to split into those that are
>> permanent and to those who are teporary (until reset). I think you might
>> need some flags for that.
>>
>
>All these are permanent; none are temporary - that's (partially) why
>we consider these to be devlink/device parameters rather than a
>netdev/ethtool thing, since they take effect after the next reset and
>before any drivers are loaded (i.e. the card uses these parameters for
>its default/startup configuration).

Understood. But I think that this iface should be capable to serve the
options of non-permanent as well. Or this could be 2 separate interfaces
with 2 separate cmd pair. Thoughts?


>
>I will take a closer look at splitting these between per-port and per-asic.
>
>Thanks,
>Steve

Re: [PATCH 4/7] devlink: Adding perm config of link settings

[Steve Lin]

On Wed, Oct 18, 2017 at 9:01 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Wed, Oct 18, 2017 at 02:39:42PM CEST, steven.lin1@broadcom.com wrote:
>>On Wed, Oct 18, 2017 at 3:31 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>
>>> You need to split the config option to those that are per-port and to
>>> those that are per-asic. For each family, you have to use ither
>>> devlink_port of devlink handle. Also, you need to split into those that are
>>> permanent and to those who are teporary (until reset). I think you might
>>> need some flags for that.
>>>
>>
>>All these are permanent; none are temporary - that's (partially) why
>>we consider these to be devlink/device parameters rather than a
>>netdev/ethtool thing, since they take effect after the next reset and
>>before any drivers are loaded (i.e. the card uses these parameters for
>>its default/startup configuration).
>
> Understood. But I think that this iface should be capable to serve the
> options of non-permanent as well. Or this could be 2 separate interfaces
> with 2 separate cmd pair. Thoughts?
>

I would prefer to keep this command for permanent config only, and use
a separate command for transient configuration.  I think that
transient device configuration should be tackled in the separate
discussion that was started in the RFC version of this patchset,
related to moving ethtool ops to devlink/netlink.

I think that's a separate topic that requires a little more thought
and discussion, but really isn't that related to this current patchset
for permanent device configuration.  What do you think?

Steve

Re: [PATCH 4/7] devlink: Adding perm config of link settings

[Jiri Pirko]

Wed, Oct 18, 2017 at 03:22:03PM CEST, steven.lin1@broadcom.com wrote:
>On Wed, Oct 18, 2017 at 9:01 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Wed, Oct 18, 2017 at 02:39:42PM CEST, steven.lin1@broadcom.com wrote:
>>>On Wed, Oct 18, 2017 at 3:31 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>
>>>> You need to split the config option to those that are per-port and to
>>>> those that are per-asic. For each family, you have to use ither
>>>> devlink_port of devlink handle. Also, you need to split into those that are
>>>> permanent and to those who are teporary (until reset). I think you might
>>>> need some flags for that.
>>>>
>>>
>>>All these are permanent; none are temporary - that's (partially) why
>>>we consider these to be devlink/device parameters rather than a
>>>netdev/ethtool thing, since they take effect after the next reset and
>>>before any drivers are loaded (i.e. the card uses these parameters for
>>>its default/startup configuration).
>>
>> Understood. But I think that this iface should be capable to serve the
>> options of non-permanent as well. Or this could be 2 separate interfaces
>> with 2 separate cmd pair. Thoughts?
>>
>
>I would prefer to keep this command for permanent config only, and use
>a separate command for transient configuration.  I think that
>transient device configuration should be tackled in the separate
>discussion that was started in the RFC version of this patchset,
>related to moving ethtool ops to devlink/netlink.
>
>I think that's a separate topic that requires a little more thought
>and discussion, but really isn't that related to this current patchset
>for permanent device configuration.  What do you think?

Makes sense. Then you need to clearly mark the cmds with "permanent"
keyword in name and enhance the description.

RE: [PATCH 4/7] devlink: Adding perm config of link settings

[Yuval Mintz]

> +enum devlink_autoneg_protocol {
> +	DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY_BAM,
> +	DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY_CONSORTIUM,
> +	DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY,
> +	DEVLINK_AUTONEG_PROTOCOL_BAM,		/* Broadcom
> Autoneg Mode */
> +	DEVLINK_AUTONEG_PROTOCOL_CONSORTIUM,	/*
> Consortium Autoneg Mode */
> +};

Wouldn't adding BAM as a 'generic' mode of operation be like adding
non-consortium speeds to ethtool API?
[I profess ignorance in this area; For all I know it can be a widely accepted
industry standard]

Re: [PATCH 4/7] devlink: Adding perm config of link settings

[Steve Lin]

On Thu, Oct 19, 2017 at 2:07 AM, Yuval Mintz <yuvalm@mellanox.com> wrote:
>> +enum devlink_autoneg_protocol {
>> +     DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY_BAM,
>> +     DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY_CONSORTIUM,
>> +     DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY,
>> +     DEVLINK_AUTONEG_PROTOCOL_BAM,           /* Broadcom
>> Autoneg Mode */
>> +     DEVLINK_AUTONEG_PROTOCOL_CONSORTIUM,    /*
>> Consortium Autoneg Mode */
>> +};
>
> Wouldn't adding BAM as a 'generic' mode of operation be like adding
> non-consortium speeds to ethtool API?
> [I profess ignorance in this area; For all I know it can be a widely accepted
> industry standard]
>

Yuval, I'm glad to get input from other NIC vendors.  The high-level
goal of this effort is to allow users of various vendors' NICs to be
able to configure these types of NVRAM/permanent/default settings
using an inbox tool, rather than the collection of vendor-specific
tools that is the status quo.

In order to provide that functionality, it seems like the
vendor-specific parameters and also the vendor-specific settings of
common parameters both need to be supported in this manner.

Ideally there will be much overlap in both the set of parameters
available as well as the options for each parameter, but in the real
world, there will always be differences between vendors and even
between different devices (drivers) from the same vendor.  Despite
that reality, I think there is still great benefit in having a common
inbox tool that users can use for device configuration of this type.
It just means that not all drivers will support all parameters, nor
all options for each parameter that they do support.

Thanks,
Steve

RE: [PATCH 4/7] devlink: Adding perm config of link settings

[Yuval Mintz]

> On Thu, Oct 19, 2017 at 2:07 AM, Yuval Mintz <yuvalm@mellanox.com>
> wrote:
> >> +enum devlink_autoneg_protocol {
> >> +     DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY_BAM,
> >> +     DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY_CONSORTIUM,
> >> +     DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY,
> >> +     DEVLINK_AUTONEG_PROTOCOL_BAM,           /* Broadcom
> >> Autoneg Mode */
> >> +     DEVLINK_AUTONEG_PROTOCOL_CONSORTIUM,    /*
> >> Consortium Autoneg Mode */
> >> +};
> >
> > Wouldn't adding BAM as a 'generic' mode of operation be like adding
> > non-consortium speeds to ethtool API?
> > [I profess ignorance in this area; For all I know it can be a widely accepted
> > industry standard]
> >
> 
> Yuval, I'm glad to get input from other NIC vendors.  
Other switch vendors ;)

>The high-level goal of this effort is to allow users of various vendors' NICs to be
> able to configure these types of NVRAM/permanent/default settings
> using an inbox tool, rather than the collection of vendor-specific
> tools that is the status quo.
> 
> In order to provide that functionality, it seems like the
> vendor-specific parameters and also the vendor-specific settings of
> common parameters both need to be supported in this manner.
> 
> Ideally there will be much overlap in both the set of parameters
> available as well as the options for each parameter, but in the real
> world, there will always be differences between vendors and even
> between different devices (drivers) from the same vendor.  Despite
> that reality, I think there is still great benefit in having a common
> inbox tool that users can use for device configuration of this type.
> It just means that not all drivers will support all parameters, nor
> all options for each parameter that they do support.

I don't object the end-goal; I think my hesitation is due to the same
enum containing both generic and vendor-specific values mixed
together. I feel like we need some clear distinction between the two.

> 
> Thanks,
> Steve

[PATCH 5/7] devlink: Adding pre-boot permanent config parameters

[Steve Lin]

Extending DEVLINK_ATTR_PERM_CFG (permanent/NVRAM device configuration)
to include some pre-boot device configuration settings:

DEVLINK_ATTR_PERM_CFG_MBA_ENABLED: 1 to enable Multiple Boot
Agent (BMA), 0 to disable.

DEVLINK_ATTR_PERM_CFG_MBA_BOOT_TYPE: Controls mechanism MBA will
use to insert itself into the list of devices recognized by the
BIOS; use enum devlink_mba_boot_type.

DEVLINK_ATTR_PERM_CFG_MBA_DELAY_TIME: Controls how long MBA
banner display and ability to enter MBA setup will persist
during initialization, in seconds.

DEVLINK_ATTR_PERM_CFG_MBA_SETUP_HOT_KEY: Configures which hot-key
will be used to enter MBA setup; use enum devlink_mba_setup_hot_key.

DEVLINK_ATTR_PERM_CFG_MBA_HIDE_SETUP_PROMPT: 1 to enable hiding
of 'enter setup' prompt during initialization, 0 to disable.

DEVLINK_ATTR_PERM_CFG_MBA_BOOT_RETRY_COUNT: MBA retries booting
this number of times, if it fails initially.

DEVLINK_ATTR_PERM_CFG_MBA_VLAN_ENABLED: 1 to enable using VLAN
when executing MBA host software (PXE/iSCSI), 0 to disable.

DEVLINK_ATTR_PERM_CFG_MBA_VLAN_TAG: The 16 bit VLAN tag to use
if MBA_VLAN_ENABLED is set.

DEVLINK_ATTR_PERM_CFG_MBA_BOOT_PROTOCOL: Selects MBA boot
protocol; use enum devlink_mba_boot_protocol.

DEVLINK_ATTR_PERM_CFG_MBA_LINK_SPEED: Configured link speed
while executing MBA host software (PXI/iSCSI); use enum
devlink_mba_link_speed.

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

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 2e1c006..609784a 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -162,6 +162,33 @@ enum devlink_pre_os_link_speed {
 	DEVLINK_PRE_OS_LINK_SPEED_100M = 0xf,
 };
 
+enum devlink_mba_boot_type {
+	DEVLINK_MBA_BOOT_TYPE_AUTO_DETECT,
+	DEVLINK_MBA_BOOT_TYPE_BBS,		/* BIOS Boot Specification */
+	DEVLINK_MBA_BOOT_TYPE_INTR18,		/* Hook interrupt 0x18 */
+	DEVLINK_MBA_BOOT_TYPE_INTR19,		/* Hook interrupt 0x19 */
+};
+
+enum devlink_mba_setup_hot_key {
+	DEVLINK_MBA_SETUP_HOT_KEY_CTRL_S,
+	DEVLINK_MBA_SETUP_HOT_KEY_CTRL_B,
+};
+
+enum devlink_mba_boot_protocol {
+	DEVLINK_MBA_BOOT_PROTOCOL_PXE,
+	DEVLINK_MBA_BOOT_PROTOCOL_ISCSI,
+	DEVLINK_MBA_BOOT_PROTOCOL_NONE = 0x7,
+};
+
+enum devlink_mba_link_speed {
+	DEVLINK_MBA_LINK_SPEED_AUTONEG,
+	DEVLINK_MBA_LINK_SPEED_1G,
+	DEVLINK_MBA_LINK_SPEED_10G,
+	DEVLINK_MBA_LINK_SPEED_25G,
+	DEVLINK_MBA_LINK_SPEED_40G,
+	DEVLINK_MBA_LINK_SPEED_50G,
+};
+
 enum devlink_attr {
 	/* don't change the order or add anything between, this is ABI! */
 	DEVLINK_ATTR_UNSPEC,
@@ -274,9 +301,19 @@ enum devlink_attr {
 	DEVLINK_ATTR_PERM_CFG_PHY_SELECT,		/* u8 */
 	DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D0,	/* u32 */
 	DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D3,	/* u32 */
+	DEVLINK_ATTR_PERM_CFG_MBA_ENABLED,		/* u8 */
+	DEVLINK_ATTR_PERM_CFG_MBA_BOOT_TYPE,		/* u32 */
+	DEVLINK_ATTR_PERM_CFG_MBA_DELAY_TIME,		/* u32 */
+	DEVLINK_ATTR_PERM_CFG_MBA_SETUP_HOT_KEY,	/* u32 */
+	DEVLINK_ATTR_PERM_CFG_MBA_HIDE_SETUP_PROMPT,	/* u8 */
+	DEVLINK_ATTR_PERM_CFG_MBA_BOOT_RETRY_COUNT,	/* u32 */
+	DEVLINK_ATTR_PERM_CFG_MBA_VLAN_ENABLED,		/* u8 */
+	DEVLINK_ATTR_PERM_CFG_MBA_VLAN_TAG,		/* u16 */
+	DEVLINK_ATTR_PERM_CFG_MBA_BOOT_PROTOCOL,	/* u32 */
+	DEVLINK_ATTR_PERM_CFG_MBA_LINK_SPEED,		/* u32 */
 
 	/* Add new permanent config parameters above here */
-	DEVLINK_ATTR_PERM_CFG_LAST = DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D3,
+	DEVLINK_ATTR_PERM_CFG_LAST = DEVLINK_ATTR_PERM_CFG_MBA_LINK_SPEED,
 
 	/* add new attributes above here, update the policy in devlink.c */
 
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 80a2a50..2eaa566 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2567,6 +2567,16 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_PERM_CFG_PHY_SELECT] = { .type = NLA_U8 },
 	[DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D0] = { .type = NLA_U32 },
 	[DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D3] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CFG_MBA_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PERM_CFG_MBA_BOOT_TYPE] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CFG_MBA_DELAY_TIME] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CFG_MBA_SETUP_HOT_KEY] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CFG_MBA_HIDE_SETUP_PROMPT] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PERM_CFG_MBA_BOOT_RETRY_COUNT] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CFG_MBA_VLAN_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PERM_CFG_MBA_VLAN_TAG] = { .type = NLA_U16 },
+	[DEVLINK_ATTR_PERM_CFG_MBA_BOOT_PROTOCOL] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CFG_MBA_LINK_SPEED] = { .type = NLA_U32 },
 };
 
 static const struct genl_ops devlink_nl_ops[] = {
-- 
2.7.4

RE: [PATCH 5/7] devlink: Adding pre-boot permanent config parameters

[Yuval Mintz]

> DEVLINK_ATTR_PERM_CFG_MBA_LINK_SPEED: Configured link speed
> while executing MBA host software (PXI/iSCSI); use enum
> devlink_mba_link_speed.

#4 introduces:

> DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D0: Configure default
> pre-OS link speed in full power (D0) state; use enum
> devlink_pre_os_link_speed.

> DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D3: Configure default
> pre-OS link speed in sleep (D3) state; use enum
> devlink_pre_os_link_speed.

Why would user need an additional value for the MBA speed?

[PATCH 6/7] bnxt: Move generic devlink code to new file

[Steve Lin]

Moving generic devlink code (registration) out of VR-R code
into new bnxt_devlink file.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/Makefile       |  2 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt.c         |  1 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 65 +++++++++++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h | 39 ++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c     | 53 ++----------------
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h     | 37 ++-----------
 6 files changed, 112 insertions(+), 85 deletions(-)
 create mode 100644 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
 create mode 100644 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h

diff --git a/drivers/net/ethernet/broadcom/bnxt/Makefile b/drivers/net/ethernet/broadcom/bnxt/Makefile
index 457201f..59c8ec9 100644
--- a/drivers/net/ethernet/broadcom/bnxt/Makefile
+++ b/drivers/net/ethernet/broadcom/bnxt/Makefile
@@ -1,4 +1,4 @@
 obj-$(CONFIG_BNXT) += bnxt_en.o
 
-bnxt_en-y := bnxt.o bnxt_sriov.o bnxt_ethtool.o bnxt_dcb.o bnxt_ulp.o bnxt_xdp.o bnxt_vfr.o
+bnxt_en-y := bnxt.o bnxt_sriov.o bnxt_ethtool.o bnxt_dcb.o bnxt_ulp.o bnxt_xdp.o bnxt_vfr.o bnxt_devlink.o
 bnxt_en-$(CONFIG_BNXT_FLOWER_OFFLOAD) += bnxt_tc.o
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 5ba4993..52cc38d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -61,6 +61,7 @@
 #include "bnxt_xdp.h"
 #include "bnxt_vfr.h"
 #include "bnxt_tc.h"
+#include "bnxt_devlink.h"
 
 #define BNXT_TX_TIMEOUT		(5 * HZ)
 
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
new file mode 100644
index 0000000..f3f6aa8
--- /dev/null
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -0,0 +1,65 @@
+/* Broadcom NetXtreme-C/E network driver.
+ *
+ * Copyright (c) 2017 Broadcom Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/pci.h>
+#include <linux/netdevice.h>
+#include "bnxt_hsi.h"
+#include "bnxt.h"
+#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 */
+};
+
+int bnxt_dl_register(struct bnxt *bp)
+{
+	struct devlink *dl;
+	int rc;
+
+	if (!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV))
+		return 0;
+
+	if (bp->hwrm_spec_code < 0x10800) {
+		netdev_warn(bp->dev, "Firmware does not support SR-IOV E-Switch SWITCHDEV mode.\n");
+		return -ENOTSUPP;
+	}
+
+	dl = devlink_alloc(&bnxt_dl_ops, sizeof(struct bnxt_dl));
+	if (!dl) {
+		netdev_warn(bp->dev, "devlink_alloc failed");
+		return -ENOMEM;
+	}
+
+	bnxt_link_bp_to_dl(bp, dl);
+	bp->eswitch_mode = DEVLINK_ESWITCH_MODE_LEGACY;
+	rc = devlink_register(dl, &bp->pdev->dev);
+	if (rc) {
+		bnxt_link_bp_to_dl(bp, NULL);
+		devlink_free(dl);
+		netdev_warn(bp->dev, "devlink_register failed. rc=%d", rc);
+		return rc;
+	}
+
+	return 0;
+}
+
+void bnxt_dl_unregister(struct bnxt *bp)
+{
+	struct devlink *dl = bp->dl;
+
+	if (!dl)
+		return;
+
+	devlink_unregister(dl);
+	devlink_free(dl);
+}
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
new file mode 100644
index 0000000..e92a35d
--- /dev/null
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
@@ -0,0 +1,39 @@
+/* Broadcom NetXtreme-C/E network driver.
+ *
+ * Copyright (c) 2017 Broadcom Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation.
+ */
+
+#ifndef BNXT_DEVLINK_H
+#define BNXT_DEVLINK_H
+
+/* Struct to hold housekeeping info needed by devlink interface */
+struct bnxt_dl {
+	struct bnxt *bp;	/* back ptr to the controlling dev */
+};
+
+static inline struct bnxt *bnxt_get_bp_from_dl(struct devlink *dl)
+{
+	return ((struct bnxt_dl *)devlink_priv(dl))->bp;
+}
+
+/* To clear devlink pointer from bp, pass NULL dl */
+static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
+{
+	bp->dl = dl;
+
+	/* add a back pointer in dl to bp */
+	if (dl) {
+		struct bnxt_dl *bp_dl = devlink_priv(dl);
+
+		bp_dl->bp = bp;
+	}
+}
+
+int bnxt_dl_register(struct bnxt *bp);
+void bnxt_dl_unregister(struct bnxt *bp);
+
+#endif /* BNXT_DEVLINK_H */
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
index e75db04..b76849f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
@@ -16,6 +16,7 @@
 #include "bnxt_hsi.h"
 #include "bnxt.h"
 #include "bnxt_vfr.h"
+#include "bnxt_devlink.h"
 #include "bnxt_tc.h"
 
 #ifdef CONFIG_BNXT_SRIOV
@@ -416,7 +417,7 @@ static int bnxt_vf_reps_create(struct bnxt *bp)
 }
 
 /* Devlink related routines */
-static int bnxt_dl_eswitch_mode_get(struct devlink *devlink, u16 *mode)
+int bnxt_dl_eswitch_mode_get(struct devlink *devlink, u16 *mode)
 {
 	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
 
@@ -424,7 +425,7 @@ static int bnxt_dl_eswitch_mode_get(struct devlink *devlink, u16 *mode)
 	return 0;
 }
 
-static int bnxt_dl_eswitch_mode_set(struct devlink *devlink, u16 mode)
+int bnxt_dl_eswitch_mode_set(struct devlink *devlink, u16 mode)
 {
 	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
 	int rc = 0;
@@ -462,52 +463,4 @@ static int bnxt_dl_eswitch_mode_set(struct devlink *devlink, u16 mode)
 	return rc;
 }
 
-static const struct devlink_ops bnxt_dl_ops = {
-	.eswitch_mode_set = bnxt_dl_eswitch_mode_set,
-	.eswitch_mode_get = bnxt_dl_eswitch_mode_get
-};
-
-int bnxt_dl_register(struct bnxt *bp)
-{
-	struct devlink *dl;
-	int rc;
-
-	if (!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV))
-		return 0;
-
-	if (bp->hwrm_spec_code < 0x10800) {
-		netdev_warn(bp->dev, "Firmware does not support SR-IOV E-Switch SWITCHDEV mode.\n");
-		return -ENOTSUPP;
-	}
-
-	dl = devlink_alloc(&bnxt_dl_ops, sizeof(struct bnxt_dl));
-	if (!dl) {
-		netdev_warn(bp->dev, "devlink_alloc failed");
-		return -ENOMEM;
-	}
-
-	bnxt_link_bp_to_dl(bp, dl);
-	bp->eswitch_mode = DEVLINK_ESWITCH_MODE_LEGACY;
-	rc = devlink_register(dl, &bp->pdev->dev);
-	if (rc) {
-		bnxt_link_bp_to_dl(bp, NULL);
-		devlink_free(dl);
-		netdev_warn(bp->dev, "devlink_register failed. rc=%d", rc);
-		return rc;
-	}
-
-	return 0;
-}
-
-void bnxt_dl_unregister(struct bnxt *bp)
-{
-	struct devlink *dl = bp->dl;
-
-	if (!dl)
-		return;
-
-	devlink_unregister(dl);
-	devlink_free(dl);
-}
-
 #endif
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h
index 7787cd24..fb06bbe 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h
@@ -14,31 +14,6 @@
 
 #define	MAX_CFA_CODE			65536
 
-/* Struct to hold housekeeping info needed by devlink interface */
-struct bnxt_dl {
-	struct bnxt *bp;	/* back ptr to the controlling dev */
-};
-
-static inline struct bnxt *bnxt_get_bp_from_dl(struct devlink *dl)
-{
-	return ((struct bnxt_dl *)devlink_priv(dl))->bp;
-}
-
-/* To clear devlink pointer from bp, pass NULL dl */
-static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
-{
-	bp->dl = dl;
-
-	/* add a back pointer in dl to bp */
-	if (dl) {
-		struct bnxt_dl *bp_dl = devlink_priv(dl);
-
-		bp_dl->bp = bp;
-	}
-}
-
-int bnxt_dl_register(struct bnxt *bp);
-void bnxt_dl_unregister(struct bnxt *bp);
 void bnxt_vf_reps_destroy(struct bnxt *bp);
 void bnxt_vf_reps_close(struct bnxt *bp);
 void bnxt_vf_reps_open(struct bnxt *bp);
@@ -53,16 +28,10 @@ static inline u16 bnxt_vf_rep_get_fid(struct net_device *dev)
 	return bp->pf.vf[vf_rep->vf_idx].fw_fid;
 }
 
-#else
-
-static inline int bnxt_dl_register(struct bnxt *bp)
-{
-	return 0;
-}
+int bnxt_dl_eswitch_mode_get(struct devlink *devlink, u16 *mode);
+int bnxt_dl_eswitch_mode_set(struct devlink *devlink, u16 mode);
 
-static inline void bnxt_dl_unregister(struct bnxt *bp)
-{
-}
+#else
 
 static inline void bnxt_vf_reps_close(struct bnxt *bp)
 {
-- 
2.7.4

Re: [PATCH 6/7] bnxt: Move generic devlink code to new file

[Jiri Pirko]

Tue, Oct 17, 2017 at 10:44:28PM CEST, steven.lin1@broadcom.com wrote:
>Moving generic devlink code (registration) out of VR-R code
>into new bnxt_devlink file.

You can send this patch separatelly and let it be applied before the
patchset.

Re: [PATCH 6/7] bnxt: Move generic devlink code to new file

[Steve Lin]

On Wed, Oct 18, 2017 at 3:33 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Tue, Oct 17, 2017 at 10:44:28PM CEST, steven.lin1@broadcom.com wrote:
>>Moving generic devlink code (registration) out of VR-R code
>>into new bnxt_devlink file.
>
> You can send this patch separatelly and let it be applied before the
> patchset.

Ok, will do.  Thanks again for all the feedback, Jiri.

Steve

[PATCH 7/7] bnxt: Add devlink support for config get/set

[Steve Lin]

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

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

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index f3f6aa8..e247cae 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -14,11 +14,309 @@
 #include "bnxt_vfr.h"
 #include "bnxt_devlink.h"
 
-static const struct devlink_ops bnxt_dl_ops = {
+struct bnxt_drv_cfgparam bnxt_drv_cfgparam_list[] = {
+	{DEVLINK_ATTR_PERM_CFG_MAX_NUM_PF_MSIX_VECT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 10, 108},
+	{DEVLINK_ATTR_PERM_CFG_IGNORE_ARI_CAPABILITY, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 1, 164},
+	{DEVLINK_ATTR_PERM_CFG_PME_CAPABILITY_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 1, 166},
+	{DEVLINK_ATTR_PERM_CFG_LLDP_NEAREST_BRIDGE_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 1, 269},
+	{DEVLINK_ATTR_PERM_CFG_LLDP_NEAREST_NONTPMR_BRIDGE_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 1, 270},
+	{DEVLINK_ATTR_PERM_CFG_SECURE_NIC_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 1, 162},
+	{DEVLINK_ATTR_PERM_CFG_PHY_SELECT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 1, 329},
+	{DEVLINK_ATTR_PERM_CFG_SRIOV_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 1, 401},
+
+	{DEVLINK_ATTR_PERM_CFG_MBA_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 1, 351},
+	{DEVLINK_ATTR_PERM_CFG_MBA_BOOT_TYPE, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 2, 352},
+	{DEVLINK_ATTR_PERM_CFG_MBA_DELAY_TIME, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 4, 353},
+	{DEVLINK_ATTR_PERM_CFG_MBA_SETUP_HOT_KEY, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 1, 354},
+	{DEVLINK_ATTR_PERM_CFG_MBA_HIDE_SETUP_PROMPT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 1, 355},
+	{DEVLINK_ATTR_PERM_CFG_MBA_VLAN_TAG, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 16, 357},
+	{DEVLINK_ATTR_PERM_CFG_MBA_VLAN_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 1, 358},
+	{DEVLINK_ATTR_PERM_CFG_MBA_LINK_SPEED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 4, 359},
+	{DEVLINK_ATTR_PERM_CFG_MBA_BOOT_RETRY_COUNT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 3, 360},
+	{DEVLINK_ATTR_PERM_CFG_MBA_BOOT_PROTOCOL, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 3, 361},
+	{DEVLINK_ATTR_PERM_CFG_NUM_VF_PER_PF, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 8, 404},
+	{DEVLINK_ATTR_PERM_CFG_MSIX_VECTORS_PER_VF, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 10, 406},
+	{DEVLINK_ATTR_PERM_CFG_NPAR_BW_RESERVATION, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 10, 501},
+	{DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 10, 502},
+	{DEVLINK_ATTR_PERM_CFG_RDMA_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 1, 506},
+	{DEVLINK_ATTR_PERM_CFG_NPAR_BW_IN_PERCENT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 1, 507},
+	{DEVLINK_ATTR_PERM_CFG_NPAR_BW_RESERVATION_VALID, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 1, 508},
+	{DEVLINK_ATTR_PERM_CFG_NPAR_BW_LIMIT_VALID, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 1, 509},
+
+	{DEVLINK_ATTR_PERM_CFG_MAGIC_PACKET_WOL_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_PORT, 1, 152},
+	{DEVLINK_ATTR_PERM_CFG_DCBX_MODE, BNXT_DRV_PF,
+		BNXT_DRV_APPL_PORT, 4, 155},
+	{DEVLINK_ATTR_PERM_CFG_MULTIFUNC_MODE, BNXT_DRV_PF,
+		BNXT_DRV_APPL_PORT, 5, 157},
+	{DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D0, BNXT_DRV_PF,
+		BNXT_DRV_APPL_PORT, 4, 205},
+	{DEVLINK_ATTR_PERM_CFG_EEE_PWR_SAVE_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_PORT, 1, 208},
+	{DEVLINK_ATTR_PERM_CFG_PRE_OS_LINK_SPEED_D3, BNXT_DRV_PF,
+		BNXT_DRV_APPL_PORT, 4, 210},
+	{DEVLINK_ATTR_PERM_CFG_MEDIA_AUTO_DETECT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_PORT, 1, 213},
+	{DEVLINK_ATTR_PERM_CFG_AUTONEG_PROTOCOL, BNXT_DRV_PF,
+		BNXT_DRV_APPL_PORT, 8, 312},
+	{DEVLINK_ATTR_PERM_CFG_NPAR_NUM_PARTITIONS_PER_PORT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_PORT, 8, 503},
+};
+
+#define BNXT_NUM_DRV_CFGPARAM ARRAY_SIZE(bnxt_drv_cfgparam_list)
+
+static int bnxt_nvm_read(struct bnxt *bp, int nvm_param, int idx,
+			 void *buf, int size)
+{
+	struct hwrm_nvm_get_variable_input req = {0};
+	void *dest_data_addr = NULL;
+	dma_addr_t dest_data_dma_addr;
+	int rc;
+	int bytesize;
+
+	bytesize = (size + 7) / 8;
+	dest_data_addr = dma_alloc_coherent(&bp->pdev->dev, bytesize,
+					    &dest_data_dma_addr, GFP_KERNEL);
+	if (!dest_data_addr) {
+		netdev_err(bp->dev, "dma_alloc_coherent failure\n");
+		return -ENOMEM;
+	}
+
+	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_NVM_GET_VARIABLE, -1, -1);
+	req.dest_data_addr = cpu_to_le64(dest_data_dma_addr);
+	req.data_len = cpu_to_le16(size);
+	req.option_num = cpu_to_le16(nvm_param);
+	req.index_0 = cpu_to_le16(idx);
+	if (idx != 0)
+		req.dimensions = cpu_to_le16(1);
+
+	rc = _hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);
+
+	memcpy(buf, dest_data_addr, bytesize);
+
+	dma_free_coherent(&bp->pdev->dev, bytesize, dest_data_addr,
+			  dest_data_dma_addr);
+
+	return rc;
+}
+
+static int bnxt_nvm_write(struct bnxt *bp, int nvm_param, int idx,
+			  const void *buf, int size)
+{
+	struct hwrm_nvm_set_variable_input req = {0};
+	void *src_data_addr = NULL;
+	dma_addr_t src_data_dma_addr;
+	int rc;
+	int bytesize;
+
+	bytesize = (size + 7) / 8;
+
+	src_data_addr = dma_alloc_coherent(&bp->pdev->dev, bytesize,
+					   &src_data_dma_addr, GFP_KERNEL);
+	if (!src_data_addr) {
+		netdev_err(bp->dev, "dma_alloc_coherent failure\n");
+		return -ENOMEM;
+	}
+
+	memcpy(src_data_addr, buf, bytesize);
+
+	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_NVM_SET_VARIABLE, -1, -1);
+	req.src_data_addr = cpu_to_le64(src_data_dma_addr);
+	req.data_len = cpu_to_le16(size);
+	req.option_num = cpu_to_le16(nvm_param);
+	req.index_0 = cpu_to_le16(idx);
+	if (idx != 0)
+		req.dimensions = cpu_to_le16(1);
+
+	rc = _hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);
+
+	dma_free_coherent(&bp->pdev->dev, bytesize, src_data_addr,
+			  src_data_dma_addr);
+
+	return 0;
+}
+
+static int bnxt_dl_config_set(struct devlink *devlink,
+			      enum devlink_attr attr, u32 value,
+			      u8 *restart_reqd)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
+	int i;
+	int idx = 0;
+	void *data;
+	int ret = 0;
+	u32 bytesize;
+	struct bnxt_drv_cfgparam *entry;
+
+	*restart_reqd = 0;
+
+	/* Find parameter in table */
+	for (i = 0; i < BNXT_NUM_DRV_CFGPARAM; i++) {
+		if (attr == bnxt_drv_cfgparam_list[i].attr) {
+			entry = &bnxt_drv_cfgparam_list[i];
+			break;
+		}
+	}
+
+	/* Not found */
+	if (i == BNXT_NUM_DRV_CFGPARAM)
+		return -EINVAL;
+
+	/* Check to see if this func type can access variable */
+	if (BNXT_PF(bp) && !(entry->func & BNXT_DRV_PF))
+		return -EOPNOTSUPP;
+	if (BNXT_VF(bp) && !(entry->func & BNXT_DRV_VF))
+		return -EOPNOTSUPP;
+
+	/* If parameter is per port or function, compute index */
+	if (entry->appl == BNXT_DRV_APPL_PORT) {
+		idx = bp->pf.port_id;
+	} else if (entry->appl == BNXT_DRV_APPL_FUNCTION) {
+		if (BNXT_PF(bp))
+			idx = bp->pf.fw_fid - BNXT_FIRST_PF_FID;
+#ifdef CONFIG_BNXT_SRIOV
+		else
+			idx = bp->vf.fw_fid - BNXT_FIRST_VF_FID;
+#endif /* CONFIG_BNXT_SRIOV */
+	}
+
+	bytesize = (entry->bitlength + 7) / 8;
+	data = kmalloc(bytesize, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	if (bytesize == 1) {
+		u8 val8 = (value & 0xff);
+
+		memcpy(data, &val8, sizeof(u8));
+	} else if (bytesize == 2) {
+		u16 val16 = (value & 0xffff);
+
+		memcpy(data, &val16, sizeof(u16));
+	} else {
+		memcpy(data, &value, sizeof(u32));
+	}
+
+	ret = bnxt_nvm_write(bp, entry->nvm_param, idx, data,
+			     entry->bitlength);
+
+	/* Restart required for all nvm parameter writes */
+	*restart_reqd = 1;
+
+	kfree(data);
+
+	return ret;
+}
+
+static int bnxt_dl_config_get(struct devlink *devlink,
+			      enum devlink_attr attr, u32 *value)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
+
+	int i;
+	int idx = 0;
+	void *data;
+	int ret = 0;
+	u32 bytesize;
+	struct bnxt_drv_cfgparam *entry;
+
+	/* Find parameter in table */
+	for (i = 0; i < BNXT_NUM_DRV_CFGPARAM; i++) {
+		if (attr == bnxt_drv_cfgparam_list[i].attr) {
+			entry = &bnxt_drv_cfgparam_list[i];
+			break;
+		}
+	}
+
+	/* Not found */
+	if (i == BNXT_NUM_DRV_CFGPARAM)
+		return -EINVAL;
+
+	/* Check to see if this func type can access variable */
+	if (BNXT_PF(bp) && !(entry->func & BNXT_DRV_PF))
+		return -EOPNOTSUPP;
+	if (BNXT_VF(bp) && !(entry->func & BNXT_DRV_VF))
+		return -EOPNOTSUPP;
+
+	/* If parameter is per port or function, compute index */
+	if (entry->appl == BNXT_DRV_APPL_PORT) {
+		idx = bp->pf.port_id;
+	} else if (entry->appl == BNXT_DRV_APPL_FUNCTION) {
+		if (BNXT_PF(bp))
+			idx = bp->pf.fw_fid - BNXT_FIRST_PF_FID;
+#ifdef CONFIG_BNXT_SRIOV
+		else
+			idx = bp->vf.fw_fid - BNXT_FIRST_VF_FID;
+#endif /* CONFIG_BNXT_SRIOV */
+	}
+
+	/* Allocate space, retrieve value, and copy to result */
+	bytesize = (entry->bitlength + 7) / 8;
+	data = kmalloc(bytesize, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	ret = bnxt_nvm_read(bp, entry->nvm_param, idx, data, entry->bitlength);
+
+	if (ret) {
+		kfree(data);
+		return ret;
+	}
+
+	if (bytesize == 1) {
+		u8 val;
+
+		memcpy(&val, data, sizeof(u8));
+		*value = val;
+	} else if (bytesize == 2) {
+		u16 val;
+
+		memcpy(&val, data, sizeof(u16));
+		*value = val;
+	} else {
+		u32 val;
+
+		memcpy(&val, data, sizeof(u32));
+		*value = val;
+	}
+
+	kfree(data);
+
+	return 0;
+}
+
+static struct devlink_ops bnxt_dl_ops = {
 #ifdef CONFIG_BNXT_SRIOV
 	.eswitch_mode_set = bnxt_dl_eswitch_mode_set,
 	.eswitch_mode_get = bnxt_dl_eswitch_mode_get,
 #endif /* CONFIG_BNXT_SRIOV */
+	.config_get = bnxt_dl_config_get,
+	.config_set = bnxt_dl_config_set,
 };
 
 int bnxt_dl_register(struct bnxt *bp)
@@ -26,12 +324,12 @@ int bnxt_dl_register(struct bnxt *bp)
 	struct devlink *dl;
 	int rc;
 
-	if (!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV))
-		return 0;
-
-	if (bp->hwrm_spec_code < 0x10800) {
+	if ((!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV)) ||
+	    bp->hwrm_spec_code < 0x10800) {
+		/* eswitch switchdev mode not supported */
+		bnxt_dl_ops.eswitch_mode_set = NULL;
+		bnxt_dl_ops.eswitch_mode_get = NULL;
 		netdev_warn(bp->dev, "Firmware does not support SR-IOV E-Switch SWITCHDEV mode.\n");
-		return -ENOTSUPP;
 	}
 
 	dl = devlink_alloc(&bnxt_dl_ops, sizeof(struct bnxt_dl));
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
index e92a35d..ee28890 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_attr	attr;
+	u8			func; /* BNXT_DRV_PF | BNXT_DRV_VF */
+	enum bnxt_drv_appl	appl; /* applicability (shared, func, port) */
+	u32			bitlength; /* length, in bits */
+	u32			nvm_param;
+};
+
 /* Struct to hold housekeeping info needed by devlink interface */
 struct bnxt_dl {
 	struct bnxt *bp;	/* back ptr to the controlling dev */
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
index cb04cc7..8c25731 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
@@ -5676,6 +5676,106 @@ struct hwrm_nvm_install_update_cmd_err {
 	u8 unused_0[7];
 };
 
+/* hwrm_nvm_get_variable */
+/* Input (40 bytes) */
+struct hwrm_nvm_get_variable_input {
+	__le16 req_type;
+	__le16 cmpl_ring;
+	__le16 seq_id;
+	__le16 target_id;
+	__le64 resp_addr;
+	__le64 dest_data_addr;
+	__le16 data_len;
+	__le16 option_num;
+	#define NVM_GET_VARIABLE_REQ_OPTION_NUM_RSVD_0		   0x0UL
+	#define NVM_GET_VARIABLE_REQ_OPTION_NUM_RSVD_FFFF	   0xffffUL
+	__le16 dimensions;
+	__le16 index_0;
+	__le16 index_1;
+	__le16 index_2;
+	__le16 index_3;
+	u8 flags;
+	#define NVM_GET_VARIABLE_REQ_FLAGS_FACTORY_DFLT	    0x1UL
+	u8 unused_0;
+};
+
+/* Output (16 bytes) */
+struct hwrm_nvm_get_variable_output {
+	__le16 error_code;
+	__le16 req_type;
+	__le16 seq_id;
+	__le16 resp_len;
+	__le16 data_len;
+	__le16 option_num;
+	#define NVM_GET_VARIABLE_RESP_OPTION_NUM_RSVD_0	   0x0UL
+	#define NVM_GET_VARIABLE_RESP_OPTION_NUM_RSVD_FFFF	   0xffffUL
+	u8 unused_0;
+	u8 unused_1;
+	u8 unused_2;
+	u8 valid;
+};
+
+/* Command specific Error Codes (8 bytes) */
+struct hwrm_nvm_get_variable_cmd_err {
+	u8 code;
+	#define NVM_GET_VARIABLE_CMD_ERR_CODE_UNKNOWN		   0x0UL
+	#define NVM_GET_VARIABLE_CMD_ERR_CODE_VAR_NOT_EXIST       0x1UL
+	#define NVM_GET_VARIABLE_CMD_ERR_CODE_CORRUPT_VAR	   0x2UL
+	#define NVM_GET_VARIABLE_CMD_ERR_CODE_LEN_TOO_SHORT       0x3UL
+	u8 unused_0[7];
+};
+
+/* hwrm_nvm_set_variable */
+/* Input (40 bytes) */
+struct hwrm_nvm_set_variable_input {
+	__le16 req_type;
+	__le16 cmpl_ring;
+	__le16 seq_id;
+	__le16 target_id;
+	__le64 resp_addr;
+	__le64 src_data_addr;
+	__le16 data_len;
+	__le16 option_num;
+	#define NVM_SET_VARIABLE_REQ_OPTION_NUM_RSVD_0		   0x0UL
+	#define NVM_SET_VARIABLE_REQ_OPTION_NUM_RSVD_FFFF	   0xffffUL
+	__le16 dimensions;
+	__le16 index_0;
+	__le16 index_1;
+	__le16 index_2;
+	__le16 index_3;
+	u8 flags;
+	#define NVM_SET_VARIABLE_REQ_FLAGS_FORCE_FLUSH		    0x1UL
+	#define NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_MASK       0xeUL
+	#define NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_SFT	    1
+	#define NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_NONE      (0x0UL << 1)
+	#define NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_HMAC_SHA1 (0x1UL << 1)
+	#define NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_LAST \
+		NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_HMAC_SHA1
+	u8 unused_0;
+};
+
+/* Output (16 bytes) */
+struct hwrm_nvm_set_variable_output {
+	__le16 error_code;
+	__le16 req_type;
+	__le16 seq_id;
+	__le16 resp_len;
+	__le32 unused_0;
+	u8 unused_1;
+	u8 unused_2;
+	u8 unused_3;
+	u8 valid;
+};
+
+/* Command specific Error Codes (8 bytes) */
+struct hwrm_nvm_set_variable_cmd_err {
+	u8 code;
+	#define NVM_SET_VARIABLE_CMD_ERR_CODE_UNKNOWN		   0x0UL
+	#define NVM_SET_VARIABLE_CMD_ERR_CODE_VAR_NOT_EXIST       0x1UL
+	#define NVM_SET_VARIABLE_CMD_ERR_CODE_CORRUPT_VAR	   0x2UL
+	u8 unused_0[7];
+};
+
 /* hwrm_selftest_qlist */
 /* Input (16 bytes) */
 struct hwrm_selftest_qlist_input {
-- 
2.7.4

Re: [PATCH 0/7] Adding permanent config get/set to devlink

[Steve Lin]

My apologies - this patchset was intended for net-next; I forgot to
add that to the subject line, though.

Steve

On Tue, Oct 17, 2017 at 4:44 PM, Steve Lin <steven.lin1@broadcom.com> wrote:
> DIFFERENCES FROM RFC:
> Implemented most of the changes suggested by Jiri and others.
> Thanks for the valuable feedback!
>
> Adds a devlink command for getting & setting permanent
> (persistent / NVRAM) device configuration parameters, and
> enumerates the parameters as nested devlink attributes.
>
> bnxt driver patches make use of these new devlink cmds/
> attributes.
>
> Steve Lin (7):
>   devlink: Add permanent config parameter get/set operations
>   devlink: Adding NPAR permanent config parameters
>   devlink: Adding high level dev perm config params
>   devlink: Adding perm config of link settings
>   devlink: Adding pre-boot permanent config parameters
>   bnxt: Move generic devlink code to new file
>   bnxt: Add devlink support for config get/set
>
>  drivers/net/ethernet/broadcom/bnxt/Makefile       |   2 +-
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c         |   1 +
>  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 363 ++++++++++++++++++++++
>  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  56 ++++
>  drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h     | 100 ++++++
>  drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c     |  53 +---
>  drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h     |  37 +--
>  include/net/devlink.h                             |   4 +
>  include/uapi/linux/devlink.h                      | 113 +++++++
>  net/core/devlink.c                                | 300 ++++++++++++++++++
>  10 files changed, 944 insertions(+), 85 deletions(-)
>  create mode 100644 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>  create mode 100644 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
>
> --
> 2.7.4
>