[ofa-general] [PATCH 6/11] IB/ipoib: add checksum offload support

[Or Gerlitz]

Eli Cohen wrote:
> On Tue, 2007-09-25 at 12:33 +0200, Or Gerlitz wrote:
>> Eli Cohen wrote:
>>> Add checksum offload support to ipoib
>> Can you clarify the relation between this patch to "[PATCHv3] IB/ipoib: 
>> HW checksum support" patch posted later by Michael? for example, I see 
>> that you patch makes IPoIB to publish the NETIF_F_IP_CSUM capability and 
>> Michael's one publishes NETIF_F_HW_CSUM, etc
> 
> These two patches are not related. Michael's patch relies on infinband's
> icrc to not require checksum generation/validation while my patch relies
> on HW's capability to insert checksum on outgoing packets and calculate
> checksum of incoming packtes.

I am not with you. Does the connectX actually computes and inserts 
tcp/udp checksum on outgoing packets? through the discussion at the 
other thread I understood that the answer is no.

If it does not insert a checksum, over which HW will your patch be 
operative, a future one? If it does insert checksum, why is Michael's 
patch needed at all?

see more comments below,

>>> Index: ofa_1_3_dev_kernel/drivers/infiniband/ulp/ipoib/ipoib.h
>>> ===================================================================
>>> --- ofa_1_3_dev_kernel.orig/drivers/infiniband/ulp/ipoib/ipoib.h	2007-09-24 12:09:21.000000000 +0200
>>> +++ ofa_1_3_dev_kernel/drivers/infiniband/ulp/ipoib/ipoib.h	2007-09-24 12:49:00.000000000 +0200
>>> @@ -86,6 +86,7 @@ enum {
>>>  	IPOIB_MCAST_STARTED       = 8,
>>>  	IPOIB_FLAG_NETIF_STOPPED  = 9,
>>>  	IPOIB_FLAG_ADMIN_CM 	  = 10,
>>> +	IPOIB_FLAG_RX_CSUM        = 11,
>>>  
>>>  	IPOIB_MAX_BACKOFF_SECONDS = 16,
>>>  
>>> Index: ofa_1_3_dev_kernel/drivers/infiniband/ulp/ipoib/ipoib_cm.c
>>> ===================================================================
>>> --- ofa_1_3_dev_kernel.orig/drivers/infiniband/ulp/ipoib/ipoib_cm.c	2007-09-24 12:23:26.000000000 +0200
>>> +++ ofa_1_3_dev_kernel/drivers/infiniband/ulp/ipoib/ipoib_cm.c	2007-09-24 13:05:21.000000000 +0200
>>> @@ -1258,6 +1258,13 @@ static ssize_t set_mode(struct device *d

>>> @@ -1266,6 +1273,10 @@ static ssize_t set_mode(struct device *d
>>> +		if (priv->ca->flags & IB_DEVICE_IP_CSUM)
>>> +			dev->features |= NETIF_F_IP_CSUM; /* ipv6 too */

>> didn't you want to use NETIF_F_HW_CSUM here?

> No, our hw does ip/udp/tcp checksum only.

If its only for IPv4 then NETIF_F_IP_CSUM is fine, if you support also 
IPv6 then you want to OR also NETIF_F_IPV6_CSUM, correct?

>>> Index: ofa_1_3_dev_kernel/drivers/infiniband/ulp/ipoib/ipoib_main.c
>>> ===================================================================
>>> --- ofa_1_3_dev_kernel.orig/drivers/infiniband/ulp/ipoib/ipoib_main.c	2007-09-24 12:23:00.000000000 +0200
>>> +++ ofa_1_3_dev_kernel/drivers/infiniband/ulp/ipoib/ipoib_main.c	2007-09-24 13:04:52.000000000 +0200
>>> @@ -1109,6 +1109,29 @@ int ipoib_add_pkey_attr(struct net_devic
>>>  	return device_create_file(&dev->dev, &dev_attr_pkey);
>>>  }
>>>  
>>> +static void set_tx_csum(struct net_device *dev)
>>> +{
>>> +	struct ipoib_dev_priv *priv = netdev_priv(dev);
>>> +
>>> +	if (test_bit(IPOIB_FLAG_ADMIN_CM, &priv->flags))
>>> +		return;
>>> +
>>> +	if (!(priv->ca->flags & IB_DEVICE_IP_CSUM))
>>> +		return;
>>> +
>>> +	dev->features |= NETIF_F_SG | NETIF_F_IP_CSUM; /* turn on ipv6 too */

>> can you explain why this line belongs specifically to set_tx_csum() ?

???

Or.