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

[PATCH net-next v3 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.

There is also the issue of IPv6 over a vlan that is transparent to the
bridge; the multicast querier will not reach inside the vlan, and so
the switch can not learn about multicast routing within the vlan.

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 | 19 +++++++++++++++++++
 include/net/dsa.h                |  2 ++
 net/dsa/dsa_priv.h               |  2 ++
 net/dsa/port.c                   | 28 +++++++++++++++++++++++++++-
 net/dsa/slave.c                  |  6 ++++++
 5 files changed, 56 insertions(+), 1 deletion(-)

v2: fix a couple of compile errors in patch 2 and patch 3 (oops).
v3: change interface between core DSA and drivers

-- 
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 v3 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  |  2 ++
 net/dsa/dsa_priv.h |  2 ++
 net/dsa/port.c     | 16 ++++++++++++++++
 net/dsa/slave.c    |  6 ++++++
 4 files changed, 26 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 7f2a668ef2cc..2c2c10812814 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -400,6 +400,8 @@ 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_egress_floods)(struct dsa_switch *ds, int port,
+				      bool unicast, bool multicast);
 
 	/*
 	 * 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..b84d010fb165 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -177,6 +177,22 @@ 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 0;
+
+	if (ds->ops->port_egress_floods)
+		ds->ops->port_egress_floods(ds, port, flags & BR_FLOOD,
+					    flags & BR_MCAST_FLOOD);
+
+	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..f99161c3b1ea 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->port_egress_floods)
+			attr->u.brport_flags_support |= BR_FLOOD |
+							BR_MCAST_FLOOD;
 		break;
 	default:
 		return -EOPNOTSUPP;
-- 
2.7.4

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

[Vivien Didelot]

Hi Russell,

On Wed, 20 Feb 2019 12:36:48 +0000, Russell King <rmk+kernel@armlinux.org.uk> 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.

This is not true anymore ;-)

> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  include/net/dsa.h  |  2 ++
>  net/dsa/dsa_priv.h |  2 ++
>  net/dsa/port.c     | 16 ++++++++++++++++
>  net/dsa/slave.c    |  6 ++++++
>  4 files changed, 26 insertions(+)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 7f2a668ef2cc..2c2c10812814 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -400,6 +400,8 @@ 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_egress_floods)(struct dsa_switch *ds, int port,
> +				      bool unicast, bool multicast);
>  
>  	/*
>  	 * 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..b84d010fb165 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -177,6 +177,22 @@ 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 0;
> +
> +	if (ds->ops->port_egress_floods)
> +		ds->ops->port_egress_floods(ds, port, flags & BR_FLOOD,
> +					    flags & BR_MCAST_FLOOD);

Even though we're not supposed to fail in the switchdev commit phase, I'd
prefer not to ignore the return code.

> +
> +	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..f99161c3b1ea 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->port_egress_floods)
> +			attr->u.brport_flags_support |= BR_FLOOD |
> +							BR_MCAST_FLOOD;
>  		break;
>  	default:
>  		return -EOPNOTSUPP;

Otherwise, LGTM:

Reviewed-by: Vivien Didelot <vivien.didelot@gmail.com>

[PATCH net-next v3 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 | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 32e7af5caa69..937370639fe4 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4692,6 +4692,24 @@ static int mv88e6xxx_port_mdb_del(struct dsa_switch *ds, int port,
 	return err;
 }
 
+static int mv88e6xxx_port_egress_floods(struct dsa_switch *ds, int port,
+					 bool unicast, bool multicast)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	int ret = -EOPNOTSUPP;
+
+	WARN_ON_ONCE(dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port));
+
+	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 const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 #if IS_ENABLED(CONFIG_NET_DSA_LEGACY)
 	.probe			= mv88e6xxx_drv_probe,
@@ -4719,6 +4737,7 @@ 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_egress_floods	= mv88e6xxx_port_egress_floods,
 	.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

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

[Vivien Didelot]

Hi Russell,

On Wed, 20 Feb 2019 12:36:54 +0000, Russell King <rmk+kernel@armlinux.org.uk> 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>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 32e7af5caa69..937370639fe4 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -4692,6 +4692,24 @@ static int mv88e6xxx_port_mdb_del(struct dsa_switch *ds, int port,
>  	return err;
>  }
>  
> +static int mv88e6xxx_port_egress_floods(struct dsa_switch *ds, int port,
> +					 bool unicast, bool multicast)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	int ret = -EOPNOTSUPP;

I'd prefer "err" like in the rest of the driver.

> +
> +	WARN_ON_ONCE(dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port));

No need to warn, the driver does not need to care about which port is targeted
by a call.

> +
> +	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 const struct dsa_switch_ops mv88e6xxx_switch_ops = {
>  #if IS_ENABLED(CONFIG_NET_DSA_LEGACY)
>  	.probe			= mv88e6xxx_drv_probe,
> @@ -4719,6 +4737,7 @@ 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_egress_floods	= mv88e6xxx_port_egress_floods,
>  	.port_stp_state_set	= mv88e6xxx_port_stp_state_set,
>  	.port_fast_age		= mv88e6xxx_port_fast_age,
>  	.port_vlan_filtering	= mv88e6xxx_port_vlan_filtering,

Otherwise it looks good to me.

[PATCH net-next v3 3/3] net: dsa: enable flooding for bridge ports

[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 disables flooding of
multicast and unicast frames for bridged ports.  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 for
unknown unicast and unknown multicast frames to be flooded to all ports
on the bridge, 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 whenever a port is added to a bridge, and
disables the flooding when a port leaves the bridge.  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>
---
 net/dsa/port.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index b84d010fb165..9e7aab13957e 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -105,6 +105,11 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br)
 	};
 	int err;
 
+	/* Set the flooding mode before joining */
+	err = dsa_port_bridge_flags(dp, BR_FLOOD | BR_MCAST_FLOOD, NULL);
+	if (err)
+		return err;
+
 	/* Here the port is already bridged. Reflect the current configuration
 	 * so that drivers can program their chips accordingly.
 	 */
@@ -113,8 +118,10 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br)
 	err = dsa_port_notify(dp, DSA_NOTIFIER_BRIDGE_JOIN, &info);
 
 	/* The bridging is rolled back on error */
-	if (err)
+	if (err) {
+		dsa_port_bridge_flags(dp, 0, NULL);
 		dp->bridge_dev = NULL;
+	}
 
 	return err;
 }
@@ -137,6 +144,9 @@ void dsa_port_bridge_leave(struct dsa_port *dp, struct net_device *br)
 	if (err)
 		pr_err("DSA: failed to notify DSA_NOTIFIER_BRIDGE_LEAVE\n");
 
+	/* Port is leaving the bridge, disable flooding */
+	dsa_port_bridge_flags(dp, BR_LEARNING, NULL);
+
 	/* Port left the bridge, put in BR_STATE_DISABLED by the bridge layer,
 	 * so allow it to be in BR_STATE_FORWARDING to be kept functional
 	 */
-- 
2.7.4

Re: [PATCH net-next v3 3/3] net: dsa: enable flooding for bridge ports

[Vivien Didelot]

Hi Russell,

On Wed, 20 Feb 2019 12:36:59 +0000, Russell King <rmk+kernel@armlinux.org.uk> 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 disables flooding of
> multicast and unicast frames for bridged ports.  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 for
> unknown unicast and unknown multicast frames to be flooded to all ports
> on the bridge, 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 whenever a port is added to a bridge, and
> disables the flooding when a port leaves the bridge.  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>
> ---
>  net/dsa/port.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index b84d010fb165..9e7aab13957e 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -105,6 +105,11 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br)
>  	};
>  	int err;
>  
> +	/* Set the flooding mode before joining */

Note that as stated by the comment just below, the port has already joined
the bridge here.

> +	err = dsa_port_bridge_flags(dp, BR_FLOOD | BR_MCAST_FLOOD, NULL);
> +	if (err)
> +		return err;
> +
>  	/* Here the port is already bridged. Reflect the current configuration
>  	 * so that drivers can program their chips accordingly.
>  	 */
> @@ -113,8 +118,10 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br)
>  	err = dsa_port_notify(dp, DSA_NOTIFIER_BRIDGE_JOIN, &info);
>  
>  	/* The bridging is rolled back on error */
> -	if (err)
> +	if (err) {
> +		dsa_port_bridge_flags(dp, 0, NULL);
>  		dp->bridge_dev = NULL;
> +	}
>  
>  	return err;
>  }
> @@ -137,6 +144,9 @@ void dsa_port_bridge_leave(struct dsa_port *dp, struct net_device *br)
>  	if (err)
>  		pr_err("DSA: failed to notify DSA_NOTIFIER_BRIDGE_LEAVE\n");
>  
> +	/* Port is leaving the bridge, disable flooding */
> +	dsa_port_bridge_flags(dp, BR_LEARNING, NULL);
> +
>  	/* Port left the bridge, put in BR_STATE_DISABLED by the bridge layer,
>  	 * so allow it to be in BR_STATE_FORWARDING to be kept functional
>  	 */


This makes it clear that we must add this logic which sets the expected
default flags into the bridge code itself. But this can be done later.


Reviewed-by: Vivien Didelot <vivien.didelot@gmail.com>

Re: [PATCH net-next v3 3/3] net: dsa: enable flooding for bridge ports

[Russell King - ARM Linux admin]

On Wed, Feb 20, 2019 at 12:23:55PM -0500, Vivien Didelot wrote:
> Hi Russell,
> 
> On Wed, 20 Feb 2019 12:36:59 +0000, Russell King <rmk+kernel@armlinux.org.uk> 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 disables flooding of
> > multicast and unicast frames for bridged ports.  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 for
> > unknown unicast and unknown multicast frames to be flooded to all ports
> > on the bridge, 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 whenever a port is added to a bridge, and
> > disables the flooding when a port leaves the bridge.  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>
> > ---
> >  net/dsa/port.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/dsa/port.c b/net/dsa/port.c
> > index b84d010fb165..9e7aab13957e 100644
> > --- a/net/dsa/port.c
> > +++ b/net/dsa/port.c
> > @@ -105,6 +105,11 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br)
> >  	};
> >  	int err;
> >  
> > +	/* Set the flooding mode before joining */
> 
> Note that as stated by the comment just below, the port has already joined
> the bridge here.

The software interface has joined at this point, but not the physical
port itself.  I actually find that the statement in the comment below
this code is the confusing statement.

> 
> > +	err = dsa_port_bridge_flags(dp, BR_FLOOD | BR_MCAST_FLOOD, NULL);
> > +	if (err)
> > +		return err;
> > +
> >  	/* Here the port is already bridged. Reflect the current configuration
> >  	 * so that drivers can program their chips accordingly.

This is the confusing statement: the switch port is not bridged at
this point since we haven't programmed the hardware to make that happen.
The Linux interface corresponding to the switch port is what has been
bridged.

> >  	 */
> > @@ -113,8 +118,10 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br)
> >  	err = dsa_port_notify(dp, DSA_NOTIFIER_BRIDGE_JOIN, &info);
> >  
> >  	/* The bridging is rolled back on error */
> > -	if (err)
> > +	if (err) {
> > +		dsa_port_bridge_flags(dp, 0, NULL);
> >  		dp->bridge_dev = NULL;
> > +	}
> >  
> >  	return err;
> >  }
> > @@ -137,6 +144,9 @@ void dsa_port_bridge_leave(struct dsa_port *dp, struct net_device *br)
> >  	if (err)
> >  		pr_err("DSA: failed to notify DSA_NOTIFIER_BRIDGE_LEAVE\n");
> >  
> > +	/* Port is leaving the bridge, disable flooding */
> > +	dsa_port_bridge_flags(dp, BR_LEARNING, NULL);

Hmm, that should've been 0 not BR_LEARNING since we are not dealing
with that flag yet.  I'll send a v4 later this evening.

> > +
> >  	/* Port left the bridge, put in BR_STATE_DISABLED by the bridge layer,
> >  	 * so allow it to be in BR_STATE_FORWARDING to be kept functional
> >  	 */
> 
> 
> This makes it clear that we must add this logic which sets the expected
> default flags into the bridge code itself. But this can be done later.

Note that the above is carefully chosen to ensure that the port is
configured for flooding whenever traffic may pass in bridge mode,
which I suggest is something that is kept in future.

> 
> 
> Reviewed-by: Vivien Didelot <vivien.didelot@gmail.com>
> 

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