[ofa-general] Re: [PATCH v4 for-2.6.27] IPOIB: add LRO support.

[Roland Dreier]

thanks, applied...

a couple comments though (I fixed everything up myself, no need to resend):

 > From c859eed63331e43993b0c3f0c903e48d1e27def2 Mon Sep 17 00:00:00 2001
 > From: Vladimir Sokolovsky <vlad at mellanox.co.il>
 > Date: Mon, 23 Jun 2008 15:38:27 +0300
 > Subject: [PATCH] IPOIB: add LRO support.

Can you guys stop including this header in the patches you send?  I just
have to edit it out by hand to avoid it getting included in the
changelog.  If you want to control the author field (which makes sense
in this case, since Vlad sent it from his gmail account), then just put
the From: line in.

 > +extern int ipoib_use_lro;
 > +extern int ipoib_lro_max_aggr;

these variables can just be static to ipoib_main.c, since no other file
uses them.  Which is fortunate because you put the extern declaration
inside #ifdef CONFIG_INFINIBAND_IPOIB_DEBUG.

 > +	/* check if ip header and tcp header are complete */
 > +	if (iph->tot_len < ip_len + tcp_hdrlen(skb))

iph->tot_len is a __be16, so I assume this line should be fixed to read:

	if (be16_to_cpu(iph->tot_len) < ip_len + tcp_hdrlen(skb))

(running sparse found this bug -- another reason to run sparse on your
patches before submitting them).

 > +		lro_receive_skb(&priv->lro.lro_mgr, skb, 0);

sparse prefers NULL for a pointer to 0 here.

 > +#define IPOIB_STATS_LEN  ARRAY_SIZE(ipoib_gstrings_stats)

seems cleaner just to use the expression itself in the one place that it
is referenced.

 > +ipoib_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 > +{
 > +	switch (stringset) {
 > +	case ETH_SS_STATS:
 > +		memcpy(data, *ipoib_gstrings_stats,
 > +			sizeof(ipoib_gstrings_stats));
 > +			data += sizeof(ipoib_gstrings_stats);

indentation broke, and the "data +=" line is pointless anyway, since
data is a local variable in a function that doesn't do anything more
after that line.

 - R.