On 6/28/19 5:37 AM, Vladimir Oltean wrote: > On Fri, 28 Jun 2019 at 15:30, Ido Schimmel <ido...@idosch.org> wrote: >> >> On Tue, Jun 25, 2019 at 11:49:29PM +0300, Vladimir Oltean wrote: >>> A number of DSA drivers (BCM53XX, Microchip KSZ94XX, Mediatek MT7530 >>> at the very least), as well as Mellanox Spectrum (I didn't look at all >>> the pure switchdev drivers) try to restore the pvid to a default value >>> on .port_vlan_del. >> >> I don't know about DSA drivers, but that's not what mlxsw is doing. If >> the VLAN that is configured as PVID is deleted from the bridge port, the >> driver instructs the port to discard untagged and prio-tagged packets. >> This is consistent with the bridge driver's behavior. >> >> We do have a flow the "restores" the PVID, but that's when a port is >> unlinked from its bridge master. The PVID we set is 4095 which cannot be >> configured by the 8021q / bridge driver. This is due to the way the >> underlying hardware works. Even if a port is not bridged and used purely >> for routing, packets still do L2 lookup first which sends them directly >> to the router block. If PVID is not configured, untagged packets could >> not be routed. Obviously, at egress we strip this VLAN. >> >>> Sure, the port stops receiving traffic when its pvid is a VLAN ID that >>> is not installed in its hw filter, but as far as the bridge core is >>> concerned, this is to be expected: >>> >>> # bridge vlan add dev swp2 vid 100 pvid untagged >>> # bridge vlan >>> port vlan ids >>> swp5 1 PVID Egress Untagged >>> >>> swp2 1 Egress Untagged >>> 100 PVID Egress Untagged >>> >>> swp3 1 PVID Egress Untagged >>> >>> swp4 1 PVID Egress Untagged >>> >>> br0 1 PVID Egress Untagged >>> # ping 10.0.0.1 >>> PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data. >>> 64 bytes from 10.0.0.1: icmp_seq=1 ttl=64 time=0.682 ms >>> 64 bytes from 10.0.0.1: icmp_seq=2 ttl=64 time=0.299 ms >>> 64 bytes from 10.0.0.1: icmp_seq=3 ttl=64 time=0.251 ms >>> 64 bytes from 10.0.0.1: icmp_seq=4 ttl=64 time=0.324 ms >>> 64 bytes from 10.0.0.1: icmp_seq=5 ttl=64 time=0.257 ms >>> ^C >>> --- 10.0.0.1 ping statistics --- >>> 5 packets transmitted, 5 received, 0% packet loss, time 4188ms >>> rtt min/avg/max/mdev = 0.251/0.362/0.682/0.163 ms >>> # bridge vlan del dev swp2 vid 100 >>> # bridge vlan >>> port vlan ids >>> swp5 1 PVID Egress Untagged >>> >>> swp2 1 Egress Untagged >>> >>> swp3 1 PVID Egress Untagged >>> >>> swp4 1 PVID Egress Untagged >>> >>> br0 1 PVID Egress Untagged >>> >>> # ping 10.0.0.1 >>> PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data. >>> ^C >>> --- 10.0.0.1 ping statistics --- >>> 8 packets transmitted, 0 received, 100% packet loss, time 7267ms >>> >>> What is the consensus here? Is there a reason why the bridge driver >>> doesn't take care of this? >> >> Take care of what? :) Always maintaining a PVID on the bridge port? It's >> completely OK not to have a PVID. >> > > Yes, I didn't think it through during the first email. I came to the > same conclusion in the second one. > >>> Do switchdev drivers have to restore the pvid to always be >>> operational, even if their state becomes inconsistent with the upper >>> dev? Is it just 'nice to have'? What if VID 1 isn't in the hw filter >>> either (perfectly legal)? >> >> Are you saying that DSA drivers always maintain a PVID on the bridge >> port and allow untagged traffic to ingress regardless of the bridge >> driver's configuration? If so, I think this needs to be fixed. > > Well, not at the DSA core level. > But for Microchip: > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/drivers/net/dsa/microchip/ksz9477.c#n576 > For Broadcom: > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/drivers/net/dsa/b53/b53_common.c#n1376 > For Mediatek: > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/drivers/net/dsa/mt7530.c#n1196 > > There might be others as well.
That sounds bogus indeed, and I bet that the two other drivers just followed the b53 driver there. I will have to test this again and come up with a patch eventually. When the port leaves the bridge we do bring it back into a default PVID (which is different than the bridge's default PVID) so that part should be okay. -- Florian
