> -----Original Message-----
> From: Korba, Przemyslaw
> Sent: Thursday, July 23, 2026 3:16 PM
> To: 'Petr Oros' <[email protected]>; [email protected]
> Cc: [email protected]; Kubalewski, Arkadiusz
> <[email protected]>; Loktionov, Aleksandr
> <[email protected]>; Nguyen, Anthony L
> <[email protected]>; Kitszel, Przemyslaw
> <[email protected]>;
> Ilichev, Konstantin <[email protected]>
> Subject: RE: [Intel-wired-lan] [PATCH iwl-net 2/4] ice: clear unexpected Tx
> timestamp ready bits to prevent stuck PHY
>
>
>
>
> > -----Original Message-----
> > From: Petr Oros <[email protected]>
> > Sent: Thursday, July 23, 2026 10:27 AM
> > To: Korba, Przemyslaw <[email protected]>;
> > [email protected]
> > Cc: [email protected]; Kubalewski, Arkadiusz
> > <[email protected]>; Loktionov, Aleksandr
> > <[email protected]>; Nguyen, Anthony L
> > <[email protected]>; Kitszel, Przemyslaw
> > <[email protected]>;
> > Ilichev, Konstantin <[email protected]>
> > Subject: Re: [Intel-wired-lan] [PATCH iwl-net 2/4] ice: clear unexpected Tx
> > timestamp ready bits to prevent stuck PHY
> >
> >
> > On 7/20/26 14:01, Przemyslaw Korba wrote:
> > > From: Jacob Keller <[email protected]>
> > >
> > > Clear orphaned timestamp ready bits left in the PHY when a packet is
> > > timestamped just as the link goes down. The driver clears its software
> > > in_use bits during link-down cleanup, but the PHY has already latched
> > > the timestamp, so on the next link-up a stale ready bit from the
> > > previous link cycle remains set with no matching in_use entry.
> > >
> > > The PHY timestamp interrupt logic will not generate a new interrupt
> > > until ALL outstanding ready bits have been read. ice_ptp_process_tx_
> > > tstamp() only iterates slots set in the software in_use bitmap, so it
> > > never reads these orphaned slots. The result is a permanent interrupt
> > > deadlock:
> > >
> > > 1. The PHY has ready bits set for slots with no in_use tracker entry
> > > 2. The driver never reads those slots because in_use is clear
> > > 3. The PHY refuses to generate new timestamp interrupts
> > > 4. All future Tx timestamps permanently fail
> > > 5. Only a power-on reset can recover the device
> > >
> > > Clear these stale ready bits on link-up, in ice_ptp_link_change(),
> > > before any new timestamp requests arrive, scoped to the affected port
> > > only so timestamps still pending on other ports are left untouched. For
> > > every affected MAC the clearing iterates the not-in_use slots with
> > > for_each_clear_bit(), so a pending software request is never discarded
> > > and tx->lock need not be held across the PHY access:
> > >
> > > - E810 is skipped (guarded by tx->has_ready_bitmap) because
> > > ice_get_phy_tx_tstamp_ready_e810() returns an all-ones bitmap rather
> > > than a real ready status.
> > > - E830 and E825/eth56g read the real Tx timestamp ready bitmap and
> > > clear only the slots the PHY actually latched, via
> > > ice_clear_phy_tstamp(). If the ready bitmap cannot be read, the
> > > clear is skipped and a rate-limited warning is logged.
> > >
> > > On every MAC the ready bit is cleared by reading the slot's timestamp
> > > memory; the orphaned bits linger only because the normal processing path
> > > never reads those not-in_use slots. The link-up recovery clears them by
> > > reading the timestamp memory via ice_clear_phy_tstamp():
> > >
> > > - E830: add ice_clear_phy_tstamp_e830(), which reads the
> > > PRTTSYN_TXTIME_H/L registers to clear the entry, and add an
> > > ICE_MAC_E830 case to the ice_clear_phy_tstamp() dispatch.
> > > - eth56g: ice_clear_ptp_tstamp_eth56g() reads the timestamp memory
> > > location, which per the PHY spec is the operation that clears the
> > > entry's valid bit and its corresponding (read-only) ts_memory_status
> > > bit. The ts_memory_status registers cannot be written to clear a
> > > bit, so only reading the timestamp memory has any effect.
> > >
> > > The new for_each_clear_bit() loop runs from the service task via link
> > > events. During device removal, tx->in_use is freed before the service
> > > task is stopped. Fix by stopping the service task at the start of
> > > ice_unload(), before ice_ptp_release() frees tx->in_use.
> > >
> > > Reviewed-by: Grzegorz Nitka <[email protected]>
> > > Signed-off-by: Jacob Keller <[email protected]>
> > > Signed-off-by: Arkadiusz Kubalewski <[email protected]>
> > > Signed-off-by: Przemyslaw Korba <[email protected]>
> > > ---
> > > drivers/net/ethernet/intel/ice/ice_main.c | 1 +
> > > drivers/net/ethernet/intel/ice/ice_ptp.c | 72 +++++++++++++++--
> > > drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 89 +++++++++++++++++----
> > > drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 1 +
> > > drivers/net/ethernet/intel/ice/ice_type.h | 7 ++
> > > 5 files changed, 149 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
> > > b/drivers/net/ethernet/intel/ice/ice_main.c
> > > index e3d3810c791f..231d533309cb 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_main.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> > > @@ -5169,6 +5169,7 @@ void ice_unload(struct ice_pf *pf)
> > >
> > > devl_assert_locked(priv_to_devlink(pf));
> > >
> > > + ice_service_task_stop(pf);
> > > ice_unplug_aux_dev(pf);
> > > ice_deinit_rdma(pf);
> > > ice_deinit_features(pf);
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
> > > b/drivers/net/ethernet/intel/ice/ice_ptp.c
> > > index 1aa440b0639f..9d9d9958fe5c 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> > > @@ -1372,15 +1372,73 @@ void ice_ptp_link_change(struct ice_pf *pf, bool
> > > linkup)
> > > switch (hw->mac_type) {
> > > case ICE_MAC_E810:
> > > case ICE_MAC_E830:
> > > - /* Do not reconfigure E810 or E830 PHY */
> > > + case ICE_MAC_GENERIC_3K_E825:
> > > + /* Do not reconfigure E810 or E830 PHY, but on link-up clear
> > > + * any stale timestamp ready bits left over from a previous
> > > + * link cycle. The PHY may have latched timestamps for packets
> > > + * in flight when the link went down; these must be cleared
> > > + * before new timestamp requests arrive.
> > > + *
> > > + * E810 does not have a real ready bitmap
> > > + * (ice_get_phy_tx_tstamp_ready_e810 returns all-ones), so
> > > + * skip clearing on E810 to avoid unnecessary sideband queue
> > > + * operations for every not-in-use slot on each link-up.
> > > + */
> > > + if (linkup && ptp_port->tx.has_ready_bitmap) {
> > > + struct ice_ptp_tx *tx = &ptp_port->tx;
> > > + u64 tstamp_ready;
> > > + int i;
> > > +
> > > + if (ice_get_phy_tx_tstamp_ready(hw, tx->block,
> > > + &tstamp_ready)) {
> > > + dev_warn_ratelimited(ice_pf_to_dev(pf),
> > > + "PTP failed to read Tx
> > > timestamp ready bitmap on link-up; stale PHY timestamps may
> > remain and stall Tx timestamping\n");
> > > + } else {
> > > + /* Only clear stale ready bits for slots that
> > > + * have no in-flight software request. Iterating
> > > + * the not-in-use slots skips any concurrent
> > > + * ice_ptp_request_ts() allocation without
> > > + * holding tx->lock across the PHY access. E830
> > > + * and E825 reach this clear; E810 is filtered
> > > + * out above by has_ready_bitmap.
> > > + */
> > > + for_each_clear_bit(i, tx->in_use, tx->len) {
> > > + u8 phy_idx = i + tx->offset;
> > > +
> > > + if (tstamp_ready & BIT_ULL(phy_idx))
> > > + ice_clear_phy_tstamp(hw,
> > > + tx->block,
> > > + phy_idx);
> > > + }
> > > + }
> > > + }
> > > +
> > > + /* E810 and E830 need no further PHY reconfiguration */
> > > + if (hw->mac_type != ICE_MAC_GENERIC_3K_E825)
> > > + return;
> > > +
> > > + /* E825 recovers its Tx path by soft resetting the PHY
> > > + * timestamp block and restarting the port, but only on
> > > + * link-up. The reset is a three-step register toggle; if it
> > > + * fails partway through, the port can be left held in reset,
> > > + * and programming a PHY that is stuck in reset via
> > > + * ice_ptp_port_phy_restart() would leave Tx timestamping
> > > + * permanently broken. So warn and skip the restart on
> > > + * failure; the sequence is retried on the next link-up event.
> > > + */
> > > + if (!linkup)
> > > + return;
> > > +
> > > + if (ice_ptp_phy_soft_reset_eth56g(hw, ptp_port->port_num))
> > > + dev_warn(ice_pf_to_dev(pf),
> > > + "PTP failed to soft reset PHY port %u on
> > > link-up; skipping restart, Tx timestamping may be stuck, try toggle a
> > link to recover\n",
> > > + ptp_port->port_num);
> > > + else
> > > + ice_ptp_port_phy_restart(ptp_port);
> >
> > We hit a regression from this hunk while testing the series on an
> > E825 T-BC system: after the soft reset the port no longer signals
> > latched Tx timestamps. Nothing re-enables the port interrupt
> > configuration (PHY_REG_TS_INT_CONFIG) afterwards, the only writers
> > are the clock owner init and rebuild paths via
> > ice_ptp_cfg_phy_interrupt(), and ice_ptp_port_phy_restart() does not
> > touch it. So the first link-up that takes this path leaves the port
> > without Tx timestamp interrupts until the next reset of the clock
> > owner PF or a driver reload.
> >
> > Regards,
> >
> > Petr
>
> Hi, thanks for letting me know. I will look into it.
Hi,
If I am not mistaken, Jake Keller sent you a patch fixing the issue with this
patch regarding PHY reconfiguration.
Did you have a chance to test this? If so, I will implement this fix in the new
revision.
>
> >
> > > return;
> > > case ICE_MAC_GENERIC:
> > > ice_ptp_port_phy_restart(ptp_port);
> > > return;
> > > - case ICE_MAC_GENERIC_3K_E825:
> > > - if (linkup)
> > > - ice_ptp_port_phy_restart(ptp_port);
> > > - return;
> > > default:
> > > dev_warn(ice_pf_to_dev(pf), "%s: Unknown PHY type\n",
> > > __func__);
> > > }
> > > @@ -3380,6 +3438,7 @@ void ice_ptp_init(struct ice_pf *pf)
> > >
> > > ptp->state = ICE_PTP_INITIALIZING;
> > >
> > > + mutex_init(&hw->ptp.tx_tstamp_lock);
> > > if (hw->lane_num < 0) {
> > > err = hw->lane_num;
> > > goto err_exit;
> > > @@ -3443,6 +3502,7 @@ void ice_ptp_init(struct ice_pf *pf)
> > >
> > > ice_ptp_cleanup_adapter(pf);
> > > err_exit:
> > > + mutex_destroy(&hw->ptp.tx_tstamp_lock);
> > > /* If we registered a PTP clock, release it */
> > > if (pf->ptp.clock) {
> > > ptp_clock_unregister(ptp->clock);
> > > @@ -3469,6 +3529,7 @@ void ice_ptp_release(struct ice_pf *pf)
> > >
> > > if (pf->ptp.state != ICE_PTP_READY) {
> > > mutex_destroy(&pf->ptp.port.ps_lock);
> > > + mutex_destroy(&pf->hw.ptp.tx_tstamp_lock);
> > > ice_ptp_cleanup_pf(pf);
> > > ice_ptp_cleanup_adapter(pf);
> > > if (pf->ptp.clock) {
> > > @@ -3495,6 +3556,7 @@ void ice_ptp_release(struct ice_pf *pf)
> > >
> > > ice_ptp_port_phy_stop(&pf->ptp.port);
> > > mutex_destroy(&pf->ptp.port.ps_lock);
> > > + mutex_destroy(&pf->hw.ptp.tx_tstamp_lock);
> > > if (pf->ptp.kworker) {
> > > kthread_destroy_worker(pf->ptp.kworker);
> > > pf->ptp.kworker = NULL;
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > > b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > > index a1970a887c38..b23b68cd18ee 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > > @@ -1160,11 +1160,15 @@ static int ice_read_ptp_tstamp_eth56g(struct
> > > ice_hw *hw, u8 port, u8 idx,
> > > *
> > > * 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().
> > > + * ice_ptp_reset_ts_memory_eth56g().
> > > *
> > > * This function should only be called on an idx whose bit is set
> > > according to
> > > * ice_get_phy_tx_tstamp_ready().
> > > *
> > > + * Serialized against ice_ptp_clear_tx_memory_status_eth56g() via
> > > + * tx_tstamp_lock so the two paths do not interleave their reads of the
> > > same
> > > + * port's Tx timestamp memory.
> > > + *
> > > * Return:
> > > * * %0 - success
> > > * * %other - failed to write to PHY
> > > @@ -1175,25 +1179,62 @@ static int ice_clear_ptp_tstamp_eth56g(struct
> > > ice_hw *hw, u8 port, u8 idx)
> > > u16 lo_addr;
> > > int err;
> > >
> > > - /* Read the timestamp register to ensure the timestamp status bit is
> > > - * cleared.
> > > + lo_addr = (u16)PHY_TSTAMP_L(idx);
> > > +
> > > + mutex_lock(&hw->ptp.tx_tstamp_lock);
> > > +
> > > + /* 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. This clears only this index, leaving any
> > > + * other in-flight timestamps on the port untouched.
> > > */
> > > err = ice_read_ptp_tstamp_eth56g(hw, port, idx, &unused_tstamp);
> > > - if (err) {
> > > + 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) {
> > > + 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;
> > > +
> > > + mutex_unlock(&hw->ptp.tx_tstamp_lock);
> > > +
> > > + return err;
> > > +}
> > > +
> > > +/**
> > > + * 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().
> > > + *
> > > + * Holds tx_tstamp_lock so this full-port sweep does not interleave with
> > > the
> > > + * per-index reads in ice_clear_ptp_tstamp_eth56g() on the same port.
> > > + *
> > > + * Return: 0 on success, negative error code on failure to read the PHY.
> > > + */
> > > +int ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port)
> > > +{
> > > + u64 unused_tstamp;
> > > + int err = 0;
> > > + u8 idx;
> > > +
> > > + mutex_lock(&hw->ptp.tx_tstamp_lock);
> > > + for (idx = 0; idx < INDEX_PER_PORT; idx++) {
> > > + err = ice_read_ptp_tstamp_eth56g(hw, port, idx, &unused_tstamp);
> > > + if (err)
> > > + break;
> > > }
> > > + mutex_unlock(&hw->ptp.tx_tstamp_lock);
> > >
> > > - return 0;
> > > + return err;
> > > }
> > >
> > > /**
> > > @@ -1204,12 +1245,8 @@ static void ice_ptp_reset_ts_memory_eth56g(struct
> > > ice_hw *hw)
> > > {
> > > unsigned int port;
> > >
> > > - 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 (port = 0; port < hw->ptp.num_lports; port++)
> > > + ice_ptp_clear_tx_memory_status_eth56g(hw, port);
> > > }
> > >
> > > /**
> > > @@ -5277,6 +5314,23 @@ static void ice_read_phy_tstamp_e830(const struct
> > > ice_hw *hw, u8 idx,
> > > FIELD_PREP(PHY_EXT_40B_LOW_M, lo);
> > > }
> > >
> > > +/**
> > > + * ice_clear_phy_tstamp_e830 - Clear a timestamp from the E830 PHY
> > > + * @hw: pointer to the HW struct
> > > + * @idx: the timestamp index to clear
> > > + *
> > > + * Clear the valid bit for the given timestamp index in the Tx memory
> > > (TS_MEM).
> > > + * On E830 devices the PRTMAC_TS_TX_MEM_VALID_L/H registers are read-only
> > > + * mirrors of the per-entry TX_VALID bits and cannot be written. The
> > > actual
> > > + * TS_MEM entry's TX_VALID bit is cleared by reading the corresponding
> > > + * PRTTSYN_TXTIME_L/H registers (read-to-clear).
> > > + */
> > > +static void ice_clear_phy_tstamp_e830(const struct ice_hw *hw, u8 idx)
> > > +{
> > > + rd32(hw, E830_PRTTSYN_TXTIME_H(idx));
> > > + rd32(hw, E830_PRTTSYN_TXTIME_L(idx));
> > > +}
> > > +
> > > /**
> > > * ice_get_phy_tx_tstamp_ready_e830 - Read Tx memory status register
> > > * @hw: pointer to the HW struct
> > > @@ -5772,6 +5826,9 @@ int ice_clear_phy_tstamp(struct ice_hw *hw, u8
> > > block, u8 idx)
> > > switch (hw->mac_type) {
> > > case ICE_MAC_E810:
> > > return ice_clear_phy_tstamp_e810(hw, block, idx);
> > > + case ICE_MAC_E830:
> > > + ice_clear_phy_tstamp_e830(hw, idx);
> > > + return 0;
> > > case ICE_MAC_GENERIC:
> > > return ice_clear_phy_tstamp_e82x(hw, block, idx);
> > > case ICE_MAC_GENERIC_3K_E825:
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> > > b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> > > index 16b1988e993d..b003e0aae8b1 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> > > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
> > > @@ -304,6 +304,7 @@ int ice_ptp_clear_phy_offset_ready_e82x(struct ice_hw
> > > *hw);
> > > int ice_read_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx, u64
> > > *tstamp);
> > > int ice_clear_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx);
> > > void ice_ptp_reset_ts_memory(struct ice_hw *hw);
> > > +int ice_ptp_clear_tx_memory_status_eth56g(struct ice_hw *hw, u8 port);
> > > int ice_ptp_init_phc(struct ice_hw *hw);
> > > void ice_ptp_init_hw(struct ice_hw *hw);
> > > int ice_get_phy_tx_tstamp_ready(struct ice_hw *hw, u8 block, u64
> > > *tstamp_ready);
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_type.h
> > > b/drivers/net/ethernet/intel/ice/ice_type.h
> > > index 710c519d670d..d8c73fcafc25 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_type.h
> > > +++ b/drivers/net/ethernet/intel/ice/ice_type.h
> > > @@ -888,6 +888,13 @@ enum ice_global_link_topo {
> > >
> > > struct ice_ptp_hw {
> > > union ice_phy_params phy;
> > > + /* Serializes eth56g Tx timestamp-memory reads (the per-index
> > > + * timestamp entries, not the TX_MEMORY_STATUS registers). Reading an
> > > + * entry auto-clears its ts_memory_status bit as a side effect, so the
> > > + * per-index and full-port clear paths must not interleave on the same
> > > + * port or a bit could re-latch the interrupt.
> > > + */
> > > + struct mutex tx_tstamp_lock;
> > > u8 num_lports;
> > > u8 ports_per_phy;
> > > };
