[PATCH] net: fec: remove HW IP header checksum for IPV6 frame

[PATCH] net: fec: remove HW IP header checksum for IPV6 frame

[Fugang Duan]

The commit 96c50caa5148 (net: fec: Enable IP header hardware checksum)
enable HW IP header checksum for IPV4 and IPV6, which causes IPV6 TCP/UDP
cannot work. (The issue is reported by Russell King)

The reason is that FEC HW IP cannot detect the transmit frame type (can
detect received frame type), and treats all packets as IPv4, overwrites
the point in the packet which would be the IPv4 checksum field. Since IPV6
don't have IP header checksum, so this results in the frame being corrupted.

The patch just add software detect the current packet type, if it is IPV6
frame, it don't enable IP header checksum.

Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Fugang Duan <B38611@freescale.com>
---
 drivers/net/ethernet/freescale/fec_main.c |   36 +++++++++++++++++++++-------
 1 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 38d9d27..c13185a 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -320,6 +320,11 @@ static void *swap_buffer(void *bufaddr, int len)
 	return bufaddr;
 }
 
+static inline bool is_ipv4_pkt(struct sk_buff *skb)
+{
+	return skb->protocol == htons(ETH_P_IP) && ip_hdr(skb)->version == 4;
+}
+
 static int
 fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
 {
@@ -330,7 +335,8 @@ fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
 	if (unlikely(skb_cow_head(skb, 0)))
 		return -1;
 
-	ip_hdr(skb)->check = 0;
+	if (is_ipv4_pkt(skb))
+		ip_hdr(skb)->check = 0;
 	*(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) = 0;
 
 	return 0;
@@ -391,8 +397,11 @@ fec_enet_txq_submit_frag_skb(struct sk_buff *skb, struct net_device *ndev)
 		}
 
 		if (fep->bufdesc_ex) {
-			if (skb->ip_summed == CHECKSUM_PARTIAL)
-				estatus |= BD_ENET_TX_PINS | BD_ENET_TX_IINS;
+			if (skb->ip_summed == CHECKSUM_PARTIAL) {
+				estatus |= BD_ENET_TX_PINS;
+				if (is_ipv4_pkt(skb))
+					estatus |= BD_ENET_TX_IINS;
+			}
 			ebdp->cbd_bdu = 0;
 			ebdp->cbd_esc = estatus;
 		}
@@ -518,8 +527,11 @@ static int fec_enet_txq_submit_skb(struct sk_buff *skb, struct net_device *ndev)
 			fep->hwts_tx_en))
 			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 
-		if (skb->ip_summed == CHECKSUM_PARTIAL)
-			estatus |= BD_ENET_TX_PINS | BD_ENET_TX_IINS;
+		if (skb->ip_summed == CHECKSUM_PARTIAL) {
+			estatus |= BD_ENET_TX_PINS;
+			if (is_ipv4_pkt(skb))
+				estatus |= BD_ENET_TX_IINS;
+		}
 
 		ebdp->cbd_bdu = 0;
 		ebdp->cbd_esc = estatus;
@@ -590,8 +602,11 @@ fec_enet_txq_put_data_tso(struct sk_buff *skb, struct net_device *ndev,
 	}
 
 	if (fep->bufdesc_ex) {
-		if (skb->ip_summed == CHECKSUM_PARTIAL)
-			estatus |= BD_ENET_TX_PINS | BD_ENET_TX_IINS;
+		if (skb->ip_summed == CHECKSUM_PARTIAL) {
+			estatus |= BD_ENET_TX_PINS;
+			if (is_ipv4_pkt(skb))
+				estatus |= BD_ENET_TX_IINS;
+		}
 		ebdp->cbd_bdu = 0;
 		ebdp->cbd_esc = estatus;
 	}
@@ -652,8 +667,11 @@ fec_enet_txq_put_hdr_tso(struct sk_buff *skb, struct net_device *ndev,
 	bdp->cbd_datlen = hdr_len;
 
 	if (fep->bufdesc_ex) {
-		if (skb->ip_summed == CHECKSUM_PARTIAL)
-			estatus |= BD_ENET_TX_PINS | BD_ENET_TX_IINS;
+		if (skb->ip_summed == CHECKSUM_PARTIAL) {
+			estatus |= BD_ENET_TX_PINS;
+			if (is_ipv4_pkt(skb))
+				estatus |= BD_ENET_TX_IINS;
+		}
 		ebdp->cbd_bdu = 0;
 		ebdp->cbd_esc = estatus;
 	}
-- 
1.7.8

Re: [PATCH] net: fec: remove HW IP header checksum for IPV6 frame

[Russell King - ARM Linux]

On Tue, Jun 17, 2014 at 04:31:35PM +0800, Fugang Duan wrote:
> The commit 96c50caa5148 (net: fec: Enable IP header hardware checksum)
> enable HW IP header checksum for IPV4 and IPV6, which causes IPV6 TCP/UDP
> cannot work. (The issue is reported by Russell King)
> 
> The reason is that FEC HW IP cannot detect the transmit frame type (can
> detect received frame type), and treats all packets as IPv4, overwrites
> the point in the packet which would be the IPv4 checksum field. Since IPV6
> don't have IP header checksum, so this results in the frame being corrupted.
> 
> The patch just add software detect the current packet type, if it is IPV6
> frame, it don't enable IP header checksum.

I think it /can/ detect the packet type - the hardware showed no signs of
overwriting the checksum field with a non-zero value.  The zeroing of the
bytes seems to come purely from the driver assuming that every partial
checksummed packet is an IPv4 packet:

> @@ -330,7 +335,8 @@ fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
>  	if (unlikely(skb_cow_head(skb, 0)))
>  		return -1;
>  
> -	ip_hdr(skb)->check = 0;
> +	if (is_ipv4_pkt(skb))
> +		ip_hdr(skb)->check = 0;
>  	*(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) = 0;
>  
>  	return 0;

So, I will first test whether avoiding this resolves the issue I'm
seeing before deciding whether to test the remainder of the patch.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH] net: fec: remove HW IP header checksum for IPV6 frame

[Russell King - ARM Linux]

On Tue, Jun 17, 2014 at 04:31:35PM +0800, Fugang Duan wrote:
> The commit 96c50caa5148 (net: fec: Enable IP header hardware checksum)
> enable HW IP header checksum for IPV4 and IPV6, which causes IPV6 TCP/UDP
> cannot work. (The issue is reported by Russell King)
> 
> The reason is that FEC HW IP cannot detect the transmit frame type (can
> detect received frame type), and treats all packets as IPv4, overwrites
> the point in the packet which would be the IPv4 checksum field. Since IPV6
> don't have IP header checksum, so this results in the frame being corrupted.
> 
> The patch just add software detect the current packet type, if it is IPV6
> frame, it don't enable IP header checksum.
> 
> Cc: Russell King <linux@arm.linux.org.uk>
> Signed-off-by: Fugang Duan <B38611@freescale.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c |   36 +++++++++++++++++++++-------
>  1 files changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 38d9d27..c13185a 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -320,6 +320,11 @@ static void *swap_buffer(void *bufaddr, int len)
>  	return bufaddr;
>  }
>  
> +static inline bool is_ipv4_pkt(struct sk_buff *skb)
> +{
> +	return skb->protocol == htons(ETH_P_IP) && ip_hdr(skb)->version == 4;
> +}
> +
>  static int
>  fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
>  {
> @@ -330,7 +335,8 @@ fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
>  	if (unlikely(skb_cow_head(skb, 0)))
>  		return -1;
>  
> -	ip_hdr(skb)->check = 0;
> +	if (is_ipv4_pkt(skb))
> +		ip_hdr(skb)->check = 0;
>  	*(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) = 0;
>  
>  	return 0;

As I suspected, just applying the above two hunks fixes the problem
I was seeing.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH] net: fec: remove HW IP header checksum for IPV6 frame

[David Miller]

From: Russell King - ARM Linux <linux@arm.linux.org.uk>
Date: Tue, 17 Jun 2014 17:31:21 +0100

> On Tue, Jun 17, 2014 at 04:31:35PM +0800, Fugang Duan wrote:
>> The commit 96c50caa5148 (net: fec: Enable IP header hardware checksum)
>> enable HW IP header checksum for IPV4 and IPV6, which causes IPV6 TCP/UDP
>> cannot work. (The issue is reported by Russell King)
>> 
>> The reason is that FEC HW IP cannot detect the transmit frame type (can
>> detect received frame type), and treats all packets as IPv4, overwrites
>> the point in the packet which would be the IPv4 checksum field. Since IPV6
>> don't have IP header checksum, so this results in the frame being corrupted.
>> 
>> The patch just add software detect the current packet type, if it is IPV6
>> frame, it don't enable IP header checksum.
>> 
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Signed-off-by: Fugang Duan <B38611@freescale.com>
>> ---
>>  drivers/net/ethernet/freescale/fec_main.c |   36 +++++++++++++++++++++-------
>>  1 files changed, 27 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
>> index 38d9d27..c13185a 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -320,6 +320,11 @@ static void *swap_buffer(void *bufaddr, int len)
>>  	return bufaddr;
>>  }
>>  
>> +static inline bool is_ipv4_pkt(struct sk_buff *skb)
>> +{
>> +	return skb->protocol == htons(ETH_P_IP) && ip_hdr(skb)->version == 4;
>> +}
>> +
>>  static int
>>  fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
>>  {
>> @@ -330,7 +335,8 @@ fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
>>  	if (unlikely(skb_cow_head(skb, 0)))
>>  		return -1;
>>  
>> -	ip_hdr(skb)->check = 0;
>> +	if (is_ipv4_pkt(skb))
>> +		ip_hdr(skb)->check = 0;
>>  	*(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) = 0;
>>  
>>  	return 0;
> 
> As I suspected, just applying the above two hunks fixes the problem
> I was seeing.

Someone please respin the patch so that it's just these two hunks and that
the commit message accurately describes the real situation.

Thanks.

Re: [PATCH] net: fec: remove HW IP header checksum for IPV6 frame

[Russell King - ARM Linux]

On Tue, Jun 17, 2014 at 03:34:07PM -0700, David Miller wrote:
> From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Date: Tue, 17 Jun 2014 17:31:21 +0100
> > As I suspected, just applying the above two hunks fixes the problem
> > I was seeing.
> 
> Someone please respin the patch so that it's just these two hunks and that
> the commit message accurately describes the real situation.

I'm hoping for a reply from Freescale before I give an explicit tested-by
against this.  It would be nice to have some explicit confirmation that
they would be happy with a reduced patch.  It may be that the hardware
really does need the IP header checksum bit cleared for non-IPv4
packets.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

RE: [PATCH] net: fec: remove HW IP header checksum for IPV6 frame

[fugang.duan]

From: Russell King - ARM Linux <linux@arm.linux.org.uk> Data: Wednesday, June 18, 2014 7:22 AM
>To: David Miller
>Cc: Duan Fugang-B38611; netdev@vger.kernel.org
>Subject: Re: [PATCH] net: fec: remove HW IP header checksum for IPV6 frame
>
>On Tue, Jun 17, 2014 at 03:34:07PM -0700, David Miller wrote:
>> From: Russell King - ARM Linux <linux@arm.linux.org.uk>
>> Date: Tue, 17 Jun 2014 17:31:21 +0100
>> > As I suspected, just applying the above two hunks fixes the problem
>> > I was seeing.
>>
>> Someone please respin the patch so that it's just these two hunks and
>> that the commit message accurately describes the real situation.
>
>I'm hoping for a reply from Freescale before I give an explicit tested-by
>against this.  It would be nice to have some explicit confirmation that
>they would be happy with a reduced patch.  It may be that the hardware
>really does need the IP header checksum bit cleared for non-IPv4 packets.
>
I test it again that is your expectation HW can detect IPV6 frame type, if it is IPV6 packet,
The IP header checksum function don't take effect. 
I will submit the patch again.