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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 6 Jul 2020 18:27:21 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Sergey Organov <sorganov@...il.com>, richardcochran@...il.com
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Fugang Duan <fugang.duan@....com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH  3/5] net: fec: initialize clock with 0 rather than
 current kernel time

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@...il.com>
> ---

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