Re: [PATCH iwl-net 07/12] ice: E825: perform a soft reset when starting the PHY timer

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

On 8/21/2026 5:13 PM, Jacob Keller wrote:
> To ensure that the E825 PHY timer begins in a clean state, initiate a PHY
> soft reset prior to programming the PHY. This ensures that we clear any
> outstanding Tx timestamp memory, and ensures that the PHY internal state
> has been completely reset.
> 
> Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C products")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index b7d217ac31f3..10e973876608 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> @@ -2119,9 +2119,11 @@ int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port, bool soft_reset)
>   * @hw: pointer to the HW struct
>   * @port: the PHY port to start
>   *
> - * Start the clock of a PHY port. This must be done as part of the flow to
> - * re-calibrate Tx and Rx timestamping offsets whenever the clock time is
> - * initialized or when link speed changes.
> + * Perform a PHY soft reset and then start the clock for the PHY port.
> + *
> + * This must be done as part of the flow to re-calibrate Tx and Rx
> + * timestamping offsets whenever the clock time is initialized or when link
> + * speed changes.
>   *
>   * Return:
>   * * %0     - success
> @@ -2137,6 +2139,10 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
>  
>  	tmr_idx = ice_get_ptp_src_clock_index(hw);
>  
> +	err = ice_ptp_phy_soft_reset_eth56g(hw, port);
> +	if (err)
> +		return err;
> +
>  	err = ice_stop_phy_timer_eth56g(hw, port, false);
>  	if (err)
>  		return err;
> 

Sashiko points out a couple of pre-existing issues that are unrelated to
this change:

> This is a pre-existing issue, but does ice_ptp_update_cached_phctime() use
> the ICE_CFG_BUSY bit flag as an ad-hoc lock?
> In drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_update_cached_phctime(),
> the flag guards iteration over the vsi array:
>     if (test_and_set_bit(ICE_CFG_BUSY, pf->state))
>         return -EAGAIN;
> and then clears it on exit:
>     clear_bit(ICE_CFG_BUSY, pf->state);
> Does using a bit flag instead of a proper lock bypass lockdep and deadlock
> detection?

This likely could and should be improved, but I am not certain if it
makes sense to try and do so in this series, as this affects much larger
portions of the driver and is a long standing issue with the ice driver
(and Intel drivers in general) :\

> This is a pre-existing issue, but does the lockless read of
> tx->tstamps[idx].start in ice_ptp_process_tx_tstamp() race with its
> initialization?
> In drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_request_ts(), the in_use
> bit is set before the start timestamp is initialized:
>     set_bit(idx, tx->in_use);
>     clear_bit(idx, tx->stale);
>     tx->tstamps[idx].start = jiffies;
> If a concurrent reader in ice_ptp_process_tx_tstamp() iterates over the
> in_use bits locklessly:
>     for_each_set_bit(idx, tx->in_use, tx->len) {
>         ...
>         if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ)) {
>             drop_ts = true;
> can it observe the newly set in_use bit and read a stale start value,
> causing it to erroneously drop a valid SKB?

This should be fixed, and we can fix it by correctly ordering so that
the start time is updated before setting the bit, (possibly with a
barrier?). I will add the fix to the series.