[Linuxptp-devel] [PATCH RFC 1/2] port: Fix input to the BMC for forming the spanning tree.

[Richard Cochran]

If a non-slave port on a boundary clock see an announce message, then it
must decide whether it should take on the MASTER or the PASSIVE role. When
the GM fields from the local clock are identical to those in the announce,
then the sender/receiver ports are used as a tie breaker.

Following a typographical error in 1588, the code wrongly uses the port
identity of the upstream parent as the "receiver" id. As a result, a port
that should be PASSIVE may choose MASTER instead. This patch fixes the
code to use local port id.

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

diff --git a/port.c b/port.c
index 9f07cea..5868983 100644
--- a/port.c
+++ b/port.c
@@ -145,7 +145,7 @@ static int announce_compare(struct ptp_message *m1, struct ptp_message *m2)
return memcmp(&a->grandmasterPriority1, &b->grandmasterPriority1, len);
}

-static void announce_to_dataset(struct ptp_message *m, struct clock *c,
+static void announce_to_dataset(struct ptp_message *m, struct port *p,
struct dataset *out)
{
struct announce_msg *a = &m->announce;
@@ -155,7 +155,7 @@ static void announce_to_dataset(struct ptp_message *m, struct clock *c,
out->priority2 = a->grandmasterPriority2;
out->stepsRemoved = a->stepsRemoved;
out->sender = m->header.sourcePortIdentity;
- out->receiver = clock_parent_identity(c);
+ out->receiver = p->portIdentity;
}

static int msg_current(struct ptp_message *m, struct timespec now)
@@ -2032,7 +2032,7 @@ struct foreign_clock *port_compute_best(struct port *p)
if (!tmp)
continue;

- announce_to_dataset(tmp, p->clock, &fc->dataset);
+ announce_to_dataset(tmp, p, &fc->dataset);

fc_prune(fc);

[Richard Cochran]

Contrary to the letter of the standard, we do not throw a state decision
event every announce interval, but only when the inputs to the algorithm
have changed. When a port on a BC forms a spanning tree by deciding
between the MASTER and PASSIVE states, the identity of the port which sent
the announce message acts as a tie breaker.

However, the current code ignores that field when deciding whether the
inputs have changed or not. This patch fixes the comparison to include
the source port id.

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

diff --git a/port.c b/port.c
index 5868983..b9f6497 100644
--- a/port.c
+++ b/port.c
@@ -135,14 +135,20 @@ static void port_nrate_initialize(struct port *p);
static int announce_compare(struct ptp_message *m1, struct ptp_message *m2)
{
struct announce_msg *a = &m1->announce, *b = &m2->announce;
- int len =
+ int diff, len =
sizeof(a->grandmasterPriority1) +
sizeof(a->grandmasterClockQuality) +
sizeof(a->grandmasterPriority2) +
sizeof(a->grandmasterIdentity) +
sizeof(a->stepsRemoved);

- return memcmp(&a->grandmasterPriority1, &b->grandmasterPriority1, len);
+ diff = memcmp(&a->grandmasterPriority1, &b->grandmasterPriority1, len);
+ if (diff) {
+ return diff;
+ }
+ return memcmp(&m1->header.sourcePortIdentity,
+ &m2->header.sourcePortIdentity,
+ sizeof(m1->header.sourcePortIdentity));
}

static void announce_to_dataset(struct ptp_message *m, struct port *p,

[Keller, Jacob E]

Post by Richard Cochran
Contrary to the letter of the standard, we do not throw a state decision
event every announce interval, but only when the inputs to the
algorithm
have changed.  When a port on a BC forms a spanning tree by deciding
between the MASTER and PASSIVE states, the identity of the port which sent
the announce message acts as a tie breaker.
However, the current code ignores that field when deciding whether the
inputs have changed or not.  This patch fixes the comparison to
include
the source port id.

Makes sense to me, and code does what it says.

Post by Richard Cochran
---
 port.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/port.c b/port.c
index 5868983..b9f6497 100644
--- a/port.c
+++ b/port.c
@@ -135,14 +135,20 @@ static void port_nrate_initialize(struct port *p);
 static int announce_compare(struct ptp_message *m1, struct
ptp_message *m2)
 {
  struct announce_msg *a = &m1->announce, *b = &m2->announce;
- int len =
+ int diff, len =
  sizeof(a->grandmasterPriority1) +
  sizeof(a->grandmasterClockQuality) +
  sizeof(a->grandmasterPriority2) +
  sizeof(a->grandmasterIdentity) +
  sizeof(a->stepsRemoved);
 
- return memcmp(&a->grandmasterPriority1, &b-

Post by Richard Cochran
grandmasterPriority1, len);

+ diff = memcmp(&a->grandmasterPriority1, &b-

Post by Richard Cochran
grandmasterPriority1, len);

+ if (diff) {
+ return diff;
+ }
+ return memcmp(&m1->header.sourcePortIdentity,
+       &m2->header.sourcePortIdentity,
+       sizeof(m1->header.sourcePortIdentity));
 }
 
 static void announce_to_dataset(struct ptp_message *m, struct port
*p,

------------------------------------------------------------------------------

[Richard Cochran]

Post by Richard Cochran
However, the current code ignores that field when deciding whether the
inputs have changed or not. This patch fixes the comparison to include
the source port id.

So this patch is not needed after all.

Each port already keeps separate foreign_clock instances,
differentiated by the sourcePortIdentity, and so this memcmp

Post by Richard Cochran
+ return memcmp(&m1->header.sourcePortIdentity,
+ &m2->header.sourcePortIdentity,
+ sizeof(m1->header.sourcePortIdentity));

always returns zero.

Thanks,
Richard

------------------------------------------------------------------------------

[Keller, Jacob E]

Post by Richard Cochran

Post by Richard Cochran
However, the current code ignores that field when deciding whether the
inputs have changed or not.  This patch fixes the comparison to
include
the source port id.

So this patch is not needed after all.
Each port already keeps separate foreign_clock instances,
differentiated by the sourcePortIdentity, and so this memcmp

Post by Richard Cochran
+ return memcmp(&m1->header.sourcePortIdentity,
+       &m2->header.sourcePortIdentity,
+       sizeof(m1->header.sourcePortIdentity));

always returns zero.

I see. So only the first patch is necessary then? Would it be worth
adding a comment to this check to indicate why we don't need to check
the source Port identities?

Thanks,
Jake

Post by Richard Cochran
Thanks,
Richard
-------------------------------------------------------------------
-----------
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

------------------------------------------------------------------------------

[Richard Cochran]

Post by Keller, Jacob E
I see. So only the first patch is necessary then?

Yes.

Post by Keller, Jacob E
Would it be worth
adding a comment to this check to indicate why we don't need to check
the source Port identities?

It wouldn't hurt.

Thanks,
Richard

------------------------------------------------------------------------------