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

[Florian Fainelli <f.fainelli@gmail.com>]

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