[PATCH net-next 0/4] Software fallback for bridging in DSA

[PATCH net-next 0/4] Software fallback for bridging in DSA

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

As was discussed here:
https://patchwork.kernel.org/project/netdevbpf/patch/20201202091356.24075-3-tobias@waldekranz.com/

it is desirable to not reject a LAG interface (bonding, team) even if
the switch isn't able to offload bridging towards that link aggregation
group. At least the DSA setups I have are not that unbalanced between
the horsepower of the CPU and the horsepower of the switch such that
software forwarding to be completely impractical.

This series makes all switch drivers theoretically able to do the right
thing when they are configured in a way similar to this (credits to
Tobias Waldekranz for the drawing):

      br0
     /   \
  team0   \
   / \     \
swp0 swp1  swp2

although in practice there is one more prerequisite: for software
fallback mode, they need to disable address learning. It is preferable
that they do this by implementing the .port_pre_bridge_join and
.port_bridge_join methods.

Vladimir Oltean (4):
  net: dsa: don't offload switchdev objects on ports that don't offload
    the bridge
  net: dsa: reject switchdev objects centrally from
    dsa_slave_port_obj_{add,del}
  net: dsa: return -EOPNOTSUPP if .port_lag_join is not implemented
  net: dsa: don't set skb->offload_fwd_mark when not offloading the
    bridge

 net/dsa/dsa_priv.h         | 16 ++++++++++++++++
 net/dsa/slave.c            | 21 +++++++++++----------
 net/dsa/switch.c           | 13 ++++++++++---
 net/dsa/tag_brcm.c         |  2 +-
 net/dsa/tag_dsa.c          |  9 +++++----
 net/dsa/tag_hellcreek.c    |  2 +-
 net/dsa/tag_ksz.c          |  2 +-
 net/dsa/tag_lan9303.c      |  4 +++-
 net/dsa/tag_mtk.c          |  2 +-
 net/dsa/tag_ocelot.c       |  2 +-
 net/dsa/tag_ocelot_8021q.c |  2 +-
 net/dsa/tag_rtl4_a.c       |  2 +-
 net/dsa/tag_sja1105.c      |  4 ++--
 net/dsa/tag_xrs700x.c      |  3 +--
 14 files changed, 55 insertions(+), 29 deletions(-)

-- 
2.25.1

[PATCH net-next 1/4] net: dsa: don't offload switchdev objects on ports that don't offload the bridge

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Starting with commit 058102a6e9eb ("net: dsa: Link aggregation support"),
DSA warns that certain configurations of upper interfaces are not offloaded
to hardware. When a DSA port does not offload a LAG interface, the
dp->lag_dev pointer is always NULL. However the same cannot be said about
offloading a bridge: dp->bridge_dev will get populated regardless of
whether the driver can put the port into the bridge's forwarding domain
or not.

Instead of silently returning 0 if the driver doesn't implement
.port_bridge_join, return -EOPNOTSUPP instead, and print a message via
netlink extack that the configuration was not offloaded to hardware.

Now we can use the check whether dp->bridge_dev is NULL in order to
avoid offloading at all switchdev attributes and objects for ports that
don't even offload the basic operation of switching. Those can still do
the required L2 forwarding using the bridge software datapath, but
enabling any hardware features specific to the bridge such as address
learning would just ask for problems.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa_priv.h | 2 ++
 net/dsa/slave.c    | 5 +++++
 net/dsa/switch.c   | 7 +++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index f5949b39f6f7..7b0dd2d5f3f8 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -205,6 +205,8 @@ static inline bool dsa_port_offloads_netdev(struct dsa_port *dp,
 					    struct net_device *dev)
 {
 	/* Switchdev offloading can be configured on: */
+	if (!dp->bridge_dev)
+		return false;
 
 	if (dev == dp->slave)
 		/* DSA ports directly connected to a bridge, and event
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 8c9a41a7209a..94bce3596eb6 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1922,6 +1922,11 @@ static int dsa_slave_changeupper(struct net_device *dev,
 			err = dsa_port_bridge_join(dp, info->upper_dev);
 			if (!err)
 				dsa_bridge_mtu_normalization(dp);
+			if (err == -EOPNOTSUPP) {
+				NL_SET_ERR_MSG_MOD(info->info.extack,
+						   "Offloading not supported");
+				err = 0;
+			}
 			err = notifier_from_errno(err);
 		} else {
 			dsa_port_bridge_leave(dp, info->upper_dev);
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 1906179e59f7..4137716d0de5 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -88,9 +88,12 @@ static int dsa_switch_bridge_join(struct dsa_switch *ds,
 {
 	struct dsa_switch_tree *dst = ds->dst;
 
-	if (dst->index == info->tree_index && ds->index == info->sw_index &&
-	    ds->ops->port_bridge_join)
+	if (dst->index == info->tree_index && ds->index == info->sw_index) {
+		if (!ds->ops->port_bridge_join)
+			return -EOPNOTSUPP;
+
 		return ds->ops->port_bridge_join(ds, info->port, info->br);
+	}
 
 	if ((dst->index != info->tree_index || ds->index != info->sw_index) &&
 	    ds->ops->crosschip_bridge_join)
-- 
2.25.1

[PATCH net-next 2/4] net: dsa: reject switchdev objects centrally from dsa_slave_port_obj_{add,del}

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The dsa_port_offloads_netdev check is inside dsa_slave_vlan_{add,del},
but outside dsa_port_mdb_{add,del}. We can reduce the number of
occurrences of dsa_port_offloads_netdev by checking only once, at the
beginning of dsa_slave_port_obj_add and dsa_slave_port_obj_del.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/slave.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 94bce3596eb6..8768b213c6e0 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -340,9 +340,6 @@ static int dsa_slave_vlan_add(struct net_device *dev,
 	struct switchdev_obj_port_vlan vlan;
 	int err;
 
-	if (!dsa_port_offloads_netdev(dp, obj->orig_dev))
-		return -EOPNOTSUPP;
-
 	if (dsa_port_skip_vlan_configuration(dp)) {
 		NL_SET_ERR_MSG_MOD(extack, "skipping configuration of VLAN");
 		return 0;
@@ -385,10 +382,11 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	int err;
 
+	if (!dsa_port_offloads_netdev(dp, obj->orig_dev))
+		return -EOPNOTSUPP;
+
 	switch (obj->id) {
 	case SWITCHDEV_OBJ_ID_PORT_MDB:
-		if (!dsa_port_offloads_netdev(dp, obj->orig_dev))
-			return -EOPNOTSUPP;
 		err = dsa_port_mdb_add(dp, SWITCHDEV_OBJ_PORT_MDB(obj));
 		break;
 	case SWITCHDEV_OBJ_ID_HOST_MDB:
@@ -416,9 +414,6 @@ static int dsa_slave_vlan_del(struct net_device *dev,
 	struct switchdev_obj_port_vlan *vlan;
 	int err;
 
-	if (!dsa_port_offloads_netdev(dp, obj->orig_dev))
-		return -EOPNOTSUPP;
-
 	if (dsa_port_skip_vlan_configuration(dp))
 		return 0;
 
@@ -442,10 +437,11 @@ static int dsa_slave_port_obj_del(struct net_device *dev,
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	int err;
 
+	if (!dsa_port_offloads_netdev(dp, obj->orig_dev))
+		return -EOPNOTSUPP;
+
 	switch (obj->id) {
 	case SWITCHDEV_OBJ_ID_PORT_MDB:
-		if (!dsa_port_offloads_netdev(dp, obj->orig_dev))
-			return -EOPNOTSUPP;
 		err = dsa_port_mdb_del(dp, SWITCHDEV_OBJ_PORT_MDB(obj));
 		break;
 	case SWITCHDEV_OBJ_ID_HOST_MDB:
-- 
2.25.1

[PATCH net-next 3/4] net: dsa: return -EOPNOTSUPP if .port_lag_join is not implemented

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

There is currently some provisioning for DSA to use the software
fallback for link aggregation, but only if the .port_lag_join is
implemented but it fails (for example because there are more link
aggregation groups than the switch supports, or because the xmit hash
policy cannot be done in hardware, or ...).

But when .port_lag_join is not implemented at all, the DSA switch
notifier returns zero and software fallback does not kick in.
Change that.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/switch.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 4137716d0de5..15b0c936ba01 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -203,9 +203,13 @@ static int dsa_switch_lag_change(struct dsa_switch *ds,
 static int dsa_switch_lag_join(struct dsa_switch *ds,
 			       struct dsa_notifier_lag_info *info)
 {
-	if (ds->index == info->sw_index && ds->ops->port_lag_join)
+	if (ds->index == info->sw_index) {
+		if (!ds->ops->port_lag_join)
+			return -EOPNOTSUPP;
+
 		return ds->ops->port_lag_join(ds, info->port, info->lag,
 					      info->info);
+	}
 
 	if (ds->index != info->sw_index && ds->ops->crosschip_lag_join)
 		return ds->ops->crosschip_lag_join(ds, info->sw_index,
-- 
2.25.1

[PATCH net-next 4/4] net: dsa: don't set skb->offload_fwd_mark when not offloading the bridge

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

DSA has gained the recent ability to deal gracefully with upper
interfaces it cannot offload, such as the bridge, bonding or team
drivers. When such uppers exist, the ports are still in standalone mode
as far as the hardware is concerned.

But when we deliver packets to the software bridge in order for that to
do the forwarding, there is an unpleasant surprise in that the bridge
will refuse to forward them. This is because we unconditionally set
skb->offload_fwd_mark = true, meaning that the bridge thinks the frames
were already forwarded in hardware by us.

Since dp->bridge_dev is populated only when there is hardware offload
for it, but not in the software fallback case, let's introduce a new
helper that can be called from the tagger data path which sets the
skb->offload_fwd_mark accordingly to zero when there is no hardware
offload for bridging. This lets the bridge forward packets back to other
interfaces of our switch, if needed.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa_priv.h         | 14 ++++++++++++++
 net/dsa/tag_brcm.c         |  2 +-
 net/dsa/tag_dsa.c          |  9 +++++----
 net/dsa/tag_hellcreek.c    |  2 +-
 net/dsa/tag_ksz.c          |  2 +-
 net/dsa/tag_lan9303.c      |  4 +++-
 net/dsa/tag_mtk.c          |  2 +-
 net/dsa/tag_ocelot.c       |  2 +-
 net/dsa/tag_ocelot_8021q.c |  2 +-
 net/dsa/tag_rtl4_a.c       |  2 +-
 net/dsa/tag_sja1105.c      |  4 ++--
 net/dsa/tag_xrs700x.c      |  3 +--
 12 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 7b0dd2d5f3f8..4226ce1967d3 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -326,6 +326,20 @@ static inline struct sk_buff *dsa_untag_bridge_pvid(struct sk_buff *skb)
 	return skb;
 }
 
+/* If the ingress port offloads the bridge, we mark the frame as autonomously
+ * forwarded by hardware, so the software bridge doesn't forward in twice, back
+ * to us, because we already did. However, if we're in fallback mode and we do
+ * software bridging, we are not offloading it, therefore the dp->bridge_dev
+ * pointer is not populated, and flooding needs to be done by software (we are
+ * effectively operating in standalone ports mode).
+ */
+static inline void dsa_default_offload_fwd_mark(struct sk_buff *skb)
+{
+	struct dsa_port *dp = dsa_slave_to_port(skb->dev);
+
+	skb->offload_fwd_mark = !!(dp->bridge_dev);
+}
+
 /* switch.c */
 int dsa_switch_register_notifier(struct dsa_switch *ds);
 void dsa_switch_unregister_notifier(struct dsa_switch *ds);
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index e2577a7dcbca..a8880b3bb106 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -150,7 +150,7 @@ static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb,
 	/* Remove Broadcom tag and update checksum */
 	skb_pull_rcsum(skb, BRCM_TAG_LEN);
 
-	skb->offload_fwd_mark = 1;
+	dsa_default_offload_fwd_mark(skb);
 
 	return skb;
 }
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index 7e7b7decdf39..91f405b59f75 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -162,8 +162,8 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
 static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 				  u8 extra)
 {
+	bool trap = false, trunk = false;
 	int source_device, source_port;
-	bool trunk = false;
 	enum dsa_code code;
 	enum dsa_cmd cmd;
 	u8 *dsa_header;
@@ -174,8 +174,6 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 	cmd = dsa_header[0] >> 6;
 	switch (cmd) {
 	case DSA_CMD_FORWARD:
-		skb->offload_fwd_mark = 1;
-
 		trunk = !!(dsa_header[1] & 7);
 		break;
 
@@ -194,7 +192,6 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 			 * device (like a bridge) that forwarding has
 			 * already been done by hardware.
 			 */
-			skb->offload_fwd_mark = 1;
 			break;
 		case DSA_CODE_MGMT_TRAP:
 		case DSA_CODE_IGMP_MLD_TRAP:
@@ -202,6 +199,7 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 			/* Traps have, by definition, not been
 			 * forwarded by hardware, so don't mark them.
 			 */
+			trap = true;
 			break;
 		default:
 			/* Reserved code, this could be anything. Drop
@@ -235,6 +233,9 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 	if (!skb->dev)
 		return NULL;
 
+	if (!trap)
+		dsa_default_offload_fwd_mark(skb);
+
 	/* If the 'tagged' bit is set; convert the DSA tag to a 802.1Q
 	 * tag, and delete the ethertype (extra) if applicable. If the
 	 * 'tagged' bit is cleared; delete the DSA tag, and ethertype
diff --git a/net/dsa/tag_hellcreek.c b/net/dsa/tag_hellcreek.c
index a09805c8e1ab..c1ee6eefafe4 100644
--- a/net/dsa/tag_hellcreek.c
+++ b/net/dsa/tag_hellcreek.c
@@ -44,7 +44,7 @@ static struct sk_buff *hellcreek_rcv(struct sk_buff *skb,
 
 	pskb_trim_rcsum(skb, skb->len - HELLCREEK_TAG_LEN);
 
-	skb->offload_fwd_mark = true;
+	dsa_default_offload_fwd_mark(skb);
 
 	return skb;
 }
diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 4820dbcedfa2..8eee63a5b93b 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -24,7 +24,7 @@ static struct sk_buff *ksz_common_rcv(struct sk_buff *skb,
 
 	pskb_trim_rcsum(skb, skb->len - len);
 
-	skb->offload_fwd_mark = true;
+	dsa_default_offload_fwd_mark(skb);
 
 	return skb;
 }
diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index aa1318dccaf0..3fd85139a3a6 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -115,7 +115,9 @@ static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev,
 	skb_pull_rcsum(skb, 2 + 2);
 	memmove(skb->data - ETH_HLEN, skb->data - (ETH_HLEN + LAN9303_TAG_LEN),
 		2 * ETH_ALEN);
-	skb->offload_fwd_mark = !(lan9303_tag1 & LAN9303_TAG_RX_TRAPPED_TO_CPU);
+
+	if (!(lan9303_tag1 & LAN9303_TAG_RX_TRAPPED_TO_CPU))
+		dsa_default_offload_fwd_mark(skb);
 
 	return skb;
 }
diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index 38dcdded74c0..08387fa37d17 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c
@@ -97,7 +97,7 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 
 	/* Only unicast or broadcast frames are offloaded */
 	if (likely(!is_multicast_skb))
-		skb->offload_fwd_mark = 1;
+		dsa_default_offload_fwd_mark(skb);
 
 	return skb;
 }
diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c
index 16a1afd5b8e1..7f0898569876 100644
--- a/net/dsa/tag_ocelot.c
+++ b/net/dsa/tag_ocelot.c
@@ -225,7 +225,7 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
 		 */
 		return NULL;
 
-	skb->offload_fwd_mark = 1;
+	dsa_default_offload_fwd_mark(skb);
 	skb->priority = qos_class;
 
 	/* Ocelot switches copy frames unmodified to the CPU. However, it is
diff --git a/net/dsa/tag_ocelot_8021q.c b/net/dsa/tag_ocelot_8021q.c
index 8991ebf098a3..a9ad03626b2e 100644
--- a/net/dsa/tag_ocelot_8021q.c
+++ b/net/dsa/tag_ocelot_8021q.c
@@ -48,7 +48,7 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
 	if (!skb->dev)
 		return NULL;
 
-	skb->offload_fwd_mark = 1;
+	dsa_default_offload_fwd_mark(skb);
 	skb->priority = qos_class;
 
 	return skb;
diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c
index 2646abe5a69e..c942d8697ed8 100644
--- a/net/dsa/tag_rtl4_a.c
+++ b/net/dsa/tag_rtl4_a.c
@@ -101,7 +101,7 @@ static struct sk_buff *rtl4a_tag_rcv(struct sk_buff *skb,
 		skb->data - ETH_HLEN - RTL4_A_HDR_LEN,
 		2 * ETH_ALEN);
 
-	skb->offload_fwd_mark = 1;
+	dsa_default_offload_fwd_mark(skb);
 
 	return skb;
 }
diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index 50496013cdb7..45cdf64f0e88 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -295,8 +295,6 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
 	is_link_local = sja1105_is_link_local(skb);
 	is_meta = sja1105_is_meta_frame(skb);
 
-	skb->offload_fwd_mark = 1;
-
 	if (is_tagged) {
 		/* Normal traffic path. */
 		skb_push_rcsum(skb, ETH_HLEN);
@@ -339,6 +337,8 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
 		return NULL;
 	}
 
+	dsa_default_offload_fwd_mark(skb);
+
 	if (subvlan)
 		sja1105_decode_subvlan(skb, subvlan);
 
diff --git a/net/dsa/tag_xrs700x.c b/net/dsa/tag_xrs700x.c
index 858cdf9d2913..215ecceea89e 100644
--- a/net/dsa/tag_xrs700x.c
+++ b/net/dsa/tag_xrs700x.c
@@ -45,8 +45,7 @@ static struct sk_buff *xrs700x_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (pskb_trim_rcsum(skb, skb->len - 1))
 		return NULL;
 
-	/* Frame is forwarded by hardware, don't forward in software. */
-	skb->offload_fwd_mark = 1;
+	dsa_default_offload_fwd_mark(skb);
 
 	return skb;
 }
-- 
2.25.1

Re: [PATCH net-next 0/4] Software fallback for bridging in DSA

[Vladimir Oltean]

On Sun, Feb 14, 2021 at 05:53:22PM +0200, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> As was discussed here:
> https://patchwork.kernel.org/project/netdevbpf/patch/20201202091356.24075-3-tobias@waldekranz.com/
> 
> it is desirable to not reject a LAG interface (bonding, team) even if
> the switch isn't able to offload bridging towards that link aggregation
> group. At least the DSA setups I have are not that unbalanced between
> the horsepower of the CPU and the horsepower of the switch such that
> software forwarding to be completely impractical.
> 
> This series makes all switch drivers theoretically able to do the right
> thing when they are configured in a way similar to this (credits to
> Tobias Waldekranz for the drawing):
> 
>       br0
>      /   \
>   team0   \
>    / \     \
> swp0 swp1  swp2
> 
> although in practice there is one more prerequisite: for software
> fallback mode, they need to disable address learning. It is preferable
> that they do this by implementing the .port_pre_bridge_join and
> .port_bridge_join methods.

Sadly there is some false marketing on my part here and this series is
probably not enough for the above configuration to work in software.
I have to confess that I tested with software bridging, and with
software LAG, but not with both at the same time. I sent this series way
too quickly and I should probably spend more time on it. Sorry to
everyone copied for the noise.

Re: [PATCH net-next 4/4] net: dsa: don't set skb->offload_fwd_mark when not offloading the bridge

[George McCollister]

On Sun, Feb 14, 2021 at 9:54 AM Vladimir Oltean <olteanv@gmail.com> wrote:
[snip]
> diff --git a/net/dsa/tag_xrs700x.c b/net/dsa/tag_xrs700x.c
> index 858cdf9d2913..215ecceea89e 100644
> --- a/net/dsa/tag_xrs700x.c
> +++ b/net/dsa/tag_xrs700x.c
> @@ -45,8 +45,7 @@ static struct sk_buff *xrs700x_rcv(struct sk_buff *skb, struct net_device *dev,
>         if (pskb_trim_rcsum(skb, skb->len - 1))
>                 return NULL;
>
> -       /* Frame is forwarded by hardware, don't forward in software. */
> -       skb->offload_fwd_mark = 1;
> +       dsa_default_offload_fwd_mark(skb);

Does it make sense that the following would have worked prior to this
change? Is this only an issue for bridging between DSA ports when
offloading is supported? lan0 is a port an an xrs700x switch:

ip link set eth0 up
ip link del veth0
ip link add veth0 type veth peer name veth1

for eth in veth0 veth1 lan1; do
    ip link set ${eth} up
done
ip link add br0 type bridge
ip link set veth1 master br0
ip link set lan1 master br0
ip link set br0 up

ip addr add 192.168.2.1/24 dev veth0

# ping host connected to physical LAN that lan0 is on
ping 192.168.2.249 (works!)

I was trying to come up with a way to test this change and expected
this would fail (and your patch) would fix it based on what you're
described.

-George

>
>         return skb;
>  }
> --
> 2.25.1
>

Re: [PATCH net-next 4/4] net: dsa: don't set skb->offload_fwd_mark when not offloading the bridge

[Vladimir Oltean]

Hi George,

On Mon, Feb 15, 2021 at 09:48:38AM -0600, George McCollister wrote:
> On Sun, Feb 14, 2021 at 9:54 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> [snip]
> > diff --git a/net/dsa/tag_xrs700x.c b/net/dsa/tag_xrs700x.c
> > index 858cdf9d2913..215ecceea89e 100644
> > --- a/net/dsa/tag_xrs700x.c
> > +++ b/net/dsa/tag_xrs700x.c
> > @@ -45,8 +45,7 @@ static struct sk_buff *xrs700x_rcv(struct sk_buff *skb, struct net_device *dev,
> >         if (pskb_trim_rcsum(skb, skb->len - 1))
> >                 return NULL;
> >
> > -       /* Frame is forwarded by hardware, don't forward in software. */
> > -       skb->offload_fwd_mark = 1;
> > +       dsa_default_offload_fwd_mark(skb);
>
> Does it make sense that the following would have worked prior to this
> change? Is this only an issue for bridging between DSA ports when
> offloading is supported? lan0 is a port an an xrs700x switch:
>
> ip link set eth0 up
> ip link del veth0
> ip link add veth0 type veth peer name veth1
>
> for eth in veth0 veth1 lan1; do
>     ip link set ${eth} up
> done
> ip link add br0 type bridge
> ip link set veth1 master br0
> ip link set lan1 master br0
> ip link set br0 up
>
> ip addr add 192.168.2.1/24 dev veth0
>
> # ping host connected to physical LAN that lan0 is on
> ping 192.168.2.249 (works!)
>
> I was trying to come up with a way to test this change and expected
> this would fail (and your patch) would fix it based on what you're
> described.

No, the configuration you've shown should be supported and functional
already (as you've noticed, in fact). I call it 'bridging with a
foreign interface', where a foreign interface is a bridge port that has
a different switchdev mark compared to the DSA switch. A switchdev mark
is a number assigned to every bridge port by nbp_switchdev_mark_set,
based on the "physical switch id"*.

There is a simple rule with switchdev: on reception of an skb, the bridge
checks if it was marked as 'already forwarded in hardware' (checks if
skb->offload_fwd_mark == 1), and if it is, it puts a mark of its own on
that skb, with the switchdev mark of the ingress port. Then during
forwarding, it enforces that the egress port must have a different switchdev
mark than the ingress one (this is done in nbp_switchdev_allowed_egress).

The veth driver does not implement any sort of method for retrieving a
physical switch id (neither devlink nor .ndo_get_port_parent_id),
therefore the bridge assigns it a switchdev mark of 0, and packets
coming from it will always have skb->offload_fwd_mark = 0. So there
aren't any restrictions.

Problems appear as soon as software bridging is attempted between two
interfaces that have the same switchdev mark. If skb->offload_fwd_mark=1,
the bridge will say that forwarding was already performed in hw, so it
will deny it in sw.
The issue is that a bond0 (or hsr0) upper of lan0 will be assigned the
same switchdev mark as lan0 itself, because the function that assigns
switchdev marks to bridge ports, nbp_switchdev_mark_set, recurses
through that port's lower interfaces until it finds something that
implements devlink.

What I tested is actually pretty laughable and a far cry from a
real-life scenario: I commented out the .port_bridge_join and
.port_bridge_leave methods of a driver and made sure that forwarding
between ports still works regardless of what uppers they have
(even that used not to). But this bypasses the switchdev mark checks in
nbp_switchdev_allowed_egress because the skb->offload_fwd_mark=0 now.
This is an important prerequisite for seamless operation, true, but it
isn't quite what we want. For one thing, we may have a topology like
this:

         +-- br0 -+
        /   / |    \
       /   /  |     \
      /   /   |      \
     /   /    |       \
    /   /     |        \
   /    |     |       bond0
  /     |     |      /    \
 swp0  swp1  swp2  swp3  swp4

where it is desirable that the presence of swp3 and swp4 under a
non-offloaded LAG does not preclude us from doing hardware bridging
beteen swp0, swp1 and swp2.

But this creates an impossible paradox if we continue in the way that I
started in this patch.

When the CPU receives a packet from swp0 (say, due to flooding), the
tagger must set skb->offload_fwd_mark to something.

If we set it to 0, then the bridge will forward it towards swp1, swp2
and bond0. But the switch has already forwarded it towards swp1 and
swp2 (not to bond0, remember, that isn't offloaded, so as far as the
switch is concerned, ports swp3 and swp4 are not looking up the FDB, and
the entire bond0 is a destination that is strictly behind the CPU). But
we don't want duplicated traffic towards swp1 and swp2, so it's not ok
to set skb->offload_fwd_mark = 0.

If we set it to 1, then the bridge will not forward the skb towards the
ports with the same switchdev mark, i.e. not to swp1, swp2 and bond0.
Towards swp1 and swp2 that's ok, but towards bond0? It should have
forwarded the skb there.

An actual solution to this problem, which has nothing to do with my
series, is to give the bridge more hints as to what switchdev mark it
should use for each port.

Currently, the bridging offload is very 'silent': a driver registers a
netdevice notifier, which is put on the netns's notifier chain, and
which sniffs around for NETDEV_CHANGEUPPER events where the upper is a
bridge, and the lower is an interface it knows about (one registered by
this driver, normally). Then, from within that notifier, it does a bunch
of stuff behind the bridge's back, without the bridge necessarily
knowing that there's somebody offloading that port. It looks like this:

     ip link set lan0 master br0
                  |
                  v
   bridge calls netdev_master_upper_dev_link
                  |
                  v
        call_netdevice_notifiers
                  |
                  v
       dsa_slave_netdevice_event
                  |
                  v
        oh, hey! it's for me!
                  |
                  v
           .port_bridge_join

What we should probably do to solve the conundrum is to be less silent,
and emit a notification back. Something like this:

     ip link set lan0 master br0
                  |
                  v
   bridge calls netdev_master_upper_dev_link
                  |
                  v                    bridge: Aye! I'll use this
        call_netdevice_notifiers           ^  switch_id as the
                  |                        |  switchdev mark for
                  v                        |  this port, and zero
       dsa_slave_netdevice_event           |  if I got nothing.
                  |                        |
                  v                        |
        oh, hey! it's for me!              |
                  |                        |
                  v                        |
           .port_bridge_join               |
                  |                        |
                  +------------------------+
      call_switchdev_notifiers(lan0, SWITCHDEV_BRPORT_OFFLOADED, switch_id)

Then stacked interfaces (like bond0 on top of swp3/swp4) would be
treated differently in DSA, depending on whether we can or cannot
offload them.

The offload case:

    ip link set bond0 master br0
                  |
                  v
   bridge calls netdev_master_upper_dev_link
                  |
                  v                    bridge: Aye! I'll use this
        call_netdevice_notifiers           ^  switch_id as the
                  |                        |  switchdev mark for
                  v                        |        bond0.
       dsa_slave_netdevice_event           | Coincidentally (or not),
                  |                        | bond0 and swp0, swp1, swp2
                  v                        | all have the same switchdev
        hmm, it's not quite for me,        | mark now, since the ASIC
         but my driver has already         | is able to forward towards
           called .port_lag_join           | all these ports in hw.
          for it, because I have           |
      a port with dp->lag_dev == bond0.    |
                  |                        |
                  v                        |
           .port_bridge_join               |
           for swp3 and swp4               |
                  |                        |
                  +------------------------+
      call_switchdev_notifiers(bond0, SWITCHDEV_BRPORT_OFFLOADED, switch_id)

And the non-offload case:

    ip link set bond0 master br0
                  |
                  v
   bridge calls netdev_master_upper_dev_link
                  |
                  v                    bridge waiting:
        call_netdevice_notifiers           ^  huh, no SWITCHDEV_BRPORT_OFFLOADED
                  |                        |  event, okay, I'll use a switchdev
                  v                        |  mark of zero for this one.
       dsa_slave_netdevice_event           :  Then packets received on swp0 will
                  |                        :  not be forwarded towards swp1, but
                  v                        :  they will towards bond0.
         it's not for me, but
       bond0 is an upper of swp3
      and swp4, but their dp->lag_dev
       is NULL because they couldn't
            offload it.

This is what I should have really done. For some reason though, I was so
trigger-happy that I got the data path working (without the surrounding
control logic to manage the switchdev marks automatically) that I just
got carried away and sent this small patch set.

I need some time to take my mind off of this for a while, and then I'll
come with a serious proposal eventually. Sorry again for the confusion.


*This is retrieved, in DSA's case, through the "switch_id" attribute
that we populate in dsa_port_devlink_setup. DSA says that the entire
DSA switch tree dst has the same switch_id, because it assumes that any
driver capable of cross-chip bridging (aka Marvell) is able to do
hardware forwarding towards any other switch in the same "switching
fabric". So it's not really a "switch_id", but a "port parent" somehow.

Re: [PATCH net-next 1/4] net: dsa: don't offload switchdev objects on ports that don't offload the bridge

[Tobias Waldekranz]

On Sun, Feb 14, 2021 at 17:53, Vladimir Oltean <olteanv@gmail.com> wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> Starting with commit 058102a6e9eb ("net: dsa: Link aggregation support"),
> DSA warns that certain configurations of upper interfaces are not offloaded
> to hardware. When a DSA port does not offload a LAG interface, the
> dp->lag_dev pointer is always NULL. However the same cannot be said about
> offloading a bridge: dp->bridge_dev will get populated regardless of
> whether the driver can put the port into the bridge's forwarding domain
> or not.
>
> Instead of silently returning 0 if the driver doesn't implement
> .port_bridge_join, return -EOPNOTSUPP instead, and print a message via
> netlink extack that the configuration was not offloaded to hardware.
>
> Now we can use the check whether dp->bridge_dev is NULL in order to
> avoid offloading at all switchdev attributes and objects for ports that
> don't even offload the basic operation of switching. Those can still do
> the required L2 forwarding using the bridge software datapath, but
> enabling any hardware features specific to the bridge such as address
> learning would just ask for problems.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>

Re: [PATCH net-next 2/4] net: dsa: reject switchdev objects centrally from dsa_slave_port_obj_{add,del}

[Tobias Waldekranz]

On Sun, Feb 14, 2021 at 17:53, Vladimir Oltean <olteanv@gmail.com> wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> The dsa_port_offloads_netdev check is inside dsa_slave_vlan_{add,del},
> but outside dsa_port_mdb_{add,del}. We can reduce the number of
> occurrences of dsa_port_offloads_netdev by checking only once, at the
> beginning of dsa_slave_port_obj_add and dsa_slave_port_obj_del.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>

Re: [PATCH net-next 3/4] net: dsa: return -EOPNOTSUPP if .port_lag_join is not implemented

[Tobias Waldekranz]

On Sun, Feb 14, 2021 at 17:53, Vladimir Oltean <olteanv@gmail.com> wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> There is currently some provisioning for DSA to use the software
> fallback for link aggregation, but only if the .port_lag_join is
> implemented but it fails (for example because there are more link
> aggregation groups than the switch supports, or because the xmit hash
> policy cannot be done in hardware, or ...).
>
> But when .port_lag_join is not implemented at all, the DSA switch
> notifier returns zero and software fallback does not kick in.
> Change that.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>

Re: [PATCH net-next 4/4] net: dsa: don't set skb->offload_fwd_mark when not offloading the bridge

[Tobias Waldekranz]

On Sun, Feb 14, 2021 at 17:53, Vladimir Oltean <olteanv@gmail.com> wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> DSA has gained the recent ability to deal gracefully with upper
> interfaces it cannot offload, such as the bridge, bonding or team
> drivers. When such uppers exist, the ports are still in standalone mode
> as far as the hardware is concerned.
>
> But when we deliver packets to the software bridge in order for that to
> do the forwarding, there is an unpleasant surprise in that the bridge
> will refuse to forward them. This is because we unconditionally set
> skb->offload_fwd_mark = true, meaning that the bridge thinks the frames
> were already forwarded in hardware by us.
>
> Since dp->bridge_dev is populated only when there is hardware offload
> for it, but not in the software fallback case, let's introduce a new
> helper that can be called from the tagger data path which sets the
> skb->offload_fwd_mark accordingly to zero when there is no hardware
> offload for bridging. This lets the bridge forward packets back to other
> interfaces of our switch, if needed.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

For the generic and tag_dsa.c related changes:

Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>

Re: [PATCH net-next 4/4] net: dsa: don't set skb->offload_fwd_mark when not offloading the bridge

[Vladimir Oltean]

On Thu, Feb 25, 2021 at 08:25:23PM +0100, Tobias Waldekranz wrote:
> On Sun, Feb 14, 2021 at 17:53, Vladimir Oltean <olteanv@gmail.com> wrote:
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> >
> > DSA has gained the recent ability to deal gracefully with upper
> > interfaces it cannot offload, such as the bridge, bonding or team
> > drivers. When such uppers exist, the ports are still in standalone mode
> > as far as the hardware is concerned.
> >
> > But when we deliver packets to the software bridge in order for that to
> > do the forwarding, there is an unpleasant surprise in that the bridge
> > will refuse to forward them. This is because we unconditionally set
> > skb->offload_fwd_mark = true, meaning that the bridge thinks the frames
> > were already forwarded in hardware by us.
> >
> > Since dp->bridge_dev is populated only when there is hardware offload
> > for it, but not in the software fallback case, let's introduce a new
> > helper that can be called from the tagger data path which sets the
> > skb->offload_fwd_mark accordingly to zero when there is no hardware
> > offload for bridging. This lets the bridge forward packets back to other
> > interfaces of our switch, if needed.
> >
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> 
> For the generic and tag_dsa.c related changes:
> 
> Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>

Actually with my switchdev_bridge_port_offload_notify() proposal, I
don't think this patch is going to be needed at all. I think the bridge
happily ignores a packet with skb->offload_fwd_mark = 1 if it comes from
a port which has an offload_fwd_mark of 0, although I haven't tested that.