Skip to content

Commit

Permalink
net: bridge: move the switchdev object replay helpers to "push" mode
Browse files Browse the repository at this point in the history
Starting with commit 4f2673b ("net: bridge: add helper to replay
port and host-joined mdb entries"), DSA has introduced some bridge
helpers that replay switchdev events (FDB/MDB/VLAN additions and
deletions) that can be lost by the switchdev drivers in a variety of
circumstances:

- an IP multicast group was host-joined on the bridge itself before any
  switchdev port joined the bridge, leading to the host MDB entries
  missing in the hardware database.
- during the bridge creation process, the MAC address of the bridge was
  added to the FDB as an entry pointing towards the bridge device
  itself, but with no switchdev ports being part of the bridge yet, this
  local FDB entry would remain unknown to the switchdev hardware
  database.
- a VLAN/FDB/MDB was added to a bridge port that is a LAG interface,
  before any switchdev port joined that LAG, leading to the hardware
  database missing those entries.
- a switchdev port left a LAG that is a bridge port, while the LAG
  remained part of the bridge, and all FDB/MDB/VLAN entries remained
  installed in the hardware database of the switchdev port.

Also, since commit 0d2cfbd ("net: bridge: ignore switchdev events
for LAG ports which didn't request replay"), DSA introduced a method,
based on a const void *ctx, to ensure that two switchdev ports under the
same LAG that is a bridge port do not see the same MDB/VLAN entry being
replayed twice by the bridge, once for every bridge port that joins the
LAG.

With so many ordering corner cases being possible, it seems unreasonable
to expect a switchdev driver writer to get it right from the first try.
Therefore, now that DSA has experimented with the bridge replay helpers
for a little bit, we can move the code to the bridge driver where it is
more readily available to all switchdev drivers.

To convert the switchdev object replay helpers from "pull mode" (where
the driver asks for them) to a "push mode" (where the bridge offers them
automatically), the biggest problem is that the bridge needs to be aware
when a switchdev port joins and leaves, even when the switchdev is only
indirectly a bridge port (for example when the bridge port is a LAG
upper of the switchdev).

Luckily, we already have a hook for that, in the form of the newly
introduced switchdev_bridge_port_offload() and
switchdev_bridge_port_unoffload() calls. These offer a natural place for
hooking the object addition and deletion replays.

Extend the above 2 functions with:
- pointers to the switchdev atomic notifier (for FDB replays) and the
  blocking notifier (for MDB and VLAN replays).
- the "const void *ctx" argument required for drivers to be able to
  disambiguate between which port is targeted, when multiple ports are
  lowers of the same LAG that is a bridge port. Most of the drivers pass
  NULL to this argument, except the ones that support LAG offload and have
  the proper context check already in place in the switchdev blocking
  notifier handler.

Also unexport the replay helpers, since nobody except the bridge calls
them directly now.

Note that:
(a) we abuse the terminology slightly, because FDB entries are not
    "switchdev objects", but we count them as objects nonetheless.
    With no direct way to prove it, I think they are not modeled as
    switchdev objects because those can only be installed by the bridge
    to the hardware (as opposed to FDB entries which can be propagated
    in the other direction too). This is merely an abuse of terms, FDB
    entries are replayed too, despite not being objects.
(b) the bridge does not attempt to sync port attributes to newly joined
    ports, just the countable stuff (the objects). The reason for this
    is simple: no universal and symmetric way to sync and unsync them is
    known. For example, VLAN filtering: what to do on unsync, disable or
    leave it enabled? Similarly, STP state, ageing timer, etc etc. What
    a switchdev port does when it becomes standalone again is not really
    up to the bridge's competence, and the driver should deal with it.
    On the other hand, replaying deletions of switchdev objects can be
    seen a matter of cleanup and therefore be treated by the bridge,
    hence this patch.

We make the replay helpers opt-in for drivers, because they might not
bring immediate benefits for them:

- nbp_vlan_init() is called _after_ netdev_master_upper_dev_link(),
  so br_vlan_replay() should not do anything for the new drivers on
  which we call it. The existing drivers where there was even a slight
  possibility for there to exist a VLAN on a bridge port before they
  join it are already guarded against this: mlxsw and prestera deny
  joining LAG interfaces that are members of a bridge.

- br_fdb_replay() should now notify of local FDB entries, but I patched
  all drivers except DSA to ignore these new entries in commit
  2c4eca3 ("net: bridge: switchdev: include local flag in FDB
  notifications"). Driver authors can lift this restriction as they
  wish, and when they do, they can also opt into the FDB replay
  functionality.

- br_mdb_replay() should fix a real issue which is described in commit
  4f2673b ("net: bridge: add helper to replay port and host-joined
  mdb entries"). However most drivers do not offload the
  SWITCHDEV_OBJ_ID_HOST_MDB to see this issue: only cpsw and am65_cpsw
  offload this switchdev object, and I don't completely understand the
  way in which they offload this switchdev object anyway. So I'll leave
  it up to these drivers' respective maintainers to opt into
  br_mdb_replay().

So most of the drivers pass NULL notifier blocks for the replay helpers,
except:
- dpaa2-switch which was already acked/regression-tested with the
  helpers enabled (and there isn't much of a downside in having them)
- ocelot which already had replay logic in "pull" mode
- DSA which already had replay logic in "pull" mode

An important observation is that the drivers which don't currently
request bridge event replays don't even have the
switchdev_bridge_port_{offload,unoffload} calls placed in proper places
right now. This was done to avoid unnecessary rework for drivers which
might never even add support for this. For driver writers who wish to
add replay support, this can be used as a tentative placement guide:
https://patchwork.kernel.org/project/netdevbpf/patch/20210720134655.892334-11-vladimir.oltean@nxp.com/

Cc: Vadym Kochan <vkochan@marvell.com>
Cc: Taras Chornyi <tchornyi@marvell.com>
Cc: Ioana Ciornei <ioana.ciornei@nxp.com>
Cc: Lars Povlsen <lars.povlsen@microchip.com>
Cc: Steen Hegelund <Steen.Hegelund@microchip.com>
Cc: UNGLinuxDriver@microchip.com
Cc: Claudiu Manoil <claudiu.manoil@nxp.com>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Acked-by: Ioana Ciornei <ioana.ciornei@nxp.com> # dpaa2-switch
Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
vladimiroltean authored and davem330 committed Jul 22, 2021
1 parent 7105b50 commit 4e51bf4
Show file tree
Hide file tree
Showing 17 changed files with 182 additions and 163 deletions.
12 changes: 10 additions & 2 deletions drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
Original file line number Diff line number Diff line change
Expand Up @@ -1889,6 +1889,9 @@ static int dpaa2_switch_port_attr_set_event(struct net_device *netdev,
return notifier_from_errno(err);
}

static struct notifier_block dpaa2_switch_port_switchdev_nb;
static struct notifier_block dpaa2_switch_port_switchdev_blocking_nb;

static int dpaa2_switch_port_bridge_join(struct net_device *netdev,
struct net_device *upper_dev,
struct netlink_ext_ack *extack)
Expand Down Expand Up @@ -1930,7 +1933,10 @@ static int dpaa2_switch_port_bridge_join(struct net_device *netdev,
if (err)
goto err_egress_flood;

err = switchdev_bridge_port_offload(netdev, netdev, extack);
err = switchdev_bridge_port_offload(netdev, netdev, NULL,
&dpaa2_switch_port_switchdev_nb,
&dpaa2_switch_port_switchdev_blocking_nb,
extack);
if (err)
goto err_switchdev_offload;

Expand Down Expand Up @@ -1964,7 +1970,9 @@ static int dpaa2_switch_port_restore_rxvlan(struct net_device *vdev, int vid, vo

static void dpaa2_switch_port_pre_bridge_leave(struct net_device *netdev)
{
switchdev_bridge_port_unoffload(netdev);
switchdev_bridge_port_unoffload(netdev, NULL,
&dpaa2_switch_port_switchdev_nb,
&dpaa2_switch_port_switchdev_blocking_nb);
}

static int dpaa2_switch_port_bridge_leave(struct net_device *netdev)
Expand Down
7 changes: 4 additions & 3 deletions drivers/net/ethernet/marvell/prestera/prestera_switchdev.c
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,8 @@ int prestera_bridge_port_join(struct net_device *br_dev,
goto err_brport_create;
}

err = switchdev_bridge_port_offload(br_port->dev, port->dev, extack);
err = switchdev_bridge_port_offload(br_port->dev, port->dev, NULL,
NULL, NULL, extack);
if (err)
goto err_switchdev_offload;

Expand All @@ -515,7 +516,7 @@ int prestera_bridge_port_join(struct net_device *br_dev,
return 0;

err_port_join:
switchdev_bridge_port_unoffload(br_port->dev);
switchdev_bridge_port_unoffload(br_port->dev, NULL, NULL, NULL);
err_switchdev_offload:
prestera_bridge_port_put(br_port);
err_brport_create:
Expand Down Expand Up @@ -591,7 +592,7 @@ void prestera_bridge_port_leave(struct net_device *br_dev,
else
prestera_bridge_1d_port_leave(br_port);

switchdev_bridge_port_unoffload(br_port->dev);
switchdev_bridge_port_unoffload(br_port->dev, NULL, NULL, NULL);

prestera_hw_port_learning_set(port, false);
prestera_hw_port_flood_set(port, BR_FLOOD | BR_MCAST_FLOOD, 0);
Expand Down
4 changes: 2 additions & 2 deletions drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ mlxsw_sp_bridge_port_create(struct mlxsw_sp_bridge_device *bridge_device,
bridge_port->ref_count = 1;

err = switchdev_bridge_port_offload(brport_dev, mlxsw_sp_port->dev,
extack);
NULL, NULL, NULL, extack);
if (err)
goto err_switchdev_offload;

Expand All @@ -377,7 +377,7 @@ mlxsw_sp_bridge_port_create(struct mlxsw_sp_bridge_device *bridge_device,
static void
mlxsw_sp_bridge_port_destroy(struct mlxsw_sp_bridge_port *bridge_port)
{
switchdev_bridge_port_unoffload(bridge_port->dev);
switchdev_bridge_port_unoffload(bridge_port->dev, NULL, NULL, NULL);
list_del(&bridge_port->list);
WARN_ON(!list_empty(&bridge_port->vlans_list));
kfree(bridge_port);
Expand Down
5 changes: 3 additions & 2 deletions drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ static int sparx5_port_bridge_join(struct sparx5_port *port,

set_bit(port->portno, sparx5->bridge_mask);

err = switchdev_bridge_port_offload(ndev, ndev, extack);
err = switchdev_bridge_port_offload(ndev, ndev, NULL, NULL, NULL,
extack);
if (err)
goto err_switchdev_offload;

Expand All @@ -133,7 +134,7 @@ static void sparx5_port_bridge_leave(struct sparx5_port *port,
{
struct sparx5 *sparx5 = port->sparx5;

switchdev_bridge_port_unoffload(port->ndev);
switchdev_bridge_port_unoffload(port->ndev, NULL, NULL, NULL);

clear_bit(port->portno, sparx5->bridge_mask);
if (bitmap_empty(sparx5->bridge_mask, SPX5_PORTS))
Expand Down
45 changes: 18 additions & 27 deletions drivers/net/ethernet/mscc/ocelot_net.c
Original file line number Diff line number Diff line change
Expand Up @@ -1154,38 +1154,19 @@ static int ocelot_switchdev_sync(struct ocelot *ocelot, int port,
struct net_device *bridge_dev,
struct netlink_ext_ack *extack)
{
struct ocelot_port *ocelot_port = ocelot->ports[port];
struct ocelot_port_private *priv;
clock_t ageing_time;
u8 stp_state;
int err;

priv = container_of(ocelot_port, struct ocelot_port_private, port);

ocelot_inherit_brport_flags(ocelot, port, brport_dev);

stp_state = br_port_get_stp_state(brport_dev);
ocelot_bridge_stp_state_set(ocelot, port, stp_state);

err = ocelot_port_vlan_filtering(ocelot, port,
br_vlan_enabled(bridge_dev));
if (err)
return err;

ageing_time = br_get_ageing_time(bridge_dev);
ocelot_port_attr_ageing_set(ocelot, port, ageing_time);

err = br_mdb_replay(bridge_dev, brport_dev, priv, true,
&ocelot_switchdev_blocking_nb, extack);
if (err && err != -EOPNOTSUPP)
return err;

err = br_vlan_replay(bridge_dev, brport_dev, priv, true,
&ocelot_switchdev_blocking_nb, extack);
if (err && err != -EOPNOTSUPP)
return err;

return 0;
return ocelot_port_vlan_filtering(ocelot, port,
br_vlan_enabled(bridge_dev));
}

static int ocelot_switchdev_unsync(struct ocelot *ocelot, int port)
Expand Down Expand Up @@ -1216,7 +1197,10 @@ static int ocelot_netdevice_bridge_join(struct net_device *dev,

ocelot_port_bridge_join(ocelot, port, bridge);

err = switchdev_bridge_port_offload(brport_dev, dev, extack);
err = switchdev_bridge_port_offload(brport_dev, dev, priv,
&ocelot_netdevice_nb,
&ocelot_switchdev_blocking_nb,
extack);
if (err)
goto err_switchdev_offload;

Expand All @@ -1227,15 +1211,22 @@ static int ocelot_netdevice_bridge_join(struct net_device *dev,
return 0;

err_switchdev_sync:
switchdev_bridge_port_unoffload(brport_dev);
switchdev_bridge_port_unoffload(brport_dev, priv,
&ocelot_netdevice_nb,
&ocelot_switchdev_blocking_nb);
err_switchdev_offload:
ocelot_port_bridge_leave(ocelot, port, bridge);
return err;
}

static void ocelot_netdevice_pre_bridge_leave(struct net_device *brport_dev)
static void ocelot_netdevice_pre_bridge_leave(struct net_device *dev,
struct net_device *brport_dev)
{
switchdev_bridge_port_unoffload(brport_dev);
struct ocelot_port_private *priv = netdev_priv(dev);

switchdev_bridge_port_unoffload(brport_dev, priv,
&ocelot_netdevice_nb,
&ocelot_switchdev_blocking_nb);
}

static int ocelot_netdevice_bridge_leave(struct net_device *dev,
Expand Down Expand Up @@ -1299,7 +1290,7 @@ static void ocelot_netdevice_pre_lag_leave(struct net_device *dev,
if (!bridge_dev || !netif_is_bridge_master(bridge_dev))
return;

ocelot_netdevice_pre_bridge_leave(bond);
ocelot_netdevice_pre_bridge_leave(dev, bond);
}

static int ocelot_netdevice_lag_leave(struct net_device *dev,
Expand Down Expand Up @@ -1384,7 +1375,7 @@ ocelot_netdevice_prechangeupper(struct net_device *dev,
struct netdev_notifier_changeupper_info *info)
{
if (netif_is_bridge_master(info->upper_dev) && !info->linking)
ocelot_netdevice_pre_bridge_leave(brport_dev);
ocelot_netdevice_pre_bridge_leave(dev, brport_dev);

if (netif_is_lag_master(info->upper_dev) && !info->linking)
ocelot_netdevice_pre_lag_leave(dev, info->upper_dev);
Expand Down
5 changes: 3 additions & 2 deletions drivers/net/ethernet/rocker/rocker_ofdpa.c
Original file line number Diff line number Diff line change
Expand Up @@ -2598,15 +2598,16 @@ static int ofdpa_port_bridge_join(struct ofdpa_port *ofdpa_port,
if (err)
return err;

return switchdev_bridge_port_offload(dev, dev, extack);
return switchdev_bridge_port_offload(dev, dev, NULL, NULL, NULL,
extack);
}

static int ofdpa_port_bridge_leave(struct ofdpa_port *ofdpa_port)
{
struct net_device *dev = ofdpa_port->dev;
int err;

switchdev_bridge_port_unoffload(dev);
switchdev_bridge_port_unoffload(dev, NULL, NULL, NULL);

err = ofdpa_port_vlan_del(ofdpa_port, OFDPA_UNTAGGED_VID, 0);
if (err)
Expand Down
5 changes: 3 additions & 2 deletions drivers/net/ethernet/ti/am65-cpsw-nuss.c
Original file line number Diff line number Diff line change
Expand Up @@ -2096,7 +2096,8 @@ static int am65_cpsw_netdevice_port_link(struct net_device *ndev,
return -EOPNOTSUPP;
}

err = switchdev_bridge_port_offload(ndev, ndev, extack);
err = switchdev_bridge_port_offload(ndev, ndev, NULL, NULL, NULL,
extack);
if (err)
return err;

Expand All @@ -2112,7 +2113,7 @@ static void am65_cpsw_netdevice_port_unlink(struct net_device *ndev)
struct am65_cpsw_common *common = am65_ndev_to_common(ndev);
struct am65_cpsw_ndev_priv *priv = am65_ndev_to_priv(ndev);

switchdev_bridge_port_unoffload(ndev);
switchdev_bridge_port_unoffload(ndev, NULL, NULL, NULL);

common->br_members &= ~BIT(priv->port->port_id);

Expand Down
5 changes: 3 additions & 2 deletions drivers/net/ethernet/ti/cpsw_new.c
Original file line number Diff line number Diff line change
Expand Up @@ -1517,7 +1517,8 @@ static int cpsw_netdevice_port_link(struct net_device *ndev,
return -EOPNOTSUPP;
}

err = switchdev_bridge_port_offload(ndev, ndev, extack);
err = switchdev_bridge_port_offload(ndev, ndev, NULL, NULL, NULL,
extack);
if (err)
return err;

Expand All @@ -1533,7 +1534,7 @@ static void cpsw_netdevice_port_unlink(struct net_device *ndev)
struct cpsw_priv *priv = netdev_priv(ndev);
struct cpsw_common *cpsw = priv->cpsw;

switchdev_bridge_port_unoffload(ndev);
switchdev_bridge_port_unoffload(ndev, NULL, NULL, NULL);

cpsw->br_members &= ~BIT(priv->emac_port);

Expand Down
54 changes: 18 additions & 36 deletions include/linux/if_bridge.h
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,6 @@ bool br_multicast_has_querier_adjacent(struct net_device *dev, int proto);
bool br_multicast_has_router_adjacent(struct net_device *dev, int proto);
bool br_multicast_enabled(const struct net_device *dev);
bool br_multicast_router(const struct net_device *dev);
int br_mdb_replay(struct net_device *br_dev, struct net_device *dev,
const void *ctx, bool adding, struct notifier_block *nb,
struct netlink_ext_ack *extack);
#else
static inline int br_multicast_list_adjacent(struct net_device *dev,
struct list_head *br_ip_list)
Expand Down Expand Up @@ -104,13 +101,6 @@ static inline bool br_multicast_router(const struct net_device *dev)
{
return false;
}
static inline int br_mdb_replay(const struct net_device *br_dev,
const struct net_device *dev, const void *ctx,
bool adding, struct notifier_block *nb,
struct netlink_ext_ack *extack)
{
return -EOPNOTSUPP;
}
#endif

#if IS_ENABLED(CONFIG_BRIDGE) && IS_ENABLED(CONFIG_BRIDGE_VLAN_FILTERING)
Expand All @@ -120,9 +110,6 @@ int br_vlan_get_pvid_rcu(const struct net_device *dev, u16 *p_pvid);
int br_vlan_get_proto(const struct net_device *dev, u16 *p_proto);
int br_vlan_get_info(const struct net_device *dev, u16 vid,
struct bridge_vlan_info *p_vinfo);
int br_vlan_replay(struct net_device *br_dev, struct net_device *dev,
const void *ctx, bool adding, struct notifier_block *nb,
struct netlink_ext_ack *extack);
#else
static inline bool br_vlan_enabled(const struct net_device *dev)
{
Expand All @@ -149,14 +136,6 @@ static inline int br_vlan_get_info(const struct net_device *dev, u16 vid,
{
return -EINVAL;
}

static inline int br_vlan_replay(struct net_device *br_dev,
struct net_device *dev, const void *ctx,
bool adding, struct notifier_block *nb,
struct netlink_ext_ack *extack)
{
return -EOPNOTSUPP;
}
#endif

#if IS_ENABLED(CONFIG_BRIDGE)
Expand All @@ -167,8 +146,6 @@ void br_fdb_clear_offload(const struct net_device *dev, u16 vid);
bool br_port_flag_is_set(const struct net_device *dev, unsigned long flag);
u8 br_port_get_stp_state(const struct net_device *dev);
clock_t br_get_ageing_time(const struct net_device *br_dev);
int br_fdb_replay(const struct net_device *br_dev, const struct net_device *dev,
const void *ctx, bool adding, struct notifier_block *nb);
#else
static inline struct net_device *
br_fdb_find_port(const struct net_device *br_dev,
Expand Down Expand Up @@ -197,32 +174,37 @@ static inline clock_t br_get_ageing_time(const struct net_device *br_dev)
{
return 0;
}

static inline int br_fdb_replay(const struct net_device *br_dev,
const struct net_device *dev, const void *ctx,
bool adding, struct notifier_block *nb)
{
return -EOPNOTSUPP;
}
#endif

#if IS_ENABLED(CONFIG_BRIDGE) && IS_ENABLED(CONFIG_NET_SWITCHDEV)

int switchdev_bridge_port_offload(struct net_device *brport_dev,
struct net_device *dev,
struct net_device *dev, const void *ctx,
struct notifier_block *atomic_nb,
struct notifier_block *blocking_nb,
struct netlink_ext_ack *extack);
void switchdev_bridge_port_unoffload(struct net_device *brport_dev);
void switchdev_bridge_port_unoffload(struct net_device *brport_dev,
const void *ctx,
struct notifier_block *atomic_nb,
struct notifier_block *blocking_nb);

#else

static inline int switchdev_bridge_port_offload(struct net_device *brport_dev,
struct net_device *dev,
struct netlink_ext_ack *extack)
static inline int
switchdev_bridge_port_offload(struct net_device *brport_dev,
struct net_device *dev, const void *ctx,
struct notifier_block *atomic_nb,
struct notifier_block *blocking_nb,
struct netlink_ext_ack *extack)
{
return -EINVAL;
}

static inline void switchdev_bridge_port_unoffload(struct net_device *brport_dev)
static inline void
switchdev_bridge_port_unoffload(struct net_device *brport_dev,
const void *ctx,
struct notifier_block *atomic_nb,
struct notifier_block *blocking_nb)
{
}
#endif
Expand Down
1 change: 0 additions & 1 deletion net/bridge/br_fdb.c
Original file line number Diff line number Diff line change
Expand Up @@ -795,7 +795,6 @@ int br_fdb_replay(const struct net_device *br_dev, const struct net_device *dev,

return err;
}
EXPORT_SYMBOL_GPL(br_fdb_replay);

static void fdb_notify(struct net_bridge *br,
const struct net_bridge_fdb_entry *fdb, int type,
Expand Down
1 change: 0 additions & 1 deletion net/bridge/br_mdb.c
Original file line number Diff line number Diff line change
Expand Up @@ -689,7 +689,6 @@ int br_mdb_replay(struct net_device *br_dev, struct net_device *dev,

return err;
}
EXPORT_SYMBOL_GPL(br_mdb_replay);

static void br_mdb_switchdev_host_port(struct net_device *dev,
struct net_device *lower_dev,
Expand Down
Loading

0 comments on commit 4e51bf4

Please sign in to comment.