diff mbox

[net-next,v3,09/12] net: dsa: add Broadcom tag RX/TX handler

Message ID 1408905869-10471-10-git-send-email-f.fainelli@gmail.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Florian Fainelli Aug. 24, 2014, 6:44 p.m. UTC 
Add support for the 4-bytes Broadcom tag that built-in switches such as
the Starfighter 2 might insert when receiving packets, or that we need
to insert while targetting specific switch ports. We use a fake
EtherType field for this 4-bytes switch tag: ETH_P_BRCMTAG since we need
to use the skb->protocol override in the receive path.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v3:
- add ETH_P_BRCMTAG as part of this changeset and not in a previous patch
- fixed an early de-reference in the receive hook

 include/uapi/linux/if_ether.h |   1 +
 net/dsa/Kconfig               |   3 +
 net/dsa/Makefile              |   1 +
 net/dsa/dsa.c                 |   6 ++
 net/dsa/dsa_priv.h            |   4 +
 net/dsa/slave.c               |  17 +++++
 net/dsa/tag_brcm.c            | 173 ++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 205 insertions(+)
 create mode 100644 net/dsa/tag_brcm.c

Comments

Alexander H Duyck Aug. 24, 2014, 10:51 p.m. UTC | #1 
On 08/24/2014 11:44 AM, Florian Fainelli wrote:
> Add support for the 4-bytes Broadcom tag that built-in switches such as
> the Starfighter 2 might insert when receiving packets, or that we need
> to insert while targetting specific switch ports. We use a fake
> EtherType field for this 4-bytes switch tag: ETH_P_BRCMTAG since we need
> to use the skb->protocol override in the receive path.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes in v3:
> - add ETH_P_BRCMTAG as part of this changeset and not in a previous patch
> - fixed an early de-reference in the receive hook

I was just wondering.  Is there a reason we need to register this as yet
another protocol?

It seems like we should be able to just register one DSA tag protocol
and then we could push the parsing function out to a function pointer
contained in the DSA switch structure.  My concern is that as we add
each new switch message format we are looking at the potential for yet
another Ethertype and they are a limited resource.

So for example in the section below you already have to dig out the
dsa_switch structure and tree before you even start processing the
headers. 

> +
> +static int brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev,
> +			struct packet_type *pt, struct net_device *orig_dev)
> +{
> +	struct dsa_switch_tree *dst = dev->dsa_ptr;
> +	struct dsa_switch *ds;
> +	int source_port;
> +	u8 *brcm_tag;
> +
> +	if (unlikely(dst == NULL))
> +		goto out_drop;
> +
> +	ds = dst->ds[0];
> +
> +	skb = skb_unshare(skb, GFP_ATOMIC);
> +	if (skb == NULL)
> +		goto out;

At this point here we already have the dsa pointers and could just pull
up a function pointer so all of the code below could potentially be
moved into a separate function allowing us to drop the need to have
multiple Ethertypes and so we could just use ETH_P_DSA for all DSA
tagging type.  We could then also rewrite the check for if we need to
insert a DSA tag to simply check for if this function pointer is set or not.

> +	if (unlikely(!pskb_may_pull(skb, BRCM_TAG_LEN)))
> +		goto out_drop;
> +
> +	/* skb->data points to the EtherType, the tag is right before it */
> +	brcm_tag = skb->data - 2;
> +
> +	/* The opcode should never be different than 0b000 */
> +	if (unlikely((brcm_tag[0] >> BRCM_OPCODE_SHIFT) & BRCM_OPCODE_MASK))
> +		goto out_drop;
> +
> +	/* We should never see a reserved reason code without knowing how to
> +	 * handle it
> +	 */
> +	WARN_ON(brcm_tag[2] & BRCM_EG_RC_RSVD);
> +
> +	/* Locate which port this is coming from */
> +	source_port = brcm_tag[3] & BRCM_EG_PID_MASK;
> +
> +	/* Validate port against switch setup, either the port is totally */
> +	if (source_port >= DSA_MAX_PORTS || ds->ports[source_port] == NULL)
> +		goto out_drop;
> +
> +	/* Remove Broadcom tag and update checksum */
> +	skb_pull_rcsum(skb, BRCM_TAG_LEN);
> +
> +	/* Move the Ethernet DA and SA */
> +	memmove(skb->data - ETH_HLEN,
> +		skb->data - ETH_HLEN - BRCM_TAG_LEN,
> +		2 * ETH_ALEN);
> +
> +	skb_push(skb, ETH_HLEN);
> +	skb->pkt_type = PACKET_HOST;
> +	skb->dev = ds->ports[source_port];
> +	skb->protocol = eth_type_trans(skb, skb->dev);
> +
> +	skb->dev->stats.rx_packets++;
> +	skb->dev->stats.rx_bytes += skb->len;
> +
> +	netif_receive_skb(skb);
> +
> +	return 0;
> +
> +out_drop:
> +	kfree_skb(skb);
> +out:
> +	return 0;
> +}
> +
> +struct packet_type brcm_tag_packet_type __read_mostly = {
> +	.type	= cpu_to_be16(ETH_P_BRCMTAG),
> +	.func	= brcm_tag_rcv,
> +};

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Fainelli Aug. 25, 2014, 2:37 a.m. UTC | #2 
Le 24/08/2014 15:51, Alexander Duyck a écrit :
> On 08/24/2014 11:44 AM, Florian Fainelli wrote:
>> Add support for the 4-bytes Broadcom tag that built-in switches such as
>> the Starfighter 2 might insert when receiving packets, or that we need
>> to insert while targetting specific switch ports. We use a fake
>> EtherType field for this 4-bytes switch tag: ETH_P_BRCMTAG since we need
>> to use the skb->protocol override in the receive path.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> Changes in v3:
>> - add ETH_P_BRCMTAG as part of this changeset and not in a previous patch
>> - fixed an early de-reference in the receive hook
>
> I was just wondering.  Is there a reason we need to register this as yet
> another protocol?

For the broadcom tag format, not specifically no, this is mostly so it 
looks like what has been done so far, but since this is not a real 
ethertype, there is no need for this.

>
> It seems like we should be able to just register one DSA tag protocol
> and then we could push the parsing function out to a function pointer
> contained in the DSA switch structure.  My concern is that as we add
> each new switch message format we are looking at the potential for yet
> another Ethertype and they are a limited resource.
>
> So for example in the section below you already have to dig out the
> dsa_switch structure and tree before you even start processing the
> headers.
>
>> +
>> +static int brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev,
>> +			struct packet_type *pt, struct net_device *orig_dev)
>> +{
>> +	struct dsa_switch_tree *dst = dev->dsa_ptr;
>> +	struct dsa_switch *ds;
>> +	int source_port;
>> +	u8 *brcm_tag;
>> +
>> +	if (unlikely(dst == NULL))
>> +		goto out_drop;
>> +
>> +	ds = dst->ds[0];
>> +
>> +	skb = skb_unshare(skb, GFP_ATOMIC);
>> +	if (skb == NULL)
>> +		goto out;
>
> At this point here we already have the dsa pointers and could just pull
> up a function pointer so all of the code below could potentially be
> moved into a separate function allowing us to drop the need to have
> multiple Ethertypes and so we could just use ETH_P_DSA for all DSA
> tagging type.

I see, or rather maybe just use ETH_P_EDSA which is a real assigned 
ethertype?
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Duyck, Alexander H Aug. 25, 2014, 2:51 p.m. UTC | #3 
On 08/24/2014 07:37 PM, Florian Fainelli wrote:
> Le 24/08/2014 15:51, Alexander Duyck a écrit :
>> On 08/24/2014 11:44 AM, Florian Fainelli wrote:
>>> +
>>> +static int brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev,
>>> +            struct packet_type *pt, struct net_device *orig_dev)
>>> +{
>>> +    struct dsa_switch_tree *dst = dev->dsa_ptr;
>>> +    struct dsa_switch *ds;
>>> +    int source_port;
>>> +    u8 *brcm_tag;
>>> +
>>> +    if (unlikely(dst == NULL))
>>> +        goto out_drop;
>>> +
>>> +    ds = dst->ds[0];
>>> +
>>> +    skb = skb_unshare(skb, GFP_ATOMIC);
>>> +    if (skb == NULL)
>>> +        goto out;
>>
>> At this point here we already have the dsa pointers and could just pull
>> up a function pointer so all of the code below could potentially be
>> moved into a separate function allowing us to drop the need to have
>> multiple Ethertypes and so we could just use ETH_P_DSA for all DSA
>> tagging type.
> 
> I see, or rather maybe just use ETH_P_EDSA which is a real assigned
> ethertype?
> -- 
> Florian

I suppose we could probably do that.  Although I would probably say we
should just rename it to ETH_P_DSA since we can just go through and drop
all of the other defined Ethertypes for DSA.  In addition since the EDSA
isn't actually a registered type we probably don't need to bother with
conserving the name anyway.

Thanks,

Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
index 0f8210b8e0bc..2e9f13768abd 100644
--- a/include/uapi/linux/if_ether.h
+++ b/include/uapi/linux/if_ether.h
@@ -128,6 +128,7 @@ 
 #define ETH_P_PHONET	0x00F5		/* Nokia Phonet frames          */
 #define ETH_P_IEEE802154 0x00F6		/* IEEE802.15.4 frame		*/
 #define ETH_P_CAIF	0x00F7		/* ST-Ericsson CAIF protocol	*/
+#define ETH_P_BRCMTAG	0x00F8		/* Broadcom tag (4 bytes *)	*/
 
 /*
  *	This is an Ethernet frame header.
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index f5eede1d6cb8..a585fd6352eb 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -12,6 +12,9 @@  config NET_DSA
 if NET_DSA
 
 # tagging formats
+config NET_DSA_TAG_BRCM
+	bool
+
 config NET_DSA_TAG_DSA
 	bool
 
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index 7b9fcbbeda5d..da06ed1df620 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -3,6 +3,7 @@  obj-$(CONFIG_NET_DSA) += dsa_core.o
 dsa_core-y += dsa.o slave.o
 
 # tagging formats
+dsa_core-$(CONFIG_NET_DSA_TAG_BRCM) += tag_brcm.o
 dsa_core-$(CONFIG_NET_DSA_TAG_DSA) += tag_dsa.o
 dsa_core-$(CONFIG_NET_DSA_TAG_EDSA) += tag_edsa.o
 dsa_core-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 62f2f4e67d7d..cd33ddf6ff4f 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -650,6 +650,9 @@  static int __init dsa_init_module(void)
 #ifdef CONFIG_NET_DSA_TAG_TRAILER
 	dev_add_pack(&trailer_packet_type);
 #endif
+#ifdef CONFIG_NET_DSA_TAG_BRCM
+	dev_add_pack(&brcm_tag_packet_type);
+#endif
 	return 0;
 }
 module_init(dsa_init_module);
@@ -665,6 +668,9 @@  static void __exit dsa_cleanup_module(void)
 #ifdef CONFIG_NET_DSA_TAG_DSA
 	dev_remove_pack(&dsa_packet_type);
 #endif
+#ifdef CONFIG_NET_DSA_TAG_BRCM
+	dev_remove_pack(&brcm_tag_packet_type);
+#endif
 	platform_driver_unregister(&dsa_driver);
 }
 module_exit(dsa_cleanup_module);
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index a7f6f0c5fa31..8fb71da70dd6 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -60,5 +60,9 @@  extern struct packet_type edsa_packet_type;
 netdev_tx_t trailer_xmit(struct sk_buff *skb, struct net_device *dev);
 extern struct packet_type trailer_packet_type;
 
+/* tag_brcm.c */
+netdev_tx_t brcm_tag_xmit(struct sk_buff *skb, struct net_device *dev);
+extern struct packet_type brcm_tag_packet_type;
+
 
 #endif
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index ee019ec29dec..279728c53c91 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -295,6 +295,18 @@  static const struct ethtool_ops dsa_slave_ethtool_ops = {
 	.get_sset_count		= dsa_slave_get_sset_count,
 };
 
+#ifdef CONFIG_NET_DSA_TAG_BRCM
+static const struct net_device_ops brcm_netdev_ops = {
+	.ndo_init		= dsa_slave_init,
+	.ndo_open		= dsa_slave_open,
+	.ndo_stop		= dsa_slave_close,
+	.ndo_start_xmit		= brcm_tag_xmit,
+	.ndo_change_rx_flags	= dsa_slave_change_rx_flags,
+	.ndo_set_rx_mode	= dsa_slave_set_rx_mode,
+	.ndo_set_mac_address	= dsa_slave_set_mac_address,
+	.ndo_do_ioctl		= dsa_slave_ioctl,
+};
+#endif
 #ifdef CONFIG_NET_DSA_TAG_DSA
 static const struct net_device_ops dsa_netdev_ops = {
 	.ndo_init		= dsa_slave_init,
@@ -474,6 +486,11 @@  dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 		slave_dev->netdev_ops = &trailer_netdev_ops;
 		break;
 #endif
+#ifdef CONFIG_NET_DSA_TAG_BRCM
+	case htons(ETH_P_BRCMTAG):
+		slave_dev->netdev_ops = &brcm_netdev_ops;
+		break;
+#endif
 	default:
 		slave_dev->netdev_ops = &notag_netdev_ops;
 		break;
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
new file mode 100644
index 000000000000..4b1c2c1896a3
--- /dev/null
+++ b/net/dsa/tag_brcm.c
@@ -0,0 +1,173 @@ 
+/*
+ * Broadcom tag support
+ *
+ * Copyright (C) 2014 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/etherdevice.h>
+#include <linux/list.h>
+#include <linux/netdevice.h>
+#include <linux/slab.h>
+#include "dsa_priv.h"
+
+/* This tag length is 4 bytes, older ones were 6 bytes, we do not
+ * handle them
+ */
+#define BRCM_TAG_LEN	4
+
+/* Tag is constructed and desconstructed using byte by byte access
+ * because the tag is placed after the MAC Source Address, which does
+ * not make it 4-bytes aligned, so this might cause unaligned accesses
+ * on most systems where this is used.
+ */
+
+/* Ingress and egress opcodes */
+#define BRCM_OPCODE_SHIFT	5
+#define BRCM_OPCODE_MASK	0x7
+
+/* Ingress fields */
+/* 1st byte in the tag */
+#define BRCM_IG_TC_SHIFT	2
+#define BRCM_IG_TC_MASK		0x7
+/* 2nd byte in the tag */
+#define BRCM_IG_TE_MASK		0x3
+#define BRCM_IG_TS_SHIFT	7
+/* 3rd byte in the tag */
+#define BRCM_IG_DSTMAP2_MASK	1
+#define BRCM_IG_DSTMAP1_MASK	0xff
+
+/* Egress fields */
+
+/* 2nd byte in the tag */
+#define BRCM_EG_CID_MASK	0xff
+
+/* 3rd byte in the tag */
+#define BRCM_EG_RC_MASK		0xff
+#define  BRCM_EG_RC_RSVD	(3 << 6)
+#define  BRCM_EG_RC_EXCEPTION	(1 << 5)
+#define  BRCM_EG_RC_PROT_SNOOP	(1 << 4)
+#define  BRCM_EG_RC_PROT_TERM	(1 << 3)
+#define  BRCM_EG_RC_SWITCH	(1 << 2)
+#define  BRCM_EG_RC_MAC_LEARN	(1 << 1)
+#define  BRCM_EG_RC_MIRROR	(1 << 0)
+#define BRCM_EG_TC_SHIFT	5
+#define BRCM_EG_TC_MASK		0x7
+#define BRCM_EG_PID_MASK	0x1f
+
+netdev_tx_t brcm_tag_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	u8 *brcm_tag;
+
+	dev->stats.tx_packets++;
+	dev->stats.tx_bytes += skb->len;
+
+	if (skb_cow_head(skb, BRCM_TAG_LEN) < 0)
+		goto out_free;
+
+	skb_push(skb, BRCM_TAG_LEN);
+
+	memmove(skb->data, skb->data + BRCM_TAG_LEN, 2 * ETH_ALEN);
+
+	/* Build the tag after the MAC Source Address */
+	brcm_tag = skb->data + 2 * ETH_ALEN;
+
+	/* Set the ingress opcode, traffic class, tag enforcment is
+	 * deprecated
+	 */
+	brcm_tag[0] = (1 << BRCM_OPCODE_SHIFT) |
+			((skb->priority << BRCM_IG_TC_SHIFT) & BRCM_IG_TC_MASK);
+	brcm_tag[1] = 0;
+	brcm_tag[2] = 0;
+	if (p->port == 8)
+		brcm_tag[2] = BRCM_IG_DSTMAP2_MASK;
+	brcm_tag[3] = (1 << p->port) & BRCM_IG_DSTMAP1_MASK;
+
+	/* Queue the SKB for transmission on the parent interface, but
+	 * do not modify its EtherType
+	 */
+	skb->protocol = htons(ETH_P_BRCMTAG);
+	skb->dev = p->parent->dst->master_netdev;
+	dev_queue_xmit(skb);
+
+	return NETDEV_TX_OK;
+
+out_free:
+	kfree_skb(skb);
+	return NETDEV_TX_OK;
+}
+
+static int brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev,
+			struct packet_type *pt, struct net_device *orig_dev)
+{
+	struct dsa_switch_tree *dst = dev->dsa_ptr;
+	struct dsa_switch *ds;
+	int source_port;
+	u8 *brcm_tag;
+
+	if (unlikely(dst == NULL))
+		goto out_drop;
+
+	ds = dst->ds[0];
+
+	skb = skb_unshare(skb, GFP_ATOMIC);
+	if (skb == NULL)
+		goto out;
+
+	if (unlikely(!pskb_may_pull(skb, BRCM_TAG_LEN)))
+		goto out_drop;
+
+	/* skb->data points to the EtherType, the tag is right before it */
+	brcm_tag = skb->data - 2;
+
+	/* The opcode should never be different than 0b000 */
+	if (unlikely((brcm_tag[0] >> BRCM_OPCODE_SHIFT) & BRCM_OPCODE_MASK))
+		goto out_drop;
+
+	/* We should never see a reserved reason code without knowing how to
+	 * handle it
+	 */
+	WARN_ON(brcm_tag[2] & BRCM_EG_RC_RSVD);
+
+	/* Locate which port this is coming from */
+	source_port = brcm_tag[3] & BRCM_EG_PID_MASK;
+
+	/* Validate port against switch setup, either the port is totally */
+	if (source_port >= DSA_MAX_PORTS || ds->ports[source_port] == NULL)
+		goto out_drop;
+
+	/* Remove Broadcom tag and update checksum */
+	skb_pull_rcsum(skb, BRCM_TAG_LEN);
+
+	/* Move the Ethernet DA and SA */
+	memmove(skb->data - ETH_HLEN,
+		skb->data - ETH_HLEN - BRCM_TAG_LEN,
+		2 * ETH_ALEN);
+
+	skb_push(skb, ETH_HLEN);
+	skb->pkt_type = PACKET_HOST;
+	skb->dev = ds->ports[source_port];
+	skb->protocol = eth_type_trans(skb, skb->dev);
+
+	skb->dev->stats.rx_packets++;
+	skb->dev->stats.rx_bytes += skb->len;
+
+	netif_receive_skb(skb);
+
+	return 0;
+
+out_drop:
+	kfree_skb(skb);
+out:
+	return 0;
+}
+
+struct packet_type brcm_tag_packet_type __read_mostly = {
+	.type	= cpu_to_be16(ETH_P_BRCMTAG),
+	.func	= brcm_tag_rcv,
+};