[Linuxptp-devel] [PATCH RFC V1 1/5] port: Make the finite state machine into a function variable.

[Richard Cochran]

This allows adding new FSM flavors in the future.

Signed-off-by: Richard Cochran <***@gmail.com>
---
port.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/port.c b/port.c
index a1ad6f6..afe0057 100644
--- a/port.c
+++ b/port.c
@@ -94,6 +94,8 @@ struct port {
unsigned int pdr_missing;
unsigned int multiple_seq_pdr_count;
unsigned int multiple_pdr_detected;
+ enum port_state (*state_machine)(enum port_state state,
+ enum fsm_event event, int mdiff);
/* portDS */
struct PortIdentity portIdentity;
enum port_state state; /*portState*/
@@ -2142,10 +2144,8 @@ int port_dispatch(struct port *p, enum fsm_event event, int mdiff)
if (event == EV_RS_MASTER || event == EV_RS_GRAND_MASTER) {
port_slave_priority_warning(p);
}
- next = ptp_slave_fsm(p->state, event, mdiff);
- } else {
- next = ptp_fsm(p->state, event, mdiff);
}
+ next = p->state_machine(p->state, event, mdiff);

if (!fault_interval(p, last_fault_type(p), &i) &&
((i.val == FRI_ASAP && i.type == FTMO_LOG2_SECONDS) ||
@@ -2555,6 +2555,7 @@ struct port *port_open(int phc_index,

memset(p, 0, sizeof(*p));

+ p->state_machine = clock_slave_only(clock) ? ptp_slave_fsm : ptp_fsm;
p->phc_index = phc_index;
p->jbod = config_get_int(cfg, interface->name, "boundary_clock_jbod");
transport = config_get_int(cfg, interface->name, "network_transport");

[Richard Cochran]

Looking at the fault logic in port_dispatch(), you might think that
the function, fault_interval(), checks whether a fault is active, but
you would be wrong, since that function always returns zero.

This patch removes the superfluous input error checking inside of
fault_interval() and changes the return type to void, making the
actual behavior explicit. Dropping the input check is safe because
that function has exactly two callers, both of whom always provide
valid inputs.

Signed-off-by: Richard Cochran <***@linutronix.de>
---
port.c | 15 +++++----------
port.h | 5 ++---
2 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/port.c b/port.c
index afe0057..6c9aa72 100644
--- a/port.c
+++ b/port.c
@@ -205,16 +205,11 @@ enum fault_type last_fault_type(struct port *port)
return port->last_fault_type;
}

-int fault_interval(struct port *port, enum fault_type ft,
- struct fault_interval *i)
+void fault_interval(struct port *port, enum fault_type ft,
+ struct fault_interval *i)
{
- if (!port || !i)
- return -EINVAL;
- if (ft < 0 || ft >= FT_CNT)
- return -EINVAL;
i->type = port->flt_interval_pertype[ft].type;
i->val = port->flt_interval_pertype[ft].val;
- return 0;
}

int port_fault_fd(struct port *port)
@@ -2147,9 +2142,9 @@ int port_dispatch(struct port *p, enum fsm_event event, int mdiff)
}
next = p->state_machine(p->state, event, mdiff);

- if (!fault_interval(p, last_fault_type(p), &i) &&
- ((i.val == FRI_ASAP && i.type == FTMO_LOG2_SECONDS) ||
- (i.val == 0 && i.type == FTMO_LINEAR_SECONDS)))
+ fault_interval(p, last_fault_type(p), &i);
+ if ((i.val == FRI_ASAP && i.type == FTMO_LOG2_SECONDS) ||
+ (i.val == 0 && i.type == FTMO_LINEAR_SECONDS))
fri_asap = 1;
if (PS_INITIALIZING == next || (PS_FAULTY == next && fri_asap)) {
/*
diff --git a/port.h b/port.h
index 19dec4a..d2e0865 100644
--- a/port.h
+++ b/port.h
@@ -318,9 +318,8 @@ enum fault_type last_fault_type(struct port *port);
* @param port A port instance.
* @param ft Fault type.
* @param i Pointer to the struct which will be filled in.
- * @return Zero on success, non-zero otherwise.
*/
-int fault_interval(struct port *port, enum fault_type ft,
- struct fault_interval *i);
+void fault_interval(struct port *port, enum fault_type ft,
+ struct fault_interval *i);

#endif

[Richard Cochran]

The code that decides whether a fault qualifies for ASAP treatment is
a tangle of logical operators. This patch replaces the open coded
logic with a helper function whose name makes the intent clear. This
is a cosmetic change only.

Signed-off-by: Richard Cochran <***@linutronix.de>
---
port.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/port.c b/port.c
index 6c9aa72..02dbabb 100644
--- a/port.c
+++ b/port.c
@@ -161,6 +161,19 @@ static void announce_to_dataset(struct ptp_message *m, struct port *p,
out->receiver = p->portIdentity;
}

+static int clear_fault_asap(struct fault_interval *faint)
+{
+ switch (faint->type) {
+ case FTMO_LINEAR_SECONDS:
+ return faint->val == 0 ? 1 : 0;
+ case FTMO_LOG2_SECONDS:
+ return faint->val == FRI_ASAP ? 1 : 0;
+ case FTMO_CNT:
+ return 0;
+ }
+ return 0;
+}
+
static int msg_current(struct ptp_message *m, struct timespec now)
{
int64_t t1, t2, tmo;
@@ -2143,9 +2156,9 @@ int port_dispatch(struct port *p, enum fsm_event event, int mdiff)
next = p->state_machine(p->state, event, mdiff);

fault_interval(p, last_fault_type(p), &i);
- if ((i.val == FRI_ASAP && i.type == FTMO_LOG2_SECONDS) ||
- (i.val == 0 && i.type == FTMO_LINEAR_SECONDS))
+ if (clear_fault_asap(&i)) {
fri_asap = 1;
+ }
if (PS_INITIALIZING == next || (PS_FAULTY == next && fri_asap)) {
/*
* This is a special case. Since we initialize the

[Richard Cochran]

Although leaving the INITIALIZING state and clearing the FAULTY state
ASAP both result in a port entering the LISTENING state, still there
is no benefit from conflating the two. In the FAULTY case, the
current code actually skips the INITIALIZING state altogether.

This patch separates the two cases resulting in two benefits. First,
the check for ASAP fault status is only made when a fault is actually
present, unlike the present unconditional check. Second, this change
will allow us to cleanly support alternative state machines later on.

Signed-off-by: Richard Cochran <***@linutronix.de>
---
port.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/port.c b/port.c
index 02dbabb..ebe7342 100644
--- a/port.c
+++ b/port.c
@@ -2145,8 +2145,6 @@ static void port_p2p_transition(struct port *p, enum port_state next)
int port_dispatch(struct port *p, enum fsm_event event, int mdiff)
{
enum port_state next;
- struct fault_interval i;
- int fri_asap = 0;

if (clock_slave_only(p->clock)) {
if (event == EV_RS_MASTER || event == EV_RS_GRAND_MASTER) {
@@ -2155,11 +2153,15 @@ int port_dispatch(struct port *p, enum fsm_event event, int mdiff)
}
next = p->state_machine(p->state, event, mdiff);

- fault_interval(p, last_fault_type(p), &i);
- if (clear_fault_asap(&i)) {
- fri_asap = 1;
+ if (PS_FAULTY == next) {
+ struct fault_interval i;
+ fault_interval(p, last_fault_type(p), &i);
+ if (clear_fault_asap(&i)) {
+ pr_notice("port %hu: clearing fault immediately", portnum(p));
+ next = PS_INITIALIZING;
+ }
}
- if (PS_INITIALIZING == next || (PS_FAULTY == next && fri_asap)) {
+ if (PS_INITIALIZING == next) {
/*
* This is a special case. Since we initialize the
* port immediately, we can skip right to listening

[Richard Cochran]

From: Richard Cochran <***@gmail.com>

The state machines in 1588 do not specify an event that causes a transition
out of the initializing state. This was left as a local issue. For this
transition, the current code assigns the next state outside of the FSM. But
doing so prevents an alternative FSM to handle this transition differently.

By introducing a new event, this patch places this transition where it
belongs, namely under the control of the FSM code,

Signed-off-by: Richard Cochran <***@gmail.com>
---
fsm.c | 22 ++++++++++++++++++++--
fsm.h | 1 +
port.c | 15 +++++++--------
util.c | 1 +
4 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/fsm.c b/fsm.c
index d1423e7..ce6efad 100644
--- a/fsm.c
+++ b/fsm.c
@@ -27,7 +27,16 @@ enum port_state ptp_fsm(enum port_state state, enum fsm_event event, int mdiff)

switch (state) {
case PS_INITIALIZING:
- next = PS_LISTENING;
+ switch (event) {
+ case EV_FAULT_DETECTED:
+ next = PS_FAULTY;
+ break;
+ case EV_INIT_COMPLETE:
+ next = PS_LISTENING;
+ break;
+ default:
+ break;
+ }
break;

case PS_FAULTY:
@@ -220,7 +229,16 @@ enum port_state ptp_slave_fsm(enum port_state state, enum fsm_event event,

switch (state) {
case PS_INITIALIZING:
- next = PS_LISTENING;
+ switch (event) {
+ case EV_FAULT_DETECTED:
+ next = PS_FAULTY;
+ break;
+ case EV_INIT_COMPLETE:
+ next = PS_LISTENING;
+ break;
+ default:
+ break;
+ }
break;

case PS_FAULTY:
diff --git a/fsm.h b/fsm.h
index 5d4ae91..0616daa 100644
--- a/fsm.h
+++ b/fsm.h
@@ -48,6 +48,7 @@ enum fsm_event {
EV_ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES,
EV_SYNCHRONIZATION_FAULT,
EV_MASTER_CLOCK_SELECTED,
+ EV_INIT_COMPLETE,
EV_RS_MASTER,
EV_RS_GRAND_MASTER,
EV_RS_SLAVE,
diff --git a/port.c b/port.c
index ebe7342..5f1646b 100644
--- a/port.c
+++ b/port.c
@@ -2120,6 +2120,7 @@ static void port_p2p_transition(struct port *p, enum port_state next)
break;
case PS_LISTENING:
port_set_announce_tmo(p);
+ port_set_delay_tmo(p);
break;
case PS_PRE_MASTER:
port_set_qualification_tmo(p);
@@ -2158,7 +2159,7 @@ int port_dispatch(struct port *p, enum fsm_event event, int mdiff)
fault_interval(p, last_fault_type(p), &i);
if (clear_fault_asap(&i)) {
pr_notice("port %hu: clearing fault immediately", portnum(p));
- next = PS_INITIALIZING;
+ next = p->state_machine(next, EV_FAULT_CLEARED, 0);
}
}
if (PS_INITIALIZING == next) {
@@ -2170,14 +2171,12 @@ int port_dispatch(struct port *p, enum fsm_event event, int mdiff)
if (port_is_enabled(p)) {
port_disable(p);
}
- next = port_initialize(p) ? PS_FAULTY : PS_LISTENING;
- port_show_transition(p, next, event);
- p->state = next;
- if (next == PS_LISTENING && p->delayMechanism == DM_P2P) {
- port_set_delay_tmo(p);
+ if (port_initialize(p)) {
+ event = EV_FAULT_DETECTED;
+ } else {
+ event = EV_INIT_COMPLETE;
}
- port_notify_event(p, NOTIFY_PORT_STATE);
- return 1;
+ next = p->state_machine(next, event, 0);
}

if (next == p->state)
diff --git a/util.c b/util.c
index 594b49f..2b880ff 100644
--- a/util.c
+++ b/util.c
@@ -61,6 +61,7 @@ const char *ev_str[] = {
"ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES",
"SYNCHRONIZATION_FAULT",
"MASTER_CLOCK_SELECTED",
+ "INIT_COMPLETE",
"RS_MASTER",
"RS_GRAND_MASTER",
"RS_SLAVE",

[Keller, Jacob E]

-----Original Message-----
Sent: Saturday, December 17, 2016 10:17 AM
Subject: [Linuxptp-devel] [PATCH RFC V1 0/5] Prepare the FSM for alternate BMC
algorithms
Recently we have had some discussion about supporting alternate BMC
algorithms. This series takes the first step in that direction.
Right now, there is a short passage outside of the FSM functions that
directly fiddles with the port state for the transition out of the
INITIALIZING state. In order to cleanly support other algorithms, we
have to clean this spot up, placing all state transitions under
control of the FSM code proper.
Thanks in advance for your comments and reviews,
Richard

Hi Richard,

At initial glance, this code looks good. It's good cleanup even if we didn't continue and add support for additional algorithms. I will try to review this in more depth tomorrow.

Thanks,
Jake