[PATCH net-next v2 0/5] devlink: make ethtool compat reliable

[PATCH net-next v2 0/5] devlink: make ethtool compat reliable

[Jakub Kicinski]

Hi!

This is a follow up to the series which added device flash
updates via devlink. I went with the approach of adding a
new NDO in the end. It seems to end up looking cleaner
(the port favour patches are coming regardless ;)).

First patch removes the option to build devlink as a module.
Users can still decide to not build it, but the module option
ends up not being worth the maintenance cost.

Second patch fixes a potential issue with ethtool code, which
will become even more problematic as the NDO introduced by
the third patch does not hold RTNL. The new NDO allows the core
to get the devlink instance based on a netdev pointer.

Next the NDO is implemented in the NFP, and ethtool flashing
ops removed.

v2: add netdev_to_devlink() in patch 3.

Jakub Kicinski (5):
  net: devlink: turn devlink into a built-in
  ethtool: hold a reference to the netdevice around devlink compat
  devlink: create a special NDO for getting the devlink instance
  nfp: add .ndo_get_devlink
  nfp: remove ethtool flashing fallback

 drivers/infiniband/hw/bnxt_re/Kconfig         |  1 -
 drivers/infiniband/hw/mlx4/Kconfig            |  1 -
 drivers/net/Kconfig                           |  1 -
 drivers/net/ethernet/broadcom/Kconfig         |  1 -
 drivers/net/ethernet/cavium/Kconfig           |  1 -
 drivers/net/ethernet/mellanox/mlx4/Kconfig    |  1 -
 .../net/ethernet/mellanox/mlx5/core/Kconfig   |  1 -
 drivers/net/ethernet/mellanox/mlxsw/Kconfig   |  1 -
 drivers/net/ethernet/netronome/Kconfig        |  1 -
 drivers/net/ethernet/netronome/nfp/nfp_app.h  |  2 +
 .../net/ethernet/netronome/nfp/nfp_devlink.c  | 11 +++
 .../ethernet/netronome/nfp/nfp_net_common.c   |  1 +
 .../ethernet/netronome/nfp/nfp_net_ethtool.c  | 24 ------
 .../net/ethernet/netronome/nfp/nfp_net_repr.c |  1 +
 include/linux/netdevice.h                     |  6 ++
 include/net/devlink.h                         | 19 +++--
 net/Kconfig                                   | 11 +--
 net/core/devlink.c                            | 73 +++++--------------
 net/core/ethtool.c                            | 11 ++-
 net/dsa/Kconfig                               |  2 +-
 20 files changed, 61 insertions(+), 109 deletions(-)

-- 
2.19.2

[PATCH net-next v2 1/5] net: devlink: turn devlink into a built-in

[Jakub Kicinski]

Being able to build devlink as a module causes growing pains.
First all drivers had to add a meta dependency to make sure
they are not built in when devlink is built as a module.  Now
we are struggling to invoke ethtool compat code reliably.

Make devlink code built-in, users can still not build it at
all but the dynamically loadable module option is removed.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/infiniband/hw/bnxt_re/Kconfig           |  1 -
 drivers/infiniband/hw/mlx4/Kconfig              |  1 -
 drivers/net/Kconfig                             |  1 -
 drivers/net/ethernet/broadcom/Kconfig           |  1 -
 drivers/net/ethernet/cavium/Kconfig             |  1 -
 drivers/net/ethernet/mellanox/mlx4/Kconfig      |  1 -
 drivers/net/ethernet/mellanox/mlx5/core/Kconfig |  1 -
 drivers/net/ethernet/mellanox/mlxsw/Kconfig     |  1 -
 drivers/net/ethernet/netronome/Kconfig          |  1 -
 include/net/devlink.h                           | 10 ++++------
 net/Kconfig                                     | 11 +----------
 net/core/devlink.c                              | 15 ++-------------
 net/dsa/Kconfig                                 |  2 +-
 13 files changed, 8 insertions(+), 39 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/Kconfig b/drivers/infiniband/hw/bnxt_re/Kconfig
index 18f5ed082f41..19982a4a9bba 100644
--- a/drivers/infiniband/hw/bnxt_re/Kconfig
+++ b/drivers/infiniband/hw/bnxt_re/Kconfig
@@ -1,7 +1,6 @@
 config INFINIBAND_BNXT_RE
     tristate "Broadcom Netxtreme HCA support"
     depends on ETHERNET && NETDEVICES && PCI && INET && DCB
-    depends on MAY_USE_DEVLINK
     select NET_VENDOR_BROADCOM
     select BNXT
     ---help---
diff --git a/drivers/infiniband/hw/mlx4/Kconfig b/drivers/infiniband/hw/mlx4/Kconfig
index d1de3285fd88..4e9936731867 100644
--- a/drivers/infiniband/hw/mlx4/Kconfig
+++ b/drivers/infiniband/hw/mlx4/Kconfig
@@ -2,7 +2,6 @@ config MLX4_INFINIBAND
 	tristate "Mellanox ConnectX HCA support"
 	depends on NETDEVICES && ETHERNET && PCI && INET
 	depends on INFINIBAND_USER_ACCESS || !INFINIBAND_USER_ACCESS
-	depends on MAY_USE_DEVLINK
 	select NET_VENDOR_MELLANOX
 	select MLX4_CORE
 	---help---
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 6210757772ed..5e4ca082cfcd 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -505,7 +505,6 @@ source "drivers/net/hyperv/Kconfig"
 config NETDEVSIM
 	tristate "Simulated networking device"
 	depends on DEBUG_FS
-	depends on MAY_USE_DEVLINK
 	help
 	  This driver is a developer testing tool and software model that can
 	  be used to test various control path networking APIs, especially
diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig
index c1d3ee9baf7e..716bfbba59cf 100644
--- a/drivers/net/ethernet/broadcom/Kconfig
+++ b/drivers/net/ethernet/broadcom/Kconfig
@@ -194,7 +194,6 @@ config SYSTEMPORT
 config BNXT
 	tristate "Broadcom NetXtreme-C/E support"
 	depends on PCI
-	depends on MAY_USE_DEVLINK
 	select FW_LOADER
 	select LIBCRC32C
 	---help---
diff --git a/drivers/net/ethernet/cavium/Kconfig b/drivers/net/ethernet/cavium/Kconfig
index 05f4a3b21e29..6650e2a5f171 100644
--- a/drivers/net/ethernet/cavium/Kconfig
+++ b/drivers/net/ethernet/cavium/Kconfig
@@ -64,7 +64,6 @@ config CAVIUM_PTP
 config LIQUIDIO
 	tristate "Cavium LiquidIO support"
 	depends on 64BIT && PCI
-	depends on MAY_USE_DEVLINK
 	depends on PCI
 	imply PTP_1588_CLOCK
 	select FW_LOADER
diff --git a/drivers/net/ethernet/mellanox/mlx4/Kconfig b/drivers/net/ethernet/mellanox/mlx4/Kconfig
index f200b8c420d5..ff8057ed97ee 100644
--- a/drivers/net/ethernet/mellanox/mlx4/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlx4/Kconfig
@@ -4,7 +4,6 @@
 
 config MLX4_EN
 	tristate "Mellanox Technologies 1/10/40Gbit Ethernet support"
-	depends on MAY_USE_DEVLINK
 	depends on PCI && NETDEVICES && ETHERNET && INET
 	select MLX4_CORE
 	imply PTP_1588_CLOCK
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
index 37a551436e4a..6debffb8336b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
@@ -4,7 +4,6 @@
 
 config MLX5_CORE
 	tristate "Mellanox 5th generation network adapters (ConnectX series) core driver"
-	depends on MAY_USE_DEVLINK
 	depends on PCI
 	imply PTP_1588_CLOCK
 	imply VXLAN
diff --git a/drivers/net/ethernet/mellanox/mlxsw/Kconfig b/drivers/net/ethernet/mellanox/mlxsw/Kconfig
index b9a25aed5d11..9c195dfed031 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlxsw/Kconfig
@@ -4,7 +4,6 @@
 
 config MLXSW_CORE
 	tristate "Mellanox Technologies Switch ASICs support"
-	depends on MAY_USE_DEVLINK
 	---help---
 	  This driver supports Mellanox Technologies Switch ASICs family.
 
diff --git a/drivers/net/ethernet/netronome/Kconfig b/drivers/net/ethernet/netronome/Kconfig
index 66f15b05b65e..549898d5d450 100644
--- a/drivers/net/ethernet/netronome/Kconfig
+++ b/drivers/net/ethernet/netronome/Kconfig
@@ -19,7 +19,6 @@ config NFP
 	tristate "Netronome(R) NFP4000/NFP6000 NIC driver"
 	depends on PCI && PCI_MSI
 	depends on VXLAN || VXLAN=n
-	depends on MAY_USE_DEVLINK
 	---help---
 	  This driver supports the Netronome(R) NFP4000/NFP6000 based
 	  cards working as a advanced Ethernet NIC.  It works with both
diff --git a/include/net/devlink.h b/include/net/devlink.h
index a2da49dd9147..f9f7fe974652 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -707,6 +707,10 @@ devlink_health_reporter_priv(struct devlink_health_reporter *reporter);
 int devlink_health_report(struct devlink_health_reporter *reporter,
 			  const char *msg, void *priv_ctx);
 
+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 struct devlink *devlink_alloc(const struct devlink_ops *ops,
@@ -1190,13 +1194,7 @@ devlink_health_report(struct devlink_health_reporter *reporter,
 {
 	return 0;
 }
-#endif
 
-#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)
 {
diff --git a/net/Kconfig b/net/Kconfig
index 62da6148e9f8..1efe1f9ee492 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -429,21 +429,12 @@ config NET_SOCK_MSG
 	  with the help of BPF programs.
 
 config NET_DEVLINK
-	tristate "Network physical/parent device Netlink interface"
+	bool "Network physical/parent device Netlink interface"
 	help
 	  Network physical/parent device Netlink interface provides
 	  infrastructure to support access to physical chip-wide config and
 	  monitoring.
 
-config MAY_USE_DEVLINK
-	tristate
-	default m if NET_DEVLINK=m
-	default y if NET_DEVLINK=y || NET_DEVLINK=n
-	help
-	  Drivers using the devlink infrastructure should have a dependency
-	  on MAY_USE_DEVLINK to ensure they do not cause link errors when
-	  devlink is a loadable module and the driver using it is built-in.
-
 config PAGE_POOL
        bool
 
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 4f31ddc883e7..05e04ea0a5c7 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6482,20 +6482,9 @@ int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
 	return -EOPNOTSUPP;
 }
 
-static int __init devlink_module_init(void)
+static int __init devlink_init(void)
 {
 	return genl_register_family(&devlink_nl_family);
 }
 
-static void __exit devlink_module_exit(void)
-{
-	genl_unregister_family(&devlink_nl_family);
-}
-
-module_init(devlink_module_init);
-module_exit(devlink_module_exit);
-
-MODULE_LICENSE("GPL v2");
-MODULE_AUTHOR("Jiri Pirko <jiri@mellanox.com>");
-MODULE_DESCRIPTION("Network physical device Netlink interface");
-MODULE_ALIAS_GENL_FAMILY(DEVLINK_GENL_NAME);
+subsys_initcall(devlink_init);
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index 91e52973ee13..fab49132345f 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -6,7 +6,7 @@ config HAVE_NET_DSA
 
 config NET_DSA
 	tristate "Distributed Switch Architecture"
-	depends on HAVE_NET_DSA && MAY_USE_DEVLINK
+	depends on HAVE_NET_DSA
 	depends on BRIDGE || BRIDGE=n
 	select NET_SWITCHDEV
 	select PHYLINK
-- 
2.19.2

[PATCH net-next v2 2/5] ethtool: hold a reference to the netdevice around devlink compat

[Jakub Kicinski]

When calling into devlink compat code make sure we have a reference
on the netdevice on which the operation was invoked.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 net/core/ethtool.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 1320e8dce559..6832476dfcaf 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -805,11 +805,14 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
 	if (ops->get_eeprom_len)
 		info.eedump_len = ops->get_eeprom_len(dev);
 
-	rtnl_unlock();
-	if (!info.fw_version[0])
+	if (!info.fw_version[0]) {
+		dev_hold(dev);
+		rtnl_unlock();
 		devlink_compat_running_version(dev, info.fw_version,
 					       sizeof(info.fw_version));
-	rtnl_lock();
+		rtnl_lock();
+		dev_put(dev);
+	}
 
 	if (copy_to_user(useraddr, &info, sizeof(info)))
 		return -EFAULT;
@@ -2043,9 +2046,11 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev,
 	if (!dev->ethtool_ops->flash_device) {
 		int ret;
 
+		dev_hold(dev);
 		rtnl_unlock();
 		ret = devlink_compat_flash_update(dev, efl.data);
 		rtnl_lock();
+		dev_put(dev);
 
 		return ret;
 	}
-- 
2.19.2

[PATCH net-next v2 3/5] devlink: create a special NDO for getting the devlink instance

[Jakub Kicinski]

Instead of iterating over all devlink ports add a NDO which
will return the devlink instance from the driver.

v2: add the netdev_to_devlink() helper (Michal)

Suggested-by: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 include/linux/netdevice.h |  6 ++++
 include/net/devlink.h     |  9 ++++++
 net/core/devlink.c        | 58 ++++++++++-----------------------------
 3 files changed, 30 insertions(+), 43 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index aab4d9f6613d..eebcef6b8191 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -940,6 +940,8 @@ struct dev_ifalias {
 	char ifalias[];
 };
 
+struct devlink;
+
 /*
  * This structure defines the management hooks for network devices.
  * The following hooks can be defined; unless noted otherwise, they are
@@ -1248,6 +1250,9 @@ struct dev_ifalias {
  *	that got dropped are freed/returned via xdp_return_frame().
  *	Returns negative number, means general error invoking ndo, meaning
  *	no frames were xmit'ed and core-caller will free all frames.
+ * struct devlink *(*ndo_get_devlink)(struct net_device *dev);
+ *	Get devlink instance associated with a given netdev.
+ *	Called with a reference on the device only, rtnl_lock is not held.
  */
 struct net_device_ops {
 	int			(*ndo_init)(struct net_device *dev);
@@ -1446,6 +1451,7 @@ struct net_device_ops {
 						u32 flags);
 	int			(*ndo_xsk_async_xmit)(struct net_device *dev,
 						      u32 queue_id);
+	struct devlink *	(*ndo_get_devlink)(struct net_device *dev);
 };
 
 /**
diff --git a/include/net/devlink.h b/include/net/devlink.h
index f9f7fe974652..7f5a0bdca228 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -538,6 +538,15 @@ static inline struct devlink *priv_to_devlink(void *priv)
 	return container_of(priv, struct devlink, priv);
 }
 
+static inline struct devlink *netdev_to_devlink(struct net_device *dev)
+{
+#if IS_ENABLED(CONFIG_NET_DEVLINK)
+	if (dev->netdev_ops->ndo_get_devlink)
+		return dev->netdev_ops->ndo_get_devlink(dev);
+#endif
+	return NULL;
+}
+
 struct ib_device;
 
 #if IS_ENABLED(CONFIG_NET_DEVLINK)
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 05e04ea0a5c7..31e9494e7fc3 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6397,9 +6397,6 @@ static void __devlink_compat_running_version(struct devlink *devlink,
 	struct sk_buff *msg;
 	int rem, err;
 
-	if (!devlink->ops->info_get)
-		return;
-
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg)
 		return;
@@ -6431,55 +6428,30 @@ static void __devlink_compat_running_version(struct devlink *devlink,
 void devlink_compat_running_version(struct net_device *dev,
 				    char *buf, size_t len)
 {
-	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) {
-			if (devlink_port->type == DEVLINK_PORT_TYPE_ETH &&
-			    devlink_port->type_dev == dev) {
-				__devlink_compat_running_version(devlink,
-								 buf, len);
-				mutex_unlock(&devlink->lock);
-				goto out;
-			}
-		}
-		mutex_unlock(&devlink->lock);
-	}
-out:
-	mutex_unlock(&devlink_mutex);
+	devlink = netdev_to_devlink(dev);
+	if (!devlink || !devlink->ops->info_get)
+		return;
+
+	mutex_lock(&devlink->lock);
+	__devlink_compat_running_version(devlink, buf, len);
+	mutex_unlock(&devlink->lock);
 }
 
 int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
 {
-	struct devlink_port *devlink_port;
 	struct devlink *devlink;
+	int ret;
 
-	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);
+	devlink = netdev_to_devlink(dev);
+	if (!devlink || !devlink->ops->flash_update)
+		return -EOPNOTSUPP;
 
-	return -EOPNOTSUPP;
+	mutex_lock(&devlink->lock);
+	ret = devlink->ops->flash_update(devlink, file_name, NULL, NULL);
+	mutex_unlock(&devlink->lock);
+	return ret;
 }
 
 static int __init devlink_init(void)
-- 
2.19.2

[PATCH net-next v2 4/5] nfp: add .ndo_get_devlink

[Jakub Kicinski]

Support getting devlink instance from a new NDO.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/nfp_app.h        |  2 ++
 drivers/net/ethernet/netronome/nfp/nfp_devlink.c    | 11 +++++++++++
 drivers/net/ethernet/netronome/nfp/nfp_net_common.c |  1 +
 drivers/net/ethernet/netronome/nfp/nfp_net_repr.c   |  1 +
 4 files changed, 15 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_app.h b/drivers/net/ethernet/netronome/nfp/nfp_app.h
index d578d856a009..f8d422713705 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_app.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_app.h
@@ -433,4 +433,6 @@ int nfp_app_nic_vnic_alloc(struct nfp_app *app, struct nfp_net *nn,
 int nfp_app_nic_vnic_init_phy_port(struct nfp_pf *pf, struct nfp_app *app,
 				   struct nfp_net *nn, unsigned int id);
 
+struct devlink *nfp_devlink_get_devlink(struct net_device *netdev);
+
 #endif
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
index db2da99f6aa7..e9eca99cf493 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
@@ -376,3 +376,14 @@ void nfp_devlink_port_unregister(struct nfp_port *port)
 {
 	devlink_port_unregister(&port->dl_port);
 }
+
+struct devlink *nfp_devlink_get_devlink(struct net_device *netdev)
+{
+	struct nfp_app *app;
+
+	app = nfp_app_from_netdev(netdev);
+	if (!app)
+		return NULL;
+
+	return priv_to_devlink(app->pf);
+}
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 776f6c07701b..6d1b8816552e 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -3531,6 +3531,7 @@ const struct net_device_ops nfp_net_netdev_ops = {
 	.ndo_udp_tunnel_del	= nfp_net_del_vxlan_port,
 	.ndo_bpf		= nfp_net_xdp,
 	.ndo_get_port_parent_id	= nfp_port_get_port_parent_id,
+	.ndo_get_devlink	= nfp_devlink_get_devlink,
 };
 
 /**
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
index 62839807e21e..d2c803bb4e56 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
@@ -273,6 +273,7 @@ const struct net_device_ops nfp_repr_netdev_ops = {
 	.ndo_set_features	= nfp_port_set_features,
 	.ndo_set_mac_address    = eth_mac_addr,
 	.ndo_get_port_parent_id	= nfp_port_get_port_parent_id,
+	.ndo_get_devlink	= nfp_devlink_get_devlink,
 };
 
 void
-- 
2.19.2

[PATCH net-next v2 5/5] nfp: remove ethtool flashing fallback

[Jakub Kicinski]

Now that devlink fallback will be called reliably, we can remove
the ethtool flashing code.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 .../ethernet/netronome/nfp/nfp_net_ethtool.c  | 24 -------------------
 1 file changed, 24 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index 8f189149efc5..690b62718dbb 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
@@ -1234,28 +1234,6 @@ static int nfp_net_set_channels(struct net_device *netdev,
 	return nfp_net_set_num_rings(nn, total_rx, total_tx);
 }
 
-static int
-nfp_net_flash_device(struct net_device *netdev, struct ethtool_flash *flash)
-{
-	struct nfp_app *app;
-	int ret;
-
-	if (flash->region != ETHTOOL_FLASH_ALL_REGIONS)
-		return -EOPNOTSUPP;
-
-	app = nfp_app_from_netdev(netdev);
-	if (!app)
-		return -EOPNOTSUPP;
-
-	dev_hold(netdev);
-	rtnl_unlock();
-	ret = nfp_flash_update_common(app->pf, flash->data, NULL);
-	rtnl_lock();
-	dev_put(netdev);
-
-	return ret;
-}
-
 static const struct ethtool_ops nfp_net_ethtool_ops = {
 	.get_drvinfo		= nfp_net_get_drvinfo,
 	.get_link		= ethtool_op_get_link,
@@ -1266,7 +1244,6 @@ static const struct ethtool_ops nfp_net_ethtool_ops = {
 	.get_sset_count		= nfp_net_get_sset_count,
 	.get_rxnfc		= nfp_net_get_rxnfc,
 	.set_rxnfc		= nfp_net_set_rxnfc,
-	.flash_device		= nfp_net_flash_device,
 	.get_rxfh_indir_size	= nfp_net_get_rxfh_indir_size,
 	.get_rxfh_key_size	= nfp_net_get_rxfh_key_size,
 	.get_rxfh		= nfp_net_get_rxfh,
@@ -1292,7 +1269,6 @@ const struct ethtool_ops nfp_port_ethtool_ops = {
 	.get_strings		= nfp_port_get_strings,
 	.get_ethtool_stats	= nfp_port_get_stats,
 	.get_sset_count		= nfp_port_get_sset_count,
-	.flash_device		= nfp_net_flash_device,
 	.set_dump		= nfp_app_set_dump,
 	.get_dump_flag		= nfp_app_get_dump_flag,
 	.get_dump_data		= nfp_app_get_dump_data,
-- 
2.19.2

Re: [PATCH net-next v2 3/5] devlink: create a special NDO for getting the devlink instance

[Florian Fainelli]

On 2/22/19 11:54 AM, Jakub Kicinski wrote:
> Instead of iterating over all devlink ports add a NDO which
> will return the devlink instance from the driver.
> 
> v2: add the netdev_to_devlink() helper (Michal)
> 
> Suggested-by: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
[snip]

> -	mutex_unlock(&devlink_mutex);
> +	devlink = netdev_to_devlink(dev);
> +	if (!devlink || !devlink->ops->info_get)
> +		return;

AFAICT devlink_create() allows one to specify a NULL ops argument (not
that this makes sense), so probably need to add a check against
devlink->ops here as well, unless we want to change devlink_create() to
refuse a NULL ops pointer.

Other than that:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

> +
> +	mutex_lock(&devlink->lock);
> +	__devlink_compat_running_version(devlink, buf, len);
> +	mutex_unlock(&devlink->lock);
>  }
>  
>  int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
>  {
> -	struct devlink_port *devlink_port;
>  	struct devlink *devlink;
> +	int ret;
>  
> -	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);
> +	devlink = netdev_to_devlink(dev);
> +	if (!devlink || !devlink->ops->flash_update)
> +		return -EOPNOTSUPP;
>  

And same here.
-- 
Florian

Re: [PATCH net-next v2 1/5] net: devlink: turn devlink into a built-in

[Florian Fainelli]

On 2/22/19 11:54 AM, Jakub Kicinski wrote:
> Being able to build devlink as a module causes growing pains.
> First all drivers had to add a meta dependency to make sure
> they are not built in when devlink is built as a module.  Now
> we are struggling to invoke ethtool compat code reliably.
> 
> Make devlink code built-in, users can still not build it at
> all but the dynamically loadable module option is removed.

One use case I was potentially thinking about was that you have to load
devlink at some point to e.g.: recover your device from an initramfs,
and then you don't use devlink anymore so just you rmmod it and you have
switched to a different filesystem entirely (possibly on the network
that you just helped recover). That sounds like a corner case though so:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next v2 2/5] ethtool: hold a reference to the netdevice around devlink compat

[Florian Fainelli]

On 2/22/19 11:54 AM, Jakub Kicinski wrote:
> When calling into devlink compat code make sure we have a reference
> on the netdevice on which the operation was invoked.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
>  net/core/ethtool.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 1320e8dce559..6832476dfcaf 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -805,11 +805,14 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
>  	if (ops->get_eeprom_len)
>  		info.eedump_len = ops->get_eeprom_len(dev);
>  
> -	rtnl_unlock();
> -	if (!info.fw_version[0])
> +	if (!info.fw_version[0]) {
> +		dev_hold(dev);
> +		rtnl_unlock();
>  		devlink_compat_running_version(dev, info.fw_version,
>  					       sizeof(info.fw_version));
> -	rtnl_lock();
> +		rtnl_lock();
> +		dev_put(dev);
> +	}

Would it make sense to make the locking and reference holding implicit
within the compat versions of devlink_* because they use a net_device ->
devlink object manipulation as opposed to doing in the caller. We are
more or less guaranteed that the compatibility layer is used from within
ethtool.
-- 
Florian

Re: [PATCH net-next v2 3/5] devlink: create a special NDO for getting the devlink instance

[Jakub Kicinski]

On Fri, 22 Feb 2019 12:04:07 -0800, Florian Fainelli wrote:
> On 2/22/19 11:54 AM, Jakub Kicinski wrote:
> > Instead of iterating over all devlink ports add a NDO which
> > will return the devlink instance from the driver.
> > 
> > v2: add the netdev_to_devlink() helper (Michal)
> > 
> > Suggested-by: Jiri Pirko <jiri@resnulli.us>
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > ---  
> [snip]
> 
> > -	mutex_unlock(&devlink_mutex);
> > +	devlink = netdev_to_devlink(dev);
> > +	if (!devlink || !devlink->ops->info_get)
> > +		return;  
> 
> AFAICT devlink_create() allows one to specify a NULL ops argument (not
> that this makes sense), so probably need to add a check against
> devlink->ops here as well, unless we want to change devlink_create() to
> refuse a NULL ops pointer.

Oh, you're right!  devlink_nl_cmd_reload() seems to be missing that
check as well.  Let me respin quickly.

> Other than that:
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks!

Re: [PATCH net-next v2 2/5] ethtool: hold a reference to the netdevice around devlink compat

[Jakub Kicinski]

On Fri, 22 Feb 2019 12:09:32 -0800, Florian Fainelli wrote:
> On 2/22/19 11:54 AM, Jakub Kicinski wrote:
> > When calling into devlink compat code make sure we have a reference
> > on the netdevice on which the operation was invoked.
> > 
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > ---
> >  net/core/ethtool.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > index 1320e8dce559..6832476dfcaf 100644
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> > @@ -805,11 +805,14 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
> >  	if (ops->get_eeprom_len)
> >  		info.eedump_len = ops->get_eeprom_len(dev);
> >  
> > -	rtnl_unlock();
> > -	if (!info.fw_version[0])
> > +	if (!info.fw_version[0]) {
> > +		dev_hold(dev);
> > +		rtnl_unlock();
> >  		devlink_compat_running_version(dev, info.fw_version,
> >  					       sizeof(info.fw_version));
> > -	rtnl_lock();
> > +		rtnl_lock();
> > +		dev_put(dev);
> > +	}  
> 
> Would it make sense to make the locking and reference holding implicit
> within the compat versions of devlink_* because they use a net_device ->
> devlink object manipulation as opposed to doing in the caller. We are
> more or less guaranteed that the compatibility layer is used from within
> ethtool.

I guess we can, in general it feels wrong to call some devlink_
function with rtnl_lock held, but I guess devlink_compat_ can be
special in this regard.  v3 coming up.