[patch net-next 0/6] mlxsw: Offload bridge device mrouter

[patch net-next 0/6] mlxsw: Offload bridge device mrouter

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Yotam says:

Similarly to a bridged port, the bridge device itself can be configured by
the user to be an mrouter port. In this case, all multicast traffic should
be forwarded to it. Make the mlxsw Spectrum driver offload these directives
to the Spectrum hardware.

Patches 1-3 add a new switchdev notification for bridge device mrouter
port status and make the bridge module notify about it.

Patches 4-6 change the mlxsw Spectrum driver to handle these notifications
by adding the Spectrum router port to the bridge MDB entries.

Yotam Gigi (6):
  net: bridge: Use the MDB_RTR_TYPE_TEMP on bridge device too
  net: bridge: Notify on bridge device mrouter state changes
  net: bridge: Export bridge multicast router state
  mlxsw: spectrum: router: Export the mlxsw_sp_router_port function
  mlxsw: spectrum_switchdev: Add support for router port in SMID entries
  mlxsw: spectrum_switchdev: Support bridge mrouter notifications

 .../net/ethernet/mellanox/mlxsw/spectrum_router.c  |  2 +-
 .../net/ethernet/mellanox/mlxsw/spectrum_router.h  |  1 +
 .../ethernet/mellanox/mlxsw/spectrum_switchdev.c   | 76 +++++++++++++++++++++-
 include/linux/if_bridge.h                          |  5 ++
 include/net/switchdev.h                            |  1 +
 net/bridge/br_multicast.c                          | 56 ++++++++++++++--
 net/bridge/br_netlink.c                            |  3 +-
 net/bridge/br_private.h                            | 13 +++-
 net/bridge/br_sysfs_br.c                           |  3 +-
 9 files changed, 147 insertions(+), 13 deletions(-)

-- 
2.9.5

[patch net-next 1/6] net: bridge: Use the MDB_RTR_TYPE_TEMP on bridge device too

[Jiri Pirko]

From: Yotam Gigi <yotamg@mellanox.com>

Every bridge port is in one of four mcast router port states:
 - MDB_RTR_TYPE_PERM - the port is set by the user to be an mrouter port
   regardless of IGMP queries.
 - MDB_RTR_TYPE_DISABLED - the port is set by the user to not be an mrouter
   port regardless of IGMP queries.
 - MDB_RTR_TYPE_TEMP - the port is set by the user to be in mcast router
   learning state, but currently it is not an mrouter port as no IGMP query
   has been received by it for the last multicast_querier_interval.
 - MDB_RTR_TYPE_TEMP_QUERY - the port is set by the user to be in mcast
   router learning state, and currently it is an mrouter port due to an
   IGMP query that has been received by it during the passed
   multicast_querier_interval.

The bridge device (brX) itself can also be configured by the user to be
either fixed, disabled or learning mrouter port states, but currently there
is no distinction between the MDB_RTR_TYPE_TEMP_QUERY and MDB_RTR_TYPE_TEMP
in the bridge internal state. Due to that, when an IGMP query is received,
it is not straightforward to tell whether it changes the bridge device
mrouter port status or not.

Further patches in this patch-set will introduce notifications upon the
bridge device mrouter port state. In order to prevent resending bridge
mrouter notification when it is not needed, such distinction is necessary.

Hence, add the distinction between MDB_RTR_TYPE_TEMP and
MDB_RTR_TYPE_TEMP_QUERY states for the bridge device, similarly to any
other bridge port.

In order to not break the current kernel-user API, don't propagate the new
state to the user and use it only in the bridge internal state. Thus, if
the user reads (either via sysfs or netlink) the bridge device mrouter
state, he will get the MDB_RTR_TYPE_TEMP_QUERY state even if the current
bridge state is MDB_RTR_TYPE_TEMP.

Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
Reviewed-by: Nogah Frankel <nogahf@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/bridge/br_multicast.c | 25 +++++++++++++++++++++----
 net/bridge/br_netlink.c   |  3 ++-
 net/bridge/br_private.h   | 13 ++++++++++---
 net/bridge/br_sysfs_br.c  |  3 ++-
 4 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 8dc5c8d..b86307b 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -861,6 +861,17 @@ static void br_multicast_router_expired(unsigned long data)
 
 static void br_multicast_local_router_expired(unsigned long data)
 {
+	struct net_bridge *br = (struct net_bridge *) data;
+
+	spin_lock(&br->multicast_lock);
+	if (br->multicast_router == MDB_RTR_TYPE_DISABLED ||
+	    br->multicast_router == MDB_RTR_TYPE_PERM ||
+	    timer_pending(&br->multicast_router_timer))
+		goto out;
+
+	br->multicast_router = MDB_RTR_TYPE_TEMP_QUERY;
+out:
+	spin_unlock(&br->multicast_lock);
 }
 
 static void br_multicast_querier_expired(struct net_bridge *br,
@@ -1364,9 +1375,12 @@ static void br_multicast_mark_router(struct net_bridge *br,
 	unsigned long now = jiffies;
 
 	if (!port) {
-		if (br->multicast_router == MDB_RTR_TYPE_TEMP_QUERY)
+		if (br->multicast_router == MDB_RTR_TYPE_TEMP_QUERY ||
+		    br->multicast_router == MDB_RTR_TYPE_TEMP) {
 			mod_timer(&br->multicast_router_timer,
 				  now + br->multicast_querier_interval);
+			br->multicast_router = MDB_RTR_TYPE_TEMP;
+		}
 		return;
 	}
 
@@ -1952,7 +1966,7 @@ void br_multicast_init(struct net_bridge *br)
 
 	spin_lock_init(&br->multicast_lock);
 	setup_timer(&br->multicast_router_timer,
-		    br_multicast_local_router_expired, 0);
+		    br_multicast_local_router_expired, (unsigned long)br);
 	setup_timer(&br->ip4_other_query.timer,
 		    br_ip4_multicast_querier_expired, (unsigned long)br);
 	setup_timer(&br->ip4_own_query.timer, br_ip4_multicast_query_expired,
@@ -2043,11 +2057,14 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
 	case MDB_RTR_TYPE_DISABLED:
 	case MDB_RTR_TYPE_PERM:
 		del_timer(&br->multicast_router_timer);
-		/* fall through */
-	case MDB_RTR_TYPE_TEMP_QUERY:
 		br->multicast_router = val;
 		err = 0;
 		break;
+	case MDB_RTR_TYPE_TEMP_QUERY:
+		if (br->multicast_router != MDB_RTR_TYPE_TEMP)
+			br->multicast_router = val;
+		err = 0;
+		break;
 	}
 
 	spin_unlock_bh(&br->multicast_lock);
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index dea88a2..cee5016 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1357,7 +1357,8 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
 		return -EMSGSIZE;
 #endif
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
-	if (nla_put_u8(skb, IFLA_BR_MCAST_ROUTER, br->multicast_router) ||
+	if (nla_put_u8(skb, IFLA_BR_MCAST_ROUTER,
+		       br_multicast_router_translate(br->multicast_router)) ||
 	    nla_put_u8(skb, IFLA_BR_MCAST_SNOOPING, !br->multicast_disabled) ||
 	    nla_put_u8(skb, IFLA_BR_MCAST_QUERY_USE_IFADDR,
 		       br->multicast_query_use_ifaddr) ||
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index ab4df24..e6e3fec 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -649,9 +649,8 @@ void br_multicast_get_stats(const struct net_bridge *br,
 
 static inline bool br_multicast_is_router(struct net_bridge *br)
 {
-	return br->multicast_router == 2 ||
-	       (br->multicast_router == 1 &&
-		timer_pending(&br->multicast_router_timer));
+	return br->multicast_router == MDB_RTR_TYPE_PERM ||
+	       br->multicast_router == MDB_RTR_TYPE_TEMP;
 }
 
 static inline bool
@@ -790,6 +789,14 @@ static inline int br_multicast_igmp_type(const struct sk_buff *skb)
 }
 #endif
 
+static inline unsigned char
+br_multicast_router_translate(unsigned char multicast_router)
+{
+	if (multicast_router == MDB_RTR_TYPE_TEMP)
+		return MDB_RTR_TYPE_TEMP_QUERY;
+	return multicast_router;
+}
+
 /* br_vlan.c */
 #ifdef CONFIG_BRIDGE_VLAN_FILTERING
 bool br_allowed_ingress(const struct net_bridge *br,
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 723f25e..9b9c597 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -340,7 +340,8 @@ static ssize_t multicast_router_show(struct device *d,
 				     struct device_attribute *attr, char *buf)
 {
 	struct net_bridge *br = to_bridge(d);
-	return sprintf(buf, "%d\n", br->multicast_router);
+	return sprintf(buf, "%d\n",
+		       br_multicast_router_translate(br->multicast_router));
 }
 
 static ssize_t multicast_router_store(struct device *d,
-- 
2.9.5

[patch net-next 2/6] net: bridge: Notify on bridge device mrouter state changes

[Jiri Pirko]

From: Yotam Gigi <yotamg@mellanox.com>

Add the SWITCHDEV_ATTR_ID_BRIDGE_MROUTER switchdev notification type, used
to indicate whether the bridge is or isn't mrouter. Notify when the bridge
changes its state, similarly to the already existing bridged port mrouter
notifications.

The notification uses the switchdev_attr.u.mrouter boolean flag to indicate
the current bridge mrouter status. Thus, it only indicates whether the
bridge is currently used as an mrouter or not, and does not indicate the
exact mrouter state of the bridge (learning, permanent, etc.).

Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
Reviewed-by: Nogah Frankel <nogahf@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/net/switchdev.h   |  1 +
 net/bridge/br_multicast.c | 21 ++++++++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index d767b79..d756fbe 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -51,6 +51,7 @@ enum switchdev_attr_id {
 	SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
 	SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING,
 	SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
+	SWITCHDEV_ATTR_ID_BRIDGE_MROUTER,
 };
 
 struct switchdev_attr {
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index b86307b..4d4fcb5 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -859,6 +859,19 @@ static void br_multicast_router_expired(unsigned long data)
 	spin_unlock(&br->multicast_lock);
 }
 
+static void br_mc_router_state_change(struct net_bridge *p,
+				      bool is_mc_router)
+{
+	struct switchdev_attr attr = {
+		.orig_dev = p->dev,
+		.id = SWITCHDEV_ATTR_ID_BRIDGE_MROUTER,
+		.flags = SWITCHDEV_F_DEFER,
+		.u.mrouter = is_mc_router,
+	};
+
+	switchdev_port_attr_set(p->dev, &attr);
+}
+
 static void br_multicast_local_router_expired(unsigned long data)
 {
 	struct net_bridge *br = (struct net_bridge *) data;
@@ -869,6 +882,7 @@ static void br_multicast_local_router_expired(unsigned long data)
 	    timer_pending(&br->multicast_router_timer))
 		goto out;
 
+	br_mc_router_state_change(br, false);
 	br->multicast_router = MDB_RTR_TYPE_TEMP_QUERY;
 out:
 	spin_unlock(&br->multicast_lock);
@@ -1379,6 +1393,8 @@ static void br_multicast_mark_router(struct net_bridge *br,
 		    br->multicast_router == MDB_RTR_TYPE_TEMP) {
 			mod_timer(&br->multicast_router_timer,
 				  now + br->multicast_querier_interval);
+			if (br->multicast_router == MDB_RTR_TYPE_TEMP_QUERY)
+				br_mc_router_state_change(br, true);
 			br->multicast_router = MDB_RTR_TYPE_TEMP;
 		}
 		return;
@@ -2056,13 +2072,16 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
 	switch (val) {
 	case MDB_RTR_TYPE_DISABLED:
 	case MDB_RTR_TYPE_PERM:
+		br_mc_router_state_change(br, val == MDB_RTR_TYPE_PERM);
 		del_timer(&br->multicast_router_timer);
 		br->multicast_router = val;
 		err = 0;
 		break;
 	case MDB_RTR_TYPE_TEMP_QUERY:
-		if (br->multicast_router != MDB_RTR_TYPE_TEMP)
+		if (br->multicast_router != MDB_RTR_TYPE_TEMP) {
+			br_mc_router_state_change(br, false);
 			br->multicast_router = val;
+		}
 		err = 0;
 		break;
 	}
-- 
2.9.5

[patch net-next 3/6] net: bridge: Export bridge multicast router state

[Jiri Pirko]

From: Yotam Gigi <yotamg@mellanox.com>

Add an access function that, given a bridge netdevice, returns whether the
bridge device is currently an mrouter or not. The function uses the already
existing br_multicast_is_router function to check that.

This function is needed in order to allow ports that join an already
existing bridge to know the current mrouter state of the bridge device.
Together with the bridge device mrouter ports switchdev notifications, it
is possible to have full offloading of the semantics of the bridge device
mcast router state.

Due to the fact that the bridge multicast router status can change in
packet RX path, take the multicast_router bridge spinlock to protect the
read.

Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
Reviewed-by: Nogah Frankel <nogahf@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/linux/if_bridge.h |  5 +++++
 net/bridge/br_multicast.c | 12 ++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index 3cd18ac..283a9be 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -63,6 +63,7 @@ int br_multicast_list_adjacent(struct net_device *dev,
 bool br_multicast_has_querier_anywhere(struct net_device *dev, int proto);
 bool br_multicast_has_querier_adjacent(struct net_device *dev, int proto);
 bool br_multicast_enabled(const struct net_device *dev);
+bool br_multicast_router(const struct net_device *dev);
 #else
 static inline int br_multicast_list_adjacent(struct net_device *dev,
 					     struct list_head *br_ip_list)
@@ -83,6 +84,10 @@ static inline bool br_multicast_enabled(const struct net_device *dev)
 {
 	return false;
 }
+static inline bool br_multicast_router(const struct net_device *dev)
+{
+	return false;
+}
 #endif
 
 #if IS_ENABLED(CONFIG_BRIDGE) && IS_ENABLED(CONFIG_BRIDGE_VLAN_FILTERING)
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 4d4fcb5..b4c98a3 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -2220,6 +2220,18 @@ bool br_multicast_enabled(const struct net_device *dev)
 }
 EXPORT_SYMBOL_GPL(br_multicast_enabled);
 
+bool br_multicast_router(const struct net_device *dev)
+{
+	struct net_bridge *br = netdev_priv(dev);
+	bool is_router;
+
+	spin_lock_bh(&br->multicast_lock);
+	is_router = br_multicast_is_router(br);
+	spin_unlock_bh(&br->multicast_lock);
+	return is_router;
+}
+EXPORT_SYMBOL_GPL(br_multicast_router);
+
 int br_multicast_set_querier(struct net_bridge *br, unsigned long val)
 {
 	unsigned long max_delay;
-- 
2.9.5

[patch net-next 4/6] mlxsw: spectrum: router: Export the mlxsw_sp_router_port function

[Jiri Pirko]

From: Yotam Gigi <yotamg@mellanox.com>

In Spectrum hardware, the router port is a virtual port that is the gateway
to the routing mechanism. Hence, in order for a packet to be L3 forwarded,
it must first be L2 forwarded to the router port inside the hardware.

Further patches in this patchset are going to introduce support in bridge
device used as an mrouter port. In this case, the router port index will be
needed in order to update the MDB entries to include the router port. Thus,
export the mlxsw_sp_router_port function, which returns the index of the
Spectrum router port.

Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
Reviewed-by: Nogah Frankel <nogahf@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 2 +-
 drivers/net/ethernet/mellanox/mlxsw/spectrum_router.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 58bc04c..58adb23 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -5947,7 +5947,7 @@ static int mlxsw_sp_rif_vlan_fid_op(struct mlxsw_sp_rif *rif,
 	return mlxsw_reg_write(mlxsw_sp->core, MLXSW_REG(ritr), ritr_pl);
 }
 
-static u8 mlxsw_sp_router_port(const struct mlxsw_sp *mlxsw_sp)
+u8 mlxsw_sp_router_port(const struct mlxsw_sp *mlxsw_sp)
 {
 	return mlxsw_core_max_ports(mlxsw_sp->core) + 1;
 }
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.h
index 3d44918..3f2d840 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.h
@@ -70,6 +70,7 @@ u16 mlxsw_sp_rif_index(const struct mlxsw_sp_rif *rif);
 u16 mlxsw_sp_ipip_lb_rif_index(const struct mlxsw_sp_rif_ipip_lb *rif);
 u16 mlxsw_sp_ipip_lb_ul_vr_id(const struct mlxsw_sp_rif_ipip_lb *rif);
 int mlxsw_sp_rif_dev_ifindex(const struct mlxsw_sp_rif *rif);
+u8 mlxsw_sp_router_port(const struct mlxsw_sp *mlxsw_sp);
 const struct net_device *mlxsw_sp_rif_dev(const struct mlxsw_sp_rif *rif);
 int mlxsw_sp_rif_counter_value_get(struct mlxsw_sp *mlxsw_sp,
 				   struct mlxsw_sp_rif *rif,
-- 
2.9.5

[patch net-next 5/6] mlxsw: spectrum_switchdev: Add support for router port in SMID entries

[Jiri Pirko]

From: Yotam Gigi <yotamg@mellanox.com>

In Spectrum, MDB entries point to MID entries, that indicate which ports a
packet should be forwarded to. Add the support in creating MID entries that
forward the packet to the Spectrum router port.

This will be later used to handle the bridge mrouter port switchdev
notifications.

Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
Reviewed-by: Nogah Frankel <nogahf@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
index 0f9eac5..092231a 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
@@ -48,6 +48,7 @@
 #include <linux/rtnetlink.h>
 #include <net/switchdev.h>
 
+#include "spectrum_router.h"
 #include "spectrum.h"
 #include "core.h"
 #include "reg.h"
@@ -1241,7 +1242,8 @@ static int mlxsw_sp_port_mdb_op(struct mlxsw_sp *mlxsw_sp, const char *addr,
 }
 
 static int mlxsw_sp_port_smid_full_entry(struct mlxsw_sp *mlxsw_sp, u16 mid_idx,
-					 long *ports_bitmap)
+					 long *ports_bitmap,
+					 bool set_router_port)
 {
 	char *smid_pl;
 	int err, i;
@@ -1256,9 +1258,15 @@ static int mlxsw_sp_port_smid_full_entry(struct mlxsw_sp *mlxsw_sp, u16 mid_idx,
 			mlxsw_reg_smid_port_mask_set(smid_pl, i, 1);
 	}
 
+	mlxsw_reg_smid_port_mask_set(smid_pl,
+				     mlxsw_sp_router_port(mlxsw_sp), 1);
+
 	for_each_set_bit(i, ports_bitmap, mlxsw_core_max_ports(mlxsw_sp->core))
 		mlxsw_reg_smid_port_set(smid_pl, i, 1);
 
+	mlxsw_reg_smid_port_set(smid_pl, mlxsw_sp_router_port(mlxsw_sp),
+				set_router_port);
+
 	err = mlxsw_reg_write(mlxsw_sp->core, MLXSW_REG(smid), smid_pl);
 	kfree(smid_pl);
 	return err;
@@ -1362,7 +1370,8 @@ mlxsw_sp_mc_write_mdb_entry(struct mlxsw_sp *mlxsw_sp,
 	mlxsw_sp_mc_get_mrouters_bitmap(flood_bitmap, bridge_device, mlxsw_sp);
 
 	mid->mid = mid_idx;
-	err = mlxsw_sp_port_smid_full_entry(mlxsw_sp, mid_idx, flood_bitmap);
+	err = mlxsw_sp_port_smid_full_entry(mlxsw_sp, mid_idx, flood_bitmap,
+					    false);
 	kfree(flood_bitmap);
 	if (err)
 		return false;
-- 
2.9.5

[patch net-next 6/6] mlxsw: spectrum_switchdev: Support bridge mrouter notifications

[Jiri Pirko]

From: Yotam Gigi <yotamg@mellanox.com>

Support the SWITCHDEV_ATTR_ID_BRIDGE_MROUTER port attribute switchdev
notification.

To do that, add the mrouter flag to struct mlxsw_sp_bridge_device, which
indicates whether the bridge device was set to be mrouter port. This field
is set when:
 - A new bridge is created, where the value is taken from the kernel
   bridge value.
 - A switchdev SWITCHDEV_ATTR_ID_BRIDGE_MROUTER notification is sent.

In addition, change the bridge MID entries to include the router port when
the bridge device is configured to be mrouter port. The MID entries are
updated in the following cases:
 - When a new MID entry is created, update the router port according to the
   bridge mrouter state.
 - When a SWITCHDEV_ATTR_ID_BRIDGE_MROUTER notification is sent, update all
   the bridge's MID entries.

This is aligned with the case where a bridge slave is configured to be
mrouter port.

Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
Reviewed-by: Nogah Frankel <nogahf@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 .../ethernet/mellanox/mlxsw/spectrum_switchdev.c   | 65 +++++++++++++++++++++-
 1 file changed, 63 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
index 092231a..15bb5f9 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
@@ -79,7 +79,8 @@ struct mlxsw_sp_bridge_device {
 	struct list_head ports_list;
 	struct list_head mids_list;
 	u8 vlan_enabled:1,
-	   multicast_enabled:1;
+	   multicast_enabled:1,
+	   mrouter:1;
 	const struct mlxsw_sp_bridge_ops *ops;
 };
 
@@ -169,6 +170,7 @@ mlxsw_sp_bridge_device_create(struct mlxsw_sp_bridge *bridge,
 	bridge_device->dev = br_dev;
 	bridge_device->vlan_enabled = vlan_enabled;
 	bridge_device->multicast_enabled = br_multicast_enabled(br_dev);
+	bridge_device->mrouter = br_multicast_router(br_dev);
 	INIT_LIST_HEAD(&bridge_device->ports_list);
 	if (vlan_enabled) {
 		bridge->vlan_enabled_exists = true;
@@ -811,6 +813,60 @@ static int mlxsw_sp_port_mc_disabled_set(struct mlxsw_sp_port *mlxsw_sp_port,
 	return 0;
 }
 
+static int mlxsw_sp_smid_router_port_set(struct mlxsw_sp *mlxsw_sp,
+					 u16 mid_idx, bool add)
+{
+	char *smid_pl;
+	int err;
+
+	smid_pl = kmalloc(MLXSW_REG_SMID_LEN, GFP_KERNEL);
+	if (!smid_pl)
+		return -ENOMEM;
+
+	mlxsw_reg_smid_pack(smid_pl, mid_idx,
+			    mlxsw_sp_router_port(mlxsw_sp), add);
+	err = mlxsw_reg_write(mlxsw_sp->core, MLXSW_REG(smid), smid_pl);
+	kfree(smid_pl);
+	return err;
+}
+
+static void
+mlxsw_sp_bridge_mrouter_update_mdb(struct mlxsw_sp *mlxsw_sp,
+				   struct mlxsw_sp_bridge_device *bridge_device,
+				   bool add)
+{
+	struct mlxsw_sp_mid *mid;
+
+	list_for_each_entry(mid, &bridge_device->mids_list, list)
+		mlxsw_sp_smid_router_port_set(mlxsw_sp, mid->mid, add);
+}
+
+static int
+mlxsw_sp_port_attr_br_mrouter_set(struct mlxsw_sp_port *mlxsw_sp_port,
+				  struct switchdev_trans *trans,
+				  struct net_device *orig_dev,
+				  bool is_mrouter)
+{
+	struct mlxsw_sp *mlxsw_sp = mlxsw_sp_port->mlxsw_sp;
+	struct mlxsw_sp_bridge_device *bridge_device;
+
+	if (switchdev_trans_ph_prepare(trans))
+		return 0;
+
+	/* It's possible we failed to enslave the port, yet this
+	 * operation is executed due to it being deferred.
+	 */
+	bridge_device = mlxsw_sp_bridge_device_find(mlxsw_sp->bridge, orig_dev);
+	if (!bridge_device)
+		return 0;
+
+	if (bridge_device->mrouter != is_mrouter)
+		mlxsw_sp_bridge_mrouter_update_mdb(mlxsw_sp, bridge_device,
+						   is_mrouter);
+	bridge_device->mrouter = is_mrouter;
+	return 0;
+}
+
 static int mlxsw_sp_port_attr_set(struct net_device *dev,
 				  const struct switchdev_attr *attr,
 				  struct switchdev_trans *trans)
@@ -848,6 +904,11 @@ static int mlxsw_sp_port_attr_set(struct net_device *dev,
 						    attr->orig_dev,
 						    attr->u.mc_disabled);
 		break;
+	case SWITCHDEV_ATTR_ID_BRIDGE_MROUTER:
+		err = mlxsw_sp_port_attr_br_mrouter_set(mlxsw_sp_port, trans,
+							attr->orig_dev,
+							attr->u.mrouter);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
@@ -1371,7 +1432,7 @@ mlxsw_sp_mc_write_mdb_entry(struct mlxsw_sp *mlxsw_sp,
 
 	mid->mid = mid_idx;
 	err = mlxsw_sp_port_smid_full_entry(mlxsw_sp, mid_idx, flood_bitmap,
-					    false);
+					    bridge_device->mrouter);
 	kfree(flood_bitmap);
 	if (err)
 		return false;
-- 
2.9.5

Re: [patch net-next 1/6] net: bridge: Use the MDB_RTR_TYPE_TEMP on bridge device too

[Nikolay Aleksandrov]

On 05/10/17 13:36, Jiri Pirko wrote:
> From: Yotam Gigi <yotamg@mellanox.com>
> 
> Every bridge port is in one of four mcast router port states:
>  - MDB_RTR_TYPE_PERM - the port is set by the user to be an mrouter port
>    regardless of IGMP queries.
>  - MDB_RTR_TYPE_DISABLED - the port is set by the user to not be an mrouter
>    port regardless of IGMP queries.
>  - MDB_RTR_TYPE_TEMP - the port is set by the user to be in mcast router
>    learning state, but currently it is not an mrouter port as no IGMP query
>    has been received by it for the last multicast_querier_interval.
>  - MDB_RTR_TYPE_TEMP_QUERY - the port is set by the user to be in mcast
>    router learning state, and currently it is an mrouter port due to an
>    IGMP query that has been received by it during the passed
>    multicast_querier_interval.

I think you got the last two partially mixed up, MDB_RTR_TYPE_TEMP marks the port as a router
regardless if there were any igmp queries, while TYPE_TEMP_QUERY means it's in learning
state. It is the timer (armed vs not) that defines if currently the port is a router
when one of the TEMP/TEMP_QUERY are set. In the _TEMP case it is always armed as it
is refreshed by user or igmp queries which was the point of that mode.
So this means in br_multicast_router() just check for the timer_pending or perm mode.

In the port code you have the following transitions:
 _TEMP -> TEMP_QUERY (on timer fire or user-set val, port becomes learning only)
 _TEMP -> _TEMP (noop on user refresh or igmp query, timer refreshes)
 _TEMP_QUERY -> _TEMP_QUERY (on igmp query the timer is armed, port becomes router)

you never have _TEMP_QUERY -> _TEMP, which you're using here to denote the timer
getting armed and the bridge becoming a router.

> 
> The bridge device (brX) itself can also be configured by the user to be
> either fixed, disabled or learning mrouter port states, but currently there
> is no distinction between the MDB_RTR_TYPE_TEMP_QUERY and MDB_RTR_TYPE_TEMP
> in the bridge internal state. Due to that, when an IGMP query is received,
> it is not straightforward to tell whether it changes the bridge device
> mrouter port status or not.

But before this patch the bridge device could not get that set.

> 
> Further patches in this patch-set will introduce notifications upon the
> bridge device mrouter port state. In order to prevent resending bridge
> mrouter notification when it is not needed, such distinction is necessary.
> 

Granted the bridge device hasn't got a way to clearly distinguish the transitions
without the chance for a race and if using the timer one could get an unnecessary
notification but that seems like a corner case when the timer fires exactly at the
same time as the igmp query is received. Can't it be handled by just checking if
the new state is different in the notification receiver ?
If it can't and is a problem then I'd prefer to add a new boolean to denote that
router on/off transition rather than doing this.

> Hence, add the distinction between MDB_RTR_TYPE_TEMP and
> MDB_RTR_TYPE_TEMP_QUERY states for the bridge device, similarly to any
> other bridge port.
> 

This does not add proper MDB_RTR_TYPE_TEMP support for the bridge device
but seems to abuse it to distinguish the timer state, and changes
the meaning of MDB_RTR_TYPE_TEMP. Can't you just use the timer instead ?
I think it will simplify the set and avoid all of this.

> In order to not break the current kernel-user API, don't propagate the new
> state to the user and use it only in the bridge internal state. Thus, if
> the user reads (either via sysfs or netlink) the bridge device mrouter
> state, he will get the MDB_RTR_TYPE_TEMP_QUERY state even if the current
> bridge state is MDB_RTR_TYPE_TEMP.
> 
> Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
> Reviewed-by: Nogah Frankel <nogahf@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  net/bridge/br_multicast.c | 25 +++++++++++++++++++++----
>  net/bridge/br_netlink.c   |  3 ++-
>  net/bridge/br_private.h   | 13 ++++++++++---
>  net/bridge/br_sysfs_br.c  |  3 ++-
>  4 files changed, 35 insertions(+), 9 deletions(-)
> 
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 8dc5c8d..b86307b 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -861,6 +861,17 @@ static void br_multicast_router_expired(unsigned long data)
>  
>  static void br_multicast_local_router_expired(unsigned long data)
>  {
> +	struct net_bridge *br = (struct net_bridge *) data;
> +
> +	spin_lock(&br->multicast_lock);
> +	if (br->multicast_router == MDB_RTR_TYPE_DISABLED ||
> +	    br->multicast_router == MDB_RTR_TYPE_PERM ||
> +	    timer_pending(&br->multicast_router_timer))
> +		goto out;
> +
> +	br->multicast_router = MDB_RTR_TYPE_TEMP_QUERY;
> +out:
> +	spin_unlock(&br->multicast_lock);
>  }
>  
>  static void br_multicast_querier_expired(struct net_bridge *br,
> @@ -1364,9 +1375,12 @@ static void br_multicast_mark_router(struct net_bridge *br,
>  	unsigned long now = jiffies;
>  
>  	if (!port) {
> -		if (br->multicast_router == MDB_RTR_TYPE_TEMP_QUERY)
> +		if (br->multicast_router == MDB_RTR_TYPE_TEMP_QUERY ||
> +		    br->multicast_router == MDB_RTR_TYPE_TEMP) {
>  			mod_timer(&br->multicast_router_timer,
>  				  now + br->multicast_querier_interval);
> +			br->multicast_router = MDB_RTR_TYPE_TEMP;
> +		}
>  		return;
>  	}
>  
> @@ -1952,7 +1966,7 @@ void br_multicast_init(struct net_bridge *br)
>  
>  	spin_lock_init(&br->multicast_lock);
>  	setup_timer(&br->multicast_router_timer,
> -		    br_multicast_local_router_expired, 0);
> +		    br_multicast_local_router_expired, (unsigned long)br);
>  	setup_timer(&br->ip4_other_query.timer,
>  		    br_ip4_multicast_querier_expired, (unsigned long)br);
>  	setup_timer(&br->ip4_own_query.timer, br_ip4_multicast_query_expired,
> @@ -2043,11 +2057,14 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
>  	case MDB_RTR_TYPE_DISABLED:
>  	case MDB_RTR_TYPE_PERM:
>  		del_timer(&br->multicast_router_timer);
> -		/* fall through */
> -	case MDB_RTR_TYPE_TEMP_QUERY:
>  		br->multicast_router = val;
>  		err = 0;
>  		break;
> +	case MDB_RTR_TYPE_TEMP_QUERY:
> +		if (br->multicast_router != MDB_RTR_TYPE_TEMP)
> +			br->multicast_router = val;
> +		err = 0;
> +		break;
>  	}
>  
>  	spin_unlock_bh(&br->multicast_lock);
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index dea88a2..cee5016 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -1357,7 +1357,8 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
>  		return -EMSGSIZE;
>  #endif
>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> -	if (nla_put_u8(skb, IFLA_BR_MCAST_ROUTER, br->multicast_router) ||
> +	if (nla_put_u8(skb, IFLA_BR_MCAST_ROUTER,
> +		       br_multicast_router_translate(br->multicast_router)) ||
>  	    nla_put_u8(skb, IFLA_BR_MCAST_SNOOPING, !br->multicast_disabled) ||
>  	    nla_put_u8(skb, IFLA_BR_MCAST_QUERY_USE_IFADDR,
>  		       br->multicast_query_use_ifaddr) ||
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index ab4df24..e6e3fec 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -649,9 +649,8 @@ void br_multicast_get_stats(const struct net_bridge *br,
>  
>  static inline bool br_multicast_is_router(struct net_bridge *br)
>  {
> -	return br->multicast_router == 2 ||
> -	       (br->multicast_router == 1 &&
> -		timer_pending(&br->multicast_router_timer));
> +	return br->multicast_router == MDB_RTR_TYPE_PERM ||
> +	       br->multicast_router == MDB_RTR_TYPE_TEMP;
>  }
>  
>  static inline bool
> @@ -790,6 +789,14 @@ static inline int br_multicast_igmp_type(const struct sk_buff *skb)
>  }
>  #endif
>  
> +static inline unsigned char

u8

> +br_multicast_router_translate(unsigned char multicast_router)

u8, if need be change the type of the struct member

> +{
> +	if (multicast_router == MDB_RTR_TYPE_TEMP)
> +		return MDB_RTR_TYPE_TEMP_QUERY;
> +	return multicast_router;
> +}
> +
>  /* br_vlan.c */
>  #ifdef CONFIG_BRIDGE_VLAN_FILTERING
>  bool br_allowed_ingress(const struct net_bridge *br,
> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
> index 723f25e..9b9c597 100644
> --- a/net/bridge/br_sysfs_br.c
> +++ b/net/bridge/br_sysfs_br.c
> @@ -340,7 +340,8 @@ static ssize_t multicast_router_show(struct device *d,
>  				     struct device_attribute *attr, char *buf)
>  {
>  	struct net_bridge *br = to_bridge(d);
> -	return sprintf(buf, "%d\n", br->multicast_router);
> +	return sprintf(buf, "%d\n",
> +		       br_multicast_router_translate(br->multicast_router));
>  }
>  
>  static ssize_t multicast_router_store(struct device *d,
> 

Re: [patch net-next 1/6] net: bridge: Use the MDB_RTR_TYPE_TEMP on bridge device too

[Nikolay Aleksandrov]

On 05/10/17 15:09, Nikolay Aleksandrov wrote:
> On 05/10/17 13:36, Jiri Pirko wrote:
>> From: Yotam Gigi <yotamg@mellanox.com>
>>
>> Every bridge port is in one of four mcast router port states:
>>  - MDB_RTR_TYPE_PERM - the port is set by the user to be an mrouter port
>>    regardless of IGMP queries.
>>  - MDB_RTR_TYPE_DISABLED - the port is set by the user to not be an mrouter
>>    port regardless of IGMP queries.
>>  - MDB_RTR_TYPE_TEMP - the port is set by the user to be in mcast router
>>    learning state, but currently it is not an mrouter port as no IGMP query
>>    has been received by it for the last multicast_querier_interval.
>>  - MDB_RTR_TYPE_TEMP_QUERY - the port is set by the user to be in mcast
>>    router learning state, and currently it is an mrouter port due to an
>>    IGMP query that has been received by it during the passed
>>    multicast_querier_interval.
> 
> I think you got the last two partially mixed up, MDB_RTR_TYPE_TEMP marks the port as a router
> regardless if there were any igmp queries, while TYPE_TEMP_QUERY means it's in learning
> state. It is the timer (armed vs not) that defines if currently the port is a router
> when one of the TEMP/TEMP_QUERY are set. In the _TEMP case it is always armed as it
> is refreshed by user or igmp queries which was the point of that mode.
> So this means in br_multicast_router() just check for the timer_pending or perm mode.
> 
> In the port code you have the following transitions:
>  _TEMP -> TEMP_QUERY (on timer fire or user-set val, port becomes learning only)
>  _TEMP -> _TEMP (noop on user refresh or igmp query, timer refreshes)
>  _TEMP_QUERY -> _TEMP_QUERY (on igmp query the timer is armed, port becomes router)
> 
> you never have _TEMP_QUERY -> _TEMP, which you're using here to denote the timer
> getting armed and the bridge becoming a router.

Okay, technically the user can change the mode in such way manually. But it is not done
automatically is what I was trying to say.

> 
>>
>> The bridge device (brX) itself can also be configured by the user to be
>> either fixed, disabled or learning mrouter port states, but currently there
>> is no distinction between the MDB_RTR_TYPE_TEMP_QUERY and MDB_RTR_TYPE_TEMP
>> in the bridge internal state. Due to that, when an IGMP query is received,
>> it is not straightforward to tell whether it changes the bridge device
>> mrouter port status or not.
> 
> But before this patch the bridge device could not get that set.
> 
>>
>> Further patches in this patch-set will introduce notifications upon the
>> bridge device mrouter port state. In order to prevent resending bridge
>> mrouter notification when it is not needed, such distinction is necessary.
>>
> 
> Granted the bridge device hasn't got a way to clearly distinguish the transitions
> without the chance for a race and if using the timer one could get an unnecessary
> notification but that seems like a corner case when the timer fires exactly at the
> same time as the igmp query is received. Can't it be handled by just checking if
> the new state is different in the notification receiver ?
> If it can't and is a problem then I'd prefer to add a new boolean to denote that
> router on/off transition rather than doing this.
> 
>> Hence, add the distinction between MDB_RTR_TYPE_TEMP and
>> MDB_RTR_TYPE_TEMP_QUERY states for the bridge device, similarly to any
>> other bridge port.
>>
> 
> This does not add proper MDB_RTR_TYPE_TEMP support for the bridge device
> but seems to abuse it to distinguish the timer state, and changes
> the meaning of MDB_RTR_TYPE_TEMP. Can't you just use the timer instead ?
> I think it will simplify the set and avoid all of this.
> 
>> In order to not break the current kernel-user API, don't propagate the new
>> state to the user and use it only in the bridge internal state. Thus, if
>> the user reads (either via sysfs or netlink) the bridge device mrouter
>> state, he will get the MDB_RTR_TYPE_TEMP_QUERY state even if the current
>> bridge state is MDB_RTR_TYPE_TEMP.
>>
>> Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
>> Reviewed-by: Nogah Frankel <nogahf@mellanox.com>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>>  net/bridge/br_multicast.c | 25 +++++++++++++++++++++----
>>  net/bridge/br_netlink.c   |  3 ++-
>>  net/bridge/br_private.h   | 13 ++++++++++---
>>  net/bridge/br_sysfs_br.c  |  3 ++-
>>  4 files changed, 35 insertions(+), 9 deletions(-)
>>
>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>> index 8dc5c8d..b86307b 100644
>> --- a/net/bridge/br_multicast.c
>> +++ b/net/bridge/br_multicast.c
>> @@ -861,6 +861,17 @@ static void br_multicast_router_expired(unsigned long data)
>>  
>>  static void br_multicast_local_router_expired(unsigned long data)
>>  {
>> +	struct net_bridge *br = (struct net_bridge *) data;
>> +
>> +	spin_lock(&br->multicast_lock);
>> +	if (br->multicast_router == MDB_RTR_TYPE_DISABLED ||
>> +	    br->multicast_router == MDB_RTR_TYPE_PERM ||
>> +	    timer_pending(&br->multicast_router_timer))
>> +		goto out;
>> +
>> +	br->multicast_router = MDB_RTR_TYPE_TEMP_QUERY;
>> +out:
>> +	spin_unlock(&br->multicast_lock);
>>  }
>>  
>>  static void br_multicast_querier_expired(struct net_bridge *br,
>> @@ -1364,9 +1375,12 @@ static void br_multicast_mark_router(struct net_bridge *br,
>>  	unsigned long now = jiffies;
>>  
>>  	if (!port) {
>> -		if (br->multicast_router == MDB_RTR_TYPE_TEMP_QUERY)
>> +		if (br->multicast_router == MDB_RTR_TYPE_TEMP_QUERY ||
>> +		    br->multicast_router == MDB_RTR_TYPE_TEMP) {
>>  			mod_timer(&br->multicast_router_timer,
>>  				  now + br->multicast_querier_interval);
>> +			br->multicast_router = MDB_RTR_TYPE_TEMP;
>> +		}
>>  		return;
>>  	}
>>  
>> @@ -1952,7 +1966,7 @@ void br_multicast_init(struct net_bridge *br)
>>  
>>  	spin_lock_init(&br->multicast_lock);
>>  	setup_timer(&br->multicast_router_timer,
>> -		    br_multicast_local_router_expired, 0);
>> +		    br_multicast_local_router_expired, (unsigned long)br);
>>  	setup_timer(&br->ip4_other_query.timer,
>>  		    br_ip4_multicast_querier_expired, (unsigned long)br);
>>  	setup_timer(&br->ip4_own_query.timer, br_ip4_multicast_query_expired,
>> @@ -2043,11 +2057,14 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
>>  	case MDB_RTR_TYPE_DISABLED:
>>  	case MDB_RTR_TYPE_PERM:
>>  		del_timer(&br->multicast_router_timer);
>> -		/* fall through */
>> -	case MDB_RTR_TYPE_TEMP_QUERY:
>>  		br->multicast_router = val;
>>  		err = 0;
>>  		break;
>> +	case MDB_RTR_TYPE_TEMP_QUERY:
>> +		if (br->multicast_router != MDB_RTR_TYPE_TEMP)
>> +			br->multicast_router = val;
>> +		err = 0;
>> +		break;
>>  	}
>>  
>>  	spin_unlock_bh(&br->multicast_lock);
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index dea88a2..cee5016 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -1357,7 +1357,8 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
>>  		return -EMSGSIZE;
>>  #endif
>>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>> -	if (nla_put_u8(skb, IFLA_BR_MCAST_ROUTER, br->multicast_router) ||
>> +	if (nla_put_u8(skb, IFLA_BR_MCAST_ROUTER,
>> +		       br_multicast_router_translate(br->multicast_router)) ||
>>  	    nla_put_u8(skb, IFLA_BR_MCAST_SNOOPING, !br->multicast_disabled) ||
>>  	    nla_put_u8(skb, IFLA_BR_MCAST_QUERY_USE_IFADDR,
>>  		       br->multicast_query_use_ifaddr) ||
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index ab4df24..e6e3fec 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -649,9 +649,8 @@ void br_multicast_get_stats(const struct net_bridge *br,
>>  
>>  static inline bool br_multicast_is_router(struct net_bridge *br)
>>  {
>> -	return br->multicast_router == 2 ||
>> -	       (br->multicast_router == 1 &&
>> -		timer_pending(&br->multicast_router_timer));
>> +	return br->multicast_router == MDB_RTR_TYPE_PERM ||
>> +	       br->multicast_router == MDB_RTR_TYPE_TEMP;
>>  }
>>  
>>  static inline bool
>> @@ -790,6 +789,14 @@ static inline int br_multicast_igmp_type(const struct sk_buff *skb)
>>  }
>>  #endif
>>  
>> +static inline unsigned char
> 
> u8
> 
>> +br_multicast_router_translate(unsigned char multicast_router)
> 
> u8, if need be change the type of the struct member
> 
>> +{
>> +	if (multicast_router == MDB_RTR_TYPE_TEMP)
>> +		return MDB_RTR_TYPE_TEMP_QUERY;
>> +	return multicast_router;
>> +}
>> +
>>  /* br_vlan.c */
>>  #ifdef CONFIG_BRIDGE_VLAN_FILTERING
>>  bool br_allowed_ingress(const struct net_bridge *br,
>> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
>> index 723f25e..9b9c597 100644
>> --- a/net/bridge/br_sysfs_br.c
>> +++ b/net/bridge/br_sysfs_br.c
>> @@ -340,7 +340,8 @@ static ssize_t multicast_router_show(struct device *d,
>>  				     struct device_attribute *attr, char *buf)
>>  {
>>  	struct net_bridge *br = to_bridge(d);
>> -	return sprintf(buf, "%d\n", br->multicast_router);
>> +	return sprintf(buf, "%d\n",
>> +		       br_multicast_router_translate(br->multicast_router));
>>  }
>>  
>>  static ssize_t multicast_router_store(struct device *d,
>>
> 

Re: [patch net-next 1/6] net: bridge: Use the MDB_RTR_TYPE_TEMP on bridge device too

[Nikolay Aleksandrov]

On 05/10/17 15:09, Nikolay Aleksandrov wrote:
> On 05/10/17 13:36, Jiri Pirko wrote:
>> From: Yotam Gigi <yotamg@mellanox.com>
>>
>> Every bridge port is in one of four mcast router port states:
>>  - MDB_RTR_TYPE_PERM - the port is set by the user to be an mrouter port
>>    regardless of IGMP queries.
>>  - MDB_RTR_TYPE_DISABLED - the port is set by the user to not be an mrouter
>>    port regardless of IGMP queries.
>>  - MDB_RTR_TYPE_TEMP - the port is set by the user to be in mcast router
>>    learning state, but currently it is not an mrouter port as no IGMP query
>>    has been received by it for the last multicast_querier_interval.
>>  - MDB_RTR_TYPE_TEMP_QUERY - the port is set by the user to be in mcast
>>    router learning state, and currently it is an mrouter port due to an
>>    IGMP query that has been received by it during the passed
>>    multicast_querier_interval.
> 
> I think you got the last two partially mixed up, MDB_RTR_TYPE_TEMP marks the port as a router
> regardless if there were any igmp queries, while TYPE_TEMP_QUERY means it's in learning
> state. It is the timer (armed vs not) that defines if currently the port is a router
> when one of the TEMP/TEMP_QUERY are set. In the _TEMP case it is always armed as it
> is refreshed by user or igmp queries which was the point of that mode.
> So this means in br_multicast_router() just check for the timer_pending or perm mode.
> 
> In the port code you have the following transitions:
>  _TEMP -> TEMP_QUERY (on timer fire or user-set val, port becomes learning only)
>  _TEMP -> _TEMP (noop on user refresh or igmp query, timer refreshes)
>  _TEMP_QUERY -> _TEMP_QUERY (on igmp query the timer is armed, port becomes router)
> 
> you never have _TEMP_QUERY -> _TEMP, which you're using here to denote the timer
> getting armed and the bridge becoming a router.
> 
>>
>> The bridge device (brX) itself can also be configured by the user to be
>> either fixed, disabled or learning mrouter port states, but currently there
>> is no distinction between the MDB_RTR_TYPE_TEMP_QUERY and MDB_RTR_TYPE_TEMP
>> in the bridge internal state. Due to that, when an IGMP query is received,
>> it is not straightforward to tell whether it changes the bridge device
>> mrouter port status or not.
> 
> But before this patch the bridge device could not get that set.
> 
>>
>> Further patches in this patch-set will introduce notifications upon the
>> bridge device mrouter port state. In order to prevent resending bridge
>> mrouter notification when it is not needed, such distinction is necessary.
>>
> 
> Granted the bridge device hasn't got a way to clearly distinguish the transitions
> without the chance for a race and if using the timer one could get an unnecessary
> notification but that seems like a corner case when the timer fires exactly at the
> same time as the igmp query is received. Can't it be handled by just checking if
> the new state is different in the notification receiver ?

Scratch the sentence below, on a second thought I'd prefer to stick with this
version if it's a problem. :-)

> If it can't and is a problem then I'd prefer to add a new boolean to denote that
> router on/off transition rather than doing this.
> 
>> Hence, add the distinction between MDB_RTR_TYPE_TEMP and
>> MDB_RTR_TYPE_TEMP_QUERY states for the bridge device, similarly to any
>> other bridge port.
>>
> 
> This does not add proper MDB_RTR_TYPE_TEMP support for the bridge device
> but seems to abuse it to distinguish the timer state, and changes
> the meaning of MDB_RTR_TYPE_TEMP. Can't you just use the timer instead ?
> I think it will simplify the set and avoid all of this.
> 
>> In order to not break the current kernel-user API, don't propagate the new
>> state to the user and use it only in the bridge internal state. Thus, if
>> the user reads (either via sysfs or netlink) the bridge device mrouter
>> state, he will get the MDB_RTR_TYPE_TEMP_QUERY state even if the current
>> bridge state is MDB_RTR_TYPE_TEMP.
>>
[snip]

Re: [patch net-next 3/6] net: bridge: Export bridge multicast router state

[Nikolay Aleksandrov]

On 05/10/17 13:36, Jiri Pirko wrote:
> From: Yotam Gigi <yotamg@mellanox.com>
> 
> Add an access function that, given a bridge netdevice, returns whether the
> bridge device is currently an mrouter or not. The function uses the already
> existing br_multicast_is_router function to check that.
> 
> This function is needed in order to allow ports that join an already
> existing bridge to know the current mrouter state of the bridge device.
> Together with the bridge device mrouter ports switchdev notifications, it
> is possible to have full offloading of the semantics of the bridge device
> mcast router state.
> 
> Due to the fact that the bridge multicast router status can change in
> packet RX path, take the multicast_router bridge spinlock to protect the
> read.
> 
> Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
> Reviewed-by: Nogah Frankel <nogahf@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  include/linux/if_bridge.h |  5 +++++
>  net/bridge/br_multicast.c | 12 ++++++++++++
>  2 files changed, 17 insertions(+)
> 

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

Re: [patch net-next 2/6] net: bridge: Notify on bridge device mrouter state changes

[Nikolay Aleksandrov]

On 05/10/17 13:36, Jiri Pirko wrote:
> From: Yotam Gigi <yotamg@mellanox.com>
> 
> Add the SWITCHDEV_ATTR_ID_BRIDGE_MROUTER switchdev notification type, used
> to indicate whether the bridge is or isn't mrouter. Notify when the bridge
> changes its state, similarly to the already existing bridged port mrouter
> notifications.
> 
> The notification uses the switchdev_attr.u.mrouter boolean flag to indicate
> the current bridge mrouter status. Thus, it only indicates whether the
> bridge is currently used as an mrouter or not, and does not indicate the
> exact mrouter state of the bridge (learning, permanent, etc.).
> 
> Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
> Reviewed-by: Nogah Frankel <nogahf@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  include/net/switchdev.h   |  1 +
>  net/bridge/br_multicast.c | 21 ++++++++++++++++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 

LGTM, but if we switch to using the timer state it will need some adjustment.
Anyway for this version,

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

Re: [patch net-next 0/6] mlxsw: Offload bridge device mrouter

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Thu,  5 Oct 2017 12:36:36 +0200

> From: Jiri Pirko <jiri@mellanox.com>
> 
> Yotam says:
> 
> Similarly to a bridged port, the bridge device itself can be configured by
> the user to be an mrouter port. In this case, all multicast traffic should
> be forwarded to it. Make the mlxsw Spectrum driver offload these directives
> to the Spectrum hardware.
> 
> Patches 1-3 add a new switchdev notification for bridge device mrouter
> port status and make the bridge module notify about it.
> 
> Patches 4-6 change the mlxsw Spectrum driver to handle these notifications
> by adding the Spectrum router port to the bridge MDB entries.

It looks like Nikolay wants some feedback addressed for patch #1.

Thanks.

Re: [patch net-next 1/6] net: bridge: Use the MDB_RTR_TYPE_TEMP on bridge device too

[Yotam Gigi]

On 10/05/2017 03:09 PM, Nikolay Aleksandrov wrote:
> On 05/10/17 13:36, Jiri Pirko wrote:
>> From: Yotam Gigi <yotamg@mellanox.com>
>>
>> Every bridge port is in one of four mcast router port states:
>>  - MDB_RTR_TYPE_PERM - the port is set by the user to be an mrouter port
>>    regardless of IGMP queries.
>>  - MDB_RTR_TYPE_DISABLED - the port is set by the user to not be an mrouter
>>    port regardless of IGMP queries.
>>  - MDB_RTR_TYPE_TEMP - the port is set by the user to be in mcast router
>>    learning state, but currently it is not an mrouter port as no IGMP query
>>    has been received by it for the last multicast_querier_interval.
>>  - MDB_RTR_TYPE_TEMP_QUERY - the port is set by the user to be in mcast
>>    router learning state, and currently it is an mrouter port due to an
>>    IGMP query that has been received by it during the passed
>>    multicast_querier_interval.
> I think you got the last two partially mixed up, MDB_RTR_TYPE_TEMP marks the port as a router
> regardless if there were any igmp queries, while TYPE_TEMP_QUERY means it's in learning
> state. It is the timer (armed vs not) that defines if currently the port is a router
> when one of the TEMP/TEMP_QUERY are set. In the _TEMP case it is always armed as it
> is refreshed by user or igmp queries which was the point of that mode.
> So this means in br_multicast_router() just check for the timer_pending or perm mode.


As much as I tried to make this clear, it seems like I failed :)

The 4 states I described are currently the "bridged port" states, not the
"bridge device" state. A bridged port has these 4 states, all can be set by the
user, while the bridge device only uses 3 of these states. This patch makes the
bridge device use the 4 states too. I thought it makes sense.

The first paragraph describes the current states of a bridged port, and the
second one explains the difference between bridged port and bridge device. I
will (try to) make it clearer if we agree on resending this patch.

Is it clearer now?


>
> In the port code you have the following transitions:
>  _TEMP -> TEMP_QUERY (on timer fire or user-set val, port becomes learning only)
>  _TEMP -> _TEMP (noop on user refresh or igmp query, timer refreshes)
>  _TEMP_QUERY -> _TEMP_QUERY (on igmp query the timer is armed, port becomes router)
>
> you never have _TEMP_QUERY -> _TEMP, which you're using here to denote the timer
> getting armed and the bridge becoming a router.


I am not sure I got this one. I do address that: when an IGMP query is recieved
and the current state is _TEMP_QUERY, I arm the timer and set the state to
_TEMP. I marked that place on the patch, so you can see below.


>
>> The bridge device (brX) itself can also be configured by the user to be
>> either fixed, disabled or learning mrouter port states, but currently there
>> is no distinction between the MDB_RTR_TYPE_TEMP_QUERY and MDB_RTR_TYPE_TEMP
>> in the bridge internal state. Due to that, when an IGMP query is received,
>> it is not straightforward to tell whether it changes the bridge device
>> mrouter port status or not.
> But before this patch the bridge device could not get that set.
>
>> Further patches in this patch-set will introduce notifications upon the
>> bridge device mrouter port state. In order to prevent resending bridge
>> mrouter notification when it is not needed, such distinction is necessary.
>>
> Granted the bridge device hasn't got a way to clearly distinguish the transitions
> without the chance for a race and if using the timer one could get an unnecessary

Is there a race condition?

> notification but that seems like a corner case when the timer fires exactly at the
> same time as the igmp query is received. Can't it be handled by just checking if
> the new state is different in the notification receiver ?
> If it can't and is a problem then I'd prefer to add a new boolean to denote that
> router on/off transition rather than doing this.
>
>> Hence, add the distinction between MDB_RTR_TYPE_TEMP and
>> MDB_RTR_TYPE_TEMP_QUERY states for the bridge device, similarly to any
>> other bridge port.
>>
> This does not add proper MDB_RTR_TYPE_TEMP support for the bridge device
> but seems to abuse it to distinguish the timer state, and changes
> the meaning of MDB_RTR_TYPE_TEMP. Can't you just use the timer instead ?
> I think it will simplify the set and avoid all of this.
>
>> In order to not break the current kernel-user API, don't propagate the new
>> state to the user and use it only in the bridge internal state. Thus, if
>> the user reads (either via sysfs or netlink) the bridge device mrouter
>> state, he will get the MDB_RTR_TYPE_TEMP_QUERY state even if the current
>> bridge state is MDB_RTR_TYPE_TEMP.
>>
>> Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
>> Reviewed-by: Nogah Frankel <nogahf@mellanox.com>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>>  net/bridge/br_multicast.c | 25 +++++++++++++++++++++----
>>  net/bridge/br_netlink.c   |  3 ++-
>>  net/bridge/br_private.h   | 13 ++++++++++---
>>  net/bridge/br_sysfs_br.c  |  3 ++-
>>  4 files changed, 35 insertions(+), 9 deletions(-)
>>
>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>> index 8dc5c8d..b86307b 100644
>> --- a/net/bridge/br_multicast.c
>> +++ b/net/bridge/br_multicast.c
>> @@ -861,6 +861,17 @@ static void br_multicast_router_expired(unsigned long data)
>>  
>>  static void br_multicast_local_router_expired(unsigned long data)
>>  {
>> +	struct net_bridge *br = (struct net_bridge *) data;
>> +
>> +	spin_lock(&br->multicast_lock);
>> +	if (br->multicast_router == MDB_RTR_TYPE_DISABLED ||
>> +	    br->multicast_router == MDB_RTR_TYPE_PERM ||
>> +	    timer_pending(&br->multicast_router_timer))
>> +		goto out;
>> +
>> +	br->multicast_router = MDB_RTR_TYPE_TEMP_QUERY;
>> +out:
>> +	spin_unlock(&br->multicast_lock);
>>  }
>>  
>>  static void br_multicast_querier_expired(struct net_bridge *br,
>> @@ -1364,9 +1375,12 @@ static void br_multicast_mark_router(struct net_bridge *br,
>>  	unsigned long now = jiffies;
>>  
>>  	if (!port) {
>> -		if (br->multicast_router == MDB_RTR_TYPE_TEMP_QUERY)
>> +		if (br->multicast_router == MDB_RTR_TYPE_TEMP_QUERY ||
>> +		    br->multicast_router == MDB_RTR_TYPE_TEMP) {
>>  			mod_timer(&br->multicast_router_timer,
>>  				  now + br->multicast_querier_interval);
>> +			br->multicast_router = MDB_RTR_TYPE_TEMP;
>> +		}


These are the transitions:
_TEMP -> _TEMP_QUERY
_TEMP_QUERY -> _TEMP_QUERY

In both transitions the timer is armed.


>>  		return;
>>  	}
>>  
>> @@ -1952,7 +1966,7 @@ void br_multicast_init(struct net_bridge *br)
>>  
>>  	spin_lock_init(&br->multicast_lock);
>>  	setup_timer(&br->multicast_router_timer,
>> -		    br_multicast_local_router_expired, 0);
>> +		    br_multicast_local_router_expired, (unsigned long)br);
>>  	setup_timer(&br->ip4_other_query.timer,
>>  		    br_ip4_multicast_querier_expired, (unsigned long)br);
>>  	setup_timer(&br->ip4_own_query.timer, br_ip4_multicast_query_expired,
>> @@ -2043,11 +2057,14 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
>>  	case MDB_RTR_TYPE_DISABLED:
>>  	case MDB_RTR_TYPE_PERM:
>>  		del_timer(&br->multicast_router_timer);
>> -		/* fall through */
>> -	case MDB_RTR_TYPE_TEMP_QUERY:
>>  		br->multicast_router = val;
>>  		err = 0;
>>  		break;
>> +	case MDB_RTR_TYPE_TEMP_QUERY:
>> +		if (br->multicast_router != MDB_RTR_TYPE_TEMP)
>> +			br->multicast_router = val;
>> +		err = 0;
>> +		break;
>>  	}
>>  
>>  	spin_unlock_bh(&br->multicast_lock);
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index dea88a2..cee5016 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -1357,7 +1357,8 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
>>  		return -EMSGSIZE;
>>  #endif
>>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>> -	if (nla_put_u8(skb, IFLA_BR_MCAST_ROUTER, br->multicast_router) ||
>> +	if (nla_put_u8(skb, IFLA_BR_MCAST_ROUTER,
>> +		       br_multicast_router_translate(br->multicast_router)) ||
>>  	    nla_put_u8(skb, IFLA_BR_MCAST_SNOOPING, !br->multicast_disabled) ||
>>  	    nla_put_u8(skb, IFLA_BR_MCAST_QUERY_USE_IFADDR,
>>  		       br->multicast_query_use_ifaddr) ||
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index ab4df24..e6e3fec 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -649,9 +649,8 @@ void br_multicast_get_stats(const struct net_bridge *br,
>>  
>>  static inline bool br_multicast_is_router(struct net_bridge *br)
>>  {
>> -	return br->multicast_router == 2 ||
>> -	       (br->multicast_router == 1 &&
>> -		timer_pending(&br->multicast_router_timer));
>> +	return br->multicast_router == MDB_RTR_TYPE_PERM ||
>> +	       br->multicast_router == MDB_RTR_TYPE_TEMP;
>>  }
>>  
>>  static inline bool
>> @@ -790,6 +789,14 @@ static inline int br_multicast_igmp_type(const struct sk_buff *skb)
>>  }
>>  #endif
>>  
>> +static inline unsigned char
> u8
>
>> +br_multicast_router_translate(unsigned char multicast_router)
> u8, if need be change the type of the struct member


Sorry, didn't quite get that. Currently, both the bridge_port and bridge have
the multicast_router field, and in both cases it is of type unsigned char. Do
you suggest to change both to be "u8"?


>
>> +{
>> +	if (multicast_router == MDB_RTR_TYPE_TEMP)
>> +		return MDB_RTR_TYPE_TEMP_QUERY;
>> +	return multicast_router;
>> +}
>> +
>>  /* br_vlan.c */
>>  #ifdef CONFIG_BRIDGE_VLAN_FILTERING
>>  bool br_allowed_ingress(const struct net_bridge *br,
>> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
>> index 723f25e..9b9c597 100644
>> --- a/net/bridge/br_sysfs_br.c
>> +++ b/net/bridge/br_sysfs_br.c
>> @@ -340,7 +340,8 @@ static ssize_t multicast_router_show(struct device *d,
>>  				     struct device_attribute *attr, char *buf)
>>  {
>>  	struct net_bridge *br = to_bridge(d);
>> -	return sprintf(buf, "%d\n", br->multicast_router);
>> +	return sprintf(buf, "%d\n",
>> +		       br_multicast_router_translate(br->multicast_router));
>>  }
>>  
>>  static ssize_t multicast_router_store(struct device *d,
>>

Re: [patch net-next 1/6] net: bridge: Use the MDB_RTR_TYPE_TEMP on bridge device too

[Nikolay Aleksandrov]

On 08/10/17 08:23, Yotam Gigi wrote:
> On 10/05/2017 03:09 PM, Nikolay Aleksandrov wrote:
>> On 05/10/17 13:36, Jiri Pirko wrote:
>>> From: Yotam Gigi <yotamg@mellanox.com>
>>>
>>> Every bridge port is in one of four mcast router port states:
>>>  - MDB_RTR_TYPE_PERM - the port is set by the user to be an mrouter port
>>>    regardless of IGMP queries.
>>>  - MDB_RTR_TYPE_DISABLED - the port is set by the user to not be an mrouter
>>>    port regardless of IGMP queries.
>>>  - MDB_RTR_TYPE_TEMP - the port is set by the user to be in mcast router
>>>    learning state, but currently it is not an mrouter port as no IGMP query
>>>    has been received by it for the last multicast_querier_interval.
>>>  - MDB_RTR_TYPE_TEMP_QUERY - the port is set by the user to be in mcast
>>>    router learning state, and currently it is an mrouter port due to an
>>>    IGMP query that has been received by it during the passed
>>>    multicast_querier_interval.
>> I think you got the last two partially mixed up, MDB_RTR_TYPE_TEMP marks the port as a router
>> regardless if there were any igmp queries, while TYPE_TEMP_QUERY means it's in learning
>> state. It is the timer (armed vs not) that defines if currently the port is a router
>> when one of the TEMP/TEMP_QUERY are set. In the _TEMP case it is always armed as it
>> is refreshed by user or igmp queries which was the point of that mode.
>> So this means in br_multicast_router() just check for the timer_pending or perm mode.
> 
> 
> As much as I tried to make this clear, it seems like I failed :)
> 
> The 4 states I described are currently the "bridged port" states, not the
> "bridge device" state. A bridged port has these 4 states, all can be set by the
> user, while the bridge device only uses 3 of these states. This patch makes the
> bridge device use the 4 states too. I thought it makes sense.

(disclaimer: this is all about bridge ports, not bridge device)
Right, I'll try to explain again: _TEMP always marks the port as a mcast router,
it does not put it into just learning state waiting for an igmp query and it can
be refreshed by either a query or the user again setting the port in _TEMP.
While _TEMP_QUERY puts the port in learning state waiting for a query to become
a router, and _TEMP downgrades to _TEMP_QUERY if it expires.

Does that make it clearer ?

so for _TEMP you say:
>>>  - MDB_RTR_TYPE_TEMP - the port is set by the user to be in mcast router
>>>    learning state, but currently it is not an mrouter port as no IGMP query
>>>    has been received by it for the last multicast_querier_interval.

which is not the case, it is always a router when that mode is set on a port.
Same for _TEMP_QUERY.

> 
> The first paragraph describes the current states of a bridged port, and the
> second one explains the difference between bridged port and bridge device. I
> will (try to) make it clearer if we agree on resending this patch.
> 
> Is it clearer now?
> 
> 
>>
>> In the port code you have the following transitions:
>>  _TEMP -> TEMP_QUERY (on timer fire or user-set val, port becomes learning only)
>>  _TEMP -> _TEMP (noop on user refresh or igmp query, timer refreshes)
>>  _TEMP_QUERY -> _TEMP_QUERY (on igmp query the timer is armed, port becomes router)
>>
>> you never have _TEMP_QUERY -> _TEMP, which you're using here to denote the timer
>> getting armed and the bridge becoming a router.
> 
> 
> I am not sure I got this one. I do address that: when an IGMP query is recieved
> and the current state is _TEMP_QUERY, I arm the timer and set the state to
> _TEMP. I marked that place on the patch, so you can see below.
> 

Exactly, there is no such transition for the ports. I tried to say that you're using
the router type to distinguish between when a query is received and it is just learning.
I get that you need to do so, but that deviates from how ports are handled, thus I
suggested to use the timer state instead and drop the _TEMP for bridge device altogether.
If it's possible then the patch will be much simpler and you will not need the hacks
to hide the state from user-space which is the part I really don't like.

> 
>>
>>> The bridge device (brX) itself can also be configured by the user to be
>>> either fixed, disabled or learning mrouter port states, but currently there
>>> is no distinction between the MDB_RTR_TYPE_TEMP_QUERY and MDB_RTR_TYPE_TEMP
>>> in the bridge internal state. Due to that, when an IGMP query is received,
>>> it is not straightforward to tell whether it changes the bridge device
>>> mrouter port status or not.
>> But before this patch the bridge device could not get that set.
>>
>>> Further patches in this patch-set will introduce notifications upon the
>>> bridge device mrouter port state. In order to prevent resending bridge
>>> mrouter notification when it is not needed, such distinction is necessary.
>>>
>> Granted the bridge device hasn't got a way to clearly distinguish the transitions
>> without the chance for a race and if using the timer one could get an unnecessary
> 
> Is there a race condition?
> 

With this state - no, if you try to use the timer state though there might be
an extra notification as I've explained below, that's why I asked if it would be
a problem. I think it is worth looking into using the timer state because it will
remove a lot of the hacks for hiding the state needed in this patch.

>> notification but that seems like a corner case when the timer fires exactly at the
>> same time as the igmp query is received. Can't it be handled by just checking if
>> the new state is different in the notification receiver ?
>> If it can't and is a problem then I'd prefer to add a new boolean to denote that
>> router on/off transition rather than doing this.
>>
>>> Hence, add the distinction between MDB_RTR_TYPE_TEMP and
>>> MDB_RTR_TYPE_TEMP_QUERY states for the bridge device, similarly to any
>>> other bridge port.
>>>
>> This does not add proper MDB_RTR_TYPE_TEMP support for the bridge device
>> but seems to abuse it to distinguish the timer state, and changes
>> the meaning of MDB_RTR_TYPE_TEMP. Can't you just use the timer instead ?
>> I think it will simplify the set and avoid all of this.
>>
>>> In order to not break the current kernel-user API, don't propagate the new
>>> state to the user and use it only in the bridge internal state. Thus, if
>>> the user reads (either via sysfs or netlink) the bridge device mrouter
>>> state, he will get the MDB_RTR_TYPE_TEMP_QUERY state even if the current
>>> bridge state is MDB_RTR_TYPE_TEMP.
>>>
>>> Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
>>> Reviewed-by: Nogah Frankel <nogahf@mellanox.com>
>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>> ---
>>>  net/bridge/br_multicast.c | 25 +++++++++++++++++++++----
>>>  net/bridge/br_netlink.c   |  3 ++-
>>>  net/bridge/br_private.h   | 13 ++++++++++---
>>>  net/bridge/br_sysfs_br.c  |  3 ++-
>>>  4 files changed, 35 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>>> index 8dc5c8d..b86307b 100644
>>> --- a/net/bridge/br_multicast.c
>>> +++ b/net/bridge/br_multicast.c
>>> @@ -861,6 +861,17 @@ static void br_multicast_router_expired(unsigned long data)
>>>  
>>>  static void br_multicast_local_router_expired(unsigned long data)
>>>  {
>>> +	struct net_bridge *br = (struct net_bridge *) data;
>>> +
>>> +	spin_lock(&br->multicast_lock);
>>> +	if (br->multicast_router == MDB_RTR_TYPE_DISABLED ||
>>> +	    br->multicast_router == MDB_RTR_TYPE_PERM ||
>>> +	    timer_pending(&br->multicast_router_timer))
>>> +		goto out;
>>> +
>>> +	br->multicast_router = MDB_RTR_TYPE_TEMP_QUERY;
>>> +out:
>>> +	spin_unlock(&br->multicast_lock);
>>>  }
>>>  
>>>  static void br_multicast_querier_expired(struct net_bridge *br,
>>> @@ -1364,9 +1375,12 @@ static void br_multicast_mark_router(struct net_bridge *br,
>>>  	unsigned long now = jiffies;
>>>  
>>>  	if (!port) {
>>> -		if (br->multicast_router == MDB_RTR_TYPE_TEMP_QUERY)
>>> +		if (br->multicast_router == MDB_RTR_TYPE_TEMP_QUERY ||
>>> +		    br->multicast_router == MDB_RTR_TYPE_TEMP) {
>>>  			mod_timer(&br->multicast_router_timer,
>>>  				  now + br->multicast_querier_interval);
>>> +			br->multicast_router = MDB_RTR_TYPE_TEMP;
>>> +		}
> 
> 
> These are the transitions:
> _TEMP -> _TEMP_QUERY
> _TEMP_QUERY -> _TEMP_QUERY

You clearly always set multicast_router to _TEMP, where did you see a transition
_TEMP_QUERY -> _TEMP_QUERY or _TEMP -> _TEMP_QUERY ?
All transitions are -> _TEMP, because you use it to differentiate between learning
state and when a query was received. Maybe we have different definition for 
a transition :-)

> 
> In both transitions the timer is armed.
> 
> 
>>>  		return;
>>>  	}
>>>  
>>> @@ -1952,7 +1966,7 @@ void br_multicast_init(struct net_bridge *br)
>>>  
>>>  	spin_lock_init(&br->multicast_lock);
>>>  	setup_timer(&br->multicast_router_timer,
>>> -		    br_multicast_local_router_expired, 0);
>>> +		    br_multicast_local_router_expired, (unsigned long)br);
>>>  	setup_timer(&br->ip4_other_query.timer,
>>>  		    br_ip4_multicast_querier_expired, (unsigned long)br);
>>>  	setup_timer(&br->ip4_own_query.timer, br_ip4_multicast_query_expired,
>>> @@ -2043,11 +2057,14 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
>>>  	case MDB_RTR_TYPE_DISABLED:
>>>  	case MDB_RTR_TYPE_PERM:
>>>  		del_timer(&br->multicast_router_timer);
>>> -		/* fall through */
>>> -	case MDB_RTR_TYPE_TEMP_QUERY:
>>>  		br->multicast_router = val;
>>>  		err = 0;
>>>  		break;
>>> +	case MDB_RTR_TYPE_TEMP_QUERY:
>>> +		if (br->multicast_router != MDB_RTR_TYPE_TEMP)
>>> +			br->multicast_router = val;
>>> +		err = 0;
>>> +		break;
>>>  	}
>>>  
>>>  	spin_unlock_bh(&br->multicast_lock);
>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>>> index dea88a2..cee5016 100644
>>> --- a/net/bridge/br_netlink.c
>>> +++ b/net/bridge/br_netlink.c
>>> @@ -1357,7 +1357,8 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
>>>  		return -EMSGSIZE;
>>>  #endif
>>>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>>> -	if (nla_put_u8(skb, IFLA_BR_MCAST_ROUTER, br->multicast_router) ||
>>> +	if (nla_put_u8(skb, IFLA_BR_MCAST_ROUTER,
>>> +		       br_multicast_router_translate(br->multicast_router)) ||
>>>  	    nla_put_u8(skb, IFLA_BR_MCAST_SNOOPING, !br->multicast_disabled) ||
>>>  	    nla_put_u8(skb, IFLA_BR_MCAST_QUERY_USE_IFADDR,
>>>  		       br->multicast_query_use_ifaddr) ||
>>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>>> index ab4df24..e6e3fec 100644
>>> --- a/net/bridge/br_private.h
>>> +++ b/net/bridge/br_private.h
>>> @@ -649,9 +649,8 @@ void br_multicast_get_stats(const struct net_bridge *br,
>>>  
>>>  static inline bool br_multicast_is_router(struct net_bridge *br)
>>>  {
>>> -	return br->multicast_router == 2 ||
>>> -	       (br->multicast_router == 1 &&
>>> -		timer_pending(&br->multicast_router_timer));
>>> +	return br->multicast_router == MDB_RTR_TYPE_PERM ||
>>> +	       br->multicast_router == MDB_RTR_TYPE_TEMP;
>>>  }
>>>  
>>>  static inline bool
>>> @@ -790,6 +789,14 @@ static inline int br_multicast_igmp_type(const struct sk_buff *skb)
>>>  }
>>>  #endif
>>>  
>>> +static inline unsigned char
>> u8
>>
>>> +br_multicast_router_translate(unsigned char multicast_router)
>> u8, if need be change the type of the struct member
> 
> 
> Sorry, didn't quite get that. Currently, both the bridge_port and bridge have
> the multicast_router field, and in both cases it is of type unsigned char. Do
> you suggest to change both to be "u8"?
> 

Right, and this is unnecessarily long and requires to be broken ugly like that with
the type above and name below. So I suggested to use u8 to avoid that.

> 
>>
>>> +{
>>> +	if (multicast_router == MDB_RTR_TYPE_TEMP)
>>> +		return MDB_RTR_TYPE_TEMP_QUERY;
>>> +	return multicast_router;
>>> +}
>>> +
>>>  /* br_vlan.c */
>>>  #ifdef CONFIG_BRIDGE_VLAN_FILTERING
>>>  bool br_allowed_ingress(const struct net_bridge *br,
>>> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
>>> index 723f25e..9b9c597 100644
>>> --- a/net/bridge/br_sysfs_br.c
>>> +++ b/net/bridge/br_sysfs_br.c
>>> @@ -340,7 +340,8 @@ static ssize_t multicast_router_show(struct device *d,
>>>  				     struct device_attribute *attr, char *buf)
>>>  {
>>>  	struct net_bridge *br = to_bridge(d);
>>> -	return sprintf(buf, "%d\n", br->multicast_router);
>>> +	return sprintf(buf, "%d\n",
>>> +		       br_multicast_router_translate(br->multicast_router));
>>>  }
>>>  
>>>  static ssize_t multicast_router_store(struct device *d,
>>>
> 

Re: [patch net-next 1/6] net: bridge: Use the MDB_RTR_TYPE_TEMP on bridge device too

[Nikolay Aleksandrov]

On 08/10/17 12:39, Nikolay Aleksandrov wrote:
> On 08/10/17 08:23, Yotam Gigi wrote:
>> On 10/05/2017 03:09 PM, Nikolay Aleksandrov wrote:
>>> On 05/10/17 13:36, Jiri Pirko wrote:
>>>> From: Yotam Gigi <yotamg@mellanox.com>
>>>>
>>>> Every bridge port is in one of four mcast router port states:
>>>>  - MDB_RTR_TYPE_PERM - the port is set by the user to be an mrouter port
>>>>    regardless of IGMP queries.
>>>>  - MDB_RTR_TYPE_DISABLED - the port is set by the user to not be an mrouter
>>>>    port regardless of IGMP queries.
>>>>  - MDB_RTR_TYPE_TEMP - the port is set by the user to be in mcast router
>>>>    learning state, but currently it is not an mrouter port as no IGMP query
>>>>    has been received by it for the last multicast_querier_interval.
>>>>  - MDB_RTR_TYPE_TEMP_QUERY - the port is set by the user to be in mcast
>>>>    router learning state, and currently it is an mrouter port due to an
>>>>    IGMP query that has been received by it during the passed
>>>>    multicast_querier_interval.
>>> I think you got the last two partially mixed up, MDB_RTR_TYPE_TEMP marks the port as a router
>>> regardless if there were any igmp queries, while TYPE_TEMP_QUERY means it's in learning
>>> state. It is the timer (armed vs not) that defines if currently the port is a router
>>> when one of the TEMP/TEMP_QUERY are set. In the _TEMP case it is always armed as it
>>> is refreshed by user or igmp queries which was the point of that mode.
>>> So this means in br_multicast_router() just check for the timer_pending or perm mode.
>>
>>
>> As much as I tried to make this clear, it seems like I failed :)
>>
>> The 4 states I described are currently the "bridged port" states, not the
>> "bridge device" state. A bridged port has these 4 states, all can be set by the
>> user, while the bridge device only uses 3 of these states. This patch makes the
>> bridge device use the 4 states too. I thought it makes sense.
> 
> (disclaimer: this is all about bridge ports, not bridge device)
> Right, I'll try to explain again: _TEMP always marks the port as a mcast router,
> it does not put it into just learning state waiting for an igmp query and it can
> be refreshed by either a query or the user again setting the port in _TEMP.
> While _TEMP_QUERY puts the port in learning state waiting for a query to become
> a router, and _TEMP downgrades to _TEMP_QUERY if it expires.
> 
> Does that make it clearer ?
> 
> so for _TEMP you say:
>>>>  - MDB_RTR_TYPE_TEMP - the port is set by the user to be in mcast router
>>>>    learning state, but currently it is not an mrouter port as no IGMP query
>>>>    has been received by it for the last multicast_querier_interval.
> 
> which is not the case, it is always a router when that mode is set on a port.
> Same for _TEMP_QUERY.

Err, sorry by same I meant it is not correct, not that the _TEMP definition is the same.
Need to get coffee :-)

> 
>>
>> The first paragraph describes the current states of a bridged port, and the
>> second one explains the difference between bridged port and bridge device. I
>> will (try to) make it clearer if we agree on resending this patch.
>>
>> Is it clearer now?
>>
>>
>>>
>>> In the port code you have the following transitions:
>>>  _TEMP -> TEMP_QUERY (on timer fire or user-set val, port becomes learning only)
>>>  _TEMP -> _TEMP (noop on user refresh or igmp query, timer refreshes)
>>>  _TEMP_QUERY -> _TEMP_QUERY (on igmp query the timer is armed, port becomes router)
>>>
>>> you never have _TEMP_QUERY -> _TEMP, which you're using here to denote the timer
>>> getting armed and the bridge becoming a router.
>>
>>
>> I am not sure I got this one. I do address that: when an IGMP query is recieved
>> and the current state is _TEMP_QUERY, I arm the timer and set the state to
>> _TEMP. I marked that place on the patch, so you can see below.
>>
> 
> Exactly, there is no such transition for the ports. I tried to say that you're using
> the router type to distinguish between when a query is received and it is just learning.
> I get that you need to do so, but that deviates from how ports are handled, thus I
> suggested to use the timer state instead and drop the _TEMP for bridge device altogether.
> If it's possible then the patch will be much simpler and you will not need the hacks
> to hide the state from user-space which is the part I really don't like.
> 
>>
>>>
>>>> The bridge device (brX) itself can also be configured by the user to be
>>>> either fixed, disabled or learning mrouter port states, but currently there
>>>> is no distinction between the MDB_RTR_TYPE_TEMP_QUERY and MDB_RTR_TYPE_TEMP
>>>> in the bridge internal state. Due to that, when an IGMP query is received,
>>>> it is not straightforward to tell whether it changes the bridge device
>>>> mrouter port status or not.
>>> But before this patch the bridge device could not get that set.
>>>
>>>> Further patches in this patch-set will introduce notifications upon the
>>>> bridge device mrouter port state. In order to prevent resending bridge
>>>> mrouter notification when it is not needed, such distinction is necessary.
>>>>
>>> Granted the bridge device hasn't got a way to clearly distinguish the transitions
>>> without the chance for a race and if using the timer one could get an unnecessary
>>
>> Is there a race condition?
>>
> 
> With this state - no, if you try to use the timer state though there might be
> an extra notification as I've explained below, that's why I asked if it would be
> a problem. I think it is worth looking into using the timer state because it will
> remove a lot of the hacks for hiding the state needed in this patch.
> 
>>> notification but that seems like a corner case when the timer fires exactly at the
>>> same time as the igmp query is received. Can't it be handled by just checking if
>>> the new state is different in the notification receiver ?
>>> If it can't and is a problem then I'd prefer to add a new boolean to denote that
>>> router on/off transition rather than doing this.
>>>
>>>> Hence, add the distinction between MDB_RTR_TYPE_TEMP and
>>>> MDB_RTR_TYPE_TEMP_QUERY states for the bridge device, similarly to any
>>>> other bridge port.
>>>>
>>> This does not add proper MDB_RTR_TYPE_TEMP support for the bridge device
>>> but seems to abuse it to distinguish the timer state, and changes
>>> the meaning of MDB_RTR_TYPE_TEMP. Can't you just use the timer instead ?
>>> I think it will simplify the set and avoid all of this.
>>>
>>>> In order to not break the current kernel-user API, don't propagate the new
>>>> state to the user and use it only in the bridge internal state. Thus, if
>>>> the user reads (either via sysfs or netlink) the bridge device mrouter
>>>> state, he will get the MDB_RTR_TYPE_TEMP_QUERY state even if the current
>>>> bridge state is MDB_RTR_TYPE_TEMP.
>>>>
>>>> Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
>>>> Reviewed-by: Nogah Frankel <nogahf@mellanox.com>
>>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>>> ---
>>>>  net/bridge/br_multicast.c | 25 +++++++++++++++++++++----
>>>>  net/bridge/br_netlink.c   |  3 ++-
>>>>  net/bridge/br_private.h   | 13 ++++++++++---
>>>>  net/bridge/br_sysfs_br.c  |  3 ++-
>>>>  4 files changed, 35 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>>>> index 8dc5c8d..b86307b 100644
>>>> --- a/net/bridge/br_multicast.c
>>>> +++ b/net/bridge/br_multicast.c
>>>> @@ -861,6 +861,17 @@ static void br_multicast_router_expired(unsigned long data)
>>>>  
>>>>  static void br_multicast_local_router_expired(unsigned long data)
>>>>  {
>>>> +	struct net_bridge *br = (struct net_bridge *) data;
>>>> +
>>>> +	spin_lock(&br->multicast_lock);
>>>> +	if (br->multicast_router == MDB_RTR_TYPE_DISABLED ||
>>>> +	    br->multicast_router == MDB_RTR_TYPE_PERM ||
>>>> +	    timer_pending(&br->multicast_router_timer))
>>>> +		goto out;
>>>> +
>>>> +	br->multicast_router = MDB_RTR_TYPE_TEMP_QUERY;
>>>> +out:
>>>> +	spin_unlock(&br->multicast_lock);
>>>>  }
>>>>  
>>>>  static void br_multicast_querier_expired(struct net_bridge *br,
>>>> @@ -1364,9 +1375,12 @@ static void br_multicast_mark_router(struct net_bridge *br,
>>>>  	unsigned long now = jiffies;
>>>>  
>>>>  	if (!port) {
>>>> -		if (br->multicast_router == MDB_RTR_TYPE_TEMP_QUERY)
>>>> +		if (br->multicast_router == MDB_RTR_TYPE_TEMP_QUERY ||
>>>> +		    br->multicast_router == MDB_RTR_TYPE_TEMP) {
>>>>  			mod_timer(&br->multicast_router_timer,
>>>>  				  now + br->multicast_querier_interval);
>>>> +			br->multicast_router = MDB_RTR_TYPE_TEMP;
>>>> +		}
>>
>>
>> These are the transitions:
>> _TEMP -> _TEMP_QUERY
>> _TEMP_QUERY -> _TEMP_QUERY
> 
> You clearly always set multicast_router to _TEMP, where did you see a transition
> _TEMP_QUERY -> _TEMP_QUERY or _TEMP -> _TEMP_QUERY ?
> All transitions are -> _TEMP, because you use it to differentiate between learning
> state and when a query was received. Maybe we have different definition for 
> a transition :-)
> 
>>
>> In both transitions the timer is armed.
>>
>>
>>>>  		return;
>>>>  	}
>>>>  
>>>> @@ -1952,7 +1966,7 @@ void br_multicast_init(struct net_bridge *br)
>>>>  
>>>>  	spin_lock_init(&br->multicast_lock);
>>>>  	setup_timer(&br->multicast_router_timer,
>>>> -		    br_multicast_local_router_expired, 0);
>>>> +		    br_multicast_local_router_expired, (unsigned long)br);
>>>>  	setup_timer(&br->ip4_other_query.timer,
>>>>  		    br_ip4_multicast_querier_expired, (unsigned long)br);
>>>>  	setup_timer(&br->ip4_own_query.timer, br_ip4_multicast_query_expired,
>>>> @@ -2043,11 +2057,14 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
>>>>  	case MDB_RTR_TYPE_DISABLED:
>>>>  	case MDB_RTR_TYPE_PERM:
>>>>  		del_timer(&br->multicast_router_timer);
>>>> -		/* fall through */
>>>> -	case MDB_RTR_TYPE_TEMP_QUERY:
>>>>  		br->multicast_router = val;
>>>>  		err = 0;
>>>>  		break;
>>>> +	case MDB_RTR_TYPE_TEMP_QUERY:
>>>> +		if (br->multicast_router != MDB_RTR_TYPE_TEMP)
>>>> +			br->multicast_router = val;
>>>> +		err = 0;
>>>> +		break;
>>>>  	}
>>>>  
>>>>  	spin_unlock_bh(&br->multicast_lock);
>>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>>>> index dea88a2..cee5016 100644
>>>> --- a/net/bridge/br_netlink.c
>>>> +++ b/net/bridge/br_netlink.c
>>>> @@ -1357,7 +1357,8 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
>>>>  		return -EMSGSIZE;
>>>>  #endif
>>>>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>>>> -	if (nla_put_u8(skb, IFLA_BR_MCAST_ROUTER, br->multicast_router) ||
>>>> +	if (nla_put_u8(skb, IFLA_BR_MCAST_ROUTER,
>>>> +		       br_multicast_router_translate(br->multicast_router)) ||
>>>>  	    nla_put_u8(skb, IFLA_BR_MCAST_SNOOPING, !br->multicast_disabled) ||
>>>>  	    nla_put_u8(skb, IFLA_BR_MCAST_QUERY_USE_IFADDR,
>>>>  		       br->multicast_query_use_ifaddr) ||
>>>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>>>> index ab4df24..e6e3fec 100644
>>>> --- a/net/bridge/br_private.h
>>>> +++ b/net/bridge/br_private.h
>>>> @@ -649,9 +649,8 @@ void br_multicast_get_stats(const struct net_bridge *br,
>>>>  
>>>>  static inline bool br_multicast_is_router(struct net_bridge *br)
>>>>  {
>>>> -	return br->multicast_router == 2 ||
>>>> -	       (br->multicast_router == 1 &&
>>>> -		timer_pending(&br->multicast_router_timer));
>>>> +	return br->multicast_router == MDB_RTR_TYPE_PERM ||
>>>> +	       br->multicast_router == MDB_RTR_TYPE_TEMP;
>>>>  }
>>>>  
>>>>  static inline bool
>>>> @@ -790,6 +789,14 @@ static inline int br_multicast_igmp_type(const struct sk_buff *skb)
>>>>  }
>>>>  #endif
>>>>  
>>>> +static inline unsigned char
>>> u8
>>>
>>>> +br_multicast_router_translate(unsigned char multicast_router)
>>> u8, if need be change the type of the struct member
>>
>>
>> Sorry, didn't quite get that. Currently, both the bridge_port and bridge have
>> the multicast_router field, and in both cases it is of type unsigned char. Do
>> you suggest to change both to be "u8"?
>>
> 
> Right, and this is unnecessarily long and requires to be broken ugly like that with
> the type above and name below. So I suggested to use u8 to avoid that.
> 
>>
>>>
>>>> +{
>>>> +	if (multicast_router == MDB_RTR_TYPE_TEMP)
>>>> +		return MDB_RTR_TYPE_TEMP_QUERY;
>>>> +	return multicast_router;
>>>> +}
>>>> +
>>>>  /* br_vlan.c */
>>>>  #ifdef CONFIG_BRIDGE_VLAN_FILTERING
>>>>  bool br_allowed_ingress(const struct net_bridge *br,
>>>> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
>>>> index 723f25e..9b9c597 100644
>>>> --- a/net/bridge/br_sysfs_br.c
>>>> +++ b/net/bridge/br_sysfs_br.c
>>>> @@ -340,7 +340,8 @@ static ssize_t multicast_router_show(struct device *d,
>>>>  				     struct device_attribute *attr, char *buf)
>>>>  {
>>>>  	struct net_bridge *br = to_bridge(d);
>>>> -	return sprintf(buf, "%d\n", br->multicast_router);
>>>> +	return sprintf(buf, "%d\n",
>>>> +		       br_multicast_router_translate(br->multicast_router));
>>>>  }
>>>>  
>>>>  static ssize_t multicast_router_store(struct device *d,
>>>>
>>
> 

Re: [patch net-next 1/6] net: bridge: Use the MDB_RTR_TYPE_TEMP on bridge device too

[Yotam Gigi]

On 10/08/2017 12:42 PM, Nikolay Aleksandrov wrote:
> On 08/10/17 12:39, Nikolay Aleksandrov wrote:
>> On 08/10/17 08:23, Yotam Gigi wrote:
>>> On 10/05/2017 03:09 PM, Nikolay Aleksandrov wrote:
>>>> On 05/10/17 13:36, Jiri Pirko wrote:
>>>>> From: Yotam Gigi <yotamg@mellanox.com>
>>>>>
>>>>> Every bridge port is in one of four mcast router port states:
>>>>>  - MDB_RTR_TYPE_PERM - the port is set by the user to be an mrouter port
>>>>>    regardless of IGMP queries.
>>>>>  - MDB_RTR_TYPE_DISABLED - the port is set by the user to not be an mrouter
>>>>>    port regardless of IGMP queries.
>>>>>  - MDB_RTR_TYPE_TEMP - the port is set by the user to be in mcast router
>>>>>    learning state, but currently it is not an mrouter port as no IGMP query
>>>>>    has been received by it for the last multicast_querier_interval.
>>>>>  - MDB_RTR_TYPE_TEMP_QUERY - the port is set by the user to be in mcast
>>>>>    router learning state, and currently it is an mrouter port due to an
>>>>>    IGMP query that has been received by it during the passed
>>>>>    multicast_querier_interval.
>>>> I think you got the last two partially mixed up, MDB_RTR_TYPE_TEMP marks the port as a router
>>>> regardless if there were any igmp queries, while TYPE_TEMP_QUERY means it's in learning
>>>> state. It is the timer (armed vs not) that defines if currently the port is a router
>>>> when one of the TEMP/TEMP_QUERY are set. In the _TEMP case it is always armed as it
>>>> is refreshed by user or igmp queries which was the point of that mode.
>>>> So this means in br_multicast_router() just check for the timer_pending or perm mode.
>>>
>>> As much as I tried to make this clear, it seems like I failed :)
>>>
>>> The 4 states I described are currently the "bridged port" states, not the
>>> "bridge device" state. A bridged port has these 4 states, all can be set by the
>>> user, while the bridge device only uses 3 of these states. This patch makes the
>>> bridge device use the 4 states too. I thought it makes sense.
>> (disclaimer: this is all about bridge ports, not bridge device)
>> Right, I'll try to explain again: _TEMP always marks the port as a mcast router,
>> it does not put it into just learning state waiting for an igmp query and it can
>> be refreshed by either a query or the user again setting the port in _TEMP.
>> While _TEMP_QUERY puts the port in learning state waiting for a query to become
>> a router, and _TEMP downgrades to _TEMP_QUERY if it expires.
>>
>> Does that make it clearer ?
>>
>> so for _TEMP you say:
>>>>>  - MDB_RTR_TYPE_TEMP - the port is set by the user to be in mcast router
>>>>>    learning state, but currently it is not an mrouter port as no IGMP query
>>>>>    has been received by it for the last multicast_querier_interval.
>> which is not the case, it is always a router when that mode is set on a port.
>> Same for _TEMP_QUERY.
> Err, sorry by same I meant it is not correct, not that the _TEMP definition is the same.
> Need to get coffee :-)

Ho, I see  that I was clear but not right :)

I had a look at the code and seems like you are right - for some reason I
thought that _TEMP is learning-active and _TEMP_QUERY is learning-inactive, and
the ports change states when according to IGMP queries.


>
>>> The first paragraph describes the current states of a bridged port, and the
>>> second one explains the difference between bridged port and bridge device. I
>>> will (try to) make it clearer if we agree on resending this patch.
>>>
>>> Is it clearer now?


Yes it is.

>>>
>>>
>>>> In the port code you have the following transitions:
>>>>  _TEMP -> TEMP_QUERY (on timer fire or user-set val, port becomes learning only)
>>>>  _TEMP -> _TEMP (noop on user refresh or igmp query, timer refreshes)
>>>>  _TEMP_QUERY -> _TEMP_QUERY (on igmp query the timer is armed, port becomes router)
>>>>
>>>> you never have _TEMP_QUERY -> _TEMP, which you're using here to denote the timer
>>>> getting armed and the bridge becoming a router.
>>>
>>> I am not sure I got this one. I do address that: when an IGMP query is recieved
>>> and the current state is _TEMP_QUERY, I arm the timer and set the state to
>>> _TEMP. I marked that place on the patch, so you can see below.
>>>
>> Exactly, there is no such transition for the ports. I tried to say that you're using
>> the router type to distinguish between when a query is received and it is just learning.
>> I get that you need to do so, but that deviates from how ports are handled, thus I
>> suggested to use the timer state instead and drop the _TEMP for bridge device altogether.
>> If it's possible then the patch will be much simpler and you will not need the hacks
>> to hide the state from user-space which is the part I really don't like.


Yeah, I agree. Thanks for the feedback and sorry for the late answer :)

>>
>>>>> The bridge device (brX) itself can also be configured by the user to be
>>>>> either fixed, disabled or learning mrouter port states, but currently there
>>>>> is no distinction between the MDB_RTR_TYPE_TEMP_QUERY and MDB_RTR_TYPE_TEMP
>>>>> in the bridge internal state. Due to that, when an IGMP query is received,
>>>>> it is not straightforward to tell whether it changes the bridge device
>>>>> mrouter port status or not.
>>>> But before this patch the bridge device could not get that set.
>>>>
>>>>> Further patches in this patch-set will introduce notifications upon the
>>>>> bridge device mrouter port state. In order to prevent resending bridge
>>>>> mrouter notification when it is not needed, such distinction is necessary.
>>>>>
>>>> Granted the bridge device hasn't got a way to clearly distinguish the transitions
>>>> without the chance for a race and if using the timer one could get an unnecessary
>>> Is there a race condition?
>>>
>> With this state - no, if you try to use the timer state though there might be
>> an extra notification as I've explained below, that's why I asked if it would be
>> a problem. I think it is worth looking into using the timer state because it will
>> remove a lot of the hacks for hiding the state needed in this patch.
>>
>>>> notification but that seems like a corner case when the timer fires exactly at the
>>>> same time as the igmp query is received. Can't it be handled by just checking if
>>>> the new state is different in the notification receiver ?
>>>> If it can't and is a problem then I'd prefer to add a new boolean to denote that
>>>> router on/off transition rather than doing this.
>>>>
>>>>> Hence, add the distinction between MDB_RTR_TYPE_TEMP and
>>>>> MDB_RTR_TYPE_TEMP_QUERY states for the bridge device, similarly to any
>>>>> other bridge port.
>>>>>
>>>> This does not add proper MDB_RTR_TYPE_TEMP support for the bridge device
>>>> but seems to abuse it to distinguish the timer state, and changes
>>>> the meaning of MDB_RTR_TYPE_TEMP. Can't you just use the timer instead ?
>>>> I think it will simplify the set and avoid all of this.
>>>>
>>>>> In order to not break the current kernel-user API, don't propagate the new
>>>>> state to the user and use it only in the bridge internal state. Thus, if
>>>>> the user reads (either via sysfs or netlink) the bridge device mrouter
>>>>> state, he will get the MDB_RTR_TYPE_TEMP_QUERY state even if the current
>>>>> bridge state is MDB_RTR_TYPE_TEMP.
>>>>>
>>>>> Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
>>>>> Reviewed-by: Nogah Frankel <nogahf@mellanox.com>
>>>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>>>> ---
>>>>>  net/bridge/br_multicast.c | 25 +++++++++++++++++++++----
>>>>>  net/bridge/br_netlink.c   |  3 ++-
>>>>>  net/bridge/br_private.h   | 13 ++++++++++---
>>>>>  net/bridge/br_sysfs_br.c  |  3 ++-
>>>>>  4 files changed, 35 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>>>>> index 8dc5c8d..b86307b 100644
>>>>> --- a/net/bridge/br_multicast.c
>>>>> +++ b/net/bridge/br_multicast.c
>>>>> @@ -861,6 +861,17 @@ static void br_multicast_router_expired(unsigned long data)
>>>>>  
>>>>>  static void br_multicast_local_router_expired(unsigned long data)
>>>>>  {
>>>>> +	struct net_bridge *br = (struct net_bridge *) data;
>>>>> +
>>>>> +	spin_lock(&br->multicast_lock);
>>>>> +	if (br->multicast_router == MDB_RTR_TYPE_DISABLED ||
>>>>> +	    br->multicast_router == MDB_RTR_TYPE_PERM ||
>>>>> +	    timer_pending(&br->multicast_router_timer))
>>>>> +		goto out;
>>>>> +
>>>>> +	br->multicast_router = MDB_RTR_TYPE_TEMP_QUERY;
>>>>> +out:
>>>>> +	spin_unlock(&br->multicast_lock);
>>>>>  }
>>>>>  
>>>>>  static void br_multicast_querier_expired(struct net_bridge *br,
>>>>> @@ -1364,9 +1375,12 @@ static void br_multicast_mark_router(struct net_bridge *br,
>>>>>  	unsigned long now = jiffies;
>>>>>  
>>>>>  	if (!port) {
>>>>> -		if (br->multicast_router == MDB_RTR_TYPE_TEMP_QUERY)
>>>>> +		if (br->multicast_router == MDB_RTR_TYPE_TEMP_QUERY ||
>>>>> +		    br->multicast_router == MDB_RTR_TYPE_TEMP) {
>>>>>  			mod_timer(&br->multicast_router_timer,
>>>>>  				  now + br->multicast_querier_interval);
>>>>> +			br->multicast_router = MDB_RTR_TYPE_TEMP;
>>>>> +		}
>>>
>>> These are the transitions:
>>> _TEMP -> _TEMP_QUERY
>>> _TEMP_QUERY -> _TEMP_QUERY
>> You clearly always set multicast_router to _TEMP, where did you see a transition
>> _TEMP_QUERY -> _TEMP_QUERY or _TEMP -> _TEMP_QUERY ?
>> All transitions are -> _TEMP, because you use it to differentiate between learning
>> state and when a query was received. Maybe we have different definition for 
>> a transition :-)
>>
>>> In both transitions the timer is armed.
>>>
>>>
>>>>>  		return;
>>>>>  	}
>>>>>  
>>>>> @@ -1952,7 +1966,7 @@ void br_multicast_init(struct net_bridge *br)
>>>>>  
>>>>>  	spin_lock_init(&br->multicast_lock);
>>>>>  	setup_timer(&br->multicast_router_timer,
>>>>> -		    br_multicast_local_router_expired, 0);
>>>>> +		    br_multicast_local_router_expired, (unsigned long)br);
>>>>>  	setup_timer(&br->ip4_other_query.timer,
>>>>>  		    br_ip4_multicast_querier_expired, (unsigned long)br);
>>>>>  	setup_timer(&br->ip4_own_query.timer, br_ip4_multicast_query_expired,
>>>>> @@ -2043,11 +2057,14 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
>>>>>  	case MDB_RTR_TYPE_DISABLED:
>>>>>  	case MDB_RTR_TYPE_PERM:
>>>>>  		del_timer(&br->multicast_router_timer);
>>>>> -		/* fall through */
>>>>> -	case MDB_RTR_TYPE_TEMP_QUERY:
>>>>>  		br->multicast_router = val;
>>>>>  		err = 0;
>>>>>  		break;
>>>>> +	case MDB_RTR_TYPE_TEMP_QUERY:
>>>>> +		if (br->multicast_router != MDB_RTR_TYPE_TEMP)
>>>>> +			br->multicast_router = val;
>>>>> +		err = 0;
>>>>> +		break;
>>>>>  	}
>>>>>  
>>>>>  	spin_unlock_bh(&br->multicast_lock);
>>>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>>>>> index dea88a2..cee5016 100644
>>>>> --- a/net/bridge/br_netlink.c
>>>>> +++ b/net/bridge/br_netlink.c
>>>>> @@ -1357,7 +1357,8 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
>>>>>  		return -EMSGSIZE;
>>>>>  #endif
>>>>>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>>>>> -	if (nla_put_u8(skb, IFLA_BR_MCAST_ROUTER, br->multicast_router) ||
>>>>> +	if (nla_put_u8(skb, IFLA_BR_MCAST_ROUTER,
>>>>> +		       br_multicast_router_translate(br->multicast_router)) ||
>>>>>  	    nla_put_u8(skb, IFLA_BR_MCAST_SNOOPING, !br->multicast_disabled) ||
>>>>>  	    nla_put_u8(skb, IFLA_BR_MCAST_QUERY_USE_IFADDR,
>>>>>  		       br->multicast_query_use_ifaddr) ||
>>>>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>>>>> index ab4df24..e6e3fec 100644
>>>>> --- a/net/bridge/br_private.h
>>>>> +++ b/net/bridge/br_private.h
>>>>> @@ -649,9 +649,8 @@ void br_multicast_get_stats(const struct net_bridge *br,
>>>>>  
>>>>>  static inline bool br_multicast_is_router(struct net_bridge *br)
>>>>>  {
>>>>> -	return br->multicast_router == 2 ||
>>>>> -	       (br->multicast_router == 1 &&
>>>>> -		timer_pending(&br->multicast_router_timer));
>>>>> +	return br->multicast_router == MDB_RTR_TYPE_PERM ||
>>>>> +	       br->multicast_router == MDB_RTR_TYPE_TEMP;
>>>>>  }
>>>>>  
>>>>>  static inline bool
>>>>> @@ -790,6 +789,14 @@ static inline int br_multicast_igmp_type(const struct sk_buff *skb)
>>>>>  }
>>>>>  #endif
>>>>>  
>>>>> +static inline unsigned char
>>>> u8
>>>>
>>>>> +br_multicast_router_translate(unsigned char multicast_router)
>>>> u8, if need be change the type of the struct member
>>>
>>> Sorry, didn't quite get that. Currently, both the bridge_port and bridge have
>>> the multicast_router field, and in both cases it is of type unsigned char. Do
>>> you suggest to change both to be "u8"?
>>>
>> Right, and this is unnecessarily long and requires to be broken ugly like that with
>> the type above and name below. So I suggested to use u8 to avoid that.
>>
>>>>> +{
>>>>> +	if (multicast_router == MDB_RTR_TYPE_TEMP)
>>>>> +		return MDB_RTR_TYPE_TEMP_QUERY;
>>>>> +	return multicast_router;
>>>>> +}
>>>>> +
>>>>>  /* br_vlan.c */
>>>>>  #ifdef CONFIG_BRIDGE_VLAN_FILTERING
>>>>>  bool br_allowed_ingress(const struct net_bridge *br,
>>>>> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
>>>>> index 723f25e..9b9c597 100644
>>>>> --- a/net/bridge/br_sysfs_br.c
>>>>> +++ b/net/bridge/br_sysfs_br.c
>>>>> @@ -340,7 +340,8 @@ static ssize_t multicast_router_show(struct device *d,
>>>>>  				     struct device_attribute *attr, char *buf)
>>>>>  {
>>>>>  	struct net_bridge *br = to_bridge(d);
>>>>> -	return sprintf(buf, "%d\n", br->multicast_router);
>>>>> +	return sprintf(buf, "%d\n",
>>>>> +		       br_multicast_router_translate(br->multicast_router));
>>>>>  }
>>>>>  
>>>>>  static ssize_t multicast_router_store(struct device *d,
>>>>>