* [PATCH iwl-net 1/2] ice: ptp: serialize E825 PHY timer start with PTP lock and incval
2026-09-25 22:28 [PATCH iwl-net 0/2] ice: fix E825 PTP timer race and drop redundant cross-timestamp access Grzegorz Nitka
@ 2026-09-25 22:28 ` Grzegorz Nitka
2026-09-26 17:05 ` Simon Horman
2026-09-25 22:28 ` [PATCH iwl-net 2/2] ice: remove redundant cross-timestamp PTP command Grzegorz Nitka
1 sibling, 1 reply; 5+ messages in thread
From: Grzegorz Nitka @ 2026-09-25 22:28 UTC (permalink / raw)
To: intel-wired-lan
Cc: netdev, linux-kernel, richardcochran, andrew+netdev,
przemyslaw.kitszel, anthony.l.nguyen, arkadiusz.kubalewski,
davem, edumazet, Maciek Machnikowski, Grzegorz Nitka,
Marc Neustadter, Sergey Temerkhanov, Milena Olech
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH iwl-net 2/2] ice: remove redundant cross-timestamp PTP command
2026-09-25 22:28 [PATCH iwl-net 0/2] ice: fix E825 PTP timer race and drop redundant cross-timestamp access Grzegorz Nitka
2026-09-25 22:28 ` [PATCH iwl-net 1/2] ice: ptp: serialize E825 PHY timer start with PTP lock and incval Grzegorz Nitka
@ 2026-09-25 22:28 ` Grzegorz Nitka
2026-09-26 17:06 ` Simon Horman
1 sibling, 1 reply; 5+ messages in thread
From: Grzegorz Nitka @ 2026-09-25 22:28 UTC (permalink / raw)
To: intel-wired-lan
Cc: netdev, linux-kernel, richardcochran, andrew+netdev,
przemyslaw.kitszel, anthony.l.nguyen, arkadiusz.kubalewski,
davem, edumazet, Grzegorz Nitka, Sergey Temerkhanov,
Milena Olech
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
^ permalink raw reply [flat|nested] 5+ messages in thread