On 3/8/21 9:00 AM, Vladimir Oltean 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
>> 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.
Agreed.
--
Florian
