[PATCH v2 net-next 0/2] ionic: add devlink dev flash support

[PATCH v2 net-next 0/2] 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.  This is 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.

We don'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
don't yet have the API to fully support that.

v2: change "Activate" to "Select" in status messages

Shannon Nelson (2):
  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    | 195 ++++++++++++++++++
 .../net/ethernet/pensando/ionic/ionic_if.h    |  33 ++-
 .../net/ethernet/pensando/ionic/ionic_main.c  |  17 +-
 6 files changed, 251 insertions(+), 13 deletions(-)
 create mode 100644 drivers/net/ethernet/pensando/ionic/ionic_fw.c

-- 
2.17.1

[PATCH v2 net-next 1/2] 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 2b72a51be1d0..f1fd9a98ae4a 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 v2 net-next 2/2] 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    | 195 ++++++++++++++++++
 .../net/ethernet/pensando/ionic/ionic_main.c  |  13 +-
 5 files changed, 222 insertions(+), 5 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..e8886893227a
--- /dev/null
+++ b/drivers/net/ethernet/pensando/ionic/ionic_fw.c
@@ -0,0 +1,195 @@
+// 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_ACTIVATE_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_install_status(struct ionic_dev *idev)
+{
+	union ionic_dev_cmd cmd = {
+		.fw_control.opcode = IONIC_CMD_FW_CONTROL,
+		.fw_control.oper = IONIC_FW_INSTALL_STATUS
+	};
+
+	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 void ionic_dev_cmd_firmware_activate_status(struct ionic_dev *idev)
+{
+	union ionic_dev_cmd cmd = {
+		.fw_control.opcode = IONIC_CMD_FW_CONTROL,
+		.fw_control.oper = IONIC_FW_ACTIVATE_STATUS,
+	};
+
+	ionic_dev_cmd_go(idev, &cmd);
+}
+
+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));
+
+	devlink_flash_update_status_notify(dl, "Downloading", NULL, 0, fw->size);
+	offset = 0;
+	next_interval = fw->size / IONIC_FW_INTERVAL_FRACTION;
+	while (offset < fw->size) {
+		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;
+
+		if (offset > next_interval) {
+			devlink_flash_update_status_notify(dl, "Downloading",
+							   NULL, offset, fw->size);
+			next_interval = offset + (fw->size / IONIC_FW_INTERVAL_FRACTION);
+		}
+	}
+	devlink_flash_update_status_notify(dl, "Downloading", NULL, 1, 1);
+
+	devlink_flash_update_status_notify(dl, "Installing", NULL, 0, 2);
+
+	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;
+	}
+
+	devlink_flash_update_status_notify(dl, "Installing", NULL, 1, 2);
+	mutex_lock(&ionic->dev_cmd_lock);
+	ionic_dev_cmd_firmware_install_status(idev);
+	err = ionic_dev_cmd_wait(ionic, IONIC_FW_INSTALL_TIMEOUT);
+	mutex_unlock(&ionic->dev_cmd_lock);
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Firmware install failed");
+		goto err_out;
+	}
+	devlink_flash_update_status_notify(dl, "Installing", NULL, 2, 2);
+
+	devlink_flash_update_status_notify(dl, "Selecting", NULL, 0, 2);
+
+	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;
+	}
+
+	devlink_flash_update_status_notify(dl, "Selecting", NULL, 1, 2);
+	mutex_lock(&ionic->dev_cmd_lock);
+	ionic_dev_cmd_firmware_activate_status(idev);
+	err = ionic_dev_cmd_wait(ionic, IONIC_FW_ACTIVATE_TIMEOUT);
+	mutex_unlock(&ionic->dev_cmd_lock);
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Firmware select failed");
+		goto err_out;
+	}
+	devlink_flash_update_status_notify(dl, "Selecting", NULL, 2, 2);
+
+	netdev_info(netdev, "Firmware update completed\n");
+
+err_out:
+	if (err)
+		devlink_flash_update_status_notify(dl, "Flash failed", 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 f1fd9a98ae4a..4b4ff885ebf8 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_main.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_main.c
@@ -361,17 +361,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);
@@ -396,7 +401,7 @@ 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",
+			dev_dbg(ionic->dev, "DEV_CMD %s (%d), %s (%d) retrying...\n",
 				ionic_opcode_to_str(opcode), opcode,
 				ionic_error_to_str(err), err);
 
-- 
2.17.1

Re: [PATCH v2 net-next 2/2] ionic: add devlink firmware update

[Jakub Kicinski]

On Thu,  3 Sep 2020 17:05:34 -0700 Shannon Nelson wrote:
> +	devlink_flash_update_status_notify(dl, "Downloading", NULL, 0, fw->size);
> +	offset = 0;
> +	next_interval = fw->size / IONIC_FW_INTERVAL_FRACTION;
> +	while (offset < fw->size) {
> +		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;
> +
> +		if (offset > next_interval) {
> +			devlink_flash_update_status_notify(dl, "Downloading",
> +							   NULL, offset, fw->size);
> +			next_interval = offset + (fw->size / IONIC_FW_INTERVAL_FRACTION);
> +		}
> +	}
> +	devlink_flash_update_status_notify(dl, "Downloading", NULL, 1, 1);

This is quite awkward. You send a notification with a different size,
and potentially an unnecessary one if last iteration of the loop
triggered offset > next_interval.

Please just add || offset == fw->size to the condition at the end of
the loop and it will always trigger, with the correct length.

Re: [PATCH v2 net-next 2/2] ionic: add devlink firmware update

[Shannon Nelson]

On 9/5/20 12:52 PM, Jakub Kicinski wrote:
> On Thu,  3 Sep 2020 17:05:34 -0700 Shannon Nelson wrote:
>> +	devlink_flash_update_status_notify(dl, "Downloading", NULL, 0, fw->size);
>> +	offset = 0;
>> +	next_interval = fw->size / IONIC_FW_INTERVAL_FRACTION;
>> +	while (offset < fw->size) {
>> +		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;
>> +
>> +		if (offset > next_interval) {
>> +			devlink_flash_update_status_notify(dl, "Downloading",
>> +							   NULL, offset, fw->size);
>> +			next_interval = offset + (fw->size / IONIC_FW_INTERVAL_FRACTION);
>> +		}
>> +	}
>> +	devlink_flash_update_status_notify(dl, "Downloading", NULL, 1, 1);
> This is quite awkward. You send a notification with a different size,
> and potentially an unnecessary one if last iteration of the loop
> triggered offset > next_interval.
>
> Please just add || offset == fw->size to the condition at the end of
> the loop and it will always trigger, with the correct length.

Or maybe make that last one look like
     devlink_flash_update_status_notify(dl, "Downloading", NULL, 
fw->size, fw->size);
to be less awkward and to keep the style of using a final status_notify 
at the end of the block, as done in the Install and Select blocks 
further along?

sln

Re: [PATCH v2 net-next 2/2] ionic: add devlink firmware update

[Jakub Kicinski]

On Sat, 5 Sep 2020 14:47:58 -0700 Shannon Nelson wrote:
> On 9/5/20 12:52 PM, Jakub Kicinski wrote:
> > On Thu,  3 Sep 2020 17:05:34 -0700 Shannon Nelson wrote:  
> >> +		if (offset > next_interval) {
> >> +			devlink_flash_update_status_notify(dl, "Downloading",
> >> +							   NULL, offset, fw->size);
> >> +			next_interval = offset + (fw->size / IONIC_FW_INTERVAL_FRACTION);
> >> +		}
> >> +	}
> >> +	devlink_flash_update_status_notify(dl, "Downloading", NULL, 1, 1);  
> > This is quite awkward. You send a notification with a different size,
> > and potentially an unnecessary one if last iteration of the loop
> > triggered offset > next_interval.
> >
> > Please just add || offset == fw->size to the condition at the end of
> > the loop and it will always trigger, with the correct length.  
> 
> Or maybe make that last one look like
>      devlink_flash_update_status_notify(dl, "Downloading", NULL, 
> fw->size, fw->size);
> to be less awkward and to keep the style of using a final status_notify 
> at the end of the block, as done in the Install and Select blocks 
> further along?
 
That may still generate two notifications at the end tho, no?
Unless the loop one in the loop is && offset != fw->size.

Re: [PATCH v2 net-next 2/2] ionic: add devlink firmware update

[Jakub Kicinski]

On Thu,  3 Sep 2020 17:05:34 -0700 Shannon Nelson wrote:
> +/* 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.

25 minutes is really quite scary. And user will see no notification
other than "Installing 50%" for 25 minutes? And will most likely not 
be able to do anything that'd involve talking to the FW, like setting
port info/speed?

Is it possible for the FW to inform that it's updating the CPLD?

Can you release the dev_cmd_lock periodically to allow other commands
to be executed while waiting?

Re: [PATCH v2 net-next 2/2] ionic: add devlink firmware update

[Shannon Nelson]

On 9/5/20 1:04 PM, Jakub Kicinski wrote:
> On Thu,  3 Sep 2020 17:05:34 -0700 Shannon Nelson wrote:
>> +/* 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.
> 25 minutes is really quite scary. And user will see no notification
> other than "Installing 50%" for 25 minutes? And will most likely not
> be able to do anything that'd involve talking to the FW, like setting
> port info/speed?

Yeah, it's pretty annoying, and the READMEs with the FW will need to 
warn that the install time will be much longer than usual.

>
> Is it possible for the FW to inform that it's updating the CPLD?

We don't have any useful feedback mechanism for this kind of thing, but 
I'll think about how it might work and see if I can get something from 
the FW folks.  Another option would be for the driver to learn how to 
read the FW blob, but I'd really rather not go down that road.

>
> Can you release the dev_cmd_lock periodically to allow other commands
> to be executed while waiting?

I think this could be done.  I suspect I'll need to give the dev_cmd the 
regular timeout and have this routine manage the longer potential 
timeout.  I'll likely have to mess with the low-level dev_cmd_wait to 
not complain about a timeout when it is a FW status command.

The status_notify messages could then be updated in order to show some 
progress, but would we base the 100% on the remote possibility that it 
might take 25 minutes?  Or use some scaled update time, taking longer 
between updates as time goes on? Hmmm...

I'll think about this over the long weekend.

Thanks,
sln

Re: [PATCH v2 net-next 2/2] ionic: add devlink firmware update

[Jakub Kicinski]

On Sat, 5 Sep 2020 15:06:07 -0700 Shannon Nelson wrote:
> On 9/5/20 1:04 PM, Jakub Kicinski wrote:
> > On Thu,  3 Sep 2020 17:05:34 -0700 Shannon Nelson wrote:  
> >> +/* 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.  
> > 25 minutes is really quite scary. And user will see no notification
> > other than "Installing 50%" for 25 minutes? And will most likely not
> > be able to do anything that'd involve talking to the FW, like setting
> > port info/speed?  
> 
> Yeah, it's pretty annoying, and the READMEs with the FW will need to 
> warn that the install time will be much longer than usual.
> 
> > Is it possible for the FW to inform that it's updating the CPLD?  
> 
> We don't have any useful feedback mechanism for this kind of thing, but 
> I'll think about how it might work and see if I can get something from 
> the FW folks.  Another option would be for the driver to learn how to 
> read the FW blob, but I'd really rather not go down that road.

Yes, parsing the firmware blobs in the drivers in not advisable.

> > Can you release the dev_cmd_lock periodically to allow other commands
> > to be executed while waiting?  
> 
> I think this could be done.  I suspect I'll need to give the dev_cmd the 
> regular timeout and have this routine manage the longer potential 
> timeout.  I'll likely have to mess with the low-level dev_cmd_wait to 
> not complain about a timeout when it is a FW status command.
> 
> The status_notify messages could then be updated in order to show some 
> progress, but would we base the 100% on the remote possibility that it 
> might take 25 minutes?  Or use some scaled update time, taking longer 
> between updates as time goes on? Hmmm...

I wonder if we can steal a page from systemd's book and display
"time until timeout", or whatchamacallit, like systemd does when it's
waiting for processes to quit. All drivers have some timeout set on the
operation. If users knew the driver sets timeout to n minutes and they
see the timer ticking up they'd be less likely to think the command has
hanged..

Re: [PATCH v2 net-next 0/2] ionic: add devlink dev flash support

[Jakub Kicinski]

On Thu,  3 Sep 2020 17:05:32 -0700 Shannon Nelson wrote:
> Add support for using devlink's dev flash facility to update the
> firmware on an ionic device.  This is 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.
> 
> We don'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
> don't yet have the API to fully support that.
> 
> v2: change "Activate" to "Select" in status messages

Thanks!

Slightly off-topic for these patches but every new C file in ionic adds
a set of those warnings (from sparse, I think, because it still builds):

../drivers/net/ethernet/pensando/ionic/ionic_debugfs.c: note: in included file (through ../drivers/net/ethernet/pensando/ionic/ionic.h):
../drivers/net/ethernet/pensando/ionic/ionic_dev.h:37:1: error: static assertion failed: "sizeof(union ionic_dev_regs) == 4096"
../drivers/net/ethernet/pensando/ionic/ionic_dev.h:39:1: error: static assertion failed: "sizeof(union ionic_dev_cmd_regs) == 2048"
../drivers/net/ethernet/pensando/ionic/ionic_dev.h:55:1: error: static assertion failed: "sizeof(struct ionic_dev_getattr_comp) == 16"
../drivers/net/ethernet/pensando/ionic/ionic_dev.h:56:1: error: static assertion failed: "sizeof(struct ionic_dev_setattr_cmd) == 64"
../drivers/net/ethernet/pensando/ionic/ionic_dev.h:57:1: error: static assertion failed: "sizeof(struct ionic_dev_setattr_comp) == 16"
../drivers/net/ethernet/pensando/ionic/ionic_dev.h:67:1: error: static assertion failed: "sizeof(struct ionic_port_getattr_comp) == 16"
../drivers/net/ethernet/pensando/ionic/ionic_dev.h:77:1: error: static assertion failed: "sizeof(struct ionic_lif_getattr_comp) == 16"
../drivers/net/ethernet/pensando/ionic/ionic_dev.h:78:1: error: static assertion failed: "sizeof(struct ionic_lif_setattr_cmd) == 64"
../drivers/net/ethernet/pensando/ionic/ionic_dev.h:79:1: error: static assertion failed: "sizeof(struct ionic_lif_setattr_comp) == 16"
../drivers/net/ethernet/pensando/ionic/ionic_dev.h:81:1: error: static assertion failed: "sizeof(struct ionic_q_init_cmd) == 64"
../drivers/net/ethernet/pensando/ionic/ionic_dev.h:118:1: error: static assertion failed: "sizeof(struct ionic_vf_setattr_cmd) == 64"
../drivers/net/ethernet/pensando/ionic/ionic_dev.h:121:1: error: static assertion failed: "sizeof(struct ionic_vf_getattr_comp) == 16"

Any ideas on why?

Re: [PATCH v2 net-next 0/2] ionic: add devlink dev flash support

[Shannon Nelson]

On 9/4/20 8:01 AM, Jakub Kicinski wrote:
> On Thu,  3 Sep 2020 17:05:32 -0700 Shannon Nelson wrote:
>> Add support for using devlink's dev flash facility to update the
>> firmware on an ionic device.  This is 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.
>>
>> We don'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
>> don't yet have the API to fully support that.
>>
>> v2: change "Activate" to "Select" in status messages
> Thanks!
>
> Slightly off-topic for these patches but every new C file in ionic adds
> a set of those warnings (from sparse, I think, because it still builds):
>
> ../drivers/net/ethernet/pensando/ionic/ionic_debugfs.c: note: in included file (through ../drivers/net/ethernet/pensando/ionic/ionic.h):
> ../drivers/net/ethernet/pensando/ionic/ionic_dev.h:37:1: error: static assertion failed: "sizeof(union ionic_dev_regs) == 4096"
> ../drivers/net/ethernet/pensando/ionic/ionic_dev.h:39:1: error: static assertion failed: "sizeof(union ionic_dev_cmd_regs) == 2048"
> ../drivers/net/ethernet/pensando/ionic/ionic_dev.h:55:1: error: static assertion failed: "sizeof(struct ionic_dev_getattr_comp) == 16"
> ../drivers/net/ethernet/pensando/ionic/ionic_dev.h:56:1: error: static assertion failed: "sizeof(struct ionic_dev_setattr_cmd) == 64"
> ../drivers/net/ethernet/pensando/ionic/ionic_dev.h:57:1: error: static assertion failed: "sizeof(struct ionic_dev_setattr_comp) == 16"
> ../drivers/net/ethernet/pensando/ionic/ionic_dev.h:67:1: error: static assertion failed: "sizeof(struct ionic_port_getattr_comp) == 16"
> ../drivers/net/ethernet/pensando/ionic/ionic_dev.h:77:1: error: static assertion failed: "sizeof(struct ionic_lif_getattr_comp) == 16"
> ../drivers/net/ethernet/pensando/ionic/ionic_dev.h:78:1: error: static assertion failed: "sizeof(struct ionic_lif_setattr_cmd) == 64"
> ../drivers/net/ethernet/pensando/ionic/ionic_dev.h:79:1: error: static assertion failed: "sizeof(struct ionic_lif_setattr_comp) == 16"
> ../drivers/net/ethernet/pensando/ionic/ionic_dev.h:81:1: error: static assertion failed: "sizeof(struct ionic_q_init_cmd) == 64"
> ../drivers/net/ethernet/pensando/ionic/ionic_dev.h:118:1: error: static assertion failed: "sizeof(struct ionic_vf_setattr_cmd) == 64"
> ../drivers/net/ethernet/pensando/ionic/ionic_dev.h:121:1: error: static assertion failed: "sizeof(struct ionic_vf_getattr_comp) == 16"
>
> Any ideas on why?

It's probably related to this discussion:
https://lore.kernel.org/linux-sparse/ecdd10cb-0022-8f8a-ec36-9d51b3ae85ee@pensando.io/

I thought we'd worked out our struct alignment issues, but I'll see if I 
can carve out some time to take another look at that.

sln

Re: [PATCH v2 net-next 0/2] ionic: add devlink dev flash support

[Jakub Kicinski]

On Fri, 4 Sep 2020 11:20:11 -0700 Shannon Nelson wrote:
> It's probably related to this discussion:
> https://lore.kernel.org/linux-sparse/ecdd10cb-0022-8f8a-ec36-9d51b3ae85ee@pensando.io/
> 
> I thought we'd worked out our struct alignment issues, but I'll see if I 
> can carve out some time to take another look at that.

Cool, could perhaps be something with union handling in sprase I see
that there were some changes in sparse.git recently regarding unions -
maybe it's fixed already.

Re: [PATCH v2 net-next 0/2] ionic: add devlink dev flash support

[Shannon Nelson]

On 9/4/20 3:47 PM, Jakub Kicinski wrote:
> On Fri, 4 Sep 2020 11:20:11 -0700 Shannon Nelson wrote:
>> It's probably related to this discussion:
>> https://lore.kernel.org/linux-sparse/ecdd10cb-0022-8f8a-ec36-9d51b3ae85ee@pensando.io/
>>
>> I thought we'd worked out our struct alignment issues, but I'll see if I
>> can carve out some time to take another look at that.
> Cool, could perhaps be something with union handling in sprase I see
> that there were some changes in sparse.git recently regarding unions -
> maybe it's fixed already.

Probably not fixed for ionic, yet.  I just pulled down the latest(?) 
sparse 0.6.2 and see the same issues.  I can't get to it right now, but 
thanks for letting me know about it.

sln