[PATCH net-next v3 0/6] devlink: make ethtool compat reliable

[PATCH net-next v3 0/6] 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.

Last but not least missing checks for devlink->ops are added.
There is currently no driver registering devlink without ops,
so can just fix this in -next.

v3 (Florian):
 - add missing checks for devlink->ops;
 - move locking/holding into devlink_compat_ functions.
v2 (Michal): add netdev_to_devlink() in patch 3.

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

 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                            | 87 +++++++------------
 net/core/ethtool.c                            | 13 +--
 net/dsa/Kconfig                               |  2 +-
 20 files changed, 69 insertions(+), 117 deletions(-)

-- 
2.19.2

[PATCH net-next v3 1/6] 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>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.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 v3 2/6] 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)
v3: check that devlink has ops (Florian)

Suggested-by: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.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..a13055160be0 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 || !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 || !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 v3 3/6] 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 v3 4/6] 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

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

[Jakub Kicinski]

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

v3: move the hold/lock logic into devlink_compat_* functions (Florian)

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 net/core/devlink.c | 34 +++++++++++++++++++++++-----------
 net/core/ethtool.c | 13 ++-----------
 2 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index a13055160be0..78c6ea1870ca 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6430,27 +6430,39 @@ void devlink_compat_running_version(struct net_device *dev,
 {
 	struct devlink *devlink;
 
+	dev_hold(dev);
+	rtnl_unlock();
+
 	devlink = netdev_to_devlink(dev);
-	if (!devlink || !devlink->ops || !devlink->ops->info_get)
-		return;
+	if (devlink && devlink->ops && devlink->ops->info_get) {
+		mutex_lock(&devlink->lock);
+		__devlink_compat_running_version(devlink, buf, len);
+		mutex_unlock(&devlink->lock);
+	}
 
-	mutex_lock(&devlink->lock);
-	__devlink_compat_running_version(devlink, buf, len);
-	mutex_unlock(&devlink->lock);
+	rtnl_lock();
+	dev_put(dev);
 }
 
 int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
 {
 	struct devlink *devlink;
-	int ret;
+	int ret = -EOPNOTSUPP;
+
+	dev_hold(dev);
+	rtnl_unlock();
 
 	devlink = netdev_to_devlink(dev);
-	if (!devlink || !devlink->ops || !devlink->ops->flash_update)
-		return -EOPNOTSUPP;
+	if (devlink && devlink->ops && devlink->ops->flash_update) {
+		mutex_lock(&devlink->lock);
+		ret = devlink->ops->flash_update(devlink, file_name,
+						 NULL, NULL);
+		mutex_unlock(&devlink->lock);
+	}
+
+	rtnl_lock();
+	dev_put(dev);
 
-	mutex_lock(&devlink->lock);
-	ret = devlink->ops->flash_update(devlink, file_name, NULL, NULL);
-	mutex_unlock(&devlink->lock);
 	return ret;
 }
 
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 1320e8dce559..47558ffae423 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -805,11 +805,9 @@ 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])
 		devlink_compat_running_version(dev, info.fw_version,
 					       sizeof(info.fw_version));
-	rtnl_lock();
 
 	if (copy_to_user(useraddr, &info, sizeof(info)))
 		return -EFAULT;
@@ -2040,15 +2038,8 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev,
 		return -EFAULT;
 	efl.data[ETHTOOL_FLASH_MAX_FILENAME - 1] = 0;
 
-	if (!dev->ethtool_ops->flash_device) {
-		int ret;
-
-		rtnl_unlock();
-		ret = devlink_compat_flash_update(dev, efl.data);
-		rtnl_lock();
-
-		return ret;
-	}
+	if (!dev->ethtool_ops->flash_device)
+		return devlink_compat_flash_update(dev, efl.data);
 
 	return dev->ethtool_ops->flash_device(dev, &efl);
 }
-- 
2.19.2

[PATCH net-next v3 6/6] devlink: add missing NULL checks for devlink ops

[Jakub Kicinski]

Commit 76726ccb7f46 ("devlink: add flash update command") and
commit 2d8dc5bbf4e7 ("devlink: Add support for reload")
access devlink ops without NULL-checking. Add the missing checks.
Note that all drivers currently implementing devlink pass non-NULL
ops, so this is not a problem.

Reported-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 net/core/devlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 78c6ea1870ca..38cb0239dede 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2651,7 +2651,7 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
 	struct devlink *devlink = info->user_ptr[0];
 	int err;
 
-	if (!devlink->ops->reload)
+	if (!devlink->ops || !devlink->ops->reload)
 		return -EOPNOTSUPP;
 
 	err = devlink_resources_validate(devlink, NULL, info);
@@ -2669,7 +2669,7 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
 	const char *file_name, *component;
 	struct nlattr *nla_component;
 
-	if (!devlink->ops->flash_update)
+	if (!devlink->ops || !devlink->ops->flash_update)
 		return -EOPNOTSUPP;
 
 	if (!info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME])
-- 
2.19.2

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

[Jiri Pirko]

Fri, Feb 22, 2019 at 11:07:52PM CET, jakub.kicinski@netronome.com 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.
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

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

Re: [PATCH net-next v3 2/6] devlink: create a special NDO for getting the devlink instance

[Jiri Pirko]

Fri, Feb 22, 2019 at 11:07:53PM CET, jakub.kicinski@netronome.com 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)
>v3: check that devlink has ops (Florian)
>
>Suggested-by: Jiri Pirko <jiri@resnulli.us>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

[...]


> 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);

What prevents devlink from being unregistered now from another thread?
I think you should hold &devlink_mutex here to prevent it.



>+	if (!devlink || !devlink->ops || !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);

Same here.


>+	if (!devlink || !devlink->ops || !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
>

Re: [PATCH net-next v3 3/6] nfp: add .ndo_get_devlink

[Jiri Pirko]

Fri, Feb 22, 2019 at 11:07:54PM CET, jakub.kicinski@netronome.com wrote:
>Support getting devlink instance from a new NDO.
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

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

Re: [PATCH net-next v3 4/6] nfp: remove ethtool flashing fallback

[Jiri Pirko]

Fri, Feb 22, 2019 at 11:07:55PM CET, jakub.kicinski@netronome.com wrote:
>Now that devlink fallback will be called reliably, we can remove
>the ethtool flashing code.
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

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

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

[Jiri Pirko]

Fri, Feb 22, 2019 at 11:07:56PM CET, jakub.kicinski@netronome.com wrote:
>When ethtool is calling into devlink compat code make sure we have
>a reference on the netdevice on which the operation was invoked.
>
>v3: move the hold/lock logic into devlink_compat_* functions (Florian)
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>---
> net/core/devlink.c | 34 +++++++++++++++++++++++-----------
> net/core/ethtool.c | 13 ++-----------
> 2 files changed, 25 insertions(+), 22 deletions(-)
>
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index a13055160be0..78c6ea1870ca 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -6430,27 +6430,39 @@ void devlink_compat_running_version(struct net_device *dev,
> {
> 	struct devlink *devlink;
> 
>+	dev_hold(dev);
>+	rtnl_unlock();

Ha, I got it now. You rely on dev_hold to make sure the
devlink instance does not dissappear. But until this patch, that is not
guaranteed (or I'm missing it).


>+
> 	devlink = netdev_to_devlink(dev);
>-	if (!devlink || !devlink->ops || !devlink->ops->info_get)
>-		return;
>+	if (devlink && devlink->ops && devlink->ops->info_get) {
>+		mutex_lock(&devlink->lock);
>+		__devlink_compat_running_version(devlink, buf, len);
>+		mutex_unlock(&devlink->lock);
>+	}
> 
>-	mutex_lock(&devlink->lock);
>-	__devlink_compat_running_version(devlink, buf, len);
>-	mutex_unlock(&devlink->lock);
>+	rtnl_lock();
>+	dev_put(dev);
> }
> 
> int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
> {
> 	struct devlink *devlink;
>-	int ret;
>+	int ret = -EOPNOTSUPP;
>+
>+	dev_hold(dev);
>+	rtnl_unlock();
> 
> 	devlink = netdev_to_devlink(dev);
>-	if (!devlink || !devlink->ops || !devlink->ops->flash_update)
>-		return -EOPNOTSUPP;
>+	if (devlink && devlink->ops && devlink->ops->flash_update) {
>+		mutex_lock(&devlink->lock);
>+		ret = devlink->ops->flash_update(devlink, file_name,
>+						 NULL, NULL);
>+		mutex_unlock(&devlink->lock);
>+	}
>+
>+	rtnl_lock();
>+	dev_put(dev);
> 
>-	mutex_lock(&devlink->lock);
>-	ret = devlink->ops->flash_update(devlink, file_name, NULL, NULL);
>-	mutex_unlock(&devlink->lock);
> 	return ret;
> }
> 
>diff --git a/net/core/ethtool.c b/net/core/ethtool.c
>index 1320e8dce559..47558ffae423 100644
>--- a/net/core/ethtool.c
>+++ b/net/core/ethtool.c
>@@ -805,11 +805,9 @@ 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])
> 		devlink_compat_running_version(dev, info.fw_version,
> 					       sizeof(info.fw_version));
>-	rtnl_lock();
> 
> 	if (copy_to_user(useraddr, &info, sizeof(info)))
> 		return -EFAULT;
>@@ -2040,15 +2038,8 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev,
> 		return -EFAULT;
> 	efl.data[ETHTOOL_FLASH_MAX_FILENAME - 1] = 0;
> 
>-	if (!dev->ethtool_ops->flash_device) {
>-		int ret;
>-
>-		rtnl_unlock();
>-		ret = devlink_compat_flash_update(dev, efl.data);
>-		rtnl_lock();
>-
>-		return ret;
>-	}
>+	if (!dev->ethtool_ops->flash_device)
>+		return devlink_compat_flash_update(dev, efl.data);
> 
> 	return dev->ethtool_ops->flash_device(dev, &efl);
> }
>-- 
>2.19.2
>

Re: [PATCH net-next v3 6/6] devlink: add missing NULL checks for devlink ops

[Jiri Pirko]

Fri, Feb 22, 2019 at 11:07:57PM CET, jakub.kicinski@netronome.com wrote:
>Commit 76726ccb7f46 ("devlink: add flash update command") and
>commit 2d8dc5bbf4e7 ("devlink: Add support for reload")
>access devlink ops without NULL-checking. Add the missing checks.
>Note that all drivers currently implementing devlink pass non-NULL
>ops, so this is not a problem.

Wouldn't it be better to rather put WARN_ON&fail when driver calls
devlink_alloc() with NULL ops and avoid these checks in the whole code?


>
>Reported-by: Florian Fainelli <f.fainelli@gmail.com>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>---
> net/core/devlink.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 78c6ea1870ca..38cb0239dede 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -2651,7 +2651,7 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
> 	struct devlink *devlink = info->user_ptr[0];
> 	int err;
> 
>-	if (!devlink->ops->reload)
>+	if (!devlink->ops || !devlink->ops->reload)
> 		return -EOPNOTSUPP;
> 
> 	err = devlink_resources_validate(devlink, NULL, info);
>@@ -2669,7 +2669,7 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
> 	const char *file_name, *component;
> 	struct nlattr *nla_component;
> 
>-	if (!devlink->ops->flash_update)
>+	if (!devlink->ops || !devlink->ops->flash_update)
> 		return -EOPNOTSUPP;
> 
> 	if (!info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME])
>-- 
>2.19.2
>

Re: [PATCH net-next v3 0/6] devlink: make ethtool compat reliable

[David Miller]

Jakub please address the feedback you were given.

Thanks.

Re: [PATCH net-next v3 3/6] nfp: add .ndo_get_devlink

[Florian Fainelli]

Le 2/22/19 à 2:07 PM, Jakub Kicinski a écrit :
> Support getting devlink instance from a new NDO.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

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

Re: [PATCH net-next v3 4/6] nfp: remove ethtool flashing fallback

[Florian Fainelli]

Le 2/22/19 à 2:07 PM, Jakub Kicinski a écrit :
> Now that devlink fallback will be called reliably, we can remove
> the ethtool flashing code.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

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

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

[Jakub Kicinski]

On Sun, 24 Feb 2019 12:00:03 +0100, Jiri Pirko wrote:
> Fri, Feb 22, 2019 at 11:07:56PM CET, jakub.kicinski@netronome.com wrote:
> >When ethtool is calling into devlink compat code make sure we have
> >a reference on the netdevice on which the operation was invoked.
> >
> >v3: move the hold/lock logic into devlink_compat_* functions (Florian)
> >
> >Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> >---
> > net/core/devlink.c | 34 +++++++++++++++++++++++-----------
> > net/core/ethtool.c | 13 ++-----------
> > 2 files changed, 25 insertions(+), 22 deletions(-)
> >
> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >index a13055160be0..78c6ea1870ca 100644
> >--- a/net/core/devlink.c
> >+++ b/net/core/devlink.c
> >@@ -6430,27 +6430,39 @@ void devlink_compat_running_version(struct net_device *dev,
> > {
> > 	struct devlink *devlink;
> > 
> >+	dev_hold(dev);
> >+	rtnl_unlock();  
> 
> Ha, I got it now. You rely on dev_hold to make sure the
> devlink instance does not dissappear. But until this patch, that is not
> guaranteed (or I'm missing it).

Yup, I think the expectation that drivers should free netdevs before
unregistering devlink holds today. But it may be a source of bugs :S

I can add take devlink_mutex, and check the devlink instance is
registered. Do you prefer an explicit ->registered field like port has,
or can I do this:

diff --git a/net/core/devlink.c b/net/core/devlink.c
index cefcc0f45d44..be39ad6a4e2e 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -5275,6 +5275,7 @@ struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size)
                return NULL;
        devlink->ops = ops;
        devlink_net_set(devlink, &init_net);
+       INIT_LIST_HEAD(&devlink->list);
        INIT_LIST_HEAD(&devlink->port_list);
        INIT_LIST_HEAD(&devlink->sb_list);
        INIT_LIST_HEAD_RCU(&devlink->dpipe_table_list);
@@ -5303,6 +5304,11 @@ int devlink_register(struct devlink *devlink, struct device *dev)
 }
 EXPORT_SYMBOL_GPL(devlink_register);
 
+static bool devlink_is_registered(struct devlink *devlink)
+{
+       return list_empty(&devlink->list);
+}
+
 /**
  *     devlink_unregister - Unregister devlink instance
  *
@@ -5312,7 +5318,7 @@ void devlink_unregister(struct devlink *devlink)
 {
        mutex_lock(&devlink_mutex);
        devlink_notify(devlink, DEVLINK_CMD_DEL);
-       list_del(&devlink->list);
+       list_del_init(&devlink->list);
        mutex_unlock(&devlink_mutex);
 }
 EXPORT_SYMBOL_GPL(devlink_unregister);

?

Re: [PATCH net-next v3 6/6] devlink: add missing NULL checks for devlink ops

[Jakub Kicinski]

On Sun, 24 Feb 2019 12:03:19 +0100, Jiri Pirko wrote:
> Fri, Feb 22, 2019 at 11:07:57PM CET, jakub.kicinski@netronome.com wrote:
>> Commit 76726ccb7f46 ("devlink: add flash update command") and
>> commit 2d8dc5bbf4e7 ("devlink: Add support for reload")
>> access devlink ops without NULL-checking. Add the missing checks.
>> Note that all drivers currently implementing devlink pass non-NULL
>> ops, so this is not a problem.  
> 
> Wouldn't it be better to rather put WARN_ON&fail when driver calls
> devlink_alloc() with NULL ops and avoid these checks in the whole code?

Sounds good! Should I remove the existing ones?

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

[Jakub Kicinski]

On Mon, 25 Feb 2019 10:31:27 -0800, Jakub Kicinski wrote:
> +static bool devlink_is_registered(struct devlink *devlink)
> +{
> +       return list_empty(&devlink->list);
> +}

Nevermind, this won't really help the drivers that much, those
registering devlink after netdevs will have to take care of the
potential race by marking the devlink instance as dead in their priv
data structure from the thread doing the devlink_unregister, using
their own locking.

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

[Jiri Pirko]

Mon, Feb 25, 2019 at 07:31:27PM CET, jakub.kicinski@netronome.com wrote:
>On Sun, 24 Feb 2019 12:00:03 +0100, Jiri Pirko wrote:
>> Fri, Feb 22, 2019 at 11:07:56PM CET, jakub.kicinski@netronome.com wrote:
>> >When ethtool is calling into devlink compat code make sure we have
>> >a reference on the netdevice on which the operation was invoked.
>> >
>> >v3: move the hold/lock logic into devlink_compat_* functions (Florian)
>> >
>> >Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>> >---
>> > net/core/devlink.c | 34 +++++++++++++++++++++++-----------
>> > net/core/ethtool.c | 13 ++-----------
>> > 2 files changed, 25 insertions(+), 22 deletions(-)
>> >
>> >diff --git a/net/core/devlink.c b/net/core/devlink.c
>> >index a13055160be0..78c6ea1870ca 100644
>> >--- a/net/core/devlink.c
>> >+++ b/net/core/devlink.c
>> >@@ -6430,27 +6430,39 @@ void devlink_compat_running_version(struct net_device *dev,
>> > {
>> > 	struct devlink *devlink;
>> > 
>> >+	dev_hold(dev);
>> >+	rtnl_unlock();  
>> 
>> Ha, I got it now. You rely on dev_hold to make sure the
>> devlink instance does not dissappear. But until this patch, that is not
>> guaranteed (or I'm missing it).
>
>Yup, I think the expectation that drivers should free netdevs before
>unregistering devlink holds today. But it may be a source of bugs :S

Sure.

>
>I can add take devlink_mutex, and check the devlink instance is
>registered. Do you prefer an explicit ->registered field like port has,
>or can I do this:

Yeah, let's add WARN_ON.


>
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index cefcc0f45d44..be39ad6a4e2e 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -5275,6 +5275,7 @@ struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size)
>                return NULL;
>        devlink->ops = ops;
>        devlink_net_set(devlink, &init_net);
>+       INIT_LIST_HEAD(&devlink->list);
>        INIT_LIST_HEAD(&devlink->port_list);
>        INIT_LIST_HEAD(&devlink->sb_list);
>        INIT_LIST_HEAD_RCU(&devlink->dpipe_table_list);
>@@ -5303,6 +5304,11 @@ int devlink_register(struct devlink *devlink, struct device *dev)
> }
> EXPORT_SYMBOL_GPL(devlink_register);
> 
>+static bool devlink_is_registered(struct devlink *devlink)
>+{
>+       return list_empty(&devlink->list);
>+}
>+
> /**
>  *     devlink_unregister - Unregister devlink instance
>  *
>@@ -5312,7 +5318,7 @@ void devlink_unregister(struct devlink *devlink)
> {
>        mutex_lock(&devlink_mutex);
>        devlink_notify(devlink, DEVLINK_CMD_DEL);
>-       list_del(&devlink->list);
>+       list_del_init(&devlink->list);
>        mutex_unlock(&devlink_mutex);
> }
> EXPORT_SYMBOL_GPL(devlink_unregister);
>
>?

Re: [PATCH net-next v3 6/6] devlink: add missing NULL checks for devlink ops

[Jiri Pirko]

Mon, Feb 25, 2019 at 07:32:49PM CET, jakub.kicinski@netronome.com wrote:
>On Sun, 24 Feb 2019 12:03:19 +0100, Jiri Pirko wrote:
>> Fri, Feb 22, 2019 at 11:07:57PM CET, jakub.kicinski@netronome.com wrote:
>>> Commit 76726ccb7f46 ("devlink: add flash update command") and
>>> commit 2d8dc5bbf4e7 ("devlink: Add support for reload")
>>> access devlink ops without NULL-checking. Add the missing checks.
>>> Note that all drivers currently implementing devlink pass non-NULL
>>> ops, so this is not a problem.  
>> 
>> Wouldn't it be better to rather put WARN_ON&fail when driver calls
>> devlink_alloc() with NULL ops and avoid these checks in the whole code?
>
>Sounds good! Should I remove the existing ones?

Yes please.