[PATCH v2 net-next 0/6] mv88e6xxx broadcast flooding in hardware

[PATCH v2 net-next 0/6] mv88e6xxx broadcast flooding in hardware

[Andrew Lunn]

This patchset makes the mv88e6xxx driver perform flooding in hardware,
rather than let the software bridge perform the flooding. This is a
prerequisite for IGMP snooping on the bridge interface.

In order to make hardware broadcasting work, a few other issues need
fixing or improving. SWITCHDEV_ATTR_ID_PORT_PARENT_ID is broken, which
is apparent when testing on the ZII devel board with multiple
switches.

Some of these patches are taken from a previous RFC patchset of IGMP
support.

Andrew Lunn (6):
  net: dsa: Fix SWITCHDEV_ATTR_ID_PORT_PARENT_ID
  net: dsa: {e}dsa: set offload_fwd_mark on received packets
  net: dsa: mv88e6xxx: Fixed port netdev check for VLANs
  net: dsa: mv88e6xxx: Print offending port when vlan check fails
  net: dsa: mv88e6xxx: Move mv88e6xxx_port_db_load_purge()
  net: dsa: mv88e6xxx: Flood broadcast frames in hardware

 drivers/net/dsa/mv88e6xxx/chip.c | 127 ++++++++++++++++++++++++---------------
 net/dsa/slave.c                  |   5 +-
 net/dsa/tag_dsa.c                |   2 +
 net/dsa/tag_edsa.c               |   2 +
 4 files changed, 86 insertions(+), 50 deletions(-)

-- 
2.15.0

[PATCH v2 net-next 1/6] net: dsa: Fix SWITCHDEV_ATTR_ID_PORT_PARENT_ID

[Andrew Lunn]

SWITCHDEV_ATTR_ID_PORT_PARENT_ID is used by the software bridge when
determining which ports to flood a packet out. If the packet
originated from a switch, it assumes the switch has already flooded
the packet out the switches ports, so the bridge should not flood the
packet itself out switch ports. Ports on the same switch are expected
to return the same parent ID when SWITCHDEV_ATTR_ID_PORT_PARENT_ID is
called.

DSA gets this wrong with clusters of switches. As far as the software
bridge is concerned, the cluster is all one switch. A packet from any
switch in the cluster can be assumed to have been flooded as needed
out of all ports of the cluster, not just the switch it originated
from. Hence all ports of a cluster should return the same parent. The
old implementation did not, each switch in the cluster had its own ID.

Also wrong was that the ID was not unique if multiple DSA instances
are in operation.

Use the tree ID as the parent ID, which is the same for all switches
in a cluster and unique across switch clusters.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
v2: Swap from MAC address to dst->tree
v3: Move variables to eliminate block in case.
    Fix present perfect passive infinitive have and add missing of
---
 net/dsa/slave.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 9b75d0ac4092..1179e4cd6701 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -342,11 +342,12 @@ static int dsa_slave_port_attr_get(struct net_device *dev,
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	struct dsa_switch *ds = dp->ds;
+	struct dsa_switch_tree *dst = ds->dst;
 
 	switch (attr->id) {
 	case SWITCHDEV_ATTR_ID_PORT_PARENT_ID:
-		attr->u.ppid.id_len = sizeof(ds->index);
-		memcpy(&attr->u.ppid.id, &ds->index, attr->u.ppid.id_len);
+		attr->u.ppid.id_len = sizeof(dst->tree);
+		memcpy(&attr->u.ppid.id, &dst->tree, attr->u.ppid.id_len);
 		break;
 	case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT:
 		attr->u.brport_flags_support = 0;
-- 
2.15.0

Re: [PATCH v2 net-next 1/6] net: dsa: Fix SWITCHDEV_ATTR_ID_PORT_PARENT_ID

[Florian Fainelli]

On 11/06/2017 03:02 PM, Andrew Lunn wrote:
> SWITCHDEV_ATTR_ID_PORT_PARENT_ID is used by the software bridge when
> determining which ports to flood a packet out. If the packet
> originated from a switch, it assumes the switch has already flooded
> the packet out the switches ports, so the bridge should not flood the
> packet itself out switch ports. Ports on the same switch are expected
> to return the same parent ID when SWITCHDEV_ATTR_ID_PORT_PARENT_ID is
> called.
> 
> DSA gets this wrong with clusters of switches. As far as the software
> bridge is concerned, the cluster is all one switch. A packet from any
> switch in the cluster can be assumed to have been flooded as needed
> out of all ports of the cluster, not just the switch it originated
> from. Hence all ports of a cluster should return the same parent. The
> old implementation did not, each switch in the cluster had its own ID.
> 
> Also wrong was that the ID was not unique if multiple DSA instances
> are in operation.
> 
> Use the tree ID as the parent ID, which is the same for all switches
> in a cluster and unique across switch clusters.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

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

[PATCH v2 net-next 2/6] net: dsa: {e}dsa: set offload_fwd_mark on received packets

[Andrew Lunn]

The software bridge needs to know if a packet has already been bridged
by hardware offload to ports in the same hardware offload, in order
that it does not re-flood them, causing duplicates. This is
particularly true for broadcast and multicast traffic which the host
has requested.

By setting offload_fwd_mark in the skb the bridge will only flood to
ports in other offloads and other netifs. Set this flag in the DSA and
EDSA tag driver.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---

v2
--
For the moment, do this in the tag drivers, not the generic code.
Once we get more test results from other switches, maybe move it back
again.
---
 net/dsa/tag_dsa.c  | 2 ++
 net/dsa/tag_edsa.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index dbbcdafed8c3..cd13cfc542ce 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -141,6 +141,8 @@ static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev,
 			2 * ETH_ALEN);
 	}
 
+	skb->offload_fwd_mark = 1;
+
 	return skb;
 }
 
diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
index f38a626b3a05..4083326b806e 100644
--- a/net/dsa/tag_edsa.c
+++ b/net/dsa/tag_edsa.c
@@ -160,6 +160,8 @@ static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev,
 			2 * ETH_ALEN);
 	}
 
+	skb->offload_fwd_mark = 1;
+
 	return skb;
 }
 
-- 
2.15.0

[PATCH v2 net-next 3/6] net: dsa: mv88e6xxx: Fixed port netdev check for VLANs

[Andrew Lunn]

Having the same VLAN on multiple bridges is currently unsupported as
an offload. mv88e6xxx_port_check_hw_vlan() is used to ensure that a
VLAN is not on multiple bridges when adding a VLAN range to a port. It
loops the ports and checks to see if there are ports in a different
bridge with the same VLAN.

While walking all switch ports, the code was checking if the new port
has a netdev slave attached to it. If not, skip checking the port
being walked. This seems like a typ0. If the new port does not have a
slave, how has a VLAN been added to it in the first place, requiring
this check be performed at all? More likely, we should be checking if
the port being walked has a slave. Without the port having a slave, it
cannot have a VLAN on it, so there is no need to check further for
that particular port.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 09a66d4d9492..420621c759e4 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1137,7 +1137,7 @@ static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
 			if (dsa_is_dsa_port(ds, i) || dsa_is_cpu_port(ds, i))
 				continue;
 
-			if (!ds->ports[port].slave)
+			if (!ds->ports[i].slave)
 				continue;
 
 			if (vlan.member[i] ==
-- 
2.15.0

[PATCH v2 net-next 4/6] net: dsa: mv88e6xxx: Print offending port when vlan check fails

[Andrew Lunn]

When testing if a VLAN is one more than one bridge, we print an error
message that the VLAN is already in use somewhere else. Print both the
new port which would like the VLAN, and the port which already has it,
to aid debugging.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 420621c759e4..0a4a740ebea6 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1151,8 +1151,8 @@ static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
 			if (!dsa_to_port(ds, i)->bridge_dev)
 				continue;
 
-			dev_err(ds->dev, "p%d: hw VLAN %d already used by %s\n",
-				port, vlan.vid,
+			dev_err(ds->dev, "p%d: hw VLAN %d already used by port %d in %s\n",
+				port, vlan.vid, i,
 				netdev_name(dsa_to_port(ds, i)->bridge_dev));
 			err = -EOPNOTSUPP;
 			goto unlock;
-- 
2.15.0

[PATCH v2 net-next 5/6] net: dsa: mv88e6xxx: Move mv88e6xxx_port_db_load_purge()

[Andrew Lunn]

This function is going to be needed by a soon to be added new
function. Move it earlier so we can avoid a forward declaration.
No functional changes.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 88 ++++++++++++++++++++--------------------
 1 file changed, 44 insertions(+), 44 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 0a4a740ebea6..263b714a5ae3 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1208,6 +1208,50 @@ mv88e6xxx_port_vlan_prepare(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
+					const unsigned char *addr, u16 vid,
+					u8 state)
+{
+	struct mv88e6xxx_vtu_entry vlan;
+	struct mv88e6xxx_atu_entry entry;
+	int err;
+
+	/* Null VLAN ID corresponds to the port private database */
+	if (vid == 0)
+		err = mv88e6xxx_port_get_fid(chip, port, &vlan.fid);
+	else
+		err = mv88e6xxx_vtu_get(chip, vid, &vlan, false);
+	if (err)
+		return err;
+
+	entry.state = MV88E6XXX_G1_ATU_DATA_STATE_UNUSED;
+	ether_addr_copy(entry.mac, addr);
+	eth_addr_dec(entry.mac);
+
+	err = mv88e6xxx_g1_atu_getnext(chip, vlan.fid, &entry);
+	if (err)
+		return err;
+
+	/* Initialize a fresh ATU entry if it isn't found */
+	if (entry.state == MV88E6XXX_G1_ATU_DATA_STATE_UNUSED ||
+	    !ether_addr_equal(entry.mac, addr)) {
+		memset(&entry, 0, sizeof(entry));
+		ether_addr_copy(entry.mac, addr);
+	}
+
+	/* Purge the ATU entry only if no port is using it anymore */
+	if (state == MV88E6XXX_G1_ATU_DATA_STATE_UNUSED) {
+		entry.portvec &= ~BIT(port);
+		if (!entry.portvec)
+			entry.state = MV88E6XXX_G1_ATU_DATA_STATE_UNUSED;
+	} else {
+		entry.portvec |= BIT(port);
+		entry.state = state;
+	}
+
+	return mv88e6xxx_g1_atu_loadpurge(chip, vlan.fid, &entry);
+}
+
 static int _mv88e6xxx_port_vlan_add(struct mv88e6xxx_chip *chip, int port,
 				    u16 vid, u8 member)
 {
@@ -1324,50 +1368,6 @@ static int mv88e6xxx_port_vlan_del(struct dsa_switch *ds, int port,
 	return err;
 }
 
-static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
-					const unsigned char *addr, u16 vid,
-					u8 state)
-{
-	struct mv88e6xxx_vtu_entry vlan;
-	struct mv88e6xxx_atu_entry entry;
-	int err;
-
-	/* Null VLAN ID corresponds to the port private database */
-	if (vid == 0)
-		err = mv88e6xxx_port_get_fid(chip, port, &vlan.fid);
-	else
-		err = mv88e6xxx_vtu_get(chip, vid, &vlan, false);
-	if (err)
-		return err;
-
-	entry.state = MV88E6XXX_G1_ATU_DATA_STATE_UNUSED;
-	ether_addr_copy(entry.mac, addr);
-	eth_addr_dec(entry.mac);
-
-	err = mv88e6xxx_g1_atu_getnext(chip, vlan.fid, &entry);
-	if (err)
-		return err;
-
-	/* Initialize a fresh ATU entry if it isn't found */
-	if (entry.state == MV88E6XXX_G1_ATU_DATA_STATE_UNUSED ||
-	    !ether_addr_equal(entry.mac, addr)) {
-		memset(&entry, 0, sizeof(entry));
-		ether_addr_copy(entry.mac, addr);
-	}
-
-	/* Purge the ATU entry only if no port is using it anymore */
-	if (state == MV88E6XXX_G1_ATU_DATA_STATE_UNUSED) {
-		entry.portvec &= ~BIT(port);
-		if (!entry.portvec)
-			entry.state = MV88E6XXX_G1_ATU_DATA_STATE_UNUSED;
-	} else {
-		entry.portvec |= BIT(port);
-		entry.state = state;
-	}
-
-	return mv88e6xxx_g1_atu_loadpurge(chip, vlan.fid, &entry);
-}
-
 static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
 				  const unsigned char *addr, u16 vid)
 {
-- 
2.15.0

[PATCH v2 net-next 6/6] net: dsa: mv88e6xxx: Flood broadcast frames in hardware

[Andrew Lunn]

By default, the switch does not flood broadcast frames. Instead the
broadcast address is unknown in the ATU, so the frame gets forwarded
out the cpu port. The software bridge then floods it back to the
individual switch ports which are members of the bridge.

Add an ATU entry in the switch so that it floods broadcast frames out
ports, rather than have the software bridge do it. Also, send a copy
out the cpu port and any dsa ports. Rely on the port vectors to
prevent broadcast frames leaking between bridges, and separated ports.

Additionally, when a VLAN is added, a new FID is allocated.  This
represents a new table of ATU entries. A broadcast entry is added to
the new FID.

With offload_fwd_mark being set, the software bridge will not flood
the frames it receives back to the switch.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
v2: ++port -> port++
    use temporary variable to avoid long line
---
 drivers/net/dsa/mv88e6xxx/chip.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 263b714a5ae3..6dd5fdfeafcf 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1252,6 +1252,29 @@ static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
 	return mv88e6xxx_g1_atu_loadpurge(chip, vlan.fid, &entry);
 }
 
+static int mv88e6xxx_port_add_broadcast(struct mv88e6xxx_chip *chip, int port,
+					u16 vid)
+{
+	const char broadcast[6] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
+	u8 state = MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC;
+
+	return mv88e6xxx_port_db_load_purge(chip, port, broadcast, vid, state);
+}
+
+static int mv88e6xxx_broadcast_setup(struct mv88e6xxx_chip *chip, u16 vid)
+{
+	int port;
+	int err;
+
+	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
+		err = mv88e6xxx_port_add_broadcast(chip, port, vid);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 static int _mv88e6xxx_port_vlan_add(struct mv88e6xxx_chip *chip, int port,
 				    u16 vid, u8 member)
 {
@@ -1264,7 +1287,11 @@ static int _mv88e6xxx_port_vlan_add(struct mv88e6xxx_chip *chip, int port,
 
 	vlan.member[port] = member;
 
-	return mv88e6xxx_vtu_loadpurge(chip, &vlan);
+	err = mv88e6xxx_vtu_loadpurge(chip, &vlan);
+	if (err)
+		return err;
+
+	return mv88e6xxx_broadcast_setup(chip, vid);
 }
 
 static void mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port,
@@ -2049,6 +2076,10 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 	if (err)
 		goto unlock;
 
+	err = mv88e6xxx_broadcast_setup(chip, 0);
+	if (err)
+		goto unlock;
+
 	err = mv88e6xxx_pot_setup(chip);
 	if (err)
 		goto unlock;
-- 
2.15.0

Re: [PATCH v2 net-next 0/6] mv88e6xxx broadcast flooding in hardware

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Tue,  7 Nov 2017 00:02:25 +0100

> This patchset makes the mv88e6xxx driver perform flooding in hardware,
> rather than let the software bridge perform the flooding. This is a
> prerequisite for IGMP snooping on the bridge interface.
> 
> In order to make hardware broadcasting work, a few other issues need
> fixing or improving. SWITCHDEV_ATTR_ID_PORT_PARENT_ID is broken, which
> is apparent when testing on the ZII devel board with multiple
> switches.
> 
> Some of these patches are taken from a previous RFC patchset of IGMP
> support.

Series applied, thanks Andrew.

Re: [PATCH v2 net-next 0/6] mv88e6xxx broadcast flooding in hardware

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 08 Nov 2017 13:58:09 +0900 (KST)

> From: Andrew Lunn <andrew@lunn.ch>
> Date: Tue,  7 Nov 2017 00:02:25 +0100
> 
>> This patchset makes the mv88e6xxx driver perform flooding in hardware,
>> rather than let the software bridge perform the flooding. This is a
>> prerequisite for IGMP snooping on the bridge interface.
>> 
>> In order to make hardware broadcasting work, a few other issues need
>> fixing or improving. SWITCHDEV_ATTR_ID_PORT_PARENT_ID is broken, which
>> is apparent when testing on the ZII devel board with multiple
>> switches.
>> 
>> Some of these patches are taken from a previous RFC patchset of IGMP
>> support.
> 
> Series applied, thanks Andrew.

Hmmm, I think there might be some in-flight conflict with Vivien's recent
DSA refactoring.

net/dsa/slave.c: In function ‘dsa_slave_port_attr_get’:
net/dsa/slave.c:349:35: error: ‘struct dsa_switch_tree’ has no member named ‘tree’
   attr->u.ppid.id_len = sizeof(dst->tree);
                                   ^~
net/dsa/slave.c:350:32: error: ‘struct dsa_switch_tree’ has no member named ‘tree’
   memcpy(&attr->u.ppid.id, &dst->tree, attr->u.ppid.id_len);
                                ^~

Anyways, I had to revert, please fix this up and resubmit.

Thank you!

Re: [PATCH v2 net-next 0/6] mv88e6xxx broadcast flooding in hardware

[Vivien Didelot]

Hi,

David Miller <davem@davemloft.net> writes:

> net/dsa/slave.c: In function ‘dsa_slave_port_attr_get’:
> net/dsa/slave.c:349:35: error: ‘struct dsa_switch_tree’ has no member named ‘tree’
>    attr->u.ppid.id_len = sizeof(dst->tree);
>                                    ^~
> net/dsa/slave.c:350:32: error: ‘struct dsa_switch_tree’ has no member named ‘tree’
>    memcpy(&attr->u.ppid.id, &dst->tree, attr->u.ppid.id_len);
>                                 ^~

The 'u32 tree' of dsa_switch_tree is now an 'unsigned int index',
as for dsa_switch and dsa_port structures.


Thanks,

        Vivien