Broken TX checksumming offloads

Broken TX checksumming offloads

[Michał Mirosław]

Hi!

Unless I'm horribly mistaken, generic HW checksumming works as follows:
 - driver sets netdev->features & NETIF_F_HW_CSUM to indicate support
   for generic checksumming; if the flag is not set, networking core
   will checksum skb before calling ndo_start_xmit (let's ignore
   other checksumming options for now) and not pass skb with
   skb->ip_summed == CHECKSUM_PARTIAL
 - ndo_start_xmit() should use skb->csum_start and skb->csum_offset
   (or skb->csum) to update checksum in software or instruct HW to do so

Looking at pch_gbe_xmit_frame() and its callee - pch_gbe_tx_queue() it
looks like the driver should set NETIF_F_IP_CSUM instead of NETIF_F_HW_CSUM
feature.

Similar thing happens in ixgbe driver: it sets NETIF_F_HW_CSUM and checks
for skb->ip_summed == CHECKSUM_PARTIAL, but then just warns on protocols
other that TCP and SCTP (see: ixgbe_psum()).

This means that these drivers might send packets with broken checksums
when TX checksumming offload is enabled. I haven't checked other drivers, yet.

Best Regards,
Michał Mirosław

Re: Broken TX checksumming offloads

[Ben Hutchings]

On Mon, 2010-11-29 at 19:17 +0100, Michał Mirosław wrote:
> Hi!
> 
> Unless I'm horribly mistaken, generic HW checksumming works as follows:
>  - driver sets netdev->features & NETIF_F_HW_CSUM to indicate support
>    for generic checksumming; if the flag is not set, networking core
>    will checksum skb before calling ndo_start_xmit (let's ignore
>    other checksumming options for now) and not pass skb with
>    skb->ip_summed == CHECKSUM_PARTIAL
>  - ndo_start_xmit() should use skb->csum_start and skb->csum_offset
>    (or skb->csum) to update checksum in software or instruct HW to do so
> 
> Looking at pch_gbe_xmit_frame() and its callee - pch_gbe_tx_queue() it
> looks like the driver should set NETIF_F_IP_CSUM instead of NETIF_F_HW_CSUM
> feature.
>
> Similar thing happens in ixgbe driver: it sets NETIF_F_HW_CSUM and checks
> for skb->ip_summed == CHECKSUM_PARTIAL, but then just warns on protocols
> other that TCP and SCTP (see: ixgbe_psum()).

AFAIK only {TCP,UDP}/IPv{4,6} use the simple 16-bit checksum algorithm
that NETIF_F_HW_CSUM implies, so in practice it is equivalent to
NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM even though it doesn't mean the same
thing.

Older kernel versions lacked a definititon of NETIF_F_IPV6_CSUM so
out-of-tree drivers and in-tree drivers that started out-of-tree are
likely to use NETIF_F_HW_CSUM as a workaround for that.

Since the minimum size of a {TCP,UDP}/IPv6/Ethernet frame is 62 bytes +
CRC, the workaround in pch_gbe_tx_queue() may not be needed for IPv6.
(I'm assuming that the critical length of 64 bytes actually includes the
CRC, although the workaround code currently assumes otherwise.)

> This means that these drivers might send packets with broken checksums
> when TX checksumming offload is enabled. I haven't checked other drivers, yet.

They might or they might not; it's hard to tell without access to the
hardware.  But if the driver never references csum_{start,offset} then
it is probably valid to replace NETIF_F_HW_CSUM with NETIF_F_IP_CSUM |
NETIF_F_IPV6_CSUM (and similarly for the ethtool operations).

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: Broken TX checksumming offloads

[Jesse Gross]

2010/11/29 Ben Hutchings <bhutchings@solarflare.com>:
> On Mon, 2010-11-29 at 19:17 +0100, Michał Mirosław wrote:
>> Hi!
>>
>> Unless I'm horribly mistaken, generic HW checksumming works as follows:
>>  - driver sets netdev->features & NETIF_F_HW_CSUM to indicate support
>>    for generic checksumming; if the flag is not set, networking core
>>    will checksum skb before calling ndo_start_xmit (let's ignore
>>    other checksumming options for now) and not pass skb with
>>    skb->ip_summed == CHECKSUM_PARTIAL
>>  - ndo_start_xmit() should use skb->csum_start and skb->csum_offset
>>    (or skb->csum) to update checksum in software or instruct HW to do so
>>
>> Looking at pch_gbe_xmit_frame() and its callee - pch_gbe_tx_queue() it
>> looks like the driver should set NETIF_F_IP_CSUM instead of NETIF_F_HW_CSUM
>> feature.
>>
>> Similar thing happens in ixgbe driver: it sets NETIF_F_HW_CSUM and checks
>> for skb->ip_summed == CHECKSUM_PARTIAL, but then just warns on protocols
>> other that TCP and SCTP (see: ixgbe_psum()).
>
> AFAIK only {TCP,UDP}/IPv{4,6} use the simple 16-bit checksum algorithm
> that NETIF_F_HW_CSUM implies, so in practice it is equivalent to
> NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM even though it doesn't mean the same
> thing.

There are a few cases that I can think of where the extra generality
of NETIF_F_HW_CSUM is useful:

* The checksum in GRE (maybe other protocols as well) uses the same
algorithm as TCP/UDP.  In theory this means that we could use hardware
offloading for GRE on cards that have NETIF_F_HW_CSUM.  The one issue
is that NETIF_F_IP_CSUM really means TCP/UDP checksumming but we only
test if the protocol is IP.  This means that other IP based protocols
(like GRE) can't use checksum offloading since there is no software
fallback path.

* TCP/UDP packets that are deeply encapsulated.  Currently we can
express that checksum offloading is supported in one level of vlan
encapsulation through vlan_features.  However, a NIC that exposes
NETIF_F_HW_CSUM should in theory be able to checksum over a packet
that has an arbitrary number of vlan tags in front of it, a GRE
header, or any other type of encapsulation.

[PATCH] net: Fix drivers advertising HW_CSUM feature to use csum_start

[MichałMirosław]

Some drivers are using skb_transport_offset(skb) instead of skb->csum_start.
This does not matter now, but if someone implements checksumming of
encapsulated packets then this will break silently.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/net/atl1c/atl1c_main.c    |    2 +-
 drivers/net/atl1e/atl1e_main.c    |    2 +-
 drivers/net/atlx/atl1.c           |    2 +-
 drivers/net/cassini.c             |    2 +-
 drivers/net/e1000/e1000_main.c    |    2 +-
 drivers/net/e1000e/netdev.c       |    2 +-
 drivers/net/enic/enic_main.c      |    2 +-
 drivers/net/ixgb/ixgb_main.c      |    2 +-
 drivers/net/ll_temac_main.c       |    2 +-
 drivers/net/macvtap.c             |    3 +--
 drivers/net/myri10ge/myri10ge.c   |    2 +-
 drivers/net/niu.c                 |    2 +-
 drivers/net/skge.c                |    2 +-
 drivers/net/sungem.c              |    2 +-
 drivers/net/sunhme.c              |    2 +-
 drivers/net/tun.c                 |    2 +-
 drivers/net/usb/smsc95xx.c        |    7 +++----
 drivers/net/virtio_net.c          |    2 +-
 drivers/net/vmxnet3/vmxnet3_drv.c |    4 ++--
 include/linux/skbuff.h            |    5 +++++
 20 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/drivers/net/atl1c/atl1c_main.c b/drivers/net/atl1c/atl1c_main.c
index 09b099b..e48ea95 100644
--- a/drivers/net/atl1c/atl1c_main.c
+++ b/drivers/net/atl1c/atl1c_main.c
@@ -2078,7 +2078,7 @@ static int atl1c_tso_csum(struct atl1c_adapter *adapter,
 check_sum:
 	if (likely(skb->ip_summed == CHECKSUM_PARTIAL)) {
 		u8 css, cso;
-		cso = skb_transport_offset(skb);
+		cso = skb_checksum_start_offset(skb);
 
 		if (unlikely(cso & 0x1)) {
 			if (netif_msg_tx_err(adapter))
diff --git a/drivers/net/atl1e/atl1e_main.c b/drivers/net/atl1e/atl1e_main.c
index ef6349b..e28f8ba 100644
--- a/drivers/net/atl1e/atl1e_main.c
+++ b/drivers/net/atl1e/atl1e_main.c
@@ -1649,7 +1649,7 @@ check_sum:
 	if (likely(skb->ip_summed == CHECKSUM_PARTIAL)) {
 		u8 css, cso;
 
-		cso = skb_transport_offset(skb);
+		cso = skb_checksum_start_offset(skb);
 		if (unlikely(cso & 0x1)) {
 			netdev_err(adapter->netdev,
 				   "payload offset should not ant event number\n");
diff --git a/drivers/net/atlx/atl1.c b/drivers/net/atlx/atl1.c
index 5336310..def8df8 100644
--- a/drivers/net/atlx/atl1.c
+++ b/drivers/net/atlx/atl1.c
@@ -2174,7 +2174,7 @@ static int atl1_tx_csum(struct atl1_adapter *adapter, struct sk_buff *skb,
 	u8 css, cso;
 
 	if (likely(skb->ip_summed == CHECKSUM_PARTIAL)) {
-		css = (u8) (skb->csum_start - skb_headroom(skb));
+		css = skb_checksum_start_offset(skb);
 		cso = css + (u8) skb->csum_offset;
 		if (unlikely(css & 0x1)) {
 			/* L1 hardware requires an even number here */
diff --git a/drivers/net/cassini.c b/drivers/net/cassini.c
index d6b6d6a..35568c1 100644
--- a/drivers/net/cassini.c
+++ b/drivers/net/cassini.c
@@ -2788,7 +2788,7 @@ static inline int cas_xmit_tx_ringN(struct cas *cp, int ring,
 
 	ctrl = 0;
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
-		const u64 csum_start_off = skb_transport_offset(skb);
+		const u64 csum_start_off = skb_checksum_start_offset(skb);
 		const u64 csum_stuff_off = csum_start_off + skb->csum_offset;
 
 		ctrl =  TX_DESC_CSUM_EN |
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index dcb7f82..9426e04 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -2720,7 +2720,7 @@ static bool e1000_tx_csum(struct e1000_adapter *adapter,
 		break;
 	}
 
-	css = skb_transport_offset(skb);
+	css = skb_checksum_start_offset(skb);
 
 	i = tx_ring->next_to_use;
 	buffer_info = &tx_ring->buffer_info[i];
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 0adcb79..9bb9406 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -4473,7 +4473,7 @@ static bool e1000_tx_csum(struct e1000_adapter *adapter, struct sk_buff *skb)
 		break;
 	}
 
-	css = skb_transport_offset(skb);
+	css = skb_checksum_start_offset(skb);
 
 	i = tx_ring->next_to_use;
 	buffer_info = &tx_ring->buffer_info[i];
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 9f293fa..78e9d8b 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -702,7 +702,7 @@ static inline void enic_queue_wq_skb_csum_l4(struct enic *enic,
 {
 	unsigned int head_len = skb_headlen(skb);
 	unsigned int len_left = skb->len - head_len;
-	unsigned int hdr_len = skb_transport_offset(skb);
+	unsigned int hdr_len = skb_checksum_start_offset(skb);
 	unsigned int csum_offset = hdr_len + skb->csum_offset;
 	int eop = (len_left == 0);
 
diff --git a/drivers/net/ixgb/ixgb_main.c b/drivers/net/ixgb/ixgb_main.c
index 211a169..23ec89e 100644
--- a/drivers/net/ixgb/ixgb_main.c
+++ b/drivers/net/ixgb/ixgb_main.c
@@ -1250,7 +1250,7 @@ ixgb_tx_csum(struct ixgb_adapter *adapter, struct sk_buff *skb)
 
 	if (likely(skb->ip_summed == CHECKSUM_PARTIAL)) {
 		struct ixgb_buffer *buffer_info;
-		css = skb_transport_offset(skb);
+		css = skb_checksum_start_offset(skb);
 		cso = css + skb->csum_offset;
 
 		i = adapter->tx_ring.next_to_use;
diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c
index 9f8e702..e2c2a72 100644
--- a/drivers/net/ll_temac_main.c
+++ b/drivers/net/ll_temac_main.c
@@ -692,7 +692,7 @@ static int temac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 
 	cur_p->app0 = 0;
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
-		unsigned int csum_start_off = skb_transport_offset(skb);
+		unsigned int csum_start_off = skb_checksum_start_offset(skb);
 		unsigned int csum_index_off = csum_start_off + skb->csum_offset;
 
 		cur_p->app0 |= 1; /* TX Checksum Enabled */
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 4256727..21845af 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -504,8 +504,7 @@ static int macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
 
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
 		vnet_hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
-		vnet_hdr->csum_start = skb->csum_start -
-					skb_headroom(skb);
+		vnet_hdr->csum_start = skb_checksum_start_offset(skb);
 		vnet_hdr->csum_offset = skb->csum_offset;
 	} /* else everything is zero */
 
diff --git a/drivers/net/myri10ge/myri10ge.c b/drivers/net/myri10ge/myri10ge.c
index 8524cc4..c504e29 100644
--- a/drivers/net/myri10ge/myri10ge.c
+++ b/drivers/net/myri10ge/myri10ge.c
@@ -2736,7 +2736,7 @@ again:
 	odd_flag = 0;
 	flags = (MXGEFW_FLAGS_NO_TSO | MXGEFW_FLAGS_FIRST);
 	if (likely(skb->ip_summed == CHECKSUM_PARTIAL)) {
-		cksum_offset = skb_transport_offset(skb);
+		cksum_offset = skb_checksum_start_offset(skb);
 		pseudo_hdr_offset = cksum_offset + skb->csum_offset;
 		/* If the headers are excessively large, then we must
 		 * fall back to a software checksum */
diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index 781e368..f54d358 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -6589,7 +6589,7 @@ static u64 niu_compute_tx_flags(struct sk_buff *skb, struct ethhdr *ehdr,
 			     (ip_proto == IPPROTO_UDP ?
 			      TXHDR_CSUM_UDP : TXHDR_CSUM_SCTP));
 
-		start = skb_transport_offset(skb) -
+		start = skb_checksum_start_offset(skb) -
 			(pad_bytes + sizeof(struct tx_pkt_hdr));
 		stuff = start + skb->csum_offset;
 
diff --git a/drivers/net/skge.c b/drivers/net/skge.c
index 220e039..1959438 100644
--- a/drivers/net/skge.c
+++ b/drivers/net/skge.c
@@ -2764,7 +2764,7 @@ static netdev_tx_t skge_xmit_frame(struct sk_buff *skb,
 	td->dma_hi = map >> 32;
 
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
-		const int offset = skb_transport_offset(skb);
+		const int offset = skb_checksum_start_offset(skb);
 
 		/* This seems backwards, but it is what the sk98lin
 		 * does.  Looks like hardware is wrong?
diff --git a/drivers/net/sungem.c b/drivers/net/sungem.c
index 4ceb3cf..4842fca 100644
--- a/drivers/net/sungem.c
+++ b/drivers/net/sungem.c
@@ -1004,7 +1004,7 @@ static netdev_tx_t gem_start_xmit(struct sk_buff *skb,
 
 	ctrl = 0;
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
-		const u64 csum_start_off = skb_transport_offset(skb);
+		const u64 csum_start_off = skb_checksum_start_offset(skb);
 		const u64 csum_stuff_off = csum_start_off + skb->csum_offset;
 
 		ctrl = (TXDCTRL_CENAB |
diff --git a/drivers/net/sunhme.c b/drivers/net/sunhme.c
index 5e28c41..55bbb9c 100644
--- a/drivers/net/sunhme.c
+++ b/drivers/net/sunhme.c
@@ -2266,7 +2266,7 @@ static netdev_tx_t happy_meal_start_xmit(struct sk_buff *skb,
 
 	tx_flags = TXFLAG_OWN;
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
-		const u32 csum_start_off = skb_transport_offset(skb);
+		const u32 csum_start_off = skb_checksum_start_offset(skb);
 		const u32 csum_stuff_off = csum_start_off + skb->csum_offset;
 
 		tx_flags = (TXFLAG_OWN | TXFLAG_CSENABLE |
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 55f3a3e..7599c45 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -757,7 +757,7 @@ static __inline__ ssize_t tun_put_user(struct tun_struct *tun,
 
 		if (skb->ip_summed == CHECKSUM_PARTIAL) {
 			gso.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
-			gso.csum_start = skb->csum_start - skb_headroom(skb);
+			gso.csum_start = skb_checksum_start_offset(skb);
 			gso.csum_offset = skb->csum_offset;
 		} /* else everything is zero */
 
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 65cb1ab..bc86f4b 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1163,9 +1163,8 @@ static int smsc95xx_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 
 static u32 smsc95xx_calc_csum_preamble(struct sk_buff *skb)
 {
-	int len = skb->data - skb->head;
-	u16 high_16 = (u16)(skb->csum_offset + skb->csum_start - len);
-	u16 low_16 = (u16)(skb->csum_start - len);
+	u16 low_16 = (u16)skb_checksum_start_offset(skb);
+	u16 high_16 = low_16 + skb->csum_offset;
 	return (high_16 << 16) | low_16;
 }
 
@@ -1193,7 +1192,7 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
 		if (skb->len <= 45) {
 			/* workaround - hardware tx checksum does not work
 			 * properly with extremely small packets */
-			long csstart = skb->csum_start - skb_headroom(skb);
+			long csstart = skb_checksum_start_offset(skb);
 			__wsum calc = csum_partial(skb->data + csstart,
 				skb->len - csstart, 0);
 			*((__sum16 *)(skb->data + csstart
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index b6d4028..90a23e4 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -519,7 +519,7 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
 
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
 		hdr->hdr.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
-		hdr->hdr.csum_start = skb->csum_start - skb_headroom(skb);
+		hdr->hdr.csum_start = skb_checksum_start_offset(skb);
 		hdr->hdr.csum_offset = skb->csum_offset;
 	} else {
 		hdr->hdr.flags = 0;
diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index 65860a9..3b61e21 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -798,7 +798,7 @@ vmxnet3_parse_and_copy_hdr(struct sk_buff *skb, struct vmxnet3_tx_queue *tq,
 {
 	struct Vmxnet3_TxDataDesc *tdd;
 
-	if (ctx->mss) {
+	if (ctx->mss) {	/* GSO */
 		ctx->eth_ip_hdr_size = skb_transport_offset(skb);
 		ctx->l4_hdr_size = ((struct tcphdr *)
 				   skb_transport_header(skb))->doff * 4;
@@ -807,7 +807,7 @@ vmxnet3_parse_and_copy_hdr(struct sk_buff *skb, struct vmxnet3_tx_queue *tq,
 		unsigned int pull_size;
 
 		if (skb->ip_summed == CHECKSUM_PARTIAL) {
-			ctx->eth_ip_hdr_size = skb_transport_offset(skb);
+			ctx->eth_ip_hdr_size = skb_checksum_start_offset(skb);
 
 			if (ctx->ipv4) {
 				struct iphdr *iph = (struct iphdr *)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 19f37a6..0491da5 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1355,6 +1355,11 @@ static inline void skb_set_mac_header(struct sk_buff *skb, const int offset)
 }
 #endif /* NET_SKBUFF_DATA_USES_OFFSET */
 
+static inline int skb_checksum_start_offset(const struct sk_buff *skb)
+{
+	return skb->csum_start - skb_headroom(skb);
+}
+
 static inline int skb_transport_offset(const struct sk_buff *skb)
 {
 	return skb_transport_header(skb) - skb->data;

[PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features

[MichałMirosław]

NETIF_F_HW_CSUM is superset of NETIF_F_IP_CSUM+NETIF_F_IPV6_CSUM, but
some drivers miss the difference. Fix this and also fix UFO dependency
on checksumming offload as it makes the same mistake in assumptions.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/net/benet/be_main.c           |    6 ++++--
 drivers/net/bnx2x/bnx2x_main.c        |    4 ++--
 drivers/net/jme.c                     |   16 +++++++++-------
 drivers/net/pch_gbe/pch_gbe_ethtool.c |   19 +------------------
 drivers/net/pch_gbe/pch_gbe_main.c    |    6 +++---
 drivers/net/sc92031.c                 |    3 ++-
 drivers/net/stmmac/stmmac_ethtool.c   |   12 +-----------
 drivers/net/stmmac/stmmac_main.c      |    5 +++--
 drivers/net/vxge/vxge-ethtool.c       |    2 +-
 drivers/net/vxge/vxge-main.c          |    2 +-
 net/core/dev.c                        |    7 +++++--
 net/core/ethtool.c                    |    4 +++-
 12 files changed, 35 insertions(+), 51 deletions(-)

diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
index 102567e..2754280 100644
--- a/drivers/net/benet/be_main.c
+++ b/drivers/net/benet/be_main.c
@@ -2583,10 +2583,12 @@ static void be_netdev_init(struct net_device *netdev)
 	int i;
 
 	netdev->features |= NETIF_F_SG | NETIF_F_HW_VLAN_RX | NETIF_F_TSO |
-		NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_FILTER | NETIF_F_HW_CSUM |
+		NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_FILTER |
+		NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 		NETIF_F_GRO | NETIF_F_TSO6;
 
-	netdev->vlan_features |= NETIF_F_SG | NETIF_F_TSO | NETIF_F_HW_CSUM;
+	netdev->vlan_features |= NETIF_F_SG | NETIF_F_TSO |
+		NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 
 	if (lancer_chip(adapter))
 		netdev->vlan_features |= NETIF_F_TSO6;
diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
index f53edfd..40ce95a 100644
--- a/drivers/net/bnx2x/bnx2x_main.c
+++ b/drivers/net/bnx2x/bnx2x_main.c
@@ -8761,7 +8761,7 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
 	dev->netdev_ops = &bnx2x_netdev_ops;
 	bnx2x_set_ethtool_ops(dev);
 	dev->features |= NETIF_F_SG;
-	dev->features |= NETIF_F_HW_CSUM;
+	dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 	if (bp->flags & USING_DAC_FLAG)
 		dev->features |= NETIF_F_HIGHDMA;
 	dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
@@ -8769,7 +8769,7 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
 	dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
 
 	dev->vlan_features |= NETIF_F_SG;
-	dev->vlan_features |= NETIF_F_HW_CSUM;
+	dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 	if (bp->flags & USING_DAC_FLAG)
 		dev->vlan_features |= NETIF_F_HIGHDMA;
 	dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
diff --git a/drivers/net/jme.c b/drivers/net/jme.c
index c57d9a4..ad0935c 100644
--- a/drivers/net/jme.c
+++ b/drivers/net/jme.c
@@ -2076,12 +2076,11 @@ jme_change_mtu(struct net_device *netdev, int new_mtu)
 	}
 
 	if (new_mtu > 1900) {
-		netdev->features &= ~(NETIF_F_HW_CSUM |
-				NETIF_F_TSO |
-				NETIF_F_TSO6);
+		netdev->features &= ~(NETIF_F_IP_CSUM | NETIFI_F_IPV6_CSUM |
+				NETIF_F_TSO | NETIF_F_TSO6);
 	} else {
 		if (test_bit(JME_FLAG_TXCSUM, &jme->flags))
-			netdev->features |= NETIF_F_HW_CSUM;
+			netdev->features |= NETIF_F_IP_CSUM | NETIFI_F_IPV6_CSUM;
 		if (test_bit(JME_FLAG_TSO, &jme->flags))
 			netdev->features |= NETIF_F_TSO | NETIF_F_TSO6;
 	}
@@ -2514,10 +2513,12 @@ jme_set_tx_csum(struct net_device *netdev, u32 on)
 	if (on) {
 		set_bit(JME_FLAG_TXCSUM, &jme->flags);
 		if (netdev->mtu <= 1900)
-			netdev->features |= NETIF_F_HW_CSUM;
+			netdev->features |=
+				NETIF_F_IP_CSUM | NETIFI_F_IPV6_CSUM;
 	} else {
 		clear_bit(JME_FLAG_TXCSUM, &jme->flags);
-		netdev->features &= ~NETIF_F_HW_CSUM;
+		netdev->features &=
+				~(NETIF_F_IP_CSUM | NETIFI_F_IPV6_CSUM);
 	}
 
 	return 0;
@@ -2797,7 +2798,8 @@ jme_init_one(struct pci_dev *pdev,
 	netdev->netdev_ops = &jme_netdev_ops;
 	netdev->ethtool_ops		= &jme_ethtool_ops;
 	netdev->watchdog_timeo		= TX_TIMEOUT;
-	netdev->features		=	NETIF_F_HW_CSUM |
+	netdev->features		=	NETIF_F_IP_CSUM |
+						NETIF_F_IPV6_CSUM |
 						NETIF_F_SG |
 						NETIF_F_TSO |
 						NETIF_F_TSO6 |
diff --git a/drivers/net/pch_gbe/pch_gbe_ethtool.c b/drivers/net/pch_gbe/pch_gbe_ethtool.c
index c8cc32c..c8c873b 100644
--- a/drivers/net/pch_gbe/pch_gbe_ethtool.c
+++ b/drivers/net/pch_gbe/pch_gbe_ethtool.c
@@ -469,18 +469,6 @@ static int pch_gbe_set_rx_csum(struct net_device *netdev, u32 data)
 }
 
 /**
- * pch_gbe_get_tx_csum - Report whether transmit checksums are turned on or off
- * @netdev:  Network interface device structure
- * Returns
- *	true(1):  Checksum On
- *	false(0): Checksum Off
- */
-static u32 pch_gbe_get_tx_csum(struct net_device *netdev)
-{
-	return (netdev->features & NETIF_F_HW_CSUM) != 0;
-}
-
-/**
  * pch_gbe_set_tx_csum - Turn transmit checksums on or off
  * @netdev: Network interface device structure
  * @data:   Checksum on[true] or off[false]
@@ -493,11 +481,7 @@ static int pch_gbe_set_tx_csum(struct net_device *netdev, u32 data)
 	struct pch_gbe_adapter *adapter = netdev_priv(netdev);
 
 	adapter->tx_csum = data;
-	if (data)
-		netdev->features |= NETIF_F_HW_CSUM;
-	else
-		netdev->features &= ~NETIF_F_HW_CSUM;
-	return 0;
+	return ethtool_op_set_tx_ipv6_csum(netdev, data);
 }
 
 /**
@@ -572,7 +556,6 @@ static const struct ethtool_ops pch_gbe_ethtool_ops = {
 	.set_pauseparam = pch_gbe_set_pauseparam,
 	.get_rx_csum = pch_gbe_get_rx_csum,
 	.set_rx_csum = pch_gbe_set_rx_csum,
-	.get_tx_csum = pch_gbe_get_tx_csum,
 	.set_tx_csum = pch_gbe_set_tx_csum,
 	.get_strings = pch_gbe_get_strings,
 	.get_ethtool_stats = pch_gbe_get_ethtool_stats,
diff --git a/drivers/net/pch_gbe/pch_gbe_main.c b/drivers/net/pch_gbe/pch_gbe_main.c
index afb7506..58e7903 100644
--- a/drivers/net/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/pch_gbe/pch_gbe_main.c
@@ -2319,7 +2319,7 @@ static int pch_gbe_probe(struct pci_dev *pdev,
 	netdev->watchdog_timeo = PCH_GBE_WATCHDOG_PERIOD;
 	netif_napi_add(netdev, &adapter->napi,
 		       pch_gbe_napi_poll, PCH_GBE_RX_WEIGHT);
-	netdev->features = NETIF_F_HW_CSUM | NETIF_F_GRO;
+	netdev->features = NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | NETIF_F_GRO;
 	pch_gbe_set_ethtool_ops(netdev);
 
 	pch_gbe_mac_reset_hw(&adapter->hw);
@@ -2358,9 +2358,9 @@ static int pch_gbe_probe(struct pci_dev *pdev,
 	pch_gbe_check_options(adapter);
 
 	if (adapter->tx_csum)
-		netdev->features |= NETIF_F_HW_CSUM;
+		netdev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 	else
-		netdev->features &= ~NETIF_F_HW_CSUM;
+		netdev->features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM);
 
 	/* initialize the wol settings based on the eeprom settings */
 	adapter->wake_up_evt = PCH_GBE_WL_INIT_SETTING;
diff --git a/drivers/net/sc92031.c b/drivers/net/sc92031.c
index 417adf3..76290a8 100644
--- a/drivers/net/sc92031.c
+++ b/drivers/net/sc92031.c
@@ -1449,7 +1449,8 @@ static int __devinit sc92031_probe(struct pci_dev *pdev,
 	dev->irq = pdev->irq;
 
 	/* faked with skb_copy_and_csum_dev */
-	dev->features = NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_HIGHDMA;
+	dev->features = NETIF_F_SG | NETIF_F_HIGHDMA |
+		NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 
 	dev->netdev_ops		= &sc92031_netdev_ops;
 	dev->watchdog_timeo	= TX_TIMEOUT;
diff --git a/drivers/net/stmmac/stmmac_ethtool.c b/drivers/net/stmmac/stmmac_ethtool.c
index f2695fd..fd719ed 100644
--- a/drivers/net/stmmac/stmmac_ethtool.c
+++ b/drivers/net/stmmac/stmmac_ethtool.c
@@ -197,16 +197,6 @@ static void stmmac_ethtool_gregs(struct net_device *dev,
 	}
 }
 
-static int stmmac_ethtool_set_tx_csum(struct net_device *netdev, u32 data)
-{
-	if (data)
-		netdev->features |= NETIF_F_HW_CSUM;
-	else
-		netdev->features &= ~NETIF_F_HW_CSUM;
-
-	return 0;
-}
-
 static u32 stmmac_ethtool_get_rx_csum(struct net_device *dev)
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
@@ -370,7 +360,7 @@ static struct ethtool_ops stmmac_ethtool_ops = {
 	.get_link = ethtool_op_get_link,
 	.get_rx_csum = stmmac_ethtool_get_rx_csum,
 	.get_tx_csum = ethtool_op_get_tx_csum,
-	.set_tx_csum = stmmac_ethtool_set_tx_csum,
+	.set_tx_csum = ethtool_op_set_tx_ipv6_csum,
 	.get_sg = ethtool_op_get_sg,
 	.set_sg = ethtool_op_set_sg,
 	.get_pauseparam = stmmac_get_pauseparam,
diff --git a/drivers/net/stmmac/stmmac_main.c b/drivers/net/stmmac/stmmac_main.c
index 730a6fd..bfc2d12 100644
--- a/drivers/net/stmmac/stmmac_main.c
+++ b/drivers/net/stmmac/stmmac_main.c
@@ -1494,7 +1494,8 @@ static int stmmac_probe(struct net_device *dev)
 	dev->netdev_ops = &stmmac_netdev_ops;
 	stmmac_set_ethtool_ops(dev);
 
-	dev->features |= (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_HIGHDMA);
+	dev->features |= NETIF_F_SG | NETIF_F_HIGHDMA |
+		NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 	dev->watchdog_timeo = msecs_to_jiffies(watchdog);
 #ifdef STMMAC_VLAN_TAG_USED
 	/* Both mac100 and gmac support receive VLAN tag detection */
@@ -1525,7 +1526,7 @@ static int stmmac_probe(struct net_device *dev)
 
 	DBG(probe, DEBUG, "%s: Scatter/Gather: %s - HW checksums: %s\n",
 	    dev->name, (dev->features & NETIF_F_SG) ? "on" : "off",
-	    (dev->features & NETIF_F_HW_CSUM) ? "on" : "off");
+	    (dev->features & NETIF_F_IP_CSUM) ? "on" : "off");
 
 	spin_lock_init(&priv->lock);
 
diff --git a/drivers/net/vxge/vxge-ethtool.c b/drivers/net/vxge/vxge-ethtool.c
index bc9bd10..5ece50d 100644
--- a/drivers/net/vxge/vxge-ethtool.c
+++ b/drivers/net/vxge/vxge-ethtool.c
@@ -1177,7 +1177,7 @@ static const struct ethtool_ops vxge_ethtool_ops = {
 	.get_rx_csum		= vxge_get_rx_csum,
 	.set_rx_csum		= vxge_set_rx_csum,
 	.get_tx_csum		= ethtool_op_get_tx_csum,
-	.set_tx_csum		= ethtool_op_set_tx_hw_csum,
+	.set_tx_csum		= ethtool_op_set_tx_csum,
 	.get_sg			= ethtool_op_get_sg,
 	.set_sg			= ethtool_op_set_sg,
 	.get_tso		= ethtool_op_get_tso,
diff --git a/drivers/net/vxge/vxge-main.c b/drivers/net/vxge/vxge-main.c
index a21dae1..9f6d379 100644
--- a/drivers/net/vxge/vxge-main.c
+++ b/drivers/net/vxge/vxge-main.c
@@ -3368,7 +3368,7 @@ static int __devinit vxge_device_register(struct __vxge_hw_device *hldev,
 
 	ndev->features |= NETIF_F_SG;
 
-	ndev->features |= NETIF_F_HW_CSUM;
+	ndev->features |= NETIF_F_IP_CSUM;
 	vxge_debug_init(vxge_hw_device_trace_level_get(hldev),
 		"%s : checksuming enabled", __func__);
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 3259d2c..622f85a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5041,10 +5041,13 @@ unsigned long netdev_fix_features(unsigned long features, const char *name)
 	}
 
 	if (features & NETIF_F_UFO) {
-		if (!(features & NETIF_F_GEN_CSUM)) {
+		/* maybe split UFO into V4 and V6? */
+		if (!((features & NETIF_F_GEN_CSUM) ||
+		    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
+			    == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
 			if (name)
 				printk(KERN_ERR "%s: Dropping NETIF_F_UFO "
-				       "since no NETIF_F_HW_CSUM feature.\n",
+				       "since no checksum offload features.\n",
 				       name);
 			features &= ~NETIF_F_UFO;
 		}
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 956a9f4..d5bc2881 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1171,7 +1171,9 @@ static int ethtool_set_ufo(struct net_device *dev, char __user *useraddr)
 		return -EFAULT;
 	if (edata.data && !(dev->features & NETIF_F_SG))
 		return -EINVAL;
-	if (edata.data && !(dev->features & NETIF_F_HW_CSUM))
+	if (edata.data && !((dev->features & NETIF_F_GEN_CSUM) ||
+		(dev->features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
+			== (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM)))
 		return -EINVAL;
 	return dev->ethtool_ops->set_ufo(dev, edata.data);
 }

Re: Broken TX checksumming offloads

[MichałMirosław]

On Mon, Nov 29, 2010 at 06:35:44PM -0800, Jesse Gross wrote:
> 2010/11/29 Ben Hutchings <bhutchings@solarflare.com>:
> > On Mon, 2010-11-29 at 19:17 +0100, Michał Mirosław wrote:
> >> Hi!
> >>
> >> Unless I'm horribly mistaken, generic HW checksumming works as follows:
> >>  - driver sets netdev->features & NETIF_F_HW_CSUM to indicate support
> >>    for generic checksumming; if the flag is not set, networking core
> >>    will checksum skb before calling ndo_start_xmit (let's ignore
> >>    other checksumming options for now) and not pass skb with
> >>    skb->ip_summed == CHECKSUM_PARTIAL
> >>  - ndo_start_xmit() should use skb->csum_start and skb->csum_offset
> >>    (or skb->csum) to update checksum in software or instruct HW to do so
> >>
> >> Looking at pch_gbe_xmit_frame() and its callee - pch_gbe_tx_queue() it
> >> looks like the driver should set NETIF_F_IP_CSUM instead of NETIF_F_HW_CSUM
> >> feature.
> >>
> >> Similar thing happens in ixgbe driver: it sets NETIF_F_HW_CSUM and checks
> >> for skb->ip_summed == CHECKSUM_PARTIAL, but then just warns on protocols
> >> other that TCP and SCTP (see: ixgbe_psum()).
> >
> > AFAIK only {TCP,UDP}/IPv{4,6} use the simple 16-bit checksum algorithm
> > that NETIF_F_HW_CSUM implies, so in practice it is equivalent to
> > NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM even though it doesn't mean the same
> > thing.
> 
> There are a few cases that I can think of where the extra generality
> of NETIF_F_HW_CSUM is useful:
> 
> * The checksum in GRE (maybe other protocols as well) uses the same
> algorithm as TCP/UDP.  In theory this means that we could use hardware
> offloading for GRE on cards that have NETIF_F_HW_CSUM.  The one issue
> is that NETIF_F_IP_CSUM really means TCP/UDP checksumming but we only
> test if the protocol is IP.  This means that other IP based protocols
> (like GRE) can't use checksum offloading since there is no software
> fallback path.
> 
> * TCP/UDP packets that are deeply encapsulated.  Currently we can
> express that checksum offloading is supported in one level of vlan
> encapsulation through vlan_features.  However, a NIC that exposes
> NETIF_F_HW_CSUM should in theory be able to checksum over a packet
> that has an arbitrary number of vlan tags in front of it, a GRE
> header, or any other type of encapsulation.

So the following patches should fix what's in tree regarding
NETIF_F_HW_CSUM interpretation. The patches have no interdependencies,
so can be applied independently. The last one is a cleanup that tagged
along.

Drivers that didn't care about csum_{start,offset}:

benet
bnx2x
jme
pch_gbe
sc92031
stmmac
vxge

Best Regards,
Michał Mirosław

[PATCH] net: Move check of checksum features to netdev_fix_features()

[MichałMirosław]

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

diff --git a/net/core/dev.c b/net/core/dev.c
index 622f85a..c64a848 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5023,6 +5023,23 @@ static void rollback_registered(struct net_device *dev)
 
 unsigned long netdev_fix_features(unsigned long features, const char *name)
 {
+	/* Fix illegal checksum combinations */
+	if ((features & NETIF_F_HW_CSUM) &&
+	    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
+		if (name)
+			printk(KERN_NOTICE "%s: mixed HW and IP checksum settings.\n",
+				name);
+		features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
+	}
+
+	if ((features & NETIF_F_NO_CSUM) &&
+	    (features & (NETIF_F_HW_CSUM|NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
+		if (name)
+			printk(KERN_NOTICE "%s: mixed no checksumming and other settings.\n",
+				name);
+		features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM|NETIF_F_HW_CSUM);
+	}
+
 	/* Fix illegal SG+CSUM combinations. */
 	if ((features & NETIF_F_SG) &&
 	    !(features & NETIF_F_ALL_CSUM)) {
@@ -5206,21 +5223,6 @@ int register_netdevice(struct net_device *dev)
 	if (dev->iflink == -1)
 		dev->iflink = dev->ifindex;
 
-	/* Fix illegal checksum combinations */
-	if ((dev->features & NETIF_F_HW_CSUM) &&
-	    (dev->features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
-		printk(KERN_NOTICE "%s: mixed HW and IP checksum settings.\n",
-		       dev->name);
-		dev->features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
-	}
-
-	if ((dev->features & NETIF_F_NO_CSUM) &&
-	    (dev->features & (NETIF_F_HW_CSUM|NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
-		printk(KERN_NOTICE "%s: mixed no checksumming and other settings.\n",
-		       dev->name);
-		dev->features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM|NETIF_F_HW_CSUM);
-	}
-
 	dev->features = netdev_fix_features(dev->features, dev->name);
 
 	/* Enable software GSO if SG is supported. */

Re: [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features

[MichałMirosław]

On Tue, Nov 30, 2010 at 03:13:25PM +0000, Ben Hutchings wrote:
> On Tue, 2010-11-30 at 15:23 +0100, MichałMirosław wrote:
> > NETIF_F_HW_CSUM is superset of NETIF_F_IP_CSUM+NETIF_F_IPV6_CSUM, but
> > some drivers miss the difference. Fix this and also fix UFO dependency
> > on checksumming offload as it makes the same mistake in assumptions.
> [...]
> > diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
> > index f53edfd..40ce95a 100644
> > --- a/drivers/net/bnx2x/bnx2x_main.c
> > +++ b/drivers/net/bnx2x/bnx2x_main.c
> > @@ -8761,7 +8761,7 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
> >  	dev->netdev_ops = &bnx2x_netdev_ops;
> >  	bnx2x_set_ethtool_ops(dev);
> >  	dev->features |= NETIF_F_SG;
> > -	dev->features |= NETIF_F_HW_CSUM;
> > +	dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> >  	if (bp->flags & USING_DAC_FLAG)
> >  		dev->features |= NETIF_F_HIGHDMA;
> >  	dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> > @@ -8769,7 +8769,7 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
> >  	dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
> >  
> >  	dev->vlan_features |= NETIF_F_SG;
> > -	dev->vlan_features |= NETIF_F_HW_CSUM;
> > +	dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> >  	if (bp->flags & USING_DAC_FLAG)
> >  		dev->vlan_features |= NETIF_F_HIGHDMA;
> >  	dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> > diff --git a/drivers/net/jme.c b/drivers/net/jme.c
> > index c57d9a4..ad0935c 100644
> > --- a/drivers/net/jme.c
> > +++ b/drivers/net/jme.c
> > @@ -2076,12 +2076,11 @@ jme_change_mtu(struct net_device *netdev, int new_mtu)
> >  	}
> >  
> >  	if (new_mtu > 1900) {
> > -		netdev->features &= ~(NETIF_F_HW_CSUM |
> > -				NETIF_F_TSO |
> > -				NETIF_F_TSO6);
> > +		netdev->features &= ~(NETIF_F_IP_CSUM | NETIFI_F_IPV6_CSUM |
> > +				NETIF_F_TSO | NETIF_F_TSO6);
> >  	} else {
> >  		if (test_bit(JME_FLAG_TXCSUM, &jme->flags))
> > -			netdev->features |= NETIF_F_HW_CSUM;
> > +			netdev->features |= NETIF_F_IP_CSUM | NETIFI_F_IPV6_CSUM;
> 
> In this file you've written 'NETIFI_F_IPV6_CSUM' several times.  Do try
> compiling your work. :-)

Sorry for that. I compile tested whole series, where another patch I'm working on
just removes this part.

> [...]
> > diff --git a/drivers/net/pch_gbe/pch_gbe_ethtool.c b/drivers/net/pch_gbe/pch_gbe_ethtool.c
> > index c8cc32c..c8c873b 100644
> > --- a/drivers/net/pch_gbe/pch_gbe_ethtool.c
> > +++ b/drivers/net/pch_gbe/pch_gbe_ethtool.c
> > @@ -469,18 +469,6 @@ static int pch_gbe_set_rx_csum(struct net_device *netdev, u32 data)
> >  }
> >  
> >  /**
> > - * pch_gbe_get_tx_csum - Report whether transmit checksums are turned on or off
> > - * @netdev:  Network interface device structure
> > - * Returns
> > - *	true(1):  Checksum On
> > - *	false(0): Checksum Off
> > - */
> > -static u32 pch_gbe_get_tx_csum(struct net_device *netdev)
> > -{
> > -	return (netdev->features & NETIF_F_HW_CSUM) != 0;
> > -}
> > -
> > -/**
> >   * pch_gbe_set_tx_csum - Turn transmit checksums on or off
> >   * @netdev: Network interface device structure
> >   * @data:   Checksum on[true] or off[false]
> > @@ -493,11 +481,7 @@ static int pch_gbe_set_tx_csum(struct net_device *netdev, u32 data)
> >  	struct pch_gbe_adapter *adapter = netdev_priv(netdev);
> >  
> >  	adapter->tx_csum = data;
> > -	if (data)
> > -		netdev->features |= NETIF_F_HW_CSUM;
> > -	else
> > -		netdev->features &= ~NETIF_F_HW_CSUM;
> > -	return 0;
> > +	return ethtool_op_set_tx_ipv6_csum(netdev, data);
> >  }
> >  
> >  /**
> > @@ -572,7 +556,6 @@ static const struct ethtool_ops pch_gbe_ethtool_ops = {
> >  	.set_pauseparam = pch_gbe_set_pauseparam,
> >  	.get_rx_csum = pch_gbe_get_rx_csum,
> >  	.set_rx_csum = pch_gbe_set_rx_csum,
> > -	.get_tx_csum = pch_gbe_get_tx_csum,
> >  	.set_tx_csum = pch_gbe_set_tx_csum,
> >  	.get_strings = pch_gbe_get_strings,
> >  	.get_ethtool_stats = pch_gbe_get_ethtool_stats,
> 
> pch_gbe_get_tx_csum can simply be replaced with
> ethtool_op_set_tx_ipv6_csum.

pch_gbe_set_tx_csum() also changes adapter->tx_csum, which I didn't want
to touch in this patch. (I'm assuming you mean ...set_tx_csum not ...get_tx_csum).

> [...]
> > diff --git a/drivers/net/vxge/vxge-ethtool.c b/drivers/net/vxge/vxge-ethtool.c
> > index bc9bd10..5ece50d 100644
> > --- a/drivers/net/vxge/vxge-ethtool.c
> > +++ b/drivers/net/vxge/vxge-ethtool.c
> > @@ -1177,7 +1177,7 @@ static const struct ethtool_ops vxge_ethtool_ops = {
> >  	.get_rx_csum		= vxge_get_rx_csum,
> >  	.set_rx_csum		= vxge_set_rx_csum,
> >  	.get_tx_csum		= ethtool_op_get_tx_csum,
> > -	.set_tx_csum		= ethtool_op_set_tx_hw_csum,
> > +	.set_tx_csum		= ethtool_op_set_tx_csum,
> >  	.get_sg			= ethtool_op_get_sg,
> >  	.set_sg			= ethtool_op_set_sg,
> >  	.get_tso		= ethtool_op_get_tso,
> > diff --git a/drivers/net/vxge/vxge-main.c b/drivers/net/vxge/vxge-main.c
> > index a21dae1..9f6d379 100644
> > --- a/drivers/net/vxge/vxge-main.c
> > +++ b/drivers/net/vxge/vxge-main.c
> > @@ -3368,7 +3368,7 @@ static int __devinit vxge_device_register(struct __vxge_hw_device *hldev,
> >  
> >  	ndev->features |= NETIF_F_SG;
> >  
> > -	ndev->features |= NETIF_F_HW_CSUM;
> > +	ndev->features |= NETIF_F_IP_CSUM;
> >  	vxge_debug_init(vxge_hw_device_trace_level_get(hldev),
> >  		"%s : checksuming enabled", __func__);
> >  
> Why are you disabling IPv6 checksum offload?  From a quick look at the
> driver, I think the hardware does support it.

In vxge_xmit() (at drivers/net/vxge/vxge-main.c:922 in net-next) there
is the following code, that suggested otherwise:

        if (skb->ip_summed == CHECKSUM_PARTIAL)
                vxge_hw_fifo_txdl_cksum_set_bits(dtr,
                                        VXGE_HW_FIFO_TXD_TX_CKO_IPV4_EN |
                                        VXGE_HW_FIFO_TXD_TX_CKO_TCP_EN |
                                        VXGE_HW_FIFO_TXD_TX_CKO_UDP_EN);

> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 3259d2c..622f85a 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -5041,10 +5041,13 @@ unsigned long netdev_fix_features(unsigned long features, const char *name)
> >  	}
> >  
> >  	if (features & NETIF_F_UFO) {
> > -		if (!(features & NETIF_F_GEN_CSUM)) {
> > +		/* maybe split UFO into V4 and V6? */
> > +		if (!((features & NETIF_F_GEN_CSUM) ||
> > +		    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
> > +			    == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
> >  			if (name)
> >  				printk(KERN_ERR "%s: Dropping NETIF_F_UFO "
> > -				       "since no NETIF_F_HW_CSUM feature.\n",
> > +				       "since no checksum offload features.\n",
> >  				       name);
> >  			features &= ~NETIF_F_UFO;
> >  		}
> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > index 956a9f4..d5bc2881 100644
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> > @@ -1171,7 +1171,9 @@ static int ethtool_set_ufo(struct net_device *dev, char __user *useraddr)
> >  		return -EFAULT;
> >  	if (edata.data && !(dev->features & NETIF_F_SG))
> >  		return -EINVAL;
> > -	if (edata.data && !(dev->features & NETIF_F_HW_CSUM))
> > +	if (edata.data && !((dev->features & NETIF_F_GEN_CSUM) ||
> > +		(dev->features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
> > +			== (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM)))
> >  		return -EINVAL;
> >  	return dev->ethtool_ops->set_ufo(dev, edata.data);
> >  }
> I believe UFO is for IPv4 only; IPv6 has an entirely different kind of
> fragmentation.  So I think the check should be dev->features &
> NETIF_F_V4_CSUM.

net/ipv6/ip6_output.c references NETIF_F_UFO without checking
IPv6 checksumming features.

Best Regards,
Michał Mirosław

Re: [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features

[Michał Mirosław]

On Tue, Nov 30, 2010 at 04:28:38PM +0100, MichałMirosław wrote:
> On Tue, Nov 30, 2010 at 03:13:25PM +0000, Ben Hutchings wrote:
> > On Tue, 2010-11-30 at 15:23 +0100, MichałMirosław wrote:
> > > NETIF_F_HW_CSUM is superset of NETIF_F_IP_CSUM+NETIF_F_IPV6_CSUM, but
> > > some drivers miss the difference. Fix this and also fix UFO dependency
> > > on checksumming offload as it makes the same mistake in assumptions.
[...]
> > > diff --git a/drivers/net/vxge/vxge-ethtool.c b/drivers/net/vxge/vxge-ethtool.c
> > > index bc9bd10..5ece50d 100644
> > > --- a/drivers/net/vxge/vxge-ethtool.c
> > > +++ b/drivers/net/vxge/vxge-ethtool.c
> > > @@ -1177,7 +1177,7 @@ static const struct ethtool_ops vxge_ethtool_ops = {
> > >  	.get_rx_csum		= vxge_get_rx_csum,
> > >  	.set_rx_csum		= vxge_set_rx_csum,
> > >  	.get_tx_csum		= ethtool_op_get_tx_csum,
> > > -	.set_tx_csum		= ethtool_op_set_tx_hw_csum,
> > > +	.set_tx_csum		= ethtool_op_set_tx_csum,
> > >  	.get_sg			= ethtool_op_get_sg,
> > >  	.set_sg			= ethtool_op_set_sg,
> > >  	.get_tso		= ethtool_op_get_tso,
> > > diff --git a/drivers/net/vxge/vxge-main.c b/drivers/net/vxge/vxge-main.c
> > > index a21dae1..9f6d379 100644
> > > --- a/drivers/net/vxge/vxge-main.c
> > > +++ b/drivers/net/vxge/vxge-main.c
> > > @@ -3368,7 +3368,7 @@ static int __devinit vxge_device_register(struct __vxge_hw_device *hldev,
> > >  
> > >  	ndev->features |= NETIF_F_SG;
> > >  
> > > -	ndev->features |= NETIF_F_HW_CSUM;
> > > +	ndev->features |= NETIF_F_IP_CSUM;
> > >  	vxge_debug_init(vxge_hw_device_trace_level_get(hldev),
> > >  		"%s : checksuming enabled", __func__);
> > >  
> > Why are you disabling IPv6 checksum offload?  From a quick look at the
> > driver, I think the hardware does support it.
> 
> In vxge_xmit() (at drivers/net/vxge/vxge-main.c:922 in net-next) there
> is the following code, that suggested otherwise:
> 
>         if (skb->ip_summed == CHECKSUM_PARTIAL)
>                 vxge_hw_fifo_txdl_cksum_set_bits(dtr,
>                                         VXGE_HW_FIFO_TXD_TX_CKO_IPV4_EN |
>                                         VXGE_HW_FIFO_TXD_TX_CKO_TCP_EN |
>                                         VXGE_HW_FIFO_TXD_TX_CKO_UDP_EN);
> 

Lets ask vxge driver maintainters on this.

Does vxge support IPv6 TCP/UDP checksumming offload?

Best Regards,
Michał Mirosław

Re: [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features

[Ben Hutchings]

On Tue, 2010-11-30 at 16:28 +0100, MichałMirosław wrote:
[...]
> > > -/**
> > >   * pch_gbe_set_tx_csum - Turn transmit checksums on or off
> > >   * @netdev: Network interface device structure
> > >   * @data:   Checksum on[true] or off[false]
> > > @@ -493,11 +481,7 @@ static int pch_gbe_set_tx_csum(struct net_device *netdev, u32 data)
> > >  	struct pch_gbe_adapter *adapter = netdev_priv(netdev);
> > >  
> > >  	adapter->tx_csum = data;
> > > -	if (data)
> > > -		netdev->features |= NETIF_F_HW_CSUM;
> > > -	else
> > > -		netdev->features &= ~NETIF_F_HW_CSUM;
> > > -	return 0;
> > > +	return ethtool_op_set_tx_ipv6_csum(netdev, data);
> > >  }
> > >  
> > >  /**
> > > @@ -572,7 +556,6 @@ static const struct ethtool_ops pch_gbe_ethtool_ops = {
> > >  	.set_pauseparam = pch_gbe_set_pauseparam,
> > >  	.get_rx_csum = pch_gbe_get_rx_csum,
> > >  	.set_rx_csum = pch_gbe_set_rx_csum,
> > > -	.get_tx_csum = pch_gbe_get_tx_csum,
> > >  	.set_tx_csum = pch_gbe_set_tx_csum,
> > >  	.get_strings = pch_gbe_get_strings,
> > >  	.get_ethtool_stats = pch_gbe_get_ethtool_stats,
> > 
> > pch_gbe_get_tx_csum can simply be replaced with
> > ethtool_op_set_tx_ipv6_csum.
> 
> pch_gbe_set_tx_csum() also changes adapter->tx_csum, which I didn't want
> to touch in this patch. (I'm assuming you mean ...set_tx_csum not ...get_tx_csum).

Sorry, I missed that.

[...]
> > Why are you disabling IPv6 checksum offload?  From a quick look at the
> > driver, I think the hardware does support it.
> 
> In vxge_xmit() (at drivers/net/vxge/vxge-main.c:922 in net-next) there
> is the following code, that suggested otherwise:
> 
>         if (skb->ip_summed == CHECKSUM_PARTIAL)
>                 vxge_hw_fifo_txdl_cksum_set_bits(dtr,
>                                         VXGE_HW_FIFO_TXD_TX_CKO_IPV4_EN |
>                                         VXGE_HW_FIFO_TXD_TX_CKO_TCP_EN |
>                                         VXGE_HW_FIFO_TXD_TX_CKO_UDP_EN);

I bet IPV4_EN refers to the IPv4 header checksum.  Since IPv6 doesn't
have a header checksum, there won't be a flag for it here.

> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 3259d2c..622f85a 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -5041,10 +5041,13 @@ unsigned long netdev_fix_features(unsigned long features, const char *name)
> > >  	}
> > >  
> > >  	if (features & NETIF_F_UFO) {
> > > -		if (!(features & NETIF_F_GEN_CSUM)) {
> > > +		/* maybe split UFO into V4 and V6? */
> > > +		if (!((features & NETIF_F_GEN_CSUM) ||
> > > +		    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
> > > +			    == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
> > >  			if (name)
> > >  				printk(KERN_ERR "%s: Dropping NETIF_F_UFO "
> > > -				       "since no NETIF_F_HW_CSUM feature.\n",
> > > +				       "since no checksum offload features.\n",
> > >  				       name);
> > >  			features &= ~NETIF_F_UFO;
> > >  		}
> > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > > index 956a9f4..d5bc2881 100644
> > > --- a/net/core/ethtool.c
> > > +++ b/net/core/ethtool.c
> > > @@ -1171,7 +1171,9 @@ static int ethtool_set_ufo(struct net_device *dev, char __user *useraddr)
> > >  		return -EFAULT;
> > >  	if (edata.data && !(dev->features & NETIF_F_SG))
> > >  		return -EINVAL;
> > > -	if (edata.data && !(dev->features & NETIF_F_HW_CSUM))
> > > +	if (edata.data && !((dev->features & NETIF_F_GEN_CSUM) ||
> > > +		(dev->features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
> > > +			== (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM)))
> > >  		return -EINVAL;
> > >  	return dev->ethtool_ops->set_ufo(dev, edata.data);
> > >  }
> > I believe UFO is for IPv4 only; IPv6 has an entirely different kind of
> > fragmentation.  So I think the check should be dev->features &
> > NETIF_F_V4_CSUM.
> 
> net/ipv6/ip6_output.c references NETIF_F_UFO without checking
> IPv6 checksumming features.

Got it.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features

[Jon Mason]

On Tue, Nov 30, 2010 at 07:41:23AM -0800, Michał Mirosław wrote:
> On Tue, Nov 30, 2010 at 04:28:38PM +0100, MichałMirosław wrote:
> > On Tue, Nov 30, 2010 at 03:13:25PM +0000, Ben Hutchings wrote:
> > > On Tue, 2010-11-30 at 15:23 +0100, MichałMirosław wrote:
> > > > NETIF_F_HW_CSUM is superset of NETIF_F_IP_CSUM+NETIF_F_IPV6_CSUM, but
> > > > some drivers miss the difference. Fix this and also fix UFO dependency
> > > > on checksumming offload as it makes the same mistake in assumptions.
> [...]
> > > > diff --git a/drivers/net/vxge/vxge-ethtool.c b/drivers/net/vxge/vxge-ethtool.c
> > > > index bc9bd10..5ece50d 100644
> > > > --- a/drivers/net/vxge/vxge-ethtool.c
> > > > +++ b/drivers/net/vxge/vxge-ethtool.c
> > > > @@ -1177,7 +1177,7 @@ static const struct ethtool_ops vxge_ethtool_ops = {
> > > >  	.get_rx_csum		= vxge_get_rx_csum,
> > > >  	.set_rx_csum		= vxge_set_rx_csum,
> > > >  	.get_tx_csum		= ethtool_op_get_tx_csum,
> > > > -	.set_tx_csum		= ethtool_op_set_tx_hw_csum,
> > > > +	.set_tx_csum		= ethtool_op_set_tx_csum,
> > > >  	.get_sg			= ethtool_op_get_sg,
> > > >  	.set_sg			= ethtool_op_set_sg,
> > > >  	.get_tso		= ethtool_op_get_tso,
> > > > diff --git a/drivers/net/vxge/vxge-main.c b/drivers/net/vxge/vxge-main.c
> > > > index a21dae1..9f6d379 100644
> > > > --- a/drivers/net/vxge/vxge-main.c
> > > > +++ b/drivers/net/vxge/vxge-main.c
> > > > @@ -3368,7 +3368,7 @@ static int __devinit vxge_device_register(struct __vxge_hw_device *hldev,
> > > >  
> > > >  	ndev->features |= NETIF_F_SG;
> > > >  
> > > > -	ndev->features |= NETIF_F_HW_CSUM;
> > > > +	ndev->features |= NETIF_F_IP_CSUM;
> > > >  	vxge_debug_init(vxge_hw_device_trace_level_get(hldev),
> > > >  		"%s : checksuming enabled", __func__);
> > > >  
> > > Why are you disabling IPv6 checksum offload?  From a quick look at the
> > > driver, I think the hardware does support it.
> > 
> > In vxge_xmit() (at drivers/net/vxge/vxge-main.c:922 in net-next) there
> > is the following code, that suggested otherwise:
> > 
> >         if (skb->ip_summed == CHECKSUM_PARTIAL)
> >                 vxge_hw_fifo_txdl_cksum_set_bits(dtr,
> >                                         VXGE_HW_FIFO_TXD_TX_CKO_IPV4_EN |
> >                                         VXGE_HW_FIFO_TXD_TX_CKO_TCP_EN |
> >                                         VXGE_HW_FIFO_TXD_TX_CKO_UDP_EN);
> > 
> 
> Lets ask vxge driver maintainters on this.
> 
> Does vxge support IPv6 TCP/UDP checksumming offload?

Yes, the underlying hardware can handle checksumming TCP/UDP in an
IPv6 frame.  So the change in the former code above would revert this.
The latter piece of code is for inserting the IPv4 checksum.

Thanks,
Jon

> 
> Best Regards,
> Michał Mirosław

Re: [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features

[MichałMirosław]

On Tue, Nov 30, 2010 at 03:51:58PM +0000, Ben Hutchings wrote:
> > > Why are you disabling IPv6 checksum offload?  From a quick look at the
> > > driver, I think the hardware does support it.
> > 
> > In vxge_xmit() (at drivers/net/vxge/vxge-main.c:922 in net-next) there
> > is the following code, that suggested otherwise:
> > 
> >         if (skb->ip_summed == CHECKSUM_PARTIAL)
> >                 vxge_hw_fifo_txdl_cksum_set_bits(dtr,
> >                                         VXGE_HW_FIFO_TXD_TX_CKO_IPV4_EN |
> >                                         VXGE_HW_FIFO_TXD_TX_CKO_TCP_EN |
> >                                         VXGE_HW_FIFO_TXD_TX_CKO_UDP_EN);
> 
> I bet IPV4_EN refers to the IPv4 header checksum.  Since IPv6 doesn't
> have a header checksum, there won't be a flag for it here.

That makes sense. I'll fix that.

Thanks,
Michał Mirosław

[PATCH v2] net: Fix too optimistic NETIF_F_HW_CSUM features

[MichałMirosław]

NETIF_F_HW_CSUM is a superset of NETIF_F_IP_CSUM+NETIF_F_IPV6_CSUM, but
some drivers miss the difference. Fix this and also fix UFO dependency
on checksumming offload as it makes the same mistake in assumptions.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---

Changes from v1:
- fixed compilation of jme driver
- enable vxge support for IPv6 checksum
---
 drivers/net/vxge/vxge-ethtool.c |    2 +-
 drivers/net/vxge/vxge-main.c    |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
index 102567e..2754280 100644
--- a/drivers/net/benet/be_main.c
+++ b/drivers/net/benet/be_main.c
@@ -2583,10 +2583,12 @@ static void be_netdev_init(struct net_device *netdev)
 	int i;
 
 	netdev->features |= NETIF_F_SG | NETIF_F_HW_VLAN_RX | NETIF_F_TSO |
-		NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_FILTER | NETIF_F_HW_CSUM |
+		NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_FILTER |
+		NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 		NETIF_F_GRO | NETIF_F_TSO6;
 
-	netdev->vlan_features |= NETIF_F_SG | NETIF_F_TSO | NETIF_F_HW_CSUM;
+	netdev->vlan_features |= NETIF_F_SG | NETIF_F_TSO |
+		NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 
 	if (lancer_chip(adapter))
 		netdev->vlan_features |= NETIF_F_TSO6;
diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
index f53edfd..40ce95a 100644
--- a/drivers/net/bnx2x/bnx2x_main.c
+++ b/drivers/net/bnx2x/bnx2x_main.c
@@ -8761,7 +8761,7 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
 	dev->netdev_ops = &bnx2x_netdev_ops;
 	bnx2x_set_ethtool_ops(dev);
 	dev->features |= NETIF_F_SG;
-	dev->features |= NETIF_F_HW_CSUM;
+	dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 	if (bp->flags & USING_DAC_FLAG)
 		dev->features |= NETIF_F_HIGHDMA;
 	dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
@@ -8769,7 +8769,7 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
 	dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
 
 	dev->vlan_features |= NETIF_F_SG;
-	dev->vlan_features |= NETIF_F_HW_CSUM;
+	dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 	if (bp->flags & USING_DAC_FLAG)
 		dev->vlan_features |= NETIF_F_HIGHDMA;
 	dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
diff --git a/drivers/net/jme.c b/drivers/net/jme.c
index c57d9a4..2411e72 100644
--- a/drivers/net/jme.c
+++ b/drivers/net/jme.c
@@ -2076,12 +2076,11 @@ jme_change_mtu(struct net_device *netdev, int new_mtu)
 	}
 
 	if (new_mtu > 1900) {
-		netdev->features &= ~(NETIF_F_HW_CSUM |
-				NETIF_F_TSO |
-				NETIF_F_TSO6);
+		netdev->features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+				NETIF_F_TSO | NETIF_F_TSO6);
 	} else {
 		if (test_bit(JME_FLAG_TXCSUM, &jme->flags))
-			netdev->features |= NETIF_F_HW_CSUM;
+			netdev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 		if (test_bit(JME_FLAG_TSO, &jme->flags))
 			netdev->features |= NETIF_F_TSO | NETIF_F_TSO6;
 	}
@@ -2514,10 +2513,12 @@ jme_set_tx_csum(struct net_device *netdev, u32 on)
 	if (on) {
 		set_bit(JME_FLAG_TXCSUM, &jme->flags);
 		if (netdev->mtu <= 1900)
-			netdev->features |= NETIF_F_HW_CSUM;
+			netdev->features |=
+				NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 	} else {
 		clear_bit(JME_FLAG_TXCSUM, &jme->flags);
-		netdev->features &= ~NETIF_F_HW_CSUM;
+		netdev->features &=
+				~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM);
 	}
 
 	return 0;
@@ -2797,7 +2798,8 @@ jme_init_one(struct pci_dev *pdev,
 	netdev->netdev_ops = &jme_netdev_ops;
 	netdev->ethtool_ops		= &jme_ethtool_ops;
 	netdev->watchdog_timeo		= TX_TIMEOUT;
-	netdev->features		=	NETIF_F_HW_CSUM |
+	netdev->features		=	NETIF_F_IP_CSUM |
+						NETIF_F_IPV6_CSUM |
 						NETIF_F_SG |
 						NETIF_F_TSO |
 						NETIF_F_TSO6 |
diff --git a/drivers/net/pch_gbe/pch_gbe_ethtool.c b/drivers/net/pch_gbe/pch_gbe_ethtool.c
index c8cc32c..c8c873b 100644
--- a/drivers/net/pch_gbe/pch_gbe_ethtool.c
+++ b/drivers/net/pch_gbe/pch_gbe_ethtool.c
@@ -469,18 +469,6 @@ static int pch_gbe_set_rx_csum(struct net_device *netdev, u32 data)
 }
 
 /**
- * pch_gbe_get_tx_csum - Report whether transmit checksums are turned on or off
- * @netdev:  Network interface device structure
- * Returns
- *	true(1):  Checksum On
- *	false(0): Checksum Off
- */
-static u32 pch_gbe_get_tx_csum(struct net_device *netdev)
-{
-	return (netdev->features & NETIF_F_HW_CSUM) != 0;
-}
-
-/**
  * pch_gbe_set_tx_csum - Turn transmit checksums on or off
  * @netdev: Network interface device structure
  * @data:   Checksum on[true] or off[false]
@@ -493,11 +481,7 @@ static int pch_gbe_set_tx_csum(struct net_device *netdev, u32 data)
 	struct pch_gbe_adapter *adapter = netdev_priv(netdev);
 
 	adapter->tx_csum = data;
-	if (data)
-		netdev->features |= NETIF_F_HW_CSUM;
-	else
-		netdev->features &= ~NETIF_F_HW_CSUM;
-	return 0;
+	return ethtool_op_set_tx_ipv6_csum(netdev, data);
 }
 
 /**
@@ -572,7 +556,6 @@ static const struct ethtool_ops pch_gbe_ethtool_ops = {
 	.set_pauseparam = pch_gbe_set_pauseparam,
 	.get_rx_csum = pch_gbe_get_rx_csum,
 	.set_rx_csum = pch_gbe_set_rx_csum,
-	.get_tx_csum = pch_gbe_get_tx_csum,
 	.set_tx_csum = pch_gbe_set_tx_csum,
 	.get_strings = pch_gbe_get_strings,
 	.get_ethtool_stats = pch_gbe_get_ethtool_stats,
diff --git a/drivers/net/pch_gbe/pch_gbe_main.c b/drivers/net/pch_gbe/pch_gbe_main.c
index afb7506..58e7903 100644
--- a/drivers/net/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/pch_gbe/pch_gbe_main.c
@@ -2319,7 +2319,7 @@ static int pch_gbe_probe(struct pci_dev *pdev,
 	netdev->watchdog_timeo = PCH_GBE_WATCHDOG_PERIOD;
 	netif_napi_add(netdev, &adapter->napi,
 		       pch_gbe_napi_poll, PCH_GBE_RX_WEIGHT);
-	netdev->features = NETIF_F_HW_CSUM | NETIF_F_GRO;
+	netdev->features = NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | NETIF_F_GRO;
 	pch_gbe_set_ethtool_ops(netdev);
 
 	pch_gbe_mac_reset_hw(&adapter->hw);
@@ -2358,9 +2358,9 @@ static int pch_gbe_probe(struct pci_dev *pdev,
 	pch_gbe_check_options(adapter);
 
 	if (adapter->tx_csum)
-		netdev->features |= NETIF_F_HW_CSUM;
+		netdev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 	else
-		netdev->features &= ~NETIF_F_HW_CSUM;
+		netdev->features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM);
 
 	/* initialize the wol settings based on the eeprom settings */
 	adapter->wake_up_evt = PCH_GBE_WL_INIT_SETTING;
diff --git a/drivers/net/sc92031.c b/drivers/net/sc92031.c
index 417adf3..76290a8 100644
--- a/drivers/net/sc92031.c
+++ b/drivers/net/sc92031.c
@@ -1449,7 +1449,8 @@ static int __devinit sc92031_probe(struct pci_dev *pdev,
 	dev->irq = pdev->irq;
 
 	/* faked with skb_copy_and_csum_dev */
-	dev->features = NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_HIGHDMA;
+	dev->features = NETIF_F_SG | NETIF_F_HIGHDMA |
+		NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 
 	dev->netdev_ops		= &sc92031_netdev_ops;
 	dev->watchdog_timeo	= TX_TIMEOUT;
diff --git a/drivers/net/stmmac/stmmac_ethtool.c b/drivers/net/stmmac/stmmac_ethtool.c
index f2695fd..fd719ed 100644
--- a/drivers/net/stmmac/stmmac_ethtool.c
+++ b/drivers/net/stmmac/stmmac_ethtool.c
@@ -197,16 +197,6 @@ static void stmmac_ethtool_gregs(struct net_device *dev,
 	}
 }
 
-static int stmmac_ethtool_set_tx_csum(struct net_device *netdev, u32 data)
-{
-	if (data)
-		netdev->features |= NETIF_F_HW_CSUM;
-	else
-		netdev->features &= ~NETIF_F_HW_CSUM;
-
-	return 0;
-}
-
 static u32 stmmac_ethtool_get_rx_csum(struct net_device *dev)
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
@@ -370,7 +360,7 @@ static struct ethtool_ops stmmac_ethtool_ops = {
 	.get_link = ethtool_op_get_link,
 	.get_rx_csum = stmmac_ethtool_get_rx_csum,
 	.get_tx_csum = ethtool_op_get_tx_csum,
-	.set_tx_csum = stmmac_ethtool_set_tx_csum,
+	.set_tx_csum = ethtool_op_set_tx_ipv6_csum,
 	.get_sg = ethtool_op_get_sg,
 	.set_sg = ethtool_op_set_sg,
 	.get_pauseparam = stmmac_get_pauseparam,
diff --git a/drivers/net/stmmac/stmmac_main.c b/drivers/net/stmmac/stmmac_main.c
index 730a6fd..bfc2d12 100644
--- a/drivers/net/stmmac/stmmac_main.c
+++ b/drivers/net/stmmac/stmmac_main.c
@@ -1494,7 +1494,8 @@ static int stmmac_probe(struct net_device *dev)
 	dev->netdev_ops = &stmmac_netdev_ops;
 	stmmac_set_ethtool_ops(dev);
 
-	dev->features |= (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_HIGHDMA);
+	dev->features |= NETIF_F_SG | NETIF_F_HIGHDMA |
+		NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 	dev->watchdog_timeo = msecs_to_jiffies(watchdog);
 #ifdef STMMAC_VLAN_TAG_USED
 	/* Both mac100 and gmac support receive VLAN tag detection */
@@ -1525,7 +1526,7 @@ static int stmmac_probe(struct net_device *dev)
 
 	DBG(probe, DEBUG, "%s: Scatter/Gather: %s - HW checksums: %s\n",
 	    dev->name, (dev->features & NETIF_F_SG) ? "on" : "off",
-	    (dev->features & NETIF_F_HW_CSUM) ? "on" : "off");
+	    (dev->features & NETIF_F_IP_CSUM) ? "on" : "off");
 
 	spin_lock_init(&priv->lock);
 
diff --git a/drivers/net/vxge/vxge-ethtool.c b/drivers/net/vxge/vxge-ethtool.c
index bc9bd10..1dd3a21 100644
--- a/drivers/net/vxge/vxge-ethtool.c
+++ b/drivers/net/vxge/vxge-ethtool.c
@@ -1177,7 +1177,7 @@ static const struct ethtool_ops vxge_ethtool_ops = {
 	.get_rx_csum		= vxge_get_rx_csum,
 	.set_rx_csum		= vxge_set_rx_csum,
 	.get_tx_csum		= ethtool_op_get_tx_csum,
-	.set_tx_csum		= ethtool_op_set_tx_hw_csum,
+	.set_tx_csum		= ethtool_op_set_tx_ipv6_csum,
 	.get_sg			= ethtool_op_get_sg,
 	.set_sg			= ethtool_op_set_sg,
 	.get_tso		= ethtool_op_get_tso,
diff --git a/drivers/net/vxge/vxge-main.c b/drivers/net/vxge/vxge-main.c
index a21dae1..d339f5b 100644
--- a/drivers/net/vxge/vxge-main.c
+++ b/drivers/net/vxge/vxge-main.c
@@ -3368,7 +3368,7 @@ static int __devinit vxge_device_register(struct __vxge_hw_device *hldev,
 
 	ndev->features |= NETIF_F_SG;
 
-	ndev->features |= NETIF_F_HW_CSUM;
+	ndev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 	vxge_debug_init(vxge_hw_device_trace_level_get(hldev),
 		"%s : checksuming enabled", __func__);
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 3259d2c..622f85a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5041,10 +5041,13 @@ unsigned long netdev_fix_features(unsigned long features, const char *name)
 	}
 
 	if (features & NETIF_F_UFO) {
-		if (!(features & NETIF_F_GEN_CSUM)) {
+		/* maybe split UFO into V4 and V6? */
+		if (!((features & NETIF_F_GEN_CSUM) ||
+		    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
+			    == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
 			if (name)
 				printk(KERN_ERR "%s: Dropping NETIF_F_UFO "
-				       "since no NETIF_F_HW_CSUM feature.\n",
+				       "since no checksum offload features.\n",
 				       name);
 			features &= ~NETIF_F_UFO;
 		}
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 956a9f4..d5bc2881 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1171,7 +1171,9 @@ static int ethtool_set_ufo(struct net_device *dev, char __user *useraddr)
 		return -EFAULT;
 	if (edata.data && !(dev->features & NETIF_F_SG))
 		return -EINVAL;
-	if (edata.data && !(dev->features & NETIF_F_HW_CSUM))
+	if (edata.data && !((dev->features & NETIF_F_GEN_CSUM) ||
+		(dev->features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
+			== (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM)))
 		return -EINVAL;
 	return dev->ethtool_ops->set_ufo(dev, edata.data);
 }

Re: [PATCH v2] net: Fix too optimistic NETIF_F_HW_CSUM features

[Eric Dumazet]

Le mardi 30 novembre 2010 à 17:38 +0100, MichałMirosław a écrit :
> NETIF_F_HW_CSUM is a superset of NETIF_F_IP_CSUM+NETIF_F_IPV6_CSUM, but
> some drivers miss the difference. Fix this and also fix UFO dependency
> on checksumming offload as it makes the same mistake in assumptions.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> 
> Changes from v1:
> - fixed compilation of jme driver
> - enable vxge support for IPv6 checksum
> ---
>  drivers/net/vxge/vxge-ethtool.c |    2 +-
>  drivers/net/vxge/vxge-main.c    |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

I have no idea how you got this diffstat.

Your patch touches more files than that...

Re: [PATCH v2] net: Fix too optimistic NETIF_F_HW_CSUM features

[MichałMirosław]

On Tue, Nov 30, 2010 at 05:53:46PM +0100, Eric Dumazet wrote:
> Le mardi 30 novembre 2010 à 17:38 +0100, MichałMirosław a écrit :
> > NETIF_F_HW_CSUM is a superset of NETIF_F_IP_CSUM+NETIF_F_IPV6_CSUM, but
> > some drivers miss the difference. Fix this and also fix UFO dependency
> > on checksumming offload as it makes the same mistake in assumptions.
> > 
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > ---
> > 
> > Changes from v1:
> > - fixed compilation of jme driver
> > - enable vxge support for IPv6 checksum
> > ---
> >  drivers/net/vxge/vxge-ethtool.c |    2 +-
> >  drivers/net/vxge/vxge-main.c    |    2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> I have no idea how you got this diffstat.
> Your patch touches more files than that...

Hmm. Looks like a bug in StGit.

stg refresh didn't fix this. stg repair didn't either. :/

Best Regards,
Michał Mirosław

Re: [PATCH v2] net: Fix too optimistic NETIF_F_HW_CSUM features

[MichałMirosław]

On Tue, Nov 30, 2010 at 06:07:55PM +0100, MichałMirosław wrote:
> On Tue, Nov 30, 2010 at 05:53:46PM +0100, Eric Dumazet wrote:
> > Le mardi 30 novembre 2010 à 17:38 +0100, MichałMirosław a écrit :
> > > NETIF_F_HW_CSUM is a superset of NETIF_F_IP_CSUM+NETIF_F_IPV6_CSUM, but
> > > some drivers miss the difference. Fix this and also fix UFO dependency
> > > on checksumming offload as it makes the same mistake in assumptions.
> > > 
> > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > ---
> > > 
> > > Changes from v1:
> > > - fixed compilation of jme driver
> > > - enable vxge support for IPv6 checksum
> > > ---
> > >  drivers/net/vxge/vxge-ethtool.c |    2 +-
> > >  drivers/net/vxge/vxge-main.c    |    2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > I have no idea how you got this diffstat.
> > Your patch touches more files than that...
> Hmm. Looks like a bug in StGit.
> stg refresh didn't fix this. stg repair didn't either. :/

In case someone is interested: stg rebase fixes this.

Should I resend the patch?

Best Regards,
Michał Mirosław

Re: [PATCH v2] net: Fix too optimistic NETIF_F_HW_CSUM features

[Eric Dumazet]

Le mardi 30 novembre 2010 à 18:15 +0100, MichałMirosław a écrit :

> In case someone is interested: stg rebase fixes this.
> 
> Should I resend the patch?

I guess not ;)

Re: [PATCH v2] net: Fix too optimistic NETIF_F_HW_CSUM features

[Jon Mason]

On Tue, Nov 30, 2010 at 08:38:00AM -0800, MichałMirosław wrote:
> NETIF_F_HW_CSUM is a superset of NETIF_F_IP_CSUM+NETIF_F_IPV6_CSUM, but
> some drivers miss the difference. Fix this and also fix UFO dependency
> on checksumming offload as it makes the same mistake in assumptions.
>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

The vxge sections look sane to me.

Acked-by: Jon Mason <jon.mason@exar.com>
> ---
>
> Changes from v1:
> - fixed compilation of jme driver
> - enable vxge support for IPv6 checksum
> ---
>  drivers/net/vxge/vxge-ethtool.c |    2 +-
>  drivers/net/vxge/vxge-main.c    |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
> index 102567e..2754280 100644
> --- a/drivers/net/benet/be_main.c
> +++ b/drivers/net/benet/be_main.c
> @@ -2583,10 +2583,12 @@ static void be_netdev_init(struct net_device *netdev)
>       int i;
>
>       netdev->features |= NETIF_F_SG | NETIF_F_HW_VLAN_RX | NETIF_F_TSO |
> -             NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_FILTER | NETIF_F_HW_CSUM |
> +             NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_FILTER |
> +             NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>               NETIF_F_GRO | NETIF_F_TSO6;
>
> -     netdev->vlan_features |= NETIF_F_SG | NETIF_F_TSO | NETIF_F_HW_CSUM;
> +     netdev->vlan_features |= NETIF_F_SG | NETIF_F_TSO |
> +             NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
>
>       if (lancer_chip(adapter))
>               netdev->vlan_features |= NETIF_F_TSO6;
> diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
> index f53edfd..40ce95a 100644
> --- a/drivers/net/bnx2x/bnx2x_main.c
> +++ b/drivers/net/bnx2x/bnx2x_main.c
> @@ -8761,7 +8761,7 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
>       dev->netdev_ops = &bnx2x_netdev_ops;
>       bnx2x_set_ethtool_ops(dev);
>       dev->features |= NETIF_F_SG;
> -     dev->features |= NETIF_F_HW_CSUM;
> +     dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
>       if (bp->flags & USING_DAC_FLAG)
>               dev->features |= NETIF_F_HIGHDMA;
>       dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> @@ -8769,7 +8769,7 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
>       dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
>
>       dev->vlan_features |= NETIF_F_SG;
> -     dev->vlan_features |= NETIF_F_HW_CSUM;
> +     dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
>       if (bp->flags & USING_DAC_FLAG)
>               dev->vlan_features |= NETIF_F_HIGHDMA;
>       dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> diff --git a/drivers/net/jme.c b/drivers/net/jme.c
> index c57d9a4..2411e72 100644
> --- a/drivers/net/jme.c
> +++ b/drivers/net/jme.c
> @@ -2076,12 +2076,11 @@ jme_change_mtu(struct net_device *netdev, int new_mtu)
>       }
>
>       if (new_mtu > 1900) {
> -             netdev->features &= ~(NETIF_F_HW_CSUM |
> -                             NETIF_F_TSO |
> -                             NETIF_F_TSO6);
> +             netdev->features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> +                             NETIF_F_TSO | NETIF_F_TSO6);
>       } else {
>               if (test_bit(JME_FLAG_TXCSUM, &jme->flags))
> -                     netdev->features |= NETIF_F_HW_CSUM;
> +                     netdev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
>               if (test_bit(JME_FLAG_TSO, &jme->flags))
>                       netdev->features |= NETIF_F_TSO | NETIF_F_TSO6;
>       }
> @@ -2514,10 +2513,12 @@ jme_set_tx_csum(struct net_device *netdev, u32 on)
>       if (on) {
>               set_bit(JME_FLAG_TXCSUM, &jme->flags);
>               if (netdev->mtu <= 1900)
> -                     netdev->features |= NETIF_F_HW_CSUM;
> +                     netdev->features |=
> +                             NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
>       } else {
>               clear_bit(JME_FLAG_TXCSUM, &jme->flags);
> -             netdev->features &= ~NETIF_F_HW_CSUM;
> +             netdev->features &=
> +                             ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM);
>       }
>
>       return 0;
> @@ -2797,7 +2798,8 @@ jme_init_one(struct pci_dev *pdev,
>       netdev->netdev_ops = &jme_netdev_ops;
>       netdev->ethtool_ops             = &jme_ethtool_ops;
>       netdev->watchdog_timeo          = TX_TIMEOUT;
> -     netdev->features                =       NETIF_F_HW_CSUM |
> +     netdev->features                =       NETIF_F_IP_CSUM |
> +                                             NETIF_F_IPV6_CSUM |
>                                               NETIF_F_SG |
>                                               NETIF_F_TSO |
>                                               NETIF_F_TSO6 |
> diff --git a/drivers/net/pch_gbe/pch_gbe_ethtool.c b/drivers/net/pch_gbe/pch_gbe_ethtool.c
> index c8cc32c..c8c873b 100644
> --- a/drivers/net/pch_gbe/pch_gbe_ethtool.c
> +++ b/drivers/net/pch_gbe/pch_gbe_ethtool.c
> @@ -469,18 +469,6 @@ static int pch_gbe_set_rx_csum(struct net_device *netdev, u32 data)
>  }
>
>  /**
> - * pch_gbe_get_tx_csum - Report whether transmit checksums are turned on or off
> - * @netdev:  Network interface device structure
> - * Returns
> - *   true(1):  Checksum On
> - *   false(0): Checksum Off
> - */
> -static u32 pch_gbe_get_tx_csum(struct net_device *netdev)
> -{
> -     return (netdev->features & NETIF_F_HW_CSUM) != 0;
> -}
> -
> -/**
>   * pch_gbe_set_tx_csum - Turn transmit checksums on or off
>   * @netdev: Network interface device structure
>   * @data:   Checksum on[true] or off[false]
> @@ -493,11 +481,7 @@ static int pch_gbe_set_tx_csum(struct net_device *netdev, u32 data)
>       struct pch_gbe_adapter *adapter = netdev_priv(netdev);
>
>       adapter->tx_csum = data;
> -     if (data)
> -             netdev->features |= NETIF_F_HW_CSUM;
> -     else
> -             netdev->features &= ~NETIF_F_HW_CSUM;
> -     return 0;
> +     return ethtool_op_set_tx_ipv6_csum(netdev, data);
>  }
>
>  /**
> @@ -572,7 +556,6 @@ static const struct ethtool_ops pch_gbe_ethtool_ops = {
>       .set_pauseparam = pch_gbe_set_pauseparam,
>       .get_rx_csum = pch_gbe_get_rx_csum,
>       .set_rx_csum = pch_gbe_set_rx_csum,
> -     .get_tx_csum = pch_gbe_get_tx_csum,
>       .set_tx_csum = pch_gbe_set_tx_csum,
>       .get_strings = pch_gbe_get_strings,
>       .get_ethtool_stats = pch_gbe_get_ethtool_stats,
> diff --git a/drivers/net/pch_gbe/pch_gbe_main.c b/drivers/net/pch_gbe/pch_gbe_main.c
> index afb7506..58e7903 100644
> --- a/drivers/net/pch_gbe/pch_gbe_main.c
> +++ b/drivers/net/pch_gbe/pch_gbe_main.c
> @@ -2319,7 +2319,7 @@ static int pch_gbe_probe(struct pci_dev *pdev,
>       netdev->watchdog_timeo = PCH_GBE_WATCHDOG_PERIOD;
>       netif_napi_add(netdev, &adapter->napi,
>                      pch_gbe_napi_poll, PCH_GBE_RX_WEIGHT);
> -     netdev->features = NETIF_F_HW_CSUM | NETIF_F_GRO;
> +     netdev->features = NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | NETIF_F_GRO;
>       pch_gbe_set_ethtool_ops(netdev);
>
>       pch_gbe_mac_reset_hw(&adapter->hw);
> @@ -2358,9 +2358,9 @@ static int pch_gbe_probe(struct pci_dev *pdev,
>       pch_gbe_check_options(adapter);
>
>       if (adapter->tx_csum)
> -             netdev->features |= NETIF_F_HW_CSUM;
> +             netdev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
>       else
> -             netdev->features &= ~NETIF_F_HW_CSUM;
> +             netdev->features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM);
>
>       /* initialize the wol settings based on the eeprom settings */
>       adapter->wake_up_evt = PCH_GBE_WL_INIT_SETTING;
> diff --git a/drivers/net/sc92031.c b/drivers/net/sc92031.c
> index 417adf3..76290a8 100644
> --- a/drivers/net/sc92031.c
> +++ b/drivers/net/sc92031.c
> @@ -1449,7 +1449,8 @@ static int __devinit sc92031_probe(struct pci_dev *pdev,
>       dev->irq = pdev->irq;
>
>       /* faked with skb_copy_and_csum_dev */
> -     dev->features = NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_HIGHDMA;
> +     dev->features = NETIF_F_SG | NETIF_F_HIGHDMA |
> +             NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
>
>       dev->netdev_ops         = &sc92031_netdev_ops;
>       dev->watchdog_timeo     = TX_TIMEOUT;
> diff --git a/drivers/net/stmmac/stmmac_ethtool.c b/drivers/net/stmmac/stmmac_ethtool.c
> index f2695fd..fd719ed 100644
> --- a/drivers/net/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/stmmac/stmmac_ethtool.c
> @@ -197,16 +197,6 @@ static void stmmac_ethtool_gregs(struct net_device *dev,
>       }
>  }
>
> -static int stmmac_ethtool_set_tx_csum(struct net_device *netdev, u32 data)
> -{
> -     if (data)
> -             netdev->features |= NETIF_F_HW_CSUM;
> -     else
> -             netdev->features &= ~NETIF_F_HW_CSUM;
> -
> -     return 0;
> -}
> -
>  static u32 stmmac_ethtool_get_rx_csum(struct net_device *dev)
>  {
>       struct stmmac_priv *priv = netdev_priv(dev);
> @@ -370,7 +360,7 @@ static struct ethtool_ops stmmac_ethtool_ops = {
>       .get_link = ethtool_op_get_link,
>       .get_rx_csum = stmmac_ethtool_get_rx_csum,
>       .get_tx_csum = ethtool_op_get_tx_csum,
> -     .set_tx_csum = stmmac_ethtool_set_tx_csum,
> +     .set_tx_csum = ethtool_op_set_tx_ipv6_csum,
>       .get_sg = ethtool_op_get_sg,
>       .set_sg = ethtool_op_set_sg,
>       .get_pauseparam = stmmac_get_pauseparam,
> diff --git a/drivers/net/stmmac/stmmac_main.c b/drivers/net/stmmac/stmmac_main.c
> index 730a6fd..bfc2d12 100644
> --- a/drivers/net/stmmac/stmmac_main.c
> +++ b/drivers/net/stmmac/stmmac_main.c
> @@ -1494,7 +1494,8 @@ static int stmmac_probe(struct net_device *dev)
>       dev->netdev_ops = &stmmac_netdev_ops;
>       stmmac_set_ethtool_ops(dev);
>
> -     dev->features |= (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_HIGHDMA);
> +     dev->features |= NETIF_F_SG | NETIF_F_HIGHDMA |
> +             NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
>       dev->watchdog_timeo = msecs_to_jiffies(watchdog);
>  #ifdef STMMAC_VLAN_TAG_USED
>       /* Both mac100 and gmac support receive VLAN tag detection */
> @@ -1525,7 +1526,7 @@ static int stmmac_probe(struct net_device *dev)
>
>       DBG(probe, DEBUG, "%s: Scatter/Gather: %s - HW checksums: %s\n",
>           dev->name, (dev->features & NETIF_F_SG) ? "on" : "off",
> -         (dev->features & NETIF_F_HW_CSUM) ? "on" : "off");
> +         (dev->features & NETIF_F_IP_CSUM) ? "on" : "off");
>
>       spin_lock_init(&priv->lock);
>
> diff --git a/drivers/net/vxge/vxge-ethtool.c b/drivers/net/vxge/vxge-ethtool.c
> index bc9bd10..1dd3a21 100644
> --- a/drivers/net/vxge/vxge-ethtool.c
> +++ b/drivers/net/vxge/vxge-ethtool.c
> @@ -1177,7 +1177,7 @@ static const struct ethtool_ops vxge_ethtool_ops = {
>       .get_rx_csum            = vxge_get_rx_csum,
>       .set_rx_csum            = vxge_set_rx_csum,
>       .get_tx_csum            = ethtool_op_get_tx_csum,
> -     .set_tx_csum            = ethtool_op_set_tx_hw_csum,
> +     .set_tx_csum            = ethtool_op_set_tx_ipv6_csum,
>       .get_sg                 = ethtool_op_get_sg,
>       .set_sg                 = ethtool_op_set_sg,
>       .get_tso                = ethtool_op_get_tso,
> diff --git a/drivers/net/vxge/vxge-main.c b/drivers/net/vxge/vxge-main.c
> index a21dae1..d339f5b 100644
> --- a/drivers/net/vxge/vxge-main.c
> +++ b/drivers/net/vxge/vxge-main.c
> @@ -3368,7 +3368,7 @@ static int __devinit vxge_device_register(struct __vxge_hw_device *hldev,
>
>       ndev->features |= NETIF_F_SG;
>
> -     ndev->features |= NETIF_F_HW_CSUM;
> +     ndev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
>       vxge_debug_init(vxge_hw_device_trace_level_get(hldev),
>               "%s : checksuming enabled", __func__);
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 3259d2c..622f85a 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5041,10 +5041,13 @@ unsigned long netdev_fix_features(unsigned long features, const char *name)
>       }
>
>       if (features & NETIF_F_UFO) {
> -             if (!(features & NETIF_F_GEN_CSUM)) {
> +             /* maybe split UFO into V4 and V6? */
> +             if (!((features & NETIF_F_GEN_CSUM) ||
> +                 (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
> +                         == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
>                       if (name)
>                               printk(KERN_ERR "%s: Dropping NETIF_F_UFO "
> -                                    "since no NETIF_F_HW_CSUM feature.\n",
> +                                    "since no checksum offload features.\n",
>                                      name);
>                       features &= ~NETIF_F_UFO;
>               }
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 956a9f4..d5bc2881 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1171,7 +1171,9 @@ static int ethtool_set_ufo(struct net_device *dev, char __user *useraddr)
>               return -EFAULT;
>       if (edata.data && !(dev->features & NETIF_F_SG))
>               return -EINVAL;
> -     if (edata.data && !(dev->features & NETIF_F_HW_CSUM))
> +     if (edata.data && !((dev->features & NETIF_F_GEN_CSUM) ||
> +             (dev->features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
> +                     == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM)))
>               return -EINVAL;
>       return dev->ethtool_ops->set_ufo(dev, edata.data);
>  }
>

The information and any attached documents contained in this message
may be confidential and/or legally privileged.  The message is
intended solely for the addressee(s).  If you are not the intended
recipient, you are hereby notified that any use, dissemination, or
reproduction is strictly prohibited and may be unlawful.  If you are
not the intended recipient, please contact the sender immediately by
return e-mail and destroy all copies of the original message.

Re: [PATCH v2] net: Fix too optimistic NETIF_F_HW_CSUM features

[David Miller]

From: MichałMirosław <mirq-linux@rere.qmqm.pl>
Date: Tue, 30 Nov 2010 17:38:00 +0100 (CET)

> NETIF_F_HW_CSUM is a superset of NETIF_F_IP_CSUM+NETIF_F_IPV6_CSUM, but
> some drivers miss the difference. Fix this and also fix UFO dependency
> on checksumming offload as it makes the same mistake in assumptions.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Applied.