> -----Original Message-----
> From: Grzegorz Nitka <[email protected]>
> Sent: Saturday, September 26, 2026 12:28 AM
> To: [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Kitszel, Przemyslaw
> <[email protected]>; Nguyen, Anthony L
> <[email protected]>; Kubalewski, Arkadiusz
> <[email protected]>; [email protected];
> [email protected]; Machnikowski, Maciej
> <[email protected]>; Nitka, Grzegorz
> <[email protected]>; Neustadter, Marc
> <[email protected]>; Temerkhanov, Sergey
> <[email protected]>; Olech, Milena <[email protected]>
> Subject: [PATCH iwl-net 1/2] ice: ptp: serialize E825 PHY timer start
> with PTP lock and incval
>
> From: Maciek Machnikowski <[email protected]>
>
> 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 <[email protected]>
> Co-developed-by: Grzegorz Nitka <[email protected]>
> Signed-off-by: Grzegorz Nitka <[email protected]>
> Suggested-by: Marc Neustadter <[email protected]>
> Reviewed-by: Sergey Temerkhanov <[email protected]>
> Reviewed-by: Milena Olech <[email protected]>
> ---
> 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
Reviewed-by: Aleksandr Loktionov <[email protected]>
