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

On 18.02.2020 16:02, Vladimir Oltean wrote:

The problem is on RX.

quoted

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

For the unicast packet.

When you have it working (in your setup, with your patch applied). Does
the ping reply packet have an IFH (DSA-tag)?

Or is it a frame on the NPI port without an IFH.

This is important as this will tell us of the frame was copied to CPU
and then redirected to the NPI port, or if it was plain forwarded.

I need to understand the problem better before trying to solve it.

quoted

quoted

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.

quoted

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.

quoted

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.

It looks like you disagree with having BIT(ocelot->cpu) in PGID_SRC +
p (the forwarding matrix) and just want to rely on whitelisting
towards PGID_CPU*?

Yes.

When the port is not member of the bridge, it should act as a normal NIC
interface.

With this change frames are being forwarded even when the port is not
member of the bridge. This may be what you want in a DSA (or may not -
not sure), but it is not ideal in the Ocelot/switchdev solution as we
want to use the MAC-table to do the RX filtering.

But you already have that logic present in your driver, it's just not
called from a useful place for Felix.
So it logically follows that we should remove these lines from
ocelot_bridge_stp_state_set, no?

           } else {
                   /* Only the CPU port, this is compatible with link
                    * aggregation.
                    */
                   ocelot_write_rix(ocelot,
                                    BIT(ocelot->cpu),
                                    ANA_PGID_PGID, PGID_SRC + p);

This should not be removed. When the port is member of the bridge this
bit must be set. When it is removed it must be cleared again.

*I admit that I have no idea why it works for you, and why the frames
learned towards PGID_CPU are forwarded to the CPU _despite_
BIT(ocelot->cpu) not being present in PGID_SRC + p.

I believe this is because we have the MAC address in the MAC table.

It seems that you want to use learning to forward frames to the CPU,
also in the case when the port is not a member of the bridge. I'm not
too keen on this, mainly because I'm not sure how well it will work. If
you are certain this is what you want for Felix then lets try find a way
to make it happend for Felix without chancing the behaivural for Ocelot.

An alternative solution would be to use the MAC-table for white listing
of unicast packets. But as I understand the thread this is not so easy
to do with DSA. Sorry, I do not know DSA very well, and was not able to
fully understand why. But this is as far as I know the only way to get
the proper RX filtering.

An other solution, is to skip the RX filtering, and when a port is not
member of a beidge set the 'ANA:PORT[0-11]:CPU_FWD_CFG.CPU_SRC_COPY_ENA'
bit. This will cause all fraems to be copied to the CPU. Again, we need
to find a way to do this which does not affect Ocelot.

/Allan