On 9/23/2026 2:47 AM, Loktionov, Aleksandr wrote:
>
>
>> -----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]>
>
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
>
