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 <[email protected]>
> 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/[email protected]/
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 <[email protected]>
> > 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/[email protected]/
I must say that this condescending tone is a real turn off.
Alban
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