[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 9 Sep 2020 10:22:42 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: davem@...emloft.net, vivien.didelot@...il.com, andrew@...n.ch,
netdev@...r.kernel.org
Subject: Re: [PATCH net-next 4/4] net: dsa: set
configure_vlan_while_not_filtering to true by default
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
Powered by blists - more mailing lists