[PATCH net-next 00/14] net: dsa: management mode for bcm_sf2

[PATCH net-next 00/14] net: dsa: management mode for bcm_sf2

[Florian Fainelli]

Hi all,

This patch series does a number of things in order to enable
management mode for bcm_sf2 (which could be easily extended to b53 with
proper testing later on). In order to get there, there were several use
cases that did not work correctly and that needed to be fixed:

- VLAN devices on top of switch ports not being member of a bridge, with
  other switch ports being bridged, with the bridge having VLAN
  filtering enabled.

- lack of multicast filtering by default on network ports which should
  be happening in order for the non-bridged DSA ports to behave strictly
  as Ethernet NICs with proper filering. This is accomplished by hooking
  a ndo_set_rx_mode() function to the DSA slave network devices

- when VLAN filtering is globally enabled on the switch (because at
  least a bridge device requires it), then we also need to make sure
  that when doing multicast over VLAN devices over a switch port
  (bridged or not) happens with the correct MDB address *and* VID

Hopefully the changes to net/8021q and net/bridge are deemed acceptable.

The Broadcom switches have a switch-wide VLAN filtering attribute,
which is why we must always make sure there is a valid VLAN entry even
for switch ports which are not part of a bridge device, yet there is at
least one bridge device spanning the switch.

Multicast flooding can be done on a per-port basis, including for the
CPU/management port, however, once multicast reception is enabled on the
CPU port, it bypasses the ARL (Address Resolution Logic), so we receive
*all* multicast, even from ports do not have their flooding bit set,
which is unfortunate. This is the reason why we must continue adding
enough HOST_MDB notifications to let the CPU port continue to filter
multicast traffic.

Here are some of the uses cases that were tested after this patch series
(all commands are running on the device being tested) and
iperf/ping/etc. should be working for/after all steps:

echo "file drivers/net/dsa/b53/b53_common.c +p" > /sys/kernel/debug/dynamic_debug/control
echo 8 7 4 1 > /proc/sys/kernel/printk
killall udhcpc
ip addr flush dev gphy
ip link add dev br0 type bridge
echo 1 > /sys/class/net/br0/bridge/vlan_filtering
ip link set dev gphy master br0
udhcpc -i br0
ip ro add 226.94.1.1/32 dev br0
iperf -s -B 226.94.1.1 -u &
vconfig add rgmii_1 100
ifconfig rgmii_1.100 192.168.100.10
ping -c 5 192.168.100.1
ip ro add 226.95.1.2/32 dev rgmii_1.100
iperf -s -B 226.95.1.2 -u &
vconfig add br0 42
bridge vlan add vid 42 dev gphy
bridge vlan add vid 42 dev br0 self
ifconfig br0.42 192.168.42.2
ip ro add 226.96.1.3/32 dev br0.42

Florian Fainelli (14):
  net: bridge: multicast: Propagate br_mc_disabled_update() return
  net: dsa: b53: Fix default VLAN ID
  net: dsa: b53: Properly account for VLAN filtering
  net: systemport: Fix reception of BPDUs
  net: dsa: b53: Define registers for IGMP snooping
  net: dsa: b53: Add support for MDB
  net: dsa: Add ability to program multicast filter for CPU port
  net: dsa: Add ndo_vlan_rx_{add,kill}_vid implementation
  net: bridge: Propagate MC addresses with VID through switchdev
  net: vlan: Propagate MC addresses with VID through switchdev
  net: dsa: Make VLAN filtering use DSA notifiers
  net: dsa: Wire up multicast IGMP snooping attribute notification
  net: dsa: b53: Add support for toggling IGMP snooping
  net: dsa: bcm_sf2: Enable management mode

 drivers/net/dsa/b53/b53_common.c           | 219 ++++++++++++++++++---
 drivers/net/dsa/b53/b53_priv.h             |  14 +-
 drivers/net/dsa/b53/b53_regs.h             |  22 +++
 drivers/net/dsa/bcm_sf2.c                  |  56 ++++--
 drivers/net/dsa/bcm_sf2_regs.h             |   5 +
 drivers/net/ethernet/broadcom/bcmsysport.c |   4 +
 include/net/dsa.h                          |   2 +
 net/8021q/vlan_dev.c                       |  40 ++++
 net/bridge/br_device.c                     |  55 ++++++
 net/bridge/br_multicast.c                  |  19 +-
 net/dsa/dsa_priv.h                         |  22 ++-
 net/dsa/port.c                             |  42 ++--
 net/dsa/slave.c                            | 107 +++++++++-
 net/dsa/switch.c                           |  57 ++++++
 14 files changed, 607 insertions(+), 57 deletions(-)

-- 
2.17.1

[PATCH net-next 01/14] net: bridge: multicast: Propagate br_mc_disabled_update() return

[Florian Fainelli]

Some Ethernet switches might not be able to support disabling multicast
flooding globally when e.g: several bridges span the same physical
device, propagate the return value of br_mc_disabled_update() such that
this propagates correctly to user-space.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/bridge/br_multicast.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 3aeff0895669..09fc92541873 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -813,7 +813,7 @@ static void br_ip6_multicast_port_query_expired(struct timer_list *t)
 }
 #endif
 
-static void br_mc_disabled_update(struct net_device *dev, bool value)
+static int br_mc_disabled_update(struct net_device *dev, bool value)
 {
 	struct switchdev_attr attr = {
 		.orig_dev = dev,
@@ -822,11 +822,13 @@ static void br_mc_disabled_update(struct net_device *dev, bool value)
 		.u.mc_disabled = !value,
 	};
 
-	switchdev_port_attr_set(dev, &attr);
+	return switchdev_port_attr_set(dev, &attr);
 }
 
 int br_multicast_add_port(struct net_bridge_port *port)
 {
+	int ret;
+
 	port->multicast_router = MDB_RTR_TYPE_TEMP_QUERY;
 
 	timer_setup(&port->multicast_router_timer,
@@ -837,8 +839,11 @@ int br_multicast_add_port(struct net_bridge_port *port)
 	timer_setup(&port->ip6_own_query.timer,
 		    br_ip6_multicast_port_query_expired, 0);
 #endif
-	br_mc_disabled_update(port->dev,
-			      br_opt_get(port->br, BROPT_MULTICAST_ENABLED));
+	ret = br_mc_disabled_update(port->dev,
+				    br_opt_get(port->br,
+					       BROPT_MULTICAST_ENABLED));
+	if (ret)
+		return ret;
 
 	port->mcast_stats = netdev_alloc_pcpu_stats(struct bridge_mcast_stats);
 	if (!port->mcast_stats)
@@ -1937,12 +1942,16 @@ static void br_multicast_start_querier(struct net_bridge *br,
 int br_multicast_toggle(struct net_bridge *br, unsigned long val)
 {
 	struct net_bridge_port *port;
+	int err;
 
 	spin_lock_bh(&br->multicast_lock);
 	if (!!br_opt_get(br, BROPT_MULTICAST_ENABLED) == !!val)
 		goto unlock;
 
-	br_mc_disabled_update(br->dev, val);
+	err = br_mc_disabled_update(br->dev, val);
+	if (err)
+		goto unlock;
+
 	br_opt_toggle(br, BROPT_MULTICAST_ENABLED, !!val);
 	if (!br_opt_get(br, BROPT_MULTICAST_ENABLED))
 		goto unlock;
-- 
2.17.1

[PATCH net-next 02/14] net: dsa: b53: Fix default VLAN ID

[Florian Fainelli]

We were not consistent in how the default VID of a given port was
defined, b53_br_leave() would make sure the VLAN ID would be either 0/1
depending on the switch generation, but b53_configure_vlan(), which is
the default configuration would unconditionally set it to 1. The correct
value is 1 for 5325/5365 series and 0 otherwise. To avoid repeating that
mistake ever again, introduce a helper function: b53_default_pvid() to
factor that out.

Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 0e4bbdcc614f..964a9ec4652a 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -632,15 +632,25 @@ static void b53_enable_mib(struct b53_device *dev)
 	b53_write8(dev, B53_MGMT_PAGE, B53_GLOBAL_CONFIG, gc);
 }
 
+static u16 b53_default_pvid(struct b53_device *dev)
+{
+	if (is5325(dev) || is5365(dev))
+		return 1;
+	else
+		return 0;
+}
+
 int b53_configure_vlan(struct dsa_switch *ds)
 {
 	struct b53_device *dev = ds->priv;
 	struct b53_vlan vl = { 0 };
-	int i;
+	int i, def_vid;
+
+	def_vid = b53_default_pvid(dev);
 
 	/* clear all vlan entries */
 	if (is5325(dev) || is5365(dev)) {
-		for (i = 1; i < dev->num_vlans; i++)
+		for (i = def_vid; i < dev->num_vlans; i++)
 			b53_set_vlan_entry(dev, i, &vl);
 	} else {
 		b53_do_vlan_op(dev, VTA_CMD_CLEAR);
@@ -650,7 +660,7 @@ int b53_configure_vlan(struct dsa_switch *ds)
 
 	b53_for_each_port(dev, i)
 		b53_write16(dev, B53_VLAN_PAGE,
-			    B53_VLAN_PORT_DEF_TAG(i), 1);
+			    B53_VLAN_PORT_DEF_TAG(i), def_vid);
 
 	if (!is5325(dev) && !is5365(dev))
 		b53_set_jumbo(dev, dev->enable_jumbo, false);
@@ -1326,12 +1336,8 @@ int b53_vlan_del(struct dsa_switch *ds, int port,
 
 		vl->members &= ~BIT(port);
 
-		if (pvid == vid) {
-			if (is5325(dev) || is5365(dev))
-				pvid = 1;
-			else
-				pvid = 0;
-		}
+		if (pvid == vid)
+			pvid = b53_default_pvid(dev);
 
 		if (untagged && !dsa_is_cpu_port(ds, port))
 			vl->untag &= ~(BIT(port));
@@ -1644,10 +1650,7 @@ void b53_br_leave(struct dsa_switch *ds, int port, struct net_device *br)
 	b53_write16(dev, B53_PVLAN_PAGE, B53_PVLAN_PORT_MASK(port), pvlan);
 	dev->ports[port].vlan_ctl_mask = pvlan;
 
-	if (is5325(dev) || is5365(dev))
-		pvid = 1;
-	else
-		pvid = 0;
+	pvid = b53_default_pvid(dev);
 
 	/* Make this port join all VLANs without VLAN entries */
 	if (is58xx(dev)) {
-- 
2.17.1

[PATCH net-next 03/14] net: dsa: b53: Properly account for VLAN filtering

[Florian Fainelli]

VLAN filtering can be built into the kernel, and also dynamically turned
on/off through the bridge master device. Allow re-configuring the switch
appropriately to account for that by deciding whether VLAN table
(v_table) misses should lead to a drop or forward.

Fixes: a2482d2ce349 ("net: dsa: b53: Plug in VLAN support")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 59 +++++++++++++++++++++++++++++---
 drivers/net/dsa/b53/b53_priv.h   |  3 ++
 2 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 964a9ec4652a..2fef4c564420 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -344,7 +344,8 @@ static void b53_set_forwarding(struct b53_device *dev, int enable)
 	b53_write8(dev, B53_CTRL_PAGE, B53_SWITCH_CTRL, mgmt);
 }
 
-static void b53_enable_vlan(struct b53_device *dev, bool enable)
+static void b53_enable_vlan(struct b53_device *dev, bool enable,
+			    bool enable_filtering)
 {
 	u8 mgmt, vc0, vc1, vc4 = 0, vc5;
 
@@ -369,8 +370,13 @@ static void b53_enable_vlan(struct b53_device *dev, bool enable)
 		vc0 |= VC0_VLAN_EN | VC0_VID_CHK_EN | VC0_VID_HASH_VID;
 		vc1 |= VC1_RX_MCST_UNTAG_EN | VC1_RX_MCST_FWD_EN;
 		vc4 &= ~VC4_ING_VID_CHECK_MASK;
-		vc4 |= VC4_ING_VID_VIO_DROP << VC4_ING_VID_CHECK_S;
-		vc5 |= VC5_DROP_VTABLE_MISS;
+		if (enable_filtering) {
+			vc4 |= VC4_ING_VID_VIO_DROP << VC4_ING_VID_CHECK_S;
+			vc5 |= VC5_DROP_VTABLE_MISS;
+		} else {
+			vc4 |= VC4_ING_VID_VIO_FWD << VC4_ING_VID_CHECK_S;
+			vc5 &= ~VC5_DROP_VTABLE_MISS;
+		}
 
 		if (is5325(dev))
 			vc0 &= ~VC0_RESERVED_1;
@@ -420,6 +426,9 @@ static void b53_enable_vlan(struct b53_device *dev, bool enable)
 	}
 
 	b53_write8(dev, B53_CTRL_PAGE, B53_SWITCH_MODE, mgmt);
+
+	dev->vlan_enabled = enable;
+	dev->vlan_filtering_enabled = enable_filtering;
 }
 
 static int b53_set_jumbo(struct b53_device *dev, bool enable, bool allow_10_100)
@@ -656,7 +665,7 @@ int b53_configure_vlan(struct dsa_switch *ds)
 		b53_do_vlan_op(dev, VTA_CMD_CLEAR);
 	}
 
-	b53_enable_vlan(dev, false);
+	b53_enable_vlan(dev, false, dev->vlan_filtering_enabled);
 
 	b53_for_each_port(dev, i)
 		b53_write16(dev, B53_VLAN_PAGE,
@@ -1265,6 +1274,46 @@ EXPORT_SYMBOL(b53_phylink_mac_link_up);
 
 int b53_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering)
 {
+	struct b53_device *dev = ds->priv;
+	struct net_device *bridge_dev;
+	unsigned int i;
+	u16 pvid, new_pvid;
+
+	/* Handle the case were multiple bridges span the same switch device
+	 * and one of them has a different setting than what is being requested
+	 * which would be breaking filtering semantics for any of the other
+	 * bridge devices.
+	 */
+	b53_for_each_port(dev, i) {
+		bridge_dev = dsa_to_port(ds, i)->bridge_dev;
+		if (bridge_dev &&
+		    bridge_dev != dsa_to_port(ds, port)->bridge_dev &&
+		    br_vlan_enabled(bridge_dev) != vlan_filtering) {
+			netdev_err(bridge_dev,
+				   "VLAN filtering is global to the switch!\n");
+			return -EINVAL;
+		}
+	}
+
+	b53_read16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port), &pvid);
+	new_pvid = pvid;
+	if (dev->vlan_filtering_enabled && !vlan_filtering) {
+		/* Filtering is currently enabled, use the default PVID since
+		 * the bridge does not expect tagging anymore
+		 */
+		dev->ports[port].pvid = pvid;
+		new_pvid = b53_default_pvid(dev);
+	} else if (!dev->vlan_filtering_enabled && vlan_filtering) {
+		/* Filtering is currently disabled, restore the previous PVID */
+		new_pvid = dev->ports[port].pvid;
+	}
+
+	if (pvid != new_pvid)
+		b53_write16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port),
+			    new_pvid);
+
+	b53_enable_vlan(dev, dev->vlan_enabled, vlan_filtering);
+
 	return 0;
 }
 EXPORT_SYMBOL(b53_vlan_filtering);
@@ -1280,7 +1329,7 @@ int b53_vlan_prepare(struct dsa_switch *ds, int port,
 	if (vlan->vid_end > dev->num_vlans)
 		return -ERANGE;
 
-	b53_enable_vlan(dev, true);
+	b53_enable_vlan(dev, true, dev->vlan_filtering_enabled);
 
 	return 0;
 }
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index ec796482792d..4dc7ee38b258 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -91,6 +91,7 @@ enum {
 struct b53_port {
 	u16		vlan_ctl_mask;
 	struct ethtool_eee eee;
+	u16		pvid;
 };
 
 struct b53_vlan {
@@ -137,6 +138,8 @@ struct b53_device {
 
 	unsigned int num_vlans;
 	struct b53_vlan *vlans;
+	bool vlan_enabled;
+	bool vlan_filtering_enabled;
 	unsigned int num_ports;
 	struct b53_port *ports;
 };
-- 
2.17.1

[PATCH net-next 04/14] net: systemport: Fix reception of BPDUs

[Florian Fainelli]

SYSTEMPORT has its RXCHK parser block that attempts to validate the
packet structures, unfortunately setting the L2 header check bit will
cause Bridge PDUs (BPDUs) to be incorrectly rejected because they look
like LLC/SNAP packets with a non-IPv4 or non-IPv6 Ethernet Type.

Fixes: 4e8aedfe78c7 ("net: systemport: Turn on offloads by default")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index f9521d0274b7..f374c8ddf719 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -134,6 +134,10 @@ static void bcm_sysport_set_rx_csum(struct net_device *dev,
 
 	priv->rx_chk_en = !!(wanted & NETIF_F_RXCSUM);
 	reg = rxchk_readl(priv, RXCHK_CONTROL);
+	/* Clear L2 header checks, which would prevent BPDUs
+	 * from being received.
+	 */
+	reg &= ~RXCHK_L2_HDR_DIS;
 	if (priv->rx_chk_en)
 		reg |= RXCHK_EN;
 	else
-- 
2.17.1

[PATCH net-next 05/14] net: dsa: b53: Define registers for IGMP snooping

[Florian Fainelli]

Define all necessary registers in order to implement IGMP snooping later
on, which are mostly comprised of the high-level protocol register
control definitions.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_regs.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/dsa/b53/b53_regs.h b/drivers/net/dsa/b53/b53_regs.h
index 2a9f421680aa..b4aecd4552b6 100644
--- a/drivers/net/dsa/b53/b53_regs.h
+++ b/drivers/net/dsa/b53/b53_regs.h
@@ -115,6 +115,8 @@
 #define B53_UC_FLOOD_MASK		0x32
 #define B53_MC_FLOOD_MASK		0x34
 #define B53_IPMC_FLOOD_MASK		0x36
+#define B53_DIS_LEARN			0x3c
+#define B53_SFT_LRN_CTRL		0x3e
 
 /*
  * Override Ports 0-7 State on devices with xMII interfaces (8 bit)
@@ -253,6 +255,26 @@
 /* Revision ID register (8 bit) */
 #define B53_REV_ID			0x40
 
+/* High-level Protocol Control Register (32 bit) */
+#define B53_HL_PRTC_CTRL		0x50
+#define  HL_PRTC_ARP_EN			(1 << 0)
+#define  HL_PRTC_RARP_EN		(1 << 1)
+#define  HL_PRTC_DHCP_EN		(1 << 2)
+#define  HL_PRTC_ICMPV4_EN		(1 << 3)
+#define  HL_PRTC_ICMPV6_EN		(1 << 4)
+#define  HL_PRTC_ICMPV6_FWD_MODE	(1 << 5)
+#define  HL_PRTC_IGMP_DIP_EN		(1 << 8)
+#define  HL_PRTC_IGMP_RPTLVE_EN		(1 << 9)
+#define  HL_PRTC_IGMP_RPTVLE_FWD_MODE	(1 << 10)
+#define  HL_PRTC_IGMP_QRY_EN		(1 << 11)
+#define  HL_PRTC_IGMP_QRY_FWD_MODE	(1 << 12)
+#define  HL_PRTC_IGMP_UKN_EN		(1 << 13)
+#define  HL_PRTC_IGMP_UKN_FWD_MODE	(1 << 14)
+#define  HL_PRTC_MLD_RPTDONE_EN		(1 << 15)
+#define  HL_PRTC_MLD_RPTDONE_FWD_MODE	(1 << 16)
+#define  HL_PRTC_MLD_QRY_EN		(1 << 17)
+#define  HL_PRTC_MLD_QRY_FWD_MODE	(1 << 18)
+
 /* Broadcom header RX control (16 bit) */
 #define B53_BRCM_HDR_RX_DIS		0x60
 
-- 
2.17.1

[PATCH net-next 06/14] net: dsa: b53: Add support for MDB

[Florian Fainelli]

In preparation for supporting IGMP snooping with or without the use of
a bridge, add support within b53_common.c to program the ARL entries for
multicast operations. The key difference is that a multicast ARL entry
is comprised of a bitmask of enabled ports, instead of a port number.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 62 ++++++++++++++++++++++++++++++--
 drivers/net/dsa/b53/b53_priv.h   |  8 ++++-
 2 files changed, 67 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 2fef4c564420..6c894ad4768a 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1503,11 +1503,25 @@ static int b53_arl_op(struct b53_device *dev, int op, int port,
 		idx = 1;
 	}
 
-	memset(&ent, 0, sizeof(ent));
-	ent.port = port;
+	/* For multicast address, the port is a bitmask and the validity
+	 * is determined by having at least one port being still active
+	 */
+	if (!is_multicast_ether_addr(addr)) {
+		ent.port = port;
+		ent.is_valid = is_valid;
+	} else {
+		if (is_valid)
+			ent.port |= BIT(port);
+		else
+			ent.port &= ~BIT(port);
+
+		ent.is_valid = !!(ent.port);
+	}
+
 	ent.is_valid = is_valid;
 	ent.vid = vid;
 	ent.is_static = true;
+	ent.is_age = false;
 	memcpy(ent.mac, addr, ETH_ALEN);
 	b53_arl_from_entry(&mac_vid, &fwd_entry, &ent);
 
@@ -1626,6 +1640,47 @@ int b53_fdb_dump(struct dsa_switch *ds, int port,
 }
 EXPORT_SYMBOL(b53_fdb_dump);
 
+int b53_mdb_prepare(struct dsa_switch *ds, int port,
+		    const struct switchdev_obj_port_mdb *mdb)
+{
+	struct b53_device *priv = ds->priv;
+
+	/* 5325 and 5365 require some more massaging, but could
+	 * be supported eventually
+	 */
+	if (is5325(priv) || is5365(priv))
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+EXPORT_SYMBOL(b53_mdb_prepare);
+
+void b53_mdb_add(struct dsa_switch *ds, int port,
+		 const struct switchdev_obj_port_mdb *mdb)
+{
+	struct b53_device *priv = ds->priv;
+	int ret;
+
+	ret = b53_arl_op(priv, 0, port, mdb->addr, mdb->vid, true);
+	if (ret)
+		dev_err(ds->dev, "failed to add MDB entry\n");
+}
+EXPORT_SYMBOL(b53_mdb_add);
+
+int b53_mdb_del(struct dsa_switch *ds, int port,
+		const struct switchdev_obj_port_mdb *mdb)
+{
+	struct b53_device *priv = ds->priv;
+	int ret;
+
+	ret = b53_arl_op(priv, 0, port, mdb->addr, mdb->vid, false);
+	if (ret)
+		dev_err(ds->dev, "failed to delete MDB entry\n");
+
+	return ret;
+}
+EXPORT_SYMBOL(b53_mdb_del);
+
 int b53_br_join(struct dsa_switch *ds, int port, struct net_device *br)
 {
 	struct b53_device *dev = ds->priv;
@@ -1969,6 +2024,9 @@ static const struct dsa_switch_ops b53_switch_ops = {
 	.port_fdb_del		= b53_fdb_del,
 	.port_mirror_add	= b53_mirror_add,
 	.port_mirror_del	= b53_mirror_del,
+	.port_mdb_prepare	= b53_mdb_prepare,
+	.port_mdb_add		= b53_mdb_add,
+	.port_mdb_del		= b53_mdb_del,
 };
 
 struct b53_chip_data {
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 4dc7ee38b258..620638ff9338 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -251,7 +251,7 @@ b53_build_op(write48, u64);
 b53_build_op(write64, u64);
 
 struct b53_arl_entry {
-	u8 port;
+	u16 port;
 	u8 mac[ETH_ALEN];
 	u16 vid;
 	u8 is_valid:1;
@@ -350,6 +350,12 @@ int b53_fdb_del(struct dsa_switch *ds, int port,
 		const unsigned char *addr, u16 vid);
 int b53_fdb_dump(struct dsa_switch *ds, int port,
 		 dsa_fdb_dump_cb_t *cb, void *data);
+int b53_mdb_prepare(struct dsa_switch *ds, int port,
+		    const struct switchdev_obj_port_mdb *mdb);
+void b53_mdb_add(struct dsa_switch *ds, int port,
+		 const struct switchdev_obj_port_mdb *mdb);
+int b53_mdb_del(struct dsa_switch *ds, int port,
+		const struct switchdev_obj_port_mdb *mdb);
 int b53_mirror_add(struct dsa_switch *ds, int port,
 		   struct dsa_mall_mirror_tc_entry *mirror, bool ingress);
 enum dsa_tag_protocol b53_get_tag_protocol(struct dsa_switch *ds, int port);
-- 
2.17.1

[PATCH net-next 07/14] net: dsa: Add ability to program multicast filter for CPU port

[Florian Fainelli]

When the switch ports operate as individual network devices, the switch
driver might have configured the switch to flood multicast all the way
to the CPU port. This is really undesireable as it can lead to receiving
a lot of unwanted traffic that the network stack needs to filter in
software.

For each valid multicast address, program it into the switch's MDB only
when the host is interested in receiving such traffic, e.g: running an
multicast application.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/slave.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index a3fcc1d01615..33f6b88b6fd6 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -68,6 +68,39 @@ static int dsa_slave_get_iflink(const struct net_device *dev)
 	return dsa_slave_to_master(dev)->ifindex;
 }
 
+static int dsa_slave_sync_unsync_mdb_addr(struct net_device *dev,
+					  const unsigned char *addr, bool add)
+{
+	struct switchdev_obj_port_mdb mdb = {
+		.obj = {
+			.id = SWITCHDEV_OBJ_ID_HOST_MDB,
+			.flags = SWITCHDEV_F_DEFER,
+		},
+		.vid = 0,
+	};
+	int ret = -EOPNOTSUPP;
+
+	ether_addr_copy(mdb.addr, addr);
+	if (add)
+		ret = switchdev_port_obj_add(dev, &mdb.obj, NULL);
+	else
+		ret = switchdev_port_obj_del(dev, &mdb.obj);
+
+	return ret;
+}
+
+static int dsa_slave_sync_mdb_addr(struct net_device *dev,
+				   const unsigned char *addr)
+{
+	return dsa_slave_sync_unsync_mdb_addr(dev, addr, true);
+}
+
+static int dsa_slave_unsync_mdb_addr(struct net_device *dev,
+				     const unsigned char *addr)
+{
+	return dsa_slave_sync_unsync_mdb_addr(dev, addr, false);
+}
+
 static int dsa_slave_open(struct net_device *dev)
 {
 	struct net_device *master = dsa_slave_to_master(dev);
@@ -126,6 +159,8 @@ static int dsa_slave_close(struct net_device *dev)
 
 	dev_mc_unsync(master, dev);
 	dev_uc_unsync(master, dev);
+	__hw_addr_unsync_dev(&dev->mc, dev, dsa_slave_unsync_mdb_addr);
+
 	if (dev->flags & IFF_ALLMULTI)
 		dev_set_allmulti(master, -1);
 	if (dev->flags & IFF_PROMISC)
@@ -150,7 +185,17 @@ static void dsa_slave_change_rx_flags(struct net_device *dev, int change)
 static void dsa_slave_set_rx_mode(struct net_device *dev)
 {
 	struct net_device *master = dsa_slave_to_master(dev);
+	struct dsa_port *dp = dsa_slave_to_port(dev);
 
+	/* If the port is bridged, the bridge takes care of sending
+	 * SWITCHDEV_OBJ_ID_HOST_MDB to program the host's MC filter
+	 */
+	if (netdev_mc_empty(dev) || dp->bridge_dev)
+		goto out;
+
+	__hw_addr_sync_dev(&dev->mc, dev, dsa_slave_sync_mdb_addr,
+			   dsa_slave_unsync_mdb_addr);
+out:
 	dev_mc_sync(master, dev);
 	dev_uc_sync(master, dev);
 }
@@ -1395,6 +1440,11 @@ static int dsa_slave_changeupper(struct net_device *dev,
 
 	if (netif_is_bridge_master(info->upper_dev)) {
 		if (info->linking) {
+			/* Remove existing MC addresses that might have been
+			 * programmed
+			 */
+			__hw_addr_unsync_dev(&dev->mc, dev,
+					     dsa_slave_unsync_mdb_addr);
 			err = dsa_port_bridge_join(dp, info->upper_dev);
 			err = notifier_from_errno(err);
 		} else {
-- 
2.17.1

[PATCH net-next 08/14] net: dsa: Add ndo_vlan_rx_{add,kill}_vid implementation

[Florian Fainelli]

In order to properly support VLAN filtering being enabled/disabled on a
bridge, while having other ports being non bridge port members, we need
to support the ndo_vlan_rx_{add,kill}_vid callbacks in order to make
sure the non-bridge ports can continue receiving VLAN tags, even when
the switch is globally configured to do ingress/egress VID checking.

We don't allow configuring VLAN devices on a bridge port member though,
since the bridge with VLAN awareness should be taking care of that, if
needed.

Since we can call dsa_port_vlan_{add,del} with a bridge_dev pointer
NULL, we now need to check that in these two functions.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/port.c  | 12 +++++++++--
 net/dsa/slave.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index 2d7e01b23572..185e85a4f5f0 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -252,7 +252,11 @@ int dsa_port_vlan_add(struct dsa_port *dp,
 		.vlan = vlan,
 	};
 
-	if (br_vlan_enabled(dp->bridge_dev))
+	/* Can be called from dsa_slave_port_obj_add() or
+	 * dsa_slave_vlan_rx_add_vid()
+	 */
+	if ((dp->bridge_dev && br_vlan_enabled(dp->bridge_dev)) ||
+	    !dp->bridge_dev)
 		return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_ADD, &info);
 
 	return 0;
@@ -270,7 +274,11 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 	if (netif_is_bridge_master(vlan->obj.orig_dev))
 		return -EOPNOTSUPP;
 
-	if (br_vlan_enabled(dp->bridge_dev))
+	/* Can be called from dsa_slave_port_obj_del() or
+	 * dsa_slave_vlan_rx_kill_vid()
+	 */
+	if ((dp->bridge_dev && br_vlan_enabled(dp->bridge_dev)) ||
+	    !dp->bridge_dev)
 		return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
 
 	return 0;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 33f6b88b6fd6..e266ef329583 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1027,6 +1027,54 @@ static int dsa_slave_get_ts_info(struct net_device *dev,
 	return ds->ops->get_ts_info(ds, p->dp->index, ts);
 }
 
+static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
+				     u16 vid)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct switchdev_obj_port_vlan vlan = { };
+	int ret = 0;
+
+	/* If the port is bridged and the bridge is VLAN aware, let the bridge
+	 * manage VLANs
+	 */
+	if (dp->bridge_dev && br_vlan_enabled(dp->bridge_dev))
+		return -EINVAL;
+
+	/* This API only allows programming tagged, non-PVID VIDs */
+	vlan.vid_begin = vid;
+	vlan.vid_end = vid;
+
+	ret = dsa_port_vlan_add(dp, &vlan, NULL);
+	if (ret == -EOPNOTSUPP)
+		ret = 0;
+
+	return ret;
+}
+
+static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
+				      u16 vid)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct switchdev_obj_port_vlan vlan = { };
+	int ret = 0;
+
+	/* If the port is bridged and the bridge is VLAN aware, let the bridge
+	 * manage VLANs
+	 */
+	if (dp->bridge_dev && br_vlan_enabled(dp->bridge_dev))
+		return -EINVAL;
+
+	/* This API only allows programming tagged, non-PVID VIDs */
+	vlan.vid_begin = vid;
+	vlan.vid_end = vid;
+
+	ret = dsa_port_vlan_del(dp, &vlan);
+	if (ret == -EOPNOTSUPP)
+		ret = 0;
+
+	return ret;
+}
+
 static const struct ethtool_ops dsa_slave_ethtool_ops = {
 	.get_drvinfo		= dsa_slave_get_drvinfo,
 	.get_regs_len		= dsa_slave_get_regs_len,
@@ -1090,6 +1138,8 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
 	.ndo_get_phys_port_name	= dsa_slave_get_phys_port_name,
 	.ndo_setup_tc		= dsa_slave_setup_tc,
 	.ndo_get_stats64	= dsa_slave_get_stats64,
+	.ndo_vlan_rx_add_vid	= dsa_slave_vlan_rx_add_vid,
+	.ndo_vlan_rx_kill_vid	= dsa_slave_vlan_rx_kill_vid,
 };
 
 static const struct switchdev_ops dsa_slave_switchdev_ops = {
@@ -1350,7 +1400,8 @@ int dsa_slave_create(struct dsa_port *port)
 	if (slave_dev == NULL)
 		return -ENOMEM;
 
-	slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
+	slave_dev->features = master->vlan_features | NETIF_F_HW_TC |
+				NETIF_F_HW_VLAN_CTAG_FILTER;
 	slave_dev->hw_features |= NETIF_F_HW_TC;
 	slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
 	eth_hw_addr_inherit(slave_dev, master);
-- 
2.17.1

[PATCH net-next 09/14] net: bridge: Propagate MC addresses with VID through switchdev

[Florian Fainelli]

In order for bridge port members to get a chance to implement unicast
and multicast address filtering correctly, which would matter for e.g:
switch network devices, synchronize the UC and MC lists down to the
individual bridge port members using switchdev HOST_MDB objects such
that this does not impact drivers that already have a ndo_set_rx_mode()
operation which likely already operate in promiscuous mode.

When the bridge has multicast snooping enabled, proper HOST_MDB
notifications will be sent through br_mdb_notify() already.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/bridge/br_device.c | 55 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 013323b6dbe4..ce10d6b7b151 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -18,6 +18,7 @@
 #include <linux/ethtool.h>
 #include <linux/list.h>
 #include <linux/netfilter_bridge.h>
+#include <net/switchdev.h>
 
 #include <linux/uaccess.h>
 #include "br_private.h"
@@ -182,8 +183,62 @@ static int br_dev_open(struct net_device *dev)
 	return 0;
 }
 
+static int bridge_sync_unsync_mc_addr(struct net_device *dev,
+				      const unsigned char *addr,
+				      bool add)
+{
+	struct net_bridge *br = netdev_priv(dev);
+	struct switchdev_obj_port_mdb mdb = {
+		.obj = {
+			.orig_dev = dev,
+			.id = SWITCHDEV_OBJ_ID_HOST_MDB,
+			.flags = SWITCHDEV_F_DEFER,
+		},
+		.vid = 0,
+	};
+	struct net_bridge_port *p;
+	int ret = -EOPNOTSUPP;
+
+#ifdef CONFIG_BRIDGE_VLAN_FILTERING
+	if (br_vlan_enabled(dev))
+		mdb.vid = br->default_pvid;
+#endif
+
+	ether_addr_copy(mdb.addr, addr);
+	spin_lock_bh(&br->lock);
+	list_for_each_entry(p, &br->port_list, list) {
+		if (add)
+			ret = switchdev_port_obj_add(p->dev, &mdb.obj, NULL);
+		else
+			ret = switchdev_port_obj_del(p->dev, &mdb.obj);
+		if (ret)
+			goto out;
+	}
+out:
+	spin_unlock_bh(&br->lock);
+	return ret;
+}
+
+static int bridge_sync_mc_addr(struct net_device *dev,
+			       const unsigned char *addr)
+{
+	return bridge_sync_unsync_mc_addr(dev, addr, true);
+}
+
+static int bridge_unsync_mc_addr(struct net_device *dev,
+				 const unsigned char *addr)
+{
+	return bridge_sync_unsync_mc_addr(dev, addr, false);
+}
+
 static void br_dev_set_multicast_list(struct net_device *dev)
 {
+	/* HOST_MDB notifications are sent through MDB notifications */
+	if (br_multicast_enabled(dev))
+		return;
+
+	__hw_addr_sync_dev(&dev->mc, dev, bridge_sync_mc_addr,
+			   bridge_unsync_mc_addr);
 }
 
 static void br_dev_change_rx_flags(struct net_device *dev, int change)
-- 
2.17.1

[PATCH net-next 10/14] net: vlan: Propagate MC addresses with VID through switchdev

[Florian Fainelli]

The VLAN real device could be an Ethernet switch port and that switch
might have VLAN filtering globally enabled (because of a bridge
requesting VLAN filtering on the switch on another port) and so when
programming multicast addresses, we need the multicast filter
programming to be aware of the correct VLAN ID as well.

Ethernet drivers that do not implement switchdev_port_{add,del}
operations and do not specifically check for SWITCHDEV_OBJ_ID_HOST_MDB
are not affected by that change.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/8021q/vlan_dev.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index b2d9c8f27cd7..ea2ef9d78dcb 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -312,6 +312,43 @@ static int vlan_dev_open(struct net_device *dev)
 	return err;
 }
 
+static int vlan_dev_sync_unsync_mc_addr(struct net_device *dev,
+                                        const unsigned char *addr,
+                                        bool add)
+{
+	struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
+	struct switchdev_obj_port_mdb mdb = {
+		.obj = {
+			.orig_dev = dev,
+			.id = SWITCHDEV_OBJ_ID_HOST_MDB,
+			.flags = SWITCHDEV_F_DEFER,
+		},
+		.vid = vlan_dev_vlan_id(dev),
+	};
+	int ret = -EOPNOTSUPP;
+
+	ether_addr_copy(mdb.addr, addr);
+        if (add)
+		ret = switchdev_port_obj_add(real_dev, &mdb.obj, NULL);
+        else
+		ret = switchdev_port_obj_del(real_dev, &mdb.obj);
+
+	return ret;
+}
+
+static int vlan_dev_sync_mc_addr(struct net_device *dev,
+                                 const unsigned char *addr)
+{
+	return vlan_dev_sync_unsync_mc_addr(dev, addr, true);
+}
+
+static int vlan_dev_unsync_mc_addr(struct net_device *dev,
+                                   const unsigned char *addr)
+{
+	return vlan_dev_sync_unsync_mc_addr(dev, addr, false);
+}
+
+
 static int vlan_dev_stop(struct net_device *dev)
 {
 	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
@@ -319,6 +356,7 @@ static int vlan_dev_stop(struct net_device *dev)
 
 	dev_mc_unsync(real_dev, dev);
 	dev_uc_unsync(real_dev, dev);
+	__hw_addr_unsync_dev(&dev->mc, dev, vlan_dev_unsync_mc_addr);
 	if (dev->flags & IFF_ALLMULTI)
 		dev_set_allmulti(real_dev, -1);
 	if (dev->flags & IFF_PROMISC)
@@ -483,6 +521,8 @@ static void vlan_dev_change_rx_flags(struct net_device *dev, int change)
 
 static void vlan_dev_set_rx_mode(struct net_device *vlan_dev)
 {
+	__hw_addr_sync_dev(&vlan_dev->mc, vlan_dev, vlan_dev_sync_mc_addr,
+			   vlan_dev_unsync_mc_addr);
 	dev_mc_sync(vlan_dev_priv(vlan_dev)->real_dev, vlan_dev);
 	dev_uc_sync(vlan_dev_priv(vlan_dev)->real_dev, vlan_dev);
 }
-- 
2.17.1

[PATCH net-next 11/14] net: dsa: Make VLAN filtering use DSA notifiers

[Florian Fainelli]

In preparation for allowing for global checks that would apply to the
entire switch and not just on a per-port basis, make the VLAN filtering
attribute follow other switchdev attributes/objects and make it use the
DSA notifier infrastructure.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/dsa_priv.h | 11 ++++++++++-
 net/dsa/port.c     | 17 +++++++----------
 net/dsa/switch.c   | 29 +++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 026a05774bf7..aad8acc70183 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -26,6 +26,7 @@ enum {
 	DSA_NOTIFIER_MDB_DEL,
 	DSA_NOTIFIER_VLAN_ADD,
 	DSA_NOTIFIER_VLAN_DEL,
+	DSA_NOTIFIER_VLAN_FILTERING,
 };
 
 /* DSA_NOTIFIER_AGEING_TIME */
@@ -57,7 +58,7 @@ struct dsa_notifier_mdb_info {
 	int port;
 };
 
-/* DSA_NOTIFIER_VLAN_* */
+/* DSA_NOTIFIER_VLAN_{ADD,DEL} */
 struct dsa_notifier_vlan_info {
 	const struct switchdev_obj_port_vlan *vlan;
 	struct switchdev_trans *trans;
@@ -65,6 +66,14 @@ struct dsa_notifier_vlan_info {
 	int port;
 };
 
+/* DSA_NOTIFIER_VLAN_FILTERING */
+struct dsa_notifier_vlan_filtering_info {
+	bool vlan_filtering;
+	struct switchdev_trans *trans;
+	int sw_index;
+	int port;
+};
+
 struct dsa_slave_priv {
 	/* Copy of CPU port xmit for faster access in slave transmit hot path */
 	struct sk_buff *	(*xmit)(struct sk_buff *skb,
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 185e85a4f5f0..d7b057d46460 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -146,17 +146,14 @@ void dsa_port_bridge_leave(struct dsa_port *dp, struct net_device *br)
 int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
 			    struct switchdev_trans *trans)
 {
-	struct dsa_switch *ds = dp->ds;
-
-	/* bridge skips -EOPNOTSUPP, so skip the prepare phase */
-	if (switchdev_trans_ph_prepare(trans))
-		return 0;
-
-	if (ds->ops->port_vlan_filtering)
-		return ds->ops->port_vlan_filtering(ds, dp->index,
-						    vlan_filtering);
+	struct dsa_notifier_vlan_filtering_info info = {
+		.sw_index = dp->ds->index,
+		.port = dp->index,
+		.trans = trans,
+		.vlan_filtering = vlan_filtering,
+	};
 
-	return 0;
+	return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_FILTERING, &info);
 }
 
 int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock,
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 142b294d3446..831334dc5e79 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -235,6 +235,32 @@ static int dsa_switch_vlan_del(struct dsa_switch *ds,
 	return 0;
 }
 
+static int dsa_switch_vlan_filtering(struct dsa_switch *ds,
+				     struct dsa_notifier_vlan_filtering_info *info)
+{
+	struct switchdev_trans *trans = info->trans;
+	bool vlan_filtering = info->vlan_filtering;
+	int port = info->port;
+	int err;
+
+	/* bridge skips -EOPNOTSUPP, so skip the prepare phase */
+	if (switchdev_trans_ph_prepare(trans))
+		return 0;
+
+	/* Build a mask of port members */
+	bitmap_zero(ds->bitmap, ds->num_ports);
+	if (ds->index == info->sw_index)
+		set_bit(port, ds->bitmap);
+
+	for_each_set_bit(port, ds->bitmap, ds->num_ports) {
+		err = ds->ops->port_vlan_filtering(ds, port, vlan_filtering);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 static int dsa_switch_event(struct notifier_block *nb,
 			    unsigned long event, void *info)
 {
@@ -269,6 +295,9 @@ static int dsa_switch_event(struct notifier_block *nb,
 	case DSA_NOTIFIER_VLAN_DEL:
 		err = dsa_switch_vlan_del(ds, info);
 		break;
+	case DSA_NOTIFIER_VLAN_FILTERING:
+		err = dsa_switch_vlan_filtering(ds, info);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
-- 
2.17.1

[PATCH net-next 12/14] net: dsa: Wire up multicast IGMP snooping attribute notification

[Florian Fainelli]

The bridge can at runtime be configured with or without IGMP snooping
enabled but we were not processing the switchdev attribute that notifies
about that toggle, do this now.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/net/dsa.h  |  2 ++
 net/dsa/dsa_priv.h | 11 +++++++++++
 net/dsa/port.c     | 13 +++++++++++++
 net/dsa/slave.c    |  4 ++++
 net/dsa/switch.c   | 28 ++++++++++++++++++++++++++++
 5 files changed, 58 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index b3eefe8e18fd..11cd4db3bc9e 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -479,6 +479,8 @@ struct dsa_switch_ops {
 	/*
 	 * Multicast database
 	 */
+	int	(*port_multicast_toggle)(struct dsa_switch *ds, int port,
+					 bool mc_disabled);
 	int (*port_mdb_prepare)(struct dsa_switch *ds, int port,
 				const struct switchdev_obj_port_mdb *mdb);
 	void (*port_mdb_add)(struct dsa_switch *ds, int port,
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index aad8acc70183..6f7ed2b3494f 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -27,6 +27,7 @@ enum {
 	DSA_NOTIFIER_VLAN_ADD,
 	DSA_NOTIFIER_VLAN_DEL,
 	DSA_NOTIFIER_VLAN_FILTERING,
+	DSA_NOTIFIER_MC_DISABLED,
 };
 
 /* DSA_NOTIFIER_AGEING_TIME */
@@ -74,6 +75,14 @@ struct dsa_notifier_vlan_filtering_info {
 	int port;
 };
 
+/* DSA_NOTIFIER_MC_DISABLED */
+struct dsa_notifier_mc_disabled_info {
+	bool mc_disabled;
+	struct switchdev_trans *trans;
+	int sw_index;
+	int port;
+};
+
 struct dsa_slave_priv {
 	/* Copy of CPU port xmit for faster access in slave transmit hot path */
 	struct sk_buff *	(*xmit)(struct sk_buff *skb,
@@ -154,6 +163,8 @@ int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy);
 void dsa_port_disable(struct dsa_port *dp, struct phy_device *phy);
 int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br);
 void dsa_port_bridge_leave(struct dsa_port *dp, struct net_device *br);
+int dsa_port_multicast_toggle(struct dsa_port *dp, bool mc_disabled,
+			      struct switchdev_trans *trans);
 int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
 			    struct switchdev_trans *trans);
 int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock,
diff --git a/net/dsa/port.c b/net/dsa/port.c
index d7b057d46460..148458941b51 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -143,6 +143,19 @@ void dsa_port_bridge_leave(struct dsa_port *dp, struct net_device *br)
 	dsa_port_set_state_now(dp, BR_STATE_FORWARDING);
 }
 
+int dsa_port_multicast_toggle(struct dsa_port *dp, bool mc_disabled,
+			      struct switchdev_trans *trans)
+{
+	struct dsa_notifier_mc_disabled_info info = {
+		.sw_index = dp->ds->index,
+		.port = dp->index,
+		.trans = trans,
+		.mc_disabled = mc_disabled,
+	};
+
+	return dsa_port_notify(dp, DSA_NOTIFIER_MC_DISABLED, &info);
+}
+
 int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
 			    struct switchdev_trans *trans)
 {
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index e266ef329583..acb7f1830e98 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -337,6 +337,10 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
 	case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME:
 		ret = dsa_port_ageing_time(dp, attr->u.ageing_time, trans);
 		break;
+	case SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED:
+		ret = dsa_port_multicast_toggle(dp, attr->u.mc_disabled,
+						trans);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 831334dc5e79..e095eb808434 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -261,6 +261,31 @@ static int dsa_switch_vlan_filtering(struct dsa_switch *ds,
 	return 0;
 }
 
+static int dsa_switch_mc_disabled(struct dsa_switch *ds,
+				  struct dsa_notifier_mc_disabled_info *info)
+{
+	struct switchdev_trans *trans = info->trans;
+	bool mc_disabled = info->mc_disabled;
+	int port = info->port;
+	int err;
+
+	if (switchdev_trans_ph_prepare(trans))
+		return ds->ops->port_multicast_toggle ? 0 : -EOPNOTSUPP;
+
+	/* Build a mask of port members */
+	bitmap_zero(ds->bitmap, ds->num_ports);
+	if (ds->index == info->sw_index)
+		set_bit(port, ds->bitmap);
+
+	for_each_set_bit(port, ds->bitmap, ds->num_ports) {
+		err = ds->ops->port_multicast_toggle(ds, port, mc_disabled);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 static int dsa_switch_event(struct notifier_block *nb,
 			    unsigned long event, void *info)
 {
@@ -298,6 +323,9 @@ static int dsa_switch_event(struct notifier_block *nb,
 	case DSA_NOTIFIER_VLAN_FILTERING:
 		err = dsa_switch_vlan_filtering(ds, info);
 		break;
+	case DSA_NOTIFIER_MC_DISABLED:
+		err = dsa_switch_mc_disabled(ds, info);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
-- 
2.17.1

[PATCH net-next 13/14] net: dsa: b53: Add support for toggling IGMP snooping

[Florian Fainelli]

Add the required configuration knobs to honor the turning off of IGMP
snooping (typically through the bridge interface) which means that when
IGMP snooping is off, we must be flooding mutlicast since we do not get
any notifications about IGMP join/leave through the network stack
running on the bridge.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 30 ++++++++++++++++++++++++++++++
 drivers/net/dsa/b53/b53_priv.h   |  2 ++
 2 files changed, 32 insertions(+)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 6c894ad4768a..2c9f6f6abdf3 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1640,6 +1640,35 @@ int b53_fdb_dump(struct dsa_switch *ds, int port,
 }
 EXPORT_SYMBOL(b53_fdb_dump);
 
+int b53_multicast_toggle(struct dsa_switch *ds, int port,
+			 bool mc_disabled)
+{
+	struct b53_device *dev = ds->priv;
+	u16 mc_ctrl;
+
+	if (is5325(dev) || is5365(dev))
+		return -EOPNOTSUPP;
+
+	/* Allow port to flood multicast */
+	b53_read16(dev, B53_CTRL_PAGE, B53_MC_FLOOD_MASK, &mc_ctrl);
+	if (mc_disabled)
+		mc_ctrl |= BIT(port);
+	else
+		mc_ctrl &= ~BIT(port);
+	b53_write16(dev, B53_CTRL_PAGE, B53_MC_FLOOD_MASK, mc_ctrl);
+
+	/* And flood IP multicast as well */
+	b53_read16(dev, B53_CTRL_PAGE, B53_IPMC_FLOOD_MASK, &mc_ctrl);
+	if (mc_disabled)
+		mc_ctrl |= BIT(port);
+	else
+		mc_ctrl &= ~BIT(port);
+	b53_write16(dev, B53_CTRL_PAGE, B53_IPMC_FLOOD_MASK, mc_ctrl);
+
+	return 0;
+}
+EXPORT_SYMBOL(b53_multicast_toggle);
+
 int b53_mdb_prepare(struct dsa_switch *ds, int port,
 		    const struct switchdev_obj_port_mdb *mdb)
 {
@@ -2025,6 +2054,7 @@ static const struct dsa_switch_ops b53_switch_ops = {
 	.port_mirror_add	= b53_mirror_add,
 	.port_mirror_del	= b53_mirror_del,
 	.port_mdb_prepare	= b53_mdb_prepare,
+	.port_multicast_toggle	= b53_multicast_toggle,
 	.port_mdb_add		= b53_mdb_add,
 	.port_mdb_del		= b53_mdb_del,
 };
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 620638ff9338..cd259fb8b00e 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -350,6 +350,8 @@ int b53_fdb_del(struct dsa_switch *ds, int port,
 		const unsigned char *addr, u16 vid);
 int b53_fdb_dump(struct dsa_switch *ds, int port,
 		 dsa_fdb_dump_cb_t *cb, void *data);
+int b53_multicast_toggle(struct dsa_switch *ds, int port,
+			 bool mc_disabled);
 int b53_mdb_prepare(struct dsa_switch *ds, int port,
 		    const struct switchdev_obj_port_mdb *mdb);
 void b53_mdb_add(struct dsa_switch *ds, int port,
-- 
2.17.1

[PATCH net-next 14/14] net: dsa: bcm_sf2: Enable management mode

[Florian Fainelli]

Now that we have all the necessary plumbing in place to get notified
when a multicast MAC address must be programmed, configure the switch to
oeprate in managed mode and let the network stack learn about management
traffic.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 39 ++++++++++++++++++++--
 drivers/net/dsa/b53/b53_priv.h   |  1 +
 drivers/net/dsa/bcm_sf2.c        | 56 +++++++++++++++++++++++---------
 drivers/net/dsa/bcm_sf2_regs.h   |  5 +++
 4 files changed, 84 insertions(+), 17 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 2c9f6f6abdf3..37bc25107158 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -364,8 +364,6 @@ static void b53_enable_vlan(struct b53_device *dev, bool enable,
 		b53_read8(dev, B53_VLAN_PAGE, B53_VLAN_CTRL5, &vc5);
 	}
 
-	mgmt &= ~SM_SW_FWD_MODE;
-
 	if (enable) {
 		vc0 |= VC0_VLAN_EN | VC0_VID_CHK_EN | VC0_VID_HASH_VID;
 		vc1 |= VC1_RX_MCST_UNTAG_EN | VC1_RX_MCST_FWD_EN;
@@ -490,6 +488,43 @@ static int b53_fast_age_vlan(struct b53_device *dev, u16 vid)
 	return b53_flush_arl(dev, FAST_AGE_VLAN);
 }
 
+void b53_port_learn_setup(struct dsa_switch *ds, int port)
+{
+	struct b53_device *dev = ds->priv;
+	u16 reg;
+
+	/* Enable learning */
+	b53_read16(dev, B53_CTRL_PAGE, B53_DIS_LEARN, &reg);
+	reg &= ~BIT(port);
+	b53_write16(dev, B53_CTRL_PAGE, B53_DIS_LEARN, reg);
+
+	/* Software learning control disabled */
+	b53_read16(dev, B53_CTRL_PAGE, B53_SFT_LRN_CTRL, &reg);
+	reg &= ~BIT(port);
+	b53_write16(dev, B53_CTRL_PAGE, B53_SFT_LRN_CTRL, reg);
+
+	/* Configure IP multicast, allow Unicast ARL misses to be forwarded */
+	b53_read16(dev, B53_CTRL_PAGE, B53_IP_MULTICAST_CTRL, &reg);
+	reg |= B53_IPMC_FWD_EN | B53_UC_FWD_EN;
+	b53_write16(dev, B53_CTRL_PAGE, B53_IP_MULTICAST_CTRL, reg);
+
+	/* Set port in Unicast lookup forward map */
+	b53_read16(dev, B53_CTRL_PAGE, B53_UC_FLOOD_MASK, &reg);
+	reg |= BIT(port);
+	b53_write16(dev, B53_CTRL_PAGE, B53_UC_FLOOD_MASK, reg);
+
+	/* Do not set port in Multicast lookup forward map, learn */
+	b53_read16(dev, B53_CTRL_PAGE, B53_MC_FLOOD_MASK, &reg);
+	reg &= ~BIT(port);
+	b53_write16(dev, B53_CTRL_PAGE, B53_MC_FLOOD_MASK, reg);
+
+	/* Do not set port in IP multicast lookup formward map, learn */
+	b53_read16(dev, B53_CTRL_PAGE, B53_IPMC_FLOOD_MASK, &reg);
+	reg &= ~BIT(port);
+	b53_write16(dev, B53_CTRL_PAGE, B53_IPMC_FLOOD_MASK, reg);
+}
+EXPORT_SYMBOL(b53_port_learn_setup);
+
 void b53_imp_vlan_setup(struct dsa_switch *ds, int cpu_port)
 {
 	struct b53_device *dev = ds->priv;
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index cd259fb8b00e..1806304c38cc 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -309,6 +309,7 @@ static inline int b53_switch_get_reset_gpio(struct b53_device *dev)
 #endif
 
 /* Exported functions towards other drivers */
+void b53_port_learn_setup(struct dsa_switch *ds, int port);
 void b53_imp_vlan_setup(struct dsa_switch *ds, int cpu_port);
 int b53_configure_vlan(struct dsa_switch *ds);
 void b53_get_strings(struct dsa_switch *ds, int port, u32 stringset,
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 361fbde76654..c9e6ffb737a4 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -51,19 +51,19 @@ static void bcm_sf2_imp_setup(struct dsa_switch *ds, int port)
 	reg &= ~P_TXQ_PSM_VDD(port);
 	core_writel(priv, reg, CORE_MEM_PSM_VDD_CTRL);
 
-	/* Enable Broadcast, Multicast, Unicast forwarding to IMP port */
-	reg = core_readl(priv, CORE_IMP_CTL);
-	reg |= (RX_BCST_EN | RX_MCST_EN | RX_UCST_EN);
-	reg &= ~(RX_DIS | TX_DIS);
-	core_writel(priv, reg, CORE_IMP_CTL);
+	/* Enable forwarding and managed mode */
+	core_writel(priv, SW_FWDG_EN | SW_FWDG_MODE, CORE_SWMODE);
 
-	/* Enable forwarding */
-	core_writel(priv, SW_FWDG_EN, CORE_SWMODE);
+	/* Configure port for learning */
+	b53_port_learn_setup(ds, port);
 
-	/* Enable IMP port in dumb mode */
-	reg = core_readl(priv, CORE_SWITCH_CTRL);
-	reg |= MII_DUMB_FWDG_EN;
-	core_writel(priv, reg, CORE_SWITCH_CTRL);
+	/* Enable IGMP and MLD high-level protocol snooping support */
+	reg = HL_PRTC_IGMP_RPTLVE_EN | HL_PRTC_IGMP_RPTVLE_FWD_MODE |
+	      HL_PRTC_IGMP_QRY_EN | HL_PRTC_IGMP_QRY_FWD_MODE |
+	      HL_PRTC_IGMP_UKN_EN | HL_PRTC_IGMP_UKN_FWD_MODE |
+	      HL_PRTC_MLD_RPTDONE_EN | HL_PRTC_MLD_RPTDONE_FWD_MODE |
+	      HL_PRTC_MLD_QRY_EN | HL_PRTC_MLD_QRY_FWD_MODE;
+	b53_write32(priv->dev, B53_MGMT_PAGE, B53_HL_PRTC_CTRL, reg);
 
 	/* Configure Traffic Class to QoS mapping, allow each priority to map
 	 * to a different queue number
@@ -75,10 +75,26 @@ static void bcm_sf2_imp_setup(struct dsa_switch *ds, int port)
 
 	b53_brcm_hdr_setup(ds, port);
 
+	/* Set IMP0 or IMP1 port to be managed port, enable BPDU */
+	reg = core_readl(priv, CORE_GMNCFGCFG);
+	reg &= ~(FRM_MGNP_MASK << FRM_MGNP_SHIFT);
+	if (port == core_readl(priv, CORE_IMP0_PRT_ID))
+		reg |= FRM_MNGP_IMP0 << FRM_MGNP_SHIFT;
+	if (port == core_readl(priv, CORE_IMP1_PRT_ID))
+		reg |= FRM_MGNP_IMP_DUAL << FRM_MGNP_SHIFT;
+	reg |= RXBPDU_EN;
+	core_writel(priv, reg, CORE_GMNCFGCFG);
+
 	/* Force link status for IMP port */
 	reg = core_readl(priv, offset);
 	reg |= (MII_SW_OR | LINK_STS);
 	core_writel(priv, reg, offset);
+
+	/* Enable Broadcast, Unicast forwarding to IMP port */
+	reg = core_readl(priv, CORE_IMP_CTL);
+	reg |= (RX_BCST_EN | RX_UCST_EN);
+	reg &= ~(RX_DIS | TX_DIS);
+	core_writel(priv, reg, CORE_IMP_CTL);
 }
 
 static void bcm_sf2_gphy_enable_set(struct dsa_switch *ds, bool enable)
@@ -166,10 +182,8 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
 	reg &= ~P_TXQ_PSM_VDD(port);
 	core_writel(priv, reg, CORE_MEM_PSM_VDD_CTRL);
 
-	/* Enable learning */
-	reg = core_readl(priv, CORE_DIS_LEARN);
-	reg &= ~BIT(port);
-	core_writel(priv, reg, CORE_DIS_LEARN);
+	/* Configure port for learning */
+	b53_port_learn_setup(ds, port);
 
 	/* Enable Broadcom tags for that port if requested */
 	if (priv->brcm_tag_mask & BIT(port))
@@ -683,6 +697,7 @@ static int bcm_sf2_sw_suspend(struct dsa_switch *ds)
 {
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
 	unsigned int port;
+	u32 reg;
 
 	bcm_sf2_intr_disable(priv);
 
@@ -695,6 +710,13 @@ static int bcm_sf2_sw_suspend(struct dsa_switch *ds)
 			bcm_sf2_port_disable(ds, port, NULL);
 	}
 
+	/* Disable management mode since we won't be able to
+	 * perform any tasks while being suspended.
+	 */
+	reg = core_readl(priv, CORE_SWMODE);
+	reg &= ~SW_FWDG_MODE;
+	core_writel(priv, reg, CORE_SWMODE);
+
 	return 0;
 }
 
@@ -930,6 +952,10 @@ static const struct dsa_switch_ops bcm_sf2_ops = {
 	.set_rxnfc		= bcm_sf2_set_rxnfc,
 	.port_mirror_add	= b53_mirror_add,
 	.port_mirror_del	= b53_mirror_del,
+	.port_multicast_toggle	= b53_multicast_toggle,
+	.port_mdb_prepare	= b53_mdb_prepare,
+	.port_mdb_add		= b53_mdb_add,
+	.port_mdb_del		= b53_mdb_del,
 };
 
 struct bcm_sf2_of_data {
diff --git a/drivers/net/dsa/bcm_sf2_regs.h b/drivers/net/dsa/bcm_sf2_regs.h
index 0a1e530d52b7..211db9a2e9e9 100644
--- a/drivers/net/dsa/bcm_sf2_regs.h
+++ b/drivers/net/dsa/bcm_sf2_regs.h
@@ -222,8 +222,13 @@ enum bcm_sf2_reg_offs {
 #define CORE_GMNCFGCFG			0x0800
 #define  RST_MIB_CNT			(1 << 0)
 #define  RXBPDU_EN			(1 << 1)
+#define  FRM_MGNP_SHIFT			6
+#define  FRM_MGNP_MASK			0x3
+#define  FRM_MNGP_IMP0			2
+#define  FRM_MGNP_IMP_DUAL		3
 
 #define CORE_IMP0_PRT_ID		0x0804
+#define CORE_IMP1_PRT_ID		0x0808
 
 #define CORE_RST_MIB_CNT_EN		0x0950
 
-- 
2.17.1

Re: [PATCH net-next 01/14] net: bridge: multicast: Propagate br_mc_disabled_update() return

[Ido Schimmel]

On Wed, Jan 16, 2019 at 12:00:49PM -0800, Florian Fainelli wrote:
> Some Ethernet switches might not be able to support disabling multicast
> flooding globally when e.g: several bridges span the same physical
> device, propagate the return value of br_mc_disabled_update() such that
> this propagates correctly to user-space.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  net/bridge/br_multicast.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 3aeff0895669..09fc92541873 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -813,7 +813,7 @@ static void br_ip6_multicast_port_query_expired(struct timer_list *t)
>  }
>  #endif
>  
> -static void br_mc_disabled_update(struct net_device *dev, bool value)
> +static int br_mc_disabled_update(struct net_device *dev, bool value)
>  {
>  	struct switchdev_attr attr = {
>  		.orig_dev = dev,
> @@ -822,11 +822,13 @@ static void br_mc_disabled_update(struct net_device *dev, bool value)
>  		.u.mc_disabled = !value,
>  	};
>  
> -	switchdev_port_attr_set(dev, &attr);
> +	return switchdev_port_attr_set(dev, &attr);

Did you test this for veth? If switchdev ops are not defined, this
returns -EOPNOTSUPP. See __br_vlan_filter_toggle() for reference

>  }
>  
>  int br_multicast_add_port(struct net_bridge_port *port)
>  {
> +	int ret;
> +
>  	port->multicast_router = MDB_RTR_TYPE_TEMP_QUERY;
>  
>  	timer_setup(&port->multicast_router_timer,
> @@ -837,8 +839,11 @@ int br_multicast_add_port(struct net_bridge_port *port)
>  	timer_setup(&port->ip6_own_query.timer,
>  		    br_ip6_multicast_port_query_expired, 0);
>  #endif
> -	br_mc_disabled_update(port->dev,
> -			      br_opt_get(port->br, BROPT_MULTICAST_ENABLED));
> +	ret = br_mc_disabled_update(port->dev,
> +				    br_opt_get(port->br,
> +					       BROPT_MULTICAST_ENABLED));
> +	if (ret)
> +		return ret;
>  
>  	port->mcast_stats = netdev_alloc_pcpu_stats(struct bridge_mcast_stats);
>  	if (!port->mcast_stats)
> @@ -1937,12 +1942,16 @@ static void br_multicast_start_querier(struct net_bridge *br,
>  int br_multicast_toggle(struct net_bridge *br, unsigned long val)
>  {
>  	struct net_bridge_port *port;
> +	int err;
>  
>  	spin_lock_bh(&br->multicast_lock);
>  	if (!!br_opt_get(br, BROPT_MULTICAST_ENABLED) == !!val)
>  		goto unlock;
>  
> -	br_mc_disabled_update(br->dev, val);
> +	err = br_mc_disabled_update(br->dev, val);
> +	if (err)
> +		goto unlock;
> +
>  	br_opt_toggle(br, BROPT_MULTICAST_ENABLED, !!val);
>  	if (!br_opt_get(br, BROPT_MULTICAST_ENABLED))
>  		goto unlock;

You never return the error

> -- 
> 2.17.1
> 

Re: [PATCH net-next 09/14] net: bridge: Propagate MC addresses with VID through switchdev

[Ido Schimmel]

On Wed, Jan 16, 2019 at 12:00:57PM -0800, Florian Fainelli wrote:
> In order for bridge port members to get a chance to implement unicast
> and multicast address filtering correctly, which would matter for e.g:
> switch network devices, synchronize the UC and MC lists down to the
> individual bridge port members using switchdev HOST_MDB objects such
> that this does not impact drivers that already have a ndo_set_rx_mode()
> operation which likely already operate in promiscuous mode.
> 
> When the bridge has multicast snooping enabled, proper HOST_MDB
> notifications will be sent through br_mdb_notify() already.

I don't understand the change. HOST_MDB is used to notify underlying
drivers about MDB entries that should be configured to locally receive
packets. This is triggered by the transmission of an IGMP report through
the bridge, for example.

It seems that you're trying to overload HOST_MDB with multicast address
filtering on bridge ports? Why are you performing this filtering?
Shouldn't you allow all MAC addresses to ingress?

Re: [PATCH net-next 10/14] net: vlan: Propagate MC addresses with VID through switchdev

[Ido Schimmel]

On Wed, Jan 16, 2019 at 12:00:58PM -0800, Florian Fainelli wrote:
> The VLAN real device could be an Ethernet switch port and that switch
> might have VLAN filtering globally enabled (because of a bridge
> requesting VLAN filtering on the switch on another port) and so when
> programming multicast addresses, we need the multicast filter
> programming to be aware of the correct VLAN ID as well.

This looks like a quirk of a specific device. How bad is it to patch the
driver to add a multicast address for every configured VLAN?

Also, I think it's weird that we have one API to program address and a
completely different API (via switchdev) to program address+VID pairs.
Extending current API might make more sense.

Re: [PATCH net-next 03/14] net: dsa: b53: Properly account for VLAN filtering

[Vivien Didelot]

Hi Florian,

On Wed, 16 Jan 2019 12:00:51 -0800, Florian Fainelli <f.fainelli@gmail.com> wrote:

> +	/* Handle the case were multiple bridges span the same switch device
> +	 * and one of them has a different setting than what is being requested
> +	 * which would be breaking filtering semantics for any of the other
> +	 * bridge devices.
> +	 */
> +	b53_for_each_port(dev, i) {
> +		bridge_dev = dsa_to_port(ds, i)->bridge_dev;
> +		if (bridge_dev &&
> +		    bridge_dev != dsa_to_port(ds, port)->bridge_dev &&
> +		    br_vlan_enabled(bridge_dev) != vlan_filtering) {
> +			netdev_err(bridge_dev,
> +				   "VLAN filtering is global to the switch!\n");
> +			return -EINVAL;
> +		}
> +	}
 
Unbridged ports must act as standard NICs and thus forward taggued frames.
What happens to them if there's a bridge with VLAN filtering enabled spawned
on other ports of your switch? Will the unbridged ports filter VLAN?


Thanks,

	Vivien

Re: [PATCH net-next 03/14] net: dsa: b53: Properly account for VLAN filtering

[Florian Fainelli]

On 1/17/19 8:36 AM, Vivien Didelot wrote:
> Hi Florian,
> 
> On Wed, 16 Jan 2019 12:00:51 -0800, Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
>> +	/* Handle the case were multiple bridges span the same switch device
>> +	 * and one of them has a different setting than what is being requested
>> +	 * which would be breaking filtering semantics for any of the other
>> +	 * bridge devices.
>> +	 */
>> +	b53_for_each_port(dev, i) {
>> +		bridge_dev = dsa_to_port(ds, i)->bridge_dev;
>> +		if (bridge_dev &&
>> +		    bridge_dev != dsa_to_port(ds, port)->bridge_dev &&
>> +		    br_vlan_enabled(bridge_dev) != vlan_filtering) {
>> +			netdev_err(bridge_dev,
>> +				   "VLAN filtering is global to the switch!\n");
>> +			return -EINVAL;
>> +		}
>> +	}
>  
> Unbridged ports must act as standard NICs and thus forward taggued frames.
> What happens to them if there's a bridge with VLAN filtering enabled spawned
> on other ports of your switch? Will the unbridged ports filter VLAN?
Because VLAN filtering a global setting to the switch, unbridged network
ports will effectively have VLAN filtering enabled, which is why the
ndo_vlan_rx_{add,kill}_vid functions to permit that use case.
-- 
Florian

Re: [PATCH net-next 12/14] net: dsa: Wire up multicast IGMP snooping attribute notification

[Vivien Didelot]

On Wed, 16 Jan 2019 12:01:00 -0800, Florian Fainelli <f.fainelli@gmail.com> wrote:

> +	int	(*port_multicast_toggle)(struct dsa_switch *ds, int port,
> +					 bool mc_disabled);
 
Waw this looks counter-intuitive and error-prone... Would you prefer to make it
symmetrical to ops->port_vlan_filtering() by implementing ops->port_multicast()
and passing !obj->u.mc_disabled to it?


Thanks,

	Vivien

Re: [PATCH net-next 03/14] net: dsa: b53: Properly account for VLAN filtering

[Vivien Didelot]

On Thu, 17 Jan 2019 09:48:53 -0800, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 1/17/19 8:36 AM, Vivien Didelot wrote:
> > Hi Florian,
> > 
> > On Wed, 16 Jan 2019 12:00:51 -0800, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > 
> >> +	/* Handle the case were multiple bridges span the same switch device
> >> +	 * and one of them has a different setting than what is being requested
> >> +	 * which would be breaking filtering semantics for any of the other
> >> +	 * bridge devices.
> >> +	 */
> >> +	b53_for_each_port(dev, i) {
> >> +		bridge_dev = dsa_to_port(ds, i)->bridge_dev;
> >> +		if (bridge_dev &&
> >> +		    bridge_dev != dsa_to_port(ds, port)->bridge_dev &&
> >> +		    br_vlan_enabled(bridge_dev) != vlan_filtering) {
> >> +			netdev_err(bridge_dev,
> >> +				   "VLAN filtering is global to the switch!\n");
> >> +			return -EINVAL;
> >> +		}
> >> +	}
> >  
> > Unbridged ports must act as standard NICs and thus forward taggued frames.
> > What happens to them if there's a bridge with VLAN filtering enabled spawned
> > on other ports of your switch? Will the unbridged ports filter VLAN?
> Because VLAN filtering a global setting to the switch, unbridged network
> ports will effectively have VLAN filtering enabled, which is why the
> ndo_vlan_rx_{add,kill}_vid functions to permit that use case.

But then vlan_filtering must simply not be allowed on your switch if you
have unbridged ports, no?

I might be mixing things up here but I don't understand yet how you can
have bridged and unbridged ports working correctly on your switch when it
has global VLAN filtering turned on. I understand that the switch will drop
the tagged frames on ingress.


Thanks,

	Vivien

Re: [PATCH net-next 03/14] net: dsa: b53: Properly account for VLAN filtering

[Florian Fainelli]

On 1/17/19 10:47 AM, Vivien Didelot wrote:
> On Thu, 17 Jan 2019 09:48:53 -0800, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 1/17/19 8:36 AM, Vivien Didelot wrote:
>>> Hi Florian,
>>>
>>> On Wed, 16 Jan 2019 12:00:51 -0800, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>
>>>> +	/* Handle the case were multiple bridges span the same switch device
>>>> +	 * and one of them has a different setting than what is being requested
>>>> +	 * which would be breaking filtering semantics for any of the other
>>>> +	 * bridge devices.
>>>> +	 */
>>>> +	b53_for_each_port(dev, i) {
>>>> +		bridge_dev = dsa_to_port(ds, i)->bridge_dev;
>>>> +		if (bridge_dev &&
>>>> +		    bridge_dev != dsa_to_port(ds, port)->bridge_dev &&
>>>> +		    br_vlan_enabled(bridge_dev) != vlan_filtering) {
>>>> +			netdev_err(bridge_dev,
>>>> +				   "VLAN filtering is global to the switch!\n");
>>>> +			return -EINVAL;
>>>> +		}
>>>> +	}
>>>  
>>> Unbridged ports must act as standard NICs and thus forward taggued frames.
>>> What happens to them if there's a bridge with VLAN filtering enabled spawned
>>> on other ports of your switch? Will the unbridged ports filter VLAN?
>> Because VLAN filtering a global setting to the switch, unbridged network
>> ports will effectively have VLAN filtering enabled, which is why the
>> ndo_vlan_rx_{add,kill}_vid functions to permit that use case.
> 
> But then vlan_filtering must simply not be allowed on your switch if you
> have unbridged ports, no?

VLAN filtering is an attribute that applies to the bridge, not the
non-bridged network ports.

> 
> I might be mixing things up here but I don't understand yet how you can
> have bridged and unbridged ports working correctly on your switch when it
> has global VLAN filtering turned on. I understand that the switch will drop
> the tagged frames on ingress.

With ndo_vlan_rx_{add,kill}_vid(), you get proper VLAN entries
programmed into the switch for non-bridged ports, therefore your switch
can allow VLAN tagged traffic (which these interfaces only support) just
fine, even when a bridge has VLAN filtering turned on on a bridge device
that has other switch ports enslaved.
-- 
Florian

Re: [PATCH net-next 12/14] net: dsa: Wire up multicast IGMP snooping attribute notification

[Florian Fainelli]

On 1/17/19 10:36 AM, Vivien Didelot wrote:
> On Wed, 16 Jan 2019 12:01:00 -0800, Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
>> +	int	(*port_multicast_toggle)(struct dsa_switch *ds, int port,
>> +					 bool mc_disabled);
>  
> Waw this looks counter-intuitive and error-prone... Would you prefer to make it
> symmetrical to ops->port_vlan_filtering() by implementing ops->port_multicast()
> and passing !obj->u.mc_disabled to it?

I debated doing that, but I think it is better to have the same boolean
polarity from end to end, even if it is highly counter intuitive as you
said. That is kind of the story of bridge attributes annyway, completely
counter intuitive.
-- 
Florian

Re: [PATCH net-next 10/14] net: vlan: Propagate MC addresses with VID through switchdev

[Florian Fainelli]

On 1/17/19 6:49 AM, Ido Schimmel wrote:
> On Wed, Jan 16, 2019 at 12:00:58PM -0800, Florian Fainelli wrote:
>> The VLAN real device could be an Ethernet switch port and that switch
>> might have VLAN filtering globally enabled (because of a bridge
>> requesting VLAN filtering on the switch on another port) and so when
>> programming multicast addresses, we need the multicast filter
>> programming to be aware of the correct VLAN ID as well.
> 
> This looks like a quirk of a specific device. How bad is it to patch the
> driver to add a multicast address for every configured VLAN?

There is at least another driver that can be benefit from that which is
cpsw, if I understand Ivan's use case correctly.

If there is a ndo_set_rx_mode() function implemented by the virtual
device, and that does call dev_{mc,uc}_sync(master, dev), then this
means that you do want to be able to filter UC and MC addresses. If we
added the entire class D range of multicast addresses to the switch's
MDB, that would not be filtering, we would be passing up everything to
the stack and let it filter in software because there is no multicast
socket listening on that address.

> 
> Also, I think it's weird that we have one API to program address and a
> completely different API (via switchdev) to program address+VID pairs.
> Extending current API might make more sense.
> 

Do you mean ndo_set_rx_mode() and dev_mc_sync()? That is what Ivan
proposed doing not so long ago here:

https://www.spinics.net/lists/netdev/msg537424.html

but that is IMHO wasting storage space, because the kernel is
maintaining the address lists, and now also needs to gain knowledge
about the VID. With up to 4K - 2 VLAN interfaces per switch port, this
bloats the memory footprint, we arguably still need to maintain those
address lists anyway...

The reason why I chose switchdev here is because:

- this is mostly relevant for switch devices, not so much for NICs (it
seems), if it was, they would have solved the problem by now

- this allows to have an unified path from the switch driver perspective
to program MDB addresses targeting the CPU/management port, no need to
have X different ways of doing the same operation
-- 
Florian

Re: [PATCH net-next 09/14] net: bridge: Propagate MC addresses with VID through switchdev

[Florian Fainelli]

On 1/17/19 6:05 AM, Ido Schimmel wrote:
> On Wed, Jan 16, 2019 at 12:00:57PM -0800, Florian Fainelli wrote:
>> In order for bridge port members to get a chance to implement unicast
>> and multicast address filtering correctly, which would matter for e.g:
>> switch network devices, synchronize the UC and MC lists down to the
>> individual bridge port members using switchdev HOST_MDB objects such
>> that this does not impact drivers that already have a ndo_set_rx_mode()
>> operation which likely already operate in promiscuous mode.
>>
>> When the bridge has multicast snooping enabled, proper HOST_MDB
>> notifications will be sent through br_mdb_notify() already.
> 
> I don't understand the change. HOST_MDB is used to notify underlying
> drivers about MDB entries that should be configured to locally receive
> packets. This is triggered by the transmission of an IGMP report through
> the bridge, for example.
> 
> It seems that you're trying to overload HOST_MDB with multicast address
> filtering on bridge ports?

I don't really think this is an abuse of HOST_MDB, since in case the
bridge has multicast_snooping enabled, and there is e.g: a multicast
application bound to the bridge master device, we would get those
notifications through HOST_MDB already. This is the same use case that I
am addressing here, ndo_set_rx_mode() learns about local multicast
addresses that should be programmed, which means there is a multicast
application listening on the bridge master device itself.

The problem that I want to solve is that with Broadcom b53/bcm_sf2
switches, we cannot easily filter/flood multicast for the CPU/management
port.

We have per-port controls for MC/IPMC flooding, and we also have a
separate control for CPU/management port receiving multicast. If either
of these two bits/settings are configured, then the CPU port will always
receive multicast, even when we should be filtering it in HW. The only
way to perform selective reception of multicast to the CPU port is to
program a corresponding MDB entry.

>Why are you performing this filtering?

If I do not filter, then non-bridged ports on which there is no
multicast application bound to would be passing up multicast traffic all
the way to the CPU port, which then has to be dropped in software. This
is not acceptable IMHO because it is a deviation from how a standalone
NIC supporting multicast filtering would operate.

> Shouldn't you allow all MAC addresses to ingress?

I do allow all MC addresses to ingress on the front-panel switch ports
(while honoring the multicast_snooping setting), but we have no control
over what the CPU/management port should be doing.

As I wrote earlier, if we flood to the CPU/management port, because
there is at least one switch device port, in the bridge, and that bridge
has multicast_snooping disabled, then this could break filtering for
other, non-bridged ports. That is really not acceptable IMHO.

The reason why I chose switchdev HOST_MDB notification here are two fold:

- this is the same use case as with multicast_snooping=1 and we target
the CPU port within DSA to resolve that use case, so from the switch
driver perspective, there is no difference in the context

- this does not impact network device drivers that have a
ndo_set_rx_mode() and somehow decide to support things through that API
since those would typically have a switchdev_port_attr_set() callback

Let me know if this is not clear.
-- 
Florian

Re: [PATCH net-next 01/14] net: bridge: multicast: Propagate br_mc_disabled_update() return

[Florian Fainelli]

On 1/17/19 5:47 AM, Ido Schimmel wrote:
> On Wed, Jan 16, 2019 at 12:00:49PM -0800, Florian Fainelli wrote:
>> Some Ethernet switches might not be able to support disabling multicast
>> flooding globally when e.g: several bridges span the same physical
>> device, propagate the return value of br_mc_disabled_update() such that
>> this propagates correctly to user-space.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  net/bridge/br_multicast.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>> index 3aeff0895669..09fc92541873 100644
>> --- a/net/bridge/br_multicast.c
>> +++ b/net/bridge/br_multicast.c
>> @@ -813,7 +813,7 @@ static void br_ip6_multicast_port_query_expired(struct timer_list *t)
>>  }
>>  #endif
>>  
>> -static void br_mc_disabled_update(struct net_device *dev, bool value)
>> +static int br_mc_disabled_update(struct net_device *dev, bool value)
>>  {
>>  	struct switchdev_attr attr = {
>>  		.orig_dev = dev,
>> @@ -822,11 +822,13 @@ static void br_mc_disabled_update(struct net_device *dev, bool value)
>>  		.u.mc_disabled = !value,
>>  	};
>>  
>> -	switchdev_port_attr_set(dev, &attr);
>> +	return switchdev_port_attr_set(dev, &attr);
> 
> Did you test this for veth? If switchdev ops are not defined, this
> returns -EOPNOTSUPP. See __br_vlan_filter_toggle() for reference

I did not, good catch, I will add a check on -EOPNOTSUPP and silence
that error code:

if (err && err != -ENOTSUPP)
	return err;

> 
>>  }
>>  
>>  int br_multicast_add_port(struct net_bridge_port *port)
>>  {
>> +	int ret;
>> +
>>  	port->multicast_router = MDB_RTR_TYPE_TEMP_QUERY;
>>  
>>  	timer_setup(&port->multicast_router_timer,
>> @@ -837,8 +839,11 @@ int br_multicast_add_port(struct net_bridge_port *port)
>>  	timer_setup(&port->ip6_own_query.timer,
>>  		    br_ip6_multicast_port_query_expired, 0);
>>  #endif
>> -	br_mc_disabled_update(port->dev,
>> -			      br_opt_get(port->br, BROPT_MULTICAST_ENABLED));
>> +	ret = br_mc_disabled_update(port->dev,
>> +				    br_opt_get(port->br,
>> +					       BROPT_MULTICAST_ENABLED));
>> +	if (ret)
>> +		return ret;
>>  
>>  	port->mcast_stats = netdev_alloc_pcpu_stats(struct bridge_mcast_stats);
>>  	if (!port->mcast_stats)
>> @@ -1937,12 +1942,16 @@ static void br_multicast_start_querier(struct net_bridge *br,
>>  int br_multicast_toggle(struct net_bridge *br, unsigned long val)
>>  {
>>  	struct net_bridge_port *port;
>> +	int err;
>>  
>>  	spin_lock_bh(&br->multicast_lock);
>>  	if (!!br_opt_get(br, BROPT_MULTICAST_ENABLED) == !!val)
>>  		goto unlock;
>>  
>> -	br_mc_disabled_update(br->dev, val);
>> +	err = br_mc_disabled_update(br->dev, val);
>> +	if (err)
>> +		goto unlock;
>> +
>>  	br_opt_toggle(br, BROPT_MULTICAST_ENABLED, !!val);
>>  	if (!br_opt_get(br, BROPT_MULTICAST_ENABLED))
>>  		goto unlock;
> 
> You never return the error

Huh, indeed, thanks!
-- 
Florian

Re: [PATCH net-next 09/14] net: bridge: Propagate MC addresses with VID through switchdev

[Ido Schimmel]

On Thu, Jan 17, 2019 at 11:17:57AM -0800, Florian Fainelli wrote:
> On 1/17/19 6:05 AM, Ido Schimmel wrote:
> > On Wed, Jan 16, 2019 at 12:00:57PM -0800, Florian Fainelli wrote:
> >> In order for bridge port members to get a chance to implement unicast
> >> and multicast address filtering correctly, which would matter for e.g:
> >> switch network devices, synchronize the UC and MC lists down to the
> >> individual bridge port members using switchdev HOST_MDB objects such
> >> that this does not impact drivers that already have a ndo_set_rx_mode()
> >> operation which likely already operate in promiscuous mode.
> >>
> >> When the bridge has multicast snooping enabled, proper HOST_MDB
> >> notifications will be sent through br_mdb_notify() already.
> > 
> > I don't understand the change. HOST_MDB is used to notify underlying
> > drivers about MDB entries that should be configured to locally receive
> > packets. This is triggered by the transmission of an IGMP report through
> > the bridge, for example.
> > 
> > It seems that you're trying to overload HOST_MDB with multicast address
> > filtering on bridge ports?
> 
> I don't really think this is an abuse of HOST_MDB, since in case the
> bridge has multicast_snooping enabled, and there is e.g: a multicast
> application bound to the bridge master device, we would get those
> notifications through HOST_MDB already. This is the same use case that I
> am addressing here, ndo_set_rx_mode() learns about local multicast
> addresses that should be programmed, which means there is a multicast
> application listening on the bridge master device itself.
> 
> The problem that I want to solve is that with Broadcom b53/bcm_sf2
> switches, we cannot easily filter/flood multicast for the CPU/management
> port.
> 
> We have per-port controls for MC/IPMC flooding, and we also have a
> separate control for CPU/management port receiving multicast. If either
> of these two bits/settings are configured, then the CPU port will always
> receive multicast, even when we should be filtering it in HW. The only
> way to perform selective reception of multicast to the CPU port is to
> program a corresponding MDB entry.
> 
> >Why are you performing this filtering?
> 
> If I do not filter, then non-bridged ports on which there is no
> multicast application bound to would be passing up multicast traffic all
> the way to the CPU port, which then has to be dropped in software. This
> is not acceptable IMHO because it is a deviation from how a standalone
> NIC supporting multicast filtering would operate.
> 
> > Shouldn't you allow all MAC addresses to ingress?
> 
> I do allow all MC addresses to ingress on the front-panel switch ports
> (while honoring the multicast_snooping setting), but we have no control
> over what the CPU/management port should be doing.
> 
> As I wrote earlier, if we flood to the CPU/management port, because
> there is at least one switch device port, in the bridge, and that bridge
> has multicast_snooping disabled, then this could break filtering for
> other, non-bridged ports. That is really not acceptable IMHO.

When multicast snooping is disabled every multicast packet should be
flooded to the CPU port. Not only addresses configured on the bridge
device's address list. Check br_handle_frame_finish(), it sets
'local_rcv' to 'true'.

> 
> The reason why I chose switchdev HOST_MDB notification here are two fold:
> 
> - this is the same use case as with multicast_snooping=1 and we target
> the CPU port within DSA to resolve that use case, so from the switch
> driver perspective, there is no difference in the context
> 
> - this does not impact network device drivers that have a
> ndo_set_rx_mode() and somehow decide to support things through that API
> since those would typically have a switchdev_port_attr_set() callback
> 
> Let me know if this is not clear.
> -- 
> Florian

Re: [PATCH net-next 09/14] net: bridge: Propagate MC addresses with VID through switchdev

[Ido Schimmel]

On Thu, Jan 17, 2019 at 11:17:57AM -0800, Florian Fainelli wrote:
> On 1/17/19 6:05 AM, Ido Schimmel wrote:
> > On Wed, Jan 16, 2019 at 12:00:57PM -0800, Florian Fainelli wrote:
> >> In order for bridge port members to get a chance to implement unicast
> >> and multicast address filtering correctly, which would matter for e.g:
> >> switch network devices, synchronize the UC and MC lists down to the
> >> individual bridge port members using switchdev HOST_MDB objects such
> >> that this does not impact drivers that already have a ndo_set_rx_mode()
> >> operation which likely already operate in promiscuous mode.
> >>
> >> When the bridge has multicast snooping enabled, proper HOST_MDB
> >> notifications will be sent through br_mdb_notify() already.
> > 
> > I don't understand the change. HOST_MDB is used to notify underlying
> > drivers about MDB entries that should be configured to locally receive
> > packets. This is triggered by the transmission of an IGMP report through
> > the bridge, for example.
> > 
> > It seems that you're trying to overload HOST_MDB with multicast address
> > filtering on bridge ports?
> 
> I don't really think this is an abuse of HOST_MDB, since in case the
> bridge has multicast_snooping enabled, and there is e.g: a multicast
> application bound to the bridge master device, we would get those
> notifications through HOST_MDB already. This is the same use case that I
> am addressing here, ndo_set_rx_mode() learns about local multicast
> addresses that should be programmed, which means there is a multicast
> application listening on the bridge master device itself.
> 
> The problem that I want to solve is that with Broadcom b53/bcm_sf2
> switches, we cannot easily filter/flood multicast for the CPU/management
> port.
> 
> We have per-port controls for MC/IPMC flooding, and we also have a
> separate control for CPU/management port receiving multicast. If either
> of these two bits/settings are configured, then the CPU port will always
> receive multicast, even when we should be filtering it in HW. The only
> way to perform selective reception of multicast to the CPU port is to
> program a corresponding MDB entry.
> 
> >Why are you performing this filtering?
> 
> If I do not filter, then non-bridged ports on which there is no
> multicast application bound to would be passing up multicast traffic all
> the way to the CPU port, which then has to be dropped in software. This
> is not acceptable IMHO because it is a deviation from how a standalone
> NIC supporting multicast filtering would operate.
> 
> > Shouldn't you allow all MAC addresses to ingress?
> 
> I do allow all MC addresses to ingress on the front-panel switch ports
> (while honoring the multicast_snooping setting), but we have no control
> over what the CPU/management port should be doing.
> 
> As I wrote earlier, if we flood to the CPU/management port, because
> there is at least one switch device port, in the bridge, and that bridge
> has multicast_snooping disabled, then this could break filtering for
> other, non-bridged ports. That is really not acceptable IMHO.
> 
> The reason why I chose switchdev HOST_MDB notification here are two fold:
> 
> - this is the same use case as with multicast_snooping=1 and we target
> the CPU port within DSA to resolve that use case, so from the switch
> driver perspective, there is no difference in the context
> 
> - this does not impact network device drivers that have a
> ndo_set_rx_mode() and somehow decide to support things through that API
> since those would typically have a switchdev_port_attr_set() callback

HOST_MDB was added for a very specific use case. To allow the bridge
driver to notify underlying switch drivers about MDB entries that should
be programmed to locally receive packets when multicast is enabled.
Andrew described it very nicely in merge commit
5d37636abd15ace8686a54167b488364ee79e88d

Ingress filtering is something completely different and not applicable
to bridged ports that should allow every address to ingress.

switchdev allows to offload the bridge datapath to capable devices, but
you're abusing to it allow non-bridged ports to perform address
filtering. Completely unrelated.

Therefore, it seems completely inappropriate to me to use HOST_MDB for
this reason. This applies to patch #10 as well.

It really sounds like the HW you're working with is not designed to work
in this mixed state where some ports are bridged and some are expected to
act as standalone NICs.

If you're still determined to support this use case, I suggest the
following. In your driver, program the bridge's address list as MDB
entries when the first port is enslaved to it. Then, add a new netdev
event whenever an address is added / removed from this list (in
__dev_set_rx_mode() ?). Have your driver listen to it and program MDB
entries accordingly.

Re: [PATCH net-next 09/14] net: bridge: Propagate MC addresses with VID through switchdev

[Florian Fainelli]

On 1/18/19 3:41 AM, Ido Schimmel wrote:
> On Thu, Jan 17, 2019 at 11:17:57AM -0800, Florian Fainelli wrote:
>> On 1/17/19 6:05 AM, Ido Schimmel wrote:
>>> On Wed, Jan 16, 2019 at 12:00:57PM -0800, Florian Fainelli wrote:
>>>> In order for bridge port members to get a chance to implement unicast
>>>> and multicast address filtering correctly, which would matter for e.g:
>>>> switch network devices, synchronize the UC and MC lists down to the
>>>> individual bridge port members using switchdev HOST_MDB objects such
>>>> that this does not impact drivers that already have a ndo_set_rx_mode()
>>>> operation which likely already operate in promiscuous mode.
>>>>
>>>> When the bridge has multicast snooping enabled, proper HOST_MDB
>>>> notifications will be sent through br_mdb_notify() already.
>>>
>>> I don't understand the change. HOST_MDB is used to notify underlying
>>> drivers about MDB entries that should be configured to locally receive
>>> packets. This is triggered by the transmission of an IGMP report through
>>> the bridge, for example.
>>>
>>> It seems that you're trying to overload HOST_MDB with multicast address
>>> filtering on bridge ports?
>>
>> I don't really think this is an abuse of HOST_MDB, since in case the
>> bridge has multicast_snooping enabled, and there is e.g: a multicast
>> application bound to the bridge master device, we would get those
>> notifications through HOST_MDB already. This is the same use case that I
>> am addressing here, ndo_set_rx_mode() learns about local multicast
>> addresses that should be programmed, which means there is a multicast
>> application listening on the bridge master device itself.
>>
>> The problem that I want to solve is that with Broadcom b53/bcm_sf2
>> switches, we cannot easily filter/flood multicast for the CPU/management
>> port.
>>
>> We have per-port controls for MC/IPMC flooding, and we also have a
>> separate control for CPU/management port receiving multicast. If either
>> of these two bits/settings are configured, then the CPU port will always
>> receive multicast, even when we should be filtering it in HW. The only
>> way to perform selective reception of multicast to the CPU port is to
>> program a corresponding MDB entry.
>>
>>> Why are you performing this filtering?
>>
>> If I do not filter, then non-bridged ports on which there is no
>> multicast application bound to would be passing up multicast traffic all
>> the way to the CPU port, which then has to be dropped in software. This
>> is not acceptable IMHO because it is a deviation from how a standalone
>> NIC supporting multicast filtering would operate.
>>
>>> Shouldn't you allow all MAC addresses to ingress?
>>
>> I do allow all MC addresses to ingress on the front-panel switch ports
>> (while honoring the multicast_snooping setting), but we have no control
>> over what the CPU/management port should be doing.
>>
>> As I wrote earlier, if we flood to the CPU/management port, because
>> there is at least one switch device port, in the bridge, and that bridge
>> has multicast_snooping disabled, then this could break filtering for
>> other, non-bridged ports. That is really not acceptable IMHO.
>>
>> The reason why I chose switchdev HOST_MDB notification here are two fold:
>>
>> - this is the same use case as with multicast_snooping=1 and we target
>> the CPU port within DSA to resolve that use case, so from the switch
>> driver perspective, there is no difference in the context
>>
>> - this does not impact network device drivers that have a
>> ndo_set_rx_mode() and somehow decide to support things through that API
>> since those would typically have a switchdev_port_attr_set() callback
> 
> HOST_MDB was added for a very specific use case. To allow the bridge
> driver to notify underlying switch drivers about MDB entries that should
> be programmed to locally receive packets when multicast is enabled.
> Andrew described it very nicely in merge commit
> 5d37636abd15ace8686a54167b488364ee79e88d
> 
> Ingress filtering is something completely different and not applicable
> to bridged ports that should allow every address to ingress.

I actually made a mistake in this patch because there is no need to
iterate over the switch port members and generate a HOST_MDB
notification for each of them because what we want to target is the CPU
port, which DSA internally resolves for us anyway.

What we want to tell the switch HW here is basically: you have a
multicast application bound to the bridge master device, so please let
this MC address go through your CPU/management port. This is effectively
egress filtering at the CPU port side.

Because the bridge has multicast_snooping=false, the switch ports have
been configured to flood MC/IPMC already, but as I wrote, if we do that
for the CPU port, then we "break" non-bridge ports.

It seems to me that this is exactly the same use case that what Andrew
did originally, and drivers that are not pathological like mine can just
decide to ignore that notification and flood everything to the CPU port.
The end results would be the same from an end user perspective.

Do you still think this is too much of a stretch?

> 
> switchdev allows to offload the bridge datapath to capable devices, but
> you're abusing to it allow non-bridged ports to perform address
> filtering. Completely unrelated.
> 
> Therefore, it seems completely inappropriate to me to use HOST_MDB for
> this reason. This applies to patch #10 as well.
> 
> It really sounds like the HW you're working with is not designed to work
> in this mixed state where some ports are bridged and some are expected to
> act as standalone NICs.

That is quite true, the HW that I work with is limited, and does not
really play well with mixed port usage, but with the help of the network
stack and notifications, we can get very close, or even support it.

One thing that I forgot to explain is that the Ethernet MAC connected to
its internal bcm_sf2 switch, because it is only used with an integrated
switch has been greatly simplified, it does not support any type of
filtering and relies on the switch to do that. It effectively operates
in promiscuous mode all the time.

> 
> If you're still determined to support this use case, I suggest the
> following. In your driver, program the bridge's address list as MDB
> entries when the first port is enslaved to it. Then, add a new netdev
> event whenever an address is added / removed from this list (in
> __dev_set_rx_mode() ?). Have your driver listen to it and program MDB
> entries accordingly.
> 


-- 
Florian

Re: [PATCH net-next 09/14] net: bridge: Propagate MC addresses with VID through switchdev

[Ido Schimmel]

On Fri, Jan 18, 2019 at 01:48:56PM -0800, Florian Fainelli wrote:
> On 1/18/19 3:41 AM, Ido Schimmel wrote:
> > On Thu, Jan 17, 2019 at 11:17:57AM -0800, Florian Fainelli wrote:
> >> On 1/17/19 6:05 AM, Ido Schimmel wrote:
> >>> On Wed, Jan 16, 2019 at 12:00:57PM -0800, Florian Fainelli wrote:
> >>>> In order for bridge port members to get a chance to implement unicast
> >>>> and multicast address filtering correctly, which would matter for e.g:
> >>>> switch network devices, synchronize the UC and MC lists down to the
> >>>> individual bridge port members using switchdev HOST_MDB objects such
> >>>> that this does not impact drivers that already have a ndo_set_rx_mode()
> >>>> operation which likely already operate in promiscuous mode.
> >>>>
> >>>> When the bridge has multicast snooping enabled, proper HOST_MDB
> >>>> notifications will be sent through br_mdb_notify() already.
> >>>
> >>> I don't understand the change. HOST_MDB is used to notify underlying
> >>> drivers about MDB entries that should be configured to locally receive
> >>> packets. This is triggered by the transmission of an IGMP report through
> >>> the bridge, for example.
> >>>
> >>> It seems that you're trying to overload HOST_MDB with multicast address
> >>> filtering on bridge ports?
> >>
> >> I don't really think this is an abuse of HOST_MDB, since in case the
> >> bridge has multicast_snooping enabled, and there is e.g: a multicast
> >> application bound to the bridge master device, we would get those
> >> notifications through HOST_MDB already. This is the same use case that I
> >> am addressing here, ndo_set_rx_mode() learns about local multicast
> >> addresses that should be programmed, which means there is a multicast
> >> application listening on the bridge master device itself.
> >>
> >> The problem that I want to solve is that with Broadcom b53/bcm_sf2
> >> switches, we cannot easily filter/flood multicast for the CPU/management
> >> port.
> >>
> >> We have per-port controls for MC/IPMC flooding, and we also have a
> >> separate control for CPU/management port receiving multicast. If either
> >> of these two bits/settings are configured, then the CPU port will always
> >> receive multicast, even when we should be filtering it in HW. The only
> >> way to perform selective reception of multicast to the CPU port is to
> >> program a corresponding MDB entry.
> >>
> >>> Why are you performing this filtering?
> >>
> >> If I do not filter, then non-bridged ports on which there is no
> >> multicast application bound to would be passing up multicast traffic all
> >> the way to the CPU port, which then has to be dropped in software. This
> >> is not acceptable IMHO because it is a deviation from how a standalone
> >> NIC supporting multicast filtering would operate.
> >>
> >>> Shouldn't you allow all MAC addresses to ingress?
> >>
> >> I do allow all MC addresses to ingress on the front-panel switch ports
> >> (while honoring the multicast_snooping setting), but we have no control
> >> over what the CPU/management port should be doing.
> >>
> >> As I wrote earlier, if we flood to the CPU/management port, because
> >> there is at least one switch device port, in the bridge, and that bridge
> >> has multicast_snooping disabled, then this could break filtering for
> >> other, non-bridged ports. That is really not acceptable IMHO.
> >>
> >> The reason why I chose switchdev HOST_MDB notification here are two fold:
> >>
> >> - this is the same use case as with multicast_snooping=1 and we target
> >> the CPU port within DSA to resolve that use case, so from the switch
> >> driver perspective, there is no difference in the context
> >>
> >> - this does not impact network device drivers that have a
> >> ndo_set_rx_mode() and somehow decide to support things through that API
> >> since those would typically have a switchdev_port_attr_set() callback
> > 
> > HOST_MDB was added for a very specific use case. To allow the bridge
> > driver to notify underlying switch drivers about MDB entries that should
> > be programmed to locally receive packets when multicast is enabled.
> > Andrew described it very nicely in merge commit
> > 5d37636abd15ace8686a54167b488364ee79e88d
> > 
> > Ingress filtering is something completely different and not applicable
> > to bridged ports that should allow every address to ingress.
> 
> I actually made a mistake in this patch because there is no need to
> iterate over the switch port members and generate a HOST_MDB
> notification for each of them because what we want to target is the CPU
> port, which DSA internally resolves for us anyway.
> 
> What we want to tell the switch HW here is basically: you have a
> multicast application bound to the bridge master device, so please let
> this MC address go through your CPU/management port. This is effectively
> egress filtering at the CPU port side.
> 
> Because the bridge has multicast_snooping=false, the switch ports have
> been configured to flood MC/IPMC already, but as I wrote, if we do that
> for the CPU port, then we "break" non-bridge ports.
> 
> It seems to me that this is exactly the same use case that what Andrew
> did originally, and drivers that are not pathological like mine can just
> decide to ignore that notification and flood everything to the CPU port.
> The end results would be the same from an end user perspective.
> 
> Do you still think this is too much of a stretch?

Yes, but I don't have a much better solution either.

Can we take a step back and re-consider the whole thing? Is it really
worth it?

IIUC, you are trying to enable Rx filtering on switch ports that are
used as standalone NICs while other switch ports are enslaved to a
bridge. This is with HW that was not designed with this use case in
mind. From that I derive that it is not commonly used like that. I might
be wrong.

Furthermore, there's an application running on top of the bridge which
is using multicast, but for some reason multicast is disabled on the
bridge, despite the fact it is on by default.

To get multicast packets to the application you can either flood them to
the CPU or program specific MDB entries. If you use the first option, it
means that other ports - used as standalone NICs - will also be affected
and will not perform Rx filtering in HW. Is it really critical? The CPU
will be overwhelmed by that? These ports are expected to drop a lot of
packets in HW due to Rx filtering?

I'm not trying to hold you up, I just want to make sure that we are
doing the right thing. This code will need to be maintained and debugged
and if no one will use it beside syzbot, then maybe we can live without
it.

> 
> > 
> > switchdev allows to offload the bridge datapath to capable devices, but
> > you're abusing to it allow non-bridged ports to perform address
> > filtering. Completely unrelated.
> > 
> > Therefore, it seems completely inappropriate to me to use HOST_MDB for
> > this reason. This applies to patch #10 as well.
> > 
> > It really sounds like the HW you're working with is not designed to work
> > in this mixed state where some ports are bridged and some are expected to
> > act as standalone NICs.
> 
> That is quite true, the HW that I work with is limited, and does not
> really play well with mixed port usage, but with the help of the network
> stack and notifications, we can get very close, or even support it.
> 
> One thing that I forgot to explain is that the Ethernet MAC connected to
> its internal bcm_sf2 switch, because it is only used with an integrated
> switch has been greatly simplified, it does not support any type of
> filtering and relies on the switch to do that. It effectively operates
> in promiscuous mode all the time.

Got it. Thanks for explaining.

> 
> > 
> > If you're still determined to support this use case, I suggest the
> > following. In your driver, program the bridge's address list as MDB
> > entries when the first port is enslaved to it. Then, add a new netdev
> > event whenever an address is added / removed from this list (in
> > __dev_set_rx_mode() ?). Have your driver listen to it and program MDB
> > entries accordingly.
> > 
> 
> 
> -- 
> Florian

Re: [PATCH net-next 09/14] net: bridge: Propagate MC addresses with VID through switchdev

[Florian Fainelli]

On 1/19/2019 5:55 AM, Ido Schimmel wrote:
> On Fri, Jan 18, 2019 at 01:48:56PM -0800, Florian Fainelli wrote:
>> On 1/18/19 3:41 AM, Ido Schimmel wrote:
>>> On Thu, Jan 17, 2019 at 11:17:57AM -0800, Florian Fainelli wrote:
>>>> On 1/17/19 6:05 AM, Ido Schimmel wrote:
>>>>> On Wed, Jan 16, 2019 at 12:00:57PM -0800, Florian Fainelli wrote:
>>>>>> In order for bridge port members to get a chance to implement unicast
>>>>>> and multicast address filtering correctly, which would matter for e.g:
>>>>>> switch network devices, synchronize the UC and MC lists down to the
>>>>>> individual bridge port members using switchdev HOST_MDB objects such
>>>>>> that this does not impact drivers that already have a ndo_set_rx_mode()
>>>>>> operation which likely already operate in promiscuous mode.
>>>>>>
>>>>>> When the bridge has multicast snooping enabled, proper HOST_MDB
>>>>>> notifications will be sent through br_mdb_notify() already.
>>>>>
>>>>> I don't understand the change. HOST_MDB is used to notify underlying
>>>>> drivers about MDB entries that should be configured to locally receive
>>>>> packets. This is triggered by the transmission of an IGMP report through
>>>>> the bridge, for example.
>>>>>
>>>>> It seems that you're trying to overload HOST_MDB with multicast address
>>>>> filtering on bridge ports?
>>>>
>>>> I don't really think this is an abuse of HOST_MDB, since in case the
>>>> bridge has multicast_snooping enabled, and there is e.g: a multicast
>>>> application bound to the bridge master device, we would get those
>>>> notifications through HOST_MDB already. This is the same use case that I
>>>> am addressing here, ndo_set_rx_mode() learns about local multicast
>>>> addresses that should be programmed, which means there is a multicast
>>>> application listening on the bridge master device itself.
>>>>
>>>> The problem that I want to solve is that with Broadcom b53/bcm_sf2
>>>> switches, we cannot easily filter/flood multicast for the CPU/management
>>>> port.
>>>>
>>>> We have per-port controls for MC/IPMC flooding, and we also have a
>>>> separate control for CPU/management port receiving multicast. If either
>>>> of these two bits/settings are configured, then the CPU port will always
>>>> receive multicast, even when we should be filtering it in HW. The only
>>>> way to perform selective reception of multicast to the CPU port is to
>>>> program a corresponding MDB entry.
>>>>
>>>>> Why are you performing this filtering?
>>>>
>>>> If I do not filter, then non-bridged ports on which there is no
>>>> multicast application bound to would be passing up multicast traffic all
>>>> the way to the CPU port, which then has to be dropped in software. This
>>>> is not acceptable IMHO because it is a deviation from how a standalone
>>>> NIC supporting multicast filtering would operate.
>>>>
>>>>> Shouldn't you allow all MAC addresses to ingress?
>>>>
>>>> I do allow all MC addresses to ingress on the front-panel switch ports
>>>> (while honoring the multicast_snooping setting), but we have no control
>>>> over what the CPU/management port should be doing.
>>>>
>>>> As I wrote earlier, if we flood to the CPU/management port, because
>>>> there is at least one switch device port, in the bridge, and that bridge
>>>> has multicast_snooping disabled, then this could break filtering for
>>>> other, non-bridged ports. That is really not acceptable IMHO.
>>>>
>>>> The reason why I chose switchdev HOST_MDB notification here are two fold:
>>>>
>>>> - this is the same use case as with multicast_snooping=1 and we target
>>>> the CPU port within DSA to resolve that use case, so from the switch
>>>> driver perspective, there is no difference in the context
>>>>
>>>> - this does not impact network device drivers that have a
>>>> ndo_set_rx_mode() and somehow decide to support things through that API
>>>> since those would typically have a switchdev_port_attr_set() callback
>>>
>>> HOST_MDB was added for a very specific use case. To allow the bridge
>>> driver to notify underlying switch drivers about MDB entries that should
>>> be programmed to locally receive packets when multicast is enabled.
>>> Andrew described it very nicely in merge commit
>>> 5d37636abd15ace8686a54167b488364ee79e88d
>>>
>>> Ingress filtering is something completely different and not applicable
>>> to bridged ports that should allow every address to ingress.
>>
>> I actually made a mistake in this patch because there is no need to
>> iterate over the switch port members and generate a HOST_MDB
>> notification for each of them because what we want to target is the CPU
>> port, which DSA internally resolves for us anyway.
>>
>> What we want to tell the switch HW here is basically: you have a
>> multicast application bound to the bridge master device, so please let
>> this MC address go through your CPU/management port. This is effectively
>> egress filtering at the CPU port side.
>>
>> Because the bridge has multicast_snooping=false, the switch ports have
>> been configured to flood MC/IPMC already, but as I wrote, if we do that
>> for the CPU port, then we "break" non-bridge ports.
>>
>> It seems to me that this is exactly the same use case that what Andrew
>> did originally, and drivers that are not pathological like mine can just
>> decide to ignore that notification and flood everything to the CPU port.
>> The end results would be the same from an end user perspective.
>>
>> Do you still think this is too much of a stretch?
> 
> Yes, but I don't have a much better solution either.
> 
> Can we take a step back and re-consider the whole thing? Is it really
> worth it?
> 
> IIUC, you are trying to enable Rx filtering on switch ports that are
> used as standalone NICs while other switch ports are enslaved to a
> bridge. This is with HW that was not designed with this use case in
> mind. From that I derive that it is not commonly used like that. I might
> be wrong.

I bet you are right, we happen to have such an use case but it is
uncommon I agree.

> 
> Furthermore, there's an application running on top of the bridge which
> is using multicast, but for some reason multicast is disabled on the
> bridge, despite the fact it is on by default.

Well, it is again because I wanted to allow supporting all possible use
cases of having multicast snooping on/off and let that be changeable at
runtime. Now that this attribute propagates back to the caller, we could
enforce having bridge multicast snooping always on (provided certain
conditions) and it would not require that specific patch 9 we are
discussing see below for details..

> 
> To get multicast packets to the application you can either flood them to
> the CPU or program specific MDB entries. If you use the first option, it
> means that other ports - used as standalone NICs - will also be affected
> and will not perform Rx filtering in HW. Is it really critical? The CPU
> will be overwhelmed by that? These ports are expected to drop a lot of
> packets in HW due to Rx filtering?
> 
> I'm not trying to hold you up, I just want to make sure that we are
> doing the right thing. This code will need to be maintained and debugged
> and if no one will use it beside syzbot, then maybe we can live without
> it.

I would not have gone through all of this if there was not an use case
that came my way to begin with, and now that there is an use case this
is something that I test too.

Maybe let's be more pragmatic in the approach to support those use cases
and do the following with respect to this particular b53 switch HW: it
is only permissible to have a bridge with multicast snooping disabled
under the following conditions: all other ports are a member of that
bridge or another bridge that has the same multicast snooping setting
(turned on). If there is at least one standalone port, or another bridge
requires multicast snooping on, then turning off multicast snooping in
any bridge device is not possible.

Does that sound acceptable?

Another approach like you hinted is to introduce an even more targeted
event/object in the spirit of ndo_set_rx_mode() but with the flexibility
of switchdev such that only a subset of drivers can act on those
notifications. I felt that HOST_MDB would do, and callers could look at
br_multicast_enabled() on the HOST_MDB's switchdev_obj::orig_dev.

Let's continue discussing the VLAN changes since those could be
beneficial for cpsw as well.

> 
>>
>>>
>>> switchdev allows to offload the bridge datapath to capable devices, but
>>> you're abusing to it allow non-bridged ports to perform address
>>> filtering. Completely unrelated.
>>>
>>> Therefore, it seems completely inappropriate to me to use HOST_MDB for
>>> this reason. This applies to patch #10 as well.
>>>
>>> It really sounds like the HW you're working with is not designed to work
>>> in this mixed state where some ports are bridged and some are expected to
>>> act as standalone NICs.
>>
>> That is quite true, the HW that I work with is limited, and does not
>> really play well with mixed port usage, but with the help of the network
>> stack and notifications, we can get very close, or even support it.
>>
>> One thing that I forgot to explain is that the Ethernet MAC connected to
>> its internal bcm_sf2 switch, because it is only used with an integrated
>> switch has been greatly simplified, it does not support any type of
>> filtering and relies on the switch to do that. It effectively operates
>> in promiscuous mode all the time.
> 
> Got it. Thanks for explaining.
> 
>>
>>>
>>> If you're still determined to support this use case, I suggest the
>>> following. In your driver, program the bridge's address list as MDB
>>> entries when the first port is enslaved to it. Then, add a new netdev
>>> event whenever an address is added / removed from this list (in
>>> __dev_set_rx_mode() ?). Have your driver listen to it and program MDB
>>> entries accordingly.
>>>
>>
>>
>> -- 
>> Florian

-- 
Florian

Re: [PATCH net-next 09/14] net: bridge: Propagate MC addresses with VID through switchdev

[Ido Schimmel]

On Sat, Jan 19, 2019 at 07:22:21PM -0800, Florian Fainelli wrote:
> Maybe let's be more pragmatic in the approach to support those use cases
> and do the following with respect to this particular b53 switch HW: it
> is only permissible to have a bridge with multicast snooping disabled
> under the following conditions: all other ports are a member of that
> bridge or another bridge that has the same multicast snooping setting
> (turned on). If there is at least one standalone port, or another bridge
> requires multicast snooping on, then turning off multicast snooping in
> any bridge device is not possible.
> 
> Does that sound acceptable?

Yes.

> Let's continue discussing the VLAN changes since those could be
> beneficial for cpsw as well.

OK. Will reply there.

Re: [PATCH net-next 09/14] net: bridge: Propagate MC addresses with VID through switchdev

[Jiri Pirko]

Fri, Jan 18, 2019 at 12:41:08PM CET, idosch@mellanox.com wrote:
>On Thu, Jan 17, 2019 at 11:17:57AM -0800, Florian Fainelli wrote:

[...]


>
>switchdev allows to offload the bridge datapath to capable devices, but
>you're abusing to it allow non-bridged ports to perform address
>filtering. Completely unrelated.

Florian, please make sure not to do that. Switchdev code is now 100%
bridge offload facility, it would be preferable to keep it that way. I
plan to move the code to bridge and get rid of net/switchdev completely
to avoid confusions. Arkadi was working on that in past but he did not
finish it, unfortunatelly.

Re: [PATCH net-next 10/14] net: vlan: Propagate MC addresses with VID through switchdev

[Ido Schimmel]

On Thu, Jan 17, 2019 at 11:12:24AM -0800, Florian Fainelli wrote:
> On 1/17/19 6:49 AM, Ido Schimmel wrote:
> > On Wed, Jan 16, 2019 at 12:00:58PM -0800, Florian Fainelli wrote:
> >> The VLAN real device could be an Ethernet switch port and that switch
> >> might have VLAN filtering globally enabled (because of a bridge
> >> requesting VLAN filtering on the switch on another port) and so when
> >> programming multicast addresses, we need the multicast filter
> >> programming to be aware of the correct VLAN ID as well.
> > 
> > This looks like a quirk of a specific device. How bad is it to patch the
> > driver to add a multicast address for every configured VLAN?
> 
> There is at least another driver that can be benefit from that which is
> cpsw, if I understand Ivan's use case correctly.

I understand and I have no argument against the need for this. I just
think we should use a different mechanism than switchdev.

> If there is a ndo_set_rx_mode() function implemented by the virtual
> device, and that does call dev_{mc,uc}_sync(master, dev), then this
> means that you do want to be able to filter UC and MC addresses. If we
> added the entire class D range of multicast addresses to the switch's
> MDB, that would not be filtering, we would be passing up everything to
> the stack and let it filter in software because there is no multicast
> socket listening on that address.

OK.

> > Also, I think it's weird that we have one API to program address and a
> > completely different API (via switchdev) to program address+VID pairs.
> > Extending current API might make more sense.
> > 
> 
> Do you mean ndo_set_rx_mode() and dev_mc_sync()? That is what Ivan
> proposed doing not so long ago here:
> 
> https://www.spinics.net/lists/netdev/msg537424.html
> 
> but that is IMHO wasting storage space, because the kernel is
> maintaining the address lists, and now also needs to gain knowledge
> about the VID. With up to 4K - 2 VLAN interfaces per switch port, this
> bloats the memory footprint, we arguably still need to maintain those
> address lists anyway...

I didn't review Ivan's changes in details, but it makes much more sense
to me to simply extend the current Rx filtering mechanism than to use a
completely unrelated infrastructure.

> 
> The reason why I chose switchdev here is because:
> 
> - this is mostly relevant for switch devices, not so much for NICs (it
> seems), if it was, they would have solved the problem by now

I don't see any use of switchdev APIs in the driver Ivan is patching.
The cover letter doesn't indicate anything about it either.

> - this allows to have an unified path from the switch driver perspective
> to program MDB addresses targeting the CPU/management port, no need to
> have X different ways of doing the same operation

But it's not the same thing. Allowing certain packets to ingress the
device is not the same as having the device send them to the CPU. We
have VLAN filters as well. Allowing VID X to ingress does not mean that
we trap each packet with this VID to CPU.

Re: [PATCH net-next 10/14] net: vlan: Propagate MC addresses with VID through switchdev

[Ilias Apalodimas]

Hi Ido,
> > 
> > The reason why I chose switchdev here is because:
> > 
> > - this is mostly relevant for switch devices, not so much for NICs (it
> > seems), if it was, they would have solved the problem by now
> 
> I don't see any use of switchdev APIs in the driver Ivan is patching.
> The cover letter doesn't indicate anything about it either.

There were RFCs for it a few months ago https://patchwork.ozlabs.org/cover/929367/

We decided that rewriting the driver instead of adding switchdev support on
the current one is cleaner and preferred, so we'll be posting a new driver for
this at some point (most of the work is already done).

> 
> > - this allows to have an unified path from the switch driver perspective
> > to program MDB addresses targeting the CPU/management port, no need to
> > have X different ways of doing the same operation
> 
> But it's not the same thing. Allowing certain packets to ingress the
> device is not the same as having the device send them to the CPU. We
> have VLAN filters as well. Allowing VID X to ingress does not mean that
> we trap each packet with this VID to CPU.
/Ilias

Re: [PATCH net-next 10/14] net: vlan: Propagate MC addresses with VID through switchdev

[Ivan Khoronzhuk]

On Mon, Jan 21, 2019 at 09:13:01AM +0000, Ido Schimmel wrote:
>On Thu, Jan 17, 2019 at 11:12:24AM -0800, Florian Fainelli wrote:
>> On 1/17/19 6:49 AM, Ido Schimmel wrote:
>> > On Wed, Jan 16, 2019 at 12:00:58PM -0800, Florian Fainelli wrote:
>> >> The VLAN real device could be an Ethernet switch port and that switch
>> >> might have VLAN filtering globally enabled (because of a bridge
>> >> requesting VLAN filtering on the switch on another port) and so when
>> >> programming multicast addresses, we need the multicast filter
>> >> programming to be aware of the correct VLAN ID as well.
>> >
>> > This looks like a quirk of a specific device. How bad is it to patch the
>> > driver to add a multicast address for every configured VLAN?
>>
>> There is at least another driver that can be benefit from that which is
>> cpsw, if I understand Ivan's use case correctly.
>
>I understand and I have no argument against the need for this. I just
>think we should use a different mechanism than switchdev.
>
>> If there is a ndo_set_rx_mode() function implemented by the virtual
>> device, and that does call dev_{mc,uc}_sync(master, dev), then this
>> means that you do want to be able to filter UC and MC addresses. If we
>> added the entire class D range of multicast addresses to the switch's
>> MDB, that would not be filtering, we would be passing up everything to
>> the stack and let it filter in software because there is no multicast
>> socket listening on that address.
>
>OK.
>
>> > Also, I think it's weird that we have one API to program address and a
>> > completely different API (via switchdev) to program address+VID pairs.
>> > Extending current API might make more sense.
>> >
>>
>> Do you mean ndo_set_rx_mode() and dev_mc_sync()? That is what Ivan
>> proposed doing not so long ago here:
>>
>> https://www.spinics.net/lists/netdev/msg537424.html
>>
>> but that is IMHO wasting storage space, because the kernel is
>> maintaining the address lists, and now also needs to gain knowledge
>> about the VID. With up to 4K - 2 VLAN interfaces per switch port, this
>> bloats the memory footprint, we arguably still need to maintain those
>> address lists anyway...

Yes wasting storage it's a factor, even it brings a lot of other
benefits. But compared to this it's more legal.

As alternative I've also proposed completely working model when vlan
identification is real dev responsibility, allowing to split address
space between vlans (for mc and uc). And better to add more commments
to the referencies I provided there.

>
>I didn't review Ivan's changes in details, but it makes much more sense
>to me to simply extend the current Rx filtering mechanism than to use a
>completely unrelated infrastructure.

True.

>
>>
>> The reason why I chose switchdev here is because:
>>
>> - this is mostly relevant for switch devices, not so much for NICs (it
>> seems), if it was, they would have solved the problem by now
>
>I don't see any use of switchdev APIs in the driver Ivan is patching.
>The cover letter doesn't indicate anything about it either.

Yes, it's not related to switched.

>
>> - this allows to have an unified path from the switch driver perspective
>> to program MDB addresses targeting the CPU/management port, no need to
>> have X different ways of doing the same operation
>
>But it's not the same thing. Allowing certain packets to ingress the
>device is not the same as having the device send them to the CPU. We
>have VLAN filters as well. Allowing VID X to ingress does not mean that
>we trap each packet with this VID to CPU.

-- 
Regards,
Ivan Khoronzhuk

Re: [PATCH net-next 10/14] net: vlan: Propagate MC addresses with VID through switchdev

[Ivan Khoronzhuk]

On Thu, Jan 17, 2019 at 11:12:24AM -0800, Florian Fainelli wrote:
>On 1/17/19 6:49 AM, Ido Schimmel wrote:
>> On Wed, Jan 16, 2019 at 12:00:58PM -0800, Florian Fainelli wrote:
>>> The VLAN real device could be an Ethernet switch port and that switch
>>> might have VLAN filtering globally enabled (because of a bridge
>>> requesting VLAN filtering on the switch on another port) and so when
>>> programming multicast addresses, we need the multicast filter
>>> programming to be aware of the correct VLAN ID as well.
>>
>> This looks like a quirk of a specific device. How bad is it to patch the
>> driver to add a multicast address for every configured VLAN?
>
>There is at least another driver that can be benefit from that which is
>cpsw, if I understand Ivan's use case correctly.
>
>If there is a ndo_set_rx_mode() function implemented by the virtual
>device, and that does call dev_{mc,uc}_sync(master, dev), then this
>means that you do want to be able to filter UC and MC addresses. If we
>added the entire class D range of multicast addresses to the switch's
>MDB, that would not be filtering, we would be passing up everything to
>the stack and let it filter in software because there is no multicast
>socket listening on that address.
>
>>
>> Also, I think it's weird that we have one API to program address and a
>> completely different API (via switchdev) to program address+VID pairs.
>> Extending current API might make more sense.
>>
>
>Do you mean ndo_set_rx_mode() and dev_mc_sync()? That is what Ivan
>proposed doing not so long ago here:
>
>https://www.spinics.net/lists/netdev/msg537424.html
>
>but that is IMHO wasting storage space, because the kernel is
>maintaining the address lists, and now also needs to gain knowledge
>about the VID. With up to 4K - 2 VLAN interfaces per switch port, this
>bloats the memory footprint, we arguably still need to maintain those
>address lists anyway...
>
>The reason why I chose switchdev here is because:
>
>- this is mostly relevant for switch devices, not so much for NICs (it
>seems), if it was, they would have solved the problem by now
If provide normal interface for this it could spread.
And personally my aim is to avoid redundant mcast traffic when i don't
expect it saving CPU performance, latency .... and overall it should
be done in this way, switchdev here seems like otth.

>
>- this allows to have an unified path from the switch driver perspective
>to program MDB addresses targeting the CPU/management port, no need to
>have X different ways of doing the same operation
>-- 
>Florian

-- 
Regards,
Ivan Khoronzhuk