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 <[email protected]>
...
> @@ -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,
...
