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.

> 
> >   		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;
> >   };