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

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

On 9/9/2020 9:31 AM, Vladimir Oltean wrote:
> On Tue, Sep 08, 2020 at 05:02:06PM -0700, Florian Fainelli wrote:
>> Found the problem, we do not allow the CPU port to be configured as
>> untagged, and when we toggle vlan_filtering we actually incorrectly "move"
>> the PVID from 1 to 0,
> 
> pvid 1 must be coming from the default_pvid of the bridge, I assume.
> Where is pvid 0 (aka dev->ports[port].pvid) coming from? Is it simply
> the cached value from B53_VLAN_PORT_DEF_TAG, from a previous
> b53_vlan_filtering() call? Strange.

The logic that writes to B53_VLAN_PORT_DEF_TAG does not update the 
shadow copy in dev->ports[port].pvid which is how they are out of sync.

> 
>> which is incorrect, but since the CPU is also untagged in VID 0 this
>> is why it "works" or rather two mistakes canceling it each other.
> 
> How does the CPU end up untagged in VLAN 0?

The CPU port gets also programmed with 0 in B53_VLAN_PORT_DEF_TAG.

> 
>> I still need to confirm this, but the bridge in VLAN filtering mode seems to
>> support receiving frames with the default_pvid as tagged, and it will untag
>> it for the bridge master device transparently.
> 
> So it seems.
> 
>> The reason for not allowing the CPU port to be untagged
>> (ca8931948344c485569b04821d1f6bcebccd376b) was because the CPU port could be
>> added as untagged in several VLANs, e.g.: when port0-3 are PVID 1 untagged,
>> and port 4 is PVID 2 untagged. Back then there was no support for Broadcom
>> tags, so the only way to differentiate traffic properly was to also add a
>> pair of tagged VIDs to the DSA master.
>> I am still trying to remember whether there were other concerns that
>> prompted me to make that change and would appreciate some thoughts on that.
> 
> I think it makes some sense to always configure the VLANs on the CPU
> port as tagged either way. I did the same in Felix and it's ok. But that
> was due to a hardware limitation. On sja1105 I'm keeping the same flags
> as on the user port, and that is ok too.

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?

Initially the concern I had was with the use case described above which 
was a 802.1Q separation, but in hindsight MAC address learning would 
result in the frames going to the appropriate ports/VLANs anyway.

> 
>> Tangentially, maybe we should finally add support for programming the CPU
>> port's VLAN membership independently from the other ports.
> 
> How?

Something like this:

https://lore.kernel.org/lkml/20180625091713.GA13442@apalos/T/

> 
>> The following appears to work nicely now and allows us to get rid of the
>> b53_vlan_filtering() logic, which would no longer work now because it
>> assumed that toggling vlan_filtering implied that there would be no VLAN
>> configuration when filtering was off.
>>
>> diff --git a/drivers/net/dsa/b53/b53_common.c
>> b/drivers/net/dsa/b53/b53_common.c
>> index 26fcff85d881..fac033730f4a 100644
>> --- a/drivers/net/dsa/b53/b53_common.c
>> +++ b/drivers/net/dsa/b53/b53_common.c
>> @@ -1322,23 +1322,6 @@ EXPORT_SYMBOL(b53_phylink_mac_link_up);
>>   int b53_vlan_filtering(struct dsa_switch *ds, int port, bool
>> vlan_filtering)
>>   {
>>          struct b53_device *dev = ds->priv;
>> -       u16 pvid, new_pvid;
>> -
>> -       b53_read16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port), &pvid);
>> -       if (!vlan_filtering) {
>> -               /* Filtering is currently enabled, use the default PVID
>> since
>> -                * the bridge does not expect tagging anymore
>> -                */
>> -               dev->ports[port].pvid = pvid;
>> -               new_pvid = b53_default_pvid(dev);
>> -       } else {
>> -               /* Filtering is currently disabled, restore the previous
>> PVID */
>> -               new_pvid = dev->ports[port].pvid;
>> -       }
>> -
>> -       if (pvid != new_pvid)
>> -               b53_write16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port),
>> -                           new_pvid);
> 
> Yes, much simpler.
> 
>>
>>          b53_enable_vlan(dev, dev->vlan_enabled, vlan_filtering);
>>
>> @@ -1389,7 +1372,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);
>> @@ -1427,7 +1410,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)
> 
> Ok, so you're removing this workaround now. A welcome simplification.
> 
>>                          vl->untag &= ~(BIT(port));
>>
>>                  b53_set_vlan_entry(dev, vid, vl);
>> @@ -2563,6 +2546,8 @@ struct b53_device *b53_switch_alloc(struct device
>> *base,
>>          dev->priv = priv;
>>          dev->ops = ops;
>>          ds->ops = &b53_switch_ops;
>> +       ds->configure_vlan_while_not_filtering = true;
>> +       dev->vlan_enabled = ds->configure_vlan_while_not_filtering;
>>          mutex_init(&dev->reg_mutex);
>>          mutex_init(&dev->stats_mutex);
>>
>>
>> -- 
>> Florian
> 
> Looks good!
> 
> I'm going to hold off with my configure_vlan_while_not_filtering patch.
> You can send this one before me.

That's the plan, thanks!
-- 
Florian