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

[PATCHv4 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.

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)

v3:
a) fix hw_enc_features asign order (Sabrina Dubroca)
b) set virtual dev feature defination 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 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

 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                  | 95 +++++++++++++++++++++++++++++++
 6 files changed, 127 insertions(+), 191 deletions(-)

-- 
2.50.1

[PATCHv4 net-next 1/4] 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/netdev_features.h | 18 +++++++
 include/linux/netdevice.h       |  1 +
 net/core/dev.c                  | 95 +++++++++++++++++++++++++++++++++
 3 files changed, 114 insertions(+)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 7a01c518e573..f3fe2d59ea96 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 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)
+
 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..8e28fee247f5 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_features_from_lowers(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 a64cef2c537e..54f0e792fbd2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -12616,6 +12616,101 @@ 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
+ *	@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_features_from_lowers(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 = 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 short max_header_len = ETH_HLEN;
+	unsigned int tso_max_size = TSO_MAX_SIZE;
+	u16 tso_max_segs = TSO_MAX_SEGS;
+	struct net_device *lower_dev;
+	unsigned short max_headroom;
+	unsigned short max_tailroom;
+	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);
+
+		enc_features = netdev_increment_features(enc_features,
+							 lower_dev->hw_enc_features,
+							 VIRTUAL_DEV_ENC_FEATURES);
+
+#ifdef CONFIG_XFRM_OFFLOAD
+		xfrm_features = netdev_increment_features(xfrm_features,
+							  lower_dev->hw_enc_features,
+							  VIRTUAL_DEV_XFRM_FEATURES);
+#endif
+
+		mpls_features = netdev_increment_features(mpls_features,
+							  lower_dev->mpls_features,
+							  VIRTUAL_DEV_MPLS_FEATURES);
+
+		dst_release_flag &= lower_dev->priv_flags;
+
+		if (update_header) {
+			max_header_len = max_t(unsigned short, max_header_len,
+					lower_dev->hard_header_len);
+			max_headroom = max_t(unsigned short, max_headroom,
+					lower_dev->needed_headroom);
+			max_tailroom = max_t(unsigned short, 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;
+#ifdef CONFIG_XFRM_OFFLOAD
+	dev->hw_enc_features |= xfrm_features;
+#endif
+	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_features_from_lowers);
+
 static struct hlist_head * __net_init netdev_create_hash(void)
 {
 	int i;
-- 
2.50.1

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

[Jiri Pirko]

Tue, Oct 14, 2025 at 10:02:14AM +0200, liuhangbin@gmail.com wrote:

[..]


>+#define VIRTUAL_DEV_VLAN_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \


I don't like the "virtual" naming. In the past, we always tried to avoid
that for lower-upper devices like bond/team/bridge/others. Soft-device
was the used term. Please let the "virtual" term for vitrualization,
would that be possible?

How about "master_upper"? This is already widely used to refer to
bond/team/bridge/other master soft devices.

MASTER_UPPER_DEV_VLAN_FEATURES?

[..]


>+void netdev_compute_features_from_lowers(struct net_device *dev, bool update_header)

netdev_compute_master_upper_features?

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

[Hangbin Liu]

Hi Jiri,
On Tue, Oct 14, 2025 at 11:40:12AM +0200, Jiri Pirko wrote:
> >+#define VIRTUAL_DEV_VLAN_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \
> 
> I don't like the "virtual" naming. In the past, we always tried to avoid
> that for lower-upper devices like bond/team/bridge/others. Soft-device
> was the used term. Please let the "virtual" term for vitrualization,
> would that be possible?

Sure
> 
> How about "master_upper"? This is already widely used to refer to
> bond/team/bridge/other master soft devices.
> 
> MASTER_UPPER_DEV_VLAN_FEATURES?

I'm not sure if we should avoid using "master" now. Maybe just UPPER_DEV_VLAN_FEATURES?

> [..]
> 
> 
> >+void netdev_compute_features_from_lowers(struct net_device *dev, bool update_header)
> 
> netdev_compute_master_upper_features?

netdev_compute_upper_features?

Thanks
Hangbin

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

[Jiri Pirko]

Wed, Oct 15, 2025 at 03:25:59AM +0200, liuhangbin@gmail.com wrote:
>Hi Jiri,
>On Tue, Oct 14, 2025 at 11:40:12AM +0200, Jiri Pirko wrote:
>> >+#define VIRTUAL_DEV_VLAN_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \
>> 
>> I don't like the "virtual" naming. In the past, we always tried to avoid
>> that for lower-upper devices like bond/team/bridge/others. Soft-device
>> was the used term. Please let the "virtual" term for vitrualization,
>> would that be possible?
>
>Sure
>> 
>> How about "master_upper"? This is already widely used to refer to
>> bond/team/bridge/other master soft devices.
>> 
>> MASTER_UPPER_DEV_VLAN_FEATURES?
>
>I'm not sure if we should avoid using "master" now. Maybe just UPPER_DEV_VLAN_FEATURES?

Why? We have "master_upper" to point exactly at this kind of device.


>
>> [..]
>> 
>> 
>> >+void netdev_compute_features_from_lowers(struct net_device *dev, bool update_header)
>> 
>> netdev_compute_master_upper_features?
>
>netdev_compute_upper_features?
>
>Thanks
>Hangbin

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

[Hangbin Liu]

On Thu, Oct 16, 2025 at 01:27:00PM +0200, Jiri Pirko wrote:
> >> How about "master_upper"? This is already widely used to refer to
> >> bond/team/bridge/other master soft devices.
> >> 
> >> MASTER_UPPER_DEV_VLAN_FEATURES?
> >
> >I'm not sure if we should avoid using "master" now. Maybe just UPPER_DEV_VLAN_FEATURES?
> 
> Why? We have "master_upper" to point exactly at this kind of device.

I mean try not use "master/slave" words. I'm OK to use UPPER_DEV_* prefix.

I will update the name if there is a next version.

Thanks
Hangbin

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

[Jiri Pirko]

Thu, Oct 16, 2025 at 02:38:15PM +0200, liuhangbin@gmail.com wrote:
>On Thu, Oct 16, 2025 at 01:27:00PM +0200, Jiri Pirko wrote:
>> >> How about "master_upper"? This is already widely used to refer to
>> >> bond/team/bridge/other master soft devices.
>> >> 
>> >> MASTER_UPPER_DEV_VLAN_FEATURES?
>> >
>> >I'm not sure if we should avoid using "master" now. Maybe just UPPER_DEV_VLAN_FEATURES?
>> 
>> Why? We have "master_upper" to point exactly at this kind of device.
>
>I mean try not use "master/slave" words. I'm OK to use UPPER_DEV_* prefix.

If you don't want to use that, change the existing code. But when the
existing code uses that, new code should be consistent with it.


>
>I will update the name if there is a next version.
>
>Thanks
>Hangbin

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

[Hangbin Liu]

On Thu, Oct 16, 2025 at 03:24:59PM +0200, Jiri Pirko wrote:
> Thu, Oct 16, 2025 at 02:38:15PM +0200, liuhangbin@gmail.com wrote:
> >On Thu, Oct 16, 2025 at 01:27:00PM +0200, Jiri Pirko wrote:
> >> >> How about "master_upper"? This is already widely used to refer to
> >> >> bond/team/bridge/other master soft devices.
> >> >> 
> >> >> MASTER_UPPER_DEV_VLAN_FEATURES?
> >> >
> >> >I'm not sure if we should avoid using "master" now. Maybe just UPPER_DEV_VLAN_FEATURES?
> >> 
> >> Why? We have "master_upper" to point exactly at this kind of device.
> >
> >I mean try not use "master/slave" words. I'm OK to use UPPER_DEV_* prefix.
> 
> If you don't want to use that, change the existing code. But when the
> existing code uses that, new code should be consistent with it.

OK, I will update the name.

Thanks
Hangbin

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

[Simon Horman]

On Tue, Oct 14, 2025 at 08:02:14AM +0000, Hangbin Liu wrote:

...

> diff --git a/net/core/dev.c b/net/core/dev.c
> index a64cef2c537e..54f0e792fbd2 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -12616,6 +12616,101 @@ 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
> + *	@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_features_from_lowers(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 = VIRTUAL_DEV_GSO_PARTIAL_FEATURES;
> +#ifdef CONFIG_XFRM_OFFLOAD
> +	netdev_features_t xfrm_features = VIRTUAL_DEV_XFRM_FEATURES;
> +#endif

Hi Hangbin,

It would be nice to avoid the #ifdefs in this function.

Could xfrm_features be declared unconditoinally.
And then used behind if(IS_ENABLED(CONFIG_XFRM_OFFLOAD)) conditions?
This would increase compile coverage (and readability IMHO).

> +	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 short max_header_len = ETH_HLEN;
> +	unsigned int tso_max_size = TSO_MAX_SIZE;
> +	u16 tso_max_segs = TSO_MAX_SEGS;
> +	struct net_device *lower_dev;
> +	unsigned short max_headroom;
> +	unsigned short max_tailroom;
> +	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);
> +
> +		enc_features = netdev_increment_features(enc_features,
> +							 lower_dev->hw_enc_features,
> +							 VIRTUAL_DEV_ENC_FEATURES);
> +
> +#ifdef CONFIG_XFRM_OFFLOAD
> +		xfrm_features = netdev_increment_features(xfrm_features,
> +							  lower_dev->hw_enc_features,
> +							  VIRTUAL_DEV_XFRM_FEATURES);
> +#endif
> +
> +		mpls_features = netdev_increment_features(mpls_features,
> +							  lower_dev->mpls_features,
> +							  VIRTUAL_DEV_MPLS_FEATURES);
> +
> +		dst_release_flag &= lower_dev->priv_flags;
> +
> +		if (update_header) {
> +			max_header_len = max_t(unsigned short, max_header_len,
> +					lower_dev->hard_header_len);

Both the type of max_header_len and .hard_header_len is unsigned short.
So I think max() can be used here instead of max_t(). Likewise for the
following two lines.

> +			max_headroom = max_t(unsigned short, max_headroom,
> +					lower_dev->needed_headroom);

Max Headroom [1] is used uninitialised the first time we reach here.
Likewise for max_tailroom below.

[1] https://en.wikipedia.org/wiki/Max_Headroom

Flagged by Smatch.

> +			max_tailroom = max_t(unsigned short, 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;
> +#ifdef CONFIG_XFRM_OFFLOAD
> +	dev->hw_enc_features |= xfrm_features;
> +#endif
> +	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;

Also, maybe it can't happen in practice. But I think that max_headroom and
max_tailroom will may be used uninitialised here if the previous
'update_header' condition is never reached/met.

Also flagged by Smatch.

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

...

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

[Hangbin Liu]

Hi Simon,

Thanks for the comments, I will fix all of them.

Regards
Hangbin
On Tue, Oct 14, 2025 at 03:02:23PM +0100, Simon Horman wrote:
> On Tue, Oct 14, 2025 at 08:02:14AM +0000, Hangbin Liu wrote:
> 
> ...
> 
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index a64cef2c537e..54f0e792fbd2 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -12616,6 +12616,101 @@ 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
> > + *	@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_features_from_lowers(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 = VIRTUAL_DEV_GSO_PARTIAL_FEATURES;
> > +#ifdef CONFIG_XFRM_OFFLOAD
> > +	netdev_features_t xfrm_features = VIRTUAL_DEV_XFRM_FEATURES;
> > +#endif
> 
> Hi Hangbin,
> 
> It would be nice to avoid the #ifdefs in this function.
> 
> Could xfrm_features be declared unconditoinally.
> And then used behind if(IS_ENABLED(CONFIG_XFRM_OFFLOAD)) conditions?
> This would increase compile coverage (and readability IMHO).
> 
> > +	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 short max_header_len = ETH_HLEN;
> > +	unsigned int tso_max_size = TSO_MAX_SIZE;
> > +	u16 tso_max_segs = TSO_MAX_SEGS;
> > +	struct net_device *lower_dev;
> > +	unsigned short max_headroom;
> > +	unsigned short max_tailroom;
> > +	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);
> > +
> > +		enc_features = netdev_increment_features(enc_features,
> > +							 lower_dev->hw_enc_features,
> > +							 VIRTUAL_DEV_ENC_FEATURES);
> > +
> > +#ifdef CONFIG_XFRM_OFFLOAD
> > +		xfrm_features = netdev_increment_features(xfrm_features,
> > +							  lower_dev->hw_enc_features,
> > +							  VIRTUAL_DEV_XFRM_FEATURES);
> > +#endif
> > +
> > +		mpls_features = netdev_increment_features(mpls_features,
> > +							  lower_dev->mpls_features,
> > +							  VIRTUAL_DEV_MPLS_FEATURES);
> > +
> > +		dst_release_flag &= lower_dev->priv_flags;
> > +
> > +		if (update_header) {
> > +			max_header_len = max_t(unsigned short, max_header_len,
> > +					lower_dev->hard_header_len);
> 
> Both the type of max_header_len and .hard_header_len is unsigned short.
> So I think max() can be used here instead of max_t(). Likewise for the
> following two lines.
> 
> > +			max_headroom = max_t(unsigned short, max_headroom,
> > +					lower_dev->needed_headroom);
> 
> Max Headroom [1] is used uninitialised the first time we reach here.
> Likewise for max_tailroom below.
> 
> [1] https://en.wikipedia.org/wiki/Max_Headroom
> 
> Flagged by Smatch.
> 
> > +			max_tailroom = max_t(unsigned short, 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;
> > +#ifdef CONFIG_XFRM_OFFLOAD
> > +	dev->hw_enc_features |= xfrm_features;
> > +#endif
> > +	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;
> 
> Also, maybe it can't happen in practice. But I think that max_headroom and
> max_tailroom will may be used uninitialised here if the previous
> 'update_header' condition is never reached/met.
> 
> Also flagged by Smatch.
> 
> > +	}
> > +
> > +	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);
> > +
> 
> ...

[PATCHv4 net-next 2/4] 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 4da619210c1f..3b78984e5912 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_features_from_lowers(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_features_from_lowers(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_features_from_lowers(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 = 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

[PATCHv4 net-next 3/4] 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 | 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..03df6a06e0b8 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, 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_features_from_lowers(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 = VIRTUAL_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_features_from_lowers(port->team->dev, true);
 			port->team->notifier_ctx = false;
 		}
 		break;
-- 
2.50.1

[PATCHv4 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_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.

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