Re: [PATCH net-next 3/3] nfp: devlink: allow flashing the device via devlink

[Jiri Pirko <jiri@resnulli.us>]

Thu, Feb 14, 2019 at 10:40:46PM CET, jakub.kicinski@netronome.com wrote:
>Devlink now allows updating device flash.  Implement this
>callback.
>
>Compared to ethtool update we no longer have to release
>the networking locks - devlink doesn't take them.
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>---
> .../net/ethernet/netronome/nfp/nfp_devlink.c  | 10 +++++
> drivers/net/ethernet/netronome/nfp/nfp_main.c | 41 +++++++++++++++++++
> drivers/net/ethernet/netronome/nfp/nfp_main.h |  2 +
> .../ethernet/netronome/nfp/nfp_net_ethtool.c  | 35 ++--------------
> 4 files changed, 56 insertions(+), 32 deletions(-)
>
>diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>index 080a301f379e..db2da99f6aa7 100644
>--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>@@ -330,6 +330,15 @@ nfp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
> 	return err;
> }
> 
>+static int
>+nfp_devlink_flash_update(struct devlink *devlink, const char *path,

devlink code calles "path" a "file_name". It is good to be consistent in
this.

Other than this, the patch looks fine:


>+			 const char *component, struct netlink_ext_ack *extack)
>+{
>+	if (component)
>+		return -EOPNOTSUPP;
>+	return nfp_flash_update_common(devlink_priv(devlink), path, extack);
>+}
>+
> const struct devlink_ops nfp_devlink_ops = {
> 	.port_split		= nfp_devlink_port_split,
> 	.port_unsplit		= nfp_devlink_port_unsplit,
>@@ -338,6 +347,7 @@ const struct devlink_ops nfp_devlink_ops = {
> 	.eswitch_mode_get	= nfp_devlink_eswitch_mode_get,
> 	.eswitch_mode_set	= nfp_devlink_eswitch_mode_set,
> 	.info_get		= nfp_devlink_info_get,
>+	.flash_update		= nfp_devlink_flash_update,
> };
> 
> int nfp_devlink_port_register(struct nfp_app *app, struct nfp_port *port)
>diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.c b/drivers/net/ethernet/netronome/nfp/nfp_main.c
>index 6c10e8d119e4..f4c8776e42b6 100644
>--- a/drivers/net/ethernet/netronome/nfp/nfp_main.c
>+++ b/drivers/net/ethernet/netronome/nfp/nfp_main.c
>@@ -300,6 +300,47 @@ 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,
>+			    struct netlink_ext_ack *extack)
>+{
>+	struct device *dev = &pf->pdev->dev;
>+	const struct firmware *fw;
>+	struct nfp_nsp *nsp;
>+	int err;
>+
>+	nsp = nfp_nsp_open(pf->cpp);
>+	if (IS_ERR(nsp)) {
>+		err = PTR_ERR(nsp);
>+		if (extack)
>+			NL_SET_ERR_MSG_MOD(extack, "can't access NSP");
>+		else
>+			dev_err(dev, "Failed to access the NSP: %d\n", err);
>+		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;
>+	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;
>+}
>+
> static const struct firmware *
> nfp_net_fw_request(struct pci_dev *pdev, struct nfp_pf *pf, const char *name)
> {
>diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.h b/drivers/net/ethernet/netronome/nfp/nfp_main.h
>index a3613a2e0aa5..b7211f200d22 100644
>--- a/drivers/net/ethernet/netronome/nfp/nfp_main.h
>+++ b/drivers/net/ethernet/netronome/nfp/nfp_main.h
>@@ -164,6 +164,8 @@ 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,
>+			    struct netlink_ext_ack *extack);
> 
> enum nfp_dump_diag {
> 	NFP_DUMP_NSP_DIAG = 0,
>diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
>index cb9c512abc76..8f189149efc5 100644
>--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
>+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
>@@ -1237,11 +1237,8 @@ static int nfp_net_set_channels(struct net_device *netdev,
> static int
> nfp_net_flash_device(struct net_device *netdev, struct ethtool_flash *flash)
> {
>-	const struct firmware *fw;
> 	struct nfp_app *app;
>-	struct nfp_nsp *nsp;
>-	struct device *dev;
>-	int err;
>+	int ret;
> 
> 	if (flash->region != ETHTOOL_FLASH_ALL_REGIONS)
> 		return -EOPNOTSUPP;
>@@ -1250,39 +1247,13 @@ nfp_net_flash_device(struct net_device *netdev, struct ethtool_flash *flash)
> 	if (!app)
> 		return -EOPNOTSUPP;
> 
>-	dev = &app->pdev->dev;
>-
>-	nsp = nfp_nsp_open(app->cpp);
>-	if (IS_ERR(nsp)) {
>-		err = PTR_ERR(nsp);
>-		dev_err(dev, "Failed to access the NSP: %d\n", err);
>-		return err;
>-	}
>-
>-	err = request_firmware_direct(&fw, flash->data, dev);
>-	if (err)
>-		goto exit_close_nsp;
>-
>-	dev_info(dev, "Please be patient while writing flash image: %s\n",
>-		 flash->data);
> 	dev_hold(netdev);
> 	rtnl_unlock();
>-
>-	err = nfp_nsp_write_flash(nsp, fw);
>-	if (err < 0) {
>-		dev_err(dev, "Flash write failed: %d\n", err);
>-		goto exit_rtnl_lock;
>-	}
>-	dev_info(dev, "Finished writing flash image\n");
>-
>-exit_rtnl_lock:
>+	ret = nfp_flash_update_common(app->pf, flash->data, NULL);
> 	rtnl_lock();
> 	dev_put(netdev);
>-	release_firmware(fw);
> 
>-exit_close_nsp:
>-	nfp_nsp_close(nsp);
>-	return err;
>+	return ret;
> }
> 
> static const struct ethtool_ops nfp_net_ethtool_ops = {

Why don't you use the compat fallback? I think you should.


>-- 
>2.19.2
>