From d3d4166d17bfd85d1ef4a84f370b8b4a3cfba5af Mon Sep 17 00:00:00 2001 From: Vitaly Grinberg Date: Sun, 28 Jun 2026 19:05:13 +0300 Subject: [PATCH] force freerun when non-leading DPLL loses lock In chained-NIC T-GM/T-BC topologies, a non-leading DPLL losing lock must degrade the composite clock to FREERUN (S0). Previously the clock incorrectly reported HOLDOVER (S1) because: 1. UpdateState() gave HOLDOVER priority over FREERUN 2. The event package had no leading vs non-leading DPLL awareness. 3. stateDecision() used source-specific branches (GNSS/PPS/PTP4l) that don't reflect actual topology in multi-NIC configs. Changes: - Fix UpdateState() priority (stats.go) - Add hasNonLeadingDPLLFault() to detect follower faults when the leading DPLL is still locked (event.go) - Apply to both T-GM (updateGMState) and T-BC (updateBCState) paths - Refactor stateDecision() HOLDOVER branch: unify GNSS/PTP4l under hasLeadingSource(), non-leading DPLLs go directly to FREERUN - Add leader-follower matrix state tests (stats_test.go) Co-authored-by: Cursor --- pkg/dpll/dpll.go | 56 +++++++++--------------- pkg/event/event.go | 31 ++++++++++++++ pkg/event/event_tbc.go | 16 ++++--- pkg/event/stats.go | 12 +++--- pkg/event/stats_test.go | 94 ++++++++++++++++++++++++++++++++++++++++- 5 files changed, 158 insertions(+), 51 deletions(-) diff --git a/pkg/dpll/dpll.go b/pkg/dpll/dpll.go index 9bb07b629..660db99f7 100644 --- a/pkg/dpll/dpll.go +++ b/pkg/dpll/dpll.go @@ -763,45 +763,29 @@ func (d *DpllConfig) stateDecision() { case DPLL_HOLDOVER: switch { - case d.hasPPSAsSource(): + case !d.hasLeadingSource(): d.state = event.PTP_FREERUN d.phaseOffset = FaultyPhaseOffset - glog.Infof("Follower card DPLL is on holdover - hardware failure or pins misconfigured") - case d.hasGNSSAsSource(): - if !d.inSpec { // d.inSpec is updated by the holdover routine - glog.Infof("dpll is not in spec, state is DPLL_HOLDOVER, offset is out of range, state is FREERUN(%s)", d.iface) - d.state = event.PTP_FREERUN - d.phaseOffset = FaultyPhaseOffset - select { - case d.holdoverCloseCh <- true: - glog.Infof("closing holdover for %s since offset if out of spec", d.iface) - default: - } - } else if !d.onHoldover { - d.holdoverCloseCh = make(chan bool) - d.onHoldover = true - d.state = event.PTP_HOLDOVER - glog.Infof("starting holdover (%s)", d.iface) - go d.holdover() - } - case d.hasPTPAsSource(): - if d.PhaseOffset() > LocalMaxHoldoverOffSet { - glog.Infof("dpll offset is above MaxHoldoverOffSet, state is FREERUN(%s)", d.iface) - d.state = event.PTP_FREERUN - d.phaseOffset = FaultyPhaseOffset - d.sourceLost = true - select { - case d.holdoverCloseCh <- true: - glog.Infof("closing holdover for %s since offset if above MaxHoldoverOffSet", d.iface) - default: - } - } else if !d.onHoldover && !d.closing { - d.holdoverCloseCh = make(chan bool) - d.onHoldover = true - d.state = event.PTP_HOLDOVER - glog.Infof("starting holdover (%s)", d.iface) - go d.holdover() + glog.Infof("non-leading DPLL %s is in holdover, reporting FREERUN", d.iface) + // TODO: GNSS holdover currently doesn't offer holdover out of spec. Tech Debt: should work the same as T-BC + // making transitions programmable by users + case !d.inSpec || (d.hasPTPAsSource() && math.Abs(float64(d.PhaseOffset())) > float64(LocalMaxHoldoverOffSet)): + glog.Infof("leading DPLL %s holdover out of spec (inSpec=%v, offset=%d, max=%d), state is FREERUN", + d.iface, d.inSpec, d.PhaseOffset(), LocalMaxHoldoverOffSet) + d.state = event.PTP_FREERUN + d.phaseOffset = FaultyPhaseOffset + d.sourceLost = true + select { + case d.holdoverCloseCh <- true: + glog.Infof("closing holdover for %s since holdover is out of spec", d.iface) + default: } + case !d.onHoldover && !d.closing: + d.holdoverCloseCh = make(chan bool) + d.onHoldover = true + d.state = event.PTP_HOLDOVER + glog.Infof("starting holdover (%s)", d.iface) + go d.holdover() } case DPLL_LOCKED_HO_ACQ, DPLL_LOCKED: diff --git a/pkg/event/event.go b/pkg/event/event.go index 6ab7c475a..70b41e5b3 100644 --- a/pkg/event/event.go +++ b/pkg/event/event.go @@ -382,6 +382,9 @@ func (e *EventHandler) updateGMState(cfgName string) clockSyncState { switch d.ProcessName { case DPLL: dpllState = d.State + if e.hasNonLeadingDPLLFault(cfgName, leadingInterface) { + dpllState = PTP_FREERUN + } case GNSS: gnssState = d.State // expecting to have at least one interface @@ -536,6 +539,34 @@ func (e *EventHandler) isSourceLost(cfgName string) bool { return false } +// hasNonLeadingDPLLFault returns true when the leading DPLL is locked but at +// least one non-leading DPLL is not locked, indicating a follower fault that +// should force the composite clock to FREERUN. +func (e *EventHandler) hasNonLeadingDPLLFault(cfgName, leadingInterface string) bool { + if leadingInterface == LEADING_INTERFACE_UNKNOWN { + return false + } + if data, ok := e.data[cfgName]; ok { + for _, d := range data { + if d.ProcessName != DPLL { + continue + } + leadingDetail := d.GetDataDetails(leadingInterface) + if leadingDetail == nil || leadingDetail.State != PTP_LOCKED { + return false + } + for _, dd := range d.Details { + if dd.IFace != leadingInterface && dd.State != PTP_LOCKED { + glog.Infof("non-leading DPLL %s is %s while leading %s is locked, composite DPLL forced to FREERUN", + dd.IFace, dd.State, leadingInterface) + return true + } + } + } + } + return false +} + func (e *EventHandler) getLeadingInterface(cfgName string) string { if data, ok := e.data[cfgName]; ok { for _, d := range data { diff --git a/pkg/event/event_tbc.go b/pkg/event/event_tbc.go index a27606509..aa6e6c220 100644 --- a/pkg/event/event_tbc.go +++ b/pkg/event/event_tbc.go @@ -144,7 +144,7 @@ func (e *EventHandler) updateBCState(event EventChannel) (clockSyncState, bool) updateDownstreamData = true } case PTP_LOCKED: - if e.freeRunCondition(cfgName) { + if e.freeRunCondition(cfgName) || e.hasNonLeadingDPLLFault(cfgName, leadingInterface) { e.clkSyncState[cfgName].state = PTP_FREERUN e.clkSyncState[cfgName].clockClass = protocol.ClockClassFreerun glog.Info("BC FSM: LOCKED to FREERUN") @@ -172,16 +172,18 @@ func (e *EventHandler) updateBCState(event EventChannel) (clockSyncState, bool) } } case PTP_HOLDOVER: - if e.inSyncCondition(cfgName) && !e.isSourceLostBC(cfgName) { - e.clkSyncState[cfgName].state = PTP_LOCKED - glog.Info("BC FSM: HOLDOVER to LOCKED") - updateDownstreamData = true - } else if e.freeRunCondition(cfgName) { + nonLeadingFault := e.hasNonLeadingDPLLFault(cfgName, leadingInterface) + switch { + case nonLeadingFault || e.freeRunCondition(cfgName): e.clkSyncState[cfgName].state = PTP_FREERUN e.clkSyncState[cfgName].clockClass = protocol.ClockClassFreerun glog.Info("BC FSM: HOLDOVER to FREERUN") updateDownstreamData = true - } else { + case e.inSyncCondition(cfgName) && !e.isSourceLostBC(cfgName): + e.clkSyncState[cfgName].state = PTP_LOCKED + glog.Info("BC FSM: HOLDOVER to LOCKED") + updateDownstreamData = true + default: if event.IFace == leadingInterface { inSpec := false if e.LeadingClockData.lastInSpec { diff --git a/pkg/event/stats.go b/pkg/event/stats.go index 6435af7e8..a0489950e 100644 --- a/pkg/event/stats.go +++ b/pkg/event/stats.go @@ -49,14 +49,14 @@ func (d *Data) UpdateState() { state := PTP_UNKNOWN for _, detail := range d.Details { // 2 ts2phc or 2 dpll etc switch detail.State { - case PTP_FREERUN: // if its free run and main state is not holdover then this is the state - if state != PTP_HOLDOVER { + case PTP_FREERUN: // FREERUN is the worst state (S0) and always takes priority + state = detail.State + case PTP_HOLDOVER: // HOLDOVER (S1) takes priority over LOCKED but not FREERUN + if state != PTP_FREERUN { state = detail.State } - case PTP_HOLDOVER: // if one of them is in holdover then this is the state - state = detail.State - case PTP_LOCKED: // if this is locked and none of them are in UNKNOWN or FREE run then this is the state - if state != PTP_FREERUN && state != PTP_HOLDOVER { // previous state + case PTP_LOCKED: // LOCKED (S2) is best; only sets if nothing worse exists + if state != PTP_FREERUN && state != PTP_HOLDOVER { state = detail.State } } diff --git a/pkg/event/stats_test.go b/pkg/event/stats_test.go index e81ee7282..c3568fe9e 100644 --- a/pkg/event/stats_test.go +++ b/pkg/event/stats_test.go @@ -101,8 +101,8 @@ func Test_updateStats(t *testing.T) { }, State: event.PTP_UNKNOWN, }}}, - wantedState: event.PTP_HOLDOVER, - desc: "3. GNSS is in HOLDOVER, PPS is in FREERUN", + wantedState: event.PTP_FREERUN, + desc: "3. GNSS is in HOLDOVER, PPS is in FREERUN - FREERUN takes priority (worst state)", }, { data: map[string][]*event.Data{ "a.0.config": { @@ -139,3 +139,93 @@ func Test_updateStats(t *testing.T) { } } + +func Test_updateState_LeadingFollowerMatrix(t *testing.T) { + t.Parallel() + + tests := []struct { + desc string + leadingState event.PTPState + followerState event.PTPState + wantedState event.PTPState + }{ + { + desc: "both locked", + leadingState: event.PTP_LOCKED, + followerState: event.PTP_LOCKED, + wantedState: event.PTP_LOCKED, + }, + { + desc: "follower freerun, leader locked - follower degrades to S0", + leadingState: event.PTP_LOCKED, + followerState: event.PTP_FREERUN, + wantedState: event.PTP_FREERUN, + }, + { + desc: "follower freerun, leader holdover - FREERUN wins over HOLDOVER", + leadingState: event.PTP_HOLDOVER, + followerState: event.PTP_FREERUN, + wantedState: event.PTP_FREERUN, + }, + { + desc: "leader holdover, follower locked - HOLDOVER propagates", + leadingState: event.PTP_HOLDOVER, + followerState: event.PTP_LOCKED, + wantedState: event.PTP_HOLDOVER, + }, + { + desc: "both freerun", + leadingState: event.PTP_FREERUN, + followerState: event.PTP_FREERUN, + wantedState: event.PTP_FREERUN, + }, + { + desc: "leader freerun, follower locked - FREERUN wins", + leadingState: event.PTP_FREERUN, + followerState: event.PTP_LOCKED, + wantedState: event.PTP_FREERUN, + }, + { + desc: "both holdover", + leadingState: event.PTP_HOLDOVER, + followerState: event.PTP_HOLDOVER, + wantedState: event.PTP_HOLDOVER, + }, + { + desc: "leader locked, follower holdover", + leadingState: event.PTP_LOCKED, + followerState: event.PTP_HOLDOVER, + wantedState: event.PTP_HOLDOVER, + }, + { + desc: "leader freerun, follower holdover - FREERUN wins", + leadingState: event.PTP_FREERUN, + followerState: event.PTP_HOLDOVER, + wantedState: event.PTP_FREERUN, + }, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + t.Parallel() + d := &event.Data{ + ProcessName: "dpll", + Details: []*event.DataDetails{ + { + IFace: "leading-nic", + State: tt.leadingState, + Metrics: map[event.ValueType]event.DataMetric{}, + }, + { + IFace: "follower-nic", + State: tt.followerState, + Metrics: map[event.ValueType]event.DataMetric{}, + }, + }, + State: event.PTP_UNKNOWN, + } + d.UpdateState() + assert.Equal(t, tt.wantedState, d.State, tt.desc) + }) + } +}