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

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

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The Ocelot switches have what is, in my opinion, a design flaw: their
DSA header is in front of the Ethernet header, which means that they
subvert the DSA master's RX filter, which for all practical purposes,
either needs to be in promiscuous mode, or the OCELOT_TAG_PREFIX_LONG
needs to be used for extraction, which makes the switch add a fake DMAC
of ff:ff:ff:ff:ff:ff so that the DSA master accepts the frame.

The issue with this design, of course, is that the CPU will be spammed
with frames that it doesn't want to respond to, and there isn't any
hardware offload in place by default to drop them.

What is being done in the VSC7514 Ocelot driver is a process of
selective whitelisting. The "MAC address" of each Ocelot switch net
device, with all VLANs installed on that port, is being added as a FDB
entry towards PGID_CPU.

PGID_CPU is is a multicast set containing only BIT(cpu). I don't know
why it was chosen to be a multicast PGID (59) and not simply the unicast
one of this port, but it doesn't matter. The point is that the the CPU
port is special, and frames are "copied" to the CPU, disregarding the
source masks (third PGID lookup), if BIT(cpu) is found to be set in the
destination masks (first PGID lookup).

Frames that match the FDB will go to PGID_CPU by virtue of the DEST_IDX
from the respective MAC table entry, and frames that don't will go to
PGID_UC or PGID_MC, by virtue of the FLD_UNICAST, FLD_BROADCAST etc
settings for flooding. And that is where the distinction is made:
flooded frames will be subject to the third PGID lookup, while frames
that are whitelisted to the PGID_CPU by the MAC table aren't.

So we can use this mechanism to simulate an RX filter, given that we are
subverting the DSA master's implicit one, as mentioned in the first
paragraph. But this has some limitations:

- In Ocelot each net device has its own MAC address. When simulating
  this with MAC table entries, it will practically result in having N
  MAC addresses for each of the N front-panel ports (because FDB entries
  are not per source port). A bit strange, I think.

- In DSA we don't have the infrastructure in place to support this
  whitelisting mechanism. Calling .port_fdb_add on the CPU port for each
  slave net device dev_addr isn't, in itself, hard. The problem is with
  the VLANs that this port is part of. We would need to keep a duplicate
  list of the VLANs from the bridge, plus the ones added from 8021q, for
  each port. And we would need reference counting on each MAC address,
  such that when a front-panel port changes its MAC address and we need
  to delete the old FDB entry, we don't actually delete it if the other
  front-panel ports are still using it. Not to mention that this FDB
  entry would have to be added on the whole net of upstream DSA switches.

- Cascading a different DSA switch that has tags before the Ethernet
  header would not possibly work if we rely on the whitelisting
  mechanism exclusively.

So... it's complicated. What this patch does is to simply allow frames
to be flooded to the CPU, which is anyway what the Ocelot driver is
doing after removing the bridge from the net devices, see this snippet
from ocelot_bridge_stp_state_set:

    /* Apply FWD mask. The loop is needed to add/remove the current port as
     * a source for the other ports.
     */
    for (p = 0; p < ocelot->num_phys_ports; p++) {
            if (p == ocelot->cpu || (ocelot->bridge_fwd_mask & BIT(p))) {
                    (...)
            } else {
                    /* Only the CPU port, this is compatible with link
                     * aggregation.
                     */
                    ocelot_write_rix(ocelot,
                                     BIT(ocelot->cpu),
                                     ANA_PGID_PGID, PGID_SRC + p);
            }

Otherwise said, the ocelot driver itself is already not self-coherent,
since immediately after probe time, and immediately after removal from a
bridge, it behaves in different ways, although the front panel ports are
standalone in both cases.

While standalone traffic _does_ work for the Felix DSA wrapper after
enslaving and removing the ports from a bridge, this patch makes
standalone traffic work at probe time too, with the caveat that even
irrelevant frames will get processed by software, making it more
susceptible to potential denial of service.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

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);
+		if (port == cpu)
+			continue;
+		ocelot_rmw_rix(ocelot, BIT(cpu), BIT(cpu),
+			       ANA_PGID_PGID, PGID_SRC + port);
+	}
+
 	/* Configure and enable the CPU port. */
 	ocelot_write_rix(ocelot, 0, ANA_PGID_PGID, cpu);
 	ocelot_write_rix(ocelot, BIT(cpu), ANA_PGID_PGID, PGID_CPU);
-- 
2.17.1

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

[Allan W. Nielsen]

On 17.02.2020 17:00, Vladimir Oltean wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
>The Ocelot switches have what is, in my opinion, a design flaw: their
>DSA header is in front of the Ethernet header, which means that they
>subvert the DSA master's RX filter, which for all practical purposes,
>either needs to be in promiscuous mode, or the OCELOT_TAG_PREFIX_LONG
>needs to be used for extraction, which makes the switch add a fake DMAC
>of ff:ff:ff:ff:ff:ff so that the DSA master accepts the frame.
>
>The issue with this design, of course, is that the CPU will be spammed
>with frames that it doesn't want to respond to, and there isn't any
>hardware offload in place by default to drop them.
In the case of Ocelot, the NPI port is expected to be connected back to
back to the CPU, meaning that it should not matter what DMAC is set.

It was also my understanding that this is how you have connected this.

I'm not able to see why this would cause spamming.

>What is being done in the VSC7514 Ocelot driver is a process of
>selective whitelisting. The "MAC address" of each Ocelot switch net
>device, with all VLANs installed on that port, is being added as a FDB
>entry towards PGID_CPU.
>
>PGID_CPU is is a multicast set containing only BIT(cpu). I don't know
>why it was chosen to be a multicast PGID (59) and not simply the unicast
>one of this port, but it doesn't matter. The point is that the the CPU
>port is special, and frames are "copied" to the CPU, disregarding the
>source masks (third PGID lookup), if BIT(cpu) is found to be set in the
>destination masks (first PGID lookup).
>
>Frames that match the FDB will go to PGID_CPU by virtue of the DEST_IDX
>from the respective MAC table entry, and frames that don't will go to
>PGID_UC or PGID_MC, by virtue of the FLD_UNICAST, FLD_BROADCAST etc
>settings for flooding. And that is where the distinction is made:
>flooded frames will be subject to the third PGID lookup, while frames
>that are whitelisted to the PGID_CPU by the MAC table aren't.
>
>So we can use this mechanism to simulate an RX filter, given that we are
>subverting the DSA master's implicit one, as mentioned in the first
>paragraph. But this has some limitations:
>
>- In Ocelot each net device has its own MAC address. When simulating
>  this with MAC table entries, it will practically result in having N
>  MAC addresses for each of the N front-panel ports (because FDB entries
>  are not per source port). A bit strange, I think.
>
>- In DSA we don't have the infrastructure in place to support this
>  whitelisting mechanism. Calling .port_fdb_add on the CPU port for each
>  slave net device dev_addr isn't, in itself, hard. The problem is with
>  the VLANs that this port is part of. We would need to keep a duplicate
>  list of the VLANs from the bridge, plus the ones added from 8021q, for
>  each port. And we would need reference counting on each MAC address,
>  such that when a front-panel port changes its MAC address and we need
>  to delete the old FDB entry, we don't actually delete it if the other
>  front-panel ports are still using it. Not to mention that this FDB
>  entry would have to be added on the whole net of upstream DSA switches.
>
>- Cascading a different DSA switch that has tags before the Ethernet
>  header would not possibly work if we rely on the whitelisting
>  mechanism exclusively.
>
>So... it's complicated. What this patch does is to simply allow frames
>to be flooded to the CPU, which is anyway what the Ocelot driver is
>doing after removing the bridge from the net devices, see this snippet
>from ocelot_bridge_stp_state_set:
>
>    /* Apply FWD mask. The loop is needed to add/remove the current port as
>     * a source for the other ports.
>     */
>    for (p = 0; p < ocelot->num_phys_ports; p++) {
>            if (p == ocelot->cpu || (ocelot->bridge_fwd_mask & BIT(p))) {
>                    (...)
>            } else {
>                    /* Only the CPU port, this is compatible with link
>                     * aggregation.
>                     */
>                    ocelot_write_rix(ocelot,
>                                     BIT(ocelot->cpu),
>                                     ANA_PGID_PGID, PGID_SRC + p);
>            }
>
>Otherwise said, the ocelot driver itself is already not self-coherent,
>since immediately after probe time, and immediately after removal from a
>bridge, it behaves in different ways, although the front panel ports are
>standalone in both cases.
Maybe you found a bug, maybe you have different expectations.

The idea is that after probe time all the ports must behave as NIC
devices. No forwarding are being done, and all traffic is copied to the
CPU.

When a port is added to the bridge, the given ports-bit must be set in
the PGID_SRC.

As I read the code, this seems to be done right. If you believe you have
found a bug regarding this then please clarify this a bit.

>While standalone traffic _does_ work for the Felix DSA wrapper after
>enslaving and removing the ports from a bridge, this patch makes
>standalone traffic work at probe time too, with the caveat that even
>irrelevant frames will get processed by software, making it more
>susceptible to potential denial of service.
>
>Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
>---
> drivers/net/ethernet/mscc/ocelot.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
>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?

>+               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...

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.

/Allan

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

[Vladimir Oltean]

Hi Allan,

On Tue, 18 Feb 2020 at 13:32, Allan W. Nielsen
<allan.nielsen@microchip.com> wrote:
>
> On 17.02.2020 17:00, Vladimir Oltean wrote:
> >EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> >From: Vladimir Oltean <vladimir.oltean@nxp.com>
> >
> >The Ocelot switches have what is, in my opinion, a design flaw: their
> >DSA header is in front of the Ethernet header, which means that they
> >subvert the DSA master's RX filter, which for all practical purposes,
> >either needs to be in promiscuous mode, or the OCELOT_TAG_PREFIX_LONG
> >needs to be used for extraction, which makes the switch add a fake DMAC
> >of ff:ff:ff:ff:ff:ff so that the DSA master accepts the frame.
> >
> >The issue with this design, of course, is that the CPU will be spammed
> >with frames that it doesn't want to respond to, and there isn't any
> >hardware offload in place by default to drop them.
> In the case of Ocelot, the NPI port is expected to be connected back to
> back to the CPU, meaning that it should not matter what DMAC is set.
>

You are omitting the fact that the host Ethernet port has an RX filter
as well. By default it should drop frames that aren't broadcast or
aren't sent to a destination MAC equal to its configured MAC address.
Most DSA switches add their tag _after_ the Ethernet header. This
makes the DMAC and SMAC seen by the front-panel port of the switch be
the same as the DMAC and SMAC seen by the host port. Combined with the
fact that DSA sets up switch port MAC addresses to be inherited from
the host port, RX filtering 'just works'.

> It was also my understanding that this is how you have connected this.
>

Yup.

> I'm not able to see why this would cause spamming.
>

With the Ocelot switch adding just the tag on extraction, the host
port would interpret random garbage as the DMAC (basically first 6
bytes of the extraction header whose format is detailed in
net/dsa/tag_ocelot.c.
So it would drop basically all packets, unless by pure chance, the
first 6 bytes of the extraction header form a pattern that is equal to
the MAC address configured on the host port (highly unlikely).
With the Ocelot switches, the host port should really be
unconditionally put in promiscuous mode. DSA doesn't support
specifying this yet, because no other switch has needed this.
Another workaround (the one currently used now) is to use this long
prefix format, which makes the Ocelot switch add some fake DMAC and
SMAC before the extraction header, such that the frames will pass
through any host port RX filter.
Using either one of the 2 workarounds (promiscuous mode or long
prefix), the advantage and the disadvantage is the same: the host port
can no longer drop frames in hardware for unknown DMACs (either
because it doesn't know where the real DMAC is, or because it is told
not to drop them).
The disadvantage is that a mechanism that worked implicitly so far for
every other DSA switch does not work for Ocelot. Of course it causes
spamming only if you send traffic towards a DMAC that is irrelevant to
the CPU.

> >What is being done in the VSC7514 Ocelot driver is a process of
> >selective whitelisting. The "MAC address" of each Ocelot switch net
> >device, with all VLANs installed on that port, is being added as a FDB
> >entry towards PGID_CPU.
> >
> >PGID_CPU is is a multicast set containing only BIT(cpu). I don't know
> >why it was chosen to be a multicast PGID (59) and not simply the unicast
> >one of this port, but it doesn't matter. The point is that the the CPU
> >port is special, and frames are "copied" to the CPU, disregarding the
> >source masks (third PGID lookup), if BIT(cpu) is found to be set in the
> >destination masks (first PGID lookup).
> >
> >Frames that match the FDB will go to PGID_CPU by virtue of the DEST_IDX
> >from the respective MAC table entry, and frames that don't will go to
> >PGID_UC or PGID_MC, by virtue of the FLD_UNICAST, FLD_BROADCAST etc
> >settings for flooding. And that is where the distinction is made:
> >flooded frames will be subject to the third PGID lookup, while frames
> >that are whitelisted to the PGID_CPU by the MAC table aren't.
> >
> >So we can use this mechanism to simulate an RX filter, given that we are
> >subverting the DSA master's implicit one, as mentioned in the first
> >paragraph. But this has some limitations:
> >
> >- In Ocelot each net device has its own MAC address. When simulating
> >  this with MAC table entries, it will practically result in having N
> >  MAC addresses for each of the N front-panel ports (because FDB entries
> >  are not per source port). A bit strange, I think.
> >
> >- In DSA we don't have the infrastructure in place to support this
> >  whitelisting mechanism. Calling .port_fdb_add on the CPU port for each
> >  slave net device dev_addr isn't, in itself, hard. The problem is with
> >  the VLANs that this port is part of. We would need to keep a duplicate
> >  list of the VLANs from the bridge, plus the ones added from 8021q, for
> >  each port. And we would need reference counting on each MAC address,
> >  such that when a front-panel port changes its MAC address and we need
> >  to delete the old FDB entry, we don't actually delete it if the other
> >  front-panel ports are still using it. Not to mention that this FDB
> >  entry would have to be added on the whole net of upstream DSA switches.
> >
> >- Cascading a different DSA switch that has tags before the Ethernet
> >  header would not possibly work if we rely on the whitelisting
> >  mechanism exclusively.
> >
> >So... it's complicated. What this patch does is to simply allow frames
> >to be flooded to the CPU, which is anyway what the Ocelot driver is
> >doing after removing the bridge from the net devices, see this snippet
> >from ocelot_bridge_stp_state_set:
> >
> >    /* Apply FWD mask. The loop is needed to add/remove the current port as
> >     * a source for the other ports.
> >     */
> >    for (p = 0; p < ocelot->num_phys_ports; p++) {
> >            if (p == ocelot->cpu || (ocelot->bridge_fwd_mask & BIT(p))) {
> >                    (...)
> >            } else {
> >                    /* Only the CPU port, this is compatible with link
> >                     * aggregation.
> >                     */
> >                    ocelot_write_rix(ocelot,
> >                                     BIT(ocelot->cpu),
> >                                     ANA_PGID_PGID, PGID_SRC + p);
> >            }
> >
> >Otherwise said, the ocelot driver itself is already not self-coherent,
> >since immediately after probe time, and immediately after removal from a
> >bridge, it behaves in different ways, although the front panel ports are
> >standalone in both cases.
> Maybe you found a bug, maybe you have different expectations.
>
> The idea is that after probe time all the ports must behave as NIC
> devices. No forwarding are being done, and all traffic is copied to the
> CPU.
>

Yup.

> When a port is added to the bridge, the given ports-bit must be set in
> the PGID_SRC.
>

Yup.

> As I read the code, this seems to be done right. If you believe you have
> found a bug regarding this then please clarify this a bit.
>

Writing BIT(ocelot->cpu) into PGID_SRC + p is only done from
ocelot_bridge_stp_state_set. That function does not get called at
probe time. So BIT(ocelot->cpu) is not present in PGID_SRC + p at
probe time.

> >While standalone traffic _does_ work for the Felix DSA wrapper after
> >enslaving and removing the ports from a bridge, this patch makes
> >standalone traffic work at probe time too, with the caveat that even
> >irrelevant frames will get processed by software, making it more
> >susceptible to potential denial of service.
> >
> >Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> >---
> > drivers/net/ethernet/mscc/ocelot.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> >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
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.

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. 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.

>
> /Allan
>

Regards,
-Vladimir

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

[Allan W. Nielsen]

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

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

[Vladimir Oltean]

On Tue, 18 Feb 2020 at 15:48, Allan W. Nielsen
<allan.nielsen@microchip.com> wrote:
>
> 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.
>

The problem is on RX.

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

For the 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.

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*?
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);

*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.

>
> /Allan
>

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

[Allan W. Nielsen]

On 18.02.2020 16:02, Vladimir Oltean wrote:
>The problem is on RX.
>
>> 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.

>> >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.
>
>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

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

[Vladimir Oltean]

Hi Allan,

On Wed, 19 Feb 2020 at 12:11, Allan W. Nielsen
<allan.nielsen@microchip.com> wrote:
>
> On 18.02.2020 16:02, Vladimir Oltean wrote:
> >The problem is on RX.
> >
> >> Is it with the broadcast ARP, or is it the following unicast packet?
> >For the unicast packet.

My bad, it is not just the unicast packet that is dropped, the
broadcast ARP packet is dropped too. All get dropped with the
"drop_local" counter on the front-panel port, the "no valid
destinations" counter.

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

FYI any packet sent through a swpN net device (an Ocelot/Felix net
device, that is) will have an Injection Frame Header.

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

No, I'm not trying to ping over an IP set on the DSA master interface,
if that's what you're asking.

> 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.
>

IFH is "injection" (host -> switch).
I suppose you meant Extraction Frame Header?
Nothing is seen by tcpdumping on the DSA master interface either.
Moreover, the "drop_local" counter on the front-panel swp0 increments
proportionally with the number of packets that the CPU should be
seeing.

By the way, could you please clarify the mechanisms by which the
switch will add an EFH on some but not all packets sent towards the
NPI port? The fact that this is possible is news to me.

> I need to understand the problem better before trying to solve it.
>
> >> >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.
> >
> >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.

Yes, but why keep BIT(ocelot->cpu) in the "valid destinations for this
source port p" mask (PGID_SRC + p) at all, if you just explained above
that you don't need it, because the MAC table takes care of getting
frames copied to the CPU, and there are dubious loopholes in the
hardware implementation that make this possible even if the third PGID
lookup (for source masks) denies it?

>
> >*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.
>

Could you please confirm this using stronger language than "I believe"?
Just for everybody to follow along here, in Ocelot/Felix, Port Group
IDs (PGIDs) are masks of destination ports. The switch performs 3
lookups in the PGID table for each frame, and forwards the frame to
the ports that are present in the logical AND of all 3 PGIDs (for the
most part, see below).

The first PGID lookup is for the destination masks and the PGID table
is indexed by the DEST_IDX field from the MAC table (FDB).
The PGID can be an unicast set: PGIDs 0-11 are the per-port PGIDs, and
by convention PGID i has only BIT(i) set, aka only this port is set in
the destination mask.
Or the PGID can be a multicast set: PGIDs 12-63 can (again, still by
convention) hold a richer destination mask comprised of multiple
ports.

I'll be ignoring the second PGID lookup, for aggregation, here, since
it doesn't interfere (it doesn't restrict the valid destinations mask
in any way).

The third PGID lookup is for source masks: PGID entries 80-91 answer
the question: is port i allowed to forward traffic to port j? If yes,
then BIT(j) of PGID 80+i will be found set. _These_ are what we're
talking about here (PGID_SRC + i).

What is interesting about the CPU port in this whole story is that, in
the way the driver sets up the PGIDs, its bit isn't set in any source
mask PGID of any other port (therefore, the third lookup would always
decide to exclude the CPU port from this list). So frames are never
_forwarded_ to the CPU.

There is a loophole in this PGID mechanism which is described in the
VSC7514 manual:

        If an entry is found in the MAC table entry of ENTRY_TYPE 0 or 1
        and the CPU port is set in the PGID pointed to by the MAC table
        entry, CPU extraction queue PGID.DST_PGID is added to the CPUQ.

In other words, the CPU port is special, and frames are "copied" to
the CPU, disregarding the source masks (third PGID lookup), if
BIT(cpu) is found to be set in the destination masks (first PGID
lookup).

So my question was: is the "copy to CPU" action special-cased in the
Ocelot hardware to bypass the L2 forwarding matrix? Because if it is,
that isn't how any of the other DSA switches works.

> 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.

With VLAN filtering enabled, keeping the MAC of the interface
installed in the MAC table in all VLANs that the port is a member of
will be a nightmare. Also think about reference counting (in DSA all
ports have the same MAC address by default. When should you remove the
MAC table entry corresponding to a port?) Also consider that I'd have
to change a lot of core DSA stuff for one switch.

>
> 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.

But I don't want to do this, do I? I want a forwarding path towards
the CPU, not to spam the CPU.

>
> /Allan
>

Regards,
-Vladimir

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

[Vladimir Oltean]

On Wed, 19 Feb 2020 at 12:11, Allan W. Nielsen
<allan.nielsen@microchip.com> wrote:
>
> 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.
>

I really think this is not the way to go.
As I've explained, you already have the code path in your driver that
I want Felix to operate in.
You just need to put your front-panel port in a bridge, and then
remove it from the bridge. That's it. Whatever your switch does in
that mode, I would like Felix to do it at probe time too. Chances are
that it doesn't bother you, otherwise that code would have been
removed already.

>
> /Allan
>

Thanks,
-Vladimir

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

[Andrew Lunn]

On Tue, Feb 18, 2020 at 02:29:15PM +0200, Vladimir Oltean wrote:
> Hi Allan,
> 
> On Tue, 18 Feb 2020 at 13:32, Allan W. Nielsen
> <allan.nielsen@microchip.com> wrote:
> >
> > On 17.02.2020 17:00, Vladimir Oltean wrote:
> > >EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > >
> > >From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > >
> > >The Ocelot switches have what is, in my opinion, a design flaw: their
> > >DSA header is in front of the Ethernet header, which means that they
> > >subvert the DSA master's RX filter, which for all practical purposes,
> > >either needs to be in promiscuous mode, or the OCELOT_TAG_PREFIX_LONG
> > >needs to be used for extraction, which makes the switch add a fake DMAC
> > >of ff:ff:ff:ff:ff:ff so that the DSA master accepts the frame.
> > >
> > >The issue with this design, of course, is that the CPU will be spammed
> > >with frames that it doesn't want to respond to, and there isn't any
> > >hardware offload in place by default to drop them.
> > In the case of Ocelot, the NPI port is expected to be connected back to
> > back to the CPU, meaning that it should not matter what DMAC is set.
> >
> 
> You are omitting the fact that the host Ethernet port has an RX filter
> as well. By default it should drop frames that aren't broadcast or
> aren't sent to a destination MAC equal to its configured MAC address.
> Most DSA switches add their tag _after_ the Ethernet header. This
> makes the DMAC and SMAC seen by the front-panel port of the switch be
> the same as the DMAC and SMAC seen by the host port. Combined with the
> fact that DSA sets up switch port MAC addresses to be inherited from
> the host port, RX filtering 'just works'.

It is a little bit more complex than that, but basically yes. If the
slave interface is in promisc mode, the master interface is also made
promisc. So as soon as you add a slave to a bridge, the master it set
promisc. Also, if the slave has a different MAC address to the master,
the MAC address is added to the masters RX filter.

If the DSA header is before the DMAC, you need promisc mode all the
time. But i don't expect the CPU port to be spammed. The switch should
only be forwarding frames to the CPU which the CPU is actually
interested in.

> 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

That is expected to work.

> 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.

It probably also works because when the port is added to the bridge,
the bridge puts the port into promisc mode. That in term causes the
master to be put into promisc mode.

       Andrew

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

[Vladimir Oltean]

Hi Andrew,

On Tue, 18 Feb 2020 at 16:01, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Feb 18, 2020 at 02:29:15PM +0200, Vladimir Oltean wrote:
> > Hi Allan,
> >
> > On Tue, 18 Feb 2020 at 13:32, Allan W. Nielsen
> > <allan.nielsen@microchip.com> wrote:
> > >
> > > On 17.02.2020 17:00, Vladimir Oltean wrote:
> > > >EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > > >
> > > >From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > >
> > > >The Ocelot switches have what is, in my opinion, a design flaw: their
> > > >DSA header is in front of the Ethernet header, which means that they
> > > >subvert the DSA master's RX filter, which for all practical purposes,
> > > >either needs to be in promiscuous mode, or the OCELOT_TAG_PREFIX_LONG
> > > >needs to be used for extraction, which makes the switch add a fake DMAC
> > > >of ff:ff:ff:ff:ff:ff so that the DSA master accepts the frame.
> > > >
> > > >The issue with this design, of course, is that the CPU will be spammed
> > > >with frames that it doesn't want to respond to, and there isn't any
> > > >hardware offload in place by default to drop them.
> > > In the case of Ocelot, the NPI port is expected to be connected back to
> > > back to the CPU, meaning that it should not matter what DMAC is set.
> > >
> >
> > You are omitting the fact that the host Ethernet port has an RX filter
> > as well. By default it should drop frames that aren't broadcast or
> > aren't sent to a destination MAC equal to its configured MAC address.
> > Most DSA switches add their tag _after_ the Ethernet header. This
> > makes the DMAC and SMAC seen by the front-panel port of the switch be
> > the same as the DMAC and SMAC seen by the host port. Combined with the
> > fact that DSA sets up switch port MAC addresses to be inherited from
> > the host port, RX filtering 'just works'.
>
> It is a little bit more complex than that, but basically yes. If the
> slave interface is in promisc mode, the master interface is also made
> promisc. So as soon as you add a slave to a bridge, the master it set
> promisc. Also, if the slave has a different MAC address to the master,
> the MAC address is added to the masters RX filter.
>
> If the DSA header is before the DMAC, you need promisc mode all the
> time. But i don't expect the CPU port to be spammed. The switch should
> only be forwarding frames to the CPU which the CPU is actually
> interested in.
>
> > 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
>
> That is expected to work.
>
> > 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.
>
> It probably also works because when the port is added to the bridge,
> the bridge puts the port into promisc mode. That in term causes the
> master to be put into promisc mode.

Promisc on the DSA master is not why this works. The switch drops the
packets instead of forwarding them to the CPU, because the CPU is not
in the list of valid destination for unknown unicast traffic received
on the front panel ports.
Promiscuous mode on the DSA master is also disabled when the switch
ports exit the bridge, by the way.

>
>        Andrew

Regards,
-Vladimir

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

[Allan W. Nielsen]

On 18.02.2020 15:01, Andrew Lunn wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>On Tue, Feb 18, 2020 at 02:29:15PM +0200, Vladimir Oltean wrote:
>> Hi Allan,
>>
>> On Tue, 18 Feb 2020 at 13:32, Allan W. Nielsen
>> <allan.nielsen@microchip.com> wrote:
>> >
>> > On 17.02.2020 17:00, Vladimir Oltean wrote:
>> > >EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>> > >
>> > >From: Vladimir Oltean <vladimir.oltean@nxp.com>
>> > >
>> > >The Ocelot switches have what is, in my opinion, a design flaw: their
>> > >DSA header is in front of the Ethernet header, which means that they
>> > >subvert the DSA master's RX filter, which for all practical purposes,
>> > >either needs to be in promiscuous mode, or the OCELOT_TAG_PREFIX_LONG
>> > >needs to be used for extraction, which makes the switch add a fake DMAC
>> > >of ff:ff:ff:ff:ff:ff so that the DSA master accepts the frame.
>> > >
>> > >The issue with this design, of course, is that the CPU will be spammed
>> > >with frames that it doesn't want to respond to, and there isn't any
>> > >hardware offload in place by default to drop them.
>> > In the case of Ocelot, the NPI port is expected to be connected back to
>> > back to the CPU, meaning that it should not matter what DMAC is set.
>> >
>>
>> You are omitting the fact that the host Ethernet port has an RX filter
>> as well. By default it should drop frames that aren't broadcast or
>> aren't sent to a destination MAC equal to its configured MAC address.
>> Most DSA switches add their tag _after_ the Ethernet header. This
>> makes the DMAC and SMAC seen by the front-panel port of the switch be
>> the same as the DMAC and SMAC seen by the host port. Combined with the
>> fact that DSA sets up switch port MAC addresses to be inherited from
>> the host port, RX filtering 'just works'.
>
>It is a little bit more complex than that, but basically yes. If the
>slave interface is in promisc mode, the master interface is also made
>promisc. So as soon as you add a slave to a bridge, the master it set
>promisc. Also, if the slave has a different MAC address to the master,
>the MAC address is added to the masters RX filter.
Good to know. I assume this will mean that in the case of Felix+NXP cpu
the master interface is in promisc mode?

>If the DSA header is before the DMAC, you need promisc mode all the
>time. But i don't expect the CPU port to be spammed. The switch should
>only be forwarding frames to the CPU which the CPU is actually
>interested in.
With Ocelot we do not see this spamming - and I still do not understand
why this is seen with Felix.

It is the same core with process the frames, and decide which frames
should be copied to the CPU. The only difference is that in Ocelot the
CPU queue is connected to a frame-DMA, while in Felix it is a MAC-to-MAC
connection.

>> 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
>
>That is expected to work.
>
>> 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.
>
>It probably also works because when the port is added to the bridge,
>the bridge puts the port into promisc mode. That in term causes the
>master to be put into promisc mode.
>
>       Andrew

/Allan

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

[Vladimir Oltean]

On Wed, 19 Feb 2020 at 12:17, Allan W. Nielsen
<allan.nielsen@microchip.com> wrote:
>
> With Ocelot we do not see this spamming - and I still do not understand
> why this is seen with Felix.
>

I should have watched my words.
When doing what all the other DSA switches do, which is enabling a
bulk forwarding path between the front-panel ports and the CPU, the
Ocelot switch core is more susceptible to doing more software
processing work than other devices in its class, for the same end
effect of dropping packets that the CPU is not interested in (say an
unknown unicast MAC that is not present in the switch FDB nor in the
DSA master's RX filter). In such a scenario, any other DSA system
would have the host port drop these packets in hardware, by virtue of
the unknown unicast MAC not being being present RX filter. With
Ocelot, this mechanism that prevents software work being done for
dropping is subverted. So to avoid this design limitation, the Ocelot
core does not enable a bulk forwarding path between the front-panel
ports and the CPU.

Hope this is clearer.

-Vladimir

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

[Allan W. Nielsen]

Hi,

On 19.02.2020 16:05, Vladimir Oltean wrote:
>On Wed, 19 Feb 2020 at 12:17, Allan W. Nielsen
><allan.nielsen@microchip.com> wrote:
>>
>> With Ocelot we do not see this spamming - and I still do not understand
>> why this is seen with Felix.
>>
>
>I should have watched my words.
>When doing what all the other DSA switches do, which is enabling a
>bulk forwarding path between the front-panel ports and the CPU, the
>Ocelot switch core is more susceptible to doing more software
>processing work than other devices in its class, for the same end
>effect of dropping packets that the CPU is not interested in (say an
>unknown unicast MAC that is not present in the switch FDB nor in the
>DSA master's RX filter). In such a scenario, any other DSA system
>would have the host port drop these packets in hardware, by virtue of
>the unknown unicast MAC not being being present RX filter. With
>Ocelot, this mechanism that prevents software work being done for
>dropping is subverted. So to avoid this design limitation, the Ocelot
>core does not enable a bulk forwarding path between the front-panel
>ports and the CPU.
>
>Hope this is clearer.

Horatiu and I have looked further into this, done a few experiments, and
discussed with the HW engineers who have a more detailed version of how
the chips are working and how Ocelot and Felix differs.

Here are our findings:

- The most significant bit in the PGID table is "special" as it is a
   CPU-copy bit.
- This bit is not being used in the source filtering! This means that
   your original patch can be applied without breaking Ocelot (the
   uninitialized cpu field must be fixed though).
   - Still I do not think we should do this as it is not the root-casuse
- In Felix we have 2 ways to get frames to the CPU, in Ocelot we have 1
   (Ocelot also has two if it uses an NPI port, but it does not do that
   in the current driver).
   - In Felix you can get frames to the CPU by either using the CPU port
     (port 6), or by using the NPI port (which can be any in the range of
     0-5).
     - But you should only use the CPU port, and not the NPI port
       directly. Using the NPI port directly will cause the two targets
       to behave differently, and this is not what we do when testing all
       the use-cases on the switch.
   - In Ocelot you can only get frames to the CPU by using the CPU port
     (port 11).

Due to this, I very much think you need to fix this, such that Felix
always port 6 to reach the CPU (with the exception of writing
QSYS_EXT_CPU_CFG where you "connect" the CPU queue/port to the NPI
port).

If you do this change, then the Ocelot and Felix should start to work in
the same way.

Then, if you want the CPU to be part of the unicast flooding (this is
where this discussion started), then you should add the CPU port to the
PGID entry pointed at in ANA:ANA:FLOODING:FLD_UNICAST. This should be
done for Felix and not for Ocelot.

If you want the analyser (where the MAC table sits), to "learn" the CPU
MAC (which is needed if you do not want to have the CPU mac as a static
entry in the MAC-table), then you need to set the 'src-port' to 6 (if it
is Ocelot then it will be 11) in the IFH:

anielsen@lx-anielsen ~ $ ef hex ifh-oc1 help
ifh-oc1          Injection Frame Header for Ocelot1

Specify the ifh-oc1 header by using one or more of the following fields:
- Name ------------ offset:width --- Description --------------------------
   bypass              +  0:  1  Skip analyzer processing
   b1-rew-mac          +  1:  1  Replace SMAC address
   b1-rew-op           +  2:  9  Rewriter operation command
   b0-masq             +  1:  1  Enable masquerading
   b0-masq-port        +  2:  4  Masquerading port
   rew-val             + 11: 32  Receive time stamp
   res1                + 43: 17  Reserved
   dest                + 60: 12  Destination set for the frame. Dest[11] is the CPU
   res2                + 72:  9  Reserved
   src-port            + 81:  4  The port number where the frame was injected (0-12)  <------------------- THIS FIELD
   res3                + 85:  2  Reserved
   trfm-timer          + 87:  4  Timer for periodic transmissions (1..8). If zero then normal injection
   res4                + 91:  6  Reserved
   dp                  + 97:  1  Drop precedence level after policing
   pop-cnt             + 98:  2  Number of VLAN tags that must be popped
   cpuq                +100:  8  CPU extraction queue mask
   qos-class           +108:  3  Classified QoS class
   tag-type            +111:  1  Tag information's associated Tag Protocol Identifier (TPID)
   pcp                 +112:  3  Classified PCP
   dei                 +115:  1  Classified DEI
   vid                 +116: 12  Classified VID


/Allan

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

[Vladimir Oltean]

On Thu, 20 Feb 2020 at 15:23, Allan W. Nielsen
<allan.nielsen@microchip.com> wrote:
>
> Horatiu and I have looked further into this, done a few experiments, and
> discussed with the HW engineers who have a more detailed version of how
> the chips are working and how Ocelot and Felix differs.
>
> Here are our findings:
>
> - The most significant bit in the PGID table is "special" as it is a
>    CPU-copy bit.

Wow.
Looking at the code after this realization, it is so confusing to call
the NPI port "ocelot->cpu" now, since it doesn't benefit from this
"privilege" that a "real" CPU port (from the Ocelot hardware
perspective) has.
Not to mention how strange it is for the hardware to behave this way.

> - This bit is not being used in the source filtering!

So BIT(6) on Felix and BIT(11) on Ocelot are just being interpreted
for the first and second PGID lookup (destination and aggregation
masks) but not for source masks? Don't you want to actually document
this somewhere?

So the frame is copied to the CPU based on the AND between first and
second lookup, and to all the other ports, including the NPI port,
based on the first, second and third PGID lookup.
So frames can reach the NPI port directly, via 3 PGID lookups, or
indirectly, via 2 PGID lookups. What I did is make the direct path
work. You're suggesting me to set the indirect mode up.

> This means that
>    your original patch can be applied without breaking Ocelot (the
>    uninitialized cpu field must be fixed though).

So my patch makes the Felix NPI port work in an unintended way and
does not affect Ocelot in any way. Roger.

>    - Still I do not think we should do this as it is not the root-casuse
> - In Felix we have 2 ways to get frames to the CPU, in Ocelot we have 1
>    (Ocelot also has two if it uses an NPI port, but it does not do that
>    in the current driver).
>    - In Felix you can get frames to the CPU by either using the CPU port
>      (port 6), or by using the NPI port (which can be any in the range of
>      0-5).
>      - But you should only use the CPU port, and not the NPI port
>        directly. Using the NPI port directly will cause the two targets
>        to behave differently, and this is not what we do when testing all
>        the use-cases on the switch.

Differently in what way?

>    - In Ocelot you can only get frames to the CPU by using the CPU port
>      (port 11).
>
> Due to this, I very much think you need to fix this, such that Felix
> always port 6 to reach the CPU (with the exception of writing
> QSYS_EXT_CPU_CFG where you "connect" the CPU queue/port to the NPI
> port).
>

What PGIDs should I use for the NPI port if I use it with indirection
via port 6?

> If you do this change, then the Ocelot and Felix should start to work in
> the same way.
>
> Then, if you want the CPU to be part of the unicast flooding (this is
> where this discussion started), then you should add the CPU port to the
> PGID entry pointed at in ANA:ANA:FLOODING:FLD_UNICAST. This should be
> done for Felix and not for Ocelot.

No, the question is why don't _you_ want the CPU to be in the
FLD_UNICAST PGID (which is PGID_UC). The distinction you're making
between Felix and Ocelot here is quite arbitrary, and seems to be
based just on "your CPU is more powerful".

So right now, multicast and broadcast traffic goes to PGID_MC (61),
and unknown unicast goes to PGID_UC (60).
The destination ports mask for PGID_MC is
GENMASK(ocelot->num_phys_ports, 0) - 0x7f for me, 0xfff for you.
The destination ports mask for PGID_UC is
GENMASK(ocelot->num_phys_ports - 1, 0) - this is the hardware default
value - 0x3f for me, 0x7ff for you.
So you are keeping the non-physical CPU port in the destination mask
for broadcast, but not in that for unknown unicast. In the way the
system is configured, it is still susceptible to broadcast storms. So
I don't think there is any real benefit in crippling the system like
this. If port 6 was in PGID_UC by default, I would have never bat an
eye and more than likely never needed to know the gory details.

Is it possible to set up policers for traffic going to CPU? I _did_
see this phrase already in the manual:

"Frames where the DMAC lookup returned a PGID with the CPU port set
are always forwarded to the CPU even when the frame
is policed by the storm policers."

So the traffic I'm seeing now on the NPI port is not copied to the
CPU, it is forwarded.
If there's no other way to set up storm policers for the mode you're
suggesting me to change Felix to, then I would respectfully keep it
the way it is right now.

>
> If you want the analyser (where the MAC table sits), to "learn" the CPU
> MAC (which is needed if you do not want to have the CPU mac as a static
> entry in the MAC-table), then you need to set the 'src-port' to 6 (if it
> is Ocelot then it will be 11) in the IFH:
>

Please tell me more about this. Don't I need to set BYPASS=1 for
frames to go on the front-panel port specified in DEST? And doesn't
BYPASS=1 mean no source MAC learning for injected traffic?
As much as I would like the analyzer to run, I won't do it if it is
going to compromise xmit from Linux. I don't want traffic sent from
Linux to an unknown unicast in standalone mode to be flooded to all
front-panel ports with no way for me to control it.

> anielsen@lx-anielsen ~ $ ef hex ifh-oc1 help
> ifh-oc1          Injection Frame Header for Ocelot1
>
> Specify the ifh-oc1 header by using one or more of the following fields:
> - Name ------------ offset:width --- Description --------------------------
>    bypass              +  0:  1  Skip analyzer processing
>    b1-rew-mac          +  1:  1  Replace SMAC address
>    b1-rew-op           +  2:  9  Rewriter operation command
>    b0-masq             +  1:  1  Enable masquerading
>    b0-masq-port        +  2:  4  Masquerading port
>    rew-val             + 11: 32  Receive time stamp
>    res1                + 43: 17  Reserved
>    dest                + 60: 12  Destination set for the frame. Dest[11] is the CPU
>    res2                + 72:  9  Reserved
>    src-port            + 81:  4  The port number where the frame was injected (0-12)  <------------------- THIS FIELD
>    res3                + 85:  2  Reserved
>    trfm-timer          + 87:  4  Timer for periodic transmissions (1..8). If zero then normal injection
>    res4                + 91:  6  Reserved
>    dp                  + 97:  1  Drop precedence level after policing
>    pop-cnt             + 98:  2  Number of VLAN tags that must be popped
>    cpuq                +100:  8  CPU extraction queue mask
>    qos-class           +108:  3  Classified QoS class
>    tag-type            +111:  1  Tag information's associated Tag Protocol Identifier (TPID)
>    pcp                 +112:  3  Classified PCP
>    dei                 +115:  1  Classified DEI
>    vid                 +116: 12  Classified VID
>
>
> /Allan
>

Thanks,
-Vladimir