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

Commit Message

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

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

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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

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

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

Alvin Šipraga Aug. 31, 2021, 12:24 a.m. UTC | #2

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

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 <[e-mail]>

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

Linus Walleij Sept. 7, 2021, 5:48 p.m. UTC | #3

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

Vladimir Oltean Sept. 8, 2021, 8:10 p.m. UTC | #4

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

Linus Walleij Sept. 10, 2021, 1:59 p.m. UTC | #5

On Wed, Sep 8, 2021 at 10:10 PM Vladimir Oltean <[e-mail]> 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

Vladimir Oltean Sept. 10, 2021, 5:57 p.m. UTC | #6

On Fri, Sep 10, 2021 at 03:59:18PM +0200, Linus Walleij wrote:
> On Wed, Sep 8, 2021 at 10:10 PM Vladimir Oltean <[e-mail]> 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/[e-mail]/

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.

Patch

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