[Linuxptp-devel] [PATCH] phc2sys: don't synchronize clock to itself in automatic mode.

[Miroslav Lichvar]

When no source is found in the automatic mode and a clock is selected as
the default source, set its state temporarily to slave to prevent the
clock from being synchronized to itself and drifting quickly away.

Also, don't use this mode with only one PTP clock and don't include the
system clock.

This fixes phc2sys with SHM servo (e.g. used by timemaster).
---
phc2sys.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/phc2sys.c b/phc2sys.c
index 3ec1907..9ff5bf9 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -332,9 +332,14 @@ static void reconfigure(struct node *node)
}
last = c;
}
- if (dst_cnt && !src) {
+ if (dst_cnt > 1 && !src) {
if (!rt || rt->dest_only) {
node->master = last;
+ /* Reset to original state in next reconfiguration. */
+ node->master->new_state = node->master->state;
+ node->master->state = PS_SLAVE;
+ if (rt)
+ rt->state = PS_SLAVE;
pr_info("no source, selecting %s as the default clock",
last->device);
return;

[Richard Cochran]

Post by Miroslav Lichvar
When no source is found in the automatic mode and a clock is selected as
the default source, set its state temporarily to slave to prevent the
clock from being synchronized to itself and drifting quickly away.
Also, don't use this mode with only one PTP clock and don't include the
system clock.
This fixes phc2sys with SHM servo (e.g. used by timemaster).
---
phc2sys.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/phc2sys.c b/phc2sys.c
index 3ec1907..9ff5bf9 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -332,9 +332,14 @@ static void reconfigure(struct node *node)
}
last = c;
}
- if (dst_cnt && !src) {
+ if (dst_cnt > 1 && !src) {
if (!rt || rt->dest_only) {
node->master = last;
+ /* Reset to original state in next reconfiguration. */
+ node->master->new_state = node->master->state;
+ node->master->state = PS_SLAVE;
+ if (rt)
+ rt->state = PS_SLAVE;

Wait, if rt->dest_only==1, then shouldn't rt->state = PS_MASTER?

BTW, the code following the LIST_FOREACH is confusing as hell. (That
is why I placed this block of mine before everything else!) I wish
this logic could be expressed in a way that is simple to understand.

Post by Miroslav Lichvar
pr_info("no source, selecting %s as the default clock",
last->device);
return;

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran

Post by Miroslav Lichvar
@@ -332,9 +332,14 @@ static void reconfigure(struct node *node)
}
last = c;
}
- if (dst_cnt && !src) {
+ if (dst_cnt > 1 && !src) {
if (!rt || rt->dest_only) {
node->master = last;
+ /* Reset to original state in next reconfiguration. */
+ node->master->new_state = node->master->state;
+ node->master->state = PS_SLAVE;
+ if (rt)
+ rt->state = PS_SLAVE;

Wait, if rt->dest_only==1, then shouldn't rt->state = PS_MASTER?

No, I think we don't want to synchronize the system clock unless there
is a synchronized PTP clock.

Post by Richard Cochran
BTW, the code following the LIST_FOREACH is confusing as hell. (That
is why I placed this block of mine before everything else!) I wish
this logic could be expressed in a way that is simple to understand.

Yeah, the crossed mapping of port states (master/slave) to the phc2sys
clock synchronization state (slave/master) is confusing to me.

[Richard Cochran]

Post by Miroslav Lichvar
When no source is found in the automatic mode and a clock is selected as
the default source, set its state temporarily to slave to prevent the
clock from being synchronized to itself and drifting quickly away.
Also, don't use this mode with only one PTP clock and don't include the
system clock.
This fixes phc2sys with SHM servo (e.g. used by timemaster).

Applied.

Thanks,
Richard