* [PATCH net] net: dsa: fix unintended change of bridge interface STP state
@ 2019-02-20 10:32 Russell King
2019-02-20 16:38 ` Vivien Didelot
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Russell King @ 2019-02-20 10:32 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Vivien Didelot; +Cc: David S. Miller, netdev
When a DSA port is added to a bridge and brought up, the resulting STP
state programmed into the hardware depends on the order that these
operations are performed. However, the Linux bridge code believes that
the port is in disabled mode.
If the DSA port is first added to a bridge and then brought up, it will
be in blocking mode. If it is brought up and then added to the bridge,
it will be in disabled mode.
This difference is caused by DSA always setting the STP mode in
dsa_port_enable() whether or not this port is part of a bridge. Since
bridge always sets the STP state when the port is added, brought up or
taken down, it is unnecessary for us to manipulate the STP state.
Apparently, this code was copied from Rocker, and the very next day a
similar fix for Rocker was merged but was not propagated to DSA. See
e47172ab7e41 ("rocker: put port in FORWADING state after leaving bridge")
Fixes: b73adef67765 ("net: dsa: integrate with SWITCHDEV for HW bridging")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
net/dsa/port.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/dsa/port.c b/net/dsa/port.c
index ed0595459df1..ea7efc86b9d7 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -69,7 +69,6 @@ static void dsa_port_set_state_now(struct dsa_port *dp, u8 state)
int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy)
{
- u8 stp_state = dp->bridge_dev ? BR_STATE_BLOCKING : BR_STATE_FORWARDING;
struct dsa_switch *ds = dp->ds;
int port = dp->index;
int err;
@@ -80,7 +79,8 @@ int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy)
return err;
}
- dsa_port_set_state_now(dp, stp_state);
+ if (!dp->bridge_dev)
+ dsa_port_set_state_now(dp, BR_STATE_FORWARDING);
return 0;
}
@@ -90,7 +90,8 @@ void dsa_port_disable(struct dsa_port *dp, struct phy_device *phy)
struct dsa_switch *ds = dp->ds;
int port = dp->index;
- dsa_port_set_state_now(dp, BR_STATE_DISABLED);
+ if (!dp->bridge_dev)
+ dsa_port_set_state_now(dp, BR_STATE_DISABLED);
if (ds->ops->port_disable)
ds->ops->port_disable(ds, port, phy);
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net] net: dsa: fix unintended change of bridge interface STP state
2019-02-20 10:32 [PATCH net] net: dsa: fix unintended change of bridge interface STP state Russell King
@ 2019-02-20 16:38 ` Vivien Didelot
2019-02-20 17:22 ` Florian Fainelli
2019-02-20 19:09 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: Vivien Didelot @ 2019-02-20 16:38 UTC (permalink / raw)
To: Russell King; +Cc: Andrew Lunn, Florian Fainelli, David S. Miller, netdev
On Wed, 20 Feb 2019 10:32:52 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> When a DSA port is added to a bridge and brought up, the resulting STP
> state programmed into the hardware depends on the order that these
> operations are performed. However, the Linux bridge code believes that
> the port is in disabled mode.
>
> If the DSA port is first added to a bridge and then brought up, it will
> be in blocking mode. If it is brought up and then added to the bridge,
> it will be in disabled mode.
>
> This difference is caused by DSA always setting the STP mode in
> dsa_port_enable() whether or not this port is part of a bridge. Since
> bridge always sets the STP state when the port is added, brought up or
> taken down, it is unnecessary for us to manipulate the STP state.
>
> Apparently, this code was copied from Rocker, and the very next day a
> similar fix for Rocker was merged but was not propagated to DSA. See
> e47172ab7e41 ("rocker: put port in FORWADING state after leaving bridge")
>
> Fixes: b73adef67765 ("net: dsa: integrate with SWITCHDEV for HW bridging")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Vivien Didelot <vivien.didelot@gmail.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] net: dsa: fix unintended change of bridge interface STP state
2019-02-20 10:32 [PATCH net] net: dsa: fix unintended change of bridge interface STP state Russell King
2019-02-20 16:38 ` Vivien Didelot
@ 2019-02-20 17:22 ` Florian Fainelli
2019-02-20 17:27 ` Andrew Lunn
2019-02-20 19:09 ` David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2019-02-20 17:22 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Vivien Didelot; +Cc: David S. Miller, netdev
On 2/20/19 2:32 AM, Russell King wrote:
> When a DSA port is added to a bridge and brought up, the resulting STP
> state programmed into the hardware depends on the order that these
> operations are performed. However, the Linux bridge code believes that
> the port is in disabled mode.
>
> If the DSA port is first added to a bridge and then brought up, it will
> be in blocking mode. If it is brought up and then added to the bridge,
> it will be in disabled mode.
>
> This difference is caused by DSA always setting the STP mode in
> dsa_port_enable() whether or not this port is part of a bridge. Since
> bridge always sets the STP state when the port is added, brought up or
> taken down, it is unnecessary for us to manipulate the STP state.
>
> Apparently, this code was copied from Rocker, and the very next day a
> similar fix for Rocker was merged but was not propagated to DSA. See
> e47172ab7e41 ("rocker: put port in FORWADING state after leaving bridge")
>
> Fixes: b73adef67765 ("net: dsa: integrate with SWITCHDEV for HW bridging")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Nice example of cargo cult programming, thanks for fixing this!
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] net: dsa: fix unintended change of bridge interface STP state
2019-02-20 17:22 ` Florian Fainelli
@ 2019-02-20 17:27 ` Andrew Lunn
2019-02-20 18:38 ` Florian Fainelli
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2019-02-20 17:27 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Russell King, Vivien Didelot, David S. Miller, netdev
On Wed, Feb 20, 2019 at 09:22:30AM -0800, Florian Fainelli wrote:
> On 2/20/19 2:32 AM, Russell King wrote:
> > When a DSA port is added to a bridge and brought up, the resulting STP
> > state programmed into the hardware depends on the order that these
> > operations are performed. However, the Linux bridge code believes that
> > the port is in disabled mode.
> >
> > If the DSA port is first added to a bridge and then brought up, it will
> > be in blocking mode. If it is brought up and then added to the bridge,
> > it will be in disabled mode.
> >
> > This difference is caused by DSA always setting the STP mode in
> > dsa_port_enable() whether or not this port is part of a bridge. Since
> > bridge always sets the STP state when the port is added, brought up or
> > taken down, it is unnecessary for us to manipulate the STP state.
> >
> > Apparently, this code was copied from Rocker, and the very next day a
> > similar fix for Rocker was merged but was not propagated to DSA. See
> > e47172ab7e41 ("rocker: put port in FORWADING state after leaving bridge")
> >
> > Fixes: b73adef67765 ("net: dsa: integrate with SWITCHDEV for HW bridging")
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>
> Nice example of cargo cult programming, thanks for fixing this!
Maybe now would be a good time to look at other drivers. Does the
Microsemi Ocelot driver have the same issue?
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] net: dsa: fix unintended change of bridge interface STP state
2019-02-20 17:27 ` Andrew Lunn
@ 2019-02-20 18:38 ` Florian Fainelli
0 siblings, 0 replies; 6+ messages in thread
From: Florian Fainelli @ 2019-02-20 18:38 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Russell King, Vivien Didelot, David S. Miller, netdev
On 2/20/19 9:27 AM, Andrew Lunn wrote:
> On Wed, Feb 20, 2019 at 09:22:30AM -0800, Florian Fainelli wrote:
>> On 2/20/19 2:32 AM, Russell King wrote:
>>> When a DSA port is added to a bridge and brought up, the resulting STP
>>> state programmed into the hardware depends on the order that these
>>> operations are performed. However, the Linux bridge code believes that
>>> the port is in disabled mode.
>>>
>>> If the DSA port is first added to a bridge and then brought up, it will
>>> be in blocking mode. If it is brought up and then added to the bridge,
>>> it will be in disabled mode.
>>>
>>> This difference is caused by DSA always setting the STP mode in
>>> dsa_port_enable() whether or not this port is part of a bridge. Since
>>> bridge always sets the STP state when the port is added, brought up or
>>> taken down, it is unnecessary for us to manipulate the STP state.
>>>
>>> Apparently, this code was copied from Rocker, and the very next day a
>>> similar fix for Rocker was merged but was not propagated to DSA. See
>>> e47172ab7e41 ("rocker: put port in FORWADING state after leaving bridge")
>>>
>>> Fixes: b73adef67765 ("net: dsa: integrate with SWITCHDEV for HW bridging")
>>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>>
>> Nice example of cargo cult programming, thanks for fixing this!
>
> Maybe now would be a good time to look at other drivers. Does the
> Microsemi Ocelot driver have the same issue?
Checked fsl-dpaa2/ethsw and ocelot and neither of those seem to be
affected by this problem.
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: dsa: fix unintended change of bridge interface STP state
2019-02-20 10:32 [PATCH net] net: dsa: fix unintended change of bridge interface STP state Russell King
2019-02-20 16:38 ` Vivien Didelot
2019-02-20 17:22 ` Florian Fainelli
@ 2019-02-20 19:09 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2019-02-20 19:09 UTC (permalink / raw)
To: rmk+kernel; +Cc: andrew, f.fainelli, vivien.didelot, netdev
From: Russell King <rmk+kernel@armlinux.org.uk>
Date: Wed, 20 Feb 2019 10:32:52 +0000
> When a DSA port is added to a bridge and brought up, the resulting STP
> state programmed into the hardware depends on the order that these
> operations are performed. However, the Linux bridge code believes that
> the port is in disabled mode.
>
> If the DSA port is first added to a bridge and then brought up, it will
> be in blocking mode. If it is brought up and then added to the bridge,
> it will be in disabled mode.
>
> This difference is caused by DSA always setting the STP mode in
> dsa_port_enable() whether or not this port is part of a bridge. Since
> bridge always sets the STP state when the port is added, brought up or
> taken down, it is unnecessary for us to manipulate the STP state.
>
> Apparently, this code was copied from Rocker, and the very next day a
> similar fix for Rocker was merged but was not propagated to DSA. See
> e47172ab7e41 ("rocker: put port in FORWADING state after leaving bridge")
>
> Fixes: b73adef67765 ("net: dsa: integrate with SWITCHDEV for HW bridging")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Applied, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-20 19:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-20 10:32 [PATCH net] net: dsa: fix unintended change of bridge interface STP state Russell King
2019-02-20 16:38 ` Vivien Didelot
2019-02-20 17:22 ` Florian Fainelli
2019-02-20 17:27 ` Andrew Lunn
2019-02-20 18:38 ` Florian Fainelli
2019-02-20 19:09 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).