[net-next,2/5] net: dsa: add out-of-band tagging protocol

Commit Message

Maxime Chevallier April 22, 2022, 6:03 p.m. UTC

This tagging protocol is designed for the situation where the link
between the MAC and the Switch is designed such that the Destination
Port, which is usually embedded in some part of the Ethernet Header, is
sent out-of-band, and isn't present at all in the Ethernet frame.

This can happen when the MAC and Switch are tightly integrated on an
SoC, as is the case with the Qualcomm IPQ4019 for example, where the DSA
tag is inserted directly into the DMA descriptors. In that case,
the MAC driver is responsible for sending the tag to the switch using
the out-of-band medium. To do so, the MAC driver needs to have the
information of the destination port for that skb.

This tagging protocol relies on a new set of fields in skb->shinfo to
transmit the dsa tagging information to and from the MAC driver.

Signed-off-by: Maxime Chevallier <[e-mail]>
---
 include/linux/skbuff.h |  7 +++++++
 include/net/dsa.h      |  2 ++
 net/dsa/Kconfig        |  7 +++++++
 net/dsa/Makefile       |  1 +
 net/dsa/tag_oob.c      | 45 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 62 insertions(+)
 create mode 100644 net/dsa/tag_oob.c

Comments

Florian Fainelli April 22, 2022, 6:28 p.m. UTC | #1

On 4/22/22 11:03, Maxime Chevallier wrote:
> This tagging protocol is designed for the situation where the link
> between the MAC and the Switch is designed such that the Destination
> Port, which is usually embedded in some part of the Ethernet Header, is
> sent out-of-band, and isn't present at all in the Ethernet frame.
> 
> This can happen when the MAC and Switch are tightly integrated on an
> SoC, as is the case with the Qualcomm IPQ4019 for example, where the DSA
> tag is inserted directly into the DMA descriptors. In that case,
> the MAC driver is responsible for sending the tag to the switch using
> the out-of-band medium. To do so, the MAC driver needs to have the
> information of the destination port for that skb.
> 
> This tagging protocol relies on a new set of fields in skb->shinfo to
> transmit the dsa tagging information to and from the MAC driver.
> 
> Signed-off-by: Maxime Chevallier <[e-mail]>

First off, I am not a big fan of expanding skb::shared_info because it 
is sensitive to cache line sizes and is critical for performance at much 
higher speeds, I would expect Eric and Jakub to not be terribly happy 
about it.

The Broadcom systemport (bcmsysport.c) has a mode where it can extract 
the Broadcom tag and put it in front of the actual packet contents which 
appears to be very similar here. From there on, you can have two strategies:

- have the Ethernet controller mangle the packet contents such that the 
QCA tag is located in front of the actual Ethernet frame and create a 
new tagging protocol variant for QCA, similar to the TAG_BRCM versus 
TAG_BRCM_PREPEND

- provide the necessary information for the tagger to work using an out 
of band mechanism, which is what you have done, in which case, maybe you 
can use skb->cb[] instead of using skb::shared_info?

Maxime Chevallier April 26, 2022, 1:57 p.m. UTC | #2

Hello Florian,

On Fri, 22 Apr 2022 11:28:30 -0700
Florian Fainelli <[e-mail]> wrote:

Thanks for the review :)

> On 4/22/22 11:03, Maxime Chevallier wrote:
> > This tagging protocol is designed for the situation where the link
> > between the MAC and the Switch is designed such that the Destination
> > Port, which is usually embedded in some part of the Ethernet
> > Header, is sent out-of-band, and isn't present at all in the
> > Ethernet frame.
> > 
> > This can happen when the MAC and Switch are tightly integrated on an
> > SoC, as is the case with the Qualcomm IPQ4019 for example, where
> > the DSA tag is inserted directly into the DMA descriptors. In that
> > case, the MAC driver is responsible for sending the tag to the
> > switch using the out-of-band medium. To do so, the MAC driver needs
> > to have the information of the destination port for that skb.
> > 
> > This tagging protocol relies on a new set of fields in skb->shinfo
> > to transmit the dsa tagging information to and from the MAC driver.
> > 
> > Signed-off-by: Maxime Chevallier <[e-mail]>  
> 
> First off, I am not a big fan of expanding skb::shared_info because
> it is sensitive to cache line sizes and is critical for performance
> at much higher speeds, I would expect Eric and Jakub to not be
> terribly happy about it.

No problem, I'm testing with the skb->cb approach as you suggested and
see how it goes.

> The Broadcom systemport (bcmsysport.c) has a mode where it can
> extract the Broadcom tag and put it in front of the actual packet
> contents which appears to be very similar here. From there on, you
> can have two strategies:
> 
> - have the Ethernet controller mangle the packet contents such that
> the QCA tag is located in front of the actual Ethernet frame and
> create a new tagging protocol variant for QCA, similar to the
> TAG_BRCM versus TAG_BRCM_PREPEND
> 
> - provide the necessary information for the tagger to work using an
> out of band mechanism, which is what you have done, in which case,
> maybe you can use skb->cb[] instead of using skb::shared_info?

One of the reason why I chose the second is to support possible future
cases where another controller would face a similar situation, and also
make use of the out-of-band tagger.

I understand that it's not very elegant in the sense that this breaks
the nice tagging model we have, but adding/removing data before the
payload also seems convoluted to achieve the same thing :) It seems
that this approach comes with a bit of an overhead since it implies
mangling the skb a bit, but I've yet to test this myself.

That's actually what I wanted your opinion on, it also seems like
Andrew likes the idea of putting the tag ahead of the frame to stick
with the actual model.

I don't have strong feelings myself on the way of doing this, I'm
looking for an approach that is efficient but yet easily maintainable.

Thanks,

Maxime

Vladimir Oltean April 26, 2022, 6:52 p.m. UTC | #3

Hi Maxime,

On Tue, Apr 26, 2022 at 03:57:32PM +0200, Maxime Chevallier wrote:
> > First off, I am not a big fan of expanding skb::shared_info because
> > it is sensitive to cache line sizes and is critical for performance
> > at much higher speeds, I would expect Eric and Jakub to not be
> > terribly happy about it.
> 
> No problem, I'm testing with the skb->cb approach as you suggested and
> see how it goes.
> 
> > The Broadcom systemport (bcmsysport.c) has a mode where it can
> > extract the Broadcom tag and put it in front of the actual packet
> > contents which appears to be very similar here. From there on, you
> > can have two strategies:
> > 
> > - have the Ethernet controller mangle the packet contents such that
> > the QCA tag is located in front of the actual Ethernet frame and
> > create a new tagging protocol variant for QCA, similar to the
> > TAG_BRCM versus TAG_BRCM_PREPEND
> > 
> > - provide the necessary information for the tagger to work using an
> > out of band mechanism, which is what you have done, in which case,
> > maybe you can use skb->cb[] instead of using skb::shared_info?
> 
> One of the reason why I chose the second is to support possible future
> cases where another controller would face a similar situation, and also
> make use of the out-of-band tagger.
> 
> I understand that it's not very elegant in the sense that this breaks
> the nice tagging model we have, but adding/removing data before the
> payload also seems convoluted to achieve the same thing :) It seems
> that this approach comes with a bit of an overhead since it implies
> mangling the skb a bit, but I've yet to test this myself.
> 
> That's actually what I wanted your opinion on, it also seems like
> Andrew likes the idea of putting the tag ahead of the frame to stick
> with the actual model.
> 
> I don't have strong feelings myself on the way of doing this, I'm
> looking for an approach that is efficient but yet easily maintainable.

The skb->cb is not free to use for passing data between the DSA master
and the switch driver. There's the qdisc layer on TX, GRO on RX, maybe
others, and these mangle that memory region. So that would make it a -1
for skb->cb, and a +1 from my side for making the DSA master driver push
a fake prepended header, and consuming it "as usual" from the DSA
tagging protocol driver. That, plus the hope that I'll be long dead by
the time we'd need to find a solution that scales to more switches
designed like this :)

I'll take a closer look at your patches a bit later too, probably not today,
don't wait for my feedback if you think you're otherwise ready to repost.

Patch

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 0ef11df1bc67..6f8012cf9246 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -685,6 +685,11 @@  int skb_zerocopy_iter_stream(struct sock *sk, struct sk_buff *skb,
 			     struct msghdr *msg, int len,
 			     struct ubuf_info *uarg);
 
+struct dsa_tag_info {
+	unsigned int proto;
+	unsigned int dp;
+};
+
 /* This data is invariant across clones and lives at
  * the end of the header data, ie. at skb->end.
  */
@@ -701,6 +706,8 @@  struct skb_shared_info {
 	unsigned int	gso_type;
 	u32		tskey;
 
+	struct dsa_tag_info dsa_tag_info;
+
 	/*
 	 * Warning : all fields before dataref are cleared in __alloc_skb()
 	 */
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 14e10cda7267..9951df858912 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -53,6 +53,7 @@  struct phylink_link_state;
 #define DSA_TAG_PROTO_SJA1110_VALUE		23
 #define DSA_TAG_PROTO_RTL8_4_VALUE		24
 #define DSA_TAG_PROTO_RTL8_4T_VALUE		25
+#define DSA_TAG_PROTO_OOB_VALUE			26
 
 enum dsa_tag_protocol {
 	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
@@ -81,6 +82,7 @@  enum dsa_tag_protocol {
 	DSA_TAG_PROTO_SJA1110		= DSA_TAG_PROTO_SJA1110_VALUE,
 	DSA_TAG_PROTO_RTL8_4		= DSA_TAG_PROTO_RTL8_4_VALUE,
 	DSA_TAG_PROTO_RTL8_4T		= DSA_TAG_PROTO_RTL8_4T_VALUE,
+	DSA_TAG_PROTO_OOB		= DSA_TAG_PROTO_OOB_VALUE,
 };
 
 struct dsa_switch;
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index 8cb87b5067ee..b7aa4d8552b2 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -57,6 +57,13 @@  config NET_DSA_TAG_HELLCREEK
 	  Say Y or M if you want to enable support for tagging frames
 	  for the Hirschmann Hellcreek TSN switches.
 
+config NET_DSA_TAG_OOB
+	tristate "Tag driver for Out-of-band tagging drivers"
+	help
+	  Say Y or M if you want to enable support for tagging out-of-band. In
+	  that case, the MAC driver becomes responsible for sending the tag to
+	  the switch, outside the inband data.
+
 config NET_DSA_TAG_GSWIP
 	tristate "Tag driver for Lantiq / Intel GSWIP switches"
 	help
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index 9f75820e7c98..b156e20f9c0a 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -9,6 +9,7 @@  obj-$(CONFIG_NET_DSA_TAG_BRCM_COMMON) += tag_brcm.o
 obj-$(CONFIG_NET_DSA_TAG_DSA_COMMON) += tag_dsa.o
 obj-$(CONFIG_NET_DSA_TAG_GSWIP) += tag_gswip.o
 obj-$(CONFIG_NET_DSA_TAG_HELLCREEK) += tag_hellcreek.o
+obj-$(CONFIG_NET_DSA_TAG_OOB) += tag_oob.o
 obj-$(CONFIG_NET_DSA_TAG_KSZ) += tag_ksz.o
 obj-$(CONFIG_NET_DSA_TAG_LAN9303) += tag_lan9303.o
 obj-$(CONFIG_NET_DSA_TAG_MTK) += tag_mtk.o
diff --git a/net/dsa/tag_oob.c b/net/dsa/tag_oob.c
new file mode 100644
index 000000000000..045c7c06e81f
--- /dev/null
+++ b/net/dsa/tag_oob.c
@@ -0,0 +1,45 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+
+/* Copyright (c) 2022, Maxime Chevallier <[e-mail]> */
+
+#include <linux/bitfield.h>
+
+#include "dsa_priv.h"
+
+static struct sk_buff *oob_tag_xmit(struct sk_buff *skb,
+				    struct net_device *dev)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct dsa_tag_info *tag_info = &skb_shinfo(skb)->dsa_tag_info;
+
+	tag_info->dp = BIT(dp->index);
+	tag_info->proto = DSA_TAG_PROTO_OOB;
+
+	return skb;
+}
+
+static struct sk_buff *oob_tag_rcv(struct sk_buff *skb,
+				   struct net_device *dev)
+{
+	struct dsa_tag_info *tag_info = &skb_shinfo(skb)->dsa_tag_info;
+
+	skb->dev = dsa_master_find_slave(dev, 0, tag_info->dp);
+	if (!skb->dev)
+		return NULL;
+
+	return skb;
+}
+
+const struct dsa_device_ops oob_tag_dsa_ops = {
+	.name	= "oob",
+	.proto	= DSA_TAG_PROTO_OOB,
+	.xmit	= oob_tag_xmit,
+	.rcv	= oob_tag_rcv,
+};
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("DSA tag driver for out-of-band tagging");
+MODULE_AUTHOR("Maxime Chevallier <[e-mail]>");
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_OOB);
+
+module_dsa_tag_driver(oob_tag_dsa_ops);