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

[Ben Hutchings <bhutchings@solarflare.com>]

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.