Re: Broken TX checksumming offloads

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

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.