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

[Nitka, Grzegorz]

> -----Original Message-----
> From: Nitka, Grzegorz <grzegorz.nitka@xxxxxxxxx>
> Sent: Tuesday, July 28, 2026 11:13 AM
> To: intel-wired-lan@xxxxxxxxxxxxxxxx
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> richardcochran@xxxxxxxxx; andrew+netdev@xxxxxxx; Kitszel, Przemyslaw
> <przemyslaw.kitszel@xxxxxxxxx>; Nguyen, Anthony L
> <anthony.l.nguyen@xxxxxxxxx>; Kubalewski, Arkadiusz
> <arkadiusz.kubalewski@xxxxxxxxx>; pabeni@xxxxxxxxxx; kuba@xxxxxxxxxx;
> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; Nitka, Grzegorz
> <grzegorz.nitka@xxxxxxxxx>; Korba, Przemyslaw
> <przemyslaw.korba@xxxxxxxxx>
> Subject: [PATCH 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.
> 
> 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 <przemyslaw.korba@xxxxxxxxx>
> Signed-off-by: Grzegorz Nitka <grzegorz.nitka@xxxxxxxxx>
> ---
>  drivers/net/ethernet/intel/ice/ice_dpll.c  | 398 +++++++++++++++++++--
>  drivers/net/ethernet/intel/ice/ice_dpll.h  |   4 +
>  drivers/net/ethernet/intel/ice/ice_ptp.c   |  10 +
>  drivers/net/ethernet/intel/ice/ice_tspll.c |  34 +-
>  drivers/net/ethernet/intel/ice/ice_tspll.h |   2 +
>  5 files changed, 409 insertions(+), 39 deletions(-)
> 

[...]
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
> b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index a997be5f7d8f..cbd9f6455c05 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -2898,6 +2898,7 @@ static void
> ice_ptp_maybe_trigger_tx_interrupt(struct ice_pf *pf)
>  static void ice_ptp_tspll_monitor(struct ice_pf *pf)
>  {
>  	bool lock_lost;
> +	bool dpll_ready;
>  	int err;
> 
>  	if (pf->hw.mac_type != ICE_MAC_GENERIC_3K_E825 ||
> @@ -2917,7 +2918,16 @@ static void ice_ptp_tspll_monitor(struct ice_pf
> *pf)
>  		if (!(pf->ptp.tspll_lock_retries % ICE_TSPLL_LOG_INTERVAL))
>  			dev_warn(ice_pf_to_dev(pf),
>  				 "TimeSync PLL lock lost. Retrying to acquire
> lock.\n");
> +		/* Serialize R23 restart against TSPLL userspace reconfig.
> +		 * Guard with ICE_FLAG_DPLL because init failure may
> already
> +		 * have destroyed pf->dplls.lock on the error path.
> +		 */
> +		dpll_ready = test_bit(ICE_FLAG_DPLL, pf->flags);
> +		if (dpll_ready)
> +			mutex_lock(&pf->dplls.lock);
>  		err = ice_tspll_restart_e825c(&pf->hw);
> +		if (dpll_ready)
> +			mutex_unlock(&pf->dplls.lock);
>  		if (err)
>  			dev_err_ratelimited(ice_pf_to_dev(pf),
>  					    "Failed to restart TimeSync PLL (err:
> %d).\n",
About Sashiko comment from:
https://sashiko.dev/#/patchset/20260728091314.1420656-1-grzegorz.nitka%40intel.com
"Does this code introduce a TOCTOU race leading to hardware state corruption?
During initialization in ice_dpll_init_e825(), there is a window where the
TSPLL DPLL device and pins are fully registered and exposed to userspace
before ICE_FLAG_DPLL is set.
If userspace reconfigures the TSPLL during this window,
ice_dpll_tspll_state_on_dpll_set() (in drivers/net/ethernet/intel/ice/ice_dpll.c)
will acquire pf->dplls.lock and modify the hardware."

In general, this concern is valid however limited only to very narrow window at
the driver initialization. I think the best way to fix it is to move dpll.lock
init/destroy outside of dpll_init/deinit procedures and handle it in the driver
probe/remove routines.  This way, the flag guard is eliminated, so the monitor
code becomes straightforward: "take the mutex, restart the PLL, drop the mutex."
[...]
> --
> 2.39.3