Re: [PATCH net-next 0/3] net: vlan: fix bridge binding behavior and add selftests

[Roopa Prabhu]

On 7/30/22 09:48, Sevinj Aghayeva wrote:
> (Resending this because the first email was rejected due to being in HTML.)
>
>
> On Sat, Jul 30, 2022 at 12:46 PM Sevinj Aghayeva
> <sevinj.aghayeva@xxxxxxxxx> wrote:
> > On Sat, Jul 30, 2022 at 12:22 PM Nikolay Aleksandrov <razor@xxxxxxxxxxxxx> wrote:
> > > On 7/30/22 19:03, Sevinj Aghayeva wrote:
> > > > When bridge binding is enabled for a vlan interface, it is expected
> > > > that the link state of the vlan interface will track the subset of the
> > > > ports that are also members of the corresponding vlan, rather than
> > > > that of all ports.
> > > >
> > > > Currently, this feature works as expected when a vlan interface is
> > > > created with bridge binding enabled:
> > > >
> > > >     ip link add link br name vlan10 type vlan id 10 protocol 802.1q \
> > > >           bridge_binding on
> > > >
> > > > However, the feature does not work when a vlan interface is created
> > > > with bridge binding disabled, and then enabled later:
> > > >
> > > >     ip link add link br name vlan10 type vlan id 10 protocol 802.1q \
> > > >           bridge_binding off
> > > >     ip link set vlan10 type vlan bridge_binding on
> > > >
> > > > After these two commands, the link state of the vlan interface
> > > > continues to track that of all ports, which is inconsistent and
> > > > confusing to users. This series fixes this bug and introduces two
> > > > tests for the valid behavior.
> > > >
> > > > Sevinj Aghayeva (3):
> > > >     net: bridge: export br_vlan_upper_change
> > > >     net: 8021q: fix bridge binding behavior for vlan interfaces
> > > >     selftests: net: tests for bridge binding behavior
> > > >
> > > >    include/linux/if_bridge.h                     |   9 ++
> > > >    net/8021q/vlan.h                              |   2 +-
> > > >    net/8021q/vlan_dev.c                          |  21 ++-
> > > >    net/bridge/br_vlan.c                          |   7 +-
> > > >    tools/testing/selftests/net/Makefile          |   1 +
> > > >    .../selftests/net/bridge_vlan_binding_test.sh | 143 ++++++++++++++++++
> > > >    6 files changed, 176 insertions(+), 7 deletions(-)
> > > >    create mode 100755 tools/testing/selftests/net/bridge_vlan_binding_test.sh
> > > Hmm.. I don't like this and don't think this bridge function should be
> > > exported at all.
> > >
> > > Calling bridge state changing functions from 8021q module is not the
> > > proper way to solve this. The problem is that the bridge doesn't know
> > > that the state has changed, so you can process NETDEV_CHANGE events and
> > > check for the bridge vlan which got its state changed and react based on
> > > it. I haven't checked in detail, but I think it should be doable. So all
> > > the logic is kept inside the bridge.
> >
> > Hi Nik,
> >
> > Can please elaborate on where I should process NETDEV_CHANGE events? I'm doing this as part of outreachy project and this is my first kernel task, so I don't know the bridging code that well.
> >
> > Thanks!

good point Nikolay.

Sevinj, see br_vlan_bridge_event and __vlan_device_event  for how both 
drivers react to netdev change events.

I have not looked at it in detail yet, but lets explore and discuss if 
we can make use of events to achieve same results.