[PATCH iwl-net 0/2] igb: PTP Tx timestamp state fixes

[PATCH iwl-net 0/2] igb: PTP Tx timestamp state fixes

[Shivani Gupta]

The igb driver keeps a single outstanding Tx hardware timestamp
request in adapter->ptp_tx_skb, guarded only by the atomic
__IGB_PTP_TX_IN_PROGRESS bit. The fields around it (ptp_tx_skb,
ptp_tx_start) are accessed from the transmit path, the retrieval
worker, the watchdog and the teardown paths without any common
synchronization.

Patch 1 ports the fix igc received in commit 9c50e2b150c8 ("igc: Fix
race condition in PTP tx code") - which the igc series cover letter
already announced as intended for igb - to igb's single-timestamp
model: a new ptp_tx_lock protects the timestamp request state and is
initialized during software setup, before register_netdev() exposes any
consumer. The state bit is removed, and the unsafe cancel_work_sync()
calls in the watchdog path (deadlock with the new lock) and the
atomic-context transmit error path (sleeping while atomic) are dropped
in favor of the worker checking the request state under the lock.
igb-specific paths that do not exist in igc (the 82576 polling worker
with its timeout branch, and transmit error cleanup that verifies slot
ownership) are covered as well.

Patch 2 fixes a related lifecycle bug: disabling Tx timestamping with
a request outstanding strands the request. On re-enable, all new
requests are dropped until the stale one is flagged as a bogus
"Tx timestamp hang" by the watchdog up to 15 seconds later. Pending
requests are now dropped at disable time. The disabled mode and empty
slot are published atomically under the lock introduced by patch 1, so
the transmit path cannot admit a replacement request during cleanup
(which is why the two patches form one series).

Both issues were validated on QEMU's igb (82576) model. The unpatched
disable/re-enable blackout reproduces deterministically (recovery only
via the 15 s watchdog timeout); the final patches complete 100 cycles
without a blackout. A 60-second timestamp stress test with link flaps
delivered 6445 of 6469 requests and completed without a KASAN, lockdep,
DEBUG_ATOMIC_SLEEP, warning, or panic report. The race window itself is
nanoseconds wide and was not directly reproduced; it is identical in
structure to the igc bug fixed by 9c50e2b150c8.

Note: this series touches the same schedule_work() call sites as Kurt
Kanzenbach's pending "[PATCH iwl-next v5] igb: Retrieve Tx timestamp
from BH workqueue" (20260305-igb_irq_ts-v5-1-d3b96828ab5b@linutronix.de).
The overlap is textual only - that patch changes which workqueue runs
ptp_tx_work, this series changes the locking around the request state -
and either rebases trivially on the other.

Shivani Gupta (2):
  igb: Fix race condition in PTP tx code
  igb: Clear pending Tx timestamp requests when disabling Tx
    timestamping

 drivers/net/ethernet/intel/igb/igb.h      |   3 +-
 drivers/net/ethernet/intel/igb/igb_main.c |  49 +++++++-----
 drivers/net/ethernet/intel/igb/igb_ptp.c  | 120 ++++++++++++++++++++----------
 3 files changed, 113 insertions(+), 59 deletions(-)


base-commit: 24ef02f934eeb48830cff6b739abc3c62b1d107b

[PATCH iwl-net 1/2] igb: Fix race condition in PTP tx code

[Shivani Gupta]

igb stores one outstanding hardware Tx timestamp request in ptp_tx_skb.
The state bit used for admission is atomic, but ptp_tx_skb and
ptp_tx_start are accessed without common synchronization by the
transmit path, timestamp worker, watchdog, error cleanup, and suspend
path.

The watchdog can run after the transmit path sets the state bit and
stores the skb but before it refreshes ptp_tx_start. If the previous
request is older than IGB_PTP_TX_TIMEOUT, the watchdog treats the new
request as timed out and releases its timestamp reference. A later
hardware latch can then be associated with another request.

Add ptp_tx_lock and use it for all accesses to the request pointer and
start time. Initialize the lock during software setup, before
register_netdev() can expose the watchdog and transmit paths. The
pointer itself now provides admission control, so remove
__IGB_PTP_TX_IN_PROGRESS.

The worker and watchdog share a lock-held timeout helper. The watchdog
no longer calls cancel_work_sync(), which would deadlock while holding
the new lock and is unnecessary because the worker rechecks the pointer
under that lock.

Also remove cancel_work_sync() from the ndo_start_xmit error path, where
sleeping is not allowed. Clear the slot only if it still belongs to the
skb whose transmit failed, so a completed request cannot make error
cleanup discard a replacement request. Suspend retains synchronous
cancellation outside the lock before freeing the pending reference.

This follows the locking model introduced for igc by commit
9c50e2b150c8 ("igc: Fix race condition in PTP tx code"), with the
additional 82576 polling and transmit-error paths required by igb.

Fixes: e5f36ad14c93 ("igb: check for Tx timestamp timeouts during watchdog")
Fixes: 74344e32fcc0 ("igb: avoid permanent lock of *_PTP_TX_IN_PROGRESS")
Signed-off-by: Shivani Gupta <shivani07g@gmail.com>
---
 drivers/net/ethernet/intel/igb/igb.h      |   3 +-
 drivers/net/ethernet/intel/igb/igb_main.c |  25 ++++--
 drivers/net/ethernet/intel/igb/igb_ptp.c  | 104 +++++++++++++---------
 3 files changed, 83 insertions(+), 49 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 0fff1df81b7b..ecd284f51d3e 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -625,6 +625,8 @@ struct igb_adapter {
 	struct ptp_clock_info ptp_caps;
 	struct delayed_work ptp_overflow_work;
 	struct work_struct ptp_tx_work;
+	/* Access to ptp_tx_skb and ptp_tx_start is protected by ptp_tx_lock. */
+	spinlock_t ptp_tx_lock;
 	struct sk_buff *ptp_tx_skb;
 	struct kernel_hwtstamp_config tstamp_config;
 	unsigned long ptp_tx_start;
@@ -714,7 +716,6 @@ enum e1000_state_t {
 	__IGB_TESTING,
 	__IGB_RESETTING,
 	__IGB_DOWN,
-	__IGB_PTP_TX_IN_PROGRESS,
 };
 
 enum igb_boards {
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index a1e89a375744..fc70f7aa4ce0 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -4062,6 +4062,8 @@ static int igb_sw_init(struct igb_adapter *adapter)
 	adapter->max_frame_size = netdev->mtu + IGB_ETH_PKT_HDR_PAD;
 	adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
 
+	/* The netdev watchdog can run as soon as the device is registered. */
+	spin_lock_init(&adapter->ptp_tx_lock);
 	spin_lock_init(&adapter->nfc_lock);
 	spin_lock_init(&adapter->stats64_lock);
 
@@ -6561,10 +6563,11 @@ netdev_tx_t igb_xmit_frame_ring(struct sk_buff *skb,
 
 	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
 		struct igb_adapter *adapter = netdev_priv(tx_ring->netdev);
+		unsigned long flags;
 
+		spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
 		if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON &&
-		    !test_and_set_bit_lock(__IGB_PTP_TX_IN_PROGRESS,
-					   &adapter->state)) {
+		    !adapter->ptp_tx_skb) {
 			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 			tx_flags |= IGB_TX_FLAGS_TSTAMP;
 
@@ -6575,6 +6578,7 @@ netdev_tx_t igb_xmit_frame_ring(struct sk_buff *skb,
 		} else {
 			adapter->tx_hwtstamp_skipped++;
 		}
+		spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
 	}
 
 	if (skb_vlan_tag_present(skb)) {
@@ -6603,12 +6607,19 @@ netdev_tx_t igb_xmit_frame_ring(struct sk_buff *skb,
 cleanup_tx_tstamp:
 	if (unlikely(tx_flags & IGB_TX_FLAGS_TSTAMP)) {
 		struct igb_adapter *adapter = netdev_priv(tx_ring->netdev);
+		unsigned long flags;
 
-		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);
+		/* ndo_start_xmit runs in atomic context, so the scheduled
+		 * ptp_tx_work cannot be cancelled here. It checks
+		 * ptp_tx_skb under ptp_tx_lock and does nothing once the
+		 * pending timestamp request is cleared.
+		 */
+		spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
+		if (adapter->ptp_tx_skb == skb) {
+			dev_kfree_skb_any(adapter->ptp_tx_skb);
+			adapter->ptp_tx_skb = NULL;
+		}
+		spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
 	}
 
 	return NETDEV_TX_OK;
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 638d8242b66b..3cc05f9198f8 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -796,6 +796,21 @@ static int igb_ptp_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
 	return 0;
 }
 
+/* Requires adapter->ptp_tx_lock held by caller. */
+static void igb_ptp_tx_timeout(struct igb_adapter *adapter)
+{
+	struct e1000_hw *hw = &adapter->hw;
+
+	dev_kfree_skb_any(adapter->ptp_tx_skb);
+	adapter->ptp_tx_skb = NULL;
+	adapter->tx_hwtstamp_timeouts++;
+	/* Clear the tx valid bit in TSYNCTXCTL register to enable
+	 * interrupt
+	 */
+	rd32(E1000_TXSTMPH);
+	dev_warn(&adapter->pdev->dev, "clearing Tx timestamp hang\n");
+}
+
 /**
  * igb_ptp_tx_work
  * @work: pointer to work struct
@@ -808,23 +823,18 @@ static void igb_ptp_tx_work(struct work_struct *work)
 	struct igb_adapter *adapter = container_of(work, struct igb_adapter,
 						   ptp_tx_work);
 	struct e1000_hw *hw = &adapter->hw;
+	unsigned long flags;
 	u32 tsynctxctl;
 
+	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
+
 	if (!adapter->ptp_tx_skb)
-		return;
+		goto unlock;
 
 	if (time_is_before_jiffies(adapter->ptp_tx_start +
 				   IGB_PTP_TX_TIMEOUT)) {
-		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++;
-		/* Clear the tx valid bit in TSYNCTXCTL register to enable
-		 * interrupt
-		 */
-		rd32(E1000_TXSTMPH);
-		dev_warn(&adapter->pdev->dev, "clearing Tx timestamp hang\n");
-		return;
+		igb_ptp_tx_timeout(adapter);
+		goto unlock;
 	}
 
 	tsynctxctl = rd32(E1000_TSYNCTXCTL);
@@ -833,6 +843,9 @@ static void igb_ptp_tx_work(struct work_struct *work)
 	else
 		/* reschedule to check later */
 		schedule_work(&adapter->ptp_tx_work);
+
+unlock:
+	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
 }
 
 static void igb_ptp_overflow_check(struct work_struct *work)
@@ -897,32 +910,45 @@ void igb_ptp_rx_hang(struct igb_adapter *adapter)
  */
 void igb_ptp_tx_hang(struct igb_adapter *adapter)
 {
-	struct e1000_hw *hw = &adapter->hw;
-	bool timeout = time_is_before_jiffies(adapter->ptp_tx_start +
-					      IGB_PTP_TX_TIMEOUT);
+	unsigned long flags;
+
+	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
 
 	if (!adapter->ptp_tx_skb)
-		return;
+		goto unlock;
 
-	if (!test_bit(__IGB_PTP_TX_IN_PROGRESS, &adapter->state))
-		return;
+	if (time_is_after_jiffies(adapter->ptp_tx_start + IGB_PTP_TX_TIMEOUT))
+		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);
-		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++;
-		/* Clear the tx valid bit in TSYNCTXCTL register to enable
-		 * interrupt
-		 */
-		rd32(E1000_TXSTMPH);
-		dev_warn(&adapter->pdev->dev, "clearing Tx timestamp hang\n");
-	}
+	igb_ptp_tx_timeout(adapter);
+
+unlock:
+	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
+}
+
+/**
+ * igb_ptp_clear_tx_tstamp - drop a pending Tx timestamp request
+ * @adapter: private network adapter structure
+ *
+ * Cancel the timestamp retrieval work and free a pending timestamp skb.
+ *
+ * Context: Must be called in sleepable context with ptp_tx_lock not held;
+ *          cancel_work_sync() waits for igb_ptp_tx_work() which takes it.
+ */
+static void igb_ptp_clear_tx_tstamp(struct igb_adapter *adapter)
+{
+	unsigned long flags;
+
+	cancel_work_sync(&adapter->ptp_tx_work);
+
+	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
+	dev_kfree_skb_any(adapter->ptp_tx_skb);
+	adapter->ptp_tx_skb = NULL;
+	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
 }
 
 /**
@@ -932,6 +958,8 @@ void igb_ptp_tx_hang(struct igb_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 igb_ptp_tx_hwtstamp(struct igb_adapter *adapter)
 {
@@ -963,15 +991,14 @@ static void igb_ptp_tx_hwtstamp(struct igb_adapter *adapter)
 	shhwtstamps.hwtstamp =
 		ktime_add_ns(shhwtstamps.hwtstamp, adjust);
 
-	/* 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.
+	/* Clear the pending request before calling skb_tstamp_tx so that
+	 * a new timestamp request can be accepted. 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 */
+	/* Notify the stack and free the skb */
 	skb_tstamp_tx(skb, &shhwtstamps);
 	dev_kfree_skb_any(skb);
 }
@@ -1434,12 +1461,7 @@ void igb_ptp_suspend(struct igb_adapter *adapter)
 	if (adapter->ptp_flags & IGB_PTP_OVERFLOW_CHECK)
 		cancel_delayed_work_sync(&adapter->ptp_overflow_work);
 
-	cancel_work_sync(&adapter->ptp_tx_work);
-	if (adapter->ptp_tx_skb) {
-		dev_kfree_skb_any(adapter->ptp_tx_skb);
-		adapter->ptp_tx_skb = NULL;
-		clear_bit_unlock(__IGB_PTP_TX_IN_PROGRESS, &adapter->state);
-	}
+	igb_ptp_clear_tx_tstamp(adapter);
 }
 
 /**

[PATCH iwl-net 2/2] igb: Clear pending Tx timestamp requests when disabling Tx timestamping

[Shivani Gupta]

Disabling Tx hardware timestamping clears TSYNCTXCTL.EN but leaves an
outstanding request in ptp_tx_skb. Re-enabling within
IGB_PTP_TX_TIMEOUT therefore finds the single timestamp slot occupied
and skips new requests until the watchdog reports a misleading timeout,
producing a blackout of up to 15 seconds.

Cancel timestamp retrieval when disabling Tx timestamping, then publish
HWTSTAMP_TX_OFF and release the pending request under ptp_tx_lock. The
transmit path tests both the configured mode and slot ownership under
the same lock, so no request can be admitted between clearing the slot
and publishing the disabled state. Updates to the saved configuration
and the watchdog mode check use that lock as well.

Suspend uses the same cleanup helper without changing the saved mode,
allowing reset to restore the requested configuration after resume.

Only count a timestamp request as skipped when hardware timestamping is
enabled and the slot is occupied. Requests carrying SKBTX_HW_TSTAMP
while the feature is disabled could never have been serviced and are
ignored without changing the counter.

Fixes: 1f6e8178d685 ("igb: Prevent dropped Tx timestamps via work items and interrupts.")
Signed-off-by: Shivani Gupta <shivani07g@gmail.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 26 +++++++++++++----------
 drivers/net/ethernet/intel/igb/igb_ptp.c  | 20 +++++++++++++++--
 2 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index fc70f7aa4ce0..42204e3307d6 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6565,18 +6565,22 @@ netdev_tx_t igb_xmit_frame_ring(struct sk_buff *skb,
 		struct igb_adapter *adapter = netdev_priv(tx_ring->netdev);
 		unsigned long flags;
 
+		/* A timestamp that was requested while Tx timestamping was
+		 * not enabled can never be delivered, it is not "skipped".
+		 */
 		spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
-		if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON &&
-		    !adapter->ptp_tx_skb) {
-			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
-			tx_flags |= IGB_TX_FLAGS_TSTAMP;
-
-			adapter->ptp_tx_skb = skb_get(skb);
-			adapter->ptp_tx_start = jiffies;
-			if (adapter->hw.mac.type == e1000_82576)
-				schedule_work(&adapter->ptp_tx_work);
-		} else {
-			adapter->tx_hwtstamp_skipped++;
+		if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON) {
+			if (!adapter->ptp_tx_skb) {
+				skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+				tx_flags |= IGB_TX_FLAGS_TSTAMP;
+
+				adapter->ptp_tx_skb = skb_get(skb);
+				adapter->ptp_tx_start = jiffies;
+				if (adapter->hw.mac.type == e1000_82576)
+					schedule_work(&adapter->ptp_tx_work);
+			} else {
+				adapter->tx_hwtstamp_skipped++;
+			}
 		}
 		spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
 	}
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 3cc05f9198f8..adc7be6a2ee2 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -914,6 +914,9 @@ void igb_ptp_tx_hang(struct igb_adapter *adapter)
 
 	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
 
+	if (adapter->tstamp_config.tx_type != HWTSTAMP_TX_ON)
+		goto unlock;
+
 	if (!adapter->ptp_tx_skb)
 		goto unlock;
 
@@ -933,19 +936,22 @@ void igb_ptp_tx_hang(struct igb_adapter *adapter)
 /**
  * igb_ptp_clear_tx_tstamp - drop a pending Tx timestamp request
  * @adapter: private network adapter structure
+ * @disable: whether to disable Tx timestamp admission
  *
  * Cancel the timestamp retrieval work and free a pending timestamp skb.
  *
  * Context: Must be called in sleepable context with ptp_tx_lock not held;
  *          cancel_work_sync() waits for igb_ptp_tx_work() which takes it.
  */
-static void igb_ptp_clear_tx_tstamp(struct igb_adapter *adapter)
+static void igb_ptp_clear_tx_tstamp(struct igb_adapter *adapter, bool disable)
 {
 	unsigned long flags;
 
 	cancel_work_sync(&adapter->ptp_tx_work);
 
 	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
+	if (disable)
+		adapter->tstamp_config.tx_type = HWTSTAMP_TX_OFF;
 	dev_kfree_skb_any(adapter->ptp_tx_skb);
 	adapter->ptp_tx_skb = NULL;
 	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
@@ -1250,6 +1256,13 @@ static int igb_ptp_set_timestamp_mode(struct igb_adapter *adapter,
 	regval |= tsync_tx_ctl;
 	wr32(E1000_TSYNCTXCTL, regval);
 
+	/* Drop a possibly pending Tx timestamp request when disabling Tx
+	 * timestamping. It would otherwise block new requests until it is
+	 * flagged as timed out by the watchdog up to 15 seconds later.
+	 */
+	if (!tsync_tx_ctl)
+		igb_ptp_clear_tx_tstamp(adapter, true);
+
 	/* enable/disable RX */
 	regval = rd32(E1000_TSYNCRXCTL);
 	regval &= ~(E1000_TSYNCRXCTL_ENABLED | E1000_TSYNCRXCTL_TYPE_MASK);
@@ -1310,6 +1323,7 @@ int igb_ptp_hwtstamp_set(struct net_device *netdev,
 			 struct netlink_ext_ack *extack)
 {
 	struct igb_adapter *adapter = netdev_priv(netdev);
+	unsigned long flags;
 	int err;
 
 	err = igb_ptp_set_timestamp_mode(adapter, config);
@@ -1317,7 +1331,9 @@ int igb_ptp_hwtstamp_set(struct net_device *netdev,
 		return err;
 
 	/* save these settings for future reference */
+	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
 	adapter->tstamp_config = *config;
+	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
 
 	return 0;
 }
@@ -1461,7 +1477,7 @@ void igb_ptp_suspend(struct igb_adapter *adapter)
 	if (adapter->ptp_flags & IGB_PTP_OVERFLOW_CHECK)
 		cancel_delayed_work_sync(&adapter->ptp_overflow_work);
 
-	igb_ptp_clear_tx_tstamp(adapter);
+	igb_ptp_clear_tx_tstamp(adapter, false);
 }
 
 /**

Re: [PATCH iwl-net 1/2] igb: Fix race condition in PTP tx code

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260815010827.91912-2-shivani07g@gmail.com?part=1
---
--- Patch [1]: [PATCH iwl-net 1/2] igb: Fix race condition in PTP tx code ---
commit fe4808a653156f9a6aff1ed75f8c696e88ed8a49
Author: Shivani Gupta <shivani07g@gmail.com>

igb: Fix race condition in PTP tx code

This adds a new adapter->ptp_tx_lock spinlock covering ptp_tx_skb and
ptp_tx_start, removes the __IGB_PTP_TX_IN_PROGRESS state bit so that the
pointer itself provides admission control, and factors the timeout handling
used by the worker and the watchdog into igb_ptp_tx_timeout().  It follows
the model already used by igc, plus the 82576 polling and transmit-error
paths specific to igb.

A few questions below.

> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> index 0fff1df81b7bd..ecd284f51d3e3 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -625,6 +625,8 @@ struct igb_adapter {
>  	struct ptp_clock_info ptp_caps;
>  	struct delayed_work ptp_overflow_work;
>  	struct work_struct ptp_tx_work;
> +	/* Access to ptp_tx_skb and ptp_tx_start is protected by ptp_tx_lock. */
> +	spinlock_t ptp_tx_lock;
>  	struct sk_buff *ptp_tx_skb;
>  	struct kernel_hwtstamp_config tstamp_config;
>  	unsigned long ptp_tx_start;

[Severity: Low]
[Finding: gpt-5-6-sol-5-11]
[Sources: gpt-5-6-sol, opus-5]
This isn't a bug introduced by this patch, but should the lock scope comment
also mention tstamp_config?

igb_xmit_frame_ring() now reads tstamp_config.tx_type inside the new
ptp_tx_lock section, while with this patch alone the writers,
igb_ptp_hwtstamp_set() and igb_ptp_init(), still update tstamp_config
without the lock.

The follow-up patch in the series, "igb: Clear pending Tx timestamping
requests when disabling Tx timestamping", publishes adapter->tstamp_config
under ptp_tx_lock and drops an already admitted request through
igb_ptp_clear_tx_tstamp(adapter, true), so the functional part is handled
there; only the comment here still lists just ptp_tx_skb and ptp_tx_start.

igb_ptp_hwtstamp_get() also copies the shadow config without the lock, which
looks fine since it is rtnl serialized against the setter.

> @@ -714,7 +716,6 @@ enum e1000_state_t {
>  	__IGB_TESTING,
>  	__IGB_RESETTING,
>  	__IGB_DOWN,
> -	__IGB_PTP_TX_IN_PROGRESS,
>  };
>  
>  enum igb_boards {
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index a1e89a375744c..fc70f7aa4ce05 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c

[ ... ]

> @@ -6561,10 +6563,11 @@ netdev_tx_t igb_xmit_frame_ring(struct sk_buff *skb,
>  
>  	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
>  		struct igb_adapter *adapter = netdev_priv(tx_ring->netdev);
> +		unsigned long flags;
>  
> +		spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
>  		if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON &&
> -		    !test_and_set_bit_lock(__IGB_PTP_TX_IN_PROGRESS,
> -					   &adapter->state)) {
> +		    !adapter->ptp_tx_skb) {
>  			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
>  			tx_flags |= IGB_TX_FLAGS_TSTAMP;
>  

[ ... ]

> diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
> index 638d8242b66bb..3cc05f9198f85 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> @@ -796,6 +796,21 @@ static int igb_ptp_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
>  	return 0;
>  }
>  
> +/* Requires adapter->ptp_tx_lock held by caller. */
> +static void igb_ptp_tx_timeout(struct igb_adapter *adapter)
> +{
> +	struct e1000_hw *hw = &adapter->hw;
> +
> +	dev_kfree_skb_any(adapter->ptp_tx_skb);
> +	adapter->ptp_tx_skb = NULL;
> +	adapter->tx_hwtstamp_timeouts++;
> +	/* Clear the tx valid bit in TSYNCTXCTL register to enable
> +	 * interrupt
> +	 */
> +	rd32(E1000_TXSTMPH);
> +	dev_warn(&adapter->pdev->dev, "clearing Tx timestamp hang\n");
> +}
> +
>  /**
>   * igb_ptp_tx_work
>   * @work: pointer to work struct
> @@ -808,23 +823,18 @@ static void igb_ptp_tx_work(struct work_struct *work)
>  	struct igb_adapter *adapter = container_of(work, struct igb_adapter,
>  						   ptp_tx_work);
>  	struct e1000_hw *hw = &adapter->hw;
> +	unsigned long flags;
>  	u32 tsynctxctl;
>  
> +	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
> +
>  	if (!adapter->ptp_tx_skb)
> -		return;
> +		goto unlock;
>  
>  	if (time_is_before_jiffies(adapter->ptp_tx_start +
>  				   IGB_PTP_TX_TIMEOUT)) {
> -		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++;
> -		/* Clear the tx valid bit in TSYNCTXCTL register to enable
> -		 * interrupt
> -		 */
> -		rd32(E1000_TXSTMPH);
> -		dev_warn(&adapter->pdev->dev, "clearing Tx timestamp hang\n");
> -		return;
> +		igb_ptp_tx_timeout(adapter);
> +		goto unlock;
>  	}
>  
>  	tsynctxctl = rd32(E1000_TSYNCTXCTL);
> @@ -833,6 +843,9 @@ static void igb_ptp_tx_work(struct work_struct *work)
>  	else
>  		/* reschedule to check later */
>  		schedule_work(&adapter->ptp_tx_work);
> +
> +unlock:
> +	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
>  }
>  
>  static void igb_ptp_overflow_check(struct work_struct *work)
> @@ -897,32 +910,45 @@ void igb_ptp_rx_hang(struct igb_adapter *adapter)
>   */
>  void igb_ptp_tx_hang(struct igb_adapter *adapter)
>  {
> -	struct e1000_hw *hw = &adapter->hw;
> -	bool timeout = time_is_before_jiffies(adapter->ptp_tx_start +
> -					      IGB_PTP_TX_TIMEOUT);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
>  
>  	if (!adapter->ptp_tx_skb)
> -		return;
> +		goto unlock;
>  
> -	if (!test_bit(__IGB_PTP_TX_IN_PROGRESS, &adapter->state))
> -		return;
> +	if (time_is_after_jiffies(adapter->ptp_tx_start + IGB_PTP_TX_TIMEOUT))
> +		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.
>  	 */
                       ^^^^^^^^^^^^^^^^^

[Severity: Low]
[Finding: gpt-5-6-sol-2-3]
[Sources: gpt-5-6-sol]
This isn't a bug, but the retained sentence in igb_ptp_tx_hang() still talks
about unlocking "the timestamp bit", and this patch deletes
__IGB_PTP_TX_IN_PROGRESS from enum e1000_state_t.  The timeout path now
releases admission in igb_ptp_tx_timeout() with:

	dev_kfree_skb_any(adapter->ptp_tx_skb);
	adapter->ptp_tx_skb = NULL;

Would it read better as clearing the pending timestamp request?
igc_ptp_tx_hang() dropped this sentence when the same locking model landed
there.  The phrase can also be read as the hardware Tx valid bit, which
igb_ptp_tx_timeout() still clears via rd32(E1000_TXSTMPH), so the wording is
ambiguous either way.

> -	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++;
> -		/* Clear the tx valid bit in TSYNCTXCTL register to enable
> -		 * interrupt
> -		 */
> -		rd32(E1000_TXSTMPH);
> -		dev_warn(&adapter->pdev->dev, "clearing Tx timestamp hang\n");
> -	}
> +	igb_ptp_tx_timeout(adapter);
> +
> +unlock:
> +	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
> +}
> +
> +/**
> + * igb_ptp_clear_tx_tstamp - drop a pending Tx timestamp request
> + * @adapter: private network adapter structure
> + *
> + * Cancel the timestamp retrieval work and free a pending timestamp skb.
> + *
> + * Context: Must be called in sleepable context with ptp_tx_lock not held;
> + *          cancel_work_sync() waits for igb_ptp_tx_work() which takes it.
> + */
> +static void igb_ptp_clear_tx_tstamp(struct igb_adapter *adapter)
> +{
> +	unsigned long flags;
> +
> +	cancel_work_sync(&adapter->ptp_tx_work);
> +
> +	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
> +	dev_kfree_skb_any(adapter->ptp_tx_skb);
> +	adapter->ptp_tx_skb = NULL;
> +	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
>  }

[Severity: High]
[Finding: gpt-5-6-sol-4-6]
[Sources: gpt-5-6-sol, opus-5]
This isn't a bug introduced by this patch, but the new kernel-doc presents
this cancel_work_sync() as the teardown guarantee, and it remains the only
synchronous cancellation of ptp_tx_work in the driver.  Can ptp_tx_work be
queued again after it has run?

igb_remove() reaches it early:

igb_remove()
    igb_ptp_stop() -> igb_ptp_suspend() -> igb_ptp_clear_tx_tstamp()
    ...
    unregister_netdev(netdev);
    ...
    pci_iounmap(pdev, adapter->io_addr);
    ...
    free_netdev(netdev);

The netdev stays registered until unregister_netdev(), tstamp_config.tx_type
is left at HWTSTAMP_TX_ON by this path, and igb_xmit_frame_ring() has no
__IGB_DOWN gate (only igb_maybe_stop_tx() and IGB_RING_FLAG_TX_DISABLED), so
a timestamped frame in that window still does:

	adapter->ptp_tx_skb = skb_get(skb);
	adapter->ptp_tx_start = jiffies;
	if (adapter->hw.mac.type == e1000_82576)
		schedule_work(&adapter->ptp_tx_work);

and for the other parts igb_tsync_interrupt() does the same:

	if (tsicr & E1000_TSICR_TXTS) {
		/* retrieve hardware timestamp */
		schedule_work(&adapter->ptp_tx_work);
	}

Once queued with a pending request, igb_ptp_tx_work() re-arms itself while
TSYNCTXCTL.VALID is clear, for up to IGB_PTP_TX_TIMEOUT (15 * HZ):

	tsynctxctl = rd32(E1000_TSYNCTXCTL);
	if (tsynctxctl & E1000_TSYNCTXCTL_VALID)
		igb_ptp_tx_hwtstamp(adapter);
	else
		/* reschedule to check later */
		schedule_work(&adapter->ptp_tx_work);

Nothing in the rest of igb_remove() cancels or flushes ptp_tx_work, so can
the worker end up doing rd32(E1000_TSYNCTXCTL) after pci_iounmap(), and
taking spin_lock_irqsave(&adapter->ptp_tx_lock) plus reading
adapter->ptp_tx_skb after free_netdev() freed the adapter that contains both
the work_struct and the lock?

The pre-patch code has the same window, and the later patches in the series
do not appear to close it either, since igb_ptp_suspend() leaves
tstamp_config.tx_type untouched and no extra cancel or flush is added.

>  
>  /**
> @@ -932,6 +958,8 @@ void igb_ptp_tx_hang(struct igb_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 igb_ptp_tx_hwtstamp(struct igb_adapter *adapter)
>  {
> @@ -963,15 +991,14 @@ static void igb_ptp_tx_hwtstamp(struct igb_adapter *adapter)
>  	shhwtstamps.hwtstamp =
>  		ktime_add_ns(shhwtstamps.hwtstamp, adjust);
>  
> -	/* 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.
> +	/* Clear the pending request before calling skb_tstamp_tx so that
> +	 * a new timestamp request can be accepted. 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;

[Severity: Low]
[Finding: main-1-0]
[Sources: opus-5]
This isn't a bug, but does this comment still match the new locking model?
It sits under the Context line added just above the function, which says
ptp_tx_lock is held by the caller.

The only caller is igb_ptp_tx_work(), which holds ptp_tx_lock with interrupts
disabled across igb_ptp_tx_hwtstamp(), so also across skb_tstamp_tx() and
dev_kfree_skb_any().  igb_xmit_frame_ring() has to take the same lock to
install a new ptp_tx_skb.

That means no new request can be accepted here, and no other thread can
change the pointer, until the caller unlocks.  Would it be clearer to drop
both of those rationales?

>  
> -	/* Notify the stack and free the skb after we've unlocked */
> +	/* Notify the stack and free the skb */
>  	skb_tstamp_tx(skb, &shhwtstamps);
>  	dev_kfree_skb_any(skb);
>  }
> @@ -1434,12 +1461,7 @@ void igb_ptp_suspend(struct igb_adapter *adapter)
>  	if (adapter->ptp_flags & IGB_PTP_OVERFLOW_CHECK)
>  		cancel_delayed_work_sync(&adapter->ptp_overflow_work);
>  
> -	cancel_work_sync(&adapter->ptp_tx_work);
> -	if (adapter->ptp_tx_skb) {
> -		dev_kfree_skb_any(adapter->ptp_tx_skb);
> -		adapter->ptp_tx_skb = NULL;
> -		clear_bit_unlock(__IGB_PTP_TX_IN_PROGRESS, &adapter->state);
> -	}
> +	igb_ptp_clear_tx_tstamp(adapter);
>  }