Re: [PATCH net-next] net: mscc: ocelot: Workaround to allow traffic to CPU in standalone mode

On 18.02.2020 14:29, Vladimir Oltean wrote:

quoted

quoted

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 86d543ab1ab9..94d39ccea017 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c

@@ -2297,6 +2297,18 @@ void ocelot_set_cpu_port(struct ocelot *ocelot, int cpu,
                        enum ocelot_tag_prefix injection,
                        enum ocelot_tag_prefix extraction)

{
+       int port;
+
+       for (port = 0; port < ocelot->num_phys_ports; port++) {
+               /* Disable old CPU port and enable new one */
+               ocelot_rmw_rix(ocelot, 0, BIT(ocelot->cpu),
+                              ANA_PGID_PGID, PGID_SRC + port);

I do not understand why you have an "old" CPU. The ocelot->cpu field is
not initialized at this point (at least not in case of Ocelot).

Are you trying to move the NPI port?

Yes, that's what this function does. It sets the NPI port. It should
be able to work even if called multiple times (even though the felix
and ocelot drivers both call it exactly one time).
But I can (and will) remove/simplify the logic for the "old" CPU port.
I had the patch formatted already, and I didn't want to change it
because I was lazy to re-test after the changes.

quoted

quoted

+               if (port == cpu)
+                       continue;
+               ocelot_rmw_rix(ocelot, BIT(cpu), BIT(cpu),
+                              ANA_PGID_PGID, PGID_SRC + port);

So you want all ports to be able to forward traffic to your CPU port,
regardless of if these ports are member of a bridge...

Yes.

quoted

I have read through this several times, and I'm still not convinced I
understood it.

Can you please provide a specific example of how things are being
forwarded (wrongly), and describe how you would like them to be
forwarded.

Be there 4 net devices: swp0, swp1, swp2, swp3.
At probe time, the following doesn't work on the Felix DSA driver:
ip addr add 192.168.1.1/24 dev swp0
ping 192.168.1.2

This does work with Ocelot, without your patch. I would like to
understand why this does not work in your case.

Is it in RX or TX you have the problem.

Is it with the broadcast ARP, or is it the following unicast packet?

But if I do this:
ip link add dev br0 type bridge
ip link set dev swp0 master br0
ip link set dev swp0 nomaster
ping 192.168.1.2
Then it works, because the code path from ocelot_bridge_stp_state_set
that puts the CPU port in the forwarding mask of the other ports gets
executed on the "bridge leave" action.
The whole point is to have the same behavior at probe time as after
removing the ports from the bridge.

This does sound like a bug, but I still do not agree in the solution.

The code with ocelot_mact_learn towards PGID_CPU for the MAC addresses
of the switch port netdevices is all bypassed in Felix DSA. Even if it
weren't, it isn't the best solution.
On your switch, this test would probably work exactly because of that
ocelot_mact_learn.

So I guess it is the reception of the unicast packet which is causing
problems.

But try to receive packets sent at any other unicast DMAC immediately
after probe time, and you should see them in tcpdump but won't.

That is true - this is because we have no way of implementing promisc
mode, which still allow us to HW offload of the switching. We discussed
this before.

Long story short, it sounds like you have an issue because the
Felix/DSA driver behave differently than the Ocelot. Could you try to do
your fix such that it only impact Felix and does not change the Ocelot
behavioral.

/Allan