[RFC PATCH 0/3] net: macb: add pad and fcs support

[RFC PATCH 0/3] net: macb: add pad and fcs support

[Claudiu Beznea]

Hi,

In [1] it was reported that UDP checksum is offloaded to hardware no mather
it was previously computed in software or not. The proposal on [1] was to
disable TX checksum offload.

This series (mostly patch 3/3) address the issue described at [1] by
setting NOCRC bit to TX buffer descriptor for SKBs that arrived from
networking stack with checksum computed. For these packets padding and FCS
need to be added (hardware doesn't compute them if NOCRC bit is set). The
minimum packet size that hardware expects is 64 bytes (including FCS).
This feature could not be used in case of GSO, so, it was used only for
no GSO SKBs.

For SKBs wich requires padding and FCS computation macb_pad_and_fcs()
checks if there is enough headroom and tailroom in SKB to avoid copying
SKB structure. Since macb_pad_and_fcs() may change SKB the
macb_pad_and_fcs() was places in macb_start_xmit() b/w macb_csum_clear()
and skb_headlen() calls.

This patch was tested with pktgen in kernel tool in a script like this:
(pktgen_sample01_simple.sh is at [2]):

minSize=1
maxSize=1500

for i in `seq $minSize $maxSize` ; do
	copy="$(shuf -i 1-2000 -n 1)"
	./pktgen_sample01_simple.sh -i eth0 \
		-m <dst-mac-addr> -d <dst-ip-addr> -x -s $i -c $copy
done

minStep=1
maxStep=200
for i in `seq $minStep $maxStep` ; do
	copy="$(shuf -i 1-2000 -n 1)"
	size="$(shuf -i 1-1500 -n 1)"
	./pktgen_sample01_simple.sh -i eth0 \
		-m <dst-mac-addr> -d <dst-ip-addr> -x -s $size -c $copy
done

[1] https://www.spinics.net/lists/netdev/msg505065.html
[2] https://github.com/netoptimizer/network-testing/blob/master/pktgen/pktgen_sample01_simple.sh

Claudiu Beznea (3):
  net: macb: use netdev_tx_t return type for ndo_start_xmit functions
  net: macb: move checksum clearing outside of spinlock.
  net: macb: add support for padding and fcs computation

 drivers/net/ethernet/cadence/macb_main.c | 88 +++++++++++++++++++++++++++++---
 1 file changed, 80 insertions(+), 8 deletions(-)

-- 
2.7.4

[RFC PATCH 1/3] net: macb: use netdev_tx_t return type for ndo_start_xmit functions

[Claudiu Beznea]

Use netdev_tx_t return type for ndo_start_xmit function of macb driver.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 96cc03a6d942..860436474c3e 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1651,7 +1651,7 @@ static inline int macb_clear_csum(struct sk_buff *skb)
 	return 0;
 }
 
-static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
+static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	u16 queue_index = skb_get_queue_mapping(skb);
 	struct macb *bp = netdev_priv(dev);
@@ -1660,6 +1660,7 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	unsigned int desc_cnt, nr_frags, frag_size, f;
 	unsigned int hdrlen;
 	bool is_lso, is_udp = 0;
+	netdev_tx_t ret = NETDEV_TX_OK;
 
 	is_lso = (skb_shinfo(skb)->gso_size != 0);
 
@@ -1739,7 +1740,7 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 unlock:
 	spin_unlock_irqrestore(&bp->lock, flags);
 
-	return NETDEV_TX_OK;
+	return ret;
 }
 
 static void macb_init_rx_buffer_size(struct macb *bp, size_t size)
@@ -3547,7 +3548,8 @@ static int at91ether_close(struct net_device *dev)
 }
 
 /* Transmit packet */
-static int at91ether_start_xmit(struct sk_buff *skb, struct net_device *dev)
+static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
+					struct net_device *dev)
 {
 	struct macb *lp = netdev_priv(dev);
 
-- 
2.7.4

[RFC PATCH 2/3] net: macb: move checksum clearing outside of spinlock

[Claudiu Beznea]

Move checksum clearing outside of spinlock. The SKB is protected by
networking lock (HARD_TX_LOCK()).

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 860436474c3e..1c12afe4a0ce 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1662,6 +1662,11 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	bool is_lso, is_udp = 0;
 	netdev_tx_t ret = NETDEV_TX_OK;
 
+	if (macb_clear_csum(skb)) {
+		dev_kfree_skb_any(skb);
+		return ret;
+	}
+
 	is_lso = (skb_shinfo(skb)->gso_size != 0);
 
 	if (is_lso) {
@@ -1717,11 +1722,6 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		return NETDEV_TX_BUSY;
 	}
 
-	if (macb_clear_csum(skb)) {
-		dev_kfree_skb_any(skb);
-		goto unlock;
-	}
-
 	/* Map socket buffer for DMA transfer */
 	if (!macb_tx_map(bp, queue, skb, hdrlen)) {
 		dev_kfree_skb_any(skb);
-- 
2.7.4

[RFC PATCH 3/3] net: macb: add support for padding and fcs computation

[Claudiu Beznea]

For packets with computed IP/TCP/UDP checksum there is no need to tell
hardware to recompute it. For such kind of packets hardware expects
the packet to be at least 64 bytes and FCS to be computed.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 70 ++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 1c12afe4a0ce..43d700191f82 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -10,6 +10,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 #include <linux/clk.h>
+#include <linux/crc32.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/kernel.h>
@@ -1565,6 +1566,9 @@ static unsigned int macb_tx_map(struct macb *bp,
 		if (i == queue->tx_head) {
 			ctrl |= MACB_BF(TX_LSO, lso_ctrl);
 			ctrl |= MACB_BF(TX_TCP_SEQ_SRC, seq_ctrl);
+			if ((bp->dev->features & NETIF_F_HW_CSUM) &&
+			    skb->ip_summed != CHECKSUM_PARTIAL && !lso_ctrl)
+				ctrl |= MACB_BIT(TX_NOCRC);
 		} else
 			/* Only set MSS/MFS on payload descriptors
 			 * (second or later descriptor)
@@ -1651,6 +1655,67 @@ static inline int macb_clear_csum(struct sk_buff *skb)
 	return 0;
 }
 
+static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *ndev)
+{
+	struct sk_buff *nskb;
+	int padlen = ETH_ZLEN - (*skb)->len;
+	int headroom = skb_headroom(*skb);
+	int tailroom = skb_tailroom(*skb);
+	bool cloned = skb_cloned(*skb) || skb_header_cloned(*skb);
+	u32 fcs;
+
+	if (!(ndev->features & NETIF_F_HW_CSUM) ||
+	    !((*skb)->ip_summed != CHECKSUM_PARTIAL) ||
+	    skb_shinfo(*skb)->gso_size)	/* Not available for GSO */
+		return 0;
+
+	if (padlen <= 0) {
+		/* FCS could be appeded to tailroom. */
+		if (tailroom >= ETH_FCS_LEN)
+			goto add_fcs;
+		/* FCS could be appeded by moving data to headroom. */
+		else if (!cloned && headroom + tailroom >= ETH_FCS_LEN)
+			padlen = 0;
+		/* No room for FCS, need to reallocate skb. */
+		else
+			padlen = ETH_FCS_LEN - tailroom;
+	} else {
+		/* Add room for FCS. */
+		padlen += ETH_FCS_LEN;
+	}
+
+	if (!cloned && headroom + tailroom >= padlen) {
+		(*skb)->data = memmove((*skb)->head, (*skb)->data, (*skb)->len);
+		skb_set_tail_pointer(*skb, (*skb)->len);
+	} else {
+		nskb = skb_copy_expand(*skb, 0, padlen, GFP_ATOMIC);
+		if (!nskb)
+			return -ENOMEM;
+
+		dev_kfree_skb_any(*skb);
+		*skb = nskb;
+	}
+
+	if (padlen) {
+		if (padlen >= ETH_FCS_LEN)
+			skb_put_zero(*skb, padlen - ETH_FCS_LEN);
+		else
+			skb_trim(*skb, ETH_FCS_LEN - padlen);
+	}
+
+add_fcs:
+	/* set FCS to packet */
+	fcs = crc32_le(~0, (*skb)->data, (*skb)->len);
+	fcs = ~fcs;
+
+	skb_put_u8(*skb, fcs		& 0xff);
+	skb_put_u8(*skb, (fcs >> 8)	& 0xff);
+	skb_put_u8(*skb, (fcs >> 16)	& 0xff);
+	skb_put_u8(*skb, (fcs >> 24)	& 0xff);
+
+	return 0;
+}
+
 static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	u16 queue_index = skb_get_queue_mapping(skb);
@@ -1667,6 +1732,11 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		return ret;
 	}
 
+	if (macb_pad_and_fcs(&skb, dev)) {
+		dev_kfree_skb_any(skb);
+		return ret;
+	}
+
 	is_lso = (skb_shinfo(skb)->gso_size != 0);
 
 	if (is_lso) {
-- 
2.7.4

Re: [RFC PATCH 3/3] net: macb: add support for padding and fcs computation

[David Miller]

From: Claudiu Beznea <claudiu.beznea@microchip.com>
Date: Wed, 18 Jul 2018 15:58:09 +0300

>  
> +static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *ndev)
> +{
> +	struct sk_buff *nskb;
> +	int padlen = ETH_ZLEN - (*skb)->len;
> +	int headroom = skb_headroom(*skb);
> +	int tailroom = skb_tailroom(*skb);
> +	bool cloned = skb_cloned(*skb) || skb_header_cloned(*skb);
> +	u32 fcs;

Please keep local variable ordered from longest to shortest line
(ie. reverse christmas tree format).

Thank you.

Re: [RFC PATCH 3/3] net: macb: add support for padding and fcs computation

[Claudiu Beznea]

On 18.07.2018 20:54, David Miller wrote:
> From: Claudiu Beznea <claudiu.beznea@microchip.com>
> Date: Wed, 18 Jul 2018 15:58:09 +0300
> 
>>  
>> +static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *ndev)
>> +{
>> +	struct sk_buff *nskb;
>> +	int padlen = ETH_ZLEN - (*skb)->len;
>> +	int headroom = skb_headroom(*skb);
>> +	int tailroom = skb_tailroom(*skb);
>> +	bool cloned = skb_cloned(*skb) || skb_header_cloned(*skb);
>> +	u32 fcs;
> 
> Please keep local variable ordered from longest to shortest line
> (ie. reverse christmas tree format).

OK! Thank you!

> 
> Thank you.
>