Message-ID: <20200725173551.GN23489@earth.li>
Date:   Sat, 25 Jul 2020 18:35:51 +0100
From:   Jonathan McDowell <noodles@...th.li>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Matthew Hagan <mnhagan88@...il.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] net: dsa: qca8k: Add 802.1q VLAN support

On Thu, Jul 23, 2020 at 01:58:47AM +0300, Vladimir Oltean wrote:
> On Wed, Jul 22, 2020 at 03:36:38PM -0700, Florian Fainelli wrote:
> > On 7/22/20 12:38 PM, Jonathan McDowell wrote:
> > > On Tue, Jul 21, 2020 at 10:26:07AM -0700, Florian Fainelli wrote:
> > >> On 7/21/20 10:16 AM, Jonathan McDowell wrote:
> > >>> This adds full 802.1q VLAN support to the qca8k, allowing the use of
> > >>> vlan_filtering and more complicated bridging setups than allowed by
> > >>> basic port VLAN support.
> > >>>
> > >>> Tested with a number of untagged ports with separate VLANs and then a
> > >>> trunk port with all the VLANs tagged on it.
> > >>
> > >> This looks good to me at first glance, at least not seeing obvious
> > >> issue, however below are a few questions:
> > > 
> > > Thanks for the comments.
> > > 
> > >> - vid == 0 appears to be unsupported according to your port_vlan_prepare
> > >> callback, is this really the case, or is it more a case of VID 0 should
> > >> be pvid untagged, which is what dsa_slave_vlan_rx_add_vid() would
> > >> attempt to program
> > > 
> > > I don't quite follow you here. VID 0 doesn't appear to be supported by
> > > the hardware (and several other DSA drivers don't seem to like it
> > > either), hence the check; I'm not clear if there's something alternate I
> > > should be doing in that case instead?
> > 
> > Is it really that the hardware does not support it, or is it that it was
> > not tried or poorly handled before? If the switch supports programming
> > the VID 0 entry as PVID egress untagged, which is what
> > dsa_slave_vlan_rx_add_vid() requests, then this is great, because you
> > have almost nothing to do.
> > 
> > If not, what you are doing is definitively okay, because you have a
> > port_bridge_join that ensures that the port matrix gets configured
> > correctly for bridging, if that was not supported we would be in trouble.
> 
> Things added by dsa_slave_vlan_rx_add_vid() are definitely not "pvid
> untagged", they are installed with flags=0 which means "non-pvid,
> egress-tagged", aka a simple vlan with tagged ingress and egress.
> 
> The case of VLAN 0 is special because according to 802.1Q it is "not a
> VLAN", but simply a way to transmit the other stuff in a VLAN header,
> namely a class of service (VLAN PCP). The VLAN ID should not be used for
> segregation of forwarding domains, should not be assigned as port-based
> VLAN to untagged traffic (from what I recall from the 802.1Q standard)
> and should always be sent as egress-tagged.
...
> So maybe it's worth checking what is your switch's behavior with regard
> to VLAN 0. If it already does what's supposed to, then you might just as
> well stop fighting the system and silently accept the configuration
> while not doing anything.  As Russell implied, the bridge can't add a
> VLAN of 0. It is just the 8021q module that does it.  The fact that we
> have the same callbacks being used for both in DSA is merely an artefact
> of implementation.

Ok, thanks for the clarification, that helps a lot.

I've done some experimentation injecting packets on untagged ports with
VLAN 0 headers. It looks like it's doing the right thing; the intact
VLAN 0 / classification comes through to the CPU port, and the packet is
also correctly sent out tagged with the correct VLAN (from the untagged
port configuration) on a trunk port. So I think I can just silently drop
the request for VLAN 0 configuration rather than returning an error.

> > >> - since we have a qca8k_port_bridge_join() callback which programs the
> > >> port lookup control register, putting all ports by default in VID==1
> > >> does not break per-port isolation between non-bridged and bridged ports,
> > >> right?
> > > 
> > > VLAN_MODE_NONE (set when we don't have VLAN filtering enabled)
> > > configures us for port filtering, ignoring the VLAN info, so I think
> > > we're good here.
> > 
> > OK, good, so just to be sure, there is no cross talk between non-bridged
> > ports and bridged ports even when VLAN filtering is not enabled on the
> > bridge, right?

Yup. When VLAN filtering is off off we only allow ports to talk to each
other that we get bridge_join calls for (that's the way the device is
currently supported by the kernel).

J.

-- 
/-\                             |  Be Ye Not Lost Among Precepts of
|@/  Debian GNU/Linux Developer |                Order
\-                              |

Powered by blists - more mailing lists