Date:   Mon, 30 Aug 2021 23:22:11 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Vladimir Oltean <olteanv@...il.com>
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>,
        DENG Qingfang <dqfext@...il.com>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Mauri Sandberg <sandberg@...lfence.com>
Subject: Re: [PATCH net-next 1/2] net: dsa: rtl8366rb: support bridge offloading

On Mon, Aug 30, 2021 at 10:12 AM Vladimir Oltean <olteanv@...il.com> wrote:

> > +/* 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)
>
> If RTL8366RB_NUM_PORTS is 6, then why is RTL8366RB_PORT_ISO_PORTS_MASK a
> 7-bit field?

It's a 6 bit field actually from bit 1 to bit 7 just shifted up one
bit because bit 0 is "enable".

> > +     /* 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 |
> > +                                BIT(RTL8366RB_PORT_NUM_CPU + 1));
>
> The shifting due to RTL8366RB_PORT_ISO_EN looks weird, I can see it
> being mishandled in the future, with code moved around, copied and
> pasted between realtek drivers and such. How about making a macro
>
> #define RTL8366RB_PORT_ISO_PORTS(x)     ((x) << 1)

OK

> > +     /* CPU port can access all ports */
>
> Except itself maybe? RTL8366RB_PORT_NUM_CPU is 5, so maybe use something
> like
>
> RTL8366RB_PORT_ISO_PORTS(dsa_user_ports(ds))

Tested this and it appears to work just fine!

> Looks okay for the most part. It is to be expected for a new driver that
> introduces bridging offload to also handle .port_pre_bridge_flags,
> .port_bridge_flags and .port_fast_age, for two reasons:
> (a) it is expected that a port which does not offload the bridge, and
>     performs forwarding in software, to not perform address learning in
>     hardware
> (b) it is expected that the addresses learned while the port was under a
>     bridge are not carried over into its life as a standalone port, when
>     it leaves that bridge

I studied the vendor code drop and register file and implemented
the BR_LEARNING flag, and I also managed to implement fast aging.
Each as a separate patch. Thanks for pointing this out!

> Also, it would be nice if you could do some minimal isolation at the
> level of the FDB lookup. Currently, if I am not mistaken, a port will
> perform FDB lookup even if it is standalone, and it might find an FDB
> entry for a given {MAC DA, VLAN ID} pair that belongs to a port outside
> of its isolation mask, so forwarding will be blocked and that packet
> will be dropped (instead of the expected behavior which is for that
> packet to be forwarded to the CPU).
>
> Normally the expectation is that this FDB-level isolation can be achieved
> by configuring the VLANs of one bridge to use a filter ID that is
> different from the VLANs of another bridge, and the port-based default
> VLAN of standalone ports to use yet another filter ID. This is yet
> another reason to disable learning on standalone ports, so that their
> filter ID never contains any FDB entry, and packets are always flooded
> to their only possible destination, the CPU port.
>
> Currently in DSA we do not offer a streamlined way for you to determine
> what filter ID to use for a certain VLAN belonging to a certain bridge,
> but at the very least you can test FDB isolation between standalone
> ports and bridged ports. The simplest way to do that, assuming you
> already have a forwarding setup with 2 switch ports swp0 and swp1, is to
> enable CONFIG_BONDING=y, and then:
>
> ip link add br0 type bridge
> ip link set bond0 master br0
> ip link set swp1 master bond0
> ip link set swp0 master br0
>
> Then ping between station A attached to swp0 and station B attached to
> swp1.
>
> Because swp1 cannot offload bond0, it will fall back to software
> forwarding and act as standalone, i.e. what you had up till now.
> With hardware address learning enabled on swp0 (a port that offloads
> br0), it will learn station A's source MAC address. Then when swp1 needs
> to send a packet to station A's destination MAC address, it would be
> tempted to look up the FDB, find that address, and forward to swp0. But
> swp0 is isolated from swp1. If you use a filter ID for standalone ports
> and another filter ID for bridged ports you will avoid that problem, and
> you will also lay the groundwork for the full FDB isolation even between
> bridges that will be coming during the next development cycle.
>
> If you feel that the second part is too much for now, you can just add
> the extra callbacks for address learning and flushing (although I do
> have some genuine concerns about how reliable was the software forwarding
> with this driver, seeing that right now it enables hardware learning
> unconditionally). Is there something that isolates FDB lookups already?

Ugh that was massive, I'm not that smart ;)

I kinda understand it but have no idea how to achieve this with
the current hardware, driver and vendor code mess.

I prefer to fix the first part for now.

Yours,
Linus Walleij

Powered by blists - more mailing lists