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

[Ivan Vecera <ivecera@redhat.com>]

On 11/10/26 18:53, netdev-bot+sashiko@kernel.org wrote:
> 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?

[...]

> 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.
Right, the first comment still gives the reason from before this patch.
Now the write of pf->ptp.tspll_locked is what brings the new state to
ice_dpll_periodic_work_e825(). v2 will merge both comments into one that
says so.

Thanks,
Ivan