net: dsa: lantiq_gswip: getting the first selftests to pass

net: dsa: lantiq_gswip: getting the first selftests to pass

[Martin Blumenstingl]

Hello,

there are some pending issues with the Lantiq GSWIP driver.
Vladimir suggested to get the kernel selftests to pass in a first step.
I am starting with
tools/testing/selftests/drivers/net/dsa/local_termination.sh as my
understanding is that this contains the most basic tests and should be
the first step.

The good news is that not all tests are broken!
There are eight tests which are not passing. Those eight can be split
into two groups of four, because it's the same four tests that are
failing for "standalone" and "bridge" interfaces:
- Unicast IPv4 to unknown MAC address
- Unicast IPv4 to unknown MAC address, allmulti
- Multicast IPv4 to unknown group
- Multicast IPv6 to unknown group

What they all have in common is the fact that we're expecting that no
packets are received. But in reality packets are received. I manually
confirmed this by examining the tcpdump file which is generated by the
selftests.

Vladimir suggested in [0]:
> [...] we'll need to make smaller steps, like disable address
> learning on standalone ports, isolate FDBs, maybe offload the bridge TX
> forwarding process (in order to populate the "Force no learning" bit in
> tag_gswip.c properly), and only then will the local_termination test
> also pass [...]

Based on the failing tests I am wondering which step would be a good
one to start with.
Is this problem that the selftests are seeing a flooding issue? In
that case I suspect that the "interesting behavior" (of the GSWIP's
flooding behavior) that Vladimir described in [1] would be a starting
point.

Full local_termination.sh selftest output:
TEST: lan2: Unicast IPv4 to primary MAC address                 [ OK ]
TEST: lan2: Unicast IPv4 to macvlan MAC address                 [ OK ]
TEST: lan2: Unicast IPv4 to unknown MAC address                 [FAIL]
        reception succeeded, but should have failed
TEST: lan2: Unicast IPv4 to unknown MAC address, promisc        [ OK ]
TEST: lan2: Unicast IPv4 to unknown MAC address, allmulti       [FAIL]
        reception succeeded, but should have failed
TEST: lan2: Multicast IPv4 to joined group                      [ OK ]
TEST: lan2: Multicast IPv4 to unknown group                     [FAIL]
        reception succeeded, but should have failed
TEST: lan2: Multicast IPv4 to unknown group, promisc            [ OK ]
TEST: lan2: Multicast IPv4 to unknown group, allmulti           [ OK ]
TEST: lan2: Multicast IPv6 to joined group                      [ OK ]
TEST: lan2: Multicast IPv6 to unknown group                     [FAIL]
        reception succeeded, but should have failed
TEST: lan2: Multicast IPv6 to unknown group, promisc            [ OK ]
TEST: lan2: Multicast IPv6 to unknown group, allmulti           [ OK ]
TEST: br0: Unicast IPv4 to primary MAC address                  [ OK ]
TEST: br0: Unicast IPv4 to macvlan MAC address                  [ OK ]
TEST: br0: Unicast IPv4 to unknown MAC address                  [FAIL]
        reception succeeded, but should have failed
TEST: br0: Unicast IPv4 to unknown MAC address, promisc         [ OK ]
TEST: br0: Unicast IPv4 to unknown MAC address, allmulti        [FAIL]
        reception succeeded, but should have failed
TEST: br0: Multicast IPv4 to joined group                       [ OK ]
TEST: br0: Multicast IPv4 to unknown group                      [FAIL]
        reception succeeded, but should have failed
TEST: br0: Multicast IPv4 to unknown group, promisc             [ OK ]
TEST: br0: Multicast IPv4 to unknown group, allmulti            [ OK ]
TEST: br0: Multicast IPv6 to joined group                       [ OK ]
TEST: br0: Multicast IPv6 to unknown group                      [FAIL]
        reception succeeded, but should have failed
TEST: br0: Multicast IPv6 to unknown group, promisc             [ OK ]
TEST: br0: Multicast IPv6 to unknown group, allmulti            [ OK ]


Thank you!
Best regards,
Martin

[0] https://lore.kernel.org/netdev/20220706210651.ozvjcwwp2hquzmhn@skbuf/
[1] https://lore.kernel.org/netdev/20220702185652.dpzrxuitacqp6m3t@skbuf/

Re: net: dsa: lantiq_gswip: getting the first selftests to pass

[Florian Fainelli]

On 7/27/22 13:36, Martin Blumenstingl wrote:
> Hello,
> 
> there are some pending issues with the Lantiq GSWIP driver.
> Vladimir suggested to get the kernel selftests to pass in a first step.
> I am starting with
> tools/testing/selftests/drivers/net/dsa/local_termination.sh as my
> understanding is that this contains the most basic tests and should be
> the first step.

Since I am in the process of re-designing my test rack at home with DSA devices, how do you run the selftests out of curiosity? Is there a nice diagram that explains how to get a physical connection set-up?

I used to have between 2 and 4 Ethernet controllers dedicated to each port of the switch of the DUT so I could run bridge/standalone/bandwidth testing but I feel like this is a tad extreme and am cutting down on the number of Ethernet ports so I can put NVMe drives in the machine instead.

Thanks!

> 
> The good news is that not all tests are broken!
> There are eight tests which are not passing. Those eight can be split
> into two groups of four, because it's the same four tests that are
> failing for "standalone" and "bridge" interfaces:
> - Unicast IPv4 to unknown MAC address
> - Unicast IPv4 to unknown MAC address, allmulti
> - Multicast IPv4 to unknown group
> - Multicast IPv6 to unknown group
> 
> What they all have in common is the fact that we're expecting that no
> packets are received. But in reality packets are received. I manually
> confirmed this by examining the tcpdump file which is generated by the
> selftests.
> 
> Vladimir suggested in [0]:
>> [...] we'll need to make smaller steps, like disable address
>> learning on standalone ports, isolate FDBs, maybe offload the bridge TX
>> forwarding process (in order to populate the "Force no learning" bit in
>> tag_gswip.c properly), and only then will the local_termination test
>> also pass [...]
> 
> Based on the failing tests I am wondering which step would be a good
> one to start with.
> Is this problem that the selftests are seeing a flooding issue? In
> that case I suspect that the "interesting behavior" (of the GSWIP's
> flooding behavior) that Vladimir described in [1] would be a starting
> point.
> 
> Full local_termination.sh selftest output:
> TEST: lan2: Unicast IPv4 to primary MAC address                 [ OK ]
> TEST: lan2: Unicast IPv4 to macvlan MAC address                 [ OK ]
> TEST: lan2: Unicast IPv4 to unknown MAC address                 [FAIL]
>         reception succeeded, but should have failed
> TEST: lan2: Unicast IPv4 to unknown MAC address, promisc        [ OK ]
> TEST: lan2: Unicast IPv4 to unknown MAC address, allmulti       [FAIL]
>         reception succeeded, but should have failed
> TEST: lan2: Multicast IPv4 to joined group                      [ OK ]
> TEST: lan2: Multicast IPv4 to unknown group                     [FAIL]
>         reception succeeded, but should have failed
> TEST: lan2: Multicast IPv4 to unknown group, promisc            [ OK ]
> TEST: lan2: Multicast IPv4 to unknown group, allmulti           [ OK ]
> TEST: lan2: Multicast IPv6 to joined group                      [ OK ]
> TEST: lan2: Multicast IPv6 to unknown group                     [FAIL]
>         reception succeeded, but should have failed
> TEST: lan2: Multicast IPv6 to unknown group, promisc            [ OK ]
> TEST: lan2: Multicast IPv6 to unknown group, allmulti           [ OK ]
> TEST: br0: Unicast IPv4 to primary MAC address                  [ OK ]
> TEST: br0: Unicast IPv4 to macvlan MAC address                  [ OK ]
> TEST: br0: Unicast IPv4 to unknown MAC address                  [FAIL]
>         reception succeeded, but should have failed
> TEST: br0: Unicast IPv4 to unknown MAC address, promisc         [ OK ]
> TEST: br0: Unicast IPv4 to unknown MAC address, allmulti        [FAIL]
>         reception succeeded, but should have failed
> TEST: br0: Multicast IPv4 to joined group                       [ OK ]
> TEST: br0: Multicast IPv4 to unknown group                      [FAIL]
>         reception succeeded, but should have failed
> TEST: br0: Multicast IPv4 to unknown group, promisc             [ OK ]
> TEST: br0: Multicast IPv4 to unknown group, allmulti            [ OK ]
> TEST: br0: Multicast IPv6 to joined group                       [ OK ]
> TEST: br0: Multicast IPv6 to unknown group                      [FAIL]
>         reception succeeded, but should have failed
> TEST: br0: Multicast IPv6 to unknown group, promisc             [ OK ]
> TEST: br0: Multicast IPv6 to unknown group, allmulti            [ OK ]
> 
> 
> Thank you!
> Best regards,
> Martin
> 
> [0] https://lore.kernel.org/netdev/20220706210651.ozvjcwwp2hquzmhn@skbuf/
> [1] https://lore.kernel.org/netdev/20220702185652.dpzrxuitacqp6m3t@skbuf/


-- 
Florian

Re: net: dsa: lantiq_gswip: getting the first selftests to pass

[Vladimir Oltean]

On Wed, Jul 27, 2022 at 02:07:51PM -0700, Florian Fainelli wrote:
> Since I am in the process of re-designing my test rack at home with
> DSA devices, how do you run the selftests out of curiosity? Is there a
> nice diagram that explains how to get a physical connection set-up?
> 
> I used to have between 2 and 4 Ethernet controllers dedicated to each
> port of the switch of the DUT so I could run
> bridge/standalone/bandwidth testing but I feel like this is a tad
> extreme and am cutting down on the number of Ethernet ports so I can
> put NVMe drives in the machine instead.

tools/testing/selftests/net/forwarding/README

                             br0
                              +
               vrf-h1         |           vrf-h2
                 +        +---+----+        +
                 |        |        |        |
    192.0.2.1/24 +        +        +        + 192.0.2.2/24
               swp1     swp2     swp3     swp4
                 +        +        +        +
                 |        |        |        |
                 +--------+        +--------+

Most of the tests assume these 4 ports, otherwise the topology is
mentioned in a drawing for that particular selftest.

The names used by the tests are actually $h1 and $h2 for the host ports
(extreme left and extreme right) - these terminate traffic - and $swp1
and $swp2 (mid left and mid right) - these forward traffic. In the
drawing from the README, I suppose the names "swp1 ... swp4" were used
to illustrate that you can use switch net devices as host ports, and
also as forwarding ports.

Re: net: dsa: lantiq_gswip: getting the first selftests to pass

[Florian Fainelli]

On 7/27/22 17:02, Vladimir Oltean wrote:
> On Wed, Jul 27, 2022 at 02:07:51PM -0700, Florian Fainelli wrote:
>> Since I am in the process of re-designing my test rack at home with
>> DSA devices, how do you run the selftests out of curiosity? Is there a
>> nice diagram that explains how to get a physical connection set-up?
>>
>> I used to have between 2 and 4 Ethernet controllers dedicated to each
>> port of the switch of the DUT so I could run
>> bridge/standalone/bandwidth testing but I feel like this is a tad
>> extreme and am cutting down on the number of Ethernet ports so I can
>> put NVMe drives in the machine instead.
> 
> tools/testing/selftests/net/forwarding/README
> 
>                              br0
>                               +
>                vrf-h1         |           vrf-h2
>                  +        +---+----+        +
>                  |        |        |        |
>     192.0.2.1/24 +        +        +        + 192.0.2.2/24
>                swp1     swp2     swp3     swp4
>                  +        +        +        +
>                  |        |        |        |
>                  +--------+        +--------+
> 
> Most of the tests assume these 4 ports, otherwise the topology is
> mentioned in a drawing for that particular selftest.
> 
> The names used by the tests are actually $h1 and $h2 for the host ports
> (extreme left and extreme right) - these terminate traffic - and $swp1
> and $swp2 (mid left and mid right) - these forward traffic. In the
> drawing from the README, I suppose the names "swp1 ... swp4" were used
> to illustrate that you can use switch net devices as host ports, and
> also as forwarding ports.

Thanks! I will get to that later this week.
-- 
Florian

Re: net: dsa: lantiq_gswip: getting the first selftests to pass

[Vladimir Oltean]

Hi Martin,

On Wed, Jul 27, 2022 at 10:36:55PM +0200, Martin Blumenstingl wrote:
> Hello,
> 
> there are some pending issues with the Lantiq GSWIP driver.
> Vladimir suggested to get the kernel selftests to pass in a first step.
> I am starting with
> tools/testing/selftests/drivers/net/dsa/local_termination.sh as my
> understanding is that this contains the most basic tests and should be
> the first step.

I don't think I've said local_termination.sh contains the most basic
tests. Some tests are basic, but not all are.

> The good news is that not all tests are broken!

Looking at the tests which fail, I think the gswip driver is in a pretty
good state functionally speaking.

> There are eight tests which are not passing. Those eight can be split
> into two groups of four, because it's the same four tests that are
> failing for "standalone" and "bridge" interfaces:
> - Unicast IPv4 to unknown MAC address
> - Unicast IPv4 to unknown MAC address, allmulti
> - Multicast IPv4 to unknown group
> - Multicast IPv6 to unknown group
> 
> What they all have in common is the fact that we're expecting that no
> packets are received. But in reality packets are received. I manually
> confirmed this by examining the tcpdump file which is generated by the
> selftests.
> 
> Vladimir suggested in [0]:
> > [...] we'll need to make smaller steps, like disable address
> > learning on standalone ports, isolate FDBs, maybe offload the bridge TX
> > forwarding process (in order to populate the "Force no learning" bit in
> > tag_gswip.c properly), and only then will the local_termination test
> > also pass [...]
> 
> Based on the failing tests I am wondering which step would be a good
> one to start with.
> Is this problem that the selftests are seeing a flooding issue? In
> that case I suspect that the "interesting behavior" (of the GSWIP's
> flooding behavior) that Vladimir described in [1] would be a starting
> point.

It has to do with that, yes. What I said there is that the switch
doesn't autonomously flood unknown packets from one bridged port to
another, but instead, sends them to the CPU and lets the CPU do it.

While that is perfectly respectable from a correctness point of view,
it is also not optimal if you consider performance. The selftests here
try to capture the fact that the switch doesn't send unknown packets to
the CPU. And in this case the driver sends them by construction.

So the absolute first step would be to control the bridge port flags
(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD) and start
with good defaults for standalone mode (also set skb->offload_fwd_mark
when appropriate in the tagging protocol driver). I think you can use
bridge_vlan_aware.sh and bridge_vlan_unaware.sh as starting points to
check that these flags still work fine after you've offloaded them to
hardware.

When flooding a packet to find its destination can be achieved without
involving the CPU (*), the next thing will be to simply disable flooding
packets of all kind to the CPU (except broadcast). That's when you'll
enjoy watching how all the local_termination.sh selftests fail, and
you'll be making them pass again, one by one.

> 
> Full local_termination.sh selftest output:
> TEST: lan2: Unicast IPv4 to primary MAC address                 [ OK ]

For this to pass, the driver must properly respond to a port_fdb_add()
on the CPU port, with the MAC address of the $swp1 user port's net device,
offloaded in the DSA_DB_PORT corresponding to $swp1.

In turn, for DSA to even consider passing you FDB entries in DSA_DB_PORT,
you must make dsa_switch_supports_uc_filtering() return true.

(if you don't know what the words here mean, I've updated the documentation at
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/tree/Documentation/networking/dsa/dsa.rst)

> TEST: lan2: Unicast IPv4 to macvlan MAC address                 [ OK ]

If the above passes, this should pass too. There's nothing different
from the driver's perspective, it's just that port_fdb_add() will be
called on the CPU port with a MAC address that isn't the MAC address of
any user port, but rather one added by the macvlan driver using dev_uc_add().

> TEST: lan2: Unicast IPv4 to unknown MAC address                 [FAIL]
>         reception succeeded, but should have failed

So this test should now pass after you've reworked the entire driver,
essentially. The point is that when flooding is disabled on the CPU port,
packets will be sent there only if they match an entry from the FDB.
Nobody will call port_fdb_add() for the address tested here.

> TEST: lan2: Unicast IPv4 to unknown MAC address, promisc        [ OK ]

Now this passes because the expectation of promiscuous ports is to
receive all packets regardless of MAC DA, that's the definition of
promiscuity. The driver currently already floods to the CPU, so why
wouldn't this pass.

Here, what we actually want to capture is that dsa_slave_manage_host_flood(),
which responds to changes in the IFF_PROMISC flag on a user port, does
actually notify the driver via a call to port_set_host_flood() for that
user port. Through this method, the driver is responsible for turning
flooding towards the CPU port(s) on or off, from the user port given as
argument. If CPU flood control does not depend on user port, then you'll
have to keep CPU flooding enabled as long as any user port wants it.

> TEST: lan2: Unicast IPv4 to unknown MAC address, allmulti       [FAIL]
>         reception succeeded, but should have failed

So I won't repeat why reception of unnecessary packets succeeds, because
the reason is always the same - the driver doesn't dynamically adapt to
all the indications DSA is giving it.

Here, the test captures the fact that IFF_ALLMULTI should enable host
flooding just for unknown multicast, while IFF_PROMISC should allow both
unicast and multicast. The packet was unicast, so IFF_ALLMULTI shouldn't
allow it.

> TEST: lan2: Multicast IPv4 to joined group                      [ OK ]

Here, I used a trivial program I found online to emit a IP_ADD_MEMBERSHIP
setsockopt, to trigger the kernel code that calls dev_mc_add() on the
net device. It seems to not be possible by design to join an IP
multicast group using a dedicated command in a similar way to how you'd
add an FDB entry on a port; instead the kernel joins the multicast group
for as long as the user application persists, and leaves the group afterwards.

As you can probably guess, dev_mc_add() calls made by modules outside
DSA are translated by dsa_slave_set_rx_mode() into a port_mdb_add() on
the CPU port, with DSA_DB_PORT.

If the gswip driver doesn't implement port_mdb_add() but rather treats
multicast as broadcast (by sending it to the CPU), naturally this test
"passes" in the sense that it thinks the driver reacted properly to what
was asked.

> TEST: lan2: Multicast IPv4 to unknown group                     [FAIL]
>         reception succeeded, but should have failed

This tests packet delivery to a multicast address for which dev_mc_add()
wasn't called.

> TEST: lan2: Multicast IPv4 to unknown group, promisc            [ OK ]
> TEST: lan2: Multicast IPv4 to unknown group, allmulti           [ OK ]
> TEST: lan2: Multicast IPv6 to joined group                      [ OK ]

No driver change needed between IPv4 and IPv6 multicast, the addresses
offloaded via switchdev are the L2 translations anyway (01:00:5e:xx:xx:xx
for IPv4 and 33:33:xx:xx:xx:xx for IPv6), so multiple IP addresses will
map to the same MAC address. Side note, the reason why I had to fork
mtools was to add support for IPV6_ADD_MEMBERSHIP, otherwise the
principles are more or less the same.

> TEST: lan2: Multicast IPv6 to unknown group                     [FAIL]
>         reception succeeded, but should have failed
> TEST: lan2: Multicast IPv6 to unknown group, promisc            [ OK ]
> TEST: lan2: Multicast IPv6 to unknown group, allmulti           [ OK ]
> TEST: br0: Unicast IPv4 to primary MAC address                  [ OK ]

Here is where things get interesting. I'm going to take a pause and
explain that the bridge related selftests fail in the same way for me
too, and that the fixes should go to the bridge driver rather than to
DSA.

The problem is that DSA treats IFF_PROMISC as "enable host flooding for
this port", and the bridge puts its ports in IFF_PROMISC mode by reflex.
So in theory, the patches here don't really capture anything relevant as
long as that keeps being the case. Regardless of whether an address is
present in the hardware FDB or not, the IFF_PROMISC triggered by the
bridge keeps host flooding enabled, which makes us receive more than we
need.

I've published a patch set here explaining how I think this would need
to be solved.
https://patchwork.kernel.org/project/netdevbpf/cover/20220408200337.718067-1-vladimir.oltean@nxp.com/
I'd probably do things a bit differently when revisiting.

> TEST: br0: Unicast IPv4 to macvlan MAC address                  [ OK ]

There is even more bridge driver rework to do here. What this test
captures is a macvlan interface on top of a bridge on top of a DSA user
port. Earlier I said that macvlan uses dev_uc_add() towards its lower
interface - the bridge - to ensure its RX filter doesn't drop required
packets. But if the net device doesn't support IFF_UNICAST_FLT (and the
bridge doesn't), dev_uc_add() falls back to putting the interface in
promiscuous mode. And the bridge has this crazy interpretation of what
promiscuous mode means, which is hard to reason about. So eventually,
even though the higher and upper layers of the bridge do what's expected,
we still end up with DSA in promiscuous mode, and hence receiving
packets irrespective of MAC DA. This is why this test really passes
(the promiscuity masks the presence of the macvlan address in the FDB),
and will continue to mask it for as long as the bridge doesn't implement
IFF_UNICAST_FLT.

> TEST: br0: Unicast IPv4 to unknown MAC address                  [FAIL]
>         reception succeeded, but should have failed
> TEST: br0: Unicast IPv4 to unknown MAC address, promisc         [ OK ]
> TEST: br0: Unicast IPv4 to unknown MAC address, allmulti        [FAIL]
>         reception succeeded, but should have failed
> TEST: br0: Multicast IPv4 to joined group                       [ OK ]

Just one more small comment to make.
The addresses in the "br0" tests are also notified through port_mdb_add(),
but they use DSA_DB_BRIDGE since they come to DSA via switchdev rather
than via dev_mc_add() - they came _to_the_bridge_ via dev_mc_add().
DSA drivers are expect to offload these multicast entries to a different
database than the port_mdb_add() I've described above. This is a
database that is active only while $swp1 is part of a bridge, while
DSA_DB_PORT is active only while $swp1 is standalone.

> TEST: br0: Multicast IPv4 to unknown group                      [FAIL]
>         reception succeeded, but should have failed
> TEST: br0: Multicast IPv4 to unknown group, promisc             [ OK ]
> TEST: br0: Multicast IPv4 to unknown group, allmulti            [ OK ]
> TEST: br0: Multicast IPv6 to joined group                       [ OK ]
> TEST: br0: Multicast IPv6 to unknown group                      [FAIL]
>         reception succeeded, but should have failed
> TEST: br0: Multicast IPv6 to unknown group, promisc             [ OK ]
> TEST: br0: Multicast IPv6 to unknown group, allmulti            [ OK ]
> 
> 
> Thank you!
> Best regards,
> Martin
> 
> [0] https://lore.kernel.org/netdev/20220706210651.ozvjcwwp2hquzmhn@skbuf/
> [1] https://lore.kernel.org/netdev/20220702185652.dpzrxuitacqp6m3t@skbuf/

Re: net: dsa: lantiq_gswip: getting the first selftests to pass

[Martin Blumenstingl]

Hi Vladimir,

On Thu, Jul 28, 2022 at 12:44 AM Vladimir Oltean <olteanv@gmail.com> wrote:
[...]
> > I am starting with
> > tools/testing/selftests/drivers/net/dsa/local_termination.sh as my
> > understanding is that this contains the most basic tests and should be
> > the first step.
>
> I don't think I've said local_termination.sh contains the most basic
> tests. Some tests are basic, but not all are.
noted, thanks for clarifying this

[...]
> So the absolute first step would be to control the bridge port flags
> (BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD) and start
> with good defaults for standalone mode (also set skb->offload_fwd_mark
> when appropriate in the tagging protocol driver). I think you can use
> bridge_vlan_aware.sh and bridge_vlan_unaware.sh as starting points to
> check that these flags still work fine after you've offloaded them to
> hardware.
My understanding of "good defaults" is:
- disable learning on all ports
- disable unicast flooding on all ports
- enable broadcast flooding on all ports
- (GSWIP can only enable broadcast and multicast at the same time, so
that's enabled too)

I think skb->offload_fwd_mark needs to be set unless we know that the
hardware wasn't able to forward the frame/packet.
In the vendor sources I was able to find the whole RX tag structure: [0]
I am not sure about the "mirror" bit (I assume this is: packet was
received on this port because this port is configured as a mirroring
target). All other bits seem irrelevant for skb->offload_fwd_mark -
meaning we always have to set skb->offload_fwd_mark.

I have lots of failures in bridge_vlan_aware.sh and
bridge_vlan_unaware.sh - even before any of my changes - which I'll
need to investigate.

> When flooding a packet to find its destination can be achieved without
> involving the CPU (*), the next thing will be to simply disable flooding
> packets of all kind to the CPU (except broadcast). That's when you'll
> enjoy watching how all the local_termination.sh selftests fail, and
> you'll be making them pass again, one by one.
it's called test driven development :-)

> >
> > Full local_termination.sh selftest output:
> > TEST: lan2: Unicast IPv4 to primary MAC address                 [ OK ]
>
> For this to pass, the driver must properly respond to a port_fdb_add()
> on the CPU port, with the MAC address of the $swp1 user port's net device,
> offloaded in the DSA_DB_PORT corresponding to $swp1.
>
> In turn, for DSA to even consider passing you FDB entries in DSA_DB_PORT,
> you must make dsa_switch_supports_uc_filtering() return true.
>
> (if you don't know what the words here mean, I've updated the documentation at
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/tree/Documentation/networking/dsa/dsa.rst)
Three out of four cases for the FDB isolation code are clear to me:
- the DSA_DB_BRIDGE case is easy as this is basically what we had
implemented before and I "just" need to look up the FID based on
db.bridge.dev
- DSA_DB_PORT for a non-CPU/user port: we should use the same FID as
standalone ("single port bridge") ports use (that's port number + 1;
see gswip_add_single_port_br())
- GSWIP doesn't support DSA_DB_LAG currently, so I can handle this
with -EOPNOTSUPP

One case took me a while to figure out but I think I finally
understood it (and now it's clear why the FDB patch I sent earlier
cannot be upstreamed):
- DSA_DB_PORT for the CPU port: the port argument for port_fdb_add is
the CPU port - but we can't map this to a FID (those are always tied
to either a bridge or a user port). So instead I need to look at db.dp
and for example use it's index for getting the FID (for standalone
ports the FID is: port index + 1). That results in: we're requested to
install the CPU ports MAC address on the CPU port (6), but what we
actually do is install the FDB entry with the CPU port's MAC address
on a user port (let's say 4, which we get from db.dp). Now if a
packet/frame should target the CPU port we don't need flooding because
the switch knows the destination port based on the FDB entry we
installed.

Note to myself: I'll also need to look at
assisted_learning_on_cpu_port when I'm at the point where I can do
optimizations (rather than fixing failing tests).

[...]
> Nobody will call port_fdb_add() for the address tested here.
>
> > TEST: lan2: Unicast IPv4 to unknown MAC address, promisc        [ OK ]
>
> Now this passes because the expectation of promiscuous ports is to
> receive all packets regardless of MAC DA, that's the definition of
> promiscuity. The driver currently already floods to the CPU, so why
> wouldn't this pass.
>
> Here, what we actually want to capture is that dsa_slave_manage_host_flood(),
> which responds to changes in the IFF_PROMISC flag on a user port, does
> actually notify the driver via a call to port_set_host_flood() for that
> user port. Through this method, the driver is responsible for turning
> flooding towards the CPU port(s) on or off, from the user port given as
> argument. If CPU flood control does not depend on user port, then you'll
> have to keep CPU flooding enabled as long as any user port wants it.
Thanks for this hint. Indeed, I need to implement it like you did in
felix_port_set_host_flood() because GSWIP can only enable/disable
flooding to the CPU port globally - it can't configure this based on
the source port.

[...]
> > TEST: lan2: Multicast IPv6 to unknown group                     [FAIL]
> >         reception succeeded, but should have failed
> > TEST: lan2: Multicast IPv6 to unknown group, promisc            [ OK ]
> > TEST: lan2: Multicast IPv6 to unknown group, allmulti           [ OK ]
> > TEST: br0: Unicast IPv4 to primary MAC address                  [ OK ]
>
> Here is where things get interesting. I'm going to take a pause and
> explain that the bridge related selftests fail in the same way for me
> too, and that the fixes should go to the bridge driver rather than to
> DSA.
oh, that's good to know - thanks for sharing this info!

[...]
> > TEST: br0: Unicast IPv4 to unknown MAC address                  [FAIL]
> >         reception succeeded, but should have failed
> > TEST: br0: Unicast IPv4 to unknown MAC address, promisc         [ OK ]
> > TEST: br0: Unicast IPv4 to unknown MAC address, allmulti        [FAIL]
> >         reception succeeded, but should have failed
> > TEST: br0: Multicast IPv4 to joined group                       [ OK ]
>
> Just one more small comment to make.
> The addresses in the "br0" tests are also notified through port_mdb_add(),
> but they use DSA_DB_BRIDGE since they come to DSA via switchdev rather
> than via dev_mc_add() - they came _to_the_bridge_ via dev_mc_add().
> DSA drivers are expect to offload these multicast entries to a different
> database than the port_mdb_add() I've described above. This is a
> database that is active only while $swp1 is part of a bridge, while
> DSA_DB_PORT is active only while $swp1 is standalone.
The vendor driver lists a bunch of possible tables: [1]
I'll leave any effort for multicast out for now... it's not the main
use-case I am looking at right now.

As always: thank you for your amazing explanations, hints and pointers!
Also I would like to point out that I am still doing all of this in my
spare time. Whenever you have other conversations to focus on: please
do so! For me it's not critical if you're getting back to me a few
days sooner or later.


Best regards,
Martin


[0] https://github.com/paldier/K3C/blob/ca7353eb397090c363632409d9ca568d3cca09c7/ugw/target/linux/lantiq/patches-3.10/7000-NET-lantiq-adds-eth-drv.patch#L2238-L2259
[1] https://github.com/paldier/K3C/blob/ca7353eb397090c363632409d9ca568d3cca09c7/ugw/target/linux/lantiq/files/drivers/net/ethernet/lantiq/switch-api/ltq_flow_core.h#L164-L192

Re: net: dsa: lantiq_gswip: getting the first selftests to pass

[Vladimir Oltean]

On Fri, Jul 29, 2022 at 12:09:50AM +0200, Martin Blumenstingl wrote:
> Hi Vladimir,
> 
> On Thu, Jul 28, 2022 at 12:44 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> [...]
> > > I am starting with
> > > tools/testing/selftests/drivers/net/dsa/local_termination.sh as my
> > > understanding is that this contains the most basic tests and should be
> > > the first step.
> >
> > I don't think I've said local_termination.sh contains the most basic
> > tests. Some tests are basic, but not all are.
> noted, thanks for clarifying this
> 
> [...]
> > So the absolute first step would be to control the bridge port flags
> > (BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD) and start
> > with good defaults for standalone mode (also set skb->offload_fwd_mark
> > when appropriate in the tagging protocol driver). I think you can use
> > bridge_vlan_aware.sh and bridge_vlan_unaware.sh as starting points to
> > check that these flags still work fine after you've offloaded them to
> > hardware.
> My understanding of "good defaults" is:
> - disable learning on all ports

Yes, here's just one other example of what can go wrong if it's enabled
on standalone ports, if you need to see it:
https://lore.kernel.org/netdev/20220727233249.fpn7gyivnkdg5uhe@skbuf/T/#m2e27a5385f70ee3440ee7f6250aaafdbfdc7446b

Essentially every time when there's a chance that the switch will
receive on one port what another port has sent, learning will be a
problem. This is why it's also problematic for the selftests - because
we intentionally put 2 pairs of ports in loopback.

> - disable unicast flooding on all ports

I am having trouble saying 'yes' or 'no' to this because I don't know
exactly what you mean. By flooding a packet, I understand "if its MAC DA
is unknown to the FDB, deliver it to this set of ports". But flooding,
like learning, is essentially a bridging service concept, so it applies
only to packets coming from a particular bridging domain. In the case of
a standalone port, packets come only from the CPU, via the control
plane. Depending how the hardware is constructed, when you inject a
packet to a port, maybe there won't be any ifs or buts and the switch
will just deliver it there (I call this behavior: "control packets
bypass FDB lookup", or "CPU is in god mode"). So maybe it doesn't matter
whether unicast flooding is enabled on all standalone ports or not, as
long as the macroscopically expected behavior can be observed: if
software xmits a packet to a port, the packet gets delivered regardless
of MAC DA.

> - enable broadcast flooding on all ports

Here I don't understand, why unicast no but broadcast yes? Flooding is
an egress setting, at least in the terminology from 'man bridge'
("Controls whether unicast traffic for which there is no FDB entry will
be flooded towards this given port.")

The same thing applies as for unicast flooding above: maybe it doesn't
matter.

> - (GSWIP can only enable broadcast and multicast at the same time, so
> that's enabled too)

I think the GSWIP would not be the only one in that category. The
mv88e6xxx driver puts the ff:ff:ff:ff:ff:ff address in the FDB and that
controls broadcast flooding, while the single knob that you mention
controls what's left - i.e. multicast.

> I think skb->offload_fwd_mark needs to be set unless we know that the
> hardware wasn't able to forward the frame/packet.
> In the vendor sources I was able to find the whole RX tag structure: [0]
> I am not sure about the "mirror" bit (I assume this is: packet was
> received on this port because this port is configured as a mirroring
> target). All other bits seem irrelevant for skb->offload_fwd_mark -
> meaning we always have to set skb->offload_fwd_mark.
> 
> I have lots of failures in bridge_vlan_aware.sh and
> bridge_vlan_unaware.sh - even before any of my changes - which I'll
> need to investigate.

I don't remember the problems I faced while making these tests pass on
my hardware, and I also don't think they'll be the same as the ones
you'll face.

> > When flooding a packet to find its destination can be achieved without
> > involving the CPU (*), the next thing will be to simply disable flooding
> > packets of all kind to the CPU (except broadcast). That's when you'll
> > enjoy watching how all the local_termination.sh selftests fail, and
> > you'll be making them pass again, one by one.
> it's called test driven development :-)

Yeah, thanks for putting a name on it. It also happens to be the reason
why I haven't resent my patches for multiple CPU ports this kernel cycle;
I still have some bridge-related local_termination.sh tests that fail,
which didn't before. Normally I would have been happy that hey, yay, it
compiles! let me send it to the list! and I would be looking at cases of
failures as reports would come in (or rely on others to have the inspiration
to tell me that this is going to be broken).

> > > Full local_termination.sh selftest output:
> > > TEST: lan2: Unicast IPv4 to primary MAC address                 [ OK ]
> >
> > For this to pass, the driver must properly respond to a port_fdb_add()
> > on the CPU port, with the MAC address of the $swp1 user port's net device,
> > offloaded in the DSA_DB_PORT corresponding to $swp1.
> >
> > In turn, for DSA to even consider passing you FDB entries in DSA_DB_PORT,
> > you must make dsa_switch_supports_uc_filtering() return true.
> >
> > (if you don't know what the words here mean, I've updated the documentation at
> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/tree/Documentation/networking/dsa/dsa.rst)
> Three out of four cases for the FDB isolation code are clear to me:
> - the DSA_DB_BRIDGE case is easy as this is basically what we had
> implemented before and I "just" need to look up the FID based on
> db.bridge.dev

Or db.bridge.num (this is currently set to 0 by DSA because you don't
declare ds->fdb_isolation = true), whichever is more convenient.

> - DSA_DB_PORT for a non-CPU/user port: we should use the same FID as
> standalone ("single port bridge") ports use (that's port number + 1;
> see gswip_add_single_port_br())

The db.dp from a DSA_DB_PORT entry will always be a user port, and this
is by construction, you don't even have to check.

> - GSWIP doesn't support DSA_DB_LAG currently, so I can handle this
> with -EOPNOTSUPP

Yes, furthermore, I think DSA_DB_LAG will get deleted at some point.

> One case took me a while to figure out but I think I finally
> understood it (and now it's clear why the FDB patch I sent earlier
> cannot be upstreamed):
> - DSA_DB_PORT for the CPU port: the port argument for port_fdb_add is
> the CPU port - but we can't map this to a FID (those are always tied
> to either a bridge or a user port). So instead I need to look at db.dp
> and for example use it's index for getting the FID (for standalone
> ports the FID is: port index + 1).

Looking at db.dp to determine the FID is not a workaround, but rather
exactly what you are expected to do.

> That results in: we're requested to install the CPU ports MAC address
> on the CPU port (6),

No. The CPU port doesn't have a MAC address (and in fact no port does;
it's a switch). But user ports have MAC addresses which are a purely
software construct to denote L2 termination. Every user port net device
can have its own MAC address, different from the other, and different
from the MAC address of the DSA master. Its interpretation is: "if a
user port receives a packet with a MAC DA that's equal to the net device's
MAC address, send the packet to the CPU, otherwise drop it".
It makes the standalone NIC illusion work.

The CPU port is just a dumb pipe, it just transports packets to/from our
actual user ports. We don't have a termination point for it (or as written
in other places: "we don't have a net_device"), so no MAC address, not
even as a software construct.

A pipe is exactly how you should see the CPU port. It doesn't have a FID
(a single port bridge) of its own because it is a part of all FIDs.

> but what we actually do is install the FDB entry with the CPU port's
> MAC address on a user port (let's say 4, which we get from db.dp).

No, quite the other way around.

Let's take an example based on what you've described: user port swp4 has
MAC address 00:01:02:03:04:05, and CPU port is 6. You'll get a call to

port_fdb_add(ds, port = 6, addr = 00:01:02:03:04:05, vid = 0,
	     db = {type = DSA_DB_PORT, dp = swp4}).

What you need to do is create an FDB entry on which only packets
received by swp4 in standalone mode will match (so it needs to have a
FID equal to the FID that swp4 classifies packets to, in standalone mode),
and delivers these packets to the CPU port 6, which is already in that FID,
as it is part of every FID. Remember, when swp4 receives a packet and is
standalone, it always assigns the FID of that packet to the value that
it's configured to (port index + 1, or 5, if you say so). This packet
in this FID can either find an entry in the FDB, case in which its
destination is certainly the CPU port (that's why port = 6), or the
address will be absent from the FDB, case in which the packet will be
flooded nowhere (the only other port in this FID, the CPU port, has
flooding turned off) => dropped.

As mentioned earlier, it's desirable that packets delivered by software,
over the CPU port and towards a standalone one, are sent in "god mode",
so that the FDB won't be searched at all in that direction.

You seem to have something reversed in your terminology, although I
can't exactly pinpoint what. When you say "install an FDB entry on port X",
what I understand is "make the packets with that FDB entry's MAC DA be
delivered towards port X". Or maybe I have something reversed?
I'm quite curious to know.

> Now if a packet/frame should target the CPU port we don't need
> flooding because the switch knows the destination port based on the
> FDB entry we installed.

Yes, so rather than the CPU port being a 'dumb' pipe which passes all
packets through it, you're making it a slightly 'smarter' pipe which
essentially uses the FDB as an RX filter for standalone user ports.

> Also I would like to point out that I am still doing all of this in my
> spare time.

I'm doing this in my spare time as well, and I'm having fun while at it.
Sorry for being handwavy and insisting only on explaining the general
idea rather than opening the GSWIP manual and checking that what I'm
saying is actually implementable. I'll do so if you have a specific
question about something apparently not mapping to the expectations.

Re: net: dsa: lantiq_gswip: getting the first selftests to pass

[Martin Blumenstingl]

Hi Vladimir,

On Fri, Jul 29, 2022 at 2:05 AM Vladimir Oltean <olteanv@gmail.com> wrote:
[...]
> > - disable learning on all ports
>
> Yes, here's just one other example of what can go wrong if it's enabled
> on standalone ports, if you need to see it:
> https://lore.kernel.org/netdev/20220727233249.fpn7gyivnkdg5uhe@skbuf/T/#m2e27a5385f70ee3440ee7f6250aaafdbfdc7446b
>
> Essentially every time when there's a chance that the switch will
> receive on one port what another port has sent, learning will be a
> problem. This is why it's also problematic for the selftests - because
> we intentionally put 2 pairs of ports in loopback.
Makes sense, thanks!

> > - disable unicast flooding on all ports
>
> I am having trouble saying 'yes' or 'no' to this because I don't know
> exactly what you mean. By flooding a packet, I understand "if its MAC DA
> is unknown to the FDB, deliver it to this set of ports". But flooding,
> like learning, is essentially a bridging service concept, so it applies
> only to packets coming from a particular bridging domain. In the case of
> a standalone port, packets come only from the CPU, via the control
> plane. Depending how the hardware is constructed, when you inject a
> packet to a port, maybe there won't be any ifs or buts and the switch
> will just deliver it there (I call this behavior: "control packets
> bypass FDB lookup", or "CPU is in god mode"). So maybe it doesn't matter
> whether unicast flooding is enabled on all standalone ports or not, as
> long as the macroscopically expected behavior can be observed: if
> software xmits a packet to a port, the packet gets delivered regardless
> of MAC DA.
I think I do understand it now.
We want the defaults to apply to standalone ports. Since flooding does
not exist there we should disable it (for both, unicast and
broadcast/multicast traffic). When flooding is wanted on a specific
port it'll be enabled through port_bridge_flags.

[...]
> > - (GSWIP can only enable broadcast and multicast at the same time, so
> > that's enabled too)
>
> I think the GSWIP would not be the only one in that category. The
> mv88e6xxx driver puts the ff:ff:ff:ff:ff:ff address in the FDB and that
> controls broadcast flooding, while the single knob that you mention
> controls what's left - i.e. multicast.

> > I think skb->offload_fwd_mark needs to be set unless we know that the
> > hardware wasn't able to forward the frame/packet.
> > In the vendor sources I was able to find the whole RX tag structure: [0]
> > I am not sure about the "mirror" bit (I assume this is: packet was
> > received on this port because this port is configured as a mirroring
> > target). All other bits seem irrelevant for skb->offload_fwd_mark -
> > meaning we always have to set skb->offload_fwd_mark.
> >
> > I have lots of failures in bridge_vlan_aware.sh and
> > bridge_vlan_unaware.sh - even before any of my changes - which I'll
> > need to investigate.
>
> I don't remember the problems I faced while making these tests pass on
> my hardware, and I also don't think they'll be the same as the ones
> you'll face.
I'll postpone bridge_vlan_unaware.sh investigations until I have the
standalone tests (which are relevant for GSWIP, meaning: excluding the
multicast ones) from local_termination.sh passing.

[...]
> > - the DSA_DB_BRIDGE case is easy as this is basically what we had
> > implemented before and I "just" need to look up the FID based on
> > db.bridge.dev
>
> Or db.bridge.num (this is currently set to 0 by DSA because you don't
> declare ds->fdb_isolation = true), whichever is more convenient.
Using db.bridge.num will probably allow us to get rid of the
priv->vlans array in the GSWIP driver. For now I'm using the bridge
dev since "it works" until tests are passing.

[...]
> > - DSA_DB_PORT for the CPU port: the port argument for port_fdb_add is
> > the CPU port - but we can't map this to a FID (those are always tied
> > to either a bridge or a user port). So instead I need to look at db.dp
> > and for example use it's index for getting the FID (for standalone
> > ports the FID is: port index + 1).
>
> Looking at db.dp to determine the FID is not a workaround, but rather
> exactly what you are expected to do.
Thanks for confirming!

> > That results in: we're requested to install the CPU ports MAC address
> > on the CPU port (6),
>
> No. The CPU port doesn't have a MAC address (and in fact no port does;
> it's a switch). But user ports have MAC addresses which are a purely
> software construct to denote L2 termination. Every user port net device
> can have its own MAC address, different from the other, and different
> from the MAC address of the DSA master. Its interpretation is: "if a
> user port receives a packet with a MAC DA that's equal to the net device's
> MAC address, send the packet to the CPU, otherwise drop it".
> It makes the standalone NIC illusion work.
>
> The CPU port is just a dumb pipe, it just transports packets to/from our
> actual user ports. We don't have a termination point for it (or as written
> in other places: "we don't have a net_device"), so no MAC address, not
> even as a software construct.
>
> A pipe is exactly how you should see the CPU port. It doesn't have a FID
> (a single port bridge) of its own because it is a part of all FIDs.
>
> > but what we actually do is install the FDB entry with the CPU port's
> > MAC address on a user port (let's say 4, which we get from db.dp).
>
> No, quite the other way around.
>
> Let's take an example based on what you've described: user port swp4 has
> MAC address 00:01:02:03:04:05, and CPU port is 6. You'll get a call to
>
> port_fdb_add(ds, port = 6, addr = 00:01:02:03:04:05, vid = 0,
>              db = {type = DSA_DB_PORT, dp = swp4}).
>
> What you need to do is create an FDB entry on which only packets
> received by swp4 in standalone mode will match (so it needs to have a
> FID equal to the FID that swp4 classifies packets to, in standalone mode),
> and delivers these packets to the CPU port 6, which is already in that FID,
> as it is part of every FID. Remember, when swp4 receives a packet and is
> standalone, it always assigns the FID of that packet to the value that
> it's configured to (port index + 1, or 5, if you say so). This packet
> in this FID can either find an entry in the FDB, case in which its
> destination is certainly the CPU port (that's why port = 6), or the
> address will be absent from the FDB, case in which the packet will be
> flooded nowhere (the only other port in this FID, the CPU port, has
> flooding turned off) => dropped.
>
> As mentioned earlier, it's desirable that packets delivered by software,
> over the CPU port and towards a standalone one, are sent in "god mode",
> so that the FDB won't be searched at all in that direction.
>
> You seem to have something reversed in your terminology, although I
> can't exactly pinpoint what. When you say "install an FDB entry on port X",
> what I understand is "make the packets with that FDB entry's MAC DA be
> delivered towards port X". Or maybe I have something reversed?
> I'm quite curious to know.
Thanks a lot for explaining this (yet again)!
There's three issues with my original sentence:
- I should have used the term "user port's MAC address" instead of
"CPU ports MAC"
- "on the CPU port (6)" needs to be more precise, it should be
"towards the CPU port (6)"
- I'm not mentioning the source port (user port) number and thus FID at all

Also I need to get the idea out of my head that the CPU port is equal to eth0.
It's not, eth0 is connected to the CPU port on the switch.

While working on my patches a more practical question came up while I
was breaking the driver and then trying to make local_termination.sh
pass again.
At the start of run_tests for the standalone port scenario I am
getting the following values:
  rcv_dmac = 00:01:02:03:04:02
  MACVLAN_ADDR = 00:00:de:ad:be:ef
My expectation is that port_fdb_add() is called with these MAC
addresses. I verified that dsa_switch_supports_uc_filtering() returns
true, but still I

> > Now if a packet/frame should target the CPU port we don't need
> > flooding because the switch knows the destination port based on the
> > FDB entry we installed.
>
> Yes, so rather than the CPU port being a 'dumb' pipe which passes all
> packets through it, you're making it a slightly 'smarter' pipe which
> essentially uses the FDB as an RX filter for standalone user ports.
>
> > Also I would like to point out that I am still doing all of this in my
> > spare time.
>
> I'm doing this in my spare time as well, and I'm having fun while at it.
> Sorry for being handwavy and insisting only on explaining the general
> idea rather than opening the GSWIP manual and checking that what I'm
> saying is actually implementable. [...]
I fully understand this and it makes sense as others can also benefit
from your explanation (since it's generic, not driver specific).

> I'll do so if you have a specific question about something apparently
> not mapping to the expectations.
I still have an issue which I believe is related to the FDB handling.

I *think* that I have implemented FDB isolation correctly in my
work-in-progress branch [0].

The GSWIP140 datasheet (page 82) has a "MAC Learning disable and MAC
Learning Limitation Description" (table 26).
In the xRX200 vendor kernel I cannot find the LNDIS bit in
PCE_PCTRL_3, so I suspect it has only been added in newer GSWIP
revisions (xRX200 is at least one major IP revision behind GSW140).
Maybe Hauke knows?
So what I'm doing to disable learning is setting the "learning limit"
(which limits the number of entries that can be learned) for that port
to zero.

 My problem is that whenever I disable learning a lot of tests from
local_termination.sh are failing, including:
- TEST: lan2: Unicast IPv4 to primary MAC address
- TEST: lan2: Unicast IPv4 to macvlan MAC address

Setting the PLIMMOD bit to 1 means that GSWIP won't drop the packet if
the learning limit is exceeded (the default value seems to be 0).
This at least works around the first failing test (Unicast IPv4 to
primary MAC address).

Based on your understanding of my issue: I am going in the right
direction when I'm saying that this is an FDB issue?


Thank you!
Martin


[0] https://github.com/xdarklight/linux/commits/lantiq-gswip-integration-20220730

Re: net: dsa: lantiq_gswip: getting the first selftests to pass

[Aleksander Bajkowski]

Hi Martin,

On 7/31/22 22:49, Martin Blumenstingl wrote:
> Hi Vladimir,
> 
> On Fri, Jul 29, 2022 at 2:05 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> [...]
>>> - disable learning on all ports
>>
>> Yes, here's just one other example of what can go wrong if it's enabled
>> on standalone ports, if you need to see it:
>> https://lore.kernel.org/netdev/20220727233249.fpn7gyivnkdg5uhe@skbuf/T/#m2e27a5385f70ee3440ee7f6250aaafdbfdc7446b
>>
>> Essentially every time when there's a chance that the switch will
>> receive on one port what another port has sent, learning will be a
>> problem. This is why it's also problematic for the selftests - because
>> we intentionally put 2 pairs of ports in loopback.
> Makes sense, thanks!
> 
>>> - disable unicast flooding on all ports
>>
>> I am having trouble saying 'yes' or 'no' to this because I don't know
>> exactly what you mean. By flooding a packet, I understand "if its MAC DA
>> is unknown to the FDB, deliver it to this set of ports". But flooding,
>> like learning, is essentially a bridging service concept, so it applies
>> only to packets coming from a particular bridging domain. In the case of
>> a standalone port, packets come only from the CPU, via the control
>> plane. Depending how the hardware is constructed, when you inject a
>> packet to a port, maybe there won't be any ifs or buts and the switch
>> will just deliver it there (I call this behavior: "control packets
>> bypass FDB lookup", or "CPU is in god mode"). So maybe it doesn't matter
>> whether unicast flooding is enabled on all standalone ports or not, as
>> long as the macroscopically expected behavior can be observed: if
>> software xmits a packet to a port, the packet gets delivered regardless
>> of MAC DA.
> I think I do understand it now.
> We want the defaults to apply to standalone ports. Since flooding does
> not exist there we should disable it (for both, unicast and
> broadcast/multicast traffic). When flooding is wanted on a specific
> port it'll be enabled through port_bridge_flags.
> 
> [...]
>>> - (GSWIP can only enable broadcast and multicast at the same time, so
>>> that's enabled too)
>>
>> I think the GSWIP would not be the only one in that category. The
>> mv88e6xxx driver puts the ff:ff:ff:ff:ff:ff address in the FDB and that
>> controls broadcast flooding, while the single knob that you mention
>> controls what's left - i.e. multicast.
> 
>>> I think skb->offload_fwd_mark needs to be set unless we know that the
>>> hardware wasn't able to forward the frame/packet.
>>> In the vendor sources I was able to find the whole RX tag structure: [0]
>>> I am not sure about the "mirror" bit (I assume this is: packet was
>>> received on this port because this port is configured as a mirroring
>>> target). All other bits seem irrelevant for skb->offload_fwd_mark -
>>> meaning we always have to set skb->offload_fwd_mark.
>>>
>>> I have lots of failures in bridge_vlan_aware.sh and
>>> bridge_vlan_unaware.sh - even before any of my changes - which I'll
>>> need to investigate.
>>
>> I don't remember the problems I faced while making these tests pass on
>> my hardware, and I also don't think they'll be the same as the ones
>> you'll face.
> I'll postpone bridge_vlan_unaware.sh investigations until I have the
> standalone tests (which are relevant for GSWIP, meaning: excluding the
> multicast ones) from local_termination.sh passing.
> 
> [...]
>>> - the DSA_DB_BRIDGE case is easy as this is basically what we had
>>> implemented before and I "just" need to look up the FID based on
>>> db.bridge.dev
>>
>> Or db.bridge.num (this is currently set to 0 by DSA because you don't
>> declare ds->fdb_isolation = true), whichever is more convenient.
> Using db.bridge.num will probably allow us to get rid of the
> priv->vlans array in the GSWIP driver. For now I'm using the bridge
> dev since "it works" until tests are passing.
> 
> [...]
>>> - DSA_DB_PORT for the CPU port: the port argument for port_fdb_add is
>>> the CPU port - but we can't map this to a FID (those are always tied
>>> to either a bridge or a user port). So instead I need to look at db.dp
>>> and for example use it's index for getting the FID (for standalone
>>> ports the FID is: port index + 1).
>>
>> Looking at db.dp to determine the FID is not a workaround, but rather
>> exactly what you are expected to do.
> Thanks for confirming!
> 
>>> That results in: we're requested to install the CPU ports MAC address
>>> on the CPU port (6),
>>
>> No. The CPU port doesn't have a MAC address (and in fact no port does;
>> it's a switch). But user ports have MAC addresses which are a purely
>> software construct to denote L2 termination. Every user port net device
>> can have its own MAC address, different from the other, and different
>> from the MAC address of the DSA master. Its interpretation is: "if a
>> user port receives a packet with a MAC DA that's equal to the net device's
>> MAC address, send the packet to the CPU, otherwise drop it".
>> It makes the standalone NIC illusion work.
>>
>> The CPU port is just a dumb pipe, it just transports packets to/from our
>> actual user ports. We don't have a termination point for it (or as written
>> in other places: "we don't have a net_device"), so no MAC address, not
>> even as a software construct.
>>
>> A pipe is exactly how you should see the CPU port. It doesn't have a FID
>> (a single port bridge) of its own because it is a part of all FIDs.
>>
>>> but what we actually do is install the FDB entry with the CPU port's
>>> MAC address on a user port (let's say 4, which we get from db.dp).
>>
>> No, quite the other way around.
>>
>> Let's take an example based on what you've described: user port swp4 has
>> MAC address 00:01:02:03:04:05, and CPU port is 6. You'll get a call to
>>
>> port_fdb_add(ds, port = 6, addr = 00:01:02:03:04:05, vid = 0,
>>              db = {type = DSA_DB_PORT, dp = swp4}).
>>
>> What you need to do is create an FDB entry on which only packets
>> received by swp4 in standalone mode will match (so it needs to have a
>> FID equal to the FID that swp4 classifies packets to, in standalone mode),
>> and delivers these packets to the CPU port 6, which is already in that FID,
>> as it is part of every FID. Remember, when swp4 receives a packet and is
>> standalone, it always assigns the FID of that packet to the value that
>> it's configured to (port index + 1, or 5, if you say so). This packet
>> in this FID can either find an entry in the FDB, case in which its
>> destination is certainly the CPU port (that's why port = 6), or the
>> address will be absent from the FDB, case in which the packet will be
>> flooded nowhere (the only other port in this FID, the CPU port, has
>> flooding turned off) => dropped.
>>
>> As mentioned earlier, it's desirable that packets delivered by software,
>> over the CPU port and towards a standalone one, are sent in "god mode",
>> so that the FDB won't be searched at all in that direction.
>>
>> You seem to have something reversed in your terminology, although I
>> can't exactly pinpoint what. When you say "install an FDB entry on port X",
>> what I understand is "make the packets with that FDB entry's MAC DA be
>> delivered towards port X". Or maybe I have something reversed?
>> I'm quite curious to know.
> Thanks a lot for explaining this (yet again)!
> There's three issues with my original sentence:
> - I should have used the term "user port's MAC address" instead of
> "CPU ports MAC"
> - "on the CPU port (6)" needs to be more precise, it should be
> "towards the CPU port (6)"
> - I'm not mentioning the source port (user port) number and thus FID at all
> 
> Also I need to get the idea out of my head that the CPU port is equal to eth0.
> It's not, eth0 is connected to the CPU port on the switch.
> 
> While working on my patches a more practical question came up while I
> was breaking the driver and then trying to make local_termination.sh
> pass again.
> At the start of run_tests for the standalone port scenario I am
> getting the following values:
>   rcv_dmac = 00:01:02:03:04:02
>   MACVLAN_ADDR = 00:00:de:ad:be:ef
> My expectation is that port_fdb_add() is called with these MAC
> addresses. I verified that dsa_switch_supports_uc_filtering() returns
> true, but still I
> 
>>> Now if a packet/frame should target the CPU port we don't need
>>> flooding because the switch knows the destination port based on the
>>> FDB entry we installed.
>>
>> Yes, so rather than the CPU port being a 'dumb' pipe which passes all
>> packets through it, you're making it a slightly 'smarter' pipe which
>> essentially uses the FDB as an RX filter for standalone user ports.
>>
>>> Also I would like to point out that I am still doing all of this in my
>>> spare time.
>>
>> I'm doing this in my spare time as well, and I'm having fun while at it.
>> Sorry for being handwavy and insisting only on explaining the general
>> idea rather than opening the GSWIP manual and checking that what I'm
>> saying is actually implementable. [...]
> I fully understand this and it makes sense as others can also benefit
> from your explanation (since it's generic, not driver specific).
> 
>> I'll do so if you have a specific question about something apparently
>> not mapping to the expectations.
> I still have an issue which I believe is related to the FDB handling.
> 
> I *think* that I have implemented FDB isolation correctly in my
> work-in-progress branch [0].
> 
> The GSWIP140 datasheet (page 82) has a "MAC Learning disable and MAC
> Learning Limitation Description" (table 26).
> In the xRX200 vendor kernel I cannot find the LNDIS bit in
> PCE_PCTRL_3, so I suspect it has only been added in newer GSWIP
> revisions (xRX200 is at least one major IP revision behind GSW140).
> Maybe Hauke knows?


In the GPL sources [1] for the xRX330, the LNDIS bit is marked as added
in revision 2.2 of the GSWIP IP core. The xRX200 uses revision 2.0 or 2.1.


> So what I'm doing to disable learning is setting the "learning limit"
> (which limits the number of entries that can be learned) for that port
> to zero.
> 
>  My problem is that whenever I disable learning a lot of tests from
> local_termination.sh are failing, including:
> - TEST: lan2: Unicast IPv4 to primary MAC address
> - TEST: lan2: Unicast IPv4 to macvlan MAC address
> 
> Setting the PLIMMOD bit to 1 means that GSWIP won't drop the packet if
> the learning limit is exceeded (the default value seems to be 0).
> This at least works around the first failing test (Unicast IPv4 to
> primary MAC address).
> 
> Based on your understanding of my issue: I am going in the right
> direction when I'm saying that this is an FDB issue?
> 
> 
> Thank you!
> Martin
> 
> 
> [0] https://github.com/xdarklight/linux/commits/lantiq-gswip-integration-20220730

[1] https://github.com/brunompena/dwr-966/blob/807b1dbd1179ba16c89948bd7990d729074b4bc6/kernel/drivers/net/ethernet/lantiq/switch-api/ltq_flow_reg_switch.h

Best Regards,
Aleksander

Re: net: dsa: lantiq_gswip: getting the first selftests to pass

[Martin Blumenstingl]

Hi Aleksander,

On Sun, Jul 31, 2022 at 11:29 PM Aleksander Bajkowski <olek2@wp.pl> wrote:
[...]
> > The GSWIP140 datasheet (page 82) has a "MAC Learning disable and MAC
> > Learning Limitation Description" (table 26).
> > In the xRX200 vendor kernel I cannot find the LNDIS bit in
> > PCE_PCTRL_3, so I suspect it has only been added in newer GSWIP
> > revisions (xRX200 is at least one major IP revision behind GSW140).
> > Maybe Hauke knows?
>
>
> In the GPL sources [1] for the xRX330, the LNDIS bit is marked as added
> in revision 2.2 of the GSWIP IP core. The xRX200 uses revision 2.0 or 2.1.
oh, that's some really useful information - thanks a lot!


Best regards,
Martin

Re: net: dsa: lantiq_gswip: getting the first selftests to pass

[Vladimir Oltean]

On Sun, Jul 31, 2022 at 10:49:35PM +0200, Martin Blumenstingl wrote:
> While working on my patches a more practical question came up while I
> was breaking the driver and then trying to make local_termination.sh
> pass again.
> At the start of run_tests for the standalone port scenario I am
> getting the following values:
>   rcv_dmac = 00:01:02:03:04:02
>   MACVLAN_ADDR = 00:00:de:ad:be:ef
> My expectation is that port_fdb_add() is called with these MAC
> addresses. I verified that dsa_switch_supports_uc_filtering() returns
> true, but still I

Unfinished phrase, I suppose you wanted to say that you don't get a call
to port_fdb_add() with $MACVLAN_ADDR. I don't see why that would be the
case. Try to put some prints in dsa_slave_sync_uc(), since that's the
path through which $MACVLAN_ADDR comes in. On the other hand, $rcv_dmac
comes from dsa_slave_open().

> > I'll do so if you have a specific question about something apparently
> > not mapping to the expectations.
> I still have an issue which I believe is related to the FDB handling.
> 
> I *think* that I have implemented FDB isolation correctly in my
> work-in-progress branch [0].
> 
> The GSWIP140 datasheet (page 82) has a "MAC Learning disable and MAC
> Learning Limitation Description" (table 26).
> In the xRX200 vendor kernel I cannot find the LNDIS bit in
> PCE_PCTRL_3, so I suspect it has only been added in newer GSWIP
> revisions (xRX200 is at least one major IP revision behind GSW140).
> Maybe Hauke knows?
> So what I'm doing to disable learning is setting the "learning limit"
> (which limits the number of entries that can be learned) for that port
> to zero.
> 
>  My problem is that whenever I disable learning a lot of tests from
> local_termination.sh are failing, including:
> - TEST: lan2: Unicast IPv4 to primary MAC address
> - TEST: lan2: Unicast IPv4 to macvlan MAC address
> 
> Setting the PLIMMOD bit to 1 means that GSWIP won't drop the packet if
> the learning limit is exceeded (the default value seems to be 0).

Yes, I'm not sure why you'd want to drop packets that aren't learned, so
I would expect PLIMMOD to be 1 if you're disabling learning via LRNLIM=0.

> This at least works around the first failing test (Unicast IPv4 to
> primary MAC address).

Not sure why "works around" is the choice of words here.

> Based on your understanding of my issue: I am going in the right
> direction when I'm saying that this is an FDB issue?

I don't know why the tests fail. I downloaded your code and I see that
you touch PLIMMOD from ds->ops->port_set_host_flood(). Why not just
leave it at 1? It's a global bit anyway, it affects what happens with
all ports that have source address learning disabled, it seems a very
odd choice to do what you're doing.

When you have learning disabled on standalone ports (by design), and
local_termination.sh receives packets on $swp1, a standalone port
(by design), don't you agree that the MAC SA of these packets won't be
learned, and you're telling the switch "yeah, drop them"?

> [0] https://github.com/xdarklight/linux/commits/lantiq-gswip-integration-20220730

There are many things to say about the code, however it's a bit
difficult to review it just by looking at the Github commits.
For example I'm looking at gswip_port_fdb(), I don't really understand
why this:

	for (i = max_ports; i < ARRAY_SIZE(priv->vlans); i++) {
		if (priv->vlans[i].bridge == bridge) {
			fid = priv->vlans[i].fid;
			break;
		}
	}

is not this:

	for (i = max_ports; i < ARRAY_SIZE(priv->vlans); i++) {
		if (priv->vlans[i].bridge == bridge &&
		    priv->vlans[i].vid == vid) {
			fid = priv->vlans[i].fid;
			break;
		}
	}

Re: net: dsa: lantiq_gswip: getting the first selftests to pass

[Hauke Mehrtens]

On 7/31/22 22:49, Martin Blumenstingl wrote:
> Hi Vladimir,
> 
> On Fri, Jul 29, 2022 at 2:05 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> [...]
>>> - disable learning on all ports
>>
>> Yes, here's just one other example of what can go wrong if it's enabled
>> on standalone ports, if you need to see it:
>> https://lore.kernel.org/netdev/20220727233249.fpn7gyivnkdg5uhe@skbuf/T/#m2e27a5385f70ee3440ee7f6250aaafdbfdc7446b
>>
>> Essentially every time when there's a chance that the switch will
>> receive on one port what another port has sent, learning will be a
>> problem. This is why it's also problematic for the selftests - because
>> we intentionally put 2 pairs of ports in loopback.
> Makes sense, thanks!
> 
>>> - disable unicast flooding on all ports
>>
>> I am having trouble saying 'yes' or 'no' to this because I don't know
>> exactly what you mean. By flooding a packet, I understand "if its MAC DA
>> is unknown to the FDB, deliver it to this set of ports". But flooding,
>> like learning, is essentially a bridging service concept, so it applies
>> only to packets coming from a particular bridging domain. In the case of
>> a standalone port, packets come only from the CPU, via the control
>> plane. Depending how the hardware is constructed, when you inject a
>> packet to a port, maybe there won't be any ifs or buts and the switch
>> will just deliver it there (I call this behavior: "control packets
>> bypass FDB lookup", or "CPU is in god mode"). So maybe it doesn't matter
>> whether unicast flooding is enabled on all standalone ports or not, as
>> long as the macroscopically expected behavior can be observed: if
>> software xmits a packet to a port, the packet gets delivered regardless
>> of MAC DA.
> I think I do understand it now.
> We want the defaults to apply to standalone ports. Since flooding does
> not exist there we should disable it (for both, unicast and
> broadcast/multicast traffic). When flooding is wanted on a specific
> port it'll be enabled through port_bridge_flags.
> 
> [...]
>>> - (GSWIP can only enable broadcast and multicast at the same time, so
>>> that's enabled too)
>>
>> I think the GSWIP would not be the only one in that category. The
>> mv88e6xxx driver puts the ff:ff:ff:ff:ff:ff address in the FDB and that
>> controls broadcast flooding, while the single knob that you mention
>> controls what's left - i.e. multicast.
> 
>>> I think skb->offload_fwd_mark needs to be set unless we know that the
>>> hardware wasn't able to forward the frame/packet.
>>> In the vendor sources I was able to find the whole RX tag structure: [0]
>>> I am not sure about the "mirror" bit (I assume this is: packet was
>>> received on this port because this port is configured as a mirroring
>>> target). All other bits seem irrelevant for skb->offload_fwd_mark -
>>> meaning we always have to set skb->offload_fwd_mark.
>>>
>>> I have lots of failures in bridge_vlan_aware.sh and
>>> bridge_vlan_unaware.sh - even before any of my changes - which I'll
>>> need to investigate.
>>
>> I don't remember the problems I faced while making these tests pass on
>> my hardware, and I also don't think they'll be the same as the ones
>> you'll face.
> I'll postpone bridge_vlan_unaware.sh investigations until I have the
> standalone tests (which are relevant for GSWIP, meaning: excluding the
> multicast ones) from local_termination.sh passing.
> 
> [...]
>>> - the DSA_DB_BRIDGE case is easy as this is basically what we had
>>> implemented before and I "just" need to look up the FID based on
>>> db.bridge.dev
>>
>> Or db.bridge.num (this is currently set to 0 by DSA because you don't
>> declare ds->fdb_isolation = true), whichever is more convenient.
> Using db.bridge.num will probably allow us to get rid of the
> priv->vlans array in the GSWIP driver. For now I'm using the bridge
> dev since "it works" until tests are passing.
> 
> [...]
>>> - DSA_DB_PORT for the CPU port: the port argument for port_fdb_add is
>>> the CPU port - but we can't map this to a FID (those are always tied
>>> to either a bridge or a user port). So instead I need to look at db.dp
>>> and for example use it's index for getting the FID (for standalone
>>> ports the FID is: port index + 1).
>>
>> Looking at db.dp to determine the FID is not a workaround, but rather
>> exactly what you are expected to do.
> Thanks for confirming!
> 
>>> That results in: we're requested to install the CPU ports MAC address
>>> on the CPU port (6),
>>
>> No. The CPU port doesn't have a MAC address (and in fact no port does;
>> it's a switch). But user ports have MAC addresses which are a purely
>> software construct to denote L2 termination. Every user port net device
>> can have its own MAC address, different from the other, and different
>> from the MAC address of the DSA master. Its interpretation is: "if a
>> user port receives a packet with a MAC DA that's equal to the net device's
>> MAC address, send the packet to the CPU, otherwise drop it".
>> It makes the standalone NIC illusion work.
>>
>> The CPU port is just a dumb pipe, it just transports packets to/from our
>> actual user ports. We don't have a termination point for it (or as written
>> in other places: "we don't have a net_device"), so no MAC address, not
>> even as a software construct.
>>
>> A pipe is exactly how you should see the CPU port. It doesn't have a FID
>> (a single port bridge) of its own because it is a part of all FIDs.
>>
>>> but what we actually do is install the FDB entry with the CPU port's
>>> MAC address on a user port (let's say 4, which we get from db.dp).
>>
>> No, quite the other way around.
>>
>> Let's take an example based on what you've described: user port swp4 has
>> MAC address 00:01:02:03:04:05, and CPU port is 6. You'll get a call to
>>
>> port_fdb_add(ds, port = 6, addr = 00:01:02:03:04:05, vid = 0,
>>               db = {type = DSA_DB_PORT, dp = swp4}).
>>
>> What you need to do is create an FDB entry on which only packets
>> received by swp4 in standalone mode will match (so it needs to have a
>> FID equal to the FID that swp4 classifies packets to, in standalone mode),
>> and delivers these packets to the CPU port 6, which is already in that FID,
>> as it is part of every FID. Remember, when swp4 receives a packet and is
>> standalone, it always assigns the FID of that packet to the value that
>> it's configured to (port index + 1, or 5, if you say so). This packet
>> in this FID can either find an entry in the FDB, case in which its
>> destination is certainly the CPU port (that's why port = 6), or the
>> address will be absent from the FDB, case in which the packet will be
>> flooded nowhere (the only other port in this FID, the CPU port, has
>> flooding turned off) => dropped.
>>
>> As mentioned earlier, it's desirable that packets delivered by software,
>> over the CPU port and towards a standalone one, are sent in "god mode",
>> so that the FDB won't be searched at all in that direction.
>>
>> You seem to have something reversed in your terminology, although I
>> can't exactly pinpoint what. When you say "install an FDB entry on port X",
>> what I understand is "make the packets with that FDB entry's MAC DA be
>> delivered towards port X". Or maybe I have something reversed?
>> I'm quite curious to know.
> Thanks a lot for explaining this (yet again)!
> There's three issues with my original sentence:
> - I should have used the term "user port's MAC address" instead of
> "CPU ports MAC"
> - "on the CPU port (6)" needs to be more precise, it should be
> "towards the CPU port (6)"
> - I'm not mentioning the source port (user port) number and thus FID at all
> 
> Also I need to get the idea out of my head that the CPU port is equal to eth0.
> It's not, eth0 is connected to the CPU port on the switch.
> 
> While working on my patches a more practical question came up while I
> was breaking the driver and then trying to make local_termination.sh
> pass again.
> At the start of run_tests for the standalone port scenario I am
> getting the following values:
>    rcv_dmac = 00:01:02:03:04:02
>    MACVLAN_ADDR = 00:00:de:ad:be:ef
> My expectation is that port_fdb_add() is called with these MAC
> addresses. I verified that dsa_switch_supports_uc_filtering() returns
> true, but still I
> 
>>> Now if a packet/frame should target the CPU port we don't need
>>> flooding because the switch knows the destination port based on the
>>> FDB entry we installed.
>>
>> Yes, so rather than the CPU port being a 'dumb' pipe which passes all
>> packets through it, you're making it a slightly 'smarter' pipe which
>> essentially uses the FDB as an RX filter for standalone user ports.
>>
>>> Also I would like to point out that I am still doing all of this in my
>>> spare time.
>>
>> I'm doing this in my spare time as well, and I'm having fun while at it.
>> Sorry for being handwavy and insisting only on explaining the general
>> idea rather than opening the GSWIP manual and checking that what I'm
>> saying is actually implementable. [...]
> I fully understand this and it makes sense as others can also benefit
> from your explanation (since it's generic, not driver specific).
> 
>> I'll do so if you have a specific question about something apparently
>> not mapping to the expectations.
> I still have an issue which I believe is related to the FDB handling.
> 
> I *think* that I have implemented FDB isolation correctly in my
> work-in-progress branch [0].
> 
> The GSWIP140 datasheet (page 82) has a "MAC Learning disable and MAC
> Learning Limitation Description" (table 26).
> In the xRX200 vendor kernel I cannot find the LNDIS bit in
> PCE_PCTRL_3, so I suspect it has only been added in newer GSWIP
> revisions (xRX200 is at least one major IP revision behind GSW140).
> Maybe Hauke knows?
> So what I'm doing to disable learning is setting the "learning limit"
> (which limits the number of entries that can be learned) for that port
> to zero.

There is no LNDIS bit in PCE_PCTRL_3 on the VR9 as far as I know.

What do you want to archive?

If you want to forward the packets only, but not learn the source mac 
address you can configure the PSTATE register inb PCTRL_0 to forwarding 
instead of learning. This is already implemented in 
gswip_port_stp_state_set().
> 
>   My problem is that whenever I disable learning a lot of tests from
> local_termination.sh are failing, including:
> - TEST: lan2: Unicast IPv4 to primary MAC address
> - TEST: lan2: Unicast IPv4 to macvlan MAC address
> 
> Setting the PLIMMOD bit to 1 means that GSWIP won't drop the packet if
> the learning limit is exceeded (the default value seems to be 0).
> This at least works around the first failing test (Unicast IPv4 to
> primary MAC address).
> 
> Based on your understanding of my issue: I am going in the right
> direction when I'm saying that this is an FDB issue?
> 
> 
> Thank you!
> Martin
> 
> 
> [0] https://github.com/xdarklight/linux/commits/lantiq-gswip-integration-20220730

Re: net: dsa: lantiq_gswip: getting the first selftests to pass

[Hauke Mehrtens]

Hi Martin,


On 7/29/22 00:09, Martin Blumenstingl wrote:
......
> The vendor driver lists a bunch of possible tables: [1]
> I'll leave any effort for multicast out for now... it's not the main
> use-case I am looking at right now.

Most of these tables are needed because the switch HW can do layer 3 
(IP) multicast handling. I think normally Linux takes care of this with 
DSA drivers and we only tell the switch where to forward based on MAC 
addresses.

> As always: thank you for your amazing explanations, hints and pointers!
> Also I would like to point out that I am still doing all of this in my
> spare time. Whenever you have other conversations to focus on: please
> do so! For me it's not critical if you're getting back to me a few
> days sooner or later.
> 
> 
> Best regards,
> Martin
> 
> 
> [0] https://github.com/paldier/K3C/blob/ca7353eb397090c363632409d9ca568d3cca09c7/ugw/target/linux/lantiq/patches-3.10/7000-NET-lantiq-adds-eth-drv.patch#L2238-L2259
> [1] https://github.com/paldier/K3C/blob/ca7353eb397090c363632409d9ca568d3cca09c7/ugw/target/linux/lantiq/files/drivers/net/ethernet/lantiq/switch-api/ltq_flow_core.h#L164-L192

Re: net: dsa: lantiq_gswip: getting the first selftests to pass

[Hauke Mehrtens]

On 7/28/22 00:44, Vladimir Oltean wrote:
> Hi Martin,
> 
> On Wed, Jul 27, 2022 at 10:36:55PM +0200, Martin Blumenstingl wrote:
>> Hello,
>>
.....
>> Vladimir suggested in [0]:
>>> [...] we'll need to make smaller steps, like disable address
>>> learning on standalone ports, isolate FDBs, maybe offload the bridge TX
>>> forwarding process (in order to populate the "Force no learning" bit in
>>> tag_gswip.c properly), and only then will the local_termination test
>>> also pass [...]
>>
>> Based on the failing tests I am wondering which step would be a good
>> one to start with.
>> Is this problem that the selftests are seeing a flooding issue? In
>> that case I suspect that the "interesting behavior" (of the GSWIP's
>> flooding behavior) that Vladimir described in [1] would be a starting
>> point.
> 
> It has to do with that, yes. What I said there is that the switch
> doesn't autonomously flood unknown packets from one bridged port to
> another, but instead, sends them to the CPU and lets the CPU do it.
> 
> While that is perfectly respectable from a correctness point of view,
> it is also not optimal if you consider performance. The selftests here
> try to capture the fact that the switch doesn't send unknown packets to
> the CPU. And in this case the driver sends them by construction.

This was done intentional, the driver configures the switch to send all 
unknown unicast and unknown multicast packets to the CPU.
The PMAP_3 register configures the target port of unicast packets where 
the destination mac address is not found in the mac forwarding table.
The PMAP_2 register configures where the switch should send multicast 
packets without a destination mac in the mac table.
The PMAP_1 register configures the destination port where all packets 
are forwarded (mirrored) to.

If the packets are mirrored a special bit will be set in the special CPU 
tag. (Bit 0 in Byte 3). I think this will only be set when it is 
forwarded using PMAP_1 register.

I think we can not configure this per source port.

> So the absolute first step would be to control the bridge port flags
> (BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD) and start
> with good defaults for standalone mode (also set skb->offload_fwd_mark
> when appropriate in the tagging protocol driver). I think you can use
> bridge_vlan_aware.sh and bridge_vlan_unaware.sh as starting points to
> check that these flags still work fine after you've offloaded them to
> hardware.

I think it is not possible to configure a per source port flooding.

We can configure a port into these modes:
* listen only
* transmit disable, receive enable
* transmit enable, receive disable
* learning
* forwarding

> When flooding a packet to find its destination can be achieved without
> involving the CPU (*), the next thing will be to simply disable flooding
> packets of all kind to the CPU (except broadcast). That's when you'll
> enjoy watching how all the local_termination.sh selftests fail, and
> you'll be making them pass again, one by one.
> 
>>
>> Full local_termination.sh selftest output:
>> TEST: lan2: Unicast IPv4 to primary MAC address                 [ OK ]
> 
> For this to pass, the driver must properly respond to a port_fdb_add()
> on the CPU port, with the MAC address of the $swp1 user port's net device,
> offloaded in the DSA_DB_PORT corresponding to $swp1.

I think this is already done.

> In turn, for DSA to even consider passing you FDB entries in DSA_DB_PORT,
> you must make dsa_switch_supports_uc_filtering() return true.
> 
> (if you don't know what the words here mean, I've updated the documentation at
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/tree/Documentation/networking/dsa/dsa.rst)
> 
.....
> 
>> TEST: lan2: Unicast IPv4 to unknown MAC address, promisc        [ OK ]
> 
> Now this passes because the expectation of promiscuous ports is to
> receive all packets regardless of MAC DA, that's the definition of
> promiscuity. The driver currently already floods to the CPU, so why
> wouldn't this pass.
> 
> Here, what we actually want to capture is that dsa_slave_manage_host_flood(),
> which responds to changes in the IFF_PROMISC flag on a user port, does
> actually notify the driver via a call to port_set_host_flood() for that
> user port. Through this method, the driver is responsible for turning
> flooding towards the CPU port(s) on or off, from the user port given as
> argument. If CPU flood control does not depend on user port, then you'll
> have to keep CPU flooding enabled as long as any user port wants it.

We could use the mirror feature here.
We could activate port mirror with the CPU port as target and only 
activate receive mirroring on ports where we activate IFF_PROMISC.
....
> 
>> TEST: lan2: Multicast IPv4 to joined group                      [ OK ]
> 
> Here, I used a trivial program I found online to emit a IP_ADD_MEMBERSHIP
> setsockopt, to trigger the kernel code that calls dev_mc_add() on the
> net device. It seems to not be possible by design to join an IP
> multicast group using a dedicated command in a similar way to how you'd
> add an FDB entry on a port; instead the kernel joins the multicast group
> for as long as the user application persists, and leaves the group afterwards.
> 
> As you can probably guess, dev_mc_add() calls made by modules outside
> DSA are translated by dsa_slave_set_rx_mode() into a port_mdb_add() on
> the CPU port, with DSA_DB_PORT.
> 
> If the gswip driver doesn't implement port_mdb_add() but rather treats
> multicast as broadcast (by sending it to the CPU), naturally this test
> "passes" in the sense that it thinks the driver reacted properly to what
> was asked.

Yes port_mdb_add() is not implemented yet, but should be easy, it is the 
same as port_fdb_add() it just allows multiple destinations.