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 03:53:50PM -0800, Florian Fainelli wrote:
> On 2/19/19 3:34 PM, Russell King - ARM Linux admin wrote:
> > On Tue, Feb 19, 2019 at 05:00:59PM +0000, Russell King - ARM Linux admin wrote:
> >> 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.
> > 
> > Okay, the problem is what we do when we up the port.
> > 
> > When the port is added to the bridge device, and it's down, the bridge
> > code sets the STP state to "disabled".
> > 
> > Then when we up the interface, dsa_slave_open() calls dsa_port_enable(),
> > which then decides to change the STP state on its own without reference
> > to the state assigned by net/bridge:
> > 
> > int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy)
> > {
> >         u8 stp_state = dp->bridge_dev ? BR_STATE_BLOCKING : BR_STATE_FORWARDING;
> > ...
> >         dsa_port_set_state_now(dp, stp_state);
> > ...
> > }
> > 
> > I can understand setting the state to BR_STATE_FORWARDING for
> > stand-alone ports, but why for bridged ports when the bridge code has
> > already taken care of configuring the STP state of the port?
> 
> There was no reason for doing that in commit
> b73adef67765b72f2a0d01ef15aff9d784dc85da ("net: dsa: integrate with
> SWITCHDEV for HW bridging") other than copying what rocker had done
> (which served as model back then), and which got changed the next day in
> rocker with: e47172ab7e4176883077b454286bbd5b87b5f488 ("rocker: put port
> in FORWADING state after leaving bridge")
> 
> Good catch!

As mentioned on IRC, I'll send a patch for this tomorrow for the net
tree once I've untangled it from the floods work.

-- 
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