[PATCH net-next 0/3] net: Switch tag HW extraction/insertion

[PATCH net-next 0/3] net: Switch tag HW extraction/insertion

[Florian Fainelli]

Hi all,

This patch series adds two new feature flags to allow Ethernet controllers
capable of doing Ethernet switch proprietary tag insertion/extraction to
benefit from that.

The last two patches modify the Broadcom tag parsing code in DSA to utilize
that feature and finally, the SYSTEMPORT controller driver is updated to
turn on that feature when requested.

The ethtool patch will follow-up shortly as well.

Insertion on transmit is still work in progress and requires more turning on
how the SYSTEMPORT transmit queues are mapped to their respective switch ports
and queues (there are 32 queues on SYSTEMPORT to output to a 4-port switch each
port with 8 transmit queues).

Florian Fainelli (3):
  net: add flags for HW assisted extraction/insertions of switch tags
  net: dsa: tag_brcm: Support extracting tags from HW
  net: systemport: Add support for switch tag HW extraction

 drivers/net/ethernet/broadcom/bcmsysport.c | 40 +++++++++++++++++++++++++++++-
 drivers/net/ethernet/broadcom/bcmsysport.h |  1 +
 include/linux/netdev_features.h            |  4 +++
 include/linux/netdevice.h                  | 13 ++++++++++
 include/net/dsa.h                          |  5 ++++
 include/uapi/linux/ethtool.h               |  2 ++
 net/core/ethtool.c                         | 16 ++++++++++--
 net/dsa/tag_brcm.c                         | 38 +++++++++++++++++-----------
 8 files changed, 101 insertions(+), 18 deletions(-)

-- 
2.1.0

[PATCH net-next 1/3] net: add flags for HW assisted extraction/insertions of switch tags

[Florian Fainelli]

Some hardware (e.g: Broadcom's SYSTEMPORT) is capable of automatically
extracting and inserting a proprietary switch tag (e.g: Broadcom tag)
which saves us packet modifications when using DSA. Add the required
ethtool changes and netdevice feature bits to support such devices.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/linux/netdev_features.h |  4 ++++
 include/uapi/linux/ethtool.h    |  2 ++
 net/core/ethtool.c              | 16 ++++++++++++++--
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 9672781c593d..08e00e341497 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -66,6 +66,8 @@ enum {
 	NETIF_F_HW_VLAN_STAG_FILTER_BIT,/* Receive filtering on VLAN STAGs */
 	NETIF_F_HW_L2FW_DOFFLOAD_BIT,	/* Allow L2 Forwarding in Hardware */
 	NETIF_F_BUSY_POLL_BIT,		/* Busy poll */
+	NETIF_F_HW_SWITCH_TAG_RX_BIT,	/* Receive switch tag acceleration */
+	NETIF_F_HW_SWITCH_TAG_TX_BIT,	/* Transmit switch tag acceleration */
 
 	/*
 	 * Add your fresh new feature above and remember to update
@@ -124,6 +126,8 @@ enum {
 #define NETIF_F_HW_VLAN_STAG_TX	__NETIF_F(HW_VLAN_STAG_TX)
 #define NETIF_F_HW_L2FW_DOFFLOAD	__NETIF_F(HW_L2FW_DOFFLOAD)
 #define NETIF_F_BUSY_POLL	__NETIF_F(BUSY_POLL)
+#define NETIF_F_HW_SWITCH_TAG_RX __NETIF_F(HW_SWITCH_TAG_RX)
+#define NETIF_F_HW_SWITCH_TAG_TX __NETIF_F(HW_SWITCH_TAG_TX)
 
 /* Features valid for ethtool to change */
 /* = all defined minus driver/device-class-related */
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index cd1629170103..93710e249032 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -684,6 +684,8 @@ enum ethtool_flags {
 	ETH_FLAG_LRO		= (1 << 15),	/* LRO is enabled */
 	ETH_FLAG_NTUPLE		= (1 << 27),	/* N-tuple filters enabled */
 	ETH_FLAG_RXHASH		= (1 << 28),
+	ETH_FLAG_RXSWITCH	= (1 << 29),	/* RX switch tag offload */
+	ETH_FLAG_TXSWITCH	= (1 << 30),	/* TX switch tag offload */
 };
 
 /* The following structures are for supporting RX network flow
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index b495ab1797fa..6f642dc4bd1d 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -98,6 +98,8 @@ static const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN]
 	[NETIF_F_RXALL_BIT] =            "rx-all",
 	[NETIF_F_HW_L2FW_DOFFLOAD_BIT] = "l2-fwd-offload",
 	[NETIF_F_BUSY_POLL_BIT] =        "busy-poll",
+	[NETIF_F_HW_SWITCH_TAG_RX_BIT]	= "rx-switch-tag-hw-parse",
+	[NETIF_F_HW_SWITCH_TAG_TX_BIT]	= "tx-switch-tag-hw-insert",
 };
 
 static const char
@@ -298,10 +300,12 @@ static int ethtool_set_one_feature(struct net_device *dev,
 }
 
 #define ETH_ALL_FLAGS    (ETH_FLAG_LRO | ETH_FLAG_RXVLAN | ETH_FLAG_TXVLAN | \
-			  ETH_FLAG_NTUPLE | ETH_FLAG_RXHASH)
+			  ETH_FLAG_NTUPLE | ETH_FLAG_RXHASH | ETH_FLAG_RXSWITCH | \
+			  ETH_FLAG_TXSWITCH)
 #define ETH_ALL_FEATURES (NETIF_F_LRO | NETIF_F_HW_VLAN_CTAG_RX | \
 			  NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_NTUPLE | \
-			  NETIF_F_RXHASH)
+			  NETIF_F_RXHASH | NETIF_F_HW_SWITCH_TAG_RX | \
+			  NETIF_F_HW_SWITCH_TAG_TX)
 
 static u32 __ethtool_get_flags(struct net_device *dev)
 {
@@ -317,6 +321,10 @@ static u32 __ethtool_get_flags(struct net_device *dev)
 		flags |= ETH_FLAG_NTUPLE;
 	if (dev->features & NETIF_F_RXHASH)
 		flags |= ETH_FLAG_RXHASH;
+	if (dev->features & NETIF_F_HW_SWITCH_TAG_RX)
+		flags |= ETH_FLAG_RXSWITCH;
+	if (dev->features & NETIF_F_HW_SWITCH_TAG_TX)
+		flags |= ETH_FLAG_TXSWITCH;
 
 	return flags;
 }
@@ -338,6 +346,10 @@ static int __ethtool_set_flags(struct net_device *dev, u32 data)
 		features |= NETIF_F_NTUPLE;
 	if (data & ETH_FLAG_RXHASH)
 		features |= NETIF_F_RXHASH;
+	if (data & ETH_FLAG_RXSWITCH)
+		features |= NETIF_F_HW_SWITCH_TAG_RX;
+	if (data & ETH_FLAG_TXSWITCH)
+		features |= NETIF_F_HW_SWITCH_TAG_TX;
 
 	/* allow changing only bits set in hw_features */
 	changed = (features ^ dev->features) & ETH_ALL_FEATURES;
-- 
2.1.0

[PATCH net-next 2/3] net: dsa: tag_brcm: Support extracting tags from HW

[Florian Fainelli]

In case an adapter advertises NETIF_F_HW_SWITCH_RX_TAG, proceed with
extracting the 4-bytes Broadcom tag directly from skb->cb[] and utilize
that instead of copying and memmoving() data around.

To establish a contract between the Ethernet MAC advertisign
NETIF_F_HW_SWITCH_RX_TAG and the Broadcom tag DSA parsing code,
introduce a helper function to copy such a tag into skb->cb[] at a
pre-defined offset which is checked against other parts of the kernel we
compete with: NAPI GRO cb.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/linux/netdevice.h | 13 +++++++++++++
 include/net/dsa.h         |  5 +++++
 net/dsa/tag_brcm.c        | 38 +++++++++++++++++++++++---------------
 3 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 607b5f41f46f..02790c6acdb8 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2004,6 +2004,19 @@ struct napi_gro_cb {
 
 #define NAPI_GRO_CB(skb) ((struct napi_gro_cb *)(skb)->cb)
 
+/* We compete with napi_gro_cb when an Ethernet adapter wants to put a 4-bytes
+ * Broadcom tag into skb->cb since this will typically be done before the call
+ * to napi_gro_receive, so assert a build time that we have enough room to make
+ * this possible
+ */
+#define DSA_BRCM_TAG_OFF	(sizeof(((struct sk_buff *)0)->cb) - BRCM_TAG_LEN)
+
+static inline void dsa_copy_brcm_tag(struct sk_buff *skb, const void *tag)
+{
+	BUILD_BUG_ON(sizeof(skb->cb) - sizeof(struct napi_gro_cb) < BRCM_TAG_LEN);
+	memcpy(&skb->cb[DSA_BRCM_TAG_OFF], tag, BRCM_TAG_LEN);
+}
+
 struct packet_type {
 	__be16			type;	/* This is really htons(ether_type). */
 	struct net_device	*dev;	/* NULL is wildcarded here	     */
diff --git a/include/net/dsa.h b/include/net/dsa.h
index fbca63ba8f73..d8bf698d5189 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -31,6 +31,11 @@ enum dsa_tag_protocol {
 #define DSA_MAX_SWITCHES	4
 #define DSA_MAX_PORTS		12
 
+/* This tag length is 4 bytes, older ones were 6 bytes, we do not
+ * handle them
+ */
+#define BRCM_TAG_LEN	4
+
 struct dsa_chip_data {
 	/*
 	 * How to access the switch configuration registers.
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index 83d3572cdb20..6c006524540f 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -14,11 +14,6 @@
 #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
@@ -105,6 +100,7 @@ static int brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
 	struct dsa_switch *ds;
+	bool hw_accel = false;
 	int source_port;
 	u8 *brcm_tag;
 
@@ -117,11 +113,17 @@ static int brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (skb == NULL)
 		goto out;
 
-	if (unlikely(!pskb_may_pull(skb, BRCM_TAG_LEN)))
-		goto out_drop;
+	/* Verify if the underlying device supports automatic tag extraction */
+	hw_accel = !!(orig_dev->features & NETIF_F_HW_SWITCH_TAG_RX);
+	if (hw_accel) {
+		brcm_tag = (u8 *)&skb->cb[DSA_BRCM_TAG_OFF];
+	} else {
+		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;
+		/* 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))
@@ -139,14 +141,20 @@ static int brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 	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);
+	if (!hw_accel) {
+		/* 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);
+		/* Move the Ethernet DA and SA */
+		memmove(skb->data - ETH_HLEN,
+			skb->data - ETH_HLEN - BRCM_TAG_LEN,
+			2 * ETH_ALEN);
 
+	}
+
+	/* With or without hardware acceleration, the initial eth_type_trans
+	 * will have pulled the SKB by ETH_HLEN, reset that here
+	 */
 	skb_push(skb, ETH_HLEN);
 	skb->pkt_type = PACKET_HOST;
 	skb->dev = ds->ports[source_port];
-- 
2.1.0

[PATCH net-next 3/3] net: systemport: Add support for switch tag HW extraction

[Florian Fainelli]

The Broadcom SYSTEMPORT Ethernet controller is capable of extracting a
switch tag (4-bytes Broadcom tag) and put it in the second 32-bits word
of its pre-pended Receive Status Block. When this feature is requested,
make sure that we can satisfy it by turning on receive checksum offload,
and copy the 32-bits words with the tag into skb->cb[] using the
appropriate helper function: dsa_copy_brcm_tag().

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 40 +++++++++++++++++++++++++++++-
 drivers/net/ethernet/broadcom/bcmsysport.h |  1 +
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 4566cdf0bc39..a3db2d9f1c36 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -173,6 +173,23 @@ static int bcm_sysport_set_tx_csum(struct net_device *dev,
 	return 0;
 }
 
+static int bcm_sysport_set_rx_sw_tag(struct net_device *dev,
+				     netdev_features_t wanted)
+{
+	struct bcm_sysport_priv *priv = netdev_priv(dev);
+	u32 reg;
+
+	priv->rx_tag_extract = !!(wanted & NETIF_F_HW_SWITCH_TAG_RX);
+	reg = rbuf_readl(priv, RBUF_CONTROL);
+	if (priv->rx_tag_extract)
+		reg |= RBUF_BRCM_TAG_STRIP;
+	else
+		reg &= ~RBUF_BRCM_TAG_STRIP;
+	rbuf_writel(priv, reg, RBUF_CONTROL);
+
+	return 0;
+}
+
 static int bcm_sysport_set_features(struct net_device *dev,
 				    netdev_features_t features)
 {
@@ -184,10 +201,25 @@ static int bcm_sysport_set_features(struct net_device *dev,
 		ret = bcm_sysport_set_rx_csum(dev, wanted);
 	if (changed & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM))
 		ret = bcm_sysport_set_tx_csum(dev, wanted);
+	if (changed & NETIF_F_HW_SWITCH_TAG_RX)
+		ret = bcm_sysport_set_rx_sw_tag(dev, wanted);
 
 	return ret;
 }
 
+static netdev_features_t bcm_sysport_fix_features(struct net_device *dev,
+						  netdev_features_t features)
+{
+	/* In order for the switch tag extraction to work, we need to turn on
+	 * the RXCHK block and enable receive checksum offload, do this here to
+	 * satisfy the feature request.
+	 */
+	if (features & NETIF_F_HW_SWITCH_TAG_RX)
+		features |= NETIF_F_RXCSUM;
+
+	return features;
+}
+
 /* Hardware counters must be kept in sync because the order/offset
  * is important here (order in structure declaration = order in hardware)
  */
@@ -670,6 +702,10 @@ static unsigned int bcm_sysport_desc_rx(struct bcm_sysport_priv *priv,
 		if (likely(status & DESC_L4_CSUM))
 			skb->ip_summed = CHECKSUM_UNNECESSARY;
 
+		/* Extract the switch egress tag */
+		if (likely(priv->rx_tag_extract))
+			dsa_copy_brcm_tag(skb, &rsb->brcm_egress_tag);
+
 		/* Hardware pre-pends packets with 2bytes before Ethernet
 		 * header plus we have the Receive Status Block, strip off all
 		 * of this from the SKB.
@@ -1721,6 +1757,7 @@ static const struct net_device_ops bcm_sysport_netdev_ops = {
 	.ndo_open		= bcm_sysport_open,
 	.ndo_stop		= bcm_sysport_stop,
 	.ndo_set_features	= bcm_sysport_set_features,
+	.ndo_fix_features	= bcm_sysport_fix_features,
 	.ndo_set_rx_mode	= bcm_sysport_set_rx_mode,
 	.ndo_set_mac_address	= bcm_sysport_change_mac,
 };
@@ -1806,7 +1843,8 @@ static int bcm_sysport_probe(struct platform_device *pdev)
 
 	/* HW supported features, none enabled by default */
 	dev->hw_features |= NETIF_F_RXCSUM | NETIF_F_HIGHDMA |
-				NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
+				NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+				NETIF_F_HW_SWITCH_TAG_RX;
 
 	/* Request the WOL interrupt and advertise suspend if available */
 	priv->wol_irq_disabled = 1;
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h
index f28bf545d7f4..6d925401d706 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.h
+++ b/drivers/net/ethernet/broadcom/bcmsysport.h
@@ -680,6 +680,7 @@ struct bcm_sysport_priv {
 	unsigned int		rx_chk_en:1;
 	unsigned int		tsb_en:1;
 	unsigned int		crc_fwd:1;
+	unsigned int		rx_tag_extract:1;
 	u16			rev;
 	u32			wolopts;
 	unsigned int		wol_irq_disabled:1;
-- 
2.1.0

Re: [PATCH net-next 0/3] net: Switch tag HW extraction/insertion

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 29 Jul 2015 15:32:37 -0700

> This patch series adds two new feature flags to allow Ethernet controllers
> capable of doing Ethernet switch proprietary tag insertion/extraction to
> benefit from that.
> 
> The last two patches modify the Broadcom tag parsing code in DSA to utilize
> that feature and finally, the SYSTEMPORT controller driver is updated to
> turn on that feature when requested.
> 
> The ethtool patch will follow-up shortly as well.
> 
> Insertion on transmit is still work in progress and requires more turning on
> how the SYSTEMPORT transmit queues are mapped to their respective switch ports
> and queues (there are 32 queues on SYSTEMPORT to output to a 4-port switch each
> port with 8 transmit queues).

This looks fine, series applied, thanks.

Re: [PATCH net-next 0/3] net: Switch tag HW extraction/insertion

[David Miller]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: Text/Plain; charset=iso-8859-7, Size: 1742 bytes --]

From: David Miller <davem@davemloft.net>
Date: Thu, 30 Jul 2015 14:19:35 -0700 (PDT)

> This looks fine, series applied, thanks.

I think your control block is too large, you'll need to rework this
somehow.

In function ¡dsa_copy_brcm_tag¢,
    inlined from ¡bcm_sysport_desc_rx¢ at drivers/net/ethernet/broadcom/bcmsysport.c:707:4,
    inlined from ¡bcm_sysport_poll¢ at drivers/net/ethernet/broadcom/bcmsysport.c:864:12:
include/linux/compiler.h:447:38: error: call to ¡__compiletime_assert_2016¢ declared with attribute error: BUILD_BUG_ON failed: sizeof(skb->cb) - sizeof(struct napi_gro_cb) < BRCM_TAG_LEN
  _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
                                      ^
include/linux/compiler.h:430:4: note: in definition of macro ¡__compiletime_assert¢
    prefix ## suffix();    \
    ^
include/linux/compiler.h:447:2: note: in expansion of macro ¡_compiletime_assert¢
  _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
  ^
include/linux/bug.h:50:37: note: in expansion of macro ¡compiletime_assert¢
 #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
                                     ^
include/linux/bug.h:74:2: note: in expansion of macro ¡BUILD_BUG_ON_MSG¢
  BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
  ^
include/linux/netdevice.h:2016:2: note: in expansion of macro ¡BUILD_BUG_ON¢
  BUILD_BUG_ON(sizeof(skb->cb) - sizeof(struct napi_gro_cb) < BRCM_TAG_LEN);
  ^
scripts/Makefile.build:264: recipe for target 'drivers/net/ethernet/broadcom/bcmsysport.o' failed
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH net-next 0/3] net: Switch tag HW extraction/insertion

[Florian Fainelli]

On 30/07/15 15:51, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Thu, 30 Jul 2015 14:19:35 -0700 (PDT)
> 
>> This looks fine, series applied, thanks.
> 
> I think your control block is too large, you'll need to rework this
> somehow.

Interesting, this only seems to show up with 64-bits build, which is why
I did not catch this earlier (building for ARMv7 typically), sorry about
that, I will come up with a fix.

> 
> In function ‘dsa_copy_brcm_tag’,
>     inlined from ‘bcm_sysport_desc_rx’ at drivers/net/ethernet/broadcom/bcmsysport.c:707:4,
>     inlined from ‘bcm_sysport_poll’ at drivers/net/ethernet/broadcom/bcmsysport.c:864:12:
> include/linux/compiler.h:447:38: error: call to ‘__compiletime_assert_2016’ declared with attribute error: BUILD_BUG_ON failed: sizeof(skb->cb) - sizeof(struct napi_gro_cb) < BRCM_TAG_LEN
>   _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
>                                       ^
> include/linux/compiler.h:430:4: note: in definition of macro ‘__compiletime_assert’
>     prefix ## suffix();    \
>     ^
> include/linux/compiler.h:447:2: note: in expansion of macro ‘_compiletime_assert’
>   _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
>   ^
> include/linux/bug.h:50:37: note: in expansion of macro ‘compiletime_assert’
>  #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
>                                      ^
> include/linux/bug.h:74:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
>   BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
>   ^
> include/linux/netdevice.h:2016:2: note: in expansion of macro ‘BUILD_BUG_ON’
>   BUILD_BUG_ON(sizeof(skb->cb) - sizeof(struct napi_gro_cb) < BRCM_TAG_LEN);
>   ^
> scripts/Makefile.build:264: recipe for target 'drivers/net/ethernet/broadcom/bcmsysport.o' failed
> 


-- 
Florian

Re: [PATCH net-next 0/3] net: Switch tag HW extraction/insertion

[Florian Fainelli]

On 30/07/15 15:51, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Thu, 30 Jul 2015 14:19:35 -0700 (PDT)
> 
>> This looks fine, series applied, thanks.
> 
> I think your control block is too large, you'll need to rework this
> somehow.

So napi_gro_cb really is 48 bytes on 64-bits architectures (had not
realized it was so big).

What would you recommend to do here considering that this driver is
currently used on 32-bits platforms, but I see no reason why someone
would no want to use this feature on a 64-bit platform, yet we are
competing with napi_gro_cb, and adding a new skbuff member is pretty
much a no-no? Would it be acceptable to have a new member which is ifdef
CONFIG_NET_DSA_TAG_BRCM?

FWIW, this does provide a small 2-3% throughput increase for RX.

> 
> In function ‘dsa_copy_brcm_tag’,
>     inlined from ‘bcm_sysport_desc_rx’ at drivers/net/ethernet/broadcom/bcmsysport.c:707:4,
>     inlined from ‘bcm_sysport_poll’ at drivers/net/ethernet/broadcom/bcmsysport.c:864:12:
> include/linux/compiler.h:447:38: error: call to ‘__compiletime_assert_2016’ declared with attribute error: BUILD_BUG_ON failed: sizeof(skb->cb) - sizeof(struct napi_gro_cb) < BRCM_TAG_LEN
>   _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
>                                       ^
> include/linux/compiler.h:430:4: note: in definition of macro ‘__compiletime_assert’
>     prefix ## suffix();    \
>     ^
> include/linux/compiler.h:447:2: note: in expansion of macro ‘_compiletime_assert’
>   _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
>   ^
> include/linux/bug.h:50:37: note: in expansion of macro ‘compiletime_assert’
>  #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
>                                      ^
> include/linux/bug.h:74:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
>   BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
>   ^
> include/linux/netdevice.h:2016:2: note: in expansion of macro ‘BUILD_BUG_ON’
>   BUILD_BUG_ON(sizeof(skb->cb) - sizeof(struct napi_gro_cb) < BRCM_TAG_LEN);
>   ^
> scripts/Makefile.build:264: recipe for target 'drivers/net/ethernet/broadcom/bcmsysport.o' failed
> 


-- 
Florian

Re: [PATCH net-next 0/3] net: Switch tag HW extraction/insertion

[Chris Packham]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3150 bytes --]

Hi Florian,

On 07/31/2015 01:51 PM, Florian Fainelli wrote:
> On 30/07/15 15:51, David Miller wrote:
>> From: David Miller <davem@davemloft.net>
>> Date: Thu, 30 Jul 2015 14:19:35 -0700 (PDT)
>>
>>> This looks fine, series applied, thanks.
>>
>> I think your control block is too large, you'll need to rework this
>> somehow.
>
> So napi_gro_cb really is 48 bytes on 64-bits architectures (had not
> realized it was so big).
>
> What would you recommend to do here considering that this driver is
> currently used on 32-bits platforms, but I see no reason why someone
> would no want to use this feature on a 64-bit platform,

I haven't read a lot of the detail in your patch series yet nor do I 
understand the ramifications of the error this has triggered. But I will 
stick my hand up for this point. We do have a router with a 64-bit CPU 
and a bcm L2 switch (b53 is the code name being flung around).

None of this is upstream (yet) but the CPU vendor has at least come to 
the party and has landed patches in 4.2. We'd probably have to do the 
switch support ourselves (perhaps based on code from openWRT).

 > yet we are
> competing with napi_gro_cb, and adding a new skbuff member is pretty
> much a no-no? Would it be acceptable to have a new member which is ifdef
> CONFIG_NET_DSA_TAG_BRCM?
>
> FWIW, this does provide a small 2-3% throughput increase for RX.

I'll leaves this for the netdev folks to comment on. I just wanted to 
let you know that such a use-case _may_ exist.

>>
>> In function ‘dsa_copy_brcm_tag’,
>>      inlined from ‘bcm_sysport_desc_rx’ at drivers/net/ethernet/broadcom/bcmsysport.c:707:4,
>>      inlined from ‘bcm_sysport_poll’ at drivers/net/ethernet/broadcom/bcmsysport.c:864:12:
>> include/linux/compiler.h:447:38: error: call to ‘__compiletime_assert_2016’ declared with attribute error: BUILD_BUG_ON failed: sizeof(skb->cb) - sizeof(struct napi_gro_cb) < BRCM_TAG_LEN
>>    _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
>>                                        ^
>> include/linux/compiler.h:430:4: note: in definition of macro ‘__compiletime_assert’
>>      prefix ## suffix();    \
>>      ^
>> include/linux/compiler.h:447:2: note: in expansion of macro ‘_compiletime_assert’
>>    _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
>>    ^
>> include/linux/bug.h:50:37: note: in expansion of macro ‘compiletime_assert’
>>   #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
>>                                       ^
>> include/linux/bug.h:74:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
>>    BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
>>    ^
>> include/linux/netdevice.h:2016:2: note: in expansion of macro ‘BUILD_BUG_ON’
>>    BUILD_BUG_ON(sizeof(skb->cb) - sizeof(struct napi_gro_cb) < BRCM_TAG_LEN);
>>    ^
>> scripts/Makefile.build:264: recipe for target 'drivers/net/ethernet/broadcom/bcmsysport.o' failed
>>
>
>ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH net-next 0/3] net: Switch tag HW extraction/insertion

[Eric Dumazet]

On Thu, 2015-07-30 at 18:51 -0700, Florian Fainelli wrote:
> On 30/07/15 15:51, David Miller wrote:
> > From: David Miller <davem@davemloft.net>
> > Date: Thu, 30 Jul 2015 14:19:35 -0700 (PDT)
> > 
> >> This looks fine, series applied, thanks.
> > 
> > I think your control block is too large, you'll need to rework this
> > somehow.
> 
> So napi_gro_cb really is 48 bytes on 64-bits architectures (had not
> realized it was so big).
> 
> What would you recommend to do here considering that this driver is
> currently used on 32-bits platforms, but I see no reason why someone
> would no want to use this feature on a 64-bit platform, yet we are
> competing with napi_gro_cb, and adding a new skbuff member is pretty
> much a no-no? Would it be acceptable to have a new member which is ifdef
> CONFIG_NET_DSA_TAG_BRCM?
> 
> FWIW, this does provide a small 2-3% throughput increase for RX.

Which layer will read this tag after GRO processing ?

It seems you simply can use skb->cb[] like other layers : At offset 0.

BTW brcm_tag_rcv() does not even use GRO, as it calls
netif_receive_skb()

Re: [PATCH net-next 0/3] net: Switch tag HW extraction/insertion

[Florian Fainelli]

On 30/07/15 23:08, Eric Dumazet wrote:
> On Thu, 2015-07-30 at 18:51 -0700, Florian Fainelli wrote:
>> On 30/07/15 15:51, David Miller wrote:
>>> From: David Miller <davem@davemloft.net>
>>> Date: Thu, 30 Jul 2015 14:19:35 -0700 (PDT)
>>>
>>>> This looks fine, series applied, thanks.
>>>
>>> I think your control block is too large, you'll need to rework this
>>> somehow.
>>
>> So napi_gro_cb really is 48 bytes on 64-bits architectures (had not
>> realized it was so big).
>>
>> What would you recommend to do here considering that this driver is
>> currently used on 32-bits platforms, but I see no reason why someone
>> would no want to use this feature on a 64-bit platform, yet we are
>> competing with napi_gro_cb, and adding a new skbuff member is pretty
>> much a no-no? Would it be acceptable to have a new member which is ifdef
>> CONFIG_NET_DSA_TAG_BRCM?
>>
>> FWIW, this does provide a small 2-3% throughput increase for RX.
> 
> Which layer will read this tag after GRO processing ?

DSA would read this tag, but in general any ethertype hook using
packet_type would be in the same boat.

> 
> It seems you simply can use skb->cb[] like other layers : At offset 0.
> 
> BTW brcm_tag_rcv() does not even use GRO, as it calls
> netif_receive_skb()

That is right here, we will come from the RX processing of a driver that
might use napi_gro_receive, as it turns out the sequence looks like this
here:

bcm_sysport_desc_rx()
	-> extract tag
	eth_type_trans() sets skb->protocol to ETH_P_XDSA
	napi_gro_receive(), sets skb->cb to napi_gro_cb
		-> walk the list of packet_type find the one for ETH_P_XDSA
		-> brcm_tag_rcv()

BTW, there is no build time assertion that napi_gro_cb is not exceeding
skb->cb right now, even though they both have the same size on 64-bits,
should we have one?
-- 
Florian

Re: [PATCH net-next 0/3] net: Switch tag HW extraction/insertion

[David Miller]

From: Florian Fainelli <florian@alphacore.org>
Date: Thu, 30 Jul 2015 18:51:57 -0700

> On 30/07/15 15:51, David Miller wrote:
>> From: David Miller <davem@davemloft.net>
>> Date: Thu, 30 Jul 2015 14:19:35 -0700 (PDT)
>> 
>>> This looks fine, series applied, thanks.
>> 
>> I think your control block is too large, you'll need to rework this
>> somehow.
> 
> So napi_gro_cb really is 48 bytes on 64-bits architectures (had not
> realized it was so big).
> 
> What would you recommend to do here considering that this driver is
> currently used on 32-bits platforms, but I see no reason why someone
> would no want to use this feature on a 64-bit platform, yet we are
> competing with napi_gro_cb, and adding a new skbuff member is pretty
> much a no-no? Would it be acceptable to have a new member which is ifdef
> CONFIG_NET_DSA_TAG_BRCM?
> 
> FWIW, this does provide a small 2-3% throughput increase for RX.

You really need to find a way to make it fit in it's current size
on 64-bit.