diff mbox series

[1/2] net: dsa: Add flag for 802.1AD when adding VLAN for dsa switch and port

Message ID 20200720104119.19146-1-hongbo.wang@nxp.com
State Rejected
Delegated to: David Miller
Headers show
Series [1/2] net: dsa: Add flag for 802.1AD when adding VLAN for dsa switch and port | expand

Commit Message

Hongbo Wang July 20, 2020, 10:41 a.m. UTC 
From: "hongbo.wang" <hongbo.wang@nxp.com>

the following command can be supported:
ip link add link swp1 name swp1.100 type vlan protocol 802.1ad id 100

Signed-off-by: hongbo.wang <hongbo.wang@nxp.com>
---
 include/uapi/linux/if_bridge.h | 1 +
 net/dsa/slave.c                | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

Comments

Nikolay Aleksandrov July 20, 2020, 10:44 a.m. UTC | #1 
On 20/07/2020 13:41, hongbo.wang@nxp.com wrote:
> From: "hongbo.wang" <hongbo.wang@nxp.com>
> 
> the following command can be supported:
> ip link add link swp1 name swp1.100 type vlan protocol 802.1ad id 100
> 
> Signed-off-by: hongbo.wang <hongbo.wang@nxp.com>
> ---
>  include/uapi/linux/if_bridge.h | 1 +
>  net/dsa/slave.c                | 9 +++++++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 

This is not bridge related at all, please leave its flags out of it.

Nacked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>



> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
> index caa6914a3e53..ecd960aa65c7 100644
> --- a/include/uapi/linux/if_bridge.h
> +++ b/include/uapi/linux/if_bridge.h
> @@ -132,6 +132,7 @@ enum {
>  #define BRIDGE_VLAN_INFO_RANGE_END	(1<<4) /* VLAN is end of vlan range */
>  #define BRIDGE_VLAN_INFO_BRENTRY	(1<<5) /* Global bridge VLAN entry */
>  #define BRIDGE_VLAN_INFO_ONLY_OPTS	(1<<6) /* Skip create/delete/flags */
> +#define BRIDGE_VLAN_INFO_8021AD	(1<<7) /* VLAN is 802.1AD protocol */
>  
>  struct bridge_vlan_info {
>  	__u16 flags;
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 4c7f086a047b..376d7ac5f1e5 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1232,6 +1232,7 @@ static int dsa_slave_get_ts_info(struct net_device *dev,
>  static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
>  				     u16 vid)
>  {
> +	u16 flags = 0;
>  	struct dsa_port *dp = dsa_slave_to_port(dev);
>  	struct bridge_vlan_info info;
>  	int ret;
> @@ -1252,7 +1253,10 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
>  			return -EBUSY;
>  	}
>  
> -	ret = dsa_port_vid_add(dp, vid, 0);
> +	if (ntohs(proto) == ETH_P_8021AD)
> +		flags |= BRIDGE_VLAN_INFO_8021AD;
> +
> +	ret = dsa_port_vid_add(dp, vid, flags);
>  	if (ret)
>  		return ret;
>  
> @@ -1744,7 +1748,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;
>
Hongbo Wang July 21, 2020, 3:10 a.m. UTC | #2 
Hi Nikolay,

 Thanks for your comments.

The original intention is that I want to run a command to set single port into QinQ mode,
the related commands are:
ip link set br0 type bridge vlan_protocol 802.1ad  // this command will set all ports under the bridge br0
ip link add link swp1 name swp1.100 type vlan protocol 802.1ad id 100  // this command can set single port for vlan

I trace the related code of these two commands, find the same issue that dsa_slave_vlan_rx_add_vid didn't pass the parameter "proto" to next port level, so I create this patch.

I understand your concern, If don't use the flags for bridge, another way is that add new item "u16 proto" in struct switchdev_obj_port_vlan, the slave port can get proto from that, like that:

struct switchdev_obj_port_vlan {
	struct switchdev_obj obj;
	u16 flags;
	u16 vid_begin;
	u16 vid_end;
   + u16 proto;
};

The related modification has:
int dsa_port_vid_add(struct dsa_port *dp, u16 vid, u16 flags, u16 proto);  // add parameter proto
int dsa_port_vid_del(struct dsa_port *dp, u16 vid, u16 proto);  // add parameter proto

Any comments?
Thanks


-----Original Message-----
From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> 
Sent: 2020年7月20日 18:45
To: Hongbo Wang <hongbo.wang@nxp.com>; Xiaoliang Yang <xiaoliang.yang_1@nxp.com>; allan.nielsen@microchip.com; Po Liu <po.liu@nxp.com>; Claudiu Manoil <claudiu.manoil@nxp.com>; Alexandru Marginean <alexandru.marginean@nxp.com>; Vladimir Oltean <vladimir.oltean@nxp.com>; Leo Li <leoyang.li@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; andrew@lunn.ch; f.fainelli@gmail.com; vivien.didelot@gmail.com; davem@davemloft.net; jiri@resnulli.us; idosch@idosch.org; kuba@kernel.org; vinicius.gomes@intel.com; roopa@cumulusnetworks.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; horatiu.vultur@microchip.com; alexandre.belloni@bootlin.com; UNGLinuxDriver@microchip.com; linux-devel@linux.nxdi.nxp.com
Subject: [EXT] Re: [PATCH 1/2] net: dsa: Add flag for 802.1AD when adding VLAN for dsa switch and port

Caution: EXT Email

On 20/07/2020 13:41, hongbo.wang@nxp.com wrote:
> From: "hongbo.wang" <hongbo.wang@nxp.com>
>
> the following command can be supported:
> ip link add link swp1 name swp1.100 type vlan protocol 802.1ad id 100
>
> Signed-off-by: hongbo.wang <hongbo.wang@nxp.com>
> ---
>  include/uapi/linux/if_bridge.h | 1 +
>  net/dsa/slave.c                | 9 +++++++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
>

This is not bridge related at all, please leave its flags out of it.

Nacked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>



> diff --git a/include/uapi/linux/if_bridge.h 
> b/include/uapi/linux/if_bridge.h index caa6914a3e53..ecd960aa65c7 
> 100644
> --- a/include/uapi/linux/if_bridge.h
> +++ b/include/uapi/linux/if_bridge.h
> @@ -132,6 +132,7 @@ enum {
>  #define BRIDGE_VLAN_INFO_RANGE_END   (1<<4) /* VLAN is end of vlan range */
>  #define BRIDGE_VLAN_INFO_BRENTRY     (1<<5) /* Global bridge VLAN entry */
>  #define BRIDGE_VLAN_INFO_ONLY_OPTS   (1<<6) /* Skip create/delete/flags */
> +#define BRIDGE_VLAN_INFO_8021AD      (1<<7) /* VLAN is 802.1AD protocol */
>
>  struct bridge_vlan_info {
>       __u16 flags;
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 
> 4c7f086a047b..376d7ac5f1e5 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1232,6 +1232,7 @@ static int dsa_slave_get_ts_info(struct 
> net_device *dev,  static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
>                                    u16 vid)  {
> +     u16 flags = 0;
>       struct dsa_port *dp = dsa_slave_to_port(dev);
>       struct bridge_vlan_info info;
>       int ret;
> @@ -1252,7 +1253,10 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
>                       return -EBUSY;
>       }
>
> -     ret = dsa_port_vid_add(dp, vid, 0);
> +     if (ntohs(proto) == ETH_P_8021AD)
> +             flags |= BRIDGE_VLAN_INFO_8021AD;
> +
> +     ret = dsa_port_vid_add(dp, vid, flags);
>       if (ret)
>               return ret;
>
> @@ -1744,7 +1748,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;
>
Florian Fainelli July 21, 2020, 4:57 a.m. UTC | #3 
On 7/20/2020 3:41 AM, hongbo.wang@nxp.com wrote:
> From: "hongbo.wang" <hongbo.wang@nxp.com>
> 
> the following command can be supported:
> ip link add link swp1 name swp1.100 type vlan protocol 802.1ad id 100

You should probably include the switch driver that is going to be
benefiting from doing these changes in the patch series, provide a cover
letter when sending more than one patch, and also combine both the add
and delete parts.

Since you already have visibility into proto, it may not be necessary at
all for now to define a BRIDGE_VLAN_INFO_8021AD bit in order to pass
that flag down to DSA for programming the VLAN, just pass proto to
dsa_port_vid_add() or a boolean flag which indicates whether this is the
customer or service tag that you are trying to program?
Hongbo Wang July 21, 2020, 6:01 a.m. UTC | #4 
Thanks for your suggestion!
I will change the code.
thanks.

-----Original Message-----
From: Florian Fainelli <f.fainelli@gmail.com> 
Sent: 2020年7月21日 12:57
To: Hongbo Wang <hongbo.wang@nxp.com>; Xiaoliang Yang <xiaoliang.yang_1@nxp.com>; allan.nielsen@microchip.com; Po Liu <po.liu@nxp.com>; Claudiu Manoil <claudiu.manoil@nxp.com>; Alexandru Marginean <alexandru.marginean@nxp.com>; Vladimir Oltean <vladimir.oltean@nxp.com>; Leo Li <leoyang.li@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; andrew@lunn.ch; vivien.didelot@gmail.com; davem@davemloft.net; jiri@resnulli.us; idosch@idosch.org; kuba@kernel.org; vinicius.gomes@intel.com; nikolay@cumulusnetworks.com; roopa@cumulusnetworks.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; horatiu.vultur@microchip.com; alexandre.belloni@bootlin.com; UNGLinuxDriver@microchip.com; linux-devel@linux.nxdi.nxp.com
Subject: [EXT] Re: [PATCH 1/2] net: dsa: Add flag for 802.1AD when adding VLAN for dsa switch and port

Caution: EXT Email

On 7/20/2020 3:41 AM, hongbo.wang@nxp.com wrote:
> From: "hongbo.wang" <hongbo.wang@nxp.com>
>
> the following command can be supported:
> ip link add link swp1 name swp1.100 type vlan protocol 802.1ad id 100

You should probably include the switch driver that is going to be benefiting from doing these changes in the patch series, provide a cover letter when sending more than one patch, and also combine both the add and delete parts.

Since you already have visibility into proto, it may not be necessary at all for now to define a BRIDGE_VLAN_INFO_8021AD bit in order to pass that flag down to DSA for programming the VLAN, just pass proto to
dsa_port_vid_add() or a boolean flag which indicates whether this is the customer or service tag that you are trying to program?
--
Florian
diff mbox series

Patch

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index caa6914a3e53..ecd960aa65c7 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -132,6 +132,7 @@  enum {
 #define BRIDGE_VLAN_INFO_RANGE_END	(1<<4) /* VLAN is end of vlan range */
 #define BRIDGE_VLAN_INFO_BRENTRY	(1<<5) /* Global bridge VLAN entry */
 #define BRIDGE_VLAN_INFO_ONLY_OPTS	(1<<6) /* Skip create/delete/flags */
+#define BRIDGE_VLAN_INFO_8021AD	(1<<7) /* VLAN is 802.1AD protocol */
 
 struct bridge_vlan_info {
 	__u16 flags;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 4c7f086a047b..376d7ac5f1e5 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1232,6 +1232,7 @@  static int dsa_slave_get_ts_info(struct net_device *dev,
 static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
 				     u16 vid)
 {
+	u16 flags = 0;
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	struct bridge_vlan_info info;
 	int ret;
@@ -1252,7 +1253,10 @@  static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
 			return -EBUSY;
 	}
 
-	ret = dsa_port_vid_add(dp, vid, 0);
+	if (ntohs(proto) == ETH_P_8021AD)
+		flags |= BRIDGE_VLAN_INFO_8021AD;
+
+	ret = dsa_port_vid_add(dp, vid, flags);
 	if (ret)
 		return ret;
 
@@ -1744,7 +1748,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;