[PATCH] ehea: add the correct LRO status at dev->features

[PATCH] ehea: add the correct LRO status at dev->features

[leitao]

Currently ehea is not setting NETIF_F_LRO, and it is not providing
a callback for get_flags on ethtool. This patch fixes it.

Signed-off-by: Breno Leitao <leitao@linux.vnet.ibm.com>
---
 drivers/net/ehea/ehea_ethtool.c |    1 +
 drivers/net/ehea/ehea_main.c    |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ehea/ehea_ethtool.c b/drivers/net/ehea/ehea_ethtool.c
index 75b099c..b0892b0 100644
--- a/drivers/net/ehea/ehea_ethtool.c
+++ b/drivers/net/ehea/ehea_ethtool.c
@@ -273,6 +273,7 @@ const struct ethtool_ops ehea_ethtool_ops = {
 	.get_ethtool_stats = ehea_get_ethtool_stats,
 	.get_rx_csum = ehea_get_rx_csum,
 	.set_settings = ehea_set_settings,
+	.get_flags = ethtool_op_get_flags,
 	.nway_reset = ehea_nway_reset,		/* Restart autonegotiation */
 };
 
diff --git a/drivers/net/ehea/ehea_main.c b/drivers/net/ehea/ehea_main.c
index a84c389..da75846 100644
--- a/drivers/net/ehea/ehea_main.c
+++ b/drivers/net/ehea/ehea_main.c
@@ -3266,6 +3266,9 @@ struct ehea_port *ehea_setup_single_port(struct ehea_adapter *adapter,
 		      | NETIF_F_LLTX;
 	dev->watchdog_timeo = EHEA_WATCH_DOG_TIMEOUT;
 
+	if (use_lro)
+		dev->features |= NETIF_F_LRO;
+
 	INIT_WORK(&port->reset_task, ehea_reset_port);
 
 	ret = register_netdev(dev);
-- 
1.7.3.2

Re: [PATCH] ehea: add the correct LRO status at dev->features

[Stephen Hemminger]

On Mon,  6 Dec 2010 14:39:42 -0200
leitao@linux.vnet.ibm.com wrote:

> Currently ehea is not setting NETIF_F_LRO, and it is not providing
> a callback for get_flags on ethtool. This patch fixes it.
> 
> Signed-off-by: Breno Leitao <leitao@linux.vnet.ibm.com>

More importantly, ehea does not support set_flags to disable LRO.

I will be more blunt. Any device that supports LRO and does
not have the necessary interface to disable it is broken and should
not be used.

-- 

Re: [PATCH] ehea: add the correct LRO status at dev->features

[Breno Leitao]

On 12/06/2010 02:48 PM, Stephen Hemminger wrote:
> On Mon,  6 Dec 2010 14:39:42 -0200
> leitao@linux.vnet.ibm.com wrote:
>
>> Currently ehea is not setting NETIF_F_LRO, and it is not providing
>> a callback for get_flags on ethtool. This patch fixes it.
>>
>> Signed-off-by: Breno Leitao<leitao@linux.vnet.ibm.com>
>
> More importantly, ehea does not support set_flags to disable LRO.
Correct, currently LRO is a module parameter. I have an item in my TODO 
list to implement set_flags, and thus, the LRO scheme.

So, if you prefer I can send this patch with the future set_flags ones. 
But, for now, this patch allows the user to check when LRO is enabled. 
As it is today, it shows that LRO is disabled all the time.

Anyway, you choose what is the best option.

Thanks,
Breno

[RFC] mark devices with broken LRO implementation

[Stephen Hemminger]

The kernel uses dev_disable_lro to disable Large Receive Offload
for cases where it is inappropriate. But several drivers do not implement
the required hook.  This is a "penalty box" patch to encourage those
drivers to add support for the necessary ethtool set_flags.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
---
I only found three drivers that are broken. Normally, I would just fix
them; but since changing state in the device is hardware specific, and
I don't have the hardware or specs to do anything useful to fix it.

--- a/drivers/net/Kconfig	2010-12-06 08:52:30.376926696 -0800
+++ b/drivers/net/Kconfig	2010-12-06 09:02:02.013224592 -0800
@@ -28,6 +28,13 @@ menuconfig NETDEVICES
 # that for each of the symbols.
 if NETDEVICES
 
+config BROKEN_LRO
+       bool
+       ---help---
+         Allow drivers with partial Large Receive Offload support. These drivers
+	 do not implement the necessary feature of disabling LRO support via
+	 the ethtool set_flags operation.
+
 config IFB
 	tristate "Intermediate Functional Block support"
 	depends on NET_CLS_ACT
@@ -2675,7 +2682,7 @@ config CHELSIO_T4VF
 
 config EHEA
 	tristate "eHEA Ethernet support"
-	depends on IBMEBUS && INET && SPARSEMEM
+	depends on IBMEBUS && INET && SPARSEMEM && BROKEN_LRO
 	select INET_LRO
 	---help---
 	  This driver supports the IBM pSeries eHEA ethernet adapter.
@@ -2848,7 +2855,7 @@ config NIU
 
 config PASEMI_MAC
 	tristate "PA Semi 1/10Gbit MAC"
-	depends on PPC_PASEMI && PCI && INET
+	depends on PPC_PASEMI && PCI && INET && BROKEN_LRO
 	select PHYLIB
 	select INET_LRO
 	help
@@ -2857,7 +2864,7 @@ config PASEMI_MAC
 
 config MLX4_EN
 	tristate "Mellanox Technologies 10Gbit Ethernet support"
-	depends on PCI && INET
+	depends on PCI && INET && BROKEN_LRO
 	select MLX4_CORE
 	select INET_LRO
 	help

Re: [RFC] mark devices with broken LRO implementation

[Olof Johansson]

On Mon, Dec 06, 2010 at 09:10:10AM -0800, Stephen Hemminger wrote:
> The kernel uses dev_disable_lro to disable Large Receive Offload
> for cases where it is inappropriate. But several drivers do not implement
> the required hook.  This is a "penalty box" patch to encourage those
> drivers to add support for the necessary ethtool set_flags.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

pasemi_mac:

Acked-by: Olof Johansson <olof@lixom.net>

> I only found three drivers that are broken. Normally, I would just fix
> them; but since changing state in the device is hardware specific, and
> I don't have the hardware or specs to do anything useful to fix it.

Thanks. All my hardware is currently in storage due to my relocation, but
I'll take a look at it when I have something to run on back. :-)


-Olof

Re: [RFC] mark devices with broken LRO implementation

[David Miller]

From: Olof Johansson <olof@lixom.net>
Date: Mon, 6 Dec 2010 11:18:16 -0600

> On Mon, Dec 06, 2010 at 09:10:10AM -0800, Stephen Hemminger wrote:
>> The kernel uses dev_disable_lro to disable Large Receive Offload
>> for cases where it is inappropriate. But several drivers do not implement
>> the required hook.  This is a "penalty box" patch to encourage those
>> drivers to add support for the necessary ethtool set_flags.
>> 
>> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> pasemi_mac:
> 
> Acked-by: Olof Johansson <olof@lixom.net>
> 
>> I only found three drivers that are broken. Normally, I would just fix
>> them; but since changing state in the device is hardware specific, and
>> I don't have the hardware or specs to do anything useful to fix it.
> 
> Thanks. All my hardware is currently in storage due to my relocation, but
> I'll take a look at it when I have something to run on back. :-)

I don't think we can do what this patch does.

It serves no purpose, as it just means the user is going to have an
extra headache to turn their driver on.

And %99 of them won't see it, the distribution folks will.

So this is all for nothing.

Better to just work on fixing the problem in the few drivers for
which the issue exists.

And I'm always happy to receive patches that just plain rip LRO
support out of such drivers.  That's a real legitimate patch unlike
this one because it keeps the user able to get their driver enabled
with no fuss and it removes only the improperly functioning feature.

Re: [PATCH] ehea: add the correct LRO status at dev->features

[David Miller]

From: Breno Leitao <leitao@linux.vnet.ibm.com>
Date: Mon, 06 Dec 2010 14:57:26 -0200

> On 12/06/2010 02:48 PM, Stephen Hemminger wrote:
>> On Mon,  6 Dec 2010 14:39:42 -0200
>> leitao@linux.vnet.ibm.com wrote:
>>
>>> Currently ehea is not setting NETIF_F_LRO, and it is not providing
>>> a callback for get_flags on ethtool. This patch fixes it.
>>>
>>> Signed-off-by: Breno Leitao<leitao@linux.vnet.ibm.com>
>>
>> More importantly, ehea does not support set_flags to disable LRO.
> Correct, currently LRO is a module parameter. I have an item in my
> TODO list to implement set_flags, and thus, the LRO scheme.
> 
> So, if you prefer I can send this patch with the future set_flags
> ones. But, for now, this patch allows the user to check when LRO is
> enabled. As it is today, it shows that LRO is disabled all the time.
> 
> Anyway, you choose what is the best option.

Your options are: 1) send a set_flags patch now 2) rip LRO support
completely out of the ehea driver.

The feature is implemented improperly, and as such we have every right
to forcefully disable it or remove it until it is fixed to function
correctly.

Re: [RFC] mark devices with broken LRO implementation

[Stephen Hemminger]

On Mon, 06 Dec 2010 09:28:55 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Olof Johansson <olof@lixom.net>
> Date: Mon, 6 Dec 2010 11:18:16 -0600
> 
> > On Mon, Dec 06, 2010 at 09:10:10AM -0800, Stephen Hemminger wrote:
> >> The kernel uses dev_disable_lro to disable Large Receive Offload
> >> for cases where it is inappropriate. But several drivers do not implement
> >> the required hook.  This is a "penalty box" patch to encourage those
> >> drivers to add support for the necessary ethtool set_flags.
> >> 
> >> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> > 
> > pasemi_mac:
> > 
> > Acked-by: Olof Johansson <olof@lixom.net>
> > 
> >> I only found three drivers that are broken. Normally, I would just fix
> >> them; but since changing state in the device is hardware specific, and
> >> I don't have the hardware or specs to do anything useful to fix it.
> > 
> > Thanks. All my hardware is currently in storage due to my relocation, but
> > I'll take a look at it when I have something to run on back. :-)
> 
> I don't think we can do what this patch does.
> 
> It serves no purpose, as it just means the user is going to have an
> extra headache to turn their driver on.
> 
> And %99 of them won't see it, the distribution folks will.
> 
> So this is all for nothing.
> 
> Better to just work on fixing the problem in the few drivers for
> which the issue exists.
> 
> And I'm always happy to receive patches that just plain rip LRO
> support out of such drivers.  That's a real legitimate patch unlike
> this one because it keeps the user able to get their driver enabled
> with no fuss and it removes only the improperly functioning feature.

I am less worried about the drivers for embedded devices, the one that
surprises me the mellanox driver.  But it looks like that driver doesn't
really do LRO (only GRO).

Subject: mlx4: remove reference to LRO

This device does not do LRO, the current version does GRO.
This patch removes unused inline and changes the configuration
and comments to reflect that.  Compile tested only.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
 drivers/net/Kconfig        |    1 -
 drivers/net/mlx4/en_rx.c   |    6 +++---
 drivers/net/mlx4/mlx4_en.h |   17 -----------------
 3 files changed, 3 insertions(+), 21 deletions(-)

--- a/drivers/net/Kconfig	2010-12-06 09:49:11.210488154 -0800
+++ b/drivers/net/Kconfig	2010-12-06 09:49:45.461406671 -0800
@@ -2859,7 +2859,6 @@ config MLX4_EN
 	tristate "Mellanox Technologies 10Gbit Ethernet support"
 	depends on PCI && INET
 	select MLX4_CORE
-	select INET_LRO
 	help
 	  This driver supports Mellanox Technologies ConnectX Ethernet
 	  devices.
--- a/drivers/net/mlx4/en_rx.c	2010-12-06 09:51:38.316270948 -0800
+++ b/drivers/net/mlx4/en_rx.c	2010-12-06 09:51:55.758024934 -0800
@@ -584,7 +584,7 @@ int mlx4_en_process_rx_cq(struct net_dev
 			if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPOK)) &&
 			    (cqe->checksum == cpu_to_be16(0xffff))) {
 				priv->port_stats.rx_chksum_good++;
-				/* This packet is eligible for LRO if it is:
+				/* This packet is eligible for GRO if it is:
 				 * - DIX Ethernet (type interpretation)
 				 * - TCP/IP (v4)
 				 * - without IP options
@@ -616,7 +616,7 @@ int mlx4_en_process_rx_cq(struct net_dev
 					goto next;
 				}
 
-				/* LRO not possible, complete processing here */
+				/* GRO not possible, complete processing here */
 				ip_summed = CHECKSUM_UNNECESSARY;
 			} else {
 				ip_summed = CHECKSUM_NONE;
@@ -657,7 +657,7 @@ next:
 		cqe = &cq->buf[index];
 		if (++polled == budget) {
 			/* We are here because we reached the NAPI budget -
-			 * flush only pending LRO sessions */
+			 * flush only pending GRO sessions */
 			goto out;
 		}
 	}
--- a/drivers/net/mlx4/mlx4_en.h	2010-12-06 09:50:40.142348439 -0800
+++ b/drivers/net/mlx4/mlx4_en.h	2010-12-06 09:51:59.470397030 -0800
@@ -83,8 +83,6 @@
 #define MLX4_EN_ALLOC_ORDER	2
 #define MLX4_EN_ALLOC_SIZE	(PAGE_SIZE << MLX4_EN_ALLOC_ORDER)
 
-#define MLX4_EN_MAX_LRO_DESCRIPTORS	32
-
 /* Receive fragment sizes; we use at most 4 fragments (for 9600 byte MTU
  * and 4K allocations) */
 enum {
@@ -268,21 +266,6 @@ struct mlx4_en_rx_ring {
 	unsigned long packets;
 };
 
-
-static inline int mlx4_en_can_lro(__be16 status)
-{
-	return (status & cpu_to_be16(MLX4_CQE_STATUS_IPV4	|
-				     MLX4_CQE_STATUS_IPV4F	|
-				     MLX4_CQE_STATUS_IPV6	|
-				     MLX4_CQE_STATUS_IPV4OPT	|
-				     MLX4_CQE_STATUS_TCP	|
-				     MLX4_CQE_STATUS_UDP	|
-				     MLX4_CQE_STATUS_IPOK)) ==
-		cpu_to_be16(MLX4_CQE_STATUS_IPV4 |
-			    MLX4_CQE_STATUS_IPOK |
-			    MLX4_CQE_STATUS_TCP);
-}
-
 struct mlx4_en_cq {
 	struct mlx4_cq          mcq;
 	struct mlx4_hwq_resources wqres;