[v4,net-next,02/11] net: dsa: mv88e6xxx: deny vid 0 on the CPU port and DSA links too

Commit Message

Vladimir Oltean Jan. 9, 2021, 12:01 a.m. UTC

From: Vladimir Oltean <[e-mail]>

mv88e6xxx apparently has a problem offloading VID 0, which the 8021q
module tries to install as part of commit ad1afb003939 ("vlan_dev: VLAN
0 should be treated as "no vlan tag" (802.1p packet)"). That mv88e6xxx
restriction seems to have been introduced by the "VTU GetNext VID-1
trick to retrieve a single entry" - see commit 2fb5ef09de7c ("net: dsa:
mv88e6xxx: extract single VLAN retrieval").

There is one more problem. The mv88e6xxx CPU port and DSA links do not
report properly in the prepare phase what are the VLANs that they can
offload. They'll say they can offload everything:

mv88e6xxx_port_vlan_prepare
-> mv88e6xxx_port_check_hw_vlan:

	/* DSA and CPU ports have to be members of multiple vlans */
	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
		return 0;

Except that if you actually try to commit to it, they'll error out and
print this message:

[   32.802438] mv88e6085 d0032004.mdio-mii:12: p9: failed to add VLAN 0t

which comes from:

mv88e6xxx_port_vlan_add
-> mv88e6xxx_port_vlan_join:

	if (!vid)
		return -EOPNOTSUPP;

What prevents this condition from triggering in real life? The fact that
when a DSA_NOTIFIER_VLAN_ADD is emitted, it never targets a DSA link
directly. Instead, the notifier will always target either a user port or
a CPU port. DSA links just happen to get dragged in by:

static bool dsa_switch_vlan_match(struct dsa_switch *ds, int port,
				  struct dsa_notifier_vlan_info *info)
{
	...
	if (dsa_is_dsa_port(ds, port))
		return true;
	...
}

So for every DSA VLAN notifier, during the prepare phase, it will just
so happen that there will be somebody to say "no, don't do that".

This will become a problem when the switchdev prepare/commit transactional
model goes away. Every port needs to think on its own. DSA links can no
longer bluff and rely on the fact that the prepare phase will not go
through to the end, because there will be no prepare phase any longer.

Fix this issue before it becomes a problem, by having the "vid == 0"
check earlier than the check whether we are a CPU port / DSA link or not.
Also, the "vid == 0" check becomes unnecessary in the .port_vlan_add
callback, so we can remove it.

Signed-off-by: Vladimir Oltean <[e-mail]>
---
Changes in v4:
retouched the commit message a little bit.

Changes in v3:
Patch is new.

 drivers/net/dsa/mv88e6xxx/chip.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Comments

Florian Fainelli Jan. 9, 2021, 4:05 a.m. UTC | #1

On 1/8/2021 4:01 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <[e-mail]>
> 
> mv88e6xxx apparently has a problem offloading VID 0, which the 8021q
> module tries to install as part of commit ad1afb003939 ("vlan_dev: VLAN
> 0 should be treated as "no vlan tag" (802.1p packet)"). That mv88e6xxx
> restriction seems to have been introduced by the "VTU GetNext VID-1
> trick to retrieve a single entry" - see commit 2fb5ef09de7c ("net: dsa:
> mv88e6xxx: extract single VLAN retrieval").
> 
> There is one more problem. The mv88e6xxx CPU port and DSA links do not
> report properly in the prepare phase what are the VLANs that they can
> offload. They'll say they can offload everything:
> 
> mv88e6xxx_port_vlan_prepare
> -> mv88e6xxx_port_check_hw_vlan:
> 
> 	/* DSA and CPU ports have to be members of multiple vlans */
> 	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
> 		return 0;
> 
> Except that if you actually try to commit to it, they'll error out and
> print this message:
> 
> [   32.802438] mv88e6085 d0032004.mdio-mii:12: p9: failed to add VLAN 0t
> 
> which comes from:
> 
> mv88e6xxx_port_vlan_add
> -> mv88e6xxx_port_vlan_join:
> 
> 	if (!vid)
> 		return -EOPNOTSUPP;
> 
> What prevents this condition from triggering in real life? The fact that
> when a DSA_NOTIFIER_VLAN_ADD is emitted, it never targets a DSA link
> directly. Instead, the notifier will always target either a user port or
> a CPU port. DSA links just happen to get dragged in by:
> 
> static bool dsa_switch_vlan_match(struct dsa_switch *ds, int port,
> 				  struct dsa_notifier_vlan_info *info)
> {
> 	...
> 	if (dsa_is_dsa_port(ds, port))
> 		return true;
> 	...
> }
> 
> So for every DSA VLAN notifier, during the prepare phase, it will just
> so happen that there will be somebody to say "no, don't do that".
> 
> This will become a problem when the switchdev prepare/commit transactional
> model goes away. Every port needs to think on its own. DSA links can no
> longer bluff and rely on the fact that the prepare phase will not go
> through to the end, because there will be no prepare phase any longer.
> 
> Fix this issue before it becomes a problem, by having the "vid == 0"
> check earlier than the check whether we are a CPU port / DSA link or not.
> Also, the "vid == 0" check becomes unnecessary in the .port_vlan_add
> callback, so we can remove it.
> 
> Signed-off-by: Vladimir Oltean <[e-mail]>

Reviewed-by: Florian Fainelli <[e-mail]>

Patch

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 4834be9e4e86..fb25cb87156a 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1535,13 +1535,13 @@  static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
 	struct mv88e6xxx_vtu_entry vlan;
 	int i, err;
 
+	if (!vid)
+		return -EOPNOTSUPP;
+
 	/* DSA and CPU ports have to be members of multiple vlans */
 	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
 		return 0;
 
-	if (!vid)
-		return -EOPNOTSUPP;
-
 	vlan.vid = vid - 1;
 	vlan.valid = false;
 
@@ -1920,9 +1920,6 @@  static int mv88e6xxx_port_vlan_join(struct mv88e6xxx_chip *chip, int port,
 	struct mv88e6xxx_vtu_entry vlan;
 	int i, err;
 
-	if (!vid)
-		return -EOPNOTSUPP;
-
 	vlan.vid = vid - 1;
 	vlan.valid = false;