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

> -----Original Message-----
> From: Nitka, Grzegorz <[email protected]>
> Sent: Monday, August 3, 2026 1:35 PM
> To: [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Kitszel, Przemyslaw
> <[email protected]>; Nguyen, Anthony L
> <[email protected]>; Kubalewski, Arkadiusz
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; Nitka, Grzegorz
> <[email protected]>; Korba, Przemyslaw
> <[email protected]>
> Subject: [PATCH v3 iwl-next 2/3] ice: add TSPLL DPLL device and TIME_REF =
pin
> for E825
>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20
> 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=3Dtrue and keep current behavior.
>=20
> Reviewed-by: Przemyslaw Korba <[email protected]>
> Signed-off-by: Grzegorz Nitka <[email protected]>
> ---
>  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(-)
>

[...]

> @@ -4093,8 +4371,25 @@ static int ice_dpll_init_pins_e825(struct ice_pf
> *pf)
>  		goto unregister_pins;
>=20
>  	ret =3D 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 =3D 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);
> +		}
> +	}
>=20
>  unregister_pins:
>  	if (ret) {
> @@ -4288,6 +4583,7 @@ static void ice_dpll_deinit_worker(struct ice_pf *p=
f)
>=20
>  	kthread_cancel_delayed_work_sync(&d->work);
>  	kthread_destroy_worker(d->kworker);
> +	d->kworker =3D NULL;
>  }
>=20
Sashiko says:
>  	ret =3D 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 =3D 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);
> +		}
> +	}
> =20
>  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 =3D 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 mar=
ked as
false-positive from my side then.
I don't think this path is reachable. ice_dpll_init_pins_e825() starts
with:

    ret =3D 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 =3D 0; i < pf->dplls.rclk.num_parents; i++) {
        pins[start_idx + i].pf =3D pf;
        ...
        ret =3D 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