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 |
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; >
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; >
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?
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 --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;