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

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Grzegorz Nitka <grzegorz.nitka@xxxxxxxxx>
> Sent: Saturday, September 26, 2026 12:28 AM
> To: intel-wired-lan@xxxxxxxxxxxxxxxx
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> richardcochran@xxxxxxxxx; andrew+netdev@xxxxxxx; Kitszel, Przemyslaw
> <przemyslaw.kitszel@xxxxxxxxx>; Nguyen, Anthony L
> <anthony.l.nguyen@xxxxxxxxx>; Kubalewski, Arkadiusz
> <arkadiusz.kubalewski@xxxxxxxxx>; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; Machnikowski, Maciej
> <maciej.machnikowski@xxxxxxxxx>; Nitka, Grzegorz
> <grzegorz.nitka@xxxxxxxxx>; Neustadter, Marc
> <marc.neustadter@xxxxxxxxx>; Temerkhanov, Sergey
> <sergey.temerkhanov@xxxxxxxxx>; Olech, Milena <milena.olech@xxxxxxxxx>
> Subject: [PATCH iwl-net 1/2] ice: ptp: serialize E825 PHY timer start
> with PTP lock and incval
> 
> 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>
> ---
>  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
>   * @hw: pointer to the HW struct
>   * @port: the PHY port to synchronize
>   *
> @@ -2058,22 +2058,18 @@ static int
> ice_read_phy_and_phc_time_eth56g(struct ice_hw *hw, u8 port,
>   *
>   * Return:
>   * * %0     - success
> - * * %-EBUSY- failed to acquire PTP semaphore
>   * * %other - PHY read/write failed
> + *
> + * The caller must acquire PTP semaphore lock
>   */
> -static int ice_sync_phy_timer_eth56g(struct ice_hw *hw, u8 port)
> +static int ice_sync_phy_timer_eth56g_unlocked(struct ice_hw *hw, u8
> +port)
>  {
>  	u64 phc_time, phy_time, difference;
>  	int err;
> 
> -	if (!ice_ptp_lock(hw)) {
> -		ice_debug(hw, ICE_DBG_PTP, "Failed to acquire PTP
> semaphore\n");
> -		return -EBUSY;
> -	}
> -
>  	err = ice_read_phy_and_phc_time_eth56g(hw, port, &phy_time,
> &phc_time);
>  	if (err)
> -		goto err_unlock;
> +		return err;
> 
>  	/* Calculate the amount required to add to the port time in
> order for
>  	 * it to match the PHC time.
> @@ -2089,11 +2085,11 @@ static int ice_sync_phy_timer_eth56g(struct
> ice_hw *hw, u8 port)
> 
>  	err = ice_ptp_prep_port_adj_eth56g(hw, port, (s64)difference);
>  	if (err)
> -		goto err_unlock;
> +		return err;
> 
>  	err = ice_ptp_one_port_cmd(hw, port, ICE_PTP_ADJ_TIME);
>  	if (err)
> -		goto err_unlock;
> +		return err;
> 
>  	/* Issue the sync to activate the time adjustment */
>  	ice_ptp_exec_tmr_cmd(hw);
> @@ -2103,15 +2099,13 @@ static int ice_sync_phy_timer_eth56g(struct
> ice_hw *hw, u8 port)
>  	 */
>  	err = ice_read_phy_and_phc_time_eth56g(hw, port, &phy_time,
> &phc_time);
>  	if (err)
> -		goto err_unlock;
> +		return err;
> 
>  	dev_info(ice_hw_to_dev(hw),
>  		 "Port %u PHY time synced to PHC: 0x%016llX,
> 0x%016llX\n",
>  		 port, phy_time, phc_time);
> 
> -err_unlock:
> -	ice_ptp_unlock(hw);
> -	return err;
> +	return 0;
>  }
> 
>  /**
> @@ -2174,19 +2168,24 @@ int ice_start_phy_timer_eth56g(struct ice_hw
> *hw, u8 port)
>  	if (err)
>  		return err;
> 
> +	if (!ice_ptp_lock(hw)) {
> +		dev_err(ice_hw_to_dev(hw), "Failed to acquire PTP
> semaphore\n");
> +		return -EBUSY;
> +	}
> +
>  	ice_ptp_src_cmd(hw, ICE_PTP_NOP);
> 
>  	err = ice_phy_cfg_parpcs_eth56g(hw, port);
>  	if (err)
> -		return err;
> +		goto err_ptp_unlock;
> 
>  	err = ice_phy_cfg_ptp_1step_eth56g(hw, port);
>  	if (err)
> -		return err;
> +		goto err_ptp_unlock;
> 
>  	err = ice_phy_cfg_mac_eth56g(hw, port);
>  	if (err)
> -		return err;
> +		goto err_ptp_unlock;
> 
>  	if (ice_is_primary(hw)) {
>  		lo = rd32(hw, GLTSYN_INCVAL_L(tmr_idx)); @@ -2203,11
> +2202,6 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
>  	}
>  	incval = (u64)hi << 32 | lo;
> 
> -	if (!ice_ptp_lock(hw)) {
> -		dev_err(ice_hw_to_dev(hw), "Failed to acquire PTP
> semaphore\n");
> -		return -EBUSY;
> -	}
> -
>  	err = ice_write_40b_ptp_reg_eth56g(hw, port, PHY_REG_TIMETUS_L,
> incval);
>  	if (err)
>  		goto err_ptp_unlock;
> @@ -2218,11 +2212,11 @@ int ice_start_phy_timer_eth56g(struct ice_hw
> *hw, u8 port)
> 
>  	ice_ptp_exec_tmr_cmd(hw);
> 
> -	ice_ptp_unlock(hw);
> -
> -	err = ice_sync_phy_timer_eth56g(hw, port);
> +	err = ice_sync_phy_timer_eth56g_unlocked(hw, port);
>  	if (err)
> -		return err;
> +		goto err_ptp_unlock;
> +
> +	ice_ptp_unlock(hw);
> 
>  	err = ice_write_ptp_reg_eth56g(hw, port,
> PHY_REG_TX_OFFSET_READY, 1);
>  	if (err)
> --
> 2.39.3

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@xxxxxxxxx>