[PATCHv2 net-next 0/5] net: common feature compute for upper interface

[PATCHv2 net-next 0/5] net: common feature compute for upper interface

[Hangbin Liu]

Some high-level virtual drivers need to compute features from their
lower devices, but each currently has its own implementation and may
miss some feature computations. This patch set introduces a common function
to compute features for such devices.

Currently, bonding, team, and bridge have been updated to use the new
helper.

v2:
a) remove hard_header_len setting. I will set needed_headroom for bond/team
   in a separate patch as bridge has it's own ways. (Ido Schimmel)
b) Add test file to Makefile, set RET=0 to a proper location. (Ido Schimmel)

Hangbin Liu (5):
  net: add a common function to compute features from lowers devices
  bonding: use common function to compute the features
  team: use common function to compute the features
  net: bridge: use common function to compute the features
  selftests/net: add offload checking test for virtual interface

 drivers/net/bonding/bond_main.c             |  99 +----------
 drivers/net/team/team_core.c                |  73 +-------
 include/linux/netdevice.h                   |  19 +++
 net/bridge/br_if.c                          |  22 +--
 net/core/dev.c                              |  76 +++++++++
 tools/testing/selftests/net/Makefile        |   1 +
 tools/testing/selftests/net/config          |   2 +
 tools/testing/selftests/net/vdev_offload.sh | 176 ++++++++++++++++++++
 8 files changed, 285 insertions(+), 183 deletions(-)
 create mode 100755 tools/testing/selftests/net/vdev_offload.sh

-- 
2.50.1

[PATCHv2 net-next 1/5] net: add a common function to compute features from lowers devices

[Hangbin Liu]

Some high level virtual drivers need to compute features from lower
devices. But each has their own implementations and may lost some
feature compute. Let's use one common function to compute features
for kinds of these devices.

The new helper uses the current bond implementation as the reference
one, as the latter already handles all the relevant aspects: netdev
features, TSO limits and dst retention.

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/linux/netdevice.h | 19 ++++++++++
 net/core/dev.c            | 76 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f3a3b761abfb..42742a47f2c6 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -5279,6 +5279,25 @@ int __netdev_update_features(struct net_device *dev);
 void netdev_update_features(struct net_device *dev);
 void netdev_change_features(struct net_device *dev);
 
+/* netdevice features */
+#define VIRTUAL_DEV_VLAN_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \
+					 NETIF_F_FRAGLIST | NETIF_F_GSO_SOFTWARE | \
+					 NETIF_F_GSO_ENCAP_ALL | \
+					 NETIF_F_HIGHDMA | NETIF_F_LRO)
+
+#define VIRTUAL_DEV_ENC_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \
+					 NETIF_F_RXCSUM | NETIF_F_GSO_SOFTWARE | \
+					 NETIF_F_GSO_PARTIAL)
+
+#define VIRTUAL_DEV_MPLS_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \
+					 NETIF_F_GSO_SOFTWARE)
+
+#define VIRTUAL_DEV_XFRM_FEATURES	(NETIF_F_HW_ESP | NETIF_F_HW_ESP_TX_CSUM | \
+					 NETIF_F_GSO_ESP)
+
+#define VIRTUAL_DEV_GSO_PARTIAL_FEATURES (NETIF_F_GSO_ESP)
+void netdev_compute_features_from_lowers(struct net_device *dev);
+
 void netif_stacked_transfer_operstate(const struct net_device *rootdev,
 					struct net_device *dev);
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 1d1650d9ecff..5c1c8b016c8e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -12577,6 +12577,82 @@ netdev_features_t netdev_increment_features(netdev_features_t all,
 }
 EXPORT_SYMBOL(netdev_increment_features);
 
+/**
+ *	netdev_compute_features_from_lowers - compute feature from lowers
+ *	@dev: the upper device
+ *
+ *	Recompute the upper device's feature based on all lower devices.
+ */
+void netdev_compute_features_from_lowers(struct net_device *dev)
+{
+	unsigned int dst_release_flag = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
+	netdev_features_t gso_partial_features = VIRTUAL_DEV_GSO_PARTIAL_FEATURES;
+#ifdef CONFIG_XFRM_OFFLOAD
+	netdev_features_t xfrm_features  = VIRTUAL_DEV_XFRM_FEATURES;
+#endif
+	netdev_features_t mpls_features  = VIRTUAL_DEV_MPLS_FEATURES;
+	netdev_features_t vlan_features = VIRTUAL_DEV_VLAN_FEATURES;
+	netdev_features_t enc_features  = VIRTUAL_DEV_ENC_FEATURES;
+	unsigned int tso_max_size = TSO_MAX_SIZE;
+	u16 tso_max_segs = TSO_MAX_SEGS;
+	struct net_device *lower_dev;
+	struct list_head *iter;
+
+	mpls_features = netdev_base_features(mpls_features);
+	vlan_features = netdev_base_features(vlan_features);
+	enc_features = netdev_base_features(enc_features);
+
+	netdev_for_each_lower_dev(dev, lower_dev, iter) {
+		gso_partial_features = netdev_increment_features(gso_partial_features,
+								 lower_dev->gso_partial_features,
+								 VIRTUAL_DEV_GSO_PARTIAL_FEATURES);
+
+		vlan_features = netdev_increment_features(vlan_features,
+							  lower_dev->vlan_features,
+							  VIRTUAL_DEV_VLAN_FEATURES);
+
+#ifdef CONFIG_XFRM_OFFLOAD
+		xfrm_features = netdev_increment_features(xfrm_features,
+							  lower_dev->hw_enc_features,
+							  VIRTUAL_DEV_XFRM_FEATURES);
+#endif
+
+		enc_features = netdev_increment_features(enc_features,
+							 lower_dev->hw_enc_features,
+							 VIRTUAL_DEV_ENC_FEATURES);
+
+		mpls_features = netdev_increment_features(mpls_features,
+							  lower_dev->mpls_features,
+							  VIRTUAL_DEV_MPLS_FEATURES);
+
+		dst_release_flag &= lower_dev->priv_flags;
+
+		tso_max_size = min(tso_max_size, lower_dev->tso_max_size);
+		tso_max_segs = min(tso_max_segs, lower_dev->tso_max_segs);
+	}
+
+	dev->gso_partial_features = gso_partial_features;
+	dev->vlan_features = vlan_features;
+#ifdef CONFIG_XFRM_OFFLOAD
+	dev->hw_enc_features |= xfrm_features;
+#endif
+	dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
+				    NETIF_F_HW_VLAN_CTAG_TX |
+				    NETIF_F_HW_VLAN_STAG_TX;
+	dev->mpls_features = mpls_features;
+
+	dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
+	if ((dev->priv_flags & IFF_XMIT_DST_RELEASE_PERM) &&
+	    dst_release_flag == (IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM))
+		dev->priv_flags |= IFF_XMIT_DST_RELEASE;
+
+	netif_set_tso_max_segs(dev, tso_max_segs);
+	netif_set_tso_max_size(dev, tso_max_size);
+
+	netdev_change_features(dev);
+}
+EXPORT_SYMBOL(netdev_compute_features_from_lowers);
+
 static struct hlist_head * __net_init netdev_create_hash(void)
 {
 	int i;
-- 
2.50.1

[PATCHv2 net-next 2/5] bonding: use common function to compute the features

[Hangbin Liu]

Use the new functon netdev_compute_features_from_lowers() to compute the
bonding features.

Note that bond_compute_features() currently uses bond_for_each_slave()
to traverse the lower devices list, and that is just a macro wrapper of
netdev_for_each_lower_private(). We use similar helper
netdev_for_each_lower_dev() in netdev_compute_features_from_lowers() to
iterate the slave device, as there is not need to get the private data.

No functional change intended.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 99 ++-------------------------------
 1 file changed, 4 insertions(+), 95 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 257333c88710..ec098a52ef75 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1540,97 +1540,6 @@ static netdev_features_t bond_fix_features(struct net_device *dev,
 	return features;
 }
 
-#define BOND_VLAN_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \
-				 NETIF_F_FRAGLIST | NETIF_F_GSO_SOFTWARE | \
-				 NETIF_F_GSO_ENCAP_ALL | \
-				 NETIF_F_HIGHDMA | NETIF_F_LRO)
-
-#define BOND_ENC_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \
-				 NETIF_F_RXCSUM | NETIF_F_GSO_SOFTWARE | \
-				 NETIF_F_GSO_PARTIAL)
-
-#define BOND_MPLS_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \
-				 NETIF_F_GSO_SOFTWARE)
-
-#define BOND_GSO_PARTIAL_FEATURES (NETIF_F_GSO_ESP)
-
-
-static void bond_compute_features(struct bonding *bond)
-{
-	netdev_features_t gso_partial_features = BOND_GSO_PARTIAL_FEATURES;
-	unsigned int dst_release_flag = IFF_XMIT_DST_RELEASE |
-					IFF_XMIT_DST_RELEASE_PERM;
-	netdev_features_t vlan_features = BOND_VLAN_FEATURES;
-	netdev_features_t enc_features  = BOND_ENC_FEATURES;
-#ifdef CONFIG_XFRM_OFFLOAD
-	netdev_features_t xfrm_features  = BOND_XFRM_FEATURES;
-#endif /* CONFIG_XFRM_OFFLOAD */
-	netdev_features_t mpls_features  = BOND_MPLS_FEATURES;
-	struct net_device *bond_dev = bond->dev;
-	struct list_head *iter;
-	struct slave *slave;
-	unsigned short max_hard_header_len = ETH_HLEN;
-	unsigned int tso_max_size = TSO_MAX_SIZE;
-	u16 tso_max_segs = TSO_MAX_SEGS;
-
-	if (!bond_has_slaves(bond))
-		goto done;
-
-	vlan_features = netdev_base_features(vlan_features);
-	mpls_features = netdev_base_features(mpls_features);
-
-	bond_for_each_slave(bond, slave, iter) {
-		vlan_features = netdev_increment_features(vlan_features,
-			slave->dev->vlan_features, BOND_VLAN_FEATURES);
-
-		enc_features = netdev_increment_features(enc_features,
-							 slave->dev->hw_enc_features,
-							 BOND_ENC_FEATURES);
-
-#ifdef CONFIG_XFRM_OFFLOAD
-		xfrm_features = netdev_increment_features(xfrm_features,
-							  slave->dev->hw_enc_features,
-							  BOND_XFRM_FEATURES);
-#endif /* CONFIG_XFRM_OFFLOAD */
-
-		gso_partial_features = netdev_increment_features(gso_partial_features,
-								 slave->dev->gso_partial_features,
-								 BOND_GSO_PARTIAL_FEATURES);
-
-		mpls_features = netdev_increment_features(mpls_features,
-							  slave->dev->mpls_features,
-							  BOND_MPLS_FEATURES);
-
-		dst_release_flag &= slave->dev->priv_flags;
-		if (slave->dev->hard_header_len > max_hard_header_len)
-			max_hard_header_len = slave->dev->hard_header_len;
-
-		tso_max_size = min(tso_max_size, slave->dev->tso_max_size);
-		tso_max_segs = min(tso_max_segs, slave->dev->tso_max_segs);
-	}
-	bond_dev->hard_header_len = max_hard_header_len;
-
-done:
-	bond_dev->gso_partial_features = gso_partial_features;
-	bond_dev->vlan_features = vlan_features;
-	bond_dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
-				    NETIF_F_HW_VLAN_CTAG_TX |
-				    NETIF_F_HW_VLAN_STAG_TX;
-#ifdef CONFIG_XFRM_OFFLOAD
-	bond_dev->hw_enc_features |= xfrm_features;
-#endif /* CONFIG_XFRM_OFFLOAD */
-	bond_dev->mpls_features = mpls_features;
-	netif_set_tso_max_segs(bond_dev, tso_max_segs);
-	netif_set_tso_max_size(bond_dev, tso_max_size);
-
-	bond_dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
-	if ((bond_dev->priv_flags & IFF_XMIT_DST_RELEASE_PERM) &&
-	    dst_release_flag == (IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM))
-		bond_dev->priv_flags |= IFF_XMIT_DST_RELEASE;
-
-	netdev_change_features(bond_dev);
-}
-
 static void bond_setup_by_slave(struct net_device *bond_dev,
 				struct net_device *slave_dev)
 {
@@ -2370,7 +2279,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	}
 
 	bond->slave_cnt++;
-	bond_compute_features(bond);
+	netdev_compute_features_from_lowers(bond->dev);
 	bond_set_carrier(bond);
 
 	/* Needs to be called before bond_select_active_slave(), which will
@@ -2622,7 +2531,7 @@ static int __bond_release_one(struct net_device *bond_dev,
 		call_netdevice_notifiers(NETDEV_RELEASE, bond->dev);
 	}
 
-	bond_compute_features(bond);
+	netdev_compute_features_from_lowers(bond->dev);
 	if (!(bond_dev->features & NETIF_F_VLAN_CHALLENGED) &&
 	    (old_features & NETIF_F_VLAN_CHALLENGED))
 		slave_info(bond_dev, slave_dev, "last VLAN challenged slave left bond - VLAN blocking is removed\n");
@@ -4126,7 +4035,7 @@ static int bond_slave_netdev_event(unsigned long event,
 	case NETDEV_FEAT_CHANGE:
 		if (!bond->notifier_ctx) {
 			bond->notifier_ctx = true;
-			bond_compute_features(bond);
+			netdev_compute_features_from_lowers(bond->dev);
 			bond->notifier_ctx = false;
 		}
 		break;
@@ -6109,7 +6018,7 @@ void bond_setup(struct net_device *bond_dev)
 	 * capable
 	 */
 
-	bond_dev->hw_features = BOND_VLAN_FEATURES |
+	bond_dev->hw_features = VIRTUAL_DEV_VLAN_FEATURES |
 				NETIF_F_HW_VLAN_CTAG_RX |
 				NETIF_F_HW_VLAN_CTAG_FILTER |
 				NETIF_F_HW_VLAN_STAG_RX |
-- 
2.50.1

[PATCHv2 net-next 3/5] team: use common function to compute the features

[Hangbin Liu]

Use the new helper netdev_compute_features_from_lowers() to compute the
team device features. This helper performs both the feature computation
and the netdev_change_features() call.

Note that such change replace the lower layer traversing currently done
using team->port_list with netdev_for_each_lower_dev(). Such change is
safe as `port_list` contains exactly the same elements as
`team->dev->adj_list.lower` and the helper is always invoked under the
RTNL lock.

With this change, the explicit netdev_change_features() in
team_add_slave() can be safely removed, as team_port_add()
already takes care of the notification via
netdev_compute_features_from_lowers(), and same thing for team_del_slave()

This also fixes missing computations for MPLS, XFRM, and TSO/GSO partial
features.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/team/team_core.c | 73 ++----------------------------------
 1 file changed, 4 insertions(+), 69 deletions(-)

diff --git a/drivers/net/team/team_core.c b/drivers/net/team/team_core.c
index 17f07eb0ee52..018d876e140a 100644
--- a/drivers/net/team/team_core.c
+++ b/drivers/net/team/team_core.c
@@ -982,63 +982,6 @@ static void team_port_disable(struct team *team,
 	team_lower_state_changed(port);
 }
 
-#define TEAM_VLAN_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \
-			    NETIF_F_FRAGLIST | NETIF_F_GSO_SOFTWARE | \
-			    NETIF_F_HIGHDMA | NETIF_F_LRO | \
-			    NETIF_F_GSO_ENCAP_ALL)
-
-#define TEAM_ENC_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \
-				 NETIF_F_RXCSUM | NETIF_F_GSO_SOFTWARE)
-
-static void __team_compute_features(struct team *team)
-{
-	struct team_port *port;
-	netdev_features_t vlan_features = TEAM_VLAN_FEATURES;
-	netdev_features_t enc_features  = TEAM_ENC_FEATURES;
-	unsigned short max_hard_header_len = ETH_HLEN;
-	unsigned int dst_release_flag = IFF_XMIT_DST_RELEASE |
-					IFF_XMIT_DST_RELEASE_PERM;
-
-	rcu_read_lock();
-	if (list_empty(&team->port_list))
-		goto done;
-
-	vlan_features = netdev_base_features(vlan_features);
-	enc_features = netdev_base_features(enc_features);
-
-	list_for_each_entry_rcu(port, &team->port_list, list) {
-		vlan_features = netdev_increment_features(vlan_features,
-					port->dev->vlan_features,
-					TEAM_VLAN_FEATURES);
-		enc_features =
-			netdev_increment_features(enc_features,
-						  port->dev->hw_enc_features,
-						  TEAM_ENC_FEATURES);
-
-		dst_release_flag &= port->dev->priv_flags;
-		if (port->dev->hard_header_len > max_hard_header_len)
-			max_hard_header_len = port->dev->hard_header_len;
-	}
-done:
-	rcu_read_unlock();
-
-	team->dev->vlan_features = vlan_features;
-	team->dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
-				     NETIF_F_HW_VLAN_CTAG_TX |
-				     NETIF_F_HW_VLAN_STAG_TX;
-	team->dev->hard_header_len = max_hard_header_len;
-
-	team->dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
-	if (dst_release_flag == (IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM))
-		team->dev->priv_flags |= IFF_XMIT_DST_RELEASE;
-}
-
-static void team_compute_features(struct team *team)
-{
-	__team_compute_features(team);
-	netdev_change_features(team->dev);
-}
-
 static int team_port_enter(struct team *team, struct team_port *port)
 {
 	int err = 0;
@@ -1300,7 +1243,7 @@ static int team_port_add(struct team *team, struct net_device *port_dev,
 	port->index = -1;
 	list_add_tail_rcu(&port->list, &team->port_list);
 	team_port_enable(team, port);
-	__team_compute_features(team);
+	netdev_compute_features_from_lowers(team->dev);
 	__team_port_change_port_added(port, !!netif_oper_up(port_dev));
 	__team_options_change_check(team);
 
@@ -1382,7 +1325,7 @@ static int team_port_del(struct team *team, struct net_device *port_dev)
 	dev_set_mtu(port_dev, port->orig.mtu);
 	kfree_rcu(port, rcu);
 	netdev_info(dev, "Port device %s removed\n", portname);
-	__team_compute_features(team);
+	netdev_compute_features_from_lowers(team->dev);
 
 	return 0;
 }
@@ -1976,9 +1919,6 @@ static int team_add_slave(struct net_device *dev, struct net_device *port_dev,
 
 	err = team_port_add(team, port_dev, extack);
 
-	if (!err)
-		netdev_change_features(dev);
-
 	return err;
 }
 
@@ -1991,11 +1931,6 @@ static int team_del_slave(struct net_device *dev, struct net_device *port_dev)
 
 	err = team_port_del(team, port_dev);
 
-	if (err)
-		return err;
-
-	netdev_change_features(dev);
-
 	return err;
 }
 
@@ -2190,7 +2125,7 @@ static void team_setup(struct net_device *dev)
 
 	dev->features |= NETIF_F_GRO;
 
-	dev->hw_features = TEAM_VLAN_FEATURES |
+	dev->hw_features = VIRTUAL_DEV_VLAN_FEATURES |
 			   NETIF_F_HW_VLAN_CTAG_RX |
 			   NETIF_F_HW_VLAN_CTAG_FILTER |
 			   NETIF_F_HW_VLAN_STAG_RX |
@@ -2994,7 +2929,7 @@ static int team_device_event(struct notifier_block *unused,
 	case NETDEV_FEAT_CHANGE:
 		if (!port->team->notifier_ctx) {
 			port->team->notifier_ctx = true;
-			team_compute_features(port->team);
+			netdev_compute_features_from_lowers(port->team->dev);
 			port->team->notifier_ctx = false;
 		}
 		break;
-- 
2.50.1

[PATCHv2 net-next 4/5] net: bridge: use common function to compute the features

[Hangbin Liu]

Previously, bridge ignored all features propagation and DST retention,
only handling explicitly the GSO limits.

By switching to the new helper netdev_compute_features_from_lowers(),
the bridge now expose additional features, depending on the lowers
capabilities.

Since br_set_gso_limits() is already covered by the helper, it can be
removed safely.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/bridge/br_if.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 98c5b9c3145f..8fe44e8c008c 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -525,20 +525,6 @@ void br_mtu_auto_adjust(struct net_bridge *br)
 	br_opt_toggle(br, BROPT_MTU_SET_BY_USER, false);
 }
 
-static void br_set_gso_limits(struct net_bridge *br)
-{
-	unsigned int tso_max_size = TSO_MAX_SIZE;
-	const struct net_bridge_port *p;
-	u16 tso_max_segs = TSO_MAX_SEGS;
-
-	list_for_each_entry(p, &br->port_list, list) {
-		tso_max_size = min(tso_max_size, p->dev->tso_max_size);
-		tso_max_segs = min(tso_max_segs, p->dev->tso_max_segs);
-	}
-	netif_set_tso_max_size(br->dev, tso_max_size);
-	netif_set_tso_max_segs(br->dev, tso_max_segs);
-}
-
 /*
  * Recomputes features using slave's features
  */
@@ -652,8 +638,6 @@ int br_add_if(struct net_bridge *br, struct net_device *dev,
 			netdev_err(dev, "failed to sync bridge static fdb addresses to this port\n");
 	}
 
-	netdev_update_features(br->dev);
-
 	br_hr = br->dev->needed_headroom;
 	dev_hr = netdev_get_fwd_headroom(dev);
 	if (br_hr < dev_hr)
@@ -694,7 +678,8 @@ int br_add_if(struct net_bridge *br, struct net_device *dev,
 		call_netdevice_notifiers(NETDEV_CHANGEADDR, br->dev);
 
 	br_mtu_auto_adjust(br);
-	br_set_gso_limits(br);
+
+	netdev_compute_features_from_lowers(br->dev);
 
 	kobject_uevent(&p->kobj, KOBJ_ADD);
 
@@ -740,7 +725,6 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 	del_nbp(p);
 
 	br_mtu_auto_adjust(br);
-	br_set_gso_limits(br);
 
 	spin_lock_bh(&br->lock);
 	changed_addr = br_stp_recalculate_bridge_id(br);
@@ -749,7 +733,7 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 	if (changed_addr)
 		call_netdevice_notifiers(NETDEV_CHANGEADDR, br->dev);
 
-	netdev_update_features(br->dev);
+	netdev_compute_features_from_lowers(br->dev);
 
 	return 0;
 }
-- 
2.50.1

[PATCHv2 net-next 5/5] selftests/net: add offload checking test for virtual interface

[Hangbin Liu]

make sure the virtual interface offload setting is correct after
changing lower devices.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 tools/testing/selftests/net/Makefile        |   1 +
 tools/testing/selftests/net/config          |   2 +
 tools/testing/selftests/net/vdev_offload.sh | 176 ++++++++++++++++++++
 3 files changed, 179 insertions(+)
 create mode 100755 tools/testing/selftests/net/vdev_offload.sh

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index eef0b8f8a7b0..e195ab4038a0 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -118,6 +118,7 @@ TEST_PROGS += tfo_passive.sh
 TEST_PROGS += broadcast_pmtu.sh
 TEST_PROGS += ipv6_force_forwarding.sh
 TEST_PROGS += route_hint.sh
+TEST_PROGS += vdev_offload.sh
 
 # YNL files, must be before "include ..lib.mk"
 YNL_GEN_FILES := busy_poller netlink-dumps
diff --git a/tools/testing/selftests/net/config b/tools/testing/selftests/net/config
index d548611e2698..0f3a64a86474 100644
--- a/tools/testing/selftests/net/config
+++ b/tools/testing/selftests/net/config
@@ -117,6 +117,7 @@ CONFIG_IP_SCTP=m
 CONFIG_NETFILTER_XT_MATCH_POLICY=m
 CONFIG_CRYPTO_ARIA=y
 CONFIG_XFRM_INTERFACE=m
+CONFIG_XFRM_OFFLOAD=y
 CONFIG_XFRM_USER=m
 CONFIG_IP_NF_MATCH_RPFILTER=m
 CONFIG_IP6_NF_MATCH_RPFILTER=m
@@ -128,3 +129,4 @@ CONFIG_NETKIT=y
 CONFIG_NET_PKTGEN=m
 CONFIG_IPV6_ILA=m
 CONFIG_IPV6_RPL_LWTUNNEL=y
+CONFIG_NET_TEAM=m
diff --git a/tools/testing/selftests/net/vdev_offload.sh b/tools/testing/selftests/net/vdev_offload.sh
new file mode 100755
index 000000000000..78fc212efd4a
--- /dev/null
+++ b/tools/testing/selftests/net/vdev_offload.sh
@@ -0,0 +1,176 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# shellcheck disable=SC1091
+source lib.sh
+
+# Set related offload on lower deivces and check if upper devices re-compute
+# Some features are fixed on veth interface. Just list here in case we have a
+# better way to test in future.
+set_offload()
+{
+	local dev="$1"
+	local state="$2"
+
+	# VLAN features
+	# NETIF_F_FRAGLIST: tx-scatter-gather-fraglist
+	# shellcheck disable=SC2154
+	ip netns exec "$ns" ethtool -K "$dev" tx-scatter-gather-fraglist "$state"
+
+	# ENC features
+	# NETIF_F_RXCSUM: rx-checksum (bond/team/bridge fixed)
+
+	# XFRM features (veth fixed, netdevsim supports)
+	# NETIF_F_HW_ESP: esp-hw-offload
+	# NETIF_F_GSO_ESP: tx-esp-segmentation
+
+	# GSO partial features
+	# NETIF_F_GSO_PARTIAL: tx-gso-partial (veth/bond fixed)
+
+	# Common features
+	# NETIF_F_SG: tx-scatter-gather
+	ip netns exec "$ns" ethtool -K "$dev" tx-scatter-gather "$state" &> /dev/null
+	# NETIF_F_GSO_SOFTWARE: NETIF_F_GSO_ACCECN: tx-tcp-accecn-segmentation
+	ip netns exec "$ns" ethtool -K "$dev" tx-tcp-accecn-segmentation "$state"
+	# NETIF_F_GSO_SOFTWARE: NETIF_F_GSO_SCTP: tx-sctp-segmentation
+	ip netns exec "$ns" ethtool -K "$dev" tx-sctp-segmentation "$state"
+	# NETIF_F_GSO_SOFTWARE: NETIF_F_GSO_FRAGLIST: tx-gso-list
+	ip netns exec "$ns" ethtool -K "$dev" tx-gso-list "$state"
+}
+
+__check_offload()
+{
+	local dev=$1
+	local opt=$2
+	local expect=$3
+
+	ip netns exec "$ns" ethtool --json -k "$dev" | \
+		jq -r -e ".[].\"$opt\".active == ${expect}" >/dev/null
+}
+
+check_offload()
+{
+	local dev=$1
+	local state=$2
+
+	__check_offload "$dev" "tx-scatter-gather-fraglist" "$state" || RET=1
+	__check_offload "$dev" "tx-scatter-gather" "$state" || RET=1
+	__check_offload "$dev" "tx-tcp-accecn-segmentation" "$state" || RET=1
+	__check_offload "$dev" "tx-sctp-segmentation" "$state" || RET=1
+	__check_offload "$dev" "tx-gso-list" "$state" || RET=1
+}
+
+setup_veth()
+{
+	# Set up test netns
+	setup_ns ns switch
+
+	# shellcheck disable=SC2154
+	ip -n "$ns" link add veth0 type veth peer name veth0 netns "$switch"
+	ip -n "$ns" link add veth1 type veth peer name veth1 netns "$switch"
+	ip -n "$switch" link set veth0 up
+	ip -n "$switch" link set veth1 up
+
+	link_0=veth0
+	link_1=veth1
+}
+
+setup_netdevsim()
+{
+	setup_ns ns
+	# The create_netdevsim() function will set the interface up. Later,
+	# when it is added to bonded, we need to set it down first. And when
+	# set down, it will have no carrier. So we need to add netdevsim ourselves.
+	modprobe netdevsim
+	udevadm settle
+	echo "0 2" | ip netns exec "$ns" tee /sys/bus/netdevsim/new_device >/dev/null
+	link_0=$(ip netns exec "$ns" ls /sys/bus/netdevsim/devices/netdevsim0/net | head -n 1)
+	link_1=$(ip netns exec "$ns" ls /sys/bus/netdevsim/devices/netdevsim0/net | tail -n 1)
+}
+
+cleanup()
+{
+	cleanup_netdevsim 0
+	cleanup_all_ns
+}
+
+setup_bond()
+{
+	ip -n "$ns" link set "$link_0" nomaster
+	ip -n "$ns" link set "$link_1" nomaster
+	ip -n "$ns" link add bond0 type bond mode active-backup miimon 100
+	ip -n "$ns" link set "$link_0" master bond0
+	ip -n "$ns" link set "$link_1" master bond0
+	ip -n "$ns" link set bond0 up
+}
+
+setup_team()
+{
+	ip -n "$ns" link set "$link_0" nomaster
+	ip -n "$ns" link set "$link_1" nomaster
+	ip -n "$ns" link add team0 type team
+	ip -n "$ns" link set "$link_0" master team0
+	ip -n "$ns" link set "$link_1" master team0
+	ip -n "$ns" link set team0 up
+}
+
+setup_bridge()
+{
+	ip -n "$ns" link set "$link_0" nomaster
+	ip -n "$ns" link set "$link_1" nomaster
+	ip -n "$ns" link add br0 type bridge
+	ip -n "$ns" link set "$link_0" master br0
+	ip -n "$ns" link set "$link_1" master br0
+	ip -n "$ns" link set br0 up
+}
+
+check_xfrm()
+{
+	local dev=$1
+	local src=192.0.2.1
+	local dst=192.0.2.2
+	local key="0x3132333435363738393031323334353664636261"
+
+	RET=0
+
+	ip -n "$ns" xfrm state flush
+	ip -n "$ns" xfrm state add proto esp src "$src" dst "$dst" spi 9 \
+		mode transport reqid 42 aead "rfc4106(gcm(aes))" "$key" 128 \
+		sel src "$src"/24 dst "$dst"/24 offload dev "$dev" dir out
+
+	# shellcheck disable=SC2034
+	ip -n "$ns" xfrm state list | grep -q "crypto offload parameters: dev $dev dir" || RET=1
+	log_test "$dev" "xfrm offload"
+}
+
+do_test()
+{
+	local dev=$1
+
+	RET=0
+	set_offload veth0 "on"
+	set_offload veth1 "on"
+	check_offload "$dev" "true"
+	log_test "$dev" "enable offload"
+
+	RET=0
+	set_offload veth0 "off"
+	set_offload veth1 "off"
+	check_offload "$dev" "false"
+	log_test "$dev" "disable offload"
+}
+
+trap cleanup EXIT
+setup_veth
+setup_bond
+do_test bond0
+setup_team
+do_test team0
+setup_bridge
+do_test br0
+
+# Check NETIF_F_HW_ESP
+# Only test bond as team and bridge haven't implemented xfrm offload
+setup_netdevsim
+setup_bond
+check_xfrm bond0
-- 
2.50.1

Re: [PATCHv2 net-next 1/5] net: add a common function to compute features from lowers devices

[Sabrina Dubroca]

2025-09-02, 07:25:58 +0000, Hangbin Liu wrote:
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1d1650d9ecff..5c1c8b016c8e 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -12577,6 +12577,82 @@ netdev_features_t netdev_increment_features(netdev_features_t all,
>  }
>  EXPORT_SYMBOL(netdev_increment_features);
>  
> +/**
> + *	netdev_compute_features_from_lowers - compute feature from lowers
> + *	@dev: the upper device
> + *
> + *	Recompute the upper device's feature based on all lower devices.
> + */
> +void netdev_compute_features_from_lowers(struct net_device *dev)
> +{
> +	unsigned int dst_release_flag = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
> +	netdev_features_t gso_partial_features = VIRTUAL_DEV_GSO_PARTIAL_FEATURES;
> +#ifdef CONFIG_XFRM_OFFLOAD
> +	netdev_features_t xfrm_features  = VIRTUAL_DEV_XFRM_FEATURES;
> +#endif
> +	netdev_features_t mpls_features  = VIRTUAL_DEV_MPLS_FEATURES;
> +	netdev_features_t vlan_features = VIRTUAL_DEV_VLAN_FEATURES;
> +	netdev_features_t enc_features  = VIRTUAL_DEV_ENC_FEATURES;
> +	unsigned int tso_max_size = TSO_MAX_SIZE;
> +	u16 tso_max_segs = TSO_MAX_SEGS;
> +	struct net_device *lower_dev;
> +	struct list_head *iter;
> +
> +	mpls_features = netdev_base_features(mpls_features);
> +	vlan_features = netdev_base_features(vlan_features);
> +	enc_features = netdev_base_features(enc_features);
> +
> +	netdev_for_each_lower_dev(dev, lower_dev, iter) {
> +		gso_partial_features = netdev_increment_features(gso_partial_features,
> +								 lower_dev->gso_partial_features,
> +								 VIRTUAL_DEV_GSO_PARTIAL_FEATURES);
> +
> +		vlan_features = netdev_increment_features(vlan_features,
> +							  lower_dev->vlan_features,
> +							  VIRTUAL_DEV_VLAN_FEATURES);
> +
> +#ifdef CONFIG_XFRM_OFFLOAD
> +		xfrm_features = netdev_increment_features(xfrm_features,
> +							  lower_dev->hw_enc_features,
> +							  VIRTUAL_DEV_XFRM_FEATURES);
> +#endif
> +
> +		enc_features = netdev_increment_features(enc_features,
> +							 lower_dev->hw_enc_features,
> +							 VIRTUAL_DEV_ENC_FEATURES);
> +
> +		mpls_features = netdev_increment_features(mpls_features,
> +							  lower_dev->mpls_features,
> +							  VIRTUAL_DEV_MPLS_FEATURES);
> +
> +		dst_release_flag &= lower_dev->priv_flags;
> +
> +		tso_max_size = min(tso_max_size, lower_dev->tso_max_size);
> +		tso_max_segs = min(tso_max_segs, lower_dev->tso_max_segs);
> +	}
> +
> +	dev->gso_partial_features = gso_partial_features;
> +	dev->vlan_features = vlan_features;
> +#ifdef CONFIG_XFRM_OFFLOAD
> +	dev->hw_enc_features |= xfrm_features;
> +#endif

I'm not completely sure we want xfrm_features for upper devices other
than bonding [1], but this will get overwritten immediately
afterwards:

> +	dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
> +				    NETIF_F_HW_VLAN_CTAG_TX |
> +				    NETIF_F_HW_VLAN_STAG_TX;


[1] those lines in bond_compute_features were only added alongside
bond IPsec offload, see 18cb261afd7b ("bonding: support hardware
encryption offload to slaves")

but AFAIU hw_enc_features is only used as a mask over dev->features so
it shouldn't be a problem to have xfrm stuff in bridge/team as well

-- 
Sabrina

Re: [PATCHv2 net-next 5/5] selftests/net: add offload checking test for virtual interface

[Sabrina Dubroca]

2025-09-02, 07:26:02 +0000, Hangbin Liu wrote:
> diff --git a/tools/testing/selftests/net/vdev_offload.sh b/tools/testing/selftests/net/vdev_offload.sh
> new file mode 100755
> index 000000000000..78fc212efd4a
> --- /dev/null
> +++ b/tools/testing/selftests/net/vdev_offload.sh
> @@ -0,0 +1,176 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# shellcheck disable=SC1091
> +source lib.sh
> +
> +# Set related offload on lower deivces and check if upper devices re-compute

nit: s/deivces/devices/

> +# Some features are fixed on veth interface. Just list here in case we have a
> +# better way to test in future.
> +set_offload()
> +{

[...]
> +check_xfrm()
> +{
> +	local dev=$1
> +	local src=192.0.2.1
> +	local dst=192.0.2.2
> +	local key="0x3132333435363738393031323334353664636261"
> +
> +	RET=0
> +
> +	ip -n "$ns" xfrm state flush
> +	ip -n "$ns" xfrm state add proto esp src "$src" dst "$dst" spi 9 \
> +		mode transport reqid 42 aead "rfc4106(gcm(aes))" "$key" 128 \
> +		sel src "$src"/24 dst "$dst"/24 offload dev "$dev" dir out

It's maybe not something you would expect, but this codepath will not
check that NETIF_F_HW_ESP is set on $dev (you can verify that by
running "ip xfrm state add ... offload ..." on the same bond+netdevsim
combination before/after toggling esp-hw-offload on/off for the
bond). Why not use __check_offload again for this feature?

> +	# shellcheck disable=SC2034
> +	ip -n "$ns" xfrm state list | grep -q "crypto offload parameters: dev $dev dir" || RET=1
> +	log_test "$dev" "xfrm offload"
> +}

-- 
Sabrina

Re: [PATCHv2 net-next 1/5] net: add a common function to compute features from lowers devices

[Jakub Kicinski]

On Tue,  2 Sep 2025 07:25:58 +0000 Hangbin Liu wrote:
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -5279,6 +5279,25 @@ int __netdev_update_features(struct net_device *dev);
>  void netdev_update_features(struct net_device *dev);
>  void netdev_change_features(struct net_device *dev);
>  
> +/* netdevice features */
> +#define VIRTUAL_DEV_VLAN_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \
> +					 NETIF_F_FRAGLIST | NETIF_F_GSO_SOFTWARE | \
> +					 NETIF_F_GSO_ENCAP_ALL | \
> +					 NETIF_F_HIGHDMA | NETIF_F_LRO)
> +
> +#define VIRTUAL_DEV_ENC_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \
> +					 NETIF_F_RXCSUM | NETIF_F_GSO_SOFTWARE | \
> +					 NETIF_F_GSO_PARTIAL)
> +
> +#define VIRTUAL_DEV_MPLS_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \
> +					 NETIF_F_GSO_SOFTWARE)
> +
> +#define VIRTUAL_DEV_XFRM_FEATURES	(NETIF_F_HW_ESP | NETIF_F_HW_ESP_TX_CSUM | \
> +					 NETIF_F_GSO_ESP)
> +
> +#define VIRTUAL_DEV_GSO_PARTIAL_FEATURES (NETIF_F_GSO_ESP)
> +void netdev_compute_features_from_lowers(struct net_device *dev);

Isn't this what the ALL_FOR_ALL, ONE_FOR_ALL etc. flag sets were
supposed to also achieve? Please try to move the new code closer
to those, both in terms of where they are defined and naming...

Re: [PATCHv2 net-next 1/5] net: add a common function to compute features from lowers devices

[Hangbin Liu]

On Sat, Sep 06, 2025 at 07:42:48PM +0200, Sabrina Dubroca wrote:
> > +	dev->gso_partial_features = gso_partial_features;
> > +	dev->vlan_features = vlan_features;
> > +#ifdef CONFIG_XFRM_OFFLOAD
> > +	dev->hw_enc_features |= xfrm_features;
> > +#endif
> 
> I'm not completely sure we want xfrm_features for upper devices other
> than bonding [1], but this will get overwritten immediately
> afterwards:
> 
> > +	dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
> > +				    NETIF_F_HW_VLAN_CTAG_TX |
> > +				    NETIF_F_HW_VLAN_STAG_TX;

Ah.. right. I need more careful when change the order.

> 
> 
> [1] those lines in bond_compute_features were only added alongside
> bond IPsec offload, see 18cb261afd7b ("bonding: support hardware
> encryption offload to slaves")
> 
> but AFAIU hw_enc_features is only used as a mask over dev->features so
> it shouldn't be a problem to have xfrm stuff in bridge/team as well

In this case I will keep this setting. Putting the feature compute to
another function will violate our original intention of putting
the feature compute together as much as possible.

Thanks
Hangbin

Re: [PATCHv2 net-next 5/5] selftests/net: add offload checking test for virtual interface

[Hangbin Liu]

On Sat, Sep 06, 2025 at 11:30:58PM +0200, Sabrina Dubroca wrote:
> > +check_xfrm()
> > +{
> > +	local dev=$1
> > +	local src=192.0.2.1
> > +	local dst=192.0.2.2
> > +	local key="0x3132333435363738393031323334353664636261"
> > +
> > +	RET=0
> > +
> > +	ip -n "$ns" xfrm state flush
> > +	ip -n "$ns" xfrm state add proto esp src "$src" dst "$dst" spi 9 \
> > +		mode transport reqid 42 aead "rfc4106(gcm(aes))" "$key" 128 \
> > +		sel src "$src"/24 dst "$dst"/24 offload dev "$dev" dir out
> 
> It's maybe not something you would expect, but this codepath will not
> check that NETIF_F_HW_ESP is set on $dev (you can verify that by
> running "ip xfrm state add ... offload ..." on the same bond+netdevsim
> combination before/after toggling esp-hw-offload on/off for the
> bond). Why not use __check_offload again for this feature?

The esp-hw-offload is fixed on netdevsim

# ethtool -k eni0np1 | grep -i esp-hw-offload
esp-hw-offload: on [fixed]

There is no way to disable it. After we add the netdevsim to bond,
the bond also shows "esp-hw-offload off" as the flag is inherit
in dev->hw_enc_features, not dev->features.

It looks the only way to check if bond dev->hw_enc_features has NETIF_F_HW_ESP
is try set xfrm offload. As

static int xfrm_api_check(struct net_device *dev)
{
#ifdef CONFIG_XFRM_OFFLOAD
        if ((dev->features & NETIF_F_HW_ESP_TX_CSUM) &&
            !(dev->features & NETIF_F_HW_ESP))
                return NOTIFY_BAD;

        if ((dev->features & NETIF_F_HW_ESP) &&
            (!(dev->xfrmdev_ops &&
               dev->xfrmdev_ops->xdo_dev_state_add &&
               dev->xfrmdev_ops->xdo_dev_state_delete)))
                return NOTIFY_BAD;

Please correct me if I made any mistake.

Thanks
Hangbin

Re: [PATCHv2 net-next 5/5] selftests/net: add offload checking test for virtual interface

[Sabrina Dubroca]

2025-09-08, 04:15:38 +0000, Hangbin Liu wrote:
> On Sat, Sep 06, 2025 at 11:30:58PM +0200, Sabrina Dubroca wrote:
> > > +check_xfrm()
> > > +{
> > > +	local dev=$1
> > > +	local src=192.0.2.1
> > > +	local dst=192.0.2.2
> > > +	local key="0x3132333435363738393031323334353664636261"
> > > +
> > > +	RET=0
> > > +
> > > +	ip -n "$ns" xfrm state flush
> > > +	ip -n "$ns" xfrm state add proto esp src "$src" dst "$dst" spi 9 \
> > > +		mode transport reqid 42 aead "rfc4106(gcm(aes))" "$key" 128 \
> > > +		sel src "$src"/24 dst "$dst"/24 offload dev "$dev" dir out
> > 
> > It's maybe not something you would expect, but this codepath will not
> > check that NETIF_F_HW_ESP is set on $dev (you can verify that by
> > running "ip xfrm state add ... offload ..." on the same bond+netdevsim
> > combination before/after toggling esp-hw-offload on/off for the
> > bond). Why not use __check_offload again for this feature?
> 
> The esp-hw-offload is fixed on netdevsim
> 
> # ethtool -k eni0np1 | grep -i esp-hw-offload
> esp-hw-offload: on [fixed]
> 
> There is no way to disable it.

I don't think this is intentional. nsim_ipsec_init only adds
NSIM_ESP_FEATURES to ->features but not to ->hw_features, but I think
it was just forgotten. I added a few in 494bd83bb519 ("netdevsim: add
more hw_features"), extending nsim_ipsec_init (and nsim_macsec_init
since I made the same mistake) to also add features to ->hw_features
would make sense to me.


> After we add the netdevsim to bond,
> the bond also shows "esp-hw-offload off" as the flag is inherit
> in dev->hw_enc_features, not dev->features.

Did you mean dev->hw_features?

> It looks the only way to check if bond dev->hw_enc_features has NETIF_F_HW_ESP
> is try set xfrm offload. As

Was this test meant to check hw_enc_features?

To check hw_enc_features, I think the only way would be sending GSO
packets, since it's only used in those situations.


> static int xfrm_api_check(struct net_device *dev)
> {

But this doesn't get called when creating a new xfrm state. Trying to
create a new offloaded xfrm state doesn't look at any of the
netdev->*features (and we can't change that behavior anymore).

xfrm_api_check only gets called for NETDEV_REGISTER/NETDEV_FEAT_CHANGE
to validate whether the netdevice is set up correctly.

> #ifdef CONFIG_XFRM_OFFLOAD
>         if ((dev->features & NETIF_F_HW_ESP_TX_CSUM) &&
>             !(dev->features & NETIF_F_HW_ESP))
>                 return NOTIFY_BAD;
> 
>         if ((dev->features & NETIF_F_HW_ESP) &&
>             (!(dev->xfrmdev_ops &&
>                dev->xfrmdev_ops->xdo_dev_state_add &&
>                dev->xfrmdev_ops->xdo_dev_state_delete)))
>                 return NOTIFY_BAD;
> 
> Please correct me if I made any mistake.
> 
> Thanks
> Hangbin

-- 
Sabrina

Re: [PATCHv2 net-next 5/5] selftests/net: add offload checking test for virtual interface

[Hangbin Liu]

On Mon, Sep 08, 2025 at 11:36:21AM +0200, Sabrina Dubroca wrote:
> > The esp-hw-offload is fixed on netdevsim
> > 
> > # ethtool -k eni0np1 | grep -i esp-hw-offload
> > esp-hw-offload: on [fixed]
> > 
> > There is no way to disable it.
> 
> I don't think this is intentional. nsim_ipsec_init only adds
> NSIM_ESP_FEATURES to ->features but not to ->hw_features, but I think
> it was just forgotten. I added a few in 494bd83bb519 ("netdevsim: add
> more hw_features"), extending nsim_ipsec_init (and nsim_macsec_init
> since I made the same mistake) to also add features to ->hw_features
> would make sense to me.

This could be done in another patch.

> 
> > After we add the netdevsim to bond,
> > the bond also shows "esp-hw-offload off" as the flag is inherit
> > in dev->hw_enc_features, not dev->features.
> 
> Did you mean dev->hw_features?

No, the xfrm_features in patch 01 updates dev->hw_enc_features, not
dev->hw_features. Do you think if we should update dev->hw_features in the
patch?

> 
> > It looks the only way to check if bond dev->hw_enc_features has NETIF_F_HW_ESP
> > is try set xfrm offload. As
> 
> Was this test meant to check hw_enc_features?
> 
> To check hw_enc_features, I think the only way would be sending GSO
> packets, since it's only used in those situations.

Oh.. That would make the test complex. Can we ignore this test first?

BTW, I'm a bit lost in the callbacks.gso_segment. e.g.

esp4_gso_segment
 - xfrm4_outer_mode_gso_segment
   - xfrm4_transport_gso_segment
     - ops->callbacks.gso_segment

But who calls esp4_gso_segment? I can't find where the features is assigned.

> 
> 
> > static int xfrm_api_check(struct net_device *dev)
> > {
> 
> But this doesn't get called when creating a new xfrm state. Trying to
> create a new offloaded xfrm state doesn't look at any of the
> netdev->*features (and we can't change that behavior anymore).
> 
> xfrm_api_check only gets called for NETDEV_REGISTER/NETDEV_FEAT_CHANGE
> to validate whether the netdevice is set up correctly.

Thanks for correcting me.

Regards
Hangbin