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. > * @hw: pointer to the HW struct > * @port: the PHY port to synchronize > * ...
