On 9/26/2026 10:05 AM, Simon Horman wrote: > On Sat, Sep 26, 2026 at 12:28:23AM +0200, Grzegorz Nitka wrote: >> 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]> > > Reviewed-by: Simon Horman <[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 > > nit: I'm not entirely sure renaming the function is warranted. > But it's your call. >
I agree, renaming this is unnecessary, and I think slightly confusing as usually "unlocked" should mean "this will take the lock" while "_locked" means "the caller is already locked. >> * @hw: pointer to the HW struct >> * @port: the PHY port to synchronize >> * > > ...
