| Message ID | 20210210091445.741269-5-olteanv@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup in brport flags switchdev offload for DSA | expand |
On 2/10/2021 1:14 AM, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > For a DSA switch port operating in standalone mode, address learning > doesn't make much sense since that is a bridge function. In fact, > address learning even breaks setups such as this one: > > +---------------------------------------------+ > | | > | +-------------------+ | > | | br0 | send receive | > | +--------+-+--------+ +--------+ +--------+ | > | | | | | | | | | | > | | swp0 | | swp1 | | swp2 | | swp3 | | > | | | | | | | | | | > +-+--------+-+--------+-+--------+-+--------+-+ > | ^ | ^ > | | | | > | +-----------+ | > | | > +--------------------------------+ > > because if the switch has a single FDB (can offload a single bridge) > then source address learning on swp3 can "steal" the source MAC address > of swp2 from br0's FDB, because learning frames coming from swp2 will be > done twice: first on the swp1 ingress port, second on the swp3 ingress > port. So the hardware FDB will become out of sync with the software > bridge, and when swp2 tries to send one more packet towards swp1, the > ASIC will attempt to short-circuit the forwarding path and send it > directly to swp3 (since that's the last port it learned that address on), > which it obviously can't, because swp3 operates in standalone mode. > > So DSA drivers operating in standalone mode should still configure a > list of bridge port flags even when they are standalone. Currently DSA > attempts to call dsa_port_bridge_flags with 0, which disables egress > flooding of unknown unicast and multicast, something which doesn't make > much sense. For the switches that implement .port_egress_floods - b53 > and mv88e6xxx, it probably doesn't matter too much either, since they > can possibly inject traffic from the CPU into a standalone port, > regardless of MAC DA, even if egress flooding is turned off for that > port, but certainly not all DSA switches can do that - sja1105, for > example, can't. So it makes sense to use a better common default there, > such as "flood everything". > > It should also be noted that what DSA calls "dsa_port_bridge_flags()" > is a degenerate name for just calling .port_egress_floods(), since > nothing else is implemented - not learning, in particular. But disabling > address learning, something that this driver is also coding up for, will > be supported by individual drivers once .port_egress_floods is replaced > with a more generic .port_bridge_flags. > > Previous attempts to code up this logic have been in the common bridge > layer, but as pointed out by Ido Schimmel, there are corner cases that > are missed when doing that: > https://patchwork.kernel.org/project/netdevbpf/patch/20210209151936.97382-5-olteanv@gmail.com/ > > So, at least for now, let's leave DSA in charge of setting port flags > before and after the bridge join and leave. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
diff --git a/net/dsa/port.c b/net/dsa/port.c index 5e079a61528e..9f65ba11ad00 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -122,6 +122,27 @@ void dsa_port_disable(struct dsa_port *dp) rtnl_unlock(); } +static void dsa_port_change_brport_flags(struct dsa_port *dp, + bool bridge_offload) +{ + unsigned long mask, flags; + int flag, err; + + mask = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD; + if (bridge_offload) + flags = mask; + else + flags = mask & ~BR_LEARNING; + + for_each_set_bit(flag, &mask, 32) { + err = dsa_port_pre_bridge_flags(dp, BIT(flag)); + if (err) + continue; + + dsa_port_bridge_flags(dp, flags & BIT(flag)); + } +} + int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br) { struct dsa_notifier_bridge_info info = { @@ -132,10 +153,10 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br) }; int err; - /* Set the flooding mode before joining the port in the switch */ - err = dsa_port_bridge_flags(dp, BR_FLOOD | BR_MCAST_FLOOD); - if (err) - return err; + /* Notify the port driver to set its configurable flags in a way that + * matches the initial settings of a bridge port. + */ + dsa_port_change_brport_flags(dp, true); /* Here the interface is already bridged. Reflect the current * configuration so that drivers can program their chips accordingly. @@ -146,7 +167,7 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br) /* The bridging is rolled back on error */ if (err) { - dsa_port_bridge_flags(dp, 0); + dsa_port_change_brport_flags(dp, false); dp->bridge_dev = NULL; } @@ -172,8 +193,18 @@ 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, 0); + /* Configure the port for standalone mode (no address learning, + * flood everything). + * The bridge only emits SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS events + * when the user requests it through netlink or sysfs, but not + * automatically at port join or leave, so we need to handle resetting + * the brport flags ourselves. But we even prefer it that way, because + * otherwise, some setups might never get the notification they need, + * for example, when a port leaves a LAG that offloads the bridge, + * it becomes standalone, but as far as the bridge is concerned, no + * port ever left. + */ + dsa_port_change_brport_flags(dp, false); /* 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