Re: [PATCH net-next 01/10] ice: dpll: notify TSPLL lock status from the periodic work only

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch leaves the comments in
  ice_dpll_tspll_state_on_dpll_set()…

--

ice: dpll: notify TSPLL lock status from the periodic work only

This patch stops ice_dpll_tspll_state_on_dpll_set() from sending a TSPLL
device change notification from the TIME_REF pin callback. The callback
now only updates dpll_state. ice_dpll_periodic_work_e825() refreshes
dpll_state on every run and sends the notification when the state differs
from prev_dpll_state.

> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
> index ade1fb3dd0674..bdbcf7af296eb 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c

[ ... ]

> @@ -2947,15 +2946,12 @@ ice_dpll_tspll_state_on_dpll_set(const struct dpll_pin *pin, void *pin_priv,
>  		WRITE_ONCE(pf->ptp.tspll_locked,
>  			   new_state == DPLL_LOCK_STATUS_LOCKED);
>  
> -		if (tp->prev_dpll_state != new_state) {
> -			tp->dpll_state = new_state;
> -			tp->prev_dpll_state = new_state;
> -			changed = true;
> -		}
> +		/* ice_dpll_periodic_work_e825() compares the state with
> +		 * prev_dpll_state and sends the notification.
> +		 */
> +		tp->dpll_state = new_state;

[Severity: Low]
This isn't a bug, but do the comments in ice_dpll_tspll_state_on_dpll_set()
still match how the notification path works after this change?

The new comment reads as if the periodic work compares the tp->dpll_state
written here. In ice_dpll_periodic_work_e825() that value is never read.
The work recomputes the state from the cache and overwrites dpll_state
before it compares:

ice_dpll_periodic_work_e825() {
	...
	new_state = ice_dpll_tspll_lock_status_get(pf, true);

	tp->dpll_state = new_state;
	if (tp->prev_dpll_state != new_state) {
	...
}

The older comment just above the WRITE_ONCE() was not changed:

	/* Sync the cached lock state to what we just observed from HW
	 * so ice_dpll_periodic_work_e825() (cached path) does not emit
	 * a spurious UNLOCKED notification between now and the next
	 * PTP monitor tick.
	 */

That reason applied when the callback also set prev_dpll_state. Now that
it doesn't, the WRITE_ONCE() to pf->ptp.tspll_locked is the only way the
callback's HW reading gets to the periodic work.

Without it, take a switch from TCXO (prev_dpll_state is UNLOCKED) to
TIME_REF where the HW reports LOCKED but the cache still holds false. The
next periodic run would compute UNLOCKED, which matches prev_dpll_state.
No notification would be sent, and dpll_state would go back to UNLOCKED.
The result would be a lost notification, not a spurious one.

Could both comments say that the periodic work picks up the new state
through pf->ptp.tspll_locked? Then a later change is less likely to drop
the WRITE_ONCE() as just a way to avoid spurious notifications. As far as
I can tell, the comments are still unchanged at the end of the series,
after "ice: dpll: update locking comments".

>  	}
>  	mutex_unlock(&pf->dplls.lock);
> -	if (changed)
> -		__dpll_device_change_ntf(tp->dpll);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106121005.1327603-1-ivecera%40redhat.com