Bug 259036

Summary:	Add PPS_SYNC to GENERIC kernel configuration
Product:	Base System	Reporter:	John Hay <john>
Component:	conf	Assignee:	Konstantin Belousov <kib>
Status:	Closed FIXED
Severity:	Affects Some People	CC:	ian, imp, kib, phk
Priority:	---	Keywords:	easy, feature
Version:	Unspecified	Flags:	koobs: maintainer-feedback+ koobs: maintainer-feedback+ koobs: mfc-stable13? koobs: mfc-stable12?
Hardware:	Any
OS:	Any

Description John Hay 2021-10-10 06:29:38 UTC

For those of us using a GPS or other form of PPS signal with ntpd, the only reason to recompile the kernel is to add the PPS_SYNC option. Other FreeBSD derived projects like pfsense already have it in by default.

There are only two files affected by this option, kern_ntptime.c and kern_tc.c. (kern_clock.c also includes opt_ntp.h, but does not use PPS_SYNC.)

It adds very little code. Last time somebody counted, it was about 218 lines.

In kern_tc.c it only adds code to implement the PPS_IOC_KCBIND ioctl and adds code to pps_event(). Both of these code paths will only be used by people using PPS.

If one has to select only one architecture, it would be amd64, but the different arm kernels would also be very useful.

Comment 1 Poul-Henning Kamp freebsd_committer

2021-10-10 13:55:36 UTC

OK with me.

You may want to check with imp@ and gnn@ too?

Comment 2 Konstantin Belousov freebsd_committer

2021-10-10 14:23:31 UTC

I will commit the following after tinderbox finishes.

commit 3822304620c392fbef3e7bee197bf1f3b0860aed
Author: Konstantin Belousov <kib@FreeBSD.org>
Date:   Sun Oct 10 15:20:45 2021 +0300

    Enable PPS_SYNC on amd64, arm64 and armv7.
    
    Remove the option from NOTES/LINT.
    
    PR:     259036
    Requested by:   John Hay <john@sanren.ac.za>
    Sponsored by:   The FreeBSD Foundation
    MFC after:      1 week

diff --git a/sys/amd64/conf/GENERIC b/sys/amd64/conf/GENERIC
index 40c14e90aef0..f47b995beb2c 100644
--- a/sys/amd64/conf/GENERIC
+++ b/sys/amd64/conf/GENERIC
@@ -138,6 +138,12 @@ options		PCI_IOV			# PCI SR-IOV support
 
 options 	COMPAT_LINUXKPI
 
+# Enable support for the kernel PLL to use an external PPS signal,
+# under supervision of [x]ntpd(8)
+# More info in ntpd documentation: http://www.eecis.udel.edu/~ntp
+
+options 	PPS_SYNC
+
 # Floppy drives
 device		fdc
 
diff --git a/sys/arm/conf/std.armv7 b/sys/arm/conf/std.armv7
index 2d1dfa135780..fda291df289a 100644
--- a/sys/arm/conf/std.armv7
+++ b/sys/arm/conf/std.armv7
@@ -84,3 +84,9 @@ options 	VERBOSE_SYSINIT=0	# Support debug.verbose_sysinit, off by default
 #options 	KTR_VERBOSE=0
 #options 	USB_REQ_DEBUG
 #options 	USB_VERBOSE
+
+# Enable support for the kernel PLL to use an external PPS signal,
+# under supervision of [x]ntpd(8)
+# More info in ntpd documentation: http://www.eecis.udel.edu/~ntp
+
+options 	PPS_SYNC
diff --git a/sys/arm64/conf/std.arm64 b/sys/arm64/conf/std.arm64
index 8460d449939d..5c26f19b4299 100644
--- a/sys/arm64/conf/std.arm64
+++ b/sys/arm64/conf/std.arm64
@@ -96,3 +96,9 @@ options 	NETDUMP			# netdump(4) client support
 
 # Make an SMP-capable kernel by default
 options 	SMP			# Symmetric MultiProcessor Kernel
+
+# Enable support for the kernel PLL to use an external PPS signal,
+# under supervision of [x]ntpd(8)
+# More info in ntpd documentation: http://www.eecis.udel.edu/~ntp
+
+options 	PPS_SYNC
diff --git a/sys/conf/NOTES b/sys/conf/NOTES
index 39446fba06b3..e41b00edf2f6 100644
--- a/sys/conf/NOTES
+++ b/sys/conf/NOTES
@@ -1245,12 +1245,6 @@ options 	CAPABILITY_MODE	# sandboxes with no global namespace access
 
 options 	HZ=100
 
-# Enable support for the kernel PLL to use an external PPS signal,
-# under supervision of [x]ntpd(8)
-# More info in ntpd documentation: http://www.eecis.udel.edu/~ntp
-
-options 	PPS_SYNC
-
 # Enable support for generic feed-forward clocks in the kernel.
 # The feed-forward clock support is an alternative to the feedback oriented
 # ntpd/system clock approach, and is to be used with a feed-forward

Comment 3 Warner Losh freebsd_committer

2021-10-10 15:34:04 UTC

Removing it from NOTES means it won't build on powerpc and riscv64 at all. Are you sure that's what you want to do?

A quick visual inspection suggests that this is fine. It does make time keeping a tiny bit more expensive, and the 'tiny bit' has gotten to be so small in the !pps case that this is fine.

I'd be tempted to hold off committing until we hear from Ian, since he's the person most likely to know of issues compiling this in and then not feeding it a PPS. I don't see any, but this area of the tree has subtle side effects often enough that hearing from him, if he's not unduly delayed, would be prudent imho.

Comment 4 Konstantin Belousov freebsd_committer

2021-10-10 15:45:18 UTC

I think this option has no practical use on ppc or riscv.

I will wait for Ian to respond, for several days.

Comment 5 Warner Losh freebsd_committer

2021-10-10 15:48:07 UTC

(In reply to Konstantin Belousov from comment #4)
We should keep it compiling, at least for riscv64. That's what NOTES is for. GPS devices do exist that you can connect to current riscv64 hardware, so it's relevant for that reason. I don't think any of them have the necessary FreeBSD support just yet, though. Adding it to at least sys/riscv64/conf/NOTES seems an easy way to keep this future proofed...

Comment 6 Konstantin Belousov freebsd_committer

2021-10-10 15:55:22 UTC

(In reply to Warner Losh from comment #5)
I added the option both to powerpc and riscv NOTES.

Comment 7 Ian Lepore freebsd_committer

2021-10-10 17:06:25 UTC

I don't know of any downside to having OPTION PPS in the kernel and not using the feature other than a bit of wasted code space (which probably doesn't add up to more than 1-2kbytes).

I don't understand the comment about PPS not being useful on ppc or riscv.  You can feed a PPS pulse to a system on any gpio pin or serial port, including usb-serial adapters.  If you use ftdi adapters you can get accuracies in the +- 50us range, cheaper brands still get you millisecond accuracy which is as good as a network connection).

Comment 8 commit-hook freebsd_committer

2021-10-10 19:35:55 UTC

A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=e81e77c5a055d1cbf6d6a6f0acbaf443267aa84f

commit e81e77c5a055d1cbf6d6a6f0acbaf443267aa84f
Author:     Konstantin Belousov <kib@FreeBSD.org>
AuthorDate: 2021-10-10 12:20:45 +0000
Commit:     Konstantin Belousov <kib@FreeBSD.org>
CommitDate: 2021-10-10 19:34:40 +0000

    Enable PPS_SYNC on amd64, arm64 and armv7

    Remove the option from NOTES/LINT, and add to NOTES for powerpc and
    riscv.

    PR:     259036
    Requested by:   John Hay <john@sanren.ac.za>
    Discussed with: ian, imp
    Sponsored by:   The FreeBSD Foundation
    MFC after:      1 week

 sys/amd64/conf/GENERIC   | 6 ++++++
 sys/arm/conf/std.armv7   | 6 ++++++
 sys/arm64/conf/std.arm64 | 6 ++++++
 sys/conf/NOTES           | 6 ------
 sys/powerpc/conf/NOTES   | 2 ++
 sys/riscv/conf/NOTES     | 2 ++
 6 files changed, 22 insertions(+), 6 deletions(-)

Comment 9 Kubilay Kocak freebsd_committer

2021-10-10 23:10:28 UTC

^Triage: Assign to committer resolving. If there's additional feedback still desired, please add using maintainer-feedback ? <email>

Thank you for the tight turnaround everyone

Comment 10 commit-hook freebsd_committer

2021-10-17 00:29:08 UTC

A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=c683063a00d0ff37afd16f7baf6d0a5e84e3a18f

commit c683063a00d0ff37afd16f7baf6d0a5e84e3a18f
Author:     Konstantin Belousov <kib@FreeBSD.org>
AuthorDate: 2021-10-10 12:20:45 +0000
Commit:     Konstantin Belousov <kib@FreeBSD.org>
CommitDate: 2021-10-17 00:28:13 +0000

    Enable PPS_SYNC on amd64, arm64 and armv7

    PR:     259036

    (cherry picked from commit e81e77c5a055d1cbf6d6a6f0acbaf443267aa84f)

 sys/amd64/conf/GENERIC | 6 ++++++
 sys/arm/conf/std.armv7 | 6 ++++++
 sys/arm64/conf/GENERIC | 2 ++
 sys/conf/NOTES         | 6 ------
 sys/powerpc/conf/NOTES | 2 ++
 sys/riscv/conf/NOTES   | 2 ++
 6 files changed, 18 insertions(+), 6 deletions(-)