| Message ID | 20230511170202.742087-3-alexis.lothore@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: rzn1-a5psw: disabled learning for standalone ports and fix STP support | expand |
On Thu, May 11, 2023 at 07:02:01PM +0200, alexis.lothore@bootlin.com wrote: > From: Clément Léger <clement.leger@bootlin.com> > > stp_set_state() should actually allow receiving BPDU while in LEARNING > mode which is 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 > allow to send BPDUs even if disabled. This can be done by using forced > forwarding with the switch tagging mechanism but keeping "filtering" > disabled (which is already the case in the rzn1-a5sw tag driver). With > these fixes, STP support is now functional. > > Fixes: 888cdb892b61 ("net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver") > Signed-off-by: Clément Léger <clement.leger@bootlin.com> > Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com> > --- Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > @@ -344,28 +376,35 @@ 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 learning_enabled, rx_enabled, tx_enabled; Absolutely minor comment: in the networking subsystem there is a coding style preference to order lines with variable declarations longest to shortest (reverse Christmas tree). Since I don't see another less frivolous reason to resend the patch set, I thought I'd just mention for next time.
Hello Vladimir, thanks for the fast review ! On 5/11/23 23:37, Vladimir Oltean wrote: > On Thu, May 11, 2023 at 07:02:01PM +0200, alexis.lothore@bootlin.com wrote: >> From: Clément Léger <clement.leger@bootlin.com> >> >> stp_set_state() should actually allow receiving BPDU while in LEARNING >> mode which is 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 >> allow to send BPDUs even if disabled. This can be done by using forced >> forwarding with the switch tagging mechanism but keeping "filtering" >> disabled (which is already the case in the rzn1-a5sw tag driver). With >> these fixes, STP support is now functional. >> >> Fixes: 888cdb892b61 ("net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver") >> Signed-off-by: Clément Léger <clement.leger@bootlin.com> >> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com> >> --- > > Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > >> @@ -344,28 +376,35 @@ 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 learning_enabled, rx_enabled, tx_enabled; > > Absolutely minor comment: in the networking subsystem there is a coding > style preference to order lines with variable declarations longest to > shortest (reverse Christmas tree). Since I don't see another less > frivolous reason to resend the patch set, I thought I'd just mention > for next time. ACK. Since an error has been raised by CI bot on this series, I will send a V3 and fix this ordering too. Regards,
diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c index 8a419e2ffe2a..a3acac29ffa6 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 ports will be able to send packets that are tagged. + * This allows to implement STP support when ports are in a state where + * forwarding traffic should be stopped but BPDUs should still be sent. + */ + a5psw_reg_rmw(a5psw, A5PSW_PORT_ENA, mask, reg); +} + static void a5psw_port_enable_set(struct a5psw *a5psw, int port, bool enable) { u32 port_ena = 0; @@ -292,6 +308,22 @@ 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 learn) +{ + u32 mask = A5PSW_INPUT_LEARN_DIS(port); + u32 reg = !learn ? mask : 0; + + a5psw_reg_rmw(a5psw, A5PSW_INPUT_LEARN, mask, reg); +} + +static void a5psw_port_rx_block_set(struct a5psw *a5psw, int port, bool block) +{ + u32 mask = A5PSW_INPUT_LEARN_BLOCK(port); + u32 reg = block ? mask : 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 +376,35 @@ 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 learning_enabled, rx_enabled, tx_enabled; 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); + rx_enabled = false; + tx_enabled = false; + learning_enabled = false; break; case BR_STATE_LEARNING: - reg |= A5PSW_INPUT_LEARN_BLOCK(port); + rx_enabled = false; + tx_enabled = false; + learning_enabled = true; break; case BR_STATE_FORWARDING: - default: + rx_enabled = true; + tx_enabled = true; + learning_enabled = 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, learning_enabled); + a5psw_port_rx_block_set(a5psw, port, !rx_enabled); + a5psw_port_tx_enable(a5psw, port, tx_enabled); } static void a5psw_port_fast_age(struct dsa_switch *ds, int port) diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h index c67abd49c013..b869192eef3f 100644 --- a/drivers/net/dsa/rzn1_a5psw.h +++ b/drivers/net/dsa/rzn1_a5psw.h @@ -19,6 +19,7 @@ #define A5PSW_PORT_OFFSET(port) (0x400 * (port)) #define A5PSW_PORT_ENA 0x8 +#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 +37,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)