[PATCH iwl-net] ice: Skip PTP timestamp processing during release

[PATCH iwl-net] ice: Skip PTP timestamp processing during release

[Sergey Temerkhanov]

Do not run a timestamp processing callback on unready hardware
thus avoiding hitting uninitialized structures etc.

Fixes: 88b68f35eb43 ("ice: PTP: fix missing timestamps on E825 hardware")
Signed-off-by: Sergey Temerkhanov <sergey.temerkhanov@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 24d4ab2197c3..d04a4099db92 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -2806,6 +2806,9 @@ s8 ice_ptp_request_ts(struct ice_ptp_tx *tx, struct sk_buff *skb)
 
 void ice_ptp_process_ts(struct ice_pf *pf)
 {
+	if (pf->ptp.state != ICE_PTP_READY)
+		return;
+
 	switch (pf->ptp.tx_interrupt_mode) {
 	case ICE_PTP_TX_INTERRUPT_NONE:
 		/* This device has the clock owner handle timestamps for it */
-- 
2.53.0

Re: [PATCH iwl-net] ice: Skip PTP timestamp processing during release

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ice: early return in ice_ptp_process_ts causes infinite interrupt loop
--

commit bedb82d9a31e734c0c507d3503f8d563a9326bcf
Author: Sergey Temerkhanov <sergey.temerkhanov@intel.com>

ice: Skip PTP timestamp processing during release

This commit skips PTP timestamp processing when the hardware is not in
the ICE_PTP_READY state. The intent is to avoid executing the timestamp
callbacks on unready hardware and hitting uninitialized structures.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index b2de76065f46d..095dfe23140d5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -2815,6 +2815,9 @@ s8 ice_ptp_request_ts(struct ice_ptp_tx *tx, struct sk_buff *skb)
>  
>  void ice_ptp_process_ts(struct ice_pf *pf)
>  {
> +	if (pf->ptp.state != ICE_PTP_READY)
> +		return;
> +

[Severity: High]
Could this early return cause an infinite interrupt loop or kthread storm?

If the PTP state changes to ICE_PTP_UNINIT (during driver unload) or
ICE_PTP_RESETTING (during reset) while there are pending timestamps, it
appears this early return bypasses ice_ptp_process_tx_tstamp(). This leaves
the tx->in_use bits permanently set since the garbage collection of pending
timestamps is skipped.

If tx->in_use is not cleared, ice_ptp_tx_tstamps_pending() will continue to
evaluate to true.

On E830 devices, this can cause an infinite loop in hard IRQ context:

drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_ts_irq() {
    ...
		if (ice_ptp_tx_tstamps_pending(pf, true)) {
			/* Process outstanding Tx timestamps. If there
			 * is more work, re-arm the interrupt to trigger again.
			 */
			wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);
			ice_flush(hw);
		}
    ...
}

On non-E830 devices, it can continuously re-arm the OICR interrupt from the
worker thread, causing a kthread storm:

drivers/net/ethernet/intel/ice/ice_main.c:ice_misc_intr_thread_fn() {
    ...
	if (ice_ptp_tx_tstamps_pending(pf, true)) {
		/* If any new Tx timestamps happened while in interrupt,
		 * re-arm the interrupt to trigger it again.
		 */
		wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);
		ice_flush(hw);
	}
    ...
}

Does there need to be a way to clear tx->in_use and free the associated
SKBs when bypassing the timestamp processing here?

>  	switch (pf->ptp.tx_interrupt_mode) {
>  	case ICE_PTP_TX_INTERRUPT_NONE:
>  		/* This device has the clock owner handle timestamps for it */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001115232.13681-1-sergey.temerkhanov@intel.com?part=1

RE: [PATCH iwl-net] ice: Skip PTP timestamp processing during release

[Temerkhanov, Sergey]

> -----Original Message-----
> From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
> Sent: Thursday, October 1, 2026 4:49 PM
> To: Temerkhanov, Sergey <sergey.temerkhanov@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org
> Subject: Re: [PATCH iwl-net] ice: Skip PTP timestamp processing during release
> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [High] ice: early return in ice_ptp_process_ts causes infinite interrupt loop
> --
> 
> commit bedb82d9a31e734c0c507d3503f8d563a9326bcf
> Author: Sergey Temerkhanov <sergey.temerkhanov@intel.com>
> 
> ice: Skip PTP timestamp processing during release
> 
> This commit skips PTP timestamp processing when the hardware is not in the
> ICE_PTP_READY state. The intent is to avoid executing the timestamp callbacks
> on unready hardware and hitting uninitialized structures.
> 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
> > b/drivers/net/ethernet/intel/ice/ice_ptp.c
> > index b2de76065f46d..095dfe23140d5 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> > @@ -2815,6 +2815,9 @@ s8 ice_ptp_request_ts(struct ice_ptp_tx *tx,
> > struct sk_buff *skb)
> >
> >  void ice_ptp_process_ts(struct ice_pf *pf)  {
> > +	if (pf->ptp.state != ICE_PTP_READY)
> > +		return;
> > +
> 
> [Severity: High]
> Could this early return cause an infinite interrupt loop or kthread storm?
> 
> If the PTP state changes to ICE_PTP_UNINIT (during driver unload) or
> ICE_PTP_RESETTING (during reset) while there are pending timestamps, it
> appears this early return bypasses ice_ptp_process_tx_tstamp(). This leaves
> the tx->in_use bits permanently set since the garbage collection of pending
> timestamps is skipped.
> 
> If tx->in_use is not cleared, ice_ptp_tx_tstamps_pending() will continue to
> evaluate to true.
> 
> On E830 devices, this can cause an infinite loop in hard IRQ context:
> 
> drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_ts_irq() {
>     ...
> 		if (ice_ptp_tx_tstamps_pending(pf, true)) {
> 			/* Process outstanding Tx timestamps. If there
> 			 * is more work, re-arm the interrupt to trigger again.
> 			 */
> 			wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);
> 			ice_flush(hw);
> 		}
>     ...
> }
> 
> On non-E830 devices, it can continuously re-arm the OICR interrupt from the
> worker thread, causing a kthread storm:
> 
> drivers/net/ethernet/intel/ice/ice_main.c:ice_misc_intr_thread_fn() {
>     ...
> 	if (ice_ptp_tx_tstamps_pending(pf, true)) {
> 		/* If any new Tx timestamps happened while in interrupt,
> 		 * re-arm the interrupt to trigger it again.
> 		 */
> 		wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);
> 		ice_flush(hw);
> 	}
>     ...
> }

The reported impact is apparently a false positive, but it
did point at a real (if bounded) gap on E830, which is going to be addressed in v2.

Why the described failure mode doesn't occur:

1. The re-arm condition is not "in_use is non-empty".
   ice_ptp_tx_tstamps_pending(pf, true) goes through ice_port_has_timestamps(),
   which requires tx->init, then ice_ptp_is_tx_tracker_up() (tx->init &&
   !tx->calibrating && link_up), and finally bitmap_andnot(in_use, stale).

2. The release path does clear in_use and does free the SKBs.
   ice_ptp_release() sets state = ICE_PTP_UNINIT and then calls
   ice_ptp_release_tx_tracker(), which clears tx->init under tx->lock, does
   synchronize_irq(pf->oicr_irq.virq), and then ice_ptp_flush_tx_tracker()
   clears in_use/stale and dev_kfree_skb_any()s every pending SKB. Once
   tx->init == 0, pending() is false. The window is short and self-terminating.

3. The reset path is already guarded and is flushed on rebuild.
   ICE_PTP_RESETTING is only set by ice_ptp_prepare_for_reset(), which runs from
   ice_prepare_for_reset() after ICE_*_REQ / ICE_RESET_OICR_RECV is already in
   pf->state. ice_misc_intr_thread_fn() already does
   "if (ice_is_reset_in_progress(pf->state)) goto skip_irq;" before calling
   ice_ptp_process_ts(), so this commit changes nothing on that path during
   reset - the described kthread storm would be pre-existing. On rebuild,
   ice_ptp_rebuild_owner() calls ice_ptp_flush_all_tx_tracker() for every port
   on the clock.

4. The old code did not garbage-collect here either.
   ice_ptp_process_tx_tstamp() returns early when ice_get_phy_tx_tstamp_ready()
   fails, and only reaches clear_bit(idx, tx->in_use) for indices the PHY marks
   ready. During teardown or reset the PHY read fails or reports nothing ready,
   so the 2-second staleness drop never ran in the old code in these states
   either. The early return removes an unsafe PHY access, not a guaranteed
   cleanup.

The valid part: ice_ptp_ts_irq() has no reset-in-progress check, and
ice_prepare_for_reset() does not call ice_ptp_link_change(pf, false), so
ptp_port->link_up stays true until ice_rebuild(). On E830 that leaves a bounded
window where the hard IRQ can re-arm PFINT_OICR with nothing consuming the
timestamps. Bounded, not infinite - but pointless. I'll add to v2:

    case ICE_MAC_E830:
        /* E830 can read timestamps in the top half using rd32() */
        if (pf->ptp.state != ICE_PTP_READY)
            return IRQ_HANDLED;

        ice_ptp_process_ts(pf);

This makes the re-arm decision consistent with the processing decision and
mirrors the guard ice_misc_intr_thread_fn() already has for the other MAC types.

> 
> Does there need to be a way to clear tx->in_use and free the associated SKBs
> when bypassing the timestamp processing here?
> 
> >  	switch (pf->ptp.tx_interrupt_mode) {
> >  	case ICE_PTP_TX_INTERRUPT_NONE:
> >  		/* This device has the clock owner handle timestamps for it */
> 
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261001115232.13681-
> 1-sergey.temerkhanov@intel.com?part=1