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

Reply via email to