Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

2023-11-17 Thread Richard Cochran
On Fri, Nov 17, 2023 at 08:27:43PM -0800, Richard Cochran wrote:
> How does this requirement improve synchronization?
> 
> What benefit does it bring to users of the PTP?

rhetorical questions  :^(


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

2023-11-17 Thread Richard Cochran
On Fri, Nov 17, 2023 at 01:51:18PM +0100, Andrew Zaborowski wrote:

> It is exposed on the wire in the Pdelay messages.  Compliance tests
> look at this.  They also simulate a few hypothetical scenarios like a
> domain 0 PTP port trying to communicate with a CMLDS link port since
> 1588 talks about this.

How does this requirement improve synchronization?

What benefit does it bring to users of the PTP?

Thanks,
Richard


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

2023-11-17 Thread Andrew Zaborowski
On Fri, 17 Nov 2023 at 05:50, Richard Cochran  wrote:
> On Fri, Nov 17, 2023 at 01:41:19AM +0100, Andrew Zaborowski wrote:
> > Do you want to require the user to enforce that the port numbering is
> > the same between the ptp4l processes?
>
> No.

(I meant: do you want to require that the user takes care to maintain
the order of interfaces on the command line / in the config file,
which I think you do -- Ok)

>
> > In our use case spec compliance
> > is the priority, including the unique clockIdentity for CMLDS
> > requirement, so we'd definitely need to be running a separate ptp4l
> > process for CMLDS in this schema.
>
> The clockIdentity is not exposed, so what do you mean by "compliance"?

It is exposed on the wire in the Pdelay messages.  Compliance tests
look at this.  They also simulate a few hypothetical scenarios like a
domain 0 PTP port trying to communicate with a CMLDS link port since
1588 talks about this.

Best regards


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

2023-11-16 Thread Richard Cochran
On Fri, Nov 17, 2023 at 01:41:19AM +0100, Andrew Zaborowski wrote:
> On Thu, 16 Nov 2023 at 23:46, Richard Cochran  
> wrote:
> > No, I mean the PTP port number.  These are taken from the order of the
> > interfaces on the command line and in the configuration file.
> 
> Won't this be the same as the UDS message's sourcePortIdentity.portNumber?

Ah, yes, you are right.  Even simpler!

> Do you want to require the user to enforce that the port numbering is
> the same between the ptp4l processes?

No.

> In our use case spec compliance
> is the priority, including the unique clockIdentity for CMLDS
> requirement, so we'd definitely need to be running a separate ptp4l
> process for CMLDS in this schema.

The clockIdentity is not exposed, so what do you mean by "compliance"?

> I'm asking because the port
> numbering thing is unobvious and the user effort to configure a
> compliant setup with ptp4l is already very high.

That is just par for the course.  I'm not the one making up tons of
crazy new options.

The strength of linuxptp is modularity and fine grained, configurable
options.  This means that a) ptp4l works even with equipment that
fails to follow the standards and/or profiles, and b) ptp4l can be
really hard to configure.

> So one solution would be to let the user configure a custom portNumber
> under port settings.

Port numbers start with 1 and increase in the order of the configured
interfaces.  If it really is required for CMLDS ports to have
*different* numbers than the normal ports, we can add a "port index
offset" option that would start the numbering at some given value.

Thanks,
Richard


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

2023-11-16 Thread Andrew Zaborowski
On Thu, 16 Nov 2023 at 23:46, Richard Cochran  wrote:
> On Thu, Nov 16, 2023 at 11:11:50PM +0100, Andrew Zaborowski wrote:
> > The two timestamps are passed to clock_peer_delay() by the receiving
> > port and stored in c->tsproc.  Then they're accessed by
> > get_raw_delay() which is used in the filter logic.  I'm not sure how
> > much value that has, we can possibly pass 0s to clock_peer_delay().
>
> If the client uses CMLDS, then it doesn't need tsproc logic at all.
> It simply take the delay value from the CMLDS server.

Make sense, the CMLDS would already be filtering the timestamps once.

>
> > Regarding "source_port_index", I assume that would contain the ifindex
> > of the interface?  With virtual clocks I believe the PHC indices may
> > differ between the PTP ports on one physical port ("link port").
>
> No, I mean the PTP port number.  These are taken from the order of the
> interfaces on the command line and in the configuration file.

Won't this be the same as the UDS message's sourcePortIdentity.portNumber?

Do you want to require the user to enforce that the port numbering is
the same between the ptp4l processes?  In our use case spec compliance
is the priority, including the unique clockIdentity for CMLDS
requirement, so we'd definitely need to be running a separate ptp4l
process for CMLDS in this schema.  I'm asking because the port
numbering thing is unobvious and the user effort to configure a
compliant setup with ptp4l is already very high.

So one solution would be to let the user configure a custom portNumber
under port settings.  My colleague even had a local change to allow
passing -i ,.  This would also remove the
(possibly unobvious) requirement to run all ports sharing the
clockIdentity as one ptp4l process.

Another option would be to pass the ifindex as I thought you were suggesting.

Best regards


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

2023-11-16 Thread Richard Cochran
On Thu, Nov 16, 2023 at 11:11:50PM +0100, Andrew Zaborowski wrote:

> The two timestamps are passed to clock_peer_delay() by the receiving
> port and stored in c->tsproc.  Then they're accessed by
> get_raw_delay() which is used in the filter logic.  I'm not sure how
> much value that has, we can possibly pass 0s to clock_peer_delay().

If the client uses CMLDS, then it doesn't need tsproc logic at all.
It simply take the delay value from the CMLDS server.
 
> Regarding "source_port_index", I assume that would contain the ifindex
> of the interface?  With virtual clocks I believe the PHC indices may
> differ between the PTP ports on one physical port ("link port").

No, I mean the PTP port number.  These are taken from the order of the
interfaces on the command line and in the configuration file.

Thanks,
Richard


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

2023-11-16 Thread Andrew Zaborowski
On Thu, 16 Nov 2023 at 05:27, Richard Cochran  wrote:
> On Mon, May 15, 2023 at 06:26:04PM -0400, Kishen Maloor wrote:
> > @@ -1129,6 +1130,27 @@ static int port_management_fill_response(struct port 
> > *target,
> >   memcpy(pwr, >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  and ."
> > +  */
> > + cmlds->egress_ts = tmv_to_nanoseconds(target->peer_delay_t1);
> > + cmlds->rx_ts = tmv_to_nanoseconds(target->peer_delay_t2);
>
> Please drop these two fields.  They don't provide any benefit.  If the
> clients don't trust the CMLDS values, then they are free to measure
> the p2p delay themselves!

The two timestamps are passed to clock_peer_delay() by the receiving
port and stored in c->tsproc.  Then they're accessed by
get_raw_delay() which is used in the filter logic.  I'm not sure how
much value that has, we can possibly pass 0s to clock_peer_delay().

Regarding "source_port_index", I assume that would contain the ifindex
of the interface?  With virtual clocks I believe the PHC indices may
differ between the PTP ports on one physical port ("link port").

Best regards


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

2023-11-15 Thread Richard Cochran
On Wed, Nov 15, 2023 at 09:45:52PM -0800, Richard Cochran wrote:
> On Mon, May 15, 2023 at 06:26:04PM -0400, Kishen Maloor wrote:
> 
> > @@ -473,6 +476,14 @@ struct msg_interface_rate_tlv {
> > UInteger16numberOfBitsAfterTimestamp;
> >  } PACKED;
> >  
> > +struct cmlds_info_np {
> > +   Integer8 serviceMeasurementValid;
> > +   TimeInterval meanLinkDelay;
> > +   Integer32scaledNeighborRateRatio;
> 
> I think you need one more field, like "source_port_index"
> 
> If a client subscribes to TLVs from multiple ports, then it needs a
> way to tell which is which.

Or maybe the subscription will cause the CMLDS server to forward all
p2p delay measurements from all p2p ports.

Still the TLVs needs to include the port index.

Thanks,
Richard


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

2023-11-15 Thread Richard Cochran
On Mon, May 15, 2023 at 06:26:04PM -0400, Kishen Maloor wrote:

> @@ -473,6 +476,14 @@ struct msg_interface_rate_tlv {
> UInteger16numberOfBitsAfterTimestamp;
>  } PACKED;
>  
> +struct cmlds_info_np {
> + Integer8 serviceMeasurementValid;
> + TimeInterval meanLinkDelay;
> + Integer32scaledNeighborRateRatio;

I think you need one more field, like "source_port_index"

If a client subscribes to TLVs from multiple ports, then it needs a
way to tell which is which.

> + Integer64egress_ts;
> + Integer64rx_ts;
> +} PACKED;

Thanks,
Richard


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

2023-11-15 Thread Richard Cochran
On Mon, May 15, 2023 at 06:26:04PM -0400, Kishen Maloor wrote:

> @@ -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(>meanLinkDelay);
> + NTOHL(cmlds->scaledNeighborRateRatio);
> + net2host64_unaligned(>egress_ts);
> + net2host64_unaligned(>rx_ts);
> + break;

Place after MID_POWER_PROFILE_SETTINGS_NP please.

>   }
>   if (extra_len) {
>   if (extra_len % 2)

Thanks,
Richard


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

2023-11-15 Thread Richard Cochran
On Mon, May 15, 2023 at 06:26:04PM -0400, Kishen Maloor wrote:

> @@ -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;

Nit: please place this after MID_POWER_PROFILE_SETTINGS_NP
rather than tacking on the end of the switch/case.

Thanks,
Richard


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

2023-11-15 Thread Richard Cochran
On Mon, May 15, 2023 at 06:26:04PM -0400, Kishen Maloor wrote:

> @@ -473,6 +476,14 @@ struct msg_interface_rate_tlv {
> UInteger16numberOfBitsAfterTimestamp;
>  } PACKED;
>  
> +struct cmlds_info_np {
> + Integer8 serviceMeasurementValid;
> + TimeInterval meanLinkDelay;
> + Integer32scaledNeighborRateRatio;
> + Integer64egress_ts;
> + Integer64rx_ts;
> +} PACKED;

This layout is very poor for packed formats -- think about alignment!

Please order the fields according to size, from largest to smallest.

Thanks,
Richard


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

2023-11-15 Thread Richard Cochran
On Mon, May 15, 2023 at 06:26:04PM -0400, Kishen Maloor wrote:

> @@ -1129,6 +1130,27 @@ static int port_management_fill_response(struct port 
> *target,
>   memcpy(pwr, >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  and ."
> +  */
> + cmlds->egress_ts = tmv_to_nanoseconds(target->peer_delay_t1);
> + cmlds->rx_ts = tmv_to_nanoseconds(target->peer_delay_t2);

Please drop these two fields.  They don't provide any benefit.  If the
clients don't trust the CMLDS values, then they are free to measure
the p2p delay themselves!

Thanks,
Richard


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

2023-05-18 Thread Erez
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  wrote:

> On 5/16/23 3:56 AM, Erez wrote:
> > On Tue, 16 May 2023 at 00:29, Kishen Maloor 
> 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 
> >> Signed-off-by: Kishen Maloor 
> >> ---
> >>  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 

Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

2023-05-16 Thread Kishen Maloor
On 5/16/23 3:56 AM, Erez wrote:
> On Tue, 16 May 2023 at 00:29, Kishen Maloor  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 
>> Signed-off-by: Kishen Maloor 
>> ---
>>  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, >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.
>> +*/
>> +   

Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

2023-05-16 Thread Erez
On Tue, 16 May 2023 at 00:29, Kishen Maloor  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 
> Signed-off-by: Kishen Maloor 
> ---
>  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, >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
> &&
> +   

[Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

2023-05-15 Thread Kishen Maloor
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 
Signed-off-by: Kishen Maloor 
---
 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, >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