Le Thu, 30 Mar 2023 18:16:53 +0300,
Vladimir Oltean <olteanv@xxxxxxxxx> 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
> >
>
--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com
