[PATCH] ehea: Fixing LRO configuration

[PATCH] ehea: Fixing LRO configuration

[leitao]

In order to set LRO on ehea, the user must set a module parameter, which
is not the standard way to do so. This patch adds a way to set LRO using
the ethtool tool.

Signed-off-by: Breno Leitao <leitao@linux.vnet.ibm.com>

diff --git a/drivers/net/ehea/ehea_ethtool.c b/drivers/net/ehea/ehea_ethtool.c
index 75b099c..1f37ee6 100644
--- a/drivers/net/ehea/ehea_ethtool.c
+++ b/drivers/net/ehea/ehea_ethtool.c
@@ -261,6 +261,13 @@ static void ehea_get_ethtool_stats(struct net_device *dev,
 
 }
 
+static int ehea_set_flags(struct net_device *dev, u32 data)
+{
+	return ethtool_op_set_flags(dev, data, ETH_FLAG_LRO
+					| ETH_FLAG_TXVLAN
+					| ETH_FLAG_RXVLAN);
+}
+
 const struct ethtool_ops ehea_ethtool_ops = {
 	.get_settings = ehea_get_settings,
 	.get_drvinfo = ehea_get_drvinfo,
@@ -273,6 +280,8 @@ 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,
+	.set_flags = ehea_set_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..c7132e8 100644
--- a/drivers/net/ehea/ehea_main.c
+++ b/drivers/net/ehea/ehea_main.c
@@ -675,7 +675,7 @@ static void ehea_proc_skb(struct ehea_port_res *pr, struct ehea_cqe *cqe,
 	int vlan_extracted = ((cqe->status & EHEA_CQE_VLAN_TAG_XTRACT) &&
 			      pr->port->vgrp);
 
-	if (use_lro) {
+	if (skb->dev->features && NETIF_F_LRO) {
 		if (vlan_extracted)
 			lro_vlan_hwaccel_receive_skb(&pr->lro_mgr, skb,
 						     pr->port->vgrp,
@@ -777,7 +777,7 @@ static int ehea_proc_rwqes(struct net_device *dev,
 		}
 		cqe = ehea_poll_rq1(qp, &wqe_index);
 	}
-	if (use_lro)
+	if (dev->features && NETIF_F_LRO)
 		lro_flush_all(&pr->lro_mgr);
 
 	pr->rx_packets += processed;
@@ -3265,6 +3265,9 @@ struct ehea_port *ehea_setup_single_port(struct ehea_adapter *adapter,
 		      | NETIF_F_HW_VLAN_RX | NETIF_F_HW_VLAN_FILTER
 		      | 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);
 
-- 
1.7.3.2

Re: [PATCH] ehea: Fixing LRO configuration

[Ben Hutchings]

On Wed, 2010-12-08 at 16:31 -0200, leitao@linux.vnet.ibm.com wrote:
> In order to set LRO on ehea, the user must set a module parameter, which
> is not the standard way to do so. This patch adds a way to set LRO using
> the ethtool tool.
[...]
> diff --git a/drivers/net/ehea/ehea_main.c b/drivers/net/ehea/ehea_main.c
> index a84c389..c7132e8 100644
> --- a/drivers/net/ehea/ehea_main.c
> +++ b/drivers/net/ehea/ehea_main.c
> @@ -675,7 +675,7 @@ static void ehea_proc_skb(struct ehea_port_res *pr, struct ehea_cqe *cqe,
>  	int vlan_extracted = ((cqe->status & EHEA_CQE_VLAN_TAG_XTRACT) &&
>  			      pr->port->vgrp);
>  
> -	if (use_lro) {
> +	if (skb->dev->features && NETIF_F_LRO) {

Should be & not &&.

>  		if (vlan_extracted)
>  			lro_vlan_hwaccel_receive_skb(&pr->lro_mgr, skb,
>  						     pr->port->vgrp,
> @@ -777,7 +777,7 @@ static int ehea_proc_rwqes(struct net_device *dev,
>  		}
>  		cqe = ehea_poll_rq1(qp, &wqe_index);
>  	}
> -	if (use_lro)
> +	if (dev->features && NETIF_F_LRO)

Ditto.

Ben.

>  		lro_flush_all(&pr->lro_mgr);
>  
>  	pr->rx_packets += processed;
> @@ -3265,6 +3265,9 @@ struct ehea_port *ehea_setup_single_port(struct ehea_adapter *adapter,
>  		      | NETIF_F_HW_VLAN_RX | NETIF_F_HW_VLAN_FILTER
>  		      | 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);
>  

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH] ehea: Fixing LRO configuration

[leitao]

In order to set LRO on ehea, the user must set a module parameter, which
is not the standard way to do so. This patch adds a way to set LRO using
the ethtool tool.

Signed-off-by: Breno Leitao <leitao@linux.vnet.ibm.com>

diff --git a/drivers/net/ehea/ehea_ethtool.c b/drivers/net/ehea/ehea_ethtool.c
index 75b099c..1f37ee6 100644
--- a/drivers/net/ehea/ehea_ethtool.c
+++ b/drivers/net/ehea/ehea_ethtool.c
@@ -261,6 +261,13 @@ static void ehea_get_ethtool_stats(struct net_device *dev,
 
 }
 
+static int ehea_set_flags(struct net_device *dev, u32 data)
+{
+	return ethtool_op_set_flags(dev, data, ETH_FLAG_LRO
+					| ETH_FLAG_TXVLAN
+					| ETH_FLAG_RXVLAN);
+}
+
 const struct ethtool_ops ehea_ethtool_ops = {
 	.get_settings = ehea_get_settings,
 	.get_drvinfo = ehea_get_drvinfo,
@@ -273,6 +280,8 @@ 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,
+	.set_flags = ehea_set_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..c7132e8 100644
--- a/drivers/net/ehea/ehea_main.c
+++ b/drivers/net/ehea/ehea_main.c
@@ -675,7 +675,7 @@ static void ehea_proc_skb(struct ehea_port_res *pr, struct ehea_cqe *cqe,
 	int vlan_extracted = ((cqe->status & EHEA_CQE_VLAN_TAG_XTRACT) &&
 			      pr->port->vgrp);
 
-	if (use_lro) {
+	if (skb->dev->features & NETIF_F_LRO) {
 		if (vlan_extracted)
 			lro_vlan_hwaccel_receive_skb(&pr->lro_mgr, skb,
 						     pr->port->vgrp,
@@ -777,7 +777,7 @@ static int ehea_proc_rwqes(struct net_device *dev,
 		}
 		cqe = ehea_poll_rq1(qp, &wqe_index);
 	}
-	if (use_lro)
+	if (dev->features & NETIF_F_LRO)
 		lro_flush_all(&pr->lro_mgr);
 
 	pr->rx_packets += processed;
@@ -3265,6 +3265,9 @@ struct ehea_port *ehea_setup_single_port(struct ehea_adapter *adapter,
 		      | NETIF_F_HW_VLAN_RX | NETIF_F_HW_VLAN_FILTER
 		      | 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);
 
-- 
1.7.3.2

Re: [PATCH] ehea: Fixing LRO configuration

[David Miller]

From: leitao@linux.vnet.ibm.com
Date: Wed,  8 Dec 2010 16:51:20 -0200

> In order to set LRO on ehea, the user must set a module parameter, which
> is not the standard way to do so. This patch adds a way to set LRO using
> the ethtool tool.
> 
> Signed-off-by: Breno Leitao <leitao@linux.vnet.ibm.com>

Applied, thanks Breno.

Re: [PATCH] ehea: Fixing LRO configuration

[Jesse Gross]

On Wed, Dec 8, 2010 at 10:31 AM,  <leitao@linux.vnet.ibm.com> wrote:
> In order to set LRO on ehea, the user must set a module parameter, which
> is not the standard way to do so. This patch adds a way to set LRO using
> the ethtool tool.
>
> Signed-off-by: Breno Leitao <leitao@linux.vnet.ibm.com>
>
> diff --git a/drivers/net/ehea/ehea_ethtool.c b/drivers/net/ehea/ehea_ethtool.c
> index 75b099c..1f37ee6 100644
> --- a/drivers/net/ehea/ehea_ethtool.c
> +++ b/drivers/net/ehea/ehea_ethtool.c
> @@ -261,6 +261,13 @@ static void ehea_get_ethtool_stats(struct net_device *dev,
>
>  }
>
> +static int ehea_set_flags(struct net_device *dev, u32 data)
> +{
> +       return ethtool_op_set_flags(dev, data, ETH_FLAG_LRO
> +                                       | ETH_FLAG_TXVLAN
> +                                       | ETH_FLAG_RXVLAN);

I don't think that this should enable those vlan offloading flags.  I
don't see any logic to actually disable vlan stripping on receive if
that flag is toggled.  Transmit might be OK, since it will cause the
networking core to insert the tag in software if offloading is
disabled.  However, I see some places where skb->protocol is accessed
to determine the protocol of the packet being transmitted.  In most
drivers this causes a problem when transmit vlan offloading is
disabled because the packet type becomes ETH_P_8021Q, instead of the
expected protocol.  On the other hand, it appears that there aren't
any offloads in vlan_features, so maybe it doesn't matter.

Re: [PATCH] ehea: Fixing LRO configuration

[Breno Leitao]

Hi Jesse,

On 12/12/2010 10:39 PM, Jesse Gross wrote:
> I don't think that this should enable those vlan offloading flags.
I just inserted ETH_FLAG_TXVLAN and ETH_FLAG_RXVLAN there because when I 
call ethtool -K ethX lro on/off, it sends the ETH_FLAG_LRO plus the vlan 
flags if they are enabled (and they are by default).

So, if it doesn't make sense to be toggled, I can mask these flags at 
ehea_set_flags(). so that it doesn't arrive at ethtool_op_set_flags().

Does it sound better to you ?

Thanks
Breno

Re: [PATCH] ehea: Fixing LRO configuration

[Jesse Gross]

On Tue, Dec 14, 2010 at 5:01 AM, Breno Leitao <leitao@linux.vnet.ibm.com> wrote:
> Hi Jesse,
>
> On 12/12/2010 10:39 PM, Jesse Gross wrote:
>>
>> I don't think that this should enable those vlan offloading flags.
>
> I just inserted ETH_FLAG_TXVLAN and ETH_FLAG_RXVLAN there because when I
> call ethtool -K ethX lro on/off, it sends the ETH_FLAG_LRO plus the vlan
> flags if they are enabled (and they are by default).
>
> So, if it doesn't make sense to be toggled, I can mask these flags at
> ehea_set_flags(). so that it doesn't arrive at ethtool_op_set_flags().

I think the right solution is to check that they are set before the
call to ethtool_op_set_flags() and return -EINVAL if not.  Masking
them out would result in turning the features off, which isn't a valid
state here.

Of course, the alternative is to actually add support for the settings
to be toggled.  This would be ideal but it doesn't look the driver
ever does that currently, so maybe it's not supported by hardware.

Thanks.

[PATCH] ehea: Avoid changing vlan flags

[leitao]

This patch avoids disabling the vlan flags using ethtool.

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

diff --git a/drivers/net/ehea/ehea_ethtool.c b/drivers/net/ehea/ehea_ethtool.c
index afebf20..3e2e734 100644
--- a/drivers/net/ehea/ehea_ethtool.c
+++ b/drivers/net/ehea/ehea_ethtool.c
@@ -265,6 +265,13 @@ static void ehea_get_ethtool_stats(struct net_device *dev,
 
 static int ehea_set_flags(struct net_device *dev, u32 data)
 {
+	/* Avoid changing the VLAN flags */
+	if ((data & (ETH_FLAG_RXVLAN | ETH_FLAG_TXVLAN)) !=
+	    (ethtool_op_get_flags(dev) & (ETH_FLAG_RXVLAN |
+					  ETH_FLAG_TXVLAN))){
+		return -EINVAL;
+	}
+
 	return ethtool_op_set_flags(dev, data, ETH_FLAG_LRO
 					| ETH_FLAG_TXVLAN
 					| ETH_FLAG_RXVLAN);
-- 
1.7.3.3

Re: [PATCH] ehea: Avoid changing vlan flags

[David Miller]

From: leitao@linux.vnet.ibm.com
Date: Mon, 20 Dec 2010 17:02:37 -0200

> This patch avoids disabling the vlan flags using ethtool.
> 
> Signed-off-by: Breno Leitao <leitao@linux.vnet.ibm.com>

Applied.

Although in the long term, having each and every driver handle this
seems rediculious.