netdev - Re: [PATCH net-next 4/4 v4] net: dsa: rtl8366rb: Support setting STP state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210929215430.kyi5ekdu5i36um2k@skbuf>
Date:   Thu, 30 Sep 2021 00:54:30 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Mauri Sandberg <sandberg@...lfence.com>,
        DENG Qingfang <dqfext@...il.com>
Subject: Re: [PATCH net-next 4/4 v4] net: dsa: rtl8366rb: Support setting STP
 state

On Wed, Sep 29, 2021 at 11:03:49PM +0200, Linus Walleij wrote:
> This adds support for setting the STP state to the RTL8366RB
> DSA switch. This rids the following message from the kernel on
> e.g. OpenWrt:
> 
> DSA: failed to set STP state 3 (-95)
> 
> Since the RTL8366RB has one STP state register per FID with
> two bit per port in each, we simply loop over all the FIDs
> and set the state on all of them.
> 
> Cc: Vladimir Oltean <olteanv@...il.com>
> Cc: Alvin Šipraga <alsi@...g-olufsen.dk>
> Cc: Mauri Sandberg <sandberg@...lfence.com>
> Cc: DENG Qingfang <dqfext@...il.com>
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> ---
> ChangeLog v1->v4:
> - New patch after discovering that we can do really nice
>   bridge offloading with these bits.
> ---
>  drivers/net/dsa/rtl8366rb.c | 47 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index 748f22ab9130..c143fdab4802 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -110,6 +110,14 @@
>  
>  #define RTL8366RB_POWER_SAVING_REG	0x0021
>  
> +/* Spanning tree status (STP) control, two bits per port per FID */
> +#define RTL8368S_SPT_STATE_BASE		0x0050 /* 0x0050..0x0057 */

What does "SPT" stand for?

Also, is there any particular reason why these are named after RTL8368S,
when the entire driver has a naming scheme which follows RTL8366RB?

> +#define RTL8368S_SPT_STATE_MSK		0x3
> +#define RTL8368S_SPT_STATE_DISABLED	0x0
> +#define RTL8368S_SPT_STATE_BLOCKING	0x1
> +#define RTL8368S_SPT_STATE_LEARNING	0x2
> +#define RTL8368S_SPT_STATE_FORWARDING	0x3
> +
>  /* CPU port control reg */
>  #define RTL8368RB_CPU_CTRL_REG		0x0061
>  #define RTL8368RB_CPU_PORTS_MSK		0x00FF
> @@ -254,6 +262,7 @@
>  #define RTL8366RB_NUM_LEDGROUPS		4
>  #define RTL8366RB_NUM_VIDS		4096
>  #define RTL8366RB_PRIORITYMAX		7
> +#define RTL8366RB_NUM_FIDS		8
>  #define RTL8366RB_FIDMAX		7
>  
>  #define RTL8366RB_PORT_1		BIT(0) /* In userspace port 0 */
> @@ -1359,6 +1368,43 @@ rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
>  	return 0;
>  }
>  
> +static void
> +rtl8366rb_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +	u16 mask;
> +	u32 val;
> +	int i;
> +
> +	switch (state) {
> +	case BR_STATE_DISABLED:
> +		val = RTL8368S_SPT_STATE_DISABLED;
> +		break;
> +	case BR_STATE_BLOCKING:
> +	case BR_STATE_LISTENING:
> +		val = RTL8368S_SPT_STATE_BLOCKING;
> +		break;
> +	case BR_STATE_LEARNING:
> +		val = RTL8368S_SPT_STATE_LEARNING;
> +		break;
> +	case BR_STATE_FORWARDING:
> +		val = RTL8368S_SPT_STATE_FORWARDING;
> +		break;
> +	default:
> +		dev_err(smi->dev, "unknown bridge state requested\n");
> +		return;
> +	};
> +
> +	mask = (RTL8368S_SPT_STATE_MSK << (port * 2));

Could you not add a port argument:

#define RTL8366RB_STP_MASK			GENMASK(1, 0)
#define RTL8366RB_STP_STATE(port, state)	(((state) << ((port) * 2))
#define RTL8366RB_STP_STATE_MASK(port)		RTL8366RB_STP_STATE(RTL8366RB_STP_MASK, (port))

	regmap_update_bits(smi->map, RTL8366RB_STP_STATE_BASE + i,
			   RTL8366RB_STP_STATE_MASK(port),
			   RTL8366RB_STP_STATE(port, val));

> +	val <<= (port * 2);
> +
> +	/* Set the same status for the port on all the FIDs */
> +	for (i = 0; i < RTL8366RB_NUM_FIDS; i++) {
> +		regmap_update_bits(smi->map, RTL8368S_SPT_STATE_BASE + i,
> +				   mask, val);
> +	}
> +}
> +
>  static void
>  rtl8366rb_port_fast_age(struct dsa_switch *ds, int port)
>  {
> @@ -1784,6 +1830,7 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = {
>  	.port_disable = rtl8366rb_port_disable,
>  	.port_pre_bridge_flags = rtl8366rb_port_pre_bridge_flags,
>  	.port_bridge_flags = rtl8366rb_port_bridge_flags,
> +	.port_stp_state_set = rtl8366rb_port_stp_state_set,
>  	.port_fast_age = rtl8366rb_port_fast_age,
>  	.port_change_mtu = rtl8366rb_change_mtu,
>  	.port_max_mtu = rtl8366rb_max_mtu,
> -- 
> 2.31.1
>