[PATCH v4 0/2] Add 802.1AD protocol support for dsa switch and ocelot driver

[PATCH v4 0/2] Add 802.1AD protocol support for dsa switch and ocelot driver

[hongbo.wang]

From: "hongbo.wang" <hongbo.wang@nxp.com>

1. the patch 0001* is for setting single port into 802.1AD(QinQ) mode,
before this patch, the function dsa_slave_vlan_rx_add_vid didn't pass 
the parameter "proto" to next port level, so switch's port can't get
parameter "proto"
  after applying this patch, the following command can be supported:
  ip link set br0 type bridge vlan_protocol 802.1ad
  ip link add link swp1 name swp1.100 type vlan protocol 802.1ad id 100

2. the patch 0002* is for setting QinQ related registers in ocelot 
switch driver, after applying this patch, the switch(VSC99599)'s port can
enable or disable QinQ mode.

3. Version log
v4: 
a. modify slave.c to support "ip set br0 type bridge vlan_protocol 802.1ad"
b. modify ocelot.c, if enable QinQ, set VLAN_AWARE_ENA and VLAN_POP_CNT per
   port when vlan_filter=1
v3: combine two patches to one post

hongbo.wang (2):
  net: dsa: Add protocol support for 802.1AD when adding or   deleting
    vlan for dsa switch port
  net: dsa: ocelot: Add support for QinQ Operation

 drivers/net/dsa/ocelot/felix.c     | 12 +++++++
 drivers/net/ethernet/mscc/ocelot.c | 53 +++++++++++++++++++++++++-----
 include/net/switchdev.h            |  1 +
 include/soc/mscc/ocelot.h          |  2 ++
 net/dsa/dsa_priv.h                 |  4 +--
 net/dsa/port.c                     |  6 ++--
 net/dsa/slave.c                    | 27 +++++++++++----
 net/dsa/tag_8021q.c                |  4 +--
 8 files changed, 89 insertions(+), 20 deletions(-)

-- 
2.17.1

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

[hongbo.wang]

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;
 };
 
 #define SWITCHDEV_OBJ_PORT_VLAN(OBJ) \
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 1653e3377cb3..52685b9875e5 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -164,8 +164,8 @@ int dsa_port_vlan_add(struct dsa_port *dp,
 		      struct switchdev_trans *trans);
 int dsa_port_vlan_del(struct dsa_port *dp,
 		      const struct switchdev_obj_port_vlan *vlan);
-int dsa_port_vid_add(struct dsa_port *dp, u16 vid, u16 flags);
-int dsa_port_vid_del(struct dsa_port *dp, u16 vid);
+int dsa_port_vid_add(struct dsa_port *dp, u16 vid, u16 proto, u16 flags);
+int dsa_port_vid_del(struct dsa_port *dp, u16 vid, u16 proto);
 int dsa_port_link_register_of(struct dsa_port *dp);
 void dsa_port_link_unregister_of(struct dsa_port *dp);
 extern const struct phylink_mac_ops dsa_port_phylink_mac_ops;
diff --git a/net/dsa/port.c b/net/dsa/port.c
index e23ece229c7e..c98bbac3980a 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -433,13 +433,14 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 	return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
 }
 
-int dsa_port_vid_add(struct dsa_port *dp, u16 vid, u16 flags)
+int dsa_port_vid_add(struct dsa_port *dp, u16 vid, u16 proto, u16 flags)
 {
 	struct switchdev_obj_port_vlan vlan = {
 		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
 		.flags = flags,
 		.vid_begin = vid,
 		.vid_end = vid,
+		.proto = proto,
 	};
 	struct switchdev_trans trans;
 	int err;
@@ -454,12 +455,13 @@ int dsa_port_vid_add(struct dsa_port *dp, u16 vid, u16 flags)
 }
 EXPORT_SYMBOL(dsa_port_vid_add);
 
-int dsa_port_vid_del(struct dsa_port *dp, u16 vid)
+int dsa_port_vid_del(struct dsa_port *dp, u16 vid, u16 proto)
 {
 	struct switchdev_obj_port_vlan vlan = {
 		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
 		.vid_begin = vid,
 		.vid_end = vid,
+		.proto = proto,
 	};
 
 	return dsa_port_vlan_del(dp, &vlan);
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;
+
 		/* 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;
 	}
 
 	/* 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;
 	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);
 
-	return dsa_port_vid_del(dp, vid);
+	return dsa_port_vid_del(dp, vid, 0);
 }
 
 /* RX VLAN tagging (left) and TX VLAN tagging (right) setup shown for a single
-- 
2.17.1

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

[Florian Fainelli]

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

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.

[PATCH v4 2/2] net: dsa: ocelot: Add support for QinQ Operation

[hongbo.wang]

From: "hongbo.wang" <hongbo.wang@nxp.com>

This featue can be test using network test tools
TX-tool -----> swp0  -----> swp1 -----> RX-tool

TX-tool simulates Customer that will send and receive packets with single
VLAN tag(CTAG), RX-tool simulates Service-Provider that will send and
receive packets with double VLAN tag(STAG and CTAG). This refers to
"4.3.3 Provider Bridges and Q-in-Q Operation" in VSC99599_1_00_TS.pdf.

The related test commands:
1.
ip link add dev br0 type bridge
ip link set dev swp1 master br0
ip link set br0 type bridge vlan_protocol 802.1ad
ip link set dev swp0 master br0

2.
ip link set dev br0 type bridge vlan_filtering 1
bridge vlan add dev swp0 vid 100 pvid
bridge vlan add dev swp1 vid 100
Result:
Customer(tpid:8100 vid:111) -> swp0 -> swp1 -> ISP(STAG \
                tpid:88A8 vid:100, CTAG tpid:8100 vid:111)

3.
bridge vlan del dev swp0 vid 1 pvid
bridge vlan add dev swp0 vid 100 pvid untagged
Result:
ISP(tpid:88A8 vid:100 tpid:8100 vid:222) -> swp1 -> swp0 ->\
		Customer(tpid:8100 vid:222)

Signed-off-by: hongbo.wang <hongbo.wang@nxp.com>
---
 drivers/net/dsa/ocelot/felix.c     | 12 +++++++
 drivers/net/ethernet/mscc/ocelot.c | 53 +++++++++++++++++++++++++-----
 include/soc/mscc/ocelot.h          |  2 ++
 3 files changed, 59 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index c69d9592a2b7..72a27b61080e 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -131,10 +131,16 @@ static void felix_vlan_add(struct dsa_switch *ds, int port,
 			   const struct switchdev_obj_port_vlan *vlan)
 {
 	struct ocelot *ocelot = ds->priv;
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
 	u16 flags = vlan->flags;
 	u16 vid;
 	int err;
 
+	if (vlan->proto == ETH_P_8021AD) {
+		ocelot->enable_qinq = true;
+		ocelot_port->qinq_mode = true;
+	}
+
 	if (dsa_is_cpu_port(ds, port))
 		flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
 
@@ -154,9 +160,15 @@ static int felix_vlan_del(struct dsa_switch *ds, int port,
 			  const struct switchdev_obj_port_vlan *vlan)
 {
 	struct ocelot *ocelot = ds->priv;
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
 	u16 vid;
 	int err;
 
+	if (vlan->proto == ETH_P_8021AD) {
+		ocelot->enable_qinq = false;
+		ocelot_port->qinq_mode = false;
+	}
+
 	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
 		err = ocelot_vlan_del(ocelot, port, vid);
 		if (err) {
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index f2d94b026d88..b5fec6855afd 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -143,6 +143,8 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
 				       u16 vid)
 {
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
+	u32 port_tpid = 0;
+	u32 tag_tpid = 0;
 	u32 val = 0;
 
 	if (ocelot_port->vid != vid) {
@@ -156,8 +158,14 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
 		ocelot_port->vid = vid;
 	}
 
-	ocelot_rmw_gix(ocelot, REW_PORT_VLAN_CFG_PORT_VID(vid),
-		       REW_PORT_VLAN_CFG_PORT_VID_M,
+	if (ocelot_port->qinq_mode)
+		port_tpid = REW_PORT_VLAN_CFG_PORT_TPID(ETH_P_8021AD);
+	else
+		port_tpid = REW_PORT_VLAN_CFG_PORT_TPID(ETH_P_8021Q);
+
+	ocelot_rmw_gix(ocelot, REW_PORT_VLAN_CFG_PORT_VID(vid) | port_tpid,
+		       REW_PORT_VLAN_CFG_PORT_VID_M |
+		       REW_PORT_VLAN_CFG_PORT_TPID_M,
 		       REW_PORT_VLAN_CFG, port);
 
 	if (ocelot_port->vlan_aware && !ocelot_port->vid)
@@ -180,12 +188,28 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
 		else
 			/* Tag all frames */
 			val = REW_TAG_CFG_TAG_CFG(3);
+
+		if (ocelot_port->qinq_mode)
+			tag_tpid = REW_TAG_CFG_TAG_TPID_CFG(1);
+		else
+			tag_tpid = REW_TAG_CFG_TAG_TPID_CFG(0);
 	} else {
-		/* Port tagging disabled. */
-		val = REW_TAG_CFG_TAG_CFG(0);
+		if (ocelot_port->qinq_mode) {
+			if (ocelot_port->vid)
+				val = REW_TAG_CFG_TAG_CFG(1);
+			else
+				val = REW_TAG_CFG_TAG_CFG(3);
+
+			tag_tpid = REW_TAG_CFG_TAG_TPID_CFG(1);
+		} else {
+			/* Port tagging disabled. */
+			val = REW_TAG_CFG_TAG_CFG(0);
+			tag_tpid = REW_TAG_CFG_TAG_TPID_CFG(0);
+		}
 	}
-	ocelot_rmw_gix(ocelot, val,
-		       REW_TAG_CFG_TAG_CFG_M,
+
+	ocelot_rmw_gix(ocelot, val | tag_tpid,
+		       REW_TAG_CFG_TAG_CFG_M | REW_TAG_CFG_TAG_TPID_CFG_M,
 		       REW_TAG_CFG, port);
 
 	return 0;
@@ -204,6 +228,15 @@ void ocelot_port_vlan_filtering(struct ocelot *ocelot, int port,
 		      ANA_PORT_VLAN_CFG_VLAN_POP_CNT(1);
 	else
 		val = 0;
+
+	/* if switch is enabled for QinQ, the port for LAN should set
+	 * VLAN_CFG.VLAN_POP_CNT=0 && VLAN_CFG.VLAN_AWARE_ENA=0.
+	 * the port for MAN should set VLAN_CFG.VLAN_POP_CNT=1 &&
+	 * VLAN_CFG.VLAN_AWARE_ENA=1. referring to 4.3.3 in VSC9959_1_00_TS.pdf
+	 */
+	if (ocelot->enable_qinq && !ocelot_port->qinq_mode)
+		val = 0;
+
 	ocelot_rmw_gix(ocelot, val,
 		       ANA_PORT_VLAN_CFG_VLAN_AWARE_ENA |
 		       ANA_PORT_VLAN_CFG_VLAN_POP_CNT_M,
@@ -217,10 +250,14 @@ EXPORT_SYMBOL(ocelot_port_vlan_filtering);
 static void ocelot_port_set_pvid(struct ocelot *ocelot, int port, u16 pvid)
 {
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
+	u32 tag_type = 0;
+
+	if (ocelot_port->qinq_mode)
+		tag_type = ANA_PORT_VLAN_CFG_VLAN_TAG_TYPE;
 
 	ocelot_rmw_gix(ocelot,
-		       ANA_PORT_VLAN_CFG_VLAN_VID(pvid),
-		       ANA_PORT_VLAN_CFG_VLAN_VID_M,
+		       ANA_PORT_VLAN_CFG_VLAN_VID(pvid) | tag_type,
+		       ANA_PORT_VLAN_CFG_VLAN_VID_M | tag_type,
 		       ANA_PORT_VLAN_CFG, port);
 
 	ocelot_port->pvid = pvid;
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index da369b12005f..59020bb8fe68 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -556,6 +556,7 @@ struct ocelot_port {
 	struct regmap			*target;
 
 	bool				vlan_aware;
+	bool				qinq_mode;
 
 	/* Ingress default VLAN (pvid) */
 	u16				pvid;
@@ -632,6 +633,7 @@ struct ocelot {
 	/* Protects the PTP clock */
 	spinlock_t			ptp_clock_lock;
 	struct ptp_pin_desc		ptp_pins[OCELOT_PTP_PINS_NUM];
+	bool				enable_qinq;
 };
 
 struct ocelot_policer {
-- 
2.17.1

Re: [PATCH v4 2/2] net: dsa: ocelot: Add support for QinQ Operation

[David Miller]

From: hongbo.wang@nxp.com
Date: Thu, 30 Jul 2020 18:25:05 +0800

> +	if (vlan->proto == ETH_P_8021AD) {
> +		ocelot->enable_qinq = true;
> +		ocelot_port->qinq_mode = true;
> +	}
 ...
> +	if (vlan->proto == ETH_P_8021AD) {
> +		ocelot->enable_qinq = false;
> +		ocelot_port->qinq_mode = false;
> +	}
> +

I don't understand how this can work just by using a boolean to track
the state.

This won't work properly if you are handling multiple QinQ VLAN entries.

Also, I need Andrew and Florian to review and ACK the DSA layer changes
that add the protocol value to the device notifier block.

RE: [EXT] Re: [PATCH v4 2/2] net: dsa: ocelot: Add support for QinQ Operation

[Hongbo Wang]

> > +     if (vlan->proto == ETH_P_8021AD) {
> > +             ocelot->enable_qinq = true;
> > +             ocelot_port->qinq_mode = true;
> > +     }
>  ...
> > +     if (vlan->proto == ETH_P_8021AD) {
> > +             ocelot->enable_qinq = false;
> > +             ocelot_port->qinq_mode = false;
> > +     }
> > +
> 
> I don't understand how this can work just by using a boolean to track the
> state.
> 
> This won't work properly if you are handling multiple QinQ VLAN entries.
> 
> Also, I need Andrew and Florian to review and ACK the DSA layer changes that
> add the protocol value to the device notifier block.

Hi David,
Thanks for reply.

When setting bridge's VLAN protocol to 802.1AD by the command "ip link set br0 type bridge vlan_protocol 802.1ad", it will call dsa_slave_vlan_rx_add(dev, proto, vid) for every port in the bridge, the parameter vid is port's pvid 1,
if pvid's proto is 802.1AD, I will enable switch's enable_qinq, and the related port's qinq_mode,

When there are multiple QinQ VLAN entries, If one VLAN's proto is 802.1AD, I will enable switch and the related port into QinQ mode.

Thanks,
hongbo

Re: [EXT] Re: [PATCH v4 2/2] net: dsa: ocelot: Add support for QinQ Operation

[Florian Fainelli]

On 8/3/2020 11:36 PM, Hongbo Wang wrote:
>>> +     if (vlan->proto == ETH_P_8021AD) {
>>> +             ocelot->enable_qinq = true;
>>> +             ocelot_port->qinq_mode = true;
>>> +     }
>>  ...
>>> +     if (vlan->proto == ETH_P_8021AD) {
>>> +             ocelot->enable_qinq = false;
>>> +             ocelot_port->qinq_mode = false;
>>> +     }
>>> +
>>
>> I don't understand how this can work just by using a boolean to track the
>> state.
>>
>> This won't work properly if you are handling multiple QinQ VLAN entries.
>>
>> Also, I need Andrew and Florian to review and ACK the DSA layer changes that
>> add the protocol value to the device notifier block.
> 
> Hi David,
> Thanks for reply.
> 
> When setting bridge's VLAN protocol to 802.1AD by the command "ip link set br0 type bridge vlan_protocol 802.1ad", it will call dsa_slave_vlan_rx_add(dev, proto, vid) for every port in the bridge, the parameter vid is port's pvid 1,
> if pvid's proto is 802.1AD, I will enable switch's enable_qinq, and the related port's qinq_mode,
> 
> When there are multiple QinQ VLAN entries, If one VLAN's proto is 802.1AD, I will enable switch and the related port into QinQ mode.

The enabling appears fine, the problem is the disabling, the first
802.1AD VLAN entry that gets deleted will lead to the port and switch no
longer being in QinQ mode, and this does not look intended.
-- 
Florian

RE: [EXT] Re: [PATCH v4 2/2] net: dsa: ocelot: Add support for QinQ Operation

[Hongbo Wang]

> Caution: EXT Email
> 
> On 8/3/2020 11:36 PM, Hongbo Wang wrote:
> >>> +     if (vlan->proto == ETH_P_8021AD) {
> >>> +             ocelot->enable_qinq = true;
> >>> +             ocelot_port->qinq_mode = true;
> >>> +     }
> >>  ...
> >>> +     if (vlan->proto == ETH_P_8021AD) {
> >>> +             ocelot->enable_qinq = false;
> >>> +             ocelot_port->qinq_mode = false;
> >>> +     }
> >>> +
> >>
> >> I don't understand how this can work just by using a boolean to track
> >> the state.
> >>
> >> This won't work properly if you are handling multiple QinQ VLAN entries.
> >>
> >> Also, I need Andrew and Florian to review and ACK the DSA layer
> >> changes that add the protocol value to the device notifier block.
> >
> > Hi David,
> > Thanks for reply.
> >
> > When setting bridge's VLAN protocol to 802.1AD by the command "ip link
> > set br0 type bridge vlan_protocol 802.1ad", it will call
> > dsa_slave_vlan_rx_add(dev, proto, vid) for every port in the bridge,
> > the parameter vid is port's pvid 1, if pvid's proto is 802.1AD, I will
> > enable switch's enable_qinq, and the related port's qinq_mode,
> >
> > When there are multiple QinQ VLAN entries, If one VLAN's proto is 802.1AD,
> I will enable switch and the related port into QinQ mode.
> 
> The enabling appears fine, the problem is the disabling, the first 802.1AD VLAN
> entry that gets deleted will lead to the port and switch no longer being in QinQ
> mode, and this does not look intended.
> --
> Florian

Thanks, Florian

I will add reference counter.
When deleting VLAN entry, only if the counter is zero, I will set the switch to exit QinQ mode.

hongbo

RE: [EXT] Re: [PATCH v4 2/2] net: dsa: ocelot: Add support for QinQ Operation

[Hongbo Wang]

> On 8/3/2020 11:36 PM, Hongbo Wang wrote:
> >>> +     if (vlan->proto == ETH_P_8021AD) {
> >>> +             ocelot->enable_qinq = true;
> >>> +             ocelot_port->qinq_mode = true;
> >>> +     }
> >>  ...
> >>> +     if (vlan->proto == ETH_P_8021AD) {
> >>> +             ocelot->enable_qinq = false;
> >>> +             ocelot_port->qinq_mode = false;
> >>> +     }
> >>> +
> >>
> >> I don't understand how this can work just by using a boolean to track
> >> the state.
> >>
> >> This won't work properly if you are handling multiple QinQ VLAN entries.
> >>
> >> Also, I need Andrew and Florian to review and ACK the DSA layer
> >> changes that add the protocol value to the device notifier block.
> >
> > Hi David,
> > Thanks for reply.
> >
> > When setting bridge's VLAN protocol to 802.1AD by the command "ip link
> > set br0 type bridge vlan_protocol 802.1ad", it will call
> > dsa_slave_vlan_rx_add(dev, proto, vid) for every port in the bridge,
> > the parameter vid is port's pvid 1, if pvid's proto is 802.1AD, I will
> > enable switch's enable_qinq, and the related port's qinq_mode,
> >
> > When there are multiple QinQ VLAN entries, If one VLAN's proto is 802.1AD,
> I will enable switch and the related port into QinQ mode.
> 
> The enabling appears fine, the problem is the disabling, the first 802.1AD VLAN
> entry that gets deleted will lead to the port and switch no longer being in QinQ
> mode, and this does not look intended.
> --
> Florian

When I try to add reference counter, I found that:
1.
the command "ip link set br0 type bridge vlan_protocol 802.1ad" call path is:
br_changelink -> __br_vlan_set_proto -> vlan_vid_add -> ... -> ndo_vlan_rx_add_vid -> dsa_slave_vlan_rx_add_vid(dev, proto, vid) -> felix_vlan_add

dsa_slave_vlan_rx_add_vid can pass correct protocol and vid(1) to ocelot driver.

vlan_vid_add is in net/8021q/vlan_core.c, it maintains a vid_list that stores the map of vid and protocol,
the function vlan_vid_info_get can read the map.

but when deleting bridge using "ip link del dev br0 type bridge", the call path is:
br_dev_delete -> ... -> br_switchdev_port_vlan_del -> ... -> dsa_slave_port_obj_del -> dsa_slave_vlan_del -> ... -> felix_vlan_del

br_switchdev_port_vlan_del is in net/bridge/br_switchdev.c, it didn't have the list for map vid and protocol,
so it can't pass correct protocol that corresponding with vid to ocelot driver.

2.
For ocelot QinQ case, the switch port linked to customer has different actions with the port for ISP,

uplink: Customer LAN(CTAG) -> swp0(vlan_aware:0 pop_cnt:0) -> swp1(add STAG) -> ISP MAN(STAG + CTAG)
downlink: ISP MAN(STAG + CTAG) -> swp1(vlan_aware:1 pop_cnt:1, pop STAG) -> swp0(only CTAG) -> Customer LAN

the different action is descripted in "4.3.3 Provider Bridges and Q-in-Q Operation" in VSC99599_1_00_TS.pdf

so I need a standard command to set swp0 and swp1 for different mode, 
but "ip link set br0 type bridge vlan_protocol 802.1ad" will set all ports into the same mode, it's not my intent.

3.
I thought some ways to resovle the above issue:
a. br_switchdev_port_vlan_del will pass default value ETH_P_8021Q, but don't care it in felix_vlan_del.
b. In felix_vlan_add and felix_vlan_del, only when vid is ocelot_port's pvid, it enable or disable switch's enable_qinq.
c. Maybe I can use devlink to set swp0 and swp1 into different mode.
d. let br_switchdev_port_vlan_del call vlan_vid_info_get to get protocol for vid, but vlan_vid_info_get is static in vlan_core.c, so this need to add related functions in br_switchdev.c.

Any comments is welcome!

Thanks
Hongbo

Re: [PATCH v4 2/2] net: dsa: ocelot: Add support for QinQ Operation

[Florian Fainelli]

On 7/30/2020 3:25 AM, hongbo.wang@nxp.com wrote:
> From: "hongbo.wang" <hongbo.wang@nxp.com>
> 
> This featue can be test using network test tools

mispelled: feature, can be used to test network test tools? or can be
used to exercise network test tool?

> TX-tool -----> swp0  -----> swp1 -----> RX-tool
> 
> TX-tool simulates Customer that will send and receive packets with single
> VLAN tag(CTAG), RX-tool simulates Service-Provider that will send and
> receive packets with double VLAN tag(STAG and CTAG). This refers to
> "4.3.3 Provider Bridges and Q-in-Q Operation" in VSC99599_1_00_TS.pdf.
> 
> The related test commands:
> 1.
> ip link add dev br0 type bridge
> ip link set dev swp1 master br0
> ip link set br0 type bridge vlan_protocol 802.1ad
> ip link set dev swp0 master br0
> 
> 2.
> ip link set dev br0 type bridge vlan_filtering 1
> bridge vlan add dev swp0 vid 100 pvid
> bridge vlan add dev swp1 vid 100
> Result:
> Customer(tpid:8100 vid:111) -> swp0 -> swp1 -> ISP(STAG \
>                 tpid:88A8 vid:100, CTAG tpid:8100 vid:111)
> 
> 3.
> bridge vlan del dev swp0 vid 1 pvid
> bridge vlan add dev swp0 vid 100 pvid untagged
> Result:
> ISP(tpid:88A8 vid:100 tpid:8100 vid:222) -> swp1 -> swp0 ->\
> 		Customer(tpid:8100 vid:222)
> 
> Signed-off-by: hongbo.wang <hongbo.wang@nxp.com>
> ---
>  drivers/net/dsa/ocelot/felix.c     | 12 +++++++
>  drivers/net/ethernet/mscc/ocelot.c | 53 +++++++++++++++++++++++++-----
>  include/soc/mscc/ocelot.h          |  2 ++
>  3 files changed, 59 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index c69d9592a2b7..72a27b61080e 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -131,10 +131,16 @@ static void felix_vlan_add(struct dsa_switch *ds, int port,
>  			   const struct switchdev_obj_port_vlan *vlan)
>  {
>  	struct ocelot *ocelot = ds->priv;
> +	struct ocelot_port *ocelot_port = ocelot->ports[port];
>  	u16 flags = vlan->flags;
>  	u16 vid;
>  	int err;
>  
> +	if (vlan->proto == ETH_P_8021AD) {
> +		ocelot->enable_qinq = true;
> +		ocelot_port->qinq_mode = true;
> +	}
> +
>  	if (dsa_is_cpu_port(ds, port))
>  		flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
>  
> @@ -154,9 +160,15 @@ static int felix_vlan_del(struct dsa_switch *ds, int port,
>  			  const struct switchdev_obj_port_vlan *vlan)
>  {
>  	struct ocelot *ocelot = ds->priv;
> +	struct ocelot_port *ocelot_port = ocelot->ports[port];
>  	u16 vid;
>  	int err;
>  
> +	if (vlan->proto == ETH_P_8021AD) {
> +		ocelot->enable_qinq = false;
> +		ocelot_port->qinq_mode = false;
> +	}

You need the delete part to be reference counted, otherwise the first
802.1AD VLAN delete request that comes in, regardless of whether other
802.1AD VLAN entries are installed will disable qinq_mode and
enable_qinq for the entire port and switch, that does not sound like
what you want.

Is not ocelot->enable_qinq the logical or of all ocelo_port instances's
qinq_mode as well?

> +
>  	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
>  		err = ocelot_vlan_del(ocelot, port, vid);
>  		if (err) {
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index f2d94b026d88..b5fec6855afd 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -143,6 +143,8 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
>  				       u16 vid)
>  {
>  	struct ocelot_port *ocelot_port = ocelot->ports[port];
> +	u32 port_tpid = 0;
> +	u32 tag_tpid = 0;
>  	u32 val = 0;
>  
>  	if (ocelot_port->vid != vid) {
> @@ -156,8 +158,14 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
>  		ocelot_port->vid = vid;
>  	}
>  
> -	ocelot_rmw_gix(ocelot, REW_PORT_VLAN_CFG_PORT_VID(vid),
> -		       REW_PORT_VLAN_CFG_PORT_VID_M,
> +	if (ocelot_port->qinq_mode)
> +		port_tpid = REW_PORT_VLAN_CFG_PORT_TPID(ETH_P_8021AD);
> +	else
> +		port_tpid = REW_PORT_VLAN_CFG_PORT_TPID(ETH_P_8021Q);
> +
> +	ocelot_rmw_gix(ocelot, REW_PORT_VLAN_CFG_PORT_VID(vid) | port_tpid,
> +		       REW_PORT_VLAN_CFG_PORT_VID_M |
> +		       REW_PORT_VLAN_CFG_PORT_TPID_M,
>  		       REW_PORT_VLAN_CFG, port);
>  
>  	if (ocelot_port->vlan_aware && !ocelot_port->vid)
> @@ -180,12 +188,28 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
>  		else
>  			/* Tag all frames */
>  			val = REW_TAG_CFG_TAG_CFG(3);
> +
> +		if (ocelot_port->qinq_mode)
> +			tag_tpid = REW_TAG_CFG_TAG_TPID_CFG(1);
> +		else
> +			tag_tpid = REW_TAG_CFG_TAG_TPID_CFG(0);
>  	} else {
> -		/* Port tagging disabled. */
> -		val = REW_TAG_CFG_TAG_CFG(0);
> +		if (ocelot_port->qinq_mode) {
> +			if (ocelot_port->vid)
> +				val = REW_TAG_CFG_TAG_CFG(1);
> +			else
> +				val = REW_TAG_CFG_TAG_CFG(3);
> +> +			tag_tpid = REW_TAG_CFG_TAG_TPID_CFG(1);

This is nearly the same branch as the one above, can you merge the
conditions vlan_aware || qinq_mode and just set an appropriate TAG_CFG()
register value based on either booleans?

Are you also not possibly missing a if (untaged || qinq_mode) check in
ocelot_vlan_add() to call into ocelot_port_set_native_vlan()?
-- 
Florian

RE: [EXT] Re: [PATCH v4 2/2] net: dsa: ocelot: Add support for QinQ Operation

[Hongbo Wang]

> > This featue can be test using network test tools
> 
> mispelled: feature, can be used to test network test tools? or can be used to
> exercise network test tool?

when testing this feature, I need network tool to send packet with VLAN tag(pcp proto and vid), I will change it to avoid ambiguity.

> 
> >       struct ocelot *ocelot = ds->priv;
> > +     struct ocelot_port *ocelot_port = ocelot->ports[port];
> >       u16 vid;
> >       int err;
> >
> > +     if (vlan->proto == ETH_P_8021AD) {
> > +             ocelot->enable_qinq = false;
> > +             ocelot_port->qinq_mode = false;
> > +     }
> 
> You need the delete part to be reference counted, otherwise the first 802.1AD
> VLAN delete request that comes in, regardless of whether other 802.1AD VLAN
> entries are installed will disable qinq_mode and enable_qinq for the entire
> port and switch, that does not sound like what you want.

I will add reference count in next version.
maybe I can disable qinq_mode and enable_qinq only when deleting pvid 1, I will test and change it.

> 
> Is not ocelot->enable_qinq the logical or of all ocelo_port instances's
> qinq_mode as well?

enable_qinq is flag of switch, and qinq_mode is flag of single port,
if switch is in working in QinQ mode, some ports that linked to ISP networking should enable qinq_mode,
other ports that linked to customer networking don't need set qinq_mode. these two types of port have different action.

> >               else
> >                       /* Tag all frames */
> >                       val = REW_TAG_CFG_TAG_CFG(3);
> > +
> > +             if (ocelot_port->qinq_mode)
> > +                     tag_tpid = REW_TAG_CFG_TAG_TPID_CFG(1);
> > +             else
> > +                     tag_tpid = REW_TAG_CFG_TAG_TPID_CFG(0);
> >       } else {
> > -             /* Port tagging disabled. */
> > -             val = REW_TAG_CFG_TAG_CFG(0);
> > +             if (ocelot_port->qinq_mode) {
> > +                     if (ocelot_port->vid)
> > +                             val = REW_TAG_CFG_TAG_CFG(1);
> > +                     else
> > +                             val = REW_TAG_CFG_TAG_CFG(3);
> > +> +                  tag_tpid = REW_TAG_CFG_TAG_TPID_CFG(1);
> 
> This is nearly the same branch as the one above, can you merge the conditions
> vlan_aware || qinq_mode and just set an appropriate TAG_CFG() register value
> based on either booleans?

this feature needs vlan_filter=1, so the branch if (vlan_filter == 0 && qinq_mode) can be deleted now.
I will optimize the related code.

> 
> Are you also not possibly missing a if (untaged || qinq_mode) check in
> ocelot_vlan_add() to call into ocelot_port_set_native_vlan()?

The qinq_mode action can be triggered by ocelot_port_vlan_filtering, so don't need if (untagged || qinq_mode).

Thanks!
Hongbo