[net-next 00/13][pull request] Intel Wired LAN Driver Updates

This series contains updates to e1000 and igb.  Patches for e1000
consist of code cleanups and patches for igb adds loopback support
for i210 as well as PTP fixes.

The following are changes since commit 0fa7fa98dbcc2789409ed24e885485e645803d7f:
  packet: Protect packet sk list with mutex (v2)
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/net-next master

Bruce Allan (7):
  e1000e: use correct type for read of 32-bit register
  e1000e: cleanup strict checkpatch check
  e1000e: cleanup - remove inapplicable comment
  e1000e: cleanup checkpatch PREFER_PR_LEVEL warning
  e1000e: cleanup - remove unnecessary variable
  e1000e: update driver version number
  e1000e: cleanup strict checkpatch MEMORY_BARRIER checks

Carolyn Wyborny (1):
  igb: Add loopback test support for i210.

Eric Dumazet (1):
  igb: reduce Rx header size

Matthew Vick (4):
  igb: Tidy up wrapping for CONFIG_IGB_PTP.
  igb: Update PTP function names/variables and locations.
  igb: Correct PTP support query from ethtool.
  igb: Store the MAC address in the name in the PTP struct.

 drivers/net/ethernet/intel/e1000e/82571.c    |   4 +-
 drivers/net/ethernet/intel/e1000e/ethtool.c  |   3 +-
 drivers/net/ethernet/intel/e1000e/netdev.c   |  28 +-
 drivers/net/ethernet/intel/igb/igb.h         |  29 +-
 drivers/net/ethernet/intel/igb/igb_ethtool.c | 136 ++++----
 drivers/net/ethernet/intel/igb/igb_main.c    | 272 +--------------
 drivers/net/ethernet/intel/igb/igb_ptp.c     | 488 ++++++++++++++++++++-------
 7 files changed, 490 insertions(+), 470 deletions(-)

-- 
1.7.11.4

From: Bruce Allan <redacted>

CHECK: multiple assignments should be avoided

Signed-off-by: Bruce Allan <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
 drivers/net/ethernet/intel/e1000e/ethtool.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
index 2e76f06..c11ac27 100644
--- a/drivers/net/ethernet/intel/e1000e/ethtool.c
+++ b/drivers/net/ethernet/intel/e1000e/ethtool.c

@@ -1942,7 +1942,8 @@ static int e1000_set_coalesce(struct net_device *netdev,
 		return -EINVAL;
 
 	if (ec->rx_coalesce_usecs == 4) {
-		adapter->itr = adapter->itr_setting = 4;
+		adapter->itr_setting = 4;
+		adapter->itr = adapter->itr_setting;
 	} else if (ec->rx_coalesce_usecs <= 3) {
 		adapter->itr = 20000;
 		adapter->itr_setting = ec->rx_coalesce_usecs;

-- 
1.7.11.4

From: Bruce Allan <redacted>

The POEMB register is 32 bits, not 16.

Signed-off-by: Bruce Allan <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
 drivers/net/ethernet/intel/e1000e/82571.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/82571.c b/drivers/net/ethernet/intel/e1000e/82571.c
index 080c890..c985864 100644
--- a/drivers/net/ethernet/intel/e1000e/82571.c
+++ b/drivers/net/ethernet/intel/e1000e/82571.c

@@ -653,7 +653,7 @@ static void e1000_put_hw_semaphore_82574(struct e1000_hw *hw)
  **/
 static s32 e1000_set_d0_lplu_state_82574(struct e1000_hw *hw, bool active)
 {
-	u16 data = er32(POEMB);
+	u32 data = er32(POEMB);
 
 	if (active)
 		data |= E1000_PHY_CTRL_D0A_LPLU;

@@ -677,7 +677,7 @@ static s32 e1000_set_d0_lplu_state_82574(struct e1000_hw *hw, bool active)
  **/
 static s32 e1000_set_d3_lplu_state_82574(struct e1000_hw *hw, bool active)
 {
-	u16 data = er32(POEMB);
+	u32 data = er32(POEMB);
 
 	if (!active) {
 		data &= ~E1000_PHY_CTRL_NOND0A_LPLU;

-- 
1.7.11.4

From: Bruce Allan <redacted>

Early Receive has been disabled in the driver so this comment is no longer
applicable.

Signed-off-by: Bruce Allan <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 46c3b1f..e4d8041 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c

@@ -3446,7 +3446,7 @@ void e1000e_reset(struct e1000_adapter *adapter)
 
 			/*
 			 * if short on Rx space, Rx wins and must trump Tx
-			 * adjustment or use Early Receive if available
+			 * adjustment
 			 */
 			if (pba < min_rx_space)
 				pba = min_rx_space;

-- 
1.7.11.4

From: Bruce Allan <redacted>

checkpatch warning: Prefer pr_info(... to printk(KERN_INFO, ...

Signed-off-by: Bruce Allan <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index e4d8041..bc611b4 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c

@@ -4330,9 +4330,8 @@ static void e1000_print_link_info(struct e1000_adapter *adapter)
 	u32 ctrl = er32(CTRL);
 
 	/* Link status message must follow this format for user tools */
-	printk(KERN_INFO "e1000e: %s NIC Link is Up %d Mbps %s Duplex, Flow Control: %s\n",
-		adapter->netdev->name,
-		adapter->link_speed,
+	pr_info("e1000e: %s NIC Link is Up %d Mbps %s Duplex, Flow Control: %s\n",
+		adapter->netdev->name, adapter->link_speed,
 		adapter->link_duplex == FULL_DUPLEX ? "Full" : "Half",
 		(ctrl & E1000_CTRL_TFCE) && (ctrl & E1000_CTRL_RFCE) ? "Rx/Tx" :
 		(ctrl & E1000_CTRL_RFCE) ? "Rx" :

@@ -4558,8 +4557,8 @@ static void e1000_watchdog_task(struct work_struct *work)
 			adapter->link_speed = 0;
 			adapter->link_duplex = 0;
 			/* Link status message must follow this format */
-			printk(KERN_INFO "e1000e: %s NIC Link is Down\n",
-			       adapter->netdev->name);
+			pr_info("e1000e: %s NIC Link is Down\n",
+				adapter->netdev->name);
 			netif_carrier_off(netdev);
 			if (!test_bit(__E1000_DOWN, &adapter->state))
 				mod_timer(&adapter->phy_info_timer,

-- 
1.7.11.4

On Thu, 2012-08-23 at 02:56 -0700, Jeff Kirsher wrote:

From: Bruce Allan <redacted>

checkpatch warning: Prefer pr_info(... to printk(KERN_INFO, ...

[]

[]

I think these conversions are not a good idea.

When you have a specific message format that must be
followed, use printk.

pr_<level> may at some point in the near future use
#define pr_fmt(fmt) KBUiLD_MODNAME ": " fmt
as a global default equivalent.

On Thu, 2012-08-23 at 07:01 -0700, Joe Perches wrote:

On Thu, 2012-08-23 at 02:56 -0700, Jeff Kirsher wrote:

quoted

From: Bruce Allan <redacted>

checkpatch warning: Prefer pr_info(... to printk(KERN_INFO, ...

[]

quoted

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c

[]

quoted

@@ -4330,9 +4330,8 @@ static void e1000_print_link_info(struct e1000_adapter *adapter)
 	u32 ctrl = er32(CTRL);
 
 	/* Link status message must follow this format for user tools */
-	printk(KERN_INFO "e1000e: %s NIC Link is Up %d Mbps %s Duplex, Flow Control: %s\n",
-		adapter->netdev->name,
-		adapter->link_speed,
+	pr_info("e1000e: %s NIC Link is Up %d Mbps %s Duplex, Flow Control: %s\n",
+		adapter->netdev->name, adapter->link_speed,
 		adapter->link_duplex == FULL_DUPLEX ? "Full" : "Half",
 		(ctrl & E1000_CTRL_TFCE) && (ctrl & E1000_CTRL_RFCE) ? "Rx/Tx" :
 		(ctrl & E1000_CTRL_RFCE) ? "Rx" :

I think these conversions are not a good idea.

When you have a specific message format that must be
followed, use printk.

pr_<level> may at some point in the near future use
#define pr_fmt(fmt) KBUiLD_MODNAME ": " fmt
as a global default equivalent.

Hey Jeff.

The comment above this change (and the other) reads

  	/* Link status message must follow this format for user tools */

This file already uses #define pr_fmt(fmt) KBUILD_MODNAME...
With this patch, the output form changes to use 2 prefixes.

Is that really desired?  Probably not.

If the comments are old and don't apply any more, they
should be removed.

On Fri, 2012-08-24 at 11:02 -0700, Joe Perches wrote:

On Thu, 2012-08-23 at 07:01 -0700, Joe Perches wrote:

quoted

On Thu, 2012-08-23 at 02:56 -0700, Jeff Kirsher wrote:

quoted

From: Bruce Allan <redacted>

checkpatch warning: Prefer pr_info(... to printk(KERN_INFO, ...

[]

quoted

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c

[]

quoted

@@ -4330,9 +4330,8 @@ static void e1000_print_link_info(struct e1000_adapter *adapter)
 	u32 ctrl = er32(CTRL);
 
 	/* Link status message must follow this format for user tools */
-	printk(KERN_INFO "e1000e: %s NIC Link is Up %d Mbps %s Duplex, Flow Control: %s\n",
-		adapter->netdev->name,
-		adapter->link_speed,
+	pr_info("e1000e: %s NIC Link is Up %d Mbps %s Duplex, Flow Control: %s\n",
+		adapter->netdev->name, adapter->link_speed,
 		adapter->link_duplex == FULL_DUPLEX ? "Full" : "Half",
 		(ctrl & E1000_CTRL_TFCE) && (ctrl & E1000_CTRL_RFCE) ? "Rx/Tx" :
 		(ctrl & E1000_CTRL_RFCE) ? "Rx" :

I think these conversions are not a good idea.

When you have a specific message format that must be
followed, use printk.

pr_<level> may at some point in the near future use
#define pr_fmt(fmt) KBUiLD_MODNAME ": " fmt
as a global default equivalent.

Hey Jeff.

The comment above this change (and the other) reads

  	/* Link status message must follow this format for user tools */

This file already uses #define pr_fmt(fmt) KBUILD_MODNAME...
With this patch, the output form changes to use 2 prefixes.

Is that really desired?  Probably not.

If the comments are old and don't apply any more, they
should be removed.

Bruce really should answer this since this is his patch and there was a
reason why he made the change.  My guess was the current output was
providing incorrect or mis-leading information.

On Fri, 2012-08-24 at 14:19 -0700, Jeff Kirsher wrote:

On Fri, 2012-08-24 at 11:02 -0700, Joe Perches wrote:

quoted

On Thu, 2012-08-23 at 07:01 -0700, Joe Perches wrote:

quoted

On Thu, 2012-08-23 at 02:56 -0700, Jeff Kirsher wrote:

quoted

From: Bruce Allan <redacted>

checkpatch warning: Prefer pr_info(... to printk(KERN_INFO, ...

[]

quoted

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c

[]

quoted

@@ -4330,9 +4330,8 @@ static void e1000_print_link_info(struct e1000_adapter *adapter)
 	u32 ctrl = er32(CTRL);
 
 	/* Link status message must follow this format for user tools */
-	printk(KERN_INFO "e1000e: %s NIC Link is Up %d Mbps %s Duplex, Flow Control: %s\n",
-		adapter->netdev->name,
-		adapter->link_speed,
+	pr_info("e1000e: %s NIC Link is Up %d Mbps %s Duplex, Flow Control: %s\n",
+		adapter->netdev->name, adapter->link_speed,
 		adapter->link_duplex == FULL_DUPLEX ? "Full" : "Half",
 		(ctrl & E1000_CTRL_TFCE) && (ctrl & E1000_CTRL_RFCE) ? "Rx/Tx" :
 		(ctrl & E1000_CTRL_RFCE) ? "Rx" :

[]

quoted

The comment above this change (and the other) reads

  	/* Link status message must follow this format for user tools */

This file already uses #define pr_fmt(fmt) KBUILD_MODNAME...
With this patch, the output form changes to use 2 prefixes.

Is that really desired?  Probably not.

If the comments are old and don't apply any more, they
should be removed.

Bruce really should answer this since this is his patch and there was a
reason why he made the change.  My guess was the current output was
providing incorrect or mis-leading information.

My guess is he was just shutting up checkpatch and
didn't notice the newly doubled prefix.

happy weekend...

From: Bruce Allan <redacted>

Signed-off-by: Bruce Allan <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index bc611b4..fb42152 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c

@@ -4660,7 +4660,7 @@ static int e1000_tso(struct e1000_ring *tx_ring, struct sk_buff *skb)
 	struct e1000_buffer *buffer_info;
 	unsigned int i;
 	u32 cmd_length = 0;
-	u16 ipcse = 0, tucse, mss;
+	u16 ipcse = 0, mss;
 	u8 ipcss, ipcso, tucss, tucso, hdr_len;
 
 	if (!skb_is_gso(skb))

@@ -4694,7 +4694,6 @@ static int e1000_tso(struct e1000_ring *tx_ring, struct sk_buff *skb)
 	ipcso = (void *)&(ip_hdr(skb)->check) - (void *)skb->data;
 	tucss = skb_transport_offset(skb);
 	tucso = (void *)&(tcp_hdr(skb)->check) - (void *)skb->data;
-	tucse = 0;
 
 	cmd_length |= (E1000_TXD_CMD_DEXT | E1000_TXD_CMD_TSE |
 	               E1000_TXD_CMD_TCP | (skb->len - (hdr_len)));

@@ -4708,7 +4707,7 @@ static int e1000_tso(struct e1000_ring *tx_ring, struct sk_buff *skb)
 	context_desc->lower_setup.ip_fields.ipcse  = cpu_to_le16(ipcse);
 	context_desc->upper_setup.tcp_fields.tucss = tucss;
 	context_desc->upper_setup.tcp_fields.tucso = tucso;
-	context_desc->upper_setup.tcp_fields.tucse = cpu_to_le16(tucse);
+	context_desc->upper_setup.tcp_fields.tucse = 0;
 	context_desc->tcp_seg_setup.fields.mss     = cpu_to_le16(mss);
 	context_desc->tcp_seg_setup.fields.hdr_len = hdr_len;
 	context_desc->cmd_and_length = cpu_to_le32(cmd_length);

-- 
1.7.11.4

From: Bruce Allan <redacted>

Signed-off-by: Bruce Allan <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index fb42152..e7226b0 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c

@@ -56,7 +56,7 @@
 
 #define DRV_EXTRAVERSION "-k"
 
-#define DRV_VERSION "2.0.0" DRV_EXTRAVERSION
+#define DRV_VERSION "2.1.4" DRV_EXTRAVERSION
 char e1000e_driver_name[] = "e1000e";
 const char e1000e_driver_version[] = DRV_VERSION;
 

-- 
1.7.11.4

From: Carolyn Wyborny <redacted>

Early release of i210 devices had the loopback test of the ethtool
self-test disabled. This patch enables the loopback test for i210 devices.

Signed-off-by: Carolyn Wyborny <redacted>
Tested-by: Jeff Pieper <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
 drivers/net/ethernet/intel/igb/igb_ethtool.c | 51 +++++++++-------------------
 1 file changed, 16 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index be02168..c4def55 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c

@@ -1511,33 +1511,22 @@ static int igb_integrated_phy_loopback(struct igb_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	u32 ctrl_reg = 0;
-	u16 phy_reg = 0;
 
 	hw->mac.autoneg = false;
 
-	switch (hw->phy.type) {
-	case e1000_phy_m88:
-		/* Auto-MDI/MDIX Off */
-		igb_write_phy_reg(hw, M88E1000_PHY_SPEC_CTRL, 0x0808);
-		/* reset to update Auto-MDI/MDIX */
-		igb_write_phy_reg(hw, PHY_CONTROL, 0x9140);
-		/* autoneg off */
-		igb_write_phy_reg(hw, PHY_CONTROL, 0x8140);
-		break;
-	case e1000_phy_82580:
-		/* enable MII loopback */
-		igb_write_phy_reg(hw, I82580_PHY_LBK_CTRL, 0x8041);
-		break;
-	case e1000_phy_i210:
-		/* set loopback speed in PHY */
-		igb_read_phy_reg(hw, (GS40G_PAGE_SELECT & GS40G_PAGE_2),
-					&phy_reg);
-		phy_reg |= GS40G_MAC_SPEED_1G;
-		igb_write_phy_reg(hw, (GS40G_PAGE_SELECT & GS40G_PAGE_2),
-					phy_reg);
-		ctrl_reg = rd32(E1000_CTRL_EXT);
-	default:
-		break;
+	if (hw->phy.type == e1000_phy_m88) {
+		if (hw->phy.id != I210_I_PHY_ID) {
+			/* Auto-MDI/MDIX Off */
+			igb_write_phy_reg(hw, M88E1000_PHY_SPEC_CTRL, 0x0808);
+			/* reset to update Auto-MDI/MDIX */
+			igb_write_phy_reg(hw, PHY_CONTROL, 0x9140);
+			/* autoneg off */
+			igb_write_phy_reg(hw, PHY_CONTROL, 0x8140);
+		} else {
+			/* force 1000, set loopback  */
+			igb_write_phy_reg(hw, I347AT4_PAGE_SELECT, 0);
+			igb_write_phy_reg(hw, PHY_CONTROL, 0x4140);
+		}
 	}
 
 	/* add small delay to avoid loopback test failure */

@@ -1555,7 +1544,7 @@ static int igb_integrated_phy_loopback(struct igb_adapter *adapter)
 		     E1000_CTRL_FD |	 /* Force Duplex to FULL */
 		     E1000_CTRL_SLU);	 /* Set link up enable bit */
 
-	if ((hw->phy.type == e1000_phy_m88) || (hw->phy.type == e1000_phy_i210))
+	if (hw->phy.type == e1000_phy_m88)
 		ctrl_reg |= E1000_CTRL_ILOS; /* Invert Loss of Signal */
 
 	wr32(E1000_CTRL, ctrl_reg);

@@ -1563,11 +1552,10 @@ static int igb_integrated_phy_loopback(struct igb_adapter *adapter)
 	/* Disable the receiver on the PHY so when a cable is plugged in, the
 	 * PHY does not begin to autoneg when a cable is reconnected to the NIC.
 	 */
-	if ((hw->phy.type == e1000_phy_m88) || (hw->phy.type == e1000_phy_i210))
+	if (hw->phy.type == e1000_phy_m88)
 		igb_phy_disable_receiver(adapter);
 
-	udelay(500);
-
+	mdelay(500);
 	return 0;
 }
 

@@ -1827,13 +1815,6 @@ static int igb_loopback_test(struct igb_adapter *adapter, u64 *data)
 		*data = 0;
 		goto out;
 	}
-	if ((adapter->hw.mac.type == e1000_i210)
-		|| (adapter->hw.mac.type == e1000_i211)) {
-		dev_err(&adapter->pdev->dev,
-			"Loopback test not supported on this part at this time.\n");
-		*data = 0;
-		goto out;
-	}
 	*data = igb_setup_desc_rings(adapter);
 	if (*data)
 		goto out;

-- 
1.7.11.4

From: Bruce Allan <redacted>

Add comments to memory barriers per strict checkpatch.

Signed-off-by: Bruce Allan <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index e7226b0..d8a65af 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c

@@ -3746,6 +3746,10 @@ static irqreturn_t e1000_intr_msi_test(int irq, void *data)
 	e_dbg("icr is %08X\n", icr);
 	if (icr & E1000_ICR_RXSEQ) {
 		adapter->flags &= ~FLAG_MSI_TEST_FAILED;
+		/*
+		 * Force memory writes to complete before acknowledging the
+		 * interrupt is handled.
+		 */
 		wmb();
 	}
 

@@ -3787,6 +3791,10 @@ static int e1000_test_msi_interrupt(struct e1000_adapter *adapter)
 		goto msi_test_failed;
 	}
 
+	/*
+	 * Force memory writes to complete before enabling and firing an
+	 * interrupt.
+	 */
 	wmb();
 
 	e1000_irq_enable(adapter);

@@ -3798,7 +3806,7 @@ static int e1000_test_msi_interrupt(struct e1000_adapter *adapter)
 
 	e1000_irq_disable(adapter);
 
-	rmb();
+	rmb();			/* read flags after interrupt has been fired */
 
 	if (adapter->flags & FLAG_MSI_TEST_FAILED) {
 		adapter->int_mode = E1000E_INT_MODE_LEGACY;

-- 
1.7.11.4

From: Eric Dumazet <edumazet@google.com>

Reduce skb truesize by 256 bytes.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Alexander Duyck <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
 drivers/net/ethernet/intel/igb/igb.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 9e572dd..0c9f62c 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h

@@ -131,9 +131,9 @@ struct vf_data_storage {
 #define MAXIMUM_ETHERNET_VLAN_SIZE 1522
 
 /* Supported Rx Buffer Sizes */
-#define IGB_RXBUFFER_512   512
+#define IGB_RXBUFFER_256   256
 #define IGB_RXBUFFER_16384 16384
-#define IGB_RX_HDR_LEN     IGB_RXBUFFER_512
+#define IGB_RX_HDR_LEN     IGB_RXBUFFER_256
 
 /* How many Tx Descriptors do we need to call netif_wake_queue ? */
 #define IGB_TX_QUEUE_WAKE	16

-- 
1.7.11.4

From: Matthew Vick <redacted>

For users without CONFIG_IGB_PTP=y, we should not be compiling any PTP
code into the driver. Tidy up the wrapping in igb to support this.

Signed-off-by: Matthew Vick <redacted>
Acked-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Jeff Pieper  <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
 drivers/net/ethernet/intel/igb/igb.h         |  8 ++++++--
 drivers/net/ethernet/intel/igb/igb_ethtool.c |  4 ++--
 drivers/net/ethernet/intel/igb/igb_main.c    | 23 +++++++++++++++++------
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 0c9f62c..a3b5b90 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h

@@ -34,9 +34,11 @@
 #include "e1000_mac.h"
 #include "e1000_82575.h"
 
+#ifdef CONFIG_IGB_PTP
 #include <linux/clocksource.h>
 #include <linux/net_tstamp.h>
 #include <linux/ptp_clock_kernel.h>
+#endif /* CONFIG_IGB_PTP */
 #include <linux/bitops.h>
 #include <linux/if_vlan.h>
 

@@ -376,12 +378,15 @@ struct igb_adapter {
 	int node;
 	u32 *shadow_vfta;
 
+#ifdef CONFIG_IGB_PTP
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info caps;
 	struct delayed_work overflow_work;
 	spinlock_t tmreg_lock;
 	struct cyclecounter cc;
 	struct timecounter tc;
+#endif /* CONFIG_IGB_PTP */
+
 	char fw_version[32];
 };
 

@@ -436,12 +441,11 @@ extern void igb_set_fw_version(struct igb_adapter *);
 #ifdef CONFIG_IGB_PTP
 extern void igb_ptp_init(struct igb_adapter *adapter);
 extern void igb_ptp_remove(struct igb_adapter *adapter);
-
 extern void igb_systim_to_hwtstamp(struct igb_adapter *adapter,
 				   struct skb_shared_hwtstamps *hwtstamps,
 				   u64 systim);
+#endif /* CONFIG_IGB_PTP */
 
-#endif
 static inline s32 igb_reset_phy(struct e1000_hw *hw)
 {
 	if (hw->phy.ops.reset)

diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index c4def55..6adb0f7 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c

@@ -2323,8 +2323,8 @@ static int igb_ethtool_get_ts_info(struct net_device *dev,
 
 	return 0;
 }
+#endif /* CONFIG_IGB_PTP */
 
-#endif
 static const struct ethtool_ops igb_ethtool_ops = {
 	.get_settings           = igb_get_settings,
 	.set_settings           = igb_set_settings,

@@ -2355,7 +2355,7 @@ static const struct ethtool_ops igb_ethtool_ops = {
 	.complete		= igb_ethtool_complete,
 #ifdef CONFIG_IGB_PTP
 	.get_ts_info		= igb_ethtool_get_ts_info,
-#endif
+#endif /* CONFIG_IGB_PTP */
 };
 
 void igb_set_ethtool_ops(struct net_device *netdev)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 73cc273..03477d7 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c

@@ -2180,11 +2180,12 @@ static int __devinit igb_probe(struct pci_dev *pdev,
 	}
 
 #endif
+
 #ifdef CONFIG_IGB_PTP
 	/* do hw tstamp init after resetting */
 	igb_ptp_init(adapter);
+#endif /* CONFIG_IGB_PTP */
 
-#endif
 	dev_info(&pdev->dev, "Intel(R) Gigabit Ethernet Network Connection\n");
 	/* print bus type/speed/width info */
 	dev_info(&pdev->dev, "%s: (PCIe:%s:%s) %pM\n",

@@ -2260,8 +2261,8 @@ static void __devexit igb_remove(struct pci_dev *pdev)
 	pm_runtime_get_noresume(&pdev->dev);
 #ifdef CONFIG_IGB_PTP
 	igb_ptp_remove(adapter);
+#endif /* CONFIG_IGB_PTP */
 
-#endif
 	/*
 	 * The watchdog timer may be rescheduled, so explicitly
 	 * disable watchdog from being rescheduled.

@@ -3184,8 +3185,10 @@ void igb_configure_rx_ring(struct igb_adapter *adapter,
 	srrctl |= (PAGE_SIZE / 2) >> E1000_SRRCTL_BSIZEPKT_SHIFT;
 #endif
 	srrctl |= E1000_SRRCTL_DESCTYPE_HDR_SPLIT_ALWAYS;
+#ifdef CONFIG_IGB_PTP
 	if (hw->mac.type >= e1000_82580)
 		srrctl |= E1000_SRRCTL_TIMESTAMP;
+#endif /* CONFIG_IGB_PTP */
 	/* Only set Drop Enable if we are supporting multiple queues */
 	if (adapter->vfs_allocated_count || adapter->num_rx_queues > 1)
 		srrctl |= E1000_SRRCTL_DROP_EN;

@@ -4229,9 +4232,11 @@ static __le32 igb_tx_cmd_type(u32 tx_flags)
 	if (tx_flags & IGB_TX_FLAGS_VLAN)
 		cmd_type |= cpu_to_le32(E1000_ADVTXD_DCMD_VLE);
 
+#ifdef CONFIG_IGB_PTP
 	/* set timestamp bit if present */
 	if (tx_flags & IGB_TX_FLAGS_TSTAMP)
 		cmd_type |= cpu_to_le32(E1000_ADVTXD_MAC_TSTAMP);
+#endif /* CONFIG_IGB_PTP */
 
 	/* set segmentation bits for TSO */
 	if (tx_flags & IGB_TX_FLAGS_TSO)

@@ -4462,10 +4467,12 @@ netdev_tx_t igb_xmit_frame_ring(struct sk_buff *skb,
 	first->bytecount = skb->len;
 	first->gso_segs = 1;
 
+#ifdef CONFIG_IGB_PTP
 	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
 		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 		tx_flags |= IGB_TX_FLAGS_TSTAMP;
 	}
+#endif /* CONFIG_IGB_PTP */
 
 	if (vlan_tx_tag_present(skb)) {
 		tx_flags |= IGB_TX_FLAGS_VLAN;

@@ -5772,8 +5779,8 @@ static void igb_tx_hwtstamp(struct igb_q_vector *q_vector,
 	igb_systim_to_hwtstamp(adapter, &shhwtstamps, regval);
 	skb_tstamp_tx(buffer_info->skb, &shhwtstamps);
 }
+#endif /* CONFIG_IGB_PTP */
 
-#endif
 /**
  * igb_clean_tx_irq - Reclaim resources after transmit completes
  * @q_vector: pointer to q_vector containing needed info

@@ -5821,8 +5828,8 @@ static bool igb_clean_tx_irq(struct igb_q_vector *q_vector)
 #ifdef CONFIG_IGB_PTP
 		/* retrieve hardware timestamp */
 		igb_tx_hwtstamp(q_vector, tx_buffer);
+#endif /* CONFIG_IGB_PTP */
 
-#endif
 		/* free the skb */
 		dev_kfree_skb_any(tx_buffer->skb);
 		tx_buffer->skb = NULL;

@@ -6033,8 +6040,8 @@ static void igb_rx_hwtstamp(struct igb_q_vector *q_vector,
 
 	igb_systim_to_hwtstamp(adapter, skb_hwtstamps(skb), regval);
 }
+#endif /* CONFIG_IGB_PTP */
 
-#endif
 static void igb_rx_vlan(struct igb_ring *ring,
 			union e1000_adv_rx_desc *rx_desc,
 			struct sk_buff *skb)

@@ -6147,7 +6154,7 @@ static bool igb_clean_rx_irq(struct igb_q_vector *q_vector, int budget)
 
 #ifdef CONFIG_IGB_PTP
 		igb_rx_hwtstamp(q_vector, rx_desc, skb);
-#endif
+#endif /* CONFIG_IGB_PTP */
 		igb_rx_hash(rx_ring, rx_desc, skb);
 		igb_rx_checksum(rx_ring, rx_desc, skb);
 		igb_rx_vlan(rx_ring, rx_desc, skb);

@@ -6340,6 +6347,7 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
 	return 0;
 }
 
+#ifdef CONFIG_IGB_PTP
 /**
  * igb_hwtstamp_ioctl - control hardware time stamping
  * @netdev:

@@ -6514,6 +6522,7 @@ static int igb_hwtstamp_ioctl(struct net_device *netdev,
 	return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ?
 		-EFAULT : 0;
 }
+#endif /* CONFIG_IGB_PTP */
 
 /**
  * igb_ioctl -

@@ -6528,8 +6537,10 @@ static int igb_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
 	case SIOCGMIIREG:
 	case SIOCSMIIREG:
 		return igb_mii_ioctl(netdev, ifr, cmd);
+#ifdef CONFIG_IGB_PTP
 	case SIOCSHWTSTAMP:
 		return igb_hwtstamp_ioctl(netdev, ifr, cmd);
+#endif /* CONFIG_IGB_PTP */
 	default:
 		return -EOPNOTSUPP;
 	}

-- 
1.7.11.4

On Thu, Aug 23, 2012 at 02:56:50AM -0700, Jeff Kirsher wrote:

From: Matthew Vick <redacted>

For users without CONFIG_IGB_PTP=y, we should not be compiling any PTP
code into the driver. Tidy up the wrapping in igb to support this.

Actually, you are doing more than that. You are adding a bunch of
comments onto the already existing #endifs.
 

This is legitimate, to reduce memory footprint.

But this is just churn. You aren't actually improving anything
here. It is already clear that the #endif belongs to the #ifdef just
three lines above.

If you are on some kind of campaign to comment all the endifs, then
you should say so, and, to be consistent, you should not overlook the
CONFIG_IGB_DCA blocks.

But I think it really isn't needed.

Thanks,
Richard

-----Original Message-----
From: Richard Cochran [mailto:richardcochran@gmail.com]
Sent: Thursday, August 23, 2012 4:04 AM
To: Kirsher, Jeffrey T
Cc: davem@davemloft.net; Vick, Matthew; netdev@vger.kernel.org;
gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

On Thu, Aug 23, 2012 at 02:56:50AM -0700, Jeff Kirsher wrote:

quoted

From: Matthew Vick <redacted>

For users without CONFIG_IGB_PTP=y, we should not be compiling any

PTP

quoted

code into the driver. Tidy up the wrapping in igb to support this.

Actually, you are doing more than that. You are adding a bunch of
comments onto the already existing #endifs.

Fair enough. Would you like me to update the patch description?

quoted

+#ifdef CONFIG_IGB_PTP
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info caps;
 	struct delayed_work overflow_work;
 	spinlock_t tmreg_lock;
 	struct cyclecounter cc;
 	struct timecounter tc;
+#endif /* CONFIG_IGB_PTP */
+

This is legitimate, to reduce memory footprint.

[...]

But this is just churn. You aren't actually improving anything here. It
is already clear that the #endif belongs to the #ifdef just three lines
above.

If you are on some kind of campaign to comment all the endifs, then you
should say so, and, to be consistent, you should not overlook the
CONFIG_IGB_DCA blocks.

But I think it really isn't needed.

Thanks,
Richard

I'm willing to work on a follow on patch to add proper endings to CONFIG_IGB_DCA blocks. The reasoning here was "I'm working on the wrapping for the code here anyway, so I might as well make it consistent."

Personally, I think it is an improvement. If you want to add #endif comments, you may as well be consistent about it.

Cheers,
Matthew

On Thu, Aug 23, 2012 at 04:09:30PM +0000, Vick, Matthew wrote:

quoted

-----Original Message-----
From: Richard Cochran [mailto:richardcochran@gmail.com]
Sent: Thursday, August 23, 2012 4:04 AM
To: Kirsher, Jeffrey T
Cc: davem@davemloft.net; Vick, Matthew; netdev@vger.kernel.org;
gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

On Thu, Aug 23, 2012 at 02:56:50AM -0700, Jeff Kirsher wrote:

quoted

From: Matthew Vick <redacted>

For users without CONFIG_IGB_PTP=y, we should not be compiling any

PTP

quoted

code into the driver. Tidy up the wrapping in igb to support this.

Actually, you are doing more than that. You are adding a bunch of
comments onto the already existing #endifs.

Fair enough. Would you like me to update the patch description?

Better would be to drop of the pendantic #endif /*CONFIG_FOO*/ stuff.
It is just churn.

Thanks,
Richard

-----Original Message-----
From: Richard Cochran [mailto:richardcochran@gmail.com]
Sent: Thursday, August 23, 2012 10:30 AM
To: Vick, Matthew
Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

On Thu, Aug 23, 2012 at 04:09:30PM +0000, Vick, Matthew wrote:

quoted

quoted

-----Original Message-----
From: Richard Cochran [mailto:richardcochran@gmail.com]
Sent: Thursday, August 23, 2012 4:04 AM
To: Kirsher, Jeffrey T
Cc: davem@davemloft.net; Vick, Matthew; netdev@vger.kernel.org;
gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net-next 10/13] igb: Tidy up wrapping for

CONFIG_IGB_PTP.

quoted

quoted

On Thu, Aug 23, 2012 at 02:56:50AM -0700, Jeff Kirsher wrote:

quoted

From: Matthew Vick <redacted>

For users without CONFIG_IGB_PTP=y, we should not be compiling

any

quoted

quoted

PTP

quoted

code into the driver. Tidy up the wrapping in igb to support

this.

quoted

quoted

Actually, you are doing more than that. You are adding a bunch of
comments onto the already existing #endifs.

Fair enough. Would you like me to update the patch description?

Better would be to drop of the pendantic #endif /*CONFIG_FOO*/ stuff.
It is just churn.

Thanks,
Richard

I'm willing to drop it, but I would like to drop it universally in the driver if that's the case. Is that acceptable? There's no overly long or complex wrapping section in the driver that I think merits being sloppy with the end comments.

Cheers,
Matthew

On Thu, Aug 23, 2012 at 05:40:01PM +0000, Vick, Matthew wrote:

quoted

Better would be to drop of the pendantic #endif /*CONFIG_FOO*/ stuff.
It is just churn.

Thanks,
Richard

I'm willing to drop it, but I would like to drop it universally in the driver if that's the case. Is that acceptable? There's no overly long or complex wrapping section in the driver that I think merits being sloppy with the end comments.

Well, I don't know about the other cases.

drivers/net/ethernet/intel/igb$ grep \#endif *.c | grep -v CONFIG | wc -l
28

drivers/net/ethernet/intel/igb$ grep \#endif *.c | grep CONFIG | wc -l
8

(none of these are for _IGB_PTP)

Having #endif comments is useful when it makes the code more clear. It
is a matter of taste, but I do think having such comments for just two
lines in between is ugly and silly.

Thanks,
Richard

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
On Behalf Of Richard Cochran
Sent: Thursday, August 23, 2012 10:30 AM
To: Vick, Matthew
Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

On Thu, Aug 23, 2012 at 04:09:30PM +0000, Vick, Matthew wrote:

quoted

quoted

-----Original Message-----
From: Richard Cochran [mailto:richardcochran@gmail.com]
Sent: Thursday, August 23, 2012 4:04 AM
To: Kirsher, Jeffrey T
Cc: davem@davemloft.net; Vick, Matthew; netdev@vger.kernel.org;
gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net-next 10/13] igb: Tidy up wrapping for

CONFIG_IGB_PTP.

quoted

quoted

On Thu, Aug 23, 2012 at 02:56:50AM -0700, Jeff Kirsher wrote:

quoted

From: Matthew Vick <redacted>

For users without CONFIG_IGB_PTP=y, we should not be compiling any

PTP

quoted

code into the driver. Tidy up the wrapping in igb to support this.

Actually, you are doing more than that. You are adding a bunch of
comments onto the already existing #endifs.

Fair enough. Would you like me to update the patch description?

Better would be to drop of the pendantic #endif /*CONFIG_FOO*/ stuff.
It is just churn.

Personally disagree here. I do agree that the churn is annoying with how it breaks git blame, however, in general I prefer tags at the end of #ifdefs even for short ones because it increases my ability to quickly spot matches. The end comment aligns with the starting comment, and even for small blocks makes it easier to process. It is less necessary the smaller the block, but I always prefer to have it than not.

That said, it is nice when git blame points to the right place, and the comments aren't super necessary for such short blocks.

- Jake

Thanks,
Richard
--
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

-----Original Message-----
From: Keller, Jacob E
Sent: Thursday, August 23, 2012 11:04 AM
To: Richard Cochran; Vick, Matthew
Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
gospo@redhat.com; sassmann@redhat.com
Subject: RE: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[...]

Personally disagree here. I do agree that the churn is annoying with
how it breaks git blame, however, in general I prefer tags at the end
of #ifdefs even for short ones because it increases my ability to
quickly spot matches. The end comment aligns with the starting comment,
and even for small blocks makes it easier to process. It is less
necessary the smaller the block, but I always prefer to have it than
not.

That said, it is nice when git blame points to the right place, and the
comments aren't super necessary for such short blocks.

- Jake

I tend to agree with Jake here--I like having the information. I'm fine removing them, but I'd like to do it for all CONFIG_IGB_PTP wrapping if we're going to do it. What do you think, Richard?

Cheers,
Matthew

On Thu, Aug 23, 2012 at 06:40:25PM +0000, Vick, Matthew wrote:

I tend to agree with Jake here--I like having the information. I'm fine removing them, but I'd like to do it for all CONFIG_IGB_PTP wrapping if we're going to do it. What do you think, Richard?

I took a look at the #ifdefs in igb_main.c, and I would say they are
okay as is, except for the CONFIG_PM ones, which could use a comment
at the #endifs.

(a matter of taste, though)

Thanks,
Richard

On Thu, Aug 23, 2012 at 06:40:25PM +0000, Vick, Matthew wrote:

I tend to agree with Jake here--I like having the information. I'm fine removing them, but I'd like to do it for all CONFIG_IGB_PTP wrapping if we're going to do it. What do you think, Richard?

Come to think of it, I never liked the CONFIG_IGB_PTP very much in the
first place. These were added after the fact by Jeff Kirsher. He had
said off list that there was some issue with CONFIG_PTP_1588_CLOCK and
igb as a module, or something like that. At that time I said, just go
ahead and fix it up.

I think it would be better if the "time stamp all Rx packets" of the
82580 were always available, and that the PHC feature always be
compiled when CONFIG_PTP_1588_CLOCK is selected.

Maybe you could ask Jeff what the issue was, and then see if there is
a way to remove CONFIG_IGB_PTP altogether.

Thanks,
Richard

On Fri, 2012-08-24 at 12:10 +0200, Richard Cochran wrote:

On Thu, Aug 23, 2012 at 06:40:25PM +0000, Vick, Matthew wrote:

quoted

I tend to agree with Jake here--I like having the information. I'm fine removing them, but I'd like to do it for all CONFIG_IGB_PTP wrapping if we're going to do it. What do you think, Richard?

Come to think of it, I never liked the CONFIG_IGB_PTP very much in the
first place. These were added after the fact by Jeff Kirsher. He had
said off list that there was some issue with CONFIG_PTP_1588_CLOCK and
igb as a module, or something like that. At that time I said, just go
ahead and fix it up.

If there is some feature of a driver that depends on an optional
modularisable subsystem, and you want the driver to be built without
that feature when the subsystem is missing, then the feature has to be
disabled when the driver is built-in and the subsystem is a module.  So
in general you need:

config DRIVER_FEATURE
	bool "This is a really neat feature for the driver"
	depends on DRIVER && SUBSYSTEM && !(DRIVER=y && SUBYSTEM=m)

I think it would be better if the "time stamp all Rx packets" of the
82580 were always available, and that the PHC feature always be
compiled when CONFIG_PTP_1588_CLOCK is selected.

Maybe you could ask Jeff what the issue was, and then see if there is
a way to remove CONFIG_IGB_PTP altogether.

Even if they are to be enabled automatically, CONFIG_IGB_PTP and similar
config symbols are important as shorthand.  So I think what you want is:

config IGB_PTP
	def_bool y
	depends on IGB && PTP_1588_CLOCK && !(IGB=y && PTP_1588_CLOCK=m)

(But currently it's actually *selecting* PTP_1588_CLOCK.  We should
probably have drivers consistently select or depend on it, not a
mixture.)

Ben.

-- 
Ben Hutchings, Staff 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.

-----Original Message-----
From: Ben Hutchings [mailto:bhutchings@solarflare.com]
Sent: Friday, August 24, 2012 9:51 AM
To: Richard Cochran
Cc: Vick, Matthew; Keller, Jacob E; Kirsher, Jeffrey T;
davem@davemloft.net; netdev@vger.kernel.org; gospo@redhat.com;
sassmann@redhat.com
Subject: Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

On Fri, 2012-08-24 at 12:10 +0200, Richard Cochran wrote:

quoted

On Thu, Aug 23, 2012 at 06:40:25PM +0000, Vick, Matthew wrote:

quoted

I tend to agree with Jake here--I like having the information. I'm

fine removing them, but I'd like to do it for all CONFIG_IGB_PTP wrapping
if we're going to do it. What do you think, Richard?

quoted

Come to think of it, I never liked the CONFIG_IGB_PTP very much in the
first place. These were added after the fact by Jeff Kirsher. He had
said off list that there was some issue with CONFIG_PTP_1588_CLOCK and
igb as a module, or something like that. At that time I said, just go
ahead and fix it up.

If there is some feature of a driver that depends on an optional
modularisable subsystem, and you want the driver to be built without that
feature when the subsystem is missing, then the feature has to be disabled
when the driver is built-in and the subsystem is a module.  So in general
you need:

config DRIVER_FEATURE
	bool "This is a really neat feature for the driver"
	depends on DRIVER && SUBSYSTEM && !(DRIVER=y && SUBYSTEM=m)

quoted

I think it would be better if the "time stamp all Rx packets" of the
82580 were always available, and that the PHC feature always be
compiled when CONFIG_PTP_1588_CLOCK is selected.

Maybe you could ask Jeff what the issue was, and then see if there is
a way to remove CONFIG_IGB_PTP altogether.

Even if they are to be enabled automatically, CONFIG_IGB_PTP and similar
config symbols are important as shorthand.  So I think what you want is:

config IGB_PTP
	def_bool y
	depends on IGB && PTP_1588_CLOCK && !(IGB=y && PTP_1588_CLOCK=m)

(But currently it's actually *selecting* PTP_1588_CLOCK.  We should
probably have drivers consistently select or depend on it, not a
mixture.)

Ben.

--
Ben Hutchings, Staff 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.

The IGP_PTP is necessary otherwise you have to do something like #if defined(CONFIG_PTP_1588_CLOCK), or #ifdef CONFIG_PTP_1588_CLOCK \ #ifdef CONFIG_PTP_1588_CLOCK_MODULE

The main reason that I wouldn't want to un-wrap the timestamping is because the hwtstamp ioctl is somewhat problematic because it is almost all ptp only. Also, some of the parts for igb driver don't support timestamp all, they only support ptp only packets, and this would be a lot more confusing since I would say still only allow that if ptp is on.. (since those values are useless except with the PHC clock)

- Jake

On Fri, Aug 24, 2012 at 07:38:38PM +0000, Keller, Jacob E wrote:
 

The IGP_PTP is necessary otherwise you have to do something like #if defined(CONFIG_PTP_1588_CLOCK), or #ifdef CONFIG_PTP_1588_CLOCK \ #ifdef CONFIG_PTP_1588_CLOCK_MODULE

Yes, but maybe use IGP_PTP as Ben described? 
 

The main reason that I wouldn't want to un-wrap the timestamping is because the hwtstamp ioctl is somewhat problematic because it is almost all ptp only. Also, some of the parts for igb driver don't support timestamp all, they only support ptp only packets, and this would be a lot more confusing since I would say still only allow that if ptp is on.. (since those values are useless except with the PHC clock)

I keep trying to explain that receive time stamping (HWTSTAMP_FILTER_ALL)
is useful all by itself. It has nothing to do with PTP, and the 82580
can do this with very little ** overhead.

I myself have used this feature when debugging and profiling quasi
real time Ethernet protocols like EtherCAT and IEC61850-9-2. The other
drivers that offer this (gianfar and vxge) do so without any CONFIG
conditionals.

IMHO, the Rx time stamping feature should always be available. It is
really not much different than the vlan feature.

Thanks,
Richard

** Both igb_rx_hwtstamp and igb_tx_hwtstamp only add a single test
   when time stamping is not enabled.

-----Original Message-----
From: Richard Cochran [mailto:richardcochran@gmail.com]
Sent: Friday, August 24, 2012 11:20 PM
To: Keller, Jacob E
Cc: Ben Hutchings; Vick, Matthew; Kirsher, Jeffrey T; davem@davemloft.net;
netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

On Fri, Aug 24, 2012 at 07:38:38PM +0000, Keller, Jacob E wrote:

quoted

The IGP_PTP is necessary otherwise you have to do something like #if
defined(CONFIG_PTP_1588_CLOCK), or #ifdef CONFIG_PTP_1588_CLOCK \
#ifdef CONFIG_PTP_1588_CLOCK_MODULE

Yes, but maybe use IGP_PTP as Ben described?

That's exactly what I was saying :) That's the reason why IGP_PTP is necessary.

quoted

The main reason that I wouldn't want to un-wrap the timestamping is
because the hwtstamp ioctl is somewhat problematic because it is
almost all ptp only. Also, some of the parts for igb driver don't
support timestamp all, they only support ptp only packets, and this
would be a lot more confusing since I would say still only allow that
if ptp is on.. (since those values are useless except with the PHC
clock)

I keep trying to explain that receive time stamping (HWTSTAMP_FILTER_ALL)
is useful all by itself. It has nothing to do with PTP, and the 82580 can
do this with very little ** overhead.

I myself have used this feature when debugging and profiling quasi real
time Ethernet protocols like EtherCAT and IEC61850-9-2. The other drivers
that offer this (gianfar and vxge) do so without any CONFIG conditionals.

IMHO, the Rx time stamping feature should always be available. It is
really not much different than the vlan feature.

IMO it should be but only for parts with HWTSTAMP_FILTER_ALL, and only for that mode (other modes should be ignored) because timestamping only PTP packets is a PTP feature, so this change should still disable other modes if they exist.

Thanks,
Richard

** Both igb_rx_hwtstamp and igb_tx_hwtstamp only add a single test
   when time stamping is not enabled.

On Sun, Aug 26, 2012 at 01:33:42AM +0000, Keller, Jacob E wrote:

IMO it should be but only for parts with HWTSTAMP_FILTER_ALL, and only for that mode (other modes should be ignored) because timestamping only PTP packets is a PTP feature, so this change should still disable other modes if they exist.

Yes, the other Rx modes are PTP specific, and they don't make sense
unless the PTP clock is also enabled.

IIRC, on the Tx side, the 82580 can time stamp any marked packet
regardless of the packet contents, so this should also be always
available, along with HWTSTAMP_FILTER_ALL.

Thanks,
Richard

On Thu, Aug 23, 2012 at 02:56:50AM -0700, Jeff Kirsher wrote:

From: Matthew Vick <redacted>

For users without CONFIG_IGB_PTP=y, we should not be compiling any PTP
code into the driver. Tidy up the wrapping in igb to support this.

I would appreciate being put on Cc: for any PTP stuff.

Also, if you really want to improve the IGB driver, maybe you could
look at what happens to the time stamping when you do an ifdown/ifup
on the card. It seems to screw everything (observed on kernel 3.5).

Thanks,
Richard

-----Original Message-----
From: Richard Cochran [mailto:richardcochran@gmail.com]
Sent: Thursday, August 23, 2012 4:29 AM
To: Kirsher, Jeffrey T
Cc: davem@davemloft.net; Vick, Matthew; netdev@vger.kernel.org;
gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

On Thu, Aug 23, 2012 at 02:56:50AM -0700, Jeff Kirsher wrote:

quoted

From: Matthew Vick <redacted>

For users without CONFIG_IGB_PTP=y, we should not be compiling any

PTP

quoted

code into the driver. Tidy up the wrapping in igb to support this.

I would appreciate being put on Cc: for any PTP stuff.

Also, if you really want to improve the IGB driver, maybe you could
look at what happens to the time stamping when you do an ifdown/ifup on
the card. It seems to screw everything (observed on kernel 3.5).

Thanks,
Richard

Will do going forward. I appreciate the review.

I actually have two patches coming up that bring some further improvements--these first couple patches are foundation for those. In a patch coming up, the ifdown/ifup issue gets resolved.

Cheers,
Matthew

From: Matthew Vick <redacted>

Update ethtool_get_ts_info to not report any supported functionality on
82575 and add support for V2 Sync and V2 Delay packets. In the case
where CONFIG_IGB_PTP is not defined, we should be reporting default
values.

Signed-off-by: Matthew Vick <redacted>
Acked-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Jeff Pieper <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
 drivers/net/ethernet/intel/igb/igb_ethtool.c | 59 +++++++++++++++++-----------
 1 file changed, 37 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index d1a120e..e18fd20 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c

@@ -2280,37 +2280,54 @@ static void igb_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 	}
 }
 
-#ifdef CONFIG_IGB_PTP
 static int igb_get_ts_info(struct net_device *dev,
 				   struct ethtool_ts_info *info)
 {
 	struct igb_adapter *adapter = netdev_priv(dev);
 
-	info->so_timestamping =
-		SOF_TIMESTAMPING_TX_HARDWARE |
-		SOF_TIMESTAMPING_RX_HARDWARE |
-		SOF_TIMESTAMPING_RAW_HARDWARE;
+	switch (adapter->hw.mac.type) {
+#ifdef CONFIG_IGB_PTP
+	case e1000_82576:
+	case e1000_82580:
+	case e1000_i350:
+	case e1000_i210:
+	case e1000_i211:
+		info->so_timestamping =
+			SOF_TIMESTAMPING_TX_HARDWARE |
+			SOF_TIMESTAMPING_RX_HARDWARE |
+			SOF_TIMESTAMPING_RAW_HARDWARE;
 
-	if (adapter->ptp_clock)
-		info->phc_index = ptp_clock_index(adapter->ptp_clock);
-	else
-		info->phc_index = -1;
+		if (adapter->ptp_clock)
+			info->phc_index = ptp_clock_index(adapter->ptp_clock);
+		else
+			info->phc_index = -1;
 
-	info->tx_types =
-		(1 << HWTSTAMP_TX_OFF) |
-		(1 << HWTSTAMP_TX_ON);
+		info->tx_types =
+			(1 << HWTSTAMP_TX_OFF) |
+			(1 << HWTSTAMP_TX_ON);
 
-	info->rx_filters =
-		(1 << HWTSTAMP_FILTER_NONE) |
-		(1 << HWTSTAMP_FILTER_ALL) |
-		(1 << HWTSTAMP_FILTER_SOME) |
-		(1 << HWTSTAMP_FILTER_PTP_V1_L4_SYNC) |
-		(1 << HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ) |
-		(1 << HWTSTAMP_FILTER_PTP_V2_EVENT);
+		info->rx_filters = 1 << HWTSTAMP_FILTER_NONE;
+
+		/* 82576 does not support timestamping all packets. */
+		if (adapter->hw.mac.type >= e1000_82580)
+			info->rx_filters |= 1 << HWTSTAMP_FILTER_ALL;
+		else
+			info->rx_filters |=
+				(1 << HWTSTAMP_FILTER_PTP_V1_L4_SYNC) |
+				(1 << HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ) |
+				(1 << HWTSTAMP_FILTER_PTP_V2_SYNC) |
+				(1 << HWTSTAMP_FILTER_PTP_V2_DELAY_REQ) |
+				(1 << HWTSTAMP_FILTER_PTP_V2_EVENT);
+
+		break;
+#endif /* CONFIG_IGB_PTP */
+	default:
+		return ethtool_op_get_ts_info(dev, info);
+		break;
+	}
 
 	return 0;
 }
-#endif /* CONFIG_IGB_PTP */
 
 static int igb_ethtool_begin(struct net_device *netdev)
 {

@@ -2351,9 +2368,7 @@ static const struct ethtool_ops igb_ethtool_ops = {
 	.get_ethtool_stats      = igb_get_ethtool_stats,
 	.get_coalesce           = igb_get_coalesce,
 	.set_coalesce           = igb_set_coalesce,
-#ifdef CONFIG_IGB_PTP
 	.get_ts_info            = igb_get_ts_info,
-#endif /* CONFIG_IGB_PTP */
 	.begin			= igb_ethtool_begin,
 	.complete		= igb_ethtool_complete,
 };

-- 
1.7.11.4

On Thu, Aug 23, 2012 at 02:56:52AM -0700, Jeff Kirsher wrote:

From: Matthew Vick <redacted>

Update ethtool_get_ts_info to not report any supported functionality on
82575 and add support for V2 Sync and V2 Delay packets. In the case
where CONFIG_IGB_PTP is not defined, we should be reporting default
values.

...

+#endif /* CONFIG_IGB_PTP */
+	default:
+		return ethtool_op_get_ts_info(dev, info);

This is wrong. Using this function advertises that you support SW TX
time stamps, which you do not.

Thanks,
Richard

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-
owner@vger.kernel.org] On Behalf Of Richard Cochran
Sent: Thursday, August 23, 2012 4:24 AM
To: Kirsher, Jeffrey T
Cc: davem@davemloft.net; Vick, Matthew; netdev@vger.kernel.org;
gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net-next 12/13] igb: Correct PTP support query from
ethtool.

On Thu, Aug 23, 2012 at 02:56:52AM -0700, Jeff Kirsher wrote:

quoted

From: Matthew Vick <redacted>

Update ethtool_get_ts_info to not report any supported functionality
on
82575 and add support for V2 Sync and V2 Delay packets. In the case
where CONFIG_IGB_PTP is not defined, we should be reporting default
values.

quoted

diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c

b/drivers/net/ethernet/intel/igb/igb_ethtool.c

...

quoted

+#endif /* CONFIG_IGB_PTP */
+	default:
+		return ethtool_op_get_ts_info(dev, info);

This is wrong. Using this function advertises that you support SW TX
time stamps, which you do not.

Thanks,
Richard

Thanks--good catch! I thought we supported SW timestamps already. Since we don't, I'll spin a v2 of this patch.

Cheers,
Matthew

From: Matthew Vick <redacted>

Where possible, move PTP-related functions into igb_ptp.c and update the
names of functions and variables to match the established coding style
in the files and specify that they are PTP-specific.

Signed-off-by: Matthew Vick <redacted>
Tested-by: Jeff Pieper  <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
 drivers/net/ethernet/intel/igb/igb.h         |  17 +-
 drivers/net/ethernet/intel/igb/igb_ethtool.c |  34 +-
 drivers/net/ethernet/intel/igb/igb_main.c    | 257 +-------------
 drivers/net/ethernet/intel/igb/igb_ptp.c     | 485 ++++++++++++++++++++-------
 4 files changed, 398 insertions(+), 395 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index a3b5b90..7973469 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h

@@ -344,7 +344,6 @@ struct igb_adapter {
 
 	/* OS defined structs */
 	struct pci_dev *pdev;
-	struct hwtstamp_config hwtstamp_config;
 
 	spinlock_t stats64_lock;
 	struct rtnl_link_stats64 stats64;

@@ -380,8 +379,8 @@ struct igb_adapter {
 
 #ifdef CONFIG_IGB_PTP
 	struct ptp_clock *ptp_clock;
-	struct ptp_clock_info caps;
-	struct delayed_work overflow_work;
+	struct ptp_clock_info ptp_caps;
+	struct delayed_work ptp_overflow_work;
 	spinlock_t tmreg_lock;
 	struct cyclecounter cc;
 	struct timecounter tc;

@@ -440,10 +439,14 @@ extern void igb_power_up_link(struct igb_adapter *);
 extern void igb_set_fw_version(struct igb_adapter *);
 #ifdef CONFIG_IGB_PTP
 extern void igb_ptp_init(struct igb_adapter *adapter);
-extern void igb_ptp_remove(struct igb_adapter *adapter);
-extern void igb_systim_to_hwtstamp(struct igb_adapter *adapter,
-				   struct skb_shared_hwtstamps *hwtstamps,
-				   u64 systim);
+extern void igb_ptp_stop(struct igb_adapter *adapter);
+extern void igb_ptp_tx_hwtstamp(struct igb_q_vector *q_vector,
+				struct igb_tx_buffer *buffer_info);
+extern void igb_ptp_rx_hwtstamp(struct igb_q_vector *q_vector,
+				union e1000_adv_rx_desc *rx_desc,
+				struct sk_buff *skb);
+extern int igb_ptp_hwtstamp_ioctl(struct net_device *netdev,
+				  struct ifreq *ifr, int cmd);
 #endif /* CONFIG_IGB_PTP */
 
 static inline s32 igb_reset_phy(struct e1000_hw *hw)

diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 6adb0f7..d1a120e 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c

@@ -2280,21 +2280,8 @@ static void igb_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 	}
 }
 
-static int igb_ethtool_begin(struct net_device *netdev)
-{
-	struct igb_adapter *adapter = netdev_priv(netdev);
-	pm_runtime_get_sync(&adapter->pdev->dev);
-	return 0;
-}
-
-static void igb_ethtool_complete(struct net_device *netdev)
-{
-	struct igb_adapter *adapter = netdev_priv(netdev);
-	pm_runtime_put(&adapter->pdev->dev);
-}
-
 #ifdef CONFIG_IGB_PTP
-static int igb_ethtool_get_ts_info(struct net_device *dev,
+static int igb_get_ts_info(struct net_device *dev,
 				   struct ethtool_ts_info *info)
 {
 	struct igb_adapter *adapter = netdev_priv(dev);

@@ -2325,6 +2312,19 @@ static int igb_ethtool_get_ts_info(struct net_device *dev,
 }
 #endif /* CONFIG_IGB_PTP */
 
+static int igb_ethtool_begin(struct net_device *netdev)
+{
+	struct igb_adapter *adapter = netdev_priv(netdev);
+	pm_runtime_get_sync(&adapter->pdev->dev);
+	return 0;
+}
+
+static void igb_ethtool_complete(struct net_device *netdev)
+{
+	struct igb_adapter *adapter = netdev_priv(netdev);
+	pm_runtime_put(&adapter->pdev->dev);
+}
+
 static const struct ethtool_ops igb_ethtool_ops = {
 	.get_settings           = igb_get_settings,
 	.set_settings           = igb_set_settings,

@@ -2351,11 +2351,11 @@ static const struct ethtool_ops igb_ethtool_ops = {
 	.get_ethtool_stats      = igb_get_ethtool_stats,
 	.get_coalesce           = igb_get_coalesce,
 	.set_coalesce           = igb_set_coalesce,
-	.begin			= igb_ethtool_begin,
-	.complete		= igb_ethtool_complete,
 #ifdef CONFIG_IGB_PTP
-	.get_ts_info		= igb_ethtool_get_ts_info,
+	.get_ts_info            = igb_get_ts_info,
 #endif /* CONFIG_IGB_PTP */
+	.begin			= igb_ethtool_begin,
+	.complete		= igb_ethtool_complete,
 };
 
 void igb_set_ethtool_ops(struct net_device *netdev)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 03477d7..6e39f0c 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c

@@ -2260,7 +2260,7 @@ static void __devexit igb_remove(struct pci_dev *pdev)
 
 	pm_runtime_get_noresume(&pdev->dev);
 #ifdef CONFIG_IGB_PTP
-	igb_ptp_remove(adapter);
+	igb_ptp_stop(adapter);
 #endif /* CONFIG_IGB_PTP */
 
 	/*

@@ -5750,37 +5750,6 @@ static int igb_poll(struct napi_struct *napi, int budget)
 	return 0;
 }
 
-#ifdef CONFIG_IGB_PTP
-/**
- * igb_tx_hwtstamp - utility function which checks for TX time stamp
- * @q_vector: pointer to q_vector containing needed info
- * @buffer: pointer to igb_tx_buffer structure
- *
- * If we were asked to do hardware stamping and such a time stamp is
- * available, then it must have been for this skb here because we only
- * allow only one such packet into the queue.
- */
-static void igb_tx_hwtstamp(struct igb_q_vector *q_vector,
-			    struct igb_tx_buffer *buffer_info)
-{
-	struct igb_adapter *adapter = q_vector->adapter;
-	struct e1000_hw *hw = &adapter->hw;
-	struct skb_shared_hwtstamps shhwtstamps;
-	u64 regval;
-
-	/* if skb does not support hw timestamp or TX stamp not valid exit */
-	if (likely(!(buffer_info->tx_flags & IGB_TX_FLAGS_TSTAMP)) ||
-	    !(rd32(E1000_TSYNCTXCTL) & E1000_TSYNCTXCTL_VALID))
-		return;
-
-	regval = rd32(E1000_TXSTMPL);
-	regval |= (u64)rd32(E1000_TXSTMPH) << 32;
-
-	igb_systim_to_hwtstamp(adapter, &shhwtstamps, regval);
-	skb_tstamp_tx(buffer_info->skb, &shhwtstamps);
-}
-#endif /* CONFIG_IGB_PTP */
-
 /**
  * igb_clean_tx_irq - Reclaim resources after transmit completes
  * @q_vector: pointer to q_vector containing needed info

@@ -5827,7 +5796,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector *q_vector)
 
 #ifdef CONFIG_IGB_PTP
 		/* retrieve hardware timestamp */
-		igb_tx_hwtstamp(q_vector, tx_buffer);
+		igb_ptp_tx_hwtstamp(q_vector, tx_buffer);
 #endif /* CONFIG_IGB_PTP */
 
 		/* free the skb */

@@ -6001,47 +5970,6 @@ static inline void igb_rx_hash(struct igb_ring *ring,
 		skb->rxhash = le32_to_cpu(rx_desc->wb.lower.hi_dword.rss);
 }
 
-#ifdef CONFIG_IGB_PTP
-static void igb_rx_hwtstamp(struct igb_q_vector *q_vector,
-			    union e1000_adv_rx_desc *rx_desc,
-			    struct sk_buff *skb)
-{
-	struct igb_adapter *adapter = q_vector->adapter;
-	struct e1000_hw *hw = &adapter->hw;
-	u64 regval;
-
-	if (!igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP |
-				       E1000_RXDADV_STAT_TS))
-		return;
-
-	/*
-	 * If this bit is set, then the RX registers contain the time stamp. No
-	 * other packet will be time stamped until we read these registers, so
-	 * read the registers to make them available again. Because only one
-	 * packet can be time stamped at a time, we know that the register
-	 * values must belong to this one here and therefore we don't need to
-	 * compare any of the additional attributes stored for it.
-	 *
-	 * If nothing went wrong, then it should have a shared tx_flags that we
-	 * can turn into a skb_shared_hwtstamps.
-	 */
-	if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) {
-		u32 *stamp = (u32 *)skb->data;
-		regval = le32_to_cpu(*(stamp + 2));
-		regval |= (u64)le32_to_cpu(*(stamp + 3)) << 32;
-		skb_pull(skb, IGB_TS_HDR_LEN);
-	} else {
-		if(!(rd32(E1000_TSYNCRXCTL) & E1000_TSYNCRXCTL_VALID))
-			return;
-
-		regval = rd32(E1000_RXSTMPL);
-		regval |= (u64)rd32(E1000_RXSTMPH) << 32;
-	}
-
-	igb_systim_to_hwtstamp(adapter, skb_hwtstamps(skb), regval);
-}
-#endif /* CONFIG_IGB_PTP */
-
 static void igb_rx_vlan(struct igb_ring *ring,
 			union e1000_adv_rx_desc *rx_desc,
 			struct sk_buff *skb)

@@ -6153,7 +6081,7 @@ static bool igb_clean_rx_irq(struct igb_q_vector *q_vector, int budget)
 		}
 
 #ifdef CONFIG_IGB_PTP
-		igb_rx_hwtstamp(q_vector, rx_desc, skb);
+		igb_ptp_rx_hwtstamp(q_vector, rx_desc, skb);
 #endif /* CONFIG_IGB_PTP */
 		igb_rx_hash(rx_ring, rx_desc, skb);
 		igb_rx_checksum(rx_ring, rx_desc, skb);

@@ -6347,183 +6275,6 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
 	return 0;
 }
 
-#ifdef CONFIG_IGB_PTP
-/**
- * igb_hwtstamp_ioctl - control hardware time stamping
- * @netdev:
- * @ifreq:
- * @cmd:
- *
- * Outgoing time stamping can be enabled and disabled. Play nice and
- * disable it when requested, although it shouldn't case any overhead
- * when no packet needs it. At most one packet in the queue may be
- * marked for time stamping, otherwise it would be impossible to tell
- * for sure to which packet the hardware time stamp belongs.
- *
- * Incoming time stamping has to be configured via the hardware
- * filters. Not all combinations are supported, in particular event
- * type has to be specified. Matching the kind of event packet is
- * not supported, with the exception of "all V2 events regardless of
- * level 2 or 4".
- *
- **/
-static int igb_hwtstamp_ioctl(struct net_device *netdev,
-			      struct ifreq *ifr, int cmd)
-{
-	struct igb_adapter *adapter = netdev_priv(netdev);
-	struct e1000_hw *hw = &adapter->hw;
-	struct hwtstamp_config config;
-	u32 tsync_tx_ctl = E1000_TSYNCTXCTL_ENABLED;
-	u32 tsync_rx_ctl = E1000_TSYNCRXCTL_ENABLED;
-	u32 tsync_rx_cfg = 0;
-	bool is_l4 = false;
-	bool is_l2 = false;
-	u32 regval;
-
-	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
-		return -EFAULT;
-
-	/* reserved for future extensions */
-	if (config.flags)
-		return -EINVAL;
-
-	switch (config.tx_type) {
-	case HWTSTAMP_TX_OFF:
-		tsync_tx_ctl = 0;
-	case HWTSTAMP_TX_ON:
-		break;
-	default:
-		return -ERANGE;
-	}
-
-	switch (config.rx_filter) {
-	case HWTSTAMP_FILTER_NONE:
-		tsync_rx_ctl = 0;
-		break;
-	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
-	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
-	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
-	case HWTSTAMP_FILTER_ALL:
-		/*
-		 * register TSYNCRXCFG must be set, therefore it is not
-		 * possible to time stamp both Sync and Delay_Req messages
-		 * => fall back to time stamping all packets
-		 */
-		tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_ALL;
-		config.rx_filter = HWTSTAMP_FILTER_ALL;
-		break;
-	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
-		tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_L4_V1;
-		tsync_rx_cfg = E1000_TSYNCRXCFG_PTP_V1_SYNC_MESSAGE;
-		is_l4 = true;
-		break;
-	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
-		tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_L4_V1;
-		tsync_rx_cfg = E1000_TSYNCRXCFG_PTP_V1_DELAY_REQ_MESSAGE;
-		is_l4 = true;
-		break;
-	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
-	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
-		tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_L2_L4_V2;
-		tsync_rx_cfg = E1000_TSYNCRXCFG_PTP_V2_SYNC_MESSAGE;
-		is_l2 = true;
-		is_l4 = true;
-		config.rx_filter = HWTSTAMP_FILTER_SOME;
-		break;
-	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
-	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
-		tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_L2_L4_V2;
-		tsync_rx_cfg = E1000_TSYNCRXCFG_PTP_V2_DELAY_REQ_MESSAGE;
-		is_l2 = true;
-		is_l4 = true;
-		config.rx_filter = HWTSTAMP_FILTER_SOME;
-		break;
-	case HWTSTAMP_FILTER_PTP_V2_EVENT:
-	case HWTSTAMP_FILTER_PTP_V2_SYNC:
-	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
-		tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_EVENT_V2;
-		config.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
-		is_l2 = true;
-		is_l4 = true;
-		break;
-	default:
-		return -ERANGE;
-	}
-
-	if (hw->mac.type == e1000_82575) {
-		if (tsync_rx_ctl | tsync_tx_ctl)
-			return -EINVAL;
-		return 0;
-	}
-
-	/*
-	 * Per-packet timestamping only works if all packets are
-	 * timestamped, so enable timestamping in all packets as
-	 * long as one rx filter was configured.
-	 */
-	if ((hw->mac.type >= e1000_82580) && tsync_rx_ctl) {
-		tsync_rx_ctl = E1000_TSYNCRXCTL_ENABLED;
-		tsync_rx_ctl |= E1000_TSYNCRXCTL_TYPE_ALL;
-	}
-
-	/* enable/disable TX */
-	regval = rd32(E1000_TSYNCTXCTL);
-	regval &= ~E1000_TSYNCTXCTL_ENABLED;
-	regval |= tsync_tx_ctl;
-	wr32(E1000_TSYNCTXCTL, regval);
-
-	/* enable/disable RX */
-	regval = rd32(E1000_TSYNCRXCTL);
-	regval &= ~(E1000_TSYNCRXCTL_ENABLED | E1000_TSYNCRXCTL_TYPE_MASK);
-	regval |= tsync_rx_ctl;
-	wr32(E1000_TSYNCRXCTL, regval);
-
-	/* define which PTP packets are time stamped */
-	wr32(E1000_TSYNCRXCFG, tsync_rx_cfg);
-
-	/* define ethertype filter for timestamped packets */
-	if (is_l2)
-		wr32(E1000_ETQF(3),
-		                (E1000_ETQF_FILTER_ENABLE | /* enable filter */
-		                 E1000_ETQF_1588 | /* enable timestamping */
-		                 ETH_P_1588));     /* 1588 eth protocol type */
-	else
-		wr32(E1000_ETQF(3), 0);
-
-#define PTP_PORT 319
-	/* L4 Queue Filter[3]: filter by destination port and protocol */
-	if (is_l4) {
-		u32 ftqf = (IPPROTO_UDP /* UDP */
-			| E1000_FTQF_VF_BP /* VF not compared */
-			| E1000_FTQF_1588_TIME_STAMP /* Enable Timestamping */
-			| E1000_FTQF_MASK); /* mask all inputs */
-		ftqf &= ~E1000_FTQF_MASK_PROTO_BP; /* enable protocol check */
-
-		wr32(E1000_IMIR(3), htons(PTP_PORT));
-		wr32(E1000_IMIREXT(3),
-		     (E1000_IMIREXT_SIZE_BP | E1000_IMIREXT_CTRL_BP));
-		if (hw->mac.type == e1000_82576) {
-			/* enable source port check */
-			wr32(E1000_SPQF(3), htons(PTP_PORT));
-			ftqf &= ~E1000_FTQF_MASK_SOURCE_PORT_BP;
-		}
-		wr32(E1000_FTQF(3), ftqf);
-	} else {
-		wr32(E1000_FTQF(3), E1000_FTQF_MASK);
-	}
-	wrfl();
-
-	adapter->hwtstamp_config = config;
-
-	/* clear TX/RX time stamp registers, just to be sure */
-	regval = rd32(E1000_TXSTMPH);
-	regval = rd32(E1000_RXSTMPH);
-
-	return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ?
-		-EFAULT : 0;
-}
-#endif /* CONFIG_IGB_PTP */
-
 /**
  * igb_ioctl -
  * @netdev: