[PATCH net-next 0/3] net: dsa: mv88e6xxx: fix IPv6

[PATCH net-next 0/3] net: dsa: mv88e6xxx: fix IPv6

[Russell King - ARM Linux admin]

We'have had some emails in private over this issue, this is my current
patch set rebased on top of net-next which provides working IPv6 (and
probably other protocols as well) over mv88e6xxx DSA switches.

The problem comes down to mv88e6xxx defaulting to not flood unknown
unicast and multicast datagrams, as they would be by dumb switches,
and as the Linux bridge code does by default.

These flood settings can be disabled via the Linux bridge code if it's
desired to make the switch behave more like a managed switch, eg, by
enabling the multicast querier.  However, the multicast querier
defaults to being disabled which effectively means that by default,
mv88e6xxx switches block all multicast traffic.  This is at odds with
the Linux bridge documentation, and the defaults that the Linux bridge
code adopts.

So, this patch set adds DSA support for Linux bridge flags, adds
mv88e6xxx support for the unicast and multicast flooding flags, and
lastly enables flooding of these frames by default to match the
Linux bridge defaults.

 drivers/net/dsa/mv88e6xxx/chip.c | 42 ++++++++++++++++++++++++++++++++++++----
 include/net/dsa.h                |  3 +++
 net/dsa/dsa_priv.h               |  2 ++
 net/dsa/port.c                   | 15 ++++++++++++++
 net/dsa/slave.c                  |  6 ++++++
 5 files changed, 64 insertions(+), 4 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[PATCH net-next 1/3] net: dsa: add support for bridge flags

[Russell King]

The Linux bridge implementation allows various properties of the bridge
to be controlled, such as flooding unknown unicast and multicast frames.
This patch adds the necessary DSA infrastructure to allow the Linux
bridge support to control these properties for DSA switches.

We implement this by providing two new methods: one to get the switch-
wide support bitmask, and another to set the properties.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 include/net/dsa.h  |  3 +++
 net/dsa/dsa_priv.h |  2 ++
 net/dsa/port.c     | 15 +++++++++++++++
 net/dsa/slave.c    |  6 ++++++
 4 files changed, 26 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 7f2a668ef2cc..6cc1222aa2ca 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -400,6 +400,9 @@ struct dsa_switch_ops {
 	void	(*port_stp_state_set)(struct dsa_switch *ds, int port,
 				      u8 state);
 	void	(*port_fast_age)(struct dsa_switch *ds, int port);
+	int	(*port_bridge_flags)(struct dsa_switch *ds, int port,
+				     unsigned long);
+	unsigned long (*bridge_flags_support)(struct dsa_switch *ds);
 
 	/*
 	 * VLAN support
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 1f4972dab9f2..f4f99ec29f5d 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -160,6 +160,8 @@ int dsa_port_mdb_add(const struct dsa_port *dp,
 		     struct switchdev_trans *trans);
 int dsa_port_mdb_del(const struct dsa_port *dp,
 		     const struct switchdev_obj_port_mdb *mdb);
+int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags,
+			  struct switchdev_trans *trans);
 int dsa_port_vlan_add(struct dsa_port *dp,
 		      const struct switchdev_obj_port_vlan *vlan,
 		      struct switchdev_trans *trans);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 2d7e01b23572..eb745041b1d9 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -177,6 +177,21 @@ int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock,
 	return dsa_port_notify(dp, DSA_NOTIFIER_AGEING_TIME, &info);
 }
 
+int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags,
+			  struct switchdev_trans *trans)
+{
+	struct dsa_switch *ds = dp->ds;
+	int port = dp->index;
+
+	if (switchdev_trans_ph_prepare(trans))
+		return ds->ops->port_bridge_flags ? 0 : -EOPNOTSUPP;
+
+	if (ds->ops->port_bridge_flags)
+		ds->ops->port_bridge_flags(ds, port, flags);
+
+	return 0;
+}
+
 int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 		     u16 vid)
 {
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 2e5e7c04821b..af53cdd14a29 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -295,6 +295,9 @@ 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_PORT_BRIDGE_FLAGS:
+		ret = dsa_port_bridge_flags(dp, attr->u.brport_flags, trans);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
@@ -384,6 +387,9 @@ static int dsa_slave_port_attr_get(struct net_device *dev,
 	switch (attr->id) {
 	case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT:
 		attr->u.brport_flags_support = 0;
+		if (ds->ops->bridge_flags_support)
+			attr->u.brport_flags_support |=
+				ds->ops->bridge_flags_support(ds);
 		break;
 	default:
 		return -EOPNOTSUPP;
-- 
2.7.4

[PATCH net-next 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Russell King]

Add support for the bridge flags to Marvell 88e6xxx bridges, allowing
the multicast and unicast flood properties to be controlled.  These
can be controlled on a per-port basis via commands such as:

	bridge link set dev lan1 flood on|off
	bridge link set dev lan1 mcast_flood on|off

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 32e7af5caa69..b75a865a293d 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4692,6 +4692,37 @@ static int mv88e6xxx_port_mdb_del(struct dsa_switch *ds, int port,
 	return err;
 }
 
+static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
+				       unsigned long flags)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	bool unicast, multicast;
+	int ret = -EOPNOTSUPP;
+
+	unicast = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) ||
+		flags & BR_FLOOD;
+	multicast = flags & BR_MCAST_FLOOD;
+
+	mutex_lock(&chip->reg_lock);
+	if (chip->info->ops->port_set_egress_floods)
+		ret = chip->info->ops->port_set_egress_floods(chip, port,
+							      unicast,
+							      multicast);
+	mutex_unlock(&chip->reg_lock);
+
+	return ret;
+}
+
+static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
+{
+	unsigned long support = 0;
+
+	if (chip->info->ops->port_set_egress_floods)
+		support |= BR_FLOOD | BR_MCAST_FLOOD;
+
+	return support;
+}
+
 static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 #if IS_ENABLED(CONFIG_NET_DSA_LEGACY)
 	.probe			= mv88e6xxx_drv_probe,
@@ -4719,6 +4750,8 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.set_ageing_time	= mv88e6xxx_set_ageing_time,
 	.port_bridge_join	= mv88e6xxx_port_bridge_join,
 	.port_bridge_leave	= mv88e6xxx_port_bridge_leave,
+	.port_bridge_flags	= mv88e6xxx_port_bridge_flags,
+	.bridge_flags_support	= mv88e6xxx_bridge_flags_support,
 	.port_stp_state_set	= mv88e6xxx_port_stp_state_set,
 	.port_fast_age		= mv88e6xxx_port_fast_age,
 	.port_vlan_filtering	= mv88e6xxx_port_vlan_filtering,
-- 
2.7.4

[PATCH net-next 3/3] net: dsa: mv88e6xxx: defautl to multicast and unicast flooding

[Russell King]

Switches work by learning the MAC address for each attached station by
monitoring traffic from each station.  When a station sends a packet,
the switch records which port the MAC address is connected to.

With IPv4 networking, before communication commences with a neighbour,
an ARP packet is broadcasted to all stations asking for the MAC address
corresponding with the IPv4.  The desired station responds with an ARP
reply, and the ARP reply causes the switch to learn which port the
station is connected to.

With IPv6 networking, the situation is rather different.  Rather than
broadcasting ARP packets, a "neighbour solicitation" is multicasted
rather than broadcasted.  This multicast needs to reach the intended
station in order for the neighbour to be discovered.

Once a neighbour has been discovered, and entered into the sending
stations neighbour cache, communication can restart at a point later
without sending a new neighbour solicitation, even if the entry in
the neighbour cache is marked as stale.  This can be after the MAC
address has expired from the forwarding cache of the DSA switch -
when that occurs, there is a long pause in communication.

Our DSA implementation for mv88e6xxx switches has defaulted to having
multicast and unicast flooding disabled.  As per the above description,
this is fine for IPv4 networking, since the broadcasted ARP queries
will be sent to and received by all stations on the same network.
However, this breaks IPv6 very badly - blocking neighbour solicitations
and later causing connections to stall.

The defaults that the Linux bridge code expect from bridges are that
unknown unicast frames and unknown multicast frames are flooded to
all stations, which is at odds to the defaults adopted by our DSA
implementation for mv88e6xxx switches.

This commit enables by default flooding of both unknown unicast and
unknown multicast frames.  This means that mv88e6xxx DSA switches now
behave as per the bridge(8) man page, and IPv6 works flawlessly through
such a switch.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index b75a865a293d..eb5e3d88374f 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2144,13 +2144,14 @@ static int mv88e6xxx_setup_message_port(struct mv88e6xxx_chip *chip, int port)
 static int mv88e6xxx_setup_egress_floods(struct mv88e6xxx_chip *chip, int port)
 {
 	struct dsa_switch *ds = chip->ds;
-	bool flood;
 
-	/* Upstream ports flood frames with unknown unicast or multicast DA */
-	flood = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port);
+	/* Linux bridges are expected to flood unknown multicast and
+	 * unicast frames to all ports - as per the defaults specified
+	 * in the iproute2 bridge(8) man page. Not doing this causes
+	 * stalls and failures with IPv6 over Marvell bridges. */
 	if (chip->info->ops->port_set_egress_floods)
 		return chip->info->ops->port_set_egress_floods(chip, port,
-							       flood, flood);
+							       true, true);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: defautl to multicast and unicast flooding

[Russell King - ARM Linux admin]

On Sun, Feb 17, 2019 at 02:25:17PM +0000, Russell King wrote:
> Switches work by learning the MAC address for each attached station by
> monitoring traffic from each station.  When a station sends a packet,
> the switch records which port the MAC address is connected to.
> 
> With IPv4 networking, before communication commences with a neighbour,
> an ARP packet is broadcasted to all stations asking for the MAC address
> corresponding with the IPv4.  The desired station responds with an ARP
> reply, and the ARP reply causes the switch to learn which port the
> station is connected to.
> 
> With IPv6 networking, the situation is rather different.  Rather than
> broadcasting ARP packets, a "neighbour solicitation" is multicasted
> rather than broadcasted.  This multicast needs to reach the intended
> station in order for the neighbour to be discovered.
> 
> Once a neighbour has been discovered, and entered into the sending
> stations neighbour cache, communication can restart at a point later
> without sending a new neighbour solicitation, even if the entry in
> the neighbour cache is marked as stale.  This can be after the MAC
> address has expired from the forwarding cache of the DSA switch -
> when that occurs, there is a long pause in communication.
> 
> Our DSA implementation for mv88e6xxx switches has defaulted to having
> multicast and unicast flooding disabled.  As per the above description,
> this is fine for IPv4 networking, since the broadcasted ARP queries
> will be sent to and received by all stations on the same network.
> However, this breaks IPv6 very badly - blocking neighbour solicitations
> and later causing connections to stall.
> 
> The defaults that the Linux bridge code expect from bridges are that
> unknown unicast frames and unknown multicast frames are flooded to
> all stations, which is at odds to the defaults adopted by our DSA
> implementation for mv88e6xxx switches.
> 
> This commit enables by default flooding of both unknown unicast and
> unknown multicast frames.  This means that mv88e6xxx DSA switches now
> behave as per the bridge(8) man page, and IPv6 works flawlessly through
> such a switch.

Note that there is the open question whether this affects the case where
each port is used as a separate network interface: that case has not yet
been tested.

> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index b75a865a293d..eb5e3d88374f 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -2144,13 +2144,14 @@ static int mv88e6xxx_setup_message_port(struct mv88e6xxx_chip *chip, int port)
>  static int mv88e6xxx_setup_egress_floods(struct mv88e6xxx_chip *chip, int port)
>  {
>  	struct dsa_switch *ds = chip->ds;
> -	bool flood;
>  
> -	/* Upstream ports flood frames with unknown unicast or multicast DA */
> -	flood = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port);
> +	/* Linux bridges are expected to flood unknown multicast and
> +	 * unicast frames to all ports - as per the defaults specified
> +	 * in the iproute2 bridge(8) man page. Not doing this causes
> +	 * stalls and failures with IPv6 over Marvell bridges. */
>  	if (chip->info->ops->port_set_egress_floods)
>  		return chip->info->ops->port_set_egress_floods(chip, port,
> -							       flood, flood);
> +							       true, true);
>  
>  	return 0;
>  }
> -- 
> 2.7.4
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[PATCH net-next v2 0/3] net: dsa: mv88e6xxx: fix IPv6

[Russell King - ARM Linux admin]

We have had some emails in private over this issue, this is my current
patch set rebased on top of net-next which provides working IPv6 (and
probably other protocols as well) over mv88e6xxx DSA switches.

The problem comes down to mv88e6xxx defaulting to not flood unknown
unicast and multicast datagrams, as they would be by dumb switches,
and as the Linux bridge code does by default.

These flood settings can be disabled via the Linux bridge code if it's
desired to make the switch behave more like a managed switch, eg, by
enabling the multicast querier.  However, the multicast querier
defaults to being disabled which effectively means that by default,
mv88e6xxx switches block all multicast traffic.  This is at odds with
the Linux bridge documentation, and the defaults that the Linux bridge
code adopts.

So, this patch set adds DSA support for Linux bridge flags, adds
mv88e6xxx support for the unicast and multicast flooding flags, and
lastly enables flooding of these frames by default to match the
Linux bridge defaults.

 drivers/net/dsa/mv88e6xxx/chip.c | 45 ++++++++++++++++++++++++++++++++++------
 include/net/dsa.h                |  3 +++
 net/dsa/dsa_priv.h               |  2 ++
 net/dsa/port.c                   | 15 ++++++++++++++
 net/dsa/slave.c                  |  6 ++++++
 5 files changed, 65 insertions(+), 6 deletions(-)

v2: fix a couple of compile errors in patch 2 and patch 3 (oops).
-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[PATCH net-next v2 1/3] net: dsa: add support for bridge flags

[Russell King]

The Linux bridge implementation allows various properties of the bridge
to be controlled, such as flooding unknown unicast and multicast frames.
This patch adds the necessary DSA infrastructure to allow the Linux
bridge support to control these properties for DSA switches.

We implement this by providing two new methods: one to get the switch-
wide support bitmask, and another to set the properties.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 include/net/dsa.h  |  3 +++
 net/dsa/dsa_priv.h |  2 ++
 net/dsa/port.c     | 15 +++++++++++++++
 net/dsa/slave.c    |  6 ++++++
 4 files changed, 26 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 7f2a668ef2cc..6cc1222aa2ca 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -400,6 +400,9 @@ struct dsa_switch_ops {
 	void	(*port_stp_state_set)(struct dsa_switch *ds, int port,
 				      u8 state);
 	void	(*port_fast_age)(struct dsa_switch *ds, int port);
+	int	(*port_bridge_flags)(struct dsa_switch *ds, int port,
+				     unsigned long);
+	unsigned long (*bridge_flags_support)(struct dsa_switch *ds);
 
 	/*
 	 * VLAN support
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 1f4972dab9f2..f4f99ec29f5d 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -160,6 +160,8 @@ int dsa_port_mdb_add(const struct dsa_port *dp,
 		     struct switchdev_trans *trans);
 int dsa_port_mdb_del(const struct dsa_port *dp,
 		     const struct switchdev_obj_port_mdb *mdb);
+int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags,
+			  struct switchdev_trans *trans);
 int dsa_port_vlan_add(struct dsa_port *dp,
 		      const struct switchdev_obj_port_vlan *vlan,
 		      struct switchdev_trans *trans);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 2d7e01b23572..eb745041b1d9 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -177,6 +177,21 @@ int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock,
 	return dsa_port_notify(dp, DSA_NOTIFIER_AGEING_TIME, &info);
 }
 
+int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags,
+			  struct switchdev_trans *trans)
+{
+	struct dsa_switch *ds = dp->ds;
+	int port = dp->index;
+
+	if (switchdev_trans_ph_prepare(trans))
+		return ds->ops->port_bridge_flags ? 0 : -EOPNOTSUPP;
+
+	if (ds->ops->port_bridge_flags)
+		ds->ops->port_bridge_flags(ds, port, flags);
+
+	return 0;
+}
+
 int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 		     u16 vid)
 {
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 2e5e7c04821b..af53cdd14a29 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -295,6 +295,9 @@ 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_PORT_BRIDGE_FLAGS:
+		ret = dsa_port_bridge_flags(dp, attr->u.brport_flags, trans);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
@@ -384,6 +387,9 @@ static int dsa_slave_port_attr_get(struct net_device *dev,
 	switch (attr->id) {
 	case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT:
 		attr->u.brport_flags_support = 0;
+		if (ds->ops->bridge_flags_support)
+			attr->u.brport_flags_support |=
+				ds->ops->bridge_flags_support(ds);
 		break;
 	default:
 		return -EOPNOTSUPP;
-- 
2.7.4

[PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Russell King]

Add support for the bridge flags to Marvell 88e6xxx bridges, allowing
the multicast and unicast flood properties to be controlled.  These
can be controlled on a per-port basis via commands such as:

	bridge link set dev lan1 flood on|off
	bridge link set dev lan1 mcast_flood on|off

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 32e7af5caa69..72db6e74be48 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4692,6 +4692,38 @@ static int mv88e6xxx_port_mdb_del(struct dsa_switch *ds, int port,
 	return err;
 }
 
+static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
+				       unsigned long flags)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	bool unicast, multicast;
+	int ret = -EOPNOTSUPP;
+
+	unicast = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) ||
+		flags & BR_FLOOD;
+	multicast = flags & BR_MCAST_FLOOD;
+
+	mutex_lock(&chip->reg_lock);
+	if (chip->info->ops->port_set_egress_floods)
+		ret = chip->info->ops->port_set_egress_floods(chip, port,
+							      unicast,
+							      multicast);
+	mutex_unlock(&chip->reg_lock);
+
+	return ret;
+}
+
+static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	unsigned long support = 0;
+
+	if (chip->info->ops->port_set_egress_floods)
+		support |= BR_FLOOD | BR_MCAST_FLOOD;
+
+	return support;
+}
+
 static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 #if IS_ENABLED(CONFIG_NET_DSA_LEGACY)
 	.probe			= mv88e6xxx_drv_probe,
@@ -4719,6 +4751,8 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.set_ageing_time	= mv88e6xxx_set_ageing_time,
 	.port_bridge_join	= mv88e6xxx_port_bridge_join,
 	.port_bridge_leave	= mv88e6xxx_port_bridge_leave,
+	.port_bridge_flags	= mv88e6xxx_port_bridge_flags,
+	.bridge_flags_support	= mv88e6xxx_bridge_flags_support,
 	.port_stp_state_set	= mv88e6xxx_port_stp_state_set,
 	.port_fast_age		= mv88e6xxx_port_fast_age,
 	.port_vlan_filtering	= mv88e6xxx_port_vlan_filtering,
-- 
2.7.4

[PATCH net-next v2 3/3] net: dsa: mv88e6xxx: default to multicast and unicast flooding

[Russell King]

Switches work by learning the MAC address for each attached station by
monitoring traffic from each station.  When a station sends a packet,
the switch records which port the MAC address is connected to.

With IPv4 networking, before communication commences with a neighbour,
an ARP packet is broadcasted to all stations asking for the MAC address
corresponding with the IPv4.  The desired station responds with an ARP
reply, and the ARP reply causes the switch to learn which port the
station is connected to.

With IPv6 networking, the situation is rather different.  Rather than
broadcasting ARP packets, a "neighbour solicitation" is multicasted
rather than broadcasted.  This multicast needs to reach the intended
station in order for the neighbour to be discovered.

Once a neighbour has been discovered, and entered into the sending
stations neighbour cache, communication can restart at a point later
without sending a new neighbour solicitation, even if the entry in
the neighbour cache is marked as stale.  This can be after the MAC
address has expired from the forwarding cache of the DSA switch -
when that occurs, there is a long pause in communication.

Our DSA implementation for mv88e6xxx switches has defaulted to having
multicast and unicast flooding disabled.  As per the above description,
this is fine for IPv4 networking, since the broadcasted ARP queries
will be sent to and received by all stations on the same network.
However, this breaks IPv6 very badly - blocking neighbour solicitations
and later causing connections to stall.

The defaults that the Linux bridge code expect from bridges are that
unknown unicast frames and unknown multicast frames are flooded to
all stations, which is at odds to the defaults adopted by our DSA
implementation for mv88e6xxx switches.

This commit enables by default flooding of both unknown unicast and
unknown multicast frames.  This means that mv88e6xxx DSA switches now
behave as per the bridge(8) man page, and IPv6 works flawlessly through
such a switch.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 72db6e74be48..c1bcd13af13f 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2143,14 +2143,13 @@ static int mv88e6xxx_setup_message_port(struct mv88e6xxx_chip *chip, int port)
 
 static int mv88e6xxx_setup_egress_floods(struct mv88e6xxx_chip *chip, int port)
 {
-	struct dsa_switch *ds = chip->ds;
-	bool flood;
-
-	/* Upstream ports flood frames with unknown unicast or multicast DA */
-	flood = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port);
+	/* Linux bridges are expected to flood unknown multicast and
+	 * unicast frames to all ports - as per the defaults specified
+	 * in the iproute2 bridge(8) man page. Not doing this causes
+	 * stalls and failures with IPv6 over Marvell bridges. */
 	if (chip->info->ops->port_set_egress_floods)
 		return chip->info->ops->port_set_egress_floods(chip, port,
-							       flood, flood);
+							       true, true);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: defautl to multicast and unicast flooding

[Russell King - ARM Linux admin]

On Sun, Feb 17, 2019 at 02:27:16PM +0000, Russell King - ARM Linux admin wrote:
> On Sun, Feb 17, 2019 at 02:25:17PM +0000, Russell King wrote:
> > Switches work by learning the MAC address for each attached station by
> > monitoring traffic from each station.  When a station sends a packet,
> > the switch records which port the MAC address is connected to.
> > 
> > With IPv4 networking, before communication commences with a neighbour,
> > an ARP packet is broadcasted to all stations asking for the MAC address
> > corresponding with the IPv4.  The desired station responds with an ARP
> > reply, and the ARP reply causes the switch to learn which port the
> > station is connected to.
> > 
> > With IPv6 networking, the situation is rather different.  Rather than
> > broadcasting ARP packets, a "neighbour solicitation" is multicasted
> > rather than broadcasted.  This multicast needs to reach the intended
> > station in order for the neighbour to be discovered.
> > 
> > Once a neighbour has been discovered, and entered into the sending
> > stations neighbour cache, communication can restart at a point later
> > without sending a new neighbour solicitation, even if the entry in
> > the neighbour cache is marked as stale.  This can be after the MAC
> > address has expired from the forwarding cache of the DSA switch -
> > when that occurs, there is a long pause in communication.
> > 
> > Our DSA implementation for mv88e6xxx switches has defaulted to having
> > multicast and unicast flooding disabled.  As per the above description,
> > this is fine for IPv4 networking, since the broadcasted ARP queries
> > will be sent to and received by all stations on the same network.
> > However, this breaks IPv6 very badly - blocking neighbour solicitations
> > and later causing connections to stall.
> > 
> > The defaults that the Linux bridge code expect from bridges are that
> > unknown unicast frames and unknown multicast frames are flooded to
> > all stations, which is at odds to the defaults adopted by our DSA
> > implementation for mv88e6xxx switches.
> > 
> > This commit enables by default flooding of both unknown unicast and
> > unknown multicast frames.  This means that mv88e6xxx DSA switches now
> > behave as per the bridge(8) man page, and IPv6 works flawlessly through
> > such a switch.
> 
> Note that there is the open question whether this affects the case where
> each port is used as a separate network interface: that case has not yet
> been tested.

I've checked with a mv88e6131 on the clearfog gt8k board.  lan1
connected to my lan with plenty of traffic on, and configured as
part of a bridge.  lan2 connected to the zii board, but not part
of the bridge.  Monitoring lan2 from the zii board shows no traffic
that was received from lan1.

So it looks fine.

> 
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/dsa/mv88e6xxx/chip.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > index b75a865a293d..eb5e3d88374f 100644
> > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > @@ -2144,13 +2144,14 @@ static int mv88e6xxx_setup_message_port(struct mv88e6xxx_chip *chip, int port)
> >  static int mv88e6xxx_setup_egress_floods(struct mv88e6xxx_chip *chip, int port)
> >  {
> >  	struct dsa_switch *ds = chip->ds;
> > -	bool flood;
> >  
> > -	/* Upstream ports flood frames with unknown unicast or multicast DA */
> > -	flood = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port);
> > +	/* Linux bridges are expected to flood unknown multicast and
> > +	 * unicast frames to all ports - as per the defaults specified
> > +	 * in the iproute2 bridge(8) man page. Not doing this causes
> > +	 * stalls and failures with IPv6 over Marvell bridges. */
> >  	if (chip->info->ops->port_set_egress_floods)
> >  		return chip->info->ops->port_set_egress_floods(chip, port,
> > -							       flood, flood);
> > +							       true, true);
> >  
> >  	return 0;
> >  }
> > -- 
> > 2.7.4
> > 
> > 
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next 1/3] net: dsa: add support for bridge flags

[Florian Fainelli]

On 2/17/2019 6:25 AM, Russell King wrote:
> The Linux bridge implementation allows various properties of the bridge
> to be controlled, such as flooding unknown unicast and multicast frames.
> This patch adds the necessary DSA infrastructure to allow the Linux
> bridge support to control these properties for DSA switches.
> 
> We implement this by providing two new methods: one to get the switch-
> wide support bitmask, and another to set the properties.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---

[snip]

>  
> +int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags,
> +			  struct switchdev_trans *trans)
> +{
> +	struct dsa_switch *ds = dp->ds;
> +	int port = dp->index;
> +
> +	if (switchdev_trans_ph_prepare(trans))
> +		return ds->ops->port_bridge_flags ? 0 : -EOPNOTSUPP;
> +
> +	if (ds->ops->port_bridge_flags)
> +		ds->ops->port_bridge_flags(ds, port, flags);

If you have a switch fabric with multiple switches, it seems to me that
you also need to make sure that the DSA and CPU ports will have
compatible flooding attribute, so just like the port_vlan_add()
callback, you probably need to make this a switch fabric-wide event and
use a notifier here. At least the DSA ports need to have MC flooding
turned on for an user port to also have MC flooding working.

Other than that LGTM.
-- 
Florian

Re: [PATCH net-next 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Florian Fainelli]

On 2/17/2019 6:25 AM, Russell King wrote:
> Add support for the bridge flags to Marvell 88e6xxx bridges, allowing
> the multicast and unicast flood properties to be controlled.  These
> can be controlled on a per-port basis via commands such as:
> 
> 	bridge link set dev lan1 flood on|off
> 	bridge link set dev lan1 mcast_flood on|off
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

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

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: defautl to multicast and unicast flooding

[Florian Fainelli]

On 2/17/2019 8:34 AM, Russell King - ARM Linux admin wrote:
> On Sun, Feb 17, 2019 at 02:27:16PM +0000, Russell King - ARM Linux admin wrote:
>> On Sun, Feb 17, 2019 at 02:25:17PM +0000, Russell King wrote:
>>> Switches work by learning the MAC address for each attached station by
>>> monitoring traffic from each station.  When a station sends a packet,
>>> the switch records which port the MAC address is connected to.
>>>
>>> With IPv4 networking, before communication commences with a neighbour,
>>> an ARP packet is broadcasted to all stations asking for the MAC address
>>> corresponding with the IPv4.  The desired station responds with an ARP
>>> reply, and the ARP reply causes the switch to learn which port the
>>> station is connected to.
>>>
>>> With IPv6 networking, the situation is rather different.  Rather than
>>> broadcasting ARP packets, a "neighbour solicitation" is multicasted
>>> rather than broadcasted.  This multicast needs to reach the intended
>>> station in order for the neighbour to be discovered.
>>>
>>> Once a neighbour has been discovered, and entered into the sending
>>> stations neighbour cache, communication can restart at a point later
>>> without sending a new neighbour solicitation, even if the entry in
>>> the neighbour cache is marked as stale.  This can be after the MAC
>>> address has expired from the forwarding cache of the DSA switch -
>>> when that occurs, there is a long pause in communication.
>>>
>>> Our DSA implementation for mv88e6xxx switches has defaulted to having
>>> multicast and unicast flooding disabled.  As per the above description,
>>> this is fine for IPv4 networking, since the broadcasted ARP queries
>>> will be sent to and received by all stations on the same network.
>>> However, this breaks IPv6 very badly - blocking neighbour solicitations
>>> and later causing connections to stall.
>>>
>>> The defaults that the Linux bridge code expect from bridges are that
>>> unknown unicast frames and unknown multicast frames are flooded to
>>> all stations, which is at odds to the defaults adopted by our DSA
>>> implementation for mv88e6xxx switches.
>>>
>>> This commit enables by default flooding of both unknown unicast and
>>> unknown multicast frames.  This means that mv88e6xxx DSA switches now
>>> behave as per the bridge(8) man page, and IPv6 works flawlessly through
>>> such a switch.
>>
>> Note that there is the open question whether this affects the case where
>> each port is used as a separate network interface: that case has not yet
>> been tested.
> 
> I've checked with a mv88e6131 on the clearfog gt8k board.  lan1
> connected to my lan with plenty of traffic on, and configured as
> part of a bridge.  lan2 connected to the zii board, but not part
> of the bridge.  Monitoring lan2 from the zii board shows no traffic
> that was received from lan1.
> 
> So it looks fine.

With the current state whereby we do not have the necessary hooks to
perform filtering on non-bridged/standalone ports, this is entirely fine
indeed.

In the future this is part of something I want to address because it is
IMHO highly undesirable to have non-bridged ports be flooded with
unknown multicast or unknown unicast for that matter because that makes
them deviate from a standard NIC interface. Unknown unicast is not
necessarily a low hanging fruit, but still, if we have switches capable
of filtering, we might as well make use of that. Of course, one
difficulty is that we must not break running tcpdump on those DSA slave
network interfaces.

> 
>>
>>>
>>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>>> ---
>>>  drivers/net/dsa/mv88e6xxx/chip.c | 9 +++++----
>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>>> index b75a865a293d..eb5e3d88374f 100644
>>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>>> @@ -2144,13 +2144,14 @@ static int mv88e6xxx_setup_message_port(struct mv88e6xxx_chip *chip, int port)
>>>  static int mv88e6xxx_setup_egress_floods(struct mv88e6xxx_chip *chip, int port)
>>>  {
>>>  	struct dsa_switch *ds = chip->ds;
>>> -	bool flood;
>>>  
>>> -	/* Upstream ports flood frames with unknown unicast or multicast DA */
>>> -	flood = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port);
>>> +	/* Linux bridges are expected to flood unknown multicast and
>>> +	 * unicast frames to all ports - as per the defaults specified
>>> +	 * in the iproute2 bridge(8) man page. Not doing this causes
>>> +	 * stalls and failures with IPv6 over Marvell bridges. */
>>>  	if (chip->info->ops->port_set_egress_floods)
>>>  		return chip->info->ops->port_set_egress_floods(chip, port,
>>> -							       flood, flood);
>>> +							       true, true);
>>>  
>>>  	return 0;
>>>  }
>>> -- 
>>> 2.7.4
>>>
>>>
>>
>> -- 
>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
>> According to speedtest.net: 11.9Mbps down 500kbps up
> 

-- 
Florian

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: defautl to multicast and unicast flooding

[Russell King - ARM Linux admin]

On Sun, Feb 17, 2019 at 01:45:24PM -0800, Florian Fainelli wrote:
> 
> 
> On 2/17/2019 8:34 AM, Russell King - ARM Linux admin wrote:
> > On Sun, Feb 17, 2019 at 02:27:16PM +0000, Russell King - ARM Linux admin wrote:
> >> On Sun, Feb 17, 2019 at 02:25:17PM +0000, Russell King wrote:
> >>> Switches work by learning the MAC address for each attached station by
> >>> monitoring traffic from each station.  When a station sends a packet,
> >>> the switch records which port the MAC address is connected to.
> >>>
> >>> With IPv4 networking, before communication commences with a neighbour,
> >>> an ARP packet is broadcasted to all stations asking for the MAC address
> >>> corresponding with the IPv4.  The desired station responds with an ARP
> >>> reply, and the ARP reply causes the switch to learn which port the
> >>> station is connected to.
> >>>
> >>> With IPv6 networking, the situation is rather different.  Rather than
> >>> broadcasting ARP packets, a "neighbour solicitation" is multicasted
> >>> rather than broadcasted.  This multicast needs to reach the intended
> >>> station in order for the neighbour to be discovered.
> >>>
> >>> Once a neighbour has been discovered, and entered into the sending
> >>> stations neighbour cache, communication can restart at a point later
> >>> without sending a new neighbour solicitation, even if the entry in
> >>> the neighbour cache is marked as stale.  This can be after the MAC
> >>> address has expired from the forwarding cache of the DSA switch -
> >>> when that occurs, there is a long pause in communication.
> >>>
> >>> Our DSA implementation for mv88e6xxx switches has defaulted to having
> >>> multicast and unicast flooding disabled.  As per the above description,
> >>> this is fine for IPv4 networking, since the broadcasted ARP queries
> >>> will be sent to and received by all stations on the same network.
> >>> However, this breaks IPv6 very badly - blocking neighbour solicitations
> >>> and later causing connections to stall.
> >>>
> >>> The defaults that the Linux bridge code expect from bridges are that
> >>> unknown unicast frames and unknown multicast frames are flooded to
> >>> all stations, which is at odds to the defaults adopted by our DSA
> >>> implementation for mv88e6xxx switches.
> >>>
> >>> This commit enables by default flooding of both unknown unicast and
> >>> unknown multicast frames.  This means that mv88e6xxx DSA switches now
> >>> behave as per the bridge(8) man page, and IPv6 works flawlessly through
> >>> such a switch.
> >>
> >> Note that there is the open question whether this affects the case where
> >> each port is used as a separate network interface: that case has not yet
> >> been tested.
> > 
> > I've checked with a mv88e6131 on the clearfog gt8k board.  lan1
> > connected to my lan with plenty of traffic on, and configured as
> > part of a bridge.  lan2 connected to the zii board, but not part
> > of the bridge.  Monitoring lan2 from the zii board shows no traffic
> > that was received from lan1.
> > 
> > So it looks fine.
> 
> With the current state whereby we do not have the necessary hooks to
> perform filtering on non-bridged/standalone ports, this is entirely fine
> indeed.
> 
> In the future this is part of something I want to address because it is
> IMHO highly undesirable to have non-bridged ports be flooded with
> unknown multicast or unknown unicast for that matter because that makes
> them deviate from a standard NIC interface. Unknown unicast is not
> necessarily a low hanging fruit, but still, if we have switches capable
> of filtering, we might as well make use of that. Of course, one
> difficulty is that we must not break running tcpdump on those DSA slave
> network interfaces.

Sorry, I think you have the wrong end of the stick.

For a non-bridged port, I am seeing _no_ traffic apart from that
explicitly sent out through that port.  In other words, there are
_no_ flooded frames coming out of the non-bridged port.

This patch appears to have no material effect on non-bridged ports.

> 
> > 
> >>
> >>>
> >>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> >>> ---
> >>>  drivers/net/dsa/mv88e6xxx/chip.c | 9 +++++----
> >>>  1 file changed, 5 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> >>> index b75a865a293d..eb5e3d88374f 100644
> >>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> >>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> >>> @@ -2144,13 +2144,14 @@ static int mv88e6xxx_setup_message_port(struct mv88e6xxx_chip *chip, int port)
> >>>  static int mv88e6xxx_setup_egress_floods(struct mv88e6xxx_chip *chip, int port)
> >>>  {
> >>>  	struct dsa_switch *ds = chip->ds;
> >>> -	bool flood;
> >>>  
> >>> -	/* Upstream ports flood frames with unknown unicast or multicast DA */
> >>> -	flood = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port);
> >>> +	/* Linux bridges are expected to flood unknown multicast and
> >>> +	 * unicast frames to all ports - as per the defaults specified
> >>> +	 * in the iproute2 bridge(8) man page. Not doing this causes
> >>> +	 * stalls and failures with IPv6 over Marvell bridges. */
> >>>  	if (chip->info->ops->port_set_egress_floods)
> >>>  		return chip->info->ops->port_set_egress_floods(chip, port,
> >>> -							       flood, flood);
> >>> +							       true, true);
> >>>  
> >>>  	return 0;
> >>>  }
> >>> -- 
> >>> 2.7.4
> >>>
> >>>
> >>
> >> -- 
> >> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> >> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> >> According to speedtest.net: 11.9Mbps down 500kbps up
> > 
> 
> -- 
> Florian
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: defautl to multicast and unicast flooding

[Florian Fainelli]

On 2/17/2019 1:58 PM, Russell King - ARM Linux admin wrote:
> On Sun, Feb 17, 2019 at 01:45:24PM -0800, Florian Fainelli wrote:
>>
>>
>> On 2/17/2019 8:34 AM, Russell King - ARM Linux admin wrote:
>>> On Sun, Feb 17, 2019 at 02:27:16PM +0000, Russell King - ARM Linux admin wrote:
>>>> On Sun, Feb 17, 2019 at 02:25:17PM +0000, Russell King wrote:
>>>>> Switches work by learning the MAC address for each attached station by
>>>>> monitoring traffic from each station.  When a station sends a packet,
>>>>> the switch records which port the MAC address is connected to.
>>>>>
>>>>> With IPv4 networking, before communication commences with a neighbour,
>>>>> an ARP packet is broadcasted to all stations asking for the MAC address
>>>>> corresponding with the IPv4.  The desired station responds with an ARP
>>>>> reply, and the ARP reply causes the switch to learn which port the
>>>>> station is connected to.
>>>>>
>>>>> With IPv6 networking, the situation is rather different.  Rather than
>>>>> broadcasting ARP packets, a "neighbour solicitation" is multicasted
>>>>> rather than broadcasted.  This multicast needs to reach the intended
>>>>> station in order for the neighbour to be discovered.
>>>>>
>>>>> Once a neighbour has been discovered, and entered into the sending
>>>>> stations neighbour cache, communication can restart at a point later
>>>>> without sending a new neighbour solicitation, even if the entry in
>>>>> the neighbour cache is marked as stale.  This can be after the MAC
>>>>> address has expired from the forwarding cache of the DSA switch -
>>>>> when that occurs, there is a long pause in communication.
>>>>>
>>>>> Our DSA implementation for mv88e6xxx switches has defaulted to having
>>>>> multicast and unicast flooding disabled.  As per the above description,
>>>>> this is fine for IPv4 networking, since the broadcasted ARP queries
>>>>> will be sent to and received by all stations on the same network.
>>>>> However, this breaks IPv6 very badly - blocking neighbour solicitations
>>>>> and later causing connections to stall.
>>>>>
>>>>> The defaults that the Linux bridge code expect from bridges are that
>>>>> unknown unicast frames and unknown multicast frames are flooded to
>>>>> all stations, which is at odds to the defaults adopted by our DSA
>>>>> implementation for mv88e6xxx switches.
>>>>>
>>>>> This commit enables by default flooding of both unknown unicast and
>>>>> unknown multicast frames.  This means that mv88e6xxx DSA switches now
>>>>> behave as per the bridge(8) man page, and IPv6 works flawlessly through
>>>>> such a switch.
>>>>
>>>> Note that there is the open question whether this affects the case where
>>>> each port is used as a separate network interface: that case has not yet
>>>> been tested.
>>>
>>> I've checked with a mv88e6131 on the clearfog gt8k board.  lan1
>>> connected to my lan with plenty of traffic on, and configured as
>>> part of a bridge.  lan2 connected to the zii board, but not part
>>> of the bridge.  Monitoring lan2 from the zii board shows no traffic
>>> that was received from lan1.
>>>
>>> So it looks fine.
>>
>> With the current state whereby we do not have the necessary hooks to
>> perform filtering on non-bridged/standalone ports, this is entirely fine
>> indeed.
>>
>> In the future this is part of something I want to address because it is
>> IMHO highly undesirable to have non-bridged ports be flooded with
>> unknown multicast or unknown unicast for that matter because that makes
>> them deviate from a standard NIC interface. Unknown unicast is not
>> necessarily a low hanging fruit, but still, if we have switches capable
>> of filtering, we might as well make use of that. Of course, one
>> difficulty is that we must not break running tcpdump on those DSA slave
>> network interfaces.
> 
> Sorry, I think you have the wrong end of the stick.
> 
> For a non-bridged port, I am seeing _no_ traffic apart from that
> explicitly sent out through that port.  In other words, there are
> _no_ flooded frames coming out of the non-bridged port.
> 
> This patch appears to have no material effect on non-bridged ports.

Presumably because that non-bridged port and the CPU port are part of
the same domain with only those 2 ports and that is what we want.

Now what happens if say you have a station that sends multicast traffic
through that port to e.g.: 226.94.1.1, I bet that port happily sends
that multicast traffic to the CPU port with no filtering what so ever
and this ends-up being dropped in the network stack because there is a
socket look up failure there. IMHO unless you have a receiver/server on
that network interface on the DSA network interface and a matching
socket you should not be receiving that multicast traffic and the switch
should be filtering it. Since the network stack will call into
ndo_set_rx_mode() for those cases, we really just need to make that
multicast traffic known, instead of unknown to the switch.
-- 
Florian

Re: [PATCH net-next 1/3] net: dsa: add support for bridge flags

[Russell King - ARM Linux admin]

On Sun, Feb 17, 2019 at 01:37:19PM -0800, Florian Fainelli wrote:
> 
> 
> On 2/17/2019 6:25 AM, Russell King wrote:
> > The Linux bridge implementation allows various properties of the bridge
> > to be controlled, such as flooding unknown unicast and multicast frames.
> > This patch adds the necessary DSA infrastructure to allow the Linux
> > bridge support to control these properties for DSA switches.
> > 
> > We implement this by providing two new methods: one to get the switch-
> > wide support bitmask, and another to set the properties.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> 
> [snip]
> 
> >  
> > +int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags,
> > +			  struct switchdev_trans *trans)
> > +{
> > +	struct dsa_switch *ds = dp->ds;
> > +	int port = dp->index;
> > +
> > +	if (switchdev_trans_ph_prepare(trans))
> > +		return ds->ops->port_bridge_flags ? 0 : -EOPNOTSUPP;
> > +
> > +	if (ds->ops->port_bridge_flags)
> > +		ds->ops->port_bridge_flags(ds, port, flags);
> 
> If you have a switch fabric with multiple switches, it seems to me that
> you also need to make sure that the DSA and CPU ports will have
> compatible flooding attribute, so just like the port_vlan_add()
> callback, you probably need to make this a switch fabric-wide event and
> use a notifier here. At least the DSA ports need to have MC flooding
> turned on for an user port to also have MC flooding working.

mv88e6xxx already today detects CPU and DSA ports and sets unicast
and multicast flooding for these ports - see
mv88e6xxx_setup_egress_floods():

        /* Upstream ports flood frames with unknown unicast or multicast DA */
        flood = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port);
        if (chip->info->ops->port_set_egress_floods)
                return chip->info->ops->port_set_egress_floods(chip, port,
                                                               flood, flood);

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next 1/3] net: dsa: add support for bridge flags

[Florian Fainelli]

On 2/17/2019 2:04 PM, Russell King - ARM Linux admin wrote:
> On Sun, Feb 17, 2019 at 01:37:19PM -0800, Florian Fainelli wrote:
>>
>>
>> On 2/17/2019 6:25 AM, Russell King wrote:
>>> The Linux bridge implementation allows various properties of the bridge
>>> to be controlled, such as flooding unknown unicast and multicast frames.
>>> This patch adds the necessary DSA infrastructure to allow the Linux
>>> bridge support to control these properties for DSA switches.
>>>
>>> We implement this by providing two new methods: one to get the switch-
>>> wide support bitmask, and another to set the properties.
>>>
>>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>>> ---
>>
>> [snip]
>>
>>>  
>>> +int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags,
>>> +			  struct switchdev_trans *trans)
>>> +{
>>> +	struct dsa_switch *ds = dp->ds;
>>> +	int port = dp->index;
>>> +
>>> +	if (switchdev_trans_ph_prepare(trans))
>>> +		return ds->ops->port_bridge_flags ? 0 : -EOPNOTSUPP;
>>> +
>>> +	if (ds->ops->port_bridge_flags)
>>> +		ds->ops->port_bridge_flags(ds, port, flags);
>>
>> If you have a switch fabric with multiple switches, it seems to me that
>> you also need to make sure that the DSA and CPU ports will have
>> compatible flooding attribute, so just like the port_vlan_add()
>> callback, you probably need to make this a switch fabric-wide event and
>> use a notifier here. At least the DSA ports need to have MC flooding
>> turned on for an user port to also have MC flooding working.
> 
> mv88e6xxx already today detects CPU and DSA ports and sets unicast
> and multicast flooding for these ports - see
> mv88e6xxx_setup_egress_floods():

Indeed, probably for historical reasons, since that type of logic should
ideally be migrated to the core DSA layer, this is fine for now though.

> 
>         /* Upstream ports flood frames with unknown unicast or multicast DA */
>         flood = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port);
>         if (chip->info->ops->port_set_egress_floods)
>                 return chip->info->ops->port_set_egress_floods(chip, port,
>                                                                flood, flood);
> 

-- 
Florian

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: defautl to multicast and unicast flooding

[Russell King - ARM Linux admin]

On Sun, Feb 17, 2019 at 02:03:40PM -0800, Florian Fainelli wrote:
> 
> 
> On 2/17/2019 1:58 PM, Russell King - ARM Linux admin wrote:
> > On Sun, Feb 17, 2019 at 01:45:24PM -0800, Florian Fainelli wrote:
> >>
> >>
> >> On 2/17/2019 8:34 AM, Russell King - ARM Linux admin wrote:
> >>> On Sun, Feb 17, 2019 at 02:27:16PM +0000, Russell King - ARM Linux admin wrote:
> >>>> On Sun, Feb 17, 2019 at 02:25:17PM +0000, Russell King wrote:
> >>>>> Switches work by learning the MAC address for each attached station by
> >>>>> monitoring traffic from each station.  When a station sends a packet,
> >>>>> the switch records which port the MAC address is connected to.
> >>>>>
> >>>>> With IPv4 networking, before communication commences with a neighbour,
> >>>>> an ARP packet is broadcasted to all stations asking for the MAC address
> >>>>> corresponding with the IPv4.  The desired station responds with an ARP
> >>>>> reply, and the ARP reply causes the switch to learn which port the
> >>>>> station is connected to.
> >>>>>
> >>>>> With IPv6 networking, the situation is rather different.  Rather than
> >>>>> broadcasting ARP packets, a "neighbour solicitation" is multicasted
> >>>>> rather than broadcasted.  This multicast needs to reach the intended
> >>>>> station in order for the neighbour to be discovered.
> >>>>>
> >>>>> Once a neighbour has been discovered, and entered into the sending
> >>>>> stations neighbour cache, communication can restart at a point later
> >>>>> without sending a new neighbour solicitation, even if the entry in
> >>>>> the neighbour cache is marked as stale.  This can be after the MAC
> >>>>> address has expired from the forwarding cache of the DSA switch -
> >>>>> when that occurs, there is a long pause in communication.
> >>>>>
> >>>>> Our DSA implementation for mv88e6xxx switches has defaulted to having
> >>>>> multicast and unicast flooding disabled.  As per the above description,
> >>>>> this is fine for IPv4 networking, since the broadcasted ARP queries
> >>>>> will be sent to and received by all stations on the same network.
> >>>>> However, this breaks IPv6 very badly - blocking neighbour solicitations
> >>>>> and later causing connections to stall.
> >>>>>
> >>>>> The defaults that the Linux bridge code expect from bridges are that
> >>>>> unknown unicast frames and unknown multicast frames are flooded to
> >>>>> all stations, which is at odds to the defaults adopted by our DSA
> >>>>> implementation for mv88e6xxx switches.
> >>>>>
> >>>>> This commit enables by default flooding of both unknown unicast and
> >>>>> unknown multicast frames.  This means that mv88e6xxx DSA switches now
> >>>>> behave as per the bridge(8) man page, and IPv6 works flawlessly through
> >>>>> such a switch.
> >>>>
> >>>> Note that there is the open question whether this affects the case where
> >>>> each port is used as a separate network interface: that case has not yet
> >>>> been tested.
> >>>
> >>> I've checked with a mv88e6131 on the clearfog gt8k board.  lan1
> >>> connected to my lan with plenty of traffic on, and configured as
> >>> part of a bridge.  lan2 connected to the zii board, but not part
> >>> of the bridge.  Monitoring lan2 from the zii board shows no traffic
> >>> that was received from lan1.
> >>>
> >>> So it looks fine.
> >>
> >> With the current state whereby we do not have the necessary hooks to
> >> perform filtering on non-bridged/standalone ports, this is entirely fine
> >> indeed.
> >>
> >> In the future this is part of something I want to address because it is
> >> IMHO highly undesirable to have non-bridged ports be flooded with
> >> unknown multicast or unknown unicast for that matter because that makes
> >> them deviate from a standard NIC interface. Unknown unicast is not
> >> necessarily a low hanging fruit, but still, if we have switches capable
> >> of filtering, we might as well make use of that. Of course, one
> >> difficulty is that we must not break running tcpdump on those DSA slave
> >> network interfaces.
> > 
> > Sorry, I think you have the wrong end of the stick.
> > 
> > For a non-bridged port, I am seeing _no_ traffic apart from that
> > explicitly sent out through that port.  In other words, there are
> > _no_ flooded frames coming out of the non-bridged port.
> > 
> > This patch appears to have no material effect on non-bridged ports.
> 
> Presumably because that non-bridged port and the CPU port are part of
> the same domain with only those 2 ports and that is what we want.
> 
> Now what happens if say you have a station that sends multicast traffic
> through that port to e.g.: 226.94.1.1, I bet that port happily sends
> that multicast traffic to the CPU port with no filtering what so ever
> and this ends-up being dropped in the network stack because there is a
> socket look up failure there. IMHO unless you have a receiver/server on
> that network interface on the DSA network interface and a matching
> socket you should not be receiving that multicast traffic and the switch
> should be filtering it. Since the network stack will call into
> ndo_set_rx_mode() for those cases, we really just need to make that
> multicast traffic known, instead of unknown to the switch.

If the port is not bridged, then it's operating as network interface,
and traffic to/from that port needs to be routed to the CPU port so
that it appears as it would do from a real network interface.
Doing anything else makes breaks the idea that you can use a set
of DSA ports as individual interfaces and run anything but IPv4
non-multicast over them.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: defautl to multicast and unicast flooding

[Florian Fainelli]

On 2/17/2019 2:19 PM, Russell King - ARM Linux admin wrote:
> On Sun, Feb 17, 2019 at 02:03:40PM -0800, Florian Fainelli wrote:
>>
>>
>> On 2/17/2019 1:58 PM, Russell King - ARM Linux admin wrote:
>>> On Sun, Feb 17, 2019 at 01:45:24PM -0800, Florian Fainelli wrote:
>>>>
>>>>
>>>> On 2/17/2019 8:34 AM, Russell King - ARM Linux admin wrote:
>>>>> On Sun, Feb 17, 2019 at 02:27:16PM +0000, Russell King - ARM Linux admin wrote:
>>>>>> On Sun, Feb 17, 2019 at 02:25:17PM +0000, Russell King wrote:
>>>>>>> Switches work by learning the MAC address for each attached station by
>>>>>>> monitoring traffic from each station.  When a station sends a packet,
>>>>>>> the switch records which port the MAC address is connected to.
>>>>>>>
>>>>>>> With IPv4 networking, before communication commences with a neighbour,
>>>>>>> an ARP packet is broadcasted to all stations asking for the MAC address
>>>>>>> corresponding with the IPv4.  The desired station responds with an ARP
>>>>>>> reply, and the ARP reply causes the switch to learn which port the
>>>>>>> station is connected to.
>>>>>>>
>>>>>>> With IPv6 networking, the situation is rather different.  Rather than
>>>>>>> broadcasting ARP packets, a "neighbour solicitation" is multicasted
>>>>>>> rather than broadcasted.  This multicast needs to reach the intended
>>>>>>> station in order for the neighbour to be discovered.
>>>>>>>
>>>>>>> Once a neighbour has been discovered, and entered into the sending
>>>>>>> stations neighbour cache, communication can restart at a point later
>>>>>>> without sending a new neighbour solicitation, even if the entry in
>>>>>>> the neighbour cache is marked as stale.  This can be after the MAC
>>>>>>> address has expired from the forwarding cache of the DSA switch -
>>>>>>> when that occurs, there is a long pause in communication.
>>>>>>>
>>>>>>> Our DSA implementation for mv88e6xxx switches has defaulted to having
>>>>>>> multicast and unicast flooding disabled.  As per the above description,
>>>>>>> this is fine for IPv4 networking, since the broadcasted ARP queries
>>>>>>> will be sent to and received by all stations on the same network.
>>>>>>> However, this breaks IPv6 very badly - blocking neighbour solicitations
>>>>>>> and later causing connections to stall.
>>>>>>>
>>>>>>> The defaults that the Linux bridge code expect from bridges are that
>>>>>>> unknown unicast frames and unknown multicast frames are flooded to
>>>>>>> all stations, which is at odds to the defaults adopted by our DSA
>>>>>>> implementation for mv88e6xxx switches.
>>>>>>>
>>>>>>> This commit enables by default flooding of both unknown unicast and
>>>>>>> unknown multicast frames.  This means that mv88e6xxx DSA switches now
>>>>>>> behave as per the bridge(8) man page, and IPv6 works flawlessly through
>>>>>>> such a switch.
>>>>>>
>>>>>> Note that there is the open question whether this affects the case where
>>>>>> each port is used as a separate network interface: that case has not yet
>>>>>> been tested.
>>>>>
>>>>> I've checked with a mv88e6131 on the clearfog gt8k board.  lan1
>>>>> connected to my lan with plenty of traffic on, and configured as
>>>>> part of a bridge.  lan2 connected to the zii board, but not part
>>>>> of the bridge.  Monitoring lan2 from the zii board shows no traffic
>>>>> that was received from lan1.
>>>>>
>>>>> So it looks fine.
>>>>
>>>> With the current state whereby we do not have the necessary hooks to
>>>> perform filtering on non-bridged/standalone ports, this is entirely fine
>>>> indeed.
>>>>
>>>> In the future this is part of something I want to address because it is
>>>> IMHO highly undesirable to have non-bridged ports be flooded with
>>>> unknown multicast or unknown unicast for that matter because that makes
>>>> them deviate from a standard NIC interface. Unknown unicast is not
>>>> necessarily a low hanging fruit, but still, if we have switches capable
>>>> of filtering, we might as well make use of that. Of course, one
>>>> difficulty is that we must not break running tcpdump on those DSA slave
>>>> network interfaces.
>>>
>>> Sorry, I think you have the wrong end of the stick.
>>>
>>> For a non-bridged port, I am seeing _no_ traffic apart from that
>>> explicitly sent out through that port.  In other words, there are
>>> _no_ flooded frames coming out of the non-bridged port.
>>>
>>> This patch appears to have no material effect on non-bridged ports.
>>
>> Presumably because that non-bridged port and the CPU port are part of
>> the same domain with only those 2 ports and that is what we want.
>>
>> Now what happens if say you have a station that sends multicast traffic
>> through that port to e.g.: 226.94.1.1, I bet that port happily sends
>> that multicast traffic to the CPU port with no filtering what so ever
>> and this ends-up being dropped in the network stack because there is a
>> socket look up failure there. IMHO unless you have a receiver/server on
>> that network interface on the DSA network interface and a matching
>> socket you should not be receiving that multicast traffic and the switch
>> should be filtering it. Since the network stack will call into
>> ndo_set_rx_mode() for those cases, we really just need to make that
>> multicast traffic known, instead of unknown to the switch.
> 
> If the port is not bridged, then it's operating as network interface,
> and traffic to/from that port needs to be routed to the CPU port so
> that it appears as it would do from a real network interface.
> Doing anything else makes breaks the idea that you can use a set
> of DSA ports as individual interfaces and run anything but IPv4
> non-multicast over them.

I am not proposing changing any of that just making use of the switch's
ability to snoop management traffic (ARP, DHCP, MLD, etc.) and use of
the network stack's hints in order to avoid flooding unknown multicast.
This is starting to diverge a little bit from the original issue though.
-- 
Florian

Re: [PATCH net-next 1/3] net: dsa: add support for bridge flags

[Russell King - ARM Linux admin]

On Sun, Feb 17, 2019 at 02:07:54PM -0800, Florian Fainelli wrote:
> 
> 
> On 2/17/2019 2:04 PM, Russell King - ARM Linux admin wrote:
> > On Sun, Feb 17, 2019 at 01:37:19PM -0800, Florian Fainelli wrote:
> >>
> >>
> >> On 2/17/2019 6:25 AM, Russell King wrote:
> >>> The Linux bridge implementation allows various properties of the bridge
> >>> to be controlled, such as flooding unknown unicast and multicast frames.
> >>> This patch adds the necessary DSA infrastructure to allow the Linux
> >>> bridge support to control these properties for DSA switches.
> >>>
> >>> We implement this by providing two new methods: one to get the switch-
> >>> wide support bitmask, and another to set the properties.
> >>>
> >>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> >>> ---
> >>
> >> [snip]
> >>
> >>>  
> >>> +int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags,
> >>> +			  struct switchdev_trans *trans)
> >>> +{
> >>> +	struct dsa_switch *ds = dp->ds;
> >>> +	int port = dp->index;
> >>> +
> >>> +	if (switchdev_trans_ph_prepare(trans))
> >>> +		return ds->ops->port_bridge_flags ? 0 : -EOPNOTSUPP;
> >>> +
> >>> +	if (ds->ops->port_bridge_flags)
> >>> +		ds->ops->port_bridge_flags(ds, port, flags);
> >>
> >> If you have a switch fabric with multiple switches, it seems to me that
> >> you also need to make sure that the DSA and CPU ports will have
> >> compatible flooding attribute, so just like the port_vlan_add()
> >> callback, you probably need to make this a switch fabric-wide event and
> >> use a notifier here. At least the DSA ports need to have MC flooding
> >> turned on for an user port to also have MC flooding working.
> > 
> > mv88e6xxx already today detects CPU and DSA ports and sets unicast
> > and multicast flooding for these ports - see
> > mv88e6xxx_setup_egress_floods():
> 
> Indeed, probably for historical reasons, since that type of logic should
> ideally be migrated to the core DSA layer, this is fine for now though.

From what I can see, the port_vlan_add()/port_vlan_del() implementation
is far from ideal, just like "always enabling flooding for CPU/DSA
ports" is not ideal.

From what I can see, when we add a vlan to any port in a group of
switches, we turn on forwarding of frames within that vlan to all
switches and the CPU port regardless of whether there are any ports
configured on those other switches.

If we apply the same logic to flooding (which is an egress control),
then as soon as we enable flooding on any port in a bridged group of
switches, we start forwarding flooded frames to all switches and the
CPU port regardless of whether anyone there is interested.

What would be more optimal is to only enable flooding in the direction
of switches that have an interest in those patches.  For example, lets
assume we have three daisy-chained switches with three lan ports each,
with interfaces of the format swNlanN.

All of sw1's lan interfaces and one of sw2's lan interfaces are part
of a Linux bridge.  Let's say we have enabled flooding on only one of
sw1's ports and the rest are disabled.  With the way port_vlan_add()
works (which you're suggesting as a model for flooding too) we would
end up enabling flooding across all switches and to the CPU port, even
though:

- sw3 has no interest in any of the flooded frames as none of its ports
  are part of the bridge
- sw2 has no interest in any of the frames flooded from sw1

Doesn't seem optimal.

Given that, is it worth applying the same implementation to flooding
as already exists for vlans?  It would mean that as soon as we have
any single port in the group of switches that has flooding enabled,
we've enabled flooding across the entire group of switches.  As we
are unable to access the neighbouring switches state, we're unable
to turn flooding off (just like we're already unable to disable vlan
forwarding on the CPU/DSA ports).

Maybe if DSA had some infrastructure to know what downstream and
upstream switches wanted in terms of which frames, we could do better
but I don't currently see any infrastructure for that.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next 1/3] net: dsa: add support for bridge flags

[Russell King - ARM Linux admin]

On Mon, Feb 18, 2019 at 12:50:31AM +0000, Russell King - ARM Linux admin wrote:
> From what I can see, the port_vlan_add()/port_vlan_del() implementation
> is far from ideal, just like "always enabling flooding for CPU/DSA
> ports" is not ideal.

There also seems to be a discrepency between what net/dsa wants to do
and some of the implementations in drivers/net/dsa:

dsa_switch_vlan_add() does this:

        bitmap_zero(ds->bitmap, ds->num_ports);
        if (ds->index == info->sw_index)
                set_bit(info->port, ds->bitmap);
        for (port = 0; port < ds->num_ports; port++)
                if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
                        set_bit(port, ds->bitmap);

We then call ds->ops->port_vlan_add() for each port in ds->bitmap,
which will include DSA and CPU ports on every switch in the tree.

For rtl8366, this calls into rtl8366_vlan_add(), which does:

        if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
                dev_err(smi->dev, "port is DSA or CPU port\n");

which is surely a guaranteed error message if we have any CPU or DSA
ports specified on a rtl8366.  The example in the DT documentation
for this driver does suggest that it is capable of having CPU ports.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next v2 0/3] net: dsa: mv88e6xxx: fix IPv6

[Russell King - ARM Linux admin]

On Sun, Feb 17, 2019 at 04:31:14PM +0000, Russell King - ARM Linux admin wrote:
> We have had some emails in private over this issue, this is my current
> patch set rebased on top of net-next which provides working IPv6 (and
> probably other protocols as well) over mv88e6xxx DSA switches.
> 
> The problem comes down to mv88e6xxx defaulting to not flood unknown
> unicast and multicast datagrams, as they would be by dumb switches,
> and as the Linux bridge code does by default.
> 
> These flood settings can be disabled via the Linux bridge code if it's
> desired to make the switch behave more like a managed switch, eg, by
> enabling the multicast querier.  However, the multicast querier
> defaults to being disabled which effectively means that by default,
> mv88e6xxx switches block all multicast traffic.  This is at odds with
> the Linux bridge documentation, and the defaults that the Linux bridge
> code adopts.
> 
> So, this patch set adds DSA support for Linux bridge flags, adds
> mv88e6xxx support for the unicast and multicast flooding flags, and
> lastly enables flooding of these frames by default to match the
> Linux bridge defaults.

While looking at some of the other DSA drivers, I've noticed that
others are also programmed to forward unknown frames to the CPU
port.  Does this not end up breaking stuff?

If I tcpdump the ethernet interface for the CPU port, what I see
is:

11:21:21.901127 00:22:68:15:37:dd (oui Unknown) > 52:54:00:00:06:25 (oui
Unknown), ethertype MEDSA (0xdada), length 126: Forward, untagged,
dev.port:vlan 0.4:0, pri 0: ethertype IPv6 (0x86dd)
e0022681537dd.dyn.armlinux.org.uk > tftp.armlinux.org.uk: ICMP6, echo
request, seq 1, length 64

which is the unknown frame being delivered to the CPU port.  It seems
nothing else happens with the frame - it is ignored.  Before my fixes
for mv88e6xxx, that frame (and the following frames for the same MAC
address) would end up being forwarded only to the CPU port and dropped
on the floor, never making their way to their intended destination.

It seems that "the hardware doesn't know what to do, forward it to
Linux to sort out" doesn't actually work.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next v2 3/3] net: dsa: mv88e6xxx: default to multicast and unicast flooding

[Russell King - ARM Linux admin]

On Sun, Feb 17, 2019 at 04:32:40PM +0000, Russell King wrote:
> Switches work by learning the MAC address for each attached station by
> monitoring traffic from each station.  When a station sends a packet,
> the switch records which port the MAC address is connected to.
> 
> With IPv4 networking, before communication commences with a neighbour,
> an ARP packet is broadcasted to all stations asking for the MAC address
> corresponding with the IPv4.  The desired station responds with an ARP
> reply, and the ARP reply causes the switch to learn which port the
> station is connected to.
> 
> With IPv6 networking, the situation is rather different.  Rather than
> broadcasting ARP packets, a "neighbour solicitation" is multicasted
> rather than broadcasted.  This multicast needs to reach the intended
> station in order for the neighbour to be discovered.
> 
> Once a neighbour has been discovered, and entered into the sending
> stations neighbour cache, communication can restart at a point later
> without sending a new neighbour solicitation, even if the entry in
> the neighbour cache is marked as stale.  This can be after the MAC
> address has expired from the forwarding cache of the DSA switch -
> when that occurs, there is a long pause in communication.
> 
> Our DSA implementation for mv88e6xxx switches has defaulted to having
> multicast and unicast flooding disabled.  As per the above description,
> this is fine for IPv4 networking, since the broadcasted ARP queries
> will be sent to and received by all stations on the same network.
> However, this breaks IPv6 very badly - blocking neighbour solicitations
> and later causing connections to stall.
> 
> The defaults that the Linux bridge code expect from bridges are that
> unknown unicast frames and unknown multicast frames are flooded to
> all stations, which is at odds to the defaults adopted by our DSA
> implementation for mv88e6xxx switches.
> 
> This commit enables by default flooding of both unknown unicast and
> unknown multicast frames.  This means that mv88e6xxx DSA switches now
> behave as per the bridge(8) man page, and IPv6 works flawlessly through
> such a switch.

Thinking about this a bit more, this approach probably isn't the best.
If we have a port that goes through this life-cycle:

1. assigned to a bridge
2. configured not to flood
3. reassigned to a new bridge

the port will retain its settings from the first bridge, which will be
at odds with the settings that the Linux bridge code expects and the
settings visible to the user.

So, how about this, which basically reverts this patch and applies the
flood settings each time a port joins a bridge, and clears them when
the port leaves a bridge.

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 5ac2c93b7cee..6f68c35d416f 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1962,6 +1962,13 @@ static int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port,
 
 	mutex_lock(&chip->reg_lock);
 	err = mv88e6xxx_bridge_map(chip, br);
+	/* Linux bridges are expected to flood unknown multicast and
+	 * unicast frames to all ports - as per the defaults specified
+	 * in the iproute2 bridge(8) man page. Not doing this causes
+	 * stalls and failures with IPv6 over Marvell bridges. */
+	if (err == 0 && chip->info->ops->port_set_egress_floods)
+		err = chip->info->ops->port_set_egress_floods(chip, port,
+							      true, true);
 	mutex_unlock(&chip->reg_lock);
 
 	return err;
@@ -1976,6 +1983,9 @@ static void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port,
 	if (mv88e6xxx_bridge_map(chip, br) ||
 	    mv88e6xxx_port_vlan_map(chip, port))
 		dev_err(ds->dev, "failed to remap in-chip Port VLAN\n");
+	if (chip->info->ops->port_set_egress_floods &&
+	    chip->info->ops->port_set_egress_floods(chip, port, false, false))
+		dev_err(ds->dev, "failed to disable port floods\n");
 	mutex_unlock(&chip->reg_lock);
 }
 
@@ -2134,13 +2144,14 @@ static int mv88e6xxx_setup_message_port(struct mv88e6xxx_chip *chip, int port)
 
 static int mv88e6xxx_setup_egress_floods(struct mv88e6xxx_chip *chip, int port)
 {
-	/* Linux bridges are expected to flood unknown multicast and
-	 * unicast frames to all ports - as per the defaults specified
-	 * in the iproute2 bridge(8) man page. Not doing this causes
-	 * stalls and failures with IPv6 over Marvell bridges. */
+	struct dsa_switch *ds = chip->ds;
+	bool flood;
+
+	/* Upstream ports flood frames with unknown unicast or multicast DA */
+	flood = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port);
 	if (chip->info->ops->port_set_egress_floods)
 		return chip->info->ops->port_set_egress_floods(chip, port,
-							       true, true);
+							       flood, flood);
 
 	return 0;
 }

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next 1/3] net: dsa: add support for bridge flags

[Vivien Didelot]

Hi Russell,

On Mon, 18 Feb 2019 11:23:55 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> On Mon, Feb 18, 2019 at 12:50:31AM +0000, Russell King - ARM Linux admin wrote:
> > From what I can see, the port_vlan_add()/port_vlan_del() implementation
> > is far from ideal, just like "always enabling flooding for CPU/DSA
> > ports" is not ideal.
> 
> There also seems to be a discrepency between what net/dsa wants to do
> and some of the implementations in drivers/net/dsa:
> 
> dsa_switch_vlan_add() does this:
> 
>         bitmap_zero(ds->bitmap, ds->num_ports);
>         if (ds->index == info->sw_index)
>                 set_bit(info->port, ds->bitmap);
>         for (port = 0; port < ds->num_ports; port++)
>                 if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
>                         set_bit(port, ds->bitmap);
> 
> We then call ds->ops->port_vlan_add() for each port in ds->bitmap,
> which will include DSA and CPU ports on every switch in the tree.
> 
> For rtl8366, this calls into rtl8366_vlan_add(), which does:
> 
>         if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
>                 dev_err(smi->dev, "port is DSA or CPU port\n");
> 
> which is surely a guaranteed error message if we have any CPU or DSA
> ports specified on a rtl8366.  The example in the DT documentation
> for this driver does suggest that it is capable of having CPU ports.

Correct, at the very beginning, most of the logic was implemented by the DSA
drivers themselves, like programming DSA and CPU ports. But this makes them
more complicated for no value, so the logic is slowly moved up to the core,
which now handles more of the generic logic.

Ideally I would like DSA drivers to be dumb, implementing simple switch-wide
ops reflecting their specs, e.g. switch-wide vlan_add ops taking a bitmap
of ports for a given VID.

This is a work in progress and we are slowly getting there. In the meantime
there are indeed discrepencies between DSA core and drivers as you pointed out.


Thanks,

	Vivien

Re: [PATCH net-next v2 3/3] net: dsa: mv88e6xxx: default to multicast and unicast flooding

[Vivien Didelot]

Hi Russell,

On Mon, 18 Feb 2019 12:53:45 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> On Sun, Feb 17, 2019 at 04:32:40PM +0000, Russell King wrote:
> > Switches work by learning the MAC address for each attached station by
> > monitoring traffic from each station.  When a station sends a packet,
> > the switch records which port the MAC address is connected to.
> > 
> > With IPv4 networking, before communication commences with a neighbour,
> > an ARP packet is broadcasted to all stations asking for the MAC address
> > corresponding with the IPv4.  The desired station responds with an ARP
> > reply, and the ARP reply causes the switch to learn which port the
> > station is connected to.
> > 
> > With IPv6 networking, the situation is rather different.  Rather than
> > broadcasting ARP packets, a "neighbour solicitation" is multicasted
> > rather than broadcasted.  This multicast needs to reach the intended
> > station in order for the neighbour to be discovered.
> > 
> > Once a neighbour has been discovered, and entered into the sending
> > stations neighbour cache, communication can restart at a point later
> > without sending a new neighbour solicitation, even if the entry in
> > the neighbour cache is marked as stale.  This can be after the MAC
> > address has expired from the forwarding cache of the DSA switch -
> > when that occurs, there is a long pause in communication.

Thank you for the very informative message above.

> > Our DSA implementation for mv88e6xxx switches has defaulted to having
> > multicast and unicast flooding disabled.  As per the above description,
> > this is fine for IPv4 networking, since the broadcasted ARP queries
> > will be sent to and received by all stations on the same network.
> > However, this breaks IPv6 very badly - blocking neighbour solicitations
> > and later causing connections to stall.
> > 
> > The defaults that the Linux bridge code expect from bridges are that
> > unknown unicast frames and unknown multicast frames are flooded to
> > all stations, which is at odds to the defaults adopted by our DSA
> > implementation for mv88e6xxx switches.
> > 
> > This commit enables by default flooding of both unknown unicast and
> > unknown multicast frames.  This means that mv88e6xxx DSA switches now
> > behave as per the bridge(8) man page, and IPv6 works flawlessly through
> > such a switch.
> 
> Thinking about this a bit more, this approach probably isn't the best.
> If we have a port that goes through this life-cycle:
> 
> 1. assigned to a bridge
> 2. configured not to flood
> 3. reassigned to a new bridge
> 
> the port will retain its settings from the first bridge, which will be
> at odds with the settings that the Linux bridge code expects and the
> settings visible to the user.
> 
> So, how about this, which basically reverts this patch and applies the
> flood settings each time a port joins a bridge, and clears them when
> the port leaves a bridge.

Isn't the bridge code programming flooding on the port correctly on leave/join,
because the BR_*FLOOD flags have been learned? I would expect that.


Thanks,

	Vivien

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Vivien Didelot]

Hi Russell,

On Sun, 17 Feb 2019 16:32:34 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> +static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
> +				       unsigned long flags)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	bool unicast, multicast;
> +	int ret = -EOPNOTSUPP;
> +
> +	unicast = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) ||
> +		flags & BR_FLOOD;
> +	multicast = flags & BR_MCAST_FLOOD;
> +
> +	mutex_lock(&chip->reg_lock);
> +	if (chip->info->ops->port_set_egress_floods)
> +		ret = chip->info->ops->port_set_egress_floods(chip, port,
> +							      unicast,
> +							      multicast);
> +	mutex_unlock(&chip->reg_lock);
> +
> +	return ret;
> +}
> +
> +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	unsigned long support = 0;
> +
> +	if (chip->info->ops->port_set_egress_floods)
> +		support |= BR_FLOOD | BR_MCAST_FLOOD;
> +
> +	return support;
> +}

I think that it isn't necessary to propagate the notion of bridge flags down
to the DSA drivers. It might be just enough to add:

    port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)

to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
if the targeted driver has ds->ops->port_set_egress_flood. What do you think?


Thanks,

	Vivien

Re: [PATCH net-next v2 3/3] net: dsa: mv88e6xxx: default to multicast and unicast flooding

[Russell King - ARM Linux admin]

On Tue, Feb 19, 2019 at 11:05:37AM -0500, Vivien Didelot wrote:
> Hi Russell,
> 
> On Mon, 18 Feb 2019 12:53:45 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > On Sun, Feb 17, 2019 at 04:32:40PM +0000, Russell King wrote:
> > > Switches work by learning the MAC address for each attached station by
> > > monitoring traffic from each station.  When a station sends a packet,
> > > the switch records which port the MAC address is connected to.
> > > 
> > > With IPv4 networking, before communication commences with a neighbour,
> > > an ARP packet is broadcasted to all stations asking for the MAC address
> > > corresponding with the IPv4.  The desired station responds with an ARP
> > > reply, and the ARP reply causes the switch to learn which port the
> > > station is connected to.
> > > 
> > > With IPv6 networking, the situation is rather different.  Rather than
> > > broadcasting ARP packets, a "neighbour solicitation" is multicasted
> > > rather than broadcasted.  This multicast needs to reach the intended
> > > station in order for the neighbour to be discovered.
> > > 
> > > Once a neighbour has been discovered, and entered into the sending
> > > stations neighbour cache, communication can restart at a point later
> > > without sending a new neighbour solicitation, even if the entry in
> > > the neighbour cache is marked as stale.  This can be after the MAC
> > > address has expired from the forwarding cache of the DSA switch -
> > > when that occurs, there is a long pause in communication.
> 
> Thank you for the very informative message above.
> 
> > > Our DSA implementation for mv88e6xxx switches has defaulted to having
> > > multicast and unicast flooding disabled.  As per the above description,
> > > this is fine for IPv4 networking, since the broadcasted ARP queries
> > > will be sent to and received by all stations on the same network.
> > > However, this breaks IPv6 very badly - blocking neighbour solicitations
> > > and later causing connections to stall.
> > > 
> > > The defaults that the Linux bridge code expect from bridges are that
> > > unknown unicast frames and unknown multicast frames are flooded to
> > > all stations, which is at odds to the defaults adopted by our DSA
> > > implementation for mv88e6xxx switches.
> > > 
> > > This commit enables by default flooding of both unknown unicast and
> > > unknown multicast frames.  This means that mv88e6xxx DSA switches now
> > > behave as per the bridge(8) man page, and IPv6 works flawlessly through
> > > such a switch.
> > 
> > Thinking about this a bit more, this approach probably isn't the best.
> > If we have a port that goes through this life-cycle:
> > 
> > 1. assigned to a bridge
> > 2. configured not to flood
> > 3. reassigned to a new bridge
> > 
> > the port will retain its settings from the first bridge, which will be
> > at odds with the settings that the Linux bridge code expects and the
> > settings visible to the user.
> > 
> > So, how about this, which basically reverts this patch and applies the
> > flood settings each time a port joins a bridge, and clears them when
> > the port leaves a bridge.
> 
> Isn't the bridge code programming flooding on the port correctly on leave/join,
> because the BR_*FLOOD flags have been learned? I would expect that.

If you're asking whether the bridge code sends a
SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS message on leave/join, it seems
that it does not.

There is only one place in the bridge code that this message is
generated, that is in net/bridge/br_switchdev.c
br_switchdev_set_port_flag().  That is called from one place in the
bridge code, which is br_set_port_flag() in net/bridge/br_netlink.c,
which is in response to a RTM_SETLINK netlink message where the bridge
code processes all the various bridge link options.

There appears to be no call when adding or removing an interface to/
from a bridge.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Russell King - ARM Linux admin]

On Tue, Feb 19, 2019 at 11:16:12AM -0500, Vivien Didelot wrote:
> Hi Russell,
> 
> On Sun, 17 Feb 2019 16:32:34 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> > +static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
> > +				       unsigned long flags)
> > +{
> > +	struct mv88e6xxx_chip *chip = ds->priv;
> > +	bool unicast, multicast;
> > +	int ret = -EOPNOTSUPP;
> > +
> > +	unicast = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) ||
> > +		flags & BR_FLOOD;
> > +	multicast = flags & BR_MCAST_FLOOD;
> > +
> > +	mutex_lock(&chip->reg_lock);
> > +	if (chip->info->ops->port_set_egress_floods)
> > +		ret = chip->info->ops->port_set_egress_floods(chip, port,
> > +							      unicast,
> > +							      multicast);
> > +	mutex_unlock(&chip->reg_lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > +{
> > +	struct mv88e6xxx_chip *chip = ds->priv;
> > +	unsigned long support = 0;
> > +
> > +	if (chip->info->ops->port_set_egress_floods)
> > +		support |= BR_FLOOD | BR_MCAST_FLOOD;
> > +
> > +	return support;
> > +}
> 
> I think that it isn't necessary to propagate the notion of bridge flags down
> to the DSA drivers. It might be just enough to add:
> 
>     port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
> 
> to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> if the targeted driver has ds->ops->port_set_egress_flood. What do you think?

There are two other flags that I haven't covered which the bridge code
expects to be offloaded, and those are the broadcast flood flag and
the learning flag.

I know that the Marvell switches don't have a bit to control the
broadcast flooding, that appears to be controlled via a static entry
in the ATU which would have to be modified as the broadcast flood flag
is manipulated.  I don't know how that is handled in other bridges.

Do we want to include the broadcast flood in the above prototype?
If we go for this, how do we detect which options a switch supports?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Vivien Didelot]

Hi Russell,

On Tue, 19 Feb 2019 16:24:35 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > > +{
> > > +	struct mv88e6xxx_chip *chip = ds->priv;
> > > +	unsigned long support = 0;
> > > +
> > > +	if (chip->info->ops->port_set_egress_floods)
> > > +		support |= BR_FLOOD | BR_MCAST_FLOOD;
> > > +
> > > +	return support;
> > > +}
> > 
> > I think that it isn't necessary to propagate the notion of bridge flags down
> > to the DSA drivers. It might be just enough to add:
> > 
> >     port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
> > 
> > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> > if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
> 
> There are two other flags that I haven't covered which the bridge code
> expects to be offloaded, and those are the broadcast flood flag and
> the learning flag.

I see. What does the bridge code do if these flags are set? Does it expect
the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it
program this entry into the bridged ports?

In the latter case we have almost nothing to do. In the former case, we can
make the core call dsa_port_mdb_add on setup and when a VLAN is added.

mv88e6xxx tries to be smart and is already doing that and I'm really not a fan.

If tomorrow there's a switch capable of simply toggling a bit to do that,
we can add a new ops and skip the port_mdb_add call in the core.

> I know that the Marvell switches don't have a bit to control the
> broadcast flooding, that appears to be controlled via a static entry
> in the ATU which would have to be modified as the broadcast flood flag
> is manipulated.  I don't know how that is handled in other bridges.
> 
> Do we want to include the broadcast flood in the above prototype?
> If we go for this, how do we detect which options a switch supports?

If the necessary dsa_switch_ops routine is correctly prototyped, having it
implemented by a driver or not should be enough to inform the core that the
related feature(s) is/are supported by the switch.

I'll try to give a bit more context on why I'd prefer this approach, hoping
it makes sense: a switch driver does not need to understand bridge flags
per-se, the core should give enough abstraction to this layer (and any other
net-specifics). The core just needs to know if a driver can program this or
that. More importantly, it can easily become messy to maintain switch-cases
of arbitrary flags in all drivers and the core.


Thanks,

	Vivien

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Russell King - ARM Linux admin]

On Tue, Feb 19, 2019 at 11:16:12AM -0500, Vivien Didelot wrote:
> Hi Russell,
> 
> On Sun, 17 Feb 2019 16:32:34 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> > +static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
> > +				       unsigned long flags)
> > +{
> > +	struct mv88e6xxx_chip *chip = ds->priv;
> > +	bool unicast, multicast;
> > +	int ret = -EOPNOTSUPP;
> > +
> > +	unicast = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) ||
> > +		flags & BR_FLOOD;
> > +	multicast = flags & BR_MCAST_FLOOD;
> > +
> > +	mutex_lock(&chip->reg_lock);
> > +	if (chip->info->ops->port_set_egress_floods)
> > +		ret = chip->info->ops->port_set_egress_floods(chip, port,
> > +							      unicast,
> > +							      multicast);
> > +	mutex_unlock(&chip->reg_lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > +{
> > +	struct mv88e6xxx_chip *chip = ds->priv;
> > +	unsigned long support = 0;
> > +
> > +	if (chip->info->ops->port_set_egress_floods)
> > +		support |= BR_FLOOD | BR_MCAST_FLOOD;
> > +
> > +	return support;
> > +}
> 
> I think that it isn't necessary to propagate the notion of bridge flags down
> to the DSA drivers. It might be just enough to add:
> 
>     port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
> 
> to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> if the targeted driver has ds->ops->port_set_egress_flood. What do you think?

I've just changed my last patch to set these modes from
dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing,
I notice this on the ZII rev B board:

At boot (without anything connected to any of the switch ports):

br0: port 1(lan0) entered blocking state
br0: port 1(lan0) entered disabled state
device lan0 entered promiscuous mode
device eth1 entered promiscuous mode
br0: port 2(lan1) entered blocking state
br0: port 2(lan1) entered disabled state
device lan1 entered promiscuous mode
...

I then removed lan0 from the bridge:

device lan0 left promiscuous mode
br0: port 1(lan0) entered disabled state

and then added it back:

br0: port 1(lan0) entered blocking state
br0: port 1(lan0) entered disabled state
device lan0 entered promiscuous mode

Now, you'd expect lan0 and lan1 to be configured the same at this
point, and the same as it was before lan0 was removed from the bridge?
lan0 is port 0, lan1 is port 1 on this switch - and the register debug
says:

    GLOBAL GLOBAL2 SERDES     0    1    2    3    4    5    6
 0:  c800       0    1140  500f 500f 500f 500f 500f 4e07 4d04
...
 4:  40a8     258     1e0   43c  43d  43d   7c  430  53f 373f

Note that port 0 is in disabled state, but port 1 and 2 are in
blocking state... but wait, the kernel printed a message saying it was
in disabled state!

If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as
expected, and then returns to 0x43c.

It looks like DSA isn't always in sync with bridge as per port state.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Russell King - ARM Linux admin]

Hi Vivien,

On Tue, Feb 19, 2019 at 12:00:00PM -0500, Vivien Didelot wrote:
> Hi Russell,
> 
> On Tue, 19 Feb 2019 16:24:35 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > > > +{
> > > > +	struct mv88e6xxx_chip *chip = ds->priv;
> > > > +	unsigned long support = 0;
> > > > +
> > > > +	if (chip->info->ops->port_set_egress_floods)
> > > > +		support |= BR_FLOOD | BR_MCAST_FLOOD;
> > > > +
> > > > +	return support;
> > > > +}
> > > 
> > > I think that it isn't necessary to propagate the notion of bridge flags down
> > > to the DSA drivers. It might be just enough to add:
> > > 
> > >     port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
> > > 
> > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
> > 
> > There are two other flags that I haven't covered which the bridge code
> > expects to be offloaded, and those are the broadcast flood flag and
> > the learning flag.
> 
> I see. What does the bridge code do if these flags are set? Does it expect
> the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it
> program this entry into the bridged ports?

The bridge code defaults to all four flags set.  See new_nbp() in
net/bridge/br_if.c:

	p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;

bridge(8) doesn't touch BR_BCAST_FLOOD, but it is made available to
userspace via netlink and IFLA_BRPORT_BCAST_FLOOD.  Hence, there's
no man page documentation for that flag.

According to br_flood() in net/bridge/br_forward.c, it controls
whether broadcast frames are flooded to all ports or not.  Changing
this flag is merely handled just like the multicast/unicast flooding
flags - a call is made to set the offloaded flags, and if it isn't
returned as being supported, a warning is printed.  No attempt is
made to create or change a forwarding entry for the broadcast MAC
address.

bridge(8) does document BR_LEARNING via IFLA_BRPORT_LEARNING:

       learning on or learning off
              Controls whether a given port will learn MAC addresses from
              received traffic or not. If learning if off, the bridge will end
              up flooding any traffic for which it has no FDB entry. By
              default this flag is on.

> In the latter case we have almost nothing to do. In the former case, we can
> make the core call dsa_port_mdb_add on setup and when a VLAN is added.
> 
> mv88e6xxx tries to be smart and is already doing that and I'm really not a fan.
> 
> If tomorrow there's a switch capable of simply toggling a bit to do that,
> we can add a new ops and skip the port_mdb_add call in the core.
> 
> > I know that the Marvell switches don't have a bit to control the
> > broadcast flooding, that appears to be controlled via a static entry
> > in the ATU which would have to be modified as the broadcast flood flag
> > is manipulated.  I don't know how that is handled in other bridges.
> > 
> > Do we want to include the broadcast flood in the above prototype?
> > If we go for this, how do we detect which options a switch supports?
> 
> If the necessary dsa_switch_ops routine is correctly prototyped, having it
> implemented by a driver or not should be enough to inform the core that the
> related feature(s) is/are supported by the switch.
> 
> I'll try to give a bit more context on why I'd prefer this approach, hoping
> it makes sense: a switch driver does not need to understand bridge flags
> per-se, the core should give enough abstraction to this layer (and any other
> net-specifics). The core just needs to know if a driver can program this or
> that. More importantly, it can easily become messy to maintain switch-cases
> of arbitrary flags in all drivers and the core.

So, should we go the other way and have:

	int (*port_learning)(struct dsa_switch *ds, int port, bool enable);
	int (*port_egress_flood_uc)(struct dsa_switch *ds, int port, bool enable);
	int (*port_egress_flood_mc)(struct dsa_switch *ds, int port, bool enable);
	int (*port_egress_flood_bc)(struct dsa_switch *ds, int port, bool enable);

rather than trying to combine uc/mc into one?  It would mean that we'd
be performing more bus reads/writes, but I guess that doesn't matter
for these configuration parameters.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Vivien Didelot]

Hi Russell,

On Tue, 19 Feb 2019 17:00:59 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> > if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
> 
> I've just changed my last patch to set these modes from
> dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing,
> I notice this on the ZII rev B board:
> 
> At boot (without anything connected to any of the switch ports):
> 
> br0: port 1(lan0) entered blocking state
> br0: port 1(lan0) entered disabled state
> device lan0 entered promiscuous mode
> device eth1 entered promiscuous mode
> br0: port 2(lan1) entered blocking state
> br0: port 2(lan1) entered disabled state
> device lan1 entered promiscuous mode
> ...
> 
> I then removed lan0 from the bridge:
> 
> device lan0 left promiscuous mode
> br0: port 1(lan0) entered disabled state
> 
> and then added it back:
> 
> br0: port 1(lan0) entered blocking state
> br0: port 1(lan0) entered disabled state
> device lan0 entered promiscuous mode
> 
> Now, you'd expect lan0 and lan1 to be configured the same at this
> point, and the same as it was before lan0 was removed from the bridge?
> lan0 is port 0, lan1 is port 1 on this switch - and the register debug
> says:
> 
>     GLOBAL GLOBAL2 SERDES     0    1    2    3    4    5    6
>  0:  c800       0    1140  500f 500f 500f 500f 500f 4e07 4d04
> ...
>  4:  40a8     258     1e0   43c  43d  43d   7c  430  53f 373f
> 
> Note that port 0 is in disabled state, but port 1 and 2 are in
> blocking state... but wait, the kernel printed a message saying it was
> in disabled state!
> 
> If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as
> expected, and then returns to 0x43c.
> 
> It looks like DSA isn't always in sync with bridge as per port state.

One thing we have to handle in DSA core are the unbridged ports. Such isolated
ports must be in forwarding state if they are up, so that they can be used
without a bridge, as a standard network interface.

Maybe it'd be simpler if the bridge code would put the interface down when
unbridging it. That way we could remove the dsa_port_set_state_now(dp,
BR_STATE_FORWARDING) hack from dsa_port_bridge_leave. Not sure if that makes
sense though.


Thanks,

	Vivien

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Russell King - ARM Linux admin]

On Tue, Feb 19, 2019 at 12:23:04PM -0500, Vivien Didelot wrote:
> Hi Russell,
> 
> On Tue, 19 Feb 2019 17:00:59 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
> > 
> > I've just changed my last patch to set these modes from
> > dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing,
> > I notice this on the ZII rev B board:
> > 
> > At boot (without anything connected to any of the switch ports):
> > 
> > br0: port 1(lan0) entered blocking state
> > br0: port 1(lan0) entered disabled state
> > device lan0 entered promiscuous mode
> > device eth1 entered promiscuous mode
> > br0: port 2(lan1) entered blocking state
> > br0: port 2(lan1) entered disabled state
> > device lan1 entered promiscuous mode
> > ...
> > 
> > I then removed lan0 from the bridge:
> > 
> > device lan0 left promiscuous mode
> > br0: port 1(lan0) entered disabled state
> > 
> > and then added it back:
> > 
> > br0: port 1(lan0) entered blocking state
> > br0: port 1(lan0) entered disabled state
> > device lan0 entered promiscuous mode
> > 
> > Now, you'd expect lan0 and lan1 to be configured the same at this
> > point, and the same as it was before lan0 was removed from the bridge?
> > lan0 is port 0, lan1 is port 1 on this switch - and the register debug
> > says:
> > 
> >     GLOBAL GLOBAL2 SERDES     0    1    2    3    4    5    6
> >  0:  c800       0    1140  500f 500f 500f 500f 500f 4e07 4d04
> > ...
> >  4:  40a8     258     1e0   43c  43d  43d   7c  430  53f 373f
> > 
> > Note that port 0 is in disabled state, but port 1 and 2 are in
> > blocking state... but wait, the kernel printed a message saying it was
> > in disabled state!
> > 
> > If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as
> > expected, and then returns to 0x43c.
> > 
> > It looks like DSA isn't always in sync with bridge as per port state.
> 
> One thing we have to handle in DSA core are the unbridged ports. Such isolated
> ports must be in forwarding state if they are up, so that they can be used
> without a bridge, as a standard network interface.
> 
> Maybe it'd be simpler if the bridge code would put the interface down when
> unbridging it. That way we could remove the dsa_port_set_state_now(dp,
> BR_STATE_FORWARDING) hack from dsa_port_bridge_leave. Not sure if that makes
> sense though.

I'm not concerned about what happens when the port is removed from the
bridge above.

What I'm concerned about is the two different states when the port is
part of a bridge.

First time, the DSA switch is set to blocking mode, despite the kernel
saying the port is disabled.

Second time, the DSA switch is set to disabled mode, and the kernel
says the port is disabled.

Why is the port in blocking mode first time around?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Vivien Didelot]

Hi Russell,

On Tue, 19 Feb 2019 17:14:14 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > > > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > > > > +{
> > > > > +	struct mv88e6xxx_chip *chip = ds->priv;
> > > > > +	unsigned long support = 0;
> > > > > +
> > > > > +	if (chip->info->ops->port_set_egress_floods)
> > > > > +		support |= BR_FLOOD | BR_MCAST_FLOOD;
> > > > > +
> > > > > +	return support;
> > > > > +}
> > > > 
> > > > I think that it isn't necessary to propagate the notion of bridge flags down
> > > > to the DSA drivers. It might be just enough to add:
> > > > 
> > > >     port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
> > > > 
> > > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> > > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
> > > 
> > > There are two other flags that I haven't covered which the bridge code
> > > expects to be offloaded, and those are the broadcast flood flag and
> > > the learning flag.
> > 
> > I see. What does the bridge code do if these flags are set? Does it expect
> > the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it
> > program this entry into the bridged ports?
> 
> The bridge code defaults to all four flags set.  See new_nbp() in
> net/bridge/br_if.c:
> 
> 	p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;
> 
> bridge(8) doesn't touch BR_BCAST_FLOOD, but it is made available to
> userspace via netlink and IFLA_BRPORT_BCAST_FLOOD.  Hence, there's
> no man page documentation for that flag.
> 
> According to br_flood() in net/bridge/br_forward.c, it controls
> whether broadcast frames are flooded to all ports or not.  Changing
> this flag is merely handled just like the multicast/unicast flooding
> flags - a call is made to set the offloaded flags, and if it isn't
> returned as being supported, a warning is printed.  No attempt is
> made to create or change a forwarding entry for the broadcast MAC
> address.

OK, thanks for the details. The programming of the broadcast MAC address
must be handled in the core then, I will move this from mv88e6xxx up to the
DSA layer later, but that's totally orthogonal here.

> 
> bridge(8) does document BR_LEARNING via IFLA_BRPORT_LEARNING:
> 
>        learning on or learning off
>               Controls whether a given port will learn MAC addresses from
>               received traffic or not. If learning if off, the bridge will end
>               up flooding any traffic for which it has no FDB entry. By
>               default this flag is on.
> 
> > In the latter case we have almost nothing to do. In the former case, we can
> > make the core call dsa_port_mdb_add on setup and when a VLAN is added.
> > 
> > mv88e6xxx tries to be smart and is already doing that and I'm really not a fan.
> > 
> > If tomorrow there's a switch capable of simply toggling a bit to do that,
> > we can add a new ops and skip the port_mdb_add call in the core.
> > 
> > > I know that the Marvell switches don't have a bit to control the
> > > broadcast flooding, that appears to be controlled via a static entry
> > > in the ATU which would have to be modified as the broadcast flood flag
> > > is manipulated.  I don't know how that is handled in other bridges.
> > > 
> > > Do we want to include the broadcast flood in the above prototype?
> > > If we go for this, how do we detect which options a switch supports?
> > 
> > If the necessary dsa_switch_ops routine is correctly prototyped, having it
> > implemented by a driver or not should be enough to inform the core that the
> > related feature(s) is/are supported by the switch.
> > 
> > I'll try to give a bit more context on why I'd prefer this approach, hoping
> > it makes sense: a switch driver does not need to understand bridge flags
> > per-se, the core should give enough abstraction to this layer (and any other
> > net-specifics). The core just needs to know if a driver can program this or
> > that. More importantly, it can easily become messy to maintain switch-cases
> > of arbitrary flags in all drivers and the core.
> 
> So, should we go the other way and have:
> 
> 	int (*port_learning)(struct dsa_switch *ds, int port, bool enable);
> 	int (*port_egress_flood_uc)(struct dsa_switch *ds, int port, bool enable);
> 	int (*port_egress_flood_mc)(struct dsa_switch *ds, int port, bool enable);
> 	int (*port_egress_flood_bc)(struct dsa_switch *ds, int port, bool enable);
> 
> rather than trying to combine uc/mc into one?  It would mean that we'd
> be performing more bus reads/writes, but I guess that doesn't matter
> for these configuration parameters.

I like this very much. As long as these flags can be programmed in switch
devices, these ops totally make sense.


Thanks,

	Vivien

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Florian Fainelli]

On 2/19/19 9:38 AM, Vivien Didelot wrote:
> Hi Russell,
> 
> On Tue, 19 Feb 2019 17:14:14 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
>>>>>> +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
>>>>>> +{
>>>>>> +	struct mv88e6xxx_chip *chip = ds->priv;
>>>>>> +	unsigned long support = 0;
>>>>>> +
>>>>>> +	if (chip->info->ops->port_set_egress_floods)
>>>>>> +		support |= BR_FLOOD | BR_MCAST_FLOOD;
>>>>>> +
>>>>>> +	return support;
>>>>>> +}
>>>>>
>>>>> I think that it isn't necessary to propagate the notion of bridge flags down
>>>>> to the DSA drivers. It might be just enough to add:
>>>>>
>>>>>     port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
>>>>>
>>>>> to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
>>>>> if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
>>>>
>>>> There are two other flags that I haven't covered which the bridge code
>>>> expects to be offloaded, and those are the broadcast flood flag and
>>>> the learning flag.
>>>
>>> I see. What does the bridge code do if these flags are set? Does it expect
>>> the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it
>>> program this entry into the bridged ports?
>>
>> The bridge code defaults to all four flags set.  See new_nbp() in
>> net/bridge/br_if.c:
>>
>> 	p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;
>>
>> bridge(8) doesn't touch BR_BCAST_FLOOD, but it is made available to
>> userspace via netlink and IFLA_BRPORT_BCAST_FLOOD.  Hence, there's
>> no man page documentation for that flag.
>>
>> According to br_flood() in net/bridge/br_forward.c, it controls
>> whether broadcast frames are flooded to all ports or not.  Changing
>> this flag is merely handled just like the multicast/unicast flooding
>> flags - a call is made to set the offloaded flags, and if it isn't
>> returned as being supported, a warning is printed.  No attempt is
>> made to create or change a forwarding entry for the broadcast MAC
>> address.
> 
> OK, thanks for the details. The programming of the broadcast MAC address
> must be handled in the core then, I will move this from mv88e6xxx up to the
> DSA layer later, but that's totally orthogonal here.

I am not sure if it makes sense for us to work hard on supporting
BR_BCAST_FLOOD, for instance, on Broadcom switches, there does not
appear to be an easy way to specify whether broadcast traffic will be
flooded or not, it will be. The only way to tsolve that is to create a
MDB/FDB entry with negative logic (e.g.: forward to a
non-existing/connected port). Every other bridge flag typically maps 1:1
with a corresponding hardware feature, so we should support them.

> 
>>
>> bridge(8) does document BR_LEARNING via IFLA_BRPORT_LEARNING:
>>
>>        learning on or learning off
>>               Controls whether a given port will learn MAC addresses from
>>               received traffic or not. If learning if off, the bridge will end
>>               up flooding any traffic for which it has no FDB entry. By
>>               default this flag is on.
>>
>>> In the latter case we have almost nothing to do. In the former case, we can
>>> make the core call dsa_port_mdb_add on setup and when a VLAN is added.
>>>
>>> mv88e6xxx tries to be smart and is already doing that and I'm really not a fan.
>>>
>>> If tomorrow there's a switch capable of simply toggling a bit to do that,
>>> we can add a new ops and skip the port_mdb_add call in the core.
>>>
>>>> I know that the Marvell switches don't have a bit to control the
>>>> broadcast flooding, that appears to be controlled via a static entry
>>>> in the ATU which would have to be modified as the broadcast flood flag
>>>> is manipulated.  I don't know how that is handled in other bridges.
>>>>
>>>> Do we want to include the broadcast flood in the above prototype?
>>>> If we go for this, how do we detect which options a switch supports?
>>>
>>> If the necessary dsa_switch_ops routine is correctly prototyped, having it
>>> implemented by a driver or not should be enough to inform the core that the
>>> related feature(s) is/are supported by the switch.
>>>
>>> I'll try to give a bit more context on why I'd prefer this approach, hoping
>>> it makes sense: a switch driver does not need to understand bridge flags
>>> per-se, the core should give enough abstraction to this layer (and any other
>>> net-specifics). The core just needs to know if a driver can program this or
>>> that. More importantly, it can easily become messy to maintain switch-cases
>>> of arbitrary flags in all drivers and the core.
>>
>> So, should we go the other way and have:
>>
>> 	int (*port_learning)(struct dsa_switch *ds, int port, bool enable);
>> 	int (*port_egress_flood_uc)(struct dsa_switch *ds, int port, bool enable);
>> 	int (*port_egress_flood_mc)(struct dsa_switch *ds, int port, bool enable);
>> 	int (*port_egress_flood_bc)(struct dsa_switch *ds, int port, bool enable);
>>
>> rather than trying to combine uc/mc into one?  It would mean that we'd
>> be performing more bus reads/writes, but I guess that doesn't matter
>> for these configuration parameters.
> 
> I like this very much. As long as these flags can be programmed in switch
> devices, these ops totally make sense.

No objections or preference here, whatever works :)
-- 
Florian

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Russell King - ARM Linux admin]

On Tue, Feb 19, 2019 at 12:38:28PM -0500, Vivien Didelot wrote:
> Hi Russell,
> 
> On Tue, 19 Feb 2019 17:14:14 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > > > > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > > > > > +{
> > > > > > +	struct mv88e6xxx_chip *chip = ds->priv;
> > > > > > +	unsigned long support = 0;
> > > > > > +
> > > > > > +	if (chip->info->ops->port_set_egress_floods)
> > > > > > +		support |= BR_FLOOD | BR_MCAST_FLOOD;
> > > > > > +
> > > > > > +	return support;
> > > > > > +}
> > > > > 
> > > > > I think that it isn't necessary to propagate the notion of bridge flags down
> > > > > to the DSA drivers. It might be just enough to add:
> > > > > 
> > > > >     port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
> > > > > 
> > > > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> > > > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
> > > > 
> > > > There are two other flags that I haven't covered which the bridge code
> > > > expects to be offloaded, and those are the broadcast flood flag and
> > > > the learning flag.
> > > 
> > > I see. What does the bridge code do if these flags are set? Does it expect
> > > the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it
> > > program this entry into the bridged ports?
> > 
> > The bridge code defaults to all four flags set.  See new_nbp() in
> > net/bridge/br_if.c:
> > 
> > 	p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;
> > 
> > bridge(8) doesn't touch BR_BCAST_FLOOD, but it is made available to
> > userspace via netlink and IFLA_BRPORT_BCAST_FLOOD.  Hence, there's
> > no man page documentation for that flag.
> > 
> > According to br_flood() in net/bridge/br_forward.c, it controls
> > whether broadcast frames are flooded to all ports or not.  Changing
> > this flag is merely handled just like the multicast/unicast flooding
> > flags - a call is made to set the offloaded flags, and if it isn't
> > returned as being supported, a warning is printed.  No attempt is
> > made to create or change a forwarding entry for the broadcast MAC
> > address.
> 
> OK, thanks for the details. The programming of the broadcast MAC address
> must be handled in the core then, I will move this from mv88e6xxx up to the
> DSA layer later, but that's totally orthogonal here.
> 
> > 
> > bridge(8) does document BR_LEARNING via IFLA_BRPORT_LEARNING:
> > 
> >        learning on or learning off
> >               Controls whether a given port will learn MAC addresses from
> >               received traffic or not. If learning if off, the bridge will end
> >               up flooding any traffic for which it has no FDB entry. By
> >               default this flag is on.
> > 
> > > In the latter case we have almost nothing to do. In the former case, we can
> > > make the core call dsa_port_mdb_add on setup and when a VLAN is added.
> > > 
> > > mv88e6xxx tries to be smart and is already doing that and I'm really not a fan.
> > > 
> > > If tomorrow there's a switch capable of simply toggling a bit to do that,
> > > we can add a new ops and skip the port_mdb_add call in the core.
> > > 
> > > > I know that the Marvell switches don't have a bit to control the
> > > > broadcast flooding, that appears to be controlled via a static entry
> > > > in the ATU which would have to be modified as the broadcast flood flag
> > > > is manipulated.  I don't know how that is handled in other bridges.
> > > > 
> > > > Do we want to include the broadcast flood in the above prototype?
> > > > If we go for this, how do we detect which options a switch supports?
> > > 
> > > If the necessary dsa_switch_ops routine is correctly prototyped, having it
> > > implemented by a driver or not should be enough to inform the core that the
> > > related feature(s) is/are supported by the switch.
> > > 
> > > I'll try to give a bit more context on why I'd prefer this approach, hoping
> > > it makes sense: a switch driver does not need to understand bridge flags
> > > per-se, the core should give enough abstraction to this layer (and any other
> > > net-specifics). The core just needs to know if a driver can program this or
> > > that. More importantly, it can easily become messy to maintain switch-cases
> > > of arbitrary flags in all drivers and the core.
> > 
> > So, should we go the other way and have:
> > 
> > 	int (*port_learning)(struct dsa_switch *ds, int port, bool enable);
> > 	int (*port_egress_flood_uc)(struct dsa_switch *ds, int port, bool enable);
> > 	int (*port_egress_flood_mc)(struct dsa_switch *ds, int port, bool enable);
> > 	int (*port_egress_flood_bc)(struct dsa_switch *ds, int port, bool enable);
> > 
> > rather than trying to combine uc/mc into one?  It would mean that we'd
> > be performing more bus reads/writes, but I guess that doesn't matter
> > for these configuration parameters.
> 
> I like this very much. As long as these flags can be programmed in switch
> devices, these ops totally make sense.

Having these as separate functions means that we would then need
additional complexity in mv88e6xxx to store the per-port flooding state,
so we can do this:

        reg &= ~MV88E6352_PORT_CTL0_EGRESS_FLOODS_MASK;

        if (unicast && multicast)
                reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_ALL_UNKNOWN_DA;
        else if (unicast)
                reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_MC_DA;
        else if (multicast)
                reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_UC_DA;
        else
                reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_DA;

for some of the switches.  It looks to me like mv88e6xxx would prefer
having at least both the unicast and multicast flags together.

Even without that, it means more code in mv88e6xxx to wrap each of
these calls between the DSA ops and the chip specific ops...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Vivien Didelot]

Hi Florian,

On Tue, 19 Feb 2019 09:44:32 -0800, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > OK, thanks for the details. The programming of the broadcast MAC address
> > must be handled in the core then, I will move this from mv88e6xxx up to the
> > DSA layer later, but that's totally orthogonal here.
> 
> I am not sure if it makes sense for us to work hard on supporting
> BR_BCAST_FLOOD, for instance, on Broadcom switches, there does not
> appear to be an easy way to specify whether broadcast traffic will be
> flooded or not, it will be. The only way to tsolve that is to create a
> MDB/FDB entry with negative logic (e.g.: forward to a
> non-existing/connected port). Every other bridge flag typically maps 1:1
> with a corresponding hardware feature, so we should support them.

I think it's best to keep away from driver-specific hacks :-)

Since broadcom floods multicast and BR_BCAST_FLOOD is set by default,
this would simply translate as not implementing a port_flood_bc ops in the
broadcom driver.

But I agree that it doesn't seem necessary to implement support for this yet.


Thanks,

	Vivien

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Vivien Didelot]

Hi Russell,

On Tue, 19 Feb 2019 18:08:11 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> Having these as separate functions means that we would then need
> additional complexity in mv88e6xxx to store the per-port flooding state,
> so we can do this:
> 
>         reg &= ~MV88E6352_PORT_CTL0_EGRESS_FLOODS_MASK;
> 
>         if (unicast && multicast)
>                 reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_ALL_UNKNOWN_DA;
>         else if (unicast)
>                 reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_MC_DA;
>         else if (multicast)
>                 reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_UC_DA;
>         else
>                 reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_DA;
> 
> for some of the switches.  It looks to me like mv88e6xxx would prefer
> having at least both the unicast and multicast flags together.
> 
> Even without that, it means more code in mv88e6xxx to wrap each of
> these calls between the DSA ops and the chip specific ops...

True, let's stick with ops->port_egress_flood(ds, port, bool uc, bool mc).
I do not think that it is necessary to add support for BR_BCAST_FLOOD yet,
we can extend this routine later if we need to.

Your dsa_port_bridge_flags() core function can notify the understood
features. This will allow us to scope the support of the bridge flags in
the core, and preventing the drivers to do that themselves.


Thanks,

	Vivien

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Russell King - ARM Linux admin]

On Tue, Feb 19, 2019 at 02:04:44PM -0500, Vivien Didelot wrote:
> Hi Russell,
> 
> On Tue, 19 Feb 2019 18:08:11 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > Having these as separate functions means that we would then need
> > additional complexity in mv88e6xxx to store the per-port flooding state,
> > so we can do this:
> > 
> >         reg &= ~MV88E6352_PORT_CTL0_EGRESS_FLOODS_MASK;
> > 
> >         if (unicast && multicast)
> >                 reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_ALL_UNKNOWN_DA;
> >         else if (unicast)
> >                 reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_MC_DA;
> >         else if (multicast)
> >                 reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_UC_DA;
> >         else
> >                 reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_DA;
> > 
> > for some of the switches.  It looks to me like mv88e6xxx would prefer
> > having at least both the unicast and multicast flags together.
> > 
> > Even without that, it means more code in mv88e6xxx to wrap each of
> > these calls between the DSA ops and the chip specific ops...
> 
> True, let's stick with ops->port_egress_flood(ds, port, bool uc, bool mc).
> I do not think that it is necessary to add support for BR_BCAST_FLOOD yet,
> we can extend this routine later if we need to.
> 
> Your dsa_port_bridge_flags() core function can notify the understood
> features. This will allow us to scope the support of the bridge flags in
> the core, and preventing the drivers to do that themselves.

So, if we have ops->port_egress_flood, then we tell bridge that
we support BR_FLOOD | BR_MCAST_FLOOD, irrespective of whether the
bridge actually supports both?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Florian Fainelli]

On 2/19/19 11:10 AM, Russell King - ARM Linux admin wrote:
> On Tue, Feb 19, 2019 at 02:04:44PM -0500, Vivien Didelot wrote:
>> Hi Russell,
>>
>> On Tue, 19 Feb 2019 18:08:11 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
>>> Having these as separate functions means that we would then need
>>> additional complexity in mv88e6xxx to store the per-port flooding state,
>>> so we can do this:
>>>
>>>         reg &= ~MV88E6352_PORT_CTL0_EGRESS_FLOODS_MASK;
>>>
>>>         if (unicast && multicast)
>>>                 reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_ALL_UNKNOWN_DA;
>>>         else if (unicast)
>>>                 reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_MC_DA;
>>>         else if (multicast)
>>>                 reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_UC_DA;
>>>         else
>>>                 reg |= MV88E6352_PORT_CTL0_EGRESS_FLOODS_NO_UNKNOWN_DA;
>>>
>>> for some of the switches.  It looks to me like mv88e6xxx would prefer
>>> having at least both the unicast and multicast flags together.
>>>
>>> Even without that, it means more code in mv88e6xxx to wrap each of
>>> these calls between the DSA ops and the chip specific ops...
>>
>> True, let's stick with ops->port_egress_flood(ds, port, bool uc, bool mc).
>> I do not think that it is necessary to add support for BR_BCAST_FLOOD yet,
>> we can extend this routine later if we need to.
>>
>> Your dsa_port_bridge_flags() core function can notify the understood
>> features. This will allow us to scope the support of the bridge flags in
>> the core, and preventing the drivers to do that themselves.
> 
> So, if we have ops->port_egress_flood, then we tell bridge that
> we support BR_FLOOD | BR_MCAST_FLOOD, irrespective of whether the
> bridge actually supports both?

I have a patch series which removes the need for BRIDGE_FLAGS_SUPPORT
and requires you to implement an attribute setter for PRE_BRIDGE_FLAGS,
there, you can map that to the same DSA switch operations and check the
flags and deny one or both that the driver does not support.

Should I re-send it and you submit on top, or the other way around?
Either way is fine, just tell me.
-- 
Florian

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Vivien Didelot]

Hi Russell,

On Tue, 19 Feb 2019 19:10:16 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > True, let's stick with ops->port_egress_flood(ds, port, bool uc, bool mc).
> > I do not think that it is necessary to add support for BR_BCAST_FLOOD yet,
> > we can extend this routine later if we need to.
> > 
> > Your dsa_port_bridge_flags() core function can notify the understood
> > features. This will allow us to scope the support of the bridge flags in
> > the core, and preventing the drivers to do that themselves.
> 
> So, if we have ops->port_egress_flood, then we tell bridge that
> we support BR_FLOOD | BR_MCAST_FLOOD, irrespective of whether the
> bridge actually supports both?

I would say so yes. If a driver implements port_egress_flood(), this means
its switch device supports both BR_FLOOD | BR_MCAST_FLOOD.

I have one concern though. The documentation of mcast_flood for bridge(8)
says that this flag "controls whether a given port will *be flooded* with
[unknown] multicast traffic". From this I understand allowing this port to
*receive* frames with unknown destination addresses. But with mv88e6xxx, we
program whether the port is allowed to egress a frame that has an unknown
destination address. Otherwise, it will not go out this port.

Am I mistaken? If I understood correctly, is it safe to assume it is the
same thing we are implementing here?


Thanks,

	Vivien

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Russell King - ARM Linux admin]

On Tue, Feb 19, 2019 at 02:56:27PM -0500, Vivien Didelot wrote:
> Hi Russell,
> 
> On Tue, 19 Feb 2019 19:10:16 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > > True, let's stick with ops->port_egress_flood(ds, port, bool uc, bool mc).
> > > I do not think that it is necessary to add support for BR_BCAST_FLOOD yet,
> > > we can extend this routine later if we need to.
> > > 
> > > Your dsa_port_bridge_flags() core function can notify the understood
> > > features. This will allow us to scope the support of the bridge flags in
> > > the core, and preventing the drivers to do that themselves.
> > 
> > So, if we have ops->port_egress_flood, then we tell bridge that
> > we support BR_FLOOD | BR_MCAST_FLOOD, irrespective of whether the
> > bridge actually supports both?
> 
> I would say so yes. If a driver implements port_egress_flood(), this means
> its switch device supports both BR_FLOOD | BR_MCAST_FLOOD.
> 
> I have one concern though. The documentation of mcast_flood for bridge(8)
> says that this flag "controls whether a given port will *be flooded* with
> [unknown] multicast traffic". From this I understand allowing this port to
> *receive* frames with unknown destination addresses. But with mv88e6xxx, we
> program whether the port is allowed to egress a frame that has an unknown
> destination address. Otherwise, it will not go out this port.
> 
> Am I mistaken? If I understood correctly, is it safe to assume it is the
> same thing we are implementing here?

Please look at the net/bridge code to resolve questions such as this.
The relevant code is net/bridge/br_forward.c::br_flood():

void br_flood(struct net_bridge *br, struct sk_buff *skb,
              enum br_pkt_type pkt_type, bool local_rcv, bool local_orig)
{
...
        list_for_each_entry_rcu(p, &br->port_list, list) {
                /* Do not flood unicast traffic to ports that turn it off, nor
                 * other traffic if flood off, except for traffic we originate
                 */
                switch (pkt_type) {
                case BR_PKT_UNICAST:
                        if (!(p->flags & BR_FLOOD))
                                continue;
                        break;
                case BR_PKT_MULTICAST:
                        if (!(p->flags & BR_MCAST_FLOOD) && skb->dev != br->dev)                                continue;
                        break;
                case BR_PKT_BROADCAST:
                        if (!(p->flags & BR_BCAST_FLOOD) && skb->dev != br->dev)                                continue;
                        break;
                }
...
                prev = maybe_deliver(prev, p, skb, local_orig);
        }

So, BR_FLOOD, BR_MCAST_FLOOD and BR_BCAST_FLOOD control whether the
packet of type pkt_type being flooded on the bridge egresses from
port p, where p is each port attached to the bridge.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Russell King - ARM Linux admin]

On Tue, Feb 19, 2019 at 05:00:59PM +0000, Russell King - ARM Linux admin wrote:
> I've just changed my last patch to set these modes from
> dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing,
> I notice this on the ZII rev B board:
> 
> At boot (without anything connected to any of the switch ports):
> 
> br0: port 1(lan0) entered blocking state
> br0: port 1(lan0) entered disabled state
> device lan0 entered promiscuous mode
> device eth1 entered promiscuous mode
> br0: port 2(lan1) entered blocking state
> br0: port 2(lan1) entered disabled state
> device lan1 entered promiscuous mode
> ...
> 
> I then removed lan0 from the bridge:
> 
> device lan0 left promiscuous mode
> br0: port 1(lan0) entered disabled state
> 
> and then added it back:
> 
> br0: port 1(lan0) entered blocking state
> br0: port 1(lan0) entered disabled state
> device lan0 entered promiscuous mode
> 
> Now, you'd expect lan0 and lan1 to be configured the same at this
> point, and the same as it was before lan0 was removed from the bridge?
> lan0 is port 0, lan1 is port 1 on this switch - and the register debug
> says:
> 
>     GLOBAL GLOBAL2 SERDES     0    1    2    3    4    5    6
>  0:  c800       0    1140  500f 500f 500f 500f 500f 4e07 4d04
> ...
>  4:  40a8     258     1e0   43c  43d  43d   7c  430  53f 373f
> 
> Note that port 0 is in disabled state, but port 1 and 2 are in
> blocking state... but wait, the kernel printed a message saying it was
> in disabled state!
> 
> If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as
> expected, and then returns to 0x43c.
> 
> It looks like DSA isn't always in sync with bridge as per port state.

Okay, the problem is what we do when we up the port.

When the port is added to the bridge device, and it's down, the bridge
code sets the STP state to "disabled".

Then when we up the interface, dsa_slave_open() calls dsa_port_enable(),
which then decides to change the STP state on its own without reference
to the state assigned by net/bridge:

int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy)
{
        u8 stp_state = dp->bridge_dev ? BR_STATE_BLOCKING : BR_STATE_FORWARDING;
...
        dsa_port_set_state_now(dp, stp_state);
...
}

I can understand setting the state to BR_STATE_FORWARDING for
stand-alone ports, but why for bridged ports when the bridge code has
already taken care of configuring the STP state of the port?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Florian Fainelli]

On 2/19/19 3:34 PM, Russell King - ARM Linux admin wrote:
> On Tue, Feb 19, 2019 at 05:00:59PM +0000, Russell King - ARM Linux admin wrote:
>> I've just changed my last patch to set these modes from
>> dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing,
>> I notice this on the ZII rev B board:
>>
>> At boot (without anything connected to any of the switch ports):
>>
>> br0: port 1(lan0) entered blocking state
>> br0: port 1(lan0) entered disabled state
>> device lan0 entered promiscuous mode
>> device eth1 entered promiscuous mode
>> br0: port 2(lan1) entered blocking state
>> br0: port 2(lan1) entered disabled state
>> device lan1 entered promiscuous mode
>> ...
>>
>> I then removed lan0 from the bridge:
>>
>> device lan0 left promiscuous mode
>> br0: port 1(lan0) entered disabled state
>>
>> and then added it back:
>>
>> br0: port 1(lan0) entered blocking state
>> br0: port 1(lan0) entered disabled state
>> device lan0 entered promiscuous mode
>>
>> Now, you'd expect lan0 and lan1 to be configured the same at this
>> point, and the same as it was before lan0 was removed from the bridge?
>> lan0 is port 0, lan1 is port 1 on this switch - and the register debug
>> says:
>>
>>     GLOBAL GLOBAL2 SERDES     0    1    2    3    4    5    6
>>  0:  c800       0    1140  500f 500f 500f 500f 500f 4e07 4d04
>> ...
>>  4:  40a8     258     1e0   43c  43d  43d   7c  430  53f 373f
>>
>> Note that port 0 is in disabled state, but port 1 and 2 are in
>> blocking state... but wait, the kernel printed a message saying it was
>> in disabled state!
>>
>> If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as
>> expected, and then returns to 0x43c.
>>
>> It looks like DSA isn't always in sync with bridge as per port state.
> 
> Okay, the problem is what we do when we up the port.
> 
> When the port is added to the bridge device, and it's down, the bridge
> code sets the STP state to "disabled".
> 
> Then when we up the interface, dsa_slave_open() calls dsa_port_enable(),
> which then decides to change the STP state on its own without reference
> to the state assigned by net/bridge:
> 
> int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy)
> {
>         u8 stp_state = dp->bridge_dev ? BR_STATE_BLOCKING : BR_STATE_FORWARDING;
> ...
>         dsa_port_set_state_now(dp, stp_state);
> ...
> }
> 
> I can understand setting the state to BR_STATE_FORWARDING for
> stand-alone ports, but why for bridged ports when the bridge code has
> already taken care of configuring the STP state of the port?

There was no reason for doing that in commit
b73adef67765b72f2a0d01ef15aff9d784dc85da ("net: dsa: integrate with
SWITCHDEV for HW bridging") other than copying what rocker had done
(which served as model back then), and which got changed the next day in
rocker with: e47172ab7e4176883077b454286bbd5b87b5f488 ("rocker: put port
in FORWADING state after leaving bridge")

Good catch!
--
Florian

Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags

[Russell King - ARM Linux admin]

On Tue, Feb 19, 2019 at 03:53:50PM -0800, Florian Fainelli wrote:
> On 2/19/19 3:34 PM, Russell King - ARM Linux admin wrote:
> > On Tue, Feb 19, 2019 at 05:00:59PM +0000, Russell King - ARM Linux admin wrote:
> >> I've just changed my last patch to set these modes from
> >> dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing,
> >> I notice this on the ZII rev B board:
> >>
> >> At boot (without anything connected to any of the switch ports):
> >>
> >> br0: port 1(lan0) entered blocking state
> >> br0: port 1(lan0) entered disabled state
> >> device lan0 entered promiscuous mode
> >> device eth1 entered promiscuous mode
> >> br0: port 2(lan1) entered blocking state
> >> br0: port 2(lan1) entered disabled state
> >> device lan1 entered promiscuous mode
> >> ...
> >>
> >> I then removed lan0 from the bridge:
> >>
> >> device lan0 left promiscuous mode
> >> br0: port 1(lan0) entered disabled state
> >>
> >> and then added it back:
> >>
> >> br0: port 1(lan0) entered blocking state
> >> br0: port 1(lan0) entered disabled state
> >> device lan0 entered promiscuous mode
> >>
> >> Now, you'd expect lan0 and lan1 to be configured the same at this
> >> point, and the same as it was before lan0 was removed from the bridge?
> >> lan0 is port 0, lan1 is port 1 on this switch - and the register debug
> >> says:
> >>
> >>     GLOBAL GLOBAL2 SERDES     0    1    2    3    4    5    6
> >>  0:  c800       0    1140  500f 500f 500f 500f 500f 4e07 4d04
> >> ...
> >>  4:  40a8     258     1e0   43c  43d  43d   7c  430  53f 373f
> >>
> >> Note that port 0 is in disabled state, but port 1 and 2 are in
> >> blocking state... but wait, the kernel printed a message saying it was
> >> in disabled state!
> >>
> >> If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as
> >> expected, and then returns to 0x43c.
> >>
> >> It looks like DSA isn't always in sync with bridge as per port state.
> > 
> > Okay, the problem is what we do when we up the port.
> > 
> > When the port is added to the bridge device, and it's down, the bridge
> > code sets the STP state to "disabled".
> > 
> > Then when we up the interface, dsa_slave_open() calls dsa_port_enable(),
> > which then decides to change the STP state on its own without reference
> > to the state assigned by net/bridge:
> > 
> > int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy)
> > {
> >         u8 stp_state = dp->bridge_dev ? BR_STATE_BLOCKING : BR_STATE_FORWARDING;
> > ...
> >         dsa_port_set_state_now(dp, stp_state);
> > ...
> > }
> > 
> > I can understand setting the state to BR_STATE_FORWARDING for
> > stand-alone ports, but why for bridged ports when the bridge code has
> > already taken care of configuring the STP state of the port?
> 
> There was no reason for doing that in commit
> b73adef67765b72f2a0d01ef15aff9d784dc85da ("net: dsa: integrate with
> SWITCHDEV for HW bridging") other than copying what rocker had done
> (which served as model back then), and which got changed the next day in
> rocker with: e47172ab7e4176883077b454286bbd5b87b5f488 ("rocker: put port
> in FORWADING state after leaving bridge")
> 
> Good catch!

As mentioned on IRC, I'll send a patch for this tomorrow for the net
tree once I've untangled it from the floods work.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up