Re: [PATCH net v3 14/14] ice: don't clear in_use until HW clears ready bitmap

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In ice_ptp_maybe_trigger_tx_interrupt()
  (drivers/net/ethernet/intel/ice/ice_ptp.c), the patch now reads the…
- [Medium] ice_ptp_flush_tx_tracker() now increments
  pf->ptp.tx_hwtstamp_flushed on every flushed request, and also…
- [Medium] The new 'ready bit not cleared' branch in
  ice_ptp_process_tx_tstamp() decides based on PHY reads done without…
- [Low] pf->ptp.tx_hwtstamp_timeouts now has two writers that are not
  serialized against each other.

--

ice: don't clear in_use until HW clears ready bitmap

This commit re-reads the PHY Tx timestamp ready bitmap after each read in
ice_ptp_process_tx_tstamp(). If the hardware did not clear the bit, the
index stays locked and is marked stale. It also moves the timeout count to
the point where the index is released, counts timeouts on flush, and moves
the hardware ready bitmap check out of ice_ptp_tx_tstamps_pending() and
into ice_ptp_maybe_trigger_tx_interrupt().

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index a9658f859c2e8..74e0c0138c501 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c

[ ... ]

> @@ -627,6 +622,27 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
>  		if (err && !drop_ts)
>  			continue;
>  
> +		/* verify ready bit cleared */
> +		if (tx->has_ready_bitmap) {
> +			err = ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready);
> +			if (err || tstamp_ready & BIT_ULL(phy_idx)) {
> +				spin_lock_irqsave(&tx->lock, flags);
> +				if (test_bit(idx, tx->in_use) &&
> +				    !test_and_set_bit(idx, tx->stale))
> +					dev_dbg(ice_pf_to_dev(pf), "PHY port %u failed to clear ready bit for idx %u\n",
> +						ptp_port->port_num, phy_idx);
> +				skb = tx->tstamps[idx].skb;
> +				tx->tstamps[idx].skb = NULL;
> +				spin_unlock_irqrestore(&tx->lock, flags);
> +				/* Free the SKB immediately since we're never
> +				 * going to report a timestamp for the stale
> +				 * packet.
> +				 */
> +				dev_kfree_skb_any(skb);
> +				continue;
> +			}
> +		}

[Severity: Medium]
Can this branch mark a newer request in the same slot as stale and free
its skb?

The decision here comes from ice_read_phy_tstamp() and the ready bitmap
re-read. Neither is done under tx->lock. After the lock is taken, the only
check is test_bit(idx, tx->in_use), and that check can't tell the original
request from a new one that reused idx.

On E825, a link-up event can run this sequence while the IRQ thread sits
between the re-read and spin_lock_irqsave():

ice_ptp_link_change()
  ice_ptp_port_phy_restart()
    ptp_port->tx.calibrating = true;
    ice_ptp_flush_tx_tracker()   /* clears in_use[idx], frees skb */
    ice_start_phy_timer_eth56g()
    ptp_port->tx.calibrating = false;

ice_ptp_request_ts()
    tx->tstamps[idx].start = jiffies;
    tx->tstamps[idx].skb = skb_get(skb);
    ...
    clear_bit(idx, tx->stale);
    smp_mb__before_atomic();
    set_bit(idx, tx->in_use);

Nothing in that sequence waits for an IRQ thread that is already inside the
loop. When the IRQ thread resumes, test_bit(idx, tx->in_use) returns true.
It then sets stale[idx] and frees the new request's skb because of the old
request's hardware state. The new packet's Tx timestamp is lost and
skb_tstamp_tx() is never called for it.

The timing window is narrow, and the freed reference came from skb_get(),
so this is a lost timestamp, not a memory safety issue. The existing
skip_ts_read path has a similar check-then-act gap.

>  		ice_trace(tx_tstamp_fw_done, tx->tstamps[idx].skb, idx);
>  
>  		/* For PHYs which don't implement a proper timestamp ready
> @@ -643,6 +659,9 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
>  			drop_ts = true;
>  
>  skip_ts_read:
> +		if (timeout)
> +			pf->ptp.tx_hwtstamp_timeouts++;
> +
>  		spin_lock_irqsave(&tx->lock, flags);
>  		if (!tx->has_ready_bitmap && raw_tstamp)
>  			tx->tstamps[idx].cached_tstamp = raw_tstamp;

[Severity: Low]
Is the timeout counted here only when this path is the one that actually
releases the index?

The increment happens before tx->lock is taken, and it doesn't check that
in_use[idx] is still set. This patch also adds an increment of
tx_hwtstamp_timeouts in ice_ptp_flush_tx_tracker(), under tx->lock.

On E82x/E825, ice_ptp_process_tx_tstamp() runs in the clock owner's misc
IRQ thread without ps_lock. The flush can run at the same time from
ice_ptp_port_phy_restart() (E825 link up), from
ice_ptp_flush_all_tx_tracker() during reset, or from tracker release.

Here is one way it can happen:

1. The IRQ thread sees a request that is older than 2 seconds with its
   ready bit clear, sets timeout = true and jumps to skip_ts_read.
2. The flush takes tx->lock, increments tx_hwtstamp_timeouts and clears
   in_use.
3. The IRQ thread increments tx_hwtstamp_timeouts again, and its
   clear_bit() does nothing.

One request ends up counted twice. Also, a plain u32 increment from two
CPUs can lose updates.

The commit message says:

  "This ensures that we don't double count timeouts or count them
  prematurely."

Is that still true now that the flush path also increments this counter?

[ ... ]

> @@ -849,14 +869,20 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx)
>  			ice_clear_phy_tstamp(hw, tx->block, phy_idx);
>  
>  		spin_lock_irqsave(&tx->lock, flags);
> +
> +		/* Count the number of Tx timestamps flushed */
> +		pf->ptp.tx_hwtstamp_flushed++;
> +
> +		/* Also count timeouts if we flush a request that timed out */
> +		if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ))
> +			pf->ptp.tx_hwtstamp_timeouts++;
> +

[Severity: Medium]
Doesn't this count one flushed request as both err and lost in the ethtool
timestamping stats?

Both counters are incremented here for the same idx. ice_get_ts_stats()
reports them as:

drivers/net/ethernet/intel/ice/ice_ethtool.c:ice_get_ts_stats() {
    ...
	ts_stats->err = ptp->tx_hwtstamp_skipped +
			ptp->tx_hwtstamp_flushed +
			ptp->tx_hwtstamp_discarded;
	ts_stats->lost = ptp->tx_hwtstamp_timeouts;
}

The kernel-doc for struct ethtool_ts_stats in include/linux/ethtool.h says:

 * @err: Number of arbitrary timestamp generation error events that the
 *	hardware encountered, exclusive of @lost statistics.

This patch lets stuck slots stay locked for well over 2 seconds, so the
E825 link-up flush from ice_ptp_port_phy_restart() is likely to hit this
case. Should tx_hwtstamp_flushed be skipped when the request is counted as
a timeout?

A related detail: an index whose skb was already freed by the new
stuck-ready-bit path still counts towards tx_hwtstamp_flushed, even though
tx->tstamps[idx].skb is NULL. Is that intended?

[ ... ]

> @@ -3001,7 +3009,15 @@ static void ice_ptp_maybe_trigger_tx_interrupt(struct ice_pf *pf)
>  	    !ice_pf_src_tmr_owned(pf))
>  		return;
>  
> -	if (ice_ptp_tx_tstamps_pending(pf, false)) {
> +	ret = ice_check_phy_tx_tstamp_ready(hw);
> +	if (ret < 0) {
> +		dev_dbg(dev, "Unable to read PHY Tx timestamp ready bitmap, err %pe\n",
> +			ERR_PTR(ret));
> +		/* Don't trigger an IRQ if we are unable to access the PHY */
> +		return;
> +	}
> +
> +	if (ret > 0 || ice_ptp_tx_tstamps_pending(pf, false)) {

[Severity: Medium]
Can this early return stop the watchdog from re-triggering the interrupt
for ports that still have outstanding requests?

Before this patch, ice_ptp_tx_tstamps_pending(pf, false) checked the
software in_use bitmaps first, and returned true whenever any request was
outstanding. The hardware check only ran when software had nothing
pending, and an error from it only turned off that hardware-only trigger.

Now the hardware read happens first. On E82x and E825
(ICE_PTP_TX_INTERRUPT_ALL on the clock owner), ice_check_phy_tx_tstamp_ready()
walks every quad or port and returns the first error it hits:

drivers/net/ethernet/intel/ice/ice_ptp_hw.c:ice_check_phy_tx_tstamp_ready_eth56g() {
	for (port = 0; port < hw->ptp.num_lports; port++) {
		...
		err = ice_get_phy_tx_tstamp_ready(hw, port, &tstamp_ready);
		if (err)
			return err;
    ...
}

ice_check_phy_tx_tstamp_ready_e82x() does the same across quads.

As long as one PHY can't be read, the watchdog triggers no IRQ for any
port. ice_ptp_process_tx_tstamp() is not re-run, so timed-out or stale
slots on healthy ports are not reaped and a stalled hardware interrupt is
not recovered.

This patch also stops the IRQ path from re-arming for stale entries and
for link-down or calibrating trackers, which leaves the watchdog as the
only fallback. The commit message also says PHY access errors can happen
during the link transition window this patch targets. The commit message
doesn't mention this ordering change.

The kernel-doc for ice_ptp_maybe_trigger_tx_interrupt() still says:

 * To avoid getting permanently stuck, periodically check if there is any
 * outstanding timestamps or if the PHY has marked any index as ready.

Would it work to check ice_ptp_tx_tstamps_pending() first, and treat a
hardware read error the same as no hardware-ready timestamps?

>  		dev_dbg(dev, "PTP periodic task detected waiting timestamps. Triggering Tx timestamp interrupt now.\n");
>  
>  		wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015191102.1174111-1-anthony.l.nguyen%40intel.com