Re: [PATCH net-next 0/3] VLANs, DSA switches and multiple bridges

[Florian Fainelli <f.fainelli@gmail.com>]

On 2/19/20 10:07 AM, Vivien Didelot wrote:
> Hi Russell,
> 
> On Wed, 19 Feb 2020 09:19:00 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
>> On Wed, Feb 19, 2020 at 04:47:30AM +0100, Andrew Lunn wrote:
>>> On Wed, Feb 19, 2020 at 12:17:37AM +0000, Russell King - ARM Linux admin wrote:
>>>> On Tue, Feb 18, 2020 at 04:00:08PM -0800, Florian Fainelli wrote:
>>>>> On 2/18/20 3:45 AM, Russell King - ARM Linux admin wrote:
>>>>>> Hi,
>>>>>>
>>>>>> This is a repost of the previously posted RFC back in December, which
>>>>>> did not get fully reviewed.  I've dropped the RFC tag this time as no
>>>>>> one really found anything too problematical in the RFC posting.
>>>>>>
>>>>>> I've been trying to configure DSA for VLANs and not having much success.
>>>>>> The setup is quite simple:
>>>>>>
>>>>>> - The main network is untagged
>>>>>> - The wifi network is a vlan tagged with id $VN running over the main
>>>>>>   network.
>>>>>>
>>>>>> I have an Armada 388 Clearfog with a PCIe wifi card which I'm trying to
>>>>>> setup to provide wifi access to the vlan $VN network, while the switch
>>>>>> is also part of the main network.
>>>>>
>>>>> Why not just revert 2ea7a679ca2abd251c1ec03f20508619707e1749 ("net: dsa:
>>>>> Don't add vlans when vlan filtering is disabled")? If a driver wants to
>>>>> veto the programming of VLANs while it has ports enslaved to a bridge
>>>>> that does not have VLAN filtering, it should have enough information to
>>>>> not do that operation.
>>>>
>>>> I do not have the knowledge to know whether reverting that commit
>>>> would be appropriate; I do not know how the non-Marvell switches will
>>>> behave with such a revert - what was the reason for the commit in
>>>> the first place?
>>>>
>>>> The commit says:
>>>>
>>>>     This fixes at least one corner case. There are still issues in other
>>>>     corners, such as when vlan_filtering is later enabled.
>>>>
>>>> but it doesn't say what that corner case was.  So, presumably reverting
>>>> it will cause a regression of whatever that corner case was...
>>>
>>> Yes, sorry, bad commit message. I'm not too sure, but it could of been
>>> that the switch was adding the VLANs to its tables, even though it
>>> should not because filtering is disabled. And i also think the default
>>> VLAN was not defined at that point, it only gets defined when
>>> vlan_filtering is enabled?
>>
>> It's been too long since I researched all these details, but I seem
>> to remember that in the Linux software bridge, vlan 1 is always
>> present even when vlan filtering is not enabled.
>>
>> Looking at br_vlan_init():
>>
>>         br->default_pvid = 1;
>>
>> and nbp_vlan_init() propagates that irrespective of the bridge vlan
>> enable state to switchdev.  nbp_vlan_init() is called whenever any
>> interface is added to a bridge (in br_add_if()).
>>
>> As I believe I mentioned somewhere in the commit messages or covering
>> message, for at least some of the Marvell DSA switches, it is safe to
>> add VTU entries - they do not even look at the VTU when the port has
>> 802.1Q disabled.  Whether that is true for all Marvell's DSA switches
>> I don't know without trawling every functional spec, and I was hoping
>> that you guys would know.  I guess I need to trawl the specs.
> 
> Some switches like the Marvell 88E6060 don't have a VTU, so programming the
> default PVID would return -EOPNOTSUPP. Switches supporting only global VLAN
> filtering cannot have a VLAN filtering aware bridge as well as a non VLAN
> filtering aware bridge spanning their ports at the same time. But all this
> shouldn't be a problem because drivers inform the stack whether they support
> ds->vlan_filtering per-port, globally or not. We should simply reject the
> operation when vlan_filtering is being enabled on unsupported hardware.
> 
> Linux bridge is the reference for the implementation of an Ethernet bridge,
> if it programs VLAN entries, supported DSA hardware must do so. I'm not a
> fan of having our own bridge logic in DSA, so the limitation implemented by
> 2ea7a679ca2a ("net: dsa: Don't add vlans when vlan filtering is disabled")
> needs to go in my opinion.

Agreed.

This also helps with switches who only support the creation of broadcast
domains via VLANs (not the case with b53 and mv88e6xxx AFAICT they have
specific egress vector controls). Because then you could put each
standalone port in say, VID (4094 - port number), and once enslaved in a
bridge, have them in VID 1 to maintain broadcast domains, whether the
bridge has VLAN filtering or not.
-- 
Florian