[Intel-wired-lan] [PATCH 0/4] igc: PTP tx fixes

[Intel-wired-lan] [PATCH 0/4] igc: PTP tx fixes

[Andre Guedes]

Hi all,

This patch series fixes some issues in the PTP tx code from the IGC driver.
The most relevant one is a race condition between igc_xmit_frame_ring() and
igc_ptp_tx_hang() which is fixed by the last patch.

- Andre

Andre Guedes (4):
  igc: Rename IGC_TSYNCTXCTL_VALID macro
  igc: Don't reschedule ptp_tx work
  igc: Remove timeout check from ptp_tx work
  igc: Fix race condition in PTP tx code

 drivers/net/ethernet/intel/igc/igc.h         |  5 +-
 drivers/net/ethernet/intel/igc/igc_defines.h |  2 +-
 drivers/net/ethernet/intel/igc/igc_main.c    |  7 ++-
 drivers/net/ethernet/intel/igc/igc_ptp.c     | 60 +++++++++++---------
 4 files changed, 43 insertions(+), 31 deletions(-)

-- 
2.26.2

[Intel-wired-lan] [PATCH 1/4] igc: Rename IGC_TSYNCTXCTL_VALID macro

[Andre Guedes]

This patch renames the IGC_TSYNCTXCTL_VALID macro to IGC_TSYNCTXCTL_
TXTT_0 so it matches the datasheet.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_defines.h | 2 +-
 drivers/net/ethernet/intel/igc/igc_ptp.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index 1c032c1ce4d3..28885be15ee8 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -409,7 +409,7 @@
 #define IGC_IMIREXT_SIZE_BP	0x00001000  /* Packet size bypass */
 
 /* Time Sync Transmit Control bit definitions */
-#define IGC_TSYNCTXCTL_VALID			0x00000001  /* Tx timestamp valid */
+#define IGC_TSYNCTXCTL_TXTT_0			0x00000001  /* Tx timestamp reg 0 valid */
 #define IGC_TSYNCTXCTL_ENABLED			0x00000010  /* enable Tx timestamping */
 #define IGC_TSYNCTXCTL_MAX_ALLOWED_DLY_MASK	0x0000F000  /* max delay */
 #define IGC_TSYNCTXCTL_SYNC_COMP_ERR		0x20000000  /* sync err */
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index e67d4655b47e..79802fb4ea83 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -410,7 +410,7 @@ static void igc_ptp_tx_work(struct work_struct *work)
 	}
 
 	tsynctxctl = rd32(IGC_TSYNCTXCTL);
-	if (tsynctxctl & IGC_TSYNCTXCTL_VALID)
+	if (tsynctxctl & IGC_TSYNCTXCTL_TXTT_0)
 		igc_ptp_tx_hwtstamp(adapter);
 	else
 		/* reschedule to check later */
-- 
2.26.2

[Intel-wired-lan] [PATCH 1/4] igc: Rename IGC_TSYNCTXCTL_VALID macro

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Tuesday, July 28, 2020 4:38 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 1/4] igc: Rename IGC_TSYNCTXCTL_VALID
> macro
> 
> This patch renames the IGC_TSYNCTXCTL_VALID macro to IGC_TSYNCTXCTL_
> TXTT_0 so it matches the datasheet.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
Tested-by: Aaron Brown <aaron.f.brown@intel.com>

[Intel-wired-lan] [PATCH 2/4] igc: Don't reschedule ptp_tx work

[Andre Guedes]

The ptp_tx work is scheduled only if TSICR.TXTS bit is set, therefore
TSYNCTXCTL.TXTT_0 bit is expected to be set when we check it igc_ptp_tx_
work(). If it isn't, something is really off and rescheduling the ptp_tx
work to check it later doesn't help much. This patch changes the code to
WARN_ON_ONCE() if this situation ever happens.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_ptp.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index 79802fb4ea83..b912830ea7bb 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -410,11 +410,10 @@ static void igc_ptp_tx_work(struct work_struct *work)
 	}
 
 	tsynctxctl = rd32(IGC_TSYNCTXCTL);
-	if (tsynctxctl & IGC_TSYNCTXCTL_TXTT_0)
-		igc_ptp_tx_hwtstamp(adapter);
-	else
-		/* reschedule to check later */
-		schedule_work(&adapter->ptp_tx_work);
+	if (WARN_ON_ONCE(!(tsynctxctl & IGC_TSYNCTXCTL_TXTT_0)))
+		return;
+
+	igc_ptp_tx_hwtstamp(adapter);
 }
 
 /**
-- 
2.26.2

[Intel-wired-lan] [PATCH 2/4] igc: Don't reschedule ptp_tx work

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Tuesday, July 28, 2020 4:38 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 2/4] igc: Don't reschedule ptp_tx work
> 
> The ptp_tx work is scheduled only if TSICR.TXTS bit is set, therefore
> TSYNCTXCTL.TXTT_0 bit is expected to be set when we check it igc_ptp_tx_
> work(). If it isn't, something is really off and rescheduling the ptp_tx
> work to check it later doesn't help much. This patch changes the code to
> WARN_ON_ONCE() if this situation ever happens.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
Tested-by: Aaron Brown <aaron.f.brown@intel.com>

[Intel-wired-lan] [PATCH 3/4] igc: Remove timeout check from ptp_tx work

[Andre Guedes]

The tx timestamp timeout is already checked by the watchdog_task work
which runs periodically. In addition to that, from the ptp_tx work
perspective, if __IGC_PTP_TX_IN_PROGRESS flag is set we always want
handle the timestamp stored in hardware and update the skb. So this
patch removes the timeout check in igc_ptp_tx_work() function.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_ptp.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index b912830ea7bb..22b2cf80b22f 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -403,12 +403,6 @@ static void igc_ptp_tx_work(struct work_struct *work)
 	if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
 		return;
 
-	if (time_is_before_jiffies(adapter->ptp_tx_start +
-				   IGC_PTP_TX_TIMEOUT)) {
-		igc_ptp_tx_timeout(adapter);
-		return;
-	}
-
 	tsynctxctl = rd32(IGC_TSYNCTXCTL);
 	if (WARN_ON_ONCE(!(tsynctxctl & IGC_TSYNCTXCTL_TXTT_0)))
 		return;
-- 
2.26.2

[Intel-wired-lan] [PATCH 3/4] igc: Remove timeout check from ptp_tx work

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Tuesday, July 28, 2020 4:38 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 3/4] igc: Remove timeout check from
> ptp_tx work
> 
> The tx timestamp timeout is already checked by the watchdog_task work
> which runs periodically. In addition to that, from the ptp_tx work
> perspective, if __IGC_PTP_TX_IN_PROGRESS flag is set we always want
> handle the timestamp stored in hardware and update the skb. So this
> patch removes the timeout check in igc_ptp_tx_work() function.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_ptp.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
Tested-by: Aaron Brown <aaron.f.brown@intel.com>

[Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in PTP tx code

[Andre Guedes]

Currently, the igc driver supports timestamping only one tx packet at a
time. During the transmission flow, the skb that requires hardware
timestamping is saved in adapter->ptp_tx_skb. Once hardware has the
timestamp, an interrupt is delivered, and adapter->ptp_tx_work is
scheduled. In igc_ptp_tx_work(), we read the timestamp register, update
adapter->ptp_tx_skb, and notify the network stack.

While the thread executing the transmission flow (the user process
running in kernel mode) and the thread executing ptp_tx_work don't
access adapter->ptp_tx_skb concurrently, there are two other places
where adapter->ptp_tx_skb is accessed: igc_ptp_tx_hang() and
igc_ptp_suspend().

igc_ptp_tx_hang() is executed by the adapter->watchdog_task worker
thread which runs periodically so it is possible we have two threads
accessing ptp_tx_skb at the same time. Consider the following scenario:
right after __IGC_PTP_TX_IN_PROGRESS is set in igc_xmit_frame_ring(),
igc_ptp_tx_hang() is executed. Since adapter->ptp_tx_start hasn't been
written yet, this is considered a timeout and adapter->ptp_tx_skb is
cleaned up.

This patch fixes the issue described above by adding the ptp_tx_lock to
protect access to ptp_tx_skb and ptp_tx_start fields from igc_adapter.
Since igc_xmit_frame_ring() called in atomic context by the networking
stack, ptp_tx_lock is defined as a spinlock.

With the introduction of the ptp_tx_lock, the __IGC_PTP_TX_IN_PROGRESS
flag doesn't provide much of a use anymore so this patch gets rid of it.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h      |  5 ++-
 drivers/net/ethernet/intel/igc/igc_main.c |  7 +++-
 drivers/net/ethernet/intel/igc/igc_ptp.c  | 49 ++++++++++++++---------
 3 files changed, 40 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 3070dfdb7eb4..19f88f705ec3 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -207,6 +207,10 @@ struct igc_adapter {
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info ptp_caps;
 	struct work_struct ptp_tx_work;
+	/* Access to ptp_tx_skb and ptp_tx_start is protected by the
+	 * ptp_tx_lock.
+	 */
+	spinlock_t ptp_tx_lock;
 	struct sk_buff *ptp_tx_skb;
 	struct hwtstamp_config tstamp_config;
 	unsigned long ptp_tx_start;
@@ -361,7 +365,6 @@ enum igc_state_t {
 	__IGC_TESTING,
 	__IGC_RESETTING,
 	__IGC_DOWN,
-	__IGC_PTP_TX_IN_PROGRESS,
 };
 
 enum igc_tx_flags {
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index ab5b302d6655..d6b37d91cad9 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1347,13 +1347,14 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
 	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
 		struct igc_adapter *adapter = netdev_priv(tx_ring->netdev);
 
+		spin_lock(&adapter->ptp_tx_lock);
+
 		/* FIXME: add support for retrieving timestamps from
 		 * the other timer registers before skipping the
 		 * timestamping request.
 		 */
 		if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON &&
-		    !test_and_set_bit_lock(__IGC_PTP_TX_IN_PROGRESS,
-					   &adapter->state)) {
+		    !adapter->ptp_tx_skb) {
 			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 			tx_flags |= IGC_TX_FLAGS_TSTAMP;
 
@@ -1362,6 +1363,8 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
 		} else {
 			adapter->tx_hwtstamp_skipped++;
 		}
+
+		spin_unlock(&adapter->ptp_tx_lock);
 	}
 
 	/* record initial flags and protocol */
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index 22b2cf80b22f..3dda5bd05cc6 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -320,35 +320,35 @@ static int igc_ptp_set_timestamp_mode(struct igc_adapter *adapter,
 	return 0;
 }
 
+/* Requires adapter->ptp_tx_lock held by caller. */
 static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
 {
 	struct igc_hw *hw = &adapter->hw;
 
 	dev_kfree_skb_any(adapter->ptp_tx_skb);
 	adapter->ptp_tx_skb = NULL;
+	adapter->ptp_tx_start = 0;
 	adapter->tx_hwtstamp_timeouts++;
-	clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
 	/* Clear the tx valid bit in TSYNCTXCTL register to enable interrupt. */
 	rd32(IGC_TXSTMPH);
+
 	netdev_warn(adapter->netdev, "Tx timestamp timeout\n");
 }
 
 void igc_ptp_tx_hang(struct igc_adapter *adapter)
 {
-	bool timeout = time_is_before_jiffies(adapter->ptp_tx_start +
-					      IGC_PTP_TX_TIMEOUT);
+	spin_lock(&adapter->ptp_tx_lock);
 
-	if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
-		return;
+	if (!adapter->ptp_tx_skb)
+		goto unlock;
 
-	/* 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);
-		igc_ptp_tx_timeout(adapter);
-	}
+	if (time_is_after_jiffies(adapter->ptp_tx_start + IGC_PTP_TX_TIMEOUT))
+		goto unlock;
+
+	igc_ptp_tx_timeout(adapter);
+
+unlock:
+	spin_unlock(&adapter->ptp_tx_lock);
 }
 
 /**
@@ -358,6 +358,8 @@ void igc_ptp_tx_hang(struct igc_adapter *adapter)
  * 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.
+ *
+ * Context: Expects adapter->ptp_tx_lock to be held by caller.
  */
 static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
 {
@@ -379,7 +381,7 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
 	 * while we're notifying the stack.
 	 */
 	adapter->ptp_tx_skb = NULL;
-	clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
+	adapter->ptp_tx_start = 0;
 
 	/* Notify the stack and free the skb after we've unlocked */
 	skb_tstamp_tx(skb, &shhwtstamps);
@@ -400,14 +402,19 @@ static void igc_ptp_tx_work(struct work_struct *work)
 	struct igc_hw *hw = &adapter->hw;
 	u32 tsynctxctl;
 
-	if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
-		return;
+	spin_lock(&adapter->ptp_tx_lock);
+
+	if (!adapter->ptp_tx_skb)
+		goto unlock;
 
 	tsynctxctl = rd32(IGC_TSYNCTXCTL);
 	if (WARN_ON_ONCE(!(tsynctxctl & IGC_TSYNCTXCTL_TXTT_0)))
-		return;
+		goto unlock;
 
 	igc_ptp_tx_hwtstamp(adapter);
+
+unlock:
+	spin_unlock(&adapter->ptp_tx_lock);
 }
 
 /**
@@ -484,6 +491,7 @@ void igc_ptp_init(struct igc_adapter *adapter)
 	}
 
 	spin_lock_init(&adapter->tmreg_lock);
+	spin_lock_init(&adapter->ptp_tx_lock);
 	INIT_WORK(&adapter->ptp_tx_work, igc_ptp_tx_work);
 
 	adapter->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
@@ -515,9 +523,14 @@ void igc_ptp_suspend(struct igc_adapter *adapter)
 		return;
 
 	cancel_work_sync(&adapter->ptp_tx_work);
+
+	spin_lock(&adapter->ptp_tx_lock);
+
 	dev_kfree_skb_any(adapter->ptp_tx_skb);
 	adapter->ptp_tx_skb = NULL;
-	clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
+	adapter->ptp_tx_start = 0;
+
+	spin_unlock(&adapter->ptp_tx_lock);
 }
 
 /**
-- 
2.26.2

[Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in PTP tx code

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Tuesday, July 28, 2020 4:38 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in PTP tx code
> 
> Currently, the igc driver supports timestamping only one tx packet at a
> time. During the transmission flow, the skb that requires hardware
> timestamping is saved in adapter->ptp_tx_skb. Once hardware has the
> timestamp, an interrupt is delivered, and adapter->ptp_tx_work is
> scheduled. In igc_ptp_tx_work(), we read the timestamp register, update
> adapter->ptp_tx_skb, and notify the network stack.
> 
> While the thread executing the transmission flow (the user process
> running in kernel mode) and the thread executing ptp_tx_work don't
> access adapter->ptp_tx_skb concurrently, there are two other places
> where adapter->ptp_tx_skb is accessed: igc_ptp_tx_hang() and
> igc_ptp_suspend().
> 
> igc_ptp_tx_hang() is executed by the adapter->watchdog_task worker
> thread which runs periodically so it is possible we have two threads
> accessing ptp_tx_skb at the same time. Consider the following scenario:
> right after __IGC_PTP_TX_IN_PROGRESS is set in igc_xmit_frame_ring(),
> igc_ptp_tx_hang() is executed. Since adapter->ptp_tx_start hasn't been
> written yet, this is considered a timeout and adapter->ptp_tx_skb is
> cleaned up.
> 
> This patch fixes the issue described above by adding the ptp_tx_lock to
> protect access to ptp_tx_skb and ptp_tx_start fields from igc_adapter.
> Since igc_xmit_frame_ring() called in atomic context by the networking
> stack, ptp_tx_lock is defined as a spinlock.
> 
> With the introduction of the ptp_tx_lock, the __IGC_PTP_TX_IN_PROGRESS
> flag doesn't provide much of a use anymore so this patch gets rid of it.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc.h      |  5 ++-
>  drivers/net/ethernet/intel/igc/igc_main.c |  7 +++-
>  drivers/net/ethernet/intel/igc/igc_ptp.c  | 49 ++++++++++++++---------
>  3 files changed, 40 insertions(+), 21 deletions(-)
> 
Tested-by: Aaron Brown <aaron.f.brown@intel.com>

[Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in PTP tx code

[Andre Guedes]

Hi Jeff/Tony,

Quoting Brown, Aaron F (2020-08-13 20:09:42)
> > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> > Andre Guedes
> > Sent: Tuesday, July 28, 2020 4:38 PM
> > To: intel-wired-lan at lists.osuosl.org
> > Subject: [Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in PTP tx code
> > 
> > Currently, the igc driver supports timestamping only one tx packet at a
> > time. During the transmission flow, the skb that requires hardware
> > timestamping is saved in adapter->ptp_tx_skb. Once hardware has the
> > timestamp, an interrupt is delivered, and adapter->ptp_tx_work is
> > scheduled. In igc_ptp_tx_work(), we read the timestamp register, update
> > adapter->ptp_tx_skb, and notify the network stack.
> > 
> > While the thread executing the transmission flow (the user process
> > running in kernel mode) and the thread executing ptp_tx_work don't
> > access adapter->ptp_tx_skb concurrently, there are two other places
> > where adapter->ptp_tx_skb is accessed: igc_ptp_tx_hang() and
> > igc_ptp_suspend().
> > 
> > igc_ptp_tx_hang() is executed by the adapter->watchdog_task worker
> > thread which runs periodically so it is possible we have two threads
> > accessing ptp_tx_skb at the same time. Consider the following scenario:
> > right after __IGC_PTP_TX_IN_PROGRESS is set in igc_xmit_frame_ring(),
> > igc_ptp_tx_hang() is executed. Since adapter->ptp_tx_start hasn't been
> > written yet, this is considered a timeout and adapter->ptp_tx_skb is
> > cleaned up.
> > 
> > This patch fixes the issue described above by adding the ptp_tx_lock to
> > protect access to ptp_tx_skb and ptp_tx_start fields from igc_adapter.
> > Since igc_xmit_frame_ring() called in atomic context by the networking
> > stack, ptp_tx_lock is defined as a spinlock.
> > 
> > With the introduction of the ptp_tx_lock, the __IGC_PTP_TX_IN_PROGRESS
> > flag doesn't provide much of a use anymore so this patch gets rid of it.
> > 
> > Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> > ---
> >  drivers/net/ethernet/intel/igc/igc.h      |  5 ++-
> >  drivers/net/ethernet/intel/igc/igc_main.c |  7 +++-
> >  drivers/net/ethernet/intel/igc/igc_ptp.c  | 49 ++++++++++++++---------
> >  3 files changed, 40 insertions(+), 21 deletions(-)
> > 
> Tested-by: Aaron Brown <aaron.f.brown@intel.com>

Please hold this patch back. I think I found an issue with it. I'm
investigating it and should send a v2 soon.

Cheers,
Andre

[Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in PTP tx code

[Nguyen, Anthony L]

On Fri, 2020-08-21 at 18:24 -0700, Andre Guedes wrote:
> Hi Jeff/Tony,
> 
> Quoting Brown, Aaron F (2020-08-13 20:09:42)
> > > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On
> > > Behalf Of
> > > Andre Guedes
> > > Sent: Tuesday, July 28, 2020 4:38 PM
> > > To: intel-wired-lan at lists.osuosl.org
> > > Subject: [Intel-wired-lan] [PATCH 4/4] igc: Fix race condition in
> > > PTP tx code
> > > 
> > > Currently, the igc driver supports timestamping only one tx
> > > packet at a
> > > time. During the transmission flow, the skb that requires
> > > hardware
> > > timestamping is saved in adapter->ptp_tx_skb. Once hardware has
> > > the
> > > timestamp, an interrupt is delivered, and adapter->ptp_tx_work is
> > > scheduled. In igc_ptp_tx_work(), we read the timestamp register,
> > > update
> > > adapter->ptp_tx_skb, and notify the network stack.
> > > 
> > > While the thread executing the transmission flow (the user
> > > process
> > > running in kernel mode) and the thread executing ptp_tx_work
> > > don't
> > > access adapter->ptp_tx_skb concurrently, there are two other
> > > places
> > > where adapter->ptp_tx_skb is accessed: igc_ptp_tx_hang() and
> > > igc_ptp_suspend().
> > > 
> > > igc_ptp_tx_hang() is executed by the adapter->watchdog_task
> > > worker
> > > thread which runs periodically so it is possible we have two
> > > threads
> > > accessing ptp_tx_skb at the same time. Consider the following
> > > scenario:
> > > right after __IGC_PTP_TX_IN_PROGRESS is set in
> > > igc_xmit_frame_ring(),
> > > igc_ptp_tx_hang() is executed. Since adapter->ptp_tx_start hasn't
> > > been
> > > written yet, this is considered a timeout and adapter->ptp_tx_skb 
> > > is
> > > cleaned up.
> > > 
> > > This patch fixes the issue described above by adding the
> > > ptp_tx_lock to
> > > protect access to ptp_tx_skb and ptp_tx_start fields from
> > > igc_adapter.
> > > Since igc_xmit_frame_ring() called in atomic context by the
> > > networking
> > > stack, ptp_tx_lock is defined as a spinlock.
> > > 
> > > With the introduction of the ptp_tx_lock, the
> > > __IGC_PTP_TX_IN_PROGRESS
> > > flag doesn't provide much of a use anymore so this patch gets rid
> > > of it.
> > > 
> > > Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> > > ---
> > >  drivers/net/ethernet/intel/igc/igc.h      |  5 ++-
> > >  drivers/net/ethernet/intel/igc/igc_main.c |  7 +++-
> > >  drivers/net/ethernet/intel/igc/igc_ptp.c  | 49 ++++++++++++++---
> > > ------
> > >  3 files changed, 40 insertions(+), 21 deletions(-)
> > > 
> > 
> > Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> 
> Please hold this patch back. I think I found an issue with it. I'm
> investigating it and should send a v2 soon.

Thanks Andre, I've dropped this patch from dev-queue.