Re: [PATCH RFC net-next 09/10] net: dsa: mv88e6xxx: Enable mc flood for mrouter port
From: Simon Horman <horms@kernel.org>
Date: 2024-04-03 15:49:26
Also in:
bridge, lkml
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 <redacted>...
quoted hunk ↗ jump to hunk
@@ -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
quoted hunk ↗ jump to hunk
+ + 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,
...