diff mbox series

[RFC,net-next,09/10] net: dsa: mv88e6xxx: Enable mc flood for mrouter port

Message ID 20240402001137.2980589-10-Joseph.Huang@garmin.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series MC Flood disable and snooping | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 943 this patch: 943
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang fail Errors and warnings before: 954 this patch: 957
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 954 this patch: 954
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 130 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Joseph Huang April 2, 2024, 12:11 a.m. UTC 
When a port turns into an mrouter port, enable multicast flooding
on that port even if multicast flooding is disabled by user config. This
is necessary so that in a distributed system, the multicast packets
can be forwarded to the Querier when the multicast source is attached
to a Non-Querier bridge.

Consider the following scenario:

                 +--------------------+
                 |                    |
                 |      Snooping      |    +------------+
                 |      Bridge 1      |----| Listener 1 |
                 |     (Querier)      |    +------------+
                 |                    |
                 +--------------------+
                           |
                           |
                 +--------------------+
                 |    | mrouter |     |
+-----------+    |    +---------+     |
| MC Source |----|      Snooping      |
+-----------|    |      Bridge 2      |
                 |    (Non-Querier)   |
                 +--------------------+

In this scenario, Listener 1 will never receive multicast traffic
from MC Source if multicast flooding is disabled on the mrouter port on
Snooping Bridge 2.

Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 86 ++++++++++++++++++++++++++++++--
 drivers/net/dsa/mv88e6xxx/chip.h |  1 +
 2 files changed, 83 insertions(+), 4 deletions(-)

Comments

Simon Horman April 3, 2024, 3:49 p.m. UTC | #1 
On Mon, Apr 01, 2024 at 08:11:08PM -0400, Joseph Huang wrote:
> When a port turns into an mrouter port, enable multicast flooding
> on that port even if multicast flooding is disabled by user config. This
> is necessary so that in a distributed system, the multicast packets
> can be forwarded to the Querier when the multicast source is attached
> to a Non-Querier bridge.
> 
> Consider the following scenario:
> 
>                  +--------------------+
>                  |                    |
>                  |      Snooping      |    +------------+
>                  |      Bridge 1      |----| Listener 1 |
>                  |     (Querier)      |    +------------+
>                  |                    |
>                  +--------------------+
>                            |
>                            |
>                  +--------------------+
>                  |    | mrouter |     |
> +-----------+    |    +---------+     |
> | MC Source |----|      Snooping      |
> +-----------|    |      Bridge 2      |
>                  |    (Non-Querier)   |
>                  +--------------------+
> 
> In this scenario, Listener 1 will never receive multicast traffic
> from MC Source if multicast flooding is disabled on the mrouter port on
> Snooping Bridge 2.
> 
> Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>

...

> @@ -6849,11 +6864,28 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
>  
>  	if (flags.mask & BR_MCAST_FLOOD) {
>  		bool multicast = !!(flags.val & BR_MCAST_FLOOD);
> +		struct mv88e6xxx_bridge *mv_bridge;
> +		struct mv88e6xxx_port *p;
> +		bool mrouter;
>  
> -		err = chip->info->ops->port_set_mcast_flood(chip, port,
> -							    multicast);
> -		if (err)
> -			goto out;
> +		mv_bridge = mv88e6xxx_bridge_by_port(chip, port);
> +		if (!mv_bridge)
> +			return -EINVAL;

I think that mv88e6xxx_reg_unlock(chip) is needed here.
So perhaps (completely untested!):

		if (!mv_bridge) {
			err = -EINVAL;
			goto out;
		}

Flagged by Smatch

> +
> +		p = &chip->ports[port];
> +		mrouter = !!(mv_bridge->mrouter_ports & BIT(port));
> +
> +		if (!mrouter) {
> +			err = chip->info->ops->port_set_mcast_flood(chip, port,
> +								    multicast);
> +			if (err)
> +				goto out;
> +		}
> +
> +		if (multicast)
> +			p->flags |= MV88E6XXX_PORT_FLAG_MC_FLOOD;
> +		else
> +			p->flags &= ~MV88E6XXX_PORT_FLAG_MC_FLOOD;
>  	}
>  
>  	if (flags.mask & BR_BCAST_FLOOD) {
> @@ -6883,6 +6915,51 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
>  	return err;
>  }
>  
> +static int mv88e6xxx_port_mrouter(struct dsa_switch *ds, int port,
> +				  bool mrouter,
> +				  struct netlink_ext_ack *extack)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	struct mv88e6xxx_bridge *mv_bridge;
> +	struct mv88e6xxx_port *p;
> +	bool old_mrouter;
> +	bool mc_flood;
> +	int err;
> +
> +	if (!chip->info->ops->port_set_mcast_flood)
> +		return -EOPNOTSUPP;
> +
> +	mv_bridge = mv88e6xxx_bridge_by_port(chip, port);
> +	if (!mv_bridge)
> +		return -EINVAL;
> +
> +	old_mrouter = !!(mv_bridge->mrouter_ports & BIT(port));
> +	if (mrouter == old_mrouter)
> +		return 0;
> +
> +	p = &chip->ports[port];
> +	mc_flood = !!(p->flags & MV88E6XXX_PORT_FLAG_MC_FLOOD);
> +
> +	mv88e6xxx_reg_lock(chip);
> +
> +	if (!mc_flood) {
> +		err = chip->info->ops->port_set_mcast_flood(chip, port,
> +							    mrouter);
> +		if (err)
> +			goto out;
> +	}
> +
> +	if (mrouter)
> +		mv_bridge->mrouter_ports |= BIT(port);
> +	else
> +		mv_bridge->mrouter_ports &= ~BIT(port);
> +
> +out:
> +	mv88e6xxx_reg_unlock(chip);

If mc_flood is true then err is uninitialised here.

Flagged by clang-17 W=1 build, and Smatch.

> +
> +	return err;
> +}
> +
>  static bool mv88e6xxx_lag_can_offload(struct dsa_switch *ds,
>  				      struct dsa_lag lag,
>  				      struct netdev_lag_upper_info *info,

...
diff mbox series

Patch

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 32a613c965b1..9831aa370921 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -47,6 +47,7 @@  struct mv88e6xxx_bridge {
 	struct list_head head;
 	struct net_device *br_dev;
 	u16 ports;
+	u16 mrouter_ports;
 	struct list_head br_mdb_list;
 };
 
@@ -2993,6 +2994,7 @@  static void mv88e6xxx_bridge_destroy(struct mv88e6xxx_bridge *mv_bridge)
 	list_del(&mv_bridge->head);
 
 	WARN_ON(mv_bridge->ports);
+	WARN_ON(mv_bridge->mrouter_ports);
 	WARN_ON(!list_empty(&mv_bridge->br_mdb_list));
 	kfree(mv_bridge);
 }
@@ -3010,6 +3012,19 @@  struct mv88e6xxx_bridge *mv88e6xxx_bridge_by_dev(struct mv88e6xxx_chip *chip,
 	return NULL;
 }
 
+static
+struct mv88e6xxx_bridge *mv88e6xxx_bridge_by_port(struct mv88e6xxx_chip *chip,
+						  int port)
+{
+	struct mv88e6xxx_bridge *mv_bridge;
+
+	list_for_each_entry(mv_bridge, &chip->bridge_list, head)
+		if (mv_bridge->ports & BIT(port))
+			return mv_bridge;
+
+	return NULL;
+}
+
 static struct mv88e6xxx_bridge *
 mv88e6xxx_bridge_get(struct mv88e6xxx_chip *chip, struct net_device *br_dev)
 {
@@ -6849,11 +6864,28 @@  static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
 
 	if (flags.mask & BR_MCAST_FLOOD) {
 		bool multicast = !!(flags.val & BR_MCAST_FLOOD);
+		struct mv88e6xxx_bridge *mv_bridge;
+		struct mv88e6xxx_port *p;
+		bool mrouter;
 
-		err = chip->info->ops->port_set_mcast_flood(chip, port,
-							    multicast);
-		if (err)
-			goto out;
+		mv_bridge = mv88e6xxx_bridge_by_port(chip, port);
+		if (!mv_bridge)
+			return -EINVAL;
+
+		p = &chip->ports[port];
+		mrouter = !!(mv_bridge->mrouter_ports & BIT(port));
+
+		if (!mrouter) {
+			err = chip->info->ops->port_set_mcast_flood(chip, port,
+								    multicast);
+			if (err)
+				goto out;
+		}
+
+		if (multicast)
+			p->flags |= MV88E6XXX_PORT_FLAG_MC_FLOOD;
+		else
+			p->flags &= ~MV88E6XXX_PORT_FLAG_MC_FLOOD;
 	}
 
 	if (flags.mask & BR_BCAST_FLOOD) {
@@ -6883,6 +6915,51 @@  static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
 	return err;
 }
 
+static int mv88e6xxx_port_mrouter(struct dsa_switch *ds, int port,
+				  bool mrouter,
+				  struct netlink_ext_ack *extack)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	struct mv88e6xxx_bridge *mv_bridge;
+	struct mv88e6xxx_port *p;
+	bool old_mrouter;
+	bool mc_flood;
+	int err;
+
+	if (!chip->info->ops->port_set_mcast_flood)
+		return -EOPNOTSUPP;
+
+	mv_bridge = mv88e6xxx_bridge_by_port(chip, port);
+	if (!mv_bridge)
+		return -EINVAL;
+
+	old_mrouter = !!(mv_bridge->mrouter_ports & BIT(port));
+	if (mrouter == old_mrouter)
+		return 0;
+
+	p = &chip->ports[port];
+	mc_flood = !!(p->flags & MV88E6XXX_PORT_FLAG_MC_FLOOD);
+
+	mv88e6xxx_reg_lock(chip);
+
+	if (!mc_flood) {
+		err = chip->info->ops->port_set_mcast_flood(chip, port,
+							    mrouter);
+		if (err)
+			goto out;
+	}
+
+	if (mrouter)
+		mv_bridge->mrouter_ports |= BIT(port);
+	else
+		mv_bridge->mrouter_ports &= ~BIT(port);
+
+out:
+	mv88e6xxx_reg_unlock(chip);
+
+	return err;
+}
+
 static bool mv88e6xxx_lag_can_offload(struct dsa_switch *ds,
 				      struct dsa_lag lag,
 				      struct netdev_lag_upper_info *info,
@@ -7199,6 +7276,7 @@  static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.port_bridge_leave	= mv88e6xxx_port_bridge_leave,
 	.port_pre_bridge_flags	= mv88e6xxx_port_pre_bridge_flags,
 	.port_bridge_flags	= mv88e6xxx_port_bridge_flags,
+	.port_mrouter		= mv88e6xxx_port_mrouter,
 	.port_stp_state_set	= mv88e6xxx_port_stp_state_set,
 	.port_mst_state_set	= mv88e6xxx_port_mst_state_set,
 	.port_fast_age		= mv88e6xxx_port_fast_age,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 205f6777c2ac..47e056dc7925 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -274,6 +274,7 @@  struct mv88e6xxx_vlan {
 
 /* MacAuth Bypass Control Flag */
 #define MV88E6XXX_PORT_FLAG_MAB		BIT(0)
+#define MV88E6XXX_PORT_FLAG_MC_FLOOD	BIT(1)
 
 struct mv88e6xxx_port {
 	struct mv88e6xxx_chip *chip;