Re: [Linuxptp-devel] [PATCH] phc2sys: Add external time stamping support
Hi Frantisek, Thank you for your work! I have to mention you on the mailing list. I found your paper "Hardware Assisted IEEE 1588 Clock Synchronization Under Linux", and there is similar work "Precision Time Protocol for Spectroscope Synchronization" from ZHAW university. I'm currently working on stratum-1 time server with GPSDO on board. The GPSDO driver exposes PHC interface, where timescale is TAI. We have two Synopsys NICs on board that have external timestamping support which are disciplined with GPSDO PHC clock. The results are accurate enough. I could share measurement results taken from Calnex Sentinel. I don't know how external timestamping support fits linuxptp project. It's very rare use-case, but I would like to have it in upstream. Thanks On Fri, 1 Feb 2019 at 23:27, Frantisek Rysanek wrote: > > Dear gentlemen, > > apologies if I'm completely off the mark in my response below, I'm > not following the linuxptp internal developments and I don't quite > understand the guts under the hood. Yet I feel like I could add some > points, even if they're possibly void... for a start, this debate > seems to be a little confused. > Three people with different angles... > > On 1 Feb 2019 at 17:42, Vladimir Oltean wrote: > > On 01.02.2019 16:25, Miroslav Lichvar wrote: > > > On Fri, Jan 18, 2019 at 03:50:51PM +0300, Artem Panfilov wrote: > > >> The auxiliary snapshot feature allows you to store a snapshot > > >> (timestamp) of the system time based on an external event. > > >> This feature is supported on Intel and Synopsys GMAC NICs. > > >> 1PPS signal could be connected as external timestamping source > > >> and used to estimate error. > > > > > I believe we should take this description at face value. > Seems to me that the "aux event input" of the Intel NIC is used as a > mere GPIO, used to trigger the taking of a timestamp, where the > reference is the system time for some reason, rather than the NIC's > very PHC... do I understand this correctly? > > In that case, it doesn't make sense to me, what use this > functionality is as part of the phc2sys program, as part of the > linuxptp project. Again I'm not conversant with the workings of this > project, so apologies for being so ignorant and opinionated - but the > motivation to take timestamps for your own use, triggered by a GPIO, > feels like a very generic application :-) and would probably deserve > a "translation unit" (program name) of its own. > > Timestamping an external event input. > Some Meinberg hardware has that function, for general purpose use > = "give me a precise timestamp when I tug at this TTL input". > Some people probably have a need for that feature, irrespective of > linuxptp. > > > > It's not clear to me what exactly is this supposed to do. It requires > > > the system clock to be specified as the source clock. Why? > > > > > > The only thing I managed to do with it is this: > > > # ./phc2sys -e 0 -c /dev/ptp1 -s CLOCK_REALTIME -O 0 -m > > > phc2sys[3070.094]: PPS is not in sync with PHC (0.940064462) > > > phc2sys[3071.094]: PPS is not in sync with PHC (0.940014584) > > > phc2sys[3072.094]: PPS is not in sync with PHC (0.940011248) > > > phc2sys[3073.094]: PPS is not in sync with PHC (0.939944354) > > > > > > The patch needs to update the documention and explain what is > > > synchronized to what in this mode. > > > > > > What I'd like to see implemented, and I'm not sure if that's what this > > > patch is trying to do, is synchronization of a PHC to its own PPS > > > input. When the PHC is synchronized to an external PPS signal, the > > > system clock could be synchronized to the PHC. > > > > That sounds pretty simple to achieve, right? On top of what phc2sys > already does... > > I believe I do a bit of that in my proggie. But I have an irrelevant > motivation: packet capture with precise timestamping, rather than > PTP. > So my proggie initially sets the PHC's Time of Day to system time > (UTC) rather than TAI, and I'm not trying to keep an eye on leap > seconds, and not trying to enslave the system time to the > PPS-disciplined PHC - because the "attention span" of my capture > sessions is a couple seconds to a couple days maybe. > There's even a comment in my code, that remarks "you'd better > discipline your system time using NTP or PTP or some such" - like I'm > leaving that up to the sysadmin (=that is typically me on my machines > :-) I really was interested in getting the precise ns-level PHC time > in my captured packets (think t-shark). > > If memory serves, Richard Cochran's code snippet was aimed to build a > primitive boundary clock. Its name is synbc.c . His code would use > one PHC (PTP slave) as a PPS source and, using some discrete wiring, > used its PPS output pin to drive some downstream PHC's (again via > GPIO input) in the role of PTP masters. > (So the first thing I did, I gutted that PTP slave PHC from the code > :-) and went on from there.) > > > Hi Miroslav, > > > > I've been meaning to ask some
Re: [Linuxptp-devel] [PATCH] phc2sys: Add external time stamping support
Dear gentlemen, apologies if I'm completely off the mark in my response below, I'm not following the linuxptp internal developments and I don't quite understand the guts under the hood. Yet I feel like I could add some points, even if they're possibly void... for a start, this debate seems to be a little confused. Three people with different angles... On 1 Feb 2019 at 17:42, Vladimir Oltean wrote: > On 01.02.2019 16:25, Miroslav Lichvar wrote: > > On Fri, Jan 18, 2019 at 03:50:51PM +0300, Artem Panfilov wrote: > >> The auxiliary snapshot feature allows you to store a snapshot > >> (timestamp) of the system time based on an external event. > >> This feature is supported on Intel and Synopsys GMAC NICs. > >> 1PPS signal could be connected as external timestamping source > >> and used to estimate error. > > I believe we should take this description at face value. Seems to me that the "aux event input" of the Intel NIC is used as a mere GPIO, used to trigger the taking of a timestamp, where the reference is the system time for some reason, rather than the NIC's very PHC... do I understand this correctly? In that case, it doesn't make sense to me, what use this functionality is as part of the phc2sys program, as part of the linuxptp project. Again I'm not conversant with the workings of this project, so apologies for being so ignorant and opinionated - but the motivation to take timestamps for your own use, triggered by a GPIO, feels like a very generic application :-) and would probably deserve a "translation unit" (program name) of its own. Timestamping an external event input. Some Meinberg hardware has that function, for general purpose use = "give me a precise timestamp when I tug at this TTL input". Some people probably have a need for that feature, irrespective of linuxptp. > > It's not clear to me what exactly is this supposed to do. It requires > > the system clock to be specified as the source clock. Why? > > > > The only thing I managed to do with it is this: > > # ./phc2sys -e 0 -c /dev/ptp1 -s CLOCK_REALTIME -O 0 -m > > phc2sys[3070.094]: PPS is not in sync with PHC (0.940064462) > > phc2sys[3071.094]: PPS is not in sync with PHC (0.940014584) > > phc2sys[3072.094]: PPS is not in sync with PHC (0.940011248) > > phc2sys[3073.094]: PPS is not in sync with PHC (0.939944354) > > > > The patch needs to update the documention and explain what is > > synchronized to what in this mode. > > > > What I'd like to see implemented, and I'm not sure if that's what this > > patch is trying to do, is synchronization of a PHC to its own PPS > > input. When the PHC is synchronized to an external PPS signal, the > > system clock could be synchronized to the PHC. > > That sounds pretty simple to achieve, right? On top of what phc2sys already does... I believe I do a bit of that in my proggie. But I have an irrelevant motivation: packet capture with precise timestamping, rather than PTP. So my proggie initially sets the PHC's Time of Day to system time (UTC) rather than TAI, and I'm not trying to keep an eye on leap seconds, and not trying to enslave the system time to the PPS-disciplined PHC - because the "attention span" of my capture sessions is a couple seconds to a couple days maybe. There's even a comment in my code, that remarks "you'd better discipline your system time using NTP or PTP or some such" - like I'm leaving that up to the sysadmin (=that is typically me on my machines :-) I really was interested in getting the precise ns-level PHC time in my captured packets (think t-shark). If memory serves, Richard Cochran's code snippet was aimed to build a primitive boundary clock. Its name is synbc.c . His code would use one PHC (PTP slave) as a PPS source and, using some discrete wiring, used its PPS output pin to drive some downstream PHC's (again via GPIO input) in the role of PTP masters. (So the first thing I did, I gutted that PTP slave PHC from the code :-) and went on from there.) > Hi Miroslav, > > I've been meaning to ask some clarification on this patch as well, and > perhaps even ask for some suggestions. > I believe the typical use case is to use a GNSS module as a clock > provider. NMEA data would provide absolute time of day while the servo > is unlocked, and PPS signal would provide phase alignment. > I do not think this is the motivation of the patch that started this mailinglist thread. But I'd also like to comment on your interesting point Vladimir: > The issue seems to be that there is no GNSS driver in Linux that exposes > the time of day as a PHC. Aditionally, the PPS signal from the GNSS > module usually needs to be processed by a PHC driver belonging to an > Ethernet controller (and served to userspace as extts events). > This sure is an interesting angle :-) You're right - the NIC's contain a PHC = effectively a timebase with some synthetic frequency adjustment capabilities, that can provide precise nanosecond-level timestamps be
Re: [Linuxptp-devel] [PATCH] phc2sys: Add external time stamping support
On 01.02.2019 16:25, Miroslav Lichvar wrote: > On Fri, Jan 18, 2019 at 03:50:51PM +0300, Artem Panfilov wrote: >> The auxiliary snapshot feature allows you to store a snapshot >> (timestamp) of the system time based on an external event. >> This feature is supported on Intel and Synopsys GMAC NICs. >> 1PPS signal could be connected as external timestamping source >> and used to estimate error. > > It's not clear to me what exactly is this supposed to do. It requires > the system clock to be specified as the source clock. Why? > > The only thing I managed to do with it is this: > # ./phc2sys -e 0 -c /dev/ptp1 -s CLOCK_REALTIME -O 0 -m > phc2sys[3070.094]: PPS is not in sync with PHC (0.940064462) > phc2sys[3071.094]: PPS is not in sync with PHC (0.940014584) > phc2sys[3072.094]: PPS is not in sync with PHC (0.940011248) > phc2sys[3073.094]: PPS is not in sync with PHC (0.939944354) > > The patch needs to update the documention and explain what is > synchronized to what in this mode. > > What I'd like to see implemented, and I'm not sure if that's what this > patch is trying to do, is synchronization of a PHC to its own PPS > input. When the PHC is synchronized to an external PPS signal, the > system clock could be synchronized to the PHC. > Hi Miroslav, I've been meaning to ask some clarification on this patch as well, and perhaps even ask for some suggestions. I believe the typical use case is to use a GNSS module as a clock provider. NMEA data would provide absolute time of day while the servo is unlocked, and PPS signal would provide phase alignment. The issue seems to be that there is no GNSS driver in Linux that exposes the time of day as a PHC. Aditionally, the PPS signal from the GNSS module usually needs to be processed by a PHC driver belonging to an Ethernet controller (and served to userspace as extts events). Frantisek Rysanek added the same functionality a while ago in a program called i210_ext_pps (https://sourceforge.net/p/linuxptp/mailman/message/36228042/) Unfortunately it suffered of the same limitation as the currently proposed patch does: it had no standard interface over which it could collect the GNSS module's time of day. -Vladimir ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?
On Fri, Feb 01, 2019 at 08:16:41AM -0800, Richard Cochran wrote:
> On Fri, Feb 01, 2019 at 10:16:53AM +, Vincent Li X wrote:
> >
> > Hi Richard,
> > I mean the change of that two lines to like:
> > if (cnt < m->header.messageLength || m->header.messageLength <
> > pdulen) {
> > pr_debug("wrong length, cnt: %d messageLength: %hu pdulen
>
> The rule in communication is to be strict when sending, but generous
> when receiving. This code generously allows incorrect messageLength.
By "this" I mean the current linuxptp code, not the your example!
> That field is redundant, because all of the transports already provide
> the packet length. Thus there is no need to check it.
>
> If we were using a stream transport like TCP, then checking
> messageLength would of course be essential.
>
> Thanks,
> Richard
>
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?
On Fri, Feb 01, 2019 at 10:16:53AM +, Vincent Li X wrote:
>
> Hi Richard,
> I mean the change of that two lines to like:
> if (cnt < m->header.messageLength || m->header.messageLength <
> pdulen) {
> pr_debug("wrong length, cnt: %d messageLength: %hu pdulen
The rule in communication is to be strict when sending, but generous
when receiving. This code generously allows incorrect messageLength.
That field is redundant, because all of the transports already provide
the packet length. Thus there is no need to check it.
If we were using a stream transport like TCP, then checking
messageLength would of course be essential.
Thanks,
Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] phc2sys: Add external time stamping support
On Fri, Jan 18, 2019 at 03:50:51PM +0300, Artem Panfilov wrote: > The auxiliary snapshot feature allows you to store a snapshot > (timestamp) of the system time based on an external event. > This feature is supported on Intel and Synopsys GMAC NICs. > 1PPS signal could be connected as external timestamping source > and used to estimate error. It's not clear to me what exactly is this supposed to do. It requires the system clock to be specified as the source clock. Why? The only thing I managed to do with it is this: # ./phc2sys -e 0 -c /dev/ptp1 -s CLOCK_REALTIME -O 0 -m phc2sys[3070.094]: PPS is not in sync with PHC (0.940064462) phc2sys[3071.094]: PPS is not in sync with PHC (0.940014584) phc2sys[3072.094]: PPS is not in sync with PHC (0.940011248) phc2sys[3073.094]: PPS is not in sync with PHC (0.939944354) The patch needs to update the documention and explain what is synchronized to what in this mode. What I'd like to see implemented, and I'm not sure if that's what this patch is trying to do, is synchronization of a PHC to its own PPS input. When the PHC is synchronized to an external PPS signal, the system clock could be synchronized to the PHC. -- Miroslav Lichvar ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?
Sorry Miroslav again for missing your msg again in Junk box! How about the combination with the length-check mentioned in my last msg? -Original Message- From: Miroslav Lichvar Sent: Friday, February 1, 2019 9:19 AM To: Vincent Li X Cc: Jiri Benc ; Richard Cochran ; Mats Bergman H ; Richard Jönsson ; Linuxptp-devel@lists.sourceforge.net Subject: Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV? On Thu, Jan 31, 2019 at 04:28:30PM +, Vincent Li X wrote: > But we still think it's more safe to use header.messageLength instead > of socket count, Msg.c > err = suffix_post_recv(m, cnt - pdulen); ==> > err = suffix_post_recv(m, m->header.messageLength - pdulen); I'm not sure that is more safe. If the field had a large value, it might enable reading of uninitialized data, possibly even past the buffer. A better way is to check the length in each transport specific code and either remove the padding or drop the packet if the transport doesn't allow padding. -- Miroslav Lichvar smime.p7s Description: S/MIME cryptographic signature ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?
Hi Richard,
I mean the change of that two lines to like:
if (cnt < m->header.messageLength || m->header.messageLength <
pdulen) {
pr_debug("wrong length, cnt: %d messageLength: %hu pdulen
:%d",
cnt, m->header.messageLength, pdulen);
return -EBADMSG;
}
Thanks,
vincent
-Original Message-
From: Richard Cochran
Sent: Friday, February 1, 2019 4:45 AM
To: Vincent Li X
Cc: Jiri Benc ; Miroslav Lichvar ;
Mats Bergman H ; Richard Jönsson
; Linuxptp-devel@lists.sourceforge.net
Subject: Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?
On Thu, Jan 31, 2019 at 04:28:30PM +, Vincent Li X wrote:
> we might also need to check again m->header.messageLength is bigger
> than cnt.
What? We already have
if (cnt < pdulen)
return -EBADMSG;
in msg_post_recv();
Or did you mean something else?
Thanks,
Richard
smime.p7s
Description: S/MIME cryptographic signature
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?
Hello, Since I see the subject is ethernet padding I would like to mention what we use together with PTP: A timestamping card which adds 9 bytes of VSS-monitoring trailer to the ethernet packet. Depending on the exact value of the timestamp, wireshark can misdetect it and show a "wrong" FCS plus 5 bytes padding, see https://github.com/boundary/wireshark/blob/master/epan/dissectors/packet-vssmonitoring.c#L139 I'm attaching a small capture where you can see traffic from 2 NICs, one which adds a trailer to all incoming packets and one which does not. I am anonymizing the MAC vendor for the timestamping card. On 31/01/2019 19:24, Jiri Benc wrote: > On Thu, 31 Jan 2019 16:28:30 +, Vincent Li X wrote: >> we might also need to check again m->header.messageLength is bigger than >> cnt. > > This might not be a bad idea; if the packet length is inconsistent with > the PTP or 802.3 standard, a warning can be emitted and the packet > dropped and not processed further. This would make linuxptp more robust > against randomly malformed packets and would give some indication to > users that the other end or intermediate network equipment is broken. > > Jiri ptp2.pcap Description: application/vnd.tcpdump.pcap ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?
On Thu, Jan 31, 2019 at 04:28:30PM +, Vincent Li X wrote: > But we still think it's more safe to use header.messageLength instead of > socket count, > Msg.c > err = suffix_post_recv(m, cnt - pdulen); > ==> > err = suffix_post_recv(m, m->header.messageLength - pdulen); I'm not sure that is more safe. If the field had a large value, it might enable reading of uninitialized data, possibly even past the buffer. A better way is to check the length in each transport specific code and either remove the padding or drop the packet if the transport doesn't allow padding. -- Miroslav Lichvar ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
