* [PATCH net 0/6] ice: E82x: reduced timestamp processing logic fixes
@ 2026-10-28 0:05 Jacob Keller
2026-10-28 0:05 ` [PATCH net 1/6] ice: use reference counting and SRCU for PTP port access Jacob Keller
` (7 more replies)
0 siblings, 8 replies; 13+ messages in thread
From: Jacob Keller @ 2026-10-28 0:05 UTC (permalink / raw)
To: Przemek Kitszel, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Petr Oros, netdev
Cc: Grzegorz Nitka, Aleksandr Loktionov, Alexander Nowlin,
Tony Nguyen, jacob.e.keller, maciej.machnikowski,
przemyslaw.korba, sergey.temerkhanov, arkadiusz.kubalewski,
richardcochran, horms
Jake Keller says:
This is a reduced set patches originally posted as part of [1] which
previously passes Sashiko review without comments. It does not completely
resolve the outstanding issues that we had, but can hopefully unblock some
progress.
First, the locking around the PTP ports list in the adapter structure is
converted to use RCU primitives and a spinlock, resolving a couple of
reports from Petr about places where the original list was accessed without
lock protection. Note that an older version of this fix used an xarray
instead of the list. The xarray has more overhead and results in an
increase of ~25 microseconds to the average latency for processing Tx
timestamps. The list is simpler and avoids this overhead.
Next, the ice_ptp_request_ts() function is updated to sequence the marking
of the in_use bitmap in order to work properly with the lockless reader in
the IRQ thread. This issue was reported by Sashiko during development.
Next, Przemyslaw Korba fixes the E822 timestamping logic to keep timestamp
requests disabled until Vernier calibration completes.
Next, the driver is modified to stop clearing the PHY_REG_TX_OFFSET_READY
bit. This bits only purpose is to tell hardware to mark any captured
timestamps as invalid. Since this also disables the necessary side effects
on read it is problematic to have cleared. According to hardware engineers,
keeping it enabled should not have any other side effects. Instead, the
tx.disabled field is used to disable new timestamps from software in a
similar manner to the older E822 devices.
Next, the driver is modified to clear the PHY_REG_TX_MEMORY_STATUS by
reading each index *prior* to the PHY soft reset. This ensures that any
stale or invalid data left in the memory array is cleared, followed by the
counter being reset via the PHY soft reset procedure.
Finally, the ice_ptp_process_tx_tstamp() function is modified to skip
unnecessarily checking the ready bitmap for ports which do not have active
timestamp requests.
I plan to rework and follow up the remaining patches from the original
series after this lands.
[1]: https://lore.kernel.org/netdev/710677e5-3668-4588-bd36-2d8f7fb692a9@intel.com/
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Sashiko previously had no complaints about the SRCU changes on the port
list. I suspect it may now complain about some invalid accesses due to the
error cleanup flows in the reset or failure. I believe those should be
considered pre-existing issues and not block this reduced series.
---
Jacob Keller (5):
ice: use reference counting and SRCU for PTP port access
ice: set in_use only after preparing Tx timestamp index
ice: E825: stop clearing PHY_REG_TX_OFFSET_READY
ice: E825: clear PHY_TSTAMP memory prior to soft reset
ice: skip reading Tx ready bitmap on ports with no timestamps
Karol Kolacinski (1):
ice: E822: keep Tx timestamps disabled during offset calibration
drivers/net/ethernet/intel/ice/ice_adapter.h | 17 ++-
drivers/net/ethernet/intel/ice/ice_ptp.h | 11 +-
drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 2 +-
drivers/net/ethernet/intel/ice/ice_adapter.c | 17 ++-
drivers/net/ethernet/intel/ice/ice_ptp.c | 218 ++++++++++++++++++++-------
drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 143 ++++++++++--------
6 files changed, 275 insertions(+), 133 deletions(-)
---
base-commit: abeacca61eea4f6803df4a595b656ea45c2d3249
base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git main
change-id: 20261027-reduction-fe6ac6f36cb2
Best regards,
--
Jacob Keller <jacob.e.keller@intel.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net 1/6] ice: use reference counting and SRCU for PTP port access
2026-10-28 0:05 [PATCH net 0/6] ice: E82x: reduced timestamp processing logic fixes Jacob Keller
@ 2026-10-28 0:05 ` Jacob Keller
2026-10-28 0:05 ` [PATCH net 2/6] ice: set in_use only after preparing Tx timestamp index Jacob Keller
` (6 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Jacob Keller @ 2026-10-28 0:05 UTC (permalink / raw)
To: Przemek Kitszel, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Petr Oros, netdev
Cc: Grzegorz Nitka, Aleksandr Loktionov, Alexander Nowlin,
Tony Nguyen, jacob.e.keller, maciej.machnikowski,
przemyslaw.korba, sergey.temerkhanov, arkadiusz.kubalewski,
richardcochran, horms
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.
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 | 17 ++--
drivers/net/ethernet/intel/ice/ice_ptp.h | 4 +
drivers/net/ethernet/intel/ice/ice_adapter.c | 17 +++-
drivers/net/ethernet/intel/ice/ice_ptp.c | 125 ++++++++++++++++++++-------
4 files changed, 121 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..c61bd645e460 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.h
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
@@ -6,6 +6,7 @@
#include <linux/types.h>
#include <linux/mutex.h>
+#include <linux/srcu.h>
#include <linux/spinlock_types.h>
#include <linux/refcount_types.h>
@@ -17,15 +18,19 @@ struct ice_pf;
/**
* struct ice_port_list - data used to store the list of adapter ports
*
- * This structure contains data used to maintain a list of adapter ports
+ * This structure contains data used to maintain a list of adapter ports.
+ * Writers modifying the list *must* acquire the lock, and use SRCU safe list
+ * operations. Readers should use srcu_read_lock() on the provided domain.
*
- * @ports: list of ports
- * @lock: protect access to the ports list
+ * @list: list of ports
+ * @lock: protect write access to the list
+ * @srcu: Sleepable RCU domain for this adapter
*/
struct ice_port_list {
- struct list_head ports;
- /* To synchronize the ports list operations */
- struct mutex lock;
+ struct list_head list;
+ /* To synchronize write operations on the port list */
+ spinlock_t lock;
+ struct srcu_struct srcu;
};
/**
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index c4b0da7ce20e..da2003ba3bb0 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -5,6 +5,8 @@
#define _ICE_PTP_H_
#include <linux/ptp_clock_kernel.h>
+#include <linux/rculist.h>
+#include <linux/kref.h>
#include <linux/kthread.h>
#include "ice_ptp_hw.h"
@@ -138,6 +140,7 @@ struct ice_ptp_tx {
* and determine when the port's PHY offset is valid.
*
* @list_node: list member structure
+ * @ref: reference counter for use with adapter ports list
* @tx: Tx timestamp tracking for this port
* @ov_work: delayed work task for tracking when PHY offset is valid
* @ps_lock: mutex used to protect the overall PTP PHY start procedure
@@ -149,6 +152,7 @@ struct ice_ptp_tx {
*/
struct ice_ptp_port {
struct list_head list_node;
+ struct kref ref;
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..536923b6ae97 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"
@@ -54,11 +55,18 @@ static unsigned long ice_adapter_xa_index(struct pci_dev *pdev)
static struct ice_adapter *ice_adapter_new(struct pci_dev *pdev)
{
struct ice_adapter *adapter;
+ int err;
adapter = kzalloc_obj(*adapter);
if (!adapter)
return NULL;
+ err = init_srcu_struct(&adapter->ports.srcu);
+ if (err) {
+ kfree(adapter);
+ return NULL;
+ }
+
adapter->index = ice_adapter_index(pdev);
spin_lock_init(&adapter->ptp_gltsyn_time_lock);
spin_lock_init(&adapter->txq_ctx_lock);
@@ -66,18 +74,19 @@ 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);
return adapter;
}
static void ice_adapter_free(struct ice_adapter *adapter)
{
- WARN_ON(!list_empty(&adapter->ports.ports));
+ WARN_ON(!list_empty(&adapter->ports.list));
for (int i = 0; i < ARRAY_SIZE(adapter->cpi_phy_lock); i++)
mutex_destroy(&adapter->cpi_phy_lock[i]);
- mutex_destroy(&adapter->ports.lock);
+
+ cleanup_srcu_struct(&adapter->ports.srcu);
kfree(adapter);
}
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index eaec36ab6ae3..e6442b956aa9 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -1,6 +1,9 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (C) 2021, Intel Corporation. */
+#include <linux/rculist.h>
+#include <linux/srcu.h>
+#include <linux/wait_bit.h>
#include "ice.h"
#include "ice_lib.h"
#include "ice_trace.h"
@@ -673,20 +676,37 @@ 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() requires full ordering but kref_put only provides
+ * release ordering.
+ */
+ smp_mb__after_atomic();
+ wake_up_var(ref);
+}
+
static void ice_ptp_tx_tstamp_owner(struct ice_pf *pf)
{
+ struct ice_port_list *ports = &pf->adapter->ports;
struct ice_ptp_port *port;
+ int srcu_idx;
- mutex_lock(&pf->adapter->ports.lock);
- list_for_each_entry(port, &pf->adapter->ports.ports, 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)) {
struct ice_ptp_tx *tx = &port->tx;
- if (!tx || !tx->init)
+ if (!tx->init)
+ continue;
+
+ if (!kref_get_unless_zero(&port->ref))
continue;
ice_ptp_process_tx_tstamp(tx);
+
+ kref_put(&port->ref, ice_ptp_release_port_srcu);
}
- mutex_unlock(&pf->adapter->ports.lock);
+ srcu_read_unlock(&ports->srcu, srcu_idx);
}
/**
@@ -806,10 +826,19 @@ ice_ptp_mark_tx_tracker_stale(struct ice_ptp_tx *tx)
static void
ice_ptp_flush_all_tx_tracker(struct ice_pf *pf)
{
+ struct ice_port_list *ports = &pf->adapter->ports;
struct ice_ptp_port *port;
+ int srcu_idx;
- list_for_each_entry(port, &pf->adapter->ports.ports, 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;
ice_ptp_flush_tx_tracker(ptp_port_to_pf(port), &port->tx);
+ kref_put(&port->ref, ice_ptp_release_port_srcu);
+ }
+ srcu_read_unlock(&ports->srcu, srcu_idx);
}
/**
@@ -1424,16 +1453,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);
+
+ kref_put(&port->ref, ice_ptp_release_port_srcu);
}
+ srcu_read_unlock(&ports->srcu, srcu_idx);
}
/**
@@ -2694,19 +2729,28 @@ static bool ice_port_has_timestamps(struct ice_ptp_tx *tx)
static bool ice_any_port_has_timestamps(struct ice_pf *pf)
{
+ struct ice_port_list *ports = &pf->adapter->ports;
+ bool have_tstamps = false;
struct ice_ptp_port *port;
+ int srcu_idx;
- scoped_guard(mutex, &pf->adapter->ports.lock) {
- list_for_each_entry(port, &pf->adapter->ports.ports,
- list_node) {
- struct ice_ptp_tx *tx = &port->tx;
+ 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 (ice_port_has_timestamps(tx))
- return true;
- }
+ if (ice_port_has_timestamps(&port->tx))
+ have_tstamps = true;
+
+ kref_put(&port->ref, ice_ptp_release_port_srcu);
+
+ if (have_tstamps)
+ break;
}
+ srcu_read_unlock(&ports->srcu, srcu_idx);
- return false;
+ return have_tstamps;
}
bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf)
@@ -2890,14 +2934,18 @@ 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_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)) {
struct ice_pf *peer_pf = ptp_port_to_pf(port);
+ if (!kref_get_unless_zero(&port->ref))
+ continue;
+
if (!ice_is_primary(&peer_pf->hw)) {
if (rebuild) {
/* TODO: When implementing rebuild=true:
@@ -2909,7 +2957,10 @@ static void ice_ptp_prepare_rebuild_sec(struct ice_pf *pf, bool rebuild,
ice_ptp_prepare_for_reset(peer_pf, reset_type);
}
}
+
+ kref_put(&port->ref, ice_ptp_release_port_srcu);
}
+ srcu_read_unlock(&ports->srcu, srcu_idx);
}
/**
@@ -3086,11 +3137,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);
/* Seed the per-PHY Tx reference clock usage map for this port.
* Only meaningful on E825 (other MAC types don't expose tx-clk
@@ -3112,13 +3163,23 @@ static int ice_ptp_setup_pf(struct ice_pf *pf)
static void ice_ptp_cleanup_pf(struct ice_pf *pf)
{
+ struct ice_port_list *ports = &pf->adapter->ports;
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;
+
+ 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));
+
+ synchronize_srcu(&ports->srcu);
}
/**
--
2.56.0.215.g47fdf2208c5b
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net 2/6] ice: set in_use only after preparing Tx timestamp index
2026-10-28 0:05 [PATCH net 0/6] ice: E82x: reduced timestamp processing logic fixes Jacob Keller
2026-10-28 0:05 ` [PATCH net 1/6] ice: use reference counting and SRCU for PTP port access Jacob Keller
@ 2026-10-28 0:05 ` Jacob Keller
2026-10-28 0:05 ` [PATCH net 3/6] ice: E822: keep Tx timestamps disabled during offset calibration Jacob Keller
` (5 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Jacob Keller @ 2026-10-28 0:05 UTC (permalink / raw)
To: Przemek Kitszel, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Petr Oros, netdev
Cc: Grzegorz Nitka, Aleksandr Loktionov, Alexander Nowlin,
Tony Nguyen, jacob.e.keller, maciej.machnikowski,
przemyslaw.korba, sergey.temerkhanov, arkadiusz.kubalewski,
richardcochran, horms
The ice_ptp_request_ts() function is used to request a timestamp index for
use with a packet. When reserving an index, it sets the start time and
saves a pointer to the skb into the appropriate index. The function marks
the in_use bit first before doing any of these steps. The IRQ handler which
clears the timestamps reads the in_use bits uses a lockless flow for
reading the in_use bits to determine which ones are in-use. This is
necessary as actually processing a complete timestamp must be able to sleep
so we cannot hold the timestamp tracker lock over the entire sequence.
Additionally, blocking the Tx hotpath with such a lock indefinitely would
be problematic.
However, the existing flow now has a very narrow window where the IRQ
handler could see a timestamp as in-use but read a stale value for its
"start" time.
Fix this by ordering the sequence to mark the in_use bit last, and add a
memory barrier to prevent re-ordering of the previous writes to setup the
index.
This was found and reported by Sashiko while reviewing an unrelated change.
Closes: https://sashiko.dev/#/patchset/20260821-jk-e825c-minimized-fixes-v1-0-9d0731eb4858%40intel.com?part=7
Fixes: ea9b847cda64 ("ice: enable transmit timestamps for E810 devices")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
drivers/net/ethernet/intel/ice/ice_ptp.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index e6442b956aa9..48fde5561668 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -592,6 +592,12 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
bool drop_ts = !link_up;
struct sk_buff *skb;
+ /* Paired with smp_mb__before_atomic() in ice_ptp_request_ts()
+ * to ensure ordering the in_use bitmap and the indexed fields
+ * in the tstamps array.
+ */
+ smp_rmb();
+
/* Drop packets which have waited for more than 2 seconds */
if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ)) {
drop_ts = true;
@@ -2673,11 +2679,13 @@ s8 ice_ptp_request_ts(struct ice_ptp_tx *tx, struct sk_buff *skb)
* a reference to the skb and the start time to allow discarding old
* requests.
*/
- set_bit(idx, tx->in_use);
- clear_bit(idx, tx->stale);
tx->tstamps[idx].start = jiffies;
tx->tstamps[idx].skb = skb_get(skb);
skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+ clear_bit(idx, tx->stale);
+ /* Ensure index is setup before marking it as used */
+ smp_mb__before_atomic();
+ set_bit(idx, tx->in_use);
ice_trace(tx_tstamp_request, skb, idx);
}
--
2.56.0.215.g47fdf2208c5b
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net 3/6] ice: E822: keep Tx timestamps disabled during offset calibration
2026-10-28 0:05 [PATCH net 0/6] ice: E82x: reduced timestamp processing logic fixes Jacob Keller
2026-10-28 0:05 ` [PATCH net 1/6] ice: use reference counting and SRCU for PTP port access Jacob Keller
2026-10-28 0:05 ` [PATCH net 2/6] ice: set in_use only after preparing Tx timestamp index Jacob Keller
@ 2026-10-28 0:05 ` Jacob Keller
2026-10-28 0:05 ` [PATCH net 4/6] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY Jacob Keller
` (4 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Jacob Keller @ 2026-10-28 0:05 UTC (permalink / raw)
To: Przemek Kitszel, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Petr Oros, netdev
Cc: Grzegorz Nitka, Aleksandr Loktionov, Alexander Nowlin,
Tony Nguyen, jacob.e.keller, maciej.machnikowski,
przemyslaw.korba, sergey.temerkhanov, arkadiusz.kubalewski,
richardcochran, horms
From: Karol Kolacinski <karol.kolacinski@intel.com>
Do not clear the tx.calibrating flag immediately after starting the PHY
timer in ice_ptp_port_phy_restart(). Instead, keep Tx timestamps
disabled until the offset verification work (ice_ptp_wait_for_offsets)
has confirmed that the Tx PHY offset is properly configured.
Previously, tx.calibrating was set to true, then immediately back to
false right after ice_start_phy_timer_e82x() returned. This allowed Tx
timestamp requests to be served during the window where offset
verification was still pending. Timestamps produced during this window
should have their valid bit cleared, but it is better to prevent requests
until the driver has confirmed calibration is complete.
Rename the tx.calibrating field to tx.disabled, and change where it is
cleared. Only clear it in ice_ptp_wait_for_offsets(), and only after Tx
offset configuration has completed successfully. This ensures that new Tx
timestamp requests are only accepted after the PHY has properly calibrated
the Tx offset.
If ice_start_phy_timer_e82x() fails, do not restore disabled to false.
The device is in a state where timestamps cannot succeed properly anyways.
A dev_err message is already logged on failure to start the timer at the
end of the function.
Renaming the field improves clarity and will better reflect the nature of
the flag in a future change that also applies disabling Tx timestamps in
software in a following commit.
Log a debug message while offset calibration is still pending, including
the specific Tx/Rx error codes to aid debugging stalled calibration.
This path is expected on every routine link-up: ov_work is first queued
with no delay and the vernier offset cannot be computed until at least
one packet has been transmitted, so the first several invocations
normally land here. Use dev_dbg() rather than a rate-limited warning to
avoid emitting KERN_WARNING on every link-up during normal operation.
Log a debug message when calibration completes successfully.
Note that sashiko review has previously complained that leaving the
disabled flag set "permanently" disables Tx timestamps if vernier
calibration never completes. This is true, but its important to realize
that timestamps would still fail regardless of whether the flag is set.
Until vernier calibration completes the device will not report valid
timestamps regardless. Thus, keeping the calibrating flag set simply
prevents new timestamp requests from software while it is known that the
hardware will not complete them.
Fixes: 3a7496234d17 ("ice: implement basic E822 PTP support")
Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
drivers/net/ethernet/intel/ice/ice_ptp.h | 6 +++---
drivers/net/ethernet/intel/ice/ice_ptp.c | 37 ++++++++++++++++++++++----------
2 files changed, 29 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index da2003ba3bb0..8d46e57368ca 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -106,8 +106,8 @@ enum ice_tx_tstamp_work {
* @offset: offset into timestamp block to get the real index
* @len: length of the tstamps and in_use fields.
* @init: if true, the tracker is initialized;
- * @calibrating: if true, the PHY is calibrating the Tx offset. During this
- * window, timestamps are temporarily disabled.
+ * @disabled: set to true when new Tx timestamp requests are disabled, such as
+ * while the PHY Tx offset configuration has not yet completed.
* @has_ready_bitmap: if true, the hardware has a valid Tx timestamp ready
* bitmap register. If false, fall back to verifying new
* timestamp values against previously cached copy.
@@ -122,7 +122,7 @@ struct ice_ptp_tx {
u8 offset;
u8 len;
u8 init : 1;
- u8 calibrating : 1;
+ u8 disabled : 1;
u8 has_ready_bitmap : 1;
s8 last_ll_ts_idx_read;
};
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 48fde5561668..2f8563eb6fb1 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -367,7 +367,7 @@ ice_ptp_is_tx_tracker_up(struct ice_ptp_tx *tx)
{
lockdep_assert_held(&tx->lock);
- return tx->init && !tx->calibrating;
+ return tx->init && !tx->disabled;
}
/**
@@ -1168,6 +1168,7 @@ static int ice_ptp_check_tx_fifo(struct ice_ptp_port *port)
static void ice_ptp_wait_for_offsets(struct kthread_work *work)
{
struct ice_ptp_port *port;
+ unsigned long flags;
struct ice_pf *pf;
struct ice_hw *hw;
int tx_err;
@@ -1188,9 +1189,26 @@ static void ice_ptp_wait_for_offsets(struct kthread_work *work)
tx_err = ice_ptp_check_tx_fifo(port);
if (!tx_err)
tx_err = ice_phy_cfg_tx_offset_e82x(hw, port->port_num);
+ if (!tx_err) {
+ /* Tx offset has been configured, re-enable Tx timestamps */
+ spin_lock_irqsave(&port->tx.lock, flags);
+ if (port->tx.disabled) {
+ port->tx.disabled = false;
+ dev_dbg(ice_pf_to_dev(pf), "PTP Tx offset valid for port %u, Tx timestamps enabled\n",
+ port->port_num);
+ }
+ spin_unlock_irqrestore(&port->tx.lock, flags);
+ }
+
rx_err = ice_phy_cfg_rx_offset_e82x(hw, port->port_num);
if (tx_err || rx_err) {
- /* Tx and/or Rx offset not yet configured, try again later */
+ /* Tx and/or Rx offset not yet configured, try again later.
+ * This is expected during normal link-up: the vernier offset
+ * calibration cannot complete until at least one packet has
+ * been transmitted, so the first retries routinely land here.
+ */
+ dev_dbg(ice_pf_to_dev(pf), "PTP offset not yet valid for port %u (tx_err=%d rx_err=%d)\n",
+ port->port_num, tx_err, rx_err);
kthread_queue_delayed_work(pf->ptp.kworker,
&port->ov_work,
msecs_to_jiffies(100));
@@ -1268,11 +1286,9 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
/* Start the PHY timer in Vernier mode */
kthread_cancel_delayed_work_sync(&ptp_port->ov_work);
- /* temporarily disable Tx timestamps while calibrating
- * PHY offset
- */
+ /* Disable Tx timestamps until PHY calibration completes */
spin_lock_irqsave(&ptp_port->tx.lock, flags);
- ptp_port->tx.calibrating = true;
+ ptp_port->tx.disabled = true;
spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
ptp_port->tx_fifo_busy_cnt = 0;
@@ -1281,11 +1297,10 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
if (err)
break;
- /* Enable Tx timestamps right away */
- spin_lock_irqsave(&ptp_port->tx.lock, flags);
- ptp_port->tx.calibrating = false;
- spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
-
+ /* Do not clear disabled flag here. Tx timestamp requests
+ * remain disabled until ice_ptp_wait_for_offsets() has
+ * verified that the Tx offset calibration has completed.
+ */
kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work,
0);
break;
--
2.56.0.215.g47fdf2208c5b
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net 4/6] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY
2026-10-28 0:05 [PATCH net 0/6] ice: E82x: reduced timestamp processing logic fixes Jacob Keller
` (2 preceding siblings ...)
2026-10-28 0:05 ` [PATCH net 3/6] ice: E822: keep Tx timestamps disabled during offset calibration Jacob Keller
@ 2026-10-28 0:05 ` Jacob Keller
2026-10-29 0:53 ` netdev-bot+sashiko
2026-10-28 0:05 ` [PATCH net 5/6] ice: E825: clear PHY_TSTAMP memory prior to soft reset Jacob Keller
` (3 subsequent siblings)
7 siblings, 1 reply; 13+ messages in thread
From: Jacob Keller @ 2026-10-28 0:05 UTC (permalink / raw)
To: Przemek Kitszel, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Petr Oros, netdev
Cc: Grzegorz Nitka, Aleksandr Loktionov, Alexander Nowlin,
Tony Nguyen, jacob.e.keller, maciej.machnikowski,
przemyslaw.korba, sergey.temerkhanov, arkadiusz.kubalewski,
richardcochran, horms
The ice_stop_phy_timer_eth56g() function is called by the driver for E825
devices to ensure that the PHY timer has been stopped. The equivalent
function for older E822 devices performed many steps. However, on E825 it
only clears the PHY_REG_TX_OFFSET_READY and PHY_REG_RX_OFFSET_READY bits to
indicate to HW that it should no longer treat the PHY offset as valid.
When PHY_REG_TX_OFFSET_READY is cleared, the hardware still captures Tx
timestamps, but it no longer sets the valid bit for these timestamps. This
sounds reasonable at first glance. However, this results in the internal
outstanding timestamp counter becoming out of sync.
When capturing a timestamp, hardware increments its internal counter and
sets the associated "ready" bit in the timestamp memory status. Then it
compares the timestamp count to the threshold to determine if it should
trigger an interrupt to the MAC.
Upon reading the timestamp hardware is supposed to decrement the counter,
clear the valid bit, and clear the associated bit from the memory status
register. However, it only performs these steps *if* the valid bit is set.
Since the valid bit is not set while PHY_REG_TX_OFFSET_READY is clear, the
timestamp counter is not decremented and the memory status is not cleared.
This leaves the counter out-of-sync until a PHY soft reset.
According to the hardware engineers, the PHY_REG_TX_OFFSET_READY bit has no
other effects. It only controls whether hardware captures timestamps with
the valid bit set or not. Since capturing timestamps with the valid bit
clear is problematic, they recommend simply not clearing
PHY_REG_TX_OFFSET_READY.
Note that the PHY_REG_RX_OFFSET_READY performs a similar task. However,
clearing it is fine as there is no associated timestamp counter on the Rx
side. Receive timestamps are simply inserted into the descriptor. Clearing
this register clears the valid bit for timestamps until we complete
calibration and re-enable the register.
Notice that the soft_reset parameter of ice_stop_phy_timer_eth56g() is
totally unused. It is a relic from a copy-paste of
ice_stop_phy_timer_e82x() that is unnecessary, so remove it.
Now that we do not clear the PHY_REG_TX_OFFSET_READY, new timestamp
requests could happen while the PHY is calibrating. To avoid this, set the
tx.disabled field of the Tx timestamp tracker when stopping the timer
and clear it when finishing the restart. This ensures that any new requests
will be rejected until the PHY timer calibration has completed. Also call
ice_ptp_mark_tx_tracker_stale() to prevent reporting any previous
outstanding timestamps to the stack. Fix ice_ptp_mark_tx_tracker_stale() to
properly check if the tracker is initialized, to prevent a possible
use-after-free on the bitmap.
Unlike E822 devices, set the disabled flag when "stopping" the PHY and
clear it immediately after the start procedure. The E825 device does not
perform vernier calibration and thus does not need to wait for hardware to
mark the offsets as valid.
In the event that the PHY timer start procedure fails, the device is in an
unknown state and timestamps will not behave properly. As such, and similar
to E822 devices, the disabled field is not cleared on failure. This
leaves timestamp requests disabled until the next link restart.
Failure in the start flow is unexpected and it is unclear precisely what
state the hardware is left in. Attempting to add a complex retry mechanism
for a rare event is not worthwhile. The port restart procedure can already
be re-initiated by triggering a link reset (i.e. via ethtool).
Instead update the dev_err message at the end of ice_ptp_port_phy_restart.
Clearly indicate that timestamping is not functional, and add a note that a
link toggle might recover the device. For the invalid MAC type path
returning -ENODEV, skip this message and log a dev_dbg that we failed with
an unknown MAC type instead.
Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C products")
Suggested-by: Maciej Machnikowski <maciej.machnikowski@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
drivers/net/ethernet/intel/ice/ice_ptp.h | 5 ++--
drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 2 +-
drivers/net/ethernet/intel/ice/ice_ptp.c | 42 +++++++++++++++++++++++------
drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 25 ++++++++---------
4 files changed, 51 insertions(+), 23 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index 8d46e57368ca..0bc9d77478a9 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -106,8 +106,9 @@ enum ice_tx_tstamp_work {
* @offset: offset into timestamp block to get the real index
* @len: length of the tstamps and in_use fields.
* @init: if true, the tracker is initialized;
- * @disabled: set to true when new Tx timestamp requests are disabled, such as
- * while the PHY Tx offset configuration has not yet completed.
+ * @disabled: set to true when new Tx timestamp requests are disabled. This
+ * includes during E82X Vernier calibration, and for as long as the
+ * PHY is considered stopped on E825.
* @has_ready_bitmap: if true, the hardware has a valid Tx timestamp ready
* bitmap register. If false, fall back to verifying new
* timestamp values against previously cached copy.
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index 16b1988e993d..17000df77ce9 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -378,7 +378,7 @@ int ice_cgu_get_output_pin_state_caps(struct ice_hw *hw, u8 pin_id,
/* ETH56G family functions */
int ice_ptp_read_tx_hwtstamp_status_eth56g(struct ice_hw *hw, u32 *ts_status);
-int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port, bool soft_reset);
+int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port);
int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port);
int ice_phy_cfg_intr_eth56g(struct ice_hw *hw, u8 port, bool ena, u8 threshold);
int ice_phy_cfg_ptp_1step_eth56g(struct ice_hw *hw, u8 port);
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 2f8563eb6fb1..3ba6632bb196 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -808,9 +808,6 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
* Mark currently outstanding Tx timestamps as stale. This prevents sending
* their timestamp value to the stack. This is required to prevent extending
* the 40bit hardware timestamp incorrectly.
- *
- * This should be called when the PTP clock is modified such as after a set
- * time request.
*/
static void
ice_ptp_mark_tx_tracker_stale(struct ice_ptp_tx *tx)
@@ -818,7 +815,8 @@ ice_ptp_mark_tx_tracker_stale(struct ice_ptp_tx *tx)
unsigned long flags;
spin_lock_irqsave(&tx->lock, flags);
- bitmap_or(tx->stale, tx->stale, tx->in_use, tx->len);
+ if (tx->init)
+ bitmap_or(tx->stale, tx->stale, tx->in_use, tx->len);
spin_unlock_irqrestore(&tx->lock, flags);
}
@@ -1226,6 +1224,7 @@ ice_ptp_port_phy_stop(struct ice_ptp_port *ptp_port)
struct ice_pf *pf = ptp_port_to_pf(ptp_port);
u8 port = ptp_port->port_num;
struct ice_hw *hw = &pf->hw;
+ unsigned long flags;
int err;
mutex_lock(&ptp_port->ps_lock);
@@ -1241,7 +1240,14 @@ ice_ptp_port_phy_stop(struct ice_ptp_port *ptp_port)
err = ice_stop_phy_timer_e82x(hw, port, true);
break;
case ICE_MAC_GENERIC_3K_E825:
- err = ice_stop_phy_timer_eth56g(hw, port, true);
+ /* Disable new Tx timestamp requests */
+ spin_lock_irqsave(&ptp_port->tx.lock, flags);
+ ptp_port->tx.disabled = true;
+ spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
+
+ ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
+
+ err = ice_stop_phy_timer_eth56g(hw, port);
break;
default:
err = -ENODEV;
@@ -1305,15 +1311,35 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
0);
break;
case ICE_MAC_GENERIC_3K_E825:
+ /* ice_ptp_port_phy_stop() may have already disabled
+ * timestamps, but some restarts occur without first stopping
+ * the timer, so we ensure that new requests are disabled
+ * here.
+ */
+ spin_lock_irqsave(&ptp_port->tx.lock, flags);
+ ptp_port->tx.disabled = true;
+ spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
+
+ ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
+
err = ice_start_phy_timer_eth56g(hw, port);
+ if (err)
+ break;
+
+ spin_lock_irqsave(&ptp_port->tx.lock, flags);
+ ptp_port->tx.disabled = false;
+ spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
+
break;
default:
- err = -ENODEV;
+ dev_dbg(ice_pf_to_dev(pf), "PTP failed to restart PHY port %u with unknown MAC type %d\n",
+ port, hw->mac_type);
+ return -ENODEV;
}
if (err)
- dev_err(ice_pf_to_dev(pf), "PTP failed to set PHY port %d up, err %d\n",
- port, err);
+ dev_err(ice_pf_to_dev(pf), "PTP failed to restart PHY port %u with err %pe; Timestamping not enabled; A link-toggle may recover.\n",
+ port, ERR_PTR(err));
mutex_unlock(&ptp_port->ps_lock);
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 3a41c711e751..07b55fbb88dd 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -2098,32 +2098,33 @@ static int ice_sync_phy_timer_eth56g(struct ice_hw *hw, u8 port)
}
/**
- * ice_stop_phy_timer_eth56g - Stop the PHY clock timer
+ * ice_stop_phy_timer_eth56g - Clear PHY Rx offset ready flag
* @hw: pointer to the HW struct
* @port: the PHY port to stop
- * @soft_reset: if true, hold the SOFT_RESET bit of PHY_REG_PS
*
- * Stop the clock of a PHY port. This must be done as part of the flow to
- * re-calibrate Tx and Rx timestamping offsets whenever the clock time is
- * initialized or when link speed changes.
+ * Disable Rx timestamping by clearing the PHY_REG_RX_OFFSET_READY. This
+ * causes Rx timestamps to be captured with their valid bit clear, ensuring we
+ * discard any timestamp captured while the PHY is being recalibrated.
+ *
+ * Note this does *not* clear PHY_REG_TX_OFFSET_READY. Clearing it would
+ * cause the Tx timestamps to be captured with their valid bit clear.
+ * Unfortunately the captured timestamps still increment the internal counter
+ * and result in off-by-one accounting. Instead, Tx timestamp requests should
+ * be disabled by other means.
*
* Return:
* * %0 - success
* * %other - failed to write to PHY
*/
-int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port, bool soft_reset)
+int ice_stop_phy_timer_eth56g(struct ice_hw *hw, u8 port)
{
int err;
- err = ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_OFFSET_READY, 0);
- if (err)
- return err;
-
err = ice_write_ptp_reg_eth56g(hw, port, PHY_REG_RX_OFFSET_READY, 0);
if (err)
return err;
- ice_debug(hw, ICE_DBG_PTP, "Disabled clock on PHY port %u\n", port);
+ ice_debug(hw, ICE_DBG_PTP, "Disabled Rx timestamps on PHY port %u\n", port);
return 0;
}
@@ -2151,7 +2152,7 @@ int ice_start_phy_timer_eth56g(struct ice_hw *hw, u8 port)
tmr_idx = ice_get_ptp_src_clock_index(hw);
- err = ice_stop_phy_timer_eth56g(hw, port, false);
+ err = ice_stop_phy_timer_eth56g(hw, port);
if (err)
return err;
--
2.56.0.215.g47fdf2208c5b
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net 5/6] ice: E825: clear PHY_TSTAMP memory prior to soft reset
2026-10-28 0:05 [PATCH net 0/6] ice: E82x: reduced timestamp processing logic fixes Jacob Keller
` (3 preceding siblings ...)
2026-10-28 0:05 ` [PATCH net 4/6] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY Jacob Keller
@ 2026-10-28 0:05 ` Jacob Keller
2026-10-29 0:53 ` netdev-bot+sashiko
2026-10-28 0:05 ` [PATCH net 6/6] ice: skip reading Tx ready bitmap on ports with no timestamps Jacob Keller
` (2 subsequent siblings)
7 siblings, 1 reply; 13+ messages in thread
From: Jacob Keller @ 2026-10-28 0:05 UTC (permalink / raw)
To: Przemek Kitszel, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Petr Oros, netdev
Cc: Grzegorz Nitka, Aleksandr Loktionov, Alexander Nowlin,
Tony Nguyen, jacob.e.keller, maciej.machnikowski,
przemyslaw.korba, sergey.temerkhanov, arkadiusz.kubalewski,
richardcochran, horms
The current implementation of ice_ptp_reset_ts_memory_eth56g() is flawed.
It tries to clear the timestamp memory by writing to the
PHY_REG_TX_MEMORY_STATUS region. This does not work properly, as it does
not trigger appropriate PHY actions.
To clear outstanding timestamp memory, the driver must read the timestamps.
However, naively doing this as part of ice_ptp_reset_ts_memory() is
problematic. When reading the timestamp index, hardware kicks off a chain
of actions including clearing the ready bitmap index, and decrementing an
internal counter if the timestamp index was marked as valid.
This can potentially leave the internal hardware counter out of sync with
the actual number of timestamps. This occurs because the
PHY_TSTAMP memory region containing the timestamp data is not
zero-initialized when the devices boots up. Instead, it is filled with
garbage. On a cold power on, attempts to read the stale data result in the
hardware triggering a counter decrement for a timestamp that never
happened. This underflows the counter, and prevents new timestamp
interrupts from being triggered for real timestamp requests.
In normal operation this is "fine" because the driver should never read
from the PHY_TSTAMP region without first checking if the
PHY_REG_TX_MEMORY_STATUS bit for a given index is set. However, driver bugs
or programmer error could result in accidentally reading this data.
Attempting to write to the PHY_TSTAMP region is also ineffective as the
registers are read only. Instead, reading a register triggers PHY action to
clear its valid bit. However, the PHY also performs other actions such as
clearing the bit in the PHY_REG_TX_MEMORY_STATUS and decrementing the
internal counter.
We must read the PHY_TSTAMP memory in order to clear stale timestamps. But
doing so may cause a desync with the counter. To prevent issues, perform
this clearing always and only right before initiating a PHY soft reset.
The soft reset will clear and reset the internal counter and the
PHY_REG_TX_MEMORY_STATUS ready bitmap. The reads to PHY_TSTAMP reagion will
reset the region valid bits ensuring that no stale data is left behind.
This combination ensures that we always have a clean slate with no stale
data and with the counter properly reset to zero.
If a read fails during the sweep (i.e. due to a transient sideband queue
failure) it is not treated as a fatal error. There is already a dynamic
debug message for each read failure. Keep track of the total number of
timestamp registers that fail for a given port and log a warning message
indicating the total number of failures. Continuing is acceptable as
clearing the memory is a precaution against faulty software. Correct
software access won't read the index twice and won't read it at all except
when it has already confirmed that the index is valid by reading
ice_get_phy_tx_tstamp_ready(), at which point the hardware will have
overwritten any stale data.
To help clarify behavior should the driver fail to access the PHY over
sideband queue, also document how callers should treat failures from
ice_ptp_phy_soft_reset_eth56g(). This is especially important since an
early failure to begin the soft reset after the PHY timestamp sweep may
result in the PHY being in a state where the PHY counter is out-of-sync.
Such access failures are expected to be rare and likely indicate a much
bigger problem with access to the sideband queue. The driver already treats
these errors as either fatal at driver load, or as an indication that the
timestamping block is broken, and must be restarted by a link reset. Upon a
link reset, the driver will re-sweep the timestamp indexes and perform a
fresh soft reset, clearing any inconsistency from the left over failed
state.
Since ice_ptp_reset_ts_memory now only works for E82X hardware, update its
kdoc comment to clarify what hardware is skipped and note specifically why
E825 is skipped.
Fixes: 3ec46e157c7f ("ice: perform PHY soft reset for E825C ports at initialization")
Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 118 +++++++++++++++-------------
1 file changed, 65 insertions(+), 53 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 07b55fbb88dd..5c63f88bbf48 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -737,24 +737,6 @@ static int ice_read_port_mem_eth56g(struct ice_hw *hw, u8 port, u16 offset,
return ice_read_port_eth56g(hw, port, offset, val, ETH56G_PHY_MEM_PTP);
}
-/**
- * ice_write_port_mem_eth56g - Write a PHY port memory location
- * @hw: pointer to the HW struct
- * @port: Port number to be read
- * @offset: Offset from PHY port register base
- * @val: Pointer to the value to read (out param)
- *
- * Return:
- * * %0 - success
- * * %EINVAL - invalid port number or resource type
- * * %other - failed to write to PHY
- */
-static int ice_write_port_mem_eth56g(struct ice_hw *hw, u8 port, u16 offset,
- u32 val)
-{
- return ice_write_port_eth56g(hw, port, offset, val, ETH56G_PHY_MEM_PTP);
-}
-
/**
* ice_write_quad_ptp_reg_eth56g - Write a PHY quad register
* @hw: pointer to the HW struct
@@ -1129,47 +1111,35 @@ static int ice_read_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx,
}
/**
- * ice_clear_ptp_tstamp_eth56g - Clear a timestamp from the quad block
+ * ice_clear_ptp_tstamp_eth56g - Clear a timestamp from PHY memory
* @hw: pointer to the HW struct
- * @port: the quad to read from
+ * @port: the port to read from
* @idx: the timestamp index to reset
*
- * Read and then forcibly clear the timestamp index to ensure the valid bit is
- * cleared and the timestamp status bit is reset in the PHY port memory of
- * internal PHYs of the 56G devices.
+ * Read the timestamp index to ensure that the valid bit is cleared and the
+ * timestamp status bit is reset in the PHY port memory.
*
- * To directly clear the contents of the timestamp block entirely, discarding
- * all timestamp data at once, software should instead use
- * ice_ptp_reset_ts_memory_quad_eth56g().
- *
- * This function should only be called on an idx whose bit is set according to
- * ice_get_phy_tx_tstamp_ready().
+ * This function should only be called on an index whose bit is set according
+ * to ice_get_phy_tx_tstamp_ready(), or as part of a full sweep when paired
+ * with a PHY soft reset via ice_ptp_phy_soft_reset_eth56g().
*
* Return:
* * %0 - success
- * * %other - failed to write to PHY
+ * * %other - failed to read from PHY
*/
static int ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx)
{
u64 unused_tstamp;
- u16 lo_addr;
int err;
- /* Read the timestamp register to ensure the timestamp status bit is
- * cleared.
+ /* Per the PHY spec, reading the timestamp memory location is what
+ * clears the entry's valid bit and its corresponding (read-only)
+ * ts_memory_status bit.
*/
err = ice_read_ptp_tstamp_eth56g(hw, port, idx, &unused_tstamp);
if (err) {
ice_debug(hw, ICE_DBG_PTP, "Failed to read the PHY timestamp register for port %u, idx %u, err %d\n",
port, idx, err);
- }
-
- lo_addr = (u16)PHY_TSTAMP_L(idx);
-
- err = ice_write_port_mem_eth56g(hw, port, lo_addr, 0);
- if (err) {
- ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register for port %u, idx %u, err %d\n",
- port, idx, err);
return err;
}
@@ -1177,19 +1147,43 @@ static int ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx)
}
/**
- * ice_ptp_reset_ts_memory_eth56g - Clear all timestamps from the port block
+ * ice_ptp_clear_tx_memory_status_eth56g - Reset one port's Tx timestamp memory
* @hw: pointer to the HW struct
+ * @port: port number to clear
+ *
+ * Fully reset a single PHY port's Tx timestamp memory. Per the PHY spec, the
+ * only way to clear a timestamp valid bit (and its read-only ts_memory_status
+ * bit) is to read the timestamp memory location, so read every entry for the
+ * port (two 32-bit reads each). This discards all timestamp data on the port,
+ * so it must only be used for a full reset; callers that must preserve
+ * in-flight timestamps clear individual indices via ice_clear_phy_tstamp().
+ *
+ * Due to interactions with an internal HW counter for the number of
+ * outstanding Tx timestamps, this *must* not be called outside if the
+ * ice_ptp_phy_soft_reset_eth56g() procedure. Otherwise, the internal counter
+ * may become out of sync and prevent new timestamp interrupts.
+ *
+ * Failure to read a given index (i.e. due to a transient sideband queue
+ * failure) is not considered fatal as the PHY port is about to be soft reset.
+ * While the soft reset does not clear the timestamp memory, software
+ * shouldn't be reading the timestamp memory without already knowing it is
+ * valid via ice_get_phy_tx_tstamp_ready(), and this sweep is just
+ * a precaution to ensure the memory is in a known state.
*/
-static void ice_ptp_reset_ts_memory_eth56g(struct ice_hw *hw)
+static void ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port)
{
- unsigned int port;
+ int err, failed = 0;
+ u8 idx;
- for (port = 0; port < hw->ptp.num_lports; port++) {
- ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_MEMORY_STATUS_L,
- 0);
- ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TX_MEMORY_STATUS_U,
- 0);
+ for (idx = 0; idx < INDEX_PER_PORT; idx++) {
+ err = ice_clear_ptp_tstamp_eth56g(hw, port, idx);
+ if (err)
+ failed++;
}
+
+ if (failed)
+ dev_warn(ice_hw_to_dev(hw), "Failed to clear %d PHY timestamp registers for port %u\n",
+ failed, port);
}
/**
@@ -2295,6 +2289,7 @@ int ice_ptp_read_tx_hwtstamp_status_eth56g(struct ice_hw *hw, u32 *ts_status)
*
* Trigger a soft reset of the ETH56G PHY by toggling the soft reset
* bit in the PHY global register. The reset sequence consists of:
+ * 0. Reading every timestamp memory register to clear its valid bit
* 1. Clearing the soft reset bit
* 2. Asserting the soft reset bit
* 3. Clearing the soft reset bit again
@@ -2303,6 +2298,20 @@ int ice_ptp_read_tx_hwtstamp_status_eth56g(struct ice_hw *hw, u32 *ts_status)
* to settle. This provides a controlled way to reinitialize the PHY
* without requiring a full device reset.
*
+ * To ensure that the internal counter matches the contents of the
+ * PHY_TSTAMP memory region, read every timestamp index prior to performing
+ * the soft reset. The read side effects ensure that the timestamp memory is
+ * cleared while the soft reset ensures that the PHY_REG_TX_MEMORY_STATUS and
+ * internal counter are reset to zero.
+ *
+ * In the event that the driver fails to access the PHY during the completion
+ * of the soft reset, (e.g. due to sideband queue message failures), the
+ * device may be left in an inconsistent state. Such failures are expected to
+ * be rare. Callers are responsible for checking the return of this function
+ * and treating errors as indicating that the timestamping block of the PHY is
+ * non-functional. Recovery may be possible by issuing another soft reset
+ * after any transient access errors have abated.
+ *
* Return: 0 on success, or a negative error code on failure when
* reading or writing the PHY register.
*/
@@ -2311,6 +2320,8 @@ int ice_ptp_phy_soft_reset_eth56g(struct ice_hw *hw, u8 port)
u32 global_val;
int err;
+ ice_ptp_clear_tx_memory_status_eth56g(hw, port);
+
err = ice_read_ptp_reg_eth56g(hw, port, PHY_REG_GLOBAL, &global_val);
if (err) {
ice_debug(hw, ICE_DBG_PTP, "Failed to read PHY_REG_GLOBAL for port %d, err %d\n",
@@ -5794,6 +5805,11 @@ static int ice_get_pf_c827_idx(struct ice_hw *hw, u8 *idx)
/**
* ice_ptp_reset_ts_memory - Reset timestamp memory for all blocks
* @hw: pointer to the HW struct
+ *
+ * Only E82X hardware can directly reset memory for all blocks. E825 hardware
+ * can only safely reset timestamp memory as part of the soft reset, so is not
+ * reset here. E810 and E830 also do not implement direct reset of the
+ * timestamp memory region.
*/
void ice_ptp_reset_ts_memory(struct ice_hw *hw)
{
@@ -5801,10 +5817,6 @@ void ice_ptp_reset_ts_memory(struct ice_hw *hw)
case ICE_MAC_GENERIC:
ice_ptp_reset_ts_memory_e82x(hw);
break;
- case ICE_MAC_GENERIC_3K_E825:
- ice_ptp_reset_ts_memory_eth56g(hw);
- break;
- case ICE_MAC_E810:
default:
return;
}
--
2.56.0.215.g47fdf2208c5b
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net 6/6] ice: skip reading Tx ready bitmap on ports with no timestamps
2026-10-28 0:05 [PATCH net 0/6] ice: E82x: reduced timestamp processing logic fixes Jacob Keller
` (4 preceding siblings ...)
2026-10-28 0:05 ` [PATCH net 5/6] ice: E825: clear PHY_TSTAMP memory prior to soft reset Jacob Keller
@ 2026-10-28 0:05 ` Jacob Keller
2026-10-28 0:10 ` [PATCH net 0/6] ice: E82x: reduced timestamp processing logic fixes netdev-bot+sinfo
2026-10-29 1:20 ` patchwork-bot+netdevbpf
7 siblings, 0 replies; 13+ messages in thread
From: Jacob Keller @ 2026-10-28 0:05 UTC (permalink / raw)
To: Przemek Kitszel, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Petr Oros, netdev
Cc: Grzegorz Nitka, Aleksandr Loktionov, Alexander Nowlin,
Tony Nguyen, jacob.e.keller, maciej.machnikowski,
przemyslaw.korba, sergey.temerkhanov, arkadiusz.kubalewski,
richardcochran, horms
On E82x devices, the interrupt for Tx timestamps are handled by the clock
owner. When an interrupt with the Tx timestamp cause is fired, the clock
owner PF iterates the list of ports and checks for timestamps across all
ports.
The existing logic reads the PHY timestamp ready bitmap before iterating
the list of in-use timestamp indexes, even for ports which have no
timestamps waiting in the software timestamp tracker. This has a
significant and measurable latency impact on reporting Tx timestamps.
Check the bitmap and exit early in the event that there are no timestamps
waiting on a port. Observant reviewers may notice that the check is done
without acquiring the lock. This is fine, as whether the thread sees or
fails to see a new outstanding timestamp does not affect correctness, only
determining whether or not it should do extra work.
Skipping the check has the highest impact on E822 an E825 devices which
iterate all ports in a single thread, but it is applied universally to all
device types since the extra read is unnecessary regardless.
The average latency of a Tx timestamp is impacted by several factors
including system load, the number of timestamp requests, and some random
factors that are difficult to control. However for comparison on my system
with these changes, while operating ptp4l on a single port with a sync rate
of 16/second:
Before: mean 317.43 microseconds, standard deviation 34.51
After: mean 189.71 microseconds, standard deviation 43.24
Of course this level of improvement may not be indicative of a production
setup as one might expect timestamps to be operating out of multiple ports
on the device. However, even in that case an improvement is still likely
depending on the actual timestamp rates.
Fixes: d938a8cca88a ("ice: Auxbus devices & driver for E822 TS")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
drivers/net/ethernet/intel/ice/ice_ptp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 3ba6632bb196..ecfde5bbca9f 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -572,7 +572,7 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
pf = ptp_port_to_pf(ptp_port);
hw = &pf->hw;
- if (!tx->init)
+ if (!tx->init || bitmap_empty(tx->in_use, tx->len))
return;
/* Read the Tx ready status first */
--
2.56.0.215.g47fdf2208c5b
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH net 0/6] ice: E82x: reduced timestamp processing logic fixes
2026-10-28 0:05 [PATCH net 0/6] ice: E82x: reduced timestamp processing logic fixes Jacob Keller
` (5 preceding siblings ...)
2026-10-28 0:05 ` [PATCH net 6/6] ice: skip reading Tx ready bitmap on ports with no timestamps Jacob Keller
@ 2026-10-28 0:10 ` netdev-bot+sinfo
2026-10-29 1:20 ` patchwork-bot+netdevbpf
7 siblings, 0 replies; 13+ messages in thread
From: netdev-bot+sinfo @ 2026-10-28 0:10 UTC (permalink / raw)
To: Jacob Keller
Cc: Przemek Kitszel, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Petr Oros, netdev, Grzegorz Nitka,
Aleksandr Loktionov, Alexander Nowlin, Tony Nguyen,
maciej.machnikowski, przemyslaw.korba, sergey.temerkhanov,
arkadiusz.kubalewski, richardcochran, horms
Hi!
This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:
- Whether the issue was actually triggered, or is only theoretical
(e.g. found by code inspection). If it was triggered please include
the symptoms, like the stack trace or error messages.
- What hardware the change was tested on. For driver fixes please
mention the device (and if relevant firmware version) used for
testing, or say that the change was not tested on real hardware.
Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.
The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net 4/6] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY
2026-10-28 0:05 ` [PATCH net 4/6] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY Jacob Keller
@ 2026-10-29 0:53 ` netdev-bot+sashiko
2026-10-29 1:08 ` Jakub Kicinski
0 siblings, 1 reply; 13+ messages in thread
From: netdev-bot+sashiko @ 2026-10-29 0:53 UTC (permalink / raw)
To: jacob.e.keller
Cc: przemyslaw.kitszel, andrew+netdev, davem, edumazet, kuba, pabeni,
poros, netdev, grzegorz.nitka, aleksandr.loktionov,
alexander.nowlin, anthony.l.nguyen, maciej.machnikowski,
przemyslaw.korba, sergey.temerkhanov, arkadiusz.kubalewski,
richardcochran, horms
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 0 · 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] ice_ptp_port_phy_restart()
(drivers/net/ethernet/intel/ice/ice_ptp.c) now returns from the…
--
ice: E825: stop clearing PHY_REG_TX_OFFSET_READY
On E825, ice_stop_phy_timer_eth56g() no longer clears
PHY_REG_TX_OFFSET_READY. Clearing it left the hardware's outstanding Tx
timestamp counter out of sync. New Tx timestamp requests are now blocked
through the tracker's tx.disabled flag while the PHY is stopped or
restarting. The patch also adds a tx->init check in
ice_ptp_mark_tx_tracker_stale(), and changes the error messages in
ice_ptp_port_phy_restart().
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 2f8563eb6fb19..3ba6632bb1965 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
[ ... ]
> @@ -1305,15 +1311,35 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
[ ... ]
> break;
> default:
> - err = -ENODEV;
> + dev_dbg(ice_pf_to_dev(pf), "PTP failed to restart PHY port %u with unknown MAC type %d\n",
> + port, hw->mac_type);
> + return -ENODEV;
> }
>
> if (err)
> - dev_err(ice_pf_to_dev(pf), "PTP failed to set PHY port %d up, err %d\n",
> - port, err);
> + dev_err(ice_pf_to_dev(pf), "PTP failed to restart PHY port %u with err %pe; Timestamping not enabled; A link-toggle may recover.\n",
> + port, ERR_PTR(err));
>
> mutex_unlock(&ptp_port->ps_lock);
[Severity: High]
Does this new return leave ptp_port->ps_lock held?
ice_ptp_port_phy_restart() takes the mutex before the switch:
mutex_lock(&ptp_port->ps_lock);
switch (hw->mac_type) {
Before this patch, the default arm set err = -ENODEV and fell through to
the mutex_unlock() at the end. The new return -ENODEV skips that unlock.
After that, any ice_ptp_port_phy_stop() or ice_ptp_port_phy_restart() on
the same port would block forever in mutex_lock(). That includes the
ice_ptp_release() -> ice_ptp_port_phy_stop() teardown path.
The default arm in ice_ptp_port_phy_stop() still uses err = -ENODEV and
reaches its unlock.
The commit message only describes this hunk as a logging change:
"For the invalid MAC type path returning -ENODEV, skip this message and
log a dev_dbg that we failed with an unknown MAC type instead."
The current tree does not seem to hit this path at runtime.
ice_ptp_init_port() rejects unknown MAC types before
ice_ptp_reset_phy_timestamping() runs. However, the path becomes
reachable if a MAC type is ever added to ice_ptp_init_port() but not
here.
The last patch in the series ("ice: skip reading Tx ready bitmap on
ports with no timestamps") still has the same return with the lock held.
Could the default arm call mutex_unlock() before returning? Another
option is to set err and jump to a label that skips the dev_err() but
still unlocks.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027-reduction-v1-0-81237c67a361%40intel.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net 5/6] ice: E825: clear PHY_TSTAMP memory prior to soft reset
2026-10-28 0:05 ` [PATCH net 5/6] ice: E825: clear PHY_TSTAMP memory prior to soft reset Jacob Keller
@ 2026-10-29 0:53 ` netdev-bot+sashiko
0 siblings, 0 replies; 13+ messages in thread
From: netdev-bot+sashiko @ 2026-10-29 0:53 UTC (permalink / raw)
To: jacob.e.keller
Cc: przemyslaw.kitszel, andrew+netdev, davem, edumazet, kuba, pabeni,
poros, netdev, grzegorz.nitka, aleksandr.loktionov,
alexander.nowlin, anthony.l.nguyen, maciej.machnikowski,
przemyslaw.korba, sergey.temerkhanov, arkadiusz.kubalewski,
richardcochran, horms
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 1
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).
- [Medium] In ice_ptp_phy_soft_reset_eth56g()
(drivers/net/ethernet/intel/ice/ice_ptp_hw.c), the patch adds an…
- [Low] The new kdoc for ice_ptp_clear_tx_memory_status_eth56g() in
drivers/net/ethernet/intel/ice/ice_ptp_hw.c has a typo in the sentence…
--
ice: E825: clear PHY_TSTAMP memory prior to soft reset
This patch drops the E825 path that cleared Tx timestamp memory by writing
PHY_REG_TX_MEMORY_STATUS. Instead, ice_ptp_phy_soft_reset_eth56g() now
first reads every PHY_TSTAMP index for the port, then runs the soft reset,
so stale valid bits are cleared and the internal counter is reset. The
kdoc for ice_ptp_reset_ts_memory() and ice_ptp_phy_soft_reset_eth56g() is
updated to match.
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index 07b55fbb88dde..5c63f88bbf48a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
[ ... ]
> @@ -1177,19 +1147,43 @@ static int ice_clear_ptp_tstamp_eth56g(struct ice_hw *hw, u8 port, u8 idx)
> }
>
> /**
> - * ice_ptp_reset_ts_memory_eth56g - Clear all timestamps from the port block
> + * ice_ptp_clear_tx_memory_status_eth56g - Reset one port's Tx timestamp memory
> * @hw: pointer to the HW struct
[ ... ]
> + * Due to interactions with an internal HW counter for the number of
> + * outstanding Tx timestamps, this *must* not be called outside if the
> + * ice_ptp_phy_soft_reset_eth56g() procedure. Otherwise, the internal counter
> + * may become out of sync and prevent new timestamp interrupts.
[Severity: Low]
This isn't a bug, but there's a typo in this kdoc: "outside if the"
should be "outside of the".
[ ... ]
> @@ -2303,6 +2298,20 @@ int ice_ptp_read_tx_hwtstamp_status_eth56g(struct ice_hw *hw, u32 *ts_status)
[ ... ]
> + * In the event that the driver fails to access the PHY during the completion
> + * of the soft reset, (e.g. due to sideband queue message failures), the
> + * device may be left in an inconsistent state. Such failures are expected to
> + * be rare. Callers are responsible for checking the return of this function
> + * and treating errors as indicating that the timestamping block of the PHY is
> + * non-functional. Recovery may be possible by issuing another soft reset
> + * after any transient access errors have abated.
> + *
> * Return: 0 on success, or a negative error code on failure when
> * reading or writing the PHY register.
> */
> @@ -2311,6 +2320,8 @@ int ice_ptp_phy_soft_reset_eth56g(struct ice_hw *hw, u8 port)
> u32 global_val;
> int err;
>
> + ice_ptp_clear_tx_memory_status_eth56g(hw, port);
> +
> err = ice_read_ptp_reg_eth56g(hw, port, PHY_REG_GLOBAL, &global_val);
> if (err) {
> ice_debug(hw, ICE_DBG_PTP, "Failed to read PHY_REG_GLOBAL for port %d, err %d\n",
[Severity: Medium]
Suppose the sweep finishes, and then the ice_read_ptp_reg_eth56g() of
PHY_REG_GLOBAL fails, or one of the three ice_write_ptp_reg_eth56g() calls
to PHY_REG_GLOBAL fails. The function returns early and the soft reset bit
is never toggled.
The commit message says each read of a valid entry, including cold boot
garbage, decrements the internal counter. Only the soft reset puts the
counter back to zero.
Can this leave the counter underflowed, with no Tx timestamp interrupts?
That is the condition this patch tries to prevent. Before this patch, a
failed soft reset did no PHY_TSTAMP reads.
The commit message says recovery happens on a link reset:
"Upon a link reset, the driver will re-sweep the timestamp indexes and
perform a fresh soft reset, clearing any inconsistency from the left
over failed state."
The new kdoc also says another soft reset may recover.
Is there a code path that does this? The only caller of
ice_ptp_phy_soft_reset_eth56g() is ice_ptp_init_phc_e825c(). That is
reached only from ice_ptp_init_owner() and ice_ptp_rebuild_owner(), through
ice_ptp_init_phc().
On a link change, ice_ptp_link_change()->ice_ptp_port_phy_restart() does
this for E825:
case ICE_MAC_GENERIC_3K_E825:
...
ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
err = ice_start_phy_timer_eth56g(hw, port);
This does not sweep and does not soft reset. Also, ice_ptp_link_change()
returns early when pf->ptp.state != ICE_PTP_READY. That is the case after
a failed init or rebuild.
After a failed rebuild the state is ICE_PTP_ERROR, and later calls to
ice_ptp_rebuild() exit here:
} else if (ptp->state != ICE_PTP_RESETTING) {
err = -EINVAL;
dev_err(ice_pf_to_dev(pf), "PTP was not initialized\n");
goto err;
}
So ice_ptp_rebuild_owner() is never retried. It looks like only a driver
reload would run the sweep and soft reset again.
Separately, ice_ptp_init_phc_e825c() returns on the first port that fails:
for (int port = 0; port < hw->ptp.num_lports; port++) {
err = ice_ptp_phy_soft_reset_eth56g(hw, port);
if (err) {
...
return err;
}
}
The ports after it are neither swept nor reset in that pass.
Should the commit message and kdoc say a driver reload is needed? Or
should a real retry be added, for example by checking PHY_REG_GLOBAL access
before the sweep, or by retrying the reset when a failure happens after the
sweep?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027-reduction-v1-0-81237c67a361%40intel.com
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net 4/6] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY
2026-10-29 0:53 ` netdev-bot+sashiko
@ 2026-10-29 1:08 ` Jakub Kicinski
2026-10-29 16:52 ` Jacob Keller
0 siblings, 1 reply; 13+ messages in thread
From: Jakub Kicinski @ 2026-10-29 1:08 UTC (permalink / raw)
To: netdev-bot+sashiko
Cc: jacob.e.keller, przemyslaw.kitszel, andrew+netdev, davem,
edumazet, pabeni, poros, netdev, grzegorz.nitka,
aleksandr.loktionov, alexander.nowlin, anthony.l.nguyen,
maciej.machnikowski, przemyslaw.korba, sergey.temerkhanov,
arkadiusz.kubalewski, richardcochran, horms
On Thu, 29 Oct 2026 00:53:35 +0000 netdev-bot+sashiko@kernel.org wrote:
> - [High] ice_ptp_port_phy_restart()
> (drivers/net/ethernet/intel/ice/ice_ptp.c) now returns from the…
Looks like this is because of the patch droppage.
Let me take the first 3.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net 0/6] ice: E82x: reduced timestamp processing logic fixes
2026-10-28 0:05 [PATCH net 0/6] ice: E82x: reduced timestamp processing logic fixes Jacob Keller
` (6 preceding siblings ...)
2026-10-28 0:10 ` [PATCH net 0/6] ice: E82x: reduced timestamp processing logic fixes netdev-bot+sinfo
@ 2026-10-29 1:20 ` patchwork-bot+netdevbpf
7 siblings, 0 replies; 13+ messages in thread
From: patchwork-bot+netdevbpf @ 2026-10-29 1:20 UTC (permalink / raw)
To: Jacob Keller
Cc: przemyslaw.kitszel, andrew+netdev, davem, edumazet, kuba, pabeni,
poros, netdev, grzegorz.nitka, aleksandr.loktionov,
alexander.nowlin, anthony.l.nguyen, maciej.machnikowski,
przemyslaw.korba, sergey.temerkhanov, arkadiusz.kubalewski,
richardcochran, horms
Hello:
This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Tue, 27 Oct 2026 17:05:36 -0700 you wrote:
> Jake Keller says:
>
> This is a reduced set patches originally posted as part of [1] which
> previously passes Sashiko review without comments. It does not completely
> resolve the outstanding issues that we had, but can hopefully unblock some
> progress.
>
> [...]
Here is the summary with links:
- [net,1/6] ice: use reference counting and SRCU for PTP port access
https://git.kernel.org/netdev/net/c/d5d859a6efff
- [net,2/6] ice: set in_use only after preparing Tx timestamp index
https://git.kernel.org/netdev/net/c/50870d621160
- [net,3/6] ice: E822: keep Tx timestamps disabled during offset calibration
https://git.kernel.org/netdev/net/c/56af436b90ac
- [net,4/6] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY
(no matching commit)
- [net,5/6] ice: E825: clear PHY_TSTAMP memory prior to soft reset
(no matching commit)
- [net,6/6] ice: skip reading Tx ready bitmap on ports with no timestamps
(no matching commit)
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net 4/6] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY
2026-10-29 1:08 ` Jakub Kicinski
@ 2026-10-29 16:52 ` Jacob Keller
0 siblings, 0 replies; 13+ messages in thread
From: Jacob Keller @ 2026-10-29 16:52 UTC (permalink / raw)
To: Jakub Kicinski, netdev-bot+sashiko
Cc: przemyslaw.kitszel, andrew+netdev, davem, edumazet, pabeni, poros,
netdev, grzegorz.nitka, aleksandr.loktionov, alexander.nowlin,
anthony.l.nguyen, maciej.machnikowski, przemyslaw.korba,
sergey.temerkhanov, arkadiusz.kubalewski, richardcochran, horms
On 10/28/2026 6:08 PM, Jakub Kicinski wrote:
> On Thu, 29 Oct 2026 00:53:35 +0000 netdev-bot+sashiko@kernel.org wrote:
>> - [High] ice_ptp_port_phy_restart()
>> (drivers/net/ethernet/intel/ice/ice_ptp.c) now returns from the…
>
> Looks like this is because of the patch droppage.
> Let me take the first 3.
Ahh.. ya. At least it caught it. That's how the original series grew so
big :\ Little bits I kept digging at that created dependencies like this.
Thanks for pulling the first 3. I'm going to rework the remaining bits
and try to get another smaller batch that includes the ones some
stakeholders are most requesting ready.
Thanks,
Jake
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2026-10-29 16:52 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-28 0:05 [PATCH net 0/6] ice: E82x: reduced timestamp processing logic fixes Jacob Keller
2026-10-28 0:05 ` [PATCH net 1/6] ice: use reference counting and SRCU for PTP port access Jacob Keller
2026-10-28 0:05 ` [PATCH net 2/6] ice: set in_use only after preparing Tx timestamp index Jacob Keller
2026-10-28 0:05 ` [PATCH net 3/6] ice: E822: keep Tx timestamps disabled during offset calibration Jacob Keller
2026-10-28 0:05 ` [PATCH net 4/6] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY Jacob Keller
2026-10-29 0:53 ` netdev-bot+sashiko
2026-10-29 1:08 ` Jakub Kicinski
2026-10-29 16:52 ` Jacob Keller
2026-10-28 0:05 ` [PATCH net 5/6] ice: E825: clear PHY_TSTAMP memory prior to soft reset Jacob Keller
2026-10-29 0:53 ` netdev-bot+sashiko
2026-10-28 0:05 ` [PATCH net 6/6] ice: skip reading Tx ready bitmap on ports with no timestamps Jacob Keller
2026-10-28 0:10 ` [PATCH net 0/6] ice: E82x: reduced timestamp processing logic fixes netdev-bot+sinfo
2026-10-29 1:20 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox