* [PATCH v2 net 1/4] net: dsa: fix bridge_num not getting cleared after ports leaving the bridge
2021-10-05 0:14 [PATCH v2 net 0/4] DSA bridge TX forwarding offload fixes - part 1 Vladimir Oltean
@ 2021-10-05 0:14 ` Vladimir Oltean
2021-10-05 0:14 ` [PATCH v2 net 2/4] net: dsa: tag_dsa: send packets with TX fwd offload from VLAN-unaware bridges using VID 0 Vladimir Oltean
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Vladimir Oltean @ 2021-10-05 0:14 UTC (permalink / raw)
To: netdev, Jakub Kicinski, David S. Miller
Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, Tobias Waldekranz,
DENG Qingfang
The dp->bridge_num is zero-based, with -1 being the encoding for an
invalid value. But dsa_bridge_num_put used to check for an invalid value
by comparing bridge_num with 0, which is of course incorrect.
The result is that the bridge_num will never get cleared by
dsa_bridge_num_put, and further port joins to other bridges will get a
bridge_num larger than the previous one, and once all the available
bridges with TX forwarding offload supported by the hardware get
exhausted, the TX forwarding offload feature is simply disabled.
In the case of sja1105, 7 iterations of the loop below are enough to
exhaust the TX forwarding offload bits, and further bridge joins operate
without that feature.
ip link add br0 type bridge vlan_filtering 1
while :; do
ip link set sw0p2 master br0 && sleep 1
ip link set sw0p2 nomaster && sleep 1
done
This issue is enough of an indication that having the dp->bridge_num
invalid encoding be a negative number is prone to bugs, so this will be
changed to a one-based value, with the dp->bridge_num of zero being the
indication of no bridge. However, that is material for net-next.
Fixes: f5e165e72b29 ("net: dsa: track unique bridge numbers across all DSA switch trees")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none
net/dsa/dsa2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index b29262eee00b..6d5cc0217133 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -170,7 +170,7 @@ void dsa_bridge_num_put(const struct net_device *bridge_dev, int bridge_num)
/* Check if the bridge is still in use, otherwise it is time
* to clean it up so we can reuse this bridge_num later.
*/
- if (!dsa_bridge_num_find(bridge_dev))
+ if (dsa_bridge_num_find(bridge_dev) < 0)
clear_bit(bridge_num, &dsa_fwd_offloading_bridges);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 net 2/4] net: dsa: tag_dsa: send packets with TX fwd offload from VLAN-unaware bridges using VID 0
2021-10-05 0:14 [PATCH v2 net 0/4] DSA bridge TX forwarding offload fixes - part 1 Vladimir Oltean
2021-10-05 0:14 ` [PATCH v2 net 1/4] net: dsa: fix bridge_num not getting cleared after ports leaving the bridge Vladimir Oltean
@ 2021-10-05 0:14 ` Vladimir Oltean
2021-10-05 0:14 ` [PATCH v2 net 3/4] net: dsa: mv88e6xxx: keep the pvid at 0 when VLAN-unaware Vladimir Oltean
2021-10-05 0:14 ` [PATCH v2 net 4/4] net: dsa: mv88e6xxx: isolate the ATU databases of standalone and bridged ports Vladimir Oltean
3 siblings, 0 replies; 7+ messages in thread
From: Vladimir Oltean @ 2021-10-05 0:14 UTC (permalink / raw)
To: netdev, Jakub Kicinski, David S. Miller
Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, Tobias Waldekranz,
DENG Qingfang
The present code is structured this way due to an incomplete thought
process. In Documentation/networking/switchdev.rst we document that if a
bridge is VLAN-unaware, then the presence or lack of a pvid on a bridge
port (or on the bridge itself, for that matter) should not affect the
ability to receive and transmit tagged or untagged packets.
If the bridge on behalf of which we are sending this packet is
VLAN-aware, then the TX forwarding offload API ensures that the skb will
be VLAN-tagged (if the packet was sent by user space as untagged, it
will get transmitted town to the driver as tagged with the bridge
device's pvid). But if the bridge is VLAN-unaware, it may or may not be
VLAN-tagged. In fact the logic to insert the bridge's PVID came from the
idea that we should emulate what is being done in the VLAN-aware case.
But we shouldn't.
It appears that injecting packets using a VLAN ID of 0 serves the
purpose of forwarding the packets to the egress port with no VLAN tag
added or stripped by the hardware, and no filtering being performed.
So we can simply remove the superfluous logic.
One reason why this logic is broken is that when CONFIG_BRIDGE_VLAN_FILTERING=n,
we call br_vlan_get_pvid_rcu() but that returns an error and we do error
out, dropping all packets on xmit. Not really smart. This is also an
issue when the user deletes the bridge pvid:
$ bridge vlan del dev br0 vid 1 self
As mentioned, in both cases, packets should still flow freely, and they
do just that on any net device where the bridge is not offloaded, but on
mv88e6xxx they don't.
Fixes: d82f8ab0d874 ("net: dsa: tag_dsa: offload the bridge forwarding process")
Reported-by: Andrew Lunn <andrew@lunn.ch>
Link: https://patchwork.kernel.org/project/netdevbpf/patch/20211003155141.2241314-1-andrew@lunn.ch/
Link: https://patchwork.kernel.org/project/netdevbpf/patch/20210928233708.1246774-1-vladimir.oltean@nxp.com/
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: fix up commit message
net/dsa/tag_dsa.c | 20 ++------------------
1 file changed, 2 insertions(+), 18 deletions(-)
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index e5127b7d1c6a..68d5ddc3ef35 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -129,12 +129,9 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
u8 tag_dev, tag_port;
enum dsa_cmd cmd;
u8 *dsa_header;
- u16 pvid = 0;
- int err;
if (skb->offload_fwd_mark) {
struct dsa_switch_tree *dst = dp->ds->dst;
- struct net_device *br = dp->bridge_dev;
cmd = DSA_CMD_FORWARD;
@@ -144,19 +141,6 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
*/
tag_dev = dst->last_switch + 1 + dp->bridge_num;
tag_port = 0;
-
- /* If we are offloading forwarding for a VLAN-unaware bridge,
- * inject packets to hardware using the bridge's pvid, since
- * that's where the packets ingressed from.
- */
- if (!br_vlan_enabled(br)) {
- /* Safe because __dev_queue_xmit() runs under
- * rcu_read_lock_bh()
- */
- err = br_vlan_get_pvid_rcu(br, &pvid);
- if (err)
- return NULL;
- }
} else {
cmd = DSA_CMD_FROM_CPU;
tag_dev = dp->ds->index;
@@ -188,8 +172,8 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
dsa_header[0] = (cmd << 6) | tag_dev;
dsa_header[1] = tag_port << 3;
- dsa_header[2] = pvid >> 8;
- dsa_header[3] = pvid & 0xff;
+ dsa_header[2] = 0;
+ dsa_header[3] = 0;
}
return skb;
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 net 3/4] net: dsa: mv88e6xxx: keep the pvid at 0 when VLAN-unaware
2021-10-05 0:14 [PATCH v2 net 0/4] DSA bridge TX forwarding offload fixes - part 1 Vladimir Oltean
2021-10-05 0:14 ` [PATCH v2 net 1/4] net: dsa: fix bridge_num not getting cleared after ports leaving the bridge Vladimir Oltean
2021-10-05 0:14 ` [PATCH v2 net 2/4] net: dsa: tag_dsa: send packets with TX fwd offload from VLAN-unaware bridges using VID 0 Vladimir Oltean
@ 2021-10-05 0:14 ` Vladimir Oltean
2021-10-05 0:14 ` [PATCH v2 net 4/4] net: dsa: mv88e6xxx: isolate the ATU databases of standalone and bridged ports Vladimir Oltean
3 siblings, 0 replies; 7+ messages in thread
From: Vladimir Oltean @ 2021-10-05 0:14 UTC (permalink / raw)
To: netdev, Jakub Kicinski, David S. Miller
Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, Tobias Waldekranz,
DENG Qingfang
The VLAN support in mv88e6xxx has a loaded history. Commit 2ea7a679ca2a
("net: dsa: Don't add vlans when vlan filtering is disabled") noticed
some issues with VLAN and decided the best way to deal with them was to
make the DSA core ignore VLANs added by the bridge while VLAN awareness
is turned off. Those issues were never explained, just presented as
"at least one corner case".
That approach had problems of its own, presented by
commit 54a0ed0df496 ("net: dsa: provide an option for drivers to always
receive bridge VLANs") for the DSA core, followed by
commit 1fb74191988f ("net: dsa: mv88e6xxx: fix vlan setup") which
applied ds->configure_vlan_while_not_filtering = true for mv88e6xxx in
particular.
We still don't know what corner case Andrew saw when he wrote
commit 2ea7a679ca2a ("net: dsa: Don't add vlans when vlan filtering is
disabled"), but Tobias now reports that when we use TX forwarding
offload, pinging an external station from the bridge device is broken if
the front-facing DSA user port has flooding turned off. The full
description is in the link below, but for short, when a mv88e6xxx port
is under a VLAN-unaware bridge, it inherits that bridge's pvid.
So packets ingressing a user port will be classified to e.g. VID 1
(assuming that value for the bridge_default_pvid), whereas when
tag_dsa.c xmits towards a user port, it always sends packets using a VID
of 0 if that port is standalone or under a VLAN-unaware bridge - or at
least it did so prior to commit d82f8ab0d874 ("net: dsa: tag_dsa:
offload the bridge forwarding process").
In any case, when there is a conversation between the CPU and a station
connected to a user port, the station's MAC address is learned in VID 1
but the CPU tries to transmit through VID 0. The packets reach the
intended station, but via flooding and not by virtue of matching the
existing ATU entry.
DSA has established (and enforced in other drivers: sja1105, felix,
mt7530) that a VLAN-unaware port should use a private pvid, and not
inherit the one from the bridge. The bridge's pvid should only be
inherited when that bridge is VLAN-aware, so all state transitions need
to be handled. On the other hand, all bridge VLANs should sit in the VTU
starting with the moment when the bridge offloads them via switchdev,
they are just not used.
This solves the problem that Tobias sees because packets ingressing on
VLAN-unaware user ports now get classified to VID 0, which is also the
VID used by tag_dsa.c on xmit.
Fixes: d82f8ab0d874 ("net: dsa: tag_dsa: offload the bridge forwarding process")
Link: https://patchwork.kernel.org/project/netdevbpf/patch/20211003222312.284175-2-vladimir.oltean@nxp.com/#24491503
Reported-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new
drivers/net/dsa/mv88e6xxx/chip.c | 53 ++++++++++++++++++++++++++++----
drivers/net/dsa/mv88e6xxx/chip.h | 6 ++++
drivers/net/dsa/mv88e6xxx/port.c | 21 +++++++++++++
drivers/net/dsa/mv88e6xxx/port.h | 2 ++
4 files changed, 76 insertions(+), 6 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 03744d1c43fc..d672112afffd 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1677,6 +1677,26 @@ static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
return 0;
}
+static int mv88e6xxx_port_commit_pvid(struct mv88e6xxx_chip *chip, int port)
+{
+ struct dsa_port *dp = dsa_to_port(chip->ds, port);
+ struct mv88e6xxx_port *p = &chip->ports[port];
+ bool drop_untagged = false;
+ u16 pvid = 0;
+ int err;
+
+ if (dp->bridge_dev && br_vlan_enabled(dp->bridge_dev)) {
+ pvid = p->bridge_pvid.vid;
+ drop_untagged = !p->bridge_pvid.valid;
+ }
+
+ err = mv88e6xxx_port_set_pvid(chip, port, pvid);
+ if (err)
+ return err;
+
+ return mv88e6xxx_port_drop_untagged(chip, port, drop_untagged);
+}
+
static int mv88e6xxx_port_vlan_filtering(struct dsa_switch *ds, int port,
bool vlan_filtering,
struct netlink_ext_ack *extack)
@@ -1690,7 +1710,16 @@ static int mv88e6xxx_port_vlan_filtering(struct dsa_switch *ds, int port,
return -EOPNOTSUPP;
mv88e6xxx_reg_lock(chip);
+
err = mv88e6xxx_port_set_8021q_mode(chip, port, mode);
+ if (err)
+ goto unlock;
+
+ err = mv88e6xxx_port_commit_pvid(chip, port);
+ if (err)
+ goto unlock;
+
+unlock:
mv88e6xxx_reg_unlock(chip);
return err;
@@ -2123,6 +2152,7 @@ static int mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port,
struct mv88e6xxx_chip *chip = ds->priv;
bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
+ struct mv88e6xxx_port *p = &chip->ports[port];
bool warn;
u8 member;
int err;
@@ -2156,13 +2186,21 @@ static int mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port,
}
if (pvid) {
- err = mv88e6xxx_port_set_pvid(chip, port, vlan->vid);
- if (err) {
- dev_err(ds->dev, "p%d: failed to set PVID %d\n",
- port, vlan->vid);
+ p->bridge_pvid.vid = vlan->vid;
+ p->bridge_pvid.valid = true;
+
+ err = mv88e6xxx_port_commit_pvid(chip, port);
+ if (err)
+ goto out;
+ } else if (vlan->vid && p->bridge_pvid.vid == vlan->vid) {
+ /* The old pvid was reinstalled as a non-pvid VLAN */
+ p->bridge_pvid.valid = false;
+
+ err = mv88e6xxx_port_commit_pvid(chip, port);
+ if (err)
goto out;
- }
}
+
out:
mv88e6xxx_reg_unlock(chip);
@@ -2212,6 +2250,7 @@ static int mv88e6xxx_port_vlan_del(struct dsa_switch *ds, int port,
const struct switchdev_obj_port_vlan *vlan)
{
struct mv88e6xxx_chip *chip = ds->priv;
+ struct mv88e6xxx_port *p = &chip->ports[port];
int err = 0;
u16 pvid;
@@ -2229,7 +2268,9 @@ static int mv88e6xxx_port_vlan_del(struct dsa_switch *ds, int port,
goto unlock;
if (vlan->vid == pvid) {
- err = mv88e6xxx_port_set_pvid(chip, port, 0);
+ p->bridge_pvid.valid = false;
+
+ err = mv88e6xxx_port_commit_pvid(chip, port);
if (err)
goto unlock;
}
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 59f316cc8583..33d067e8396d 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -246,9 +246,15 @@ struct mv88e6xxx_policy {
u16 vid;
};
+struct mv88e6xxx_vlan {
+ u16 vid;
+ bool valid;
+};
+
struct mv88e6xxx_port {
struct mv88e6xxx_chip *chip;
int port;
+ struct mv88e6xxx_vlan bridge_pvid;
u64 serdes_stats[2];
u64 atu_member_violation;
u64 atu_miss_violation;
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 451028c57af8..d9817b20ea64 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -1257,6 +1257,27 @@ int mv88e6xxx_port_set_8021q_mode(struct mv88e6xxx_chip *chip, int port,
return 0;
}
+int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port,
+ bool drop_untagged)
+{
+ u16 old, new;
+ int err;
+
+ err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_CTL2, &old);
+ if (err)
+ return err;
+
+ if (drop_untagged)
+ new = old | MV88E6XXX_PORT_CTL2_DISCARD_UNTAGGED;
+ else
+ new = old & ~MV88E6XXX_PORT_CTL2_DISCARD_UNTAGGED;
+
+ if (new == old)
+ return 0;
+
+ return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL2, new);
+}
+
int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port)
{
u16 reg;
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index b10e5aebacf6..03382b66f800 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -423,6 +423,8 @@ int mv88e6393x_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
phy_interface_t mode);
int mv88e6185_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode);
int mv88e6352_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode);
+int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port,
+ bool drop_untagged);
int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port);
int mv88e6095_port_set_upstream_port(struct mv88e6xxx_chip *chip, int port,
int upstream_port);
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 net 4/4] net: dsa: mv88e6xxx: isolate the ATU databases of standalone and bridged ports
2021-10-05 0:14 [PATCH v2 net 0/4] DSA bridge TX forwarding offload fixes - part 1 Vladimir Oltean
` (2 preceding siblings ...)
2021-10-05 0:14 ` [PATCH v2 net 3/4] net: dsa: mv88e6xxx: keep the pvid at 0 when VLAN-unaware Vladimir Oltean
@ 2021-10-05 0:14 ` Vladimir Oltean
2021-10-05 18:32 ` Tobias Waldekranz
3 siblings, 1 reply; 7+ messages in thread
From: Vladimir Oltean @ 2021-10-05 0:14 UTC (permalink / raw)
To: netdev, Jakub Kicinski, David S. Miller
Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, Tobias Waldekranz,
DENG Qingfang
Similar to commit 6087175b7991 ("net: dsa: mt7530: use independent VLAN
learning on VLAN-unaware bridges"), software forwarding between an
unoffloaded LAG port (a bonding interface with an unsupported policy)
and a mv88e6xxx user port directly under a bridge is broken.
We adopt the same strategy, which is to make the standalone ports not
find any ATU entry learned on a bridge port.
Theory: the mv88e6xxx ATU is looked up by FID and MAC address. There are
as many FIDs as VIDs (4096). The FID is derived from the VID when
possible (the VTU maps a VID to a FID), with a fallback to the port
based default FID value when not (802.1Q Mode is disabled on the port,
or the classified VID isn't present in the VTU).
The mv88e6xxx driver makes the following use of FIDs and VIDs:
- the port's DefaultVID (to which untagged & pvid-tagged packets get
classified) is absent from the VTU, so this kind of packets is
processed in FID 0, the default FID assigned by mv88e6xxx_setup_port.
- every time a bridge VLAN is created, mv88e6xxx_port_vlan_join() ->
mv88e6xxx_atu_new() associates a FID with that VID which increases
linearly starting from 1. Like this:
bridge vlan add dev lan0 vid 100 # FID 1
bridge vlan add dev lan1 vid 100 # still FID 1
bridge vlan add dev lan2 vid 1024 # FID 2
The FID allocation made by the driver is sub-optimal for the following
reasons:
(a) A standalone port has a DefaultPVID of 0 and a default FID of 0 too.
A VLAN-unaware bridged port has a DefaultPVID of 0 and a default FID
of 0 too. The difference is that the bridged ports may learn ATU
entries, while the standalone port may not, and must not find them
either. Standalone ports must not use the same FID as ports
belonging to a bridge. They can use the same FID between each other,
since the ATU will never have an entry in that FID.
(b) Multiple VLAN-unaware bridges will all use a DefaultPVID of 0 and a
default FID of 0 on all their ports. The FDBs will not be isolated
between these bridges. Every VLAN-unaware bridge must use the same
FID on all its ports, different from the FID of other bridge ports.
(c) Each bridge VLAN uses a unique FID which is useful for Independent
VLAN Learning, but the same VLAN ID on multiple VLAN-aware bridges
will result in the same FID being used by mv88e6xxx_atu_new().
The correct behavior is for VLAN 1 in br0 to have a different FID
compared to VLAN 1 in br1.
This patch cannot fix all the above. Traditionally the DSA framework did
not care about this, and the reality is that DSA core involvement is
needed for the aforementioned issues to be solved. The only thing we can
solve here is an issue which does not require API changes, and that is
issue (a).
The first step is deciding what VID and FID to use for standalone ports,
and what VID and FID for bridged ports. The 0/0 pair for standalone
ports is what they used up till now, let's keep using that. For bridged
ports, there are 2 cases:
- VLAN-unaware ports will end up using the port default FID, because we
leave their DefaultVID (pvid) at zero (just as in the case of
standalone ports), a VID which is absent from the VTU.
- VLAN-aware ports will never end up using the port default FID, because
packets will always be classified to a VID in the VTU or dropped
otherwise. The FID is the one associated with the VID in the VTU.
Having that said, any value will do for the FID of VLAN-unaware bridge
ports, but we choose 1 because it comes after 0.
So on ingress from user ports that are under a VLAN-unaware bridge,
tag_dsa.c will see a packet with VID 0 and FID 1. So for the xmit to
look up the same ATU database, we need to xmit in FID 1 as well.
Because CPU and DSA ports are shared ports, we need to be smarter about
the way in which we classify a packet to FID 1, we can't just rely on
the port default FID. The only way in which that appears possible is by
enabling the 802.1Q Mode on the shared ports (currently it is disabled),
so as to let the switch derive the VID from the skb, and then the FID
from the VID. We choose the least strict 802.1Q mode, Fallback, which:
- if the ingress frame's VID isn't in the VTU
-> doesn't drop it
-> forwards it according to the ingress port's forwarding matrix
-> classifies it to the DefaultVID (=> port default FID)
-> if the ingress frame's VID is in the VTU
-> forwards it according to the ingress port's forwarding matrix AND
the mask of ports that are members of that VID
-> classifies it to that VID (=> its associated FID in the VTU)
But to get the desired effect of classifying some packets to FID 1 on
xmit, we need to install a VID in the ATU which maps to that FID. We
choose VID 4095, because the bridge cannot install a VID with that value
so nobody will notice. We install VID 4095 to the VTU in
mv88e6xxx_setup_port(), with the mention that mv88e6xxx_vtu_setup()
which was located right below that call was flushing the VTU so those
entries wouldn't be preserved. So we need to relocate the VTU flushing
prior to the port initialization during ->setup(). Also note that this
is why it is safe to assume that VID 4095 will get associated with FID 1:
the user ports haven't been created, so there is no avenue for the user
to create a bridge VLAN which could otherwise race with the creation of
another FID which would otherwise use up the non-reserved FID value of 1.
Currently mv88e6xxx_port_vlan_join() doesn't have the option of
specifying a preferred FID, it always calls mv88e6xxx_atu_new().
mv88e6xxx_port_db_load_purge() is the function to access the ATU for
FDB/MDB entries, and it used to determine the FID to use for
VLAN-unaware FDB entries (VID=0) using mv88e6xxx_port_get_fid().
But the driver only called mv88e6xxx_port_set_fid() once, during probe,
so no surprises, the port FID was always 0. As much as I would have
wanted to not touch that code, the logic is broken when we add a new
FID which is not the port-based default. Sure, FID 1 is the default FID
for bridged ports, but the host-filtered FDB entries which are installed
by DSA on the shared (DSA and CPU) ports should be installed in FID 1,
not in FID 0, but FID 1 is not the port default FID on the shared ports.
So we need to recognize that is more correct to simply hardcode FID 1 in
mv88e6xxx_port_db_load_purge() and revisit when we have FDB isolation in
the DSA API.
Lastly, the tagger needs to check, when it is transmitting a VLAN
untagged skb, whether it is sending it towards a bridged or a standalone
port. When we see it is bridged we assume the bridge is VLAN-unaware.
Not because it cannot be VLAN-aware but:
- if we are transmitting from a VLAN-aware bridge we are likely doing so
using TX forwarding offload. That code path guarantees that skbs have
a vlan hwaccel tag in them, so we would not enter the "else" branch
of the "if (skb->protocol == htons(ETH_P_8021Q))" condition.
- if we are transmitting on behalf of a VLAN-aware bridge but with no TX
forwarding offload (no PVT support, out of space in the PVT, whatever),
we would indeed be transmitting with VLAN 4095 instead of the bridge
device's pvid. However we would be injecting a "From CPU" frame, and
the switch won't learn from that - it only learns from "Forward" frames.
So it is inconsequential for address learning. And VLAN 4095 is
absolutely enough for the frame to exit the switch, since we never
remove that VLAN from any port.
Fixes: 57e661aae6a8 ("net: dsa: mv88e6xxx: Link aggregation support")
Reported-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new
MAINTAINERS | 1 +
drivers/net/dsa/mv88e6xxx/chip.c | 59 +++++++++++++++++++++++++-------
drivers/net/dsa/mv88e6xxx/chip.h | 3 ++
include/linux/dsa/mv88e6xxx.h | 13 +++++++
net/dsa/tag_dsa.c | 12 +++++--
5 files changed, 73 insertions(+), 15 deletions(-)
create mode 100644 include/linux/dsa/mv88e6xxx.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 6fbedd4784a3..632580791d2d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11147,6 +11147,7 @@ S: Maintained
F: Documentation/devicetree/bindings/net/dsa/marvell.txt
F: Documentation/networking/devlink/mv88e6xxx.rst
F: drivers/net/dsa/mv88e6xxx/
+F: include/linux/dsa/mv88e6xxx.h
F: include/linux/platform_data/mv88e6xxx.h
MARVELL ARMADA 3700 PHY DRIVERS
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index d672112afffd..0aa7e4394aab 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -12,6 +12,7 @@
#include <linux/bitfield.h>
#include <linux/delay.h>
+#include <linux/dsa/mv88e6xxx.h>
#include <linux/etherdevice.h>
#include <linux/ethtool.h>
#include <linux/if_bridge.h>
@@ -1754,11 +1755,15 @@ static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
u16 fid;
int err;
- /* Null VLAN ID corresponds to the port private database */
+ /* Ports have two private address databases: one for when the port is
+ * standalone and one for when the port is under a bridge and the
+ * 802.1Q mode is disabled. When the port is standalone, DSA wants its
+ * address database to remain 100% empty, so we never load an ATU entry
+ * into a standalone port's database. Therefore, translate the null
+ * VLAN ID into the port's database used for VLAN-unaware bridging.
+ */
if (vid == 0) {
- err = mv88e6xxx_port_get_fid(chip, port, &fid);
- if (err)
- return err;
+ fid = MV88E6XXX_FID_BRIDGED;
} else {
err = mv88e6xxx_vtu_get(chip, vid, &vlan);
if (err)
@@ -2434,7 +2439,16 @@ static int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port,
int err;
mv88e6xxx_reg_lock(chip);
+
err = mv88e6xxx_bridge_map(chip, br);
+ if (err)
+ goto unlock;
+
+ err = mv88e6xxx_port_set_fid(chip, port, MV88E6XXX_FID_BRIDGED);
+ if (err)
+ goto unlock;
+
+unlock:
mv88e6xxx_reg_unlock(chip);
return err;
@@ -2446,9 +2460,14 @@ static void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port,
struct mv88e6xxx_chip *chip = ds->priv;
mv88e6xxx_reg_lock(chip);
+
if (mv88e6xxx_bridge_map(chip, br) ||
mv88e6xxx_port_vlan_map(chip, port))
dev_err(ds->dev, "failed to remap in-chip Port VLAN\n");
+
+ if (mv88e6xxx_port_set_fid(chip, port, MV88E6XXX_FID_STANDALONE))
+ dev_err(ds->dev, "failed to restore port default FID\n");
+
mv88e6xxx_reg_unlock(chip);
}
@@ -2823,8 +2842,8 @@ static int mv88e6xxx_setup_upstream_port(struct mv88e6xxx_chip *chip, int port)
static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
{
struct dsa_switch *ds = chip->ds;
+ u16 reg, mode, member;
int err;
- u16 reg;
chip->ports[port].chip = chip;
chip->ports[port].port = port;
@@ -2889,8 +2908,24 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
if (err)
return err;
- err = mv88e6xxx_port_set_8021q_mode(chip, port,
- MV88E6XXX_PORT_CTL2_8021Q_MODE_DISABLED);
+ if (dsa_is_user_port(ds, port)) {
+ mode = MV88E6XXX_PORT_CTL2_8021Q_MODE_DISABLED;
+ member = MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNTAGGED;
+ } else {
+ mode = MV88E6XXX_PORT_CTL2_8021Q_MODE_FALLBACK;
+ member = MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNMODIFIED;
+ }
+
+ err = mv88e6xxx_port_set_8021q_mode(chip, port, mode);
+ if (err)
+ return err;
+
+ /* Associate MV88E6XXX_VID_BRIDGED with MV88E6XXX_FID_BRIDGED in the
+ * ATU by virtue of the fact that mv88e6xxx_atu_new() will pick it as
+ * the first free FID after MV88E6XXX_FID_STANDALONE.
+ */
+ err = mv88e6xxx_port_vlan_join(chip, port, MV88E6XXX_VID_BRIDGED,
+ member, false);
if (err)
return err;
@@ -2966,7 +3001,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
* database, and allow bidirectional communication between the
* CPU and DSA port(s), and the other ports.
*/
- err = mv88e6xxx_port_set_fid(chip, port, 0);
+ err = mv88e6xxx_port_set_fid(chip, port, MV88E6XXX_FID_STANDALONE);
if (err)
return err;
@@ -3156,6 +3191,10 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
}
}
+ err = mv88e6xxx_vtu_setup(chip);
+ if (err)
+ goto unlock;
+
/* Setup Switch Port Registers */
for (i = 0; i < mv88e6xxx_num_ports(chip); i++) {
if (dsa_is_unused_port(ds, i))
@@ -3185,10 +3224,6 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
if (err)
goto unlock;
- err = mv88e6xxx_vtu_setup(chip);
- if (err)
- goto unlock;
-
err = mv88e6xxx_pvt_setup(chip);
if (err)
goto unlock;
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 33d067e8396d..8271b8aa7b71 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -21,6 +21,9 @@
#define EDSA_HLEN 8
#define MV88E6XXX_N_FID 4096
+#define MV88E6XXX_FID_STANDALONE 0
+#define MV88E6XXX_FID_BRIDGED 1
+
/* PVT limits for 4-bit port and 5-bit switch */
#define MV88E6XXX_MAX_PVT_SWITCHES 32
#define MV88E6XXX_MAX_PVT_PORTS 16
diff --git a/include/linux/dsa/mv88e6xxx.h b/include/linux/dsa/mv88e6xxx.h
new file mode 100644
index 000000000000..8c3d45eca46b
--- /dev/null
+++ b/include/linux/dsa/mv88e6xxx.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0
+ * Copyright 2021 NXP
+ */
+
+#ifndef _NET_DSA_TAG_MV88E6XXX_H
+#define _NET_DSA_TAG_MV88E6XXX_H
+
+#include <linux/if_vlan.h>
+
+#define MV88E6XXX_VID_STANDALONE 0
+#define MV88E6XXX_VID_BRIDGED (VLAN_N_VID - 1)
+
+#endif
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index 68d5ddc3ef35..b3da4b2ea11c 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -45,6 +45,7 @@
* 6 6 2 2 4 2 N
*/
+#include <linux/dsa/mv88e6xxx.h>
#include <linux/etherdevice.h>
#include <linux/list.h>
#include <linux/slab.h>
@@ -164,16 +165,21 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
dsa_header[2] &= ~0x10;
}
} else {
+ struct net_device *br = dp->bridge_dev;
+ u16 vid;
+
+ vid = br ? MV88E6XXX_VID_BRIDGED : MV88E6XXX_VID_STANDALONE;
+
skb_push(skb, DSA_HLEN + extra);
dsa_alloc_etype_header(skb, DSA_HLEN + extra);
- /* Construct untagged DSA tag. */
+ /* Construct DSA header from untagged frame. */
dsa_header = dsa_etype_header_pos_tx(skb) + extra;
dsa_header[0] = (cmd << 6) | tag_dev;
dsa_header[1] = tag_port << 3;
- dsa_header[2] = 0;
- dsa_header[3] = 0;
+ dsa_header[2] = vid >> 8;
+ dsa_header[3] = vid & 0xff;
}
return skb;
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 net 4/4] net: dsa: mv88e6xxx: isolate the ATU databases of standalone and bridged ports
2021-10-05 0:14 ` [PATCH v2 net 4/4] net: dsa: mv88e6xxx: isolate the ATU databases of standalone and bridged ports Vladimir Oltean
@ 2021-10-05 18:32 ` Tobias Waldekranz
2021-10-05 18:37 ` Vladimir Oltean
0 siblings, 1 reply; 7+ messages in thread
From: Tobias Waldekranz @ 2021-10-05 18:32 UTC (permalink / raw)
To: Vladimir Oltean, netdev, Jakub Kicinski, David S. Miller
Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, DENG Qingfang
On Tue, Oct 05, 2021 at 03:14, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> Similar to commit 6087175b7991 ("net: dsa: mt7530: use independent VLAN
> learning on VLAN-unaware bridges"), software forwarding between an
> unoffloaded LAG port (a bonding interface with an unsupported policy)
> and a mv88e6xxx user port directly under a bridge is broken.
>
> We adopt the same strategy, which is to make the standalone ports not
> find any ATU entry learned on a bridge port.
>
> Theory: the mv88e6xxx ATU is looked up by FID and MAC address. There are
> as many FIDs as VIDs (4096). The FID is derived from the VID when
> possible (the VTU maps a VID to a FID), with a fallback to the port
> based default FID value when not (802.1Q Mode is disabled on the port,
> or the classified VID isn't present in the VTU).
>
> The mv88e6xxx driver makes the following use of FIDs and VIDs:
>
> - the port's DefaultVID (to which untagged & pvid-tagged packets get
> classified) is absent from the VTU, so this kind of packets is
> processed in FID 0, the default FID assigned by mv88e6xxx_setup_port.
>
> - every time a bridge VLAN is created, mv88e6xxx_port_vlan_join() ->
> mv88e6xxx_atu_new() associates a FID with that VID which increases
> linearly starting from 1. Like this:
>
> bridge vlan add dev lan0 vid 100 # FID 1
> bridge vlan add dev lan1 vid 100 # still FID 1
> bridge vlan add dev lan2 vid 1024 # FID 2
>
> The FID allocation made by the driver is sub-optimal for the following
> reasons:
>
> (a) A standalone port has a DefaultPVID of 0 and a default FID of 0 too.
> A VLAN-unaware bridged port has a DefaultPVID of 0 and a default FID
> of 0 too. The difference is that the bridged ports may learn ATU
> entries, while the standalone port may not, and must not find them
> either. Standalone ports must not use the same FID as ports
> belonging to a bridge. They can use the same FID between each other,
> since the ATU will never have an entry in that FID.
>
> (b) Multiple VLAN-unaware bridges will all use a DefaultPVID of 0 and a
> default FID of 0 on all their ports. The FDBs will not be isolated
> between these bridges. Every VLAN-unaware bridge must use the same
> FID on all its ports, different from the FID of other bridge ports.
>
> (c) Each bridge VLAN uses a unique FID which is useful for Independent
> VLAN Learning, but the same VLAN ID on multiple VLAN-aware bridges
> will result in the same FID being used by mv88e6xxx_atu_new().
> The correct behavior is for VLAN 1 in br0 to have a different FID
> compared to VLAN 1 in br1.
>
> This patch cannot fix all the above. Traditionally the DSA framework did
> not care about this, and the reality is that DSA core involvement is
> needed for the aforementioned issues to be solved. The only thing we can
> solve here is an issue which does not require API changes, and that is
> issue (a).
>
> The first step is deciding what VID and FID to use for standalone ports,
> and what VID and FID for bridged ports. The 0/0 pair for standalone
> ports is what they used up till now, let's keep using that. For bridged
> ports, there are 2 cases:
>
> - VLAN-unaware ports will end up using the port default FID, because we
> leave their DefaultVID (pvid) at zero (just as in the case of
> standalone ports), a VID which is absent from the VTU.
I believe this patch gets aaaalmost everything right. But I think we
need to set the port's PVID to 4095 when it is attached to a
VLAN-unaware bridge. Unfortunately I won't be able to prove it until I
get back to the office tomorrow (need physical access to move some
cables around), but I will go out on a limb and present the theory now
anyway :)
Basically, it has to do with the same issue that you have identified in
the CPU tx path. On user-port ingress, setting the port's default FID is
enough to make it select the correct database -- on a single chip
system. Once you're going over a DSA port though, you need to carry that
information in the tag, just like when sending from the CPU. So in this
scenario:
CPU
| .----.
.--0--. | .--0--.
| sw0 | | | sw1 |
'-1-2-' | '-1-2-'
'---'
Say that sw0p1 and sw1p1 are members of a VLAN-unaware bridge and sw1p2
is in standalone mode. Packets from both sw1p1 and sw1p2 will ingress on
sw0p2 with VID 0 - the port can only have one default FID - yet it has
to map one flow to FID 1 and one to FID 0.
Setting the PVID of sw1p1 should provide that missing piece of
information.
> - VLAN-aware ports will never end up using the port default FID, because
> packets will always be classified to a VID in the VTU or dropped
> otherwise. The FID is the one associated with the VID in the VTU.
>
> Having that said, any value will do for the FID of VLAN-unaware bridge
> ports, but we choose 1 because it comes after 0.
>
> So on ingress from user ports that are under a VLAN-unaware bridge,
> tag_dsa.c will see a packet with VID 0 and FID 1. So for the xmit to
> look up the same ATU database, we need to xmit in FID 1 as well.
>
> Because CPU and DSA ports are shared ports, we need to be smarter about
> the way in which we classify a packet to FID 1, we can't just rely on
> the port default FID. The only way in which that appears possible is by
> enabling the 802.1Q Mode on the shared ports (currently it is disabled),
> so as to let the switch derive the VID from the skb, and then the FID
> from the VID. We choose the least strict 802.1Q mode, Fallback, which:
>
> - if the ingress frame's VID isn't in the VTU
> -> doesn't drop it
> -> forwards it according to the ingress port's forwarding matrix
> -> classifies it to the DefaultVID (=> port default FID)
>
> -> if the ingress frame's VID is in the VTU
> -> forwards it according to the ingress port's forwarding matrix AND
> the mask of ports that are members of that VID
> -> classifies it to that VID (=> its associated FID in the VTU)
>
> But to get the desired effect of classifying some packets to FID 1 on
> xmit, we need to install a VID in the ATU which maps to that FID. We
> choose VID 4095, because the bridge cannot install a VID with that value
> so nobody will notice. We install VID 4095 to the VTU in
> mv88e6xxx_setup_port(), with the mention that mv88e6xxx_vtu_setup()
> which was located right below that call was flushing the VTU so those
> entries wouldn't be preserved. So we need to relocate the VTU flushing
> prior to the port initialization during ->setup(). Also note that this
> is why it is safe to assume that VID 4095 will get associated with FID 1:
> the user ports haven't been created, so there is no avenue for the user
> to create a bridge VLAN which could otherwise race with the creation of
> another FID which would otherwise use up the non-reserved FID value of 1.
> Currently mv88e6xxx_port_vlan_join() doesn't have the option of
> specifying a preferred FID, it always calls mv88e6xxx_atu_new().
>
> mv88e6xxx_port_db_load_purge() is the function to access the ATU for
> FDB/MDB entries, and it used to determine the FID to use for
> VLAN-unaware FDB entries (VID=0) using mv88e6xxx_port_get_fid().
> But the driver only called mv88e6xxx_port_set_fid() once, during probe,
> so no surprises, the port FID was always 0. As much as I would have
> wanted to not touch that code, the logic is broken when we add a new
> FID which is not the port-based default. Sure, FID 1 is the default FID
> for bridged ports, but the host-filtered FDB entries which are installed
> by DSA on the shared (DSA and CPU) ports should be installed in FID 1,
> not in FID 0, but FID 1 is not the port default FID on the shared ports.
> So we need to recognize that is more correct to simply hardcode FID 1 in
> mv88e6xxx_port_db_load_purge() and revisit when we have FDB isolation in
> the DSA API.
>
> Lastly, the tagger needs to check, when it is transmitting a VLAN
> untagged skb, whether it is sending it towards a bridged or a standalone
> port. When we see it is bridged we assume the bridge is VLAN-unaware.
> Not because it cannot be VLAN-aware but:
>
> - if we are transmitting from a VLAN-aware bridge we are likely doing so
> using TX forwarding offload. That code path guarantees that skbs have
> a vlan hwaccel tag in them, so we would not enter the "else" branch
> of the "if (skb->protocol == htons(ETH_P_8021Q))" condition.
>
> - if we are transmitting on behalf of a VLAN-aware bridge but with no TX
> forwarding offload (no PVT support, out of space in the PVT, whatever),
> we would indeed be transmitting with VLAN 4095 instead of the bridge
> device's pvid. However we would be injecting a "From CPU" frame, and
> the switch won't learn from that - it only learns from "Forward" frames.
> So it is inconsequential for address learning. And VLAN 4095 is
> absolutely enough for the frame to exit the switch, since we never
> remove that VLAN from any port.
>
> Fixes: 57e661aae6a8 ("net: dsa: mv88e6xxx: Link aggregation support")
> Reported-by: Tobias Waldekranz <tobias@waldekranz.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v1->v2: patch is new
>
> MAINTAINERS | 1 +
> drivers/net/dsa/mv88e6xxx/chip.c | 59 +++++++++++++++++++++++++-------
> drivers/net/dsa/mv88e6xxx/chip.h | 3 ++
> include/linux/dsa/mv88e6xxx.h | 13 +++++++
> net/dsa/tag_dsa.c | 12 +++++--
> 5 files changed, 73 insertions(+), 15 deletions(-)
> create mode 100644 include/linux/dsa/mv88e6xxx.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6fbedd4784a3..632580791d2d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11147,6 +11147,7 @@ S: Maintained
> F: Documentation/devicetree/bindings/net/dsa/marvell.txt
> F: Documentation/networking/devlink/mv88e6xxx.rst
> F: drivers/net/dsa/mv88e6xxx/
> +F: include/linux/dsa/mv88e6xxx.h
> F: include/linux/platform_data/mv88e6xxx.h
>
> MARVELL ARMADA 3700 PHY DRIVERS
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index d672112afffd..0aa7e4394aab 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -12,6 +12,7 @@
>
> #include <linux/bitfield.h>
> #include <linux/delay.h>
> +#include <linux/dsa/mv88e6xxx.h>
> #include <linux/etherdevice.h>
> #include <linux/ethtool.h>
> #include <linux/if_bridge.h>
> @@ -1754,11 +1755,15 @@ static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
> u16 fid;
> int err;
>
> - /* Null VLAN ID corresponds to the port private database */
> + /* Ports have two private address databases: one for when the port is
> + * standalone and one for when the port is under a bridge and the
> + * 802.1Q mode is disabled. When the port is standalone, DSA wants its
> + * address database to remain 100% empty, so we never load an ATU entry
> + * into a standalone port's database. Therefore, translate the null
> + * VLAN ID into the port's database used for VLAN-unaware bridging.
> + */
> if (vid == 0) {
> - err = mv88e6xxx_port_get_fid(chip, port, &fid);
> - if (err)
> - return err;
> + fid = MV88E6XXX_FID_BRIDGED;
> } else {
> err = mv88e6xxx_vtu_get(chip, vid, &vlan);
> if (err)
> @@ -2434,7 +2439,16 @@ static int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port,
> int err;
>
> mv88e6xxx_reg_lock(chip);
> +
> err = mv88e6xxx_bridge_map(chip, br);
> + if (err)
> + goto unlock;
> +
> + err = mv88e6xxx_port_set_fid(chip, port, MV88E6XXX_FID_BRIDGED);
> + if (err)
> + goto unlock;
> +
> +unlock:
> mv88e6xxx_reg_unlock(chip);
>
> return err;
> @@ -2446,9 +2460,14 @@ static void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port,
> struct mv88e6xxx_chip *chip = ds->priv;
>
> mv88e6xxx_reg_lock(chip);
> +
> if (mv88e6xxx_bridge_map(chip, br) ||
> mv88e6xxx_port_vlan_map(chip, port))
> dev_err(ds->dev, "failed to remap in-chip Port VLAN\n");
> +
> + if (mv88e6xxx_port_set_fid(chip, port, MV88E6XXX_FID_STANDALONE))
> + dev_err(ds->dev, "failed to restore port default FID\n");
> +
> mv88e6xxx_reg_unlock(chip);
> }
>
> @@ -2823,8 +2842,8 @@ static int mv88e6xxx_setup_upstream_port(struct mv88e6xxx_chip *chip, int port)
> static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
> {
> struct dsa_switch *ds = chip->ds;
> + u16 reg, mode, member;
> int err;
> - u16 reg;
>
> chip->ports[port].chip = chip;
> chip->ports[port].port = port;
> @@ -2889,8 +2908,24 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
> if (err)
> return err;
>
> - err = mv88e6xxx_port_set_8021q_mode(chip, port,
> - MV88E6XXX_PORT_CTL2_8021Q_MODE_DISABLED);
> + if (dsa_is_user_port(ds, port)) {
> + mode = MV88E6XXX_PORT_CTL2_8021Q_MODE_DISABLED;
> + member = MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNTAGGED;
> + } else {
> + mode = MV88E6XXX_PORT_CTL2_8021Q_MODE_FALLBACK;
> + member = MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNMODIFIED;
> + }
> +
> + err = mv88e6xxx_port_set_8021q_mode(chip, port, mode);
> + if (err)
> + return err;
> +
> + /* Associate MV88E6XXX_VID_BRIDGED with MV88E6XXX_FID_BRIDGED in the
> + * ATU by virtue of the fact that mv88e6xxx_atu_new() will pick it as
> + * the first free FID after MV88E6XXX_FID_STANDALONE.
> + */
> + err = mv88e6xxx_port_vlan_join(chip, port, MV88E6XXX_VID_BRIDGED,
> + member, false);
> if (err)
> return err;
>
> @@ -2966,7 +3001,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
> * database, and allow bidirectional communication between the
> * CPU and DSA port(s), and the other ports.
> */
> - err = mv88e6xxx_port_set_fid(chip, port, 0);
> + err = mv88e6xxx_port_set_fid(chip, port, MV88E6XXX_FID_STANDALONE);
> if (err)
> return err;
>
> @@ -3156,6 +3191,10 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
> }
> }
>
> + err = mv88e6xxx_vtu_setup(chip);
> + if (err)
> + goto unlock;
> +
> /* Setup Switch Port Registers */
> for (i = 0; i < mv88e6xxx_num_ports(chip); i++) {
> if (dsa_is_unused_port(ds, i))
> @@ -3185,10 +3224,6 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
> if (err)
> goto unlock;
>
> - err = mv88e6xxx_vtu_setup(chip);
> - if (err)
> - goto unlock;
> -
> err = mv88e6xxx_pvt_setup(chip);
> if (err)
> goto unlock;
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
> index 33d067e8396d..8271b8aa7b71 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.h
> +++ b/drivers/net/dsa/mv88e6xxx/chip.h
> @@ -21,6 +21,9 @@
> #define EDSA_HLEN 8
> #define MV88E6XXX_N_FID 4096
>
> +#define MV88E6XXX_FID_STANDALONE 0
> +#define MV88E6XXX_FID_BRIDGED 1
> +
> /* PVT limits for 4-bit port and 5-bit switch */
> #define MV88E6XXX_MAX_PVT_SWITCHES 32
> #define MV88E6XXX_MAX_PVT_PORTS 16
> diff --git a/include/linux/dsa/mv88e6xxx.h b/include/linux/dsa/mv88e6xxx.h
> new file mode 100644
> index 000000000000..8c3d45eca46b
> --- /dev/null
> +++ b/include/linux/dsa/mv88e6xxx.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + * Copyright 2021 NXP
> + */
> +
> +#ifndef _NET_DSA_TAG_MV88E6XXX_H
> +#define _NET_DSA_TAG_MV88E6XXX_H
> +
> +#include <linux/if_vlan.h>
> +
> +#define MV88E6XXX_VID_STANDALONE 0
> +#define MV88E6XXX_VID_BRIDGED (VLAN_N_VID - 1)
> +
> +#endif
> diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
> index 68d5ddc3ef35..b3da4b2ea11c 100644
> --- a/net/dsa/tag_dsa.c
> +++ b/net/dsa/tag_dsa.c
> @@ -45,6 +45,7 @@
> * 6 6 2 2 4 2 N
> */
>
> +#include <linux/dsa/mv88e6xxx.h>
> #include <linux/etherdevice.h>
> #include <linux/list.h>
> #include <linux/slab.h>
> @@ -164,16 +165,21 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
> dsa_header[2] &= ~0x10;
> }
> } else {
> + struct net_device *br = dp->bridge_dev;
> + u16 vid;
> +
> + vid = br ? MV88E6XXX_VID_BRIDGED : MV88E6XXX_VID_STANDALONE;
> +
> skb_push(skb, DSA_HLEN + extra);
> dsa_alloc_etype_header(skb, DSA_HLEN + extra);
>
> - /* Construct untagged DSA tag. */
> + /* Construct DSA header from untagged frame. */
> dsa_header = dsa_etype_header_pos_tx(skb) + extra;
>
> dsa_header[0] = (cmd << 6) | tag_dev;
> dsa_header[1] = tag_port << 3;
> - dsa_header[2] = 0;
> - dsa_header[3] = 0;
> + dsa_header[2] = vid >> 8;
> + dsa_header[3] = vid & 0xff;
> }
>
> return skb;
> --
> 2.25.1
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 net 4/4] net: dsa: mv88e6xxx: isolate the ATU databases of standalone and bridged ports
2021-10-05 18:32 ` Tobias Waldekranz
@ 2021-10-05 18:37 ` Vladimir Oltean
0 siblings, 0 replies; 7+ messages in thread
From: Vladimir Oltean @ 2021-10-05 18:37 UTC (permalink / raw)
To: Tobias Waldekranz
Cc: netdev, Jakub Kicinski, David S. Miller, Florian Fainelli,
Andrew Lunn, Vivien Didelot, DENG Qingfang
On Tue, Oct 05, 2021 at 08:32:06PM +0200, Tobias Waldekranz wrote:
> I believe this patch gets aaaalmost everything right. But I think we
> need to set the port's PVID to 4095 when it is attached to a
> VLAN-unaware bridge. Unfortunately I won't be able to prove it until I
> get back to the office tomorrow (need physical access to move some
> cables around), but I will go out on a limb and present the theory now
> anyway :)
>
> Basically, it has to do with the same issue that you have identified in
> the CPU tx path. On user-port ingress, setting the port's default FID is
> enough to make it select the correct database -- on a single chip
> system. Once you're going over a DSA port though, you need to carry that
> information in the tag, just like when sending from the CPU. So in this
> scenario:
>
> CPU
> | .----.
> .--0--. | .--0--.
> | sw0 | | | sw1 |
> '-1-2-' | '-1-2-'
> '---'
>
> Say that sw0p1 and sw1p1 are members of a VLAN-unaware bridge and sw1p2
> is in standalone mode. Packets from both sw1p1 and sw1p2 will ingress on
> sw0p2 with VID 0 - the port can only have one default FID - yet it has
> to map one flow to FID 1 and one to FID 0.
>
> Setting the PVID of sw1p1 should provide that missing piece of
> information.
Hey, Spiderman!
Yes, that makes sense, and is something I can actually test, no need to
wait until tomorrow. Give me a few hours, I need to finish something else first.
^ permalink raw reply [flat|nested] 7+ messages in thread