[Intel-wired-lan] [PATCH v3 0/5] ptp cornercase cleanups

[Intel-wired-lan] [PATCH v3 0/5] ptp cornercase cleanups

[Jacob Keller]

This version fixes two minor issues I realized after I was porting them
back to out-of-tree drivers. Sorry for the thrash.

Jacob Keller (5):
  net-intel: fix race condition with PTP_TX_IN_PROGRESS bits
  net-intel: avoid permanent lock of *_PTP_TX_IN_PROGRESS
  net-intel: add statistic indicating number of skipped Tx timestamps
  i40e: use pf data structure directly in i40e_ptp_rx_hang
  net-intel: check for Tx timestamp timeouts during watchdog

 drivers/net/ethernet/intel/e1000e/e1000.h        |  1 +
 drivers/net/ethernet/intel/e1000e/ethtool.c      |  1 +
 drivers/net/ethernet/intel/e1000e/netdev.c       | 27 ++++++++-----
 drivers/net/ethernet/intel/i40e/i40e.h           |  5 ++-
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c   |  1 +
 drivers/net/ethernet/intel/i40e/i40e_main.c      |  3 +-
 drivers/net/ethernet/intel/i40e/i40e_ptp.c       | 48 +++++++++++++++++++++---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c      | 28 +++++++++++---
 drivers/net/ethernet/intel/igb/igb.h             |  2 +
 drivers/net/ethernet/intel/igb/igb_ethtool.c     |  1 +
 drivers/net/ethernet/intel/igb/igb_main.c        | 26 ++++++++++---
 drivers/net/ethernet/intel/igb/igb_ptp.c         | 41 +++++++++++++++++++-
 drivers/net/ethernet/intel/ixgbe/ixgbe.h         |  2 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c |  3 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    | 42 ++++++++++++++-------
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c     | 42 +++++++++++++++++++--
 16 files changed, 227 insertions(+), 46 deletions(-)

-- 
2.13.0.rc0.317.gcc792a6cad5a

[Intel-wired-lan] [PATCH v3 1/5] net-intel: fix race condition with PTP_TX_IN_PROGRESS bits

[Jacob Keller]

Several Intel drivers have a hardware limitation on Tx PTP packets which
requires them to limit to a single Tx timestamp at once. THey do this
mostly using a state bit lock which enforces that only one timestamp
request is honored at a time.

Unfortunately they all suffer from a similar race condition. The bit
lock is not cleared until after skb_tstamp_tx() is called notifying
applications of a new Tx timestamp. Even a well behaved application
sending only one packet at a time and waiting for a response can wake up
and send a new packet before the bit lock is cleared. This results in
needlessly dropping some Tx timestamp requests.

We can fix this by unlocking the state bit as soon as we read the
Timestamp register, as this is the first point at which it is safe to
unlock.

To avoid issues with the skb pointer, we'll use a copy of the pointer
and set the global variable in the driver structure to NULL first. This
ensures that the next timestamp request does not modify our local copy
of the skb pointer.

This ensures that well behaved applications do not accidentally race
with the unlock bit. Obviously an application which sends multiple Tx
timestamp requests at once will still only timestamp one packet at
a time. Unfortunately there is nothing we can do about this.

Since the fix is almost the same for each driver, this patch combines
the fixes for e1000e, igb, ixgbe and i40e.

Reported-by: David Mirabito <davidm@metamako.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c   | 10 ++++++++--
 drivers/net/ethernet/intel/i40e/i40e_ptp.c   | 14 +++++++++++---
 drivers/net/ethernet/intel/igb/igb_ptp.c     | 12 ++++++++++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 15 ++++++++++++---
 4 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index b3679728caac..ec9a50a72550 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -1183,6 +1183,7 @@ static void e1000e_tx_hwtstamp_work(struct work_struct *work)
 	struct e1000_hw *hw = &adapter->hw;
 
 	if (er32(TSYNCTXCTL) & E1000_TSYNCTXCTL_VALID) {
+		struct sk_buff *skb = adapter->tx_hwtstamp_skb;
 		struct skb_shared_hwtstamps shhwtstamps;
 		u64 txstmp;
 
@@ -1191,9 +1192,14 @@ static void e1000e_tx_hwtstamp_work(struct work_struct *work)
 
 		e1000e_systim_to_hwtstamp(adapter, &shhwtstamps, txstmp);
 
-		skb_tstamp_tx(adapter->tx_hwtstamp_skb, &shhwtstamps);
-		dev_kfree_skb_any(adapter->tx_hwtstamp_skb);
+		/* Clear the global tx_hwtstamp_skb pointer and force writes
+		 * prior to notifying the stack of a Tx timestamp.
+		 */
 		adapter->tx_hwtstamp_skb = NULL;
+		wmb(); /* force write prior to skb_tstamp_tx */
+
+		skb_tstamp_tx(skb, &shhwtstamps);
+		dev_kfree_skb_any(skb);
 	} else if (time_after(jiffies, adapter->tx_hwtstamp_start
 			      + adapter->tx_timeout_factor * HZ)) {
 		dev_kfree_skb_any(adapter->tx_hwtstamp_skb);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
index 18c1cc08da97..5a4854e9fd29 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
@@ -338,6 +338,7 @@ void i40e_ptp_rx_hang(struct i40e_vsi *vsi)
 void i40e_ptp_tx_hwtstamp(struct i40e_pf *pf)
 {
 	struct skb_shared_hwtstamps shhwtstamps;
+	struct sk_buff *skb = pf->ptp_tx_skb;
 	struct i40e_hw *hw = &pf->hw;
 	u32 hi, lo;
 	u64 ns;
@@ -353,12 +354,19 @@ void i40e_ptp_tx_hwtstamp(struct i40e_pf *pf)
 	hi = rd32(hw, I40E_PRTTSYN_TXTIME_H);
 
 	ns = (((u64)hi) << 32) | lo;
-
 	i40e_ptp_convert_to_hwtstamp(&shhwtstamps, ns);
-	skb_tstamp_tx(pf->ptp_tx_skb, &shhwtstamps);
-	dev_kfree_skb_any(pf->ptp_tx_skb);
+
+	/* Clear the bit lock as soon as possible after reading the register,
+	 * and prior to notifying the stack via skb_tstamp_tx(). Otherwise
+	 * applications might wake up and attempt to request another transmit
+	 * timestamp prior to the bit lock being cleared.
+	 */
 	pf->ptp_tx_skb = NULL;
 	clear_bit_unlock(__I40E_PTP_TX_IN_PROGRESS, pf->state);
+
+	/* Notify the stack and free the skb after we've unlocked */
+	skb_tstamp_tx(skb, &shhwtstamps);
+	dev_kfree_skb_any(skb);
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 7a3fd4d74592..a2da5738bfb5 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -721,6 +721,7 @@ void igb_ptp_rx_hang(struct igb_adapter *adapter)
  **/
 static void igb_ptp_tx_hwtstamp(struct igb_adapter *adapter)
 {
+	struct sk_buff *skb = adapter->ptp_tx_skb;
 	struct e1000_hw *hw = &adapter->hw;
 	struct skb_shared_hwtstamps shhwtstamps;
 	u64 regval;
@@ -748,10 +749,17 @@ static void igb_ptp_tx_hwtstamp(struct igb_adapter *adapter)
 	shhwtstamps.hwtstamp =
 		ktime_add_ns(shhwtstamps.hwtstamp, adjust);
 
-	skb_tstamp_tx(adapter->ptp_tx_skb, &shhwtstamps);
-	dev_kfree_skb_any(adapter->ptp_tx_skb);
+	/* Clear the lock early before calling skb_tstamp_tx so that
+	 * applications are not woken up before the lock bit is clear. We use
+	 * a copy of the skb pointer to ensure other threads can't change it
+	 * while we're notifying the stack.
+	 */
 	adapter->ptp_tx_skb = NULL;
 	clear_bit_unlock(__IGB_PTP_TX_IN_PROGRESS, &adapter->state);
+
+	/* Notify the stack and free the skb after we've unlocked */
+	skb_tstamp_tx(skb, &shhwtstamps);
+	dev_kfree_skb_any(skb);
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
index ef0635e0918c..9270e6f4fcff 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
@@ -672,17 +672,26 @@ static void ixgbe_ptp_clear_tx_timestamp(struct ixgbe_adapter *adapter)
  */
 static void ixgbe_ptp_tx_hwtstamp(struct ixgbe_adapter *adapter)
 {
+	struct sk_buff *skb = adapter->ptp_tx_skb;
 	struct ixgbe_hw *hw = &adapter->hw;
 	struct skb_shared_hwtstamps shhwtstamps;
 	u64 regval = 0;
 
 	regval |= (u64)IXGBE_READ_REG(hw, IXGBE_TXSTMPL);
 	regval |= (u64)IXGBE_READ_REG(hw, IXGBE_TXSTMPH) << 32;
-
 	ixgbe_ptp_convert_to_hwtstamp(adapter, &shhwtstamps, regval);
-	skb_tstamp_tx(adapter->ptp_tx_skb, &shhwtstamps);
 
-	ixgbe_ptp_clear_tx_timestamp(adapter);
+	/* Handle cleanup of the ptp_tx_skb ourselves, and unlock the state
+	 * bit prior to notifying the stack via skb_tstamp_tx(). This prevents
+	 * well behaved applications from attempting to timestamp again prior
+	 * to the lock bit being clear.
+	 */
+	adapter->ptp_tx_skb = NULL;
+	clear_bit_unlock(__IXGBE_PTP_TX_IN_PROGRESS, &adapter->state);
+
+	/* Notify the stack and then free the skb after we've unlocked */
+	skb_tstamp_tx(skb, &shhwtstamps);
+	dev_kfree_skb_any(skb);
 }
 
 /**
-- 
2.13.0.rc0.317.gcc792a6cad5a

[Intel-wired-lan] [PATCH v3 1/5] net-intel: fix race condition with PTP_TX_IN_PROGRESS bits

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Jacob Keller
> Sent: Friday, April 28, 2017 11:44 AM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: Richard Cochran <richardcochran@gmail.com>
> Subject: [Intel-wired-lan] [PATCH v3 1/5] net-intel: fix race condition with
> PTP_TX_IN_PROGRESS bits
> 
> Several Intel drivers have a hardware limitation on Tx PTP packets which
> requires them to limit to a single Tx timestamp at once. THey do this mostly
> using a state bit lock which enforces that only one timestamp request is
> honored at a time.
> 
> Unfortunately they all suffer from a similar race condition. The bit lock is not
> cleared until after skb_tstamp_tx() is called notifying applications of a new Tx
> timestamp. Even a well behaved application sending only one packet at a
> time and waiting for a response can wake up and send a new packet before
> the bit lock is cleared. This results in needlessly dropping some Tx timestamp
> requests.
> 
> We can fix this by unlocking the state bit as soon as we read the Timestamp
> register, as this is the first point at which it is safe to unlock.
> 
> To avoid issues with the skb pointer, we'll use a copy of the pointer and set
> the global variable in the driver structure to NULL first. This ensures that the
> next timestamp request does not modify our local copy of the skb pointer.
> 
> This ensures that well behaved applications do not accidentally race with the
> unlock bit. Obviously an application which sends multiple Tx timestamp
> requests at once will still only timestamp one packet at a time. Unfortunately
> there is nothing we can do about this.
> 
> Since the fix is almost the same for each driver, this patch combines the fixes
> for e1000e, igb, ixgbe and i40e.
> 
> Reported-by: David Mirabito <davidm@metamako.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c   | 10 ++++++++--
>  drivers/net/ethernet/intel/i40e/i40e_ptp.c   | 14 +++++++++++---
>  drivers/net/ethernet/intel/igb/igb_ptp.c     | 12 ++++++++++--
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 15 ++++++++++++---
>  4 files changed, 41 insertions(+), 10 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [PATCH v3 2/5] net-intel: avoid permanent lock of *_PTP_TX_IN_PROGRESS

[Jacob Keller]

Several drivers share a pattern for Tx timestamping using a bit lock to
indicate when the timestamp is in progress so that multiple packets
cannot be timestamped at once. This is required because we only have one
set of Tx timestamp registers. There exist a few corner cases where we
were not properly cleaning up after ourselves on failure to transmit,
which potentially resulted in the state bit being locked forever.

Add some code at the end of the *_xmit_frame() routines to check and
make sure we cleanup in the case of failure. For the *_tx_map()
functions, we also need to add a return code to indicate when DMA
failure occurred.

This resolves a possible permanent lock of the PTP_TX_IN_PROGRESS bit
for igb, ixgbe, and i40e. The e1000e driver does not have the same
problem due to the ordering of the Tx flow.

Reported-by: Reported-by: David Mirabito <davidm@metamako.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 26 ++++++++++++++++++++------
 drivers/net/ethernet/intel/igb/igb_main.c     | 23 ++++++++++++++++++-----
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 20 +++++++++++++++-----
 3 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 29321a6167a6..19984be0f70c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -2932,10 +2932,12 @@ bool __i40e_chk_linearize(struct sk_buff *skb)
  * @hdr_len:  size of the packet header
  * @td_cmd:   the command field in the descriptor
  * @td_offset: offset for checksum or crc
+ *
+ * Returns 0 on success, -1 on failure to DMA
  **/
-static inline void i40e_tx_map(struct i40e_ring *tx_ring, struct sk_buff *skb,
-			       struct i40e_tx_buffer *first, u32 tx_flags,
-			       const u8 hdr_len, u32 td_cmd, u32 td_offset)
+static inline int i40e_tx_map(struct i40e_ring *tx_ring, struct sk_buff *skb,
+			      struct i40e_tx_buffer *first, u32 tx_flags,
+			      const u8 hdr_len, u32 td_cmd, u32 td_offset)
 {
 	unsigned int data_len = skb->data_len;
 	unsigned int size = skb_headlen(skb);
@@ -3093,7 +3095,7 @@ static inline void i40e_tx_map(struct i40e_ring *tx_ring, struct sk_buff *skb,
 		mmiowb();
 	}
 
-	return;
+	return 0;
 
 dma_error:
 	dev_info(tx_ring->dev, "TX DMA map failed\n");
@@ -3110,6 +3112,8 @@ static inline void i40e_tx_map(struct i40e_ring *tx_ring, struct sk_buff *skb,
 	}
 
 	tx_ring->next_to_use = i;
+
+	return -1;
 }
 
 /**
@@ -3210,8 +3214,9 @@ static netdev_tx_t i40e_xmit_frame_ring(struct sk_buff *skb,
 	 */
 	i40e_atr(tx_ring, skb, tx_flags);
 
-	i40e_tx_map(tx_ring, skb, first, tx_flags, hdr_len,
-		    td_cmd, td_offset);
+	if (i40e_tx_map(tx_ring, skb, first, tx_flags, hdr_len,
+			td_cmd, td_offset))
+		goto cleanup_tx_tstamp;
 
 	return NETDEV_TX_OK;
 
@@ -3219,6 +3224,15 @@ static netdev_tx_t i40e_xmit_frame_ring(struct sk_buff *skb,
 	i40e_trace(xmit_frame_ring_drop, first->skb, tx_ring);
 	dev_kfree_skb_any(first->skb);
 	first->skb = NULL;
+cleanup_tx_tstamp:
+	if (unlikely(tx_flags & I40E_TX_FLAGS_TSYN)) {
+		struct i40e_pf *pf = i40e_netdev_to_pf(tx_ring->netdev);
+
+		dev_kfree_skb_any(pf->ptp_tx_skb);
+		pf->ptp_tx_skb = NULL;
+		clear_bit_unlock(__I40E_PTP_TX_IN_PROGRESS, pf->state);
+	}
+
 	return NETDEV_TX_OK;
 }
 
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index dbb4e3cf7dbf..978908f426a7 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -5201,9 +5201,9 @@ static inline int igb_maybe_stop_tx(struct igb_ring *tx_ring, const u16 size)
 	return __igb_maybe_stop_tx(tx_ring, size);
 }
 
-static void igb_tx_map(struct igb_ring *tx_ring,
-		       struct igb_tx_buffer *first,
-		       const u8 hdr_len)
+static int igb_tx_map(struct igb_ring *tx_ring,
+		      struct igb_tx_buffer *first,
+		      const u8 hdr_len)
 {
 	struct sk_buff *skb = first->skb;
 	struct igb_tx_buffer *tx_buffer;
@@ -5314,7 +5314,7 @@ static void igb_tx_map(struct igb_ring *tx_ring,
 		 */
 		mmiowb();
 	}
-	return;
+	return 0;
 
 dma_error:
 	dev_err(tx_ring->dev, "TX DMA map failed\n");
@@ -5345,6 +5345,8 @@ static void igb_tx_map(struct igb_ring *tx_ring,
 	tx_buffer->skb = NULL;
 
 	tx_ring->next_to_use = i;
+
+	return -1;
 }
 
 netdev_tx_t igb_xmit_frame_ring(struct sk_buff *skb,
@@ -5410,13 +5412,24 @@ netdev_tx_t igb_xmit_frame_ring(struct sk_buff *skb,
 	else if (!tso)
 		igb_tx_csum(tx_ring, first);
 
-	igb_tx_map(tx_ring, first, hdr_len);
+	if (igb_tx_map(tx_ring, first, hdr_len))
+		goto cleanup_tx_tstamp;
 
 	return NETDEV_TX_OK;
 
 out_drop:
 	dev_kfree_skb_any(first->skb);
 	first->skb = NULL;
+cleanup_tx_tstamp:
+	if (unlikely(tx_flags & IGB_TX_FLAGS_TSTAMP)) {
+		struct igb_adapter *adapter = netdev_priv(tx_ring->netdev);
+
+		dev_kfree_skb_any(adapter->ptp_tx_skb);
+		adapter->ptp_tx_skb = NULL;
+		if (adapter->hw.mac.type == e1000_82576)
+			cancel_work_sync(&adapter->ptp_tx_work);
+		clear_bit_unlock(__IGB_PTP_TX_IN_PROGRESS, &adapter->state);
+	}
 
 	return NETDEV_TX_OK;
 }
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 68d849346cdf..54bf0f9f5573 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7885,9 +7885,9 @@ static inline int ixgbe_maybe_stop_tx(struct ixgbe_ring *tx_ring, u16 size)
 #define IXGBE_TXD_CMD (IXGBE_TXD_CMD_EOP | \
 		       IXGBE_TXD_CMD_RS)
 
-static void ixgbe_tx_map(struct ixgbe_ring *tx_ring,
-			 struct ixgbe_tx_buffer *first,
-			 const u8 hdr_len)
+static int ixgbe_tx_map(struct ixgbe_ring *tx_ring,
+			struct ixgbe_tx_buffer *first,
+			const u8 hdr_len)
 {
 	struct sk_buff *skb = first->skb;
 	struct ixgbe_tx_buffer *tx_buffer;
@@ -8014,7 +8014,7 @@ static void ixgbe_tx_map(struct ixgbe_ring *tx_ring,
 		mmiowb();
 	}
 
-	return;
+	return 0;
 dma_error:
 	dev_err(tx_ring->dev, "TX DMA map failed\n");
 	tx_buffer = &tx_ring->tx_buffer_info[i];
@@ -8044,6 +8044,8 @@ static void ixgbe_tx_map(struct ixgbe_ring *tx_ring,
 	first->skb = NULL;
 
 	tx_ring->next_to_use = i;
+
+	return -1;
 }
 
 static void ixgbe_atr(struct ixgbe_ring *ring,
@@ -8416,13 +8418,21 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb,
 #ifdef IXGBE_FCOE
 xmit_fcoe:
 #endif /* IXGBE_FCOE */
-	ixgbe_tx_map(tx_ring, first, hdr_len);
+	if (ixgbe_tx_map(tx_ring, first, hdr_len))
+		goto cleanup_tx_timestamp;
 
 	return NETDEV_TX_OK;
 
 out_drop:
 	dev_kfree_skb_any(first->skb);
 	first->skb = NULL;
+cleanup_tx_timestamp:
+	if (unlikely(tx_flags & IXGBE_TX_FLAGS_TSTAMP)) {
+		dev_kfree_skb_any(adapter->ptp_tx_skb);
+		adapter->ptp_tx_skb = NULL;
+		cancel_work_sync(&adapter->ptp_tx_work);
+		clear_bit_unlock(__IXGBE_PTP_TX_IN_PROGRESS, &adapter->state);
+	}
 
 	return NETDEV_TX_OK;
 }
-- 
2.13.0.rc0.317.gcc792a6cad5a

[Intel-wired-lan] [PATCH v3 2/5] net-intel: avoid permanent lock of *_PTP_TX_IN_PROGRESS

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Jacob Keller
> Sent: Friday, April 28, 2017 11:44 AM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: Richard Cochran <richardcochran@gmail.com>
> Subject: [Intel-wired-lan] [PATCH v3 2/5] net-intel: avoid permanent lock of
> *_PTP_TX_IN_PROGRESS
> 
> Several drivers share a pattern for Tx timestamping using a bit lock to indicate
> when the timestamp is in progress so that multiple packets cannot be
> timestamped at once. This is required because we only have one set of Tx
> timestamp registers. There exist a few corner cases where we were not
> properly cleaning up after ourselves on failure to transmit, which potentially
> resulted in the state bit being locked forever.
> 
> Add some code at the end of the *_xmit_frame() routines to check and
> make sure we cleanup in the case of failure. For the *_tx_map() functions,
> we also need to add a return code to indicate when DMA failure occurred.
> 
> This resolves a possible permanent lock of the PTP_TX_IN_PROGRESS bit for
> igb, ixgbe, and i40e. The e1000e driver does not have the same problem due
> to the ordering of the Tx flow.
> 
> Reported-by: Reported-by: David Mirabito <davidm@metamako.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 26
> ++++++++++++++++++++------
>  drivers/net/ethernet/intel/igb/igb_main.c     | 23 ++++++++++++++++++----
> -
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 20 +++++++++++++++-----
>  3 files changed, 53 insertions(+), 16 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [PATCH v3 3/5] net-intel: add statistic indicating number of skipped Tx timestamps

[Jacob Keller]

The e1000e, igb, i40e, and ixgbe drivers share similar code for handling
Tx timestamps, with a similar limitation of only one Tx timestamp
register set. Because the hardware only supports one Tx timestamp at
a time, the drivers may silently ignore timestamp requests for packets
while waiting for a previous packet timestamp to complete. Add a new
statistic, tx_hwtstamp_skipped, which indicates how many times this has
happened. This is useful when trying to determine why a specific
application is not receiving timestamps. Without this statistic, the
developer may not have any indication that the timestamp was
intentionally skipped.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/e1000e/e1000.h        |  1 +
 drivers/net/ethernet/intel/e1000e/ethtool.c      |  1 +
 drivers/net/ethernet/intel/e1000e/netdev.c       | 17 ++++++++++-------
 drivers/net/ethernet/intel/i40e/i40e.h           |  1 +
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c   |  1 +
 drivers/net/ethernet/intel/i40e/i40e_txrx.c      |  1 +
 drivers/net/ethernet/intel/igb/igb.h             |  1 +
 drivers/net/ethernet/intel/igb/igb_ethtool.c     |  1 +
 drivers/net/ethernet/intel/igb/igb_main.c        |  2 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe.h         |  1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c |  3 +++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    | 21 ++++++++++++---------
 12 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
index c7c994eb410e..98e68888abb1 100644
--- a/drivers/net/ethernet/intel/e1000e/e1000.h
+++ b/drivers/net/ethernet/intel/e1000e/e1000.h
@@ -268,6 +268,7 @@ struct e1000_adapter {
 	u32 tx_fifo_size;
 	u32 tx_dma_failed;
 	u32 tx_hwtstamp_timeouts;
+	u32 tx_hwtstamp_skipped;
 
 	/* Rx */
 	bool (*clean_rx)(struct e1000_ring *ring, int *work_done,
diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
index e23dbd9190d6..c658f6ebf7cb 100644
--- a/drivers/net/ethernet/intel/e1000e/ethtool.c
+++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
@@ -105,6 +105,7 @@ static const struct e1000_stats e1000_gstrings_stats[] = {
 	E1000_STAT("uncorr_ecc_errors", uncorr_errors),
 	E1000_STAT("corr_ecc_errors", corr_errors),
 	E1000_STAT("tx_hwtstamp_timeouts", tx_hwtstamp_timeouts),
+	E1000_STAT("tx_hwtstamp_skipped", tx_hwtstamp_skipped),
 };
 
 #define E1000_GLOBAL_STATS_LEN	ARRAY_SIZE(e1000_gstrings_stats)
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index ec9a50a72550..42424274853c 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -5866,13 +5866,16 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
 			     nr_frags);
 	if (count) {
 		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
-		    (adapter->flags & FLAG_HAS_HW_TIMESTAMP) &&
-		    !adapter->tx_hwtstamp_skb) {
-			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
-			tx_flags |= E1000_TX_FLAGS_HWTSTAMP;
-			adapter->tx_hwtstamp_skb = skb_get(skb);
-			adapter->tx_hwtstamp_start = jiffies;
-			schedule_work(&adapter->tx_hwtstamp_work);
+		    (adapter->flags & FLAG_HAS_HW_TIMESTAMP)) {
+			if (!adapter->tx_hwtstamp_skb) {
+				skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+				tx_flags |= E1000_TX_FLAGS_HWTSTAMP;
+				adapter->tx_hwtstamp_skb = skb_get(skb);
+				adapter->tx_hwtstamp_start = jiffies;
+				schedule_work(&adapter->tx_hwtstamp_work);
+			} else {
+				adapter->tx_hwtstamp_skipped++;
+			}
 		} else {
 			skb_tx_timestamp(skb);
 		}
diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index cdde3cc28fb5..aa46ae016539 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -506,6 +506,7 @@ struct i40e_pf {
 	struct mutex tmreg_lock; /* Used to protect the SYSTIME registers. */
 	u64 ptp_base_adj;
 	u32 tx_hwtstamp_timeouts;
+	u32 tx_hwtstamp_skipped;
 	u32 rx_hwtstamp_cleared;
 	u32 latch_event_flags;
 	spinlock_t ptp_rx_lock; /* Used to protect Rx timestamp registers. */
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 7a8eb486b9ea..35a246f05520 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -147,6 +147,7 @@ static const struct i40e_stats i40e_gstrings_stats[] = {
 	I40E_PF_STAT("VF_admin_queue_requests", vf_aq_requests),
 	I40E_PF_STAT("arq_overflows", arq_overflows),
 	I40E_PF_STAT("rx_hwtstamp_cleared", rx_hwtstamp_cleared),
+	I40E_PF_STAT("tx_hwtstamp_skipped", tx_hwtstamp_skipped),
 	I40E_PF_STAT("fdir_flush_cnt", fd_flush_cnt),
 	I40E_PF_STAT("fdir_atr_match", stats.fd_atr_match),
 	I40E_PF_STAT("fdir_atr_tunnel_match", stats.fd_atr_tunnel_match),
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 19984be0f70c..c69ee4b0cfe2 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -2630,6 +2630,7 @@ static int i40e_tsyn(struct i40e_ring *tx_ring, struct sk_buff *skb,
 		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 		pf->ptp_tx_skb = skb_get(skb);
 	} else {
+		pf->tx_hwtstamp_skipped++;
 		return 0;
 	}
 
diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index bf9bf9056d0c..be35edcf6b08 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -563,6 +563,7 @@ struct igb_adapter {
 	struct cyclecounter cc;
 	struct timecounter tc;
 	u32 tx_hwtstamp_timeouts;
+	u32 tx_hwtstamp_skipped;
 	u32 rx_hwtstamp_cleared;
 	bool pps_sys_wrap_on;
 
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 0efb62db6efd..8730f7cbce68 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -90,6 +90,7 @@ static const struct igb_stats igb_gstrings_stats[] = {
 	IGB_STAT("os2bmc_tx_by_host", stats.o2bspc),
 	IGB_STAT("os2bmc_rx_by_host", stats.b2ogprc),
 	IGB_STAT("tx_hwtstamp_timeouts", tx_hwtstamp_timeouts),
+	IGB_STAT("tx_hwtstamp_skipped", tx_hwtstamp_skipped),
 	IGB_STAT("rx_hwtstamp_cleared", rx_hwtstamp_cleared),
 };
 
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 978908f426a7..21b455bfb4ca 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -5392,6 +5392,8 @@ netdev_tx_t igb_xmit_frame_ring(struct sk_buff *skb,
 			adapter->ptp_tx_start = jiffies;
 			if (adapter->hw.mac.type == e1000_82576)
 				schedule_work(&adapter->ptp_tx_work);
+		} else {
+			adapter->tx_hwtstamp_skipped++;
 		}
 	}
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 76263762bea1..eb36106218ad 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -733,6 +733,7 @@ struct ixgbe_adapter {
 	struct timecounter hw_tc;
 	u32 base_incval;
 	u32 tx_hwtstamp_timeouts;
+	u32 tx_hwtstamp_skipped;
 	u32 rx_hwtstamp_cleared;
 	void (*ptp_setup_sdp)(struct ixgbe_adapter *);
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index cced74dd5a63..0b75d304b009 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -111,6 +111,9 @@ static const struct ixgbe_stats ixgbe_gstrings_stats[] = {
 	{"os2bmc_tx_by_bmc", IXGBE_STAT(stats.b2ospc)},
 	{"os2bmc_tx_by_host", IXGBE_STAT(stats.o2bspc)},
 	{"os2bmc_rx_by_host", IXGBE_STAT(stats.b2ogprc)},
+	{"tx_hwtstamp_timeouts", IXGBE_STAT(tx_hwtstamp_timeouts)},
+	{"tx_hwtstamp_skipped", IXGBE_STAT(tx_hwtstamp_skipped)},
+	{"rx_hwtstamp_cleared", IXGBE_STAT(rx_hwtstamp_cleared)},
 #ifdef IXGBE_FCOE
 	{"fcoe_bad_fccrc", IXGBE_STAT(stats.fccrc)},
 	{"rx_fcoe_dropped", IXGBE_STAT(stats.fcoerpdc)},
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 54bf0f9f5573..c61459fcc21e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8346,16 +8346,19 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb,
 	protocol = vlan_get_protocol(skb);
 
 	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
-	    adapter->ptp_clock &&
-	    !test_and_set_bit_lock(__IXGBE_PTP_TX_IN_PROGRESS,
-				   &adapter->state)) {
-		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
-		tx_flags |= IXGBE_TX_FLAGS_TSTAMP;
+	    adapter->ptp_clock) {
+		if (!test_and_set_bit_lock(__IXGBE_PTP_TX_IN_PROGRESS,
+					   &adapter->state)) {
+			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+			tx_flags |= IXGBE_TX_FLAGS_TSTAMP;
 
-		/* schedule check for Tx timestamp */
-		adapter->ptp_tx_skb = skb_get(skb);
-		adapter->ptp_tx_start = jiffies;
-		schedule_work(&adapter->ptp_tx_work);
+			/* schedule check for Tx timestamp */
+			adapter->ptp_tx_skb = skb_get(skb);
+			adapter->ptp_tx_start = jiffies;
+			schedule_work(&adapter->ptp_tx_work);
+		} else {
+			adapter->tx_hwtstamp_skipped++;
+		}
 	}
 
 	skb_tx_timestamp(skb);
-- 
2.13.0.rc0.317.gcc792a6cad5a

[Intel-wired-lan] [PATCH v3 3/5] net-intel: add statistic indicating number of skipped Tx timestamps

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Jacob Keller
> Sent: Friday, April 28, 2017 11:44 AM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: Richard Cochran <richardcochran@gmail.com>
> Subject: [Intel-wired-lan] [PATCH v3 3/5] net-intel: add statistic indicating
> number of skipped Tx timestamps
> 
> The e1000e, igb, i40e, and ixgbe drivers share similar code for handling Tx
> timestamps, with a similar limitation of only one Tx timestamp register set.
> Because the hardware only supports one Tx timestamp at a time, the drivers
> may silently ignore timestamp requests for packets while waiting for a
> previous packet timestamp to complete. Add a new statistic,
> tx_hwtstamp_skipped, which indicates how many times this has happened.
> This is useful when trying to determine why a specific application is not
> receiving timestamps. Without this statistic, the developer may not have any
> indication that the timestamp was intentionally skipped.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/e1000e/e1000.h        |  1 +
>  drivers/net/ethernet/intel/e1000e/ethtool.c      |  1 +
>  drivers/net/ethernet/intel/e1000e/netdev.c       | 17 ++++++++++-------
>  drivers/net/ethernet/intel/i40e/i40e.h           |  1 +
>  drivers/net/ethernet/intel/i40e/i40e_ethtool.c   |  1 +
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c      |  1 +
>  drivers/net/ethernet/intel/igb/igb.h             |  1 +
>  drivers/net/ethernet/intel/igb/igb_ethtool.c     |  1 +
>  drivers/net/ethernet/intel/igb/igb_main.c        |  2 ++
>  drivers/net/ethernet/intel/ixgbe/ixgbe.h         |  1 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c |  3 +++
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    | 21 ++++++++++++--------
> -
>  12 files changed, 35 insertions(+), 16 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [PATCH v3 4/5] i40e: use pf data structure directly in i40e_ptp_rx_hang

[Jacob Keller]

There's no reason to pass a *vsi pointer if we already have the *pf
pointer in the only location where we call this function. Lets update
the signature and directly pass the *pf data structure pointer.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e.h      | 2 +-
 drivers/net/ethernet/intel/i40e/i40e_main.c | 2 +-
 drivers/net/ethernet/intel/i40e/i40e_ptp.c  | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index aa46ae016539..f4465afe1fe1 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -956,7 +956,7 @@ bool i40e_dcb_need_reconfig(struct i40e_pf *pf,
 			    struct i40e_dcbx_config *old_cfg,
 			    struct i40e_dcbx_config *new_cfg);
 #endif /* CONFIG_I40E_DCB */
-void i40e_ptp_rx_hang(struct i40e_vsi *vsi);
+void i40e_ptp_rx_hang(struct i40e_pf *pf);
 void i40e_ptp_tx_hwtstamp(struct i40e_pf *pf);
 void i40e_ptp_rx_hwtstamp(struct i40e_pf *pf, struct sk_buff *skb, u8 index);
 void i40e_ptp_set_increment(struct i40e_pf *pf);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index d5c9c9e06ff5..c019dec988e3 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -6372,7 +6372,7 @@ static void i40e_watchdog_subtask(struct i40e_pf *pf)
 				i40e_update_veb_stats(pf->veb[i]);
 	}
 
-	i40e_ptp_rx_hang(pf->vsi[pf->lan_vsi]);
+	i40e_ptp_rx_hang(pf);
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
index 5a4854e9fd29..876ea169816a 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
@@ -269,6 +269,7 @@ static u32 i40e_ptp_get_rx_events(struct i40e_pf *pf)
 
 /**
  * i40e_ptp_rx_hang - Detect error case when Rx timestamp registers are hung
+ * @pf: The PF private data structure
  * @vsi: The VSI with the rings relevant to 1588
  *
  * This watchdog task is scheduled to detect error case where hardware has
@@ -276,9 +277,8 @@ static u32 i40e_ptp_get_rx_events(struct i40e_pf *pf)
  * particular error is rare but leaves the device in a state unable to timestamp
  * any future packets.
  **/
-void i40e_ptp_rx_hang(struct i40e_vsi *vsi)
+void i40e_ptp_rx_hang(struct i40e_pf *pf)
 {
-	struct i40e_pf *pf = vsi->back;
 	struct i40e_hw *hw = &pf->hw;
 	unsigned int i, cleared = 0;
 
-- 
2.13.0.rc0.317.gcc792a6cad5a

[Intel-wired-lan] [PATCH v3 4/5] i40e: use pf data structure directly in i40e_ptp_rx_hang

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Jacob Keller
> Sent: Friday, April 28, 2017 11:44 AM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: Richard Cochran <richardcochran@gmail.com>
> Subject: [Intel-wired-lan] [PATCH v3 4/5] i40e: use pf data structure directly
> in i40e_ptp_rx_hang
> 
> There's no reason to pass a *vsi pointer if we already have the *pf pointer in
> the only location where we call this function. Lets update the signature and
> directly pass the *pf data structure pointer.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e.h      | 2 +-
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 2 +-
> drivers/net/ethernet/intel/i40e/i40e_ptp.c  | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [PATCH v3 5/5] net-intel: check for Tx timestamp timeouts during watchdog

[Jacob Keller]

Several drivers share similar logic for handling the Tx timestamps.
These drivers have some parts which rely on the interrupt instead of
only a polling work task. Although unlikely it may be possible in some
circumstances for the PTP tx timestamp to never occur. If this happens,
the result is that all future Tx timestamp requests will be ignored
permanently.

Fix this by adding a *ptp_tx_hang() function similar to the already
existing *ptp_rx_hang() routine which checks to make sure that the
timestamp hasn't taken too long. This ensures that we will eventually
recover from this case.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e.h        |  2 ++
 drivers/net/ethernet/intel/i40e/i40e_main.c   |  1 +
 drivers/net/ethernet/intel/i40e/i40e_ptp.c    | 30 +++++++++++++++++++++++++++
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   |  1 +
 drivers/net/ethernet/intel/igb/igb.h          |  1 +
 drivers/net/ethernet/intel/igb/igb_main.c     |  1 +
 drivers/net/ethernet/intel/igb/igb_ptp.c      | 29 ++++++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |  1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c  | 27 ++++++++++++++++++++++++
 10 files changed, 94 insertions(+)

diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index f4465afe1fe1..25bf336c5f38 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -502,6 +502,7 @@ struct i40e_pf {
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info ptp_caps;
 	struct sk_buff *ptp_tx_skb;
+	unsigned long ptp_tx_start;
 	struct hwtstamp_config tstamp_config;
 	struct mutex tmreg_lock; /* Used to protect the SYSTIME registers. */
 	u64 ptp_base_adj;
@@ -957,6 +958,7 @@ bool i40e_dcb_need_reconfig(struct i40e_pf *pf,
 			    struct i40e_dcbx_config *new_cfg);
 #endif /* CONFIG_I40E_DCB */
 void i40e_ptp_rx_hang(struct i40e_pf *pf);
+void i40e_ptp_tx_hang(struct i40e_pf *pf);
 void i40e_ptp_tx_hwtstamp(struct i40e_pf *pf);
 void i40e_ptp_rx_hwtstamp(struct i40e_pf *pf, struct sk_buff *skb, u8 index);
 void i40e_ptp_set_increment(struct i40e_pf *pf);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index c019dec988e3..e4eb97832413 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -6373,6 +6373,7 @@ static void i40e_watchdog_subtask(struct i40e_pf *pf)
 	}
 
 	i40e_ptp_rx_hang(pf);
+	i40e_ptp_tx_hang(pf);
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
index 876ea169816a..cd35c4b9a8b0 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
@@ -327,6 +327,36 @@ void i40e_ptp_rx_hang(struct i40e_pf *pf)
 	pf->rx_hwtstamp_cleared += cleared;
 }
 
+/**
+ * i40e_ptp_tx_hang - Detect error case when Tx timestamp register is hung
+ * @pf: The PF private data structure
+ *
+ * This watchdog task is run periodically to make sure that we clear the Tx
+ * timestamp logic if we don't obtain a timestamp in a reasonable amount of
+ * time. It is unexpected in the normal case but if it occurs it results in
+ * permanently prevent timestamps of future packets
+ **/
+void i40e_ptp_tx_hang(struct i40e_pf *pf)
+{
+	if (!(pf->flags & I40E_FLAG_PTP) || !pf->ptp_tx)
+		return;
+
+	/* Nothing to do if we're not already waiting for a timestamp */
+	if (!test_bit(__I40E_PTP_TX_IN_PROGRESS, pf->state))
+		return;
+
+	/* We already have a handler routine which is run when we are notified
+	 * of a Tx timestamp in the hardware. If we don't get an interrupt
+	 * within a second it is reasonable to assume that we never will.
+	 */
+	if (time_is_before_jiffies(pf->ptp_tx_start + HZ)) {
+		dev_kfree_skb_any(pf->ptp_tx_skb);
+		pf->ptp_tx_skb = NULL;
+		clear_bit_unlock(__I40E_PTP_TX_IN_PROGRESS, pf->state);
+		pf->tx_hwtstamp_timeouts++;
+	}
+}
+
 /**
  * i40e_ptp_tx_hwtstamp - Utility function which returns the Tx timestamp
  * @pf: Board private structure
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index c69ee4b0cfe2..c2e9013d05eb 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -2628,6 +2628,7 @@ static int i40e_tsyn(struct i40e_ring *tx_ring, struct sk_buff *skb,
 	if (pf->ptp_tx &&
 	    !test_and_set_bit_lock(__I40E_PTP_TX_IN_PROGRESS, pf->state)) {
 		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+		pf->ptp_tx_start = jiffies;
 		pf->ptp_tx_skb = skb_get(skb);
 	} else {
 		pf->tx_hwtstamp_skipped++;
diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index be35edcf6b08..ff4d9073781a 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -677,6 +677,7 @@ void igb_ptp_stop(struct igb_adapter *adapter);
 void igb_ptp_reset(struct igb_adapter *adapter);
 void igb_ptp_suspend(struct igb_adapter *adapter);
 void igb_ptp_rx_hang(struct igb_adapter *adapter);
+void igb_ptp_tx_hang(struct igb_adapter *adapter);
 void igb_ptp_rx_rgtstamp(struct igb_q_vector *q_vector, struct sk_buff *skb);
 void igb_ptp_rx_pktstamp(struct igb_q_vector *q_vector, void *va,
 			 struct sk_buff *skb);
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 21b455bfb4ca..eec54d9df06b 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -4726,6 +4726,7 @@ static void igb_watchdog_task(struct work_struct *work)
 
 	igb_spoof_check(adapter);
 	igb_ptp_rx_hang(adapter);
+	igb_ptp_tx_hang(adapter);
 
 	/* Check LVMMC register on i350/i354 only */
 	if ((adapter->hw.mac.type == e1000_i350) ||
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index a2da5738bfb5..b2f67c169bd2 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -711,6 +711,35 @@ void igb_ptp_rx_hang(struct igb_adapter *adapter)
 	}
 }
 
+/**
+ * igb_ptp_tx_hang - detect error case where Tx timestamp never finishes
+ * @adapter: private network adapter structure
+ */
+void igb_ptp_tx_hang(struct igb_adapter *adapter)
+{
+	bool timeout = time_is_before_jiffies(adapter->ptp_tx_start +
+					      IGB_PTP_TX_TIMEOUT);
+
+	if (!adapter->ptp_tx_skb)
+		return;
+
+	if (!test_bit(__IGB_PTP_TX_IN_PROGRESS, &adapter->state))
+		return;
+
+	/* If we haven't received a timestamp within the timeout, it is
+	 * reasonable to assume that it will never occur, so we can unlock the
+	 * timestamp bit when this occurs.
+	 */
+	if (timeout) {
+		cancel_work_sync(&adapter->ptp_tx_work);
+		dev_kfree_skb_any(adapter->ptp_tx_skb);
+		adapter->ptp_tx_skb = NULL;
+		clear_bit_unlock(__IGB_PTP_TX_IN_PROGRESS, &adapter->state);
+		adapter->tx_hwtstamp_timeouts++;
+		dev_warn(&adapter->pdev->dev, "clearing Tx timestamp hang\n");
+	}
+}
+
 /**
  * igb_ptp_tx_hwtstamp - utility function which checks for TX time stamp
  * @adapter: Board private structure.
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index eb36106218ad..dd5578756ae0 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -961,6 +961,7 @@ void ixgbe_ptp_suspend(struct ixgbe_adapter *adapter);
 void ixgbe_ptp_stop(struct ixgbe_adapter *adapter);
 void ixgbe_ptp_overflow_check(struct ixgbe_adapter *adapter);
 void ixgbe_ptp_rx_hang(struct ixgbe_adapter *adapter);
+void ixgbe_ptp_tx_hang(struct ixgbe_adapter *adapter);
 void ixgbe_ptp_rx_pktstamp(struct ixgbe_q_vector *, struct sk_buff *);
 void ixgbe_ptp_rx_rgtstamp(struct ixgbe_q_vector *, struct sk_buff *skb);
 static inline void ixgbe_ptp_rx_hwtstamp(struct ixgbe_ring *rx_ring,
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index c61459fcc21e..f3199c73faed 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7645,6 +7645,7 @@ static void ixgbe_service_task(struct work_struct *work)
 	if (test_bit(__IXGBE_PTP_RUNNING, &adapter->state)) {
 		ixgbe_ptp_overflow_check(adapter);
 		ixgbe_ptp_rx_hang(adapter);
+		ixgbe_ptp_tx_hang(adapter);
 	}
 
 	ixgbe_service_event_complete(adapter);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
index 9270e6f4fcff..24f4eaf37727 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
@@ -662,6 +662,33 @@ static void ixgbe_ptp_clear_tx_timestamp(struct ixgbe_adapter *adapter)
 	clear_bit_unlock(__IXGBE_PTP_TX_IN_PROGRESS, &adapter->state);
 }
 
+/**
+ * ixgbe_ptp_tx_hang - detect error case where Tx timestamp never finishes
+ * @adapter: private network adapter structure
+ */
+void ixgbe_ptp_tx_hang(struct ixgbe_adapter *adapter)
+{
+	bool timeout = time_is_before_jiffies(adapter->ptp_tx_start +
+					      IXGBE_PTP_TX_TIMEOUT);
+
+	if (!adapter->ptp_tx_skb)
+		return;
+
+	if (!test_bit(__IXGBE_PTP_TX_IN_PROGRESS, &adapter->state))
+		return;
+
+	/* If we haven't received a timestamp within the timeout, it is
+	 * reasonable to assume that it will never occur, so we can unlock the
+	 * timestamp bit when this occurs.
+	 */
+	if (timeout) {
+		cancel_work_sync(&adapter->ptp_tx_work);
+		ixgbe_ptp_clear_tx_timestamp(adapter);
+		adapter->tx_hwtstamp_timeouts++;
+		e_warn(drv, "clearing Tx timestamp hang\n");
+	}
+}
+
 /**
  * ixgbe_ptp_tx_hwtstamp - utility function which checks for TX time stamp
  * @adapter: the private adapter struct
-- 
2.13.0.rc0.317.gcc792a6cad5a

[Intel-wired-lan] [PATCH v3 5/5] net-intel: check for Tx timestamp timeouts during watchdog

[Shannon Nelson]

On 4/28/2017 11:44 AM, Jacob Keller wrote:
> Several drivers share similar logic for handling the Tx timestamps.
> These drivers have some parts which rely on the interrupt instead of
> only a polling work task. Although unlikely it may be possible in some
> circumstances for the PTP tx timestamp to never occur. If this happens,
> the result is that all future Tx timestamp requests will be ignored
> permanently.
>
> Fix this by adding a *ptp_tx_hang() function similar to the already
> existing *ptp_rx_hang() routine which checks to make sure that the
> timestamp hasn't taken too long. This ensures that we will eventually
> recover from this case.
>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e.h        |  2 ++
>  drivers/net/ethernet/intel/i40e/i40e_main.c   |  1 +
>  drivers/net/ethernet/intel/i40e/i40e_ptp.c    | 30 +++++++++++++++++++++++++++
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c   |  1 +
>  drivers/net/ethernet/intel/igb/igb.h          |  1 +
>  drivers/net/ethernet/intel/igb/igb_main.c     |  1 +
>  drivers/net/ethernet/intel/igb/igb_ptp.c      | 29 ++++++++++++++++++++++++++
>  drivers/net/ethernet/intel/ixgbe/ixgbe.h      |  1 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  1 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c  | 27 ++++++++++++++++++++++++
>  10 files changed, 94 insertions(+)

I'd prefer to see the patches split up between the drivers.  I realize 
that some commits in this series are simple patches applied to all the 
drivers, but something like this is big enough that each driver should 
be split out to a separate patch.

sln

>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
> index f4465afe1fe1..25bf336c5f38 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e.h
> @@ -502,6 +502,7 @@ struct i40e_pf {
>  	struct ptp_clock *ptp_clock;
>  	struct ptp_clock_info ptp_caps;
>  	struct sk_buff *ptp_tx_skb;
> +	unsigned long ptp_tx_start;
>  	struct hwtstamp_config tstamp_config;
>  	struct mutex tmreg_lock; /* Used to protect the SYSTIME registers. */
>  	u64 ptp_base_adj;
> @@ -957,6 +958,7 @@ bool i40e_dcb_need_reconfig(struct i40e_pf *pf,
>  			    struct i40e_dcbx_config *new_cfg);
>  #endif /* CONFIG_I40E_DCB */
>  void i40e_ptp_rx_hang(struct i40e_pf *pf);
> +void i40e_ptp_tx_hang(struct i40e_pf *pf);
>  void i40e_ptp_tx_hwtstamp(struct i40e_pf *pf);
>  void i40e_ptp_rx_hwtstamp(struct i40e_pf *pf, struct sk_buff *skb, u8 index);
>  void i40e_ptp_set_increment(struct i40e_pf *pf);
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index c019dec988e3..e4eb97832413 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -6373,6 +6373,7 @@ static void i40e_watchdog_subtask(struct i40e_pf *pf)
>  	}
>
>  	i40e_ptp_rx_hang(pf);
> +	i40e_ptp_tx_hang(pf);
>  }
>
>  /**
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> index 876ea169816a..cd35c4b9a8b0 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> @@ -327,6 +327,36 @@ void i40e_ptp_rx_hang(struct i40e_pf *pf)
>  	pf->rx_hwtstamp_cleared += cleared;
>  }
>
> +/**
> + * i40e_ptp_tx_hang - Detect error case when Tx timestamp register is hung
> + * @pf: The PF private data structure
> + *
> + * This watchdog task is run periodically to make sure that we clear the Tx
> + * timestamp logic if we don't obtain a timestamp in a reasonable amount of
> + * time. It is unexpected in the normal case but if it occurs it results in
> + * permanently prevent timestamps of future packets
> + **/
> +void i40e_ptp_tx_hang(struct i40e_pf *pf)
> +{
> +	if (!(pf->flags & I40E_FLAG_PTP) || !pf->ptp_tx)
> +		return;
> +
> +	/* Nothing to do if we're not already waiting for a timestamp */
> +	if (!test_bit(__I40E_PTP_TX_IN_PROGRESS, pf->state))
> +		return;
> +
> +	/* We already have a handler routine which is run when we are notified
> +	 * of a Tx timestamp in the hardware. If we don't get an interrupt
> +	 * within a second it is reasonable to assume that we never will.
> +	 */
> +	if (time_is_before_jiffies(pf->ptp_tx_start + HZ)) {
> +		dev_kfree_skb_any(pf->ptp_tx_skb);
> +		pf->ptp_tx_skb = NULL;
> +		clear_bit_unlock(__I40E_PTP_TX_IN_PROGRESS, pf->state);
> +		pf->tx_hwtstamp_timeouts++;
> +	}
> +}
> +
>  /**
>   * i40e_ptp_tx_hwtstamp - Utility function which returns the Tx timestamp
>   * @pf: Board private structure
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index c69ee4b0cfe2..c2e9013d05eb 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> @@ -2628,6 +2628,7 @@ static int i40e_tsyn(struct i40e_ring *tx_ring, struct sk_buff *skb,
>  	if (pf->ptp_tx &&
>  	    !test_and_set_bit_lock(__I40E_PTP_TX_IN_PROGRESS, pf->state)) {
>  		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +		pf->ptp_tx_start = jiffies;
>  		pf->ptp_tx_skb = skb_get(skb);
>  	} else {
>  		pf->tx_hwtstamp_skipped++;
> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> index be35edcf6b08..ff4d9073781a 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -677,6 +677,7 @@ void igb_ptp_stop(struct igb_adapter *adapter);
>  void igb_ptp_reset(struct igb_adapter *adapter);
>  void igb_ptp_suspend(struct igb_adapter *adapter);
>  void igb_ptp_rx_hang(struct igb_adapter *adapter);
> +void igb_ptp_tx_hang(struct igb_adapter *adapter);
>  void igb_ptp_rx_rgtstamp(struct igb_q_vector *q_vector, struct sk_buff *skb);
>  void igb_ptp_rx_pktstamp(struct igb_q_vector *q_vector, void *va,
>  			 struct sk_buff *skb);
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 21b455bfb4ca..eec54d9df06b 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -4726,6 +4726,7 @@ static void igb_watchdog_task(struct work_struct *work)
>
>  	igb_spoof_check(adapter);
>  	igb_ptp_rx_hang(adapter);
> +	igb_ptp_tx_hang(adapter);
>
>  	/* Check LVMMC register on i350/i354 only */
>  	if ((adapter->hw.mac.type == e1000_i350) ||
> diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
> index a2da5738bfb5..b2f67c169bd2 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> @@ -711,6 +711,35 @@ void igb_ptp_rx_hang(struct igb_adapter *adapter)
>  	}
>  }
>
> +/**
> + * igb_ptp_tx_hang - detect error case where Tx timestamp never finishes
> + * @adapter: private network adapter structure
> + */
> +void igb_ptp_tx_hang(struct igb_adapter *adapter)
> +{
> +	bool timeout = time_is_before_jiffies(adapter->ptp_tx_start +
> +					      IGB_PTP_TX_TIMEOUT);
> +
> +	if (!adapter->ptp_tx_skb)
> +		return;
> +
> +	if (!test_bit(__IGB_PTP_TX_IN_PROGRESS, &adapter->state))
> +		return;
> +
> +	/* If we haven't received a timestamp within the timeout, it is
> +	 * reasonable to assume that it will never occur, so we can unlock the
> +	 * timestamp bit when this occurs.
> +	 */
> +	if (timeout) {
> +		cancel_work_sync(&adapter->ptp_tx_work);
> +		dev_kfree_skb_any(adapter->ptp_tx_skb);
> +		adapter->ptp_tx_skb = NULL;
> +		clear_bit_unlock(__IGB_PTP_TX_IN_PROGRESS, &adapter->state);
> +		adapter->tx_hwtstamp_timeouts++;
> +		dev_warn(&adapter->pdev->dev, "clearing Tx timestamp hang\n");
> +	}
> +}
> +
>  /**
>   * igb_ptp_tx_hwtstamp - utility function which checks for TX time stamp
>   * @adapter: Board private structure.
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> index eb36106218ad..dd5578756ae0 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> @@ -961,6 +961,7 @@ void ixgbe_ptp_suspend(struct ixgbe_adapter *adapter);
>  void ixgbe_ptp_stop(struct ixgbe_adapter *adapter);
>  void ixgbe_ptp_overflow_check(struct ixgbe_adapter *adapter);
>  void ixgbe_ptp_rx_hang(struct ixgbe_adapter *adapter);
> +void ixgbe_ptp_tx_hang(struct ixgbe_adapter *adapter);
>  void ixgbe_ptp_rx_pktstamp(struct ixgbe_q_vector *, struct sk_buff *);
>  void ixgbe_ptp_rx_rgtstamp(struct ixgbe_q_vector *, struct sk_buff *skb);
>  static inline void ixgbe_ptp_rx_hwtstamp(struct ixgbe_ring *rx_ring,
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index c61459fcc21e..f3199c73faed 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -7645,6 +7645,7 @@ static void ixgbe_service_task(struct work_struct *work)
>  	if (test_bit(__IXGBE_PTP_RUNNING, &adapter->state)) {
>  		ixgbe_ptp_overflow_check(adapter);
>  		ixgbe_ptp_rx_hang(adapter);
> +		ixgbe_ptp_tx_hang(adapter);
>  	}
>
>  	ixgbe_service_event_complete(adapter);
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
> index 9270e6f4fcff..24f4eaf37727 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
> @@ -662,6 +662,33 @@ static void ixgbe_ptp_clear_tx_timestamp(struct ixgbe_adapter *adapter)
>  	clear_bit_unlock(__IXGBE_PTP_TX_IN_PROGRESS, &adapter->state);
>  }
>
> +/**
> + * ixgbe_ptp_tx_hang - detect error case where Tx timestamp never finishes
> + * @adapter: private network adapter structure
> + */
> +void ixgbe_ptp_tx_hang(struct ixgbe_adapter *adapter)
> +{
> +	bool timeout = time_is_before_jiffies(adapter->ptp_tx_start +
> +					      IXGBE_PTP_TX_TIMEOUT);
> +
> +	if (!adapter->ptp_tx_skb)
> +		return;
> +
> +	if (!test_bit(__IXGBE_PTP_TX_IN_PROGRESS, &adapter->state))
> +		return;
> +
> +	/* If we haven't received a timestamp within the timeout, it is
> +	 * reasonable to assume that it will never occur, so we can unlock the
> +	 * timestamp bit when this occurs.
> +	 */
> +	if (timeout) {
> +		cancel_work_sync(&adapter->ptp_tx_work);
> +		ixgbe_ptp_clear_tx_timestamp(adapter);
> +		adapter->tx_hwtstamp_timeouts++;
> +		e_warn(drv, "clearing Tx timestamp hang\n");
> +	}
> +}
> +
>  /**
>   * ixgbe_ptp_tx_hwtstamp - utility function which checks for TX time stamp
>   * @adapter: the private adapter struct
>

[Intel-wired-lan] [PATCH v3 5/5] net-intel: check for Tx timestamp timeouts during watchdog

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Jacob Keller
> Sent: Friday, April 28, 2017 11:44 AM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: Richard Cochran <richardcochran@gmail.com>
> Subject: [Intel-wired-lan] [PATCH v3 5/5] net-intel: check for Tx timestamp
> timeouts during watchdog
> 
> Several drivers share similar logic for handling the Tx timestamps.
> These drivers have some parts which rely on the interrupt instead of only a
> polling work task. Although unlikely it may be possible in some circumstances
> for the PTP tx timestamp to never occur. If this happens, the result is that all
> future Tx timestamp requests will be ignored permanently.
> 
> Fix this by adding a *ptp_tx_hang() function similar to the already existing
> *ptp_rx_hang() routine which checks to make sure that the timestamp
> hasn't taken too long. This ensures that we will eventually recover from this
> case.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e.h        |  2 ++
>  drivers/net/ethernet/intel/i40e/i40e_main.c   |  1 +
>  drivers/net/ethernet/intel/i40e/i40e_ptp.c    | 30
> +++++++++++++++++++++++++++
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c   |  1 +
>  drivers/net/ethernet/intel/igb/igb.h          |  1 +
>  drivers/net/ethernet/intel/igb/igb_main.c     |  1 +
>  drivers/net/ethernet/intel/igb/igb_ptp.c      | 29
> ++++++++++++++++++++++++++
>  drivers/net/ethernet/intel/ixgbe/ixgbe.h      |  1 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  1 +
> drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c  | 27
> ++++++++++++++++++++++++
>  10 files changed, 94 insertions(+)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [PATCH v3 0/5] ptp cornercase cleanups

[Keller, Jacob E]

> -----Original Message-----
> From: Keller, Jacob E
> Sent: Friday, April 28, 2017 11:44 AM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: Richard Cochran <richardcochran@gmail.com>; David Mirabito
> <davidm@metamako.com>; Keller, Jacob E <jacob.e.keller@intel.com>
> Subject: [PATCH v3 0/5] ptp cornercase cleanups
> 
> This version fixes two minor issues I realized after I was porting them
> back to out-of-tree drivers. Sorry for the thrash.
> 
> Jacob Keller (5):
>   net-intel: fix race condition with PTP_TX_IN_PROGRESS bits
>   net-intel: avoid permanent lock of *_PTP_TX_IN_PROGRESS
>   net-intel: add statistic indicating number of skipped Tx timestamps
>   i40e: use pf data structure directly in i40e_ptp_rx_hang
>   net-intel: check for Tx timestamp timeouts during watchdog
> 
>  drivers/net/ethernet/intel/e1000e/e1000.h        |  1 +
>  drivers/net/ethernet/intel/e1000e/ethtool.c      |  1 +
>  drivers/net/ethernet/intel/e1000e/netdev.c       | 27 ++++++++-----
>  drivers/net/ethernet/intel/i40e/i40e.h           |  5 ++-
>  drivers/net/ethernet/intel/i40e/i40e_ethtool.c   |  1 +
>  drivers/net/ethernet/intel/i40e/i40e_main.c      |  3 +-
>  drivers/net/ethernet/intel/i40e/i40e_ptp.c       | 48 +++++++++++++++++++++--
> -
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c      | 28 +++++++++++---
>  drivers/net/ethernet/intel/igb/igb.h             |  2 +
>  drivers/net/ethernet/intel/igb/igb_ethtool.c     |  1 +
>  drivers/net/ethernet/intel/igb/igb_main.c        | 26 ++++++++++---
>  drivers/net/ethernet/intel/igb/igb_ptp.c         | 41 +++++++++++++++++++-
>  drivers/net/ethernet/intel/ixgbe/ixgbe.h         |  2 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c |  3 ++
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    | 42 ++++++++++++++-------
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c     | 42 +++++++++++++++++++--
>  16 files changed, 227 insertions(+), 46 deletions(-)
> 
> --
> 2.13.0.rc0.317.gcc792a6cad5a

Jeff, can you please drop this series? I'm about to send out an (identical content) v2 which splits the patches apart to each driver, and necessarily requires new subject names.

Thanks,
Jake