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

["Allan W. Nielsen" <allan.nielsen@microchip.com>]

On 18.02.2020 14:29, Vladimir Oltean wrote:
>> >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.
>
>> >+               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.
>
>> 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