[PATCH 0/2] net: dsa: mv88e6xxx: fix vlan filtering for 6250

[PATCH 0/2] net: dsa: mv88e6xxx: fix vlan filtering for 6250

[Rasmus Villemoes]

I finally managed to figure out why enabling VLAN filtering on the
6250 broke all (ingressing) traffic,
cf. https://lore.kernel.org/netdev/6424c14e-bd25-2a06-cf0b-f1a07f9a3604@prevas.dk/
.

The first patch is the minimal fix and for net, while the second one
is a little cleanup for net-next.

Rasmus Villemoes (2):
  net: dsa: mv88e6xxx: also read STU state in mv88e6250_g1_vtu_getnext
  net: dsa: mv88e6xxx: use mv88e6185_g1_vtu_getnext() for the 6250

 drivers/net/dsa/mv88e6xxx/chip.c        |  2 +-
 drivers/net/dsa/mv88e6xxx/global1.h     |  2 --
 drivers/net/dsa/mv88e6xxx/global1_vtu.c | 32 ++-----------------------
 3 files changed, 3 insertions(+), 33 deletions(-)

-- 
2.23.0

[PATCH net 1/2] net: dsa: mv88e6xxx: also read STU state in mv88e6250_g1_vtu_getnext

[Rasmus Villemoes]

mv88e6xxx_port_vlan_join checks whether the VTU already contains an
entry for the given vid (via mv88e6xxx_vtu_getnext), and if so, merely
changes the relevant .member[] element and loads the updated entry
into the VTU.

However, at least for the mv88e6250, the on-stack struct
mv88e6xxx_vtu_entry vlan never has its .state[] array explicitly
initialized, neither in mv88e6xxx_port_vlan_join() nor inside the
getnext implementation. So the new entry has random garbage for the
STU bits, breaking VLAN filtering.

When the VTU entry is initially created, those bits are all zero, and
we should make sure to keep them that way when the entry is updated.

Fixes: 92307069a96c (net: dsa: mv88e6xxx: Avoid VTU corruption on 6097)
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/dsa/mv88e6xxx/global1_vtu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/global1_vtu.c b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
index 66ddf67b8737..7b96396be609 100644
--- a/drivers/net/dsa/mv88e6xxx/global1_vtu.c
+++ b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
@@ -351,6 +351,10 @@ int mv88e6250_g1_vtu_getnext(struct mv88e6xxx_chip *chip,
 		if (err)
 			return err;
 
+		err = mv88e6185_g1_stu_data_read(chip, entry);
+		if (err)
+			return err;
+
 		/* VTU DBNum[3:0] are located in VTU Operation 3:0
 		 * VTU DBNum[5:4] are located in VTU Operation 9:8
 		 */
-- 
2.23.0

[PATCH net-next 2/2] net: dsa: mv88e6xxx: use mv88e6185_g1_vtu_getnext() for the 6250

[Rasmus Villemoes]

mv88e6250_g1_vtu_getnext is almost identical to
mv88e6185_g1_vtu_getnext, except for the 6250 only having 64 databases
instead of 256. We can reduce code duplication by simply masking off
the extra two garbage bits when assembling the fid from VTU op [3:0]
and [11:8].

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/dsa/mv88e6xxx/chip.c        |  2 +-
 drivers/net/dsa/mv88e6xxx/global1.h     |  2 --
 drivers/net/dsa/mv88e6xxx/global1_vtu.c | 36 ++-----------------------
 3 files changed, 3 insertions(+), 37 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 5cc1465fd635..8a0df1e903bf 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4023,7 +4023,7 @@ static const struct mv88e6xxx_ops mv88e6250_ops = {
 	.mgmt_rsvd2cpu = mv88e6352_g2_mgmt_rsvd2cpu,
 	.pot_clear = mv88e6xxx_g2_pot_clear,
 	.reset = mv88e6250_g1_reset,
-	.vtu_getnext = mv88e6250_g1_vtu_getnext,
+	.vtu_getnext = mv88e6185_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6250_g1_vtu_loadpurge,
 	.avb_ops = &mv88e6352_avb_ops,
 	.ptp_ops = &mv88e6250_ptp_ops,
diff --git a/drivers/net/dsa/mv88e6xxx/global1.h b/drivers/net/dsa/mv88e6xxx/global1.h
index 80a182c5b98a..d2dd2f4e4730 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.h
+++ b/drivers/net/dsa/mv88e6xxx/global1.h
@@ -336,8 +336,6 @@ int mv88e6185_g1_vtu_getnext(struct mv88e6xxx_chip *chip,
 			     struct mv88e6xxx_vtu_entry *entry);
 int mv88e6185_g1_vtu_loadpurge(struct mv88e6xxx_chip *chip,
 			       struct mv88e6xxx_vtu_entry *entry);
-int mv88e6250_g1_vtu_getnext(struct mv88e6xxx_chip *chip,
-			     struct mv88e6xxx_vtu_entry *entry);
 int mv88e6250_g1_vtu_loadpurge(struct mv88e6xxx_chip *chip,
 			       struct mv88e6xxx_vtu_entry *entry);
 int mv88e6352_g1_vtu_getnext(struct mv88e6xxx_chip *chip,
diff --git a/drivers/net/dsa/mv88e6xxx/global1_vtu.c b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
index 7b96396be609..519ae48ba96e 100644
--- a/drivers/net/dsa/mv88e6xxx/global1_vtu.c
+++ b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
@@ -336,39 +336,6 @@ int mv88e6xxx_g1_vtu_getnext(struct mv88e6xxx_chip *chip,
 	return mv88e6xxx_g1_vtu_vid_read(chip, entry);
 }
 
-int mv88e6250_g1_vtu_getnext(struct mv88e6xxx_chip *chip,
-			     struct mv88e6xxx_vtu_entry *entry)
-{
-	u16 val;
-	int err;
-
-	err = mv88e6xxx_g1_vtu_getnext(chip, entry);
-	if (err)
-		return err;
-
-	if (entry->valid) {
-		err = mv88e6185_g1_vtu_data_read(chip, entry);
-		if (err)
-			return err;
-
-		err = mv88e6185_g1_stu_data_read(chip, entry);
-		if (err)
-			return err;
-
-		/* VTU DBNum[3:0] are located in VTU Operation 3:0
-		 * VTU DBNum[5:4] are located in VTU Operation 9:8
-		 */
-		err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_VTU_OP, &val);
-		if (err)
-			return err;
-
-		entry->fid = val & 0x000f;
-		entry->fid |= (val & 0x0300) >> 4;
-	}
-
-	return 0;
-}
-
 int mv88e6185_g1_vtu_getnext(struct mv88e6xxx_chip *chip,
 			     struct mv88e6xxx_vtu_entry *entry)
 {
@@ -389,7 +356,7 @@ int mv88e6185_g1_vtu_getnext(struct mv88e6xxx_chip *chip,
 			return err;
 
 		/* VTU DBNum[3:0] are located in VTU Operation 3:0
-		 * VTU DBNum[7:4] are located in VTU Operation 11:8
+		 * VTU DBNum[7:4] ([5:4] for 6250) are located in VTU Operation 11:8 (9:8)
 		 */
 		err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_VTU_OP, &val);
 		if (err)
@@ -397,6 +364,7 @@ int mv88e6185_g1_vtu_getnext(struct mv88e6xxx_chip *chip,
 
 		entry->fid = val & 0x000f;
 		entry->fid |= (val & 0x0f00) >> 4;
+		entry->fid &= mv88e6xxx_num_databases(chip) - 1;
 	}
 
 	return 0;
-- 
2.23.0

Re: [PATCH net 1/2] net: dsa: mv88e6xxx: also read STU state in mv88e6250_g1_vtu_getnext

[Florian Fainelli]

On 1/15/2021 6:39 PM, Rasmus Villemoes wrote:
> mv88e6xxx_port_vlan_join checks whether the VTU already contains an
> entry for the given vid (via mv88e6xxx_vtu_getnext), and if so, merely
> changes the relevant .member[] element and loads the updated entry
> into the VTU.
> 
> However, at least for the mv88e6250, the on-stack struct
> mv88e6xxx_vtu_entry vlan never has its .state[] array explicitly
> initialized, neither in mv88e6xxx_port_vlan_join() nor inside the
> getnext implementation. So the new entry has random garbage for the
> STU bits, breaking VLAN filtering.
> 
> When the VTU entry is initially created, those bits are all zero, and
> we should make sure to keep them that way when the entry is updated.
> 
> Fixes: 92307069a96c (net: dsa: mv88e6xxx: Avoid VTU corruption on 6097)
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: use mv88e6185_g1_vtu_getnext() for the 6250

[Florian Fainelli]

On 1/15/2021 6:39 PM, Rasmus Villemoes wrote:
> mv88e6250_g1_vtu_getnext is almost identical to
> mv88e6185_g1_vtu_getnext, except for the 6250 only having 64 databases
> instead of 256. We can reduce code duplication by simply masking off
> the extra two garbage bits when assembling the fid from VTU op [3:0]
> and [11:8].
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net 1/2] net: dsa: mv88e6xxx: also read STU state in mv88e6250_g1_vtu_getnext

[Tobias Waldekranz]

On Sat, Jan 16, 2021 at 03:39, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:
> mv88e6xxx_port_vlan_join checks whether the VTU already contains an
> entry for the given vid (via mv88e6xxx_vtu_getnext), and if so, merely
> changes the relevant .member[] element and loads the updated entry
> into the VTU.
>
> However, at least for the mv88e6250, the on-stack struct
> mv88e6xxx_vtu_entry vlan never has its .state[] array explicitly
> initialized, neither in mv88e6xxx_port_vlan_join() nor inside the
> getnext implementation. So the new entry has random garbage for the
> STU bits, breaking VLAN filtering.
>
> When the VTU entry is initially created, those bits are all zero, and
> we should make sure to keep them that way when the entry is updated.
>
> Fixes: 92307069a96c (net: dsa: mv88e6xxx: Avoid VTU corruption on 6097)
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>
Tested-by: Tobias Waldekranz <tobias@waldekranz.com>

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: use mv88e6185_g1_vtu_getnext() for the 6250

[Tobias Waldekranz]

On Sat, Jan 16, 2021 at 03:39, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:
> mv88e6250_g1_vtu_getnext is almost identical to
> mv88e6185_g1_vtu_getnext, except for the 6250 only having 64 databases
> instead of 256. We can reduce code duplication by simply masking off
> the extra two garbage bits when assembling the fid from VTU op [3:0]
> and [11:8].
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

We might also want to give mv88e6250_g1_vtu_loadpurge the same
treatment.

Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>
Tested-by: Tobias Waldekranz <tobias@waldekranz.com>

Re: [PATCH 0/2] net: dsa: mv88e6xxx: fix vlan filtering for 6250

[Vladimir Oltean]

Hi Rasmus,

On Sat, Jan 16, 2021 at 03:39:34AM +0100, Rasmus Villemoes wrote:
> I finally managed to figure out why enabling VLAN filtering on the
> 6250 broke all (ingressing) traffic,
> cf. https://lore.kernel.org/netdev/6424c14e-bd25-2a06-cf0b-f1a07f9a3604@prevas.dk/
> .
> 
> The first patch is the minimal fix and for net, while the second one
> is a little cleanup for net-next.
> 
> Rasmus Villemoes (2):
>   net: dsa: mv88e6xxx: also read STU state in mv88e6250_g1_vtu_getnext
>   net: dsa: mv88e6xxx: use mv88e6185_g1_vtu_getnext() for the 6250

It's strange to put a patch for net and one for net-next in the same
series. Nobody will keep a note for you to apply the second patch after
net has been merged back into net-next. So if you want to keep the
two-patch approach, you'd have to send just the "net" patch now, and the
"net-next" patch later.
But is there any reason why you don't just apply the second patch to
"net"?

Re: [PATCH 0/2] net: dsa: mv88e6xxx: fix vlan filtering for 6250

[Rasmus Villemoes]

On 17/01/2021 22.08, Vladimir Oltean wrote:
> Hi Rasmus,
> 
> On Sat, Jan 16, 2021 at 03:39:34AM +0100, Rasmus Villemoes wrote:
>> I finally managed to figure out why enabling VLAN filtering on the
>> 6250 broke all (ingressing) traffic,
>> cf. https://lore.kernel.org/netdev/6424c14e-bd25-2a06-cf0b-f1a07f9a3604@prevas.dk/
>> .
>>
>> The first patch is the minimal fix and for net, while the second one
>> is a little cleanup for net-next.
>>
>> Rasmus Villemoes (2):
>>   net: dsa: mv88e6xxx: also read STU state in mv88e6250_g1_vtu_getnext
>>   net: dsa: mv88e6xxx: use mv88e6185_g1_vtu_getnext() for the 6250
> 
> It's strange to put a patch for net and one for net-next in the same
> series. 

Well, maybe, but one is a logical continuation of the other, and
including the second one preempted review comments saying "why don't you
merge the two implementations".

> But is there any reason why you don't just apply the second patch to
> "net"?

That's not really for me to decide? I thought net was just for the
things that needed fixing and should be sent to -stable - which is the
only reason I even split this in two, so there's a minimal logical fix
for the 6250. Otherwise I'd just have squashed the two, so that I don't
add lines only to delete them, along with the rest of the function, later.

Jakub, David, it's up to you.

Rasmus

Re: [PATCH 0/2] net: dsa: mv88e6xxx: fix vlan filtering for 6250

[Jakub Kicinski]

On Mon, 18 Jan 2021 14:22:57 +0100 Rasmus Villemoes wrote:
> On 17/01/2021 22.08, Vladimir Oltean wrote:
> > Hi Rasmus,
> > 
> > On Sat, Jan 16, 2021 at 03:39:34AM +0100, Rasmus Villemoes wrote:  
> >> I finally managed to figure out why enabling VLAN filtering on the
> >> 6250 broke all (ingressing) traffic,
> >> cf. https://lore.kernel.org/netdev/6424c14e-bd25-2a06-cf0b-f1a07f9a3604@prevas.dk/
> >> .
> >>
> >> The first patch is the minimal fix and for net, while the second one
> >> is a little cleanup for net-next.
> >>
> >> Rasmus Villemoes (2):
> >>   net: dsa: mv88e6xxx: also read STU state in mv88e6250_g1_vtu_getnext
> >>   net: dsa: mv88e6xxx: use mv88e6185_g1_vtu_getnext() for the 6250  
> > 
> > It's strange to put a patch for net and one for net-next in the same
> > series.   
> 
> Well, maybe, but one is a logical continuation of the other, and
> including the second one preempted review comments saying "why don't you
> merge the two implementations".
> 
> > But is there any reason why you don't just apply the second patch to
> > "net"?  
> 
> That's not really for me to decide? I thought net was just for the
> things that needed fixing and should be sent to -stable - which is the
> only reason I even split this in two, so there's a minimal logical fix
> for the 6250. Otherwise I'd just have squashed the two, so that I don't
> add lines only to delete them, along with the rest of the function, later.
> 
> Jakub, David, it's up to you.

Vladimir is right, this is a strange way to post things. In the future
please send just the "net" changes first and include a note in the
cover letter or under "---" saying something like "cleanup of XYZ is
left for a followup in -next".

I've applied patch 1 please resend the cleanup after net->net-next
merge (~Friday).

Thanks!