From f9cef64fa23f6c1ff177be5082113c5a94e34e5d Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Wed, 2 Mar 2022 21:14:13 +0200 Subject: net: dsa: felix: migrate host FDB and MDB entries when changing tag proto The "ocelot" and "ocelot-8021q" tagging protocols make use of different hardware resources, and host FDB entries have different destination ports in the switch analyzer module, practically speaking. So when the user requests a tagging protocol change, the driver must migrate all host FDB and MDB entries from the NPI port (in fact CPU port module) towards the same physical port, but this time used as a regular port. It is pointless for the felix driver to keep a copy of the host addresses, when we can create and export DSA helpers for walking through the addresses that it already needs to keep on the CPU port, for refcounting purposes. felix_classify_db() is moved up to avoid a forward declaration. We pass "bool change" because dp->fdbs and dp->mdbs are uninitialized lists when felix_setup() first calls felix_set_tag_protocol(), so we need to avoid calling dsa_port_walk_fdbs() during probe time. Signed-off-by: Vladimir Oltean Signed-off-by: David S. Miller --- net/dsa/dsa.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) (limited to 'net/dsa/dsa.c') diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index c43f7446a75d..06d5de28a43e 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -467,6 +467,46 @@ struct dsa_port *dsa_port_from_netdev(struct net_device *netdev) } EXPORT_SYMBOL_GPL(dsa_port_from_netdev); +int dsa_port_walk_fdbs(struct dsa_switch *ds, int port, dsa_fdb_walk_cb_t cb) +{ + struct dsa_port *dp = dsa_to_port(ds, port); + struct dsa_mac_addr *a; + int err; + + mutex_lock(&dp->addr_lists_lock); + + list_for_each_entry(a, &dp->fdbs, list) { + err = cb(ds, port, a->addr, a->vid, a->db); + if (err) + break; + } + + mutex_unlock(&dp->addr_lists_lock); + + return err; +} +EXPORT_SYMBOL_GPL(dsa_port_walk_fdbs); + +int dsa_port_walk_mdbs(struct dsa_switch *ds, int port, dsa_fdb_walk_cb_t cb) +{ + struct dsa_port *dp = dsa_to_port(ds, port); + struct dsa_mac_addr *a; + int err; + + mutex_lock(&dp->addr_lists_lock); + + list_for_each_entry(a, &dp->mdbs, list) { + err = cb(ds, port, a->addr, a->vid, a->db); + if (err) + break; + } + + mutex_unlock(&dp->addr_lists_lock); + + return err; +} +EXPORT_SYMBOL_GPL(dsa_port_walk_mdbs); + static int __init dsa_init_module(void) { int rc; -- cgit v1.2.3-59-g8ed1b From cd5169841c49dab75ecc3b20b3bd1100395b6909 Mon Sep 17 00:00:00 2001 From: Tom Rix Date: Sat, 5 Mar 2022 09:14:48 -0800 Subject: net: dsa: return success if there was nothing to do Clang static analysis reports this representative issue dsa.c:486:2: warning: Undefined or garbage value returned to caller return err; ^~~~~~~~~~ err is only set in the loop. If the loop is empty, garbage will be returned. So initialize err to 0 to handle this noop case. Signed-off-by: Tom Rix Reviewed-by: Andrew Lunn Reviewed-by: Vladimir Oltean Signed-off-by: David S. Miller --- net/dsa/dsa.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'net/dsa/dsa.c') diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index 06d5de28a43e..fe971a2c15cd 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -471,7 +471,7 @@ int dsa_port_walk_fdbs(struct dsa_switch *ds, int port, dsa_fdb_walk_cb_t cb) { struct dsa_port *dp = dsa_to_port(ds, port); struct dsa_mac_addr *a; - int err; + int err = 0; mutex_lock(&dp->addr_lists_lock); @@ -491,7 +491,7 @@ int dsa_port_walk_mdbs(struct dsa_switch *ds, int port, dsa_fdb_walk_cb_t cb) { struct dsa_port *dp = dsa_to_port(ds, port); struct dsa_mac_addr *a; - int err; + int err = 0; mutex_lock(&dp->addr_lists_lock); -- cgit v1.2.3-59-g8ed1b From 7e580490ac9819dd55a36be2a9b3380d1391f91b Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Tue, 8 Mar 2022 11:15:15 +0200 Subject: net: dsa: felix: avoid early deletion of host FDB entries The Felix driver declares FDB isolation but puts all standalone ports in VID 0. This is mostly problem-free as discussed with Alvin here: https://patchwork.kernel.org/project/netdevbpf/cover/20220302191417.1288145-1-vladimir.oltean@nxp.com/#24763870 however there is one catch. DSA still thinks that FDB entries are installed on the CPU port as many times as there are user ports, and this is problematic when multiple user ports share the same MAC address. Consider the default case where all user ports inherit their MAC address from the DSA master, and then the user runs: ip link set swp0 address 00:01:02:03:04:05 The above will make dsa_slave_set_mac_address() call dsa_port_standalone_host_fdb_add() for 00:01:02:03:04:05 in port 0's standalone database, and dsa_port_standalone_host_fdb_del() for the old address of swp0, again in swp0's standalone database. Both the ->port_fdb_add() and ->port_fdb_del() will be propagated down to the felix driver, which will end up deleting the old MAC address from the CPU port. But this is still in use by other user ports, so we end up breaking unicast termination for them. There isn't a problem in the fact that DSA keeps track of host standalone addresses in the individual database of each user port: some drivers like sja1105 need this. There also isn't a problem in the fact that some drivers choose the same VID/FID for all standalone ports. It is just that the deletion of these host addresses must be delayed until they are known to not be in use any longer, and only the driver has this knowledge. Since DSA keeps these addresses in &cpu_dp->fdbs and &cpu_db->mdbs, it is just a matter of walking over those lists and see whether the same MAC address is present on the CPU port in the port db of another user port. I have considered reusing the generic dsa_port_walk_fdbs() and dsa_port_walk_mdbs() schemes for this, but locking makes it difficult. In the ->port_fdb_add() method and co, &dp->addr_lists_lock is held, but dsa_port_walk_fdbs() also acquires that lock. Also, even assuming that we introduce an unlocked variant of the address iterator, we'd still need some relatively complex data structures, and a void *ctx in the dsa_fdb_walk_cb_t which we don't currently pass, such that drivers are able to figure out, after iterating, whether the same MAC address is or isn't present in the port db of another port. All the above, plus the fact that I expect other drivers to follow the same model as felix where all standalone ports use the same FID, made me conclude that a generic method provided by DSA is necessary: dsa_fdb_present_in_other_db() and the mdb equivalent. Felix calls this from the ->port_fdb_del() handler for the CPU port, when the database was classified to either a port db, or a LAG db. For symmetry, we also call this from ->port_fdb_add(), because if the address was installed once, then installing it a second time serves no purpose: it's already in hardware in VID 0 and it affects all standalone ports. This change moves dsa_db_equal() from switch.c to dsa.c, since it now has one more caller. Fixes: 54c319846086 ("net: mscc: ocelot: enforce FDB isolation when VLAN-unaware") Signed-off-by: Vladimir Oltean Signed-off-by: David S. Miller --- drivers/net/dsa/ocelot/felix.c | 16 +++++++++++ include/net/dsa.h | 6 +++++ net/dsa/dsa.c | 60 ++++++++++++++++++++++++++++++++++++++++++ net/dsa/dsa_priv.h | 2 ++ net/dsa/switch.c | 18 ------------- 5 files changed, 84 insertions(+), 18 deletions(-) (limited to 'net/dsa/dsa.c') diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index e475186b70c7..35b436a491e1 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -739,6 +739,10 @@ static int felix_fdb_add(struct dsa_switch *ds, int port, if (IS_ERR(bridge_dev)) return PTR_ERR(bridge_dev); + if (dsa_is_cpu_port(ds, port) && !bridge_dev && + dsa_fdb_present_in_other_db(ds, port, addr, vid, db)) + return 0; + return ocelot_fdb_add(ocelot, port, addr, vid, bridge_dev); } @@ -752,6 +756,10 @@ static int felix_fdb_del(struct dsa_switch *ds, int port, if (IS_ERR(bridge_dev)) return PTR_ERR(bridge_dev); + if (dsa_is_cpu_port(ds, port) && !bridge_dev && + dsa_fdb_present_in_other_db(ds, port, addr, vid, db)) + return 0; + return ocelot_fdb_del(ocelot, port, addr, vid, bridge_dev); } @@ -791,6 +799,10 @@ static int felix_mdb_add(struct dsa_switch *ds, int port, if (IS_ERR(bridge_dev)) return PTR_ERR(bridge_dev); + if (dsa_is_cpu_port(ds, port) && !bridge_dev && + dsa_mdb_present_in_other_db(ds, port, mdb, db)) + return 0; + return ocelot_port_mdb_add(ocelot, port, mdb, bridge_dev); } @@ -804,6 +816,10 @@ static int felix_mdb_del(struct dsa_switch *ds, int port, if (IS_ERR(bridge_dev)) return PTR_ERR(bridge_dev); + if (dsa_is_cpu_port(ds, port) && !bridge_dev && + dsa_mdb_present_in_other_db(ds, port, mdb, db)) + return 0; + return ocelot_port_mdb_del(ocelot, port, mdb, bridge_dev); } diff --git a/include/net/dsa.h b/include/net/dsa.h index 759479fe8573..9d16505fc0e2 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -1227,6 +1227,12 @@ typedef int dsa_fdb_walk_cb_t(struct dsa_switch *ds, int port, int dsa_port_walk_fdbs(struct dsa_switch *ds, int port, dsa_fdb_walk_cb_t cb); int dsa_port_walk_mdbs(struct dsa_switch *ds, int port, dsa_fdb_walk_cb_t cb); +bool dsa_fdb_present_in_other_db(struct dsa_switch *ds, int port, + const unsigned char *addr, u16 vid, + struct dsa_db db); +bool dsa_mdb_present_in_other_db(struct dsa_switch *ds, int port, + const struct switchdev_obj_port_mdb *mdb, + struct dsa_db db); /* Keep inline for faster access in hot path */ static inline bool netdev_uses_dsa(const struct net_device *dev) diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index fe971a2c15cd..89c6c86e746f 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -507,6 +507,66 @@ int dsa_port_walk_mdbs(struct dsa_switch *ds, int port, dsa_fdb_walk_cb_t cb) } EXPORT_SYMBOL_GPL(dsa_port_walk_mdbs); +bool dsa_db_equal(const struct dsa_db *a, const struct dsa_db *b) +{ + if (a->type != b->type) + return false; + + switch (a->type) { + case DSA_DB_PORT: + return a->dp == b->dp; + case DSA_DB_LAG: + return a->lag.dev == b->lag.dev; + case DSA_DB_BRIDGE: + return a->bridge.num == b->bridge.num; + default: + WARN_ON(1); + return false; + } +} + +bool dsa_fdb_present_in_other_db(struct dsa_switch *ds, int port, + const unsigned char *addr, u16 vid, + struct dsa_db db) +{ + struct dsa_port *dp = dsa_to_port(ds, port); + struct dsa_mac_addr *a; + + lockdep_assert_held(&dp->addr_lists_lock); + + list_for_each_entry(a, &dp->fdbs, list) { + if (!ether_addr_equal(a->addr, addr) || a->vid != vid) + continue; + + if (a->db.type == db.type && !dsa_db_equal(&a->db, &db)) + return true; + } + + return false; +} +EXPORT_SYMBOL_GPL(dsa_fdb_present_in_other_db); + +bool dsa_mdb_present_in_other_db(struct dsa_switch *ds, int port, + const struct switchdev_obj_port_mdb *mdb, + struct dsa_db db) +{ + struct dsa_port *dp = dsa_to_port(ds, port); + struct dsa_mac_addr *a; + + lockdep_assert_held(&dp->addr_lists_lock); + + list_for_each_entry(a, &dp->mdbs, list) { + if (!ether_addr_equal(a->addr, mdb->addr) || a->vid != mdb->vid) + continue; + + if (a->db.type == db.type && !dsa_db_equal(&a->db, &db)) + return true; + } + + return false; +} +EXPORT_SYMBOL_GPL(dsa_mdb_present_in_other_db); + static int __init dsa_init_module(void) { int rc; diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index c3c7491ace72..f20bdd8ea0a8 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -182,6 +182,8 @@ const struct dsa_device_ops *dsa_tag_driver_get(int tag_protocol); void dsa_tag_driver_put(const struct dsa_device_ops *ops); const struct dsa_device_ops *dsa_find_tagger_by_name(const char *buf); +bool dsa_db_equal(const struct dsa_db *a, const struct dsa_db *b); + bool dsa_schedule_work(struct work_struct *work); const char *dsa_tag_protocol_to_str(const struct dsa_device_ops *ops); diff --git a/net/dsa/switch.c b/net/dsa/switch.c index 327d66bf7b47..d25cd1da3eb3 100644 --- a/net/dsa/switch.c +++ b/net/dsa/switch.c @@ -212,24 +212,6 @@ static bool dsa_port_host_address_match(struct dsa_port *dp, return false; } -static bool dsa_db_equal(const struct dsa_db *a, const struct dsa_db *b) -{ - if (a->type != b->type) - return false; - - switch (a->type) { - case DSA_DB_PORT: - return a->dp == b->dp; - case DSA_DB_LAG: - return a->lag.dev == b->lag.dev; - case DSA_DB_BRIDGE: - return a->bridge.num == b->bridge.num; - default: - WARN_ON(1); - return false; - } -} - static struct dsa_mac_addr *dsa_mac_addr_find(struct list_head *addr_list, const unsigned char *addr, u16 vid, struct dsa_db db) -- cgit v1.2.3-59-g8ed1b