[PATCH] mt7530 fix mt7530_fdb_write vid missing ivl bit

[PATCH] mt7530 fix mt7530_fdb_write vid missing ivl bit

[ericwouds]

From: Eric Woudstra <ericwouds@gmail.com>

According to reference guides mt7530 (mt7620) and mt7531:

NOTE: When IVL is reset, MAC[47:0] and FID[2:0] will be used to 
read/write the address table. When IVL is set, MAC[47:0] and CVID[11:0] 
will be used to read/write the address table.

Since the function only fills in CVID and no FID, we need to set the
IVL bit. The existing code does not set it.

This is a fix for the issue I dropped here earlier:

http://lists.infradead.org/pipermail/linux-mediatek/2021-June/025697.html

With this patch, it is now possible to delete the 'self' fdb entry
manually. However, wifi roaming still has the same issue, the entry
does not get deleted automatically. Wifi roaming also needs a fix
somewhere else to function correctly in combination with vlan.

Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
---
 drivers/net/dsa/mt7530.c | 1 +
 drivers/net/dsa/mt7530.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 93136f7e6..9e4df35f9 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -366,6 +366,7 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
 	int i;
 
 	reg[1] |= vid & CVID_MASK;
+	reg[1] |= ATA2_IVL;
 	reg[2] |= (aging & AGE_TIMER_MASK) << AGE_TIMER;
 	reg[2] |= (port_mask & PORT_MAP_MASK) << PORT_MAP;
 	/* STATIC_ENT indicate that entry is static wouldn't
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 334d610a5..b19b389ff 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -79,6 +79,7 @@ enum mt753x_bpdu_port_fw {
 #define  STATIC_EMP			0
 #define  STATIC_ENT			3
 #define MT7530_ATA2			0x78
+#define  ATA2_IVL			BIT(15)
 
 /* Register for address table write data */
 #define MT7530_ATWD			0x7c
-- 
2.25.1

Re: [PATCH] mt7530 fix mt7530_fdb_write vid missing ivl bit

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Fri, 16 Jul 2021 17:36:39 +0200 you wrote:
> From: Eric Woudstra <ericwouds@gmail.com>
> 
> According to reference guides mt7530 (mt7620) and mt7531:
> 
> NOTE: When IVL is reset, MAC[47:0] and FID[2:0] will be used to
> read/write the address table. When IVL is set, MAC[47:0] and CVID[11:0]
> will be used to read/write the address table.
> 
> [...]

Here is the summary with links:
  - mt7530 fix mt7530_fdb_write vid missing ivl bit
    https://git.kernel.org/netdev/net/c/11d8d98cbeef

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH] mt7530 fix mt7530_fdb_write vid missing ivl bit

[Vladimir Oltean]

On Fri, Jul 16, 2021 at 05:36:39PM +0200, ericwouds@gmail.com wrote:
> From: Eric Woudstra <ericwouds@gmail.com>
>
> According to reference guides mt7530 (mt7620) and mt7531:
>
> NOTE: When IVL is reset, MAC[47:0] and FID[2:0] will be used to
> read/write the address table. When IVL is set, MAC[47:0] and CVID[11:0]
> will be used to read/write the address table.
>
> Since the function only fills in CVID and no FID, we need to set the
> IVL bit. The existing code does not set it.
>
> This is a fix for the issue I dropped here earlier:
>
> http://lists.infradead.org/pipermail/linux-mediatek/2021-June/025697.html
>
> With this patch, it is now possible to delete the 'self' fdb entry
> manually. However, wifi roaming still has the same issue, the entry
> does not get deleted automatically. Wifi roaming also needs a fix
> somewhere else to function correctly in combination with vlan.
>
> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
> ---
>  drivers/net/dsa/mt7530.c | 1 +
>  drivers/net/dsa/mt7530.h | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 93136f7e6..9e4df35f9 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -366,6 +366,7 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
>  	int i;
>
>  	reg[1] |= vid & CVID_MASK;
> +	reg[1] |= ATA2_IVL;
>  	reg[2] |= (aging & AGE_TIMER_MASK) << AGE_TIMER;
>  	reg[2] |= (port_mask & PORT_MAP_MASK) << PORT_MAP;
>  	/* STATIC_ENT indicate that entry is static wouldn't
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index 334d610a5..b19b389ff 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -79,6 +79,7 @@ enum mt753x_bpdu_port_fw {
>  #define  STATIC_EMP			0
>  #define  STATIC_ENT			3
>  #define MT7530_ATA2			0x78
> +#define  ATA2_IVL			BIT(15)
>
>  /* Register for address table write data */
>  #define MT7530_ATWD			0x7c
> --
> 2.25.1
>

Can VLAN-unaware FDB entries still be manipulated successfully after
this patch, since it changes 'fid 0' to be interpreted as 'vid 0'?

What is your problem with roaming exactly? Have you tried to disable
hardware address learning on the CPU port and set
ds->assisted_learning_on_cpu_port = true for mt7530?

Please note that since kernel v5.14, raw 'self' entries can no longer be
managed directly using 'bridge fdb', you need to use 'master static' and
go through the bridge:
https://www.kernel.org/doc/html/latest/networking/dsa/configuration.html#forwarding-database-fdb-management
You will need to update your 'bridgefdbd' program, if it proves to be at
all necessary to achieve what you want.

Re: [PATCH] mt7530 fix mt7530_fdb_write vid missing ivl bit

[Eric Woudstra]

You are right now there is a problem with vlan unaware bridge.

We need to change the line to:

if (vid > 1) reg[1] |= ATA2_IVL;

I have just tested this with a vlan unaware bridge and also with vlan bridge option disabled in the kernel. Only after applying the if statement it works for vlan unaware bridges/kernel.

On Jul 16, 2021, 11:06 PM, at 11:06 PM, Vladimir Oltean <olteanv@gmail.com> wrote:
>On Fri, Jul 16, 2021 at 05:36:39PM +0200, ericwouds@gmail.com wrote:
>> From: Eric Woudstra <ericwouds@gmail.com>
>>
>> According to reference guides mt7530 (mt7620) and mt7531:
>>
>> NOTE: When IVL is reset, MAC[47:0] and FID[2:0] will be used to
>> read/write the address table. When IVL is set, MAC[47:0] and
>CVID[11:0]
>> will be used to read/write the address table.
>>
>> Since the function only fills in CVID and no FID, we need to set the
>> IVL bit. The existing code does not set it.
>>
>> This is a fix for the issue I dropped here earlier:
>>
>>
>http://lists.infradead.org/pipermail/linux-mediatek/2021-June/025697.html
>>
>> With this patch, it is now possible to delete the 'self' fdb entry
>> manually. However, wifi roaming still has the same issue, the entry
>> does not get deleted automatically. Wifi roaming also needs a fix
>> somewhere else to function correctly in combination with vlan.
>>
>> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
>> ---
>>  drivers/net/dsa/mt7530.c | 1 +
>>  drivers/net/dsa/mt7530.h | 1 +
>>  2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 93136f7e6..9e4df35f9 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -366,6 +366,7 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16
>vid,
>>  	int i;
>>
>>  	reg[1] |= vid & CVID_MASK;
>> +	reg[1] |= ATA2_IVL;
>>  	reg[2] |= (aging & AGE_TIMER_MASK) << AGE_TIMER;
>>  	reg[2] |= (port_mask & PORT_MAP_MASK) << PORT_MAP;
>>  	/* STATIC_ENT indicate that entry is static wouldn't
>> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
>> index 334d610a5..b19b389ff 100644
>> --- a/drivers/net/dsa/mt7530.h
>> +++ b/drivers/net/dsa/mt7530.h
>> @@ -79,6 +79,7 @@ enum mt753x_bpdu_port_fw {
>>  #define  STATIC_EMP			0
>>  #define  STATIC_ENT			3
>>  #define MT7530_ATA2			0x78
>> +#define  ATA2_IVL			BIT(15)
>>
>>  /* Register for address table write data */
>>  #define MT7530_ATWD			0x7c
>> --
>> 2.25.1
>>
>
>Can VLAN-unaware FDB entries still be manipulated successfully after
>this patch, since it changes 'fid 0' to be interpreted as 'vid 0'?
>
>What is your problem with roaming exactly? Have you tried to disable
>hardware address learning on the CPU port and set
>ds->assisted_learning_on_cpu_port = true for mt7530?
>
>Please note that since kernel v5.14, raw 'self' entries can no longer
>be
>managed directly using 'bridge fdb', you need to use 'master static'
>and
>go through the bridge:
>https://www.kernel.org/doc/html/latest/networking/dsa/configuration.html#forwarding-database-fdb-management
>You will need to update your 'bridgefdbd' program, if it proves to be
>at
>all necessary to achieve what you want.

Re: [PATCH] mt7530 fix mt7530_fdb_write vid missing ivl bit

[Vladimir Oltean]

On Sat, Jul 17, 2021 at 10:09:53AM +0200, Eric Woudstra wrote:
> You are right now there is a problem with vlan unaware bridge.
>
> We need to change the line to:
>
> if (vid > 1) reg[1] |= ATA2_IVL;
>
> I have just tested this with a vlan unaware bridge and also with vlan
> bridge option disabled in the kernel. Only after applying the if
> statement it works for vlan unaware bridges/kernel.

Ok, make sure to read Documentation/process/submitting-patches.rst for
how to add a Fixes: tag to your patch, Documentation/networking/netdev-FAQ.rst
for how to set the subject-prefix to "PATCH net" in your git-send-email command,
and send a fixup patch.

Re: [PATCH] mt7530 fix mt7530_fdb_write vid missing ivl bit

[DENG Qingfang]

On Sat, Jul 17, 2021 at 10:09:53AM +0200, Eric Woudstra wrote:
> 
> You are right now there is a problem with vlan unaware bridge.
> 
> We need to change the line to:
> 
> if (vid > 1) reg[1] |= ATA2_IVL;

Does it not work with vid 1?

> 
> I have just tested this with a vlan unaware bridge and also with vlan bridge option disabled in the kernel. Only after applying the if statement it works for vlan unaware bridges/kernel.

Re: [PATCH] mt7530 fix mt7530_fdb_write vid missing ivl bit

[Eric Woudstra]

On Sat 17 jul. 2021 at 17:41, DENG Qingfang <dqfext@gmail.com>: wrote:
>
> On Sat, Jul 17, 2021 at 10:09:53AM +0200, Eric Woudstra wrote:
> >
> > You are right now there is a problem with vlan unaware bridge.
> >
> > We need to change the line to:
> >
> > if (vid > 1) reg[1] |= ATA2_IVL;
>
> Does it not work with vid 1?

No, I also thought so, but it actually does not. I'm working here on
5.12.11, but there should not be any difference. It needs: if (vid >
1). Just tried it with (vid > 0) but then it does not work.

I really like your fix on wifi roaming, it works nicely. However I
found, still after this patch, it sadly does not work on vlan > 1. At
least it does not on 5.12.11 (the 'self' entry does not get removed
automatically, but after manual remove the client connects ok). I need
to go 5.14 one of these days because I just read DSA has a major
update. Then I also move from ubuntu focal to a more recent version.
Then I'll try wifi roaming on vlan again.

>
> >
> > I have just tested this with a vlan unaware bridge and also with vlan bridge option disabled in the kernel. Only after applying the if statement it works for vlan unaware bridges/kernel.

Re: [PATCH] mt7530 fix mt7530_fdb_write vid missing ivl bit

[Vladimir Oltean]

On Mon, Aug 09, 2021 at 01:00:24AM +0800, DENG Qingfang wrote:
> On Fri, Jul 16, 2021 at 05:22:11PM +0200, ericwouds@gmail.com wrote:
> > From: Eric Woudstra <37153012+ericwoud@users.noreply.github.com>
> >
> > According to reference guides mt7530 (mt7620) and mt7531:
> >
> > NOTE: When IVL is reset, MAC[47:0] and FID[2:0] will be used to
> > read/write the address table. When IVL is set, MAC[47:0] and CVID[11:0]
> > will be used to read/write the address table.
> >
> > Since the function only fills in CVID and no FID, we need to set the
> > IVL bit. The existing code does not set it.
> >
> > This is a fix for the issue I dropped here earlier:
> >
> > http://lists.infradead.org/pipermail/linux-mediatek/2021-June/025697.html
> >
> > With this patch, it is now possible to delete the 'self' fdb entry
> > manually. However, wifi roaming still has the same issue, the entry
> > does not get deleted automatically. Wifi roaming also needs a fix
> > somewhere else to function correctly in combination with vlan.
>
> Sorry to bump this up, but I think I identified the issue:
>
> Consider a VLAN-aware bridge br0, with two ports set to different PVIDs:
>
> > bridge vlan
> > port         vlan-id
> > swp0         1 PVID Egress Untagged
> > swp1         2 PVID Egress Untagged
>
> When the bridge core sends a packet to swp1, the packet will be sent to
> the CPU port of the switch as untagged because swp1 is set as "Egress
> Untagged". However if the switch uses independent VLAN learning, the CPU
> port PVID will be used to update the FDB.

Sadly the Banana Pi MT7531 reference manual I have does not appear to
cover the DSA tagging header, so I am not actually clear what
MTK_HDR_XMIT_SA_DIS does when not set. Does it default to the CPU port's
value from the PSC register?

If it does, then I expect that your patch 0b69c54c74bc ("net: dsa:
mt7530: enable assisted learning on CPU port") fixes the issue Eric was
seeing, which in turn was caused by your other patch 5e5502e012b8 ("net:
dsa: mt7530: fix roaming from DSA user ports").

> As we don't change its PVID
> (not reasonable to change it anyway), hardware learning may not update
> the correct FDB.
>
> A possible solution is always send packets as tagged when serving a
> VLAN-aware bridge.

So as usual, VLANs put the "hard" in "hardware learning on the CPU port".
I would say "a possible solution is to not attempt to learn from
CPU-injected frames unless they are sent using the tx_fwd_offload
framework"....

>
> mv88e6xxx has been using hardware learning on CPU port since commit
> d82f8ab0d874 ("net: dsa: tag_dsa: offload the bridge forwarding process"),
> does it have the same issue?

...which ensures that bridge data plane packets are always sent to the
CPU port as VLAN-tagged:

br_handle_vlan:

	/* If the skb will be sent using forwarding offload, the assumption is
	 * that the switchdev will inject the packet into hardware together
	 * with the bridge VLAN, so that it can be forwarded according to that
	 * VLAN. The switchdev should deal with popping the VLAN header in
	 * hardware on each egress port as appropriate. So only strip the VLAN
	 * header if forwarding offload is not being used.
	 */
	if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED &&
	    !br_switchdev_frame_uses_tx_fwd_offload(skb))
		__vlan_hwaccel_clear_tag(skb);

Seriously, I expect that a packet injected through the CPU port will,
under normal circumstances, not default not look up the FDB, not update
the FDB, etc etc.

As long as you let the frame analyzer look in depth at the packet you do
need to ensure that it has a valid VLAN ID. Otherwise it is an actual
forwarding correctness issue and not just a "learn in wrong VLAN" issue:

https://patchwork.kernel.org/project/netdevbpf/patch/20210426170411.1789186-6-tobias@waldekranz.com/

Re: [PATCH] mt7530 fix mt7530_fdb_write vid missing ivl bit

[DENG Qingfang]

On Fri, Jul 16, 2021 at 05:22:11PM +0200, ericwouds@gmail.com wrote:
> From: Eric Woudstra <37153012+ericwoud@users.noreply.github.com>
> 
> According to reference guides mt7530 (mt7620) and mt7531:
> 
> NOTE: When IVL is reset, MAC[47:0] and FID[2:0] will be used to 
> read/write the address table. When IVL is set, MAC[47:0] and CVID[11:0] 
> will be used to read/write the address table.
> 
> Since the function only fills in CVID and no FID, we need to set the
> IVL bit. The existing code does not set it.
> 
> This is a fix for the issue I dropped here earlier:
> 
> http://lists.infradead.org/pipermail/linux-mediatek/2021-June/025697.html
> 
> With this patch, it is now possible to delete the 'self' fdb entry
> manually. However, wifi roaming still has the same issue, the entry
> does not get deleted automatically. Wifi roaming also needs a fix
> somewhere else to function correctly in combination with vlan.

Sorry to bump this up, but I think I identified the issue:

Consider a VLAN-aware bridge br0, with two ports set to different PVIDs:

> bridge vlan
> port         vlan-id
> swp0         1 PVID Egress Untagged
> swp1         2 PVID Egress Untagged

When the bridge core sends a packet to swp1, the packet will be sent to
the CPU port of the switch as untagged because swp1 is set as "Egress
Untagged". However if the switch uses independent VLAN learning, the CPU
port PVID will be used to update the FDB. As we don't change its PVID
(not reasonable to change it anyway), hardware learning may not update
the correct FDB.

A possible solution is always send packets as tagged when serving a
VLAN-aware bridge.

mv88e6xxx has been using hardware learning on CPU port since commit
d82f8ab0d874 ("net: dsa: tag_dsa: offload the bridge forwarding process"),
does it have the same issue?

Re: [PATCH] mt7530 fix mt7530_fdb_write vid missing ivl bit

[Andrew Lunn]

On Fri, Jul 16, 2021 at 05:22:11PM +0200, ericwouds@gmail.com wrote:
> From: Eric Woudstra <37153012+ericwoud@users.noreply.github.com>
> 
> According to reference guides mt7530 (mt7620) and mt7531:
> 
> NOTE: When IVL is reset, MAC[47:0] and FID[2:0] will be used to 
> read/write the address table. When IVL is set, MAC[47:0] and CVID[11:0] 
> will be used to read/write the address table.
> 
> Since the function only fills in CVID and no FID, we need to set the
> IVL bit. The existing code does not set it.
> 
> This is a fix for the issue I dropped here earlier:
> 
> http://lists.infradead.org/pipermail/linux-mediatek/2021-June/025697.html
> 
> With this patch, it is now possible to delete the 'self' fdb entry
> manually. However, wifi roaming still has the same issue, the entry
> does not get deleted automatically. Wifi roaming also needs a fix
> somewhere else to function correctly in combination with vlan.
> 
> Signed-off-by: Eric Woudstra <37153012+ericwoud@users.noreply.github.com>

Hi Eric

We need a real email address in the Signed-off-by, and the noreply bit
makes me think this will not work.

      Andrew

[PATCH] mt7530 fix mt7530_fdb_write vid missing ivl bit

[ericwouds]

From: Eric Woudstra <37153012+ericwoud@users.noreply.github.com>

According to reference guides mt7530 (mt7620) and mt7531:

NOTE: When IVL is reset, MAC[47:0] and FID[2:0] will be used to 
read/write the address table. When IVL is set, MAC[47:0] and CVID[11:0] 
will be used to read/write the address table.

Since the function only fills in CVID and no FID, we need to set the
IVL bit. The existing code does not set it.

This is a fix for the issue I dropped here earlier:

http://lists.infradead.org/pipermail/linux-mediatek/2021-June/025697.html

With this patch, it is now possible to delete the 'self' fdb entry
manually. However, wifi roaming still has the same issue, the entry
does not get deleted automatically. Wifi roaming also needs a fix
somewhere else to function correctly in combination with vlan.

Signed-off-by: Eric Woudstra <37153012+ericwoud@users.noreply.github.com>
---
 drivers/net/dsa/mt7530.c | 1 +
 drivers/net/dsa/mt7530.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 93136f7e6..9e4df35f9 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -366,6 +366,7 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
 	int i;
 
 	reg[1] |= vid & CVID_MASK;
+	reg[1] |= ATA2_IVL;
 	reg[2] |= (aging & AGE_TIMER_MASK) << AGE_TIMER;
 	reg[2] |= (port_mask & PORT_MAP_MASK) << PORT_MAP;
 	/* STATIC_ENT indicate that entry is static wouldn't
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 334d610a5..b19b389ff 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -79,6 +79,7 @@ enum mt753x_bpdu_port_fw {
 #define  STATIC_EMP			0
 #define  STATIC_ENT			3
 #define MT7530_ATA2			0x78
+#define  ATA2_IVL			BIT(15)
 
 /* Register for address table write data */
 #define MT7530_ATWD			0x7c
-- 
2.25.1