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
