[PATCH net-next 0/2] net: sparx5: Add multicast support

[PATCH net-next 0/2] net: sparx5: Add multicast support

[Casper Andersson]

Add multicast support to Sparx5.

--
I apologies, I accidentally forgot to add the netdev
mailing list the first time I sent this. Here it comes 
to netdev as well.

Casper Andersson (2):
  net: sparx5: Add arbiter for managing PGID table
  net: sparx5: Add mdb handlers

 .../net/ethernet/microchip/sparx5/Makefile    |   2 +-
 .../microchip/sparx5/sparx5_mactable.c        |  33 ++++--
 .../ethernet/microchip/sparx5/sparx5_main.c   |   3 +
 .../ethernet/microchip/sparx5/sparx5_main.h   |  23 ++++
 .../ethernet/microchip/sparx5/sparx5_pgid.c   |  60 ++++++++++
 .../microchip/sparx5/sparx5_switchdev.c       | 111 ++++++++++++++++++
 6 files changed, 221 insertions(+), 11 deletions(-)
 create mode 100644 drivers/net/ethernet/microchip/sparx5/sparx5_pgid.c

-- 
2.30.2

[PATCH net-next 1/2] net: sparx5: Add arbiter for managing PGID table

[Casper Andersson]

The PGID (Port Group ID) table holds port masks
for different purposes. The first 72 are reserved
for port destination masks, flood masks, and CPU
forwarding. The rest are shared between multicast,
link aggregation, and virtualization profiles. The
GLAG area is reserved to not be used by anything
else, since it is a subset of the MCAST area.

The arbiter keeps track of which entries are in
use. You can ask for a free ID or give back one
you are done using.

Signed-off-by: Casper Andersson <casper.casan@gmail.com>
---
 .../net/ethernet/microchip/sparx5/Makefile    |  2 +-
 .../ethernet/microchip/sparx5/sparx5_main.c   |  3 +
 .../ethernet/microchip/sparx5/sparx5_main.h   | 21 +++++++
 .../ethernet/microchip/sparx5/sparx5_pgid.c   | 60 +++++++++++++++++++
 4 files changed, 85 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/microchip/sparx5/sparx5_pgid.c

diff --git a/drivers/net/ethernet/microchip/sparx5/Makefile b/drivers/net/ethernet/microchip/sparx5/Makefile
index e9dd348a6ebb..4402c3ed1dc5 100644
--- a/drivers/net/ethernet/microchip/sparx5/Makefile
+++ b/drivers/net/ethernet/microchip/sparx5/Makefile
@@ -8,4 +8,4 @@ obj-$(CONFIG_SPARX5_SWITCH) += sparx5-switch.o
 sparx5-switch-objs  := sparx5_main.o sparx5_packet.o \
  sparx5_netdev.o sparx5_phylink.o sparx5_port.o sparx5_mactable.o sparx5_vlan.o \
  sparx5_switchdev.o sparx5_calendar.o sparx5_ethtool.o sparx5_fdma.o \
- sparx5_ptp.o
+ sparx5_ptp.o sparx5_pgid.o
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
index 5f7c7030ce03..01be7bd84181 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
@@ -626,6 +626,9 @@ static int sparx5_start(struct sparx5 *sparx5)
 	/* Init MAC table, ageing */
 	sparx5_mact_init(sparx5);
 
+	/* Init PGID table arbitrator */
+	sparx5_pgid_init(sparx5);
+
 	/* Setup VLANs */
 	sparx5_vlan_init(sparx5);
 
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
index df68a0891029..e97fa091c740 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
@@ -66,6 +66,12 @@ enum sparx5_vlan_port_type {
 #define PGID_BCAST	       (PGID_BASE + 6)
 #define PGID_CPU	       (PGID_BASE + 7)
 
+#define PGID_TABLE_SIZE	       3290
+
+#define PGID_MCAST_START 65
+#define PGID_GLAG_START 833
+#define PGID_GLAG_END 1088
+
 #define IFH_LEN                9 /* 36 bytes */
 #define NULL_VID               0
 #define SPX5_MACT_PULL_DELAY   (2 * HZ)
@@ -271,6 +277,8 @@ struct sparx5 {
 	struct mutex ptp_lock; /* lock for ptp interface state */
 	u16 ptp_skbs;
 	int ptp_irq;
+	/* PGID allocation map */
+	u8 pgid_map[PGID_TABLE_SIZE];
 };
 
 /* sparx5_switchdev.c */
@@ -359,6 +367,19 @@ void sparx5_ptp_txtstamp_release(struct sparx5_port *port,
 				 struct sk_buff *skb);
 irqreturn_t sparx5_ptp_irq_handler(int irq, void *args);
 
+/* sparx5_pgid.c */
+enum sparx5_pgid_type {
+	SPX5_PGID_FREE,
+	SPX5_PGID_RESERVED,
+	SPX5_PGID_MULTICAST,
+	SPX5_PGID_GLAG
+};
+
+void sparx5_pgid_init(struct sparx5 *spx5);
+int sparx5_pgid_alloc_glag(struct sparx5 *spx5, u16 *idx);
+int sparx5_pgid_alloc_mcast(struct sparx5 *spx5, u16 *idx);
+int sparx5_pgid_free(struct sparx5 *spx5, u16 idx);
+
 /* Clock period in picoseconds */
 static inline u32 sparx5_clk_period(enum sparx5_core_clockfreq cclock)
 {
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_pgid.c b/drivers/net/ethernet/microchip/sparx5/sparx5_pgid.c
new file mode 100644
index 000000000000..90366fcb9958
--- /dev/null
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_pgid.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0+
+#include "sparx5_main.h"
+
+void sparx5_pgid_init(struct sparx5 *spx5)
+{
+	int i;
+
+	for (i = 0; i < PGID_TABLE_SIZE; i++)
+		spx5->pgid_map[i] = SPX5_PGID_FREE;
+
+	/* Reserved for unicast, flood control, broadcast, and CPU.
+	 * These cannot be freed.
+	 */
+	for (i = 0; i <= PGID_CPU; i++)
+		spx5->pgid_map[i] = SPX5_PGID_RESERVED;
+}
+
+int sparx5_pgid_alloc_glag(struct sparx5 *spx5, u16 *idx)
+{
+	int i;
+
+	for (i = PGID_GLAG_START; i <= PGID_GLAG_END; i++)
+		if (spx5->pgid_map[i] == SPX5_PGID_FREE) {
+			spx5->pgid_map[i] = SPX5_PGID_GLAG;
+			*idx = i;
+			return 0;
+		}
+
+	return -EBUSY;
+}
+
+int sparx5_pgid_alloc_mcast(struct sparx5 *spx5, u16 *idx)
+{
+	int i;
+
+	for (i = PGID_MCAST_START; i < PGID_TABLE_SIZE; i++) {
+		if (i == PGID_GLAG_START)
+			i = PGID_GLAG_END + 1;
+
+		if (spx5->pgid_map[i] == SPX5_PGID_FREE) {
+			spx5->pgid_map[i] = SPX5_PGID_MULTICAST;
+			*idx = i;
+			return 0;
+		}
+	}
+
+	return -EBUSY;
+}
+
+int sparx5_pgid_free(struct sparx5 *spx5, u16 idx)
+{
+	if (idx <= PGID_CPU || idx >= PGID_TABLE_SIZE)
+		return -EINVAL;
+
+	if (spx5->pgid_map[idx] == SPX5_PGID_FREE)
+		return -EINVAL;
+
+	spx5->pgid_map[idx] = SPX5_PGID_FREE;
+	return 0;
+}
-- 
2.30.2

Re: [PATCH net-next 1/2] net: sparx5: Add arbiter for managing PGID table

[Steen Hegelund]

Hi Casper,

On Mon, 2022-03-21 at 11:14 +0100, Casper Andersson wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> The PGID (Port Group ID) table holds port masks
> for different purposes. The first 72 are reserved
> for port destination masks, flood masks, and CPU
> forwarding. The rest are shared between multicast,
> link aggregation, and virtualization profiles. The
> GLAG area is reserved to not be used by anything
> else, since it is a subset of the MCAST area.
> 
> The arbiter keeps track of which entries are in
> use. You can ask for a free ID or give back one
> you are done using.
> 
> Signed-off-by: Casper Andersson <casper.casan@gmail.com>
> ---
>  .../net/ethernet/microchip/sparx5/Makefile    |  2 +-
>  .../ethernet/microchip/sparx5/sparx5_main.c   |  3 +
>  .../ethernet/microchip/sparx5/sparx5_main.h   | 21 +++++++
>  .../ethernet/microchip/sparx5/sparx5_pgid.c   | 60 +++++++++++++++++++
>  4 files changed, 85 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/ethernet/microchip/sparx5/sparx5_pgid.c
> 
> diff --git a/drivers/net/ethernet/microchip/sparx5/Makefile
> b/drivers/net/ethernet/microchip/sparx5/Makefile
> index e9dd348a6ebb..4402c3ed1dc5 100644
> --- a/drivers/net/ethernet/microchip/sparx5/Makefile
> +++ b/drivers/net/ethernet/microchip/sparx5/Makefile
> @@ -8,4 +8,4 @@ obj-$(CONFIG_SPARX5_SWITCH) += sparx5-switch.o
>  sparx5-switch-objs  := sparx5_main.o sparx5_packet.o \
>   sparx5_netdev.o sparx5_phylink.o sparx5_port.o sparx5_mactable.o sparx5_vlan.o \
>   sparx5_switchdev.o sparx5_calendar.o sparx5_ethtool.o sparx5_fdma.o \
> - sparx5_ptp.o
> + sparx5_ptp.o sparx5_pgid.o
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> index 5f7c7030ce03..01be7bd84181 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
> @@ -626,6 +626,9 @@ static int sparx5_start(struct sparx5 *sparx5)
>         /* Init MAC table, ageing */
>         sparx5_mact_init(sparx5);
> 
> +       /* Init PGID table arbitrator */
> +       sparx5_pgid_init(sparx5);
> +
>         /* Setup VLANs */
>         sparx5_vlan_init(sparx5);
> 
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> index df68a0891029..e97fa091c740 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> @@ -66,6 +66,12 @@ enum sparx5_vlan_port_type {
>  #define PGID_BCAST            (PGID_BASE + 6)
>  #define PGID_CPU              (PGID_BASE + 7)
> 
> +#define PGID_TABLE_SIZE               3290
> +
> +#define PGID_MCAST_START 65

This overlaps with PGID_UC_FLOOD above.  You should drop this.
Please see this description:

https://microchip-ung.github.io/sparx-5_reginfo/reginfo_sparx-5.html?select=ana_ac,pgid

> 
> +#define PGID_GLAG_START 833
> +#define PGID_GLAG_END 1088

You do not appear to put the GLAG feature into use so you should remove these for now.

> +
>  #define IFH_LEN                9 /* 36 bytes */
>  #define NULL_VID               0
>  #define SPX5_MACT_PULL_DELAY   (2 * HZ)
> @@ -271,6 +277,8 @@ struct sparx5 {
>         struct mutex ptp_lock; /* lock for ptp interface state */
>         u16 ptp_skbs;
>         int ptp_irq;
> +       /* PGID allocation map */
> +       u8 pgid_map[PGID_TABLE_SIZE];
>  };
> 
>  /* sparx5_switchdev.c */
> @@ -359,6 +367,19 @@ void sparx5_ptp_txtstamp_release(struct sparx5_port *port,
>                                  struct sk_buff *skb);
>  irqreturn_t sparx5_ptp_irq_handler(int irq, void *args);
> 
> +/* sparx5_pgid.c */
> +enum sparx5_pgid_type {
> +       SPX5_PGID_FREE,
> +       SPX5_PGID_RESERVED,
> +       SPX5_PGID_MULTICAST,
> +       SPX5_PGID_GLAG
> +};
> +
> +void sparx5_pgid_init(struct sparx5 *spx5);
> +int sparx5_pgid_alloc_glag(struct sparx5 *spx5, u16 *idx);
> +int sparx5_pgid_alloc_mcast(struct sparx5 *spx5, u16 *idx);
> +int sparx5_pgid_free(struct sparx5 *spx5, u16 idx);
> +
>  /* Clock period in picoseconds */
>  static inline u32 sparx5_clk_period(enum sparx5_core_clockfreq cclock)
>  {
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_pgid.c
> b/drivers/net/ethernet/microchip/sparx5/sparx5_pgid.c
> new file mode 100644
> index 000000000000..90366fcb9958
> --- /dev/null
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_pgid.c
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +#include "sparx5_main.h"
> +
> +void sparx5_pgid_init(struct sparx5 *spx5)
> +{
> +       int i;
> +
> +       for (i = 0; i < PGID_TABLE_SIZE; i++)
> +               spx5->pgid_map[i] = SPX5_PGID_FREE;
> +
> +       /* Reserved for unicast, flood control, broadcast, and CPU.
> +        * These cannot be freed.
> +        */
> +       for (i = 0; i <= PGID_CPU; i++)
> +               spx5->pgid_map[i] = SPX5_PGID_RESERVED;
> +}
> +
> +int sparx5_pgid_alloc_glag(struct sparx5 *spx5, u16 *idx)
> +{
> +       int i;
> +
> +       for (i = PGID_GLAG_START; i <= PGID_GLAG_END; i++)
> +               if (spx5->pgid_map[i] == SPX5_PGID_FREE) {
> +                       spx5->pgid_map[i] = SPX5_PGID_GLAG;
> +                       *idx = i;
> +                       return 0;
> +               }
> +
> +       return -EBUSY;
> +}

You do not appear to put the GLAG feature into use so you should remove this function for now.

> +
> +int sparx5_pgid_alloc_mcast(struct sparx5 *spx5, u16 *idx)
> +{
> +       int i;
> +
> +       for (i = PGID_MCAST_START; i < PGID_TABLE_SIZE; i++) {
> +               if (i == PGID_GLAG_START)
> +                       i = PGID_GLAG_END + 1;
> +
> +               if (spx5->pgid_map[i] == SPX5_PGID_FREE) {
> +                       spx5->pgid_map[i] = SPX5_PGID_MULTICAST;
> +                       *idx = i;
> +                       return 0;
> +               }
> +       }
> +
> +       return -EBUSY;
> +}
> +
> +int sparx5_pgid_free(struct sparx5 *spx5, u16 idx)
> +{
> +       if (idx <= PGID_CPU || idx >= PGID_TABLE_SIZE)
> +               return -EINVAL;
> +
> +       if (spx5->pgid_map[idx] == SPX5_PGID_FREE)
> +               return -EINVAL;
> +
> +       spx5->pgid_map[idx] = SPX5_PGID_FREE;
> +       return 0;
> +}
> --
> 2.30.2
> 

[PATCH net-next 2/2] net: sparx5: Add mdb handlers

[Casper Andersson]

Adds mdb handlers. Uses the PGID arbiter to
find a free entry in the PGID table for the
multicast group port mask.

Signed-off-by: Casper Andersson <casper.casan@gmail.com>
---
 .../microchip/sparx5/sparx5_mactable.c        |  33 ++++--
 .../ethernet/microchip/sparx5/sparx5_main.h   |   2 +
 .../microchip/sparx5/sparx5_switchdev.c       | 111 ++++++++++++++++++
 3 files changed, 136 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
index 82b1b3c9a065..35abb3d0ce19 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
@@ -186,11 +186,11 @@ bool sparx5_mact_getnext(struct sparx5 *sparx5,
 	return ret == 0;
 }
 
-static int sparx5_mact_lookup(struct sparx5 *sparx5,
-			      const unsigned char mac[ETH_ALEN],
-			      u16 vid)
+bool sparx5_mact_find(struct sparx5 *sparx5,
+		      const unsigned char mac[ETH_ALEN], u16 vid, u32 *pcfg2)
 {
 	int ret;
+	u32 cfg2;
 
 	mutex_lock(&sparx5->lock);
 
@@ -202,16 +202,29 @@ static int sparx5_mact_lookup(struct sparx5 *sparx5,
 		sparx5, LRN_COMMON_ACCESS_CTRL);
 
 	ret = sparx5_mact_wait_for_completion(sparx5);
-	if (ret)
-		goto out;
-
-	ret = LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_VLD_GET
-		(spx5_rd(sparx5, LRN_MAC_ACCESS_CFG_2));
+	if (ret == 0) {
+		cfg2 = spx5_rd(sparx5, LRN_MAC_ACCESS_CFG_2);
+		if (LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_VLD_GET(cfg2))
+			*pcfg2 = cfg2;
+		else
+			ret = -ENOENT;
+	}
 
-out:
 	mutex_unlock(&sparx5->lock);
 
-	return ret;
+	return ret == 0;
+}
+
+static int sparx5_mact_lookup(struct sparx5 *sparx5,
+			      const unsigned char mac[ETH_ALEN],
+			      u16 vid)
+{
+	u32 pcfg2;
+
+	if (sparx5_mact_find(sparx5, mac, vid, &pcfg2))
+		return 1;
+
+	return 0;
 }
 
 int sparx5_mact_forget(struct sparx5 *sparx5,
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
index e97fa091c740..7a04b8f2a546 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
@@ -310,6 +310,8 @@ int sparx5_mact_learn(struct sparx5 *sparx5, int port,
 		      const unsigned char mac[ETH_ALEN], u16 vid);
 bool sparx5_mact_getnext(struct sparx5 *sparx5,
 			 unsigned char mac[ETH_ALEN], u16 *vid, u32 *pcfg2);
+bool sparx5_mact_find(struct sparx5 *sparx5,
+		      const unsigned char mac[ETH_ALEN], u16 vid, u32 *pcfg2);
 int sparx5_mact_forget(struct sparx5 *sparx5,
 		       const unsigned char mac[ETH_ALEN], u16 vid);
 int sparx5_add_mact_entry(struct sparx5 *sparx5,
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c b/drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c
index 2d5de1c06fab..9e1ea35d0c40 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c
@@ -366,6 +366,109 @@ static int sparx5_handle_port_vlan_add(struct net_device *dev,
 				  v->flags & BRIDGE_VLAN_INFO_UNTAGGED);
 }
 
+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;
+	int res, err;
+
+	/* When VLAN unaware the vlan value is not parsed and we receive vid 0.
+	 * Fall back to bridge vid 1.
+	 */
+	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) {
+		pgid_idx = LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_ADDR_GET(mact_entry);
+
+		/* MC_IDX has an offset of 65 in the PGID table. */
+		pgid_idx += PGID_MCAST_START;
+		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");
+			return err;
+		}
+		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_update_mask(port, pgid_idx, false);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+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;
+
+	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;
+}
+
+static int sparx5_handle_port_mdb_del(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, res, pgid_entry[3];
+	int err;
+
+	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) {
+		pgid_idx = LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_ADDR_GET(mact_entry);
+
+		/* MC_IDX has an offset of 65 in the PGID table. */
+		pgid_idx += PGID_MCAST_START;
+		sparx5_pgid_update_mask(port, pgid_idx, false);
+
+		pgid_entry[0] = spx5_rd(spx5, ANA_AC_PGID_CFG(pgid_idx));
+		pgid_entry[1] = spx5_rd(spx5, ANA_AC_PGID_CFG1(pgid_idx));
+		pgid_entry[2] = spx5_rd(spx5, ANA_AC_PGID_CFG2(pgid_idx));
+		if (pgid_entry[0] == 0 && pgid_entry[1] == 0 && pgid_entry[2] == 0) {
+			/* No ports are in MC group. Remove entry */
+			err = sparx5_mdb_del_entry(dev, spx5, v->addr, vid, pgid_idx);
+			if (err)
+				return err;
+		}
+	}
+
+	return 0;
+}
+
 static int sparx5_handle_port_obj_add(struct net_device *dev,
 				      struct notifier_block *nb,
 				      struct switchdev_notifier_port_obj_info *info)
@@ -378,6 +481,10 @@ static int sparx5_handle_port_obj_add(struct net_device *dev,
 		err = sparx5_handle_port_vlan_add(dev, nb,
 						  SWITCHDEV_OBJ_PORT_VLAN(obj));
 		break;
+	case SWITCHDEV_OBJ_ID_PORT_MDB:
+		err = sparx5_handle_port_mdb_add(dev, nb,
+						 SWITCHDEV_OBJ_PORT_MDB(obj));
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
@@ -426,6 +533,10 @@ static int sparx5_handle_port_obj_del(struct net_device *dev,
 		err = sparx5_handle_port_vlan_del(dev, nb,
 						  SWITCHDEV_OBJ_PORT_VLAN(obj)->vid);
 		break;
+	case SWITCHDEV_OBJ_ID_PORT_MDB:
+		err = sparx5_handle_port_mdb_del(dev, nb,
+						 SWITCHDEV_OBJ_PORT_MDB(obj));
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
-- 
2.30.2

Re: [PATCH net-next 2/2] net: sparx5: Add mdb handlers

[Steen Hegelund]

On Mon, 2022-03-21 at 11:14 +0100, Casper Andersson wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Adds mdb handlers. Uses the PGID arbiter to
> find a free entry in the PGID table for the
> multicast group port mask.
> 
> Signed-off-by: Casper Andersson <casper.casan@gmail.com>
> ---
>  .../microchip/sparx5/sparx5_mactable.c        |  33 ++++--
>  .../ethernet/microchip/sparx5/sparx5_main.h   |   2 +
>  .../microchip/sparx5/sparx5_switchdev.c       | 111 ++++++++++++++++++
>  3 files changed, 136 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
> b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
> index 82b1b3c9a065..35abb3d0ce19 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
> @@ -186,11 +186,11 @@ bool sparx5_mact_getnext(struct sparx5 *sparx5,
>         return ret == 0;
>  }
> 
> -static int sparx5_mact_lookup(struct sparx5 *sparx5,
> -                             const unsigned char mac[ETH_ALEN],
> -                             u16 vid)
> +bool sparx5_mact_find(struct sparx5 *sparx5,
> +                     const unsigned char mac[ETH_ALEN], u16 vid, u32 *pcfg2)
>  {
>         int ret;
> +       u32 cfg2;
> 
>         mutex_lock(&sparx5->lock);
> 
> @@ -202,16 +202,29 @@ static int sparx5_mact_lookup(struct sparx5 *sparx5,
>                 sparx5, LRN_COMMON_ACCESS_CTRL);
> 
>         ret = sparx5_mact_wait_for_completion(sparx5);
> -       if (ret)
> -               goto out;
> -
> -       ret = LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_VLD_GET
> -               (spx5_rd(sparx5, LRN_MAC_ACCESS_CFG_2));
> +       if (ret == 0) {
> +               cfg2 = spx5_rd(sparx5, LRN_MAC_ACCESS_CFG_2);
> +               if (LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_VLD_GET(cfg2))
> +                       *pcfg2 = cfg2;
> +               else
> +                       ret = -ENOENT;
> +       }
> 
> -out:
>         mutex_unlock(&sparx5->lock);
> 
> -       return ret;
> +       return ret == 0;
> +}
> +
> +static int sparx5_mact_lookup(struct sparx5 *sparx5,
> +                             const unsigned char mac[ETH_ALEN],
> +                             u16 vid)
> +{
> +       u32 pcfg2;
> +
> +       if (sparx5_mact_find(sparx5, mac, vid, &pcfg2))
> +               return 1;
> +
> +       return 0;
>  }

I suggest to drop this and only use your new function (or at least let it return a real error code
like -ENOENT in case of an error).

> 
>  int sparx5_mact_forget(struct sparx5 *sparx5,
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> index e97fa091c740..7a04b8f2a546 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> @@ -310,6 +310,8 @@ int sparx5_mact_learn(struct sparx5 *sparx5, int port,
>                       const unsigned char mac[ETH_ALEN], u16 vid);
>  bool sparx5_mact_getnext(struct sparx5 *sparx5,
>                          unsigned char mac[ETH_ALEN], u16 *vid, u32 *pcfg2);
> +bool sparx5_mact_find(struct sparx5 *sparx5,
> +                     const unsigned char mac[ETH_ALEN], u16 vid, u32 *pcfg2);
>  int sparx5_mact_forget(struct sparx5 *sparx5,
>                        const unsigned char mac[ETH_ALEN], u16 vid);
>  int sparx5_add_mact_entry(struct sparx5 *sparx5,
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c
> b/drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c
> index 2d5de1c06fab..9e1ea35d0c40 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_switchdev.c
> @@ -366,6 +366,109 @@ static int sparx5_handle_port_vlan_add(struct net_device *dev,
>                                   v->flags & BRIDGE_VLAN_INFO_UNTAGGED);
>  }
> 
> +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;
> +       int res, err;
> +
> +       /* When VLAN unaware the vlan value is not parsed and we receive vid 0.
> +        * Fall back to bridge vid 1.
> +        */
> +       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) {
> +               pgid_idx = LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_ADDR_GET(mact_entry);
> +
> +               /* MC_IDX has an offset of 65 in the PGID table. */
> +               pgid_idx += PGID_MCAST_START;

This will overlap some of the first ports with the flood masks according to:

https://microchip-ung.github.io/sparx-5_reginfo/reginfo_sparx-5.html?select=ana_ac,pgid

You should use the custom area (PGID_BASE + 8 and onwards) for this new feature.

> +               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");
> +                       return err;
> +               }
> +               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_update_mask(port, pgid_idx, false);
> +                       return err;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +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;
> +
> +       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;
> +}
> +
> +static int sparx5_handle_port_mdb_del(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, res, pgid_entry[3];
> +       int err;
> +
> +       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) {
> +               pgid_idx = LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_ADDR_GET(mact_entry);
> +
> +               /* MC_IDX has an offset of 65 in the PGID table. */
> +               pgid_idx += PGID_MCAST_START;
> +               sparx5_pgid_update_mask(port, pgid_idx, false);
> +
> +               pgid_entry[0] = spx5_rd(spx5, ANA_AC_PGID_CFG(pgid_idx));
> +               pgid_entry[1] = spx5_rd(spx5, ANA_AC_PGID_CFG1(pgid_idx));
> +               pgid_entry[2] = spx5_rd(spx5, ANA_AC_PGID_CFG2(pgid_idx));
> +               if (pgid_entry[0] == 0 && pgid_entry[1] == 0 && pgid_entry[2] == 0) {

Looks like you could use a function that gets the pgid port mask (the inverse of the
sparx5_pgid_update_mask() function)


> +                       /* No ports are in MC group. Remove entry */
> +                       err = sparx5_mdb_del_entry(dev, spx5, v->addr, vid, pgid_idx);
> +                       if (err)
> +                               return err;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  static int sparx5_handle_port_obj_add(struct net_device *dev,
>                                       struct notifier_block *nb,
>                                       struct switchdev_notifier_port_obj_info *info)
> @@ -378,6 +481,10 @@ static int sparx5_handle_port_obj_add(struct net_device *dev,
>                 err = sparx5_handle_port_vlan_add(dev, nb,
>                                                   SWITCHDEV_OBJ_PORT_VLAN(obj));
>                 break;
> +       case SWITCHDEV_OBJ_ID_PORT_MDB:
> +               err = sparx5_handle_port_mdb_add(dev, nb,
> +                                                SWITCHDEV_OBJ_PORT_MDB(obj));
> +               break;
>         default:
>                 err = -EOPNOTSUPP;
>                 break;
> @@ -426,6 +533,10 @@ static int sparx5_handle_port_obj_del(struct net_device *dev,
>                 err = sparx5_handle_port_vlan_del(dev, nb,
>                                                   SWITCHDEV_OBJ_PORT_VLAN(obj)->vid);
>                 break;
> +       case SWITCHDEV_OBJ_ID_PORT_MDB:
> +               err = sparx5_handle_port_mdb_del(dev, nb,
> +                                                SWITCHDEV_OBJ_PORT_MDB(obj));
> +               break;
>         default:
>                 err = -EOPNOTSUPP;
>                 break;
> --
> 2.30.2
> 

Best Regards
Steen

Re: [PATCH net-next 2/2] net: sparx5: Add mdb handlers

[Casper Andersson]

So this was already merged, but I have some comments on the feedback for
the follow up patch.

> > +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;
> > +       int res, err;
> > +
> > +       /* When VLAN unaware the vlan value is not parsed and we receive vid 0.
> > +        * Fall back to bridge vid 1.
> > +        */
> > +       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) {
> > +               pgid_idx = LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_ADDR_GET(mact_entry);
> > +
> > +               /* MC_IDX has an offset of 65 in the PGID table. */
> > +               pgid_idx += PGID_MCAST_START;
> 
> This will overlap some of the first ports with the flood masks according to:
> 
> https://microchip-ung.github.io/sparx-5_reginfo/reginfo_sparx-5.html?select=ana_ac,pgid
> 
> You should use the custom area (PGID_BASE + 8 and onwards) for this new feature.

I'm aware of the overlap, hence why the PGID table has those fields
marked as reserved. But your datasheet says that the multicast index 
has an offset of 65 (ie. MC_IDX = 0 is at PGID = 65). This is already 
taken into account in the mact_learn function. I could set the
allocation to start at PGID_BASE + 8, but the offset still needs to 
be 65, right?

BR
Casper

Re: [PATCH net-next 2/2] net: sparx5: Add mdb handlers

[Steen Hegelund]

Hi Casper

On Tue, 2022-03-22 at 10:59 +0100, Casper Andersson wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> So this was already merged, but I have some comments on the feedback for
> the follow up patch.
> 
> > > +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;
> > > +       int res, err;
> > > +
> > > +       /* When VLAN unaware the vlan value is not parsed and we receive vid 0.
> > > +        * Fall back to bridge vid 1.
> > > +        */
> > > +       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) {
> > > +               pgid_idx = LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_ADDR_GET(mact_entry);
> > > +
> > > +               /* MC_IDX has an offset of 65 in the PGID table. */
> > > +               pgid_idx += PGID_MCAST_START;
> > 
> > This will overlap some of the first ports with the flood masks according to:
> > 
> > https://microchip-ung.github.io/sparx-5_reginfo/reginfo_sparx-5.html?select=ana_ac,pgid
> > 
> > You should use the custom area (PGID_BASE + 8 and onwards) for this new feature.
> 
> I'm aware of the overlap, hence why the PGID table has those fields
> marked as reserved. But your datasheet says that the multicast index
> has an offset of 65 (ie. MC_IDX = 0 is at PGID = 65). This is already
> taken into account in the mact_learn function. I could set the
> allocation to start at PGID_BASE + 8, but the offset still needs to
> be 65, right?

As I understand the PGID table functionality, you will need to start your custom table at PGID_BASE
+ 8 as the bitmasks at offset 65 to 70 are used as flood masks, so their purpose are already
defined.

BR
Steen

> 
> BR
> Casper

Re: [PATCH net-next 2/2] net: sparx5: Add mdb handlers

[Casper Andersson]

On 2022-03-22 15:51, Steen Hegelund wrote:
> Hi Casper
> 
> On Tue, 2022-03-22 at 10:59 +0100, Casper Andersson wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > So this was already merged, but I have some comments on the feedback for
> > the follow up patch.
> > 
> > > > +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;
> > > > +       int res, err;
> > > > +
> > > > +       /* When VLAN unaware the vlan value is not parsed and we receive vid 0.
> > > > +        * Fall back to bridge vid 1.
> > > > +        */
> > > > +       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) {
> > > > +               pgid_idx = LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_ADDR_GET(mact_entry);
> > > > +
> > > > +               /* MC_IDX has an offset of 65 in the PGID table. */
> > > > +               pgid_idx += PGID_MCAST_START;
> > > 
> > > This will overlap some of the first ports with the flood masks according to:
> > > 
> > > https://microchip-ung.github.io/sparx-5_reginfo/reginfo_sparx-5.html?select=ana_ac,pgid
> > > 
> > > You should use the custom area (PGID_BASE + 8 and onwards) for this new feature.
> > 
> > I'm aware of the overlap, hence why the PGID table has those fields
> > marked as reserved. But your datasheet says that the multicast index
> > has an offset of 65 (ie. MC_IDX = 0 is at PGID = 65). This is already
> > taken into account in the mact_learn function. I could set the
> > allocation to start at PGID_BASE + 8, but the offset still needs to
> > be 65, right?
> 
> As I understand the PGID table functionality, you will need to start your custom table at PGID_BASE
> + 8 as the bitmasks at offset 65 to 70 are used as flood masks, so their purpose are already
> defined.

It is fine to start the allocation of multicast entries at PGID_BASE + 8,
but the multicast index (MC_IDX) in the mactable (that points at the
PGID table, to get the port mask) assumes that MC_IDX = 0 is at PGID = 65.
Not sure if it's appropriate to reference the datasheet here on Netdev,
but on figure 4-48 (PGID Layout) this is shown.

I tried setting the offset to base + 8 but it does not seem to find the 
right mask. Because if I say I put the mask on MC_IDX 0, and then place
it on base + 8 (73), then it will not find that mask because the hardware
will look for the mask at 65.

Even though the offset is 65, the PGID allocation will make sure
that it never allocates anything below 73. Meaning that the lowest
possible MC_IDX it can allocate will be 8.

I'm also a bit confused as to why the multicast offset is 65, but there
are a lot of overlapping areas in the PGID table and, e.g., GLAG has 
another offset where its index 0 is at PGID = 833 (as can be seen in
figure 4-48).

BR
Casper

> BR
> Steen
> 
> > 
> > BR
> > Casper
> 

Re: [PATCH net-next 0/2] net: sparx5: Add multicast support

[patchwork-bot+netdevbpf]

Hello:

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

On Mon, 21 Mar 2022 11:14:44 +0100 you wrote:
> Add multicast support to Sparx5.
> 
> --
> I apologies, I accidentally forgot to add the netdev
> mailing list the first time I sent this. Here it comes
> to netdev as well.
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] net: sparx5: Add arbiter for managing PGID table
    https://git.kernel.org/netdev/net-next/c/af9b45d08eb4
  - [net-next,2/2] net: sparx5: Add mdb handlers
    https://git.kernel.org/netdev/net-next/c/3bacfccdcb2d

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/2] net: sparx5: Add multicast support

[Steen Hegelund]

Hi Dave,

That must be a mistake.

I have just added some comments to the series, and I have not had more than a few hours to look at
it, so I do not think that you have given this enough time to mature.

BR
Steen

On Mon, 2022-03-21 at 13:30 +0000, patchwork-bot+netdevbpf@kernel.org wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hello:
> 
> This series was applied to netdev/net-next.git (master)
> by David S. Miller <davem@davemloft.net>:
> 
> On Mon, 21 Mar 2022 11:14:44 +0100 you wrote:
> > Add multicast support to Sparx5.
> > 
> > --
> > I apologies, I accidentally forgot to add the netdev
> > mailing list the first time I sent this. Here it comes
> > to netdev as well.
> > 
> > [...]
> 
> Here is the summary with links:
>   - [net-next,1/2] net: sparx5: Add arbiter for managing PGID table
>     https://git.kernel.org/netdev/net-next/c/af9b45d08eb4
>   - [net-next,2/2] net: sparx5: Add mdb handlers
>     https://git.kernel.org/netdev/net-next/c/3bacfccdcb2d
> 
> 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/2] net: sparx5: Add multicast support

[Jakub Kicinski]

On Mon, 21 Mar 2022 14:33:34 +0100 Steen Hegelund wrote:
> I have just added some comments to the series, and I have not had
> more than a few hours to look at it, so I do not think that you have
> given this enough time to mature.

Sorry about that. Is it possible to fix the issues in a follow up
or should we revert the patches?

Re: [PATCH net-next 0/2] net: sparx5: Add multicast support

[Steen Hegelund]

Hi Jacub,

I guess that Casper can fix the issues in a follow up.
What do you say Casper?

BR
Steen

On Mon, 2022-03-21 at 12:47 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, 21 Mar 2022 14:33:34 +0100 Steen Hegelund wrote:
> > I have just added some comments to the series, and I have not had
> > more than a few hours to look at it, so I do not think that you have
> > given this enough time to mature.
> 
> Sorry about that. Is it possible to fix the issues in a follow up
> or should we revert the patches?

Re: [PATCH net-next 0/2] net: sparx5: Add multicast support

[Casper Andersson]

I will do a follow up. Thanks for the feedback, Steen.

BR
Casper

On 2022-03-22 09:06, Steen Hegelund wrote:
> Hi Jacub,
> 
> I guess that Casper can fix the issues in a follow up.
> What do you say Casper?
> 
> BR
> Steen
> 
> On Mon, 2022-03-21 at 12:47 -0700, Jakub Kicinski wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Mon, 21 Mar 2022 14:33:34 +0100 Steen Hegelund wrote:
> > > I have just added some comments to the series, and I have not had
> > > more than a few hours to look at it, so I do not think that you have
> > > given this enough time to mature.
> > 
> > Sorry about that. Is it possible to fix the issues in a follow up
> > or should we revert the patches?
>