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

[Hangbin Liu <liuhangbin@gmail.com>]

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