| Message ID | 20210731191023.1329446-1-dqfext@gmail.com |
|---|---|
| Headers | show |
| Series | mt7530 software fallback bridging fix | expand |
On Sun, Aug 01, 2021 at 03:10:19AM +0800, DENG Qingfang wrote: > Consider the following bridge configuration, where bond0 is not > offloaded: > > +-- br0 --+ > / / | \ > / / | \ > / | | bond0 > / | | / \ > swp0 swp1 swp2 swp3 swp4 > . . . > . . . > A B C > > Address learning is enabled on offloaded ports (swp0~2) and the CPU > port, so when client A sends a packet to C, the following will happen: > > 1. The switch learns that client A can be reached at swp0. > 2. The switch probably already knows that client C can be reached at the > CPU port, so it forwards the packet to the CPU. > 3. The bridge core knows client C can be reached at bond0, so it > forwards the packet back to the switch. > 4. The switch learns that client A can be reached at the CPU port. > 5. The switch forwards the packet to either swp3 or swp4, according to > the packet's tag. > > That makes client A's MAC address flap between swp0 and the CPU port. If > client B sends a packet to A, it is possible that the packet is > forwarded to the CPU. With offload_fwd_mark = 1, the bridge core won't > forward it back to the switch, resulting in packet loss. > > As we have the assisted_learning_on_cpu_port in DSA core now, enable > that and disable hardware learning on the CPU port. > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > --- Reviewed-by: Vladimir Oltean <oltean@gmail.com>
On Sun, Aug 01, 2021 at 03:10:21AM +0800, DENG Qingfang wrote: > As filter ID 1 is used, set STP state also on it. > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > --- > drivers/net/dsa/mt7530.c | 3 ++- > drivers/net/dsa/mt7530.h | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > index 3876e265f844..38d6ce37d692 100644 > --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -1147,7 +1147,8 @@ mt7530_stp_state_set(struct dsa_switch *ds, int port, u8 state) > break; > } > > - mt7530_rmw(priv, MT7530_SSP_P(port), FID_PST_MASK, stp_state); > + mt7530_rmw(priv, MT7530_SSP_P(port), FID_PST_MASK, > + FID_PST(stp_state)); > } > > static int > diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h > index a308886fdebc..294ff1cbd9e0 100644 > --- a/drivers/net/dsa/mt7530.h > +++ b/drivers/net/dsa/mt7530.h > @@ -181,7 +181,7 @@ enum mt7530_vlan_egress_attr { > > /* Register for port STP state control */ > #define MT7530_SSP_P(x) (0x2000 + ((x) * 0x100)) > -#define FID_PST(x) ((x) & 0x3) Shouldn't these macros have _two_ arguments, the FID and the port state? > +#define FID_PST(x) (((x) & 0x3) * 0x5) "* 5": explanation? > #define FID_PST_MASK FID_PST(0x3) > > enum mt7530_stp_state { > -- > 2.25.1 > I don't exactly understand how this patch works, sorry. Are you altering port state only on bridged ports, or also on standalone ports after this patch? Are standalone ports in the proper STP state (FORWARDING)?
On Mon, Aug 02, 2021 at 04:43:36PM +0300, Vladimir Oltean wrote: > On Sun, Aug 01, 2021 at 03:10:21AM +0800, DENG Qingfang wrote: > > --- a/drivers/net/dsa/mt7530.h > > +++ b/drivers/net/dsa/mt7530.h > > @@ -181,7 +181,7 @@ enum mt7530_vlan_egress_attr { > > > > /* Register for port STP state control */ > > #define MT7530_SSP_P(x) (0x2000 + ((x) * 0x100)) > > -#define FID_PST(x) ((x) & 0x3) > > Shouldn't these macros have _two_ arguments, the FID and the port state? > > > +#define FID_PST(x) (((x) & 0x3) * 0x5) > > "* 5": explanation? > > > #define FID_PST_MASK FID_PST(0x3) > > > > enum mt7530_stp_state { > > -- > > 2.25.1 > > > > I don't exactly understand how this patch works, sorry. > Are you altering port state only on bridged ports, or also on standalone > ports after this patch? Are standalone ports in the proper STP state > (FORWARDING)? The current code only sets FID 0's STP state. This patch sets both 0's and 1's states. The *5 part is binary magic. [1:0] is FID 0's state, [3:2] is FID 1's state and so on. Since 5 == 4'b0101, the value in [1:0] is copied to [3:2] after the multiplication. Perhaps I should only change FID 1's state.
On Mon, Aug 02, 2021 at 11:31:29PM +0800, DENG Qingfang wrote: > On Mon, Aug 02, 2021 at 04:43:36PM +0300, Vladimir Oltean wrote: > > On Sun, Aug 01, 2021 at 03:10:21AM +0800, DENG Qingfang wrote: > > > --- a/drivers/net/dsa/mt7530.h > > > +++ b/drivers/net/dsa/mt7530.h > > > @@ -181,7 +181,7 @@ enum mt7530_vlan_egress_attr { > > > > > > /* Register for port STP state control */ > > > #define MT7530_SSP_P(x) (0x2000 + ((x) * 0x100)) > > > -#define FID_PST(x) ((x) & 0x3) > > > > Shouldn't these macros have _two_ arguments, the FID and the port state? > > > > > +#define FID_PST(x) (((x) & 0x3) * 0x5) > > > > "* 5": explanation? > > > > > #define FID_PST_MASK FID_PST(0x3) > > > > > > enum mt7530_stp_state { > > > -- > > > 2.25.1 > > > > > > > I don't exactly understand how this patch works, sorry. > > Are you altering port state only on bridged ports, or also on standalone > > ports after this patch? Are standalone ports in the proper STP state > > (FORWARDING)? > > The current code only sets FID 0's STP state. This patch sets both 0's and > 1's states. > > The *5 part is binary magic. [1:0] is FID 0's state, [3:2] is FID 1's state > and so on. Since 5 == 4'b0101, the value in [1:0] is copied to [3:2] after > the multiplication. > > Perhaps I should only change FID 1's state. Keep the patches dumb for us mortals please. If you only change FID 1's state, I am concerned that the driver no longer initializes FID 0's port state, and might leave that to the default set by other pre-kernel initialization stage (bootloader?). So even if you might assume that standalone ports are FORWARDING, they might not be.
On Mon, Aug 02, 2021 at 06:42:26PM +0300, Vladimir Oltean wrote: > On Mon, Aug 02, 2021 at 11:31:29PM +0800, DENG Qingfang wrote: > > The current code only sets FID 0's STP state. This patch sets both 0's and > > 1's states. > > > > The *5 part is binary magic. [1:0] is FID 0's state, [3:2] is FID 1's state > > and so on. Since 5 == 4'b0101, the value in [1:0] is copied to [3:2] after > > the multiplication. > > > > Perhaps I should only change FID 1's state. > > Keep the patches dumb for us mortals please. > If you only change FID 1's state, I am concerned that the driver no > longer initializes FID 0's port state, and might leave that to the > default set by other pre-kernel initialization stage (bootloader?). > So even if you might assume that standalone ports are FORWARDING, they > might not be. The default value is forwarding, and the switch is reset by the driver so any pre-kernel initialization stage is no more.
On Mon, Aug 02, 2021 at 11:58:10PM +0800, DENG Qingfang wrote: > On Mon, Aug 02, 2021 at 06:42:26PM +0300, Vladimir Oltean wrote: > > On Mon, Aug 02, 2021 at 11:31:29PM +0800, DENG Qingfang wrote: > > > The current code only sets FID 0's STP state. This patch sets both 0's and > > > 1's states. > > > > > > The *5 part is binary magic. [1:0] is FID 0's state, [3:2] is FID 1's state > > > and so on. Since 5 == 4'b0101, the value in [1:0] is copied to [3:2] after > > > the multiplication. > > > > > > Perhaps I should only change FID 1's state. > > > > Keep the patches dumb for us mortals please. > > If you only change FID 1's state, I am concerned that the driver no > > longer initializes FID 0's port state, and might leave that to the > > default set by other pre-kernel initialization stage (bootloader?). > > So even if you might assume that standalone ports are FORWARDING, they > > might not be. > > The default value is forwarding, and the switch is reset by the driver > so any pre-kernel initialization stage is no more. So then change the port STP state only for FID 1 and resend. Any other reason why this patch series is marked RFC? It looked okay to me otherwise.
On Tue, Aug 03, 2021 at 12:00:06AM +0300, Vladimir Oltean wrote: > > So then change the port STP state only for FID 1 and resend. Any other > reason why this patch series is marked RFC? It looked okay to me otherwise. Okay, will resend with that change and without RFC. By the way, if I were to implement .port_fast_age, should I only flush dynamically learned FDB entries? What about MDB entries?
On Tue, Aug 03, 2021 at 04:23:16PM +0800, DENG Qingfang wrote: > On Tue, Aug 03, 2021 at 12:00:06AM +0300, Vladimir Oltean wrote: > > > > So then change the port STP state only for FID 1 and resend. Any other > > reason why this patch series is marked RFC? It looked okay to me otherwise. > > Okay, will resend with that change and without RFC. > > By the way, if I were to implement .port_fast_age, should I only flush > dynamically learned FDB entries? What about MDB entries? Yes, only dynamically learned entries. That should also answer the question about MDB entries, since a multicast address should never be a source MAC address so they should never be dynamically learned. The bridge should handle the deletion of static entries when needed.
DSA core has gained software fallback support since commit 2f5dc00f7a3e, but it does not work properly on mt7530. This patch series fixes the issues. DENG Qingfang (4): net: dsa: mt7530: enable assisted learning on CPU port net: dsa: mt7530: use independent VLAN learning on VLAN-unaware bridges net: dsa: mt7530: set STP state also on filter ID 1 Revert "mt7530 mt7530_fdb_write only set ivl bit vid larger than 1" drivers/net/dsa/mt7530.c | 86 +++++++++++++++++++++++++++------------- drivers/net/dsa/mt7530.h | 6 ++- 2 files changed, 63 insertions(+), 29 deletions(-)