Re: [Intel-wired-lan] [PATCH iwl-net 2/4] ice: clear unexpected Tx timestamp ready bits to prevent stuck PHY

On 7/27/2026 1:21 AM, Korba, Przemyslaw wrote:
> 
> 
> 
>> -----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.
> 
We have a patch undergoing testing, but I am not confident it will fix
the issue. I believe we have a gap here where the ice_ptp_init and link
state can race such that the PTP state shows the link is down, but we
never follow up by calling ice_ptp_link_change on link going up.

I'm still trying to investigate the full path, but I think the patch in
this series which removes the call of ice_ptp_link_change from ice_down
and ice_up is possibly the root cause here.

I am not certain what a correct fix looks like yet, and Petr and I are
still working on reproducing and confirming the situation.

Thanks,
Jake