[PATCH iwl-net 0/2] ice: fix E825 PTP timer race and drop redundant cross-timestamp access

[PATCH iwl-net 0/2] ice: fix E825 PTP timer race and drop redundant cross-timestamp access

[Grzegorz Nitka]

This series fixes a race in the E825 PHY timer start path and removes a
redundant cross-timestamp command programming sequence.

The first patch serializes the E825 PHY timer start sequence with the PTP
lock and keeps the shared GLTSYN_CMD register access and incval programming
within the same critical section. This prevents concurrent PTP updates from
clobbering the source timer command and leaving the E825 PHY clock offset
from the PHC.

The second patch removes the redundant cross-timestamp programming of
GLTSYN_CMD in the capture path. The register accesses were mistakenly
assumed to be required for E823-class devices. In fact, it was the
E824-family (not supported by this driver) not E823.

Grzegorz Nitka (1):
  ice: remove redundant cross-timestamp PTP command

Maciek Machnikowski (1):
  ice: ptp: serialize E825 PHY timer start with PTP lock and incval

 drivers/net/ethernet/intel/ice/ice_ptp.c    |  6 ---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 48 +++++++++------------
 2 files changed, 21 insertions(+), 33 deletions(-)


base-commit: 349099e5d8082ab01e60d91fba9d107591c17371
-- 
2.39.3

[PATCH iwl-net 1/2] ice: ptp: serialize E825 PHY timer start with PTP lock and incval

[Grzegorz Nitka]

From: Maciek Machnikowski <maciej.machnikowski@intel.com>

ice_start_phy_timer_eth56g() programmed the PHY increment value and
performed the PHY-to-PHC phase synchronization in two separate PTP
semaphore critical sections, and touched the shared source command
register (GLTSYN_CMD via ice_ptp_src_cmd()) and read the source incval
before acquiring the semaphore at all.

Because GLTSYN_CMD is a single global register latched for the source
timer and all PHY ports by GLTSYN_CMD_SYNC, a concurrent adjtime (small
offset, <= S32_MAX) or adjfine could clobber the source command between
the incval-init and the phase-sync steps. In the worst case the source
timer command written by ice_ptp_adj_clock() (ICE_PTP_ADJ_TIME) was
overwritten by the unlocked ICE_PTP_NOP from ice_start_phy_timer_eth56g(),
so the adjustment was applied to the PHY ports but not to the PHC. This
left the E825 PHY clock permanently offset from the PHC, showing up as an
intermittent clock drift.

Hold the PTP semaphore across the entire start sequence: acquire it
before the first shared-register access (ice_ptp_src_cmd()), keep it held
through the incval read, PHY incval init, and the phase synchronization,
then release it once. Since the hardware semaphore is not recursive, split
the phase-sync helper into ice_sync_phy_timer_eth56g_unlocked(), which
assumes the caller already holds the lock, and call it directly from
ice_start_phy_timer_eth56g(). The locking wrapper is removed as it no
longer has any callers.

Fixes: 781ff8f2d575 ("ice: ptp: serialize E825 PHY timer start with PTP lock")
Signed-off-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
Co-developed-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
Suggested-by: Marc Neustadter <marc.neustadter@intel.com>
Reviewed-by: Sergey Temerkhanov <sergey.temerkhanov@intel.com>
Reviewed-by: Milena Olech <milena.olech@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 48 +++++++++------------
 1 file changed, 21 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 20bd1813650f..d3e4aeb7e496 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -2045,7 +2045,7 @@ static int ice_read_phy_and_phc_time_eth56g(struct ice_hw *hw, u8 port,
 }
 
 /**
- * ice_sync_phy_timer_eth56g - Synchronize the PHY timer with PHC timer
+ * ice_sync_phy_timer_eth56g_unlocked - Synchronize the PHY timer with PHC timer
  * @hw: pointer to the HW struct
  * @port: the PHY port to synchronize
  *
@@ -2058,22 +2058,18 @@ static int ice_read_phy_and_phc_time_eth56g(struct ice_hw *hw, u8 port,
  *
  * Return:
  * * %0     - success
- * * %-EBUSY- failed to acquire PTP semaphore
  * * %other - PHY read/write failed
+ *
+ * The caller must acquire PTP semaphore lock
  */
-static int ice_sync_phy_timer_eth56g(struct ice_hw *hw, u8 port)
+static int ice_sync_phy_timer_eth56g_unlocked(struct ice_hw *hw, u8 port)
 {
 	u64 phc_time, phy_time, difference;
 	int err;
 
-	if (!ice_ptp_lock(hw)) {
-		ice_debug(hw, ICE_DBG_PTP, "Failed to acquire PTP semaphore\n");
-		return -EBUSY;
-	}
-
 	err = ice_read_phy_and_phc_time_eth56g(hw, port, &phy_time, &phc_time);
 	if (err)
-		goto err_unlock;
+		return err;
 
 	/* Calculate the amount required to add to the port time in order for
 	 * it to match the PHC time.
@@ -2089,11 +2085,11 @@ static int ice_sync_phy_timer_eth56g(struct ice_hw *hw, u8 port)
 
 	err = ice_ptp_prep_port_adj_eth56g(hw, port, (s64)difference);
 	if (err)
-		goto err_unlock;
+		return err;
 
 	err = ice_ptp_one_port_cmd(hw, port, ICE_PTP_ADJ_TIME);
 	if (err)
-		goto err_unlock;
+		return err;
 
 	/* Issue the sync to activate the time adjustment */
 	ice_ptp_exec_tmr_cmd(hw);
@@ -2103,15 +2099,13 @@ static int ice_sync_phy_timer_eth56g(struct ice_hw *hw, u8 port)
 	 */
 	err = ice_read_phy_and_phc_time_eth56g(hw, port, &phy_time, &phc_time);
 	if (err)
-		goto err_unlock;
+		return err;
 
 	dev_info(ice_hw_to_dev(hw),
 		 "Port %u PHY time synced to PHC: 0x%016llX, 0x%016llX\n",
 		 port, phy_time, phc_time);
 
-err_unlock:
-	ice_ptp_unlock(hw);
-	return err;
+	return 0;
 }
 
 /**
@@ -2174,19 +2168,24 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
 	if (err)
 		return err;
 
+	if (!ice_ptp_lock(hw)) {
+		dev_err(ice_hw_to_dev(hw), "Failed to acquire PTP semaphore\n");
+		return -EBUSY;
+	}
+
 	ice_ptp_src_cmd(hw, ICE_PTP_NOP);
 
 	err = ice_phy_cfg_parpcs_eth56g(hw, port);
 	if (err)
-		return err;
+		goto err_ptp_unlock;
 
 	err = ice_phy_cfg_ptp_1step_eth56g(hw, port);
 	if (err)
-		return err;
+		goto err_ptp_unlock;
 
 	err = ice_phy_cfg_mac_eth56g(hw, port);
 	if (err)
-		return err;
+		goto err_ptp_unlock;
 
 	if (ice_is_primary(hw)) {
 		lo = rd32(hw, GLTSYN_INCVAL_L(tmr_idx));
@@ -2203,11 +2202,6 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
 	}
 	incval = (u64)hi << 32 | lo;
 
-	if (!ice_ptp_lock(hw)) {
-		dev_err(ice_hw_to_dev(hw), "Failed to acquire PTP semaphore\n");
-		return -EBUSY;
-	}
-
 	err = ice_write_40b_ptp_reg_eth56g(hw, port, PHY_REG_TIMETUS_L, incval);
 	if (err)
 		goto err_ptp_unlock;
@@ -2218,11 +2212,11 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
 
 	ice_ptp_exec_tmr_cmd(hw);
 
-	ice_ptp_unlock(hw);
-
-	err = ice_sync_phy_timer_eth56g(hw, port);
+	err = ice_sync_phy_timer_eth56g_unlocked(hw, port);
 	if (err)
-		return err;
+		goto err_ptp_unlock;
+
+	ice_ptp_unlock(hw);
 
 	err = ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_OFFSET_READY, 1);
 	if (err)
-- 
2.39.3

[PATCH iwl-net 2/2] ice: remove redundant cross-timestamp PTP command

[Grzegorz Nitka]

The cross-timestamp capture path programs GLTSYN_CMD with READ_TIME before
starting the ART/device capture and clears it with NOP afterwards.

It was assumed that this change was needed for E823 devices, which
turned out to be not true. The hardware supported by this driver
does not require it.

Since GLTSYN_CMD is shared with other PTP operations, touching it here
only adds a potential race source.
Remove the redundant source command programming instead of adding another
guard around it.

Testing hints:
Verify crosstimestamp path, for example with phc2sys tool, on
E822/E823/E825/E830 devices. Please note that the phc2sys tool performs
cross-timestamping by default when supported by the hardware. This can
be observed when the reported delay is 0, indicating that the system
time and PHC time were latched simultaneously.

Fixes: 88c360e49f51 ("ice: Support cross-timestamping for E823 devices")
Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
Reviewed-by: Sergey Temerkhanov <sergey.temerkhanov@intel.com>
Reviewed-by: Milena Olech <milena.olech@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index fd97190a89a9..23c0a45050d7 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -2229,9 +2229,6 @@ static int ice_capture_crosststamp(ktime_t *device,
 	/* Snapshot system time for historic interpolation */
 	ktime_get_snapshot_id(ctx->snapshot_clock_id, &ctx->snapshot);
 
-	/* Program cmd to master timer */
-	ice_ptp_src_cmd(hw, ICE_PTP_READ_TIME);
-
 	/* Start the ART and device clock sync sequence */
 	ctl = rd32(hw, cfg->ctl_reg);
 	ctl |= cfg->ctl_active;
@@ -2258,9 +2255,6 @@ static int ice_capture_crosststamp(ktime_t *device,
 	*device = ns_to_ktime(ts);
 
 err_timeout:
-	/* Clear the master timer */
-	ice_ptp_src_cmd(hw, ICE_PTP_NOP);
-
 	/* Release HW lock */
 	lock = rd32(hw, cfg->lock_reg);
 	lock &= ~cfg->lock_busy;
-- 
2.39.3

Re: [PATCH iwl-net 1/2] ice: ptp: serialize E825 PHY timer start with PTP lock and incval

[Simon Horman]

On Sat, Sep 26, 2026 at 12:28:23AM +0200, Grzegorz Nitka wrote:
> From: Maciek Machnikowski <maciej.machnikowski@intel.com>
> 
> ice_start_phy_timer_eth56g() programmed the PHY increment value and
> performed the PHY-to-PHC phase synchronization in two separate PTP
> semaphore critical sections, and touched the shared source command
> register (GLTSYN_CMD via ice_ptp_src_cmd()) and read the source incval
> before acquiring the semaphore at all.
> 
> Because GLTSYN_CMD is a single global register latched for the source
> timer and all PHY ports by GLTSYN_CMD_SYNC, a concurrent adjtime (small
> offset, <= S32_MAX) or adjfine could clobber the source command between
> the incval-init and the phase-sync steps. In the worst case the source
> timer command written by ice_ptp_adj_clock() (ICE_PTP_ADJ_TIME) was
> overwritten by the unlocked ICE_PTP_NOP from ice_start_phy_timer_eth56g(),
> so the adjustment was applied to the PHY ports but not to the PHC. This
> left the E825 PHY clock permanently offset from the PHC, showing up as an
> intermittent clock drift.
> 
> Hold the PTP semaphore across the entire start sequence: acquire it
> before the first shared-register access (ice_ptp_src_cmd()), keep it held
> through the incval read, PHY incval init, and the phase synchronization,
> then release it once. Since the hardware semaphore is not recursive, split
> the phase-sync helper into ice_sync_phy_timer_eth56g_unlocked(), which
> assumes the caller already holds the lock, and call it directly from
> ice_start_phy_timer_eth56g(). The locking wrapper is removed as it no
> longer has any callers.
> 
> Fixes: 781ff8f2d575 ("ice: ptp: serialize E825 PHY timer start with PTP lock")
> Signed-off-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> Co-developed-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> Suggested-by: Marc Neustadter <marc.neustadter@intel.com>
> Reviewed-by: Sergey Temerkhanov <sergey.temerkhanov@intel.com>
> Reviewed-by: Milena Olech <milena.olech@intel.com>

Reviewed-by: Simon Horman <horms@kernel.org>

> ---
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 48 +++++++++------------
>  1 file changed, 21 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index 20bd1813650f..d3e4aeb7e496 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> @@ -2045,7 +2045,7 @@ static int ice_read_phy_and_phc_time_eth56g(struct ice_hw *hw, u8 port,
>  }
>  
>  /**
> - * ice_sync_phy_timer_eth56g - Synchronize the PHY timer with PHC timer
> + * ice_sync_phy_timer_eth56g_unlocked - Synchronize the PHY timer with PHC timer

nit: I'm not entirely sure renaming the function is warranted.
     But it's your call.

>   * @hw: pointer to the HW struct
>   * @port: the PHY port to synchronize
>   *

...

Re: [PATCH iwl-net 2/2] ice: remove redundant cross-timestamp PTP command

[Simon Horman]

On Sat, Sep 26, 2026 at 12:28:24AM +0200, Grzegorz Nitka wrote:
> The cross-timestamp capture path programs GLTSYN_CMD with READ_TIME before
> starting the ART/device capture and clears it with NOP afterwards.
> 
> It was assumed that this change was needed for E823 devices, which
> turned out to be not true. The hardware supported by this driver
> does not require it.
> 
> Since GLTSYN_CMD is shared with other PTP operations, touching it here
> only adds a potential race source.
> Remove the redundant source command programming instead of adding another
> guard around it.
> 
> Testing hints:
> Verify crosstimestamp path, for example with phc2sys tool, on
> E822/E823/E825/E830 devices. Please note that the phc2sys tool performs
> cross-timestamping by default when supported by the hardware. This can
> be observed when the reported delay is 0, indicating that the system
> time and PHC time were latched simultaneously.
> 
> Fixes: 88c360e49f51 ("ice: Support cross-timestamping for E823 devices")
> Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> Reviewed-by: Sergey Temerkhanov <sergey.temerkhanov@intel.com>
> Reviewed-by: Milena Olech <milena.olech@intel.com>

Reviewed-by: Simon Horman <horms@kernel.org>