Re: [PATCH net-next 1/3] ice: monitor TSPLL lock from PTP periodic worker

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

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

- [Medium] ice_ptp_tspll_monitor()'s recovery cadence and its log rate-
  limit (ICE_TSPLL_LOG_INTERVAL = 120, documented as "~60 s at normal…
- [Medium] The lock-lost dev_warn() and the lock-acquired dev_info() in
  ice_ptp_tspll_monitor() are effectively unrate-limited when the TSPLL…
- [Low] Commit message misdescribes the tree state: it says the cached
  lock state is added "so a follow-up change can consume it from the…
- [Low] ice_ptp_tspll_monitor() is invoked at the top of
  ice_ptp_periodic_work(), ahead of ice_ptp_update_cached_phctime().
- [Low] ice_ptp_init() initializes only ptp->tspll_locked = true and
  never resets the newly added ptp->tspll_lock_retries, whereas the…

--

ice: monitor TSPLL lock from PTP periodic worker

Adds ice_ptp_tspll_monitor() to the PTP periodic worker so E825 owner PFs
poll the TSPLL lock status over CGU registers and restart the PLL when the
lock is lost.  The observed state is cached in pf->ptp.tspll_locked, and
three TSPLL helpers are added/exported for the read, the restart and the
clock source string.

A few questions about the new monitor and about the changelog below.

Cache the observed lock state in pf->ptp.tspll_locked using
WRITE_ONCE()/READ_ONCE() so a follow-up change can consume it from the
DPLL periodic worker (for user-space notification via
dpll_device_change_ntf()) and drop the redundant poll+recovery from
that path.

[Severity: Low]
Is there really a redundant poll+recovery in that path today?

At the baseline, ice_dpll.c has no reference to ICE_CGU_RO_LOCK,
ICE_CGU_R23_R24_TSPLL_ENABLE or any lost-lock/restart helper; grepping the
ice/ directory matches only ice_tspll.c and ice_common.h.
ice_dpll_periodic_work() only calls ice_dpll_update_state(), which polls
EEC/PPS state over AQ.

The follow-up patch in this series adds a consumer of the new cache
(ice_dpll_periodic_work_e825() -> ice_dpll_tspll_lock_status_get() ->
READ_ONCE(pf->ptp.tspll_locked)) but does not remove any TSPLL poll or
recovery.

Could the wording be adjusted to say this adds new TSPLL monitoring and
recovery rather than relocating existing recovery out of the DPLL worker?
The same claim is repeated in the new kernel-doc for @tspll_locked in
ice_ptp.h.

[ ... ]

[Severity: Medium]
The kernel-doc above says lock-lost events are logged "every 120 retries
(~60 s at normal poll rate)", and the new @tspll_lock_retries documentation
in ice_ptp.h repeats "~every 120 retries / ~60 s".  Does that hold when the
worker requeues itself early?

ice_ptp_periodic_work() ends with:

	/* Run twice a second or reschedule if phc update failed */
	kthread_queue_delayed_work(ptp->kworker, &ptp->work,
				   msecs_to_jiffies(err ? 10 : 500));

and ice_ptp_update_cached_phctime() returns an error whenever ICE_CFG_BUSY is
already set:

	if (test_and_set_bit(ICE_CFG_BUSY, pf->state))
		return -EAGAIN;

ICE_CFG_BUSY is held for the whole duration of ice_set_ringparam(),
ice_set_link_ksettings() and queue count reconfiguration.  If one of those
overlaps a lost TSPLL lock, the monitor runs every ~10 ms rather than every
500 ms.

Each of those ticks calls ice_tspll_restart_e825c(), which clears
ICE_CGU_R23_R24_TSPLL_ENABLE, sleeps 1-2 ms with the source PLL disabled and
re-enables it.

Is it intended that the source PLL is held disabled for 10-20% of that
window, that 3-4 sideband transactions are issued every 10 ms, and that the
documented ~60 s warning interval collapses to ~1.2 s while "after %u
retries" no longer maps to a fixed time base?  The later commit in this
series that wraps the monitor body in pf->dplls.lock leaves this cadence
unchanged.

+	} else {
+		if (pf->ptp.tspll_lock_retries) {
+			const char *src_str = "unknown";
+			enum ice_clk_src clk_src;
+
+			if (!ice_tspll_get_clk_src(&pf->hw, &clk_src))
+				src_str = ice_tspll_clk_src_str(clk_src);
+			dev_info(ice_pf_to_dev(pf),
+				 "TimeSync PLL lock acquired with %s clock source after %u retries.\n",
+				 src_str, pf->ptp.tspll_lock_retries);
+		}
+		WRITE_ONCE(pf->ptp.tspll_locked, true);
+		pf->ptp.tspll_lock_retries = 0;
+	}
+}

[Severity: Medium]
Because this branch resets pf->ptp.tspll_lock_retries to zero, the
'% ICE_TSPLL_LOG_INTERVAL' gate on the dev_warn() is satisfied again on the
very next lock-lost tick.

For a reference clock that alternates between locked and unlocked, does this
emit one dev_warn("TimeSync PLL lock lost...") plus one unrate-limited
dev_info("TimeSync PLL lock acquired ... after %u retries.") per poll pair,
i.e. roughly two messages per second for as long as the input flaps (and up
to ~100 per second if the worker is also requeueing at 10 ms)?

Would dev_warn_ratelimited()/dev_info_ratelimited(), or a counter that a
transient re-lock does not clear, match the stated intent of surfacing
persistent failures "without flooding the log"?

[Severity: Low]
The monitor runs ahead of the cached PHC refresh in the same serialized
kthread work item.  On a lock-lost tick it issues up to four sideband CGU
transactions plus the unconditional usleep_range(USEC_PER_MSEC,
2 * USEC_PER_MSEC) in ice_tspll_restart_e825c(), and each control queue
command is bounded only by ICE_CTL_Q_SQ_CMD_TIMEOUT (USEC_PER_SEC, "max
delay 1s" in ice_controlq.h).

ice_ptp_update_cached_phctime() documents that it "must be called
periodically to ensure that the cached value is never more than 2 seconds
old", and it warns and bumps late_cached_phc_updates when that is exceeded.

Would calling ice_ptp_tspll_monitor() after ice_ptp_update_cached_phctime(),
or from a separate work item, keep the sleeping hardware recovery sequence
off the time-critical path?

[Severity: Low]
Should ptp->tspll_lock_retries be cleared here as well?  The rebuild path
above resets both fields, but ice_ptp_init() only sets tspll_locked.

struct ice_ptp is embedded in the probe-lifetime struct ice_pf,
ice_ptp_release() does not clear either field, and
ice_init_features()->ice_ptp_init() /
ice_deinit_features()->ice_ptp_release() run again on the same pf across a
devlink reload.

If the TSPLL happened to be unlocked at unload time, does the leftover
non-zero counter make the first successful poll after reload print
"TimeSync PLL lock acquired with %s clock source after %u retries." for a
lock that was never lost in this instance, and start the
'% ICE_TSPLL_LOG_INTERVAL' gate at a stale phase so a genuine first
lock-lost event stays unlogged for up to ~119 poll cycles?

[ ... ]

[ ... ]

[ ... ]

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