| Message ID | 20210109000156.1246735-3-olteanv@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 1/8/2021 4:01 PM, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > 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 <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
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;