[PATCH net-next 0/7] net: bridge: Notify about bridge VLANs

[PATCH net-next 0/7] net: bridge: Notify about bridge VLANs

[Petr Machata]

In commit 946a11e7408e ("mlxsw: spectrum_span: Allow bridge for gretap
mirror"), mlxsw got support for offloading mirror-to-gretap such that
the underlay packet path involves a bridge. In that case, the offload is
also influenced by PVID setting of said bridge. However, changes to VLAN
configuration of the bridge itself do not generate switchdev
notifications, so there's no mechanism to prod mlxsw to update the
offload when these settings change.

In this patchset, the problem is resolved by distributing the switchdev
notification SWITCHDEV_OBJ_ID_PORT_VLAN also for configuration changes
on bridge VLANs. Since stacked devices distribute the notification to
lower devices, such event eventually reaches the driver, which can
determine whether it's a bridge or port VLAN by inspecting orig_dev.

To keep things consistent, the newly-distributed notifications observe
the same protocol as the existing ones: dual prepare/commit, with
-EOPNOTSUPP indicating lack of support, even though there's currently
nothing to prepare for and nothing to support. Correspondingly, all
switchdev drivers have been updated to return -EOPNOTSUPP for bridge
VLAN notifications.

In patch #1, the code to send notifications for adding and deleting is
factored out into two named functions.

In patches #2-#5, respectively for mlxsw, rocker, DSA and DPAA2 ethsw,
the new notifications (which are not enabled yet) are ignored to
maintain the current behavior.

In patch #6, the notification is actually enabled.

In patch #7, mlxsw is changed to update offloads of mirror-to-gre also
for bridge-related notifications.

Petr Machata (7):
  net: bridge: Extract boilerplate around switchdev_port_obj_*()
  mlxsw: spectrum_switchdev: Ignore bridge VLAN events
  rocker: rocker_main: Ignore bridge VLAN events
  dsa: port: Ignore bridge VLAN events
  staging: fsl-dpaa2: ethsw: Ignore bridge VLAN events
  net: bridge: Notify about bridge VLANs
  mlxsw: spectrum_switchdev: Schedule respin during trans prepare

 .../ethernet/mellanox/mlxsw/spectrum_switchdev.c   |  8 ++-
 drivers/net/ethernet/rocker/rocker_main.c          |  6 +++
 drivers/staging/fsl-dpaa2/ethsw/ethsw.c            |  6 +++
 net/bridge/br_vlan.c                               | 58 ++++++++++++++--------
 net/dsa/port.c                                     |  6 +++
 5 files changed, 62 insertions(+), 22 deletions(-)

-- 
2.4.11

[Bridge] [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs

[Petr Machata]

In commit 946a11e7408e ("mlxsw: spectrum_span: Allow bridge for gretap
mirror"), mlxsw got support for offloading mirror-to-gretap such that
the underlay packet path involves a bridge. In that case, the offload is
also influenced by PVID setting of said bridge. However, changes to VLAN
configuration of the bridge itself do not generate switchdev
notifications, so there's no mechanism to prod mlxsw to update the
offload when these settings change.

In this patchset, the problem is resolved by distributing the switchdev
notification SWITCHDEV_OBJ_ID_PORT_VLAN also for configuration changes
on bridge VLANs. Since stacked devices distribute the notification to
lower devices, such event eventually reaches the driver, which can
determine whether it's a bridge or port VLAN by inspecting orig_dev.

To keep things consistent, the newly-distributed notifications observe
the same protocol as the existing ones: dual prepare/commit, with
-EOPNOTSUPP indicating lack of support, even though there's currently
nothing to prepare for and nothing to support. Correspondingly, all
switchdev drivers have been updated to return -EOPNOTSUPP for bridge
VLAN notifications.

In patch #1, the code to send notifications for adding and deleting is
factored out into two named functions.

In patches #2-#5, respectively for mlxsw, rocker, DSA and DPAA2 ethsw,
the new notifications (which are not enabled yet) are ignored to
maintain the current behavior.

In patch #6, the notification is actually enabled.

In patch #7, mlxsw is changed to update offloads of mirror-to-gre also
for bridge-related notifications.

Petr Machata (7):
  net: bridge: Extract boilerplate around switchdev_port_obj_*()
  mlxsw: spectrum_switchdev: Ignore bridge VLAN events
  rocker: rocker_main: Ignore bridge VLAN events
  dsa: port: Ignore bridge VLAN events
  staging: fsl-dpaa2: ethsw: Ignore bridge VLAN events
  net: bridge: Notify about bridge VLANs
  mlxsw: spectrum_switchdev: Schedule respin during trans prepare

 .../ethernet/mellanox/mlxsw/spectrum_switchdev.c   |  8 ++-
 drivers/net/ethernet/rocker/rocker_main.c          |  6 +++
 drivers/staging/fsl-dpaa2/ethsw/ethsw.c            |  6 +++
 net/bridge/br_vlan.c                               | 58 ++++++++++++++--------
 net/dsa/port.c                                     |  6 +++
 5 files changed, 62 insertions(+), 22 deletions(-)

-- 
2.4.11

[PATCH net-next 1/7] net: bridge: Extract boilerplate around switchdev_port_obj_*()

[Petr Machata]

A call to switchdev_port_obj_add() or switchdev_port_obj_del() involves
initializing a struct switchdev_obj_port_vlan, a piece of code that
repeats on each call site almost verbatim. While in the current codebase
there is just one duplicated add call, the follow-up patches add more of
both add and del calls.

Thus to remove the duplication, extract the repetition into named
functions and reuse.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 net/bridge/br_vlan.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index dc832c09..a75fe930 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -79,8 +79,7 @@ static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
 	return ret || !!(old_flags ^ v->flags);
 }
 
-static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
-			  u16 vid, u16 flags)
+static int br_switchdev_port_obj_add(struct net_device *dev, u16 vid, u16 flags)
 {
 	struct switchdev_obj_port_vlan v = {
 		.obj.orig_dev = dev,
@@ -89,12 +88,29 @@ static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
 		.vid_begin = vid,
 		.vid_end = vid,
 	};
-	int err;
 
+	return switchdev_port_obj_add(dev, &v.obj);
+}
+
+static int br_switchdev_port_obj_del(struct net_device *dev, u16 vid)
+{
+	struct switchdev_obj_port_vlan v = {
+		.obj.orig_dev = dev,
+		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
+		.vid_begin = vid,
+		.vid_end = vid,
+	};
+
+	return switchdev_port_obj_del(dev, &v.obj);
+}
+
+static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
+			  u16 vid, u16 flags)
+{
 	/* Try switchdev op first. In case it is not supported, fallback to
 	 * 8021q add.
 	 */
-	err = switchdev_port_obj_add(dev, &v.obj);
+	int err = br_switchdev_port_obj_add(dev, vid, flags);
 	if (err == -EOPNOTSUPP)
 		return vlan_vid_add(dev, br->vlan_proto, vid);
 	return err;
@@ -130,18 +146,11 @@ static void __vlan_del_list(struct net_bridge_vlan *v)
 static int __vlan_vid_del(struct net_device *dev, struct net_bridge *br,
 			  u16 vid)
 {
-	struct switchdev_obj_port_vlan v = {
-		.obj.orig_dev = dev,
-		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
-		.vid_begin = vid,
-		.vid_end = vid,
-	};
-	int err;
-
 	/* Try switchdev op first. In case it is not supported, fallback to
 	 * 8021q del.
 	 */
-	err = switchdev_port_obj_del(dev, &v.obj);
+	int err = br_switchdev_port_obj_del(dev, vid);
+
 	if (err == -EOPNOTSUPP) {
 		vlan_vid_del(dev, br->vlan_proto, vid);
 		return 0;
@@ -1053,13 +1062,6 @@ int nbp_vlan_init(struct net_bridge_port *p)
 int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
 		 bool *changed)
 {
-	struct switchdev_obj_port_vlan v = {
-		.obj.orig_dev = port->dev,
-		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
-		.flags = flags,
-		.vid_begin = vid,
-		.vid_end = vid,
-	};
 	struct net_bridge_vlan *vlan;
 	int ret;
 
@@ -1069,7 +1071,7 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
 	vlan = br_vlan_find(nbp_vlan_group(port), vid);
 	if (vlan) {
 		/* Pass the flags to the hardware bridge */
-		ret = switchdev_port_obj_add(port->dev, &v.obj);
+		ret = br_switchdev_port_obj_add(port->dev, vid, flags);
 		if (ret && ret != -EOPNOTSUPP)
 			return ret;
 		*changed = __vlan_add_flags(vlan, flags);
-- 
2.4.11

[Bridge] [PATCH net-next 1/7] net: bridge: Extract boilerplate around switchdev_port_obj_*()

[Petr Machata]

A call to switchdev_port_obj_add() or switchdev_port_obj_del() involves
initializing a struct switchdev_obj_port_vlan, a piece of code that
repeats on each call site almost verbatim. While in the current codebase
there is just one duplicated add call, the follow-up patches add more of
both add and del calls.

Thus to remove the duplication, extract the repetition into named
functions and reuse.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 net/bridge/br_vlan.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index dc832c09..a75fe930 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -79,8 +79,7 @@ static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
 	return ret || !!(old_flags ^ v->flags);
 }
 
-static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
-			  u16 vid, u16 flags)
+static int br_switchdev_port_obj_add(struct net_device *dev, u16 vid, u16 flags)
 {
 	struct switchdev_obj_port_vlan v = {
 		.obj.orig_dev = dev,
@@ -89,12 +88,29 @@ static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
 		.vid_begin = vid,
 		.vid_end = vid,
 	};
-	int err;
 
+	return switchdev_port_obj_add(dev, &v.obj);
+}
+
+static int br_switchdev_port_obj_del(struct net_device *dev, u16 vid)
+{
+	struct switchdev_obj_port_vlan v = {
+		.obj.orig_dev = dev,
+		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
+		.vid_begin = vid,
+		.vid_end = vid,
+	};
+
+	return switchdev_port_obj_del(dev, &v.obj);
+}
+
+static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
+			  u16 vid, u16 flags)
+{
 	/* Try switchdev op first. In case it is not supported, fallback to
 	 * 8021q add.
 	 */
-	err = switchdev_port_obj_add(dev, &v.obj);
+	int err = br_switchdev_port_obj_add(dev, vid, flags);
 	if (err == -EOPNOTSUPP)
 		return vlan_vid_add(dev, br->vlan_proto, vid);
 	return err;
@@ -130,18 +146,11 @@ static void __vlan_del_list(struct net_bridge_vlan *v)
 static int __vlan_vid_del(struct net_device *dev, struct net_bridge *br,
 			  u16 vid)
 {
-	struct switchdev_obj_port_vlan v = {
-		.obj.orig_dev = dev,
-		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
-		.vid_begin = vid,
-		.vid_end = vid,
-	};
-	int err;
-
 	/* Try switchdev op first. In case it is not supported, fallback to
 	 * 8021q del.
 	 */
-	err = switchdev_port_obj_del(dev, &v.obj);
+	int err = br_switchdev_port_obj_del(dev, vid);
+
 	if (err == -EOPNOTSUPP) {
 		vlan_vid_del(dev, br->vlan_proto, vid);
 		return 0;
@@ -1053,13 +1062,6 @@ int nbp_vlan_init(struct net_bridge_port *p)
 int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
 		 bool *changed)
 {
-	struct switchdev_obj_port_vlan v = {
-		.obj.orig_dev = port->dev,
-		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
-		.flags = flags,
-		.vid_begin = vid,
-		.vid_end = vid,
-	};
 	struct net_bridge_vlan *vlan;
 	int ret;
 
@@ -1069,7 +1071,7 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
 	vlan = br_vlan_find(nbp_vlan_group(port), vid);
 	if (vlan) {
 		/* Pass the flags to the hardware bridge */
-		ret = switchdev_port_obj_add(port->dev, &v.obj);
+		ret = br_switchdev_port_obj_add(port->dev, vid, flags);
 		if (ret && ret != -EOPNOTSUPP)
 			return ret;
 		*changed = __vlan_add_flags(vlan, flags);
-- 
2.4.11

[PATCH net-next 2/7] mlxsw: spectrum_switchdev: Ignore bridge VLAN events

[Petr Machata]

Ignore VLAN events where the orig_dev is the bridge device itself.

Signed-off-by: Petr Machata <petrm@mellanox.com>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
index 8c9cf8e..cbc8fab 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
@@ -1144,6 +1144,9 @@ static int mlxsw_sp_port_vlans_add(struct mlxsw_sp_port *mlxsw_sp_port,
 	struct mlxsw_sp_bridge_port *bridge_port;
 	u16 vid;
 
+	if (netif_is_bridge_master(orig_dev))
+		return -EOPNOTSUPP;
+
 	if (switchdev_trans_ph_prepare(trans))
 		return 0;
 
@@ -1741,6 +1744,9 @@ static int mlxsw_sp_port_vlans_del(struct mlxsw_sp_port *mlxsw_sp_port,
 	struct mlxsw_sp_bridge_port *bridge_port;
 	u16 vid;
 
+	if (netif_is_bridge_master(orig_dev))
+		return -EOPNOTSUPP;
+
 	bridge_port = mlxsw_sp_bridge_port_find(mlxsw_sp->bridge, orig_dev);
 	if (WARN_ON(!bridge_port))
 		return -EINVAL;
-- 
2.4.11

[Bridge] [PATCH net-next 2/7] mlxsw: spectrum_switchdev: Ignore bridge VLAN events

[Petr Machata]

Ignore VLAN events where the orig_dev is the bridge device itself.

Signed-off-by: Petr Machata <petrm@mellanox.com>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
index 8c9cf8e..cbc8fab 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
@@ -1144,6 +1144,9 @@ static int mlxsw_sp_port_vlans_add(struct mlxsw_sp_port *mlxsw_sp_port,
 	struct mlxsw_sp_bridge_port *bridge_port;
 	u16 vid;
 
+	if (netif_is_bridge_master(orig_dev))
+		return -EOPNOTSUPP;
+
 	if (switchdev_trans_ph_prepare(trans))
 		return 0;
 
@@ -1741,6 +1744,9 @@ static int mlxsw_sp_port_vlans_del(struct mlxsw_sp_port *mlxsw_sp_port,
 	struct mlxsw_sp_bridge_port *bridge_port;
 	u16 vid;
 
+	if (netif_is_bridge_master(orig_dev))
+		return -EOPNOTSUPP;
+
 	bridge_port = mlxsw_sp_bridge_port_find(mlxsw_sp->bridge, orig_dev);
 	if (WARN_ON(!bridge_port))
 		return -EINVAL;
-- 
2.4.11

[PATCH net-next 3/7] rocker: rocker_main: Ignore bridge VLAN events

[Petr Machata]

Ignore VLAN events where the orig_dev is the bridge device itself.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 drivers/net/ethernet/rocker/rocker_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/rocker/rocker_main.c b/drivers/net/ethernet/rocker/rocker_main.c
index e73e4fe..aeafdb9 100644
--- a/drivers/net/ethernet/rocker/rocker_main.c
+++ b/drivers/net/ethernet/rocker/rocker_main.c
@@ -1632,6 +1632,9 @@ rocker_world_port_obj_vlan_add(struct rocker_port *rocker_port,
 {
 	struct rocker_world_ops *wops = rocker_port->rocker->wops;
 
+	if (netif_is_bridge_master(vlan->obj.orig_dev))
+		return -EOPNOTSUPP;
+
 	if (!wops->port_obj_vlan_add)
 		return -EOPNOTSUPP;
 
@@ -1647,6 +1650,9 @@ rocker_world_port_obj_vlan_del(struct rocker_port *rocker_port,
 {
 	struct rocker_world_ops *wops = rocker_port->rocker->wops;
 
+	if (netif_is_bridge_master(vlan->obj.orig_dev))
+		return -EOPNOTSUPP;
+
 	if (!wops->port_obj_vlan_del)
 		return -EOPNOTSUPP;
 	return wops->port_obj_vlan_del(rocker_port, vlan);
-- 
2.4.11

[Bridge] [PATCH net-next 3/7] rocker: rocker_main: Ignore bridge VLAN events

[Petr Machata]

Ignore VLAN events where the orig_dev is the bridge device itself.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 drivers/net/ethernet/rocker/rocker_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/rocker/rocker_main.c b/drivers/net/ethernet/rocker/rocker_main.c
index e73e4fe..aeafdb9 100644
--- a/drivers/net/ethernet/rocker/rocker_main.c
+++ b/drivers/net/ethernet/rocker/rocker_main.c
@@ -1632,6 +1632,9 @@ rocker_world_port_obj_vlan_add(struct rocker_port *rocker_port,
 {
 	struct rocker_world_ops *wops = rocker_port->rocker->wops;
 
+	if (netif_is_bridge_master(vlan->obj.orig_dev))
+		return -EOPNOTSUPP;
+
 	if (!wops->port_obj_vlan_add)
 		return -EOPNOTSUPP;
 
@@ -1647,6 +1650,9 @@ rocker_world_port_obj_vlan_del(struct rocker_port *rocker_port,
 {
 	struct rocker_world_ops *wops = rocker_port->rocker->wops;
 
+	if (netif_is_bridge_master(vlan->obj.orig_dev))
+		return -EOPNOTSUPP;
+
 	if (!wops->port_obj_vlan_del)
 		return -EOPNOTSUPP;
 	return wops->port_obj_vlan_del(rocker_port, vlan);
-- 
2.4.11

[PATCH net-next 4/7] dsa: port: Ignore bridge VLAN events

[Petr Machata]

Ignore VLAN events where the orig_dev is the bridge device itself.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 net/dsa/port.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index 2413beb..ed05954 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -252,6 +252,9 @@ int dsa_port_vlan_add(struct dsa_port *dp,
 		.vlan = vlan,
 	};
 
+	if (netif_is_bridge_master(vlan->obj.orig_dev))
+		return -EOPNOTSUPP;
+
 	if (br_vlan_enabled(dp->bridge_dev))
 		return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_ADD, &info);
 
@@ -267,6 +270,9 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 		.vlan = vlan,
 	};
 
+	if (netif_is_bridge_master(vlan->obj.orig_dev))
+		return -EOPNOTSUPP;
+
 	if (br_vlan_enabled(dp->bridge_dev))
 		return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
 
-- 
2.4.11

[Bridge] [PATCH net-next 4/7] dsa: port: Ignore bridge VLAN events

[Petr Machata]

Ignore VLAN events where the orig_dev is the bridge device itself.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 net/dsa/port.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index 2413beb..ed05954 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -252,6 +252,9 @@ int dsa_port_vlan_add(struct dsa_port *dp,
 		.vlan = vlan,
 	};
 
+	if (netif_is_bridge_master(vlan->obj.orig_dev))
+		return -EOPNOTSUPP;
+
 	if (br_vlan_enabled(dp->bridge_dev))
 		return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_ADD, &info);
 
@@ -267,6 +270,9 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 		.vlan = vlan,
 	};
 
+	if (netif_is_bridge_master(vlan->obj.orig_dev))
+		return -EOPNOTSUPP;
+
 	if (br_vlan_enabled(dp->bridge_dev))
 		return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
 
-- 
2.4.11

[PATCH net-next 5/7] staging: fsl-dpaa2: ethsw: Ignore bridge VLAN events

[Petr Machata]

Ignore VLAN events where the orig_dev is the bridge device itself.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
index c723a04..a17dd29 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
@@ -719,6 +719,9 @@ static int port_vlans_add(struct net_device *netdev,
 	struct ethsw_port_priv *port_priv = netdev_priv(netdev);
 	int vid, err;
 
+	if (netif_is_bridge_master(vlan->obj.orig_dev))
+		return -EOPNOTSUPP;
+
 	if (switchdev_trans_ph_prepare(trans))
 		return 0;
 
@@ -873,6 +876,9 @@ static int port_vlans_del(struct net_device *netdev,
 	struct ethsw_port_priv *port_priv = netdev_priv(netdev);
 	int vid, err;
 
+	if (netif_is_bridge_master(vlan->obj.orig_dev))
+		return -EOPNOTSUPP;
+
 	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
 		err = ethsw_port_del_vlan(port_priv, vid);
 		if (err)
-- 
2.4.11

[Bridge] [PATCH net-next 5/7] staging: fsl-dpaa2: ethsw: Ignore bridge VLAN events

[Petr Machata]

Ignore VLAN events where the orig_dev is the bridge device itself.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
index c723a04..a17dd29 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
@@ -719,6 +719,9 @@ static int port_vlans_add(struct net_device *netdev,
 	struct ethsw_port_priv *port_priv = netdev_priv(netdev);
 	int vid, err;
 
+	if (netif_is_bridge_master(vlan->obj.orig_dev))
+		return -EOPNOTSUPP;
+
 	if (switchdev_trans_ph_prepare(trans))
 		return 0;
 
@@ -873,6 +876,9 @@ static int port_vlans_del(struct net_device *netdev,
 	struct ethsw_port_priv *port_priv = netdev_priv(netdev);
 	int vid, err;
 
+	if (netif_is_bridge_master(vlan->obj.orig_dev))
+		return -EOPNOTSUPP;
+
 	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
 		err = ethsw_port_del_vlan(port_priv, vid);
 		if (err)
-- 
2.4.11

[PATCH net-next 6/7] net: bridge: Notify about bridge VLANs

[Petr Machata]

A driver might need to react to changes in settings of brentry VLANs.
Therefore send switchdev port notifications for these as well. Reuse
SWITCHDEV_OBJ_ID_PORT_VLAN for this purpose. Listeners should use
netif_is_bridge_master() on orig_dev to determine whether the
notification is about a bridge port or a bridge.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 net/bridge/br_vlan.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index a75fe930..14c1b6c 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -268,6 +268,10 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags)
 			goto out_filt;
 		v->brvlan = masterv;
 		v->stats = masterv->stats;
+	} else {
+		err = br_switchdev_port_obj_add(dev, v->vid, flags);
+		if (err && err != -EOPNOTSUPP)
+			goto out;
 	}
 
 	/* Add the dev mac and count the vlan only if it's usable */
@@ -303,6 +307,8 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags)
 			br_vlan_put_master(masterv);
 			v->brvlan = NULL;
 		}
+	} else {
+		br_switchdev_port_obj_del(dev, v->vid);
 	}
 
 	goto out;
@@ -328,6 +334,11 @@ static int __vlan_del(struct net_bridge_vlan *v)
 		err = __vlan_vid_del(p->dev, p->br, v->vid);
 		if (err)
 			goto out;
+	} else {
+		err = br_switchdev_port_obj_del(v->br->dev, v->vid);
+		if (err && err != -EOPNOTSUPP)
+			goto out;
+		err = 0;
 	}
 
 	if (br_vlan_should_use(v)) {
@@ -605,6 +616,9 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags, bool *changed)
 			vg->num_vlans++;
 			*changed = true;
 		}
+		ret = br_switchdev_port_obj_add(br->dev, vid, flags);
+		if (ret && ret != -EOPNOTSUPP)
+			return ret;
 		if (__vlan_add_flags(vlan, flags))
 			*changed = true;
 
-- 
2.4.11

[Bridge] [PATCH net-next 6/7] net: bridge: Notify about bridge VLANs

[Petr Machata]

A driver might need to react to changes in settings of brentry VLANs.
Therefore send switchdev port notifications for these as well. Reuse
SWITCHDEV_OBJ_ID_PORT_VLAN for this purpose. Listeners should use
netif_is_bridge_master() on orig_dev to determine whether the
notification is about a bridge port or a bridge.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 net/bridge/br_vlan.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index a75fe930..14c1b6c 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -268,6 +268,10 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags)
 			goto out_filt;
 		v->brvlan = masterv;
 		v->stats = masterv->stats;
+	} else {
+		err = br_switchdev_port_obj_add(dev, v->vid, flags);
+		if (err && err != -EOPNOTSUPP)
+			goto out;
 	}
 
 	/* Add the dev mac and count the vlan only if it's usable */
@@ -303,6 +307,8 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags)
 			br_vlan_put_master(masterv);
 			v->brvlan = NULL;
 		}
+	} else {
+		br_switchdev_port_obj_del(dev, v->vid);
 	}
 
 	goto out;
@@ -328,6 +334,11 @@ static int __vlan_del(struct net_bridge_vlan *v)
 		err = __vlan_vid_del(p->dev, p->br, v->vid);
 		if (err)
 			goto out;
+	} else {
+		err = br_switchdev_port_obj_del(v->br->dev, v->vid);
+		if (err && err != -EOPNOTSUPP)
+			goto out;
+		err = 0;
 	}
 
 	if (br_vlan_should_use(v)) {
@@ -605,6 +616,9 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags, bool *changed)
 			vg->num_vlans++;
 			*changed = true;
 		}
+		ret = br_switchdev_port_obj_add(br->dev, vid, flags);
+		if (ret && ret != -EOPNOTSUPP)
+			return ret;
 		if (__vlan_add_flags(vlan, flags))
 			*changed = true;
 
-- 
2.4.11

[PATCH net-next 7/7] mlxsw: spectrum_switchdev: Schedule respin during trans prepare

[Petr Machata]

Since there's no special support for the bridge events, the driver
returns -EOPNOTSUPP, and thus the commit never happens. Therefore
schedule respin during the prepare stage: there's no real difference one
way or another.

This fixes the problem that mirror-to-gretap offload wouldn't adapt to
changes in bridge vlan configuration right away and another notification
would have to arrive for mlxsw to catch up.

Signed-off-by: Petr Machata <petrm@mellanox.com>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
index cbc8fab..8a15ac4 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
@@ -1697,7 +1697,7 @@ static int mlxsw_sp_port_obj_add(struct net_device *dev,
 		vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
 		err = mlxsw_sp_port_vlans_add(mlxsw_sp_port, vlan, trans);
 
-		if (switchdev_trans_ph_commit(trans)) {
+		if (switchdev_trans_ph_prepare(trans)) {
 			/* The event is emitted before the changes are actually
 			 * applied to the bridge. Therefore schedule the respin
 			 * call for later, so that the respin logic sees the
-- 
2.4.11

[Bridge] [PATCH net-next 7/7] mlxsw: spectrum_switchdev: Schedule respin during trans prepare

[Petr Machata]

Since there's no special support for the bridge events, the driver
returns -EOPNOTSUPP, and thus the commit never happens. Therefore
schedule respin during the prepare stage: there's no real difference one
way or another.

This fixes the problem that mirror-to-gretap offload wouldn't adapt to
changes in bridge vlan configuration right away and another notification
would have to arrive for mlxsw to catch up.

Signed-off-by: Petr Machata <petrm@mellanox.com>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
index cbc8fab..8a15ac4 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
@@ -1697,7 +1697,7 @@ static int mlxsw_sp_port_obj_add(struct net_device *dev,
 		vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
 		err = mlxsw_sp_port_vlans_add(mlxsw_sp_port, vlan, trans);
 
-		if (switchdev_trans_ph_commit(trans)) {
+		if (switchdev_trans_ph_prepare(trans)) {
 			/* The event is emitted before the changes are actually
 			 * applied to the bridge. Therefore schedule the respin
 			 * call for later, so that the respin logic sees the
-- 
2.4.11

Re: [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs

[Florian Fainelli]

Hi Petr,

On 05/24/2018 08:09 AM, Petr Machata wrote:
> In commit 946a11e7408e ("mlxsw: spectrum_span: Allow bridge for gretap
> mirror"), mlxsw got support for offloading mirror-to-gretap such that
> the underlay packet path involves a bridge. In that case, the offload is
> also influenced by PVID setting of said bridge. However, changes to VLAN
> configuration of the bridge itself do not generate switchdev
> notifications, so there's no mechanism to prod mlxsw to update the
> offload when these settings change.
> 
> In this patchset, the problem is resolved by distributing the switchdev
> notification SWITCHDEV_OBJ_ID_PORT_VLAN also for configuration changes
> on bridge VLANs. Since stacked devices distribute the notification to
> lower devices, such event eventually reaches the driver, which can
> determine whether it's a bridge or port VLAN by inspecting orig_dev.
> 
> To keep things consistent, the newly-distributed notifications observe
> the same protocol as the existing ones: dual prepare/commit, with
> -EOPNOTSUPP indicating lack of support, even though there's currently
> nothing to prepare for and nothing to support. Correspondingly, all
> switchdev drivers have been updated to return -EOPNOTSUPP for bridge
> VLAN notifications.

You seem to have approached the bridge changes a little differently from
this series:

https://lists.linux-foundation.org/pipermail/bridge/2016-November/010112.html

Both have the same intent that by targeting the bridge device itself,
you can propagate that through switchdev to the switch drivers, and in
turn create configurations where for instance, you have:

- CPU/management port present in specific VLANs that is a subset or
superset of the VLANs configured on front-panel ports
- CPU/management port tagged/untagged in specific VLANs which can be a
different setting from the front-panel ports

One problem we have in DSA at the moment is that we always add the CPU
port to the VLANs configured to the front-panel port but we do this with
the same attributes as the front panel ports! For instance, if you add
Port 0 to VLAN1 untagged, the the CPU port also gets added to that
VLAN1, also untagged. As long as there is just one VLAN untagged, this
is not much of a problem. Now do this with another VLAN or another port,
and the CPU can no longer differentiate the traffic from which VLAN it
is coming from, no bueno.

I had specifically changed b53 to always add the CPU port as tagged,
because that would always allow for differentiating traffic, but I would
rather have the capability to configure that at the bridge layer, which
you series seem to allow.

For the record, here is what the first commit in the series intended to
let an user do:

The following happens now (assuming bridge master device is already
created):

bridge vlan add vid 2 dev port0 pvid untagged
	-> port0 (e.g: switch port 0) gets programmed
	-> CPU port gets programmed
bridge vlan add vid 2 dev br0 self
	-> CPU port gets programmed
bridge vlan add vid 2 dev port0
	-> port0 (switch port 0) gets programmed

Are these use cases possible with your series? It seems to me like it is
if we drop the netif_is_bridge_master() checks and resolve orig_dev as
being a hint for the CPU/management port.

Thanks for reading me :)

> 
> In patch #1, the code to send notifications for adding and deleting is
> factored out into two named functions.
> 
> In patches #2-#5, respectively for mlxsw, rocker, DSA and DPAA2 ethsw,
> the new notifications (which are not enabled yet) are ignored to
> maintain the current behavior.
> 
> In patch #6, the notification is actually enabled.
> 
> In patch #7, mlxsw is changed to update offloads of mirror-to-gre also
> for bridge-related notifications.
> 
> Petr Machata (7):
>   net: bridge: Extract boilerplate around switchdev_port_obj_*()
>   mlxsw: spectrum_switchdev: Ignore bridge VLAN events
>   rocker: rocker_main: Ignore bridge VLAN events
>   dsa: port: Ignore bridge VLAN events
>   staging: fsl-dpaa2: ethsw: Ignore bridge VLAN events
>   net: bridge: Notify about bridge VLANs
>   mlxsw: spectrum_switchdev: Schedule respin during trans prepare
> 
>  .../ethernet/mellanox/mlxsw/spectrum_switchdev.c   |  8 ++-
>  drivers/net/ethernet/rocker/rocker_main.c          |  6 +++
>  drivers/staging/fsl-dpaa2/ethsw/ethsw.c            |  6 +++
>  net/bridge/br_vlan.c                               | 58 ++++++++++++++--------
>  net/dsa/port.c                                     |  6 +++
>  5 files changed, 62 insertions(+), 22 deletions(-)
> 


-- 
Florian

Re: [Bridge] [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs

[Florian Fainelli]

Hi Petr,

On 05/24/2018 08:09 AM, Petr Machata wrote:
> In commit 946a11e7408e ("mlxsw: spectrum_span: Allow bridge for gretap
> mirror"), mlxsw got support for offloading mirror-to-gretap such that
> the underlay packet path involves a bridge. In that case, the offload is
> also influenced by PVID setting of said bridge. However, changes to VLAN
> configuration of the bridge itself do not generate switchdev
> notifications, so there's no mechanism to prod mlxsw to update the
> offload when these settings change.
> 
> In this patchset, the problem is resolved by distributing the switchdev
> notification SWITCHDEV_OBJ_ID_PORT_VLAN also for configuration changes
> on bridge VLANs. Since stacked devices distribute the notification to
> lower devices, such event eventually reaches the driver, which can
> determine whether it's a bridge or port VLAN by inspecting orig_dev.
> 
> To keep things consistent, the newly-distributed notifications observe
> the same protocol as the existing ones: dual prepare/commit, with
> -EOPNOTSUPP indicating lack of support, even though there's currently
> nothing to prepare for and nothing to support. Correspondingly, all
> switchdev drivers have been updated to return -EOPNOTSUPP for bridge
> VLAN notifications.

You seem to have approached the bridge changes a little differently from
this series:

https://lists.linux-foundation.org/pipermail/bridge/2016-November/010112.html

Both have the same intent that by targeting the bridge device itself,
you can propagate that through switchdev to the switch drivers, and in
turn create configurations where for instance, you have:

- CPU/management port present in specific VLANs that is a subset or
superset of the VLANs configured on front-panel ports
- CPU/management port tagged/untagged in specific VLANs which can be a
different setting from the front-panel ports

One problem we have in DSA at the moment is that we always add the CPU
port to the VLANs configured to the front-panel port but we do this with
the same attributes as the front panel ports! For instance, if you add
Port 0 to VLAN1 untagged, the the CPU port also gets added to that
VLAN1, also untagged. As long as there is just one VLAN untagged, this
is not much of a problem. Now do this with another VLAN or another port,
and the CPU can no longer differentiate the traffic from which VLAN it
is coming from, no bueno.

I had specifically changed b53 to always add the CPU port as tagged,
because that would always allow for differentiating traffic, but I would
rather have the capability to configure that at the bridge layer, which
you series seem to allow.

For the record, here is what the first commit in the series intended to
let an user do:

The following happens now (assuming bridge master device is already
created):

bridge vlan add vid 2 dev port0 pvid untagged
	-> port0 (e.g: switch port 0) gets programmed
	-> CPU port gets programmed
bridge vlan add vid 2 dev br0 self
	-> CPU port gets programmed
bridge vlan add vid 2 dev port0
	-> port0 (switch port 0) gets programmed

Are these use cases possible with your series? It seems to me like it is
if we drop the netif_is_bridge_master() checks and resolve orig_dev as
being a hint for the CPU/management port.

Thanks for reading me :)

> 
> In patch #1, the code to send notifications for adding and deleting is
> factored out into two named functions.
> 
> In patches #2-#5, respectively for mlxsw, rocker, DSA and DPAA2 ethsw,
> the new notifications (which are not enabled yet) are ignored to
> maintain the current behavior.
> 
> In patch #6, the notification is actually enabled.
> 
> In patch #7, mlxsw is changed to update offloads of mirror-to-gre also
> for bridge-related notifications.
> 
> Petr Machata (7):
>   net: bridge: Extract boilerplate around switchdev_port_obj_*()
>   mlxsw: spectrum_switchdev: Ignore bridge VLAN events
>   rocker: rocker_main: Ignore bridge VLAN events
>   dsa: port: Ignore bridge VLAN events
>   staging: fsl-dpaa2: ethsw: Ignore bridge VLAN events
>   net: bridge: Notify about bridge VLANs
>   mlxsw: spectrum_switchdev: Schedule respin during trans prepare
> 
>  .../ethernet/mellanox/mlxsw/spectrum_switchdev.c   |  8 ++-
>  drivers/net/ethernet/rocker/rocker_main.c          |  6 +++
>  drivers/staging/fsl-dpaa2/ethsw/ethsw.c            |  6 +++
>  net/bridge/br_vlan.c                               | 58 ++++++++++++++--------
>  net/dsa/port.c                                     |  6 +++
>  5 files changed, 62 insertions(+), 22 deletions(-)
> 


-- 
Florian

Re: [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs

[Florian Fainelli]

On 05/24/2018 10:20 AM, Florian Fainelli wrote:
> Hi Petr,
> 
> On 05/24/2018 08:09 AM, Petr Machata wrote:
>> In commit 946a11e7408e ("mlxsw: spectrum_span: Allow bridge for gretap
>> mirror"), mlxsw got support for offloading mirror-to-gretap such that
>> the underlay packet path involves a bridge. In that case, the offload is
>> also influenced by PVID setting of said bridge. However, changes to VLAN
>> configuration of the bridge itself do not generate switchdev
>> notifications, so there's no mechanism to prod mlxsw to update the
>> offload when these settings change.
>>
>> In this patchset, the problem is resolved by distributing the switchdev
>> notification SWITCHDEV_OBJ_ID_PORT_VLAN also for configuration changes
>> on bridge VLANs. Since stacked devices distribute the notification to
>> lower devices, such event eventually reaches the driver, which can
>> determine whether it's a bridge or port VLAN by inspecting orig_dev.
>>
>> To keep things consistent, the newly-distributed notifications observe
>> the same protocol as the existing ones: dual prepare/commit, with
>> -EOPNOTSUPP indicating lack of support, even though there's currently
>> nothing to prepare for and nothing to support. Correspondingly, all
>> switchdev drivers have been updated to return -EOPNOTSUPP for bridge
>> VLAN notifications.
> 
> You seem to have approached the bridge changes a little differently from
> this series:
> 
> https://lists.linux-foundation.org/pipermail/bridge/2016-November/010112.html
> 
> Both have the same intent that by targeting the bridge device itself,
> you can propagate that through switchdev to the switch drivers, and in
> turn create configurations where for instance, you have:
> 
> - CPU/management port present in specific VLANs that is a subset or
> superset of the VLANs configured on front-panel ports
> - CPU/management port tagged/untagged in specific VLANs which can be a
> different setting from the front-panel ports
> 
> One problem we have in DSA at the moment is that we always add the CPU
> port to the VLANs configured to the front-panel port but we do this with
> the same attributes as the front panel ports! For instance, if you add
> Port 0 to VLAN1 untagged, the the CPU port also gets added to that
> VLAN1, also untagged. As long as there is just one VLAN untagged, this
> is not much of a problem. Now do this with another VLAN or another port,
> and the CPU can no longer differentiate the traffic from which VLAN it
> is coming from, no bueno.
> 
> I had specifically changed b53 to always add the CPU port as tagged,
> because that would always allow for differentiating traffic, but I would
> rather have the capability to configure that at the bridge layer, which
> you series seem to allow.
> 
> For the record, here is what the first commit in the series intended to
> let an user do:
> 
> The following happens now (assuming bridge master device is already
> created):
> 
> bridge vlan add vid 2 dev port0 pvid untagged
> 	-> port0 (e.g: switch port 0) gets programmed
> 	-> CPU port gets programmed
> bridge vlan add vid 2 dev br0 self
> 	-> CPU port gets programmed
> bridge vlan add vid 2 dev port0
> 	-> port0 (switch port 0) gets programmed
> 
> Are these use cases possible with your series? It seems to me like it is
> if we drop the netif_is_bridge_master() checks and resolve orig_dev as
> being a hint for the CPU/management port.

So I changed your code a little bit in net/dsa/port.c to verify what
would happen and so far, this looks good except that I am seeing more
programming events than I am expecting. The change I did is the following:

diff --git a/net/dsa/port.c b/net/dsa/port.c
index ed0595459df1..37385e491117 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -253,7 +253,7 @@ int dsa_port_vlan_add(struct dsa_port *dp,
        };

        if (netif_is_bridge_master(vlan->obj.orig_dev))
-               return -EOPNOTSUPP;
+               info.port = dp->cpu_dp->index;

        if (br_vlan_enabled(dp->bridge_dev))
                return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_ADD, &info);
@@ -271,7 +271,7 @@ int dsa_port_vlan_del(struct dsa_port *dp,
        };

        if (netif_is_bridge_master(vlan->obj.orig_dev))
-               return -EOPNOTSUPP;
+               info.port = dp->cpu_dp->index;

        if (br_vlan_enabled(dp->bridge_dev))
                return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);

And the commands above result in the following:

root@net-vm:~# bridge vlan add vid 2 dev lan1 pvid untagged
[  478.065728] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 0
vlan: 2, untagged
[  478.066440] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5
vlan: 2, untagged
[  478.067890] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5
vlan: 2, tagged
[  478.068486] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5
vlan: 2, tagged
root@net-vm:~# bridge vlan add vid 2 dev br0 self
[  507.931313] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5
vlan: 2, tagged
[  507.931826] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5
vlan: 2, tagged
root@net-vm:~# bridge vlan add vid 2 dev lan1
[  518.955814] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 0
vlan: 2, tagged
[  518.956454] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5
vlan: 2, tagged
root@net-vm:~#

So commands #1 and #2 get too many events targeting the CPU port, if we
remove the following hunk:

diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index b93511726069..60ecc87bf6c0 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -212,7 +212,7 @@ static int dsa_switch_vlan_add(struct dsa_switch *ds,
        if (ds->index == info->sw_index)
                set_bit(info->port, members);
        for (port = 0; port < ds->num_ports; port++)
-               if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
+               if (dsa_is_dsa_port(ds, port))
                        set_bit(port, members);

        if (switchdev_trans_ph_prepare(trans))

Then we get the expected number of events for the ports:

root@net-vm:~# bridge vlan add vid 2 dev lan1 pvid untagged
[  111.906710] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 0
vlan: 2, untagged, pvid: 1
[  111.908988] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5
vlan: 2, tagged, pvid: 0
root@net-vm:~# bridge vlan add vid 2 dev br0 self
[  121.829272] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5
vlan: 2, tagged, pvid: 0
root@net-vm:~# bridge vlan add vid 2 dev lan1
[  133.224113] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 0
vlan: 2, tagged, pvid: 0

Andrew, Vivien, if the following hunks get applied are we possibly
breaking mv88e6xxx? This is the use case that is really missing IMHO at
the moment in DSA: we cannot control the VLAN membership and attributes
of the CPU port(s), so either we make it always tagged in every VLAN
(not great), or we introduce the ability to target the CPU port which is
what Petr's patches + mine do.

Thanks a second time for reading me :)
-- 
Florian

Re: [Bridge] [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs

[Florian Fainelli]

On 05/24/2018 10:20 AM, Florian Fainelli wrote:
> Hi Petr,
> 
> On 05/24/2018 08:09 AM, Petr Machata wrote:
>> In commit 946a11e7408e ("mlxsw: spectrum_span: Allow bridge for gretap
>> mirror"), mlxsw got support for offloading mirror-to-gretap such that
>> the underlay packet path involves a bridge. In that case, the offload is
>> also influenced by PVID setting of said bridge. However, changes to VLAN
>> configuration of the bridge itself do not generate switchdev
>> notifications, so there's no mechanism to prod mlxsw to update the
>> offload when these settings change.
>>
>> In this patchset, the problem is resolved by distributing the switchdev
>> notification SWITCHDEV_OBJ_ID_PORT_VLAN also for configuration changes
>> on bridge VLANs. Since stacked devices distribute the notification to
>> lower devices, such event eventually reaches the driver, which can
>> determine whether it's a bridge or port VLAN by inspecting orig_dev.
>>
>> To keep things consistent, the newly-distributed notifications observe
>> the same protocol as the existing ones: dual prepare/commit, with
>> -EOPNOTSUPP indicating lack of support, even though there's currently
>> nothing to prepare for and nothing to support. Correspondingly, all
>> switchdev drivers have been updated to return -EOPNOTSUPP for bridge
>> VLAN notifications.
> 
> You seem to have approached the bridge changes a little differently from
> this series:
> 
> https://lists.linux-foundation.org/pipermail/bridge/2016-November/010112.html
> 
> Both have the same intent that by targeting the bridge device itself,
> you can propagate that through switchdev to the switch drivers, and in
> turn create configurations where for instance, you have:
> 
> - CPU/management port present in specific VLANs that is a subset or
> superset of the VLANs configured on front-panel ports
> - CPU/management port tagged/untagged in specific VLANs which can be a
> different setting from the front-panel ports
> 
> One problem we have in DSA at the moment is that we always add the CPU
> port to the VLANs configured to the front-panel port but we do this with
> the same attributes as the front panel ports! For instance, if you add
> Port 0 to VLAN1 untagged, the the CPU port also gets added to that
> VLAN1, also untagged. As long as there is just one VLAN untagged, this
> is not much of a problem. Now do this with another VLAN or another port,
> and the CPU can no longer differentiate the traffic from which VLAN it
> is coming from, no bueno.
> 
> I had specifically changed b53 to always add the CPU port as tagged,
> because that would always allow for differentiating traffic, but I would
> rather have the capability to configure that at the bridge layer, which
> you series seem to allow.
> 
> For the record, here is what the first commit in the series intended to
> let an user do:
> 
> The following happens now (assuming bridge master device is already
> created):
> 
> bridge vlan add vid 2 dev port0 pvid untagged
> 	-> port0 (e.g: switch port 0) gets programmed
> 	-> CPU port gets programmed
> bridge vlan add vid 2 dev br0 self
> 	-> CPU port gets programmed
> bridge vlan add vid 2 dev port0
> 	-> port0 (switch port 0) gets programmed
> 
> Are these use cases possible with your series? It seems to me like it is
> if we drop the netif_is_bridge_master() checks and resolve orig_dev as
> being a hint for the CPU/management port.

So I changed your code a little bit in net/dsa/port.c to verify what
would happen and so far, this looks good except that I am seeing more
programming events than I am expecting. The change I did is the following:

diff --git a/net/dsa/port.c b/net/dsa/port.c
index ed0595459df1..37385e491117 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -253,7 +253,7 @@ int dsa_port_vlan_add(struct dsa_port *dp,
        };

        if (netif_is_bridge_master(vlan->obj.orig_dev))
-               return -EOPNOTSUPP;
+               info.port = dp->cpu_dp->index;

        if (br_vlan_enabled(dp->bridge_dev))
                return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_ADD, &info);
@@ -271,7 +271,7 @@ int dsa_port_vlan_del(struct dsa_port *dp,
        };

        if (netif_is_bridge_master(vlan->obj.orig_dev))
-               return -EOPNOTSUPP;
+               info.port = dp->cpu_dp->index;

        if (br_vlan_enabled(dp->bridge_dev))
                return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);

And the commands above result in the following:

root@net-vm:~# bridge vlan add vid 2 dev lan1 pvid untagged
[  478.065728] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 0
vlan: 2, untagged
[  478.066440] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5
vlan: 2, untagged
[  478.067890] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5
vlan: 2, tagged
[  478.068486] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5
vlan: 2, tagged
root@net-vm:~# bridge vlan add vid 2 dev br0 self
[  507.931313] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5
vlan: 2, tagged
[  507.931826] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5
vlan: 2, tagged
root@net-vm:~# bridge vlan add vid 2 dev lan1
[  518.955814] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 0
vlan: 2, tagged
[  518.956454] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5
vlan: 2, tagged
root@net-vm:~#

So commands #1 and #2 get too many events targeting the CPU port, if we
remove the following hunk:

diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index b93511726069..60ecc87bf6c0 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -212,7 +212,7 @@ static int dsa_switch_vlan_add(struct dsa_switch *ds,
        if (ds->index == info->sw_index)
                set_bit(info->port, members);
        for (port = 0; port < ds->num_ports; port++)
-               if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
+               if (dsa_is_dsa_port(ds, port))
                        set_bit(port, members);

        if (switchdev_trans_ph_prepare(trans))

Then we get the expected number of events for the ports:

root@net-vm:~# bridge vlan add vid 2 dev lan1 pvid untagged
[  111.906710] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 0
vlan: 2, untagged, pvid: 1
[  111.908988] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5
vlan: 2, tagged, pvid: 0
root@net-vm:~# bridge vlan add vid 2 dev br0 self
[  121.829272] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 5
vlan: 2, tagged, pvid: 0
root@net-vm:~# bridge vlan add vid 2 dev lan1
[  133.224113] dsa-loop fixed-0:1f: dsa_loop_port_vlan_add: port: 0
vlan: 2, tagged, pvid: 0

Andrew, Vivien, if the following hunks get applied are we possibly
breaking mv88e6xxx? This is the use case that is really missing IMHO at
the moment in DSA: we cannot control the VLAN membership and attributes
of the CPU port(s), so either we make it always tagged in every VLAN
(not great), or we introduce the ability to target the CPU port which is
what Petr's patches + mine do.

Thanks a second time for reading me :)
-- 
Florian

Re: [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs

[Florian Fainelli]

On 05/24/2018 08:09 AM, Petr Machata wrote:
> In commit 946a11e7408e ("mlxsw: spectrum_span: Allow bridge for gretap
> mirror"), mlxsw got support for offloading mirror-to-gretap such that
> the underlay packet path involves a bridge. In that case, the offload is
> also influenced by PVID setting of said bridge. However, changes to VLAN
> configuration of the bridge itself do not generate switchdev
> notifications, so there's no mechanism to prod mlxsw to update the
> offload when these settings change.
> 
> In this patchset, the problem is resolved by distributing the switchdev
> notification SWITCHDEV_OBJ_ID_PORT_VLAN also for configuration changes
> on bridge VLANs. Since stacked devices distribute the notification to
> lower devices, such event eventually reaches the driver, which can
> determine whether it's a bridge or port VLAN by inspecting orig_dev.
> 
> To keep things consistent, the newly-distributed notifications observe
> the same protocol as the existing ones: dual prepare/commit, with
> -EOPNOTSUPP indicating lack of support, even though there's currently
> nothing to prepare for and nothing to support. Correspondingly, all
> switchdev drivers have been updated to return -EOPNOTSUPP for bridge
> VLAN notifications.

This is great, see the other two emails about why I like it so much:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks!

> 
> In patch #1, the code to send notifications for adding and deleting is
> factored out into two named functions.
> 
> In patches #2-#5, respectively for mlxsw, rocker, DSA and DPAA2 ethsw,
> the new notifications (which are not enabled yet) are ignored to
> maintain the current behavior.
> 
> In patch #6, the notification is actually enabled.
> 
> In patch #7, mlxsw is changed to update offloads of mirror-to-gre also
> for bridge-related notifications.
> 
> Petr Machata (7):
>   net: bridge: Extract boilerplate around switchdev_port_obj_*()
>   mlxsw: spectrum_switchdev: Ignore bridge VLAN events
>   rocker: rocker_main: Ignore bridge VLAN events
>   dsa: port: Ignore bridge VLAN events
>   staging: fsl-dpaa2: ethsw: Ignore bridge VLAN events
>   net: bridge: Notify about bridge VLANs
>   mlxsw: spectrum_switchdev: Schedule respin during trans prepare
> 
>  .../ethernet/mellanox/mlxsw/spectrum_switchdev.c   |  8 ++-
>  drivers/net/ethernet/rocker/rocker_main.c          |  6 +++
>  drivers/staging/fsl-dpaa2/ethsw/ethsw.c            |  6 +++
>  net/bridge/br_vlan.c                               | 58 ++++++++++++++--------
>  net/dsa/port.c                                     |  6 +++
>  5 files changed, 62 insertions(+), 22 deletions(-)
> 

-- 
Florian

Re: [Bridge] [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs

[Florian Fainelli]

On 05/24/2018 08:09 AM, Petr Machata wrote:
> In commit 946a11e7408e ("mlxsw: spectrum_span: Allow bridge for gretap
> mirror"), mlxsw got support for offloading mirror-to-gretap such that
> the underlay packet path involves a bridge. In that case, the offload is
> also influenced by PVID setting of said bridge. However, changes to VLAN
> configuration of the bridge itself do not generate switchdev
> notifications, so there's no mechanism to prod mlxsw to update the
> offload when these settings change.
> 
> In this patchset, the problem is resolved by distributing the switchdev
> notification SWITCHDEV_OBJ_ID_PORT_VLAN also for configuration changes
> on bridge VLANs. Since stacked devices distribute the notification to
> lower devices, such event eventually reaches the driver, which can
> determine whether it's a bridge or port VLAN by inspecting orig_dev.
> 
> To keep things consistent, the newly-distributed notifications observe
> the same protocol as the existing ones: dual prepare/commit, with
> -EOPNOTSUPP indicating lack of support, even though there's currently
> nothing to prepare for and nothing to support. Correspondingly, all
> switchdev drivers have been updated to return -EOPNOTSUPP for bridge
> VLAN notifications.

This is great, see the other two emails about why I like it so much:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks!

> 
> In patch #1, the code to send notifications for adding and deleting is
> factored out into two named functions.
> 
> In patches #2-#5, respectively for mlxsw, rocker, DSA and DPAA2 ethsw,
> the new notifications (which are not enabled yet) are ignored to
> maintain the current behavior.
> 
> In patch #6, the notification is actually enabled.
> 
> In patch #7, mlxsw is changed to update offloads of mirror-to-gre also
> for bridge-related notifications.
> 
> Petr Machata (7):
>   net: bridge: Extract boilerplate around switchdev_port_obj_*()
>   mlxsw: spectrum_switchdev: Ignore bridge VLAN events
>   rocker: rocker_main: Ignore bridge VLAN events
>   dsa: port: Ignore bridge VLAN events
>   staging: fsl-dpaa2: ethsw: Ignore bridge VLAN events
>   net: bridge: Notify about bridge VLANs
>   mlxsw: spectrum_switchdev: Schedule respin during trans prepare
> 
>  .../ethernet/mellanox/mlxsw/spectrum_switchdev.c   |  8 ++-
>  drivers/net/ethernet/rocker/rocker_main.c          |  6 +++
>  drivers/staging/fsl-dpaa2/ethsw/ethsw.c            |  6 +++
>  net/bridge/br_vlan.c                               | 58 ++++++++++++++--------
>  net/dsa/port.c                                     |  6 +++
>  5 files changed, 62 insertions(+), 22 deletions(-)
> 

-- 
Florian

Re: [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs

[Petr Machata]

Florian Fainelli <f.fainelli@gmail.com> writes:

> You seem to have approached the bridge changes a little differently from
> this series:
>
> https://lists.linux-foundation.org/pipermail/bridge/2016-November/010112.html

It pretty much extends the patchset to also send the notifications for
the CPU port.

I missed this e-mail yesterday and now I see you already found out for
yourself how it behaves.

> Both have the same intent that by targeting the bridge device itself,
> you can propagate that through switchdev to the switch drivers, and in
> turn create configurations where for instance, you have:
>
> - CPU/management port present in specific VLANs that is a subset or
> superset of the VLANs configured on front-panel ports
> - CPU/management port tagged/untagged in specific VLANs which can be a
> different setting from the front-panel ports
>
> One problem we have in DSA at the moment is that we always add the CPU
> port to the VLANs configured to the front-panel port but we do this with
> the same attributes as the front panel ports! For instance, if you add
> Port 0 to VLAN1 untagged, the the CPU port also gets added to that
> VLAN1, also untagged. As long as there is just one VLAN untagged, this
> is not much of a problem. Now do this with another VLAN or another port,
> and the CPU can no longer differentiate the traffic from which VLAN it
> is coming from, no bueno.

Yep, with this patchset you should be able to use the CPU port
notifications to configure things exactly.

> bridge vlan add vid 2 dev port0 pvid untagged
> 	-> port0 (e.g: switch port 0) gets programmed
> 	-> CPU port gets programmed
> bridge vlan add vid 2 dev br0 self
> 	-> CPU port gets programmed
> bridge vlan add vid 2 dev port0
> 	-> port0 (switch port 0) gets programmed
>
> Are these use cases possible with your series? It seems to me like it is
> if we drop the netif_is_bridge_master() checks and resolve orig_dev as
> being a hint for the CPU/management port.

Yeah, that's how it behaves. If you accept the events where
netif_is_bridge_master(orig_dev), you can tell the CPU port-related
events from the rest by BRIDGE_VLAN_INFO_BRENTRY.

Thanks,
Petr

Re: [Bridge] [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs

[Petr Machata]

Florian Fainelli <f.fainelli@gmail.com> writes:

> You seem to have approached the bridge changes a little differently from
> this series:
>
> https://lists.linux-foundation.org/pipermail/bridge/2016-November/010112.html

It pretty much extends the patchset to also send the notifications for
the CPU port.

I missed this e-mail yesterday and now I see you already found out for
yourself how it behaves.

> Both have the same intent that by targeting the bridge device itself,
> you can propagate that through switchdev to the switch drivers, and in
> turn create configurations where for instance, you have:
>
> - CPU/management port present in specific VLANs that is a subset or
> superset of the VLANs configured on front-panel ports
> - CPU/management port tagged/untagged in specific VLANs which can be a
> different setting from the front-panel ports
>
> One problem we have in DSA at the moment is that we always add the CPU
> port to the VLANs configured to the front-panel port but we do this with
> the same attributes as the front panel ports! For instance, if you add
> Port 0 to VLAN1 untagged, the the CPU port also gets added to that
> VLAN1, also untagged. As long as there is just one VLAN untagged, this
> is not much of a problem. Now do this with another VLAN or another port,
> and the CPU can no longer differentiate the traffic from which VLAN it
> is coming from, no bueno.

Yep, with this patchset you should be able to use the CPU port
notifications to configure things exactly.

> bridge vlan add vid 2 dev port0 pvid untagged
> 	-> port0 (e.g: switch port 0) gets programmed
> 	-> CPU port gets programmed
> bridge vlan add vid 2 dev br0 self
> 	-> CPU port gets programmed
> bridge vlan add vid 2 dev port0
> 	-> port0 (switch port 0) gets programmed
>
> Are these use cases possible with your series? It seems to me like it is
> if we drop the netif_is_bridge_master() checks and resolve orig_dev as
> being a hint for the CPU/management port.

Yeah, that's how it behaves. If you accept the events where
netif_is_bridge_master(orig_dev), you can tell the CPU port-related
events from the rest by BRIDGE_VLAN_INFO_BRENTRY.

Thanks,
Petr

Re: [PATCH net-next 1/7] net: bridge: Extract boilerplate around switchdev_port_obj_*()

[Nikolay Aleksandrov]

On 24/05/18 18:10, Petr Machata wrote:
> A call to switchdev_port_obj_add() or switchdev_port_obj_del() involves
> initializing a struct switchdev_obj_port_vlan, a piece of code that
> repeats on each call site almost verbatim. While in the current codebase
> there is just one duplicated add call, the follow-up patches add more of
> both add and del calls.
> 
> Thus to remove the duplication, extract the repetition into named
> functions and reuse.
> 
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---
>  net/bridge/br_vlan.c | 44 +++++++++++++++++++++++---------------------
>  1 file changed, 23 insertions(+), 21 deletions(-)
> 

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

Re: [Bridge] [PATCH net-next 1/7] net: bridge: Extract boilerplate around switchdev_port_obj_*()

[Nikolay Aleksandrov]

On 24/05/18 18:10, Petr Machata wrote:
> A call to switchdev_port_obj_add() or switchdev_port_obj_del() involves
> initializing a struct switchdev_obj_port_vlan, a piece of code that
> repeats on each call site almost verbatim. While in the current codebase
> there is just one duplicated add call, the follow-up patches add more of
> both add and del calls.
> 
> Thus to remove the duplication, extract the repetition into named
> functions and reuse.
> 
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---
>  net/bridge/br_vlan.c | 44 +++++++++++++++++++++++---------------------
>  1 file changed, 23 insertions(+), 21 deletions(-)
> 

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

Re: [PATCH net-next 6/7] net: bridge: Notify about bridge VLANs

[Nikolay Aleksandrov]

On 24/05/18 18:10, Petr Machata wrote:
> A driver might need to react to changes in settings of brentry VLANs.
> Therefore send switchdev port notifications for these as well. Reuse
> SWITCHDEV_OBJ_ID_PORT_VLAN for this purpose. Listeners should use
> netif_is_bridge_master() on orig_dev to determine whether the
> notification is about a bridge port or a bridge.
> 
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---
>  net/bridge/br_vlan.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 

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

Re: [Bridge] [PATCH net-next 6/7] net: bridge: Notify about bridge VLANs

[Nikolay Aleksandrov]

On 24/05/18 18:10, Petr Machata wrote:
> A driver might need to react to changes in settings of brentry VLANs.
> Therefore send switchdev port notifications for these as well. Reuse
> SWITCHDEV_OBJ_ID_PORT_VLAN for this purpose. Listeners should use
> netif_is_bridge_master() on orig_dev to determine whether the
> notification is about a bridge port or a bridge.
> 
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---
>  net/bridge/br_vlan.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 

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

Re: [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs

[Ilias Apalodimas]

On Fri, May 25, 2018 at 01:09:46PM +0300, Petr Machata wrote:
> Florian Fainelli <f.fainelli@gmail.com> writes:
> 
> > You seem to have approached the bridge changes a little differently from
> > this series:
> >
> > https://lists.linux-foundation.org/pipermail/bridge/2016-November/010112.html
> 
> It pretty much extends the patchset to also send the notifications for
> the CPU port.
> 
> I missed this e-mail yesterday and now I see you already found out for
> yourself how it behaves.
> 
> > Both have the same intent that by targeting the bridge device itself,
> > you can propagate that through switchdev to the switch drivers, and in
> > turn create configurations where for instance, you have:
> >
> > - CPU/management port present in specific VLANs that is a subset or
> > superset of the VLANs configured on front-panel ports
> > - CPU/management port tagged/untagged in specific VLANs which can be a
> > different setting from the front-panel ports
> >
> > One problem we have in DSA at the moment is that we always add the CPU
> > port to the VLANs configured to the front-panel port but we do this with
> > the same attributes as the front panel ports! For instance, if you add
> > Port 0 to VLAN1 untagged, the the CPU port also gets added to that
> > VLAN1, also untagged. As long as there is just one VLAN untagged, this
> > is not much of a problem. Now do this with another VLAN or another port,
> > and the CPU can no longer differentiate the traffic from which VLAN it
> > is coming from, no bueno.
> 
> Yep, with this patchset you should be able to use the CPU port
> notifications to configure things exactly.
> 
> > bridge vlan add vid 2 dev port0 pvid untagged
> > 	-> port0 (e.g: switch port 0) gets programmed
> > 	-> CPU port gets programmed
> > bridge vlan add vid 2 dev br0 self
> > 	-> CPU port gets programmed
> > bridge vlan add vid 2 dev port0
> > 	-> port0 (switch port 0) gets programmed
> >
> > Are these use cases possible with your series? It seems to me like it is
> > if we drop the netif_is_bridge_master() checks and resolve orig_dev as
> > being a hint for the CPU/management port.
> 
> Yeah, that's how it behaves. If you accept the events where
> netif_is_bridge_master(orig_dev), you can tell the CPU port-related
> events from the rest by BRIDGE_VLAN_INFO_BRENTRY.
This also addresses the issue i am having trying to add switchdev functionality
to a driver that needs separate configuration for cpu port.  
https://www.spinics.net/lists/netdev/msg504577.html

Tested it and it works fine

Thanks,
Ilias

Re: [Bridge] [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs

[Ilias Apalodimas]

On Fri, May 25, 2018 at 01:09:46PM +0300, Petr Machata wrote:
> Florian Fainelli <f.fainelli@gmail.com> writes:
> 
> > You seem to have approached the bridge changes a little differently from
> > this series:
> >
> > https://lists.linux-foundation.org/pipermail/bridge/2016-November/010112.html
> 
> It pretty much extends the patchset to also send the notifications for
> the CPU port.
> 
> I missed this e-mail yesterday and now I see you already found out for
> yourself how it behaves.
> 
> > Both have the same intent that by targeting the bridge device itself,
> > you can propagate that through switchdev to the switch drivers, and in
> > turn create configurations where for instance, you have:
> >
> > - CPU/management port present in specific VLANs that is a subset or
> > superset of the VLANs configured on front-panel ports
> > - CPU/management port tagged/untagged in specific VLANs which can be a
> > different setting from the front-panel ports
> >
> > One problem we have in DSA at the moment is that we always add the CPU
> > port to the VLANs configured to the front-panel port but we do this with
> > the same attributes as the front panel ports! For instance, if you add
> > Port 0 to VLAN1 untagged, the the CPU port also gets added to that
> > VLAN1, also untagged. As long as there is just one VLAN untagged, this
> > is not much of a problem. Now do this with another VLAN or another port,
> > and the CPU can no longer differentiate the traffic from which VLAN it
> > is coming from, no bueno.
> 
> Yep, with this patchset you should be able to use the CPU port
> notifications to configure things exactly.
> 
> > bridge vlan add vid 2 dev port0 pvid untagged
> > 	-> port0 (e.g: switch port 0) gets programmed
> > 	-> CPU port gets programmed
> > bridge vlan add vid 2 dev br0 self
> > 	-> CPU port gets programmed
> > bridge vlan add vid 2 dev port0
> > 	-> port0 (switch port 0) gets programmed
> >
> > Are these use cases possible with your series? It seems to me like it is
> > if we drop the netif_is_bridge_master() checks and resolve orig_dev as
> > being a hint for the CPU/management port.
> 
> Yeah, that's how it behaves. If you accept the events where
> netif_is_bridge_master(orig_dev), you can tell the CPU port-related
> events from the rest by BRIDGE_VLAN_INFO_BRENTRY.
This also addresses the issue i am having trying to add switchdev functionality
to a driver that needs separate configuration for cpu port.  
https://www.spinics.net/lists/netdev/msg504577.html

Tested it and it works fine

Thanks,
Ilias

Re: [PATCH net-next 1/7] net: bridge: Extract boilerplate around switchdev_port_obj_*()

[Vivien Didelot]

Hi Petr,

Petr Machata <petrm@mellanox.com> writes:

> -static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
> -			  u16 vid, u16 flags)
> +static int br_switchdev_port_obj_add(struct net_device *dev, u16 vid, u16 flags)
>  {
>  	struct switchdev_obj_port_vlan v = {
>  		.obj.orig_dev = dev,
> @@ -89,12 +88,29 @@ static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
>  		.vid_begin = vid,
>  		.vid_end = vid,
>  	};
> -	int err;
>  
> +	return switchdev_port_obj_add(dev, &v.obj);
> +}
> +
> +static int br_switchdev_port_obj_del(struct net_device *dev, u16 vid)
> +{
> +	struct switchdev_obj_port_vlan v = {
> +		.obj.orig_dev = dev,
> +		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
> +		.vid_begin = vid,
> +		.vid_end = vid,
> +	};
> +
> +	return switchdev_port_obj_del(dev, &v.obj);
> +}

Shouldn't they be br_switchdev_port_vlan_add (or similar) implemented in
net/bridge/br_switchdev.c instead, since they are VLAN specific?

Other than that, the change looks good!

Thanks,

        Vivien

Re: [Bridge] [PATCH net-next 1/7] net: bridge: Extract boilerplate around switchdev_port_obj_*()

[Vivien Didelot]

Hi Petr,

Petr Machata <petrm@mellanox.com> writes:

> -static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
> -			  u16 vid, u16 flags)
> +static int br_switchdev_port_obj_add(struct net_device *dev, u16 vid, u16 flags)
>  {
>  	struct switchdev_obj_port_vlan v = {
>  		.obj.orig_dev = dev,
> @@ -89,12 +88,29 @@ static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
>  		.vid_begin = vid,
>  		.vid_end = vid,
>  	};
> -	int err;
>  
> +	return switchdev_port_obj_add(dev, &v.obj);
> +}
> +
> +static int br_switchdev_port_obj_del(struct net_device *dev, u16 vid)
> +{
> +	struct switchdev_obj_port_vlan v = {
> +		.obj.orig_dev = dev,
> +		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
> +		.vid_begin = vid,
> +		.vid_end = vid,
> +	};
> +
> +	return switchdev_port_obj_del(dev, &v.obj);
> +}

Shouldn't they be br_switchdev_port_vlan_add (or similar) implemented in
net/bridge/br_switchdev.c instead, since they are VLAN specific?

Other than that, the change looks good!

Thanks,

        Vivien

Re: [PATCH net-next 4/7] dsa: port: Ignore bridge VLAN events

[Vivien Didelot]

Hi Petr,

Petr Machata <petrm@mellanox.com> writes:

> Ignore VLAN events where the orig_dev is the bridge device itself.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Thanks,

        Vivien

Re: [Bridge] [PATCH net-next 4/7] dsa: port: Ignore bridge VLAN events

[Vivien Didelot]

Hi Petr,

Petr Machata <petrm@mellanox.com> writes:

> Ignore VLAN events where the orig_dev is the bridge device itself.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Thanks,

        Vivien

Re: [PATCH net-next 6/7] net: bridge: Notify about bridge VLANs

[Vivien Didelot]

Hi Petr,

Petr Machata <petrm@mellanox.com> writes:

> A driver might need to react to changes in settings of brentry VLANs.
> Therefore send switchdev port notifications for these as well. Reuse
> SWITCHDEV_OBJ_ID_PORT_VLAN for this purpose. Listeners should use
> netif_is_bridge_master() on orig_dev to determine whether the
> notification is about a bridge port or a bridge.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>

> +	} else {
> +		err = br_switchdev_port_obj_add(dev, v->vid, flags);
> +		if (err && err != -EOPNOTSUPP)
> +			goto out;
>  	}

Except that br_switchdev_port_obj_add taking vid and flags arguments
seems confusing to me, the change looks good:

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Thanks,

        Vivien

Re: [Bridge] [PATCH net-next 6/7] net: bridge: Notify about bridge VLANs

[Vivien Didelot]

Hi Petr,

Petr Machata <petrm@mellanox.com> writes:

> A driver might need to react to changes in settings of brentry VLANs.
> Therefore send switchdev port notifications for these as well. Reuse
> SWITCHDEV_OBJ_ID_PORT_VLAN for this purpose. Listeners should use
> netif_is_bridge_master() on orig_dev to determine whether the
> notification is about a bridge port or a bridge.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>

> +	} else {
> +		err = br_switchdev_port_obj_add(dev, v->vid, flags);
> +		if (err && err != -EOPNOTSUPP)
> +			goto out;
>  	}

Except that br_switchdev_port_obj_add taking vid and flags arguments
seems confusing to me, the change looks good:

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Thanks,

        Vivien

Re: [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs

[Vivien Didelot]

Hi Florian,

Florian Fainelli <f.fainelli@gmail.com> writes:

> Andrew, Vivien, if the following hunks get applied are we possibly
> breaking mv88e6xxx? This is the use case that is really missing IMHO at
> the moment in DSA: we cannot control the VLAN membership and attributes
> of the CPU port(s), so either we make it always tagged in every VLAN
> (not great), or we introduce the ability to target the CPU port which is
> what Petr's patches + mine do.

Your change looks good to me. mv88e6xxx programs the DSA and CPU ports'
membership as "unmodified" (i.e. "as-is", which is a Marvell feature),
so that shouldn't change the current behavior.


Thanks,

        Vivien

Re: [Bridge] [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs

[Vivien Didelot]

Hi Florian,

Florian Fainelli <f.fainelli@gmail.com> writes:

> Andrew, Vivien, if the following hunks get applied are we possibly
> breaking mv88e6xxx? This is the use case that is really missing IMHO at
> the moment in DSA: we cannot control the VLAN membership and attributes
> of the CPU port(s), so either we make it always tagged in every VLAN
> (not great), or we introduce the ability to target the CPU port which is
> what Petr's patches + mine do.

Your change looks good to me. mv88e6xxx programs the DSA and CPU ports'
membership as "unmodified" (i.e. "as-is", which is a Marvell feature),
so that shouldn't change the current behavior.


Thanks,

        Vivien

Re: [PATCH net-next 1/7] net: bridge: Extract boilerplate around switchdev_port_obj_*()

[Petr Machata]

Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:

> Hi Petr,
>
> Petr Machata <petrm@mellanox.com> writes:
>
>> -static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
>> -			  u16 vid, u16 flags)
>> +static int br_switchdev_port_obj_add(struct net_device *dev, u16 vid, u16 flags)
>>  {
>>  	struct switchdev_obj_port_vlan v = {
>>  		.obj.orig_dev = dev,
>> @@ -89,12 +88,29 @@ static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
>>  		.vid_begin = vid,
>>  		.vid_end = vid,
>>  	};
>> -	int err;
>>  
>> +	return switchdev_port_obj_add(dev, &v.obj);
>> +}
>> +
>> +static int br_switchdev_port_obj_del(struct net_device *dev, u16 vid)
>> +{
>> +	struct switchdev_obj_port_vlan v = {
>> +		.obj.orig_dev = dev,
>> +		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
>> +		.vid_begin = vid,
>> +		.vid_end = vid,
>> +	};
>> +
>> +	return switchdev_port_obj_del(dev, &v.obj);
>> +}
>
> Shouldn't they be br_switchdev_port_vlan_add (or similar) implemented in
> net/bridge/br_switchdev.c instead, since they are VLAN specific?

(You mean switchdev-specific?)

This logic was in br_vlan.c before as well, so it's natural to think
about the functions as helpers of VLAN module. I can move to
br_switchdev.c if you think that's the better place.

Thanks,
Petr

Re: [Bridge] [PATCH net-next 1/7] net: bridge: Extract boilerplate around switchdev_port_obj_*()

[Petr Machata]

Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:

> Hi Petr,
>
> Petr Machata <petrm@mellanox.com> writes:
>
>> -static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
>> -			  u16 vid, u16 flags)
>> +static int br_switchdev_port_obj_add(struct net_device *dev, u16 vid, u16 flags)
>>  {
>>  	struct switchdev_obj_port_vlan v = {
>>  		.obj.orig_dev = dev,
>> @@ -89,12 +88,29 @@ static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
>>  		.vid_begin = vid,
>>  		.vid_end = vid,
>>  	};
>> -	int err;
>>  
>> +	return switchdev_port_obj_add(dev, &v.obj);
>> +}
>> +
>> +static int br_switchdev_port_obj_del(struct net_device *dev, u16 vid)
>> +{
>> +	struct switchdev_obj_port_vlan v = {
>> +		.obj.orig_dev = dev,
>> +		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
>> +		.vid_begin = vid,
>> +		.vid_end = vid,
>> +	};
>> +
>> +	return switchdev_port_obj_del(dev, &v.obj);
>> +}
>
> Shouldn't they be br_switchdev_port_vlan_add (or similar) implemented in
> net/bridge/br_switchdev.c instead, since they are VLAN specific?

(You mean switchdev-specific?)

This logic was in br_vlan.c before as well, so it's natural to think
about the functions as helpers of VLAN module. I can move to
br_switchdev.c if you think that's the better place.

Thanks,
Petr

Re: [PATCH net-next 6/7] net: bridge: Notify about bridge VLANs

[Petr Machata]

Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:

>> +	} else {
>> +		err = br_switchdev_port_obj_add(dev, v->vid, flags);
>> +		if (err && err != -EOPNOTSUPP)
>> +			goto out;
>>  	}
>
> Except that br_switchdev_port_obj_add taking vid and flags arguments
> seems confusing to me, the change looks good:

I'm not sure what you're aiming at. Both VID and flags are sent with the
notification, so they need to be passed on to the function somehow. Do
you have a counterproposal for the API?

Thanks,
Petr

Re: [Bridge] [PATCH net-next 6/7] net: bridge: Notify about bridge VLANs

[Petr Machata]

Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:

>> +	} else {
>> +		err = br_switchdev_port_obj_add(dev, v->vid, flags);
>> +		if (err && err != -EOPNOTSUPP)
>> +			goto out;
>>  	}
>
> Except that br_switchdev_port_obj_add taking vid and flags arguments
> seems confusing to me, the change looks good:

I'm not sure what you're aiming at. Both VID and flags are sent with the
notification, so they need to be passed on to the function somehow. Do
you have a counterproposal for the API?

Thanks,
Petr

Re: [PATCH net-next 6/7] net: bridge: Notify about bridge VLANs

[Vivien Didelot]

Hi Petr,

Petr Machata <petrm@mellanox.com> writes:

> Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:
>
>>> +	} else {
>>> +		err = br_switchdev_port_obj_add(dev, v->vid, flags);
>>> +		if (err && err != -EOPNOTSUPP)
>>> +			goto out;
>>>  	}
>>
>> Except that br_switchdev_port_obj_add taking vid and flags arguments
>> seems confusing to me, the change looks good:
>
> I'm not sure what you're aiming at. Both VID and flags are sent with the
> notification, so they need to be passed on to the function somehow. Do
> you have a counterproposal for the API?

I'm only questioning the code organization here, not the functional
aspect which I do agree with. What I'm saying is that you name a new
switchdev helper br_switchdev_port_OBJ_add, which takes VLAN arguments
(vid and flags.) How would you call another eventual helper taking MDB
arguments, br_switchdev_port_OBJ_add again? So something like
br_switchdev_port_VLAN_add would be more intuitive.

At the same time there's an effort to centralize all switchdev helpers
of the bridge layer (i.e. the software -> hardware bridge calls) into
net/bridge/br_switchdev.c, so that file would be more adequate.

You may discard my comments but I think it'd be beneficial to us all to
finally keep a bit of consistency in that bridge layer code.


Thanks,

        Vivien

Re: [Bridge] [PATCH net-next 6/7] net: bridge: Notify about bridge VLANs

[Vivien Didelot]

Hi Petr,

Petr Machata <petrm@mellanox.com> writes:

> Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:
>
>>> +	} else {
>>> +		err = br_switchdev_port_obj_add(dev, v->vid, flags);
>>> +		if (err && err != -EOPNOTSUPP)
>>> +			goto out;
>>>  	}
>>
>> Except that br_switchdev_port_obj_add taking vid and flags arguments
>> seems confusing to me, the change looks good:
>
> I'm not sure what you're aiming at. Both VID and flags are sent with the
> notification, so they need to be passed on to the function somehow. Do
> you have a counterproposal for the API?

I'm only questioning the code organization here, not the functional
aspect which I do agree with. What I'm saying is that you name a new
switchdev helper br_switchdev_port_OBJ_add, which takes VLAN arguments
(vid and flags.) How would you call another eventual helper taking MDB
arguments, br_switchdev_port_OBJ_add again? So something like
br_switchdev_port_VLAN_add would be more intuitive.

At the same time there's an effort to centralize all switchdev helpers
of the bridge layer (i.e. the software -> hardware bridge calls) into
net/bridge/br_switchdev.c, so that file would be more adequate.

You may discard my comments but I think it'd be beneficial to us all to
finally keep a bit of consistency in that bridge layer code.


Thanks,

        Vivien

Re: [PATCH net-next 6/7] net: bridge: Notify about bridge VLANs

[Petr Machata]

Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:

> Hi Petr,
>
> Petr Machata <petrm@mellanox.com> writes:
>
>> Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:
>>
>>>> +	} else {
>>>> +		err = br_switchdev_port_obj_add(dev, v->vid, flags);
>>>> +		if (err && err != -EOPNOTSUPP)
>>>> +			goto out;
>>>>  	}
>>>
>>> Except that br_switchdev_port_obj_add taking vid and flags arguments
>>> seems confusing to me, the change looks good:
>>
>> I'm not sure what you're aiming at. Both VID and flags are sent with the
>> notification, so they need to be passed on to the function somehow. Do
>> you have a counterproposal for the API?
>
> I'm only questioning the code organization here, not the functional
> aspect which I do agree with. What I'm saying is that you name a new
> switchdev helper br_switchdev_port_OBJ_add, which takes VLAN arguments
> (vid and flags.) How would you call another eventual helper taking MDB
> arguments, br_switchdev_port_OBJ_add again? So something like
> br_switchdev_port_VLAN_add would be more intuitive.
>
> At the same time there's an effort to centralize all switchdev helpers
> of the bridge layer (i.e. the software -> hardware bridge calls) into
> net/bridge/br_switchdev.c, so that file would be more adequate.
>
> You may discard my comments but I think it'd be beneficial to us all to
> finally keep a bit of consistency in that bridge layer code.

Nope, those are reasonable points. I'll post a v2 along those lines.

Thanks,
Petr

Re: [Bridge] [PATCH net-next 6/7] net: bridge: Notify about bridge VLANs

[Petr Machata]

Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:

> Hi Petr,
>
> Petr Machata <petrm@mellanox.com> writes:
>
>> Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:
>>
>>>> +	} else {
>>>> +		err = br_switchdev_port_obj_add(dev, v->vid, flags);
>>>> +		if (err && err != -EOPNOTSUPP)
>>>> +			goto out;
>>>>  	}
>>>
>>> Except that br_switchdev_port_obj_add taking vid and flags arguments
>>> seems confusing to me, the change looks good:
>>
>> I'm not sure what you're aiming at. Both VID and flags are sent with the
>> notification, so they need to be passed on to the function somehow. Do
>> you have a counterproposal for the API?
>
> I'm only questioning the code organization here, not the functional
> aspect which I do agree with. What I'm saying is that you name a new
> switchdev helper br_switchdev_port_OBJ_add, which takes VLAN arguments
> (vid and flags.) How would you call another eventual helper taking MDB
> arguments, br_switchdev_port_OBJ_add again? So something like
> br_switchdev_port_VLAN_add would be more intuitive.
>
> At the same time there's an effort to centralize all switchdev helpers
> of the bridge layer (i.e. the software -> hardware bridge calls) into
> net/bridge/br_switchdev.c, so that file would be more adequate.
>
> You may discard my comments but I think it'd be beneficial to us all to
> finally keep a bit of consistency in that bridge layer code.

Nope, those are reasonable points. I'll post a v2 along those lines.

Thanks,
Petr