Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Wed, 27 Sep 2023 02:58:48 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     =?utf-8?B?UGF3ZcWC?= Dembicki <paweldembicki@gmail.com>
Cc:     netdev@vger.kernel.org, Dan Carpenter <dan.carpenter@linaro.org>,
        Simon Horman <simon.horman@corigine.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Russell King <linux@armlinux.org.uk>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering
Message-ID: <20230926235848.3uftpkj7m24qsord@skbuf>
References: <20230912122201.3752918-1-paweldembicki@gmail.com>
 <20230912122201.3752918-6-paweldembicki@gmail.com>
 <20230912161709.g34slexfaop6xp7w@skbuf>
 <CAJN1Kkwzwt++6GtrAnCbKzYto-uQECYZz5=N7bePqK9wsK2_+g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAJN1Kkwzwt++6GtrAnCbKzYto-uQECYZz5=N7bePqK9wsK2_+g@mail.gmail.com>
Precedence: bulk

On Fri, Sep 22, 2023 at 04:26:00PM +0200, Paweł Dembicki wrote:
> > > +             if (vsc->untagged_storage[port] < VLAN_N_VID &&
> > > +                 !vid_is_dsa_8021q(vsc->untagged_storage[port]) &&
> > > +                 !vid_is_dsa_8021q(vid) &&
> >
> > The problem here which led to these vid_is_dsa_8021q() checks is that
> > dsa_switch_tag_8021q_vlan_add() sets all flags on user ports to
> > BRIDGE_VLAN_INFO_UNTAGGED | BRIDGE_VLAN_INFO_PVID, and you can't offload
> > those, correct?
> 
> In my case, the major problem with tag8021q vlans is
> "dsa_tag_8021q_bridge_join" function:
> "dsa_port_tag_8021q_vlan_add" is called before "dsa_port_tag_8021q_vlan_del".
> I must disable pvid/untagged checking, because it will always fail. I
> let kernel do the job,
> it keeps only one untagged/pvid per port after "dsa_tag_8021q_bridge_join".

I'm not sure that you described the problem in a way that I can understand, here.

After dsa_tag_8021q_bridge_join():
-> dsa_port_tag_8021q_vlan_add(dp, bridge_vid)
-> dsa_port_tag_8021q_vlan_del(dp, standalone_vid)

it's *expected* that there should be only one untagged/pvid per port: the bridge_vid.

For context, consider the fact that you can run the following commands:

bridge vlan add dev eth0 vid 10 pvid
bridge vlan add dev eth0 vid 11 pvid

and after the second command, vid 10 stops being a pvid.

So I think that the "Port %d can have only one pvid! Now is: %d.\n" behavior
is not correct. You need to implement the pvid overwriting behavior, since
there's always only 1 pvid.

So that leaves the "untagged" flag as being problematic, correct? Could
you comment...

> 
> > But when the port is VSC73XX_VLAN_IGNORE mode (and
> > tag_8021q is active), VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0, and thus,
> > *all* VLANs are egress-untagged VLANs, correct?
> >
> > If that is the case, why do you call vsc73xx_vlan_set_untagged() in the
> > first place, for tag_8021q VLANs, if you don't rely on the port's native
> > VLAN for egress untagging?

... on this? Here I'm pointing out that "all VLANs have the egress-untagged flag"
is a configuration that can actually be supported by vsc73xx. You just
need to ensure that VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0. And tag_8021q
basically requests exactly that configuration on user ports (both the
bridge_vid and the standalone_vid are egress-untagged). So your check is
too restrictive, you are denying a configuration that would work.
The problem only appears when you mix egress-tagged with egress-untagged
VLANs on a port. Only then there can be at most 1 egress-untagged VID,
because you need to enable VSC73XX_TXUPDCFG_TX_INSERT_TAG for the
egress-tagged VIDs to work.

> > A comment would be good which states that the flipping between the
> > hardware and the storage values relies on the fact that vsc73xx_port_vlan_filtering()
> > only gets called on actual changes to vlan_filtering, and thus, to
> > vsc73xx_tag_8021q_active(). So, we know for sure that what is in storage
> > needs to go to hardware, and what is in hardware needs to go to storage.
> >
> > It's an interesting implementation for sure.
> >
> 
> Thank you.

I'm not sure if that was a compliment :) At least in this form, it's
certainly non-trivial to determine by looking at the code if it is
correct or not, and it uses different patterns than the other VLAN
implementations in DSA drivers. Generally, boring and obvious is
preferable. But after I took the time to understand, it seems plausible
that the approach might work.

Let's see how the same idea looks, cleaned up a bit but not redesigned,
in v4.