On Mon, Mar 08, 2021 at 19:00, Vladimir Oltean <olte...@gmail.com> wrote:
> 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 <tob...@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
Alright, we do not want to lie to the stack, got it...
>> 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.
...hang on, are we OK with lying or not? Yes, I guess?
> It's a lot easier that way, otherwise you will end up having to replay
> them somehow.
I think vlan_for_each should be enough to perform the replay when
toggling VLAN filtering on a port?
More importantly, there are other sequences that we do not guard against
today:
- Adding VID to a bridge port that is used on an 1Q upper of another
bridged port.
.100 br0
\ / \
lan0 lan1
$ ip link add dev br0 type bridge vlan_filtering 1
$ ip link add dev lan0.100 link lan0 type vlan id 100
$ ip link set dev lan0 master br0
$ ip link set dev lan1 master br0
$ bridge vlan add dev lan1 vid 100 # This should fail
After this sequence, the switch will forward VID 100 tagged frames
between lan0 and lan1.
- Briding two ports that both have 1Q uppers using the same VID.
.100 br0 .100
\ / \ /
lan0 lan1
$ ip link add dev br0 type bridge vlan_filtering 1
$ ip link add dev lan0.100 link lan0 type vlan id 100
$ ip link add dev lan1.100 link lan1 type vlan id 100
$ ip link set dev lan0 master br0
$ ip link set dev lan1 master br0 # This should fail
This is also allowed by DSA today, and produces the same switch
config as the previous sequence.
So in summary:
- Try to design some generic VLAN validation that can be used when:
- Adding VLANs to standalone ports.
- Adding VLANs to bridged ports.
- Toggling VLAN filtering on ports.
- Remove 1/2.
- Rework 2/2 to:
- `return 0` when adding a VLAN to a non-bridged port, not -EOPNOTSUPP.
- Lazy load/unload VIDs from VLAN uppers when toggling filtering on a
port using vlan_for_each or similar.
Does that sound reasonable?
Are we still in net territory or is this more suited for net-next?
