[PATCH net-next v4 00/10] net: break dependency of drivers on geneve and vxlan

[PATCH net-next v4 00/10] net: break dependency of drivers on geneve and vxlan

[Hannes Frederic Sowa]

Device drivers which support geneve or vxlan offloading have a dependency
on the correlating tunnel kernel modules. Thus those drivers automatically
load the geneve or vxlan modules. Break this dependency with this
small series.

Additionally this series features a review of the respective ->ndo_open
and other functions around vxlan_get_rx_port and geneve_get_rx_port.

* Result:
$ cd drivers/net/ethernet/
$ find . -name '*.ko' | xargs modinfo | egrep  '^depends:.*(vxlan|geneve)'  | wc -l
0

I also incorporated feedback from Jesse Gross to only use one new
netdevice notifiers type, namely NETDEV_REFRESH_OFFLOADS. Otherwise this
series is very much the same as v1.

This series (v4) incorperates further feedback from Jesse Gross:
* before calling down to the ndo_add_{vxlan,geneve}_port, check if the
  driver has actually installed the function
* provide only one callback function, namely netdev_refresh_offloads
* provide updates to the comments in netdevice.h how driver should handle
  those functions

Hannes Frederic Sowa (10):
  qlcnic: protect qlcnic_82xx_io_slot_reset with rtnl lock
  mlx4: add rtnl lock protection in mlx4_en_restart
  ixgbe: add rtnl locking in service task around vxlan_get_rx_port
  benet: add rtnl lock protection around be_open in be_resume
  fm10k: add rtnl lock protection in fm10k_io_resume
  netdev: add netdevice notifier type to trigger a reprogramming of
    offloads
  vxlan: break dependency to network drivers
  geneve: break dependency to network drivers
  net: harmonize vxlan_get_rx_port and geneve_get_rx_port to
    netdev_refresh_offloads
  netdev: update comments and explain idempotency and rtnl locking

 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c   |  4 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt.c          |  2 +-
 drivers/net/ethernet/emulex/benet/be_main.c        | 11 +++---
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c    |  2 +-
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c       |  2 +
 drivers/net/ethernet/intel/i40e/i40e_main.c        |  7 +---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c      |  6 ++-
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c     |  4 +-
 .../net/ethernet/netronome/nfp/nfp_net_common.c    |  2 +-
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c   | 12 ++++--
 drivers/net/geneve.c                               | 37 ++++++++++++++----
 drivers/net/vxlan.c                                | 20 +++++++---
 include/linux/netdevice.h                          | 45 ++++++++++++++--------
 include/net/geneve.h                               | 10 +----
 include/net/vxlan.h                                |  8 ----
 15 files changed, 105 insertions(+), 67 deletions(-)

-- 
2.5.0

[PATCH net-next v4 01/10] qlcnic: protect qlcnic_82xx_io_slot_reset with rtnl lock

[Hannes Frederic Sowa]

Cc: Dept-GELinuxNICDev@qlogic.com
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
index 1205f6f9c94173..1c29105b6c364f 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
@@ -3952,8 +3952,14 @@ static pci_ers_result_t qlcnic_82xx_io_error_detected(struct pci_dev *pdev,
 
 static pci_ers_result_t qlcnic_82xx_io_slot_reset(struct pci_dev *pdev)
 {
-	return qlcnic_attach_func(pdev) ? PCI_ERS_RESULT_DISCONNECT :
-				PCI_ERS_RESULT_RECOVERED;
+	pci_ers_result_t res;
+
+	rtnl_lock();
+	res = qlcnic_attach_func(pdev) ? PCI_ERS_RESULT_DISCONNECT :
+					 PCI_ERS_RESULT_RECOVERED;
+	rtnl_unlock();
+
+	return res;
 }
 
 static void qlcnic_82xx_io_resume(struct pci_dev *pdev)
-- 
2.5.0

[PATCH net-next v4 02/10] mlx4: add rtnl lock protection in mlx4_en_restart

[Hannes Frederic Sowa]

I don't really understand the use of the state_lock. Can't it be simply
replaced by rtnl_lock? It seems a lot of current users depend on
rtnl_lock anyway.

Anyway, fix this up for the moment.

Cc: Eugenia Emantayev <eugenia@mellanox.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 0c7e3f69a73bb6..94abd0843901cf 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -1846,6 +1846,7 @@ static void mlx4_en_restart(struct work_struct *work)
 
 	en_dbg(DRV, priv, "Watchdog task called for port %d\n", priv->port);
 
+	rtnl_lock();
 	mutex_lock(&mdev->state_lock);
 	if (priv->port_up) {
 		mlx4_en_stop_port(dev, 1);
@@ -1853,6 +1854,7 @@ static void mlx4_en_restart(struct work_struct *work)
 			en_err(priv, "Failed restarting port %d\n", priv->port);
 	}
 	mutex_unlock(&mdev->state_lock);
+	rtnl_unlock();
 }
 
 static void mlx4_en_clear_stats(struct net_device *dev)
-- 
2.5.0

[PATCH net-next v4 03/10] ixgbe: add rtnl locking in service task around vxlan_get_rx_port

[Hannes Frederic Sowa]

Protect IXGBE_FLAG2_VXLAN_REREG_NEEDED flag with rtnl_lock as it seems
it could be concurrently modified by ixgbe_set_features and
ixgbe_del_vxlan_port.

Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index c4003a88bbf6ea..85ec389bd46560 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7118,10 +7118,12 @@ static void ixgbe_service_task(struct work_struct *work)
 		return;
 	}
 #ifdef CONFIG_IXGBE_VXLAN
+	rtnl_lock();
 	if (adapter->flags2 & IXGBE_FLAG2_VXLAN_REREG_NEEDED) {
 		adapter->flags2 &= ~IXGBE_FLAG2_VXLAN_REREG_NEEDED;
 		vxlan_get_rx_port(adapter->netdev);
 	}
+	rtnl_unlock();
 #endif /* CONFIG_IXGBE_VXLAN */
 	ixgbe_reset_subtask(adapter);
 	ixgbe_phy_interrupt_subtask(adapter);
-- 
2.5.0

[PATCH net-next v4 04/10] benet: add rtnl lock protection around be_open in be_resume

[Hannes Frederic Sowa]

Cc: Sathya Perla <sathya.perla@avagotech.com>
Cc: Ajit Khaparde <ajit.khaparde@avagotech.com>
Cc: Padmanabh Ratnakar <padmanabh.ratnakar@avagotech.com>
Cc: Sriharsha Basavapatna <sriharsha.basavapatna@avagotech.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/ethernet/emulex/benet/be_main.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index f99de3657ce3b5..3200f48ddd5a68 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -4846,11 +4846,12 @@ static int be_resume(struct be_adapter *adapter)
 	if (status)
 		return status;
 
-	if (netif_running(netdev)) {
+	rtnl_lock();
+	if (netif_running(netdev))
 		status = be_open(netdev);
-		if (status)
-			return status;
-	}
+	rtnl_unlock();
+	if (status)
+		return status;
 
 	netif_device_attach(netdev);
 
-- 
2.5.0

[PATCH net-next v4 05/10] fm10k: add rtnl lock protection in fm10k_io_resume

[Hannes Frederic Sowa]

Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
index 4eb7a6fa6b0ddc..846d9b731c8c14 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
@@ -2350,8 +2350,10 @@ static void fm10k_io_resume(struct pci_dev *pdev)
 	/* reset clock */
 	fm10k_ts_reset(interface);
 
+	rtnl_lock();
 	if (netif_running(netdev))
 		err = fm10k_open(netdev);
+	rtnl_lock();
 
 	/* final check of hardware state before registering the interface */
 	err = err ? : fm10k_hw_ready(interface);
-- 
2.5.0

[PATCH net-next v4 06/10] netdev: add netdevice notifier type to trigger a reprogramming of offloads

[Hannes Frederic Sowa]

Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 include/linux/netdevice.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 8d8e5ca951b493..9750e46760695d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2183,6 +2183,7 @@ struct netdev_lag_lower_state_info {
 #define NETDEV_BONDING_INFO	0x0019
 #define NETDEV_PRECHANGEUPPER	0x001A
 #define NETDEV_CHANGELOWERSTATE	0x001B
+#define NETDEV_REFRESH_OFFLOADS	0x001C
 
 int register_netdevice_notifier(struct notifier_block *nb);
 int unregister_netdevice_notifier(struct notifier_block *nb);
-- 
2.5.0

[PATCH net-next v4 07/10] vxlan: break dependency to network drivers

[Hannes Frederic Sowa]

Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/vxlan.c | 20 ++++++++++++++------
 include/net/vxlan.h |  5 +----
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index fecf7b6c732e96..ee45c796cb03cd 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -2468,7 +2468,7 @@ static struct device_type vxlan_type = {
  * supply the listening VXLAN udp ports. Callers are expected
  * to implement the ndo_add_vxlan_port.
  */
-void vxlan_get_rx_port(struct net_device *dev)
+static void vxlan_notify_refresh_netdev(struct net_device *dev)
 {
 	struct vxlan_sock *vs;
 	struct net *net = dev_net(dev);
@@ -2477,6 +2477,9 @@ void vxlan_get_rx_port(struct net_device *dev)
 	__be16 port;
 	unsigned int i;
 
+	if (!dev->netdev_ops->ndo_add_vxlan_port)
+		return;
+
 	spin_lock(&vn->sock_lock);
 	for (i = 0; i < PORT_HASH_SIZE; ++i) {
 		hlist_for_each_entry_rcu(vs, &vn->sock_list[i], hlist) {
@@ -2488,7 +2491,6 @@ void vxlan_get_rx_port(struct net_device *dev)
 	}
 	spin_unlock(&vn->sock_lock);
 }
-EXPORT_SYMBOL_GPL(vxlan_get_rx_port);
 
 /* Initialize the device structure. */
 static void vxlan_setup(struct net_device *dev)
@@ -3164,20 +3166,26 @@ static void vxlan_handle_lowerdev_unregister(struct vxlan_net *vn,
 	unregister_netdevice_many(&list_kill);
 }
 
-static int vxlan_lowerdev_event(struct notifier_block *unused,
-				unsigned long event, void *ptr)
+static int vxlan_notifier(struct notifier_block *unused,
+			  unsigned long event, void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	struct vxlan_net *vn = net_generic(dev_net(dev), vxlan_net_id);
 
-	if (event == NETDEV_UNREGISTER)
+	switch (event) {
+	case NETDEV_REFRESH_OFFLOADS:
+		vxlan_notify_refresh_netdev(dev);
+		break;
+	case NETDEV_UNREGISTER:
 		vxlan_handle_lowerdev_unregister(vn, dev);
+		break;
+	}
 
 	return NOTIFY_DONE;
 }
 
 static struct notifier_block vxlan_notifier_block __read_mostly = {
-	.notifier_call = vxlan_lowerdev_event,
+	.notifier_call = vxlan_notifier,
 };
 
 static __net_init int vxlan_init_net(struct net *net)
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 0fb86442544b26..48d0450160c91f 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -242,13 +242,10 @@ static inline netdev_features_t vxlan_features_check(struct sk_buff *skb,
 /* IPv6 header + UDP + VXLAN + Ethernet header */
 #define VXLAN6_HEADROOM (40 + 8 + 8 + 14)
 
-#if IS_ENABLED(CONFIG_VXLAN)
-void vxlan_get_rx_port(struct net_device *netdev);
-#else
 static inline void vxlan_get_rx_port(struct net_device *netdev)
 {
+	call_netdevice_notifiers(NETDEV_REFRESH_OFFLOADS, netdev);
 }
-#endif
 
 static inline unsigned short vxlan_get_sk_family(struct vxlan_sock *vs)
 {
-- 
2.5.0

[PATCH net-next v4 08/10] geneve: break dependency to network drivers

[Hannes Frederic Sowa]

Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/geneve.c | 37 ++++++++++++++++++++++++++++++-------
 include/net/geneve.h |  7 +++----
 2 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 24b077a32c1c9c..ebc1bba33f189c 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1106,11 +1106,7 @@ static struct device_type geneve_type = {
 	.name = "geneve",
 };
 
-/* Calls the ndo_add_geneve_port of the caller in order to
- * supply the listening GENEVE udp ports. Callers are expected
- * to implement the ndo_add_geneve_port.
- */
-void geneve_get_rx_port(struct net_device *dev)
+static void geneve_notify_refresh_netdev(struct net_device *dev)
 {
 	struct net *net = dev_net(dev);
 	struct geneve_net *gn = net_generic(net, geneve_net_id);
@@ -1119,6 +1115,9 @@ void geneve_get_rx_port(struct net_device *dev)
 	struct sock *sk;
 	__be16 port;
 
+	if (!dev->netdev_ops->ndo_add_geneve_port)
+		return;
+
 	rcu_read_lock();
 	list_for_each_entry_rcu(gs, &gn->sock_list, list) {
 		sk = gs->sock->sk;
@@ -1128,7 +1127,6 @@ void geneve_get_rx_port(struct net_device *dev)
 	}
 	rcu_read_unlock();
 }
-EXPORT_SYMBOL_GPL(geneve_get_rx_port);
 
 /* Initialize the device structure. */
 static void geneve_setup(struct net_device *dev)
@@ -1450,6 +1448,24 @@ struct net_device *geneve_dev_create_fb(struct net *net, const char *name,
 }
 EXPORT_SYMBOL_GPL(geneve_dev_create_fb);
 
+static int geneve_notifier(struct notifier_block *unused,
+			   unsigned long event, void *ptr)
+{
+	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+
+	switch (event) {
+	case NETDEV_REFRESH_OFFLOADS:
+		geneve_notify_refresh_netdev(dev);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block geneve_notifier_block __read_mostly = {
+	.notifier_call = geneve_notifier,
+};
+
 static __net_init int geneve_init_net(struct net *net)
 {
 	struct geneve_net *gn = net_generic(net, geneve_net_id);
@@ -1502,11 +1518,17 @@ static int __init geneve_init_module(void)
 	if (rc)
 		goto out1;
 
-	rc = rtnl_link_register(&geneve_link_ops);
+	rc = register_netdevice_notifier(&geneve_notifier_block);
 	if (rc)
 		goto out2;
 
+	rc = rtnl_link_register(&geneve_link_ops);
+	if (rc)
+		goto out3;
+
 	return 0;
+out3:
+	unregister_netdevice_notifier(&geneve_notifier_block);
 out2:
 	unregister_pernet_subsys(&geneve_net_ops);
 out1:
@@ -1517,6 +1539,7 @@ late_initcall(geneve_init_module);
 static void __exit geneve_cleanup_module(void)
 {
 	rtnl_link_unregister(&geneve_link_ops);
+	unregister_netdevice_notifier(&geneve_notifier_block);
 	unregister_pernet_subsys(&geneve_net_ops);
 }
 module_exit(geneve_cleanup_module);
diff --git a/include/net/geneve.h b/include/net/geneve.h
index e6c23dc765f7ec..7d52077b72faa3 100644
--- a/include/net/geneve.h
+++ b/include/net/geneve.h
@@ -1,6 +1,8 @@
 #ifndef __NET_GENEVE_H
 #define __NET_GENEVE_H  1
 
+#include <linux/netdevice.h>
+
 #ifdef CONFIG_INET
 #include <net/udp_tunnel.h>
 #endif
@@ -62,13 +64,10 @@ struct genevehdr {
 	struct geneve_opt options[];
 };
 
-#if IS_ENABLED(CONFIG_GENEVE)
-void geneve_get_rx_port(struct net_device *netdev);
-#else
 static inline void geneve_get_rx_port(struct net_device *netdev)
 {
+	call_netdevice_notifiers(NETDEV_REFRESH_OFFLOADS, netdev);
 }
-#endif
 
 #ifdef CONFIG_INET
 struct net_device *geneve_dev_create_fb(struct net *net, const char *name,
-- 
2.5.0

[PATCH net-next v4 09/10] net: harmonize vxlan_get_rx_port and geneve_get_rx_port to netdev_refresh_offloads

[Hannes Frederic Sowa]

Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c    | 4 ++--
 drivers/net/ethernet/broadcom/bnxt/bnxt.c           | 2 +-
 drivers/net/ethernet/emulex/benet/be_main.c         | 2 +-
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c     | 2 +-
 drivers/net/ethernet/intel/i40e/i40e_main.c         | 7 ++-----
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c       | 4 ++--
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c      | 2 +-
 drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 2 +-
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c    | 2 +-
 include/linux/netdevice.h                           | 4 ++++
 include/net/geneve.h                                | 5 -----
 include/net/vxlan.h                                 | 5 -----
 12 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 6c4e3a69976fca..6204154f1f2fe2 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -10286,7 +10286,7 @@ sp_rtnl_not_reset:
 			netdev_info(bp->dev,
 				    "Deleted vxlan dest port %d", port);
 			bp->vxlan_dst_port = 0;
-			vxlan_get_rx_port(bp->dev);
+			netdev_refresh_offloads(bp->dev);
 		}
 	}
 #endif
@@ -12492,7 +12492,7 @@ static int bnx2x_open(struct net_device *dev)
 
 #ifdef CONFIG_BNX2X_VXLAN
 	if (IS_PF(bp))
-		vxlan_get_rx_port(dev);
+		netdev_refresh_offloads(dev);
 #endif
 
 	return 0;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index f9569493034b4d..1641f94aa315c1 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -4637,7 +4637,7 @@ static int __bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
 
 	if (irq_re_init) {
 #if defined(CONFIG_VXLAN) || defined(CONFIG_VXLAN_MODULE)
-		vxlan_get_rx_port(bp->dev);
+		netdev_refresh_offloads(bp->dev);
 #endif
 		if (!bnxt_hwrm_tunnel_dst_port_alloc(
 				bp, htons(0x17c1),
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 3200f48ddd5a68..16edce294bbb2b 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -3601,7 +3601,7 @@ static int be_open(struct net_device *netdev)
 	netif_tx_start_all_queues(netdev);
 #ifdef CONFIG_BE2NET_VXLAN
 	if (skyhawk_chip(adapter))
-		vxlan_get_rx_port(netdev);
+		netdev_refresh_offloads(netdev);
 #endif
 
 	return 0;
diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
index 662569d5b7c01a..a2f0775ddd5460 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
@@ -558,7 +558,7 @@ int fm10k_open(struct net_device *netdev)
 
 #ifdef CONFIG_FM10K_VXLAN
 	/* update VXLAN port configuration */
-	vxlan_get_rx_port(netdev);
+	netdev_refresh_offloads(netdev);
 #endif
 
 	fm10k_up(interface);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index bb4612c159fd19..0a78cf046df608 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -5344,11 +5344,8 @@ int i40e_open(struct net_device *netdev)
 						       TCP_FLAG_CWR) >> 16);
 	wr32(&pf->hw, I40E_GLLAN_TSOMSK_L, be32_to_cpu(TCP_FLAG_CWR) >> 16);
 
-#ifdef CONFIG_I40E_VXLAN
-	vxlan_get_rx_port(netdev);
-#endif
-#ifdef CONFIG_I40E_GENEVE
-	geneve_get_rx_port(netdev);
+#if defined(CONFIG_I40E_VXLAN) || defined(CONFIG_I40E_GENEVE)
+	netdev_refresh_offloads(netdev);
 #endif
 
 	return 0;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 85ec389bd46560..b288b18fa0c6c0 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -6041,7 +6041,7 @@ static int ixgbe_open(struct net_device *netdev)
 
 	ixgbe_clear_vxlan_port(adapter);
 #ifdef CONFIG_IXGBE_VXLAN
-	vxlan_get_rx_port(netdev);
+	netdev_refresh_offloads(netdev);
 #endif
 
 	return 0;
@@ -7121,7 +7121,7 @@ static void ixgbe_service_task(struct work_struct *work)
 	rtnl_lock();
 	if (adapter->flags2 & IXGBE_FLAG2_VXLAN_REREG_NEEDED) {
 		adapter->flags2 &= ~IXGBE_FLAG2_VXLAN_REREG_NEEDED;
-		vxlan_get_rx_port(adapter->netdev);
+		netdev_refresh_offloads(adapter->netdev);
 	}
 	rtnl_unlock();
 #endif /* CONFIG_IXGBE_VXLAN */
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 94abd0843901cf..4b077d892cf193 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -1683,7 +1683,7 @@ int mlx4_en_start_port(struct net_device *dev)
 
 #ifdef CONFIG_MLX4_EN_VXLAN
 	if (priv->mdev->dev->caps.tunnel_offload_mode == MLX4_TUNNEL_OFFLOAD_MODE_VXLAN)
-		vxlan_get_rx_port(dev);
+		netdev_refresh_offloads(dev);
 #endif
 	priv->port_up = true;
 	netif_tx_start_all_queues(dev);
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 43c618bafdb641..70338327b47550 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -1795,7 +1795,7 @@ static int nfp_net_netdev_open(struct net_device *netdev)
 	if (nn->ctrl & NFP_NET_CFG_CTRL_VXLAN) {
 		memset(&nn->vxlan_ports, 0, sizeof(nn->vxlan_ports));
 		memset(&nn->vxlan_usecnt, 0, sizeof(nn->vxlan_usecnt));
-		vxlan_get_rx_port(netdev);
+		netdev_refresh_offloads(netdev);
 	}
 
 	/* Step 3: Enable for kernel
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
index 1c29105b6c364f..22ba8b7c8db1b1 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
@@ -2017,7 +2017,7 @@ qlcnic_attach(struct qlcnic_adapter *adapter)
 
 #ifdef CONFIG_QLCNIC_VXLAN
 	if (qlcnic_encap_rx_offload(adapter))
-		vxlan_get_rx_port(netdev);
+		netdev_refresh_offloads(netdev);
 #endif
 
 	adapter->is_up = QLCNIC_ADAPTER_UP_MAGIC;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 9750e46760695d..4d93d474f1cd44 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2224,6 +2224,10 @@ netdev_notifier_info_to_dev(const struct netdev_notifier_info *info)
 
 int call_netdevice_notifiers(unsigned long val, struct net_device *dev);
 
+static inline void netdev_refresh_offloads(struct net_device *netdev)
+{
+	call_netdevice_notifiers(NETDEV_REFRESH_OFFLOADS, netdev);
+}
 
 extern rwlock_t				dev_base_lock;		/* Device list lock */
 
diff --git a/include/net/geneve.h b/include/net/geneve.h
index 7d52077b72faa3..9e27fc7027052d 100644
--- a/include/net/geneve.h
+++ b/include/net/geneve.h
@@ -64,11 +64,6 @@ struct genevehdr {
 	struct geneve_opt options[];
 };
 
-static inline void geneve_get_rx_port(struct net_device *netdev)
-{
-	call_netdevice_notifiers(NETDEV_REFRESH_OFFLOADS, netdev);
-}
-
 #ifdef CONFIG_INET
 struct net_device *geneve_dev_create_fb(struct net *net, const char *name,
 					u8 name_assign_type, u16 dst_port);
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 48d0450160c91f..eb0a7e3368ca1c 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -242,11 +242,6 @@ static inline netdev_features_t vxlan_features_check(struct sk_buff *skb,
 /* IPv6 header + UDP + VXLAN + Ethernet header */
 #define VXLAN6_HEADROOM (40 + 8 + 8 + 14)
 
-static inline void vxlan_get_rx_port(struct net_device *netdev)
-{
-	call_netdevice_notifiers(NETDEV_REFRESH_OFFLOADS, netdev);
-}
-
 static inline unsigned short vxlan_get_sk_family(struct vxlan_sock *vs)
 {
 	return vs->sock->sk->sk_family;
-- 
2.5.0

[PATCH net-next v4 10/10] netdev: update comments and explain idempotency and rtnl locking

[Hannes Frederic Sowa]

Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 include/linux/netdevice.h | 40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 4d93d474f1cd44..0dc3e4617db6ef 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1006,31 +1006,36 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
  *	not implement this, it is assumed that the hw is not able to have
  *	multiple net devices on single physical port.
  *
+ *	*** Offloading callbacks: ***
+ *
+ *	Notify drivers about a new type of offloading being requested
+ *	or removed from the driver. The type is based on the function
+ *	name, sa_family distinguishes between ipv4 and ipv6 and the
+ *	corresponding port number (for now only being UDP tunnel
+ *	protocol numbers) is signaled down to the driver.
+ *
+ *	Calls into the offloading callback functions are always done
+ *	with rtnl_lock held.
+ *
+ *	Further more, the callbacks can happen multiple times for the
+ *	same primitive to be installed, so it is forbidden to use
+ *	reference counting on the {offload_type, sa_family, port}
+ *	tuple because the driver might see multiple calls to those
+ *	functions for one installed primitive.
+ *
+ *	If the driver wants to get the current ports reprogrammed it
+ *	can simply call netdev_refresh_offloads with rtnl_lock held.
+ *
  * void (*ndo_add_vxlan_port)(struct  net_device *dev,
  *			      sa_family_t sa_family, __be16 port);
- *	Called by vxlan to notiy a driver about the UDP port and socket
- *	address family that vxlan is listnening to. It is called only when
- *	a new port starts listening. The operation is protected by the
- *	vxlan_net->sock_lock.
- *
  * void (*ndo_add_geneve_port)(struct net_device *dev,
  *			      sa_family_t sa_family, __be16 port);
- *	Called by geneve to notify a driver about the UDP port and socket
- *	address family that geneve is listnening to. It is called only when
- *	a new port starts listening. The operation is protected by the
- *	geneve_net->sock_lock.
- *
  * void (*ndo_del_geneve_port)(struct net_device *dev,
  *			      sa_family_t sa_family, __be16 port);
- *	Called by geneve to notify the driver about a UDP port and socket
- *	address family that geneve is not listening to anymore. The operation
- *	is protected by the geneve_net->sock_lock.
- *
  * void (*ndo_del_vxlan_port)(struct  net_device *dev,
  *			      sa_family_t sa_family, __be16 port);
- *	Called by vxlan to notify the driver about a UDP port and socket
- *	address family that vxlan is not listening to anymore. The operation
- *	is protected by the vxlan_net->sock_lock.
+ *
+ *
  *
  * void* (*ndo_dfwd_add_station)(struct net_device *pdev,
  *				 struct net_device *dev)
@@ -2224,6 +2229,7 @@ netdev_notifier_info_to_dev(const struct netdev_notifier_info *info)
 
 int call_netdevice_notifiers(unsigned long val, struct net_device *dev);
 
+/* Obviously the driver needs to hold rtnl_lock while calling this function. */
 static inline void netdev_refresh_offloads(struct net_device *netdev)
 {
 	call_netdevice_notifiers(NETDEV_REFRESH_OFFLOADS, netdev);
-- 
2.5.0

Re: [PATCH net-next v4 06/10] netdev: add netdevice notifier type to trigger a reprogramming of offloads

[Tom Herbert]

On Sat, Jan 9, 2016 at 7:07 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
>  include/linux/netdevice.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 8d8e5ca951b493..9750e46760695d 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2183,6 +2183,7 @@ struct netdev_lag_lower_state_info {
>  #define NETDEV_BONDING_INFO    0x0019
>  #define NETDEV_PRECHANGEUPPER  0x001A
>  #define NETDEV_CHANGELOWERSTATE        0x001B
> +#define NETDEV_REFRESH_OFFLOADS        0x001C
>
Per previous discussion we don't want to generalize this current
offload interface. Can we just NETDEV_UP as the notifier?

>  int register_netdevice_notifier(struct notifier_block *nb);
>  int unregister_netdevice_notifier(struct notifier_block *nb);
> --
> 2.5.0
>

Re: [PATCH net-next v4 06/10] netdev: add netdevice notifier type to trigger a reprogramming of offloads

[Hannes Frederic Sowa]

On 09.01.2016 18:25, Tom Herbert wrote:
> On Sat, Jan 9, 2016 at 7:07 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>> ---
>>   include/linux/netdevice.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 8d8e5ca951b493..9750e46760695d 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -2183,6 +2183,7 @@ struct netdev_lag_lower_state_info {
>>   #define NETDEV_BONDING_INFO    0x0019
>>   #define NETDEV_PRECHANGEUPPER  0x001A
>>   #define NETDEV_CHANGELOWERSTATE        0x001B
>> +#define NETDEV_REFRESH_OFFLOADS        0x001C
>>
> Per previous discussion we don't want to generalize this current
> offload interface. Can we just NETDEV_UP as the notifier?

The problem with only using NETDEV_UP/REGISTER is that some drivers need 
to reconfigure their offloads during operation while keeping the 
netdevice in UP state. One example is ixgbe. This was my first idea also.

I am fine with both approaches, my initial submission had two types to 
notify geneve and vxlan so their doesn't happen any "cross-over".

Bye,
Hannes

Re: [PATCH net-next v4 05/10] fm10k: add rtnl lock protection in fm10k_io_resume

[Florian Westphal]

Hannes Frederic Sowa <hannes@stressinduktion.org> wrote:
> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
> index 4eb7a6fa6b0ddc..846d9b731c8c14 100644
> --- a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
> +++ b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
> @@ -2350,8 +2350,10 @@ static void fm10k_io_resume(struct pci_dev *pdev)
>  	/* reset clock */
>  	fm10k_ts_reset(interface);
>  
> +	rtnl_lock();
>  	if (netif_running(netdev))
>  		err = fm10k_open(netdev);
> +	rtnl_lock();

Hmm.  Looks like copy&paste error :)

Re: [PATCH net-next v4 08/10] geneve: break dependency to network drivers

[Jesse Gross]

On Sat, Jan 9, 2016 at 7:07 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
>  drivers/net/geneve.c | 37 ++++++++++++++++++++++++++++++-------
>  include/net/geneve.h |  7 +++----
>  2 files changed, 33 insertions(+), 11 deletions(-)

Reviewed-by: Jesse Gross <jesse@kernel.org>

Thanks!

Re: [PATCH net-next v4 06/10] netdev: add netdevice notifier type to trigger a reprogramming of offloads

[Tom Herbert]

On Sat, Jan 9, 2016 at 9:30 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On 09.01.2016 18:25, Tom Herbert wrote:
>>
>> On Sat, Jan 9, 2016 at 7:07 AM, Hannes Frederic Sowa
>> <hannes@stressinduktion.org> wrote:
>>>
>>> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>>> ---
>>>   include/linux/netdevice.h | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>> index 8d8e5ca951b493..9750e46760695d 100644
>>> --- a/include/linux/netdevice.h
>>> +++ b/include/linux/netdevice.h
>>> @@ -2183,6 +2183,7 @@ struct netdev_lag_lower_state_info {
>>>   #define NETDEV_BONDING_INFO    0x0019
>>>   #define NETDEV_PRECHANGEUPPER  0x001A
>>>   #define NETDEV_CHANGELOWERSTATE        0x001B
>>> +#define NETDEV_REFRESH_OFFLOADS        0x001C
>>>
>> Per previous discussion we don't want to generalize this current
>> offload interface. Can we just NETDEV_UP as the notifier?
>
>
> The problem with only using NETDEV_UP/REGISTER is that some drivers need to
> reconfigure their offloads during operation while keeping the netdevice in
> UP state. One example is ixgbe. This was my first idea also.
>
It's configuration. Just get it once and save the port number(s) in
the driver. Configure the device only when
IXGBE_FLAG_VXLAN_OFFLOAD_CAPABLE is set.

> I am fine with both approaches, my initial submission had two types to
> notify geneve and vxlan so their doesn't happen any "cross-over".
>
> Bye,
> Hannes
>

Re: [PATCH net-next v4 05/10] fm10k: add rtnl lock protection in fm10k_io_resume

[Hannes Frederic Sowa]

On 09.01.2016 22:51, Florian Westphal wrote:
> Hannes Frederic Sowa <hannes@stressinduktion.org> wrote:
>> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
>> index 4eb7a6fa6b0ddc..846d9b731c8c14 100644
>> --- a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
>> +++ b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
>> @@ -2350,8 +2350,10 @@ static void fm10k_io_resume(struct pci_dev *pdev)
>>   	/* reset clock */
>>   	fm10k_ts_reset(interface);
>>
>> +	rtnl_lock();
>>   	if (netif_running(netdev))
>>   		err = fm10k_open(netdev);
>> +	rtnl_lock();
>
> Hmm.  Looks like copy&paste error :)

Oh, yeah. Thanks, Florian, fixed it up locally.

Re: [PATCH net-next v4 06/10] netdev: add netdevice notifier type to trigger a reprogramming of offloads

[Hannes Frederic Sowa]

On 09.01.2016 23:27, Tom Herbert wrote:
> On Sat, Jan 9, 2016 at 9:30 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> On 09.01.2016 18:25, Tom Herbert wrote:
>>>
>>> On Sat, Jan 9, 2016 at 7:07 AM, Hannes Frederic Sowa
>>> <hannes@stressinduktion.org> wrote:
>>>>
>>>> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>>>> ---
>>>>    include/linux/netdevice.h | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>> index 8d8e5ca951b493..9750e46760695d 100644
>>>> --- a/include/linux/netdevice.h
>>>> +++ b/include/linux/netdevice.h
>>>> @@ -2183,6 +2183,7 @@ struct netdev_lag_lower_state_info {
>>>>    #define NETDEV_BONDING_INFO    0x0019
>>>>    #define NETDEV_PRECHANGEUPPER  0x001A
>>>>    #define NETDEV_CHANGELOWERSTATE        0x001B
>>>> +#define NETDEV_REFRESH_OFFLOADS        0x001C
>>>>
>>> Per previous discussion we don't want to generalize this current
>>> offload interface. Can we just NETDEV_UP as the notifier?
>>
>>
>> The problem with only using NETDEV_UP/REGISTER is that some drivers need to
>> reconfigure their offloads during operation while keeping the netdevice in
>> UP state. One example is ixgbe. This was my first idea also.
>>
> It's configuration. Just get it once and save the port number(s) in
> the driver. Configure the device only when
> IXGBE_FLAG_VXLAN_OFFLOAD_CAPABLE is set.

I don't see much value that each driver has to hold a database of ports 
to offload, the kernel knows much better and can easily be queried via 
this mechanism.

This is not only about ixgbe, but also about benet, which f.e. needs the 
list of offloads available again during resume. Of course, drivers can 
do their own stateful handling of ports, but this seems to much more 
generate problems. I rather would like to see more logic moved into the 
core and less code in the drivers.

Bye,
Hannes

Re: [PATCH net-next v4 06/10] netdev: add netdevice notifier type to trigger a reprogramming of offloads

[Hannes Frederic Sowa]

On 10.01.2016 18:16, Tom Herbert wrote:
> On Sat, Jan 9, 2016 at 2:52 PM, Hannes Frederic Sowa <
> hannes@stressinduktion.org> wrote:
>> On 09.01.2016 23:27, Tom Herbert wrote:
>>>
>>> On Sat, Jan 9, 2016 at 9:30 AM, Hannes Frederic Sowa
>>> <hannes@stressinduktion.org> wrote:
>>>>
>>>> On 09.01.2016 18:25, Tom Herbert wrote:
>>>>>
>>>>>
>>>>> On Sat, Jan 9, 2016 at 7:07 AM, Hannes Frederic Sowa
>>>>> <hannes@stressinduktion.org> wrote:
>>>>>>
>>>>>>
>>>>>> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>>>>>> ---
>>>>>> include/linux/netdevice.h | 1 +
>>>>>> 1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>>>> index 8d8e5ca951b493..9750e46760695d 100644
>>>>>> --- a/include/linux/netdevice.h
>>>>>> +++ b/include/linux/netdevice.h
>>>>>> @@ -2183,6 +2183,7 @@ struct netdev_lag_lower_state_info {
>>>>>> #define NETDEV_BONDING_INFO 0x0019
>>>>>> #define NETDEV_PRECHANGEUPPER 0x001A
>>>>>> #define NETDEV_CHANGELOWERSTATE 0x001B
>>>>>> +#define NETDEV_REFRESH_OFFLOADS 0x001C
>>>>>>
>>>>> Per previous discussion we don't want to generalize this current
>>>>> offload interface. Can we just NETDEV_UP as the notifier?
>>>>
>>>>
>>>>
>>>> The problem with only using NETDEV_UP/REGISTER is that some drivers need
>>>> to
>>>> reconfigure their offloads during operation while keeping the netdevice
>>>> in
>>>> UP state. One example is ixgbe. This was my first idea also.
>>>>
>>> It's configuration. Just get it once and save the port number(s) in
>>> the driver. Configure the device only when
>>> IXGBE_FLAG_VXLAN_OFFLOAD_CAPABLE is set.
>>
>>
>> I don't see much value that each driver has to hold a database of ports to
>> offload, the kernel knows much better and can easily be queried via this
>> mechanism.
>>
>> This is not only about ixgbe, but also about benet, which f.e. needs the
>> list of offloads available again during resume. Of course, drivers can do
>> their own stateful handling of ports, but this seems to much more generate
>> problems. I rather would like to see more logic moved into the core and
> less
>> code in the drivers.
>>
> All the drivers that support VXLAN already save the ports in a stateful
> fashion. Some support a single port, some and array, some a list. The only
> question is whether they bother to save the information when the offload
> feature is disabled. ixgbe does not for instance, but it looks like fm10k
> does since it doesn't check any feature flags. Fixing those that don't save
> the information all the times should be straightforward.

Yes, would probably be straight forward to add another list plus extra 
reference counting and maybe a lock, but I don't see why we need to 
duplicate the policy in the drivers? I personally think ixgbe does ok in 
this regard.

Anyway, this series merely tries to decouple 10gigbit cards from vxlan 
and/or geneve. It will be no problem to deal with this later on.

Bye,
Hannes

RE: [PATCH net-next v4 02/10] mlx4: add rtnl lock protection in mlx4_en_restart

[Eugenia Emantayev]

Hi Hannes,

state_lock is used for internal driver resources synchronization.
Our code will need further examination where rtnl_lock should be used.
For now, thank you for suggestion.
ACK for below patch.

Jenny


-----Original Message-----
From: Hannes Frederic Sowa [mailto:hannes@stressinduktion.org] 
Sent: Saturday, January 09, 2016 17:07
To: netdev@vger.kernel.org
Cc: jesse@kernel.org; Eugenia Emantayev <eugenia@mellanox.com>
Subject: [PATCH net-next v4 02/10] mlx4: add rtnl lock protection in mlx4_en_restart

I don't really understand the use of the state_lock. Can't it be simply replaced by rtnl_lock? It seems a lot of current users depend on rtnl_lock anyway.

Anyway, fix this up for the moment.

Cc: Eugenia Emantayev <eugenia@mellanox.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 0c7e3f69a73bb6..94abd0843901cf 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -1846,6 +1846,7 @@ static void mlx4_en_restart(struct work_struct *work)
 
 	en_dbg(DRV, priv, "Watchdog task called for port %d\n", priv->port);
 
+	rtnl_lock();
 	mutex_lock(&mdev->state_lock);
 	if (priv->port_up) {
 		mlx4_en_stop_port(dev, 1);
@@ -1853,6 +1854,7 @@ static void mlx4_en_restart(struct work_struct *work)
 			en_err(priv, "Failed restarting port %d\n", priv->port);
 	}
 	mutex_unlock(&mdev->state_lock);
+	rtnl_unlock();
 }
 
 static void mlx4_en_clear_stats(struct net_device *dev)

Re: [PATCH net-next v4 06/10] netdev: add netdevice notifier type to trigger a reprogramming of offloads

[Tom Herbert]

On Mon, Jan 11, 2016 at 4:11 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On 10.01.2016 18:16, Tom Herbert wrote:
>>
>> On Sat, Jan 9, 2016 at 2:52 PM, Hannes Frederic Sowa <
>> hannes@stressinduktion.org> wrote:
>>>
>>> On 09.01.2016 23:27, Tom Herbert wrote:
>>>>
>>>>
>>>> On Sat, Jan 9, 2016 at 9:30 AM, Hannes Frederic Sowa
>>>> <hannes@stressinduktion.org> wrote:
>>>>>
>>>>>
>>>>> On 09.01.2016 18:25, Tom Herbert wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Sat, Jan 9, 2016 at 7:07 AM, Hannes Frederic Sowa
>>>>>> <hannes@stressinduktion.org> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>>>>>>> ---
>>>>>>> include/linux/netdevice.h | 1 +
>>>>>>> 1 file changed, 1 insertion(+)
>>>>>>>
>>>>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>>>>> index 8d8e5ca951b493..9750e46760695d 100644
>>>>>>> --- a/include/linux/netdevice.h
>>>>>>> +++ b/include/linux/netdevice.h
>>>>>>> @@ -2183,6 +2183,7 @@ struct netdev_lag_lower_state_info {
>>>>>>> #define NETDEV_BONDING_INFO 0x0019
>>>>>>> #define NETDEV_PRECHANGEUPPER 0x001A
>>>>>>> #define NETDEV_CHANGELOWERSTATE 0x001B
>>>>>>> +#define NETDEV_REFRESH_OFFLOADS 0x001C
>>>>>>>
>>>>>> Per previous discussion we don't want to generalize this current
>>>>>> offload interface. Can we just NETDEV_UP as the notifier?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> The problem with only using NETDEV_UP/REGISTER is that some drivers
>>>>> need
>>>>> to
>>>>> reconfigure their offloads during operation while keeping the netdevice
>>>>> in
>>>>> UP state. One example is ixgbe. This was my first idea also.
>>>>>
>>>> It's configuration. Just get it once and save the port number(s) in
>>>> the driver. Configure the device only when
>>>> IXGBE_FLAG_VXLAN_OFFLOAD_CAPABLE is set.
>>>
>>>
>>>
>>> I don't see much value that each driver has to hold a database of ports
>>> to
>>> offload, the kernel knows much better and can easily be queried via this
>>> mechanism.
>>>
>>> This is not only about ixgbe, but also about benet, which f.e. needs the
>>> list of offloads available again during resume. Of course, drivers can do
>>> their own stateful handling of ports, but this seems to much more
>>> generate
>>> problems. I rather would like to see more logic moved into the core and
>>
>> less
>>>
>>> code in the drivers.
>>>
>> All the drivers that support VXLAN already save the ports in a stateful
>> fashion. Some support a single port, some and array, some a list. The only
>> question is whether they bother to save the information when the offload
>> feature is disabled. ixgbe does not for instance, but it looks like fm10k
>> does since it doesn't check any feature flags. Fixing those that don't
>> save
>> the information all the times should be straightforward.
>
>
> Yes, would probably be straight forward to add another list plus extra
> reference counting and maybe a lock, but I don't see why we need to
> duplicate the policy in the drivers? I personally think ixgbe does ok in
> this regard.
>
Again, to reiterate, _all_ the drivers that support VXLAN already save
the ports in a stateful fashion. Adding a new notifier is unnecessary
and makes things more complicated, not less. Also to reiterate, this
is not generic functionality that the driver is providing; we should
not need to add anything for this into the core APIs outside of the
two ndo functions that already exist.

Tom

> Anyway, this series merely tries to decouple 10gigbit cards from vxlan
> and/or geneve. It will be no problem to deal with this later on.
>
> Bye,
> Hannes
>

Re: [PATCH net-next v4 06/10] netdev: add netdevice notifier type to trigger a reprogramming of offloads

[Hannes Frederic Sowa]

On 11.01.2016 17:09, Tom Herbert wrote:
> On Mon, Jan 11, 2016 at 4:11 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> On 10.01.2016 18:16, Tom Herbert wrote:
>>>
>>> On Sat, Jan 9, 2016 at 2:52 PM, Hannes Frederic Sowa <
>>> hannes@stressinduktion.org> wrote:
>>>>
>>>> On 09.01.2016 23:27, Tom Herbert wrote:
>>>>>
>>>>>
>>>>> On Sat, Jan 9, 2016 at 9:30 AM, Hannes Frederic Sowa
>>>>> <hannes@stressinduktion.org> wrote:
>>>>>>
>>>>>>
>>>>>> On 09.01.2016 18:25, Tom Herbert wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Sat, Jan 9, 2016 at 7:07 AM, Hannes Frederic Sowa
>>>>>>> <hannes@stressinduktion.org> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>>>>>>>> ---
>>>>>>>> include/linux/netdevice.h | 1 +
>>>>>>>> 1 file changed, 1 insertion(+)
>>>>>>>>
>>>>>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>>>>>> index 8d8e5ca951b493..9750e46760695d 100644
>>>>>>>> --- a/include/linux/netdevice.h
>>>>>>>> +++ b/include/linux/netdevice.h
>>>>>>>> @@ -2183,6 +2183,7 @@ struct netdev_lag_lower_state_info {
>>>>>>>> #define NETDEV_BONDING_INFO 0x0019
>>>>>>>> #define NETDEV_PRECHANGEUPPER 0x001A
>>>>>>>> #define NETDEV_CHANGELOWERSTATE 0x001B
>>>>>>>> +#define NETDEV_REFRESH_OFFLOADS 0x001C
>>>>>>>>
>>>>>>> Per previous discussion we don't want to generalize this current
>>>>>>> offload interface. Can we just NETDEV_UP as the notifier?
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> The problem with only using NETDEV_UP/REGISTER is that some drivers
>>>>>> need
>>>>>> to
>>>>>> reconfigure their offloads during operation while keeping the netdevice
>>>>>> in
>>>>>> UP state. One example is ixgbe. This was my first idea also.
>>>>>>
>>>>> It's configuration. Just get it once and save the port number(s) in
>>>>> the driver. Configure the device only when
>>>>> IXGBE_FLAG_VXLAN_OFFLOAD_CAPABLE is set.
>>>>
>>>>
>>>>
>>>> I don't see much value that each driver has to hold a database of ports
>>>> to
>>>> offload, the kernel knows much better and can easily be queried via this
>>>> mechanism.
>>>>
>>>> This is not only about ixgbe, but also about benet, which f.e. needs the
>>>> list of offloads available again during resume. Of course, drivers can do
>>>> their own stateful handling of ports, but this seems to much more
>>>> generate
>>>> problems. I rather would like to see more logic moved into the core and
>>>
>>> less
>>>>
>>>> code in the drivers.
>>>>
>>> All the drivers that support VXLAN already save the ports in a stateful
>>> fashion. Some support a single port, some and array, some a list. The only
>>> question is whether they bother to save the information when the offload
>>> feature is disabled. ixgbe does not for instance, but it looks like fm10k
>>> does since it doesn't check any feature flags. Fixing those that don't
>>> save
>>> the information all the times should be straightforward.
>>
>>
>> Yes, would probably be straight forward to add another list plus extra
>> reference counting and maybe a lock, but I don't see why we need to
>> duplicate the policy in the drivers? I personally think ixgbe does ok in
>> this regard.
>>
> Again, to reiterate, _all_ the drivers that support VXLAN already save
> the ports in a stateful fashion. Adding a new notifier is unnecessary
> and makes things more complicated, not less. Also to reiterate, this
> is not generic functionality that the driver is providing; we should
> not need to add anything for this into the core APIs outside of the
> two ndo functions that already exist.

I consider the offloading interface work in progress:

I agree this interface can be much improved. But this series focuses on 
firstly removing the dependency on the modules without changing too much 
otherwise. No new callback is added just two ones are migrated into one.

Currently drivers don't handle a list of all ports, they discard new 
ones as soon as they don't fit into the hardware anymore (mostly only 
one). So currently the state is not completely replicated in the 
drivers. We could call the ndo_add_*_ports again after we deleted some 
ports and try to refresh from the core kernel side. I think instead of 
adding this logic to all drivers to fix this, it is easier to callback 
into the kernel instead of adding this logic to almost all network drivers.

Lot's of drivers refresh the ports list by calling ndo_open function 
again because of error recovery or resuming and thus get a new port list 
(here some drivers actually currently increment the refcounter for 
already installed ports by mistake and I am looking into how to fix 
this, probably by throwing away the list of currently installed port 
numbers first and then refreshing all ports). It is even worse: it seems 
drivers actually increment the port refcounter by mistake when you 
change ring settings (or other settings causing a device reset) after 
installing the vxlan offload.

I agree it would be great to get away with the callback completely but I 
haven't yet fully figured out how error handling should be done then. It 
seems also not useful to put the error handling in the ndo_add_*_port 
functions as most simply configure the structs and call a worker 
afterwards to process them. So the correct propagation of errors can 
also be tricky.

I just don't want to mess with the state in the current drivers but as a 
first step remove the dependency.

The reason why I want to keep the callback is that without completely 
understanding the rest of problems I don't know if we need it or not.

Thoughts?

Thanks,
Hannes

Re: [PATCH net-next v4 06/10] netdev: add netdevice notifier type to trigger a reprogramming of offloads

[Tom Herbert]

On Mon, Jan 11, 2016 at 10:56 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On 11.01.2016 17:09, Tom Herbert wrote:
>>
>> On Mon, Jan 11, 2016 at 4:11 AM, Hannes Frederic Sowa
>> <hannes@stressinduktion.org> wrote:
>>>
>>> On 10.01.2016 18:16, Tom Herbert wrote:
>>>>
>>>>
>>>> On Sat, Jan 9, 2016 at 2:52 PM, Hannes Frederic Sowa <
>>>> hannes@stressinduktion.org> wrote:
>>>>>
>>>>>
>>>>> On 09.01.2016 23:27, Tom Herbert wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Sat, Jan 9, 2016 at 9:30 AM, Hannes Frederic Sowa
>>>>>> <hannes@stressinduktion.org> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 09.01.2016 18:25, Tom Herbert wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Sat, Jan 9, 2016 at 7:07 AM, Hannes Frederic Sowa
>>>>>>>> <hannes@stressinduktion.org> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>>>>>>>>> ---
>>>>>>>>> include/linux/netdevice.h | 1 +
>>>>>>>>> 1 file changed, 1 insertion(+)
>>>>>>>>>
>>>>>>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>>>>>>> index 8d8e5ca951b493..9750e46760695d 100644
>>>>>>>>> --- a/include/linux/netdevice.h
>>>>>>>>> +++ b/include/linux/netdevice.h
>>>>>>>>> @@ -2183,6 +2183,7 @@ struct netdev_lag_lower_state_info {
>>>>>>>>> #define NETDEV_BONDING_INFO 0x0019
>>>>>>>>> #define NETDEV_PRECHANGEUPPER 0x001A
>>>>>>>>> #define NETDEV_CHANGELOWERSTATE 0x001B
>>>>>>>>> +#define NETDEV_REFRESH_OFFLOADS 0x001C
>>>>>>>>>
>>>>>>>> Per previous discussion we don't want to generalize this current
>>>>>>>> offload interface. Can we just NETDEV_UP as the notifier?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The problem with only using NETDEV_UP/REGISTER is that some drivers
>>>>>>> need
>>>>>>> to
>>>>>>> reconfigure their offloads during operation while keeping the
>>>>>>> netdevice
>>>>>>> in
>>>>>>> UP state. One example is ixgbe. This was my first idea also.
>>>>>>>
>>>>>> It's configuration. Just get it once and save the port number(s) in
>>>>>> the driver. Configure the device only when
>>>>>> IXGBE_FLAG_VXLAN_OFFLOAD_CAPABLE is set.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I don't see much value that each driver has to hold a database of ports
>>>>> to
>>>>> offload, the kernel knows much better and can easily be queried via
>>>>> this
>>>>> mechanism.
>>>>>
>>>>> This is not only about ixgbe, but also about benet, which f.e. needs
>>>>> the
>>>>> list of offloads available again during resume. Of course, drivers can
>>>>> do
>>>>> their own stateful handling of ports, but this seems to much more
>>>>> generate
>>>>> problems. I rather would like to see more logic moved into the core and
>>>>
>>>>
>>>> less
>>>>>
>>>>>
>>>>> code in the drivers.
>>>>>
>>>> All the drivers that support VXLAN already save the ports in a stateful
>>>> fashion. Some support a single port, some and array, some a list. The
>>>> only
>>>> question is whether they bother to save the information when the offload
>>>> feature is disabled. ixgbe does not for instance, but it looks like
>>>> fm10k
>>>> does since it doesn't check any feature flags. Fixing those that don't
>>>> save
>>>> the information all the times should be straightforward.
>>>
>>>
>>>
>>> Yes, would probably be straight forward to add another list plus extra
>>> reference counting and maybe a lock, but I don't see why we need to
>>> duplicate the policy in the drivers? I personally think ixgbe does ok in
>>> this regard.
>>>
>> Again, to reiterate, _all_ the drivers that support VXLAN already save
>> the ports in a stateful fashion. Adding a new notifier is unnecessary
>> and makes things more complicated, not less. Also to reiterate, this
>> is not generic functionality that the driver is providing; we should
>> not need to add anything for this into the core APIs outside of the
>> two ndo functions that already exist.
>
>
> I consider the offloading interface work in progress:
>
I consider it a closed interface. New devices should implement
protocol generic offloads so that we don't need to extend the protocol
specific offload interfaces any further. We do need to break the
dependency between drivers and VXLAN but that can be done without any
additional APIs.

> I agree this interface can be much improved. But this series focuses on
> firstly removing the dependency on the modules without changing too much
> otherwise. No new callback is added just two ones are migrated into one.
>
> Currently drivers don't handle a list of all ports, they discard new ones as
> soon as they don't fit into the hardware anymore (mostly only one). So
> currently the state is not completely replicated in the drivers. We could
> call the ndo_add_*_ports again after we deleted some ports and try to
> refresh from the core kernel side. I think instead of adding this logic to
> all drivers to fix this, it is easier to callback into the kernel instead of
> adding this logic to almost all network drivers.
>
Almost all network drivers??? I count only nine that are setting
.ndo_add_vxlan_port.

> Lot's of drivers refresh the ports list by calling ndo_open function again
> because of error recovery or resuming and thus get a new port list (here
> some drivers actually currently increment the refcounter for already
> installed ports by mistake and I am looking into how to fix this, probably
> by throwing away the list of currently installed port numbers first and then
> refreshing all ports). It is even worse: it seems drivers actually increment
> the port refcounter by mistake when you change ring settings (or other
> settings causing a device reset) after installing the vxlan offload.
>
> I agree it would be great to get away with the callback completely but I
> haven't yet fully figured out how error handling should be done then. It
> seems also not useful to put the error handling in the ndo_add_*_port
> functions as most simply configure the structs and call a worker afterwards
> to process them. So the correct propagation of errors can also be tricky.
>
> I just don't want to mess with the state in the current drivers but as a
> first step remove the dependency.
>
> The reason why I want to keep the callback is that without completely
> understanding the rest of problems I don't know if we need it or not.
>
It is true that the drivers are all over the board on how they
implement this. They need to be fixed regardless (as you point ref
counting is a mess). A new notifier does not address this and just
maintains what is a bad model in the first place. Inflicting more
complexity into core APIs for this simply isn't justified, let's try
to fix the drivers in question.

> Thoughts?
>
> Thanks,
> Hannes
>

Re: [PATCH net-next v4 06/10] netdev: add netdevice notifier type to trigger a reprogramming of offloads

[Hannes Frederic Sowa]

Hi Tom,

On 11.01.2016 21:46, Tom Herbert wrote:
> On Mon, Jan 11, 2016 at 10:56 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> I consider the offloading interface work in progress:
>>
> I consider it a closed interface. New devices should implement
> protocol generic offloads so that we don't need to extend the protocol
> specific offload interfaces any further. We do need to break the
> dependency between drivers and VXLAN but that can be done without any
> additional APIs.

I am totally with you here.

The reason why I did this series is that I don't want to have vxlan and 
geneve modules autoloaded when I modprobe any of those drivers which 
implement ndo_add_*_port and use the current style of callbacks. So I 
think this is a step to reduce this entanglement already. I solely 
switch from vxlan_get_rx_port and geneve_get_rx_port to 
netdev_refresh_offloads.

>> I agree this interface can be much improved. But this series focuses on
>> firstly removing the dependency on the modules without changing too much
>> otherwise. No new callback is added just two ones are migrated into one.
>>
>> Currently drivers don't handle a list of all ports, they discard new ones as
>> soon as they don't fit into the hardware anymore (mostly only one). So
>> currently the state is not completely replicated in the drivers. We could
>> call the ndo_add_*_ports again after we deleted some ports and try to
>> refresh from the core kernel side. I think instead of adding this logic to
>> all drivers to fix this, it is easier to callback into the kernel instead of
>> adding this logic to almost all network drivers.
>>
> Almost all network drivers??? I count only nine that are setting
> .ndo_add_vxlan_port.

I was referring to the drivers which already implement ndo_add_{vxlan, 
geneve}_port, of course. Sorry for not being specific enough here. All 
the other drivers don't need to be considered at all.

>> Lot's of drivers refresh the ports list by calling ndo_open function again
>> because of error recovery or resuming and thus get a new port list (here
>> some drivers actually currently increment the refcounter for already
>> installed ports by mistake and I am looking into how to fix this, probably
>> by throwing away the list of currently installed port numbers first and then
>> refreshing all ports). It is even worse: it seems drivers actually increment
>> the port refcounter by mistake when you change ring settings (or other
>> settings causing a device reset) after installing the vxlan offload.
>>
>> I agree it would be great to get away with the callback completely but I
>> haven't yet fully figured out how error handling should be done then. It
>> seems also not useful to put the error handling in the ndo_add_*_port
>> functions as most simply configure the structs and call a worker afterwards
>> to process them. So the correct propagation of errors can also be tricky.
>>
>> I just don't want to mess with the state in the current drivers but as a
>> first step remove the dependency.
>>
>> The reason why I want to keep the callback is that without completely
>> understanding the rest of problems I don't know if we need it or not.
>>
> It is true that the drivers are all over the board on how they
> implement this. They need to be fixed regardless (as you point ref
> counting is a mess). A new notifier does not address this and just
> maintains what is a bad model in the first place. Inflicting more
> complexity into core APIs for this simply isn't justified, let's try
> to fix the drivers in question.

I think we have a core entanglement between the drivers that offload 
vxlan and geneve to the tunnel modules which is hurting most right now 
without dealing too much with the offloading API.

As net-next is closed now do you think the series which I actually send 
hopefully in time is still worth being considered to at least get the 
autoloading fixed. It just replaces the callback by a different callback 
which does not depend on the modules.

Quite some drivers need to be touched by that (which implement those 
offloads) and I am not sure if requesting a new port list from the 
kernel in case of error conditions and the reinitialization of the NIC 
during its UP operation without state is still the way to go and easier 
to implement. It feels that we have to add more code to the drivers 
otherwise. By looking at the current code it might make sense to just 
remove the reference counting and ask the kernel to just send down the 
list of new ports.

As soon as an interface for generic offloads is found it can be favored 
by the kernel stack and the vxlan specific code can be ifdefed out and 
later on removed as soon as all features are adequately replaced by the 
generic offloading interface.

Does that make sense?

Thanks,
Hannes