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

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

On 10/12/2026 4:15 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 3 (4) · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] The new non-READY (ERROR/RESETTING) branch of ice_ptp_release()
>   destroys the worker too early.
> - [Medium] The patch removes ice_ptp_release_tx_tracker() from
>   ice_ptp_prepare_for_reset() and puts nothing in its place.
> - [Medium] At this commit, when ice_ptp_init_work() fails, the code now
>   jumps to err_clean_pf.
> - [Medium] Now that the Tx tracker survives CORE/GLOBAL/EMP resets, its
>   per-index tstamps[idx].cached_tstamp and last_ll_ts_idx_read are never…
> 
> Pre-existing issues:
> - [Medium] The non-READY branch of ice_ptp_release() tears everything
>   down: kworker, clock, list_del via ice_ptp_cleanup_pf(), the Tx…
> - [Medium] At this commit, ice_ptp_flush_all_tx_tracker(),
>   ice_ptp_restart_all_phy() and ice_ptp_prepare_rebuild_sec() walk…
> - [Medium] On E825, ice_ptp_setup_pf() sets this port's bit in the
>   control PF's shared tx_refclks[phy][tx_clk] map.
> - [Medium] Neither the ice_ptp_init() error unwind (after
>   ice_ptp_reset_phy_timestamping() and ice_ptp_cfg_tx_interrupt()) nor…
> 
> --
> 
> ice: fix removal of PTP timestamp tracker during reset
> 
> This commit stops releasing the PTP Tx timestamp tracker in
> ice_ptp_prepare_for_reset(), so timestamping can resume after CORE, GLOBAL
> or EMP resets. It reorders ice_ptp_init() so the tracker is allocated
> before the port is added to the port list, and the tracker is released on
> error. ice_ptp_release() now cancels the kworker and releases the tracker
> when PTP is not in the READY state.
> 
> Closes: https://sashiko.dev/#/patchset/20260821-jk-e825c-minimized-fixes-v1-0-9d0731eb4858%40intel.com?part=8
> Closes: https://lore.kernel.org/netdev/20260916011213.1632286-1-kuba@kernel.org/
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index eaec36ab6ae3f..fe21cee4f9dee 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> @@ -2939,8 +2939,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: Medium]
> 
> With this call removed, nothing clears tx->init or calls synchronize_irq()
> before a CORE, GLOBAL or EMP reset. Is that intended?
> 

Yes, this was intended.

> The Tx timestamp paths check only tx->init and tx->calibrating. They never
> check ptp->state. Suppose ice_ptp_rebuild() fails:
> 
> ice_ptp_rebuild()
>     err:
>         ptp->state = ICE_PTP_ERROR;
> 
> ice_rebuild() still calls ice_ptp_restore_timestamp_mode()
> unconditionally, and that re-enables the Tx timestamp interrupt. After
> that, ice_tstamp()->ice_ptp_request_ts() only checks:
> 
>     if (!ice_ptp_is_tx_tracker_up(tx)) {
> 
> So slots are still handed out (skb_get() plus SKBTX_IN_PROGRESS) against a
> PHC that isn't running. In the ERROR state cached_phc_jiffies is never
> refreshed, so ice_ptp_extend_40b_ts() would discard the result. Before
> this patch, these requests were refused and counted in
> tx_hwtstamp_skipped.
> 
> During the reset window itself, ice_ll_ts_intr() only checks tx->init:
> 
>     if (tx->init) {
>         ice_ptp_complete_tx_single_tstamp(tx);
> 
> The E810 path in ice_ptp_ts_irq() also checks ice_pf_state_is_nominal().
> Can the LL proxy registers now be accessed in the middle of a reset?
> 

I am not certain any of this actually results in a real issue. We could
add a check to prevent new timestamp requests while the state is
non-ready I suppose.. But I don't know if anything here actually fails.
The messaging could be improved. In practice I am unsure if the driver
actually *can* timestamp here since it would have stopped the traffic
queues during queisce before reset.

> The commit message doesn't mention this change in behaviour. Later patches
> in the series only add link_up and calibrating gating, so tx->init still
> stays set across the reset.
> 

Yes, tx->init stays valid, because we need to keep the tracker and
removing it was problematic. Checking the reset state may make sense but
I am not convinced this is a real visible problem.

> [Severity: Medium]
> 
> The tracker now survives the reset, so tstamps[idx].cached_tstamp and
> last_ll_ts_idx_read are no longer reset. The E810 PHY timestamp memory is
> still reset. Can the two get out of sync?
> 
> On rebuild, ice_ptp_rebuild_owner()->ice_ptp_flush_all_tx_tracker()->
> ice_ptp_flush_tx_tracker() calls ice_clear_phy_tstamp() for every in_use
> index, because tstamp_ready is all ones on E810.
> ice_clear_phy_tstamp_e810() then writes 0 to both halves:
> 
>     err = ice_write_phy_reg_e810(hw, lo_addr, 0);
>     ...
>     err = ice_write_phy_reg_e810(hw, hi_addr, 0);
> 
> E810 has no ready bitmap. It detects a new capture by checking raw_tstamp
> != cached_tstamp. After the reset, a reused index that hasn't captured yet
> reads 0, and 0 differs from the stale cached value.
> 
> In ice_ptp_process_tx_tstamp(), the missing VALID bit makes it drop the
> timestamp and release the slot. Because raw is 0, cached_tstamp isn't
> updated, so the later real capture could be credited to the next request
> that uses that index.
> 

This looks like a preexisting issue that we should clear the cached
value when writing the PHY registers for E810. This should be fixed, but
IMO should not block this series.

> ice_ptp_complete_tx_single_tstamp() has no VALID check:
> 
>     if (raw_tstamp == tx->tstamps[idx].cached_tstamp)
>         return;
> 
>     tx->tstamps[idx].cached_tstamp = raw_tstamp;
>     clear_bit(idx, tx->in_use);
> 
> It would then pass ice_ptp_extend_40b_ts(pf, 0) to skb_tstamp_tx().
> 

This also looks like a preexisting issue in the low latency path that
should have been checking valid bits.

> Before 7a25fe5cd5fb the tracker was reallocated with zeroed cached values,
> so software and hardware stayed in sync. This is still present at the end
> of the series.


Before this series, timestamping stopped permanently after a device reset.

> 
>>  	/* Disable periodic outputs */
>>  	ice_ptp_disable_all_perout(pf);
>>  
>> @@ -3347,13 +3345,13 @@ void ice_ptp_init(struct ice_pf *pf)
>>  		}
>>  	}
>>  
>> -	err = ice_ptp_setup_pf(pf);
>> +	err = ice_ptp_init_port(pf, &ptp->port);
>>  	if (err)
>> -		goto err_exit;
>> +		goto err_destroy_ps_lock;
>>  
>> -	err = ice_ptp_init_port(pf, &ptp->port);
>> +	err = ice_ptp_setup_pf(pf);
>>  	if (err)
>> -		goto err_clean_pf;
>> +		goto err_release_tx_tracker;
>>  
>>  	/* Start the PHY timestamping block */
>>  	ice_ptp_reset_phy_timestamping(pf);
> 
> [Severity: Medium]
> 
> This isn't a bug introduced by this patch, but neither the ice_ptp_init()
> error unwind nor the non-READY branch of ice_ptp_release() puts the
> hardware back into a quiet state.
> 

True. These are *error cleanup* flows which are just trying to do their
best to cleanup and avoid leaks.

> ice_ptp_reset_phy_timestamping() and ice_ptp_cfg_tx_interrupt() run before
> ice_ptp_init_work(). If ice_ptp_init_work() then fails, the unwind never
> calls ice_ptp_disable_timestamp_mode(), ice_ptp_port_phy_stop() or the
> perout/extts disables.
> 
> Can OICR TSYN_TX, PHY timestamping and the SDP outputs stay armed after the
> software state has been torn down?

Presumably yes if you fail at exactly the right way. But the software
really has no way to know which parts would need to be torn down, and
whether it even has access. Should we balloon the error handling logic
to try and detect and cleanup every possible state here? I don't think
thats helpful.

> 
> [ ... ]
> 
>> @@ -3365,14 +3363,17 @@ void ice_ptp_init(struct ice_pf *pf)
>>  
>>  	err = ice_ptp_init_work(pf, ptp);
>>  	if (err)
>> -		goto err_exit;
>> +		goto err_clean_pf;
> 
> [Severity: Medium]
> 
> When ice_ptp_init_work() fails, this path now frees the Tx tracker and
> destroys ps_lock. However, ptp->state was set just above:
> 
>     ptp->state = ICE_PTP_READY;
> 
>     err = ice_ptp_init_work(pf, ptp);
>     if (err)
>         goto err_clean_pf;
> 
> The PHC is also still registered at this point. Could ice_ptp_link_change(),
> ice_ptp_queue_work() or the PHC callbacks touch the freed tracker or the
> destroyed mutex in this window? One example is
> ice_ptp_reset_cached_phctime()->ice_ptp_mark_tx_tracker_stale().
> 
> The NULL kworker exposure while READY was already there before. Later in
> the series, ice_ptp_init() creates the kworker earlier and sets
> ICE_PTP_READY only after ice_ptp_init_work() succeeds, which appears to
> close this. At this commit the window is still open.
> 

This has been incredibly frustrating, as every time I change the order
here I end up with some other complaint about teardown order :\

We should be cleaning up the PTP clock first, but this is complicated by
the fact that each PF unloads independently and only the clock owner PF
actually has a clock. This outline does imply that the paths could
access the kworker while the port is still in the list, so ya I guess
this really should be afterwards.

The worker thread does appear to check the PTP state so I guess
reordering it would be safe.

>>  
>>  	dev_info(ice_pf_to_dev(pf), "PTP init successful\n");
>>  	return;
>>  
>>  err_clean_pf:
>> -	mutex_destroy(&ptp->port.ps_lock);
>>  	ice_ptp_cleanup_pf(pf);
> 
> [Severity: Medium]
> 
> This is a pre-existing issue, but on E825 ice_ptp_setup_pf() sets this
> port's bit in the control PF's shared map:
> 
>     set_bit(port_num,
>             &ctrl_ptp->tx_refclks[phy][pf->ptp.port.tx_clk]);
> 
> ice_ptp_cleanup_pf() only does list_del() and never clears that bit. After
> this error path, or after an unbind, would ice_txclk_any_port_uses() still
> count this port as a user of the reference clock?
> 

This looks like an issue for the developer of the refclk code to fix.

>> +err_release_tx_tracker:
>> +	ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
>> +err_destroy_ps_lock:
>> +	mutex_destroy(&ptp->port.ps_lock);
>>  err_exit:
> 
> [ ... ]
> 
>> @@ -3399,12 +3400,20 @@ void ice_ptp_release(struct ice_pf *pf)
>>  		return;
>>  
>>  	if (pf->ptp.state != ICE_PTP_READY) {
>> -		mutex_destroy(&pf->ptp.port.ps_lock);
>> -		ice_ptp_cleanup_pf(pf);
>> +		if (pf->ptp.kworker) {
>> +			kthread_cancel_delayed_work_sync(&pf->ptp.work);
>> +			if (pf->hw.mac_type == ICE_MAC_GENERIC)
>> +				kthread_cancel_delayed_work_sync(&pf->ptp.port.ov_work);
>> +			kthread_destroy_worker(pf->ptp.kworker);
>> +			pf->ptp.kworker = NULL;
>> +		}
> 
> [Severity: High]
> 
> The worker is destroyed here and pf->ptp.kworker is set to NULL. The PHC is
> only unregistered further down, and the port stays on
> adapter->ports.ports until ice_ptp_cleanup_pf() runs. Can anything still
> reach this worker in between?
> 
> On ICE_MAC_GENERIC, ice_ptp_settime64() calls ice_ptp_restart_all_phy()
> without checking ptp->state. ice_ptp_adjtime() does the same through its
> non-atomic path. ice_ptp_restart_all_phy() walks the list without
> ports.lock:
> 
>     list_for_each(entry, &pf->adapter->ports.ports) {
>         ...
>         if (port->link_up)
>             ice_ptp_port_phy_restart(port);
>     }
> 
> ice_ptp_port_phy_restart() then does:
> 
>     case ICE_MAC_GENERIC:
>         kthread_cancel_delayed_work_sync(&ptp_port->ov_work);
>         ...
>         kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work,
>                                    0);
> 
> Here pf is the releasing PF. That covers the releasing PF's own port, and
> also its port when it is a peer and the owner PF is the one calling
> settime.
> 
> ice_ptp_reset_cached_phctime() also queues on pf->ptp.kworker when
> ICE_CFG_BUSY is set, with no NULL or state check.
> 
> Before this patch the non-READY branch leaked the worker rather than
> freeing it, so this NULL dereference or use-after-free of the
> kthread_worker looks new. It can be hit if a PF in ERROR or RESETTING
> state is unbound while userspace calls clock_settime() on the owner PHC.
> 
> Could the clock be unregistered and the port removed from the list before
> the worker is destroyed? The commit message already describes that
> ordering for ps_lock. The ordering here is unchanged at the end of the
> series.

Yea, I guess we need to do the cleanup first and then teardown the
kworkers.. This has been quite frustrating because I believe I got other
complaints about order when trying to do that way before :\ Hopefully we
can find something that works...

> 
>>  		if (pf->ptp.clock) {
>>  			ptp_clock_unregister(pf->ptp.clock);
>>  			pf->ptp.clock = NULL;
>>  		}
>> +		ice_ptp_cleanup_pf(pf);
>> +		ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
> 
> [Severity: Medium]
> 
> This is a pre-existing issue, but three functions walk
> adapter->ports.ports without ports.lock: ice_ptp_flush_all_tx_tracker(),
> ice_ptp_restart_all_phy() and ice_ptp_prepare_rebuild_sec(). For example:
> 
>     list_for_each_entry(port, &pf->adapter->ports.ports, list_node)
>         ice_ptp_flush_tx_tracker(ptp_port_to_pf(port), &port->tx);
> 
> Meanwhile ice_ptp_cleanup_pf() unlinks the port under the lock, and
> ice_ptp_release_tx_tracker() then frees the tracker. Trackers now stay
> allocated across CORE/GLOBAL resets, so the owner's rebuild-time flush
> works on live peer trackers. Can one of these walkers race with this
> teardown?
> 
> A later patch in this series, "ice: use reference counting and SRCU for
> PTP port access", converts these walkers to SRCU with kref and appears to
> close this race.

Yes. I tried to have that patch first but it got far too many complaints
about *this* issue, so I re-ordered the series.

> 
>> +		mutex_destroy(&pf->ptp.port.ps_lock);
>>  		return;
>>  	}
> 
> [Severity: Medium]
> 
> This isn't a bug introduced by this patch, but this branch tears
> everything down and returns without setting pf->ptp.state to
> ICE_PTP_UNINIT. If ice_ptp_release() runs again before ice_ptp_init()
> resets the state, would ice_ptp_cleanup_pf() call list_del() on a node
> that already holds LIST_POISON pointers?
> 

Another on the pile of possible pre-existing issues... :D

> One possible sequence:
> 
> 1. devlink reinit_down runs while PTP is in ERROR or RESETTING, which goes
>    through ice_unload()->ice_ptp_release().
> 2. reinit_up fails before ice_load().
> 3. ice_remove()->ice_unload()->ice_deinit_features()->ice_ptp_release()
>    runs again.
> 
> The old branch had the same list_del() without a state change, and the end
> of the series still doesn't set ICE_PTP_UNINIT here.
> 

Problem is that I am not sure if setting to UNINIT is totally correct
here :\ Of course the original design/logic here was that this is part
of driver remove and not something that would be re-triggered such as
via devlink.