mbox series

[net-next,0/7] VLAN improvements for Ocelot switch

Message ID 20201031102916.667619-1-vladimir.oltean@nxp.com
Headers show
Series VLAN improvements for Ocelot switch | expand

Message

Vladimir Oltean Oct. 31, 2020, 10:29 a.m. UTC
The main reason why I started this work is that deleting the bridge mdb
entries fails when the bridge is deleted, as described here:
https://lore.kernel.org/netdev/20201015173355.564934-1-vladimir.oltean@nxp.com/

In short, that happens because the bridge mdb entries are added with a
vid of 1, but deletion is attempted with a vid of 0. So the deletion
code fails to find the mdb entries.

The solution is to make ocelot use a pvid of 0 when it is under a bridge
with vlan_filtering 0. When vlan_filtering is 1, the pvid of the bridge
is what is programmed into the hardware.

The patch series also uncovers more bugs and does some more cleanup, but
the above is the main idea behind it.

Vladimir Oltean (7):
  net: mscc: ocelot: use the pvid of zero when bridged with
    vlan_filtering=0
  net: mscc: ocelot: don't reset the pvid to 0 when deleting it
  net: mscc: ocelot: transform the pvid and native vlan values into a
    structure
  net: mscc: ocelot: add a "valid" boolean to struct ocelot_vlan
  net: mscc: ocelot: move the logic to drop 802.1p traffic to the pvid
    deletion
  net: mscc: ocelot: deny changing the native VLAN from the prepare
    phase
  net: dsa: felix: improve the workaround for multiple native VLANs on
    NPI port

 drivers/net/dsa/ocelot/felix.c         |  27 ++++-
 drivers/net/ethernet/mscc/ocelot.c     | 147 +++++++++++++------------
 drivers/net/ethernet/mscc/ocelot_net.c |  38 +++++--
 include/soc/mscc/ocelot.h              |  17 ++-
 4 files changed, 138 insertions(+), 91 deletions(-)

Comments

Alexandre Belloni Nov. 2, 2020, 8:47 a.m. UTC | #1
Hello,

On 31/10/2020 12:29:10+0200, Vladimir Oltean wrote:
> Currently, mscc_ocelot ports configure pvid=0 in standalone mode, and

> inherit the pvid from the bridge when one is present.

> 

> When the bridge has vlan_filtering=0, the software semantics are that

> packets should be received regardless of whether there's a pvid

> configured on the ingress port or not. However, ocelot does not observe

> those semantics today.

> 

> Moreover, changing the PVID is also a problem with vlan_filtering=0.

> We are privately remapping the VID of FDB, MDB entries to the port's

> PVID when those are VLAN-unaware (i.e. when the VID of these entries

> comes to us as 0). But we have no logic of adjusting that remapping when

> the user changes the pvid and vlan_filtering is 0. So stale entries

> would be left behind, and untagged traffic will stop matching on them.

> 

> And even if we were to solve that, there's an even bigger problem. If

> swp0 has pvid 1, and swp1 has pvid 2, and both are under a vlan_filtering=0

> bridge, they should be able to forward traffic between one another.

> However, with ocelot they wouldn't do that.

> 

> The simplest way of fixing this is to never configure the pvid based on

> what the bridge is asking for, when vlan_filtering is 0. Only if there

> was a VLAN that the bridge couldn't mangle, that we could use as pvid....

> So, turns out, there's 0 just for that. And for a reason: IEEE

> 802.1Q-2018, page 247, Table 9-2-Reserved VID values says:

> 

> 	The null VID. Indicates that the tag header contains only

> 	priority information; no VID is present in the frame.

> 	This VID value shall not be configured as a PVID or a member

> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> 	of a VID Set, or configured in any FDB entry, or used in any

> 	Management operation.

> 

> So, aren't we doing exactly what 802.1Q says not to? Well, in a way, but

> what we're doing here is just driver-level bookkeeping, all for the

> better. The fact that we're using a pvid of 0 is not observable behavior

> from the outside world: the network stack does not see the classified

> VLAN that the switch uses, in vlan_filtering=0 mode. And we're also more

> consistent with the standalone mode now.

> 


IIRC, we are using pvid 1 because else bridging breaks when
CONFIG_VLAN_8021Q is not enabled. Did you test that configuration?



-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Vladimir Oltean Nov. 2, 2020, 3:35 p.m. UTC | #2
On Mon, Nov 02, 2020 at 09:47:20AM +0100, Alexandre Belloni wrote:
> IIRC, we are using pvid 1 because else bridging breaks when

> CONFIG_VLAN_8021Q is not enabled. Did you test that configuration?


Pertinent question.
I hadn't tested that, but I did now.

[root@LS1028ARDB ~] # zcat /proc/config.gz | grep 8021Q
# CONFIG_VLAN_8021Q is not set
[root@LS1028ARDB ~] # ip addr flush swp0
[root@LS1028ARDB ~] # ip addr add 192.168.1.2/24 dev swp0
[root@LS1028ARDB ~] # ping 192.168.1.1
PING 192.168.1.1 (192.168.1.1): 56 data bytes
64 bytes from 192.168.1.1: seq=0 ttl=64 time=0.717 ms
64 bytes from 192.168.1.1: seq=1 ttl=64 time=0.442 ms
^C
--- 192.168.1.1 ping statistics ---
2 packets transmitted, 2 packets received, 0% packet loss
round-trip min/avg/max = 0.442/0.579/0.717 ms
[root@LS1028ARDB ~] # ip addr flush swp0
[root@LS1028ARDB ~] # ip link add br0 type bridge
[root@LS1028ARDB ~] # ip link set swp0 master br0
[  409.576303] br0: port 1(swp0) entered blocking state
[  409.581380] br0: port 1(swp0) entered disabled state
[  409.586738] device swp0 entered promiscuous mode
[  409.591866] br0: port 1(swp0) entered blocking state
[  409.596852] br0: port 1(swp0) entered forwarding state
[root@LS1028ARDB ~] # ip addr add 192.168.1.2/24 dev br0
[root@LS1028ARDB ~] # ping 192.168.1.1
PING 192.168.1.1 (192.168.1.1): 56 data bytes
64 bytes from 192.168.1.1: seq=0 ttl=64 time=0.768 ms
64 bytes from 192.168.1.1: seq=1 ttl=64 time=0.657 ms
64 bytes from 192.168.1.1: seq=2 ttl=64 time=0.509 ms
64 bytes from 192.168.1.1: seq=3 ttl=64 time=0.513 ms
64 bytes from 192.168.1.1: seq=4 ttl=64 time=0.496 ms
^C
--- 192.168.1.1 ping statistics ---
5 packets transmitted, 5 packets received, 0% packet loss
round-trip min/avg/max = 0.496/0.588/0.768 ms
[root@LS1028ARDB ~] # ip link del br0
[  135.526825] device swp0 left promiscuous mode
[  135.531729] br0: port 1(swp0) entered disabled state
[root@LS1028ARDB ~] # ip addr add 192.168.1.2/24 dev swp0
[root@LS1028ARDB ~] # ping 192.168.1.1
PING 192.168.1.1 (192.168.1.1): 56 data bytes
64 bytes from 192.168.1.1: seq=0 ttl=64 time=0.783 ms
64 bytes from 192.168.1.1: seq=1 ttl=64 time=0.289 ms
64 bytes from 192.168.1.1: seq=2 ttl=64 time=0.412 ms
64 bytes from 192.168.1.1: seq=3 ttl=64 time=0.399 ms
64 bytes from 192.168.1.1: seq=4 ttl=64 time=0.396 ms
64 bytes from 192.168.1.1: seq=5 ttl=64 time=0.390 ms
^C
--- 192.168.1.1 ping statistics ---
6 packets transmitted, 6 packets received, 0% packet loss
round-trip min/avg/max = 0.289/0.444/0.783 ms

There's no logical reason why it wouldn't work. Thanks to your code
which ensures VLAN 0 is in the VLAN table. Nobody is removing VLAN 0
right now.

	/* Because VLAN filtering is enabled, we need VID 0 to get untagged
	 * traffic.  It is added automatically if 8021q module is loaded, but
	 * we can't rely on it since module may be not loaded.
	 */
	ocelot->vlan_mask[0] = GENMASK(ocelot->num_phys_ports - 1, 0);
	ocelot_vlant_set_mask(ocelot, 0, ocelot->vlan_mask[0]);

I cannot test the mscc_ocelot driver, I am only testing felix DSA, and
for that reason I can't even go very far down the history. Remember that
when CONFIG_VLAN_8021Q is disabled, CONFIG_BRIDGE_VLAN_FILTERING also
needs to be disabled. So logically speaking, nobody calls any VLAN
function when CONFIG_VLAN_8021Q is disabled. The standalone
configuration should work in this mode too, shouldn't it? I am not sure
if there's any relevant difference for mscc_ocelot.
Jakub Kicinski Nov. 3, 2020, 1:10 a.m. UTC | #3
On Sat, 31 Oct 2020 12:29:09 +0200 Vladimir Oltean wrote:
> The main reason why I started this work is that deleting the bridge mdb

> entries fails when the bridge is deleted, as described here:

> https://lore.kernel.org/netdev/20201015173355.564934-1-vladimir.oltean@nxp.com/

> 

> In short, that happens because the bridge mdb entries are added with a

> vid of 1, but deletion is attempted with a vid of 0. So the deletion

> code fails to find the mdb entries.

> 

> The solution is to make ocelot use a pvid of 0 when it is under a bridge

> with vlan_filtering 0. When vlan_filtering is 1, the pvid of the bridge

> is what is programmed into the hardware.

> 

> The patch series also uncovers more bugs and does some more cleanup, but

> the above is the main idea behind it.


Applied, thanks!