RE: [EXT] Re: [PATCH v4 1/2] net: dsa: Add protocol support for 802.1AD when adding or deleting vlan for dsa switch port

[Hongbo Wang]

> You are adding a new member to the switchdev VLAN object, so you should
> make sure that all call paths creating and parsing that object get updated as
> well, for now, you are doing this solely within DSA which is probably
> reasonable if we assume proto is uninitialized and unused elsewhere, there is
> no change of functionality.

I will review the related code, and confirm it.

> [snip]
> > +             ret = br_vlan_get_proto(dp->bridge_dev, &br_proto);
> > +             if (ret == 0 && br_proto != vlan_proto)
> > +                     change_proto = true;
> 
> 
> This deserves a comment, because the change_proto variable is not really
> explaining what this is about, maybe more like "incompatible_proto" would?
> 
> First you query the VLAN protocol currently configured on the bridge master
> device, and if this VLAN protocol is different than the one being requested,
> then you treat this as an error. It might make sense to also print a message
> towards the user that the bridge device protocol should be changed, or that
> the bridge device should be removed and re-created accordingly.
> 
> Does it not work if we have a bridge currently configured with 802.1ad and a
> 802.1q VLAN programming request comes in? In premise it should, right?
> Likewise, if we had a 802.1ad bridge configured already and we want to
> configure a 802.1Q VLAN on a bridged port, there should be a way for this
> configuration to work.
> 
> And both cases, it ought to be possible to configure the switch in double
> tagged mode and just make sure that there is no S-tag being added unless
> requested.

Thanks for long reply.

When create a bridge br0, it's default protocol is 802.1Q, then add a port swp1 to bridge, the bridge will call add_vlan add a default VLAN to swp1, its vid is pvid 1, the vlan's protocol is 802.1Q. the related command is:
# ip link add dev br0 type bridge
# ip link set dev swp1 master br0

After testing port's 802.1Q mode, If want to test 802.1AD mode, we can use the following command to set switch and its  ports into QinQ mode, 
# ip link set br0 type bridge vlan_protocol 802.1ad
it will call vlan_vid_add(dev, proto, vid) for every port, the parameter proto is 802.1AD and the vid is also 1, after that, it will set br->vlan_proto to 802.1AD, the related code is __br_vlan_set_proto in br_vlan.c

vlan_vid_add will call dsa_slave_vlan_rx_add_vid.
But in dsa_slave_vlan_rx_add_vid, because we had added vlan 1 before, so br_vlan_get_info will return 0, then the function return -EBUSY, it can't call dsa_port_vid_add, so I add the code to check protocol changing.

I will add code to print the message for protocol changing.

> > +             ret = br_vlan_get_proto(dp->bridge_dev, &br_proto);
> > +             if (ret == 0 && br_proto != vlan_proto)
> > +                     change_proto = true;
> > +
> >               /* br_vlan_get_info() returns -EINVAL or -ENOENT if the
> >                * device, respectively the VID is not found, returning
> >                * 0 means success, which is a failure for us here.
> >                */
> >               ret = br_vlan_get_info(dp->bridge_dev, vid, &info);
> > -             if (ret == 0)
> > +             if (ret == 0 && !change_proto)
> >                       return -EBUSY;
> 
> Since we are copying the same code than in the add_vid path, it might make
> sense to extract this to a helper function eventually.

I will extract the code to helper function.

> >       slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
> >       if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
> > -             slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> > +             slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER |
> > +
> NETIF_F_HW_VLAN_STAG_FILTER;
> 
> You cannot advertise this netdev feature for *all* DSA switch driver unless you
> have verified that each DSA driver implementing
> port_vlan_add() will work correctly. Please assign this flag from within the
> ocelot driver for now.

I will change it in next version.

> 
> >
> >       if (enabled)
> > -             return dsa_port_vid_add(dp, vid, flags);
> > +             return dsa_port_vid_add(dp, vid, 0, flags);
> 
> Why not pass ETH_P_8021Q here to indicate we want a 802.1Q, not .AD
> configuration request?
> --
I will change it in next version.