Re: [PATCH net 09/15] ice: E825: perform a soft reset when starting the PHY timer

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

On 9/15/2026 6:12 PM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> ice: E825: perform a soft reset when starting the PHY timer
> 
> This adds a call to ice_ptp_phy_soft_reset_eth56g() at the top of
> ice_start_phy_timer_eth56g() so that the E825 PHY PTP block is reset before
> it is reprogrammed. The kernel-doc for the function is updated to mention
> the reset.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> index a3a9f7ce04d2b..64e18fe1176b1 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> 
> [ ... ]
> 
>> @@ -2138,6 +2140,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;
>> +
> 
> [Severity: Medium]
> Should the software Tx timestamp tracker be reconciled here?
> 
> ice_ptp_phy_soft_reset_eth56g() calls
> ice_ptp_clear_tx_memory_status_eth56g(), whose kernel-doc says:
> 
>   * for the port (two 32-bit reads each). This discards all timestamp data on the port,
>   * so it must only be used for a full reset; callers that must preserve
>   * in-flight timestamps clear individual indices via ice_clear_phy_tstamp().
> 
> This now runs on an ordinary link-up path:
> 
>   ice_ptp_link_change(true)
>     -> ice_ptp_port_phy_restart()
>       -> case ICE_MAC_GENERIC_3K_E825: ice_start_phy_timer_eth56g()
>         -> ice_ptp_phy_soft_reset_eth56g()
> 
> The ICE_MAC_GENERIC branch of ice_ptp_port_phy_restart() sets
> ptp_port->tx.calibrating under tx->lock, and ice_ptp_rebuild_owner() does:
> 
>   /* Flush software tracking of any outstanding timestamps since we're
>    * about to flush the PHY timestamp block.
>    */
>   ice_ptp_flush_all_tx_tracker(pf);
> 
> The ICE_MAC_GENERIC_3K_E825 branch does neither, so indices still set in
> tx->in_use keep their skb references while the hardware timestamp memory
> and ready bits are wiped underneath them. Those entries are then only
> resolved by the 2 second timeout in ice_ptp_process_tx_tstamp(), bumping
> tx_hwtstamp_timeouts. Later patches in the series ("ice: keep Tx timestamp
> slots tracked until completion or timeout" and "ice: don't clear in_use
> until HW clears ready bitmap") make the periodic work reap those slots, so
> the effect is bounded rather than permanent, but should the E825 branch
> still gate or flush the tracker around the reset?
> 

Hmm. My goal had been to make the E825 only ever release an index from
the Tx timestamp thread. Its quite difficult to figure out what the PHY
actually does in various cases and the consequences of blocking a
timestamp for 2 seconds seemed safer than potentially allowing some gap
that results in blocking all interrupts indefinitely.

> [Severity: Medium]
> Can this leave the PHY parked in soft reset?
> 
> ice_ptp_phy_soft_reset_eth56g() asserts the bit and then has several
> error returns before the final de-assert:
> 
>   global_val |= PHY_REG_GLOBAL_SOFT_RESET_M;
>   err = ice_write_ptp_reg_eth56g(hw, port, PHY_REG_GLOBAL, global_val);
>   if (err) {
>           ice_debug(...);
>           return err;
>   }
>   usleep_range(5000, 6000);
> 
>   global_val &= ~PHY_REG_GLOBAL_SOFT_RESET_M;
>   ...
> 
> If a sideband access fails after the assert (queue full, timeout, or a
> concurrent device reset returning -EBUSY/-EIO/-ETIMEDOUT), the new early
> return here skips ice_stop_phy_timer_eth56g(),
> ice_phy_cfg_parpcs_eth56g(), ice_phy_cfg_ptp_1step_eth56g(),
> ice_phy_cfg_mac_eth56g(), the incval programming, the timer sync and the
> TX/RX_OFFSET_READY writes.
> 
> ice_ptp_port_phy_restart() only logs the failure:
> 
>   if (err)
>           dev_err(ice_pf_to_dev(pf), "PTP failed to set PHY port %d up, err %d\n",
>                   port, err);
> 
>   mutex_unlock(&ptp_port->ps_lock);
> 
> There is no ov_work retry for ICE_MAC_GENERIC_3K_E825, and
> ice_ptp_link_change() only calls the restart on a link-up transition, so
> the port stays with SOFT_RESET asserted and stale offset-ready bits until
> the next link-up or a rebuild. Would it be better to de-assert the bit on
> the error paths inside ice_ptp_phy_soft_reset_eth56g()?
> 

Maybe? I guess the issue is that if we fail this we don't really know
what the hardware state is in. I think the only real recovery here is to
reset or link toggle. Even if we de-asert the soft reset bit if we don't
get a clean soft reset we can't reliably guarantee the timestamps and
interrupt will work... I guess it may make sense to deassert the bit but
still report error so we don't re-enable timestamping until we get a
clean link reset.

> [Severity: Medium]
> What serialises this reset against the Tx timestamp reader on the other PF?
> 
> The reset holds the block in reset across two settling windows:
> 
>   usleep_range(5000, 6000);
> 
>   global_val |= PHY_REG_GLOBAL_SOFT_RESET_M;
>   ...
>   usleep_range(5000, 6000);
> 
> The writer side holds only its own ptp_port->ps_lock:
> 
>   ice_ptp_link_change() -> ice_ptp_port_phy_restart() -> ice_start_phy_timer_eth56g()
> 
> The reader side runs on the source-timer-owning PF:
> 
>   ice_ptp_process_ts() -> ice_ptp_tx_tstamp_owner() -> ice_ptp_process_tx_tstamp()
>       ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready);
>       ...
>       ice_read_phy_tstamp(hw, tx->block, phy_idx, &raw_tstamp);
> 
> and for E825 that PF processes all ports, including the peer PF's port:
> 
>   if (ice_pf_src_tmr_owned(pf))
>           pf->ptp.tx_interrupt_mode = ICE_PTP_TX_INTERRUPT_ALL;
> 
> ice_ptp_process_tx_tstamp() takes only tx->lock around the bitmap/skb
> update, never ps_lock, and the E825 branch of ice_ptp_port_phy_restart()
> does not set tx.calibrating. So can ice_read_phy_tstamp() land on a PHY
> PTP block that another PF is holding mid-reset, and if such a read returns
> a word with ICE_PTP_TS_VALID set, would it be extended with the cached PHC
> and reported to userspace as a real hardware timestamp?
> 

I think with the suggestion to block using calibrating in one of the
other comments will fix this.

> [Severity: Medium]
> Does PHY_REG_TS_INT_CONFIG survive this reset?
> 
> Everything else in the per-lane PTP block is reprogrammed below this point
> (PAR/PCS, 1-step, MAC/XIF, TIMETUS incval, timer sync, TX/RX_OFFSET_READY),
> but PHY_REG_TS_INT_CONFIG (Tx timestamp interrupt enable plus threshold, in
> the same per-lane block as PHY_REG_GLOBAL) is not. Its only writer is
> ice_phy_cfg_intr_eth56g() via ice_ptp_cfg_phy_interrupt(), reached only from
> ice_ptp_init_owner() and ice_ptp_rebuild_owner(), and it is a
> read-modify-write, so a value returned to defaults cannot be recovered.
> 
> The existing ordering in ice_ptp_init_phc_e825c() is reset first:
> 
>   /* Soft reset all ports, to ensure everything is at a clean state */
>   for (int port = 0; port < hw->ptp.num_lports; port++) {
>           err = ice_ptp_phy_soft_reset_eth56g(hw, port);
> 
> whereas ice_ptp_rebuild_owner() now configures the interrupt and then
> resets every port:
> 
>   err = ice_ptp_cfg_phy_interrupt(pf, true, 1);
>   if (err)
>           return err;
> 
>   ice_ptp_restart_all_phy(pf);
> 
> Given the commit message says the reset "ensures that the PHY internal
> state has been completely reset", could this disarm the port's Tx timestamp
> threshold interrupt after the first link-up while software still believes
> it is enabled? The software fallback ice_ptp_maybe_trigger_tx_interrupt()
> only runs on the source-timer-owning PF. Commit 3ec46e157c7f describes the
> reset's effects without mentioning configuration registers, so this may
> well be fine; could the commit message state whether
> PHY_REG_TS_INT_CONFIG is preserved, or should
> ice_phy_cfg_intr_eth56g() be re-applied after the reset?
> 

I'll clarify this in the commit message. Experimentally we confirmed
that PHY_REG_TS_INT_CONFIG doesn't get reset by the PHY soft reset. I
will try my best to explain my understanding (I really do not like this
PHY and how limited my access to information on it is).

As far as I understand, the soft reset affects the PHY timers (resetting
them to zero, resetting the increment to 0), and the internal timestamp
counter. Experimentally I have proven it does not clear this
PHY_REG_TS_INT_CONFIG. I'll try to address this in the commit message.

>>  	err = ice_stop_phy_timer_eth56g(hw, port, false);
> 
> [Severity: Low]
> This is a pre-existing issue, not introduced by this patch, but the new
> juxtaposition makes it more visible. ice_stop_phy_timer_eth56g() documents
> a parameter and an action it does not implement:
> 
>   * @soft_reset: if true, hold the SOFT_RESET bit of PHY_REG_PS
>   *
>   * Stop the clock of a PHY port. ...
>   int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port, bool soft_reset)
>   {
>           err = ice_write_ptp_reg_eth56g(hw, port, PHY_REG_RX_OFFSET_READY, 0);
> 
> soft_reset is never read, so ice_ptp_port_phy_stop() passing true gets no
> soft reset:
> 
>   case ICE_MAC_GENERIC_3K_E825:
>           err = ice_stop_phy_timer_eth56g(hw, port, true);
> 
> Now that the soft reset is done explicitly right before a call that passes
> false, could the parameter be dropped and the summary line corrected?
> 

Yes, that will be fixed in one of the other comments on the series.

>>  	if (err)
>>  		return err;