All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next 0/4] DSA: promisc on master, generic flow dissector code
@ 2020-05-11 20:20 Vladimir Oltean
  2020-05-11 20:20 ` [PATCH net-next 1/4] net: dsa: allow drivers to request promiscuous mode on master Vladimir Oltean
                   ` (4 more replies)
  0 siblings, 5 replies; 18+ messages in thread
From: Vladimir Oltean @ 2020-05-11 20:20 UTC (permalink / raw)
  To: andrew, f.fainelli, vivien.didelot; +Cc: davem, kuba, netdev

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

The initial purpose of this series was to implement the .flow_dissect
method for sja1105 and for ocelot/felix. But on Felix this posed a
problem, as the DSA headers were of different lengths on RX and on TX.
A better solution than to just increase the smaller one was to also try
to shrink the larger one, but in turn that required the DSA master to be
put in promiscuous mode (which sja1105 also needed, for other reasons).

Finally, we can add the missing .flow_dissect methods to ocelot and
sja1105 (as well as generalize the formula to other taggers as well).

Vladimir Oltean (4):
  net: dsa: allow drivers to request promiscuous mode on master
  net: dsa: sja1105: request promiscuous mode for master
  net: dsa: tag_ocelot: use a short prefix on both ingress and egress
  net: dsa: implement and use a generic procedure for the flow dissector

 drivers/net/dsa/ocelot/felix.c         |  5 ++--
 drivers/net/dsa/sja1105/sja1105_main.c |  3 ++
 include/net/dsa.h                      | 13 +++++++++
 net/dsa/dsa_priv.h                     | 22 +++++++++++++++
 net/dsa/master.c                       | 39 +++++++++++++++++++++++++-
 net/dsa/tag_ar9331.c                   |  9 ++++++
 net/dsa/tag_brcm.c                     |  5 ++--
 net/dsa/tag_dsa.c                      |  4 +--
 net/dsa/tag_edsa.c                     |  4 +--
 net/dsa/tag_lan9303.c                  |  9 ++++++
 net/dsa/tag_mtk.c                      |  3 +-
 net/dsa/tag_ocelot.c                   | 29 +++++++++++++++----
 net/dsa/tag_qca.c                      |  3 +-
 net/dsa/tag_sja1105.c                  | 13 +++++++++
 14 files changed, 143 insertions(+), 18 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH net-next 1/4] net: dsa: allow drivers to request promiscuous mode on master
  2020-05-11 20:20 [PATCH net-next 0/4] DSA: promisc on master, generic flow dissector code Vladimir Oltean
@ 2020-05-11 20:20 ` Vladimir Oltean
  2020-05-11 23:19   ` Florian Fainelli
  2020-05-11 20:20 ` [PATCH net-next 2/4] net: dsa: sja1105: request promiscuous mode for master Vladimir Oltean
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Vladimir Oltean @ 2020-05-11 20:20 UTC (permalink / raw)
  To: andrew, f.fainelli, vivien.didelot; +Cc: davem, kuba, netdev

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

Currently DSA assumes that taggers don't mess with the destination MAC
address of the frames on RX. That is not always the case. Some DSA
headers are placed before the Ethernet header (ocelot), and others
simply mangle random bytes from the destination MAC address (sja1105
with its incl_srcpt option).

Currently the DSA master goes to promiscuous mode automatically when the
slave devices go too (such as when enslaved to a bridge), but in
standalone mode this is a problem that needs to be dealt with.

So give drivers the possibility to signal that their tagging protocol
will get randomly dropped otherwise, and let DSA deal with fixing that.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/net/dsa.h | 13 +++++++++++++
 net/dsa/master.c  | 39 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 312c2f067e65..ddc970430a63 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -217,6 +217,12 @@ struct dsa_port {
 	 */
 	const struct net_device_ops *orig_ndo_ops;
 
+	/*
+	 * Original master netdev flags in case we need to put it in
+	 * promiscuous mode
+	 */
+	unsigned int orig_master_flags;
+
 	bool setup;
 };
 
@@ -298,6 +304,13 @@ struct dsa_switch {
 	 */
 	bool			mtu_enforcement_ingress;
 
+	/* Some tagging protocols either mangle or shift the destination MAC
+	 * address, in which case the DSA master would drop packets on ingress
+	 * if what it understands out of the destination MAC address is not in
+	 * its RX filter.
+	 */
+	bool promisc_on_master;
+
 	size_t num_ports;
 };
 
diff --git a/net/dsa/master.c b/net/dsa/master.c
index a621367c6e8c..5d1873026612 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -294,6 +294,37 @@ static void dsa_master_ndo_teardown(struct net_device *dev)
 	cpu_dp->orig_ndo_ops = NULL;
 }
 
+static void dsa_master_set_promisc(struct net_device *dev)
+{
+	struct dsa_port *cpu_dp = dev->dsa_ptr;
+	struct dsa_switch *ds = cpu_dp->ds;
+	unsigned int flags;
+
+	if (!ds->promisc_on_master)
+		return;
+
+	flags = dev_get_flags(dev);
+
+	cpu_dp->orig_master_flags = flags;
+
+	rtnl_lock();
+	dev_change_flags(dev, flags | IFF_PROMISC, NULL);
+	rtnl_unlock();
+}
+
+static void dsa_master_reset_promisc(struct net_device *dev)
+{
+	struct dsa_port *cpu_dp = dev->dsa_ptr;
+	struct dsa_switch *ds = cpu_dp->ds;
+
+	if (!ds->promisc_on_master)
+		return;
+
+	rtnl_lock();
+	dev_change_flags(dev, cpu_dp->orig_master_flags, NULL);
+	rtnl_unlock();
+}
+
 static ssize_t tagging_show(struct device *d, struct device_attribute *attr,
 			    char *buf)
 {
@@ -345,9 +376,12 @@ int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
 	wmb();
 
 	dev->dsa_ptr = cpu_dp;
+
+	dsa_master_set_promisc(dev);
+
 	ret = dsa_master_ethtool_setup(dev);
 	if (ret)
-		return ret;
+		goto out_err_reset_promisc;
 
 	ret = dsa_master_ndo_setup(dev);
 	if (ret)
@@ -363,6 +397,8 @@ int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
 	dsa_master_ndo_teardown(dev);
 out_err_ethtool_teardown:
 	dsa_master_ethtool_teardown(dev);
+out_err_reset_promisc:
+	dsa_master_reset_promisc(dev);
 	return ret;
 }
 
@@ -372,6 +408,7 @@ void dsa_master_teardown(struct net_device *dev)
 	dsa_master_ndo_teardown(dev);
 	dsa_master_ethtool_teardown(dev);
 	dsa_master_reset_mtu(dev);
+	dsa_master_reset_promisc(dev);
 
 	dev->dsa_ptr = NULL;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH net-next 2/4] net: dsa: sja1105: request promiscuous mode for master
  2020-05-11 20:20 [PATCH net-next 0/4] DSA: promisc on master, generic flow dissector code Vladimir Oltean
  2020-05-11 20:20 ` [PATCH net-next 1/4] net: dsa: allow drivers to request promiscuous mode on master Vladimir Oltean
@ 2020-05-11 20:20 ` Vladimir Oltean
  2020-05-11 23:19   ` Florian Fainelli
  2020-05-11 20:20 ` [PATCH net-next 3/4] net: dsa: tag_ocelot: use a short prefix on both ingress and egress Vladimir Oltean
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Vladimir Oltean @ 2020-05-11 20:20 UTC (permalink / raw)
  To: andrew, f.fainelli, vivien.didelot; +Cc: davem, kuba, netdev

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

Currently PTP is broken when ports are in standalone mode (the tagger
keeps printing this message):

sja1105 spi0.1: Expected meta frame, is  180c200000e in the DSA master multicast filter?

Sure, one might say "simply add 01-80-c2-00-00-0e to the master's RX
filter" but things become more complicated because:

- Actually all frames in the 01-80-c2-xx-xx-xx and 01-1b-19-xx-xx-xx
  range are trapped to the CPU automatically
- The switch mangles bytes 3 and 4 of the MAC address via the incl_srcpt
  ("include source port [in the DMAC]") option, so an address installed
  to the RX filter would, at the end of the day, not correspond to the
  final address seen by the DSA master.

Assume RX filters on DSA masters are typically too small to include all
necessary addresses for PTP to work properly on sja1105, and just
request promiscuous mode unconditionally.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index d5de9305df25..786e698b1856 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -2080,6 +2080,9 @@ static int sja1105_setup(struct dsa_switch *ds)
 		dev_err(ds->dev, "Failed to configure MII clocking: %d\n", rc);
 		return rc;
 	}
+
+	ds->promisc_on_master = true;
+
 	/* On SJA1105, VLAN filtering per se is always enabled in hardware.
 	 * The only thing we can do to disable it is lie about what the 802.1Q
 	 * EtherType is.
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH net-next 3/4] net: dsa: tag_ocelot: use a short prefix on both ingress and egress
  2020-05-11 20:20 [PATCH net-next 0/4] DSA: promisc on master, generic flow dissector code Vladimir Oltean
  2020-05-11 20:20 ` [PATCH net-next 1/4] net: dsa: allow drivers to request promiscuous mode on master Vladimir Oltean
  2020-05-11 20:20 ` [PATCH net-next 2/4] net: dsa: sja1105: request promiscuous mode for master Vladimir Oltean
@ 2020-05-11 20:20 ` Vladimir Oltean
  2020-05-11 22:40   ` Jakub Kicinski
  2020-05-11 23:22   ` Florian Fainelli
  2020-05-11 20:20 ` [PATCH net-next 4/4] net: dsa: implement and use a generic procedure for the flow dissector Vladimir Oltean
  2020-05-11 23:28 ` [PATCH net-next 0/4] DSA: promisc on master, generic flow dissector code Florian Fainelli
  4 siblings, 2 replies; 18+ messages in thread
From: Vladimir Oltean @ 2020-05-11 20:20 UTC (permalink / raw)
  To: andrew, f.fainelli, vivien.didelot; +Cc: davem, kuba, netdev

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

There are 2 goals that we follow:

- Reduce the header size
- Make the header size equal between RX and TX

The issue that required long prefix on RX was the fact that the ocelot
DSA tag, being put before Ethernet as it is, would overlap with the area
that a DSA master uses for RX filtering (destination MAC address
mainly).

Now that we can ask DSA to put the master in promiscuous mode, in theory
we could remove the prefix altogether and call it a day, but it looks
like we can't. Using no prefix on ingress, some packets (such as ICMP)
would be received, while others (such as PTP) would not be received.
This is because the DSA master we use (enetc) triggers parse errors
("MAC rx frame errors") presumably because it sees Ethernet frames with
a bad length. And indeed, when using no prefix, the EtherType (bytes
12-13 of the frame, bits 96-111) falls over the REW_VAL field from the
extraction header, aka the PTP timestamp.

When turning the short (32-bit) prefix on, the EtherType overlaps with
bits 64-79 of the extraction header, which are a reserved area
transmitted as zero by the switch. The packets are not dropped by the
DSA master with a short prefix. Actually, the frames look like this in
tcpdump (below is a PTP frame, with an extra dsa_8021q tag - dadb 0482 -
added by a downstream sja1105).

89:0c:a9:f2:01:00 > 88:80:00:0a:00:1d, 802.3, length 0: LLC, \
	dsap Unknown (0x10) Individual, ssap ProWay NM (0x0e) Response, \
	ctrl 0x0004: Information, send seq 2, rcv seq 0, \
	Flags [Response], length 78

0x0000:  8880 000a 001d 890c a9f2 0100 0000 100f  ................
0x0010:  0400 0000 0180 c200 000e 001f 7b63 0248  ............{c.H
0x0020:  dadb 0482 88f7 1202 0036 0000 0000 0000  .........6......
0x0030:  0000 0000 0000 0000 0000 001f 7bff fe63  ............{..c
0x0040:  0248 0001 1f81 0500 0000 0000 0000 0000  .H..............
0x0050:  0000 0000 0000 0000 0000 0000            ............

So the short prefix is our new default: we've shortened our RX frames by
12 octets, increased TX by 4, and headers are now equal between RX and
TX. Note that we still need promiscuous mode for the DSA master to not
drop it.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/ocelot/felix.c |  5 +++--
 net/dsa/tag_ocelot.c           | 20 +++++++++++++++-----
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index a2dfd73f8a1a..8b8d463223d5 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -545,8 +545,8 @@ static int felix_setup(struct dsa_switch *ds)
 		/* Bring up the CPU port module and configure the NPI port */
 		if (dsa_is_cpu_port(ds, port))
 			ocelot_configure_cpu(ocelot, port,
-					     OCELOT_TAG_PREFIX_NONE,
-					     OCELOT_TAG_PREFIX_LONG);
+					     OCELOT_TAG_PREFIX_SHORT,
+					     OCELOT_TAG_PREFIX_SHORT);
 	}
 
 	/* Include the CPU port module in the forwarding mask for unknown
@@ -564,6 +564,7 @@ static int felix_setup(struct dsa_switch *ds)
 				 ANA_FLOODING_FLD_UNICAST(PGID_UC),
 				 ANA_FLOODING, tc);
 
+	ds->promisc_on_master = true;
 	ds->mtu_enforcement_ingress = true;
 	/* It looks like the MAC/PCS interrupt register - PM0_IEVENT (0x8040)
 	 * isn't instantiated for the Felix PF.
diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c
index 59de1315100f..778a7f34f8ec 100644
--- a/net/dsa/tag_ocelot.c
+++ b/net/dsa/tag_ocelot.c
@@ -5,6 +5,9 @@
 #include <linux/packing.h>
 #include "dsa_priv.h"
 
+#define OCELOT_PREFIX_VAL	0x8880000a
+#define OCELOT_TOTAL_TAG_LEN	(OCELOT_SHORT_PREFIX_LEN + OCELOT_TAG_LEN)
+
 /* The CPU injection header and the CPU extraction header can have 3 types of
  * prefixes: long, short and no prefix. The format of the header itself is the
  * same in all 3 cases.
@@ -143,8 +146,11 @@ static struct sk_buff *ocelot_xmit(struct sk_buff *skb,
 	struct ocelot *ocelot = ds->priv;
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
 	u8 *injection;
+	u32 *prefix;
+	int rc;
 
-	if (unlikely(skb_cow_head(skb, OCELOT_TAG_LEN) < 0)) {
+	rc = skb_cow_head(skb, OCELOT_TOTAL_TAG_LEN);
+	if (unlikely(rc < 0)) {
 		netdev_err(netdev, "Cannot make room for tag.\n");
 		return NULL;
 	}
@@ -174,6 +180,10 @@ static struct sk_buff *ocelot_xmit(struct sk_buff *skb,
 		packing(injection, &rew_op, 125, 117, OCELOT_TAG_LEN, PACK, 0);
 	}
 
+	prefix = skb_push(skb, OCELOT_SHORT_PREFIX_LEN);
+
+	*prefix = cpu_to_be32(OCELOT_PREFIX_VAL);
+
 	return skb;
 }
 
@@ -189,11 +199,11 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
 	 * so it points to the beginning of the frame.
 	 */
 	skb_push(skb, ETH_HLEN);
-	/* We don't care about the long prefix, it is just for easy entrance
+	/* We don't care about the short prefix, it is just for easy entrance
 	 * into the DSA master's RX filter. Discard it now by moving it into
 	 * the headroom.
 	 */
-	skb_pull(skb, OCELOT_LONG_PREFIX_LEN);
+	skb_pull(skb, OCELOT_SHORT_PREFIX_LEN);
 	/* And skb->data now points to the extraction frame header.
 	 * Keep a pointer to it.
 	 */
@@ -207,7 +217,7 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
 	skb_pull(skb, ETH_HLEN);
 
 	/* Remove from inet csum the extraction header */
-	skb_postpull_rcsum(skb, start, OCELOT_LONG_PREFIX_LEN + OCELOT_TAG_LEN);
+	skb_postpull_rcsum(skb, start, OCELOT_TOTAL_TAG_LEN);
 
 	packing(extraction, &src_port,  46, 43, OCELOT_TAG_LEN, UNPACK, 0);
 	packing(extraction, &qos_class, 19, 17, OCELOT_TAG_LEN, UNPACK, 0);
@@ -233,7 +243,7 @@ static struct dsa_device_ops ocelot_netdev_ops = {
 	.proto			= DSA_TAG_PROTO_OCELOT,
 	.xmit			= ocelot_xmit,
 	.rcv			= ocelot_rcv,
-	.overhead		= OCELOT_TAG_LEN + OCELOT_LONG_PREFIX_LEN,
+	.overhead		= OCELOT_TOTAL_TAG_LEN,
 };
 
 MODULE_LICENSE("GPL v2");
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH net-next 4/4] net: dsa: implement and use a generic procedure for the flow dissector
  2020-05-11 20:20 [PATCH net-next 0/4] DSA: promisc on master, generic flow dissector code Vladimir Oltean
                   ` (2 preceding siblings ...)
  2020-05-11 20:20 ` [PATCH net-next 3/4] net: dsa: tag_ocelot: use a short prefix on both ingress and egress Vladimir Oltean
@ 2020-05-11 20:20 ` Vladimir Oltean
  2020-05-11 23:15   ` Florian Fainelli
  2020-05-11 23:28 ` [PATCH net-next 0/4] DSA: promisc on master, generic flow dissector code Florian Fainelli
  4 siblings, 1 reply; 18+ messages in thread
From: Vladimir Oltean @ 2020-05-11 20:20 UTC (permalink / raw)
  To: andrew, f.fainelli, vivien.didelot; +Cc: davem, kuba, netdev

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

For all DSA formats that don't use tail tags, it looks like behind the
obscure number crunching they're all doing the same thing: locating the
real EtherType behind the DSA tag. Nonetheless, this is not immediately
obvious, so create a generic helper for those DSA taggers that put the
header before the EtherType.

Another assumption for the generic function is that the DSA tags are of
equal length on RX and on TX. Prior to the previous patch, this was not
true for ocelot and for gswip. The problem was resolved for ocelot, but
for gswip it still remains, so that hasn't been converted.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa_priv.h    | 22 ++++++++++++++++++++++
 net/dsa/tag_ar9331.c  |  9 +++++++++
 net/dsa/tag_brcm.c    |  5 +++--
 net/dsa/tag_dsa.c     |  4 ++--
 net/dsa/tag_edsa.c    |  4 ++--
 net/dsa/tag_lan9303.c |  9 +++++++++
 net/dsa/tag_mtk.c     |  3 +--
 net/dsa/tag_ocelot.c  |  9 +++++++++
 net/dsa/tag_qca.c     |  3 +--
 net/dsa/tag_sja1105.c | 13 +++++++++++++
 10 files changed, 71 insertions(+), 10 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index a1a0ae242012..cad276ff28d1 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -195,6 +195,28 @@ dsa_slave_to_master(const struct net_device *dev)
 	return dp->cpu_dp->master;
 }
 
+/* All DSA tags that push the EtherType to the right (basically all except tail
+ * tags, which don't break dissection) can be treated the same from the
+ * perspective of the flow dissector.
+ *
+ * We need to return:
+ *  - offset: the (B - A) difference between:
+ *    A. the position of the real EtherType and
+ *    B. the current skb->data (aka ETH_HLEN bytes into the frame, aka 2 bytes
+ *       after the normal EtherType was supposed to be)
+ *    The offset in bytes is exactly equal to the tagger overhead (and half of
+ *    that, in __be16 shorts).
+ *
+ *  - proto: the value of the real EtherType.
+ */
+static inline void dsa_tag_generic_flow_dissect(const struct sk_buff *skb,
+						__be16 *proto, int *offset,
+						int tag_len)
+{
+	*offset = tag_len;
+	*proto = ((__be16 *)skb->data)[(tag_len / 2) - 1];
+}
+
 /* 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_ar9331.c b/net/dsa/tag_ar9331.c
index 55b00694cdba..9aaae107483b 100644
--- a/net/dsa/tag_ar9331.c
+++ b/net/dsa/tag_ar9331.c
@@ -83,12 +83,21 @@ static struct sk_buff *ar9331_tag_rcv(struct sk_buff *skb,
 	return skb;
 }
 
+static int ar9331_flow_dissect(const struct sk_buff *skb, __be16 *proto,
+			       int *offset)
+{
+	dsa_tag_generic_flow_dissect(skb, proto, offset, AR9331_HDR_LEN);
+
+	return 0;
+}
+
 static const struct dsa_device_ops ar9331_netdev_ops = {
 	.name	= "ar9331",
 	.proto	= DSA_TAG_PROTO_AR9331,
 	.xmit	= ar9331_tag_xmit,
 	.rcv	= ar9331_tag_rcv,
 	.overhead = AR9331_HDR_LEN,
+	.flow_dissect = ar9331_flow_dissect,
 };
 
 MODULE_LICENSE("GPL v2");
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index cc8512b5f9e2..9c6c30649d13 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -160,9 +160,10 @@ static int brcm_tag_flow_dissect(const struct sk_buff *skb, __be16 *proto,
 	 * skb->data points 2 bytes before the actual Ethernet type field and
 	 * we have an offset of 4bytes between where skb->data and where the
 	 * payload starts.
+	 * So we can use the generic helper in both cases.
 	 */
-	*offset = BRCM_TAG_LEN;
-	*proto = ((__be16 *)skb->data)[1];
+	dsa_tag_generic_flow_dissect(skb, proto, offset, BRCM_TAG_LEN);
+
 	return 0;
 }
 #endif
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index 7ddec9794477..09479ab9a325 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -145,8 +145,8 @@ static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev,
 static int dsa_tag_flow_dissect(const struct sk_buff *skb, __be16 *proto,
 				int *offset)
 {
-	*offset = 4;
-	*proto = ((__be16 *)skb->data)[1];
+	dsa_tag_generic_flow_dissect(skb, proto, offset, DSA_HLEN);
+
 	return 0;
 }
 
diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
index e8eaa804ccb9..af16910a06c0 100644
--- a/net/dsa/tag_edsa.c
+++ b/net/dsa/tag_edsa.c
@@ -164,8 +164,8 @@ static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev,
 static int edsa_tag_flow_dissect(const struct sk_buff *skb, __be16 *proto,
 				 int *offset)
 {
-	*offset = 8;
-	*proto = ((__be16 *)skb->data)[3];
+	dsa_tag_generic_flow_dissect(skb, proto, offset, EDSA_HLEN);
+
 	return 0;
 }
 
diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index eb0e7a32e53d..2ec1c9092bb2 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -128,12 +128,21 @@ static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev,
 	return skb;
 }
 
+static int lan9303_flow_dissect(const struct sk_buff *skb, __be16 *proto,
+				int *offset)
+{
+	dsa_tag_generic_flow_dissect(skb, proto, offset, LAN9303_TAG_LEN);
+
+	return 0;
+}
+
 static const struct dsa_device_ops lan9303_netdev_ops = {
 	.name = "lan9303",
 	.proto	= DSA_TAG_PROTO_LAN9303,
 	.xmit = lan9303_xmit,
 	.rcv = lan9303_rcv,
 	.overhead = LAN9303_TAG_LEN,
+	.flow_dissect = lan9303_flow_dissect,
 };
 
 MODULE_LICENSE("GPL");
diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index b5705cba8318..e6cf8f68dffc 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c
@@ -92,8 +92,7 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 static int mtk_tag_flow_dissect(const struct sk_buff *skb, __be16 *proto,
 				int *offset)
 {
-	*offset = 4;
-	*proto = ((__be16 *)skb->data)[1];
+	dsa_tag_generic_flow_dissect(skb, proto, offset, MTK_HDR_LEN);
 
 	return 0;
 }
diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c
index 778a7f34f8ec..5a2de13b108f 100644
--- a/net/dsa/tag_ocelot.c
+++ b/net/dsa/tag_ocelot.c
@@ -238,12 +238,21 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
 	return skb;
 }
 
+static int ocelot_flow_dissect(const struct sk_buff *skb, __be16 *proto,
+			       int *offset)
+{
+	dsa_tag_generic_flow_dissect(skb, proto, offset, OCELOT_TOTAL_TAG_LEN);
+
+	return 0;
+}
+
 static struct dsa_device_ops ocelot_netdev_ops = {
 	.name			= "ocelot",
 	.proto			= DSA_TAG_PROTO_OCELOT,
 	.xmit			= ocelot_xmit,
 	.rcv			= ocelot_rcv,
 	.overhead		= OCELOT_TOTAL_TAG_LEN,
+	.flow_dissect		= ocelot_flow_dissect,
 };
 
 MODULE_LICENSE("GPL v2");
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index 70db7c909f74..43a5a4eebf16 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -90,8 +90,7 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 static int qca_tag_flow_dissect(const struct sk_buff *skb, __be16 *proto,
                                 int *offset)
 {
-	*offset = QCA_HDR_LEN;
-	*proto = ((__be16 *)skb->data)[0];
+	dsa_tag_generic_flow_dissect(skb, proto, offset, QCA_HDR_LEN);
 
 	return 0;
 }
diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index d553bf36bd41..55cc51a71b13 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -304,6 +304,18 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
 					      is_meta);
 }
 
+static int sja1105_flow_dissect(const struct sk_buff *skb, __be16 *proto,
+				int *offset)
+{
+	/* No tag added for management frames, all ok */
+	if (unlikely(sja1105_is_link_local(skb)))
+		return 0;
+
+	dsa_tag_generic_flow_dissect(skb, proto, offset, VLAN_HLEN);
+
+	return 0;
+}
+
 static struct dsa_device_ops sja1105_netdev_ops = {
 	.name = "sja1105",
 	.proto = DSA_TAG_PROTO_SJA1105,
@@ -311,6 +323,7 @@ static struct dsa_device_ops sja1105_netdev_ops = {
 	.rcv = sja1105_rcv,
 	.filter = sja1105_filter,
 	.overhead = VLAN_HLEN,
+	.flow_dissect = sja1105_flow_dissect,
 };
 
 MODULE_LICENSE("GPL v2");
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 3/4] net: dsa: tag_ocelot: use a short prefix on both ingress and egress
  2020-05-11 20:20 ` [PATCH net-next 3/4] net: dsa: tag_ocelot: use a short prefix on both ingress and egress Vladimir Oltean
@ 2020-05-11 22:40   ` Jakub Kicinski
  2020-05-11 22:44     ` Vladimir Oltean
  2020-05-11 23:22   ` Florian Fainelli
  1 sibling, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2020-05-11 22:40 UTC (permalink / raw)
  To: Vladimir Oltean; +Cc: andrew, f.fainelli, vivien.didelot, davem, netdev

On Mon, 11 May 2020 23:20:45 +0300 Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> There are 2 goals that we follow:
> 
> - Reduce the header size
> - Make the header size equal between RX and TX

Getting this from sparse:

../net/dsa/tag_ocelot.c:185:17: warning: incorrect type in assignment (different base types)
../net/dsa/tag_ocelot.c:185:17:    expected unsigned int [usertype]
../net/dsa/tag_ocelot.c:185:17:    got restricted __be32 [usertype]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 3/4] net: dsa: tag_ocelot: use a short prefix on both ingress and egress
  2020-05-11 22:40   ` Jakub Kicinski
@ 2020-05-11 22:44     ` Vladimir Oltean
  2020-05-11 23:11       ` David Miller
  0 siblings, 1 reply; 18+ messages in thread
From: Vladimir Oltean @ 2020-05-11 22:44 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot, David S. Miller, netdev

On Tue, 12 May 2020 at 01:40, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 11 May 2020 23:20:45 +0300 Vladimir Oltean wrote:
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> >
> > There are 2 goals that we follow:
> >
> > - Reduce the header size
> > - Make the header size equal between RX and TX
>
> Getting this from sparse:
>
> ../net/dsa/tag_ocelot.c:185:17: warning: incorrect type in assignment (different base types)
> ../net/dsa/tag_ocelot.c:185:17:    expected unsigned int [usertype]
> ../net/dsa/tag_ocelot.c:185:17:    got restricted __be32 [usertype]

I hate this warning :(

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 3/4] net: dsa: tag_ocelot: use a short prefix on both ingress and egress
  2020-05-11 22:44     ` Vladimir Oltean
@ 2020-05-11 23:11       ` David Miller
  2020-05-11 23:19         ` Vladimir Oltean
  0 siblings, 1 reply; 18+ messages in thread
From: David Miller @ 2020-05-11 23:11 UTC (permalink / raw)
  To: olteanv; +Cc: kuba, andrew, f.fainelli, vivien.didelot, netdev

From: Vladimir Oltean <olteanv@gmail.com>
Date: Tue, 12 May 2020 01:44:53 +0300

> On Tue, 12 May 2020 at 01:40, Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> On Mon, 11 May 2020 23:20:45 +0300 Vladimir Oltean wrote:
>> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
>> >
>> > There are 2 goals that we follow:
>> >
>> > - Reduce the header size
>> > - Make the header size equal between RX and TX
>>
>> Getting this from sparse:
>>
>> ../net/dsa/tag_ocelot.c:185:17: warning: incorrect type in assignment (different base types)
>> ../net/dsa/tag_ocelot.c:185:17:    expected unsigned int [usertype]
>> ../net/dsa/tag_ocelot.c:185:17:    got restricted __be32 [usertype]
> 
> I hate this warning :(

You hate that endianness bugs are caught automatically? :-)


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 4/4] net: dsa: implement and use a generic procedure for the flow dissector
  2020-05-11 20:20 ` [PATCH net-next 4/4] net: dsa: implement and use a generic procedure for the flow dissector Vladimir Oltean
@ 2020-05-11 23:15   ` Florian Fainelli
  2020-05-12  1:11     ` Vladimir Oltean
  0 siblings, 1 reply; 18+ messages in thread
From: Florian Fainelli @ 2020-05-11 23:15 UTC (permalink / raw)
  To: Vladimir Oltean, andrew, vivien.didelot; +Cc: davem, kuba, netdev



On 5/11/2020 1:20 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> For all DSA formats that don't use tail tags, it looks like behind the
> obscure number crunching they're all doing the same thing: locating the
> real EtherType behind the DSA tag. Nonetheless, this is not immediately
> obvious, so create a generic helper for those DSA taggers that put the
> header before the EtherType.
> 
> Another assumption for the generic function is that the DSA tags are of
> equal length on RX and on TX. Prior to the previous patch, this was not
> true for ocelot and for gswip. The problem was resolved for ocelot, but
> for gswip it still remains, so that hasn't been converted.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Given that __skb_flow_dissect() already de-references dsa_device_ops
from skb->dev->dsa_ptr, maybe we can go one step further and just have
dsa_tag_generic_flow_dissect obtain the overhead from the SKB directly
since this will already have touched the cache lines involved. This then
makes it unnecessary for the various taggers to specify a custom
function and instead, dsa_tag_generic_flow_dissect() can be assigned
where the dsa_device_ops are declared for the various tags. Did I miss
something?

It also looks like tag_ocelot.c and tag_sja1105.c should have their
dsa_device_ops structures const, as a separate patch certainly.
-- 
Florian

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 3/4] net: dsa: tag_ocelot: use a short prefix on both ingress and egress
  2020-05-11 23:11       ` David Miller
@ 2020-05-11 23:19         ` Vladimir Oltean
  0 siblings, 0 replies; 18+ messages in thread
From: Vladimir Oltean @ 2020-05-11 23:19 UTC (permalink / raw)
  To: David Miller
  Cc: Jakub Kicinski, Andrew Lunn, Florian Fainelli, Vivien Didelot, netdev

On Tue, 12 May 2020 at 02:11, David Miller <davem@davemloft.net> wrote:
>
> From: Vladimir Oltean <olteanv@gmail.com>
> Date: Tue, 12 May 2020 01:44:53 +0300
>
> > On Tue, 12 May 2020 at 01:40, Jakub Kicinski <kuba@kernel.org> wrote:
> >>
> >> On Mon, 11 May 2020 23:20:45 +0300 Vladimir Oltean wrote:
> >> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> >> >
> >> > There are 2 goals that we follow:
> >> >
> >> > - Reduce the header size
> >> > - Make the header size equal between RX and TX
> >>
> >> Getting this from sparse:
> >>
> >> ../net/dsa/tag_ocelot.c:185:17: warning: incorrect type in assignment (different base types)
> >> ../net/dsa/tag_ocelot.c:185:17:    expected unsigned int [usertype]
> >> ../net/dsa/tag_ocelot.c:185:17:    got restricted __be32 [usertype]
> >
> > I hate this warning :(
>
> You hate that endianness bugs are caught automatically? :-)
>

Well, there's no bug here, really, what's annoying is that it's
warning me when a convention is not being followed. It would have been
smarter if it just limited itself to the situations when not following
that convention actually causes a problem.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 1/4] net: dsa: allow drivers to request promiscuous mode on master
  2020-05-11 20:20 ` [PATCH net-next 1/4] net: dsa: allow drivers to request promiscuous mode on master Vladimir Oltean
@ 2020-05-11 23:19   ` Florian Fainelli
  0 siblings, 0 replies; 18+ messages in thread
From: Florian Fainelli @ 2020-05-11 23:19 UTC (permalink / raw)
  To: Vladimir Oltean, andrew, vivien.didelot; +Cc: davem, kuba, netdev



On 5/11/2020 1:20 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Currently DSA assumes that taggers don't mess with the destination MAC
> address of the frames on RX. That is not always the case. Some DSA
> headers are placed before the Ethernet header (ocelot), and others
> simply mangle random bytes from the destination MAC address (sja1105
> with its incl_srcpt option).
> 
> Currently the DSA master goes to promiscuous mode automatically when the
> slave devices go too (such as when enslaved to a bridge), but in
> standalone mode this is a problem that needs to be dealt with.
> 
> So give drivers the possibility to signal that their tagging protocol
> will get randomly dropped otherwise, and let DSA deal with fixing that.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 2/4] net: dsa: sja1105: request promiscuous mode for master
  2020-05-11 20:20 ` [PATCH net-next 2/4] net: dsa: sja1105: request promiscuous mode for master Vladimir Oltean
@ 2020-05-11 23:19   ` Florian Fainelli
  0 siblings, 0 replies; 18+ messages in thread
From: Florian Fainelli @ 2020-05-11 23:19 UTC (permalink / raw)
  To: Vladimir Oltean, andrew, vivien.didelot; +Cc: davem, kuba, netdev



On 5/11/2020 1:20 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Currently PTP is broken when ports are in standalone mode (the tagger
> keeps printing this message):
> 
> sja1105 spi0.1: Expected meta frame, is  180c200000e in the DSA master multicast filter?
> 
> Sure, one might say "simply add 01-80-c2-00-00-0e to the master's RX
> filter" but things become more complicated because:
> 
> - Actually all frames in the 01-80-c2-xx-xx-xx and 01-1b-19-xx-xx-xx
>   range are trapped to the CPU automatically
> - The switch mangles bytes 3 and 4 of the MAC address via the incl_srcpt
>   ("include source port [in the DMAC]") option, so an address installed
>   to the RX filter would, at the end of the day, not correspond to the
>   final address seen by the DSA master.
> 
> Assume RX filters on DSA masters are typically too small to include all
> necessary addresses for PTP to work properly on sja1105, and just
> request promiscuous mode unconditionally.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 3/4] net: dsa: tag_ocelot: use a short prefix on both ingress and egress
  2020-05-11 20:20 ` [PATCH net-next 3/4] net: dsa: tag_ocelot: use a short prefix on both ingress and egress Vladimir Oltean
  2020-05-11 22:40   ` Jakub Kicinski
@ 2020-05-11 23:22   ` Florian Fainelli
  1 sibling, 0 replies; 18+ messages in thread
From: Florian Fainelli @ 2020-05-11 23:22 UTC (permalink / raw)
  To: Vladimir Oltean, andrew, vivien.didelot; +Cc: davem, kuba, netdev



On 5/11/2020 1:20 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> There are 2 goals that we follow:
> 
> - Reduce the header size
> - Make the header size equal between RX and TX
> 
> The issue that required long prefix on RX was the fact that the ocelot
> DSA tag, being put before Ethernet as it is, would overlap with the area
> that a DSA master uses for RX filtering (destination MAC address
> mainly).
> 
> Now that we can ask DSA to put the master in promiscuous mode, in theory
> we could remove the prefix altogether and call it a day, but it looks
> like we can't. Using no prefix on ingress, some packets (such as ICMP)
> would be received, while others (such as PTP) would not be received.
> This is because the DSA master we use (enetc) triggers parse errors
> ("MAC rx frame errors") presumably because it sees Ethernet frames with
> a bad length. And indeed, when using no prefix, the EtherType (bytes
> 12-13 of the frame, bits 96-111) falls over the REW_VAL field from the
> extraction header, aka the PTP timestamp.
> 
> When turning the short (32-bit) prefix on, the EtherType overlaps with
> bits 64-79 of the extraction header, which are a reserved area
> transmitted as zero by the switch. The packets are not dropped by the
> DSA master with a short prefix. Actually, the frames look like this in
> tcpdump (below is a PTP frame, with an extra dsa_8021q tag - dadb 0482 -
> added by a downstream sja1105).
> 
> 89:0c:a9:f2:01:00 > 88:80:00:0a:00:1d, 802.3, length 0: LLC, \
> 	dsap Unknown (0x10) Individual, ssap ProWay NM (0x0e) Response, \
> 	ctrl 0x0004: Information, send seq 2, rcv seq 0, \
> 	Flags [Response], length 78
> 
> 0x0000:  8880 000a 001d 890c a9f2 0100 0000 100f  ................
> 0x0010:  0400 0000 0180 c200 000e 001f 7b63 0248  ............{c.H
> 0x0020:  dadb 0482 88f7 1202 0036 0000 0000 0000  .........6......
> 0x0030:  0000 0000 0000 0000 0000 001f 7bff fe63  ............{..c
> 0x0040:  0248 0001 1f81 0500 0000 0000 0000 0000  .H..............
> 0x0050:  0000 0000 0000 0000 0000 0000            ............
> 
> So the short prefix is our new default: we've shortened our RX frames by
> 12 octets, increased TX by 4, and headers are now equal between RX and
> TX. Note that we still need promiscuous mode for the DSA master to not
> drop it.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 0/4] DSA: promisc on master, generic flow dissector code
  2020-05-11 20:20 [PATCH net-next 0/4] DSA: promisc on master, generic flow dissector code Vladimir Oltean
                   ` (3 preceding siblings ...)
  2020-05-11 20:20 ` [PATCH net-next 4/4] net: dsa: implement and use a generic procedure for the flow dissector Vladimir Oltean
@ 2020-05-11 23:28 ` Florian Fainelli
  2020-05-11 23:52   ` Vladimir Oltean
  4 siblings, 1 reply; 18+ messages in thread
From: Florian Fainelli @ 2020-05-11 23:28 UTC (permalink / raw)
  To: Vladimir Oltean, andrew, vivien.didelot; +Cc: davem, kuba, netdev



On 5/11/2020 1:20 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> The initial purpose of this series was to implement the .flow_dissect
> method for sja1105 and for ocelot/felix. But on Felix this posed a
> problem, as the DSA headers were of different lengths on RX and on TX.
> A better solution than to just increase the smaller one was to also try
> to shrink the larger one, but in turn that required the DSA master to be
> put in promiscuous mode (which sja1105 also needed, for other reasons).
> 
> Finally, we can add the missing .flow_dissect methods to ocelot and
> sja1105 (as well as generalize the formula to other taggers as well).

On a separate note, do you have any systems for which it would be
desirable that the DSA standalone port implemented receive filtering? On
BCM7278 devices, the Ethernet MAC connected to the switch is always in
promiscuous mode, and so we rely on the switch not to flood the CPU port
unnecessarily with MC traffic (if nothing else), this is currently
implemented in our downstream kernel, but has not made it upstream yet,
previous attempt was here:

https://www.spinics.net/lists/netdev/msg544361.html

I would like to revisit that at some point.
-- 
Florian

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 0/4] DSA: promisc on master, generic flow dissector code
  2020-05-11 23:28 ` [PATCH net-next 0/4] DSA: promisc on master, generic flow dissector code Florian Fainelli
@ 2020-05-11 23:52   ` Vladimir Oltean
  2020-05-12  0:03     ` Florian Fainelli
  0 siblings, 1 reply; 18+ messages in thread
From: Vladimir Oltean @ 2020-05-11 23:52 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Andrew Lunn, Vivien Didelot, David S. Miller, Jakub Kicinski, netdev

On Tue, 12 May 2020 at 02:28, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 5/11/2020 1:20 PM, Vladimir Oltean wrote:
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> >
> > The initial purpose of this series was to implement the .flow_dissect
> > method for sja1105 and for ocelot/felix. But on Felix this posed a
> > problem, as the DSA headers were of different lengths on RX and on TX.
> > A better solution than to just increase the smaller one was to also try
> > to shrink the larger one, but in turn that required the DSA master to be
> > put in promiscuous mode (which sja1105 also needed, for other reasons).
> >
> > Finally, we can add the missing .flow_dissect methods to ocelot and
> > sja1105 (as well as generalize the formula to other taggers as well).
>
> On a separate note, do you have any systems for which it would be
> desirable that the DSA standalone port implemented receive filtering? On
> BCM7278 devices, the Ethernet MAC connected to the switch is always in
> promiscuous mode, and so we rely on the switch not to flood the CPU port
> unnecessarily with MC traffic (if nothing else), this is currently
> implemented in our downstream kernel, but has not made it upstream yet,
> previous attempt was here:
>
> https://www.spinics.net/lists/netdev/msg544361.html
>
> I would like to revisit that at some point.
> --
> Florian

Yes, CPU filtering of traffic (not just multicast) is one of the
problems we're facing. I'll take a look at your patches and maybe I'll
pick them up.

Thanks,
-Vladimir

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 0/4] DSA: promisc on master, generic flow dissector code
  2020-05-11 23:52   ` Vladimir Oltean
@ 2020-05-12  0:03     ` Florian Fainelli
  2020-05-14 15:27       ` Vladimir Oltean
  0 siblings, 1 reply; 18+ messages in thread
From: Florian Fainelli @ 2020-05-12  0:03 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Andrew Lunn, Vivien Didelot, David S. Miller, Jakub Kicinski, netdev



On 5/11/2020 4:52 PM, Vladimir Oltean wrote:
> On Tue, 12 May 2020 at 02:28, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>
>>
>> On 5/11/2020 1:20 PM, Vladimir Oltean wrote:
>>> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>>>
>>> The initial purpose of this series was to implement the .flow_dissect
>>> method for sja1105 and for ocelot/felix. But on Felix this posed a
>>> problem, as the DSA headers were of different lengths on RX and on TX.
>>> A better solution than to just increase the smaller one was to also try
>>> to shrink the larger one, but in turn that required the DSA master to be
>>> put in promiscuous mode (which sja1105 also needed, for other reasons).
>>>
>>> Finally, we can add the missing .flow_dissect methods to ocelot and
>>> sja1105 (as well as generalize the formula to other taggers as well).
>>
>> On a separate note, do you have any systems for which it would be
>> desirable that the DSA standalone port implemented receive filtering? On
>> BCM7278 devices, the Ethernet MAC connected to the switch is always in
>> promiscuous mode, and so we rely on the switch not to flood the CPU port
>> unnecessarily with MC traffic (if nothing else), this is currently
>> implemented in our downstream kernel, but has not made it upstream yet,
>> previous attempt was here:
>>
>> https://www.spinics.net/lists/netdev/msg544361.html
>>
>> I would like to revisit that at some point.
>> --
>> Florian
> 
> Yes, CPU filtering of traffic (not just multicast) is one of the
> problems we're facing. I'll take a look at your patches and maybe I'll
> pick them up.

The part that modifies DSA to program the known MC addresses should
still be largely applicable, there were essentially two problems that I
was facing, which could be tackled separately.

1) flooding of unknown MC traffic on DSA standalone ports is not very
intuitive if you come from NICs that did filtering before. We should
leverage a DSA switch driver's ability to support port_egress_floods and
support port_mdb_add and combine them to avoid flooding the CPU port.

2) Programming of known multicast addresses for VLAN devices on top of
DSA standalone ports while the switch implements global VLAN filtering.
In that case when we get to the DSA slave device's ndo_set_rx_mode() we
have lost all information about which VID the MAC address is coming from
so we cannot insert the MAC address with the correct VID to support
proper filtering. TI's cpsw driver implements a super complicated scheme
to solve that problem and this was worked on by Ivan in a more generic
and usable form: https://lwn.net/Articles/780783/
-- 
Florian

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 4/4] net: dsa: implement and use a generic procedure for the flow dissector
  2020-05-11 23:15   ` Florian Fainelli
@ 2020-05-12  1:11     ` Vladimir Oltean
  0 siblings, 0 replies; 18+ messages in thread
From: Vladimir Oltean @ 2020-05-12  1:11 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Andrew Lunn, Vivien Didelot, David S. Miller, Jakub Kicinski, netdev

On Tue, 12 May 2020 at 02:15, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 5/11/2020 1:20 PM, Vladimir Oltean wrote:
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> >
> > For all DSA formats that don't use tail tags, it looks like behind the
> > obscure number crunching they're all doing the same thing: locating the
> > real EtherType behind the DSA tag. Nonetheless, this is not immediately
> > obvious, so create a generic helper for those DSA taggers that put the
> > header before the EtherType.
> >
> > Another assumption for the generic function is that the DSA tags are of
> > equal length on RX and on TX. Prior to the previous patch, this was not
> > true for ocelot and for gswip. The problem was resolved for ocelot, but
> > for gswip it still remains, so that hasn't been converted.
> >
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> Given that __skb_flow_dissect() already de-references dsa_device_ops
> from skb->dev->dsa_ptr, maybe we can go one step further and just have
> dsa_tag_generic_flow_dissect obtain the overhead from the SKB directly
> since this will already have touched the cache lines involved. This then
> makes it unnecessary for the various taggers to specify a custom
> function and instead, dsa_tag_generic_flow_dissect() can be assigned
> where the dsa_device_ops are declared for the various tags. Did I miss
> something?
>
> It also looks like tag_ocelot.c and tag_sja1105.c should have their
> dsa_device_ops structures const, as a separate patch certainly.
> --
> Florian

Actually I wrote this patch in such a way that the assembly code
generated would remain the same, since dsa_tag_generic_flow_dissect is
inline, the arithmetic would be done at compile time, which it
wouldn't be if this function were to look at the tagger .overhead.
I don't know, I can do either way.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 0/4] DSA: promisc on master, generic flow dissector code
  2020-05-12  0:03     ` Florian Fainelli
@ 2020-05-14 15:27       ` Vladimir Oltean
  0 siblings, 0 replies; 18+ messages in thread
From: Vladimir Oltean @ 2020-05-14 15:27 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Andrew Lunn, Vivien Didelot, David S. Miller, Jakub Kicinski, netdev

Hi Florian,

On Tue, 12 May 2020 at 03:03, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 5/11/2020 4:52 PM, Vladimir Oltean wrote:
> > On Tue, 12 May 2020 at 02:28, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>
> >>
> >>
> >> On 5/11/2020 1:20 PM, Vladimir Oltean wrote:
> >>> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> >>>
> >>> The initial purpose of this series was to implement the .flow_dissect
> >>> method for sja1105 and for ocelot/felix. But on Felix this posed a
> >>> problem, as the DSA headers were of different lengths on RX and on TX.
> >>> A better solution than to just increase the smaller one was to also try
> >>> to shrink the larger one, but in turn that required the DSA master to be
> >>> put in promiscuous mode (which sja1105 also needed, for other reasons).
> >>>
> >>> Finally, we can add the missing .flow_dissect methods to ocelot and
> >>> sja1105 (as well as generalize the formula to other taggers as well).
> >>
> >> On a separate note, do you have any systems for which it would be
> >> desirable that the DSA standalone port implemented receive filtering? On
> >> BCM7278 devices, the Ethernet MAC connected to the switch is always in
> >> promiscuous mode, and so we rely on the switch not to flood the CPU port
> >> unnecessarily with MC traffic (if nothing else), this is currently
> >> implemented in our downstream kernel, but has not made it upstream yet,
> >> previous attempt was here:
> >>
> >> https://www.spinics.net/lists/netdev/msg544361.html
> >>
> >> I would like to revisit that at some point.
> >> --
> >> Florian
> >
> > Yes, CPU filtering of traffic (not just multicast) is one of the
> > problems we're facing. I'll take a look at your patches and maybe I'll
> > pick them up.
>
> The part that modifies DSA to program the known MC addresses should
> still be largely applicable, there were essentially two problems that I
> was facing, which could be tackled separately.
>
> 1) flooding of unknown MC traffic on DSA standalone ports is not very
> intuitive if you come from NICs that did filtering before. We should
> leverage a DSA switch driver's ability to support port_egress_floods and
> support port_mdb_add and combine them to avoid flooding the CPU port.
>

Could you clarify one thing for me:
- SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED is supposed to sync/unsync all
known multicast {mac, vid} addresses to the hardware filtering table
- SWITCHDEV_ATTR_ID_BRIDGE_MROUTER is supposed to toggle flooding of
unknown multicast addresses to the CPU
- What is BR_MCAST_FLOOD from SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS
supposed to do? The same thing?

> 2) Programming of known multicast addresses for VLAN devices on top of
> DSA standalone ports while the switch implements global VLAN filtering.
> In that case when we get to the DSA slave device's ndo_set_rx_mode() we
> have lost all information about which VID the MAC address is coming from
> so we cannot insert the MAC address with the correct VID to support
> proper filtering. TI's cpsw driver implements a super complicated scheme
> to solve that problem and this was worked on by Ivan in a more generic
> and usable form: https://lwn.net/Articles/780783/

So you're thinking of pulling his vlan_dev_get_addr_vid API and
syncing {DMAC, VID} addresses to the DSA slave ports by installing mdb
rules (or fdb in case of unicast filtering) on the CPU port?

But I think the only thing that would make global VLAN filtering more
complicated than the general case is that we would have to:
- deny switch ports from being enslaved to a VLAN-unaware bridge if
there is at least one other DSA slave in this switch that has
addresses with a non-pvid VLAN in its RX filter
- deny changes to the VLAN filtering state of any bridge that spans a
switch which has at least a port with a non-pvid VLAN in its RX filter
Am I missing something?

Also, for unicast filtering, I believe you would agree to leverage
port_egress_floods(unicast=false), and have the DSA core install one
fdb entry on the CPU port per each slave netdevice MAC address. Sadly,
for that to work, we'd have to keep our own parallel reference
counting in dsa_slave_sync_unsync_fdb_addr (because the refcount in
__hw_addr_sync_dev is per slave device and not per cpu port).

Last but not least, how do you see the CPU membership of VLANs problem
being approached? Using Ivan's vlan_dev_get_addr_vid API, only let the
CPU see traffic from a particular VLAN if there is any upper 8021q net
device installed on any DSA slave? What should happen when we bridge a
DSA slave port? Let the flood gates open? If the DSA slave is bridged
only with other DSA slaves from the same switch, hopefully the answer
is no. Hopefully we can open the flood gates only when bridging with a
foreign interface.

> --
> Florian

Thanks,
-Vladimir

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2020-05-14 15:27 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-11 20:20 [PATCH net-next 0/4] DSA: promisc on master, generic flow dissector code Vladimir Oltean
2020-05-11 20:20 ` [PATCH net-next 1/4] net: dsa: allow drivers to request promiscuous mode on master Vladimir Oltean
2020-05-11 23:19   ` Florian Fainelli
2020-05-11 20:20 ` [PATCH net-next 2/4] net: dsa: sja1105: request promiscuous mode for master Vladimir Oltean
2020-05-11 23:19   ` Florian Fainelli
2020-05-11 20:20 ` [PATCH net-next 3/4] net: dsa: tag_ocelot: use a short prefix on both ingress and egress Vladimir Oltean
2020-05-11 22:40   ` Jakub Kicinski
2020-05-11 22:44     ` Vladimir Oltean
2020-05-11 23:11       ` David Miller
2020-05-11 23:19         ` Vladimir Oltean
2020-05-11 23:22   ` Florian Fainelli
2020-05-11 20:20 ` [PATCH net-next 4/4] net: dsa: implement and use a generic procedure for the flow dissector Vladimir Oltean
2020-05-11 23:15   ` Florian Fainelli
2020-05-12  1:11     ` Vladimir Oltean
2020-05-11 23:28 ` [PATCH net-next 0/4] DSA: promisc on master, generic flow dissector code Florian Fainelli
2020-05-11 23:52   ` Vladimir Oltean
2020-05-12  0:03     ` Florian Fainelli
2020-05-14 15:27       ` 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.