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

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

On 10/25/2026 4:43 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 (1) · 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).
> 
> - [Medium] The new error unwind in ice_ptp_init() destroys the PTP
>   kworker while the owner's PTP clock is still registered.
> - [Medium] On E810 (has_ready_bitmap == 0), the Tx tracker now survives
>   CORE/GLOBAL/EMP resets.
> 
> Pre-existing issues:
> - [Medium] Concern: ice_ptp_flush_all_tx_tracker(),
>   ice_ptp_restart_all_phy() and ice_ptp_prepare_rebuild_sec() walk…
> 
> --
> 
> ice: fix removal of PTP timestamp tracker during reset
> 
> This patch stops ice_ptp_prepare_for_reset() from releasing the Tx
> timestamp tracker, so timestamping works again after CORE, GLOBAL or EMP
> resets. It also reorders ice_ptp_init() so the kworker and tracker are set
> up before the port is added to the port list, with a matching error
> unwind. The non-READY branch of ice_ptp_release() now frees the tracker
> and kworker.
> 
> 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 eaec36ab6ae3..e1c66cd0f94f 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);
>> -
>>  	/* Disable periodic outputs */
>>  	ice_ptp_disable_all_perout(pf);
> 
> [Severity: Medium]
> The tracker now survives CORE/GLOBAL/EMP resets. On E810, can the
> cached_tstamp values then drift from the PHY state after the post-reset
> flush?
> 

This is a pre-existing issue that should be resolved separately, as
discussed on previous versions of this series.

> [Severity: Medium]
> Is the PTP clock still registered while the kworker is torn down here?
> 
> On the clock owner, ice_ptp_init_owner() has already registered the
> clock. /dev/ptpN stays live until err_exit calls ptp_clock_unregister().
> 
> The settime64 and adjtime ops do not check ptp->state, and they call
> ice_ptp_reset_cached_phctime(). Suppose ice_ptp_update_cached_phctime()
> returns -EAGAIN because ICE_CFG_BUSY is held, for example by a concurrent
> adjtime on the same PHC. That function then does:
> 
> 	kthread_queue_delayed_work(pf->ptp.kworker, &pf->ptp.work,
> 				   msecs_to_jiffies(10));
> 
> If this runs after kthread_cancel_delayed_work_sync() but before or during
> kthread_destroy_worker(), a timer is left pending. Could
> kthread_delayed_work_timer_fn() later touch the freed worker?
> 
> If it runs after ptp->kworker = NULL, would it dereference NULL?
> 
> One way to reach this window: ice_ptp_init_port() fails with -ENOMEM from
> ice_ptp_alloc_tx_tracker() while a process with /dev/ptpN open calls
> clock_settime() or clock_adjtime().
> 
> In this same patch, the non-READY branch of ice_ptp_release() unregisters
> the clock before tearing down the worker. Should the init error path use
> the same order? This ordering is still present at the end of the series.
> 

This is actually a problem that I think needs to be addressed
separately, and work from other developers around the lifetime of
adapter->ctrl_pf. I've tried multiple different ordering schemes while
trying to get this series to pass Sashiko review, and every time I get
something else as a problem.

The current behavior is a problem, and the ways I've tried to fix it are
less bad problems, (but still problems). I am getting extremely
frustrated by this entire process.

>> @@ -3399,12 +3405,22 @@ 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.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 and was not introduced by this patch.
> ice_ptp_flush_all_tx_tracker(), ice_ptp_restart_all_phy() and
> ice_ptp_prepare_rebuild_sec() walk the adapter port list without holding
> ports.lock:
> 
> 	list_for_each_entry(port, &pf->adapter->ports.ports, list_node)
> 		ice_ptp_flush_tx_tracker(ptp_port_to_pf(port), &port->tx);
> 
> At the same time, a peer PF's ice_ptp_release() can remove its port from
> the list and free the tracker, kworker and ps_lock. Since trackers are now
> kept across resets, the owner's flush walks memory that belongs to live
> peers.
> 
> The next patch in the series, "ice: use reference counting and SRCU for
> PTP port access", fixes this. It converts these walkers to
> srcu_read_lock() with list_for_each_entry_srcu() and
> kref_get_unless_zero(). It also makes ice_ptp_cleanup_pf() wait for the
> references and SRCU readers to finish before the tracker and worker are
> freed. The race is gone by the end of the series.
> 

Yes. I tried different orderings of the series, but putting the RCU
fixes first  seemed to have it complain about problems fixed by this
series. etc. etc.