[RFC net-next 0/6] devlink: add device (driver) information API

[RFC net-next 0/6] devlink: add device (driver) information API

[Jakub Kicinski]

Hi!

For quite some time now the ethtool -i API has been showing its age.
The driver version field is generally considered obsolete these
days, and driver authors are encouraged to report the kernel version.
fw_version field does not suit modern needs with 31 characters being
quite limiting on more complex systems.  There is also no distinction
between the running and flashed versions of the firmware.

Since the driver information pertains to the entire device, rather
than a particular netdev, it seems wise to move it do devlink, at
the same time fixing the aforementioned issues.

The new API allows exposing the device serial number and versions
of the components of the card - both hardware, firmware (running
and flashed).  Driver authors can choose descriptive identifiers
for the version fields.  There is a potential for defining common
fields here, but given the general direction of the stack I don't
think people would like that.

Example:
$ devlink  info show
pci/0000:05:00.0:
  serial_number: 00:15:4d:12:20:7e
  versions:
    fixed:
      board.model carbon
      board.partno AMDA0099-0001
      board.revision 07
      board.vendor SMA
    running:
      fw.mgmt: 010156.010156.010156
      fw.cpld: 0x44
      fw.app: sriov-2.1.16
    stored:
      fw.mgmt: 010158.010158.010158
      fw.cpld: 0x44
      fw.app: sriov-2.1.20

Jakub Kicinski (6):
  devlink: add device information API
  devlink: add version reporting API
  nfp: devlink: report serial number
  nfp: devlink: report fixed versions
  nfp: nsp: add support for versions command
  nfp: devlink: report the running and flashed versions

 .../net/ethernet/netronome/nfp/nfp_devlink.c  | 154 +++++++++++++++
 .../ethernet/netronome/nfp/nfpcore/nfp_nsp.c  |  53 ++++++
 .../ethernet/netronome/nfp/nfpcore/nfp_nsp.h  |  14 ++
 include/net/devlink.h                         |  15 ++
 include/uapi/linux/devlink.h                  |  10 +
 net/core/devlink.c                            | 177 ++++++++++++++++++
 6 files changed, 423 insertions(+)

-- 
2.19.2

[RFC net-next 1/6] devlink: add device information API

[Jakub Kicinski]

ethtool -i has served us well for a long time, but its showing
its limitations more and more.  The device information should
also be reported per device not per-netdev.

Lay foundation for a simple devlink-based ethtool -i replacement.
Add device serial number as initial piece of information exposed
via this standard API.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 include/net/devlink.h        |   2 +
 include/uapi/linux/devlink.h |   4 ++
 net/core/devlink.c           | 112 +++++++++++++++++++++++++++++++++++
 3 files changed, 118 insertions(+)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 67f4293bc970..4358c111ce83 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -475,6 +475,8 @@ struct devlink_ops {
 	int (*eswitch_encap_mode_get)(struct devlink *devlink, u8 *p_encap_mode);
 	int (*eswitch_encap_mode_set)(struct devlink *devlink, u8 encap_mode,
 				      struct netlink_ext_ack *extack);
+	int (*serial_get)(struct devlink *devlink, u8 *buf, size_t buf_len,
+			  size_t *len, struct netlink_ext_ack *extack);
 };
 
 static inline void *devlink_priv(struct devlink *devlink)
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 6e52d3660654..760c9c360330 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -89,6 +89,8 @@ enum devlink_command {
 	DEVLINK_CMD_REGION_DEL,
 	DEVLINK_CMD_REGION_READ,
 
+	DEVLINK_CMD_INFO_GET,
+
 	/* add new commands above here */
 	__DEVLINK_CMD_MAX,
 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
@@ -285,6 +287,8 @@ enum devlink_attr {
 	DEVLINK_ATTR_REGION_CHUNK_ADDR,         /* u64 */
 	DEVLINK_ATTR_REGION_CHUNK_LEN,          /* u64 */
 
+	DEVLINK_ATTR_INFO_SERIAL_NUMBER,	/* binary */
+
 	/* 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 abb0da9d7b4b..55b7b006df28 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3597,6 +3597,110 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 	return 0;
 }
 
+static int devlink_nl_info_sn_fill(struct sk_buff *msg, struct devlink *devlink,
+				   struct netlink_ext_ack *extack)
+{
+	unsigned char sn[32];
+	size_t len = 0;
+	int err;
+
+	if (!devlink->ops->serial_get)
+		return 0;
+
+	err = devlink->ops->serial_get(devlink, sn, ARRAY_SIZE(sn), &len,
+				       extack);
+	if (err)
+		return err;
+
+	return nla_put(msg, DEVLINK_ATTR_INFO_SERIAL_NUMBER, len, sn);
+}
+
+static int
+devlink_nl_info_fill(struct sk_buff *msg, struct devlink *devlink,
+		     enum devlink_command cmd, u32 portid,
+		     u32 seq, int flags, struct netlink_ext_ack *extack)
+{
+	void *hdr;
+	int err;
+
+	hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags, cmd);
+	if (!hdr)
+		return -EMSGSIZE;
+
+	err = -EMSGSIZE;
+	if (devlink_nl_put_handle(msg, devlink))
+		goto err_cancel_msg;
+
+	err = devlink_nl_info_sn_fill(msg, devlink, extack);
+	if (err)
+		goto err_cancel_msg;
+
+	genlmsg_end(msg, hdr);
+	return 0;
+
+err_cancel_msg:
+	genlmsg_cancel(msg, hdr);
+	return err;
+}
+
+static int devlink_nl_cmd_info_get_doit(struct sk_buff *skb,
+					struct genl_info *info)
+{
+	struct devlink *devlink = info->user_ptr[0];
+	struct sk_buff *msg;
+	int err;
+
+	if (!devlink->ops || !devlink->ops->serial_get)
+		return -EOPNOTSUPP;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	err = devlink_nl_info_fill(msg, devlink, DEVLINK_CMD_INFO_GET,
+				   info->snd_portid, info->snd_seq, 0,
+				   info->extack);
+	if (err) {
+		nlmsg_free(msg);
+		return err;
+	}
+
+	return genlmsg_reply(msg, info);
+}
+
+static int devlink_nl_cmd_info_get_dumpit(struct sk_buff *msg,
+					  struct netlink_callback *cb)
+{
+	struct devlink *devlink;
+	int start = cb->args[0];
+	int idx = 0;
+	int err;
+
+	mutex_lock(&devlink_mutex);
+	list_for_each_entry(devlink, &devlink_list, list) {
+		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
+			continue;
+		if (idx < start) {
+			idx++;
+			continue;
+		}
+
+		mutex_lock(&devlink->lock);
+		err = devlink_nl_info_fill(msg, devlink, DEVLINK_CMD_INFO_GET,
+					   NETLINK_CB(cb->skb).portid,
+					   cb->nlh->nlmsg_seq, NLM_F_MULTI,
+					   cb->extack);
+		mutex_unlock(&devlink->lock);
+		if (err)
+			break;
+		idx++;
+	}
+	mutex_unlock(&devlink_mutex);
+
+	cb->args[0] = idx;
+	return msg->len;
+}
+
 static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING },
 	[DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING },
@@ -3842,6 +3946,14 @@ static const struct genl_ops devlink_nl_ops[] = {
 		.flags = GENL_ADMIN_PERM,
 		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
 	},
+	{
+		.cmd = DEVLINK_CMD_INFO_GET,
+		.doit = devlink_nl_cmd_info_get_doit,
+		.dumpit = devlink_nl_cmd_info_get_dumpit,
+		.policy = devlink_nl_policy,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
+		/* can be retrieved by unprivileged users */
+	},
 };
 
 static struct genl_family devlink_nl_family __ro_after_init = {
-- 
2.19.2

[RFC net-next 2/6] devlink: add version reporting API

[Jakub Kicinski]

ethtool -i has a few fixed-size fields which can be used to report
firmware version and expansion ROM version.  Unfortunately, modern
hardware has more firmware components.  There is usually some
datapath microcode, management controller, PXE drivers, and a
CPLD load.  Running ethtool -i on modern controllers reveals the
fact that vendors cram multiple values into firmware version field.

Here are some examples from systems I could lay my hands on quickly:

tg3:  "FFV20.2.17 bc 5720-v1.39"
i40e: "6.01 0x800034a4 1.1747.0"
nfp:  "0.0.3.5 0.25 sriov-2.1.16 nic"

Add a new devlink API to allow retrieving multiple versions, and
provide user-readable name for those versions.

While at it break down the versions into three categories:
 - fixed - this is the board/fixed component version, usually vendors
           report information like the board version in the PCI VPD,
           but it will benefit from naming and common API as well;
 - running - this is the running firmware version;
 - stored - this is firmware in the flash, after firmware update
            this value will reflect the flashed version, while the
            running version may only be updated after reboot.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 include/net/devlink.h        | 13 +++++++
 include/uapi/linux/devlink.h |  6 ++++
 net/core/devlink.c           | 67 +++++++++++++++++++++++++++++++++++-
 3 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 4358c111ce83..51f08edb1138 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -477,6 +477,8 @@ struct devlink_ops {
 				      struct netlink_ext_ack *extack);
 	int (*serial_get)(struct devlink *devlink, u8 *buf, size_t buf_len,
 			  size_t *len, struct netlink_ext_ack *extack);
+	int (*versions_get)(struct devlink *devlink, struct sk_buff *skb,
+			    struct netlink_ext_ack *extack);
 };
 
 static inline void *devlink_priv(struct devlink *devlink)
@@ -586,6 +588,10 @@ int devlink_region_snapshot_create(struct devlink_region *region, u64 data_len,
 				   u8 *data, u32 snapshot_id,
 				   devlink_snapshot_data_dest_t *data_destructor);
 
+int devlink_versions_report(struct sk_buff *skb, enum devlink_attr attr,
+			    const char *version_name,
+			    const char *version_value);
+
 #else
 
 static inline struct devlink *devlink_alloc(const struct devlink_ops *ops,
@@ -846,6 +852,13 @@ devlink_region_snapshot_create(struct devlink_region *region, u64 data_len,
 	return 0;
 }
 
+static inline int
+devlink_versions_report(struct sk_buff *skb, enum devlink_attr attr,
+			const char *version_name, const char *version_value)
+{
+	return 0;
+}
+
 #endif
 
 #endif /* _NET_DEVLINK_H_ */
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 760c9c360330..eb97a52246ef 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -288,6 +288,12 @@ enum devlink_attr {
 	DEVLINK_ATTR_REGION_CHUNK_LEN,          /* u64 */
 
 	DEVLINK_ATTR_INFO_SERIAL_NUMBER,	/* binary */
+	DEVLINK_ATTR_INFO_VERSIONS,		/* nested */
+	DEVLINK_ATTR_INFO_VERSIONS_FIXED,	/* nested */
+	DEVLINK_ATTR_INFO_VERSIONS_RUNNING,	/* nested */
+	DEVLINK_ATTR_INFO_VERSIONS_STORED,	/* nested */
+	DEVLINK_ATTR_INFO_VERSIONS_NAME,	/* string */
+	DEVLINK_ATTR_INFO_VERSIONS_VALUE,	/* string */
 
 	/* add new attributes above here, update the policy in devlink.c */
 
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 55b7b006df28..f35d917da7f2 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3615,6 +3615,32 @@ static int devlink_nl_info_sn_fill(struct sk_buff *msg, struct devlink *devlink,
 	return nla_put(msg, DEVLINK_ATTR_INFO_SERIAL_NUMBER, len, sn);
 }
 
+static int devlink_nl_info_versions_fill(struct sk_buff *msg,
+					 struct devlink *devlink,
+					 struct netlink_ext_ack *extack)
+{
+	struct nlattr *attr;
+	int err;
+
+	if (!devlink->ops->versions_get)
+		return 0;
+
+	attr = nla_nest_start(msg, DEVLINK_ATTR_INFO_VERSIONS);
+	if (!attr)
+		return -EMSGSIZE;
+
+	err = devlink->ops->versions_get(devlink, msg, extack);
+	if (err)
+		goto err_cancel;
+
+	nla_nest_end(msg, attr);
+	return 0;
+
+err_cancel:
+	nla_nest_cancel(msg, attr);
+	return err;
+}
+
 static int
 devlink_nl_info_fill(struct sk_buff *msg, struct devlink *devlink,
 		     enum devlink_command cmd, u32 portid,
@@ -3635,6 +3661,10 @@ devlink_nl_info_fill(struct sk_buff *msg, struct devlink *devlink,
 	if (err)
 		goto err_cancel_msg;
 
+	err = devlink_nl_info_versions_fill(msg, devlink, extack);
+	if (err)
+		goto err_cancel_msg;
+
 	genlmsg_end(msg, hdr);
 	return 0;
 
@@ -3650,7 +3680,8 @@ static int devlink_nl_cmd_info_get_doit(struct sk_buff *skb,
 	struct sk_buff *msg;
 	int err;
 
-	if (!devlink->ops || !devlink->ops->serial_get)
+	if (!devlink->ops ||
+	    (!devlink->ops->serial_get && !devlink->ops->versions_get))
 		return -EOPNOTSUPP;
 
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
@@ -3701,6 +3732,40 @@ static int devlink_nl_cmd_info_get_dumpit(struct sk_buff *msg,
 	return msg->len;
 }
 
+int devlink_versions_report(struct sk_buff *skb, enum devlink_attr attr,
+			    const char *version_name, const char *version_value)
+{
+	struct nlattr *nest;
+	int err;
+
+	if (attr < DEVLINK_ATTR_INFO_VERSIONS_FIXED ||
+	    attr > DEVLINK_ATTR_INFO_VERSIONS_STORED)
+		return -EINVAL;
+
+	nest = nla_nest_start(skb, attr);
+	if (!nest)
+		return -EMSGSIZE;
+
+	err = nla_put_string(skb, DEVLINK_ATTR_INFO_VERSIONS_NAME,
+			     version_name);
+	if (err)
+		goto nla_put_failure;
+
+	err = nla_put_string(skb, DEVLINK_ATTR_INFO_VERSIONS_VALUE,
+			     version_value);
+	if (err)
+		goto nla_put_failure;
+
+	nla_nest_end(skb, nest);
+
+	return 0;
+
+nla_put_failure:
+	nla_nest_cancel(skb, nest);
+	return err;
+}
+EXPORT_SYMBOL_GPL(devlink_versions_report);
+
 static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING },
 	[DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING },
-- 
2.19.2

[RFC net-next 3/6] nfp: devlink: report serial number

[Jakub Kicinski]

Report serial number via the info devlink command.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 .../net/ethernet/netronome/nfp/nfp_devlink.c  | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
index 808647ec3573..4422da939937 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
@@ -4,6 +4,7 @@
 #include <linux/rtnetlink.h>
 #include <net/devlink.h>
 
+#include "nfpcore/nfp_cpp.h"
 #include "nfpcore/nfp_nsp.h"
 #include "nfp_app.h"
 #include "nfp_main.h"
@@ -171,6 +172,26 @@ static int nfp_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode,
 	return ret;
 }
 
+static int
+nfp_devlink_serial_get(struct devlink *devlink, u8 *buf, size_t buf_len,
+		       size_t *len, struct netlink_ext_ack *extack)
+{
+	struct nfp_pf *pf = devlink_priv(devlink);
+	const u8 *serial;
+	int ret;
+
+	ret = nfp_cpp_serial(pf->cpp, &serial);
+	if (ret < 0)
+		return ret;
+
+	if (buf_len < ret)
+		return -EINVAL;
+	*len = ret;
+
+	memcpy(buf, serial, *len);
+	return 0;
+}
+
 const struct devlink_ops nfp_devlink_ops = {
 	.port_split		= nfp_devlink_port_split,
 	.port_unsplit		= nfp_devlink_port_unsplit,
@@ -178,6 +199,7 @@ const struct devlink_ops nfp_devlink_ops = {
 	.sb_pool_set		= nfp_devlink_sb_pool_set,
 	.eswitch_mode_get	= nfp_devlink_eswitch_mode_get,
 	.eswitch_mode_set	= nfp_devlink_eswitch_mode_set,
+	.serial_get		= nfp_devlink_serial_get,
 };
 
 int nfp_devlink_port_register(struct nfp_app *app, struct nfp_port *port)
-- 
2.19.2

[RFC net-next 4/6] nfp: devlink: report fixed versions

[Jakub Kicinski]

For now we only use HWinfo for fixed versions.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 .../net/ethernet/netronome/nfp/nfp_devlink.c  | 72 +++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
index 4422da939937..c7759564316d 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
@@ -192,6 +192,77 @@ nfp_devlink_serial_get(struct devlink *devlink, u8 *buf, size_t buf_len,
 	return 0;
 }
 
+static const struct nfp_devlink_versions_simple {
+	const char *key;
+	const char *hwinfo;
+} nfp_devlink_versions_hwinfo[] = {
+	{ "board.model",	"assembly.model", },
+	{ "board.partno",	"assembly.partno", },
+	{ "board.revision",	"assembly.revision", },
+	{ "board.vendor",	"assembly.vendor", },
+};
+
+static int
+nfp_devlink_versions_get_hwinfo(struct sk_buff *skb, struct nfp_nsp *nsp,
+				struct netlink_ext_ack *extack)
+{
+	unsigned int i;
+	int attr;
+	int err;
+
+	attr = DEVLINK_ATTR_INFO_VERSIONS_FIXED;
+
+	for (i = 0; i < ARRAY_SIZE(nfp_devlink_versions_hwinfo); i++) {
+		const struct nfp_devlink_versions_simple *info;
+		char buf[256] = {};
+
+		info = &nfp_devlink_versions_hwinfo[i];
+		strcpy(buf, info->hwinfo);
+
+		err = nfp_nsp_hwinfo_lookup(nsp, buf, sizeof(buf));
+		if (err == -ENOENT)
+			continue;
+		if (err) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "error reading versions string from FW");
+			return err;
+		}
+
+		err = devlink_versions_report(skb, attr, info->key, buf);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int
+nfp_devlink_versions_get(struct devlink *devlink, struct sk_buff *skb,
+			 struct netlink_ext_ack *extack)
+{
+	struct nfp_pf *pf = devlink_priv(devlink);
+	struct nfp_nsp *nsp;
+	int err;
+
+	nsp = nfp_nsp_open(pf->cpp);
+	if (IS_ERR(nsp)) {
+		NL_SET_ERR_MSG_MOD(extack, "can't access NSP");
+		return PTR_ERR(nsp);
+	}
+
+	err = nfp_devlink_versions_get_hwinfo(skb, nsp, extack);
+	if (err)
+		goto err_close_nsp;
+
+	nfp_nsp_close(nsp);
+
+	return 0;
+
+err_close_nsp:
+	nfp_nsp_close(nsp);
+	return err;
+}
+
 const struct devlink_ops nfp_devlink_ops = {
 	.port_split		= nfp_devlink_port_split,
 	.port_unsplit		= nfp_devlink_port_unsplit,
@@ -200,6 +271,7 @@ const struct devlink_ops nfp_devlink_ops = {
 	.eswitch_mode_get	= nfp_devlink_eswitch_mode_get,
 	.eswitch_mode_set	= nfp_devlink_eswitch_mode_set,
 	.serial_get		= nfp_devlink_serial_get,
+	.versions_get		= nfp_devlink_versions_get,
 };
 
 int nfp_devlink_port_register(struct nfp_app *app, struct nfp_port *port)
-- 
2.19.2

[RFC net-next 5/6] nfp: nsp: add support for versions command

[Jakub Kicinski]

Retrieve the FW versions with the new command.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 .../ethernet/netronome/nfp/nfpcore/nfp_nsp.c  | 53 +++++++++++++++++++
 .../ethernet/netronome/nfp/nfpcore/nfp_nsp.h  | 14 +++++
 2 files changed, 67 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.c
index ce1577bbbd2a..6b503277fd43 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.c
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.c
@@ -7,6 +7,7 @@
  *         Jason McMullan <jason.mcmullan@netronome.com>
  */
 
+#include <asm/unaligned.h>
 #include <linux/bitfield.h>
 #include <linux/delay.h>
 #include <linux/firmware.h>
@@ -62,6 +63,12 @@
 
 #define NFP_HWINFO_LOOKUP_SIZE	GENMASK(11, 0)
 
+#define NFP_VERSIONS_SIZE	GENMASK(11, 0)
+#define NFP_VERSIONS_CNT_OFF	0
+#define NFP_VERSIONS_BSP_OFF	2
+#define NFP_VERSIONS_CPLD_OFF	6
+#define NFP_VERSIONS_APP_OFF	10
+
 enum nfp_nsp_cmd {
 	SPCODE_NOOP		= 0, /* No operation */
 	SPCODE_SOFT_RESET	= 1, /* Soft reset the NFP */
@@ -77,6 +84,7 @@ enum nfp_nsp_cmd {
 	SPCODE_NSP_IDENTIFY	= 13, /* Read NSP version */
 	SPCODE_FW_STORED	= 16, /* If no FW loaded, load flash app FW */
 	SPCODE_HWINFO_LOOKUP	= 17, /* Lookup HWinfo with overwrites etc. */
+	SPCODE_VERSIONS 	= 20, /* Report FW versions */
 };
 
 static const struct {
@@ -711,3 +719,48 @@ int nfp_nsp_hwinfo_lookup(struct nfp_nsp *state, void *buf, unsigned int size)
 
 	return 0;
 }
+
+int nfp_nsp_versions(struct nfp_nsp *state, void *buf, unsigned int size)
+{
+	struct nfp_nsp_command_buf_arg versions = {
+		{
+			.code		= SPCODE_VERSIONS,
+			.option		= min_t(u32, size, NFP_VERSIONS_SIZE),
+		},
+		.out_buf	= buf,
+		.out_size	= min_t(u32, size, NFP_VERSIONS_SIZE),
+	};
+
+	return nfp_nsp_command_buf(state, &versions);
+}
+
+const char *nfp_nsp_versions_get(enum nfp_nsp_versions id, bool flash,
+				 const u8 *buf, unsigned int size)
+{
+	static const u32 id2off[] = {
+		[NFP_VERSIONS_BSP] =	NFP_VERSIONS_BSP_OFF,
+		[NFP_VERSIONS_CPLD] =	NFP_VERSIONS_CPLD_OFF,
+		[NFP_VERSIONS_APP] =	NFP_VERSIONS_APP_OFF,
+	};
+	unsigned int field, buf_field_cnt, buf_off;
+
+	if (id >= ARRAY_SIZE(id2off) || !id2off[id])
+		return ERR_PTR(-EINVAL);
+
+	field = id * 2 + flash;
+
+	buf_field_cnt = get_unaligned_le16(buf);
+	if (buf_field_cnt <= field)
+		return ERR_PTR(-ENOENT);
+
+	buf_off = get_unaligned_le16(buf + id2off[id] + flash * 2);
+	if (!buf_off)
+		return ERR_PTR(-ENOENT);
+
+	if (buf_off >= size)
+		return ERR_PTR(-EINVAL);
+	if (strnlen(&buf[buf_off], size - buf_off) == size - buf_off)
+		return ERR_PTR(-EINVAL);
+
+	return (const char *)&buf[buf_off];
+}
diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h
index ff33ac54097a..65b2b5819811 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp.h
@@ -38,6 +38,11 @@ static inline bool nfp_nsp_has_hwinfo_lookup(struct nfp_nsp *state)
 	return nfp_nsp_get_abi_ver_minor(state) > 24;
 }
 
+static inline bool nfp_nsp_has_versions(struct nfp_nsp *state)
+{
+	return nfp_nsp_get_abi_ver_minor(state) > 0xff;
+}
+
 enum nfp_eth_interface {
 	NFP_INTERFACE_NONE	= 0,
 	NFP_INTERFACE_SFP	= 1,
@@ -208,4 +213,13 @@ enum nfp_nsp_sensor_id {
 int nfp_hwmon_read_sensor(struct nfp_cpp *cpp, enum nfp_nsp_sensor_id id,
 			  long *val);
 
+enum nfp_nsp_versions {
+	NFP_VERSIONS_BSP,
+	NFP_VERSIONS_CPLD,
+	NFP_VERSIONS_APP,
+};
+
+int nfp_nsp_versions(struct nfp_nsp *state, void *buf, unsigned int size);
+const char *nfp_nsp_versions_get(enum nfp_nsp_versions id, bool flash,
+				 const u8 *buf, unsigned int size);
 #endif
-- 
2.19.2

[RFC net-next 6/6] nfp: devlink: report the running and flashed versions

[Jakub Kicinski]

Report versions of firmware components using the new NSP command.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 .../net/ethernet/netronome/nfp/nfp_devlink.c  | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
index c7759564316d..a0a8c6232827 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
@@ -236,6 +236,48 @@ nfp_devlink_versions_get_hwinfo(struct sk_buff *skb, struct nfp_nsp *nsp,
 	return 0;
 }
 
+static const struct nfp_devlink_versions {
+	enum nfp_nsp_versions id;
+	const char *key;
+} nfp_devlink_versions_nsp[] = {
+	{ NFP_VERSIONS_BSP,	"fw.mgmt", },
+	{ NFP_VERSIONS_CPLD,	"fw.cpld", },
+	{ NFP_VERSIONS_APP,	"fw.app", },
+};
+
+static int
+nfp_devlink_versions_get_nsp(struct sk_buff *skb, bool flash,
+			     const u8 *buf, unsigned int size)
+{
+	unsigned int i;
+	int attr;
+	int err;
+
+	attr = flash ? DEVLINK_ATTR_INFO_VERSIONS_STORED :
+		       DEVLINK_ATTR_INFO_VERSIONS_RUNNING;
+
+	for (i = 0; i < ARRAY_SIZE(nfp_devlink_versions_nsp); i++) {
+		const struct nfp_devlink_versions *info;
+		const char *version;
+
+		info = &nfp_devlink_versions_nsp[i];
+
+		version = nfp_nsp_versions_get(info->id, flash, buf, size);
+		if (IS_ERR(version)) {
+			if (PTR_ERR(version) == -ENOENT)
+				continue;
+			else
+				return PTR_ERR(version);
+		}
+
+		err = devlink_versions_report(skb, attr, info->key, version);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 static int
 nfp_devlink_versions_get(struct devlink *devlink, struct sk_buff *skb,
 			 struct netlink_ext_ack *extack)
@@ -254,6 +296,24 @@ nfp_devlink_versions_get(struct devlink *devlink, struct sk_buff *skb,
 	if (err)
 		goto err_close_nsp;
 
+	if (nfp_nsp_has_versions(nsp)) {
+		char buf[512] = {};
+
+		err = nfp_nsp_versions(nsp, buf, sizeof(buf));
+		if (err)
+			goto err_close_nsp;
+
+		err = nfp_devlink_versions_get_nsp(skb, false,
+						   buf, sizeof(buf));
+		if (err)
+			goto err_close_nsp;
+
+		err = nfp_devlink_versions_get_nsp(skb, true,
+						   buf, sizeof(buf));
+		if (err)
+			goto err_close_nsp;
+	}
+
 	nfp_nsp_close(nsp);
 
 	return 0;
-- 
2.19.2

[RFC iproute2-next] devlink: add info subcommand

[Jakub Kicinski]

Add support for reading the device serial number and versions
from the kernel.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 devlink/devlink.c       | 223 +++++++++++++++++++++++++++++++++++++++-
 man/man8/devlink-info.8 |  75 ++++++++++++++
 man/man8/devlink.8      |   5 +
 3 files changed, 302 insertions(+), 1 deletion(-)
 create mode 100644 man/man8/devlink-info.8

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 3651e90c1159..a8ba6e8d3d9d 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -199,6 +199,7 @@ static void ifname_map_free(struct ifname_map *ifname_map)
 #define DL_OPT_REGION_SNAPSHOT_ID	BIT(22)
 #define DL_OPT_REGION_ADDRESS		BIT(23)
 #define DL_OPT_REGION_LENGTH		BIT(24)
+#define DL_OPT_VERSIONS_TYPE		BIT(25)
 
 struct dl_opts {
 	uint32_t present; /* flags of present items */
@@ -230,6 +231,7 @@ struct dl_opts {
 	uint32_t region_snapshot_id;
 	uint64_t region_address;
 	uint64_t region_length;
+	int versions_attr;
 };
 
 struct dl {
@@ -383,6 +385,12 @@ static const enum mnl_attr_data_type devlink_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_REGION_CHUNK_DATA] = MNL_TYPE_BINARY,
 	[DEVLINK_ATTR_REGION_CHUNK_ADDR] = MNL_TYPE_U64,
 	[DEVLINK_ATTR_REGION_CHUNK_LEN] = MNL_TYPE_U64,
+	[DEVLINK_ATTR_INFO_VERSIONS] = MNL_TYPE_NESTED,
+	[DEVLINK_ATTR_INFO_VERSIONS_FIXED] = MNL_TYPE_NESTED,
+	[DEVLINK_ATTR_INFO_VERSIONS_RUNNING] = MNL_TYPE_NESTED,
+	[DEVLINK_ATTR_INFO_VERSIONS_STORED] = MNL_TYPE_NESTED,
+	[DEVLINK_ATTR_INFO_VERSIONS_NAME] = MNL_TYPE_STRING,
+	[DEVLINK_ATTR_INFO_VERSIONS_VALUE] = MNL_TYPE_STRING,
 };
 
 static int attr_cb(const struct nlattr *attr, void *data)
@@ -943,6 +951,21 @@ static int param_cmode_get(const char *cmodestr,
 	return 0;
 }
 
+static int versions_type_get(const char *typestr, int *p_attr)
+{
+	if (strcmp(typestr, "fixed") == 0) {
+		*p_attr = DEVLINK_ATTR_INFO_VERSIONS_FIXED;
+	} else if (strcmp(typestr, "stored") == 0) {
+		*p_attr = DEVLINK_ATTR_INFO_VERSIONS_STORED;
+	} else if (strcmp(typestr, "running") == 0) {
+		*p_attr = DEVLINK_ATTR_INFO_VERSIONS_RUNNING;
+	} else {
+		pr_err("Unknown versions type \"%s\"\n", typestr);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int dl_argv_parse(struct dl *dl, uint32_t o_required,
 			 uint32_t o_optional)
 {
@@ -1178,6 +1201,19 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required,
 			if (err)
 				return err;
 			o_found |= DL_OPT_REGION_LENGTH;
+		} else if (dl_argv_match(dl, "versions") &&
+			   (o_all & DL_OPT_VERSIONS_TYPE)) {
+			const char *versionstr;
+
+			dl_arg_inc(dl);
+			err = dl_argv_str(dl, &versionstr);
+			if (err)
+				return err;
+			err = versions_type_get(versionstr,
+						&opts->versions_attr);
+			if (err)
+				return err;
+			o_found |= DL_OPT_VERSIONS_TYPE;
 		} else {
 			pr_err("Unknown option \"%s\"\n", dl_argv(dl));
 			return -EINVAL;
@@ -1684,6 +1720,28 @@ static void pr_out_u64(struct dl *dl, const char *name, uint64_t val)
 	return pr_out_uint(dl, name, val);
 }
 
+static void pr_out_bytes(struct dl *dl, const char *name,
+			 uint8_t *data, uint32_t len)
+{
+	unsigned int i;
+
+	if (dl->json_output) {
+		jsonw_name(dl->jw, name);
+		jsonw_start_array(dl->jw);
+		for (i = 0; i < len; i++)
+			jsonw_printf(dl->jw, "%d", data[i]);
+		jsonw_end_array(dl->jw);
+	} else {
+		__pr_out_indent_inc();
+		__pr_out_newline();
+		pr_out("%s: %02hhx", name, data[0]);
+		for (i = 1; i < len; i++)
+			pr_out(":%02hhx", data[i]);
+		__pr_out_newline();
+		__pr_out_indent_dec();
+	}
+}
+
 static void pr_out_region_chunk_start(struct dl *dl, uint64_t addr)
 {
 	if (dl->json_output) {
@@ -1775,6 +1833,30 @@ static void pr_out_array_end(struct dl *dl)
 	}
 }
 
+static void pr_out_object_start(struct dl *dl, const char *name)
+{
+	if (dl->json_output) {
+		jsonw_name(dl->jw, name);
+		jsonw_start_object(dl->jw);
+	} else {
+		__pr_out_indent_inc();
+		__pr_out_newline();
+		pr_out("%s:", name);
+		__pr_out_indent_inc();
+		__pr_out_newline();
+	}
+}
+
+static void pr_out_object_end(struct dl *dl)
+{
+	if (dl->json_output) {
+		jsonw_end_object(dl->jw);
+	} else {
+		__pr_out_indent_dec();
+		__pr_out_indent_dec();
+	}
+}
+
 static void pr_out_entry_start(struct dl *dl)
 {
 	if (dl->json_output)
@@ -5557,11 +5639,147 @@ static int cmd_region(struct dl *dl)
 	return -ENOENT;
 }
 
+static void pr_out_versions_single(struct dl *dl, struct nlattr *versions_attr,
+				   const char *name, int type)
+{
+	struct nlattr *version;
+
+	if (dl->opts.versions_attr && dl->opts.versions_attr != type)
+		return;
+
+	mnl_attr_for_each_nested(version, versions_attr) {
+		struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
+		const char *ver_value;
+		const char *ver_name;
+		int err;
+
+		if (mnl_attr_get_type(version) != type)
+			continue;
+
+		err = mnl_attr_parse_nested(version, attr_cb, tb);
+		if (err != MNL_CB_OK)
+			continue;
+
+		if (!tb[DEVLINK_ATTR_INFO_VERSIONS_NAME] ||
+		    !tb[DEVLINK_ATTR_INFO_VERSIONS_VALUE])
+			continue;
+
+		if (name) {
+			pr_out_object_start(dl, name);
+			name = NULL;
+		}
+
+		ver_name = mnl_attr_get_str(tb[DEVLINK_ATTR_INFO_VERSIONS_NAME]);
+		ver_value = mnl_attr_get_str(tb[DEVLINK_ATTR_INFO_VERSIONS_VALUE]);
+
+		pr_out_str(dl, ver_name, ver_value);
+		if (!dl->json_output)
+			__pr_out_newline();
+	}
+
+	if (!name)
+		pr_out_object_end(dl);
+}
+
+static void pr_out_info(struct dl *dl, struct nlattr **tb)
+{
+	__pr_out_handle_start(dl, tb, true, false);
+
+	if (!dl->opts.versions_attr && tb[DEVLINK_ATTR_INFO_SERIAL_NUMBER]) {
+		struct nlattr *nla_sn = tb[DEVLINK_ATTR_INFO_SERIAL_NUMBER];
+
+		pr_out_bytes(dl, "serial_number",
+			     mnl_attr_get_payload(nla_sn),
+			     mnl_attr_get_payload_len(nla_sn));
+	}
+
+	if (tb[DEVLINK_ATTR_INFO_VERSIONS]) {
+		pr_out_object_start(dl, "versions");
+
+		pr_out_versions_single(dl, tb[DEVLINK_ATTR_INFO_VERSIONS],
+				       "fixed",
+				       DEVLINK_ATTR_INFO_VERSIONS_FIXED);
+		pr_out_versions_single(dl, tb[DEVLINK_ATTR_INFO_VERSIONS],
+				       "running",
+				       DEVLINK_ATTR_INFO_VERSIONS_RUNNING);
+		pr_out_versions_single(dl, tb[DEVLINK_ATTR_INFO_VERSIONS],
+				       "stored",
+				       DEVLINK_ATTR_INFO_VERSIONS_STORED);
+
+		pr_out_object_end(dl);
+	}
+
+	pr_out_handle_end(dl);
+}
+
+static int cmd_versions_show_cb(const struct nlmsghdr *nlh, void *data)
+{
+	struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh);
+	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
+	struct dl *dl = data;
+
+	mnl_attr_parse(nlh, sizeof(*genl), attr_cb, tb);
+
+	if (!tb[DEVLINK_ATTR_BUS_NAME] || !tb[DEVLINK_ATTR_DEV_NAME] ||
+	    (!tb[DEVLINK_ATTR_INFO_VERSIONS] &&
+	     !tb[DEVLINK_ATTR_INFO_SERIAL_NUMBER]))
+		return MNL_CB_ERROR;
+
+	pr_out_info(dl, tb);
+
+	return MNL_CB_OK;
+}
+
+static int cmd_info_show(struct dl *dl)
+{
+	struct nlmsghdr *nlh;
+	uint16_t flags = NLM_F_REQUEST | NLM_F_ACK;
+	int err;
+
+	if (dl_argc(dl) == 0)
+		flags |= NLM_F_DUMP;
+
+	nlh = mnlg_msg_prepare(dl->nlg, DEVLINK_CMD_INFO_GET, flags);
+
+	if (dl_argc(dl) > 0) {
+		err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE,
+					DL_OPT_VERSIONS_TYPE);
+		if (err)
+			return err;
+	}
+
+	pr_out_section_start(dl, "info");
+	err = _mnlg_socket_sndrcv(dl->nlg, nlh, cmd_versions_show_cb, dl);
+	pr_out_section_end(dl);
+	return err;
+}
+
+static void cmd_info_help(void)
+{
+	pr_err("Usage: devlink info show [ DEV [ { versions [ VTYPE ] } ] ]\n"
+	       "       VTYPE := { fixed | running | stored }\n");
+}
+
+static int cmd_info(struct dl *dl)
+{
+	if (dl_no_arg(dl)) {
+		return cmd_info_show(dl);
+	} else if (dl_argv_match(dl, "help")) {
+		cmd_info_help();
+		return 0;
+	} else if (dl_argv_match(dl, "show")) {
+		dl_arg_inc(dl);
+		return cmd_info_show(dl);
+	}
+	pr_err("Command \"%s\" not found\n", dl_argv(dl));
+	return -ENOENT;
+}
+
 static void help(void)
 {
 	pr_err("Usage: devlink [ OPTIONS ] OBJECT { COMMAND | help }\n"
 	       "       devlink [ -f[orce] ] -b[atch] filename\n"
-	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region }\n"
+	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region | info }\n"
 	       "       OPTIONS := { -V[ersion] | -n[o-nice-names] | -j[son] | -p[retty] | -v[erbose] }\n");
 }
 
@@ -5594,6 +5812,9 @@ static int dl_cmd(struct dl *dl, int argc, char **argv)
 	} else if (dl_argv_match(dl, "region")) {
 		dl_arg_inc(dl);
 		return cmd_region(dl);
+	} else if (dl_argv_match(dl, "info")) {
+		dl_arg_inc(dl);
+		return cmd_info(dl);
 	}
 	pr_err("Object \"%s\" not found\n", dl_argv(dl));
 	return -ENOENT;
diff --git a/man/man8/devlink-info.8 b/man/man8/devlink-info.8
new file mode 100644
index 000000000000..f32b05091f51
--- /dev/null
+++ b/man/man8/devlink-info.8
@@ -0,0 +1,75 @@
+.TH DEVLINK\-INFO 8 "11 Jan 2019" "iproute2" "Linux"
+.SH NAME
+devlink-info \- devlink device info reporting
+.SH SYNOPSIS
+.sp
+.ad l
+.in +8
+.ti -8
+.B devlink
+.RI "[ " OPTIONS " ]"
+.B info
+.RI  " { " COMMAND " | "
+.BR help " }"
+.sp
+
+.ti -8
+.IR OPTIONS " := { "
+\fB\-V\fR[\fIersion\fR] |
+\fB\-n\fR[\fIno-nice-names\fR] }
+
+.ti -8
+.B devlink info show
+.RI "[ " DEV " ]"
+.RI "["
+.BR versions
+.RI "{ "
+.BR fixed " | " running " | " stored
+.RI "} "
+.RI "]"
+
+.ti -8
+.B devlink info help
+
+.SH "DESCRIPTION"
+.SS devlink info show - display device information
+Display device information provided by the driver. This command can be used
+to query versions of the hardware components or device components which
+can't be updated (
+.I fixed
+) as well as device firmware which can be updated. For firmware components
+.I running
+displays the versions of firmware currently loaded into the devce, while
+.I stored
+reports the versions in device flash.
+.I Running
+and
+.I stored
+versions may be different after flash update, but before reboot.
+
+.PP
+.I "DEV"
+- specifies the devlink device to show.
+If this argument is omitted all devices are listed.
+
+.in +4
+Format is:
+.in +2
+BUS_NAME/BUS_ADDRESS
+
+.PP
+.BR versions
+.BR fixed
+.RI " | "
+.BR running
+.RI " | "
+.BR stored
+- specifies the versions category to show.
+If this argument is omitted all categories are listed.
+
+.SH SEE ALSO
+.BR devlink (8),
+.br
+
+.SH AUTHOR
+Jakub Kicinski <jakub.kicinski@netronome.com>
diff --git a/man/man8/devlink.8 b/man/man8/devlink.8
index 8d527e7e1d60..4cb3a1113117 100644
--- a/man/man8/devlink.8
+++ b/man/man8/devlink.8
@@ -78,6 +78,10 @@ Turn on verbose output.
 .B region
 - devlink address region access
 
+.TP
+.B info
+- devlink device info reporting.
+
 .SS
 .I COMMAND
 
@@ -109,6 +113,7 @@ Exit status is 0 if command was successful or a positive integer upon failure.
 .BR devlink-sb (8),
 .BR devlink-resource (8),
 .BR devlink-region (8),
+.BR devlink-info (8),
 .br
 
 .SH REPORTING BUGS
-- 
2.19.2

Re: [RFC net-next 0/6] devlink: add device (driver) information API

[Florian Fainelli]

On 1/14/19 4:50 PM, Jakub Kicinski wrote:
> Hi!
> 
> For quite some time now the ethtool -i API has been showing its age.
> The driver version field is generally considered obsolete these
> days, and driver authors are encouraged to report the kernel version.
> fw_version field does not suit modern needs with 31 characters being
> quite limiting on more complex systems.  There is also no distinction
> between the running and flashed versions of the firmware.
> 
> Since the driver information pertains to the entire device, rather
> than a particular netdev, it seems wise to move it do devlink, at
> the same time fixing the aforementioned issues.
> 
> The new API allows exposing the device serial number and versions
> of the components of the card - both hardware, firmware (running
> and flashed).  Driver authors can choose descriptive identifiers
> for the version fields.  There is a potential for defining common
> fields here, but given the general direction of the stack I don't
> think people would like that.

I does not seem realistic to expect people to agree on that indeed :)

Took a quick look and this is pretty sensible thanks Jakub!

> 
> Example:
> $ devlink  info show
> pci/0000:05:00.0:
>   serial_number: 00:15:4d:12:20:7e
>   versions:
>     fixed:
>       board.model carbon
>       board.partno AMDA0099-0001
>       board.revision 07
>       board.vendor SMA
>     running:
>       fw.mgmt: 010156.010156.010156
>       fw.cpld: 0x44
>       fw.app: sriov-2.1.16
>     stored:
>       fw.mgmt: 010158.010158.010158
>       fw.cpld: 0x44
>       fw.app: sriov-2.1.20
> 
> Jakub Kicinski (6):
>   devlink: add device information API
>   devlink: add version reporting API
>   nfp: devlink: report serial number
>   nfp: devlink: report fixed versions
>   nfp: nsp: add support for versions command
>   nfp: devlink: report the running and flashed versions
> 
>  .../net/ethernet/netronome/nfp/nfp_devlink.c  | 154 +++++++++++++++
>  .../ethernet/netronome/nfp/nfpcore/nfp_nsp.c  |  53 ++++++
>  .../ethernet/netronome/nfp/nfpcore/nfp_nsp.h  |  14 ++
>  include/net/devlink.h                         |  15 ++
>  include/uapi/linux/devlink.h                  |  10 +
>  net/core/devlink.c                            | 177 ++++++++++++++++++
>  6 files changed, 423 insertions(+)
> 


-- 
Florian

Re: [RFC net-next 0/6] devlink: add device (driver) information API

[Andrew Lunn]

On Mon, Jan 14, 2019 at 04:50:02PM -0800, Jakub Kicinski wrote:
> Hi!
> 
> For quite some time now the ethtool -i API has been showing its age.
> The driver version field is generally considered obsolete these
> days, and driver authors are encouraged to report the kernel version.
> fw_version field does not suit modern needs with 31 characters being
> quite limiting on more complex systems.  There is also no distinction
> between the running and flashed versions of the firmware.

Hi Jakub

I agree with what you are saying. However, the ethtool API is not
going to go away anyway soon. By introducing a second place to look
for version information, we are just going to confuse users.

There is an effort going on to replace the basic ethtool kAPI with a
netlink socket. That opens up the possibility to return additional
information. We can avoid the two places to look. Just tell your users
to use the netlink version of ethtool and they can get additional
version information.

I can understand using devlink where there is no existing API, but
when we do have an API, we should try to avoid duplicating it in a new
tool, just to extend it.

       Andrew

Re: [RFC net-next 0/6] devlink: add device (driver) information API

[Jakub Kicinski]

On Tue, 15 Jan 2019 02:18:59 +0100, Andrew Lunn wrote:
> On Mon, Jan 14, 2019 at 04:50:02PM -0800, Jakub Kicinski wrote:
> > Hi!
> > 
> > For quite some time now the ethtool -i API has been showing its age.
> > The driver version field is generally considered obsolete these
> > days, and driver authors are encouraged to report the kernel version.
> > fw_version field does not suit modern needs with 31 characters being
> > quite limiting on more complex systems.  There is also no distinction
> > between the running and flashed versions of the firmware.  
> 
> Hi Jakub
> 
> I agree with what you are saying. However, the ethtool API is not
> going to go away anyway soon. By introducing a second place to look
> for version information, we are just going to confuse users.
> 
> There is an effort going on to replace the basic ethtool kAPI with a
> netlink socket. That opens up the possibility to return additional
> information. We can avoid the two places to look. Just tell your users
> to use the netlink version of ethtool and they can get additional
> version information.
> 
> I can understand using devlink where there is no existing API, but
> when we do have an API, we should try to avoid duplicating it in a new
> tool, just to extend it.

I think the plan was to use this opportunity to move the information
which belongs in devlink to devlink.  There is absolutely nothing
netdev specific here, and ethtool uses a netdev as a handle.  We can
have the new ethtool command just issue a devlink request behind the
scenes if we care.

There are failure modes in which netdevs are not available because FW
init fails, but the driver can hang onto the device with devlink
active, to allow users to troubleshoot and hopefully one day - flash 
a new FW (flashing is another thing that doesn't belong in ethtool).

Re: [RFC net-next 0/6] devlink: add device (driver) information API

[Andrew Lunn]

> I think the plan was to use this opportunity to move the information
> which belongs in devlink to devlink.  There is absolutely nothing
> netdev specific here, and ethtool uses a netdev as a handle.  We can
> have the new ethtool command just issue a devlink request behind the
> scenes if we care.

Hi Jakub

Using that argument, you should probably make the devlink core call
the ethtool .get_drvinfo op if the device does not implement the
devlink op.

    Andrew

Re: [RFC net-next 0/6] devlink: add device (driver) information API

[Jakub Kicinski]

On Tue, 15 Jan 2019 02:57:55 +0100, Andrew Lunn wrote:
> > I think the plan was to use this opportunity to move the information
> > which belongs in devlink to devlink.  There is absolutely nothing
> > netdev specific here, and ethtool uses a netdev as a handle.  We can
> > have the new ethtool command just issue a devlink request behind the
> > scenes if we care.  
> 
> Hi Jakub
> 
> Using that argument, you should probably make the devlink core call
> the ethtool .get_drvinfo op if the device does not implement the
> devlink op.

Could it possibly be done in user space?  Have iproute2/devlink call old
ethtool API on first netdev associated with one of the ports?

What concerns me is that the versions drivers report become de facto
uABI.  Deployment scripts all over the place may be using bits of
information contained in there.  If we start reporting .get_drvinfo as,
let's say, field "ethtool.fw_version" or "legacy.fw_version" of devlink
output we may have to _always_ report it.  We can't make it disappear
once the driver decides to break down the info in more detail.

We also don't really know whether ethool.fw_version is fixed, flashed
or running version.

Hm..

Re: [RFC net-next 0/6] devlink: add device (driver) information API

[Michal Kubecek]

On Mon, Jan 14, 2019 at 07:27:34PM -0800, Jakub Kicinski wrote:
> On Tue, 15 Jan 2019 02:57:55 +0100, Andrew Lunn wrote:
> > 
> > Using that argument, you should probably make the devlink core call
> > the ethtool .get_drvinfo op if the device does not implement the
> > devlink op.
> 
> Could it possibly be done in user space?  Have iproute2/devlink call old
> ethtool API on first netdev associated with one of the ports?

I suppose it could, as long as there is an interface to find the
corresponding netdev name (or ifindex). Some time ago I tried to find
a way to get a devlink handle for a given net device and I couldn't find
any straightforward way even in kernel. (I needed the opposite direction
as I was investigating the options for implementing ethtool dump/flash
commands using devlink regions.)

Michal Kubecek

Re: [RFC net-next 0/6] devlink: add device (driver) information API

[Jiri Pirko]

Tue, Jan 15, 2019 at 02:57:55AM CET, andrew@lunn.ch wrote:
>> I think the plan was to use this opportunity to move the information
>> which belongs in devlink to devlink.  There is absolutely nothing
>> netdev specific here, and ethtool uses a netdev as a handle.  We can
>> have the new ethtool command just issue a devlink request behind the
>> scenes if we care.
>
>Hi Jakub
>
>Using that argument, you should probably make the devlink core call
>the ethtool .get_drvinfo op if the device does not implement the
>devlink op.

I imagine it happening the other way around. Updated drivers would
implement only the devlink interface. A compat layer would be introduced
to redirect from ethtool calls into devlink to gather information (like
fw version and other stuff which is currently in ethtool).

This approach would provide a single and complete interface between
driver and kernel and it would also maintain exinsting ethtool
interface.


>
>    Andrew

Re: [RFC iproute2-next] devlink: add info subcommand

[Jiri Pirko]

[...]

> static void help(void)
> {
> 	pr_err("Usage: devlink [ OPTIONS ] OBJECT { COMMAND | help }\n"
> 	       "       devlink [ -f[orce] ] -b[atch] filename\n"
>-	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region }\n"
>+	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region | info }\n"

I think that "info" should be nested under "dev". It is related to dev.
Maybe it even does not have to be a separate command and can be a nested
attribute to existing DEVLINK_CMD_GET cmd.



> 	       "       OPTIONS := { -V[ersion] | -n[o-nice-names] | -j[son] | -p[retty] | -v[erbose] }\n");
> }

Re: [RFC net-next 2/6] devlink: add version reporting API

[Jiri Pirko]

Tue, Jan 15, 2019 at 01:50:04AM CET, jakub.kicinski@netronome.com wrote:

[...]

>@@ -3701,6 +3732,40 @@ static int devlink_nl_cmd_info_get_dumpit(struct sk_buff *msg,
> 	return msg->len;
> }
> 
>+int devlink_versions_report(struct sk_buff *skb, enum devlink_attr attr,
>+			    const char *version_name, const char *version_value)

Hmm, so this is a helper to construct the msg. I think it would be good
to have this somehow separated from the netlink. Meaning a separate enum
just for fixed/running/stored and perhaps a "context structure" which
you can use just as a pointer with layout invisible for the driver
(containing skb). Passing skb itself is a bit confusing and gives the
driver opportunity to put in some weird crap. Better to disallow it by
the api design.


>+{
>+	struct nlattr *nest;
>+	int err;
>+
>+	if (attr < DEVLINK_ATTR_INFO_VERSIONS_FIXED ||
>+	    attr > DEVLINK_ATTR_INFO_VERSIONS_STORED)
>+		return -EINVAL;
>+
>+	nest = nla_nest_start(skb, attr);
>+	if (!nest)
>+		return -EMSGSIZE;
>+
>+	err = nla_put_string(skb, DEVLINK_ATTR_INFO_VERSIONS_NAME,
>+			     version_name);
>+	if (err)
>+		goto nla_put_failure;
>+
>+	err = nla_put_string(skb, DEVLINK_ATTR_INFO_VERSIONS_VALUE,
>+			     version_value);
>+	if (err)
>+		goto nla_put_failure;
>+
>+	nla_nest_end(skb, nest);
>+
>+	return 0;
>+
>+nla_put_failure:
>+	nla_nest_cancel(skb, nest);
>+	return err;
>+}
>+EXPORT_SYMBOL_GPL(devlink_versions_report);
>+
> static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
> 	[DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING },
> 	[DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING },
>-- 
>2.19.2
>

Re: [RFC net-next 1/6] devlink: add device information API

[Jiri Pirko]

Tue, Jan 15, 2019 at 01:50:03AM CET, jakub.kicinski@netronome.com wrote:

[...]

>+static int devlink_nl_info_sn_fill(struct sk_buff *msg, struct devlink *devlink,
>+				   struct netlink_ext_ack *extack)
>+{
>+	unsigned char sn[32];

:/ Not good to have number directly here.

>+	size_t len = 0;
>+	int err;
>+
>+	if (!devlink->ops->serial_get)
>+		return 0;
>+
>+	err = devlink->ops->serial_get(devlink, sn, ARRAY_SIZE(sn), &len,
>+				       extack);

Maybe it would be good to have a helper to fill this as well. That way,
driver could pass buffer of any length and helper would encode it into
skb.



>+	if (err)
>+		return err;
>+
>+	return nla_put(msg, DEVLINK_ATTR_INFO_SERIAL_NUMBER, len, sn);
>+}

[...]

Re: [RFC net-next 4/6] nfp: devlink: report fixed versions

[Jiri Pirko]

Tue, Jan 15, 2019 at 01:50:06AM CET, jakub.kicinski@netronome.com wrote:
>For now we only use HWinfo for fixed versions.
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>---
> .../net/ethernet/netronome/nfp/nfp_devlink.c  | 72 +++++++++++++++++++
> 1 file changed, 72 insertions(+)
>
>diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>index 4422da939937..c7759564316d 100644
>--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>@@ -192,6 +192,77 @@ nfp_devlink_serial_get(struct devlink *devlink, u8 *buf, size_t buf_len,
> 	return 0;
> }
> 
>+static const struct nfp_devlink_versions_simple {
>+	const char *key;
>+	const char *hwinfo;
>+} nfp_devlink_versions_hwinfo[] = {
>+	{ "board.model",	"assembly.model", },
>+	{ "board.partno",	"assembly.partno", },
>+	{ "board.revision",	"assembly.revision", },
>+	{ "board.vendor",	"assembly.vendor", },

That means that every driver would re-define these strings. Would it
make sense to have them (at least some of them) defined in a generic
way? Something in a sense of devlink params, where we have generic and
driver-specific ones.


>+};
>+
>+static int
>+nfp_devlink_versions_get_hwinfo(struct sk_buff *skb, struct nfp_nsp *nsp,
>+				struct netlink_ext_ack *extack)
>+{
>+	unsigned int i;
>+	int attr;
>+	int err;
>+
>+	attr = DEVLINK_ATTR_INFO_VERSIONS_FIXED;
>+
>+	for (i = 0; i < ARRAY_SIZE(nfp_devlink_versions_hwinfo); i++) {
>+		const struct nfp_devlink_versions_simple *info;
>+		char buf[256] = {};
>+
>+		info = &nfp_devlink_versions_hwinfo[i];
>+		strcpy(buf, info->hwinfo);
>+
>+		err = nfp_nsp_hwinfo_lookup(nsp, buf, sizeof(buf));
>+		if (err == -ENOENT)
>+			continue;
>+		if (err) {
>+			NL_SET_ERR_MSG_MOD(extack,
>+					   "error reading versions string from FW");
>+			return err;
>+		}
>+
>+		err = devlink_versions_report(skb, attr, info->key, buf);

So this is always "DEVLINK_ATTR_INFO_VERSIONS_FIXED". I don't
understand.


>+		if (err)
>+			return err;
>+	}
>+
>+	return 0;
>+}
>+
>+static int
>+nfp_devlink_versions_get(struct devlink *devlink, struct sk_buff *skb,
>+			 struct netlink_ext_ack *extack)
>+{
>+	struct nfp_pf *pf = devlink_priv(devlink);
>+	struct nfp_nsp *nsp;
>+	int err;
>+
>+	nsp = nfp_nsp_open(pf->cpp);
>+	if (IS_ERR(nsp)) {
>+		NL_SET_ERR_MSG_MOD(extack, "can't access NSP");
>+		return PTR_ERR(nsp);
>+	}
>+
>+	err = nfp_devlink_versions_get_hwinfo(skb, nsp, extack);
>+	if (err)
>+		goto err_close_nsp;
>+
>+	nfp_nsp_close(nsp);
>+
>+	return 0;
>+
>+err_close_nsp:
>+	nfp_nsp_close(nsp);
>+	return err;
>+}
>+
> const struct devlink_ops nfp_devlink_ops = {
> 	.port_split		= nfp_devlink_port_split,
> 	.port_unsplit		= nfp_devlink_port_unsplit,
>@@ -200,6 +271,7 @@ const struct devlink_ops nfp_devlink_ops = {
> 	.eswitch_mode_get	= nfp_devlink_eswitch_mode_get,
> 	.eswitch_mode_set	= nfp_devlink_eswitch_mode_set,
> 	.serial_get		= nfp_devlink_serial_get,
>+	.versions_get		= nfp_devlink_versions_get,
> };
> 
> int nfp_devlink_port_register(struct nfp_app *app, struct nfp_port *port)
>-- 
>2.19.2
>

Re: [RFC iproute2-next] devlink: add info subcommand

[Andrew Lunn]

On Tue, Jan 15, 2019 at 09:20:11AM +0100, Jiri Pirko wrote:
> [...]
> 
> > static void help(void)
> > {
> > 	pr_err("Usage: devlink [ OPTIONS ] OBJECT { COMMAND | help }\n"
> > 	       "       devlink [ -f[orce] ] -b[atch] filename\n"
> >-	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region }\n"
> >+	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region | info }\n"
> 
> I think that "info" should be nested under "dev". It is related to dev.
> Maybe it even does not have to be a separate command and can be a nested
> attribute to existing DEVLINK_CMD_GET cmd.

Hi Jiri

Not that i agree devlink is the right place for this, but i think it
probably needs to be nested both under dev and port. I could have a
line card implementing a port which has version information, as well
as version information for the backplane which would be under dev.

   Andrew

Re: [RFC iproute2-next] devlink: add info subcommand

[Jiri Pirko]

Tue, Jan 15, 2019 at 03:00:46PM CET, andrew@lunn.ch wrote:
>On Tue, Jan 15, 2019 at 09:20:11AM +0100, Jiri Pirko wrote:
>> [...]
>> 
>> > static void help(void)
>> > {
>> > 	pr_err("Usage: devlink [ OPTIONS ] OBJECT { COMMAND | help }\n"
>> > 	       "       devlink [ -f[orce] ] -b[atch] filename\n"
>> >-	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region }\n"
>> >+	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region | info }\n"
>> 
>> I think that "info" should be nested under "dev". It is related to dev.
>> Maybe it even does not have to be a separate command and can be a nested
>> attribute to existing DEVLINK_CMD_GET cmd.
>
>Hi Jiri
>
>Not that i agree devlink is the right place for this, but i think it

Where else to put it?

>probably needs to be nested both under dev and port. I could have a
>line card implementing a port which has version information, as well
>as version information for the backplane which would be under dev.

Okay. Sounds good to me.


>
>   Andrew

Re: [RFC net-next 1/6] devlink: add device information API

[Jakub Kicinski]

On Tue, 15 Jan 2019 11:15:12 +0100, Jiri Pirko wrote:
> Tue, Jan 15, 2019 at 01:50:03AM CET, jakub.kicinski@netronome.com wrote:
> >+static int devlink_nl_info_sn_fill(struct sk_buff *msg, struct devlink *devlink,
> >+				   struct netlink_ext_ack *extack)
> >+{
> >+	unsigned char sn[32];  
> 
> :/ Not good to have number directly here.
> 
> >+	size_t len = 0;
> >+	int err;
> >+
> >+	if (!devlink->ops->serial_get)
> >+		return 0;
> >+
> >+	err = devlink->ops->serial_get(devlink, sn, ARRAY_SIZE(sn), &len,
> >+				       extack);  
> 
> Maybe it would be good to have a helper to fill this as well. That way,
> driver could pass buffer of any length and helper would encode it into
> skb.

Ack, will do.  IMHO it's a bit of an overkill, one could "safely
assume" serial number won't be longer than 32 bytes.. :)  But no
problem, will do!

> >+	if (err)
> >+		return err;
> >+
> >+	return nla_put(msg, DEVLINK_ATTR_INFO_SERIAL_NUMBER, len, sn);
> >+}  

Re: [RFC iproute2-next] devlink: add info subcommand

[Jakub Kicinski]

On Tue, 15 Jan 2019 09:20:11 +0100, Jiri Pirko wrote:
> > static void help(void)
> > {
> > 	pr_err("Usage: devlink [ OPTIONS ] OBJECT { COMMAND | help }\n"
> > 	       "       devlink [ -f[orce] ] -b[atch] filename\n"
> >-	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region }\n"
> >+	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region | info }\n"  
> 
> I think that "info" should be nested under "dev". It is related to dev.

Ack.

> Maybe it even does not have to be a separate command and can be a nested
> attribute to existing DEVLINK_CMD_GET cmd.

I thought about that, but I'd rather keep it as a separate command.  
I think it'd be good to keep DEVLINK_CMD_GET nice and lean.  

For versions there may be FW communication required, and reading stuff
out of flash.  I bit of overhead for users who just want the list of
devlink instances.

Having in under dev but as a separate command seems quite nice indeed.
Especially given that there can only be a show subcommand..  So:

For dump:
$ devlink dev info

But for get:
$ devlink dev pci/0000:82:00.0 info

or

$ devlink dev info pci/0000:82:00.0

?

Re: [RFC iproute2-next] devlink: add info subcommand

[Jakub Kicinski]

On Tue, 15 Jan 2019 15:00:46 +0100, Andrew Lunn wrote:
> I could have a line card implementing a port which has version
> information, as well as version information for the backplane which
> would be under dev.

I'd argue a line card is not a port, so adding port info for line cards
is, again, taking a step backward towards unclear semantics.  Line cards
are a very well understood concept, and they deserve their own
handling.  And ports may _belong_ to line cards.

Re: [RFC iproute2-next] devlink: add info subcommand

[Jiri Pirko]

Tue, Jan 15, 2019 at 06:53:52PM CET, jakub.kicinski@netronome.com wrote:
>On Tue, 15 Jan 2019 09:20:11 +0100, Jiri Pirko wrote:
>> > static void help(void)
>> > {
>> > 	pr_err("Usage: devlink [ OPTIONS ] OBJECT { COMMAND | help }\n"
>> > 	       "       devlink [ -f[orce] ] -b[atch] filename\n"
>> >-	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region }\n"
>> >+	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region | info }\n"  
>> 
>> I think that "info" should be nested under "dev". It is related to dev.
>
>Ack.
>
>> Maybe it even does not have to be a separate command and can be a nested
>> attribute to existing DEVLINK_CMD_GET cmd.
>
>I thought about that, but I'd rather keep it as a separate command.  
>I think it'd be good to keep DEVLINK_CMD_GET nice and lean.  

Okay. Fair enough.


>
>For versions there may be FW communication required, and reading stuff
>out of flash.  I bit of overhead for users who just want the list of
>devlink instances.
>
>Having in under dev but as a separate command seems quite nice indeed.
>Especially given that there can only be a show subcommand..  So:
>
>For dump:
>$ devlink dev info
>
>But for get:
>$ devlink dev pci/0000:82:00.0 info
>
>or
>
>$ devlink dev info pci/0000:82:00.0

This is aligned with the rest.


>
>?

Re: [RFC net-next 4/6] nfp: devlink: report fixed versions

[Jakub Kicinski]

On Tue, 15 Jan 2019 11:18:18 +0100, Jiri Pirko wrote:
> Tue, Jan 15, 2019 at 01:50:06AM CET, jakub.kicinski@netronome.com wrote:
> >For now we only use HWinfo for fixed versions.
> >
> >Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> >---
> > .../net/ethernet/netronome/nfp/nfp_devlink.c  | 72 +++++++++++++++++++
> > 1 file changed, 72 insertions(+)
> >
> >diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> >index 4422da939937..c7759564316d 100644
> >--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> >+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> >@@ -192,6 +192,77 @@ nfp_devlink_serial_get(struct devlink *devlink, u8 *buf, size_t buf_len,
> > 	return 0;
> > }
> > 
> >+static const struct nfp_devlink_versions_simple {
> >+	const char *key;
> >+	const char *hwinfo;
> >+} nfp_devlink_versions_hwinfo[] = {
> >+	{ "board.model",	"assembly.model", },
> >+	{ "board.partno",	"assembly.partno", },
> >+	{ "board.revision",	"assembly.revision", },
> >+	{ "board.vendor",	"assembly.vendor", },  
> 
> That means that every driver would re-define these strings. Would it
> make sense to have them (at least some of them) defined in a generic
> way? Something in a sense of devlink params, where we have generic and
> driver-specific ones.

Ack, I will add those as defines in the devlink header plus some docs.
Let's see how it goes..

> >+};
> >+
> >+static int
> >+nfp_devlink_versions_get_hwinfo(struct sk_buff *skb, struct nfp_nsp *nsp,
> >+				struct netlink_ext_ack *extack)
> >+{
> >+	unsigned int i;
> >+	int attr;
> >+	int err;
> >+
> >+	attr = DEVLINK_ATTR_INFO_VERSIONS_FIXED;
> >+
> >+	for (i = 0; i < ARRAY_SIZE(nfp_devlink_versions_hwinfo); i++) {
> >+		const struct nfp_devlink_versions_simple *info;
> >+		char buf[256] = {};
> >+
> >+		info = &nfp_devlink_versions_hwinfo[i];
> >+		strcpy(buf, info->hwinfo);
> >+
> >+		err = nfp_nsp_hwinfo_lookup(nsp, buf, sizeof(buf));
> >+		if (err == -ENOENT)
> >+			continue;
> >+		if (err) {
> >+			NL_SET_ERR_MSG_MOD(extack,
> >+					   "error reading versions string from FW");
> >+			return err;
> >+		}
> >+
> >+		err = devlink_versions_report(skb, attr, info->key, buf);  
> 
> So this is always "DEVLINK_ATTR_INFO_VERSIONS_FIXED". I don't
> understand.

Tmp var makes it look more like the other function (from next patch)...
and it makes the line fit in 80 char ;)

Re: [RFC iproute2-next] devlink: add info subcommand

[Jakub Kicinski]

On Tue, 15 Jan 2019 19:05:01 +0100, Jiri Pirko wrote:
> > For versions there may be FW communication required, and reading stuff
> > out of flash.  I bit of overhead for users who just want the list of
> > devlink instances.
> >
> > Having in under dev but as a separate command seems quite nice indeed.
> > Especially given that there can only be a show subcommand..  So:
> >
> > For dump:
> > $ devlink dev info
> >
> > But for get:
> > $ devlink dev pci/0000:82:00.0 info
> >
> > or
> >
> > $ devlink dev info pci/0000:82:00.0
>
> This is aligned with the rest.

Cool, thanks for all the comments!

Re: [RFC net-next 0/6] devlink: add device (driver) information API

[Jonathan Lemon]

On 14 Jan 2019, at 16:50, Jakub Kicinski wrote:

> Hi!
>
> For quite some time now the ethtool -i API has been showing its age.
> The driver version field is generally considered obsolete these
> days, and driver authors are encouraged to report the kernel version.
> fw_version field does not suit modern needs with 31 characters being
> quite limiting on more complex systems.  There is also no distinction
> between the running and flashed versions of the firmware.
>
> Since the driver information pertains to the entire device, rather
> than a particular netdev, it seems wise to move it do devlink, at
> the same time fixing the aforementioned issues.
>
> The new API allows exposing the device serial number and versions
> of the components of the card - both hardware, firmware (running
> and flashed).  Driver authors can choose descriptive identifiers
> for the version fields.  There is a potential for defining common
> fields here, but given the general direction of the stack I don't
> think people would like that.
>
> Example:
> $ devlink  info show
> pci/0000:05:00.0:
>   serial_number: 00:15:4d:12:20:7e
>   versions:
>     fixed:
>       board.model carbon
>       board.partno AMDA0099-0001
>       board.revision 07
>       board.vendor SMA
>     running:
>       fw.mgmt: 010156.010156.010156
>       fw.cpld: 0x44
>       fw.app: sriov-2.1.16
>     stored:
>       fw.mgmt: 010158.010158.010158
>       fw.cpld: 0x44
>       fw.app: sriov-2.1.20

How about adding the driver name and version as well?
When connecting to an unknown system, "ethtool -i" is useful in
discovering what is actually running.
-- 
Jonathan

Re: [RFC net-next 1/6] devlink: add device information API

[Andrew Lunn]

> Ack, will do.  IMHO it's a bit of an overkill, one could "safely
> assume" serial number won't be longer than 32 bytes.. :)  But no
> problem, will do!

A git hash is 40 bytes.

  Andrew

Re: [RFC net-next 0/6] devlink: add device (driver) information API

[Jakub Kicinski]

On Tue, 15 Jan 2019 11:30:10 -0800, Jonathan Lemon wrote:
> On 14 Jan 2019, at 16:50, Jakub Kicinski wrote:
> 
> > Hi!
> >
> > For quite some time now the ethtool -i API has been showing its age.
> > The driver version field is generally considered obsolete these
> > days, and driver authors are encouraged to report the kernel version.
> > fw_version field does not suit modern needs with 31 characters being
> > quite limiting on more complex systems.  There is also no distinction
> > between the running and flashed versions of the firmware.
> >
> > Since the driver information pertains to the entire device, rather
> > than a particular netdev, it seems wise to move it do devlink, at
> > the same time fixing the aforementioned issues.
> >
> > The new API allows exposing the device serial number and versions
> > of the components of the card - both hardware, firmware (running
> > and flashed).  Driver authors can choose descriptive identifiers
> > for the version fields.  There is a potential for defining common
> > fields here, but given the general direction of the stack I don't
> > think people would like that.
> >
> > Example:
> > $ devlink  info show
> > pci/0000:05:00.0:
> >   serial_number: 00:15:4d:12:20:7e
> >   versions:
> >     fixed:
> >       board.model carbon
> >       board.partno AMDA0099-0001
> >       board.revision 07
> >       board.vendor SMA
> >     running:
> >       fw.mgmt: 010156.010156.010156
> >       fw.cpld: 0x44
> >       fw.app: sriov-2.1.16
> >     stored:
> >       fw.mgmt: 010158.010158.010158
> >       fw.cpld: 0x44
> >       fw.app: sriov-2.1.20  
> 
> How about adding the driver name and version as well?
> When connecting to an unknown system, "ethtool -i" is useful in
> discovering what is actually running.

I'm happy to add the driver name, I'd, however, rather steer clear of
the driver version.  In most scenarios kernel version is most reliable.
It's mostly out-of-tree drivers that need the driver version.

Re: [RFC net-next 1/6] devlink: add device information API

[Jakub Kicinski]

On Tue, 15 Jan 2019 21:00:12 +0100, Andrew Lunn wrote:
> > Ack, will do.  IMHO it's a bit of an overkill, one could "safely
> > assume" serial number won't be longer than 32 bytes.. :)  But no
> > problem, will do!  
> 
> A git hash is 40 bytes.

It's 20 bytes, this is in binary form.  Plus serial numbers are unique
ID of the device, not sure how hash would work there ;)

But I take your point, I'll redo this bit! :)

Re: [RFC net-next 0/6] devlink: add device (driver) information API

[Jiri Pirko]

Tue, Jan 15, 2019 at 10:06:42PM CET, jakub.kicinski@netronome.com wrote:
>On Tue, 15 Jan 2019 11:30:10 -0800, Jonathan Lemon wrote:
>> On 14 Jan 2019, at 16:50, Jakub Kicinski wrote:
>> 
>> > Hi!
>> >
>> > For quite some time now the ethtool -i API has been showing its age.
>> > The driver version field is generally considered obsolete these
>> > days, and driver authors are encouraged to report the kernel version.
>> > fw_version field does not suit modern needs with 31 characters being
>> > quite limiting on more complex systems.  There is also no distinction
>> > between the running and flashed versions of the firmware.
>> >
>> > Since the driver information pertains to the entire device, rather
>> > than a particular netdev, it seems wise to move it do devlink, at
>> > the same time fixing the aforementioned issues.
>> >
>> > The new API allows exposing the device serial number and versions
>> > of the components of the card - both hardware, firmware (running
>> > and flashed).  Driver authors can choose descriptive identifiers
>> > for the version fields.  There is a potential for defining common
>> > fields here, but given the general direction of the stack I don't
>> > think people would like that.
>> >
>> > Example:
>> > $ devlink  info show
>> > pci/0000:05:00.0:
>> >   serial_number: 00:15:4d:12:20:7e
>> >   versions:
>> >     fixed:
>> >       board.model carbon
>> >       board.partno AMDA0099-0001
>> >       board.revision 07
>> >       board.vendor SMA
>> >     running:
>> >       fw.mgmt: 010156.010156.010156
>> >       fw.cpld: 0x44
>> >       fw.app: sriov-2.1.16
>> >     stored:
>> >       fw.mgmt: 010158.010158.010158
>> >       fw.cpld: 0x44
>> >       fw.app: sriov-2.1.20  
>> 
>> How about adding the driver name and version as well?
>> When connecting to an unknown system, "ethtool -i" is useful in
>> discovering what is actually running.
>
>I'm happy to add the driver name, I'd, however, rather steer clear of

Agreed. I intended to do that for initial devlink implementation.


>the driver version.  In most scenarios kernel version is most reliable.
>It's mostly out-of-tree drivers that need the driver version.

Yep. The "driver version" really makes no sense.

Re: [RFC net-next 0/6] devlink: add device (driver) information API

[Jonathan Lemon]

On 15 Jan 2019, at 13:06, Jakub Kicinski wrote:

> On Tue, 15 Jan 2019 11:30:10 -0800, Jonathan Lemon wrote:
>> On 14 Jan 2019, at 16:50, Jakub Kicinski wrote:
>>
>>> Hi!
>>>
>>> For quite some time now the ethtool -i API has been showing its age.
>>> The driver version field is generally considered obsolete these
>>> days, and driver authors are encouraged to report the kernel version.
>>> fw_version field does not suit modern needs with 31 characters being
>>> quite limiting on more complex systems.  There is also no distinction
>>> between the running and flashed versions of the firmware.
>>>
>>> Since the driver information pertains to the entire device, rather
>>> than a particular netdev, it seems wise to move it do devlink, at
>>> the same time fixing the aforementioned issues.
>>>
>>> The new API allows exposing the device serial number and versions
>>> of the components of the card - both hardware, firmware (running
>>> and flashed).  Driver authors can choose descriptive identifiers
>>> for the version fields.  There is a potential for defining common
>>> fields here, but given the general direction of the stack I don't
>>> think people would like that.
>>>
>>> Example:
>>> $ devlink  info show
>>> pci/0000:05:00.0:
>>>   serial_number: 00:15:4d:12:20:7e
>>>   versions:
>>>     fixed:
>>>       board.model carbon
>>>       board.partno AMDA0099-0001
>>>       board.revision 07
>>>       board.vendor SMA
>>>     running:
>>>       fw.mgmt: 010156.010156.010156
>>>       fw.cpld: 0x44
>>>       fw.app: sriov-2.1.16
>>>     stored:
>>>       fw.mgmt: 010158.010158.010158
>>>       fw.cpld: 0x44
>>>       fw.app: sriov-2.1.20
>>
>> How about adding the driver name and version as well?
>> When connecting to an unknown system, "ethtool -i" is useful in
>> discovering what is actually running.
>
> I'm happy to add the driver name, I'd, however, rather steer clear of
> the driver version.  In most scenarios kernel version is most reliable.
> It's mostly out-of-tree drivers that need the driver version.

The driver name + kernel version will work just fine as well.

Thanks!
-- 
Jonathan