[PATCH iwl-net 00/12] ice: E82x: timestamp processing logic fixes

[PATCH iwl-net 00/12] ice: E82x: timestamp processing logic fixes

[Jacob Keller]

This series contains several related fixes for the ice driver PTP logic
relating to timestamp handling and device (re)initialization.

Of particular note is some changes around the handling of timestamps that
are requested near device state changes such as administrative up/down
cycles and link change events.

The E825 device logic in the PHY has an internal counter which is used as
part of the "threshold" logic which determines when the device will trigger
an interrupt signal from a given PHY port to the MAC. This internal counter
requires some precise handling to ensure that the internal device state
remains in sync with software expectations. Otherwise, the device can be
finagled into a state where the PHY stops producing new timestamp interrupt
notifications to the MAC indefinitely. This in turn degrades the timestamp
processing latency and results in application failures for common
timestamping applications such as ptp4l.

There are four major categories of problem resolved by this series:

 * Timestamp requests made while the PHY_REG_TX_OFFSET_READY bit is cleared
   will increment the internal counter, but leave their valid bit set to 0.
   Upon read, the counter is not decremented. This leads to a desync of the
   counter and blocks the PHY interrupt.

 * Software logic for tracking timestamps incorrectly cleared in-use bits
   without waiting for completion in certain cases. If the timestamp *does*
   later complete, it leaves an "orphaned" ready bit which is not
   tracked by software. This results in the internal counter becoming
   desynced if that index is re-used.

 * The PHY timestamp memory region lacks pull-down zero-initialization at
   power on, resulting in uninitialized random data in the memory region.
   If software reads these values, an entry with its valid bit set to 1 can
   trigger a counter decrement and cause an underflow which results in the
   counter becoming desynced.

 * Timestamp request which complete near the beginning of the PHY losing
   link can become "stuck" such that the hardware logic triggered by a
   timestamp read does not activate. The memory status bit and the valid
   bit in the the timestamp index are not cleared. This window where this
   may occur begins *before* the firmware notifies the driver of link loss.
   When this occurs, the driver may accidentally re-use a stale timestamp,
   and the IRQ re-trigger logic triggers a repeated IRQ "storm" that can
   consume significant excess CPU time.

The series' primary focus is towards preventing driver flows that can
trigger the above sequences. It is based on work from Przemyslaw Korba which
was previously posted at [1]. During that series development, Petr from
RedHat reported the 3rd issue mentioned above. While attempting to root
cause that issue, several other issues were uncovered and those fixes have
also been included in this series.

First, the locking around the PTP ports list in the adapter structure is
converted to use RCU primitives and a spinlock, resolving a couple of
reports from Petr about places where the original list was accessed without
lock protection. Note that an older version of this fix used an xarray
instead of the list. The xarray has more overhead and results in an
increase of ~25 microseconds to the average latency for processing Tx
timestamps. The list is simpler and avoids this overhead.

Next, come two fixes for E822 hardware that were originally posted as part
of Przemyslaw Korba's work [1]. The E822-only "vernier" offset validation
work task is properly canceled during device reset, and new timestamp
requests are kept disabled until the validation task completes.

Next, Arkadiusz modifies the driver to stop pretending that the link has
gone down during ice_down(). This removes "virtual" PTP link changes that
occurred on several flows including MTU change, Eswitch setup, and others.
Now, the driver only triggers a PTP PHY timer reinitialization when the
physical PHY link has changed instead of during many other actions.

Next, the driver is modified to stop clearing the PHY_REG_TX_OFFSET_READY
bit. This bits only purpose is to tell hardware to mark any captured
timestamps as invalid. Since this also disables the necessary side effects
on read it is problematic to have cleared. According to hardware engineers,
keeping it enabled should not have any other side effects.

Next, the driver is modified to clear the PHY_REG_TX_MEMORY_STATUS by
reading each index *prior* to the PHY soft reset. This ensures that any
stale or invalid data left in the memory array is cleared, followed by the
counter being reset via the PHY soft reset procedure.

Next, the E825 timer start procedure is modified to first include a soft
reset. This ensures that upon link up the device is reconfigured from a
known-good state with its internal counter reset and everything cleared.

Next, Petr modifies the ice_ptp_flush_tx_tracker() function to wait a little
bit for any outstanding timestamps before flushing.

Next, Petr modifies the ice_ptp_process_tx_tstamp() function to avoid
releasing any index from software unless either a) it is actually completed
by hardware or b) it is timed out waiting for a full two seconds. This
closes the final gap from the second issue mentioned above. Instead of
immediately releasing the index, the software now waits until hardware has
completed it or the driver has waited long enough to be sufficiently sure
that no such timestamp will be done.

Next, the driver is modified to no longer mark timestamps as "stale" during
a clock adjust event. This avoids marking timestamps as stale unnecessarily.

Finally, the ice_ptp_process_tx_tstamp() function is modified to verify
that hardware actually cleared the ready bitmap. This ensures that we do
not report false timestamps near a link down event.

Link: [1] https://lore.kernel.org/intel-wired-lan/20260720120151.2675206-1-przemyslaw.korba@intel.com/
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Arkadiusz Kubalewski (1):
      ice: call PTP link change only from link events

Jacob Keller (7):
      ice: use reference counting and RCU for PTP port access
      ice: E825: stop clearing PHY_REG_TX_OFFSET_READY
      ice: E825: clear PHY_REG_TX_MEMORY_STATUS prior to soft reset
      ice: E825: perform a soft reset when starting the PHY timer
      ice: remove unnecessary discarding of timestamps after clock adjust
      ice: skip reading Tx ready bitmap on ports with no timestamps
      ice: don't clear in_use until HW clears ready bitmap

Karol Kolacinski (2):
      ice: E822: keep Tx timestamps disabled during offset calibration
      ice: E822: cancel offset verification work during reset preparation

Petr Oros (2):
      ice: wait for in-flight Tx timestamps before flushing the tracker
      ice: keep Tx timestamp slots tracked until completion or timeout

 drivers/net/ethernet/intel/ice/ice_adapter.h |   6 +-
 drivers/net/ethernet/intel/ice/ice_ptp.h     |  12 +-
 drivers/net/ethernet/intel/ice/ice_adapter.c |   7 +-
 drivers/net/ethernet/intel/ice/ice_main.c    |  13 +-
 drivers/net/ethernet/intel/ice/ice_ptp.c     | 348 ++++++++++++++++++---------
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c  | 105 ++++----
 6 files changed, 318 insertions(+), 173 deletions(-)
---
base-commit: 564973a259ec76f2dad0853420e7034cc43994c4
change-id: 20260819-jk-e825c-minimized-fixes-8559addec172

Best regards,
--  
Jacob Keller <jacob.e.keller@intel.com>

[PATCH iwl-net 01/12] ice: use reference counting and RCU for PTP port access

[Jacob Keller]

The ice adapter structure maintains a list of ports associated with the
adapter. This is used for supporting PTP, where the clock owner must handle
many operations that require access to the PTP port structures of the
associated PFs.

This is implemented using a linked list and a mutex. This sort of works,
but a few places within the code do not acquire the mutex when iterating
the list. This includes ice_ptp_flush_all_tx_tracker(),
ice_ptp_restart_all_phy(), and ice_ptp_prepare_rebuild_sec().

Fixing this is tricky, especially since it is not clear if we can simply
acquire the lock around the complete iterations.

The pattern of use for the port list is read-mostly with modifications only
happening during PF initialization when elements are inserted. This
typically only happens during early boot, though a PF could in principle be
removed or loaded at arbitrary times via bind and unbind operations.

The use of a mutex does mean the driver can sleep while holding it, but it
still creates complicates with lock ordering and prevents iterating the
list in any code path that *can't* sleep.

Instead, use the RCU primitives for the port linked list, along with a
reference count on the port. The kref reference counter ensures that we can
safely acquire pointers with a guarantee of their lifetime, ensuring the
associated PF will not be removed until the reference is released.

For port iterations which are short and definitely can't sleep, wrap the
entire loop with rcu_read_lock() and rcu_read_unlock().

For longer operations, or those which might sleep, we need to close the
critical section between each loop iteration. To make this safe, start
the loop iteration with rcu_read_lock(), then acquire a reference for the
port with kref_get_unless_zero. If this returns 0, the port is already in
the process of being removed, so that port should be skipped when
iterating. Once a reference to the port is acquired, exit the RCU critical
section. Then, perform the desired operations on the port, followed by
releasing the reference with kref_put and then re-entering a critical
section at the end of the loop body. Note that kref_put() is done outside
the RCU critical section. This is safe, as the port will not be freed until
all references are dropped.

The ice_ptp_release_port_rcu() function is used as the release function for
the kref_put() call. To avoid a potential infinite loop of new references,
the release function simply uses a wake_up_var() call to wake the closing
thread. The ice_ptp_cleanup_pf() function will remove the port from the linked
list using list_del_rcu, then release its primary reference, then wait for all
references to drop via wait queue. Finally synchronize_rcu() is called to
guarantee the port remains valid for at least one RCU grace period. Then PF
removal will continue.

This flow ensures that all accesses to ports via the port list will remain
valid until either the RCU critical sections end, or the references have
been dropped.

One major complication of this reference count is that ice_ptp_port is
embedded inside of other structures and not merely allocated. As a result,
we can't use the standard pattern of kfree_rcu() to just delay freeing
until references are dropped, and instead are delaying PF port teardown. If
any code path leaks the reference, the driver will be unable to teardown.
Instead, a 15 second timeout with a WARN() is used when waiting to finally
allow PF teardown to continue. This has the risk of potentially allowing
use-after-free, assuming some path really is stuck for 15 seconds. However,
this both less likely and a less bad outcome compared to blocking
indefinitely on a reference leak.

Fixes: e800654e85b5 ("ice: Use ice_adapter for PTP shared data instead of auxdev")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_adapter.h |   6 +-
 drivers/net/ethernet/intel/ice/ice_ptp.h     |   4 +
 drivers/net/ethernet/intel/ice/ice_adapter.c |   7 +-
 drivers/net/ethernet/intel/ice/ice_ptp.c     | 118 +++++++++++++++++++--------
 4 files changed, 96 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
index 4f695f32da3d..39923dedd534 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.h
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
@@ -19,13 +19,13 @@ struct ice_pf;
  *
  * This structure contains data used to maintain a list of adapter ports
  *
- * @ports: list of ports
+ * @list: list of ports
  * @lock: protect access to the ports list
  */
 struct ice_port_list {
-	struct list_head ports;
+	struct list_head list;
 	/* To synchronize the ports list operations */
-	struct mutex lock;
+	spinlock_t lock;
 };
 
 /**
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index c4b0da7ce20e..da2003ba3bb0 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -5,6 +5,8 @@
 #define _ICE_PTP_H_
 
 #include <linux/ptp_clock_kernel.h>
+#include <linux/rculist.h>
+#include <linux/kref.h>
 #include <linux/kthread.h>
 
 #include "ice_ptp_hw.h"
@@ -138,6 +140,7 @@ struct ice_ptp_tx {
  * and determine when the port's PHY offset is valid.
  *
  * @list_node: list member structure
+ * @ref: reference counter for use with adapter ports list
  * @tx: Tx timestamp tracking for this port
  * @ov_work: delayed work task for tracking when PHY offset is valid
  * @ps_lock: mutex used to protect the overall PTP PHY start procedure
@@ -149,6 +152,7 @@ struct ice_ptp_tx {
  */
 struct ice_ptp_port {
 	struct list_head list_node;
+	struct kref ref;
 	struct ice_ptp_tx tx;
 	struct kthread_delayed_work ov_work;
 	struct mutex ps_lock; /* protects overall PTP PHY start procedure */
diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
index 2dc3629d6d0f..d1643bf8a1b5 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.c
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
@@ -66,18 +66,17 @@ static struct ice_adapter *ice_adapter_new(struct pci_dev *pdev)
 		mutex_init(&adapter->cpi_phy_lock[i]);
 	refcount_set(&adapter->refcount, 1);
 
-	mutex_init(&adapter->ports.lock);
-	INIT_LIST_HEAD(&adapter->ports.ports);
+	spin_lock_init(&adapter->ports.lock);
+	INIT_LIST_HEAD(&adapter->ports.list);
 
 	return adapter;
 }
 
 static void ice_adapter_free(struct ice_adapter *adapter)
 {
-	WARN_ON(!list_empty(&adapter->ports.ports));
+	WARN_ON(!list_empty(&adapter->ports.list));
 	for (int i = 0; i < ARRAY_SIZE(adapter->cpi_phy_lock); i++)
 		mutex_destroy(&adapter->cpi_phy_lock[i]);
-	mutex_destroy(&adapter->ports.lock);
 
 	kfree(adapter);
 }
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index eaec36ab6ae3..1d647ccce7c4 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (C) 2021, Intel Corporation. */
 
+#include <linux/rculist.h>
+#include <linux/wait_bit.h>
 #include "ice.h"
 #include "ice_lib.h"
 #include "ice_trace.h"
@@ -673,20 +675,33 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
 	pf->ptp.tx_hwtstamp_good += tstamp_good;
 }
 
+static void ice_ptp_release_port_rcu(struct kref *ref)
+{
+	wake_up_var(ref);
+}
+
 static void ice_ptp_tx_tstamp_owner(struct ice_pf *pf)
 {
 	struct ice_ptp_port *port;
 
-	mutex_lock(&pf->adapter->ports.lock);
-	list_for_each_entry(port, &pf->adapter->ports.ports, list_node) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(port, &pf->adapter->ports.list, list_node) {
 		struct ice_ptp_tx *tx = &port->tx;
 
-		if (!tx || !tx->init)
+		if (!tx->init)
 			continue;
 
+		if (!kref_get_unless_zero(&port->ref))
+			continue;
+
+		rcu_read_unlock();
+
 		ice_ptp_process_tx_tstamp(tx);
+
+		kref_put(&port->ref, ice_ptp_release_port_rcu);
+		rcu_read_lock();
 	}
-	mutex_unlock(&pf->adapter->ports.lock);
+	rcu_read_unlock();
 }
 
 /**
@@ -808,8 +823,16 @@ ice_ptp_flush_all_tx_tracker(struct ice_pf *pf)
 {
 	struct ice_ptp_port *port;
 
-	list_for_each_entry(port, &pf->adapter->ports.ports, list_node)
+	rcu_read_lock();
+	list_for_each_entry_rcu(port, &pf->adapter->ports.list, list_node) {
+		if (!kref_get_unless_zero(&port->ref))
+			continue;
+		rcu_read_unlock();
 		ice_ptp_flush_tx_tracker(ptp_port_to_pf(port), &port->tx);
+		kref_put(&port->ref, ice_ptp_release_port_rcu);
+		rcu_read_lock();
+	}
+	rcu_read_unlock();
 }
 
 /**
@@ -1424,16 +1447,21 @@ static void ice_ptp_reset_phy_timestamping(struct ice_pf *pf)
  */
 static void ice_ptp_restart_all_phy(struct ice_pf *pf)
 {
-	struct list_head *entry;
+	struct ice_ptp_port *port;
 
-	list_for_each(entry, &pf->adapter->ports.ports) {
-		struct ice_ptp_port *port = list_entry(entry,
-						       struct ice_ptp_port,
-						       list_node);
+	rcu_read_lock();
+	list_for_each_entry_rcu(port, &pf->adapter->ports.list, list_node) {
+		if (!kref_get_unless_zero(&port->ref))
+			continue;
+		rcu_read_unlock();
 
 		if (port->link_up)
 			ice_ptp_port_phy_restart(port);
+
+		kref_put(&port->ref, ice_ptp_release_port_rcu);
+		rcu_read_lock();
 	}
+	rcu_read_unlock();
 }
 
 /**
@@ -2694,19 +2722,19 @@ static bool ice_port_has_timestamps(struct ice_ptp_tx *tx)
 
 static bool ice_any_port_has_timestamps(struct ice_pf *pf)
 {
+	bool have_tstamps = false;
 	struct ice_ptp_port *port;
 
-	scoped_guard(mutex, &pf->adapter->ports.lock) {
-		list_for_each_entry(port, &pf->adapter->ports.ports,
-				    list_node) {
-			struct ice_ptp_tx *tx = &port->tx;
-
-			if (ice_port_has_timestamps(tx))
-				return true;
+	rcu_read_lock();
+	list_for_each_entry_rcu(port, &pf->adapter->ports.list, list_node) {
+		if (ice_port_has_timestamps(&port->tx)) {
+			have_tstamps = true;
+			break;
 		}
 	}
+	rcu_read_unlock();
 
-	return false;
+	return have_tstamps;
 }
 
 bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf)
@@ -2890,14 +2918,16 @@ void ice_ptp_queue_work(struct ice_pf *pf)
 static void ice_ptp_prepare_rebuild_sec(struct ice_pf *pf, bool rebuild,
 					enum ice_reset_req reset_type)
 {
-	struct list_head *entry;
+	struct ice_ptp_port *port;
 
-	list_for_each(entry, &pf->adapter->ports.ports) {
-		struct ice_ptp_port *port = list_entry(entry,
-						       struct ice_ptp_port,
-						       list_node);
+	rcu_read_lock();
+	list_for_each_entry_rcu(port, &pf->adapter->ports.list, list_node) {
 		struct ice_pf *peer_pf = ptp_port_to_pf(port);
 
+		if (!kref_get_unless_zero(&port->ref))
+			continue;
+		rcu_read_unlock();
+
 		if (!ice_is_primary(&peer_pf->hw)) {
 			if (rebuild) {
 				/* TODO: When implementing rebuild=true:
@@ -2909,7 +2939,11 @@ static void ice_ptp_prepare_rebuild_sec(struct ice_pf *pf, bool rebuild,
 				ice_ptp_prepare_for_reset(peer_pf, reset_type);
 			}
 		}
+
+		kref_put(&port->ref, ice_ptp_release_port_rcu);
+		rcu_read_lock();
 	}
+	rcu_read_unlock();
 }
 
 /**
@@ -3086,11 +3120,11 @@ static int ice_ptp_setup_pf(struct ice_pf *pf)
 		return -ENODEV;
 
 	INIT_LIST_HEAD(&ptp->port.list_node);
-	mutex_lock(&pf->adapter->ports.lock);
+	kref_init(&ptp->port.ref);
 
-	list_add(&ptp->port.list_node,
-		 &pf->adapter->ports.ports);
-	mutex_unlock(&pf->adapter->ports.lock);
+	spin_lock(&pf->adapter->ports.lock);
+	list_add_rcu(&ptp->port.list_node, &pf->adapter->ports.list);
+	spin_unlock(&pf->adapter->ports.lock);
 
 	/* Seed the per-PHY Tx reference clock usage map for this port.
 	 * Only meaningful on E825 (other MAC types don't expose tx-clk
@@ -3113,12 +3147,32 @@ static int ice_ptp_setup_pf(struct ice_pf *pf)
 static void ice_ptp_cleanup_pf(struct ice_pf *pf)
 {
 	struct ice_ptp *ptp = &pf->ptp;
+	struct kref *ref;
 
-	if (pf->hw.mac_type != ICE_MAC_UNKNOWN) {
-		mutex_lock(&pf->adapter->ports.lock);
-		list_del(&ptp->port.list_node);
-		mutex_unlock(&pf->adapter->ports.lock);
-	}
+	if (pf->hw.mac_type == ICE_MAC_UNKNOWN)
+		return;
+
+	/* The PF cannot be removed until there are no more remaining
+	 * outstanding references to the PTP port. To make sure this is true,
+	 * first remove the port from the list, then drop the primary
+	 * reference this PF holds on the port. Once done, wait until all
+	 * existing references are dropped. Finally, synchronize_rcu() to
+	 * ensure that all RCU critical sections that might attempt to
+	 * dereference the port are finished.
+	 */
+
+	spin_lock(&pf->adapter->ports.lock);
+	list_del_rcu(&ptp->port.list_node);
+	spin_unlock(&pf->adapter->ports.lock);
+
+	ref = &ptp->port.ref;
+	kref_put(ref, ice_ptp_release_port_rcu);
+
+	dev_WARN_ONCE(ice_pf_to_dev(pf),
+		      !wait_var_event_timeout(ref, !kref_read(ref), 15 * HZ),
+		      "Timed out waiting for port references to release. Continuing to unload anyways.");
+
+	synchronize_rcu();
 }
 
 /**

-- 
2.55.0.814.gc42f45431d0f

[PATCH iwl-net 02/12] ice: E822: keep Tx timestamps disabled during offset calibration

[Jacob Keller]

From: Karol Kolacinski <karol.kolacinski@intel.com>

Do not clear the tx.calibrating flag immediately after starting the PHY
timer in ice_ptp_port_phy_restart(). Instead, keep Tx timestamps
disabled until the offset verification work (ice_ptp_wait_for_offsets)
has confirmed that both Tx and Rx PHY offsets are properly configured.

Previously, tx.calibrating was set to true, then immediately back to
false right after ice_start_phy_timer_e82x() returned. This allowed Tx
timestamp requests to be served during the window where offset
verification was still pending. Timestamps produced during this window
use uncalibrated PHY offsets and can produce incorrect values.

When ptp4l receives incorrect timestamps, it may reject them and wait
for the next sync interval (typically 1-2 seconds), compounding delays
during link cycling. This contributes to the time transmitter port
becoming unresponsive after repeated link down/up cycles.

Move the tx.calibrating = false to ice_ptp_wait_for_offsets(), after
both Tx and Rx offset configuration has completed successfully. This
ensures that Tx timestamps are only reported with properly calibrated
PHY offsets.

If ice_start_phy_timer_e82x() fails, restore calibrating to false on
the error path to prevent permanently disabling Tx timestamps when
ov_work is never queued.

Log a debug message while offset calibration is still pending, including
the specific Tx/Rx error codes to aid debugging stalled calibration.
This path is expected on every routine link-up: ov_work is first queued
with no delay and the vernier offset cannot be computed until at least
one packet has been transmitted, so the first several invocations
normally land here. Use dev_dbg() rather than a rate-limited warning to
avoid emitting KERN_WARNING on every link-up during normal operation.
Log a debug message when calibration completes successfully.

Fixes: 3a7496234d17 ("ice: implement basic E822 PTP support")
Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 36 ++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 1d647ccce7c4..9d99cbb42463 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -1156,6 +1156,7 @@ static int ice_ptp_check_tx_fifo(struct ice_ptp_port *port)
 static void ice_ptp_wait_for_offsets(struct kthread_work *work)
 {
 	struct ice_ptp_port *port;
+	unsigned long flags;
 	struct ice_pf *pf;
 	struct ice_hw *hw;
 	int tx_err;
@@ -1178,12 +1179,28 @@ static void ice_ptp_wait_for_offsets(struct kthread_work *work)
 		tx_err = ice_phy_cfg_tx_offset_e82x(hw, port->port_num);
 	rx_err = ice_phy_cfg_rx_offset_e82x(hw, port->port_num);
 	if (tx_err || rx_err) {
-		/* Tx and/or Rx offset not yet configured, try again later */
+		/* Tx and/or Rx offset not yet configured, try again later.
+		 * This is expected during normal link-up: the vernier offset
+		 * calibration cannot complete until at least one packet has
+		 * been transmitted, so the first retries routinely land here.
+		 */
+		dev_dbg(ice_pf_to_dev(pf),
+			"PTP offset not yet valid for port %u (tx_err=%d rx_err=%d)\n",
+			port->port_num, tx_err, rx_err);
 		kthread_queue_delayed_work(pf->ptp.kworker,
 					   &port->ov_work,
 					   msecs_to_jiffies(100));
 		return;
 	}
+
+	/* Tx and Rx offsets are now configured, enable Tx timestamps */
+	spin_lock_irqsave(&port->tx.lock, flags);
+	port->tx.calibrating = false;
+	spin_unlock_irqrestore(&port->tx.lock, flags);
+
+	dev_dbg(ice_pf_to_dev(pf),
+		"PTP offset valid for port %u, Tx timestamps enabled\n",
+		port->port_num);
 }
 
 /**
@@ -1266,13 +1283,20 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
 
 		/* Start the PHY timer in Vernier mode */
 		err = ice_start_phy_timer_e82x(hw, port);
-		if (err)
+		if (err) {
+			spin_lock_irqsave(&ptp_port->tx.lock, flags);
+			ptp_port->tx.calibrating = false;
+			spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
 			break;
+		}
 
-		/* Enable Tx timestamps right away */
-		spin_lock_irqsave(&ptp_port->tx.lock, flags);
-		ptp_port->tx.calibrating = false;
-		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
+		/* Do not clear calibrating flag here. Tx timestamps remain
+		 * disabled until ice_ptp_wait_for_offsets() has verified
+		 * that the Tx and Rx offset calibration has completed.
+		 * Clearing it here would allow Tx timestamps to be reported
+		 * before the PHY offset registers are configured, leading
+		 * to incorrect timestamp values.
+		 */
 
 		kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work,
 					   0);

-- 
2.55.0.814.gc42f45431d0f

[PATCH iwl-net 03/12] ice: E822: cancel offset verification work during reset preparation

[Jacob Keller]

From: Karol Kolacinski <karol.kolacinski@intel.com>

Cancel the offset verification delayed work (ov_work) during PTP reset
preparation to prevent it from running concurrently with the reset
sequence.

Without this, ice_ptp_wait_for_offsets() can execute during a reset,
find that ice_is_reset_in_progress() is true, and re-queue itself in a
tight loop. Meanwhile, the reset path in ice_ptp_rebuild_owner() calls
ice_ptp_port_phy_restart(), which starts a new ov_work. This results in
two ov_work instances running simultaneously, racing over the PHY offset
registers and the calibrating flag.

Cancel ov_work in ice_ptp_prepare_for_reset() alongside the existing
cancellation of the Tx tracker. The comment in the existing code already
documents that ov_work interference during reset is a concern but only
cancels it during ice_ptp_release().

Fixes: 4809671015a1 ("ice: Fix E810 PTP reset flow")
Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 9d99cbb42463..b8647a39db6d 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -2994,6 +2994,14 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
 	if (reset_type == ICE_RESET_PFR)
 		return;
 
+	/* Cancel the offset verification work for E82x before releasing the
+	 * Tx tracker. If ov_work is running during reset, it may issue
+	 * sideband queue commands that will fail or timeout, and may
+	 * reference state that is being torn down.
+	 */
+	if (hw->mac_type == ICE_MAC_GENERIC)
+		kthread_cancel_delayed_work_sync(&ptp->port.ov_work);
+
 	if (ice_pf_src_tmr_owned(pf) && hw->mac_type == ICE_MAC_GENERIC_3K_E825)
 		ice_ptp_prepare_rebuild_sec(pf, false, reset_type);
 

-- 
2.55.0.814.gc42f45431d0f

[PATCH iwl-net 04/12] ice: call PTP link change only from link events

[Jacob Keller]

From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

Remove redundant ice_ptp_link_change() calls from ice_up_complete() and
ice_down(). These duplicate the call already made from
ice_handle_link_event(), creating three problems:

1. Double initialization on link-up: ice_handle_link_event() calls
   ice_ptp_link_change(true), then ice_up_complete() calls it again.
   The second call re-enters ice_ptp_port_phy_restart(), re-setting the
   calibrating flag and restarting the PHY timer while the first
   invocation's offset verification work (ov_work) may still be running.

2. Premature cleanup on administrative down: ice_down() calls
   ice_ptp_link_change(false) during ifconfig down or reset preparation,
   even when the physical link is still up. This clears timestamp state
   unnecessarily and can interfere with ongoing PTP operations.

3. Ordering dependency: ice_down()/ice_up_complete() are called during
   reset sequences where PTP may not be fully initialized, creating
   edge cases with partially configured state.

The link event handler is the correct and sufficient place to drive PTP
link state changes, as it reflects actual physical link transitions. Remove
the calls of ice_ptp_link_change from the ice_down()/ice_up() flows.

Initialize the link_up in ice_ptp_init() and ensure that we check and
restore the link status at the end of the rebuild flow, ensuring that we
initialize the PHY timer appropriately after a reset.

Fixes: 6b1ff5d39228 ("ice: always call ice_ptp_link_change and make it void")
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
Signed-off-by: Petr Oros <poros@redhat.com>
Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 11 +++++++--
 drivers/net/ethernet/intel/ice/ice_ptp.c  | 38 +++++++++++++++++++++++--------
 2 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index d88835482d3a..bb631ae9e67d 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -6745,7 +6745,6 @@ static int ice_up_complete(struct ice_vsi *vsi)
 		ice_print_link_msg(vsi, true);
 		netif_tx_start_all_queues(vsi->netdev);
 		netif_carrier_on(vsi->netdev);
-		ice_ptp_link_change(pf, true);
 	}
 
 	/* Perform an initial read of the statistics registers now to
@@ -7273,7 +7272,6 @@ int ice_down(struct ice_vsi *vsi)
 
 	if (vsi->netdev) {
 		vlan_err = ice_vsi_del_vlan_zero(vsi);
-		ice_ptp_link_change(vsi->back, false);
 		netif_carrier_off(vsi->netdev);
 		netif_tx_disable(vsi->netdev);
 	}
@@ -7794,6 +7792,15 @@ static void ice_rebuild(struct ice_pf *pf, enum ice_reset_req reset_type)
 
 	ice_update_pf_netdev_link(pf);
 
+	if (test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags) && pf->hw.port_info) {
+		bool link_up;
+
+		link_up = !!(pf->hw.port_info->phy.link_info.link_info &
+			     ICE_AQ_LINK_UP);
+		if (pf->ptp.port.link_up != link_up)
+			ice_ptp_link_change(pf, link_up);
+	}
+
 	/* tell the firmware we are up */
 	err = ice_send_version(pf);
 	if (err) {
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index b8647a39db6d..8aa49dda90a2 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -1327,14 +1327,14 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
 	struct ice_ptp_port *ptp_port;
 	struct ice_hw *hw = &pf->hw;
 
-	if (pf->ptp.state != ICE_PTP_READY)
-		return;
-
 	ptp_port = &pf->ptp.port;
 
 	/* Update cached link status for this port immediately */
 	ptp_port->link_up = linkup;
 
+	if (pf->ptp.state != ICE_PTP_READY)
+		return;
+
 	/* Skip HW writes if reset is in progress */
 	if (pf->hw.reset_ongoing)
 		return;
@@ -3296,9 +3296,13 @@ static int ice_ptp_init_owner(struct ice_pf *pf)
 }
 
 /**
- * ice_ptp_init_work - Initialize PTP work threads
+ * ice_ptp_init_work - Initialize the PTP kworker
  * @pf: Board private structure
  * @ptp: PF PTP structure
+ *
+ * Allocate the kworker and initialize the periodic work function. The
+ * periodic work is not queued here; the caller starts it once the PTP
+ * state is ICE_PTP_READY.
  */
 static int ice_ptp_init_work(struct ice_pf *pf, struct ice_ptp *ptp)
 {
@@ -3317,9 +3321,6 @@ static int ice_ptp_init_work(struct ice_pf *pf, struct ice_ptp *ptp)
 
 	ptp->kworker = kworker;
 
-	/* Start periodic work going */
-	kthread_queue_delayed_work(ptp->kworker, &ptp->work, 0);
-
 	return 0;
 }
 
@@ -3441,6 +3442,22 @@ void ice_ptp_init(struct ice_pf *pf)
 	if (err)
 		goto err_clean_pf;
 
+	/* Seed link_up from current PHY status, since link may already be up
+	 * (e.g. after PXE boot) with no link-change edge to catch it later.
+	 */
+	if (pf->hw.port_info)
+		ptp->port.link_up =
+			!!(pf->hw.port_info->phy.link_info.link_info &
+			ICE_AQ_LINK_UP);
+
+	/* Create the kworker before restarting the PHY, which queues work on
+	 * it in the E82x restart path. This prevents concurrent link events
+	 * from reaching ice_ptp_port_phy_restart() while kworker is still NULL
+	 */
+	err = ice_ptp_init_work(pf, ptp);
+	if (err)
+		goto err_exit;
+
 	/* Start the PHY timestamping block */
 	ice_ptp_reset_phy_timestamping(pf);
 
@@ -3449,9 +3466,10 @@ void ice_ptp_init(struct ice_pf *pf)
 
 	ptp->state = ICE_PTP_READY;
 
-	err = ice_ptp_init_work(pf, ptp);
-	if (err)
-		goto err_exit;
+	/* Start periodic work only after the state is READY; the worker
+	 * returns without rescheduling while the state is not READY.
+	 */
+	kthread_queue_delayed_work(ptp->kworker, &ptp->work, 0);
 
 	dev_info(ice_pf_to_dev(pf), "PTP init successful\n");
 	return;

-- 
2.55.0.814.gc42f45431d0f

[PATCH iwl-net 05/12] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY

[Jacob Keller]

The ice_stop_phy_timer_eth56g() function is called by the driver for E825
devices to ensure that the PHY timer has been stopped. The equivalent
function for older E822 devices performed many steps. However, on E825 it
only clears the PHY_REG_TX_OFFSET_READY and PHY_REG_RX_OFFSET_READY bits to
indicate to HW that it should no longer treat the PHY offset as valid.

When PHY_REG_TX_OFFSET_READY is cleared, the hardware still captures Tx
timestamps, but it no longer sets the valid bit for these timestamps. This
sounds reasonable at first glance. However, this results in the internal
outstanding timestamp counter becoming out of sync.

When capturing a timestamp, hardware increments its internal counter and
sets the associated "ready" bit in the timestamp memory status. Then it
compares the timestamp count to the threshold to determine if it should
trigger an interrupt to the MAC.

Upon reading the timestamp hardware is supposed to decrement the counter,
clear the valid bit, and clear the associated bit from the memory status
register. However, it only performs these steps *if* the valid bit is set.

Since the valid bit is not set while PHY_REG_TX_OFFSET_READY is clear, the
timestamp counter is not decremented and the memory status is not cleared.
This leaves the counter out-of-sync until a PHY soft reset.

According to the hardware engineers, the PHY_REG_TX_OFFSET_READY bit has no
other effects. It only controls whether hardware captures timestamps with
the valid bit set or not. Since capturing timestamps with the valid bit
clear is problematic, they recommend simply not clearing
PHY_REG_TX_OFFSET_READY.

Note that the PHY_REG_RX_OFFSET_READY performs a similar task. However,
clearing it is fine as there is no associated timestamp counter on the Rx
side. Receive timestamps are simply inserted into the descriptor. Clearing
this register clears the valid bit for timestamps until we complete
calibration and re-enable the register.

Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C products")
Suggested-by: Maciej Machnikowski <maciej.machnikowski@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 3a41c711e751..d48eb3c61823 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -2115,10 +2115,6 @@ int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port, bool soft_reset)
 {
 	int err;
 
-	err = ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_OFFSET_READY, 0);
-	if (err)
-		return err;
-
 	err = ice_write_ptp_reg_eth56g(hw, port, PHY_REG_RX_OFFSET_READY, 0);
 	if (err)
 		return err;

-- 
2.55.0.814.gc42f45431d0f

[PATCH iwl-net 06/12] ice: E825: clear PHY_REG_TX_MEMORY_STATUS prior to soft reset

[Jacob Keller]

The drivers current implementation of ice_ptp_reset_ts_memory_eth56g() is
flawed. It tries to clear the timestamp memory by writing to the
PHY_REG_TX_MEMORY_STATUS region. This does not work properly, as it does
not trigger appropriate PHY actions.

To clear outstanding timestamp memory, the driver must read the timestamps.
However, naively doing this as part of ice_ptp_reset_ts_memory() is
problematic. When reading the timestamp index, hardware kicks off a chain
of actions including clearing the ready bitmap index, and decrementing an
internal counter if the timestamp index was marked as valid.

This can potentially leave the internal hardware counter out of sync with
the actual number of timestamps. This occurs because the
PHY_REG_TX_MEMORY_STATUS region is not zero-initialized when the device
boots up. Instead, it is filled with garbage. On a cold power on, attempts
to read the stale data result in the hardware triggering a counter
decrement for a timestamp that never happened. This underflows the counter,
and prevents new timestamp interrupts from being triggered for real
timestamp requests.

We must read the PHY_REG_TX_MEMORY_STATUS in order to clear stale
timestamps. But doing so may cause a desync with the counter. To prevent
issues, perform this clearing always and only right before initiating a PHY
soft reset.

The soft reset will clear and reset the internal counter and the ready
bitmap. The reads to PHY_REG_TX_MEMORY_STATUS will reset the region valid
bits ensuring that no stale data is left behind. This combination ensures
that we always have a clean slate with no stale data and with the counter
properly reset to zero.

Fixes: 3ec46e157c7f ("ice: perform PHY soft reset for E825C ports at initialization")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 89 +++++++++++++++--------------
 1 file changed, 47 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index d48eb3c61823..b7d217ac31f3 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -737,24 +737,6 @@ static int ice_read_port_mem_eth56g(struct ice_hw *hw, u8 port, u16 offset,
 	return ice_read_port_eth56g(hw, port, offset, val, ETH56G_PHY_MEM_PTP);
 }
 
-/**
- * ice_write_port_mem_eth56g - Write a PHY port memory location
- * @hw: pointer to the HW struct
- * @port: Port number to be read
- * @offset: Offset from PHY port register base
- * @val: Pointer to the value to read (out param)
- *
- * Return:
- * * %0      - success
- * * %EINVAL - invalid port number or resource type
- * * %other  - failed to write to PHY
- */
-static int ice_write_port_mem_eth56g(struct ice_hw *hw, u8 port, u16 offset,
-				     u32 val)
-{
-	return ice_write_port_eth56g(hw, port, offset, val, ETH56G_PHY_MEM_PTP);
-}
-
 /**
  * ice_write_quad_ptp_reg_eth56g - Write a PHY quad register
  * @hw: pointer to the HW struct
@@ -1139,8 +1121,8 @@ static int ice_read_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx,
  * internal PHYs of the 56G devices.
  *
  * To directly clear the contents of the timestamp block entirely, discarding
- * all timestamp data at once, software should instead use
- * ice_ptp_reset_ts_memory_quad_eth56g().
+ * all timestamp data at once, software should instead perform a PHY soft
+ * reset via ice_ptp_phy_soft_reset_eth56g().
  *
  * This function should only be called on an idx whose bit is set according to
  * ice_get_phy_tx_tstamp_ready().
@@ -1152,11 +1134,11 @@ static int ice_read_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx,
 static int ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx)
 {
 	u64 unused_tstamp;
-	u16 lo_addr;
 	int err;
 
-	/* Read the timestamp register to ensure the timestamp status bit is
-	 * cleared.
+	/* Per the PHY spec, reading the timestamp memory location is what
+	 * clears the entry's valid bit and its corresponding (read-only)
+	 * ts_memory_status bit.
 	 */
 	err = ice_read_ptp_tstamp_eth56g(hw, port, idx, &unused_tstamp);
 	if (err) {
@@ -1164,32 +1146,40 @@ static int ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx)
 			  port, idx, err);
 	}
 
-	lo_addr = (u16)PHY_TSTAMP_L(idx);
-
-	err = ice_write_port_mem_eth56g(hw, port, lo_addr, 0);
-	if (err) {
-		ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register for port %u, idx %u, err %d\n",
-			  port, idx, err);
-		return err;
-	}
-
 	return 0;
 }
 
 /**
- * ice_ptp_reset_ts_memory_eth56g - Clear all timestamps from the port block
+ * ice_ptp_clear_tx_memory_status_eth56g - Reset one port's Tx timestamp memory
  * @hw: pointer to the HW struct
+ * @port: port number to clear
+ *
+ * Fully reset a single PHY port's Tx timestamp memory. Per the PHY spec, the
+ * only way to clear a timestamp valid bit (and its read-only ts_memory_status
+ * bit) is to read the timestamp memory location, so read every entry for the
+ * port (two 32-bit reads each). This discards all timestamp data on the port,
+ * so it must only be used for a full reset; callers that must preserve
+ * in-flight timestamps clear individual indices via ice_clear_phy_tstamp().
+ *
+ * Due to interactions with an internal HW counter for the number of
+ * outstanding Tx timestamps, this *must* only be called as part of the
+ * ice_ptp_phy_soft_reset_eth56g() procedure. Otherwise, the internal counter
+ * may become out of sync and prevent new timestamp interrupts.
+ *
+ * Return: 0 on success, negative error code on failure to read the PHY.
  */
-static void ice_ptp_reset_ts_memory_eth56g(struct ice_hw *hw)
+static int ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port)
 {
-	unsigned int port;
+	int err = 0;
+	u8 idx;
 
-	for (port = 0; port < hw->ptp.num_lports; port++) {
-		ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_MEMORY_STATUS_L,
-					 0);
-		ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_MEMORY_STATUS_U,
-					 0);
+	for (idx = 0; idx < INDEX_PER_PORT; idx++) {
+		err = ice_clear_ptp_tstamp_eth56g(hw, port, idx);
+		if (err)
+			return err;
 	}
+
+	return 0;
 }
 
 /**
@@ -2290,6 +2280,7 @@ int ice_ptp_read_tx_hwtstamp_status_eth56g(struct ice_hw *hw, u32 *ts_status)
  *
  * Trigger a soft reset of the ETH56G PHY by toggling the soft reset
  * bit in the PHY global register. The reset sequence consists of:
+ *   0. Reading every timestamp memory register to clear its valid bit
  *   1. Clearing the soft reset bit
  *   2. Asserting the soft reset bit
  *   3. Clearing the soft reset bit again
@@ -2298,6 +2289,12 @@ int ice_ptp_read_tx_hwtstamp_status_eth56g(struct ice_hw *hw, u32 *ts_status)
  * to settle. This provides a controlled way to reinitialize the PHY
  * without requiring a full device reset.
  *
+ * To ensure that the internal counter matches the contents of the
+ * PHY_REG_TX_MEMORY_STATUS, read every timestamp index prior to performing
+ * the soft reset. The PHY_REG_TX_MEMORY_STATUS reads ensure that the region
+ * is cleared, while the soft reset procedure ensures that the timsetamp
+ * counter is reset to zero.
+ *
  * Return: 0 on success, or a negative error code on failure when
  *         reading or writing the PHY register.
  */
@@ -2306,6 +2303,13 @@ int ice_ptp_phy_soft_reset_eth56g(struct ice_hw *hw, u8 port)
 	u32 global_val;
 	int err;
 
+	err = ice_ptp_clear_tx_memory_status_eth56g(hw, port);
+	if (err) {
+		ice_debug(hw, ICE_DBG_PTP, "Failed to clear PHY_REG_TX_MEMORY_STATUS for port %d, err %d\n",
+			  port, err);
+		return err;
+	}
+
 	err = ice_read_ptp_reg_eth56g(hw, port, PHY_REG_GLOBAL, &global_val);
 	if (err) {
 		ice_debug(hw, ICE_DBG_PTP, "Failed to read PHY_REG_GLOBAL for port %d, err %d\n",
@@ -5797,8 +5801,9 @@ void ice_ptp_reset_ts_memory(struct ice_hw *hw)
 		ice_ptp_reset_ts_memory_e82x(hw);
 		break;
 	case ICE_MAC_GENERIC_3K_E825:
-		ice_ptp_reset_ts_memory_eth56g(hw);
-		break;
+		/* E825 hardware must only reset timestamp memory as part of
+		 * the soft reset procedure.
+		 */
 	case ICE_MAC_E810:
 	default:
 		return;

-- 
2.55.0.814.gc42f45431d0f

[PATCH iwl-net 07/12] ice: E825: perform a soft reset when starting the PHY timer

[Jacob Keller]

To ensure that the E825 PHY timer begins in a clean state, initiate a PHY
soft reset prior to programming the PHY. This ensures that we clear any
outstanding Tx timestamp memory, and ensures that the PHY internal state
has been completely reset.

Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C products")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index b7d217ac31f3..10e973876608 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -2119,9 +2119,11 @@ int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port, bool soft_reset)
  * @hw: pointer to the HW struct
  * @port: the PHY port to start
  *
- * Start the clock of a PHY port. This must be done as part of the flow to
- * re-calibrate Tx and Rx timestamping offsets whenever the clock time is
- * initialized or when link speed changes.
+ * Perform a PHY soft reset and then start the clock for the PHY port.
+ *
+ * This must be done as part of the flow to re-calibrate Tx and Rx
+ * timestamping offsets whenever the clock time is initialized or when link
+ * speed changes.
  *
  * Return:
  * * %0     - success
@@ -2137,6 +2139,10 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
 
 	tmr_idx = ice_get_ptp_src_clock_index(hw);
 
+	err = ice_ptp_phy_soft_reset_eth56g(hw, port);
+	if (err)
+		return err;
+
 	err = ice_stop_phy_timer_eth56g(hw, port, false);
 	if (err)
 		return err;

-- 
2.55.0.814.gc42f45431d0f

[PATCH iwl-net 08/12] ice: wait for in-flight Tx timestamps before flushing the tracker

[Jacob Keller]

From: Petr Oros <poros@redhat.com>

ice_ptp_flush_tx_tracker() frees every tracked request, but a request
whose timestamp is still being captured by the PHY at that moment is
freed without touching the PHY entry. The ready bit published shortly
after has no tracked owner, and the PHY does not raise another Tx
timestamp interrupt until every outstanding ready bit is read, so
delivery for the whole quad degrades to the periodic work.

Wait up to 10 ms for in-flight captures to publish their ready bits
before flushing, so the flush clears them together with the rest.

Fixes: ea9b847cda64 ("ice: enable transmit timestamps for E810 devices")
Signed-off-by: Petr Oros <poros@redhat.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 33 ++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 8aa49dda90a2..a049dc7a2241 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -740,6 +740,37 @@ ice_ptp_alloc_tx_tracker(struct ice_ptp_tx *tx)
 	return 0;
 }
 
+static void
+ice_ptp_wait_for_tracker_drain(struct ice_pf *pf, struct ice_ptp_tx *tx)
+{
+	unsigned long deadline = jiffies + msecs_to_jiffies(10);
+	struct ice_hw *hw = &pf->hw;
+	u64 tstamp_ready;
+	bool pending;
+	u8 idx;
+
+	if (hw->reset_ongoing)
+		return;
+
+	do {
+		if (ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready))
+			return;
+
+		pending = false;
+		for_each_set_bit(idx, tx->in_use, tx->len) {
+			if (!(tstamp_ready & BIT_ULL(idx + tx->offset)))
+				pending = true;
+		}
+		if (!pending)
+			return;
+
+		usleep_range(500, 1000);
+	} while (time_before(jiffies, deadline));
+
+	dev_dbg(ice_pf_to_dev(pf), "Timed out waiting for in-flight Tx timestamps on block %u\n",
+		tx->block);
+}
+
 /**
  * ice_ptp_flush_tx_tracker - Flush any remaining timestamps from the tracker
  * @pf: Board private structure
@@ -756,6 +787,8 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
 	int err;
 	u8 idx;
 
+	ice_ptp_wait_for_tracker_drain(pf, tx);
+
 	err = ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready);
 	if (err) {
 		dev_dbg(ice_pf_to_dev(pf), "Failed to get the Tx tstamp ready bitmap for block %u, err %d\n",

-- 
2.55.0.814.gc42f45431d0f

[PATCH iwl-net 09/12] ice: keep Tx timestamp slots tracked until completion or timeout

[Jacob Keller]

From: Petr Oros <poros@redhat.com>

When the link goes down the processing loop drops every outstanding
request, and a request whose timestamp is not ready yet is freed
without reading the PHY slot. The hardware completes the capture a
moment later, the orphaned ready bit blocks the port interrupt until
the next link-up sweep, and the freed index can meanwhile be reused
by a new request whose slot the hardware then overwrites. Captured on
a reproducer as ready bits with no in_use owner right after a link
bounce.

Stop dropping on link down. Mark the outstanding requests stale so
their completions are read and discarded, reject new requests while
the link is down, and free a not yet ready slot only after the two
second timeout. This way an index is never reused while the hardware
can still write it and never left untracked while a completion can
still arrive.

To avoid an IRQ storm in the event that we really do have a stale packet
that is not timestamped, modify ice_ptp_tx_tstamps_pending() to ignore
stale timestamps when checking for whether to re-arm the IRQ from the
miscellaneous thread function. Instead, only check for stale packets in the
auxiliary work thread. This way we do not check in a tight loop waiting for
a timestamp that may never come.

This effectively reverts commit fcc2cef37fed ("ice/ptp: fix the PTP worker
retrying indefinitely if the link went down"), which tried to release an
index before this 2 second wait period.

Fixes: fcc2cef37fed ("ice/ptp: fix the PTP worker retrying indefinitely if the link went down")
Suggested-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Petr Oros <poros@redhat.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.h  |  8 ++++--
 drivers/net/ethernet/intel/ice/ice_main.c |  2 +-
 drivers/net/ethernet/intel/ice/ice_ptp.c  | 44 +++++++++++++++----------------
 3 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index da2003ba3bb0..13158a9319fb 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -132,6 +132,9 @@ struct ice_ptp_tx {
 #define INDEX_PER_PORT_E82X		16
 #define INDEX_PER_PORT			64
 
+/* Maximum number of timestamp indexes across all devices */
+#define INDEX_PER_PORT_MAX              INDEX_PER_PORT
+
 /**
  * struct ice_ptp_port - data used to initialize an external port for PTP
  *
@@ -316,7 +319,7 @@ void ice_ptp_req_tx_single_tstamp(struct ice_ptp_tx *tx, u8 idx);
 void ice_ptp_complete_tx_single_tstamp(struct ice_ptp_tx *tx);
 void ice_ptp_process_ts(struct ice_pf *pf);
 irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf);
-bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf);
+bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf, bool in_irq);
 u64 ice_ptp_read_src_clk_reg(struct ice_pf *pf,
 			     struct ptp_system_timestamp *sts);
 
@@ -364,7 +367,8 @@ static inline irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf)
 	return IRQ_HANDLED;
 }
 
-static inline bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf)
+static inline bool
+ice_ptp_tx_tstamps_pending(struct ice_pf *pf, bool in_irq)
 {
 	return false;
 }
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index bb631ae9e67d..c84d63b8d261 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3248,7 +3248,7 @@ static irqreturn_t ice_misc_intr_thread_fn(int __always_unused irq, void *data)
 	ice_irq_dynamic_ena(hw, NULL, NULL);
 	ice_flush(hw);
 
-	if (ice_ptp_tx_tstamps_pending(pf)) {
+	if (ice_ptp_tx_tstamps_pending(pf, true)) {
 		/* If any new Tx timestamps happened while in interrupt,
 		 * re-arm the interrupt to trigger it again.
 		 */
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index a049dc7a2241..1a9bf8839404 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -364,9 +364,12 @@ static u64 ice_ptp_extend_40b_ts(struct ice_pf *pf, u64 in_tstamp)
 static bool
 ice_ptp_is_tx_tracker_up(struct ice_ptp_tx *tx)
 {
+	struct ice_ptp_port *ptp_port =
+		container_of(tx, struct ice_ptp_port, tx);
+
 	lockdep_assert_held(&tx->lock);
 
-	return tx->init && !tx->calibrating;
+	return tx->init && !tx->calibrating && ptp_port->link_up;
 }
 
 /**
@@ -563,7 +566,6 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
 	struct ice_pf *pf;
 	struct ice_hw *hw;
 	u64 tstamp_ready;
-	bool link_up;
 	int err;
 	u8 idx;
 
@@ -581,14 +583,11 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
 			return;
 	}
 
-	/* Drop packets if the link went down */
-	link_up = ptp_port->link_up;
-
 	for_each_set_bit(idx, tx->in_use, tx->len) {
 		struct skb_shared_hwtstamps shhwtstamps = {};
 		u8 phy_idx = idx + tx->offset;
 		u64 raw_tstamp = 0, tstamp;
-		bool drop_ts = !link_up;
+		bool drop_ts = false;
 		struct sk_buff *skb;
 
 		/* Drop packets which have waited for more than 2 seconds */
@@ -1372,6 +1371,9 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
 	if (pf->hw.reset_ongoing)
 		return;
 
+	if (!linkup)
+		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
+
 	if (hw->mac_type == ICE_MAC_GENERIC_3K_E825 &&
 	    test_bit(ICE_FLAG_DPLL, pf->flags)) {
 		int pin, err;
@@ -2763,28 +2765,29 @@ void ice_ptp_process_ts(struct ice_pf *pf)
 	}
 }
 
-static bool ice_port_has_timestamps(struct ice_ptp_tx *tx)
+static bool ice_port_has_timestamps(struct ice_ptp_tx *tx, bool in_irq)
 {
-	bool more_timestamps;
+	DECLARE_BITMAP(tstamps, INDEX_PER_PORT_MAX) = {};
 
 	scoped_guard(spinlock_irqsave, &tx->lock) {
 		if (!tx->init)
 			return false;
 
-		more_timestamps = !bitmap_empty(tx->in_use, tx->len);
+		if (in_irq)
+			return bitmap_andnot(tstamps, tx->in_use, tx->stale, tx->len);
+		else
+			return !bitmap_empty(tx->in_use, tx->len);
 	}
-
-	return more_timestamps;
 }
 
-static bool ice_any_port_has_timestamps(struct ice_pf *pf)
+static bool ice_any_port_has_timestamps(struct ice_pf *pf, bool in_irq)
 {
 	bool have_tstamps = false;
 	struct ice_ptp_port *port;
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(port, &pf->adapter->ports.list, list_node) {
-		if (ice_port_has_timestamps(&port->tx)) {
+		if (ice_port_has_timestamps(&port->tx, in_irq)) {
 			have_tstamps = true;
 			break;
 		}
@@ -2794,7 +2797,7 @@ static bool ice_any_port_has_timestamps(struct ice_pf *pf)
 	return have_tstamps;
 }
 
-bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf)
+bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf, bool in_irq)
 {
 	struct ice_hw *hw = &pf->hw;
 	int ret;
@@ -2804,11 +2807,11 @@ bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf)
 	case ICE_PTP_TX_INTERRUPT_NONE:
 		return false;
 	case ICE_PTP_TX_INTERRUPT_SELF:
-		if (ice_port_has_timestamps(&pf->ptp.port.tx))
+		if (ice_port_has_timestamps(&pf->ptp.port.tx, in_irq))
 			return true;
 		break;
 	case ICE_PTP_TX_INTERRUPT_ALL:
-		if (ice_any_port_has_timestamps(pf))
+		if (ice_any_port_has_timestamps(pf, in_irq))
 			return true;
 		break;
 	default:
@@ -2884,7 +2887,7 @@ irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf)
 		/* E830 can read timestamps in the top half using rd32() */
 		ice_ptp_process_ts(pf);
 
-		if (ice_ptp_tx_tstamps_pending(pf)) {
+		if (ice_ptp_tx_tstamps_pending(pf, true)) {
 			/* Process outstanding Tx timestamps. If there
 			 * is more work, re-arm the interrupt to trigger again.
 			 */
@@ -2914,7 +2917,6 @@ static void ice_ptp_maybe_trigger_tx_interrupt(struct ice_pf *pf)
 {
 	struct device *dev = ice_pf_to_dev(pf);
 	struct ice_hw *hw = &pf->hw;
-	int ret;
 
 	if (!pf->ptp.port.tx.has_ready_bitmap)
 		return;
@@ -2922,11 +2924,7 @@ static void ice_ptp_maybe_trigger_tx_interrupt(struct ice_pf *pf)
 	if (!ice_pf_src_tmr_owned(pf))
 		return;
 
-	ret = ice_check_phy_tx_tstamp_ready(hw);
-	if (ret < 0) {
-		dev_dbg(dev, "PTP periodic task unable to read PHY timestamp ready bitmap, err %d\n",
-			ret);
-	} else if (ret) {
+	if (ice_ptp_tx_tstamps_pending(pf, false)) {
 		dev_dbg(dev, "PTP periodic task detected waiting timestamps. Triggering Tx timestamp interrupt now.\n");
 
 		wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);

-- 
2.55.0.814.gc42f45431d0f

RE: [PATCH iwl-net 01/12] ice: use reference counting and RCU for PTP port access

[Nowlin, Alexander]

> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@intel.com> 
> Sent: Friday, August 21, 2026 5:13 PM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: netdev@vger.kernel.org; Machnikowski, Maciej <maciej.machnikowski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Korba, Przemyslaw <przemyslaw.korba@intel.com>; Nitka, Grzegorz <grzegorz.nitka@intel.com>; Oros, Petr <poros@redhat.com>; Nowlin, Alexander <alexander.nowlin@intel.com>; Bross, Kevin <kevin.bross@intel.com>; Cavatur, Ranjit <ranjit.cavatur@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>; Machnikowski, Maciej <maciej.machnikowski@intel.com>
> Subject: [PATCH iwl-net 01/12] ice: use reference counting and RCU for PTP port access
> 
> The ice adapter structure maintains a list of ports associated with the adapter. This is used for supporting PTP, where the clock owner must handle many operations that require access to the PTP port structures > of the associated PFs.
> 
> This is implemented using a linked list and a mutex. This sort of works, but a few places within the code do not acquire the mutex when iterating the list. This includes ice_ptp_flush_all_tx_tracker(), ice_ptp_restart_all_phy(), and ice_ptp_prepare_rebuild_sec().
> 
> Fixing this is tricky, especially since it is not clear if we can simply acquire the lock around the complete iterations.
> 
> The pattern of use for the port list is read-mostly with modifications only happening during PF initialization when elements are inserted. This typically only happens during early boot, though a PF could in principle be removed or loaded at arbitrary times via bind and unbind operations.
> 
> The use of a mutex does mean the driver can sleep while holding it, but it still creates complicates with lock ordering and prevents iterating the list in any code path that *can't* sleep.
> 
> Instead, use the RCU primitives for the port linked list, along with a reference count on the port. The kref reference counter ensures that we can safely acquire pointers with a guarantee of their lifetime, ensuring > the associated PF will not be removed until the reference is released.
> 
> For port iterations which are short and definitely can't sleep, wrap the entire loop with rcu_read_lock() and rcu_read_unlock().
> 
> For longer operations, or those which might sleep, we need to close the critical section between each loop iteration. To make this safe, start the loop iteration with rcu_read_lock(), then acquire a reference for the port with kref_get_unless_zero. If this returns 0, the port is already in the process of being removed, so that port should be skipped when iterating. Once a reference to the port is acquired, exit the RCU critical > section. Then, perform the desired operations on the port, followed by releasing the reference with kref_put and then re-entering a critical section at the end of the loop body. Note that kref_put() is done outside the RCU critical section. This is safe, as the port will not be freed until all references are dropped.
> 
> The ice_ptp_release_port_rcu() function is used as the release function for the kref_put() call. To avoid a potential infinite loop of new references, the release function simply uses a wake_up_var() call to wake the closing thread. The ice_ptp_cleanup_pf() function will remove the port from the linked list using list_del_rcu, then release its primary reference, then wait for all references to drop via wait queue. Finally synchronize_rcu() is called to guarantee the port remains valid for at least one RCU grace period. Then PF removal will continue.
> 
> This flow ensures that all accesses to ports via the port list will remain valid until either the RCU critical sections end, or the references have been dropped.
> 
> One major complication of this reference count is that ice_ptp_port is embedded inside of other structures and not merely allocated. As a result, we can't use the standard pattern of kfree_rcu() to just delay freeing until references are dropped, and instead are delaying PF port teardown. If any code path leaks the reference, the driver will be unable to teardown.
> Instead, a 15 second timeout with a WARN() is used when waiting to finally allow PF teardown to continue. This has the risk of potentially allowing use-after-free, assuming some path really is stuck for 15 seconds. However, this both less likely and a less bad outcome compared to blocking indefinitely on a reference leak.
> 
> Fixes: e800654e85b5 ("ice: Use ice_adapter for PTP shared data instead of auxdev")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_adapter.h |   6 +-
>  drivers/net/ethernet/intel/ice/ice_ptp.h     |   4 +
>  drivers/net/ethernet/intel/ice/ice_adapter.c |   7 +-
>  drivers/net/ethernet/intel/ice/ice_ptp.c     | 118 +++++++++++++++++++--------
>  4 files changed, 96 insertions(+), 39 deletions(-)

Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>

RE: [PATCH iwl-net 02/12] ice: E822: keep Tx timestamps disabled during offset calibration

[Nowlin, Alexander]

> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@intel.com> 
> Sent: Friday, August 21, 2026 5:13 PM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: netdev@vger.kernel.org; Machnikowski, Maciej <maciej.machnikowski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Korba, Przemyslaw <przemyslaw.korba@intel.com>; Nitka, Grzegorz <grzegorz.nitka@intel.com>; Oros, Petr <poros@redhat.com>; Nowlin, Alexander <alexander.nowlin@intel.com>; Bross, Kevin <kevin.bross@intel.com>; Cavatur, Ranjit <ranjit.cavatur@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>; Karol Kolacinski <karol.kolacinski@intel.com>; Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Korba, Przemyslaw <przemyslaw.korba@intel.com>
> Subject: [PATCH iwl-net 02/12] ice: E822: keep Tx timestamps disabled during offset calibration
> 
> From: Karol Kolacinski <karol.kolacinski@intel.com>
> 
> Do not clear the tx.calibrating flag immediately after starting the PHY timer in ice_ptp_port_phy_restart(). Instead, keep Tx timestamps disabled until the offset verification work (ice_ptp_wait_for_offsets) has confirmed that both Tx and Rx PHY offsets are properly configured.
> 
> Previously, tx.calibrating was set to true, then immediately back to false right after ice_start_phy_timer_e82x() returned. This allowed Tx timestamp requests to be served during the window where offset verification was still pending. Timestamps produced during this window use uncalibrated PHY offsets and can produce incorrect values.
> 
> When ptp4l receives incorrect timestamps, it may reject them and wait for the next sync interval (typically 1-2 seconds), compounding delays during link cycling. This contributes to the time transmitter port becoming unresponsive after repeated link down/up cycles.
> 
> Move the tx.calibrating = false to ice_ptp_wait_for_offsets(), after both Tx and Rx offset configuration has completed successfully. This ensures that Tx timestamps are only reported with properly calibrated PHY > offsets.
> 
> If ice_start_phy_timer_e82x() fails, restore calibrating to false on the error path to prevent permanently disabling Tx timestamps when ov_work is never queued.
> 
> Log a debug message while offset calibration is still pending, including the specific Tx/Rx error codes to aid debugging stalled calibration.
> This path is expected on every routine link-up: ov_work is first queued with no delay and the vernier offset cannot be computed until at least one packet has been transmitted, so the first several invocations normally land here. Use dev_dbg() rather than a rate-limited warning to avoid emitting KERN_WARNING on every link-up during normal operation.
> Log a debug message when calibration completes successfully.
> 
> Fixes: 3a7496234d17 ("ice: implement basic E822 PTP support")
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.c | 36 ++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)

Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>

RE: [PATCH iwl-net 03/12] ice: E822: cancel offset verification work during reset preparation

[Nowlin, Alexander]

> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@intel.com> 
> Sent: Friday, August 21, 2026 5:13 PM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: netdev@vger.kernel.org; Machnikowski, Maciej <maciej.machnikowski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Korba, Przemyslaw <przemyslaw.korba@intel.com>; Nitka, Grzegorz <grzegorz.nitka@intel.com>; Oros, Petr <poros@redhat.com>; Nowlin, Alexander <alexander.nowlin@intel.com>; Bross, Kevin <kevin.bross@intel.com>; Cavatur, Ranjit <ranjit.cavatur@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>; Karol Kolacinski <karol.kolacinski@intel.com>; Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Korba, Przemyslaw <przemyslaw.korba@intel.com>; Machnikowski, Maciej <maciej.machnikowski@intel.com>
> Subject: [PATCH iwl-net 03/12] ice: E822: cancel offset verification work during reset preparation
> 
> From: Karol Kolacinski <karol.kolacinski@intel.com>
> 
> Cancel the offset verification delayed work (ov_work) during PTP reset preparation to prevent it from running concurrently with the reset sequence.
> 
> Without this, ice_ptp_wait_for_offsets() can execute during a reset, find that ice_is_reset_in_progress() is true, and re-queue itself in a tight loop. Meanwhile, the reset path in ice_ptp_rebuild_owner() calls ice_ptp_port_phy_restart(), which starts a new ov_work. This results in two ov_work instances running simultaneously, racing over the PHY offset registers and the calibrating flag.
> 
> Cancel ov_work in ice_ptp_prepare_for_reset() alongside the existing cancellation of the Tx tracker. The comment in the existing code already documents that ov_work interference during reset is a concern but > only cancels it during ice_ptp_release().
> 
> Fixes: 4809671015a1 ("ice: Fix E810 PTP reset flow")
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
> Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>

RE: [PATCH iwl-net 04/12] ice: call PTP link change only from link events

[Nowlin, Alexander]

> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@intel.com> 
> Sent: Friday, August 21, 2026 5:13 PM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: netdev@vger.kernel.org; Machnikowski, Maciej <maciej.machnikowski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Korba, Przemyslaw <przemyslaw.korba@intel.com>; Nitka, Grzegorz <grzegorz.nitka@intel.com>; Oros, Petr <poros@redhat.com>; Nowlin, Alexander <alexander.nowlin@intel.com>; Bross, Kevin <kevin.bross@intel.com>; Cavatur, Ranjit <ranjit.cavatur@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; Korba, Przemyslaw <przemyslaw.korba@intel.com>; Machnikowski, > Maciej <maciej.machnikowski@intel.com>
> Subject: [PATCH iwl-net 04/12] ice: call PTP link change only from link events
> 
> From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> 
> Remove redundant ice_ptp_link_change() calls from ice_up_complete() and ice_down(). These duplicate the call already made from ice_handle_link_event(), creating three problems:
> 
> 1. Double initialization on link-up: ice_handle_link_event() calls
>    ice_ptp_link_change(true), then ice_up_complete() calls it again.
>    The second call re-enters ice_ptp_port_phy_restart(), re-setting the
>    calibrating flag and restarting the PHY timer while the first
>    invocation's offset verification work (ov_work) may still be running.
> 
> 2. Premature cleanup on administrative down: ice_down() calls
>    ice_ptp_link_change(false) during ifconfig down or reset preparation,
>    even when the physical link is still up. This clears timestamp state
>    unnecessarily and can interfere with ongoing PTP operations.
> 
> 3. Ordering dependency: ice_down()/ice_up_complete() are called during
>    reset sequences where PTP may not be fully initialized, creating
>    edge cases with partially configured state.
> 
> The link event handler is the correct and sufficient place to drive PTP link state changes, as it reflects actual physical link transitions. Remove the calls of ice_ptp_link_change from the ice_down()/ice_up() flows.
> 
> Initialize the link_up in ice_ptp_init() and ensure that we check and restore the link status at the end of the rebuild flow, ensuring that we initialize the PHY timer appropriately after a reset.
> 
> Fixes: 6b1ff5d39228 ("ice: always call ice_ptp_link_change and make it void")
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
> Signed-off-by: Petr Oros <poros@redhat.com>
> Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_main.c | 11 +++++++--  drivers/net/ethernet/intel/ice/ice_ptp.c  | 38 +++++++++++++++++++++++--------
>  2 files changed, 37 insertions(+), 12 deletions(-)

Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>

RE: [PATCH iwl-net 05/12] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY

[Nowlin, Alexander]

> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@intel.com> 
> Sent: Friday, August 21, 2026 5:13 PM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: netdev@vger.kernel.org; Machnikowski, Maciej <maciej.machnikowski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Korba, Przemyslaw <przemyslaw.korba@intel.com>; Nitka, Grzegorz <grzegorz.nitka@intel.com>; Oros, Petr <poros@redhat.com>; Nowlin, Alexander <alexander.nowlin@intel.com>; Bross, Kevin <kevin.bross@intel.com>; Cavatur, Ranjit <ranjit.cavatur@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>; Machnikowski, Maciej <maciej.machnikowski@intel.com>
> Subject: [PATCH iwl-net 05/12] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY
> 
> The ice_stop_phy_timer_eth56g() function is called by the driver for E825 devices to ensure that the PHY timer has been stopped. The equivalent function for older E822 devices performed many steps. However, on E825 it only clears the PHY_REG_TX_OFFSET_READY and PHY_REG_RX_OFFSET_READY bits to indicate to HW that it should no longer treat the PHY offset as valid.
> 
> When PHY_REG_TX_OFFSET_READY is cleared, the hardware still captures Tx timestamps, but it no longer sets the valid bit for these timestamps. This sounds reasonable at first glance. However, this results in the internal outstanding timestamp counter becoming out of sync.
> 
> When capturing a timestamp, hardware increments its internal counter and sets the associated "ready" bit in the timestamp memory status. Then it compares the timestamp count to the threshold to determine > if it should trigger an interrupt to the MAC.
> 
> Upon reading the timestamp hardware is supposed to decrement the counter, clear the valid bit, and clear the associated bit from the memory status register. However, it only performs these steps *if* the valid > bit is set.
> 
> Since the valid bit is not set while PHY_REG_TX_OFFSET_READY is clear, the timestamp counter is not decremented and the memory status is not cleared.
> This leaves the counter out-of-sync until a PHY soft reset.
> 
> According to the hardware engineers, the PHY_REG_TX_OFFSET_READY bit has no other effects. It only controls whether hardware captures timestamps with the valid bit set or not. Since capturing timestamps with the valid bit clear is problematic, they recommend simply not clearing PHY_REG_TX_OFFSET_READY.
> 
> Note that the PHY_REG_RX_OFFSET_READY performs a similar task. However, clearing it is fine as there is no associated timestamp counter on the Rx side. Receive timestamps are simply inserted into the descriptor. Clearing this register clears the valid bit for timestamps until we complete calibration and re-enable the register.
> 
> Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C products")
> Suggested-by: Maciej Machnikowski <maciej.machnikowski@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 4 ----
>  1 file changed, 4 deletions(-)

Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>

RE: [PATCH iwl-net 06/12] ice: E825: clear PHY_REG_TX_MEMORY_STATUS prior to soft reset

[Nowlin, Alexander]

> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@intel.com> 
> Sent: Friday, August 21, 2026 5:13 PM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: netdev@vger.kernel.org; Machnikowski, Maciej <maciej.machnikowski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Korba, Przemyslaw <przemyslaw.korba@intel.com>; Nitka, Grzegorz <grzegorz.nitka@intel.com>; Oros, Petr <poros@redhat.com>; Nowlin, Alexander <alexander.nowlin@intel.com>; Bross, Kevin <kevin.bross@intel.com>; Cavatur, Ranjit <ranjit.cavatur@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>; Machnikowski, Maciej <maciej.machnikowski@intel.com>
> Subject: [PATCH iwl-net 06/12] ice: E825: clear PHY_REG_TX_MEMORY_STATUS prior to soft reset
> 
> The drivers current implementation of ice_ptp_reset_ts_memory_eth56g() is flawed. It tries to clear the timestamp memory by writing to the PHY_REG_TX_MEMORY_STATUS region. This does not work properly, as it does not trigger appropriate PHY actions.
> 
> To clear outstanding timestamp memory, the driver must read the timestamps.
> However, naively doing this as part of ice_ptp_reset_ts_memory() is problematic. When reading the timestamp index, hardware kicks off a chain of actions including clearing the ready bitmap index, and decrementing an internal counter if the timestamp index was marked as valid.
> 
> This can potentially leave the internal hardware counter out of sync with the actual number of timestamps. This occurs because the PHY_REG_TX_MEMORY_STATUS region is not zero-initialized when the device > boots up. Instead, it is filled with garbage. On a cold power on, attempts to read the stale data result in the hardware triggering a counter decrement for a timestamp that never happened. This underflows the counter, and prevents new timestamp interrupts from being triggered for real timestamp requests.
> 
> We must read the PHY_REG_TX_MEMORY_STATUS in order to clear stale timestamps. But doing so may cause a desync with the counter. To prevent issues, perform this clearing always and only right before initiating a PHY soft reset.
> 
> The soft reset will clear and reset the internal counter and the ready bitmap. The reads to PHY_REG_TX_MEMORY_STATUS will reset the region valid bits ensuring that no stale data is left behind. This combination ensures that we always have a clean slate with no stale data and with the counter properly reset to zero.
> 
> Fixes: 3ec46e157c7f ("ice: perform PHY soft reset for E825C ports at initialization")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 89 +++++++++++++++--------------
>  1 file changed, 47 insertions(+), 42 deletions(-)

Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>

RE: [PATCH iwl-net 07/12] ice: E825: perform a soft reset when starting the PHY timer

[Nowlin, Alexander]

> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@intel.com> 
> Sent: Friday, August 21, 2026 5:13 PM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: netdev@vger.kernel.org; Machnikowski, Maciej <maciej.machnikowski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Korba, Przemyslaw <przemyslaw.korba@intel.com>; Nitka, Grzegorz <grzegorz.nitka@intel.com>; Oros, Petr <poros@redhat.com>; Nowlin, Alexander <alexander.nowlin@intel.com>; Bross, Kevin <kevin.bross@intel.com>; Cavatur, Ranjit <ranjit.cavatur@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>; Machnikowski, Maciej <maciej.machnikowski@intel.com>
> Subject: [PATCH iwl-net 07/12] ice: E825: perform a soft reset when starting the PHY timer
> 
> To ensure that the E825 PHY timer begins in a clean state, initiate a PHY soft reset prior to programming the PHY. This ensures that we clear any outstanding Tx timestamp memory, and ensures that the PHY internal state has been completely reset.
> 
> Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C products")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)

Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>

RE: [PATCH iwl-net 08/12] ice: wait for in-flight Tx timestamps before flushing the tracker

[Nowlin, Alexander]

> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@intel.com> 
> Sent: Friday, August 21, 2026 5:13 PM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: netdev@vger.kernel.org; Machnikowski, Maciej <maciej.machnikowski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Korba, Przemyslaw <przemyslaw.korba@intel.com>; Nitka, Grzegorz <grzegorz.nitka@intel.com>; Oros, Petr <poros@redhat.com>; Nowlin, Alexander <alexander.nowlin@intel.com>; Bross, Kevin <kevin.bross@intel.com>; Cavatur, Ranjit <ranjit.cavatur@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>; Machnikowski, Maciej <maciej.machnikowski@intel.com>
> Subject: [PATCH iwl-net 08/12] ice: wait for in-flight Tx timestamps before flushing the tracker
> 
> From: Petr Oros <poros@redhat.com>
> 
> ice_ptp_flush_tx_tracker() frees every tracked request, but a request whose timestamp is still being captured by the PHY at that moment is freed without touching the PHY entry. The ready bit published shortly > after has no tracked owner, and the PHY does not raise another Tx timestamp interrupt until every outstanding ready bit is read, so delivery for the whole quad degrades to the periodic work.
> 
> Wait up to 10 ms for in-flight captures to publish their ready bits before flushing, so the flush clears them together with the rest.
> 
> Fixes: ea9b847cda64 ("ice: enable transmit timestamps for E810 devices")
> Signed-off-by: Petr Oros <poros@redhat.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.c | 33 ++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)

Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>

RE: [PATCH iwl-net 09/12] ice: keep Tx timestamp slots tracked until completion or timeout

[Nowlin, Alexander]

> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@intel.com> 
> Sent: Friday, August 21, 2026 5:13 PM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: netdev@vger.kernel.org; Machnikowski, Maciej <maciej.machnikowski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Korba, Przemyslaw <przemyslaw.korba@intel.com>; Nitka, Grzegorz <grzegorz.nitka@intel.com>; Oros, Petr <poros@redhat.com>; Nowlin, Alexander <alexander.nowlin@intel.com>; Bross, Kevin <kevin.bross@intel.com>; Cavatur, Ranjit <ranjit.cavatur@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>; Machnikowski, Maciej <maciej.machnikowski@intel.com>
> Subject: [PATCH iwl-net 09/12] ice: keep Tx timestamp slots tracked until completion or timeout
> 
> From: Petr Oros <poros@redhat.com>
> 
> When the link goes down the processing loop drops every outstanding request, and a request whose timestamp is not ready yet is freed without reading the PHY slot. The hardware completes the capture a moment later, the orphaned ready bit blocks the port interrupt until the next link-up sweep, and the freed index can meanwhile be reused by a new request whose slot the hardware then overwrites. Captured on > a reproducer as ready bits with no in_use owner right after a link bounce.
> 
> Stop dropping on link down. Mark the outstanding requests stale so their completions are read and discarded, reject new requests while the link is down, and free a not yet ready slot only after the two second timeout. This way an index is never reused while the hardware can still write it and never left untracked while a completion can still arrive.
> 
> To avoid an IRQ storm in the event that we really do have a stale packet that is not timestamped, modify ice_ptp_tx_tstamps_pending() to ignore stale timestamps when checking for whether to re-arm the IRQ from the miscellaneous thread function. Instead, only check for stale packets in the auxiliary work thread. This way we do not check in a tight loop waiting for a timestamp that may never come.
> 
> This effectively reverts commit fcc2cef37fed ("ice/ptp: fix the PTP worker retrying indefinitely if the link went down"), which tried to release an index before this 2 second wait period.
> 
> Fixes: fcc2cef37fed ("ice/ptp: fix the PTP worker retrying indefinitely if the link went down")
> Suggested-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Petr Oros <poros@redhat.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.h  |  8 ++++--  drivers/net/ethernet/intel/ice/ice_main.c |  2 +-  drivers/net/ethernet/intel/ice/ice_ptp.c  | 44 +++++++++++++++----------------
>  3 files changed, 28 insertions(+), 26 deletions(-)

Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>

RE: [PATCH iwl-net 10/12] ice: remove unnecessary discarding of timestamps after clock adjust

[Nowlin, Alexander]

> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@intel.com> 
> Sent: Friday, August 21, 2026 5:13 PM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: netdev@vger.kernel.org; Machnikowski, Maciej <maciej.machnikowski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Korba, Przemyslaw <przemyslaw.korba@intel.com>; Nitka, Grzegorz <grzegorz.nitka@intel.com>; Oros, Petr <poros@redhat.com>; Nowlin, Alexander <alexander.nowlin@intel.com>; Bross, Kevin <kevin.bross@intel.com>; Cavatur, Ranjit <ranjit.cavatur@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>; Machnikowski, Maciej <maciej.machnikowski@intel.com>
> Subject: [PATCH iwl-net 10/12] ice: remove unnecessary discarding of timestamps after clock adjust
> 
> The ice driver currently discards any outstanding timestamps that are happening very near to a .adjtime or .settime callback. This was originally add by commit d40fd6009332 ("ice: handle flushing stale Tx timestamps in ice_ptp_tx_tstamp").
> 
> The original motivation for discarding timestamps was that extending an old timestamp using the new cached value of PHC was a problem, as it could produce incorrect results. The change did not describe what > such "incorrect results" were.
> 
> There are no such incorrect results. Extending the 32 bit timestamp with the new time value just means that the timestamp is reported in terms of the newly updated and adjusted system clock. This won't produce incorrect results or problematic timestamps to applications. Either the timestamp will be extended with the value of the PHC just prior to the time adjustment (if the timestamp completes prior to the adjust callback), or it will be extended using the new PHC value after the adjustment. In either case, the resulting extended timestamp value makes sense.
> 
> The timestamp extension logic is very similar to the logic found in timecounter_cyc2time, the primary difference being that the ice hardware maintains the full 64 bits of nanoseconds in the MAC rather than being maintained purely by software as in the timecounter case.
> 
> Indeed, I couldn't find an example of a driver using timecounter_cyc2time which does discard timestamps that occur nearby a time adjustment. The ice driver behavior of discarding such timestamps just results > in failure to deliver a Tx timestamp to userspace, resulting in applications such as ptp4l to timeout and enter a fault state. Reporting the extended timestamp based on the updated PHC value isn't producing "garbage" results, and doesn't lead to incorrect behavior.
> 
> This effectively reverts commit d40fd6009332 ("ice: handle flushing stale Tx timestamps in ice_ptp_tx_tstamp"). However, the stale logic remains, as we now use it to inform the driver to drop timestamps which > might fail due to link down.
> 
> Fixes: d40fd6009332 ("ice: handle flushing stale Tx timestamps in ice_ptp_tx_tstamp")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)

Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>

RE: [PATCH iwl-net 11/12] ice: skip reading Tx ready bitmap on ports with no timestamps

[Nowlin, Alexander]

> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@intel.com> 
> Sent: Friday, August 21, 2026 5:13 PM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: netdev@vger.kernel.org; Machnikowski, Maciej <maciej.machnikowski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Korba, Przemyslaw <przemyslaw.korba@intel.com>; Nitka, Grzegorz <grzegorz.nitka@intel.com>; Oros, Petr <poros@redhat.com>; Nowlin, Alexander <alexander.nowlin@intel.com>; Bross, Kevin <kevin.bross@intel.com>; Cavatur, Ranjit <ranjit.cavatur@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>
> Subject: [PATCH iwl-net 11/12] ice: skip reading Tx ready bitmap on ports with no timestamps
> 
> On E82x devices, the interrupt for Tx timestamps are handled by the clock owner. When an interrupt with the Tx timestamp cause is fired, the clock owner PF iterates the list of ports and checks for timestamps across all ports.
> 
> The existing logic reads the PHY timestamp ready bitmap before iterating the list of in-use timestamp indexes, even for ports which have no timestamps waiting in the software timestamp tracker. This has a significant and measurable latency impact on reporting Tx timestamps.
> 
> Check the bitmap and exit early in the event that there are no timestamps waiting on a port. Observant reviewers may notice that the check is done without acquiring the lock. This is fine, as the only thread that > can clear in_use bits is the miscellaneous interrupt handler. Whether the thread sees or fails to see a new outstanding timestamp does not affect correctness, only determining whether or not it should do extra > work.
> 
> Using the ice Tx timestamp traces, with a simple ptp4l setup the average latency appears to be around 175 to 200 microseconds with a few outliers taking hundreds of microseconds to be reported. With the check to skip empty bitmaps (and thus skip reading the ready bitmap for inactive ports), the average latency drops ~50 microseconds.
> 
> Fixes: d938a8cca88a ("ice: Auxbus devices & driver for E822 TS")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>

RE: [PATCH iwl-net 12/12] ice: don't clear in_use until HW clears ready bitmap

[Nowlin, Alexander]

> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@intel.com> 
> Sent: Friday, August 21, 2026 5:13 PM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: netdev@vger.kernel.org; Machnikowski, Maciej <maciej.machnikowski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Korba, Przemyslaw <przemyslaw.korba@intel.com>; Nitka, Grzegorz <grzegorz.nitka@intel.com>; Oros, Petr <poros@redhat.com>; Nowlin, Alexander <alexander.nowlin@intel.com>; Bross, Kevin <kevin.bross@intel.com>; Cavatur, Ranjit <ranjit.cavatur@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>
> Subject: [PATCH iwl-net 12/12] ice: don't clear in_use until HW clears ready bitmap
> 
> During a link down transition, the E825 PHY has a small window where it does not properly respond to reading the PHY timestamp registers. When this occurs, the PHY does not automatically clear the ready bitmap or the valid bit for the timestamp. This begins happening slightly before a link transition even before the firmware has notified the driver of the state change.
> 
> The driver happily completes the timestamp, releasing the in_use bit. This allows another request to reuse the bit potentially reporting an invalid stale timestamp. Additionally, with the ready bit still set high the > driver continues to re-trigger the IRQ and check for timestamps in a tight loop, wasting CPU cycles.
> 
> To fix this, re-read the PHY timestamp memory status after each read of a PHY index. Double check if the hardware cleared the index properly. If it hasn't, mark the timestamp index as stale and skip processing it.
> 
> Stale timestamps are already ignored by the ice_any_port_has_timestamps() function. However, the ice_ptp_tx_tstamps_pending() function also checks the ready bitmap. Instead, modify it to only check the software tracker.
> Additionally, stop re-triggering the interrupt from the IRQ if the timestamp tracker is calibrating or has the link marked as down. Continue to check the hardware ready bitmap from the watchdog to catch cases of unexpected timestamps.
> 
> With these changes, the timestamp processing no longer triggers a repeated spamming of the IRQ during link down events where timestamps get stuck as the PHY transitions to link down. Once link is restored, the PHY will be reset and the stuck timestamps are cleared.
> 
> Measuring CPU utilization of the miscellaneous IRQ thread function during timestamp storms near a link reset shows that this prevents the spikes caused by the "stuck" ready bit. Without this fix, the CPU handling the IRQ becomes slammed due to the IRQ re-triggering logic.
> 
> Measuring latency using the ice Tx timestamp traces does show that this fix comes at a latency cost. Latency is measured using the ice Tx timestamp traces for the request to completion time. I measured a couple of different workloads both before and after this fix:
> 
>  * ptp4l using a profile with ~16 SYNC messages per second
> 
>     before: 159.40 microseconds mean, stdev 45.28
>      after: 182.07 microseconds mean,  stdev 43.43
> 
>  * a C program generating 16 timestamp requests every 10 milliseconds on
>    two different ports:
> 
>     before: 604.35 microseconds mean, stdev 345.32
>      after: 990.13 microseconds mean, stdev 625.64
> 
> In the normal work flows this comes with about a 20 microsecond penalty on the average, and the standard deviation remains approximately the same. For heavy workloads with many more timestamps than expected for typical applications this comes at a significant cost. This is because we handle all timestamps in a single thread. If there are many concurrent timestamps being requested at once, any which use the later slots on ports later in the port list will take much longer to be processed once the interrupt is fired. Since each timestamp now requires an additional PHY register access, this cost is much higher in the case where the device is under unusually heavy load.
> 
> However, *correctness* is more important than speed here. Additionally, we still remain well below the default limit of 10 milliseconds that ptp4l will wait before complaining about missing timestamps.
> 
> Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C products")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.c | 54 +++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 28 deletions(-)

Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>

RE: [PATCH iwl-net 06/12] ice: E825: clear PHY_REG_TX_MEMORY_STATUS prior to soft reset

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Jacob Keller <jacob.e.keller@intel.com>
> Sent: Saturday, August 22, 2026 2:13 AM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: netdev@vger.kernel.org; Machnikowski, Maciej
> <maciej.machnikowski@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Korba, Przemyslaw
> <przemyslaw.korba@intel.com>; Nitka, Grzegorz
> <grzegorz.nitka@intel.com>; Oros, Petr <poros@redhat.com>; Nowlin,
> Alexander <alexander.nowlin@intel.com>; Bross, Kevin
> <kevin.bross@intel.com>; Cavatur, Ranjit <ranjit.cavatur@intel.com>;
> Keller, Jacob E <jacob.e.keller@intel.com>; Machnikowski, Maciej
> <maciej.machnikowski@intel.com>
> Subject: [PATCH iwl-net 06/12] ice: E825: clear
> PHY_REG_TX_MEMORY_STATUS prior to soft reset
> 
> The drivers current implementation of ice_ptp_reset_ts_memory_eth56g()
Probably 'The drivers current implementation' -> 'The driver's current implementation' ?


> is flawed. It tries to clear the timestamp memory by writing to the
> PHY_REG_TX_MEMORY_STATUS region. This does not work properly, as it
> does not trigger appropriate PHY actions.
> 
> To clear outstanding timestamp memory, the driver must read the
> timestamps.
> However, naively doing this as part of ice_ptp_reset_ts_memory() is
> problematic. When reading the timestamp index, hardware kicks off a
> chain of actions including clearing the ready bitmap index, and
> decrementing an internal counter if the timestamp index was marked as
> valid.
> 
> This can potentially leave the internal hardware counter out of sync
> with the actual number of timestamps. This occurs because the
> PHY_REG_TX_MEMORY_STATUS region is not zero-initialized when the
> device boots up. Instead, it is filled with garbage. On a cold power
> on, attempts to read the stale data result in the hardware triggering
> a counter decrement for a timestamp that never happened. This
> underflows the counter, and prevents new timestamp interrupts from
> being triggered for real timestamp requests.
> 
> We must read the PHY_REG_TX_MEMORY_STATUS in order to clear stale
> timestamps. But doing so may cause a desync with the counter. To
> prevent issues, perform this clearing always and only right before
> initiating a PHY soft reset.
> 
> The soft reset will clear and reset the internal counter and the ready
> bitmap. The reads to PHY_REG_TX_MEMORY_STATUS will reset the region
> valid bits ensuring that no stale data is left behind. This
> combination ensures that we always have a clean slate with no stale
> data and with the counter properly reset to zero.
> 
> Fixes: 3ec46e157c7f ("ice: perform PHY soft reset for E825C ports at
> initialization")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 89 +++++++++++++++-----
> ---------
>  1 file changed, 47 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index d48eb3c61823..b7d217ac31f3 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> @@ -737,24 +737,6 @@ static int ice_read_port_mem_eth56g(struct ice_hw
> *hw, u8 port, u16 offset,
>  	return ice_read_port_eth56g(hw, port, offset, val,
> ETH56G_PHY_MEM_PTP);  }
> 
> -/**
> - * ice_write_port_mem_eth56g - Write a PHY port memory location
> - * @hw: pointer to the HW struct
> - * @port: Port number to be read
> - * @offset: Offset from PHY port register base
> - * @val: Pointer to the value to read (out param)
> - *
> - * Return:
> - * * %0      - success
> - * * %EINVAL - invalid port number or resource type
> - * * %other  - failed to write to PHY
> - */
> -static int ice_write_port_mem_eth56g(struct ice_hw *hw, u8 port, u16
> offset,
> -				     u32 val)
> -{
> -	return ice_write_port_eth56g(hw, port, offset, val,
> ETH56G_PHY_MEM_PTP);
> -}
> -
>  /**
>   * ice_write_quad_ptp_reg_eth56g - Write a PHY quad register
>   * @hw: pointer to the HW struct
> @@ -1139,8 +1121,8 @@ static int ice_read_ptp_tstamp_eth56g(struct
> ice_hw *hw, u8 port, u8 idx,
>   * internal PHYs of the 56G devices.
>   *
>   * To directly clear the contents of the timestamp block entirely,
> discarding
> - * all timestamp data at once, software should instead use
> - * ice_ptp_reset_ts_memory_quad_eth56g().
> + * all timestamp data at once, software should instead perform a PHY
> + soft
> + * reset via ice_ptp_phy_soft_reset_eth56g().
>   *
>   * This function should only be called on an idx whose bit is set
> according to
>   * ice_get_phy_tx_tstamp_ready().
> @@ -1152,11 +1134,11 @@ static int ice_read_ptp_tstamp_eth56g(struct
> ice_hw *hw, u8 port, u8 idx,  static int
> ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx)  {
>  	u64 unused_tstamp;
> -	u16 lo_addr;
>  	int err;
> 
> -	/* Read the timestamp register to ensure the timestamp status
> bit is
> -	 * cleared.
> +	/* Per the PHY spec, reading the timestamp memory location is
> what
> +	 * clears the entry's valid bit and its corresponding (read-
> only)
> +	 * ts_memory_status bit.
>  	 */
>  	err = ice_read_ptp_tstamp_eth56g(hw, port, idx,
> &unused_tstamp);
>  	if (err) {
> @@ -1164,32 +1146,40 @@ static int ice_clear_ptp_tstamp_eth56g(struct
> ice_hw *hw, u8 port, u8 idx)
>  			  port, idx, err);
>  	}
> 
> -	lo_addr = (u16)PHY_TSTAMP_L(idx);
> -
> -	err = ice_write_port_mem_eth56g(hw, port, lo_addr, 0);
> -	if (err) {
> -		ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP
> timestamp register for port %u, idx %u, err %d\n",
> -			  port, idx, err);
> -		return err;
> -	}
> -
>  	return 0;
>  }
> 
>  /**
> - * ice_ptp_reset_ts_memory_eth56g - Clear all timestamps from the
> port block
> + * ice_ptp_clear_tx_memory_status_eth56g - Reset one port's Tx
> + timestamp memory
>   * @hw: pointer to the HW struct
> + * @port: port number to clear
> + *
> + * Fully reset a single PHY port's Tx timestamp memory. Per the PHY
> + spec, the
> + * only way to clear a timestamp valid bit (and its read-only
> + ts_memory_status
> + * bit) is to read the timestamp memory location, so read every entry
> + for the
> + * port (two 32-bit reads each). This discards all timestamp data on
> + the port,
> + * so it must only be used for a full reset; callers that must
> preserve
> + * in-flight timestamps clear individual indices via
> ice_clear_phy_tstamp().
> + *
> + * Due to interactions with an internal HW counter for the number of
> + * outstanding Tx timestamps, this *must* only be called as part of
> the
> + * ice_ptp_phy_soft_reset_eth56g() procedure. Otherwise, the internal
> + counter
> + * may become out of sync and prevent new timestamp interrupts.
> + *
> + * Return: 0 on success, negative error code on failure to read the
> PHY.
>   */
> -static void ice_ptp_reset_ts_memory_eth56g(struct ice_hw *hw)
> +static int ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw,
> u8
> +port)
>  {
> -	unsigned int port;
> +	int err = 0;
> +	u8 idx;
> 
> -	for (port = 0; port < hw->ptp.num_lports; port++) {
> -		ice_write_ptp_reg_eth56g(hw, port,
> PHY_REG_TX_MEMORY_STATUS_L,
> -					 0);
> -		ice_write_ptp_reg_eth56g(hw, port,
> PHY_REG_TX_MEMORY_STATUS_U,
> -					 0);
> +	for (idx = 0; idx < INDEX_PER_PORT; idx++) {
> +		err = ice_clear_ptp_tstamp_eth56g(hw, port, idx);
> +		if (err)
> +			return err;
>  	}
> +
> +	return 0;
>  }
> 
>  /**
> @@ -2290,6 +2280,7 @@ int
> ice_ptp_read_tx_hwtstamp_status_eth56g(struct ice_hw *hw, u32
> *ts_status)
>   *
>   * Trigger a soft reset of the ETH56G PHY by toggling the soft reset
>   * bit in the PHY global register. The reset sequence consists of:
> + *   0. Reading every timestamp memory register to clear its valid
> bit
>   *   1. Clearing the soft reset bit
>   *   2. Asserting the soft reset bit
>   *   3. Clearing the soft reset bit again
> @@ -2298,6 +2289,12 @@ int
> ice_ptp_read_tx_hwtstamp_status_eth56g(struct ice_hw *hw, u32
> *ts_status)
>   * to settle. This provides a controlled way to reinitialize the PHY
>   * without requiring a full device reset.
>   *
> + * To ensure that the internal counter matches the contents of the
> + * PHY_REG_TX_MEMORY_STATUS, read every timestamp index prior to
> + performing
> + * the soft reset. The PHY_REG_TX_MEMORY_STATUS reads ensure that the
> + region
> + * is cleared, while the soft reset procedure ensures that the
> + timestamp
timestamp  -> timestamp


> + * counter is reset to zero.
> + *
>   * Return: 0 on success, or a negative error code on failure when
>   *         reading or writing the PHY register.
>   */
> @@ -2306,6 +2303,13 @@ int ice_ptp_phy_soft_reset_eth56g(struct ice_hw
> *hw, u8 port)
>  	u32 global_val;
>  	int err;
> 
> +	err = ice_ptp_clear_tx_memory_status_eth56g(hw, port);
> +	if (err) {
> +		ice_debug(hw, ICE_DBG_PTP, "Failed to clear
> PHY_REG_TX_MEMORY_STATUS for port %d, err %d\n",
> +			  port, err);
> +		return err;
> +	}
> +
>  	err = ice_read_ptp_reg_eth56g(hw, port, PHY_REG_GLOBAL,
> &global_val);
>  	if (err) {
>  		ice_debug(hw, ICE_DBG_PTP, "Failed to read
> PHY_REG_GLOBAL for port %d, err %d\n", @@ -5797,8 +5801,9 @@ void
> ice_ptp_reset_ts_memory(struct ice_hw *hw)
>  		ice_ptp_reset_ts_memory_e82x(hw);
>  		break;
>  	case ICE_MAC_GENERIC_3K_E825:
> -		ice_ptp_reset_ts_memory_eth56g(hw);
> -		break;
> +		/* E825 hardware must only reset timestamp memory as
> part of
> +		 * the soft reset procedure.
> +		 */
>  	case ICE_MAC_E810:
>  	default:
>  		return;
> 
> --
> 2.55.0.814.gc42f45431d0f


Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

RE: [PATCH iwl-net 12/12] ice: don't clear in_use until HW clears ready bitmap

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Jacob Keller <jacob.e.keller@intel.com>
> Sent: Saturday, August 22, 2026 2:13 AM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: netdev@vger.kernel.org; Machnikowski, Maciej
> <maciej.machnikowski@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Korba, Przemyslaw
> <przemyslaw.korba@intel.com>; Nitka, Grzegorz
> <grzegorz.nitka@intel.com>; Oros, Petr <poros@redhat.com>; Nowlin,
> Alexander <alexander.nowlin@intel.com>; Bross, Kevin
> <kevin.bross@intel.com>; Cavatur, Ranjit <ranjit.cavatur@intel.com>;
> Keller, Jacob E <jacob.e.keller@intel.com>
> Subject: [PATCH iwl-net 12/12] ice: don't clear in_use until HW clears
> ready bitmap
> 
> During a link down transition, the E825 PHY has a small window where
> it does not properly respond to reading the PHY timestamp registers.
> When this occurs, the PHY does not automatically clear the ready
> bitmap or the valid bit for the timestamp. This begins happening
> slightly before a link transition even before the firmware has
> notified the driver of the state change.
> 
> The driver happily completes the timestamp, releasing the in_use bit.
> This allows another request to reuse the bit potentially reporting an
> invalid stale timestamp. Additionally, with the ready bit still set
> high the driver continues to re-trigger the IRQ and check for
> timestamps in a tight loop, wasting CPU cycles.
> 
> To fix this, re-read the PHY timestamp memory status after each read
> of a PHY index. Double check if the hardware cleared the index
> properly. If it hasn't, mark the timestamp index as stale and skip
> processing it.
> 
> Stale timestamps are already ignored by the
> ice_any_port_has_timestamps() function. However, the
> ice_ptp_tx_tstamps_pending() function also checks the ready bitmap.
> Instead, modify it to only check the software tracker.
> Additionally, stop re-triggering the interrupt from the IRQ if the
> timestamp tracker is calibrating or has the link marked as down.
> Continue to check the hardware ready bitmap from the watchdog to catch
> cases of unexpected timestamps.
> 
> With these changes, the timestamp processing no longer triggers a
> repeated spamming of the IRQ during link down events where timestamps
> get stuck as the PHY transitions to link down. Once link is restored,
> the PHY will be reset and the stuck timestamps are cleared.
> 
> Measuring CPU utilization of the miscellaneous IRQ thread function
> during timestamp storms near a link reset shows that this prevents the
> spikes caused by the "stuck" ready bit. Without this fix, the CPU
> handling the IRQ becomes slammed due to the IRQ re-triggering logic.
> 
> Measuring latency using the ice Tx timestamp traces does show that
> this fix comes at a latency cost. Latency is measured using the ice Tx
> timestamp traces for the request to completion time. I measured a
> couple of different workloads both before and after this fix:
> 
>  * ptp4l using a profile with ~16 SYNC messages per second
> 
>     before: 159.40 microseconds mean, stdev 45.28
>      after: 182.07 microseconds mean,  stdev 43.43
> 
>  * a C program generating 16 timestamp requests every 10 milliseconds
> on
>    two different ports:
> 
>     before: 604.35 microseconds mean, stdev 345.32
>      after: 990.13 microseconds mean, stdev 625.64
> 
> In the normal work flows this comes with about a 20 microsecond
> penalty on the average, and the standard deviation remains
> approximately the same. For heavy workloads with many more timestamps
> than expected for typical applications this comes at a significant
> cost. This is because we handle all timestamps in a single thread. If
> there are many concurrent timestamps being requested at once, any
> which use the later slots on ports later in the port list will take
> much longer to be processed once the interrupt is fired. Since each
> timestamp now requires an additional PHY register access, this cost is
> much higher in the case where the device is under unusually heavy
> load.
> 
> However, *correctness* is more important than speed here.
> Additionally, we still remain well below the default limit of 10
> milliseconds that ptp4l will wait before complaining about missing
> timestamps.
> 
> Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C
> products")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.c | 54 +++++++++++++++--------
> ---------
>  1 file changed, 26 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
> b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index b337247c94e0..0e44ed6b0ae8 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -620,6 +620,22 @@ static void ice_ptp_process_tx_tstamp(struct
> ice_ptp_tx *tx)
>  		if (err && !drop_ts)
>  			continue;
> 
> +		/* verify ready bit cleared */
> +		if (tx->has_ready_bitmap) {
> +			err = ice_get_phy_tx_tstamp_ready(hw, tx->block,
> &tstamp_ready);
> +			if (err)
> +				continue;
> +
> +			if (tstamp_ready & BIT_ULL(phy_idx)) {
> +				spin_lock_irqsave(&tx->lock, flags);
> +				if (!test_and_set_bit(idx, tx->stale))
> +					dev_dbg(ice_pf_to_dev(pf), "PHY port
> %u failed to clear ready bit for idx %u\n",
> +						ptp_port->port_num, phy_idx);
> +				spin_unlock_irqrestore(&tx->lock, flags);
> +				continue;
> +			}
> +		}
> +
>  		ice_trace(tx_tstamp_fw_done, tx->tstamps[idx].skb, idx);
> 
>  		/* For PHYs which don't implement a proper timestamp
> ready @@ -2764,10 +2780,14 @@ static bool
> ice_port_has_timestamps(struct ice_ptp_tx *tx, bool in_irq)
>  		if (!tx->init)
>  			return false;
> 
> -		if (in_irq)
> +		if (in_irq) {
> +			if (!ice_ptp_is_tx_tracker_up(tx))
> +				return false;
> +
>  			return bitmap_andnot(tstamps, tx->in_use, tx-
> >stale, tx->len);
> -		else
> +		} else {
>  			return !bitmap_empty(tx->in_use, tx->len);
> +		}
>  	}
>  }
> 
> @@ -2790,41 +2810,18 @@ static bool ice_any_port_has_timestamps(struct
> ice_pf *pf, bool in_irq)
> 
>  bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf, bool in_irq)  {
> -	struct ice_hw *hw = &pf->hw;
> -	int ret;
> -
> -	/* Check software indicator */
>  	switch (pf->ptp.tx_interrupt_mode) {
>  	case ICE_PTP_TX_INTERRUPT_NONE:
>  		return false;
>  	case ICE_PTP_TX_INTERRUPT_SELF:
> -		if (ice_port_has_timestamps(&pf->ptp.port.tx, in_irq))
> -			return true;
> -		break;
> +		return ice_port_has_timestamps(&pf->ptp.port.tx,
> in_irq);
>  	case ICE_PTP_TX_INTERRUPT_ALL:
> -		if (ice_any_port_has_timestamps(pf, in_irq))
> -			return true;
> -		break;
> +		return ice_any_port_has_timestamps(pf, in_irq);
>  	default:
>  		WARN_ONCE(1, "Unexpected Tx timestamp interrupt mode
> %u\n",
>  			  pf->ptp.tx_interrupt_mode);
> -		break;
> -	}
> -
> -	/* Check hardware indicator */
> -	ret = ice_check_phy_tx_tstamp_ready(hw);
> -	if (ret < 0) {
> -		dev_dbg(ice_pf_to_dev(pf), "Unable to read PHY Tx
> timestamp ready bitmap, err %d\n",
> -			ret);
> -		/* Stop triggering IRQs if we're unable to read PHY */
>  		return false;
>  	}
> -
> -	/* ice_check_phy_tx_tstamp_ready() returns 1 if there are
> timestamps
> -	 * available, 0 if there are no waiting timestamps, and a
> negative
> -	 * value if there was an error (which we checked for above).
> -	 */
> -	return ret > 0;
>  }
> 
>  /**
> @@ -2915,7 +2912,8 @@ static void
> ice_ptp_maybe_trigger_tx_interrupt(struct ice_pf *pf)
>  	if (!ice_pf_src_tmr_owned(pf))
>  		return;
> 
> -	if (ice_ptp_tx_tstamps_pending(pf, false)) {
> +	if (ice_ptp_tx_tstamps_pending(pf, false) ||
> +	    ice_check_phy_tx_tstamp_ready(hw)) {
Not obvious conversion of returned -EOPNOTSUPP and other errors as bool.


>  		dev_dbg(dev, "PTP periodic task detected waiting
> timestamps. Triggering Tx timestamp interrupt now.\n");
> 
>  		wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);
> 
> --
> 2.55.0.814.gc42f45431d0f

Re: [PATCH iwl-net 01/12] ice: use reference counting and RCU for PTP port access

[Jacob Keller]

On 8/21/2026 5:13 PM, Jacob Keller wrote:
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
> index c4b0da7ce20e..da2003ba3bb0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.h
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
> @@ -673,20 +675,33 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
>  	pf->ptp.tx_hwtstamp_good += tstamp_good;
>  }
>  
> +static void ice_ptp_release_port_rcu(struct kref *ref)
> +{
> +	wake_up_var(ref);
> +}
> +
>  static void ice_ptp_tx_tstamp_owner(struct ice_pf *pf)
>  {
>  	struct ice_ptp_port *port;
>  
> -	mutex_lock(&pf->adapter->ports.lock);
> -	list_for_each_entry(port, &pf->adapter->ports.ports, list_node) {
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(port, &pf->adapter->ports.list, list_node) {
>  		struct ice_ptp_tx *tx = &port->tx;
>  
> -		if (!tx || !tx->init)
> +		if (!tx->init)
>  			continue;
>  
> +		if (!kref_get_unless_zero(&port->ref))
> +			continue;
> +
> +		rcu_read_unlock();
> +
>  		ice_ptp_process_tx_tstamp(tx);
> +
> +		kref_put(&port->ref, ice_ptp_release_port_rcu);
> +		rcu_read_lock();
>  	}
> -	mutex_unlock(&pf->adapter->ports.lock);
> +	rcu_read_unlock();
>  }
>  

Sashiko says:

> Can this sequence lead to a use-after-free when advancing the iterator?
> The list_for_each_entry_rcu() macro evaluates port->list_node.next at the end
> of each iteration to find the next element.
> If kref_put() drops the last reference here, it wakes up the teardown thread
> in ice_ptp_cleanup_pf().
> Since the iterator thread previously dropped the RCU read lock via
> rcu_read_unlock(), could the following race happen?
> Iterator thread:
>     kref_put(&port->ref, ice_ptp_release_port_rcu);
>     // Last ref dropped, wakes up ice_ptp_cleanup_pf()
> Teardown thread (ice_ptp_cleanup_pf):
>     wakes up from wait_var_event_timeout()
>     synchronize_rcu(); // Completes without waiting for iterator thread
>     // Port memory is freed
> Iterator thread:
>     rcu_read_lock();
>     // list_for_each_entry_rcu evaluates port->list_node.next on freed port
> Is it necessary to use a safe iterator or pre-fetch the next pointer before
> dropping the reference, to prevent reading from the freed port?
> This same traversal pattern dropping the lock and reference appears to also
> be present in ice_ptp_flush_all_tx_tracker(), ice_ptp_restart_all_phy(),
> and ice_ptp_prepare_rebuild_sec().

I am not sure there is a problem here. I need to investigate carefully.

I think there is a problem here because the original logic was built
around xarray iterators which may not work exactly the same as the list
iterator. We currently remove the item from the list before releasing
the final reference.

I believe that worked for the xarray but doesn't work for this list
based approach. I think the best solution here is to follow the guidance
from Documentation/core-api/kref.rst under the kref + RCU section.

I will fix this in a v2, along with any other issues pointed out by sashiko

Re: [PATCH iwl-net 02/12] ice: E822: keep Tx timestamps disabled during offset calibration

[Jacob Keller]

On 8/21/2026 5:13 PM, Jacob Keller wrote:
> From: Karol Kolacinski <karol.kolacinski@intel.com>
> 
> Do not clear the tx.calibrating flag immediately after starting the PHY
> timer in ice_ptp_port_phy_restart(). Instead, keep Tx timestamps
> disabled until the offset verification work (ice_ptp_wait_for_offsets)
> has confirmed that both Tx and Rx PHY offsets are properly configured.
> 
> Previously, tx.calibrating was set to true, then immediately back to
> false right after ice_start_phy_timer_e82x() returned. This allowed Tx
> timestamp requests to be served during the window where offset
> verification was still pending. Timestamps produced during this window
> use uncalibrated PHY offsets and can produce incorrect values.
> 
> When ptp4l receives incorrect timestamps, it may reject them and wait
> for the next sync interval (typically 1-2 seconds), compounding delays
> during link cycling. This contributes to the time transmitter port
> becoming unresponsive after repeated link down/up cycles.
> 
> Move the tx.calibrating = false to ice_ptp_wait_for_offsets(), after
> both Tx and Rx offset configuration has completed successfully. This
> ensures that Tx timestamps are only reported with properly calibrated
> PHY offsets.
> 
> If ice_start_phy_timer_e82x() fails, restore calibrating to false on
> the error path to prevent permanently disabling Tx timestamps when
> ov_work is never queued.
> 
> Log a debug message while offset calibration is still pending, including
> the specific Tx/Rx error codes to aid debugging stalled calibration.
> This path is expected on every routine link-up: ov_work is first queued
> with no delay and the vernier offset cannot be computed until at least
> one packet has been transmitted, so the first several invocations
> normally land here. Use dev_dbg() rather than a rate-limited warning to
> avoid emitting KERN_WARNING on every link-up during normal operation.
> Log a debug message when calibration completes successfully.
> 
> Fixes: 3a7496234d17 ("ice: implement basic E822 PTP support")
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.c | 36 ++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 1d647ccce7c4..9d99cbb42463 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -1266,13 +1283,20 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
>  
>  		/* Start the PHY timer in Vernier mode */
>  		err = ice_start_phy_timer_e82x(hw, port);
> -		if (err)
> +		if (err) {
> +			spin_lock_irqsave(&ptp_port->tx.lock, flags);
> +			ptp_port->tx.calibrating = false;
> +			spin_unlock_irqrestore(&ptp_port->tx.lock, flags);

Sashiko says:

> When ice_start_phy_timer_e82x() fails, does setting tx.calibrating to false
> cause timestamp requests to be accepted even though the PHY timer is not
> running?
> If tx.calibrating is false, the requests will be accepted but since the
> timer isn't running, the hardware won't generate completions. This appears
> to cause skb references to be held until they time out (e.g. after 2
> seconds) rather than failing fast, which can exhaust the Tx tracker.
> Since the next call to ice_ptp_port_phy_restart() would set calibrating
> back to true before attempting to start the timer again anyway, would it be
> safer to leave calibrating as true on this error path so that subsequent
> requests are immediately rejected?


This was done intentionally in the original patch and called out in the
commit message. I don't know for sure which is better. Need to get input
from Arek on this.

Perhaps its better to just leave this disabled since the timestamps
won't work properly until the timer does actually get initialized properly.

>  			break;
> +		}
>  
> -		/* Enable Tx timestamps right away */
> -		spin_lock_irqsave(&ptp_port->tx.lock, flags);
> -		ptp_port->tx.calibrating = false;
> -		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
> +		/* Do not clear calibrating flag here. Tx timestamps remain
> +		 * disabled until ice_ptp_wait_for_offsets() has verified
> +		 * that the Tx and Rx offset calibration has completed.
> +		 * Clearing it here would allow Tx timestamps to be reported
> +		 * before the PHY offset registers are configured, leading
> +		 * to incorrect timestamp values.
> +		 */
>  
>  		kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work,
>  					   0);
> 

Re: [PATCH iwl-net 04/12] ice: call PTP link change only from link events

[Jacob Keller]

On 8/21/2026 5:13 PM, Jacob Keller wrote:
> From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> 
> Remove redundant ice_ptp_link_change() calls from ice_up_complete() and
> ice_down(). These duplicate the call already made from
> ice_handle_link_event(), creating three problems:
> 
> 1. Double initialization on link-up: ice_handle_link_event() calls
>    ice_ptp_link_change(true), then ice_up_complete() calls it again.
>    The second call re-enters ice_ptp_port_phy_restart(), re-setting the
>    calibrating flag and restarting the PHY timer while the first
>    invocation's offset verification work (ov_work) may still be running.
> 
> 2. Premature cleanup on administrative down: ice_down() calls
>    ice_ptp_link_change(false) during ifconfig down or reset preparation,
>    even when the physical link is still up. This clears timestamp state
>    unnecessarily and can interfere with ongoing PTP operations.
> 
> 3. Ordering dependency: ice_down()/ice_up_complete() are called during
>    reset sequences where PTP may not be fully initialized, creating
>    edge cases with partially configured state.
> 
> The link event handler is the correct and sufficient place to drive PTP
> link state changes, as it reflects actual physical link transitions. Remove
> the calls of ice_ptp_link_change from the ice_down()/ice_up() flows.
> 
> Initialize the link_up in ice_ptp_init() and ensure that we check and
> restore the link status at the end of the rebuild flow, ensuring that we
> initialize the PHY timer appropriately after a reset.
> 
> Fixes: 6b1ff5d39228 ("ice: always call ice_ptp_link_change and make it void")
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
> Signed-off-by: Petr Oros <poros@redhat.com>
> Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_main.c | 11 +++++++--
>  drivers/net/ethernet/intel/ice/ice_ptp.c  | 38 +++++++++++++++++++++++--------
>  2 files changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index d88835482d3a..bb631ae9e67d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -7794,6 +7792,15 @@ static void ice_rebuild(struct ice_pf *pf, enum ice_reset_req reset_type)
>  
>  	ice_update_pf_netdev_link(pf);
>  
> +	if (test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags) && pf->hw.port_info) {
> +		bool link_up;
> +
> +		link_up = !!(pf->hw.port_info->phy.link_info.link_info &
> +			     ICE_AQ_LINK_UP);
> +		if (pf->ptp.port.link_up != link_up)
> +			ice_ptp_link_change(pf, link_up);
> +	}
> +


Sashiko says:

> Does conditionally skipping ice_ptp_link_change() here leave the non-owner PF
> PHY timer stopped indefinitely after a global reset if the link remains up?
> During a global reset (like EMPR or GLOBR), concurrent rebuilds are triggered
> across all PFs. The clock-owner PF iterates over all adapter ports in
> ice_ptp_rebuild_owner() and calls ice_ptp_port_phy_restart().
> If the clock-owner PF executes this before a non-owner PF has finished
> ice_init_all_ctrlq() or cleared its reset state, the SBQ command fails.
> Previously, the unconditional call to ice_ptp_link_change() masked this race
> by forcing a successful retry. Since ice_down() no longer clears link_up, and
> the physical link may stay UP across the reset, the condition
> pf->ptp.port.link_up != link_up will evaluate to false, leaving the non-owner
> PF's PHY timer stopped indefinitely.

I think this is correct. We need to ensure that we restart the PHY
somewhere in the chain after the CQ is active and the timer has been
reconfigured by the clock owner. Will investigate.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index b8647a39db6d..8aa49dda90a2 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -1327,14 +1327,14 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
>  	struct ice_ptp_port *ptp_port;
>  	struct ice_hw *hw = &pf->hw;
>  
> -	if (pf->ptp.state != ICE_PTP_READY)
> -		return;
> -
>  	ptp_port = &pf->ptp.port;
>  
>  	/* Update cached link status for this port immediately */
>  	ptp_port->link_up = linkup;
>  
> +	if (pf->ptp.state != ICE_PTP_READY)
> +		return;
> +


Sashiko says:

> This is a pre-existing issue, but is there a Time-of-Check to Time-of-Use
> (TOCTOU) race between checking ICE_PTP_READY here and module unload?
> In ice_remove(), ice_unload() is called which invokes ice_ptp_release().
> This destroys the PTP port mutex and the kworker. However, the background
> service task is not stopped until ice_deinit_dev() is called much later.
> If the service task processes a link event concurrently, ice_ptp_link_change()
> checks ICE_PTP_READY without synchronization. If preempted immediately after
> this check, ice_ptp_release() can destroy the resources.
> When ice_ptp_link_change() resumes, it calls ice_ptp_port_phy_restart()
> which attempts to acquire the destroyed ps_lock and queues work on the freed
> kworker, potentially causing a use-after-free or NULL pointer dereference.

I think this is best fixed by having ice_ptp_link_change acquire a
reference using the kref in the first patch to ensure that the port
can't be removed until the function exits. Will fix this in the first patch.

Re: [PATCH iwl-net 06/12] ice: E825: clear PHY_REG_TX_MEMORY_STATUS prior to soft reset

[Jacob Keller]

On 8/21/2026 5:13 PM, Jacob Keller wrote:
> The drivers current implementation of ice_ptp_reset_ts_memory_eth56g() is
> flawed. It tries to clear the timestamp memory by writing to the
> PHY_REG_TX_MEMORY_STATUS region. This does not work properly, as it does
> not trigger appropriate PHY actions.
> 
> To clear outstanding timestamp memory, the driver must read the timestamps.
> However, naively doing this as part of ice_ptp_reset_ts_memory() is
> problematic. When reading the timestamp index, hardware kicks off a chain
> of actions including clearing the ready bitmap index, and decrementing an
> internal counter if the timestamp index was marked as valid.
> 
> This can potentially leave the internal hardware counter out of sync with
> the actual number of timestamps. This occurs because the
> PHY_REG_TX_MEMORY_STATUS region is not zero-initialized when the device
> boots up. Instead, it is filled with garbage. On a cold power on, attempts
> to read the stale data result in the hardware triggering a counter
> decrement for a timestamp that never happened. This underflows the counter,
> and prevents new timestamp interrupts from being triggered for real
> timestamp requests.
> 
> We must read the PHY_REG_TX_MEMORY_STATUS in order to clear stale
> timestamps. But doing so may cause a desync with the counter. To prevent
> issues, perform this clearing always and only right before initiating a PHY
> soft reset.
> 
> The soft reset will clear and reset the internal counter and the ready
> bitmap. The reads to PHY_REG_TX_MEMORY_STATUS will reset the region valid
> bits ensuring that no stale data is left behind. This combination ensures
> that we always have a clean slate with no stale data and with the counter
> properly reset to zero.
> 
> Fixes: 3ec46e157c7f ("ice: perform PHY soft reset for E825C ports at initialization")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 89 +++++++++++++++--------------
>  1 file changed, 47 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index d48eb3c61823..b7d217ac31f3 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> @@ -737,24 +737,6 @@ static int ice_read_port_mem_eth56g(struct ice_hw *hw, u8 port, u16 offset,
>  	return ice_read_port_eth56g(hw, port, offset, val, ETH56G_PHY_MEM_PTP);
>  }
>  
> -/**
> - * ice_write_port_mem_eth56g - Write a PHY port memory location
> - * @hw: pointer to the HW struct
> - * @port: Port number to be read
> - * @offset: Offset from PHY port register base
> - * @val: Pointer to the value to read (out param)
> - *
> - * Return:
> - * * %0      - success
> - * * %EINVAL - invalid port number or resource type
> - * * %other  - failed to write to PHY
> - */
> -static int ice_write_port_mem_eth56g(struct ice_hw *hw, u8 port, u16 offset,
> -				     u32 val)
> -{
> -	return ice_write_port_eth56g(hw, port, offset, val, ETH56G_PHY_MEM_PTP);
> -}
> -
>  /**
>   * ice_write_quad_ptp_reg_eth56g - Write a PHY quad register
>   * @hw: pointer to the HW struct
> @@ -1139,8 +1121,8 @@ static int ice_read_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx,
>   * internal PHYs of the 56G devices.
>   *
>   * To directly clear the contents of the timestamp block entirely, discarding
> - * all timestamp data at once, software should instead use
> - * ice_ptp_reset_ts_memory_quad_eth56g().
> + * all timestamp data at once, software should instead perform a PHY soft
> + * reset via ice_ptp_phy_soft_reset_eth56g().
>   *
>   * This function should only be called on an idx whose bit is set according to
>   * ice_get_phy_tx_tstamp_ready().
> @@ -1152,11 +1134,11 @@ static int ice_read_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx,
>  static int ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx)
>  {
>  	u64 unused_tstamp;
> -	u16 lo_addr;
>  	int err;
>  
> -	/* Read the timestamp register to ensure the timestamp status bit is
> -	 * cleared.
> +	/* Per the PHY spec, reading the timestamp memory location is what
> +	 * clears the entry's valid bit and its corresponding (read-only)
> +	 * ts_memory_status bit.
>  	 */
>  	err = ice_read_ptp_tstamp_eth56g(hw, port, idx, &unused_tstamp);
>  	if (err) {
> @@ -1164,32 +1146,40 @@ static int ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx)
>  			  port, idx, err);
>  	}
>  
> -	lo_addr = (u16)PHY_TSTAMP_L(idx);
> -
> -	err = ice_write_port_mem_eth56g(hw, port, lo_addr, 0);
> -	if (err) {
> -		ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register for port %u, idx %u, err %d\n",
> -			  port, idx, err);
> -		return err;
> -	}
> -

Sashiko says:

> Does ice_clear_ptp_tstamp_eth56g() intentionally ignore the error from
> ice_read_ptp_tstamp_eth56g() and unconditionally return 0?
> If the read fails, the timestamp memory valid bits won't be cleared, which
> could leave stale bits intact before the soft reset.
> Additionally, unconditionally returning 0 here makes the error check in the
> newly introduced ice_ptp_clear_tx_memory_status_eth56g() loop dead code:
> 	for (idx = 0; idx < INDEX_PER_PORT; idx++) {
> 		err = ice_clear_ptp_tstamp_eth56g(hw, port, idx);
> 		if (err)
> 			return err; /* This is never reached */
> 	}
> Should ice_clear_ptp_tstamp_eth56g() return err instead of 0 when the PHY
> timestamp read fails?

It makes sense to me that we should bail here if we fail to access the
device. Presumably multiple other accesses would be failing but I think
its not correct to hide the error in the clear timestamp function. Will
fix in v2.

Re: [PATCH iwl-net 07/12] ice: E825: perform a soft reset when starting the PHY timer

[Jacob Keller]

On 8/21/2026 5:13 PM, Jacob Keller wrote:
> To ensure that the E825 PHY timer begins in a clean state, initiate a PHY
> soft reset prior to programming the PHY. This ensures that we clear any
> outstanding Tx timestamp memory, and ensures that the PHY internal state
> has been completely reset.
> 
> Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C products")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index b7d217ac31f3..10e973876608 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> @@ -2119,9 +2119,11 @@ int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port, bool soft_reset)
>   * @hw: pointer to the HW struct
>   * @port: the PHY port to start
>   *
> - * Start the clock of a PHY port. This must be done as part of the flow to
> - * re-calibrate Tx and Rx timestamping offsets whenever the clock time is
> - * initialized or when link speed changes.
> + * Perform a PHY soft reset and then start the clock for the PHY port.
> + *
> + * This must be done as part of the flow to re-calibrate Tx and Rx
> + * timestamping offsets whenever the clock time is initialized or when link
> + * speed changes.
>   *
>   * Return:
>   * * %0     - success
> @@ -2137,6 +2139,10 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
>  
>  	tmr_idx = ice_get_ptp_src_clock_index(hw);
>  
> +	err = ice_ptp_phy_soft_reset_eth56g(hw, port);
> +	if (err)
> +		return err;
> +
>  	err = ice_stop_phy_timer_eth56g(hw, port, false);
>  	if (err)
>  		return err;
> 

Sashiko points out a couple of pre-existing issues that are unrelated to
this change:

> This is a pre-existing issue, but does ice_ptp_update_cached_phctime() use
> the ICE_CFG_BUSY bit flag as an ad-hoc lock?
> In drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_update_cached_phctime(),
> the flag guards iteration over the vsi array:
>     if (test_and_set_bit(ICE_CFG_BUSY, pf->state))
>         return -EAGAIN;
> and then clears it on exit:
>     clear_bit(ICE_CFG_BUSY, pf->state);
> Does using a bit flag instead of a proper lock bypass lockdep and deadlock
> detection?

This likely could and should be improved, but I am not certain if it
makes sense to try and do so in this series, as this affects much larger
portions of the driver and is a long standing issue with the ice driver
(and Intel drivers in general) :\

> This is a pre-existing issue, but does the lockless read of
> tx->tstamps[idx].start in ice_ptp_process_tx_tstamp() race with its
> initialization?
> In drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_request_ts(), the in_use
> bit is set before the start timestamp is initialized:
>     set_bit(idx, tx->in_use);
>     clear_bit(idx, tx->stale);
>     tx->tstamps[idx].start = jiffies;
> If a concurrent reader in ice_ptp_process_tx_tstamp() iterates over the
> in_use bits locklessly:
>     for_each_set_bit(idx, tx->in_use, tx->len) {
>         ...
>         if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ)) {
>             drop_ts = true;
> can it observe the newly set in_use bit and read a stale start value,
> causing it to erroneously drop a valid SKB?

This should be fixed, and we can fix it by correctly ordering so that
the start time is updated before setting the bit, (possibly with a
barrier?). I will add the fix to the series.

Re: [PATCH iwl-net 08/12] ice: wait for in-flight Tx timestamps before flushing the tracker

[Jacob Keller]

On 8/21/2026 5:13 PM, Jacob Keller wrote:
> From: Petr Oros <poros@redhat.com>
> 
> ice_ptp_flush_tx_tracker() frees every tracked request, but a request
> whose timestamp is still being captured by the PHY at that moment is
> freed without touching the PHY entry. The ready bit published shortly
> after has no tracked owner, and the PHY does not raise another Tx
> timestamp interrupt until every outstanding ready bit is read, so
> delivery for the whole quad degrades to the periodic work.
> 
> Wait up to 10 ms for in-flight captures to publish their ready bits
> before flushing, so the flush clears them together with the rest.
> 
> Fixes: ea9b847cda64 ("ice: enable transmit timestamps for E810 devices")
> Signed-off-by: Petr Oros <poros@redhat.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.c | 33 ++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 8aa49dda90a2..a049dc7a2241 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -740,6 +740,37 @@ ice_ptp_alloc_tx_tracker(struct ice_ptp_tx *tx)
>  	return 0;
>  }
>  
> +static void
> +ice_ptp_wait_for_tracker_drain(struct ice_pf *pf, struct ice_ptp_tx *tx)
> +{
> +	unsigned long deadline = jiffies + msecs_to_jiffies(10);
> +	struct ice_hw *hw = &pf->hw;
> +	u64 tstamp_ready;
> +	bool pending;
> +	u8 idx;
> +
> +	if (hw->reset_ongoing)
> +		return;
> +
> +	do {
> +		if (ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready))
> +			return;
> +
> +		pending = false;
> +		for_each_set_bit(idx, tx->in_use, tx->len) {
> +			if (!(tstamp_ready & BIT_ULL(idx + tx->offset)))
> +				pending = true;


Sashiko says:

> 
> This isn't a bug introduced by this patch, but does this loop risk a NULL
> pointer dereference if tx->in_use is NULL?
> If ice_ptp_alloc_tx_tracker() fails due to memory pressure, it leaves
> tx->in_use as NULL while tx->len > 0.
> If a concurrent PF rebuild then calls ice_ptp_flush_all_tx_tracker(), it
> will iterate over the list, find this partially initialized port, and call
> ice_ptp_flush_tx_tracker(). This now calls ice_ptp_wait_for_tracker_drain()
> which would dereference tx->in_use here.


I am not sure about this one. I think if we fail to allocate the in_use
array we check and bail out and fail to initialize PTP, leaving the PTP
state in ICE_PTP_UNINIT.

ice_ptp_rebuild_owner does iterate the port list.. And if we fail to
initialize the PTP port it will get removed from the list. I guess it
may be that there is a small window where a concurrent rebuild could
cause a problem before it gets out of the teardown?

We could possibly delay inserting the port into the list until a later
step or have the loop check of PTP initialized before continuing here. Hmm.

I'll try to address this in v2, likely as part of the first patch with
the kref and RCU.

> +		}
> +		if (!pending)
> +			return;
> +
> +		usleep_range(500, 1000);
> +	} while (time_before(jiffies, deadline));
> +

Sashiko says:

> This is a pre-existing issue, but does adding this sleep here exacerbate an
> RCU use-after-free when iterating PTP ports during teardown?
> In ice_ptp_flush_all_tx_tracker(), the code drops rcu_read_lock(), calls
> ice_ptp_flush_tx_tracker() (which now sleeps here for up to 10ms), and calls
> kref_put() on the current port.
> If the last reference is dropped, a blocked teardown thread could wake up,
> complete synchronize_rcu(), and free the port.
> When ice_ptp_flush_all_tx_tracker() re-acquires the lock, it will evaluate
> port->list_node.next and dereference the freed port.

I think this will be fixed by correcting the locking and kref use of the
first patch, ensuring that we can't release the port in this way.


> +	dev_dbg(ice_pf_to_dev(pf), "Timed out waiting for in-flight Tx timestamps on block %u\n",
> +		tx->block);
> +}
> +
>  /**
>   * ice_ptp_flush_tx_tracker - Flush any remaining timestamps from the tracker
>   * @pf: Board private structure
> @@ -756,6 +787,8 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
>  	int err;
>  	u8 idx;
>  
> +	ice_ptp_wait_for_tracker_drain(pf, tx);
> +
>  	err = ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready);
>  	if (err) {
>  		dev_dbg(ice_pf_to_dev(pf), "Failed to get the Tx tstamp ready bitmap for block %u, err %d\n",
> 

Finally, sashiko says:

> This is a pre-existing issue, but does Tx timestamping get permanently
> disabled after a non-PFR reset?
> When a non-PFR reset (like CORER or GLOBR) is triggered,
> ice_ptp_prepare_for_reset() calls ice_ptp_release_tx_tracker(). This frees
> tx->tstamps, tx->in_use, and tx->stale, setting tx->len = 0.
> However, the rebuild function ice_ptp_rebuild() doesn't seem to call
> ice_ptp_init_tx() to reallocate these resources, meaning subsequent Tx
> timestamp requests would silently fail.

This is a bug in commit 7a25fe5cd5fb ("ice: stop destroying and
reinitalizing Tx tracker during reset") which tried to remove the
destruction of the Tx tracker during reset. Unfortunately it looks like
that commit didn't actually remove the actual destruction of the tracker.

I'll include a fix for this in v2.

Re: [PATCH iwl-net 09/12] ice: keep Tx timestamp slots tracked until completion or timeout

[Jacob Keller]

On 8/21/2026 5:13 PM, Jacob Keller wrote:
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index a049dc7a2241..1a9bf8839404 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -581,14 +583,11 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
>  			return;
>  	}
>  
> -	/* Drop packets if the link went down */
> -	link_up = ptp_port->link_up;
> -
>  	for_each_set_bit(idx, tx->in_use, tx->len) {
>  		struct skb_shared_hwtstamps shhwtstamps = {};
>  		u8 phy_idx = idx + tx->offset;
>  		u64 raw_tstamp = 0, tstamp;
> -		bool drop_ts = !link_up;
> +		bool drop_ts = false;
>  		struct sk_buff *skb;
>  

Sashiko says:

> This is a pre-existing issue, but does the lockless traversal of
> tx->in_use here race with new timestamp allocations?
> In drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_request_ts(), the in_use
> bit is set before the timestamp start and skb fields are initialized:
>     set_bit(idx, tx->in_use);
>     clear_bit(idx, tx->stale);
>     tx->tstamps[idx].start = jiffies;
>     tx->tstamps[idx].skb = skb_get(skb);
> If this processing loop runs concurrently in an IRQ or worker thread, could
> it see the newly set in_use bit but read a stale start value from a prior
> use of the index?
> If that stale time happens to be older than the 2-second timeout, the loop
> will set drop_ts = true, acquire tx->lock, and clear in_use. Depending on
> whether the new skb pointer is visible yet, couldn't this cause the new Tx
> request to be incorrectly dropped, or its skb to be permanently leaked?

I think this was pointed out by a previous change as well. I'll include
a proper fix for this in the series in v2.

Thanks,
Jake

Re: [PATCH iwl-net 10/12] ice: remove unnecessary discarding of timestamps after clock adjust

[Jacob Keller]

On 8/21/2026 5:13 PM, Jacob Keller wrote:
> The ice driver currently discards any outstanding timestamps that are
> happening very near to a .adjtime or .settime callback. This was originally
> add by commit d40fd6009332 ("ice: handle flushing stale Tx timestamps in
> ice_ptp_tx_tstamp").
> 
> The original motivation for discarding timestamps was that extending an old
> timestamp using the new cached value of PHC was a problem, as it could
> produce incorrect results. The change did not describe what such "incorrect
> results" were.
> 
> There are no such incorrect results. Extending the 32 bit timestamp with
> the new time value just means that the timestamp is reported in terms of
> the newly updated and adjusted system clock. This won't produce incorrect
> results or problematic timestamps to applications. Either the timestamp
> will be extended with the value of the PHC just prior to the time
> adjustment (if the timestamp completes prior to the adjust callback), or it
> will be extended using the new PHC value after the adjustment. In either
> case, the resulting extended timestamp value makes sense.
> 

Sashiko says:

> Could removing the stale flush logic actually lead to corrupted timestamps if
> the time jumps by more than 2.14 seconds?
> Looking at ice_ptp_extend_32b_ts():
> ice_ptp_extend_32b_ts() {
>         ...
>         phc_time_lo = (u32)cached_phc_time;
>         delta = (in_tstamp - phc_time_lo);
>         ...
> }
> If a time adjustment exceeds 2.14 seconds while a Tx timestamp is in flight,
> won't the 32-bit modulo math wrap? It appears reconstructing the old hardware
> timestamp using the post-jump cached_phc_time would produce a meaningless
> value rather than a correctly extended one.

I no longer believe that result would be incorrect. I don't see other
users of similar logic around struct timecounter needing any similar
logic. The users of timecounter are very similar with the only
difference being that they maintain the "upper" bits entirely in
software, while the ice driver has hardware maintain them, effectively.
Perhaps I am wrong and missing something? If others agree I can drop
this from the series.

> The timestamp extension logic is very similar to the logic found in
> timecounter_cyc2time, the primary difference being that the ice hardware
> maintains the full 64 bits of nanoseconds in the MAC rather than being
> maintained purely by software as in the timecounter case.
> 
> Indeed, I couldn't find an example of a driver using timecounter_cyc2time
> which does discard timestamps that occur nearby a time adjustment. The ice
> driver behavior of discarding such timestamps just results in failure to
> deliver a Tx timestamp to userspace, resulting in applications such as
> ptp4l to timeout and enter a fault state. Reporting the extended timestamp
> based on the updated PHC value isn't producing "garbage" results, and
> doesn't lead to incorrect behavior.
> 
> This effectively reverts commit d40fd6009332 ("ice: handle flushing stale
> Tx timestamps in ice_ptp_tx_tstamp"). However, the stale logic remains, as
> we now use it to inform the driver to drop timestamps which might fail due
> to link down.
> 
> Fixes: d40fd6009332 ("ice: handle flushing stale Tx timestamps in ice_ptp_tx_tstamp")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 1a9bf8839404..8c2dd83d8d6c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -826,12 +826,10 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
>   * ice_ptp_mark_tx_tracker_stale - Mark unfinished timestamps as stale
>   * @tx: the tracker to mark
>   *
> - * Mark currently outstanding Tx timestamps as stale. This prevents sending
> - * their timestamp value to the stack. This is required to prevent extending
> - * the 40bit hardware timestamp incorrectly.
> - *
> - * This should be called when the PTP clock is modified such as after a set
> - * time request.
> + * Mark currently outstanding Tx timestamps as stale. This prevents the driver
> + * from reporting the timestamp to the stack. This is called to inform the
> + * driver that a timestamp is expected to fail if it was initiated as the link
> + * went down.
>   */
>  static void
>  ice_ptp_mark_tx_tracker_stale(struct ice_ptp_tx *tx)
> @@ -1049,13 +1047,6 @@ static void ice_ptp_reset_cached_phctime(struct ice_pf *pf)
>  		kthread_queue_delayed_work(pf->ptp.kworker, &pf->ptp.work,
>  					   msecs_to_jiffies(10));
>  	}


Sashiko says:


> This is a pre-existing issue, but is it possible for pf->ptp.kworker to be
> NULL here during driver teardown?
> Looking at ice_ptp_release(), it destroys the kworker before unregistering
> the PTP clock:
> ice_ptp_release() {
>         if (pf->ptp.kworker) {
>                 kthread_destroy_worker(pf->ptp.kworker);
>                 pf->ptp.kworker = NULL;
>         }
>         if (!pf->ptp.clock)
>                 return;
>         ...
>         ptp_clock_unregister(pf->ptp.clock);
> }
> If userspace calls adjtime or settime concurrently during this window, it
> would unconditionally call ice_ptp_reset_cached_phctime(). If the update
> fails and falls back to queuing work, could this dereference the NULL
> pf->ptp.kworker pointer?


I will check this. I think the correct answer is to kref on the port
access to ensure that we don't finish teardown until the function exits,
which would be included as part of the first patch of the series.

Re: [PATCH iwl-net 11/12] ice: skip reading Tx ready bitmap on ports with no timestamps

[Jacob Keller]

On 8/21/2026 5:13 PM, Jacob Keller wrote:
> On E82x devices, the interrupt for Tx timestamps are handled by the clock
> owner. When an interrupt with the Tx timestamp cause is fired, the clock
> owner PF iterates the list of ports and checks for timestamps across all
> ports.
> 
> The existing logic reads the PHY timestamp ready bitmap before iterating
> the list of in-use timestamp indexes, even for ports which have no
> timestamps waiting in the software timestamp tracker. This has a
> significant and measurable latency impact on reporting Tx timestamps.
> 
> Check the bitmap and exit early in the event that there are no timestamps
> waiting on a port. Observant reviewers may notice that the check is done
> without acquiring the lock. This is fine, as the only thread that can clear
> in_use bits is the miscellaneous interrupt handler. Whether the thread sees
> or fails to see a new outstanding timestamp does not affect correctness,
> only determining whether or not it should do extra work.
> 
> Using the ice Tx timestamp traces, with a simple ptp4l setup the average
> latency appears to be around 175 to 200 microseconds with a few outliers
> taking hundreds of microseconds to be reported. With the check to skip
> empty bitmaps (and thus skip reading the ready bitmap for inactive ports),
> the average latency drops ~50 microseconds.
> 
> Fixes: d938a8cca88a ("ice: Auxbus devices & driver for E822 TS")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 8c2dd83d8d6c..b337247c94e0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -573,7 +573,7 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
>  	pf = ptp_port_to_pf(ptp_port);
>  	hw = &pf->hw;
>  
> -	if (!tx->init)
> +	if (!tx->init || bitmap_empty(tx->in_use, tx->len))
>  		return;
>  
>  	/* Read the Tx ready status first */
> 

Sashiko says:

> This is a pre-existing issue, but could this lockless check of tx->in_use
> result in a use-after-free or NULL pointer dereference during a cross-PF
> teardown?
> In ice_ptp_release_tx_tracker(), a secondary PF undergoing reset sets
> tx->init = 0 and frees tx->in_use:
> drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_release_tx_tracker() {
>     tx->init = 0;
>     spin_unlock_irqrestore(&tx->lock, flags);
>     /* wait for potentially outstanding interrupt to complete */
>     synchronize_irq(pf->oicr_irq.virq);
>     ...
>     bitmap_free(tx->in_use);
>     tx->in_use = NULL;
> }
> Since synchronize_irq() only waits on the resetting PF's IRQ, it doesn't
> block the clock owner PF which runs on a different IRQ.
> Can the clock owner PF running ice_ptp_process_tx_tstamp() evaluate
> tx->init as true, get preempted, and then dereference tx->in_use after
> it has been freed by the resetting PF?


I believe this will be fixed by the patch which removes the call to stop
clearing the tracker except on load. I will investigate if we need any
further change as there may be a similar issue with teardown. We might
need to synchronize against the clock owner IRQ for ports using the
INTERRUPT_ALL mode.

> This is also a pre-existing issue, but does adding this early return prevent
> the driver from recovering if a hardware timestamp takes too long to arrive?
> When a timestamp request takes longer than 2 seconds, the software drops it
> and clears its index from tx->in_use:
> drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_process_tx_tstamp() {
>     ...
>     if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ)) {
>         drop_ts = true;
>     ...
> skip_ts_read:
>     ...
>     clear_bit(idx, tx->in_use);
> }
> If the hardware subsequently completes this dropped timestamp, the ready bit
> will assert. However, with tx->in_use now being empty, the early return
> prevents the driver from calling ice_read_phy_tstamp().
> As noted in the driver comments in this same function, failing to read valid
> PHY timestamps can cause the hardware interrupt generation logic to become
> permanently stuck on some devices. Should this path ensure orphaned timestamps
> are still read and cleared from PHY memory?

If hardware somehow holds onto a timestamp for longer than 2 seconds the
logic we have already fails, but we need some cut off. It *is* possible
that a timestamp never happens if it occurs near a link event. We have
no way to be informed by hardware that it won't complete a timestamp. If
we do nothing the more common case of a missed timestamp would lock the
index indefinitely.

The assumption being made here is that 2 seconds is sufficient time to
be certain the hardware will no longer complete the timestamp. I don't
think we can make the software robust in both ways, and have to make
some trade off here.

Re: [PATCH iwl-net 12/12] ice: don't clear in_use until HW clears ready bitmap

[Jacob Keller]

On 8/21/2026 5:13 PM, Jacob Keller wrote:
> During a link down transition, the E825 PHY has a small window where it
> does not properly respond to reading the PHY timestamp registers. When this
> occurs, the PHY does not automatically clear the ready bitmap or the valid
> bit for the timestamp. This begins happening slightly before a link
> transition even before the firmware has notified the driver of the state
> change.
> 
> The driver happily completes the timestamp, releasing the in_use bit. This
> allows another request to reuse the bit potentially reporting an invalid
> stale timestamp. Additionally, with the ready bit still set high the driver
> continues to re-trigger the IRQ and check for timestamps in a tight loop,
> wasting CPU cycles.
> 
> To fix this, re-read the PHY timestamp memory status after each read of a
> PHY index. Double check if the hardware cleared the index properly. If it
> hasn't, mark the timestamp index as stale and skip processing it.
> 
> Stale timestamps are already ignored by the ice_any_port_has_timestamps()
> function. However, the ice_ptp_tx_tstamps_pending() function also checks
> the ready bitmap. Instead, modify it to only check the software tracker.
> Additionally, stop re-triggering the interrupt from the IRQ if the
> timestamp tracker is calibrating or has the link marked as down. Continue
> to check the hardware ready bitmap from the watchdog to catch cases of
> unexpected timestamps.
> 
> With these changes, the timestamp processing no longer triggers a repeated
> spamming of the IRQ during link down events where timestamps get stuck as
> the PHY transitions to link down. Once link is restored, the PHY will be
> reset and the stuck timestamps are cleared.
> 
> Measuring CPU utilization of the miscellaneous IRQ thread function during
> timestamp storms near a link reset shows that this prevents the spikes
> caused by the "stuck" ready bit. Without this fix, the CPU handling the IRQ
> becomes slammed due to the IRQ re-triggering logic.
> 
> Measuring latency using the ice Tx timestamp traces does show that this fix
> comes at a latency cost. Latency is measured using the ice Tx timestamp
> traces for the request to completion time. I measured a couple of different
> workloads both before and after this fix:
> 
>  * ptp4l using a profile with ~16 SYNC messages per second
> 
>     before: 159.40 microseconds mean, stdev 45.28
>      after: 182.07 microseconds mean,  stdev 43.43
> 
>  * a C program generating 16 timestamp requests every 10 milliseconds on
>    two different ports:
> 
>     before: 604.35 microseconds mean, stdev 345.32
>      after: 990.13 microseconds mean, stdev 625.64
> 
> In the normal work flows this comes with about a 20 microsecond penalty on
> the average, and the standard deviation remains approximately the same. For
> heavy workloads with many more timestamps than expected for typical
> applications this comes at a significant cost. This is because we handle
> all timestamps in a single thread. If there are many concurrent timestamps
> being requested at once, any which use the later slots on ports later in
> the port list will take much longer to be processed once the interrupt is
> fired. Since each timestamp now requires an additional PHY register access,
> this cost is much higher in the case where the device is under unusually
> heavy load.
> 
> However, *correctness* is more important than speed here. Additionally, we
> still remain well below the default limit of 10 milliseconds that ptp4l
> will wait before complaining about missing timestamps.
> 
> Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C products")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.c | 54 +++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index b337247c94e0..0e44ed6b0ae8 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -620,6 +620,22 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)

Sashiko says:

> This is a pre-existing issue, but I noticed something about the tx->in_use
> logic in ice_ptp_process_tx_tstamp().
> At the beginning of the function, there is an early return:
>     if (!tx->init || bitmap_empty(tx->in_use, tx->len))
>         return;
> If a timestamp times out after 2 seconds, the driver frees the SKB and clears
> the slot from tx->in_use. If the hardware subsequently completes the capture
> and sets the ready bit, does this early return prevent the driver from
> clearing that unexpected hardware timestamp ready bit?
> Could this lead to data corruption if a new packet reuses the slot, reads the
> stale timestamp, and clears the ready bit, thereby passing the verification
> checks?

I don't believe we can fix this. If hardware fails to complete a
timestamp within 2 seconds *and then* completes it after two seconds we
end up in a bad state. However, if hardware never completes the
timestamp and we remove our timeout logic then we also end up in a bad
state.

In principle there is no possible "cutoff" we can make that correctly
handles both cases of either a super delayed timestamp or one that
simply never completes.

I believe 2 seconds is a sufficient cutoff to practically ensure we
handle all cases correctly as it should not be reasonable for the PHY to
hold a packet for 2 seconds before transmitting it once enqueued. We
could increase this to a larger time and be more and more certain.
However, if we increase to infinity then indexes can and will become
locked should a packet be enqueued which gets dropped before transmission.


>  		if (err && !drop_ts)
>  			continue;
>  
> +		/* verify ready bit cleared */
> +		if (tx->has_ready_bitmap) {
> +			err = ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready);
> +			if (err)
> +				continue;
> +
> +			if (tstamp_ready & BIT_ULL(phy_idx)) {
> +				spin_lock_irqsave(&tx->lock, flags);
> +				if (!test_and_set_bit(idx, tx->stale))
> +					dev_dbg(ice_pf_to_dev(pf), "PHY port %u failed to clear ready bit for idx %u\n",
> +						ptp_port->port_num, phy_idx);
> +				spin_unlock_irqrestore(&tx->lock, flags);
> +				continue;
> +			}

Sashiko says:

> If a timestamp request times out (i.e. older than 2 seconds), the driver sets
> drop_ts = true earlier in the loop.
> If ice_get_phy_tx_tstamp_ready() fails and returns an error, this
> unconditionally executes continue. Does this bypass the skip_ts_read
> cleanup block that would normally clear in_use and free the socket buffer?
> Could this result in permanently leaking the SKB, exhausting the Tx tracker
> slots, and unbounded timeout statistic increments?

I think we probably should continue but mark the timestamp as stale just
like we do with checking the ready bitmap. That way we safely handle
that case and avoid storming the IRQ.

Sashiko also says:

> Removing the hardware read error fallback here, combined with the early
> continue on error in ice_ptp_process_tx_tstamp() above, appears to leave
> in_use set without marking the timestamp as stale.
> If the PHY becomes temporarily unreadable during a link down transition, does
> ice_ptp_tx_tstamps_pending() in IRQ context evaluate to true, continuously
> re-arming and triggering the interrupt in an infinite loop?
The IRQ will storm regardless in that case because the in_use is set. If
we mark it as stale that would stop the re-arming of the interrupt.


> +		}
> +
>  		ice_trace(tx_tstamp_fw_done, tx->tstamps[idx].skb, idx);
>  
>  		/* For PHYs which don't implement a proper timestamp ready
> @@ -2764,10 +2780,14 @@ static bool ice_port_has_timestamps(struct ice_ptp_tx *tx, bool in_irq)
>  		if (!tx->init)
>  			return false;
>  
> -		if (in_irq)
> +		if (in_irq) {
> +			if (!ice_ptp_is_tx_tracker_up(tx))
> +				return false;
> +
>  			return bitmap_andnot(tstamps, tx->in_use, tx->stale, tx->len);
> -		else
> +		} else {
>  			return !bitmap_empty(tx->in_use, tx->len);
> +		}
>  	}
>  }
>  
> @@ -2790,41 +2810,18 @@ static bool ice_any_port_has_timestamps(struct ice_pf *pf, bool in_irq)
>  
>  bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf, bool in_irq)
>  {
> -	struct ice_hw *hw = &pf->hw;
> -	int ret;
> -
> -	/* Check software indicator */
>  	switch (pf->ptp.tx_interrupt_mode) {
>  	case ICE_PTP_TX_INTERRUPT_NONE:
>  		return false;
>  	case ICE_PTP_TX_INTERRUPT_SELF:
> -		if (ice_port_has_timestamps(&pf->ptp.port.tx, in_irq))
> -			return true;
> -		break;
> +		return ice_port_has_timestamps(&pf->ptp.port.tx, in_irq);
>  	case ICE_PTP_TX_INTERRUPT_ALL:
> -		if (ice_any_port_has_timestamps(pf, in_irq))
> -			return true;
> -		break;
> +		return ice_any_port_has_timestamps(pf, in_irq);
>  	default:
>  		WARN_ONCE(1, "Unexpected Tx timestamp interrupt mode %u\n",
>  			  pf->ptp.tx_interrupt_mode);
> -		break;
> -	}
> -
> -	/* Check hardware indicator */
> -	ret = ice_check_phy_tx_tstamp_ready(hw);
> -	if (ret < 0) {
> -		dev_dbg(ice_pf_to_dev(pf), "Unable to read PHY Tx timestamp ready bitmap, err %d\n",
> -			ret);
> -		/* Stop triggering IRQs if we're unable to read PHY */
>  		return false;
>  	}
> -
> -	/* ice_check_phy_tx_tstamp_ready() returns 1 if there are timestamps
> -	 * available, 0 if there are no waiting timestamps, and a negative
> -	 * value if there was an error (which we checked for above).
> -	 */
> -	return ret > 0;
>  }
>  
>  /**
> @@ -2915,7 +2912,8 @@ static void ice_ptp_maybe_trigger_tx_interrupt(struct ice_pf *pf)
>  	if (!ice_pf_src_tmr_owned(pf))
>  		return;
>  
> -	if (ice_ptp_tx_tstamps_pending(pf, false)) {
> +	if (ice_ptp_tx_tstamps_pending(pf, false) ||
> +	    ice_check_phy_tx_tstamp_ready(hw)) {
>  		dev_dbg(dev, "PTP periodic task detected waiting timestamps. Triggering Tx timestamp interrupt now.\n");
>  

Sashiko (and Aleks Loktionov!) says:

> The ice_check_phy_tx_tstamp_ready() function can return a negative error
> code on failure.
> By evaluating it directly in this boolean condition without checking if the
> return value is > 0, will a negative error code evaluate to true?
> Could this cause the watchdog to endlessly trigger spurious Tx timestamp
> hardware interrupts whenever the PHY read fails?
I'll fix this in v2.

>  		wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);
>