Hi Scott, On Tue, May 26, 2015 at 02:04:00AM -0700, Scott Feldman wrote: > On Tue, May 26, 2015 at 12:28 AM, Scott Feldman <sfel...@gmail.com> wrote: > > On Mon, May 25, 2015 at 5:55 PM, Simon Horman > > <simon.hor...@netronome.com> wrote: > >> This will occur anyway if the 8021q module is loaded as it will > >> call rocker_port_vlan_rx_add_vid for vlan 0. This code is here > >> to handle the case where the 8021q module is not loaded. > >> > >> This patch also handles the case where the 8021q is unloaded > >> removing all VLANs from all ports. > >> > >> This change should not affect bridging, although the rules are > >> harmlessly installed anyway. This is in keeping with the behaviour > >> for VLANs when the 8021q modules is loaded. > >> > >> To aid implementation of the above provide a helper > >> and use it to replace some existing code. > >> > >> Signed-off-by: Simon Horman <simon.hor...@netronome.com> > > [cut] > > > > > Hi Simon, > > > > Thanks for looking into this one. I looked at your patch and the code > > and I think we can streamline it a little bit more. For the > > no-8021q-module case we use rocker_port_vlan_add() and > > rocker_port_vlan_del() to add/del bridge vlans. We should be able to > > move those functions up in the file so they can be called from > > rocker_port_vlan_rx_add_vid() and rocker_port_vlan_rx_kill_vid(), > > passing trans=SWITCHDEV_TRANS_NONE, but only if vid != 0. Next, like > > in your patch, we need to call rocker_port_vlan_add() in > > rocker_port_open(), passing in vid=0 for untagged. And in > > rocker_port_stop(), call rocker_port_vlan_del(), again passing in > > vid=0. > > > > To summarize: > > > > Call rocker_port_vlan_add() from: > > > > 1) rocker_port_open with vid=0 > > 2) rocker_port_vlans_add() // bridge vlan > > 3) rocker_port_vlan_rx_add_vid() if vid != 0 // 8021q vlan > > > > Call rocker_port_vlan_del() from: > > > > 1) rocker_port_stop with vid=0 > > 2) rocker_port_vlans_del() // bridge vlan > > 3) rocker_port_vlan_rx_kill_vid() if vid != 0 // 8021q vlan > > > > Does this look right?
It seems like it ought to work, I can try implementing the above idea if you think it is worthwhile. Can I clarify that its ok to ignore vid != 0 in rocker_port_vlan_rx_add_vid() and rocker_port_vlan_rx_kill_vid()? If so I think that leads to an easy simplification of my proposed change to rocker_port_vlan_rx_kill_vid(): the logic to restore vlan 0 if no vlans are present can be dropped. Of course your suggestion goes further than that. > Hmmmm...things get simpler if we removed support for 8021q module in > rocker driver by removing NETIF_F_HW_VLAN_CTAG_FILTER. That gets rid > of rocker_port_vlan_rx_add_vid() and rocker_port_vlan_rx_kill_vid(). > Leaving us with the bridge VLAN interface to add/del/show vlans on the > port. I'm wondering if we can also avoid setting up untagged traffic > on the port during port open by requiring a explicit command on the > port from the user: > > bridge vlan add vid 0 dev DEV master self // enable untagged > traffic on port I have some questions about that approach: * Does that behaviour differ from other devices (that don't set NETIF_F_HW_VLAN_CTAG_FILTER)? It seems like it may be an extra unnecessary step to me. * Does that behaviour change the current behaviour supported by rocker? If so it seems unwise to change it. * Does the scheme described above work when rocker ports are not bridged? This is the scenario I am interested in at this time. > > Do you have a requirement for 8021q module? I'm leaning towards a > clean break from 8021q and using just the built-in bridge VLAN > support. I'm curious if others have an opinion about 8021q module > used with switchdev devices. I do not have a requirement on the 8021q module at this time. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
