[net,v2,2/3] net: dsa: rzn1-a5psw: fix STP states handling
Commit Message
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>
---
drivers/net/dsa/rzn1_a5psw.c | 57 ++++++++++++++++++++++++++++++------
drivers/net/dsa/rzn1_a5psw.h | 3 +-
2 files changed, 50 insertions(+), 10 deletions(-)
Comments
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,
@@ -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)
@@ -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)