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

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

On 9/23/2026 2:47 AM, Loktionov, Aleksandr wrote:
> 
> 
>> -----Original Message-----
>> From: Jacob Keller <jacob.e.keller@intel.com>
>> Sent: Tuesday, September 22, 2026 8:03 PM
>> To: Keller, Jacob E <jacob.e.keller@intel.com>; Nitka, Grzegorz
>> <grzegorz.nitka@intel.com>; Kubalewski, Arkadiusz
>> <arkadiusz.kubalewski@intel.com>; Intel Wired LAN <intel-wired-
>> lan@lists.osuosl.org>; Machnikowski, Maciej
>> <maciej.machnikowski@intel.com>; Korba, Przemyslaw
>> <przemyslaw.korba@intel.com>; netdev@vger.kernel.org; Nguyen, Anthony
>> L <anthony.l.nguyen@intel.com>
>> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Machnikowski, Maciej
>> <maciej.machnikowski@intel.com>
>> 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 <maciej.machnikowski@intel.com>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> ---
>>  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 <aleksandr.loktionov@intel.com>
> 

Oop. I think I was looking at some sample code that didn't check it.
Will fix.

>>
>>  	return adapter;
>>  }
>>
>>  static void ice_adapter_free(struct ice_adapter *adapter)  {
> 
> ...
> 
>>  }
>>
>>  /**
>>
>> --
>> 2.56.0.rc0.395.gd1f3524e15dc
>