[PATCH] net: lan743x_ptp: convert to ktime_get_clocktai_ts64

[PATCH] net: lan743x_ptp: convert to ktime_get_clocktai_ts64

[Arnd Bergmann]

timekeeping_clocktai64() has been renamed to ktime_get_clocktai_ts64()
for consistency with the other ktime_get_* access functions.

Rename the new caller that has come up as well.

Question: this is the only ptp driver that sets the hardware time
to the current system time in TAI. Why does it do that?

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/ethernet/microchip/lan743x_ptp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_ptp.c b/drivers/net/ethernet/microchip/lan743x_ptp.c
index 029a2af90d5e..0e851fa3e0cc 100644
--- a/drivers/net/ethernet/microchip/lan743x_ptp.c
+++ b/drivers/net/ethernet/microchip/lan743x_ptp.c
@@ -832,8 +832,7 @@ static void lan743x_ptp_sync_to_system_clock(struct lan743x_adapter *adapter)
 {
 	struct timespec64 ts;
 
-	memset(&ts, 0, sizeof(ts));
-	timekeeping_clocktai64(&ts);
+	ktime_get_clocktai_ts64(&ts);
 
 	lan743x_ptp_clock_set(adapter, ts.tv_sec, ts.tv_nsec, 0);
 }
-- 
2.18.0

RE: [PATCH] net: lan743x_ptp: convert to ktime_get_clocktai_ts64

[Bryan.Whitehead]

> 
> Question: this is the only ptp driver that sets the hardware time to the
> current system time in TAI. Why does it do that?

This is done when the driver starts up after reset. Otherwise the clock is off by 48 years. 
It seemed to me that the system time was the most appropriate clock to sync to. 
If my reasoning is incorrect, please enlighten me.

Thanks,
Bryan

Re: [PATCH] net: lan743x_ptp: convert to ktime_get_clocktai_ts64

[Arnd Bergmann]

On Wed, Aug 15, 2018 at 8:03 PM <Bryan.Whitehead@microchip.com> wrote:
>
> >
> > Question: this is the only ptp driver that sets the hardware time to the
> > current system time in TAI. Why does it do that?
>
> This is done when the driver starts up after reset. Otherwise the clock is off by 48 years.
> It seemed to me that the system time was the most appropriate clock to sync to.
> If my reasoning is incorrect, please enlighten me.

I've never worked with PTP, but my understanding from looking at the other
drivers is that the time normally gets set either from another host through the
PTP protocol, or using clock_settime() from user space with the current time.

       Arnd

RE: [PATCH] net: lan743x_ptp: convert to ktime_get_clocktai_ts64

[Bryan.Whitehead]

> > > Question: this is the only ptp driver that sets the hardware time to
> > > the current system time in TAI. Why does it do that?
> >
> > This is done when the driver starts up after reset. Otherwise the clock is off
> by 48 years.
> > It seemed to me that the system time was the most appropriate clock to
> sync to.
> > If my reasoning is incorrect, please enlighten me.
> 
> I've never worked with PTP, but my understanding from looking at the other
> drivers is that the time normally gets set either from another host through
> the PTP protocol, or using clock_settime() from user space with the current
> time.

Those methods will still work. But if it's not set by those methods, I thought the clock should at least be set once on driver startup to align with the system clock. After that, other methods are free to reset it again.

Bryan

Re: [PATCH] net: lan743x_ptp: convert to ktime_get_clocktai_ts64

[Arnd Bergmann]

On Wed, Aug 15, 2018 at 10:41 PM <Bryan.Whitehead@microchip.com> wrote:
>
> > > > Question: this is the only ptp driver that sets the hardware time to
> > > > the current system time in TAI. Why does it do that?
> > >
> > > This is done when the driver starts up after reset. Otherwise the clock is off
> > by 48 years.
> > > It seemed to me that the system time was the most appropriate clock to
> > sync to.
> > > If my reasoning is incorrect, please enlighten me.
> >
> > I've never worked with PTP, but my understanding from looking at the other
> > drivers is that the time normally gets set either from another host through
> > the PTP protocol, or using clock_settime() from user space with the current
> > time.
>
> Those methods will still work. But if it's not set by those methods, I thought the
> clock should at least be set once on driver startup to align with the system clock.
> After that, other methods are free to reset it again.

(adding Richard Cochran to Cc for more insight here)

I would argue that it's more important that the driver behaves like all other
PTP implementations. If we want the behavior to be that the initial PTP time is
set to the ktime_get_clocktai_ts64() value, then this should be done by the
PTP core rather than the device driver. If there is a good reason that the
other drivers don't do it like this, then I would assume the same reason applies
to lan743x.

        Arnd

RE: [PATCH] net: lan743x_ptp: convert to ktime_get_clocktai_ts64

[Bryan.Whitehead]

> > > > > Question: this is the only ptp driver that sets the hardware
> > > > > time to the current system time in TAI. Why does it do that?
> > > >
> > > > This is done when the driver starts up after reset. Otherwise the
> > > > clock is off
> > > by 48 years.
> > > > It seemed to me that the system time was the most appropriate
> > > > clock to
> > > sync to.
> > > > If my reasoning is incorrect, please enlighten me.
> > >
> > > I've never worked with PTP, but my understanding from looking at the
> > > other drivers is that the time normally gets set either from another
> > > host through the PTP protocol, or using clock_settime() from user
> > > space with the current time.
> >
> > Those methods will still work. But if it's not set by those methods, I
> > thought the clock should at least be set once on driver startup to align with
> the system clock.
> > After that, other methods are free to reset it again.
> 
> (adding Richard Cochran to Cc for more insight here)
> 
> I would argue that it's more important that the driver behaves like all other
> PTP implementations. If we want the behavior to be that the initial PTP time
> is set to the ktime_get_clocktai_ts64() value, then this should be done by the
> PTP core rather than the device driver. If there is a good reason that the
> other drivers don't do it like this, then I would assume the same reason
> applies to lan743x.
> 

Sounds reasonable to me. I will yield to Richard's insight.
But it would be nice if requirements like these were documented.

Re: [PATCH] net: lan743x_ptp: convert to ktime_get_clocktai_ts64

[Richard Cochran]

On Wed, Aug 15, 2018 at 08:50:03PM +0000, Bryan.Whitehead@microchip.com wrote:
> Sounds reasonable to me. I will yield to Richard's insight.
> But it would be nice if requirements like these were documented.

I think the only reason for initializing to the system UTC (as most
drivers do) is historical.  The first Intel driver was simply copied.
I've been thinking recently that we should standardize this.  I'm open
for suggestions on how to do this. Remember that the system time is
likely wrong at driver initialization time.

Thanks,
Richard

Re: [PATCH] net: lan743x_ptp: convert to ktime_get_clocktai_ts64

[Arnd Bergmann]

On Fri, Aug 17, 2018 at 6:26 PM Richard Cochran
<richardcochran@gmail.com> wrote:
>
> On Wed, Aug 15, 2018 at 08:50:03PM +0000, Bryan.Whitehead@microchip.com wrote:
> > Sounds reasonable to me. I will yield to Richard's insight.
> > But it would be nice if requirements like these were documented.
>
> I think the only reason for initializing to the system UTC (as most
> drivers do) is historical.  The first Intel driver was simply copied.
> I've been thinking recently that we should standardize this.  I'm open
> for suggestions on how to do this. Remember that the system time is
> likely wrong at driver initialization time.

Ah, so you mean the other drivers in fact do initialize the hardware clock,
they just set it to UTC instead of TAI? I must have looked wrong then,
but I see it now in most implementations (exceptions include ravb, sfc
and stmmac).

This certainly seems to be an "interesting" problem, given that the Linux
implementations (other than the new lan743x) then start out with UTC,
while the PTP spec mandates TAI. So even if the system clock is
perfectly synchronized to UTC at boot, we set the PTP hardware 37
seconds slow.

It would not be hard to change all PTP drivers to explicitly initialize to
TAI, but that might create its own set of problems if random code
depends on the current behavior.

I also see that "phc_ctl /dev/ptp0 set" defaults to CLOCK_REALTIME
and has no option to use CLOCK_TAI instead. How is this meant to
work? I see a lot of other code that tries to deal with leap seconds and
the tai offset, so I hope I was just misreading that code.

       Arnd

Re: [PATCH] net: lan743x_ptp: convert to ktime_get_clocktai_ts64

[Richard Cochran]

On Fri, Aug 17, 2018 at 09:29:56PM +0200, Arnd Bergmann wrote:
> This certainly seems to be an "interesting" problem, given that the Linux
> implementations (other than the new lan743x) then start out with UTC,
> while the PTP spec mandates TAI. So even if the system clock is
> perfectly synchronized to UTC at boot,

When the system boots, it is not synchronized.  Only after ntpd or
ptp4l start their work can we say that.

> we set the PTP hardware 37 
> seconds slow.

s/slow/behind/
 
> It would not be hard to change all PTP drivers to explicitly initialize to
> TAI, but that might create its own set of problems if random code
> depends on the current behavior.

Right. (But there was never any guarantee.)

Also, devices that don't have an RTC (like many embedded) start with
1970 anyhow.  So you really can't have "correct" time at boot.  The
question is, which incorrect time would you prefer?

IHMO a clearly wrong value is more useful than one that might be
mistaken as correct by a casual glance from the sysadmin.

> I also see that "phc_ctl /dev/ptp0 set" defaults to CLOCK_REALTIME
> and has no option to use CLOCK_TAI instead. How is this meant to
> work? I see a lot of other code that tries to deal with leap seconds and
> the tai offset, so I hope I was just misreading that code.

You *can* set UTC and then jump the clock by 37 in two steps.

But I don't see an simple solution for setting the TAI-UTC offset at
boot without actually consulting an outside source.  Even if you have
the NTP leap seconds file, how does the system know the file is up to
date?

For PTP and PHC devices, there are two use cases.

1. The device is a slave.  In this case, applications need to wait
   until the PTP status bits say that the time is valid.  The invalid
   time before shouldn't be trusted at all.

2. The device is a grand master.  In this case, the PTP stack has to
   wait until its global time source (like GPS) is ready.  Then it
   will synchronize the local PHC to the global source, and only then
   should it advertise the valid bits.

So I don't think the particular kind of wrong start-up value makes
much difference in practice.

You could argue that if

  a) the system has an RTC, and
  b) the battery isn't dead, and
  c) there is a leap seconds file that isn't out of date,

then the initial PHC time will be less wrong (for some definition of
wrong) using TAI than if the driver had used UTC or 1970.

I myself can't get too excited about having "less wrong" time, but I
wouldn't mind trying to set TAI in the PHC layer if people feel
strongly about it.

Thanks,
Richard

Re: [PATCH] net: lan743x_ptp: convert to ktime_get_clocktai_ts64

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, 15 Aug 2018 19:49:49 +0200

> timekeeping_clocktai64() has been renamed to ktime_get_clocktai_ts64()
> for consistency with the other ktime_get_* access functions.
> 
> Rename the new caller that has come up as well.
> 
> Question: this is the only ptp driver that sets the hardware time
> to the current system time in TAI. Why does it do that?
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Deciding whether PTP drivers should set the hardware time at boot
to the current system time is a separate discussion from using
the new name for the timekeeping_clocktai64() interface, I'm applying
this.

Thanks Arnd.