[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240403154921.GN26556@kernel.org>
Date: Wed, 3 Apr 2024 16:49:21 +0100
From: Simon Horman <horms@...nel.org>
To: Joseph Huang <Joseph.Huang@...min.com>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
Florian Fainelli <f.fainelli@...il.com>,
Vladimir Oltean <olteanv@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Roopa Prabhu <roopa@...dia.com>,
Nikolay Aleksandrov <razor@...ckwall.org>,
Linus Lüssing <linus.luessing@...3.blue>,
linux-kernel@...r.kernel.org, bridge@...ts.linux.dev
Subject: Re: [PATCH RFC net-next 09/10] net: dsa: mv88e6xxx: Enable mc flood
for mrouter port
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@...min.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,
...
Powered by blists - more mailing lists