* RE: [PATCH iwl-net] ice: Skip PTP timestamp processing during release
2026-10-01 14:49 ` sashiko-bot
@ 2026-10-01 15:08 ` Temerkhanov, Sergey
0 siblings, 0 replies; 3+ messages in thread
From: Temerkhanov, Sergey @ 2026-10-01 15:08 UTC (permalink / raw)
To: sashiko-reviews@lists.linux.dev; +Cc: intel-wired-lan@lists.osuosl.org
> -----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
^ permalink raw reply [flat|nested] 3+ messages in thread