[ofa-general] [PATCH 4/16] Add checksum offload support for ipoib

[Eli Cohen]

On Thu, 2008-01-17 at 15:18 +0200, Or Gerlitz wrote:
> Eli Cohen wrote:
> > --- a/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> > +++ b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> > @@ -231,6 +232,18 @@ static void ipoib_ib_handle_rx_wc(struct net_device *dev, struct ib_wc *wc)
> >  	skb->dev = dev;
> >  	/* XXX get correct PACKET_ type here */
> >  	skb->pkt_type = PACKET_HOST;
> > +
> > +	/* check rx csum */
> > +	if (test_bit(IPOIB_FLAG_CSUM, &priv->flags) && likely(wc->csum_ok)) {
> 
> First, since the device IB_DEVICE_IP_CSUM capability means that "devices 
> which publish this capability must support insertion of UDP and TCP 
> checksum on outgoing packets and can verify the validity of checksum for 
> incoming packets" the IPOIB_FLAG_CSUM bit is redundant, I suggest to 
> remove it.
IB_DEVICE_IP_CSUM is used by IPOIB to mark the devices which are capable
of verifying checksum. This flag is reflected in both IPOIB_FLAG_CSUM
and NETIF_F_IP_CSUM. is your suggestion to use NETIF_F_IP_CSUM instead
of IPOIB_FLAG_CSUM? Because I don't see how we can omit it unless we
require the low lwvel drivers to set cqe.csum_ok only if they are sure
that checksum is OK.


>  Second, the csum_ok bit is not well defined, etc as I 
> commented on patch #4

I will make it clearer by stating that Mellanox devices require one more
condition but the semantics of this field is that it is set only if
checksum is known to be good. Another option is to use the condition I
put in ipoib in the low level driver and that will remove the confusion.
What do you think?

> 
> > @@ -394,6 +407,12 @@ void ipoib_send(struct net_device *dev, struct sk_buff *skb,
> >  		return;
> >  	}
> >  
> > +	if (priv->ca->flags & IB_DEVICE_IP_CSUM &&
> > +	    skb->ip_summed == CHECKSUM_PARTIAL)
> > +		priv->tx_wr.send_flags |= IB_SEND_IP_CSUM;
> > +	else
> > +		priv->tx_wr.send_flags &= IB_SEND_IP_CSUM;
> 
> I think that the code would be somehow clearer if you use dev->features 
> and not priv->ca->flags
> 

OK, I will change this.