On Sat, Apr 12, 2025 at 3:50 PM Jonas Gorski <jonas.gor...@gmail.com> wrote: > > On Sat, Apr 12, 2025 at 3:34 PM Vladimir Oltean <olte...@gmail.com> wrote: > > > > On Sat, Apr 12, 2025 at 02:24:26PM +0200, Jonas Gorski wrote: > > > While trying to figure out the hardware behavior of a DSA supported > > > switch chip and printing various internal vlan state changes, I noticed > > > that some flows never triggered adding the cpu port to vlans, preventing > > > it from receiving any of the VLANs traffic. > > > > > > E.g. the following sequence would cause the cpu port not being member of > > > the vlan, despite the bridge vlan output looking correct: > > > > > > $ ip link add swbridge type bridge vlan_filtering 1 vlan_default_pvid 1 > > > $ ip link set lan1 master swbridge > > > > At this step, dsa_port_bridge_join() -> switchdev_bridge_port_offload() > > -> ... -> br_switchdev_port_offload() -> nbp_switchdev_sync_objs() -> > > br_switchdev_vlan_replay() -> > > br_switchdev_vlan_replay_group(br_vlan_group(br)) > > -> br_switchdev_vlan_replay_one() should have notified DSA, with > > changed=false. > > It should be processed by dsa_user_host_vlan_add() -> > > dsa_port_host_vlan_add(). > > > > You make it sound like that doesn't happen. > > Yes, because I messed up writing down what I did. That should have > been vlan_default_pvid 0 so there are no VLANs configured by default. > > > > I notice you didn't mention which "DSA supported chip" you are using. > > By any chance, does its driver set ds->configure_vlan_while_not_filtering = > > false? > > That would be my prime suspect, making dsa_port_skip_vlan_configuration() > > ignore > > the code path above, because the bridge port is not yet VLAN filtering. > > It becomes VLAN filtering only a bit later in dsa_port_bridge_join(), > > with the dsa_port_switchdev_sync_attrs() -> > > dsa_port_vlan_filtering(br_vlan_enabled(br)) > > call. > > > > If that is the case, the only thing that is slightly confusing to me is > > why you haven't seen the "skipping configuration of VLAN" extack message. > > But anyway, if the theory above is true, you should instead be looking > > at adding proper VLAN support to said driver, and drop this set instead, > > because VLAN replay isn't working properly. > > It's b53, and on first glance it does not set it. But as I just > noticed, I messed up the example.
So I had the chance to properly check and b53 does not unset it, so it should be true. > So adding the lan1 vid is supposed to be the very first vlan that is > configured. > > > > > > $ bridge vlan add dev lan1 vid 1 pvid untagged > > > $ bridge vlan add dev swbridge vid 1 pvid untagged self > > > > > > Adding more printk debugging, I traced it br_vlan_add_existing() setting > > > changed to true (since the vlan "gained" the pvid untagged flags), and > > > then the dsa code ignoring the vlan notification, since it is a vlan for > > > the cpu port that is updated. > > > > Yes, this part and everything that follows should be correct. So as an addendum that I probably should have included in the cover letter and commit message: Starting with no vlans configured on the bridge: $ bridge vlan add dev lan1 vid 1 pvid untagged $ bridge vlan add dev swbridge vid 1 pvid untagged self does not work, but $ bridge vlan add dev lan1 vid 1 pvid untagged $ bridge vlan add dev swbridge vid 1 self does properly trigger a configuration of the vlan on the cpu port. And the difference is that in the former, would_change => vlan->changed is true (due to "pvid untagged"), and in the latter it is not. Best regards, Jonas
