Re: [PATCH net-next 2/3] ice: add TSPLL DPLL device and TIME_REF pin for E825

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] E825 TSPLL DPLL device is registered and its periodic worker
  started based only on `ice_pf_src_tmr_owned(pf) &&…
- [Medium] ice_dpll_deinit_tspll_pins() unconditionally calls
  ice_dpll_stop_fwnode_pin_activity(&pf->dplls.tspll_in, true), which…
- [Medium] The patch introduces `!IS_ERR_OR_NULL(pf->dplls.tspll.dpll)`
  as the existence predicate for the new TSPLL dpll device (in…
- [Medium] pf->dplls.lock is taken only by the new userspace TSPLL
  reconfiguration path (ice_dpll_tspll_state_on_dpll_set()) and the PTP…
- [Medium] The new userspace-reachable path
  ice_dpll_tspll_state_on_dpll_set() -> ice_tspll_set_cfg() ->…
- [Medium] ice_dpll_tspll_state_on_dpll_set() never compares the
  requested pin state with the currently programmed clock source…
- [Medium] TSPLL source changes and E825 periodic-output configuration
  perform unsynchronized read-modify-write operations on different…
- [Medium] A userspace-selected TSPLL clock source is not preserved
  across an E825 reset: ice_dpll_tspll_state_on_dpll_set() writes the…
- [Low] ice_tspll_cfg_e825c() now returns -EAGAIN for 'configuration
  applied but PLL not locked yet', but the new contract is not reflected…
- [Low] struct ice_dplls's kernel-doc still describes @lock as a purely
  DPLL-internal mutex ('protects DPLL configuration', '@lock serializes…

--

ice: add TSPLL DPLL device and TIME_REF pin for E825

This exposes the E825 source timer PLL (TSPLL) as a DPLL_TYPE_GENERIC device
with a fwnode-backed "time_ref" input pin, so its lock status can be read and
its clock source switched between TIME_REF and TCXO from userspace.  It also
moves the pf->dplls.lock lifetime out to ice_init_features() /
ice_deinit_features() and takes that mutex in the PTP TSPLL monitor.

A few questions below about the new paths.

> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
> index 85a74cd6ea1f..00e3f7de7810 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c

[ ... ]

> @@ -2814,6 +2817,160 @@ static const struct dpll_pin_ops ice_dpll_txclk_ops = {
>  	.direction_get = ice_dpll_input_direction,
>  };
>  
> +/**
> + * ice_dpll_tspll_lock_status_get - derive TSPLL state for dpll subsystem
> + * @pf: board private structure
> + * @use_cached: if true, read lock state from pf->ptp.tspll_locked (maintained
> + *              by the PTP periodic worker); if false, query hardware directly
> + *
> + * If TIME_REF is not selected, TSPLL is treated as unlocked from the dpll
> + * subsystem perspective, regardless of raw lock bit.
> + *
> + * Return: TSPLL lock status
> + */
> +static enum dpll_lock_status
> +ice_dpll_tspll_lock_status_get(struct ice_pf *pf, bool use_cached)
> +{
> +	enum ice_clk_src clk_src;
> +	bool locked;
> +
> +	if (ice_tspll_get_clk_src(&pf->hw, &clk_src) ||
> +	    clk_src != ICE_CLK_SRC_TIME_REF)
> +		return DPLL_LOCK_STATUS_UNLOCKED;
> +
> +	if (use_cached) {
> +		locked = READ_ONCE(pf->ptp.tspll_locked);
> +	} else {
> +		bool lock_lost;
> +
> +		if (ice_tspll_lost_lock_e825c(&pf->hw, &lock_lost))
> +			return DPLL_LOCK_STATUS_UNLOCKED;
> +		locked = !lock_lost;
> +	}
> +	return locked ? DPLL_LOCK_STATUS_LOCKED : DPLL_LOCK_STATUS_UNLOCKED;
> +}

[ ... ]

> +static int
> +ice_dpll_tspll_state_on_dpll_set(const struct dpll_pin *pin, void *pin_priv,
> +				 const struct dpll_device *dpll,
> +				 void *dpll_priv, enum dpll_pin_state state,
> +				 struct netlink_ext_ack *extack)
> +{
> +	struct ice_dpll_pin *p = pin_priv;
> +	struct ice_pf *pf = p->pf;
> +	enum ice_clk_src clk_src;
> +	bool changed = false;
> +	struct ice_dpll *tp;
> +	int ret;
> +
> +	if (ice_dpll_is_reset(pf, extack))
> +		return -EBUSY;
> +
> +	if (state != DPLL_PIN_STATE_CONNECTED &&
> +	    state != DPLL_PIN_STATE_DISCONNECTED) {
> +		NL_SET_ERR_MSG(extack,
> +			       "unsupported pin state for TSPLL reference clock");
> +		return -EINVAL;
> +	}
> +
> +	clk_src = (state == DPLL_PIN_STATE_CONNECTED) ? ICE_CLK_SRC_TIME_REF :
> +							ICE_CLK_SRC_TCXO;

[Severity: Medium]
Should this compare the requested state against the currently programmed
clock source before reprogramming?

The dpll core does not filter a set request that matches the current state -
dpll_pin_state_set() in drivers/dpll/dpll_netlink.c calls
ops->state_on_dpll_set() unconditionally:

	ret = ops->state_on_dpll_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
				     dpll, dpll_priv(dpll), state, extack);

So a management daemon that periodically re-asserts DPLL_PIN_STATE_CONNECTED
runs the full ice_tspll_set_cfg() sequence each time, which clears
ICE_CGU_R23_R24_TSPLL_ENABLE and ICE_CGU_R9_TIME_SYNC_EN, reprograms the
dividers, sleeps and waits 1-2 ms for re-lock.  Can that stop the PHC long
enough for ice_ptp_tspll_monitor() to see a lost lock and enter its restart
path, for a request that should be a no-op?  The get callback already has
ice_tspll_get_clk_src() available for the comparison.

> +	tp = &pf->dplls.tspll;
> +	/* Serialize multi-register TSPLL reconfiguration with restart path. */
> +	mutex_lock(&pf->dplls.lock);
> +	ret = ice_tspll_set_cfg(&pf->hw, ICE_TSPLL_FREQ_156_250, clk_src);

[Severity: Medium]
Is pf->dplls.lock enough to cover all TSPLL writers?  The commit message says:

    Serialize both with pf->dplls.lock across their HW-read -> cache-write
    sequence

but there is a third writer that takes no lock at all.  ice_ptp_rebuild_owner()
reprograms the same CGU registers and writes the same cache:

	err = ice_tspll_init(hw);
	if (err)
		return err;
	/* Rebuild reinitialized TSPLL, so reset monitor retry state. */
	WRITE_ONCE(ptp->tspll_locked, true);

Neither ice_prepare_for_reset() nor ice_ptp_prepare_for_reset() takes
pf->dplls.lock, and the ice_dpll_is_reset() check above happens before the
mutex is acquired, so a reset can start after the check passes.

Can two read-modify-write sequences on ICE_CGU_R9 / ICE_CGU_R23 then overlap
while ice_tspll_cfg_e825c() has the PLL disabled and is sleeping, and can the
unlocked WRITE_ONCE(ptp->tspll_locked, true) clobber the value published under
the mutex here?

[Severity: Medium]
Does the source selected here survive a reset?  Nothing records the requested
clk_src, while reset recovery reprograms the source from the firmware
capability value in ice_tspll_init():

	tspll_freq = (enum ice_tspll_freq)ts_info->time_ref;
	clk_src = (enum ice_clk_src)ts_info->clk_src;

ice_ptp_rebuild_owner() calls ice_tspll_init() for the source timer owner on
every non-PFR reset, so after an admin switches TIME_REF -> TCXO through this
pin, any PF/CORE/GLOBAL reset would silently revert it while the same pin
stays registered and only the lock status is republished.

[ ... ]

> @@ -3134,12 +3291,44 @@ ice_dpll_update_state(struct ice_pf *pf, struct ice_dpll *d, bool init)
>  	return ret;
>  }
>  
> +/**
> + * ice_dpll_periodic_work_e825 - TSPLL DPLL periodic update for E825
> + * @pf: board private structure
> + *
> + * Publish TSPLL lock status to the dpll subsystem. The PTP periodic worker
> + * owns TSPLL lock polling and recovery; this function consumes the cached
> + * result for dpll notifications.
> + *
> + * Context: Must be called without pf->dplls.lock held.
> + */
> +static void ice_dpll_periodic_work_e825(struct ice_pf *pf)
> +{
> +	struct ice_dpll *tp = &pf->dplls.tspll;
> +	enum dpll_lock_status new_state;
> +	bool changed = false;
> +
> +	mutex_lock(&pf->dplls.lock);
> +	new_state = ice_dpll_tspll_lock_status_get(pf, true);

[Severity: Medium]
What updates pf->ptp.tspll_locked when ice_ptp_init() failed?

The TSPLL device and this worker are started from ice_dpll_init_e825() based
on ice_pf_src_tmr_owned(pf) && test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags),
which does not imply PTP reached ICE_PTP_READY.  ice_init_features() ignores
the outcome of ice_ptp_init(), and the error exits there (hw->lane_num < 0,
ice_ptp_init_owner() failure, ice_ptp_init_work() failure) set state to
ICE_PTP_UNINIT without clearing ICE_FLAG_PTP_SUPPORTED.

The only producer of the cache then never runs:

ice_ptp_periodic_work()
	if (pf->ptp.state != ICE_PTP_READY)
		return;

	ice_ptp_tspll_monitor(pf);

So with use_cached = true the published status appears to freeze at whatever
ice_ptp_init() left in tspll_locked (it optimistically assigns
ptp->tspll_locked = true before the failing steps), for the lifetime of the
driver.  Can the first tick here also overwrite the init-time direct hardware
read done in ice_dpll_init_info_e825c() and emit a change notification that
does not reflect hardware?

[ ... ]

> @@ -3806,6 +4022,25 @@ static int ice_dpll_deinit_txclk_pins(struct ice_pf *pf)
>  	return 0;
>  }
>  
> +/**
> + * ice_dpll_deinit_tspll_pins - deinitialize TSPLL fwnode pin
> + * @pf: board private structure
> + *
> + * Stop notifier activity and release the TIME_REF fwnode pin, unregistering
> + * it from the TSPLL DPLL if it was registered.
> + */
> +static void ice_dpll_deinit_tspll_pins(struct ice_pf *pf)
> +{
> +	struct ice_dpll_pin *time_ref = &pf->dplls.tspll_in;
> +	struct ice_dpll *tp = &pf->dplls.tspll;
> +
> +	ice_dpll_stop_fwnode_pin_activity(time_ref, true);
> +	if (!IS_ERR_OR_NULL(time_ref->pin))
> +		dpll_pin_unregister(tp->dpll, time_ref->pin,
> +				    &ice_dpll_tspll_in_ops, time_ref);
> +	ice_dpll_release_fwnode_pin(time_ref);
> +}
> +
>  /**
>   * ice_dpll_deinit_pins - deinitialize direct pins
>   * @pf: board private structure
> @@ -3826,7 +4061,10 @@ static void ice_dpll_deinit_pins(struct ice_pf *pf, bool cgu)
>  
>  	ice_dpll_deinit_rclk_pin(pf);
>  	if (pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825) {
> -		ice_dpll_deinit_txclk_pins(pf);
> +		if (ice_pf_src_tmr_owned(pf) &&
> +		    test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags))
> +			ice_dpll_deinit_tspll_pins(pf);
> +		ice_dpll_deinit_txclk_pins(pf, true);
>  		ice_dpll_deinit_fwnode_pins(pf, pf->dplls.inputs, 0);
>  	}

[Severity: Medium]
Is it safe to decide this teardown from re-evaluated runtime state rather than
from the pin object itself?

ice_dpll_stop_fwnode_pin_activity() dereferences pin->pf with no NULL check:

	unregister_dpll_notifier(&pin->nb);
	if (flush)
		flush_workqueue(pin->pf->dplls.wq);

and tspll_in.pf / tspll_in.nb are populated only inside
ice_dpll_init_tspll_pins() (unlike txclks and inputs, whose pf is assigned
unconditionally in ice_dpll_init_info_txclk_pins_e825c() and
ice_dpll_init_info_direct_pins()).

ice_pf_src_tmr_owned() reads pf->hw.func_caps.ts_func_info.src_tmr_owned,
which ice_rebuild() re-populates from firmware via ice_get_caps() while the
driver stays loaded.  If the gate was false at init and true here, does this
oops on flush_workqueue(pin->pf->dplls.wq) and unregister a zeroed
notifier_block?  In the inverse case, is the registered notifier and the
fwnode pin reference leaked past unload?  Guarding on tspll_in.fwnode or
tspll_in.pf would make init and deinit symmetric.

[ ... ]

> @@ -4022,6 +4260,45 @@ static int ice_dpll_init_txclk_pins(struct ice_pf *pf, int start_idx)
>  	return ret;
>  }
>  
> +static int ice_dpll_init_tspll_pins(struct ice_pf *pf)
> +{
> +	struct ice_dpll_pin *time_ref = &pf->dplls.tspll_in;
> +	struct ice_dpll *tp = &pf->dplls.tspll;
> +	int ret;
> +
> +	time_ref->pf = pf;
> +	ret = ice_dpll_init_fwnode_pin(time_ref, ice_dpll_fwnode_time_ref);
> +	if (ret)
> +		return ret;

[ ... ]

> @@ -4237,6 +4530,7 @@ static void ice_dpll_deinit_worker(struct ice_pf *pf)
>  
>  	kthread_cancel_delayed_work_sync(&d->work);
>  	kthread_destroy_worker(d->kworker);
> +	d->kworker = NULL;
>  }

[ ... ]

> @@ -4861,9 +5161,10 @@ void ice_dpll_deinit(struct ice_pf *pf)
>  		ice_dpll_deinit_dpll(pf, &pf->dplls.eec, cgu);
>  	if (!IS_ERR_OR_NULL(pf->dplls.txc.dpll))
>  		ice_dpll_deinit_dpll(pf, &pf->dplls.txc, false);
> +	if (!IS_ERR_OR_NULL(pf->dplls.tspll.dpll))
> +		ice_dpll_deinit_dpll(pf, &pf->dplls.tspll, false);
>  
>  	ice_dpll_deinit_info(pf);
> -	mutex_destroy(&pf->dplls.lock);
>  }

[Severity: Medium]
Can pf->dplls.tspll.dpll be stale here?  ice_dpll_deinit_dpll() puts the
reference but never clears the pointer:

	if (cgu || pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825)
		dpll_device_unregister(d->dpll, d->ops, d);
	dpll_device_put(d->dpll, &d->tracker);

pf (and pf->dplls) survives an unload/load cycle via devlink reload, and the
TSPLL device is created only when ice_pf_src_tmr_owned(pf) &&
ICE_FLAG_PTP_SUPPORTED holds.  If that gate is false on a later cycle, does
this predicate still see the already-put pointer from the previous cycle and
call dpll_device_unregister()/dpll_device_put() on a device it no longer owns?

The same patch adds d->kworker = NULL in ice_dpll_deinit_worker() precisely so
the new if (pf->dplls.kworker) predicate is truthful - would a matching
d->dpll = NULL be appropriate?

[ ... ]

> @@ -4904,9 +5202,30 @@ static void ice_dpll_init_e825(struct ice_pf *pf)
>  	err = ice_dpll_init_info_e825c(pf);
>  	if (err)
>  		goto err_exit;
> +	if (ice_pf_src_tmr_owned(pf) &&
> +	    test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags)) {
> +		err = ice_dpll_init_dpll(pf, &pf->dplls.tspll, false,
> +					 DPLL_TYPE_GENERIC);
> +		if (err)
> +			goto deinit_info;

[ ... ]

> +		err = ice_dpll_init_worker(pf);
> +		if (err)
> +			goto deinit_tspll;
> +	}
>  	err = ice_dpll_init_dpll(pf, &pf->dplls.txc, false, DPLL_TYPE_GENERIC);
>  	if (err)
> -		goto deinit_info;
> +		goto deinit_worker;
>  	err = ice_dpll_init_pins_e825(pf);
>  	if (err)
>  		goto deinit_txclk;
> @@ -4917,10 +5236,15 @@ static void ice_dpll_init_e825(struct ice_pf *pf)
>  
>  deinit_txclk:
>  	ice_dpll_deinit_dpll(pf, &pf->dplls.txc, false);
> +deinit_worker:
> +	if (pf->dplls.kworker)
> +		ice_dpll_deinit_worker(pf);
> +deinit_tspll:
> +	if (!IS_ERR_OR_NULL(pf->dplls.tspll.dpll))
> +		ice_dpll_deinit_dpll(pf, &pf->dplls.tspll, false);
>  deinit_info:
>  	ice_dpll_deinit_info(pf);
>  err_exit:
> -	mutex_destroy(&d->lock);
>  	dev_warn(ice_pf_to_dev(pf), "DPLLs init failure err:%d\n", err);
>  }

[ ... ]

> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index d88835482d3a..2f372dd37b53 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -4725,6 +4725,14 @@ static void ice_init_features(struct ice_pf *pf)
>  	if (ice_is_safe_mode(pf))
>  		return;
>  
> +	/* pf->dplls.lock guards TSPLL/CGU access shared between the DPLL
> +	 * subsystem callbacks and the PTP periodic worker's TSPLL monitor.
> +	 * Initialize it before ice_ptp_init() so the PTP kworker never sees
> +	 * an uninitialized mutex, and destroy it in ice_deinit_features()
> +	 * only after ice_ptp_release() has drained the kworker.
> +	 */
> +	mutex_init(&pf->dplls.lock);
> +

[Severity: Low]
Could the kernel-doc for the mutex be updated along with its new owner?

struct ice_dplls in ice_dpll.h still describes @lock as "protects DPLL
configuration (see Locking below)" and states that "@lock serializes all DPLL
state mutations on this PF", with a Locking section that lists only DPLL-side
acquirers.  After this change the mutex is also taken from
ice_ptp_tspll_monitor() in ice_ptp.c, and its lifetime belongs to
ice_init_features()/ice_deinit_features() rather than
ice_dpll_init()/ice_dpll_deinit().

The @tspll_locked kernel-doc in ice_ptp.h also still says the field is
"intended to be read (without pf->dplls.lock) by the DPLL periodic worker in
a follow-up change", while this patch reads it with pf->dplls.lock held.

[ ... ]

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 4b1040d09f70..e54a98a0bdd0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -2878,8 +2878,19 @@ static void ice_ptp_tspll_monitor(struct ice_pf *pf)
>  	    !ice_pf_src_tmr_owned(pf))
>  		return;
>  
> +	/* Serialize the entire monitor tick against TSPLL userspace reconfig
> +	 * (ice_dpll_tspll_state_on_dpll_set()). Both paths read HW state and
> +	 * write pf->ptp.tspll_locked; without holding pf->dplls.lock across
> +	 * the HW read here, a preempted monitor could observe stale HW state
> +	 * and then overwrite an accurate cache update from the DPLL callback.
> +	 * pf->dplls.lock is initialized in ice_init_features() before the PTP
> +	 * kworker starts and destroyed in ice_deinit_features() only after
> +	 * ice_ptp_release() has drained the kworker, so it is always valid.
> +	 */
> +	mutex_lock(&pf->dplls.lock);
>  	err = ice_tspll_lost_lock_e825c(&pf->hw, &lock_lost);

[ ... ]

> diff --git a/drivers/net/ethernet/intel/ice/ice_tspll.c b/drivers/net/ethernet/intel/ice/ice_tspll.c
> index 78752dc1e762..85bacb7c3a79 100644
> --- a/drivers/net/ethernet/intel/ice/ice_tspll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_tspll.c
> @@ -334,8 +334,9 @@ static int ice_tspll_dis_sticky_bits_e82x(struct ice_hw *hw)
>   * Return:
>   * * %0       - success
>   * * %-EINVAL - input parameters are incorrect
> - * * %-EBUSY  - failed to lock TSPLL
> - * * %other   - CGU read/write failure
> + * * %-EAGAIN - TSPLL configuration succeeded but lock is not acquired yet
> + * * %-EBUSY  - CGU access is busy (for example during reset)
> + * * %other   - other CGU read/write failures
>   */
>  static int ice_tspll_cfg_e825c(struct ice_hw *hw, enum ice_tspll_freq clk_freq,
>  			       enum ice_clk_src clk_src)
> @@ -467,7 +468,7 @@ static int ice_tspll_cfg_e825c(struct ice_hw *hw, enum ice_tspll_freq clk_freq,
>  
>  	if (!(val & ICE_CGU_RO_LOCK_TRUE_LOCK)) {
>  		dev_warn(ice_hw_to_dev(hw), "CGU PLL failed to lock\n");
> -		return -EBUSY;
> +		return -EAGAIN;
>  	}

[Severity: Low]
Should ice_tspll_cfg_e82x() and the ice_tspll_cfg() wrapper doc follow this
new convention?  ice_tspll_cfg_e82x() still returns -EBUSY for the identical
"configured but not locked" check on ICE_CGU_RO_BWM_LF_TRUE_LOCK, so the two
implementations behind one wrapper now report different errnos for the same
hardware condition, and the wrapper's kernel-doc still mentions only "other
negative error codes when failed to configure CGU".

The E825-only path means this cannot be hit through ice_tspll_set_cfg() today,
but a future caller reading the wrapper contract could get it wrong.

> +/**
> + * ice_tspll_set_cfg - configure TS PLL with new settings
> + * @hw: board private hw structure
> + * @clk_freq: clock frequency to program
> + * @clk_src: clock source to select (TIME_REF, or TCXO)
> + *
> + * Configure CGU with new clock source and clock frequency settings.
> + *
> + * Return:
> + * * 0 - OK
> + * * negative - error
> + */

Could this kernel-doc list -EAGAIN as well?  Its only caller,
ice_dpll_tspll_state_on_dpll_set(), special-cases that value and maps it to
success, which the "negative - error" wording contradicts.

> +int ice_tspll_set_cfg(struct ice_hw *hw, enum ice_tspll_freq clk_freq,
> +		      enum ice_clk_src clk_src)
> +{
> +	int ret;
> +
> +	if (!ice_tspll_check_params(hw, clk_freq, clk_src))
> +		return -EINVAL;
> +
> +	ret = ice_tspll_dis_sticky_bits(hw);
> +	if (ret)
> +		return ret;
> +
> +	return ice_tspll_cfg(hw, clk_freq, clk_src);
> +}

[Severity: Medium]
What restores the hardware if this sequence fails half way through?

ice_tspll_cfg_e825c() first clears ICE_CGU_R23_R24_TSPLL_ENABLE and
ICE_CGU_R9_TIME_SYNC_EN and re-enables them only at the end.  Any sideband
read or write failure in between returns early with no rollback, so the PLL is
left disabled and TIME_SYNC_EN gated.

The runtime recovery path only touches R23:

ice_tspll_restart_e825c()
	err = ice_read_cgu_reg(hw, ICE_CGU_R23, &val);
	...
	val &= ~ICE_CGU_R23_R24_TSPLL_ENABLE;
	...
	val |= ICE_CGU_R23_R24_TSPLL_ENABLE;
	return ice_write_cgu_reg(hw, ICE_CGU_R23, val);

so ICE_CGU_R9_TIME_SYNC_EN is never restored.  Can the PLL then re-lock,
ice_ptp_tspll_monitor() log "lock acquired" and set tspll_locked = true, and
the new TSPLL DPLL device publish LOCKED, while the 1588 clock feeding the
source timer stays gated and the PHC is frozen?  Before this patch the
sequence ran only from ice_tspll_init() at probe/reset, where failure aborts
PTP init.

[Severity: Medium]
Can this collide with the E825 periodic-output configuration on ICE_CGU_R9?

ice_tspll_cfg_e825c() reads R9 at the start and writes the whole register back
several sideband transactions later, while ice_tspll_cfg_pps_out_e825c() does
its own full-register read-modify-write of the 1PPS fields:

	err = ice_read_cgu_reg(hw, ICE_CGU_R9, &val);
	...
	val &= ~(ICE_CGU_R9_ONE_PPS_OUT_EN | ICE_CGU_R9_ONE_PPS_OUT_AMP);
	...
	return ice_write_cgu_reg(hw, ICE_CGU_R9, val);

That path is reached from ice_ptp_write_perout() on the ptp_clock ioctl and
does not take pf->dplls.lock.  Can a perout enable be erased by the
source-change write, or a perout write based on the pre-change R9 snapshot
restore the old TIME_REF_EN/CLK_EREF0_EN/TIME_SYNC_EN after R23 has already
switched TIME_REF_SEL, leaving receiver and PLL source inconsistent?

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