Re: [PATCH net 01/15] ice: use reference counting and RCU for PTP port access

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: use reference counting and RCU for PTP port access

The adapter-wide PTP port list is converted from a mutex-protected list to
an RCU list plus a kref on each struct ice_ptp_port. Iterators either stay
inside a single RCU read section, or take a reference and drop RCU across
the per-port work. PF teardown unlinks the port, drops the primary
reference, waits (with a 15 second timeout) for the remaining references,
and then calls synchronize_rcu().

A couple of questions below about the new iteration pattern and about a
few lifetime claims in the commit message.

quoted

The kref reference counter ensures that we can
safely acquire pointers with a guarantee of their lifetime, ensuring the
associated PF will not be removed until the reference is released.

[Severity: High]

This isn't a bug introduced by this patch, but does that sentence read
wider than the change actually is?  The new kref covers the port list
only; the other cross-PF pointer on the adapter is still unprotected:

quoted

drivers/net/ethernet/intel/ice/ice_ptp.c:

static void ice_ptp_setup_adapter(struct ice_pf *pf)
{
	pf->adapter->ctrl_pf = pf;
}

drivers/net/ethernet/intel/ice/ice.h:
static inline struct ice_pf *ice_get_ctrl_pf(struct ice_pf *pf)
{
	return !pf->adapter ? NULL : pf->adapter->ctrl_pf;
}

grep over drivers/net/ethernet/intel/ice/ finds exactly one assignment of
ctrl_pf and no site that ever clears it.

If the clock owner PF is unbound while a sibling PF of the same adapter
stays bound, the ice_adapter survives on the sibling's refcount, but
ctrl_pf still points at the freed struct ice_pf.  Can a later tx-clk
change then dereference it?

drivers/net/ethernet/intel/ice/ice_txclk.c:ice_txclk_set_clk()
	mutex_lock(&ctrl_pf->dplls.lock);
	clk_in_use = ice_txclk_any_port_uses(ctrl_pf, phy, clk);

The IS_ERR_OR_NULL(ctrl_pf) checks in that file cannot detect a stale
non-NULL pointer.  Would it be worth stating in the commit message that
the new reference counting is scoped to the port list, so that ctrl_pf
isn't assumed to be covered?

Yes, I'll update it. We have another developer working on fixing ctrl_pf.

quoted

diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
index 4f695f32da3d8..39923dedd534e 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.h
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.h

@@ -19,13 +19,13 @@ struct ice_pf;
  *
  * This structure contains data used to maintain a list of adapter ports
  *
- * @ports: list of ports
+ * @list: list of ports
  * @lock: protect access to the ports list
  */
 struct ice_port_list {
-	struct list_head ports;
+	struct list_head list;
 	/* To synchronize the ports list operations */
-	struct mutex lock;
+	spinlock_t lock;
 };

[Severity: Low]

This isn't a bug, but should the kernel-doc be updated along with the
locking model?  After this patch, ports.lock is taken only around
list_add_rcu() in ice_ptp_setup_pf() and list_del_rcu() in
ice_ptp_cleanup_pf(); all five traversal sites in ice_ptp.c use
rcu_read_lock() plus list_for_each_entry_rcu() and never take the lock.

The comments still say "@lock: protect access to the ports list" and "To
synchronize the ports list operations", which reads as if holding the lock
were required (or sufficient) for iteration.  Would it help to note that
the list is RCU-protected for readers, and that readers which leave the
RCU section must hold a port reference?

Yep, I'll update this.

quoted

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index eaec36ab6ae3f..b12181b8c8434 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c

[ ... ]

quoted

@@ -673,20 +675,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_rcu(struct kref *ref)
+{
+	wake_up_var(ref);
+}
+
 static void ice_ptp_tx_tstamp_owner(struct ice_pf *pf)
 {
 	struct ice_ptp_port *port;
 
-	mutex_lock(&pf->adapter->ports.lock);
-	list_for_each_entry(port, &pf->adapter->ports.ports, list_node) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(port, &pf->adapter->ports.list, list_node) {
 		struct ice_ptp_tx *tx = &port->tx;
 
-		if (!tx || !tx->init)
+		if (!tx->init)
 			continue;
 
+		if (!kref_get_unless_zero(&port->ref))
+			continue;
+
+		rcu_read_unlock();
+
 		ice_ptp_process_tx_tstamp(tx);
+
+		rcu_read_lock();
+		kref_put(&port->ref, ice_ptp_release_port_rcu);
 	}
-	mutex_unlock(&pf->adapter->ports.lock);
+	rcu_read_unlock();
 }

[Severity: High]

Is the loop advance still safe here?  The kref is held on the current port
only, and the for-loop advance reads port->list_node.next after the RCU
read section has been reopened.  list_del_rcu() only updates the
neighbours, so the removed entry's next pointer keeps pointing at its old
successor, and ice_ptp_cleanup_pf() unlinks before it drains references:

	spin_lock(&pf->adapter->ports.lock);
	list_del_rcu(&ptp->port.list_node);
	spin_unlock(&pf->adapter->ports.lock);

	ref = &ptp->port.ref;
	kref_put(ref, ice_ptp_release_port_rcu);

Can this sequence happen with two functions of the same adapter, where
port B follows port A in the list?

CPU0 (clock owner, ice_ptp_tx_tstamp_owner())
	kref_get_unless_zero(&portA->ref);
	rcu_read_unlock();
	ice_ptp_process_tx_tstamp(&portA->tx);   /* long PHY access */

CPU1 (unbind of the PF owning port B)
	ice_ptp_cleanup_pf()
	  list_del_rcu(&portB->list_node);
	  kref_put(&portB->ref, ...);            /* refcount reaches 0 */
	  wait_var_event_timeout(...)            /* returns immediately */
	  synchronize_rcu();                     /* CPU0 holds no RCU lock */
	... struct ice_pf of B (which embeds portB) is freed

CPU0
	rcu_read_lock();
	kref_put(&portA->ref, ice_ptp_release_port_rcu);
	/* loop advance: */
	pos = list_entry_rcu(portA->list_node.next, ...);  /* freed portB */
	if (!tx->init)                        /* read of freed memory */
	kref_get_unless_zero(&portB->ref)     /* cmpxchg on freed memory */

The same drop-RCU-inside-the-loop pattern appears in
ice_ptp_flush_all_tx_tracker(), ice_ptp_restart_all_phy() and
ice_ptp_prepare_rebuild_sec() below, and the windows there are wide
(ice_ptp_port_phy_restart() takes ps_lock and reprograms the PHY).

Note that ice_ptp_tx_tstamp_owner() previously held ports.lock across the
entire iteration, so this particular hazard is new for that function.
Would taking a reference on the next entry before dropping RCU, or keeping
the whole iteration inside one RCU read section, close this?

I'm switching to SRCU to allow holding the RCU critical section over the
entire sequences instead. We can't use regular rcu_read_lock() for that
because operations sleep.

quoted

 /**

@@ -808,8 +823,16 @@ ice_ptp_flush_all_tx_tracker(struct ice_pf *pf)
 {
 	struct ice_ptp_port *port;
 
-	list_for_each_entry(port, &pf->adapter->ports.ports, list_node)
+	rcu_read_lock();
+	list_for_each_entry_rcu(port, &pf->adapter->ports.list, list_node) {
+		if (!kref_get_unless_zero(&port->ref))
+			continue;
+		rcu_read_unlock();
 		ice_ptp_flush_tx_tracker(ptp_port_to_pf(port), &port->tx);
+		rcu_read_lock();
+		kref_put(&port->ref, ice_ptp_release_port_rcu);
+	}
+	rcu_read_unlock();
 }

[Severity: Medium]

This is a pre-existing issue, not something this patch introduces, but
does the new port reference also cover the Tx tracker allocations reached
through the port?  At this commit a peer reset frees them while the port
stays linked in the list:

ice_ptp_prepare_for_reset()
	ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);

Thats actually a bug that is fixed in this series, as we modified the
reset flow to avoid needing to release the tracker but accidentally
forgot to remove this cleanup.

ice_ptp_release_tx_tracker()
	synchronize_irq(pf->oicr_irq.virq);
	ice_ptp_flush_tx_tracker(pf, tx);
	kfree(tx->tstamps);

The synchronize_irq() is on the peer's own OICR vector, which does not
exclude the clock owner's IRQ thread in ICE_PTP_TX_INTERRUPT_ALL mode, and
the owner samples port->tx.init without tx->lock before calling
ice_ptp_process_tx_tstamp() or ice_ptp_flush_tx_tracker() on that peer
port.  Since the port is never unlinked in this path, the kref drain in
ice_ptp_cleanup_pf() does not apply.

The triggering call is removed later in the series by "ice: fix removal of
PTP timestamp tracker during reset", after which
ice_ptp_release_tx_tracker() only runs from ice_ptp_release() once
ice_ptp_cleanup_pf() has unlinked the port and drained references.

Yep this is correct. We could re-order the series I suppose but I think
its ok as-is.

quoted

 /**

[ ... ]

quoted

@@ -1424,16 +1453,21 @@ 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_ptp_port *port;
 
-	list_for_each(entry, &pf->adapter->ports.ports) {
-		struct ice_ptp_port *port = list_entry(entry,
-						       struct ice_ptp_port,
-						       list_node);
+	rcu_read_lock();
+	list_for_each_entry_rcu(port, &pf->adapter->ports.list, list_node) {
+		if (!kref_get_unless_zero(&port->ref))
+			continue;
+		rcu_read_unlock();
 
 		if (port->link_up)
 			ice_ptp_port_phy_restart(port);
+
+		rcu_read_lock();
+		kref_put(&port->ref, ice_ptp_release_port_rcu);
 	}
+	rcu_read_unlock();
 }

[ ... ]

quoted

@@ -2890,14 +2924,16 @@ void ice_ptp_queue_work(struct ice_pf *pf)
 static void ice_ptp_prepare_rebuild_sec(struct ice_pf *pf, bool rebuild,
 					enum ice_reset_req reset_type)
 {
-	struct list_head *entry;
+	struct ice_ptp_port *port;
 
-	list_for_each(entry, &pf->adapter->ports.ports) {
-		struct ice_ptp_port *port = list_entry(entry,
-						       struct ice_ptp_port,
-						       list_node);
+	rcu_read_lock();
+	list_for_each_entry_rcu(port, &pf->adapter->ports.list, list_node) {
 		struct ice_pf *peer_pf = ptp_port_to_pf(port);
 
+		if (!kref_get_unless_zero(&port->ref))
+			continue;
+		rcu_read_unlock();
+

[ ... ]

quoted

@@ -3086,11 +3126,11 @@ static int ice_ptp_setup_pf(struct ice_pf *pf)
 		return -ENODEV;
 
 	INIT_LIST_HEAD(&ptp->port.list_node);
-	mutex_lock(&pf->adapter->ports.lock);
+	kref_init(&ptp->port.ref);
 
-	list_add(&ptp->port.list_node,
-		 &pf->adapter->ports.ports);
-	mutex_unlock(&pf->adapter->ports.lock);
+	spin_lock(&pf->adapter->ports.lock);
+	list_add_rcu(&ptp->port.list_node, &pf->adapter->ports.list);
+	spin_unlock(&pf->adapter->ports.lock);

[Severity: Medium]

This isn't introduced by this patch, but is every successful
ice_ptp_setup_pf() matched by an ice_ptp_cleanup_pf() at this commit?  In
ice_ptp_init():

	ptp->state = ICE_PTP_READY;

	err = ice_ptp_init_work(pf, ptp);
	if (err)
		goto err_exit;
	...
err_clean_pf:
	mutex_destroy(&ptp->port.ps_lock);
	ice_ptp_cleanup_pf(pf);
err_exit:
	...
	ptp->state = ICE_PTP_UNINIT;

The err_exit path skips ice_ptp_cleanup_pf() and leaves the state at
ICE_PTP_UNINIT, and ice_ptp_release() then does:

	if (pf->ptp.state == ICE_PTP_UNINIT)
		return;

So if kthread_run_worker() in ice_ptp_init_work() fails, does the port stay
linked on adapter->ports.list with its primary reference held, inside
memory that is freed at PF removal?  That would also leave
kref_get_unless_zero() succeeding on freed memory for every peer iterating
the list, and should trip WARN_ON(!list_empty(&adapter->ports.list)) in
ice_adapter_free().

This is fixed later in the series by "ice: call PTP link change only from
link events", which moves ice_ptp_init_work() before the state assignment
and sends its failure path to err_clean_pf.

quoted

 
 	/* Seed the per-PHY Tx reference clock usage map for this port.
 	 * Only meaningful on E825 (other MAC types don't expose tx-clk

@@ -3113,12 +3153,32 @@ static int ice_ptp_setup_pf(struct ice_pf *pf)
 static void ice_ptp_cleanup_pf(struct ice_pf *pf)
 {
 	struct ice_ptp *ptp = &pf->ptp;
+	struct kref *ref;
 
-	if (pf->hw.mac_type != ICE_MAC_UNKNOWN) {
-		mutex_lock(&pf->adapter->ports.lock);
-		list_del(&ptp->port.list_node);
-		mutex_unlock(&pf->adapter->ports.lock);
-	}
+	if (pf->hw.mac_type == ICE_MAC_UNKNOWN)
+		return;
+
+	/* The PF cannot be removed until there are no more remaining
+	 * outstanding references to the PTP port. To make sure this is true,
+	 * first remove the port from the list, then drop the primary
+	 * reference this PF holds on the port. Once done, wait until all
+	 * existing references are dropped. Finally, synchronize_rcu() to
+	 * ensure that all RCU critical sections that might attempt to
+	 * dereference the port are finished.
+	 */

[Severity: Low]

The commit message describes the 15 second timeout and the tradeoff it
accepts, but this comment says the PF "cannot be removed until there are no
more remaining outstanding references" and that the code will "wait until
all existing references are dropped".  Should the comment mention the
timeout, since teardown continues after the dev_WARN_ONCE() with a nonzero
reference count?

I can update the comment.

quoted

+
+	spin_lock(&pf->adapter->ports.lock);
+	list_del_rcu(&ptp->port.list_node);
+	spin_unlock(&pf->adapter->ports.lock);
+
+	ref = &ptp->port.ref;
+	kref_put(ref, ice_ptp_release_port_rcu);
+
+	dev_WARN_ONCE(ice_pf_to_dev(pf),
+		      !wait_var_event_timeout(ref, !kref_read(ref), 15 * HZ),
+		      "Timed out waiting for port references to release. Continuing to unload anyways.");
+
+	synchronize_rcu();
 }

[Severity: Low]

This is a pre-existing ordering issue, not something this patch changes,
but now that ice_ptp_cleanup_pf() is the reference/RCU barrier, would it
make sense to call it before destroying port members?  Both teardown paths
currently do the opposite:

ice_ptp_init():
err_clean_pf:
	mutex_destroy(&ptp->port.ps_lock);
	ice_ptp_cleanup_pf(pf);

ice_ptp_release():
	if (pf->ptp.state != ICE_PTP_READY) {
		mutex_destroy(&pf->ptp.port.ps_lock);
		ice_ptp_cleanup_pf(pf);

In that window the port is still published on adapter->ports.list with its
primary reference held, so a peer in ice_ptp_restart_all_phy() or
ice_ptp_prepare_rebuild_sec() can take a reference and reach
mutex_lock(&ptp_port->ps_lock) on the destroyed mutex.  With
CONFIG_DEBUG_MUTEXES that is a DEBUG_LOCKS_WARN_ON splat rather than
corruption, since mutex_destroy() only clears lock->magic, but swapping the
two calls would keep cleanup_pf() as the single teardown point.

[ ... ]

Yes that makes sense. Will fix.