From 75e5a554c87fd48f67d1674cfd34e47e3b454fb3 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Sat, 31 Oct 2020 12:29:10 +0200
Subject: [PATCH 1/7] net: mscc: ocelot: use the pvid of zero when bridged with
vlan_filtering=0
Currently, mscc_ocelot ports configure pvid=0 in standalone mode, and
inherit the pvid from the bridge when one is present.
When the bridge has vlan_filtering=0, the software semantics are that
packets should be received regardless of whether there's a pvid
configured on the ingress port or not. However, ocelot does not observe
those semantics today.
Moreover, changing the PVID is also a problem with vlan_filtering=0.
We are privately remapping the VID of FDB, MDB entries to the port's
PVID when those are VLAN-unaware (i.e. when the VID of these entries
comes to us as 0). But we have no logic of adjusting that remapping when
the user changes the pvid and vlan_filtering is 0. So stale entries
would be left behind, and untagged traffic will stop matching on them.
And even if we were to solve that, there's an even bigger problem. If
swp0 has pvid 1, and swp1 has pvid 2, and both are under a vlan_filtering=0
bridge, they should be able to forward traffic between one another.
However, with ocelot they wouldn't do that.
The simplest way of fixing this is to never configure the pvid based on
what the bridge is asking for, when vlan_filtering is 0. Only if there
was a VLAN that the bridge couldn't mangle, that we could use as pvid....
So, turns out, there's 0 just for that. And for a reason: IEEE
802.1Q-2018, page 247, Table 9-2-Reserved VID values says:
The null VID. Indicates that the tag header contains only
priority information; no VID is present in the frame.
This VID value shall not be configured as a PVID or a member
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
of a VID Set, or configured in any FDB entry, or used in any
Management operation.
So, aren't we doing exactly what 802.1Q says not to? Well, in a way, but
what we're doing here is just driver-level bookkeeping, all for the
better. The fact that we're using a pvid of 0 is not observable behavior
from the outside world: the network stack does not see the classified
VLAN that the switch uses, in vlan_filtering=0 mode. And we're also more
consistent with the standalone mode now.
And now that we use the pvid of 0 in this mode, there's another advantage:
we don't need to perform any VID remapping for FDB and MDB entries either,
we can just use the VID of 0 that the bridge is passing to us.
The only gotcha is that every time we change the vlan_filtering setting,
we need to reapply the pvid (either to 0, or to the value from the bridge).
A small side-effect visible in the patch is that ocelot_port_set_pvid
needs to be moved above ocelot_port_vlan_filtering, so that it can be
called from there without forward-declarations.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
drivers/net/ethernet/mscc/ocelot.c | 53 ++++++++++--------------------
1 file changed, 17 insertions(+), 36 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 323dbd30661a..bc5b15d7bce7 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -199,6 +199,22 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
return 0;
}
+/* Default vlan to clasify for untagged frames (may be zero) */
+static void ocelot_port_set_pvid(struct ocelot *ocelot, int port, u16 pvid)
+{
+ struct ocelot_port *ocelot_port = ocelot->ports[port];
+
+ ocelot_port->pvid = pvid;
+
+ if (!ocelot_port->vlan_aware)
+ pvid = 0;
+
+ ocelot_rmw_gix(ocelot,
+ ANA_PORT_VLAN_CFG_VLAN_VID(pvid),
+ ANA_PORT_VLAN_CFG_VLAN_VID_M,
+ ANA_PORT_VLAN_CFG, port);
+}
+
int ocelot_port_vlan_filtering(struct ocelot *ocelot, int port,
bool vlan_aware, struct switchdev_trans *trans)
{
@@ -233,25 +249,13 @@ int ocelot_port_vlan_filtering(struct ocelot *ocelot, int port,
ANA_PORT_VLAN_CFG_VLAN_POP_CNT_M,
ANA_PORT_VLAN_CFG, port);
+ ocelot_port_set_pvid(ocelot, port, ocelot_port->pvid);
ocelot_port_set_native_vlan(ocelot, port, ocelot_port->vid);
return 0;
}
EXPORT_SYMBOL(ocelot_port_vlan_filtering);
-/* Default vlan to clasify for untagged frames (may be zero) */
-static void ocelot_port_set_pvid(struct ocelot *ocelot, int port, u16 pvid)
-{
- struct ocelot_port *ocelot_port = ocelot->ports[port];
-
- ocelot_rmw_gix(ocelot,
- ANA_PORT_VLAN_CFG_VLAN_VID(pvid),
- ANA_PORT_VLAN_CFG_VLAN_VID_M,
- ANA_PORT_VLAN_CFG, port);
-
- ocelot_port->pvid = pvid;
-}
-
int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
bool untagged)
{
@@ -542,26 +546,11 @@ EXPORT_SYMBOL(ocelot_get_txtstamp);
int ocelot_fdb_add(struct ocelot *ocelot, int port,
const unsigned char *addr, u16 vid)
{
- struct ocelot_port *ocelot_port = ocelot->ports[port];
int pgid = port;
if (port == ocelot->npi)
pgid = PGID_CPU;
- if (!vid) {
- if (!ocelot_port->vlan_aware)
- /* If the bridge is not VLAN aware and no VID was
- * provided, set it to pvid to ensure the MAC entry
- * matches incoming untagged packets
- */
- vid = ocelot_port->pvid;
- else
- /* If the bridge is VLAN aware a VID must be provided as
- * otherwise the learnt entry wouldn't match any frame.
- */
- return -EINVAL;
- }
-
return ocelot_mact_learn(ocelot, pgid, addr, vid, ENTRYTYPE_LOCKED);
}
EXPORT_SYMBOL(ocelot_fdb_add);
@@ -1048,7 +1037,6 @@ static void ocelot_encode_ports_to_mdb(unsigned char *addr,
int ocelot_port_mdb_add(struct ocelot *ocelot, int port,
const struct switchdev_obj_port_mdb *mdb)
{
- struct ocelot_port *ocelot_port = ocelot->ports[port];
unsigned char addr[ETH_ALEN];
struct ocelot_multicast *mc;
struct ocelot_pgid *pgid;
@@ -1057,9 +1045,6 @@ int ocelot_port_mdb_add(struct ocelot *ocelot, int port,
if (port == ocelot->npi)
port = ocelot->num_phys_ports;
- if (!vid)
- vid = ocelot_port->pvid;
-
mc = ocelot_multicast_get(ocelot, mdb->addr, vid);
if (!mc) {
/* New entry */
@@ -1108,7 +1093,6 @@ EXPORT_SYMBOL(ocelot_port_mdb_add);
int ocelot_port_mdb_del(struct ocelot *ocelot, int port,
const struct switchdev_obj_port_mdb *mdb)
{
- struct ocelot_port *ocelot_port = ocelot->ports[port];
unsigned char addr[ETH_ALEN];
struct ocelot_multicast *mc;
struct ocelot_pgid *pgid;
@@ -1117,9 +1101,6 @@ int ocelot_port_mdb_del(struct ocelot *ocelot, int port,
if (port == ocelot->npi)
port = ocelot->num_phys_ports;
- if (!vid)
- vid = ocelot_port->pvid;
-
mc = ocelot_multicast_get(ocelot, mdb->addr, vid);
if (!mc)
return -ENOENT;
From 110e847ca7d5e712cabc8cb866a66b629832f4a2 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Sat, 31 Oct 2020 12:29:11 +0200
Subject: [PATCH 2/7] net: mscc: ocelot: don't reset the pvid to 0 when
deleting it
I have no idea why this code is here, but I have 2 hypotheses:
1.
A desperate attempt to keep untagged traffic working when the bridge
deletes the pvid on a port.
There was a fairly okay discussion here:
https://lore.kernel.org/netdev/CA+h21hrRMrLH-RjBGhEJSTZd6_QPRSd3RkVRQF-wNKkrgKcRSA@mail.gmail.com/#t
which established that in vlan_filtering=1 mode, the absence of a pvid
should denote that the ingress port should drop untagged and priority
tagged traffic. While in vlan_filtering=0 mode, nothing should change.
So in vlan_filtering=1 mode, we should simply let things happen, and not
attempt to save the day. And in vlan_filtering=0 mode, the pvid is 0
anyway, no need to do anything.
2.
The driver encodes the native VLAN (ocelot_port->vid) value of 0 as
special, meaning "not valid". There are checks based on that. But there
are no such checks for the ocelot_port->pvid value of 0. In fact, that's
a perfectly valid value, which is used in standalone mode. Maybe there
was some confusion and the author thought that 0 means "invalid" here as
well.
In conclusion, delete the code*.
*in fact we'll add it back later, in a slightly different form, but for
an entirely different reason than the one for which this exists now.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
drivers/net/ethernet/mscc/ocelot.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index bc5b15d7bce7..ae25a79bf907 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -293,10 +293,6 @@ int ocelot_vlan_del(struct ocelot *ocelot, int port, u16 vid)
if (ret)
return ret;
- /* Ingress */
- if (ocelot_port->pvid == vid)
- ocelot_port_set_pvid(ocelot, port, 0);
-
/* Egress */
if (ocelot_port->vid == vid)
ocelot_port_set_native_vlan(ocelot, port, 0);
From c3e58a750e3d64ea51df1e39911098a46dd0d9a6 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Sat, 31 Oct 2020 12:29:12 +0200
Subject: [PATCH 3/7] net: mscc: ocelot: transform the pvid and native vlan
values into a structure
This is a mechanical patch only.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
drivers/net/ethernet/mscc/ocelot.c | 55 ++++++++++++++++----------
drivers/net/ethernet/mscc/ocelot_net.c | 16 ++++----
include/soc/mscc/ocelot.h | 14 ++++---
3 files changed, 50 insertions(+), 35 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index ae25a79bf907..a7e724ae01f7 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -148,27 +148,27 @@ static int ocelot_vlant_set_mask(struct ocelot *ocelot, u16 vid, u32 mask)
}
static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
- u16 vid)
+ struct ocelot_vlan native_vlan)
{
struct ocelot_port *ocelot_port = ocelot->ports[port];
u32 val = 0;
- if (ocelot_port->vid != vid) {
+ if (ocelot_port->native_vlan.vid != native_vlan.vid) {
/* Always permit deleting the native VLAN (vid = 0) */
- if (ocelot_port->vid && vid) {
+ if (ocelot_port->native_vlan.vid && native_vlan.vid) {
dev_err(ocelot->dev,
"Port already has a native VLAN: %d\n",
- ocelot_port->vid);
+ ocelot_port->native_vlan.vid);
return -EBUSY;
}
- ocelot_port->vid = vid;
+ ocelot_port->native_vlan = native_vlan;
}
- ocelot_rmw_gix(ocelot, REW_PORT_VLAN_CFG_PORT_VID(vid),
+ ocelot_rmw_gix(ocelot, REW_PORT_VLAN_CFG_PORT_VID(native_vlan.vid),
REW_PORT_VLAN_CFG_PORT_VID_M,
REW_PORT_VLAN_CFG, port);
- if (ocelot_port->vlan_aware && !ocelot_port->vid)
+ if (ocelot_port->vlan_aware && !ocelot_port->native_vlan.vid)
/* If port is vlan-aware and tagged, drop untagged and priority
* tagged frames.
*/
@@ -182,7 +182,7 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
ANA_PORT_DROP_CFG, port);
if (ocelot_port->vlan_aware) {
- if (ocelot_port->vid)
+ if (ocelot_port->native_vlan.vid)
/* Tag all frames except when VID == DEFAULT_VLAN */
val = REW_TAG_CFG_TAG_CFG(1);
else
@@ -200,17 +200,18 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
}
/* Default vlan to clasify for untagged frames (may be zero) */
-static void ocelot_port_set_pvid(struct ocelot *ocelot, int port, u16 pvid)
+static void ocelot_port_set_pvid(struct ocelot *ocelot, int port,
+ struct ocelot_vlan pvid_vlan)
{
struct ocelot_port *ocelot_port = ocelot->ports[port];
- ocelot_port->pvid = pvid;
+ ocelot_port->pvid_vlan = pvid_vlan;
if (!ocelot_port->vlan_aware)
- pvid = 0;
+ pvid_vlan.vid = 0;
ocelot_rmw_gix(ocelot,
- ANA_PORT_VLAN_CFG_VLAN_VID(pvid),
+ ANA_PORT_VLAN_CFG_VLAN_VID(pvid_vlan.vid),
ANA_PORT_VLAN_CFG_VLAN_VID_M,
ANA_PORT_VLAN_CFG, port);
}
@@ -249,8 +250,8 @@ int ocelot_port_vlan_filtering(struct ocelot *ocelot, int port,
ANA_PORT_VLAN_CFG_VLAN_POP_CNT_M,
ANA_PORT_VLAN_CFG, port);
- ocelot_port_set_pvid(ocelot, port, ocelot_port->pvid);
- ocelot_port_set_native_vlan(ocelot, port, ocelot_port->vid);
+ ocelot_port_set_pvid(ocelot, port, ocelot_port->pvid_vlan);
+ ocelot_port_set_native_vlan(ocelot, port, ocelot_port->native_vlan);
return 0;
}
@@ -268,12 +269,19 @@ int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
return ret;
/* Default ingress vlan classification */
- if (pvid)
- ocelot_port_set_pvid(ocelot, port, vid);
+ if (pvid) {
+ struct ocelot_vlan pvid_vlan;
+
+ pvid_vlan.vid = vid;
+ ocelot_port_set_pvid(ocelot, port, pvid_vlan);
+ }
/* Untagged egress vlan clasification */
if (untagged) {
- ret = ocelot_port_set_native_vlan(ocelot, port, vid);
+ struct ocelot_vlan native_vlan;
+
+ native_vlan.vid = vid;
+ ret = ocelot_port_set_native_vlan(ocelot, port, native_vlan);
if (ret)
return ret;
}
@@ -294,8 +302,12 @@ int ocelot_vlan_del(struct ocelot *ocelot, int port, u16 vid)
return ret;
/* Egress */
- if (ocelot_port->vid == vid)
- ocelot_port_set_native_vlan(ocelot, port, 0);
+ if (ocelot_port->native_vlan.vid == vid) {
+ struct ocelot_vlan native_vlan;
+
+ native_vlan.vid = 0;
+ ocelot_port_set_native_vlan(ocelot, port, native_vlan);
+ }
return 0;
}
@@ -1151,6 +1163,7 @@ EXPORT_SYMBOL(ocelot_port_bridge_join);
int ocelot_port_bridge_leave(struct ocelot *ocelot, int port,
struct net_device *bridge)
{
+ struct ocelot_vlan pvid = {0}, native_vlan = {0};
struct switchdev_trans trans;
int ret;
@@ -1169,8 +1182,8 @@ int ocelot_port_bridge_leave(struct ocelot *ocelot, int port,
if (ret)
return ret;
- ocelot_port_set_pvid(ocelot, port, 0);
- return ocelot_port_set_native_vlan(ocelot, port, 0);
+ ocelot_port_set_pvid(ocelot, port, pvid);
+ return ocelot_port_set_native_vlan(ocelot, port, native_vlan);
}
EXPORT_SYMBOL(ocelot_port_bridge_leave);
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index b34da11acf65..cf5c2a0ddfc0 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -409,7 +409,7 @@ static int ocelot_mc_unsync(struct net_device *dev, const unsigned char *addr)
struct ocelot_port *ocelot_port = &priv->port;
struct ocelot *ocelot = ocelot_port->ocelot;
- return ocelot_mact_forget(ocelot, addr, ocelot_port->pvid);
+ return ocelot_mact_forget(ocelot, addr, ocelot_port->pvid_vlan.vid);
}
static int ocelot_mc_sync(struct net_device *dev, const unsigned char *addr)
@@ -418,8 +418,8 @@ static int ocelot_mc_sync(struct net_device *dev, const unsigned char *addr)
struct ocelot_port *ocelot_port = &priv->port;
struct ocelot *ocelot = ocelot_port->ocelot;
- return ocelot_mact_learn(ocelot, PGID_CPU, addr, ocelot_port->pvid,
- ENTRYTYPE_LOCKED);
+ return ocelot_mact_learn(ocelot, PGID_CPU, addr,
+ ocelot_port->pvid_vlan.vid, ENTRYTYPE_LOCKED);
}
static void ocelot_set_rx_mode(struct net_device *dev)
@@ -462,10 +462,10 @@ static int ocelot_port_set_mac_address(struct net_device *dev, void *p)
const struct sockaddr *addr = p;
/* Learn the new net device MAC address in the mac table. */
- ocelot_mact_learn(ocelot, PGID_CPU, addr->sa_data, ocelot_port->pvid,
- ENTRYTYPE_LOCKED);
+ ocelot_mact_learn(ocelot, PGID_CPU, addr->sa_data,
+ ocelot_port->pvid_vlan.vid, ENTRYTYPE_LOCKED);
/* Then forget the previous one. */
- ocelot_mact_forget(ocelot, dev->dev_addr, ocelot_port->pvid);
+ ocelot_mact_forget(ocelot, dev->dev_addr, ocelot_port->pvid_vlan.vid);
ether_addr_copy(dev->dev_addr, addr->sa_data);
return 0;
@@ -1074,8 +1074,8 @@ int ocelot_probe_port(struct ocelot *ocelot, int port, struct regmap *target,
memcpy(dev->dev_addr, ocelot->base_mac, ETH_ALEN);
dev->dev_addr[ETH_ALEN - 1] += port;
- ocelot_mact_learn(ocelot, PGID_CPU, dev->dev_addr, ocelot_port->pvid,
- ENTRYTYPE_LOCKED);
+ ocelot_mact_learn(ocelot, PGID_CPU, dev->dev_addr,
+ ocelot_port->pvid_vlan.vid, ENTRYTYPE_LOCKED);
ocelot_init_port(ocelot, port);
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index cc126d1796be..baf6a498f7d1 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -571,18 +571,20 @@ struct ocelot_vcap_block {
int pol_lpr;
};
+struct ocelot_vlan {
+ u16 vid;
+};
+
struct ocelot_port {
struct ocelot *ocelot;
struct regmap *target;
bool vlan_aware;
-
- /* Ingress default VLAN (pvid) */
- u16 pvid;
-
- /* Egress default VLAN (vid) */
- u16 vid;
+ /* VLAN that untagged frames are classified to, on ingress */
+ struct ocelot_vlan pvid_vlan;
+ /* The VLAN ID that will be transmitted as untagged, on egress */
+ struct ocelot_vlan native_vlan;
u8 ptp_cmd;
struct sk_buff_head tx_skbs;
From e2b2e83e52f756decbaacd8202f28745bab49e07 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Sat, 31 Oct 2020 12:29:13 +0200
Subject: [PATCH 4/7] net: mscc: ocelot: add a "valid" boolean to struct
ocelot_vlan
Currently we are checking in some places whether the port has a native
VLAN on egress or not, by comparing the ocelot_port->vid value with zero.
That works, because VID 0 can never be a native VLAN configured by the
bridge, but now we want to make similar checks for the pvid. That won't
work, because there are cases when we do have the pvid set to 0 (not by
the bridge, by ourselves, but still.. it's confusing). And we can't
encode a negative value into an u16, so add a bool to the structure.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
drivers/net/ethernet/mscc/ocelot.c | 27 ++++++++++++++-------------
include/soc/mscc/ocelot.h | 1 +
2 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index a7e724ae01f7..d49e34430e23 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -153,22 +153,22 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
struct ocelot_port *ocelot_port = ocelot->ports[port];
u32 val = 0;
- if (ocelot_port->native_vlan.vid != native_vlan.vid) {
- /* Always permit deleting the native VLAN (vid = 0) */
- if (ocelot_port->native_vlan.vid && native_vlan.vid) {
- dev_err(ocelot->dev,
- "Port already has a native VLAN: %d\n",
- ocelot_port->native_vlan.vid);
- return -EBUSY;
- }
- ocelot_port->native_vlan = native_vlan;
+ /* Deny changing the native VLAN, but always permit deleting it */
+ if (ocelot_port->native_vlan.vid != native_vlan.vid &&
+ ocelot_port->native_vlan.valid && native_vlan.valid) {
+ dev_err(ocelot->dev,
+ "Port already has a native VLAN: %d\n",
+ ocelot_port->native_vlan.vid);
+ return -EBUSY;
}
+ ocelot_port->native_vlan = native_vlan;
+
ocelot_rmw_gix(ocelot, REW_PORT_VLAN_CFG_PORT_VID(native_vlan.vid),
REW_PORT_VLAN_CFG_PORT_VID_M,
REW_PORT_VLAN_CFG, port);
- if (ocelot_port->vlan_aware && !ocelot_port->native_vlan.vid)
+ if (ocelot_port->vlan_aware && !ocelot_port->native_vlan.valid)
/* If port is vlan-aware and tagged, drop untagged and priority
* tagged frames.
*/
@@ -182,7 +182,7 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
ANA_PORT_DROP_CFG, port);
if (ocelot_port->vlan_aware) {
- if (ocelot_port->native_vlan.vid)
+ if (native_vlan.valid)
/* Tag all frames except when VID == DEFAULT_VLAN */
val = REW_TAG_CFG_TAG_CFG(1);
else
@@ -273,6 +273,7 @@ int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
struct ocelot_vlan pvid_vlan;
pvid_vlan.vid = vid;
+ pvid_vlan.valid = true;
ocelot_port_set_pvid(ocelot, port, pvid_vlan);
}
@@ -281,6 +282,7 @@ int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
struct ocelot_vlan native_vlan;
native_vlan.vid = vid;
+ native_vlan.valid = true;
ret = ocelot_port_set_native_vlan(ocelot, port, native_vlan);
if (ret)
return ret;
@@ -303,9 +305,8 @@ int ocelot_vlan_del(struct ocelot *ocelot, int port, u16 vid)
/* Egress */
if (ocelot_port->native_vlan.vid == vid) {
- struct ocelot_vlan native_vlan;
+ struct ocelot_vlan native_vlan = {0};
- native_vlan.vid = 0;
ocelot_port_set_native_vlan(ocelot, port, native_vlan);
}
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index baf6a498f7d1..67c2af1c4c5c 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -572,6 +572,7 @@ struct ocelot_vcap_block {
};
struct ocelot_vlan {
+ bool valid;
u16 vid;
};
From be0576fed6d37d620d3d7dc416fc72677fd0f422 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Sat, 31 Oct 2020 12:29:14 +0200
Subject: [PATCH 5/7] net: mscc: ocelot: move the logic to drop 802.1p traffic
to the pvid deletion
Currently, the ocelot_port_set_native_vlan() function starts dropping
untagged and prio-tagged traffic when the native VLAN is removed?
What is the native VLAN? It is the only egress-untagged VLAN that ocelot
supports on a port. If the port is a trunk with 100 VLANs, one of those
VLANs can be transmitted as egress-untagged, and that's the native VLAN.
Is it wrong to drop untagged and prio-tagged traffic if there's no
native VLAN? Yes and no.
In this case, which is more typical, it's ok to apply that drop
configuration:
$ bridge vlan add dev swp0 vid 1 pvid untagged <- this is the native VLAN
$ bridge vlan add dev swp0 vid 100
$ bridge vlan add dev swp0 vid 101
$ bridge vlan del dev swp0 vid 1 <- delete the native VLAN
But only because the pvid and the native VLAN have the same ID.
In this case, it isn't:
$ bridge vlan add dev swp0 vid 1 pvid
$ bridge vlan add dev swp0 vid 100 untagged <- this is the native VLAN
$ bridge vlan del dev swp0 vid 101
$ bridge vlan del dev swp0 vid 100 <- delete the native VLAN
It's wrong, because the switch will drop untagged and prio-tagged
traffic now, despite having a valid pvid of 1.
The confusion seems to stem from the fact that the native VLAN is an
egress setting, while the PVID is an ingress setting. It would be
correct to drop untagged and prio-tagged traffic only if there was no
pvid on the port. So let's do just that.
Background:
https://lore.kernel.org/netdev/CA+h21hrRMrLH-RjBGhEJSTZd6_QPRSd3RkVRQF-wNKkrgKcRSA@mail.gmail.com/#t
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
drivers/net/ethernet/mscc/ocelot.c | 35 +++++++++++++++++++-----------
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index d49e34430e23..60186fc99280 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -168,19 +168,6 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
REW_PORT_VLAN_CFG_PORT_VID_M,
REW_PORT_VLAN_CFG, port);
- if (ocelot_port->vlan_aware && !ocelot_port->native_vlan.valid)
- /* If port is vlan-aware and tagged, drop untagged and priority
- * tagged frames.
- */
- val = ANA_PORT_DROP_CFG_DROP_UNTAGGED_ENA |
- ANA_PORT_DROP_CFG_DROP_PRIO_S_TAGGED_ENA |
- ANA_PORT_DROP_CFG_DROP_PRIO_C_TAGGED_ENA;
- ocelot_rmw_gix(ocelot, val,
- ANA_PORT_DROP_CFG_DROP_UNTAGGED_ENA |
- ANA_PORT_DROP_CFG_DROP_PRIO_S_TAGGED_ENA |
- ANA_PORT_DROP_CFG_DROP_PRIO_C_TAGGED_ENA,
- ANA_PORT_DROP_CFG, port);
-
if (ocelot_port->vlan_aware) {
if (native_vlan.valid)
/* Tag all frames except when VID == DEFAULT_VLAN */
@@ -204,6 +191,7 @@ static void ocelot_port_set_pvid(struct ocelot *ocelot, int port,
struct ocelot_vlan pvid_vlan)
{
struct ocelot_port *ocelot_port = ocelot->ports[port];
+ u32 val = 0;
ocelot_port->pvid_vlan = pvid_vlan;
@@ -214,6 +202,20 @@ static void ocelot_port_set_pvid(struct ocelot *ocelot, int port,
ANA_PORT_VLAN_CFG_VLAN_VID(pvid_vlan.vid),
ANA_PORT_VLAN_CFG_VLAN_VID_M,
ANA_PORT_VLAN_CFG, port);
+
+ /* If there's no pvid, we should drop not only untagged traffic (which
+ * happens automatically), but also 802.1p traffic which gets
+ * classified to VLAN 0, but that is always in our RX filter, so it
+ * would get accepted were it not for this setting.
+ */
+ if (!pvid_vlan.valid && ocelot_port->vlan_aware)
+ val = ANA_PORT_DROP_CFG_DROP_PRIO_S_TAGGED_ENA |
+ ANA_PORT_DROP_CFG_DROP_PRIO_C_TAGGED_ENA;
+
+ ocelot_rmw_gix(ocelot, val,
+ ANA_PORT_DROP_CFG_DROP_PRIO_S_TAGGED_ENA |
+ ANA_PORT_DROP_CFG_DROP_PRIO_C_TAGGED_ENA,
+ ANA_PORT_DROP_CFG, port);
}
int ocelot_port_vlan_filtering(struct ocelot *ocelot, int port,
@@ -303,6 +305,13 @@ int ocelot_vlan_del(struct ocelot *ocelot, int port, u16 vid)
if (ret)
return ret;
+ /* Ingress */
+ if (ocelot_port->pvid_vlan.vid == vid) {
+ struct ocelot_vlan pvid_vlan = {0};
+
+ ocelot_port_set_pvid(ocelot, port, pvid_vlan);
+ }
+
/* Egress */
if (ocelot_port->native_vlan.vid == vid) {
struct ocelot_vlan native_vlan = {0};
From 2f0402fedf20cc97b90837f6a9e2f5dc670afd4d Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Sat, 31 Oct 2020 12:29:15 +0200
Subject: [PATCH 6/7] net: mscc: ocelot: deny changing the native VLAN from the
prepare phase
Put the preparation phase of switchdev VLAN objects to some good use,
and move the check we already had, for preventing the existence of more
than one egress-untagged VLAN per port, to the preparation phase of the
addition.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
drivers/net/dsa/ocelot/felix.c | 13 +++++++-
drivers/net/ethernet/mscc/ocelot.c | 41 +++++++++++++++-----------
drivers/net/ethernet/mscc/ocelot_net.c | 22 ++++++++++++--
include/soc/mscc/ocelot.h | 2 ++
4 files changed, 57 insertions(+), 21 deletions(-)
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index f791860d495f..3848f6bc922b 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -112,10 +112,21 @@ static void felix_bridge_leave(struct dsa_switch *ds, int port,
ocelot_port_bridge_leave(ocelot, port, br);
}
-/* This callback needs to be present */
static int felix_vlan_prepare(struct dsa_switch *ds, int port,
const struct switchdev_obj_port_vlan *vlan)
{
+ struct ocelot *ocelot = ds->priv;
+ u16 vid, flags = vlan->flags;
+ int err;
+
+ for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
+ err = ocelot_vlan_prepare(ocelot, port, vid,
+ flags & BRIDGE_VLAN_INFO_PVID,
+ flags & BRIDGE_VLAN_INFO_UNTAGGED);
+ if (err)
+ return err;
+ }
+
return 0;
}
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 60186fc99280..2632fe2d2448 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -147,21 +147,12 @@ static int ocelot_vlant_set_mask(struct ocelot *ocelot, u16 vid, u32 mask)
return ocelot_vlant_wait_for_completion(ocelot);
}
-static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
- struct ocelot_vlan native_vlan)
+static void ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
+ struct ocelot_vlan native_vlan)
{
struct ocelot_port *ocelot_port = ocelot->ports[port];
u32 val = 0;
- /* Deny changing the native VLAN, but always permit deleting it */
- if (ocelot_port->native_vlan.vid != native_vlan.vid &&
- ocelot_port->native_vlan.valid && native_vlan.valid) {
- dev_err(ocelot->dev,
- "Port already has a native VLAN: %d\n",
- ocelot_port->native_vlan.vid);
- return -EBUSY;
- }
-
ocelot_port->native_vlan = native_vlan;
ocelot_rmw_gix(ocelot, REW_PORT_VLAN_CFG_PORT_VID(native_vlan.vid),
@@ -182,8 +173,6 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
ocelot_rmw_gix(ocelot, val,
REW_TAG_CFG_TAG_CFG_M,
REW_TAG_CFG, port);
-
- return 0;
}
/* Default vlan to clasify for untagged frames (may be zero) */
@@ -259,6 +248,24 @@ int ocelot_port_vlan_filtering(struct ocelot *ocelot, int port,
}
EXPORT_SYMBOL(ocelot_port_vlan_filtering);
+int ocelot_vlan_prepare(struct ocelot *ocelot, int port, u16 vid, bool pvid,
+ bool untagged)
+{
+ struct ocelot_port *ocelot_port = ocelot->ports[port];
+
+ /* Deny changing the native VLAN, but always permit deleting it */
+ if (untagged && ocelot_port->native_vlan.vid != vid &&
+ ocelot_port->native_vlan.valid) {
+ dev_err(ocelot->dev,
+ "Port already has a native VLAN: %d\n",
+ ocelot_port->native_vlan.vid);
+ return -EBUSY;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(ocelot_vlan_prepare);
+
int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
bool untagged)
{
@@ -285,9 +292,7 @@ int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
native_vlan.vid = vid;
native_vlan.valid = true;
- ret = ocelot_port_set_native_vlan(ocelot, port, native_vlan);
- if (ret)
- return ret;
+ ocelot_port_set_native_vlan(ocelot, port, native_vlan);
}
return 0;
@@ -1193,7 +1198,9 @@ int ocelot_port_bridge_leave(struct ocelot *ocelot, int port,
return ret;
ocelot_port_set_pvid(ocelot, port, pvid);
- return ocelot_port_set_native_vlan(ocelot, port, native_vlan);
+ ocelot_port_set_native_vlan(ocelot, port, native_vlan);
+
+ return 0;
}
EXPORT_SYMBOL(ocelot_port_bridge_leave);
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index cf5c2a0ddfc0..c65ae6f75a16 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -206,6 +206,17 @@ static void ocelot_port_adjust_link(struct net_device *dev)
ocelot_adjust_link(ocelot, port, dev->phydev);
}
+static int ocelot_vlan_vid_prepare(struct net_device *dev, u16 vid, bool pvid,
+ bool untagged)
+{
+ struct ocelot_port_private *priv = netdev_priv(dev);
+ struct ocelot_port *ocelot_port = &priv->port;
+ struct ocelot *ocelot = ocelot_port->ocelot;
+ int port = priv->chip_port;
+
+ return ocelot_vlan_prepare(ocelot, port, vid, pvid, untagged);
+}
+
static int ocelot_vlan_vid_add(struct net_device *dev, u16 vid, bool pvid,
bool untagged)
{
@@ -812,9 +823,14 @@ static int ocelot_port_obj_add_vlan(struct net_device *dev,
u16 vid;
for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
- ret = ocelot_vlan_vid_add(dev, vid,
- vlan->flags & BRIDGE_VLAN_INFO_PVID,
- vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED);
+ bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
+ bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
+
+ if (switchdev_trans_ph_prepare(trans))
+ ret = ocelot_vlan_vid_prepare(dev, vid, pvid,
+ untagged);
+ else
+ ret = ocelot_vlan_vid_add(dev, vid, pvid, untagged);
if (ret)
return ret;
}
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 67c2af1c4c5c..ea1de185f2e4 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -747,6 +747,8 @@ int ocelot_fdb_add(struct ocelot *ocelot, int port,
const unsigned char *addr, u16 vid);
int ocelot_fdb_del(struct ocelot *ocelot, int port,
const unsigned char *addr, u16 vid);
+int ocelot_vlan_prepare(struct ocelot *ocelot, int port, u16 vid, bool pvid,
+ bool untagged);
int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
bool untagged);
int ocelot_vlan_del(struct ocelot *ocelot, int port, u16 vid);
From 9a72068080f14a9f249507eb171df26c912757dc Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Sat, 31 Oct 2020 12:29:16 +0200
Subject: [PATCH 7/7] net: dsa: felix: improve the workaround for multiple
native VLANs on NPI port
After the good discussion with Florian from here:
https://lore.kernel.org/netdev/20200911000337.htwr366ng3nc3a7d@skbuf/
I realized that the VLAN settings on the NPI port (the hardware "CPU port",
in DSA parlance) don't actually make any difference, because that port
is hardcoded in hardware to use what mv88e6xxx would call "unmodified"
egress policy for VLANs.
So earlier patch 183be6f967fe ("net: dsa: felix: send VLANs on CPU port
as egress-tagged") was incorrect in the sense that it didn't actually
make the VLANs be sent on the NPI port as egress-tagged. It only made
ocelot_port_set_native_vlan shut up.
Now that we have moved the check from ocelot_port_set_native_vlan to
ocelot_vlan_prepare, we can simply shunt ocelot_vlan_prepare from DSA,
and avoid calling it. This is the correct way to deal with things,
because the NPI port configuration is DSA-specific, so the ocelot switch
library should not have the check for multiple native VLANs refined in
any way, it is correct the way it is.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
drivers/net/dsa/ocelot/felix.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 3848f6bc922b..ada75fa15861 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -119,6 +119,17 @@ static int felix_vlan_prepare(struct dsa_switch *ds, int port,
u16 vid, flags = vlan->flags;
int err;
+ /* Ocelot switches copy frames as-is to the CPU, so the flags:
+ * egress-untagged or not, pvid or not, make no difference. This
+ * behavior is already better than what DSA just tries to approximate
+ * when it installs the VLAN with the same flags on the CPU port.
+ * Just accept any configuration, and don't let ocelot deny installing
+ * multiple native VLANs on the NPI port, because the switch doesn't
+ * look at the port tag settings towards the NPI interface anyway.
+ */
+ if (port == ocelot->npi)
+ return 0;
+
for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
err = ocelot_vlan_prepare(ocelot, port, vid,
flags & BRIDGE_VLAN_INFO_PVID,
@@ -146,9 +157,6 @@ static void felix_vlan_add(struct dsa_switch *ds, int port,
u16 vid;
int err;
- if (dsa_is_cpu_port(ds, port))
- flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
-
for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
err = ocelot_vlan_add(ocelot, port, vid,
flags & BRIDGE_VLAN_INFO_PVID,