* [PATCH v2 net 0/3] Clear rx-vlan-filter feature in DSA when necessary
@ 2021-03-20 22:59 Vladimir Oltean
2021-03-20 22:59 ` [PATCH v2 net 1/3] net: dsa: only unset VLAN filtering when last port leaves last VLAN-aware bridge Vladimir Oltean
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Vladimir Oltean @ 2021-03-20 22:59 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, netdev
Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, Kurt Kanzenbach,
Tobias Waldekranz, Vladimir Oltean
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Patches 2 and 3 address the problems raised by Tobias here:
https://patchwork.kernel.org/project/netdevbpf/cover/20210308150405.3694678-1-tobias@waldekranz.com/
The key difference compared to Tobias' patch is that his approach makes
dsa_slave_vlan_rx_add_vid accept -EOPNOTSUPP and silently transforms it
into an error code of 0, while my patch series avoids calling
dsa_slave_vlan_rx_add_vid when it is not needed.
Patch 1 is another issue I found while working on a solution.
Vladimir Oltean (3):
net: dsa: only unset VLAN filtering when last port leaves last
VLAN-aware bridge
net: dsa: don't advertise 'rx-vlan-filter' if VLAN filtering not
global
net: dsa: let drivers state that they need VLAN filtering while
standalone
drivers/net/dsa/hirschmann/hellcreek.c | 1 +
include/net/dsa.h | 3 ++
net/dsa/dsa_priv.h | 2 +
net/dsa/port.c | 37 ++++++++++++++-
net/dsa/slave.c | 62 +++++++++++++++++++++++++-
net/dsa/switch.c | 35 ++++++++++-----
6 files changed, 125 insertions(+), 15 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 net 1/3] net: dsa: only unset VLAN filtering when last port leaves last VLAN-aware bridge
2021-03-20 22:59 [PATCH v2 net 0/3] Clear rx-vlan-filter feature in DSA when necessary Vladimir Oltean
@ 2021-03-20 22:59 ` Vladimir Oltean
2021-03-22 17:52 ` Tobias Waldekranz
2021-03-20 22:59 ` [PATCH v2 net 2/3] net: dsa: don't advertise 'rx-vlan-filter' if VLAN filtering not global Vladimir Oltean
2021-03-20 22:59 ` [PATCH v2 net 3/3] net: dsa: let drivers state that they need VLAN filtering while standalone Vladimir Oltean
2 siblings, 1 reply; 10+ messages in thread
From: Vladimir Oltean @ 2021-03-20 22:59 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, netdev
Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, Kurt Kanzenbach,
Tobias Waldekranz, Vladimir Oltean
From: Vladimir Oltean <vladimir.oltean@nxp.com>
DSA is aware of switches with global VLAN filtering since the blamed
commit, but it makes a bad decision when multiple bridges are spanning
the same switch:
ip link add br0 type bridge vlan_filtering 1
ip link add br1 type bridge vlan_filtering 1
ip link set swp2 master br0
ip link set swp3 master br0
ip link set swp4 master br1
ip link set swp5 master br1
ip link set swp5 nomaster
ip link set swp4 nomaster
[138665.939930] sja1105 spi0.1: port 3: dsa_core: VLAN filtering is a global setting
[138665.947514] DSA: failed to notify DSA_NOTIFIER_BRIDGE_LEAVE
When all ports leave br1, DSA blindly attempts to disable VLAN filtering
on the switch, ignoring the fact that br0 still exists and is VLAN-aware
too. It fails while doing that.
This patch checks whether any port exists at all and is under a
VLAN-aware bridge.
Fixes: d371b7c92d19 ("net: dsa: Unset vlan_filtering when ports leave the bridge")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
net/dsa/switch.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 4b5da89dc27a..32963276452f 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -107,7 +107,7 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,
bool unset_vlan_filtering = br_vlan_enabled(info->br);
struct dsa_switch_tree *dst = ds->dst;
struct netlink_ext_ack extack = {0};
- int err, i;
+ int err, port;
if (dst->index == info->tree_index && ds->index == info->sw_index &&
ds->ops->port_bridge_join)
@@ -124,13 +124,16 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,
* it. That is a good thing, because that lets us handle it and also
* handle the case where the switch's vlan_filtering setting is global
* (not per port). When that happens, the correct moment to trigger the
- * vlan_filtering callback is only when the last port left this bridge.
+ * vlan_filtering callback is only when the last port leaves the last
+ * VLAN-aware bridge.
*/
if (unset_vlan_filtering && ds->vlan_filtering_is_global) {
- for (i = 0; i < ds->num_ports; i++) {
- if (i == info->port)
- continue;
- if (dsa_to_port(ds, i)->bridge_dev == info->br) {
+ for (port = 0; port < ds->num_ports; port++) {
+ struct net_device *bridge_dev;
+
+ bridge_dev = dsa_to_port(ds, port)->bridge_dev;
+
+ if (bridge_dev && br_vlan_enabled(bridge_dev)) {
unset_vlan_filtering = false;
break;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 net 2/3] net: dsa: don't advertise 'rx-vlan-filter' if VLAN filtering not global
2021-03-20 22:59 [PATCH v2 net 0/3] Clear rx-vlan-filter feature in DSA when necessary Vladimir Oltean
2021-03-20 22:59 ` [PATCH v2 net 1/3] net: dsa: only unset VLAN filtering when last port leaves last VLAN-aware bridge Vladimir Oltean
@ 2021-03-20 22:59 ` Vladimir Oltean
2021-03-23 2:40 ` Florian Fainelli
2021-03-20 22:59 ` [PATCH v2 net 3/3] net: dsa: let drivers state that they need VLAN filtering while standalone Vladimir Oltean
2 siblings, 1 reply; 10+ messages in thread
From: Vladimir Oltean @ 2021-03-20 22:59 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, netdev
Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, Kurt Kanzenbach,
Tobias Waldekranz, Vladimir Oltean
From: Vladimir Oltean <vladimir.oltean@nxp.com>
The blamed patch has removed the driver's ability to return -EOPNOTSUPP
in the .port_vlan_add method when called from .ndo_vlan_rx_add_vid
(unmassaged by DSA, -EOPNOTSUPP is a hard error for vlan_vid_add).
But we have not managed well enough the cases under which .port_vlan_add
is called in the first place, as will be explained below. This was
reported as a problem by Tobias because mv88e6xxx_port_vlan_prepare is
stubborn and only accepts VLANs on bridged ports. That is understandably
so, because standalone mv88e6xxx ports are VLAN-unaware, and VTU entries
are said to be a scarce resource.
Otherwise said, the following fails lamentably on mv88e6xxx:
ip link add br0 type bridge vlan_filtering 1
ip link set lan3 master br0
ip link add link lan10 name lan10.1 type vlan id 1
[485256.724147] mv88e6085 d0032004.mdio-mii:12: p10: hw VLAN 1 already used by port 3 in br0
RTNETLINK answers: Operation not supported
We need to step back and explain that the dsa_slave_vlan_rx_add_vid and
dsa_slave_vlan_rx_kill_vid methods exist for drivers that need the
'rx-vlan-filter: on' feature in ethtool -k, which can be due to any of
the following reasons:
1. vlan_filtering_is_global = true, and some ports are under a
VLAN-aware bridge while others are standalone, and the standalone
ports would otherwise drop VLAN-tagged traffic. This is described in
commit 061f6a505ac3 ("net: dsa: Add ndo_vlan_rx_{add, kill}_vid
implementation").
2. the ports that are under a VLAN-aware bridge should also set this
feature, for 8021q uppers having a VID not claimed by the bridge.
In this case, the driver will essentially not even know that the VID
is coming from the 8021q layer and not the bridge.
3. Hellcreek. This driver needs it because in standalone mode, it uses
unique VLANs per port to ensure separation. For separation of untagged
traffic, it uses different PVIDs for each port, and for separation of
VLAN-tagged traffic, it never accepts 8021q uppers with the same vid
on two ports.
If a driver does not fall under any of the above 3 categories, there is
no reason why it should advertise the 'rx-vlan-filter' feature, therefore
no reason why it should offload the VLANs added through vlan_vid_add.
This commit fixes the problem by removing the 'rx-vlan-filter' feature
from the slave devices when they operate in standalone mode, and when
they offload a VLAN-unaware bridge. This gives the mv88e6xxx driver what
it wants, since it keeps the 8021q VLANs away from the VTU until VLAN
awareness is enabled (point at which the ports are no longer standalone,
hence the check in mv88e6xxx_port_vlan_prepare passes). And since the
issue predates the existence of the hellcreek driver, case 3 will be
dealt with in a separate patch.
The commit also has the nice side effect that we no longer lie to the
network stack about our VLAN filtering status.
Because the 'rx-vlan-filter' feature is now dynamically toggled, and our
.ndo_vlan_rx_add_vid does not get called when 'rx-vlan-filter' is off,
we need to avoid bugs such as the following by replaying the VLANs from
8021q uppers every time we enable VLAN filtering:
ip link add link lan0 name lan0.100 type vlan id 100
ip addr add 192.168.100.1/24 dev lan0.100
ping 192.168.100.2 # should work
ip link add br0 type bridge vlan_filtering 0
ip link set lan0 master br0
ping 192.168.100.2 # should still work
ip link set br0 type bridge vlan_filtering 1
ping 192.168.100.2 # should still work but doesn't
Fixes: 9b236d2a69da ("net: dsa: Advertise the VLAN offload netdev ability only if switch supports it")
Reported-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/dsa_priv.h | 2 ++
net/dsa/port.c | 37 +++++++++++++++++++++++++++--
net/dsa/slave.c | 58 ++++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 93 insertions(+), 4 deletions(-)
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 4c43c5406834..d7dd9e07d168 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -269,6 +269,8 @@ int dsa_slave_register_notifier(void);
void dsa_slave_unregister_notifier(void);
void dsa_slave_setup_tagger(struct net_device *slave);
int dsa_slave_change_mtu(struct net_device *dev, int new_mtu);
+int dsa_slave_manage_vlan_filtering(struct net_device *dev,
+ bool vlan_filtering);
static inline struct dsa_port *dsa_slave_to_port(const struct net_device *dev)
{
diff --git a/net/dsa/port.c b/net/dsa/port.c
index c9c6d7ab3f47..902095f04e0a 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -363,6 +363,7 @@ static bool dsa_port_can_apply_vlan_filtering(struct dsa_port *dp,
int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
struct netlink_ext_ack *extack)
{
+ bool old_vlan_filtering = dsa_port_is_vlan_filtering(dp);
struct dsa_switch *ds = dp->ds;
bool apply;
int err;
@@ -388,12 +389,44 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
if (err)
return err;
- if (ds->vlan_filtering_is_global)
+ if (ds->vlan_filtering_is_global) {
+ int port;
+
+ for (port = 0; port < ds->num_ports; port++) {
+ struct net_device *slave;
+
+ if (!dsa_is_user_port(ds, port))
+ continue;
+
+ /* We might be called in the unbind path, so not
+ * all slave devices might still be registered.
+ */
+ slave = dsa_to_port(ds, port)->slave;
+ if (!slave)
+ continue;
+
+ err = dsa_slave_manage_vlan_filtering(slave,
+ vlan_filtering);
+ if (err)
+ goto restore;
+ }
+
ds->vlan_filtering = vlan_filtering;
- else
+ } else {
+ err = dsa_slave_manage_vlan_filtering(dp->slave,
+ vlan_filtering);
+ if (err)
+ goto restore;
+
dp->vlan_filtering = vlan_filtering;
+ }
return 0;
+
+restore:
+ ds->ops->port_vlan_filtering(ds, dp->index, old_vlan_filtering, NULL);
+
+ return err;
}
/* This enforces legacy behavior for switch drivers which assume they can't
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 992fcab4b552..6d06d13cdf3a 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1387,6 +1387,62 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
return 0;
}
+static int dsa_slave_restore_vlan(struct net_device *vdev, int vid, void *arg)
+{
+ __be16 proto = vdev ? vlan_dev_vlan_proto(vdev) : htons(ETH_P_8021Q);
+
+ return dsa_slave_vlan_rx_add_vid(arg, proto, vid);
+}
+
+static int dsa_slave_clear_vlan(struct net_device *vdev, int vid, void *arg)
+{
+ __be16 proto = vdev ? vlan_dev_vlan_proto(vdev) : htons(ETH_P_8021Q);
+
+ return dsa_slave_vlan_rx_kill_vid(arg, proto, vid);
+}
+
+/* Keep the VLAN RX filtering list in sync with the hardware only if VLAN
+ * filtering is enabled.
+ *
+ * - Standalone ports offload:
+ * - no VLAN (any 8021q upper is a software VLAN) if
+ * ds->vlan_filtering_is_global = false
+ * - the 8021q upper VLANs if ds->vlan_filtering_is_global = true and there
+ * are bridges spanning this switch chip which have vlan_filtering=1
+ *
+ * - Ports under a vlan_filtering=0 bridge offload:
+ * - no VLAN if ds->configure_vlan_while_not_filtering = false (deprecated)
+ * - the bridge VLANs if ds->configure_vlan_while_not_filtering = true
+ *
+ * - Ports under a vlan_filtering=1 bridge offload:
+ * - the bridge VLANs
+ * - the 8021q upper VLANs
+ */
+int dsa_slave_manage_vlan_filtering(struct net_device *slave,
+ bool vlan_filtering)
+{
+ int err;
+
+ if (vlan_filtering) {
+ slave->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
+
+ err = vlan_for_each(slave, dsa_slave_restore_vlan, slave);
+ if (err) {
+ vlan_for_each(slave, dsa_slave_clear_vlan, slave);
+ slave->features &= ~NETIF_F_HW_VLAN_CTAG_FILTER;
+ return err;
+ }
+ } else {
+ err = vlan_for_each(slave, dsa_slave_clear_vlan, slave);
+ if (err)
+ return err;
+
+ slave->features &= ~NETIF_F_HW_VLAN_CTAG_FILTER;
+ }
+
+ return 0;
+}
+
struct dsa_hw_port {
struct list_head list;
struct net_device *dev;
@@ -1857,8 +1913,6 @@ int dsa_slave_create(struct dsa_port *port)
return -ENOMEM;
slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
- if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
- slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
slave_dev->hw_features |= NETIF_F_HW_TC;
slave_dev->features |= NETIF_F_LLTX;
slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 net 3/3] net: dsa: let drivers state that they need VLAN filtering while standalone
2021-03-20 22:59 [PATCH v2 net 0/3] Clear rx-vlan-filter feature in DSA when necessary Vladimir Oltean
2021-03-20 22:59 ` [PATCH v2 net 1/3] net: dsa: only unset VLAN filtering when last port leaves last VLAN-aware bridge Vladimir Oltean
2021-03-20 22:59 ` [PATCH v2 net 2/3] net: dsa: don't advertise 'rx-vlan-filter' if VLAN filtering not global Vladimir Oltean
@ 2021-03-20 22:59 ` Vladimir Oltean
2 siblings, 0 replies; 10+ messages in thread
From: Vladimir Oltean @ 2021-03-20 22:59 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, netdev
Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, Kurt Kanzenbach,
Tobias Waldekranz, Vladimir Oltean
From: Vladimir Oltean <vladimir.oltean@nxp.com>
As explained in the blamed patch, the hellcreek driver uses some tricks
to comply with the network stack expectations: it enforces port
separation in standalone mode using VLANs. For untagged traffic,
bridging between ports is prevented by using different PVIDs, and for
VLAN-tagged traffic, it never accepts 8021q uppers with the same VID on
two ports, so packets with one VLAN cannot leak from one port to another.
That is almost fine*, and has worked because hellcreek relied on an
implicit behavior of the DSA core that was changed by the previous
patch: the standalone ports declare the 'rx-vlan-filter' feature as 'on
[fixed]'. Since most of the DSA drivers are actually VLAN-unaware in
standalone mode, that feature was actually incorrectly reflecting the
hardware/driver state, so there was a desire to fix it. This leaves the
hellcreek driver in a situation where it has to explicitly request this
behavior from the DSA framework.
We configure the ports as follows:
- Standalone: 'rx-vlan-filter' is on. An 8021q upper on top of a
standalone hellcreek port will go through dsa_slave_vlan_rx_add_vid
and will add a VLAN to the hardware tables, giving the driver the
opportunity to refuse it through .port_prechangeupper.
- Bridged with vlan_filtering=0: 'rx-vlan-filter' is off. An 8021q upper
on top of a bridged hellcreek port will not go through
dsa_slave_vlan_rx_add_vid, because there will not be any attempt to
offload this VLAN. The driver already disables VLAN awareness, so that
upper should receive the traffic it needs.
- Bridged with vlan_filtering=1: 'rx-vlan-filter' is on. An 8021q upper
on top of a bridged hellcreek port will call dsa_slave_vlan_rx_add_vid,
and can again be vetoed through .port_prechangeupper.
*It is not actually completely fine, because if I follow through
correctly, we can have the following situation:
ip link add br0 type bridge vlan_filtering 0
ip link set lan0 master br0 # lan0 now becomes VLAN-unaware
ip link set lan0 nomaster # lan0 fails to become VLAN-aware again, therefore breaking isolation
This patch fixes that by extending the DSA core logic, based on this
requested attribute, to change the VLAN awareness state of the switch
(port) when it leaves the bridge.
Fixes: e358bef7c392 ("net: dsa: Give drivers the chance to veto certain upper devices")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/net/dsa/hirschmann/hellcreek.c | 1 +
include/net/dsa.h | 3 +++
net/dsa/slave.c | 8 ++++++--
net/dsa/switch.c | 20 +++++++++++++++-----
4 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
index 64a73dd045c0..f0f8aad8b5f3 100644
--- a/drivers/net/dsa/hirschmann/hellcreek.c
+++ b/drivers/net/dsa/hirschmann/hellcreek.c
@@ -1327,6 +1327,7 @@ static int hellcreek_setup(struct dsa_switch *ds)
* filtering setups are not supported.
*/
ds->vlan_filtering_is_global = true;
+ ds->needs_standalone_vlan_filtering = true;
/* Intercept _all_ PTP multicast traffic */
ret = hellcreek_setup_fdb(hellcreek);
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 57b2c49f72f4..d5167275d9fd 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -357,6 +357,9 @@ struct dsa_switch {
*/
bool vlan_filtering_is_global;
+ /* Keep VLAN filtering enabled on unbridged ports. */
+ bool needs_standalone_vlan_filtering;
+
/* Pass .port_vlan_add and .port_vlan_del to drivers even for bridges
* that have vlan_filtering=0. All drivers should ideally set this (and
* then the option would get removed), but it is unknown whether this
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 6d06d13cdf3a..55f862050976 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1406,9 +1406,11 @@ static int dsa_slave_clear_vlan(struct net_device *vdev, int vid, void *arg)
*
* - Standalone ports offload:
* - no VLAN (any 8021q upper is a software VLAN) if
- * ds->vlan_filtering_is_global = false
+ * ds->vlan_filtering_is_global = false and
+ * ds->needs_standalone_vlan_filtering = false
* - the 8021q upper VLANs if ds->vlan_filtering_is_global = true and there
- * are bridges spanning this switch chip which have vlan_filtering=1
+ * are bridges spanning this switch chip which have vlan_filtering=1, or
+ * ds->needs_standalone_vlan_filtering = true.
*
* - Ports under a vlan_filtering=0 bridge offload:
* - no VLAN if ds->configure_vlan_while_not_filtering = false (deprecated)
@@ -1914,6 +1916,8 @@ int dsa_slave_create(struct dsa_port *port)
slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
slave_dev->hw_features |= NETIF_F_HW_TC;
+ if (ds->needs_standalone_vlan_filtering)
+ slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
slave_dev->features |= NETIF_F_LLTX;
slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
if (!IS_ERR_OR_NULL(port->mac))
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 32963276452f..8b3a2b846789 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -104,9 +104,10 @@ static int dsa_switch_bridge_join(struct dsa_switch *ds,
static int dsa_switch_bridge_leave(struct dsa_switch *ds,
struct dsa_notifier_bridge_info *info)
{
- bool unset_vlan_filtering = br_vlan_enabled(info->br);
struct dsa_switch_tree *dst = ds->dst;
struct netlink_ext_ack extack = {0};
+ bool change_vlan_filtering = false;
+ bool vlan_filtering;
int err, port;
if (dst->index == info->tree_index && ds->index == info->sw_index &&
@@ -119,6 +120,15 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,
info->sw_index, info->port,
info->br);
+ if (ds->needs_standalone_vlan_filtering && !br_vlan_enabled(info->br)) {
+ change_vlan_filtering = true;
+ vlan_filtering = true;
+ } else if (!ds->needs_standalone_vlan_filtering &&
+ br_vlan_enabled(info->br)) {
+ change_vlan_filtering = true;
+ vlan_filtering = false;
+ }
+
/* If the bridge was vlan_filtering, the bridge core doesn't trigger an
* event for changing vlan_filtering setting upon slave ports leaving
* it. That is a good thing, because that lets us handle it and also
@@ -127,21 +137,21 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,
* vlan_filtering callback is only when the last port leaves the last
* VLAN-aware bridge.
*/
- if (unset_vlan_filtering && ds->vlan_filtering_is_global) {
+ if (change_vlan_filtering && ds->vlan_filtering_is_global) {
for (port = 0; port < ds->num_ports; port++) {
struct net_device *bridge_dev;
bridge_dev = dsa_to_port(ds, port)->bridge_dev;
if (bridge_dev && br_vlan_enabled(bridge_dev)) {
- unset_vlan_filtering = false;
+ change_vlan_filtering = false;
break;
}
}
}
- if (unset_vlan_filtering) {
+ if (change_vlan_filtering) {
err = dsa_port_vlan_filtering(dsa_to_port(ds, info->port),
- false, &extack);
+ vlan_filtering, &extack);
if (extack._msg)
dev_err(ds->dev, "port %d: %s\n", info->port,
extack._msg);
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net 1/3] net: dsa: only unset VLAN filtering when last port leaves last VLAN-aware bridge
2021-03-20 22:59 ` [PATCH v2 net 1/3] net: dsa: only unset VLAN filtering when last port leaves last VLAN-aware bridge Vladimir Oltean
@ 2021-03-22 17:52 ` Tobias Waldekranz
2021-03-22 17:56 ` Vladimir Oltean
0 siblings, 1 reply; 10+ messages in thread
From: Tobias Waldekranz @ 2021-03-22 17:52 UTC (permalink / raw)
To: Vladimir Oltean, David S . Miller, Jakub Kicinski, netdev
Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, Kurt Kanzenbach,
Vladimir Oltean
On Sun, Mar 21, 2021 at 00:59, Vladimir Oltean <olteanv@gmail.com> wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> DSA is aware of switches with global VLAN filtering since the blamed
> commit, but it makes a bad decision when multiple bridges are spanning
> the same switch:
>
> ip link add br0 type bridge vlan_filtering 1
> ip link add br1 type bridge vlan_filtering 1
> ip link set swp2 master br0
> ip link set swp3 master br0
> ip link set swp4 master br1
> ip link set swp5 master br1
> ip link set swp5 nomaster
> ip link set swp4 nomaster
> [138665.939930] sja1105 spi0.1: port 3: dsa_core: VLAN filtering is a global setting
> [138665.947514] DSA: failed to notify DSA_NOTIFIER_BRIDGE_LEAVE
>
> When all ports leave br1, DSA blindly attempts to disable VLAN filtering
> on the switch, ignoring the fact that br0 still exists and is VLAN-aware
> too. It fails while doing that.
>
> This patch checks whether any port exists at all and is under a
> VLAN-aware bridge.
>
> Fixes: d371b7c92d19 ("net: dsa: Unset vlan_filtering when ports leave the bridge")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Tested-by: Florian Fainelli <f.fainelli@gmail.com>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> net/dsa/switch.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> index 4b5da89dc27a..32963276452f 100644
> --- a/net/dsa/switch.c
> +++ b/net/dsa/switch.c
> @@ -107,7 +107,7 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,
> bool unset_vlan_filtering = br_vlan_enabled(info->br);
> struct dsa_switch_tree *dst = ds->dst;
> struct netlink_ext_ack extack = {0};
> - int err, i;
> + int err, port;
>
> if (dst->index == info->tree_index && ds->index == info->sw_index &&
> ds->ops->port_bridge_join)
> @@ -124,13 +124,16 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,
> * it. That is a good thing, because that lets us handle it and also
> * handle the case where the switch's vlan_filtering setting is global
> * (not per port). When that happens, the correct moment to trigger the
> - * vlan_filtering callback is only when the last port left this bridge.
> + * vlan_filtering callback is only when the last port leaves the last
> + * VLAN-aware bridge.
> */
> if (unset_vlan_filtering && ds->vlan_filtering_is_global) {
> - for (i = 0; i < ds->num_ports; i++) {
> - if (i == info->port)
> - continue;
> - if (dsa_to_port(ds, i)->bridge_dev == info->br) {
> + for (port = 0; port < ds->num_ports; port++) {
> + struct net_device *bridge_dev;
> +
> + bridge_dev = dsa_to_port(ds, port)->bridge_dev;
> +
> + if (bridge_dev && br_vlan_enabled(bridge_dev)) {
> unset_vlan_filtering = false;
> break;
> }
> --
> 2.25.1
Is it the case that all devices in which VLAN filtering is a global
setting are also single-chip? To my _D_SA eyes, it feels like we should
have to iterate over all ports in the tree, not just the switch.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net 1/3] net: dsa: only unset VLAN filtering when last port leaves last VLAN-aware bridge
2021-03-22 17:52 ` Tobias Waldekranz
@ 2021-03-22 17:56 ` Vladimir Oltean
0 siblings, 0 replies; 10+ messages in thread
From: Vladimir Oltean @ 2021-03-22 17:56 UTC (permalink / raw)
To: Tobias Waldekranz
Cc: David S . Miller, Jakub Kicinski, netdev, Florian Fainelli,
Andrew Lunn, Vivien Didelot, Kurt Kanzenbach, Vladimir Oltean
On Mon, Mar 22, 2021 at 06:52:58PM +0100, Tobias Waldekranz wrote:
> On Sun, Mar 21, 2021 at 00:59, Vladimir Oltean <olteanv@gmail.com> wrote:
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> >
> > DSA is aware of switches with global VLAN filtering since the blamed
> > commit, but it makes a bad decision when multiple bridges are spanning
> > the same switch:
> >
> > ip link add br0 type bridge vlan_filtering 1
> > ip link add br1 type bridge vlan_filtering 1
> > ip link set swp2 master br0
> > ip link set swp3 master br0
> > ip link set swp4 master br1
> > ip link set swp5 master br1
> > ip link set swp5 nomaster
> > ip link set swp4 nomaster
> > [138665.939930] sja1105 spi0.1: port 3: dsa_core: VLAN filtering is a global setting
> > [138665.947514] DSA: failed to notify DSA_NOTIFIER_BRIDGE_LEAVE
> >
> > When all ports leave br1, DSA blindly attempts to disable VLAN filtering
> > on the switch, ignoring the fact that br0 still exists and is VLAN-aware
> > too. It fails while doing that.
> >
> > This patch checks whether any port exists at all and is under a
> > VLAN-aware bridge.
> >
> > Fixes: d371b7c92d19 ("net: dsa: Unset vlan_filtering when ports leave the bridge")
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > Tested-by: Florian Fainelli <f.fainelli@gmail.com>
> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> > ---
> > net/dsa/switch.c | 15 +++++++++------
> > 1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> > index 4b5da89dc27a..32963276452f 100644
> > --- a/net/dsa/switch.c
> > +++ b/net/dsa/switch.c
> > @@ -107,7 +107,7 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,
> > bool unset_vlan_filtering = br_vlan_enabled(info->br);
> > struct dsa_switch_tree *dst = ds->dst;
> > struct netlink_ext_ack extack = {0};
> > - int err, i;
> > + int err, port;
> >
> > if (dst->index == info->tree_index && ds->index == info->sw_index &&
> > ds->ops->port_bridge_join)
> > @@ -124,13 +124,16 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,
> > * it. That is a good thing, because that lets us handle it and also
> > * handle the case where the switch's vlan_filtering setting is global
> > * (not per port). When that happens, the correct moment to trigger the
> > - * vlan_filtering callback is only when the last port left this bridge.
> > + * vlan_filtering callback is only when the last port leaves the last
> > + * VLAN-aware bridge.
> > */
> > if (unset_vlan_filtering && ds->vlan_filtering_is_global) {
> > - for (i = 0; i < ds->num_ports; i++) {
> > - if (i == info->port)
> > - continue;
> > - if (dsa_to_port(ds, i)->bridge_dev == info->br) {
> > + for (port = 0; port < ds->num_ports; port++) {
> > + struct net_device *bridge_dev;
> > +
> > + bridge_dev = dsa_to_port(ds, port)->bridge_dev;
> > +
> > + if (bridge_dev && br_vlan_enabled(bridge_dev)) {
> > unset_vlan_filtering = false;
> > break;
> > }
> > --
> > 2.25.1
>
> Is it the case that all devices in which VLAN filtering is a global
> setting are also single-chip? To my _D_SA eyes, it feels like we should
> have to iterate over all ports in the tree, not just the switch.
Correct, I might revisit this if I ever get my hands on a board with
sja1105 switches in a real multi-switch tree, and not in disjoint trees
as I have had access to so far.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net 2/3] net: dsa: don't advertise 'rx-vlan-filter' if VLAN filtering not global
2021-03-20 22:59 ` [PATCH v2 net 2/3] net: dsa: don't advertise 'rx-vlan-filter' if VLAN filtering not global Vladimir Oltean
@ 2021-03-23 2:40 ` Florian Fainelli
2021-03-23 12:03 ` Vladimir Oltean
0 siblings, 1 reply; 10+ messages in thread
From: Florian Fainelli @ 2021-03-23 2:40 UTC (permalink / raw)
To: Vladimir Oltean, David S . Miller, Jakub Kicinski, netdev
Cc: Andrew Lunn, Vivien Didelot, Kurt Kanzenbach, Tobias Waldekranz,
Vladimir Oltean
On 3/20/2021 3:59 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> The blamed patch has removed the driver's ability to return -EOPNOTSUPP
> in the .port_vlan_add method when called from .ndo_vlan_rx_add_vid
> (unmassaged by DSA, -EOPNOTSUPP is a hard error for vlan_vid_add).
> But we have not managed well enough the cases under which .port_vlan_add
> is called in the first place, as will be explained below. This was
> reported as a problem by Tobias because mv88e6xxx_port_vlan_prepare is
> stubborn and only accepts VLANs on bridged ports. That is understandably
> so, because standalone mv88e6xxx ports are VLAN-unaware, and VTU entries
> are said to be a scarce resource.
>
> Otherwise said, the following fails lamentably on mv88e6xxx:
>
> ip link add br0 type bridge vlan_filtering 1
> ip link set lan3 master br0
> ip link add link lan10 name lan10.1 type vlan id 1
> [485256.724147] mv88e6085 d0032004.mdio-mii:12: p10: hw VLAN 1 already used by port 3 in br0
> RTNETLINK answers: Operation not supported
>
> We need to step back and explain that the dsa_slave_vlan_rx_add_vid and
> dsa_slave_vlan_rx_kill_vid methods exist for drivers that need the
> 'rx-vlan-filter: on' feature in ethtool -k, which can be due to any of
> the following reasons:
>
> 1. vlan_filtering_is_global = true, and some ports are under a
> VLAN-aware bridge while others are standalone, and the standalone
> ports would otherwise drop VLAN-tagged traffic. This is described in
> commit 061f6a505ac3 ("net: dsa: Add ndo_vlan_rx_{add, kill}_vid
> implementation").
>
> 2. the ports that are under a VLAN-aware bridge should also set this
> feature, for 8021q uppers having a VID not claimed by the bridge.
> In this case, the driver will essentially not even know that the VID
> is coming from the 8021q layer and not the bridge.
>
> 3. Hellcreek. This driver needs it because in standalone mode, it uses
> unique VLANs per port to ensure separation. For separation of untagged
> traffic, it uses different PVIDs for each port, and for separation of
> VLAN-tagged traffic, it never accepts 8021q uppers with the same vid
> on two ports.
>
> If a driver does not fall under any of the above 3 categories, there is
> no reason why it should advertise the 'rx-vlan-filter' feature, therefore
> no reason why it should offload the VLANs added through vlan_vid_add.
>
> This commit fixes the problem by removing the 'rx-vlan-filter' feature
> from the slave devices when they operate in standalone mode, and when
> they offload a VLAN-unaware bridge. This gives the mv88e6xxx driver what
> it wants, since it keeps the 8021q VLANs away from the VTU until VLAN
> awareness is enabled (point at which the ports are no longer standalone,
> hence the check in mv88e6xxx_port_vlan_prepare passes). And since the
> issue predates the existence of the hellcreek driver, case 3 will be
> dealt with in a separate patch.
>
> The commit also has the nice side effect that we no longer lie to the
> network stack about our VLAN filtering status.
>
> Because the 'rx-vlan-filter' feature is now dynamically toggled, and our
> .ndo_vlan_rx_add_vid does not get called when 'rx-vlan-filter' is off,
> we need to avoid bugs such as the following by replaying the VLANs from
> 8021q uppers every time we enable VLAN filtering:
>
> ip link add link lan0 name lan0.100 type vlan id 100
> ip addr add 192.168.100.1/24 dev lan0.100
> ping 192.168.100.2 # should work
> ip link add br0 type bridge vlan_filtering 0
> ip link set lan0 master br0
> ping 192.168.100.2 # should still work
> ip link set br0 type bridge vlan_filtering 1
> ping 192.168.100.2 # should still work but doesn't
That example seems to work well but see caveat below.
# ip link add link gphy name gphy.42 type vlan id 42
# ip addr add 192.168.42.1/24 dev gphy.42
# ping -c 1 192.168.42.254
PING 192.168.42.254 (192.168.42.254): 56 data bytes
64 bytes from 192.168.42.254: seq=0 ttl=64 time=1.473 ms
--- 192.168.42.254 ping statistics ---
1 packets transmitted, 1 packets received, 0% packet loss
round-trip min/avg/max = 1.473/1.473/1.473 ms
# ip link add br0 type bridge vlan_filtering 0
# ip link set br0 up
# ip addr flush dev gphy
# ip link set gphy master br0
[ 102.184169] br0: port 1(gphy) entered blocking state
[ 102.189533] br0: port 1(gphy) entered disabled state
[ 102.196039] device gphy entered promiscuous mode
[ 102.200831] device eth0 entered promiscuous mode
[ 102.206781] brcm-sf2 8f00000.ethernet_switch: Port 0 VLAN enabled: 1,
filtering: 0
[ 102.214684] brcm-sf2 8f00000.ethernet_switch: VID: 1, members:
0x0001, untag: 0x0001
[ 102.228912] brcm-sf2 8f00000.ethernet_switch: Port 8 VLAN enabled: 1,
filtering: 0
[ 102.236736] brcm-sf2 8f00000.ethernet_switch: VID: 1, members:
0x0101, untag: 0x0001
[ 102.248062] br0: port 1(gphy) entered blocking state
[ 102.253210] br0: port 1(gphy) entered forwarding state
# udhcpc -i br0
udhcpc: started, v1.32.0
udhcpc: sending discover
udhcpc: sending select for 192.168.1.10
udhcpc: lease of 192.168.1.10 obtained, lease time 600
deleting routers
adding dns 192.168.1.254
# ping 192.168.42.254
PING 192.168.42.254 (192.168.42.254): 56 data bytes
64 bytes from 192.168.42.254: seq=0 ttl=64 time=1.294 ms
64 bytes from 192.168.42.254: seq=1 ttl=64 time=0.884 ms
^C
--- 192.168.42.254 ping statistics ---
2 packets transmitted, 2 packets received, 0% packet loss
round-trip min/avg/max = 0.884/1.089/1.294 ms
# ip link set br0 type bridge vlan_filtering 1
[ 116.072754] brcm-sf2 8f00000.ethernet_switch: Port 0 VLAN enabled: 1,
filtering: 1
[ 116.080522] brcm-sf2 8f00000.ethernet_switch: Port 0 VLAN enabled: 1,
filtering: 0
[ 116.088211] brcm-sf2 8f00000.ethernet_switch: VID: 42, members:
0x0001, untag: 0x0000
[ 116.098696] brcm-sf2 8f00000.ethernet_switch: Port 8 VLAN enabled: 1,
filtering: 0
[ 116.106474] brcm-sf2 8f00000.ethernet_switch: VID: 42, members:
0x0101, untag: 0x0000
# ping 192.168.42.254
PING 192.168.42.254 (192.168.42.254): 56 data bytes
64 bytes from 192.168.42.254: seq=0 ttl=64 time=0.751 ms
64 bytes from 192.168.42.254: seq=1 ttl=64 time=0.700 ms
^C
--- 192.168.42.254 ping statistics ---
2 packets transmitted, 2 packets received, 0% packet loss
round-trip min/avg/max = 0.700/0.725/0.751 ms
# ping 192.168.1.254
PING 192.168.1.254 (192.168.1.254): 56 data bytes
64 bytes from 192.168.1.254: seq=0 ttl=64 time=0.713 ms
64 bytes from 192.168.1.254: seq=1 ttl=64 time=0.916 ms
64 bytes from 192.168.1.254: seq=2 ttl=64 time=0.631 ms
^C
--- 192.168.1.254 ping statistics ---
3 packets transmitted, 3 packets received, 0% packet loss
round-trip min/avg/max = 0.631/0.753/0.916 ms
But you will notice that vlan filtering was not enabled at the switch
level for a reason I do not fully understand, or rather there were
multiple calls to port_vlan_filtering with vlan_filtering = 0 for the
same port.
Now if we have a nother port that is a member of a bridge that was
created with vlan_filtering=1 from the get go, the standalone ports are
not working if they are created before the bridge is:
# ip link add link gphy name gphy.42 type vlan id 42
# ip addr add 192.168.42.1/24 dev gphy.42
# ping -c 1 192.168.42.254
PING 192.168.42.254 (192.168.42.254): 56 data bytes
64 bytes from 192.168.42.254: seq=0 ttl=64 time=1.129 ms
--- 192.168.42.254 ping statistics ---
1 packets transmitted, 1 packets received, 0% packet loss
round-trip min/avg/max = 1.129/1.129/1.129 ms
# ip link add br0 type bridge vlan_filtering 1
# ip link set rgmii_1 master br0
[ 86.835014] br0: port 1(rgmii_1) entered blocking state
[ 86.840622] br0: port 1(rgmii_1) entered disabled state
[ 86.848084] device rgmii_1 entered promiscuous mode
[ 86.853153] device eth0 entered promiscuous mode
[ 86.858308] brcm-sf2 8f00000.ethernet_switch: Port 1 VLAN enabled: 1,
filtering: 1
[ 86.866157] brcm-sf2 8f00000.ethernet_switch: Port 0 VLAN enabled: 1,
filtering: 0
[ 86.873985] brcm-sf2 8f00000.ethernet_switch: VID: 42, members:
0x0001, untag: 0x0000
[ 86.884946] brcm-sf2 8f00000.ethernet_switch: Port 8 VLAN enabled: 1,
filtering: 0
[ 86.892879] brcm-sf2 8f00000.ethernet_switch: VID: 42, members:
0x0101, untag: 0x0000
[ 86.904274] brcm-sf2 8f00000.ethernet_switch: Port 1 VLAN enabled: 1,
filtering: 1
[ 86.912097] brcm-sf2 8f00000.ethernet_switch: VID: 1, members:
0x0002, untag: 0x0002
[ 86.925899] brcm-sf2 8f00000.ethernet_switch: Port 8 VLAN enabled: 1,
filtering: 1
[ 86.933806] brcm-sf2 8f00000.ethernet_switch: VID: 1, members:
0x0102, untag: 0x0002
# ip link set br0 up
[ 89.775094] br0: port 1(rgmii_1) entered blocking state
[ 89.780694] br0: port 1(rgmii_1) entered forwarding state
# ip addr add 192.168.4.10/24 dev br0
# ping 192.168.4.254
PING 192.168.4.254 (192.168.4.254): 56 data bytes
64 bytes from 192.168.4.254: seq=0 ttl=64 time=1.693 ms
^C
--- 192.168.4.254 ping statistics ---
1 packets transmitted, 1 packets received, 0% packet loss
round-trip min/avg/max = 1.693/1.693/1.693 ms
# ping 192.168.42.254
PING 192.168.42.254 (192.168.42.254): 56 data bytes
^C
--- 192.168.42.254 ping statistics ---
2 packets transmitted, 0 packets received, 100% packet loss
# ping 192.168.1.254
PING 192.168.1.254 (192.168.1.254): 56 data bytes
^C
--- 192.168.1.254 ping statistics ---
1 packets transmitted, 0 packets received, 100% packet loss
#
Both scenarios work correctly as of net/master prior to this patch series.
>
> Fixes: 9b236d2a69da ("net: dsa: Advertise the VLAN offload netdev ability only if switch supports it")
> Reported-by: Tobias Waldekranz <tobias@waldekranz.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> net/dsa/dsa_priv.h | 2 ++
> net/dsa/port.c | 37 +++++++++++++++++++++++++++--
> net/dsa/slave.c | 58 ++++++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 93 insertions(+), 4 deletions(-)
>
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index 4c43c5406834..d7dd9e07d168 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -269,6 +269,8 @@ int dsa_slave_register_notifier(void);
> void dsa_slave_unregister_notifier(void);
> void dsa_slave_setup_tagger(struct net_device *slave);
> int dsa_slave_change_mtu(struct net_device *dev, int new_mtu);
> +int dsa_slave_manage_vlan_filtering(struct net_device *dev,
> + bool vlan_filtering);
>
> static inline struct dsa_port *dsa_slave_to_port(const struct net_device *dev)
> {
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index c9c6d7ab3f47..902095f04e0a 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -363,6 +363,7 @@ static bool dsa_port_can_apply_vlan_filtering(struct dsa_port *dp,
> int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
> struct netlink_ext_ack *extack)
> {
> + bool old_vlan_filtering = dsa_port_is_vlan_filtering(dp);
> struct dsa_switch *ds = dp->ds;
> bool apply;
> int err;
> @@ -388,12 +389,44 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
> if (err)
> return err;
>
> - if (ds->vlan_filtering_is_global)
> + if (ds->vlan_filtering_is_global) {
> + int port;
> +
> + for (port = 0; port < ds->num_ports; port++) {
> + struct net_device *slave;
> +
> + if (!dsa_is_user_port(ds, port))
> + continue;
> +
> + /* We might be called in the unbind path, so not
> + * all slave devices might still be registered.
> + */
> + slave = dsa_to_port(ds, port)->slave;
> + if (!slave)
> + continue;
> +
> + err = dsa_slave_manage_vlan_filtering(slave,
> + vlan_filtering);
> + if (err)
> + goto restore;
> + }
> +
> ds->vlan_filtering = vlan_filtering;
> - else
> + } else {
> + err = dsa_slave_manage_vlan_filtering(dp->slave,
> + vlan_filtering);
> + if (err)
> + goto restore;
> +
> dp->vlan_filtering = vlan_filtering;
> + }
>
> return 0;
> +
> +restore:
> + ds->ops->port_vlan_filtering(ds, dp->index, old_vlan_filtering, NULL);
> +
> + return err;
> }
>
> /* This enforces legacy behavior for switch drivers which assume they can't
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 992fcab4b552..6d06d13cdf3a 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1387,6 +1387,62 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
> return 0;
> }
>
> +static int dsa_slave_restore_vlan(struct net_device *vdev, int vid, void *arg)
> +{
> + __be16 proto = vdev ? vlan_dev_vlan_proto(vdev) : htons(ETH_P_8021Q);
> +
> + return dsa_slave_vlan_rx_add_vid(arg, proto, vid);
> +}
> +
> +static int dsa_slave_clear_vlan(struct net_device *vdev, int vid, void *arg)
> +{
> + __be16 proto = vdev ? vlan_dev_vlan_proto(vdev) : htons(ETH_P_8021Q);
> +
> + return dsa_slave_vlan_rx_kill_vid(arg, proto, vid);
> +}
> +
> +/* Keep the VLAN RX filtering list in sync with the hardware only if VLAN
> + * filtering is enabled.
> + *
> + * - Standalone ports offload:
> + * - no VLAN (any 8021q upper is a software VLAN) if
> + * ds->vlan_filtering_is_global = false
> + * - the 8021q upper VLANs if ds->vlan_filtering_is_global = true and there
> + * are bridges spanning this switch chip which have vlan_filtering=1
> + *
> + * - Ports under a vlan_filtering=0 bridge offload:
> + * - no VLAN if ds->configure_vlan_while_not_filtering = false (deprecated)
> + * - the bridge VLANs if ds->configure_vlan_while_not_filtering = true
> + *
> + * - Ports under a vlan_filtering=1 bridge offload:
> + * - the bridge VLANs
> + * - the 8021q upper VLANs
> + */
> +int dsa_slave_manage_vlan_filtering(struct net_device *slave,
> + bool vlan_filtering)
> +{
> + int err;
> +
> + if (vlan_filtering) {
> + slave->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> +
> + err = vlan_for_each(slave, dsa_slave_restore_vlan, slave);
> + if (err) {
> + vlan_for_each(slave, dsa_slave_clear_vlan, slave);
> + slave->features &= ~NETIF_F_HW_VLAN_CTAG_FILTER;
> + return err;
> + }
> + } else {
> + err = vlan_for_each(slave, dsa_slave_clear_vlan, slave);
> + if (err)
> + return err;
> +
> + slave->features &= ~NETIF_F_HW_VLAN_CTAG_FILTER;
> + }
> +
> + return 0;
> +}
> +
> struct dsa_hw_port {
> struct list_head list;
> struct net_device *dev;
> @@ -1857,8 +1913,6 @@ int dsa_slave_create(struct dsa_port *port)
> return -ENOMEM;
>
> slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
> - if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
> - slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> slave_dev->hw_features |= NETIF_F_HW_TC;
> slave_dev->features |= NETIF_F_LLTX;
> slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
>
--
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net 2/3] net: dsa: don't advertise 'rx-vlan-filter' if VLAN filtering not global
2021-03-23 2:40 ` Florian Fainelli
@ 2021-03-23 12:03 ` Vladimir Oltean
2021-03-23 16:16 ` Florian Fainelli
0 siblings, 1 reply; 10+ messages in thread
From: Vladimir Oltean @ 2021-03-23 12:03 UTC (permalink / raw)
To: Florian Fainelli
Cc: David S . Miller, Jakub Kicinski, netdev, Andrew Lunn,
Vivien Didelot, Kurt Kanzenbach, Tobias Waldekranz,
Vladimir Oltean
On Mon, Mar 22, 2021 at 07:40:27PM -0700, Florian Fainelli wrote:
> On 3/20/2021 3:59 PM, Vladimir Oltean wrote:
> > Because the 'rx-vlan-filter' feature is now dynamically toggled, and our
> > .ndo_vlan_rx_add_vid does not get called when 'rx-vlan-filter' is off,
> > we need to avoid bugs such as the following by replaying the VLANs from
> > 8021q uppers every time we enable VLAN filtering:
> >
> > ip link add link lan0 name lan0.100 type vlan id 100
> > ip addr add 192.168.100.1/24 dev lan0.100
> > ping 192.168.100.2 # should work
> > ip link add br0 type bridge vlan_filtering 0
> > ip link set lan0 master br0
> > ping 192.168.100.2 # should still work
> > ip link set br0 type bridge vlan_filtering 1
> > ping 192.168.100.2 # should still work but doesn't
>
> That example seems to work well but see caveat below.
>
> # ip link add link gphy name gphy.42 type vlan id 42
> # ip addr add 192.168.42.1/24 dev gphy.42
> # ping -c 1 192.168.42.254
> PING 192.168.42.254 (192.168.42.254): 56 data bytes
> 64 bytes from 192.168.42.254: seq=0 ttl=64 time=1.473 ms
>
> --- 192.168.42.254 ping statistics ---
> 1 packets transmitted, 1 packets received, 0% packet loss
> round-trip min/avg/max = 1.473/1.473/1.473 ms
> # ip link add br0 type bridge vlan_filtering 0
> # ip link set br0 up
> # ip addr flush dev gphy
> # ip link set gphy master br0
> [ 102.184169] br0: port 1(gphy) entered blocking state
> [ 102.189533] br0: port 1(gphy) entered disabled state
> [ 102.196039] device gphy entered promiscuous mode
> [ 102.200831] device eth0 entered promiscuous mode
> [ 102.206781] brcm-sf2 8f00000.ethernet_switch: Port 0 VLAN enabled: 1, filtering: 0
> [ 102.214684] brcm-sf2 8f00000.ethernet_switch: VID: 1, members: 0x0001, untag: 0x0001
> [ 102.228912] brcm-sf2 8f00000.ethernet_switch: Port 8 VLAN enabled: 1, filtering: 0
> [ 102.236736] brcm-sf2 8f00000.ethernet_switch: VID: 1, members: 0x0101, untag: 0x0001
> [ 102.248062] br0: port 1(gphy) entered blocking state
> [ 102.253210] br0: port 1(gphy) entered forwarding state
So far so good, the call path below triggers your print for the user
port and the CPU port:
dsa_switch_vlan_add
-> b53_vlan_add
-> b53_vlan_prepare
-> b53_enable_vlan
VLAN 42 is not installed in hardware.
> # udhcpc -i br0
> udhcpc: started, v1.32.0
> udhcpc: sending discover
> udhcpc: sending select for 192.168.1.10
> udhcpc: lease of 192.168.1.10 obtained, lease time 600
> deleting routers
> adding dns 192.168.1.254
> # ping 192.168.42.254
> PING 192.168.42.254 (192.168.42.254): 56 data bytes
> 64 bytes from 192.168.42.254: seq=0 ttl=64 time=1.294 ms
> 64 bytes from 192.168.42.254: seq=1 ttl=64 time=0.884 ms
> ^C
> --- 192.168.42.254 ping statistics ---
> 2 packets transmitted, 2 packets received, 0% packet loss
> round-trip min/avg/max = 0.884/1.089/1.294 ms
> # ip link set br0 type bridge vlan_filtering 1
> [ 116.072754] brcm-sf2 8f00000.ethernet_switch: Port 0 VLAN enabled: 1, filtering: 1
Again, so far so good:
dsa_port_vlan_filtering
-> b53_vlan_filtering
-> b53_enable_vlan(dev->vlan_enabled(was true), filtering(is true))
> [ 116.080522] brcm-sf2 8f00000.ethernet_switch: Port 0 VLAN enabled: 1, filtering: 0
This is where it starts to go downhill. There is a time window inside
dsa_port_vlan_filtering, after we called ds->ops->port_vlan_filtering,
in which we have not yet committed ds->vlan_filtering, yet we still need
to call dsa_slave_manage_vlan_filtering, which may delete or restore
VLANs corresponding to 8021q uppers.
So this happens:
dsa_port_vlan_filtering
-> dsa_slave_manage_vlan_filtering
-> dsa_slave_restore_vlan
-> dsa_switch_vlan_add
-> b53_vlan_add
-> b53_vlan_prepare
-> b53_enable_vlan(vlan_enabled(is true), ds->vlan_filtering(is false because it hasn't been committed yet))
I did not take into account the fact that someone might look in
ds->vlan_filtering in port_vlan_add.
> [ 116.088211] brcm-sf2 8f00000.ethernet_switch: VID: 42, members: 0x0001, untag: 0x0000
> [ 116.098696] brcm-sf2 8f00000.ethernet_switch: Port 8 VLAN enabled: 1, filtering: 0
> [ 116.106474] brcm-sf2 8f00000.ethernet_switch: VID: 42, members: 0x0101, untag: 0x0000
The VLANs are at least restored as expected, it seems.
> # ping 192.168.42.254
> PING 192.168.42.254 (192.168.42.254): 56 data bytes
> 64 bytes from 192.168.42.254: seq=0 ttl=64 time=0.751 ms
> 64 bytes from 192.168.42.254: seq=1 ttl=64 time=0.700 ms
> ^C
> --- 192.168.42.254 ping statistics ---
> 2 packets transmitted, 2 packets received, 0% packet loss
> round-trip min/avg/max = 0.700/0.725/0.751 ms
> # ping 192.168.1.254
> PING 192.168.1.254 (192.168.1.254): 56 data bytes
> 64 bytes from 192.168.1.254: seq=0 ttl=64 time=0.713 ms
> 64 bytes from 192.168.1.254: seq=1 ttl=64 time=0.916 ms
> 64 bytes from 192.168.1.254: seq=2 ttl=64 time=0.631 ms
> ^C
> --- 192.168.1.254 ping statistics ---
> 3 packets transmitted, 3 packets received, 0% packet loss
> round-trip min/avg/max = 0.631/0.753/0.916 ms
>
> But you will notice that vlan filtering was not enabled at the switch
> level for a reason I do not fully understand, or rather there were
> multiple calls to port_vlan_filtering with vlan_filtering = 0 for the
> same port.
>
> Now if we have a nother port that is a member of a bridge that was
> created with vlan_filtering=1 from the get go, the standalone ports are
> not working if they are created before the bridge is:
>
> # ip link add link gphy name gphy.42 type vlan id 42
VLAN filtering is not enabled, so the VLAN is not installed to hardware,
all ok.
> # ip addr add 192.168.42.1/24 dev gphy.42
> # ping -c 1 192.168.42.254
> PING 192.168.42.254 (192.168.42.254): 56 data bytes
> 64 bytes from 192.168.42.254: seq=0 ttl=64 time=1.129 ms
>
> --- 192.168.42.254 ping statistics ---
> 1 packets transmitted, 1 packets received, 0% packet loss
> round-trip min/avg/max = 1.129/1.129/1.129 ms
> # ip link add br0 type bridge vlan_filtering 1
> # ip link set rgmii_1 master br0
> [ 86.835014] br0: port 1(rgmii_1) entered blocking state
> [ 86.840622] br0: port 1(rgmii_1) entered disabled state
> [ 86.848084] device rgmii_1 entered promiscuous mode
> [ 86.853153] device eth0 entered promiscuous mode
> [ 86.858308] brcm-sf2 8f00000.ethernet_switch: Port 1 VLAN enabled: 1, filtering: 1
So far so good, we have this same code path again:
dsa_port_vlan_filtering
-> b53_vlan_filtering
-> b53_enable_vlan(dev->vlan_enabled(was true), filtering(is true))
> [ 86.866157] brcm-sf2 8f00000.ethernet_switch: Port 0 VLAN enabled: 1, filtering: 0
> [ 86.873985] brcm-sf2 8f00000.ethernet_switch: VID: 42, members: 0x0001, untag: 0x0000
> [ 86.884946] brcm-sf2 8f00000.ethernet_switch: Port 8 VLAN enabled: 1, filtering: 0
> [ 86.892879] brcm-sf2 8f00000.ethernet_switch: VID: 42, members: 0x0101, untag: 0x0000
Again, we have the same code path that calls dsa_slave_manage_vlan_filtering
while ds->vlan_filtering is still uncommitted, and therefore false. The
b53 driver incorrectly saves this value.
> [ 86.904274] brcm-sf2 8f00000.ethernet_switch: Port 1 VLAN enabled: 1, filtering: 1
> [ 86.912097] brcm-sf2 8f00000.ethernet_switch: VID: 1, members: 0x0002, untag: 0x0002
> [ 86.925899] brcm-sf2 8f00000.ethernet_switch: Port 8 VLAN enabled: 1, filtering: 1
> [ 86.933806] brcm-sf2 8f00000.ethernet_switch: VID: 1, members: 0x0102, untag: 0x0002
And here we have the bridge pvid installed on the user port and the CPU
port. Since ds->vlan_filtering has been committed in the meantime,
b53_vlan_enable was called again and filtering is now enabled.
> # ip link set br0 up
> [ 89.775094] br0: port 1(rgmii_1) entered blocking state
> [ 89.780694] br0: port 1(rgmii_1) entered forwarding state
> # ip addr add 192.168.4.10/24 dev br0
> # ping 192.168.4.254
> PING 192.168.4.254 (192.168.4.254): 56 data bytes
> 64 bytes from 192.168.4.254: seq=0 ttl=64 time=1.693 ms
> ^C
> --- 192.168.4.254 ping statistics ---
> 1 packets transmitted, 1 packets received, 0% packet loss
> round-trip min/avg/max = 1.693/1.693/1.693 ms
Pinging through the VLAN-aware bridge, which uses VID 1, works, ok.
> # ping 192.168.42.254
> PING 192.168.42.254 (192.168.42.254): 56 data bytes
> ^C
> --- 192.168.42.254 ping statistics ---
> 2 packets transmitted, 0 packets received, 100% packet loss
Pinging through gphy.42 doesn't work, even though VID 42 was added both
to port 8 and to port 0. I don't understand why. I looked at the b53
driver and I don't see any logic that would skip installing a VLAN if
ds->vlan_filtering is false.
> # ping 192.168.1.254
> PING 192.168.1.254 (192.168.1.254): 56 data bytes
> ^C
> --- 192.168.1.254 ping statistics ---
> 1 packets transmitted, 0 packets received, 100% packet loss
> #
Wait a minute, what interface uses the 192.168.1.0/24 subnet in the
second case?
>
> Both scenarios work correctly as of net/master prior to this patch series.
So I have no complete idea why it fails either. I do believe DSA does
the right things, for the most part.
Would you be so kind to try this fixup patch on top?
-----------------------------[ cut here ]-----------------------------
From ddca5c56fbf74764977df70c5eba88015bb9832f Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Tue, 23 Mar 2021 13:56:24 +0200
Subject: [PATCH] net: dsa: commit vlan_filtering before calling
dsa_slave_manage_vlan_filtering
Some drivers such as b53 look at ds->vlan_filtering in .port_vlan_add.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/port.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 902095f04e0a..d291e0495084 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -392,6 +392,8 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
if (ds->vlan_filtering_is_global) {
int port;
+ ds->vlan_filtering = vlan_filtering;
+
for (port = 0; port < ds->num_ports; port++) {
struct net_device *slave;
@@ -410,15 +412,13 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
if (err)
goto restore;
}
-
- ds->vlan_filtering = vlan_filtering;
} else {
+ dp->vlan_filtering = vlan_filtering;
+
err = dsa_slave_manage_vlan_filtering(dp->slave,
vlan_filtering);
if (err)
goto restore;
-
- dp->vlan_filtering = vlan_filtering;
}
return 0;
@@ -426,6 +426,11 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
restore:
ds->ops->port_vlan_filtering(ds, dp->index, old_vlan_filtering, NULL);
+ if (ds->vlan_filtering_is_global)
+ ds->vlan_filtering = old_vlan_filtering;
+ else
+ dp->vlan_filtering = old_vlan_filtering;
+
return err;
}
-----------------------------[ cut here ]-----------------------------
Although I am much less confident now about submitting this as a bugfix
patch to go to stable trees. But I also kind of dislike the idea that
Tobias' patch (which returns -EOPNOTSUPP in dsa_slave_vlan_rx_add_vid)
only masks the problem and makes issues harder to reproduce.
Tobias, how bad is your problem? Do you mind if we tackle it in net-next?
Also, again, any chance you could make mv88e6xxx not refuse the 8021q
VLAN IDs?
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net 2/3] net: dsa: don't advertise 'rx-vlan-filter' if VLAN filtering not global
2021-03-23 12:03 ` Vladimir Oltean
@ 2021-03-23 16:16 ` Florian Fainelli
2021-03-23 19:33 ` Vladimir Oltean
0 siblings, 1 reply; 10+ messages in thread
From: Florian Fainelli @ 2021-03-23 16:16 UTC (permalink / raw)
To: Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev, Andrew Lunn,
Vivien Didelot, Kurt Kanzenbach, Tobias Waldekranz,
Vladimir Oltean
On 3/23/2021 5:03 AM, Vladimir Oltean wrote:
> On Mon, Mar 22, 2021 at 07:40:27PM -0700, Florian Fainelli wrote:
>> On 3/20/2021 3:59 PM, Vladimir Oltean wrote:
>>> Because the 'rx-vlan-filter' feature is now dynamically toggled, and our
>>> .ndo_vlan_rx_add_vid does not get called when 'rx-vlan-filter' is off,
>>> we need to avoid bugs such as the following by replaying the VLANs from
>>> 8021q uppers every time we enable VLAN filtering:
>>>
>>> ip link add link lan0 name lan0.100 type vlan id 100
>>> ip addr add 192.168.100.1/24 dev lan0.100
>>> ping 192.168.100.2 # should work
>>> ip link add br0 type bridge vlan_filtering 0
>>> ip link set lan0 master br0
>>> ping 192.168.100.2 # should still work
>>> ip link set br0 type bridge vlan_filtering 1
>>> ping 192.168.100.2 # should still work but doesn't
>>
>> That example seems to work well but see caveat below.
>>
>> # ip link add link gphy name gphy.42 type vlan id 42
>> # ip addr add 192.168.42.1/24 dev gphy.42
>> # ping -c 1 192.168.42.254
>> PING 192.168.42.254 (192.168.42.254): 56 data bytes
>> 64 bytes from 192.168.42.254: seq=0 ttl=64 time=1.473 ms
>>
>> --- 192.168.42.254 ping statistics ---
>> 1 packets transmitted, 1 packets received, 0% packet loss
>> round-trip min/avg/max = 1.473/1.473/1.473 ms
>> # ip link add br0 type bridge vlan_filtering 0
>> # ip link set br0 up
>> # ip addr flush dev gphy
>> # ip link set gphy master br0
>> [ 102.184169] br0: port 1(gphy) entered blocking state
>> [ 102.189533] br0: port 1(gphy) entered disabled state
>> [ 102.196039] device gphy entered promiscuous mode
>> [ 102.200831] device eth0 entered promiscuous mode
>> [ 102.206781] brcm-sf2 8f00000.ethernet_switch: Port 0 VLAN enabled: 1, filtering: 0
>> [ 102.214684] brcm-sf2 8f00000.ethernet_switch: VID: 1, members: 0x0001, untag: 0x0001
>> [ 102.228912] brcm-sf2 8f00000.ethernet_switch: Port 8 VLAN enabled: 1, filtering: 0
>> [ 102.236736] brcm-sf2 8f00000.ethernet_switch: VID: 1, members: 0x0101, untag: 0x0001
>> [ 102.248062] br0: port 1(gphy) entered blocking state
>> [ 102.253210] br0: port 1(gphy) entered forwarding state
>
> So far so good, the call path below triggers your print for the user
> port and the CPU port:
> dsa_switch_vlan_add
> -> b53_vlan_add
> -> b53_vlan_prepare
> -> b53_enable_vlan
> VLAN 42 is not installed in hardware.
>
>> # udhcpc -i br0
>> udhcpc: started, v1.32.0
>> udhcpc: sending discover
>> udhcpc: sending select for 192.168.1.10
>> udhcpc: lease of 192.168.1.10 obtained, lease time 600
>> deleting routers
>> adding dns 192.168.1.254
>> # ping 192.168.42.254
>> PING 192.168.42.254 (192.168.42.254): 56 data bytes
>> 64 bytes from 192.168.42.254: seq=0 ttl=64 time=1.294 ms
>> 64 bytes from 192.168.42.254: seq=1 ttl=64 time=0.884 ms
>> ^C
>> --- 192.168.42.254 ping statistics ---
>> 2 packets transmitted, 2 packets received, 0% packet loss
>> round-trip min/avg/max = 0.884/1.089/1.294 ms
>> # ip link set br0 type bridge vlan_filtering 1
>> [ 116.072754] brcm-sf2 8f00000.ethernet_switch: Port 0 VLAN enabled: 1, filtering: 1
>
> Again, so far so good:
> dsa_port_vlan_filtering
> -> b53_vlan_filtering
> -> b53_enable_vlan(dev->vlan_enabled(was true), filtering(is true))
>
>> [ 116.080522] brcm-sf2 8f00000.ethernet_switch: Port 0 VLAN enabled: 1, filtering: 0
>
> This is where it starts to go downhill. There is a time window inside
> dsa_port_vlan_filtering, after we called ds->ops->port_vlan_filtering,
> in which we have not yet committed ds->vlan_filtering, yet we still need
> to call dsa_slave_manage_vlan_filtering, which may delete or restore
> VLANs corresponding to 8021q uppers.
>
> So this happens:
> dsa_port_vlan_filtering
> -> dsa_slave_manage_vlan_filtering
> -> dsa_slave_restore_vlan
> -> dsa_switch_vlan_add
> -> b53_vlan_add
> -> b53_vlan_prepare
> -> b53_enable_vlan(vlan_enabled(is true), ds->vlan_filtering(is false because it hasn't been committed yet))
>
> I did not take into account the fact that someone might look in
> ds->vlan_filtering in port_vlan_add.
>
>> [ 116.088211] brcm-sf2 8f00000.ethernet_switch: VID: 42, members: 0x0001, untag: 0x0000
>> [ 116.098696] brcm-sf2 8f00000.ethernet_switch: Port 8 VLAN enabled: 1, filtering: 0
>> [ 116.106474] brcm-sf2 8f00000.ethernet_switch: VID: 42, members: 0x0101, untag: 0x0000
>
> The VLANs are at least restored as expected, it seems.
>
>> # ping 192.168.42.254
>> PING 192.168.42.254 (192.168.42.254): 56 data bytes
>> 64 bytes from 192.168.42.254: seq=0 ttl=64 time=0.751 ms
>> 64 bytes from 192.168.42.254: seq=1 ttl=64 time=0.700 ms
>> ^C
>> --- 192.168.42.254 ping statistics ---
>> 2 packets transmitted, 2 packets received, 0% packet loss
>> round-trip min/avg/max = 0.700/0.725/0.751 ms
>> # ping 192.168.1.254
>> PING 192.168.1.254 (192.168.1.254): 56 data bytes
>> 64 bytes from 192.168.1.254: seq=0 ttl=64 time=0.713 ms
>> 64 bytes from 192.168.1.254: seq=1 ttl=64 time=0.916 ms
>> 64 bytes from 192.168.1.254: seq=2 ttl=64 time=0.631 ms
>> ^C
>> --- 192.168.1.254 ping statistics ---
>> 3 packets transmitted, 3 packets received, 0% packet loss
>> round-trip min/avg/max = 0.631/0.753/0.916 ms
>>
>> But you will notice that vlan filtering was not enabled at the switch
>> level for a reason I do not fully understand, or rather there were
>> multiple calls to port_vlan_filtering with vlan_filtering = 0 for the
>> same port.
>>
>> Now if we have a nother port that is a member of a bridge that was
>> created with vlan_filtering=1 from the get go, the standalone ports are
>> not working if they are created before the bridge is:
>>
>> # ip link add link gphy name gphy.42 type vlan id 42
>
> VLAN filtering is not enabled, so the VLAN is not installed to hardware,
> all ok.
>
>> # ip addr add 192.168.42.1/24 dev gphy.42
>> # ping -c 1 192.168.42.254
>> PING 192.168.42.254 (192.168.42.254): 56 data bytes
>> 64 bytes from 192.168.42.254: seq=0 ttl=64 time=1.129 ms
>>
>> --- 192.168.42.254 ping statistics ---
>> 1 packets transmitted, 1 packets received, 0% packet loss
>> round-trip min/avg/max = 1.129/1.129/1.129 ms
>> # ip link add br0 type bridge vlan_filtering 1
>> # ip link set rgmii_1 master br0
>> [ 86.835014] br0: port 1(rgmii_1) entered blocking state
>> [ 86.840622] br0: port 1(rgmii_1) entered disabled state
>> [ 86.848084] device rgmii_1 entered promiscuous mode
>> [ 86.853153] device eth0 entered promiscuous mode
>> [ 86.858308] brcm-sf2 8f00000.ethernet_switch: Port 1 VLAN enabled: 1, filtering: 1
>
> So far so good, we have this same code path again:
>
> dsa_port_vlan_filtering
> -> b53_vlan_filtering
> -> b53_enable_vlan(dev->vlan_enabled(was true), filtering(is true))
>
>> [ 86.866157] brcm-sf2 8f00000.ethernet_switch: Port 0 VLAN enabled: 1, filtering: 0
>> [ 86.873985] brcm-sf2 8f00000.ethernet_switch: VID: 42, members: 0x0001, untag: 0x0000
>> [ 86.884946] brcm-sf2 8f00000.ethernet_switch: Port 8 VLAN enabled: 1, filtering: 0
>> [ 86.892879] brcm-sf2 8f00000.ethernet_switch: VID: 42, members: 0x0101, untag: 0x0000
>
> Again, we have the same code path that calls dsa_slave_manage_vlan_filtering
> while ds->vlan_filtering is still uncommitted, and therefore false. The
> b53 driver incorrectly saves this value.
>
>> [ 86.904274] brcm-sf2 8f00000.ethernet_switch: Port 1 VLAN enabled: 1, filtering: 1
>> [ 86.912097] brcm-sf2 8f00000.ethernet_switch: VID: 1, members: 0x0002, untag: 0x0002
>> [ 86.925899] brcm-sf2 8f00000.ethernet_switch: Port 8 VLAN enabled: 1, filtering: 1
>> [ 86.933806] brcm-sf2 8f00000.ethernet_switch: VID: 1, members: 0x0102, untag: 0x0002
>
> And here we have the bridge pvid installed on the user port and the CPU
> port. Since ds->vlan_filtering has been committed in the meantime,
> b53_vlan_enable was called again and filtering is now enabled.
>
>> # ip link set br0 up
>> [ 89.775094] br0: port 1(rgmii_1) entered blocking state
>> [ 89.780694] br0: port 1(rgmii_1) entered forwarding state
>> # ip addr add 192.168.4.10/24 dev br0
>> # ping 192.168.4.254
>> PING 192.168.4.254 (192.168.4.254): 56 data bytes
>> 64 bytes from 192.168.4.254: seq=0 ttl=64 time=1.693 ms
>> ^C
>> --- 192.168.4.254 ping statistics ---
>> 1 packets transmitted, 1 packets received, 0% packet loss
>> round-trip min/avg/max = 1.693/1.693/1.693 ms
>
> Pinging through the VLAN-aware bridge, which uses VID 1, works, ok.
>
>> # ping 192.168.42.254
>> PING 192.168.42.254 (192.168.42.254): 56 data bytes
>> ^C
>> --- 192.168.42.254 ping statistics ---
>> 2 packets transmitted, 0 packets received, 100% packet loss
>
> Pinging through gphy.42 doesn't work, even though VID 42 was added both
> to port 8 and to port 0. I don't understand why. I looked at the b53
> driver and I don't see any logic that would skip installing a VLAN if
> ds->vlan_filtering is false.
>
>> # ping 192.168.1.254
>> PING 192.168.1.254 (192.168.1.254): 56 data bytes
>> ^C
>> --- 192.168.1.254 ping statistics ---
>> 1 packets transmitted, 0 packets received, 100% packet loss
>> #
>
> Wait a minute, what interface uses the 192.168.1.0/24 subnet in the
> second case?
In the second case it is gphy.
>
>>
>> Both scenarios work correctly as of net/master prior to this patch series.
>
> So I have no complete idea why it fails either. I do believe DSA does
> the right things, for the most part.
>
> Would you be so kind to try this fixup patch on top?
That works for me, thank you! So for the whole patch when you resend,
you can add:
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
>
> -----------------------------[ cut here ]-----------------------------
> From ddca5c56fbf74764977df70c5eba88015bb9832f Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Tue, 23 Mar 2021 13:56:24 +0200
> Subject: [PATCH] net: dsa: commit vlan_filtering before calling
> dsa_slave_manage_vlan_filtering
>
> Some drivers such as b53 look at ds->vlan_filtering in .port_vlan_add.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> net/dsa/port.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index 902095f04e0a..d291e0495084 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -392,6 +392,8 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
> if (ds->vlan_filtering_is_global) {
> int port;
>
> + ds->vlan_filtering = vlan_filtering;
> +
> for (port = 0; port < ds->num_ports; port++) {
> struct net_device *slave;
>
> @@ -410,15 +412,13 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
> if (err)
> goto restore;
> }
> -
> - ds->vlan_filtering = vlan_filtering;
> } else {
> + dp->vlan_filtering = vlan_filtering;
> +
> err = dsa_slave_manage_vlan_filtering(dp->slave,
> vlan_filtering);
> if (err)
> goto restore;
> -
> - dp->vlan_filtering = vlan_filtering;
> }
>
> return 0;
> @@ -426,6 +426,11 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
> restore:
> ds->ops->port_vlan_filtering(ds, dp->index, old_vlan_filtering, NULL);
>
> + if (ds->vlan_filtering_is_global)
> + ds->vlan_filtering = old_vlan_filtering;
> + else
> + dp->vlan_filtering = old_vlan_filtering;
> +
> return err;
> }
>
> -----------------------------[ cut here ]-----------------------------
>
> Although I am much less confident now about submitting this as a bugfix
> patch to go to stable trees. But I also kind of dislike the idea that
> Tobias' patch (which returns -EOPNOTSUPP in dsa_slave_vlan_rx_add_vid)
> only masks the problem and makes issues harder to reproduce.
>
> Tobias, how bad is your problem? Do you mind if we tackle it in net-next?
> Also, again, any chance you could make mv88e6xxx not refuse the 8021q
> VLAN IDs?
I was thinking the same last night while sending my results, as far as I
can tell the switches that have global VLAN filtering or hellcreek are
not broken currently right?
If only mv88e6xxx seems to be requiring special treatment, how do we
feel about adding an argument to port_vlan_add() and port_vlan_del()
that tell us the context in which they are called, that is via 802.1q
upper, or via bridge and have mv88e6xxx ignore the former but not the
latter?
--
Florian
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 net 2/3] net: dsa: don't advertise 'rx-vlan-filter' if VLAN filtering not global
2021-03-23 16:16 ` Florian Fainelli
@ 2021-03-23 19:33 ` Vladimir Oltean
0 siblings, 0 replies; 10+ messages in thread
From: Vladimir Oltean @ 2021-03-23 19:33 UTC (permalink / raw)
To: Florian Fainelli
Cc: David S . Miller, Jakub Kicinski, netdev, Andrew Lunn,
Vivien Didelot, Kurt Kanzenbach, Tobias Waldekranz,
Vladimir Oltean
On Tue, Mar 23, 2021 at 09:16:03AM -0700, Florian Fainelli wrote:
> > Would you be so kind to try this fixup patch on top?
>
> That works for me, thank you! So for the whole patch when you resend,
> you can add:
>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Tested-by: Florian Fainelli <f.fainelli@gmail.com>
Thanks again for testing!
> > Although I am much less confident now about submitting this as a bugfix
> > patch to go to stable trees. But I also kind of dislike the idea that
> > Tobias' patch (which returns -EOPNOTSUPP in dsa_slave_vlan_rx_add_vid)
> > only masks the problem and makes issues harder to reproduce.
> >
> > Tobias, how bad is your problem? Do you mind if we tackle it in net-next?
> > Also, again, any chance you could make mv88e6xxx not refuse the 8021q
> > VLAN IDs?
>
> I was thinking the same last night while sending my results, as far as I
> can tell the switches that have global VLAN filtering or hellcreek are
> not broken currently right?
Yes.
> If only mv88e6xxx seems to be requiring special treatment, how do we
> feel about adding an argument to port_vlan_add() and port_vlan_del()
> that tell us the context in which they are called, that is via 802.1q
> upper, or via bridge and have mv88e6xxx ignore the former but not the
> latter?
How would you then describe to .port_vlan_add() those VLANs that don't
come either from the bridge nor from 8021q uppers, but from direct calls
to vlan_vid_add? A VLAN is a VLAN, and a driver with
configure_vlan_while_not_filtering should accept it.
If mv88e6xxx refuses this right away:
ip link add link lan0 name lan0.100 type vlan id 100
Then traffic through lan0.100 will be broken as soon as we do:
ip link add br0 type bridge vlan_filtering 1
ip link set lan0 master br0
So I believe we should be looking at how to make the Marvell driver
accept the VLAN, not how to help it refuse it in other ways.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-03-23 19:34 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-20 22:59 [PATCH v2 net 0/3] Clear rx-vlan-filter feature in DSA when necessary Vladimir Oltean
2021-03-20 22:59 ` [PATCH v2 net 1/3] net: dsa: only unset VLAN filtering when last port leaves last VLAN-aware bridge Vladimir Oltean
2021-03-22 17:52 ` Tobias Waldekranz
2021-03-22 17:56 ` Vladimir Oltean
2021-03-20 22:59 ` [PATCH v2 net 2/3] net: dsa: don't advertise 'rx-vlan-filter' if VLAN filtering not global Vladimir Oltean
2021-03-23 2:40 ` Florian Fainelli
2021-03-23 12:03 ` Vladimir Oltean
2021-03-23 16:16 ` Florian Fainelli
2021-03-23 19:33 ` Vladimir Oltean
2021-03-20 22:59 ` [PATCH v2 net 3/3] net: dsa: let drivers state that they need VLAN filtering while standalone Vladimir Oltean
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.