| Message ID | 20230330083408.63136-2-clement.leger@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | net: dsa: rzn1-a5psw: disabled learning for standalone ports and fix STP support | expand |
Le Thu, 30 Mar 2023 10:34:07 +0200, Clément Léger <clement.leger@bootlin.com> a écrit : > Current there were actually two problems which made the STP support non > working. First, the BPDU were disabled on the CPU port which means BDPUs > were actually dropped internally to the switch. TO fix that, simply enable > BPDUs at management port level. Then, the a5psw_port_stp_set_state() > should have actually received BPDU while in LEARNING mode which was not > the case. Additionally, the BLOCKEN bit does not actually forbid > sending forwarded frames from that port. To fix this, add > a5psw_port_tx_enable() function which allows to disable TX. However, while > its name suggest that TX is totally disabled, it is not and can still > allows to send BPDUs even if disabled. This can be done by using forced > forwarding with the switch tagging mecanism but keeping "filtering" > disabled (which is already the case). Which these fixes, STP support is now > functional. > > Signed-off-by: Clément Léger <clement.leger@bootlin.com> > --- > drivers/net/dsa/rzn1_a5psw.c | 53 +++++++++++++++++++++++++++++------- > drivers/net/dsa/rzn1_a5psw.h | 4 ++- > 2 files changed, 46 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c > index 919027cf2012..bbc1424ed416 100644 > --- a/drivers/net/dsa/rzn1_a5psw.c > +++ b/drivers/net/dsa/rzn1_a5psw.c > @@ -120,6 +120,22 @@ static void a5psw_port_mgmtfwd_set(struct a5psw *a5psw, int port, bool enable) > a5psw_port_pattern_set(a5psw, port, A5PSW_PATTERN_MGMTFWD, enable); > } > > +static void a5psw_port_tx_enable(struct a5psw *a5psw, int port, bool enable) > +{ > + u32 mask = A5PSW_PORT_ENA_TX(port); > + u32 reg = enable ? mask : 0; > + > + /* Even though the port TX is disabled through TXENA bit in the > + * PORT_ENA register it can still send BPDUs. This depends on the tag > + * configuration added when sending packets from the CPU port to the > + * switch port. Indeed, when using forced forwarding without filtering, > + * even disabled port will be able to send packets that are tagged. This > + * allows to implement STP support when ports are in a state were > + * forwarding traffic should be stopped but BPDUs should still be sent. > + */ > + a5psw_reg_rmw(a5psw, A5PSW_CMD_CFG(port), mask, reg); > +} > + > static void a5psw_port_enable_set(struct a5psw *a5psw, int port, bool enable) > { > u32 port_ena = 0; > @@ -292,6 +308,18 @@ static int a5psw_set_ageing_time(struct dsa_switch *ds, unsigned int msecs) > return 0; > } > > +static void a5psw_port_learning_set(struct a5psw *a5psw, int port, > + bool learning, bool blocked) > +{ > + u32 mask = A5PSW_INPUT_LEARN_DIS(port) | A5PSW_INPUT_LEARN_BLOCK(port); > + u32 reg = 0; > + > + reg |= !learning ? A5PSW_INPUT_LEARN_DIS(port) : 0; > + reg |= blocked ? A5PSW_INPUT_LEARN_BLOCK(port) : 0; > + > + a5psw_reg_rmw(a5psw, A5PSW_INPUT_LEARN, mask, reg); > +} > + > static void a5psw_flooding_set_resolution(struct a5psw *a5psw, int port, > bool set) > { > @@ -344,28 +372,33 @@ static void a5psw_port_bridge_leave(struct dsa_switch *ds, int port, > > static void a5psw_port_stp_state_set(struct dsa_switch *ds, int port, u8 state) > { > - u32 mask = A5PSW_INPUT_LEARN_DIS(port) | A5PSW_INPUT_LEARN_BLOCK(port); > struct a5psw *a5psw = ds->priv; > - u32 reg = 0; > + bool learn, block; > > switch (state) { > case BR_STATE_DISABLED: > case BR_STATE_BLOCKING: > - reg |= A5PSW_INPUT_LEARN_DIS(port); > - reg |= A5PSW_INPUT_LEARN_BLOCK(port); > - break; > case BR_STATE_LISTENING: > - reg |= A5PSW_INPUT_LEARN_DIS(port); > + block = true; > + learn = false; > + a5psw_port_tx_enable(a5psw, port, false); Actually, after leaving a bridge, it seems like the DSA core put the port in STP DISABLED state. Which means it will potentially leave that port with TX disable... Since this TX enable is applying not only on bridge port but also on standalone port, it seems like this also needs to be reenabled in bridge_leave(). > break; > case BR_STATE_LEARNING: > - reg |= A5PSW_INPUT_LEARN_BLOCK(port); > + block = true; > + learn = true; > + a5psw_port_tx_enable(a5psw, port, false); > break; > case BR_STATE_FORWARDING: > - default: > + block = false; > + learn = true; > + a5psw_port_tx_enable(a5psw, port, true); > break; > + default: > + dev_err(ds->dev, "invalid STP state: %d\n", state); > + return; > } > > - a5psw_reg_rmw(a5psw, A5PSW_INPUT_LEARN, mask, reg); > + a5psw_port_learning_set(a5psw, port, learn, block); > } > > static void a5psw_port_fast_age(struct dsa_switch *ds, int port) > @@ -673,7 +706,7 @@ static int a5psw_setup(struct dsa_switch *ds) > } > > /* Configure management port */ > - reg = A5PSW_CPU_PORT | A5PSW_MGMT_CFG_DISCARD; > + reg = A5PSW_CPU_PORT | A5PSW_MGMT_CFG_ENABLE; > a5psw_reg_writel(a5psw, A5PSW_MGMT_CFG, reg); > > /* Set pattern 0 to forward all frame to mgmt port */ > diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h > index c67abd49c013..04d9486dbd21 100644 > --- a/drivers/net/dsa/rzn1_a5psw.h > +++ b/drivers/net/dsa/rzn1_a5psw.h > @@ -19,6 +19,8 @@ > #define A5PSW_PORT_OFFSET(port) (0x400 * (port)) > > #define A5PSW_PORT_ENA 0x8 > +#define A5PSW_PORT_ENA_TX_SHIFT 0 > +#define A5PSW_PORT_ENA_TX(port) BIT(port) > #define A5PSW_PORT_ENA_RX_SHIFT 16 > #define A5PSW_PORT_ENA_TX_RX(port) (BIT((port) + A5PSW_PORT_ENA_RX_SHIFT) | \ > BIT(port)) > @@ -36,7 +38,7 @@ > #define A5PSW_INPUT_LEARN_BLOCK(p) BIT(p) > > #define A5PSW_MGMT_CFG 0x20 > -#define A5PSW_MGMT_CFG_DISCARD BIT(7) > +#define A5PSW_MGMT_CFG_ENABLE BIT(6) > > #define A5PSW_MODE_CFG 0x24 > #define A5PSW_MODE_STATS_RESET BIT(31)
On Thu, Mar 30, 2023 at 10:48:28AM +0200, Clément Léger wrote: > Actually, after leaving a bridge, it seems like the DSA core put the > port in STP DISABLED state. Which means it will potentially leave that > port with TX disable... Since this TX enable is applying not only on > bridge port but also on standalone port, it seems like this also needs > to be reenabled in bridge_leave(). That's... not true? dsa_port_switchdev_unsync_attrs() has: /* Port left the bridge, put in BR_STATE_DISABLED by the bridge layer, * so allow it to be in BR_STATE_FORWARDING to be kept functional */ dsa_port_set_state_now(dp, BR_STATE_FORWARDING, true); a dump_stack() could help explain what's going on in your system?
Le Thu, 30 Mar 2023 17:56:23 +0300, Vladimir Oltean <olteanv@gmail.com> a écrit : > On Thu, Mar 30, 2023 at 10:48:28AM +0200, Clément Léger wrote: > > Actually, after leaving a bridge, it seems like the DSA core put the > > port in STP DISABLED state. Which means it will potentially leave that > > port with TX disable... Since this TX enable is applying not only on > > bridge port but also on standalone port, it seems like this also needs > > to be reenabled in bridge_leave(). > > That's... not true? dsa_port_switchdev_unsync_attrs() has: > > /* Port left the bridge, put in BR_STATE_DISABLED by the bridge layer, > * so allow it to be in BR_STATE_FORWARDING to be kept functional > */ > dsa_port_set_state_now(dp, BR_STATE_FORWARDING, true); > > a dump_stack() could help explain what's going on in your system? Indeed, I was referring to the messages displayed by the STP setp state function (br0: port 2(lan1) entered disabled state). But the DSA core indeed calls the stp_set_state() to enable forwarding which then reenables the Tx path so I guess we are all good with this series. Sorry for that.
Commit title: s/DPBU/BPDU/ On Thu, Mar 30, 2023 at 10:34:07AM +0200, Clément Léger wrote: > Current there were actually two problems which made the STP support non > working. First, the BPDU were disabled on the CPU port which means BDPUs s/BDPU/BPDU/ > were actually dropped internally to the switch. Separate patches for the 2 problems? > TO fix that, simply enable S/TO/To/ > BPDUs at management port level. Then, the a5psw_port_stp_set_state() > should have actually received BPDU while in LEARNING mode which was not > the case. Additionally, the BLOCKEN bit does not actually forbid > sending forwarded frames from that port. To fix this, add > a5psw_port_tx_enable() function which allows to disable TX. However, while > its name suggest that TX is totally disabled, it is not and can still > allows to send BPDUs even if disabled. This can be done by using forced s/allows/allow/ > forwarding with the switch tagging mecanism but keeping "filtering" s/mecanism/mechanism/ > disabled (which is already the case). Which these fixes, STP support is now s/Which/With/ > functional. > > Signed-off-by: Clément Léger <clement.leger@bootlin.com> > --- Have you considered adding some Fixes: tags and sending to the "net" tree? > drivers/net/dsa/rzn1_a5psw.c | 53 +++++++++++++++++++++++++++++------- > drivers/net/dsa/rzn1_a5psw.h | 4 ++- > 2 files changed, 46 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c > index 919027cf2012..bbc1424ed416 100644 > --- a/drivers/net/dsa/rzn1_a5psw.c > +++ b/drivers/net/dsa/rzn1_a5psw.c > @@ -120,6 +120,22 @@ static void a5psw_port_mgmtfwd_set(struct a5psw *a5psw, int port, bool enable) > a5psw_port_pattern_set(a5psw, port, A5PSW_PATTERN_MGMTFWD, enable); > } > > +static void a5psw_port_tx_enable(struct a5psw *a5psw, int port, bool enable) > +{ > + u32 mask = A5PSW_PORT_ENA_TX(port); > + u32 reg = enable ? mask : 0; > + > + /* Even though the port TX is disabled through TXENA bit in the > + * PORT_ENA register it can still send BPDUs. This depends on the tag s/register/register,/ > + * configuration added when sending packets from the CPU port to the > + * switch port. Indeed, when using forced forwarding without filtering, > + * even disabled port will be able to send packets that are tagged. This s/port/ports/ > + * allows to implement STP support when ports are in a state were s/were/where/ > + * forwarding traffic should be stopped but BPDUs should still be sent. To be absolutely clear, when talking about BPDUs, is it applicable effectively only to STP protocol frames, or to any management traffic sent by tag_rzn1_a5psw.c which has A5PSW_CTRL_DATA_FORCE_FORWARD set? > + */ > + a5psw_reg_rmw(a5psw, A5PSW_CMD_CFG(port), mask, reg); > +} > + > static void a5psw_port_enable_set(struct a5psw *a5psw, int port, bool enable) > { > u32 port_ena = 0; > @@ -292,6 +308,18 @@ static int a5psw_set_ageing_time(struct dsa_switch *ds, unsigned int msecs) > return 0; > } > > +static void a5psw_port_learning_set(struct a5psw *a5psw, int port, > + bool learning, bool blocked) > +{ > + u32 mask = A5PSW_INPUT_LEARN_DIS(port) | A5PSW_INPUT_LEARN_BLOCK(port); > + u32 reg = 0; > + > + reg |= !learning ? A5PSW_INPUT_LEARN_DIS(port) : 0; > + reg |= blocked ? A5PSW_INPUT_LEARN_BLOCK(port) : 0; > + > + a5psw_reg_rmw(a5psw, A5PSW_INPUT_LEARN, mask, reg); > +} Would it be useful to have independent functions for "learning" and "blocked", for when learning will be made configurable? > + > static void a5psw_flooding_set_resolution(struct a5psw *a5psw, int port, > bool set) > { > @@ -344,28 +372,33 @@ static void a5psw_port_bridge_leave(struct dsa_switch *ds, int port, > > static void a5psw_port_stp_state_set(struct dsa_switch *ds, int port, u8 state) > { > - u32 mask = A5PSW_INPUT_LEARN_DIS(port) | A5PSW_INPUT_LEARN_BLOCK(port); > struct a5psw *a5psw = ds->priv; > - u32 reg = 0; > + bool learn, block; > > switch (state) { > case BR_STATE_DISABLED: > case BR_STATE_BLOCKING: > - reg |= A5PSW_INPUT_LEARN_DIS(port); > - reg |= A5PSW_INPUT_LEARN_BLOCK(port); > - break; > case BR_STATE_LISTENING: > - reg |= A5PSW_INPUT_LEARN_DIS(port); > + block = true; > + learn = false; > + a5psw_port_tx_enable(a5psw, port, false); > break; > case BR_STATE_LEARNING: > - reg |= A5PSW_INPUT_LEARN_BLOCK(port); > + block = true; > + learn = true; > + a5psw_port_tx_enable(a5psw, port, false); > break; > case BR_STATE_FORWARDING: > - default: > + block = false; > + learn = true; > + a5psw_port_tx_enable(a5psw, port, true); > break; > + default: > + dev_err(ds->dev, "invalid STP state: %d\n", state); > + return; > } > > - a5psw_reg_rmw(a5psw, A5PSW_INPUT_LEARN, mask, reg); > + a5psw_port_learning_set(a5psw, port, learn, block); To be consistent, could you add a "bool tx_enabled" and a single call to a5psw_port_tx_enable() at the end? "block" could also be named "!rx_enabled" for some similarity and clarity regarding what it does. > } > > static void a5psw_port_fast_age(struct dsa_switch *ds, int port) > @@ -673,7 +706,7 @@ static int a5psw_setup(struct dsa_switch *ds) > } > > /* Configure management port */ > - reg = A5PSW_CPU_PORT | A5PSW_MGMT_CFG_DISCARD; > + reg = A5PSW_CPU_PORT | A5PSW_MGMT_CFG_ENABLE; > a5psw_reg_writel(a5psw, A5PSW_MGMT_CFG, reg); > > /* Set pattern 0 to forward all frame to mgmt port */ > diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h > index c67abd49c013..04d9486dbd21 100644 > --- a/drivers/net/dsa/rzn1_a5psw.h > +++ b/drivers/net/dsa/rzn1_a5psw.h > @@ -19,6 +19,8 @@ > #define A5PSW_PORT_OFFSET(port) (0x400 * (port)) > > #define A5PSW_PORT_ENA 0x8 > +#define A5PSW_PORT_ENA_TX_SHIFT 0 either use it in the A5PSW_PORT_ENA_TX() definition, or remove it. > +#define A5PSW_PORT_ENA_TX(port) BIT(port) > #define A5PSW_PORT_ENA_RX_SHIFT 16 > #define A5PSW_PORT_ENA_TX_RX(port) (BIT((port) + A5PSW_PORT_ENA_RX_SHIFT) | \ > BIT(port)) > @@ -36,7 +38,7 @@ > #define A5PSW_INPUT_LEARN_BLOCK(p) BIT(p) > > #define A5PSW_MGMT_CFG 0x20 > -#define A5PSW_MGMT_CFG_DISCARD BIT(7) > +#define A5PSW_MGMT_CFG_ENABLE BIT(6) > > #define A5PSW_MODE_CFG 0x24 > #define A5PSW_MODE_STATS_RESET BIT(31) > -- > 2.39.2 >
Le Thu, 30 Mar 2023 18:16:53 +0300, Vladimir Oltean <olteanv@gmail.com> a écrit : > Have you considered adding some Fixes: tags and sending to the "net" tree? I wasn't sure if due to the refactoring that should go directly to the net tree but I'll do that. But since they are fixes, that's the way to go. > > > drivers/net/dsa/rzn1_a5psw.c | 53 +++++++++++++++++++++++++++++------- > > drivers/net/dsa/rzn1_a5psw.h | 4 ++- > > 2 files changed, 46 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c > > index 919027cf2012..bbc1424ed416 100644 > > --- a/drivers/net/dsa/rzn1_a5psw.c > > +++ b/drivers/net/dsa/rzn1_a5psw.c > > @@ -120,6 +120,22 @@ static void a5psw_port_mgmtfwd_set(struct a5psw *a5psw, int port, bool enable) > > a5psw_port_pattern_set(a5psw, port, A5PSW_PATTERN_MGMTFWD, enable); > > } > > > > +static void a5psw_port_tx_enable(struct a5psw *a5psw, int port, bool enable) > > +{ > > + u32 mask = A5PSW_PORT_ENA_TX(port); > > + u32 reg = enable ? mask : 0; > > + > > + /* Even though the port TX is disabled through TXENA bit in the > > + * PORT_ENA register it can still send BPDUs. This depends on the tag > > s/register/register,/ > > > + * configuration added when sending packets from the CPU port to the > > + * switch port. Indeed, when using forced forwarding without filtering, > > + * even disabled port will be able to send packets that are tagged. This > > s/port/ports/ > > > + * allows to implement STP support when ports are in a state were > > s/were/where/ > > > + * forwarding traffic should be stopped but BPDUs should still be sent. > > To be absolutely clear, when talking about BPDUs, is it applicable > effectively only to STP protocol frames, or to any management traffic > sent by tag_rzn1_a5psw.c which has A5PSW_CTRL_DATA_FORCE_FORWARD set? The documentation uses BPDUs but this is to be understood as in a broader sense for "management frames" since it matches all the MAC with "01-80-c2-00-00-XX". > > > + */ > > + a5psw_reg_rmw(a5psw, A5PSW_CMD_CFG(port), mask, reg); > > +} > > + > > static void a5psw_port_enable_set(struct a5psw *a5psw, int port, bool enable) > > { > > u32 port_ena = 0; > > @@ -292,6 +308,18 @@ static int a5psw_set_ageing_time(struct dsa_switch *ds, unsigned int msecs) > > return 0; > > } > > > > +static void a5psw_port_learning_set(struct a5psw *a5psw, int port, > > + bool learning, bool blocked) > > +{ > > + u32 mask = A5PSW_INPUT_LEARN_DIS(port) | A5PSW_INPUT_LEARN_BLOCK(port); > > + u32 reg = 0; > > + > > + reg |= !learning ? A5PSW_INPUT_LEARN_DIS(port) : 0; > > + reg |= blocked ? A5PSW_INPUT_LEARN_BLOCK(port) : 0; > > + > > + a5psw_reg_rmw(a5psw, A5PSW_INPUT_LEARN, mask, reg); > > +} > > Would it be useful to have independent functions for "learning" and > "blocked", for when learning will be made configurable? You are right, If we allow configuring it through bridge_flags(), this clearly needs to be split up from blocking support. > > > + > > static void a5psw_flooding_set_resolution(struct a5psw *a5psw, int port, > > bool set) > > { > > @@ -344,28 +372,33 @@ static void a5psw_port_bridge_leave(struct dsa_switch *ds, int port, > > > > static void a5psw_port_stp_state_set(struct dsa_switch *ds, int port, u8 state) > > { > > - u32 mask = A5PSW_INPUT_LEARN_DIS(port) | A5PSW_INPUT_LEARN_BLOCK(port); > > struct a5psw *a5psw = ds->priv; > > - u32 reg = 0; > > + bool learn, block; > > > > switch (state) { > > case BR_STATE_DISABLED: > > case BR_STATE_BLOCKING: > > - reg |= A5PSW_INPUT_LEARN_DIS(port); > > - reg |= A5PSW_INPUT_LEARN_BLOCK(port); > > - break; > > case BR_STATE_LISTENING: > > - reg |= A5PSW_INPUT_LEARN_DIS(port); > > + block = true; > > + learn = false; > > + a5psw_port_tx_enable(a5psw, port, false); > > break; > > case BR_STATE_LEARNING: > > - reg |= A5PSW_INPUT_LEARN_BLOCK(port); > > + block = true; > > + learn = true; > > + a5psw_port_tx_enable(a5psw, port, false); > > break; > > case BR_STATE_FORWARDING: > > - default: > > + block = false; > > + learn = true; > > + a5psw_port_tx_enable(a5psw, port, true); > > break; > > + default: > > + dev_err(ds->dev, "invalid STP state: %d\n", state); > > + return; > > } > > > > - a5psw_reg_rmw(a5psw, A5PSW_INPUT_LEARN, mask, reg); > > + a5psw_port_learning_set(a5psw, port, learn, block); > > To be consistent, could you add a "bool tx_enabled" and a single call to > a5psw_port_tx_enable() at the end? "block" could also be named "!rx_enabled" > for some similarity and clarity regarding what it does. That seems reasonnable even though they do not act on the same registers but have the same corresponding effect (stopping ingress/egress traffic but with an exception for BPDU). > > > } > > > > static void a5psw_port_fast_age(struct dsa_switch *ds, int port) > > @@ -673,7 +706,7 @@ static int a5psw_setup(struct dsa_switch *ds) > > } > > > > /* Configure management port */ > > - reg = A5PSW_CPU_PORT | A5PSW_MGMT_CFG_DISCARD; > > + reg = A5PSW_CPU_PORT | A5PSW_MGMT_CFG_ENABLE; > > a5psw_reg_writel(a5psw, A5PSW_MGMT_CFG, reg); > > > > /* Set pattern 0 to forward all frame to mgmt port */ > > diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h > > index c67abd49c013..04d9486dbd21 100644 > > --- a/drivers/net/dsa/rzn1_a5psw.h > > +++ b/drivers/net/dsa/rzn1_a5psw.h > > @@ -19,6 +19,8 @@ > > #define A5PSW_PORT_OFFSET(port) (0x400 * (port)) > > > > #define A5PSW_PORT_ENA 0x8 > > +#define A5PSW_PORT_ENA_TX_SHIFT 0 > > either use it in the A5PSW_PORT_ENA_TX() definition, or remove it. > > > +#define A5PSW_PORT_ENA_TX(port) BIT(port) > > #define A5PSW_PORT_ENA_RX_SHIFT 16 > > #define A5PSW_PORT_ENA_TX_RX(port) (BIT((port) + A5PSW_PORT_ENA_RX_SHIFT) | \ > > BIT(port)) > > @@ -36,7 +38,7 @@ > > #define A5PSW_INPUT_LEARN_BLOCK(p) BIT(p) > > > > #define A5PSW_MGMT_CFG 0x20 > > -#define A5PSW_MGMT_CFG_DISCARD BIT(7) > > +#define A5PSW_MGMT_CFG_ENABLE BIT(6) > > > > #define A5PSW_MODE_CFG 0x24 > > #define A5PSW_MODE_STATS_RESET BIT(31) > > -- > > 2.39.2 > > >
On Thu, Mar 30, 2023 at 05:44:27PM +0200, Clément Léger wrote: > Le Thu, 30 Mar 2023 18:16:53 +0300, > Vladimir Oltean <olteanv@gmail.com> a écrit : > > > Have you considered adding some Fixes: tags and sending to the "net" tree? > > I wasn't sure if due to the refactoring that should go directly to the > net tree but I'll do that. But since they are fixes, that's the way to > go. My common sense says that code quality comes first, and so, the code looks however it needs to look, keeping in mind that it still needs to be a punctual fix for the problem. This doesn't change the fact that it's a fix for an an observable bug, and so, it's a candidate for 'net'. That's just my opinion though, others may disagree. > > To be absolutely clear, when talking about BPDUs, is it applicable > > effectively only to STP protocol frames, or to any management traffic > > sent by tag_rzn1_a5psw.c which has A5PSW_CTRL_DATA_FORCE_FORWARD set? > > The documentation uses BPDUs but this is to be understood as in a > broader sense for "management frames" since it matches all the MAC with > "01-80-c2-00-00-XX". And even so, is it just for frames sent to "01-80-c2-00-00-XX", or for all frames sent with A5PSW_CTRL_DATA_FORCE_FORWARD? Other switch families can inject whatever they want into ports that are in the BLOCKING STP state.
Le Thu, 30 Mar 2023 19:51:23 +0300, Vladimir Oltean <olteanv@gmail.com> a écrit : > On Thu, Mar 30, 2023 at 05:44:27PM +0200, Clément Léger wrote: > > Le Thu, 30 Mar 2023 18:16:53 +0300, > > Vladimir Oltean <olteanv@gmail.com> a écrit : > > > > > Have you considered adding some Fixes: tags and sending to the "net" tree? > > > > I wasn't sure if due to the refactoring that should go directly to the > > net tree but I'll do that. But since they are fixes, that's the way to > > go. > > My common sense says that code quality comes first, and so, the code > looks however it needs to look, keeping in mind that it still needs to > be a punctual fix for the problem. This doesn't change the fact that > it's a fix for an an observable bug, and so, it's a candidate for 'net'. Agreed. > > That's just my opinion though, others may disagree. > > > > To be absolutely clear, when talking about BPDUs, is it applicable > > > effectively only to STP protocol frames, or to any management traffic > > > sent by tag_rzn1_a5psw.c which has A5PSW_CTRL_DATA_FORCE_FORWARD set? > > > > The documentation uses BPDUs but this is to be understood as in a > > broader sense for "management frames" since it matches all the MAC with > > "01-80-c2-00-00-XX". > > And even so, is it just for frames sent to "01-80-c2-00-00-XX", or for > all frames sent with A5PSW_CTRL_DATA_FORCE_FORWARD? Other switch > families can inject whatever they want into ports that are in the > BLOCKING STP state. Forced forwarded to disabled ports will only apply to management frames. At least this is what the documentation says for forced forwarding (section 4.5.5.4, Table 4.234): Normal frames will be filtered always (i.e. can never be transmitted to disabled ports).
diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c index 919027cf2012..bbc1424ed416 100644 --- a/drivers/net/dsa/rzn1_a5psw.c +++ b/drivers/net/dsa/rzn1_a5psw.c @@ -120,6 +120,22 @@ static void a5psw_port_mgmtfwd_set(struct a5psw *a5psw, int port, bool enable) a5psw_port_pattern_set(a5psw, port, A5PSW_PATTERN_MGMTFWD, enable); } +static void a5psw_port_tx_enable(struct a5psw *a5psw, int port, bool enable) +{ + u32 mask = A5PSW_PORT_ENA_TX(port); + u32 reg = enable ? mask : 0; + + /* Even though the port TX is disabled through TXENA bit in the + * PORT_ENA register it can still send BPDUs. This depends on the tag + * configuration added when sending packets from the CPU port to the + * switch port. Indeed, when using forced forwarding without filtering, + * even disabled port will be able to send packets that are tagged. This + * allows to implement STP support when ports are in a state were + * forwarding traffic should be stopped but BPDUs should still be sent. + */ + a5psw_reg_rmw(a5psw, A5PSW_CMD_CFG(port), mask, reg); +} + static void a5psw_port_enable_set(struct a5psw *a5psw, int port, bool enable) { u32 port_ena = 0; @@ -292,6 +308,18 @@ static int a5psw_set_ageing_time(struct dsa_switch *ds, unsigned int msecs) return 0; } +static void a5psw_port_learning_set(struct a5psw *a5psw, int port, + bool learning, bool blocked) +{ + u32 mask = A5PSW_INPUT_LEARN_DIS(port) | A5PSW_INPUT_LEARN_BLOCK(port); + u32 reg = 0; + + reg |= !learning ? A5PSW_INPUT_LEARN_DIS(port) : 0; + reg |= blocked ? A5PSW_INPUT_LEARN_BLOCK(port) : 0; + + a5psw_reg_rmw(a5psw, A5PSW_INPUT_LEARN, mask, reg); +} + static void a5psw_flooding_set_resolution(struct a5psw *a5psw, int port, bool set) { @@ -344,28 +372,33 @@ static void a5psw_port_bridge_leave(struct dsa_switch *ds, int port, static void a5psw_port_stp_state_set(struct dsa_switch *ds, int port, u8 state) { - u32 mask = A5PSW_INPUT_LEARN_DIS(port) | A5PSW_INPUT_LEARN_BLOCK(port); struct a5psw *a5psw = ds->priv; - u32 reg = 0; + bool learn, block; switch (state) { case BR_STATE_DISABLED: case BR_STATE_BLOCKING: - reg |= A5PSW_INPUT_LEARN_DIS(port); - reg |= A5PSW_INPUT_LEARN_BLOCK(port); - break; case BR_STATE_LISTENING: - reg |= A5PSW_INPUT_LEARN_DIS(port); + block = true; + learn = false; + a5psw_port_tx_enable(a5psw, port, false); break; case BR_STATE_LEARNING: - reg |= A5PSW_INPUT_LEARN_BLOCK(port); + block = true; + learn = true; + a5psw_port_tx_enable(a5psw, port, false); break; case BR_STATE_FORWARDING: - default: + block = false; + learn = true; + a5psw_port_tx_enable(a5psw, port, true); break; + default: + dev_err(ds->dev, "invalid STP state: %d\n", state); + return; } - a5psw_reg_rmw(a5psw, A5PSW_INPUT_LEARN, mask, reg); + a5psw_port_learning_set(a5psw, port, learn, block); } static void a5psw_port_fast_age(struct dsa_switch *ds, int port) @@ -673,7 +706,7 @@ static int a5psw_setup(struct dsa_switch *ds) } /* Configure management port */ - reg = A5PSW_CPU_PORT | A5PSW_MGMT_CFG_DISCARD; + reg = A5PSW_CPU_PORT | A5PSW_MGMT_CFG_ENABLE; a5psw_reg_writel(a5psw, A5PSW_MGMT_CFG, reg); /* Set pattern 0 to forward all frame to mgmt port */ diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h index c67abd49c013..04d9486dbd21 100644 --- a/drivers/net/dsa/rzn1_a5psw.h +++ b/drivers/net/dsa/rzn1_a5psw.h @@ -19,6 +19,8 @@ #define A5PSW_PORT_OFFSET(port) (0x400 * (port)) #define A5PSW_PORT_ENA 0x8 +#define A5PSW_PORT_ENA_TX_SHIFT 0 +#define A5PSW_PORT_ENA_TX(port) BIT(port) #define A5PSW_PORT_ENA_RX_SHIFT 16 #define A5PSW_PORT_ENA_TX_RX(port) (BIT((port) + A5PSW_PORT_ENA_RX_SHIFT) | \ BIT(port)) @@ -36,7 +38,7 @@ #define A5PSW_INPUT_LEARN_BLOCK(p) BIT(p) #define A5PSW_MGMT_CFG 0x20 -#define A5PSW_MGMT_CFG_DISCARD BIT(7) +#define A5PSW_MGMT_CFG_ENABLE BIT(6) #define A5PSW_MODE_CFG 0x24 #define A5PSW_MODE_STATS_RESET BIT(31)
Current there were actually two problems which made the STP support non working. First, the BPDU were disabled on the CPU port which means BDPUs were actually dropped internally to the switch. TO fix that, simply enable BPDUs at management port level. Then, the a5psw_port_stp_set_state() should have actually received BPDU while in LEARNING mode which was not the case. Additionally, the BLOCKEN bit does not actually forbid sending forwarded frames from that port. To fix this, add a5psw_port_tx_enable() function which allows to disable TX. However, while its name suggest that TX is totally disabled, it is not and can still allows to send BPDUs even if disabled. This can be done by using forced forwarding with the switch tagging mecanism but keeping "filtering" disabled (which is already the case). Which these fixes, STP support is now functional. Signed-off-by: Clément Léger <clement.leger@bootlin.com> --- drivers/net/dsa/rzn1_a5psw.c | 53 +++++++++++++++++++++++++++++------- drivers/net/dsa/rzn1_a5psw.h | 4 ++- 2 files changed, 46 insertions(+), 11 deletions(-)