[PATCH net-next] net: dsa: mv88e6xxx: 6352: parse VTU data before loading STU data

[PATCH net-next] net: dsa: mv88e6xxx: 6352: parse VTU data before loading STU data

[Tobias Waldekranz]

On the 6352, doing a VTU GetNext op, followed by an STU GetNext op
will leave you with both the member- and state- data in the VTU/STU
data registers. But on the 6097 (which uses the same implementation),
the STU GetNext will override the information gathered from the VTU
GetNext.

Separate the two stages, parsing the result of the VTU GetNext before
doing the STU GetNext.

We opt to update the existing implementation for all applicable chips,
as opposed to creating a separate callback for 6097. Though the
previous implementation did work for (at least) 6352, the datasheet
does not mention the masking behavior.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---

I was not sure if I should have created a separate callback, but I
have not found any documentation that suggests that you can expect the
STU GetNext op to mask the bits that are used to store VTU membership
information in the way that 6352 does. So depending on undocumented
behavior felt like something we would want to get rid of anyway.

Tested on 6097F and 6352.

drivers/net/dsa/mv88e6xxx/global1_vtu.c | 55 ++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 10 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/global1_vtu.c b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
index 48390b7b18ad..2f146645a723 100644
--- a/drivers/net/dsa/mv88e6xxx/global1_vtu.c
+++ b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
@@ -125,11 +125,9 @@ static int mv88e6xxx_g1_vtu_vid_write(struct mv88e6xxx_chip *chip,
  * Offset 0x08: VTU/STU Data Register 2
  * Offset 0x09: VTU/STU Data Register 3
  */
-
-static int mv88e6185_g1_vtu_data_read(struct mv88e6xxx_chip *chip,
-				      struct mv88e6xxx_vtu_entry *entry)
+static int mv88e6185_g1_vtu_stu_data_read(struct mv88e6xxx_chip *chip,
+					  u16 *regs)
 {
-	u16 regs[3];
 	int i;
 
 	/* Read all 3 VTU/STU Data registers */
@@ -142,12 +140,45 @@ static int mv88e6185_g1_vtu_data_read(struct mv88e6xxx_chip *chip,
 			return err;
 	}
 
-	/* Extract MemberTag and PortState data */
+	return 0;
+}
+
+static int mv88e6185_g1_vtu_data_read(struct mv88e6xxx_chip *chip,
+				      struct mv88e6xxx_vtu_entry *entry)
+{
+	u16 regs[3];
+	int err;
+	int i;
+
+	err = mv88e6185_g1_vtu_stu_data_read(chip, regs);
+	if (err)
+		return err;
+
+	/* Extract MemberTag data */
 	for (i = 0; i < mv88e6xxx_num_ports(chip); ++i) {
 		unsigned int member_offset = (i % 4) * 4;
-		unsigned int state_offset = member_offset + 2;
 
 		entry->member[i] = (regs[i / 4] >> member_offset) & 0x3;
+	}
+
+	return 0;
+}
+
+static int mv88e6185_g1_stu_data_read(struct mv88e6xxx_chip *chip,
+				      struct mv88e6xxx_vtu_entry *entry)
+{
+	u16 regs[3];
+	int err;
+	int i;
+
+	err = mv88e6185_g1_vtu_stu_data_read(chip, regs);
+	if (err)
+		return err;
+
+	/* Extract PortState data */
+	for (i = 0; i < mv88e6xxx_num_ports(chip); ++i) {
+		unsigned int state_offset = (i % 4) * 4 + 2;
+
 		entry->state[i] = (regs[i / 4] >> state_offset) & 0x3;
 	}
 
@@ -374,16 +405,20 @@ int mv88e6352_g1_vtu_getnext(struct mv88e6xxx_chip *chip,
 		return err;
 
 	if (entry->valid) {
-		/* Fetch (and mask) VLAN PortState data from the STU */
-		err = mv88e6xxx_g1_vtu_stu_get(chip, entry);
+		err = mv88e6185_g1_vtu_data_read(chip, entry);
 		if (err)
 			return err;
 
-		err = mv88e6185_g1_vtu_data_read(chip, entry);
+		err = mv88e6xxx_g1_vtu_fid_read(chip, entry);
 		if (err)
 			return err;
 
-		err = mv88e6xxx_g1_vtu_fid_read(chip, entry);
+		/* Fetch VLAN PortState data from the STU */
+		err = mv88e6xxx_g1_vtu_stu_get(chip, entry);
+		if (err)
+			return err;
+
+		err = mv88e6185_g1_stu_data_read(chip, entry);
 		if (err)
 			return err;
 	}
-- 
2.17.1

Re: [PATCH net-next] net: dsa: mv88e6xxx: 6352: parse VTU data before loading STU data

[Jakub Kicinski]

On Sun,  8 Nov 2020 23:38:10 +0100 Tobias Waldekranz wrote:
> On the 6352, doing a VTU GetNext op, followed by an STU GetNext op
> will leave you with both the member- and state- data in the VTU/STU
> data registers. But on the 6097 (which uses the same implementation),
> the STU GetNext will override the information gathered from the VTU
> GetNext.
> 
> Separate the two stages, parsing the result of the VTU GetNext before
> doing the STU GetNext.
> 
> We opt to update the existing implementation for all applicable chips,
> as opposed to creating a separate callback for 6097. Though the
> previous implementation did work for (at least) 6352, the datasheet
> does not mention the masking behavior.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---
> 
> I was not sure if I should have created a separate callback, but I
> have not found any documentation that suggests that you can expect the
> STU GetNext op to mask the bits that are used to store VTU membership
> information in the way that 6352 does. So depending on undocumented
> behavior felt like something we would want to get rid of anyway.
> 
> Tested on 6097F and 6352.

I'm unclear what this fixes. What functionality is broken on 6097?
Can we identify the commit for a fixes tag?

Re: [PATCH net-next] net: dsa: mv88e6xxx: 6352: parse VTU data before loading STU data

[Tobias Waldekranz]

On Wed Nov 11, 2020 at 4:27 PM CET, Jakub Kicinski wrote:
> On Sun, 8 Nov 2020 23:38:10 +0100 Tobias Waldekranz wrote:
> > On the 6352, doing a VTU GetNext op, followed by an STU GetNext op
> > will leave you with both the member- and state- data in the VTU/STU
> > data registers. But on the 6097 (which uses the same implementation),
> > the STU GetNext will override the information gathered from the VTU
> > GetNext.
> > 
> > Separate the two stages, parsing the result of the VTU GetNext before
> > doing the STU GetNext.
> > 
> > We opt to update the existing implementation for all applicable chips,
> > as opposed to creating a separate callback for 6097. Though the
> > previous implementation did work for (at least) 6352, the datasheet
> > does not mention the masking behavior.
> > 
> > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> > ---
> > 
> > I was not sure if I should have created a separate callback, but I
> > have not found any documentation that suggests that you can expect the
> > STU GetNext op to mask the bits that are used to store VTU membership
> > information in the way that 6352 does. So depending on undocumented
> > behavior felt like something we would want to get rid of anyway.
> > 
> > Tested on 6097F and 6352.
>
> I'm unclear what this fixes. What functionality is broken on 6097?

VLAN configuration. As soon as you add the second port to a VLAN, all
other port membership configuration is overwritten with zeroes. The HW
interprets this as all ports being "unmodified members" of the VLAN.

I suspect that is why it has not been discovered. In the simple case
when all ports belong to the same VLAN, switching will still work. But
using multiple VLANs or trying to set multiple ports as tagged members
will not work.

At the lowest level, the current implementation assumes that it can
perform two consecutive operations where each op will load half of a
register, and then read out the union of the information. This is true
for some devices (6352), but not for others (6097).

6352 pseudo-hdl-in-c:

stu_get_next()
{
	*data |= stu_data & 0xf0f0;
}

vtu_get_next()
{
	*data |= vtu_data & 0x0f0f;
}

6097 pseudo-hdl-in-c:

stu_get_next()
{
	*data = stu_data;
}

vtu_get_next()
{
	*data = vtu_data;
}

> Can we identify the commit for a fixes tag?

I will try to pinpoint it tomorrow. I suppose I should also rebase it
against "net" since it is a bug.

Re: [PATCH net-next] net: dsa: mv88e6xxx: 6352: parse VTU data before loading STU data

[Jakub Kicinski]

On Thu, 12 Nov 2020 00:49:03 +0100 Tobias Waldekranz wrote:
> > I'm unclear what this fixes. What functionality is broken on 6097?  
> 
> VLAN configuration. As soon as you add the second port to a VLAN, all
> other port membership configuration is overwritten with zeroes. The HW
> interprets this as all ports being "unmodified members" of the VLAN.
> 
> I suspect that is why it has not been discovered. In the simple case
> when all ports belong to the same VLAN, switching will still work. But
> using multiple VLANs or trying to set multiple ports as tagged members
> will not work.

I see, this info would be good to include in the commit message.
User impact is what backporters care about the most.

> At the lowest level, the current implementation assumes that it can
> perform two consecutive operations where each op will load half of a
> register, and then read out the union of the information. This is true
> for some devices (6352), but not for others (6097).
> 
> 6352 pseudo-hdl-in-c:
> 
> stu_get_next()
> {
> 	*data |= stu_data & 0xf0f0;
> }
> 
> vtu_get_next()
> {
> 	*data |= vtu_data & 0x0f0f;
> }
> 
> 6097 pseudo-hdl-in-c:
> 
> stu_get_next()
> {
> 	*data = stu_data;
> }
> 
> vtu_get_next()
> {
> 	*data = vtu_data;
> }
> 
> > Can we identify the commit for a fixes tag?  
> 
> I will try to pinpoint it tomorrow. I suppose I should also rebase it
> against "net" since it is a bug.

Indeed, thanks :)