diff mbox

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

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

Commit Message

Francois Romieu July 18, 2012, 8:12 p.m. UTC 
David Miller <davem@davemloft.net> :
> From: hayeswang <hayeswang@realtek.com>
> > Francois Romieu [mailto:romieu@fr.zoreil.com] 
> > [...]
> > 
> >> Hayes, should we not add into the kernel driver something similar to
> >> the rtl8168_start_xmit::skb_checksum_help stuff in Realtek's 
> >> 8168 driver ?
> >> There seems to be a bug for (skb->len < 60 && RTL_GIGA_MAC_VER_34.
> > 
> > For RTL8168E-VL (RTL_GIGA_MAC_VER_34), the hardware wouldn't send the packet
> > with the length less than 60 bytes. The hardware should pad this kind of packet
> > to 60 bytes, but it wouldn't. Therefore, the software has to pad the packet to
> > 60 bytes. However, the hw checksum would be incorrect for the modified packet,
> > so the software checksum is necessary.
> 
> I wonder how the hardware checksum can be incorrectly calculated if the padding
> is done with zeros?

A part of the apparent problem may stem from the fact that Realtek's 8168
driver claims a modified length but it does not really skb_padto... 

Hayes, would the patch below fix the original problem ?

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

David Miller July 18, 2012, 8:28 p.m. UTC | #1 
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Wed, 18 Jul 2012 22:12:01 +0200

> David Miller <davem@davemloft.net> :
>> From: hayeswang <hayeswang@realtek.com>
>> > Francois Romieu [mailto:romieu@fr.zoreil.com] 
>> > [...]
>> > 
>> >> Hayes, should we not add into the kernel driver something similar to
>> >> the rtl8168_start_xmit::skb_checksum_help stuff in Realtek's 
>> >> 8168 driver ?
>> >> There seems to be a bug for (skb->len < 60 && RTL_GIGA_MAC_VER_34.
>> > 
>> > For RTL8168E-VL (RTL_GIGA_MAC_VER_34), the hardware wouldn't send the packet
>> > with the length less than 60 bytes. The hardware should pad this kind of packet
>> > to 60 bytes, but it wouldn't. Therefore, the software has to pad the packet to
>> > 60 bytes. However, the hw checksum would be incorrect for the modified packet,
>> > so the software checksum is necessary.
>> 
>> I wonder how the hardware checksum can be incorrectly calculated if the padding
>> is done with zeros?
> 
> A part of the apparent problem may stem from the fact that Realtek's 8168
> driver claims a modified length but it does not really skb_padto... 
> 
> Hayes, would the patch below fix the original problem ?

A NETDEV_TX_OK return means we accepted the SKB, it doesn't look like
that's what you are doing in the skb_padto() failure path.

--
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
Francois Romieu July 18, 2012, 9:44 p.m. UTC | #2 
David Miller <davem@davemloft.net> :
[...]
> A NETDEV_TX_OK return means we accepted the SKB, it doesn't look like
> that's what you are doing in the skb_padto() failure path.

?

- skb_padto fails
  (original skb is implicitely freed)
- skb_padto returns error status (!= 0)
- rtl8169_tso_csum returns false
- start_xmit returns NETDEV_TX_OK. 

I'll search the missing "!" after some sleep if that's what you are talking
about. Otherwise than that, I don't get it.
Eric Dumazet July 18, 2012, 10:05 p.m. UTC | #3 
On Wed, 2012-07-18 at 23:44 +0200, Francois Romieu wrote:
> David Miller <davem@davemloft.net> :
> [...]
> > A NETDEV_TX_OK return means we accepted the SKB, it doesn't look like
> > that's what you are doing in the skb_padto() failure path.
> 
> ?
> 
> - skb_padto fails
>   (original skb is implicitely freed)
> - skb_padto returns error status (!= 0)
> - rtl8169_tso_csum returns false
> - start_xmit returns NETDEV_TX_OK. 
> 
> I'll search the missing "!" after some sleep if that's what you are talking
> about. Otherwise than that, I don't get it.
> 


Yes, I believe your patch is fine.

In fact many drivers dont account the error in their stats.

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
David Miller July 18, 2012, 10:24 p.m. UTC | #4 
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 19 Jul 2012 00:05:36 +0200

> On Wed, 2012-07-18 at 23:44 +0200, Francois Romieu wrote:
>> David Miller <davem@davemloft.net> :
>> [...]
>> > A NETDEV_TX_OK return means we accepted the SKB, it doesn't look like
>> > that's what you are doing in the skb_padto() failure path.
>> 
>> ?
>> 
>> - skb_padto fails
>>   (original skb is implicitely freed)
>> - skb_padto returns error status (!= 0)
>> - rtl8169_tso_csum returns false
>> - start_xmit returns NETDEV_TX_OK. 
>> 
>> I'll search the missing "!" after some sleep if that's what you are talking
>> about. Otherwise than that, I don't get it.
>> 
> 
> 
> Yes, I believe your patch is fine.
> 
> In fact many drivers dont account the error in their stats.

My bad, I forgot that skb_padto() frees the SKB on failure.
--
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
Hayes Wang July 20, 2012, 2:11 a.m. UTC | #5 
Francois Romieu [mailto:romieu@fr.zoreil.com] 
[...]
> A part of the apparent problem may stem from the fact that 
> Realtek's 8168
> driver claims a modified length but it does not really skb_padto... 
> 
> Hayes, would the patch below fix the original problem ?

According to the response from our hw engineer, it still has the problem even
though you pad the packet to 60 bytes with zeroes. I would still test this patch
to verify it.

>  static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
> @@ -5797,7 +5804,8 @@ static netdev_tx_t 
> rtl8169_start_xmit(struct sk_buff *skb,
>  	opts[1] = cpu_to_le32(rtl8169_tx_vlan_tag(tp, skb));
>  	opts[0] = DescOwn;
>  
> -	rtl8169_tso_csum(tp, skb, opts);
> +	if (!rtl8169_tso_csum(tp, skb, opts))
> +		goto err_update_stats;
>  

I think you should check the length of opts1 of the descriptor, too. Besides,
how about the length of dma_map_xxx? Should it use the original length or the
modified length?

>  	frags = rtl8169_xmit_frags(tp, skb, opts);
>  	if (frags < 0)
> @@ -5853,6 +5861,7 @@ err_dma_1:
>  	rtl8169_unmap_tx_skb(d, tp->tx_skb + entry, txd);
>  err_dma_0:
>  	dev_kfree_skb(skb);
> +err_update_stats:
>  	dev->stats.tx_dropped++;
>  	return NETDEV_TX_OK;
>  
 
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
Hayes Wang July 20, 2012, 7:14 a.m. UTC | #6 
Francois Romieu <romieu@fr.zoreil.com>

> >> > A NETDEV_TX_OK return means we accepted the SKB, it 
> doesn't look like
> >> > that's what you are doing in the skb_padto() failure path.
> >> 
> >> ?
> >> 
> >> - skb_padto fails
> >>   (original skb is implicitely freed)
> >> - skb_padto returns error status (!= 0)
> >> - rtl8169_tso_csum returns false
> >> - start_xmit returns NETDEV_TX_OK. 
> >> 
> >> I'll search the missing "!" after some sleep if that's 
> what you are talking
> >> about. Otherwise than that, I don't get it.
> >> 
> > 
> > 
> > Yes, I believe your patch is fine.
> > 
> > In fact many drivers dont account the error in their stats.
> 
> My bad, I forgot that skb_padto() frees the SKB on failure.
> 

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. The
software checksum is necessary.
 
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:17 p.m. UTC | #7 
From: hayeswang <hayeswang@realtek.com>
Date: Fri, 20 Jul 2012 15:14:26 +0800

> 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. The
> software checksum is necessary.

It's really strange that the hardware has any reason to update the IP
header length field when it is asked to compute the checksum.
--
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..a463697 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,12 @@  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 < 60 &&
+		    (tp->mac_version == RTL_GIGA_MAC_VER_34) &&
+		    skb_padto(skb, ETH_ZLEN))) {
+			return false;
+		}
+
 		if (ip->protocol == IPPROTO_TCP)
 			opts[offset] |= info->checksum.tcp;
 		else if (ip->protocol == IPPROTO_UDP)
@@ -5760,6 +5766,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,
@@ -5797,7 +5804,8 @@  static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 	opts[1] = cpu_to_le32(rtl8169_tx_vlan_tag(tp, skb));
 	opts[0] = DescOwn;
 
-	rtl8169_tso_csum(tp, skb, opts);
+	if (!rtl8169_tso_csum(tp, skb, opts))
+		goto err_update_stats;
 
 	frags = rtl8169_xmit_frags(tp, skb, opts);
 	if (frags < 0)
@@ -5853,6 +5861,7 @@  err_dma_1:
 	rtl8169_unmap_tx_skb(d, tp->tx_skb + entry, txd);
 err_dma_0:
 	dev_kfree_skb(skb);
+err_update_stats:
 	dev->stats.tx_dropped++;
 	return NETDEV_TX_OK;