ehea: Fixing LRO configuration

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>

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);
>

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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html