netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [net-next v3 0/2] devlink: move common flash_update calls to core
@ 2020-11-17 20:08 Jacob Keller
  2020-11-17 20:08 ` [net-next v3 1/2] devlink: move request_firmware out of driver Jacob Keller
  2020-11-17 20:08 ` [net-next v3 2/2] devlink: move flash end and begin to core devlink Jacob Keller
  0 siblings, 2 replies; 10+ messages in thread
From: Jacob Keller @ 2020-11-17 20:08 UTC (permalink / raw)
  To: netdev
  Cc: Jacob Keller, Jiri Pirko, Michael Chan, Shannon Nelson,
	Saeed Mahameed, Boris Pismenny, Bin Luo, Jakub Kicinksi

This series moves a couple common pieces done by all drivers of the
->flash_update interface into devlink.c flash update handler. Specifically,
the core code will now request_firmware and
devlink_flash_update_(begin|end)_notify.

This cleanup is intended to simplify driver implementations so that they
have less work to do and are less capable of doing the "wrong" thing.

For request_firmware, this simplification is done as it is not expected that
drivers would do anything else. It also standardizes all drivers so that
they use the same interface (request_firmware, as opposed to
request_firmware_direct), and allows reporting the netlink extended ack with
the file name attribute.

For status notification, this change prevents drivers from sending a status
message without properly sending the status end notification. The current
userspace implementation of devlink relies on this end notification to
properly close the flash update channel. Without this, the flash update
process may hang indefinitely. By moving the begin and end calls into the
core code, it is no longer possible for a driver author to get this wrong.

For the original patch that moved request_firmware, see [1]. For the v2 see
[2]. For further discussion of the issues with devlink flash status see [3].

Cc: Jiri Pirko <jiri@nvidia.com>
Cc: Michael Chan <michael.chan@broadcom.com>
Cc: Shannon Nelson <snelson@pensando.io>
Cc: Saeed Mahameed <saeedm@nvidia.com>
Cc: Boris Pismenny <borisp@nvidia.com>
Cc: Bin Luo <luobin9@huawei.com>
Cc: Jakub Kicinksi <kuba@kernel.org>

[1] https://lore.kernel.org/netdev/20201113000142.3563690-1-jacob.e.keller@intel.com/
[2] https://lore.kernel.org/netdev/20201113224559.3910864-1-jacob.e.keller@intel.com/
[3] https://lore.kernel.org/netdev/6352e9d3-02af-721e-3a54-ef99a666be29@intel.com/


Jacob Keller (2):
  devlink: move request_firmware out of driver
  devlink: move flash end and begin to core devlink

 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c |  4 +--
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 33 +++++++++++------
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.h |  4 +--
 .../net/ethernet/huawei/hinic/hinic_devlink.c | 12 +------
 drivers/net/ethernet/intel/ice/ice_devlink.c  | 17 +--------
 .../net/ethernet/mellanox/mlx5/core/devlink.c | 11 +-----
 .../net/ethernet/mellanox/mlxfw/mlxfw_fsm.c   |  3 --
 drivers/net/ethernet/mellanox/mlxsw/core.c    | 11 +-----
 .../net/ethernet/netronome/nfp/nfp_devlink.c  |  2 +-
 drivers/net/ethernet/netronome/nfp/nfp_main.c | 17 ++-------
 drivers/net/ethernet/netronome/nfp/nfp_main.h |  2 +-
 .../ethernet/pensando/ionic/ionic_devlink.c   |  2 +-
 .../ethernet/pensando/ionic/ionic_devlink.h   |  2 +-
 .../net/ethernet/pensando/ionic/ionic_fw.c    | 14 +-------
 drivers/net/netdevsim/dev.c                   |  2 --
 include/net/devlink.h                         |  9 +++--
 net/core/devlink.c                            | 36 ++++++++++++++-----
 17 files changed, 67 insertions(+), 114 deletions(-)


base-commit: 83c317d7b36bb3858cf1cb86d2635ec3f3bd6ea3
-- 
2.29.0


^ permalink raw reply	[flat|nested] 10+ messages in thread
* [net-next v3 1/2] devlink: move request_firmware out of driver
  2020-11-17 20:08 [net-next v3 0/2] devlink: move common flash_update calls to core Jacob Keller
@ 2020-11-17 20:08 ` Jacob Keller
  2020-11-17 20:10   ` Jacob Keller
  2020-11-18 16:38   ` Jiri Pirko
  2020-11-17 20:08 ` [net-next v3 2/2] devlink: move flash end and begin to core devlink Jacob Keller
  1 sibling, 2 replies; 10+ messages in thread
From: Jacob Keller @ 2020-11-17 20:08 UTC (permalink / raw)
  To: netdev
  Cc: Jacob Keller, Jiri Pirko, Michael Chan, Shannon Nelson,
	Saeed Mahameed, Boris Pismenny, Bin Luo, Jakub Kicinksi

All drivers which implement the devlink flash update support, with the
exception of netdevsim, use either request_firmware or
request_firmware_direct to locate the firmware file. Rather than having
each driver do this separately as part of its .flash_update
implementation, perform the request_firmware within net/core/devlink.c

Replace the file_name parameter in the struct devlink_flash_update_params
with a pointer to the fw object.

Use request_firmware rather than request_firmware_direct. Although most
Linux distributions today do not have the fallback mechanism
implemented, only about half the drivers used the _direct request, as
compared to the generic request_firmware. In the event that
a distribution does support the fallback mechanism, the devlink flash
update ought to be able to use it to provide the firmware contents. For
distributions which do not support the fallback userspace mechanism,
there should be essentially no difference between request_firmware and
request_firmware_direct.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Jiri Pirko <jiri@nvidia.com>
Cc: Michael Chan <michael.chan@broadcom.com>
Cc: Shannon Nelson <snelson@pensando.io>
Cc: Saeed Mahameed <saeedm@nvidia.com>
Cc: Boris Pismenny <borisp@nvidia.com>
Cc: Bin Luo <luobin9@huawei.com>
Cc: Jakub Kicinksi <kuba@kernel.org>
---
 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c |  2 +-
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 33 ++++++++++++-------
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.h |  4 +--
 .../net/ethernet/huawei/hinic/hinic_devlink.c | 12 +------
 drivers/net/ethernet/intel/ice/ice_devlink.c  | 14 +-------
 .../net/ethernet/mellanox/mlx5/core/devlink.c | 11 +------
 drivers/net/ethernet/mellanox/mlxsw/core.c    | 11 +------
 .../net/ethernet/netronome/nfp/nfp_devlink.c  |  2 +-
 drivers/net/ethernet/netronome/nfp/nfp_main.c | 17 ++--------
 drivers/net/ethernet/netronome/nfp/nfp_main.h |  2 +-
 .../ethernet/pensando/ionic/ionic_devlink.c   |  2 +-
 .../ethernet/pensando/ionic/ionic_devlink.h   |  2 +-
 .../net/ethernet/pensando/ionic/ionic_fw.c    | 12 +------
 include/net/devlink.h                         |  7 ++--
 net/core/devlink.c                            | 26 ++++++++++++---
 15 files changed, 61 insertions(+), 96 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 184b6d0513b2..4ebae8a236fd 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -32,7 +32,7 @@ bnxt_dl_flash_update(struct devlink *dl,
 
 	devlink_flash_update_begin_notify(dl);
 	devlink_flash_update_status_notify(dl, "Preparing to flash", NULL, 0, 0);
-	rc = bnxt_flash_package_from_file(bp->dev, params->file_name, 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
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 53687bc7fcf5..91e73aedcdff 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -2416,13 +2416,12 @@ static int bnxt_flash_firmware_from_file(struct net_device *dev,
 	return rc;
 }
 
-int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
-				 u32 install_type)
+int bnxt_flash_package_from_fw_obj(struct net_device *dev, const struct firmware *fw,
+				   u32 install_type)
 {
 	struct bnxt *bp = netdev_priv(dev);
 	struct hwrm_nvm_install_update_output *resp = bp->hwrm_cmd_resp_addr;
 	struct hwrm_nvm_install_update_input install = {0};
-	const struct firmware *fw;
 	u32 item_len;
 	int rc = 0;
 	u16 index;
@@ -2437,13 +2436,6 @@ int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
 		return rc;
 	}
 
-	rc = request_firmware(&fw, filename, &dev->dev);
-	if (rc != 0) {
-		netdev_err(dev, "PKG error %d requesting file: %s\n",
-			   rc, filename);
-		return rc;
-	}
-
 	if (fw->size > item_len) {
 		netdev_err(dev, "PKG insufficient update area in nvram: %lu\n",
 			   (unsigned long)fw->size);
@@ -2475,7 +2467,6 @@ int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
 					  dma_handle);
 		}
 	}
-	release_firmware(fw);
 	if (rc)
 		goto err_exit;
 
@@ -2514,6 +2505,26 @@ int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
 	return rc;
 }
 
+static int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
+					u32 install_type)
+{
+	const struct firmware *fw;
+	int rc;
+
+	rc = request_firmware(&fw, filename, &dev->dev);
+	if (rc != 0) {
+		netdev_err(dev, "PKG error %d requesting file: %s\n",
+			   rc, filename);
+		return rc;
+	}
+
+	rc = bnxt_flash_package_from_fw_obj(dev, fw, install_type);
+
+	release_firmware(fw);
+
+	return rc;
+}
+
 static int bnxt_flash_device(struct net_device *dev,
 			     struct ethtool_flash *flash)
 {
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h
index fa6fbde52bea..0a57cb6a4a4b 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h
@@ -94,8 +94,8 @@ u32 bnxt_fw_to_ethtool_speed(u16);
 u16 bnxt_get_fw_auto_link_speeds(u32);
 int bnxt_hwrm_nvm_get_dev_info(struct bnxt *bp,
 			       struct hwrm_nvm_get_dev_info_output *nvm_dev_info);
-int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
-				 u32 install_type);
+int bnxt_flash_package_from_fw_obj(struct net_device *dev, const struct firmware *fw,
+				   u32 install_type);
 void bnxt_ethtool_init(struct bnxt *bp);
 void bnxt_ethtool_free(struct bnxt *bp);
 
diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
index 2630d667f393..58d5646444b0 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
+++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
@@ -285,18 +285,8 @@ static int hinic_devlink_flash_update(struct devlink *devlink,
 				      struct netlink_ext_ack *extack)
 {
 	struct hinic_devlink_priv *priv = devlink_priv(devlink);
-	const struct firmware *fw;
-	int err;
 
-	err = request_firmware_direct(&fw, params->file_name,
-				      &priv->hwdev->hwif->pdev->dev);
-	if (err)
-		return err;
-
-	err = hinic_firmware_update(priv, fw, extack);
-	release_firmware(fw);
-
-	return err;
+	return hinic_firmware_update(priv, params->fw, extack);
 }
 
 static const struct devlink_ops hinic_devlink_ops = {
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 511da59bd6f2..0036d3e7df0b 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -247,9 +247,7 @@ ice_devlink_flash_update(struct devlink *devlink,
 			 struct netlink_ext_ack *extack)
 {
 	struct ice_pf *pf = devlink_priv(devlink);
-	struct device *dev = &pf->pdev->dev;
 	struct ice_hw *hw = &pf->hw;
-	const struct firmware *fw;
 	u8 preservation;
 	int err;
 
@@ -277,21 +275,11 @@ ice_devlink_flash_update(struct devlink *devlink,
 	if (err)
 		return err;
 
-	err = request_firmware(&fw, params->file_name, dev);
-	if (err) {
-		NL_SET_ERR_MSG_MOD(extack, "Unable to read file from disk");
-		return err;
-	}
-
-	dev_dbg(dev, "Beginning flash update with file '%s'\n", params->file_name);
-
 	devlink_flash_update_begin_notify(devlink);
 	devlink_flash_update_status_notify(devlink, "Preparing to flash", NULL, 0, 0);
-	err = ice_flash_pldm_image(pf, fw, preservation, extack);
+	err = ice_flash_pldm_image(pf, params->fw, preservation, extack);
 	devlink_flash_update_end_notify(devlink);
 
-	release_firmware(fw);
-
 	return err;
 }
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
index a28f95df2901..e2ed341648e4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
@@ -13,17 +13,8 @@ static int mlx5_devlink_flash_update(struct devlink *devlink,
 				     struct netlink_ext_ack *extack)
 {
 	struct mlx5_core_dev *dev = devlink_priv(devlink);
-	const struct firmware *fw;
-	int err;
 
-	err = request_firmware_direct(&fw, params->file_name, &dev->pdev->dev);
-	if (err)
-		return err;
-
-	err = mlx5_firmware_flash(dev, fw, extack);
-	release_firmware(fw);
-
-	return err;
+	return mlx5_firmware_flash(dev, params->fw, extack);
 }
 
 static u8 mlx5_fw_ver_major(u32 version)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index 937b8e46f8c7..88b751470c58 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -1116,16 +1116,7 @@ static int mlxsw_core_fw_flash_update(struct mlxsw_core *mlxsw_core,
 				      struct devlink_flash_update_params *params,
 				      struct netlink_ext_ack *extack)
 {
-	const struct firmware *firmware;
-	int err;
-
-	err = request_firmware_direct(&firmware, params->file_name, mlxsw_core->bus_info->dev);
-	if (err)
-		return err;
-	err = mlxsw_core_fw_flash(mlxsw_core, firmware, extack);
-	release_firmware(firmware);
-
-	return err;
+	return mlxsw_core_fw_flash(mlxsw_core, params->fw, extack);
 }
 
 static int mlxsw_core_devlink_param_fw_load_policy_validate(struct devlink *devlink, u32 id,
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
index 97d2b03208de..713ee3041d49 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
@@ -333,7 +333,7 @@ nfp_devlink_flash_update(struct devlink *devlink,
 			 struct devlink_flash_update_params *params,
 			 struct netlink_ext_ack *extack)
 {
-	return nfp_flash_update_common(devlink_priv(devlink), params->file_name, extack);
+	return nfp_flash_update_common(devlink_priv(devlink), params->fw, extack);
 }
 
 const struct devlink_ops nfp_devlink_ops = {
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.c b/drivers/net/ethernet/netronome/nfp/nfp_main.c
index e672614d2906..742a420152b3 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_main.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_main.c
@@ -301,11 +301,10 @@ static int nfp_pcie_sriov_configure(struct pci_dev *pdev, int num_vfs)
 		return nfp_pcie_sriov_enable(pdev, num_vfs);
 }
 
-int nfp_flash_update_common(struct nfp_pf *pf, const char *path,
+int nfp_flash_update_common(struct nfp_pf *pf, const struct firmware *fw,
 			    struct netlink_ext_ack *extack)
 {
 	struct device *dev = &pf->pdev->dev;
-	const struct firmware *fw;
 	struct nfp_nsp *nsp;
 	int err;
 
@@ -319,24 +318,12 @@ int nfp_flash_update_common(struct nfp_pf *pf, const char *path,
 		return err;
 	}
 
-	err = request_firmware_direct(&fw, path, dev);
-	if (err) {
-		NL_SET_ERR_MSG_MOD(extack,
-				   "unable to read flash file from disk");
-		goto exit_close_nsp;
-	}
-
-	dev_info(dev, "Please be patient while writing flash image: %s\n",
-		 path);
-
 	err = nfp_nsp_write_flash(nsp, fw);
 	if (err < 0)
-		goto exit_release_fw;
+		goto exit_close_nsp;
 	dev_info(dev, "Finished writing flash image\n");
 	err = 0;
 
-exit_release_fw:
-	release_firmware(fw);
 exit_close_nsp:
 	nfp_nsp_close(nsp);
 	return err;
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.h b/drivers/net/ethernet/netronome/nfp/nfp_main.h
index fa6b13a05941..a7dede946a33 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_main.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_main.h
@@ -166,7 +166,7 @@ nfp_pf_map_rtsym(struct nfp_pf *pf, const char *name, const char *sym_fmt,
 		 unsigned int min_size, struct nfp_cpp_area **area);
 int nfp_mbox_cmd(struct nfp_pf *pf, u32 cmd, void *in_data, u64 in_length,
 		 void *out_data, u64 out_length);
-int nfp_flash_update_common(struct nfp_pf *pf, const char *path,
+int nfp_flash_update_common(struct nfp_pf *pf, const struct firmware *fw,
 			    struct netlink_ext_ack *extack);
 
 enum nfp_dump_diag {
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
index 51d64718ed9f..b41301a5b0df 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
@@ -15,7 +15,7 @@ static int ionic_dl_flash_update(struct devlink *dl,
 {
 	struct ionic *ionic = devlink_priv(dl);
 
-	return ionic_firmware_update(ionic->lif, params->file_name, extack);
+	return ionic_firmware_update(ionic->lif, params->fw, extack);
 }
 
 static int ionic_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
index 5c01a9e306d8..0a77e8e810c5 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
@@ -6,7 +6,7 @@
 
 #include <net/devlink.h>
 
-int ionic_firmware_update(struct ionic_lif *lif, const char *fw_name,
+int ionic_firmware_update(struct ionic_lif *lif, const struct firmware *fw,
 			  struct netlink_ext_ack *extack);
 
 struct ionic *ionic_devlink_alloc(struct device *dev);
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_fw.c b/drivers/net/ethernet/pensando/ionic/ionic_fw.c
index d7bbf336c6f6..fd2ce134f66c 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_fw.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_fw.c
@@ -91,7 +91,7 @@ static int ionic_fw_status_long_wait(struct ionic *ionic,
 	return err;
 }
 
-int ionic_firmware_update(struct ionic_lif *lif, const char *fw_name,
+int ionic_firmware_update(struct ionic_lif *lif, const struct firmware *fw,
 			  struct netlink_ext_ack *extack)
 {
 	struct ionic_dev *idev = &lif->ionic->idev;
@@ -99,24 +99,15 @@ int ionic_firmware_update(struct ionic_lif *lif, const char *fw_name,
 	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,
@@ -200,7 +191,6 @@ int ionic_firmware_update(struct ionic_lif *lif, const char *fw_name,
 		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/include/net/devlink.h b/include/net/devlink.h
index b01bb9bca5a2..d1d125a33322 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -19,6 +19,7 @@
 #include <net/flow_offload.h>
 #include <uapi/linux/devlink.h>
 #include <linux/xarray.h>
+#include <linux/firmware.h>
 
 #define DEVLINK_RELOAD_STATS_ARRAY_SIZE \
 	(__DEVLINK_RELOAD_LIMIT_MAX * __DEVLINK_RELOAD_ACTION_MAX)
@@ -566,15 +567,15 @@ enum devlink_param_generic_id {
 
 /**
  * struct devlink_flash_update_params - Flash Update parameters
- * @file_name: the name of the flash firmware file to update from
+ * @fw: pointer to the firmware data to update from
  * @component: the flash component to update
  *
- * With the exception of file_name, drivers must opt-in to parameters by
+ * With the exception of fw, drivers must opt-in to parameters by
  * setting the appropriate bit in the supported_flash_update_params field in
  * their devlink_ops structure.
  */
 struct devlink_flash_update_params {
-	const char *file_name;
+	const struct firmware *fw;
 	const char *component;
 	u32 overwrite_mask;
 };
diff --git a/net/core/devlink.c b/net/core/devlink.c
index ab4b1368904f..b0121d79ac75 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3429,10 +3429,12 @@ EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify);
 static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
 				       struct genl_info *info)
 {
-	struct nlattr *nla_component, *nla_overwrite_mask;
+	struct nlattr *nla_component, *nla_overwrite_mask, *nla_file_name;
 	struct devlink_flash_update_params params = {};
 	struct devlink *devlink = info->user_ptr[0];
+	const char *file_name;
 	u32 supported_params;
+	int ret;
 
 	if (!devlink->ops->flash_update)
 		return -EOPNOTSUPP;
@@ -3442,8 +3444,6 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
 
 	supported_params = devlink->ops->supported_flash_update_params;
 
-	params.file_name = nla_data(info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME]);
-
 	nla_component = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT];
 	if (nla_component) {
 		if (!(supported_params & DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT)) {
@@ -3467,7 +3467,19 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
 		params.overwrite_mask = sections.value & sections.selector;
 	}
 
-	return devlink->ops->flash_update(devlink, &params, info->extack);
+	nla_file_name = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME];
+	file_name = nla_data(nla_file_name);
+	ret = request_firmware(&params.fw, file_name, devlink->dev);
+	if (ret) {
+		NL_SET_ERR_MSG_ATTR(info->extack, nla_file_name, "failed to locate the requested firmware file");
+		return ret;
+	}
+
+	ret = devlink->ops->flash_update(devlink, &params, info->extack);
+
+	release_firmware(params.fw);
+
+	return ret;
 }
 
 static const struct devlink_param devlink_param_generic[] = {
@@ -10225,12 +10237,16 @@ int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
 		goto out;
 	}
 
-	params.file_name = file_name;
+	ret = request_firmware(&params.fw, file_name, devlink->dev);
+	if (ret)
+		goto out;
 
 	mutex_lock(&devlink->lock);
 	ret = devlink->ops->flash_update(devlink, &params, NULL);
 	mutex_unlock(&devlink->lock);
 
+	release_firmware(params.fw);
+
 out:
 	rtnl_lock();
 	dev_put(dev);
-- 
2.29.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [net-next v3 2/2] devlink: move flash end and begin to core devlink
  2020-11-17 20:08 [net-next v3 0/2] devlink: move common flash_update calls to core Jacob Keller
  2020-11-17 20:08 ` [net-next v3 1/2] devlink: move request_firmware out of driver Jacob Keller
@ 2020-11-17 20:08 ` Jacob Keller
  2020-11-18  9:49   ` Vasundhara Volam
  2020-11-18 16:34   ` Jiri Pirko
  1 sibling, 2 replies; 10+ messages in thread
From: Jacob Keller @ 2020-11-17 20:08 UTC (permalink / raw)
  To: netdev
  Cc: Jacob Keller, Jiri Pirko, Michael Chan, Shannon Nelson,
	Saeed Mahameed, Boris Pismenny, Bin Luo, Jakub Kicinksi

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.keller@intel.com>
Cc: Jiri Pirko <jiri@nvidia.com>
Cc: Michael Chan <michael.chan@broadcom.com>
Cc: Shannon Nelson <snelson@pensando.io>
Cc: Saeed Mahameed <saeedm@nvidia.com>
Cc: Boris Pismenny <borisp@nvidia.com>
Cc: Bin Luo <luobin9@huawei.com>
Cc: Jakub Kicinksi <kuba@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,
 				      &params);
 }
-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,
 				      &params);
 }
-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, &params, 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, &params, NULL);
+	devlink_flash_update_end_notify(devlink);
 	mutex_unlock(&devlink->lock);
 
 	release_firmware(params.fw);
-- 
2.29.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [net-next v3 1/2] devlink: move request_firmware out of driver
  2020-11-17 20:08 ` [net-next v3 1/2] devlink: move request_firmware out of driver Jacob Keller
@ 2020-11-17 20:10   ` Jacob Keller
  2020-11-18  9:48     ` Vasundhara Volam
  2020-11-18 18:32     ` Jakub Kicinski
  2020-11-18 16:38   ` Jiri Pirko
  1 sibling, 2 replies; 10+ messages in thread
From: Jacob Keller @ 2020-11-17 20:10 UTC (permalink / raw)
  To: netdev
  Cc: Jiri Pirko, Michael Chan, Shannon Nelson, Saeed Mahameed,
	Boris Pismenny, Bin Luo, Jakub Kicinksi



On 11/17/2020 12:08 PM, Jacob Keller wrote:
> All drivers which implement the devlink flash update support, with the
> exception of netdevsim, use either request_firmware or
> request_firmware_direct to locate the firmware file. Rather than having
> each driver do this separately as part of its .flash_update
> implementation, perform the request_firmware within net/core/devlink.c
> 
> Replace the file_name parameter in the struct devlink_flash_update_params
> with a pointer to the fw object.
> 
> Use request_firmware rather than request_firmware_direct. Although most
> Linux distributions today do not have the fallback mechanism
> implemented, only about half the drivers used the _direct request, as
> compared to the generic request_firmware. In the event that
> a distribution does support the fallback mechanism, the devlink flash
> update ought to be able to use it to provide the firmware contents. For
> distributions which do not support the fallback userspace mechanism,
> there should be essentially no difference between request_firmware and
> request_firmware_direct.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Cc: Jiri Pirko <jiri@nvidia.com>
> Cc: Michael Chan <michael.chan@broadcom.com>
> Cc: Shannon Nelson <snelson@pensando.io>
> Cc: Saeed Mahameed <saeedm@nvidia.com>
> Cc: Boris Pismenny <borisp@nvidia.com>
> Cc: Bin Luo <luobin9@huawei.com>
> Cc: Jakub Kicinksi <kuba@kernel.org>
> ---

Oof, forgot to metion that the only change since v2 is to fix the typo
in the commit message pointed out by Shannon. Otherwise, this patch is
identical and just comes in series with the other change.

>  .../net/ethernet/broadcom/bnxt/bnxt_devlink.c |  2 +-
>  .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 33 ++++++++++++-------
>  .../net/ethernet/broadcom/bnxt/bnxt_ethtool.h |  4 +--
>  .../net/ethernet/huawei/hinic/hinic_devlink.c | 12 +------
>  drivers/net/ethernet/intel/ice/ice_devlink.c  | 14 +-------
>  .../net/ethernet/mellanox/mlx5/core/devlink.c | 11 +------
>  drivers/net/ethernet/mellanox/mlxsw/core.c    | 11 +------
>  .../net/ethernet/netronome/nfp/nfp_devlink.c  |  2 +-
>  drivers/net/ethernet/netronome/nfp/nfp_main.c | 17 ++--------
>  drivers/net/ethernet/netronome/nfp/nfp_main.h |  2 +-
>  .../ethernet/pensando/ionic/ionic_devlink.c   |  2 +-
>  .../ethernet/pensando/ionic/ionic_devlink.h   |  2 +-
>  .../net/ethernet/pensando/ionic/ionic_fw.c    | 12 +------
>  include/net/devlink.h                         |  7 ++--
>  net/core/devlink.c                            | 26 ++++++++++++---
>  15 files changed, 61 insertions(+), 96 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> index 184b6d0513b2..4ebae8a236fd 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> @@ -32,7 +32,7 @@ bnxt_dl_flash_update(struct devlink *dl,
>  
>  	devlink_flash_update_begin_notify(dl);
>  	devlink_flash_update_status_notify(dl, "Preparing to flash", NULL, 0, 0);
> -	rc = bnxt_flash_package_from_file(bp->dev, params->file_name, 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
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index 53687bc7fcf5..91e73aedcdff 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -2416,13 +2416,12 @@ static int bnxt_flash_firmware_from_file(struct net_device *dev,
>  	return rc;
>  }
>  
> -int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
> -				 u32 install_type)
> +int bnxt_flash_package_from_fw_obj(struct net_device *dev, const struct firmware *fw,
> +				   u32 install_type)
>  {
>  	struct bnxt *bp = netdev_priv(dev);
>  	struct hwrm_nvm_install_update_output *resp = bp->hwrm_cmd_resp_addr;
>  	struct hwrm_nvm_install_update_input install = {0};
> -	const struct firmware *fw;
>  	u32 item_len;
>  	int rc = 0;
>  	u16 index;
> @@ -2437,13 +2436,6 @@ int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
>  		return rc;
>  	}
>  
> -	rc = request_firmware(&fw, filename, &dev->dev);
> -	if (rc != 0) {
> -		netdev_err(dev, "PKG error %d requesting file: %s\n",
> -			   rc, filename);
> -		return rc;
> -	}
> -
>  	if (fw->size > item_len) {
>  		netdev_err(dev, "PKG insufficient update area in nvram: %lu\n",
>  			   (unsigned long)fw->size);
> @@ -2475,7 +2467,6 @@ int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
>  					  dma_handle);
>  		}
>  	}
> -	release_firmware(fw);
>  	if (rc)
>  		goto err_exit;
>  
> @@ -2514,6 +2505,26 @@ int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
>  	return rc;
>  }
>  
> +static int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
> +					u32 install_type)
> +{
> +	const struct firmware *fw;
> +	int rc;
> +
> +	rc = request_firmware(&fw, filename, &dev->dev);
> +	if (rc != 0) {
> +		netdev_err(dev, "PKG error %d requesting file: %s\n",
> +			   rc, filename);
> +		return rc;
> +	}
> +
> +	rc = bnxt_flash_package_from_fw_obj(dev, fw, install_type);
> +
> +	release_firmware(fw);
> +
> +	return rc;
> +}
> +
>  static int bnxt_flash_device(struct net_device *dev,
>  			     struct ethtool_flash *flash)
>  {
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h
> index fa6fbde52bea..0a57cb6a4a4b 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h
> @@ -94,8 +94,8 @@ u32 bnxt_fw_to_ethtool_speed(u16);
>  u16 bnxt_get_fw_auto_link_speeds(u32);
>  int bnxt_hwrm_nvm_get_dev_info(struct bnxt *bp,
>  			       struct hwrm_nvm_get_dev_info_output *nvm_dev_info);
> -int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
> -				 u32 install_type);
> +int bnxt_flash_package_from_fw_obj(struct net_device *dev, const struct firmware *fw,
> +				   u32 install_type);
>  void bnxt_ethtool_init(struct bnxt *bp);
>  void bnxt_ethtool_free(struct bnxt *bp);
>  
> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> index 2630d667f393..58d5646444b0 100644
> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> @@ -285,18 +285,8 @@ static int hinic_devlink_flash_update(struct devlink *devlink,
>  				      struct netlink_ext_ack *extack)
>  {
>  	struct hinic_devlink_priv *priv = devlink_priv(devlink);
> -	const struct firmware *fw;
> -	int err;
>  
> -	err = request_firmware_direct(&fw, params->file_name,
> -				      &priv->hwdev->hwif->pdev->dev);
> -	if (err)
> -		return err;
> -
> -	err = hinic_firmware_update(priv, fw, extack);
> -	release_firmware(fw);
> -
> -	return err;
> +	return hinic_firmware_update(priv, params->fw, extack);
>  }
>  
>  static const struct devlink_ops hinic_devlink_ops = {
> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> index 511da59bd6f2..0036d3e7df0b 100644
> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -247,9 +247,7 @@ ice_devlink_flash_update(struct devlink *devlink,
>  			 struct netlink_ext_ack *extack)
>  {
>  	struct ice_pf *pf = devlink_priv(devlink);
> -	struct device *dev = &pf->pdev->dev;
>  	struct ice_hw *hw = &pf->hw;
> -	const struct firmware *fw;
>  	u8 preservation;
>  	int err;
>  
> @@ -277,21 +275,11 @@ ice_devlink_flash_update(struct devlink *devlink,
>  	if (err)
>  		return err;
>  
> -	err = request_firmware(&fw, params->file_name, dev);
> -	if (err) {
> -		NL_SET_ERR_MSG_MOD(extack, "Unable to read file from disk");
> -		return err;
> -	}
> -
> -	dev_dbg(dev, "Beginning flash update with file '%s'\n", params->file_name);
> -
>  	devlink_flash_update_begin_notify(devlink);
>  	devlink_flash_update_status_notify(devlink, "Preparing to flash", NULL, 0, 0);
> -	err = ice_flash_pldm_image(pf, fw, preservation, extack);
> +	err = ice_flash_pldm_image(pf, params->fw, preservation, extack);
>  	devlink_flash_update_end_notify(devlink);
>  
> -	release_firmware(fw);
> -
>  	return err;
>  }
>  
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
> index a28f95df2901..e2ed341648e4 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
> @@ -13,17 +13,8 @@ static int mlx5_devlink_flash_update(struct devlink *devlink,
>  				     struct netlink_ext_ack *extack)
>  {
>  	struct mlx5_core_dev *dev = devlink_priv(devlink);
> -	const struct firmware *fw;
> -	int err;
>  
> -	err = request_firmware_direct(&fw, params->file_name, &dev->pdev->dev);
> -	if (err)
> -		return err;
> -
> -	err = mlx5_firmware_flash(dev, fw, extack);
> -	release_firmware(fw);
> -
> -	return err;
> +	return mlx5_firmware_flash(dev, params->fw, extack);
>  }
>  
>  static u8 mlx5_fw_ver_major(u32 version)
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
> index 937b8e46f8c7..88b751470c58 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/core.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
> @@ -1116,16 +1116,7 @@ static int mlxsw_core_fw_flash_update(struct mlxsw_core *mlxsw_core,
>  				      struct devlink_flash_update_params *params,
>  				      struct netlink_ext_ack *extack)
>  {
> -	const struct firmware *firmware;
> -	int err;
> -
> -	err = request_firmware_direct(&firmware, params->file_name, mlxsw_core->bus_info->dev);
> -	if (err)
> -		return err;
> -	err = mlxsw_core_fw_flash(mlxsw_core, firmware, extack);
> -	release_firmware(firmware);
> -
> -	return err;
> +	return mlxsw_core_fw_flash(mlxsw_core, params->fw, extack);
>  }
>  
>  static int mlxsw_core_devlink_param_fw_load_policy_validate(struct devlink *devlink, u32 id,
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> index 97d2b03208de..713ee3041d49 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> @@ -333,7 +333,7 @@ nfp_devlink_flash_update(struct devlink *devlink,
>  			 struct devlink_flash_update_params *params,
>  			 struct netlink_ext_ack *extack)
>  {
> -	return nfp_flash_update_common(devlink_priv(devlink), params->file_name, extack);
> +	return nfp_flash_update_common(devlink_priv(devlink), params->fw, extack);
>  }
>  
>  const struct devlink_ops nfp_devlink_ops = {
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.c b/drivers/net/ethernet/netronome/nfp/nfp_main.c
> index e672614d2906..742a420152b3 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_main.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_main.c
> @@ -301,11 +301,10 @@ static int nfp_pcie_sriov_configure(struct pci_dev *pdev, int num_vfs)
>  		return nfp_pcie_sriov_enable(pdev, num_vfs);
>  }
>  
> -int nfp_flash_update_common(struct nfp_pf *pf, const char *path,
> +int nfp_flash_update_common(struct nfp_pf *pf, const struct firmware *fw,
>  			    struct netlink_ext_ack *extack)
>  {
>  	struct device *dev = &pf->pdev->dev;
> -	const struct firmware *fw;
>  	struct nfp_nsp *nsp;
>  	int err;
>  
> @@ -319,24 +318,12 @@ int nfp_flash_update_common(struct nfp_pf *pf, const char *path,
>  		return err;
>  	}
>  
> -	err = request_firmware_direct(&fw, path, dev);
> -	if (err) {
> -		NL_SET_ERR_MSG_MOD(extack,
> -				   "unable to read flash file from disk");
> -		goto exit_close_nsp;
> -	}
> -
> -	dev_info(dev, "Please be patient while writing flash image: %s\n",
> -		 path);
> -
>  	err = nfp_nsp_write_flash(nsp, fw);
>  	if (err < 0)
> -		goto exit_release_fw;
> +		goto exit_close_nsp;
>  	dev_info(dev, "Finished writing flash image\n");
>  	err = 0;
>  
> -exit_release_fw:
> -	release_firmware(fw);
>  exit_close_nsp:
>  	nfp_nsp_close(nsp);
>  	return err;
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.h b/drivers/net/ethernet/netronome/nfp/nfp_main.h
> index fa6b13a05941..a7dede946a33 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_main.h
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_main.h
> @@ -166,7 +166,7 @@ nfp_pf_map_rtsym(struct nfp_pf *pf, const char *name, const char *sym_fmt,
>  		 unsigned int min_size, struct nfp_cpp_area **area);
>  int nfp_mbox_cmd(struct nfp_pf *pf, u32 cmd, void *in_data, u64 in_length,
>  		 void *out_data, u64 out_length);
> -int nfp_flash_update_common(struct nfp_pf *pf, const char *path,
> +int nfp_flash_update_common(struct nfp_pf *pf, const struct firmware *fw,
>  			    struct netlink_ext_ack *extack);
>  
>  enum nfp_dump_diag {
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> index 51d64718ed9f..b41301a5b0df 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> @@ -15,7 +15,7 @@ static int ionic_dl_flash_update(struct devlink *dl,
>  {
>  	struct ionic *ionic = devlink_priv(dl);
>  
> -	return ionic_firmware_update(ionic->lif, params->file_name, extack);
> +	return ionic_firmware_update(ionic->lif, params->fw, extack);
>  }
>  
>  static int ionic_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
> index 5c01a9e306d8..0a77e8e810c5 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
> @@ -6,7 +6,7 @@
>  
>  #include <net/devlink.h>
>  
> -int ionic_firmware_update(struct ionic_lif *lif, const char *fw_name,
> +int ionic_firmware_update(struct ionic_lif *lif, const struct firmware *fw,
>  			  struct netlink_ext_ack *extack);
>  
>  struct ionic *ionic_devlink_alloc(struct device *dev);
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_fw.c b/drivers/net/ethernet/pensando/ionic/ionic_fw.c
> index d7bbf336c6f6..fd2ce134f66c 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_fw.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_fw.c
> @@ -91,7 +91,7 @@ static int ionic_fw_status_long_wait(struct ionic *ionic,
>  	return err;
>  }
>  
> -int ionic_firmware_update(struct ionic_lif *lif, const char *fw_name,
> +int ionic_firmware_update(struct ionic_lif *lif, const struct firmware *fw,
>  			  struct netlink_ext_ack *extack)
>  {
>  	struct ionic_dev *idev = &lif->ionic->idev;
> @@ -99,24 +99,15 @@ int ionic_firmware_update(struct ionic_lif *lif, const char *fw_name,
>  	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,
> @@ -200,7 +191,6 @@ int ionic_firmware_update(struct ionic_lif *lif, const char *fw_name,
>  		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/include/net/devlink.h b/include/net/devlink.h
> index b01bb9bca5a2..d1d125a33322 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -19,6 +19,7 @@
>  #include <net/flow_offload.h>
>  #include <uapi/linux/devlink.h>
>  #include <linux/xarray.h>
> +#include <linux/firmware.h>
>  
>  #define DEVLINK_RELOAD_STATS_ARRAY_SIZE \
>  	(__DEVLINK_RELOAD_LIMIT_MAX * __DEVLINK_RELOAD_ACTION_MAX)
> @@ -566,15 +567,15 @@ enum devlink_param_generic_id {
>  
>  /**
>   * struct devlink_flash_update_params - Flash Update parameters
> - * @file_name: the name of the flash firmware file to update from
> + * @fw: pointer to the firmware data to update from
>   * @component: the flash component to update
>   *
> - * With the exception of file_name, drivers must opt-in to parameters by
> + * With the exception of fw, drivers must opt-in to parameters by
>   * setting the appropriate bit in the supported_flash_update_params field in
>   * their devlink_ops structure.
>   */
>  struct devlink_flash_update_params {
> -	const char *file_name;
> +	const struct firmware *fw;
>  	const char *component;
>  	u32 overwrite_mask;
>  };
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index ab4b1368904f..b0121d79ac75 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -3429,10 +3429,12 @@ EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify);
>  static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
>  				       struct genl_info *info)
>  {
> -	struct nlattr *nla_component, *nla_overwrite_mask;
> +	struct nlattr *nla_component, *nla_overwrite_mask, *nla_file_name;
>  	struct devlink_flash_update_params params = {};
>  	struct devlink *devlink = info->user_ptr[0];
> +	const char *file_name;
>  	u32 supported_params;
> +	int ret;
>  
>  	if (!devlink->ops->flash_update)
>  		return -EOPNOTSUPP;
> @@ -3442,8 +3444,6 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
>  
>  	supported_params = devlink->ops->supported_flash_update_params;
>  
> -	params.file_name = nla_data(info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME]);
> -
>  	nla_component = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT];
>  	if (nla_component) {
>  		if (!(supported_params & DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT)) {
> @@ -3467,7 +3467,19 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
>  		params.overwrite_mask = sections.value & sections.selector;
>  	}
>  
> -	return devlink->ops->flash_update(devlink, &params, info->extack);
> +	nla_file_name = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME];
> +	file_name = nla_data(nla_file_name);
> +	ret = request_firmware(&params.fw, file_name, devlink->dev);
> +	if (ret) {
> +		NL_SET_ERR_MSG_ATTR(info->extack, nla_file_name, "failed to locate the requested firmware file");
> +		return ret;
> +	}
> +
> +	ret = devlink->ops->flash_update(devlink, &params, info->extack);
> +
> +	release_firmware(params.fw);
> +
> +	return ret;
>  }
>  
>  static const struct devlink_param devlink_param_generic[] = {
> @@ -10225,12 +10237,16 @@ int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
>  		goto out;
>  	}
>  
> -	params.file_name = file_name;
> +	ret = request_firmware(&params.fw, file_name, devlink->dev);
> +	if (ret)
> +		goto out;
>  
>  	mutex_lock(&devlink->lock);
>  	ret = devlink->ops->flash_update(devlink, &params, NULL);
>  	mutex_unlock(&devlink->lock);
>  
> +	release_firmware(params.fw);
> +
>  out:
>  	rtnl_lock();
>  	dev_put(dev);
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [net-next v3 1/2] devlink: move request_firmware out of driver
  2020-11-17 20:10   ` Jacob Keller
@ 2020-11-18  9:48     ` Vasundhara Volam
  2020-11-18 18:32     ` Jakub Kicinski
  1 sibling, 0 replies; 10+ messages in thread
From: Vasundhara Volam @ 2020-11-18  9:48 UTC (permalink / raw)
  To: Jacob Keller
  Cc: Netdev, Jiri Pirko, Michael Chan, Shannon Nelson, Saeed Mahameed,
	Boris Pismenny, Bin Luo, Jakub Kicinksi

[-- Attachment #1: Type: text/plain, Size: 22086 bytes --]

On Wed, Nov 18, 2020 at 1:41 AM Jacob Keller <jacob.e.keller@intel.com> wrote:
>
>
>
> On 11/17/2020 12:08 PM, Jacob Keller wrote:
> > All drivers which implement the devlink flash update support, with the
> > exception of netdevsim, use either request_firmware or
> > request_firmware_direct to locate the firmware file. Rather than having
> > each driver do this separately as part of its .flash_update
> > implementation, perform the request_firmware within net/core/devlink.c
> >
> > Replace the file_name parameter in the struct devlink_flash_update_params
> > with a pointer to the fw object.
> >
> > Use request_firmware rather than request_firmware_direct. Although most
> > Linux distributions today do not have the fallback mechanism
> > implemented, only about half the drivers used the _direct request, as
> > compared to the generic request_firmware. In the event that
> > a distribution does support the fallback mechanism, the devlink flash
> > update ought to be able to use it to provide the firmware contents. For
> > distributions which do not support the fallback userspace mechanism,
> > there should be essentially no difference between request_firmware and
> > request_firmware_direct.
> >
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > Cc: Jiri Pirko <jiri@nvidia.com>
> > Cc: Michael Chan <michael.chan@broadcom.com>
> > Cc: Shannon Nelson <snelson@pensando.io>
> > Cc: Saeed Mahameed <saeedm@nvidia.com>
> > Cc: Boris Pismenny <borisp@nvidia.com>
> > Cc: Bin Luo <luobin9@huawei.com>
> > Cc: Jakub Kicinksi <kuba@kernel.org>
> > ---
>
> Oof, forgot to metion that the only change since v2 is to fix the typo
> in the commit message pointed out by Shannon. Otherwise, this patch is
> identical and just comes in series with the other change.
>
> >  .../net/ethernet/broadcom/bnxt/bnxt_devlink.c |  2 +-
> >  .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 33 ++++++++++++-------
> >  .../net/ethernet/broadcom/bnxt/bnxt_ethtool.h |  4 +--
> >  .../net/ethernet/huawei/hinic/hinic_devlink.c | 12 +------
> >  drivers/net/ethernet/intel/ice/ice_devlink.c  | 14 +-------
> >  .../net/ethernet/mellanox/mlx5/core/devlink.c | 11 +------
> >  drivers/net/ethernet/mellanox/mlxsw/core.c    | 11 +------
> >  .../net/ethernet/netronome/nfp/nfp_devlink.c  |  2 +-
> >  drivers/net/ethernet/netronome/nfp/nfp_main.c | 17 ++--------
> >  drivers/net/ethernet/netronome/nfp/nfp_main.h |  2 +-
> >  .../ethernet/pensando/ionic/ionic_devlink.c   |  2 +-
> >  .../ethernet/pensando/ionic/ionic_devlink.h   |  2 +-
> >  .../net/ethernet/pensando/ionic/ionic_fw.c    | 12 +------
> >  include/net/devlink.h                         |  7 ++--
> >  net/core/devlink.c                            | 26 ++++++++++++---
> >  15 files changed, 61 insertions(+), 96 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> > index 184b6d0513b2..4ebae8a236fd 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> > @@ -32,7 +32,7 @@ bnxt_dl_flash_update(struct devlink *dl,
> >
> >       devlink_flash_update_begin_notify(dl);
> >       devlink_flash_update_status_notify(dl, "Preparing to flash", NULL, 0, 0);
> > -     rc = bnxt_flash_package_from_file(bp->dev, params->file_name, 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
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > index 53687bc7fcf5..91e73aedcdff 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > @@ -2416,13 +2416,12 @@ static int bnxt_flash_firmware_from_file(struct net_device *dev,
> >       return rc;
> >  }
> >
> > -int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
> > -                              u32 install_type)
> > +int bnxt_flash_package_from_fw_obj(struct net_device *dev, const struct firmware *fw,
> > +                                u32 install_type)
> >  {
> >       struct bnxt *bp = netdev_priv(dev);
> >       struct hwrm_nvm_install_update_output *resp = bp->hwrm_cmd_resp_addr;
> >       struct hwrm_nvm_install_update_input install = {0};
> > -     const struct firmware *fw;
> >       u32 item_len;
> >       int rc = 0;
> >       u16 index;
> > @@ -2437,13 +2436,6 @@ int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
> >               return rc;
> >       }
> >
> > -     rc = request_firmware(&fw, filename, &dev->dev);
> > -     if (rc != 0) {
> > -             netdev_err(dev, "PKG error %d requesting file: %s\n",
> > -                        rc, filename);
> > -             return rc;
> > -     }
> > -
> >       if (fw->size > item_len) {
> >               netdev_err(dev, "PKG insufficient update area in nvram: %lu\n",
> >                          (unsigned long)fw->size);
> > @@ -2475,7 +2467,6 @@ int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
> >                                         dma_handle);
> >               }
> >       }
> > -     release_firmware(fw);
> >       if (rc)
> >               goto err_exit;
> >
> > @@ -2514,6 +2505,26 @@ int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
> >       return rc;
> >  }
> >
> > +static int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
> > +                                     u32 install_type)
> > +{
> > +     const struct firmware *fw;
> > +     int rc;
> > +
> > +     rc = request_firmware(&fw, filename, &dev->dev);
> > +     if (rc != 0) {
> > +             netdev_err(dev, "PKG error %d requesting file: %s\n",
> > +                        rc, filename);
> > +             return rc;
> > +     }
> > +
> > +     rc = bnxt_flash_package_from_fw_obj(dev, fw, install_type);
> > +
> > +     release_firmware(fw);
> > +
> > +     return rc;
> > +}
> > +
> >  static int bnxt_flash_device(struct net_device *dev,
> >                            struct ethtool_flash *flash)
> >  {
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h
> > index fa6fbde52bea..0a57cb6a4a4b 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h
> > @@ -94,8 +94,8 @@ u32 bnxt_fw_to_ethtool_speed(u16);
> >  u16 bnxt_get_fw_auto_link_speeds(u32);
> >  int bnxt_hwrm_nvm_get_dev_info(struct bnxt *bp,
> >                              struct hwrm_nvm_get_dev_info_output *nvm_dev_info);
> > -int bnxt_flash_package_from_file(struct net_device *dev, const char *filename,
> > -                              u32 install_type);
> > +int bnxt_flash_package_from_fw_obj(struct net_device *dev, const struct firmware *fw,
> > +                                u32 install_type);
> >  void bnxt_ethtool_init(struct bnxt *bp);
> >  void bnxt_ethtool_free(struct bnxt *bp);
> >
> > diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> > index 2630d667f393..58d5646444b0 100644
> > --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> > +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> > @@ -285,18 +285,8 @@ static int hinic_devlink_flash_update(struct devlink *devlink,
> >                                     struct netlink_ext_ack *extack)
> >  {
> >       struct hinic_devlink_priv *priv = devlink_priv(devlink);
> > -     const struct firmware *fw;
> > -     int err;
> >
> > -     err = request_firmware_direct(&fw, params->file_name,
> > -                                   &priv->hwdev->hwif->pdev->dev);
> > -     if (err)
> > -             return err;
> > -
> > -     err = hinic_firmware_update(priv, fw, extack);
> > -     release_firmware(fw);
> > -
> > -     return err;
> > +     return hinic_firmware_update(priv, params->fw, extack);
> >  }
> >
> >  static const struct devlink_ops hinic_devlink_ops = {
> > diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> > index 511da59bd6f2..0036d3e7df0b 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> > @@ -247,9 +247,7 @@ ice_devlink_flash_update(struct devlink *devlink,
> >                        struct netlink_ext_ack *extack)
> >  {
> >       struct ice_pf *pf = devlink_priv(devlink);
> > -     struct device *dev = &pf->pdev->dev;
> >       struct ice_hw *hw = &pf->hw;
> > -     const struct firmware *fw;
> >       u8 preservation;
> >       int err;
> >
> > @@ -277,21 +275,11 @@ ice_devlink_flash_update(struct devlink *devlink,
> >       if (err)
> >               return err;
> >
> > -     err = request_firmware(&fw, params->file_name, dev);
> > -     if (err) {
> > -             NL_SET_ERR_MSG_MOD(extack, "Unable to read file from disk");
> > -             return err;
> > -     }
> > -
> > -     dev_dbg(dev, "Beginning flash update with file '%s'\n", params->file_name);
> > -
> >       devlink_flash_update_begin_notify(devlink);
> >       devlink_flash_update_status_notify(devlink, "Preparing to flash", NULL, 0, 0);
> > -     err = ice_flash_pldm_image(pf, fw, preservation, extack);
> > +     err = ice_flash_pldm_image(pf, params->fw, preservation, extack);
> >       devlink_flash_update_end_notify(devlink);
> >
> > -     release_firmware(fw);
> > -
> >       return err;
> >  }
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
> > index a28f95df2901..e2ed341648e4 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
> > @@ -13,17 +13,8 @@ static int mlx5_devlink_flash_update(struct devlink *devlink,
> >                                    struct netlink_ext_ack *extack)
> >  {
> >       struct mlx5_core_dev *dev = devlink_priv(devlink);
> > -     const struct firmware *fw;
> > -     int err;
> >
> > -     err = request_firmware_direct(&fw, params->file_name, &dev->pdev->dev);
> > -     if (err)
> > -             return err;
> > -
> > -     err = mlx5_firmware_flash(dev, fw, extack);
> > -     release_firmware(fw);
> > -
> > -     return err;
> > +     return mlx5_firmware_flash(dev, params->fw, extack);
> >  }
> >
> >  static u8 mlx5_fw_ver_major(u32 version)
> > diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
> > index 937b8e46f8c7..88b751470c58 100644
> > --- a/drivers/net/ethernet/mellanox/mlxsw/core.c
> > +++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
> > @@ -1116,16 +1116,7 @@ static int mlxsw_core_fw_flash_update(struct mlxsw_core *mlxsw_core,
> >                                     struct devlink_flash_update_params *params,
> >                                     struct netlink_ext_ack *extack)
> >  {
> > -     const struct firmware *firmware;
> > -     int err;
> > -
> > -     err = request_firmware_direct(&firmware, params->file_name, mlxsw_core->bus_info->dev);
> > -     if (err)
> > -             return err;
> > -     err = mlxsw_core_fw_flash(mlxsw_core, firmware, extack);
> > -     release_firmware(firmware);
> > -
> > -     return err;
> > +     return mlxsw_core_fw_flash(mlxsw_core, params->fw, extack);
> >  }
> >
> >  static int mlxsw_core_devlink_param_fw_load_policy_validate(struct devlink *devlink, u32 id,
> > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> > index 97d2b03208de..713ee3041d49 100644
> > --- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> > +++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> > @@ -333,7 +333,7 @@ nfp_devlink_flash_update(struct devlink *devlink,
> >                        struct devlink_flash_update_params *params,
> >                        struct netlink_ext_ack *extack)
> >  {
> > -     return nfp_flash_update_common(devlink_priv(devlink), params->file_name, extack);
> > +     return nfp_flash_update_common(devlink_priv(devlink), params->fw, extack);
> >  }
> >
> >  const struct devlink_ops nfp_devlink_ops = {
> > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.c b/drivers/net/ethernet/netronome/nfp/nfp_main.c
> > index e672614d2906..742a420152b3 100644
> > --- a/drivers/net/ethernet/netronome/nfp/nfp_main.c
> > +++ b/drivers/net/ethernet/netronome/nfp/nfp_main.c
> > @@ -301,11 +301,10 @@ static int nfp_pcie_sriov_configure(struct pci_dev *pdev, int num_vfs)
> >               return nfp_pcie_sriov_enable(pdev, num_vfs);
> >  }
> >
> > -int nfp_flash_update_common(struct nfp_pf *pf, const char *path,
> > +int nfp_flash_update_common(struct nfp_pf *pf, const struct firmware *fw,
> >                           struct netlink_ext_ack *extack)
> >  {
> >       struct device *dev = &pf->pdev->dev;
> > -     const struct firmware *fw;
> >       struct nfp_nsp *nsp;
> >       int err;
> >
> > @@ -319,24 +318,12 @@ int nfp_flash_update_common(struct nfp_pf *pf, const char *path,
> >               return err;
> >       }
> >
> > -     err = request_firmware_direct(&fw, path, dev);
> > -     if (err) {
> > -             NL_SET_ERR_MSG_MOD(extack,
> > -                                "unable to read flash file from disk");
> > -             goto exit_close_nsp;
> > -     }
> > -
> > -     dev_info(dev, "Please be patient while writing flash image: %s\n",
> > -              path);
> > -
> >       err = nfp_nsp_write_flash(nsp, fw);
> >       if (err < 0)
> > -             goto exit_release_fw;
> > +             goto exit_close_nsp;
> >       dev_info(dev, "Finished writing flash image\n");
> >       err = 0;
> >
> > -exit_release_fw:
> > -     release_firmware(fw);
> >  exit_close_nsp:
> >       nfp_nsp_close(nsp);
> >       return err;
> > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.h b/drivers/net/ethernet/netronome/nfp/nfp_main.h
> > index fa6b13a05941..a7dede946a33 100644
> > --- a/drivers/net/ethernet/netronome/nfp/nfp_main.h
> > +++ b/drivers/net/ethernet/netronome/nfp/nfp_main.h
> > @@ -166,7 +166,7 @@ nfp_pf_map_rtsym(struct nfp_pf *pf, const char *name, const char *sym_fmt,
> >                unsigned int min_size, struct nfp_cpp_area **area);
> >  int nfp_mbox_cmd(struct nfp_pf *pf, u32 cmd, void *in_data, u64 in_length,
> >                void *out_data, u64 out_length);
> > -int nfp_flash_update_common(struct nfp_pf *pf, const char *path,
> > +int nfp_flash_update_common(struct nfp_pf *pf, const struct firmware *fw,
> >                           struct netlink_ext_ack *extack);
> >
> >  enum nfp_dump_diag {
> > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> > index 51d64718ed9f..b41301a5b0df 100644
> > --- a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> > +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> > @@ -15,7 +15,7 @@ static int ionic_dl_flash_update(struct devlink *dl,
> >  {
> >       struct ionic *ionic = devlink_priv(dl);
> >
> > -     return ionic_firmware_update(ionic->lif, params->file_name, extack);
> > +     return ionic_firmware_update(ionic->lif, params->fw, extack);
> >  }
> >
> >  static int ionic_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
> > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
> > index 5c01a9e306d8..0a77e8e810c5 100644
> > --- a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
> > +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
> > @@ -6,7 +6,7 @@
> >
> >  #include <net/devlink.h>
> >
> > -int ionic_firmware_update(struct ionic_lif *lif, const char *fw_name,
> > +int ionic_firmware_update(struct ionic_lif *lif, const struct firmware *fw,
> >                         struct netlink_ext_ack *extack);
> >
> >  struct ionic *ionic_devlink_alloc(struct device *dev);
> > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_fw.c b/drivers/net/ethernet/pensando/ionic/ionic_fw.c
> > index d7bbf336c6f6..fd2ce134f66c 100644
> > --- a/drivers/net/ethernet/pensando/ionic/ionic_fw.c
> > +++ b/drivers/net/ethernet/pensando/ionic/ionic_fw.c
> > @@ -91,7 +91,7 @@ static int ionic_fw_status_long_wait(struct ionic *ionic,
> >       return err;
> >  }
> >
> > -int ionic_firmware_update(struct ionic_lif *lif, const char *fw_name,
> > +int ionic_firmware_update(struct ionic_lif *lif, const struct firmware *fw,
> >                         struct netlink_ext_ack *extack)
> >  {
> >       struct ionic_dev *idev = &lif->ionic->idev;
> > @@ -99,24 +99,15 @@ int ionic_firmware_update(struct ionic_lif *lif, const char *fw_name,
> >       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,
> > @@ -200,7 +191,6 @@ int ionic_firmware_update(struct ionic_lif *lif, const char *fw_name,
> >               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/include/net/devlink.h b/include/net/devlink.h
> > index b01bb9bca5a2..d1d125a33322 100644
> > --- a/include/net/devlink.h
> > +++ b/include/net/devlink.h
> > @@ -19,6 +19,7 @@
> >  #include <net/flow_offload.h>
> >  #include <uapi/linux/devlink.h>
> >  #include <linux/xarray.h>
> > +#include <linux/firmware.h>
> >
> >  #define DEVLINK_RELOAD_STATS_ARRAY_SIZE \
> >       (__DEVLINK_RELOAD_LIMIT_MAX * __DEVLINK_RELOAD_ACTION_MAX)
> > @@ -566,15 +567,15 @@ enum devlink_param_generic_id {
> >
> >  /**
> >   * struct devlink_flash_update_params - Flash Update parameters
> > - * @file_name: the name of the flash firmware file to update from
> > + * @fw: pointer to the firmware data to update from
> >   * @component: the flash component to update
> >   *
> > - * With the exception of file_name, drivers must opt-in to parameters by
> > + * With the exception of fw, drivers must opt-in to parameters by
> >   * setting the appropriate bit in the supported_flash_update_params field in
> >   * their devlink_ops structure.
> >   */
> >  struct devlink_flash_update_params {
> > -     const char *file_name;
> > +     const struct firmware *fw;
> >       const char *component;
> >       u32 overwrite_mask;
> >  };
> > diff --git a/net/core/devlink.c b/net/core/devlink.c
> > index ab4b1368904f..b0121d79ac75 100644
> > --- a/net/core/devlink.c
> > +++ b/net/core/devlink.c
> > @@ -3429,10 +3429,12 @@ EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify);
> >  static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
> >                                      struct genl_info *info)
> >  {
> > -     struct nlattr *nla_component, *nla_overwrite_mask;
> > +     struct nlattr *nla_component, *nla_overwrite_mask, *nla_file_name;
> >       struct devlink_flash_update_params params = {};
> >       struct devlink *devlink = info->user_ptr[0];
> > +     const char *file_name;
> >       u32 supported_params;
> > +     int ret;
> >
> >       if (!devlink->ops->flash_update)
> >               return -EOPNOTSUPP;
> > @@ -3442,8 +3444,6 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
> >
> >       supported_params = devlink->ops->supported_flash_update_params;
> >
> > -     params.file_name = nla_data(info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME]);
> > -
> >       nla_component = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT];
> >       if (nla_component) {
> >               if (!(supported_params & DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT)) {
> > @@ -3467,7 +3467,19 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
> >               params.overwrite_mask = sections.value & sections.selector;
> >       }
> >
> > -     return devlink->ops->flash_update(devlink, &params, info->extack);
> > +     nla_file_name = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME];
> > +     file_name = nla_data(nla_file_name);
> > +     ret = request_firmware(&params.fw, file_name, devlink->dev);
> > +     if (ret) {
> > +             NL_SET_ERR_MSG_ATTR(info->extack, nla_file_name, "failed to locate the requested firmware file");
> > +             return ret;
> > +     }
> > +
> > +     ret = devlink->ops->flash_update(devlink, &params, info->extack);
> > +
> > +     release_firmware(params.fw);
> > +
> > +     return ret;
> >  }
> >
> >  static const struct devlink_param devlink_param_generic[] = {
> > @@ -10225,12 +10237,16 @@ int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
> >               goto out;
> >       }
> >
> > -     params.file_name = file_name;
> > +     ret = request_firmware(&params.fw, file_name, devlink->dev);
> > +     if (ret)
> > +             goto out;
> >
> >       mutex_lock(&devlink->lock);
> >       ret = devlink->ops->flash_update(devlink, &params, NULL);
> >       mutex_unlock(&devlink->lock);
> >
> > +     release_firmware(params.fw);
> > +
> >  out:
> >       rtnl_lock();
> >       dev_put(dev);
> >
Acked-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4182 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [net-next v3 2/2] devlink: move flash end and begin to core devlink
  2020-11-17 20:08 ` [net-next v3 2/2] devlink: move flash end and begin to core devlink Jacob Keller
@ 2020-11-18  9:49   ` Vasundhara Volam
  2020-11-18 16:34   ` Jiri Pirko
  1 sibling, 0 replies; 10+ messages in thread
From: Vasundhara Volam @ 2020-11-18  9:49 UTC (permalink / raw)
  To: Jacob Keller
  Cc: Netdev, Jiri Pirko, Michael Chan, Shannon Nelson, Saeed Mahameed,
	Boris Pismenny, Bin Luo, Jakub Kicinksi

[-- Attachment #1: Type: text/plain, Size: 10611 bytes --]

On Wed, Nov 18, 2020 at 1:41 AM Jacob Keller <jacob.e.keller@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.keller@intel.com>
> Cc: Jiri Pirko <jiri@nvidia.com>
> Cc: Michael Chan <michael.chan@broadcom.com>
> Cc: Shannon Nelson <snelson@pensando.io>
> Cc: Saeed Mahameed <saeedm@nvidia.com>
> Cc: Boris Pismenny <borisp@nvidia.com>
> Cc: Bin Luo <luobin9@huawei.com>
> Cc: Jakub Kicinksi <kuba@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,
>                                       &params);
>  }
> -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,
>                                       &params);
>  }
> -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, &params, 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, &params, NULL);
> +       devlink_flash_update_end_notify(devlink);
>         mutex_unlock(&devlink->lock);
>
>         release_firmware(params.fw);
> --
> 2.29.0
>
Acked-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4182 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [net-next v3 2/2] devlink: move flash end and begin to core devlink
  2020-11-17 20:08 ` [net-next v3 2/2] devlink: move flash end and begin to core devlink Jacob Keller
  2020-11-18  9:49   ` Vasundhara Volam
@ 2020-11-18 16:34   ` Jiri Pirko
  1 sibling, 0 replies; 10+ messages in thread
From: Jiri Pirko @ 2020-11-18 16:34 UTC (permalink / raw)
  To: Jacob Keller
  Cc: netdev, Jiri Pirko, Michael Chan, Shannon Nelson, Saeed Mahameed,
	Boris Pismenny, Bin Luo, Jakub Kicinksi

Tue, Nov 17, 2020 at 09:08:20PM CET, jacob.e.keller@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.keller@intel.com>

Good idea.

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [net-next v3 1/2] devlink: move request_firmware out of driver
  2020-11-17 20:08 ` [net-next v3 1/2] devlink: move request_firmware out of driver Jacob Keller
  2020-11-17 20:10   ` Jacob Keller
@ 2020-11-18 16:38   ` Jiri Pirko
  1 sibling, 0 replies; 10+ messages in thread
From: Jiri Pirko @ 2020-11-18 16:38 UTC (permalink / raw)
  To: Jacob Keller
  Cc: netdev, Jiri Pirko, Michael Chan, Shannon Nelson, Saeed Mahameed,
	Boris Pismenny, Bin Luo, Jakub Kicinksi

Tue, Nov 17, 2020 at 09:08:19PM CET, jacob.e.keller@intel.com wrote:
>All drivers which implement the devlink flash update support, with the
>exception of netdevsim, use either request_firmware or
>request_firmware_direct to locate the firmware file. Rather than having
>each driver do this separately as part of its .flash_update
>implementation, perform the request_firmware within net/core/devlink.c
>
>Replace the file_name parameter in the struct devlink_flash_update_params
>with a pointer to the fw object.
>
>Use request_firmware rather than request_firmware_direct. Although most
>Linux distributions today do not have the fallback mechanism
>implemented, only about half the drivers used the _direct request, as
>compared to the generic request_firmware. In the event that
>a distribution does support the fallback mechanism, the devlink flash
>update ought to be able to use it to provide the firmware contents. For
>distributions which do not support the fallback userspace mechanism,
>there should be essentially no difference between request_firmware and
>request_firmware_direct.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [net-next v3 1/2] devlink: move request_firmware out of driver
  2020-11-17 20:10   ` Jacob Keller
  2020-11-18  9:48     ` Vasundhara Volam
@ 2020-11-18 18:32     ` Jakub Kicinski
  2020-11-18 18:47       ` Keller, Jacob E
  1 sibling, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2020-11-18 18:32 UTC (permalink / raw)
  To: Jacob Keller
  Cc: netdev, Jiri Pirko, Michael Chan, Shannon Nelson, Saeed Mahameed,
	Boris Pismenny, Bin Luo

On Tue, 17 Nov 2020 12:10:49 -0800 Jacob Keller wrote:
> Oof, forgot to metion that the only change since v2 is to fix the typo
> in the commit message pointed out by Shannon. Otherwise, this patch is
> identical and just comes in series with the other change.

Fine by me, although I thought Shannon asked for some changes to debug
prints in ionic?

^ permalink raw reply	[flat|nested] 10+ messages in thread
* RE: [net-next v3 1/2] devlink: move request_firmware out of driver
  2020-11-18 18:32     ` Jakub Kicinski
@ 2020-11-18 18:47       ` Keller, Jacob E
  0 siblings, 0 replies; 10+ messages in thread
From: Keller, Jacob E @ 2020-11-18 18:47 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, Jiri Pirko, Michael Chan, Shannon Nelson, Saeed Mahameed,
	Boris Pismenny, Bin Luo



> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, November 18, 2020 10:32 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org; Jiri Pirko <jiri@nvidia.com>; Michael Chan
> <michael.chan@broadcom.com>; Shannon Nelson <snelson@pensando.io>;
> Saeed Mahameed <saeedm@nvidia.com>; Boris Pismenny
> <borisp@nvidia.com>; Bin Luo <luobin9@huawei.com>
> Subject: Re: [net-next v3 1/2] devlink: move request_firmware out of driver
> 
> On Tue, 17 Nov 2020 12:10:49 -0800 Jacob Keller wrote:
> > Oof, forgot to metion that the only change since v2 is to fix the typo
> > in the commit message pointed out by Shannon. Otherwise, this patch is
> > identical and just comes in series with the other change.
> 
> Fine by me, although I thought Shannon asked for some changes to debug
> prints in ionic?

Oh I might have missed those comments let me go look.

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-11-18 18:48 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-17 20:08 [net-next v3 0/2] devlink: move common flash_update calls to core Jacob Keller
2020-11-17 20:08 ` [net-next v3 1/2] devlink: move request_firmware out of driver Jacob Keller
2020-11-17 20:10   ` Jacob Keller
2020-11-18  9:48     ` Vasundhara Volam
2020-11-18 18:32     ` Jakub Kicinski
2020-11-18 18:47       ` Keller, Jacob E
2020-11-18 16:38   ` Jiri Pirko
2020-11-17 20:08 ` [net-next v3 2/2] devlink: move flash end and begin to core devlink Jacob Keller
2020-11-18  9:49   ` Vasundhara Volam
2020-11-18 16:34   ` Jiri Pirko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).