Re: [PATCH net v2 14/15] 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 6 potential
issue(s) to consider.

Critical: 0 · High: 2 · 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).

- [High] ice: Tx timestamp SKB leak when PHY ready bit is stuck
- [High] net/ice: IRQ storm due to PHY read failure bypassing stale
  cleanup
- [Medium] ice_ptp_maybe_trigger_tx_interrupt()
  (drivers/net/ethernet/intel/ice/ice_ptp.c:2994-3002) now calls…
- [Medium] This patch removes the hardware ready-bitmap fallback from the
  IRQ-thread re-arm decision.
- [Medium] The patch now deliberately leaves a PHY Tx ready bit set while
  it keeps the index in_use and stale (ice_ptp.c:622-634), and it also…
- [Low] In ice_ptp_process_tx_tstamp() (ice_ptp.c:622-634), suppose
  ice_read_phy_tstamp() succeeds and the hardware clears the ready bit,…

--

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

After each PHY timestamp read, this patch re-reads the Tx timestamp ready
bitmap. If hardware did not clear the ready bit, the index stays locked and
is marked stale. The patch also removes the hardware ready bitmap check from
ice_ptp_tx_tstamps_pending(), stops the IRQ re-arm for trackers that are not
up, and moves the hardware check into the periodic watchdog.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 9dc0b5fa33198..8d302b500a399 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c

[ ... ]

> @@ -626,6 +619,20 @@ 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);
> +				spin_unlock_irqrestore(&tx->lock, flags);
> +				continue;
> +			}
> +		}

[Severity: High]
The commit message says a failed ice_get_phy_tx_tstamp_ready() is
treated the same as a ready bit that didn't clear. That holds for
this re-read, but not for the first read at the top of
ice_ptp_process_tx_tstamp():

	if (tx->has_ready_bitmap) {
		err = ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready);
		if (err)
			return;
	}

On that error the function returns before the loop. Nothing is
marked stale and the 2 second timeout is never checked.

With the link up, the tracker passes ice_ptp_is_tx_tracker_up(), and
bitmap_andnot(in_use, stale) is still non-empty. So
ice_misc_intr_thread_fn() sees ice_ptp_tx_tstamps_pending(pf, true)
return true and writes PFINT_OICR_TSYN_TX_M again right away.

If the sideband read keeps failing, doesn't this turn into the same
tight IRQ loop the patch is trying to remove? The pending skbs would
also never time out.

The early return was there before this patch. The new tracker-up
check only covers link down and calibrating, so it doesn't help in
this case.

Should the initial failure be handled like the re-read failure? For
example, the in_use entries could be marked stale while timed out
entries are still released. Note that tstamp_ready is uninitialized
on that path, so the loop can't simply fall through. Or is a
persistent read failure with the link up not expected to happen?

[Severity: High]
When the re-read still shows the ready bit set, this continue leaves
both the in_use bit and tx->tstamps[idx].skb in place. Is holding on
to the skb here intended?

Keeping the index locked makes sense. The problem is that the skb now
has no way out of the tracker while the PHY stays stuck, and the 2
second timeout no longer helps.

With the ready bit still set, a timed out entry doesn't take the goto
skip_ts_read branch. It reads the PHY again, fails this same check,
and hits this continue again. Before this patch the timeout path
always reached skip_ts_read and freed the skb.

ice_ptp_request_ts() stores a reference to the original skb taken
with skb_get(). The sending socket therefore stays charged in
sk_wmem_alloc for as long as the entry sits here.

The commit message says stuck indexes stay locked until the PHY is
reset on link up. As far as I can tell, if the link never comes back,
the skb is only freed by a reset or a driver unload. Until then the
owning socket can't be destroyed.

An unprivileged user in a container can request Tx timestamps. If
that happens around a link drop, the netns teardown could be left
hanging.

Could the skb be pulled out of tx->tstamps[idx] and freed (or
completed without a timestamp) under tx->lock once the timeout
expires? The in_use bit could still stay set so the index isn't
reused. Or is there another path that releases these skbs that I'm
missing?

[Severity: Low]
Suppose ice_read_phy_tstamp() succeeds and hardware clears the ready bit,
but this second ice_get_phy_tx_tstamp_ready() call hits a transient
sideband error. What happens then?

The index gets marked stale and stays in_use. On the next pass the ready
bit reads as clear, so ice_ptp_process_tx_tstamp() takes this earlier
branch:

		if (tx->has_ready_bitmap &&
		    !(tstamp_ready & BIT_ULL(phy_idx))) {
			if (drop_ts)
				goto skip_ts_read;

			continue;
		}

That branch never checks the stale bit. The slot seems to stay locked until
the 2 second timeout sets drop_ts. When it is finally released, timeout is
true and pf->ptp.tx_hwtstamp_timeouts gets incremented.

Would this count a PHY access error as a Tx timestamp timeout, and hold the
slot for up to 2 seconds?

The commit message says transient errors mark packets stale. It doesn't
mention the 2 second hold or the timeout being counted.

> +
>  		ice_trace(tx_tstamp_fw_done, tx->tstamps[idx].skb, idx);

[ ... ]

> @@ -2837,10 +2847,14 @@ static bool ice_port_has_timestamps(struct ice_ptp_tx *tx, bool in_irq)
>  		if (!tx->init)
>  			return false;
>  
> -		if (in_irq)
> +		if (in_irq) {
> +			if (!ice_ptp_is_tx_tracker_up(tx))
> +				return false;
> +
>  			return bitmap_andnot(tstamps, tx->in_use, tx->stale, tx->len);

[Severity: Medium]
The verify path in ice_ptp_process_tx_tstamp() now leaves a ready bit set
while the index stays in_use and stale. Can a stuck index on one port delay
Tx timestamps for healthy ports that share the same interrupt source?

The comment above ice_ptp_maybe_trigger_tx_interrupt() says:

 * timestamp data from the PHY. It will not interrupt again until all
 * current timestamp data is read.

An earlier commit in this series also says an unread ready bit means
delivery for the whole quad degrades to the periodic work.

On E82x, the timestamp interrupt enable and threshold are per quad. See the
single Q_REG_TX_MEM_GBL_CFG in ice_phy_cfg_intr_e82x(). On E825, the TSYN_TX
causes for all ports go to the clock owner in ICE_PTP_TX_INTERRUPT_ALL mode.

Before this patch, the hardware check in the IRQ re-arm path caused the
storm, but it also kept serving the other ports in the quad within
microseconds.

The in_irq branch now looks only at software state. Would new timestamps on
healthy ports then be handled only when ice_ptp_periodic_work() writes
PFINT_OICR every 500 ms?

On E825, ice_ptp_link_change() only restarts the PHY on link up, so this
could last as long as the bad port's link stays down. The latency numbers in
the commit message only cover healthy links.

> -		else
> +		} else {
>  			return !bitmap_empty(tx->in_use, tx->len);
> +		}
>  	}
>  }
>  
> @@ -2872,41 +2886,18 @@ static bool ice_any_port_has_timestamps(struct ice_pf *pf, bool in_irq)
>  
>  bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf, bool in_irq)
>  {

[ ... ]

> -	/* Check hardware indicator */
> -	ret = ice_check_phy_tx_tstamp_ready(hw);
> -	if (ret < 0) {
> -		dev_dbg(ice_pf_to_dev(pf), "Unable to read PHY Tx timestamp ready bitmap, err %d\n",
> -			ret);
> -		/* Stop triggering IRQs if we're unable to read PHY */
>  		return false;
>  	}
> -
> -	/* ice_check_phy_tx_tstamp_ready() returns 1 if there are timestamps
> -	 * available, 0 if there are no waiting timestamps, and a negative
> -	 * value if there was an error (which we checked for above).
> -	 */
> -	return ret > 0;
>  }

[Severity: Medium]
Removing this hardware fallback also affects the in_irq callers:
ice_misc_intr_thread_fn() and the E830 top half in ice_ptp_ts_irq(). Can a
stale index on a tracker that is up miss its re-arm?

Stale entries can exist while the tracker and link are both up, for example
after a PHC settime/adjtime:

ice_ptp_reset_cached_phctime() {
    ...
	ice_ptp_mark_tx_tracker_stale(&pf->ptp.port.tx);
}

Such a stale index might become ready after ice_ptp_process_tx_tstamp() has
already passed it in the loop, or after the last refresh of tstamp_ready.
If so, it isn't read in that pass. Then ice_misc_intr_thread_fn() runs:

	if (ice_ptp_tx_tstamps_pending(pf, true)) {
		...
		wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);

The software check is now bitmap_andnot(in_use, stale), which is empty, so
no re-arm happens. Before this patch, ice_check_phy_tx_tstamp_ready() would
have seen the ready bit and re-armed.

An earlier commit in this series says the PHY doesn't raise another Tx
timestamp interrupt until every outstanding ready bit is read. If that
holds, would new non-stale timestamps then wait for ice_ptp_periodic_work()
(500 ms)? That is well past ptp4l's default 10 ms tx_timestamp_timeout.

The commit message justifies dropping the hardware check for trackers that
are calibrating or link down. It doesn't cover trackers that are up and have
stale entries whose ready bits are set.

[ ... ]

> @@ -2999,7 +2991,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]
Does this early return change when the watchdog kicks the interrupt?

Before this patch, ice_ptp_tx_tstamps_pending(pf, false) checked the
software in_use bitmap first. It only read the PHY when nothing was pending
in software.

Now a PHY read error returns before the software check runs. In that case
PFINT_OICR_TSYN_TX_M is not written, even if in_use has entries.

ice_check_phy_tx_tstamp_ready_eth56g() returns the first error from any
port, and ice_check_phy_tx_tstamp_ready_e82x() does the same over quads:

	for (port = 0; port < hw->ptp.num_lports; port++) {
		...
		err = ice_get_phy_tx_tstamp_ready(hw, port, &tstamp_ready);
		if (err)
			return err;

In ICE_PTP_TX_INTERRUPT_ALL mode, one port or quad with a failing status
read would then stop the watchdog kick for every port.

After this patch, the in_irq path skips stale entries and trackers that are
down. The 2 second timeout is only checked inside
ice_ptp_process_tx_tstamp().

While the read keeps failing, would those in_use entries and their skbs stay
unreleased until a real PHY interrupt arrives?

Would it make sense to check the software tracker first, or to ignore the
PHY error when in_use is not empty? The commit message says the watchdog
continues to check the hardware bitmap. It doesn't mention this change in
ordering.

>  		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/20261008215614.1987250-1-anthony.l.nguyen%40intel.com