Message-ID: <20210910175724.tc2tyxihsetvvsjd@skbuf>
Date:   Fri, 10 Sep 2021 20:57:24 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        netdev <netdev@...r.kernel.org>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Mauri Sandberg <sandberg@...lfence.com>,
        DENG Qingfang <dqfext@...il.com>
Subject: Re: [PATCH net-next 5/5 v2] net: dsa: rtl8366rb: Support fast aging

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@...il.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.

Powered by blists - more mailing lists