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 \- |
