Re: [PATCH net 02/15] ice: fix removal of PTP timestamp tracker during reset

[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: fix removal of PTP timestamp tracker during reset
> 
> This drops the ice_ptp_release_tx_tracker() call from
> ice_ptp_prepare_for_reset(), completing the intent of 7a25fe5cd5fb so the
> Tx timestamp tracker is no longer destroyed (and never re-created) across a
> CORE or GLOBAL reset.
> 
> Closes: https://sashiko.dev/#/patchset/20260821-jk-e825c-minimized-fixes-v1-0-9d0731eb4858%40intel.com?part=8
> 
>>     Unfortunately, the commit only removed the calls to ice_ptp_init_tx_*().
>>     This fixed a memory leak in PF reset. However, now a CORE or GLOBAL reset
>>     will leave the device unable to initiate Tx timestamp requests
>>     indefinitely.
> 
> [Severity: Low]
> Is the list of affected reset types complete here?  ice_ptp_prepare_for_reset()
> returns early only for PFR:
> 
> 	if (reset_type == ICE_RESET_PFR)
> 		return;
> 
> 	...
> 	ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
> 
> so the deleted call also ran for ICE_RESET_EMPR.  ice_reset_subtask() maps
> ICE_EMPR_RECV to ICE_RESET_EMPR and then calls ice_prepare_for_reset(), and
> the matching ice_rebuild() -> ice_ptp_rebuild() has no ice_ptp_init_tx_*()
> call either.  Should the commit message mention EMP resets as well, since
> that affects stable triage?
> 

Sure we can update the message.

>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index b12181b8c8434..142d39ee5cc52 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> @@ -2979,8 +2979,6 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
>>  	if (ice_pf_src_tmr_owned(pf) && hw->mac_type == ICE_MAC_GENERIC_3K_E825)
>>  		ice_ptp_prepare_rebuild_sec(pf, false, reset_type);
>>  
>> -	ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
>> -
> 
> [Severity: High]
> This call was also the only thing that cleared tx->init under tx->lock and
> ran synchronize_irq() for the low latency Tx timestamp path.  With the
> tracker left armed, can ice_ll_ts_intr() still run against live tracker
> state in the middle of a CORE or GLOBAL reset?
> 
> The handler is gated only on tx->init:
> 
> drivers/net/ethernet/intel/ice/ice_main.c:ice_ll_ts_intr() {
> 	spin_lock_irqsave(&tx->lock, flags);
> 	if (tx->init) {
> 		ice_ptp_complete_tx_single_tstamp(tx);
> 
> 		idx = find_next_bit_wrap(tx->in_use, tx->len,
> 					 tx->last_ll_ts_idx_read + 1);
> 		if (idx != tx->len)
> 			ice_ptp_req_tx_single_tstamp(tx, idx);
> 	}
> 	...
> }
> 
> The LL cause lives in PFINT_SB_CTL, so ice_ptp_disable_timestamp_mode() does
> not mask it (it only clears PFINT_OICR_ENA TSYN_TX), and
> ice_prepare_for_reset() only does synchronize_irq(pf->oicr_irq.virq), not
> pf->ll_ts_irq.
> 
> Two consequences worth checking:
> 
> ice_ptp_complete_tx_single_tstamp() reads REG_LL_PROXY_L/H of a resetting
> device, so can a garbage timestamp be reported through skb_tstamp_tx() for a
> still-set in_use slot?
> 
> The handler sees the pre-reset in_use bits and issues a new firmware request
> via ice_ptp_req_tx_single_tstamp(), which sets:
> 
> 	params->atqbal_flags |= ATQBAL_FLAGS_INTR_IN_PROGRESS;
> 
> That flag is cleared only by ice_ptp_complete_tx_single_tstamp(), i.e. by a
> later LL interrupt, and nothing re-initializes it on rebuild.  If the reset
> swallows the request, does every subsequent user of the LL interface block
> forever in the untimed wait in ice_ptp_prep_phy_incval_ll_e810() /
> ice_ptp_prep_phy_adj_ll_e810():
> 
> 	err = wait_event_interruptible_locked_irq(params->atqbal_wq,
> 						  !(params->atqbal_flags &
> 						    ATQBAL_FLAGS_INTR_IN_PROGRESS));
> 
> These are reached from ice_ptp_rebuild_owner() through
> ice_ptp_write_incval()/ice_ptp_write_init(), which run in the PF service task
> workqueue where no signal can break the wait.
> 
> Note ice_ptp_ts_irq() checks ice_pf_state_is_nominal() and
> ice_misc_intr_thread_fn() checks ice_is_reset_in_progress(), while
> ice_ll_ts_intr() has no reset-state check.  Would keeping the tracker across
> reset need an explicit disarm here (a state check honored by
> ice_ll_ts_intr() plus synchronize_irq(pf->ll_ts_irq.virq)) rather than just
> dropping the release?
> 

Hm. Will investigate.

> [Severity: Medium]
> Can the tracker allocations now leak at unload when the PTP state is not
> ICE_PTP_READY?
> 
> tx->tstamps, tx->in_use and tx->stale are allocated in
> ice_ptp_alloc_tx_tracker(), and after this change the only remaining free is
> in the READY branch of ice_ptp_release():
> 
> 	if (pf->ptp.state != ICE_PTP_READY) {
> 		mutex_destroy(&pf->ptp.port.ps_lock);
> 		ice_ptp_cleanup_pf(pf);
> 		if (pf->ptp.clock) {
> 			ptp_clock_unregister(pf->ptp.clock);
> 			pf->ptp.clock = NULL;
> 		}
> 		return;
> 	}
> 	...
> 	ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
> 
> ice_ptp_cleanup_pf() only unlinks the port from the adapter list and drops
> the kref, so it does not touch the tracker.
> 
> If ice_ptp_rebuild_owner() fails, ice_ptp_rebuild() leaves the state at
> ICE_PTP_ERROR:
> 
> err:
> 	ptp->state = ICE_PTP_ERROR;
> 	dev_err(ice_pf_to_dev(pf), "PTP reset failed %d\n", err);
> 
> and a failed ice_check_reset() leaves it at ICE_PTP_RESETTING.  A later
> rmmod or unbind then takes the early-return branch above.  Does that leak the
> timestamp array, both bitmaps, and any SKBs still pinned in tx->tstamps[]
> along with their socket references?
> 
> [Severity: Medium]
> With this call gone, the only reset-time flush left is
> ice_ptp_flush_all_tx_tracker() inside ice_ptp_rebuild_owner().  What reclaims
> the tracker if the reset never reaches ice_rebuild()?
> 
> drivers/net/ethernet/intel/ice/ice_main.c:ice_reset_subtask() {
> 		if (ice_check_reset(&pf->hw)) {
> 			set_bit(ICE_RESET_FAILED, pf->state);
> 		} else {
> 			...
> 			ice_rebuild(pf, reset_type);
> 	...
> }
> 
> In that path ice_ptp_prepare_for_reset() has already run, so the pre-reset
> tx->in_use bits stay set with skb_get() references pinned in tx->tstamps[]
> and SKBTX_IN_PROGRESS still set, while the PHY timestamps were destroyed by
> the reset.
> 
> The 2 second timeout drain cannot run either:
> 
> 	if (pf->ptp.state != ICE_PTP_READY)
> 		return;
> 
> in ice_ptp_periodic_work(), and the work was cancelled in
> ice_ptp_prepare_for_reset().  The OICR TSYN_TX cause was masked by
> ice_ptp_disable_timestamp_mode() and is only re-enabled by
> ice_ptp_restore_timestamp_mode() at the end of a completed ice_rebuild().
> Combined with the non-READY branch of ice_ptp_release() above, are those SKBs
> and their socket references then held for the remaining lifetime of the
> driver instance?
> 
> For contrast, the narrower case of a *PTP* rebuild failure does still drain:
> ice_ptp_rebuild() returns void, ice_rebuild() continues and calls
> ice_ptp_restore_timestamp_mode(), so the stale entries age out via the 2 s
> timeout.
> 
> [Severity: Medium]
> After a failed CORE or GLOBAL PTP rebuild, does the retained tracker keep
> accepting requests that can never complete?
> 
> tx->init now stays 1, and admission in ice_ptp_request_ts() consults only the
> tracker:
> 
> 	if (!ice_ptp_is_tx_tracker_up(tx)) {
> 		spin_unlock_irqrestore(&tx->lock, flags);
> 		return -1;
> 	}
> 
> ice_ptp_is_tx_tracker_up() tests tx->init && !tx->calibrating and never looks
> at pf->ptp.state.  Meanwhile ice_ptp_rebuild() has set ICE_PTP_ERROR and
> returned void, so ice_rebuild() reattaches the netdev and calls
> ice_ptp_restore_timestamp_mode(), re-enabling the Tx timestamp cause, while
> the PHY was never restarted (ice_ptp_restart_all_phy() sits after the failure
> point) and:
> 
> 	if (test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags) &&
> 	    pf->ptp.state == ICE_PTP_READY)
> 		kthread_queue_delayed_work(pf->ptp.kworker, &pf->ptp.work, 0);
> 
> in ice_ptp_queue_work() refuses to restart periodic processing.  Before this
> patch tx->init had been cleared, so this error path rejected requests.  Would
> each timestamp-requesting SKB now be held until the 2 second timeout drops it
> and bumps tx_hwtstamp_timeouts?
> 

We should probably check the state and reject requests when we're not ready.

> [Severity: Medium]
> Since peer PFs' trackers now survive CORER/GLOBR, can the clock owner's
> rebuild flush race the peers?
> 
> ice_ptp_rebuild_owner() -> ice_ptp_flush_all_tx_tracker() walks every port of
> the adapter:
> 
> 		ice_ptp_flush_tx_tracker(ptp_port_to_pf(port), &port->tx);
> 
> and ice_ptp_flush_tx_tracker() does its PHY register work outside tx->lock:
> 
> 	err = ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready);
> 	...
> 		if (!hw->reset_ongoing && (tstamp_ready & BIT_ULL(phy_idx)))
> 			ice_clear_phy_tstamp(hw, tx->block, phy_idx);
> 
> On E810/E830 each PF uses ICE_PTP_TX_INTERRUPT_SELF, so a peer PF that has
> already finished its own ice_rebuild() (and therefore
> ice_ptp_restore_timestamp_mode()) can be inside ice_ptp_process_tx_tstamp()
> reading the same block:
> 
> 	if (tx->has_ready_bitmap) {
> 		err = ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready);
> 	...
> 		err = ice_read_phy_tstamp(hw, tx->block, phy_idx, &raw_tstamp);
> 
> Before this patch the peers' trackers had been released (tx->len = 0, in_use
> NULL, tx->init = 0), so the owner's flush iterated nothing and the peers could
> not process.  Can a timestamp be cleared between the peer's ready-bitmap read
> and its register read here, which the comment in ice_ptp_process_tx_tstamp()
> warns can wedge the interrupt generation logic?  Note the teardown path
> (ice_ptp_release_tx_tracker()) clears tx->init and calls synchronize_irq()
> before flushing, but the rebuild flush has no equivalent quiesce for peer
> PFs.

Similar comment from above, we should probably do this same quie