* Broken TX checksumming offloads @ 2010-11-29 18:17 Michał Mirosław 2010-11-29 19:13 ` Ben Hutchings 0 siblings, 1 reply; 19+ messages in thread From: Michał Mirosław @ 2010-11-29 18:17 UTC (permalink / raw) To: netdev; +Cc: Toshiharu Okada, e1000-devel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Broken TX checksumming offloads 2010-11-29 18:17 Broken TX checksumming offloads Michał Mirosław @ 2010-11-29 19:13 ` Ben Hutchings 2010-11-30 2:35 ` Jesse Gross 0 siblings, 1 reply; 19+ messages in thread From: Ben Hutchings @ 2010-11-29 19:13 UTC (permalink / raw) To: Michał Mirosław; +Cc: netdev, Toshiharu Okada, e1000-devel 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Broken TX checksumming offloads 2010-11-29 19:13 ` Ben Hutchings @ 2010-11-30 2:35 ` Jesse Gross 2010-11-30 14:23 ` MichałMirosław 0 siblings, 1 reply; 19+ messages in thread From: Jesse Gross @ 2010-11-30 2:35 UTC (permalink / raw) To: Ben Hutchings Cc: Michał Mirosław, netdev, Toshiharu Okada, e1000-devel 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Broken TX checksumming offloads 2010-11-30 2:35 ` Jesse Gross @ 2010-11-30 14:23 ` MichałMirosław 2010-11-30 14:23 ` [PATCH] net: Move check of checksum features to netdev_fix_features() MichałMirosław ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: MichałMirosław @ 2010-11-30 14:23 UTC (permalink / raw) To: netdev; +Cc: Ben Hutchings, Jesse Gross 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] net: Move check of checksum features to netdev_fix_features() 2010-11-30 14:23 ` MichałMirosław @ 2010-11-30 14:23 ` MichałMirosław 2010-11-30 14:23 ` [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features MichałMirosław 2010-11-30 14:23 ` [PATCH] net: Fix drivers advertising HW_CSUM feature to use csum_start MichałMirosław 2 siblings, 0 replies; 19+ messages in thread From: MichałMirosław @ 2010-11-30 14:23 UTC (permalink / raw) To: netdev; +Cc: Ben Hutchings, Jesse Gross 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. */ ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features 2010-11-30 14:23 ` MichałMirosław 2010-11-30 14:23 ` [PATCH] net: Move check of checksum features to netdev_fix_features() MichałMirosław @ 2010-11-30 14:23 ` MichałMirosław [not found] ` <1291130005.21077.18.camel@bwh-desktop> 2010-11-30 16:38 ` [PATCH v2] " MichałMirosław 2010-11-30 14:23 ` [PATCH] net: Fix drivers advertising HW_CSUM feature to use csum_start MichałMirosław 2 siblings, 2 replies; 19+ messages in thread From: MichałMirosław @ 2010-11-30 14:23 UTC (permalink / raw) To: netdev; +Cc: Ben Hutchings, Jesse Gross 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); } ^ permalink raw reply related [flat|nested] 19+ messages in thread
[parent not found: <1291130005.21077.18.camel@bwh-desktop>]
* Re: [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features [not found] ` <1291130005.21077.18.camel@bwh-desktop> @ 2010-11-30 15:28 ` MichałMirosław 2010-11-30 15:41 ` Michał Mirosław 2010-11-30 15:51 ` Ben Hutchings 0 siblings, 2 replies; 19+ messages in thread From: MichałMirosław @ 2010-11-30 15:28 UTC (permalink / raw) To: Ben Hutchings; +Cc: netdev, Jesse Gross 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features 2010-11-30 15:28 ` MichałMirosław @ 2010-11-30 15:41 ` Michał Mirosław 2010-11-30 16:12 ` Jon Mason 2010-11-30 15:51 ` Ben Hutchings 1 sibling, 1 reply; 19+ messages in thread From: Michał Mirosław @ 2010-11-30 15:41 UTC (permalink / raw) To: Ramkrishna Vepa, Sivakumar Subramani, Sreenivasa Honnur, Jon Mason Cc: netdev, Ben Hutchings, Jesse Gross 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features 2010-11-30 15:41 ` Michał Mirosław @ 2010-11-30 16:12 ` Jon Mason 0 siblings, 0 replies; 19+ messages in thread From: Jon Mason @ 2010-11-30 16:12 UTC (permalink / raw) To: Michał Mirosław Cc: Ramkrishna Vepa, Sivakumar Subramani, Sreenivasa Honnur, netdev@vger.kernel.org, Ben Hutchings, Jesse Gross 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features 2010-11-30 15:28 ` MichałMirosław 2010-11-30 15:41 ` Michał Mirosław @ 2010-11-30 15:51 ` Ben Hutchings 2010-11-30 16:18 ` MichałMirosław 1 sibling, 1 reply; 19+ messages in thread From: Ben Hutchings @ 2010-11-30 15:51 UTC (permalink / raw) To: MichałMirosław; +Cc: netdev, Jesse Gross 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features 2010-11-30 15:51 ` Ben Hutchings @ 2010-11-30 16:18 ` MichałMirosław 0 siblings, 0 replies; 19+ messages in thread From: MichałMirosław @ 2010-11-30 16:18 UTC (permalink / raw) To: Ben Hutchings; +Cc: netdev, Jesse Gross 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] net: Fix too optimistic NETIF_F_HW_CSUM features 2010-11-30 14:23 ` [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features MichałMirosław [not found] ` <1291130005.21077.18.camel@bwh-desktop> @ 2010-11-30 16:38 ` MichałMirosław 2010-11-30 16:53 ` Eric Dumazet ` (2 more replies) 1 sibling, 3 replies; 19+ messages in thread From: MichałMirosław @ 2010-11-30 16:38 UTC (permalink / raw) To: netdev Cc: Ben Hutchings, Jesse Gross, Jon Mason, Ramkrishna Vepa, Sivakumar Subramani, Sreenivasa Honnur 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); } ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2] net: Fix too optimistic NETIF_F_HW_CSUM features 2010-11-30 16:38 ` [PATCH v2] " MichałMirosław @ 2010-11-30 16:53 ` Eric Dumazet 2010-11-30 17:07 ` MichałMirosław 2010-12-02 0:44 ` Jon Mason 2010-12-06 21:01 ` David Miller 2 siblings, 1 reply; 19+ messages in thread From: Eric Dumazet @ 2010-11-30 16:53 UTC (permalink / raw) To: MichałMirosław Cc: netdev, Ben Hutchings, Jesse Gross, Jon Mason, Ramkrishna Vepa, Sivakumar Subramani, Sreenivasa Honnur 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... ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] net: Fix too optimistic NETIF_F_HW_CSUM features 2010-11-30 16:53 ` Eric Dumazet @ 2010-11-30 17:07 ` MichałMirosław 2010-11-30 17:15 ` MichałMirosław 0 siblings, 1 reply; 19+ messages in thread From: MichałMirosław @ 2010-11-30 17:07 UTC (permalink / raw) To: Eric Dumazet Cc: netdev, Ben Hutchings, Jesse Gross, Jon Mason, Ramkrishna Vepa, Sivakumar Subramani, Sreenivasa Honnur 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] net: Fix too optimistic NETIF_F_HW_CSUM features 2010-11-30 17:07 ` MichałMirosław @ 2010-11-30 17:15 ` MichałMirosław 2010-11-30 17:17 ` Eric Dumazet 0 siblings, 1 reply; 19+ messages in thread From: MichałMirosław @ 2010-11-30 17:15 UTC (permalink / raw) To: Eric Dumazet Cc: netdev, Ben Hutchings, Jesse Gross, Jon Mason, Ramkrishna Vepa, Sivakumar Subramani, Sreenivasa Honnur 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] net: Fix too optimistic NETIF_F_HW_CSUM features 2010-11-30 17:15 ` MichałMirosław @ 2010-11-30 17:17 ` Eric Dumazet 0 siblings, 0 replies; 19+ messages in thread From: Eric Dumazet @ 2010-11-30 17:17 UTC (permalink / raw) To: MichałMirosław Cc: netdev, Ben Hutchings, Jesse Gross, Jon Mason, Ramkrishna Vepa, Sivakumar Subramani, Sreenivasa Honnur 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 ;) ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] net: Fix too optimistic NETIF_F_HW_CSUM features 2010-11-30 16:38 ` [PATCH v2] " MichałMirosław 2010-11-30 16:53 ` Eric Dumazet @ 2010-12-02 0:44 ` Jon Mason 2010-12-06 21:01 ` David Miller 2 siblings, 0 replies; 19+ messages in thread From: Jon Mason @ 2010-12-02 0:44 UTC (permalink / raw) To: MichałMirosław Cc: netdev@vger.kernel.org, Ben Hutchings, Jesse Gross, Ramkrishna Vepa, Sivakumar Subramani, Sreenivasa Honnur 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] net: Fix too optimistic NETIF_F_HW_CSUM features 2010-11-30 16:38 ` [PATCH v2] " MichałMirosław 2010-11-30 16:53 ` Eric Dumazet 2010-12-02 0:44 ` Jon Mason @ 2010-12-06 21:01 ` David Miller 2 siblings, 0 replies; 19+ messages in thread From: David Miller @ 2010-12-06 21:01 UTC (permalink / raw) To: mirq-linux Cc: netdev, bhutchings, jesse, jon.mason, Ramkrishna.Vepa, Sivakumar.Subramani, Sreenivasa.Honnur 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] net: Fix drivers advertising HW_CSUM feature to use csum_start 2010-11-30 14:23 ` MichałMirosław 2010-11-30 14:23 ` [PATCH] net: Move check of checksum features to netdev_fix_features() MichałMirosław 2010-11-30 14:23 ` [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features MichałMirosław @ 2010-11-30 14:23 ` MichałMirosław 2 siblings, 0 replies; 19+ messages in thread From: MichałMirosław @ 2010-11-30 14:23 UTC (permalink / raw) To: netdev; +Cc: Ben Hutchings, Jesse Gross 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; ^ permalink raw reply related [flat|nested] 19+ messages in thread
end of thread, other threads:[~2010-12-06 21:01 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-11-29 18:17 Broken TX checksumming offloads Michał Mirosław
2010-11-29 19:13 ` Ben Hutchings
2010-11-30 2:35 ` Jesse Gross
2010-11-30 14:23 ` MichałMirosław
2010-11-30 14:23 ` [PATCH] net: Move check of checksum features to netdev_fix_features() MichałMirosław
2010-11-30 14:23 ` [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features MichałMirosław
[not found] ` <1291130005.21077.18.camel@bwh-desktop>
2010-11-30 15:28 ` MichałMirosław
2010-11-30 15:41 ` Michał Mirosław
2010-11-30 16:12 ` Jon Mason
2010-11-30 15:51 ` Ben Hutchings
2010-11-30 16:18 ` MichałMirosław
2010-11-30 16:38 ` [PATCH v2] " MichałMirosław
2010-11-30 16:53 ` Eric Dumazet
2010-11-30 17:07 ` MichałMirosław
2010-11-30 17:15 ` MichałMirosław
2010-11-30 17:17 ` Eric Dumazet
2010-12-02 0:44 ` Jon Mason
2010-12-06 21:01 ` David Miller
2010-11-30 14:23 ` [PATCH] net: Fix drivers advertising HW_CSUM feature to use csum_start MichałMirosław
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).