[PATCH net-next 0/8] Bridge FDB refactoring

[PATCH net-next 0/8] Bridge FDB refactoring

[Vladimir Oltean]

This series refactors the br_fdb.c, br_switchdev.c and switchdev.c files
to offer the same level of functionality with a bit less code, and to
clarify the purpose of some functions.

No functional change intended.

Vladimir Oltean (8):
  net: bridge: remove fdb_notify forward declaration
  net: bridge: remove fdb_insert forward declaration
  net: bridge: rename fdb_insert to fdb_add_local
  net: bridge: rename br_fdb_insert to br_fdb_add_local
  net: bridge: reduce indentation level in fdb_create
  net: bridge: move br_fdb_replay inside br_switchdev.c
  net: bridge: create a common function for populating switchdev FDB
    entries
  net: switchdev: merge switchdev_handle_fdb_{add,del}_to_device

 include/net/switchdev.h   |  48 +----
 net/bridge/br_fdb.c       | 433 +++++++++++++++++---------------------
 net/bridge/br_if.c        |   2 +-
 net/bridge/br_private.h   |   6 +-
 net/bridge/br_switchdev.c |  79 ++++++-
 net/bridge/br_vlan.c      |   5 +-
 net/dsa/slave.c           |  41 +---
 net/switchdev/switchdev.c | 156 +++-----------
 8 files changed, 305 insertions(+), 465 deletions(-)

-- 
2.25.1

[PATCH net-next 1/8] net: bridge: remove fdb_notify forward declaration

[Vladimir Oltean]

fdb_notify() has a forward declaration because its first caller,
fdb_delete(), is declared before 3 functions that fdb_notify() needs:
fdb_to_nud(), fdb_fill_info() and fdb_nlmsg_size().

This patch moves the aforementioned 4 functions above fdb_delete() and
deletes the forward declaration.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/bridge/br_fdb.c | 246 ++++++++++++++++++++++----------------------
 1 file changed, 122 insertions(+), 124 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index a6a68e18c70a..bfb28a24ea81 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -34,8 +34,6 @@ static const struct rhashtable_params br_fdb_rht_params = {
 static struct kmem_cache *br_fdb_cache __read_mostly;
 static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
 		      const unsigned char *addr, u16 vid);
-static void fdb_notify(struct net_bridge *br,
-		       const struct net_bridge_fdb_entry *, int, bool);
 
 int __init br_fdb_init(void)
 {
@@ -87,6 +85,128 @@ static void fdb_rcu_free(struct rcu_head *head)
 	kmem_cache_free(br_fdb_cache, ent);
 }
 
+static int fdb_to_nud(const struct net_bridge *br,
+		      const struct net_bridge_fdb_entry *fdb)
+{
+	if (test_bit(BR_FDB_LOCAL, &fdb->flags))
+		return NUD_PERMANENT;
+	else if (test_bit(BR_FDB_STATIC, &fdb->flags))
+		return NUD_NOARP;
+	else if (has_expired(br, fdb))
+		return NUD_STALE;
+	else
+		return NUD_REACHABLE;
+}
+
+static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
+			 const struct net_bridge_fdb_entry *fdb,
+			 u32 portid, u32 seq, int type, unsigned int flags)
+{
+	const struct net_bridge_port *dst = READ_ONCE(fdb->dst);
+	unsigned long now = jiffies;
+	struct nda_cacheinfo ci;
+	struct nlmsghdr *nlh;
+	struct ndmsg *ndm;
+
+	nlh = nlmsg_put(skb, portid, seq, type, sizeof(*ndm), flags);
+	if (nlh == NULL)
+		return -EMSGSIZE;
+
+	ndm = nlmsg_data(nlh);
+	ndm->ndm_family	 = AF_BRIDGE;
+	ndm->ndm_pad1    = 0;
+	ndm->ndm_pad2    = 0;
+	ndm->ndm_flags	 = 0;
+	ndm->ndm_type	 = 0;
+	ndm->ndm_ifindex = dst ? dst->dev->ifindex : br->dev->ifindex;
+	ndm->ndm_state   = fdb_to_nud(br, fdb);
+
+	if (test_bit(BR_FDB_OFFLOADED, &fdb->flags))
+		ndm->ndm_flags |= NTF_OFFLOADED;
+	if (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
+		ndm->ndm_flags |= NTF_EXT_LEARNED;
+	if (test_bit(BR_FDB_STICKY, &fdb->flags))
+		ndm->ndm_flags |= NTF_STICKY;
+
+	if (nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->key.addr))
+		goto nla_put_failure;
+	if (nla_put_u32(skb, NDA_MASTER, br->dev->ifindex))
+		goto nla_put_failure;
+	ci.ndm_used	 = jiffies_to_clock_t(now - fdb->used);
+	ci.ndm_confirmed = 0;
+	ci.ndm_updated	 = jiffies_to_clock_t(now - fdb->updated);
+	ci.ndm_refcnt	 = 0;
+	if (nla_put(skb, NDA_CACHEINFO, sizeof(ci), &ci))
+		goto nla_put_failure;
+
+	if (fdb->key.vlan_id && nla_put(skb, NDA_VLAN, sizeof(u16),
+					&fdb->key.vlan_id))
+		goto nla_put_failure;
+
+	if (test_bit(BR_FDB_NOTIFY, &fdb->flags)) {
+		struct nlattr *nest = nla_nest_start(skb, NDA_FDB_EXT_ATTRS);
+		u8 notify_bits = FDB_NOTIFY_BIT;
+
+		if (!nest)
+			goto nla_put_failure;
+		if (test_bit(BR_FDB_NOTIFY_INACTIVE, &fdb->flags))
+			notify_bits |= FDB_NOTIFY_INACTIVE_BIT;
+
+		if (nla_put_u8(skb, NFEA_ACTIVITY_NOTIFY, notify_bits)) {
+			nla_nest_cancel(skb, nest);
+			goto nla_put_failure;
+		}
+
+		nla_nest_end(skb, nest);
+	}
+
+	nlmsg_end(skb, nlh);
+	return 0;
+
+nla_put_failure:
+	nlmsg_cancel(skb, nlh);
+	return -EMSGSIZE;
+}
+
+static inline size_t fdb_nlmsg_size(void)
+{
+	return NLMSG_ALIGN(sizeof(struct ndmsg))
+		+ nla_total_size(ETH_ALEN) /* NDA_LLADDR */
+		+ nla_total_size(sizeof(u32)) /* NDA_MASTER */
+		+ nla_total_size(sizeof(u16)) /* NDA_VLAN */
+		+ nla_total_size(sizeof(struct nda_cacheinfo))
+		+ nla_total_size(0) /* NDA_FDB_EXT_ATTRS */
+		+ nla_total_size(sizeof(u8)); /* NFEA_ACTIVITY_NOTIFY */
+}
+
+static void fdb_notify(struct net_bridge *br,
+		       const struct net_bridge_fdb_entry *fdb, int type,
+		       bool swdev_notify)
+{
+	struct net *net = dev_net(br->dev);
+	struct sk_buff *skb;
+	int err = -ENOBUFS;
+
+	if (swdev_notify)
+		br_switchdev_fdb_notify(br, fdb, type);
+
+	skb = nlmsg_new(fdb_nlmsg_size(), GFP_ATOMIC);
+	if (skb == NULL)
+		goto errout;
+
+	err = fdb_fill_info(skb, br, fdb, 0, 0, type, 0);
+	if (err < 0) {
+		/* -EMSGSIZE implies BUG in fdb_nlmsg_size() */
+		WARN_ON(err == -EMSGSIZE);
+		kfree_skb(skb);
+		goto errout;
+	}
+	rtnl_notify(skb, net, 0, RTNLGRP_NEIGH, NULL, GFP_ATOMIC);
+	return;
+errout:
+	rtnl_set_sk_err(net, RTNLGRP_NEIGH, err);
+}
+
 static struct net_bridge_fdb_entry *fdb_find_rcu(struct rhashtable *tbl,
 						 const unsigned char *addr,
 						 __u16 vid)
@@ -638,100 +758,6 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 	}
 }
 
-static int fdb_to_nud(const struct net_bridge *br,
-		      const struct net_bridge_fdb_entry *fdb)
-{
-	if (test_bit(BR_FDB_LOCAL, &fdb->flags))
-		return NUD_PERMANENT;
-	else if (test_bit(BR_FDB_STATIC, &fdb->flags))
-		return NUD_NOARP;
-	else if (has_expired(br, fdb))
-		return NUD_STALE;
-	else
-		return NUD_REACHABLE;
-}
-
-static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
-			 const struct net_bridge_fdb_entry *fdb,
-			 u32 portid, u32 seq, int type, unsigned int flags)
-{
-	const struct net_bridge_port *dst = READ_ONCE(fdb->dst);
-	unsigned long now = jiffies;
-	struct nda_cacheinfo ci;
-	struct nlmsghdr *nlh;
-	struct ndmsg *ndm;
-
-	nlh = nlmsg_put(skb, portid, seq, type, sizeof(*ndm), flags);
-	if (nlh == NULL)
-		return -EMSGSIZE;
-
-	ndm = nlmsg_data(nlh);
-	ndm->ndm_family	 = AF_BRIDGE;
-	ndm->ndm_pad1    = 0;
-	ndm->ndm_pad2    = 0;
-	ndm->ndm_flags	 = 0;
-	ndm->ndm_type	 = 0;
-	ndm->ndm_ifindex = dst ? dst->dev->ifindex : br->dev->ifindex;
-	ndm->ndm_state   = fdb_to_nud(br, fdb);
-
-	if (test_bit(BR_FDB_OFFLOADED, &fdb->flags))
-		ndm->ndm_flags |= NTF_OFFLOADED;
-	if (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
-		ndm->ndm_flags |= NTF_EXT_LEARNED;
-	if (test_bit(BR_FDB_STICKY, &fdb->flags))
-		ndm->ndm_flags |= NTF_STICKY;
-
-	if (nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->key.addr))
-		goto nla_put_failure;
-	if (nla_put_u32(skb, NDA_MASTER, br->dev->ifindex))
-		goto nla_put_failure;
-	ci.ndm_used	 = jiffies_to_clock_t(now - fdb->used);
-	ci.ndm_confirmed = 0;
-	ci.ndm_updated	 = jiffies_to_clock_t(now - fdb->updated);
-	ci.ndm_refcnt	 = 0;
-	if (nla_put(skb, NDA_CACHEINFO, sizeof(ci), &ci))
-		goto nla_put_failure;
-
-	if (fdb->key.vlan_id && nla_put(skb, NDA_VLAN, sizeof(u16),
-					&fdb->key.vlan_id))
-		goto nla_put_failure;
-
-	if (test_bit(BR_FDB_NOTIFY, &fdb->flags)) {
-		struct nlattr *nest = nla_nest_start(skb, NDA_FDB_EXT_ATTRS);
-		u8 notify_bits = FDB_NOTIFY_BIT;
-
-		if (!nest)
-			goto nla_put_failure;
-		if (test_bit(BR_FDB_NOTIFY_INACTIVE, &fdb->flags))
-			notify_bits |= FDB_NOTIFY_INACTIVE_BIT;
-
-		if (nla_put_u8(skb, NFEA_ACTIVITY_NOTIFY, notify_bits)) {
-			nla_nest_cancel(skb, nest);
-			goto nla_put_failure;
-		}
-
-		nla_nest_end(skb, nest);
-	}
-
-	nlmsg_end(skb, nlh);
-	return 0;
-
-nla_put_failure:
-	nlmsg_cancel(skb, nlh);
-	return -EMSGSIZE;
-}
-
-static inline size_t fdb_nlmsg_size(void)
-{
-	return NLMSG_ALIGN(sizeof(struct ndmsg))
-		+ nla_total_size(ETH_ALEN) /* NDA_LLADDR */
-		+ nla_total_size(sizeof(u32)) /* NDA_MASTER */
-		+ nla_total_size(sizeof(u16)) /* NDA_VLAN */
-		+ nla_total_size(sizeof(struct nda_cacheinfo))
-		+ nla_total_size(0) /* NDA_FDB_EXT_ATTRS */
-		+ nla_total_size(sizeof(u8)); /* NFEA_ACTIVITY_NOTIFY */
-}
-
 static int br_fdb_replay_one(struct net_bridge *br, struct notifier_block *nb,
 			     const struct net_bridge_fdb_entry *fdb,
 			     unsigned long action, const void *ctx)
@@ -786,34 +812,6 @@ int br_fdb_replay(const struct net_device *br_dev, const void *ctx, bool adding,
 	return err;
 }
 
-static void fdb_notify(struct net_bridge *br,
-		       const struct net_bridge_fdb_entry *fdb, int type,
-		       bool swdev_notify)
-{
-	struct net *net = dev_net(br->dev);
-	struct sk_buff *skb;
-	int err = -ENOBUFS;
-
-	if (swdev_notify)
-		br_switchdev_fdb_notify(br, fdb, type);
-
-	skb = nlmsg_new(fdb_nlmsg_size(), GFP_ATOMIC);
-	if (skb == NULL)
-		goto errout;
-
-	err = fdb_fill_info(skb, br, fdb, 0, 0, type, 0);
-	if (err < 0) {
-		/* -EMSGSIZE implies BUG in fdb_nlmsg_size() */
-		WARN_ON(err == -EMSGSIZE);
-		kfree_skb(skb);
-		goto errout;
-	}
-	rtnl_notify(skb, net, 0, RTNLGRP_NEIGH, NULL, GFP_ATOMIC);
-	return;
-errout:
-	rtnl_set_sk_err(net, RTNLGRP_NEIGH, err);
-}
-
 /* Dump information about entries, in response to GETNEIGH */
 int br_fdb_dump(struct sk_buff *skb,
 		struct netlink_callback *cb,
-- 
2.25.1

Re: [PATCH net-next 1/8] net: bridge: remove fdb_notify forward declaration

[Ido Schimmel]

On Tue, Oct 26, 2021 at 05:27:36PM +0300, Vladimir Oltean wrote:
> fdb_notify() has a forward declaration because its first caller,
> fdb_delete(), is declared before 3 functions that fdb_notify() needs:
> fdb_to_nud(), fdb_fill_info() and fdb_nlmsg_size().
> 
> This patch moves the aforementioned 4 functions above fdb_delete() and
> deletes the forward declaration.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Re: [PATCH net-next 1/8] net: bridge: remove fdb_notify forward declaration

[Nikolay Aleksandrov]

On 26/10/2021 17:27, Vladimir Oltean wrote:
> fdb_notify() has a forward declaration because its first caller,
> fdb_delete(), is declared before 3 functions that fdb_notify() needs:
> fdb_to_nud(), fdb_fill_info() and fdb_nlmsg_size().
> 
> This patch moves the aforementioned 4 functions above fdb_delete() and
> deletes the forward declaration.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/bridge/br_fdb.c | 246 ++++++++++++++++++++++----------------------
>  1 file changed, 122 insertions(+), 124 deletions(-)
> 

Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>

[PATCH net-next 2/8] net: bridge: remove fdb_insert forward declaration

[Vladimir Oltean]

fdb_insert() has a forward declaration because its first caller,
br_fdb_changeaddr(), is declared before fdb_create(), a function which
fdb_insert() needs.

This patch moves the 2 functions above br_fdb_changeaddr() and deletes
the forward declaration for fdb_insert().

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/bridge/br_fdb.c | 116 ++++++++++++++++++++++----------------------
 1 file changed, 57 insertions(+), 59 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index bfb28a24ea81..082e91130677 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -32,8 +32,6 @@ static const struct rhashtable_params br_fdb_rht_params = {
 };
 
 static struct kmem_cache *br_fdb_cache __read_mostly;
-static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
-		      const unsigned char *addr, u16 vid);
 
 int __init br_fdb_init(void)
 {
@@ -377,6 +375,63 @@ void br_fdb_find_delete_local(struct net_bridge *br,
 	spin_unlock_bh(&br->hash_lock);
 }
 
+static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
+					       struct net_bridge_port *source,
+					       const unsigned char *addr,
+					       __u16 vid,
+					       unsigned long flags)
+{
+	struct net_bridge_fdb_entry *fdb;
+
+	fdb = kmem_cache_alloc(br_fdb_cache, GFP_ATOMIC);
+	if (fdb) {
+		memcpy(fdb->key.addr.addr, addr, ETH_ALEN);
+		WRITE_ONCE(fdb->dst, source);
+		fdb->key.vlan_id = vid;
+		fdb->flags = flags;
+		fdb->updated = fdb->used = jiffies;
+		if (rhashtable_lookup_insert_fast(&br->fdb_hash_tbl,
+						  &fdb->rhnode,
+						  br_fdb_rht_params)) {
+			kmem_cache_free(br_fdb_cache, fdb);
+			fdb = NULL;
+		} else {
+			hlist_add_head_rcu(&fdb->fdb_node, &br->fdb_list);
+		}
+	}
+	return fdb;
+}
+
+static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
+		      const unsigned char *addr, u16 vid)
+{
+	struct net_bridge_fdb_entry *fdb;
+
+	if (!is_valid_ether_addr(addr))
+		return -EINVAL;
+
+	fdb = br_fdb_find(br, addr, vid);
+	if (fdb) {
+		/* it is okay to have multiple ports with same
+		 * address, just use the first one.
+		 */
+		if (test_bit(BR_FDB_LOCAL, &fdb->flags))
+			return 0;
+		br_warn(br, "adding interface %s with same address as a received packet (addr:%pM, vlan:%u)\n",
+			source ? source->dev->name : br->dev->name, addr, vid);
+		fdb_delete(br, fdb, true);
+	}
+
+	fdb = fdb_create(br, source, addr, vid,
+			 BIT(BR_FDB_LOCAL) | BIT(BR_FDB_STATIC));
+	if (!fdb)
+		return -ENOMEM;
+
+	fdb_add_hw_addr(br, addr);
+	fdb_notify(br, fdb, RTM_NEWNEIGH, true);
+	return 0;
+}
+
 void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
 {
 	struct net_bridge_vlan_group *vg;
@@ -623,63 +678,6 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
 	return num;
 }
 
-static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
-					       struct net_bridge_port *source,
-					       const unsigned char *addr,
-					       __u16 vid,
-					       unsigned long flags)
-{
-	struct net_bridge_fdb_entry *fdb;
-
-	fdb = kmem_cache_alloc(br_fdb_cache, GFP_ATOMIC);
-	if (fdb) {
-		memcpy(fdb->key.addr.addr, addr, ETH_ALEN);
-		WRITE_ONCE(fdb->dst, source);
-		fdb->key.vlan_id = vid;
-		fdb->flags = flags;
-		fdb->updated = fdb->used = jiffies;
-		if (rhashtable_lookup_insert_fast(&br->fdb_hash_tbl,
-						  &fdb->rhnode,
-						  br_fdb_rht_params)) {
-			kmem_cache_free(br_fdb_cache, fdb);
-			fdb = NULL;
-		} else {
-			hlist_add_head_rcu(&fdb->fdb_node, &br->fdb_list);
-		}
-	}
-	return fdb;
-}
-
-static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
-		  const unsigned char *addr, u16 vid)
-{
-	struct net_bridge_fdb_entry *fdb;
-
-	if (!is_valid_ether_addr(addr))
-		return -EINVAL;
-
-	fdb = br_fdb_find(br, addr, vid);
-	if (fdb) {
-		/* it is okay to have multiple ports with same
-		 * address, just use the first one.
-		 */
-		if (test_bit(BR_FDB_LOCAL, &fdb->flags))
-			return 0;
-		br_warn(br, "adding interface %s with same address as a received packet (addr:%pM, vlan:%u)\n",
-		       source ? source->dev->name : br->dev->name, addr, vid);
-		fdb_delete(br, fdb, true);
-	}
-
-	fdb = fdb_create(br, source, addr, vid,
-			 BIT(BR_FDB_LOCAL) | BIT(BR_FDB_STATIC));
-	if (!fdb)
-		return -ENOMEM;
-
-	fdb_add_hw_addr(br, addr);
-	fdb_notify(br, fdb, RTM_NEWNEIGH, true);
-	return 0;
-}
-
 /* Add entry for local address of interface */
 int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
 		  const unsigned char *addr, u16 vid)
-- 
2.25.1

Re: [PATCH net-next 2/8] net: bridge: remove fdb_insert forward declaration

[Ido Schimmel]

On Tue, Oct 26, 2021 at 05:27:37PM +0300, Vladimir Oltean wrote:
> fdb_insert() has a forward declaration because its first caller,
> br_fdb_changeaddr(), is declared before fdb_create(), a function which
> fdb_insert() needs.
> 
> This patch moves the 2 functions above br_fdb_changeaddr() and deletes
> the forward declaration for fdb_insert().
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Re: [PATCH net-next 2/8] net: bridge: remove fdb_insert forward declaration

[Nikolay Aleksandrov]

On 26/10/2021 17:27, Vladimir Oltean wrote:
> fdb_insert() has a forward declaration because its first caller,
> br_fdb_changeaddr(), is declared before fdb_create(), a function which
> fdb_insert() needs.
> 
> This patch moves the 2 functions above br_fdb_changeaddr() and deletes
> the forward declaration for fdb_insert().
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/bridge/br_fdb.c | 116 ++++++++++++++++++++++----------------------
>  1 file changed, 57 insertions(+), 59 deletions(-)
> 

Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>

[PATCH net-next 3/8] net: bridge: rename fdb_insert to fdb_add_local

[Vladimir Oltean]

fdb_insert() is not a descriptive name for this function, and also easy
to confuse with __br_fdb_add(), fdb_add_entry(), br_fdb_update().
Even more confusingly, it is not even related in any way with those
functions, neither one calls the other.

Since fdb_insert() basically deals with the creation of a BR_FDB_LOCAL
entry and is called only from functions where that is the intention:

- br_fdb_changeaddr
- br_fdb_change_mac_address
- br_fdb_insert

then rename it to fdb_add_local(), because its removal counterpart is
called fdb_delete_local().

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/bridge/br_fdb.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 082e91130677..668f87db7644 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -402,8 +402,8 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
 	return fdb;
 }
 
-static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
-		      const unsigned char *addr, u16 vid)
+static int fdb_add_local(struct net_bridge *br, struct net_bridge_port *source,
+			 const unsigned char *addr, u16 vid)
 {
 	struct net_bridge_fdb_entry *fdb;
 
@@ -458,7 +458,7 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
 
 insert:
 	/* insert new address,  may fail if invalid address or dup. */
-	fdb_insert(br, p, newaddr, 0);
+	fdb_add_local(br, p, newaddr, 0);
 
 	if (!vg || !vg->num_vlans)
 		goto done;
@@ -468,7 +468,7 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
 	 * from under us.
 	 */
 	list_for_each_entry(v, &vg->vlan_list, vlist)
-		fdb_insert(br, p, newaddr, v->vid);
+		fdb_add_local(br, p, newaddr, v->vid);
 
 done:
 	spin_unlock_bh(&br->hash_lock);
@@ -488,7 +488,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
 	    !f->dst && !test_bit(BR_FDB_ADDED_BY_USER, &f->flags))
 		fdb_delete_local(br, NULL, f);
 
-	fdb_insert(br, NULL, newaddr, 0);
+	fdb_add_local(br, NULL, newaddr, 0);
 	vg = br_vlan_group(br);
 	if (!vg || !vg->num_vlans)
 		goto out;
@@ -503,7 +503,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
 		if (f && test_bit(BR_FDB_LOCAL, &f->flags) &&
 		    !f->dst && !test_bit(BR_FDB_ADDED_BY_USER, &f->flags))
 			fdb_delete_local(br, NULL, f);
-		fdb_insert(br, NULL, newaddr, v->vid);
+		fdb_add_local(br, NULL, newaddr, v->vid);
 	}
 out:
 	spin_unlock_bh(&br->hash_lock);
@@ -685,7 +685,7 @@ int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
 	int ret;
 
 	spin_lock_bh(&br->hash_lock);
-	ret = fdb_insert(br, source, addr, vid);
+	ret = fdb_add_local(br, source, addr, vid);
 	spin_unlock_bh(&br->hash_lock);
 	return ret;
 }
-- 
2.25.1

Re: [PATCH net-next 3/8] net: bridge: rename fdb_insert to fdb_add_local

[Ido Schimmel]

On Tue, Oct 26, 2021 at 05:27:38PM +0300, Vladimir Oltean wrote:
> fdb_insert() is not a descriptive name for this function, and also easy
> to confuse with __br_fdb_add(), fdb_add_entry(), br_fdb_update().
> Even more confusingly, it is not even related in any way with those
> functions, neither one calls the other.
> 
> Since fdb_insert() basically deals with the creation of a BR_FDB_LOCAL
> entry and is called only from functions where that is the intention:
> 
> - br_fdb_changeaddr
> - br_fdb_change_mac_address
> - br_fdb_insert
> 
> then rename it to fdb_add_local(), because its removal counterpart is
> called fdb_delete_local().

Good commit message

> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Re: [PATCH net-next 3/8] net: bridge: rename fdb_insert to fdb_add_local

[Nikolay Aleksandrov]

On 26/10/2021 17:27, Vladimir Oltean wrote:
> fdb_insert() is not a descriptive name for this function, and also easy
> to confuse with __br_fdb_add(), fdb_add_entry(), br_fdb_update().
> Even more confusingly, it is not even related in any way with those
> functions, neither one calls the other.
> 
> Since fdb_insert() basically deals with the creation of a BR_FDB_LOCAL
> entry and is called only from functions where that is the intention:
> 
> - br_fdb_changeaddr
> - br_fdb_change_mac_address
> - br_fdb_insert
> 
> then rename it to fdb_add_local(), because its removal counterpart is
> called fdb_delete_local().
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/bridge/br_fdb.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 

Indeed, the naming was confusing.
Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>

[PATCH net-next 4/8] net: bridge: rename br_fdb_insert to br_fdb_add_local

[Vladimir Oltean]

br_fdb_insert() is a wrapper over fdb_insert() that also takes the
bridge hash_lock.

With fdb_insert() being renamed to fdb_add_local(), rename
br_fdb_insert() to br_fdb_add_local().

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/bridge/br_fdb.c     | 4 ++--
 net/bridge/br_if.c      | 2 +-
 net/bridge/br_private.h | 4 ++--
 net/bridge/br_vlan.c    | 5 ++---
 4 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 668f87db7644..09e7a1dd9e3c 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -679,8 +679,8 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
 }
 
 /* Add entry for local address of interface */
-int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
-		  const unsigned char *addr, u16 vid)
+int br_fdb_add_local(struct net_bridge *br, struct net_bridge_port *source,
+		     const unsigned char *addr, u16 vid)
 {
 	int ret;
 
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index c11bba3e7ec0..c1183fef1f21 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -670,7 +670,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev,
 	else
 		netdev_set_rx_headroom(dev, br_hr);
 
-	if (br_fdb_insert(br, p, dev->dev_addr, 0))
+	if (br_fdb_add_local(br, p, dev->dev_addr, 0))
 		netdev_err(dev, "failed insert local address bridge forwarding table\n");
 
 	if (br->dev->addr_assign_type != NET_ADDR_SET) {
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 37ca76406f1e..705606fc2237 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -767,8 +767,8 @@ struct net_bridge_fdb_entry *br_fdb_find_rcu(struct net_bridge *br,
 int br_fdb_test_addr(struct net_device *dev, unsigned char *addr);
 int br_fdb_fillbuf(struct net_bridge *br, void *buf, unsigned long count,
 		   unsigned long off);
-int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
-		  const unsigned char *addr, u16 vid);
+int br_fdb_add_local(struct net_bridge *br, struct net_bridge_port *source,
+		     const unsigned char *addr, u16 vid);
 void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 		   const unsigned char *addr, u16 vid, unsigned long flags);
 
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 19f65ab91a02..57bd6ee72a07 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -293,7 +293,7 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags,
 
 	/* Add the dev mac and count the vlan only if it's usable */
 	if (br_vlan_should_use(v)) {
-		err = br_fdb_insert(br, p, dev->dev_addr, v->vid);
+		err = br_fdb_add_local(br, p, dev->dev_addr, v->vid);
 		if (err) {
 			br_err(br, "failed insert local address into bridge forwarding table\n");
 			goto out_filt;
@@ -683,8 +683,7 @@ static int br_vlan_add_existing(struct net_bridge *br,
 			goto err_flags;
 		}
 		/* It was only kept for port vlans, now make it real */
-		err = br_fdb_insert(br, NULL, br->dev->dev_addr,
-				    vlan->vid);
+		err = br_fdb_add_local(br, NULL, br->dev->dev_addr, vlan->vid);
 		if (err) {
 			br_err(br, "failed to insert local address into bridge forwarding table\n");
 			goto err_fdb_insert;
-- 
2.25.1

Re: [PATCH net-next 4/8] net: bridge: rename br_fdb_insert to br_fdb_add_local

[Ido Schimmel]

On Tue, Oct 26, 2021 at 05:27:39PM +0300, Vladimir Oltean wrote:
> br_fdb_insert() is a wrapper over fdb_insert() that also takes the
> bridge hash_lock.
> 
> With fdb_insert() being renamed to fdb_add_local(), rename
> br_fdb_insert() to br_fdb_add_local().
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Re: [PATCH net-next 4/8] net: bridge: rename br_fdb_insert to br_fdb_add_local

[Nikolay Aleksandrov]

On 26/10/2021 17:27, Vladimir Oltean wrote:
> br_fdb_insert() is a wrapper over fdb_insert() that also takes the
> bridge hash_lock.
> 
> With fdb_insert() being renamed to fdb_add_local(), rename
> br_fdb_insert() to br_fdb_add_local().
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/bridge/br_fdb.c     | 4 ++--
>  net/bridge/br_if.c      | 2 +-
>  net/bridge/br_private.h | 4 ++--
>  net/bridge/br_vlan.c    | 5 ++---
>  4 files changed, 7 insertions(+), 8 deletions(-)
> 

Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>

[PATCH net-next 5/8] net: bridge: reduce indentation level in fdb_create

[Vladimir Oltean]

We can express the same logic without an "if" condition as big as the
function, just return early if the kmem_cache_alloc() call fails.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/bridge/br_fdb.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 09e7a1dd9e3c..f2b909aedabf 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -382,23 +382,26 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
 					       unsigned long flags)
 {
 	struct net_bridge_fdb_entry *fdb;
+	int err;
 
 	fdb = kmem_cache_alloc(br_fdb_cache, GFP_ATOMIC);
-	if (fdb) {
-		memcpy(fdb->key.addr.addr, addr, ETH_ALEN);
-		WRITE_ONCE(fdb->dst, source);
-		fdb->key.vlan_id = vid;
-		fdb->flags = flags;
-		fdb->updated = fdb->used = jiffies;
-		if (rhashtable_lookup_insert_fast(&br->fdb_hash_tbl,
-						  &fdb->rhnode,
-						  br_fdb_rht_params)) {
-			kmem_cache_free(br_fdb_cache, fdb);
-			fdb = NULL;
-		} else {
-			hlist_add_head_rcu(&fdb->fdb_node, &br->fdb_list);
-		}
+	if (!fdb)
+		return NULL;
+
+	memcpy(fdb->key.addr.addr, addr, ETH_ALEN);
+	WRITE_ONCE(fdb->dst, source);
+	fdb->key.vlan_id = vid;
+	fdb->flags = flags;
+	fdb->updated = fdb->used = jiffies;
+	err = rhashtable_lookup_insert_fast(&br->fdb_hash_tbl, &fdb->rhnode,
+					    br_fdb_rht_params);
+	if (err) {
+		kmem_cache_free(br_fdb_cache, fdb);
+		return NULL;
 	}
+
+	hlist_add_head_rcu(&fdb->fdb_node, &br->fdb_list);
+
 	return fdb;
 }
 
-- 
2.25.1

Re: [PATCH net-next 5/8] net: bridge: reduce indentation level in fdb_create

[Ido Schimmel]

On Tue, Oct 26, 2021 at 05:27:40PM +0300, Vladimir Oltean wrote:
> We can express the same logic without an "if" condition as big as the
> function, just return early if the kmem_cache_alloc() call fails.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Re: [PATCH net-next 5/8] net: bridge: reduce indentation level in fdb_create

[Nikolay Aleksandrov]

On 26/10/2021 17:27, Vladimir Oltean wrote:
> We can express the same logic without an "if" condition as big as the
> function, just return early if the kmem_cache_alloc() call fails.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/bridge/br_fdb.c | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 

Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>

[PATCH net-next 6/8] net: bridge: move br_fdb_replay inside br_switchdev.c

[Vladimir Oltean]

br_fdb_replay is only called from switchdev code paths, so it makes
sense to be disabled if switchdev is not enabled in the first place.

As opposed to br_mdb_replay and br_vlan_replay which might be turned off
depending on bridge support for multicast and VLANs, FDB support is
always on. So moving br_mdb_replay and br_vlan_replay inside
br_switchdev.c would mean adding some #ifdef's in br_switchdev.c, so we
keep those where they are.

The reason for the movement is that in future changes there will be some
code reuse between br_switchdev_fdb_notify and br_fdb_replay.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/bridge/br_fdb.c       | 54 ---------------------------------------
 net/bridge/br_private.h   |  2 --
 net/bridge/br_switchdev.c | 54 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 56 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index f2b909aedabf..6ccda68bd473 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -759,60 +759,6 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 	}
 }
 
-static int br_fdb_replay_one(struct net_bridge *br, struct notifier_block *nb,
-			     const struct net_bridge_fdb_entry *fdb,
-			     unsigned long action, const void *ctx)
-{
-	const struct net_bridge_port *p = READ_ONCE(fdb->dst);
-	struct switchdev_notifier_fdb_info item;
-	int err;
-
-	item.addr = fdb->key.addr.addr;
-	item.vid = fdb->key.vlan_id;
-	item.added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
-	item.offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags);
-	item.is_local = test_bit(BR_FDB_LOCAL, &fdb->flags);
-	item.info.dev = (!p || item.is_local) ? br->dev : p->dev;
-	item.info.ctx = ctx;
-
-	err = nb->notifier_call(nb, action, &item);
-	return notifier_to_errno(err);
-}
-
-int br_fdb_replay(const struct net_device *br_dev, const void *ctx, bool adding,
-		  struct notifier_block *nb)
-{
-	struct net_bridge_fdb_entry *fdb;
-	struct net_bridge *br;
-	unsigned long action;
-	int err = 0;
-
-	if (!nb)
-		return 0;
-
-	if (!netif_is_bridge_master(br_dev))
-		return -EINVAL;
-
-	br = netdev_priv(br_dev);
-
-	if (adding)
-		action = SWITCHDEV_FDB_ADD_TO_DEVICE;
-	else
-		action = SWITCHDEV_FDB_DEL_TO_DEVICE;
-
-	rcu_read_lock();
-
-	hlist_for_each_entry_rcu(fdb, &br->fdb_list, fdb_node) {
-		err = br_fdb_replay_one(br, nb, fdb, action, ctx);
-		if (err)
-			break;
-	}
-
-	rcu_read_unlock();
-
-	return err;
-}
-
 /* Dump information about entries, in response to GETNEIGH */
 int br_fdb_dump(struct sk_buff *skb,
 		struct netlink_callback *cb,
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 705606fc2237..3c9327628060 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -792,8 +792,6 @@ int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
 			      bool swdev_notify);
 void br_fdb_offloaded_set(struct net_bridge *br, struct net_bridge_port *p,
 			  const unsigned char *addr, u16 vid, bool offloaded);
-int br_fdb_replay(const struct net_device *br_dev, const void *ctx, bool adding,
-		  struct notifier_block *nb);
 
 /* br_forward.c */
 enum br_pkt_type {
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 6bf518d78f02..8a45b1cfe06f 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -270,6 +270,60 @@ static void nbp_switchdev_del(struct net_bridge_port *p)
 	}
 }
 
+static int br_fdb_replay_one(struct net_bridge *br, struct notifier_block *nb,
+			     const struct net_bridge_fdb_entry *fdb,
+			     unsigned long action, const void *ctx)
+{
+	const struct net_bridge_port *p = READ_ONCE(fdb->dst);
+	struct switchdev_notifier_fdb_info item;
+	int err;
+
+	item.addr = fdb->key.addr.addr;
+	item.vid = fdb->key.vlan_id;
+	item.added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
+	item.offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags);
+	item.is_local = test_bit(BR_FDB_LOCAL, &fdb->flags);
+	item.info.dev = (!p || item.is_local) ? br->dev : p->dev;
+	item.info.ctx = ctx;
+
+	err = nb->notifier_call(nb, action, &item);
+	return notifier_to_errno(err);
+}
+
+static int br_fdb_replay(const struct net_device *br_dev, const void *ctx,
+			 bool adding, struct notifier_block *nb)
+{
+	struct net_bridge_fdb_entry *fdb;
+	struct net_bridge *br;
+	unsigned long action;
+	int err = 0;
+
+	if (!nb)
+		return 0;
+
+	if (!netif_is_bridge_master(br_dev))
+		return -EINVAL;
+
+	br = netdev_priv(br_dev);
+
+	if (adding)
+		action = SWITCHDEV_FDB_ADD_TO_DEVICE;
+	else
+		action = SWITCHDEV_FDB_DEL_TO_DEVICE;
+
+	rcu_read_lock();
+
+	hlist_for_each_entry_rcu(fdb, &br->fdb_list, fdb_node) {
+		err = br_fdb_replay_one(br, nb, fdb, action, ctx);
+		if (err)
+			break;
+	}
+
+	rcu_read_unlock();
+
+	return err;
+}
+
 static int nbp_switchdev_sync_objs(struct net_bridge_port *p, const void *ctx,
 				   struct notifier_block *atomic_nb,
 				   struct notifier_block *blocking_nb,
-- 
2.25.1

Re: [PATCH net-next 6/8] net: bridge: move br_fdb_replay inside br_switchdev.c

[Ido Schimmel]

On Tue, Oct 26, 2021 at 05:27:41PM +0300, Vladimir Oltean wrote:
> br_fdb_replay is only called from switchdev code paths, so it makes
> sense to be disabled if switchdev is not enabled in the first place.
> 
> As opposed to br_mdb_replay and br_vlan_replay which might be turned off
> depending on bridge support for multicast and VLANs, FDB support is
> always on. So moving br_mdb_replay and br_vlan_replay inside
> br_switchdev.c would mean adding some #ifdef's in br_switchdev.c, so we
> keep those where they are.

TBH, for consistency with br_mdb_replay() and br_vlan_replay(), it would
have been good to keep it where it is, but ...

> 
> The reason for the movement is that in future changes there will be some
> code reuse between br_switchdev_fdb_notify and br_fdb_replay.

this seems like a good reason, so:

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Nik, WDYT?

Re: [PATCH net-next 6/8] net: bridge: move br_fdb_replay inside br_switchdev.c

[Nikolay Aleksandrov]

On 27/10/2021 11:16, Ido Schimmel wrote:
> On Tue, Oct 26, 2021 at 05:27:41PM +0300, Vladimir Oltean wrote:
>> br_fdb_replay is only called from switchdev code paths, so it makes
>> sense to be disabled if switchdev is not enabled in the first place.
>>
>> As opposed to br_mdb_replay and br_vlan_replay which might be turned off
>> depending on bridge support for multicast and VLANs, FDB support is
>> always on. So moving br_mdb_replay and br_vlan_replay inside
>> br_switchdev.c would mean adding some #ifdef's in br_switchdev.c, so we
>> keep those where they are.
> 
> TBH, for consistency with br_mdb_replay() and br_vlan_replay(), it would
> have been good to keep it where it is, but ...
> 
>>
>> The reason for the movement is that in future changes there will be some
>> code reuse between br_switchdev_fdb_notify and br_fdb_replay.
> 
> this seems like a good reason, so:
> 
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> 
> Nik, WDYT?
> 

Good point, it'd be nice to have them all in one place, since they all deal
specifically with switchdev we can move them to br_switchdev.c. We can also
rename them similar to other functions in br_switchdev, e.g. br_switchdev_fdb_replay

Thanks,
 Nik

Re: [PATCH net-next 6/8] net: bridge: move br_fdb_replay inside br_switchdev.c

[Vladimir Oltean]

On Wed, Oct 27, 2021 at 11:28:23AM +0300, Nikolay Aleksandrov wrote:
> On 27/10/2021 11:16, Ido Schimmel wrote:
> > On Tue, Oct 26, 2021 at 05:27:41PM +0300, Vladimir Oltean wrote:
> >> br_fdb_replay is only called from switchdev code paths, so it makes
> >> sense to be disabled if switchdev is not enabled in the first place.
> >>
> >> As opposed to br_mdb_replay and br_vlan_replay which might be turned off
> >> depending on bridge support for multicast and VLANs, FDB support is
> >> always on. So moving br_mdb_replay and br_vlan_replay inside
> >> br_switchdev.c would mean adding some #ifdef's in br_switchdev.c, so we
> >> keep those where they are.
> > 
> > TBH, for consistency with br_mdb_replay() and br_vlan_replay(), it would
> > have been good to keep it where it is, but ...
> > 
> >>
> >> The reason for the movement is that in future changes there will be some
> >> code reuse between br_switchdev_fdb_notify and br_fdb_replay.
> > 
> > this seems like a good reason, so:
> > 
> > Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> > 
> > Nik, WDYT?
> > 
> 
> Good point, it'd be nice to have them all in one place, since they all deal
> specifically with switchdev we can move them to br_switchdev.c. We can also
> rename them similar to other functions in br_switchdev, e.g. br_switchdev_fdb_replay

Looks like we cam move a surprisingly large amount of code from br_mdb.c
to br_switchdev.c. The only problem is:

                                          this used to be called br_mdb_complete
                                                         |
                                                         v
net/bridge/br_switchdev.c: In function ‘br_switchdev_mdb_complete’:
net/bridge/br_switchdev.c:437:20: error: ‘struct net_bridge’ has no member named ‘multicast_lock’; did you mean ‘multicast_ctx’?
  437 |  spin_lock_bh(&br->multicast_lock);
      |                    ^~~~~~~~~~~~~~
      |                    multicast_ctx

Would you like me to introduce a set of br_multicast_lock() and
br_multicast_unlock() helpers that have shim definitions so that they
work when CONFIG_BRIDGE_IGMP_SNOOPING is disabled?

Anyway, I'd like to do this second part of refactoring in a second patch
series, if you don't mind.

Re: [PATCH net-next 6/8] net: bridge: move br_fdb_replay inside br_switchdev.c

[Vladimir Oltean]

On Wed, Oct 27, 2021 at 03:58:56PM +0300, Vladimir Oltean wrote:
> On Wed, Oct 27, 2021 at 11:28:23AM +0300, Nikolay Aleksandrov wrote:
> > On 27/10/2021 11:16, Ido Schimmel wrote:
> > > On Tue, Oct 26, 2021 at 05:27:41PM +0300, Vladimir Oltean wrote:
> > >> br_fdb_replay is only called from switchdev code paths, so it makes
> > >> sense to be disabled if switchdev is not enabled in the first place.
> > >>
> > >> As opposed to br_mdb_replay and br_vlan_replay which might be turned off
> > >> depending on bridge support for multicast and VLANs, FDB support is
> > >> always on. So moving br_mdb_replay and br_vlan_replay inside
> > >> br_switchdev.c would mean adding some #ifdef's in br_switchdev.c, so we
> > >> keep those where they are.
> > > 
> > > TBH, for consistency with br_mdb_replay() and br_vlan_replay(), it would
> > > have been good to keep it where it is, but ...
> > > 
> > >>
> > >> The reason for the movement is that in future changes there will be some
> > >> code reuse between br_switchdev_fdb_notify and br_fdb_replay.
> > > 
> > > this seems like a good reason, so:
> > > 
> > > Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> > > 
> > > Nik, WDYT?
> > > 
> > 
> > Good point, it'd be nice to have them all in one place, since they all deal
> > specifically with switchdev we can move them to br_switchdev.c. We can also
> > rename them similar to other functions in br_switchdev, e.g. br_switchdev_fdb_replay
> 
> Looks like we cam move a surprisingly large amount of code from br_mdb.c
> to br_switchdev.c. The only problem is:
> 
>                                           this used to be called br_mdb_complete
>                                                          |
>                                                          v
> net/bridge/br_switchdev.c: In function ‘br_switchdev_mdb_complete’:
> net/bridge/br_switchdev.c:437:20: error: ‘struct net_bridge’ has no member named ‘multicast_lock’; did you mean ‘multicast_ctx’?
>   437 |  spin_lock_bh(&br->multicast_lock);
>       |                    ^~~~~~~~~~~~~~
>       |                    multicast_ctx
> 
> Would you like me to introduce a set of br_multicast_lock() and
> br_multicast_unlock() helpers that have shim definitions so that they
> work when CONFIG_BRIDGE_IGMP_SNOOPING is disabled?
> 
> Anyway, I'd like to do this second part of refactoring in a second patch
> series, if you don't mind.

Ah, don't mind me, there's more. The entire &br->mdb_list is under
CONFIG_BRIDGE_IGMP_SNOOPING. So I guess my only option is to slap a big
#ifdef CONFIG_BRIDGE_IGMP_SNOOPING if I move these to br_switchdev.c.
I guess I might do that anyway.

[PATCH net-next 7/8] net: bridge: create a common function for populating switchdev FDB entries

[Vladimir Oltean]

There are two places where a switchdev FDB entry is constructed, one is
br_switchdev_fdb_notify() and the other is br_fdb_replay(). One uses a
struct initializer, and the other declares the structure as
uninitialized and populates the elements one by one.

One problem when introducing new members of struct
switchdev_notifier_fdb_info is that there is a risk for one of these
functions to run with an uninitialized value.

So centralize the logic of populating such structure into a dedicated
function. Being the primary location where these structures are created,
using an uninitialized variable and populating the members one by one
should be fine, since this one function is supposed to assign values to
all its members.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/bridge/br_switchdev.c | 41 +++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 8a45b1cfe06f..2fbe881cdfe2 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -122,28 +122,38 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p,
 	return 0;
 }
 
+static void br_switchdev_fdb_populate(struct net_bridge *br,
+				      struct switchdev_notifier_fdb_info *item,
+				      const struct net_bridge_fdb_entry *fdb,
+				      const void *ctx)
+{
+	const struct net_bridge_port *p = READ_ONCE(fdb->dst);
+
+	item->addr = fdb->key.addr.addr;
+	item->vid = fdb->key.vlan_id;
+	item->added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
+	item->offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags);
+	item->is_local = test_bit(BR_FDB_LOCAL, &fdb->flags);
+	item->info.dev = (!p || item->is_local) ? br->dev : p->dev;
+	item->info.ctx = ctx;
+}
+
 void
 br_switchdev_fdb_notify(struct net_bridge *br,
 			const struct net_bridge_fdb_entry *fdb, int type)
 {
-	const struct net_bridge_port *dst = READ_ONCE(fdb->dst);
-	struct switchdev_notifier_fdb_info info = {
-		.addr = fdb->key.addr.addr,
-		.vid = fdb->key.vlan_id,
-		.added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags),
-		.is_local = test_bit(BR_FDB_LOCAL, &fdb->flags),
-		.offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags),
-	};
-	struct net_device *dev = (!dst || info.is_local) ? br->dev : dst->dev;
+	struct switchdev_notifier_fdb_info item;
+
+	br_switchdev_fdb_populate(br, &item, fdb, NULL);
 
 	switch (type) {
 	case RTM_DELNEIGH:
 		call_switchdev_notifiers(SWITCHDEV_FDB_DEL_TO_DEVICE,
-					 dev, &info.info, NULL);
+					 item.info.dev, &item.info, NULL);
 		break;
 	case RTM_NEWNEIGH:
 		call_switchdev_notifiers(SWITCHDEV_FDB_ADD_TO_DEVICE,
-					 dev, &info.info, NULL);
+					 item.info.dev, &item.info, NULL);
 		break;
 	}
 }
@@ -274,17 +284,10 @@ static int br_fdb_replay_one(struct net_bridge *br, struct notifier_block *nb,
 			     const struct net_bridge_fdb_entry *fdb,
 			     unsigned long action, const void *ctx)
 {
-	const struct net_bridge_port *p = READ_ONCE(fdb->dst);
 	struct switchdev_notifier_fdb_info item;
 	int err;
 
-	item.addr = fdb->key.addr.addr;
-	item.vid = fdb->key.vlan_id;
-	item.added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
-	item.offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags);
-	item.is_local = test_bit(BR_FDB_LOCAL, &fdb->flags);
-	item.info.dev = (!p || item.is_local) ? br->dev : p->dev;
-	item.info.ctx = ctx;
+	br_switchdev_fdb_populate(br, &item, fdb, ctx);
 
 	err = nb->notifier_call(nb, action, &item);
 	return notifier_to_errno(err);
-- 
2.25.1

Re: [PATCH net-next 7/8] net: bridge: create a common function for populating switchdev FDB entries

[Ido Schimmel]

On Tue, Oct 26, 2021 at 05:27:42PM +0300, Vladimir Oltean wrote:
> There are two places where a switchdev FDB entry is constructed, one is
> br_switchdev_fdb_notify() and the other is br_fdb_replay(). One uses a
> struct initializer, and the other declares the structure as
> uninitialized and populates the elements one by one.
> 
> One problem when introducing new members of struct
> switchdev_notifier_fdb_info is that there is a risk for one of these
> functions to run with an uninitialized value.
> 
> So centralize the logic of populating such structure into a dedicated
> function. Being the primary location where these structures are created,
> using an uninitialized variable and populating the members one by one
> should be fine, since this one function is supposed to assign values to
> all its members.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Re: [PATCH net-next 7/8] net: bridge: create a common function for populating switchdev FDB entries

[Nikolay Aleksandrov]

On 26/10/2021 17:27, Vladimir Oltean wrote:
> There are two places where a switchdev FDB entry is constructed, one is
> br_switchdev_fdb_notify() and the other is br_fdb_replay(). One uses a
> struct initializer, and the other declares the structure as
> uninitialized and populates the elements one by one.
> 
> One problem when introducing new members of struct
> switchdev_notifier_fdb_info is that there is a risk for one of these
> functions to run with an uninitialized value.
> 
> So centralize the logic of populating such structure into a dedicated
> function. Being the primary location where these structures are created,
> using an uninitialized variable and populating the members one by one
> should be fine, since this one function is supposed to assign values to
> all its members.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/bridge/br_switchdev.c | 41 +++++++++++++++++++++------------------
>  1 file changed, 22 insertions(+), 19 deletions(-)
> 

Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>

[PATCH net-next 8/8] net: switchdev: merge switchdev_handle_fdb_{add,del}_to_device

[Vladimir Oltean]

To reduce code churn, the same patch makes multiple changes, since they
all touch the same lines:

1. The implementations for these two are identical, just with different
   function pointers. Reduce duplications and name the function pointers
   "mod_cb" instead of "add_cb" and "del_cb". Pass the event as argument.

2. Drop the "const" attribute from "orig_dev". If the driver needs to
   check whether orig_dev belongs to itself and then
   call_switchdev_notifiers(orig_dev, SWITCHDEV_FDB_OFFLOADED), it
   can't, because call_switchdev_notifiers takes a non-const struct
   net_device *.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/net/switchdev.h   |  48 +++---------
 net/dsa/slave.c           |  41 ++--------
 net/switchdev/switchdev.c | 156 ++++++--------------------------------
 3 files changed, 43 insertions(+), 202 deletions(-)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 60d806b6a5ae..d353793dfeb5 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -299,28 +299,16 @@ void switchdev_port_fwd_mark_set(struct net_device *dev,
 				 struct net_device *group_dev,
 				 bool joining);
 
-int switchdev_handle_fdb_add_to_device(struct net_device *dev,
+int switchdev_handle_fdb_event_to_device(struct net_device *dev, unsigned long event,
 		const struct switchdev_notifier_fdb_info *fdb_info,
 		bool (*check_cb)(const struct net_device *dev),
 		bool (*foreign_dev_check_cb)(const struct net_device *dev,
 					     const struct net_device *foreign_dev),
-		int (*add_cb)(struct net_device *dev,
-			      const struct net_device *orig_dev, const void *ctx,
+		int (*mod_cb)(struct net_device *dev, struct net_device *orig_dev,
+			      unsigned long event, const void *ctx,
 			      const struct switchdev_notifier_fdb_info *fdb_info),
-		int (*lag_add_cb)(struct net_device *dev,
-				  const struct net_device *orig_dev, const void *ctx,
-				  const struct switchdev_notifier_fdb_info *fdb_info));
-
-int switchdev_handle_fdb_del_to_device(struct net_device *dev,
-		const struct switchdev_notifier_fdb_info *fdb_info,
-		bool (*check_cb)(const struct net_device *dev),
-		bool (*foreign_dev_check_cb)(const struct net_device *dev,
-					     const struct net_device *foreign_dev),
-		int (*del_cb)(struct net_device *dev,
-			      const struct net_device *orig_dev, const void *ctx,
-			      const struct switchdev_notifier_fdb_info *fdb_info),
-		int (*lag_del_cb)(struct net_device *dev,
-				  const struct net_device *orig_dev, const void *ctx,
+		int (*lag_mod_cb)(struct net_device *dev, struct net_device *orig_dev,
+				  unsigned long event, const void *ctx,
 				  const struct switchdev_notifier_fdb_info *fdb_info));
 
 int switchdev_handle_port_obj_add(struct net_device *dev,
@@ -426,32 +414,16 @@ call_switchdev_blocking_notifiers(unsigned long val,
 }
 
 static inline int
-switchdev_handle_fdb_add_to_device(struct net_device *dev,
-		const struct switchdev_notifier_fdb_info *fdb_info,
-		bool (*check_cb)(const struct net_device *dev),
-		bool (*foreign_dev_check_cb)(const struct net_device *dev,
-					     const struct net_device *foreign_dev),
-		int (*add_cb)(struct net_device *dev,
-			      const struct net_device *orig_dev, const void *ctx,
-			      const struct switchdev_notifier_fdb_info *fdb_info),
-		int (*lag_add_cb)(struct net_device *dev,
-				  const struct net_device *orig_dev, const void *ctx,
-				  const struct switchdev_notifier_fdb_info *fdb_info))
-{
-	return 0;
-}
-
-static inline int
-switchdev_handle_fdb_del_to_device(struct net_device *dev,
+switchdev_handle_fdb_event_to_device(struct net_device *dev, unsigned long event,
 		const struct switchdev_notifier_fdb_info *fdb_info,
 		bool (*check_cb)(const struct net_device *dev),
 		bool (*foreign_dev_check_cb)(const struct net_device *dev,
 					     const struct net_device *foreign_dev),
-		int (*del_cb)(struct net_device *dev,
-			      const struct net_device *orig_dev, const void *ctx,
+		int (*mod_cb)(struct net_device *dev, struct net_device *orig_dev,
+			      unsigned long event, const void *ctx,
 			      const struct switchdev_notifier_fdb_info *fdb_info),
-		int (*lag_del_cb)(struct net_device *dev,
-				  const struct net_device *orig_dev, const void *ctx,
+		int (*lag_mod_cb)(struct net_device *dev, struct net_device *orig_dev,
+				  unsigned long event, const void *ctx,
 				  const struct switchdev_notifier_fdb_info *fdb_info))
 {
 	return 0;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index adcfb2cb4e61..f7675db09d2a 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2469,10 +2469,9 @@ static bool dsa_foreign_dev_check(const struct net_device *dev,
 }
 
 static int dsa_slave_fdb_event(struct net_device *dev,
-			       const struct net_device *orig_dev,
-			       const void *ctx,
-			       const struct switchdev_notifier_fdb_info *fdb_info,
-			       unsigned long event)
+			       struct net_device *orig_dev,
+			       unsigned long event, const void *ctx,
+			       const struct switchdev_notifier_fdb_info *fdb_info)
 {
 	struct dsa_switchdev_event_work *switchdev_work;
 	struct dsa_port *dp = dsa_slave_to_port(dev);
@@ -2528,24 +2527,6 @@ static int dsa_slave_fdb_event(struct net_device *dev,
 	return 0;
 }
 
-static int
-dsa_slave_fdb_add_to_device(struct net_device *dev,
-			    const struct net_device *orig_dev, const void *ctx,
-			    const struct switchdev_notifier_fdb_info *fdb_info)
-{
-	return dsa_slave_fdb_event(dev, orig_dev, ctx, fdb_info,
-				   SWITCHDEV_FDB_ADD_TO_DEVICE);
-}
-
-static int
-dsa_slave_fdb_del_to_device(struct net_device *dev,
-			    const struct net_device *orig_dev, const void *ctx,
-			    const struct switchdev_notifier_fdb_info *fdb_info)
-{
-	return dsa_slave_fdb_event(dev, orig_dev, ctx, fdb_info,
-				   SWITCHDEV_FDB_DEL_TO_DEVICE);
-}
-
 /* Called under rcu_read_lock() */
 static int dsa_slave_switchdev_event(struct notifier_block *unused,
 				     unsigned long event, void *ptr)
@@ -2560,18 +2541,12 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,
 						     dsa_slave_port_attr_set);
 		return notifier_from_errno(err);
 	case SWITCHDEV_FDB_ADD_TO_DEVICE:
-		err = switchdev_handle_fdb_add_to_device(dev, ptr,
-							 dsa_slave_dev_check,
-							 dsa_foreign_dev_check,
-							 dsa_slave_fdb_add_to_device,
-							 NULL);
-		return notifier_from_errno(err);
 	case SWITCHDEV_FDB_DEL_TO_DEVICE:
-		err = switchdev_handle_fdb_del_to_device(dev, ptr,
-							 dsa_slave_dev_check,
-							 dsa_foreign_dev_check,
-							 dsa_slave_fdb_del_to_device,
-							 NULL);
+		err = switchdev_handle_fdb_event_to_device(dev, event, ptr,
+							   dsa_slave_dev_check,
+							   dsa_foreign_dev_check,
+							   dsa_slave_fdb_event,
+							   NULL);
 		return notifier_from_errno(err);
 	default:
 		return NOTIFY_DONE;
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 0b2c18efc079..83460470e883 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -428,17 +428,17 @@ switchdev_lower_dev_find(struct net_device *dev,
 	return switchdev_priv.lower_dev;
 }
 
-static int __switchdev_handle_fdb_add_to_device(struct net_device *dev,
-		const struct net_device *orig_dev,
+static int __switchdev_handle_fdb_event_to_device(struct net_device *dev,
+		struct net_device *orig_dev, unsigned long event,
 		const struct switchdev_notifier_fdb_info *fdb_info,
 		bool (*check_cb)(const struct net_device *dev),
 		bool (*foreign_dev_check_cb)(const struct net_device *dev,
 					     const struct net_device *foreign_dev),
-		int (*add_cb)(struct net_device *dev,
-			      const struct net_device *orig_dev, const void *ctx,
+		int (*mod_cb)(struct net_device *dev, struct net_device *orig_dev,
+			      unsigned long event, const void *ctx,
 			      const struct switchdev_notifier_fdb_info *fdb_info),
-		int (*lag_add_cb)(struct net_device *dev,
-				  const struct net_device *orig_dev, const void *ctx,
+		int (*lag_mod_cb)(struct net_device *dev, struct net_device *orig_dev,
+				  unsigned long event, const void *ctx,
 				  const struct switchdev_notifier_fdb_info *fdb_info))
 {
 	const struct switchdev_notifier_info *info = &fdb_info->info;
@@ -447,17 +447,17 @@ static int __switchdev_handle_fdb_add_to_device(struct net_device *dev,
 	int err = -EOPNOTSUPP;
 
 	if (check_cb(dev))
-		return add_cb(dev, orig_dev, info->ctx, fdb_info);
+		return mod_cb(dev, orig_dev, event, info->ctx, fdb_info);
 
 	if (netif_is_lag_master(dev)) {
 		if (!switchdev_lower_dev_find(dev, check_cb, foreign_dev_check_cb))
 			goto maybe_bridged_with_us;
 
 		/* This is a LAG interface that we offload */
-		if (!lag_add_cb)
+		if (!lag_mod_cb)
 			return -EOPNOTSUPP;
 
-		return lag_add_cb(dev, orig_dev, info->ctx, fdb_info);
+		return lag_mod_cb(dev, orig_dev, event, info->ctx, fdb_info);
 	}
 
 	/* Recurse through lower interfaces in case the FDB entry is pointing
@@ -481,10 +481,10 @@ static int __switchdev_handle_fdb_add_to_device(struct net_device *dev,
 						      foreign_dev_check_cb))
 				continue;
 
-			err = __switchdev_handle_fdb_add_to_device(lower_dev, orig_dev,
-								   fdb_info, check_cb,
-								   foreign_dev_check_cb,
-								   add_cb, lag_add_cb);
+			err = __switchdev_handle_fdb_event_to_device(lower_dev, orig_dev,
+								     event, fdb_info, check_cb,
+								     foreign_dev_check_cb,
+								     mod_cb, lag_mod_cb);
 			if (err && err != -EOPNOTSUPP)
 				return err;
 		}
@@ -503,140 +503,34 @@ static int __switchdev_handle_fdb_add_to_device(struct net_device *dev,
 	if (!switchdev_lower_dev_find(br, check_cb, foreign_dev_check_cb))
 		return 0;
 
-	return __switchdev_handle_fdb_add_to_device(br, orig_dev, fdb_info,
-						    check_cb, foreign_dev_check_cb,
-						    add_cb, lag_add_cb);
+	return __switchdev_handle_fdb_event_to_device(br, orig_dev, event, fdb_info,
+						      check_cb, foreign_dev_check_cb,
+						      mod_cb, lag_mod_cb);
 }
 
-int switchdev_handle_fdb_add_to_device(struct net_device *dev,
+int switchdev_handle_fdb_event_to_device(struct net_device *dev, unsigned long event,
 		const struct switchdev_notifier_fdb_info *fdb_info,
 		bool (*check_cb)(const struct net_device *dev),
 		bool (*foreign_dev_check_cb)(const struct net_device *dev,
 					     const struct net_device *foreign_dev),
-		int (*add_cb)(struct net_device *dev,
-			      const struct net_device *orig_dev, const void *ctx,
+		int (*mod_cb)(struct net_device *dev, struct net_device *orig_dev,
+			      unsigned long event, const void *ctx,
 			      const struct switchdev_notifier_fdb_info *fdb_info),
-		int (*lag_add_cb)(struct net_device *dev,
-				  const struct net_device *orig_dev, const void *ctx,
+		int (*lag_mod_cb)(struct net_device *dev, struct net_device *orig_dev,
+				  unsigned long event, const void *ctx,
 				  const struct switchdev_notifier_fdb_info *fdb_info))
 {
 	int err;
 
-	err = __switchdev_handle_fdb_add_to_device(dev, dev, fdb_info,
-						   check_cb,
-						   foreign_dev_check_cb,
-						   add_cb, lag_add_cb);
+	err = __switchdev_handle_fdb_event_to_device(dev, dev, event, fdb_info,
+						     check_cb, foreign_dev_check_cb,
+						     mod_cb, lag_mod_cb);
 	if (err == -EOPNOTSUPP)
 		err = 0;
 
 	return err;
 }
-EXPORT_SYMBOL_GPL(switchdev_handle_fdb_add_to_device);
-
-static int __switchdev_handle_fdb_del_to_device(struct net_device *dev,
-		const struct net_device *orig_dev,
-		const struct switchdev_notifier_fdb_info *fdb_info,
-		bool (*check_cb)(const struct net_device *dev),
-		bool (*foreign_dev_check_cb)(const struct net_device *dev,
-					     const struct net_device *foreign_dev),
-		int (*del_cb)(struct net_device *dev,
-			      const struct net_device *orig_dev, const void *ctx,
-			      const struct switchdev_notifier_fdb_info *fdb_info),
-		int (*lag_del_cb)(struct net_device *dev,
-				  const struct net_device *orig_dev, const void *ctx,
-				  const struct switchdev_notifier_fdb_info *fdb_info))
-{
-	const struct switchdev_notifier_info *info = &fdb_info->info;
-	struct net_device *br, *lower_dev;
-	struct list_head *iter;
-	int err = -EOPNOTSUPP;
-
-	if (check_cb(dev))
-		return del_cb(dev, orig_dev, info->ctx, fdb_info);
-
-	if (netif_is_lag_master(dev)) {
-		if (!switchdev_lower_dev_find(dev, check_cb, foreign_dev_check_cb))
-			goto maybe_bridged_with_us;
-
-		/* This is a LAG interface that we offload */
-		if (!lag_del_cb)
-			return -EOPNOTSUPP;
-
-		return lag_del_cb(dev, orig_dev, info->ctx, fdb_info);
-	}
-
-	/* Recurse through lower interfaces in case the FDB entry is pointing
-	 * towards a bridge device.
-	 */
-	if (netif_is_bridge_master(dev)) {
-		if (!switchdev_lower_dev_find(dev, check_cb, foreign_dev_check_cb))
-			return 0;
-
-		/* This is a bridge interface that we offload */
-		netdev_for_each_lower_dev(dev, lower_dev, iter) {
-			/* Do not propagate FDB entries across bridges */
-			if (netif_is_bridge_master(lower_dev))
-				continue;
-
-			/* Bridge ports might be either us, or LAG interfaces
-			 * that we offload.
-			 */
-			if (!check_cb(lower_dev) &&
-			    !switchdev_lower_dev_find(lower_dev, check_cb,
-						      foreign_dev_check_cb))
-				continue;
-
-			err = __switchdev_handle_fdb_del_to_device(lower_dev, orig_dev,
-								   fdb_info, check_cb,
-								   foreign_dev_check_cb,
-								   del_cb, lag_del_cb);
-			if (err && err != -EOPNOTSUPP)
-				return err;
-		}
-
-		return 0;
-	}
-
-maybe_bridged_with_us:
-	/* Event is neither on a bridge nor a LAG. Check whether it is on an
-	 * interface that is in a bridge with us.
-	 */
-	br = netdev_master_upper_dev_get_rcu(dev);
-	if (!br || !netif_is_bridge_master(br))
-		return 0;
-
-	if (!switchdev_lower_dev_find(br, check_cb, foreign_dev_check_cb))
-		return 0;
-
-	return __switchdev_handle_fdb_del_to_device(br, orig_dev, fdb_info,
-						    check_cb, foreign_dev_check_cb,
-						    del_cb, lag_del_cb);
-}
-
-int switchdev_handle_fdb_del_to_device(struct net_device *dev,
-		const struct switchdev_notifier_fdb_info *fdb_info,
-		bool (*check_cb)(const struct net_device *dev),
-		bool (*foreign_dev_check_cb)(const struct net_device *dev,
-					     const struct net_device *foreign_dev),
-		int (*del_cb)(struct net_device *dev,
-			      const struct net_device *orig_dev, const void *ctx,
-			      const struct switchdev_notifier_fdb_info *fdb_info),
-		int (*lag_del_cb)(struct net_device *dev,
-				  const struct net_device *orig_dev, const void *ctx,
-				  const struct switchdev_notifier_fdb_info *fdb_info))
-{
-	int err;
-
-	err = __switchdev_handle_fdb_del_to_device(dev, dev, fdb_info,
-						   check_cb,
-						   foreign_dev_check_cb,
-						   del_cb, lag_del_cb);
-	if (err == -EOPNOTSUPP)
-		err = 0;
-
-	return err;
-}
-EXPORT_SYMBOL_GPL(switchdev_handle_fdb_del_to_device);
+EXPORT_SYMBOL_GPL(switchdev_handle_fdb_event_to_device);
 
 static int __switchdev_handle_port_obj_add(struct net_device *dev,
 			struct switchdev_notifier_port_obj_info *port_obj_info,
-- 
2.25.1

Re: [PATCH net-next 8/8] net: switchdev: merge switchdev_handle_fdb_{add,del}_to_device

[Ido Schimmel]

On Tue, Oct 26, 2021 at 05:27:43PM +0300, Vladimir Oltean wrote:
> To reduce code churn, the same patch makes multiple changes, since they
> all touch the same lines:
> 
> 1. The implementations for these two are identical, just with different
>    function pointers. Reduce duplications and name the function pointers
>    "mod_cb" instead of "add_cb" and "del_cb". Pass the event as argument.
> 
> 2. Drop the "const" attribute from "orig_dev". If the driver needs to
>    check whether orig_dev belongs to itself and then
>    call_switchdev_notifiers(orig_dev, SWITCHDEV_FDB_OFFLOADED), it
>    can't, because call_switchdev_notifiers takes a non-const struct
>    net_device *.

Regarding 2, I don't mind about the code change itself, but can you
expand on the motivation? Is this required for a subsequent patchset you
plan to submit?

> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Re: [PATCH net-next 8/8] net: switchdev: merge switchdev_handle_fdb_{add,del}_to_device

[Vladimir Oltean]

On Wed, Oct 27, 2021 at 11:46:08AM +0300, Ido Schimmel wrote:
> On Tue, Oct 26, 2021 at 05:27:43PM +0300, Vladimir Oltean wrote:
> > To reduce code churn, the same patch makes multiple changes, since they
> > all touch the same lines:
> > 
> > 1. The implementations for these two are identical, just with different
> >    function pointers. Reduce duplications and name the function pointers
> >    "mod_cb" instead of "add_cb" and "del_cb". Pass the event as argument.
> > 
> > 2. Drop the "const" attribute from "orig_dev". If the driver needs to
> >    check whether orig_dev belongs to itself and then
> >    call_switchdev_notifiers(orig_dev, SWITCHDEV_FDB_OFFLOADED), it
> >    can't, because call_switchdev_notifiers takes a non-const struct
> >    net_device *.
> 
> Regarding 2, I don't mind about the code change itself, but can you
> expand on the motivation? Is this required for a subsequent patchset you
> plan to submit?

Yes, I have a change that calls SWITCHDEV_FDB_OFFLOADED on the orig_dev
(which is always a bridge port, or the bridge itself) instead of the DSA
interface that may be beneath it.

If I understand correctly, the purpose of BR_FDB_OFFLOADED is simply to
show the user that it is offloaded, right?

Things get a little bit interesting when we sniff an FDB event on a
foreign interface that's in a bridge with us, which we trap to the CPU.
There we would be basically telling the user that the FDB entry towards,
say, an Intel card is offloaded, which is not wrong, because it kind of
is, being programmed to hardware, but it's also not "offloaded" in the
sense that there is a data path towards that port which bypasses the
CPU. But that is never the case anyway. When you have a bridge with
mixed hardware domains, then an "offloaded" FDB entry might not always
be offloaded, depending on the hardware domain of the source port.

> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Re: [PATCH net-next 0/8] Bridge FDB refactoring

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Tue, 26 Oct 2021 17:27:35 +0300 you wrote:
> This series refactors the br_fdb.c, br_switchdev.c and switchdev.c files
> to offer the same level of functionality with a bit less code, and to
> clarify the purpose of some functions.
> 
> No functional change intended.
> 
> Vladimir Oltean (8):
>   net: bridge: remove fdb_notify forward declaration
>   net: bridge: remove fdb_insert forward declaration
>   net: bridge: rename fdb_insert to fdb_add_local
>   net: bridge: rename br_fdb_insert to br_fdb_add_local
>   net: bridge: reduce indentation level in fdb_create
>   net: bridge: move br_fdb_replay inside br_switchdev.c
>   net: bridge: create a common function for populating switchdev FDB
>     entries
>   net: switchdev: merge switchdev_handle_fdb_{add,del}_to_device
> 
> [...]

Here is the summary with links:
  - [net-next,1/8] net: bridge: remove fdb_notify forward declaration
    https://git.kernel.org/netdev/net-next/c/4682048af0c8
  - [net-next,2/8] net: bridge: remove fdb_insert forward declaration
    https://git.kernel.org/netdev/net-next/c/5f94a5e276ae
  - [net-next,3/8] net: bridge: rename fdb_insert to fdb_add_local
    https://git.kernel.org/netdev/net-next/c/4731b6d6b257
  - [net-next,4/8] net: bridge: rename br_fdb_insert to br_fdb_add_local
    https://git.kernel.org/netdev/net-next/c/f6814fdcfe1b
  - [net-next,5/8] net: bridge: reduce indentation level in fdb_create
    https://git.kernel.org/netdev/net-next/c/9574fb558044
  - [net-next,6/8] net: bridge: move br_fdb_replay inside br_switchdev.c
    https://git.kernel.org/netdev/net-next/c/5cda5272a460
  - [net-next,7/8] net: bridge: create a common function for populating switchdev FDB entries
    https://git.kernel.org/netdev/net-next/c/fab9eca88410
  - [net-next,8/8] net: switchdev: merge switchdev_handle_fdb_{add,del}_to_device
    https://git.kernel.org/netdev/net-next/c/716a30a97a52

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 0/8] Bridge FDB refactoring

[Nikolay Aleksandrov]

On 27/10/2021 17:40, patchwork-bot+netdevbpf@kernel.org wrote:
> Hello:
> 
> This series was applied to netdev/net-next.git (master)
> by David S. Miller <davem@davemloft.net>:
> 
> On Tue, 26 Oct 2021 17:27:35 +0300 you wrote:
>> This series refactors the br_fdb.c, br_switchdev.c and switchdev.c files
>> to offer the same level of functionality with a bit less code, and to
>> clarify the purpose of some functions.
>>
>> No functional change intended.
>>
>> Vladimir Oltean (8):
>>   net: bridge: remove fdb_notify forward declaration
>>   net: bridge: remove fdb_insert forward declaration
>>   net: bridge: rename fdb_insert to fdb_add_local
>>   net: bridge: rename br_fdb_insert to br_fdb_add_local
>>   net: bridge: reduce indentation level in fdb_create
>>   net: bridge: move br_fdb_replay inside br_switchdev.c
>>   net: bridge: create a common function for populating switchdev FDB
>>     entries
>>   net: switchdev: merge switchdev_handle_fdb_{add,del}_to_device
>>
>> [...]
> 
> Here is the summary with links:
>   - [net-next,1/8] net: bridge: remove fdb_notify forward declaration
>     https://git.kernel.org/netdev/net-next/c/4682048af0c8
>   - [net-next,2/8] net: bridge: remove fdb_insert forward declaration
>     https://git.kernel.org/netdev/net-next/c/5f94a5e276ae
>   - [net-next,3/8] net: bridge: rename fdb_insert to fdb_add_local
>     https://git.kernel.org/netdev/net-next/c/4731b6d6b257
>   - [net-next,4/8] net: bridge: rename br_fdb_insert to br_fdb_add_local
>     https://git.kernel.org/netdev/net-next/c/f6814fdcfe1b
>   - [net-next,5/8] net: bridge: reduce indentation level in fdb_create
>     https://git.kernel.org/netdev/net-next/c/9574fb558044
>   - [net-next,6/8] net: bridge: move br_fdb_replay inside br_switchdev.c
>     https://git.kernel.org/netdev/net-next/c/5cda5272a460
>   - [net-next,7/8] net: bridge: create a common function for populating switchdev FDB entries
>     https://git.kernel.org/netdev/net-next/c/fab9eca88410
>   - [net-next,8/8] net: switchdev: merge switchdev_handle_fdb_{add,del}_to_device
>     https://git.kernel.org/netdev/net-next/c/716a30a97a52
> 
> You are awesome, thank you!
> 

There was a discussion about patch 06 which we agreed have to turn into its own series
with more changes. Vladimir, since the set got applied please send a follow-up to
finish those changes.

Thanks,
 Nik

Re: [PATCH net-next 0/8] Bridge FDB refactoring

[Vladimir Oltean]

On Wed, 27 Oct 2021 at 17:44, Nikolay Aleksandrov <nikolay@nvidia.com> wrote:
>
> On 27/10/2021 17:40, patchwork-bot+netdevbpf@kernel.org wrote:
> > Hello:
> >
> > This series was applied to netdev/net-next.git (master)
> > by David S. Miller <davem@davemloft.net>:
> >
> > On Tue, 26 Oct 2021 17:27:35 +0300 you wrote:
> >> This series refactors the br_fdb.c, br_switchdev.c and switchdev.c files
> >> to offer the same level of functionality with a bit less code, and to
> >> clarify the purpose of some functions.
> >>
> >> No functional change intended.
> >>
> >> Vladimir Oltean (8):
> >>   net: bridge: remove fdb_notify forward declaration
> >>   net: bridge: remove fdb_insert forward declaration
> >>   net: bridge: rename fdb_insert to fdb_add_local
> >>   net: bridge: rename br_fdb_insert to br_fdb_add_local
> >>   net: bridge: reduce indentation level in fdb_create
> >>   net: bridge: move br_fdb_replay inside br_switchdev.c
> >>   net: bridge: create a common function for populating switchdev FDB
> >>     entries
> >>   net: switchdev: merge switchdev_handle_fdb_{add,del}_to_device
> >>
> >> [...]
> >
> > Here is the summary with links:
> >   - [net-next,1/8] net: bridge: remove fdb_notify forward declaration
> >     https://git.kernel.org/netdev/net-next/c/4682048af0c8
> >   - [net-next,2/8] net: bridge: remove fdb_insert forward declaration
> >     https://git.kernel.org/netdev/net-next/c/5f94a5e276ae
> >   - [net-next,3/8] net: bridge: rename fdb_insert to fdb_add_local
> >     https://git.kernel.org/netdev/net-next/c/4731b6d6b257
> >   - [net-next,4/8] net: bridge: rename br_fdb_insert to br_fdb_add_local
> >     https://git.kernel.org/netdev/net-next/c/f6814fdcfe1b
> >   - [net-next,5/8] net: bridge: reduce indentation level in fdb_create
> >     https://git.kernel.org/netdev/net-next/c/9574fb558044
> >   - [net-next,6/8] net: bridge: move br_fdb_replay inside br_switchdev.c
> >     https://git.kernel.org/netdev/net-next/c/5cda5272a460
> >   - [net-next,7/8] net: bridge: create a common function for populating switchdev FDB entries
> >     https://git.kernel.org/netdev/net-next/c/fab9eca88410
> >   - [net-next,8/8] net: switchdev: merge switchdev_handle_fdb_{add,del}_to_device
> >     https://git.kernel.org/netdev/net-next/c/716a30a97a52
> >
> > You are awesome, thank you!
> >
>
> There was a discussion about patch 06 which we agreed have to turn into its own series
> with more changes. Vladimir, since the set got applied please send a follow-up to
> finish those changes.

Wait a minute, even I got the impression that the next series I'll be
sending would be completely separate from this one...

>
> Thanks,
>  Nik
>

Re: [PATCH net-next 0/8] Bridge FDB refactoring

[Vladimir Oltean]

On Wed, 27 Oct 2021 at 17:46, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Wed, 27 Oct 2021 at 17:44, Nikolay Aleksandrov <nikolay@nvidia.com> wrote:
> >
> > On 27/10/2021 17:40, patchwork-bot+netdevbpf@kernel.org wrote:
> > > Hello:
> > >
> > > This series was applied to netdev/net-next.git (master)
> > > by David S. Miller <davem@davemloft.net>:
> > >
> > > On Tue, 26 Oct 2021 17:27:35 +0300 you wrote:
> > >> This series refactors the br_fdb.c, br_switchdev.c and switchdev.c files
> > >> to offer the same level of functionality with a bit less code, and to
> > >> clarify the purpose of some functions.
> > >>
> > >> No functional change intended.
> > >>
> > >> Vladimir Oltean (8):
> > >>   net: bridge: remove fdb_notify forward declaration
> > >>   net: bridge: remove fdb_insert forward declaration
> > >>   net: bridge: rename fdb_insert to fdb_add_local
> > >>   net: bridge: rename br_fdb_insert to br_fdb_add_local
> > >>   net: bridge: reduce indentation level in fdb_create
> > >>   net: bridge: move br_fdb_replay inside br_switchdev.c
> > >>   net: bridge: create a common function for populating switchdev FDB
> > >>     entries
> > >>   net: switchdev: merge switchdev_handle_fdb_{add,del}_to_device
> > >>
> > >> [...]
> > >
> > > Here is the summary with links:
> > >   - [net-next,1/8] net: bridge: remove fdb_notify forward declaration
> > >     https://git.kernel.org/netdev/net-next/c/4682048af0c8
> > >   - [net-next,2/8] net: bridge: remove fdb_insert forward declaration
> > >     https://git.kernel.org/netdev/net-next/c/5f94a5e276ae
> > >   - [net-next,3/8] net: bridge: rename fdb_insert to fdb_add_local
> > >     https://git.kernel.org/netdev/net-next/c/4731b6d6b257
> > >   - [net-next,4/8] net: bridge: rename br_fdb_insert to br_fdb_add_local
> > >     https://git.kernel.org/netdev/net-next/c/f6814fdcfe1b
> > >   - [net-next,5/8] net: bridge: reduce indentation level in fdb_create
> > >     https://git.kernel.org/netdev/net-next/c/9574fb558044
> > >   - [net-next,6/8] net: bridge: move br_fdb_replay inside br_switchdev.c
> > >     https://git.kernel.org/netdev/net-next/c/5cda5272a460
> > >   - [net-next,7/8] net: bridge: create a common function for populating switchdev FDB entries
> > >     https://git.kernel.org/netdev/net-next/c/fab9eca88410
> > >   - [net-next,8/8] net: switchdev: merge switchdev_handle_fdb_{add,del}_to_device
> > >     https://git.kernel.org/netdev/net-next/c/716a30a97a52
> > >
> > > You are awesome, thank you!
> > >
> >
> > There was a discussion about patch 06 which we agreed have to turn into its own series
> > with more changes. Vladimir, since the set got applied please send a follow-up to
> > finish those changes.
>
> Wait a minute, even I got the impression that the next series I'll be
> sending would be completely separate from this one...

To be clear, what I'm preparing for that second series is to:
- move all of br_vlan_replay, br_mdb_replay into br_switchdev.c
- consistently name all functions to br_switchdev_*
- refactor the switchdev logic from br_mdb_notify into a new
br_switchdev_mdb_notify for symmetry with fdb_notify and
br_switchdev_fdb_notify
But I'm still working on it, because the VLAN and IGMP snooping code
is conditionally compiled, making it a bit harder :)

So the current placement of the br_fdb_replay function is not bad,
according to the follow-up changes I am going to make.