Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     "Bedel, Alban" <alban.bedel@aerq.com>
CC:     Claudiu Manoil <claudiu.manoil@nxp.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        "UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] net: mscc: ocelot: Fix multicast to the CPU port
Thread-Topic: [PATCH] net: mscc: ocelot: Fix multicast to the CPU port
Thread-Index: AQHW7bOA9fYvcfmYzUChcnWV1prsRqotu3KA
Date:   Mon, 18 Jan 2021 18:55:01 +0000
Message-ID: <20210118185501.6wejo4xwb2lidicm@skbuf>
References: <20210118160317.554018-1-alban.bedel@aerq.com>
In-Reply-To: <20210118160317.554018-1-alban.bedel@aerq.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <5825BCDFE6426548BECBBF83C7635D22@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 85e53ce7-e104-4f9c-8295-08d8bbe28fa8
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Jan 2021 18:55:01.9134
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: mgnCRYiiHC/lZfcKuHLibJ4rhwgGK8EzuTz98heEd9xU/7BUqdT2ZwhZHFl8bO7OK0AVlfDyi+UXYLzkIrwlHA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0402MB3711
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.
>=20
> 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.
>=20
> 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.
>=20
> 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,
=20
 }
=20
+static unsigned long
+ocelot_decode_ports_from_mdb(const unsigned char *addr,
+			     enum macaccess_entry_type entry_type)
+{
+	unsigned long ports =3D 0;
+
+	if (entry_type =3D=3D ENTRYTYPE_MACv4) {
+		ports =3D addr[2];
+		ports |=3D addr[1] << 8;
+	} else if (entry_type =3D=3D ENTRYTYPE_MACv6) {
+		ports =3D addr[1];
+		ports |=3D 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 =3D 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);
=20
+	/* 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 =3D=3D ENTRYTYPE_MACv4 || type =3D=3D ENTRYTYPE_MACv6) {
+		unsigned long ports =3D ocelot_decode_ports_from_mdb(mac, type);
+
+		if (ports & BIT(ocelot->num_phys_ports))
+			flags |=3D 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);
=20
 	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/=