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

[PATCH net-next 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.

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                         | 10 +--
 net/Kconfig                                   | 11 +--
 net/core/devlink.c                            | 77 ++++++-------------
 net/core/ethtool.c                            | 11 ++-
 net/dsa/Kconfig                               |  2 +-
 20 files changed, 57 insertions(+), 108 deletions(-)

-- 
2.19.2

[PATCH net-next 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 04d98550c78c..fcf09e02f4c6 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6480,20 +6480,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 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 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.

Suggested-by: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 include/linux/netdevice.h |  6 ++++
 net/core/devlink.c        | 62 +++++++++++++--------------------------
 2 files changed, 26 insertions(+), 42 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/net/core/devlink.c b/net/core/devlink.c
index fcf09e02f4c6..050999a68c35 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6395,9 +6395,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;
@@ -6429,55 +6426,36 @@ 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);
+	if (!dev->netdev_ops->ndo_get_devlink)
+		return;
+
+	devlink = dev->netdev_ops->ndo_get_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;
+	if (!dev->netdev_ops->ndo_get_devlink)
+		return -EOPNOTSUPP;
 
-			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 = dev->netdev_ops->ndo_get_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

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

[Michal Kubecek]

On Thu, Feb 21, 2019 at 09:46:18AM -0800, Jakub Kicinski wrote:
> Instead of iterating over all devlink ports add a NDO which
> will return the devlink instance from the driver.
> 
> Suggested-by: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
>  include/linux/netdevice.h |  6 ++++
>  net/core/devlink.c        | 62 +++++++++++++--------------------------
>  2 files changed, 26 insertions(+), 42 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.

The formulation is a bit ambiguous. Do I understand correctly that what
it says is that device reference is sufficient and rtnl_lock is not
necessary (but there is no harm if caller holds rtnl_lock)?

Michal

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

[Jakub Kicinski]

On Fri, 22 Feb 2019 10:51:31 +0100, Michal Kubecek wrote:
> On Thu, Feb 21, 2019 at 09:46:18AM -0800, Jakub Kicinski wrote:
> > Instead of iterating over all devlink ports add a NDO which
> > will return the devlink instance from the driver.
> > 
> > Suggested-by: Jiri Pirko <jiri@resnulli.us>
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > ---
> >  include/linux/netdevice.h |  6 ++++
> >  net/core/devlink.c        | 62 +++++++++++++--------------------------
> >  2 files changed, 26 insertions(+), 42 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.  
> 
> The formulation is a bit ambiguous. Do I understand correctly that what
> it says is that device reference is sufficient and rtnl_lock is not
> necessary (but there is no harm if caller holds rtnl_lock)?

Yes, only reference is held, the expectation is that drivers free
netdevs before they unregister devlink.  The devlink vs rtnl ordering
gets a bit tricky, drivers should definitely not expect rtnl to be held
in this NDO.  If the caller comes from rtnl path (like ethtool) it will
likely have to release rtnl before taking devlink->lock.

[PATCH net-next 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

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

[Michal Kubecek]

On Thu, Feb 21, 2019 at 09:46:19AM -0800, Jakub Kicinski wrote:
> 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);
> +}

AFAICS this would return a pointer to zero initialized struct devlink
when built with CONFIG_DEVLINK=n. Then devlink_compat_running_version()
would execute

	if (!dev->netdev_ops->ndo_get_devlink)
		return;

	devlink = dev->netdev_ops->ndo_get_devlink(dev);
	if (!devlink || !devlink->ops->info_get)
		return;

with non-null devlink but null devlink->ops so that it dereferences null
pointer (and so does devlink_compat_flash_update()).

Maybe it would be safer not to call ndo_get_devlink directly and have
an inline wrapper like

#if IS_ENABLED(CONFIG_NET_DEVLINK)
static inline struct devlink *dev_get_devlink(struct net_device *dev)
{
	if (dev->netdev_ops->ndo_get_devlink)
		return dev->netdev_ops->ndo_get_devlink();
	else
		retrurn NULL;
}
#else
static inline struct devlink *dev_get_devlink(struct net_device *dev)
{
	return NULL;
}
#endif

so that one can simply call the wrapper and check return value for NULL.

Michal

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

[Jakub Kicinski]

On Fri, 22 Feb 2019 11:04:50 +0100, Michal Kubecek wrote:
> On Thu, Feb 21, 2019 at 09:46:19AM -0800, Jakub Kicinski wrote:
> > 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);
> > +}  
> 
> AFAICS this would return a pointer to zero initialized struct devlink
> when built with CONFIG_DEVLINK=n. Then devlink_compat_running_version()
> would execute
> 
> 	if (!dev->netdev_ops->ndo_get_devlink)
> 		return;
> 
> 	devlink = dev->netdev_ops->ndo_get_devlink(dev);
> 	if (!devlink || !devlink->ops->info_get)
> 		return;
> 
> with non-null devlink but null devlink->ops so that it dereferences null
> pointer (and so does devlink_compat_flash_update()).

devlink_compat_flash_update() is this if CONFIG_DEVLINK=n:

static inline int
devlink_compat_flash_update(struct net_device *dev, const char *file_name)
{
	return -EOPNOTSUPP;
}

No?

> Maybe it would be safer not to call ndo_get_devlink directly and have
> an inline wrapper like
> 
> #if IS_ENABLED(CONFIG_NET_DEVLINK)
> static inline struct devlink *dev_get_devlink(struct net_device *dev)
> {
> 	if (dev->netdev_ops->ndo_get_devlink)
> 		return dev->netdev_ops->ndo_get_devlink();
> 	else
> 		retrurn NULL;
> }
> #else
> static inline struct devlink *dev_get_devlink(struct net_device *dev)
> {
> 	return NULL;
> }
> #endif
> 
> so that one can simply call the wrapper and check return value for NULL.

Only devlink code can call this ndo, and it doesn't exist with
DEVLINK=n.  I don't dislike wrappers for NDOs, but I'll defer to Jiri
to decide if we want a wrapper here (without the #if/#else, just the
first part for code clarity) :)

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

[Michal Kubecek]

On Fri, Feb 22, 2019 at 09:02:04AM -0800, Jakub Kicinski wrote:
> On Fri, 22 Feb 2019 11:04:50 +0100, Michal Kubecek wrote:
> > On Thu, Feb 21, 2019 at 09:46:19AM -0800, Jakub Kicinski wrote:
> > > 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);
> > > +}  
> > 
> > AFAICS this would return a pointer to zero initialized struct devlink
> > when built with CONFIG_DEVLINK=n. Then devlink_compat_running_version()
> > would execute
> > 
> > 	if (!dev->netdev_ops->ndo_get_devlink)
> > 		return;
> > 
> > 	devlink = dev->netdev_ops->ndo_get_devlink(dev);
> > 	if (!devlink || !devlink->ops->info_get)
> > 		return;
> > 
> > with non-null devlink but null devlink->ops so that it dereferences null
> > pointer (and so does devlink_compat_flash_update()).
> 
> devlink_compat_flash_update() is this if CONFIG_DEVLINK=n:
> 
> static inline int
> devlink_compat_flash_update(struct net_device *dev, const char *file_name)
> {
> 	return -EOPNOTSUPP;
> }
> 
> No?

You are right, I missed that devlink_compat_running_version() and
devlink_compat_flash_update() also have alternative versions when
devlink is disabled. So there is no problem.

> > Maybe it would be safer not to call ndo_get_devlink directly and have
> > an inline wrapper like
> > 
> > #if IS_ENABLED(CONFIG_NET_DEVLINK)
> > static inline struct devlink *dev_get_devlink(struct net_device *dev)
> > {
> > 	if (dev->netdev_ops->ndo_get_devlink)
> > 		return dev->netdev_ops->ndo_get_devlink();
> > 	else
> > 		retrurn NULL;
> > }
> > #else
> > static inline struct devlink *dev_get_devlink(struct net_device *dev)
> > {
> > 	return NULL;
> > }
> > #endif
> > 
> > so that one can simply call the wrapper and check return value for NULL.
> 
> Only devlink code can call this ndo, and it doesn't exist with
> DEVLINK=n.  I don't dislike wrappers for NDOs, but I'll defer to Jiri
> to decide if we want a wrapper here (without the #if/#else, just the
> first part for code clarity) :)

If the NDO is only supposed to be called from devlink code (or, more
precisely, code built only with CONFIG_DEVLINK=y), it should be IMHO
mentioned in its description. Another option would be enforcing it by
adding #ifdef around the ndo_get_devlink entry in struct net_device_ops
but that would require using ifdefs also in each driver providing the
NDO which seems inconvenient.

Michal

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

[Jakub Kicinski]

On Fri, 22 Feb 2019 18:27:15 +0100, Michal Kubecek wrote:
> > > Maybe it would be safer not to call ndo_get_devlink directly and have
> > > an inline wrapper like
> > > 
> > > #if IS_ENABLED(CONFIG_NET_DEVLINK)
> > > static inline struct devlink *dev_get_devlink(struct net_device *dev)
> > > {
> > > 	if (dev->netdev_ops->ndo_get_devlink)
> > > 		return dev->netdev_ops->ndo_get_devlink();
> > > 	else
> > > 		retrurn NULL;
> > > }
> > > #else
> > > static inline struct devlink *dev_get_devlink(struct net_device *dev)
> > > {
> > > 	return NULL;
> > > }
> > > #endif
> > > 
> > > so that one can simply call the wrapper and check return value for NULL.  
> > 
> > Only devlink code can call this ndo, and it doesn't exist with
> > DEVLINK=n.  I don't dislike wrappers for NDOs, but I'll defer to Jiri
> > to decide if we want a wrapper here (without the #if/#else, just the
> > first part for code clarity) :)  
> 
> If the NDO is only supposed to be called from devlink code (or, more
> precisely, code built only with CONFIG_DEVLINK=y), it should be IMHO
> mentioned in its description. Another option would be enforcing it by
> adding #ifdef around the ndo_get_devlink entry in struct net_device_ops
> but that would require using ifdefs also in each driver providing the
> NDO which seems inconvenient.

Yes, let's just go with your first proposal of a static inline helper.
I think it's the cleanest solution.

[PATCH net-next 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