Re: [PATCH net-next 4/4] net: dsa: set configure_vlan_while_not_filtering to true by default

From: Florian Fainelli <f.fainelli@gmail.com>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: davem@davemloft.net, vivien.didelot@gmail.com, andrew@lunn.ch,
	netdev@vger.kernel.org
Subject: Re: [PATCH net-next 4/4] net: dsa: set configure_vlan_while_not_filtering to true by default
Date: Thu, 10 Sep 2020 14:58:04 -0700	[thread overview]
Message-ID: <7e45b733-de6a-67c8-2e28-30a5ba84f544@gmail.com> (raw)
In-Reply-To: <5edf3aa2-c417-e708-b259-7235de7bc8d2@gmail.com>



On 9/9/2020 11:34 AM, Florian Fainelli wrote:
> 
> 
> On 9/9/2020 10:53 AM, Vladimir Oltean wrote:
>> On Wed, Sep 09, 2020 at 10:22:42AM -0700, Florian Fainelli wrote:
>>> How do you make sure that the CPU port sees the frame untagged which 
>>> would
>>> be necessary for a VLAN-unaware bridge? Do you have a special remapping
>>> rule?
>>
>> No, I don't have any remapping rules that would be relevant here.
>> Why would the frames need to be necessarily untagged for a VLAN-unaware
>> bridge, why is it a problem if they aren't?
>>
>> bool br_allowed_ingress(const struct net_bridge *br,
>>             struct net_bridge_vlan_group *vg, struct sk_buff *skb,
>>             u16 *vid, u8 *state)
>> {
>>     /* If VLAN filtering is disabled on the bridge, all packets are
>>      * permitted.
>>      */
>>     if (!br_opt_get(br, BROPT_VLAN_ENABLED)) {
>>         BR_INPUT_SKB_CB(skb)->vlan_filtered = false;
>>         return true;
>>     }
>>
>>     return __allowed_ingress(br, vg, skb, vid, state);
>> }
>>
>> If I have a VLAN on a bridged switch port where the bridge is not
>> filtering, I have an 8021q upper of the bridge with that VLAN ID.
> 
> Yes that is the key right there, you need an 8021q upper to pop the VLAN 
> ID or push it, that is another thing that users need to be aware of 
> which is a bit awkward, most expect things to just work. Maybe we should 
> just refuse to have bridge devices that are not VLAN-aware, because this 
> is just too cumbersome to deal with.

With the drivers that you currently maintain and with the CPU port being 
always tagged in the VLANs added to the user-facing ports, when you are 
using a non-VLAN aware bridge, do you systematically add an br0.1 upper 
802.1Q device to pop/push the VLAN tag?

I am about ready to submit the changes we discussed to b53, but I am 
still a bit uncomfortable with this part of the change because it will 
make the CPU port follow the untagged attribute of an user-facing port.

@@ -1444,7 +1427,7 @@ void b53_vlan_add(struct dsa_switch *ds, int port,
                         untagged = true;

                 vl->members |= BIT(port);
-               if (untagged && !dsa_is_cpu_port(ds, port))
+               if (untagged)
                         vl->untag |= BIT(port);
                 else
                         vl->untag &= ~BIT(port);
@@ -1482,7 +1465,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port,
                 if (pvid == vid)
                         pvid = b53_default_pvid(dev);

-               if (untagged && !dsa_is_cpu_port(ds, port))
+               if (untagged)
                         vl->untag &= ~(BIT(port));

-- 
Florian
