| Message ID | 20210716153641.4678-1-ericwouds@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 11d8d98cbeef1496469b268d79938b05524731e8 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mt7530 fix mt7530_fdb_write vid missing ivl bit | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 12 of 12 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 14 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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
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.
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.
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.
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.
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.
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