Re: [PATCH net-next 1/2] net: dsa: rzn1-a5psw: enable DPBU for CPU port and fix STP states

["Clément Léger" <clement.leger@bootlin.com>]

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
> >   
> 



-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com