Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH net-next v2] net: dsa: b53: Do not force CPU to be always
 tagged
To:     Vladimir Oltean <olteanv@gmail.com>
Cc:     netdev@vger.kernel.org, mnhagan88@gmail.com,
        Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        open list <linux-kernel@vger.kernel.org>
References: <20210608212204.3978634-1-f.fainelli@gmail.com>
 <20210609223048.xsnyaoqzr6uhlqsm@skbuf>
From:   Florian Fainelli <f.fainelli@gmail.com>
Message-ID: <01be1409-599d-c16e-6459-f4cb79fcf7a0@gmail.com>
Date:   Wed, 9 Jun 2021 18:05:33 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.10.2
MIME-Version: 1.0
In-Reply-To: <20210609223048.xsnyaoqzr6uhlqsm@skbuf>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 6/9/2021 3:30 PM, Vladimir Oltean wrote:
> On Tue, Jun 08, 2021 at 02:22:04PM -0700, Florian Fainelli wrote:
>> Commit ca8931948344 ("net: dsa: b53: Keep CPU port as tagged in all
>> VLANs") forced the CPU port to be always tagged in any VLAN membership.
>> This was necessary back then because we did not support Broadcom tags
>> for all configurations so the only way to differentiate tagged and
>> untagged traffic while DSA_TAG_PROTO_NONE was used was to force the CPU
>> port into being always tagged.
>>
>> With most configurations enabling Broadcom tags, especially after
>> 8fab459e69ab ("net: dsa: b53: Enable Broadcom tags for 531x5/539x
>> families") we do not need to apply this unconditional force tagging of
>> the CPU port in all VLANs.
>>
>> A helper function is introduced to faciliate the encapsulation of the
>> specific condition requiring the CPU port to be tagged in all VLANs and
>> the dsa_switch_ops::untag_bridge_pvid boolean is moved to when
>> dsa_switch_ops::setup is called when we have already determined the
>> tagging protocol we will be using.
>>
>> Reported-by: Matthew Hagan <mnhagan88@gmail.com>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
> 
> Florian, does the hardware behave in the same way if you disable
> CONFIG_VLAN_8021Q?

Unfortunately it does not because there is no more code calling into
b53_vlan_add() with the desired egress untagged attribute:

00:01:23.015477 b8:ac:6f:80:af:7e (oui Unknown) > 00:10:18:cd:c9:c2 (oui
Unknown), BRCM tag OP: EG, CID: 0, RC: exception, TC: 0, port: 0,
ethertype 802.1Q (0x8100), length 106: vlan 0, p 0, ethertype IPv4
(0x0800), (tos 0x0, ttl 64, id 3662, offset 0, flags [none], proto ICMP
(1), length 84)
    192.168.1.254 > 192.168.1.10: ICMP echo reply, id 5127, seq 17,
length 64
        0x0000:  0010 18cd c9c2 b8ac 6f80 af7e 0000 2000  ........o..~....
        0x0010:  8100 0000 0800 4500 0054 0e4e 0000 4001  ......E..T.N..@.
        0x0020:  e802 c0a8 01fe c0a8 010a 0000 bc2c 1407  .............,..
        0x0030:  0011 3db6 f204 0000 0000 0000 0000 0000  ..=.............
        0x0040:  0000 0000 0000 0000 0000 0000 0000 0000  ................
        0x0050:  0000 0000 0000 0000 0000 0000 0000 0000  ................
        0x0060:  0000 0000 0000 0000 0000

Something like this works:

diff --git a/drivers/net/dsa/b53/b53_common.c
b/drivers/net/dsa/b53/b53_common.c
index 6e199454e41d..ac3bb5ef17e2 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -748,9 +748,13 @@ int b53_configure_vlan(struct dsa_switch *ds)

        b53_enable_vlan(dev, -1, dev->vlan_enabled, ds->vlan_filtering);

-       b53_for_each_port(dev, i)
+       b53_for_each_port(dev, i) {
+               v = &dev->vlans[def_vid];
+               v->members |= BIT(i);
+               v->untag = v->members;
                b53_write16(dev, B53_VLAN_PAGE,
                            B53_VLAN_PORT_DEF_TAG(i), def_vid);
+       }

        /* Upon initial call we have not set-up any VLANs, but upon
         * system resume, we need to restore all VLAN entries.
-- 
Florian