The switch that is present on the Renesas RZ/N1 SoC uses a specific
VLAN value followed by 6 bytes which contains forwarding configuration.
Signed-off-by: Clément Léger <[email protected]>
---
net/dsa/Kconfig | 8 +++
net/dsa/Makefile | 1 +
net/dsa/tag_rzn1_a5psw.c | 112 +++++++++++++++++++++++++++++++++++++++
3 files changed, 121 insertions(+)
create mode 100644 net/dsa/tag_rzn1_a5psw.c
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index 8cb87b5067ee..e5b17108fa22 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -132,6 +132,13 @@ config NET_DSA_TAG_RTL8_4
Say Y or M if you want to enable support for tagging frames for Realtek
switches with 8 byte protocol 4 tags, such as the Realtek RTL8365MB-VC.
+config NET_DSA_TAG_RZN1_A5PSW
+ tristate "Tag driver for Renesas RZ/N1 A5PSW switch"
+ help
+ Say Y or M if you want to enable support for tagging frames for
+ Renesas RZ/N1 embedded switch that uses a 8 byte tag located after
+ destination MAC address.
+
config NET_DSA_TAG_LAN9303
tristate "Tag driver for SMSC/Microchip LAN9303 family of switches"
help
@@ -159,4 +166,5 @@ config NET_DSA_TAG_XRS700X
Say Y or M if you want to enable support for tagging frames for
Arrow SpeedChips XRS700x switches that use a single byte tag trailer.
+
endif
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index 9f75820e7c98..af28c24ead18 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_NET_DSA_TAG_OCELOT_8021Q) += tag_ocelot_8021q.o
obj-$(CONFIG_NET_DSA_TAG_QCA) += tag_qca.o
obj-$(CONFIG_NET_DSA_TAG_RTL4_A) += tag_rtl4_a.o
obj-$(CONFIG_NET_DSA_TAG_RTL8_4) += tag_rtl8_4.o
+obj-$(CONFIG_NET_DSA_TAG_RZN1_A5PSW) += tag_rzn1_a5psw.o
obj-$(CONFIG_NET_DSA_TAG_SJA1105) += tag_sja1105.o
obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o
obj-$(CONFIG_NET_DSA_TAG_XRS700X) += tag_xrs700x.o
diff --git a/net/dsa/tag_rzn1_a5psw.c b/net/dsa/tag_rzn1_a5psw.c
new file mode 100644
index 000000000000..7818c1c0fca2
--- /dev/null
+++ b/net/dsa/tag_rzn1_a5psw.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 Schneider Electric
+ *
+ * Clément Léger <[email protected]>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/etherdevice.h>
+#include <net/dsa.h>
+
+#include "dsa_priv.h"
+
+/* To define the outgoing port and to discover the incoming port a TAG is
+ * inserted after Src MAC :
+ *
+ * Dest MAC Src MAC TAG Type
+ * ...| 1 2 3 4 5 6 | 1 2 3 4 5 6 | 1 2 3 4 5 6 7 8 | 1 2 |...
+ * |<--------------->|
+ *
+ * See struct a5psw_tag for layout
+ */
+
+#define A5PSW_TAG_VALUE 0xE001
+#define A5PSW_TAG_LEN 8
+#define A5PSW_CTRL_DATA_FORCE_FORWARD BIT(0)
+/* This is both used for xmit tag and rcv tagging */
+#define A5PSW_CTRL_DATA_PORT GENMASK(3, 0)
+
+struct a5psw_tag {
+ u16 ctrl_tag;
+ u16 ctrl_data;
+ u32 ctrl_data2;
+};
+
+static struct sk_buff *a5psw_tag_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+ struct a5psw_tag *ptag, tag = {0};
+ struct dsa_port *dp = dsa_slave_to_port(dev);
+ u32 data2_val;
+
+ /* The Ethernet switch we are interfaced with needs packets to be at
+ * least 64 bytes (including FCS) otherwise they will be discarded when
+ * they enter the switch port logic. When tagging is enabled, we need
+ * to make sure that packets are at least 68 bytes (including FCS and
+ * tag).
+ */
+ if (__skb_put_padto(skb, ETH_ZLEN + sizeof(tag), false))
+ return NULL;
+
+ /* provide 'A5PSW_TAG_LEN' bytes additional space */
+ skb_push(skb, A5PSW_TAG_LEN);
+
+ /* make room between MACs and Ether-Type to insert tag */
+ dsa_alloc_etype_header(skb, A5PSW_TAG_LEN);
+
+ ptag = dsa_etype_header_pos_tx(skb);
+
+ data2_val = FIELD_PREP(A5PSW_CTRL_DATA_PORT, BIT(dp->index));
+ tag.ctrl_tag = htons(A5PSW_TAG_VALUE);
+ tag.ctrl_data = htons(A5PSW_CTRL_DATA_FORCE_FORWARD);
+ tag.ctrl_data2 = htonl(data2_val);
+
+ memcpy(ptag, &tag, sizeof(struct a5psw_tag));
+
+ return skb;
+}
+
+static struct sk_buff *a5psw_tag_rcv(struct sk_buff *skb,
+ struct net_device *dev)
+{
+ struct a5psw_tag *tag;
+ int port;
+
+ if (unlikely(!pskb_may_pull(skb, A5PSW_TAG_LEN))) {
+ dev_warn_ratelimited(&dev->dev,
+ "Dropping packet, cannot pull\n");
+ return NULL;
+ }
+
+ tag = dsa_etype_header_pos_rx(skb);
+
+ if (tag->ctrl_tag != htons(A5PSW_TAG_VALUE)) {
+ dev_warn_ratelimited(&dev->dev, "Dropping packet due to invalid TAG marker\n");
+ return NULL;
+ }
+
+ port = FIELD_GET(A5PSW_CTRL_DATA_PORT, ntohs(tag->ctrl_data));
+
+ skb->dev = dsa_master_find_slave(dev, 0, port);
+ if (!skb->dev)
+ return NULL;
+
+ skb_pull_rcsum(skb, A5PSW_TAG_LEN);
+ dsa_strip_etype_header(skb, A5PSW_TAG_LEN);
+
+ dsa_default_offload_fwd_mark(skb);
+
+ return skb;
+}
+
+static const struct dsa_device_ops a5psw_netdev_ops = {
+ .name = "a5psw",
+ .proto = DSA_TAG_PROTO_RZN1_A5PSW,
+ .xmit = a5psw_tag_xmit,
+ .rcv = a5psw_tag_rcv,
+ .needed_headroom = A5PSW_TAG_LEN,
+};
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_A5PSW);
+module_dsa_tag_driver(a5psw_netdev_ops);
--
2.34.1
Le Thu, 14 Apr 2022 17:23:46 +0100,
"Russell King (Oracle)" <[email protected]> a écrit :
> On Thu, Apr 14, 2022 at 06:18:15PM +0200, Clément Léger wrote:
> > Le Thu, 14 Apr 2022 18:11:46 +0300,
> > Vladimir Oltean <[email protected]> a écrit :
> >
> > > On Thu, Apr 14, 2022 at 04:35:46PM +0200, Clément Léger wrote:
> > > > > Please keep variable declarations sorted in decreasing order of line
> > > > > length (applies throughout the patch series, I won't repeat this comment).
> > > >
> > > > Acked, both PCS and DSA driver are ok with that rule. Missed that one
> > > > though.
> > >
> > > Are you sure? Because a5psw_port_stp_state_set() says otherwise.
> >
> > Weeeeell, ok let's say I missed these two. Would be useful to have such
> > checks in checkpatch.pl.
>
> Note that it's a local networking coding-style issue, rather than being
> kernel-wide.
>
Hi Russell, Yes I was aware of that but if I remember correctly, there
are some netowrking checks like multi line comments without an empty
first line in checkpatch. Anyway, I'll make sure to check that mroe
carefully next time.
--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com
Le Thu, 14 Apr 2022 18:11:46 +0300,
Vladimir Oltean <[email protected]> a écrit :
> On Thu, Apr 14, 2022 at 04:35:46PM +0200, Clément Léger wrote:
> > > Please keep variable declarations sorted in decreasing order of line
> > > length (applies throughout the patch series, I won't repeat this comment).
> >
> > Acked, both PCS and DSA driver are ok with that rule. Missed that one
> > though.
>
> Are you sure? Because a5psw_port_stp_state_set() says otherwise.
Weeeeell, ok let's say I missed these two. Would be useful to have such
checks in checkpatch.pl.
>
> > > sizeof(tag), to be consistent with the other use of sizeof() above?
> > > Although, hmm, I think you could get away with editing "ptag" in place.
> >
> > I was not sure of the alignment guarantee I would have here. If the
> > VLAN header is guaranteed to be aligned on 2 bytes, then I guess it's
> > ok to do that in-place.
>
> If I look at Documentation/core-api/unaligned-memory-access.rst
>
> | Alignment vs. Networking
> | ========================
> |
> | On architectures that require aligned loads, networking requires that the IP
> | header is aligned on a four-byte boundary to optimise the IP stack. For
> | regular ethernet hardware, the constant NET_IP_ALIGN is used. On most
> | architectures this constant has the value 2 because the normal ethernet
> | header is 14 bytes long, so in order to get proper alignment one needs to
> | DMA to an address which can be expressed as 4*n + 2. One notable exception
> | here is powerpc which defines NET_IP_ALIGN to 0 because DMA to unaligned
> | addresses can be very expensive and dwarf the cost of unaligned loads.
>
> Your struct a5psw_tag *ptag starts at 10 bytes (8 for tag, 2 for Ethertype)
> before the IP header, so I'm thinking it is aligned at a 2 byte boundary
> as well. A VLAN header between the DSA header and the IP header should
> also not affect that alignment, since its length is 4 bytes.
>
> If "ctrl_tag" is aligned at a 4 byte boundary - 10, it means "ctrl_data"
> is aligned at a 4 byte boundary - 8, so also a 4 byte boundary.
>
> But "ctrl_data2" is aligned at a 4 byte boundary + 2, so you might want
> to break up the u32 access into 2 u16 accesses, just to be on the safe
> side?
Thanks for finding these, looks like a good compromise, let's go that
way then.
--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com
On Thu, Apr 14, 2022 at 04:35:46PM +0200, Cl?ment L?ger wrote:
> > Please keep variable declarations sorted in decreasing order of line
> > length (applies throughout the patch series, I won't repeat this comment).
>
> Acked, both PCS and DSA driver are ok with that rule. Missed that one
> though.
Are you sure? Because a5psw_port_stp_state_set() says otherwise.
> > sizeof(tag), to be consistent with the other use of sizeof() above?
> > Although, hmm, I think you could get away with editing "ptag" in place.
>
> I was not sure of the alignment guarantee I would have here. If the
> VLAN header is guaranteed to be aligned on 2 bytes, then I guess it's
> ok to do that in-place.
If I look at Documentation/core-api/unaligned-memory-access.rst
| Alignment vs. Networking
| ========================
|
| On architectures that require aligned loads, networking requires that the IP
| header is aligned on a four-byte boundary to optimise the IP stack. For
| regular ethernet hardware, the constant NET_IP_ALIGN is used. On most
| architectures this constant has the value 2 because the normal ethernet
| header is 14 bytes long, so in order to get proper alignment one needs to
| DMA to an address which can be expressed as 4*n + 2. One notable exception
| here is powerpc which defines NET_IP_ALIGN to 0 because DMA to unaligned
| addresses can be very expensive and dwarf the cost of unaligned loads.
Your struct a5psw_tag *ptag starts at 10 bytes (8 for tag, 2 for Ethertype)
before the IP header, so I'm thinking it is aligned at a 2 byte boundary
as well. A VLAN header between the DSA header and the IP header should
also not affect that alignment, since its length is 4 bytes.
If "ctrl_tag" is aligned at a 4 byte boundary - 10, it means "ctrl_data"
is aligned at a 4 byte boundary - 8, so also a 4 byte boundary.
But "ctrl_data2" is aligned at a 4 byte boundary + 2, so you might want
to break up the u32 access into 2 u16 accesses, just to be on the safe
side?
On Thu, Apr 14, 2022 at 02:22:40PM +0200, Cl?ment L?ger wrote:
> The switch that is present on the Renesas RZ/N1 SoC uses a specific
> VLAN value followed by 6 bytes which contains forwarding configuration.
>
> Signed-off-by: Cl?ment L?ger <[email protected]>
> ---
> net/dsa/Kconfig | 8 +++
> net/dsa/Makefile | 1 +
> net/dsa/tag_rzn1_a5psw.c | 112 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 121 insertions(+)
> create mode 100644 net/dsa/tag_rzn1_a5psw.c
>
> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index 8cb87b5067ee..e5b17108fa22 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -132,6 +132,13 @@ config NET_DSA_TAG_RTL8_4
> Say Y or M if you want to enable support for tagging frames for Realtek
> switches with 8 byte protocol 4 tags, such as the Realtek RTL8365MB-VC.
>
> +config NET_DSA_TAG_RZN1_A5PSW
> + tristate "Tag driver for Renesas RZ/N1 A5PSW switch"
> + help
> + Say Y or M if you want to enable support for tagging frames for
> + Renesas RZ/N1 embedded switch that uses a 8 byte tag located after
> + destination MAC address.
> +
> config NET_DSA_TAG_LAN9303
> tristate "Tag driver for SMSC/Microchip LAN9303 family of switches"
> help
> @@ -159,4 +166,5 @@ config NET_DSA_TAG_XRS700X
> Say Y or M if you want to enable support for tagging frames for
> Arrow SpeedChips XRS700x switches that use a single byte tag trailer.
>
> +
> endif
> diff --git a/net/dsa/Makefile b/net/dsa/Makefile
> index 9f75820e7c98..af28c24ead18 100644
> --- a/net/dsa/Makefile
> +++ b/net/dsa/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_NET_DSA_TAG_OCELOT_8021Q) += tag_ocelot_8021q.o
> obj-$(CONFIG_NET_DSA_TAG_QCA) += tag_qca.o
> obj-$(CONFIG_NET_DSA_TAG_RTL4_A) += tag_rtl4_a.o
> obj-$(CONFIG_NET_DSA_TAG_RTL8_4) += tag_rtl8_4.o
> +obj-$(CONFIG_NET_DSA_TAG_RZN1_A5PSW) += tag_rzn1_a5psw.o
> obj-$(CONFIG_NET_DSA_TAG_SJA1105) += tag_sja1105.o
> obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o
> obj-$(CONFIG_NET_DSA_TAG_XRS700X) += tag_xrs700x.o
> diff --git a/net/dsa/tag_rzn1_a5psw.c b/net/dsa/tag_rzn1_a5psw.c
> new file mode 100644
> index 000000000000..7818c1c0fca2
> --- /dev/null
> +++ b/net/dsa/tag_rzn1_a5psw.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 Schneider Electric
> + *
> + * Cl?ment L?ger <[email protected]>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/etherdevice.h>
> +#include <net/dsa.h>
> +
> +#include "dsa_priv.h"
> +
> +/* To define the outgoing port and to discover the incoming port a TAG is
> + * inserted after Src MAC :
> + *
> + * Dest MAC Src MAC TAG Type
> + * ...| 1 2 3 4 5 6 | 1 2 3 4 5 6 | 1 2 3 4 5 6 7 8 | 1 2 |...
> + * |<--------------->|
> + *
> + * See struct a5psw_tag for layout
> + */
> +
> +#define A5PSW_TAG_VALUE 0xE001
> +#define A5PSW_TAG_LEN 8
> +#define A5PSW_CTRL_DATA_FORCE_FORWARD BIT(0)
> +/* This is both used for xmit tag and rcv tagging */
> +#define A5PSW_CTRL_DATA_PORT GENMASK(3, 0)
> +
> +struct a5psw_tag {
> + u16 ctrl_tag;
> + u16 ctrl_data;
> + u32 ctrl_data2;
> +};
> +
> +static struct sk_buff *a5psw_tag_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> + struct a5psw_tag *ptag, tag = {0};
> + struct dsa_port *dp = dsa_slave_to_port(dev);
> + u32 data2_val;
It might be worth adding a BUILD_BUG_ON(sizeof(tag) != A5PSW_TAG_LEN);
That does not cost anything at runtime, and avoids hard to find bugs
when the compiler does not do what you expect in terms of packing.
Andrew
On Thu, Apr 14, 2022 at 06:18:15PM +0200, Cl?ment L?ger wrote:
> Le Thu, 14 Apr 2022 18:11:46 +0300,
> Vladimir Oltean <[email protected]> a ?crit :
>
> > On Thu, Apr 14, 2022 at 04:35:46PM +0200, Cl?ment L?ger wrote:
> > > > Please keep variable declarations sorted in decreasing order of line
> > > > length (applies throughout the patch series, I won't repeat this comment).
> > >
> > > Acked, both PCS and DSA driver are ok with that rule. Missed that one
> > > though.
> >
> > Are you sure? Because a5psw_port_stp_state_set() says otherwise.
>
> Weeeeell, ok let's say I missed these two. Would be useful to have such
> checks in checkpatch.pl.
Note that it's a local networking coding-style issue, rather than being
kernel-wide.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!