On Wed, Nov 18, 2020 at 1:41 AM Jacob Keller <jacob.e.kel...@intel.com> wrote:
>
> When performing a flash update via devlink, device drivers may inform
> user space of status updates via
> devlink_flash_update_(begin|end|timeout|status)_notify functions.
>
> It is expected that drivers do not send any status notifications unless
> they send a begin and end message. If a driver sends a status
> notification without sending the appropriate end notification upon
> finishing (regardless of success or failure), the current implementation
> of the devlink userspace program can get stuck endlessly waiting for the
> end notification that will never come.
>
> The current ice driver implementation may send such a status message
> without the appropriate end notification in rare cases.
>
> Fixing the ice driver is relatively simple: we just need to send the
> begin_notify at the start of the function and always send an end_notify
> no matter how the function exits.
>
> Rather than assuming driver authors will always get this right in the
> future, lets just fix the API so that it is not possible to get wrong.
> Make devlink_flash_update_begin_notify and
> devlink_flash_update_end_notify static, and call them in devlink.c core
> code. Always send the begin_notify just before calling the driver's
> flash_update routine. Always send the end_notify just after the routine
> returns regardless of success or failure.
>
> Doing this makes the status notification easier to use from the driver,
> as it no longer needs to worry about catching failures and cleaning up
> by calling devlink_flash_update_end_notify. It is now no longer possible
> to do the wrong thing in this regard. We also save a couple of lines of
> code in each driver.
>
> Signed-off-by: Jacob Keller <jacob.e.kel...@intel.com>
> Cc: Jiri Pirko <j...@nvidia.com>
> Cc: Michael Chan <michael.c...@broadcom.com>
> Cc: Shannon Nelson <snel...@pensando.io>
> Cc: Saeed Mahameed <sae...@nvidia.com>
> Cc: Boris Pismenny <bor...@nvidia.com>
> Cc: Bin Luo <luob...@huawei.com>
> Cc: Jakub Kicinksi <k...@kernel.org>
> ---
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 2 --
> drivers/net/ethernet/intel/ice/ice_devlink.c | 5 +----
> drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c | 3 ---
> drivers/net/ethernet/pensando/ionic/ionic_fw.c | 2 --
> drivers/net/netdevsim/dev.c | 2 --
> include/net/devlink.h | 2 --
> net/core/devlink.c | 10 ++++++----
> 7 files changed, 7 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> index 4ebae8a236fd..6b7b69ed62db 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> @@ -30,14 +30,12 @@ bnxt_dl_flash_update(struct devlink *dl,
> return -EPERM;
> }
>
> - devlink_flash_update_begin_notify(dl);
> devlink_flash_update_status_notify(dl, "Preparing to flash", NULL, 0,
> 0);
> rc = bnxt_flash_package_from_fw_obj(bp->dev, params->fw, 0);
> if (!rc)
> devlink_flash_update_status_notify(dl, "Flashing done", NULL,
> 0, 0);
> else
> devlink_flash_update_status_notify(dl, "Flashing failed",
> NULL, 0, 0);
> - devlink_flash_update_end_notify(dl);
> return rc;
> }
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c
> b/drivers/net/ethernet/intel/ice/ice_devlink.c
> index 0036d3e7df0b..29d6192b15f3 100644
> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -275,12 +275,9 @@ ice_devlink_flash_update(struct devlink *devlink,
> if (err)
> return err;
>
> - devlink_flash_update_begin_notify(devlink);
> devlink_flash_update_status_notify(devlink, "Preparing to flash",
> NULL, 0, 0);
> - err = ice_flash_pldm_image(pf, params->fw, preservation, extack);
> - devlink_flash_update_end_notify(devlink);
>
> - return err;
> + return ice_flash_pldm_image(pf, params->fw, preservation, extack);
> }
>
> static const struct devlink_ops ice_devlink_ops = {
> diff --git a/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
> b/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
> index bcd166911d44..46245e0b2462 100644
> --- a/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
> +++ b/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
> @@ -368,7 +368,6 @@ int mlxfw_firmware_flash(struct mlxfw_dev *mlxfw_dev,
> }
>
> mlxfw_info(mlxfw_dev, "Initialize firmware flash process\n");
> - devlink_flash_update_begin_notify(mlxfw_dev->devlink);
> mlxfw_status_notify(mlxfw_dev, "Initializing firmware flash process",
> NULL, 0, 0);
> err = mlxfw_dev->ops->fsm_lock(mlxfw_dev, &fwhandle);
> @@ -417,7 +416,6 @@ int mlxfw_firmware_flash(struct mlxfw_dev *mlxfw_dev,
> mlxfw_info(mlxfw_dev, "Firmware flash done\n");
> mlxfw_status_notify(mlxfw_dev, "Firmware flash done", NULL, 0, 0);
> mlxfw_mfa2_file_fini(mfa2_file);
> - devlink_flash_update_end_notify(mlxfw_dev->devlink);
> return 0;
>
> err_state_wait_activate_to_locked:
> @@ -429,7 +427,6 @@ int mlxfw_firmware_flash(struct mlxfw_dev *mlxfw_dev,
> mlxfw_dev->ops->fsm_release(mlxfw_dev, fwhandle);
> err_fsm_lock:
> mlxfw_mfa2_file_fini(mfa2_file);
> - devlink_flash_update_end_notify(mlxfw_dev->devlink);
> return err;
> }
> EXPORT_SYMBOL(mlxfw_firmware_flash);
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_fw.c
> b/drivers/net/ethernet/pensando/ionic/ionic_fw.c
> index fd2ce134f66c..4be7e932b7eb 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_fw.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_fw.c
> @@ -105,7 +105,6 @@ int ionic_firmware_update(struct ionic_lif *lif, const
> struct firmware *fw,
> u8 fw_slot;
>
> dl = priv_to_devlink(ionic);
> - devlink_flash_update_begin_notify(dl);
> devlink_flash_update_status_notify(dl, "Preparing to flash", NULL, 0,
> 0);
>
> buf_sz = sizeof(idev->dev_cmd_regs->data);
> @@ -191,6 +190,5 @@ int ionic_firmware_update(struct ionic_lif *lif, const
> struct firmware *fw,
> devlink_flash_update_status_notify(dl, "Flash failed", NULL,
> 0, 0);
> else
> devlink_flash_update_status_notify(dl, "Flash done", NULL, 0,
> 0);
> - devlink_flash_update_end_notify(dl);
> return err;
> }
> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
> index 49cc1fea9e02..5731d8b6566b 100644
> --- a/drivers/net/netdevsim/dev.c
> +++ b/drivers/net/netdevsim/dev.c
> @@ -764,7 +764,6 @@ static int nsim_dev_flash_update(struct devlink *devlink,
> return -EOPNOTSUPP;
>
> if (nsim_dev->fw_update_status) {
> - devlink_flash_update_begin_notify(devlink);
> devlink_flash_update_status_notify(devlink,
> "Preparing to flash",
> params->component, 0, 0);
> @@ -788,7 +787,6 @@ static int nsim_dev_flash_update(struct devlink *devlink,
> params->component, 81);
> devlink_flash_update_status_notify(devlink, "Flashing done",
> params->component, 0, 0);
> - devlink_flash_update_end_notify(devlink);
> }
>
> return 0;
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index d1d125a33322..457c537d0ef2 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -1577,8 +1577,6 @@ void devlink_remote_reload_actions_performed(struct
> devlink *devlink,
> enum devlink_reload_limit limit,
> u32 actions_performed);
>
> -void devlink_flash_update_begin_notify(struct devlink *devlink);
> -void devlink_flash_update_end_notify(struct devlink *devlink);
> void devlink_flash_update_status_notify(struct devlink *devlink,
> const char *status_msg,
> const char *component,
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index b0121d79ac75..bf160d9b1106 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -3370,7 +3370,7 @@ static void __devlink_flash_update_notify(struct
> devlink *devlink,
> nlmsg_free(msg);
> }
>
> -void devlink_flash_update_begin_notify(struct devlink *devlink)
> +static void devlink_flash_update_begin_notify(struct devlink *devlink)
> {
> struct devlink_flash_notify params = { 0 };
>
> @@ -3378,9 +3378,8 @@ void devlink_flash_update_begin_notify(struct devlink
> *devlink)
> DEVLINK_CMD_FLASH_UPDATE,
> ¶ms);
> }
> -EXPORT_SYMBOL_GPL(devlink_flash_update_begin_notify);
>
> -void devlink_flash_update_end_notify(struct devlink *devlink)
> +static void devlink_flash_update_end_notify(struct devlink *devlink)
> {
> struct devlink_flash_notify params = { 0 };
>
> @@ -3388,7 +3387,6 @@ void devlink_flash_update_end_notify(struct devlink
> *devlink)
> DEVLINK_CMD_FLASH_UPDATE_END,
> ¶ms);
> }
> -EXPORT_SYMBOL_GPL(devlink_flash_update_end_notify);
>
> void devlink_flash_update_status_notify(struct devlink *devlink,
> const char *status_msg,
> @@ -3475,7 +3473,9 @@ static int devlink_nl_cmd_flash_update(struct sk_buff
> *skb,
> return ret;
> }
>
> + devlink_flash_update_begin_notify(devlink);
> ret = devlink->ops->flash_update(devlink, ¶ms, info->extack);
> + devlink_flash_update_end_notify(devlink);
>
> release_firmware(params.fw);
>
> @@ -10242,7 +10242,9 @@ int devlink_compat_flash_update(struct net_device
> *dev, const char *file_name)
> goto out;
>
> mutex_lock(&devlink->lock);
> + devlink_flash_update_begin_notify(devlink);
> ret = devlink->ops->flash_update(devlink, ¶ms, NULL);
> + devlink_flash_update_end_notify(devlink);
> mutex_unlock(&devlink->lock);
>
> release_firmware(params.fw);
> --
> 2.29.0
>
Acked-by: Vasundhara Volam <vasundhara-v.vo...@broadcom.com>
smime.p7s
Description: S/MIME Cryptographic Signature
