> -----Original Message----- > From: Jacob Keller <[email protected]> > Sent: Tuesday, September 22, 2026 8:03 PM > To: Keller, Jacob E <[email protected]>; Nitka, Grzegorz > <[email protected]>; Kubalewski, Arkadiusz > <[email protected]>; Intel Wired LAN <intel-wired- > [email protected]>; Machnikowski, Maciej > <[email protected]>; Korba, Przemyslaw > <[email protected]>; [email protected]; Nguyen, Anthony > L <[email protected]> > Cc: Keller, Jacob E <[email protected]>; Machnikowski, Maciej > <[email protected]> > Subject: [PATCH iwl-net v2 01/15] ice: use reference counting and SRCU > for PTP port access > > The ice adapter structure maintains a list of ports associated with > the adapter. This is used for supporting PTP, where the clock owner > must handle many operations that require access to the PTP port > structures of the associated PFs. > > This is implemented using a linked list and a mutex. This sort of > works, but a few places within the code do not acquire the mutex when > iterating the list. This includes ice_ptp_flush_all_tx_tracker(), > ice_ptp_restart_all_phy(), and ice_ptp_prepare_rebuild_sec(). > > Fixing this is tricky, especially since it is not clear if we can > simply acquire the lock around the complete iterations. > > The pattern of use for the port list is read-mostly with modifications > only happening during PF initialization when elements are inserted. > This typically only happens during early boot, though a PF could in > principle be removed or loaded at arbitrary times via bind and unbind > operations. > > The use of a mutex does mean the driver can sleep while holding it, > but it still creates complicates with lock ordering and prevents > iterating the list in any code path that *can't* sleep. > > Instead, use SRCU primitives for the port linked list, along with a > reference count on the port. The kref reference counter ensures that a > PF will have a valid lifetime and not be removed until the reference > is released. Note that this change focuses solely on the port list and > does not make an effort to resolve access to ctrl_pf, which is > currently being investigated by another developer. > > Use of sleepable RCU is required because we often iterate the PTP port > list and perform operations that might sleep. Attempts at implementing > regular RCU have thus far not proven to be acceptable. > > The remove path first removes the port from the list, and we use > kref_get_unless_zero to ensure that such ports are skipped when > iterating the list. This ensures that once a port starts removing we > will drop references and no longer be able to acquire new ones. This > avoids loop iterations chaining together to indefinitely block > removal. > > The ice_ptp_release_port_srcu() function is used as the release > function for the kref_put() call. This uses wake_up_var() to wake the > removing thread. > The waiting thread will block until the final reference has been > removed, then it will use synchronize_srcu() to ensure any outstanding > SRCU critical sections have had the necessary grace period. > > This flow ensures that accesses to ports via the adapter port list > will remain valid until both the SRCU critical sections have ended and > all the references to the ports have been dropped. Strictly speaking, > SRCU alone might be sufficient for existing code paths, but the > reference count allows the option for passing a pointer to the port on > to other functions if necessary in the future. > > One major complication of this reference count is that ice_ptp_port is > embedded inside of other structures and not merely allocated. As a > result, we can't use the standard pattern of kfree_rcu() to just delay > freeing until references are dropped, and instead are delaying PF port > teardown. If any code path leaks the reference, the driver will be > unable to teardown. > Instead, a 15 second timeout with a WARN() is used when waiting to > finally allow PF teardown to continue. This has the risk of > potentially allowing use-after-free, assuming some path really is > stuck for 15 seconds. However, this both less likely and a less bad > outcome compared to blocking indefinitely on a reference leak. > > Fixes: e800654e85b5 ("ice: Use ice_adapter for PTP shared data instead > of auxdev") > Reviewed-by: Maciek Machnikowski <[email protected]> > Signed-off-by: Jacob Keller <[email protected]> > --- > drivers/net/ethernet/intel/ice/ice_adapter.h | 16 +-- > drivers/net/ethernet/intel/ice/ice_ptp.h | 4 + > drivers/net/ethernet/intel/ice/ice_adapter.c | 11 +- > drivers/net/ethernet/intel/ice/ice_ptp.c | 145 > +++++++++++++++++++++------ > 4 files changed, 134 insertions(+), 42 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h > b/drivers/net/ethernet/intel/ice/ice_adapter.h > index 4f695f32da3d..93f041943bdd 100644 > --- a/drivers/net/ethernet/intel/ice/ice_adapter.h > +++ b/drivers/net/ethernet/intel/ice/ice_adapter.h > @@ -17,15 +17,19 @@ struct ice_pf; > /** > * struct ice_port_list - data used to store the list of adapter > ports > *
... > struct ice_ptp_tx tx; > struct kthread_delayed_work ov_work; > struct mutex ps_lock; /* protects overall PTP PHY start > procedure */ diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c > b/drivers/net/ethernet/intel/ice/ice_adapter.c > index 2dc3629d6d0f..84ac5ee5a739 100644 > --- a/drivers/net/ethernet/intel/ice/ice_adapter.c > +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c > @@ -6,6 +6,7 @@ > #include <linux/pci.h> > #include <linux/slab.h> > #include <linux/spinlock.h> > +#include <linux/srcu.h> > #include <linux/xarray.h> > #include "ice_adapter.h" > #include "ice.h" > @@ -66,18 +67,20 @@ static struct ice_adapter *ice_adapter_new(struct > pci_dev *pdev) > mutex_init(&adapter->cpi_phy_lock[i]); > refcount_set(&adapter->refcount, 1); > > - mutex_init(&adapter->ports.lock); > - INIT_LIST_HEAD(&adapter->ports.ports); > + spin_lock_init(&adapter->ports.lock); > + INIT_LIST_HEAD(&adapter->ports.list); > + init_srcu_struct(&adapter->ports.srcu); In theory init_srcu_struct() can fail, so error code should be handled, isn't it? Reviewed-by: Aleksandr Loktionov <[email protected]> > > return adapter; > } > > static void ice_adapter_free(struct ice_adapter *adapter) { ... > } > > /** > > -- > 2.56.0.rc0.395.gd1f3524e15dc
