[RFC 0/3] Adding config get/set to devlink

[RFC 0/3] Adding config get/set to devlink

[Steve Lin]

Adds a devlink command for getting & setting device configuration
parameters, and enumerates a bunch of those parameters as devlink
attributes.  Also introduces an attribute that can be set by a
driver to indicate that the config change doesn't take effect
until the next restart (as in the case of the bnxt driver changes
in this patchset, for which all the configuration changes affect NVM
only, and aren't loaded until the next restart.)

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

Steve Lin (3):
  devlink: Add config parameter get/set operations
  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 | 403 ++++++++++++++++++++++
 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     |  97 +-----
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h     |  35 +-
 include/net/devlink.h                             |   4 +
 include/uapi/linux/devlink.h                      | 108 ++++++
 net/core/devlink.c                                | 207 +++++++++++
 10 files changed, 882 insertions(+), 131 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

[RFC 1/3] devlink: Add config parameter get/set operations

[Steve Lin]

Add support for config parameter get/set commands. Initially used by
bnxt driver, but other drivers can use the same, or new, attributes.
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.

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 | 108 ++++++++++++++++++++++
 net/core/devlink.c           | 207 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 319 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..e959716 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
@@ -124,6 +127,68 @@ 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_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_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,
@@ -202,6 +267,49 @@ enum devlink_attr {
 
 	DEVLINK_ATTR_ESWITCH_ENCAP_MODE,	/* u8 */
 
+	/* Configuration Parameters */
+	DEVLINK_ATTR_SRIOV_ENABLED,		/* u8 */
+	DEVLINK_ATTR_NUM_VF_PER_PF,		/* u32 */
+	DEVLINK_ATTR_MAX_NUM_PF_MSIX_VECT,	/* u32 */
+	DEVLINK_ATTR_MSIX_VECTORS_PER_VF,	/* u32 */
+	DEVLINK_ATTR_NPAR_NUM_PARTITIONS_PER_PORT,	/* u32 */
+	DEVLINK_ATTR_NPAR_BW_IN_PERCENT,	/* u8 */
+	DEVLINK_ATTR_NPAR_BW_RESERVATION,	/* u8 */
+	DEVLINK_ATTR_NPAR_BW_RESERVATION_VALID,	/* u8 */
+	DEVLINK_ATTR_NPAR_BW_LIMIT,		/* u8 */
+	DEVLINK_ATTR_NPAR_BW_LIMIT_VALID,	/* u8 */
+	DEVLINK_ATTR_DCBX_MODE,			/* u8 */
+	DEVLINK_ATTR_RDMA_ENABLED,		/* u8 */
+	DEVLINK_ATTR_MULTIFUNC_MODE,		/* u8 */
+	DEVLINK_ATTR_SECURE_NIC_ENABLED,	/* u8 */
+	DEVLINK_ATTR_IGNORE_ARI_CAPABILITY,	/* u8 */
+	DEVLINK_ATTR_LLDP_NEAREST_BRIDGE_ENABLED,	/* u8 */
+	DEVLINK_ATTR_LLDP_NEAREST_NONTPMR_BRIDGE_ENABLED,	/* u8 */
+	DEVLINK_ATTR_PME_CAPABILITY_ENABLED,	/* u8 */
+	DEVLINK_ATTR_MAGIC_PACKET_WOL_ENABLED,	/* u8 */
+	DEVLINK_ATTR_EEE_PWR_SAVE_ENABLED,	/* u8 */
+	DEVLINK_ATTR_AUTONEG_PROTOCOL,		/* u8 */
+	DEVLINK_ATTR_MEDIA_AUTO_DETECT,		/* u8 */
+	DEVLINK_ATTR_PHY_SELECT,		/* u8 */
+	DEVLINK_ATTR_PRE_OS_LINK_SPEED_D0,	/* u8 */
+	DEVLINK_ATTR_PRE_OS_LINK_SPEED_D3,	/* u8 */
+	DEVLINK_ATTR_MBA_ENABLED,		/* u8 */
+	DEVLINK_ATTR_MBA_BOOT_TYPE,		/* u8 */
+	DEVLINK_ATTR_MBA_DELAY_TIME,		/* u32 */
+	DEVLINK_ATTR_MBA_SETUP_HOT_KEY,		/* u8 */
+	DEVLINK_ATTR_MBA_HIDE_SETUP_PROMPT,	/* u8 */
+	DEVLINK_ATTR_MBA_BOOT_RETRY_COUNT,	/* u32 */
+	DEVLINK_ATTR_MBA_VLAN_ENABLED,		/* u8 */
+	DEVLINK_ATTR_MBA_VLAN_TAG,		/* u16 */
+	DEVLINK_ATTR_MBA_BOOT_PROTOCOL,		/* u8 */
+	DEVLINK_ATTR_MBA_LINK_SPEED,		/* u8 */
+
+	/* 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_RESTART_REQUIRED,		/* u8 */
+
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 7d430c1..701c84b 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -1566,6 +1566,164 @@ 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_config_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 = -1;
+	u32 value;
+	int i;
+	struct nla_policy policy;
+	u8 restart_reqd;
+
+	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;
+
+	for (i = 0; i < DEVLINK_ATTR_MAX; i++)
+		if (info->attrs[i])
+			attr = i;
+
+	if (attr == -1) {
+		/* Not found - invalid/unknown attribute? */
+		err = -EINVAL;
+		goto nla_put_failure;
+	}
+
+	policy = devlink_nl_policy[attr];
+
+	if (cmd == DEVLINK_CMD_CONFIG_GET) {
+		err = ops->config_get(devlink, attr, &value);
+
+		if (err)
+			goto nla_put_failure;
+
+		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:
+			goto nla_put_failure;
+		}
+
+		if (err)
+			goto nla_put_failure;
+	} else {
+		/* Must be config_set command */
+		u8 *val8;
+		u16 *val16;
+		u32 *val32;
+
+		switch (policy.type) {
+		case NLA_U8:
+			val8 = nla_data(info->attrs[attr]);
+			value = *val8;
+			break;
+		case NLA_U16:
+			val16 = nla_data(info->attrs[attr]);
+			value = *val16;
+			break;
+		case NLA_U32:
+			val32 = nla_data(info->attrs[attr]);
+			value = *val32;
+			break;
+		default:
+			goto nla_put_failure;
+		}
+
+		err = ops->config_set(devlink, attr, value, &restart_reqd);
+		if (err)
+			goto nla_put_failure;
+
+		if (restart_reqd) {
+			err = nla_put_u8(msg, DEVLINK_ATTR_RESTART_REQUIRED,
+					 restart_reqd);
+			if (err)
+				goto nla_put_failure;
+		}
+	}
+
+	genlmsg_end(msg, hdr);
+	return 0;
+
+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_fill(msg, devlink, DEVLINK_CMD_CONFIG_GET,
+				     info);
+
+	if (err) {
+		nlmsg_free(msg);
+		return err;
+	}
+
+	return genlmsg_reply(msg, info);
+}
+
+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;
+	int err;
+
+	if (!devlink->ops)
+		return -EOPNOTSUPP;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	err = devlink_nl_config_fill(msg, devlink, DEVLINK_CMD_CONFIG_SET,
+				     info);
+
+	if (err) {
+		nlmsg_free(msg);
+		return err;
+	}
+
+	return genlmsg_reply(msg, info);
+}
+
 int devlink_dpipe_match_put(struct sk_buff *skb,
 			    struct devlink_dpipe_match *match)
 {
@@ -2291,6 +2449,41 @@ 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_SRIOV_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_NUM_VF_PER_PF] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_MAX_NUM_PF_MSIX_VECT] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_MSIX_VECTORS_PER_VF] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_NPAR_NUM_PARTITIONS_PER_PORT] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_NPAR_BW_IN_PERCENT] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_NPAR_BW_RESERVATION] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_NPAR_BW_RESERVATION_VALID] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_NPAR_BW_LIMIT] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_NPAR_BW_LIMIT_VALID] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_DCBX_MODE] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_RDMA_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_MULTIFUNC_MODE] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_SECURE_NIC_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_IGNORE_ARI_CAPABILITY] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_LLDP_NEAREST_BRIDGE_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_LLDP_NEAREST_NONTPMR_BRIDGE_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PME_CAPABILITY_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_MAGIC_PACKET_WOL_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_EEE_PWR_SAVE_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_AUTONEG_PROTOCOL] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_MEDIA_AUTO_DETECT] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PHY_SELECT] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PRE_OS_LINK_SPEED_D0] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PRE_OS_LINK_SPEED_D3] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_MBA_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_MBA_BOOT_TYPE] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_MBA_DELAY_TIME] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_MBA_SETUP_HOT_KEY] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_MBA_HIDE_SETUP_PROMPT] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_MBA_BOOT_RETRY_COUNT] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_MBA_VLAN_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_MBA_VLAN_TAG] = { .type = NLA_U16 },
+	[DEVLINK_ATTR_MBA_BOOT_PROTOCOL] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_MBA_LINK_SPEED] = { .type = NLA_U32 },
 };
 
 static const struct genl_ops devlink_nl_ops[] = {
@@ -2451,6 +2644,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

[RFC 2/3] bnxt: Move generic devlink code to new file

[Steve Lin]

Moving generic devlink code (registration, etc.) out of VF-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 | 109 ++++++++++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  39 ++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c     |  97 +------------------
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h     |  35 +------
 6 files changed, 152 insertions(+), 131 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..d159cce
--- /dev/null
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -0,0 +1,109 @@
+/* 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 int bnxt_dl_eswitch_mode_get(struct devlink *devlink, u16 *mode)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
+
+	*mode = bp->eswitch_mode;
+	return 0;
+}
+
+static int bnxt_dl_eswitch_mode_set(struct devlink *devlink, u16 mode)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
+	int rc = 0;
+
+	mutex_lock(&bp->sriov_lock);
+	if (bp->eswitch_mode == mode) {
+		netdev_info(bp->dev, "already in %s eswitch mode",
+			    mode == DEVLINK_ESWITCH_MODE_LEGACY ?
+			    "legacy" : "switchdev");
+		rc = -EINVAL;
+		goto done;
+	}
+
+	switch (mode) {
+	case DEVLINK_ESWITCH_MODE_LEGACY:
+		bnxt_vf_reps_destroy(bp);
+		break;
+
+	case DEVLINK_ESWITCH_MODE_SWITCHDEV:
+		if (pci_num_vf(bp->pdev) == 0) {
+			netdev_info(bp->dev,
+				    "Enable VFs before setting switchdev mode");
+			rc = -EPERM;
+			goto done;
+		}
+		rc = bnxt_vf_reps_create(bp);
+		break;
+
+	default:
+		rc = -EINVAL;
+		goto done;
+	}
+done:
+	mutex_unlock(&bp->sriov_lock);
+	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);
+}
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..1ed2bcf 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
@@ -338,7 +338,7 @@ static void bnxt_vf_rep_netdev_init(struct bnxt *bp, struct bnxt_vf_rep *vf_rep,
 	ether_addr_copy(dev->dev_addr, dev->perm_addr);
 }
 
-static int bnxt_vf_reps_create(struct bnxt *bp)
+int bnxt_vf_reps_create(struct bnxt *bp)
 {
 	u16 *cfa_code_map = NULL, num_vfs = pci_num_vf(bp->pdev);
 	struct bnxt_vf_rep *vf_rep;
@@ -415,99 +415,4 @@ static int bnxt_vf_reps_create(struct bnxt *bp)
 	return rc;
 }
 
-/* Devlink related routines */
-static int bnxt_dl_eswitch_mode_get(struct devlink *devlink, u16 *mode)
-{
-	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
-
-	*mode = bp->eswitch_mode;
-	return 0;
-}
-
-static int bnxt_dl_eswitch_mode_set(struct devlink *devlink, u16 mode)
-{
-	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
-	int rc = 0;
-
-	mutex_lock(&bp->sriov_lock);
-	if (bp->eswitch_mode == mode) {
-		netdev_info(bp->dev, "already in %s eswitch mode",
-			    mode == DEVLINK_ESWITCH_MODE_LEGACY ?
-			    "legacy" : "switchdev");
-		rc = -EINVAL;
-		goto done;
-	}
-
-	switch (mode) {
-	case DEVLINK_ESWITCH_MODE_LEGACY:
-		bnxt_vf_reps_destroy(bp);
-		break;
-
-	case DEVLINK_ESWITCH_MODE_SWITCHDEV:
-		if (pci_num_vf(bp->pdev) == 0) {
-			netdev_info(bp->dev,
-				    "Enable VFs before setting switchdev mode");
-			rc = -EPERM;
-			goto done;
-		}
-		rc = bnxt_vf_reps_create(bp);
-		break;
-
-	default:
-		rc = -EINVAL;
-		goto done;
-	}
-done:
-	mutex_unlock(&bp->sriov_lock);
-	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..1b67ce1 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.h
@@ -14,36 +14,12 @@
 
 #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);
 void bnxt_vf_rep_rx(struct bnxt *bp, struct sk_buff *skb);
 struct net_device *bnxt_get_vf_rep(struct bnxt *bp, u16 cfa_code);
+int bnxt_vf_reps_create(struct bnxt *bp);
 
 static inline u16 bnxt_vf_rep_get_fid(struct net_device *dev)
 {
@@ -55,15 +31,6 @@ static inline u16 bnxt_vf_rep_get_fid(struct net_device *dev)
 
 #else
 
-static inline int bnxt_dl_register(struct bnxt *bp)
-{
-	return 0;
-}
-
-static inline void bnxt_dl_unregister(struct bnxt *bp)
-{
-}
-
 static inline void bnxt_vf_reps_close(struct bnxt *bp)
 {
 }
-- 
2.7.4

[RFC 3/3] 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 | 306 +++++++++++++++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  17 ++
 drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h     | 100 +++++++
 3 files changed, 417 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index d159cce..32319d5 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -14,6 +14,83 @@
 #include "bnxt_vfr.h"
 #include "bnxt_devlink.h"
 
+struct bnxt_drv_cfgparam bnxt_drv_cfgparam_list[] = {
+	{DEVLINK_ATTR_MAX_NUM_PF_MSIX_VECT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 10, 108},
+	{DEVLINK_ATTR_IGNORE_ARI_CAPABILITY, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 1, 164},
+	{DEVLINK_ATTR_PME_CAPABILITY_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 1, 166},
+	{DEVLINK_ATTR_LLDP_NEAREST_BRIDGE_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 1, 269},
+	{DEVLINK_ATTR_LLDP_NEAREST_NONTPMR_BRIDGE_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 1, 270},
+	{DEVLINK_ATTR_SECURE_NIC_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 1, 162},
+	{DEVLINK_ATTR_PHY_SELECT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 1, 329},
+	{DEVLINK_ATTR_SRIOV_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 1, 401},
+
+	{DEVLINK_ATTR_MBA_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 1, 351},
+	{DEVLINK_ATTR_MBA_BOOT_TYPE, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 2, 352},
+	{DEVLINK_ATTR_MBA_DELAY_TIME, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 4, 353},
+	{DEVLINK_ATTR_MBA_SETUP_HOT_KEY, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 1, 354},
+	{DEVLINK_ATTR_MBA_HIDE_SETUP_PROMPT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 1, 355},
+	{DEVLINK_ATTR_MBA_VLAN_TAG, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 16, 357},
+	{DEVLINK_ATTR_MBA_VLAN_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 1, 358},
+	{DEVLINK_ATTR_MBA_LINK_SPEED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 4, 359},
+	{DEVLINK_ATTR_MBA_BOOT_RETRY_COUNT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 3, 360},
+	{DEVLINK_ATTR_MBA_BOOT_PROTOCOL, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 3, 361},
+	{DEVLINK_ATTR_NUM_VF_PER_PF, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 8, 404},
+	{DEVLINK_ATTR_MSIX_VECTORS_PER_VF, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 10, 406},
+	{DEVLINK_ATTR_NPAR_BW_RESERVATION, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 10, 501},
+	{DEVLINK_ATTR_NPAR_BW_LIMIT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 10, 502},
+	{DEVLINK_ATTR_RDMA_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 1, 506},
+	{DEVLINK_ATTR_NPAR_BW_IN_PERCENT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 1, 507},
+	{DEVLINK_ATTR_NPAR_BW_RESERVATION_VALID, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 1, 508},
+	{DEVLINK_ATTR_NPAR_BW_LIMIT_VALID, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 1, 509},
+
+	{DEVLINK_ATTR_MAGIC_PACKET_WOL_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_PORT, 1, 152},
+	{DEVLINK_ATTR_DCBX_MODE, BNXT_DRV_PF,
+		BNXT_DRV_APPL_PORT, 4, 155},
+	{DEVLINK_ATTR_MULTIFUNC_MODE, BNXT_DRV_PF,
+		BNXT_DRV_APPL_PORT, 5, 157},
+	{DEVLINK_ATTR_PRE_OS_LINK_SPEED_D0, BNXT_DRV_PF,
+		BNXT_DRV_APPL_PORT, 4, 205},
+	{DEVLINK_ATTR_EEE_PWR_SAVE_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_PORT, 1, 208},
+	{DEVLINK_ATTR_PRE_OS_LINK_SPEED_D3, BNXT_DRV_PF,
+		BNXT_DRV_APPL_PORT, 4, 210},
+	{DEVLINK_ATTR_MEDIA_AUTO_DETECT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_PORT, 1, 213},
+	{DEVLINK_ATTR_AUTONEG_PROTOCOL, BNXT_DRV_PF,
+		BNXT_DRV_APPL_PORT, 8, 312},
+	{DEVLINK_ATTR_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_dl_eswitch_mode_get(struct devlink *devlink, u16 *mode)
 {
 	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
@@ -60,9 +137,226 @@ static int bnxt_dl_eswitch_mode_set(struct devlink *devlink, u16 mode)
 	return rc;
 }
 
-static const struct devlink_ops bnxt_dl_ops = {
+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, size);
+
+	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;
+		else
+			idx = bp->vf.fw_fid - BNXT_FIRST_VF_FID;
+	}
+
+	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;
+		else
+			idx = bp->vf.fw_fid - BNXT_FIRST_VF_FID;
+	}
+
+	/* 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 = {
 	.eswitch_mode_set = bnxt_dl_eswitch_mode_set,
 	.eswitch_mode_get = bnxt_dl_eswitch_mode_get,
+	.config_get = bnxt_dl_config_get,
+	.config_set = bnxt_dl_config_set,
 };
 
 int bnxt_dl_register(struct bnxt *bp)
@@ -70,12 +364,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: [RFC 1/3] devlink: Add config parameter get/set operations

[Jiri Pirko]

Thu, Oct 12, 2017 at 03:34:20PM CEST, steven.lin1@broadcom.com wrote:
>Add support for config parameter get/set commands. Initially used by
>bnxt driver, but other drivers can use the same, or new, attributes.
>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.
>

First of all, I like this approach.

I would like to see this patch split into:
1) config-options infrastructure introduction
2) specific config options introductions - would be best to have it
   per-option. We need to make sure every option is very well described
   and explained usecases. This is needed in order vendors to share
   attributes among drivers.

More nits inlined.


>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 | 108 ++++++++++++++++++++++
> net/core/devlink.c           | 207 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 319 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..e959716 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
>@@ -124,6 +127,68 @@ 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_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_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,
>@@ -202,6 +267,49 @@ enum devlink_attr {
> 
> 	DEVLINK_ATTR_ESWITCH_ENCAP_MODE,	/* u8 */
> 
>+	/* Configuration Parameters */
>+	DEVLINK_ATTR_SRIOV_ENABLED,		/* u8 */
>+	DEVLINK_ATTR_NUM_VF_PER_PF,		/* u32 */
>+	DEVLINK_ATTR_MAX_NUM_PF_MSIX_VECT,	/* u32 */
>+	DEVLINK_ATTR_MSIX_VECTORS_PER_VF,	/* u32 */
>+	DEVLINK_ATTR_NPAR_NUM_PARTITIONS_PER_PORT,	/* u32 */
>+	DEVLINK_ATTR_NPAR_BW_IN_PERCENT,	/* u8 */
>+	DEVLINK_ATTR_NPAR_BW_RESERVATION,	/* u8 */
>+	DEVLINK_ATTR_NPAR_BW_RESERVATION_VALID,	/* u8 */
>+	DEVLINK_ATTR_NPAR_BW_LIMIT,		/* u8 */
>+	DEVLINK_ATTR_NPAR_BW_LIMIT_VALID,	/* u8 */
>+	DEVLINK_ATTR_DCBX_MODE,			/* u8 */
>+	DEVLINK_ATTR_RDMA_ENABLED,		/* u8 */
>+	DEVLINK_ATTR_MULTIFUNC_MODE,		/* u8 */
>+	DEVLINK_ATTR_SECURE_NIC_ENABLED,	/* u8 */
>+	DEVLINK_ATTR_IGNORE_ARI_CAPABILITY,	/* u8 */
>+	DEVLINK_ATTR_LLDP_NEAREST_BRIDGE_ENABLED,	/* u8 */
>+	DEVLINK_ATTR_LLDP_NEAREST_NONTPMR_BRIDGE_ENABLED,	/* u8 */
>+	DEVLINK_ATTR_PME_CAPABILITY_ENABLED,	/* u8 */
>+	DEVLINK_ATTR_MAGIC_PACKET_WOL_ENABLED,	/* u8 */
>+	DEVLINK_ATTR_EEE_PWR_SAVE_ENABLED,	/* u8 */
>+	DEVLINK_ATTR_AUTONEG_PROTOCOL,		/* u8 */
>+	DEVLINK_ATTR_MEDIA_AUTO_DETECT,		/* u8 */
>+	DEVLINK_ATTR_PHY_SELECT,		/* u8 */
>+	DEVLINK_ATTR_PRE_OS_LINK_SPEED_D0,	/* u8 */
>+	DEVLINK_ATTR_PRE_OS_LINK_SPEED_D3,	/* u8 */
>+	DEVLINK_ATTR_MBA_ENABLED,		/* u8 */
>+	DEVLINK_ATTR_MBA_BOOT_TYPE,		/* u8 */
>+	DEVLINK_ATTR_MBA_DELAY_TIME,		/* u32 */
>+	DEVLINK_ATTR_MBA_SETUP_HOT_KEY,		/* u8 */
>+	DEVLINK_ATTR_MBA_HIDE_SETUP_PROMPT,	/* u8 */
>+	DEVLINK_ATTR_MBA_BOOT_RETRY_COUNT,	/* u32 */
>+	DEVLINK_ATTR_MBA_VLAN_ENABLED,		/* u8 */
>+	DEVLINK_ATTR_MBA_VLAN_TAG,		/* u16 */
>+	DEVLINK_ATTR_MBA_BOOT_PROTOCOL,		/* u8 */
>+	DEVLINK_ATTR_MBA_LINK_SPEED,		/* u8 */

Okay, I think it is about the time we should start thinking about
putting this new config attributes under nester attribute. What do you
think?


>+
>+	/* 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_RESTART_REQUIRED,		/* u8 */
>+
> 	/* add new attributes above here, update the policy in devlink.c */
> 
> 	__DEVLINK_ATTR_MAX,
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 7d430c1..701c84b 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -1566,6 +1566,164 @@ 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_config_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 = -1;

-1 is not a valid enum value. Better to just have bool to indicate attr
was found or not.


>+	u32 value;
>+	int i;
>+	struct nla_policy policy;
>+	u8 restart_reqd;
>+
>+	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;
>+
>+	for (i = 0; i < DEVLINK_ATTR_MAX; i++)
>+		if (info->attrs[i])
>+			attr = i;
>+
>+	if (attr == -1) {
>+		/* Not found - invalid/unknown attribute? */
>+		err = -EINVAL;
>+		goto nla_put_failure;
>+	}
>+
>+	policy = devlink_nl_policy[attr];
>+
>+	if (cmd == DEVLINK_CMD_CONFIG_GET) {
>+		err = ops->config_get(devlink, attr, &value);
>+
>+		if (err)
>+			goto nla_put_failure;
>+
>+		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:
>+			goto nla_put_failure;
>+		}
>+
>+		if (err)
>+			goto nla_put_failure;
>+	} else {
>+		/* Must be config_set command */
>+		u8 *val8;
>+		u16 *val16;
>+		u32 *val32;
>+
>+		switch (policy.type) {
>+		case NLA_U8:
>+			val8 = nla_data(info->attrs[attr]);
>+			value = *val8;
>+			break;
>+		case NLA_U16:
>+			val16 = nla_data(info->attrs[attr]);
>+			value = *val16;
>+			break;
>+		case NLA_U32:
>+			val32 = nla_data(info->attrs[attr]);
>+			value = *val32;
>+			break;
>+		default:
>+			goto nla_put_failure;
>+		}
>+
>+		err = ops->config_set(devlink, attr, value, &restart_reqd);
>+		if (err)
>+			goto nla_put_failure;
>+
>+		if (restart_reqd) {
>+			err = nla_put_u8(msg, DEVLINK_ATTR_RESTART_REQUIRED,
>+					 restart_reqd);
>+			if (err)
>+				goto nla_put_failure;
>+		}
>+	}
>+
>+	genlmsg_end(msg, hdr);
>+	return 0;
>+
>+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_fill(msg, devlink, DEVLINK_CMD_CONFIG_GET,
>+				     info);
>+
>+	if (err) {
>+		nlmsg_free(msg);
>+		return err;
>+	}
>+
>+	return genlmsg_reply(msg, info);
>+}
>+
>+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;
>+	int err;
>+
>+	if (!devlink->ops)
>+		return -EOPNOTSUPP;
>+
>+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+	if (!msg)
>+		return -ENOMEM;
>+
>+	err = devlink_nl_config_fill(msg, devlink, DEVLINK_CMD_CONFIG_SET,
>+				     info);

This is odd. You are using "fill" function for "set". It should be used
for "get". But for set, it really sounds odd. Please split the
devlink_nl_config_fill function into get and set parts.

Also, get should dump all available options. 


>+
>+	if (err) {
>+		nlmsg_free(msg);
>+		return err;
>+	}
>+
>+	return genlmsg_reply(msg, info);
>+}
>+
> int devlink_dpipe_match_put(struct sk_buff *skb,
> 			    struct devlink_dpipe_match *match)
> {
>@@ -2291,6 +2449,41 @@ 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_SRIOV_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_NUM_VF_PER_PF] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MAX_NUM_PF_MSIX_VECT] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MSIX_VECTORS_PER_VF] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_NPAR_NUM_PARTITIONS_PER_PORT] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_NPAR_BW_IN_PERCENT] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_NPAR_BW_RESERVATION] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_NPAR_BW_RESERVATION_VALID] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_NPAR_BW_LIMIT] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_NPAR_BW_LIMIT_VALID] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_DCBX_MODE] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_RDMA_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_MULTIFUNC_MODE] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_SECURE_NIC_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_IGNORE_ARI_CAPABILITY] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_LLDP_NEAREST_BRIDGE_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_LLDP_NEAREST_NONTPMR_BRIDGE_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_PME_CAPABILITY_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_MAGIC_PACKET_WOL_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_EEE_PWR_SAVE_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_AUTONEG_PROTOCOL] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MEDIA_AUTO_DETECT] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_PHY_SELECT] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_PRE_OS_LINK_SPEED_D0] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_PRE_OS_LINK_SPEED_D3] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MBA_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_MBA_BOOT_TYPE] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MBA_DELAY_TIME] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MBA_SETUP_HOT_KEY] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MBA_HIDE_SETUP_PROMPT] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_MBA_BOOT_RETRY_COUNT] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MBA_VLAN_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_MBA_VLAN_TAG] = { .type = NLA_U16 },
>+	[DEVLINK_ATTR_MBA_BOOT_PROTOCOL] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MBA_LINK_SPEED] = { .type = NLA_U32 },
> };
> 
> static const struct genl_ops devlink_nl_ops[] = {
>@@ -2451,6 +2644,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: [RFC 1/3] devlink: Add config parameter get/set operations

[Jiri Pirko]

Thu, Oct 12, 2017 at 03:34:20PM CEST, steven.lin1@broadcom.com wrote:
>+
>+	/* 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_RESTART_REQUIRED,		/* u8 */
>+

I think it would be nice to return this information as a part of retply
to set message - extack

Also, we need to expose to the user the original value (currently being
used) and the new one (to be used after driver re-instatiation)

Re: [RFC 0/3] Adding config get/set to devlink

[Roopa Prabhu]

On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote:
> Adds a devlink command for getting & setting device configuration
> parameters, and enumerates a bunch of those parameters as devlink
> attributes.  Also introduces an attribute that can be set by a
> driver to indicate that the config change doesn't take effect
> until the next restart (as in the case of the bnxt driver changes
> in this patchset, for which all the configuration changes affect NVM
> only, and aren't loaded until the next restart.)
>
> bnxt driver patches make use of these new devlink cmds/attributes.
>
> Steve Lin (3):
>   devlink: Add config parameter get/set operations
>   bnxt: Move generic devlink code to new file
>   bnxt: Add devlink support for config get/set
>

Is the goal here to move all ethtool operations to devlink (I saw some
attrs related to speed etc). ?.
We do need to move ethtool attrs to netlink and devlink is a good
place (and of-course leave the current ethtool api around for backward
compatibility).

Re: [RFC 1/3] devlink: Add config parameter get/set operations

[Steve Lin]

Jiri,

Thanks for your feedback below and in your other response.  I will
make some changes to address those issues and resubmit.

Thanks again!
Steve

On Thu, Oct 12, 2017 at 10:03 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 12, 2017 at 03:34:20PM CEST, steven.lin1@broadcom.com wrote:
>>Add support for config parameter get/set commands. Initially used by
>>bnxt driver, but other drivers can use the same, or new, attributes.
>>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.
>>
>
> First of all, I like this approach.
>
> I would like to see this patch split into:
> 1) config-options infrastructure introduction
> 2) specific config options introductions - would be best to have it
>    per-option. We need to make sure every option is very well described
>    and explained usecases. This is needed in order vendors to share
>    attributes among drivers.
>
> More nits inlined.
>
>
>>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 | 108 ++++++++++++++++++++++
>> net/core/devlink.c           | 207 +++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 319 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..e959716 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
>>@@ -124,6 +127,68 @@ 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_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_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,
>>@@ -202,6 +267,49 @@ enum devlink_attr {
>>
>>       DEVLINK_ATTR_ESWITCH_ENCAP_MODE,        /* u8 */
>>
>>+      /* Configuration Parameters */
>>+      DEVLINK_ATTR_SRIOV_ENABLED,             /* u8 */
>>+      DEVLINK_ATTR_NUM_VF_PER_PF,             /* u32 */
>>+      DEVLINK_ATTR_MAX_NUM_PF_MSIX_VECT,      /* u32 */
>>+      DEVLINK_ATTR_MSIX_VECTORS_PER_VF,       /* u32 */
>>+      DEVLINK_ATTR_NPAR_NUM_PARTITIONS_PER_PORT,      /* u32 */
>>+      DEVLINK_ATTR_NPAR_BW_IN_PERCENT,        /* u8 */
>>+      DEVLINK_ATTR_NPAR_BW_RESERVATION,       /* u8 */
>>+      DEVLINK_ATTR_NPAR_BW_RESERVATION_VALID, /* u8 */
>>+      DEVLINK_ATTR_NPAR_BW_LIMIT,             /* u8 */
>>+      DEVLINK_ATTR_NPAR_BW_LIMIT_VALID,       /* u8 */
>>+      DEVLINK_ATTR_DCBX_MODE,                 /* u8 */
>>+      DEVLINK_ATTR_RDMA_ENABLED,              /* u8 */
>>+      DEVLINK_ATTR_MULTIFUNC_MODE,            /* u8 */
>>+      DEVLINK_ATTR_SECURE_NIC_ENABLED,        /* u8 */
>>+      DEVLINK_ATTR_IGNORE_ARI_CAPABILITY,     /* u8 */
>>+      DEVLINK_ATTR_LLDP_NEAREST_BRIDGE_ENABLED,       /* u8 */
>>+      DEVLINK_ATTR_LLDP_NEAREST_NONTPMR_BRIDGE_ENABLED,       /* u8 */
>>+      DEVLINK_ATTR_PME_CAPABILITY_ENABLED,    /* u8 */
>>+      DEVLINK_ATTR_MAGIC_PACKET_WOL_ENABLED,  /* u8 */
>>+      DEVLINK_ATTR_EEE_PWR_SAVE_ENABLED,      /* u8 */
>>+      DEVLINK_ATTR_AUTONEG_PROTOCOL,          /* u8 */
>>+      DEVLINK_ATTR_MEDIA_AUTO_DETECT,         /* u8 */
>>+      DEVLINK_ATTR_PHY_SELECT,                /* u8 */
>>+      DEVLINK_ATTR_PRE_OS_LINK_SPEED_D0,      /* u8 */
>>+      DEVLINK_ATTR_PRE_OS_LINK_SPEED_D3,      /* u8 */
>>+      DEVLINK_ATTR_MBA_ENABLED,               /* u8 */
>>+      DEVLINK_ATTR_MBA_BOOT_TYPE,             /* u8 */
>>+      DEVLINK_ATTR_MBA_DELAY_TIME,            /* u32 */
>>+      DEVLINK_ATTR_MBA_SETUP_HOT_KEY,         /* u8 */
>>+      DEVLINK_ATTR_MBA_HIDE_SETUP_PROMPT,     /* u8 */
>>+      DEVLINK_ATTR_MBA_BOOT_RETRY_COUNT,      /* u32 */
>>+      DEVLINK_ATTR_MBA_VLAN_ENABLED,          /* u8 */
>>+      DEVLINK_ATTR_MBA_VLAN_TAG,              /* u16 */
>>+      DEVLINK_ATTR_MBA_BOOT_PROTOCOL,         /* u8 */
>>+      DEVLINK_ATTR_MBA_LINK_SPEED,            /* u8 */
>
> Okay, I think it is about the time we should start thinking about
> putting this new config attributes under nester attribute. What do you
> think?
>
>
>>+
>>+      /* 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_RESTART_REQUIRED,          /* u8 */
>>+
>>       /* add new attributes above here, update the policy in devlink.c */
>>
>>       __DEVLINK_ATTR_MAX,
>>diff --git a/net/core/devlink.c b/net/core/devlink.c
>>index 7d430c1..701c84b 100644
>>--- a/net/core/devlink.c
>>+++ b/net/core/devlink.c
>>@@ -1566,6 +1566,164 @@ 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_config_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 = -1;
>
> -1 is not a valid enum value. Better to just have bool to indicate attr
> was found or not.
>
>
>>+      u32 value;
>>+      int i;
>>+      struct nla_policy policy;
>>+      u8 restart_reqd;
>>+
>>+      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;
>>+
>>+      for (i = 0; i < DEVLINK_ATTR_MAX; i++)
>>+              if (info->attrs[i])
>>+                      attr = i;
>>+
>>+      if (attr == -1) {
>>+              /* Not found - invalid/unknown attribute? */
>>+              err = -EINVAL;
>>+              goto nla_put_failure;
>>+      }
>>+
>>+      policy = devlink_nl_policy[attr];
>>+
>>+      if (cmd == DEVLINK_CMD_CONFIG_GET) {
>>+              err = ops->config_get(devlink, attr, &value);
>>+
>>+              if (err)
>>+                      goto nla_put_failure;
>>+
>>+              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:
>>+                      goto nla_put_failure;
>>+              }
>>+
>>+              if (err)
>>+                      goto nla_put_failure;
>>+      } else {
>>+              /* Must be config_set command */
>>+              u8 *val8;
>>+              u16 *val16;
>>+              u32 *val32;
>>+
>>+              switch (policy.type) {
>>+              case NLA_U8:
>>+                      val8 = nla_data(info->attrs[attr]);
>>+                      value = *val8;
>>+                      break;
>>+              case NLA_U16:
>>+                      val16 = nla_data(info->attrs[attr]);
>>+                      value = *val16;
>>+                      break;
>>+              case NLA_U32:
>>+                      val32 = nla_data(info->attrs[attr]);
>>+                      value = *val32;
>>+                      break;
>>+              default:
>>+                      goto nla_put_failure;
>>+              }
>>+
>>+              err = ops->config_set(devlink, attr, value, &restart_reqd);
>>+              if (err)
>>+                      goto nla_put_failure;
>>+
>>+              if (restart_reqd) {
>>+                      err = nla_put_u8(msg, DEVLINK_ATTR_RESTART_REQUIRED,
>>+                                       restart_reqd);
>>+                      if (err)
>>+                              goto nla_put_failure;
>>+              }
>>+      }
>>+
>>+      genlmsg_end(msg, hdr);
>>+      return 0;
>>+
>>+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_fill(msg, devlink, DEVLINK_CMD_CONFIG_GET,
>>+                                   info);
>>+
>>+      if (err) {
>>+              nlmsg_free(msg);
>>+              return err;
>>+      }
>>+
>>+      return genlmsg_reply(msg, info);
>>+}
>>+
>>+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;
>>+      int err;
>>+
>>+      if (!devlink->ops)
>>+              return -EOPNOTSUPP;
>>+
>>+      msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>+      if (!msg)
>>+              return -ENOMEM;
>>+
>>+      err = devlink_nl_config_fill(msg, devlink, DEVLINK_CMD_CONFIG_SET,
>>+                                   info);
>
> This is odd. You are using "fill" function for "set". It should be used
> for "get". But for set, it really sounds odd. Please split the
> devlink_nl_config_fill function into get and set parts.
>
> Also, get should dump all available options.
>
>
>>+
>>+      if (err) {
>>+              nlmsg_free(msg);
>>+              return err;
>>+      }
>>+
>>+      return genlmsg_reply(msg, info);
>>+}
>>+
>> int devlink_dpipe_match_put(struct sk_buff *skb,
>>                           struct devlink_dpipe_match *match)
>> {
>>@@ -2291,6 +2449,41 @@ 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_SRIOV_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_NUM_VF_PER_PF] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MAX_NUM_PF_MSIX_VECT] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MSIX_VECTORS_PER_VF] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_NPAR_NUM_PARTITIONS_PER_PORT] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_NPAR_BW_IN_PERCENT] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_NPAR_BW_RESERVATION] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_NPAR_BW_RESERVATION_VALID] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_NPAR_BW_LIMIT] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_NPAR_BW_LIMIT_VALID] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_DCBX_MODE] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_RDMA_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_MULTIFUNC_MODE] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_SECURE_NIC_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_IGNORE_ARI_CAPABILITY] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_LLDP_NEAREST_BRIDGE_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_LLDP_NEAREST_NONTPMR_BRIDGE_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_PME_CAPABILITY_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_MAGIC_PACKET_WOL_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_EEE_PWR_SAVE_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_AUTONEG_PROTOCOL] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MEDIA_AUTO_DETECT] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_PHY_SELECT] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_PRE_OS_LINK_SPEED_D0] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_PRE_OS_LINK_SPEED_D3] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MBA_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_MBA_BOOT_TYPE] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MBA_DELAY_TIME] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MBA_SETUP_HOT_KEY] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MBA_HIDE_SETUP_PROMPT] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_MBA_BOOT_RETRY_COUNT] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MBA_VLAN_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_MBA_VLAN_TAG] = { .type = NLA_U16 },
>>+      [DEVLINK_ATTR_MBA_BOOT_PROTOCOL] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MBA_LINK_SPEED] = { .type = NLA_U32 },
>> };
>>
>> static const struct genl_ops devlink_nl_ops[] = {
>>@@ -2451,6 +2644,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: [RFC 0/3] Adding config get/set to devlink

[Jiri Pirko]

Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com wrote:
>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote:
>> Adds a devlink command for getting & setting device configuration
>> parameters, and enumerates a bunch of those parameters as devlink
>> attributes.  Also introduces an attribute that can be set by a
>> driver to indicate that the config change doesn't take effect
>> until the next restart (as in the case of the bnxt driver changes
>> in this patchset, for which all the configuration changes affect NVM
>> only, and aren't loaded until the next restart.)
>>
>> bnxt driver patches make use of these new devlink cmds/attributes.
>>
>> Steve Lin (3):
>>   devlink: Add config parameter get/set operations
>>   bnxt: Move generic devlink code to new file
>>   bnxt: Add devlink support for config get/set
>>
>
>Is the goal here to move all ethtool operations to devlink (I saw some
>attrs related to speed etc). ?.
>We do need to move ethtool attrs to netlink and devlink is a good
>place (and of-course leave the current ethtool api around for backward
>compatibility).

We need to make sure we are not moving things to devlink which don't
belong there. All options that use "netdev" as a handle should go into
rtnetlink instead.

Re: [RFC 0/3] Adding config get/set to devlink

[Steve Lin]

Hi Roopa,

The attributes added in this patchset are not really the same type as
ethtool - these are more device configuration type attributes.  The
speeds you saw, for example, affect the pre-OS [i.e. PXE boot time]
configuration for a port, and aren't run-time speed changes on a given
netdev like ethtool configures.  As Jiri mentioned, I will add some
comments to better describe each of the attributes.

So I don't think there's much duplication here with ethtool.

That said, there also shouldn't be anything in the patchset that would
preclude some future migration of ethtool settings to using devlink or
rtnetlink API.

Steve

On Thu, Oct 12, 2017 at 10:35 AM, Roopa Prabhu
<roopa@cumulusnetworks.com> wrote:
> On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote:
>> Adds a devlink command for getting & setting device configuration
>> parameters, and enumerates a bunch of those parameters as devlink
>> attributes.  Also introduces an attribute that can be set by a
>> driver to indicate that the config change doesn't take effect
>> until the next restart (as in the case of the bnxt driver changes
>> in this patchset, for which all the configuration changes affect NVM
>> only, and aren't loaded until the next restart.)
>>
>> bnxt driver patches make use of these new devlink cmds/attributes.
>>
>> Steve Lin (3):
>>   devlink: Add config parameter get/set operations
>>   bnxt: Move generic devlink code to new file
>>   bnxt: Add devlink support for config get/set
>>
>
> Is the goal here to move all ethtool operations to devlink (I saw some
> attrs related to speed etc). ?.
> We do need to move ethtool attrs to netlink and devlink is a good
> place (and of-course leave the current ethtool api around for backward
> compatibility).

Re: [RFC 0/3] Adding config get/set to devlink

[Roopa Prabhu]

On Thu, Oct 12, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com wrote:
>>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote:
>>> Adds a devlink command for getting & setting device configuration
>>> parameters, and enumerates a bunch of those parameters as devlink
>>> attributes.  Also introduces an attribute that can be set by a
>>> driver to indicate that the config change doesn't take effect
>>> until the next restart (as in the case of the bnxt driver changes
>>> in this patchset, for which all the configuration changes affect NVM
>>> only, and aren't loaded until the next restart.)
>>>
>>> bnxt driver patches make use of these new devlink cmds/attributes.
>>>
>>> Steve Lin (3):
>>>   devlink: Add config parameter get/set operations
>>>   bnxt: Move generic devlink code to new file
>>>   bnxt: Add devlink support for config get/set
>>>
>>
>>Is the goal here to move all ethtool operations to devlink (I saw some
>>attrs related to speed etc). ?.
>>We do need to move ethtool attrs to netlink and devlink is a good
>>place (and of-course leave the current ethtool api around for backward
>>compatibility).
>
> We need to make sure we are not moving things to devlink which don't
> belong there. All options that use "netdev" as a handle should go into
> rtnetlink instead.
>

Any reason you want to keep that restriction ?.
FWIS, devlink is a driver api just like ethtool is.
and ethtool needs to move to netlink soon...and It would be better to
not put the rtnl_lock burden on ethtool driver operations. Instead of
adding yet another driver api, extending devlink seems like a great
fit to me.

Re: [RFC 0/3] Adding config get/set to devlink

[Roopa Prabhu]

On Thu, Oct 12, 2017 at 7:45 AM, Steve Lin <steven.lin1@broadcom.com> wrote:
> Hi Roopa,
>
> The attributes added in this patchset are not really the same type as
> ethtool - these are more device configuration type attributes.  The
> speeds you saw, for example, affect the pre-OS [i.e. PXE boot time]
> configuration for a port, and aren't run-time speed changes on a given
> netdev like ethtool configures.  As Jiri mentioned, I will add some
> comments to better describe each of the attributes.
>
> So I don't think there's much duplication here with ethtool.
>
> That said, there also shouldn't be anything in the patchset that would
> preclude some future migration of ethtool settings to using devlink or
> rtnetlink API.
>

ok, ack, thanks for the clarification. Just trying to find a best
netlink place for future ethtool migration to netlink.

Re: [RFC 0/3] Adding config get/set to devlink

[Jiri Pirko]

Thu, Oct 12, 2017 at 04:46:24PM CEST, roopa@cumulusnetworks.com wrote:
>On Thu, Oct 12, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com wrote:
>>>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote:
>>>> Adds a devlink command for getting & setting device configuration
>>>> parameters, and enumerates a bunch of those parameters as devlink
>>>> attributes.  Also introduces an attribute that can be set by a
>>>> driver to indicate that the config change doesn't take effect
>>>> until the next restart (as in the case of the bnxt driver changes
>>>> in this patchset, for which all the configuration changes affect NVM
>>>> only, and aren't loaded until the next restart.)
>>>>
>>>> bnxt driver patches make use of these new devlink cmds/attributes.
>>>>
>>>> Steve Lin (3):
>>>>   devlink: Add config parameter get/set operations
>>>>   bnxt: Move generic devlink code to new file
>>>>   bnxt: Add devlink support for config get/set
>>>>
>>>
>>>Is the goal here to move all ethtool operations to devlink (I saw some
>>>attrs related to speed etc). ?.
>>>We do need to move ethtool attrs to netlink and devlink is a good
>>>place (and of-course leave the current ethtool api around for backward
>>>compatibility).
>>
>> We need to make sure we are not moving things to devlink which don't
>> belong there. All options that use "netdev" as a handle should go into
>> rtnetlink instead.
>>
>
>Any reason you want to keep that restriction ?.
>FWIS, devlink is a driver api just like ethtool is.
>and ethtool needs to move to netlink soon...and It would be better to
>not put the rtnl_lock burden on ethtool driver operations. Instead of
>adding yet another driver api, extending devlink seems like a great
>fit to me.

Hmm, the original purpose of devlink was to obtain iface for things that
could not use "netdev" as a handle. I try to stick with it as we already
have iface for things that could use "netdev" as a handle - rtnetlink.

Not sure we want to go this way and add "netdev"-handle things into
devlink. Thoughts?

Re: [RFC 0/3] Adding config get/set to devlink

[Roopa Prabhu]

On Thu, Oct 12, 2017 at 8:04 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 12, 2017 at 04:46:24PM CEST, roopa@cumulusnetworks.com wrote:
>>On Thu, Oct 12, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com wrote:
>>>>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote:
>>>>> Adds a devlink command for getting & setting device configuration
>>>>> parameters, and enumerates a bunch of those parameters as devlink
>>>>> attributes.  Also introduces an attribute that can be set by a
>>>>> driver to indicate that the config change doesn't take effect
>>>>> until the next restart (as in the case of the bnxt driver changes
>>>>> in this patchset, for which all the configuration changes affect NVM
>>>>> only, and aren't loaded until the next restart.)
>>>>>
>>>>> bnxt driver patches make use of these new devlink cmds/attributes.
>>>>>
>>>>> Steve Lin (3):
>>>>>   devlink: Add config parameter get/set operations
>>>>>   bnxt: Move generic devlink code to new file
>>>>>   bnxt: Add devlink support for config get/set
>>>>>
>>>>
>>>>Is the goal here to move all ethtool operations to devlink (I saw some
>>>>attrs related to speed etc). ?.
>>>>We do need to move ethtool attrs to netlink and devlink is a good
>>>>place (and of-course leave the current ethtool api around for backward
>>>>compatibility).
>>>
>>> We need to make sure we are not moving things to devlink which don't
>>> belong there. All options that use "netdev" as a handle should go into
>>> rtnetlink instead.
>>>
>>
>>Any reason you want to keep that restriction ?.
>>FWIS, devlink is a driver api just like ethtool is.
>>and ethtool needs to move to netlink soon...and It would be better to
>>not put the rtnl_lock burden on ethtool driver operations. Instead of
>>adding yet another driver api, extending devlink seems like a great
>>fit to me.
>
> Hmm, the original purpose of devlink was to obtain iface for things that
> could not use "netdev" as a handle. I try to stick with it as we already
> have iface for things that could use "netdev" as a handle - rtnetlink.
>
> Not sure we want to go this way and add "netdev"-handle things into
> devlink. Thoughts?
>

Only motivation for me is to keep all driver/hw api in a single place.
and its high time ethtool moved to netlink. I would prefer it be out
of rtnetlink if we have a choice.

Moving some of the driver ops to  rtnetlink and leaving the rest in
devlink can be a mess for drivers in the long run.
Maybe we can discuss this more at netdev2.2 ?

Re: [RFC 0/3] Adding config get/set to devlink

[Florian Fainelli]

On October 12, 2017 8:04:19 AM PDT, Jiri Pirko <jiri@resnulli.us> wrote:
>Thu, Oct 12, 2017 at 04:46:24PM CEST, roopa@cumulusnetworks.com wrote:
>>On Thu, Oct 12, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com
>wrote:
>>>>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin
><steven.lin1@broadcom.com> wrote:
>>>>> Adds a devlink command for getting & setting device configuration
>>>>> parameters, and enumerates a bunch of those parameters as devlink
>>>>> attributes.  Also introduces an attribute that can be set by a
>>>>> driver to indicate that the config change doesn't take effect
>>>>> until the next restart (as in the case of the bnxt driver changes
>>>>> in this patchset, for which all the configuration changes affect
>NVM
>>>>> only, and aren't loaded until the next restart.)
>>>>>
>>>>> bnxt driver patches make use of these new devlink cmds/attributes.
>>>>>
>>>>> Steve Lin (3):
>>>>>   devlink: Add config parameter get/set operations
>>>>>   bnxt: Move generic devlink code to new file
>>>>>   bnxt: Add devlink support for config get/set
>>>>>
>>>>
>>>>Is the goal here to move all ethtool operations to devlink (I saw
>some
>>>>attrs related to speed etc). ?.
>>>>We do need to move ethtool attrs to netlink and devlink is a good
>>>>place (and of-course leave the current ethtool api around for
>backward
>>>>compatibility).
>>>
>>> We need to make sure we are not moving things to devlink which don't
>>> belong there. All options that use "netdev" as a handle should go
>into
>>> rtnetlink instead.
>>>
>>
>>Any reason you want to keep that restriction ?.
>>FWIS, devlink is a driver api just like ethtool is.
>>and ethtool needs to move to netlink soon...and It would be better to
>>not put the rtnl_lock burden on ethtool driver operations. Instead of
>>adding yet another driver api, extending devlink seems like a great
>>fit to me.
>
>Hmm, the original purpose of devlink was to obtain iface for things
>that
>could not use "netdev" as a handle. I try to stick with it as we
>already
>have iface for things that could use "netdev" as a handle - rtnetlink.
>
>Not sure we want to go this way and add "netdev"-handle things into
>devlink. Thoughts?

In the current situation where we have ethtool and devlink operating separately on different objects as entry points to the kernel, I agree with that design.

Once we move ethtool (or however we name its successor) over to netlink there is an opportunity for accessing objects that do and do not have a netdevice representor today (e.g: management ports on switches) with the same interface, and devlink could be used for that.

In terms of compatibility though we should have a pseudo generic layer that can take ethtool ioctl() and transform that into a netlink message and then call back down to drivers with the existing ethtool_ops that are implemented. It is reasonably simple to use coccinelle to update these ethtool_ops with possibly updated signatures to support netlink attributes and whatnot, but forcing drivers to quit doing ethtool_ops entitely and implement new sets of "ethtool over netlink" ops is a non starter IMHO.

-- 
Florian

Re: [RFC 0/3] Adding config get/set to devlink

[Florian Fainelli]

On October 12, 2017 8:04:19 AM PDT, Jiri Pirko <jiri@resnulli=2Eus> wrote:
>Thu, Oct 12, 2017 at 04:46:24PM CEST, roopa@cumulusnetworks=2Ecom wrote:
>>On Thu, Oct 12, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli=2Eus> wrote:
>>> Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks=2Ecom
>wrote:
>>>>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin
><steven=2Elin1@broadcom=2Ecom> wrote:
>>>>> Adds a devlink command for getting & setting device configuration
>>>>> parameters, and enumerates a bunch of those parameters as devlink
>>>>> attributes=2E  Also introduces an attribute that can be set by a
>>>>> driver to indicate that the config change doesn't take effect
>>>>> until the next restart (as in the case of the bnxt driver changes
>>>>> in this patchset, for which all the configuration changes affect
>NVM
>>>>> only, and aren't loaded until the next restart=2E)
>>>>>
>>>>> bnxt driver patches make use of these new devlink cmds/attributes=2E
>>>>>
>>>>> Steve Lin (3):
>>>>>   devlink: Add config parameter get/set operations
>>>>>   bnxt: Move generic devlink code to new file
>>>>>   bnxt: Add devlink support for config get/set
>>>>>
>>>>
>>>>Is the goal here to move all ethtool operations to devlink (I saw
>some
>>>>attrs related to speed etc)=2E ?=2E
>>>>We do need to move ethtool attrs to netlink and devlink is a good
>>>>place (and of-course leave the current ethtool api around for
>backward
>>>>compatibility)=2E
>>>
>>> We need to make sure we are not moving things to devlink which don't
>>> belong there=2E All options that use "netdev" as a handle should go
>into
>>> rtnetlink instead=2E
>>>
>>
>>Any reason you want to keep that restriction ?=2E
>>FWIS, devlink is a driver api just like ethtool is=2E
>>and ethtool needs to move to netlink soon=2E=2E=2Eand It would be better=
 to
>>not put the rtnl_lock burden on ethtool driver operations=2E Instead of
>>adding yet another driver api, extending devlink seems like a great
>>fit to me=2E
>
>Hmm, the original purpose of devlink was to obtain iface for things
>that
>could not use "netdev" as a handle=2E I try to stick with it as we
>already
>have iface for things that could use "netdev" as a handle - rtnetlink=2E
>
>Not sure we want to go this way and add "netdev"-handle things into
>devlink=2E Thoughts?

In the current situation where we have ethtool and devlink operating separ=
ately on different objects as entry points to the kernel, I agree with that=
 design=2E

Once we move ethtool (or however we name its successor) over to netlink th=
ere is an opportunity for accessing objects that do and do not have a netde=
vice representor today (e=2Eg: management ports on switches) with the same =
interface, and devlink could be used for that=2E

In terms of compatibility though we should have a pseudo generic layer tha=
t can take ethtool ioctl() and transform that into a netlink message and th=
en call back down to drivers with the existing ethtool_ops that are impleme=
nted=2E It is reasonably simple to use coccinelle to update these ethtool_o=
ps with possibly updated signatures to support netlink attributes and whatn=
ot, but forcing drivers to quit doing ethtool_ops entitely and implement ne=
w sets of "ethtool over netlink" ops is a non starter IMHO=2E

--=20
Florian

Re: [RFC 0/3] Adding config get/set to devlink

[Roopa Prabhu]

On Thu, Oct 12, 2017 at 8:43 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On October 12, 2017 8:04:19 AM PDT, Jiri Pirko <jiri@resnulli.us> wrote:
>>Thu, Oct 12, 2017 at 04:46:24PM CEST, roopa@cumulusnetworks.com wrote:
>>>On Thu, Oct 12, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com
>>wrote:
>>>>>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin
>><steven.lin1@broadcom.com> wrote:
>>>>>> Adds a devlink command for getting & setting device configuration
>>>>>> parameters, and enumerates a bunch of those parameters as devlink
>>>>>> attributes.  Also introduces an attribute that can be set by a
>>>>>> driver to indicate that the config change doesn't take effect
>>>>>> until the next restart (as in the case of the bnxt driver changes
>>>>>> in this patchset, for which all the configuration changes affect
>>NVM
>>>>>> only, and aren't loaded until the next restart.)
>>>>>>
>>>>>> bnxt driver patches make use of these new devlink cmds/attributes.
>>>>>>
>>>>>> Steve Lin (3):
>>>>>>   devlink: Add config parameter get/set operations
>>>>>>   bnxt: Move generic devlink code to new file
>>>>>>   bnxt: Add devlink support for config get/set
>>>>>>
>>>>>
>>>>>Is the goal here to move all ethtool operations to devlink (I saw
>>some
>>>>>attrs related to speed etc). ?.
>>>>>We do need to move ethtool attrs to netlink and devlink is a good
>>>>>place (and of-course leave the current ethtool api around for
>>backward
>>>>>compatibility).
>>>>
>>>> We need to make sure we are not moving things to devlink which don't
>>>> belong there. All options that use "netdev" as a handle should go
>>into
>>>> rtnetlink instead.
>>>>
>>>
>>>Any reason you want to keep that restriction ?.
>>>FWIS, devlink is a driver api just like ethtool is.
>>>and ethtool needs to move to netlink soon...and It would be better to
>>>not put the rtnl_lock burden on ethtool driver operations. Instead of
>>>adding yet another driver api, extending devlink seems like a great
>>>fit to me.
>>
>>Hmm, the original purpose of devlink was to obtain iface for things
>>that
>>could not use "netdev" as a handle. I try to stick with it as we
>>already
>>have iface for things that could use "netdev" as a handle - rtnetlink.
>>
>>Not sure we want to go this way and add "netdev"-handle things into
>>devlink. Thoughts?
>
> In the current situation where we have ethtool and devlink operating separately on different objects as entry points to the kernel, I agree with that design.
>
> Once we move ethtool (or however we name its successor) over to netlink there is an opportunity for accessing objects that do and do not have a netdevice representor today (e.g: management ports on switches) with the same interface, and devlink could be used for that.
>
> In terms of compatibility though we should have a pseudo generic layer that can take ethtool ioctl() and transform that into a netlink message and then call back down to drivers with the existing ethtool_ops that are implemented. It is reasonably simple to use coccinelle to update these ethtool_ops with possibly updated signatures to support netlink attributes and whatnot,

ack, that sounds like a good approach.

> but forcing drivers to quit doing ethtool_ops entitely and implement new sets of "ethtool over netlink" ops is a non starter IMHO.

correct, nobody disagrees with that point.

Re: [RFC 0/3] Adding config get/set to devlink

[Roopa Prabhu]

On Thu, Oct 12, 2017 at 8:43 AM, Florian Fainelli <f.fainelli@gmail.com> wr=
ote:
> On October 12, 2017 8:04:19 AM PDT, Jiri Pirko <jiri@resnulli.us> wrote:
>>Thu, Oct 12, 2017 at 04:46:24PM CEST, roopa@cumulusnetworks.com wrote:
>>>On Thu, Oct 12, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com
>>wrote:
>>>>>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin
>><steven.lin1@broadcom.com> wrote:
>>>>>> Adds a devlink command for getting & setting device configuration
>>>>>> parameters, and enumerates a bunch of those parameters as devlink
>>>>>> attributes.  Also introduces an attribute that can be set by a
>>>>>> driver to indicate that the config change doesn't take effect
>>>>>> until the next restart (as in the case of the bnxt driver changes
>>>>>> in this patchset, for which all the configuration changes affect
>>NVM
>>>>>> only, and aren't loaded until the next restart.)
>>>>>>
>>>>>> bnxt driver patches make use of these new devlink cmds/attributes.
>>>>>>
>>>>>> Steve Lin (3):
>>>>>>   devlink: Add config parameter get/set operations
>>>>>>   bnxt: Move generic devlink code to new file
>>>>>>   bnxt: Add devlink support for config get/set
>>>>>>
>>>>>
>>>>>Is the goal here to move all ethtool operations to devlink (I saw
>>some
>>>>>attrs related to speed etc). ?.
>>>>>We do need to move ethtool attrs to netlink and devlink is a good
>>>>>place (and of-course leave the current ethtool api around for
>>backward
>>>>>compatibility).
>>>>
>>>> We need to make sure we are not moving things to devlink which don't
>>>> belong there. All options that use "netdev" as a handle should go
>>into
>>>> rtnetlink instead.
>>>>
>>>
>>>Any reason you want to keep that restriction ?.
>>>FWIS, devlink is a driver api just like ethtool is.
>>>and ethtool needs to move to netlink soon...and It would be better to
>>>not put the rtnl_lock burden on ethtool driver operations. Instead of
>>>adding yet another driver api, extending devlink seems like a great
>>>fit to me.
>>
>>Hmm, the original purpose of devlink was to obtain iface for things
>>that
>>could not use "netdev" as a handle. I try to stick with it as we
>>already
>>have iface for things that could use "netdev" as a handle - rtnetlink.
>>
>>Not sure we want to go this way and add "netdev"-handle things into
>>devlink. Thoughts?
>
> In the current situation where we have ethtool and devlink operating sepa=
rately on different objects as entry points to the kernel, I agree with tha=
t design.
>
> Once we move ethtool (or however we name its successor) over to netlink t=
here is an opportunity for accessing objects that do and do not have a netd=
evice representor today (e.g: management ports on switches) with the same i=
nterface, and devlink could be used for that.
>
> In terms of compatibility though we should have a pseudo generic layer th=
at can take ethtool ioctl() and transform that into a netlink message and t=
hen call back down to drivers with the existing ethtool_ops that are implem=
ented. It is reasonably simple to use coccinelle to update these ethtool_op=
s with possibly updated signatures to support netlink attributes and whatno=
t,

ack, that sounds like a good approach.

> but forcing drivers to quit doing ethtool_ops entitely and implement new =
sets of "ethtool over netlink" ops is a non starter IMHO.

correct, nobody disagrees with that point.

Re: [RFC 1/3] devlink: Add config parameter get/set operations

[Andy Gospodarek]

On Thu, Oct 12, 2017 at 04:03:17PM +0200, Jiri Pirko wrote:
> Thu, Oct 12, 2017 at 03:34:20PM CEST, steven.lin1@broadcom.com wrote:
> >Add support for config parameter get/set commands. Initially used by
> >bnxt driver, but other drivers can use the same, or new, attributes.
> >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.
> >
> 
> First of all, I like this approach.
> 
> I would like to see this patch split into:
> 1) config-options infrastructure introduction
> 2) specific config options introductions - would be best to have it
>    per-option. We need to make sure every option is very well described
>    and explained usecases. This is needed in order vendors to share
>    attributes among drivers.
> 
> More nits inlined.
> 
> 
> >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 | 108 ++++++++++++++++++++++
> > net/core/devlink.c           | 207 +++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 319 insertions(+)
> >
> > static inline void *devlink_priv(struct devlink *devlink)
> >diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> >index 0cbca96..e959716 100644
> >--- a/include/uapi/linux/devlink.h
> >+++ b/include/uapi/linux/devlink.h
[...]
> >@@ -202,6 +267,49 @@ enum devlink_attr {
> > 
> > 	DEVLINK_ATTR_ESWITCH_ENCAP_MODE,	/* u8 */
> > 
> >+	/* Configuration Parameters */
> >+	DEVLINK_ATTR_SRIOV_ENABLED,		/* u8 */
> >+	DEVLINK_ATTR_NUM_VF_PER_PF,		/* u32 */
> >+	DEVLINK_ATTR_MAX_NUM_PF_MSIX_VECT,	/* u32 */
> >+	DEVLINK_ATTR_MSIX_VECTORS_PER_VF,	/* u32 */
> >+	DEVLINK_ATTR_NPAR_NUM_PARTITIONS_PER_PORT,	/* u32 */
> >+	DEVLINK_ATTR_NPAR_BW_IN_PERCENT,	/* u8 */
> >+	DEVLINK_ATTR_NPAR_BW_RESERVATION,	/* u8 */
> >+	DEVLINK_ATTR_NPAR_BW_RESERVATION_VALID,	/* u8 */
> >+	DEVLINK_ATTR_NPAR_BW_LIMIT,		/* u8 */
> >+	DEVLINK_ATTR_NPAR_BW_LIMIT_VALID,	/* u8 */
> >+	DEVLINK_ATTR_DCBX_MODE,			/* u8 */
> >+	DEVLINK_ATTR_RDMA_ENABLED,		/* u8 */
> >+	DEVLINK_ATTR_MULTIFUNC_MODE,		/* u8 */
> >+	DEVLINK_ATTR_SECURE_NIC_ENABLED,	/* u8 */
> >+	DEVLINK_ATTR_IGNORE_ARI_CAPABILITY,	/* u8 */
> >+	DEVLINK_ATTR_LLDP_NEAREST_BRIDGE_ENABLED,	/* u8 */
> >+	DEVLINK_ATTR_LLDP_NEAREST_NONTPMR_BRIDGE_ENABLED,	/* u8 */
> >+	DEVLINK_ATTR_PME_CAPABILITY_ENABLED,	/* u8 */
> >+	DEVLINK_ATTR_MAGIC_PACKET_WOL_ENABLED,	/* u8 */
> >+	DEVLINK_ATTR_EEE_PWR_SAVE_ENABLED,	/* u8 */
> >+	DEVLINK_ATTR_AUTONEG_PROTOCOL,		/* u8 */
> >+	DEVLINK_ATTR_MEDIA_AUTO_DETECT,		/* u8 */
> >+	DEVLINK_ATTR_PHY_SELECT,		/* u8 */
> >+	DEVLINK_ATTR_PRE_OS_LINK_SPEED_D0,	/* u8 */
> >+	DEVLINK_ATTR_PRE_OS_LINK_SPEED_D3,	/* u8 */
> >+	DEVLINK_ATTR_MBA_ENABLED,		/* u8 */
> >+	DEVLINK_ATTR_MBA_BOOT_TYPE,		/* u8 */
> >+	DEVLINK_ATTR_MBA_DELAY_TIME,		/* u32 */
> >+	DEVLINK_ATTR_MBA_SETUP_HOT_KEY,		/* u8 */
> >+	DEVLINK_ATTR_MBA_HIDE_SETUP_PROMPT,	/* u8 */
> >+	DEVLINK_ATTR_MBA_BOOT_RETRY_COUNT,	/* u32 */
> >+	DEVLINK_ATTR_MBA_VLAN_ENABLED,		/* u8 */
> >+	DEVLINK_ATTR_MBA_VLAN_TAG,		/* u16 */
> >+	DEVLINK_ATTR_MBA_BOOT_PROTOCOL,		/* u8 */
> >+	DEVLINK_ATTR_MBA_LINK_SPEED,		/* u8 */
> 
> Okay, I think it is about the time we should start thinking about
> putting this new config attributes under nester attribute. What do you
> think?
> 

Steve and I actually had a similar discussion yesterday when I was doing
a final review of the patches.

My only objection to nesting was coming up with a way to describe these
functions that made them seem different than existing configuration
options.  In this case of the hardware we are trying to support these
are all permanent config options, so we would call them
DEVLINK_ATTR_NVRAM or DEVLINK_ATTR_PERM.  Does that seem reasonable to
others?

Re: [RFC 0/3] Adding config get/set to devlink

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Thu, 12 Oct 2017 16:40:32 +0200

> Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com wrote:
>>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote:
>>> Adds a devlink command for getting & setting device configuration
>>> parameters, and enumerates a bunch of those parameters as devlink
>>> attributes.  Also introduces an attribute that can be set by a
>>> driver to indicate that the config change doesn't take effect
>>> until the next restart (as in the case of the bnxt driver changes
>>> in this patchset, for which all the configuration changes affect NVM
>>> only, and aren't loaded until the next restart.)
>>>
>>> bnxt driver patches make use of these new devlink cmds/attributes.
>>>
>>> Steve Lin (3):
>>>   devlink: Add config parameter get/set operations
>>>   bnxt: Move generic devlink code to new file
>>>   bnxt: Add devlink support for config get/set
>>>
>>
>>Is the goal here to move all ethtool operations to devlink (I saw some
>>attrs related to speed etc). ?.
>>We do need to move ethtool attrs to netlink and devlink is a good
>>place (and of-course leave the current ethtool api around for backward
>>compatibility).
> 
> We need to make sure we are not moving things to devlink which don't
> belong there. All options that use "netdev" as a handle should go into
> rtnetlink instead.

I agree.  Let's keep devlink what is was created for, which is situations
where we don't have a netdev as the object to work upon.

Re: [RFC 0/3] Adding config get/set to devlink

[David Miller]

From: Roopa Prabhu <roopa@cumulusnetworks.com>
Date: Thu, 12 Oct 2017 07:46:24 -0700

> FWIS, devlink is a driver api just like ethtool is.

Devlink a driver API for doing operations where we don't have a
specific 'netdev' object to work upon.

> and ethtool needs to move to netlink soon...and It would be better to
> not put the rtnl_lock burden on ethtool driver operations. Instead of
> adding yet another driver api, extending devlink seems like a great
> fit to me.

You can use genetlink and avoid RTNL.

Also, Florian Westphal has been pushing the RTNL lock down into the
actual rtnetlink operation implementations.  So one does not have to
avoid rtnetlink to avoid the RTNL mutex at all.

Re: [RFC 0/3] Adding config get/set to devlink

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Thu, 12 Oct 2017 17:04:19 +0200

> Not sure we want to go this way and add "netdev"-handle things into
> devlink. Thoughts?

I think we should avoid this and keep devlink to it's designed purpose.

Re: [RFC 0/3] Adding config get/set to devlink

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Thu, 12 Oct 2017 08:43:59 -0700

> Once we move ethtool (or however we name its successor) over to
> netlink there is an opportunity for accessing objects that do and do
> not have a netdevice representor today (e.g: management ports on
> switches) with the same interface, and devlink could be used for
> that.

That is an interesting angle for including this in devlink.

I'm not so sure what to do about this.

One suggestion is that devlink is used for getting ethtool stats for
objects lacking netdev representor's, and a new genetlink family is
used for netdev based ethtool.

I think it's important that we don't expand the scope of devlink
beyond what it was originally designed for.

Re: [RFC 0/3] Adding config get/set to devlink

[Florian Fainelli]

On 10/12/2017 12:06 PM, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Thu, 12 Oct 2017 08:43:59 -0700
> 
>> Once we move ethtool (or however we name its successor) over to
>> netlink there is an opportunity for accessing objects that do and do
>> not have a netdevice representor today (e.g: management ports on
>> switches) with the same interface, and devlink could be used for
>> that.
> 
> That is an interesting angle for including this in devlink.
> 
> I'm not so sure what to do about this.
> 
> One suggestion is that devlink is used for getting ethtool stats for
> objects lacking netdev representor's, and a new genetlink family is
> used for netdev based ethtool.

Right, I was also thinking along those lines that we we would have a new
generic netlink family for ethtool to support ethtool over netlink.

> 
> I think it's important that we don't expand the scope of devlink
> beyond what it was originally designed for.

It seems to me like devlink is well defined in what it is not for: it is
not meant to be used for any object that is/has a net_device, but it is
not well defined for what it can offer to these non network devices. For
instance, we have a tremendous amount of operations that are extremely
specific to its single user(s) such as mlx5 and mlxsw.

For instance, I am not sure how the buffer reservation scheme can be
generalized, and this is always the tricky part with a single user
facility in that you try to generalize the best you can based on the HW
you know. This is not a criticism or meant to be anything negative, this
just happens to be the case, and we did not have anything better.

So maybe the first thing is to clarify what devlink operations can and
should be and what they are absolutely not allowed to cover. We should
also clarify whether a generic set/get that Steven is proposing is
something that we tolerate, or whether there should be specific function
pointers implemented for each attribute, which would be more in line
with what has been done thus far.
-- 
Florian

Re: [RFC 0/3] Adding config get/set to devlink

[Steve Lin]

On Thu, Oct 12, 2017 at 3:20 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 10/12/2017 12:06 PM, David Miller wrote:
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Date: Thu, 12 Oct 2017 08:43:59 -0700
>>
>>> Once we move ethtool (or however we name its successor) over to
>>> netlink there is an opportunity for accessing objects that do and do
>>> not have a netdevice representor today (e.g: management ports on
>>> switches) with the same interface, and devlink could be used for
>>> that.
>>
>> That is an interesting angle for including this in devlink.
>>
>> I'm not so sure what to do about this.
>>
>> One suggestion is that devlink is used for getting ethtool stats for
>> objects lacking netdev representor's, and a new genetlink family is
>> used for netdev based ethtool.
>
> Right, I was also thinking along those lines that we we would have a new
> generic netlink family for ethtool to support ethtool over netlink.
>
>>
>> I think it's important that we don't expand the scope of devlink
>> beyond what it was originally designed for.
>
> It seems to me like devlink is well defined in what it is not for: it is
> not meant to be used for any object that is/has a net_device, but it is
> not well defined for what it can offer to these non network devices. For
> instance, we have a tremendous amount of operations that are extremely
> specific to its single user(s) such as mlx5 and mlxsw.
>
> For instance, I am not sure how the buffer reservation scheme can be
> generalized, and this is always the tricky part with a single user
> facility in that you try to generalize the best you can based on the HW
> you know. This is not a criticism or meant to be anything negative, this
> just happens to be the case, and we did not have anything better.
>
> So maybe the first thing is to clarify what devlink operations can and
> should be and what they are absolutely not allowed to cover. We should
> also clarify whether a generic set/get that Steven is proposing is
> something that we tolerate, or whether there should be specific function
> pointers implemented for each attribute, which would be more in line
> with what has been done thus far.

Hi Florian,

Some of this is subjective, of course, but just to clarify, it did
seem like implementing a new devlink_op function pointer per attribute
might be more consistent with what's been done so far.  But for code
reuse purposes - i.e. to avoid replicating essentially the same
function for each of the 30+ config attributes - I elected to just
implement a single generic get and set devlink_op.

Steve

Re: [RFC 0/3] Adding config get/set to devlink

[Michal Kubecek]

On Thu, Oct 12, 2017 at 12:20:07PM -0700, Florian Fainelli wrote:
> On 10/12/2017 12:06 PM, David Miller wrote:
> > 
> > One suggestion is that devlink is used for getting ethtool stats for
> > objects lacking netdev representor's, and a new genetlink family is
> > used for netdev based ethtool.
> 
> Right, I was also thinking along those lines that we we would have a new
> generic netlink family for ethtool to support ethtool over netlink.

This is what I plan to work on on next SUSE Hackweek in November. But
I'm, of course, open to suggestions and I don't insist on this approach.

Michal Kubecek

Re: [RFC 0/3] Adding config get/set to devlink

[Roopa Prabhu]

On Thu, Oct 12, 2017 at 12:20 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 10/12/2017 12:06 PM, David Miller wrote:
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Date: Thu, 12 Oct 2017 08:43:59 -0700
>>
>>> Once we move ethtool (or however we name its successor) over to
>>> netlink there is an opportunity for accessing objects that do and do
>>> not have a netdevice representor today (e.g: management ports on
>>> switches) with the same interface, and devlink could be used for
>>> that.
>>
>> That is an interesting angle for including this in devlink.
>>
>> I'm not so sure what to do about this.
>>
>> One suggestion is that devlink is used for getting ethtool stats for
>> objects lacking netdev representor's, and a new genetlink family is
>> used for netdev based ethtool.
>
> Right, I was also thinking along those lines that we we would have a new
> generic netlink family for ethtool to support ethtool over netlink.

new api is fine by me. The reason for suggesting devlink was because
some of the devlink
port_* ops are close to ethtool ops that can operate on a port/netdev.
eg split_port could be a netdev operation
unless you want to split before the netdev is created.

There are some ops in devlink which are global hw parameters and not
specific to a port, those fit perfectly with
devlinks original goal.


>
>>
>> I think it's important that we don't expand the scope of devlink
>> beyond what it was originally designed for.
>
> It seems to me like devlink is well defined in what it is not for: it is
> not meant to be used for any object that is/has a net_device, but it is
> not well defined for what it can offer to these non network devices. For
> instance, we have a tremendous amount of operations that are extremely
> specific to its single user(s) such as mlx5 and mlxsw.
>
> For instance, I am not sure how the buffer reservation scheme can be
> generalized, and this is always the tricky part with a single user
> facility in that you try to generalize the best you can based on the HW
> you know. This is not a criticism or meant to be anything negative, this
> just happens to be the case, and we did not have anything better.
>
> So maybe the first thing is to clarify what devlink operations can and
> should be and what they are absolutely not allowed to cover. We should
> also clarify whether a generic set/get that Steven is proposing is
> something that we tolerate, or whether there should be specific function
> pointers implemented for each attribute, which would be more in line
> with what has been done thus far.
> --
> Florian

Re: [RFC 1/3] devlink: Add config parameter get/set operations

[Jiri Pirko]

Thu, Oct 12, 2017 at 08:12:48PM CEST, andrew.gospodarek@broadcom.com wrote:
>On Thu, Oct 12, 2017 at 04:03:17PM +0200, Jiri Pirko wrote:
>> Thu, Oct 12, 2017 at 03:34:20PM CEST, steven.lin1@broadcom.com wrote:
>> >Add support for config parameter get/set commands. Initially used by
>> >bnxt driver, but other drivers can use the same, or new, attributes.
>> >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.
>> >
>> 
>> First of all, I like this approach.
>> 
>> I would like to see this patch split into:
>> 1) config-options infrastructure introduction
>> 2) specific config options introductions - would be best to have it
>>    per-option. We need to make sure every option is very well described
>>    and explained usecases. This is needed in order vendors to share
>>    attributes among drivers.
>> 
>> More nits inlined.
>> 
>> 
>> >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 | 108 ++++++++++++++++++++++
>> > net/core/devlink.c           | 207 +++++++++++++++++++++++++++++++++++++++++++
>> > 3 files changed, 319 insertions(+)
>> >
>> > static inline void *devlink_priv(struct devlink *devlink)
>> >diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>> >index 0cbca96..e959716 100644
>> >--- a/include/uapi/linux/devlink.h
>> >+++ b/include/uapi/linux/devlink.h
>[...]
>> >@@ -202,6 +267,49 @@ enum devlink_attr {
>> > 
>> > 	DEVLINK_ATTR_ESWITCH_ENCAP_MODE,	/* u8 */
>> > 
>> >+	/* Configuration Parameters */
>> >+	DEVLINK_ATTR_SRIOV_ENABLED,		/* u8 */
>> >+	DEVLINK_ATTR_NUM_VF_PER_PF,		/* u32 */
>> >+	DEVLINK_ATTR_MAX_NUM_PF_MSIX_VECT,	/* u32 */
>> >+	DEVLINK_ATTR_MSIX_VECTORS_PER_VF,	/* u32 */
>> >+	DEVLINK_ATTR_NPAR_NUM_PARTITIONS_PER_PORT,	/* u32 */
>> >+	DEVLINK_ATTR_NPAR_BW_IN_PERCENT,	/* u8 */
>> >+	DEVLINK_ATTR_NPAR_BW_RESERVATION,	/* u8 */
>> >+	DEVLINK_ATTR_NPAR_BW_RESERVATION_VALID,	/* u8 */
>> >+	DEVLINK_ATTR_NPAR_BW_LIMIT,		/* u8 */
>> >+	DEVLINK_ATTR_NPAR_BW_LIMIT_VALID,	/* u8 */
>> >+	DEVLINK_ATTR_DCBX_MODE,			/* u8 */
>> >+	DEVLINK_ATTR_RDMA_ENABLED,		/* u8 */
>> >+	DEVLINK_ATTR_MULTIFUNC_MODE,		/* u8 */
>> >+	DEVLINK_ATTR_SECURE_NIC_ENABLED,	/* u8 */
>> >+	DEVLINK_ATTR_IGNORE_ARI_CAPABILITY,	/* u8 */
>> >+	DEVLINK_ATTR_LLDP_NEAREST_BRIDGE_ENABLED,	/* u8 */
>> >+	DEVLINK_ATTR_LLDP_NEAREST_NONTPMR_BRIDGE_ENABLED,	/* u8 */
>> >+	DEVLINK_ATTR_PME_CAPABILITY_ENABLED,	/* u8 */
>> >+	DEVLINK_ATTR_MAGIC_PACKET_WOL_ENABLED,	/* u8 */
>> >+	DEVLINK_ATTR_EEE_PWR_SAVE_ENABLED,	/* u8 */
>> >+	DEVLINK_ATTR_AUTONEG_PROTOCOL,		/* u8 */
>> >+	DEVLINK_ATTR_MEDIA_AUTO_DETECT,		/* u8 */
>> >+	DEVLINK_ATTR_PHY_SELECT,		/* u8 */
>> >+	DEVLINK_ATTR_PRE_OS_LINK_SPEED_D0,	/* u8 */
>> >+	DEVLINK_ATTR_PRE_OS_LINK_SPEED_D3,	/* u8 */
>> >+	DEVLINK_ATTR_MBA_ENABLED,		/* u8 */
>> >+	DEVLINK_ATTR_MBA_BOOT_TYPE,		/* u8 */
>> >+	DEVLINK_ATTR_MBA_DELAY_TIME,		/* u32 */
>> >+	DEVLINK_ATTR_MBA_SETUP_HOT_KEY,		/* u8 */
>> >+	DEVLINK_ATTR_MBA_HIDE_SETUP_PROMPT,	/* u8 */
>> >+	DEVLINK_ATTR_MBA_BOOT_RETRY_COUNT,	/* u32 */
>> >+	DEVLINK_ATTR_MBA_VLAN_ENABLED,		/* u8 */
>> >+	DEVLINK_ATTR_MBA_VLAN_TAG,		/* u16 */
>> >+	DEVLINK_ATTR_MBA_BOOT_PROTOCOL,		/* u8 */
>> >+	DEVLINK_ATTR_MBA_LINK_SPEED,		/* u8 */
>> 
>> Okay, I think it is about the time we should start thinking about
>> putting this new config attributes under nester attribute. What do you
>> think?
>> 
>
>Steve and I actually had a similar discussion yesterday when I was doing
>a final review of the patches.
>
>My only objection to nesting was coming up with a way to describe these
>functions that made them seem different than existing configuration
>options.  In this case of the hardware we are trying to support these
>are all permanent config options, so we would call them
>DEVLINK_ATTR_NVRAM or DEVLINK_ATTR_PERM.  Does that seem reasonable to
>others?

The name should go hand-in-hand with the names of the cmds.

Re: [RFC 0/3] Adding config get/set to devlink

[Jiri Pirko]

Thu, Oct 12, 2017 at 10:12:31PM CEST, steven.lin1@broadcom.com wrote:
>On Thu, Oct 12, 2017 at 3:20 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 10/12/2017 12:06 PM, David Miller wrote:
>>> From: Florian Fainelli <f.fainelli@gmail.com>
>>> Date: Thu, 12 Oct 2017 08:43:59 -0700
>>>
>>>> Once we move ethtool (or however we name its successor) over to
>>>> netlink there is an opportunity for accessing objects that do and do
>>>> not have a netdevice representor today (e.g: management ports on
>>>> switches) with the same interface, and devlink could be used for
>>>> that.
>>>
>>> That is an interesting angle for including this in devlink.
>>>
>>> I'm not so sure what to do about this.
>>>
>>> One suggestion is that devlink is used for getting ethtool stats for
>>> objects lacking netdev representor's, and a new genetlink family is
>>> used for netdev based ethtool.
>>
>> Right, I was also thinking along those lines that we we would have a new
>> generic netlink family for ethtool to support ethtool over netlink.
>>
>>>
>>> I think it's important that we don't expand the scope of devlink
>>> beyond what it was originally designed for.
>>
>> It seems to me like devlink is well defined in what it is not for: it is
>> not meant to be used for any object that is/has a net_device, but it is
>> not well defined for what it can offer to these non network devices. For
>> instance, we have a tremendous amount of operations that are extremely
>> specific to its single user(s) such as mlx5 and mlxsw.
>>
>> For instance, I am not sure how the buffer reservation scheme can be
>> generalized, and this is always the tricky part with a single user
>> facility in that you try to generalize the best you can based on the HW
>> you know. This is not a criticism or meant to be anything negative, this
>> just happens to be the case, and we did not have anything better.
>>
>> So maybe the first thing is to clarify what devlink operations can and
>> should be and what they are absolutely not allowed to cover. We should
>> also clarify whether a generic set/get that Steven is proposing is
>> something that we tolerate, or whether there should be specific function
>> pointers implemented for each attribute, which would be more in line
>> with what has been done thus far.
>
>Hi Florian,
>
>Some of this is subjective, of course, but just to clarify, it did
>seem like implementing a new devlink_op function pointer per attribute
>might be more consistent with what's been done so far.  But for code
>reuse purposes - i.e. to avoid replicating essentially the same
>function for each of the 30+ config attributes - I elected to just
>implement a single generic get and set devlink_op.

Also, it this case, unlike any existing cmds, the config options are
all permanent, written in hw. I think it is fine to have one set
of get/set cmd to handle them all at once. Same family.

Re: [RFC 0/3] Adding config get/set to devlink

[Jiri Pirko]

Thu, Oct 12, 2017 at 11:53:56PM CEST, roopa@cumulusnetworks.com wrote:
>On Thu, Oct 12, 2017 at 12:20 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 10/12/2017 12:06 PM, David Miller wrote:
>>> From: Florian Fainelli <f.fainelli@gmail.com>
>>> Date: Thu, 12 Oct 2017 08:43:59 -0700
>>>
>>>> Once we move ethtool (or however we name its successor) over to
>>>> netlink there is an opportunity for accessing objects that do and do
>>>> not have a netdevice representor today (e.g: management ports on
>>>> switches) with the same interface, and devlink could be used for
>>>> that.
>>>
>>> That is an interesting angle for including this in devlink.
>>>
>>> I'm not so sure what to do about this.
>>>
>>> One suggestion is that devlink is used for getting ethtool stats for
>>> objects lacking netdev representor's, and a new genetlink family is
>>> used for netdev based ethtool.
>>
>> Right, I was also thinking along those lines that we we would have a new
>> generic netlink family for ethtool to support ethtool over netlink.
>
>new api is fine by me. The reason for suggesting devlink was because
>some of the devlink
>port_* ops are close to ethtool ops that can operate on a port/netdev.
>eg split_port could be a netdev operation
>unless you want to split before the netdev is created.

Let me correct you. The split is always devlink_port operation. In some
cases however when there is a mapping between devlink_port and netdev,
userspace part could translate netdev->devlink_port.


>
>There are some ops in devlink which are global hw parameters and not
>specific to a port, those fit perfectly with
>devlinks original goal.

There are 2 handles from the very beginning:
1) devlink - asic-wide handle
2) devlink_port - port handle

Re: [RFC 0/3] Adding config get/set to devlink

[Roopa Prabhu]

On Fri, Oct 13, 2017 at 12:11 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 12, 2017 at 11:53:56PM CEST, roopa@cumulusnetworks.com wrote:
>>On Thu, Oct 12, 2017 at 12:20 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 10/12/2017 12:06 PM, David Miller wrote:
>>>> From: Florian Fainelli <f.fainelli@gmail.com>
>>>> Date: Thu, 12 Oct 2017 08:43:59 -0700
>>>>
>>>>> Once we move ethtool (or however we name its successor) over to
>>>>> netlink there is an opportunity for accessing objects that do and do
>>>>> not have a netdevice representor today (e.g: management ports on
>>>>> switches) with the same interface, and devlink could be used for
>>>>> that.
>>>>
>>>> That is an interesting angle for including this in devlink.
>>>>
>>>> I'm not so sure what to do about this.
>>>>
>>>> One suggestion is that devlink is used for getting ethtool stats for
>>>> objects lacking netdev representor's, and a new genetlink family is
>>>> used for netdev based ethtool.
>>>
>>> Right, I was also thinking along those lines that we we would have a new
>>> generic netlink family for ethtool to support ethtool over netlink.
>>
>>new api is fine by me. The reason for suggesting devlink was because
>>some of the devlink
>>port_* ops are close to ethtool ops that can operate on a port/netdev.
>>eg split_port could be a netdev operation
>>unless you want to split before the netdev is created.
>
> Let me correct you. The split is always devlink_port operation. In some
> cases however when there is a mapping between devlink_port and netdev,
> userspace part could translate netdev->devlink_port.

yes, thats what i was trying to hint..that in some cases devlink_port
can already be mapped to a netdev.

>
>
>>
>>There are some ops in devlink which are global hw parameters and not
>>specific to a port, those fit perfectly with
>>devlinks original goal.
>
> There are 2 handles from the very beginning:
> 1) devlink - asic-wide handle
> 2) devlink_port - port handle

yep, i know that...and i was not trying to say that is a bad thing.

I think we will end up with devlink_port operations that could also be
done on a netdev down the lane. And, we may have to then argue where
an attribute will go. Hence my suggestion on classifying the api by
the target (driver in this case vs kernel networking for rtnetlink).
If you take netdev out of the picture, the port attributes that
devlink tries to set are similar to the ethtool port attributes today.
Also, it seemed like the new port attributes set api (proposed in this
thread) was close to the ethtool attributes set. Having all link hw
attributes in the same tool/api has  advantages. I have no plans to
move anything yet...so if the general preference is to keep devlink
netdev free for now, thats fine.