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

[Andrew Z. <and...@in...>]

On Thu, 16 Nov 2023 at 05:27, Richard Cochran <ric...@gm...> 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, &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);
>
> 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