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

On Fri, Aug 29, 2025 at 09:54:26AM +0000, Hangbin Liu wrote:
> 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 <[email protected]>
> Signed-off-by: Hangbin Liu <[email protected]>
> ---
>  include/linux/netdevice.h | 19 ++++++++++
>  net/core/dev.c            | 79 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 98 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..fcad2a9f6b65 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -12577,6 +12577,85 @@ 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;
                                       ^ double space (in other places as well)

> +#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_hard_header_len = ETH_HLEN;

hard_header_len is not really a feature, so does not sound like it
belongs here. I'm pretty sure it's not needed at all.

It was added to the bond driver in 2006 by commit 54ef31371407 ("[PATCH]
bonding: Handle large hard_header_len") citing panics with gianfar on
xmit. In 2009 commit 93c1285c5d92 ("gianfar: reallocate skb when
headroom is not enough for fcb") fixed the gianfar driver to stop
assuming that it has enough room to push its custom header. Further,
commit bee9e58c9e98 ("gianfar:don't add FCB length to hard_header_len")
from 2012 fixed this driver to use needed_headroom instead of
hard_header_len.

The team driver is also adjusting hard_header_len according to the lower
devices, but it most likely copied it from the bond driver. On the other
hand, the bridge driver does not mess with hard_header_len and no
problems were reported there (that I know of).

Might be a good idea to remove this hard_header_len logic from bond and
team and instead set their needed_headroom according to the lower device
with the highest needed_headroom. Paolo added similar logic in bridge
and ovs but the use case is a bit different there.

> +	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;
> +		if (lower_dev->hard_header_len > max_hard_header_len)
> +			max_hard_header_len = lower_dev->hard_header_len;
> +
> +		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->hard_header_len = max_hard_header_len;
> +	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;
> +	netif_set_tso_max_segs(dev, tso_max_segs);
> +	netif_set_tso_max_size(dev, tso_max_size);
> +
> +	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;
> +
> +	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
>