Messages in this thread | | | Subject | Re: [PATCH v4 1/2] net: dsa: Add protocol support for 802.1AD when adding or deleting vlan for dsa switch port | From | Florian Fainelli <> | Date | Mon, 3 Aug 2020 15:38:31 -0700 |
| |
On 7/30/2020 3:25 AM, hongbo.wang@nxp.com wrote: > From: "hongbo.wang" <hongbo.wang@nxp.com> > > the following command will be supported: > > Set bridge's vlan protocol: > ip link set br0 type bridge vlan_protocol 802.1ad > Add VLAN: > ip link add link swp1 name swp1.100 type vlan protocol 802.1ad id 100 > Delete VLAN: > ip link del link swp1 name swp1.100 > > Signed-off-by: hongbo.wang <hongbo.wang@nxp.com> > --- > include/net/switchdev.h | 1 + > net/dsa/dsa_priv.h | 4 ++-- > net/dsa/port.c | 6 ++++-- > net/dsa/slave.c | 27 +++++++++++++++++++++------ > net/dsa/tag_8021q.c | 4 ++-- > 5 files changed, 30 insertions(+), 12 deletions(-) > > diff --git a/include/net/switchdev.h b/include/net/switchdev.h > index ff2246914301..7594ea82879f 100644 > --- a/include/net/switchdev.h > +++ b/include/net/switchdev.h > @@ -97,6 +97,7 @@ struct switchdev_obj_port_vlan { > u16 flags; > u16 vid_begin; > u16 vid_end; > + u16 proto;
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.
[snip]
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 41d60eeefdbd..2a03da92af0a 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -1233,7 +1233,10 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto, > u16 vid) > { > struct dsa_port *dp = dsa_slave_to_port(dev); > + u16 vlan_proto = ntohs(proto); > struct bridge_vlan_info info; > + bool change_proto = false; > + u16 br_proto = 0; > int ret; > > /* Check for a possible bridge VLAN entry now since there is no > @@ -1243,20 +1246,24 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto, > if (dsa_port_skip_vlan_configuration(dp)) > return 0; > > + 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.
> + > /* 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; > } > > - ret = dsa_port_vid_add(dp, vid, 0); > + ret = dsa_port_vid_add(dp, vid, vlan_proto, 0); > if (ret) > return ret; > > - ret = dsa_port_vid_add(dp->cpu_dp, vid, 0); > + ret = dsa_port_vid_add(dp->cpu_dp, vid, 0, 0); > if (ret) > return ret; > > @@ -1267,7 +1274,10 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, > u16 vid) > { > struct dsa_port *dp = dsa_slave_to_port(dev); > + u16 vlan_proto = ntohs(proto); > struct bridge_vlan_info info; > + bool change_proto = false; > + u16 br_proto = 0; > int ret; > > /* Check for a possible bridge VLAN entry now since there is no > @@ -1277,19 +1287,23 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, > if (dsa_port_skip_vlan_configuration(dp)) > return 0; > > + 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.
> } > > /* Do not deprogram the CPU port as it may be shared with other user > * ports which can be members of this VLAN as well. > */ > - return dsa_port_vid_del(dp, vid); > + return dsa_port_vid_del(dp, vid, vlan_proto); > } > > struct dsa_hw_port { > @@ -1744,7 +1758,8 @@ int dsa_slave_create(struct dsa_port *port) > > 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.
> slave_dev->hw_features |= NETIF_F_HW_TC; > slave_dev->features |= NETIF_F_LLTX; > slave_dev->ethtool_ops = &dsa_slave_ethtool_ops; > diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c > index 780b2a15ac9b..848f85ed5c0f 100644 > --- a/net/dsa/tag_8021q.c > +++ b/net/dsa/tag_8021q.c > @@ -152,9 +152,9 @@ static int dsa_8021q_vid_apply(struct dsa_switch *ds, int port, u16 vid, > struct dsa_port *dp = dsa_to_port(ds, port); > > 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? -- Florian
| |