Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Tue, Feb 19, 2019 at 11:16:12AM -0500, Vivien Didelot wrote:
> Hi Russell,
> 
> On Sun, 17 Feb 2019 16:32:34 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> > +static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
> > +				       unsigned long flags)
> > +{
> > +	struct mv88e6xxx_chip *chip = ds->priv;
> > +	bool unicast, multicast;
> > +	int ret = -EOPNOTSUPP;
> > +
> > +	unicast = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) ||
> > +		flags & BR_FLOOD;
> > +	multicast = flags & BR_MCAST_FLOOD;
> > +
> > +	mutex_lock(&chip->reg_lock);
> > +	if (chip->info->ops->port_set_egress_floods)
> > +		ret = chip->info->ops->port_set_egress_floods(chip, port,
> > +							      unicast,
> > +							      multicast);
> > +	mutex_unlock(&chip->reg_lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > +{
> > +	struct mv88e6xxx_chip *chip = ds->priv;
> > +	unsigned long support = 0;
> > +
> > +	if (chip->info->ops->port_set_egress_floods)
> > +		support |= BR_FLOOD | BR_MCAST_FLOOD;
> > +
> > +	return support;
> > +}
> 
> I think that it isn't necessary to propagate the notion of bridge flags down
> to the DSA drivers. It might be just enough to add:
> 
>     port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
> 
> to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> if the targeted driver has ds->ops->port_set_egress_flood. What do you think?

I've just changed my last patch to set these modes from
dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing,
I notice this on the ZII rev B board:

At boot (without anything connected to any of the switch ports):

br0: port 1(lan0) entered blocking state
br0: port 1(lan0) entered disabled state
device lan0 entered promiscuous mode
device eth1 entered promiscuous mode
br0: port 2(lan1) entered blocking state
br0: port 2(lan1) entered disabled state
device lan1 entered promiscuous mode
...

I then removed lan0 from the bridge:

device lan0 left promiscuous mode
br0: port 1(lan0) entered disabled state

and then added it back:

br0: port 1(lan0) entered blocking state
br0: port 1(lan0) entered disabled state
device lan0 entered promiscuous mode

Now, you'd expect lan0 and lan1 to be configured the same at this
point, and the same as it was before lan0 was removed from the bridge?
lan0 is port 0, lan1 is port 1 on this switch - and the register debug
says:

    GLOBAL GLOBAL2 SERDES     0    1    2    3    4    5    6
 0:  c800       0    1140  500f 500f 500f 500f 500f 4e07 4d04
...
 4:  40a8     258     1e0   43c  43d  43d   7c  430  53f 373f

Note that port 0 is in disabled state, but port 1 and 2 are in
blocking state... but wait, the kernel printed a message saying it was
in disabled state!

If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as
expected, and then returns to 0x43c.

It looks like DSA isn't always in sync with bridge as per port state.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up