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

[PATCH iwl-net v2] 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>
---

Previous version: https://lore.kernel.org/intel-wired-lan/20261001115232.13681-1-sergey.temerkhanov@intel.com/
v1->v2: Skip possible extra work on E830

 drivers/net/ethernet/intel/ice/ice_ptp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 06842c06c689..2da510850a2b 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -2820,6 +2820,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 */
@@ -2949,6 +2952,9 @@ irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf)
 		return IRQ_WAKE_THREAD;
 	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);
 
 		if (ice_ptp_tx_tstamps_pending(pf, true)) {
-- 
2.53.0

Re: [PATCH iwl-net v2] 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] net: ice: SKB memory leak in PTP Tx tracker when state is not ready
--

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

ice: Skip PTP timestamp processing during release

This commit adds early returns to the PTP timestamp processing paths
when the PTP state is not ICE_PTP_READY. It is intended to prevent running
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..c0c12e28e08fa 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]
Does this early return bypass the 2-second SKB timeout cleanup logic in
ice_ptp_process_tx_tstamp()?

If PTP initialization or rebuild fails, setting pf->ptp.state to
ICE_PTP_ERROR (or any non-READY state), but the normal NIC rebuild
continues and succeeds (link_up becomes true, tx->init remains true),
userspace can still send packets with SO_TIMESTAMPING.

In this scenario, ice_ptp_request_ts() accepts the packets and queues the SKB
in tx->tstamps[idx].skb because ice_ptp_is_tx_tracker_up() ignores
pf->ptp.state.

Since ice_ptp_process_ts() now returns immediately, the timeout cleanup logic
inside ice_ptp_process_tx_tstamp() is bypassed. Will this permanently leak the
queued SKBs until the tracker fills up and drops new requests?

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

[ ... ]

> @@ -2944,6 +2947,9 @@ irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf)
>  		return IRQ_WAKE_THREAD;
>  	case ICE_MAC_E830:
>  		/* E830 can read timestamps in the top half using rd32() */
> +		if (pf->ptp.state != ICE_PTP_READY)
> +			return IRQ_HANDLED;
> +

[Severity: High]
Similarly for the E830 IRQ handler, does this early return bypass the cleanup
logic in ice_ptp_process_ts() and cause the same SKB memory leak when the PTP
state is not ready?

>  		ice_ptp_process_ts(pf);
>  
>  		if (ice_ptp_tx_tstamps_pending(pf, true)) {
>  			/* Process outstanding Tx timestamps. If there

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

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

[Temerkhanov, Sergey]

> -----Original Message-----
> From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
> Sent: Tuesday, October 6, 2026 12:45 PM
> To: Temerkhanov, Sergey <sergey.temerkhanov@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org
> Subject: Re: [PATCH iwl-net v2] ice: Skip PTP timestamp processing during
> release
> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [High] net: ice: SKB memory leak in PTP Tx tracker when state is not ready
> --
> 
> commit 20fdb60a0f81d0a78832f24386472d8613b50aea
> Author: Sergey Temerkhanov <sergey.temerkhanov@intel.com>
> 
> ice: Skip PTP timestamp processing during release
> 
> This commit adds early returns to the PTP timestamp processing paths when
> the PTP state is not ICE_PTP_READY. It is intended to prevent running 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..c0c12e28e08fa 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]
> Does this early return bypass the 2-second SKB timeout cleanup logic in
> ice_ptp_process_tx_tstamp()?
> 
> If PTP initialization or rebuild fails, setting pf->ptp.state to ICE_PTP_ERROR (or
> any non-READY state), but the normal NIC rebuild continues and succeeds
> (link_up becomes true, tx->init remains true), userspace can still send packets
> with SO_TIMESTAMPING.
> 
> In this scenario, ice_ptp_request_ts() accepts the packets and queues the SKB
> in tx->tstamps[idx].skb because ice_ptp_is_tx_tracker_up() ignores
> pf->ptp.state.
> 
> Since ice_ptp_process_ts() now returns immediately, the timeout cleanup logic
> inside ice_ptp_process_tx_tstamp() is bypassed. Will this permanently leak the
> queued SKBs until the tracker fills up and drops new requests?


The ICE_PTP_ERROR scenario is real, but it is a producer-side gap that
predates this commit, not something the early returns introduce. Addressing it
where it belongs.

On "the early return bypasses the 2-second timeout cleanup":

1. In ICE_PTP_ERROR that cleanup was already not running.
   ice_ptp_periodic_work() starts with

    if (pf->ptp.state != ICE_PTP_READY)
        return;

   which is upstream and predates this patch (present in v6.17). That worker is
   the only caller of ice_ptp_maybe_trigger_tx_interrupt(), i.e. the only
   software mechanism that kicks PFINT_OICR_TSYN_TX_M when the PHY has stopped
   interrupting. Once the state is ICE_PTP_ERROR the worker stops rescheduling
   itself, so nothing drives ice_ptp_process_tx_tstamp() anyway. The early
   return is not what stops the GC.

2. Even if it ran, it could not produce usable timestamps.
   ice_ptp_update_cached_phctime() lives in that same worker, so cached_phc_time
   goes stale. ice_ptp_extend_40b_ts() discards anything once cached_phc_jiffies
   is older than 2 s, and discards unconditionally when cached_phc_time is 0.
   And ice_ptp_process_tx_tstamp() returns early when
   ice_get_phy_tx_tstamp_ready() fails - the likely outcome on a PF whose PTP
   rebuild just failed - and otherwise only clears in_use for indices the PHY
   marks ready. So the pre-patch path was "touch a PHY we just failed to bring
   up, then drop everything", which is exactly what the Fixes: tag is about.

3. It is not an unbounded leak.
   Retention is capped at tx->len slots (<= 64 skb refs); beyond that
   ice_ptp_request_ts() returns -1 and ice_tstamp() counts tx_hwtstamp_skipped,
   which is the existing back-pressure. The references are released by
   ice_ptp_flush_all_tx_tracker() on the next successful rebuild and by
   ice_ptp_release_tx_tracker() at unload.

4. The E830 hunk is the same answer, and removing it would reintroduce a
   different problem: ice_ptp_ts_irq() re-arms PFINT_OICR whenever
   ice_ptp_tx_tstamps_pending(pf, true) is true. Without the state check the
   hard IRQ re-arms itself while nothing consumes the timestamps.

Where the report is right: ice_ptp_is_tx_tracker_up() does not look at
pf->ptp.state, so after a failed rebuild (link_up restored by ice_rebuild(),
tx->init still set, calibrating still clear) new requests are accepted into a
tracker that nothing will drain. Pre-existing, but worth closing. The fix is on
the producer side rather than keeping a consumer running against unready HW:

    @@ ice_ptp_is_tx_tracker_up(struct ice_ptp_tx *tx)
     	lockdep_assert_held(&tx->lock);
     
    +	/* Nothing drains the tracker unless PTP is READY, so do not let new
    +	 * requests accumulate in it.
    +	 */
    +	if (ptp_port_to_pf(ptp_port)->ptp.state != ICE_PTP_READY)
    +		return false;
    +
     	return tx->init && !tx->calibrating && READ_ONCE(ptp_port->link_up);

    @@ ice_ptp_rebuild(struct ice_pf *pf, enum ice_reset_req reset_type)
     err:
     	ptp->state = ICE_PTP_ERROR;
    +	/* Timestamp processing stops in this state, so return the requests
    +	 * that were outstanding when the reset started.
    +	 */
    +	ice_ptp_flush_tx_tracker(pf, &ptp->port.tx);
     	dev_err(ice_pf_to_dev(pf), "PTP reset failed %d\n", err);

This also makes ICE_PTP_RESETTING and the ice_ptp_release() window consistent:
requests are refused instead of being queued into a tracker that is about to be
torn down. Will send as a separate patch, since it fixes a different (older)
issue than this one.

> 
> >  	switch (pf->ptp.tx_interrupt_mode) {
> >  	case ICE_PTP_TX_INTERRUPT_NONE:
> >  		/* This device has the clock owner handle timestamps for it */
> 
> [ ... ]
> 
> > @@ -2944,6 +2947,9 @@ irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf)
> >  		return IRQ_WAKE_THREAD;
> >  	case ICE_MAC_E830:
> >  		/* E830 can read timestamps in the top half using rd32() */
> > +		if (pf->ptp.state != ICE_PTP_READY)
> > +			return IRQ_HANDLED;
> > +
> 
> [Severity: High]
> Similarly for the E830 IRQ handler, does this early return bypass the cleanup
> logic in ice_ptp_process_ts() and cause the same SKB memory leak when the
> PTP state is not ready?
> 
> >  		ice_ptp_process_ts(pf);
> >
> >  		if (ice_ptp_tx_tstamps_pending(pf, true)) {
> >  			/* Process outstanding Tx timestamps. If there
> 
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/20261006095435.419128-1-
> sergey.temerkhanov@intel.com?part=1