[net-next 0/5][pull request] Intel Wired LAN Driver Update

[net-next 0/5][pull request] Intel Wired LAN Driver Update

[Jeff Kirsher]

The following series contains updates to igb and ixgbe.

igb- a fix of adding a flush after a register write, the addition
    of SerDes forced mode support and a trivial update of copyright
    header.

ixgbe- conversion to ndo_fix_features and a fix to initialize
    fdir_perfect_lock in all cases

The following are changes since commit a05e42c27f80d341a5ec2053b041e24231a002e2:
  Merge branch 'for-davem' of git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-next-2.6
and are available in the git repository at:
  master.kernel.org:/pub/scm/linux/kernel/git/jkirsher/net-next-2.6 master


Alexander Duyck (1):
  ixgbe: Make certain to initialize the fdir_perfect_lock in all cases

Carolyn Wyborny (3):
  igb: Fix lack of flush after register write and before delay
  igb: Update copyright on all igb driver files.
  igb: Add support of SerDes Forced mode for certain hardware

Don Skidmore (1):
  ixgbe: convert to ndo_fix_features

 drivers/net/igb/Makefile          |    2 +-
 drivers/net/igb/e1000_82575.c     |   22 ++++-
 drivers/net/igb/e1000_82575.h     |    4 +-
 drivers/net/igb/e1000_defines.h   |    7 +-
 drivers/net/igb/e1000_hw.h        |    2 +-
 drivers/net/igb/e1000_mac.c       |    2 +-
 drivers/net/igb/e1000_mac.h       |    2 +-
 drivers/net/igb/e1000_mbx.c       |    2 +-
 drivers/net/igb/e1000_mbx.h       |    2 +-
 drivers/net/igb/e1000_nvm.c       |    2 +-
 drivers/net/igb/e1000_nvm.h       |    2 +-
 drivers/net/igb/e1000_phy.c       |    2 +-
 drivers/net/igb/e1000_phy.h       |    2 +-
 drivers/net/igb/e1000_regs.h      |    2 +-
 drivers/net/igb/igb.h             |    2 +-
 drivers/net/igb/igb_ethtool.c     |    2 +-
 drivers/net/igb/igb_main.c        |    2 +-
 drivers/net/ixgbe/ixgbe.h         |    1 +
 drivers/net/ixgbe/ixgbe_ethtool.c |  188 -------------------------------------
 drivers/net/ixgbe/ixgbe_main.c    |  110 ++++++++++++++++++++--
 20 files changed, 142 insertions(+), 218 deletions(-)

-- 
1.7.6

[net-next 1/5] igb: Fix lack of flush after register write and before delay

[Jeff Kirsher]

From: Carolyn Wyborny <carolyn.wyborny@intel.com>

Register writes followed by a delay are required to have a flush
before the delay in order to commit the values to the register.  Without
the flush, the code following the delay may not function correctly.

Reported-by: Tong Ho <tong.ho@ericsson.com>
Reported-by: Guenter Roeck <guenter.roeck@ericsson.com>
Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com>
Tested-by:  Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/igb/e1000_82575.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/igb/e1000_82575.c b/drivers/net/igb/e1000_82575.c
index 0f563c8..493e331 100644
--- a/drivers/net/igb/e1000_82575.c
+++ b/drivers/net/igb/e1000_82575.c
@@ -1735,6 +1735,7 @@ static s32 igb_reset_hw_82580(struct e1000_hw *hw)
 		ctrl |= E1000_CTRL_RST;
 
 	wr32(E1000_CTRL, ctrl);
+	wrfl();
 
 	/* Add delay to insure DEV_RST has time to complete */
 	if (global_device_reset)
-- 
1.7.6

[net-next 2/5] igb: Update copyright on all igb driver files.

[Jeff Kirsher]

From: Carolyn Wyborny <carolyn.wyborny@intel.com>

This patch updates the copyright on the igb driver files to 2011.

Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com>
Tested-by: Aaron Brown  <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/igb/Makefile        |    2 +-
 drivers/net/igb/e1000_82575.c   |    2 +-
 drivers/net/igb/e1000_82575.h   |    2 +-
 drivers/net/igb/e1000_defines.h |    2 +-
 drivers/net/igb/e1000_hw.h      |    2 +-
 drivers/net/igb/e1000_mac.c     |    2 +-
 drivers/net/igb/e1000_mac.h     |    2 +-
 drivers/net/igb/e1000_mbx.c     |    2 +-
 drivers/net/igb/e1000_mbx.h     |    2 +-
 drivers/net/igb/e1000_nvm.c     |    2 +-
 drivers/net/igb/e1000_nvm.h     |    2 +-
 drivers/net/igb/e1000_phy.c     |    2 +-
 drivers/net/igb/e1000_phy.h     |    2 +-
 drivers/net/igb/e1000_regs.h    |    2 +-
 drivers/net/igb/igb.h           |    2 +-
 drivers/net/igb/igb_ethtool.c   |    2 +-
 drivers/net/igb/igb_main.c      |    2 +-
 17 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/net/igb/Makefile b/drivers/net/igb/Makefile
index 8372cb9..c6e4621 100644
--- a/drivers/net/igb/Makefile
+++ b/drivers/net/igb/Makefile
@@ -1,7 +1,7 @@
 ################################################################################
 #
 # Intel 82575 PCI-Express Ethernet Linux driver
-# Copyright(c) 1999 - 2009 Intel Corporation.
+# Copyright(c) 1999 - 2011 Intel Corporation.
 #
 # This program is free software; you can redistribute it and/or modify it
 # under the terms and conditions of the GNU General Public License,
diff --git a/drivers/net/igb/e1000_82575.c b/drivers/net/igb/e1000_82575.c
index 493e331..7b7e157 100644
--- a/drivers/net/igb/e1000_82575.c
+++ b/drivers/net/igb/e1000_82575.c
@@ -1,7 +1,7 @@
 /*******************************************************************************
 
   Intel(R) Gigabit Ethernet Linux driver
-  Copyright(c) 2007-2009 Intel Corporation.
+  Copyright(c) 2007-2011 Intel Corporation.
 
   This program is free software; you can redistribute it and/or modify it
   under the terms and conditions of the GNU General Public License,
diff --git a/drivers/net/igb/e1000_82575.h b/drivers/net/igb/e1000_82575.h
index dd6df34..fd28d62 100644
--- a/drivers/net/igb/e1000_82575.h
+++ b/drivers/net/igb/e1000_82575.h
@@ -1,7 +1,7 @@
 /*******************************************************************************
 
   Intel(R) Gigabit Ethernet Linux driver
-  Copyright(c) 2007-2009 Intel Corporation.
+  Copyright(c) 2007-2011 Intel Corporation.
 
   This program is free software; you can redistribute it and/or modify it
   under the terms and conditions of the GNU General Public License,
diff --git a/drivers/net/igb/e1000_defines.h b/drivers/net/igb/e1000_defines.h
index 6b80d40..446eb5c 100644
--- a/drivers/net/igb/e1000_defines.h
+++ b/drivers/net/igb/e1000_defines.h
@@ -1,7 +1,7 @@
 /*******************************************************************************
 
   Intel(R) Gigabit Ethernet Linux driver
-  Copyright(c) 2007-2009 Intel Corporation.
+  Copyright(c) 2007-2011 Intel Corporation.
 
   This program is free software; you can redistribute it and/or modify it
   under the terms and conditions of the GNU General Public License,
diff --git a/drivers/net/igb/e1000_hw.h b/drivers/net/igb/e1000_hw.h
index 27153e8..4519a13 100644
--- a/drivers/net/igb/e1000_hw.h
+++ b/drivers/net/igb/e1000_hw.h
@@ -1,7 +1,7 @@
 /*******************************************************************************
 
   Intel(R) Gigabit Ethernet Linux driver
-  Copyright(c) 2007-2009 Intel Corporation.
+  Copyright(c) 2007-2011 Intel Corporation.
 
   This program is free software; you can redistribute it and/or modify it
   under the terms and conditions of the GNU General Public License,
diff --git a/drivers/net/igb/e1000_mac.c b/drivers/net/igb/e1000_mac.c
index c822904..2b5ef76 100644
--- a/drivers/net/igb/e1000_mac.c
+++ b/drivers/net/igb/e1000_mac.c
@@ -1,7 +1,7 @@
 /*******************************************************************************
 
   Intel(R) Gigabit Ethernet Linux driver
-  Copyright(c) 2007-2009 Intel Corporation.
+  Copyright(c) 2007-2011 Intel Corporation.
 
   This program is free software; you can redistribute it and/or modify it
   under the terms and conditions of the GNU General Public License,
diff --git a/drivers/net/igb/e1000_mac.h b/drivers/net/igb/e1000_mac.h
index 601be99..4927f61 100644
--- a/drivers/net/igb/e1000_mac.h
+++ b/drivers/net/igb/e1000_mac.h
@@ -1,7 +1,7 @@
 /*******************************************************************************
 
   Intel(R) Gigabit Ethernet Linux driver
-  Copyright(c) 2007-2009 Intel Corporation.
+  Copyright(c) 2007-2011 Intel Corporation.
 
   This program is free software; you can redistribute it and/or modify it
   under the terms and conditions of the GNU General Public License,
diff --git a/drivers/net/igb/e1000_mbx.c b/drivers/net/igb/e1000_mbx.c
index 78d48c7..74f2f11 100644
--- a/drivers/net/igb/e1000_mbx.c
+++ b/drivers/net/igb/e1000_mbx.c
@@ -1,7 +1,7 @@
 /*******************************************************************************
 
   Intel(R) Gigabit Ethernet Linux driver
-  Copyright(c) 2007-2009 Intel Corporation.
+  Copyright(c) 2007-2011 Intel Corporation.
 
   This program is free software; you can redistribute it and/or modify it
   under the terms and conditions of the GNU General Public License,
diff --git a/drivers/net/igb/e1000_mbx.h b/drivers/net/igb/e1000_mbx.h
index bb112fb..eddb0f8 100644
--- a/drivers/net/igb/e1000_mbx.h
+++ b/drivers/net/igb/e1000_mbx.h
@@ -1,7 +1,7 @@
 /*******************************************************************************
 
   Intel(R) Gigabit Ethernet Linux driver
-  Copyright(c) 2007-2009 Intel Corporation.
+  Copyright(c) 2007-2011 Intel Corporation.
 
   This program is free software; you can redistribute it and/or modify it
   under the terms and conditions of the GNU General Public License,
diff --git a/drivers/net/igb/e1000_nvm.c b/drivers/net/igb/e1000_nvm.c
index 75bf36a..7dcd65c 100644
--- a/drivers/net/igb/e1000_nvm.c
+++ b/drivers/net/igb/e1000_nvm.c
@@ -1,7 +1,7 @@
 /*******************************************************************************
 
   Intel(R) Gigabit Ethernet Linux driver
-  Copyright(c) 2007-2009 Intel Corporation.
+  Copyright(c) 2007-2011 Intel Corporation.
 
   This program is free software; you can redistribute it and/or modify it
   under the terms and conditions of the GNU General Public License,
diff --git a/drivers/net/igb/e1000_nvm.h b/drivers/net/igb/e1000_nvm.h
index 7f43564..a2a7ca9 100644
--- a/drivers/net/igb/e1000_nvm.h
+++ b/drivers/net/igb/e1000_nvm.h
@@ -1,7 +1,7 @@
 /*******************************************************************************
 
   Intel(R) Gigabit Ethernet Linux driver
-  Copyright(c) 2007 Intel Corporation.
+  Copyright(c) 2011 Intel Corporation.
 
   This program is free software; you can redistribute it and/or modify it
   under the terms and conditions of the GNU General Public License,
diff --git a/drivers/net/igb/e1000_phy.c b/drivers/net/igb/e1000_phy.c
index d639706..e662554 100644
--- a/drivers/net/igb/e1000_phy.c
+++ b/drivers/net/igb/e1000_phy.c
@@ -1,7 +1,7 @@
 /*******************************************************************************
 
   Intel(R) Gigabit Ethernet Linux driver
-  Copyright(c) 2007-2009 Intel Corporation.
+  Copyright(c) 2007-2011 Intel Corporation.
 
   This program is free software; you can redistribute it and/or modify it
   under the terms and conditions of the GNU General Public License,
diff --git a/drivers/net/igb/e1000_phy.h b/drivers/net/igb/e1000_phy.h
index 2cc1177..8510797 100644
--- a/drivers/net/igb/e1000_phy.h
+++ b/drivers/net/igb/e1000_phy.h
@@ -1,7 +1,7 @@
 /*******************************************************************************
 
   Intel(R) Gigabit Ethernet Linux driver
-  Copyright(c) 2007-2009 Intel Corporation.
+  Copyright(c) 2007-2011 Intel Corporation.
 
   This program is free software; you can redistribute it and/or modify it
   under the terms and conditions of the GNU General Public License,
diff --git a/drivers/net/igb/e1000_regs.h b/drivers/net/igb/e1000_regs.h
index 958ca3b..0990f6d 100644
--- a/drivers/net/igb/e1000_regs.h
+++ b/drivers/net/igb/e1000_regs.h
@@ -1,7 +1,7 @@
 /*******************************************************************************
 
   Intel(R) Gigabit Ethernet Linux driver
-  Copyright(c) 2007-2009 Intel Corporation.
+  Copyright(c) 2007-2011 Intel Corporation.
 
   This program is free software; you can redistribute it and/or modify it
   under the terms and conditions of the GNU General Public License,
diff --git a/drivers/net/igb/igb.h b/drivers/net/igb/igb.h
index f4fa4b1..0389ff6 100644
--- a/drivers/net/igb/igb.h
+++ b/drivers/net/igb/igb.h
@@ -1,7 +1,7 @@
 /*******************************************************************************
 
   Intel(R) Gigabit Ethernet Linux driver
-  Copyright(c) 2007-2009 Intel Corporation.
+  Copyright(c) 2007-2011 Intel Corporation.
 
   This program is free software; you can redistribute it and/or modify it
   under the terms and conditions of the GNU General Public License,
diff --git a/drivers/net/igb/igb_ethtool.c b/drivers/net/igb/igb_ethtool.c
index 1862c97..ed63ff4 100644
--- a/drivers/net/igb/igb_ethtool.c
+++ b/drivers/net/igb/igb_ethtool.c
@@ -1,7 +1,7 @@
 /*******************************************************************************
 
   Intel(R) Gigabit Ethernet Linux driver
-  Copyright(c) 2007-2009 Intel Corporation.
+  Copyright(c) 2007-2011 Intel Corporation.
 
   This program is free software; you can redistribute it and/or modify it
   under the terms and conditions of the GNU General Public License,
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index d6c4bd8..f4d82b2 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -1,7 +1,7 @@
 /*******************************************************************************
 
   Intel(R) Gigabit Ethernet Linux driver
-  Copyright(c) 2007-2009 Intel Corporation.
+  Copyright(c) 2007-2011 Intel Corporation.
 
   This program is free software; you can redistribute it and/or modify it
   under the terms and conditions of the GNU General Public License,
-- 
1.7.6

[net-next 3/5] igb: Add support of SerDes Forced mode for certain hardware

[Jeff Kirsher]

From: Carolyn Wyborny <carolyn.wyborny@intel.com>

This patch changes the serdes link code to support a forced mode for
some hardware, based on bit set in EEPROM.

Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com>
Tested-by: Jeff Pieper <jeffrey.e.pieper@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/igb/e1000_82575.c   |   19 +++++++++++++++++--
 drivers/net/igb/e1000_82575.h   |    2 ++
 drivers/net/igb/e1000_defines.h |    5 +++--
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/net/igb/e1000_82575.c b/drivers/net/igb/e1000_82575.c
index 7b7e157..c0857bd 100644
--- a/drivers/net/igb/e1000_82575.c
+++ b/drivers/net/igb/e1000_82575.c
@@ -1156,10 +1156,13 @@ static s32 igb_setup_serdes_link_82575(struct e1000_hw *hw)
 {
 	u32 ctrl_ext, ctrl_reg, reg;
 	bool pcs_autoneg;
+	s32 ret_val = E1000_SUCCESS;
+	u16 data;
 
 	if ((hw->phy.media_type != e1000_media_type_internal_serdes) &&
 	    !igb_sgmii_active_82575(hw))
-		return 0;
+		return ret_val;
+
 
 	/*
 	 * On the 82575, SerDes loopback mode persists until it is
@@ -1203,6 +1206,18 @@ static s32 igb_setup_serdes_link_82575(struct e1000_hw *hw)
 		/* disable PCS autoneg and support parallel detect only */
 		pcs_autoneg = false;
 	default:
+		if (hw->mac.type == e1000_82575 ||
+		    hw->mac.type == e1000_82576) {
+			ret_val = hw->nvm.ops.read(hw, NVM_COMPAT, 1, &data);
+			if (ret_val) {
+				printk(KERN_DEBUG "NVM Read Error\n\n");
+				return ret_val;
+			}
+
+			if (data & E1000_EEPROM_PCS_AUTONEG_DISABLE_BIT)
+				pcs_autoneg = false;
+		}
+
 		/*
 		 * non-SGMII modes only supports a speed of 1000/Full for the
 		 * link so it is best to just force the MAC and let the pcs
@@ -1250,7 +1265,7 @@ static s32 igb_setup_serdes_link_82575(struct e1000_hw *hw)
 	if (!igb_sgmii_active_82575(hw))
 		igb_force_mac_fc(hw);
 
-	return 0;
+	return ret_val;
 }
 
 /**
diff --git a/drivers/net/igb/e1000_82575.h b/drivers/net/igb/e1000_82575.h
index fd28d62..786e110 100644
--- a/drivers/net/igb/e1000_82575.h
+++ b/drivers/net/igb/e1000_82575.h
@@ -243,6 +243,8 @@ struct e1000_adv_tx_context_desc {
 #define E1000_DTXCTL_MDP_EN     0x0020
 #define E1000_DTXCTL_SPOOF_INT  0x0040
 
+#define E1000_EEPROM_PCS_AUTONEG_DISABLE_BIT	(1 << 14)
+
 #define ALL_QUEUES   0xFFFF
 
 /* RX packet buffer size defines */
diff --git a/drivers/net/igb/e1000_defines.h b/drivers/net/igb/e1000_defines.h
index 446eb5c..2cd4082 100644
--- a/drivers/net/igb/e1000_defines.h
+++ b/drivers/net/igb/e1000_defines.h
@@ -437,6 +437,7 @@
 #define E1000_RAH_POOL_1 0x00040000
 
 /* Error Codes */
+#define E1000_SUCCESS      0
 #define E1000_ERR_NVM      1
 #define E1000_ERR_PHY      2
 #define E1000_ERR_CONFIG   3
@@ -587,8 +588,8 @@
 #define E1000_NVM_POLL_READ     0    /* Flag for polling for read complete */
 
 /* NVM Word Offsets */
-#define NVM_ID_LED_SETTINGS        0x0004
-/* For SERDES output amplitude adjustment. */
+#define NVM_COMPAT                 0x0003
+#define NVM_ID_LED_SETTINGS        0x0004 /* SERDES output amplitude */
 #define NVM_INIT_CONTROL2_REG      0x000F
 #define NVM_INIT_CONTROL3_PORT_B   0x0014
 #define NVM_INIT_CONTROL3_PORT_A   0x0024
-- 
1.7.6

[net-next 4/5] ixgbe: Make certain to initialize the fdir_perfect_lock in all cases

[Jeff Kirsher]

From: Alexander Duyck <alexander.h.duyck@intel.com>

This fix makes it so that the fdir_perfect_lock is initialized in all
cases.  This is necessary as the fdir_filter_exit routine will always
attempt to take the lock before inspecting the filter table.

Reported-by: Ben Greear <greearb@candelatech.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ixgbe/ixgbe_main.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index fa671ae..de30796 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -5155,8 +5155,6 @@ static int __devinit ixgbe_sw_init(struct ixgbe_adapter *adapter)
 		adapter->flags2 |= IXGBE_FLAG2_RSC_ENABLED;
 		if (hw->device_id == IXGBE_DEV_ID_82599_T3_LOM)
 			adapter->flags2 |= IXGBE_FLAG2_TEMP_SENSOR_CAPABLE;
-		/* n-tuple support exists, always init our spinlock */
-		spin_lock_init(&adapter->fdir_perfect_lock);
 		/* Flow Director hash filters enabled */
 		adapter->flags |= IXGBE_FLAG_FDIR_HASH_CAPABLE;
 		adapter->atr_sample_rate = 20;
@@ -5177,6 +5175,9 @@ static int __devinit ixgbe_sw_init(struct ixgbe_adapter *adapter)
 		break;
 	}
 
+	/* n-tuple support exists, always init our spinlock */
+	spin_lock_init(&adapter->fdir_perfect_lock);
+
 #ifdef CONFIG_IXGBE_DCB
 	/* Configure DCB traffic classes */
 	for (j = 0; j < MAX_TRAFFIC_CLASS; j++) {
-- 
1.7.6

[net-next 5/5] ixgbe: convert to ndo_fix_features

[Jeff Kirsher]

From: Don Skidmore <donald.c.skidmore@intel.com>

Private rx_csum flags are now duplicate of netdev->features &
NETIF_F_RXCSUM.  We remove those duplicates and now use the net_device_ops
ndo_set_features.  This was based on the original patch submitted by
Michal Miroslaw <mirq-linux@rere.qmqm.pl>.  I also removed the special
case not requiring a reset for X540 hardware.  It is needed just as it is
in 82599 hardware.

Signed-off-by: Don Skidmore <donald.c.skidmore@intel.com>
Cc:  Michal Miroslaw <mirq-linux@rere.qmqm.pl>
Tested-by: Phillip Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ixgbe/ixgbe.h         |    1 +
 drivers/net/ixgbe/ixgbe_ethtool.c |  188 -------------------------------------
 drivers/net/ixgbe/ixgbe_main.c    |  105 +++++++++++++++++++--
 3 files changed, 99 insertions(+), 195 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe.h b/drivers/net/ixgbe/ixgbe.h
index 744b641..2b96d8d 100644
--- a/drivers/net/ixgbe/ixgbe.h
+++ b/drivers/net/ixgbe/ixgbe.h
@@ -588,6 +588,7 @@ extern void ixgbe_clear_rscctl(struct ixgbe_adapter *adapter,
 extern void ixgbe_set_rx_mode(struct net_device *netdev);
 extern int ixgbe_setup_tc(struct net_device *dev, u8 tc);
 extern void ixgbe_tx_ctxtdesc(struct ixgbe_ring *, u32, u32, u32, u32);
+extern void ixgbe_do_reset(struct net_device *netdev);
 #ifdef IXGBE_FCOE
 extern void ixgbe_configure_fcoe(struct ixgbe_adapter *adapter);
 extern int ixgbe_fso(struct ixgbe_ring *tx_ring, struct sk_buff *skb,
diff --git a/drivers/net/ixgbe/ixgbe_ethtool.c b/drivers/net/ixgbe/ixgbe_ethtool.c
index 074e9ba..06b7f88 100644
--- a/drivers/net/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ixgbe/ixgbe_ethtool.c
@@ -442,109 +442,6 @@ static int ixgbe_set_pauseparam(struct net_device *netdev,
 	return 0;
 }
 
-static void ixgbe_do_reset(struct net_device *netdev)
-{
-	struct ixgbe_adapter *adapter = netdev_priv(netdev);
-
-	if (netif_running(netdev))
-		ixgbe_reinit_locked(adapter);
-	else
-		ixgbe_reset(adapter);
-}
-
-static u32 ixgbe_get_rx_csum(struct net_device *netdev)
-{
-	struct ixgbe_adapter *adapter = netdev_priv(netdev);
-	return adapter->flags & IXGBE_FLAG_RX_CSUM_ENABLED;
-}
-
-static void ixgbe_set_rsc(struct ixgbe_adapter *adapter)
-{
-	int i;
-
-	for (i = 0; i < adapter->num_rx_queues; i++) {
-		struct ixgbe_ring *ring = adapter->rx_ring[i];
-		if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) {
-			set_ring_rsc_enabled(ring);
-			ixgbe_configure_rscctl(adapter, ring);
-		} else {
-			ixgbe_clear_rscctl(adapter, ring);
-		}
-	}
-}
-
-static int ixgbe_set_rx_csum(struct net_device *netdev, u32 data)
-{
-	struct ixgbe_adapter *adapter = netdev_priv(netdev);
-	bool need_reset = false;
-
-	if (data) {
-		adapter->flags |= IXGBE_FLAG_RX_CSUM_ENABLED;
-	} else {
-		adapter->flags &= ~IXGBE_FLAG_RX_CSUM_ENABLED;
-
-		if (adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE) {
-			adapter->flags2 &= ~IXGBE_FLAG2_RSC_ENABLED;
-			netdev->features &= ~NETIF_F_LRO;
-		}
-
-		switch (adapter->hw.mac.type) {
-		case ixgbe_mac_X540:
-			ixgbe_set_rsc(adapter);
-			break;
-		case ixgbe_mac_82599EB:
-			need_reset = true;
-			break;
-		default:
-			break;
-		}
-	}
-
-	if (need_reset)
-		ixgbe_do_reset(netdev);
-
-	return 0;
-}
-
-static u32 ixgbe_get_tx_csum(struct net_device *netdev)
-{
-	return (netdev->features & NETIF_F_IP_CSUM) != 0;
-}
-
-static int ixgbe_set_tx_csum(struct net_device *netdev, u32 data)
-{
-	struct ixgbe_adapter *adapter = netdev_priv(netdev);
-	u32 feature_list;
-
-	feature_list = (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM);
-	switch (adapter->hw.mac.type) {
-	case ixgbe_mac_82599EB:
-	case ixgbe_mac_X540:
-		feature_list |= NETIF_F_SCTP_CSUM;
-		break;
-	default:
-		break;
-	}
-	if (data)
-		netdev->features |= feature_list;
-	else
-		netdev->features &= ~feature_list;
-
-	return 0;
-}
-
-static int ixgbe_set_tso(struct net_device *netdev, u32 data)
-{
-	if (data) {
-		netdev->features |= NETIF_F_TSO;
-		netdev->features |= NETIF_F_TSO6;
-	} else {
-		netdev->features &= ~NETIF_F_TSO;
-		netdev->features &= ~NETIF_F_TSO6;
-	}
-	return 0;
-}
-
 static u32 ixgbe_get_msglevel(struct net_device *netdev)
 {
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
@@ -2287,81 +2184,6 @@ static int ixgbe_set_coalesce(struct net_device *netdev,
 	return 0;
 }
 
-static int ixgbe_set_flags(struct net_device *netdev, u32 data)
-{
-	struct ixgbe_adapter *adapter = netdev_priv(netdev);
-	bool need_reset = false;
-	int rc;
-
-#ifdef CONFIG_IXGBE_DCB
-	if ((adapter->flags & IXGBE_FLAG_DCB_ENABLED) &&
-	    !(data & ETH_FLAG_RXVLAN))
-		return -EINVAL;
-#endif
-
-	need_reset = (data & ETH_FLAG_RXVLAN) !=
-		     (netdev->features & NETIF_F_HW_VLAN_RX);
-
-	if ((data & ETH_FLAG_RXHASH) &&
-	    !(adapter->flags & IXGBE_FLAG_RSS_ENABLED))
-		return -EOPNOTSUPP;
-
-	rc = ethtool_op_set_flags(netdev, data, ETH_FLAG_LRO | ETH_FLAG_NTUPLE |
-				  ETH_FLAG_RXVLAN | ETH_FLAG_TXVLAN |
-				  ETH_FLAG_RXHASH);
-	if (rc)
-		return rc;
-
-	/* if state changes we need to update adapter->flags and reset */
-	if ((adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE) &&
-	    (!!(data & ETH_FLAG_LRO) !=
-	     !!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED))) {
-		if ((data & ETH_FLAG_LRO) &&
-		    (!adapter->rx_itr_setting ||
-		     (adapter->rx_itr_setting > IXGBE_MAX_RSC_INT_RATE))) {
-			e_info(probe, "rx-usecs set too low, "
-				      "not enabling RSC.\n");
-		} else {
-			adapter->flags2 ^= IXGBE_FLAG2_RSC_ENABLED;
-			switch (adapter->hw.mac.type) {
-			case ixgbe_mac_X540:
-				ixgbe_set_rsc(adapter);
-				break;
-			case ixgbe_mac_82599EB:
-				need_reset = true;
-				break;
-			default:
-				break;
-			}
-		}
-	}
-
-	/*
-	 * Check if Flow Director n-tuple support was enabled or disabled.  If
-	 * the state changed, we need to reset.
-	 */
-	if (!(adapter->flags & IXGBE_FLAG_FDIR_PERFECT_CAPABLE)) {
-		/* turn off ATR, enable perfect filters and reset */
-		if (data & ETH_FLAG_NTUPLE) {
-			adapter->flags &= ~IXGBE_FLAG_FDIR_HASH_CAPABLE;
-			adapter->flags |= IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
-			need_reset = true;
-		}
-	} else if (!(data & ETH_FLAG_NTUPLE)) {
-		/* turn off Flow Director, set ATR and reset */
-		adapter->flags &= ~IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
-		if ((adapter->flags & IXGBE_FLAG_RSS_ENABLED) &&
-		    !(adapter->flags & IXGBE_FLAG_DCB_ENABLED))
-			adapter->flags |= IXGBE_FLAG_FDIR_HASH_CAPABLE;
-		need_reset = true;
-	}
-
-	if (need_reset)
-		ixgbe_do_reset(netdev);
-
-	return 0;
-}
-
 static int ixgbe_get_ethtool_fdir_entry(struct ixgbe_adapter *adapter,
 					struct ethtool_rxnfc *cmd)
 {
@@ -2744,16 +2566,8 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
 	.set_ringparam          = ixgbe_set_ringparam,
 	.get_pauseparam         = ixgbe_get_pauseparam,
 	.set_pauseparam         = ixgbe_set_pauseparam,
-	.get_rx_csum            = ixgbe_get_rx_csum,
-	.set_rx_csum            = ixgbe_set_rx_csum,
-	.get_tx_csum            = ixgbe_get_tx_csum,
-	.set_tx_csum            = ixgbe_set_tx_csum,
-	.get_sg                 = ethtool_op_get_sg,
-	.set_sg                 = ethtool_op_set_sg,
 	.get_msglevel           = ixgbe_get_msglevel,
 	.set_msglevel           = ixgbe_set_msglevel,
-	.get_tso                = ethtool_op_get_tso,
-	.set_tso                = ixgbe_set_tso,
 	.self_test              = ixgbe_diag_test,
 	.get_strings            = ixgbe_get_strings,
 	.set_phys_id            = ixgbe_set_phys_id,
@@ -2761,8 +2575,6 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
 	.get_ethtool_stats      = ixgbe_get_ethtool_stats,
 	.get_coalesce           = ixgbe_get_coalesce,
 	.set_coalesce           = ixgbe_set_coalesce,
-	.get_flags              = ethtool_op_get_flags,
-	.set_flags              = ixgbe_set_flags,
 	.get_rxnfc		= ixgbe_get_rxnfc,
 	.set_rxnfc		= ixgbe_set_rxnfc,
 };
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index de30796..206c069 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -7188,6 +7188,88 @@ int ixgbe_setup_tc(struct net_device *dev, u8 tc)
 	return 0;
 }
 
+void ixgbe_do_reset(struct net_device *netdev)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(netdev);
+
+	if (netif_running(netdev))
+		ixgbe_reinit_locked(adapter);
+	else
+		ixgbe_reset(adapter);
+}
+
+static int ixgbe_set_features(struct net_device *netdev, u32 data)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(netdev);
+	bool need_reset = false;
+
+#ifdef CONFIG_DCB
+	if ((adapter->flags & IXGBE_FLAG_DCB_ENABLED) &&
+	    !(data &  NETIF_F_HW_VLAN_RX))
+		return -EINVAL;
+#endif
+	/* return error if RXHASH is being enabled when RSS is not supported */
+	if (!(adapter->flags & IXGBE_FLAG_RSS_ENABLED) &&
+	     (data &  NETIF_F_RXHASH))
+		return -EINVAL;
+
+	/* If Rx checksum is disabled, then RSC/LRO should also be disabled */
+	if (!(data & NETIF_F_RXCSUM)) {
+		data &= ~NETIF_F_LRO;
+		adapter->flags &= ~IXGBE_FLAG_RX_CSUM_ENABLED;
+	} else {
+		adapter->flags |= IXGBE_FLAG_RX_CSUM_ENABLED;
+	}
+
+	/* if state changes we need to update adapter->flags and reset */
+	if ((adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE) &&
+	    (!!(data & NETIF_F_LRO) !=
+	     !!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED))) {
+		if ((data &  NETIF_F_LRO) &&
+		    adapter->rx_itr_setting != 1 &&
+		    adapter->rx_itr_setting > IXGBE_MAX_RSC_INT_RATE) {
+			e_info(probe, "rx-usecs set too low, "
+			       "not enabling RSC\n");
+		} else {
+			adapter->flags2 ^= IXGBE_FLAG2_RSC_ENABLED;
+			switch (adapter->hw.mac.type) {
+			case ixgbe_mac_X540:
+			case ixgbe_mac_82599EB:
+				need_reset = true;
+				break;
+			default:
+				break;
+			}
+		}
+	}
+
+	/*
+	 * Check if Flow Director n-tuple support was enabled or disabled.  If
+	 * the state changed, we need to reset.
+	 */
+	if (!(adapter->flags & IXGBE_FLAG_FDIR_PERFECT_CAPABLE)) {
+		/* turn off ATR, enable perfect filters and reset */
+		if (data & NETIF_F_NTUPLE) {
+			adapter->flags &= ~IXGBE_FLAG_FDIR_HASH_CAPABLE;
+			adapter->flags |= IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
+			need_reset = true;
+		}
+	} else if (!(data & NETIF_F_NTUPLE)) {
+		/* turn off Flow Director, set ATR and reset */
+		adapter->flags &= ~IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
+		if ((adapter->flags &  IXGBE_FLAG_RSS_ENABLED) &&
+		    !(adapter->flags &  IXGBE_FLAG_DCB_ENABLED))
+			adapter->flags |= IXGBE_FLAG_FDIR_HASH_CAPABLE;
+		need_reset = true;
+	}
+
+	if (need_reset)
+		ixgbe_do_reset(netdev);
+
+	return 0;
+
+}
+
 static const struct net_device_ops ixgbe_netdev_ops = {
 	.ndo_open		= ixgbe_open,
 	.ndo_stop		= ixgbe_close,
@@ -7219,6 +7301,7 @@ static const struct net_device_ops ixgbe_netdev_ops = {
 	.ndo_fcoe_disable = ixgbe_fcoe_disable,
 	.ndo_fcoe_get_wwn = ixgbe_fcoe_get_wwn,
 #endif /* IXGBE_FCOE */
+	.ndo_set_features = ixgbe_set_features,
 };
 
 static void __devinit ixgbe_probe_vf(struct ixgbe_adapter *adapter,
@@ -7486,20 +7569,24 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
 
 	netdev->features = NETIF_F_SG |
 			   NETIF_F_IP_CSUM |
+			   NETIF_F_IPV6_CSUM |
 			   NETIF_F_HW_VLAN_TX |
 			   NETIF_F_HW_VLAN_RX |
-			   NETIF_F_HW_VLAN_FILTER;
+			   NETIF_F_HW_VLAN_FILTER |
+			   NETIF_F_TSO |
+			   NETIF_F_TSO6 |
+			   NETIF_F_GRO |
+			   NETIF_F_RXHASH |
+			   NETIF_F_RXCSUM;
 
-	netdev->features |= NETIF_F_IPV6_CSUM;
-	netdev->features |= NETIF_F_TSO;
-	netdev->features |= NETIF_F_TSO6;
-	netdev->features |= NETIF_F_GRO;
-	netdev->features |= NETIF_F_RXHASH;
+	netdev->hw_features = netdev->features;
 
 	switch (adapter->hw.mac.type) {
 	case ixgbe_mac_82599EB:
 	case ixgbe_mac_X540:
 		netdev->features |= NETIF_F_SCTP_CSUM;
+		netdev->hw_features |= NETIF_F_SCTP_CSUM |
+				       NETIF_F_NTUPLE;
 		break;
 	default:
 		break;
@@ -7538,6 +7625,8 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
 		netdev->vlan_features |= NETIF_F_HIGHDMA;
 	}
 
+	if (adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE)
+		netdev->hw_features |= NETIF_F_LRO;
 	if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED)
 		netdev->features |= NETIF_F_LRO;
 
@@ -7574,8 +7663,10 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
 	if (err)
 		goto err_sw_init;
 
-	if (!(adapter->flags & IXGBE_FLAG_RSS_ENABLED))
+	if (!(adapter->flags & IXGBE_FLAG_RSS_ENABLED)) {
+		netdev->hw_features &= ~NETIF_F_RXHASH;
 		netdev->features &= ~NETIF_F_RXHASH;
+	}
 
 	switch (pdev->device) {
 	case IXGBE_DEV_ID_82599_SFP:
-- 
1.7.6

Re: [net-next 5/5] ixgbe: convert to ndo_fix_features

[Michal Miroslaw]

On Sat, Jul 09, 2011 at 04:50:40AM -0700, Jeff Kirsher wrote:
> From: Don Skidmore <donald.c.skidmore@intel.com>
> 
> Private rx_csum flags are now duplicate of netdev->features &
> NETIF_F_RXCSUM.  We remove those duplicates and now use the net_device_ops
> ndo_set_features.  This was based on the original patch submitted by
> Michal Miroslaw <mirq-linux@rere.qmqm.pl>.  I also removed the special
> case not requiring a reset for X540 hardware.  It is needed just as it is
> in 82599 hardware.
[...]
> --- a/drivers/net/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ixgbe/ixgbe_main.c
> @@ -7188,6 +7188,88 @@ int ixgbe_setup_tc(struct net_device *dev, u8 tc)
[...]
> +static int ixgbe_set_features(struct net_device *netdev, u32 data)
> +{
> +	struct ixgbe_adapter *adapter = netdev_priv(netdev);
> +	bool need_reset = false;
> +
> +#ifdef CONFIG_DCB
> +	if ((adapter->flags & IXGBE_FLAG_DCB_ENABLED) &&
> +	    !(data &  NETIF_F_HW_VLAN_RX))
> +		return -EINVAL;
> +#endif
> +	/* return error if RXHASH is being enabled when RSS is not supported */
> +	if (!(adapter->flags & IXGBE_FLAG_RSS_ENABLED) &&
> +	     (data &  NETIF_F_RXHASH))
> +		return -EINVAL;
> +
> +	/* If Rx checksum is disabled, then RSC/LRO should also be disabled */
> +	if (!(data & NETIF_F_RXCSUM)) {
> +		data &= ~NETIF_F_LRO;
> +		adapter->flags &= ~IXGBE_FLAG_RX_CSUM_ENABLED;
> +	} else {
> +		adapter->flags |= IXGBE_FLAG_RX_CSUM_ENABLED;
> +	}

The checks above should be done in ndo_fix_features callback. The check for
RSS_ENABLED for RXHASH is redundant, as the feature is removed in probe()
in this case (see [MARK] below).

> +
> +	/* if state changes we need to update adapter->flags and reset */
> +	if ((adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE) &&
> +	    (!!(data & NETIF_F_LRO) !=
> +	     !!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED))) {
> +		if ((data &  NETIF_F_LRO) &&
> +		    adapter->rx_itr_setting != 1 &&
> +		    adapter->rx_itr_setting > IXGBE_MAX_RSC_INT_RATE) {
> +			e_info(probe, "rx-usecs set too low, "
> +			       "not enabling RSC\n");
> +		} else {
> +			adapter->flags2 ^= IXGBE_FLAG2_RSC_ENABLED;
> +			switch (adapter->hw.mac.type) {
> +			case ixgbe_mac_X540:
> +			case ixgbe_mac_82599EB:
> +				need_reset = true;
> +				break;
> +			default:
> +				break;
> +			}
> +		}
> +	}
> +
> +	/*
> +	 * Check if Flow Director n-tuple support was enabled or disabled.  If
> +	 * the state changed, we need to reset.
> +	 */
> +	if (!(adapter->flags & IXGBE_FLAG_FDIR_PERFECT_CAPABLE)) {
> +		/* turn off ATR, enable perfect filters and reset */
> +		if (data & NETIF_F_NTUPLE) {
> +			adapter->flags &= ~IXGBE_FLAG_FDIR_HASH_CAPABLE;
> +			adapter->flags |= IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
> +			need_reset = true;
> +		}
> +	} else if (!(data & NETIF_F_NTUPLE)) {
> +		/* turn off Flow Director, set ATR and reset */
> +		adapter->flags &= ~IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
> +		if ((adapter->flags &  IXGBE_FLAG_RSS_ENABLED) &&
> +		    !(adapter->flags &  IXGBE_FLAG_DCB_ENABLED))
> +			adapter->flags |= IXGBE_FLAG_FDIR_HASH_CAPABLE;
> +		need_reset = true;
> +	}

Part of the checks above should be in ndo_fix_features, too. ndo_set_features
should just enable what it has been passed. What ndo_fix_features callback
returns is further limited by generic checks, and then (if the resulting set
is different than current dev->features) ndo_set_features is called.

> +
> +	if (need_reset)
> +		ixgbe_do_reset(netdev);
> +
> +	return 0;
> +
> +}
> +
>  static const struct net_device_ops ixgbe_netdev_ops = {
>  	.ndo_open		= ixgbe_open,
>  	.ndo_stop		= ixgbe_close,
> @@ -7219,6 +7301,7 @@ static const struct net_device_ops ixgbe_netdev_ops = {
>  	.ndo_fcoe_disable = ixgbe_fcoe_disable,
>  	.ndo_fcoe_get_wwn = ixgbe_fcoe_get_wwn,
>  #endif /* IXGBE_FCOE */
> +	.ndo_set_features = ixgbe_set_features,
>  };
>  
>  static void __devinit ixgbe_probe_vf(struct ixgbe_adapter *adapter,
> @@ -7486,20 +7569,24 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
>  
>  	netdev->features = NETIF_F_SG |
>  			   NETIF_F_IP_CSUM |
> +			   NETIF_F_IPV6_CSUM |
>  			   NETIF_F_HW_VLAN_TX |
>  			   NETIF_F_HW_VLAN_RX |
> -			   NETIF_F_HW_VLAN_FILTER;
> +			   NETIF_F_HW_VLAN_FILTER |
> +			   NETIF_F_TSO |
> +			   NETIF_F_TSO6 |
> +			   NETIF_F_GRO |
> +			   NETIF_F_RXHASH |
> +			   NETIF_F_RXCSUM;
>  
> -	netdev->features |= NETIF_F_IPV6_CSUM;
> -	netdev->features |= NETIF_F_TSO;
> -	netdev->features |= NETIF_F_TSO6;
> -	netdev->features |= NETIF_F_GRO;
> -	netdev->features |= NETIF_F_RXHASH;
> +	netdev->hw_features = netdev->features;
>  
>  	switch (adapter->hw.mac.type) {
>  	case ixgbe_mac_82599EB:
>  	case ixgbe_mac_X540:
>  		netdev->features |= NETIF_F_SCTP_CSUM;
> +		netdev->hw_features |= NETIF_F_SCTP_CSUM |
> +				       NETIF_F_NTUPLE;
>  		break;
>  	default:
>  		break;
> @@ -7538,6 +7625,8 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
>  		netdev->vlan_features |= NETIF_F_HIGHDMA;
>  	}
>  
> +	if (adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE)
> +		netdev->hw_features |= NETIF_F_LRO;
>  	if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED)
>  		netdev->features |= NETIF_F_LRO;
>  
> @@ -7574,8 +7663,10 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
>  	if (err)
>  		goto err_sw_init;
>  
> -	if (!(adapter->flags & IXGBE_FLAG_RSS_ENABLED))
> +	if (!(adapter->flags & IXGBE_FLAG_RSS_ENABLED)) {
> +		netdev->hw_features &= ~NETIF_F_RXHASH;
>  		netdev->features &= ~NETIF_F_RXHASH;
> +	}
>  

[MARK here]

Best Regards,
Michał Mirosław

RE: [net-next 5/5] ixgbe: convert to ndo_fix_features

[Skidmore, Donald C]

>-----Original Message-----
>From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
>Sent: Saturday, July 09, 2011 5:11 AM
>To: Kirsher, Jeffrey T
>Cc: davem@davemloft.net; Skidmore, Donald C; netdev@vger.kernel.org;
>gospo@redhat.com
>Subject: Re: [net-next 5/5] ixgbe: convert to ndo_fix_features
>
>On Sat, Jul 09, 2011 at 04:50:40AM -0700, Jeff Kirsher wrote:
>> From: Don Skidmore <donald.c.skidmore@intel.com>
>>
>> Private rx_csum flags are now duplicate of netdev->features &
>> NETIF_F_RXCSUM.  We remove those duplicates and now use the
>net_device_ops
>> ndo_set_features.  This was based on the original patch submitted by
>> Michal Miroslaw <mirq-linux@rere.qmqm.pl>.  I also removed the special
>> case not requiring a reset for X540 hardware.  It is needed just as it
>is
>> in 82599 hardware.
>[...]
>> --- a/drivers/net/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ixgbe/ixgbe_main.c
>> @@ -7188,6 +7188,88 @@ int ixgbe_setup_tc(struct net_device *dev, u8
>tc)
>[...]
>> +static int ixgbe_set_features(struct net_device *netdev, u32 data)
>> +{
>> +	struct ixgbe_adapter *adapter = netdev_priv(netdev);
>> +	bool need_reset = false;
>> +
>> +#ifdef CONFIG_DCB
>> +	if ((adapter->flags & IXGBE_FLAG_DCB_ENABLED) &&
>> +	    !(data &  NETIF_F_HW_VLAN_RX))
>> +		return -EINVAL;
>> +#endif
>> +	/* return error if RXHASH is being enabled when RSS is not
>supported */
>> +	if (!(adapter->flags & IXGBE_FLAG_RSS_ENABLED) &&
>> +	     (data &  NETIF_F_RXHASH))
>> +		return -EINVAL;
>> +
>> +	/* If Rx checksum is disabled, then RSC/LRO should also be
>disabled */
>> +	if (!(data & NETIF_F_RXCSUM)) {
>> +		data &= ~NETIF_F_LRO;
>> +		adapter->flags &= ~IXGBE_FLAG_RX_CSUM_ENABLED;
>> +	} else {
>> +		adapter->flags |= IXGBE_FLAG_RX_CSUM_ENABLED;
>> +	}
>
>The checks above should be done in ndo_fix_features callback. The check
>for
>RSS_ENABLED for RXHASH is redundant, as the feature is removed in
>probe()
>in this case (see [MARK] below).

Thanks for the comments Michal, it clears up a lot in my mind. :)

So in ndo_fix_features we would just turn off feature flags rather than return an error?  For example:

	if (!(data & NETIF_F_RXCSUM)) 
		data &= ~NETIF_F_LRO;

As for RSS_ENABLED/RXHASH check it was to cover the cases where RSS_ENABLED might have changed since probe.  This could happen with a resume were we don't get enough MSI-X vectors.  There are also paths in FCoE and DCB that get into code that could clear IXGBE_FLAG_RSS_ENABLED.

>
>> +
>> +	/* if state changes we need to update adapter->flags and reset */
>> +	if ((adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE) &&
>> +	    (!!(data & NETIF_F_LRO) !=
>> +	     !!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED))) {
>> +		if ((data &  NETIF_F_LRO) &&
>> +		    adapter->rx_itr_setting != 1 &&
>> +		    adapter->rx_itr_setting > IXGBE_MAX_RSC_INT_RATE) {
>> +			e_info(probe, "rx-usecs set too low, "
>> +			       "not enabling RSC\n");
>> +		} else {
>> +			adapter->flags2 ^= IXGBE_FLAG2_RSC_ENABLED;
>> +			switch (adapter->hw.mac.type) {
>> +			case ixgbe_mac_X540:
>> +			case ixgbe_mac_82599EB:
>> +				need_reset = true;
>> +				break;
>> +			default:
>> +				break;
>> +			}
>> +		}
>> +	}
>> +
>> +	/*
>> +	 * Check if Flow Director n-tuple support was enabled or disabled.
>If
>> +	 * the state changed, we need to reset.
>> +	 */
>> +	if (!(adapter->flags & IXGBE_FLAG_FDIR_PERFECT_CAPABLE)) {
>> +		/* turn off ATR, enable perfect filters and reset */
>> +		if (data & NETIF_F_NTUPLE) {
>> +			adapter->flags &= ~IXGBE_FLAG_FDIR_HASH_CAPABLE;
>> +			adapter->flags |= IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
>> +			need_reset = true;
>> +		}
>> +	} else if (!(data & NETIF_F_NTUPLE)) {
>> +		/* turn off Flow Director, set ATR and reset */
>> +		adapter->flags &= ~IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
>> +		if ((adapter->flags &  IXGBE_FLAG_RSS_ENABLED) &&
>> +		    !(adapter->flags &  IXGBE_FLAG_DCB_ENABLED))
>> +			adapter->flags |= IXGBE_FLAG_FDIR_HASH_CAPABLE;
>> +		need_reset = true;
>> +	}
>
>Part of the checks above should be in ndo_fix_features, too.
>ndo_set_features
>should just enable what it has been passed. What ndo_fix_features
>callback
>returns is further limited by generic checks, and then (if the resulting
>set
>is different than current dev->features) ndo_set_features is called.

I'm a little confused here.  From your comments I get the idea that ndo_fix_features() just modifies and error checks our feature set.  The result of this would then be just a change to the feature set (data variable in my case above).  Is that a correct assumption?  If so I'm confused as none of the two checks above change the feature set.  They do change driver flags to indicate the new state and mark whether we need a reset.  I don't believe we would want to do the reset until ndo_set_feature is called and if we broke up the setting of the driver flags into ndo_fix_features we would lose some state (i.e. if the IXGBE_FLAG2_RSC_ENABLED changed) that we need to decide if a reset is needed in ndo_set_features.  

Am I just missing something here?

Thanks
-Don

>
>> +
>> +	if (need_reset)
>> +		ixgbe_do_reset(netdev);
>> +
>> +	return 0;
>> +
>> +}
>> +
>>  static const struct net_device_ops ixgbe_netdev_ops = {
>>  	.ndo_open		= ixgbe_open,
>>  	.ndo_stop		= ixgbe_close,
>> @@ -7219,6 +7301,7 @@ static const struct net_device_ops
>ixgbe_netdev_ops = {
>>  	.ndo_fcoe_disable = ixgbe_fcoe_disable,
>>  	.ndo_fcoe_get_wwn = ixgbe_fcoe_get_wwn,
>>  #endif /* IXGBE_FCOE */
>> +	.ndo_set_features = ixgbe_set_features,
>>  };
>>
>>  static void __devinit ixgbe_probe_vf(struct ixgbe_adapter *adapter,
>> @@ -7486,20 +7569,24 @@ static int __devinit ixgbe_probe(struct
>pci_dev *pdev,
>>
>>  	netdev->features = NETIF_F_SG |
>>  			   NETIF_F_IP_CSUM |
>> +			   NETIF_F_IPV6_CSUM |
>>  			   NETIF_F_HW_VLAN_TX |
>>  			   NETIF_F_HW_VLAN_RX |
>> -			   NETIF_F_HW_VLAN_FILTER;
>> +			   NETIF_F_HW_VLAN_FILTER |
>> +			   NETIF_F_TSO |
>> +			   NETIF_F_TSO6 |
>> +			   NETIF_F_GRO |
>> +			   NETIF_F_RXHASH |
>> +			   NETIF_F_RXCSUM;
>>
>> -	netdev->features |= NETIF_F_IPV6_CSUM;
>> -	netdev->features |= NETIF_F_TSO;
>> -	netdev->features |= NETIF_F_TSO6;
>> -	netdev->features |= NETIF_F_GRO;
>> -	netdev->features |= NETIF_F_RXHASH;
>> +	netdev->hw_features = netdev->features;
>>
>>  	switch (adapter->hw.mac.type) {
>>  	case ixgbe_mac_82599EB:
>>  	case ixgbe_mac_X540:
>>  		netdev->features |= NETIF_F_SCTP_CSUM;
>> +		netdev->hw_features |= NETIF_F_SCTP_CSUM |
>> +				       NETIF_F_NTUPLE;
>>  		break;
>>  	default:
>>  		break;
>> @@ -7538,6 +7625,8 @@ static int __devinit ixgbe_probe(struct pci_dev
>*pdev,
>>  		netdev->vlan_features |= NETIF_F_HIGHDMA;
>>  	}
>>
>> +	if (adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE)
>> +		netdev->hw_features |= NETIF_F_LRO;
>>  	if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED)
>>  		netdev->features |= NETIF_F_LRO;
>>
>> @@ -7574,8 +7663,10 @@ static int __devinit ixgbe_probe(struct pci_dev
>*pdev,
>>  	if (err)
>>  		goto err_sw_init;
>>
>> -	if (!(adapter->flags & IXGBE_FLAG_RSS_ENABLED))
>> +	if (!(adapter->flags & IXGBE_FLAG_RSS_ENABLED)) {
>> +		netdev->hw_features &= ~NETIF_F_RXHASH;
>>  		netdev->features &= ~NETIF_F_RXHASH;
>> +	}
>>
>
>[MARK here]
>
>Best Regards,
>Michał Mirosław

Re: [net-next 5/5] ixgbe: convert to ndo_fix_features

[Michal Miroslaw]

On Mon, Jul 11, 2011 at 04:53:57PM -0700, Skidmore, Donald C wrote:
> >-----Original Message-----
> >From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
> >Sent: Saturday, July 09, 2011 5:11 AM
> >To: Kirsher, Jeffrey T
> >Cc: davem@davemloft.net; Skidmore, Donald C; netdev@vger.kernel.org;
> >gospo@redhat.com
> >Subject: Re: [net-next 5/5] ixgbe: convert to ndo_fix_features
> >
> >On Sat, Jul 09, 2011 at 04:50:40AM -0700, Jeff Kirsher wrote:
> >> From: Don Skidmore <donald.c.skidmore@intel.com>
> >>
> >> Private rx_csum flags are now duplicate of netdev->features &
> >> NETIF_F_RXCSUM.  We remove those duplicates and now use the
> >net_device_ops
> >> ndo_set_features.  This was based on the original patch submitted by
> >> Michal Miroslaw <mirq-linux@rere.qmqm.pl>.  I also removed the special
> >> case not requiring a reset for X540 hardware.  It is needed just as it
> >is
> >> in 82599 hardware.
> >[...]
> >> --- a/drivers/net/ixgbe/ixgbe_main.c
> >> +++ b/drivers/net/ixgbe/ixgbe_main.c
> >> @@ -7188,6 +7188,88 @@ int ixgbe_setup_tc(struct net_device *dev, u8
> >tc)
> >[...]
> >> +static int ixgbe_set_features(struct net_device *netdev, u32 data)
> >> +{
> >> +	struct ixgbe_adapter *adapter = netdev_priv(netdev);
> >> +	bool need_reset = false;
> >> +
> >> +#ifdef CONFIG_DCB
> >> +	if ((adapter->flags & IXGBE_FLAG_DCB_ENABLED) &&
> >> +	    !(data &  NETIF_F_HW_VLAN_RX))
> >> +		return -EINVAL;
> >> +#endif
> >> +	/* return error if RXHASH is being enabled when RSS is not
> >supported */
> >> +	if (!(adapter->flags & IXGBE_FLAG_RSS_ENABLED) &&
> >> +	     (data &  NETIF_F_RXHASH))
> >> +		return -EINVAL;
> >> +
> >> +	/* If Rx checksum is disabled, then RSC/LRO should also be
> >disabled */
> >> +	if (!(data & NETIF_F_RXCSUM)) {
> >> +		data &= ~NETIF_F_LRO;
> >> +		adapter->flags &= ~IXGBE_FLAG_RX_CSUM_ENABLED;
> >> +	} else {
> >> +		adapter->flags |= IXGBE_FLAG_RX_CSUM_ENABLED;
> >> +	}
> >
> >The checks above should be done in ndo_fix_features callback. The check
> >for
> >RSS_ENABLED for RXHASH is redundant, as the feature is removed in
> >probe()
> >in this case (see [MARK] below).
> Thanks for the comments Michal, it clears up a lot in my mind. :)
> 
> So in ndo_fix_features we would just turn off feature flags rather than return an error?  For example:
> 
> 	if (!(data & NETIF_F_RXCSUM)) 
> 		data &= ~NETIF_F_LRO;

Exactly.

> As for RSS_ENABLED/RXHASH check it was to cover the cases where RSS_ENABLED might have changed since probe.  This could happen with a resume were we don't get enough MSI-X vectors.  There are also paths in FCoE and DCB that get into code that could clear IXGBE_FLAG_RSS_ENABLED.

That makes sense now. The checks should be in ndo_fix_features and clear
features whenever they are unavailable.

> >> +
> >> +	/* if state changes we need to update adapter->flags and reset */
> >> +	if ((adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE) &&
> >> +	    (!!(data & NETIF_F_LRO) !=
> >> +	     !!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED))) {

ndo_fix_features() should prevent the change if
!(adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE) is true.

> >> +		if ((data &  NETIF_F_LRO) &&
> >> +		    adapter->rx_itr_setting != 1 &&
> >> +		    adapter->rx_itr_setting > IXGBE_MAX_RSC_INT_RATE) {
> >> +			e_info(probe, "rx-usecs set too low, "
> >> +			       "not enabling RSC\n");

And should clear NETIF_F_LRO when above condition is met.

> >> +		} else {
> >> +			adapter->flags2 ^= IXGBE_FLAG2_RSC_ENABLED;
> >> +			switch (adapter->hw.mac.type) {
> >> +			case ixgbe_mac_X540:
> >> +			case ixgbe_mac_82599EB:
> >> +				need_reset = true;
> >> +				break;
> >> +			default:
> >> +				break;
> >> +			}
> >> +		}
> >> +	}
> >> +
> >> +	/*
> >> +	 * Check if Flow Director n-tuple support was enabled or disabled.
> >If
> >> +	 * the state changed, we need to reset.
> >> +	 */
> >> +	if (!(adapter->flags & IXGBE_FLAG_FDIR_PERFECT_CAPABLE)) {
> >> +		/* turn off ATR, enable perfect filters and reset */
> >> +		if (data & NETIF_F_NTUPLE) {
> >> +			adapter->flags &= ~IXGBE_FLAG_FDIR_HASH_CAPABLE;
> >> +			adapter->flags |= IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
> >> +			need_reset = true;
> >> +		}
> >> +	} else if (!(data & NETIF_F_NTUPLE)) {
> >> +		/* turn off Flow Director, set ATR and reset */
> >> +		adapter->flags &= ~IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
> >> +		if ((adapter->flags &  IXGBE_FLAG_RSS_ENABLED) &&
> >> +		    !(adapter->flags &  IXGBE_FLAG_DCB_ENABLED))
> >> +			adapter->flags |= IXGBE_FLAG_FDIR_HASH_CAPABLE;
> >> +		need_reset = true;
> >> +	}
> >
> >Part of the checks above should be in ndo_fix_features, too.
> >ndo_set_features
> >should just enable what it has been passed. What ndo_fix_features
> >callback
> >returns is further limited by generic checks, and then (if the resulting
> >set
> >is different than current dev->features) ndo_set_features is called.
> 
> I'm a little confused here.  From your comments I get the idea that ndo_fix_features() just modifies and error checks our feature set.  The result of this would then be just a change to the feature set (data variable in my case above).  Is that a correct assumption?  If so I'm confused as none of the two checks above change the feature set.  They do change driver flags to indicate the new state and mark whether we need a reset.  I don't believe we would want to do the reset until ndo_set_feature is called and if we broke up the setting of the driver flags into ndo_fix_features we would lose some state (i.e. if the IXGBE_FLAG2_RSC_ENABLED changed) that we need to decide if a reset is needed in ndo_set_features.  
> 
> Am I just missing something here?

I see that this changes only driver internal state, so your right in putting
this in ndo_set_features callback.

Can you draw a map of the relevant bits and what state should result from
them (a truth table if you will)? I have a hard time understanding the idea.
This changes _CAPABLE bits depending on _ENABLED which adds to confusion.

I would expect that:
 _CAPABLE are set whenever a config is possible, disregarding dependencies
 _ENABLED are set whenever a config is requested and possible
But it looks like what I described is reversed or just wrong.

Best Regards,
Michał Mirosław

RE: [net-next 5/5] ixgbe: convert to ndo_fix_features

[Skidmore, Donald C]

>-----Original Message-----
>From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
>Sent: Tuesday, July 12, 2011 3:10 AM
>To: Skidmore, Donald C
>Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
>gospo@redhat.com
>Subject: Re: [net-next 5/5] ixgbe: convert to ndo_fix_features
>
>On Mon, Jul 11, 2011 at 04:53:57PM -0700, Skidmore, Donald C wrote:
>> >-----Original Message-----
>> >From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
>> >Sent: Saturday, July 09, 2011 5:11 AM
>> >To: Kirsher, Jeffrey T
>> >Cc: davem@davemloft.net; Skidmore, Donald C; netdev@vger.kernel.org;
>> >gospo@redhat.com
>> >Subject: Re: [net-next 5/5] ixgbe: convert to ndo_fix_features
>> >
>> >On Sat, Jul 09, 2011 at 04:50:40AM -0700, Jeff Kirsher wrote:
>> >> From: Don Skidmore <donald.c.skidmore@intel.com>
>> >>
>> >> Private rx_csum flags are now duplicate of netdev->features &
>> >> NETIF_F_RXCSUM.  We remove those duplicates and now use the
>> >net_device_ops
>> >> ndo_set_features.  This was based on the original patch submitted
>by
>> >> Michal Miroslaw <mirq-linux@rere.qmqm.pl>.  I also removed the
>special
>> >> case not requiring a reset for X540 hardware.  It is needed just as
>it
>> >is
>> >> in 82599 hardware.
>> >[...]
>> >> --- a/drivers/net/ixgbe/ixgbe_main.c
>> >> +++ b/drivers/net/ixgbe/ixgbe_main.c
>> >> @@ -7188,6 +7188,88 @@ int ixgbe_setup_tc(struct net_device *dev,
>u8
>> >tc)
>> >[...]
>> >> +static int ixgbe_set_features(struct net_device *netdev, u32 data)
>> >> +{
>> >> +	struct ixgbe_adapter *adapter = netdev_priv(netdev);
>> >> +	bool need_reset = false;
>> >> +
>> >> +#ifdef CONFIG_DCB
>> >> +	if ((adapter->flags & IXGBE_FLAG_DCB_ENABLED) &&
>> >> +	    !(data &  NETIF_F_HW_VLAN_RX))
>> >> +		return -EINVAL;
>> >> +#endif
>> >> +	/* return error if RXHASH is being enabled when RSS is not
>> >supported */
>> >> +	if (!(adapter->flags & IXGBE_FLAG_RSS_ENABLED) &&
>> >> +	     (data &  NETIF_F_RXHASH))
>> >> +		return -EINVAL;
>> >> +
>> >> +	/* If Rx checksum is disabled, then RSC/LRO should also be
>> >disabled */
>> >> +	if (!(data & NETIF_F_RXCSUM)) {
>> >> +		data &= ~NETIF_F_LRO;
>> >> +		adapter->flags &= ~IXGBE_FLAG_RX_CSUM_ENABLED;
>> >> +	} else {
>> >> +		adapter->flags |= IXGBE_FLAG_RX_CSUM_ENABLED;
>> >> +	}
>> >
>> >The checks above should be done in ndo_fix_features callback. The
>check
>> >for
>> >RSS_ENABLED for RXHASH is redundant, as the feature is removed in
>> >probe()
>> >in this case (see [MARK] below).
>> Thanks for the comments Michal, it clears up a lot in my mind. :)
>>
>> So in ndo_fix_features we would just turn off feature flags rather
>than return an error?  For example:
>>
>> 	if (!(data & NETIF_F_RXCSUM))
>> 		data &= ~NETIF_F_LRO;
>
>Exactly.
>
>> As for RSS_ENABLED/RXHASH check it was to cover the cases where
>RSS_ENABLED might have changed since probe.  This could happen with a
>resume were we don't get enough MSI-X vectors.  There are also paths in
>FCoE and DCB that get into code that could clear IXGBE_FLAG_RSS_ENABLED.
>
>That makes sense now. The checks should be in ndo_fix_features and clear
>features whenever they are unavailable.
>

I'm seeing how this would work; most of my earlier checks should fall pretty naturally in to ndo_fix_feature. :)

>> >> +
>> >> +	/* if state changes we need to update adapter->flags and
>reset */
>> >> +	if ((adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE) &&
>> >> +	    (!!(data & NETIF_F_LRO) !=
>> >> +	     !!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED))) {
>
>ndo_fix_features() should prevent the change if
>!(adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE) is true.
>

The logic in this conditional is a little complex. I believe it would end up looking something like the following:

in fix_features:

	/* Prevent change if not RSC capable or invalid ITR setting */
	if (data & NETIF_F_LRO)
		if (!(adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE) {
			data &= ~NETIF_F_LRO;
		else if (!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) &&
	    		   (adapter->rx_itr_setting != 1 &&
			    adapter->rx_itr_setting > IXGBE_MAX_RSC_INT_RATE)) {
				data &= ~NETIF_F_LRO;
		}
	} 

in set_feature:

	/* Make sure RSC matches LRO, reset if change */
	if (!!(data & NETIF_F_LRO) != 
	    !!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED)) {
		adapter->flags2 ^= IXGBE_FLAG2_RSC_ENABLED;
		switch (adapter->hw.mac.type) {
		case ixgbe_mac_X540:
		case ixgbe_mac_82599EB:
			need_reset = true;
			break;
		default:
			break;	
		}
	}

>> >> +		if ((data &  NETIF_F_LRO) &&
>> >> +		    adapter->rx_itr_setting != 1 &&
>> >> +		    adapter->rx_itr_setting > IXGBE_MAX_RSC_INT_RATE)
>{
>> >> +			e_info(probe, "rx-usecs set too low, "
>> >> +			       "not enabling RSC\n");
>
>And should clear NETIF_F_LRO when above condition is met.
>
>> >> +		} else {
>> >> +			adapter->flags2 ^= IXGBE_FLAG2_RSC_ENABLED;
>> >> +			switch (adapter->hw.mac.type) {
>> >> +			case ixgbe_mac_X540:
>> >> +			case ixgbe_mac_82599EB:
>> >> +				need_reset = true;
>> >> +				break;
>> >> +			default:
>> >> +				break;
>> >> +			}
>> >> +		}
>> >> +	}
>> >> +
>> >> +	/*
>> >> +	 * Check if Flow Director n-tuple support was enabled or
>disabled.
>> >If
>> >> +	 * the state changed, we need to reset.
>> >> +	 */
>> >> +	if (!(adapter->flags & IXGBE_FLAG_FDIR_PERFECT_CAPABLE)) {
>> >> +		/* turn off ATR, enable perfect filters and reset */
>> >> +		if (data & NETIF_F_NTUPLE) {
>> >> +			adapter->flags &= ~IXGBE_FLAG_FDIR_HASH_CAPABLE;
>> >> +			adapter->flags |=
>IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
>> >> +			need_reset = true;
>> >> +		}
>> >> +	} else if (!(data & NETIF_F_NTUPLE)) {
>> >> +		/* turn off Flow Director, set ATR and reset */
>> >> +		adapter->flags &= ~IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
>> >> +		if ((adapter->flags &  IXGBE_FLAG_RSS_ENABLED) &&
>> >> +		    !(adapter->flags &  IXGBE_FLAG_DCB_ENABLED))
>> >> +			adapter->flags |= IXGBE_FLAG_FDIR_HASH_CAPABLE;
>> >> +		need_reset = true;
>> >> +	}
>> >
>> >Part of the checks above should be in ndo_fix_features, too.
>> >ndo_set_features
>> >should just enable what it has been passed. What ndo_fix_features
>> >callback
>> >returns is further limited by generic checks, and then (if the
>resulting
>> >set
>> >is different than current dev->features) ndo_set_features is called.
>>
>> I'm a little confused here.  From your comments I get the idea that
>ndo_fix_features() just modifies and error checks our feature set.  The
>result of this would then be just a change to the feature set (data
>variable in my case above).  Is that a correct assumption?  If so I'm
>confused as none of the two checks above change the feature set.  They
>do change driver flags to indicate the new state and mark whether we
>need a reset.  I don't believe we would want to do the reset until
>ndo_set_feature is called and if we broke up the setting of the driver
>flags into ndo_fix_features we would lose some state (i.e. if the
>IXGBE_FLAG2_RSC_ENABLED changed) that we need to decide if a reset is
>needed in ndo_set_features.
>>
>> Am I just missing something here?
>
>I see that this changes only driver internal state, so your right in
>putting
>this in ndo_set_features callback.
>
>Can you draw a map of the relevant bits and what state should result
>from
>them (a truth table if you will)? I have a hard time understanding the
>idea.
>This changes _CAPABLE bits depending on _ENABLED which adds to
>confusion.
>
>I would expect that:
> _CAPABLE are set whenever a config is possible, disregarding
>dependencies
> _ENABLED are set whenever a config is requested and possible
>But it looks like what I described is reversed or just wrong.
>
>Best Regards,
>Michał Mirosław

Hopefully the code example above answers your questions.  I can see why you were having a hard time understanding it; I was making the FALSE assumption and not clearing the feature flags for the more complex conditionals. Thanks again for the clarifications, the netdev-features.txt helped a lot too. 

Assuming I'm on the right track now I'll code up a new patch and send it out after our validation runs.

Thanks,
-Don Skidmore <donald.c.skidmore@intel.com>

Re: [net-next 5/5] ixgbe: convert to ndo_fix_features

[Michal Miroslaw]

Good to see the idea converging. :) More hints below.

On Tue, Jul 12, 2011 at 02:28:15PM -0700, Skidmore, Donald C wrote:
[...]
> The logic in this conditional is a little complex. I believe it would
> end up looking something like the following:
> 
> in fix_features:
> 
> 	/* Prevent change if not RSC capable or invalid ITR setting */
> 	if (data & NETIF_F_LRO)

That's not a fast path - you can drop (data & NETIF_F_LRO) check.

> 		if (!(adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE) {
> 			data &= ~NETIF_F_LRO;
> 		else if (!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) &&
> 	    		   (adapter->rx_itr_setting != 1 &&
> 			    adapter->rx_itr_setting > IXGBE_MAX_RSC_INT_RATE)) {
> 				data &= ~NETIF_F_LRO;
> 		}
> 	} 

Same for !(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED). After this change,
you might add a call to netdev_update_features() whenever the second part
of the condition is changed by user and you get correctness all the time
'for free'.

> in set_feature:
> 
> 	/* Make sure RSC matches LRO, reset if change */
> 	if (!!(data & NETIF_F_LRO) != 
> 	    !!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED)) {
> 		adapter->flags2 ^= IXGBE_FLAG2_RSC_ENABLED;
> 		switch (adapter->hw.mac.type) {
> 		case ixgbe_mac_X540:
> 		case ixgbe_mac_82599EB:
> 			need_reset = true;
> 			break;
> 		default:
> 			break;	
> 		}
> 	}

Correct. Why would you need IXGBE_FLAG2_RSC_ENABLED now, when it just
mirrors NETIF_F_LRO?

[...]
> Thanks again for the clarifications, the netdev-features.txt helped a lot too. 

Nice to hear (read) that!

Best Regards,
Michał Mirosław

RE: [net-next 5/5] ixgbe: convert to ndo_fix_features

[Skidmore, Donald C]

>-----Original Message-----
>From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
>Sent: Tuesday, July 12, 2011 2:45 PM
>To: Skidmore, Donald C
>Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
>gospo@redhat.com
>Subject: Re: [net-next 5/5] ixgbe: convert to ndo_fix_features
>
>Good to see the idea converging. :) More hints below.
>
>On Tue, Jul 12, 2011 at 02:28:15PM -0700, Skidmore, Donald C wrote:
>[...]
>> The logic in this conditional is a little complex. I believe it would
>> end up looking something like the following:
>>
>> in fix_features:
>>
>> 	/* Prevent change if not RSC capable or invalid ITR setting */
>> 	if (data & NETIF_F_LRO)
>
>That's not a fast path - you can drop (data & NETIF_F_LRO) check.

Nice - I was spending too much time looking at the old code didn't notice this.

>
>> 		if (!(adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE) {
>> 			data &= ~NETIF_F_LRO;
>> 		else if (!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) &&
>> 	    		   (adapter->rx_itr_setting != 1 &&
>> 			    adapter->rx_itr_setting > IXGBE_MAX_RSC_INT_RATE))
>{
>> 				data &= ~NETIF_F_LRO;
>> 		}
>> 	}
>
>Same for !(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED). After this
>change,
>you might add a call to netdev_update_features() whenever the second
>part
>of the condition is changed by user and you get correctness all the time
>'for free'.

I like this too and makes much more sense to me.  This was a reflection of the original code but really didn't matter since anytime RSC_ENABLED was set the ITR values would have been correct anyway.
   
>
>> in set_feature:
>>
>> 	/* Make sure RSC matches LRO, reset if change */
>> 	if (!!(data & NETIF_F_LRO) !=
>> 	    !!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED)) {
>> 		adapter->flags2 ^= IXGBE_FLAG2_RSC_ENABLED;
>> 		switch (adapter->hw.mac.type) {
>> 		case ixgbe_mac_X540:
>> 		case ixgbe_mac_82599EB:
>> 			need_reset = true;
>> 			break;
>> 		default:
>> 			break;
>> 		}
>> 	}
>
>Correct. Why would you need IXGBE_FLAG2_RSC_ENABLED now, when it just
>mirrors NETIF_F_LRO?
>

We currently have a lot of checks for the IXGBE_FLAG2_RSC_ENABLED flag all over the code.  I bet your next question is why not just using NETIF_F_LRO. :)  Well this same code base is used in our source forge driver that has to be backward compatible with older kernels.  It might be possible to do the kcompat magic to make it all work out but it won't be trivial and I didn't want to include it in this patch.  I had planned on looking at it in the future to see if it was possible/worth doing. 

>[...]
>> Thanks again for the clarifications, the netdev-features.txt helped a
>lot too.
>
>Nice to hear (read) that!
>
>Best Regards,
>Michał Mirosław

I'll try to get a patch out soon.

Thanks again,
-Don Skidmore <donald.c.skidmore@intel.com>