[PATCH] net: bnx2x: convert to hw_features

[PATCH] net: bnx2x: convert to hw_features

[Michał Mirosław]

Since ndo_fix_features callback is postponing features change when
bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
has to be called again when this condition changes.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/net/bnx2x/bnx2x_cmn.c     |   51 ++++++++++++++++++--
 drivers/net/bnx2x/bnx2x_cmn.h     |    3 +
 drivers/net/bnx2x/bnx2x_ethtool.c |   95 -------------------------------------
 drivers/net/bnx2x/bnx2x_main.c    |   29 +++++++-----
 4 files changed, 66 insertions(+), 112 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
index e83ac6d..d09a37c 100644
--- a/drivers/net/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/bnx2x/bnx2x_cmn.c
@@ -2443,11 +2443,21 @@ alloc_err:
 
 }
 
+static int bnx2x_reload_if_running(struct net_device *dev)
+{
+	struct bnx2x *bp = netdev_priv(dev);
+
+	if (unlikely(!netif_running(dev)))
+		return 0;
+
+	bnx2x_nic_unload(bp, UNLOAD_NORMAL);
+	return bnx2x_nic_load(bp, LOAD_NORMAL);
+}
+
 /* called with rtnl_lock */
 int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
 {
 	struct bnx2x *bp = netdev_priv(dev);
-	int rc = 0;
 
 	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
 		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
@@ -2464,12 +2474,43 @@ int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
 	 */
 	dev->mtu = new_mtu;
 
-	if (netif_running(dev)) {
-		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
-		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
+	return bnx2x_reload_if_running(dev);
+}
+
+u32 bnx2x_fix_features(struct net_device *dev, u32 features)
+{
+	struct bnx2x *bp = netdev_priv(dev);
+
+	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
+		netdev_err(dev, "Handling parity error recovery. Try again later\n");
+
+		return dev->features;
+	}
+
+	/* TPA requires Rx CSUM offloading */
+	if (!(features & NETIF_F_RXCSUM) || bp->disable_tpa)
+		features &= ~NETIF_F_LRO;
+
+	return features;
+}
+
+int bnx2x_set_features(struct net_device *dev, u32 features)
+{
+	struct bnx2x *bp = netdev_priv(dev);
+	u32 flags = bp->flags;
+
+	if (features & NETIF_F_LRO)
+		flags |= TPA_ENABLE_FLAG;
+	else
+		flags &= ~TPA_ENABLE_FLAG;
+
+	if (flags ^ bp->flags) {
+		bp->flags = flags;
+
+		return bnx2x_reload_if_running(dev);
 	}
 
-	return rc;
+	return 0;
 }
 
 void bnx2x_tx_timeout(struct net_device *dev)
diff --git a/drivers/net/bnx2x/bnx2x_cmn.h b/drivers/net/bnx2x/bnx2x_cmn.h
index 775fef0..1cdab69 100644
--- a/drivers/net/bnx2x/bnx2x_cmn.h
+++ b/drivers/net/bnx2x/bnx2x_cmn.h
@@ -431,6 +431,9 @@ void bnx2x_free_mem_bp(struct bnx2x *bp);
  */
 int bnx2x_change_mtu(struct net_device *dev, int new_mtu);
 
+u32 bnx2x_fix_features(struct net_device *dev, u32 features);
+int bnx2x_set_features(struct net_device *dev, u32 features);
+
 /**
  * tx timeout netdev callback
  *
diff --git a/drivers/net/bnx2x/bnx2x_ethtool.c b/drivers/net/bnx2x/bnx2x_ethtool.c
index 1479994..ad7d91e 100644
--- a/drivers/net/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/bnx2x/bnx2x_ethtool.c
@@ -1299,91 +1299,6 @@ static int bnx2x_set_pauseparam(struct net_device *dev,
 	return 0;
 }
 
-static int bnx2x_set_flags(struct net_device *dev, u32 data)
-{
-	struct bnx2x *bp = netdev_priv(dev);
-	int changed = 0;
-	int rc = 0;
-
-	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
-		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
-		return -EAGAIN;
-	}
-
-	if (!(data & ETH_FLAG_RXVLAN))
-		return -EINVAL;
-
-	if ((data & ETH_FLAG_LRO) && bp->rx_csum && bp->disable_tpa)
-		return -EINVAL;
-
-	rc = ethtool_op_set_flags(dev, data, ETH_FLAG_LRO | ETH_FLAG_RXVLAN |
-					ETH_FLAG_TXVLAN | ETH_FLAG_RXHASH);
-	if (rc)
-		return rc;
-
-	/* TPA requires Rx CSUM offloading */
-	if ((data & ETH_FLAG_LRO) && bp->rx_csum) {
-		if (!(bp->flags & TPA_ENABLE_FLAG)) {
-			bp->flags |= TPA_ENABLE_FLAG;
-			changed = 1;
-		}
-	} else if (bp->flags & TPA_ENABLE_FLAG) {
-		dev->features &= ~NETIF_F_LRO;
-		bp->flags &= ~TPA_ENABLE_FLAG;
-		changed = 1;
-	}
-
-	if (changed && netif_running(dev)) {
-		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
-		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
-	}
-
-	return rc;
-}
-
-static u32 bnx2x_get_rx_csum(struct net_device *dev)
-{
-	struct bnx2x *bp = netdev_priv(dev);
-
-	return bp->rx_csum;
-}
-
-static int bnx2x_set_rx_csum(struct net_device *dev, u32 data)
-{
-	struct bnx2x *bp = netdev_priv(dev);
-	int rc = 0;
-
-	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
-		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
-		return -EAGAIN;
-	}
-
-	bp->rx_csum = data;
-
-	/* Disable TPA, when Rx CSUM is disabled. Otherwise all
-	   TPA'ed packets will be discarded due to wrong TCP CSUM */
-	if (!data) {
-		u32 flags = ethtool_op_get_flags(dev);
-
-		rc = bnx2x_set_flags(dev, (flags & ~ETH_FLAG_LRO));
-	}
-
-	return rc;
-}
-
-static int bnx2x_set_tso(struct net_device *dev, u32 data)
-{
-	if (data) {
-		dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
-		dev->features |= NETIF_F_TSO6;
-	} else {
-		dev->features &= ~(NETIF_F_TSO | NETIF_F_TSO_ECN);
-		dev->features &= ~NETIF_F_TSO6;
-	}
-
-	return 0;
-}
-
 static const struct {
 	char string[ETH_GSTRING_LEN];
 } bnx2x_tests_str_arr[BNX2X_NUM_TESTS] = {
@@ -2207,16 +2122,6 @@ static const struct ethtool_ops bnx2x_ethtool_ops = {
 	.set_ringparam		= bnx2x_set_ringparam,
 	.get_pauseparam		= bnx2x_get_pauseparam,
 	.set_pauseparam		= bnx2x_set_pauseparam,
-	.get_rx_csum		= bnx2x_get_rx_csum,
-	.set_rx_csum		= bnx2x_set_rx_csum,
-	.get_tx_csum		= ethtool_op_get_tx_csum,
-	.set_tx_csum		= ethtool_op_set_tx_hw_csum,
-	.set_flags		= bnx2x_set_flags,
-	.get_flags		= ethtool_op_get_flags,
-	.get_sg			= ethtool_op_get_sg,
-	.set_sg			= ethtool_op_set_sg,
-	.get_tso		= ethtool_op_get_tso,
-	.set_tso		= bnx2x_set_tso,
 	.self_test		= bnx2x_self_test,
 	.get_sset_count		= bnx2x_get_sset_count,
 	.get_strings		= bnx2x_get_strings,
diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
index f3cf889..ffa0611 100644
--- a/drivers/net/bnx2x/bnx2x_main.c
+++ b/drivers/net/bnx2x/bnx2x_main.c
@@ -7661,6 +7661,7 @@ exit_leader_reset:
 	bp->is_leader = 0;
 	bnx2x_release_hw_lock(bp, HW_LOCK_RESOURCE_RESERVED_08);
 	smp_wmb();
+	netdev_update_features(bp->dev);
 	return rc;
 }
 
@@ -7759,6 +7760,7 @@ static void bnx2x_parity_recover(struct bnx2x *bp)
 					bp->recovery_state =
 						BNX2X_RECOVERY_DONE;
 					smp_wmb();
+					netdev_update_features(bp->dev);
 					return;
 				}
 			}
@@ -8954,6 +8956,7 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
 static int bnx2x_open(struct net_device *dev)
 {
 	struct bnx2x *bp = netdev_priv(dev);
+	int rc;
 
 	netif_carrier_off(dev);
 
@@ -8993,7 +8996,9 @@ static int bnx2x_open(struct net_device *dev)
 
 	bp->recovery_state = BNX2X_RECOVERY_DONE;
 
-	return bnx2x_nic_load(bp, LOAD_OPEN);
+	rc = bnx2x_nic_load(bp, LOAD_OPEN);
+	netdev_update_features(bp->dev);
+	return rc;
 }
 
 /* called with rtnl_lock */
@@ -9304,6 +9309,8 @@ static const struct net_device_ops bnx2x_netdev_ops = {
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_do_ioctl		= bnx2x_ioctl,
 	.ndo_change_mtu		= bnx2x_change_mtu,
+	.ndo_fix_features	= bnx2x_fix_features,
+	.ndo_set_features	= bnx2x_set_features,
 	.ndo_tx_timeout		= bnx2x_tx_timeout,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= poll_bnx2x,
@@ -9430,20 +9437,18 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
 
 	dev->netdev_ops = &bnx2x_netdev_ops;
 	bnx2x_set_ethtool_ops(dev);
-	dev->features |= NETIF_F_SG;
-	dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
+
 	if (bp->flags & USING_DAC_FLAG)
 		dev->features |= NETIF_F_HIGHDMA;
-	dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
-	dev->features |= NETIF_F_TSO6;
-	dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
 
-	dev->vlan_features |= NETIF_F_SG;
-	dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
-	if (bp->flags & USING_DAC_FLAG)
-		dev->vlan_features |= NETIF_F_HIGHDMA;
-	dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
-	dev->vlan_features |= NETIF_F_TSO6;
+	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 |
+		NETIF_F_HW_VLAN_TX;
+
+	dev->features |= dev->hw_features | NETIF_F_HW_VLAN_RX;
+
+	dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_HIGHDMA;
 
 #ifdef BCM_DCBNL
 	dev->dcbnl_ops = &bnx2x_dcbnl_ops;
-- 
1.7.2.5

Re: [PATCH] net: bnx2x: convert to hw_features

[Vladislav Zolotarov]

> +int bnx2x_set_features(struct net_device *dev, u32 features)
> +{
> +	struct bnx2x *bp = netdev_priv(dev);
> +	u32 flags = bp->flags;
> +
> +	if (features & NETIF_F_LRO)
> +		flags |= TPA_ENABLE_FLAG;
> +	else
> +		flags &= ~TPA_ENABLE_FLAG;
> +
> +	if (flags ^ bp->flags) {
> +		bp->flags = flags;
> +
> +		return bnx2x_reload_if_running(dev);
>  	}
>  
> -	return rc;
> +	return 0;
>  }

Thanks a lot, Michal. Looks ok but one remark below.

Every time  bnx2x_reload_if_running(dev) is called one has to check if
there is still a recovery process in progress like u did in the
bnx2x_fix_features(). So this check is missing in the function above.

I'd like to do some unit testing with the new code. I'll let u know
about the results tomorrow.

Thanks again,
vlad 

>  
>  void bnx2x_tx_timeout(struct net_device *dev)
> diff --git a/drivers/net/bnx2x/bnx2x_cmn.h b/drivers/net/bnx2x/bnx2x_cmn.h
> index 775fef0..1cdab69 100644
> --- a/drivers/net/bnx2x/bnx2x_cmn.h
> +++ b/drivers/net/bnx2x/bnx2x_cmn.h
> @@ -431,6 +431,9 @@ void bnx2x_free_mem_bp(struct bnx2x *bp);
>   */
>  int bnx2x_change_mtu(struct net_device *dev, int new_mtu);
>  
> +u32 bnx2x_fix_features(struct net_device *dev, u32 features);
> +int bnx2x_set_features(struct net_device *dev, u32 features);
> +
>  /**
>   * tx timeout netdev callback
>   *
> diff --git a/drivers/net/bnx2x/bnx2x_ethtool.c b/drivers/net/bnx2x/bnx2x_ethtool.c
> index 1479994..ad7d91e 100644
> --- a/drivers/net/bnx2x/bnx2x_ethtool.c
> +++ b/drivers/net/bnx2x/bnx2x_ethtool.c
> @@ -1299,91 +1299,6 @@ static int bnx2x_set_pauseparam(struct net_device *dev,
>  	return 0;
>  }
>  
> -static int bnx2x_set_flags(struct net_device *dev, u32 data)
> -{
> -	struct bnx2x *bp = netdev_priv(dev);
> -	int changed = 0;
> -	int rc = 0;
> -
> -	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
> -		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
> -		return -EAGAIN;
> -	}
> -
> -	if (!(data & ETH_FLAG_RXVLAN))
> -		return -EINVAL;
> -
> -	if ((data & ETH_FLAG_LRO) && bp->rx_csum && bp->disable_tpa)
> -		return -EINVAL;
> -
> -	rc = ethtool_op_set_flags(dev, data, ETH_FLAG_LRO | ETH_FLAG_RXVLAN |
> -					ETH_FLAG_TXVLAN | ETH_FLAG_RXHASH);
> -	if (rc)
> -		return rc;
> -
> -	/* TPA requires Rx CSUM offloading */
> -	if ((data & ETH_FLAG_LRO) && bp->rx_csum) {
> -		if (!(bp->flags & TPA_ENABLE_FLAG)) {
> -			bp->flags |= TPA_ENABLE_FLAG;
> -			changed = 1;
> -		}
> -	} else if (bp->flags & TPA_ENABLE_FLAG) {
> -		dev->features &= ~NETIF_F_LRO;
> -		bp->flags &= ~TPA_ENABLE_FLAG;
> -		changed = 1;
> -	}
> -
> -	if (changed && netif_running(dev)) {
> -		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
> -		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
> -	}
> -
> -	return rc;
> -}
> -
> -static u32 bnx2x_get_rx_csum(struct net_device *dev)
> -{
> -	struct bnx2x *bp = netdev_priv(dev);
> -
> -	return bp->rx_csum;
> -}
> -
> -static int bnx2x_set_rx_csum(struct net_device *dev, u32 data)
> -{
> -	struct bnx2x *bp = netdev_priv(dev);
> -	int rc = 0;
> -
> -	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
> -		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
> -		return -EAGAIN;
> -	}
> -
> -	bp->rx_csum = data;
> -
> -	/* Disable TPA, when Rx CSUM is disabled. Otherwise all
> -	   TPA'ed packets will be discarded due to wrong TCP CSUM */
> -	if (!data) {
> -		u32 flags = ethtool_op_get_flags(dev);
> -
> -		rc = bnx2x_set_flags(dev, (flags & ~ETH_FLAG_LRO));
> -	}
> -
> -	return rc;
> -}
> -
> -static int bnx2x_set_tso(struct net_device *dev, u32 data)
> -{
> -	if (data) {
> -		dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> -		dev->features |= NETIF_F_TSO6;
> -	} else {
> -		dev->features &= ~(NETIF_F_TSO | NETIF_F_TSO_ECN);
> -		dev->features &= ~NETIF_F_TSO6;
> -	}
> -
> -	return 0;
> -}
> -
>  static const struct {
>  	char string[ETH_GSTRING_LEN];
>  } bnx2x_tests_str_arr[BNX2X_NUM_TESTS] = {
> @@ -2207,16 +2122,6 @@ static const struct ethtool_ops bnx2x_ethtool_ops = {
>  	.set_ringparam		= bnx2x_set_ringparam,
>  	.get_pauseparam		= bnx2x_get_pauseparam,
>  	.set_pauseparam		= bnx2x_set_pauseparam,
> -	.get_rx_csum		= bnx2x_get_rx_csum,
> -	.set_rx_csum		= bnx2x_set_rx_csum,
> -	.get_tx_csum		= ethtool_op_get_tx_csum,
> -	.set_tx_csum		= ethtool_op_set_tx_hw_csum,
> -	.set_flags		= bnx2x_set_flags,
> -	.get_flags		= ethtool_op_get_flags,
> -	.get_sg			= ethtool_op_get_sg,
> -	.set_sg			= ethtool_op_set_sg,
> -	.get_tso		= ethtool_op_get_tso,
> -	.set_tso		= bnx2x_set_tso,
>  	.self_test		= bnx2x_self_test,
>  	.get_sset_count		= bnx2x_get_sset_count,
>  	.get_strings		= bnx2x_get_strings,
> diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
> index f3cf889..ffa0611 100644
> --- a/drivers/net/bnx2x/bnx2x_main.c
> +++ b/drivers/net/bnx2x/bnx2x_main.c
> @@ -7661,6 +7661,7 @@ exit_leader_reset:
>  	bp->is_leader = 0;
>  	bnx2x_release_hw_lock(bp, HW_LOCK_RESOURCE_RESERVED_08);
>  	smp_wmb();
> +	netdev_update_features(bp->dev);
>  	return rc;
>  }
>  
> @@ -7759,6 +7760,7 @@ static void bnx2x_parity_recover(struct bnx2x *bp)
>  					bp->recovery_state =
>  						BNX2X_RECOVERY_DONE;
>  					smp_wmb();
> +					netdev_update_features(bp->dev);
>  					return;
>  				}
>  			}
> @@ -8954,6 +8956,7 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
>  static int bnx2x_open(struct net_device *dev)
>  {
>  	struct bnx2x *bp = netdev_priv(dev);
> +	int rc;
>  
>  	netif_carrier_off(dev);
>  
> @@ -8993,7 +8996,9 @@ static int bnx2x_open(struct net_device *dev)
>  
>  	bp->recovery_state = BNX2X_RECOVERY_DONE;
>  
> -	return bnx2x_nic_load(bp, LOAD_OPEN);
> +	rc = bnx2x_nic_load(bp, LOAD_OPEN);
> +	netdev_update_features(bp->dev);
> +	return rc;
>  }
>  
>  /* called with rtnl_lock */
> @@ -9304,6 +9309,8 @@ static const struct net_device_ops bnx2x_netdev_ops = {
>  	.ndo_validate_addr	= eth_validate_addr,
>  	.ndo_do_ioctl		= bnx2x_ioctl,
>  	.ndo_change_mtu		= bnx2x_change_mtu,
> +	.ndo_fix_features	= bnx2x_fix_features,
> +	.ndo_set_features	= bnx2x_set_features,
>  	.ndo_tx_timeout		= bnx2x_tx_timeout,
>  #ifdef CONFIG_NET_POLL_CONTROLLER
>  	.ndo_poll_controller	= poll_bnx2x,
> @@ -9430,20 +9437,18 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
>  
>  	dev->netdev_ops = &bnx2x_netdev_ops;
>  	bnx2x_set_ethtool_ops(dev);
> -	dev->features |= NETIF_F_SG;
> -	dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> +
>  	if (bp->flags & USING_DAC_FLAG)
>  		dev->features |= NETIF_F_HIGHDMA;
> -	dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> -	dev->features |= NETIF_F_TSO6;
> -	dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
>  
> -	dev->vlan_features |= NETIF_F_SG;
> -	dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> -	if (bp->flags & USING_DAC_FLAG)
> -		dev->vlan_features |= NETIF_F_HIGHDMA;
> -	dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> -	dev->vlan_features |= NETIF_F_TSO6;
> +	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> +		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 |
> +		NETIF_F_HW_VLAN_TX;
> +
> +	dev->features |= dev->hw_features | NETIF_F_HW_VLAN_RX;
> +
> +	dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> +		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_HIGHDMA;
>  
>  #ifdef BCM_DCBNL
>  	dev->dcbnl_ops = &bnx2x_dcbnl_ops;

Re: [PATCH] net: bnx2x: convert to hw_features

[Michał Mirosław]

On Sun, Apr 10, 2011 at 06:10:14PM +0300, Vladislav Zolotarov wrote:
> > +int bnx2x_set_features(struct net_device *dev, u32 features)
> > +{
> > +	struct bnx2x *bp = netdev_priv(dev);
> > +	u32 flags = bp->flags;
> > +
> > +	if (features & NETIF_F_LRO)
> > +		flags |= TPA_ENABLE_FLAG;
> > +	else
> > +		flags &= ~TPA_ENABLE_FLAG;
> > +
> > +	if (flags ^ bp->flags) {
> > +		bp->flags = flags;
> > +
> > +		return bnx2x_reload_if_running(dev);
> >  	}
> >  
> > -	return rc;
> > +	return 0;
> >  }
> 
> Thanks a lot, Michal. Looks ok but one remark below.
> 
> Every time  bnx2x_reload_if_running(dev) is called one has to check if
> there is still a recovery process in progress like u did in the
> bnx2x_fix_features(). So this check is missing in the function above.

bnx2x_set_features() might be called only if bnx2x_fix_features() returns
different feature set than what's already set in dev->features. So this
case is implicitly covered. I'll add a comment for this.

Best Regards,
Michał Mirosław

[PATCH v2] net: bnx2x: convert to hw_features

[Michał Mirosław]

Since ndo_fix_features callback is postponing features change when
bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
has to be called again when this condition changes.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
[changes from v1: comment in ndo_fix_features callback]

 drivers/net/bnx2x/bnx2x_cmn.c     |   52 ++++++++++++++++++--
 drivers/net/bnx2x/bnx2x_cmn.h     |    3 +
 drivers/net/bnx2x/bnx2x_ethtool.c |   95 -------------------------------------
 drivers/net/bnx2x/bnx2x_main.c    |   29 +++++++-----
 4 files changed, 67 insertions(+), 112 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
index e83ac6d..9691b67 100644
--- a/drivers/net/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/bnx2x/bnx2x_cmn.c
@@ -2443,11 +2443,21 @@ alloc_err:
 
 }
 
+static int bnx2x_reload_if_running(struct net_device *dev)
+{
+	struct bnx2x *bp = netdev_priv(dev);
+
+	if (unlikely(!netif_running(dev)))
+		return 0;
+
+	bnx2x_nic_unload(bp, UNLOAD_NORMAL);
+	return bnx2x_nic_load(bp, LOAD_NORMAL);
+}
+
 /* called with rtnl_lock */
 int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
 {
 	struct bnx2x *bp = netdev_priv(dev);
-	int rc = 0;
 
 	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
 		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
@@ -2464,12 +2474,44 @@ int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
 	 */
 	dev->mtu = new_mtu;
 
-	if (netif_running(dev)) {
-		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
-		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
+	return bnx2x_reload_if_running(dev);
+}
+
+u32 bnx2x_fix_features(struct net_device *dev, u32 features)
+{
+	struct bnx2x *bp = netdev_priv(dev);
+
+	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
+		netdev_err(dev, "Handling parity error recovery. Try again later\n");
+
+		/* Don't allow bnx2x_set_features() to be called now. */
+		return dev->features;
+	}
+
+	/* TPA requires Rx CSUM offloading */
+	if (!(features & NETIF_F_RXCSUM) || bp->disable_tpa)
+		features &= ~NETIF_F_LRO;
+
+	return features;
+}
+
+int bnx2x_set_features(struct net_device *dev, u32 features)
+{
+	struct bnx2x *bp = netdev_priv(dev);
+	u32 flags = bp->flags;
+
+	if (features & NETIF_F_LRO)
+		flags |= TPA_ENABLE_FLAG;
+	else
+		flags &= ~TPA_ENABLE_FLAG;
+
+	if (flags ^ bp->flags) {
+		bp->flags = flags;
+
+		return bnx2x_reload_if_running(dev);
 	}
 
-	return rc;
+	return 0;
 }
 
 void bnx2x_tx_timeout(struct net_device *dev)
diff --git a/drivers/net/bnx2x/bnx2x_cmn.h b/drivers/net/bnx2x/bnx2x_cmn.h
index 775fef0..1cdab69 100644
--- a/drivers/net/bnx2x/bnx2x_cmn.h
+++ b/drivers/net/bnx2x/bnx2x_cmn.h
@@ -431,6 +431,9 @@ void bnx2x_free_mem_bp(struct bnx2x *bp);
  */
 int bnx2x_change_mtu(struct net_device *dev, int new_mtu);
 
+u32 bnx2x_fix_features(struct net_device *dev, u32 features);
+int bnx2x_set_features(struct net_device *dev, u32 features);
+
 /**
  * tx timeout netdev callback
  *
diff --git a/drivers/net/bnx2x/bnx2x_ethtool.c b/drivers/net/bnx2x/bnx2x_ethtool.c
index 1479994..ad7d91e 100644
--- a/drivers/net/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/bnx2x/bnx2x_ethtool.c
@@ -1299,91 +1299,6 @@ static int bnx2x_set_pauseparam(struct net_device *dev,
 	return 0;
 }
 
-static int bnx2x_set_flags(struct net_device *dev, u32 data)
-{
-	struct bnx2x *bp = netdev_priv(dev);
-	int changed = 0;
-	int rc = 0;
-
-	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
-		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
-		return -EAGAIN;
-	}
-
-	if (!(data & ETH_FLAG_RXVLAN))
-		return -EINVAL;
-
-	if ((data & ETH_FLAG_LRO) && bp->rx_csum && bp->disable_tpa)
-		return -EINVAL;
-
-	rc = ethtool_op_set_flags(dev, data, ETH_FLAG_LRO | ETH_FLAG_RXVLAN |
-					ETH_FLAG_TXVLAN | ETH_FLAG_RXHASH);
-	if (rc)
-		return rc;
-
-	/* TPA requires Rx CSUM offloading */
-	if ((data & ETH_FLAG_LRO) && bp->rx_csum) {
-		if (!(bp->flags & TPA_ENABLE_FLAG)) {
-			bp->flags |= TPA_ENABLE_FLAG;
-			changed = 1;
-		}
-	} else if (bp->flags & TPA_ENABLE_FLAG) {
-		dev->features &= ~NETIF_F_LRO;
-		bp->flags &= ~TPA_ENABLE_FLAG;
-		changed = 1;
-	}
-
-	if (changed && netif_running(dev)) {
-		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
-		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
-	}
-
-	return rc;
-}
-
-static u32 bnx2x_get_rx_csum(struct net_device *dev)
-{
-	struct bnx2x *bp = netdev_priv(dev);
-
-	return bp->rx_csum;
-}
-
-static int bnx2x_set_rx_csum(struct net_device *dev, u32 data)
-{
-	struct bnx2x *bp = netdev_priv(dev);
-	int rc = 0;
-
-	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
-		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
-		return -EAGAIN;
-	}
-
-	bp->rx_csum = data;
-
-	/* Disable TPA, when Rx CSUM is disabled. Otherwise all
-	   TPA'ed packets will be discarded due to wrong TCP CSUM */
-	if (!data) {
-		u32 flags = ethtool_op_get_flags(dev);
-
-		rc = bnx2x_set_flags(dev, (flags & ~ETH_FLAG_LRO));
-	}
-
-	return rc;
-}
-
-static int bnx2x_set_tso(struct net_device *dev, u32 data)
-{
-	if (data) {
-		dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
-		dev->features |= NETIF_F_TSO6;
-	} else {
-		dev->features &= ~(NETIF_F_TSO | NETIF_F_TSO_ECN);
-		dev->features &= ~NETIF_F_TSO6;
-	}
-
-	return 0;
-}
-
 static const struct {
 	char string[ETH_GSTRING_LEN];
 } bnx2x_tests_str_arr[BNX2X_NUM_TESTS] = {
@@ -2207,16 +2122,6 @@ static const struct ethtool_ops bnx2x_ethtool_ops = {
 	.set_ringparam		= bnx2x_set_ringparam,
 	.get_pauseparam		= bnx2x_get_pauseparam,
 	.set_pauseparam		= bnx2x_set_pauseparam,
-	.get_rx_csum		= bnx2x_get_rx_csum,
-	.set_rx_csum		= bnx2x_set_rx_csum,
-	.get_tx_csum		= ethtool_op_get_tx_csum,
-	.set_tx_csum		= ethtool_op_set_tx_hw_csum,
-	.set_flags		= bnx2x_set_flags,
-	.get_flags		= ethtool_op_get_flags,
-	.get_sg			= ethtool_op_get_sg,
-	.set_sg			= ethtool_op_set_sg,
-	.get_tso		= ethtool_op_get_tso,
-	.set_tso		= bnx2x_set_tso,
 	.self_test		= bnx2x_self_test,
 	.get_sset_count		= bnx2x_get_sset_count,
 	.get_strings		= bnx2x_get_strings,
diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
index f3cf889..ffa0611 100644
--- a/drivers/net/bnx2x/bnx2x_main.c
+++ b/drivers/net/bnx2x/bnx2x_main.c
@@ -7661,6 +7661,7 @@ exit_leader_reset:
 	bp->is_leader = 0;
 	bnx2x_release_hw_lock(bp, HW_LOCK_RESOURCE_RESERVED_08);
 	smp_wmb();
+	netdev_update_features(bp->dev);
 	return rc;
 }
 
@@ -7759,6 +7760,7 @@ static void bnx2x_parity_recover(struct bnx2x *bp)
 					bp->recovery_state =
 						BNX2X_RECOVERY_DONE;
 					smp_wmb();
+					netdev_update_features(bp->dev);
 					return;
 				}
 			}
@@ -8954,6 +8956,7 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
 static int bnx2x_open(struct net_device *dev)
 {
 	struct bnx2x *bp = netdev_priv(dev);
+	int rc;
 
 	netif_carrier_off(dev);
 
@@ -8993,7 +8996,9 @@ static int bnx2x_open(struct net_device *dev)
 
 	bp->recovery_state = BNX2X_RECOVERY_DONE;
 
-	return bnx2x_nic_load(bp, LOAD_OPEN);
+	rc = bnx2x_nic_load(bp, LOAD_OPEN);
+	netdev_update_features(bp->dev);
+	return rc;
 }
 
 /* called with rtnl_lock */
@@ -9304,6 +9309,8 @@ static const struct net_device_ops bnx2x_netdev_ops = {
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_do_ioctl		= bnx2x_ioctl,
 	.ndo_change_mtu		= bnx2x_change_mtu,
+	.ndo_fix_features	= bnx2x_fix_features,
+	.ndo_set_features	= bnx2x_set_features,
 	.ndo_tx_timeout		= bnx2x_tx_timeout,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= poll_bnx2x,
@@ -9430,20 +9437,18 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
 
 	dev->netdev_ops = &bnx2x_netdev_ops;
 	bnx2x_set_ethtool_ops(dev);
-	dev->features |= NETIF_F_SG;
-	dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
+
 	if (bp->flags & USING_DAC_FLAG)
 		dev->features |= NETIF_F_HIGHDMA;
-	dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
-	dev->features |= NETIF_F_TSO6;
-	dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
 
-	dev->vlan_features |= NETIF_F_SG;
-	dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
-	if (bp->flags & USING_DAC_FLAG)
-		dev->vlan_features |= NETIF_F_HIGHDMA;
-	dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
-	dev->vlan_features |= NETIF_F_TSO6;
+	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 |
+		NETIF_F_HW_VLAN_TX;
+
+	dev->features |= dev->hw_features | NETIF_F_HW_VLAN_RX;
+
+	dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_HIGHDMA;
 
 #ifdef BCM_DCBNL
 	dev->dcbnl_ops = &bnx2x_dcbnl_ops;
-- 
1.7.2.5

Re: [PATCH v2] net: bnx2x: convert to hw_features

[Vladislav Zolotarov]

On Sun, 2011-04-10 at 08:35 -0700, Michał Mirosław wrote:
> Since ndo_fix_features callback is postponing features change when
> bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
> has to be called again when this condition changes.

Unfortunately, NACK again. See below, pls.

> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> [changes from v1: comment in ndo_fix_features callback]
> 
>  drivers/net/bnx2x/bnx2x_cmn.c     |   52 ++++++++++++++++++--
>  drivers/net/bnx2x/bnx2x_cmn.h     |    3 +
>  drivers/net/bnx2x/bnx2x_ethtool.c |   95 -------------------------------------
>  drivers/net/bnx2x/bnx2x_main.c    |   29 +++++++-----
>  4 files changed, 67 insertions(+), 112 deletions(-)
> 
> diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
> index e83ac6d..9691b67 100644
> --- a/drivers/net/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/bnx2x/bnx2x_cmn.c
> @@ -2443,11 +2443,21 @@ alloc_err:
>  
>  }
>  
> +static int bnx2x_reload_if_running(struct net_device *dev)
> +{
> +	struct bnx2x *bp = netdev_priv(dev);
> +
> +	if (unlikely(!netif_running(dev)))
> +		return 0;
> +
> +	bnx2x_nic_unload(bp, UNLOAD_NORMAL);
> +	return bnx2x_nic_load(bp, LOAD_NORMAL);
> +}
> +
>  /* called with rtnl_lock */
>  int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
>  {
>  	struct bnx2x *bp = netdev_priv(dev);
> -	int rc = 0;
>  
>  	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
>  		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
> @@ -2464,12 +2474,44 @@ int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
>  	 */
>  	dev->mtu = new_mtu;
>  
> -	if (netif_running(dev)) {
> -		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
> -		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
> +	return bnx2x_reload_if_running(dev);
> +}
> +
> +u32 bnx2x_fix_features(struct net_device *dev, u32 features)
> +{
> +	struct bnx2x *bp = netdev_priv(dev);
> +
> +	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
> +		netdev_err(dev, "Handling parity error recovery. Try again later\n");
> +
> +		/* Don't allow bnx2x_set_features() to be called now. */
> +		return dev->features;
> +	}
> +
> +	/* TPA requires Rx CSUM offloading */
> +	if (!(features & NETIF_F_RXCSUM) || bp->disable_tpa)
> +		features &= ~NETIF_F_LRO;

Shouldn't it be (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM) and not
NETIF_F_RXCSUM?

> +
> +	return features;
> +}

In addition this function should ensure NETIF_F_IP_CSUM and
NETIF_F_IPV6_CSUM are changed together.

> +
> +int bnx2x_set_features(struct net_device *dev, u32 features)
> +{
> +	struct bnx2x *bp = netdev_priv(dev);
> +	u32 flags = bp->flags;
> +
> +	if (features & NETIF_F_LRO)
> +		flags |= TPA_ENABLE_FLAG;
> +	else
> +		flags &= ~TPA_ENABLE_FLAG;
> +
> +	if (flags ^ bp->flags) {
> +		bp->flags = flags;
> +
> +		return bnx2x_reload_if_running(dev);
>  	}
>  
> -	return rc;
> +	return 0;
>  }

Since there is no set_rx_csum() anymore the above function has to handle
bp->rx_csum namely correlate it with (NETIF_F_IP_CSUM |
NETIF_F_IPV6_CSUM) bits in the 'features'. 

>  
>  void bnx2x_tx_timeout(struct net_device *dev)
> diff --git a/drivers/net/bnx2x/bnx2x_cmn.h b/drivers/net/bnx2x/bnx2x_cmn.h
> index 775fef0..1cdab69 100644
> --- a/drivers/net/bnx2x/bnx2x_cmn.h
> +++ b/drivers/net/bnx2x/bnx2x_cmn.h
> @@ -431,6 +431,9 @@ void bnx2x_free_mem_bp(struct bnx2x *bp);
>   */
>  int bnx2x_change_mtu(struct net_device *dev, int new_mtu);
>  
> +u32 bnx2x_fix_features(struct net_device *dev, u32 features);
> +int bnx2x_set_features(struct net_device *dev, u32 features);
> +
>  /**
>   * tx timeout netdev callback
>   *
> diff --git a/drivers/net/bnx2x/bnx2x_ethtool.c b/drivers/net/bnx2x/bnx2x_ethtool.c
> index 1479994..ad7d91e 100644
> --- a/drivers/net/bnx2x/bnx2x_ethtool.c
> +++ b/drivers/net/bnx2x/bnx2x_ethtool.c
> @@ -1299,91 +1299,6 @@ static int bnx2x_set_pauseparam(struct net_device *dev,
>  	return 0;
>  }
>  
> -static int bnx2x_set_flags(struct net_device *dev, u32 data)
> -{
> -	struct bnx2x *bp = netdev_priv(dev);
> -	int changed = 0;
> -	int rc = 0;
> -
> -	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
> -		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
> -		return -EAGAIN;
> -	}
> -
> -	if (!(data & ETH_FLAG_RXVLAN))
> -		return -EINVAL;
> -
> -	if ((data & ETH_FLAG_LRO) && bp->rx_csum && bp->disable_tpa)
> -		return -EINVAL;
> -
> -	rc = ethtool_op_set_flags(dev, data, ETH_FLAG_LRO | ETH_FLAG_RXVLAN |
> -					ETH_FLAG_TXVLAN | ETH_FLAG_RXHASH);
> -	if (rc)
> -		return rc;
> -
> -	/* TPA requires Rx CSUM offloading */
> -	if ((data & ETH_FLAG_LRO) && bp->rx_csum) {
> -		if (!(bp->flags & TPA_ENABLE_FLAG)) {
> -			bp->flags |= TPA_ENABLE_FLAG;
> -			changed = 1;
> -		}
> -	} else if (bp->flags & TPA_ENABLE_FLAG) {
> -		dev->features &= ~NETIF_F_LRO;
> -		bp->flags &= ~TPA_ENABLE_FLAG;
> -		changed = 1;
> -	}
> -
> -	if (changed && netif_running(dev)) {
> -		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
> -		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
> -	}
> -
> -	return rc;
> -}
> -
> -static u32 bnx2x_get_rx_csum(struct net_device *dev)
> -{
> -	struct bnx2x *bp = netdev_priv(dev);
> -
> -	return bp->rx_csum;
> -}
> -
> -static int bnx2x_set_rx_csum(struct net_device *dev, u32 data)
> -{
> -	struct bnx2x *bp = netdev_priv(dev);
> -	int rc = 0;
> -
> -	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
> -		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
> -		return -EAGAIN;
> -	}
> -
> -	bp->rx_csum = data;
> -
> -	/* Disable TPA, when Rx CSUM is disabled. Otherwise all
> -	   TPA'ed packets will be discarded due to wrong TCP CSUM */
> -	if (!data) {
> -		u32 flags = ethtool_op_get_flags(dev);
> -
> -		rc = bnx2x_set_flags(dev, (flags & ~ETH_FLAG_LRO));
> -	}
> -
> -	return rc;
> -}
> -
> -static int bnx2x_set_tso(struct net_device *dev, u32 data)
> -{
> -	if (data) {
> -		dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> -		dev->features |= NETIF_F_TSO6;
> -	} else {
> -		dev->features &= ~(NETIF_F_TSO | NETIF_F_TSO_ECN);
> -		dev->features &= ~NETIF_F_TSO6;
> -	}
> -
> -	return 0;
> -}
> -
>  static const struct {
>  	char string[ETH_GSTRING_LEN];
>  } bnx2x_tests_str_arr[BNX2X_NUM_TESTS] = {
> @@ -2207,16 +2122,6 @@ static const struct ethtool_ops bnx2x_ethtool_ops = {
>  	.set_ringparam		= bnx2x_set_ringparam,
>  	.get_pauseparam		= bnx2x_get_pauseparam,
>  	.set_pauseparam		= bnx2x_set_pauseparam,
> -	.get_rx_csum		= bnx2x_get_rx_csum,
> -	.set_rx_csum		= bnx2x_set_rx_csum,
> -	.get_tx_csum		= ethtool_op_get_tx_csum,
> -	.set_tx_csum		= ethtool_op_set_tx_hw_csum,
> -	.set_flags		= bnx2x_set_flags,
> -	.get_flags		= ethtool_op_get_flags,
> -	.get_sg			= ethtool_op_get_sg,
> -	.set_sg			= ethtool_op_set_sg,
> -	.get_tso		= ethtool_op_get_tso,
> -	.set_tso		= bnx2x_set_tso,
>  	.self_test		= bnx2x_self_test,
>  	.get_sset_count		= bnx2x_get_sset_count,
>  	.get_strings		= bnx2x_get_strings,
> diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
> index f3cf889..ffa0611 100644
> --- a/drivers/net/bnx2x/bnx2x_main.c
> +++ b/drivers/net/bnx2x/bnx2x_main.c
> @@ -7661,6 +7661,7 @@ exit_leader_reset:
>  	bp->is_leader = 0;
>  	bnx2x_release_hw_lock(bp, HW_LOCK_RESOURCE_RESERVED_08);
>  	smp_wmb();
> +	netdev_update_features(bp->dev);
>  	return rc;
>  }

Before I continue I'd like to clarify one thing: there is no sense to
call for netdev_update_features() if bnx2x_nic_load(), called right
before it, has failed as long as the following bnx2x_nic_load() that
will be called from the netdev_update_features() flow will also fail
(for the same reasons as the previous one). If bnx2x_nic_load() fails
for the certain NIC we actually shut this NIC down. So, the following
remarks will be based on the above statement.

netdev_update_features(bp->dev) should be called after a successful
"leader"'s call for a bnx2x_nic_load() and not as above. See the patch
below:

diff --git a/drivers/net/bnx2x/bnx2x_main.c
b/drivers/net/bnx2x/bnx2x_main.c
index f3cf889..71e7818 100644
--- a/drivers/net/bnx2x/bnx2x_main.c
+++ b/drivers/net/bnx2x/bnx2x_main.c
@@ -7728,6 +7728,8 @@ static void bnx2x_parity_recover(struct bnx2x *bp)
 						return;
 					}
 
+					netdev_update_features(bp->dev);
+
					return;
 				}
 
>  
> @@ -7759,6 +7760,7 @@ static void bnx2x_parity_recover(struct bnx2x *bp)
>  					bp->recovery_state =
>  						BNX2X_RECOVERY_DONE;
>  					smp_wmb();
> +					netdev_update_features(bp->dev);
>  					return;
>  				}
>  			}

Same here: netdev_update_features() should be called only if the
previous bnx2x_nic_load() call has succeeded.

> @@ -8954,6 +8956,7 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
>  static int bnx2x_open(struct net_device *dev)
>  {
>  	struct bnx2x *bp = netdev_priv(dev);
> +	int rc;
>  
>  	netif_carrier_off(dev);
>  
> @@ -8993,7 +8996,9 @@ static int bnx2x_open(struct net_device *dev)
>  
>  	bp->recovery_state = BNX2X_RECOVERY_DONE;
>  
> -	return bnx2x_nic_load(bp, LOAD_OPEN);
> +	rc = bnx2x_nic_load(bp, LOAD_OPEN);
> +	netdev_update_features(bp->dev);
> +	return rc;
>  }

U shouldn't call for netdev_update_features(bp->dev) if bnx2x_nic_load()
has failed. It would also be nice if netdev_update_features() would
propagate the exit status of ndo_set_features() when ndo_set_features()
fails in the __netdev_update_features(). See the patch for the bnx2x
below:

@@ -8993,7 +8995,14 @@ static int bnx2x_open(struct net_device *dev)
 
        bp->recovery_state = BNX2X_RECOVERY_DONE;
 
-       return bnx2x_nic_load(bp, LOAD_OPEN);
+       rc = bnx2x_nic_load(bp, LOAD_OPEN);
+       if (!rc)
+               netdev_update_features(bp->dev);
+
+       if (bp->state == BNX2X_STATE_OPEN)
+               return 0;
+       else
+               return -EBUSY;
 }
 
 /* called with rtnl_lock */
>  
>  /* called with rtnl_lock */
> @@ -9304,6 +9309,8 @@ static const struct net_device_ops bnx2x_netdev_ops = {
>  	.ndo_validate_addr	= eth_validate_addr,
>  	.ndo_do_ioctl		= bnx2x_ioctl,
>  	.ndo_change_mtu		= bnx2x_change_mtu,
> +	.ndo_fix_features	= bnx2x_fix_features,
> +	.ndo_set_features	= bnx2x_set_features,
>  	.ndo_tx_timeout		= bnx2x_tx_timeout,
>  #ifdef CONFIG_NET_POLL_CONTROLLER
>  	.ndo_poll_controller	= poll_bnx2x,
> @@ -9430,20 +9437,18 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
>  
>  	dev->netdev_ops = &bnx2x_netdev_ops;
>  	bnx2x_set_ethtool_ops(dev);
> -	dev->features |= NETIF_F_SG;
> -	dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> +
>  	if (bp->flags & USING_DAC_FLAG)
>  		dev->features |= NETIF_F_HIGHDMA;
> -	dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> -	dev->features |= NETIF_F_TSO6;
> -	dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
>  
> -	dev->vlan_features |= NETIF_F_SG;
> -	dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> -	if (bp->flags & USING_DAC_FLAG)
> -		dev->vlan_features |= NETIF_F_HIGHDMA;
> -	dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> -	dev->vlan_features |= NETIF_F_TSO6;
> +	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> +		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 |
> +		NETIF_F_HW_VLAN_TX;

hw_features are missing NETIF_F_GRO and NETIF_F_LRO flags that are
currently configured in bnx2x_init_bp(). 

> +
> +	dev->features |= dev->hw_features | NETIF_F_HW_VLAN_RX;
> +
> +	dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> +		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_HIGHDMA;

I'm not sure if it's safe to set NETIF_F_HIGHDMA unconditionally. I
think it's better to correlate it with the USING_DAC_FLAG which is set
according to what is returned by 
dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)).

thanks,
vlad

>  
>  #ifdef BCM_DCBNL
>  	dev->dcbnl_ops = &bnx2x_dcbnl_ops;

[PATCH v3] net: bnx2x: convert to hw_features

[Michał Mirosław]

Since ndo_fix_features callback is postponing features change when
bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
has to be called again when this condition changes. Previously,
ethtool_ops->set_flags callback returned -EBUSY in that case
(it's not possible in the new model).

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
v3: - include NETIF_F_LRO in hw_features
    - don't call netdev_update_features() if bnx2x_nic_load() failed
v2: - comment in ndo_fix_features callback

 drivers/net/bnx2x/bnx2x_cmn.c     |   52 ++++++++++++++++++--
 drivers/net/bnx2x/bnx2x_cmn.h     |    3 +
 drivers/net/bnx2x/bnx2x_ethtool.c |   95 -------------------------------------
 drivers/net/bnx2x/bnx2x_main.c    |   35 ++++++++------
 4 files changed, 70 insertions(+), 115 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
index e83ac6d..9691b67 100644
--- a/drivers/net/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/bnx2x/bnx2x_cmn.c
@@ -2443,11 +2443,21 @@ alloc_err:
 
 }
 
+static int bnx2x_reload_if_running(struct net_device *dev)
+{
+	struct bnx2x *bp = netdev_priv(dev);
+
+	if (unlikely(!netif_running(dev)))
+		return 0;
+
+	bnx2x_nic_unload(bp, UNLOAD_NORMAL);
+	return bnx2x_nic_load(bp, LOAD_NORMAL);
+}
+
 /* called with rtnl_lock */
 int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
 {
 	struct bnx2x *bp = netdev_priv(dev);
-	int rc = 0;
 
 	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
 		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
@@ -2464,12 +2474,44 @@ int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
 	 */
 	dev->mtu = new_mtu;
 
-	if (netif_running(dev)) {
-		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
-		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
+	return bnx2x_reload_if_running(dev);
+}
+
+u32 bnx2x_fix_features(struct net_device *dev, u32 features)
+{
+	struct bnx2x *bp = netdev_priv(dev);
+
+	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
+		netdev_err(dev, "Handling parity error recovery. Try again later\n");
+
+		/* Don't allow bnx2x_set_features() to be called now. */
+		return dev->features;
+	}
+
+	/* TPA requires Rx CSUM offloading */
+	if (!(features & NETIF_F_RXCSUM) || bp->disable_tpa)
+		features &= ~NETIF_F_LRO;
+
+	return features;
+}
+
+int bnx2x_set_features(struct net_device *dev, u32 features)
+{
+	struct bnx2x *bp = netdev_priv(dev);
+	u32 flags = bp->flags;
+
+	if (features & NETIF_F_LRO)
+		flags |= TPA_ENABLE_FLAG;
+	else
+		flags &= ~TPA_ENABLE_FLAG;
+
+	if (flags ^ bp->flags) {
+		bp->flags = flags;
+
+		return bnx2x_reload_if_running(dev);
 	}
 
-	return rc;
+	return 0;
 }
 
 void bnx2x_tx_timeout(struct net_device *dev)
diff --git a/drivers/net/bnx2x/bnx2x_cmn.h b/drivers/net/bnx2x/bnx2x_cmn.h
index 775fef0..1cdab69 100644
--- a/drivers/net/bnx2x/bnx2x_cmn.h
+++ b/drivers/net/bnx2x/bnx2x_cmn.h
@@ -431,6 +431,9 @@ void bnx2x_free_mem_bp(struct bnx2x *bp);
  */
 int bnx2x_change_mtu(struct net_device *dev, int new_mtu);
 
+u32 bnx2x_fix_features(struct net_device *dev, u32 features);
+int bnx2x_set_features(struct net_device *dev, u32 features);
+
 /**
  * tx timeout netdev callback
  *
diff --git a/drivers/net/bnx2x/bnx2x_ethtool.c b/drivers/net/bnx2x/bnx2x_ethtool.c
index 1479994..ad7d91e 100644
--- a/drivers/net/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/bnx2x/bnx2x_ethtool.c
@@ -1299,91 +1299,6 @@ static int bnx2x_set_pauseparam(struct net_device *dev,
 	return 0;
 }
 
-static int bnx2x_set_flags(struct net_device *dev, u32 data)
-{
-	struct bnx2x *bp = netdev_priv(dev);
-	int changed = 0;
-	int rc = 0;
-
-	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
-		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
-		return -EAGAIN;
-	}
-
-	if (!(data & ETH_FLAG_RXVLAN))
-		return -EINVAL;
-
-	if ((data & ETH_FLAG_LRO) && bp->rx_csum && bp->disable_tpa)
-		return -EINVAL;
-
-	rc = ethtool_op_set_flags(dev, data, ETH_FLAG_LRO | ETH_FLAG_RXVLAN |
-					ETH_FLAG_TXVLAN | ETH_FLAG_RXHASH);
-	if (rc)
-		return rc;
-
-	/* TPA requires Rx CSUM offloading */
-	if ((data & ETH_FLAG_LRO) && bp->rx_csum) {
-		if (!(bp->flags & TPA_ENABLE_FLAG)) {
-			bp->flags |= TPA_ENABLE_FLAG;
-			changed = 1;
-		}
-	} else if (bp->flags & TPA_ENABLE_FLAG) {
-		dev->features &= ~NETIF_F_LRO;
-		bp->flags &= ~TPA_ENABLE_FLAG;
-		changed = 1;
-	}
-
-	if (changed && netif_running(dev)) {
-		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
-		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
-	}
-
-	return rc;
-}
-
-static u32 bnx2x_get_rx_csum(struct net_device *dev)
-{
-	struct bnx2x *bp = netdev_priv(dev);
-
-	return bp->rx_csum;
-}
-
-static int bnx2x_set_rx_csum(struct net_device *dev, u32 data)
-{
-	struct bnx2x *bp = netdev_priv(dev);
-	int rc = 0;
-
-	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
-		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
-		return -EAGAIN;
-	}
-
-	bp->rx_csum = data;
-
-	/* Disable TPA, when Rx CSUM is disabled. Otherwise all
-	   TPA'ed packets will be discarded due to wrong TCP CSUM */
-	if (!data) {
-		u32 flags = ethtool_op_get_flags(dev);
-
-		rc = bnx2x_set_flags(dev, (flags & ~ETH_FLAG_LRO));
-	}
-
-	return rc;
-}
-
-static int bnx2x_set_tso(struct net_device *dev, u32 data)
-{
-	if (data) {
-		dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
-		dev->features |= NETIF_F_TSO6;
-	} else {
-		dev->features &= ~(NETIF_F_TSO | NETIF_F_TSO_ECN);
-		dev->features &= ~NETIF_F_TSO6;
-	}
-
-	return 0;
-}
-
 static const struct {
 	char string[ETH_GSTRING_LEN];
 } bnx2x_tests_str_arr[BNX2X_NUM_TESTS] = {
@@ -2207,16 +2122,6 @@ static const struct ethtool_ops bnx2x_ethtool_ops = {
 	.set_ringparam		= bnx2x_set_ringparam,
 	.get_pauseparam		= bnx2x_get_pauseparam,
 	.set_pauseparam		= bnx2x_set_pauseparam,
-	.get_rx_csum		= bnx2x_get_rx_csum,
-	.set_rx_csum		= bnx2x_set_rx_csum,
-	.get_tx_csum		= ethtool_op_get_tx_csum,
-	.set_tx_csum		= ethtool_op_set_tx_hw_csum,
-	.set_flags		= bnx2x_set_flags,
-	.get_flags		= ethtool_op_get_flags,
-	.get_sg			= ethtool_op_get_sg,
-	.set_sg			= ethtool_op_set_sg,
-	.get_tso		= ethtool_op_get_tso,
-	.set_tso		= bnx2x_set_tso,
 	.self_test		= bnx2x_self_test,
 	.get_sset_count		= bnx2x_get_sset_count,
 	.get_strings		= bnx2x_get_strings,
diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
index f3cf889..5a60269 100644
--- a/drivers/net/bnx2x/bnx2x_main.c
+++ b/drivers/net/bnx2x/bnx2x_main.c
@@ -7728,6 +7728,7 @@ static void bnx2x_parity_recover(struct bnx2x *bp)
 						return;
 					}
 
+					netdev_update_features(bp->dev);
 					return;
 				}
 			} else { /* non-leader */
@@ -7755,10 +7756,12 @@ static void bnx2x_parity_recover(struct bnx2x *bp)
 					  * the "process kill". It's an exit
 					  * point for a non-leader.
 					  */
-					bnx2x_nic_load(bp, LOAD_NORMAL);
+					int rc = bnx2x_nic_load(bp, LOAD_NORMAL);
 					bp->recovery_state =
 						BNX2X_RECOVERY_DONE;
 					smp_wmb();
+					if (!rc)
+						netdev_update_features(bp->dev);
 					return;
 				}
 			}
@@ -8904,8 +8907,6 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
 	bp->multi_mode = multi_mode;
 	bp->int_mode = int_mode;
 
-	bp->dev->features |= NETIF_F_GRO;
-
 	/* Set TPA flags */
 	if (disable_tpa) {
 		bp->flags &= ~TPA_ENABLE_FLAG;
@@ -8954,6 +8955,7 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
 static int bnx2x_open(struct net_device *dev)
 {
 	struct bnx2x *bp = netdev_priv(dev);
+	int rc;
 
 	netif_carrier_off(dev);
 
@@ -8993,7 +8995,10 @@ static int bnx2x_open(struct net_device *dev)
 
 	bp->recovery_state = BNX2X_RECOVERY_DONE;
 
-	return bnx2x_nic_load(bp, LOAD_OPEN);
+	rc = bnx2x_nic_load(bp, LOAD_OPEN);
+	if (!rc)
+		netdev_update_features(bp->dev);
+	return rc;
 }
 
 /* called with rtnl_lock */
@@ -9304,6 +9309,8 @@ static const struct net_device_ops bnx2x_netdev_ops = {
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_do_ioctl		= bnx2x_ioctl,
 	.ndo_change_mtu		= bnx2x_change_mtu,
+	.ndo_fix_features	= bnx2x_fix_features,
+	.ndo_set_features	= bnx2x_set_features,
 	.ndo_tx_timeout		= bnx2x_tx_timeout,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= poll_bnx2x,
@@ -9430,20 +9437,18 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
 
 	dev->netdev_ops = &bnx2x_netdev_ops;
 	bnx2x_set_ethtool_ops(dev);
-	dev->features |= NETIF_F_SG;
-	dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
+
 	if (bp->flags & USING_DAC_FLAG)
 		dev->features |= NETIF_F_HIGHDMA;
-	dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
-	dev->features |= NETIF_F_TSO6;
-	dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
 
-	dev->vlan_features |= NETIF_F_SG;
-	dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
-	if (bp->flags & USING_DAC_FLAG)
-		dev->vlan_features |= NETIF_F_HIGHDMA;
-	dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
-	dev->vlan_features |= NETIF_F_TSO6;
+	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 |
+		NETIF_F_LRO | NETIF_F_HW_VLAN_TX;
+
+	dev->features |= dev->hw_features | NETIF_F_HW_VLAN_RX;
+
+	dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_HIGHDMA;
 
 #ifdef BCM_DCBNL
 	dev->dcbnl_ops = &bnx2x_dcbnl_ops;
-- 
1.7.2.5

Re: [PATCH v2] net: bnx2x: convert to hw_features

[Michał Mirosław]

The v3 patch fixes missing LRO flag and ensures that netdev_update_features()
won't be called after failed bnx2x_nic_load(). More comments below.

On Mon, Apr 11, 2011 at 05:10:21PM +0300, Vladislav Zolotarov wrote:
> On Sun, 2011-04-10 at 08:35 -0700, Michał Mirosław wrote:
> > Since ndo_fix_features callback is postponing features change when
> > bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
> > has to be called again when this condition changes.
> Unfortunately, NACK again. See below, pls.
[...]
> > diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
> > index e83ac6d..9691b67 100644
> > --- a/drivers/net/bnx2x/bnx2x_cmn.c
> > +++ b/drivers/net/bnx2x/bnx2x_cmn.c
> > @@ -2443,11 +2443,21 @@ alloc_err:
> >  
> >  }
> >  
> > +static int bnx2x_reload_if_running(struct net_device *dev)
> > +{
> > +	struct bnx2x *bp = netdev_priv(dev);
> > +
> > +	if (unlikely(!netif_running(dev)))
> > +		return 0;
> > +
> > +	bnx2x_nic_unload(bp, UNLOAD_NORMAL);
> > +	return bnx2x_nic_load(bp, LOAD_NORMAL);
> > +}
> > +
> >  /* called with rtnl_lock */
> >  int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
> >  {
[...]
> > +u32 bnx2x_fix_features(struct net_device *dev, u32 features)
> > +{
> > +	struct bnx2x *bp = netdev_priv(dev);
> > +
> > +	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
> > +		netdev_err(dev, "Handling parity error recovery. Try again later\n");
> > +
> > +		/* Don't allow bnx2x_set_features() to be called now. */
> > +		return dev->features;
> > +	}
> > +
> > +	/* TPA requires Rx CSUM offloading */
> > +	if (!(features & NETIF_F_RXCSUM) || bp->disable_tpa)
> > +		features &= ~NETIF_F_LRO;
> Shouldn't it be (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM) and not
> NETIF_F_RXCSUM?
[...]
> In addition this function should ensure NETIF_F_IP_CSUM and
> NETIF_F_IPV6_CSUM are changed together.
[...]
> > +int bnx2x_set_features(struct net_device *dev, u32 features)
[...]
> Since there is no set_rx_csum() anymore the above function has to handle
> bp->rx_csum namely correlate it with (NETIF_F_IP_CSUM |
> NETIF_F_IPV6_CSUM) bits in the 'features'. 

You seem to confuse TX checksum offloads (IP_CSUM,IPV6_CSUM) with
RX checksum offload (RXCSUM).

The driver doesn't touch hardware state on changes to checksum offloads
so they are independent - there's no point in adding artificial
dependencies here.

[...]
> > diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
> > index f3cf889..ffa0611 100644
> > --- a/drivers/net/bnx2x/bnx2x_main.c
> > +++ b/drivers/net/bnx2x/bnx2x_main.c
> > @@ -7661,6 +7661,7 @@ exit_leader_reset:
> >  	bp->is_leader = 0;
> >  	bnx2x_release_hw_lock(bp, HW_LOCK_RESOURCE_RESERVED_08);
> >  	smp_wmb();
> > +	netdev_update_features(bp->dev);
> >  	return rc;
> >  }
> 
> Before I continue I'd like to clarify one thing: there is no sense to
> call for netdev_update_features() if bnx2x_nic_load(), called right
> before it, has failed as long as the following bnx2x_nic_load() that
> will be called from the netdev_update_features() flow will also fail
> (for the same reasons as the previous one). If bnx2x_nic_load() fails
> for the certain NIC we actually shut this NIC down. So, the following
> remarks will be based on the above statement.

In all those cases, bnx2x_reload_if_running() will be called only when
LRO state is changed while there's a recovery in progress.

[...]
> U shouldn't call for netdev_update_features(bp->dev) if bnx2x_nic_load()
> has failed. It would also be nice if netdev_update_features() would
> propagate the exit status of ndo_set_features() when ndo_set_features()
> fails in the __netdev_update_features().

That's fixed in v3.

> See the patch for the bnx2x below:
> 
> @@ -8993,7 +8995,14 @@ static int bnx2x_open(struct net_device *dev)
>  
>         bp->recovery_state = BNX2X_RECOVERY_DONE;
>  
> -       return bnx2x_nic_load(bp, LOAD_OPEN);
> +       rc = bnx2x_nic_load(bp, LOAD_OPEN);
> +       if (!rc)
> +               netdev_update_features(bp->dev);
> +
> +       if (bp->state == BNX2X_STATE_OPEN)
> +               return 0;
> +       else
> +               return -EBUSY;
>  }

Hmm. I missed this part in the v3 patch. This clobbers bnx2x_nic_load()'s
error return, though.

> >  /* called with rtnl_lock */
> > @@ -9304,6 +9309,8 @@ static const struct net_device_ops bnx2x_netdev_ops = {
> >  	.ndo_validate_addr	= eth_validate_addr,
> >  	.ndo_do_ioctl		= bnx2x_ioctl,
> >  	.ndo_change_mtu		= bnx2x_change_mtu,
> > +	.ndo_fix_features	= bnx2x_fix_features,
> > +	.ndo_set_features	= bnx2x_set_features,
> >  	.ndo_tx_timeout		= bnx2x_tx_timeout,
> >  #ifdef CONFIG_NET_POLL_CONTROLLER
> >  	.ndo_poll_controller	= poll_bnx2x,
> > @@ -9430,20 +9437,18 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
> >  
> >  	dev->netdev_ops = &bnx2x_netdev_ops;
> >  	bnx2x_set_ethtool_ops(dev);
> > -	dev->features |= NETIF_F_SG;
> > -	dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> > +
> >  	if (bp->flags & USING_DAC_FLAG)
> >  		dev->features |= NETIF_F_HIGHDMA;
> > -	dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> > -	dev->features |= NETIF_F_TSO6;
> > -	dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
> >  
> > -	dev->vlan_features |= NETIF_F_SG;
> > -	dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> > -	if (bp->flags & USING_DAC_FLAG)
> > -		dev->vlan_features |= NETIF_F_HIGHDMA;
> > -	dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> > -	dev->vlan_features |= NETIF_F_TSO6;
> > +	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> > +		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 |
> > +		NETIF_F_HW_VLAN_TX;
> hw_features are missing NETIF_F_GRO and NETIF_F_LRO flags that are
> currently configured in bnx2x_init_bp(). 

GRO is enabled by core now. LRO is fixed in v3.

> > +	dev->features |= dev->hw_features | NETIF_F_HW_VLAN_RX;
> > +
> > +	dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> > +		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_HIGHDMA;
> I'm not sure if it's safe to set NETIF_F_HIGHDMA unconditionally. I
> think it's better to correlate it with the USING_DAC_FLAG which is set
> according to what is returned by 
> dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)).

dev->vlan_features get masked with dev->features and only then applied
to VLAN device.

Best Regards,
Michał Mirosław

[PATCH v4] net: bnx2x: convert to hw_features

[Michał Mirosław]

Since ndo_fix_features callback is postponing features change when
bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
has to be called again when this condition changes. Previously,
ethtool_ops->set_flags callback returned -EBUSY in that case
(it's not possible in the new model).

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
v4: - complete bp->rx_csum -> NETIF_F_RXCSUM conversion
    - add check for failed ndo_set_features in ndo_open callback
v3: - include NETIF_F_LRO in hw_features
    - don't call netdev_update_features() if bnx2x_nic_load() failed
v2: - comment in ndo_fix_features callback
---
 drivers/net/bnx2x/bnx2x.h         |    1 -
 drivers/net/bnx2x/bnx2x_cmn.c     |   54 +++++++++++++++++++--
 drivers/net/bnx2x/bnx2x_cmn.h     |    3 +
 drivers/net/bnx2x/bnx2x_ethtool.c |   95 -------------------------------------
 drivers/net/bnx2x/bnx2x_main.c    |   41 +++++++++-------
 5 files changed, 75 insertions(+), 119 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x.h b/drivers/net/bnx2x/bnx2x.h
index b7ff87b..fefd1d5 100644
--- a/drivers/net/bnx2x/bnx2x.h
+++ b/drivers/net/bnx2x/bnx2x.h
@@ -918,7 +918,6 @@ struct bnx2x {
 
 	int			tx_ring_size;
 
-	u32			rx_csum;
 /* L2 header size + 2*VLANs (8 bytes) + LLC SNAP (8 bytes) */
 #define ETH_OVREHEAD		(ETH_HLEN + 8 + 8)
 #define ETH_MIN_PACKET_SIZE		60
diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
index e83ac6d..7f49cf4 100644
--- a/drivers/net/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/bnx2x/bnx2x_cmn.c
@@ -640,7 +640,7 @@ reuse_rx:
 
 			skb_checksum_none_assert(skb);
 
-			if (bp->rx_csum) {
+			if (bp->dev->features & NETIF_F_RXCSUM) {
 				if (likely(BNX2X_RX_CSUM_OK(cqe)))
 					skb->ip_summed = CHECKSUM_UNNECESSARY;
 				else
@@ -2443,11 +2443,21 @@ alloc_err:
 
 }
 
+static int bnx2x_reload_if_running(struct net_device *dev)
+{
+	struct bnx2x *bp = netdev_priv(dev);
+
+	if (unlikely(!netif_running(dev)))
+		return 0;
+
+	bnx2x_nic_unload(bp, UNLOAD_NORMAL);
+	return bnx2x_nic_load(bp, LOAD_NORMAL);
+}
+
 /* called with rtnl_lock */
 int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
 {
 	struct bnx2x *bp = netdev_priv(dev);
-	int rc = 0;
 
 	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
 		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
@@ -2464,12 +2474,44 @@ int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
 	 */
 	dev->mtu = new_mtu;
 
-	if (netif_running(dev)) {
-		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
-		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
+	return bnx2x_reload_if_running(dev);
+}
+
+u32 bnx2x_fix_features(struct net_device *dev, u32 features)
+{
+	struct bnx2x *bp = netdev_priv(dev);
+
+	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
+		netdev_err(dev, "Handling parity error recovery. Try again later\n");
+
+		/* Don't allow bnx2x_set_features() to be called now. */
+		return dev->features;
+	}
+
+	/* TPA requires Rx CSUM offloading */
+	if (!(features & NETIF_F_RXCSUM) || bp->disable_tpa)
+		features &= ~NETIF_F_LRO;
+
+	return features;
+}
+
+int bnx2x_set_features(struct net_device *dev, u32 features)
+{
+	struct bnx2x *bp = netdev_priv(dev);
+	u32 flags = bp->flags;
+
+	if (features & NETIF_F_LRO)
+		flags |= TPA_ENABLE_FLAG;
+	else
+		flags &= ~TPA_ENABLE_FLAG;
+
+	if (flags ^ bp->flags) {
+		bp->flags = flags;
+
+		return bnx2x_reload_if_running(dev);
 	}
 
-	return rc;
+	return 0;
 }
 
 void bnx2x_tx_timeout(struct net_device *dev)
diff --git a/drivers/net/bnx2x/bnx2x_cmn.h b/drivers/net/bnx2x/bnx2x_cmn.h
index 775fef0..1cdab69 100644
--- a/drivers/net/bnx2x/bnx2x_cmn.h
+++ b/drivers/net/bnx2x/bnx2x_cmn.h
@@ -431,6 +431,9 @@ void bnx2x_free_mem_bp(struct bnx2x *bp);
  */
 int bnx2x_change_mtu(struct net_device *dev, int new_mtu);
 
+u32 bnx2x_fix_features(struct net_device *dev, u32 features);
+int bnx2x_set_features(struct net_device *dev, u32 features);
+
 /**
  * tx timeout netdev callback
  *
diff --git a/drivers/net/bnx2x/bnx2x_ethtool.c b/drivers/net/bnx2x/bnx2x_ethtool.c
index 1479994..ad7d91e 100644
--- a/drivers/net/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/bnx2x/bnx2x_ethtool.c
@@ -1299,91 +1299,6 @@ static int bnx2x_set_pauseparam(struct net_device *dev,
 	return 0;
 }
 
-static int bnx2x_set_flags(struct net_device *dev, u32 data)
-{
-	struct bnx2x *bp = netdev_priv(dev);
-	int changed = 0;
-	int rc = 0;
-
-	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
-		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
-		return -EAGAIN;
-	}
-
-	if (!(data & ETH_FLAG_RXVLAN))
-		return -EINVAL;
-
-	if ((data & ETH_FLAG_LRO) && bp->rx_csum && bp->disable_tpa)
-		return -EINVAL;
-
-	rc = ethtool_op_set_flags(dev, data, ETH_FLAG_LRO | ETH_FLAG_RXVLAN |
-					ETH_FLAG_TXVLAN | ETH_FLAG_RXHASH);
-	if (rc)
-		return rc;
-
-	/* TPA requires Rx CSUM offloading */
-	if ((data & ETH_FLAG_LRO) && bp->rx_csum) {
-		if (!(bp->flags & TPA_ENABLE_FLAG)) {
-			bp->flags |= TPA_ENABLE_FLAG;
-			changed = 1;
-		}
-	} else if (bp->flags & TPA_ENABLE_FLAG) {
-		dev->features &= ~NETIF_F_LRO;
-		bp->flags &= ~TPA_ENABLE_FLAG;
-		changed = 1;
-	}
-
-	if (changed && netif_running(dev)) {
-		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
-		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
-	}
-
-	return rc;
-}
-
-static u32 bnx2x_get_rx_csum(struct net_device *dev)
-{
-	struct bnx2x *bp = netdev_priv(dev);
-
-	return bp->rx_csum;
-}
-
-static int bnx2x_set_rx_csum(struct net_device *dev, u32 data)
-{
-	struct bnx2x *bp = netdev_priv(dev);
-	int rc = 0;
-
-	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
-		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
-		return -EAGAIN;
-	}
-
-	bp->rx_csum = data;
-
-	/* Disable TPA, when Rx CSUM is disabled. Otherwise all
-	   TPA'ed packets will be discarded due to wrong TCP CSUM */
-	if (!data) {
-		u32 flags = ethtool_op_get_flags(dev);
-
-		rc = bnx2x_set_flags(dev, (flags & ~ETH_FLAG_LRO));
-	}
-
-	return rc;
-}
-
-static int bnx2x_set_tso(struct net_device *dev, u32 data)
-{
-	if (data) {
-		dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
-		dev->features |= NETIF_F_TSO6;
-	} else {
-		dev->features &= ~(NETIF_F_TSO | NETIF_F_TSO_ECN);
-		dev->features &= ~NETIF_F_TSO6;
-	}
-
-	return 0;
-}
-
 static const struct {
 	char string[ETH_GSTRING_LEN];
 } bnx2x_tests_str_arr[BNX2X_NUM_TESTS] = {
@@ -2207,16 +2122,6 @@ static const struct ethtool_ops bnx2x_ethtool_ops = {
 	.set_ringparam		= bnx2x_set_ringparam,
 	.get_pauseparam		= bnx2x_get_pauseparam,
 	.set_pauseparam		= bnx2x_set_pauseparam,
-	.get_rx_csum		= bnx2x_get_rx_csum,
-	.set_rx_csum		= bnx2x_set_rx_csum,
-	.get_tx_csum		= ethtool_op_get_tx_csum,
-	.set_tx_csum		= ethtool_op_set_tx_hw_csum,
-	.set_flags		= bnx2x_set_flags,
-	.get_flags		= ethtool_op_get_flags,
-	.get_sg			= ethtool_op_get_sg,
-	.set_sg			= ethtool_op_set_sg,
-	.get_tso		= ethtool_op_get_tso,
-	.set_tso		= bnx2x_set_tso,
 	.self_test		= bnx2x_self_test,
 	.get_sset_count		= bnx2x_get_sset_count,
 	.get_strings		= bnx2x_get_strings,
diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
index f3cf889..5fd7cbb 100644
--- a/drivers/net/bnx2x/bnx2x_main.c
+++ b/drivers/net/bnx2x/bnx2x_main.c
@@ -7728,6 +7728,7 @@ static void bnx2x_parity_recover(struct bnx2x *bp)
 						return;
 					}
 
+					netdev_update_features(bp->dev);
 					return;
 				}
 			} else { /* non-leader */
@@ -7755,10 +7756,12 @@ static void bnx2x_parity_recover(struct bnx2x *bp)
 					  * the "process kill". It's an exit
 					  * point for a non-leader.
 					  */
-					bnx2x_nic_load(bp, LOAD_NORMAL);
+					int rc = bnx2x_nic_load(bp, LOAD_NORMAL);
 					bp->recovery_state =
 						BNX2X_RECOVERY_DONE;
 					smp_wmb();
+					if (!rc)
+						netdev_update_features(bp->dev);
 					return;
 				}
 			}
@@ -8904,8 +8907,6 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
 	bp->multi_mode = multi_mode;
 	bp->int_mode = int_mode;
 
-	bp->dev->features |= NETIF_F_GRO;
-
 	/* Set TPA flags */
 	if (disable_tpa) {
 		bp->flags &= ~TPA_ENABLE_FLAG;
@@ -8925,8 +8926,6 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
 
 	bp->tx_ring_size = MAX_TX_AVAIL;
 
-	bp->rx_csum = 1;
-
 	/* make sure that the numbers are in the right granularity */
 	bp->tx_ticks = (50 / BNX2X_BTR) * BNX2X_BTR;
 	bp->rx_ticks = (25 / BNX2X_BTR) * BNX2X_BTR;
@@ -8954,6 +8953,7 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
 static int bnx2x_open(struct net_device *dev)
 {
 	struct bnx2x *bp = netdev_priv(dev);
+	int rc;
 
 	netif_carrier_off(dev);
 
@@ -8993,7 +8993,14 @@ static int bnx2x_open(struct net_device *dev)
 
 	bp->recovery_state = BNX2X_RECOVERY_DONE;
 
-	return bnx2x_nic_load(bp, LOAD_OPEN);
+	rc = bnx2x_nic_load(bp, LOAD_OPEN);
+	if (!rc) {
+		netdev_update_features(bp->dev);
+		if (bp->state != BNX2X_STATE_OPEN)
+			return -EBUSY;
+	}
+
+	return rc;
 }
 
 /* called with rtnl_lock */
@@ -9304,6 +9311,8 @@ static const struct net_device_ops bnx2x_netdev_ops = {
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_do_ioctl		= bnx2x_ioctl,
 	.ndo_change_mtu		= bnx2x_change_mtu,
+	.ndo_fix_features	= bnx2x_fix_features,
+	.ndo_set_features	= bnx2x_set_features,
 	.ndo_tx_timeout		= bnx2x_tx_timeout,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= poll_bnx2x,
@@ -9430,20 +9439,18 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
 
 	dev->netdev_ops = &bnx2x_netdev_ops;
 	bnx2x_set_ethtool_ops(dev);
-	dev->features |= NETIF_F_SG;
-	dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
+
 	if (bp->flags & USING_DAC_FLAG)
 		dev->features |= NETIF_F_HIGHDMA;
-	dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
-	dev->features |= NETIF_F_TSO6;
-	dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
 
-	dev->vlan_features |= NETIF_F_SG;
-	dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
-	if (bp->flags & USING_DAC_FLAG)
-		dev->vlan_features |= NETIF_F_HIGHDMA;
-	dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
-	dev->vlan_features |= NETIF_F_TSO6;
+	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 |
+		NETIF_F_RXCSUM | NETIF_F_LRO | NETIF_F_HW_VLAN_TX;
+
+	dev->features |= dev->hw_features | NETIF_F_HW_VLAN_RX;
+
+	dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_HIGHDMA;
 
 #ifdef BCM_DCBNL
 	dev->dcbnl_ops = &bnx2x_dcbnl_ops;
-- 
1.7.2.5

Re: [PATCH v2] net: bnx2x: convert to hw_features

[Vladislav Zolotarov]

On Mon, 2011-04-11 at 13:12 -0700, Michał Mirosław wrote:
> The v3 patch fixes missing LRO flag and ensures that netdev_update_features()
> won't be called after failed bnx2x_nic_load(). More comments below.

As long as there is v4 already I'll comment it and skip v3. See a few
comments on your comments below. ;)

> 
> On Mon, Apr 11, 2011 at 05:10:21PM +0300, Vladislav Zolotarov wrote:
> > On Sun, 2011-04-10 at 08:35 -0700, Michał Mirosław wrote:
> > > Since ndo_fix_features callback is postponing features change when
> > > bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
> > > has to be called again when this condition changes.
> > Unfortunately, NACK again. See below, pls.
> [...]
> > > diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
> > > index e83ac6d..9691b67 100644
> > > --- a/drivers/net/bnx2x/bnx2x_cmn.c
> > > +++ b/drivers/net/bnx2x/bnx2x_cmn.c
> > > @@ -2443,11 +2443,21 @@ alloc_err:
> > >  
> > >  }
> > >  
> > > +static int bnx2x_reload_if_running(struct net_device *dev)
> > > +{
> > > +	struct bnx2x *bp = netdev_priv(dev);
> > > +
> > > +	if (unlikely(!netif_running(dev)))
> > > +		return 0;
> > > +
> > > +	bnx2x_nic_unload(bp, UNLOAD_NORMAL);
> > > +	return bnx2x_nic_load(bp, LOAD_NORMAL);
> > > +}
> > > +
> > >  /* called with rtnl_lock */
> > >  int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
> > >  {
> [...]
> > > +u32 bnx2x_fix_features(struct net_device *dev, u32 features)
> > > +{
> > > +	struct bnx2x *bp = netdev_priv(dev);
> > > +
> > > +	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
> > > +		netdev_err(dev, "Handling parity error recovery. Try again later\n");
> > > +
> > > +		/* Don't allow bnx2x_set_features() to be called now. */
> > > +		return dev->features;
> > > +	}
> > > +
> > > +	/* TPA requires Rx CSUM offloading */
> > > +	if (!(features & NETIF_F_RXCSUM) || bp->disable_tpa)
> > > +		features &= ~NETIF_F_LRO;
> > Shouldn't it be (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM) and not
> > NETIF_F_RXCSUM?
> [...]
> > In addition this function should ensure NETIF_F_IP_CSUM and
> > NETIF_F_IPV6_CSUM are changed together.
> [...]
> > > +int bnx2x_set_features(struct net_device *dev, u32 features)
> [...]
> > Since there is no set_rx_csum() anymore the above function has to handle
> > bp->rx_csum namely correlate it with (NETIF_F_IP_CSUM |
> > NETIF_F_IPV6_CSUM) bits in the 'features'. 
> 
> You seem to confuse TX checksum offloads (IP_CSUM,IPV6_CSUM) with
> RX checksum offload (RXCSUM).

U r right. My bad. However u forgot to add RXCSUM to hw_features in v2
but I see it fixed in v4.

> 
> The driver doesn't touch hardware state on changes to checksum offloads
> so they are independent - there's no point in adding artificial
> dependencies here.

Considering Tx csum offloads u are right but this is not true regarding
the Rx csum offload and this is what I meant above. I see that v4
properly handles it now. Sorry for a confusion.

> 
> [...]
> > > diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
> > > index f3cf889..ffa0611 100644
> > > --- a/drivers/net/bnx2x/bnx2x_main.c
> > > +++ b/drivers/net/bnx2x/bnx2x_main.c
> > > @@ -7661,6 +7661,7 @@ exit_leader_reset:
> > >  	bp->is_leader = 0;
> > >  	bnx2x_release_hw_lock(bp, HW_LOCK_RESOURCE_RESERVED_08);
> > >  	smp_wmb();
> > > +	netdev_update_features(bp->dev);
> > >  	return rc;
> > >  }
> > 
> > Before I continue I'd like to clarify one thing: there is no sense to
> > call for netdev_update_features() if bnx2x_nic_load(), called right
> > before it, has failed as long as the following bnx2x_nic_load() that
> > will be called from the netdev_update_features() flow will also fail
> > (for the same reasons as the previous one). If bnx2x_nic_load() fails
> > for the certain NIC we actually shut this NIC down. So, the following
> > remarks will be based on the above statement.
> 
> In all those cases, bnx2x_reload_if_running() will be called only when
> LRO state is changed while there's a recovery in progress.

Hmmm... And what about all other features from hw_features? What if they
have changed (in wanted_features) while recovery was in progress?
According to the __netdev_update_features() code it will invoke
ndo_set_features() in these cases either. Do I miss something here?

> 
> [...]
> > U shouldn't call for netdev_update_features(bp->dev) if bnx2x_nic_load()
> > has failed. It would also be nice if netdev_update_features() would
> > propagate the exit status of ndo_set_features() when ndo_set_features()
> > fails in the __netdev_update_features().
> 
> That's fixed in v3.

Not everything. See below.

> 
> > See the patch for the bnx2x below:
> > 
> > @@ -8993,7 +8995,14 @@ static int bnx2x_open(struct net_device *dev)
> >  
> >         bp->recovery_state = BNX2X_RECOVERY_DONE;
> >  
> > -       return bnx2x_nic_load(bp, LOAD_OPEN);
> > +       rc = bnx2x_nic_load(bp, LOAD_OPEN);
> > +       if (!rc)
> > +               netdev_update_features(bp->dev);
> > +
> > +       if (bp->state == BNX2X_STATE_OPEN)
> > +               return 0;
> > +       else
> > +               return -EBUSY;
> >  }
> 
> Hmm. I missed this part in the v3 patch. This clobbers bnx2x_nic_load()'s
> error return, though.

Exactly! Quoting my remark above: "It would also be nice if
netdev_update_features() would propagate the exit status of
ndo_set_features() when ndo_set_features() fails in the
__netdev_update_features()." Could u comment on this, pls.

> 
> > >  /* called with rtnl_lock */
> > > @@ -9304,6 +9309,8 @@ static const struct net_device_ops bnx2x_netdev_ops = {
> > >  	.ndo_validate_addr	= eth_validate_addr,
> > >  	.ndo_do_ioctl		= bnx2x_ioctl,
> > >  	.ndo_change_mtu		= bnx2x_change_mtu,
> > > +	.ndo_fix_features	= bnx2x_fix_features,
> > > +	.ndo_set_features	= bnx2x_set_features,
> > >  	.ndo_tx_timeout		= bnx2x_tx_timeout,
> > >  #ifdef CONFIG_NET_POLL_CONTROLLER
> > >  	.ndo_poll_controller	= poll_bnx2x,
> > > @@ -9430,20 +9437,18 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
> > >  
> > >  	dev->netdev_ops = &bnx2x_netdev_ops;
> > >  	bnx2x_set_ethtool_ops(dev);
> > > -	dev->features |= NETIF_F_SG;
> > > -	dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> > > +
> > >  	if (bp->flags & USING_DAC_FLAG)
> > >  		dev->features |= NETIF_F_HIGHDMA;
> > > -	dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> > > -	dev->features |= NETIF_F_TSO6;
> > > -	dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
> > >  
> > > -	dev->vlan_features |= NETIF_F_SG;
> > > -	dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> > > -	if (bp->flags & USING_DAC_FLAG)
> > > -		dev->vlan_features |= NETIF_F_HIGHDMA;
> > > -	dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> > > -	dev->vlan_features |= NETIF_F_TSO6;
> > > +	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> > > +		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 |
> > > +		NETIF_F_HW_VLAN_TX;
> > hw_features are missing NETIF_F_GRO and NETIF_F_LRO flags that are
> > currently configured in bnx2x_init_bp(). 
> 
> GRO is enabled by core now. LRO is fixed in v3.

Got it. Thanks.

> 
> > > +	dev->features |= dev->hw_features | NETIF_F_HW_VLAN_RX;
> > > +
> > > +	dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> > > +		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_HIGHDMA;
> > I'm not sure if it's safe to set NETIF_F_HIGHDMA unconditionally. I
> > think it's better to correlate it with the USING_DAC_FLAG which is set
> > according to what is returned by 
> > dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)).
> 
> dev->vlan_features get masked with dev->features and only then applied
> to VLAN device.

Ok. However, could, pls., quote the above sentence of yours as a comment
for this code line? ;)

See my further comments for v4.

thanks,
vlad

> 
> Best Regards,
> Michał Mirosław
> --
> 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
> 

Re: [PATCH v2] net: bnx2x: convert to hw_features

[Vladislav Zolotarov]

> > In all those cases, bnx2x_reload_if_running() will be called only when
> > LRO state is changed while there's a recovery in progress.
> 
> Hmmm... And what about all other features from hw_features? What if they
> have changed (in wanted_features) while recovery was in progress?
> According to the __netdev_update_features() code it will invoke
> ndo_set_features() in these cases either. Do I miss something here?

I think I understood what u meant. So, yes, if the bnx2x_nic_load()
called only if TPA_ENABLED_FLAG in bp->flags has changed. And this can
happen if either NETIF_F_LRO has changed while NETIF_F_RXCSUM was set or
if NETIF_F_LRO was set and NETIF_F_RXCSUM is being cleared.

thanks,
vlad

Re: [PATCH v4] net: bnx2x: convert to hw_features

[Vladislav Zolotarov]

On Mon, 2011-04-11 at 13:26 -0700, Michał Mirosław wrote:
> Since ndo_fix_features callback is postponing features change when
> bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
> has to be called again when this condition changes. Previously,
> ethtool_ops->set_flags callback returned -EBUSY in that case
> (it's not possible in the new model).

ACK (with reservations). ;)

Could u, pls., just add this comment I've asked for in the previous
e-mail? 

The things I first thought to comment on are:
	- Removing TPA_ENABLED_FLAG the similar way u've removed the
bp->rx_csum.
	- Merging the code handling 'features' in bnx2x_init_bp() with the
similar code in bnx2x_init_dev().

However I think it would be right if we clear our mess by ourselves and
that u have already done much enough... ;) 

I've run our standard test suite (which in particular heavily tests the
RX_CSUM and LRO flags toggling) on this patch and it passed it.

Thanks a lot, Michal.
vlad

> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> v4: - complete bp->rx_csum -> NETIF_F_RXCSUM conversion
>     - add check for failed ndo_set_features in ndo_open callback
> v3: - include NETIF_F_LRO in hw_features
>     - don't call netdev_update_features() if bnx2x_nic_load() failed
> v2: - comment in ndo_fix_features callback
> ---
>  drivers/net/bnx2x/bnx2x.h         |    1 -
>  drivers/net/bnx2x/bnx2x_cmn.c     |   54 +++++++++++++++++++--
>  drivers/net/bnx2x/bnx2x_cmn.h     |    3 +
>  drivers/net/bnx2x/bnx2x_ethtool.c |   95 -------------------------------------
>  drivers/net/bnx2x/bnx2x_main.c    |   41 +++++++++-------
>  5 files changed, 75 insertions(+), 119 deletions(-)
> 
> diff --git a/drivers/net/bnx2x/bnx2x.h b/drivers/net/bnx2x/bnx2x.h
> index b7ff87b..fefd1d5 100644
> --- a/drivers/net/bnx2x/bnx2x.h
> +++ b/drivers/net/bnx2x/bnx2x.h
> @@ -918,7 +918,6 @@ struct bnx2x {
>  
>  	int			tx_ring_size;
>  
> -	u32			rx_csum;
>  /* L2 header size + 2*VLANs (8 bytes) + LLC SNAP (8 bytes) */
>  #define ETH_OVREHEAD		(ETH_HLEN + 8 + 8)
>  #define ETH_MIN_PACKET_SIZE		60
> diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
> index e83ac6d..7f49cf4 100644
> --- a/drivers/net/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/bnx2x/bnx2x_cmn.c
> @@ -640,7 +640,7 @@ reuse_rx:
>  
>  			skb_checksum_none_assert(skb);
>  
> -			if (bp->rx_csum) {
> +			if (bp->dev->features & NETIF_F_RXCSUM) {
>  				if (likely(BNX2X_RX_CSUM_OK(cqe)))
>  					skb->ip_summed = CHECKSUM_UNNECESSARY;
>  				else
> @@ -2443,11 +2443,21 @@ alloc_err:
>  
>  }
>  
> +static int bnx2x_reload_if_running(struct net_device *dev)
> +{
> +	struct bnx2x *bp = netdev_priv(dev);
> +
> +	if (unlikely(!netif_running(dev)))
> +		return 0;
> +
> +	bnx2x_nic_unload(bp, UNLOAD_NORMAL);
> +	return bnx2x_nic_load(bp, LOAD_NORMAL);
> +}
> +
>  /* called with rtnl_lock */
>  int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
>  {
>  	struct bnx2x *bp = netdev_priv(dev);
> -	int rc = 0;
>  
>  	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
>  		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
> @@ -2464,12 +2474,44 @@ int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
>  	 */
>  	dev->mtu = new_mtu;
>  
> -	if (netif_running(dev)) {
> -		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
> -		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
> +	return bnx2x_reload_if_running(dev);
> +}
> +
> +u32 bnx2x_fix_features(struct net_device *dev, u32 features)
> +{
> +	struct bnx2x *bp = netdev_priv(dev);
> +
> +	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
> +		netdev_err(dev, "Handling parity error recovery. Try again later\n");
> +
> +		/* Don't allow bnx2x_set_features() to be called now. */
> +		return dev->features;
> +	}
> +
> +	/* TPA requires Rx CSUM offloading */
> +	if (!(features & NETIF_F_RXCSUM) || bp->disable_tpa)
> +		features &= ~NETIF_F_LRO;
> +
> +	return features;
> +}
> +
> +int bnx2x_set_features(struct net_device *dev, u32 features)
> +{
> +	struct bnx2x *bp = netdev_priv(dev);
> +	u32 flags = bp->flags;
> +
> +	if (features & NETIF_F_LRO)
> +		flags |= TPA_ENABLE_FLAG;
> +	else
> +		flags &= ~TPA_ENABLE_FLAG;
> +
> +	if (flags ^ bp->flags) {
> +		bp->flags = flags;
> +
> +		return bnx2x_reload_if_running(dev);
>  	}
>  
> -	return rc;
> +	return 0;
>  }
>  
>  void bnx2x_tx_timeout(struct net_device *dev)
> diff --git a/drivers/net/bnx2x/bnx2x_cmn.h b/drivers/net/bnx2x/bnx2x_cmn.h
> index 775fef0..1cdab69 100644
> --- a/drivers/net/bnx2x/bnx2x_cmn.h
> +++ b/drivers/net/bnx2x/bnx2x_cmn.h
> @@ -431,6 +431,9 @@ void bnx2x_free_mem_bp(struct bnx2x *bp);
>   */
>  int bnx2x_change_mtu(struct net_device *dev, int new_mtu);
>  
> +u32 bnx2x_fix_features(struct net_device *dev, u32 features);
> +int bnx2x_set_features(struct net_device *dev, u32 features);
> +
>  /**
>   * tx timeout netdev callback
>   *
> diff --git a/drivers/net/bnx2x/bnx2x_ethtool.c b/drivers/net/bnx2x/bnx2x_ethtool.c
> index 1479994..ad7d91e 100644
> --- a/drivers/net/bnx2x/bnx2x_ethtool.c
> +++ b/drivers/net/bnx2x/bnx2x_ethtool.c
> @@ -1299,91 +1299,6 @@ static int bnx2x_set_pauseparam(struct net_device *dev,
>  	return 0;
>  }
>  
> -static int bnx2x_set_flags(struct net_device *dev, u32 data)
> -{
> -	struct bnx2x *bp = netdev_priv(dev);
> -	int changed = 0;
> -	int rc = 0;
> -
> -	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
> -		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
> -		return -EAGAIN;
> -	}
> -
> -	if (!(data & ETH_FLAG_RXVLAN))
> -		return -EINVAL;
> -
> -	if ((data & ETH_FLAG_LRO) && bp->rx_csum && bp->disable_tpa)
> -		return -EINVAL;
> -
> -	rc = ethtool_op_set_flags(dev, data, ETH_FLAG_LRO | ETH_FLAG_RXVLAN |
> -					ETH_FLAG_TXVLAN | ETH_FLAG_RXHASH);
> -	if (rc)
> -		return rc;
> -
> -	/* TPA requires Rx CSUM offloading */
> -	if ((data & ETH_FLAG_LRO) && bp->rx_csum) {
> -		if (!(bp->flags & TPA_ENABLE_FLAG)) {
> -			bp->flags |= TPA_ENABLE_FLAG;
> -			changed = 1;
> -		}
> -	} else if (bp->flags & TPA_ENABLE_FLAG) {
> -		dev->features &= ~NETIF_F_LRO;
> -		bp->flags &= ~TPA_ENABLE_FLAG;
> -		changed = 1;
> -	}
> -
> -	if (changed && netif_running(dev)) {
> -		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
> -		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
> -	}
> -
> -	return rc;
> -}
> -
> -static u32 bnx2x_get_rx_csum(struct net_device *dev)
> -{
> -	struct bnx2x *bp = netdev_priv(dev);
> -
> -	return bp->rx_csum;
> -}
> -
> -static int bnx2x_set_rx_csum(struct net_device *dev, u32 data)
> -{
> -	struct bnx2x *bp = netdev_priv(dev);
> -	int rc = 0;
> -
> -	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
> -		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
> -		return -EAGAIN;
> -	}
> -
> -	bp->rx_csum = data;
> -
> -	/* Disable TPA, when Rx CSUM is disabled. Otherwise all
> -	   TPA'ed packets will be discarded due to wrong TCP CSUM */
> -	if (!data) {
> -		u32 flags = ethtool_op_get_flags(dev);
> -
> -		rc = bnx2x_set_flags(dev, (flags & ~ETH_FLAG_LRO));
> -	}
> -
> -	return rc;
> -}
> -
> -static int bnx2x_set_tso(struct net_device *dev, u32 data)
> -{
> -	if (data) {
> -		dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> -		dev->features |= NETIF_F_TSO6;
> -	} else {
> -		dev->features &= ~(NETIF_F_TSO | NETIF_F_TSO_ECN);
> -		dev->features &= ~NETIF_F_TSO6;
> -	}
> -
> -	return 0;
> -}
> -
>  static const struct {
>  	char string[ETH_GSTRING_LEN];
>  } bnx2x_tests_str_arr[BNX2X_NUM_TESTS] = {
> @@ -2207,16 +2122,6 @@ static const struct ethtool_ops bnx2x_ethtool_ops = {
>  	.set_ringparam		= bnx2x_set_ringparam,
>  	.get_pauseparam		= bnx2x_get_pauseparam,
>  	.set_pauseparam		= bnx2x_set_pauseparam,
> -	.get_rx_csum		= bnx2x_get_rx_csum,
> -	.set_rx_csum		= bnx2x_set_rx_csum,
> -	.get_tx_csum		= ethtool_op_get_tx_csum,
> -	.set_tx_csum		= ethtool_op_set_tx_hw_csum,
> -	.set_flags		= bnx2x_set_flags,
> -	.get_flags		= ethtool_op_get_flags,
> -	.get_sg			= ethtool_op_get_sg,
> -	.set_sg			= ethtool_op_set_sg,
> -	.get_tso		= ethtool_op_get_tso,
> -	.set_tso		= bnx2x_set_tso,
>  	.self_test		= bnx2x_self_test,
>  	.get_sset_count		= bnx2x_get_sset_count,
>  	.get_strings		= bnx2x_get_strings,
> diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
> index f3cf889..5fd7cbb 100644
> --- a/drivers/net/bnx2x/bnx2x_main.c
> +++ b/drivers/net/bnx2x/bnx2x_main.c
> @@ -7728,6 +7728,7 @@ static void bnx2x_parity_recover(struct bnx2x *bp)
>  						return;
>  					}
>  
> +					netdev_update_features(bp->dev);
>  					return;
>  				}
>  			} else { /* non-leader */
> @@ -7755,10 +7756,12 @@ static void bnx2x_parity_recover(struct bnx2x *bp)
>  					  * the "process kill". It's an exit
>  					  * point for a non-leader.
>  					  */
> -					bnx2x_nic_load(bp, LOAD_NORMAL);
> +					int rc = bnx2x_nic_load(bp, LOAD_NORMAL);
>  					bp->recovery_state =
>  						BNX2X_RECOVERY_DONE;
>  					smp_wmb();
> +					if (!rc)
> +						netdev_update_features(bp->dev);
>  					return;
>  				}
>  			}
> @@ -8904,8 +8907,6 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
>  	bp->multi_mode = multi_mode;
>  	bp->int_mode = int_mode;
>  
> -	bp->dev->features |= NETIF_F_GRO;
> -
>  	/* Set TPA flags */
>  	if (disable_tpa) {
>  		bp->flags &= ~TPA_ENABLE_FLAG;
> @@ -8925,8 +8926,6 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
>  
>  	bp->tx_ring_size = MAX_TX_AVAIL;
>  
> -	bp->rx_csum = 1;
> -
>  	/* make sure that the numbers are in the right granularity */
>  	bp->tx_ticks = (50 / BNX2X_BTR) * BNX2X_BTR;
>  	bp->rx_ticks = (25 / BNX2X_BTR) * BNX2X_BTR;
> @@ -8954,6 +8953,7 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
>  static int bnx2x_open(struct net_device *dev)
>  {
>  	struct bnx2x *bp = netdev_priv(dev);
> +	int rc;
>  
>  	netif_carrier_off(dev);
>  
> @@ -8993,7 +8993,14 @@ static int bnx2x_open(struct net_device *dev)
>  
>  	bp->recovery_state = BNX2X_RECOVERY_DONE;
>  
> -	return bnx2x_nic_load(bp, LOAD_OPEN);
> +	rc = bnx2x_nic_load(bp, LOAD_OPEN);
> +	if (!rc) {
> +		netdev_update_features(bp->dev);
> +		if (bp->state != BNX2X_STATE_OPEN)
> +			return -EBUSY;
> +	}
> +
> +	return rc;
>  }
>  
>  /* called with rtnl_lock */
> @@ -9304,6 +9311,8 @@ static const struct net_device_ops bnx2x_netdev_ops = {
>  	.ndo_validate_addr	= eth_validate_addr,
>  	.ndo_do_ioctl		= bnx2x_ioctl,
>  	.ndo_change_mtu		= bnx2x_change_mtu,
> +	.ndo_fix_features	= bnx2x_fix_features,
> +	.ndo_set_features	= bnx2x_set_features,
>  	.ndo_tx_timeout		= bnx2x_tx_timeout,
>  #ifdef CONFIG_NET_POLL_CONTROLLER
>  	.ndo_poll_controller	= poll_bnx2x,
> @@ -9430,20 +9439,18 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
>  
>  	dev->netdev_ops = &bnx2x_netdev_ops;
>  	bnx2x_set_ethtool_ops(dev);
> -	dev->features |= NETIF_F_SG;
> -	dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> +
>  	if (bp->flags & USING_DAC_FLAG)
>  		dev->features |= NETIF_F_HIGHDMA;
> -	dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> -	dev->features |= NETIF_F_TSO6;
> -	dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
>  
> -	dev->vlan_features |= NETIF_F_SG;
> -	dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> -	if (bp->flags & USING_DAC_FLAG)
> -		dev->vlan_features |= NETIF_F_HIGHDMA;
> -	dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> -	dev->vlan_features |= NETIF_F_TSO6;
> +	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> +		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 |
> +		NETIF_F_RXCSUM | NETIF_F_LRO | NETIF_F_HW_VLAN_TX;
> +
> +	dev->features |= dev->hw_features | NETIF_F_HW_VLAN_RX;
> +
> +	dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> +		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_HIGHDMA;
>  
>  #ifdef BCM_DCBNL
>  	dev->dcbnl_ops = &bnx2x_dcbnl_ops;

Re: [PATCH v4] net: bnx2x: convert to hw_features

[Michał Mirosław]

On Tue, Apr 12, 2011 at 03:10:28PM +0300, Vladislav Zolotarov wrote:
> On Mon, 2011-04-11 at 13:26 -0700, Michał Mirosław wrote:
> > Since ndo_fix_features callback is postponing features change when
> > bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
> > has to be called again when this condition changes. Previously,
> > ethtool_ops->set_flags callback returned -EBUSY in that case
> > (it's not possible in the new model).
> ACK (with reservations). ;)
> Could u, pls., just add this comment I've asked for in the previous
> e-mail? 

The one about vlan_features? I'll just fix the comment in netdevice.h
instead, since it might be not clear enough.

> The things I first thought to comment on are:
> 	- Removing TPA_ENABLED_FLAG the similar way u've removed the
> bp->rx_csum.
> 	- Merging the code handling 'features' in bnx2x_init_bp() with the
> similar code in bnx2x_init_dev().
> 
> However I think it would be right if we clear our mess by ourselves and
> that u have already done much enough... ;) 
> 
> I've run our standard test suite (which in particular heavily tests the
> RX_CSUM and LRO flags toggling) on this patch and it passed it.

It might be possible to get rid of ndo_set_features, since it looks like
the reset/recovery handler is doing unload/load itself. This needs more
digging into the driver than this simple conversion.

Best Regards,
Michał Mirosław

Re: [PATCH v4] net: bnx2x: convert to hw_features

[Vladislav Zolotarov]

On Tue, 2011-04-12 at 07:07 -0700, Michał Mirosław wrote:
> On Tue, Apr 12, 2011 at 03:10:28PM +0300, Vladislav Zolotarov wrote:
> > On Mon, 2011-04-11 at 13:26 -0700, Michał Mirosław wrote:
> > > Since ndo_fix_features callback is postponing features change when
> > > bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
> > > has to be called again when this condition changes. Previously,
> > > ethtool_ops->set_flags callback returned -EBUSY in that case
> > > (it's not possible in the new model).
> > ACK (with reservations). ;)
> > Could u, pls., just add this comment I've asked for in the previous
> > e-mail? 
> 
> The one about vlan_features? 

Yeah, this one.

> I'll just fix the comment in netdevice.h
> instead, since it might be not clear enough.

Could u still add this comment to bnx2x in your patch as well. It'll
just make these code section more clear regardless the netdevice.h
contents... ;)



> 
> > The things I first thought to comment on are:
> > 	- Removing TPA_ENABLED_FLAG the similar way u've removed the
> > bp->rx_csum.
> > 	- Merging the code handling 'features' in bnx2x_init_bp() with the
> > similar code in bnx2x_init_dev().
> > 
> > However I think it would be right if we clear our mess by ourselves and
> > that u have already done much enough... ;) 
> > 
> > I've run our standard test suite (which in particular heavily tests the
> > RX_CSUM and LRO flags toggling) on this patch and it passed it.
> 
> It might be possible to get rid of ndo_set_features, since it looks like
> the reset/recovery handler is doing unload/load itself. This needs more
> digging into the driver than this simple conversion.

Hmm... I don't get u here. Although the recovery handler does
unload/load itself if there has been an attempt to change features
during the recovery it won't be able to get applied until the whole
recovery process ends. So, this patch added the extra call for
netdev_update_features() right after we know that the recovery process
has successfully ended. So, could u, pls., explain exactly what do u
mean here by saying "It might be possible to get rid of
ndo_set_features"?

thanks,
vlad

> 
> Best Regards,
> Michał Mirosław
> 

Re: [PATCH v4] net: bnx2x: convert to hw_features

[Michał Mirosław]

On Tue, Apr 12, 2011 at 05:36:52PM +0300, Vladislav Zolotarov wrote:
> On Tue, 2011-04-12 at 07:07 -0700, Michał Mirosław wrote:
> > On Tue, Apr 12, 2011 at 03:10:28PM +0300, Vladislav Zolotarov wrote:
> > > On Mon, 2011-04-11 at 13:26 -0700, Michał Mirosław wrote:
> > > > Since ndo_fix_features callback is postponing features change when
> > > > bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
> > > > has to be called again when this condition changes. Previously,
> > > > ethtool_ops->set_flags callback returned -EBUSY in that case
> > > > (it's not possible in the new model).
> > > ACK (with reservations). ;)
> > > Could u, pls., just add this comment I've asked for in the previous
> > > e-mail? 
> > The one about vlan_features? 
> Yeah, this one.
> > I'll just fix the comment in netdevice.h
> > instead, since it might be not clear enough.
> Could u still add this comment to bnx2x in your patch as well. It'll
> just make these code section more clear regardless the netdevice.h
> contents... ;)

This would be duplicating information easily found elsewhere.
I don't like it. :)

> > > The things I first thought to comment on are:
> > > 	- Removing TPA_ENABLED_FLAG the similar way u've removed the
> > > bp->rx_csum.
> > > 	- Merging the code handling 'features' in bnx2x_init_bp() with the
> > > similar code in bnx2x_init_dev().
> > > 
> > > However I think it would be right if we clear our mess by ourselves and
> > > that u have already done much enough... ;) 
> > > 
> > > I've run our standard test suite (which in particular heavily tests the
> > > RX_CSUM and LRO flags toggling) on this patch and it passed it.
> > It might be possible to get rid of ndo_set_features, since it looks like
> > the reset/recovery handler is doing unload/load itself. This needs more
> > digging into the driver than this simple conversion.
> Hmm... I don't get u here. Although the recovery handler does
> unload/load itself if there has been an attempt to change features
> during the recovery it won't be able to get applied until the whole
> recovery process ends. So, this patch added the extra call for
> netdev_update_features() right after we know that the recovery process
> has successfully ended. So, could u, pls., explain exactly what do u
> mean here by saying "It might be possible to get rid of
> ndo_set_features"?

Hmm. I thought one, and wrote another.

Since bnx2x_parity_recover() runs with rtnl_lock(), as should
netdev_update_features(), then in case the recovery is in progress
it should be enough to not call bnx2x_reload_if_running() then
and just change the flags --- changes will be picked up on bnx2x_nic_load()
after recovery is complete. This removes the need for netdev_update_features()
calls.

Best Regards,
Michał Mirosław

[PATCH v5] net: bnx2x: convert to hw_features

[Michał Mirosław]

Since ndo_fix_features callback is postponing features change when
bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
has to be called again when this condition changes. Previously,
ethtool_ops->set_flags callback returned -EBUSY in that case
(it's not possible in the new model).

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

v5: - don't delay set_features, as it's rtnl_locked - same as recovery process
v4: - complete bp->rx_csum -> NETIF_F_RXCSUM conversion
    - add check for failed ndo_set_features in ndo_open callback
v3: - include NETIF_F_LRO in hw_features
    - don't call netdev_update_features() if bnx2x_nic_load() failed
v2: - comment in ndo_fix_features callback
---
 drivers/net/bnx2x/bnx2x.h         |    1 -
 drivers/net/bnx2x/bnx2x_cmn.c     |   49 +++++++++++++++++--
 drivers/net/bnx2x/bnx2x_cmn.h     |    3 +
 drivers/net/bnx2x/bnx2x_ethtool.c |   95 -------------------------------------
 drivers/net/bnx2x/bnx2x_main.c    |   27 ++++------
 5 files changed, 57 insertions(+), 118 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x.h b/drivers/net/bnx2x/bnx2x.h
index e0fca70..9e87417 100644
--- a/drivers/net/bnx2x/bnx2x.h
+++ b/drivers/net/bnx2x/bnx2x.h
@@ -918,7 +918,6 @@ struct bnx2x {
 
 	int			tx_ring_size;
 
-	u32			rx_csum;
 /* L2 header size + 2*VLANs (8 bytes) + LLC SNAP (8 bytes) */
 #define ETH_OVREHEAD		(ETH_HLEN + 8 + 8)
 #define ETH_MIN_PACKET_SIZE		60
diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
index e83ac6d..bec33a8 100644
--- a/drivers/net/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/bnx2x/bnx2x_cmn.c
@@ -640,7 +640,7 @@ reuse_rx:
 
 			skb_checksum_none_assert(skb);
 
-			if (bp->rx_csum) {
+			if (bp->dev->features & NETIF_F_RXCSUM) {
 				if (likely(BNX2X_RX_CSUM_OK(cqe)))
 					skb->ip_summed = CHECKSUM_UNNECESSARY;
 				else
@@ -2443,11 +2443,21 @@ alloc_err:
 
 }
 
+static int bnx2x_reload_if_running(struct net_device *dev)
+{
+	struct bnx2x *bp = netdev_priv(dev);
+
+	if (unlikely(!netif_running(dev)))
+		return 0;
+
+	bnx2x_nic_unload(bp, UNLOAD_NORMAL);
+	return bnx2x_nic_load(bp, LOAD_NORMAL);
+}
+
 /* called with rtnl_lock */
 int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
 {
 	struct bnx2x *bp = netdev_priv(dev);
-	int rc = 0;
 
 	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
 		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
@@ -2464,12 +2474,39 @@ int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
 	 */
 	dev->mtu = new_mtu;
 
-	if (netif_running(dev)) {
-		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
-		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
+	return bnx2x_reload_if_running(dev);
+}
+
+u32 bnx2x_fix_features(struct net_device *dev, u32 features)
+{
+	struct bnx2x *bp = netdev_priv(dev);
+
+	/* TPA requires Rx CSUM offloading */
+	if (!(features & NETIF_F_RXCSUM) || bp->disable_tpa)
+		features &= ~NETIF_F_LRO;
+
+	return features;
+}
+
+int bnx2x_set_features(struct net_device *dev, u32 features)
+{
+	struct bnx2x *bp = netdev_priv(dev);
+	u32 flags = bp->flags;
+
+	if (features & NETIF_F_LRO)
+		flags |= TPA_ENABLE_FLAG;
+	else
+		flags &= ~TPA_ENABLE_FLAG;
+
+	if (flags ^ bp->flags) {
+		bp->flags = flags;
+
+		if (bp->recovery_state == BNX2X_RECOVERY_DONE)
+			return bnx2x_reload_if_running(dev);
+		/* else: bnx2x_nic_load() will be called at end of recovery */
 	}
 
-	return rc;
+	return 0;
 }
 
 void bnx2x_tx_timeout(struct net_device *dev)
diff --git a/drivers/net/bnx2x/bnx2x_cmn.h b/drivers/net/bnx2x/bnx2x_cmn.h
index 775fef0..1cdab69 100644
--- a/drivers/net/bnx2x/bnx2x_cmn.h
+++ b/drivers/net/bnx2x/bnx2x_cmn.h
@@ -431,6 +431,9 @@ void bnx2x_free_mem_bp(struct bnx2x *bp);
  */
 int bnx2x_change_mtu(struct net_device *dev, int new_mtu);
 
+u32 bnx2x_fix_features(struct net_device *dev, u32 features);
+int bnx2x_set_features(struct net_device *dev, u32 features);
+
 /**
  * tx timeout netdev callback
  *
diff --git a/drivers/net/bnx2x/bnx2x_ethtool.c b/drivers/net/bnx2x/bnx2x_ethtool.c
index 1479994..ad7d91e 100644
--- a/drivers/net/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/bnx2x/bnx2x_ethtool.c
@@ -1299,91 +1299,6 @@ static int bnx2x_set_pauseparam(struct net_device *dev,
 	return 0;
 }
 
-static int bnx2x_set_flags(struct net_device *dev, u32 data)
-{
-	struct bnx2x *bp = netdev_priv(dev);
-	int changed = 0;
-	int rc = 0;
-
-	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
-		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
-		return -EAGAIN;
-	}
-
-	if (!(data & ETH_FLAG_RXVLAN))
-		return -EINVAL;
-
-	if ((data & ETH_FLAG_LRO) && bp->rx_csum && bp->disable_tpa)
-		return -EINVAL;
-
-	rc = ethtool_op_set_flags(dev, data, ETH_FLAG_LRO | ETH_FLAG_RXVLAN |
-					ETH_FLAG_TXVLAN | ETH_FLAG_RXHASH);
-	if (rc)
-		return rc;
-
-	/* TPA requires Rx CSUM offloading */
-	if ((data & ETH_FLAG_LRO) && bp->rx_csum) {
-		if (!(bp->flags & TPA_ENABLE_FLAG)) {
-			bp->flags |= TPA_ENABLE_FLAG;
-			changed = 1;
-		}
-	} else if (bp->flags & TPA_ENABLE_FLAG) {
-		dev->features &= ~NETIF_F_LRO;
-		bp->flags &= ~TPA_ENABLE_FLAG;
-		changed = 1;
-	}
-
-	if (changed && netif_running(dev)) {
-		bnx2x_nic_unload(bp, UNLOAD_NORMAL);
-		rc = bnx2x_nic_load(bp, LOAD_NORMAL);
-	}
-
-	return rc;
-}
-
-static u32 bnx2x_get_rx_csum(struct net_device *dev)
-{
-	struct bnx2x *bp = netdev_priv(dev);
-
-	return bp->rx_csum;
-}
-
-static int bnx2x_set_rx_csum(struct net_device *dev, u32 data)
-{
-	struct bnx2x *bp = netdev_priv(dev);
-	int rc = 0;
-
-	if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
-		printk(KERN_ERR "Handling parity error recovery. Try again later\n");
-		return -EAGAIN;
-	}
-
-	bp->rx_csum = data;
-
-	/* Disable TPA, when Rx CSUM is disabled. Otherwise all
-	   TPA'ed packets will be discarded due to wrong TCP CSUM */
-	if (!data) {
-		u32 flags = ethtool_op_get_flags(dev);
-
-		rc = bnx2x_set_flags(dev, (flags & ~ETH_FLAG_LRO));
-	}
-
-	return rc;
-}
-
-static int bnx2x_set_tso(struct net_device *dev, u32 data)
-{
-	if (data) {
-		dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
-		dev->features |= NETIF_F_TSO6;
-	} else {
-		dev->features &= ~(NETIF_F_TSO | NETIF_F_TSO_ECN);
-		dev->features &= ~NETIF_F_TSO6;
-	}
-
-	return 0;
-}
-
 static const struct {
 	char string[ETH_GSTRING_LEN];
 } bnx2x_tests_str_arr[BNX2X_NUM_TESTS] = {
@@ -2207,16 +2122,6 @@ static const struct ethtool_ops bnx2x_ethtool_ops = {
 	.set_ringparam		= bnx2x_set_ringparam,
 	.get_pauseparam		= bnx2x_get_pauseparam,
 	.set_pauseparam		= bnx2x_set_pauseparam,
-	.get_rx_csum		= bnx2x_get_rx_csum,
-	.set_rx_csum		= bnx2x_set_rx_csum,
-	.get_tx_csum		= ethtool_op_get_tx_csum,
-	.set_tx_csum		= ethtool_op_set_tx_hw_csum,
-	.set_flags		= bnx2x_set_flags,
-	.get_flags		= ethtool_op_get_flags,
-	.get_sg			= ethtool_op_get_sg,
-	.set_sg			= ethtool_op_set_sg,
-	.get_tso		= ethtool_op_get_tso,
-	.set_tso		= bnx2x_set_tso,
 	.self_test		= bnx2x_self_test,
 	.get_sset_count		= bnx2x_get_sset_count,
 	.get_strings		= bnx2x_get_strings,
diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
index a6915aa..696e84a 100644
--- a/drivers/net/bnx2x/bnx2x_main.c
+++ b/drivers/net/bnx2x/bnx2x_main.c
@@ -8904,8 +8904,6 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
 	bp->multi_mode = multi_mode;
 	bp->int_mode = int_mode;
 
-	bp->dev->features |= NETIF_F_GRO;
-
 	/* Set TPA flags */
 	if (disable_tpa) {
 		bp->flags &= ~TPA_ENABLE_FLAG;
@@ -8925,8 +8923,6 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
 
 	bp->tx_ring_size = MAX_TX_AVAIL;
 
-	bp->rx_csum = 1;
-
 	/* make sure that the numbers are in the right granularity */
 	bp->tx_ticks = (50 / BNX2X_BTR) * BNX2X_BTR;
 	bp->rx_ticks = (25 / BNX2X_BTR) * BNX2X_BTR;
@@ -9304,6 +9300,8 @@ static const struct net_device_ops bnx2x_netdev_ops = {
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_do_ioctl		= bnx2x_ioctl,
 	.ndo_change_mtu		= bnx2x_change_mtu,
+	.ndo_fix_features	= bnx2x_fix_features,
+	.ndo_set_features	= bnx2x_set_features,
 	.ndo_tx_timeout		= bnx2x_tx_timeout,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= poll_bnx2x,
@@ -9430,20 +9428,17 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
 
 	dev->netdev_ops = &bnx2x_netdev_ops;
 	bnx2x_set_ethtool_ops(dev);
-	dev->features |= NETIF_F_SG;
-	dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
+
+	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 |
+		NETIF_F_RXCSUM | NETIF_F_LRO | NETIF_F_HW_VLAN_TX;
+
+	dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_HIGHDMA;
+
+	dev->features |= dev->hw_features | NETIF_F_HW_VLAN_RX;
 	if (bp->flags & USING_DAC_FLAG)
 		dev->features |= NETIF_F_HIGHDMA;
-	dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
-	dev->features |= NETIF_F_TSO6;
-	dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
-
-	dev->vlan_features |= NETIF_F_SG;
-	dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
-	if (bp->flags & USING_DAC_FLAG)
-		dev->vlan_features |= NETIF_F_HIGHDMA;
-	dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
-	dev->vlan_features |= NETIF_F_TSO6;
 
 #ifdef BCM_DCBNL
 	dev->dcbnl_ops = &bnx2x_dcbnl_ops;
-- 
1.7.2.5

Re: [PATCH v5] net: bnx2x: convert to hw_features

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Tue, 12 Apr 2011 21:38:23 +0200 (CEST)

> Since ndo_fix_features callback is postponing features change when
> bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
> has to be called again when this condition changes. Previously,
> ethtool_ops->set_flags callback returned -EBUSY in that case
> (it's not possible in the new model).
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> 
> v5: - don't delay set_features, as it's rtnl_locked - same as recovery process
> v4: - complete bp->rx_csum -> NETIF_F_RXCSUM conversion
>     - add check for failed ndo_set_features in ndo_open callback
> v3: - include NETIF_F_LRO in hw_features
>     - don't call netdev_update_features() if bnx2x_nic_load() failed
> v2: - comment in ndo_fix_features callback

I've applied this.

From what I can tell the is basic agreement from the Broadcom folks,
and if any fixups are needed that can be done with follow-up
patches.

Thanks.

Re: [PATCH v4] net: bnx2x: convert to hw_features

[Vladislav Zolotarov]

> Hmm. I thought one, and wrote another.
> 
> Since bnx2x_parity_recover() runs with rtnl_lock(), as should
> netdev_update_features(), then in case the recovery is in progress
> it should be enough to not call bnx2x_reload_if_running() then
> and just change the flags --- changes will be picked up on bnx2x_nic_load()
> after recovery is complete. This removes the need for netdev_update_features()
> calls.
> 

Ok. I see what u meant in v5... ;) 
The patch is great. Thanks for your work. There are some enhancements
following your patch that I have in mind that I'll submit later.

thanks again, Michal.

Re: [PATCH v5] net: bnx2x: convert to hw_features

[Eric Dumazet]

Le mardi 12 avril 2011 à 21:38 +0200, Michał Mirosław a écrit :
> Since ndo_fix_features callback is postponing features change when
> bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
> has to be called again when this condition changes. Previously,
> ethtool_ops->set_flags callback returned -EBUSY in that case
> (it's not possible in the new model).
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> 
> v5: - don't delay set_features, as it's rtnl_locked - same as recovery process
> v4: - complete bp->rx_csum -> NETIF_F_RXCSUM conversion
>     - add check for failed ndo_set_features in ndo_open callback
> v3: - include NETIF_F_LRO in hw_features
>     - don't call netdev_update_features() if bnx2x_nic_load() failed
> v2: - comment in ndo_fix_features callback
> ---

Hi guys

I am not sure its related to these changes, but I now have in
net-next-2.6 :

[   23.674263] ------------[ cut here ]------------
[   23.674266] WARNING: at net/core/dev.c:1318 dev_disable_lro+0x83/0x90()
[   23.674270] Hardware name: ProLiant BL460c G6
[   23.674273] Modules linked in: tg3 libphy sg
[   23.674280] Pid: 3070, comm: sysctl Tainted: G        W   2.6.39-rc2-01242-g3ef22b9-dirty #669
[   23.674282] Call Trace:
[   23.674285]  [<ffffffff813b94f3>] ? dev_disable_lro+0x83/0x90
[   23.674291]  [<ffffffff81042c9b>] warn_slowpath_common+0x8b/0xc0
[   23.674298]  [<ffffffff81042ce5>] warn_slowpath_null+0x15/0x20
[   23.674304]  [<ffffffff813b94f3>] dev_disable_lro+0x83/0x90
[   23.674309]  [<ffffffff81429789>] devinet_sysctl_forward+0x199/0x210
[   23.674313]  [<ffffffff814296e4>] ? devinet_sysctl_forward+0xf4/0x210
[   23.674318]  [<ffffffff8104e712>] ? capable+0x12/0x20
[   23.674324]  [<ffffffff81168f45>] proc_sys_call_handler+0xb5/0xd0
[   23.674328]  [<ffffffff81168f6f>] proc_sys_write+0xf/0x20
[   23.674334]  [<ffffffff81105f39>] vfs_write+0xc9/0x170
[   23.674339]  [<ffffffff81106550>] sys_write+0x50/0x90
[   23.674345]  [<ffffffff814b95a0>] sysenter_dispatch+0x7/0x33
[   23.674350] ---[ end trace 051ec497c66b228e ]---

Thanks

Re: [PATCH v5] net: bnx2x: convert to hw_features

[Vladislav Zolotarov]

On Thu, 2011-04-21 at 07:52 -0700, Eric Dumazet wrote:
> Le mardi 12 avril 2011  21:38 +0200, Micha Mirosaw a crit :
> > Since ndo_fix_features callback is postponing features change when
> > bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
> > has to be called again when this condition changes. Previously,
> > ethtool_ops->set_flags callback returned -EBUSY in that case
> > (it's not possible in the new model).
> > 
> > Signed-off-by: Micha Mirosaw <mirq-linux@rere.qmqm.pl>
> > 
> > v5: - don't delay set_features, as it's rtnl_locked - same as recovery process
> > v4: - complete bp->rx_csum -> NETIF_F_RXCSUM conversion
> >     - add check for failed ndo_set_features in ndo_open callback
> > v3: - include NETIF_F_LRO in hw_features
> >     - don't call netdev_update_features() if bnx2x_nic_load() failed
> > v2: - comment in ndo_fix_features callback
> > ---
> 
> Hi guys
> 
> I am not sure its related to these changes, but I now have in
> net-next-2.6 :
> 
> [   23.674263] ------------[ cut here ]------------
> [   23.674266] WARNING: at net/core/dev.c:1318 dev_disable_lro+0x83/0x90()
> [   23.674270] Hardware name: ProLiant BL460c G6
> [   23.674273] Modules linked in: tg3 libphy sg
> [   23.674280] Pid: 3070, comm: sysctl Tainted: G        W   2.6.39-rc2-01242-g3ef22b9-dirty #669
> [   23.674282] Call Trace:
> [   23.674285]  [<ffffffff813b94f3>] ? dev_disable_lro+0x83/0x90
> [   23.674291]  [<ffffffff81042c9b>] warn_slowpath_common+0x8b/0xc0
> [   23.674298]  [<ffffffff81042ce5>] warn_slowpath_null+0x15/0x20
> [   23.674304]  [<ffffffff813b94f3>] dev_disable_lro+0x83/0x90
> [   23.674309]  [<ffffffff81429789>] devinet_sysctl_forward+0x199/0x210
> [   23.674313]  [<ffffffff814296e4>] ? devinet_sysctl_forward+0xf4/0x210
> [   23.674318]  [<ffffffff8104e712>] ? capable+0x12/0x20
> [   23.674324]  [<ffffffff81168f45>] proc_sys_call_handler+0xb5/0xd0
> [   23.674328]  [<ffffffff81168f6f>] proc_sys_write+0xf/0x20
> [   23.674334]  [<ffffffff81105f39>] vfs_write+0xc9/0x170
> [   23.674339]  [<ffffffff81106550>] sys_write+0x50/0x90
> [   23.674345]  [<ffffffff814b95a0>] sysenter_dispatch+0x7/0x33
> [   23.674350] ---[ end trace 051ec497c66b228e ]---
> 
> Thanks
> 

More than that, in addition it is impossible to disable the LRO with 
the current bnx2x upstream driver (ethtool -K ethX lro off) and this is
because dev_disable_lro passes to __ethtool_set_flags() flags based on
the current value of dev->features while __ethtool_set_flags() expects
only the flags set in dev->hw_features. bnx2x has NETIF_F_HW_VLAN_RX
that is set in dev->features and not set in dev->hw_features and it's
passed down to the __ethtool_set_flags(). 

Regarding the 'ethtool -K ethX lro off' I noticed that there is the same
problem: the flags that are passed to the __ethtool_set_flags() include
NETIF_F_HW_VLAN_RX. The userspace app gets the flags set in
dev->features to be able to display them, then clears the needed bits
and passes the resulting value down to kernel. So, I don't know how to
fix this without changing __ethtool_set_flags() not to check the bits or
by the taking care of the bit mask before it's passed to the
__ethtool_set_flags(). Below is the implementation of the second
option: 

diff --git a/net/core/dev.c b/net/core/dev.c
index 3871bf6..fd9175b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1314,7 +1314,7 @@ void dev_disable_lro(struct net_device *dev)
 	if (!(flags & ETH_FLAG_LRO))
 		return;
 
-	__ethtool_set_flags(dev, flags & ~ETH_FLAG_LRO);
+	__ethtool_set_flags(dev, flags & ~ETH_FLAG_LRO & dev->hw_features);
 	WARN_ON(dev->features & NETIF_F_LRO);
 }
 EXPORT_SYMBOL(dev_disable_lro);
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 13d79f5..bbf75fc 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1810,7 +1810,7 @@ static int ethtool_set_value(struct net_device *dev, char __user *useraddr,
 	if (copy_from_user(&edata, useraddr, sizeof(edata)))
 		return -EFAULT;
 
-	return actor(dev, edata.data);
+	return actor(dev, edata.data & dev->hw_features);
 }
 
 static noinline_for_stack int ethtool_flash_device(struct net_device *dev,

Pls., comment.

Thanks,
vlad



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

Re: [PATCH v5] net: bnx2x: convert to hw_features

[Ben Hutchings]

On Thu, 2011-04-21 at 21:49 +0300, Vladislav Zolotarov wrote:
[...]
> More than that, in addition it is impossible to disable the LRO with 
> the current bnx2x upstream driver (ethtool -K ethX lro off) and this is
> because dev_disable_lro passes to __ethtool_set_flags() flags based on
> the current value of dev->features while __ethtool_set_flags() expects
> only the flags set in dev->hw_features. bnx2x has NETIF_F_HW_VLAN_RX
> that is set in dev->features and not set in dev->hw_features and it's
> passed down to the __ethtool_set_flags(). 
[...]

The problem is here in register_netdevice():

	/* Transfer changeable features to wanted_features and enable
	 * software offloads (GSO and GRO).
	 */
	dev->hw_features |= NETIF_F_SOFT_FEATURES;
	dev->features |= NETIF_F_SOFT_FEATURES;
	dev->wanted_features = dev->features & dev->hw_features;

This doesn't work correctly for features that are always enabled, like
NETIF_F_HW_VLAN_RX in bnx2x, which are set in dev->features but not in
dev->hw_features.

The name 'hw_features' really wasn't a good choice - the obvious meaning
and the meaning assumed by this code is 'hardware-supported features'
and not 'hardware-supported features that can be toggled'.  And since we
add NETIF_F_SOFT_FEATURES, it really only means 'features that can be
toggled'.

Ben.

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

Re: [PATCH v5] net: bnx2x: convert to hw_features

[Michał Mirosław]

On Thu, Apr 21, 2011 at 08:19:19PM +0100, Ben Hutchings wrote:
> 	/* Transfer changeable features to wanted_features and enable
> 	 * software offloads (GSO and GRO).
> 	 */
> 	dev->hw_features |= NETIF_F_SOFT_FEATURES;
> 	dev->features |= NETIF_F_SOFT_FEATURES;
> 	dev->wanted_features = dev->features & dev->hw_features;
> 
> This doesn't work correctly for features that are always enabled, like
> NETIF_F_HW_VLAN_RX in bnx2x, which are set in dev->features but not in
> dev->hw_features.

> The name 'hw_features' really wasn't a good choice - the obvious meaning
> and the meaning assumed by this code is 'hardware-supported features'
> and not 'hardware-supported features that can be toggled'.  And since we
> add NETIF_F_SOFT_FEATURES, it really only means 'features that can be
> toggled'.

I won't argue about hw_features name - I just couldn't find a better one.
Comment in include/linux/netdevice.h clearly explains the purpose of this
field.

wanted_features is supposed to be limited by hw_features (so that it's always
true that (hw_features & wanted_features) == wanted_features). If you have
an idea how to make that more clear, I'd be happy to update descriptions.

Best Regards,
Michał Mirosław

Re: [PATCH v5] net: bnx2x: convert to hw_features

[Michał Mirosław]

On Thu, Apr 21, 2011 at 04:52:11PM +0200, Eric Dumazet wrote:
> Le mardi 12 avril 2011 à 21:38 +0200, Michał Mirosław a écrit :
> > Since ndo_fix_features callback is postponing features change when
> > bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
> > has to be called again when this condition changes. Previously,
> > ethtool_ops->set_flags callback returned -EBUSY in that case
> > (it's not possible in the new model).
> > 
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > 
> > v5: - don't delay set_features, as it's rtnl_locked - same as recovery process
> > v4: - complete bp->rx_csum -> NETIF_F_RXCSUM conversion
> >     - add check for failed ndo_set_features in ndo_open callback
> > v3: - include NETIF_F_LRO in hw_features
> >     - don't call netdev_update_features() if bnx2x_nic_load() failed
> > v2: - comment in ndo_fix_features callback
> > ---
> I am not sure its related to these changes, but I now have in
> net-next-2.6 :

> [   23.674263] ------------[ cut here ]------------
> [   23.674266] WARNING: at net/core/dev.c:1318 dev_disable_lro+0x83/0x90()
> [   23.674270] Hardware name: ProLiant BL460c G6
> [   23.674273] Modules linked in: tg3 libphy sg
> [   23.674280] Pid: 3070, comm: sysctl Tainted: G        W   2.6.39-rc2-01242-g3ef22b9-dirty #669
> [   23.674282] Call Trace:
> [   23.674285]  [<ffffffff813b94f3>] ? dev_disable_lro+0x83/0x90
> [   23.674291]  [<ffffffff81042c9b>] warn_slowpath_common+0x8b/0xc0
> [   23.674298]  [<ffffffff81042ce5>] warn_slowpath_null+0x15/0x20
> [   23.674304]  [<ffffffff813b94f3>] dev_disable_lro+0x83/0x90
> [   23.674309]  [<ffffffff81429789>] devinet_sysctl_forward+0x199/0x210
[...]

Hmm. Looks like something is not allowing to disable LRO. Please check with
following patch so we can be sure which driver causes this.

Best Regards,
Michał Mirosław

[PATCH] net: make WARN_ON in dev_disable_lro() useful

[Michał Mirosław]

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 net/core/dev.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 3871bf6..3421184 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1315,7 +1315,8 @@ void dev_disable_lro(struct net_device *dev)
 		return;
 
 	__ethtool_set_flags(dev, flags & ~ETH_FLAG_LRO);
-	WARN_ON(dev->features & NETIF_F_LRO);
+	if (unlikely(dev->features & NETIF_F_LRO))
+		netdev_WARN(dev, "failed to disable LRO!\n");
 }
 EXPORT_SYMBOL(dev_disable_lro);
 
-- 
1.7.2.5

Re: [PATCH v5] net: bnx2x: convert to hw_features

[Ben Hutchings]

On Fri, 2011-04-22 at 00:15 +0200, Michał Mirosław wrote:
> On Thu, Apr 21, 2011 at 08:19:19PM +0100, Ben Hutchings wrote:
> > 	/* Transfer changeable features to wanted_features and enable
> > 	 * software offloads (GSO and GRO).
> > 	 */
> > 	dev->hw_features |= NETIF_F_SOFT_FEATURES;
> > 	dev->features |= NETIF_F_SOFT_FEATURES;
> > 	dev->wanted_features = dev->features & dev->hw_features;
> > 
> > This doesn't work correctly for features that are always enabled, like
> > NETIF_F_HW_VLAN_RX in bnx2x, which are set in dev->features but not in
> > dev->hw_features.
> 
> > The name 'hw_features' really wasn't a good choice - the obvious meaning
> > and the meaning assumed by this code is 'hardware-supported features'
> > and not 'hardware-supported features that can be toggled'.  And since we
> > add NETIF_F_SOFT_FEATURES, it really only means 'features that can be
> > toggled'.
> 
> I won't argue about hw_features name - I just couldn't find a better one.
> Comment in include/linux/netdevice.h clearly explains the purpose of this
> field.
> 
> wanted_features is supposed to be limited by hw_features (so that it's always
> true that (hw_features & wanted_features) == wanted_features). If you have
> an idea how to make that more clear, I'd be happy to update descriptions.

Then the computation of 'changed' in __ethtool_set_flags() is wrong:

	/* allow changing only bits set in hw_features */
	changed = (data ^ dev->wanted_features) & flags_dup_features;
	if (changed & ~dev->hw_features)
		return (changed & dev->hw_features) ? -EINVAL : -EOPNOTSUPP;

You need to add something like:

	/* Features that are requested to be on, are already on, and cannot
	 * be changed, have not changed.
	 */
	changes &= ~(data & dev->features & ~dev->hw_features);

It seems like there ought to be a way to simplify that, though!

Ben.

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

[PATCH] net: fix hw_features ethtool_ops->set_flags compatibility

[Michał Mirosław]

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 net/core/ethtool.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 13d79f5..a8c5b3e 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -532,7 +532,7 @@ static int ethtool_set_one_feature(struct net_device *dev,
 
 int __ethtool_set_flags(struct net_device *dev, u32 data)
 {
-	u32 changed;
+	u32 changed, forced;
 
 	if (data & ~flags_dup_features)
 		return -EINVAL;
@@ -546,7 +546,8 @@ int __ethtool_set_flags(struct net_device *dev, u32 data)
 	}
 
 	/* allow changing only bits set in hw_features */
-	changed = (data ^ dev->wanted_features) & flags_dup_features;
+	forced = dev->features & flags_dup_features & ~dev->hw_features;
+	changed = data ^ forced ^ dev->wanted_features;
 	if (changed & ~dev->hw_features)
 		return (changed & dev->hw_features) ? -EINVAL : -EOPNOTSUPP;
 
-- 
1.7.2.5

Re: [PATCH v5] net: bnx2x: convert to hw_features

[Michał Mirosław]

On Thu, Apr 21, 2011 at 11:52:22PM +0100, Ben Hutchings wrote:
> On Fri, 2011-04-22 at 00:15 +0200, Michał Mirosław wrote:
> > On Thu, Apr 21, 2011 at 08:19:19PM +0100, Ben Hutchings wrote:
> > > 	/* Transfer changeable features to wanted_features and enable
> > > 	 * software offloads (GSO and GRO).
> > > 	 */
> > > 	dev->hw_features |= NETIF_F_SOFT_FEATURES;
> > > 	dev->features |= NETIF_F_SOFT_FEATURES;
> > > 	dev->wanted_features = dev->features & dev->hw_features;
> > > 
> > > This doesn't work correctly for features that are always enabled, like
> > > NETIF_F_HW_VLAN_RX in bnx2x, which are set in dev->features but not in
> > > dev->hw_features.
> > 
> > > The name 'hw_features' really wasn't a good choice - the obvious meaning
> > > and the meaning assumed by this code is 'hardware-supported features'
> > > and not 'hardware-supported features that can be toggled'.  And since we
> > > add NETIF_F_SOFT_FEATURES, it really only means 'features that can be
> > > toggled'.
> > 
> > I won't argue about hw_features name - I just couldn't find a better one.
> > Comment in include/linux/netdevice.h clearly explains the purpose of this
> > field.
> > 
> > wanted_features is supposed to be limited by hw_features (so that it's always
> > true that (hw_features & wanted_features) == wanted_features). If you have
> > an idea how to make that more clear, I'd be happy to update descriptions.
> 
> Then the computation of 'changed' in __ethtool_set_flags() is wrong:
> 
> 	/* allow changing only bits set in hw_features */
> 	changed = (data ^ dev->wanted_features) & flags_dup_features;
> 	if (changed & ~dev->hw_features)
> 		return (changed & dev->hw_features) ? -EINVAL : -EOPNOTSUPP;

Yes! This doesn't take account of features enabled but not togglable.

> You need to add something like:
> 
> 	/* Features that are requested to be on, are already on, and cannot
> 	 * be changed, have not changed.
> 	 */
> 	changes &= ~(data & dev->features & ~dev->hw_features);
> 
> It seems like there ought to be a way to simplify that, though!

Maybe something I just sent will do.

Best Regards,
Michał Mirosław

[PATCH v2] net: fix hw_features ethtool_ops->set_flags compatibility

[Michał Mirosław]

__ethtool_set_flags() was not taking into account features set but not
user-toggleable.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 net/core/ethtool.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 13d79f5..ecef3d9 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -532,7 +532,7 @@ static int ethtool_set_one_feature(struct net_device *dev,
 
 int __ethtool_set_flags(struct net_device *dev, u32 data)
 {
-	u32 changed;
+	u32 changed, forced;
 
 	if (data & ~flags_dup_features)
 		return -EINVAL;
@@ -546,7 +546,8 @@ int __ethtool_set_flags(struct net_device *dev, u32 data)
 	}
 
 	/* allow changing only bits set in hw_features */
-	changed = (data ^ dev->wanted_features) & flags_dup_features;
+	forced = dev->features & ~dev->hw_features;
+	changed = (data ^ forced ^ dev->wanted_features) & flags_dup_features;
 	if (changed & ~dev->hw_features)
 		return (changed & dev->hw_features) ? -EINVAL : -EOPNOTSUPP;
 
-- 
1.7.2.5

[PATCH v3] net: fix hw_features ethtool_ops->set_flags compatibility

[Michał Mirosław]

__ethtool_set_flags() was not taking into account features set but not
user-toggleable.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
in v2 I forgot to 'stg refresh' before

 net/core/ethtool.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 13d79f5..ecef3d9 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -532,7 +532,7 @@ static int ethtool_set_one_feature(struct net_device *dev,
 
 int __ethtool_set_flags(struct net_device *dev, u32 data)
 {
-	u32 changed;
+	u32 changed, forced;
 
 	if (data & ~flags_dup_features)
 		return -EINVAL;
@@ -546,7 +546,8 @@ int __ethtool_set_flags(struct net_device *dev, u32 data)
 	}
 
 	/* allow changing only bits set in hw_features */
-	changed = (data ^ dev->wanted_features) & flags_dup_features;
+	forced = dev->features & ~dev->hw_features;
+	changed = (data ^ forced ^ dev->wanted_features) & flags_dup_features;
 	if (changed & ~dev->hw_features)
 		return (changed & dev->hw_features) ? -EINVAL : -EOPNOTSUPP;
 
-- 
1.7.2.5

Re: [PATCH v5] net: bnx2x: convert to hw_features

[Michał Mirosław]

On Thu, Apr 21, 2011 at 11:52:22PM +0100, Ben Hutchings wrote:
> On Fri, 2011-04-22 at 00:15 +0200, Michał Mirosław wrote:
> > On Thu, Apr 21, 2011 at 08:19:19PM +0100, Ben Hutchings wrote:
> > > 	/* Transfer changeable features to wanted_features and enable
> > > 	 * software offloads (GSO and GRO).
> > > 	 */
> > > 	dev->hw_features |= NETIF_F_SOFT_FEATURES;
> > > 	dev->features |= NETIF_F_SOFT_FEATURES;
> > > 	dev->wanted_features = dev->features & dev->hw_features;
> > > 
> > > This doesn't work correctly for features that are always enabled, like
> > > NETIF_F_HW_VLAN_RX in bnx2x, which are set in dev->features but not in
> > > dev->hw_features.
> > 
> > > The name 'hw_features' really wasn't a good choice - the obvious meaning
> > > and the meaning assumed by this code is 'hardware-supported features'
> > > and not 'hardware-supported features that can be toggled'.  And since we
> > > add NETIF_F_SOFT_FEATURES, it really only means 'features that can be
> > > toggled'.
> > 
> > I won't argue about hw_features name - I just couldn't find a better one.
> > Comment in include/linux/netdevice.h clearly explains the purpose of this
> > field.
> > 
> > wanted_features is supposed to be limited by hw_features (so that it's always
> > true that (hw_features & wanted_features) == wanted_features). If you have
> > an idea how to make that more clear, I'd be happy to update descriptions.
> 
> Then the computation of 'changed' in __ethtool_set_flags() is wrong:
> 
> 	/* allow changing only bits set in hw_features */
> 	changed = (data ^ dev->wanted_features) & flags_dup_features;
> 	if (changed & ~dev->hw_features)
> 		return (changed & dev->hw_features) ? -EINVAL : -EOPNOTSUPP;
> 
> You need to add something like:
> 
> 	/* Features that are requested to be on, are already on, and cannot
> 	 * be changed, have not changed.
> 	 */
> 	changes &= ~(data & dev->features & ~dev->hw_features);
> 
> It seems like there ought to be a way to simplify that, though!

I tried couple of variations, but your seems to be the best one. I'll send
it as a last try before I fall asleep.

Best Regards,
Michał Mirosław

[PATCH v4] net: fix hw_features ethtool_ops->set_flags compatibility

[Michał Mirosław]

__ethtool_set_flags() was not taking into account features set but not
user-toggleable.

Since GFLAGS returns masked dev->features, EINVAL is returned when
passed flags differ to it, and not to wanted_features.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
This should be the simpler way Ben was talking about.

 net/core/ethtool.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 13d79f5..d8b1a8d 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -546,12 +546,12 @@ int __ethtool_set_flags(struct net_device *dev, u32 data)
 	}
 
 	/* allow changing only bits set in hw_features */
-	changed = (data ^ dev->wanted_features) & flags_dup_features;
+	changed = (data ^ dev->features) & flags_dup_features;
 	if (changed & ~dev->hw_features)
 		return (changed & dev->hw_features) ? -EINVAL : -EOPNOTSUPP;
 
 	dev->wanted_features =
-		(dev->wanted_features & ~changed) | data;
+		(dev->wanted_features & ~changed) | (data & dev->hw_features);
 
 	__netdev_update_features(dev);
 
-- 
1.7.2.5

Re: [PATCH v4] net: fix hw_features ethtool_ops->set_flags compatibility

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Fri, 22 Apr 2011 01:59:21 +0200 (CEST)

> __ethtool_set_flags() was not taking into account features set but not
> user-toggleable.
> 
> Since GFLAGS returns masked dev->features, EINVAL is returned when
> passed flags differ to it, and not to wanted_features.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> This should be the simpler way Ben was talking about.

Applied, thanks.

Re: [PATCH v5] net: bnx2x: convert to hw_features

[Eric Dumazet]

Le vendredi 22 avril 2011 à 00:41 +0200, Michał Mirosław a écrit :
> On Thu, Apr 21, 2011 at 04:52:11PM +0200, Eric Dumazet wrote:
> > Le mardi 12 avril 2011 à 21:38 +0200, Michał Mirosław a écrit :
> > > Since ndo_fix_features callback is postponing features change when
> > > bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
> > > has to be called again when this condition changes. Previously,
> > > ethtool_ops->set_flags callback returned -EBUSY in that case
> > > (it's not possible in the new model).
> > > 
> > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > 
> > > v5: - don't delay set_features, as it's rtnl_locked - same as recovery process
> > > v4: - complete bp->rx_csum -> NETIF_F_RXCSUM conversion
> > >     - add check for failed ndo_set_features in ndo_open callback
> > > v3: - include NETIF_F_LRO in hw_features
> > >     - don't call netdev_update_features() if bnx2x_nic_load() failed
> > > v2: - comment in ndo_fix_features callback
> > > ---
> > I am not sure its related to these changes, but I now have in
> > net-next-2.6 :
> 
> > [   23.674263] ------------[ cut here ]------------
> > [   23.674266] WARNING: at net/core/dev.c:1318 dev_disable_lro+0x83/0x90()
> > [   23.674270] Hardware name: ProLiant BL460c G6
> > [   23.674273] Modules linked in: tg3 libphy sg
> > [   23.674280] Pid: 3070, comm: sysctl Tainted: G        W   2.6.39-rc2-01242-g3ef22b9-dirty #669
> > [   23.674282] Call Trace:
> > [   23.674285]  [<ffffffff813b94f3>] ? dev_disable_lro+0x83/0x90
> > [   23.674291]  [<ffffffff81042c9b>] warn_slowpath_common+0x8b/0xc0
> > [   23.674298]  [<ffffffff81042ce5>] warn_slowpath_null+0x15/0x20
> > [   23.674304]  [<ffffffff813b94f3>] dev_disable_lro+0x83/0x90
> > [   23.674309]  [<ffffffff81429789>] devinet_sysctl_forward+0x199/0x210
> [...]
> 
> Hmm. Looks like something is not allowing to disable LRO. Please check with
> following patch so we can be sure which driver causes this.
> 
> Best Regards,
> Michał Mirosław

Yes, obviously, and I suggested roughly the same patch some time ago.

Re: [PATCH] net: make WARN_ON in dev_disable_lro() useful

[Eric Dumazet]

Le vendredi 22 avril 2011 à 00:42 +0200, Michał Mirosław a écrit :
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  net/core/dev.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 3871bf6..3421184 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1315,7 +1315,8 @@ void dev_disable_lro(struct net_device *dev)
>  		return;
>  
>  	__ethtool_set_flags(dev, flags & ~ETH_FLAG_LRO);
> -	WARN_ON(dev->features & NETIF_F_LRO);
> +	if (unlikely(dev->features & NETIF_F_LRO))
> +		netdev_WARN(dev, "failed to disable LRO!\n");
>  }
>  EXPORT_SYMBOL(dev_disable_lro);
>  

Yes, as suggested one month ago
http://permalink.gmane.org/gmane.linux.network/189951

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

BTW, using unlikely() or likely() in slowpath is not exactly useful,
unless its integrated in macro (WARN_... )

Re: [PATCH v4] net: fix hw_features ethtool_ops->set_flags compatibility

[Eric Dumazet]

Le vendredi 22 avril 2011 à 01:59 +0200, Michał Mirosław a écrit :
> __ethtool_set_flags() was not taking into account features set but not
> user-toggleable.
> 
> Since GFLAGS returns masked dev->features, EINVAL is returned when
> passed flags differ to it, and not to wanted_features.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> This should be the simpler way Ben was talking about.
> 
>  net/core/ethtool.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 13d79f5..d8b1a8d 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -546,12 +546,12 @@ int __ethtool_set_flags(struct net_device *dev, u32 data)
>  	}
>  
>  	/* allow changing only bits set in hw_features */
> -	changed = (data ^ dev->wanted_features) & flags_dup_features;
> +	changed = (data ^ dev->features) & flags_dup_features;
>  	if (changed & ~dev->hw_features)
>  		return (changed & dev->hw_features) ? -EINVAL : -EOPNOTSUPP;
>  
>  	dev->wanted_features =
> -		(dev->wanted_features & ~changed) | data;
> +		(dev->wanted_features & ~changed) | (data & dev->hw_features);
>  
>  	__netdev_update_features(dev);
>  

Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Tested-by: Eric Dumazet <eric.dumazet@gmail.com>

Thanks, this solves the bnx2x problem !

Re: [PATCH v4] net: fix hw_features ethtool_ops->set_flags compatibility

[Eric Dumazet]

Le vendredi 22 avril 2011 à 07:16 +0200, Eric Dumazet a écrit :

> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Tested-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> Thanks, this solves the bnx2x problem !
> 

And already in net-next-2.6 ;)

Re: [PATCH] net: make WARN_ON in dev_disable_lro() useful

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 22 Apr 2011 07:11:53 +0200

> Le vendredi 22 avril 2011 à 00:42 +0200, Michał Mirosław a écrit :
>> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>> ---
>>  net/core/dev.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>> 
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 3871bf6..3421184 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -1315,7 +1315,8 @@ void dev_disable_lro(struct net_device *dev)
>>  		return;
>>  
>>  	__ethtool_set_flags(dev, flags & ~ETH_FLAG_LRO);
>> -	WARN_ON(dev->features & NETIF_F_LRO);
>> +	if (unlikely(dev->features & NETIF_F_LRO))
>> +		netdev_WARN(dev, "failed to disable LRO!\n");
>>  }
>>  EXPORT_SYMBOL(dev_disable_lro);
>>  
> 
> Yes, as suggested one month ago
> http://permalink.gmane.org/gmane.linux.network/189951
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied, thanks.