Looking on tlv.h The next TLV is C007 The TLV should be named "MID_TIME_INACCURACY_NP" The "_NP" implies it is LinuxPTP TLV and not a standard one.
The place you add the TLV in tlv.h looks OK. As for the rest, I did not check. Richard except the TLV handling be sorted by the TLV value per function/block. Would be nice to add a short explanation why we need the propose management TLV in the patch letter. Specify standard is good, but we need more. I personally, would use short comments in the code and put the long ones in the patch letter. I think, the patch letter should cover all the changes you have in the patch with explanations, why we need then. Third and last point. I see 3 different changes. I think you should use a patch series, with each change in a separate patch, so we can evaluate each change by itself. As the patches are in a single patch series, they can rely on each other, in the order of the series. i.e one patch for the new configuration parameters, second for the management TLV and the third for the TLV append in the follow up message. Erez -----Original Message----- From: Носиков Владимир Алексеевич <[email protected]> Sent: Tuesday, 2 November 2021 13:41 To: Geva, Erez (ext) (DI PA DCP R&D 3) <[email protected]> Cc: [email protected] Subject: Re: [Linuxptp-devel] [PATCH] Signed-off-by: Vladimir Nosikov <[email protected]> Looks like I took a wrong range, sorry. "This range is to be used for implementation-specific identifiers" C000 - DFFF Any value from this range? Except ofc C000, C001, C003 and C006. ________________________________________ From: Geva, Erez <[email protected]> Sent: Monday, November 1, 2021 9:48:30 PM To: Носиков Владимир Алексеевич Cc: [email protected] Subject: RE: [Linuxptp-devel] [PATCH] Signed-off-by: Vladimir Nosikov <[email protected]> Hi, From IEEE 1588-2019: "This range is to be assigned by an alternate PTP Profile" E000 - FFFE Can you provide the standard reference for this TLV? Erez -----Original Message----- From: Vladimir Nosikov via Linuxptp-devel <[email protected]> Sent: Monday, 1 November 2021 13:45 To: [email protected] Subject: [Linuxptp-devel] [PATCH] Signed-off-by: Vladimir Nosikov <[email protected]> Appending IEEE_C37_238 TLV to IEEE C37.238 Announce message (clock's timeInaccuracy field is settable via the pmc). --- clock.c | 19 +++++++++++++++++++ clock.h | 7 +++++++ config.c | 8 +++++++- pmc.c | 6 ++++++ pmc_common.c | 14 ++++++++++++++ port.c | 29 +++++++++++++++++++++++++++++ port_private.h | 2 ++ tlv.c | 31 +++++++++++++++++++++++++++++++ tlv.h | 19 +++++++++++++++++++ 9 files changed, 134 insertions(+), 1 deletion(-) diff --git a/clock.c b/clock.c index bbfd1a8..7075721 100644 --- a/clock.c +++ b/clock.c @@ -114,6 +114,7 @@ struct clock { int utc_offset; int time_flags; /* grand master role */ int time_source; /* grand master role */ + UInteger32 timeInaccuracy; /* Power C37.238 Profile only */ UInteger8 clock_class_threshold; UInteger8 max_steps_removed; enum servo_state servo_state; @@ -346,6 +347,7 @@ static int clock_management_fill_response(struct clock *c, struct port *p, struct grandmaster_settings_np *gsn; struct management_tlv_datum *mtd; struct subscribe_events_np *sen; + struct time_inaccuracy *ti; struct management_tlv *tlv; struct time_status_np *tsn; struct tlv_extra *extra; @@ -397,6 +399,11 @@ static int clock_management_fill_response(struct clock *c, struct port *p, mtd->val = c->dds.priority2; datalen = sizeof(*mtd); break; + case MID_TIME_INACCURACY: + ti = (struct time_inaccuracy *) tlv->data; + ti->val = c->timeInaccuracy; + datalen = sizeof(*ti); + break; case MID_DOMAIN: mtd = (struct management_tlv_datum *) tlv->data; mtd->val = c->dds.domainNumber; @@ -505,6 +512,7 @@ static int clock_management_set(struct clock *c, struct port *p, struct management_tlv_datum *mtd; struct grandmaster_settings_np *gsn; struct subscribe_events_np *sen; + struct time_inaccuracy *ti; tlv = (struct management_tlv *) req->management.suffix; @@ -521,6 +529,12 @@ static int clock_management_set(struct clock *c, struct port *p, *changed = 1; respond = 1; break; + case MID_TIME_INACCURACY: + ti = (struct time_inaccuracy *) tlv->data; + c->timeInaccuracy = ti->val; + *changed = 1; + respond = 1; + break; case MID_GRANDMASTER_SETTINGS_NP: gsn = (struct grandmaster_settings_np *) tlv->data; c->dds.clockQuality = gsn->clockQuality; @@ -800,6 +814,11 @@ static int forwarding(struct clock *c, struct port *p) /* public methods */ +UInteger32 clock_time_inaccuracy(struct clock *c) { + return c->timeInaccuracy; +} + UInteger8 clock_class(struct clock *c) { return c->dds.clockQuality.clockClass; diff --git a/clock.h b/clock.h index 0534f21..b66e341 100644 --- a/clock.h +++ b/clock.h @@ -59,6 +59,13 @@ struct dataset *clock_best_foreign(struct clock *c); */ struct port *clock_best_port(struct clock *c); +/** + * Obtain the IEEE C37.238 timeInaccuracy attribute from a clock. + * @param c The clock instance. + * @return The value of the clock's time inaccuracy. + */ +UInteger32 clock_time_inaccuracy(struct clock *c); + /** * Obtain the clockClass attribute from a clock. * @param c The clock instance. diff --git a/config.c b/config.c index f3c52ba..814420b 100644 --- a/config.c +++ b/config.c @@ -242,7 +242,11 @@ struct config_item config_tab[] = { PORT_ITEM_INT("delay_response_timeout", 0, 0, UINT8_MAX), GLOB_ITEM_INT("dscp_event", 0, 0, 63), GLOB_ITEM_INT("dscp_general", 0, 0, 63), - GLOB_ITEM_INT("domainNumber", 0, 0, 127), + /* IEEE Std C37.238-2017 5.4: The default configuration for the domain, defaultDS.domainNumber, + * shall be 254. Implementation claiming conformance to this standard shall be able to configure + * domain numbers of the range 0 to 127 and the single value 254. + */ + GLOB_ITEM_INT("domainNumber", 0, 0, UINT8_MAX), PORT_ITEM_INT("egressLatency", 0, INT_MIN, INT_MAX), PORT_ITEM_INT("fault_badpeernet_interval", 16, INT32_MIN, INT32_MAX), PORT_ITEM_INT("fault_reset_interval", 4, INT8_MIN, INT8_MAX), @@ -284,6 +288,8 @@ struct config_item config_tab[] = { PORT_ITEM_INT("operLogPdelayReqInterval", 0, INT8_MIN, INT8_MAX), PORT_ITEM_INT("operLogSyncInterval", 0, INT8_MIN, INT8_MAX), PORT_ITEM_INT("path_trace_enabled", 0, 0, 1), + PORT_ITEM_INT("c37_238_enabled", 0, 0, 1), + PORT_ITEM_INT("c37_238_grandmaster_id", 0, 0x0000, 0xffff), GLOB_ITEM_DBL("pi_integral_const", 0.0, 0.0, DBL_MAX), GLOB_ITEM_DBL("pi_integral_exponent", 0.4, -DBL_MAX, DBL_MAX), GLOB_ITEM_DBL("pi_integral_norm_max", 0.3, DBL_MIN, 2.0), diff --git a/pmc.c b/pmc.c index a1ee787..8e23cb7 100644 --- a/pmc.c +++ b/pmc.c @@ -147,6 +147,7 @@ static void pmc_show(struct ptp_message *msg, FILE *fp) struct timePropertiesDS *tp; struct management_tlv *mgt; struct time_status_np *tsn; + struct time_inaccuracy *ti; struct port_stats_np *pcp; struct tlv_extra *extra; struct port_ds_np *pnp; @@ -307,6 +308,11 @@ static void pmc_show(struct ptp_message *msg, FILE *fp) fprintf(fp, "PRIORITY2 " IFMT "priority2 %hhu", mtd->val); break; + case MID_TIME_INACCURACY: + ti = (struct time_inaccuracy *) mgt->data; + fprintf(fp, "TIME_INACCURACY " + IFMT "timeInaccuracy %u", ti->val); + break; case MID_DOMAIN: mtd = (struct management_tlv_datum *) mgt->data; fprintf(fp, "DOMAIN " diff --git a/pmc_common.c b/pmc_common.c index 7a1dbb4..0599c34 100644 --- a/pmc_common.c +++ b/pmc_common.c @@ -111,6 +111,7 @@ struct management_id idtab[] = { { "GRANDMASTER_SETTINGS_NP", MID_GRANDMASTER_SETTINGS_NP, do_set_action }, { "SUBSCRIBE_EVENTS_NP", MID_SUBSCRIBE_EVENTS_NP, do_set_action }, { "SYNCHRONIZATION_UNCERTAIN_NP", MID_SYNCHRONIZATION_UNCERTAIN_NP, do_set_action }, + { "TIME_INACCURACY", MID_TIME_INACCURACY, do_set_action }, /* Port management ID values */ { "NULL_MANAGEMENT", MID_NULL_MANAGEMENT, null_management }, { "CLOCK_DESCRIPTION", MID_CLOCK_DESCRIPTION, do_get_action }, @@ -149,6 +150,7 @@ static void do_set_action(struct pmc *pmc, int action, int index, char *str) struct grandmaster_settings_np gsn; struct management_tlv_datum mtd; struct subscribe_events_np sen; + struct time_inaccuracy ti; struct port_ds_np pnp; char onoff_port_state[4] = "off"; char onoff_time_status[4] = "off"; @@ -178,6 +180,15 @@ static void do_set_action(struct pmc *pmc, int action, int index, char *str) } pmc_send_set_action(pmc, code, &mtd, sizeof(mtd)); break; + case MID_TIME_INACCURACY: + cnt = sscanf(str, " %*s %*s " "timeInaccuracy %x ", &ti.val); + if (cnt != 1) { + fprintf(stderr, "%s SET needs 1 value\n", + idtab[index].name); + break; + } + pmc_send_set_action(pmc, code, &ti, sizeof(ti)); + break; case MID_GRANDMASTER_SETTINGS_NP: cnt = sscanf(str, " %*s %*s " "clockClass %hhu " @@ -531,6 +542,9 @@ static int pmc_tlv_datalen(struct pmc *pmc, int id) case MID_PORT_DATA_SET_NP: len += sizeof(struct port_ds_np); break; + case MID_TIME_INACCURACY: + len += sizeof(struct time_inaccuracy); + break; case MID_LOG_ANNOUNCE_INTERVAL: case MID_ANNOUNCE_RECEIPT_TIMEOUT: case MID_LOG_SYNC_INTERVAL: diff --git a/port.c b/port.c index 6e6b0aa..c80f85e 100644 --- a/port.c +++ b/port.c @@ -393,6 +393,29 @@ static int add_foreign_master(struct port *p, struct ptp_message *m) return broke_threshold || diff; } +static int c37_238_append(struct port *p, struct ptp_message *m) { + struct c37_238_tlv *c37_238; + struct tlv_extra *extra; + + extra = msg_tlv_append(m, sizeof(*c37_238)); + if (!extra) { + return -1; + } + c37_238 = (struct c37_238_tlv *) extra->tlv; + c37_238->type = TLV_ORGANIZATION_EXTENSION; + c37_238->length = sizeof(*c37_238) - sizeof(c37_238->type) - sizeof(c37_238->length); + memcpy(c37_238->id, ieee_pes_id, sizeof(ieee_pes_id)); + c37_238->subtype[2] = 2; + + c37_238->grandmasterID = p->c37_238_grandmaster_id; + memset(&c37_238->reserved1, 0, sizeof(c37_238->reserved1)); + c37_238->totalTimeInaccuracy = clock_time_inaccuracy(p->clock); + memset(&c37_238->reserved2, 0, sizeof(c37_238->reserved2)); + + return 0; +} + static int follow_up_info_append(struct ptp_message *m) { struct follow_up_info_tlv *fui; @@ -1502,6 +1525,10 @@ int port_tx_announce(struct port *p, struct address *dst) pr_err("%s: append path trace failed", p->log_name); } + if (p->c37_238_enabled && c37_238_append(p, msg)) { + pr_err("port %hu: append IEEE C37.238 TLV failed", portnum(p)); + } + err = port_prepare_and_send(p, msg, TRANS_GENERAL); if (err) { pr_err("%s: send announce failed", p->log_name); @@ -1724,6 +1751,7 @@ int port_initialize(struct port *p) p->transportSpecific <<= 4; p->match_transport_specific = !config_get_int(cfg, p->name, "ignore_transport_specific"); p->localPriority = config_get_int(cfg, p->name, "G.8275.portDS.localPriority"); + p->c37_238_grandmaster_id = config_get_int(cfg, p->name, "c37_238_grandmaster_id"); p->initialLogSyncInterval = config_get_int(cfg, p->name, "logSyncInterval"); p->logSyncInterval = p->initialLogSyncInterval; p->operLogSyncInterval = config_get_int(cfg, p->name, "operLogSyncInterval"); @@ -3159,6 +3187,7 @@ struct port *port_open(const char *phc_device, p->msg_interval_request = config_get_int(cfg, p->name, "msg_interval_request"); p->net_sync_monitor = config_get_int(cfg, p->name, "net_sync_monitor"); p->path_trace_enabled = config_get_int(cfg, p->name, "path_trace_enabled"); + p->c37_238_enabled = config_get_int(cfg, p->name, + "c37_238_enabled"); p->tc_spanning_tree = config_get_int(cfg, p->name, "tc_spanning_tree"); p->rx_timestamp_offset = config_get_int(cfg, p->name, "ingressLatency"); p->rx_timestamp_offset <<= 16; diff --git a/port_private.h b/port_private.h index 5391879..cda0fa5 100644 --- a/port_private.h +++ b/port_private.h @@ -135,6 +135,8 @@ struct port { int min_neighbor_prop_delay; int net_sync_monitor; int path_trace_enabled; + int c37_238_enabled; + uint16_t c37_238_grandmaster_id; int tc_spanning_tree; Integer64 rx_timestamp_offset; Integer64 tx_timestamp_offset; diff --git a/tlv.c b/tlv.c index cc8d507..af8cc97 100644 --- a/tlv.c +++ b/tlv.c @@ -35,6 +35,7 @@ (tlv->length < sizeof(struct type) - sizeof(struct TLV)) uint8_t ieee8021_id[3] = { IEEE_802_1_COMMITTEE }; +uint8_t ieee_pes_id[3] = { IEEE_PES }; static TAILQ_HEAD(tlv_pool, tlv_extra) tlv_pool = TAILQ_HEAD_INITIALIZER(tlv_pool); @@ -559,6 +560,7 @@ static void nsm_resp_pre_send(struct tlv_extra *extra) static int org_post_recv(struct organization_tlv *org) { struct follow_up_info_tlv *f; + struct c37_238_tlv *c37_238; if (0 == memcmp(org->id, ieee8021_id, sizeof(ieee8021_id))) { if (org->subtype[0] || org->subtype[1]) { @@ -579,6 +581,21 @@ static int org_post_recv(struct organization_tlv *org) if (org->length + sizeof(struct TLV) != sizeof(struct msg_interval_req_tlv)) goto bad_length; } + } else if (0 == memcmp(org->id, ieee_pes_id, sizeof(ieee_pes_id))) { + if (org->subtype[0] || org->subtype[1]) { + return 0; + } + switch (org->subtype[2]) { + case 2: + if (org->length + sizeof(struct TLV) != sizeof(struct c37_238_tlv)) + goto bad_length; + c37_238 = (struct c37_238_tlv *) org; + c37_238->grandmasterID = ntohs(c37_238->grandmasterID); + c37_238->reserved1 = ntohl(c37_238->reserved1); + c37_238->totalTimeInaccuracy = ntohl(c37_238->totalTimeInaccuracy); + c37_238->reserved2 = ntohs(c37_238->reserved2); + break; + } } return 0; bad_length: @@ -587,6 +604,7 @@ bad_length: static void org_pre_send(struct organization_tlv *org) { + struct c37_238_tlv *c37_238; struct follow_up_info_tlv *f; if (0 == memcmp(org->id, ieee8021_id, sizeof(ieee8021_id))) { @@ -602,6 +620,19 @@ static void org_pre_send(struct organization_tlv *org) f->scaledLastGmPhaseChange = htonl(f->scaledLastGmPhaseChange); break; } + } else if (0 == memcmp(org->id, ieee_pes_id, sizeof(ieee_pes_id))) { + if (org->subtype[0] || org->subtype[1]) { + return; + } + switch (org->subtype[2]) { + case 2: + c37_238 = (struct c37_238_tlv *) org; + c37_238->grandmasterID = htons(c37_238->grandmasterID); + c37_238->reserved1 = htonl(c37_238->reserved1); + c37_238->totalTimeInaccuracy = htonl(c37_238->totalTimeInaccuracy); + c37_238->reserved2 = htons(c37_238->reserved2); + break; + } } } diff --git a/tlv.h b/tlv.h index 97615fd..8ee5670 100644 --- a/tlv.h +++ b/tlv.h @@ -100,6 +100,7 @@ enum management_action { #define MID_GRANDMASTER_SETTINGS_NP 0xC001 #define MID_SUBSCRIBE_EVENTS_NP 0xC003 #define MID_SYNCHRONIZATION_UNCERTAIN_NP 0xC006 +#define MID_TIME_INACCURACY 0xE000 /* Port management ID values */ #define MID_NULL_MANAGEMENT 0x0000 @@ -204,10 +205,28 @@ struct nsm_resp_tlv_foot { struct Timestamp lastsync; } PACKED; +struct c37_238_tlv { + Enumeration16 type; + UInteger16 length; + Octet id[3]; + Octet subtype[3]; + uint16_t grandmasterID; + uint32_t reserved1; + UInteger32 totalTimeInaccuracy; + uint16_t reserved2; +} PACKED; + +struct time_inaccuracy { + UInteger32 val; /*nanoseconds*/ +} PACKED; + /* Organizationally Unique Identifiers */ #define IEEE_802_1_COMMITTEE 0x00, 0x80, 0xC2 extern uint8_t ieee8021_id[3]; +#define IEEE_PES 0x1c, 0x12, 0x9d +extern uint8_t ieee_pes_id[3]; + struct organization_tlv { Enumeration16 type; UInteger16 length; -- 2.30.2 _______________________________________________ Linuxptp-devel mailing list [email protected] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.sourceforge.net%2Flists%2Flistinfo%2Flinuxptp-devel&data=04%7C01%7Cerez.geva.ext%40siemens.com%7C2d74ac20d15542cac3f708d99dfe0430%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637714536593502953%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=CdGXoVbrdnt%2F1vhg80SU5SRi06Lthu6350F%2BJGL9sX0%3D&reserved=0 _______________________________________________ Linuxptp-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
