Re: [PATCH net v5 13/13] ice: don't clear in_use until HW clears ready bitmap
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-25 23:44:08
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider. Critical: 0 · High: 0 · Medium: 1 · 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] The patch removes the hardware ready-bitmap check (ice_check_phy_tx_tstamp_ready()) from ice_ptp_tx_tstamps_pending(). - [Low] The kernel-doc of ice_ptp_process_tx_tstamp() (ice_ptp.c:514-562) no longer matches the code. -- 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 the index's ready bit has not cleared, the index is marked stale and kept locked, and its SKB is freed. The patch also drops the hardware ready bitmap check from the in-IRQ re-arm decision. That check now runs only from the periodic watchdog.
quoted hunk
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c index b0ea01cfcc4f..c0d056896825 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c@@ -557,7 +557,9 @@ void ice_ptp_complete_tx_single_tstamp(struct ice_ptp_tx *tx) * extremely unlikely that a packet will ever take this long to timestamp. If * we detect a Tx timestamp request that has waited for this long we assume * the packet will never be sent by hardware and discard it without reading - * the timestamp register. + * the timestamp register. Note that in the unusual case where the PHY + * continuously fails to clear the ready bitmap index, the slot may remain + * locked for more than two seconds. */ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx) {
[Severity: Low] Is the rest of this kernel-doc still accurate after this change? The numbered list earlier in the same comment still says: * 3) read and copy the timestamp out of the PHY register * 4) unlock the index by clearing the associated in_use bit * 5) check if the timestamp is stale, and discard if so Look at the new "verify ready bit cleared" block in ice_ptp_process_tx_tstamp(). If the ready bit is still set after the read, or the re-read of the ready bitmap fails, the index is marked stale and the SKB is freed. in_use stays set, so step 4 never happens. The opening line, "loop over each index with a waiting skb", is also no longer true. in_use slots with a NULL skb are now visited on every pass until the hardware clears the bit. This paragraph also still says timed-out requests are discarded "without reading the timestamp register". A timed-out index whose ready bit is set is still read through ice_read_phy_tstamp(), and with this patch it can stay locked. Could the kernel-doc be updated to describe the new flow? [ ... ]
quoted hunk
@@ -2766,10 +2787,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); - else + } else { return !bitmap_empty(tx->in_use, tx->len); + } } }@@ -2801,41 +2826,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) { - struct ice_hw *hw = &pf->hw; - int ret; - - /* Check software indicator */ switch (pf->ptp.tx_interrupt_mode) { case ICE_PTP_TX_INTERRUPT_NONE: return false; case ICE_PTP_TX_INTERRUPT_SELF: - if (ice_port_has_timestamps(&pf->ptp.port.tx, in_irq)) - return true; - break; + return ice_port_has_timestamps(&pf->ptp.port.tx, in_irq); case ICE_PTP_TX_INTERRUPT_ALL: - if (ice_any_port_has_timestamps(pf, in_irq)) - return true; - break; + return ice_any_port_has_timestamps(pf, in_irq); default: WARN_ONCE(1, "Unexpected Tx timestamp interrupt mode %u\n", pf->ptp.tx_interrupt_mode); - break; - } - - /* 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]
Can removing this hardware fallback leave the PHY Tx timestamp interrupt
blocked until the watchdog runs?
ice_misc_intr_thread_fn() and the E830 path in ice_ptp_ts_irq() call
ice_ptp_tx_tstamps_pending(pf, true) to decide whether to re-arm:
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);
}
...
}
This change, combined with the new ice_ptp_is_tx_tracker_up() check in
ice_port_has_timestamps(), means the re-arm only looks at non-stale
in_use slots on trackers that are up.
The kernel-doc of ice_ptp_maybe_trigger_tx_interrupt() says the PHY "will
not interrupt again until all current timestamp data is read". So if a
ready entry is still unread at the end of an IRQ pass, nothing
retriggers the interrupt. It stays blocked until the 500 ms watchdog,
ice_ptp_periodic_work()->ice_ptp_maybe_trigger_tx_interrupt(), runs.
A few ways such an entry can be left behind:
- a stale request whose timestamp lands mid-pass, after the ready
bitmap was read
- a late completion on a port that is link down, or (on E82x) in
Vernier calibration with tx.disabled set until
ice_ptp_wait_for_offsets() succeeds
- an index this patch keeps locked on purpose because its ready bit
did not clear
Before this patch, the in-IRQ ice_check_phy_tx_tstamp_ready() call
retriggered the interrupt right away and drained these entries.
On E82x, timestamp memory and the interrupt are per quad (see
ice_phy_cfg_intr_e82x() and the quad split in ice_ptp_init_tx_e82x()).
In ICE_PTP_TX_INTERRUPT_ALL mode the clock owner services every port.
Could one leftover entry on a port that is going down or calibrating
then delay Tx timestamps for healthy link-up ports in the same quad by
up to about 500 ms? That is far beyond the 10 ms ptp4l default the
commit message cites. The race window looks narrow, but link flaps and
E82x recalibration happen in normal operation.
The commit "ice: keep Tx timestamp slots tracked until completion or
timeout" in this series also says an orphaned ready bit "blocks the
port interrupt".
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-jk-e825c-timestamp-processing-logic-fixes-srcu-v5-0-72289a4ac8eb%40intel.com