| Message ID | 20180815175040.3736548-1-arnd@arndb.de |
|---|---|
| State | Accepted |
| Commit | 0b3e776e2e809b808875b10e35d1047c5c548da4 |
| Headers | show |
| Series | net: lan743x_ptp: convert to ktime_get_clocktai_ts64 | expand |
> > 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
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
> > > 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
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
> > > > > 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.
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
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
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
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.
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); }
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(-) -- 2.18.0