diff mbox

[RFC] r8169 : why SG / TX checksum are default disabled

Message ID 20120720100846.GA17398@electric-eye.fr.zoreil.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Francois Romieu July 20, 2012, 10:08 a.m. UTC 
hayeswang <hayeswang@realtek.com> :
[...]
> I find that the total length field of IP header would be modified if the hw
> checksum is enabled. Therefore, skb_padto + hw checksum wouldn't work.

Ok, my patch completely ignored the fact that skb_padto does not change the
length.

However skb_padto + length adjustement + hw checksum should work (at least in
theory if not in the patch below) ?

--
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

Comments

Hayes Wang July 20, 2012, 4:01 p.m. UTC | #1 
Francois Romieu [mailto:romieu@fr.zoreil.com] 

[...]
> > I find that the total length field of IP header would be 
> modified if the hw
> > checksum is enabled. Therefore, skb_padto + hw checksum 
> wouldn't work.
> 
> Ok, my patch completely ignored the fact that skb_padto does 
> not change the
> length.
> 
> However skb_padto + length adjustement + hw checksum should 
> work (at least in
> theory if not in the patch below) ?

If the hw only fills in the checksum fields of IP header, UDP header, and TCP
header, the patch would work. However, the hw would also fill in the total
length field of IP header, so it causes problems. For example, I send a packet
with ethernet header 14 bytes + IP header 20 bytes + data 20 bytes = 54 bytes.

Case 1: Software checksum + pad zeroes to 60 bytes
   Receiver gets this packet and finds the total length in IP header would be 40
bytes. Therefore, the receiver knows the data would be 40 - 20 (IP header) = 20
bytes.

Case 2: pad zeroes to 60 bytes + hw checksum
   Receiver gets this packet and would find the total length in IP header is 40
+ (60-54) = 46 bytes, not 40 bytes. Therefore, the receiver consider the data
would be 46 - 20 = 26 bytes. However, the final 6 bytes should not be the parts
of data.
 
Best Regards,
Hayes

--
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 July 20, 2012, 4:28 p.m. UTC | #2 
From: hayeswang <hayeswang@realtek.com>
Date: Sat, 21 Jul 2012 00:01:38 +0800

> However, the hw would also fill in the total length field of IP
> header, so it causes problems.

Why does the HW modify fields we did not ask it to?
--
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/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index be4e00f..8d0cc09 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5740,7 +5740,7 @@  err_out:
 	return -EIO;
 }
 
-static inline void rtl8169_tso_csum(struct rtl8169_private *tp,
+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;
@@ -5753,6 +5753,13 @@  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))) {
+			if (skb_padto(skb, ETH_ZLEN))
+				return false;
+			skb_put(skb, ETH_ZLEN - skb->len);
+		}
+
 		if (ip->protocol == IPPROTO_TCP)
 			opts[offset] |= info->checksum.tcp;
 		else if (ip->protocol == IPPROTO_UDP)
@@ -5760,6 +5767,7 @@  static inline void rtl8169_tso_csum(struct rtl8169_private *tp,
 		else
 			WARN_ON_ONCE(1);
 	}
+	return true;
 }
 
 static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
@@ -5783,25 +5791,26 @@  static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 	if (unlikely(le32_to_cpu(txd->opts1) & DescOwn))
 		goto err_stop_0;
 
+	opts[1] = cpu_to_le32(rtl8169_tx_vlan_tag(tp, skb));
+	opts[0] = DescOwn;
+
+	if (!rtl8169_tso_csum(tp, skb, opts))
+		goto err_update_stats_0;
+
 	len = skb_headlen(skb);
 	mapping = dma_map_single(d, skb->data, len, DMA_TO_DEVICE);
 	if (unlikely(dma_mapping_error(d, mapping))) {
 		if (net_ratelimit())
 			netif_err(tp, drv, dev, "Failed to map TX DMA!\n");
-		goto err_dma_0;
+		goto err_free_skb_1;
 	}
 
 	tp->tx_skb[entry].len = len;
 	txd->addr = cpu_to_le64(mapping);
 
-	opts[1] = cpu_to_le32(rtl8169_tx_vlan_tag(tp, skb));
-	opts[0] = DescOwn;
-
-	rtl8169_tso_csum(tp, skb, opts);
-
 	frags = rtl8169_xmit_frags(tp, skb, opts);
 	if (frags < 0)
-		goto err_dma_1;
+		goto err_unmap_2;
 	else if (frags)
 		opts[0] |= FirstFrag;
 	else {
@@ -5849,10 +5858,11 @@  static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 
 	return NETDEV_TX_OK;
 
-err_dma_1:
+err_unmap_2:
 	rtl8169_unmap_tx_skb(d, tp->tx_skb + entry, txd);
-err_dma_0:
+err_free_skb_1:
 	dev_kfree_skb(skb);
+err_update_stats_0:
 	dev->stats.tx_dropped++;
 	return NETDEV_TX_OK;