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

[Nitka, Grzegorz]

> -----Original Message-----
> From: Keller, Jacob E <jacob.e.kel...@intel.com>
> Sent: Thursday, October 22, 2026 9:10 AM
> To: Simon Horman <ho...@kernel.org>; Nitka, Grzegorz
> <grzegorz.ni...@intel.com>
> Cc: intel-wired-...@lists.osuosl.org; net...@vger.kernel.org; linux-
> ker...@vger.kernel.org; richardcoch...@gmail.com;
> andrew+net...@lunn.ch; Kitszel, Przemyslaw
> <przemyslaw.kits...@intel.com>; Nguyen, Anthony L
> <anthony.l.ngu...@intel.com>; Kubalewski, Arkadiusz
> <arkadiusz.kubalew...@intel.com>; da...@davemloft.net;
> eduma...@google.com; Machnikowski, Maciej
> <maciej.machnikow...@intel.com>; Neustadter, Marc
> <marc.neustad...@intel.com>; Temerkhanov, Sergey
> <sergey.temerkha...@intel.com>; Olech, Milena <milena.ol...@intel.com>
> Subject: Re: [PATCH iwl-net 1/2] ice: ptp: serialize E825 PHY timer start with
> PTP lock and incval
> 
> 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 <maciej.machnikow...@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.machnikow...@intel.com>
> >> Co-developed-by: Grzegorz Nitka <grzegorz.ni...@intel.com>
> >> Signed-off-by: Grzegorz Nitka <grzegorz.ni...@intel.com>
> >> Suggested-by: Marc Neustadter <marc.neustad...@intel.com>
> >> Reviewed-by: Sergey Temerkhanov <sergey.temerkha...@intel.com>
> >> Reviewed-by: Milena Olech <milena.ol...@intel.com>
> >
> > Reviewed-by: Simon Horman <ho...@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.
> >
> 
> 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.
> 
OK and agree. Let's keep the original function name.
Thanks!

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