[PATCH net-next 0/3] devlink: add the ability to update device flash

[PATCH net-next 0/3] devlink: add the ability to update device flash

[Jakub Kicinski]

Hi!

This series is the second step to allow trouble shooting and recovering
devices in bad state without the use of netdevs as handles.  We can
already query FW versions over devlink, now we add the ability to update
the FW.  This will allow drivers to implement some from of "limp-mode"
where the device can't really be used for networking and hence has no
netdev, but we can interrogate it over devlink and fix the broken FW.

Small but nice advantage of devlink is that it only holds the devlink
instance lock during flashing, unlike ethtool which holds rtnl_lock().

Jakub Kicinski (3):
  devlink: add flash update command
  ethtool: add compat for flash update
  nfp: devlink: allow flashing the device via devlink

 .../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 +----------
 include/net/devlink.h                         | 10 ++++
 include/uapi/linux/devlink.h                  |  6 ++
 net/core/devlink.c                            | 60 +++++++++++++++++++
 net/core/ethtool.c                            | 12 +++-
 8 files changed, 141 insertions(+), 35 deletions(-)

-- 
2.19.2

[PATCH net-next 1/3] devlink: add flash update command

[Jakub Kicinski]

Add devlink flash update command. Advanced NICs have firmware
stored in flash and often cryptographically secured. Updating
that flash is handled by management firmware. Ethtool has a
flash update command which served us well, however, it has two
shortcomings:
 - it takes rtnl_lock unnecessarily - really flash update has
   nothing to do with networking, so using a networking device
   as a handle is suboptimal, which leads us to the second one:
 - it requires a functioning netdev - in case device enters an
   error state and can't spawn a netdev (e.g. communication
   with the device fails) there is no netdev to use as a handle
   for flashing.

Devlink already has the ability to report the firmware versions,
now with the ability to update the firmware/flash we will be
able to recover devices in bad state.

To enable updates of sub-components of the FW allow passing
component name.  This name should correspond to one of the
versions reported in devlink info.

v1: - replace target id with component name (Jiri).

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 include/net/devlink.h        |  3 +++
 include/uapi/linux/devlink.h |  6 ++++++
 net/core/devlink.c           | 30 ++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index c6d88759b7d5..18d7a051f412 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -521,6 +521,9 @@ struct devlink_ops {
 				      struct netlink_ext_ack *extack);
 	int (*info_get)(struct devlink *devlink, struct devlink_info_req *req,
 			struct netlink_ext_ack *extack);
+	int (*flash_update)(struct devlink *devlink, const char *file_name,
+			    const char *component,
+			    struct netlink_ext_ack *extack);
 };
 
 static inline void *devlink_priv(struct devlink *devlink)
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 72d9f7c89190..53de8802a000 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -103,6 +103,8 @@ enum devlink_command {
 	DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
 	DEVLINK_CMD_HEALTH_REPORTER_DUMP_CLEAR,
 
+	DEVLINK_CMD_FLASH_UPDATE,
+
 	/* add new commands above here */
 	__DEVLINK_CMD_MAX,
 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
@@ -326,6 +328,10 @@ enum devlink_attr {
 	DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS,		/* u64 */
 	DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD,	/* u64 */
 	DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER,	/* u8 */
+
+	DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME,	/* string */
+	DEVLINK_ATTR_FLASH_UPDATE_COMPONENT,	/* string */
+
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 283c3ed9f25e..bd507e13bb7b 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2660,6 +2660,27 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
 	return devlink->ops->reload(devlink, info->extack);
 }
 
+static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
+				       struct genl_info *info)
+{
+	struct devlink *devlink = info->user_ptr[0];
+	const char *file_name, *component;
+	struct nlattr *nla_component;
+
+	if (!devlink->ops->flash_update)
+		return -EOPNOTSUPP;
+
+	if (!info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME])
+		return -EINVAL;
+	file_name = nla_data(info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME]);
+
+	nla_component = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT];
+	component = nla_component ? nla_data(nla_component) : NULL;
+
+	return devlink->ops->flash_update(devlink, file_name, component,
+					  info->extack);
+}
+
 static const struct devlink_param devlink_param_generic[] = {
 	{
 		.id = DEVLINK_PARAM_GENERIC_ID_INT_ERR_RESET,
@@ -4868,6 +4889,8 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_HEALTH_REPORTER_NAME] = { .type = NLA_NUL_STRING },
 	[DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD] = { .type = NLA_U64 },
 	[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME] = { .type = NLA_NUL_STRING },
+	[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT] = { .type = NLA_NUL_STRING },
 };
 
 static const struct genl_ops devlink_nl_ops[] = {
@@ -5156,6 +5179,13 @@ static const struct genl_ops devlink_nl_ops[] = {
 		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK |
 				  DEVLINK_NL_FLAG_NO_LOCK,
 	},
+	{
+		.cmd = DEVLINK_CMD_FLASH_UPDATE,
+		.doit = devlink_nl_cmd_flash_update,
+		.policy = devlink_nl_policy,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
+	},
 };
 
 static struct genl_family devlink_nl_family __ro_after_init = {
-- 
2.19.2

[PATCH net-next 2/3] ethtool: add compat for flash update

[Jakub Kicinski]

If driver does not support ethtool flash update operation
call into devlink.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 include/net/devlink.h |  7 +++++++
 net/core/devlink.c    | 30 ++++++++++++++++++++++++++++++
 net/core/ethtool.c    | 12 +++++++++---
 3 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 18d7a051f412..a2da49dd9147 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1195,11 +1195,18 @@ devlink_health_report(struct devlink_health_reporter *reporter,
 #if IS_REACHABLE(CONFIG_NET_DEVLINK)
 void devlink_compat_running_version(struct net_device *dev,
 				    char *buf, size_t len);
+int devlink_compat_flash_update(struct net_device *dev, const char *file_name);
 #else
 static inline void
 devlink_compat_running_version(struct net_device *dev, char *buf, size_t len)
 {
 }
+
+static inline int
+devlink_compat_flash_update(struct net_device *dev, const char *file_name)
+{
+	return -EOPNOTSUPP;
+}
 #endif
 
 #endif /* _NET_DEVLINK_H_ */
diff --git a/net/core/devlink.c b/net/core/devlink.c
index bd507e13bb7b..d169b5426d3d 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6435,6 +6435,36 @@ void devlink_compat_running_version(struct net_device *dev,
 	mutex_unlock(&devlink_mutex);
 }
 
+int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
+{
+	struct devlink_port *devlink_port;
+	struct devlink *devlink;
+
+	mutex_lock(&devlink_mutex);
+	list_for_each_entry(devlink, &devlink_list, list) {
+		mutex_lock(&devlink->lock);
+		list_for_each_entry(devlink_port, &devlink->port_list, list) {
+			int ret = -EOPNOTSUPP;
+
+			if (devlink_port->type != DEVLINK_PORT_TYPE_ETH ||
+			    devlink_port->type_dev != dev)
+				continue;
+
+			mutex_unlock(&devlink_mutex);
+			if (devlink->ops->flash_update)
+				ret = devlink->ops->flash_update(devlink,
+								 file_name,
+								 NULL, NULL);
+			mutex_unlock(&devlink->lock);
+			return ret;
+		}
+		mutex_unlock(&devlink->lock);
+	}
+	mutex_unlock(&devlink_mutex);
+
+	return -EOPNOTSUPP;
+}
+
 static int __init devlink_module_init(void)
 {
 	return genl_register_family(&devlink_nl_family);
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index d2c47cdf25da..1320e8dce559 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -2038,11 +2038,17 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev,
 
 	if (copy_from_user(&efl, useraddr, sizeof(efl)))
 		return -EFAULT;
+	efl.data[ETHTOOL_FLASH_MAX_FILENAME - 1] = 0;
 
-	if (!dev->ethtool_ops->flash_device)
-		return -EOPNOTSUPP;
+	if (!dev->ethtool_ops->flash_device) {
+		int ret;
 
-	efl.data[ETHTOOL_FLASH_MAX_FILENAME - 1] = 0;
+		rtnl_unlock();
+		ret = devlink_compat_flash_update(dev, efl.data);
+		rtnl_lock();
+
+		return ret;
+	}
 
 	return dev->ethtool_ops->flash_device(dev, &efl);
 }
-- 
2.19.2

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

[Jakub Kicinski]

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,
+			 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 = {
-- 
2.19.2

Re: [PATCH net-next 2/3] ethtool: add compat for flash update

[Michal Kubecek]

On Thu, Feb 14, 2019 at 01:40:45PM -0800, Jakub Kicinski wrote:
> If driver does not support ethtool flash update operation
> call into devlink.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
...
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index bd507e13bb7b..d169b5426d3d 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -6435,6 +6435,36 @@ void devlink_compat_running_version(struct net_device *dev,
>  	mutex_unlock(&devlink_mutex);
>  }
>  
> +int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
> +{
> +	struct devlink_port *devlink_port;
> +	struct devlink *devlink;
> +
> +	mutex_lock(&devlink_mutex);
> +	list_for_each_entry(devlink, &devlink_list, list) {
> +		mutex_lock(&devlink->lock);
> +		list_for_each_entry(devlink_port, &devlink->port_list, list) {
> +			int ret = -EOPNOTSUPP;
> +
> +			if (devlink_port->type != DEVLINK_PORT_TYPE_ETH ||
> +			    devlink_port->type_dev != dev)
> +				continue;
> +
> +			mutex_unlock(&devlink_mutex);
> +			if (devlink->ops->flash_update)
> +				ret = devlink->ops->flash_update(devlink,
> +								 file_name,
> +								 NULL, NULL);
> +			mutex_unlock(&devlink->lock);
> +			return ret;
> +		}
> +		mutex_unlock(&devlink->lock);
> +	}
> +	mutex_unlock(&devlink_mutex);
> +
> +	return -EOPNOTSUPP;
> +}
> +
>  static int __init devlink_module_init(void)
>  {
>  	return genl_register_family(&devlink_nl_family);

We already have similar lookup in devlink_compat_running_version() (the
only difference seems to be that we keep holding devlink_mutex until the
end there) and it's likely the net_device -> devlink lookup will be
needed in more places in the future. How about having a helper for it?

I also wonder how does the lookup scale. But I don't have clear idea
how long the lists can become in real life and the ethtool operations
are not really time critical.

Michal Kubecek

Re: [PATCH net-next 1/3] devlink: add flash update command

[Jiri Pirko]

Thu, Feb 14, 2019 at 10:40:44PM CET, jakub.kicinski@netronome.com wrote:
>Add devlink flash update command. Advanced NICs have firmware
>stored in flash and often cryptographically secured. Updating
>that flash is handled by management firmware. Ethtool has a
>flash update command which served us well, however, it has two
>shortcomings:
> - it takes rtnl_lock unnecessarily - really flash update has
>   nothing to do with networking, so using a networking device
>   as a handle is suboptimal, which leads us to the second one:
> - it requires a functioning netdev - in case device enters an
>   error state and can't spawn a netdev (e.g. communication
>   with the device fails) there is no netdev to use as a handle
>   for flashing.
>
>Devlink already has the ability to report the firmware versions,
>now with the ability to update the firmware/flash we will be
>able to recover devices in bad state.
>
>To enable updates of sub-components of the FW allow passing
>component name.  This name should correspond to one of the
>versions reported in devlink info.
>
>v1: - replace target id with component name (Jiri).
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Jiri Pirko <jiri@mellanox.com>

Re: [PATCH net-next 2/3] ethtool: add compat for flash update

[Jiri Pirko]

Thu, Feb 14, 2019 at 10:40:45PM CET, jakub.kicinski@netronome.com wrote:
>If driver does not support ethtool flash update operation
>call into devlink.
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Jiri Pirko <jiri@mellanox.com>

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

[Jiri Pirko]

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
>

Re: [PATCH net-next 2/3] ethtool: add compat for flash update

[Jiri Pirko]

Fri, Feb 15, 2019 at 09:53:15AM CET, mkubecek@suse.cz wrote:
>On Thu, Feb 14, 2019 at 01:40:45PM -0800, Jakub Kicinski wrote:
>> If driver does not support ethtool flash update operation
>> call into devlink.
>> 
>> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>> ---
>...
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index bd507e13bb7b..d169b5426d3d 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -6435,6 +6435,36 @@ void devlink_compat_running_version(struct net_device *dev,
>>  	mutex_unlock(&devlink_mutex);
>>  }
>>  
>> +int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
>> +{
>> +	struct devlink_port *devlink_port;
>> +	struct devlink *devlink;
>> +
>> +	mutex_lock(&devlink_mutex);
>> +	list_for_each_entry(devlink, &devlink_list, list) {
>> +		mutex_lock(&devlink->lock);
>> +		list_for_each_entry(devlink_port, &devlink->port_list, list) {
>> +			int ret = -EOPNOTSUPP;
>> +
>> +			if (devlink_port->type != DEVLINK_PORT_TYPE_ETH ||
>> +			    devlink_port->type_dev != dev)
>> +				continue;
>> +
>> +			mutex_unlock(&devlink_mutex);
>> +			if (devlink->ops->flash_update)
>> +				ret = devlink->ops->flash_update(devlink,
>> +								 file_name,
>> +								 NULL, NULL);
>> +			mutex_unlock(&devlink->lock);
>> +			return ret;
>> +		}
>> +		mutex_unlock(&devlink->lock);
>> +	}
>> +	mutex_unlock(&devlink_mutex);
>> +
>> +	return -EOPNOTSUPP;
>> +}
>> +
>>  static int __init devlink_module_init(void)
>>  {
>>  	return genl_register_family(&devlink_nl_family);
>
>We already have similar lookup in devlink_compat_running_version() (the
>only difference seems to be that we keep holding devlink_mutex until the
>end there) and it's likely the net_device -> devlink lookup will be
>needed in more places in the future. How about having a helper for it?
>
>I also wonder how does the lookup scale. But I don't have clear idea
>how long the lists can become in real life and the ethtool operations
>are not really time critical.

Another thing is, that you don't really have to do the lookup. If you
have struct net_device *dev inside the driver, you can get the devlink
instance according to that. So it is just a matter of another ndo I
guess.


>
>Michal Kubecek

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

[Michal Kubecek]

On Thu, Feb 14, 2019 at 01:40:46PM -0800, Jakub Kicinski 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>
> ---
...
> 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 = {

Out of curiosity: why don't you drop the ethtool_ops callback and let
the fallback introduced in patch 2/3 do the fallback? Is it to preserve
the check of flash->region or to avoid the lookup? My understanding of
the previous patch was that it would allow new drivers providing the
devlink callback to get rid of ethtool_ops one.

Michal Kubecek

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

[Jakub Kicinski]

On Fri, 15 Feb 2019 11:15:14 +0100, Jiri Pirko wrote:
> > static const struct ethtool_ops nfp_net_ethtool_ops = {  
> 
> Why don't you use the compat fallback? I think you should.

You and Michal both asked the same so let me answer the first to ask :)
- if devlink is built as a module the fallback is not reachable.

Re: [PATCH net-next 2/3] ethtool: add compat for flash update

[Jakub Kicinski]

On Fri, 15 Feb 2019 11:17:13 +0100, Jiri Pirko wrote:
> >>  static int __init devlink_module_init(void)
> >>  {
> >>  	return genl_register_family(&devlink_nl_family);  
> >
> >We already have similar lookup in devlink_compat_running_version() (the
> >only difference seems to be that we keep holding devlink_mutex until the
> >end there) and it's likely the net_device -> devlink lookup will be
> >needed in more places in the future. How about having a helper for it?

That's the last one that's on my radar, info and flashing are the big
per-ASIC items.  If we need more hopefully we can put the compat in the
shiny new ethnl user space :)

> >I also wonder how does the lookup scale. But I don't have clear idea
> >how long the lists can become in real life and the ethtool operations
> >are not really time critical.  
> 
> Another thing is, that you don't really have to do the lookup. If you
> have struct net_device *dev inside the driver, you can get the devlink
> instance according to that. So it is just a matter of another ndo I
> guess.

Sounds like more repetitive per driver code to get to information the
core already has.  We can do a rhashtable if it really ever becomes an
issue (which is unlikely, how many ports are you going to have on a
box.. and this is not a dump - one call one lookup).

Re: [PATCH net-next 0/3] devlink: add the ability to update device flash

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Thu, 14 Feb 2019 13:40:43 -0800

> This series is the second step to allow trouble shooting and recovering
> devices in bad state without the use of netdevs as handles.  We can
> already query FW versions over devlink, now we add the ability to update
> the FW.  This will allow drivers to implement some from of "limp-mode"
> where the device can't really be used for networking and hence has no
> netdev, but we can interrogate it over devlink and fix the broken FW.
> 
> Small but nice advantage of devlink is that it only holds the devlink
> instance lock during flashing, unlike ethtool which holds rtnl_lock().

Series applied, thanks Jakub.

Looking at the feedback, the things requested can be done as follow-ups
quite easily.

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

[Jiri Pirko]

Fri, Feb 15, 2019 at 04:44:29PM CET, jakub.kicinski@netronome.com wrote:
>On Fri, 15 Feb 2019 11:15:14 +0100, Jiri Pirko wrote:
>> > static const struct ethtool_ops nfp_net_ethtool_ops = {  
>> 
>> Why don't you use the compat fallback? I think you should.
>
>You and Michal both asked the same so let me answer the first to ask :)
>- if devlink is built as a module the fallback is not reachable.

So the fallback is not really good as you can't use it for real drivers
anyway. Odd. Maybe we should compile devlink in without possibility to
have it as module.

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

[Jakub Kicinski]

On Tue, 19 Feb 2019 10:19:42 +0100, Jiri Pirko wrote:
> Fri, Feb 15, 2019 at 04:44:29PM CET, jakub.kicinski@netronome.com wrote:
> >On Fri, 15 Feb 2019 11:15:14 +0100, Jiri Pirko wrote:  
> >> > static const struct ethtool_ops nfp_net_ethtool_ops = {    
> >> 
> >> Why don't you use the compat fallback? I think you should.  
> >
> >You and Michal both asked the same so let me answer the first to ask :)
> >- if devlink is built as a module the fallback is not reachable.  
> 
> So the fallback is not really good as you can't use it for real drivers
> anyway. Odd. Maybe we should compile devlink in without possibility to
> have it as module.

Ack, I'll make devlink a bool.

I need a little extra time, I forgot that nfp's flower offload still
doesn't register all ports (using your port flavour infrastructure).

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

[Jiri Pirko]

Wed, Feb 20, 2019 at 01:49:26AM CET, jakub.kicinski@netronome.com wrote:
>On Tue, 19 Feb 2019 10:19:42 +0100, Jiri Pirko wrote:
>> Fri, Feb 15, 2019 at 04:44:29PM CET, jakub.kicinski@netronome.com wrote:
>> >On Fri, 15 Feb 2019 11:15:14 +0100, Jiri Pirko wrote:  
>> >> > static const struct ethtool_ops nfp_net_ethtool_ops = {    
>> >> 
>> >> Why don't you use the compat fallback? I think you should.  
>> >
>> >You and Michal both asked the same so let me answer the first to ask :)
>> >- if devlink is built as a module the fallback is not reachable.  
>> 
>> So the fallback is not really good as you can't use it for real drivers
>> anyway. Odd. Maybe we should compile devlink in without possibility to
>> have it as module.
>
>Ack, I'll make devlink a bool.

Thanks!

>
>I need a little extra time, I forgot that nfp's flower offload still
>doesn't register all ports (using your port flavour infrastructure).

Finally :)

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

[Florian Fainelli]

On 2/19/2019 4:49 PM, Jakub Kicinski wrote:
> On Tue, 19 Feb 2019 10:19:42 +0100, Jiri Pirko wrote:
>> Fri, Feb 15, 2019 at 04:44:29PM CET, jakub.kicinski@netronome.com wrote:
>>> On Fri, 15 Feb 2019 11:15:14 +0100, Jiri Pirko wrote:  
>>>>> static const struct ethtool_ops nfp_net_ethtool_ops = {    
>>>>
>>>> Why don't you use the compat fallback? I think you should.  
>>>
>>> You and Michal both asked the same so let me answer the first to ask :)
>>> - if devlink is built as a module the fallback is not reachable.  
>>
>> So the fallback is not really good as you can't use it for real drivers
>> anyway. Odd. Maybe we should compile devlink in without possibility to
>> have it as module.
> 
> Ack, I'll make devlink a bool.

Meh how about those poor and memory constrained embedded systems?
Ideally ethtool should/could have been modular as well, but that ship
has now sailed.

> 
> I need a little extra time, I forgot that nfp's flower offload still
> doesn't register all ports (using your port flavour infrastructure).
> 

We have had similar issues with PHYLIB before where we wanted
net/core/ethtool.c to be able to call into generic PHYLIB functions to
obtain PHY statistics, an inline helper that de-references the PHY
device's driver function pointers solved that (look for
phy_ethtool_get_{strings,sset,stats}) while letting PHYLIB remain modular.

devlink_compat_flash_update() is a bit big to be inlined, but why not?

If we make sure we always provide a devlink_mutex and devlink_list that
symbols such that this builds wheter CONFIG_DEVLINK=y|m then everything
else can be determined at runtime whether devlink.ko is loaded or not.

Does that make sense?
-- 
Florian

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

[Jakub Kicinski]

On Wed, 20 Feb 2019 18:59:05 -0800, Florian Fainelli wrote:
> On 2/19/2019 4:49 PM, Jakub Kicinski wrote:
> > On Tue, 19 Feb 2019 10:19:42 +0100, Jiri Pirko wrote:  
> >> Fri, Feb 15, 2019 at 04:44:29PM CET, jakub.kicinski@netronome.com wrote:  
> >>> On Fri, 15 Feb 2019 11:15:14 +0100, Jiri Pirko wrote:    
> >>>>> static const struct ethtool_ops nfp_net_ethtool_ops = {      
> >>>>
> >>>> Why don't you use the compat fallback? I think you should.    
> >>>
> >>> You and Michal both asked the same so let me answer the first to ask :)
> >>> - if devlink is built as a module the fallback is not reachable.    
> >>
> >> So the fallback is not really good as you can't use it for real drivers
> >> anyway. Odd. Maybe we should compile devlink in without possibility to
> >> have it as module.  
> > 
> > Ack, I'll make devlink a bool.  
> 
> Meh how about those poor and memory constrained embedded systems?
> Ideally ethtool should/could have been modular as well, but that ship
> has now sailed.

To be clear - I'm not going to add a dependency.  You'll still be able
to use ethtool without devlink (granted, you won't be able to flash the
device).

It's not immediately clear to me how devlink being modular saves memory.
The way I see it you either:

 (a) not have networking or need any devlink related functionality, and
     therefore do DEVLINK=n;

or:

 (b) have a networking device driver built and loaded which will depend
     on devlink, so devlink will practically speaking always be loaded,
     even if its =m.

> > I need a little extra time, I forgot that nfp's flower offload still
> > doesn't register all ports (using your port flavour infrastructure).
> 
> We have had similar issues with PHYLIB before where we wanted
> net/core/ethtool.c to be able to call into generic PHYLIB functions to
> obtain PHY statistics, an inline helper that de-references the PHY
> device's driver function pointers solved that (look for
> phy_ethtool_get_{strings,sset,stats}) while letting PHYLIB remain modular.
> 
> devlink_compat_flash_update() is a bit big to be inlined, but why not?
> 
> If we make sure we always provide a devlink_mutex and devlink_list that
> symbols such that this builds wheter CONFIG_DEVLINK=y|m then everything
> else can be determined at runtime whether devlink.ko is loaded or not.
> 
> Does that make sense?

I think so, we'd have to have a little object that would always be
built-in when DEVLINK=m, and therefore accessible?

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

[Jiri Pirko]

Thu, Feb 21, 2019 at 03:59:05AM CET, f.fainelli@gmail.com wrote:
>
>
>On 2/19/2019 4:49 PM, Jakub Kicinski wrote:
>> On Tue, 19 Feb 2019 10:19:42 +0100, Jiri Pirko wrote:
>>> Fri, Feb 15, 2019 at 04:44:29PM CET, jakub.kicinski@netronome.com wrote:
>>>> On Fri, 15 Feb 2019 11:15:14 +0100, Jiri Pirko wrote:  
>>>>>> static const struct ethtool_ops nfp_net_ethtool_ops = {    
>>>>>
>>>>> Why don't you use the compat fallback? I think you should.  
>>>>
>>>> You and Michal both asked the same so let me answer the first to ask :)
>>>> - if devlink is built as a module the fallback is not reachable.  
>>>
>>> So the fallback is not really good as you can't use it for real drivers
>>> anyway. Odd. Maybe we should compile devlink in without possibility to
>>> have it as module.
>> 
>> Ack, I'll make devlink a bool.
>
>Meh how about those poor and memory constrained embedded systems?
>Ideally ethtool should/could have been modular as well, but that ship
>has now sailed.
>
>> 
>> I need a little extra time, I forgot that nfp's flower offload still
>> doesn't register all ports (using your port flavour infrastructure).
>> 
>
>We have had similar issues with PHYLIB before where we wanted
>net/core/ethtool.c to be able to call into generic PHYLIB functions to
>obtain PHY statistics, an inline helper that de-references the PHY
>device's driver function pointers solved that (look for
>phy_ethtool_get_{strings,sset,stats}) while letting PHYLIB remain modular.
>
>devlink_compat_flash_update() is a bit big to be inlined, but why not?

Others compat functions are going to come.

>
>If we make sure we always provide a devlink_mutex and devlink_list that
>symbols such that this builds wheter CONFIG_DEVLINK=y|m then everything
>else can be determined at runtime whether devlink.ko is loaded or not.
>
>Does that make sense?
>-- 
>Florian

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

[Michal Kubecek]

On Wed, Feb 20, 2019 at 06:59:05PM -0800, Florian Fainelli wrote:
> On 2/19/2019 4:49 PM, Jakub Kicinski wrote:
> > On Tue, 19 Feb 2019 10:19:42 +0100, Jiri Pirko wrote:
> >> Fri, Feb 15, 2019 at 04:44:29PM CET, jakub.kicinski@netronome.com wrote:
> >>> On Fri, 15 Feb 2019 11:15:14 +0100, Jiri Pirko wrote:  
> >>>>> static const struct ethtool_ops nfp_net_ethtool_ops = {    
> >>>>
> >>>> Why don't you use the compat fallback? I think you should.  
> >>>
> >>> You and Michal both asked the same so let me answer the first to ask :)
> >>> - if devlink is built as a module the fallback is not reachable.  
> >>
> >> So the fallback is not really good as you can't use it for real drivers
> >> anyway. Odd. Maybe we should compile devlink in without possibility to
> >> have it as module.
> > 
> > Ack, I'll make devlink a bool.
> 
> Meh how about those poor and memory constrained embedded systems?
> Ideally ethtool should/could have been modular as well, but that ship
> has now sailed.

I would certainly like to make the ioctl interface optional once we
reach the end of "phase one", i.e. make ioctl-less ethtool possible.
Looking at the code, I don't see an obvious reason why it couldn't be
modular.

There seem to be only few functions in net/core/ethtool.c which are
called from outside and all seem to be simple helpers not really tied to
the rest of the code, except for netdev_rss_key variable (needed for
/proc/sys/net/core/netdev_rss_key). Some of them could even be inline.
We could always put these into some net/ethtool/stub.c (ethtool-stub.c)
and build unconditionally.

So if keeping the option to have devlink (and ethtool) as a module is
really desirable, I believe it can be done even now (unless I missed
something important).

> We have had similar issues with PHYLIB before where we wanted
> net/core/ethtool.c to be able to call into generic PHYLIB functions to
> obtain PHY statistics, an inline helper that de-references the PHY
> device's driver function pointers solved that (look for
> phy_ethtool_get_{strings,sset,stats}) while letting PHYLIB remain modular.

There is also something similar in netfilter - nf_ct_hook, nfnl_ct_hook
or nf_ipv6_ops.

> devlink_compat_flash_update() is a bit big to be inlined, but why not?

Most of it seems to be the lookup which I'm planning to factor out as
a separate helper. But that would also need to be available to external
code, of course.

Michal