Re: [PATCHv3 net 2/2] team: reset team's flags when down link is P2P device

[Paolo Abeni <pabeni@redhat.com>]

On Thu, 2023-07-20 at 17:46 +0800, Hangbin Liu wrote:
> On Thu, Jul 20, 2023 at 10:29:19AM +0200, Paolo Abeni wrote:
> > > +static void team_ether_setup(struct net_device *dev)
> > > +{
> > > +	unsigned int flags = dev->flags & IFF_UP;
> > > +
> > > +	ether_setup(dev);
> > > +	dev->flags |= flags;
> > > +	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
> > 
> > I think we can't do the above. e.g. ether_setup() sets dev->mtu to
> > ethernet default, while prior to this patch dev inherited mtu from the
> > slaved device. The change may affect the user-space in bad ways.
> 
> Hi Paolo,
> 
> I don't see the reason why we should inherited a none ethernet dev's mtu
> to an ethernet dev (i.e. add a none ethernet dev to team, then delete it and
> re-add a ethernet dev to team). I think the dev type has changed, so the
> mtu should also be updated.
> 
> BTW, after adding the port, team will also set port's mtu to team's mtu.

Let suppose the user has set the lower dev MTU to some N (< 1500) for
whatever reason (e.g. lower is a vxlan tunnel). After this change, team
will use mtu = 1500 breaking the connectivity in such scenario/

> > I think we just need an 'else' branch in the point2point check above,
> > restoring the bcast/mcast flags as needed.
> 
> Reset the flags is not enough. All the dev header_ops, type, etc are
> all need to be reset back, that's why we need to call ether_setup().

As far as I can see team_setup_by_port() takes care of that, inheriting
such infor from the current slave. What I mean is something alike the
following.

Cheers,

Paolo

---
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 555b0b1e9a78..17c8056adbe9 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2135,6 +2135,15 @@ static void team_setup_by_port(struct net_device *dev,
 	dev->mtu = port_dev->mtu;
 	memcpy(dev->broadcast, port_dev->broadcast, port_dev->addr_len);
 	eth_hw_addr_inherit(dev, port_dev);
+
+	if (port_dev->flags & IFF_POINTOPOINT) {
+		dev->flags &= ~(IFF_BROADCAST | IFF_MULTICAST);
+		dev->flags |= (IFF_POINTOPOINT | IFF_NOARP);
+	} else if ((port_dev->flags & (IFF_BROADCAST | IFF_MULTICAST)) ==
+		   (IFF_BROADCAST | IFF_MULTICAST)) {
+		dev->flags |= IFF_BROADCAST | IFF_MULTICAST;
+		dev->flags &= ~(IFF_POINTOPOINT | IFF_NOARP)
+	}
 }
 
 static int team_dev_type_check_change(struct net_device *dev,