From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-06-28 22:00:49
From: Vladimir Oltean <vladimir.oltean@nxp.com>
This is my fourth stab (identical to the third one except sent as
non-RFC) at creating a list of unicast and multicast addresses that the
DSA CPU ports must trap. I am reusing a lot of Tobias's work which he
submitted here:
https://patchwork.kernel.org/project/netdevbpf/cover/20210116012515.3152-1-tobias@waldekranz.com/
My additions to Tobias' work come in the form of taking some care that
additions and removals of host addresses are properly balanced, so that
we can do reference counting on them for cross-chip setups and multiple
bridges spanning the same switch (I am working on an NXP board where
both are real requirements).
During the last attempted submission of multiple CPU ports for DSA:
https://patchwork.kernel.org/project/netdevbpf/cover/20210410133454.4768-1-ansuelsmth@gmail.com/
it became clear that the concept of multiple CPU ports would not be
compatible with the idea of address learning on those CPU ports (when
those CPU ports are statically assigned to user ports, not in a LAG)
unless the switch supports complete FDB isolation, which most switches
do not. So DSA needs to manage in software all addresses that are
installed on the CPU port(s), which is what this patch set does.
Compared to all earlier attempts, this series does not fiddle with how
DSA operates the ports in standalone mode at all, just when bridged.
We need to sort that out properly, then any optimization that comes in
standalone mode (i.e. IFF_UNICAST_FLT) can come later.
Tobias Waldekranz (3):
net: bridge: switchdev: send FDB notifications for host addresses
net: dsa: sync static FDB entries on foreign interfaces to hardware
net: dsa: include bridge addresses which are local in the host fdb
list
Vladimir Oltean (11):
net: bridge: allow br_fdb_replay to be called for the bridge device
net: dsa: delete dsa_legacy_fdb_add and dsa_legacy_fdb_del
net: dsa: introduce dsa_is_upstream_port and dsa_switch_is_upstream_of
net: dsa: introduce a separate cross-chip notifier type for host MDBs
net: dsa: reference count the MDB entries at the cross-chip notifier
level
net: dsa: introduce a separate cross-chip notifier type for host FDBs
net: dsa: reference count the FDB addresses at the cross-chip notifier
level
net: dsa: install the host MDB and FDB entries in the master's RX
filter
net: dsa: include fdb entries pointing to bridge in the host fdb list
net: dsa: ensure during dsa_fdb_offload_notify that dev_hold and
dev_put are on the same dev
net: dsa: replay the local bridge FDB entries pointing to the bridge
dev too
include/net/dsa.h | 39 ++++++
net/bridge/br_fdb.c | 7 +-
net/bridge/br_private.h | 7 +-
net/bridge/br_switchdev.c | 11 +-
net/dsa/dsa2.c | 14 ++
net/dsa/dsa_priv.h | 14 ++
net/dsa/port.c | 86 ++++++++++++
net/dsa/slave.c | 102 +++++++-------
net/dsa/switch.c | 276 +++++++++++++++++++++++++++++++++++++-
9 files changed, 488 insertions(+), 68 deletions(-)
--
2.25.1
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-06-28 22:00:59
From: Vladimir Oltean <vladimir.oltean@nxp.com>
We want to add reference counting for FDB entries in cross-chip
topologies, and in order for that to have any chance of working and not
be unbalanced (leading to entries which are never deleted), we need to
ensure that higher layers are sane, because if they aren't, it's garbage
in, garbage out.
For example, if we add a bridge FDB entry twice, the bridge properly
errors out:
$ bridge fdb add dev swp0 00:01:02:03:04:07 master static
$ bridge fdb add dev swp0 00:01:02:03:04:07 master static
RTNETLINK answers: File exists
However, the same thing cannot be said about the bridge bypass
operations:
$ bridge fdb add dev swp0 00:01:02:03:04:07
$ bridge fdb add dev swp0 00:01:02:03:04:07
$ bridge fdb add dev swp0 00:01:02:03:04:07
$ bridge fdb add dev swp0 00:01:02:03:04:07
$ echo $?
0
But one 'bridge fdb del' is enough to remove the entry, no matter how
many times it was added.
The bridge bypass operations are impossible to maintain in these
circumstances and lack of support for reference counting the cross-chip
notifiers is holding us back from making further progress, so just drop
support for them. The only way left for users to install static bridge
FDB entries is the proper one, using the "master static" flags.
With this change, rtnl_fdb_add() falls back to calling
ndo_dflt_fdb_add() which uses the duplicate-exclusive variant of
dev_uc_add(): dev_uc_add_excl(). Because DSA does not (yet) declare
IFF_UNICAST_FLT, this results in us going to promiscuous mode:
$ bridge fdb add dev swp0 00:01:02:03:04:05
[ 28.206743] device swp0 entered promiscuous mode
$ bridge fdb add dev swp0 00:01:02:03:04:05
RTNETLINK answers: File exists
So even if it does not completely fail, there is at least some indication
that it is behaving differently from before, and closer to user space
expectations, I would argue (the lack of a "local|static" specifier
defaults to "local", or "host-only", so dev_uc_add() is a reasonable
default implementation). If the generic implementation of .ndo_fdb_add
provided by Vlad Yasevich is a proof of anything, it only proves that
the implementation provided by DSA was always wrong, by not looking at
"ndm->ndm_state & NUD_NOARP" (the "static" flag which means that the FDB
entry points outwards) and "ndm->ndm_state & NUD_PERMANENT" (the "local"
flag which means that the FDB entry points towards the host). It all
used to mean the same thing to DSA.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/slave.c | 23 -----------------------
1 file changed, 23 deletions(-)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-06-28 22:01:01
From: Tobias Waldekranz <tobias@waldekranz.com>
Treat addresses added to the bridge itself in the same way as regular
ports and send out a notification so that drivers may sync it down to
the hardware FDB.
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/bridge/br_fdb.c | 4 ++--
net/bridge/br_private.h | 7 ++++---
net/bridge/br_switchdev.c | 11 +++++------
3 files changed, 11 insertions(+), 11 deletions(-)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-06-28 22:01:05
From: Vladimir Oltean <vladimir.oltean@nxp.com>
When a port joins a bridge which already has local FDB entries pointing
to the bridge device itself, we would like to offload those, so allow
the "dev" argument to be equal to the bridge too. The code already does
what we need in that case.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/bridge/br_fdb.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-06-28 22:01:06
From: Vladimir Oltean <vladimir.oltean@nxp.com>
In preparation for the new cross-chip notifiers for host addresses,
let's introduce some more topology helpers which we are going to use to
discern switches that are in our path towards the dedicated CPU port
from switches that aren't.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
include/net/dsa.h | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
@@ -491,6 +491,32 @@ static inline unsigned int dsa_upstream_port(struct dsa_switch *ds, int port)returndsa_towards_port(ds,cpu_dp->ds->index,cpu_dp->index);}+/* Return true if this is the local port used to reach the CPU port */+staticinlinebooldsa_is_upstream_port(structdsa_switch*ds,intport)+{+if(dsa_is_unused_port(ds,port))+returnfalse;++returnport==dsa_upstream_port(ds,port);+}++/* Return true if @upstream_ds is an upstream switch of @downstream_ds, meaning+*thattheroutingportfrom@downstream_dsto@upstream_dsisalsotheport+*which@downstream_dsusestoreachitsdedicatedCPU.+*/+staticinlinebooldsa_switch_is_upstream_of(structdsa_switch*upstream_ds,+structdsa_switch*downstream_ds)+{+introuting_port;++if(upstream_ds==downstream_ds)+returntrue;++routing_port=dsa_routing_port(downstream_ds,upstream_ds->index);++returndsa_is_upstream_port(downstream_ds,routing_port);+}+staticinlinebooldsa_port_is_vlan_filtering(conststructdsa_port*dp){conststructdsa_switch*ds=dp->ds;
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-06-28 22:01:08
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Commit abd49535c380 ("net: dsa: execute dsa_switch_mdb_add only for
routing port in cross-chip topologies") does a surprisingly good job
even for the SWITCHDEV_OBJ_ID_HOST_MDB use case, where DSA simply
translates a switchdev object received on dp into a cross-chip notifier
for dp->cpu_dp.
To visualize how that works, imagine the daisy chain topology below and
consider a SWITCHDEV_OBJ_ID_HOST_MDB object emitted on sw2p0. How does
the cross-chip notifier know to match on all the right ports (sw0p4, the
dedicated CPU port, sw1p4, an upstream DSA link, and sw2p4, another
upstream DSA link)?
|
sw0p0 sw0p1 sw0p2 sw0p3 sw0p4
[ user ] [ user ] [ user ] [ dsa ] [ cpu ]
[ ] [ ] [ ] [ ] [ x ]
|
+---------+
|
sw1p0 sw1p1 sw1p2 sw1p3 sw1p4
[ user ] [ user ] [ user ] [ dsa ] [ dsa ]
[ ] [ ] [ ] [ ] [ x ]
|
+---------+
|
sw2p0 sw2p1 sw2p2 sw2p3 sw2p4
[ user ] [ user ] [ user ] [ user ] [ dsa ]
[ ] [ ] [ ] [ ] [ x ]
The answer is simple: the dedicated CPU port of sw2p0 is sw0p4, and
dsa_routing_port returns the upstream port for all switches.
That is fine, but there are other topologies where this does not work as
well. There are trees with "H" topologies in the wild, where there are 2
or more switches with DSA links between them, but every switch has its
dedicated CPU port. For these topologies, it seems stupid for the neighbor
switches to install an MDB entry on the routing port, since these
multicast addresses are fundamentally different than the usual ones we
support (and that is the justification for this patch, to introduce the
concept of a termination plane multicast MAC address, as opposed to a
forwarding plane multicast MAC address).
For example, when a SWITCHDEV_OBJ_ID_HOST_MDB would get added to sw0p0,
without this patch, it would get treated as a regular port MDB on sw0p2
and it would match on the ports below (including the sw1p3 routing port).
| |
sw0p0 sw0p1 sw0p2 sw0p3 sw1p3 sw1p2 sw1p1 sw1p0
[ user ] [ user ] [ cpu ] [ dsa ] [ dsa ] [ cpu ] [ user ] [ user ]
[ ] [ ] [ x ] [ ] ---- [ x ] [ ] [ ] [ ]
With the patch, the host MDB notifier on sw0p0 matches only on the local
switch, which is what we want for a termination plane address.
| |
sw0p0 sw0p1 sw0p2 sw0p3 sw1p3 sw1p2 sw1p1 sw1p0
[ user ] [ user ] [ cpu ] [ dsa ] [ dsa ] [ cpu ] [ user ] [ user ]
[ ] [ ] [ x ] [ ] ---- [ ] [ ] [ ] [ ]
Name this new matching function "dsa_switch_host_address_match" since we
will be reusing it soon for host FDB entries as well.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/dsa_priv.h | 6 +++++
net/dsa/port.c | 24 ++++++++++++++++++
net/dsa/slave.c | 10 ++------
net/dsa/switch.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 95 insertions(+), 8 deletions(-)
@@ -418,10 +418,7 @@ static int dsa_slave_port_obj_add(struct net_device *dev, const void *ctx,if(!dsa_port_offloads_bridge(dp,obj->orig_dev))return-EOPNOTSUPP;-/* DSA can directly translate this to a normal MDB add,-*butontheCPUport.-*/-err=dsa_port_mdb_add(dp->cpu_dp,SWITCHDEV_OBJ_PORT_MDB(obj));+err=dsa_port_host_mdb_add(dp,SWITCHDEV_OBJ_PORT_MDB(obj));break;caseSWITCHDEV_OBJ_ID_PORT_VLAN:if(!dsa_port_offloads_bridge_port(dp,obj->orig_dev))
@@ -495,10 +492,7 @@ static int dsa_slave_port_obj_del(struct net_device *dev, const void *ctx,if(!dsa_port_offloads_bridge(dp,obj->orig_dev))return-EOPNOTSUPP;-/* DSA can directly translate this to a normal MDB add,-*butontheCPUport.-*/-err=dsa_port_mdb_del(dp->cpu_dp,SWITCHDEV_OBJ_PORT_MDB(obj));+err=dsa_port_host_mdb_del(dp,SWITCHDEV_OBJ_PORT_MDB(obj));break;caseSWITCHDEV_OBJ_ID_PORT_VLAN:if(!dsa_port_offloads_bridge_port(dp,obj->orig_dev))
@@ -154,6 +154,30 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,return0;}+/* Matches for all upstream-facing ports (the CPU port and all upstream-facing+*DSAlinks)thatsitbetweenthetargetedportonwhichthenotifierwas+*emittedanditsdedicatedCPUport.+*/+staticbooldsa_switch_host_address_match(structdsa_switch*ds,intport,+intinfo_sw_index,intinfo_port)+{+structdsa_port*targeted_dp,*cpu_dp;+structdsa_switch*targeted_ds;++targeted_ds=dsa_switch_find(ds->dst->index,info_sw_index);+if(WARN_ON(!targeted_ds))+returnfalse;++targeted_dp=dsa_to_port(targeted_ds,info_port);+cpu_dp=targeted_dp->cpu_dp;++if(dsa_switch_is_upstream_of(ds,targeted_ds))+returnport==dsa_towards_port(ds,cpu_dp->ds->index,+cpu_dp->index);++returnfalse;+}+staticintdsa_switch_fdb_add(structdsa_switch*ds,structdsa_notifier_fdb_info*info){
@@ -258,6 +282,39 @@ static int dsa_switch_mdb_del(struct dsa_switch *ds,return0;}+staticintdsa_switch_host_mdb_add(structdsa_switch*ds,+structdsa_notifier_mdb_info*info)+{+interr=0;+intport;++if(!ds->ops->port_mdb_add)+return-EOPNOTSUPP;++for(port=0;port<ds->num_ports;port++){+if(dsa_switch_host_address_match(ds,port,info->sw_index,+info->port)){+err=ds->ops->port_mdb_add(ds,port,info->mdb);+if(err)+break;+}+}++returnerr;+}++staticintdsa_switch_host_mdb_del(structdsa_switch*ds,+structdsa_notifier_mdb_info*info)+{+if(!ds->ops->port_mdb_del)+return-EOPNOTSUPP;++if(ds->index==info->sw_index)+returnds->ops->port_mdb_del(ds,info->port,info->mdb);++return0;+}+staticbooldsa_switch_vlan_match(structdsa_switch*ds,intport,structdsa_notifier_vlan_info*info){
@@ -441,6 +498,12 @@ static int dsa_switch_event(struct notifier_block *nb,caseDSA_NOTIFIER_MDB_DEL:err=dsa_switch_mdb_del(ds,info);break;+caseDSA_NOTIFIER_HOST_MDB_ADD:+err=dsa_switch_host_mdb_add(ds,info);+break;+caseDSA_NOTIFIER_HOST_MDB_DEL:+err=dsa_switch_host_mdb_del(ds,info);+break;caseDSA_NOTIFIER_VLAN_ADD:err=dsa_switch_vlan_add(ds,info);break;
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-06-28 22:01:13
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Ever since the cross-chip notifiers were introduced, the design was
meant to be simplistic and just get the job done without worrying too
much about dangling resources left behind.
For example, somebody installs an MDB entry on sw0p0 in this daisy chain
topology. It gets installed using ds->ops->port_mdb_add() on sw0p0,
sw1p4 and sw2p4.
|
sw0p0 sw0p1 sw0p2 sw0p3 sw0p4
[ user ] [ user ] [ user ] [ dsa ] [ cpu ]
[ x ] [ ] [ ] [ ] [ ]
|
+---------+
|
sw1p0 sw1p1 sw1p2 sw1p3 sw1p4
[ user ] [ user ] [ user ] [ dsa ] [ dsa ]
[ ] [ ] [ ] [ ] [ x ]
|
+---------+
|
sw2p0 sw2p1 sw2p2 sw2p3 sw2p4
[ user ] [ user ] [ user ] [ user ] [ dsa ]
[ ] [ ] [ ] [ ] [ x ]
Then the same person deletes that MDB entry. The cross-chip notifier for
deletion only matches sw0p0:
|
sw0p0 sw0p1 sw0p2 sw0p3 sw0p4
[ user ] [ user ] [ user ] [ dsa ] [ cpu ]
[ x ] [ ] [ ] [ ] [ ]
|
+---------+
|
sw1p0 sw1p1 sw1p2 sw1p3 sw1p4
[ user ] [ user ] [ user ] [ dsa ] [ dsa ]
[ ] [ ] [ ] [ ] [ ]
|
+---------+
|
sw2p0 sw2p1 sw2p2 sw2p3 sw2p4
[ user ] [ user ] [ user ] [ user ] [ dsa ]
[ ] [ ] [ ] [ ] [ ]
Why?
Because the DSA links are 'trunk' ports, if we just go ahead and delete
the MDB from sw1p4 and sw2p4 directly, we might delete those multicast
entries when they are still needed. Just consider the fact that somebody
does:
- add a multicast MAC address towards sw0p0 [ via the cross-chip
notifiers it gets installed on the DSA links too ]
- add the same multicast MAC address towards sw0p1 (another port of that
same switch)
- delete the same multicast MAC address from sw0p0.
At this point, if we deleted the MAC address from the DSA links, it
would be flooded, even though there is still an entry on switch 0 which
needs it not to.
So that is why deletions only match the targeted source port and nothing
on DSA links. Of course, dangling resources means that the hardware
tables will eventually run out given enough additions/removals, but hey,
at least it's simple.
But there is a bigger concern which needs to be addressed, and that is
our support for SWITCHDEV_OBJ_ID_HOST_MDB. DSA simply translates such an
object into a dsa_port_host_mdb_add() which ends up as ds->ops->port_mdb_add()
on the upstream port, and a similar thing happens on deletion:
dsa_port_host_mdb_del() will trigger ds->ops->port_mdb_del() on the
upstream port.
When there are 2 VLAN-unaware bridges spanning the same switch (which is
a use case DSA proudly supports), each bridge will install its own
SWITCHDEV_OBJ_ID_HOST_MDB entries. But upon deletion, DSA goes ahead and
emits a DSA_NOTIFIER_MDB_DEL for dp->cpu_dp, which is shared between the
user ports enslaved to br0 and the user ports enslaved to br1. Not good.
The host-trapped multicast addresses installed by br1 will be deleted
when any state changes in br0 (IGMP timers expire, or ports leave, etc).
To avoid this, we could of course go the route of the zero-sum game and
delete the DSA_NOTIFIER_MDB_DEL call for dp->cpu_dp. But the better
design is to just admit that on shared ports like DSA links and CPU
ports, we should be reference counting calls, even if this consumes some
dynamic memory which DSA has traditionally avoided. On the flip side,
the hardware tables of switches are limited in size, so it would be good
if the OS managed them properly instead of having them eventually
overflow.
To address the memory usage concern, we only apply the refcounting of
MDB entries on ports that are really shared (CPU ports and DSA links)
and not on user ports. In a typical single-switch setup, this means only
the CPU port (and the host MDB entries are not that many, really).
The name of the newly introduced data structures (dsa_mac_addr) is
chosen in such a way that will be reusable for host FDB entries (next
patch).
With this change, we can finally have the same matching logic for the
MDB additions and deletions, as well as for their host-trapped variants.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
include/net/dsa.h | 12 ++++++
net/dsa/dsa2.c | 8 ++++
net/dsa/switch.c | 104 ++++++++++++++++++++++++++++++++++++++++++----
3 files changed, 115 insertions(+), 9 deletions(-)
@@ -285,6 +285,11 @@ struct dsa_port {*/conststructdsa_netdevice_ops*netdev_ops;+/* List of MAC addresses that must be forwarded on this port.+*TheseareonlyvalidonCPUportsandDSAlinks.+*/+structlist_headmdbs;+boolsetup;};
@@ -178,6 +178,84 @@ static bool dsa_switch_host_address_match(struct dsa_switch *ds, int port,returnfalse;}+staticstructdsa_mac_addr*dsa_mac_addr_find(structlist_head*addr_list,+constunsignedchar*addr,+u16vid)+{+structdsa_mac_addr*a;++list_for_each_entry(a,addr_list,list)+if(ether_addr_equal(a->addr,addr)&&a->vid==vid)+returna;++returnNULL;+}++staticintdsa_switch_do_mdb_add(structdsa_switch*ds,intport,+conststructswitchdev_obj_port_mdb*mdb)+{+structdsa_port*dp=dsa_to_port(ds,port);+structdsa_mac_addr*a;+interr;++/* No need to bother with refcounting for user ports */+if(!(dsa_port_is_cpu(dp)||dsa_port_is_dsa(dp)))+returnds->ops->port_mdb_add(ds,port,mdb);++a=dsa_mac_addr_find(&dp->mdbs,mdb->addr,mdb->vid);+if(a){+refcount_inc(&a->refcount);+return0;+}++a=kzalloc(sizeof(*a),GFP_KERNEL);+if(!a)+return-ENOMEM;++err=ds->ops->port_mdb_add(ds,port,mdb);+if(err){+kfree(a);+returnerr;+}++ether_addr_copy(a->addr,mdb->addr);+a->vid=mdb->vid;+refcount_set(&a->refcount,1);+list_add_tail(&a->list,&dp->mdbs);++return0;+}++staticintdsa_switch_do_mdb_del(structdsa_switch*ds,intport,+conststructswitchdev_obj_port_mdb*mdb)+{+structdsa_port*dp=dsa_to_port(ds,port);+structdsa_mac_addr*a;+interr;++/* No need to bother with refcounting for user ports */+if(!(dsa_port_is_cpu(dp)||dsa_port_is_dsa(dp)))+returnds->ops->port_mdb_del(ds,port,mdb);++a=dsa_mac_addr_find(&dp->mdbs,mdb->addr,mdb->vid);+if(!a)+return-ENOENT;++if(!refcount_dec_and_test(&a->refcount))+return0;++err=ds->ops->port_mdb_del(ds,port,mdb);+if(err){+refcount_inc(&a->refcount);+returnerr;+}++list_del(&a->list);+kfree(a);++return0;+}+staticintdsa_switch_fdb_add(structdsa_switch*ds,structdsa_notifier_fdb_info*info){
@@ -267,19 +345,18 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,if(!ds->ops->port_mdb_add)return-EOPNOTSUPP;-returnds->ops->port_mdb_add(ds,port,info->mdb);+returndsa_switch_do_mdb_add(ds,port,info->mdb);}staticintdsa_switch_mdb_del(structdsa_switch*ds,structdsa_notifier_mdb_info*info){+intport=dsa_towards_port(ds,info->sw_index,info->port);+if(!ds->ops->port_mdb_del)return-EOPNOTSUPP;-if(ds->index==info->sw_index)-returnds->ops->port_mdb_del(ds,info->port,info->mdb);--return0;+returndsa_switch_do_mdb_del(ds,port,info->mdb);}staticintdsa_switch_host_mdb_add(structdsa_switch*ds,
@@ -294,7 +371,7 @@ static int dsa_switch_host_mdb_add(struct dsa_switch *ds,for(port=0;port<ds->num_ports;port++){if(dsa_switch_host_address_match(ds,port,info->sw_index,info->port)){-err=ds->ops->port_mdb_add(ds,port,info->mdb);+err=dsa_switch_do_mdb_add(ds,port,info->mdb);if(err)break;}
@@ -306,13 +383,22 @@ static int dsa_switch_host_mdb_add(struct dsa_switch *ds,staticintdsa_switch_host_mdb_del(structdsa_switch*ds,structdsa_notifier_mdb_info*info){+interr=0;+intport;+if(!ds->ops->port_mdb_del)return-EOPNOTSUPP;-if(ds->index==info->sw_index)-returnds->ops->port_mdb_del(ds,info->port,info->mdb);+for(port=0;port<ds->num_ports;port++){+if(dsa_switch_host_address_match(ds,port,info->sw_index,+info->port)){+err=dsa_switch_do_mdb_del(ds,port,info->mdb);+if(err)+break;+}+}-return0;+returnerr;}staticbooldsa_switch_vlan_match(structdsa_switch*ds,intport,
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-06-28 22:01:24
From: Vladimir Oltean <vladimir.oltean@nxp.com>
DSA treats some bridge FDB entries by trapping them to the CPU port.
Currently, the only class of such entries are FDB addresses learnt by
the software bridge on a foreign interface. However there are many more
to be added:
- FDB entries with the is_local flag (for termination) added by the
bridge on the user ports (typically containing the MAC address of the
bridge port)
- FDB entries pointing towards the bridge net device (for termination).
Typically these contain the MAC address of the bridge net device.
- Static FDB entries installed on a foreign interface that is in the
same bridge with a DSA user port.
The reason why a separate cross-chip notifier for host FDBs is justified
compared to normal FDBs is the same as in the case of host MDBs: the
cross-chip notifier matching function in switch.c should avoid
installing these entries on routing ports that route towards the
targeted switch, but not towards the CPU. This is required in order to
have proper support for H-like multi-chip topologies.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/dsa_priv.h | 7 +++++++
net/dsa/port.c | 26 ++++++++++++++++++++++++++
net/dsa/slave.c | 21 ++++++++++++++++-----
net/dsa/switch.c | 41 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 90 insertions(+), 5 deletions(-)
@@ -2315,8 +2315,12 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work)rtnl_lock();switch(switchdev_work->event){caseSWITCHDEV_FDB_ADD_TO_DEVICE:-err=dsa_port_fdb_add(dp,switchdev_work->addr,-switchdev_work->vid);+if(switchdev_work->host_addr)+err=dsa_port_host_fdb_add(dp,switchdev_work->addr,+switchdev_work->vid);+else+err=dsa_port_fdb_add(dp,switchdev_work->addr,+switchdev_work->vid);if(err){dev_err(ds->dev,"port %d failed to add %pM vid %d to fdb: %d\n",
@@ -2328,8 +2332,12 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work)break;caseSWITCHDEV_FDB_DEL_TO_DEVICE:-err=dsa_port_fdb_del(dp,switchdev_work->addr,-switchdev_work->vid);+if(switchdev_work->host_addr)+err=dsa_port_host_fdb_del(dp,switchdev_work->addr,+switchdev_work->vid);+else+err=dsa_port_fdb_del(dp,switchdev_work->addr,+switchdev_work->vid);if(err){dev_err(ds->dev,"port %d failed to delete %pM vid %d from fdb: %d\n",
@@ -2375,6 +2383,7 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,structnet_device*dev=switchdev_notifier_info_to_dev(ptr);conststructswitchdev_notifier_fdb_info*fdb_info;structdsa_switchdev_event_work*switchdev_work;+boolhost_addr=false;structdsa_port*dp;interr;
@@ -2412,7 +2421,8 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,if(!p)returnNOTIFY_DONE;-dp=p->dp->cpu_dp;+dp=p->dp;+host_addr=true;if(!dp->ds->assisted_learning_on_cpu_port)returnNOTIFY_DONE;
@@ -2442,6 +2452,7 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,ether_addr_copy(switchdev_work->addr,fdb_info->addr);switchdev_work->vid=fdb_info->vid;+switchdev_work->host_addr=host_addr;/* Hold a reference on the slave for dsa_fdb_offload_notify */if(dsa_is_user_port(dp->ds,dp->index))
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-06-28 22:01:27
From: Vladimir Oltean <vladimir.oltean@nxp.com>
The same concerns expressed for host MDB entries are valid for host FDBs
just as well:
- in the case of multiple bridges spanning the same switch chip, deleting
a host FDB entry that belongs to one bridge will result in breakage to
the other bridge
- not deleting FDB entries across DSA links means that the switch's
hardware tables will eventually run out, given enough wear&tear
So do the same thing and introduce reference counting for CPU ports and
DSA links using the same data structures as we have for MDB entries.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
include/net/dsa.h | 1 +
net/dsa/dsa2.c | 6 ++++
net/dsa/switch.c | 88 +++++++++++++++++++++++++++++++++++++++++++----
3 files changed, 88 insertions(+), 7 deletions(-)
@@ -288,6 +288,7 @@ struct dsa_port {/* List of MAC addresses that must be forwarded on this port.*TheseareonlyvalidonCPUportsandDSAlinks.*/+structlist_headfdbs;structlist_headmdbs;boolsetup;
@@ -256,6 +256,71 @@ static int dsa_switch_do_mdb_del(struct dsa_switch *ds, int port,return0;}+staticintdsa_switch_do_fdb_add(structdsa_switch*ds,intport,+constunsignedchar*addr,u16vid)+{+structdsa_port*dp=dsa_to_port(ds,port);+structdsa_mac_addr*a;+interr;++/* No need to bother with refcounting for user ports */+if(!(dsa_port_is_cpu(dp)||dsa_port_is_dsa(dp)))+returnds->ops->port_fdb_add(ds,port,addr,vid);++a=dsa_mac_addr_find(&dp->fdbs,addr,vid);+if(a){+refcount_inc(&a->refcount);+return0;+}++a=kzalloc(sizeof(*a),GFP_KERNEL);+if(!a)+return-ENOMEM;++err=ds->ops->port_fdb_add(ds,port,addr,vid);+if(err){+kfree(a);+returnerr;+}++ether_addr_copy(a->addr,addr);+a->vid=vid;+refcount_set(&a->refcount,1);+list_add_tail(&a->list,&dp->fdbs);++return0;+}++staticintdsa_switch_do_fdb_del(structdsa_switch*ds,intport,+constunsignedchar*addr,u16vid)+{+structdsa_port*dp=dsa_to_port(ds,port);+structdsa_mac_addr*a;+interr;++/* No need to bother with refcounting for user ports */+if(!(dsa_port_is_cpu(dp)||dsa_port_is_dsa(dp)))+returnds->ops->port_fdb_del(ds,port,addr,vid);++a=dsa_mac_addr_find(&dp->fdbs,addr,vid);+if(!a)+return-ENOENT;++if(!refcount_dec_and_test(&a->refcount))+return0;++err=ds->ops->port_fdb_del(ds,port,addr,vid);+if(err){+refcount_inc(&a->refcount);+returnerr;+}++list_del(&a->list);+kfree(a);++return0;+}+staticintdsa_switch_host_fdb_add(structdsa_switch*ds,structdsa_notifier_fdb_info*info){
@@ -268,7 +333,7 @@ static int dsa_switch_host_fdb_add(struct dsa_switch *ds,for(port=0;port<ds->num_ports;port++){if(dsa_switch_host_address_match(ds,port,info->sw_index,info->port)){-err=ds->ops->port_fdb_add(ds,port,info->addr,+err=dsa_switch_do_fdb_add(ds,port,info->addr,info->vid);if(err)break;
@@ -281,14 +346,23 @@ static int dsa_switch_host_fdb_add(struct dsa_switch *ds,staticintdsa_switch_host_fdb_del(structdsa_switch*ds,structdsa_notifier_fdb_info*info){+interr=0;+intport;+if(!ds->ops->port_fdb_del)return-EOPNOTSUPP;-if(ds->index==info->sw_index)-returnds->ops->port_fdb_del(ds,info->port,info->addr,-info->vid);+for(port=0;port<ds->num_ports;port++){+if(dsa_switch_host_address_match(ds,port,info->sw_index,+info->port)){+err=dsa_switch_do_fdb_del(ds,port,info->addr,+info->vid);+if(err)+break;+}+}-return0;+returnerr;}staticintdsa_switch_fdb_add(structdsa_switch*ds,
@@ -299,7 +373,7 @@ static int dsa_switch_fdb_add(struct dsa_switch *ds,if(!ds->ops->port_fdb_add)return-EOPNOTSUPP;-returnds->ops->port_fdb_add(ds,port,info->addr,info->vid);+returndsa_switch_do_fdb_add(ds,port,info->addr,info->vid);}staticintdsa_switch_fdb_del(structdsa_switch*ds,
@@ -310,7 +384,7 @@ static int dsa_switch_fdb_del(struct dsa_switch *ds,if(!ds->ops->port_fdb_del)return-EOPNOTSUPP;-returnds->ops->port_fdb_del(ds,port,info->addr,info->vid);+returndsa_switch_do_fdb_del(ds,port,info->addr,info->vid);}staticintdsa_switch_hsr_join(structdsa_switch*ds,
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-06-28 22:01:32
From: Vladimir Oltean <vladimir.oltean@nxp.com>
If the DSA master implements strict address filtering, then the unicast
and multicast addresses kept by the DSA CPU ports should be synchronized
with the address lists of the DSA master.
Note that we want the synchronization of the master's address lists even
if the DSA switch doesn't support unicast/multicast database operations,
on the premises that the packets will be flooded to the CPU in that
case, and we should still instruct the master to receive them.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/port.c | 32 ++++++++++++++++++++++++++++----
1 file changed, 28 insertions(+), 4 deletions(-)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-06-28 22:01:37
From: Tobias Waldekranz <tobias@waldekranz.com>
The bridge automatically creates local (not forwarded) fdb entries
pointing towards physical ports with their interface MAC addresses.
For switchdev, the significance of these fdb entries is the exact
opposite of that of non-local entries: instead of sending these frame
outwards, we must send them inwards (towards the host).
NOTE: The bridge's own MAC address is also "local". If that address is
not shared with any port, the bridge's MAC is not be added by this
functionality - but the following commit takes care of that case.
NOTE 2: We mark these addresses as host-filtered regardless of the value
of ds->assisted_learning_on_cpu_port. This is because, as opposed to the
speculative logic done for dynamic address learning on foreign
interfaces, the local FDB entries are rather fixed, so there isn't any
risk of them migrating from one bridge port to another.
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/slave.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-06-28 22:01:41
From: Tobias Waldekranz <tobias@waldekranz.com>
Reuse the "assisted_learning_on_cpu_port" functionality to always add
entries for user-configured entries on foreign interfaces, even if
assisted_learning_on_cpu_port is not enabled. E.g. in this situation:
br0
/ \
swp0 dummy0
$ bridge fdb add 02:00:de:ad:00:01 dev dummy0 vlan 1 master static
Results in DSA adding an entry in the hardware FDB, pointing this
address towards the CPU port.
The same is true for entries added to the bridge itself, e.g:
$ bridge fdb add 02:00:de:ad:00:01 dev br0 vlan 1 self local
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/slave.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
@@ -2403,9 +2403,12 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,dp=dsa_slave_to_port(dev);}else{-/* Snoop addresses learnt on foreign interfaces-*bridgedwithus,forswitchesthatdon't-*automaticallylearnSAfromCPU-injectedtraffic+/* Snoop addresses added to foreign interfaces+*bridgedwithus,orthebridge+*itself.Dynamicallylearnedaddressescan+*alsobeaddedforswitchesthatdon't+*automaticallylearnSAfromCPU-injected+*traffic.*/structnet_device*br_dev;structdsa_slave_priv*p;
@@ -2424,7 +2427,8 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,dp=p->dp;host_addr=true;-if(!dp->ds->assisted_learning_on_cpu_port)+if(!fdb_info->added_by_user&&+!dp->ds->assisted_learning_on_cpu_port)returnNOTIFY_DONE;/* When the bridge learns an address on an offloaded
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-06-28 22:01:45
From: Vladimir Oltean <vladimir.oltean@nxp.com>
The bridge supports a legacy way of adding local (non-forwarded) FDB
entries, which works on an individual port basis:
bridge fdb add dev swp0 00:01:02:03:04:05 master local
As well as a new way, added by Roopa Prabhu in commit 3741873b4f73
("bridge: allow adding of fdb entries pointing to the bridge device"):
bridge fdb add dev br0 00:01:02:03:04:05 self local
The two commands are functionally equivalent, except that the first one
produces an entry with fdb->dst == swp0, and the other an entry with
fdb->dst == NULL. The confusing part, though, is that even if fdb->dst
is swp0 for the 'local on port' entry, that destination is not used.
Nonetheless, the idea is that the bridge has reference counting for
local entries, and local entries pointing towards the bridge are still
'as local' as local entries for a port.
The bridge adds the MAC addresses of the interfaces automatically as
FDB entries with is_local=1. For the MAC address of the ports, fdb->dst
will be equal to the port, and for the MAC address of the bridge,
fdb->dst will point towards the bridge (i.e. be NULL). Therefore, if the
MAC address of the bridge is not inherited from either of the physical
ports, then we must explicitly catch local FDB entries emitted towards
the br0, otherwise we'll miss the MAC address of the bridge (and, of
course, any entry with 'bridge add dev br0 ... self local').
Co-developed-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/slave.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-06-28 22:01:51
From: Vladimir Oltean <vladimir.oltean@nxp.com>
When we join a bridge that already has some local addresses pointing to
itself, we do not get those notifications. Similarly, when we leave that
bridge, we do not get notifications for the deletion of those entries.
The only switchdev notifications we get are those of entries added while
the DSA port is enslaved to the bridge.
This makes use cases such as the following work properly (with the
number of additions and removals properly balanced):
ip link add br0 type bridge
ip link add br1 type bridge
ip link set br0 address 00:01:02:03:04:05
ip link set br1 address 00:01:02:03:04:05
ip link set swp0 up
ip link set swp1 up
ip link set swp0 master br0
ip link set swp1 master br1
ip link set br0 up
ip link set br1 up
ip link del br1 # 00:01:02:03:04:05 still installed on the CPU port
ip link del br0 # 00:01:02:03:04:05 finally removed from the CPU port
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/port.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
@@ -199,11 +199,17 @@ static int dsa_port_switchdev_sync(struct dsa_port *dp,if(err&&err!=-EOPNOTSUPP)returnerr;+/* Forwarding and termination FDB entries on the port */err=br_fdb_replay(br,brport_dev,dp,true,&dsa_slave_switchdev_notifier);if(err&&err!=-EOPNOTSUPP)returnerr;+/* Termination FDB entries on the bridge itself */+err=br_fdb_replay(br,br,dp,true,&dsa_slave_switchdev_notifier);+if(err&&err!=-EOPNOTSUPP)+returnerr;+err=br_vlan_replay(br,brport_dev,dp,true,&dsa_slave_switchdev_blocking_notifier,extack);if(err&&err!=-EOPNOTSUPP)
@@ -225,11 +231,17 @@ static int dsa_port_switchdev_unsync_objs(struct dsa_port *dp,if(err&&err!=-EOPNOTSUPP)returnerr;+/* Forwarding and termination FDB entries on the port */err=br_fdb_replay(br,brport_dev,dp,false,&dsa_slave_switchdev_notifier);if(err&&err!=-EOPNOTSUPP)returnerr;+/* Termination FDB entries on the bridge itself */+err=br_fdb_replay(br,br,dp,false,&dsa_slave_switchdev_notifier);+if(err&&err!=-EOPNOTSUPP)+returnerr;+err=br_vlan_replay(br,brport_dev,dp,false,&dsa_slave_switchdev_blocking_notifier,extack);if(err&&err!=-EOPNOTSUPP)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-06-28 22:01:56
From: Vladimir Oltean <vladimir.oltean@nxp.com>
When
(a) "dev" is a bridge port which the DSA switch tree offloads, but is
otherwise not a dsa slave (such as a LAG netdev), or
(b) "dev" is the bridge net device itself
then strange things happen to the dev_hold/dev_put pair:
dsa_schedule_work() will still be called with a DSA port that offloads
that netdev, but dev_hold() will be called on the non-DSA netdev.
Then the "if" condition in dsa_slave_switchdev_event_work() does not
pass, because "dev" is not a DSA netdev, so dev_put() is not called.
This results in the simple fact that we have a reference counting
mismatch on the "dev" net device.
This can be seen when we add support for host addresses installed on the
bridge net device.
ip link add br1 type bridge
ip link set br1 address 00:01:02:03:04:05
ip link set swp0 master br1
ip link del br1
[ 968.512278] unregister_netdevice: waiting for br1 to become free. Usage count = 5
It seems foolish to do penny pinching and not add the net_device pointer
in the dsa_switchdev_event_work structure, so let's finally do that.
As an added bonus, when we start offloading local entries pointing
towards the bridge, these will now properly appear as 'offloaded' in
'bridge fdb' (this was not possible before, because 'dev' was assumed to
only be a DSA net device):
00:01:02:03:04:05 dev br0 vlan 1 offload master br0 permanent
00:01:02:03:04:05 dev br0 offload master br0 permanent
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/dsa_priv.h | 1 +
net/dsa/slave.c | 9 ++++-----
2 files changed, 5 insertions(+), 5 deletions(-)
@@ -116,6 +116,7 @@ struct dsa_notifier_mrp_ring_role_info {structdsa_switchdev_event_work{structdsa_switch*ds;intport;+structnet_device*dev;structwork_structwork;unsignedlongevent;/* Specific for SWITCHDEV_FDB_ADD_TO_DEVICE and
@@ -2469,15 +2468,15 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,switchdev_work->ds=dp->ds;switchdev_work->port=dp->index;switchdev_work->event=event;+switchdev_work->dev=dev;ether_addr_copy(switchdev_work->addr,fdb_info->addr);switchdev_work->vid=fdb_info->vid;switchdev_work->host_addr=host_addr;-/* Hold a reference on the slave for dsa_fdb_offload_notify */-if(dsa_is_user_port(dp->ds,dp->index))-dev_hold(dev);+/* Hold a reference for dsa_fdb_offload_notify */+dev_hold(dev);dsa_schedule_work(&switchdev_work->work);break;default:
From: Nikolay Aleksandrov <hidden> Date: 2021-06-29 10:40:32
On 29/06/2021 00:59, Vladimir Oltean wrote:
quoted hunk
From: Tobias Waldekranz <tobias@waldekranz.com>
Treat addresses added to the bridge itself in the same way as regular
ports and send out a notification so that drivers may sync it down to
the hardware FDB.
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/bridge/br_fdb.c | 4 ++--
net/bridge/br_private.h | 7 ++++---
net/bridge/br_switchdev.c | 11 +++++------
3 files changed, 11 insertions(+), 11 deletions(-)
you should use READ_ONCE() for fdb->dst here to make sure it's read only once,
to be fair the old code had the same issue :)
Thanks for the comment. I still have budget for one patch until I hit
the 15 limit, so I guess I'll do that separately before this one.
Just trying to make sure I get it right. You want me to annotate
fdb_create(), br_fdb_update(), fdb_add_entry() and
br_fdb_external_learn_add() with WRITE_ONCE() too, right?
Can I resend right away or did you notice other issues in the other
patches?
you should use READ_ONCE() for fdb->dst here to make sure it's read only once,
to be fair the old code had the same issue :)
Thanks for the comment. I still have budget for one patch until I hit
the 15 limit, so I guess I'll do that separately before this one.
Just trying to make sure I get it right. You want me to annotate
fdb_create(), br_fdb_update(), fdb_add_entry() and
br_fdb_external_learn_add() with WRITE_ONCE() too, right?
Can I resend right away or did you notice other issues in the other
patches?
That would be best, yes. The rest of the changes look good to me.
Thanks,
Nik