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

[Jacob Keller <jacob.e.keller@intel.com>]

On 11/5/2026 5:20 AM, Nitka, Grzegorz wrote:
> 
> 
>> -----Original Message-----
>> From: Keller, Jacob E <jacob.e.keller@intel.com>
>> Sent: Thursday, October 22, 2026 9:10 AM
>> To: Simon Horman <horms@kernel.org>; Nitka, Grzegorz
>> <grzegorz.nitka@intel.com>
>> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
>> kernel@vger.kernel.org; richardcochran@gmail.com;
>> andrew+netdev@lunn.ch; Kitszel, Przemyslaw
>> <przemyslaw.kitszel@intel.com>; Nguyen, Anthony L
>> <anthony.l.nguyen@intel.com>; Kubalewski, Arkadiusz
>> <arkadiusz.kubalewski@intel.com>; davem@davemloft.net;
>> edumazet@google.com; Machnikowski, Maciej
>> <maciej.machnikowski@intel.com>; Neustadter, Marc
>> <marc.neustadter@intel.com>; Temerkhanov, Sergey
>> <sergey.temerkhanov@intel.com>; Olech, Milena <milena.olech@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.machnikowski@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.machnikowski@intel.com>
>>>> Co-developed-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
>>>> Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
>>>> Suggested-by: Marc Neustadter <marc.neustadter@intel.com>
>>>> Reviewed-by: Sergey Temerkhanov <sergey.temerkhanov@intel.com>
>>>> Reviewed-by: Milena Olech <milena.olech@intel.com>
>>>
>>> Reviewed-by: Simon Horman <horms@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!
> 
I have adjusted the code on dev-queue and I have made a note for Tony to
double check what we submit uses this name when going to netdev.