[RFC 0/3] devlink: add the ability to update device flash

[RFC 0/3] devlink: add the ability to update device flash

[Jakub Kicinski]

Hi!

This series is the second step to allow trouble shooting and recovering
devices in bad state without the use of netdevs as handles.  We can
already query FW versions over devlink, now we add the ability to update
the FW.  This will allow drivers to implement some from of "limp-mode"
where the device can't really be used for networking and hence has no
netdev, but we can interrogate it over devlink and fix the broken FW.

Small but nice advantage of devlink is that it only holds the devlink
instance lock during flashing, unlike ethtool which holds rtnl_lock().

Sending as RFC due to impending conflicts.

Jakub Kicinski (3):
  devlink: add flash update command
  ethtool: add compat for flash update
  nfp: devlink: allow flashing the device via devlink

 .../net/ethernet/netronome/nfp/nfp_devlink.c  | 47 +++++++++++++-
 include/net/devlink.h                         | 11 ++++
 include/uapi/linux/devlink.h                  |  6 ++
 net/core/devlink.c                            | 61 +++++++++++++++++++
 net/core/ethtool.c                            | 12 +++-
 5 files changed, 133 insertions(+), 4 deletions(-)

-- 
2.19.2

[RFC 1/3] devlink: add flash update command

[Jakub Kicinski]

Add devlink flash update command. Advanced NICs have firmware
stored in flash and often cryptographically secured. Updating
that flash is handled by management firmware. Ethtool has a
flash update command which served us well, however, it has two
shortcomings:
 - it takes rtnl_lock unnecessarily - really flash update has
   nothing to do with networking, so using a networking device
   as a handle is suboptimal, which leads us to the second one:
 - it requires a functioning netdev - in case device enters an
   error state and can't spawn a netdev (e.g. communication
   with the device fails) there is no netdev to use as a handle
   for flashing.

Devlink already has the ability to report the firmware versions,
now with the ability to update the firmware/flash we will be
able to recover devices in bad state.

To enable easy interoperability with ethtool add the target
partition ID. We may or may not add a different method of
identification, but there is no such immediate need.

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

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 07660fe4c0e3..55b3478b1291 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -529,6 +529,8 @@ struct devlink_ops {
 				      struct netlink_ext_ack *extack);
 	int (*info_get)(struct devlink *devlink, struct devlink_info_req *req,
 			struct netlink_ext_ack *extack);
+	int (*flash_update)(struct devlink *devlink, const char *path,
+			    u32 target, 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 72d9f7c89190..f4417283fd1b 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -103,6 +103,8 @@ enum devlink_command {
 	DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
 	DEVLINK_CMD_HEALTH_REPORTER_DUMP_CLEAR,
 
+	DEVLINK_CMD_FLASH_UPDATE,
+
 	/* add new commands above here */
 	__DEVLINK_CMD_MAX,
 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
@@ -326,6 +328,10 @@ enum devlink_attr {
 	DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS,		/* u64 */
 	DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD,	/* u64 */
 	DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER,	/* u8 */
+
+	DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME,	/* string */
+	DEVLINK_ATTR_FLASH_UPDATE_TARGET_ID,	/* u32 */
+
 	/* 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 46c468a1f3dc..a4b5e194e33e 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2660,6 +2660,27 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
 	return devlink->ops->reload(devlink, info->extack);
 }
 
+static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
+				       struct genl_info *info)
+{
+	struct devlink *devlink = info->user_ptr[0];
+	const char *file_name;
+	u32 target = 0;
+
+	if (!devlink->ops->flash_update)
+		return -EOPNOTSUPP;
+
+	if (!info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME])
+		return -EINVAL;
+	file_name = nla_data(info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME]);
+
+	if (info->attrs[DEVLINK_ATTR_FLASH_UPDATE_TARGET_ID])
+		target = nla_get_u32(info->attrs[DEVLINK_ATTR_FLASH_UPDATE_TARGET_ID]);
+
+	return devlink->ops->flash_update(devlink, file_name, target,
+					  info->extack);
+}
+
 static const struct devlink_param devlink_param_generic[] = {
 	{
 		.id = DEVLINK_PARAM_GENERIC_ID_INT_ERR_RESET,
@@ -4876,6 +4897,8 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_HEALTH_REPORTER_NAME] = { .type = NLA_NUL_STRING },
 	[DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD] = { .type = NLA_U64 },
 	[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME] = { .type = NLA_NUL_STRING },
+	[DEVLINK_ATTR_FLASH_UPDATE_TARGET_ID] = { .type = NLA_U32 },
 };
 
 static const struct genl_ops devlink_nl_ops[] = {
@@ -5164,6 +5187,13 @@ static const struct genl_ops devlink_nl_ops[] = {
 		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK |
 				  DEVLINK_NL_FLAG_NO_LOCK,
 	},
+	{
+		.cmd = DEVLINK_CMD_FLASH_UPDATE,
+		.doit = devlink_nl_cmd_flash_update,
+		.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.19.2

[RFC 2/3] ethtool: add compat for flash update

[Jakub Kicinski]

If driver does not support ethtool flash update operation
call into devlink.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 include/net/devlink.h |  9 +++++++++
 net/core/devlink.c    | 31 +++++++++++++++++++++++++++++++
 net/core/ethtool.c    | 12 +++++++++---
 3 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 55b3478b1291..8fdadd0a43ce 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1202,11 +1202,20 @@ devlink_health_report(struct devlink_health_reporter *reporter,
 #if IS_REACHABLE(CONFIG_NET_DEVLINK)
 void devlink_compat_running_version(struct net_device *dev,
 				    char *buf, size_t len);
+int devlink_compat_flash_update(struct net_device *dev, const char *file_name,
+				u32 target);
 #else
 static inline void
 devlink_compat_running_version(struct net_device *dev, char *buf, size_t len)
 {
 }
+
+static inline int
+devlink_compat_flash_update(struct net_device *dev, const char *file_name,
+			    u32 target)
+{
+	return -EOPNOTSUPP;
+}
 #endif
 
 #endif /* _NET_DEVLINK_H_ */
diff --git a/net/core/devlink.c b/net/core/devlink.c
index a4b5e194e33e..fb1b0982281b 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6435,6 +6435,37 @@ void devlink_compat_running_version(struct net_device *dev,
 	mutex_unlock(&devlink_mutex);
 }
 
+int devlink_compat_flash_update(struct net_device *dev, const char *file_name,
+				u32 target)
+{
+	struct devlink_port *devlink_port;
+	struct devlink *devlink;
+
+	mutex_lock(&devlink_mutex);
+	list_for_each_entry(devlink, &devlink_list, list) {
+		mutex_lock(&devlink->lock);
+		list_for_each_entry(devlink_port, &devlink->port_list, list) {
+			int ret = -EOPNOTSUPP;
+
+			if (devlink_port->type != DEVLINK_PORT_TYPE_ETH ||
+			    devlink_port->type_dev != dev)
+				continue;
+
+			mutex_unlock(&devlink_mutex);
+			if (devlink->ops->flash_update)
+				ret = devlink->ops->flash_update(devlink,
+								 file_name,
+								 target, NULL);
+			mutex_unlock(&devlink->lock);
+			return ret;
+		}
+		mutex_unlock(&devlink->lock);
+	}
+	mutex_unlock(&devlink_mutex);
+
+	return -EOPNOTSUPP;
+}
+
 static int __init devlink_module_init(void)
 {
 	return genl_register_family(&devlink_nl_family);
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index d2c47cdf25da..389782ccd4c5 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -2038,11 +2038,17 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev,
 
 	if (copy_from_user(&efl, useraddr, sizeof(efl)))
 		return -EFAULT;
+	efl.data[ETHTOOL_FLASH_MAX_FILENAME - 1] = 0;
 
-	if (!dev->ethtool_ops->flash_device)
-		return -EOPNOTSUPP;
+	if (!dev->ethtool_ops->flash_device) {
+		int ret;
 
-	efl.data[ETHTOOL_FLASH_MAX_FILENAME - 1] = 0;
+		rtnl_unlock();
+		ret = devlink_compat_flash_update(dev, efl.data, efl.region);
+		rtnl_lock();
+
+		return ret;
+	}
 
 	return dev->ethtool_ops->flash_device(dev, &efl);
 }
-- 
2.19.2

[RFC 3/3] nfp: devlink: allow flashing the device via devlink

[Jakub Kicinski]

Devlink now allows updating device flash.  Implement this
callback.

Compared to ethtool update we no longer have to release
the networking locks - devlink doesn't take them.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 .../net/ethernet/netronome/nfp/nfp_devlink.c  |  9 ++++
 drivers/net/ethernet/netronome/nfp/nfp_main.c | 44 +++++++++++++++++++
 drivers/net/ethernet/netronome/nfp/nfp_main.h |  2 +
 .../ethernet/netronome/nfp/nfp_net_ethtool.c  | 39 ++--------------
 4 files changed, 59 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
index 080a301f379e..ee45a6f9030a 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
@@ -330,6 +330,14 @@ nfp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
 	return err;
 }
 
+static int
+nfp_devlink_flash_update(struct devlink *devlink, const char *path,
+			 u32 target, struct netlink_ext_ack *extack)
+{
+	return nfp_flash_update_common(devlink_priv(devlink), path, target,
+				       extack);
+}
+
 const struct devlink_ops nfp_devlink_ops = {
 	.port_split		= nfp_devlink_port_split,
 	.port_unsplit		= nfp_devlink_port_unsplit,
@@ -338,6 +346,7 @@ const struct devlink_ops nfp_devlink_ops = {
 	.eswitch_mode_get	= nfp_devlink_eswitch_mode_get,
 	.eswitch_mode_set	= nfp_devlink_eswitch_mode_set,
 	.info_get		= nfp_devlink_info_get,
+	.flash_update		= nfp_devlink_flash_update,
 };
 
 int nfp_devlink_port_register(struct nfp_app *app, struct nfp_port *port)
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.c b/drivers/net/ethernet/netronome/nfp/nfp_main.c
index 6c10e8d119e4..3f55575c2929 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_main.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_main.c
@@ -300,6 +300,50 @@ static int nfp_pcie_sriov_configure(struct pci_dev *pdev, int num_vfs)
 		return nfp_pcie_sriov_enable(pdev, num_vfs);
 }
 
+int nfp_flash_update_common(struct nfp_pf *pf, const char *path,
+			    u32 target, struct netlink_ext_ack *extack)
+{
+	struct device *dev = &pf->pdev->dev;
+	const struct firmware *fw;
+	struct nfp_nsp *nsp;
+	int err;
+
+	if (target != ETHTOOL_FLASH_ALL_REGIONS)
+		return -EOPNOTSUPP;
+
+	nsp = nfp_nsp_open(pf->cpp);
+	if (IS_ERR(nsp)) {
+		err = PTR_ERR(nsp);
+		if (extack)
+			NL_SET_ERR_MSG_MOD(extack, "can't access NSP");
+		else
+			dev_err(dev, "Failed to access the NSP: %d\n", err);
+		return err;
+	}
+
+	err = request_firmware_direct(&fw, path, dev);
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "unable to read flash file from disk");
+		goto exit_close_nsp;
+	}
+
+	dev_info(dev, "Please be patient while writing flash image: %s\n",
+		 path);
+
+	err = nfp_nsp_write_flash(nsp, fw);
+	if (err < 0)
+		goto exit_release_fw;
+	dev_info(dev, "Finished writing flash image\n");
+	err = 0;
+
+exit_release_fw:
+	release_firmware(fw);
+exit_close_nsp:
+	nfp_nsp_close(nsp);
+	return err;
+}
+
 static const struct firmware *
 nfp_net_fw_request(struct pci_dev *pdev, struct nfp_pf *pf, const char *name)
 {
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.h b/drivers/net/ethernet/netronome/nfp/nfp_main.h
index a3613a2e0aa5..6e4b509017c1 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_main.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_main.h
@@ -164,6 +164,8 @@ nfp_pf_map_rtsym(struct nfp_pf *pf, const char *name, const char *sym_fmt,
 		 unsigned int min_size, struct nfp_cpp_area **area);
 int nfp_mbox_cmd(struct nfp_pf *pf, u32 cmd, void *in_data, u64 in_length,
 		 void *out_data, u64 out_length);
+int nfp_flash_update_common(struct nfp_pf *pf, const char *path,
+			    u32 target, struct netlink_ext_ack *extack);
 
 enum nfp_dump_diag {
 	NFP_DUMP_NSP_DIAG = 0,
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index cb9c512abc76..244b60f406c2 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
@@ -1237,52 +1237,21 @@ static int nfp_net_set_channels(struct net_device *netdev,
 static int
 nfp_net_flash_device(struct net_device *netdev, struct ethtool_flash *flash)
 {
-	const struct firmware *fw;
 	struct nfp_app *app;
-	struct nfp_nsp *nsp;
-	struct device *dev;
-	int err;
-
-	if (flash->region != ETHTOOL_FLASH_ALL_REGIONS)
-		return -EOPNOTSUPP;
+	int ret;
 
 	app = nfp_app_from_netdev(netdev);
 	if (!app)
 		return -EOPNOTSUPP;
 
-	dev = &app->pdev->dev;
-
-	nsp = nfp_nsp_open(app->cpp);
-	if (IS_ERR(nsp)) {
-		err = PTR_ERR(nsp);
-		dev_err(dev, "Failed to access the NSP: %d\n", err);
-		return err;
-	}
-
-	err = request_firmware_direct(&fw, flash->data, dev);
-	if (err)
-		goto exit_close_nsp;
-
-	dev_info(dev, "Please be patient while writing flash image: %s\n",
-		 flash->data);
 	dev_hold(netdev);
 	rtnl_unlock();
-
-	err = nfp_nsp_write_flash(nsp, fw);
-	if (err < 0) {
-		dev_err(dev, "Flash write failed: %d\n", err);
-		goto exit_rtnl_lock;
-	}
-	dev_info(dev, "Finished writing flash image\n");
-
-exit_rtnl_lock:
+	ret = nfp_flash_update_common(app->pf, flash->data, flash->region,
+				      NULL);
 	rtnl_lock();
 	dev_put(netdev);
-	release_firmware(fw);
 
-exit_close_nsp:
-	nfp_nsp_close(nsp);
-	return err;
+	return ret;
 }
 
 static const struct ethtool_ops nfp_net_ethtool_ops = {
-- 
2.19.2

[RFC iproute2] devlink: add support for updating device flash

[Jakub Kicinski]

Add new command for updating flash of devices via devlink API.
Example:

$ cp flash-boot.bin /lib/firmware/
$ devlink dev flash pci/0000:05:00.0 file flash-boot.bin

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 devlink/devlink.c      | 54 ++++++++++++++++++++++++++++++++++++++++++
 man/man8/devlink-dev.8 | 29 +++++++++++++++++++++++
 2 files changed, 83 insertions(+)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index d823512a4030..dd5f153eddc6 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -199,6 +199,8 @@ 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_FLASH_FILE_NAME	BIT(25)
+#define DL_OPT_FLASH_TARGET_ID	BIT(26)
 
 struct dl_opts {
 	uint32_t present; /* flags of present items */
@@ -230,6 +232,8 @@ struct dl_opts {
 	uint32_t region_snapshot_id;
 	uint64_t region_address;
 	uint64_t region_length;
+	const char *flash_file_name;
+	uint32_t flash_target_id;
 };
 
 struct dl {
@@ -1185,6 +1189,20 @@ 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, "file") &&
+			   (o_all & DL_OPT_FLASH_FILE_NAME)) {
+			dl_arg_inc(dl);
+			err = dl_argv_str(dl, &opts->flash_file_name);
+			if (err)
+				return err;
+			o_found |= DL_OPT_FLASH_FILE_NAME;
+		} else if (dl_argv_match(dl, "target") &&
+			   (o_all & DL_OPT_FLASH_TARGET_ID)) {
+			dl_arg_inc(dl);
+			err = dl_argv_uint32_t(dl, &opts->flash_target_id);
+			if (err)
+				return err;
+			o_found |= DL_OPT_FLASH_TARGET_ID;
 		} else {
 			pr_err("Unknown option \"%s\"\n", dl_argv(dl));
 			return -EINVAL;
@@ -1389,6 +1407,12 @@ static void dl_opts_put(struct nlmsghdr *nlh, struct dl *dl)
 	if (opts->present & DL_OPT_REGION_LENGTH)
 		mnl_attr_put_u64(nlh, DEVLINK_ATTR_REGION_CHUNK_LEN,
 				 opts->region_length);
+	if (opts->present & DL_OPT_FLASH_FILE_NAME)
+		mnl_attr_put_strz(nlh, DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME,
+				  opts->flash_file_name);
+	if (opts->present & DL_OPT_FLASH_TARGET_ID)
+		mnl_attr_put_u32(nlh, DEVLINK_ATTR_FLASH_UPDATE_TARGET_ID,
+				 opts->flash_target_id);
 }
 
 static int dl_argv_parse_put(struct nlmsghdr *nlh, struct dl *dl,
@@ -1451,6 +1475,7 @@ static void cmd_dev_help(void)
 	pr_err("       devlink dev param show [DEV name PARAMETER]\n");
 	pr_err("       devlink dev reload DEV\n");
 	pr_err("       devlink dev info [ DEV ]\n");
+	pr_err("       devlink dev flash DEV file PATH [ target ID ]\n");
 }
 
 static bool cmp_arr_last_handle(struct dl *dl, const char *bus_name,
@@ -2583,6 +2608,32 @@ static int cmd_dev_info(struct dl *dl)
 	return err;
 }
 
+static void cmd_dev_flash_help(void)
+{
+	pr_err("Usage: devlink dev flash DEV file PATH [ target ID ]\n");
+}
+
+static int cmd_dev_flash(struct dl *dl)
+{
+	struct nlmsghdr *nlh;
+	int err;
+
+	if (dl_argv_match(dl, "help") || dl_no_arg(dl)) {
+		cmd_dev_flash_help();
+		return 0;
+	}
+
+	nlh = mnlg_msg_prepare(dl->nlg, DEVLINK_CMD_FLASH_UPDATE,
+			       NLM_F_REQUEST | NLM_F_ACK);
+
+	err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE | DL_OPT_FLASH_FILE_NAME,
+				DL_OPT_FLASH_TARGET_ID);
+	if (err)
+		return err;
+
+	return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL);
+}
+
 static int cmd_dev(struct dl *dl)
 {
 	if (dl_argv_match(dl, "help")) {
@@ -2604,6 +2655,9 @@ static int cmd_dev(struct dl *dl)
 	} else if (dl_argv_match(dl, "info")) {
 		dl_arg_inc(dl);
 		return cmd_dev_info(dl);
+	} else if (dl_argv_match(dl, "flash")) {
+		dl_arg_inc(dl);
+		return cmd_dev_flash(dl);
 	}
 	pr_err("Command \"%s\" not found\n", dl_argv(dl));
 	return -ENOENT;
diff --git a/man/man8/devlink-dev.8 b/man/man8/devlink-dev.8
index 47838371fecd..dda35fb09ee0 100644
--- a/man/man8/devlink-dev.8
+++ b/man/man8/devlink-dev.8
@@ -69,6 +69,16 @@ devlink-dev \- devlink device configuration
 .IR DEV
 .RI "]"
 
+.ti -8
+.BR "devlink dev flash"
+.IR DEV
+.BR file
+.IR PATH
+.RI "["
+.BR target
+.IR ID
+.RI "]"
+
 .SH "DESCRIPTION"
 .SS devlink dev show - display devlink device attributes
 
@@ -177,6 +187,25 @@ versions may differ after flash has been updated, but before reboot.
 - specifies the devlink device to show.
 If this argument is omitted all devices are listed.
 
+.SS devlink dev flash - write device's non-volatile memory.
+
+.PP
+.I "DEV"
+- specifies the devlink device to write to.
+
+.BR file
+.I PATH
+- Path to the file which will be written into device's flash. The path needs
+to be relative to one of the directories searched by the kernel firmware loaded,
+such as /lib/firmware.
+
+.BR target
+.I ID
+- If device stores multiple firmware images in non-volatile memory, this
+parameter may be used to indicate which firmware image should be written.
+The default value of 0 means that all regions should be updated.
+Interpretation of other values is driver-dependent.
+
 .SH "EXAMPLES"
 .PP
 devlink dev show
-- 
2.19.2

Re: [RFC 1/3] devlink: add flash update command

[Jiri Pirko]

Mon, Feb 11, 2019 at 07:59:20AM CET, jakub.kicinski@netronome.com wrote:
>Add devlink flash update command. Advanced NICs have firmware
>stored in flash and often cryptographically secured. Updating
>that flash is handled by management firmware. Ethtool has a
>flash update command which served us well, however, it has two
>shortcomings:
> - it takes rtnl_lock unnecessarily - really flash update has
>   nothing to do with networking, so using a networking device
>   as a handle is suboptimal, which leads us to the second one:
> - it requires a functioning netdev - in case device enters an
>   error state and can't spawn a netdev (e.g. communication
>   with the device fails) there is no netdev to use as a handle
>   for flashing.
>
>Devlink already has the ability to report the firmware versions,
>now with the ability to update the firmware/flash we will be
>able to recover devices in bad state.
>
>To enable easy interoperability with ethtool add the target
>partition ID. We may or may not add a different method of
>identification, but there is no such immediate need.
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>---
> include/net/devlink.h        |  2 ++
> include/uapi/linux/devlink.h |  6 ++++++
> net/core/devlink.c           | 30 ++++++++++++++++++++++++++++++
> 3 files changed, 38 insertions(+)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 07660fe4c0e3..55b3478b1291 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -529,6 +529,8 @@ struct devlink_ops {
> 				      struct netlink_ext_ack *extack);
> 	int (*info_get)(struct devlink *devlink, struct devlink_info_req *req,
> 			struct netlink_ext_ack *extack);
>+	int (*flash_update)(struct devlink *devlink, const char *path,
>+			    u32 target, 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 72d9f7c89190..f4417283fd1b 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -103,6 +103,8 @@ enum devlink_command {
> 	DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
> 	DEVLINK_CMD_HEALTH_REPORTER_DUMP_CLEAR,
> 
>+	DEVLINK_CMD_FLASH_UPDATE,
>+
> 	/* add new commands above here */
> 	__DEVLINK_CMD_MAX,
> 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
>@@ -326,6 +328,10 @@ enum devlink_attr {
> 	DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS,		/* u64 */
> 	DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD,	/* u64 */
> 	DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER,	/* u8 */
>+
>+	DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME,	/* string */
>+	DEVLINK_ATTR_FLASH_UPDATE_TARGET_ID,	/* u32 */

Do we need to carry this on? I mean, the ef->region is only checked in 4
drivers against ETHTOOL_FLASH_ALL_REGIONS, which is default.
There is this bnxt driver which is the only one working with this value.
I think that for compat, there should be some id-region mapping
provided by driver which the compat layer would use to translate.

I also think that this should be in sync with what is returned in
DEVLINK_ATTR_INFO_VERSION_NAME.

For example:
$ devlink dev info pci/0000:82:00.0
pci/0000:82:00.0:
  driver nfp
  serial_number 16240145
  versions:
      fixed:
        board.id AMDA0081-0001
        board.rev 15
        board.vendor SMA
        board.model hydrogen
      running:
        fw.mgmt 010181.010181.0101d4
        fw.cpld 0x1030000
        fw.app abm-d372b6
        fw.undi 0.0.2
        chip.init AMDA-0081-0001  20160318164536
      stored:
        fw.mgmt 010181.010181.0101d4
        fw.app abm-d372b6
        fw.undi 0.0.2
        chip.init AMDA-0081-0001  20160318164536

Now user should be able to use one of the identifiers to flash relevant
fw, like:

devlink dev flash pci/0000:82:00.0 XXX fw.mgmt file flash-boot.bin

I'm not sure about the name of "xxx" attribute. Maybe "id":

devlink dev flash pci/0000:82:00.0 id fw.mgmt file flash-boot.bin
devlink dev flash pci/0000:82:00.0 id fw.cpld file some-other.bin



[...]

Re: [oss-drivers] Re: [RFC 1/3] devlink: add flash update command

[Jakub Kicinski]

On Mon, 11 Feb 2019 17:45:13 +0100, Jiri Pirko wrote:
> Mon, Feb 11, 2019 at 07:59:20AM CET, jakub.kicinski@netronome.com wrote:
> >Add devlink flash update command. Advanced NICs have firmware
> >stored in flash and often cryptographically secured. Updating
> >that flash is handled by management firmware. Ethtool has a
> >flash update command which served us well, however, it has two
> >shortcomings:
> > - it takes rtnl_lock unnecessarily - really flash update has
> >   nothing to do with networking, so using a networking device
> >   as a handle is suboptimal, which leads us to the second one:
> > - it requires a functioning netdev - in case device enters an
> >   error state and can't spawn a netdev (e.g. communication
> >   with the device fails) there is no netdev to use as a handle
> >   for flashing.
> >
> >Devlink already has the ability to report the firmware versions,
> >now with the ability to update the firmware/flash we will be
> >able to recover devices in bad state.
> >
> >To enable easy interoperability with ethtool add the target
> >partition ID. We may or may not add a different method of
> >identification, but there is no such immediate need.
> >
> >Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> >---
> > include/net/devlink.h        |  2 ++
> > include/uapi/linux/devlink.h |  6 ++++++
> > net/core/devlink.c           | 30 ++++++++++++++++++++++++++++++
> > 3 files changed, 38 insertions(+)
> >
> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >index 07660fe4c0e3..55b3478b1291 100644
> >--- a/include/net/devlink.h
> >+++ b/include/net/devlink.h
> >@@ -529,6 +529,8 @@ struct devlink_ops {
> > 				      struct netlink_ext_ack *extack);
> > 	int (*info_get)(struct devlink *devlink, struct devlink_info_req *req,
> > 			struct netlink_ext_ack *extack);
> >+	int (*flash_update)(struct devlink *devlink, const char *path,
> >+			    u32 target, 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 72d9f7c89190..f4417283fd1b 100644
> >--- a/include/uapi/linux/devlink.h
> >+++ b/include/uapi/linux/devlink.h
> >@@ -103,6 +103,8 @@ enum devlink_command {
> > 	DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
> > 	DEVLINK_CMD_HEALTH_REPORTER_DUMP_CLEAR,
> > 
> >+	DEVLINK_CMD_FLASH_UPDATE,
> >+
> > 	/* add new commands above here */
> > 	__DEVLINK_CMD_MAX,
> > 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
> >@@ -326,6 +328,10 @@ enum devlink_attr {
> > 	DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS,		/* u64 */
> > 	DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD,	/* u64 */
> > 	DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER,	/* u8 */
> >+
> >+	DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME,	/* string */
> >+	DEVLINK_ATTR_FLASH_UPDATE_TARGET_ID,	/* u32 */  
> 
> Do we need to carry this on? I mean, the ef->region is only checked in 4
> drivers against ETHTOOL_FLASH_ALL_REGIONS, which is default.
> There is this bnxt driver which is the only one working with this value.
> I think that for compat, there should be some id-region mapping
> provided by driver which the compat layer would use to translate.
> 
> I also think that this should be in sync with what is returned in
> DEVLINK_ATTR_INFO_VERSION_NAME.
> 
> For example:
> $ devlink dev info pci/0000:82:00.0
> pci/0000:82:00.0:
>   driver nfp
>   serial_number 16240145
>   versions:
>       fixed:
>         board.id AMDA0081-0001
>         board.rev 15
>         board.vendor SMA
>         board.model hydrogen
>       running:
>         fw.mgmt 010181.010181.0101d4
>         fw.cpld 0x1030000
>         fw.app abm-d372b6
>         fw.undi 0.0.2
>         chip.init AMDA-0081-0001  20160318164536
>       stored:
>         fw.mgmt 010181.010181.0101d4
>         fw.app abm-d372b6
>         fw.undi 0.0.2
>         chip.init AMDA-0081-0001  20160318164536
> 
> Now user should be able to use one of the identifiers to flash relevant
> fw, like:
> 
> devlink dev flash pci/0000:82:00.0 XXX fw.mgmt file flash-boot.bin
> 
> I'm not sure about the name of "xxx" attribute. Maybe "id":
> 
> devlink dev flash pci/0000:82:00.0 id fw.mgmt file flash-boot.bin
> devlink dev flash pci/0000:82:00.0 id fw.cpld file some-other.bin

Agreed, that looks good!  TBH in case of Netronome the binary
image contains an identifier so it will update the correct component
automatically.  That's why I say "no immediate need" :)  (How about
"component" instead of "id", BTW?)

I will drop the target ID, I just added it for full backward compat
with ethtool, but it may be confusing, given it would be mostly unused.
I'll drop it in non-RFC, do you want me to add the id/component or leave
it out for now?

Re: [oss-drivers] Re: [RFC 1/3] devlink: add flash update command

[Jiri Pirko]

Mon, Feb 11, 2019 at 08:25:30PM CET, jakub.kicinski@netronome.com wrote:
>On Mon, 11 Feb 2019 17:45:13 +0100, Jiri Pirko wrote:
>> Mon, Feb 11, 2019 at 07:59:20AM CET, jakub.kicinski@netronome.com wrote:
>> >Add devlink flash update command. Advanced NICs have firmware
>> >stored in flash and often cryptographically secured. Updating
>> >that flash is handled by management firmware. Ethtool has a
>> >flash update command which served us well, however, it has two
>> >shortcomings:
>> > - it takes rtnl_lock unnecessarily - really flash update has
>> >   nothing to do with networking, so using a networking device
>> >   as a handle is suboptimal, which leads us to the second one:
>> > - it requires a functioning netdev - in case device enters an
>> >   error state and can't spawn a netdev (e.g. communication
>> >   with the device fails) there is no netdev to use as a handle
>> >   for flashing.
>> >
>> >Devlink already has the ability to report the firmware versions,
>> >now with the ability to update the firmware/flash we will be
>> >able to recover devices in bad state.
>> >
>> >To enable easy interoperability with ethtool add the target
>> >partition ID. We may or may not add a different method of
>> >identification, but there is no such immediate need.
>> >
>> >Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>> >---
>> > include/net/devlink.h        |  2 ++
>> > include/uapi/linux/devlink.h |  6 ++++++
>> > net/core/devlink.c           | 30 ++++++++++++++++++++++++++++++
>> > 3 files changed, 38 insertions(+)
>> >
>> >diff --git a/include/net/devlink.h b/include/net/devlink.h
>> >index 07660fe4c0e3..55b3478b1291 100644
>> >--- a/include/net/devlink.h
>> >+++ b/include/net/devlink.h
>> >@@ -529,6 +529,8 @@ struct devlink_ops {
>> > 				      struct netlink_ext_ack *extack);
>> > 	int (*info_get)(struct devlink *devlink, struct devlink_info_req *req,
>> > 			struct netlink_ext_ack *extack);
>> >+	int (*flash_update)(struct devlink *devlink, const char *path,
>> >+			    u32 target, 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 72d9f7c89190..f4417283fd1b 100644
>> >--- a/include/uapi/linux/devlink.h
>> >+++ b/include/uapi/linux/devlink.h
>> >@@ -103,6 +103,8 @@ enum devlink_command {
>> > 	DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>> > 	DEVLINK_CMD_HEALTH_REPORTER_DUMP_CLEAR,
>> > 
>> >+	DEVLINK_CMD_FLASH_UPDATE,
>> >+
>> > 	/* add new commands above here */
>> > 	__DEVLINK_CMD_MAX,
>> > 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
>> >@@ -326,6 +328,10 @@ enum devlink_attr {
>> > 	DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS,		/* u64 */
>> > 	DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD,	/* u64 */
>> > 	DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER,	/* u8 */
>> >+
>> >+	DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME,	/* string */
>> >+	DEVLINK_ATTR_FLASH_UPDATE_TARGET_ID,	/* u32 */  
>> 
>> Do we need to carry this on? I mean, the ef->region is only checked in 4
>> drivers against ETHTOOL_FLASH_ALL_REGIONS, which is default.
>> There is this bnxt driver which is the only one working with this value.
>> I think that for compat, there should be some id-region mapping
>> provided by driver which the compat layer would use to translate.
>> 
>> I also think that this should be in sync with what is returned in
>> DEVLINK_ATTR_INFO_VERSION_NAME.
>> 
>> For example:
>> $ devlink dev info pci/0000:82:00.0
>> pci/0000:82:00.0:
>>   driver nfp
>>   serial_number 16240145
>>   versions:
>>       fixed:
>>         board.id AMDA0081-0001
>>         board.rev 15
>>         board.vendor SMA
>>         board.model hydrogen
>>       running:
>>         fw.mgmt 010181.010181.0101d4
>>         fw.cpld 0x1030000
>>         fw.app abm-d372b6
>>         fw.undi 0.0.2
>>         chip.init AMDA-0081-0001  20160318164536
>>       stored:
>>         fw.mgmt 010181.010181.0101d4
>>         fw.app abm-d372b6
>>         fw.undi 0.0.2
>>         chip.init AMDA-0081-0001  20160318164536
>> 
>> Now user should be able to use one of the identifiers to flash relevant
>> fw, like:
>> 
>> devlink dev flash pci/0000:82:00.0 XXX fw.mgmt file flash-boot.bin
>> 
>> I'm not sure about the name of "xxx" attribute. Maybe "id":
>> 
>> devlink dev flash pci/0000:82:00.0 id fw.mgmt file flash-boot.bin
>> devlink dev flash pci/0000:82:00.0 id fw.cpld file some-other.bin
>
>Agreed, that looks good!  TBH in case of Netronome the binary
>image contains an identifier so it will update the correct component

Okay. So in case the "component" attr is omitted, there would be some
flag passed to the driver so it would know that the file contains more
component binaries and has to do parsing itself/in-fw.


>automatically.  That's why I say "no immediate need" :)  (How about
>"component" instead of "id", BTW?)

Component is fine by me.


>
>I will drop the target ID, I just added it for full backward compat
>with ethtool, but it may be confusing, given it would be mostly unused.

Ok.


>I'll drop it in non-RFC, do you want me to add the id/component or leave
>it out for now?

I think it would be good to add it and have the api complete.

Re: [RFC iproute2] devlink: add support for updating device flash

[Stephen Hemminger]

On Sun, 10 Feb 2019 22:59:23 -0800
Jakub Kicinski <jakub.kicinski@netronome.com> wrote:

> Add new command for updating flash of devices via devlink API.
> Example:
> 
> $ cp flash-boot.bin /lib/firmware/
> $ devlink dev flash pci/0000:05:00.0 file flash-boot.bin
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

This is targeted at iproute2-next since it needs stuff from net-next.