Skip to content

linuxptp: add config & init scripts for ptp4l and phc2sys - #30279

Open
aleks-mariusz wants to merge 3 commits into
openwrt:masterfrom
aleks-mariusz:linuxptp-config
Open

linuxptp: add config & init scripts for ptp4l and phc2sys#30279
aleks-mariusz wants to merge 3 commits into
openwrt:masterfrom
aleks-mariusz:linuxptp-config

Conversation

@aleks-mariusz

Copy link
Copy Markdown
Contributor

This adds UCI configuration and init scripts for the ptp4l and phc2sys
daemons included in the linuxptp package, addressing #26546.

What's included

  • /etc/config/linuxptp - UCI config with sections for ptp4l,
    ptp_interface (repeatable, one per NIC), and phc2sys
  • /etc/init.d/ptp4l - procd init script; generates ptp4l.conf
    dynamically from UCI, supports auto/client/server roles
  • /etc/init.d/phc2sys - procd init script; auto-selects sync
    direction based on ptp4l's configured role
  • sysclock-discipline-check - small C utility (compiled and
    installed to /usr/sbin/) that queries adjtimex() to check whether
    the kernel considers the system clock synchronized. Used to delay ptp4l
    startup in server mode on systems without an RTC (prevents
    broadcasting wildly incorrect timestamps at boot). The linuxptp
    upstream mailing list confirmed this responsibility belongs with the
    init system, not with ptp4l itself.

Changes vs the previous PR (#26600)

  • Fixed loglvl -> loglevel in both init scripts (config option name
    now matches the documented UCI option in linuxptp.config)
  • Removed redundant wanted_stats variable in phc2sys.init; the real
    gate is stats_interval >= 1, which is now checked directly
  • Quoted variable expansions in shell conditionals
  • Commit message body lines wrapped at 72 characters

Closes: #26546

This adds configuration and init scripts for ptp4l and phc2sys, the
daemons included with the linuxptp package.

A helper utility (sysclock-discipline-check) delays ptp4l startup when
in 'server' mode until the kernel confirms clock accuracy. This is
critical for systems without RTC that boot with incorrect timestamps.

Signed-off-by: Aleks Mariusz <a+git-commit@alek.cx>

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 new commit. The commit message matches the diff, PKG_RELEASE is correctly bumped to 2 for a no-version-change modification, conffiles is formatted correctly (absolute path, no trailing slash, no indentation), both init scripts use procd with the right shebang, and the Makefile indentation follows the recipe/metadata split. No issues there.

Three findings look like they would actually break the feature on a stock device and are worth resolving before merge:

  • ptp4l.init opens the procd instance before it validates anything, so neither the clock-sync wait nor the config-generation failure can stop ptp4l from starting — rc.common discards start_service's return value.
  • /var/etc is not created at boot, so the generated ptp4l.conf cannot be written at all.
  • phc2sys.init ignores make_cmdline's failure return and launches phc2sys with an incomplete command line.

The rest — quoting, the uninitialized struct timex, missing reload triggers, undocumented UCI options, typos — are inline and non-blocking. One open question on whether starting both daemons by default on upgrade is intended.

CI on 3817eeb is green (the single red arm_cortex-a15 job is from the earlier run of the same SHA, which was re-run successfully), so nothing to chase there.


Generated by Claude Code

Comment thread net/linuxptp/files/ptp4l.init Outdated
Comment on lines +131 to +136
procd_open_instance "ptp4l"
procd_set_param command $cmdline
procd_set_param file $CONFIGFILE
procd_set_param stdout 1
procd_set_param stderr 1
procd_close_instance

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The procd instance is registered before any of the validation below runs, so the two return 1 paths (wait_disciplined failure on line 143, config-generation failure on line 150) do not actually prevent ptp4l from starting. rc.common ignores start_service's exit status — rc_procd() at rc.common:129-135 calls procd_close_service unconditionally, which publishes every instance opened so far.

Net effect: in the "clock never disciplines" case ptp4l is started anyway (defeating the purpose of the helper this PR adds), and in the config-failure case it is started pointing at a $CONFIGFILE that line 149 has just deleted.

Please move the procd_open_instanceprocd_close_instance block to the end of start_service(), after wait_disciplined and make_configfile have both succeeded. Same shape as the make_cmdline failure in phc2sys.init (see separate comment).

While moving it: cmdline on line 129 is missing from the local declaration on line 125.


Generated by Claude Code

Comment on lines +99 to +104
. /lib/functions/network.sh

(
handle_global $cfg || exit 1
config_foreach handle_interface ptp_interface
) > "$CONFIGFILE"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/var/etc does not exist on a stock OpenWrt boot — base-files boot() at init.d/boot:26-33 creates only /var/lock, /var/log, /var/run, /var/state and /var/tmp. The redirection on line 104 therefore fails with "cannot create" and ptp4l never gets a config file. Every in-tree package that writes there creates the directory itself (net/samba4, net/ocserv, net/igmpproxy, net/tinyproxy, …).

Suggested change
. /lib/functions/network.sh
(
handle_global $cfg || exit 1
config_foreach handle_interface ptp_interface
) > "$CONFIGFILE"
. /lib/functions/network.sh
mkdir -p "${CONFIGFILE%/*}"
(
handle_global $cfg || exit 1
config_foreach handle_interface ptp_interface
) > "$CONFIGFILE"

Generated by Claude Code

Comment thread net/linuxptp/files/phc2sys.init Outdated

config_load linuxptp

cmdline=$(make_cmdline $ID)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make_cmdline returns 1 for a missing PHC device (line 23) and for an invalid sync_direction (line 35), but the return value is discarded here and the instance is opened unconditionally below. With an invalid sync_direction, $cmdline ends up as /usr/sbin/phc2sys -m -q -l 5 — no -s, so phc2sys is launched and immediately dies in a procd respawn loop, with only the "Invalid sync_direction" syslog line to explain it.

Bailing out here works because no instance has been opened yet, so procd_close_service publishes an empty service and nothing is started:

Suggested change
cmdline=$(make_cmdline $ID)
cmdline=$(make_cmdline $ID) || return 1

Generated by Claude Code

Comment on lines +9 to +16
#include <stdio.h>
#include <stdlib.h>
#include <sys/timex.h>

int main() {
struct timex txc;

txc.modes = 0;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

struct timex txc is left uninitialized apart from .modes, and the whole struct is copied into the kernel by adjtimex(). It happens to be harmless today because modes == 0 makes the call read-only, but it means the syscall is fed stack garbage in offset, freq, tick, constant, etc. — one added ADJ_* bit later and this becomes a clock-corrupting bug. Zero the struct instead.

Also: stdlib.h is not used, and int main() should be int main(void) so the compiler enforces the prototype.

Suggested change
#include <stdio.h>
#include <stdlib.h>
#include <sys/timex.h>
int main() {
struct timex txc;
txc.modes = 0;
#include <stdio.h>
#include <string.h>
#include <sys/timex.h>
int main(void) {
struct timex txc;
memset(&txc, 0, sizeof(txc));

Separately, txc.status is an int while %04X expects unsigned int on lines 24 and 27 — (unsigned int)txc.status silences that.


Generated by Claude Code

Comment thread net/linuxptp/files/ptp4l.init Outdated
Comment on lines +63 to +67
config_get interface $cfg "interface" ""
[ -z $interface ] && { log err "Required 'interface' option missing in 'ptp_interface' section"; exit 1; }

network_get_device ifname $interface
[ -z $ifname ] && { log err "Cannot resolve UCI interface '$interface' to kernel-compatible ifname."; exit 1; }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unquoted expansions in the [ tests. [ -z $interface ] with an empty $interface collapses to [ -z ], which is the one-argument form and tests whether the string -z is non-empty — i.e. it returns true by accident, not by design, and it would break as soon as the value contains whitespace. Same for [ -z $ifname ].

Suggested change
config_get interface $cfg "interface" ""
[ -z $interface ] && { log err "Required 'interface' option missing in 'ptp_interface' section"; exit 1; }
network_get_device ifname $interface
[ -z $ifname ] && { log err "Cannot resolve UCI interface '$interface' to kernel-compatible ifname."; exit 1; }
config_get interface $cfg "interface" ""
[ -z "$interface" ] && { log err "Required 'interface' option missing in 'ptp_interface' section"; exit 1; }
network_get_device ifname "$interface"
[ -z "$ifname" ] && { log err "Cannot resolve UCI interface '$interface' to kernel-compatible ifname."; exit 1; }

Same applies to [ $transport != "UDPv4" ] and [ $delay_mechanism != "E2E" ] on lines 74-75, and to [ $role = "server" ] / [ $clocksync_wanted = 1 ] on lines 139 and 142.


Generated by Claude Code

Comment thread net/linuxptp/files/ptp4l.init Outdated

procd_open_instance "ptp4l"
procd_set_param command $cmdline
procd_set_param file $CONFIGFILE

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither init script defines service_triggers(), so a uci commit linuxptp followed by reload_config will not pick up the change — procd_set_param file watches the generated /var/etc/ptp4l.conf, not /etc/config/linuxptp, and nothing regenerates it. A procd_add_reload_trigger "linuxptp" in both scripts is the usual convention here (111 of the init scripts under net/ define service_triggers).

Related and probably worth handling at the same time: handle_interface resolves the kernel device through network_get_device at START=45, which is not a guarantee that the UCI interface is already up. If it is not, config generation aborts and ptp4l never comes back on its own. An interface trigger (procd_add_interface_trigger "interface.*" "$interface" /etc/init.d/ptp4l restart) would make it self-healing.


Generated by Claude Code

Comment thread net/linuxptp/files/linuxptp.config Outdated
#list global_opts 'clockClass 6'

config ptp_interface # repeat multiple times for each interface ptp4l should work on
option interface 'lan' # required - UCI's interface name, init-script looks up kernel ifname as needed

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only active (non-commented) option in the shipped config, which means that after this PR an opkg upgrade linuxptp on an existing install starts both new services automatically: ptp4l joins the BMCA on lan, and phc2sys — defaulting to phc_to_sys — begins disciplining CLOCK_REALTIME from the NIC's PHC, competing with whatever sysntpd/ntpd the box already runs.

Today the package only ships binaries and no daemons, so this is a silent behaviour change for every current user. Was that intended, or should the ptp_interface section ship commented out (and/or the init scripts default to option disabled 1) so the admin opts in explicitly?


Generated by Claude Code

Comment thread net/linuxptp/files/linuxptp.config Outdated
#option stats_interval '0' # Log status updates every N seconds (N defaults to 1 when -R param (by-default) is 1Hz).
# By default/if unset, log-level (above) will be set to 5 and these messages will not be seen

list extra_args '' # For other phc2sys command line options.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is an active list with a single empty element, not a commented-out example like everything else in the file. config_list_foreach at phc2sys.init:48 will call echo "" for it, appending a blank entry to the command line. It is harmless only because procd_set_param command $cmdline is unquoted and word-splitting drops it. Commenting it out keeps the file consistent and removes the accident.

Suggested change
list extra_args '' # For other phc2sys command line options.
#list extra_args '' # For other phc2sys command line options.

Generated by Claude Code

local check_msg interval is_synced=0 rc role

config_get role $ID "role" ""
config_get interval $ID "clock_sync_check_interval" "15"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: clock_sync_check_interval is read here but is not documented anywhere in files/linuxptp.config, so there is no way for a user to discover it. Same for the ptp4l-section extra_args consumed at line 121 — extra_args is only documented under the phc2sys section (linuxptp.config:33). Worth adding commented-out entries for both in the config ptp4l block.


Generated by Claude Code

Comment thread net/linuxptp/files/ptp4l.init Outdated
}

stop_service() {
log info "ptp4l exitting (if running)"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "exitting" -> "exiting".

Suggested change
log info "ptp4l exitting (if running)"
log info "ptp4l exiting (if running)"

Same typo at phc2sys.init:67, and "syncronized" -> "synchronized" at line 41 above.

While here: stop_service leaves /var/etc/ptp4l.conf behind. Adding rm -f "$CONFIGFILE" would keep the generated state in step with the service.


Generated by Claude Code

- ptp4l.init: move procd_open_instance block to after validation so
  clock-sync wait and config-generation failures actually prevent startup
- ptp4l.init: add mkdir -p for /var/etc before writing ptp4l.conf
- phc2sys.init: propagate make_cmdline failure to prevent launch with
  an incomplete command line
- Both init scripts: add service_triggers() for reload_config support
- ptp4l.init: quote all variable expansions in [ ] tests
- sysclock-discipline-check.c: zero-initialise struct timex via
  memset, use int main(void), drop unused stdlib.h, cast txc.status
  to unsigned int for printf
- linuxptp.config: comment out the active ptp_interface/interface
  option so services do not auto-start on upgrade without explicit
  admin opt-in
- linuxptp.config: comment out active list extra_args and document
  clock_sync_check_interval and ptp4l extra_args options
- Fix typos: exitting->exiting, syncronized->synchronized
- ptp4l stop_service: remove generated config file on stop

Signed-off-by: Aleks Mariusz <a+git-commit@alek.cx>

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 new commit (6cb97c7). The three blocking items from the previous round are genuinely fixed: the procd instance is now opened only after wait_disciplined and make_configfile both succeed, mkdir -p "${CONFIGFILE%/*}" creates /var/etc, and make_cmdline's failure is propagated in phc2sys.init. service_triggers() is right too — with USE_PROCD and no reload_service, reload() at rc.common:165-171 falls through to start, which regenerates the config, so reload_config works. The C changes (memset, int main(void), dropped stdlib.h, (unsigned int) casts), the quoting, the new UCI documentation and the typo fixes all look correct, and PKG_RELEASE stays at 2, which is right for a second modification within the same unreleased bump.

Commit checks

  • 6cb97c7 "linuxptp: fix init script bugs and address review feedback" — the bullet "linuxptp.config: comment out the active ptp_interface/interface option so services do not auto-start on upgrade without explicit admin opt-in" holds for ptp4l only. The config phc2sys section is left active with working defaults for every option, so phc2sys is still started by default_postinst on any device that has a /dev/ptp0. Details inline.

Two other things surfaced by the fixes themselves, both inline and worth a look before merge:

  • Moving the wait below the instance registration means start_service now blocks on it, and wait_disciplined has no timeout — an RTC-less server box with no time source stalls the whole rcS sequence rather than just ptp4l.
  • With interface commented out, the empty config ptp_interface section is the only thing preventing ptp4l from being started with no port at all; deleting that section gets past the [ -s "$CONFIGFILE" ] check and into a procd respawn loop.

Neither is a regression in the strict sense — they are consequences of the shape the fixes take — and CI on 6cb97c7 is fully green, so nothing to chase there.


Generated by Claude Code

Comment thread net/linuxptp/files/linuxptp.config Outdated
#list extra_args ''

config ptp_interface # repeat multiple times for each interface ptp4l should work on
#option interface 'lan' # required - UCI's interface name, init-script looks up kernel ifname as needed

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commenting this out leaves config ptp_interface as an empty anonymous section, so the shipped default now makes ptp4l fail on every boot: handle_interface hits exit 1 at ptp4l.init:64 and start_service then logs Failed to generate /var/etc/ptp4l.conf. Aborting. — two err-level syslog lines per boot on a package the admin has not configured yet.

That is presumably the intended opt-in gate, and it does work, but it makes the empty section load-bearing in a non-obvious way. An admin who deletes it — the natural reaction to a section with nothing in it — gets the opposite failure: config_foreach handle_interface ptp_interface matches nothing, make_configfile returns 0 with a file containing only [global], the [ -s "$CONFIGFILE" ] check at ptp4l.init:144 passes, and ptp4l is started with no port at all. ptp4l exits immediately with no interface specified and procd respawn-loops it.

Worth making that validation require at least one generated port section rather than merely a non-empty file — e.g. have handle_interface record that it emitted something and test that, instead of -s. Then the empty section is no longer the only thing standing between the shipped config and a respawn loop.


Generated by Claude Code

# For interface-specific sections in ptp4l.conf
#list interface_opts 'logSyncInterval -3'

config phc2sys 'phc2sys' # configures the phc2sys daemon, which syncs either system clock from PHC in NIC (as a client, or vice versa as a server)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit message says the config change makes it so "services do not auto-start on upgrade without explicit admin opt-in", but that only gates ptp4l. This config phc2sys section is still active and every option in it has a working fallback: phc_device defaults to /dev/ptp0 at phc2sys.init:22-23 and sync_direction defaults to phc_to_sys at phc2sys.init:25-29. default_postinst() at functions.sh:401-410 runs "$i" start for every init script the package ships, and the PKG_UPGRADE guard there covers only enable, not start — so on any box that has a PHC, both installing and upgrading linuxptp starts phc2sys.

The combination is worse than before this commit rather than better: ptp4l is now the daemon that refuses to start (no interface configured), so phc2sys ends up slewing CLOCK_REALTIME from a free-running, undisciplined NIC PHC, competing with whatever sysntpd/ntpd the box already runs. Previously at least ptp4l was disciplining that PHC.

Gating phc2sys the same way would make the commit message's claim true for both daemons — e.g. drop the /dev/ptp0 default so an unset phc_device is a hard "not configured", or add an explicit option enabled that both init scripts check.


Generated by Claude Code

Comment on lines +42 to +57
while [ $is_synced -eq 0 ]; do
check_msg=$(is_clock_disciplined); rc=$?

if [ $rc -eq 0 ]; then
is_synced=1
log info "sysclock-discipline-check: $check_msg. Continuing."
return 0
elif [ $rc -eq 1 ]; then
log info "sysclock-discipline-check: $check_msg. Waiting..."
else
log crit "Clock sync check FAILED: $check_msg."
log err "ptp4l will NOT start. Resolve issue, or set 'require_clock_sync' to 0"
return 1
fi
sleep $interval
done

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop has no upper bound: as long as the helper keeps returning 1 (STA_UNSYNC still set) it sleeps and retries forever, and is_synced is only ever set on the path that already return 0s.

That was survivable while the procd instance was registered before the wait. Now that the procd_open_instance block has correctly moved below it, start_service does not return until the loop ends — and start_service runs synchronously from /etc/rc.d/S45ptp4l during rcS. So a box configured with role 'server' (where require_clock_sync defaults to 1 via line 136) that never gets a time source hangs the rest of the boot sequence: phc2sys at START=55 and every other init script ordered after 45 never runs. On a system with no RTC and no upstream NTP — precisely the scenario the helper was added for — that is an unbootable device rather than a delayed daemon.

Bounding the wait would keep the guard: a max attempt count or total deadline, after which it logs and return 1s so ptp4l simply does not start. Alternatively, keep the wait off the boot path entirely — background it and let a trigger register the instance once the clock disciplines.


Generated by Claude Code

- ptp4l.init: bound wait_disciplined with clock_sync_max_tries (default
  8 attempts, ~2 min) so a server-role device with no time source does
  not stall rcS indefinitely; set to 0 to wait without limit
- ptp4l.init: validate that at least one [interface] section was written
  to ptp4l.conf (by counting section headers); a config with only
  [global] is now rejected with a clear error rather than launching
  ptp4l into a respawn loop
- phc2sys.init: require phc_device to be explicitly configured; removes
  the /dev/ptp0 default so phc2sys does not auto-start on upgrade and
  slew CLOCK_REALTIME from an undisciplined PHC
- linuxptp.config: comment out the config ptp_interface section header
  so the shipped default contains no load-bearing anonymous section;
  validation is now in the init script rather than the config structure
- linuxptp.config: comment out phc_device so both daemons require
  explicit admin opt-in before starting
- linuxptp.config: document clock_sync_max_tries option

Signed-off-by: Aleks Mariusz <a+git-commit@alek.cx>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

linuxptp: no init script or uci-compatible configs

2 participants