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

[Jacob Keller]

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.machnikowski@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> Co-developed-by: Grzegorz Nitka <grzegorz.nitka@xxxxxxxxx>
>> Signed-off-by: Grzegorz Nitka <grzegorz.nitka@xxxxxxxxx>
>> Suggested-by: Marc Neustadter <marc.neustadter@xxxxxxxxx>
>> Reviewed-by: Sergey Temerkhanov <sergey.temerkhanov@xxxxxxxxx>
>> Reviewed-by: Milena Olech <milena.olech@xxxxxxxxx>
> 
> Reviewed-by: Simon Horman <horms@xxxxxxxxxx>
> 
>> ---
>>  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
>>   *
> 
> ...