Thanks for the reply. Please add the explanation to the commit and to the structure.
Personally, I do not have an opinion, yet I did not participate in the IEEE 1558 committee. Erez On Tue, 16 May 2023 at 23:43, Kishen Maloor <kishen.mal...@intel.com> wrote: > On 5/16/23 3:56 AM, Erez wrote: > > On Tue, 16 May 2023 at 00:29, Kishen Maloor <kishen.mal...@intel.com> > wrote: > > > >> In a setup with multiple gPTP domains, the Common Mean Link Delay > Service > >> (CMLDS) (IEEE 1588/16.6.3) performs link delay measurements in a single > >> domain and must (somehow) convey those to other domains. IEEE 1588 does > not > >> specify this interface and flags it as an implementation > >> detail (IEEE 1588/16.6.1). Accordningly, this change introduces a new > >> TLV to convey link delay measurements by the CMLDS over the management > >> interface. > >> > >> In addition to the parameters suggested in IEEE 1588/16.6.3.2, > >> the TLV also conveys the latest 'up measurements' (req and rx > timestamps) > >> recorded in the CMLDS. These values collectively aid other gPTP domains > to > >> complete their delay/offset computations via the COMMON_P2P > >> delay mechanism. > >> > >> Updated 'pmc' to support the new MID, MID_CMLDS_INFO_NP. > >> > >> Co-authored-by: Andrew Zaborowski <andrew.zaborow...@intel.com> > >> Signed-off-by: Kishen Maloor <kishen.mal...@intel.com> > >> --- > >> clock.c | 1 - > >> msg.h | 2 ++ > >> pmc.c | 13 +++++++++++++ > >> pmc_common.c | 1 + > >> port.c | 22 ++++++++++++++++++++++ > >> port_private.h | 2 ++ > >> tlv.c | 18 ++++++++++++++++++ > >> tlv.h | 11 +++++++++++ > >> 8 files changed, 69 insertions(+), 1 deletion(-) > >> > >> diff --git a/clock.c b/clock.c > >> index fe08d48ed8f8..c74a6baa9f61 100644 > >> --- a/clock.c > >> +++ b/clock.c > >> @@ -47,7 +47,6 @@ > >> #include "util.h" > >> > >> #define N_CLOCK_PFD (N_POLLFD + 1) /* one extra per port, for the fault > >> timer */ > >> -#define POW2_41 ((double)(1ULL << 41)) > >> > >> struct interface { > >> STAILQ_ENTRY(interface) list; > >> diff --git a/msg.h b/msg.h > >> index cbd09e75a2aa..db12e249f89f 100644 > >> --- a/msg.h > >> +++ b/msg.h > >> @@ -69,6 +69,8 @@ > >> #define SIGNAL_NO_CHANGE -128 > >> #define SIGNAL_SET_INITIAL 126 > >> > >> +#define POW2_41 ((double)(1ULL << 41)) > >> + > >> enum timestamp_type { > >> TS_SOFTWARE, > >> TS_HARDWARE, > >> diff --git a/pmc.c b/pmc.c > >> index 00e691f0c244..cac06fb5b41d 100644 > >> --- a/pmc.c > >> +++ b/pmc.c > >> @@ -169,6 +169,7 @@ static void pmc_show(struct ptp_message *msg, FILE > *fp) > >> struct subscribe_events_np *sen; > >> struct port_properties_np *ppn; > >> struct port_hwclock_np *phn; > >> + struct cmlds_info_np *cmlds; > >> struct timePropertiesDS *tp; > >> struct management_tlv *mgt; > >> struct time_status_np *tsn; > >> @@ -651,6 +652,18 @@ static void pmc_show(struct ptp_message *msg, FILE > >> *fp) > >> fprintf(fp, "LOG_MIN_PDELAY_REQ_INTERVAL " > >> IFMT "logMinPdelayReqInterval %hhd", mtd->val); > >> break; > >> + case MID_CMLDS_INFO_NP: > >> + cmlds = (struct cmlds_info_np *) mgt->data; > >> + fprintf(fp, "CMLDS INFO " > >> + IFMT "serviceMeasurementValid %i" > >> + IFMT "meanLinkDelay %" PRId64 > >> + IFMT "scaledNeighborRateRatio %" PRId32 > >> + IFMT "egress_ts %" PRId64 > >> + IFMT "rx_ts %" PRId64, > >> + cmlds->serviceMeasurementValid, > >> cmlds->meanLinkDelay, > >> + cmlds->scaledNeighborRateRatio, > >> + cmlds->egress_ts, cmlds->rx_ts); > >> + break; > >> } > >> out: > >> fprintf(fp, "\n"); > >> diff --git a/pmc_common.c b/pmc_common.c > >> index 9e251c43e95b..d7a6114dcd62 100644 > >> --- a/pmc_common.c > >> +++ b/pmc_common.c > >> @@ -156,6 +156,7 @@ struct management_id idtab[] = { > >> { "UNICAST_MASTER_TABLE_NP", MID_UNICAST_MASTER_TABLE_NP, > >> do_get_action }, > >> { "PORT_HWCLOCK_NP", MID_PORT_HWCLOCK_NP, do_get_action }, > >> { "POWER_PROFILE_SETTINGS_NP", MID_POWER_PROFILE_SETTINGS_NP, > >> do_set_action }, > >> + { "CMLDS_INFO_NP", MID_CMLDS_INFO_NP, do_get_action }, > >> }; > >> > >> static void do_get_action(struct pmc *pmc, int action, int index, char > >> *str) > >> diff --git a/port.c b/port.c > >> index 8b2eb04a855a..d467a69e519a 100644 > >> --- a/port.c > >> +++ b/port.c > >> @@ -887,6 +887,7 @@ static int port_management_fill_response(struct port > >> *target, > >> struct clock_description *desc; > >> struct port_properties_np *ppn; > >> struct port_hwclock_np *phn; > >> + struct cmlds_info_np *cmlds; > >> struct management_tlv *tlv; > >> struct port_stats_np *psn; > >> struct foreign_clock *fc; > >> @@ -1129,6 +1130,27 @@ static int port_management_fill_response(struct > >> port *target, > >> memcpy(pwr, &target->pwr, sizeof(*pwr)); > >> datalen = sizeof(*pwr); > >> break; > >> + case MID_CMLDS_INFO_NP: > >> + cmlds = (struct cmlds_info_np *)tlv->data; > >> + /* IEEE1588-2019 16.6.3.2 h) 1) && nrate.ratio_valid > >> because > >> + * we have no extra field to convey that separately. > >> + */ > >> + cmlds->serviceMeasurementValid = > >> + target->peer_portid_valid && > !target->pdr_missing > >> && > >> + !target->multiple_pdr_detected && > >> + target->nrate.ratio_valid; > >> + cmlds->meanLinkDelay = target->peerMeanPathDelay; > >> + cmlds->scaledNeighborRateRatio = > >> + (Integer32) (target->nrate.ratio * POW2_41 - > >> POW2_41); > >> + /* 16.6.3.2: "Upon receipt of a request for > information, > >> the > >> + * Common Mean Link Delay Service may in addition return > >> the > >> + * raw measurement data gathered by the service for use > in > >> + * estimating the <meanLinkDelay> and > <neighborRateRatio>." > >> + */ > >> + cmlds->egress_ts = > >> tmv_to_nanoseconds(target->peer_delay_t1); > >> + cmlds->rx_ts = > tmv_to_nanoseconds(target->peer_delay_t2); > >> + datalen = sizeof(*cmlds); > >> + break; > >> default: > >> /* The caller should *not* respond to this message. */ > >> tlv_extra_recycle(extra); > >> diff --git a/port_private.h b/port_private.h > >> index 3b02d2fe45c4..c9b02bc799f5 100644 > >> --- a/port_private.h > >> +++ b/port_private.h > >> @@ -99,6 +99,8 @@ struct port { > >> unsigned int pdr_missing; > >> unsigned int multiple_seq_pdr_count; > >> unsigned int multiple_pdr_detected; > >> + tmv_t peer_delay_t1; > >> + tmv_t peer_delay_t2; > >> enum port_state (*state_machine)(enum port_state state, > >> enum fsm_event event, int > mdiff); > >> int bmca; > >> diff --git a/tlv.c b/tlv.c > >> index 79400126cbc4..8b97d2cfc9f5 100644 > >> --- a/tlv.c > >> +++ b/tlv.c > >> @@ -176,6 +176,7 @@ static int mgt_post_recv(struct management_tlv *m, > >> uint16_t data_len, > >> struct port_properties_np *ppn; > >> struct port_hwclock_np *phn; > >> struct timePropertiesDS *tp; > >> + struct cmlds_info_np *cmlds; > >> struct time_status_np *tsn; > >> struct port_stats_np *psn; > >> int extra_len = 0, i, len; > >> @@ -490,6 +491,15 @@ static int mgt_post_recv(struct management_tlv *m, > >> uint16_t data_len, > >> if (data_len != 0) > >> goto bad_length; > >> break; > >> + case MID_CMLDS_INFO_NP: > >> + if (data_len < sizeof(struct cmlds_info_np)) > >> + goto bad_length; > >> + cmlds = (struct cmlds_info_np *)m->data; > >> + net2host64_unaligned(&cmlds->meanLinkDelay); > >> + NTOHL(cmlds->scaledNeighborRateRatio); > >> + net2host64_unaligned(&cmlds->egress_ts); > >> + net2host64_unaligned(&cmlds->rx_ts); > >> + break; > >> } > >> if (extra_len) { > >> if (extra_len % 2) > >> @@ -514,6 +524,7 @@ static void mgt_pre_send(struct management_tlv *m, > >> struct tlv_extra *extra) > >> struct subscribe_events_np *sen; > >> struct port_properties_np *ppn; > >> struct port_hwclock_np *phn; > >> + struct cmlds_info_np *cmlds; > >> struct timePropertiesDS *tp; > >> struct time_status_np *tsn; > >> struct port_stats_np *psn; > >> @@ -672,6 +683,13 @@ static void mgt_pre_send(struct management_tlv *m, > >> struct tlv_extra *extra) > >> HTONL(pwr->networkTimeInaccuracy); > >> HTONL(pwr->totalTimeInaccuracy); > >> break; > >> + case MID_CMLDS_INFO_NP: > >> + cmlds = (struct cmlds_info_np *)m->data; > >> + host2net64_unaligned(&cmlds->meanLinkDelay); > >> + HTONL(cmlds->scaledNeighborRateRatio); > >> + host2net64_unaligned(&cmlds->egress_ts); > >> + host2net64_unaligned(&cmlds->rx_ts); > >> + break; > >> } > >> } > >> > >> diff --git a/tlv.h b/tlv.h > >> index 8b51ffd88816..6446fc068488 100644 > >> --- a/tlv.h > >> +++ b/tlv.h > >> @@ -130,6 +130,9 @@ enum management_action { > >> #define MID_PORT_HWCLOCK_NP 0xC009 > >> #define MID_POWER_PROFILE_SETTINGS_NP 0xC00A > >> > >> +/* CMLDS management ID values */ > >> +#define MID_CMLDS_INFO_NP 0xC00B > >> + > >> /* Management error ID values */ > >> #define MID_RESPONSE_TOO_BIG 0x0001 > >> #define MID_NO_SUCH_ID 0x0002 > >> @@ -473,6 +476,14 @@ struct msg_interface_rate_tlv { > >> UInteger16 numberOfBitsAfterTimestamp; > >> } PACKED; > >> > >> +struct cmlds_info_np { > >> + Integer8 serviceMeasurementValid; > >> + TimeInterval meanLinkDelay; > >> + Integer32 scaledNeighborRateRatio; > > > > + Integer64 egress_ts; > >> + Integer64 rx_ts; > >> > > > > Not really sure what is better. > > But why not using Timestamp?, the annoying type from IEEE. > > Can you please add a remark here with cons and pros and a decision here? > > This TLV (and its conveyance method) is unspecified and is set by the > implementation. > So as such, we're handing these tmv_t timestamps as is to a requesting > port, wherefrom > they're directly passed in a call to clock_peer_delay(). > I guess we didn't think to send these values as struct Timestamp instead > because it wasn't > necessary. > > > > > > >> +} PACKED; > >> + > >> /** > >> * Allocates a new tlv_extra structure. > >> * @return Pointer to a new structure on success or NULL otherwise. > >> -- > >> 2.31.1 > >> > >> > >> > >> _______________________________________________ > >> Linuxptp-devel mailing list > >> Linuxptp-devel@lists.sourceforge.net > >> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel > >> > > > >
_______________________________________________ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel