| Message ID | 20200904000534.58052-3-snelson@pensando.io |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | ionic: add devlink dev flash support | expand |
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.
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?
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
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
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.
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..
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);
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