On 16/01/2021 02.42, Tobias Waldekranz wrote:

On Thu, Jan 14, 2021 at 14:49, Rasmus Villemoes [off-list ref] wrote:

quoted

Hi

I've noticed something rather odd with my mv88e6250, which led me to the
commit in the subject.

First, the MAC address of the master device never seems to get learned
(at least according to "mv88e6xxx_dump --atu"), so all packets destined
for the machine gets flooded out all ports 

[snip]

quoted

the master device's address doesn't get learned (nor does some garbage
address appear in the ATU), and the unicast packets are still forwarded
out all ports. So I must be missing something else.

The thing you are missing is that all packets from the CPU are sent with
FROM_CPU tags. SA learning is not performed on these as it intended for
control traffic.

Ah, yes, I do remember stumbling on the tagger using FROM_CPU and
wondering about that at some point. And I didn't recall the somewhat
subtle detail of those being treated as MGMT and thus not participating
in SA learning.

Ideally, bulk traffic would be sent with a FORWARD tag. But there is
currently no way for the DSA tagger to discriminate the bulk data from
control traffic. And changing that is no small task.

Indeed.

In the mean time we could extend Vladimir's (added to CC) work on
assisted CPU port learning to include the local bridge addresses. You
pushed me to take a first stab at this :) Please have a look at this
series:

https://lore.kernel.org/netdev/20210116012515.3152-1-tobias@waldekranz.com/ (local)

I'll try these out, thanks. FWIW, in an earlier BSP there were some
horrible hacks to go behind the kernel's back and add the CPU's address
as a static entry in the switch, which is why I haven't seen this
before. And this was done for much the same reason as we will have to it
now (it implemented ERPS, another ring redundancy protocol).

quoted

Finally, I'm wondering how the tagging could get in the way of learning
the right address, given that the tag is inserted after the DA and SA.

Yes, but the CPU port is configured in DSA mode, so the switch will use
the tag command (FROM_CPU) to determine if learning should be done or
not.

Right, but that comment was directed at commit 4c7ea3c0791e; even if SA
learning did happen, bytes 6-11 of the frame are the same with or
without the tag added, so I don't understand how _corrupted_ addresses
could get learned.

quoted

What I'm _really_ trying to do is to get my mv88e6250 to participate in
an MRP ring, which AFAICT will require that the master device's MAC gets
added as a static entry in the ATU: Otherwise, when the ring goes from
open to closed, I've seen the switch wrongly learn the node's own mac
address as being in the direction of one of the normal ports, which
obviously breaks all traffic.

Well the static entry for the bridge MAC should be installed with the
aforementioned series applied. So that should not be an issue.

Yes, so there are several good reasons for adding that address
statically to the hardware's database.

My guess is that MRP will still not work though, as you will probably
need the ability to trap certain groups to the CPU (management
entries). I.e. some MRP PDUs must be allowed to ingress on blocked
ports, no?

Indeed, and I'm currently just using a not-for-mainline patch that
hardcodes the two multicast addresses in the ATU.

--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c

@@ -1911,6 +1911,24 @@ static int mv88e6xxx_broadcast_setup(struct

mv88e6xxx_chip *chip, u16 vid)
                err = mv88e6xxx_port_add_broadcast(chip, port, vid);
                if (err)
                        return err;
+
+               if (port != dsa_upstream_port(chip->ds, port))
+                       continue;
+               if (IS_ENABLED(CONFIG_BRIDGE_MRP)) {
+                       static const u8 mrp_dmac[][ETH_ALEN] = {
+                               { 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 },
+                               { 0x1, 0x15, 0x4e, 0x0, 0x0, 0x3 },
+                       };
+                       const u8 state =
MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC_DA_MGMT;
+                       int i;
+
+                       for (i = 0; i < ARRAY_SIZE(mrp_dmac); ++i) {
+                               err = mv88e6xxx_port_db_load_purge(chip,
port,
+
mrp_dmac[i], vid, state);
+                               if (err)
+                                       return err;
+                       }
+               }
        }

        return 0;

because yes, one needs to prevent those frames from being flooded out
all ports automatically.

I suppose the real solution is having userspace do some "bridge mdb add"
yoga, but since no code currently uses
MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC_DA_MGMT, I don't think there's any
way to actually achieve this. And I have no idea how to represent the
requirement that "frames with this multicast DA are only to be directed
at the CPU" in a hardware-agnostic way.

Rasmus