lkml.org 
[lkml]   [2020]   [Aug]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v4 1/2] net: dsa: Add protocol support for 802.1AD when adding or deleting vlan for dsa switch port
    From
    Date


    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

    \
     
     \ /
      Last update: 2020-08-04 00:38    [W:5.401 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site