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

[Stanislav Fomichev <stfomichev@gmail.com>]

On 08/29, Hangbin Liu wrote:
> 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;
>  }

nit: change these to 'return team_port_xxx()' ? Can drop the 'err' as
well, don't think it's needed anymore?