[PATCH net 0/3] ice: E825: PHY restart fixes

[PATCH net 0/3] ice: E825: PHY restart fixes

[Jacob Keller]

This is a subset of the timestamp processing logic fixes that were
previously posted at [1] and [2]. It focuses specifically on the E825 timer
restart procedure, including three fixes which we have deemed the most
critical for reaching customer deployments due to recently reported issues.

First, the driver is modified to stop clearing the PHY_REG_TX_OFFSET_READY
register. This register controls whether a captured timestamp will have its
"valid" bit set. A timestamp captured while this register still increments
the internal counter. However, since the valid bit is clear, reads of this
timestamp will not trigger the internal counter decrement, leading to an
off-by-one error in the hardware logic. Instead of clearing the offset
ready register, disable timestamp requests purely in software and mark
outstanding timestamps as stale to prevent reporting timestamps that should
be considered valid.

Next, the E825 soft reset procedure is extended to sweep the timestamp
memory region with reads before the soft reset is performed. Reading a
timestamp triggers hardware side effects including automatically clearing
the "valid" bit of the timestamp in the memory, and clearing the "ready"
bit from the timestamp bitmap. The hardware soft reset register does reset
the ready bitmap, but does not automatically clear the "valid" bits from
the timestamps in the memory bank. Additionally, the hardware does not
initialize the memory bank to zero at power on. The sweep ensures that
the memory region will have all the valid bits cleared and the device will
be in a consistent state after the soft reset. The sweep is performed as
part of the soft reset because the reads could otherwise leave the internal
counter in an off-by-one state. By always pairing the sweep with a soft
reset, the memory region is cleared and the soft reset resets the ready
bitmap and internal counter to a known good state.

Finally, the E825 timer restart procedure is extended to include performing
a PHY soft reset. By performing a soft reset, the driver can be sure that
the device is in a known good state at every link up. This also provides a
mechanism to recover if any faulty logic still causes the internal
timestamp counter to become out of sync. The system administrator can reset
the link, triggering a PHY timer restart.

These fixes originally came along with other changes to the driver
timestamping logic flow, but have been separated as they are deemed the
most critical for upcoming E825 deployments. The remaining changes from the
original series continue to suffer from AI feedback pointing out potential
race conditions. The link changes especially may need some deeper
investigation to resolve. I appreciate the maintainers patience with this
work including the multiple rounds on the original series and helping find
a path forward.

Link: [1] https://lore.kernel.org/netdev/710677e5-3668-4588-bd36-2d8f7fb692a9@intel.com/
Link: [2] https://lore.kernel.org/netdev/20261027-reduction-v1-0-81237c67a361@intel.com/

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes:
- The missing mutex_unlock() in the -ENODEV case has been fixed.
- A small typo in the kdoc has been fixed.

I've double checked this round through my own local sashiko instance 3
times and didn't get any remaining reports after a few runs. I only have
claude models, but hopefully there won't be any remaining complaints... :D

---
Jacob Keller (3):
      ice: E825: stop clearing PHY_REG_TX_OFFSET_READY
      ice: E825: clear PHY_TSTAMP memory prior to soft reset
      ice: E825: perform a soft reset when starting the PHY timer

 drivers/net/ethernet/intel/ice/ice_ptp.h    |   5 +-
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h |   2 +-
 drivers/net/ethernet/intel/ice/ice_ptp.c    |  43 ++++++--
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 155 ++++++++++++++++------------
 4 files changed, 126 insertions(+), 79 deletions(-)
---
base-commit: 8f44631ba02afe9fff2f0a0fb66560ab19df87fd
base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git main
change-id: 20261029-jk-e825c-critical-timestamping-fixes-8e1f86335de1

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

[PATCH net 1/3] 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.

Notice that the soft_reset parameter of ice_stop_phy_timer_eth56g() is
totally unused. It is a relic from a copy-paste of
ice_stop_phy_timer_e82x() that is unnecessary, so remove it.

Now that we do not clear the PHY_REG_TX_OFFSET_READY, new timestamp
requests could happen while the PHY is calibrating. To avoid this, set the
tx.disabled field of the Tx timestamp tracker when stopping the timer
and clear it when finishing the restart. This ensures that any new requests
will be rejected until the PHY timer calibration has completed. Also call
ice_ptp_mark_tx_tracker_stale() to prevent reporting any previous
outstanding timestamps to the stack. Fix ice_ptp_mark_tx_tracker_stale() to
properly check if the tracker is initialized, to prevent a possible
use-after-free on the bitmap.

Unlike E822 devices, set the disabled flag when "stopping" the PHY and
clear it immediately after the start procedure. The E825 device does not
perform vernier calibration and thus does not need to wait for hardware to
mark the offsets as valid.

In the event that the PHY timer start procedure fails, the device is in an
unknown state and timestamps will not behave properly. As such, and similar
to E822 devices, the disabled field is not cleared on failure. This
leaves timestamp requests disabled until the next link restart.

Failure in the start flow is unexpected and it is unclear precisely what
state the hardware is left in. Attempting to add a complex retry mechanism
for a rare event is not worthwhile. The port restart procedure can already
be re-initiated by triggering a link reset (i.e. via ethtool).

Instead update the dev_err message at the end of ice_ptp_port_phy_restart.
Clearly indicate that timestamping is not functional, and add a note that a
link toggle might recover the device. For the invalid MAC type path
returning -ENODEV, skip this message and log a dev_dbg that we failed with
an unknown MAC type instead.

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.h    |  5 ++--
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h |  2 +-
 drivers/net/ethernet/intel/ice/ice_ptp.c    | 43 +++++++++++++++++++++++------
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 25 +++++++++--------
 4 files changed, 52 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index f0839d90e9d2..c5acfb86efb0 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -106,8 +106,9 @@ enum ice_tx_tstamp_work {
  * @offset: offset into timestamp block to get the real index
  * @len: length of the tstamps and in_use fields.
  * @init: if true, the tracker is initialized;
- * @disabled: set to true when new Tx timestamp requests are disabled, such as
- *            while the PHY Tx offset configuration has not yet completed.
+ * @disabled: set to true when new Tx timestamp requests are disabled. This
+ *            includes during E82X Vernier calibration, and for as long as the
+ *            PHY is considered stopped on E825.
  * @has_ready_bitmap: if true, the hardware has a valid Tx timestamp ready
  *                    bitmap register. If false, fall back to verifying new
  *                    timestamp values against previously cached copy.
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index 16b1988e993d..17000df77ce9 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -378,7 +378,7 @@ int ice_cgu_get_output_pin_state_caps(struct ice_hw *hw, u8 pin_id,
 
 /* ETH56G family functions */
 int ice_ptp_read_tx_hwtstamp_status_eth56g(struct ice_hw *hw, u32 *ts_status);
-int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port, bool soft_reset);
+int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port);
 int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port);
 int ice_phy_cfg_intr_eth56g(struct ice_hw *hw, u8 port, bool ena, u8 threshold);
 int ice_phy_cfg_ptp_1step_eth56g(struct ice_hw *hw, u8 port);
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 7980eaf9ce25..3df08714510d 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -811,9 +811,6 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
  * 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.
  */
 static void
 ice_ptp_mark_tx_tracker_stale(struct ice_ptp_tx *tx)
@@ -821,7 +818,8 @@ ice_ptp_mark_tx_tracker_stale(struct ice_ptp_tx *tx)
 	unsigned long flags;
 
 	spin_lock_irqsave(&tx->lock, flags);
-	bitmap_or(tx->stale, tx->stale, tx->in_use, tx->len);
+	if (tx->init)
+		bitmap_or(tx->stale, tx->stale, tx->in_use, tx->len);
 	spin_unlock_irqrestore(&tx->lock, flags);
 }
 
@@ -1229,6 +1227,7 @@ ice_ptp_port_phy_stop(struct ice_ptp_port *ptp_port)
 	struct ice_pf *pf = ptp_port_to_pf(ptp_port);
 	u8 port = ptp_port->port_num;
 	struct ice_hw *hw = &pf->hw;
+	unsigned long flags;
 	int err;
 
 	mutex_lock(&ptp_port->ps_lock);
@@ -1244,7 +1243,14 @@ ice_ptp_port_phy_stop(struct ice_ptp_port *ptp_port)
 		err = ice_stop_phy_timer_e82x(hw, port, true);
 		break;
 	case ICE_MAC_GENERIC_3K_E825:
-		err = ice_stop_phy_timer_eth56g(hw, port, true);
+		/* Disable new Tx timestamp requests */
+		spin_lock_irqsave(&ptp_port->tx.lock, flags);
+		ptp_port->tx.disabled = true;
+		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
+
+		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
+
+		err = ice_stop_phy_timer_eth56g(hw, port);
 		break;
 	default:
 		err = -ENODEV;
@@ -1308,15 +1314,36 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
 					   0);
 		break;
 	case ICE_MAC_GENERIC_3K_E825:
+		/* ice_ptp_port_phy_stop() may have already disabled
+		 * timestamps, but some restarts occur without first stopping
+		 * the timer, so we ensure that new requests are disabled
+		 * here.
+		 */
+		spin_lock_irqsave(&ptp_port->tx.lock, flags);
+		ptp_port->tx.disabled = true;
+		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
+
+		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
+
 		err = ice_start_phy_timer_eth56g(hw, port);
+		if (err)
+			break;
+
+		spin_lock_irqsave(&ptp_port->tx.lock, flags);
+		ptp_port->tx.disabled = false;
+		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
+
 		break;
 	default:
-		err = -ENODEV;
+		dev_dbg(ice_pf_to_dev(pf), "PTP failed to restart PHY port %u with unknown MAC type %d\n",
+			port, hw->mac_type);
+		mutex_unlock(&ptp_port->ps_lock);
+		return -ENODEV;
 	}
 
 	if (err)
-		dev_err(ice_pf_to_dev(pf), "PTP failed to set PHY port %d up, err %d\n",
-			port, err);
+		dev_err(ice_pf_to_dev(pf), "PTP failed to restart PHY port %u with err %pe; Timestamping not enabled; A link-toggle may recover.\n",
+			port, ERR_PTR(err));
 
 	mutex_unlock(&ptp_port->ps_lock);
 
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 3a41c711e751..07b55fbb88dd 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -2098,32 +2098,33 @@ static int ice_sync_phy_timer_eth56g(struct ice_hw *hw, u8 port)
 }
 
 /**
- * ice_stop_phy_timer_eth56g - Stop the PHY clock timer
+ * ice_stop_phy_timer_eth56g - Clear PHY Rx offset ready flag
  * @hw: pointer to the HW struct
  * @port: the PHY port to stop
- * @soft_reset: if true, hold the SOFT_RESET bit of PHY_REG_PS
  *
- * Stop 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.
+ * Disable Rx timestamping by clearing the PHY_REG_RX_OFFSET_READY. This
+ * causes Rx timestamps to be captured with their valid bit clear, ensuring we
+ * discard any timestamp captured while the PHY is being recalibrated.
+ *
+ * Note this does *not* clear PHY_REG_TX_OFFSET_READY. Clearing it would
+ * cause the Tx timestamps to be captured with their valid bit clear.
+ * Unfortunately the captured timestamps still increment the internal counter
+ * and result in off-by-one accounting. Instead, Tx timestamp requests should
+ * be disabled by other means.
  *
  * Return:
  * * %0     - success
  * * %other - failed to write to PHY
  */
-int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port, bool soft_reset)
+int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port)
 {
 	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;
 
-	ice_debug(hw, ICE_DBG_PTP, "Disabled clock on PHY port %u\n", port);
+	ice_debug(hw, ICE_DBG_PTP, "Disabled Rx timestamps on PHY port %u\n", port);
 
 	return 0;
 }
@@ -2151,7 +2152,7 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
 
 	tmr_idx = ice_get_ptp_src_clock_index(hw);
 
-	err = ice_stop_phy_timer_eth56g(hw, port, false);
+	err = ice_stop_phy_timer_eth56g(hw, port);
 	if (err)
 		return err;
 

-- 
2.56.0.215.g47fdf2208c5b

[PATCH net 2/3] ice: E825: clear PHY_TSTAMP memory prior to soft reset

[Jacob Keller]

The 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_TSTAMP memory region containing the timestamp data is not
zero-initialized when the devices 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.

In normal operation this is "fine" because the driver should never read
from the PHY_TSTAMP region without first checking if the
PHY_REG_TX_MEMORY_STATUS bit for a given index is set. However, driver bugs
or programmer error could result in accidentally reading this data.

Attempting to write to the PHY_TSTAMP region is also ineffective as the
registers are read only. Instead, reading a register triggers PHY action to
clear its valid bit. However, the PHY also performs other actions such as
clearing the bit in the PHY_REG_TX_MEMORY_STATUS and decrementing the
internal counter.

We must read the PHY_TSTAMP memory 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
PHY_REG_TX_MEMORY_STATUS ready bitmap. The reads to PHY_TSTAMP reagion 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.

If a read fails during the sweep (i.e. due to a transient sideband queue
failure) it is not treated as a fatal error. There is already a dynamic
debug message for each read failure. Keep track of the total number of
timestamp registers that fail for a given port and log a warning message
indicating the total number of failures. Continuing is acceptable as
clearing the memory is a precaution against faulty software. Correct
software access won't read the index twice and won't read it at all except
when it has already confirmed that the index is valid by reading
ice_get_phy_tx_tstamp_ready(), at which point the hardware will have
overwritten any stale data.

Expand the kernel documentation for ice_ptp_phy_soft_reset_eth56g(), to
clarify that it performs the timestamp memory sweep. Also clarify the
expected caller behavior should the soft reset procedure fail. For example,
if the driver loses PHY access part-way through a reset it might have swept
the timestamp memory but not perform a PHY reset. The caller must treat
failure returns from ice_ptp_phy_soft_reset_eth56g() as if the PHY is in an
inconsistent and non-functional state.

Since ice_ptp_reset_ts_memory now only works for E82X hardware, update its
kdoc comment to clarify what hardware is skipped and note specifically why
E825 is skipped.

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

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 07b55fbb88dd..ef59b34ba52d 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
@@ -1129,47 +1111,35 @@ static int ice_read_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx,
 }
 
 /**
- * ice_clear_ptp_tstamp_eth56g - Clear a timestamp from the quad block
+ * ice_clear_ptp_tstamp_eth56g - Clear a timestamp from PHY memory
  * @hw: pointer to the HW struct
- * @port: the quad to read from
+ * @port: the port to read from
  * @idx: the timestamp index to reset
  *
- * Read and then forcibly clear the timestamp index to ensure the valid bit is
- * cleared and the timestamp status bit is reset in the PHY port memory of
- * internal PHYs of the 56G devices.
+ * Read the timestamp index to ensure that the valid bit is cleared and the
+ * timestamp status bit is reset in the PHY port memory.
  *
- * 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().
- *
- * This function should only be called on an idx whose bit is set according to
- * ice_get_phy_tx_tstamp_ready().
+ * This function should only be called on an index whose bit is set according
+ * to ice_get_phy_tx_tstamp_ready(), or as part of a full sweep when paired
+ * with a PHY soft reset via ice_ptp_phy_soft_reset_eth56g().
  *
  * Return:
  * * %0     - success
- * * %other - failed to write to PHY
+ * * %other - failed to read from PHY
  */
 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) {
 		ice_debug(hw, ICE_DBG_PTP, "Failed to read the PHY timestamp register for port %u, idx %u, err %d\n",
 			  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;
 	}
 
@@ -1177,19 +1147,43 @@ static int ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx)
 }
 
 /**
- * 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* not be called outside of the
+ * ice_ptp_phy_soft_reset_eth56g() procedure. Otherwise, the internal counter
+ * may become out of sync and prevent new timestamp interrupts.
+ *
+ * Failure to read a given index (i.e. due to a transient sideband queue
+ * failure) is not considered fatal as the PHY port is about to be soft reset.
+ * While the soft reset does not clear the timestamp memory, software
+ * shouldn't be reading the timestamp memory without already knowing it is
+ * valid via ice_get_phy_tx_tstamp_ready(), and this sweep is just
+ * a precaution to ensure the memory is in a known state.
  */
-static void ice_ptp_reset_ts_memory_eth56g(struct ice_hw *hw)
+static void ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port)
 {
-	unsigned int port;
+	int err, failed = 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)
+			failed++;
 	}
+
+	if (failed)
+		dev_warn(ice_hw_to_dev(hw), "Failed to clear %d PHY timestamp registers for port %u\n",
+			 failed, port);
 }
 
 /**
@@ -2295,6 +2289,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
@@ -2303,6 +2298,20 @@ 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_TSTAMP memory region, read every timestamp index prior to performing
+ * the soft reset. The read side effects ensure that the timestamp memory is
+ * cleared while the soft reset ensures that the PHY_REG_TX_MEMORY_STATUS and
+ * internal counter are reset to zero.
+ *
+ * In the event that the driver fails to access the PHY during the completion
+ * of the soft reset, (e.g. due to sideband queue message failures), the
+ * device may be left in an inconsistent state. Such failures are expected to
+ * be rare. Callers are responsible for checking the return of this function
+ * and treating errors as indicating that the timestamping block of the PHY is
+ * non-functional. Recovery may be possible by issuing another soft reset
+ * after any transient access errors have abated.
+ *
  * Return: 0 on success, or a negative error code on failure when
  *         reading or writing the PHY register.
  */
@@ -2311,6 +2320,8 @@ int ice_ptp_phy_soft_reset_eth56g(struct ice_hw *hw, u8 port)
 	u32 global_val;
 	int err;
 
+	ice_ptp_clear_tx_memory_status_eth56g(hw, port);
+
 	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",
@@ -5794,6 +5805,11 @@ static int ice_get_pf_c827_idx(struct ice_hw *hw, u8 *idx)
 /**
  * ice_ptp_reset_ts_memory - Reset timestamp memory for all blocks
  * @hw: pointer to the HW struct
+ *
+ * Only E82X hardware can directly reset memory for all blocks. E825 hardware
+ * can only safely reset timestamp memory as part of the soft reset, so is not
+ * reset here. E810 and E830 also do not implement direct reset of the
+ * timestamp memory region.
  */
 void ice_ptp_reset_ts_memory(struct ice_hw *hw)
 {
@@ -5801,10 +5817,6 @@ void ice_ptp_reset_ts_memory(struct ice_hw *hw)
 	case ICE_MAC_GENERIC:
 		ice_ptp_reset_ts_memory_e82x(hw);
 		break;
-	case ICE_MAC_GENERIC_3K_E825:
-		ice_ptp_reset_ts_memory_eth56g(hw);
-		break;
-	case ICE_MAC_E810:
 	default:
 		return;
 	}

-- 
2.56.0.215.g47fdf2208c5b

Re: [PATCH net 2/3] ice: E825: clear PHY_TSTAMP memory prior to soft reset

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The loop in ice_ptp_clear_tx_memory_status_eth56g() (`for (idx =
  0; idx < INDEX_PER_PORT; idx++)`, INDEX_PER_PORT = 64) only counts…

--

ice: E825: clear PHY_TSTAMP memory prior to soft reset

This commit replaces the E825 timestamp memory reset with a sweep. The old
code wrote zeros to PHY_REG_TX_MEMORY_STATUS. The new code reads every
PHY_TSTAMP index on the port, and the sweep only runs from within
ice_ptp_phy_soft_reset_eth56g(), so the soft reset puts the internal counter
and ready bitmap back in a consistent state. Read failures during the sweep
are counted and reported with a warning but are not treated as fatal.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index 07b55fbb88dde..ef59b34ba52dc 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c

[ ... ]

> +static void ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port)
> +{
> +	int err, failed = 0;
> +	u8 idx;
> +
> +	for (idx = 0; idx < INDEX_PER_PORT; idx++) {
> +		err = ice_clear_ptp_tstamp_eth56g(hw, port, idx);
> +		if (err)
> +			failed++;
>  	}
> +
> +	if (failed)
> +		dev_warn(ice_hw_to_dev(hw), "Failed to clear %d PHY timestamp registers for port %u\n",
> +			 failed, port);
>  }

[ ... ]

> @@ -2311,6 +2320,8 @@ int ice_ptp_phy_soft_reset_eth56g(struct ice_hw *hw, u8 port)
>  	u32 global_val;
>  	int err;
>  
> +	ice_ptp_clear_tx_memory_status_eth56g(hw, port);
> +
>  	err = ice_read_ptp_reg_eth56g(hw, port, PHY_REG_GLOBAL, &global_val);
>  	if (err) {

[Severity: Low]
What happens here if the sideband queue fails persistently instead of
transiently?

The loop in ice_ptp_clear_tx_memory_status_eth56g() runs for all 64
INDEX_PER_PORT entries. It counts failures and never stops early. It also
runs before the PHY_REG_GLOBAL read, and that read is the first access in
ice_ptp_phy_soft_reset_eth56g() that would catch an unreachable PHY.

Each index takes this path:

ice_clear_ptp_tstamp_eth56g()
  ice_read_ptp_tstamp_eth56g()
    ice_read_port_mem_eth56g()
      ice_read_phy_eth56g()
        ice_sbq_rw_reg()
          ice_sq_send_cmd()
            ice_sq_done()

ice_sq_done() polls with ICE_CTL_Q_SQ_CMD_TIMEOUT, which is USEC_PER_SEC.

If the sideband queue head stops advancing, each low read waits the full
1 second and then returns an error. A failed low read skips the high read,
so this costs one timeout per index. Fast -ENOSPC failures only start once
the 64 entry ring (ICE_SBQ_LEN) fills up.

That could mean about a minute of stall in the sweep before the
PHY_REG_GLOBAL read fails and the error is returned. Before this patch, the
same condition was caught after a single timeout on PHY_REG_GLOBAL.

ice_ptp_init_phc_e825c() calls ice_ptp_phy_soft_reset_eth56g(). That makes
this reachable from both ice_ptp_init_owner() during probe and
ice_ptp_rebuild_owner() during reset rebuild.

After the later commit in this series, "ice: E825: perform a soft reset
when starting the PHY timer", it is also reachable from
ice_start_phy_timer_eth56g() via ice_ptp_port_phy_restart(), which holds
ptp_port->ps_lock.

Because the sweep runs before PHY_REG_GLOBAL is checked, its read side
effects can also land on a port whose soft reset then fails. The new kdoc
does tell callers to treat a soft reset failure as a non-functional
timestamp block.

Would it make sense to stop the sweep after consecutive failures, or on a
timeout class error? Another option is to read PHY_REG_GLOBAL before
starting the sweep.

>  		ice_debug(hw, ICE_DBG_PTP, "Failed to read PHY_REG_GLOBAL for port %d, err %d\n",

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-jk-e825c-critical-timestamping-fixes-v1-0-72128d1dd928%40intel.com

Re: [PATCH net 2/3] ice: E825: clear PHY_TSTAMP memory prior to soft reset

[Jacob Keller]

On 10/30/2026 2:40 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The loop in ice_ptp_clear_tx_memory_status_eth56g() (`for (idx =
>   0; idx < INDEX_PER_PORT; idx++)`, INDEX_PER_PORT = 64) only counts…
> 
> --
> 
> ice: E825: clear PHY_TSTAMP memory prior to soft reset
> 
> This commit replaces the E825 timestamp memory reset with a sweep. The old
> code wrote zeros to PHY_REG_TX_MEMORY_STATUS. The new code reads every
> PHY_TSTAMP index on the port, and the sweep only runs from within
> ice_ptp_phy_soft_reset_eth56g(), so the soft reset puts the internal counter
> and ready bitmap back in a consistent state. Read failures during the sweep
> are counted and reported with a warning but are not treated as fatal.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> index 07b55fbb88dde..ef59b34ba52dc 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> 
> [ ... ]
> 
>> +static void ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port)
>> +{
>> +	int err, failed = 0;
>> +	u8 idx;
>> +
>> +	for (idx = 0; idx < INDEX_PER_PORT; idx++) {
>> +		err = ice_clear_ptp_tstamp_eth56g(hw, port, idx);
>> +		if (err)
>> +			failed++;
>>  	}
>> +
>> +	if (failed)
>> +		dev_warn(ice_hw_to_dev(hw), "Failed to clear %d PHY timestamp registers for port %u\n",
>> +			 failed, port);
>>  }
> 
> [ ... ]
> 
>> @@ -2311,6 +2320,8 @@ int ice_ptp_phy_soft_reset_eth56g(struct ice_hw *hw, u8 port)
>>  	u32 global_val;
>>  	int err;
>>  
>> +	ice_ptp_clear_tx_memory_status_eth56g(hw, port);
>> +
>>  	err = ice_read_ptp_reg_eth56g(hw, port, PHY_REG_GLOBAL, &global_val);
>>  	if (err) {
> 
> [Severity: Low]
> What happens here if the sideband queue fails persistently instead of
> transiently?
> 

On a previous version I got asked to change to this method because of
possible transient failures. I think this is best since these reads and
side effects aren't entirely critical to functional device.

I would rather leave this as-is and not waffle back and forth about such
read/write failures that are entirely unexpected in practice.

> The loop in ice_ptp_clear_tx_memory_status_eth56g() runs for all 64
> INDEX_PER_PORT entries. It counts failures and never stops early. It also
> runs before the PHY_REG_GLOBAL read, and that read is the first access in
> ice_ptp_phy_soft_reset_eth56g() that would catch an unreachable PHY.
> 
> Each index takes this path:
> 
> ice_clear_ptp_tstamp_eth56g()
>   ice_read_ptp_tstamp_eth56g()
>     ice_read_port_mem_eth56g()
>       ice_read_phy_eth56g()
>         ice_sbq_rw_reg()
>           ice_sq_send_cmd()
>             ice_sq_done()
> 
> ice_sq_done() polls with ICE_CTL_Q_SQ_CMD_TIMEOUT, which is USEC_PER_SEC.
> 
> If the sideband queue head stops advancing, each low read waits the full
> 1 second and then returns an error. A failed low read skips the high read,
> so this costs one timeout per index. Fast -ENOSPC failures only start once
> the 64 entry ring (ICE_SBQ_LEN) fills up.
> 
> That could mean about a minute of stall in the sweep before the
> PHY_REG_GLOBAL read fails and the error is returned. Before this patch, the
> same condition was caught after a single timeout on PHY_REG_GLOBAL.
> 
> ice_ptp_init_phc_e825c() calls ice_ptp_phy_soft_reset_eth56g(). That makes
> this reachable from both ice_ptp_init_owner() during probe and
> ice_ptp_rebuild_owner() during reset rebuild.
> 
> After the later commit in this series, "ice: E825: perform a soft reset
> when starting the PHY timer", it is also reachable from
> ice_start_phy_timer_eth56g() via ice_ptp_port_phy_restart(), which holds
> ptp_port->ps_lock.
> 
> Because the sweep runs before PHY_REG_GLOBAL is checked, its read side
> effects can also land on a port whose soft reset then fails. The new kdoc
> does tell callers to treat a soft reset failure as a non-functional
> timestamp block.
> 
> Would it make sense to stop the sweep after consecutive failures, or on a
> timeout class error? Another option is to read PHY_REG_GLOBAL before
> starting the sweep.

I will change this if the maintainers demand, but otherwise I would much
rather see my actual issues fixed than continue to be stalled endlessly
by this AI looping me between different versions.

> 
>>  		ice_debug(hw, ICE_DBG_PTP, "Failed to read PHY_REG_GLOBAL for port %d, err %d\n",
> 
> [ ... ]
> 

[PATCH net 3/3] 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
including its timestamp counter has been reset.

Note that despite the name, this PHY soft reset only impacts the timer
block of the PHY, and not its regular Tx/Rx functionality. Additionally,
many configuration registers such as the PHY_REG_TS_INT_CONFIG and
PHY_REG_TX_OFFSET_READY are not reset.

Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C products")
Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@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 ef59b34ba52d..ee945ce08eb7 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -2128,9 +2128,11 @@ int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port)
  * @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
@@ -2150,6 +2152,10 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
 	if (err)
 		return err;
 
+	err = ice_ptp_phy_soft_reset_eth56g(hw, port);
+	if (err)
+		return err;
+
 	ice_ptp_src_cmd(hw, ICE_PTP_NOP);
 
 	err = ice_phy_cfg_parpcs_eth56g(hw, port);

-- 
2.56.0.215.g47fdf2208c5b

Re: [PATCH net 3/3] ice: E825: perform a soft reset when starting the PHY timer

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In the E825 link-change restart path, the software Tx
  timestamp tracker is not flushed, yet the PHY timestamp memory it…
- [Medium] ice_ptp_phy_soft_reset_eth56g() now runs on every E825 PHY
  restart, including while Tx traffic is live, but in-flight timestamp…

--

ice: E825: perform a soft reset when starting the PHY timer

This commit adds a call to ice_ptp_phy_soft_reset_eth56g() in
ice_start_phy_timer_eth56g(), so the E825 PHY timer block starts in a
clean state. The soft reset sweeps the Tx timestamp memory and resets the
PHY timestamp counter before the PHY is programmed.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index ef59b34ba52dc..ee945ce08eb72 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c

[ ... ]

> @@ -2150,6 +2152,10 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
>  	if (err)
>  		return err;
>  
> +	err = ice_ptp_phy_soft_reset_eth56g(hw, port);
> +	if (err)
> +		return err;
> +

[Severity: Medium]

On the link change path, should the software Tx timestamp tracker be
flushed before this runs?

ice_start_phy_timer_eth56g() is also called at runtime. The chain is
ice_link_event() or ice_up_complete() -> ice_ptp_link_change() ->
ice_ptp_port_phy_restart(). For E825, that path only marks the
outstanding entries as stale:

drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_port_phy_restart() {
    ...
	case ICE_MAC_GENERIC_3K_E825:
		...
		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);

		err = ice_start_phy_timer_eth56g(hw, port);
    ...
}

ice_ptp_mark_tx_tracker_stale() only ORs in_use into stale. Both in_use
and tstamps[idx].skb stay set.

With this patch, ice_ptp_phy_soft_reset_eth56g() then calls
ice_ptp_clear_tx_memory_status_eth56g(). That function reads all 64
entries of the port, which clears their valid and ready bits. The soft
reset then clears the ready bitmap and the counter.

Take an in_use index whose timestamp the PHY has already captured but the
IRQ thread has not yet read. Its ready bit is now gone, so
ice_ptp_process_tx_tstamp() skips that index on every pass:

	if (tx->has_ready_bitmap &&
	    !(tstamp_ready & BIT_ULL(phy_idx))) {
		if (drop_ts)
			goto skip_ts_read;

		continue;
	}

This continues until the 2 second timeout fires, and the timeout also
increments pf->ptp.tx_hwtstamp_timeouts.

Until then, ice_misc_intr_thread_fn() keeps re-arming the interrupt,
because ice_ptp_tx_tstamps_pending() sees a non-empty in_use. Each pass
does a sideband read of the ready bitmap. The skb reference taken in
ice_ptp_request_ts() is also held for that whole time.

Before this patch, those entries stayed ready in hardware. They were read
and dropped right away through the stale bit, and were not counted as
timeouts.

ice_ptp_rebuild_owner() already flushes the tracker before
ice_ptp_restart_all_phy():

	/* Flush software tracking of any outstanding timestamps since we're
	 * about to flush the PHY timestamp block.
	 */
	ice_ptp_flush_all_tx_tracker(pf);

Does the link change path need the same flush? The timeout does
eventually release these entries, so the slots are not lost for good.

[Severity: Medium]

Could a timestamp captured during the soft reset window leave the PHY
counter and the timestamp memory out of sync?

On E825, the Tx queues can be live when this runs. ice_up_complete()
starts the queues before it restarts the PHY:

	netif_tx_start_all_queues(vsi->netdev);
	netif_carrier_on(vsi->netdev);
	ice_ptp_link_change(pf, true);

ice_link_event() also restarts on a speed change while the link stays up.

ice_ptp_port_phy_restart() sets tx.disabled, but descriptors already
posted with ICE_TX_FLAGS_TSYN can still be sent.
ice_stop_phy_timer_eth56g() also leaves PHY_REG_TX_OFFSET_READY set, so
such a packet is captured as valid.

Inside ice_ptp_phy_soft_reset_eth56g(), the memory sweep runs about 5 ms
before the reset is asserted, and the reset is deasserted about 5 ms
after that:

	ice_ptp_clear_tx_memory_status_eth56g(hw, port);
	...
	usleep_range(5000, 6000);

	global_val |= PHY_REG_GLOBAL_SOFT_RESET_M;

Suppose a packet is captured after the sweep but before the reset is
deasserted. The kernel-doc of ice_ptp_clear_tx_memory_status_eth56g()
says "the soft reset does not clear the timestamp memory", so that valid
entry stays in memory. Meanwhile the reset zeroes the counter and the
ready bitmap.

Only the 2 second timeout would free that software entry. The entry could
still be read, for example by a concurrent reader, or depending on how the
hardware handles reuse of an index that is already valid. Would that read
underflow the counter and stop Tx timestamp interrupts, as that kernel-doc
warns?

Should in-flight timestamp requests be quiesced before the soft reset?
Packets captured after the reset but before ice_sync_phy_timer_eth56g()
look like they are dropped safely through the stale bitmap.

>  	ice_ptp_src_cmd(hw, ICE_PTP_NOP);
>  
>  	err = ice_phy_cfg_parpcs_eth56g(hw, port);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-jk-e825c-critical-timestamping-fixes-v1-0-72128d1dd928%40intel.com

Re: [PATCH net 3/3] ice: E825: perform a soft reset when starting the PHY timer

[Jacob Keller]

On 10/30/2026 2:40 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] In the E825 link-change restart path, the software Tx
>   timestamp tracker is not flushed, yet the PHY timestamp memory it…
> - [Medium] ice_ptp_phy_soft_reset_eth56g() now runs on every E825 PHY
>   restart, including while Tx traffic is live, but in-flight timestamp…
> 
> --
> 

I believe these remaining issues are not realistic, or minor and not
something that should be holding us up from getting these fixes landed.
I had originally tried to mitigate some of these but have gotten so many
complaints and run in circles by Sashiko here. I do not know how to
modify this so that it won't complain....

Trying to break things into smaller pieces seems to get complaints about
the things that we had planned to fix with the full series. :\

> ice: E825: perform a soft reset when starting the PHY timer
> 
> This commit adds a call to ice_ptp_phy_soft_reset_eth56g() in
> ice_start_phy_timer_eth56g(), so the E825 PHY timer block starts in a
> clean state. The soft reset sweeps the Tx timestamp memory and resets the
> PHY timestamp counter before the PHY is programmed.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> index ef59b34ba52dc..ee945ce08eb72 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> 
> [ ... ]
> 
>> @@ -2150,6 +2152,10 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
>>  	if (err)
>>  		return err;
>>  
>> +	err = ice_ptp_phy_soft_reset_eth56g(hw, port);
>> +	if (err)
>> +		return err;
>> +
> 
> [Severity: Medium]
> 
> On the link change path, should the software Tx timestamp tracker be
> flushed before this runs?
> 

I tried that in a previous version and the AI pointed out all kinds of
possible races that might occur then.

> ice_start_phy_timer_eth56g() is also called at runtime. The chain is
> ice_link_event() or ice_up_complete() -> ice_ptp_link_change() ->
> ice_ptp_port_phy_restart(). For E825, that path only marks the
> outstanding entries as stale:
> 
> drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_port_phy_restart() {
>     ...
> 	case ICE_MAC_GENERIC_3K_E825:
> 		...
> 		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
> 
> 		err = ice_start_phy_timer_eth56g(hw, port);
>     ...
> }
> 
> ice_ptp_mark_tx_tracker_stale() only ORs in_use into stale. Both in_use
> and tstamps[idx].skb stay set.
> 
> With this patch, ice_ptp_phy_soft_reset_eth56g() then calls
> ice_ptp_clear_tx_memory_status_eth56g(). That function reads all 64
> entries of the port, which clears their valid and ready bits. The soft
> reset then clears the ready bitmap and the counter.
> 
> Take an in_use index whose timestamp the PHY has already captured but the
> IRQ thread has not yet read. Its ready bit is now gone, so
> ice_ptp_process_tx_tstamp() skips that index on every pass:
> 
> 	if (tx->has_ready_bitmap &&
> 	    !(tstamp_ready & BIT_ULL(phy_idx))) {
> 		if (drop_ts)
> 			goto skip_ts_read;
> 
> 		continue;
> 	}
> 
> This continues until the 2 second timeout fires, and the timeout also
> increments pf->ptp.tx_hwtstamp_timeouts.
> 

Yes. This is the expected path in the current logic.

> Until then, ice_misc_intr_thread_fn() keeps re-arming the interrupt,
> because ice_ptp_tx_tstamps_pending() sees a non-empty in_use. Each pass
> does a sideband read of the ready bitmap. The skb reference taken in
> ice_ptp_request_ts() is also held for that whole time.
> 
> Before this patch, those entries stayed ready in hardware. They were read
> and dropped right away through the stale bit, and were not counted as
> timeouts.
> 
> ice_ptp_rebuild_owner() already flushes the tracker before
> ice_ptp_restart_all_phy():
> 
> 	/* Flush software tracking of any outstanding timestamps since we're
> 	 * about to flush the PHY timestamp block.
> 	 */
> 	ice_ptp_flush_all_tx_tracker(pf);
> 

Yes, but I am now certain that if we added this today it would get
rejected because the flushing appears to be problematic at run time. In
fact part of the other areas of the previous series was done to help
prevent this, but was undergoing its own challenges.

> Does the link change path need the same flush? The timeout does
> eventually release these entries, so the slots are not lost for good.
> 
> [Severity: Medium]
> 
> Could a timestamp captured during the soft reset window leave the PHY
> counter and the timestamp memory out of sync?
> 

I do not believe that the hardware will actually perform timestamps
while the soft reset is active.

> On E825, the Tx queues can be live when this runs. ice_up_complete()
> starts the queues before it restarts the PHY:
> 
> 	netif_tx_start_all_queues(vsi->netdev);
> 	netif_carrier_on(vsi->netdev);
> 	ice_ptp_link_change(pf, true);
> 
> ice_link_event() also restarts on a speed change while the link stays up.
> 
> ice_ptp_port_phy_restart() sets tx.disabled, but descriptors already
> posted with ICE_TX_FLAGS_TSYN can still be sent.
> ice_stop_phy_timer_eth56g() also leaves PHY_REG_TX_OFFSET_READY set, so
> such a packet is captured as valid.
> 
> Inside ice_ptp_phy_soft_reset_eth56g(), the memory sweep runs about 5 ms
> before the reset is asserted, and the reset is deasserted about 5 ms
> after that:
> 
> 	ice_ptp_clear_tx_memory_status_eth56g(hw, port);
> 	...
> 	usleep_range(5000, 6000);
> 
> 	global_val |= PHY_REG_GLOBAL_SOFT_RESET_M;
> 
> Suppose a packet is captured after the sweep but before the reset is
> deasserted. The kernel-doc of ice_ptp_clear_tx_memory_status_eth56g()
> says "the soft reset does not clear the timestamp memory", so that valid
> entry stays in memory. Meanwhile the reset zeroes the counter and the
> ready bitmap.
> 

Again, this is working on the assumption that hardware will actually
perform a timestamp while the soft reset is asserted.

> Only the 2 second timeout would free that software entry. The entry could
> still be read, for example by a concurrent reader, or depending on how the
> hardware handles reuse of an index that is already valid. Would that read
> underflow the counter and stop Tx timestamp interrupts, as that kernel-doc
> warns?
> 
> Should in-flight timestamp requests be quiesced before the soft reset?
> Packets captured after the reset but before ice_sync_phy_timer_eth56g()
> look like they are dropped safely through the stale bitmap.
> 

We did have changes in the rest of the full series to address this by
having the driver stop new requests while the timer was stopped, and
marking things as stale ensures that the indexes won't get released. I
had also tried to make sure that stale packets didn't increase the
timeout counter unexpectedly, and to change so that the skb was freed
earlier. Those were all delayed because of other issues on the full series.

>>  	ice_ptp_src_cmd(hw, ICE_PTP_NOP);
>>  
>>  	err = ice_phy_cfg_parpcs_eth56g(hw, port);
>