RE: [PATCH v3 iwl-next 2/3] ice: add TSPLL DPLL device and TIME_REF pin for E825

-----Original Message-----
From: Nitka, Grzegorz <redacted>
Sent: Monday, August 3, 2026 1:35 PM
To: intel-wired-lan@lists.osuosl.org
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
richardcochran@gmail.com; andrew+netdev@lunn.ch; Kitszel, Przemyslaw
[off-list ref]; Nguyen, Anthony L
[off-list ref]; Kubalewski, Arkadiusz
[off-list ref]; pabeni@redhat.com; kuba@kernel.org;
davem@davemloft.net; edumazet@google.com; Nitka, Grzegorz
[off-list ref]; Korba, Przemyslaw
[off-list ref]
Subject: [PATCH v3 iwl-next 2/3] ice: add TSPLL DPLL device and TIME_REF pin
for E825

This extends the E825 advanced sync-timing support introduced by the
tx-clk series, which added the TXC DPLL device for TX reference clock
control. The TSPLL, the source timer PLL, is now also exposed through
the dpll subsystem so that its lock status and clock source selection
are visible and controllable from userspace.

On E825 devices the TSPLL is the source timer PLL, distinct from the
EEC and PPS DPLLs used on E810. Register it as a DPLL_TYPE_GENERIC
device for owner PFs.

Add struct ice_dplls::tspll_in, a fwnode-backed input pin named
"time_ref". The state_on_dpll_get callback queries ICE_CGU_R23 via
ice_tspll_get_clk_src() and returns CONNECTED when TIME_REF is
selected as clock source, DISCONNECTED otherwise. The state_on_dpll_set
callback switches the source between TIME_REF and TCXO via the new
ice_tspll_set_cfg() helper. Registration is deferred via the dpll
notifier path if the pin is not yet visible in the subsystem at probe
time.

Initialize TSPLL DPLL state from direct clock-source/lock reads so the
first published state reflects hardware and prev_dpll_state matches.
During periodic polling, the DPLL worker consumes
READ_ONCE(pf->ptp.tspll_locked), maintained and recovered by the PTP
periodic worker. When the TSPLL clock source is TCXO (TIME_REF pin not
selected), UNLOCKED is reported unconditionally to reflect the
free-running state of the oscillator regardless of the raw lock bit.
To avoid stale lock-status reads after synchronous source changes, the
set callback now refreshes tspll.dpll_state immediately and emits a DPLL
change notification when the cached state changed.

If a TSPLL reconfiguration is applied but the PLL has not yet
re-acquired lock, treat the internal -EAGAIN result as success so the
PTP periodic worker can complete recovery, while real -EBUSY failures
from reset/SBQ paths still propagate to userspace.

The TSPLL userspace reconfiguration path (state_on_dpll_set) and the
PTP periodic worker (ice_ptp_tspll_monitor) both read TSPLL HW state
and write the pf->ptp.tspll_locked cache consumed by the DPLL worker.
Serialize both with pf->dplls.lock across their HW-read -> cache-write
sequence; otherwise a preempted monitor could overwrite an accurate
cache update from the DPLL callback with stale HW state. The mutex
lifetime is lifted to PF-features scope (initialized in
ice_init_features() before ice_ptp_init(), destroyed in
ice_deinit_features() after ice_ptp_release()) so the PTP monitor
can take it unconditionally regardless of DPLL init state.

Extend ice_dpll_deinit_txclk_pins() with a "flush" parameter so the
E825 init error path for the TSPLL fwnode pin can tear down TXCLK
pins without flushing pf->dplls.wq. If the flush ran here, notifier
work items queued during earlier init steps would be blocked on
pf->dplls.dpll_init, which is only completed at the unregister_pins
label reached after this teardown. destroy_workqueue() at that label
drains the queued items safely. Existing full-teardown callers pass
flush=true and keep current behavior.

Reviewed-by: Przemyslaw Korba <redacted>
Signed-off-by: Grzegorz Nitka <redacted>
---
 drivers/net/ethernet/intel/ice/ice_dpll.c  | 420 ++++++++++++++++++---
 drivers/net/ethernet/intel/ice/ice_dpll.h  |   4 +
 drivers/net/ethernet/intel/ice/ice_main.c  |   9 +
 drivers/net/ethernet/intel/ice/ice_ptp.c   |  12 +
 drivers/net/ethernet/intel/ice/ice_tspll.c |  34 +-
 drivers/net/ethernet/intel/ice/ice_tspll.h |   2 +
 6 files changed, 426 insertions(+), 55 deletions(-)

[...]

Sashiko says:

 	ret = ice_dpll_init_txclk_pins(pf, 0);
-	if (ret)
+	if (ret) {
 		ice_dpll_deinit_rclk_pin(pf);
+		goto unregister_pins;
+	}
+
+	if (ice_pf_src_tmr_owned(pf) &&
+	    test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags)) {
+		ret = ice_dpll_init_tspll_pins(pf);
+		if (ret) {
+			/*
+			 * Avoid flushing pf->dplls.wq here: notifier work
+			 * items block on pf->dplls.dpll_init which is
+			 * completed by the unregister_pins path below.
+			 * destroy_workqueue() there drains queued items.
+			 */
+			ice_dpll_deinit_txclk_pins(pf, false);
+			ice_dpll_deinit_rclk_pin(pf);
+		}
+	}
 
 unregister_pins:
 	if (ret) {

This isn't a bug introduced by this patch, but there is a pre-existing
issue in the error handling path of ice_dpll_init_pins_e825() that can lead
to a double-free, use-after-free, and NULL pointer dereference.
If ice_dpll_init_fwnode_pins() fails during pin initialization, it destroys
the workqueue and returns an error without clearing the pointer:
ice_dpll_init_fwnode_pins() {
    ...
    destroy_workqueue(pf->dplls.wq);
    return ret;
}
The caller ice_dpll_init_pins_e825() catches this error and unconditionally
executes its unregister_pins teardown block:
ice_dpll_init_pins_e825() {
    ...
unregister_pins:
    if (ret) {
        for (int i = 0; i < pf->dplls.rclk.num_parents; i++)
            ice_dpll_deinit_fwnode_pin(&pf->dplls.inputs[i]);
        destroy_workqueue(pf->dplls.wq);
    }
    ...
}
This block iterates over all pins calling ice_dpll_deinit_fwnode_pin(),
which in turn calls flush_workqueue() with the pin structure:
ice_dpll_stop_fwnode_pin_activity() {
    ...
    if (flush)
        flush_workqueue(pin->pf->dplls.wq);
}
For pins that were never initialized, pin->pf is NULL, which causes a
NULL pointer dereference. Even if pin->pf is valid, it flushes an
already-destroyed workqueue causing a use-after-free. Finally,
destroy_workqueue() is called a second time on the same pointer.
Could the workqueue pointer be safely set to NULL after it is initially
destroyed, and checked before flushing or destroying it again in this
error path?

I believe his is a repeated concern from the previous iteration. It was marked as
false-positive from my side then.
I don't think this path is reachable. ice_dpll_init_pins_e825() starts
with:

    ret = ice_dpll_init_fwnode_pins(pf, pf->dplls.inputs, 0);
    if (ret)
        return ret;

That is a plain return, not a "goto unregister_pins". The label is only
reached from the three later failures (rclk, txclk, tspll), and all of
them imply ice_dpll_init_fwnode_pins() already returned 0 - so the
workqueue is alive and every pin in [0, rclk.num_parents) has ->pf set
and its notifier registered. So there is no second destroy_workqueue()
and no flush of a destroyed workqueue.

The internal error path of ice_dpll_init_fwnode_pins() is also
self-contained:

    for (i = 0; i < pf->dplls.rclk.num_parents; i++) {
        pins[start_idx + i].pf = pf;
        ...
        ret = ice_dpll_init_fwnode_pin(&pins[start_idx + i], pin_name);
        if (ret)
            goto error;
    }
    return 0;
error:
    complete_all(&pf->dplls.dpll_init);
    while (i--)
        ice_dpll_deinit_fwnode_pin(&pins[start_idx + i]);
    destroy_workqueue(pf->dplls.wq);

"while (i--)" only walks the pins that were successfully initialised,
and ->pf is assigned before each init attempt, so pin->pf is never NULL
in the flush. All flush_workqueue() calls happen before
destroy_workqueue().

Happy to NULL pf->dplls.wq after destroy as hardening if you'd prefer
it, but it isn't needed for correctness here and this patch doesn't
change that path.

[...]

--
2.39.3