diff mbox series

[net-next,2/3] net: sparx5: add list for mdb entries in driver

Message ID 20220822140800.2651029-3-casper.casan@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: sparx5: add mrouter support | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1105 this patch: 1105
netdev/cc_maintainers warning 3 maintainers not CCed: rmk+kernel@armlinux.org.uk linux-arm-kernel@lists.infradead.org yang.lee@linux.alibaba.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1105 this patch: 1105
netdev/checkpatch warning WARNING: line length of 87 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Casper Andersson Aug. 22, 2022, 2:07 p.m. UTC
Keep track of all mdb entries in software for easy access.

Signed-off-by: Casper Andersson <casper.casan@gmail.com>
---
 .../ethernet/microchip/sparx5/sparx5_main.c   |   3 +
 .../ethernet/microchip/sparx5/sparx5_main.h   |  13 ++
 .../microchip/sparx5/sparx5_switchdev.c       | 196 ++++++++++--------
 3 files changed, 130 insertions(+), 82 deletions(-)

Comments

Andrew Lunn Aug. 22, 2022, 3:21 p.m. UTC | #1
> +struct sparx5_mdb_entry {
> +	struct list_head list;
> +	unsigned char addr[ETH_ALEN];
> +	u16 vid;
> +	DECLARE_BITMAP(port_mask, SPX5_PORTS);
> +	bool cpu_copy;
> +	u16 pgid_idx;
> +};

You have a number of holes in that structure. Maybe this is better:

> +struct sparx5_mdb_entry {
> +	struct list_head list;
> +	DECLARE_BITMAP(port_mask, SPX5_PORTS);
> +	unsigned char addr[ETH_ALEN];
> +	bool cpu_copy;
> +	u16 vid;
> +	u16 pgid_idx;
> +};

Hopefully the compiler can pack the bool straight after the 6 byte MAC
address. And the two u16 should make one u32.

> +static int sparx5_alloc_mdb_entry(struct sparx5 *sparx5,
> +				  const unsigned char *addr,
> +				  u16 vid,
> +				  struct sparx5_mdb_entry **entry_out)
> +{
> +	struct sparx5_mdb_entry *entry;
> +	u16 pgid_idx;
> +	int err;
> +
> +	entry = devm_kzalloc(sparx5->dev, sizeof(struct sparx5_mdb_entry), GFP_ATOMIC);

Does devm_kzalloc make sense here? A MDB entry has a much shorter life
time than the driver. devm has overheads, so it is good for large
allocations which last as long as the device, but less so for lots of
small short lives structures.

      Andrew
Casper Andersson Aug. 23, 2022, 7:52 a.m. UTC | #2
On 2022-08-22 17:21, Andrew Lunn wrote:
> > +struct sparx5_mdb_entry {
> > +	struct list_head list;
> > +	unsigned char addr[ETH_ALEN];
> > +	u16 vid;
> > +	DECLARE_BITMAP(port_mask, SPX5_PORTS);
> > +	bool cpu_copy;
> > +	u16 pgid_idx;
> > +};
> 
> You have a number of holes in that structure. Maybe this is better:
> 
> > +struct sparx5_mdb_entry {
> > +	struct list_head list;
> > +	DECLARE_BITMAP(port_mask, SPX5_PORTS);
> > +	unsigned char addr[ETH_ALEN];
> > +	bool cpu_copy;
> > +	u16 vid;
> > +	u16 pgid_idx;
> > +};
> 
> Hopefully the compiler can pack the bool straight after the 6 byte MAC
> address. And the two u16 should make one u32.

I had not considered that. Will fix for v2 and keep in mind for the
future!

> > +static int sparx5_alloc_mdb_entry(struct sparx5 *sparx5,
> > +				  const unsigned char *addr,
> > +				  u16 vid,
> > +				  struct sparx5_mdb_entry **entry_out)
> > +{
> > +	struct sparx5_mdb_entry *entry;
> > +	u16 pgid_idx;
> > +	int err;
> > +
> > +	entry = devm_kzalloc(sparx5->dev, sizeof(struct sparx5_mdb_entry), GFP_ATOMIC);
> 
> Does devm_kzalloc make sense here? A MDB entry has a much shorter life
> time than the driver. devm has overheads, so it is good for large
> allocations which last as long as the device, but less so for lots of
> small short lives structures.

You're right. Reading up on it I can see why kzalloc would make more
sense. Will fix! Do I have to free any remaining mdb entries when the
module is unloaded? Or is it able to handle that by deleting them when
unregistering?

Thanks for the feedback.

Best regards
Casper
Steen Hegelund Aug. 23, 2022, 9:44 a.m. UTC | #3
On Mon, 2022-08-22 at 16:07 +0200, Casper Andersson wrote:
> +static void sparx5_free_mdb_entry(struct sparx5 *sparx5,
> +                                 const unsigned char *addr,
> +                                 u16 vid)
> +{
> +       struct sparx5_mdb_entry *entry, *tmp;
> +
> +       mutex_lock(&sparx5->mdb_lock);
> +       list_for_each_entry_safe(entry, tmp, &sparx5->mdb_entries, list) {
> +               if ((vid == 0 || entry->vid == vid) &&
> +                   ether_addr_equal(addr, entry->addr)) {
> +                       list_del(&entry->list);
> +
> +                       sparx5_pgid_free(sparx5, entry->pgid_idx);
> +
> +                       devm_kfree(sparx5->dev, entry);

Could you not also bail out here, like you do below?

> +               }
> +       }
> +       mutex_unlock(&sparx5->mdb_lock);
> +}
> +
> 
> --
> 2.34.1
> 

BR
Steen
diff mbox series

Patch

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
index 01be7bd84181..ad598cf8ef44 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
@@ -661,6 +661,9 @@  static int sparx5_start(struct sparx5 *sparx5)
 	queue_delayed_work(sparx5->mact_queue, &sparx5->mact_work,
 			   SPX5_MACT_PULL_DELAY);
 
+	mutex_init(&sparx5->mdb_lock);
+	INIT_LIST_HEAD(&sparx5->mdb_entries);
+
 	err = sparx5_register_netdevs(sparx5);
 	if (err)
 		return err;
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
index b197129044b5..7c98af99c4f3 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
@@ -215,6 +215,15 @@  struct sparx5_skb_cb {
 	unsigned long jiffies;
 };
 
+struct sparx5_mdb_entry {
+	struct list_head list;
+	unsigned char addr[ETH_ALEN];
+	u16 vid;
+	DECLARE_BITMAP(port_mask, SPX5_PORTS);
+	bool cpu_copy;
+	u16 pgid_idx;
+};
+
 #define SPARX5_PTP_TIMEOUT		msecs_to_jiffies(10)
 #define SPARX5_SKB_CB(skb) \
 	((struct sparx5_skb_cb *)((skb)->cb))
@@ -256,6 +265,10 @@  struct sparx5 {
 	struct list_head mact_entries;
 	/* mac table list (mact_entries) mutex */
 	struct mutex mact_lock;
+	/* SW MDB table */
+	struct list_head mdb_entries;
+	/* mdb list mutex */
+	struct mutex mdb_lock;
 	struct delayed_work mact_work;
 	struct workqueue_struct *mact_queue;
 	/* Board specifics */
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c b/drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c
index ec07f7d0528c..dd3290168bcc 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c
@@ -386,16 +386,93 @@  static int sparx5_handle_port_vlan_add(struct net_device *dev,
 				  v->flags & BRIDGE_VLAN_INFO_UNTAGGED);
 }
 
+static int sparx5_alloc_mdb_entry(struct sparx5 *sparx5,
+				  const unsigned char *addr,
+				  u16 vid,
+				  struct sparx5_mdb_entry **entry_out)
+{
+	struct sparx5_mdb_entry *entry;
+	u16 pgid_idx;
+	int err;
+
+	entry = devm_kzalloc(sparx5->dev, sizeof(struct sparx5_mdb_entry), GFP_ATOMIC);
+	if (!entry)
+		return -ENOMEM;
+
+	err = sparx5_pgid_alloc_mcast(sparx5, &pgid_idx);
+	if (err) {
+		devm_kfree(sparx5->dev, entry);
+		return err;
+	}
+
+	memcpy(entry->addr, addr, ETH_ALEN);
+	entry->vid = vid;
+	entry->pgid_idx = pgid_idx;
+
+	mutex_lock(&sparx5->mdb_lock);
+	list_add_tail(&entry->list, &sparx5->mdb_entries);
+	mutex_unlock(&sparx5->mdb_lock);
+
+	*entry_out = entry;
+	return 0;
+}
+
+static void sparx5_free_mdb_entry(struct sparx5 *sparx5,
+				  const unsigned char *addr,
+				  u16 vid)
+{
+	struct sparx5_mdb_entry *entry, *tmp;
+
+	mutex_lock(&sparx5->mdb_lock);
+	list_for_each_entry_safe(entry, tmp, &sparx5->mdb_entries, list) {
+		if ((vid == 0 || entry->vid == vid) &&
+		    ether_addr_equal(addr, entry->addr)) {
+			list_del(&entry->list);
+
+			sparx5_pgid_free(sparx5, entry->pgid_idx);
+
+			devm_kfree(sparx5->dev, entry);
+		}
+	}
+	mutex_unlock(&sparx5->mdb_lock);
+}
+
+static struct sparx5_mdb_entry *sparx5_mdb_get_entry(struct sparx5 *sparx5,
+						     const unsigned char *addr,
+						     u16 vid)
+{
+	struct sparx5_mdb_entry *e, *found = NULL;
+
+	mutex_lock(&sparx5->mdb_lock);
+	list_for_each_entry(e, &sparx5->mdb_entries, list) {
+		if (ether_addr_equal(e->addr, addr) && e->vid == vid) {
+			found = e;
+			goto out;
+		}
+	}
+
+out:
+	mutex_unlock(&sparx5->mdb_lock);
+	return found;
+}
+
+static void sparx5_cpu_copy_ena(struct sparx5 *spx5, u16 pgid, bool enable)
+{
+	spx5_rmw(ANA_AC_PGID_MISC_CFG_PGID_CPU_COPY_ENA_SET(enable),
+		 ANA_AC_PGID_MISC_CFG_PGID_CPU_COPY_ENA, spx5,
+		 ANA_AC_PGID_MISC_CFG(pgid));
+}
+
 static int sparx5_handle_port_mdb_add(struct net_device *dev,
 				      struct notifier_block *nb,
 				      const struct switchdev_obj_port_mdb *v)
 {
 	struct sparx5_port *port = netdev_priv(dev);
 	struct sparx5 *spx5 = port->sparx5;
-	u16 pgid_idx, vid;
-	u32 mact_entry;
+	struct sparx5_mdb_entry *entry;
 	bool is_host;
-	int res, err;
+	int err;
+	u16 vid;
 
 	if (!sparx5_netdevice_check(dev))
 		return -EOPNOTSUPP;
@@ -410,66 +487,25 @@  static int sparx5_handle_port_mdb_add(struct net_device *dev,
 	else
 		vid = v->vid;
 
-	res = sparx5_mact_find(spx5, v->addr, vid, &mact_entry);
-
-	if (res == 0) {
-		pgid_idx = LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_ADDR_GET(mact_entry);
-
-		/* MC_IDX starts after the port masks in the PGID table */
-		pgid_idx += SPX5_PORTS;
-
-		if (is_host)
-			spx5_rmw(ANA_AC_PGID_MISC_CFG_PGID_CPU_COPY_ENA_SET(1),
-				 ANA_AC_PGID_MISC_CFG_PGID_CPU_COPY_ENA, spx5,
-				 ANA_AC_PGID_MISC_CFG(pgid_idx));
-		else
-			sparx5_pgid_update_mask(port, pgid_idx, true);
-
-	} else {
-		err = sparx5_pgid_alloc_mcast(spx5, &pgid_idx);
-		if (err) {
-			netdev_warn(dev, "multicast pgid table full\n");
+	entry = sparx5_mdb_get_entry(spx5, v->addr, vid);
+	if (!entry) {
+		err = sparx5_alloc_mdb_entry(spx5, v->addr, vid, &entry);
+		if (err)
 			return err;
-		}
-
-		if (is_host)
-			spx5_rmw(ANA_AC_PGID_MISC_CFG_PGID_CPU_COPY_ENA_SET(1),
-				 ANA_AC_PGID_MISC_CFG_PGID_CPU_COPY_ENA, spx5,
-				 ANA_AC_PGID_MISC_CFG(pgid_idx));
-		else
-			sparx5_pgid_update_mask(port, pgid_idx, true);
-
-		err = sparx5_mact_learn(spx5, pgid_idx, v->addr, vid);
-
-		if (err) {
-			netdev_warn(dev, "could not learn mac address %pM\n", v->addr);
-			sparx5_pgid_free(spx5, pgid_idx);
-			sparx5_pgid_update_mask(port, pgid_idx, false);
-			return err;
-		}
 	}
 
-	return 0;
-}
+	mutex_lock(&spx5->mdb_lock);
+	if (is_host && !entry->cpu_copy) {
+		sparx5_cpu_copy_ena(spx5, entry->pgid_idx, true);
+		entry->cpu_copy = true;
+	} else if (!is_host) {
+		sparx5_pgid_update_mask(port, entry->pgid_idx, true);
+		set_bit(port->portno, entry->port_mask);
+	}
+	mutex_unlock(&spx5->mdb_lock);
 
-static int sparx5_mdb_del_entry(struct net_device *dev,
-				struct sparx5 *spx5,
-				const unsigned char mac[ETH_ALEN],
-				const u16 vid,
-				u16 pgid_idx)
-{
-	int err;
+	sparx5_mact_learn(spx5, entry->pgid_idx, entry->addr, entry->vid);
 
-	err = sparx5_mact_forget(spx5, mac, vid);
-	if (err) {
-		netdev_warn(dev, "could not forget mac address %pM", mac);
-		return err;
-	}
-	err = sparx5_pgid_free(spx5, pgid_idx);
-	if (err) {
-		netdev_err(dev, "attempted to free already freed pgid\n");
-		return err;
-	}
 	return 0;
 }
 
@@ -479,42 +515,38 @@  static int sparx5_handle_port_mdb_del(struct net_device *dev,
 {
 	struct sparx5_port *port = netdev_priv(dev);
 	struct sparx5 *spx5 = port->sparx5;
-	u16 pgid_idx, vid;
-	u32 mact_entry, res, pgid_entry[3], misc_cfg;
-	bool host_ena;
+	struct sparx5_mdb_entry *entry;
+	bool is_host;
+	u16 vid;
 
 	if (!sparx5_netdevice_check(dev))
 		return -EOPNOTSUPP;
 
+	is_host = netif_is_bridge_master(v->obj.orig_dev);
+
 	if (!br_vlan_enabled(spx5->hw_bridge_dev))
 		vid = 1;
 	else
 		vid = v->vid;
 
-	res = sparx5_mact_find(spx5, v->addr, vid, &mact_entry);
-
-	if (res == 0) {
-		pgid_idx = LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_ADDR_GET(mact_entry);
-
-		/* MC_IDX starts after the port masks in the PGID table */
-		pgid_idx += SPX5_PORTS;
-
-		if (netif_is_bridge_master(v->obj.orig_dev))
-			spx5_rmw(ANA_AC_PGID_MISC_CFG_PGID_CPU_COPY_ENA_SET(0),
-				 ANA_AC_PGID_MISC_CFG_PGID_CPU_COPY_ENA, spx5,
-				 ANA_AC_PGID_MISC_CFG(pgid_idx));
-		else
-			sparx5_pgid_update_mask(port, pgid_idx, false);
-
-		misc_cfg = spx5_rd(spx5, ANA_AC_PGID_MISC_CFG(pgid_idx));
-		host_ena = ANA_AC_PGID_MISC_CFG_PGID_CPU_COPY_ENA_GET(misc_cfg);
+	entry = sparx5_mdb_get_entry(spx5, v->addr, vid);
+	if (!entry)
+		return 0;
 
-		sparx5_pgid_read_mask(spx5, pgid_idx, pgid_entry);
-		if (bitmap_empty((unsigned long *)pgid_entry, SPX5_PORTS) && !host_ena)
-			/* No ports or CPU are in MC group. Remove entry */
-			return sparx5_mdb_del_entry(dev, spx5, v->addr, vid, pgid_idx);
+	mutex_lock(&spx5->mdb_lock);
+	if (is_host && entry->cpu_copy) {
+		sparx5_cpu_copy_ena(spx5, entry->pgid_idx, false);
+		entry->cpu_copy = false;
+	} else if (!is_host) {
+		clear_bit(port->portno, entry->port_mask);
+		sparx5_pgid_update_mask(port, entry->pgid_idx, false);
 	}
+	mutex_unlock(&spx5->mdb_lock);
 
+	if (bitmap_empty(entry->port_mask, SPX5_PORTS) && !entry->cpu_copy) {
+		sparx5_mact_forget(spx5, entry->addr, entry->vid);
+		sparx5_free_mdb_entry(spx5, entry->addr, entry->vid);
+	}
 	return 0;
 }