Re: [PATCH net 1/2] net: dsa: Accept software VLANs for stacked interfaces

On Mon, Mar 08, 2021 at 05:44:46PM +0200, Vladimir Oltean wrote:

On Mon, Mar 08, 2021 at 04:04:04PM +0100, Tobias Waldekranz wrote:

quoted

The dsa_slave_vlan_rx_{add,kill}_vid ndos are required for hardware
that can not control VLAN filtering per port, rather it is a device
global setting, in order to support VLAN uppers on non-bridged ports.

For hardware that can control VLAN filtering per port, it is perfectly
fine to fallback to software VLANs in this scenario. So, make sure
that this "error" does not leave the DSA layer as vlan_add_vid does
not know the meaning of it.

The blamed commit removed this exemption by not advertising the
feature if the driver did not implement VLAN offloading. But as we
know see, the assumption that if a driver supports VLAN offloading, it
will always use it, does not hold in certain edge cases.

Fixes: 9b236d2a69da ("net: dsa: Advertise the VLAN offload netdev ability only if switch supports it")
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---

So these NDOs exist for drivers that need the 'rx-vlan-filter: on'
feature in ethtool -k, which can be due to any of the following reasons:
1. vlan_filtering_is_global = true, some ports are under a VLAN-aware
   bridge while others are standalone (this is what you described)
2. Hellcreek. This driver needs it because in standalone mode, it uses
   unique VLANs per port to ensure separation. For separation of untagged
   traffic, it uses different PVIDs for each port, and for separation of
   VLAN-tagged traffic, it never accepts 8021q uppers with the same vid
   on two ports.
3. the ports that are under a VLAN-aware bridge should also set this
   feature, for 8021q uppers having a VID not claimed by the bridge.
   In this case, the driver will essentially not even know that the VID
   is coming from the 8021q layer and not the bridge.

If a driver does not fall under any of the above 3 categories, there is
no reason why it should advertise the 'rx-vlan-filter' feature, therefore
no reason why it should implement these NDOs, and return -EOPNOTSUPP.

We are essentially saying the same thing, except what I propose is to
better manage the 'rx-vlan-filter' feature of the DSA net devices. After
your patches, the network stack still thinks that mv88e6xxx ports in
standalone mode have VLAN filtering enabled, which they don't. That
might be confusing. Not only that, but any other driver that is
VLAN-unaware in standalone mode will similarly have to ignore VLANs
coming from the 8021q layer, which may add uselessly add to their
complexity. Let me prepare an alternative patch series and let's see how
they compare against each other.

As far as I see, mv88e6xxx needs to treat the VLAN NDOs in case 3 only,
and DSA will do that without any sort of driver-level awareness. It's
all the other cases (standalone ports mode) that are bothering you.

So I stopped from sending an alternative solution, because neither mine
nor yours will fix this situation:

ip link add link lan0 name lan0.100 type vlan id 100
ip addr add 192.168.100.1/24 dev lan0.100
ping 192.168.100.2 # should work
ip link add br0 type bridge vlan_filtering 0
ip link set lan0 master br0
ping 192.168.100.2 # should still work
ip link set br0 type bridge vlan_filtering 1
ping 192.168.100.2 # should still work

Basically my point is that you disregard the vlan_vid_add from the
lan0.100 upper now because you think you don't need it, but one day will
come when you will. We've had that problem for a very long while now
with bridge VLANs, and it wasn't even completely solved yet (that's why
ds->configure_vlan_while_not_filtering is still a thing). It's
fundamentally the same with VLANs added by the 8021q layer. I think you
should see what you can do to make mv88e6xxx stop complaining and accept
the VLANs from the 8021q uppers even if they aren't needed right away.
It's a lot easier that way, otherwise you will end up having to replay
them somehow.