[PATCH net-next 0/4] net: dsa: mv88e6xxx: fix hardware bridging

[PATCH net-next 0/4] net: dsa: mv88e6xxx: fix hardware bridging

[Vivien Didelot]

DSA and its drivers currently hook the NETDEV_CHANGEUPPER net_device event in
order to configure the VLAN map of every port.

This VLAN map is a feature of these switch chips to hardcode and restrict which
output ports a given input port can egress frames to.

A Linux bridge is a simple untagged VLAN propagated by the bridge code itself.
With a proper 802.1Q support, a driver does not need this hook anymore, and
will simply program the related VLAN object.

This patchset improves the hardware bridging code in the mv88e6xxx driver with
a strict 802.1Q mode.

Ideally, the equivalent must be done for Broadcom Starfighter 2 and Rocker,
before completely getting rid of this hook.

Vivien Didelot (4):
  net: dsa: mv88e6xxx: bridges do not need an FID
  net: dsa: mv88e6xxx: do not support per-port FID
  net: dsa: do not warn unsupported bridge ops
  net: dsa: mv88e6xxx: fix hardware bridging

 drivers/net/dsa/mv88e6171.c |   2 -
 drivers/net/dsa/mv88e6352.c |   2 -
 drivers/net/dsa/mv88e6xxx.c | 215 ++++++--------------------------------------
 drivers/net/dsa/mv88e6xxx.h |   8 --
 net/dsa/slave.c             |   2 +-
 5 files changed, 26 insertions(+), 203 deletions(-)

-- 
2.6.0

[PATCH net-next 1/4] net: dsa: mv88e6xxx: bridges do not need an FID

[Vivien Didelot]

With 88E6352 and similar switch chips, each port has a map to restrict
which output port this input port can egress frames to.

The current driver code implements hardware bridging using this feature,
and assigns to a bridge group the FID of its first member.

Now that 802.1Q is fully implemented in this driver, a Linux bridge
which is a simple untagged VLAN, already gets its own FID.

This patch gets rid of the per-bridge FID and explicits the usage of the
port based VLAN map feature.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 157 +++++++++++---------------------------------
 drivers/net/dsa/mv88e6xxx.h |   1 -
 2 files changed, 40 insertions(+), 118 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index e381bfc..254f9bb 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1046,11 +1046,6 @@ static int _mv88e6xxx_atu_flush(struct dsa_switch *ds, u16 fid, bool static_too)
 	return _mv88e6xxx_atu_flush_move(ds, &entry, static_too);
 }
 
-static int _mv88e6xxx_flush_fid(struct dsa_switch *ds, int fid)
-{
-	return _mv88e6xxx_atu_flush(ds, fid, false);
-}
-
 static int _mv88e6xxx_atu_move(struct dsa_switch *ds, u16 fid, int from_port,
 			       int to_port, bool static_too)
 {
@@ -1112,130 +1107,56 @@ abort:
 	return ret;
 }
 
-/* Must be called with smi lock held */
-static int _mv88e6xxx_update_port_config(struct dsa_switch *ds, int port)
+static int _mv88e6xxx_port_vlan_map_set(struct dsa_switch *ds, int port,
+					u16 output_ports)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
-	u8 fid = ps->fid[port];
-	u16 reg = fid << 12;
+	const u16 mask = (1 << ps->num_ports) - 1;
+	int reg;
 
-	if (dsa_is_cpu_port(ds, port))
-		reg |= ds->phys_port_mask;
-	else
-		reg |= (ps->bridge_mask[fid] |
-		       (1 << dsa_upstream_port(ds))) & ~(1 << port);
+	reg = _mv88e6xxx_reg_read(ds, REG_PORT(port), PORT_BASE_VLAN);
+	if (reg < 0)
+		return reg;
+
+	reg &= ~mask;
+	reg |= output_ports & mask;
 
 	return _mv88e6xxx_reg_write(ds, REG_PORT(port), PORT_BASE_VLAN, reg);
 }
 
-/* Must be called with smi lock held */
-static int _mv88e6xxx_update_bridge_config(struct dsa_switch *ds, int fid)
-{
-	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
-	int port;
-	u32 mask;
-	int ret;
-
-	mask = ds->phys_port_mask;
-	while (mask) {
-		port = __ffs(mask);
-		mask &= ~(1 << port);
-		if (ps->fid[port] != fid)
-			continue;
-
-		ret = _mv88e6xxx_update_port_config(ds, port);
-		if (ret)
-			return ret;
-	}
-
-	return _mv88e6xxx_flush_fid(ds, fid);
-}
-
 /* Bridge handling functions */
 
+static int mv88e6xxx_map_bridge(struct dsa_switch *ds, u16 members)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	const unsigned long output = members | BIT(dsa_upstream_port(ds));
+	int port, err = 0;
+
+	mutex_lock(&ps->smi_mutex);
+
+	for_each_set_bit(port, &output, ps->num_ports) {
+		if (dsa_is_cpu_port(ds, port))
+			continue;
+
+		err = _mv88e6xxx_port_vlan_map_set(ds, port, output & ~port);
+		if (err)
+			break;
+	}
+
+	mutex_unlock(&ps->smi_mutex);
+
+	return err;
+}
+
+
 int mv88e6xxx_join_bridge(struct dsa_switch *ds, int port, u32 br_port_mask)
 {
-	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
-	int ret = 0;
-	u32 nmask;
-	int fid;
-
-	/* If the bridge group is not empty, join that group.
-	 * Otherwise create a new group.
-	 */
-	fid = ps->fid[port];
-	nmask = br_port_mask & ~(1 << port);
-	if (nmask)
-		fid = ps->fid[__ffs(nmask)];
-
-	nmask = ps->bridge_mask[fid] | (1 << port);
-	if (nmask != br_port_mask) {
-		netdev_err(ds->ports[port],
-			   "join: Bridge port mask mismatch fid=%d mask=0x%x expected 0x%x\n",
-			   fid, br_port_mask, nmask);
-		return -EINVAL;
-	}
-
-	mutex_lock(&ps->smi_mutex);
-
-	ps->bridge_mask[fid] = br_port_mask;
-
-	if (fid != ps->fid[port]) {
-		clear_bit(ps->fid[port], ps->fid_bitmap);
-		ps->fid[port] = fid;
-		ret = _mv88e6xxx_update_bridge_config(ds, fid);
-	}
-
-	mutex_unlock(&ps->smi_mutex);
-
-	return ret;
+	return mv88e6xxx_map_bridge(ds, br_port_mask);
 }
 
 int mv88e6xxx_leave_bridge(struct dsa_switch *ds, int port, u32 br_port_mask)
 {
-	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
-	u8 fid, newfid;
-	int ret;
-
-	fid = ps->fid[port];
-
-	if (ps->bridge_mask[fid] != br_port_mask) {
-		netdev_err(ds->ports[port],
-			   "leave: Bridge port mask mismatch fid=%d mask=0x%x expected 0x%x\n",
-			   fid, br_port_mask, ps->bridge_mask[fid]);
-		return -EINVAL;
-	}
-
-	/* If the port was the last port of a bridge, we are done.
-	 * Otherwise assign a new fid to the port, and fix up
-	 * the bridge configuration.
-	 */
-	if (br_port_mask == (1 << port))
-		return 0;
-
-	mutex_lock(&ps->smi_mutex);
-
-	newfid = find_next_zero_bit(ps->fid_bitmap, VLAN_N_VID, 1);
-	if (unlikely(newfid > ps->num_ports)) {
-		netdev_err(ds->ports[port], "all first %d FIDs are used\n",
-			   ps->num_ports);
-		ret = -ENOSPC;
-		goto unlock;
-	}
-
-	ps->fid[port] = newfid;
-	set_bit(newfid, ps->fid_bitmap);
-	ps->bridge_mask[fid] &= ~(1 << port);
-	ps->bridge_mask[newfid] = 1 << port;
-
-	ret = _mv88e6xxx_update_bridge_config(ds, fid);
-	if (!ret)
-		ret = _mv88e6xxx_update_bridge_config(ds, newfid);
-
-unlock:
-	mutex_unlock(&ps->smi_mutex);
-
-	return ret;
+	return mv88e6xxx_map_bridge(ds, br_port_mask & ~port);
 }
 
 int mv88e6xxx_port_stp_update(struct dsa_switch *ds, int port, u8 state)
@@ -2233,10 +2154,12 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
 	ps->fid[port] = fid;
 	set_bit(fid, ps->fid_bitmap);
 
-	if (!dsa_is_cpu_port(ds, port))
-		ps->bridge_mask[fid] = 1 << port;
+	if (dsa_is_cpu_port(ds, port))
+		reg = BIT(ps->num_ports) - 1;
+	else
+		reg = BIT(dsa_upstream_port(ds));
 
-	ret = _mv88e6xxx_update_port_config(ds, port);
+	ret = _mv88e6xxx_port_vlan_map_set(ds, port, reg & ~port);
 	if (ret)
 		goto abort;
 
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 1347a73..716f692 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -411,7 +411,6 @@ struct mv88e6xxx_priv_state {
 
 	DECLARE_BITMAP(fid_bitmap, VLAN_N_VID);	/* FIDs 1 to 4095 available */
 	u16 fid[DSA_MAX_PORTS];			/* per (non-bridged) port FID */
-	u16 bridge_mask[DSA_MAX_PORTS];		/* br groups (indexed by FID) */
 
 	unsigned long port_state_update_mask;
 	u8 port_state[DSA_MAX_PORTS];
-- 
2.6.0

[PATCH net-next 2/4] net: dsa: mv88e6xxx: do not support per-port FID

[Vivien Didelot]

Since we configure a switch chip through a Linux bridge, and a bridge is
implemented as a VLAN, there is no need for per-port FID anymore.

This patch gets rid of this and simplifies the driver code since we can
now directly map all 4095 FIDs available to all VLANs.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 69 ++++++++-------------------------------------
 drivers/net/dsa/mv88e6xxx.h |  5 ----
 2 files changed, 11 insertions(+), 63 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 254f9bb..787352df 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1468,6 +1468,7 @@ static int _mv88e6xxx_vlan_init(struct dsa_switch *ds, u16 vid,
 	struct mv88e6xxx_vtu_stu_entry vlan = {
 		.valid = true,
 		.vid = vid,
+		.fid = vid, /* We use one FID per VLAN */
 	};
 	int i;
 
@@ -1501,22 +1502,10 @@ static int _mv88e6xxx_vlan_init(struct dsa_switch *ds, u16 vid,
 				return err;
 		}
 
-		/* Non-bridged ports and bridge groups use FIDs from 1 to
-		 * num_ports; VLANs use FIDs from num_ports+1 to 4095.
-		 */
-		vlan.fid = find_next_zero_bit(ps->fid_bitmap, VLAN_N_VID,
-					      ps->num_ports + 1);
-		if (unlikely(vlan.fid == VLAN_N_VID)) {
-			pr_err("no more FID available for VLAN %d\n", vid);
-			return -ENOSPC;
-		}
-
 		/* Clear all MAC addresses from the new database */
 		err = _mv88e6xxx_atu_flush(ds, vlan.fid, true);
 		if (err)
 			return err;
-
-		set_bit(vlan.fid, ps->fid_bitmap);
 	}
 
 	*entry = vlan;
@@ -1556,7 +1545,6 @@ int mv88e6xxx_port_vlan_del(struct dsa_switch *ds, int port, u16 vid)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 	struct mv88e6xxx_vtu_stu_entry vlan;
-	bool keep = false;
 	int i, err;
 
 	mutex_lock(&ps->smi_mutex);
@@ -1574,28 +1562,22 @@ int mv88e6xxx_port_vlan_del(struct dsa_switch *ds, int port, u16 vid)
 	vlan.data[port] = GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER;
 
 	/* keep the VLAN unless all ports are excluded */
+	vlan.valid = false;
 	for (i = 0; i < ps->num_ports; ++i) {
 		if (dsa_is_cpu_port(ds, i))
 			continue;
 
 		if (vlan.data[i] != GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER) {
-			keep = true;
+			vlan.valid = true;
 			break;
 		}
 	}
 
-	vlan.valid = keep;
 	err = _mv88e6xxx_vtu_loadpurge(ds, &vlan);
 	if (err)
 		goto unlock;
 
 	err = _mv88e6xxx_atu_remove(ds, vlan.fid, port, false);
-	if (err)
-		goto unlock;
-
-	if (!keep)
-		clear_bit(vlan.fid, ps->fid_bitmap);
-
 unlock:
 	mutex_unlock(&ps->smi_mutex);
 
@@ -1722,37 +1704,13 @@ static int _mv88e6xxx_atu_load(struct dsa_switch *ds,
 	return _mv88e6xxx_atu_cmd(ds, GLOBAL_ATU_OP_LOAD_DB);
 }
 
-static int _mv88e6xxx_port_vid_to_fid(struct dsa_switch *ds, int port, u16 vid)
-{
-	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
-	struct mv88e6xxx_vtu_stu_entry vlan;
-	int err;
-
-	if (vid == 0)
-		return ps->fid[port];
-
-	err = _mv88e6xxx_port_vtu_getnext(ds, port, vid - 1, &vlan);
-	if (err)
-		return err;
-
-	if (vlan.vid == vid)
-		return vlan.fid;
-
-	return -ENOENT;
-}
-
 static int _mv88e6xxx_port_fdb_load(struct dsa_switch *ds, int port,
 				    const unsigned char *addr, u16 vid,
 				    u8 state)
 {
 	struct mv88e6xxx_atu_entry entry = { 0 };
-	int ret;
 
-	ret = _mv88e6xxx_port_vid_to_fid(ds, port, vid);
-	if (ret < 0)
-		return ret;
-
-	entry.fid = ret;
+	entry.fid = vid; /* We use one FID per VLAN */
 	entry.state = state;
 	ether_addr_copy(entry.mac, addr);
 	if (state != GLOBAL_ATU_DATA_STATE_UNUSED) {
@@ -1767,6 +1725,10 @@ int mv88e6xxx_port_fdb_prepare(struct dsa_switch *ds, int port,
 			       const struct switchdev_obj_port_fdb *fdb,
 			       struct switchdev_trans *trans)
 {
+	/* We don't use per-port FDB */
+	if (fdb->vid == 0)
+		return -EOPNOTSUPP;
+
 	/* We don't need any dynamic resource from the kernel (yet),
 	 * so skip the prepare phase.
 	 */
@@ -1864,16 +1826,11 @@ int mv88e6xxx_port_fdb_getnext(struct dsa_switch *ds, int port,
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 	struct mv88e6xxx_atu_entry next;
-	u16 fid;
+	u16 fid = *vid; /* We use one FID per VLAN */
 	int ret;
 
 	mutex_lock(&ps->smi_mutex);
 
-	ret = _mv88e6xxx_port_vid_to_fid(ds, port, *vid);
-	if (ret < 0)
-		goto unlock;
-	fid = ret;
-
 	do {
 		if (is_broadcast_ether_addr(addr)) {
 			struct mv88e6xxx_vtu_stu_entry vtu;
@@ -1924,7 +1881,7 @@ static void mv88e6xxx_bridge_work(struct work_struct *work)
 static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
-	int ret, fid;
+	int ret;
 	u16 reg;
 
 	mutex_lock(&ps->smi_mutex);
@@ -2145,15 +2102,11 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
 	if (ret)
 		goto abort;
 
-	/* Port based VLAN map: give each port its own address
+	/* Port based VLAN map: do not give each port its own address
 	 * database, allow the CPU port to talk to each of the 'real'
 	 * ports, and allow each of the 'real' ports to only talk to
 	 * the upstream port.
 	 */
-	fid = port + 1;
-	ps->fid[port] = fid;
-	set_bit(fid, ps->fid_bitmap);
-
 	if (dsa_is_cpu_port(ds, port))
 		reg = BIT(ps->num_ports) - 1;
 	else
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 716f692..d5ee01a 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -407,11 +407,6 @@ struct mv88e6xxx_priv_state {
 	int		id; /* switch product id */
 	int		num_ports;	/* number of switch ports */
 
-	/* hw bridging */
-
-	DECLARE_BITMAP(fid_bitmap, VLAN_N_VID);	/* FIDs 1 to 4095 available */
-	u16 fid[DSA_MAX_PORTS];			/* per (non-bridged) port FID */
-
 	unsigned long port_state_update_mask;
 	u8 port_state[DSA_MAX_PORTS];
 
-- 
2.6.0

[PATCH net-next 3/4] net: dsa: do not warn unsupported bridge ops

[Vivien Didelot]

A DSA driver may not provide the port_join_bridge and port_leave_bridge
functions, so don't warn in such case.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/slave.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index bb2bd3b..43d7342 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1276,7 +1276,7 @@ int dsa_slave_netdevice_event(struct notifier_block *unused,
 			goto out;
 
 		err = dsa_slave_master_changed(dev);
-		if (err)
+		if (err && err != -EOPNOTSUPP)
 			netdev_warn(dev, "failed to reflect master change\n");
 
 		break;
-- 
2.6.0

[PATCH net-next 4/4] net: dsa: mv88e6xxx: fix hardware bridging

[Vivien Didelot]

Playing with the VLAN map of every port to implement "hardware bridging"
in the 88E6352 driver was a hack until full 802.1Q was supported.

Indeed with 802.1Q port mode "Disabled" or "Fallback", this feature is
used to restrict which output ports an input port can egress frames to.

A Linux bridge is an untagged VLAN. With full 802.1Q support, we don't
need this hack anymore and can use the "Secure" strict 802.1Q port mode.

With this mode, the port-based VLAN map still needs to be configured,
but all the logic is VTU-centric. This means that the switch only cares
about rules described in its hardware VLAN table, which is exactly what
Linux bridge expects and what we want.

Note also that the hardware bridging was broken with the previous
flexible "Fallback" 802.1Q port mode. Here's an example:

Port0 and Port1 belong to the same bridge. If Port0 sends crafted tagged
frames with VID 200 to Port1, Port1 receives it. Even if Port1 is in
hardware VLAN 200, but not Port0, Port1 will still receive it, because
Fallback mode doesn't care about invalid VID or non-member source port.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6171.c |  2 --
 drivers/net/dsa/mv88e6352.c |  2 --
 drivers/net/dsa/mv88e6xxx.c | 47 +++------------------------------------------
 drivers/net/dsa/mv88e6xxx.h |  2 --
 4 files changed, 3 insertions(+), 50 deletions(-)

diff --git a/drivers/net/dsa/mv88e6171.c b/drivers/net/dsa/mv88e6171.c
index ca3330a..dfca352 100644
--- a/drivers/net/dsa/mv88e6171.c
+++ b/drivers/net/dsa/mv88e6171.c
@@ -113,8 +113,6 @@ struct dsa_switch_driver mv88e6171_switch_driver = {
 #endif
 	.get_regs_len		= mv88e6xxx_get_regs_len,
 	.get_regs		= mv88e6xxx_get_regs,
-	.port_join_bridge       = mv88e6xxx_join_bridge,
-	.port_leave_bridge      = mv88e6xxx_leave_bridge,
 	.port_stp_update        = mv88e6xxx_port_stp_update,
 	.port_pvid_get		= mv88e6xxx_port_pvid_get,
 	.port_pvid_set		= mv88e6xxx_port_pvid_set,
diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index 078a358..796fdcb 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -340,8 +340,6 @@ struct dsa_switch_driver mv88e6352_switch_driver = {
 	.set_eeprom		= mv88e6352_set_eeprom,
 	.get_regs_len		= mv88e6xxx_get_regs_len,
 	.get_regs		= mv88e6xxx_get_regs,
-	.port_join_bridge	= mv88e6xxx_join_bridge,
-	.port_leave_bridge	= mv88e6xxx_leave_bridge,
 	.port_stp_update	= mv88e6xxx_port_stp_update,
 	.port_pvid_get		= mv88e6xxx_port_pvid_get,
 	.port_pvid_set		= mv88e6xxx_port_pvid_set,
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 787352df..63736f9 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1124,41 +1124,6 @@ static int _mv88e6xxx_port_vlan_map_set(struct dsa_switch *ds, int port,
 	return _mv88e6xxx_reg_write(ds, REG_PORT(port), PORT_BASE_VLAN, reg);
 }
 
-/* Bridge handling functions */
-
-static int mv88e6xxx_map_bridge(struct dsa_switch *ds, u16 members)
-{
-	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
-	const unsigned long output = members | BIT(dsa_upstream_port(ds));
-	int port, err = 0;
-
-	mutex_lock(&ps->smi_mutex);
-
-	for_each_set_bit(port, &output, ps->num_ports) {
-		if (dsa_is_cpu_port(ds, port))
-			continue;
-
-		err = _mv88e6xxx_port_vlan_map_set(ds, port, output & ~port);
-		if (err)
-			break;
-	}
-
-	mutex_unlock(&ps->smi_mutex);
-
-	return err;
-}
-
-
-int mv88e6xxx_join_bridge(struct dsa_switch *ds, int port, u32 br_port_mask)
-{
-	return mv88e6xxx_map_bridge(ds, br_port_mask);
-}
-
-int mv88e6xxx_leave_bridge(struct dsa_switch *ds, int port, u32 br_port_mask)
-{
-	return mv88e6xxx_map_bridge(ds, br_port_mask & ~port);
-}
-
 int mv88e6xxx_port_stp_update(struct dsa_switch *ds, int port, u8 state)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
@@ -2007,7 +1972,7 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
 			reg |= PORT_CONTROL_2_FORWARD_UNKNOWN;
 	}
 
-	reg |= PORT_CONTROL_2_8021Q_FALLBACK;
+	reg |= PORT_CONTROL_2_8021Q_SECURE;
 
 	if (reg) {
 		ret = _mv88e6xxx_reg_write(ds, REG_PORT(port),
@@ -2103,15 +2068,9 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
 		goto abort;
 
 	/* Port based VLAN map: do not give each port its own address
-	 * database, allow the CPU port to talk to each of the 'real'
-	 * ports, and allow each of the 'real' ports to only talk to
-	 * the upstream port.
+	 * database, and allow every port to egress frames on all other ports.
 	 */
-	if (dsa_is_cpu_port(ds, port))
-		reg = BIT(ps->num_ports) - 1;
-	else
-		reg = BIT(dsa_upstream_port(ds));
-
+	reg = BIT(ps->num_ports) - 1; /* all ports */
 	ret = _mv88e6xxx_port_vlan_map_set(ds, port, reg & ~port);
 	if (ret)
 		goto abort;
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index d5ee01a..ba94f17 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -463,8 +463,6 @@ int mv88e6xxx_phy_write_indirect(struct dsa_switch *ds, int addr, int regnum,
 int mv88e6xxx_get_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e);
 int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
 		      struct phy_device *phydev, struct ethtool_eee *e);
-int mv88e6xxx_join_bridge(struct dsa_switch *ds, int port, u32 br_port_mask);
-int mv88e6xxx_leave_bridge(struct dsa_switch *ds, int port, u32 br_port_mask);
 int mv88e6xxx_port_stp_update(struct dsa_switch *ds, int port, u8 state);
 int mv88e6xxx_port_pvid_get(struct dsa_switch *ds, int port, u16 *vid);
 int mv88e6xxx_port_pvid_set(struct dsa_switch *ds, int port, u16 vid);
-- 
2.6.0

Re: [PATCH net-next 0/4] net: dsa: mv88e6xxx: fix hardware bridging

[David Miller]

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Sun, 11 Oct 2015 18:08:34 -0400

> DSA and its drivers currently hook the NETDEV_CHANGEUPPER net_device event in
> order to configure the VLAN map of every port.
> 
> This VLAN map is a feature of these switch chips to hardcode and restrict which
> output ports a given input port can egress frames to.
> 
> A Linux bridge is a simple untagged VLAN propagated by the bridge code itself.
> With a proper 802.1Q support, a driver does not need this hook anymore, and
> will simply program the related VLAN object.
> 
> This patchset improves the hardware bridging code in the mv88e6xxx driver with
> a strict 802.1Q mode.
> 
> Ideally, the equivalent must be done for Broadcom Starfighter 2 and Rocker,
> before completely getting rid of this hook.

Series applied, thanks Vivien.

Re: [PATCH net-next 0/4] net: dsa: mv88e6xxx: fix hardware bridging

[Andrew Lunn]

On Sun, Oct 11, 2015 at 06:08:34PM -0400, Vivien Didelot wrote:
> DSA and its drivers currently hook the NETDEV_CHANGEUPPER net_device event in
> order to configure the VLAN map of every port.
> 
> This VLAN map is a feature of these switch chips to hardcode and restrict which
> output ports a given input port can egress frames to.
> 
> A Linux bridge is a simple untagged VLAN propagated by the bridge code itself.
> With a proper 802.1Q support, a driver does not need this hook anymore, and
> will simply program the related VLAN object.
> 
> This patchset improves the hardware bridging code in the mv88e6xxx driver with
> a strict 802.1Q mode.

Hi Vivien

I just tested this as part of net-next/master, and found a problem....

If i do:

ip link set lan0 up
ip addr add 192.168.10.2/24 dev lan0

It will not ping. Looking in sys/kernel/debug/dsa0/stats i see
broadcast packets, probably ARP, being received at the port.
But they are not being forwarded out the CPU port.

If however i do

brctl addbr br0
brctl addif br0 lan0
ip addr add 192.168.10.2/24 dev br0
ip link set br0 up

i can ping.

So it looks like we are too restrictive by default. You should be able
to use interfaces as they are, without a bridge.

   Andrew

Re: [PATCH net-next 0/4] net: dsa: mv88e6xxx: fix hardware bridging

[Vivien Didelot]

On Oct. Thursday 15 (42) 12:46 AM, Andrew Lunn wrote:
> On Sun, Oct 11, 2015 at 06:08:34PM -0400, Vivien Didelot wrote:
> > DSA and its drivers currently hook the NETDEV_CHANGEUPPER net_device event in
> > order to configure the VLAN map of every port.
> > 
> > This VLAN map is a feature of these switch chips to hardcode and restrict which
> > output ports a given input port can egress frames to.
> > 
> > A Linux bridge is a simple untagged VLAN propagated by the bridge code itself.
> > With a proper 802.1Q support, a driver does not need this hook anymore, and
> > will simply program the related VLAN object.
> > 
> > This patchset improves the hardware bridging code in the mv88e6xxx driver with
> > a strict 802.1Q mode.
> 
> Hi Vivien
> 
> I just tested this as part of net-next/master, and found a problem....
> 
> If i do:
> 
> ip link set lan0 up
> ip addr add 192.168.10.2/24 dev lan0
> 
> It will not ping. Looking in sys/kernel/debug/dsa0/stats i see
> broadcast packets, probably ARP, being received at the port.
> But they are not being forwarded out the CPU port.
> 
> If however i do
> 
> brctl addbr br0
> brctl addif br0 lan0
> ip addr add 192.168.10.2/24 dev br0
> ip link set br0 up
> 
> i can ping.
> 
> So it looks like we are too restrictive by default. You should be able
> to use interfaces as they are, without a bridge.

Correct, if the ports are not in a VLAN by default, they cannot talk.

If you want to, I think the special VLAN 0 can be used for that purpose.
IIRC, in a given configuration, Linux add the interfaces (thus programs
the hardware) with VLAN 0. I'm not sure when, maybe when the
.ndo_vlan_rx_add_vid is implemented, I need to give it a shot.

Otherwise, I can send you a patch configuring the VLAN 0 on switch
setup if this is the behavior we want.

Thanks,
-v

Re: [PATCH net-next 0/4] net: dsa: mv88e6xxx: fix hardware bridging

[Florian Fainelli]

On 14/10/15 18:28, Vivien Didelot wrote:
> On Oct. Thursday 15 (42) 12:46 AM, Andrew Lunn wrote:
>> On Sun, Oct 11, 2015 at 06:08:34PM -0400, Vivien Didelot wrote:
>>> DSA and its drivers currently hook the NETDEV_CHANGEUPPER net_device event in
>>> order to configure the VLAN map of every port.
>>>
>>> This VLAN map is a feature of these switch chips to hardcode and restrict which
>>> output ports a given input port can egress frames to.
>>>
>>> A Linux bridge is a simple untagged VLAN propagated by the bridge code itself.
>>> With a proper 802.1Q support, a driver does not need this hook anymore, and
>>> will simply program the related VLAN object.
>>>
>>> This patchset improves the hardware bridging code in the mv88e6xxx driver with
>>> a strict 802.1Q mode.
>>
>> Hi Vivien
>>
>> I just tested this as part of net-next/master, and found a problem....
>>
>> If i do:
>>
>> ip link set lan0 up
>> ip addr add 192.168.10.2/24 dev lan0
>>
>> It will not ping. Looking in sys/kernel/debug/dsa0/stats i see
>> broadcast packets, probably ARP, being received at the port.
>> But they are not being forwarded out the CPU port.
>>
>> If however i do
>>
>> brctl addbr br0
>> brctl addif br0 lan0
>> ip addr add 192.168.10.2/24 dev br0
>> ip link set br0 up
>>
>> i can ping.
>>
>> So it looks like we are too restrictive by default. You should be able
>> to use interfaces as they are, without a bridge.
> 
> Correct, if the ports are not in a VLAN by default, they cannot talk.

The expectation for DSA devices, if no bridge device is configured is to
have each port be able to talk to the CPU port only, but this has to
work out of the box.

> 
> If you want to, I think the special VLAN 0 can be used for that purpose.
> IIRC, in a given configuration, Linux add the interfaces (thus programs
> the hardware) with VLAN 0. I'm not sure when, maybe when the
> .ndo_vlan_rx_add_vid is implemented, I need to give it a shot.

But if you do that, won't that put all DSA ports into VLAN 0? Would not
that break isolation between each ports as expected for a DSA switch?

> 
> Otherwise, I can send you a patch configuring the VLAN 0 on switch
> setup if this is the behavior we want.
> 
> Thanks,
> -v
> 


-- 
Florian

Re: [PATCH net-next 0/4] net: dsa: mv88e6xxx: fix hardware bridging

[Vivien Didelot]

On Oct. Wednesday 14 (42) 06:44 PM, Florian Fainelli wrote:
> On 14/10/15 18:28, Vivien Didelot wrote:
> > On Oct. Thursday 15 (42) 12:46 AM, Andrew Lunn wrote:
> >> On Sun, Oct 11, 2015 at 06:08:34PM -0400, Vivien Didelot wrote:
> >>> DSA and its drivers currently hook the NETDEV_CHANGEUPPER net_device event in
> >>> order to configure the VLAN map of every port.
> >>>
> >>> This VLAN map is a feature of these switch chips to hardcode and restrict which
> >>> output ports a given input port can egress frames to.
> >>>
> >>> A Linux bridge is a simple untagged VLAN propagated by the bridge code itself.
> >>> With a proper 802.1Q support, a driver does not need this hook anymore, and
> >>> will simply program the related VLAN object.
> >>>
> >>> This patchset improves the hardware bridging code in the mv88e6xxx driver with
> >>> a strict 802.1Q mode.
> >>
> >> Hi Vivien
> >>
> >> I just tested this as part of net-next/master, and found a problem....
> >>
> >> If i do:
> >>
> >> ip link set lan0 up
> >> ip addr add 192.168.10.2/24 dev lan0
> >>
> >> It will not ping. Looking in sys/kernel/debug/dsa0/stats i see
> >> broadcast packets, probably ARP, being received at the port.
> >> But they are not being forwarded out the CPU port.
> >>
> >> If however i do
> >>
> >> brctl addbr br0
> >> brctl addif br0 lan0
> >> ip addr add 192.168.10.2/24 dev br0
> >> ip link set br0 up
> >>
> >> i can ping.
> >>
> >> So it looks like we are too restrictive by default. You should be able
> >> to use interfaces as they are, without a bridge.
> > 
> > Correct, if the ports are not in a VLAN by default, they cannot talk.
> 
> The expectation for DSA devices, if no bridge device is configured is to
> have each port be able to talk to the CPU port only, but this has to
> work out of the box.

OK, I might have forgotten this requirement. I also just noticed that
you mentioned it in Documentation/networking/dsa/dsa.txt. Thanks for the
reminder.

> > 
> > If you want to, I think the special VLAN 0 can be used for that
> > purpose. IIRC, in a given configuration, Linux add the interfaces
> > (thus programs the hardware) with VLAN 0. I'm not sure when, maybe
> > when the .ndo_vlan_rx_add_vid is implemented, I need to give it a
> > shot.
> 
> But if you do that, won't that put all DSA ports into VLAN 0? Would
> not that break isolation between each ports as expected for a DSA
> switch?

You're correct, then VLAN 0 is not an option. I have something else in
mind, fix coming soon.

> > 
> > Otherwise, I can send you a patch configuring the VLAN 0 on switch
> > setup if this is the behavior we want.

Thanks,
-v

Re: [PATCH net-next 0/4] net: dsa: mv88e6xxx: fix hardware bridging

[Andrew Lunn]

On Wed, Oct 14, 2015 at 09:28:55PM -0400, Vivien Didelot wrote:
> On Oct. Thursday 15 (42) 12:46 AM, Andrew Lunn wrote:
> > On Sun, Oct 11, 2015 at 06:08:34PM -0400, Vivien Didelot wrote:
> > > DSA and its drivers currently hook the NETDEV_CHANGEUPPER net_device event in
> > > order to configure the VLAN map of every port.
> > > 
> > > This VLAN map is a feature of these switch chips to hardcode and restrict which
> > > output ports a given input port can egress frames to.
> > > 
> > > A Linux bridge is a simple untagged VLAN propagated by the bridge code itself.
> > > With a proper 802.1Q support, a driver does not need this hook anymore, and
> > > will simply program the related VLAN object.
> > > 
> > > This patchset improves the hardware bridging code in the mv88e6xxx driver with
> > > a strict 802.1Q mode.
> > 
> > Hi Vivien
> > 
> > I just tested this as part of net-next/master, and found a problem....
> > 
> > If i do:
> > 
> > ip link set lan0 up
> > ip addr add 192.168.10.2/24 dev lan0
> > 
> > It will not ping. Looking in sys/kernel/debug/dsa0/stats i see
> > broadcast packets, probably ARP, being received at the port.
> > But they are not being forwarded out the CPU port.
> > 
> > If however i do
> > 
> > brctl addbr br0
> > brctl addif br0 lan0
> > ip addr add 192.168.10.2/24 dev br0
> > ip link set br0 up
> > 
> > i can ping.
> > 
> > So it looks like we are too restrictive by default. You should be able
> > to use interfaces as they are, without a bridge.
> 
> Correct, if the ports are not in a VLAN by default, they cannot talk.

Hi Vivien 

This is a regression. Ports of the switch should work like normal
Linux interfaces. And up until now, they did. This patchset changed
that.

As Florian pointed out, these interfaces are separated from each
other. So you need something like a bridge per port by default, which
then gets removed and replaced when a port is added to a Linux bridge.

We also need to take care of VLANs. When the port is not a member of a
linux bridge, i expect all VLAN tagged frames to be received, as well
as untagged frames. This is normal Linux behaviour. But i never got
around to testing this with DSA.

       Andrew

Re: [PATCH net-next 0/4] net: dsa: mv88e6xxx: fix hardware bridging

[Guenter Roeck]

On 10/14/2015 07:52 PM, Andrew Lunn wrote:
> On Wed, Oct 14, 2015 at 09:28:55PM -0400, Vivien Didelot wrote:
>> On Oct. Thursday 15 (42) 12:46 AM, Andrew Lunn wrote:
>>> On Sun, Oct 11, 2015 at 06:08:34PM -0400, Vivien Didelot wrote:
>>>> DSA and its drivers currently hook the NETDEV_CHANGEUPPER net_device event in
>>>> order to configure the VLAN map of every port.
>>>>
>>>> This VLAN map is a feature of these switch chips to hardcode and restrict which
>>>> output ports a given input port can egress frames to.
>>>>
>>>> A Linux bridge is a simple untagged VLAN propagated by the bridge code itself.
>>>> With a proper 802.1Q support, a driver does not need this hook anymore, and
>>>> will simply program the related VLAN object.
>>>>
>>>> This patchset improves the hardware bridging code in the mv88e6xxx driver with
>>>> a strict 802.1Q mode.
>>>
>>> Hi Vivien
>>>
>>> I just tested this as part of net-next/master, and found a problem....
>>>
>>> If i do:
>>>
>>> ip link set lan0 up
>>> ip addr add 192.168.10.2/24 dev lan0
>>>
>>> It will not ping. Looking in sys/kernel/debug/dsa0/stats i see
>>> broadcast packets, probably ARP, being received at the port.
>>> But they are not being forwarded out the CPU port.
>>>
>>> If however i do
>>>
>>> brctl addbr br0
>>> brctl addif br0 lan0
>>> ip addr add 192.168.10.2/24 dev br0
>>> ip link set br0 up
>>>
>>> i can ping.
>>>
>>> So it looks like we are too restrictive by default. You should be able
>>> to use interfaces as they are, without a bridge.
>>
>> Correct, if the ports are not in a VLAN by default, they cannot talk.
>
> Hi Vivien
>
> This is a regression. Ports of the switch should work like normal
> Linux interfaces. And up until now, they did. This patchset changed
> that.
>
> As Florian pointed out, these interfaces are separated from each
> other. So you need something like a bridge per port by default, which
> then gets removed and replaced when a port is added to a Linux bridge.
>
> We also need to take care of VLANs. When the port is not a member of a
> linux bridge, i expect all VLAN tagged frames to be received, as well
> as untagged frames. This is normal Linux behaviour. But i never got
> around to testing this with DSA.
>

There was a reason for the original code. I had wondered how it is now
supposed to work. Guess this exchange explains it. Looking forward to see
how it is going to be fixed, and too bad I don't have time to be more
involved.

Guenter

Re: [PATCH net-next 0/4] net: dsa: mv88e6xxx: fix hardware bridging

[Vivien Didelot]

On Oct. Thursday 15 (42) 04:52 AM, Andrew Lunn wrote:
> On Wed, Oct 14, 2015 at 09:28:55PM -0400, Vivien Didelot wrote:
> > On Oct. Thursday 15 (42) 12:46 AM, Andrew Lunn wrote:
> > > On Sun, Oct 11, 2015 at 06:08:34PM -0400, Vivien Didelot wrote:
> > > > DSA and its drivers currently hook the NETDEV_CHANGEUPPER net_device event in
> > > > order to configure the VLAN map of every port.
> > > > 
> > > > This VLAN map is a feature of these switch chips to hardcode and restrict which
> > > > output ports a given input port can egress frames to.
> > > > 
> > > > A Linux bridge is a simple untagged VLAN propagated by the bridge code itself.
> > > > With a proper 802.1Q support, a driver does not need this hook anymore, and
> > > > will simply program the related VLAN object.
> > > > 
> > > > This patchset improves the hardware bridging code in the mv88e6xxx driver with
> > > > a strict 802.1Q mode.
> > > 
> > > Hi Vivien
> > > 
> > > I just tested this as part of net-next/master, and found a problem....
> > > 
> > > If i do:
> > > 
> > > ip link set lan0 up
> > > ip addr add 192.168.10.2/24 dev lan0
> > > 
> > > It will not ping. Looking in sys/kernel/debug/dsa0/stats i see
> > > broadcast packets, probably ARP, being received at the port.
> > > But they are not being forwarded out the CPU port.
> > > 
> > > If however i do
> > > 
> > > brctl addbr br0
> > > brctl addif br0 lan0
> > > ip addr add 192.168.10.2/24 dev br0
> > > ip link set br0 up
> > > 
> > > i can ping.
> > > 
> > > So it looks like we are too restrictive by default. You should be able
> > > to use interfaces as they are, without a bridge.
> > 
> > Correct, if the ports are not in a VLAN by default, they cannot talk.
> 
> Hi Vivien 
> 
> This is a regression. Ports of the switch should work like normal
> Linux interfaces. And up until now, they did. This patchset changed
> that.
> 
> As Florian pointed out, these interfaces are separated from each
> other. So you need something like a bridge per port by default, which
> then gets removed and replaced when a port is added to a Linux bridge.

I'll fix this regression and try the exact same example you provided.

> We also need to take care of VLANs. When the port is not a member of a
> linux bridge, i expect all VLAN tagged frames to be received, as well
> as untagged frames. This is normal Linux behaviour. But i never got
> around to testing this with DSA.

Thanks for testing,
-v