[PATCH net-next 0/5 v2] RTL8366RB improvements

[PATCH net-next 0/5 v2] RTL8366RB improvements

[Linus Walleij]

This series contains a few impromvents: the bridge offloading
support from Deng, then dropping of custom VLAN setup and
then some extended support suggested by Vladimir.

ChangeLog v1->v2: some fixes and more patches!

DENG Qingfang (1):
  net: dsa: rtl8366rb: support bridge offloading

Linus Walleij (4):
  net: dsa: rtl8366: Drop custom VLAN set-up
  net: dsa: rtl8366rb: Support disabling learning
  net: dsa: rtl8366rb: Support flood control
  net: dsa: rtl8366rb: Support fast aging

 drivers/net/dsa/realtek-smi-core.h |   1 -
 drivers/net/dsa/rtl8366.c          |  48 --------
 drivers/net/dsa/rtl8366rb.c        | 188 +++++++++++++++++++++++++++--
 3 files changed, 180 insertions(+), 57 deletions(-)

-- 
2.31.1

[PATCH net-next 1/5 v2] net: dsa: rtl8366rb: support bridge offloading

[Linus Walleij]

From: DENG Qingfang <dqfext@gmail.com>

Use port isolation registers to configure bridge offloading.

Tested on the D-Link DIR-685, switching between ports and
sniffing ports to make sure no packets leak.

Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Signed-off-by: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- introduce RTL8366RB_PORT_ISO_PORTS() to shift the port
  mask into place so we are not confused by the enable
  bit.
- Use this with dsa_user_ports() to isolate the CPU port
  from itself.
---
 drivers/net/dsa/rtl8366rb.c | 87 +++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index a89093bc6c6a..50ee7cd62484 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -300,6 +300,13 @@
 #define RTL8366RB_INTERRUPT_STATUS_REG	0x0442
 #define RTL8366RB_NUM_INTERRUPT		14 /* 0..13 */
 
+/* Port isolation registers */
+#define RTL8366RB_PORT_ISO_BASE		0x0F08
+#define RTL8366RB_PORT_ISO(pnum)	(RTL8366RB_PORT_ISO_BASE + (pnum))
+#define RTL8366RB_PORT_ISO_EN		BIT(0)
+#define RTL8366RB_PORT_ISO_PORTS_MASK	GENMASK(7, 1)
+#define RTL8366RB_PORT_ISO_PORTS(pmask)	(pmask << 1)
+
 /* bits 0..5 enable force when cleared */
 #define RTL8366RB_MAC_FORCE_CTRL_REG	0x0F11
 
@@ -835,6 +842,22 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	/* Isolate all user ports so only the CPU port can access them */
+	for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
+		ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(i),
+				   RTL8366RB_PORT_ISO_EN |
+				   RTL8366RB_PORT_ISO_PORTS(BIT(RTL8366RB_PORT_NUM_CPU)));
+		if (ret)
+			return ret;
+	}
+	/* CPU port can access all ports */
+	dev_info(smi->dev, "DSA user port mask: %08x\n", dsa_user_ports(ds));
+	ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(RTL8366RB_PORT_NUM_CPU),
+			   RTL8366RB_PORT_ISO_PORTS(dsa_user_ports(ds))|
+			   RTL8366RB_PORT_ISO_EN);
+	if (ret)
+		return ret;
+
 	/* Set up the "green ethernet" feature */
 	ret = rtl8366rb_jam_table(rtl8366rb_green_jam,
 				  ARRAY_SIZE(rtl8366rb_green_jam), smi, false);
@@ -1127,6 +1150,68 @@ rtl8366rb_port_disable(struct dsa_switch *ds, int port)
 	rb8366rb_set_port_led(smi, port, false);
 }
 
+static int
+rtl8366rb_port_bridge_join(struct dsa_switch *ds, int port,
+			   struct net_device *bridge)
+{
+	struct realtek_smi *smi = ds->priv;
+	unsigned int port_bitmap = 0;
+	int ret, i;
+
+	/* Loop over all other ports than this one */
+	for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
+		/* Handled last */
+		if (i == port)
+			continue;
+		/* Not on this bridge */
+		if (dsa_to_port(ds, i)->bridge_dev != bridge)
+			continue;
+		/* Join this port to each other port on the bridge */
+		ret = regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(i),
+					 RTL8366RB_PORT_ISO_PORTS(BIT(port)),
+					 RTL8366RB_PORT_ISO_PORTS(BIT(port)));
+		if (ret)
+			return ret;
+
+		port_bitmap |= BIT(i);
+	}
+
+	/* Set the bits for the ports we can access */
+	return regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(port),
+				  RTL8366RB_PORT_ISO_PORTS_MASK,
+				  RTL8366RB_PORT_ISO_PORTS(port_bitmap));
+}
+
+static void
+rtl8366rb_port_bridge_leave(struct dsa_switch *ds, int port,
+			    struct net_device *bridge)
+{
+	struct realtek_smi *smi = ds->priv;
+	unsigned int port_bitmap = 0;
+	int ret, i;
+
+	/* Loop over all other ports than this one */
+	for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
+		/* Handled last */
+		if (i == port)
+			continue;
+		/* Not on this bridge */
+		if (dsa_to_port(ds, i)->bridge_dev != bridge)
+			continue;
+		/* Remove this port from any other port on the bridge */
+		ret = regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(i),
+					 RTL8366RB_PORT_ISO_PORTS(BIT(port)), 0);
+		if (ret)
+			return;
+
+		port_bitmap |= BIT(i);
+	}
+
+	/* Clear the bits for the ports we can access */
+	regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(port),
+			   RTL8366RB_PORT_ISO_PORTS(port_bitmap), 0);
+}
+
 static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
 {
 	struct realtek_smi *smi = ds->priv;
@@ -1510,6 +1595,8 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = {
 	.get_strings = rtl8366_get_strings,
 	.get_ethtool_stats = rtl8366_get_ethtool_stats,
 	.get_sset_count = rtl8366_get_sset_count,
+	.port_bridge_join = rtl8366rb_port_bridge_join,
+	.port_bridge_leave = rtl8366rb_port_bridge_leave,
 	.port_vlan_filtering = rtl8366_vlan_filtering,
 	.port_vlan_add = rtl8366_vlan_add,
 	.port_vlan_del = rtl8366_vlan_del,
-- 
2.31.1

Re: [PATCH net-next 1/5 v2] net: dsa: rtl8366rb: support bridge offloading

[Vladimir Oltean]

On Mon, Aug 30, 2021 at 11:48:55PM +0200, Linus Walleij wrote:
> From: DENG Qingfang <dqfext@gmail.com>
> 
> Use port isolation registers to configure bridge offloading.
> 
> Tested on the D-Link DIR-685, switching between ports and
> sniffing ports to make sure no packets leak.
> 
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - introduce RTL8366RB_PORT_ISO_PORTS() to shift the port
>   mask into place so we are not confused by the enable
>   bit.
> - Use this with dsa_user_ports() to isolate the CPU port
>   from itself.
> ---
>  drivers/net/dsa/rtl8366rb.c | 87 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
> 
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index a89093bc6c6a..50ee7cd62484 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -300,6 +300,13 @@
>  #define RTL8366RB_INTERRUPT_STATUS_REG	0x0442
>  #define RTL8366RB_NUM_INTERRUPT		14 /* 0..13 */
>  
> +/* Port isolation registers */
> +#define RTL8366RB_PORT_ISO_BASE		0x0F08
> +#define RTL8366RB_PORT_ISO(pnum)	(RTL8366RB_PORT_ISO_BASE + (pnum))
> +#define RTL8366RB_PORT_ISO_EN		BIT(0)
> +#define RTL8366RB_PORT_ISO_PORTS_MASK	GENMASK(7, 1)
> +#define RTL8366RB_PORT_ISO_PORTS(pmask)	(pmask << 1)

Would be nice to enclose pmask between a set of parentheses.

> +
>  /* bits 0..5 enable force when cleared */
>  #define RTL8366RB_MAC_FORCE_CTRL_REG	0x0F11
>  
> @@ -835,6 +842,22 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
>  	if (ret)
>  		return ret;
>  
> +	/* Isolate all user ports so only the CPU port can access them */
> +	for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
> +		ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(i),
> +				   RTL8366RB_PORT_ISO_EN |
> +				   RTL8366RB_PORT_ISO_PORTS(BIT(RTL8366RB_PORT_NUM_CPU)));
> +		if (ret)
> +			return ret;
> +	}
> +	/* CPU port can access all ports */
> +	dev_info(smi->dev, "DSA user port mask: %08x\n", dsa_user_ports(ds));
> +	ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(RTL8366RB_PORT_NUM_CPU),
> +			   RTL8366RB_PORT_ISO_PORTS(dsa_user_ports(ds))|

For beauty you can add the missing space here between ) and |.

> +			   RTL8366RB_PORT_ISO_EN);
> +	if (ret)
> +		return ret;
> +
>  	/* Set up the "green ethernet" feature */
>  	ret = rtl8366rb_jam_table(rtl8366rb_green_jam,
>  				  ARRAY_SIZE(rtl8366rb_green_jam), smi, false);
> @@ -1127,6 +1150,68 @@ rtl8366rb_port_disable(struct dsa_switch *ds, int port)
>  	rb8366rb_set_port_led(smi, port, false);
>  }
>  
> +static int
> +rtl8366rb_port_bridge_join(struct dsa_switch *ds, int port,
> +			   struct net_device *bridge)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +	unsigned int port_bitmap = 0;
> +	int ret, i;
> +
> +	/* Loop over all other ports than this one */
> +	for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
> +		/* Handled last */
> +		if (i == port)
> +			continue;
> +		/* Not on this bridge */
> +		if (dsa_to_port(ds, i)->bridge_dev != bridge)
> +			continue;
> +		/* Join this port to each other port on the bridge */
> +		ret = regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(i),
> +					 RTL8366RB_PORT_ISO_PORTS(BIT(port)),
> +					 RTL8366RB_PORT_ISO_PORTS(BIT(port)));
> +		if (ret)
> +			return ret;
> +
> +		port_bitmap |= BIT(i);
> +	}
> +
> +	/* Set the bits for the ports we can access */
> +	return regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(port),
> +				  RTL8366RB_PORT_ISO_PORTS_MASK,
> +				  RTL8366RB_PORT_ISO_PORTS(port_bitmap));
> +}
> +
> +static void
> +rtl8366rb_port_bridge_leave(struct dsa_switch *ds, int port,
> +			    struct net_device *bridge)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +	unsigned int port_bitmap = 0;
> +	int ret, i;
> +
> +	/* Loop over all other ports than this one */
> +	for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
> +		/* Handled last */
> +		if (i == port)
> +			continue;
> +		/* Not on this bridge */
> +		if (dsa_to_port(ds, i)->bridge_dev != bridge)
> +			continue;
> +		/* Remove this port from any other port on the bridge */
> +		ret = regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(i),
> +					 RTL8366RB_PORT_ISO_PORTS(BIT(port)), 0);
> +		if (ret)
> +			return;

I don't think it is beneficial here to catch the error and return early?
We don't even have a print, it is a rather silent failure. I think if a
function that returns void fails, we should limp on and finish...
unbridging... the... port...

> +
> +		port_bitmap |= BIT(i);
> +	}
> +
> +	/* Clear the bits for the ports we can access */
> +	regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(port),
> +			   RTL8366RB_PORT_ISO_PORTS(port_bitmap), 0);
> +}
> +
>  static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  {
>  	struct realtek_smi *smi = ds->priv;
> @@ -1510,6 +1595,8 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = {
>  	.get_strings = rtl8366_get_strings,
>  	.get_ethtool_stats = rtl8366_get_ethtool_stats,
>  	.get_sset_count = rtl8366_get_sset_count,
> +	.port_bridge_join = rtl8366rb_port_bridge_join,
> +	.port_bridge_leave = rtl8366rb_port_bridge_leave,
>  	.port_vlan_filtering = rtl8366_vlan_filtering,
>  	.port_vlan_add = rtl8366_vlan_add,
>  	.port_vlan_del = rtl8366_vlan_del,
> -- 
> 2.31.1
> 

Re: [PATCH net-next 1/5 v2] net: dsa: rtl8366rb: support bridge offloading

[Alvin Šipraga]

On 8/30/21 11:48 PM, Linus Walleij wrote:
> From: DENG Qingfang <dqfext@gmail.com>
> 
> Use port isolation registers to configure bridge offloading.
> 
> Tested on the D-Link DIR-685, switching between ports and
> sniffing ports to make sure no packets leak.
> 
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

> ChangeLog v1->v2:
> - introduce RTL8366RB_PORT_ISO_PORTS() to shift the port
>    mask into place so we are not confused by the enable
>    bit.
> - Use this with dsa_user_ports() to isolate the CPU port
>    from itself.
> ---
>   drivers/net/dsa/rtl8366rb.c | 87 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 87 insertions(+)
> 
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index a89093bc6c6a..50ee7cd62484 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -300,6 +300,13 @@
>   #define RTL8366RB_INTERRUPT_STATUS_REG	0x0442
>   #define RTL8366RB_NUM_INTERRUPT		14 /* 0..13 */
>   
> +/* Port isolation registers */
> +#define RTL8366RB_PORT_ISO_BASE		0x0F08
> +#define RTL8366RB_PORT_ISO(pnum)	(RTL8366RB_PORT_ISO_BASE + (pnum))
> +#define RTL8366RB_PORT_ISO_EN		BIT(0)
> +#define RTL8366RB_PORT_ISO_PORTS_MASK	GENMASK(7, 1)
> +#define RTL8366RB_PORT_ISO_PORTS(pmask)	(pmask << 1)
> +
>   /* bits 0..5 enable force when cleared */
>   #define RTL8366RB_MAC_FORCE_CTRL_REG	0x0F11
>   
> @@ -835,6 +842,22 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
>   	if (ret)
>   		return ret;
>   
> +	/* Isolate all user ports so only the CPU port can access them */
> +	for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
> +		ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(i),
> +				   RTL8366RB_PORT_ISO_EN |
> +				   RTL8366RB_PORT_ISO_PORTS(BIT(RTL8366RB_PORT_NUM_CPU)));
> +		if (ret)
> +			return ret;
> +	}
> +	/* CPU port can access all ports */
> +	dev_info(smi->dev, "DSA user port mask: %08x\n", dsa_user_ports(ds));

Maybe dev_dbg? Not all people appreciate chatty drivers...

> +	ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(RTL8366RB_PORT_NUM_CPU),
> +			   RTL8366RB_PORT_ISO_PORTS(dsa_user_ports(ds))|
> +			   RTL8366RB_PORT_ISO_EN);
> +	if (ret)
> +		return ret;
> +
>   	/* Set up the "green ethernet" feature */
>   	ret = rtl8366rb_jam_table(rtl8366rb_green_jam,
>   				  ARRAY_SIZE(rtl8366rb_green_jam), smi, false);
> @@ -1127,6 +1150,68 @@ rtl8366rb_port_disable(struct dsa_switch *ds, int port)
>   	rb8366rb_set_port_led(smi, port, false);
>   }
>   
> +static int
> +rtl8366rb_port_bridge_join(struct dsa_switch *ds, int port,
> +			   struct net_device *bridge)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +	unsigned int port_bitmap = 0;
> +	int ret, i;
> +
> +	/* Loop over all other ports than this one */
> +	for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
> +		/* Handled last */
> +		if (i == port)
> +			continue;
> +		/* Not on this bridge */
> +		if (dsa_to_port(ds, i)->bridge_dev != bridge)
> +			continue;
> +		/* Join this port to each other port on the bridge */
> +		ret = regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(i),
> +					 RTL8366RB_PORT_ISO_PORTS(BIT(port)),
> +					 RTL8366RB_PORT_ISO_PORTS(BIT(port)));
> +		if (ret)
> +			return ret;
> +
> +		port_bitmap |= BIT(i);
> +	}
> +
> +	/* Set the bits for the ports we can access */
> +	return regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(port),
> +				  RTL8366RB_PORT_ISO_PORTS_MASK,
> +				  RTL8366RB_PORT_ISO_PORTS(port_bitmap));
> +}
> +
> +static void
> +rtl8366rb_port_bridge_leave(struct dsa_switch *ds, int port,
> +			    struct net_device *bridge)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +	unsigned int port_bitmap = 0;
> +	int ret, i;
> +
> +	/* Loop over all other ports than this one */
> +	for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
> +		/* Handled last */
> +		if (i == port)
> +			continue;
> +		/* Not on this bridge */
> +		if (dsa_to_port(ds, i)->bridge_dev != bridge)
> +			continue;
> +		/* Remove this port from any other port on the bridge */
> +		ret = regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(i),
> +					 RTL8366RB_PORT_ISO_PORTS(BIT(port)), 0);
> +		if (ret)
> +			return;
> +
> +		port_bitmap |= BIT(i);
> +	}
> +
> +	/* Clear the bits for the ports we can access */
> +	regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(port),
> +			   RTL8366RB_PORT_ISO_PORTS(port_bitmap), 0);
> +}
> +
>   static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>   {
>   	struct realtek_smi *smi = ds->priv;
> @@ -1510,6 +1595,8 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = {
>   	.get_strings = rtl8366_get_strings,
>   	.get_ethtool_stats = rtl8366_get_ethtool_stats,
>   	.get_sset_count = rtl8366_get_sset_count,
> +	.port_bridge_join = rtl8366rb_port_bridge_join,
> +	.port_bridge_leave = rtl8366rb_port_bridge_leave,
>   	.port_vlan_filtering = rtl8366_vlan_filtering,
>   	.port_vlan_add = rtl8366_vlan_add,
>   	.port_vlan_del = rtl8366_vlan_del,
> 

Re: [PATCH net-next 1/5 v2] net: dsa: rtl8366rb: support bridge offloading

[Florian Fainelli]

On 8/30/2021 2:48 PM, Linus Walleij wrote:
> From: DENG Qingfang <dqfext@gmail.com>
> 
> Use port isolation registers to configure bridge offloading.
> 
> Tested on the D-Link DIR-685, switching between ports and
> sniffing ports to make sure no packets leak.
> 
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - introduce RTL8366RB_PORT_ISO_PORTS() to shift the port
>    mask into place so we are not confused by the enable
>    bit.
> - Use this with dsa_user_ports() to isolate the CPU port
>    from itself.
> ---
>   drivers/net/dsa/rtl8366rb.c | 87 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 87 insertions(+)
> 
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index a89093bc6c6a..50ee7cd62484 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -300,6 +300,13 @@
>   #define RTL8366RB_INTERRUPT_STATUS_REG	0x0442
>   #define RTL8366RB_NUM_INTERRUPT		14 /* 0..13 */
>   
> +/* Port isolation registers */
> +#define RTL8366RB_PORT_ISO_BASE		0x0F08
> +#define RTL8366RB_PORT_ISO(pnum)	(RTL8366RB_PORT_ISO_BASE + (pnum))
> +#define RTL8366RB_PORT_ISO_EN		BIT(0)
> +#define RTL8366RB_PORT_ISO_PORTS_MASK	GENMASK(7, 1)
> +#define RTL8366RB_PORT_ISO_PORTS(pmask)	(pmask << 1)
> +
>   /* bits 0..5 enable force when cleared */
>   #define RTL8366RB_MAC_FORCE_CTRL_REG	0x0F11
>   
> @@ -835,6 +842,22 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
>   	if (ret)
>   		return ret;
>   
> +	/* Isolate all user ports so only the CPU port can access them */
> +	for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
> +		ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(i),
> +				   RTL8366RB_PORT_ISO_EN |
> +				   RTL8366RB_PORT_ISO_PORTS(BIT(RTL8366RB_PORT_NUM_CPU)));
> +		if (ret)
> +			return ret;
> +	}
> +	/* CPU port can access all ports */
> +	dev_info(smi->dev, "DSA user port mask: %08x\n", dsa_user_ports(ds));

As mentioned by Alvin, this should be a dev_dbg() or completely eliminated.

> +	ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(RTL8366RB_PORT_NUM_CPU),
> +			   RTL8366RB_PORT_ISO_PORTS(dsa_user_ports(ds))|
> +			   RTL8366RB_PORT_ISO_EN);
> +	if (ret)
> +		return ret;
> +
>   	/* Set up the "green ethernet" feature */
>   	ret = rtl8366rb_jam_table(rtl8366rb_green_jam,
>   				  ARRAY_SIZE(rtl8366rb_green_jam), smi, false);
> @@ -1127,6 +1150,68 @@ rtl8366rb_port_disable(struct dsa_switch *ds, int port)
>   	rb8366rb_set_port_led(smi, port, false);
>   }
>   
> +static int
> +rtl8366rb_port_bridge_join(struct dsa_switch *ds, int port,
> +			   struct net_device *bridge)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +	unsigned int port_bitmap = 0;
> +	int ret, i;
> +
> +	/* Loop over all other ports than this one */

This sentence is a bit weird, how about: Loop over all ports but this one?

> +	for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
> +		/* Handled last */

This comment is a bit misleading as it would suggest that the loop does 
act on 'port' when really it does that outside of the loop.

With those nitpicks fixed:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net-next 2/5 v2] net: dsa: rtl8366: Drop custom VLAN set-up

[Linus Walleij]

This hacky default VLAN setup was done in order to direct
packets to the right ports and provide port isolation, both
which we now support properly using custom tags and proper
bridge port isolation.

We can drop the custom VLAN code and leave all VLAN handling
alone, as users expect things to be. We can also drop
ds->configure_vlan_while_not_filtering = false; and let
the core deal with any VLANs it wants.

Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- No changes.
---
 drivers/net/dsa/realtek-smi-core.h |  1 -
 drivers/net/dsa/rtl8366.c          | 48 ------------------------------
 drivers/net/dsa/rtl8366rb.c        |  4 +--
 3 files changed, 1 insertion(+), 52 deletions(-)

diff --git a/drivers/net/dsa/realtek-smi-core.h b/drivers/net/dsa/realtek-smi-core.h
index fcf465f7f922..c8fbd7b9fd0b 100644
--- a/drivers/net/dsa/realtek-smi-core.h
+++ b/drivers/net/dsa/realtek-smi-core.h
@@ -129,7 +129,6 @@ int rtl8366_set_pvid(struct realtek_smi *smi, unsigned int port,
 int rtl8366_enable_vlan4k(struct realtek_smi *smi, bool enable);
 int rtl8366_enable_vlan(struct realtek_smi *smi, bool enable);
 int rtl8366_reset_vlan(struct realtek_smi *smi);
-int rtl8366_init_vlan(struct realtek_smi *smi);
 int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
 			   struct netlink_ext_ack *extack);
 int rtl8366_vlan_add(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index 75897a369096..59c5bc4f7b71 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c
@@ -292,54 +292,6 @@ int rtl8366_reset_vlan(struct realtek_smi *smi)
 }
 EXPORT_SYMBOL_GPL(rtl8366_reset_vlan);
 
-int rtl8366_init_vlan(struct realtek_smi *smi)
-{
-	int port;
-	int ret;
-
-	ret = rtl8366_reset_vlan(smi);
-	if (ret)
-		return ret;
-
-	/* Loop over the available ports, for each port, associate
-	 * it with the VLAN (port+1)
-	 */
-	for (port = 0; port < smi->num_ports; port++) {
-		u32 mask;
-
-		if (port == smi->cpu_port)
-			/* For the CPU port, make all ports members of this
-			 * VLAN.
-			 */
-			mask = GENMASK((int)smi->num_ports - 1, 0);
-		else
-			/* For all other ports, enable itself plus the
-			 * CPU port.
-			 */
-			mask = BIT(port) | BIT(smi->cpu_port);
-
-		/* For each port, set the port as member of VLAN (port+1)
-		 * and untagged, except for the CPU port: the CPU port (5) is
-		 * member of VLAN 6 and so are ALL the other ports as well.
-		 * Use filter 0 (no filter).
-		 */
-		dev_info(smi->dev, "VLAN%d port mask for port %d, %08x\n",
-			 (port + 1), port, mask);
-		ret = rtl8366_set_vlan(smi, (port + 1), mask, mask, 0);
-		if (ret)
-			return ret;
-
-		dev_info(smi->dev, "VLAN%d port %d, PVID set to %d\n",
-			 (port + 1), port, (port + 1));
-		ret = rtl8366_set_pvid(smi, port, (port + 1));
-		if (ret)
-			return ret;
-	}
-
-	return rtl8366_enable_vlan(smi, true);
-}
-EXPORT_SYMBOL_GPL(rtl8366_init_vlan);
-
 int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
 			   struct netlink_ext_ack *extack)
 {
diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index 50ee7cd62484..8b040440d2d4 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -986,7 +986,7 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 			return ret;
 	}
 
-	ret = rtl8366_init_vlan(smi);
+	ret = rtl8366_reset_vlan(smi);
 	if (ret)
 		return ret;
 
@@ -1000,8 +1000,6 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 		return -ENODEV;
 	}
 
-	ds->configure_vlan_while_not_filtering = false;
-
 	return 0;
 }
 
-- 
2.31.1

Re: [PATCH net-next 2/5 v2] net: dsa: rtl8366: Drop custom VLAN set-up

[Vladimir Oltean]

On Mon, Aug 30, 2021 at 11:48:56PM +0200, Linus Walleij wrote:
> This hacky default VLAN setup was done in order to direct
> packets to the right ports and provide port isolation, both
> which we now support properly using custom tags and proper
> bridge port isolation.
> 
> We can drop the custom VLAN code and leave all VLAN handling
> alone, as users expect things to be. We can also drop
> ds->configure_vlan_while_not_filtering = false; and let
> the core deal with any VLANs it wants.
> 
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Please keep this review tag when posting v3.

Re: [PATCH net-next 2/5 v2] net: dsa: rtl8366: Drop custom VLAN set-up

[Alvin Šipraga]

On 8/30/21 11:48 PM, Linus Walleij wrote:
> This hacky default VLAN setup was done in order to direct
> packets to the right ports and provide port isolation, both
> which we now support properly using custom tags and proper
> bridge port isolation.
> 
> We can drop the custom VLAN code and leave all VLAN handling
> alone, as users expect things to be. We can also drop
> ds->configure_vlan_while_not_filtering = false; and let
> the core deal with any VLANs it wants.
> 
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

> ChangeLog v1->v2:
> - No changes.
> ---
>   drivers/net/dsa/realtek-smi-core.h |  1 -
>   drivers/net/dsa/rtl8366.c          | 48 ------------------------------
>   drivers/net/dsa/rtl8366rb.c        |  4 +--
>   3 files changed, 1 insertion(+), 52 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek-smi-core.h b/drivers/net/dsa/realtek-smi-core.h
> index fcf465f7f922..c8fbd7b9fd0b 100644
> --- a/drivers/net/dsa/realtek-smi-core.h
> +++ b/drivers/net/dsa/realtek-smi-core.h
> @@ -129,7 +129,6 @@ int rtl8366_set_pvid(struct realtek_smi *smi, unsigned int port,
>   int rtl8366_enable_vlan4k(struct realtek_smi *smi, bool enable);
>   int rtl8366_enable_vlan(struct realtek_smi *smi, bool enable);
>   int rtl8366_reset_vlan(struct realtek_smi *smi);
> -int rtl8366_init_vlan(struct realtek_smi *smi);
>   int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
>   			   struct netlink_ext_ack *extack);
>   int rtl8366_vlan_add(struct dsa_switch *ds, int port,
> diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
> index 75897a369096..59c5bc4f7b71 100644
> --- a/drivers/net/dsa/rtl8366.c
> +++ b/drivers/net/dsa/rtl8366.c
> @@ -292,54 +292,6 @@ int rtl8366_reset_vlan(struct realtek_smi *smi)
>   }
>   EXPORT_SYMBOL_GPL(rtl8366_reset_vlan);
>   
> -int rtl8366_init_vlan(struct realtek_smi *smi)
> -{
> -	int port;
> -	int ret;
> -
> -	ret = rtl8366_reset_vlan(smi);
> -	if (ret)
> -		return ret;
> -
> -	/* Loop over the available ports, for each port, associate
> -	 * it with the VLAN (port+1)
> -	 */
> -	for (port = 0; port < smi->num_ports; port++) {
> -		u32 mask;
> -
> -		if (port == smi->cpu_port)
> -			/* For the CPU port, make all ports members of this
> -			 * VLAN.
> -			 */
> -			mask = GENMASK((int)smi->num_ports - 1, 0);
> -		else
> -			/* For all other ports, enable itself plus the
> -			 * CPU port.
> -			 */
> -			mask = BIT(port) | BIT(smi->cpu_port);
> -
> -		/* For each port, set the port as member of VLAN (port+1)
> -		 * and untagged, except for the CPU port: the CPU port (5) is
> -		 * member of VLAN 6 and so are ALL the other ports as well.
> -		 * Use filter 0 (no filter).
> -		 */
> -		dev_info(smi->dev, "VLAN%d port mask for port %d, %08x\n",
> -			 (port + 1), port, mask);
> -		ret = rtl8366_set_vlan(smi, (port + 1), mask, mask, 0);
> -		if (ret)
> -			return ret;
> -
> -		dev_info(smi->dev, "VLAN%d port %d, PVID set to %d\n",
> -			 (port + 1), port, (port + 1));
> -		ret = rtl8366_set_pvid(smi, port, (port + 1));
> -		if (ret)
> -			return ret;
> -	}
> -
> -	return rtl8366_enable_vlan(smi, true);
> -}
> -EXPORT_SYMBOL_GPL(rtl8366_init_vlan);
> -
>   int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
>   			   struct netlink_ext_ack *extack)
>   {
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index 50ee7cd62484..8b040440d2d4 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -986,7 +986,7 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
>   			return ret;
>   	}
>   
> -	ret = rtl8366_init_vlan(smi);
> +	ret = rtl8366_reset_vlan(smi);
>   	if (ret)
>   		return ret;
>   
> @@ -1000,8 +1000,6 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
>   		return -ENODEV;
>   	}
>   
> -	ds->configure_vlan_while_not_filtering = false;
> -
>   	return 0;
>   }
>   
> 

Re: [PATCH net-next 2/5 v2] net: dsa: rtl8366: Drop custom VLAN set-up

[Florian Fainelli]

On 8/30/2021 2:48 PM, Linus Walleij wrote:
> This hacky default VLAN setup was done in order to direct
> packets to the right ports and provide port isolation, both
> which we now support properly using custom tags and proper
> bridge port isolation.
> 
> We can drop the custom VLAN code and leave all VLAN handling
> alone, as users expect things to be. We can also drop
> ds->configure_vlan_while_not_filtering = false; and let
> the core deal with any VLANs it wants.
> 
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net-next 3/5 v2] net: dsa: rtl8366rb: Support disabling learning

[Linus Walleij]

The RTL8366RB hardware supports disabling learning per-port
so let's make use of this feature. Rename some unfortunately
named registers in the process.

Suggested-by: Vladimir Oltean <olteanv@gmail.com>
Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- New patch suggested by Vladimir.
---
 drivers/net/dsa/rtl8366rb.c | 47 +++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index 8b040440d2d4..2cadd3e57e8b 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -14,6 +14,7 @@
 
 #include <linux/bitops.h>
 #include <linux/etherdevice.h>
+#include <linux/if_bridge.h>
 #include <linux/interrupt.h>
 #include <linux/irqdomain.h>
 #include <linux/irqchip/chained_irq.h>
@@ -42,9 +43,12 @@
 /* Port Enable Control register */
 #define RTL8366RB_PECR				0x0001
 
-/* Switch Security Control registers */
-#define RTL8366RB_SSCR0				0x0002
-#define RTL8366RB_SSCR1				0x0003
+/* Switch per-port learning disablement register */
+#define RTL8366RB_PORT_LEARNDIS_CTRL		0x0002
+
+/* Security control, actually aging register */
+#define RTL8366RB_SECURITY_CTRL			0x0003
+
 #define RTL8366RB_SSCR2				0x0004
 #define RTL8366RB_SSCR2_DROP_UNKNOWN_DA		BIT(0)
 
@@ -912,12 +916,12 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 		rb->max_mtu[i] = 1532;
 
 	/* Enable learning for all ports */
-	ret = regmap_write(smi->map, RTL8366RB_SSCR0, 0);
+	ret = regmap_write(smi->map, RTL8366RB_PORT_LEARNDIS_CTRL, 0);
 	if (ret)
 		return ret;
 
 	/* Enable auto ageing for all ports */
-	ret = regmap_write(smi->map, RTL8366RB_SSCR1, 0);
+	ret = regmap_write(smi->map, RTL8366RB_SECURITY_CTRL, 0);
 	if (ret)
 		return ret;
 
@@ -1148,6 +1152,37 @@ rtl8366rb_port_disable(struct dsa_switch *ds, int port)
 	rb8366rb_set_port_led(smi, port, false);
 }
 
+static int
+rtl8366rb_port_pre_bridge_flags(struct dsa_switch *ds, int port,
+				struct switchdev_brport_flags flags,
+				struct netlink_ext_ack *extack)
+{
+	/* We support enabling/disabling learning */
+	if (flags.mask & ~(BR_LEARNING))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int
+rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
+			    struct switchdev_brport_flags flags,
+			    struct netlink_ext_ack *extack)
+{
+	struct realtek_smi *smi = ds->priv;
+	int ret;
+
+	if (flags.mask & BR_LEARNING) {
+		ret = regmap_update_bits(smi->map, RTL8366RB_PORT_LEARNDIS_CTRL,
+					 BIT(port),
+					 (flags.val & BR_LEARNING) ? 0 : BIT(port));
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int
 rtl8366rb_port_bridge_join(struct dsa_switch *ds, int port,
 			   struct net_device *bridge)
@@ -1600,6 +1635,8 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = {
 	.port_vlan_del = rtl8366_vlan_del,
 	.port_enable = rtl8366rb_port_enable,
 	.port_disable = rtl8366rb_port_disable,
+	.port_pre_bridge_flags = rtl8366rb_port_pre_bridge_flags,
+	.port_bridge_flags = rtl8366rb_port_bridge_flags,
 	.port_change_mtu = rtl8366rb_change_mtu,
 	.port_max_mtu = rtl8366rb_max_mtu,
 };
-- 
2.31.1

Re: [PATCH net-next 3/5 v2] net: dsa: rtl8366rb: Support disabling learning

[Vladimir Oltean]

On Mon, Aug 30, 2021 at 11:48:57PM +0200, Linus Walleij wrote:
> The RTL8366RB hardware supports disabling learning per-port
> so let's make use of this feature. Rename some unfortunately
> named registers in the process.
> 
> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - New patch suggested by Vladimir.
> ---
>  drivers/net/dsa/rtl8366rb.c | 47 +++++++++++++++++++++++++++++++++----
>  1 file changed, 42 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index 8b040440d2d4..2cadd3e57e8b 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -14,6 +14,7 @@
>  
>  #include <linux/bitops.h>
>  #include <linux/etherdevice.h>
> +#include <linux/if_bridge.h>
>  #include <linux/interrupt.h>
>  #include <linux/irqdomain.h>
>  #include <linux/irqchip/chained_irq.h>
> @@ -42,9 +43,12 @@
>  /* Port Enable Control register */
>  #define RTL8366RB_PECR				0x0001
>  
> -/* Switch Security Control registers */
> -#define RTL8366RB_SSCR0				0x0002
> -#define RTL8366RB_SSCR1				0x0003
> +/* Switch per-port learning disablement register */
> +#define RTL8366RB_PORT_LEARNDIS_CTRL		0x0002
> +
> +/* Security control, actually aging register */
> +#define RTL8366RB_SECURITY_CTRL			0x0003
> +
>  #define RTL8366RB_SSCR2				0x0004
>  #define RTL8366RB_SSCR2_DROP_UNKNOWN_DA		BIT(0)
>  
> @@ -912,12 +916,12 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
>  		rb->max_mtu[i] = 1532;
>  
>  	/* Enable learning for all ports */
> -	ret = regmap_write(smi->map, RTL8366RB_SSCR0, 0);
> +	ret = regmap_write(smi->map, RTL8366RB_PORT_LEARNDIS_CTRL, 0);

So the expected behavior for standalone ports would be to _disable_
learning. In rtl8366rb_setup, they are standalone.

>  	if (ret)
>  		return ret;
>  
>  	/* Enable auto ageing for all ports */
> -	ret = regmap_write(smi->map, RTL8366RB_SSCR1, 0);
> +	ret = regmap_write(smi->map, RTL8366RB_SECURITY_CTRL, 0);
>  	if (ret)
>  		return ret;
>  
> @@ -1148,6 +1152,37 @@ rtl8366rb_port_disable(struct dsa_switch *ds, int port)
>  	rb8366rb_set_port_led(smi, port, false);
>  }
>  
> +static int
> +rtl8366rb_port_pre_bridge_flags(struct dsa_switch *ds, int port,
> +				struct switchdev_brport_flags flags,
> +				struct netlink_ext_ack *extack)
> +{
> +	/* We support enabling/disabling learning */
> +	if (flags.mask & ~(BR_LEARNING))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int
> +rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
> +			    struct switchdev_brport_flags flags,
> +			    struct netlink_ext_ack *extack)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +	int ret;
> +
> +	if (flags.mask & BR_LEARNING) {
> +		ret = regmap_update_bits(smi->map, RTL8366RB_PORT_LEARNDIS_CTRL,
> +					 BIT(port),
> +					 (flags.val & BR_LEARNING) ? 0 : BIT(port));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int
>  rtl8366rb_port_bridge_join(struct dsa_switch *ds, int port,
>  			   struct net_device *bridge)
> @@ -1600,6 +1635,8 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = {
>  	.port_vlan_del = rtl8366_vlan_del,
>  	.port_enable = rtl8366rb_port_enable,
>  	.port_disable = rtl8366rb_port_disable,
> +	.port_pre_bridge_flags = rtl8366rb_port_pre_bridge_flags,
> +	.port_bridge_flags = rtl8366rb_port_bridge_flags,
>  	.port_change_mtu = rtl8366rb_change_mtu,
>  	.port_max_mtu = rtl8366rb_max_mtu,
>  };
> -- 
> 2.31.1
> 

Re: [PATCH net-next 3/5 v2] net: dsa: rtl8366rb: Support disabling learning

[Linus Walleij]

On Tue, Aug 31, 2021 at 12:40 AM Vladimir Oltean <olteanv@gmail.com> wrote:

> >       /* Enable learning for all ports */
> > -     ret = regmap_write(smi->map, RTL8366RB_SSCR0, 0);
> > +     ret = regmap_write(smi->map, RTL8366RB_PORT_LEARNDIS_CTRL, 0);
>
> So the expected behavior for standalone ports would be to _disable_
> learning. In rtl8366rb_setup, they are standalone.

OK I altered the code such that I disable learning on all ports instead,
the new callback can be used to enable it.

What about the CPU port here? Sorry if it is a dumb question...

I just disabled learning on all ports including the CPU port, but
should I just leave learning on on the CPU port?

Yours,
Linus Walleij

Re: [PATCH net-next 3/5 v2] net: dsa: rtl8366rb: Support disabling learning

[Vladimir Oltean]

On Tue, Sep 07, 2021 at 05:52:01PM +0200, Linus Walleij wrote:
> On Tue, Aug 31, 2021 at 12:40 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> > >       /* Enable learning for all ports */
> > > -     ret = regmap_write(smi->map, RTL8366RB_SSCR0, 0);
> > > +     ret = regmap_write(smi->map, RTL8366RB_PORT_LEARNDIS_CTRL, 0);
> >
> > So the expected behavior for standalone ports would be to _disable_
> > learning. In rtl8366rb_setup, they are standalone.
> 
> OK I altered the code such that I disable learning on all ports instead,
> the new callback can be used to enable it.
> 
> What about the CPU port here? Sorry if it is a dumb question...
> 
> I just disabled learning on all ports including the CPU port, but
> should I just leave learning on on the CPU port?

Not a dumb question. DSA does not change the learning state of the CPU
port and lets drivers choose among two solutions:

(a) enable hardware address learning manually on it. This was the
    traditional approach, and the exact meaning of what this actually
    does will vary from one switch vendor to another, so you need to
    know what you get and if it complies with the network stack's expectations.
    At one end, you have ASICs where despite this setting, the {MAC SA,
    VLAN ID} will not be learned for packets injected towards a precise
    destination port specified in the DSA tag (so-called "control
    packets"). These ASICs would only learn the {MAC SA, VLAN ID} from
    packets sent from the CPU that are not injected precisely into a
    port, but forwarded freely according to the FDB (in the case of your
    Realtek tagging protocol, think of it as an all-zeroes destination
    port mask).  These are so-called "data plane packets", and DSA has
    traditionally not had any means for building/sending one, hence the
    complete lack of address learning for this type of switches,
    practically.
    On the other end you will have switches which will learn the {MAC
    SA, VLAN ID} from both control and data packets, with no way of
    controlling which one gets learned and which one doesn't.  This will
    further cause problems when you consider that some packets might
    need to be partially forwarded in software between switch ports that
    are members of a hardware bridge. In general, address learning must
    be recognized as a bridge layer function, and only packets
    pertaining to the data plane of a bridge must be subject to
    learning. Ergo: if you inject a packet into a standalone port, its
    MAC SA should not be learned (for reasons that have real life
    implications, but are a bit beside the point to detail here).
    There might also exist in-between hardware implementations, where
    there might be a global learning setting for the CPU port, with an
    override per packet (a bit in the DSA tag).

(b) implement .port_fdb_add and .port_fdb_del and then set
    ds->assisted_learning_on_cpu_port = true. This was created exactly
    for the category of switches that won't learn from CPU-injected
    traffic even when asked to do so, but has since been extended to
    cover use cases even for switches where that is a possibility.
    For example, in a setup with multiple CPU ports (and this includes
    obscure setups where every switch has a single CPU port, but is part
    of a multi-chip topology), hardware address learning on any
    individual CPU port does not make sense, because that learned
    address would keep bouncing back and forth between one CPU port and
    the other, when it really should have stayed fixed on both.
    The way this option works is that it tells DSA to listen for
    switchdev events emitted by the bridge for FDB entries that were
    learned by the software path and point towards the general direction
    of the bridge (towards the bridge itself, or towards a non-DSA
    interface in the same bridge as a DSA port, like a Wi-Fi AP).
    These software FDB entries will then be installed through .port_fdb_add
    as static FDB entries on the CPU port(s), and removed when they are no
    longer needed. Drivers which support multiple CPU ports should then
    program these FDB entries to hardware in a way in which installing
    the entry towards CPU port B will not override a pre-existing FDB
    entry that was pointing towards CPU port A, but instead just append
    to the destination port mask of any pre-existing FDB entry. This is
    also the pattern used when programming multicast (MDB) entries to
    hardware.
    This solution also addresses the observation that packets injected
    towards standalone ports should not result in the {MAC SA, VLAN ID}
    getting learned, due to a combination of two factors:
    - When you set ds->assisted_learning_on_cpu_port = true, you must
      disable hardware learning on the CPU port(s)
    - Since the only FDB entries installed on the CPU port are ones
      originating from switchdev events emitted by a bridge, learning is
      limited to the bridging layer

It is understandable if you do not want to opt into ds->assisted_learning_on_cpu_port,
and if the way in which your DSA tagging protocol injects packets into
the hardware will always remain as "control packets", this might not
even show up any problem. Typically, "control packets" will be sent to
the destination port mask from the DSA tag with no questions asked. That
is to say, if you want to xmit a packet with a given {MAC DA, VLAN ID}
towards port 0, but the same {MAC DA, VLAN ID} was already learned on
the CPU port, the switch will be happy to send it towards port 0
precisely because it's a "control packet" and it will not look up the
FDB for it. With "data packets" it's an entirely different story, the
FDB will be looked up, and the switch will say "hey, I received a packet
with this MAC DA from the CPU port, but my FDB says the destination is
the CPU port. I am told to not reflect packets back from where they came
from, so just drop it". If you are sure that neither you nor anyone else
will ever implement support for data plane packets (bridge tx_fwd_offload)
for this hardware, then statically enabling hardware learning on the CPU
port might be enough if it actually works.

There is a third option where you can still learn the {MAC SA, VLAN ID}
in-band from bridging layer packets (and not from packets sent to standalone ports),
without the overhead of extra SPI/MDIO/I2C transactions that the assisted
learning feature requires.
This third option is available if you can tell the switch to learn on a
per-packet basis, and then to implement the tx_fwd_offload bridge feature.
The idea is to tell the hardware, through the DSA tag on xmit, to only
learn the MAC SA from packets which are sent on behalf of a bridge
(these will have skb->offload_fwd_mark == true).

So in the end, you have quite a bit of choice. First of all I would
start by figuring out what the hardware is capable of doing.

Enable the hardware learning bit on the CPU port, then build this setup:

           192.168.100.3
                br0
               /   \
              /     \
             /       \
         swp0         swp1
          |            |
       Station A    Station B
    192.168.100.1   192.168.100.2


And ping br0 from Station A, attached to the swp0 of your board.

Then tcpdump on Station B, to see all packets.
If you see the ICMP requests sent by Station A towards br0, it means
that the switch has not learned the MAC SA from br0's ICMP replies, and
it basically doesn't know where the MAC address of br0 is. So it will
always flood the packets targeting br0, towards all ports in br0's
forwarding domain. "All ports" includes swp1 in this case, the curious
neighbor.

With address learning functioning properly on the CPU port, you will
only see an initial pair of packets being flooded, that being during a
time when the switch does not know what the intended destination is,
because it has not seen that MAC DA in the MAC SA field of any prior
packets on any ports.

[PATCH net-next 4/5 v2] net: dsa: rtl8366rb: Support flood control

[Linus Walleij]

Now that we have implemented bridge flag handling we can easily
support flood (storm) control as well so let's do it.

Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- New patch
---
 drivers/net/dsa/rtl8366rb.c | 38 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index 2cadd3e57e8b..4cb0e336ce6b 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -149,6 +149,11 @@
 
 #define RTL8366RB_VLAN_INGRESS_CTRL2_REG	0x037f
 
+#define RTL8366RB_STORM_BC_CTRL			0x03e0
+#define RTL8366RB_STORM_MC_CTRL			0x03e1
+#define RTL8366RB_STORM_UNDA_CTRL		0x03e2
+#define RTL8366RB_STORM_UNMC_CTRL		0x03e3
+
 /* LED control registers */
 #define RTL8366RB_LED_BLINKRATE_REG		0x0430
 #define RTL8366RB_LED_BLINKRATE_MASK		0x0007
@@ -1158,7 +1163,8 @@ rtl8366rb_port_pre_bridge_flags(struct dsa_switch *ds, int port,
 				struct netlink_ext_ack *extack)
 {
 	/* We support enabling/disabling learning */
-	if (flags.mask & ~(BR_LEARNING))
+	if (flags.mask & ~(BR_LEARNING | BR_BCAST_FLOOD |
+                           BR_MCAST_FLOOD | BR_FLOOD))
 		return -EINVAL;
 
 	return 0;
@@ -1180,6 +1186,36 @@ rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
 			return ret;
 	}
 
+	if (flags.mask & BR_BCAST_FLOOD) {
+		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_BC_CTRL,
+					 BIT(port),
+					 (flags.val & BR_BCAST_FLOOD) ? BIT(port) : 0);
+		if (ret)
+			return ret;
+	}
+
+	if (flags.mask & BR_MCAST_FLOOD) {
+		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_MC_CTRL,
+					 BIT(port),
+					 (flags.val & BR_MCAST_FLOOD) ? BIT(port) : 0);
+		if (ret)
+			return ret;
+	}
+
+	/* Enable/disable both types of unicast floods */
+	if (flags.mask & BR_FLOOD) {
+		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_UNDA_CTRL,
+					 BIT(port),
+					 (flags.val & BR_FLOOD) ? BIT(port) : 0);
+		if (ret)
+			return ret;
+		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_UNMC_CTRL,
+					 BIT(port),
+					 (flags.val & BR_FLOOD) ? BIT(port) : 0);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
-- 
2.31.1

Re: [PATCH net-next 4/5 v2] net: dsa: rtl8366rb: Support flood control

[Vladimir Oltean]

On Mon, Aug 30, 2021 at 11:48:58PM +0200, Linus Walleij wrote:
> Now that we have implemented bridge flag handling we can easily
> support flood (storm) control as well so let's do it.
> 
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - New patch
> ---
>  drivers/net/dsa/rtl8366rb.c | 38 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index 2cadd3e57e8b..4cb0e336ce6b 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -149,6 +149,11 @@
>  
>  #define RTL8366RB_VLAN_INGRESS_CTRL2_REG	0x037f
>  
> +#define RTL8366RB_STORM_BC_CTRL			0x03e0
> +#define RTL8366RB_STORM_MC_CTRL			0x03e1
> +#define RTL8366RB_STORM_UNDA_CTRL		0x03e2
> +#define RTL8366RB_STORM_UNMC_CTRL		0x03e3
> +
>  /* LED control registers */
>  #define RTL8366RB_LED_BLINKRATE_REG		0x0430
>  #define RTL8366RB_LED_BLINKRATE_MASK		0x0007
> @@ -1158,7 +1163,8 @@ rtl8366rb_port_pre_bridge_flags(struct dsa_switch *ds, int port,
>  				struct netlink_ext_ack *extack)
>  {
>  	/* We support enabling/disabling learning */
> -	if (flags.mask & ~(BR_LEARNING))
> +	if (flags.mask & ~(BR_LEARNING | BR_BCAST_FLOOD |
> +                           BR_MCAST_FLOOD | BR_FLOOD))

Spaces instead of tabs here?

>  		return -EINVAL;
>  
>  	return 0;
> @@ -1180,6 +1186,36 @@ rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
>  			return ret;
>  	}
>  
> +	if (flags.mask & BR_BCAST_FLOOD) {
> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_BC_CTRL,
> +					 BIT(port),
> +					 (flags.val & BR_BCAST_FLOOD) ? BIT(port) : 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (flags.mask & BR_MCAST_FLOOD) {
> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_MC_CTRL,
> +					 BIT(port),
> +					 (flags.val & BR_MCAST_FLOOD) ? BIT(port) : 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Enable/disable both types of unicast floods */
> +	if (flags.mask & BR_FLOOD) {
> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_UNDA_CTRL,
> +					 BIT(port),
> +					 (flags.val & BR_FLOOD) ? BIT(port) : 0);
> +		if (ret)
> +			return ret;
> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_UNMC_CTRL,
> +					 BIT(port),
> +					 (flags.val & BR_FLOOD) ? BIT(port) : 0);
> +		if (ret)
> +			return ret;

UNDA and UNMC?

> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.31.1
> 

Re: [PATCH net-next 4/5 v2] net: dsa: rtl8366rb: Support flood control

[Alvin Šipraga]

On 8/31/21 12:43 AM, Vladimir Oltean wrote:
> On Mon, Aug 30, 2021 at 11:48:58PM +0200, Linus Walleij wrote:
>> Now that we have implemented bridge flag handling we can easily
>> support flood (storm) control as well so let's do it.
>>
>> Cc: Vladimir Oltean <olteanv@gmail.com>
>> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
>> Cc: Mauri Sandberg <sandberg@mailfence.com>
>> Cc: DENG Qingfang <dqfext@gmail.com>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> ChangeLog v1->v2:
>> - New patch
>> ---
>>   drivers/net/dsa/rtl8366rb.c | 38 ++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
>> index 2cadd3e57e8b..4cb0e336ce6b 100644
>> --- a/drivers/net/dsa/rtl8366rb.c
>> +++ b/drivers/net/dsa/rtl8366rb.c
>> @@ -149,6 +149,11 @@
>>   
>>   #define RTL8366RB_VLAN_INGRESS_CTRL2_REG	0x037f
>>   
>> +#define RTL8366RB_STORM_BC_CTRL			0x03e0
>> +#define RTL8366RB_STORM_MC_CTRL			0x03e1
>> +#define RTL8366RB_STORM_UNDA_CTRL		0x03e2
>> +#define RTL8366RB_STORM_UNMC_CTRL		0x03e3
>> +
>>   /* LED control registers */
>>   #define RTL8366RB_LED_BLINKRATE_REG		0x0430
>>   #define RTL8366RB_LED_BLINKRATE_MASK		0x0007
>> @@ -1158,7 +1163,8 @@ rtl8366rb_port_pre_bridge_flags(struct dsa_switch *ds, int port,
>>   				struct netlink_ext_ack *extack)
>>   {
>>   	/* We support enabling/disabling learning */
>> -	if (flags.mask & ~(BR_LEARNING))
>> +	if (flags.mask & ~(BR_LEARNING | BR_BCAST_FLOOD |
>> +                           BR_MCAST_FLOOD | BR_FLOOD))
> 
> Spaces instead of tabs here?
> 
>>   		return -EINVAL;
>>   
>>   	return 0;
>> @@ -1180,6 +1186,36 @@ rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
>>   			return ret;
>>   	}
>>   
>> +	if (flags.mask & BR_BCAST_FLOOD) {
>> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_BC_CTRL,
>> +					 BIT(port),
>> +					 (flags.val & BR_BCAST_FLOOD) ? BIT(port) : 0);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	if (flags.mask & BR_MCAST_FLOOD) {
>> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_MC_CTRL,
>> +					 BIT(port),
>> +					 (flags.val & BR_MCAST_FLOOD) ? BIT(port) : 0);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	/* Enable/disable both types of unicast floods */
>> +	if (flags.mask & BR_FLOOD) {
>> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_UNDA_CTRL,
>> +					 BIT(port),
>> +					 (flags.val & BR_FLOOD) ? BIT(port) : 0);
>> +		if (ret)
>> +			return ret;
>> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_UNMC_CTRL,
>> +					 BIT(port),
>> +					 (flags.val & BR_FLOOD) ? BIT(port) : 0);
>> +		if (ret)
>> +			return ret;
> 
> UNDA and UNMC?

Ah, I was also fooled by this. UN is not unicast. It means "unknown 
destination address" and "unknown multicast address".

So, UNMC should go under BR_MCAST_FLOOD?

> 
>> +	}
>> +
>>   	return 0;
>>   }
>>   
>> -- 
>> 2.31.1
>>
> 

Re: [PATCH net-next 4/5 v2] net: dsa: rtl8366rb: Support flood control

[Alvin Šipraga]

On 8/30/21 11:48 PM, Linus Walleij wrote:
> Now that we have implemented bridge flag handling we can easily
> support flood (storm) control as well so let's do it.
> 
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

> ChangeLog v1->v2:
> - New patch
> ---
>   drivers/net/dsa/rtl8366rb.c | 38 ++++++++++++++++++++++++++++++++++++-
>   1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index 2cadd3e57e8b..4cb0e336ce6b 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -149,6 +149,11 @@
>   
>   #define RTL8366RB_VLAN_INGRESS_CTRL2_REG	0x037f
>   
> +#define RTL8366RB_STORM_BC_CTRL			0x03e0
> +#define RTL8366RB_STORM_MC_CTRL			0x03e1
> +#define RTL8366RB_STORM_UNDA_CTRL		0x03e2
> +#define RTL8366RB_STORM_UNMC_CTRL		0x03e3
> +
>   /* LED control registers */
>   #define RTL8366RB_LED_BLINKRATE_REG		0x0430
>   #define RTL8366RB_LED_BLINKRATE_MASK		0x0007
> @@ -1158,7 +1163,8 @@ rtl8366rb_port_pre_bridge_flags(struct dsa_switch *ds, int port,
>   				struct netlink_ext_ack *extack)
>   {
>   	/* We support enabling/disabling learning */

Perhaps you can remove this comment altogether, since we support more 
things now, and it is self evident anyway.

> -	if (flags.mask & ~(BR_LEARNING))
> +	if (flags.mask & ~(BR_LEARNING | BR_BCAST_FLOOD |
> +                           BR_MCAST_FLOOD | BR_FLOOD))
>   		return -EINVAL;
>   
>   	return 0;
> @@ -1180,6 +1186,36 @@ rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
>   			return ret;
>   	}
>   
> +	if (flags.mask & BR_BCAST_FLOOD) {
> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_BC_CTRL,
> +					 BIT(port),
> +					 (flags.val & BR_BCAST_FLOOD) ? BIT(port) : 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (flags.mask & BR_MCAST_FLOOD) {
> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_MC_CTRL,
> +					 BIT(port),
> +					 (flags.val & BR_MCAST_FLOOD) ? BIT(port) : 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Enable/disable both types of unicast floods */
> +	if (flags.mask & BR_FLOOD) {
> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_UNDA_CTRL,
> +					 BIT(port),
> +					 (flags.val & BR_FLOOD) ? BIT(port) : 0);
> +		if (ret)
> +			return ret;
> +		ret = regmap_update_bits(smi->map, RTL8366RB_STORM_UNMC_CTRL,
> +					 BIT(port),
> +					 (flags.val & BR_FLOOD) ? BIT(port) : 0);
> +		if (ret)
> +			return ret;
> +	}
> +
>   	return 0;
>   }
>   
> 

Re: [PATCH net-next 4/5 v2] net: dsa: rtl8366rb: Support flood control

[Florian Fainelli]

On 8/30/2021 2:48 PM, Linus Walleij wrote:
> Now that we have implemented bridge flag handling we can easily
> support flood (storm) control as well so let's do it.

storm control is usually defined by switch vendors about defining an 
acceptable bit rate for a certain type of traffic (unicast, multicast, 
broadcast) whereas the flags you are controlling here are about 
essential bridge functional, regardless of the bitrate, I would just 
drop that mention of (storm) altogether in your v3.
-- 
Florian

[PATCH net-next 5/5 v2] net: dsa: rtl8366rb: Support fast aging

[Linus Walleij]

This implements fast aging per-port using the special "security"
register, which will flush any L2 LUTs on a port.

Suggested-by: Vladimir Oltean <olteanv@gmail.com>
Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- New patch suggested by Vladimir.
---
 drivers/net/dsa/rtl8366rb.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index 4cb0e336ce6b..548282119cc4 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -1219,6 +1219,19 @@ rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static void
+rtl8366rb_port_fast_age(struct dsa_switch *ds, int port)
+{
+	struct realtek_smi *smi = ds->priv;
+
+	/* This will age out any L2 entries */
+	regmap_update_bits(smi->map, RTL8366RB_SECURITY_CTRL,
+			   BIT(port), BIT(port));
+	/* Restore the normal state of things */
+	regmap_update_bits(smi->map, RTL8366RB_SECURITY_CTRL,
+			   BIT(port), 0);
+}
+
 static int
 rtl8366rb_port_bridge_join(struct dsa_switch *ds, int port,
 			   struct net_device *bridge)
@@ -1673,6 +1686,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_fast_age = rtl8366rb_port_fast_age,
 	.port_change_mtu = rtl8366rb_change_mtu,
 	.port_max_mtu = rtl8366rb_max_mtu,
 };
-- 
2.31.1

Re: [PATCH net-next 5/5 v2] net: dsa: rtl8366rb: Support fast aging

[Vladimir Oltean]

On Mon, Aug 30, 2021 at 11:48:59PM +0200, Linus Walleij wrote:
> This implements fast aging per-port using the special "security"
> register, which will flush any L2 LUTs on a port.
> 
> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - New patch suggested by Vladimir.
> ---
>  drivers/net/dsa/rtl8366rb.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index 4cb0e336ce6b..548282119cc4 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -1219,6 +1219,19 @@ rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
>  	return 0;
>  }
>  
> +static void
> +rtl8366rb_port_fast_age(struct dsa_switch *ds, int port)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +
> +	/* This will age out any L2 entries */

Clarify "any L2 entries". The fdb flushing process should remove the
dynamically learned FDB entries, it should keep the static ones. Did you
say "any" because rtl8366rb does not implement static FDB entries via
.port_fdb_add, and therefore all entries are dynamic, or does it really
delete static FDB entries?

> +	regmap_update_bits(smi->map, RTL8366RB_SECURITY_CTRL,
> +			   BIT(port), BIT(port));
> +	/* Restore the normal state of things */
> +	regmap_update_bits(smi->map, RTL8366RB_SECURITY_CTRL,
> +			   BIT(port), 0);
> +}
> +
>  static int
>  rtl8366rb_port_bridge_join(struct dsa_switch *ds, int port,
>  			   struct net_device *bridge)
> @@ -1673,6 +1686,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_fast_age = rtl8366rb_port_fast_age,
>  	.port_change_mtu = rtl8366rb_change_mtu,
>  	.port_max_mtu = rtl8366rb_max_mtu,
>  };
> -- 
> 2.31.1
> 

Re: [PATCH net-next 5/5 v2] net: dsa: rtl8366rb: Support fast aging

[Linus Walleij]

On Tue, Aug 31, 2021 at 12:46 AM Vladimir Oltean <olteanv@gmail.com> wrote:

> > +     /* This will age out any L2 entries */
>
> Clarify "any L2 entries". The fdb flushing process should remove the
> dynamically learned FDB entries, it should keep the static ones. Did you
> say "any" because rtl8366rb does not implement static FDB entries via
> .port_fdb_add, and therefore all entries are dynamic, or does it really
> delete static FDB entries?

It's what Realtek calls "L2 entries" sadly I do not fully understand
their lingo.

The ASIC can do static L2 entries as well, but I haven't looked into
that. The (confused) docs for the function that set these bits is
the following:

"ASIC will age out L2 entry with fields Static, Auth and IP_MULT are 0.
 Aging out function is for new address learning LUT update because
 size of LUT is limited, old address information should not be kept and
 get more new learning SA information. Age field of L2 LUT is updated
 by following sequence {0b10,0b11,0b01,0b00} which means the LUT
 entries with age value 0b00 is free for ASIC. ASIC will use this aging
 sequence to decide which entry to be replace by new SA learning
 information. This function can be replace by setting STP state each
 port."

Next it sets the bit for the port in register
RTL8366RB_SECURITY_CTRL.

Realtek talks about "LUT" which I think is the same as "FDB"
(which I assume is forwarding database, I'm not good with this stuff).

My interpretation of this convoluted text is that static, auth and ip_mult
will *not* be affected ("are 0"), but only learned entries in the LUT/FDB
will be affected. The sequence listed in the comment I interpret as a
reference to what the ASIC is doing with the age field for the entry
internally to achieve this. Then I guess they say that one can also
do fast aging by stopping the port (duh).

I'll update the doc to say "any learned L2 entries", but eager to hear
what you say about it too :)

Yours,
Linus Walleij

Re: [PATCH net-next 5/5 v2] net: dsa: rtl8366rb: Support fast aging

[Vladimir Oltean]

On Tue, Sep 07, 2021 at 07:48:43PM +0200, Linus Walleij wrote:
> On Tue, Aug 31, 2021 at 12:46 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> > > +     /* This will age out any L2 entries */
> >
> > Clarify "any L2 entries". The fdb flushing process should remove the
> > dynamically learned FDB entries, it should keep the static ones. Did you
> > say "any" because rtl8366rb does not implement static FDB entries via
> > .port_fdb_add, and therefore all entries are dynamic, or does it really
> > delete static FDB entries?
> 
> It's what Realtek calls "L2 entries" sadly I do not fully understand
> their lingo.
> 
> The ASIC can do static L2 entries as well, but I haven't looked into
> that. The (confused) docs for the function that set these bits is
> the following:
> 
> "ASIC will age out L2 entry with fields Static, Auth and IP_MULT are 0.
>  Aging out function is for new address learning LUT update because
>  size of LUT is limited, old address information should not be kept and
>  get more new learning SA information. Age field of L2 LUT is updated
>  by following sequence {0b10,0b11,0b01,0b00} which means the LUT

This is {3, 2, 1, 0} in 2-bit Gray code, really curious why they went
for that coding scheme. It is common to designate the states of an FSM
in Gray code because of the single bit change required on state
transitions, but in this case, every ageing state should have a
transition path back to 3 when a packet with that {MAC DA, VLAN ID} is
received on the port, and the L2 entry is refreshed. The Gray code
is a micro-optimization that doesn't seem to help for the primary state
transition there. Anyway, doesn't make a difference.

>  entries with age value 0b00 is free for ASIC. ASIC will use this aging
>  sequence to decide which entry to be replace by new SA learning
>  information. This function can be replace by setting STP state each
>  port."
> 
> Next it sets the bit for the port in register
> RTL8366RB_SECURITY_CTRL.
> 
> Realtek talks about "LUT" which I think is the same as "FDB"
> (which I assume is forwarding database, I'm not good with this stuff).

LUT is "look-up table", any piece of memory which translates between a
key and a value is a look-up table. In this case, the forwarding database
would qualify as a look-up table where the key is the {MAC DA, VLAN ID}
tuple, and the value is the destination port mask.

> My interpretation of this convoluted text is that static, auth and ip_mult
> will *not* be affected ("are 0"), but only learned entries in the LUT/FDB
> will be affected.

Same interpretation here. This behavior is to be expected.

> The sequence listed in the comment I interpret as a reference to what
> the ASIC is doing with the age field for the entry internally to
> achieve this. Then I guess they say that one can also do fast aging by
> stopping the port (duh).
> 
> I'll update the doc to say "any learned L2 entries", but eager to hear
> what you say about it too :)

Your interpretation seems correct (I can't think of anything else being meant),
but I don't know why you say "duh" about the update of STP state
resulting in the port losing its dynamic L2 entries. Sure, it makes
sense, but many other vendors do not do that automatically, and DSA
calls .port_fast_age whenever the STP state transitions from a value
capable of learning (LEARNING, FORWARDING) to one incapable of learning
(DISABLED, BLOCKING, LISTENING).

To prove/disprove, it would be interesting to implement port STP states,
without implementing .port_fast_age, force a port down and then back up,
and then run "bridge fdb" and see whether it is true that STP state
changes also lead to FDB flushing but at a hardware level (whether there
is any 'self' entry being reported).

By this logic, the hardware should also auto-age its dynamically learned
L2 entries when address learning is turned off, and there would be no
purpose at all in implementing the .port_fast_age method. Also curious
if that is the case.

Re: [PATCH net-next 5/5 v2] net: dsa: rtl8366rb: Support fast aging

[Linus Walleij]

On Wed, Sep 8, 2021 at 10:10 PM Vladimir Oltean <olteanv@gmail.com> wrote:

> Your interpretation seems correct (I can't think of anything else being meant),
> but I don't know why you say "duh" about the update of STP state
> resulting in the port losing its dynamic L2 entries. Sure, it makes
> sense, but many other vendors do not do that automatically, and DSA
> calls .port_fast_age whenever the STP state transitions from a value
> capable of learning (LEARNING, FORWARDING) to one incapable of learning
> (DISABLED, BLOCKING, LISTENING).
>
> To prove/disprove, it would be interesting to implement port STP states,
> without implementing .port_fast_age, force a port down and then back up,
> and then run "bridge fdb" and see whether it is true that STP state
> changes also lead to FDB flushing but at a hardware level (whether there
> is any 'self' entry being reported).

I have been looking into this.

What makes RTL8366RB so confusing is a compulsive use of FIDs.

For example Linux DSA has:

ds->ops->port_stp_state_set(ds, port, state);

This is pretty straight forward. The vendor RTL8366RB API however has this:

int32 rtl8368s_setAsicSpanningTreeStatus(enum PORTID port, enum
FIDVALUE fid, enum SPTSTATE state)

So this is set per FID instead of per VID.

I also looked into proper FDB support and there is the same problem.
For example I want to implement:

static int rtl8366rb_port_fdb_add(struct dsa_switch *ds, int port,
                  const unsigned char *addr, u16 vid)

But the FDB static (also autolearn) entries has this format:

struct l2tb_macstatic_st{
    ether_addr_t     mac;
    uint16     fid:3;
    uint16     reserved1:13;
    uint16     mbr:8;
    uint16     reserved2:4;
    uint16    block:1;
    uint16     auth:1;
    uint16     swst:1;
    uint16     ipmulti:1;
    uint16     reserved3;
};

(swst indicates a static entry ipmulti a multicast entry, mbr is apparently
for S-VLAN, which I'm not familiar with.)

So again using a FID rather than port or VID in the database.

I am starting to wonder whether I should just associate 1-1 the FID:s
with the 6 ports (0..5) to simplify things. Or one FID per defined VID
until they run out, as atleast OpenWRT connects VLAN1 to all ports on
a bridge.

Yours,
Linus Walleij

Re: [PATCH net-next 5/5 v2] net: dsa: rtl8366rb: Support fast aging

[Vladimir Oltean]

On Fri, Sep 10, 2021 at 03:59:18PM +0200, Linus Walleij wrote:
> On Wed, Sep 8, 2021 at 10:10 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> > Your interpretation seems correct (I can't think of anything else being meant),
> > but I don't know why you say "duh" about the update of STP state
> > resulting in the port losing its dynamic L2 entries. Sure, it makes
> > sense, but many other vendors do not do that automatically, and DSA
> > calls .port_fast_age whenever the STP state transitions from a value
> > capable of learning (LEARNING, FORWARDING) to one incapable of learning
> > (DISABLED, BLOCKING, LISTENING).
> >
> > To prove/disprove, it would be interesting to implement port STP states,
> > without implementing .port_fast_age, force a port down and then back up,
> > and then run "bridge fdb" and see whether it is true that STP state
> > changes also lead to FDB flushing but at a hardware level (whether there
> > is any 'self' entry being reported).
>
> I have been looking into this.
>
> What makes RTL8366RB so confusing is a compulsive use of FIDs.

It's not confusing, it's great, and I'm glad that you're around and
willing to spend some time so we can discuss this.

> For example Linux DSA has:
>
> ds->ops->port_stp_state_set(ds, port, state);
>
> This is pretty straight forward. The vendor RTL8366RB API however has this:
>
> int32 rtl8368s_setAsicSpanningTreeStatus(enum PORTID port, enum
> FIDVALUE fid, enum SPTSTATE state)
>
> So this is set per FID instead of per VID.

It's per {port,FID} actually. It is useful in case you want to support
MSTP (STP per VLAN, your port is BLOCKING in one VLAN but FORWARDING in
another)

> I also looked into proper FDB support and there is the same problem.
> For example I want to implement:
>
> static int rtl8366rb_port_fdb_add(struct dsa_switch *ds, int port,
>                   const unsigned char *addr, u16 vid)
>
> But the FDB static (also autolearn) entries has this format:
>
> struct l2tb_macstatic_st{
>     ether_addr_t     mac;
>     uint16     fid:3;
>     uint16     reserved1:13;
>     uint16     mbr:8;
>     uint16     reserved2:4;
>     uint16    block:1;
>     uint16     auth:1;
>     uint16     swst:1;
>     uint16     ipmulti:1;
>     uint16     reserved3;
> };
>
> (swst indicates a static entry ipmulti a multicast entry, mbr is apparently
> for S-VLAN, which I'm not familiar with.)
>
> So again using a FID rather than port or VID in the database.
>
> I am starting to wonder whether I should just associate 1-1 the FID:s
> with the 6 ports (0..5) to simplify things. Or one FID per defined VID
> until they run out, as atleast OpenWRT connects VLAN1 to all ports on
> a bridge.

No, don't rush it/hack it. A FID is basically the domain in which your
L2 lookup table will find a destination MAC address.

When every port has a unique FID and they are under the same bridge, one
port will never be able to find an L2 lookup table entry learned on
another port, because that other port has learned it in another FID.
That's not what you want.

On the other hand, when a port is standalone, you very much want packets
received from that port to go _only_ towards the CPU [ via flooding ],
and have no temptation at all to even try to forward that packet towards
another switch port. So standalone ports should not have the same FID as
ports that are in a bridge.

As for bridges, what FID to use?

Well, in the case of a VLAN-unaware bridge, you can configure all ports
of that bridge to use the same FID. So the FDB lookup will find entries
learned on ports that are members of the same bridge, but not entries
learned on ports that are members of other bridges.

In the case of a VLAN-aware bridge, you have two choices.

You can do the same thing as in the case of VLAN-unaware bridges, and
this will turn your switch into an 802.1Q bridge with Shared VLAN
Learning (SVL). This means that a packet will effectively be forwarded
only based on MAC DA, the VLAN ID will be ignored when performing the
FDB lookup (because the FDB lookup in your hw is actually per FID, and
we made a 1:1 association between the FID and the _bridge_, not the
_bridge_VLAN_). So you can never have two stations with the same MAC
address in different VLANs, that would confuse your hardware.

The second approach is to associate every {bridge, VLAN} pair with a
different hardware FID. This will turn your switch into an 802.1Q
Independent VLAN Learning (IVL) bridge, so you can have multiple
stations attached to the same bridge, having the same MAC address, but
they are in different VLANs, so your hardware will keep separate FDB
entries for them and they will not overlap (if your bridge was SVL,
there would be a single FDB entry for both stations, and it would bounce
from one port to another).

So in both cases, VLAN 1 of bridge br0 maps to a different FID compared
to VLAN 1 of bridge br1. That is the point. The difference is that:

- with SVL, VLAN 1, 2, 3, 4 .... of br0 all map to the same FID.
- with IVL, VLAN 1, 2, 3, 4 ... of br0 map to different FIDs.

The obvious limitation to the second approach is the number of VLANs you
can support, it is limited by the number of hardware FIDs. In your
switch, that seems to be 8, so not a lot.

So IMO, I would go for a SVL approach.

What I would do is start with these patches:
https://patchwork.kernel.org/project/netdevbpf/cover/20210818120150.892647-1-vladimir.oltean@nxp.com/

I will resubmit them (actually slightly modified variants) during the
first weeks of the next kernel development cycle.

Check out what changes in the .port_fdb_add function prototype, and see
how you can map that new "bridge_num" argument to a FID. Also check out
how other drivers make use of FIDs (search for FID_STANDALONE and
FID_BRIDGED in the mt7530 driver). That driver only performs FDB
isolation between standalone ports and bridged ports, but not between
one bridge and another. So the FIDs are underutilized.

Re: [PATCH net-next 5/5 v2] net: dsa: rtl8366rb: Support fast aging

[Alvin Šipraga]

On 8/30/21 11:48 PM, Linus Walleij wrote:
> This implements fast aging per-port using the special "security"
> register, which will flush any L2 LUTs on a port.
> 
> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> Cc: Alvin Šipraga <alsi@bang-olufsen.dk>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Cc: DENG Qingfang <dqfext@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

Vladimir is probably right about "any" being a bit misleading, but I 
think this is doing the right thing. Fixed LUT entries (nonexistent at 
the moment) should not be affected by the aging process.

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

> ChangeLog v1->v2:
> - New patch suggested by Vladimir.
> ---
>   drivers/net/dsa/rtl8366rb.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index 4cb0e336ce6b..548282119cc4 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -1219,6 +1219,19 @@ rtl8366rb_port_bridge_flags(struct dsa_switch *ds, int port,
>   	return 0;
>   }
>   
> +static void
> +rtl8366rb_port_fast_age(struct dsa_switch *ds, int port)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +
> +	/* This will age out any L2 entries */
> +	regmap_update_bits(smi->map, RTL8366RB_SECURITY_CTRL,
> +			   BIT(port), BIT(port));
> +	/* Restore the normal state of things */
> +	regmap_update_bits(smi->map, RTL8366RB_SECURITY_CTRL,
> +			   BIT(port), 0);
> +}
> +
>   static int
>   rtl8366rb_port_bridge_join(struct dsa_switch *ds, int port,
>   			   struct net_device *bridge)
> @@ -1673,6 +1686,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_fast_age = rtl8366rb_port_fast_age,
>   	.port_change_mtu = rtl8366rb_change_mtu,
>   	.port_max_mtu = rtl8366rb_max_mtu,
>   };
>