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

[PATCH net-next v2 00/12] 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.

Changes in v2:

- correctly propagate return from br_mc_disabled_update() called from
  br_multicast_toggle

- dropped changes to VLAN and bridge code that was pushing multicast
  entries with VID and instead refuse disabling multicast snooping in
  b53 when there is another standalone port, or that there are other
  bridged ports with multicast snooping enabled

Florian Fainelli (12):
  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: 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           | 257 +++++++++++++++++++--
 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/bridge/br_multicast.c                  |  23 +-
 net/dsa/dsa_priv.h                         |  22 +-
 net/dsa/port.c                             |  42 +++-
 net/dsa/slave.c                            | 107 ++++++++-
 net/dsa/switch.c                           |  57 +++++
 12 files changed, 552 insertions(+), 59 deletions(-)

-- 
2.17.1

[PATCH net-next v2 01/12] 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 | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 3aeff0895669..aff5e003d34f 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -813,20 +813,22 @@ 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,
 		.id = SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
-		.flags = SWITCHDEV_F_DEFER,
+		.flags = SWITCHDEV_F_DEFER | SWITCHDEV_F_SKIP_EOPNOTSUPP,
 		.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 = 0;
 
 	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;
@@ -1957,7 +1966,7 @@ int br_multicast_toggle(struct net_bridge *br, unsigned long val)
 unlock:
 	spin_unlock_bh(&br->multicast_lock);
 
-	return 0;
+	return err;
 }
 
 bool br_multicast_enabled(const struct net_device *dev)
-- 
2.17.1

[PATCH net-next v2 02/12] 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 v2 03/12] 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 v2 04/12] 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 v2 05/12] 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 v2 06/12] 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 v2 07/12] 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 91de3a663226..a6a803262929 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);
 }
@@ -1396,6 +1441,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 v2 08/12] 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 a6a803262929..306fd1b45f0c 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,
@@ -1091,6 +1139,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 = {
@@ -1351,7 +1401,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 v2 09/12] 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 1f4972dab9f2..1e3db5f2a699 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 v2 10/12] 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 7f2a668ef2cc..2ee1ede7df5c 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -425,6 +425,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 1e3db5f2a699..221753777cf5 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,
@@ -155,6 +164,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 306fd1b45f0c..f3b3cf34804f 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 v2 11/12] 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 | 68 ++++++++++++++++++++++++++++++++
 drivers/net/dsa/b53/b53_priv.h   |  2 +
 2 files changed, 70 insertions(+)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 6c894ad4768a..c0c064a544b9 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1640,6 +1640,73 @@ 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)
+{
+	unsigned int cpu_port = dsa_to_port(ds, port)->cpu_dp->index;
+	struct b53_device *dev = ds->priv;
+	struct net_device *bridge_dev;
+	struct dsa_port *dp;
+	unsigned int i;
+	u8 port_ctrl;
+	u16 mc_ctrl;
+
+	if (is5325(dev) || is5365(dev))
+		return -EOPNOTSUPP;
+
+	/* 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. We must also take care of non-bridged ports which
+	 * expect multicast filtering to remain turned on.
+	 */
+	for (i = 0; i < ds->num_ports; i++) {
+		if (dsa_is_unused_port(ds, i) || dsa_is_cpu_port(ds, i))
+			continue;
+
+		if (i == port)
+			continue;
+
+		dp = dsa_to_port(ds, i);
+		bridge_dev = dp->bridge_dev;
+		if ((bridge_dev &&
+		     bridge_dev != dsa_to_port(ds, port)->bridge_dev &&
+		     br_multicast_enabled(bridge_dev) != !mc_disabled) ||
+		     (!bridge_dev && mc_disabled)) {
+			netdev_err(dp->slave,
+				   "MC filtering is global to the switch!\n");
+			return -EINVAL;
+		}
+	}
+
+	/* Allow CPU port to receive multicast traffic */
+	b53_read8(dev, B53_CTRL_PAGE, B53_PORT_CTRL(cpu_port), &port_ctrl);
+	if (mc_disabled)
+		port_ctrl |= PORT_CTRL_RX_MCST_EN;
+	else
+		port_ctrl &= ~PORT_CTRL_RX_MCST_EN;
+	b53_write8(dev, B53_CTRL_PAGE, B53_PORT_CTRL(cpu_port), port_ctrl);
+
+	/* 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 +2092,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 v2 12/12] 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 c0c064a544b9..10e115a43975 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 v2 01/12] net: bridge: multicast: Propagate br_mc_disabled_update() return

[Ido Schimmel]

On Tue, Jan 29, 2019 at 04:55:37PM -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 | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 3aeff0895669..aff5e003d34f 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -813,20 +813,22 @@ 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,
>  		.id = SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
> -		.flags = SWITCHDEV_F_DEFER,
> +		.flags = SWITCHDEV_F_DEFER | SWITCHDEV_F_SKIP_EOPNOTSUPP,

Actually, since the operation is deferred I don't think the return value
from the driver is ever checked. Can you test it?

I think it would be good to convert the attributes to use the switchdev
notifier like commit d17d9f5e5143 ("switchdev: Replace port obj add/del
SDO with a notification") did for objects. Then you can have your
listener veto the operation in the same context it is happening.

>  		.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 = 0;
>  
>  	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;
> @@ -1957,7 +1966,7 @@ int br_multicast_toggle(struct net_bridge *br, unsigned long val)
>  unlock:
>  	spin_unlock_bh(&br->multicast_lock);
>  
> -	return 0;
> +	return err;
>  }
>  
>  bool br_multicast_enabled(const struct net_device *dev)
> -- 
> 2.17.1
> 

Re: [PATCH net-next v2 00/12] net: dsa: management mode for bcm_sf2

[Ido Schimmel]

On Tue, Jan 29, 2019 at 04:55:36PM -0800, Florian Fainelli wrote:
> 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.

You're not touching net/8021q :) Probably leftover from v1

...

> 
>  drivers/net/dsa/b53/b53_common.c           | 257 +++++++++++++++++++--
>  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/bridge/br_multicast.c                  |  23 +-
>  net/dsa/dsa_priv.h                         |  22 +-
>  net/dsa/port.c                             |  42 +++-
>  net/dsa/slave.c                            | 107 ++++++++-
>  net/dsa/switch.c                           |  57 +++++
>  12 files changed, 552 insertions(+), 59 deletions(-)
> 
> -- 
> 2.17.1
> 

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

[Andrew Lunn]

On Tue, Jan 29, 2019 at 04:55:46PM -0800, Florian Fainelli wrote:
> 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 7f2a668ef2cc..2ee1ede7df5c 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -425,6 +425,8 @@ struct dsa_switch_ops {
>  	/*
>  	 * Multicast database
>  	 */
> +	int	(*port_multicast_toggle)(struct dsa_switch *ds, int port,
> +					 bool mc_disabled);


Hi Florin

Looks like there is an extra tab in there?

      Andrew

Re: [PATCH net-next v2 00/12] net: dsa: management mode for bcm_sf2

[David Miller]

Florian, Ido has a question about how the driver return value is
propagated wrt. patch #1 since the operation is deferred.

Please address this.

Thanks!

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

[Vivien Didelot]

Hi Florian,

On Tue, 29 Jan 2019 16:55:43 -0800, Florian Fainelli <f.fainelli@gmail.com> wrote:

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

Assignment unneeded here.

> +
> +	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);
> +}

This wrapper isn't necessary IMO. I'd go with something like:

static int dsa_slave_sync(struct net_device *dev, const unsigned char *addr)
{
	struct switchdev_obj_port_mdb mdb = {
		.obj.id = SWITCHDEV_OBJ_ID_HOST_MDB,
		.obj.flags = SWITCHDEV_F_DEFER,
	};

	ether_addr_copy(mdb.addr, addr);

	return switchdev_port_obj_add(dev, &mdb.obj, NULL);
}

static int dsa_slave_unsync(struct net_device *dev, const unsigned char *addr)
{
	struct switchdev_obj_port_mdb mdb = {
		.obj.id = SWITCHDEV_OBJ_ID_HOST_MDB,
		.obj.flags = SWITCHDEV_F_DEFER,
	};

	ether_addr_copy(mdb.addr, addr);

	return switchdev_port_obj_del(dev, &mdb.obj);
}

We may eventually wrap this cryptic netdevery in:

static int dsa_slave_mc_sync(struct net_device *dev)
{
	return __hw_addr_sync_dev(&dev->mc, dev, dsa_slave_sync, dsa_slave_unsync);
}

static void dsa_slave_mc_unsync(struct net_device *dev)
{
	__hw_addr_unsync_dev(&dev->mc, dev, dsa_slave_sync);
}

> +
>  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);

And check the returned error code.

> +out:
>  	dev_mc_sync(master, dev);
>  	dev_uc_sync(master, dev);
>  }


Thanks,

	Vivien

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

[Florian Fainelli]

On 1/30/19 8:06 AM, Andrew Lunn wrote:
> On Tue, Jan 29, 2019 at 04:55:46PM -0800, Florian Fainelli wrote:
>> 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 7f2a668ef2cc..2ee1ede7df5c 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -425,6 +425,8 @@ struct dsa_switch_ops {
>>  	/*
>>  	 * Multicast database
>>  	 */
>> +	int	(*port_multicast_toggle)(struct dsa_switch *ds, int port,
>> +					 bool mc_disabled);
> 
> 
> Hi Florin
> 
> Looks like there is an extra tab in there?

In the first or second line?
-- 
Florian

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

[Vivien Didelot]

Hi Florian,

On Tue, 29 Jan 2019 16:55:44 -0800, Florian Fainelli <f.fainelli@gmail.com> wrote:

> -	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)

        if (!dp->bridge_dev || br_vlan_enabled(dp->bridge_dev))

This would be sufficient.

>  
>  	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)

Same here.


Thanks,

	Vivien

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

[Andrew Lunn]

> @@ -425,6 +425,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,

Hi Florian

I took a second look at this, after you asked the question.

What i see above is that port_multicast_toggle is different to
port_mdb_prepare and port_mdb_add. From this context, it looks like
there should not be a tab between int and (*port_multicast_toggle).

However, when you look at the real file, it becomes clear that it is
actually normal to have a tab between the type and the name of the
function pointer, and that port_mdb_prepare and port_mdb_add are
different to the rest.

So you patch is O.K.

   Andrew

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

[Florian Fainelli]

On 1/30/19 2:28 PM, Vivien Didelot wrote:
> Hi Florian,
> 
> On Tue, 29 Jan 2019 16:55:43 -0800, Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
>> +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;
> 
> Assignment unneeded here.
> 
>> +
>> +	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);
>> +}
> 
> This wrapper isn't necessary IMO. I'd go with something like:
> 
> static int dsa_slave_sync(struct net_device *dev, const unsigned char *addr)
> {
> 	struct switchdev_obj_port_mdb mdb = {
> 		.obj.id = SWITCHDEV_OBJ_ID_HOST_MDB,
> 		.obj.flags = SWITCHDEV_F_DEFER,
> 	};
> 
> 	ether_addr_copy(mdb.addr, addr);
> 
> 	return switchdev_port_obj_add(dev, &mdb.obj, NULL);
> }
> 
> static int dsa_slave_unsync(struct net_device *dev, const unsigned char *addr)
> {
> 	struct switchdev_obj_port_mdb mdb = {
> 		.obj.id = SWITCHDEV_OBJ_ID_HOST_MDB,
> 		.obj.flags = SWITCHDEV_F_DEFER,
> 	};
> 
> 	ether_addr_copy(mdb.addr, addr);
> 
> 	return switchdev_port_obj_del(dev, &mdb.obj);
> }
> 
> We may eventually wrap this cryptic netdevery in:
> 
> static int dsa_slave_mc_sync(struct net_device *dev)
> {
> 	return __hw_addr_sync_dev(&dev->mc, dev, dsa_slave_sync, dsa_slave_unsync);
> }
> 
> static void dsa_slave_mc_unsync(struct net_device *dev)
> {
> 	__hw_addr_unsync_dev(&dev->mc, dev, dsa_slave_sync);
> }
> 
>> +
>>  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);
> 
> And check the returned error code.

All good points, I have now incorporated your suggestions, thanks!
-- 
Florian

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

[Florian Fainelli]

On 1/30/19 2:46 PM, Andrew Lunn wrote:
>> @@ -425,6 +425,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,
> 
> Hi Florian
> 
> I took a second look at this, after you asked the question.
> 
> What i see above is that port_multicast_toggle is different to
> port_mdb_prepare and port_mdb_add. From this context, it looks like
> there should not be a tab between int and (*port_multicast_toggle).
> 
> However, when you look at the real file, it becomes clear that it is
> actually normal to have a tab between the type and the name of the
> function pointer, and that port_mdb_prepare and port_mdb_add are
> different to the rest.
> 
> So you patch is O.K.

No worries, thanks for taking a look!
-- 
Florian

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

[Florian Fainelli]

On 1/29/19 11:36 PM, Ido Schimmel wrote:
> On Tue, Jan 29, 2019 at 04:55:37PM -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 | 23 ++++++++++++++++-------
>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>> index 3aeff0895669..aff5e003d34f 100644
>> --- a/net/bridge/br_multicast.c
>> +++ b/net/bridge/br_multicast.c
>> @@ -813,20 +813,22 @@ 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,
>>  		.id = SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
>> -		.flags = SWITCHDEV_F_DEFER,
>> +		.flags = SWITCHDEV_F_DEFER | SWITCHDEV_F_SKIP_EOPNOTSUPP,
> 
> Actually, since the operation is deferred I don't think the return value
> from the driver is ever checked. Can you test it?

You are right, you get a WARN() from switchdev_attr_port_set_now(), but
this does not propagate back to the caller, so you can still create a
bridge device and enslave a device successfully despite getting warnings
on the console.

> 
> I think it would be good to convert the attributes to use the switchdev
> notifier like commit d17d9f5e5143 ("switchdev: Replace port obj add/del
> SDO with a notification") did for objects. Then you can have your
> listener veto the operation in the same context it is happening.

Alright, working on it. Would you do that just for the attr_set, or for
attr_get as well (to be symmetrical)?
-- 
Florian

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

[Ido Schimmel]

On Wed, Jan 30, 2019 at 05:00:57PM -0800, Florian Fainelli wrote:
> On 1/29/19 11:36 PM, Ido Schimmel wrote:
> > On Tue, Jan 29, 2019 at 04:55:37PM -0800, Florian Fainelli wrote:
> >> -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,
> >>  		.id = SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
> >> -		.flags = SWITCHDEV_F_DEFER,
> >> +		.flags = SWITCHDEV_F_DEFER | SWITCHDEV_F_SKIP_EOPNOTSUPP,
> > 
> > Actually, since the operation is deferred I don't think the return value
> > from the driver is ever checked. Can you test it?
> 
> You are right, you get a WARN() from switchdev_attr_port_set_now(), but
> this does not propagate back to the caller, so you can still create a
> bridge device and enslave a device successfully despite getting warnings
> on the console.
> 
> > 
> > I think it would be good to convert the attributes to use the switchdev
> > notifier like commit d17d9f5e5143 ("switchdev: Replace port obj add/del
> > SDO with a notification") did for objects. Then you can have your
> > listener veto the operation in the same context it is happening.
> 
> Alright, working on it. Would you do that just for the attr_set, or for
> attr_get as well (to be symmetrical)?

Yes, then we can get rid of switchdev_ops completely.

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

[Florian Fainelli]

On 1/30/19 11:50 PM, Ido Schimmel wrote:
> On Wed, Jan 30, 2019 at 05:00:57PM -0800, Florian Fainelli wrote:
>> On 1/29/19 11:36 PM, Ido Schimmel wrote:
>>> On Tue, Jan 29, 2019 at 04:55:37PM -0800, Florian Fainelli wrote:
>>>> -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,
>>>>  		.id = SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
>>>> -		.flags = SWITCHDEV_F_DEFER,
>>>> +		.flags = SWITCHDEV_F_DEFER | SWITCHDEV_F_SKIP_EOPNOTSUPP,
>>>
>>> Actually, since the operation is deferred I don't think the return value
>>> from the driver is ever checked. Can you test it?
>>
>> You are right, you get a WARN() from switchdev_attr_port_set_now(), but
>> this does not propagate back to the caller, so you can still create a
>> bridge device and enslave a device successfully despite getting warnings
>> on the console.
>>
>>>
>>> I think it would be good to convert the attributes to use the switchdev
>>> notifier like commit d17d9f5e5143 ("switchdev: Replace port obj add/del
>>> SDO with a notification") did for objects. Then you can have your
>>> listener veto the operation in the same context it is happening.
>>
>> Alright, working on it. Would you do that just for the attr_set, or for
>> attr_get as well (to be symmetrical)?
> 
> Yes, then we can get rid of switchdev_ops completely.
> 

OK, so here is what I have so far:

https://github.com/ffainelli/linux/pull/new/switchdev-attr

although I am seeing some invalid context operations with DSA that I am
debugging. Does this look like the right way to go from your perspective?
-- 
Florian

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

[Ido Schimmel]

On Thu, Jan 31, 2019 at 05:19:25PM -0800, Florian Fainelli wrote:
> On 1/30/19 11:50 PM, Ido Schimmel wrote:
> > On Wed, Jan 30, 2019 at 05:00:57PM -0800, Florian Fainelli wrote:
> >> On 1/29/19 11:36 PM, Ido Schimmel wrote:
> >>> On Tue, Jan 29, 2019 at 04:55:37PM -0800, Florian Fainelli wrote:
> >>>> -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,
> >>>>  		.id = SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
> >>>> -		.flags = SWITCHDEV_F_DEFER,
> >>>> +		.flags = SWITCHDEV_F_DEFER | SWITCHDEV_F_SKIP_EOPNOTSUPP,
> >>>
> >>> Actually, since the operation is deferred I don't think the return value
> >>> from the driver is ever checked. Can you test it?
> >>
> >> You are right, you get a WARN() from switchdev_attr_port_set_now(), but
> >> this does not propagate back to the caller, so you can still create a
> >> bridge device and enslave a device successfully despite getting warnings
> >> on the console.
> >>
> >>>
> >>> I think it would be good to convert the attributes to use the switchdev
> >>> notifier like commit d17d9f5e5143 ("switchdev: Replace port obj add/del
> >>> SDO with a notification") did for objects. Then you can have your
> >>> listener veto the operation in the same context it is happening.
> >>
> >> Alright, working on it. Would you do that just for the attr_set, or for
> >> attr_get as well (to be symmetrical)?
> > 
> > Yes, then we can get rid of switchdev_ops completely.
> > 
> 
> OK, so here is what I have so far:
> 
> https://github.com/ffainelli/linux/pull/new/switchdev-attr
> 
> although I am seeing some invalid context operations with DSA that I am
> debugging. Does this look like the right way to go from your perspective?

That was quick :)

I think I owe you some explanation as to why I even came up with the
idea. But before that, I have another idea how to solve your immediate
problem.

You can employ a similar trick to the one used to set bridge port flags
in br_switchdev_set_port_flag(). Like br_mc_disabled_update() it is also
called from an atomic context, so in order to allow drivers to veto
unsupported flags we introduced a new switchdev attribute:
'SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT'

The attribute is only used in get operations and never deferred. You can
introduce 'SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED_SUPPORT' and use it
before invoking 'SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED'.

Regarding the whole notifier business and switchdev in general. Using
the device chain to propagate various bridge port attributes is wrong.
We saw it multiple times in the past already. First with routes that
were converted to use notifiers because the switch needs to offload the
entire routing table and not only routes whose nexthop device is a
switch port. Then with FDB entries and recently also with VLANs in a
VLAN-aware bridge. See merge commit 02e1dbe402de ("Merge branch
'Pass-extack-to-SWITCHDEV_PORT_OBJ_ADD'") for more details.

This is also true for switchdev attributes since we might want to
support toggling of bridge port flags on a VXLAN device in the future.
For example, to allow selective flooding. Others might have more use
cases.

I want to use the opportunity to pick your brain (and others') about
more issues I see with switchdev and maybe reach some agreement and form
a plan. Just to be clear, it is not related to your patchset.

The prepare-commit model probably made sense in the beginning, but a few
years later I think we know better. At least in mlxsw we have multiple
places where we perform all the work in the prepare phase simply because
that without doing all the work we don't have a way to guarantee that
the commit phase will succeed. I'm not aware of other instances of this
model in the networking code, so I wonder why we need it in switchdev
and if we can simply remove it and simplify things.

Another issue is that I believe we can completely remove the switchdev
infrastructure as it basically boils down to bridge-specific notifiers.
If you look at where switchdev is actually used in the networking stack
while excluding the obvious suspects you get this:

$ git grep -i -n -e 'switchdev' -- 'net/*' ':!net/bridge/*' ':!net/dsa/*' ':!net/switchdev/'                                                         
net/8021q/vlan_dev.c:34:#include <net/switchdev.h>
net/Kconfig:240:source "net/switchdev/Kconfig"
net/Makefile:81:ifneq ($(CONFIG_NET_SWITCHDEV),)
net/Makefile:82:obj-y                           += switchdev/
net/core/net-sysfs.c:15:#include <net/switchdev.h>
net/core/net-sysfs.c:504:               struct switchdev_attr attr = {
net/core/net-sysfs.c:506:                       .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID,                                                                                     
net/core/net-sysfs.c:507:                       .flags = SWITCHDEV_F_NO_RECURSE,
net/core/net-sysfs.c:510:               ret = switchdev_port_attr_get(netdev, &attr);
net/core/rtnetlink.c:49:#include <net/switchdev.h>
net/core/rtnetlink.c:1150:      struct switchdev_attr attr = {
net/core/rtnetlink.c:1152:              .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID,
net/core/rtnetlink.c:1153:              .flags = SWITCHDEV_F_NO_RECURSE,
net/core/rtnetlink.c:1156:      err = switchdev_port_attr_get(dev, &attr);
net/core/skbuff.c:4925:#ifdef CONFIG_NET_SWITCHDEV
net/ipv4/ip_forward.c:72:#ifdef CONFIG_NET_SWITCHDEV
net/ipv4/ipmr.c:70:#include <net/switchdev.h>
net/ipv4/ipmr.c:841:    struct switchdev_attr attr = {
net/ipv4/ipmr.c:842:            .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID,
net/ipv4/ipmr.c:923:    if (!switchdev_port_attr_get(dev, &attr)) {
net/ipv4/ipmr.c:1802:#ifdef CONFIG_NET_SWITCHDEV
net/ipv6/ip6_output.c:381:#ifdef CONFIG_NET_SWITCHDEV

These are all instances related to the use of parent ID. Why not add a
new ndo that will allow various users to query the parent ID of a
netdev? bond and team can return an error if their slaves don't all have
the same parent ID.

You then end up with basic constructs like notifiers and ndo invocations
and not with complex objects and attributes that are propagated via the
device chain with a prepare-commit model.

WDYT?

> -- 
> Florian

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

[Florian Fainelli]

On 2/2/19 7:47 AM, Ido Schimmel wrote:
> On Thu, Jan 31, 2019 at 05:19:25PM -0800, Florian Fainelli wrote:
>> On 1/30/19 11:50 PM, Ido Schimmel wrote:
>>> On Wed, Jan 30, 2019 at 05:00:57PM -0800, Florian Fainelli wrote:
>>>> On 1/29/19 11:36 PM, Ido Schimmel wrote:
>>>>> On Tue, Jan 29, 2019 at 04:55:37PM -0800, Florian Fainelli wrote:
>>>>>> -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,
>>>>>>  		.id = SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
>>>>>> -		.flags = SWITCHDEV_F_DEFER,
>>>>>> +		.flags = SWITCHDEV_F_DEFER | SWITCHDEV_F_SKIP_EOPNOTSUPP,
>>>>>
>>>>> Actually, since the operation is deferred I don't think the return value
>>>>> from the driver is ever checked. Can you test it?
>>>>
>>>> You are right, you get a WARN() from switchdev_attr_port_set_now(), but
>>>> this does not propagate back to the caller, so you can still create a
>>>> bridge device and enslave a device successfully despite getting warnings
>>>> on the console.
>>>>
>>>>>
>>>>> I think it would be good to convert the attributes to use the switchdev
>>>>> notifier like commit d17d9f5e5143 ("switchdev: Replace port obj add/del
>>>>> SDO with a notification") did for objects. Then you can have your
>>>>> listener veto the operation in the same context it is happening.
>>>>
>>>> Alright, working on it. Would you do that just for the attr_set, or for
>>>> attr_get as well (to be symmetrical)?
>>>
>>> Yes, then we can get rid of switchdev_ops completely.
>>>
>>
>> OK, so here is what I have so far:
>>
>> https://github.com/ffainelli/linux/pull/new/switchdev-attr
>>
>> although I am seeing some invalid context operations with DSA that I am
>> debugging. Does this look like the right way to go from your perspective?
> 
> That was quick :)
> 
> I think I owe you some explanation as to why I even came up with the
> idea. But before that, I have another idea how to solve your immediate
> problem.
> 
> You can employ a similar trick to the one used to set bridge port flags
> in br_switchdev_set_port_flag(). Like br_mc_disabled_update() it is also
> called from an atomic context, so in order to allow drivers to veto
> unsupported flags we introduced a new switchdev attribute:
> 'SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT'

Yes that is a great idea, for some reason I completely missed your email
here, but this is how I am going to approach it now.

Another way to look at the problem could be to question whether we
really need to be in atomic context when such attributes are pushed?
AFAICT, we are executing with BH disabled because we want to protect the
bridge master's bridge port list, right?

> 
> The attribute is only used in get operations and never deferred. You can
> introduce 'SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED_SUPPORT' and use it
> before invoking 'SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED'.
> 
> Regarding the whole notifier business and switchdev in general. Using
> the device chain to propagate various bridge port attributes is wrong.
> We saw it multiple times in the past already. First with routes that
> were converted to use notifiers because the switch needs to offload the
> entire routing table and not only routes whose nexthop device is a
> switch port. Then with FDB entries and recently also with VLANs in a
> VLAN-aware bridge. See merge commit 02e1dbe402de ("Merge branch
> 'Pass-extack-to-SWITCHDEV_PORT_OBJ_ADD'") for more details.
> 
> This is also true for switchdev attributes since we might want to
> support toggling of bridge port flags on a VXLAN device in the future.
> For example, to allow selective flooding. Others might have more use
> cases.
> 
> I want to use the opportunity to pick your brain (and others') about
> more issues I see with switchdev and maybe reach some agreement and form
> a plan. Just to be clear, it is not related to your patchset.
> 
> The prepare-commit model probably made sense in the beginning, but a few
> years later I think we know better. At least in mlxsw we have multiple
> places where we perform all the work in the prepare phase simply because
> that without doing all the work we don't have a way to guarantee that
> the commit phase will succeed. I'm not aware of other instances of this
> model in the networking code, so I wonder why we need it in switchdev
> and if we can simply remove it and simplify things.

If we can at least re-purpose the prepare + commit model such that the
prepare phase is: can you support that operation (without allocating
resources) and do that operation in the caller context, while the commit
is deferred, then that would solve some of my issues but it could create
more for mlxsw possibly?

I have to wonder though, if we have a driver which is constructed such that:

- all HW programming is done
- a SW resource (e.g.: a rule object) is allocated last, then if the
allocation fails, we should be able to easily rollback the HW
programming we just did

Would that work?

> 
> Another issue is that I believe we can completely remove the switchdev
> infrastructure as it basically boils down to bridge-specific notifiers.
> If you look at where switchdev is actually used in the networking stack
> while excluding the obvious suspects you get this:
> 
> $ git grep -i -n -e 'switchdev' -- 'net/*' ':!net/bridge/*' ':!net/dsa/*' ':!net/switchdev/'                                                         
> net/8021q/vlan_dev.c:34:#include <net/switchdev.h>
> net/Kconfig:240:source "net/switchdev/Kconfig"
> net/Makefile:81:ifneq ($(CONFIG_NET_SWITCHDEV),)
> net/Makefile:82:obj-y                           += switchdev/
> net/core/net-sysfs.c:15:#include <net/switchdev.h>
> net/core/net-sysfs.c:504:               struct switchdev_attr attr = {
> net/core/net-sysfs.c:506:                       .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID,                                                                                     
> net/core/net-sysfs.c:507:                       .flags = SWITCHDEV_F_NO_RECURSE,
> net/core/net-sysfs.c:510:               ret = switchdev_port_attr_get(netdev, &attr);
> net/core/rtnetlink.c:49:#include <net/switchdev.h>
> net/core/rtnetlink.c:1150:      struct switchdev_attr attr = {
> net/core/rtnetlink.c:1152:              .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID,
> net/core/rtnetlink.c:1153:              .flags = SWITCHDEV_F_NO_RECURSE,
> net/core/rtnetlink.c:1156:      err = switchdev_port_attr_get(dev, &attr);
> net/core/skbuff.c:4925:#ifdef CONFIG_NET_SWITCHDEV
> net/ipv4/ip_forward.c:72:#ifdef CONFIG_NET_SWITCHDEV
> net/ipv4/ipmr.c:70:#include <net/switchdev.h>
> net/ipv4/ipmr.c:841:    struct switchdev_attr attr = {
> net/ipv4/ipmr.c:842:            .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID,
> net/ipv4/ipmr.c:923:    if (!switchdev_port_attr_get(dev, &attr)) {
> net/ipv4/ipmr.c:1802:#ifdef CONFIG_NET_SWITCHDEV
> net/ipv6/ip6_output.c:381:#ifdef CONFIG_NET_SWITCHDEV
> 
> These are all instances related to the use of parent ID. Why not add a
> new ndo that will allow various users to query the parent ID of a
> netdev? bond and team can return an error if their slaves don't all have
> the same parent ID.

This should now be addressed as you might have seen.

> 
> You then end up with basic constructs like notifiers and ndo invocations
> and not with complex objects and attributes that are propagated via the
> device chain with a prepare-commit model.
> 
> WDYT?
> 
>> -- 
>> Florian


-- 
Florian