diff mbox

r8169: fix offloaded tx checksum for small packets.

Message ID 20130516223857.GA20140@electric-eye.fr.zoreil.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Francois Romieu May 16, 2013, 10:38 p.m. UTC 
8168evl offloaded checksums are wrong since
e5195c1f31f399289347e043d6abf3ffa80f0005 pads small packets to
60 bytes (without ethernet checksum). Observed UDP checksums are
typically wrong by the count of added bytes.

It isn't worth compensating. Let's checksum by hand, then pad.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
Cc: Hayes Wang <hayeswang@realtek.com>
Tested-by: Holger Hoffstätte <holger.hoffstaette@googlemail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 40 +++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 14 deletions(-)

Comments

Sergei Shtylyov May 16, 2013, 10:59 p.m. UTC | #1 
Hello.

On 05/17/2013 02:38 AM, Francois Romieu wrote:

> 8168evl offloaded checksums are wrong since
> e5195c1f31f399289347e043d6abf3ffa80f0005

     Please, also specify that commit's summary line in parens.

> pads small packets to
> 60 bytes (without ethernet checksum). Observed UDP checksums are
> typically wrong by the count of added bytes.
>
> It isn't worth compensating. Let's checksum by hand, then pad.
>
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
> Cc: Hayes Wang <hayeswang@realtek.com>
> Tested-by: Holger Hoffstätte <holger.hoffstaette@googlemail.com>

WBR, Sergei

--
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
Ben Hutchings May 16, 2013, 11 p.m. UTC | #2 
On Fri, 2013-05-17 at 00:38 +0200, Francois Romieu wrote:
> 8168evl offloaded checksums are wrong since
> e5195c1f31f399289347e043d6abf3ffa80f0005 pads small packets to
> 60 bytes (without ethernet checksum). Observed UDP checksums are
> typically wrong by the count of added bytes.
> 
> It isn't worth compensating. Let's checksum by hand, then pad.
[...]
> @@ -5869,13 +5877,24 @@ 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(skb->len < ETH_ZLEN &&
> +		    (tp->mac_version == RTL_GIGA_MAC_VER_34))) {
> +			skb_checksum_help(skb);
> +			return rtl_skb_pad(skb);
[...]

skb_checksum_help() can fail, though it is unlikely for such a short
packet.  So I think this should be:

			return skb_checksum_help(skb) == 0 && rtl_skb_pad(skb);

Ben.
Francois Romieu May 18, 2013, 10:10 a.m. UTC | #3 
Ben Hutchings <bhutchings@solarflare.com> :
[...]
> skb_checksum_help() can fail, though it is unlikely for such a short
> packet.  So I think this should be:
> 
> 			return skb_checksum_help(skb) == 0 && rtl_skb_pad(skb);

Will do. I don't have consistent test results anyway. :o(

Thanks.
diff mbox

Patch

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 79c520b..a05596c 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5856,7 +5856,15 @@  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 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 +5877,24 @@  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(skb->len < ETH_ZLEN &&
+		    (tp->mac_version == RTL_GIGA_MAC_VER_34))) {
+			skb_checksum_help(skb);
+			return 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(skb->len < ETH_ZLEN &&
+		    (tp->mac_version == RTL_GIGA_MAC_VER_34)))
+			return rtl_skb_pad(skb);
 	}
+	return true;
 }
 
 static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
@@ -5896,17 +5915,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 +5935,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;