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

[Vladimir Oltean <olteanv@gmail.com>]

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