On Tue, Nov 10, 2020 at 10:13:25AM +0100, Tobias Waldekranz wrote:
> Ethertype DSA encodes exactly the same information in the DSA tag as
> the non-ethertype variety. So refactor out the common parts and reuse
> them for both protocols.
>
> This is ensures tag parsing and generation as always consistent across
> all mv88e6xxx chips.
>
> While we are at it, explicitly deal with all possible CPU codes on
> receive, making sure to set offload_fwd_mark as appropriate.
>
> Signed-off-by: Tobias Waldekranz <tob...@waldekranz.com>
> ---
>
> I've tried to verify all dimensions of this: Rx of TO_CPU and FORWARD,
> Tx of FROM_CPU, both tagged and untagged, from both the first and a
> cascaded chip.
>
> Tested on:
> 1. 6352+6097F
> 2. 6390X
>
> net/dsa/Kconfig | 6 +
> net/dsa/Makefile | 3 +-
> net/dsa/tag_dsa.c | 299 ++++++++++++++++++++++++++++++++++++---------
> net/dsa/tag_edsa.c | 202 ------------------------------
> 4 files changed, 245 insertions(+), 265 deletions(-)
> delete mode 100644 net/dsa/tag_edsa.c
>
> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index d975614f7dd6..42d8ad84f92f 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -68,14 +68,20 @@ config NET_DSA_TAG_GSWIP
> Say Y or M if you want to enable support for tagging frames for the
> Lantiq / Intel GSWIP switches.
>
> +config NET_DSA_TAG_DSA_COMMON
> + tristate
> + default n
I think that "default n" is implicit and should be omitted.
> +
> config NET_DSA_TAG_DSA
> tristate "Tag driver for Marvell switches using DSA headers"
> + select NET_DSA_TAG_DSA_COMMON
> help
> Say Y or M if you want to enable support for tagging frames for the
> Marvell switches which use DSA headers.
>
> config NET_DSA_TAG_EDSA
> tristate "Tag driver for Marvell switches using EtherType DSA headers"
> + select NET_DSA_TAG_DSA_COMMON
> help
> Say Y or M if you want to enable support for tagging frames for the
> Marvell switches which use EtherType DSA headers.
> diff --git a/net/dsa/Makefile b/net/dsa/Makefile
> index e25d5457964a..0fb2b75a7ae3 100644
> --- a/net/dsa/Makefile
> +++ b/net/dsa/Makefile
> @@ -7,8 +7,7 @@ dsa_core-y += dsa.o dsa2.o master.o port.o slave.o switch.o
> obj-$(CONFIG_NET_DSA_TAG_8021Q) += tag_8021q.o
> obj-$(CONFIG_NET_DSA_TAG_AR9331) += tag_ar9331.o
> obj-$(CONFIG_NET_DSA_TAG_BRCM_COMMON) += tag_brcm.o
> -obj-$(CONFIG_NET_DSA_TAG_DSA) += tag_dsa.o
> -obj-$(CONFIG_NET_DSA_TAG_EDSA) += tag_edsa.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_KSZ) += tag_ksz.o
> diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
> index 63d690a0fca6..b8ee852a46d8 100644
> --- a/net/dsa/tag_dsa.c
> +++ b/net/dsa/tag_dsa.c
> @@ -1,7 +1,47 @@
> // SPDX-License-Identifier: GPL-2.0+
> /*
> - * net/dsa/tag_dsa.c - (Non-ethertype) DSA tagging
> + * Regular and Ethertype DSA tagging
> * Copyright (c) 2008-2009 Marvell Semiconductor
> + *
> + * Regular DSA
> + * -----------
> + * For untagged (in 802.1Q terms) packes, the swich will splice in the
> + * tag between the SA and the ethertype of the original packet. Tagged
> + * frames will instead have their outermost .1Q tag converted to a DSA
> + * tag. It expects the same layout when receiving packets from the
> + * CPU.
> + *
> + * Example:
> + *
> + * .----.----.----.---------
> + * Pu: | DA | SA | ET | Payload ...
> + * '----'----'----'---------
> + * 6 6 2 N
> + * .----.----.--------.-----.----.---------
> + * Pt: | DA | SA | 0x8100 | TCI | ET | Payload ...
> + * '----'----'--------'-----'----'---------
> + * 6 6 2 2 2 N
> + * .----.----.-----.----.---------
> + * Pd: | DA | SA | DSA | ET | Payload ...
> + * '----'----'-----'----'---------
> + * 6 6 4 2 N
> + *
> + * No matter if a packet is received untagged (Pu) or tagged (Pt),
> + * they will both have the same layout (Pd) when they are sent to the
> + * CPU. This is done by ignoring 802.3, replacing the ethertype field
> + * with more metadata, among which is a bit to signal if the original
> + * packet was tagged or not.
> + *
> + * Ethertype DSA
> + * -------------
> + * Uses the exact same tag format as regular DSA, but also includes a
> + * proper ethertype field (which the mv88e6xxx driver sets to
> + * ETH_P_EDSA/0xdada) followed by two zero bytes:
> + *
> + * .----.----.--------.--------.-----.----.---------
> + * | DA | SA | 0xdada | 0x0000 | DSA | ET | Payload ...
> + * '----'----'--------'--------'-----'----'---------
> + * 6 6 2 2 4 2 N
> */
>
> #include <linux/etherdevice.h>
> @@ -10,43 +50,79 @@
>
> #include "dsa_priv.h"
>
> -#define DSA_HLEN 4
> +#if IS_ENABLED(CONFIG_NET_DSA_TAG_DSA_COMMON)
>
> -static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
> +#define DSA_HLEN 4
> +
> +/**
> + * enum dsa_cmd - DSA Command
> + * @DSA_CMD_TO_CPU: Set on packets that where trapped or mirrored to
s/where/were/
> + * the CPU port. This is needed to implement control protocols,
> + * e.g. STP and LLDP, that must not allow those control packets to
> + * be switched according to the normal rules.
> + * @DSA_CMD_FROM_CPU: Used by the CPU to send a packet to a specific
> + * port, ignoring all the barriers that the switch normally
> + * enforces (VLANs, STP port states etc.). "sudo send packet"
> + * @DSA_CMD_TO_SNIFFER: Set on packets that where mirrored to the CPU
> + * as a result of matching some user configured ingress or egress
> + * monitor criteria.
> + * @DSA_CMD_FORWARD: Everything else, i.e. the bulk data traffic.
> + */
> +enum dsa_cmd {
> + DSA_CMD_TO_CPU = 0,
> + DSA_CMD_FROM_CPU = 1,
> + DSA_CMD_TO_SNIFFER = 2,
> + DSA_CMD_FORWARD = 3
> +};
> +
> +/**
> + * enum dsa_code - TO_CPU Code
> + *
> + * A 3-bit code is used to relay why a particular frame was sent to
> + * the CPU. We only use this to determine if the packet was trapped or
> + * mirrored, i.e. whether the packet has been forwarded by hardware or
> + * not.
> + */
> +enum dsa_code {
> + DSA_CODE_MGMT_TRAP = 0,
> + DSA_CODE_FRAME2REG = 1,
> + DSA_CODE_IGMP_MLD_TRAP = 2,
> + DSA_CODE_POLICY_TRAP = 3,
> + DSA_CODE_ARP_MIRROR = 4,
> + DSA_CODE_POLICY_MIRROR = 5,
> + DSA_CODE_RESERVED_6 = 6,
> + DSA_CODE_RESERVED_7 = 7
> +};
> +
> +static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device
> *dev,
> + u8 extra)
> {
> struct dsa_port *dp = dsa_slave_to_port(dev);
> u8 *dsa_header;
>
> - /*
> - * Convert the outermost 802.1q tag to a DSA tag for tagged
> - * packets, or insert a DSA tag between the addresses and
> - * the ethertype field for untagged packets.
> - */
> if (skb->protocol == htons(ETH_P_8021Q)) {
> - /*
> - * Construct tagged FROM_CPU DSA tag from 802.1q tag.
> - */
> - dsa_header = skb->data + 2 * ETH_ALEN;
> - dsa_header[0] = 0x60 | dp->ds->index;
> + if (extra) {
> + skb_push(skb, extra);
> + memmove(skb->data, skb->data + extra, 2 * ETH_ALEN);
> + }
> +
> + /* Construct tagged FROM_CPU DSA tag from 802.1Q tag. */
> + dsa_header = skb->data + 2 * ETH_ALEN + extra;
> + dsa_header[0] = (DSA_CMD_FROM_CPU << 6) | 0x20 | dp->ds->index;
What is 0x20, BIT(5)? To denote that it's an 802.1Q tagged frame I suppose?
Could it have a macro?
> dsa_header[1] = dp->index << 3;
>
> - /*
> - * Move CFI field from byte 2 to byte 1.
> - */
> + /* Move CFI field from byte 2 to byte 1. */
> if (dsa_header[2] & 0x10) {
> dsa_header[1] |= 0x01;
> dsa_header[2] &= ~0x10;
> }
> } else {
> - skb_push(skb, DSA_HLEN);
> -
> - memmove(skb->data, skb->data + DSA_HLEN, 2 * ETH_ALEN);
> + skb_push(skb, DSA_HLEN + extra);
> + memmove(skb->data, skb->data + DSA_HLEN + extra, 2 * ETH_ALEN);
>
> - /*
> - * Construct untagged FROM_CPU DSA tag.
> - */
> - dsa_header = skb->data + 2 * ETH_ALEN;
> - dsa_header[0] = 0x40 | dp->ds->index;
> + /* Construct untagged FROM_CPU DSA tag. */
> + dsa_header = skb->data + 2 * ETH_ALEN + extra;
> + dsa_header[0] = (DSA_CMD_FROM_CPU << 6) | dp->ds->index;
> dsa_header[1] = dp->index << 3;
> dsa_header[2] = 0x00;
> dsa_header[3] = 0x00;
> @@ -55,30 +131,62 @@ static struct sk_buff *dsa_xmit(struct sk_buff *skb,
> struct net_device *dev)
> return skb;
> }
>
> -static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev,
> - struct packet_type *pt)
> +static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device
> *dev,
> + u8 extra)
> {
> + int source_device, source_port;
> + enum dsa_code code;
> + enum dsa_cmd cmd;
> u8 *dsa_header;
> - int source_device;
> - int source_port;
>
> - if (unlikely(!pskb_may_pull(skb, DSA_HLEN)))
> - return NULL;
> -
> - /*
> - * The ethertype field is part of the DSA header.
> - */
> + /* The ethertype field is part of the DSA header. */
Could these comment style changes be a separate patch?
> dsa_header = skb->data - 2;
>
> - /*
> - * Check that frame type is either TO_CPU or FORWARD.
> - */
> - if ((dsa_header[0] & 0xc0) != 0x00 && (dsa_header[0] & 0xc0) != 0xc0)
> + cmd = dsa_header[0] >> 6;
> + switch (cmd) {
> + case DSA_CMD_FORWARD:
> + skb->offload_fwd_mark = 1;
> + break;
> +
> + case DSA_CMD_TO_CPU:
> + code = (dsa_header[1] & 0x6) | ((dsa_header[2] >> 4) & 1);
> +
> + switch (code) {
> + case DSA_CODE_FRAME2REG:
> + /* Remote management frames originate from the
> + * switch itself, there is no DSA port for us
> + * to ingress it on (the port field is always
> + * 0 in these tags).
> + */
> + return NULL;
> + case DSA_CODE_ARP_MIRROR:
> + case DSA_CODE_POLICY_MIRROR:
> + /* Mark mirrored packets to notify any upper
> + * 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:
> + case DSA_CODE_POLICY_TRAP:
> + /* Traps have, by definition, not been
> + * forwarded by hardware, so don't mark them.
> + */
> + break;
> + default:
> + /* Reserved code, this could be anything. Drop
> + * seems like the safest option.
> + */
> + return NULL;
> + }
> +
> + break;
> +
> + default:
> return NULL;
> + }
>
> - /*
> - * Determine source device and port.
> - */
> source_device = dsa_header[0] & 0x1f;
> source_port = (dsa_header[1] >> 3) & 0x1f;
>
> @@ -86,16 +194,15 @@ static struct sk_buff *dsa_rcv(struct sk_buff *skb,
> struct net_device *dev,
> if (!skb->dev)
> return NULL;
>
> - /*
> - * Convert the DSA header to an 802.1q header if the 'tagged'
> - * bit in the DSA header is set. If the 'tagged' bit is clear,
> - * delete the DSA header entirely.
> + /* 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
> + * if applicable.
> */
> if (dsa_header[0] & 0x20) {
> u8 new_header[4];
>
> - /*
> - * Insert 802.1q ethertype and copy the VLAN-related
> + /* Insert 802.1Q ethertype and copy the VLAN-related
> * fields, but clear the bit that will hold CFI (since
> * DSA uses that bit location for another purpose).
> */
> @@ -104,16 +211,13 @@ static struct sk_buff *dsa_rcv(struct sk_buff *skb,
> struct net_device *dev,
> new_header[2] = dsa_header[2] & ~0x10;
> new_header[3] = dsa_header[3];
>
> - /*
> - * Move CFI bit from its place in the DSA header to
> - * its 802.1q-designated place.
> + /* Move CFI bit from its place in the DSA header to
> + * its 802.1Q-designated place.
> */
> if (dsa_header[1] & 0x01)
> new_header[2] |= 0x10;
>
> - /*
> - * Update packet checksum if skb is CHECKSUM_COMPLETE.
> - */
> + /* Update packet checksum if skb is CHECKSUM_COMPLETE. */
> if (skb->ip_summed == CHECKSUM_COMPLETE) {
> __wsum c = skb->csum;
> c = csum_add(c, csum_partial(new_header + 2, 2, 0));
> @@ -122,21 +226,39 @@ static struct sk_buff *dsa_rcv(struct sk_buff *skb,
> struct net_device *dev,
> }
>
> memcpy(dsa_header, new_header, DSA_HLEN);
> +
> + if (extra)
> + memmove(skb->data - ETH_HLEN,
> + skb->data - ETH_HLEN - extra,
> + 2 * ETH_ALEN);
> } else {
> - /*
> - * Remove DSA tag and update checksum.
> - */
> skb_pull_rcsum(skb, DSA_HLEN);
> memmove(skb->data - ETH_HLEN,
> - skb->data - ETH_HLEN - DSA_HLEN,
> + skb->data - ETH_HLEN - DSA_HLEN - extra,
> 2 * ETH_ALEN);
> }
>
> - skb->offload_fwd_mark = 1;
> -
> return skb;
> }
>
> +#endif /* CONFIG_NET_DSA_TAG_DSA_COMMON */
> +
> +#if IS_ENABLED(CONFIG_NET_DSA_TAG_DSA)
> +
> +static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> + return dsa_xmit_ll(skb, dev, 0);
> +}
> +
> +static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev,
> + struct packet_type *pt)
> +{
> + if (unlikely(!pskb_may_pull(skb, DSA_HLEN)))
> + return NULL;
> +
> + return dsa_rcv_ll(skb, dev, 0);
> +}
> +
> static const struct dsa_device_ops dsa_netdev_ops = {
> .name = "dsa",
> .proto = DSA_TAG_PROTO_DSA,
> @@ -145,7 +267,62 @@ static const struct dsa_device_ops dsa_netdev_ops = {
> .overhead = DSA_HLEN,
> };
>
> -MODULE_LICENSE("GPL");
> +DSA_TAG_DRIVER(dsa_netdev_ops);
> MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_DSA);
> +#endif /* CONFIG_NET_DSA_TAG_DSA */
> +
> +#if IS_ENABLED(CONFIG_NET_DSA_TAG_EDSA)
>
> -module_dsa_tag_driver(dsa_netdev_ops);
> +#define EDSA_HLEN 8
> +
> +static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> + u8 *edsa_header;
> +
> + skb = dsa_xmit_ll(skb, dev, EDSA_HLEN - DSA_HLEN);
> + if (!skb)
> + return NULL;
> +
> + edsa_header = skb->data + 2 * ETH_ALEN;
> + edsa_header[0] = (ETH_P_EDSA >> 8) & 0xff;
> + edsa_header[1] = ETH_P_EDSA & 0xff;
> + edsa_header[2] = 0x00;
> + edsa_header[3] = 0x00;
> + return skb;
> +}
> +
> +static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev,
> + struct packet_type *pt)
> +{
> + if (unlikely(!pskb_may_pull(skb, EDSA_HLEN)))
> + return NULL;
> +
> + skb_pull_rcsum(skb, EDSA_HLEN - DSA_HLEN);
> +
> + return dsa_rcv_ll(skb, dev, EDSA_HLEN - DSA_HLEN);
> +}
> +
> +static const struct dsa_device_ops edsa_netdev_ops = {
> + .name = "edsa",
> + .proto = DSA_TAG_PROTO_EDSA,
> + .xmit = edsa_xmit,
> + .rcv = edsa_rcv,
> + .overhead = EDSA_HLEN,
Could you reindent these to be aligned?
> +};
> +
> +DSA_TAG_DRIVER(edsa_netdev_ops);
> +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_EDSA);
> +#endif /* CONFIG_NET_DSA_TAG_EDSA */
> +
> +static struct dsa_tag_driver *dsa_tag_drivers[] = {
> +#if IS_ENABLED(CONFIG_NET_DSA_TAG_DSA)
> + &DSA_TAG_DRIVER_NAME(dsa_netdev_ops),
> +#endif
> +#if IS_ENABLED(CONFIG_NET_DSA_TAG_EDSA)
> + &DSA_TAG_DRIVER_NAME(edsa_netdev_ops),
> +#endif
> +};
> +
> +module_dsa_tag_drivers(dsa_tag_drivers);
> +
> +MODULE_LICENSE("GPL");
