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

Commit Message

Linus Walleij Aug. 30, 2021, 9:48 p.m. UTC

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 <[e-mail]>
Cc: Alvin Šipraga <[e-mail]>
Cc: Mauri Sandberg <[e-mail]>
Cc: DENG Qingfang <[e-mail]>
Signed-off-by: Linus Walleij <[e-mail]>
---
ChangeLog v1->v2:
- New patch suggested by Vladimir.
---
 drivers/net/dsa/rtl8366rb.c | 47 +++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 5 deletions(-)

Comments

Vladimir Oltean Aug. 30, 2021, 10:40 p.m. UTC | #1

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 <[e-mail]>
> Cc: Alvin Šipraga <[e-mail]>
> Cc: Mauri Sandberg <[e-mail]>
> Cc: DENG Qingfang <[e-mail]>
> Signed-off-by: Linus Walleij <[e-mail]>
> ---
> 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
>

Linus Walleij Sept. 7, 2021, 3:52 p.m. UTC | #2

On Tue, Aug 31, 2021 at 12:40 AM Vladimir Oltean <[e-mail]> 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

Vladimir Oltean Sept. 8, 2021, 9:09 p.m. UTC | #3

On Tue, Sep 07, 2021 at 05:52:01PM +0200, Linus Walleij wrote:
> On Tue, Aug 31, 2021 at 12:40 AM Vladimir Oltean <[e-mail]> 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

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,
 };