Re: [PATCH 3/5] net: fec: initialize clock with 0 rather than current kernel time

[Vladimir Oltean]

Hi Sergey,

On Mon, Jul 06, 2020 at 05:26:14PM +0300, Sergey Organov wrote:
> Initializing with 0 makes it much easier to identify time stamps from
> otherwise uninitialized clock.
> 
> Initialization of PTP clock with current kernel time makes little sense as
> PTP time scale differs from UTC time scale that kernel time represents.
> It only leads to confusion when no actual PTP initialization happens, as
> these time scales differ in a small integer number of seconds (37 at the
> time of writing.)
> 
> Signed-off-by: Sergey Organov <sorganov@xxxxxxxxx>
> ---

Reading your patch, I got reminded of my own attempt of making an
identical change to the ptp_qoriq driver:

https://www.spinics.net/lists/netdev/msg601625.html

Could we have some sort of kernel-wide convention, I wonder (even though
it might be too late for that)? After your patch, I can see equal
amounts of confusion of users expecting some boot-time output of
$(phc_ctl /dev/ptp0 get) as it used to be, and now getting something
else.

There's no correct answer, I'm afraid. Whatever the default value of the
clock may be, it's bound to be confusing for some reason, _if_ the
reason why you're investigating it in the first place is a driver bug.
Also, I don't really see how your change to use Jan 1st 1970 makes it
any less confusing.

>  drivers/net/ethernet/freescale/fec_ptp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 4a12086..e455343 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -264,7 +264,7 @@ void fec_ptp_start_cyclecounter(struct net_device *ndev)
>  	fep->cc.mult = FEC_CC_MULT;
>  
>  	/* reset the ns time counter */
> -	timecounter_init(&fep->tc, &fep->cc, ktime_to_ns(ktime_get_real()));
> +	timecounter_init(&fep->tc, &fep->cc, 0);
>  
>  	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
>  }
> -- 
> 2.10.0.1.g57b01a3
> 

Thanks,
-Vladimir