* [PATCH] ehea: add the correct LRO status at dev->features @ 2010-12-06 16:39 leitao 2010-12-06 16:48 ` Stephen Hemminger 0 siblings, 1 reply; 8+ messages in thread From: leitao @ 2010-12-06 16:39 UTC (permalink / raw) To: davem; +Cc: netdev, Breno 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ehea: add the correct LRO status at dev->features 2010-12-06 16:39 [PATCH] ehea: add the correct LRO status at dev->features leitao @ 2010-12-06 16:48 ` Stephen Hemminger 2010-12-06 16:57 ` Breno Leitao 0 siblings, 1 reply; 8+ messages in thread From: Stephen Hemminger @ 2010-12-06 16:48 UTC (permalink / raw) To: leitao; +Cc: davem, netdev 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. -- ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ehea: add the correct LRO status at dev->features 2010-12-06 16:48 ` Stephen Hemminger @ 2010-12-06 16:57 ` Breno Leitao 2010-12-06 17:10 ` [RFC] mark devices with broken LRO implementation Stephen Hemminger 2010-12-06 17:33 ` [PATCH] ehea: add the correct LRO status at dev->features David Miller 0 siblings, 2 replies; 8+ messages in thread From: Breno Leitao @ 2010-12-06 16:57 UTC (permalink / raw) To: Stephen Hemminger; +Cc: davem, netdev 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC] mark devices with broken LRO implementation 2010-12-06 16:57 ` Breno Leitao @ 2010-12-06 17:10 ` Stephen Hemminger 2010-12-06 17:18 ` Olof Johansson 2010-12-06 17:33 ` [PATCH] ehea: add the correct LRO status at dev->features David Miller 1 sibling, 1 reply; 8+ messages in thread From: Stephen Hemminger @ 2010-12-06 17:10 UTC (permalink / raw) To: Breno Leitao, Olof Johansson, Eli Cohen; +Cc: davem, netdev 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] mark devices with broken LRO implementation 2010-12-06 17:10 ` [RFC] mark devices with broken LRO implementation Stephen Hemminger @ 2010-12-06 17:18 ` Olof Johansson 2010-12-06 17:28 ` David Miller 0 siblings, 1 reply; 8+ messages in thread From: Olof Johansson @ 2010-12-06 17:18 UTC (permalink / raw) To: Stephen Hemminger; +Cc: Breno Leitao, Eli Cohen, davem, netdev 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] mark devices with broken LRO implementation 2010-12-06 17:18 ` Olof Johansson @ 2010-12-06 17:28 ` David Miller 2010-12-06 17:58 ` Stephen Hemminger 0 siblings, 1 reply; 8+ messages in thread From: David Miller @ 2010-12-06 17:28 UTC (permalink / raw) To: olof; +Cc: shemminger, leitao, eli, netdev 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] mark devices with broken LRO implementation 2010-12-06 17:28 ` David Miller @ 2010-12-06 17:58 ` Stephen Hemminger 0 siblings, 0 replies; 8+ messages in thread From: Stephen Hemminger @ 2010-12-06 17:58 UTC (permalink / raw) To: David Miller, Yevgeny Petrilin; +Cc: olof, leitao, eli, netdev 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; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ehea: add the correct LRO status at dev->features 2010-12-06 16:57 ` Breno Leitao 2010-12-06 17:10 ` [RFC] mark devices with broken LRO implementation Stephen Hemminger @ 2010-12-06 17:33 ` David Miller 1 sibling, 0 replies; 8+ messages in thread From: David Miller @ 2010-12-06 17:33 UTC (permalink / raw) To: leitao; +Cc: shemminger, netdev 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-12-06 17:58 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-12-06 16:39 [PATCH] ehea: add the correct LRO status at dev->features leitao 2010-12-06 16:48 ` Stephen Hemminger 2010-12-06 16:57 ` Breno Leitao 2010-12-06 17:10 ` [RFC] mark devices with broken LRO implementation Stephen Hemminger 2010-12-06 17:18 ` Olof Johansson 2010-12-06 17:28 ` David Miller 2010-12-06 17:58 ` Stephen Hemminger 2010-12-06 17:33 ` [PATCH] ehea: add the correct LRO status at dev->features David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).