[PATCHv6 net-next 0/4] net: common feature compute for upper interface

[PATCHv6 net-next 0/4] 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.

v6:
  * no update, only rename UPPER_DEV_* to MASTER_UPPER_DEV_* (Jiri Pirko)

v5:
  * rename VIRTUAL_DEV_* to UPPER_DEV_* (Jiri Pirko)
  * use IS_ENABLED() instead of ifdef (Simon Horman)
  * init max_headroom/tailroom (Simon Horman)
  * link: https://lore.kernel.org/netdev/20251016033828.59324-1-liuhangbin@gmail.com

v4:
  * update needed_{headroom, tailroom} in the common helper (Ido Schimmel)
  * remove unneeded err in team (Stanislav Fomichev)
  * remove selftest as `ethtool -k` does not test the dev->*_features. We
    can add back the selftest when there is a good way to test. (Sabrina Dubroca)
  * link: https://lore.kernel.org/netdev/20251014080217.47988-1-liuhangbin@gmail.com

v3:
  a) fix hw_enc_features assign order (Sabrina Dubroca)
  b) set virtual dev feature definition in netdev_features.h (Jakub Kicinski)
  c) remove unneeded err in team_del_slave (Stanislav Fomichev)
  d) remove NETIF_F_HW_ESP test as it needs to be test with GSO pkts (Sabrina Dubroca)

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 (4):
  net: add a common function to compute features for upper 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

 drivers/net/bonding/bond_main.c | 99 ++-------------------------------
 drivers/net/team/team_core.c    | 83 ++-------------------------
 include/linux/netdev_features.h | 18 ++++++
 include/linux/netdevice.h       |  1 +
 net/bridge/br_if.c              | 22 +-------
 net/core/dev.c                  | 88 +++++++++++++++++++++++++++++
 6 files changed, 120 insertions(+), 191 deletions(-)

-- 
2.50.1

[PATCHv6 net-next 1/4] net: add a common function to compute features for upper devices

[Hangbin Liu]

Some high level software 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/netdev_features.h | 18 +++++++
 include/linux/netdevice.h       |  1 +
 net/core/dev.c                  | 88 +++++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 7a01c518e573..93e4da7046a1 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -255,6 +255,24 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)
 				 NETIF_F_GSO_UDP_TUNNEL |		\
 				 NETIF_F_GSO_UDP_TUNNEL_CSUM)
 
+/* virtual device features */
+#define MASTER_UPPER_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 MASTER_UPPER_DEV_ENC_FEATURES	 (NETIF_F_HW_CSUM | NETIF_F_SG | \
+					  NETIF_F_RXCSUM | NETIF_F_GSO_SOFTWARE | \
+					  NETIF_F_GSO_PARTIAL)
+
+#define MASTER_UPPER_DEV_MPLS_FEATURES	 (NETIF_F_HW_CSUM | NETIF_F_SG | \
+					  NETIF_F_GSO_SOFTWARE)
+
+#define MASTER_UPPER_DEV_XFRM_FEATURES	 (NETIF_F_HW_ESP | NETIF_F_HW_ESP_TX_CSUM | \
+					  NETIF_F_GSO_ESP)
+
+#define MASTER_UPPER_DEV_GSO_PARTIAL_FEATURES (NETIF_F_GSO_ESP)
+
 static inline netdev_features_t netdev_base_features(netdev_features_t features)
 {
 	features &= ~NETIF_F_ONE_FOR_ALL;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d1a687444b27..7f5aad5cc9a1 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -5304,6 +5304,7 @@ static inline netdev_features_t netdev_add_tso_features(netdev_features_t featur
 int __netdev_update_features(struct net_device *dev);
 void netdev_update_features(struct net_device *dev);
 void netdev_change_features(struct net_device *dev);
+void netdev_compute_master_upper_features(struct net_device *dev, bool update_header);
 
 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 33e6101dbc45..50d8ebd13a56 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -12641,6 +12641,94 @@ netdev_features_t netdev_increment_features(netdev_features_t all,
 }
 EXPORT_SYMBOL(netdev_increment_features);
 
+/**
+ *	netdev_compute_master_upper_features - compute feature from lowers
+ *	@dev: the upper device
+ *	@update_header: whether to update upper device's header_len/headroom/tailroom
+ *
+ *	Recompute the upper device's feature based on all lower devices.
+ */
+void netdev_compute_master_upper_features(struct net_device *dev, bool update_header)
+{
+	unsigned int dst_release_flag = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
+	netdev_features_t gso_partial_features = MASTER_UPPER_DEV_GSO_PARTIAL_FEATURES;
+	netdev_features_t xfrm_features = MASTER_UPPER_DEV_XFRM_FEATURES;
+	netdev_features_t mpls_features = MASTER_UPPER_DEV_MPLS_FEATURES;
+	netdev_features_t vlan_features = MASTER_UPPER_DEV_VLAN_FEATURES;
+	netdev_features_t enc_features = MASTER_UPPER_DEV_ENC_FEATURES;
+	unsigned short max_header_len = ETH_HLEN;
+	unsigned int tso_max_size = TSO_MAX_SIZE;
+	unsigned short max_headroom = 0;
+	unsigned short max_tailroom = 0;
+	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,
+								 MASTER_UPPER_DEV_GSO_PARTIAL_FEATURES);
+
+		vlan_features = netdev_increment_features(vlan_features,
+							  lower_dev->vlan_features,
+							  MASTER_UPPER_DEV_VLAN_FEATURES);
+
+		enc_features = netdev_increment_features(enc_features,
+							 lower_dev->hw_enc_features,
+							 MASTER_UPPER_DEV_ENC_FEATURES);
+
+		if (IS_ENABLED(CONFIG_XFRM_OFFLOAD))
+			xfrm_features = netdev_increment_features(xfrm_features,
+								  lower_dev->hw_enc_features,
+								  MASTER_UPPER_DEV_XFRM_FEATURES);
+
+		mpls_features = netdev_increment_features(mpls_features,
+							  lower_dev->mpls_features,
+							  MASTER_UPPER_DEV_MPLS_FEATURES);
+
+		dst_release_flag &= lower_dev->priv_flags;
+
+		if (update_header) {
+			max_header_len = max(max_header_len, lower_dev->hard_header_len);
+			max_headroom = max(max_headroom, lower_dev->needed_headroom);
+			max_tailroom = max(max_tailroom, lower_dev->needed_tailroom);
+		}
+
+		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;
+	dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
+			       NETIF_F_HW_VLAN_CTAG_TX |
+			       NETIF_F_HW_VLAN_STAG_TX;
+	if (IS_ENABLED(CONFIG_XFRM_OFFLOAD))
+		dev->hw_enc_features |= xfrm_features;
+	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;
+
+	if (update_header) {
+		dev->hard_header_len = max_header_len;
+		dev->needed_headroom = max_headroom;
+		dev->needed_tailroom = max_tailroom;
+	}
+
+	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_master_upper_features);
+
 static struct hlist_head * __net_init netdev_create_hash(void)
 {
 	int i;
-- 
2.50.1

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

[Hangbin Liu]

Use the new functon netdev_compute_master_upper_features() 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_master_upper_features() 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 4da619210c1f..cd7da6ed8c6b 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1468,97 +1468,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)
 {
@@ -2273,7 +2182,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	}
 
 	bond->slave_cnt++;
-	bond_compute_features(bond);
+	netdev_compute_master_upper_features(bond->dev, true);
 	bond_set_carrier(bond);
 
 	/* Needs to be called before bond_select_active_slave(), which will
@@ -2525,7 +2434,7 @@ static int __bond_release_one(struct net_device *bond_dev,
 		call_netdevice_notifiers(NETDEV_RELEASE, bond->dev);
 	}
 
-	bond_compute_features(bond);
+	netdev_compute_master_upper_features(bond->dev, true);
 	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");
@@ -4028,7 +3937,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_master_upper_features(bond->dev, true);
 			bond->notifier_ctx = false;
 		}
 		break;
@@ -6011,7 +5920,7 @@ void bond_setup(struct net_device *bond_dev)
 	 * capable
 	 */
 
-	bond_dev->hw_features = BOND_VLAN_FEATURES |
+	bond_dev->hw_features = MASTER_UPPER_DEV_VLAN_FEATURES |
 				NETIF_F_HW_VLAN_CTAG_RX |
 				NETIF_F_HW_VLAN_CTAG_FILTER |
 				NETIF_F_HW_VLAN_STAG_RX |
-- 
2.50.1

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

[Hangbin Liu]

Use the new helper netdev_compute_master_upper_features() 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_master_upper_features(), 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 | 83 +++---------------------------------
 1 file changed, 6 insertions(+), 77 deletions(-)

diff --git a/drivers/net/team/team_core.c b/drivers/net/team/team_core.c
index 17f07eb0ee52..29dc04c299a3 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_master_upper_features(team->dev, true);
 	__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_master_upper_features(team->dev, true);
 
 	return 0;
 }
@@ -1970,33 +1913,19 @@ static int team_add_slave(struct net_device *dev, struct net_device *port_dev,
 			  struct netlink_ext_ack *extack)
 {
 	struct team *team = netdev_priv(dev);
-	int err;
 
 	ASSERT_RTNL();
 
-	err = team_port_add(team, port_dev, extack);
-
-	if (!err)
-		netdev_change_features(dev);
-
-	return err;
+	return team_port_add(team, port_dev, extack);
 }
 
 static int team_del_slave(struct net_device *dev, struct net_device *port_dev)
 {
 	struct team *team = netdev_priv(dev);
-	int err;
 
 	ASSERT_RTNL();
 
-	err = team_port_del(team, port_dev);
-
-	if (err)
-		return err;
-
-	netdev_change_features(dev);
-
-	return err;
+	return team_port_del(team, port_dev);
 }
 
 static netdev_features_t team_fix_features(struct net_device *dev,
@@ -2190,7 +2119,7 @@ static void team_setup(struct net_device *dev)
 
 	dev->features |= NETIF_F_GRO;
 
-	dev->hw_features = TEAM_VLAN_FEATURES |
+	dev->hw_features = MASTER_UPPER_DEV_VLAN_FEATURES |
 			   NETIF_F_HW_VLAN_CTAG_RX |
 			   NETIF_F_HW_VLAN_CTAG_FILTER |
 			   NETIF_F_HW_VLAN_STAG_RX |
@@ -2994,7 +2923,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_master_upper_features(port->team->dev, true);
 			port->team->notifier_ctx = false;
 		}
 		break;
-- 
2.50.1

[PATCHv6 net-next 4/4] 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_master_upper_features(), 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.

Bridge has it's own way to update needed_headroom. So we don't need to
update it in the helper.

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..a6d4c44890fd 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_master_upper_features(br->dev, false);
 
 	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_master_upper_features(br->dev, false);
 
 	return 0;
 }
-- 
2.50.1

Re: [PATCHv6 net-next 0/4] net: common feature compute for upper interface

[Jiri Pirko]

Fri, Oct 17, 2025 at 05:41:51AM +0200, liuhangbin@gmail.com wrote:
>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.

Looks good to me.

set-
Reviewed-by: Jiri Pirko <jiri@nvidia.com>


>
>v6:
>  * no update, only rename UPPER_DEV_* to MASTER_UPPER_DEV_* (Jiri Pirko)
>
>v5:
>  * rename VIRTUAL_DEV_* to UPPER_DEV_* (Jiri Pirko)
>  * use IS_ENABLED() instead of ifdef (Simon Horman)
>  * init max_headroom/tailroom (Simon Horman)
>  * link: https://lore.kernel.org/netdev/20251016033828.59324-1-liuhangbin@gmail.com
>
>v4:
>  * update needed_{headroom, tailroom} in the common helper (Ido Schimmel)
>  * remove unneeded err in team (Stanislav Fomichev)
>  * remove selftest as `ethtool -k` does not test the dev->*_features. We
>    can add back the selftest when there is a good way to test. (Sabrina Dubroca)
>  * link: https://lore.kernel.org/netdev/20251014080217.47988-1-liuhangbin@gmail.com
>
>v3:
>  a) fix hw_enc_features assign order (Sabrina Dubroca)
>  b) set virtual dev feature definition in netdev_features.h (Jakub Kicinski)
>  c) remove unneeded err in team_del_slave (Stanislav Fomichev)
>  d) remove NETIF_F_HW_ESP test as it needs to be test with GSO pkts (Sabrina Dubroca)
>
>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 (4):
>  net: add a common function to compute features for upper 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
>
> drivers/net/bonding/bond_main.c | 99 ++-------------------------------
> drivers/net/team/team_core.c    | 83 ++-------------------------
> include/linux/netdev_features.h | 18 ++++++
> include/linux/netdevice.h       |  1 +
> net/bridge/br_if.c              | 22 +-------
> net/core/dev.c                  | 88 +++++++++++++++++++++++++++++
> 6 files changed, 120 insertions(+), 191 deletions(-)
>
>-- 
>2.50.1
>

Re: [PATCHv6 net-next 1/4] net: add a common function to compute features for upper devices

[Sabrina Dubroca]

2025-10-17, 03:41:52 +0000, Hangbin Liu wrote:
> Some high level software 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>

No objection to this patch/series, just a nit and some discussion below, so:

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>


[...]
> +/**
> + *	netdev_compute_master_upper_features - compute feature from lowers

nit: I'm slightly annoyed (that's not quite the right word, sorry)
that we're adding a new function to "compute features" that doesn't
touch netdev->features, but I can't come up with a better name
(the best I got was "compute extra features" and it doesn't help).

> + *	@dev: the upper device
> + *	@update_header: whether to update upper device's header_len/headroom/tailroom
> + *
> + *	Recompute the upper device's feature based on all lower devices.
> + */
> +void netdev_compute_master_upper_features(struct net_device *dev, bool update_header)
> +{
[...]
> +	netif_set_tso_max_segs(dev, tso_max_segs);
> +	netif_set_tso_max_size(dev, tso_max_size);
> +
> +	netdev_change_features(dev);

Maybe a dumb idea: I'm wondering if we're doing this from the wrong
side.

Right now we have:

[some device op] -> [this new function] -> netdev_change_features -> __netdev_update_features -> ndo_fix_features

Would it make more sense to go instead:

[some device op] -> netdev_change_features -> __netdev_update_features -> ndo_fix_features -> [this new function]

?


Possible benefit: not forgetting to fix up the "extra" features in
some cases?  (ie calling netdev_change_features when we should have
called netdev_compute_master_upper_features)

> +}

-- 
Sabrina

Re: [PATCHv6 net-next 2/4] bonding: use common function to compute the features

[Sabrina Dubroca]

2025-10-17, 03:41:53 +0000, Hangbin Liu wrote:
> Use the new functon netdev_compute_master_upper_features() 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_master_upper_features() 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(-)

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

-- 
Sabrina

Re: [PATCHv6 net-next 3/4] team: use common function to compute the features

[Sabrina Dubroca]

2025-10-17, 03:41:54 +0000, Hangbin Liu wrote:
> Use the new helper netdev_compute_master_upper_features() 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_master_upper_features(), 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 | 83 +++---------------------------------
>  1 file changed, 6 insertions(+), 77 deletions(-)

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

-- 
Sabrina

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

[Sabrina Dubroca]

2025-10-17, 03:41:55 +0000, Hangbin Liu wrote:
> Previously, bridge ignored all features propagation and DST retention,
> only handling explicitly the GSO limits.
> 
> By switching to the new helper netdev_compute_master_upper_features(), 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.
> 
> Bridge has it's own way to update needed_headroom. So we don't need to
> update it in the helper.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  net/bridge/br_if.c | 22 +++-------------------
>  1 file changed, 3 insertions(+), 19 deletions(-)

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

-- 
Sabrina

Re: [PATCHv6 net-next 1/4] net: add a common function to compute features for upper devices

[Hangbin Liu]

On Mon, Oct 20, 2025 at 11:10:14AM +0200, Sabrina Dubroca wrote:
> > +/**
> > + *	netdev_compute_master_upper_features - compute feature from lowers
> 
> nit: I'm slightly annoyed (that's not quite the right word, sorry)
> that we're adding a new function to "compute features" that doesn't
> touch netdev->features, but I can't come up with a better name
> (the best I got was "compute extra features" and it doesn't help).

Ah, yes, the term "compute features" can be confusing since we don’t actually
 update netdev->features. We can rename it if there’s a better alternative.

> 
> > + *	@dev: the upper device
> > + *	@update_header: whether to update upper device's header_len/headroom/tailroom
> > + *
> > + *	Recompute the upper device's feature based on all lower devices.
> > + */
> > +void netdev_compute_master_upper_features(struct net_device *dev, bool update_header)
> > +{
> [...]
> > +	netif_set_tso_max_segs(dev, tso_max_segs);
> > +	netif_set_tso_max_size(dev, tso_max_size);
> > +
> > +	netdev_change_features(dev);
> 
> Maybe a dumb idea: I'm wondering if we're doing this from the wrong
> side.
> 
> Right now we have:
> 
> [some device op] -> [this new function] -> netdev_change_features -> __netdev_update_features -> ndo_fix_features
> 
> Would it make more sense to go instead:
> 
> [some device op] -> netdev_change_features -> __netdev_update_features -> ndo_fix_features -> [this new function]

Since we actually doesn't touch netdev->feature. I think [this new function]
and netdev_change_features() should be in parallel relationship.

> 
> Possible benefit: not forgetting to fix up the "extra" features in
> some cases?  (ie calling netdev_change_features when we should have
> called netdev_compute_master_upper_features)

That’s a good reason to call them together. However, ndo_fix_features is used
for computing new features for later use. Since we both compute and set them,
maybe we should put this in ndo_set_features instead?

Thanks
Hangbin

Re: [PATCHv6 net-next 1/4] net: add a common function to compute features for upper devices

[Paolo Abeni]

On 10/20/25 11:10 AM, Sabrina Dubroca wrote:
> 2025-10-17, 03:41:52 +0000, Hangbin Liu wrote:
>> Some high level software 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>
> 
> No objection to this patch/series, just a nit and some discussion below, so:
> 
> Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
> 
> 
> [...]
>> +/**
>> + *	netdev_compute_master_upper_features - compute feature from lowers
> 
> nit: I'm slightly annoyed (that's not quite the right word, sorry)
> that we're adding a new function to "compute features" that doesn't
> touch netdev->features, but I can't come up with a better name
> (the best I got was "compute extra features" and it doesn't help).

I'm not the right person to ask a good name, and I'm ok with the current
one, but since the question is pending... what about:

netdev_{compute,update}_offloads_from_lower()

?

As it actually updates (some of) the offloads available to the (upper)
device?

>> + *	@dev: the upper device
>> + *	@update_header: whether to update upper device's header_len/headroom/tailroom
>> + *
>> + *	Recompute the upper device's feature based on all lower devices.
>> + */
>> +void netdev_compute_master_upper_features(struct net_device *dev, bool update_header)
>> +{
> [...]
>> +	netif_set_tso_max_segs(dev, tso_max_segs);
>> +	netif_set_tso_max_size(dev, tso_max_size);
>> +
>> +	netdev_change_features(dev);
> 
> Maybe a dumb idea: I'm wondering if we're doing this from the wrong
> side.
> 
> Right now we have:
> 
> [some device op] -> [this new function] -> netdev_change_features -> __netdev_update_features -> ndo_fix_features
> 
> Would it make more sense to go instead:
> 
> [some device op] -> netdev_change_features -> __netdev_update_features -> ndo_fix_features -> [this new function]
> 
> ?

Uhmmm.... this function touches a few more things beyond dev->*features,
calling it from ndo_fix_features() looks a bit out-of-scope.

/P

Re: [PATCHv6 net-next 1/4] net: add a common function to compute features for upper devices

[Hangbin Liu]

On Tue, Oct 21, 2025 at 10:46:22AM +0200, Paolo Abeni wrote:
> >> + *	netdev_compute_master_upper_features - compute feature from lowers
> > 
> > nit: I'm slightly annoyed (that's not quite the right word, sorry)
> > that we're adding a new function to "compute features" that doesn't
> > touch netdev->features, but I can't come up with a better name
> > (the best I got was "compute extra features" and it doesn't help).
> 
> I'm not the right person to ask a good name, and I'm ok with the current
> one, but since the question is pending... what about:
> 
> netdev_{compute,update}_offloads_from_lower()

The naming is a bit inconsistent.

We originally used netdev_compute_features_from_lowers(), but later changed it
to netdev_compute_master_upper_features() based on Jiri’s suggestion.
Following that pattern, maybe netdev_compute_master_upper_offloads() would be
a suitable name?

If you agree, I can post a new version. If not, we can just leave it as is.

Thanks
Hangbin

Re: [PATCHv6 net-next 1/4] net: add a common function to compute features for upper devices

[Sabrina Dubroca]

2025-10-21, 10:46:22 +0200, Paolo Abeni wrote:
> On 10/20/25 11:10 AM, Sabrina Dubroca wrote:
> > 2025-10-17, 03:41:52 +0000, Hangbin Liu wrote:
> >> Some high level software 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>
> > 
> > No objection to this patch/series, just a nit and some discussion below, so:
> > 
> > Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
> > 
> > 
> > [...]
> >> +/**
> >> + *	netdev_compute_master_upper_features - compute feature from lowers
> > 
> > nit: I'm slightly annoyed (that's not quite the right word, sorry)
> > that we're adding a new function to "compute features" that doesn't
> > touch netdev->features, but I can't come up with a better name
> > (the best I got was "compute extra features" and it doesn't help).
> 
> I'm not the right person to ask a good name, and I'm ok with the current
> one, but since the question is pending... what about:
> 
> netdev_{compute,update}_offloads_from_lower()
> 
> ?
> 
> As it actually updates (some of) the offloads available to the (upper)
> device?

(and the DST_RELEASE flags. at least the tso_max_* kind of fits into "offloads")

I think we can keep the current name. It's more "it kind of bothers
the pedantic part of me" than "annoyed", and we can't find a better
name, so let's ignore the pedantic part. Sorry for the noise.


> >> + *	@dev: the upper device
> >> + *	@update_header: whether to update upper device's header_len/headroom/tailroom
> >> + *
> >> + *	Recompute the upper device's feature based on all lower devices.
> >> + */
> >> +void netdev_compute_master_upper_features(struct net_device *dev, bool update_header)
> >> +{
> > [...]
> >> +	netif_set_tso_max_segs(dev, tso_max_segs);
> >> +	netif_set_tso_max_size(dev, tso_max_size);
> >> +
> >> +	netdev_change_features(dev);
> > 
> > Maybe a dumb idea: I'm wondering if we're doing this from the wrong
> > side.
> > 
> > Right now we have:
> > 
> > [some device op] -> [this new function] -> netdev_change_features -> __netdev_update_features -> ndo_fix_features
> > 
> > Would it make more sense to go instead:
> > 
> > [some device op] -> netdev_change_features -> __netdev_update_features -> ndo_fix_features -> [this new function]
> > 
> > ?
> 
> Uhmmm.... this function touches a few more things beyond dev->*features,
> calling it from ndo_fix_features() looks a bit out-of-scope.

True. And as Hangbin said, it's setting (so a bit more "update" than
"compute", as you wrote above) values whereas ndo_fix_features is just
returning a value.

So if we wanted to have this done by netdev_change_features, we'd
probably need a new ndo, or some kind of flag to tell
__netdev_update_features that this device needs the new function
called. Well, we have netif_is_bridge_master, netif_is_team_master,
netif_is_bond_master. But at this stage we don't know if update_header
should be true/false. So ndo would be cleaner, but a lot
heavier... it's probably not worth all this mess.

-- 
Sabrina

Re: [PATCHv6 net-next 0/4] net: common feature compute for upper interface

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 17 Oct 2025 03:41:51 +0000 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [PATCHv6,net-next,1/4] net: add a common function to compute features for upper devices
    https://git.kernel.org/netdev/net-next/c/28098defc79f
  - [PATCHv6,net-next,2/4] bonding: use common function to compute the features
    https://git.kernel.org/netdev/net-next/c/d4fde269a970
  - [PATCHv6,net-next,3/4] team: use common function to compute the features
    https://git.kernel.org/netdev/net-next/c/745cd46c2a47
  - [PATCHv6,net-next,4/4] net: bridge: use common function to compute the features
    https://git.kernel.org/netdev/net-next/c/0152747a528a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html