Re: [PATCH net v2 02/15] ice: use reference counting and SRCU for PTP port access

[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 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 1 (2) · 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] In the non-READY branch of ice_ptp_release() (state
>   ICE_PTP_RESETTING or ICE_PTP_ERROR), the PF cancels its work, calls…
> - [Medium] Lost-wakeup hang in ice_ptp_cleanup_pf()
>   (drivers/net/ethernet/intel/ice/ice_ptp.c).
> 
> Pre-existing issues:
> - [Medium] ice_ptp_cleanup_pf() cannot safely run twice, and the non-
>   READY branch of ice_ptp_release() can run it twice.
> - [Medium] ice_ptp_prepare_rebuild_sec() (E825 clock owner, non-PFR
>   reset) calls ice_ptp_prepare_for_reset(peer_pf) while holding only…
> 
> --
> 
> ice: use reference counting and SRCU for PTP port access
> 
> This patch replaces the mutex that protects the ice adapter PTP port list
> with an SRCU-protected list and adds a kref to each ice_ptp_port. Readers
> skip ports whose kref has already dropped to zero.
> 
> ice_ptp_cleanup_pf() unlinks the port, drops the initial reference, waits
> for the count to reach zero with wait_var_event(), and then calls
> synchronize_srcu().
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index fe21cee4f9dee..94a66e9d8c05d 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> 
> [ ... ]
> 
>> @@ -673,20 +676,33 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
>>  	pf->ptp.tx_hwtstamp_good += tstamp_good;
>>  }
>>  
>> +static void ice_ptp_release_port_srcu(struct kref *ref)
>> +{
>> +	wake_up_var(ref);
>> +}
> 
> [Severity: Medium]
> Is a full memory barrier needed before this wake_up_var()?
> 
> The kernel-doc for wake_up_var() in kernel/sched/wait_bit.c says:
> 
>   In order for this to function properly there must be a full memory
>   barrier after the variable is updated ... If the variable was updated
>   by a fully ordered operation such as atomic_dec_and_test() then no
>   extra barrier is required. Otherwise smb_mb() is needed.
> 
> kref_put() goes through refcount_dec_and_test(). That is
> atomic_fetch_sub_release() followed by smp_acquire__after_ctrl_dep() on
> success, so it gives release ordering plus acquire on success. It is not
> fully ordered. __wake_up_bit() then checks waitqueue_active() without
> taking the waitqueue lock.
> 
> Can this interleaving happen on a weakly ordered architecture such as
> arm64?
> 
> CPU1 (last reader, e.g. ice_ptp_tx_tstamp_owner)
> kref_put()
>   refcount_dec_and_test()      /* refs 1 -> 0, release only */
>   ice_ptp_release_port_srcu()
>     wake_up_var(ref)
>       waitqueue_active() sees an empty queue
> 
> CPU2 (ice_ptp_cleanup_pf)
> wait_var_event(ref, !kref_read(ref))
>   adds itself to the waitqueue, set_current_state()
>   kref_read(ref) still returns 1
>   schedule()
> 
> If so, the remover sleeps in TASK_UNINTERRUPTIBLE with no timeout.
> var_wake_function() filters on the key, so an unrelated wakeup on the
> same hashed queue would not wake it.
> 
> Would adding smp_mb__after_atomic() before wake_up_var(), or using a
> fully ordered decrement, avoid this?
> 

I guess this would require the memory barrier.. despite the name
refcount_dec_and_test() doesn't provide the necessary barrier and there
is no equivalent kref_put() call, so I suppose a full memory barrier is
required.

:\

>>  static void ice_ptp_tx_tstamp_owner(struct ice_pf *pf)
>>  {
> 
> [ ... ]
> 
>> @@ -1424,16 +1449,22 @@ static void ice_ptp_reset_phy_timestamping(struct ice_pf *pf)
>>   */
>>  static void ice_ptp_restart_all_phy(struct ice_pf *pf)
>>  {
>> -	struct list_head *entry;
>> +	struct ice_port_list *ports = &pf->adapter->ports;
>> +	struct ice_ptp_port *port;
>> +	int srcu_idx;
>>  
>> -	list_for_each(entry, &pf->adapter->ports.ports) {
>> -		struct ice_ptp_port *port = list_entry(entry,
>> -						       struct ice_ptp_port,
>> -						       list_node);
>> +	srcu_idx = srcu_read_lock(&ports->srcu);
>> +	list_for_each_entry_srcu(port, &ports->list, list_node,
>> +				 srcu_read_lock_held(&ports->srcu)) {
>> +		if (!kref_get_unless_zero(&port->ref))
>> +			continue;
>>  
>>  		if (port->link_up)
>>  			ice_ptp_port_phy_restart(port);
> 
> [Severity: High]
> Can this loop reach a port whose PF has already destroyed its kthread
> worker?
> 
> When the state is ICE_PTP_RESETTING or ICE_PTP_ERROR, ice_ptp_release()
> takes its non-READY branch. That branch tears down the worker and the
> clock before the port is removed from the list:
> 

This was pointed out on the previous patch and we need to re-order
teardown to fix this.

> ice_ptp_release() {
>     ...
> 	if (pf->ptp.state != ICE_PTP_READY) {
> 		if (pf->ptp.kworker) {
> 			kthread_cancel_delayed_work_sync(&pf->ptp.work);
> 			...
> 			kthread_destroy_worker(pf->ptp.kworker);
> 			pf->ptp.kworker = NULL;
> 		}
> 		if (pf->ptp.clock) {
> 			ptp_clock_unregister(pf->ptp.clock);
> 			pf->ptp.clock = NULL;
> 		}
> 		ice_ptp_cleanup_pf(pf);
>     ...
> }
> 
> ice_ptp_cleanup_pf() is the only place that unlinks the port, so
> kref_get_unless_zero() here still succeeds until it runs. port->link_up
> can also still be true, because only ice_ptp_link_change() rewrites it,
> and only in the READY state.
> 
> For ICE_MAC_GENERIC, ice_ptp_port_phy_restart() then does:
> 
> 	kthread_cancel_delayed_work_sync(&ptp_port->ov_work);
> 	...
> 	kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work,
> 				   0);
> 
> kthread_destroy_worker() frees the worker but does not clear
> ov_work.work.worker. The cancel would therefore take the lock of a freed
> worker. The queue call would then pass a NULL pf->ptp.kworker.
> 
> The clock owner can hit this through ice_ptp_rebuild_owner(). Because
> ptp_clock_unregister() can sleep, the window is wider than it looks.
> 
> The same window seems to affect ice_ptp_prepare_rebuild_sec(). It calls
> ice_ptp_prepare_for_reset(peer_pf), which runs
> kthread_cancel_delayed_work_sync(&ptp->work) on the peer.
> 
> The READY branch already calls ice_ptp_cleanup_pf() before it cancels
> any work. Should the non-READY branch also call ice_ptp_cleanup_pf()
> before destroying the worker?
> 
> The commit message says the kref "ensures that a PF will have a valid
> lifetime and not be removed until the reference is released". It also
> lists ice_ptp_restart_all_phy() as one of the iterations being fixed.
> 


> 
> [Severity: Medium]
> This is a pre-existing issue, but does anything here serialize the peer's
> ptp->state against the peer's own ice_ptp_release()?
> 
> ice_ptp_prepare_for_reset() checks and sets the peer's state without a
> lock:
> 
> 	if (ptp->state != ICE_PTP_READY)
> 		return;
> 
> 	ptp->state = ICE_PTP_RESETTING;
> 
> 	/* Disable timestamping for both Tx and Rx */
> 	ice_ptp_disable_timestamp_mode(pf);
> 
> 	kthread_cancel_delayed_work_sync(&ptp->work);
> 
> At the same time, ice_ptp_release() on the peer reads this field and
> writes ICE_PTP_UNINIT to it.
> 
> If the owner writes RESETTING after the peer has written UNINIT, the
> peer exits release in a non-READY state. A later ice_ptp_release() would
> then repeat the cleanup.
> 
> In the opposite order, the peer takes the non-READY branch and destroys
> its kworker while the owner is about to cancel work on it. That is the
> same ordering problem described above for ice_ptp_restart_all_phy().
> Calling ice_ptp_cleanup_pf() first in that branch would fix this case
> too.
> 
> The kref and SRCU here protect the port memory and the list linkage. They
> do not protect the PF state machine. The commit message notes that the
> change "focuses solely on the port list".
> 

Yep. This likely needs to be handled as part of the work on the
ctrl_pf/ctrl_ptp.

>> +
>> +		kref_put(&port->ref, ice_ptp_release_port_srcu);
>>  	}
>> +	srcu_read_unlock(&ports->srcu, srcu_idx);
>>  }
> 
> [ ... ]
> 
>> @@ -3110,13 +3157,23 @@ static int ice_ptp_setup_pf(struct ice_pf *pf)
>>  
>>  static void ice_ptp_cleanup_pf(struct ice_pf *pf)
>>  {
> 
> [ ... ]
> 
>> +	spin_lock(&ports->lock);
>> +	list_del_rcu(&ptp->port.list_node);
>> +	spin_unlock(&ports->lock);
>> +
>> +	ref = &ptp->port.ref;
>> +	kref_put(ref, ice_ptp_release_port_srcu);
>> +
>> +	wait_var_event(ref, !kref_read(ref));
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but what happens if
> ice_ptp_cleanup_pf() runs twice for the same PF?
> 
> The non-READY branch of ice_ptp_release() calls ice_ptp_cleanup_pf() and
> returns without setting ptp.state to ICE_PTP_UNINIT. This sequence seems
> to run the cleanup twice:
> 
> ice_devlink_reinit_down()->ice_unload()->ice_deinit_features()
>   ->ice_ptp_release()
>     (state is ICE_PTP_ERROR or ICE_PTP_RESETTING, cleanup #1)
> 
> ice_devlink_reinit_up() fails before ice_init_features()
>     (ice_ptp_init() never resets the state)
> 
> ice_remove()->ice_unload()->...->ice_ptp_release()
>     (cleanup #2)
> 
> Before this patch, the second list_del() on a poisoned node either
> faulted or, with LIST_HARDENED/DEBUG_LIST, warned and was skipped.
> 
> With this patch, the second kref_put() runs on a kref that is already
> zero. refcount_dec_and_test() warns and saturates the counter without
> calling the release function.
> 
> kref_read() then never returns zero. wait_var_event() would block
> uninterruptibly forever while devl_lock is held. On hardened-list
> kernels, the result changes from a warning to a hang.
> 
> Should the non-READY branch move the state to ICE_PTP_UNINIT, or should
> ice_ptp_cleanup_pf() be made safe to call twice?
> 

I suppose setting to ICE_PTP_UNINIT makes sense. This is the same issue
as reported on the previous patch.

>> +
>> +	synchronize_srcu(&ports->srcu);
>>  }
>