Patchwork [v2,net,1/1] r8169: fix offloaded tx checksum for small packets.

login
register
mail settings
Submitter françois romieu
Date May 18, 2013, 11:24 a.m.
Message ID <20130518112446.GA21268@electric-eye.fr.zoreil.com>
Download mbox | patch
Permalink /patch/244756/
State Accepted
Delegated to: David Miller
Headers show

Comments

françois romieu - May 18, 2013, 11:24 a.m.
8168evl offloaded checksums are wrong since commit
e5195c1f31f399289347e043d6abf3ffa80f0005 ("r8169: fix 8168evl frame padding.")
pads small packets to 60 bytes (without ethernet checksum). Typical symptoms
appear as UDP checksums which are wrong by the count of added bytes.

It isn't worth compensating. Let the driver checksum.

Due to the skb length changes, TSO code is moved before the Tx descriptor gets
written.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---

 I reworked some code since v1 to avoid wtf effects in the future. Holger's
 Tested-by is thus not included. While I trust my testing, it would be nice
 to wait for his ack as well.

 This is obviously stuff for -stable.

 drivers/net/ethernet/realtek/r8169.c | 41 ++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 14 deletions(-)
Holger Hoffstätte - May 18, 2013, 2:42 p.m.
On 18.05.2013 13:24, Francois Romieu wrote:
> 8168evl offloaded checksums are wrong since commit
> e5195c1f31f399289347e043d6abf3ffa80f0005 ("r8169: fix 8168evl frame padding.")
> pads small packets to 60 bytes (without ethernet checksum). Typical symptoms
> appear as UDP checksums which are wrong by the count of added bytes.
> 
> It isn't worth compensating. Let the driver checksum.
> 
> Due to the skb length changes, TSO code is moved before the Tx descriptor gets
> written.
> 
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>

Tested again, still working for me.

Tested-by: Holger Hoffstätte <holger.hoffstaette@googlemail.com>

-h

--
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
David Miller - May 20, 2013, 7:01 a.m.
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Sat, 18 May 2013 13:24:46 +0200

> 8168evl offloaded checksums are wrong since commit
> e5195c1f31f399289347e043d6abf3ffa80f0005 ("r8169: fix 8168evl frame padding.")
> pads small packets to 60 bytes (without ethernet checksum). Typical symptoms
> appear as UDP checksums which are wrong by the count of added bytes.
> 
> It isn't worth compensating. Let the driver checksum.
> 
> Due to the skb length changes, TSO code is moved before the Tx descriptor gets
> written.
> 
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
> ---
> 
>  I reworked some code since v1 to avoid wtf effects in the future. Holger's
>  Tested-by is thus not included. While I trust my testing, it would be nice
>  to wait for his ack as well.
> 
>  This is obviously stuff for -stable.

Applied and queued up for -stable, thanks!
--
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
Holger Hoffstätte - June 10, 2013, 11:12 a.m.
On 18.05.2013 16:42, Holger Hoffstätte wrote:
> On 18.05.2013 13:24, Francois Romieu wrote:
>> 8168evl offloaded checksums are wrong since commit
>> e5195c1f31f399289347e043d6abf3ffa80f0005 ("r8169: fix 8168evl frame padding.")
>> pads small packets to 60 bytes (without ethernet checksum). Typical symptoms
>> appear as UDP checksums which are wrong by the count of added bytes.
>>
>> It isn't worth compensating. Let the driver checksum.
>>
>> Due to the skb length changes, TSO code is moved before the Tx descriptor gets
>> written.
>>
>> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
> 
> Tested again, still working for me.
> 
> Tested-by: Holger Hoffstätte <holger.hoffstaette@googlemail.com>

ping:

As far as I can tell this patch is still not in -stable 3.9.5 or queued
up for the next iteration.

Commit in Linus' tree is b423e9ae49d78ea3f53b131c8d5a6087aed16fd6

thanks :)

-h

--
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
françois romieu - June 10, 2013, 9:54 p.m.
Holger Hoffstätte <holger.hoffstaette@googlemail.com> :
[...]
> As far as I can tell this patch is still not in -stable 3.9.5 or queued
> up for the next iteration.

It is queued in davem's stable bundle, see:

http://patchwork.ozlabs.org/bundle/davem/stable/?state=*

I'll propagate it once it reaches -stable.

Patch

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 79c520b..393f961 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5856,7 +5856,20 @@  err_out:
 	return -EIO;
 }
 
-static inline void rtl8169_tso_csum(struct rtl8169_private *tp,
+static bool rtl_skb_pad(struct sk_buff *skb)
+{
+	if (skb_padto(skb, ETH_ZLEN))
+		return false;
+	skb_put(skb, ETH_ZLEN - skb->len);
+	return true;
+}
+
+static bool rtl_test_hw_pad_bug(struct rtl8169_private *tp, struct sk_buff *skb)
+{
+	return skb->len < ETH_ZLEN && tp->mac_version == RTL_GIGA_MAC_VER_34;
+}
+
+static inline bool rtl8169_tso_csum(struct rtl8169_private *tp,
 				    struct sk_buff *skb, u32 *opts)
 {
 	const struct rtl_tx_desc_info *info = tx_desc_info + tp->txd_version;
@@ -5869,13 +5882,20 @@  static inline void rtl8169_tso_csum(struct rtl8169_private *tp,
 	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
 		const struct iphdr *ip = ip_hdr(skb);
 
+		if (unlikely(rtl_test_hw_pad_bug(tp, skb)))
+			return skb_checksum_help(skb) == 0 && rtl_skb_pad(skb);
+
 		if (ip->protocol == IPPROTO_TCP)
 			opts[offset] |= info->checksum.tcp;
 		else if (ip->protocol == IPPROTO_UDP)
 			opts[offset] |= info->checksum.udp;
 		else
 			WARN_ON_ONCE(1);
+	} else {
+		if (unlikely(rtl_test_hw_pad_bug(tp, skb)))
+			return rtl_skb_pad(skb);
 	}
+	return true;
 }
 
 static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
@@ -5896,17 +5916,15 @@  static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 		goto err_stop_0;
 	}
 
-	/* 8168evl does not automatically pad to minimum length. */
-	if (unlikely(tp->mac_version == RTL_GIGA_MAC_VER_34 &&
-		     skb->len < ETH_ZLEN)) {
-		if (skb_padto(skb, ETH_ZLEN))
-			goto err_update_stats;
-		skb_put(skb, ETH_ZLEN - skb->len);
-	}
-
 	if (unlikely(le32_to_cpu(txd->opts1) & DescOwn))
 		goto err_stop_0;
 
+	opts[1] = cpu_to_le32(rtl8169_tx_vlan_tag(skb));
+	opts[0] = DescOwn;
+
+	if (!rtl8169_tso_csum(tp, skb, opts))
+		goto err_update_stats;
+
 	len = skb_headlen(skb);
 	mapping = dma_map_single(d, skb->data, len, DMA_TO_DEVICE);
 	if (unlikely(dma_mapping_error(d, mapping))) {
@@ -5918,11 +5936,6 @@  static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 	tp->tx_skb[entry].len = len;
 	txd->addr = cpu_to_le64(mapping);
 
-	opts[1] = cpu_to_le32(rtl8169_tx_vlan_tag(skb));
-	opts[0] = DescOwn;
-
-	rtl8169_tso_csum(tp, skb, opts);
-
 	frags = rtl8169_xmit_frags(tp, skb, opts);
 	if (frags < 0)
 		goto err_dma_1;