[Linuxptp-devel] [PATCH RFC 0/3] Implement the gmCapable flag for 802.1AS

[Richard Cochran]

This series addresses the mismatch between the slaveOnly BMC logic
from the 1588 standard and the expected gmCapable behavior from
802.1AS. That latter standard does not have slave only nodes, but
rather all nodes must send announce messages.

The basic idea is to just use the normal BMC state machine and simply
avoid sending sync messages, while letting the announce messages out.

Thanks,
Richard


Richard Cochran (3):
Introduce the gmCapable flag for use with 802.1AS clocks.
Inhibit sync messages from unwilling 802.1AS ports.
Add a configuration file option for the 802.1AS gmCapable flag.

clock.c | 7 +++++++
clock.h | 8 ++++++++
config.c | 6 ++++++
ds.h | 1 +
gPTP.cfg | 2 +-
port.c | 28 ++++++++++++++++++++++++++++
ptp4l.8 | 7 +++++++
ptp4l.c | 9 ++++++++-
8 files changed, 66 insertions(+), 2 deletions(-)

[Richard Cochran]

According to 802.1AS, ports are always expected to transmit announce
messages, even if they never want to become the grand master. Instead
of using a slave only BMC state machine as in 1588, 802.1AS offers a
"grand master capable" flag which allows clocks to not send sync
messages.

This patch keeps a port from transmitting sync (but not announce)
messages when there is no other master.

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

diff --git a/port.c b/port.c
index 13a34a2..df94e8c 100644
--- a/port.c
+++ b/port.c
@@ -532,6 +532,31 @@ static int port_ignore(struct port *p, struct ptp_message *m)
return 0;
}

+/*
+ * Test whether a 802.1AS port may transmit a sync message.
+ */
+static int port_incapable(struct port *p)
+{
+ struct ClockIdentity cid;
+ struct PortIdentity pid;
+
+ if (!port_is_ieee8021as(p)) {
+ return 0;
+ }
+ if (clock_gm_capable(p->clock)) {
+ return 0;
+ }
+ cid = clock_identity(p->clock);
+ pid = clock_parent_identity(p->clock);
+ if (!memcmp(&cid, &pid.clockIdentity, sizeof(cid))) {
+ /*
+ * We are the GM, but without gmCapable set.
+ */
+ return 1;
+ }
+ return 0;
+}
+
static int port_is_ieee8021as(struct port *p)
{
return p->pod.follow_up_info ? 1 : 0;
@@ -1148,6 +1173,9 @@ static int port_tx_sync(struct port *p)
if (!port_capable(p)) {
return 0;
}
+ if (port_incapable(p)) {
+ return 0;
+ }
msg = msg_allocate();
if (!msg)
return -1;

[Keller, Jacob E]

Post by Richard Cochran
According to 802.1AS, ports are always expected to transmit announce
messages, even if they never want to become the grand master. Instead
of using a slave only BMC state machine as in 1588, 802.1AS offers a
"grand master capable" flag which allows clocks to not send sync
messages.
This patch keeps a port from transmitting sync (but not announce)
messages when there is no other master.
---
port.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/port.c b/port.c
index 13a34a2..df94e8c 100644
--- a/port.c
+++ b/port.c
@@ -532,6 +532,31 @@ static int port_ignore(struct port *p, struct ptp_message *m)
return 0;
}
+/*
+ * Test whether a 802.1AS port may transmit a sync message.
+ */
+static int port_incapable(struct port *p)
+{
+ struct ClockIdentity cid;
+ struct PortIdentity pid;
+
+ if (!port_is_ieee8021as(p)) {
+ return 0;
+ }
+ if (clock_gm_capable(p->clock)) {
+ return 0;
+ }
+ cid = clock_identity(p->clock);
+ pid = clock_parent_identity(p->clock);
+ if (!memcmp(&cid, &pid.clockIdentity, sizeof(cid))) {
+ /*
+ * We are the GM, but without gmCapable set.
+ */
+ return 1;
+ }
+ return 0;
+}
+
static int port_is_ieee8021as(struct port *p)
{
return p->pod.follow_up_info ? 1 : 0;
@@ -1148,6 +1173,9 @@ static int port_tx_sync(struct port *p)
if (!port_capable(p)) {
return 0;
}

Is there a specific reason that port_capable and port_incapable are not
the same function?

Regards,
Jake

Post by Richard Cochran
+ if (port_incapable(p)) {
+ return 0;
+ }
msg = msg_allocate

[Richard Cochran]

Post by Keller, Jacob E
Is there a specific reason that port_capable and port_incapable are not
the same function?

The port_capable function is used in a few different places in the
code (including in port_tx_announce), but port_incapable is only used
by port_tx_sync. Maybe a better name would be port_sync_incapable.

Thanks,
Richard

[Keller, Jacob E]

Post by Richard Cochran

Post by Keller, Jacob E
Is there a specific reason that port_capable and port_incapable are not
the same function?

The port_capable function is used in a few different places in the
code (including in port_tx_announce), but port_incapable is only used
by port_tx_sync. Maybe a better name would be port_sync_incapable.
Thanks,
Richard

Yes I would prefer the other name

[Delio Brignoli]

Post by Richard Cochran

Post by Keller, Jacob E
Is there a specific reason that port_capable and port_incapable are not
the same function?

The port_capable function is used in a few different places in the
code (including in port_tx_announce), but port_incapable is only used
by port_tx_sync. Maybe a better name would be port_sync_incapable.
Thanks,
Richard

Yes I would prefer the other name :)

As would I. Thank you Richard for taking the time to prepare this patch series.

--
Delio

[Richard Cochran]

Post by Delio Brignoli
As would I. Thank you Richard for taking the time to prepare this patch series.

So does this fix the issue you were seeing?

If so, I'll respin it, adjusted for the comments.

Thanks,
Richard

[Delio Brignoli]

Post by Richard Cochran

Post by Delio Brignoli
As would I. Thank you Richard for taking the time to prepare this patch series.

So does this fix the issue you were seeing?

It does fix that particular issue.

Post by Richard Cochran
If so, I'll respin it, adjusted for the comments.

Yes, please.

Thank you
--
Delio

[Delio Brignoli]

Hello Richard,

Any news about this series? Let me know if you'd like me to respin it instead.

Thanks
--
Delio

Post by Delio Brignoli

Post by Richard Cochran

Post by Delio Brignoli
As would I. Thank you Richard for taking the time to prepare this patch series.

So does this fix the issue you were seeing?

It does fix that particular issue.

Post by Richard Cochran
If so, I'll respin it, adjusted for the comments.

Yes, please.
Thank you
--
Delio
------------------------------------------------------------------------------
Sponsored by Intel(R) XDK
Develop, test and display web and hybrid apps with a single code base.
Download it for free now!
http://pubads.g.doubleclick.net/gampad/clk?id=111408631&iu=/4140/ostg.clktrk
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Richard Cochran]

Post by Delio Brignoli
Hello Richard,
Any news about this series? Let me know if you'd like me to respin it instead.

I will push this out later today.

Thanks,
Richard

[Delio Brignoli]

Post by Richard Cochran

Post by Delio Brignoli
Hello Richard,
Any news about this series? Let me know if you'd like me to respin it instead.

I will push this out later today.

Thank you!
--
Delio

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 7 +++++++
clock.h | 8 ++++++++
ds.h | 1 +
ptp4l.c | 1 +
4 files changed, 17 insertions(+)

diff --git a/clock.c b/clock.c
index 1563ba9..83b700e 100644
--- a/clock.c
+++ b/clock.c
@@ -76,6 +76,7 @@ struct clock {
int nports; /* does not include the UDS port */
int free_running;
int freq_est_interval;
+ int grand_master_capable; /* for 802.1AS only */
int utc_timescale;
int leap_set;
int kernel_leap;
@@ -587,6 +588,7 @@ struct clock *clock_create(int phc_index, struct interface *iface, int count,

c->free_running = dds->free_running;
c->freq_est_interval = dds->freq_est_interval;
+ c->grand_master_capable = dds->grand_master_capable;
c->kernel_leap = dds->kernel_leap;
c->utc_offset = CURRENT_UTC_OFFSET;
c->time_source = dds->time_source;
@@ -745,6 +747,11 @@ void clock_follow_up_info(struct clock *c, struct follow_up_info_tlv *f)
sizeof(c->status.lastGmPhaseChange));
}

+int clock_gm_capable(struct clock *c)
+{
+ return c->grand_master_capable;
+}
+
struct ClockIdentity clock_identity(struct clock *c)
{
return c->dds.clockIdentity;
diff --git a/clock.h b/clock.h
index bc66dce..52d50b2 100644
--- a/clock.h
+++ b/clock.h
@@ -101,6 +101,14 @@ UInteger8 clock_domain_number(struct clock *c);
void clock_follow_up_info(struct clock *c, struct follow_up_info_tlv *f);

/**
+ * Obtain the gmCapable flag from a clock's default data set.
+ * This function is specific to the 802.1AS standard.
+ * @param c The clock instance.
+ * @return One if the clock is capable of becoming grand master, zero otherwise.
+ */
+int clock_gm_capable(struct clock *c);
+
+/**
* Obtain a clock's identity from its default data set.
* @param c The clock instance.
* @return The clock's identity.
diff --git a/ds.h b/ds.h
index 6e2ac61..0cc94b5 100644
--- a/ds.h
+++ b/ds.h
@@ -53,6 +53,7 @@ struct default_ds {
struct defaultDS dds;
int free_running;
int freq_est_interval; /*log seconds*/
+ int grand_master_capable; /*802.1AS only*/
int stats_interval; /*log seconds*/
int kernel_leap;
int sanity_freq_limit;
diff --git a/ptp4l.c b/ptp4l.c
index 5d8749a..e86dd91 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -53,6 +53,7 @@ static struct config cfg_settings = {
},
.free_running = 0,
.freq_est_interval = 1,
+ .grand_master_capable = 1,
.stats_interval = 0,
.kernel_leap = 1,
.sanity_freq_limit = 200000000,

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 6 ++++++
gPTP.cfg | 2 +-
ptp4l.8 | 7 +++++++
ptp4l.c | 8 +++++++-
4 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index c86ab01..cfe36b8 100644
--- a/config.c
+++ b/config.c
@@ -255,6 +255,12 @@ static enum parser_result parse_global_setting(const char *option,
dds->flags &= ~DDS_SLAVE_ONLY;
}

+ } else if (!strcmp(option, "gmCapable")) {
+ r = get_ranged_int(value, &val, 0, 1);
+ if (r != PARSED_OK)
+ return r;
+ cfg->dds.grand_master_capable = val;
+
} else if (!strcmp(option, "priority1")) {
r = get_ranged_uint(value, &uval, 0, UINT8_MAX);
if (r != PARSED_OK)
diff --git a/gPTP.cfg b/gPTP.cfg
index 3c0842c..249269e 100644
--- a/gPTP.cfg
+++ b/gPTP.cfg
@@ -3,7 +3,7 @@
# Default Data Set
#
twoStepFlag 1
-slaveOnly 0
+gmCapable 1
priority1 248
priority2 248
domainNumber 0
diff --git a/ptp4l.8 b/ptp4l.8
index c59b6e7..5cf642f 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -212,8 +212,15 @@ The default is 1 (enabled).
.TP
.B slaveOnly
The local clock is a slave-only clock if enabled.
+This option is only for use with 1588 clocks and should not be enabled
+for 802.1AS clocks.
The default is 0 (disabled).
.TP
+.B gmCapable
+If this option is enabled, then the local clock is able to become grand master.
+This is only for use with 802.1AS clocks and has no effect on 1588 clocks.
+The default is 1 (enabled).
+.TP
.B priority1
The priority1 attribute of the local clock. It is used in the best master
selection algorithm, lower values take precedence. Must be in the range 0 to
diff --git a/ptp4l.c b/ptp4l.c
index e86dd91..d9bb470 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -284,7 +284,13 @@ int main(int argc, char *argv[])
if (config && (c = config_read(config, &cfg_settings))) {
return c;
}
- if (ds->flags & DDS_SLAVE_ONLY) {
+ if (!cfg_settings.dds.grand_master_capable &&
+ ds->flags & DDS_SLAVE_ONLY) {
+ fprintf(stderr, "Invalid slaveOnly = 1 with gmCapable = 0.\n");
+ return -1;
+ }
+ if (!cfg_settings.dds.grand_master_capable ||
+ ds->flags & DDS_SLAVE_ONLY) {
ds->clockQuality.clockClass = 255;
}

[Keller, Jacob E]

Post by Richard Cochran
---
config.c | 6 ++++++
gPTP.cfg | 2 +-
ptp4l.8 | 7 +++++++
ptp4l.c | 8 +++++++-
4 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index c86ab01..cfe36b8 100644
--- a/config.c
+++ b/config.c
@@ -255,6 +255,12 @@ static enum parser_result parse_global_setting(const char *option,
dds->flags &= ~DDS_SLAVE_ONLY;
}
+ } else if (!strcmp(option, "gmCapable")) {
+ r = get_ranged_int(value, &val, 0, 1);
+ if (r != PARSED_OK)
+ return r;
+ cfg->dds.grand_master_capable = val;
+
} else if (!strcmp(option, "priority1")) {
r = get_ranged_uint(value, &uval, 0, UINT8_MAX);
if (r != PARSED_OK)
diff --git a/gPTP.cfg b/gPTP.cfg
index 3c0842c..249269e 100644
--- a/gPTP.cfg
+++ b/gPTP.cfg
@@ -3,7 +3,7 @@
# Default Data Set
#
twoStepFlag 1
-slaveOnly 0
+gmCapable 1
priority1 248
priority2 248
domainNumber 0
diff --git a/ptp4l.8 b/ptp4l.8
index c59b6e7..5cf642f 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -212,8 +212,15 @@ The default is 1 (enabled).
.TP
.B slaveOnly
The local clock is a slave-only clock if enabled.
+This option is only for use with 1588 clocks and should not be enabled
+for 802.1AS clocks.
The default is 0 (disabled).
.TP
+.B gmCapable
+If this option is enabled, then the local clock is able to become grand master.
+This is only for use with 802.1AS clocks and has no effect on 1588 clocks.
+The default is 1 (enabled).
+.TP
.B priority1
The priority1 attribute of the local clock. It is used in the best master
selection algorithm, lower values take precedence. Must be in the range 0 to
diff --git a/ptp4l.c b/ptp4l.c
index e86dd91..d9bb470 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -284,7 +284,13 @@ int main(int argc, char *argv[])
if (config && (c = config_read(config, &cfg_settings))) {
return c;
}
- if (ds->flags & DDS_SLAVE_ONLY) {
+ if (!cfg_settings.dds.grand_master_capable &&
+ ds->flags & DDS_SLAVE_ONLY) {
+ fprintf(stderr, "Invalid slaveOnly = 1 with gmCapable = 0.\n");
+ return -1;
+ }

Not sure I like this semi-cryptic error message.. it doesn't really
indicate why this is an issue..

What about instead doing something like a check so that slaveOnly will
warn if you attempt to use it when in 802.1AS mode? (possibly with a
"did you mean gmCapable=0")?

Thanks,
Jake

Post by Richard Cochran
+ if (!cfg_settings.dds.grand_master_capable ||
+ ds->flags & DDS_SLAVE_ONLY) {
ds->clockQuality.clockClass = 255;
}

[Richard Cochran]

Post by Keller, Jacob E
Not sure I like this semi-cryptic error message.. it doesn't really
indicate why this is an issue..

I ended up changing this to:
"Cannot mix 1588 slaveOnly with 802.1AS !gmCapable."

Post by Keller, Jacob E
What about instead doing something like a check so that slaveOnly will
warn if you attempt to use it when in 802.1AS mode? (possibly with a
"did you mean gmCapable=0")?

I thought about this, but it turns out that setting slaveOnly for gPTP
can be useful for testing. It lets the port do the peer delay thing,
but without sending the announce messages. Really the issue is trying
to have both slaveOnly=1 and gmCapable=0.

Anyhow, if you still think the message isn't clear, we can always
change it once we have a better text.

Thanks,
Richard

[Keller, Jacob E]

Post by Richard Cochran

Post by Keller, Jacob E
Not sure I like this semi-cryptic error message.. it doesn't really
indicate why this is an issue..

"Cannot mix 1588 slaveOnly with 802.1AS !gmCapable."

That sounds good.

Post by Richard Cochran

Post by Keller, Jacob E
What about instead doing something like a check so that slaveOnly will
warn if you attempt to use it when in 802.1AS mode? (possibly with a
"did you mean gmCapable=0")?

I thought about this, but it turns out that setting slaveOnly for gPTP
can be useful for testing. It lets the port do the peer delay thing,
but without sending the announce messages. Really the issue is trying
to have both slaveOnly=1 and gmCapable=0.

Sure. That makes sense :)

Post by Richard Cochran
Anyhow, if you still think the message isn't clear, we can always
change it once we have a better text.

Nop