[Linuxptp-devel] [PATCH RFC 0/6] Prepare for TC, Batch I

[Richard Cochran]

This series prepares the way for a Transparent Clock implementation.

Patches 1-4 are more or less trivial cleanups.

Patch 5 moves a large block of code from main() in ptp4l.c into
clock_create(). Right now, much of the clock initialization logic is
performed in main(), but that stuff really belongs in the clock
instance. Two more similar patches are coming in Batch II.

Patch 6 simplifies the clock_create() interface even more.

Review and comments are welcome.

Thanks,
Richard


Richard Cochran (6):
Properly initialize the message lists.
fault: protect header against multiple inclusion.
print: add missing include directive.
config: get the time stamp information early.
clock: simplify the create method.
clock: remove redundant parameter from the create method.

clock.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
clock.h | 6 +----
config.c | 7 +----
config.h | 1 -
ds.h | 5 ----
fault.h | 5 ++++
msg.c | 2 +-
port.c | 1 +
print.h | 2 ++
ptp4l.c | 86 ++---------------------------------------------------------
10 files changed, 97 insertions(+), 111 deletions(-)

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
fault.h | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/fault.h b/fault.h
index fd42b82..841ceb6 100644
--- a/fault.h
+++ b/fault.h
@@ -16,6 +16,9 @@
* with this program; if not, write to the Free Software Foundation, Inc.,
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
+#ifndef HAVE_FAULT_H
+#define HAVE_FAULT_H
+
#include <stdint.h>

enum fault_type {
@@ -37,3 +40,5 @@ struct fault_interval {
};

const char *ft_str(enum fault_type ft);
+
+#endif

[Richard Cochran]

The message lists are implemented using a TAILQ from queue(3). The heads
of the list must be initialized using the provided macros, since the field
called 'tqh_last' is non-zero in the initial state. This patch fixes a
potential null pointer dereference by properly initializing the queues.

Note that there is no actual bug in the current code, because it uses the
lists in such a way as to initialize 'tqh_last' before any dereference.

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

diff --git a/msg.c b/msg.c
index 06a3812..a38b815 100644
--- a/msg.c
+++ b/msg.c
@@ -42,7 +42,7 @@ struct message_storage {
struct ptp_message msg;
} PACKED;

-static TAILQ_HEAD(msg_pool, ptp_message) msg_pool;
+static TAILQ_HEAD(msg_pool, ptp_message) msg_pool = TAILQ_HEAD_INITIALIZER(msg_pool);

static struct {
int total;
diff --git a/port.c b/port.c
index 3f32433..93a79b1 100644
--- a/port.c
+++ b/port.c
@@ -376,6 +376,7 @@ static int add_foreign_master(struct port *p, struct ptp_message *m)
return 0;
}
memset(fc, 0, sizeof(*fc));
+ TAILQ_INIT(&fc->messages);
LIST_INSERT_HEAD(&p->foreign_masters, fc, list);
fc->port = p;
fc->dataset.sender = m->header.sourcePortIdentity;

[Richard Cochran]

This patch adds a #include needed for the declaration of the function,
rate_limited.

Signed-off-by: Richard Cochran <***@gmail.com>
---
print.h | 2 ++
1 file changed, 2 insertions(+)

diff --git a/print.h b/print.h
index 6d7aa94..e8f2c8e 100644
--- a/print.h
+++ b/print.h
@@ -22,6 +22,8 @@

#include <syslog.h>

+#include "util.h"
+
#define PRINT_LEVEL_MIN LOG_EMERG
#define PRINT_LEVEL_MAX LOG_DEBUG

[Richard Cochran]

There is no need to have a function to get this information as
we can easily obtain it when the interface is first created.

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 7 +------
config.h | 1 -
ptp4l.c | 1 -
3 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/config.c b/config.c
index d462410..7bb3b05 100644
--- a/config.c
+++ b/config.c
@@ -534,7 +534,6 @@ int config_read(char *name, struct config *cfg)
current_port = config_create_interface(port, cfg);
if (!current_port)
goto parse_error;
- config_init_interface(current_port, cfg);
}
continue;
}
@@ -606,15 +605,11 @@ struct interface *config_create_interface(char *name, struct config *cfg)
}

strncpy(iface->name, name, MAX_IFNAME_SIZE);
+ sk_get_ts_info(iface->name, &iface->ts_info);
STAILQ_INSERT_TAIL(&cfg->interfaces, iface, list);
return iface;
}

-void config_init_interface(struct interface *iface, struct config *cfg)
-{
- sk_get_ts_info(iface->name, &iface->ts_info);
-}
-
struct config *config_create(void)
{
char buf[CONFIG_LABEL_SIZE + 8];
diff --git a/config.h b/config.h
index 8aa7322..ad057e9 100644
--- a/config.h
+++ b/config.h
@@ -48,7 +48,6 @@ struct config {

int config_read(char *name, struct config *cfg);
struct interface *config_create_interface(char *name, struct config *cfg);
-void config_init_interface(struct interface *iface, struct config *cfg);
void config_destroy(struct config *cfg);

/* New, hash table based methods: */
diff --git a/ptp4l.c b/ptp4l.c
index 662e6e6..e250e2f 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -296,7 +296,6 @@ int main(int argc, char *argv[])
/* Init interface configs and check whether timestamping mode is
* supported. */
STAILQ_FOREACH(iface, &cfg->interfaces, list) {
- config_init_interface(iface, cfg);
if (iface->ts_info.valid &&
((iface->ts_info.so_timestamping & required_modes) != required_modes)) {
fprintf(stderr, "interface '%s' does not support "

[Richard Cochran]

There is no need for the 'ifaces' parameter since the list of network
interfaces is already present in the configuration.

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

diff --git a/clock.c b/clock.c
index 5198f72..8154087 100644
--- a/clock.c
+++ b/clock.c
@@ -798,8 +798,7 @@ static void clock_remove_port(struct clock *c, struct port *p)
port_close(p);
}

-struct clock *clock_create(struct config *config, int phc_index,
- struct interfaces_head *ifaces)
+struct clock *clock_create(struct config *config, int phc_index)
{
enum timestamp_type timestamping =
config_get_int(config, NULL, "time_stamping");
@@ -810,7 +809,7 @@ struct clock *clock_create(struct config *config, int phc_index,
unsigned char oui[OUI_LEN];
char phc[32], *tmp;
struct interface *udsif = &c->uds_interface;
- struct interface *iface = STAILQ_FIRST(ifaces);
+ struct interface *iface = STAILQ_FIRST(&config->interfaces);
struct timespec ts;
int sfl;

@@ -1017,7 +1016,7 @@ struct clock *clock_create(struct config *config, int phc_index,
clock_fda_changed(c);

/* Create the ports. */
- STAILQ_FOREACH(iface, ifaces, list) {
+ STAILQ_FOREACH(iface, &config->interfaces, list) {
if (clock_add_port(c, phc_index, timestamping, iface)) {
pr_err("failed to open port %s", iface->name);
return NULL;
diff --git a/clock.h b/clock.h
index 58ce129..e096dfa 100644
--- a/clock.h
+++ b/clock.h
@@ -71,11 +71,9 @@ struct config *clock_config(struct clock *c);
* @param config Pointer to the configuration database.
* @param phc_index PTP hardware clock device to use.
* Pass -1 to select CLOCK_REALTIME.
- * @param ifaces A queue of network interfaces.
* @return A pointer to the single global clock instance.
*/
-struct clock *clock_create(struct config *config, int phc_index,
- struct interfaces_head *ifaces);
+struct clock *clock_create(struct config *config, int phc_index);

/**
* Obtains a clock's default data set.
diff --git a/ptp4l.c b/ptp4l.c
index 0ccb4ba..0eb2490 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -254,7 +254,7 @@ int main(int argc, char *argv[])
pr_info("selected /dev/ptp%d as PTP clock", phc_index);
}

- clock = clock_create(cfg, phc_index, &cfg->interfaces);
+ clock = clock_create(cfg, phc_index);
if (!clock) {
fprintf(stderr, "failed to create a clock\n");
goto out;

[Richard Cochran]

With the new configuration API, there is no need to pass the default data
set. Instead, the clock code can read the configuration directly. This
patch simplifies the clock create method by removing the 'dds' parameter
and moving the code that initialized the data set into the clock module.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
clock.h | 4 +--
ds.h | 5 ----
ptp4l.c | 85 ++-----------------------------------------------------------
4 files changed, 86 insertions(+), 98 deletions(-)

diff --git a/clock.c b/clock.c
index acfc7a3..5198f72 100644
--- a/clock.c
+++ b/clock.c
@@ -799,8 +799,7 @@ static void clock_remove_port(struct clock *c, struct port *p)
}

struct clock *clock_create(struct config *config, int phc_index,
- struct interfaces_head *ifaces,
- struct default_ds *dds)
+ struct interfaces_head *ifaces)
{
enum timestamp_type timestamping =
config_get_int(config, NULL, "time_stamping");
@@ -808,9 +807,10 @@ struct clock *clock_create(struct config *config, int phc_index,
enum servo_type servo = config_get_int(config, NULL, "clock_servo");
struct clock *c = &the_clock;
struct port *p;
- char phc[32];
+ unsigned char oui[OUI_LEN];
+ char phc[32], *tmp;
struct interface *udsif = &c->uds_interface;
- struct interface *iface;
+ struct interface *iface = STAILQ_FIRST(ifaces);
struct timespec ts;
int sfl;

@@ -820,6 +820,85 @@ struct clock *clock_create(struct config *config, int phc_index,
if (c->nports)
clock_destroy(c);

+ /* Initialize the defaultDS. */
+ c->dds.clockQuality.clockClass =
+ config_get_int(config, NULL, "clockClass");
+ c->dds.clockQuality.clockAccuracy =
+ config_get_int(config, NULL, "clockAccuracy");
+ c->dds.clockQuality.offsetScaledLogVariance =
+ config_get_int(config, NULL, "offsetScaledLogVariance");
+
+ c->desc.productDescription.max_symbols = 64;
+ c->desc.revisionData.max_symbols = 32;
+ c->desc.userDescription.max_symbols = 128;
+
+ tmp = config_get_string(config, NULL, "productDescription");
+ if (count_char(tmp, ';') != 2 ||
+ static_ptp_text_set(&c->desc.productDescription, tmp)) {
+ pr_err("invalid productDescription '%s'", tmp);
+ return NULL;
+ }
+ tmp = config_get_string(config, NULL, "revisionData");
+ if (count_char(tmp, ';') != 2 ||
+ static_ptp_text_set(&c->desc.revisionData, tmp)) {
+ pr_err("invalid revisionData '%s'", tmp);
+ return NULL;
+ }
+ tmp = config_get_string(config, NULL, "userDescription");
+ if (static_ptp_text_set(&c->desc.userDescription, tmp)) {
+ pr_err("invalid userDescription '%s'", tmp);
+ return NULL;
+ }
+ tmp = config_get_string(config, NULL, "manufacturerIdentity");
+ if (OUI_LEN != sscanf(tmp, "%hhx:%hhx:%hhx", &oui[0], &oui[1], &oui[2])) {
+ pr_err("invalid manufacturerIdentity '%s'", tmp);
+ return NULL;
+ }
+ memcpy(c->desc.manufacturerIdentity, oui, OUI_LEN);
+
+ c->dds.domainNumber = config_get_int(config, NULL, "domainNumber");
+
+ if (config_get_int(config, NULL, "slaveOnly")) {
+ c->dds.flags |= DDS_SLAVE_ONLY;
+ }
+ if (config_get_int(config, NULL, "twoStepFlag")) {
+ c->dds.flags |= DDS_TWO_STEP_FLAG;
+ }
+ c->dds.priority1 = config_get_int(config, NULL, "priority1");
+ c->dds.priority2 = config_get_int(config, NULL, "priority2");
+
+ if (!config_get_int(config, NULL, "gmCapable") &&
+ c->dds.flags & DDS_SLAVE_ONLY) {
+ pr_err("Cannot mix 1588 slaveOnly with 802.1AS !gmCapable");
+ return NULL;
+ }
+ if (!config_get_int(config, NULL, "gmCapable") ||
+ c->dds.flags & DDS_SLAVE_ONLY) {
+ c->dds.clockQuality.clockClass = 255;
+ }
+
+ if (!(c->dds.flags & DDS_TWO_STEP_FLAG)) {
+ switch (config_get_int(config, NULL, "time_stamping")) {
+ case TS_SOFTWARE:
+ case TS_LEGACY_HW:
+ pr_err("one step is only possible "
+ "with hardware time stamping");
+ return NULL;
+ case TS_HARDWARE:
+ if (config_set_int(config, "time_stamping", TS_ONESTEP))
+ return NULL;
+ break;
+ case TS_ONESTEP:
+ break;
+ }
+ }
+
+ if (generate_clock_identity(&c->dds.clockIdentity, iface->name)) {
+ pr_err("failed to generate a clock identity");
+ return NULL;
+ }
+
+ /* Configure the UDS. */
snprintf(udsif->name, sizeof(udsif->name), "%s",
config_get_string(config, NULL, "uds_address"));
if (config_set_section_int(config, udsif->name,
@@ -841,7 +920,6 @@ struct clock *clock_create(struct config *config, int phc_index,
c->kernel_leap = config_get_int(config, NULL, "kernel_leap");
c->utc_offset = CURRENT_UTC_OFFSET;
c->time_source = config_get_int(config, NULL, "timeSource");
- c->desc = dds->clock_desc;

if (c->free_running) {
c->clkid = CLOCK_INVALID;
@@ -911,8 +989,6 @@ struct clock *clock_create(struct config *config, int phc_index,
}
}

- c->dds = dds->dds;
-
/* Initialize the parentDS. */
clock_update_grandmaster(c);
c->dad.pds.parentStats = 0;
diff --git a/clock.h b/clock.h
index 9a1151f..58ce129 100644
--- a/clock.h
+++ b/clock.h
@@ -72,12 +72,10 @@ struct config *clock_config(struct clock *c);
* @param phc_index PTP hardware clock device to use.
* Pass -1 to select CLOCK_REALTIME.
* @param ifaces A queue of network interfaces.
- * @param dds A pointer to a default data set for the clock.
* @return A pointer to the single global clock instance.
*/
struct clock *clock_create(struct config *config, int phc_index,
- struct interfaces_head *ifaces,
- struct default_ds *dds);
+ struct interfaces_head *ifaces);

/**
* Obtains a clock's default data set.
diff --git a/ds.h b/ds.h
index 092a3c3..b36862d 100644
--- a/ds.h
+++ b/ds.h
@@ -50,11 +50,6 @@ struct clock_description {
Octet manufacturerIdentity[OUI_LEN];
};

-struct default_ds {
- struct defaultDS dds;
- struct clock_description clock_desc;
-};
-
struct dataset {
UInteger8 priority1;
struct ClockIdentity identity;
diff --git a/ptp4l.c b/ptp4l.c
index e250e2f..0ccb4ba 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -39,8 +39,6 @@

int assume_two_step = 0;

-static struct default_ds ptp4l_dds;
-
static void usage(char *progname)
{
fprintf(stderr,
@@ -75,15 +73,12 @@ static void usage(char *progname)

int main(int argc, char *argv[])
{
- char *config = NULL, *req_phc = NULL, *progname, *tmp;
+ char *config = NULL, *req_phc = NULL, *progname;
int c, err = -1;
struct interface *iface;
struct clock *clock = NULL;
struct config *cfg;
- struct default_ds *dds = &ptp4l_dds;
- struct defaultDS *ds = &ptp4l_dds.dds;
int phc_index = -1, print_level, required_modes = 0;
- unsigned char oui[OUI_LEN];

if (handle_term_signals())
return -1;
@@ -192,61 +187,6 @@ int main(int argc, char *argv[])
sk_check_fupsync = config_get_int(cfg, NULL, "check_fup_sync");
sk_tx_timeout = config_get_int(cfg, NULL, "tx_timestamp_timeout");

- ds->clockQuality.clockClass = config_get_int(cfg, NULL, "clockClass");
- ds->clockQuality.clockAccuracy = config_get_int(cfg, NULL, "clockAccuracy");
- ds->clockQuality.offsetScaledLogVariance =
- config_get_int(cfg, NULL, "offsetScaledLogVariance");
-
- dds->clock_desc.productDescription.max_symbols = 64;
- dds->clock_desc.revisionData.max_symbols = 32;
- dds->clock_desc.userDescription.max_symbols = 128;
-
- tmp = config_get_string(cfg, NULL, "productDescription");
- if (count_char(tmp, ';') != 2 ||
- static_ptp_text_set(&dds->clock_desc.productDescription, tmp)) {
- fprintf(stderr, "invalid productDescription '%s'.\n", tmp);
- goto out;
- }
- tmp = config_get_string(cfg, NULL, "revisionData");
- if (count_char(tmp, ';') != 2 ||
- static_ptp_text_set(&dds->clock_desc.revisionData, tmp)) {
- fprintf(stderr, "invalid revisionData '%s'.\n", tmp);
- goto out;
- }
- tmp = config_get_string(cfg, NULL, "userDescription");
- if (static_ptp_text_set(&dds->clock_desc.userDescription, tmp)) {
- fprintf(stderr, "invalid userDescription '%s'.\n", tmp);
- goto out;
- }
- tmp = config_get_string(cfg, NULL, "manufacturerIdentity");
- if (OUI_LEN != sscanf(tmp, "%hhx:%hhx:%hhx", &oui[0], &oui[1], &oui[2])) {
- fprintf(stderr, "invalid manufacturerIdentity '%s'.\n", tmp);
- goto out;
- }
- memcpy(dds->clock_desc.manufacturerIdentity, oui, OUI_LEN);
-
- ds->domainNumber = config_get_int(cfg, NULL, "domainNumber");
-
- if (config_get_int(cfg, NULL, "slaveOnly")) {
- ds->flags |= DDS_SLAVE_ONLY;
- ds->clockQuality.clockClass = 248;
- }
- if (config_get_int(cfg, NULL, "twoStepFlag")) {
- ds->flags |= DDS_TWO_STEP_FLAG;
- }
- ds->priority1 = config_get_int(cfg, NULL, "priority1");
- ds->priority2 = config_get_int(cfg, NULL, "priority2");
-
- if (!config_get_int(cfg, NULL, "gmCapable") &&
- ds->flags & DDS_SLAVE_ONLY) {
- fprintf(stderr,
- "Cannot mix 1588 slaveOnly with 802.1AS !gmCapable.\n");
- goto out;
- }
- if (!config_get_int(cfg, NULL, "gmCapable") ||
- ds->flags & DDS_SLAVE_ONLY) {
- ds->clockQuality.clockClass = 255;
- }
if (config_get_int(cfg, NULL, "clock_servo") == CLOCK_SERVO_NTPSHM) {
config_set_int(cfg, "kernel_leap", 0);
config_set_int(cfg, "sanity_freq_limit", 0);
@@ -258,22 +198,6 @@ int main(int argc, char *argv[])
goto out;
}

- if (!(ds->flags & DDS_TWO_STEP_FLAG)) {
- switch (config_get_int(cfg, NULL, "time_stamping")) {
- case TS_SOFTWARE:
- case TS_LEGACY_HW:
- fprintf(stderr, "one step is only possible "
- "with hardware time stamping\n");
- goto out;
- case TS_HARDWARE:
- if (config_set_int(cfg, "time_stamping", TS_ONESTEP))
- goto out;
- break;
- case TS_ONESTEP:
- break;
- }
- }
-
switch (config_get_int(cfg, NULL, "time_stamping")) {
case TS_SOFTWARE:
required_modes |= SOF_TIMESTAMPING_TX_SOFTWARE |
@@ -330,12 +254,7 @@ int main(int argc, char *argv[])
pr_info("selected /dev/ptp%d as PTP clock", phc_index);
}

- if (generate_clock_identity(&ds->clockIdentity, iface->name)) {
- fprintf(stderr, "failed to generate a clock identity\n");
- goto out;
- }
-
- clock = clock_create(cfg, phc_index, &cfg->interfaces, &ptp4l_dds);
+ clock = clock_create(cfg, phc_index, &cfg->interfaces);
if (!clock) {
fprintf(stderr, "failed to create a clock\n");
goto out;

[Keller, Jacob E]

-----Original Message-----
Sent: Sunday, April 03, 2016 6:12 AM
Subject: [Linuxptp-devel] [PATCH RFC 0/6] Prepare for TC, Batch I
This series prepares the way for a Transparent Clock implementation.
Patches 1-4 are more or less trivial cleanups.
Patch 5 moves a large block of code from main() in ptp4l.c into
clock_create(). Right now, much of the clock initialization logic is
performed in main(), but that stuff really belongs in the clock
instance. Two more similar patches are coming in Batch II.
Patch 6 simplifies the clock_create() interface even more.
Review and comments are welcome.
Thanks,
Richard

The series looks good to me. Several really simple and nice fixes, plus two changes to reduce the complexity of the clock_create API.

I didn't see anything obviously wrong yet.

Thanks,
Jake



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