[PATCH] net: mscc: ocelot: Fix multicast to the CPU port

[PATCH] net: mscc: ocelot: Fix multicast to the CPU port

[Alban Bedel]

Multicast entries in the MAC table use the high bits of the MAC
address to encode the ports that should get the packets. But this port
mask does not work for the CPU port, to receive these packets on the
CPU port the MAC_CPU_COPY flag must be set.

Because of this IPv6 was effectively not working because neighbor
solicitations were never received. This was not apparent before commit
9403c158 (net: mscc: ocelot: support IPv4, IPv6 and plain Ethernet mdb
entries) as the IPv6 entries were broken so all incoming IPv6
multicast was then treated as unknown and flooded on all ports.

To fix this problem add a new `flags` parameter to ocelot_mact_learn()
and set MAC_CPU_COPY when the CPU port is in the port set. We still
leave the CPU port in the bitfield as it doesn't seems to hurt.

Signed-off-by: Alban Bedel <alban.bedel@aerq.com>
Fixes: 9403c158 (net: mscc: ocelot: support IPv4, IPv6 and plain Ethernet mdb entries)
---
 drivers/net/ethernet/mscc/ocelot.c | 17 ++++++++++++-----
 drivers/net/ethernet/mscc/ocelot.h |  3 ++-
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 0b9992bd6626..c33162dbf075 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -58,12 +58,13 @@ static void ocelot_mact_select(struct ocelot *ocelot,
 
 int ocelot_mact_learn(struct ocelot *ocelot, int port,
 		      const unsigned char mac[ETH_ALEN],
-		      unsigned int vid, enum macaccess_entry_type type)
+		      unsigned int vid, enum macaccess_entry_type type,
+		      u32 flags)
 {
 	ocelot_mact_select(ocelot, mac, vid);
 
 	/* Issue a write command */
-	ocelot_write(ocelot, ANA_TABLES_MACACCESS_VALID |
+	ocelot_write(ocelot, ANA_TABLES_MACACCESS_VALID | flags |
 			     ANA_TABLES_MACACCESS_DEST_IDX(port) |
 			     ANA_TABLES_MACACCESS_ENTRYTYPE(type) |
 			     ANA_TABLES_MACACCESS_MAC_TABLE_CMD(MACACCESS_CMD_LEARN),
@@ -574,7 +575,7 @@ int ocelot_fdb_add(struct ocelot *ocelot, int port,
 	if (port == ocelot->npi)
 		pgid = PGID_CPU;
 
-	return ocelot_mact_learn(ocelot, pgid, addr, vid, ENTRYTYPE_LOCKED);
+	return ocelot_mact_learn(ocelot, pgid, addr, vid, ENTRYTYPE_LOCKED, 0);
 }
 EXPORT_SYMBOL(ocelot_fdb_add);
 
@@ -1064,6 +1065,7 @@ int ocelot_port_mdb_add(struct ocelot *ocelot, int port,
 	struct ocelot_multicast *mc;
 	struct ocelot_pgid *pgid;
 	u16 vid = mdb->vid;
+	u32 flags = 0;
 
 	if (port == ocelot->npi)
 		port = ocelot->num_phys_ports;
@@ -1107,9 +1109,11 @@ int ocelot_port_mdb_add(struct ocelot *ocelot, int port,
 	    mc->entry_type != ENTRYTYPE_MACv6)
 		ocelot_write_rix(ocelot, pgid->ports, ANA_PGID_PGID,
 				 pgid->index);
+	if (mc->ports & BIT(ocelot->num_phys_ports))
+		flags |= ANA_TABLES_MACACCESS_MAC_CPU_COPY;
 
 	return ocelot_mact_learn(ocelot, pgid->index, addr, vid,
-				 mc->entry_type);
+				 mc->entry_type, flags);
 }
 EXPORT_SYMBOL(ocelot_port_mdb_add);
 
@@ -1120,6 +1124,7 @@ int ocelot_port_mdb_del(struct ocelot *ocelot, int port,
 	struct ocelot_multicast *mc;
 	struct ocelot_pgid *pgid;
 	u16 vid = mdb->vid;
+	u32 flags = 0;
 
 	if (port == ocelot->npi)
 		port = ocelot->num_phys_ports;
@@ -1151,9 +1156,11 @@ int ocelot_port_mdb_del(struct ocelot *ocelot, int port,
 	    mc->entry_type != ENTRYTYPE_MACv6)
 		ocelot_write_rix(ocelot, pgid->ports, ANA_PGID_PGID,
 				 pgid->index);
+	if (mc->ports & BIT(ocelot->num_phys_ports))
+		flags |= ANA_TABLES_MACACCESS_MAC_CPU_COPY;
 
 	return ocelot_mact_learn(ocelot, pgid->index, addr, vid,
-				 mc->entry_type);
+				 mc->entry_type, flags);
 }
 EXPORT_SYMBOL(ocelot_port_mdb_del);
 
diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
index 291d39d49c4e..63045f1ef0cd 100644
--- a/drivers/net/ethernet/mscc/ocelot.h
+++ b/drivers/net/ethernet/mscc/ocelot.h
@@ -106,7 +106,8 @@ int ocelot_port_fdb_do_dump(const unsigned char *addr, u16 vid,
 			    bool is_static, void *data);
 int ocelot_mact_learn(struct ocelot *ocelot, int port,
 		      const unsigned char mac[ETH_ALEN],
-		      unsigned int vid, enum macaccess_entry_type type);
+		      unsigned int vid, enum macaccess_entry_type type,
+		      u32 flags);
 int ocelot_mact_forget(struct ocelot *ocelot,
 		       const unsigned char mac[ETH_ALEN], unsigned int vid);
 int ocelot_port_lag_join(struct ocelot *ocelot, int port,
-- 
2.25.1

Re: [PATCH] net: mscc: ocelot: Fix multicast to the CPU port

[Vladimir Oltean]

Hi Alban,

On Mon, Jan 18, 2021 at 05:03:17PM +0100, Alban Bedel wrote:
> Multicast entries in the MAC table use the high bits of the MAC
> address to encode the ports that should get the packets. But this port
> mask does not work for the CPU port, to receive these packets on the
> CPU port the MAC_CPU_COPY flag must be set.
> 
> Because of this IPv6 was effectively not working because neighbor
> solicitations were never received. This was not apparent before commit
> 9403c158 (net: mscc: ocelot: support IPv4, IPv6 and plain Ethernet mdb
> entries) as the IPv6 entries were broken so all incoming IPv6
> multicast was then treated as unknown and flooded on all ports.
> 
> To fix this problem add a new `flags` parameter to ocelot_mact_learn()
> and set MAC_CPU_COPY when the CPU port is in the port set. We still
> leave the CPU port in the bitfield as it doesn't seems to hurt.
> 
> Signed-off-by: Alban Bedel <alban.bedel@aerq.com>
> Fixes: 9403c158 (net: mscc: ocelot: support IPv4, IPv6 and plain Ethernet mdb entries)
> ---

Good catch, it seems that I really did not test that patch with
multicast traffic received on the CPU (and not only that patch, but ever
since, in fact), shame on me.

What I don't like your patch is how it spills over the entire ocelot
driver, yet still fails to compile. You missed a bunch of
ocelot_mact_learn calls from ocelot_net.c (8 of them, in fact).
I don't know which kernel tree you applied this patch to, but clearly
not "net"/master:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git

I would prefer to see a more self-contained bug fix, such as potentially
this one:

-----------------------------[cut here]-----------------------------
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index a560d6be2a44..4d7443b123bd 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -56,18 +56,46 @@ static void ocelot_mact_select(struct ocelot *ocelot,
 
 }
 
+static unsigned long
+ocelot_decode_ports_from_mdb(const unsigned char *addr,
+			     enum macaccess_entry_type entry_type)
+{
+	unsigned long ports = 0;
+
+	if (entry_type == ENTRYTYPE_MACv4) {
+		ports = addr[2];
+		ports |= addr[1] << 8;
+	} else if (entry_type == ENTRYTYPE_MACv6) {
+		ports = addr[1];
+		ports |= addr[0] << 8;
+	}
+
+	return ports;
+}
+
 int ocelot_mact_learn(struct ocelot *ocelot, int port,
 		      const unsigned char mac[ETH_ALEN],
 		      unsigned int vid, enum macaccess_entry_type type)
 {
+	u32 flags = ANA_TABLES_MACACCESS_VALID |
+		    ANA_TABLES_MACACCESS_DEST_IDX(port) |
+		    ANA_TABLES_MACACCESS_ENTRYTYPE(type) |
+		    ANA_TABLES_MACACCESS_MAC_TABLE_CMD(MACACCESS_CMD_LEARN);
+
 	ocelot_mact_select(ocelot, mac, vid);
 
+	/* Little API trickery to make this function "just work" when the CPU
+	 * port module is included in the port mask for multicast IP entries.
+	 */
+	if (type == ENTRYTYPE_MACv4 || type == ENTRYTYPE_MACv6) {
+		unsigned long ports = ocelot_decode_ports_from_mdb(mac, type);
+
+		if (ports & BIT(ocelot->num_phys_ports))
+			flags |= ANA_TABLES_MACACCESS_MAC_CPU_COPY;
+	}
+
 	/* Issue a write command */
-	ocelot_write(ocelot, ANA_TABLES_MACACCESS_VALID |
-			     ANA_TABLES_MACACCESS_DEST_IDX(port) |
-			     ANA_TABLES_MACACCESS_ENTRYTYPE(type) |
-			     ANA_TABLES_MACACCESS_MAC_TABLE_CMD(MACACCESS_CMD_LEARN),
-			     ANA_TABLES_MACACCESS);
+	ocelot_write(ocelot, flags, ANA_TABLES_MACACCESS);
 
 	return ocelot_mact_wait_for_completion(ocelot);
 }
-----------------------------[cut here]-----------------------------

It has the advantage of actually compiling, plus it should be easier to
backport because the changes are all in one place.


Please make sure to read:
Documentation/process/submitting-patches.rst
(this will tell you what is wrong with your Fixes: tag)
Documentation/networking/netdev-FAQ.rst
(this will tell you what is wrong with this patch's --subject-prefix,
and why the patch does not build on the trees it is supposed to be
applied to):
https://patchwork.kernel.org/project/netdevbpf/patch/20210118160317.554018-1-alban.bedel@aerq.com/

Re: [PATCH] net: mscc: ocelot: Fix multicast to the CPU port

[Bedel, Alban]

[-- Attachment #1: Type: text/plain, Size: 4731 bytes --]

On Mon, 2021-01-18 at 18:55 +0000, Vladimir Oltean wrote:
> Hi Alban,
> 
> On Mon, Jan 18, 2021 at 05:03:17PM +0100, Alban Bedel wrote:
> > Multicast entries in the MAC table use the high bits of the MAC
> > address to encode the ports that should get the packets. But this
> > port
> > mask does not work for the CPU port, to receive these packets on
> > the
> > CPU port the MAC_CPU_COPY flag must be set.
> > 
> > Because of this IPv6 was effectively not working because neighbor
> > solicitations were never received. This was not apparent before
> > commit
> > 9403c158 (net: mscc: ocelot: support IPv4, IPv6 and plain Ethernet
> > mdb
> > entries) as the IPv6 entries were broken so all incoming IPv6
> > multicast was then treated as unknown and flooded on all ports.
> > 
> > To fix this problem add a new `flags` parameter to
> > ocelot_mact_learn()
> > and set MAC_CPU_COPY when the CPU port is in the port set. We still
> > leave the CPU port in the bitfield as it doesn't seems to hurt.
> > 
> > Signed-off-by: Alban Bedel <alban.bedel@aerq.com>
> > Fixes: 9403c158 (net: mscc: ocelot: support IPv4, IPv6 and plain
> > Ethernet mdb entries)
> > ---
> 
> Good catch, it seems that I really did not test that patch with
> multicast traffic received on the CPU (and not only that patch, but
> ever
> since, in fact), shame on me.
> 
> What I don't like your patch is how it spills over the entire ocelot
> driver, yet still fails to compile. You missed a bunch of
> ocelot_mact_learn calls from ocelot_net.c (8 of them, in fact).
> I don't know which kernel tree you applied this patch to, but clearly
> not "net"/master:
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git

My board use felix and I build without CONFIG_MSCC_OCELOT_SWITCH so I
missed these, my bad.

> I would prefer to see a more self-contained bug fix, such as
> potentially
> this one:
> 
> -----------------------------[cut here]-----------------------------
> diff --git a/drivers/net/ethernet/mscc/ocelot.c
> b/drivers/net/ethernet/mscc/ocelot.c
> index a560d6be2a44..4d7443b123bd 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -56,18 +56,46 @@ static void ocelot_mact_select(struct ocelot
> *ocelot,
>  
>  }
>  
> +static unsigned long
> +ocelot_decode_ports_from_mdb(const unsigned char *addr,
> +			     enum macaccess_entry_type entry_type)
> +{
> +	unsigned long ports = 0;
> +
> +	if (entry_type == ENTRYTYPE_MACv4) {
> +		ports = addr[2];
> +		ports |= addr[1] << 8;
> +	} else if (entry_type == ENTRYTYPE_MACv6) {
> +		ports = addr[1];
> +		ports |= addr[0] << 8;
> +	}
> +
> +	return ports;
> +}
> +
>  int ocelot_mact_learn(struct ocelot *ocelot, int port,
>  		      const unsigned char mac[ETH_ALEN],
>  		      unsigned int vid, enum macaccess_entry_type type)
>  {
> +	u32 flags = ANA_TABLES_MACACCESS_VALID |
> +		    ANA_TABLES_MACACCESS_DEST_IDX(port) |
> +		    ANA_TABLES_MACACCESS_ENTRYTYPE(type) |
> +		    ANA_TABLES_MACACCESS_MAC_TABLE_CMD(MACACCESS_CMD_LE
> ARN);
> +
>  	ocelot_mact_select(ocelot, mac, vid);
>  
> +	/* Little API trickery to make this function "just work" when
> the CPU
> +	 * port module is included in the port mask for multicast IP
> entries.
> +	 */
> +	if (type == ENTRYTYPE_MACv4 || type == ENTRYTYPE_MACv6) {
> +		unsigned long ports = ocelot_decode_ports_from_mdb(mac,
> type);
> +
> +		if (ports & BIT(ocelot->num_phys_ports))
> +			flags |= ANA_TABLES_MACACCESS_MAC_CPU_COPY;
> +	}
> +
>  	/* Issue a write command */
> -	ocelot_write(ocelot, ANA_TABLES_MACACCESS_VALID |
> -			     ANA_TABLES_MACACCESS_DEST_IDX(port) |
> -			     ANA_TABLES_MACACCESS_ENTRYTYPE(type) |
> -			     ANA_TABLES_MACACCESS_MAC_TABLE_CMD(MACACCE
> SS_CMD_LEARN),
> -			     ANA_TABLES_MACACCESS);
> +	ocelot_write(ocelot, flags, ANA_TABLES_MACACCESS);
>  
>  	return ocelot_mact_wait_for_completion(ocelot);
>  }
> -----------------------------[cut here]-----------------------------
> 
> It has the advantage of actually compiling, plus it should be easier
> to backport because the changes are all in one place.
> 
> 
> Please make sure to read:
> Documentation/process/submitting-patches.rst
> (this will tell you what is wrong with your Fixes: tag)
> Documentation/networking/netdev-FAQ.rst
> (this will tell you what is wrong with this patch's --subject-prefix,
> and why the patch does not build on the trees it is supposed to be
> applied to):
> https://patchwork.kernel.org/project/netdevbpf/patch/20210118160317.554018-1-alban.bedel@aerq.com/

I must say that this condescending tone is a real turn off.

Alban

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] net: mscc: ocelot: Fix multicast to the CPU port

[Vladimir Oltean]

On Tue, Jan 19, 2021 at 10:12:34AM +0000, Bedel, Alban wrote:
> My board use felix and I build without CONFIG_MSCC_OCELOT_SWITCH so I
> missed these, my bad.

Ah, ok, this makes sense. It happens to me too to forget to build with
CONFIG_MSCC_OCELOT_SWITCH enabled, more often than I'd like to admit.

> I must say that this condescending tone is a real turn off.

English is not my first language, it probably sounds harsher than it is
when translated ;)

So could you please resend a patch which:
- has a Fixes: tag with 12 characters in the sha1sum
- compiles with CONFIG_MSCC_OCELOT_SWITCH
- has a --subject-prefix of "PATCH v2 net"
- can be cherry-picked on linux-5.10.y and linux-5.9.y from
  https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/,
  but also to the net tree, which is where the patch will first be
  applied:
  https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git