[PATCH v4 net-next 0/5] ionic: add devlink dev flash support

[PATCH v4 net-next 0/5] ionic: add devlink dev flash support

[Shannon Nelson]

Add support for using devlink's dev flash facility to update the
firmware on an ionic device, and add a new timeout parameter to the
devlink flash netlink message.

For long-running flash commands, we add a timeout element to the dev
flash notify message in order for a userland utility to display a timeout
deadline to the user.  This allows the userland utility to display a
count down to the user when a firmware update action is otherwise going
to go for ahile without any updates.  An example use is added to the
netdevsim module.

The ionic driver uses this timeout element in its new flash function.
The driver uses a simple model of pushing the firmware file to the NIC,
asking the NIC to unpack and install the file into the device, and then
selecting it for the next boot.  If any of these steps fail, the whole
transaction is failed.  A couple of the steps can take a long time,
so we use the timeout status message rather than faking it with bogus
done/total messages.

The driver doesn't currently support doing these steps individually.
In the future we want to be able to list the FW that is installed and
selectable but we don't yet have the API to fully support that.

v4: Added a new devlink status notify message for showing timeout
    information, and modified the ionic fw update to use it for its long
    running firmware commands.

v3: Changed long dev_cmd timeout on status check calls to a loop around
    calls with a normal timeout, which allows for more intermediate log
    messaging when in a long wait, and for letting other threads run
    dev_cmds if waiting.

v2: Changed "Activate" to "Select" in status messages.

Shannon Nelson (5):
  devlink: add timeout information to status_notify
  devlink: collect flash notify params into a struct
  netdevsim: devlink flash timeout message
  ionic: update the fw update api
  ionic: add devlink firmware update

 drivers/net/ethernet/pensando/ionic/Makefile  |   2 +-
 .../ethernet/pensando/ionic/ionic_devlink.c   |  14 ++
 .../ethernet/pensando/ionic/ionic_devlink.h   |   3 +
 .../net/ethernet/pensando/ionic/ionic_fw.c    | 206 ++++++++++++++++++
 .../net/ethernet/pensando/ionic/ionic_if.h    |  33 ++-
 .../net/ethernet/pensando/ionic/ionic_main.c  |  27 ++-
 drivers/net/netdevsim/dev.c                   |   2 +
 include/net/devlink.h                         |  25 +++
 include/uapi/linux/devlink.h                  |   3 +
 net/core/devlink.c                            |  83 ++++---
 10 files changed, 350 insertions(+), 48 deletions(-)
 create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_fw.c

-- 
2.17.1

[PATCH v4 net-next 1/5] devlink: add timeout information to status_notify

[Shannon Nelson]

Add a timeout element to the DEVLINK_CMD_FLASH_UPDATE_STATUS
netlink message for use by a userland utility to show that
a particular firmware flash activity may take a long but
bounded time to finish.  Also add a handy helper for drivers
to make use of the new timeout value.

UI usage hints:
 - if non-zero, add timeout display to the end of the status line
 	[component] status_msg  ( Xm Ys : Am Bs )
     using the timeout value for Am Bs and updating the Xm Ys
     every second
 - if the timeout expires while awaiting the next update,
   display something like
 	[component] status_msg  ( timeout reached : Am Bs )
 - if new status notify messages are received, remove
   the timeout and start over

Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 include/net/devlink.h        |  4 ++++
 include/uapi/linux/devlink.h |  3 +++
 net/core/devlink.c           | 29 +++++++++++++++++++++++------
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index eaec0a8cc5ef..f206accf80ad 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1400,6 +1400,10 @@ void devlink_flash_update_status_notify(struct devlink *devlink,
 					const char *component,
 					unsigned long done,
 					unsigned long total);
+void devlink_flash_update_timeout_notify(struct devlink *devlink,
+					 const char *status_msg,
+					 const char *component,
+					 unsigned long timeout);
 
 int devlink_traps_register(struct devlink *devlink,
 			   const struct devlink_trap *traps,
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 40d35145c879..4a6e213cfa04 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -460,6 +460,9 @@ enum devlink_attr {
 
 	DEVLINK_ATTR_PORT_EXTERNAL,		/* u8 */
 	DEVLINK_ATTR_PORT_CONTROLLER_NUMBER,	/* u32 */
+
+	DEVLINK_ATTR_FLASH_UPDATE_STATUS_TIMEOUT,	/* u64 */
+
 	/* 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 19037f114307..01f855e53e06 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3024,7 +3024,9 @@ static int devlink_nl_flash_update_fill(struct sk_buff *msg,
 					enum devlink_command cmd,
 					const char *status_msg,
 					const char *component,
-					unsigned long done, unsigned long total)
+					unsigned long done,
+					unsigned long total,
+					unsigned long timeout)
 {
 	void *hdr;
 
@@ -3052,6 +3054,9 @@ static int devlink_nl_flash_update_fill(struct sk_buff *msg,
 	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TOTAL,
 			      total, DEVLINK_ATTR_PAD))
 		goto nla_put_failure;
+	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TIMEOUT,
+			      timeout, DEVLINK_ATTR_PAD))
+		goto nla_put_failure;
 
 out:
 	genlmsg_end(msg, hdr);
@@ -3067,7 +3072,8 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
 					  const char *status_msg,
 					  const char *component,
 					  unsigned long done,
-					  unsigned long total)
+					  unsigned long total,
+					  unsigned long timeout)
 {
 	struct sk_buff *msg;
 	int err;
@@ -3081,7 +3087,7 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
 		return;
 
 	err = devlink_nl_flash_update_fill(msg, devlink, cmd, status_msg,
-					   component, done, total);
+					   component, done, total, timeout);
 	if (err)
 		goto out_free_msg;
 
@@ -3097,7 +3103,7 @@ void devlink_flash_update_begin_notify(struct devlink *devlink)
 {
 	__devlink_flash_update_notify(devlink,
 				      DEVLINK_CMD_FLASH_UPDATE,
-				      NULL, NULL, 0, 0);
+				      NULL, NULL, 0, 0, 0);
 }
 EXPORT_SYMBOL_GPL(devlink_flash_update_begin_notify);
 
@@ -3105,7 +3111,7 @@ void devlink_flash_update_end_notify(struct devlink *devlink)
 {
 	__devlink_flash_update_notify(devlink,
 				      DEVLINK_CMD_FLASH_UPDATE_END,
-				      NULL, NULL, 0, 0);
+				      NULL, NULL, 0, 0, 0);
 }
 EXPORT_SYMBOL_GPL(devlink_flash_update_end_notify);
 
@@ -3117,10 +3123,21 @@ void devlink_flash_update_status_notify(struct devlink *devlink,
 {
 	__devlink_flash_update_notify(devlink,
 				      DEVLINK_CMD_FLASH_UPDATE_STATUS,
-				      status_msg, component, done, total);
+				      status_msg, component, done, total, 0);
 }
 EXPORT_SYMBOL_GPL(devlink_flash_update_status_notify);
 
+void devlink_flash_update_timeout_notify(struct devlink *devlink,
+					 const char *status_msg,
+					 const char *component,
+					 unsigned long timeout)
+{
+	__devlink_flash_update_notify(devlink,
+				      DEVLINK_CMD_FLASH_UPDATE_STATUS,
+				      status_msg, component, 0, 0, timeout);
+}
+EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify);
+
 static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
 				       struct genl_info *info)
 {
-- 
2.17.1

[PATCH v4 net-next 2/5] devlink: collect flash notify params into a struct

[Shannon Nelson]

The dev flash status notify function parameter lists are getting
rather long, so add a struct to be filled and passed rather than
continuously changing the function signatures.

Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 include/net/devlink.h | 21 ++++++++++++
 net/core/devlink.c    | 80 +++++++++++++++++++++++--------------------
 2 files changed, 63 insertions(+), 38 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index f206accf80ad..9ab2014885cb 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -391,6 +391,27 @@ struct devlink_param_gset_ctx {
 	enum devlink_param_cmode cmode;
 };
 
+/**
+ * struct devlink_flash_notify - devlink dev flash notify data
+ * @cmd: devlink notify command code
+ * @status_msg: current status string
+ * @component: firmware component being updated
+ * @done: amount of work completed of total amount
+ * @total: amount of work expected to be done
+ * @timeout: expected max timeout in seconds
+ *
+ * These are values to be given to userland to be displayed in order
+ * to show current activity in a firmware update process.
+ */
+struct devlink_flash_notify {
+	enum devlink_command cmd;
+	const char *status_msg;
+	const char *component;
+	unsigned long done;
+	unsigned long total;
+	unsigned long timeout;
+};
+
 /**
  * struct devlink_param - devlink configuration parameter data
  * @name: name of the parameter
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 01f855e53e06..816f27a18b16 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3021,41 +3021,36 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
 
 static int devlink_nl_flash_update_fill(struct sk_buff *msg,
 					struct devlink *devlink,
-					enum devlink_command cmd,
-					const char *status_msg,
-					const char *component,
-					unsigned long done,
-					unsigned long total,
-					unsigned long timeout)
+					struct devlink_flash_notify *params)
 {
 	void *hdr;
 
-	hdr = genlmsg_put(msg, 0, 0, &devlink_nl_family, 0, cmd);
+	hdr = genlmsg_put(msg, 0, 0, &devlink_nl_family, 0, params->cmd);
 	if (!hdr)
 		return -EMSGSIZE;
 
 	if (devlink_nl_put_handle(msg, devlink))
 		goto nla_put_failure;
 
-	if (cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS)
+	if (params->cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS)
 		goto out;
 
-	if (status_msg &&
+	if (params->status_msg &&
 	    nla_put_string(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_MSG,
-			   status_msg))
+			   params->status_msg))
 		goto nla_put_failure;
-	if (component &&
+	if (params->component &&
 	    nla_put_string(msg, DEVLINK_ATTR_FLASH_UPDATE_COMPONENT,
-			   component))
+			   params->component))
 		goto nla_put_failure;
 	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_DONE,
-			      done, DEVLINK_ATTR_PAD))
+			      params->done, DEVLINK_ATTR_PAD))
 		goto nla_put_failure;
 	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TOTAL,
-			      total, DEVLINK_ATTR_PAD))
+			      params->total, DEVLINK_ATTR_PAD))
 		goto nla_put_failure;
 	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TIMEOUT,
-			      timeout, DEVLINK_ATTR_PAD))
+			      params->timeout, DEVLINK_ATTR_PAD))
 		goto nla_put_failure;
 
 out:
@@ -3068,26 +3063,20 @@ static int devlink_nl_flash_update_fill(struct sk_buff *msg,
 }
 
 static void __devlink_flash_update_notify(struct devlink *devlink,
-					  enum devlink_command cmd,
-					  const char *status_msg,
-					  const char *component,
-					  unsigned long done,
-					  unsigned long total,
-					  unsigned long timeout)
+					  struct devlink_flash_notify *params)
 {
 	struct sk_buff *msg;
 	int err;
 
-	WARN_ON(cmd != DEVLINK_CMD_FLASH_UPDATE &&
-		cmd != DEVLINK_CMD_FLASH_UPDATE_END &&
-		cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS);
+	WARN_ON(params->cmd != DEVLINK_CMD_FLASH_UPDATE &&
+		params->cmd != DEVLINK_CMD_FLASH_UPDATE_END &&
+		params->cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS);
 
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg)
 		return;
 
-	err = devlink_nl_flash_update_fill(msg, devlink, cmd, status_msg,
-					   component, done, total, timeout);
+	err = devlink_nl_flash_update_fill(msg, devlink, params);
 	if (err)
 		goto out_free_msg;
 
@@ -3101,17 +3090,21 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
 
 void devlink_flash_update_begin_notify(struct devlink *devlink)
 {
-	__devlink_flash_update_notify(devlink,
-				      DEVLINK_CMD_FLASH_UPDATE,
-				      NULL, NULL, 0, 0, 0);
+	struct devlink_flash_notify params = {
+		.cmd = DEVLINK_CMD_FLASH_UPDATE,
+	};
+
+	__devlink_flash_update_notify(devlink, &params);
 }
 EXPORT_SYMBOL_GPL(devlink_flash_update_begin_notify);
 
 void devlink_flash_update_end_notify(struct devlink *devlink)
 {
-	__devlink_flash_update_notify(devlink,
-				      DEVLINK_CMD_FLASH_UPDATE_END,
-				      NULL, NULL, 0, 0, 0);
+	struct devlink_flash_notify params = {
+		.cmd = DEVLINK_CMD_FLASH_UPDATE_END,
+	};
+
+	__devlink_flash_update_notify(devlink, &params);
 }
 EXPORT_SYMBOL_GPL(devlink_flash_update_end_notify);
 
@@ -3121,9 +3114,15 @@ void devlink_flash_update_status_notify(struct devlink *devlink,
 					unsigned long done,
 					unsigned long total)
 {
-	__devlink_flash_update_notify(devlink,
-				      DEVLINK_CMD_FLASH_UPDATE_STATUS,
-				      status_msg, component, done, total, 0);
+	struct devlink_flash_notify params = {
+		.cmd = DEVLINK_CMD_FLASH_UPDATE_STATUS,
+		.status_msg = status_msg,
+		.component = component,
+		.done = done,
+		.total = total,
+	};
+
+	__devlink_flash_update_notify(devlink, &params);
 }
 EXPORT_SYMBOL_GPL(devlink_flash_update_status_notify);
 
@@ -3132,9 +3131,14 @@ void devlink_flash_update_timeout_notify(struct devlink *devlink,
 					 const char *component,
 					 unsigned long timeout)
 {
-	__devlink_flash_update_notify(devlink,
-				      DEVLINK_CMD_FLASH_UPDATE_STATUS,
-				      status_msg, component, 0, 0, timeout);
+	struct devlink_flash_notify params = {
+		.cmd = DEVLINK_CMD_FLASH_UPDATE_STATUS,
+		.status_msg = status_msg,
+		.component = component,
+		.timeout = timeout,
+	};
+
+	__devlink_flash_update_notify(devlink, &params);
 }
 EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify);
 
-- 
2.17.1

[PATCH v4 net-next 3/5] netdevsim: devlink flash timeout message

[Shannon Nelson]

Add a simple devlink flash timeout message to exercise
the message mechanism.

Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 drivers/net/netdevsim/dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 32f339fedb21..4123550e3f6e 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -768,6 +768,8 @@ static int nsim_dev_flash_update(struct devlink *devlink, const char *file_name,
 						   component,
 						   NSIM_DEV_FLASH_SIZE,
 						   NSIM_DEV_FLASH_SIZE);
+		devlink_flash_update_timeout_notify(devlink, "Flash timeout",
+						    component, 81);
 		devlink_flash_update_status_notify(devlink, "Flashing done",
 						   component, 0, 0);
 		devlink_flash_update_end_notify(devlink);
-- 
2.17.1

[PATCH v4 net-next 4/5] ionic: update the fw update api

[Shannon Nelson]

Add the rest of the firmware api bits needed to support the
driver running a firmware update.

Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 .../net/ethernet/pensando/ionic/ionic_if.h    | 33 ++++++++++++++-----
 .../net/ethernet/pensando/ionic/ionic_main.c  |  4 +++
 2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_if.h b/drivers/net/ethernet/pensando/ionic/ionic_if.h
index acc94b244cf3..5bb56a27a50d 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_if.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_if.h
@@ -63,8 +63,10 @@ enum ionic_cmd_opcode {
 	IONIC_CMD_QOS_RESET			= 245,
 
 	/* Firmware commands */
-	IONIC_CMD_FW_DOWNLOAD			= 254,
-	IONIC_CMD_FW_CONTROL			= 255,
+	IONIC_CMD_FW_DOWNLOAD                   = 252,
+	IONIC_CMD_FW_CONTROL                    = 253,
+	IONIC_CMD_FW_DOWNLOAD_V1		= 254,
+	IONIC_CMD_FW_CONTROL_V1		        = 255,
 };
 
 /**
@@ -2069,14 +2071,23 @@ typedef struct ionic_admin_comp ionic_fw_download_comp;
 
 /**
  * enum ionic_fw_control_oper - FW control operations
- * @IONIC_FW_RESET:     Reset firmware
- * @IONIC_FW_INSTALL:   Install firmware
- * @IONIC_FW_ACTIVATE:  Activate firmware
+ * @IONIC_FW_RESET:		Reset firmware
+ * @IONIC_FW_INSTALL:		Install firmware
+ * @IONIC_FW_ACTIVATE:		Activate firmware
+ * @IONIC_FW_INSTALL_ASYNC:	Install firmware asynchronously
+ * @IONIC_FW_INSTALL_STATUS:	Firmware installation status
+ * @IONIC_FW_ACTIVATE_ASYNC:	Activate firmware asynchronously
+ * @IONIC_FW_ACTIVATE_STATUS:	Firmware activate status
  */
 enum ionic_fw_control_oper {
-	IONIC_FW_RESET		= 0,
-	IONIC_FW_INSTALL	= 1,
-	IONIC_FW_ACTIVATE	= 2,
+	IONIC_FW_RESET			= 0,
+	IONIC_FW_INSTALL		= 1,
+	IONIC_FW_ACTIVATE		= 2,
+	IONIC_FW_INSTALL_ASYNC		= 3,
+	IONIC_FW_INSTALL_STATUS		= 4,
+	IONIC_FW_ACTIVATE_ASYNC		= 5,
+	IONIC_FW_ACTIVATE_STATUS	= 6,
+	IONIC_FW_UPDATE_CLEANUP		= 7,
 };
 
 /**
@@ -2689,6 +2700,9 @@ union ionic_dev_cmd {
 	struct ionic_q_identify_cmd q_identify;
 	struct ionic_q_init_cmd q_init;
 	struct ionic_q_control_cmd q_control;
+
+	struct ionic_fw_download_cmd fw_download;
+	struct ionic_fw_control_cmd fw_control;
 };
 
 union ionic_dev_cmd_comp {
@@ -2722,6 +2736,9 @@ union ionic_dev_cmd_comp {
 
 	struct ionic_q_identify_comp q_identify;
 	struct ionic_q_init_comp q_init;
+
+	ionic_fw_download_comp fw_download;
+	struct ionic_fw_control_comp fw_control;
 };
 
 /**
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_main.c b/drivers/net/ethernet/pensando/ionic/ionic_main.c
index cfb90bf605fe..99e9dd15a303 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_main.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_main.c
@@ -170,6 +170,10 @@ static const char *ionic_opcode_to_str(enum ionic_cmd_opcode opcode)
 		return "IONIC_CMD_FW_DOWNLOAD";
 	case IONIC_CMD_FW_CONTROL:
 		return "IONIC_CMD_FW_CONTROL";
+	case IONIC_CMD_FW_DOWNLOAD_V1:
+		return "IONIC_CMD_FW_DOWNLOAD_V1";
+	case IONIC_CMD_FW_CONTROL_V1:
+		return "IONIC_CMD_FW_CONTROL_V1";
 	case IONIC_CMD_VF_GETATTR:
 		return "IONIC_CMD_VF_GETATTR";
 	case IONIC_CMD_VF_SETATTR:
-- 
2.17.1

[PATCH v4 net-next 5/5] ionic: add devlink firmware update

[Shannon Nelson]

Add support for firmware update through the devlink interface.
This update copies the firmware object into the device, asks
the current firmware to install it, then asks the firmware to
select the new firmware for the next boot-up.

The install and select steps are launched as asynchronous
requests, which are then followed up with status request
commands.  These status request commands will be answered with
an EAGAIN return value and will try again until the request
has completed or reached the timeout specified.

Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 drivers/net/ethernet/pensando/ionic/Makefile  |   2 +-
 .../ethernet/pensando/ionic/ionic_devlink.c   |  14 ++
 .../ethernet/pensando/ionic/ionic_devlink.h   |   3 +
 .../net/ethernet/pensando/ionic/ionic_fw.c    | 206 ++++++++++++++++++
 .../net/ethernet/pensando/ionic/ionic_main.c  |  23 +-
 5 files changed, 239 insertions(+), 9 deletions(-)
 create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_fw.c

diff --git a/drivers/net/ethernet/pensando/ionic/Makefile b/drivers/net/ethernet/pensando/ionic/Makefile
index 29f304d75261..8d3c2d3cb10d 100644
--- a/drivers/net/ethernet/pensando/ionic/Makefile
+++ b/drivers/net/ethernet/pensando/ionic/Makefile
@@ -5,4 +5,4 @@ obj-$(CONFIG_IONIC) := ionic.o
 
 ionic-y := ionic_main.o ionic_bus_pci.o ionic_devlink.o ionic_dev.o \
 	   ionic_debugfs.o ionic_lif.o ionic_rx_filter.o ionic_ethtool.o \
-	   ionic_txrx.o ionic_stats.o
+	   ionic_txrx.o ionic_stats.o ionic_fw.o
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
index 8d9fb2e19cca..5348f05ebc32 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
@@ -9,6 +9,19 @@
 #include "ionic_lif.h"
 #include "ionic_devlink.h"
 
+static int ionic_dl_flash_update(struct devlink *dl,
+				 const char *fwname,
+				 const char *component,
+				 struct netlink_ext_ack *extack)
+{
+	struct ionic *ionic = devlink_priv(dl);
+
+	if (component)
+		return -EOPNOTSUPP;
+
+	return ionic_firmware_update(ionic->lif, fwname, extack);
+}
+
 static int ionic_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
 			     struct netlink_ext_ack *extack)
 {
@@ -48,6 +61,7 @@ static int ionic_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
 
 static const struct devlink_ops ionic_dl_ops = {
 	.info_get	= ionic_dl_info_get,
+	.flash_update	= ionic_dl_flash_update,
 };
 
 struct ionic *ionic_devlink_alloc(struct device *dev)
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
index 0690172fc57a..5c01a9e306d8 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
@@ -6,6 +6,9 @@
 
 #include <net/devlink.h>
 
+int ionic_firmware_update(struct ionic_lif *lif, const char *fw_name,
+			  struct netlink_ext_ack *extack);
+
 struct ionic *ionic_devlink_alloc(struct device *dev);
 void ionic_devlink_free(struct ionic *ionic);
 int ionic_devlink_register(struct ionic *ionic);
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_fw.c b/drivers/net/ethernet/pensando/ionic/ionic_fw.c
new file mode 100644
index 000000000000..f492ae406a60
--- /dev/null
+++ b/drivers/net/ethernet/pensando/ionic/ionic_fw.c
@@ -0,0 +1,206 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2020 Pensando Systems, Inc */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/errno.h>
+#include <linux/firmware.h>
+
+#include "ionic.h"
+#include "ionic_dev.h"
+#include "ionic_lif.h"
+#include "ionic_devlink.h"
+
+/* The worst case wait for the install activity is about 25 minutes when
+ * installing a new CPLD, which is very seldom.  Normal is about 30-35
+ * seconds.  Since the driver can't tell if a CPLD update will happen we
+ * set the timeout for the ugly case.
+ */
+#define IONIC_FW_INSTALL_TIMEOUT	(25 * 60)
+#define IONIC_FW_SELECT_TIMEOUT		30
+
+/* Number of periodic log updates during fw file download */
+#define IONIC_FW_INTERVAL_FRACTION	32
+
+static void ionic_dev_cmd_firmware_download(struct ionic_dev *idev, u64 addr,
+					    u32 offset, u32 length)
+{
+	union ionic_dev_cmd cmd = {
+		.fw_download.opcode = IONIC_CMD_FW_DOWNLOAD,
+		.fw_download.offset = offset,
+		.fw_download.addr = addr,
+		.fw_download.length = length
+	};
+
+	ionic_dev_cmd_go(idev, &cmd);
+}
+
+static void ionic_dev_cmd_firmware_install(struct ionic_dev *idev)
+{
+	union ionic_dev_cmd cmd = {
+		.fw_control.opcode = IONIC_CMD_FW_CONTROL,
+		.fw_control.oper = IONIC_FW_INSTALL_ASYNC
+	};
+
+	ionic_dev_cmd_go(idev, &cmd);
+}
+
+static void ionic_dev_cmd_firmware_activate(struct ionic_dev *idev, u8 slot)
+{
+	union ionic_dev_cmd cmd = {
+		.fw_control.opcode = IONIC_CMD_FW_CONTROL,
+		.fw_control.oper = IONIC_FW_ACTIVATE_ASYNC,
+		.fw_control.slot = slot
+	};
+
+	ionic_dev_cmd_go(idev, &cmd);
+}
+
+static int ionic_fw_status_long_wait(struct ionic *ionic,
+				     const char *label,
+				     unsigned long timeout,
+				     u8 fw_cmd,
+				     struct netlink_ext_ack *extack)
+{
+	union ionic_dev_cmd cmd = {
+		.fw_control.opcode = IONIC_CMD_FW_CONTROL,
+		.fw_control.oper = fw_cmd,
+	};
+	unsigned long start_time;
+	unsigned long end_time;
+	int err;
+
+	start_time = jiffies;
+	end_time = start_time + (timeout * HZ);
+	do {
+		mutex_lock(&ionic->dev_cmd_lock);
+		ionic_dev_cmd_go(&ionic->idev, &cmd);
+		err = ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
+		mutex_unlock(&ionic->dev_cmd_lock);
+
+		msleep(20);
+	} while (time_before(jiffies, end_time) && (err == -EAGAIN || err == -ETIMEDOUT));
+
+	if (err == -EAGAIN || err == -ETIMEDOUT) {
+		NL_SET_ERR_MSG_MOD(extack, "Firmware wait timed out");
+		dev_err(ionic->dev, "DEV_CMD firmware wait %s timed out\n", label);
+	} else if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Firmware wait failed");
+	}
+
+	return err;
+}
+
+int ionic_firmware_update(struct ionic_lif *lif, const char *fw_name,
+			  struct netlink_ext_ack *extack)
+{
+	struct ionic_dev *idev = &lif->ionic->idev;
+	struct net_device *netdev = lif->netdev;
+	struct ionic *ionic = lif->ionic;
+	union ionic_dev_cmd_comp comp;
+	u32 buf_sz, copy_sz, offset;
+	const struct firmware *fw;
+	struct devlink *dl;
+	int next_interval;
+	int err = 0;
+	u8 fw_slot;
+
+	netdev_info(netdev, "Installing firmware %s\n", fw_name);
+
+	dl = priv_to_devlink(ionic);
+	devlink_flash_update_begin_notify(dl);
+	devlink_flash_update_status_notify(dl, "Preparing to flash", NULL, 0, 0);
+
+	err = request_firmware(&fw, fw_name, ionic->dev);
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Unable to find firmware file");
+		goto err_out;
+	}
+
+	buf_sz = sizeof(idev->dev_cmd_regs->data);
+
+	netdev_dbg(netdev,
+		   "downloading firmware - size %d part_sz %d nparts %lu\n",
+		   (int)fw->size, buf_sz, DIV_ROUND_UP(fw->size, buf_sz));
+
+	offset = 0;
+	next_interval = 0;
+	while (offset < fw->size) {
+		if (offset >= next_interval) {
+			devlink_flash_update_status_notify(dl, "Downloading", NULL,
+							   offset, fw->size);
+			next_interval = offset + (fw->size / IONIC_FW_INTERVAL_FRACTION);
+		}
+
+		copy_sz = min_t(unsigned int, buf_sz, fw->size - offset);
+		mutex_lock(&ionic->dev_cmd_lock);
+		memcpy_toio(&idev->dev_cmd_regs->data, fw->data + offset, copy_sz);
+		ionic_dev_cmd_firmware_download(idev,
+						offsetof(union ionic_dev_cmd_regs, data),
+						offset, copy_sz);
+		err = ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
+		mutex_unlock(&ionic->dev_cmd_lock);
+		if (err) {
+			netdev_err(netdev,
+				   "download failed offset 0x%x addr 0x%lx len 0x%x\n",
+				   offset, offsetof(union ionic_dev_cmd_regs, data),
+				   copy_sz);
+			NL_SET_ERR_MSG_MOD(extack, "Segment download failed");
+			goto err_out;
+		}
+		offset += copy_sz;
+	}
+	devlink_flash_update_status_notify(dl, "Downloading", NULL,
+					   fw->size, fw->size);
+
+	devlink_flash_update_timeout_notify(dl, "Installing", NULL,
+					    IONIC_FW_INSTALL_TIMEOUT);
+
+	mutex_lock(&ionic->dev_cmd_lock);
+	ionic_dev_cmd_firmware_install(idev);
+	err = ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
+	ionic_dev_cmd_comp(idev, (union ionic_dev_cmd_comp *)&comp);
+	fw_slot = comp.fw_control.slot;
+	mutex_unlock(&ionic->dev_cmd_lock);
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to start firmware install");
+		goto err_out;
+	}
+
+	err = ionic_fw_status_long_wait(ionic, "Installing",
+					IONIC_FW_INSTALL_TIMEOUT,
+					IONIC_FW_INSTALL_STATUS,
+					extack);
+	if (err)
+		goto err_out;
+
+	devlink_flash_update_timeout_notify(dl, "Selecting", NULL,
+					    IONIC_FW_SELECT_TIMEOUT);
+
+	mutex_lock(&ionic->dev_cmd_lock);
+	ionic_dev_cmd_firmware_activate(idev, fw_slot);
+	err = ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
+	mutex_unlock(&ionic->dev_cmd_lock);
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to start firmware select");
+		goto err_out;
+	}
+
+	err = ionic_fw_status_long_wait(ionic, "Selecting",
+					IONIC_FW_SELECT_TIMEOUT,
+					IONIC_FW_ACTIVATE_STATUS,
+					extack);
+	if (err)
+		goto err_out;
+
+	netdev_info(netdev, "Firmware update completed\n");
+
+err_out:
+	if (err)
+		devlink_flash_update_status_notify(dl, "Flash failed", NULL, 0, 0);
+	else
+		devlink_flash_update_status_notify(dl, "Flash done", NULL, 0, 0);
+	release_firmware(fw);
+	devlink_flash_update_end_notify(dl);
+	return err;
+}
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_main.c b/drivers/net/ethernet/pensando/ionic/ionic_main.c
index 99e9dd15a303..e339216949a6 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_main.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_main.c
@@ -335,17 +335,22 @@ int ionic_dev_cmd_wait(struct ionic *ionic, unsigned long max_seconds)
 	 */
 	max_wait = jiffies + (max_seconds * HZ);
 try_again:
+	opcode = idev->dev_cmd_regs->cmd.cmd.opcode;
 	start_time = jiffies;
 	do {
 		done = ionic_dev_cmd_done(idev);
 		if (done)
 			break;
-		msleep(5);
-		hb = ionic_heartbeat_check(ionic);
+		usleep_range(100, 200);
+
+		/* Don't check the heartbeat on FW_CONTROL commands as they are
+		 * notorious for interrupting the firmware's heartbeat update.
+		 */
+		if (opcode != IONIC_CMD_FW_CONTROL)
+			hb = ionic_heartbeat_check(ionic);
 	} while (!done && !hb && time_before(jiffies, max_wait));
 	duration = jiffies - start_time;
 
-	opcode = idev->dev_cmd_regs->cmd.cmd.opcode;
 	dev_dbg(ionic->dev, "DEVCMD %s (%d) done=%d took %ld secs (%ld jiffies)\n",
 		ionic_opcode_to_str(opcode), opcode,
 		done, duration / HZ, duration);
@@ -369,8 +374,9 @@ int ionic_dev_cmd_wait(struct ionic *ionic, unsigned long max_seconds)
 
 	err = ionic_dev_cmd_status(&ionic->idev);
 	if (err) {
-		if (err == IONIC_RC_EAGAIN && !time_after(jiffies, max_wait)) {
-			dev_err(ionic->dev, "DEV_CMD %s (%d) error, %s (%d) retrying...\n",
+		if (err == IONIC_RC_EAGAIN &&
+		    time_before(jiffies, (max_wait - HZ))) {
+			dev_dbg(ionic->dev, "DEV_CMD %s (%d), %s (%d) retrying...\n",
 				ionic_opcode_to_str(opcode), opcode,
 				ionic_error_to_str(err), err);
 
@@ -380,9 +386,10 @@ int ionic_dev_cmd_wait(struct ionic *ionic, unsigned long max_seconds)
 			goto try_again;
 		}
 
-		dev_err(ionic->dev, "DEV_CMD %s (%d) error, %s (%d) failed\n",
-			ionic_opcode_to_str(opcode), opcode,
-			ionic_error_to_str(err), err);
+		if (!(opcode == IONIC_CMD_FW_CONTROL && err == IONIC_RC_EAGAIN))
+			dev_err(ionic->dev, "DEV_CMD %s (%d) error, %s (%d) failed\n",
+				ionic_opcode_to_str(opcode), opcode,
+				ionic_error_to_str(err), err);
 
 		return ionic_error_to_errno(err);
 	}
-- 
2.17.1

Re: [PATCH v4 net-next 1/5] devlink: add timeout information to status_notify

[Jakub Kicinski]

On Wed, 16 Sep 2020 20:02:00 -0700 Shannon Nelson wrote:
> Add a timeout element to the DEVLINK_CMD_FLASH_UPDATE_STATUS
> netlink message for use by a userland utility to show that
> a particular firmware flash activity may take a long but
> bounded time to finish.  Also add a handy helper for drivers
> to make use of the new timeout value.
> 
> UI usage hints:
>  - if non-zero, add timeout display to the end of the status line
>  	[component] status_msg  ( Xm Ys : Am Bs )
>      using the timeout value for Am Bs and updating the Xm Ys
>      every second
>  - if the timeout expires while awaiting the next update,
>    display something like
>  	[component] status_msg  ( timeout reached : Am Bs )
>  - if new status notify messages are received, remove
>    the timeout and start over
> 
> Signed-off-by: Shannon Nelson <snelson@pensando.io>

Minor nits, otherwise LGTM:

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

> @@ -3052,6 +3054,9 @@ static int devlink_nl_flash_update_fill(struct sk_buff *msg,
>  	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TOTAL,
>  			      total, DEVLINK_ATTR_PAD))
>  		goto nla_put_failure;
> +	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TIMEOUT,
> +			      timeout, DEVLINK_ATTR_PAD))
> +		goto nla_put_failure;

nit: since old kernels don't report this user space has to deal with it
     not being present so I'd be tempted to only report it if timeout
     is not 0

> +void devlink_flash_update_timeout_notify(struct devlink *devlink,
> +					 const char *status_msg,
> +					 const char *component,
> +					 unsigned long timeout)
> +{
> +	__devlink_flash_update_notify(devlink,
> +				      DEVLINK_CMD_FLASH_UPDATE_STATUS,
> +				      status_msg, component, 0, 0, timeout);

nit: did we ever report cmd == UPDATE_STATUS and total == 0?
     could this cause a division by zero in some unsuspecting
     implementation? Perhaps we should pass 1 here?

> +}
> +EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify);

Re: [PATCH v4 net-next 2/5] devlink: collect flash notify params into a struct

[Jakub Kicinski]

On Wed, 16 Sep 2020 20:02:01 -0700 Shannon Nelson wrote:
> The dev flash status notify function parameter lists are getting
> rather long, so add a struct to be filled and passed rather than
> continuously changing the function signatures.
> 
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---
>  include/net/devlink.h | 21 ++++++++++++
>  net/core/devlink.c    | 80 +++++++++++++++++++++++--------------------
>  2 files changed, 63 insertions(+), 38 deletions(-)
> 
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index f206accf80ad..9ab2014885cb 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -391,6 +391,27 @@ struct devlink_param_gset_ctx {
>  	enum devlink_param_cmode cmode;
>  };
>  
> +/**
> + * struct devlink_flash_notify - devlink dev flash notify data
> + * @cmd: devlink notify command code
> + * @status_msg: current status string
> + * @component: firmware component being updated
> + * @done: amount of work completed of total amount
> + * @total: amount of work expected to be done
> + * @timeout: expected max timeout in seconds
> + *
> + * These are values to be given to userland to be displayed in order
> + * to show current activity in a firmware update process.
> + */
> +struct devlink_flash_notify {
> +	enum devlink_command cmd;

I'd leave out cmd out of the params structure, otherwise I'll be
slightly awkward for drivers to fill in given the current helpers 
are per cmd.

> +	const char *status_msg;
> +	const char *component;
> +	unsigned long done;
> +	unsigned long total;
> +	unsigned long timeout;
> +};

Re: [PATCH v4 net-next 3/5] netdevsim: devlink flash timeout message

[Jakub Kicinski]

On Wed, 16 Sep 2020 20:02:02 -0700 Shannon Nelson wrote:
> Add a simple devlink flash timeout message to exercise
> the message mechanism.
> 
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---
>  drivers/net/netdevsim/dev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
> index 32f339fedb21..4123550e3f6e 100644
> --- a/drivers/net/netdevsim/dev.c
> +++ b/drivers/net/netdevsim/dev.c
> @@ -768,6 +768,8 @@ static int nsim_dev_flash_update(struct devlink *devlink, const char *file_name,
>  						   component,
>  						   NSIM_DEV_FLASH_SIZE,
>  						   NSIM_DEV_FLASH_SIZE);
> +		devlink_flash_update_timeout_notify(devlink, "Flash timeout",
> +						    component, 81);
>  		devlink_flash_update_status_notify(devlink, "Flashing done",
>  						   component, 0, 0);
>  		devlink_flash_update_end_notify(devlink);

To mimic a more real scenario could we perhaps change the msg to "Flash
select" ?

Re: [PATCH v4 net-next 1/5] devlink: add timeout information to status_notify

[Jacob Keller]

On 9/16/2020 8:02 PM, Shannon Nelson wrote:
> Add a timeout element to the DEVLINK_CMD_FLASH_UPDATE_STATUS
> netlink message for use by a userland utility to show that
> a particular firmware flash activity may take a long but
> bounded time to finish.  Also add a handy helper for drivers
> to make use of the new timeout value.
> 
> UI usage hints:
>  - if non-zero, add timeout display to the end of the status line
>  	[component] status_msg  ( Xm Ys : Am Bs )
>      using the timeout value for Am Bs and updating the Xm Ys
>      every second
>  - if the timeout expires while awaiting the next update,
>    display something like
>  	[component] status_msg  ( timeout reached : Am Bs )
>  - if new status notify messages are received, remove
>    the timeout and start over
> 
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---

This one looks good to me. I think the only other things that I saw from
previous discussion are:

a) we could convert the internal helper devlink_nl_flash_update_fill and
__devlink_flash_update_notify to use structs for their fields, and..

b) Jakub pointed out that most drivers don't currently use the component
field so we could remove that from the helpers.

However, I don't have strong feelings about those either way, so:

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>  include/net/devlink.h        |  4 ++++
>  include/uapi/linux/devlink.h |  3 +++
>  net/core/devlink.c           | 29 +++++++++++++++++++++++------
>  3 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index eaec0a8cc5ef..f206accf80ad 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -1400,6 +1400,10 @@ void devlink_flash_update_status_notify(struct devlink *devlink,
>  					const char *component,
>  					unsigned long done,
>  					unsigned long total);
> +void devlink_flash_update_timeout_notify(struct devlink *devlink,
> +					 const char *status_msg,
> +					 const char *component,
> +					 unsigned long timeout);
>  
>  int devlink_traps_register(struct devlink *devlink,
>  			   const struct devlink_trap *traps,
> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> index 40d35145c879..4a6e213cfa04 100644
> --- a/include/uapi/linux/devlink.h
> +++ b/include/uapi/linux/devlink.h
> @@ -460,6 +460,9 @@ enum devlink_attr {
>  
>  	DEVLINK_ATTR_PORT_EXTERNAL,		/* u8 */
>  	DEVLINK_ATTR_PORT_CONTROLLER_NUMBER,	/* u32 */
> +
> +	DEVLINK_ATTR_FLASH_UPDATE_STATUS_TIMEOUT,	/* u64 */
> +
>  	/* 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 19037f114307..01f855e53e06 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -3024,7 +3024,9 @@ static int devlink_nl_flash_update_fill(struct sk_buff *msg,
>  					enum devlink_command cmd,
>  					const char *status_msg,
>  					const char *component,
> -					unsigned long done, unsigned long total)
> +					unsigned long done,
> +					unsigned long total,
> +					unsigned long timeout)

Number of paramters here is getting rather large. Does it make sense to
convert this one to a struct now?

>  {
>  	void *hdr;
>  
> @@ -3052,6 +3054,9 @@ static int devlink_nl_flash_update_fill(struct sk_buff *msg,
>  	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TOTAL,
>  			      total, DEVLINK_ATTR_PAD))
>  		goto nla_put_failure;
> +	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TIMEOUT,
> +			      timeout, DEVLINK_ATTR_PAD))
> +		goto nla_put_failure;
>  
>  out:
>  	genlmsg_end(msg, hdr);
> @@ -3067,7 +3072,8 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
>  					  const char *status_msg,
>  					  const char *component,
>  					  unsigned long done,
> -					  unsigned long total)
> +					  unsigned long total,
> +					  unsigned long timeout)
>  {
>  	struct sk_buff *msg;
>  	int err;
> @@ -3081,7 +3087,7 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
>  		return;
>  
>  	err = devlink_nl_flash_update_fill(msg, devlink, cmd, status_msg,
> -					   component, done, total);
> +					   component, done, total, timeout);
>  	if (err)
>  		goto out_free_msg;
>  
> @@ -3097,7 +3103,7 @@ void devlink_flash_update_begin_notify(struct devlink *devlink)
>  {
>  	__devlink_flash_update_notify(devlink,
>  				      DEVLINK_CMD_FLASH_UPDATE,
> -				      NULL, NULL, 0, 0);
> +				      NULL, NULL, 0, 0, 0);
>  }
>  EXPORT_SYMBOL_GPL(devlink_flash_update_begin_notify);
>  
> @@ -3105,7 +3111,7 @@ void devlink_flash_update_end_notify(struct devlink *devlink)
>  {
>  	__devlink_flash_update_notify(devlink,
>  				      DEVLINK_CMD_FLASH_UPDATE_END,
> -				      NULL, NULL, 0, 0);
> +				      NULL, NULL, 0, 0, 0);
>  }
>  EXPORT_SYMBOL_GPL(devlink_flash_update_end_notify);
>  
> @@ -3117,10 +3123,21 @@ void devlink_flash_update_status_notify(struct devlink *devlink,
>  {
>  	__devlink_flash_update_notify(devlink,
>  				      DEVLINK_CMD_FLASH_UPDATE_STATUS,
> -				      status_msg, component, done, total);
> +				      status_msg, component, done, total, 0);
>  }
>  EXPORT_SYMBOL_GPL(devlink_flash_update_status_notify);
>  
> +void devlink_flash_update_timeout_notify(struct devlink *devlink,
> +					 const char *status_msg,
> +					 const char *component,
> +					 unsigned long timeout)

So we dropped the done and total here since in most cases we expect not
to need both a timeout and a done/total. I think that make sense. Most
of the time I think a command will benefit from one or the other.

> +{
> +	__devlink_flash_update_notify(devlink,
> +				      DEVLINK_CMD_FLASH_UPDATE_STATUS,
> +				      status_msg, component, 0, 0, timeout);
> +}
> +EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify);
> +
>  static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
>  				       struct genl_info *info)
>  {
> 

Re: [PATCH v4 net-next 5/5] ionic: add devlink firmware update

[Jakub Kicinski]

On Wed, 16 Sep 2020 20:02:04 -0700 Shannon Nelson wrote:
> Add support for firmware update through the devlink interface.
> This update copies the firmware object into the device, asks
> the current firmware to install it, then asks the firmware to
> select the new firmware for the next boot-up.
> 
> The install and select steps are launched as asynchronous
> requests, which are then followed up with status request
> commands.  These status request commands will be answered with
> an EAGAIN return value and will try again until the request
> has completed or reached the timeout specified.
> 
> Signed-off-by: Shannon Nelson <snelson@pensando.io>

Acked-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH v4 net-next 2/5] devlink: collect flash notify params into a struct

[Jacob Keller]

On 9/16/2020 8:02 PM, Shannon Nelson wrote:
> The dev flash status notify function parameter lists are getting
> rather long, so add a struct to be filled and passed rather than
> continuously changing the function signatures.
> 
> Signed-off-by: Shannon Nelson <snelson@pensando.io>

Guess I should have read farther in the series. I would have expected
this patch first before introducing the timeout.

LGTM.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> ---
>  include/net/devlink.h | 21 ++++++++++++
>  net/core/devlink.c    | 80 +++++++++++++++++++++++--------------------
>  2 files changed, 63 insertions(+), 38 deletions(-)
> 
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index f206accf80ad..9ab2014885cb 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -391,6 +391,27 @@ struct devlink_param_gset_ctx {
>  	enum devlink_param_cmode cmode;
>  };
>  
> +/**
> + * struct devlink_flash_notify - devlink dev flash notify data
> + * @cmd: devlink notify command code
> + * @status_msg: current status string
> + * @component: firmware component being updated
> + * @done: amount of work completed of total amount
> + * @total: amount of work expected to be done
> + * @timeout: expected max timeout in seconds
> + *
> + * These are values to be given to userland to be displayed in order
> + * to show current activity in a firmware update process.
> + */
> +struct devlink_flash_notify {
> +	enum devlink_command cmd;
> +	const char *status_msg;
> +	const char *component;
> +	unsigned long done;
> +	unsigned long total;
> +	unsigned long timeout;
> +};
> +
>  /**
>   * struct devlink_param - devlink configuration parameter data
>   * @name: name of the parameter
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 01f855e53e06..816f27a18b16 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -3021,41 +3021,36 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
>  
>  static int devlink_nl_flash_update_fill(struct sk_buff *msg,
>  					struct devlink *devlink,
> -					enum devlink_command cmd,
> -					const char *status_msg,
> -					const char *component,
> -					unsigned long done,
> -					unsigned long total,
> -					unsigned long timeout)
> +					struct devlink_flash_notify *params)
>  {
>  	void *hdr;
>  
> -	hdr = genlmsg_put(msg, 0, 0, &devlink_nl_family, 0, cmd);
> +	hdr = genlmsg_put(msg, 0, 0, &devlink_nl_family, 0, params->cmd);
>  	if (!hdr)
>  		return -EMSGSIZE;
>  
>  	if (devlink_nl_put_handle(msg, devlink))
>  		goto nla_put_failure;
>  
> -	if (cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS)
> +	if (params->cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS)
>  		goto out;
>  
> -	if (status_msg &&
> +	if (params->status_msg &&
>  	    nla_put_string(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_MSG,
> -			   status_msg))
> +			   params->status_msg))
>  		goto nla_put_failure;
> -	if (component &&
> +	if (params->component &&
>  	    nla_put_string(msg, DEVLINK_ATTR_FLASH_UPDATE_COMPONENT,
> -			   component))
> +			   params->component))
>  		goto nla_put_failure;
>  	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_DONE,
> -			      done, DEVLINK_ATTR_PAD))
> +			      params->done, DEVLINK_ATTR_PAD))
>  		goto nla_put_failure;
>  	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TOTAL,
> -			      total, DEVLINK_ATTR_PAD))
> +			      params->total, DEVLINK_ATTR_PAD))
>  		goto nla_put_failure;
>  	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TIMEOUT,
> -			      timeout, DEVLINK_ATTR_PAD))
> +			      params->timeout, DEVLINK_ATTR_PAD))
>  		goto nla_put_failure;
>  
>  out:
> @@ -3068,26 +3063,20 @@ static int devlink_nl_flash_update_fill(struct sk_buff *msg,
>  }
>  
>  static void __devlink_flash_update_notify(struct devlink *devlink,
> -					  enum devlink_command cmd,
> -					  const char *status_msg,
> -					  const char *component,
> -					  unsigned long done,
> -					  unsigned long total,
> -					  unsigned long timeout)
> +					  struct devlink_flash_notify *params)
>  {
>  	struct sk_buff *msg;
>  	int err;
>  
> -	WARN_ON(cmd != DEVLINK_CMD_FLASH_UPDATE &&
> -		cmd != DEVLINK_CMD_FLASH_UPDATE_END &&
> -		cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS);
> +	WARN_ON(params->cmd != DEVLINK_CMD_FLASH_UPDATE &&
> +		params->cmd != DEVLINK_CMD_FLASH_UPDATE_END &&
> +		params->cmd != DEVLINK_CMD_FLASH_UPDATE_STATUS);
>  
>  	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>  	if (!msg)
>  		return;
>  
> -	err = devlink_nl_flash_update_fill(msg, devlink, cmd, status_msg,
> -					   component, done, total, timeout);
> +	err = devlink_nl_flash_update_fill(msg, devlink, params);
>  	if (err)
>  		goto out_free_msg;
>  
> @@ -3101,17 +3090,21 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
>  
>  void devlink_flash_update_begin_notify(struct devlink *devlink)
>  {
> -	__devlink_flash_update_notify(devlink,
> -				      DEVLINK_CMD_FLASH_UPDATE,
> -				      NULL, NULL, 0, 0, 0);
> +	struct devlink_flash_notify params = {
> +		.cmd = DEVLINK_CMD_FLASH_UPDATE,
> +	};
> +
> +	__devlink_flash_update_notify(devlink, &params);
>  }
>  EXPORT_SYMBOL_GPL(devlink_flash_update_begin_notify);
>  
>  void devlink_flash_update_end_notify(struct devlink *devlink)
>  {
> -	__devlink_flash_update_notify(devlink,
> -				      DEVLINK_CMD_FLASH_UPDATE_END,
> -				      NULL, NULL, 0, 0, 0);
> +	struct devlink_flash_notify params = {
> +		.cmd = DEVLINK_CMD_FLASH_UPDATE_END,
> +	};
> +
> +	__devlink_flash_update_notify(devlink, &params);
>  }
>  EXPORT_SYMBOL_GPL(devlink_flash_update_end_notify);
>  
> @@ -3121,9 +3114,15 @@ void devlink_flash_update_status_notify(struct devlink *devlink,
>  					unsigned long done,
>  					unsigned long total)
>  {
> -	__devlink_flash_update_notify(devlink,
> -				      DEVLINK_CMD_FLASH_UPDATE_STATUS,
> -				      status_msg, component, done, total, 0);
> +	struct devlink_flash_notify params = {
> +		.cmd = DEVLINK_CMD_FLASH_UPDATE_STATUS,
> +		.status_msg = status_msg,
> +		.component = component,
> +		.done = done,
> +		.total = total,
> +	};
> +
> +	__devlink_flash_update_notify(devlink, &params);
>  }
>  EXPORT_SYMBOL_GPL(devlink_flash_update_status_notify);
>  
> @@ -3132,9 +3131,14 @@ void devlink_flash_update_timeout_notify(struct devlink *devlink,
>  					 const char *component,
>  					 unsigned long timeout)
>  {
> -	__devlink_flash_update_notify(devlink,
> -				      DEVLINK_CMD_FLASH_UPDATE_STATUS,
> -				      status_msg, component, 0, 0, timeout);
> +	struct devlink_flash_notify params = {
> +		.cmd = DEVLINK_CMD_FLASH_UPDATE_STATUS,
> +		.status_msg = status_msg,
> +		.component = component,
> +		.timeout = timeout,
> +	};
> +
> +	__devlink_flash_update_notify(devlink, &params);
>  }
>  EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify);
>  
> 

Re: [PATCH v4 net-next 5/5] ionic: add devlink firmware update

[Jacob Keller]

On 9/16/2020 8:02 PM, Shannon Nelson wrote:
> Add support for firmware update through the devlink interface.
> This update copies the firmware object into the device, asks
> the current firmware to install it, then asks the firmware to
> select the new firmware for the next boot-up.
> 
> The install and select steps are launched as asynchronous
> requests, which are then followed up with status request
> commands.  These status request commands will be answered with
> an EAGAIN return value and will try again until the request
> has completed or reached the timeout specified.
> 
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---
>  drivers/net/ethernet/pensando/ionic/Makefile  |   2 +-
>  .../ethernet/pensando/ionic/ionic_devlink.c   |  14 ++
>  .../ethernet/pensando/ionic/ionic_devlink.h   |   3 +
>  .../net/ethernet/pensando/ionic/ionic_fw.c    | 206 ++++++++++++++++++
>  .../net/ethernet/pensando/ionic/ionic_main.c  |  23 +-
>  5 files changed, 239 insertions(+), 9 deletions(-)
>  create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_fw.c
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/Makefile b/drivers/net/ethernet/pensando/ionic/Makefile
> index 29f304d75261..8d3c2d3cb10d 100644
> --- a/drivers/net/ethernet/pensando/ionic/Makefile
> +++ b/drivers/net/ethernet/pensando/ionic/Makefile
> @@ -5,4 +5,4 @@ obj-$(CONFIG_IONIC) := ionic.o
>  
>  ionic-y := ionic_main.o ionic_bus_pci.o ionic_devlink.o ionic_dev.o \
>  	   ionic_debugfs.o ionic_lif.o ionic_rx_filter.o ionic_ethtool.o \
> -	   ionic_txrx.o ionic_stats.o
> +	   ionic_txrx.o ionic_stats.o ionic_fw.o
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> index 8d9fb2e19cca..5348f05ebc32 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> @@ -9,6 +9,19 @@
>  #include "ionic_lif.h"
>  #include "ionic_devlink.h"
>  
> +static int ionic_dl_flash_update(struct devlink *dl,
> +				 const char *fwname,
> +				 const char *component,
> +				 struct netlink_ext_ack *extack)
> +{
> +	struct ionic *ionic = devlink_priv(dl);
> +
> +	if (component)
> +		return -EOPNOTSUPP;
> +
> +	return ionic_firmware_update(ionic->lif, fwname, extack);
> +}
> +
>  static int ionic_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
>  			     struct netlink_ext_ack *extack)
>  {
> @@ -48,6 +61,7 @@ static int ionic_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
>  
>  static const struct devlink_ops ionic_dl_ops = {
>  	.info_get	= ionic_dl_info_get,
> +	.flash_update	= ionic_dl_flash_update,
>  };
>  
>  struct ionic *ionic_devlink_alloc(struct device *dev)
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
> index 0690172fc57a..5c01a9e306d8 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
> @@ -6,6 +6,9 @@
>  
>  #include <net/devlink.h>
>  
> +int ionic_firmware_update(struct ionic_lif *lif, const char *fw_name,
> +			  struct netlink_ext_ack *extack);
> +
>  struct ionic *ionic_devlink_alloc(struct device *dev);
>  void ionic_devlink_free(struct ionic *ionic);
>  int ionic_devlink_register(struct ionic *ionic);
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_fw.c b/drivers/net/ethernet/pensando/ionic/ionic_fw.c
> new file mode 100644
> index 000000000000..f492ae406a60
> --- /dev/null
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_fw.c
> @@ -0,0 +1,206 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2020 Pensando Systems, Inc */
> +
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/firmware.h>
> +
> +#include "ionic.h"
> +#include "ionic_dev.h"
> +#include "ionic_lif.h"
> +#include "ionic_devlink.h"
> +
> +/* The worst case wait for the install activity is about 25 minutes when
> + * installing a new CPLD, which is very seldom.  Normal is about 30-35
> + * seconds.  Since the driver can't tell if a CPLD update will happen we
> + * set the timeout for the ugly case.
> + */
> +#define IONIC_FW_INSTALL_TIMEOUT	(25 * 60)
> +#define IONIC_FW_SELECT_TIMEOUT		30
> +
> +/* Number of periodic log updates during fw file download */
> +#define IONIC_FW_INTERVAL_FRACTION	32
> +
> +static void ionic_dev_cmd_firmware_download(struct ionic_dev *idev, u64 addr,
> +					    u32 offset, u32 length)
> +{
> +	union ionic_dev_cmd cmd = {
> +		.fw_download.opcode = IONIC_CMD_FW_DOWNLOAD,
> +		.fw_download.offset = offset,
> +		.fw_download.addr = addr,
> +		.fw_download.length = length
> +	};
> +
> +	ionic_dev_cmd_go(idev, &cmd);
> +}
> +
> +static void ionic_dev_cmd_firmware_install(struct ionic_dev *idev)
> +{
> +	union ionic_dev_cmd cmd = {
> +		.fw_control.opcode = IONIC_CMD_FW_CONTROL,
> +		.fw_control.oper = IONIC_FW_INSTALL_ASYNC
> +	};
> +
> +	ionic_dev_cmd_go(idev, &cmd);
> +}
> +
> +static void ionic_dev_cmd_firmware_activate(struct ionic_dev *idev, u8 slot)
> +{
> +	union ionic_dev_cmd cmd = {
> +		.fw_control.opcode = IONIC_CMD_FW_CONTROL,
> +		.fw_control.oper = IONIC_FW_ACTIVATE_ASYNC,
> +		.fw_control.slot = slot
> +	};
> +
> +	ionic_dev_cmd_go(idev, &cmd);
> +}
> +
> +static int ionic_fw_status_long_wait(struct ionic *ionic,
> +				     const char *label,
> +				     unsigned long timeout,
> +				     u8 fw_cmd,
> +				     struct netlink_ext_ack *extack)
> +{
> +	union ionic_dev_cmd cmd = {
> +		.fw_control.opcode = IONIC_CMD_FW_CONTROL,
> +		.fw_control.oper = fw_cmd,
> +	};
> +	unsigned long start_time;
> +	unsigned long end_time;
> +	int err;
> +
> +	start_time = jiffies;
> +	end_time = start_time + (timeout * HZ);
> +	do {
> +		mutex_lock(&ionic->dev_cmd_lock);
> +		ionic_dev_cmd_go(&ionic->idev, &cmd);
> +		err = ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
> +		mutex_unlock(&ionic->dev_cmd_lock);
> +
> +		msleep(20);
> +	} while (time_before(jiffies, end_time) && (err == -EAGAIN || err == -ETIMEDOUT));
> +
> +	if (err == -EAGAIN || err == -ETIMEDOUT) {
> +		NL_SET_ERR_MSG_MOD(extack, "Firmware wait timed out");
> +		dev_err(ionic->dev, "DEV_CMD firmware wait %s timed out\n", label);
> +	} else if (err) {
> +		NL_SET_ERR_MSG_MOD(extack, "Firmware wait failed");
> +	}
> +
> +	return err;
> +}
> +
> +int ionic_firmware_update(struct ionic_lif *lif, const char *fw_name,
> +			  struct netlink_ext_ack *extack)
> +{
> +	struct ionic_dev *idev = &lif->ionic->idev;
> +	struct net_device *netdev = lif->netdev;
> +	struct ionic *ionic = lif->ionic;
> +	union ionic_dev_cmd_comp comp;
> +	u32 buf_sz, copy_sz, offset;
> +	const struct firmware *fw;
> +	struct devlink *dl;
> +	int next_interval;
> +	int err = 0;
> +	u8 fw_slot;
> +
> +	netdev_info(netdev, "Installing firmware %s\n", fw_name);
> +
> +	dl = priv_to_devlink(ionic);
> +	devlink_flash_update_begin_notify(dl);
> +	devlink_flash_update_status_notify(dl, "Preparing to flash", NULL, 0, 0);
> +
> +	err = request_firmware(&fw, fw_name, ionic->dev);
> +	if (err) {
> +		NL_SET_ERR_MSG_MOD(extack, "Unable to find firmware file");
> +		goto err_out;
> +	}
> +
> +	buf_sz = sizeof(idev->dev_cmd_regs->data);
> +
> +	netdev_dbg(netdev,
> +		   "downloading firmware - size %d part_sz %d nparts %lu\n",
> +		   (int)fw->size, buf_sz, DIV_ROUND_UP(fw->size, buf_sz));
> +
> +	offset = 0;
> +	next_interval = 0;
> +	while (offset < fw->size) {
> +		if (offset >= next_interval) {
> +			devlink_flash_update_status_notify(dl, "Downloading", NULL,
> +							   offset, fw->size);
> +			next_interval = offset + (fw->size / IONIC_FW_INTERVAL_FRACTION);
> +		}
> +
> +		copy_sz = min_t(unsigned int, buf_sz, fw->size - offset);
> +		mutex_lock(&ionic->dev_cmd_lock);
> +		memcpy_toio(&idev->dev_cmd_regs->data, fw->data + offset, copy_sz);
> +		ionic_dev_cmd_firmware_download(idev,
> +						offsetof(union ionic_dev_cmd_regs, data),
> +						offset, copy_sz);
> +		err = ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
> +		mutex_unlock(&ionic->dev_cmd_lock);
> +		if (err) {
> +			netdev_err(netdev,
> +				   "download failed offset 0x%x addr 0x%lx len 0x%x\n",
> +				   offset, offsetof(union ionic_dev_cmd_regs, data),
> +				   copy_sz);
> +			NL_SET_ERR_MSG_MOD(extack, "Segment download failed");
> +			goto err_out;
> +		}
> +		offset += copy_sz;
> +	}
> +	devlink_flash_update_status_notify(dl, "Downloading", NULL,
> +					   fw->size, fw->size);
> +

I'm a little surprised to see these one-after-the-other here. Oh, this
first one is to complete the download message above. Ok, that makes sense.

> +	devlink_flash_update_timeout_notify(dl, "Installing", NULL,
> +					    IONIC_FW_INSTALL_TIMEOUT);
> +
> +	mutex_lock(&ionic->dev_cmd_lock);
> +	ionic_dev_cmd_firmware_install(idev);
> +	err = ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
> +	ionic_dev_cmd_comp(idev, (union ionic_dev_cmd_comp *)&comp);
> +	fw_slot = comp.fw_control.slot;
> +	mutex_unlock(&ionic->dev_cmd_lock);
> +	if (err) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to start firmware install");
> +		goto err_out;
> +	}
> +
> +	err = ionic_fw_status_long_wait(ionic, "Installing",
> +					IONIC_FW_INSTALL_TIMEOUT,
> +					IONIC_FW_INSTALL_STATUS,
> +					extack);
> +	if (err)
> +		goto err_out;
> +
> +	devlink_flash_update_timeout_notify(dl, "Selecting", NULL,
> +					    IONIC_FW_SELECT_TIMEOUT);
> +
> +	mutex_lock(&ionic->dev_cmd_lock);
> +	ionic_dev_cmd_firmware_activate(idev, fw_slot);
> +	err = ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
> +	mutex_unlock(&ionic->dev_cmd_lock);
> +	if (err) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to start firmware select");
> +		goto err_out;
> +	}
> +
> +	err = ionic_fw_status_long_wait(ionic, "Selecting",
> +					IONIC_FW_SELECT_TIMEOUT,
> +					IONIC_FW_ACTIVATE_STATUS,
> +					extack);
> +	if (err)
> +		goto err_out;
> +
> +	netdev_info(netdev, "Firmware update completed\n");
> +
> +err_out:
> +	if (err)
> +		devlink_flash_update_status_notify(dl, "Flash failed", NULL, 0, 0);
> +	else
> +		devlink_flash_update_status_notify(dl, "Flash done", NULL, 0, 0);
> +	release_firmware(fw);
> +	devlink_flash_update_end_notify(dl);
> +	return err;
> +}
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_main.c b/drivers/net/ethernet/pensando/ionic/ionic_main.c
> index 99e9dd15a303..e339216949a6 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_main.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_main.c
> @@ -335,17 +335,22 @@ int ionic_dev_cmd_wait(struct ionic *ionic, unsigned long max_seconds)
>  	 */
>  	max_wait = jiffies + (max_seconds * HZ);
>  try_again:
> +	opcode = idev->dev_cmd_regs->cmd.cmd.opcode;
>  	start_time = jiffies;
>  	do {
>  		done = ionic_dev_cmd_done(idev);
>  		if (done)
>  			break;
> -		msleep(5);
> -		hb = ionic_heartbeat_check(ionic);
> +		usleep_range(100, 200);
> +
> +		/* Don't check the heartbeat on FW_CONTROL commands as they are
> +		 * notorious for interrupting the firmware's heartbeat update.
> +		 */
> +		if (opcode != IONIC_CMD_FW_CONTROL)
> +			hb = ionic_heartbeat_check(ionic);
>  	} while (!done && !hb && time_before(jiffies, max_wait));
>  	duration = jiffies - start_time;
>  
> -	opcode = idev->dev_cmd_regs->cmd.cmd.opcode;
>  	dev_dbg(ionic->dev, "DEVCMD %s (%d) done=%d took %ld secs (%ld jiffies)\n",
>  		ionic_opcode_to_str(opcode), opcode,
>  		done, duration / HZ, duration);
> @@ -369,8 +374,9 @@ int ionic_dev_cmd_wait(struct ionic *ionic, unsigned long max_seconds)
>  
>  	err = ionic_dev_cmd_status(&ionic->idev);
>  	if (err) {
> -		if (err == IONIC_RC_EAGAIN && !time_after(jiffies, max_wait)) {
> -			dev_err(ionic->dev, "DEV_CMD %s (%d) error, %s (%d) retrying...\n",
> +		if (err == IONIC_RC_EAGAIN &&
> +		    time_before(jiffies, (max_wait - HZ))) {
> +			dev_dbg(ionic->dev, "DEV_CMD %s (%d), %s (%d) retrying...\n",
>  				ionic_opcode_to_str(opcode), opcode,
>  				ionic_error_to_str(err), err);
>  
> @@ -380,9 +386,10 @@ int ionic_dev_cmd_wait(struct ionic *ionic, unsigned long max_seconds)
>  			goto try_again;
>  		}
>  
> -		dev_err(ionic->dev, "DEV_CMD %s (%d) error, %s (%d) failed\n",
> -			ionic_opcode_to_str(opcode), opcode,
> -			ionic_error_to_str(err), err);
> +		if (!(opcode == IONIC_CMD_FW_CONTROL && err == IONIC_RC_EAGAIN))
> +			dev_err(ionic->dev, "DEV_CMD %s (%d) error, %s (%d) failed\n",
> +				ionic_opcode_to_str(opcode), opcode,
> +				ionic_error_to_str(err), err);
>  
>  		return ionic_error_to_errno(err);
>  	}
> 

Re: [PATCH v4 net-next 1/5] devlink: add timeout information to status_notify

[Shannon Nelson]

On 9/17/20 12:46 PM, Jakub Kicinski wrote:
> On Wed, 16 Sep 2020 20:02:00 -0700 Shannon Nelson wrote:
>> Add a timeout element to the DEVLINK_CMD_FLASH_UPDATE_STATUS
>> netlink message for use by a userland utility to show that
>> a particular firmware flash activity may take a long but
>> bounded time to finish.  Also add a handy helper for drivers
>> to make use of the new timeout value.
>>
>> UI usage hints:
>>   - if non-zero, add timeout display to the end of the status line
>>   	[component] status_msg  ( Xm Ys : Am Bs )
>>       using the timeout value for Am Bs and updating the Xm Ys
>>       every second
>>   - if the timeout expires while awaiting the next update,
>>     display something like
>>   	[component] status_msg  ( timeout reached : Am Bs )
>>   - if new status notify messages are received, remove
>>     the timeout and start over
>>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> Minor nits, otherwise LGTM:
>
> Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Thanks.


>
>> @@ -3052,6 +3054,9 @@ static int devlink_nl_flash_update_fill(struct sk_buff *msg,
>>   	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TOTAL,
>>   			      total, DEVLINK_ATTR_PAD))
>>   		goto nla_put_failure;
>> +	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TIMEOUT,
>> +			      timeout, DEVLINK_ATTR_PAD))
>> +		goto nla_put_failure;
> nit: since old kernels don't report this user space has to deal with it
>       not being present so I'd be tempted to only report it if timeout
>       is not 0

Hmmm... Unless there are any other comments on this, I think we can be 
consistent with the other fields here.

>
>> +void devlink_flash_update_timeout_notify(struct devlink *devlink,
>> +					 const char *status_msg,
>> +					 const char *component,
>> +					 unsigned long timeout)
>> +{
>> +	__devlink_flash_update_notify(devlink,
>> +				      DEVLINK_CMD_FLASH_UPDATE_STATUS,
>> +				      status_msg, component, 0, 0, timeout);
> nit: did we ever report cmd == UPDATE_STATUS and total == 0?
>       could this cause a division by zero in some unsuspecting
>       implementation? Perhaps we should pass 1 here?

Yes, there are several examples of this.  The userland devlink.c does a 
check for total == 0 before calculating the percentage.

sln

Re: [PATCH v4 net-next 2/5] devlink: collect flash notify params into a struct

[Shannon Nelson]

On 9/17/20 12:47 PM, Jakub Kicinski wrote:
> On Wed, 16 Sep 2020 20:02:01 -0700 Shannon Nelson wrote:
>> The dev flash status notify function parameter lists are getting
>> rather long, so add a struct to be filled and passed rather than
>> continuously changing the function signatures.
>>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
>> ---
>>   include/net/devlink.h | 21 ++++++++++++
>>   net/core/devlink.c    | 80 +++++++++++++++++++++++--------------------
>>   2 files changed, 63 insertions(+), 38 deletions(-)
>>
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index f206accf80ad..9ab2014885cb 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
>> @@ -391,6 +391,27 @@ struct devlink_param_gset_ctx {
>>   	enum devlink_param_cmode cmode;
>>   };
>>   
>> +/**
>> + * struct devlink_flash_notify - devlink dev flash notify data
>> + * @cmd: devlink notify command code
>> + * @status_msg: current status string
>> + * @component: firmware component being updated
>> + * @done: amount of work completed of total amount
>> + * @total: amount of work expected to be done
>> + * @timeout: expected max timeout in seconds
>> + *
>> + * These are values to be given to userland to be displayed in order
>> + * to show current activity in a firmware update process.
>> + */
>> +struct devlink_flash_notify {
>> +	enum devlink_command cmd;
> I'd leave out cmd out of the params structure, otherwise I'll be
> slightly awkward for drivers to fill in given the current helpers
> are per cmd.

Sure.  I wavered back and forth on that, no problem to change it.

sln

Re: [PATCH v4 net-next 3/5] netdevsim: devlink flash timeout message

[Shannon Nelson]

On 9/17/20 12:50 PM, Jakub Kicinski wrote:
> On Wed, 16 Sep 2020 20:02:02 -0700 Shannon Nelson wrote:
>> Add a simple devlink flash timeout message to exercise
>> the message mechanism.
>>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
>> ---
>>   drivers/net/netdevsim/dev.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>> index 32f339fedb21..4123550e3f6e 100644
>> --- a/drivers/net/netdevsim/dev.c
>> +++ b/drivers/net/netdevsim/dev.c
>> @@ -768,6 +768,8 @@ static int nsim_dev_flash_update(struct devlink *devlink, const char *file_name,
>>   						   component,
>>   						   NSIM_DEV_FLASH_SIZE,
>>   						   NSIM_DEV_FLASH_SIZE);
>> +		devlink_flash_update_timeout_notify(devlink, "Flash timeout",
>> +						    component, 81);
>>   		devlink_flash_update_status_notify(devlink, "Flashing done",
>>   						   component, 0, 0);
>>   		devlink_flash_update_end_notify(devlink);
> To mimic a more real scenario could we perhaps change the msg to "Flash
> select" ?
Sure.

sln

Re: [PATCH v4 net-next 5/5] ionic: add devlink firmware update

[Shannon Nelson]

On 9/17/20 12:52 PM, Jakub Kicinski wrote:
> On Wed, 16 Sep 2020 20:02:04 -0700 Shannon Nelson wrote:
>> Add support for firmware update through the devlink interface.
>> This update copies the firmware object into the device, asks
>> the current firmware to install it, then asks the firmware to
>> select the new firmware for the next boot-up.
>>
>> The install and select steps are launched as asynchronous
>> requests, which are then followed up with status request
>> commands.  These status request commands will be answered with
>> an EAGAIN return value and will try again until the request
>> has completed or reached the timeout specified.
>>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> Acked-by: Jakub Kicinski <kuba@kernel.org>

Thanks.

sln

Re: [PATCH v4 net-next 1/5] devlink: add timeout information to status_notify

[Shannon Nelson]

On 9/17/20 12:50 PM, Jacob Keller wrote:
> On 9/16/2020 8:02 PM, Shannon Nelson wrote:
>> Add a timeout element to the DEVLINK_CMD_FLASH_UPDATE_STATUS
>> netlink message for use by a userland utility to show that
>> a particular firmware flash activity may take a long but
>> bounded time to finish.  Also add a handy helper for drivers
>> to make use of the new timeout value.
>>
>> UI usage hints:
>>   - if non-zero, add timeout display to the end of the status line
>>   	[component] status_msg  ( Xm Ys : Am Bs )
>>       using the timeout value for Am Bs and updating the Xm Ys
>>       every second
>>   - if the timeout expires while awaiting the next update,
>>     display something like
>>   	[component] status_msg  ( timeout reached : Am Bs )
>>   - if new status notify messages are received, remove
>>     the timeout and start over
>>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
>> ---
> This one looks good to me. I think the only other things that I saw from
> previous discussion are:
>
> a) we could convert the internal helper devlink_nl_flash_update_fill and
> __devlink_flash_update_notify to use structs for their fields, and..
>
> b) Jakub pointed out that most drivers don't currently use the component
> field so we could remove that from the helpers.
>
> However, I don't have strong feelings about those either way, so:
>
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
>
>
Thanks,
sln