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

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

[Jeff Kirsher]

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

[net-next 01/13] e1000e: use correct type for read of 32-bit register

[Jeff Kirsher]

From: Bruce Allan <bruce.w.allan@intel.com>

The POEMB register is 32 bits, not 16.

Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 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

[net-next 02/13] e1000e: cleanup strict checkpatch check

[Jeff Kirsher]

From: Bruce Allan <bruce.w.allan@intel.com>

CHECK: multiple assignments should be avoided

Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 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

[net-next 03/13] e1000e: cleanup - remove inapplicable comment

[Jeff Kirsher]

From: Bruce Allan <bruce.w.allan@intel.com>

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

Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 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

[net-next 04/13] e1000e: cleanup checkpatch PREFER_PR_LEVEL warning

[Jeff Kirsher]

From: Bruce Allan <bruce.w.allan@intel.com>

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

Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 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

Re: [net-next 04/13] e1000e: cleanup checkpatch PREFER_PR_LEVEL warning

[Joe Perches]

On Thu, 2012-08-23 at 02:56 -0700, Jeff Kirsher wrote:
> From: Bruce Allan <bruce.w.allan@intel.com>
> 
> checkpatch warning: Prefer pr_info(... to printk(KERN_INFO, ...
[]
> diff --git 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" :

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.

Re: [net-next 04/13] e1000e: cleanup checkpatch PREFER_PR_LEVEL warning

[Joe Perches]

On Thu, 2012-08-23 at 07:01 -0700, Joe Perches wrote:
> On Thu, 2012-08-23 at 02:56 -0700, Jeff Kirsher wrote:
> > From: Bruce Allan <bruce.w.allan@intel.com>
> > 
> > checkpatch warning: Prefer pr_info(... to printk(KERN_INFO, ...
> []
> > diff --git 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" :
> 
> 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.

Re: [net-next 04/13] e1000e: cleanup checkpatch PREFER_PR_LEVEL warning

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 2021 bytes --]

On Fri, 2012-08-24 at 11:02 -0700, Joe Perches wrote:
> On Thu, 2012-08-23 at 07:01 -0700, Joe Perches wrote:
> > On Thu, 2012-08-23 at 02:56 -0700, Jeff Kirsher wrote:
> > > From: Bruce Allan <bruce.w.allan@intel.com>
> > > 
> > > checkpatch warning: Prefer pr_info(... to printk(KERN_INFO, ...
> > []
> > > diff --git 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" :
> > 
> > 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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [net-next 04/13] e1000e: cleanup checkpatch PREFER_PR_LEVEL warning

[Joe Perches]

On Fri, 2012-08-24 at 14:19 -0700, Jeff Kirsher wrote:
> On Fri, 2012-08-24 at 11:02 -0700, Joe Perches wrote:
> > On Thu, 2012-08-23 at 07:01 -0700, Joe Perches wrote:
> > > On Thu, 2012-08-23 at 02:56 -0700, Jeff Kirsher wrote:
> > > > From: Bruce Allan <bruce.w.allan@intel.com>
> > > > 
> > > > checkpatch warning: Prefer pr_info(... to printk(KERN_INFO, ...
> > > []
> > > > diff --git 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" :
[]
> > 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...

[net-next 05/13] e1000e: cleanup - remove unnecessary variable

[Jeff Kirsher]

From: Bruce Allan <bruce.w.allan@intel.com>

Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 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

[net-next 06/13] e1000e: update driver version number

[Jeff Kirsher]

From: Bruce Allan <bruce.w.allan@intel.com>

Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 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

[net-next 07/13] e1000e: cleanup strict checkpatch MEMORY_BARRIER checks

[Jeff Kirsher]

From: Bruce Allan <bruce.w.allan@intel.com>

Add comments to memory barriers per strict checkpatch.

Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 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

[net-next 08/13] igb: Add loopback test support for i210.

[Jeff Kirsher]

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

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

[net-next 09/13] igb: reduce Rx header size

[Jeff Kirsher]

From: Eric Dumazet <edumazet@google.com>

Reduce skb truesize by 256 bytes.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Alexander Duyck <alexander.h.duyck@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 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

[net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[Jeff Kirsher]

From: Matthew Vick <matthew.vick@intel.com>

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 <matthew.vick@intel.com>
Acked-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Jeff Pieper  <jeffrey.e.pieper@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 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

Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[Richard Cochran]

On Thu, Aug 23, 2012 at 02:56:50AM -0700, Jeff Kirsher wrote:
> From: Matthew Vick <matthew.vick@intel.com>
> 
> 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.
 
> Signed-off-by: Matthew Vick <matthew.vick@intel.com>
> Acked-by: Jacob Keller <jacob.e.keller@intel.com>
> Tested-by: Jeff Pieper  <jeffrey.e.pieper@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  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 */
> +

This is legitimate, to reduce memory footprint.

>  	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

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

RE: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[Vick, Matthew]

> -----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:
> > From: Matthew Vick <matthew.vick@intel.com>
> >
> > 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.

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

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

Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[Richard Cochran]

On Thu, Aug 23, 2012 at 04:09:30PM +0000, Vick, Matthew wrote:
> > -----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:
> > > From: Matthew Vick <matthew.vick@intel.com>
> > >
> > > 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.
> 
> 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

RE: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[Vick, Matthew]

> -----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:
> > > -----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:
> > > > From: Matthew Vick <matthew.vick@intel.com>
> > > >
> > > > 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.
> >
> > 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

Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[Richard Cochran]

On Thu, Aug 23, 2012 at 05:40:01PM +0000, Vick, Matthew wrote:
> > 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

RE: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[Keller, Jacob E]

> -----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:
> > > -----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:
> > > > From: Matthew Vick <matthew.vick@intel.com>
> > > >
> > > > 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.
> >
> > 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

RE: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[Vick, Matthew]

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

Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[Richard Cochran]

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

Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[Richard Cochran]

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

Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[Ben Hutchings]

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

RE: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[Keller, Jacob E]

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


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

Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[Richard Cochran]

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.

RE: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[Keller, Jacob E]

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

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

Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[Richard Cochran]

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

Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[Richard Cochran]

On Thu, Aug 23, 2012 at 02:56:50AM -0700, Jeff Kirsher wrote:
> From: Matthew Vick <matthew.vick@intel.com>
> 
> 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

RE: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

[Vick, Matthew]

> -----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:
> > From: Matthew Vick <matthew.vick@intel.com>
> >
> > 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

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

[net-next 11/13] igb: Update PTP function names/variables and locations.

[Jeff Kirsher]

From: Matthew Vick <matthew.vick@intel.com>

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 <matthew.vick@intel.com>
Tested-by: Jeff Pieper  <jeffrey.e.pieper@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 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:
@@ -6539,7 +6290,7 @@ static int igb_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
 		return igb_mii_ioctl(netdev, ifr, cmd);
 #ifdef CONFIG_IGB_PTP
 	case SIOCSHWTSTAMP:
-		return igb_hwtstamp_ioctl(netdev, ifr, cmd);
+		return igb_ptp_hwtstamp_ioctl(netdev, ifr, cmd);
 #endif /* CONFIG_IGB_PTP */
 	default:
 		return -EOPNOTSUPP;
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index c846ea9..34e0d69 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -69,22 +69,22 @@
  *   2^40 * 10^-9 /  60  = 18.3 minutes.
  */
 
-#define IGB_OVERFLOW_PERIOD	(HZ * 60 * 9)
-#define INCPERIOD_82576		(1 << E1000_TIMINCA_16NS_SHIFT)
-#define INCVALUE_82576_MASK	((1 << E1000_TIMINCA_16NS_SHIFT) - 1)
-#define INCVALUE_82576		(16 << IGB_82576_TSYNC_SHIFT)
-#define IGB_NBITS_82580		40
+#define IGB_SYSTIM_OVERFLOW_PERIOD	(HZ * 60 * 9)
+#define INCPERIOD_82576			(1 << E1000_TIMINCA_16NS_SHIFT)
+#define INCVALUE_82576_MASK		((1 << E1000_TIMINCA_16NS_SHIFT) - 1)
+#define INCVALUE_82576			(16 << IGB_82576_TSYNC_SHIFT)
+#define IGB_NBITS_82580			40
 
 /*
  * SYSTIM read access for the 82576
  */
 
-static cycle_t igb_82576_systim_read(const struct cyclecounter *cc)
+static cycle_t igb_ptp_read_82576(const struct cyclecounter *cc)
 {
-	u64 val;
-	u32 lo, hi;
 	struct igb_adapter *igb = container_of(cc, struct igb_adapter, cc);
 	struct e1000_hw *hw = &igb->hw;
+	u64 val;
+	u32 lo, hi;
 
 	lo = rd32(E1000_SYSTIML);
 	hi = rd32(E1000_SYSTIMH);
@@ -99,12 +99,12 @@ static cycle_t igb_82576_systim_read(const struct cyclecounter *cc)
  * SYSTIM read access for the 82580
  */
 
-static cycle_t igb_82580_systim_read(const struct cyclecounter *cc)
+static cycle_t igb_ptp_read_82580(const struct cyclecounter *cc)
 {
-	u64 val;
-	u32 lo, hi, jk;
 	struct igb_adapter *igb = container_of(cc, struct igb_adapter, cc);
 	struct e1000_hw *hw = &igb->hw;
+	u64 val;
+	u32 lo, hi, jk;
 
 	/*
 	 * The timestamp latches on lowest register read. For the 82580
@@ -121,17 +121,63 @@ static cycle_t igb_82580_systim_read(const struct cyclecounter *cc)
 	return val;
 }
 
+/**
+ * igb_ptp_systim_to_hwtstamp - convert system time value to hw timestamp
+ * @adapter: board private structure
+ * @hwtstamps: timestamp structure to update
+ * @systim: unsigned 64bit system time value.
+ *
+ * We need to convert the system time value stored in the RX/TXSTMP registers
+ * into a hwtstamp which can be used by the upper level timestamping functions.
+ *
+ * The 'tmreg_lock' spinlock is used to protect the consistency of the
+ * system time value. This is needed because reading the 64 bit time
+ * value involves reading two (or three) 32 bit registers. The first
+ * read latches the value. Ditto for writing.
+ *
+ * In addition, here have extended the system time with an overflow
+ * counter in software.
+ **/
+static void igb_ptp_systim_to_hwtstamp(struct igb_adapter *adapter,
+				       struct skb_shared_hwtstamps *hwtstamps,
+				       u64 systim)
+{
+	unsigned long flags;
+	u64 ns;
+
+	switch (adapter->hw.mac.type) {
+	case e1000_i210:
+	case e1000_i211:
+	case e1000_i350:
+	case e1000_82580:
+	case e1000_82576:
+		break;
+	default:
+		return;
+	}
+
+	spin_lock_irqsave(&adapter->tmreg_lock, flags);
+
+	ns = timecounter_cyc2time(&adapter->tc, systim);
+
+	spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
+
+	memset(hwtstamps, 0, sizeof(*hwtstamps));
+	hwtstamps->hwtstamp = ns_to_ktime(ns);
+}
+
 /*
  * PTP clock operations
  */
 
-static int ptp_82576_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
+static int igb_ptp_adjfreq_82576(struct ptp_clock_info *ptp, s32 ppb)
 {
+	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,
+					       ptp_caps);
+	struct e1000_hw *hw = &igb->hw;
+	int neg_adj = 0;
 	u64 rate;
 	u32 incvalue;
-	int neg_adj = 0;
-	struct igb_adapter *igb = container_of(ptp, struct igb_adapter, caps);
-	struct e1000_hw *hw = &igb->hw;
 
 	if (ppb < 0) {
 		neg_adj = 1;
@@ -153,13 +199,14 @@ static int ptp_82576_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
 	return 0;
 }
 
-static int ptp_82580_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
+static int igb_ptp_adjfreq_82580(struct ptp_clock_info *ptp, s32 ppb)
 {
+	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,
+					       ptp_caps);
+	struct e1000_hw *hw = &igb->hw;
+	int neg_adj = 0;
 	u64 rate;
 	u32 inca;
-	int neg_adj = 0;
-	struct igb_adapter *igb = container_of(ptp, struct igb_adapter, caps);
-	struct e1000_hw *hw = &igb->hw;
 
 	if (ppb < 0) {
 		neg_adj = 1;
@@ -178,11 +225,12 @@ static int ptp_82580_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
 	return 0;
 }
 
-static int igb_adjtime(struct ptp_clock_info *ptp, s64 delta)
+static int igb_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 {
-	s64 now;
+	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,
+					       ptp_caps);
 	unsigned long flags;
-	struct igb_adapter *igb = container_of(ptp, struct igb_adapter, caps);
+	s64 now;
 
 	spin_lock_irqsave(&igb->tmreg_lock, flags);
 
@@ -195,12 +243,13 @@ static int igb_adjtime(struct ptp_clock_info *ptp, s64 delta)
 	return 0;
 }
 
-static int igb_gettime(struct ptp_clock_info *ptp, struct timespec *ts)
+static int igb_ptp_gettime(struct ptp_clock_info *ptp, struct timespec *ts)
 {
+	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,
+					       ptp_caps);
+	unsigned long flags;
 	u64 ns;
 	u32 remainder;
-	unsigned long flags;
-	struct igb_adapter *igb = container_of(ptp, struct igb_adapter, caps);
 
 	spin_lock_irqsave(&igb->tmreg_lock, flags);
 
@@ -214,11 +263,13 @@ static int igb_gettime(struct ptp_clock_info *ptp, struct timespec *ts)
 	return 0;
 }
 
-static int igb_settime(struct ptp_clock_info *ptp, const struct timespec *ts)
+static int igb_ptp_settime(struct ptp_clock_info *ptp,
+			   const struct timespec *ts)
 {
-	u64 ns;
+	struct igb_adapter *igb = container_of(ptp, struct igb_adapter,
+					       ptp_caps);
 	unsigned long flags;
-	struct igb_adapter *igb = container_of(ptp, struct igb_adapter, caps);
+	u64 ns;
 
 	ns = ts->tv_sec * 1000000000ULL;
 	ns += ts->tv_nsec;
@@ -232,29 +283,265 @@ static int igb_settime(struct ptp_clock_info *ptp, const struct timespec *ts)
 	return 0;
 }
 
-static int ptp_82576_enable(struct ptp_clock_info *ptp,
-			    struct ptp_clock_request *rq, int on)
+static int igb_ptp_enable(struct ptp_clock_info *ptp,
+			  struct ptp_clock_request *rq, int on)
 {
 	return -EOPNOTSUPP;
 }
 
-static int ptp_82580_enable(struct ptp_clock_info *ptp,
-			    struct ptp_clock_request *rq, int on)
+static void igb_ptp_overflow_check(struct work_struct *work)
 {
-	return -EOPNOTSUPP;
+	struct igb_adapter *igb =
+		container_of(work, struct igb_adapter, ptp_overflow_work.work);
+	struct timespec ts;
+
+	igb_ptp_gettime(&igb->ptp_caps, &ts);
+
+	pr_debug("igb overflow check at %ld.%09lu\n", ts.tv_sec, ts.tv_nsec);
+
+	schedule_delayed_work(&igb->ptp_overflow_work,
+			      IGB_SYSTIM_OVERFLOW_PERIOD);
 }
 
-static void igb_overflow_check(struct work_struct *work)
+/**
+ * igb_ptp_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.
+ */
+void igb_ptp_tx_hwtstamp(struct igb_q_vector *q_vector,
+			 struct igb_tx_buffer *buffer_info)
 {
-	struct timespec ts;
-	struct igb_adapter *igb =
-		container_of(work, struct igb_adapter, overflow_work.work);
+	struct igb_adapter *adapter = q_vector->adapter;
+	struct e1000_hw *hw = &adapter->hw;
+	struct skb_shared_hwtstamps shhwtstamps;
+	u64 regval;
 
-	igb_gettime(&igb->caps, &ts);
+	/* 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;
 
-	pr_debug("igb overflow check at %ld.%09lu\n", ts.tv_sec, ts.tv_nsec);
+	regval = rd32(E1000_TXSTMPL);
+	regval |= (u64)rd32(E1000_TXSTMPH) << 32;
 
-	schedule_delayed_work(&igb->overflow_work, IGB_OVERFLOW_PERIOD);
+	igb_ptp_systim_to_hwtstamp(adapter, &shhwtstamps, regval);
+	skb_tstamp_tx(buffer_info->skb, &shhwtstamps);
+}
+
+void igb_ptp_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_ptp_systim_to_hwtstamp(adapter, skb_hwtstamps(skb), regval);
+}
+
+/**
+ * igb_ptp_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".
+ *
+ **/
+int igb_ptp_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();
+
+	/* 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;
 }
 
 void igb_ptp_init(struct igb_adapter *adapter)
@@ -266,39 +553,39 @@ void igb_ptp_init(struct igb_adapter *adapter)
 	case e1000_i211:
 	case e1000_i350:
 	case e1000_82580:
-		adapter->caps.owner	= THIS_MODULE;
-		strcpy(adapter->caps.name, "igb-82580");
-		adapter->caps.max_adj	= 62499999;
-		adapter->caps.n_ext_ts	= 0;
-		adapter->caps.pps	= 0;
-		adapter->caps.adjfreq	= ptp_82580_adjfreq;
-		adapter->caps.adjtime	= igb_adjtime;
-		adapter->caps.gettime	= igb_gettime;
-		adapter->caps.settime	= igb_settime;
-		adapter->caps.enable	= ptp_82580_enable;
-		adapter->cc.read	= igb_82580_systim_read;
-		adapter->cc.mask	= CLOCKSOURCE_MASK(IGB_NBITS_82580);
-		adapter->cc.mult	= 1;
-		adapter->cc.shift	= 0;
+		adapter->ptp_caps.owner = THIS_MODULE;
+		strcpy(adapter->ptp_caps.name, "igb-82580");
+		adapter->ptp_caps.max_adj = 62499999;
+		adapter->ptp_caps.n_ext_ts = 0;
+		adapter->ptp_caps.pps = 0;
+		adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82580;
+		adapter->ptp_caps.adjtime = igb_ptp_adjtime;
+		adapter->ptp_caps.gettime = igb_ptp_gettime;
+		adapter->ptp_caps.settime = igb_ptp_settime;
+		adapter->ptp_caps.enable = igb_ptp_enable;
+		adapter->cc.read = igb_ptp_read_82580;
+		adapter->cc.mask = CLOCKSOURCE_MASK(IGB_NBITS_82580);
+		adapter->cc.mult = 1;
+		adapter->cc.shift = 0;
 		/* Enable the timer functions by clearing bit 31. */
 		wr32(E1000_TSAUXC, 0x0);
 		break;
 
 	case e1000_82576:
-		adapter->caps.owner	= THIS_MODULE;
-		strcpy(adapter->caps.name, "igb-82576");
-		adapter->caps.max_adj	= 1000000000;
-		adapter->caps.n_ext_ts	= 0;
-		adapter->caps.pps	= 0;
-		adapter->caps.adjfreq	= ptp_82576_adjfreq;
-		adapter->caps.adjtime	= igb_adjtime;
-		adapter->caps.gettime	= igb_gettime;
-		adapter->caps.settime	= igb_settime;
-		adapter->caps.enable	= ptp_82576_enable;
-		adapter->cc.read	= igb_82576_systim_read;
-		adapter->cc.mask	= CLOCKSOURCE_MASK(64);
-		adapter->cc.mult	= 1;
-		adapter->cc.shift	= IGB_82576_TSYNC_SHIFT;
+		adapter->ptp_caps.owner = THIS_MODULE;
+		strcpy(adapter->ptp_caps.name, "igb-82576");
+		adapter->ptp_caps.max_adj = 1000000000;
+		adapter->ptp_caps.n_ext_ts = 0;
+		adapter->ptp_caps.pps = 0;
+		adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82576;
+		adapter->ptp_caps.adjtime = igb_ptp_adjtime;
+		adapter->ptp_caps.gettime = igb_ptp_gettime;
+		adapter->ptp_caps.settime = igb_ptp_settime;
+		adapter->ptp_caps.enable = igb_ptp_enable;
+		adapter->cc.read = igb_ptp_read_82576;
+		adapter->cc.mask = CLOCKSOURCE_MASK(64);
+		adapter->cc.mult = 1;
+		adapter->cc.shift = IGB_82576_TSYNC_SHIFT;
 		/* Dial the nominal frequency. */
 		wr32(E1000_TIMINCA, INCPERIOD_82576 | INCVALUE_82576);
 		break;
@@ -313,13 +600,14 @@ void igb_ptp_init(struct igb_adapter *adapter)
 	timecounter_init(&adapter->tc, &adapter->cc,
 			 ktime_to_ns(ktime_get_real()));
 
-	INIT_DELAYED_WORK(&adapter->overflow_work, igb_overflow_check);
+	INIT_DELAYED_WORK(&adapter->ptp_overflow_work, igb_ptp_overflow_check);
 
 	spin_lock_init(&adapter->tmreg_lock);
 
-	schedule_delayed_work(&adapter->overflow_work, IGB_OVERFLOW_PERIOD);
+	schedule_delayed_work(&adapter->ptp_overflow_work,
+			      IGB_SYSTIM_OVERFLOW_PERIOD);
 
-	adapter->ptp_clock = ptp_clock_register(&adapter->caps);
+	adapter->ptp_clock = ptp_clock_register(&adapter->ptp_caps);
 	if (IS_ERR(adapter->ptp_clock)) {
 		adapter->ptp_clock = NULL;
 		dev_err(&adapter->pdev->dev, "ptp_clock_register failed\n");
@@ -328,7 +616,13 @@ void igb_ptp_init(struct igb_adapter *adapter)
 			 adapter->netdev->name);
 }
 
-void igb_ptp_remove(struct igb_adapter *adapter)
+/**
+ * igb_ptp_stop - Disable PTP device and stop the overflow check.
+ * @adapter: Board private structure.
+ *
+ * This function stops the PTP support and cancels the delayed work.
+ **/
+void igb_ptp_stop(struct igb_adapter *adapter)
 {
 	switch (adapter->hw.mac.type) {
 	case e1000_i211:
@@ -336,7 +630,7 @@ void igb_ptp_remove(struct igb_adapter *adapter)
 	case e1000_i350:
 	case e1000_82580:
 	case e1000_82576:
-		cancel_delayed_work_sync(&adapter->overflow_work);
+		cancel_delayed_work_sync(&adapter->ptp_overflow_work);
 		break;
 	default:
 		return;
@@ -348,48 +642,3 @@ void igb_ptp_remove(struct igb_adapter *adapter)
 			 adapter->netdev->name);
 	}
 }
-
-/**
- * igb_systim_to_hwtstamp - convert system time value to hw timestamp
- * @adapter: board private structure
- * @hwtstamps: timestamp structure to update
- * @systim: unsigned 64bit system time value.
- *
- * We need to convert the system time value stored in the RX/TXSTMP registers
- * into a hwtstamp which can be used by the upper level timestamping functions.
- *
- * The 'tmreg_lock' spinlock is used to protect the consistency of the
- * system time value. This is needed because reading the 64 bit time
- * value involves reading two (or three) 32 bit registers. The first
- * read latches the value. Ditto for writing.
- *
- * In addition, here have extended the system time with an overflow
- * counter in software.
- **/
-void igb_systim_to_hwtstamp(struct igb_adapter *adapter,
-			    struct skb_shared_hwtstamps *hwtstamps,
-			    u64 systim)
-{
-	u64 ns;
-	unsigned long flags;
-
-	switch (adapter->hw.mac.type) {
-	case e1000_i210:
-	case e1000_i211:
-	case e1000_i350:
-	case e1000_82580:
-	case e1000_82576:
-		break;
-	default:
-		return;
-	}
-
-	spin_lock_irqsave(&adapter->tmreg_lock, flags);
-
-	ns = timecounter_cyc2time(&adapter->tc, systim);
-
-	spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
-
-	memset(hwtstamps, 0, sizeof(*hwtstamps));
-	hwtstamps->hwtstamp = ns_to_ktime(ns);
-}
-- 
1.7.11.4

Re: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Richard Cochran]

On Thu, Aug 23, 2012 at 02:56:51AM -0700, Jeff Kirsher wrote:
> From: Matthew Vick <matthew.vick@intel.com>
> 
> 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.

Function renaming as an end in itself? Sounds like churn to me. Also,
I disagree with some of the displacements.
 
> 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);
> -}
> -

Why have you moved this block? How are these "PTP-related"?

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

I like the old name better.

> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c

...
  
> -#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 */
> -

Here you have taken a static local function and made it into a global
function. This can have performance impacts.

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

This name stinks, too. You know that you can have time stamping all by
itself, right? It is logically separate from the ptp clock stuff.

This patch doesn't really improve the driver at all, IMHO.

Thanks,
Richard

RE: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Vick, Matthew]

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Thursday, August 23, 2012 4:16 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 11/13] igb: Update PTP function names/variables
> and locations.
> 
> On Thu, Aug 23, 2012 at 02:56:51AM -0700, Jeff Kirsher wrote:
> > From: Matthew Vick <matthew.vick@intel.com>
> >
> > 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.
> 
> Function renaming as an end in itself? Sounds like churn to me. Also, I
> disagree with some of the displacements.

The goal was to make it clear what was required for the PTP flow and what wasn't. Again, ixgbe was my reference in this. Personally, I disagree with the old naming scheme.

> > 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);
> > -}
> > -
> 
> Why have you moved this block? How are these "PTP-related"?

This is just git's diff being silly. :) Rather than move igb_get_ts_info up, git thought I moved those other functions down. I wanted igb_get_ts_info to be with the other get functions.

> >  #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,
> 
> I like the old name better.

The old name is out of the coding style of igb. Every other function is igb_get_* or igb_set_*, with the exception of igb_ethtool_begin and igb_ethtool_complete. Are you suggesting we add _ethtool to every ethtool function in igb? 

> > -#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 */
> > -
> 
> Here you have taken a static local function and made it into a global
> function. This can have performance impacts.

Which, this function calls igb_systim_to_hwtstamp anyway, which is global. Also, in a follow-on patch I have coming, igb_ptp_tx_hwtstamp won't even be called in clean_tx_irq, FWIW.

> >  /**
> >   * 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);
> 
> This name stinks, too. You know that you can have time stamping all by
> itself, right? It is logically separate from the ptp clock stuff.
> 
> This patch doesn't really improve the driver at all, IMHO.
> 
> Thanks,
> Richard

Yes, I'm aware. But, as it stands today, we don't use it for anything else. If the function is feature specific, then we should be calling it out as such.

I'm sorry you feel like this patch doesn't improve the driver. The goal is code cleanup and consistency, both of which I consider to be driver improvements and is why I made the patches. 

Cheers,
Matthew

Re: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Richard Cochran]

On Thu, Aug 23, 2012 at 04:22:02PM +0000, Vick, Matthew wrote:

> > >  #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,
> > 
> > I like the old name better.
> 
> The old name is out of the coding style of igb. Every other function is igb_get_* or igb_set_*, with the exception of igb_ethtool_begin and igb_ethtool_complete. Are you suggesting we add _ethtool to every ethtool function in igb? 

No, just leave the names alone, and keep the functions where they
are. It is just churn.

One of the most useful ways to understand code (at least for me) is to
use git blame. It tells you when code was added, what the reason was,
and how the change looks in context. By moving and renaming willy
nilly, you are obscuring this valuable information.

> > > -#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 */
> > > -
> > 
> > Here you have taken a static local function and made it into a global
> > function. This can have performance impacts.
> 
> Which, this function calls igb_systim_to_hwtstamp anyway, which is
> global.

So how does calling two global functions in series improve performance?

> Also, in a follow-on patch I have coming, igb_ptp_tx_hwtstamp won't
> even be called in clean_tx_irq, FWIW.

If this is part of some larger plan, then it would help to see that
plan.

> > >  /**
> > >   * 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);
> > 
> > This name stinks, too. You know that you can have time stamping all by
> > itself, right? It is logically separate from the ptp clock stuff.
> > 
> > This patch doesn't really improve the driver at all, IMHO.
> > 
> > Thanks,
> > Richard
> 
> Yes, I'm aware. But, as it stands today, we don't use it for anything else. If the function is feature specific, then we should be calling it out as such.

Right now the time stamping is being equated with the clock functions,
but it really should be decoupled. The 82580 can time stamp every
received packet, which can be interesting for performance monitoring,
even without PTP (and adding *that* would be a useful change).

> I'm sorry you feel like this patch doesn't improve the driver. The goal is code cleanup and consistency, both of which I consider to be driver improvements and is why I made the patches. 

But the code wasn't dirty in the first place. It doesn't need this
"cleaning." This series undoes the inline-able functions for no good
reason. As far as ixgbe goes, this driver came first, so you might as
well be making *that* driver consistent with this one.

Thanks,
Richard
 

RE: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Keller, Jacob E]

> -----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:54 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 11/13] igb: Update PTP function names/variables and
> locations.
> 
> On Thu, Aug 23, 2012 at 04:22:02PM +0000, Vick, Matthew wrote:
> 
> > > >  #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,
> > >
> > > I like the old name better.
> >
> > The old name is out of the coding style of igb. Every other function is
> igb_get_* or igb_set_*, with the exception of igb_ethtool_begin and
> igb_ethtool_complete. Are you suggesting we add _ethtool to every ethtool
> function in igb?
> 
> No, just leave the names alone, and keep the functions where they are. It
> is just churn.
> 
> One of the most useful ways to understand code (at least for me) is to use
> git blame. It tells you when code was added, what the reason was, and how
> the change looks in context. By moving and renaming willy nilly, you are
> obscuring this valuable information.
> 
> > > > -#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 */
> > > > -
> > >
> > > Here you have taken a static local function and made it into a
> > > global function. This can have performance impacts.
> >
> > Which, this function calls igb_systim_to_hwtstamp anyway, which is
> > global.
> 
> So how does calling two global functions in series improve performance?
> 
> > Also, in a follow-on patch I have coming, igb_ptp_tx_hwtstamp won't
> > even be called in clean_tx_irq, FWIW.
> 
> If this is part of some larger plan, then it would help to see that plan.
> 
> > > >  /**
> > > >   * 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);
> > >
> > > This name stinks, too. You know that you can have time stamping all
> > > by itself, right? It is logically separate from the ptp clock stuff.
> > >
> > > This patch doesn't really improve the driver at all, IMHO.
> > >
> > > Thanks,
> > > Richard
> >
> > Yes, I'm aware. But, as it stands today, we don't use it for anything
> else. If the function is feature specific, then we should be calling it
> out as such.
> 
> Right now the time stamping is being equated with the clock functions, but
> it really should be decoupled. The 82580 can time stamp every received
> packet, which can be interesting for performance monitoring, even without
> PTP (and adding *that* would be a useful change).
> 
The timestamp all does not really work with the ptp clock features gone, because you don't have the clock. You can't equate the time values of the packets when the clock isn't synched to something meaningful. Yes that does not require PTP adjustment functions, but it does require the SYSTIME setup and some method to get the clock correct, which currently is only done in the PTP init sequence. Timestamp all packets also can cause a performance hit when used with certain workloads.

> > I'm sorry you feel like this patch doesn't improve the driver. The goal
> is code cleanup and consistency, both of which I consider to be driver
> improvements and is why I made the patches.
> 
> But the code wasn't dirty in the first place. It doesn't need this
> "cleaning." This series undoes the inline-able functions for no good
> reason. As far as ixgbe goes, this driver came first, so you might as well
> be making *that* driver consistent with this one.

ixgbe hardware is (currently) even more closely synched with PTP for the register bits so it does make some sense for ixgbe to remain the way it is. Right now the igb features are partially synched (even before this change) in odd ways. The time values returned when PHC information is disabled are basically only useful for comparing between themselves, not with any meaningful clock on the device.

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

Re: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Richard Cochran]

On Thu, Aug 23, 2012 at 06:00:37PM +0000, Keller, Jacob E wrote:
> > 
> > Right now the time stamping is being equated with the clock functions, but
> > it really should be decoupled. The 82580 can time stamp every received
> > packet, which can be interesting for performance monitoring, even without
> > PTP (and adding *that* would be a useful change).
> > 
> The timestamp all does not really work with the ptp clock features
> gone, because you don't have the clock. You can't equate the time
> values of the packets when the clock isn't synched to something
> meaningful. Yes that does not require PTP adjustment functions, but
> it does require the SYSTIME setup and some method to get the clock
> correct, which currently is only done in the PTP init sequence.

Relative, high resolution time stamps can be interesting all by
themselves. That is why wireshark has a whole menu of timing choices
including relative since start, inter-packet, and so on.

> Timestamp all packets also can cause a performance hit when used with certain workloads.

Only when enabled.

> ixgbe hardware is (currently) even more closely synched with PTP for the register bits so it does make some sense for ixgbe to remain the way it is. Right now the igb features are partially synched (even before this change) in odd ways. The time values returned when PHC information is disabled are basically only useful for comparing between themselves, not with any meaningful clock on the device.

Yes, I agree that igb is a bit oddly synced WRT clock and time
stamping. I would welcome a change to let it have HW time stamping as
an independent feature.

Thanks,
Richard

RE: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Vick, Matthew]

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Thursday, August 23, 2012 11:11 AM
> To: Keller, Jacob E
> Cc: Vick, Matthew; Kirsher, Jeffrey T; davem@davemloft.net;
> netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com
> Subject: Re: [net-next 11/13] igb: Update PTP function names/variables
> and locations.
> 
> [...]
> 
> Relative, high resolution time stamps can be interesting all by
> themselves. That is why wireshark has a whole menu of timing choices
> including relative since start, inter-packet, and so on.
> 
> [...]
> 
> Yes, I agree that igb is a bit oddly synced WRT clock and time
> stamping. I would welcome a change to let it have HW time stamping as
> an independent feature.
> 
> Thanks,
> Richard

Isn't this discussion creeping outside the scope of this patch? My aim here with this patch is to help tidy up PTP code as it is today. As it is today, igb has no support for hardware timestamping without PTP. If you or someone else writes a follow-on patch to add hardware timestamping without PTP, then they can move and rename the functions accordingly (and I'd personally really appreciate it if they did rename it, since a generic function should be named like a generic function and a PTP function should be named like a PTP function).

Cheers,
Matthew 

Re: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Richard Cochran]

On Thu, Aug 23, 2012 at 06:44:24PM +0000, Vick, Matthew wrote:
> 
> Isn't this discussion creeping outside the scope of this patch?

Yes, it is. I am suggesting that the driver could use improvement, but
not by renaming functions to suit your or someone else's taste.
Instead, you could work on the time stamping feature.

(And if, along the way, you end up renaming the functions that you
change, then I won't complain ;)

Thanks,
Richard

RE: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Vick, Matthew]

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Thursday, August 23, 2012 11:56 PM
> To: Vick, Matthew
> Cc: Keller, Jacob E; Kirsher, Jeffrey T; davem@davemloft.net;
> netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com
> Subject: Re: [net-next 11/13] igb: Update PTP function names/variables
> and locations.
> 
> On Thu, Aug 23, 2012 at 06:44:24PM +0000, Vick, Matthew wrote:
> >
> > Isn't this discussion creeping outside the scope of this patch?
> 
> Yes, it is. I am suggesting that the driver could use improvement, but
> not by renaming functions to suit your or someone else's taste.
> Instead, you could work on the time stamping feature.
> 
> (And if, along the way, you end up renaming the functions that you
> change, then I won't complain ;)
> 
> Thanks,
> Richard

Looking over the discussions and by trying to reach consensus, I think what I will do is re-spin patch 3 that Jeff sent (Correct PTP support query from ethtool.) to V2 with your feedback and work with Jeff to send all of my patchset up together. This first group of patches is groundwork for the last two. How does this sound?

Cheers,
Matthew

Re: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Richard Cochran]

On Fri, Aug 24, 2012 at 03:52:30PM +0000, Vick, Matthew wrote:
> 
> Looking over the discussions and by trying to reach consensus, I think what I will do is re-spin patch 3 that Jeff sent (Correct PTP support query from ethtool.) to V2 with your feedback and work with Jeff to send all of my patchset up together. This first group of patches is groundwork for the last two. How does this sound?

Sound good to me.

BTW, if you have some bug fixes coming (like ifup/ifdown), please put
them *before* any other renaming/refactoring. In that way, the bug fix
can do directly to 3.5 stable, too.

Thanks,
Richard

RE: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Vick, Matthew]

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Friday, August 24, 2012 10:49 PM
> To: Vick, Matthew
> Cc: Keller, Jacob E; Kirsher, Jeffrey T; davem@davemloft.net;
> netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com
> Subject: Re: [net-next 11/13] igb: Update PTP function names/variables
> and locations.
> 
> On Fri, Aug 24, 2012 at 03:52:30PM +0000, Vick, Matthew wrote:
> >
> > Looking over the discussions and by trying to reach consensus, I
> think what I will do is re-spin patch 3 that Jeff sent (Correct PTP
> support query from ethtool.) to V2 with your feedback and work with
> Jeff to send all of my patchset up together. This first group of
> patches is groundwork for the last two. How does this sound?
> 
> Sound good to me.
> 
> BTW, if you have some bug fixes coming (like ifup/ifdown), please put
> them *before* any other renaming/refactoring. In that way, the bug fix
> can do directly to 3.5 stable, too.
> 
> Thanks,
> Richard

Unfortunately, the way I've built the series makes it difficult to put those fixes before the re-factoring. The need for the design change drove the re-factor to give me the foundation. I'll make a note to myself to come up with something to apply to stable.

Cheers,
Matthew 

Re: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Richard Cochran]

> 
> Unfortunately, the way I've built the series makes it difficult to put those fixes before the re-factoring. The need for the design change drove the re-factor to give me the foundation. I'll make a note to myself to come up with something to apply to stable.

Hm, I didn't see anything here that looks like bug fix. Did I miss
something?  What causes the ifup/ifdown weirdness, anyhow?

(It seems hard to believe that it has something to do with the names
or locations of the various functions.)

Thanks,
Richard

RE: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Vick, Matthew]

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Monday, August 27, 2012 9:44 AM
> To: Vick, Matthew
> Cc: Keller, Jacob E; Kirsher, Jeffrey T; davem@davemloft.net;
> netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com
> Subject: Re: [net-next 11/13] igb: Update PTP function names/variables
> and locations.
> 
> >
> > Unfortunately, the way I've built the series makes it difficult to
> put those fixes before the re-factoring. The need for the design change
> drove the re-factor to give me the foundation. I'll make a note to
> myself to come up with something to apply to stable.
> 
> Hm, I didn't see anything here that looks like bug fix. Did I miss
> something?  What causes the ifup/ifdown weirdness, anyhow?
> 
> (It seems hard to believe that it has something to do with the names or
> locations of the various functions.)
> 
> Thanks,
> Richard

Sorry, I wasn't very clear--I mean the bug fix I have for ifup/ifdown is in the next few patches with the implementation change I'm doing, since the code I have in place to fix it applies only to the new implementation. I'll have to make a separate patch for stable to address it there.

If the ifup/ifdown weirdness happens because we never re-enable timestamping in the hardware following reset and the device goes through a reset during an ifdown/ifup cycle. It's a pretty straightforward fix, so it shouldn't be much effort to spin up something for stable.

Matthew

RE: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Vick, Matthew]

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Thursday, August 23, 2012 10:54 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 11/13] igb: Update PTP function names/variables
> and locations.
> 
> On Thu, Aug 23, 2012 at 04:22:02PM +0000, Vick, Matthew wrote:
> 
> > > >  #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,
> > >
> > > I like the old name better.
> >
> > The old name is out of the coding style of igb. Every other function
> is igb_get_* or igb_set_*, with the exception of igb_ethtool_begin and
> igb_ethtool_complete. Are you suggesting we add _ethtool to every
> ethtool function in igb?
> 
> No, just leave the names alone, and keep the functions where they are.
> It is just churn.
> 
> One of the most useful ways to understand code (at least for me) is to
> use git blame. It tells you when code was added, what the reason was,
> and how the change looks in context. By moving and renaming willy
> nilly, you are obscuring this valuable information.

Repairing is not churn. I think it's better to make the code clearer than require developers use git blame to figure out who named a function in a silly way. I agree, it kind of stinks to lose that history, but I'd rather not skip making the right change because of git blame concerns.

> 
> [...]
> 
> >
> > Which, this function calls igb_systim_to_hwtstamp anyway, which is
> > global.
> 
> So how does calling two global functions in series improve performance?

It isn't calling two global functions. It's calling a global function that calls a static function.

> > Also, in a follow-on patch I have coming, igb_ptp_tx_hwtstamp won't
> > even be called in clean_tx_irq, FWIW.
> 
> If this is part of some larger plan, then it would help to see that
> plan.

Definitely fair enough. I will work with Jeff to try and push the whole series next time (at the very least, I'd like to re-spin the series for the ethtool query patch).

> > > >  /**
> > > >   * 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);
> > >
> > > This name stinks, too. You know that you can have time stamping all
> > > by itself, right? It is logically separate from the ptp clock
> stuff.
> > >
> > > This patch doesn't really improve the driver at all, IMHO.
> > >
> > > Thanks,
> > > Richard
> >
> > Yes, I'm aware. But, as it stands today, we don't use it for anything
> else. If the function is feature specific, then we should be calling it
> out as such.
> 
> Right now the time stamping is being equated with the clock functions,
> but it really should be decoupled. The 82580 can time stamp every
> received packet, which can be interesting for performance monitoring,
> even without PTP (and adding *that* would be a useful change).

As Jake said, there's no support for hardware timestamping without PTP today. As such, if the function is only useful for that feature, then I think it's less ambiguous to leave it the way I've coded it today. If we de-couple hardware timestamping and clock synchronization, which I wouldn't be opposed to, then the function name can reflect becoming more generic. As it stands, I'd rather not call something generic when it isn't. I think it's misleading, personally.

> > I'm sorry you feel like this patch doesn't improve the driver. The
> goal is code cleanup and consistency, both of which I consider to be
> driver improvements and is why I made the patches.
> 
> But the code wasn't dirty in the first place. It doesn't need this
> "cleaning." This series undoes the inline-able functions for no good
> reason. As far as ixgbe goes, this driver came first, so you might as
> well be making *that* driver consistent with this one.
> 
> Thanks,
> Richard

Actually, yes, the code *was* dirty, because it breaks the coding style of the rest of the driver and isn't as self-documenting as it could be, and the inline-able functions you claim I'm destroying called global functions anyway. Also, just because igb came first doesn't mean we should ignore porting better or clearer solutions from later drivers. If something is better or clearer, I think we should use be using it.

Cheers,
Matthew

RE: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Keller, Jacob E]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Vick, Matthew
> Sent: Thursday, August 23, 2012 11:35 AM
> To: Richard Cochran
> Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
> gospo@redhat.com; sassmann@redhat.com
> Subject: RE: [net-next 11/13] igb: Update PTP function names/variables and
> locations.
> 
> > -----Original Message-----
> > From: Richard Cochran [mailto:richardcochran@gmail.com]
> > Sent: Thursday, August 23, 2012 10:54 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 11/13] igb: Update PTP function names/variables
> > and locations.
> >
> > On Thu, Aug 23, 2012 at 04:22:02PM +0000, Vick, Matthew wrote:
> >
> > > > >  #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,
> > > >
> > > > I like the old name better.
> > >
> > > The old name is out of the coding style of igb. Every other function
> > is igb_get_* or igb_set_*, with the exception of igb_ethtool_begin and
> > igb_ethtool_complete. Are you suggesting we add _ethtool to every
> > ethtool function in igb?
> >
> > No, just leave the names alone, and keep the functions where they are.
> > It is just churn.
> >
> > One of the most useful ways to understand code (at least for me) is to
> > use git blame. It tells you when code was added, what the reason was,
> > and how the change looks in context. By moving and renaming willy
> > nilly, you are obscuring this valuable information.
> 
> Repairing is not churn. I think it's better to make the code clearer than
> require developers use git blame to figure out who named a function in a
> silly way. I agree, it kind of stinks to lose that history, but I'd rather
> not skip making the right change because of git blame concerns.

The history isn't lost, it is just obscured. I agree if the change is necessary it should be done. I think we all disagree on what is necessary. I would prefer function names to match. Richard has a point regarding the time stamping all packets, however again the ioctl turning that on tends to be quite PTP centric already. I think that value isn't necessarily added due to the current coupling of the features. It is partially coupled by the hardware anyways.

Effectively with the PTP framework disabled you have a half useful feature that is enabled by a strange ioctl that has a lot of PTP specific names, and doesn't always do what you want.

I am not sure how much work would be required to get to a state where the separation of function makes sense.

- Jake

> 
> >
> > [...]
> >
> > >
> > > Which, this function calls igb_systim_to_hwtstamp anyway, which is
> > > global.
> >
> > So how does calling two global functions in series improve performance?
> 
> It isn't calling two global functions. It's calling a global function that
> calls a static function.
> 
> > > Also, in a follow-on patch I have coming, igb_ptp_tx_hwtstamp won't
> > > even be called in clean_tx_irq, FWIW.
> >
> > If this is part of some larger plan, then it would help to see that
> > plan.
> 
> Definitely fair enough. I will work with Jeff to try and push the whole
> series next time (at the very least, I'd like to re-spin the series for
> the ethtool query patch).
> 
> > > > >  /**
> > > > >   * 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);
> > > >
> > > > This name stinks, too. You know that you can have time stamping
> > > > all by itself, right? It is logically separate from the ptp clock
> > stuff.
> > > >
> > > > This patch doesn't really improve the driver at all, IMHO.
> > > >
> > > > Thanks,
> > > > Richard
> > >
> > > Yes, I'm aware. But, as it stands today, we don't use it for
> > > anything
> > else. If the function is feature specific, then we should be calling
> > it out as such.
> >
> > Right now the time stamping is being equated with the clock functions,
> > but it really should be decoupled. The 82580 can time stamp every
> > received packet, which can be interesting for performance monitoring,
> > even without PTP (and adding *that* would be a useful change).
> 
> As Jake said, there's no support for hardware timestamping without PTP
> today. As such, if the function is only useful for that feature, then I
> think it's less ambiguous to leave it the way I've coded it today. If we
> de-couple hardware timestamping and clock synchronization, which I
> wouldn't be opposed to, then the function name can reflect becoming more
> generic. As it stands, I'd rather not call something generic when it
> isn't. I think it's misleading, personally.
> 
> > > I'm sorry you feel like this patch doesn't improve the driver. The
> > goal is code cleanup and consistency, both of which I consider to be
> > driver improvements and is why I made the patches.
> >
> > But the code wasn't dirty in the first place. It doesn't need this
> > "cleaning." This series undoes the inline-able functions for no good
> > reason. As far as ixgbe goes, this driver came first, so you might as
> > well be making *that* driver consistent with this one.
> >
> > Thanks,
> > Richard
> 
> Actually, yes, the code *was* dirty, because it breaks the coding style of
> the rest of the driver and isn't as self-documenting as it could be, and
> the inline-able functions you claim I'm destroying called global functions
> anyway. Also, just because igb came first doesn't mean we should ignore
> porting better or clearer solutions from later drivers. If something is
> better or clearer, I think we should use be using it.
> 
> Cheers,
> Matthew
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in the
> body of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

RE: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Vick, Matthew]

> -----Original Message-----
> From: Keller, Jacob E
> Sent: Thursday, August 23, 2012 11:48 AM
> To: Vick, Matthew; Richard Cochran
> Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
> gospo@redhat.com; sassmann@redhat.com
> Subject: RE: [net-next 11/13] igb: Update PTP function names/variables
> and locations.
> 
> [...]
> 
> The history isn't lost, it is just obscured. I agree if the change is
> necessary it should be done. I think we all disagree on what is
> necessary. I would prefer function names to match. Richard has a point
> regarding the time stamping all packets, however again the ioctl
> turning that on tends to be quite PTP centric already. I think that
> value isn't necessarily added due to the current coupling of the
> features. It is partially coupled by the hardware anyways.
> 
> Effectively with the PTP framework disabled you have a half useful
> feature that is enabled by a strange ioctl that has a lot of PTP
> specific names, and doesn't always do what you want.
> 
> I am not sure how much work would be required to get to a state where
> the separation of function makes sense.
> 
> - Jake

Fair point about the history.

Since these functions are tightly coupled with PTP, it makes sense to call them that for now. Long-term, I completely agree with Richard--once the function is independent of PTP, we should give it a generic name. 

Cheers,
Matthew

Re: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Joe Perches]

On Thu, 2012-08-23 at 19:53 +0200, Richard Cochran wrote:
> On Thu, Aug 23, 2012 at 04:22:02PM +0000, Vick, Matthew wrote:
> 
> > > >  #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,
> > > 
> > > I like the old name better.
> > 
> > The old name is out of the coding style of igb. Every other
> > function is igb_get_* or igb_set_*, with the exception of
> > igb_ethtool_begin and igb_ethtool_complete.

> No, just leave the names alone, and keep the functions where they
> are. It is just churn.
> 
> One of the most useful ways to understand code (at least for me) is to
> use git blame. It tells you when code was added, what the reason was,
> and how the change looks in context. By moving and renaming willy
> nilly, you are obscuring this valuable information.

[]

> > I'm sorry you feel like this patch doesn't improve the driver.
> > The goal is code cleanup and consistency, both of which I
> > consider to be driver improvements and is why I made the patches. 
> 
> But the code wasn't dirty in the first place. It doesn't need this
> "cleaning." This series undoes the inline-able functions for no good
> reason. As far as ixgbe goes, this driver came first, so you might as
> well be making *that* driver consistent with this one.

I disagree with Richard.

Improving code clarity and consistency isn't churn.

Old code isn't necessarily the best code, nor should
necessarily old code be a required guide for new code.

The most valuable form of code is the current one,
not any antecedent version.

People that need to wade through old crud to blame
someone still can.

Re: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Richard Cochran]

On Thu, Aug 23, 2012 at 08:02:25PM -0700, Joe Perches wrote:

> Improving code clarity and consistency isn't churn.

This patch series moves code around for no good reason. That is, by
definition, churn.

> The most valuable form of code is the current one,
> not any antecedent version.

You really haven't noticed all the code review and rebasing that goes
on around here in order to improve the commit history, have you?

Thanks,
Richard

Re: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Joe Perches]

On Fri, 2012-08-24 at 08:32 +0200, Richard Cochran wrote:
> On Thu, Aug 23, 2012 at 08:02:25PM -0700, Joe Perches wrote:
> 
> > Improving code clarity and consistency isn't churn.
> 
> This patch series moves code around for no good reason. That is, by
> definition, churn.

For your definition of good.

> > The most valuable form of code is the current one,
> > not any antecedent version.
> 
> You really haven't noticed all the code review and rebasing that goes
> on around here in order to improve the commit history, have you?

That'd be incorrect too.

Re: [net-next 11/13] igb: Update PTP function names/variables and locations.

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Fri, 24 Aug 2012 02:22:34 -0700

> On Fri, 2012-08-24 at 08:32 +0200, Richard Cochran wrote:
>> On Thu, Aug 23, 2012 at 08:02:25PM -0700, Joe Perches wrote:
>> 
>> > Improving code clarity and consistency isn't churn.
>> 
>> This patch series moves code around for no good reason. That is, by
>> definition, churn.
> 
> For your definition of good.

I think the people doing all of the actual development and
maintainence of the code get to decide how to define good.  And in
this case that's basically the Intel NIC development team, not you.

Moving functions around is a very valuable and useful cleanup quite
often.

So you can count me in on their definition of "good" as well.

Re: [net-next 11/13] igb: Update PTP function names/variables and locations.

[Joe Perches]

On Fri, 2012-08-24 at 11:12 -0400, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> Date: Fri, 24 Aug 2012 02:22:34 -0700
> > On Fri, 2012-08-24 at 08:32 +0200, Richard Cochran wrote:
> >> On Thu, Aug 23, 2012 at 08:02:25PM -0700, Joe Perches wrote:
> >> > Improving code clarity and consistency isn't churn.
> >> This patch series moves code around for no good reason. That is, by
> >> definition, churn.
> > 
> > For your definition of good.
> 
> I think the people doing all of the actual development and
> maintainence of the code get to decide how to define good.  And in
> this case that's basically the Intel NIC development team, not you.

Just for clarity, you do mean Richard's view and not mine.
I agree with these proposed changes.

> Moving functions around is a very valuable and useful cleanup quite
> often.
> 
> So you can count me in on their definition of "good" as well.

[net-next 12/13] igb: Correct PTP support query from ethtool.

[Jeff Kirsher]

From: Matthew Vick <matthew.vick@intel.com>

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 <matthew.vick@intel.com>
Acked-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Jeff Pieper <jeffrey.e.pieper@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 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

Re: [net-next 12/13] igb: Correct PTP support query from ethtool.

[Richard Cochran]

On Thu, Aug 23, 2012 at 02:56:52AM -0700, Jeff Kirsher wrote:
> From: Matthew Vick <matthew.vick@intel.com>
> 
> 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.

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

...

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

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

RE: [net-next 12/13] igb: Correct PTP support query from ethtool.

[Vick, Matthew]

> -----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:
> > From: Matthew Vick <matthew.vick@intel.com>
> >
> > 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.
> 
> > diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> 
> ...
> 
> > +#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

[net-next 13/13] igb: Store the MAC address in the name in the PTP struct.

[Jeff Kirsher]

From: Matthew Vick <matthew.vick@intel.com>

Change the name of the adapter in the PTP struct to enable easier
correlation between interface and PTP device.

Signed-off-by: Matthew Vick <matthew.vick@intel.com>
Acked-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by:  Jeff Pieper <jeffrey.e.pieper@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_ptp.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 34e0d69..e69555f 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -547,14 +547,15 @@ int igb_ptp_hwtstamp_ioctl(struct net_device *netdev,
 void igb_ptp_init(struct igb_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
+	struct net_device *netdev = adapter->netdev;
 
 	switch (hw->mac.type) {
 	case e1000_i210:
 	case e1000_i211:
 	case e1000_i350:
 	case e1000_82580:
+		snprintf(adapter->ptp_caps.name, 16, "%pm", netdev->dev_addr);
 		adapter->ptp_caps.owner = THIS_MODULE;
-		strcpy(adapter->ptp_caps.name, "igb-82580");
 		adapter->ptp_caps.max_adj = 62499999;
 		adapter->ptp_caps.n_ext_ts = 0;
 		adapter->ptp_caps.pps = 0;
@@ -570,10 +571,9 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		/* Enable the timer functions by clearing bit 31. */
 		wr32(E1000_TSAUXC, 0x0);
 		break;
-
 	case e1000_82576:
+		snprintf(adapter->ptp_caps.name, 16, "%pm", netdev->dev_addr);
 		adapter->ptp_caps.owner = THIS_MODULE;
-		strcpy(adapter->ptp_caps.name, "igb-82576");
 		adapter->ptp_caps.max_adj = 1000000000;
 		adapter->ptp_caps.n_ext_ts = 0;
 		adapter->ptp_caps.pps = 0;
@@ -589,7 +589,6 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		/* Dial the nominal frequency. */
 		wr32(E1000_TIMINCA, INCPERIOD_82576 | INCVALUE_82576);
 		break;
-
 	default:
 		adapter->ptp_clock = NULL;
 		return;
-- 
1.7.11.4

Re: [net-next 13/13] igb: Store the MAC address in the name in the PTP struct.

[Richard Cochran]

On Thu, Aug 23, 2012 at 02:56:53AM -0700, Jeff Kirsher wrote:
> From: Matthew Vick <matthew.vick@intel.com>
> 
> Change the name of the adapter in the PTP struct to enable easier
> correlation between interface and PTP device.

You want to put the MAC address into the clock name? This is wrong.

Besides, there is no need for this. The ethool method already makes
the correlation crystal clear.

Thanks,
Richard

RE: [net-next 13/13] igb: Store the MAC address in the name in the PTP struct.

[Vick, Matthew]

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Thursday, August 23, 2012 3:46 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 13/13] igb: Store the MAC address in the name in
> the PTP struct.
> 
> On Thu, Aug 23, 2012 at 02:56:53AM -0700, Jeff Kirsher wrote:
> > From: Matthew Vick <matthew.vick@intel.com>
> >
> > Change the name of the adapter in the PTP struct to enable easier
> > correlation between interface and PTP device.
> 
> You want to put the MAC address into the clock name? This is wrong.
> 
> Besides, there is no need for this. The ethool method already makes the
> correlation crystal clear.
> 
> Thanks,
> Richard

Actually, the name today is wrong. "igb-82580" is inapplicable to I350, I210, and I211 devices. This patch aims to bring some sense to the naming while fixing it. ixgbe implements the same naming scheme.

Cheers,
Matthew

Re: [net-next 13/13] igb: Store the MAC address in the name in the PTP struct.

[Ben Hutchings]

On Thu, 2012-08-23 at 12:45 +0200, Richard Cochran wrote:
> On Thu, Aug 23, 2012 at 02:56:53AM -0700, Jeff Kirsher wrote:
> > From: Matthew Vick <matthew.vick@intel.com>
> > 
> > Change the name of the adapter in the PTP struct to enable easier
> > correlation between interface and PTP device.
> 
> You want to put the MAC address into the clock name? This is wrong.
> 
> Besides, there is no need for this. The ethool method already makes
> the correlation crystal clear.

The name field is described as 'A short name to identify the clock'.  It
is not stated whether this is meant to be the name of the clock *device*
or the clock *driver*.  If it's the name of the device then some unique
ID such as the permanent MAC address is required.

The ixgbe driver uses MAC addresses and so did the last submitted
version of PHC support for the sfc driver.

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.

Re: [net-next 13/13] igb: Store the MAC address in the name in the PTP struct.

[Richard Cochran]

On Thu, Aug 23, 2012 at 10:35:37PM +0100, Ben Hutchings wrote:
> 
> The name field is described as 'A short name to identify the clock'.  It
> is not stated whether this is meant to be the name of the clock *device*
> or the clock *driver*.  If it's the name of the device then some unique
> ID such as the permanent MAC address is required.

In an ideal world, there is only one clock device per system, so the
original concept was to use the driver name. The hardware designers
have ignored or not understood the "one clock per system" model, and
so, unfortunately, we have to deal with it.

A clock device can and should be connected to more than one network
device (see gianfar, dp83640), and to give it a MAC address is
misleading.

The ethtool solution works perfectly to go from interface->clock,
which is all the information that a user space PTP stack needs.

I think for sysfs and the ioctl it is nicer to have the driver name,
since it is associated with the clock and its capabilities.

Thanks,
Richard

RE: [net-next 13/13] igb: Store the MAC address in the name in the PTP struct.

[Keller, Jacob E]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Richard Cochran
> Sent: Thursday, August 23, 2012 11:20 PM
> To: Ben Hutchings
> Cc: Kirsher, Jeffrey T; davem@davemloft.net; Vick, Matthew;
> netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com; Stuart
> Hodgson
> Subject: Re: [net-next 13/13] igb: Store the MAC address in the name in
> the PTP struct.
> 
> In an ideal world, there is only one clock device per system, so the
> original concept was to use the driver name. The hardware designers have
> ignored or not understood the "one clock per system" model, and so,
> unfortunately, we have to deal with it.
> 
> A clock device can and should be connected to more than one network device
> (see gianfar, dp83640), and to give it a MAC address is misleading.
> 
> The ethtool solution works perfectly to go from interface->clock, which is
> all the information that a user space PTP stack needs.
> 
> I think for sysfs and the ioctl it is nicer to have the driver name, since
> it is associated with the clock and its capabilities.
> 
Correct. MAC address is misleading. However driver name for our parts is
misleading because each driver creates one clock per port. In either case, one
driver handling multiple cards would also have to create multiple ptp devices
and that is just as misleading.

The ethtool method is definitely preferred, and is the correct way to identify
the clock device correlation.

- 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