Re: [Linuxptp-devel] [PATCH] [pmc] Avoid conflicting port IDs over PMC UDS
> -Original Message-
> From: Eyal Itkin via Linuxptp-devel
> Sent: Sunday, September 10, 2023 9:23 AM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH] [pmc] Avoid conflicting port IDs over PMC
> UDS
>
> The UDS interface is currently associated with a clock ID of zeros
> and a 16-bit port number which is the process id. However, the
> process id on Linux can easily by bigger than 16 bits (supposed
> to be limited to 22 bits). This means that several linuxptp processes
> (pmc and phc2sys for instance) can collide and use the same port id.
>
> Hence, use the lower 2 bytes of the process id for the port number,
> and the top 2 bytes as bytes 7 and 8 of the Clock ID.
>
> Signed-off-by: Eyal Itkin
> ---
Looks like this got sent twice, but it seems reasonable to me.
Reviewed-by: Jacob Keller
> pmc_common.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/pmc_common.c b/pmc_common.c
> index 9e251c4..694edf6 100644
> --- a/pmc_common.c
> +++ b/pmc_common.c
> @@ -486,13 +486,17 @@ struct pmc *pmc_create(struct config *cfg, enum
> transport_type transport_type,
> int zero_datalen)
> {
> struct pmc *pmc;
> + UInteger32 proc_id;
>
> pmc = calloc(1, sizeof *pmc);
> if (!pmc)
> return NULL;
>
> if (transport_type == TRANS_UDS) {
> - pmc->port_identity.portNumber = getpid();
> + proc_id = getpid();
> + pmc->port_identity.clockIdentity.id[6] = (proc_id & 0xFF00)
> >> 24;
> + pmc->port_identity.clockIdentity.id[7] = (proc_id & 0x00FF)
> >> 16;
> + pmc->port_identity.portNumber = proc_id & 0x;
> } else {
> if (generate_clock_identity(>port_identity.clockIdentity,
> iface_name)) {
> --
> 2.21.0
>
>
>
> ___
> 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
Re: [Linuxptp-devel] [PATCH] phc2sys: Avoid segfault with default UDS address.
> -Original Message-
> From: Richard Cochran
> Sent: Monday, September 4, 2023 11:21 AM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH] phc2sys: Avoid segfault with default UDS
> address.
>
> The recently added phc2sys multi-domain mode introduced a regression.
> In the non-automatic mode (using flag -w to wait for ptp4l) the code
> will attempt to set the UDS address using an uninitialized pointer,
> unless the -z flag is also specified.
>
> Fix the issue by testing whether the -z flag was present on the
> command line before changing the UDS address.
>
> Signed-off-by: Richard Cochran
> Reported-by: Trey Harrison
> ---
Reviewed-by: Jacob Keller
> phc2sys.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/phc2sys.c b/phc2sys.c
> index 313cdcf..9d8d42f 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -1490,7 +1490,10 @@ int main(int argc, char *argv[])
> if (wait_sync) {
> snprintf(uds_local, sizeof(uds_local),
>"/var/run/phc2sys.%d", getpid());
> - config_set_string(cfg, "uds_address", uds_remotes[0]);
> +
> + if (uds_remote_cnt > 0)
> + config_set_string(cfg, "uds_address",
> + uds_remotes[uds_remote_cnt - 1]);
>
> if (init_pmc_node(cfg, domains[0].agent, uds_local,
> phc2sys_recv_subscribed, [0]))
> --
> 2.39.2
>
>
>
> ___
> 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
Re: [Linuxptp-devel] [PATCH] phc2sys: Fix -n option with -w.
> -Original Message-
> From: Miroslav Lichvar
> Sent: Monday, September 4, 2023 11:04 PM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH] phc2sys: Fix -n option with -w.
>
> The domain number used for communication with ptp4l specified by the -n
> option is ignored in the non-automatic mode (-w option). Set the domain
> number to the last specified value.
>
> Fixes: 417de97d098b ("phc2sys: Add multi-domain synchronization.")
> Signed-off-by: Miroslav Lichvar
> ---
Reviewed-by: Jacob Keller
> phc2sys.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/phc2sys.c b/phc2sys.c
> index 9d8d42f..7ea6929 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -1494,6 +1494,9 @@ int main(int argc, char *argv[])
> if (uds_remote_cnt > 0)
> config_set_string(cfg, "uds_address",
> uds_remotes[uds_remote_cnt - 1]);
> + if (domain_number_cnt > 0)
> + config_set_int(cfg, "domainNumber",
> +domain_numbers[domain_number_cnt - 1]);
>
> if (init_pmc_node(cfg, domains[0].agent, uds_local,
> phc2sys_recv_subscribed, [0]))
> --
> 2.41.0
>
>
>
> ___
> 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
Re: [Linuxptp-devel] [PATCH 2/2] ptp4l: Add description for setting kthreads priorities
> -Original Message-
> From: Plachno, Lukasz
> Sent: Wednesday, July 19, 2023 9:48 AM
> To: linuxptp-devel@lists.sourceforge.net
> Cc: Kitszel, Przemyslaw ;
> richardcoch...@gmail.com; mac...@machnikowski.net; Keller, Jacob E
> ; Plachno, Lukasz
> Subject: [PATCH 2/2] ptp4l: Add description for setting kthreads priorities
>
> As tx timestamp timeout is something multiple users encounter,
> provide information for configuring kthread priorities in manual
> for ptp4l where tx_timestamp_timeout is already described.
>
> Signed-off-by: Lukasz Plachno
> ---
> ptp4l.8 | 20
> 1 file changed, 20 insertions(+)
>
> diff --git a/ptp4l.8 b/ptp4l.8
> index 09ff108af102..02143eec36a2 100644
> --- a/ptp4l.8
> +++ b/ptp4l.8
> @@ -1032,6 +1032,26 @@ to maintain the correct offset between UTC and PTP
> times. See
> .BR phc2sys (8)
> manual page for more details.
>
> +.SH KTHREAD PRIORITY
> +
> +In case of following log:
> +.br
> +.I timed out while polling for tx timestamp
> +.I increasing tx_timestamp_timeout or increasing
> +.I kworker priority may correct this issue,
> +.I but a driver bug likely causes it
> +.br
> +one of possible causes is kworker processing timestamps being starved,
> +user might try manually increasing the priority of the kworker.
> +
> +Example for Intel E810 card:
> +.br
> +.I pgrep \-fl ice-ptp | cut \-f1 \-d' ' | xargs \-I {} sudo chrt \-r \-\-pid
> 30 {}
> +
> +Each NIC driver assigns a different name for kworkers, in some cases
> +processing timestamps might not be using kworker at all. Also assigning
> +too high priority might lead to system becoming unstable.
> +
Strictly, drivers which make use of the kthread created by PTP stack would be
uniform. The only reason ice doesn't do such is because of needing to operate
on both ports which have a PTP clock and on ports which do not (due to its
cross PF interaction). I think it would be good to mention that thread name as
first, and have the example for ice separate since its more unique. I suppose
this could be refactored to expose the kworker now that I think of it... Hmm.
These threads are named "ptp" matching the PTP clock name, and since these
are part of the standard PTP_1588_CLOCK interface, we should mention them first.
Thanks,
Jake
> .SH SEE ALSO
> .BR pmc (8),
> .BR phc2sys (8)
> --
> 2.34.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] sk: don't report random errno on timeout
> -Original Message- > From: Maciek Machnikowski > Sent: Tuesday, July 18, 2023 12:33 AM > To: Keller, Jacob E ; Richard Cochran > > Cc: linuxptp-devel@lists.sourceforge.net; Czapnik, Lukasz > ; Kolacinski, Karol ; > Plachno, Lukasz ; Pacuszka, MateuszX > ; Glaza, Jan > Subject: Re: [Linuxptp-devel] [PATCH] sk: don't report random errno on timeout > > > > On 7/18/2023 1:08 AM, Jacob Keller wrote: > > > > > > On 7/16/2023 1:27 PM, Richard Cochran wrote: > >> On Fri, Jul 14, 2023 at 08:43:30PM +, Keller, Jacob E wrote: > >> > >>>> With this patch applied, one will get proper error in last line, > >>>> "Timer expired", and more modern suggestion about how to approach > fixing it > >>>> > >>> > >>> > >>> I think changing the message about what might be causing timeout is > >>> unnecessary. It may be helpful purely in the context of some > >>> devices, but it is not a good general message as not all hardware > >>> and drivers have the same design. In the *general* case if this > >>> timeout is hit then it is usually a bug in the driver for that > >>> hardware. In the specific case for ice hardware, the mention of > >>> thread starvation is accurate, but that is unlikely to be general > >>> across all hardware. > >> > >> But the point about kthread priority is a good hint. How about > >> keeping the part about possible driver bug (since we have had many, > >> Many, MANY questions on this list when somebody is developing a new > >> driver) and adding a hint about kworker scheduling priority? > >> > > > > Sounds good to me. It might help us reduce our own support burden when > > users say "I get this message that says its a driver bug, and I already > > tried , but still see timeouts", and then > > we need to educate them on priority to ensure that they aren't starving > > the thread which processes timestamps. > > > > But I agree keeping the message about it possibly being a bug is > > important because it has been caused by drivers a lot in the past. > > > > Maybe something like: > > > > timed out while polling for tx timestamp > > increasing tx_timestamp_timeout or increasing priority of relevant > > kworker threads may correct this issue, but it is likely caused by a > > driver bug > > > > Thanks, > > Jake > > What about: > increasing tx_timestamp_timeout or increasing kworker threads priority > may correct this issue, but a driver bug likely causes it > > That would work if we also add the instruction how to do that in the > manual. Otherwise we'll get the same number of questions - just > different ones :) > > Thanks, > Maciek > Ya a section on the Tx timestamp timeout in the man page with additional details would be useful. Thanks, Jake > > > >>> Thus, I think we should leave the error message alone and just fix > >>> the errno value. Improving the errno value is important since it > >>> would be less confusing than seeing arbitrary error values which are > >>> unrelated to the actual error. > >> > >> Yeah, errno fix is needed> > >> Thanks, > >> Richard > > > > > > ___ > > 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
Re: [Linuxptp-devel] [PATCH] sk: don't report random errno on timeout
> -Original Message-
> From: Kitszel, Przemyslaw
> Sent: Friday, July 14, 2023 4:39 AM
> To: linuxptp-devel@lists.sourceforge.net
> Cc: Richard Cochran ; Miroslav Lichvar
> ; Keller, Jacob E ; Kolacinski,
> Karol ; Glaza, Jan ; Plachno,
> Lukasz ; Pacuszka, MateuszX
> ; Czapnik, Lukasz ;
> Kitszel, Przemyslaw
> Subject: [PATCH] sk: don't report random errno on timeout
>
> Fix errno value reported up from sk_receive() on poll() timeout.
>
> With neither caller nor poll() itself zeroing errno value, it will contain
> result of previous failure, possibly from long time ago.
>
> Reporting errno=0 up from sk_receive() would bring confusion,
> as "%m" is later used in pr_err() (so one would get "error Success").
> Use ETIME as it fits here the best.
> (ETIMEDOUT instead of ETIME would look better in the code,
> but message printed would be worse).
>
> Prior to this patch, following log could be produced:
> | timed out while polling for tx timestamp
> | increasing tx_timestamp_timeout may correct this issue, but it is likely
> caused
> by a driver bug
> | PTP send sync failed : error No such device or address
>
> With this patch applied, one will get proper error in last line,
> "Timer expired", and more modern suggestion about how to approach fixing it
>
I think changing the message about what might be causing timeout is
unnecessary. It may be helpful purely in the context of some devices, but it is
not a good general message as not all hardware and drivers have the same
design. In the *general* case if this timeout is hit then it is usually a bug
in the driver for that hardware. In the specific case for ice hardware, the
mention of thread starvation is accurate, but that is unlikely to be general
across all hardware.
Thus, I think we should leave the error message alone and just fix the errno
value. Improving the errno value is important since it would be less confusing
than seeing arbitrary error values which are unrelated to the actual error.
Thanks,
Jake
>
> Signed-off-by: Przemek Kitszel
> ---
> sk.c | 13 -
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/sk.c b/sk.c
> index a72aca3b7821..d95002b24cc6 100644
> --- a/sk.c
> +++ b/sk.c
> @@ -441,12 +441,15 @@ int sk_receive(int fd, void *buf, int buflen,
> /* Retry once on EINTR to avoid logging errors before exit */
> if (res < 0 && errno == EINTR)
> res = poll(, 1, sk_tx_timeout);
> - if (res < 1) {
> - pr_err(res ? "poll for tx timestamp failed: %m" :
> - "timed out while polling for tx
> timestamp");
> - pr_err("increasing tx_timestamp_timeout may correct "
> -"this issue, but it is likely caused by a driver
> bug");
> + if (res < 0) {
> + pr_err("poll for tx timestamp failed: %m");
> return -errno;
> + } else if (!res) {
> + pr_err("timed out while polling for tx timestamp");
> + pr_err("increasing tx_timestamp_timeout may correct
> this issue,"
> +"but it is likely caused by starving some of
> PTP/driver
> threads");
> + errno = ETIME;
> + return -1;
> } else if (!(pfd.revents & sk_revents)) {
> pr_err("poll for tx timestamp woke up on non ERR
> event");
> return -1;
>
> base-commit: aa60db270be12e7144e7b0cef2f5dde4213b7057
> --
> 2.38.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] Side note: i225 / igc time sync and TSN capabilities...
> -Original Message- > From: Frantisek Rysanek > Sent: Friday, May 26, 2023 9:01 AM > To: linuxptp-devel@lists.sourceforge.net > Subject: [Linuxptp-devel] Side note: i225 / igc time sync and TSN > capabilities... > > Dear gents, > > this is just a slightly off topic gratuitous side note. > > In the context of fumbling in the "igb" driver in the other thread, I > also took a look inside driver "igc" for comparison = to see what the > i225 had to offer. > Unsurprisingly, the register map related to time sync features seems > quite similar to that of the i210. The source code of igc_ptp.c is > pretty clean. The datasheet for the i225 is not public yet and I > don't have it, so I cannot comment further. > > Interestingly to me, the TIMADJ register is now mentioned, but only > to set some novel flag/bit in that register during port init/reset... > For stepwise time adjustment, the SYSTIM registers get written > directly :-) > Hi, I didn't work directly on the i350 code, but I recall historically that the TIMADJ registers were poorly designed and didn't map very well onto the actual adjtime callback. Assuming the i350 is anything like i210, here's the description of TIMADJ from i210's datasheet: Dynamic update of SYSTIM registers can be done by using the TIMADJ registers by the following flow. It can also be done by adjusting the INC_TIME as described later in this section. Adjusting the time by TIMADJ are meant to be used only when the time difference between the master and the slave are small enough (at least smaller than one 8th of the time between consecutive SYNC cycles). If this assumption is incorrect, than this process might take longer time than the SYNC cycle to take effect. If this is an issue, software might need to set the SYSTIM by direct access as described in the “Initial Setting” phase: — Write the Tadjust value and its Sign to the TIMADJ register (the Sign bit indicates if the Tadjust value should be added or subtracted) — Following the write access to the TIMADJ register, the hardware repeats the following two steps at each 8 nsec clock as long as the Tadjust > zero. • SYSTIM = SYSTIM + INC_TIME +/- 1 nsec. Add or subtract 1 nsec is defined by TIMADJ.Sign (while 0bmeans Add and 1b means Subtract) • Tadjust = Tadjust - 1 nsec • Note that the SYSTIM timer is incremented monotonically at all times. When updating the SYSTIM by the TIMADJ and concurrent non-zero TIMINCA, the SYSTIM is incremented each clock by steps in the range of 6.5ns up to 9.5ns units. — As shown above, the time adjustment might take multiple clocks. Software might write a new value to the TIMADJ register before the hardware completed the previous adjustment. In such a case, the new value written by software, overrides the above equation. If such a race is not desired, the software could check that the previous adjustment is completed by one of the following methods: • Wait enough time before accessing the TIMADJ register which guarantees that the previous update procedure is completed. • Poll the matched TSICR.TADJ flag which is set by the hardware each time the update procedure is completed. • Enable the TADJ interrupt by setting the TADJ flag in the TSIM register and enable timesync interrupts by setting the Time_Sync flag in the IMS register. The TADJ interrupt indicates that the hardware completed the adjustment procedure. This method is unlikely to be used in nominal operation since the expected adjustments are in the sub s range. TL;DR; the TIMADJ basically just amounts to a +1 or -1 to the increment value beyond the range of the INC_TIME register. It doesn't actually do an immediate atomic adjustment. The delay makes it not as useful, and the fact that you can't enable it "permanently" makes it not useful for performing frequency adjustment as we'd have to keep re-applying the value in order to keep the +1 for longer periods. We could apply the adjustment and then poll until it completes which might take time and delay the adjtime operation, or we could set a timer and block new adjtime operations until the thing completes... Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] clock: Fix summary interval in free-running mode.
> -Original Message- > From: Miroslav Lichvar > Sent: Thursday, May 18, 2023 7:27 AM > To: linuxptp-devel@lists.sourceforge.net > Subject: [Linuxptp-devel] [PATCH] clock: Fix summary interval in free-running > mode. > > In the free-running mode the stats are updated only once per the > frequency estimation interval instead of once per sync message. The > stats max_count calculation didn't account for that, which caused a > reduced rate of printed summaries. Fix the calculation for this case. > > Signed-off-by: Miroslav Lichvar > --- Huh. I always wondered why this happened.. Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] sk: Reset timestamping mode on exit, use locks
> -Original Message- > From: Miroslav Lichvar > Sent: Tuesday, May 9, 2023 5:31 AM > To: Zaborowski, Andrew > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH] sk: Reset timestamping mode on exit, use > locks > > On Tue, May 09, 2023 at 02:02:05PM +0200, Andrew Zaborowski wrote: > > Per https://www.kernel.org/doc/Documentation/networking/timestamping.txt > > section 3: > > "User space is responsible to ensure that multiple processes don't interfere > > with each other and that the settings are reset." > > > > Add locking for the interface's HW timestamping mode to ensure that in a > > setup with multiple ptp4l sessions sharing interfaces the sessions don't > > overwrite each other's timestamping mode and that there is one session > > responsible for resetting the mode on exit. > > Is this intended to catch misconfigurations where different ptp4l > instances are setting incompatible RX filters? > > If I understand the solution correctly, stopping ptp4l would break > another ptp4l instance if they don't share the same filesystem (e.g. > containers), or a different program is using HW timestamping (e.g. > chronyd). > Yea this only prevents ptp4l instances on the same file system from interfering. I think we'd have to extend the "locking" to kernel if we wanted to protect anything more complex, but I'm not sure how useful that would be in practice. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 1/2] phc_ctl: explicitly check for adjust_phase definition
> -Original Message-
> From: Geva, Erez
> Sent: Thursday, November 17, 2022 9:21 AM
> To: Keller, Jacob E ; linuxptp-
> de...@lists.sourceforge.net
> Subject: Re: [Linuxptp-devel] [PATCH 1/2] phc_ctl: explicitly check for
> adjust_phase definition
>
> On Tue, 2022-11-15 at 16:43 -0800, Jacob Keller wrote:
> > The PTP_CLOCK_GETCAPS ioctl call in phc_ctl has a conditional check
> > for
> > whether to report the caps.adjust_phase bit. This is always set for
> > both
> > PTP_CLOCK_GETCAPS and PTP_CLOCK_GETCAPS2, and was always zero before
> > the
> > bit was moved from being reserved to indicate the adjust_phase.
> >
> > In some sense if we lack PTP_CLOCK_GETCAPS2 we know the kernel won't
> > report
> > adjust_phase, but in reality such kernel versions did not have
> > adjust_phase at all.
> >
> > There's no reason to make this check against PTP_CLOCK_GETCAPS2.
> > Instead,
> > scan the ptp_clock.h header file and check if we have adjust_phase
> > support.
> > Use this new flag instead of PTP_CLOCK_GETCAPS2.
> >
> > Signed-off-by: Jacob Keller
> > ---
> > Checking against PTP_CLOCK_GETCAPS2 is wrong because this is not when
> > the
> > adjust_phase was actually introduced. A direct check against whats in
> > the
> > header is more accurate.
>
>
> This make sense.
> Thanks for the correction :-)
>
> >
> > incdefs.sh | 5 +
> > phc_ctl.c | 2 +-
> > 2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/incdefs.sh b/incdefs.sh
> > index 21333e5109e9..a9e94f777f6b 100755
> > --- a/incdefs.sh
> > +++ b/incdefs.sh
> > @@ -63,6 +63,7 @@ kernel_flags()
> > {
> > prefix=""
> > tstamp=/usr/include/linux/net_tstamp.h
> > + ptp_clock=/usr/include/linux/ptp_clock.h
>
> Is that catch if using kernel headers?
> The location in the current kernel is
> /include/uapi/linux/ptp_clock.h
>
This is available at both /usr/include/linux/ptp_clock.h and
/include/uapi/linux/ptp_clock.h once you've finished a compilation.
> I see you add in the same way, perhaps we need to update the script?
>
>
The script already checks for existence of kernel location in common places
starting with KBUILD directory if doing a direct/targetted build, then using
/lib/modules/$(uname -r) and finally using the headers installed at /
Thanks,
Jake
> >
> > if [ "x$KBUILD_OUTPUT" != "x" ]; then
> > # With KBUILD_OUTPUT set, we are building against
> > @@ -90,6 +91,10 @@ kernel_flags()
> > if grep -q SOF_TIMESTAMPING_BIND_PHC ${prefix}${tstamp}; then
> > printf " -DHAVE_VCLOCKS"
> > fi
> > +
> > + if grep -q adjust_phase ${prefix}${ptp_clock}; then
> > + printf " -DHAVE_PTP_CAPS_ADJUST_PHASE"
> > + fi
> > }
> >
> > flags="$(user_flags)$(kernel_flags)"
> > diff --git a/phc_ctl.c b/phc_ctl.c
> > index 92e597c40a23..6a5c2f43d7c9 100644
> > --- a/phc_ctl.c
> > +++ b/phc_ctl.c
> > @@ -311,7 +311,7 @@ static int do_caps(clockid_t clkid, int cmdc,
> > char *cmdv[])
> > caps.n_pins,
> > caps.pps ? "has" : "doesn't have",
> > caps.cross_timestamping ? "has" : "doesn't have",
> > - #ifdef PTP_CLOCK_GETCAPS2
> > + #ifdef HAVE_PTP_CAPS_ADJUST_PHASE
> > caps.adjust_phase ? "has" : "doesn't have"
> > #else
> > "no information regarding"
>
>
> Erez
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/2] phc_ctl: use PTP_CLOCK_GETCAPS2 ioctl if available
> -Original Message-
> From: Geva, Erez
> Sent: Thursday, November 17, 2022 9:13 AM
> To: Keller, Jacob E ; linuxptp-
> de...@lists.sourceforge.net
> Subject: Re: [Linuxptp-devel] [PATCH 2/2] phc_ctl: use PTP_CLOCK_GETCAPS2
> ioctl if available
>
> On Tue, 2022-11-15 at 16:43 -0800, Jacob Keller wrote:
> > The PTP_CLOCK_GETCAPS2 ioctl is provided by kernel as a replacement
> > for the
> > original PTP_CLOCK_GETCAPS ioctl. This was done in order to provide
> > ioctls
> > which guarantee reserved fields are properly initialized. In practice
> > the
> > PTP_CLOCK_GETCAPS2 and PTP_CLOCK_GETCAPS both behave identically
> > since
>
> Exactly, that is why I send a patch to add the new 'adjust_phase' in
> do_caps(), without changing the ioctl itself :-)
>
>
The reason PTP_CLOCK_GETCAPS2 was created was around the same time as the other
ioctls were fixed to properly a) validate reserved fields and b) ensure that
all reported reserved fields were zeroed.
Functionally its 100% identical to PTP_CLOCK_GETCAPS because this ioctl already
did the right thing.
The reason I propose switching to PTP_CLOCK_GETCAPS2 over PTP_CLOCK_GETCAPS is
because it reduces cognitive burden to REMEMBER that the old PTP_CLOCK_GETCAPS
is ok. All I remembered was "the non-2 ioctls might be broken and not receive
new features".
> > PTP_CLOCK_GETCAPS always zeroed the caps structure. However, it is
> > good
> > practice to use the newer version consistently.
>
> But, it can also breaks when using the new linuxptp version build on
> new machine, but used with old kernel.
> It may be reasonable in few years, when these old kernels becomes
> obsolete.
This uses a fallback to PTP_CLOCK_GETCAPS if PTP_CLOCK_GETCAPS2 is not found.
> But since the new PTP_CLOCK_GETCAPS2 was introduce in 2019, I think we
> should wait with it.
> As not everyone build linuxptp, most users use pre-build.
>
>
> >
> > Signed-off-by: Jacob Keller
> > ---
> > Technically this is identical in current kernels, both ioctls return
> > exactly
> > the same conent.. I still think its better to use the '2' variants in
> > principle that developer doesn't have to remember "is this variant ok
> > to get
> > all the new features going forward?"
>
> There are more 12 bytes in the reserve.
> I guess once we pass them, we would need a non-backward flag, but till
> then, I think we can keep using the PTP_CLOCK_GETCAPS flag, and leave
> the new flags in do_caps().
>
The only motiviation for this patch is consistency in using the "2" style
ioctls across ptp4l, because it makes it less likely someone will use the non-2
version of an ioctl that actually does have behavioral differences in the
future.
Thanks,
Jake
> Erez
>
> >
> > missing.h | 7 +++
> > phc_ctl.c | 2 +-
> > 2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/missing.h b/missing.h
> > index 79a87d425993..41427d3a38b2 100644
> > --- a/missing.h
> > +++ b/missing.h
> > @@ -98,6 +98,13 @@ enum {
> > #define PTP_PEROUT_REQUEST2 PTP_PEROUT_REQUEST
> > #endif
> >
> > +#ifdef PTP_CLOCK_GETCAPS2
> > +#define PTP_CLOCK_GETCAPS_REQUEST_FAILED "PTP_CLOCK_GETCAPS2
> failed:
> > %m"
> > +#else
> > +#define PTP_CLOCK_GETCAPS_REQUEST_FAILED "PTP_CLOCK_GETCAPS failed:
> > %m"
> > +#define PTP_CLOCK_GETCAPS2 PTP_CLOCK_GETCAPS
> > +#endif
> > +
> > #if LINUX_VERSION_CODE < KERNEL_VERSION(5,8,0)
> >
> > /* from upcoming Linux kernel version 5.8 */
> > diff --git a/phc_ctl.c b/phc_ctl.c
> > index 6a5c2f43d7c9..17a615f8aae6 100644
> > --- a/phc_ctl.c
> > +++ b/phc_ctl.c
> > @@ -288,7 +288,7 @@ static int do_caps(clockid_t clkid, int cmdc,
> > char *cmdv[])
> > return 0;
> > }
> >
> > - if (ioctl(CLOCKID_TO_FD(clkid), PTP_CLOCK_GETCAPS, )) {
> > + if (ioctl(CLOCKID_TO_FD(clkid), PTP_CLOCK_GETCAPS2, )) {
> > pr_err("get capabilities failed: %s",
> > strerror(errno));
> > return -1;
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] servo: stop rounding initial frequency to nearest ppb
> -Original Message- > From: Miroslav Lichvar > Sent: Wednesday, November 16, 2022 4:13 AM > To: Keller, Jacob E > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH] servo: stop rounding initial frequency > to > nearest ppb > > On Tue, Nov 15, 2022 at 04:41:09PM -0800, Jacob Keller wrote: > > Refactor the servo_create API to take a double value instead of an integer > > value. Fix the clockadj_get_freq and clockadj_set_freq initialization to > > avoid casting to an integer and thus rounding the value. > > Looks good to me. Thanks for submitting the patch. > > I think this patch conflicts with the clockcheck patchset. I'll update > it and resend if this one is accepted earlier. > > -- > Miroslav Lichvar Ah right, you refactored to drop a bunch of the code this modifies. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/4] Add sock servo.
> -Original Message- > From: Hal Murray > Sent: Tuesday, November 15, 2022 2:09 AM > To: Keller, Jacob E > Cc: Maciek Machnikowski ; Hal Murray > ; linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH 2/4] Add sock servo. > > >> What about rcl_sock or refclock_sock? It's used in the file linked by > Miroslav. > > Both of those sound good to me. Slight preference to refclock_sock if its > > not too > long. > > How about SOCK? > > In the ntp context, we already have SHM and PPS. Both show up in the refid > slot in packets. > > Just to make sure we are on the same wavelength... I'm looking for a term > that can be used as a handle when the context is well known for things like > "Try SOCK, it worked for me." > Yea. I just found "sock" on its own to be generic and not tell me anything about what the servo did. I don't want to get too distracted by the name though... Thanks, Jake > > -- > These are my opinions. I hate spam. > > ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/4] Add sock servo.
> -Original Message- > From: Maciek Machnikowski > Sent: Monday, November 14, 2022 10:55 PM > To: Keller, Jacob E > Cc: Hal Murray ; Miroslav Lichvar > ; linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH 2/4] Add sock servo. > > On Mon, Nov 14, 2022 at 05:06:17PM +, Keller, Jacob E wrote: > > > > > > > -Original Message- > > > From: Hal Murray > > > Sent: Monday, November 14, 2022 4:34 AM > > > To: Miroslav Lichvar > > > Cc: Keller, Jacob E ; linuxptp- > > > de...@lists.sourceforge.net; Hal Murray > > > Subject: Re: [Linuxptp-devel] [PATCH 2/4] Add sock servo. > > > > > > > > > >> How specific is this to chronyd? > > > > AFAIK no other application implements the server side of the protocol. > > > >> Would it make sense to call this chronysock > > > >> instead of just sock? > > > > Yes, that makes sense. If there are no other issues with the patches, I > > > > can > > > > resend. > > > > > > Calling it chronysock has the disadvantage of sounding like only chrony > > > should > > > use it. > > > > > > > Yea, but I feel that just "sock" is vague. I'm not totally opposed to it > > though. > > What about rcl_sock or refclock_sock? It's used in the file linked by > Miroslav. > > Regards > Maciek Both of those sound good to me. Slight preference to refclock_sock if its not too long. Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/4] Add sock servo.
> -Original Message- > From: Hal Murray > Sent: Monday, November 14, 2022 4:34 AM > To: Miroslav Lichvar > Cc: Keller, Jacob E ; linuxptp- > de...@lists.sourceforge.net; Hal Murray > Subject: Re: [Linuxptp-devel] [PATCH 2/4] Add sock servo. > > > >> How specific is this to chronyd? > > AFAIK no other application implements the server side of the protocol. > >> Would it make sense to call this chronysock > >> instead of just sock? > > Yes, that makes sense. If there are no other issues with the patches, I can > > resend. > > Calling it chronysock has the disadvantage of sounding like only chrony should > use it. > Yea, but I feel that just "sock" is vague. I'm not totally opposed to it though. Thanks, Jake > >> The implementation seems fine but its using an interface that was defined > >> by > >> chrony. I suppose another application could implement the same interface > >> though.. > > > ntpsec might be interested in implementing it. We'll see. > > Is there a URL for the spec? I don't want an RFC. Good comments in a header > file may be enough. A separate document may be better if there are > complications that need explaining. > > is there a version number? > > > > I agree that the current SHM setup is far from wonderful. There is a clean > way to make SHM read-only by receivers so you can have multiple receivers. > That would let you run gpsmon while chronyd/ntpd is running. > > > -- > These are my opinions. I hate spam. > > ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] phc_ctl: add pin_cfg command to display pin functions
> -Original Message- > From: Miroslav Lichvar > Sent: Thursday, October 27, 2022 8:00 AM > To: Keller, Jacob E > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH] phc_ctl: add pin_cfg command to display > pin functions > > On Thu, Oct 27, 2022 at 03:32:48AM -0700, Jacob Keller wrote: > > Add a new function to phc_ctl to display the devices pin configuration > > data. First, obtain the device capabilities to determine the number of > > pins. Then, for each pin, print the name, function, and channel > > information. > > Nice feature. In a quick test, it printed the state correctly after > setting it with "testptp -L". > > Do you plan to add also pin_set? > > -- > Miroslav Lichvar I just found this while looking through patches I forgot to send. I think pin_set might be useful so I can go ahead and try to follow up with that. Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 4/4] Extend clockcheck to check for changes in frequency.
> -Original Message-
> From: Miroslav Lichvar
> Sent: Monday, October 24, 2022 5:43 AM
> To: Keller, Jacob E
> Cc: linuxptp-devel@lists.sourceforge.net
> Subject: Re: [Linuxptp-devel] [PATCH v2 4/4] Extend clockcheck to check for
> changes in frequency.
>
> On Thu, Oct 20, 2022 at 04:50:37PM +, Keller, Jacob E wrote:
> > > index f0141be..b5a69cc 100644
> > > --- a/clockcheck.c
> > > +++ b/clockcheck.c
> > > @@ -123,6 +123,16 @@ void clockcheck_set_freq(struct clockcheck *cc, int
> freq)
> > > cc->freq_known = 1;
> > > }
> > >
> > > +int clockcheck_freq(struct clockcheck *cc, int freq)
> > > +{
> > > + /* Allow difference of 1 ppb due to conversion to/from double */
> > > + if (cc->freq_known && abs(cc->current_freq - freq) > 1) {
> > > + pr_warning("clockcheck: clock frequency changed
> > > unexpectedly!");
> >
> >
> > The modified documentation would make me think this should allow up to the
> sanity_freq_limit as a difference? Perhaps the documentation should be
> improved somewhat to clarify the difference?
>
> If I expand the description a bit, is it better?
>
> .B sanity_freq_limit
> The maximum allowed frequency offset between uncorrected clock and the
> system
> monotonic clock in parts per billion (ppb). This is used as a sanity check of
> the synchronized clock. When a larger offset is measured, a warning message
> will be printed and the servo will be reset. If the frequency correction set
> by
> ptp4l changes unexpectedly between updates of the clock (e.g. due to another
> process trying to control the clock), a warning message will be printed. When
> set to 0, the sanity check is disabled. The default is 2 (20%).
>
>
> If not, what would you suggest?
>
I like this better!
Thanks,
Jake
> Thanks,
>
> --
> Miroslav Lichvar
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 0/4] Bug fix and improved clockcheck
> -Original Message- > From: Miroslav Lichvar > Sent: Thursday, October 20, 2022 7:13 AM > To: linuxptp-devel@lists.sourceforge.net > Subject: [Linuxptp-devel] [PATCH v2 0/4] Bug fix and improved clockcheck > > v2: > - added patch to exit on errors returned by read-only clock_adjtime() > - simplified last patch to reuse the set frequency and allow +/-1 ppb > error due to conversions between int, double and scaled ppm > - improved commit messages > Thanks! The series overall looks good to me. A minor documentation question/concern but I think the implementation seems fine. Reviewed-by: Jacob Keller > This set improves the clock check to consider the last frequency set by > ptp4l/phc2sys in order to detect cases where multiple processes are > trying to control the same clock due to a misconfiguration or bug. > > Miroslav Lichvar (4): > phc2sys: Add clocks after processing configuration. > Drop support for old kernels returning zero frequency. > Don't accept errors in clockadj_get_freq(). > Extend clockcheck to check for changes in frequency. > > clock.c | 10 +++--- > clockadj.c | 2 ++ > clockcheck.c | 10 ++ > clockcheck.h | 8 > phc2sys.8| 2 +- > phc2sys.c| 48 ++-- > ptp4l.8 | 4 +++- > ts2phc.c | 7 --- > 8 files changed, 53 insertions(+), 38 deletions(-) > > -- > 2.37.3 > > > > ___ > 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
Re: [Linuxptp-devel] [PATCH v2 4/4] Extend clockcheck to check for changes in frequency.
> -Original Message-
> From: Miroslav Lichvar
> Sent: Thursday, October 20, 2022 7:13 AM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH v2 4/4] Extend clockcheck to check for
> changes
> in frequency.
>
> Before setting the new frequency offset on a clock update, compare the
> current frequency returned by the kernel with the value saved from the
> previous update. Print a warning message if the difference is larger
> than 1 ppb, allowing for rounding errors in conversion to and from
> double. The kernel caches the value set by clock_adjtime() in shifted
> ppm, it doesn't request it from the driver, which can have a lower
> resulution.
>
> This should detect misconfigurations where multiple processes are trying
> to control the clock (e.g. another ptp4l/phc2sys instance or an NTP
> client), even when they don't step the clock.
>
> Signed-off-by: Miroslav Lichvar
> ---
> clock.c | 3 +++
> clockcheck.c | 10 ++
> clockcheck.h | 8
> phc2sys.c| 3 +++
> ptp4l.8 | 4 +++-
> 5 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/clock.c b/clock.c
> index 46ac9c2..8177e77 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -1776,6 +1776,9 @@ static void clock_step_window(struct clock *c)
>
> static void clock_synchronize_locked(struct clock *c, double adj)
> {
> + if (c->sanity_check) {
> + clockcheck_freq(c->sanity_check, clockadj_get_freq(c->clkid));
> + }
> clockadj_set_freq(c->clkid, -adj);
> if (c->clkid == CLOCK_REALTIME) {
> sysclk_set_sync();
> diff --git a/clockcheck.c b/clockcheck.c
> index f0141be..b5a69cc 100644
> --- a/clockcheck.c
> +++ b/clockcheck.c
> @@ -123,6 +123,16 @@ void clockcheck_set_freq(struct clockcheck *cc, int freq)
> cc->freq_known = 1;
> }
>
> +int clockcheck_freq(struct clockcheck *cc, int freq)
> +{
> + /* Allow difference of 1 ppb due to conversion to/from double */
> + if (cc->freq_known && abs(cc->current_freq - freq) > 1) {
> + pr_warning("clockcheck: clock frequency changed
> unexpectedly!");
The modified documentation would make me think this should allow up to the
sanity_freq_limit as a difference? Perhaps the documentation should be improved
somewhat to clarify the difference?
Thanks,
Jake
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] sk.c: Unreachable switch case TS_SOFTWARE
> -Original Message-
> From: Michael Galassi
> Sent: Sunday, September 25, 2022 11:38 AM
> To: Richard Cochran
> Cc: linuxptp-devel@lists.sourceforge.net
> Subject: Re: [Linuxptp-devel] [PATCH] sk.c: Unreachable switch case
> TS_SOFTWARE
>
> On Sun, Sep 25, 2022 at 11:23 AM Richard Cochran
> wrote:
> >
> > On Thu, Jul 21, 2022 at 03:10:40PM +0530, Siddharth Vadapalli via Linuxptp-
> devel wrote:
> > > Remove the unreachable switch case TS_SOFTWARE.
> > >
> > > Signed-off-by: Siddharth Vadapalli
> > > ---
> > > sk.c | 3 ---
> > > 1 file changed, 3 deletions(-)
> > >
> > > diff --git a/sk.c b/sk.c
> > > index b55d6b5..a49bcf4 100644
> > > --- a/sk.c
> > > +++ b/sk.c
> > > @@ -477,9 +477,6 @@ int sk_timestamping_init(int fd, const char *device,
> enum timestamp_type type,
> > > if (type != TS_SOFTWARE) {
> > > filter1 = HWTSTAMP_FILTER_PTP_V2_EVENT;
> > > switch (type) {
> > > - case TS_SOFTWARE:
> > > - tx_type = HWTSTAMP_TX_OFF;
> > > - break;
> >
> > This introduces a build failure with -Werror
> >
> > /home/richard/git/linuxptp/sk.c: In function ‘sk_timestamping_init’:
> > /home/richard/git/linuxptp/sk.c:484:3: error: enumeration value
> ‘TS_SOFTWARE’ not handled in switch [-Werror=switch]
> >switch (type) {
> >^~
> > cc1: all warnings being treated as errors
> > make: *** [: sk.o] Error 1
> >
> > So NAK to this patch.
>
> I've developed the habit of closing all non-exhaustive switches where
> no sensible default exists with a default which asserts with a
> meaningful diagnostic. Doing so has helped me find a number of my own
> mistaken assumptions. I suspect, however, that a project with as many
> eyes on it as this one won't have too many of those.
>
> -michael
>
>
That's what the compiler does for switch cases which are exhaustive. For
non-exhaustive ones they should have a default behavior for any un-covered
cases.
> ___
> 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
Re: [Linuxptp-devel] [PATCHv2 2/4] sysoff: Change sysoff_measure() to return ioctl errno.
> -Original Message-
> From: Miroslav Lichvar
> Sent: Thursday, March 24, 2022 6:20 AM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCHv2 2/4] sysoff: Change sysoff_measure() to
> return ioctl errno.
>
> Return -errno instead of SYSOFF_RUN_TIME_MISSING from the sysoff
> measurement functions to allow the callers to check for specific errors.
>
This is changing from returning the type to returning an error code. I would
have expected equivalent changes to the other applications that use this..
> Signed-off-by: Miroslav Lichvar
> ---
> sysoff.c | 15 ---
> sysoff.h | 2 +-
> 2 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/sysoff.c b/sysoff.c
> index 31b792d..0678df6 100644
> --- a/sysoff.c
> +++ b/sysoff.c
> @@ -17,6 +17,7 @@
> * with this program; if not, write to the Free Software Foundation, Inc.,
> * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> */
> +#include
> #include
> #include
> #include
> @@ -38,11 +39,11 @@ static int sysoff_precise(int fd, int64_t *result,
> uint64_t
> *ts)
> memset(, 0, sizeof(pso));
> if (ioctl(fd, PTP_SYS_OFFSET_PRECISE, )) {
> pr_debug("ioctl PTP_SYS_OFFSET_PRECISE: %m");
> - return SYSOFF_RUN_TIME_MISSING;
> + return -errno;
> }
> *result = pctns(_realtime) - pctns();
> *ts = pctns(_realtime);
> - return SYSOFF_PRECISE;
> + return 0;
> }
>
> static int64_t sysoff_estimate(struct ptp_clock_time *pct, int extended,
> @@ -98,10 +99,10 @@ static int sysoff_extended(int fd, int n_samples,
> pso.n_samples = n_samples;
> if (ioctl(fd, PTP_SYS_OFFSET_EXTENDED, )) {
> pr_debug("ioctl PTP_SYS_OFFSET_EXTENDED: %m");
> - return SYSOFF_RUN_TIME_MISSING;
> + return -errno;
> }
> *result = sysoff_estimate([0][0], 1, n_samples, ts, delay);
> - return SYSOFF_EXTENDED;
> + return 0;
> }
>
> static int sysoff_basic(int fd, int n_samples,
> @@ -112,10 +113,10 @@ static int sysoff_basic(int fd, int n_samples,
> pso.n_samples = n_samples;
> if (ioctl(fd, PTP_SYS_OFFSET, )) {
> pr_debug("ioctl PTP_SYS_OFFSET: %m");
> - return SYSOFF_RUN_TIME_MISSING;
> + return -errno;
> }
> *result = sysoff_estimate(pso.ts, 0, n_samples, ts, delay);
> - return SYSOFF_BASIC;
> + return 0;
> }
>
> int sysoff_measure(int fd, int method, int n_samples,
> @@ -130,7 +131,7 @@ int sysoff_measure(int fd, int method, int n_samples,
> case SYSOFF_BASIC:
> return sysoff_basic(fd, n_samples, result, ts, delay);
> }
> - return SYSOFF_RUN_TIME_MISSING;
> + return -EOPNOTSUPP;
> }
>
> int sysoff_probe(int fd, int n_samples)
> diff --git a/sysoff.h b/sysoff.h
> index e4de919..ed338fd 100644
> --- a/sysoff.h
> +++ b/sysoff.h
> @@ -44,7 +44,7 @@ int sysoff_probe(int fd, int n_samples);
> * @param result The estimated offset in nanoseconds.
> * @param ts The system time corresponding to the 'result'.
> * @param delay The delay in reading of the clock in nanoseconds.
> - * @return One of the SYSOFF_ enumeration values.
> + * @return Zero on success, negative otherwise.
> */
> int sysoff_measure(int fd, int method, int n_samples,
> int64_t *result, uint64_t *ts, int64_t *delay);
> --
> 2.35.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
Re: [Linuxptp-devel] [PATCH v2 1/4] unicast: Add support to check if message was received from an entry in the unicast master table.
> -Original Message- > From: vincent.cheng...@renesas.com > Sent: Saturday, March 19, 2022 7:04 PM > To: Keller, Jacob E > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH v2 1/4] unicast: Add support to check if > message was received from an entry in the unicast master table. > > >I'm ok with the name if Richard is, but thought I'd voice my bikeshed > >opinion. > > Changed to unicast_client_msg_is_from_master_table_entry() in v3 patch. > The unicast_client prefix is to keep with existing naming in the file. > > Thanks, > Vincent Yep that seems reasonable to me. Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] phc2sys: Don't exit when reading of PHC fails.
> -Original Message- > From: Miroslav Lichvar > Sent: Wednesday, March 16, 2022 1:58 AM > To: Keller, Jacob E > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH] phc2sys: Don't exit when reading of PHC > fails. > > On Tue, Mar 15, 2022 at 10:40:02PM +, Keller, Jacob E wrote: > > The ice hardware has a semaphore to protect accesses to the clock register. > Multiple functions might access the clock at the same time. The semaphore > prevents this. If we fail to acquire the semaphore in a short time, we give > up on > the gettime function and exit with -EBUSY. > > > > Does the inner call log a debug message? if not perhaps we might want to add > such a debug message to this as well? > > Yes, there is a debug message for that: > > phc2sys[497426.052]: ioctl PTP_SYS_OFFSET_EXTENDED: Device or resource busy > phc2sys[497434.057]: ioctl PTP_SYS_OFFSET_EXTENDED: Device or resource busy > phc2sys[497452.132]: ioctl PTP_SYS_OFFSET_EXTENDED: Device or resource busy > Ok great. > A potential issue is that if the ioctl happens to fail on the first > use in sysoff_probe(), phc2sys will fall back to a worse ioctl. We > could repeat the probe multiple times and use the best result, or > maybe specifically check for the known error numbers like -EBUSY, -EIO. > Repeating it a couple times seems reasonable to me. Depending on specific error codes is also nice, but we should ideally document those if we do. Specifically document them in the ioctl and in the ptp_clock_info callback descriptions. Thanks, Jake > -- > Miroslav Lichvar ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 1/1] TLV management messages need to be aligned to 16 bits.
> -Original Message- > From: Dale Smith > Sent: Tuesday, March 15, 2022 5:07 PM > To: Erez Geva > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH 1/1] TLV management messages need to be > aligned to 16 bits. > > On 3/8/22, Erez Geva wrote: > > struct PortIdentity portIdentity; > > Integer32 phc_index; > > UInteger8 flags; > > + uint8_t reserved; > > What is the difference between uint8_t and UInteger8 ? > Shouldn't these be the same type? > > I don't know, but it looks fishy to me. > > -Dale I believe UInteger8 is used because this is precisely how the structures are defined in the standard, which uses these names. Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 3/4] port: unicast client - do not add master to foreign master table if not in the unicast master table.
> -Original Message-
> From: vincent.cheng...@renesas.com
> Sent: Sunday, March 13, 2022 10:01 PM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH v2 3/4] port: unicast client - do not add
> master
> to foreign master table if not in the unicast master table.
>
> From: Vincent Cheng
>
> Signed-off-by: Vincent Cheng
Looks like some whitespace got changed on accident. Otherwise this patch looks
good to me.
Reviewed-by: Jacob Keller
> ---
> port.c | 28 +++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/port.c b/port.c
> index f2b666c..0169161 100644
> --- a/port.c
> +++ b/port.c
> @@ -167,6 +167,27 @@ static int msg_source_equal(struct ptp_message *m1,
> struct foreign_clock *fc)
> return 0 == memcmp(id1, id2, sizeof(*id1));
> }
>
> +static int port_unicast_message_valid(struct port *p, struct ptp_message *m)
> +{
> + struct unicast_master_address master;
> +
> + if (!unicast_client_unicast_master_table_received(p, m)) {
> + memset(, 0, sizeof(master));
> + master.address = m->address;
> + master.portIdentity = m->header.sourcePortIdentity;
> +
> + pr_warning("%s: new foreign master %s not in unicast master
> table",
> +p->log_name, pid2str(
> >header.sourcePortIdentity));
> +
> + if (unicast_client_tx_cancel(p, )) {
> + pr_warning("%s: cancel unicast transmission to %s
> failed",
> +p->log_name, pid2str(
> >header.sourcePortIdentity));
> + }
> + return 0;
> + }
> + return 1;
> +}
> +
> int source_pid_eq(struct ptp_message *m1, struct ptp_message *m2)
> {
> return pid_eq(>header.sourcePortIdentity,
> @@ -351,8 +372,13 @@ static int add_foreign_master(struct port *p, struct
> ptp_message *m)
> }
> }
> if (!fc) {
> + if (unicast_client_enabled(p)) {
> + if (!port_unicast_message_valid(p, m)) {
> + return 0;
> + }
> + }
> pr_notice("%s: new foreign master %s", p->log_name,
> - pid2str(>header.sourcePortIdentity));
> + pid2str(>header.sourcePortIdentity));
What's going on here? some whitespace fixup?
>
> fc = malloc(sizeof(*fc));
> if (!fc) {
> --
> 2.34.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
Re: [Linuxptp-devel] [PATCH v2 4/4] port: cancel unicast transmission when closing port.
> -Original Message-
> From: vincent.cheng...@renesas.com
> Sent: Sunday, March 13, 2022 10:01 PM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH v2 4/4] port: cancel unicast transmission
> when
> closing port.
>
> From: Vincent Cheng
>
> Signed-off-by: Vincent Cheng
Reviewed-by: Jacob Keller
> ---
> port.c | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/port.c b/port.c
> index 0169161..1480630 100644
> --- a/port.c
> +++ b/port.c
> @@ -167,6 +167,21 @@ static int msg_source_equal(struct ptp_message *m1,
> struct foreign_clock *fc)
> return 0 == memcmp(id1, id2, sizeof(*id1));
> }
>
> +static void port_cancel_unicast(struct port *p)
> +{
> + struct unicast_master_address *ucma;
> +
> + if (!unicast_client_enabled(p)) {
> + return;
> + }
> +
> + STAILQ_FOREACH(ucma, >unicast_master_table->addrs, list) {
> + if (ucma) {
> + unicast_client_tx_cancel(p, ucma);
> + }
> + }
> +}
> +
> static int port_unicast_message_valid(struct port *p, struct ptp_message *m)
> {
> struct unicast_master_address master;
> @@ -2488,6 +2503,7 @@ void process_sync(struct port *p, struct ptp_message
> *m)
> void port_close(struct port *p)
> {
> if (port_is_enabled(p)) {
> + port_cancel_unicast(p);
> port_disable(p);
> }
>
> --
> 2.34.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
Re: [Linuxptp-devel] [PATCH v2 2/4] unicast: Add support to send CANCEL_UNICAST_TRANSMISSION TLVs.
> -Original Message-
> From: vincent.cheng...@renesas.com
> Sent: Sunday, March 13, 2022 10:01 PM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH v2 2/4] unicast: Add support to send
> CANCEL_UNICAST_TRANSMISSION TLVs.
>
> From: Vincent Cheng
>
> Signed-off-by: Vincent Cheng
> ---
This one looks good to me.
Reviewed-by: Jacob Keller
> unicast_client.c | 50
> unicast_client.h | 8
> 2 files changed, 58 insertions(+)
>
> diff --git a/unicast_client.c b/unicast_client.c
> index 4d6386e..7688814 100644
> --- a/unicast_client.c
> +++ b/unicast_client.c
> @@ -64,6 +64,23 @@ static int attach_request(struct ptp_message *msg, int
> log_period,
> return 0;
> }
>
> +static int attach_cancel(struct ptp_message *msg, uint8_t message_type)
> +{
> + struct cancel_unicast_xmit_tlv *req;
> + struct tlv_extra *extra;
> +
> + extra = msg_tlv_append(msg, sizeof(*req));
> + if (!extra) {
> + return -1;
> + }
> + req = (struct cancel_unicast_xmit_tlv *) extra->tlv;
> + req->type = TLV_CANCEL_UNICAST_TRANSMISSION;
> + req->length = sizeof(*req) - sizeof(req->type) - sizeof(req->length);
> + req->message_type_flags = message_type << 4;
> +
> + return 0;
> +}
> +
> static int unicast_client_announce(struct port *p,
> struct unicast_master_address *dst)
> {
> @@ -563,3 +580,36 @@ int unicast_client_unicast_master_table_received(struct
> port *p, struct ptp_mess
> return ucma ? 1 : 0;
> }
>
> +int unicast_client_tx_cancel(struct port *p,
> + struct unicast_master_address *dst)
> +{
> + struct ptp_message *msg;
> + int err;
> +
> + msg = port_signaling_uc_construct(p, >address,
> >portIdentity);
> + if (!msg) {
> + return -1;
> + }
> + err = attach_cancel(msg, ANNOUNCE);
> + if (err) {
> + goto out;
> + }
> + err = attach_cancel(msg, SYNC);
> + if (err) {
> + goto out;
> + }
> + if (p->delayMechanism != DM_P2P) {
> + err = attach_cancel(msg, DELAY_RESP);
> + if (err) {
> + goto out;
> + }
> + }
> +
> + err = port_prepare_and_send(p, msg, TRANS_GENERAL);
> + if (err) {
> + pr_err("%s: signaling message failed", p->log_name);
> + }
> +out:
> + msg_put(msg);
> + return err;
> +}
> diff --git a/unicast_client.h b/unicast_client.h
> index b24c4eb..bb0f80a 100644
> --- a/unicast_client.h
> +++ b/unicast_client.h
> @@ -93,4 +93,12 @@ int unicast_client_timer(struct port *p);
> int unicast_client_unicast_master_table_received(struct port *p,
>struct ptp_message *m);
>
> +/**
> + * Transmit CANCEL_UNICAST_TRANSMISSION TLV to destination address.
> + * @param p The port in question.
> + * @param dstThe destination address.
> + * @return Zero on success, non-zero otherwise.
> + */
> +int unicast_client_tx_cancel(struct port *p,
> + struct unicast_master_address *dst);
> #endif
> --
> 2.34.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
Re: [Linuxptp-devel] [PATCH v2 1/4] unicast: Add support to check if message was received from an entry in the unicast master table.
> -Original Message-
> From: vincent.cheng...@renesas.com
> Sent: Sunday, March 13, 2022 10:01 PM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH v2 1/4] unicast: Add support to check if
> message was received from an entry in the unicast master table.
>
> From: Vincent Cheng
>
> Signed-off-by: Vincent Cheng
> ---
> unicast_client.c | 16
> unicast_client.h | 11 +++
> 2 files changed, 27 insertions(+)
>
> diff --git a/unicast_client.c b/unicast_client.c
> index 87d8471..4d6386e 100644
> --- a/unicast_client.c
> +++ b/unicast_client.c
> @@ -547,3 +547,19 @@ int unicast_client_timer(struct port *p)
> unicast_client_set_tmo(p);
> return err;
> }
> +
> +int unicast_client_unicast_master_table_received(struct port *p, struct
> ptp_message *m)
> +{
> + struct unicast_master_address *ucma;
> +
> + if (!unicast_client_enabled(p)) {
> + return 0;
> + }
> + STAILQ_FOREACH(ucma, >unicast_master_table->addrs, list) {
> + if (addreq(transport_type(p->trp), >address,
> >address)) {
> + break;
> + }
> + }
Does STALQ_FOREACH guarantee that ucma is NULL after exiting?
For code clarity I'd rather have a separate variable set set to 1 when addreq
returns true. That is far easier to read, since not every for each iterator
works this way.
> + return ucma ? 1 : 0;
> +}
> +
> diff --git a/unicast_client.h b/unicast_client.h
> index 16e291f..b24c4eb 100644
> --- a/unicast_client.h
> +++ b/unicast_client.h
> @@ -82,4 +82,15 @@ void unicast_client_state_changed(struct port *p);
> */
> int unicast_client_timer(struct port *p);
>
> +/**
> + * Check whether a message was received from an entry in the unicast
> + * master table.
> + * @param p The port in question.
> + * @param m The message in question.
> + * @return One (1) if the message is from an entry in the unicast
> + * master table, or zero otherwise.
> + */
> +int unicast_client_unicast_master_table_received(struct port *p,
> + struct ptp_message *m);
> +
> #endif
The name of the function doesn't quite capture its meaning to me.
Perhaps something like "unicast_message_is_from_master_table_entry"?
I'm ok with the name if Richard is, but thought I'd voice my bikeshed opinion.
> --
> 2.34.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
Re: [Linuxptp-devel] [PATCH v2 0/4] port: Cancel unicast service when closing port.
> -Original Message- > From: vincent.cheng...@renesas.com > Sent: Sunday, March 13, 2022 10:01 PM > To: linuxptp-devel@lists.sourceforge.net > Subject: [Linuxptp-devel] [PATCH v2 0/4] port: Cancel unicast service when > closing port. > > From: Vincent Cheng > > ptp4l currently does not cancel unicast service on program exit. > This causes problems for subsequent ptp4l session that is using different > masters. > > Below example has debug statements to print out announce master address and > received packet address. > > Start unicast slave session with master 10.64.10.13, and unicast_req_duration > 300. > > ptp4l[682807.352]: port 1 (eth0): INITIALIZING to LISTENING on INIT_COMPLETE > ptp4l[682811.352]: debug: announce master 10.64.10.13 > ptp4l[682812.770]: debug: sk_receive from 10.64.10.13 > ptp4l[682812.810]: debug: sk_receive from 10.64.10.13 > ptp4l[682813.120]: selected local clock 000a35.fffe.002201 as best master > ptp4l[682814.755]: port 1 (eth0): new foreign master 84c807.fffe.10c570-1 > > Exit ptp4l. > > The unicast master continues to send SYNC packets to slave until renewal > period > expires. > > Start unicast same slave, but different master 10.64.10.14. This master is > not > online, so ptp4l should choose local clock as best master. > > However, because there is no checking of received unicast master to configured > unicast table, ptp4l is quite content with packets from previous master > 10.64.10.13. > > ptp4l[682848.858]: port 1 (eth0): INITIALIZING to LISTENING on INIT_COMPLETE > ptp4l[682848.892]: debug: sk_receive from 10.64.10.13 > ... > ptp4l[682850.704]: debug: sk_receive from 10.64.10.13 > ptp4l[682850.751]: debug: sk_receive from 10.64.10.13 > ptp4l[682850.751]: port 1 (eth0): new foreign master 84c807.fffe.10c570-1 > ptp4l[682850.766]: debug: sk_receive from 10.64.10.13 > ... > ptp4l[682852.829]: debug: sk_receive from 10.64.10.13 > ptp4l[682852.858]: debug: announce master 10.64.10.14 > ptp4l[682852.891]: debug: sk_receive from 10.64.10.13 > ptp4l[682854.751]: port 1 (eth0): LISTENING to UNCALIBRATED on RS_SLAVE > ptp4l[682854.766]: debug: sk_receive from 10.64.10.13 > ... > ptp4l[682854.891]: master offset -3269 s0 freq-397 path delay > 4757 > > Add check to ensure a unicast client will check the source address of an > announce > message is from an entry in the unicast master table before adding to foreign > master table. > Add canceling unicast session on exit so that subsequent session is not > polluted > by lingering unicast session traffic. > So with this series, we now fix our program to be nice and stop the session, and we also fix our program to verify that we got a valid unicast address before accepting it. Excellent. > Vincent Cheng (4): > unicast: Add support to check if message was received from an entry in > the unicast master table. > unicast: Add support to send CANCEL_UNICAST_TRANSMISSION TLVs. > port: unicast client - do not add master to foreign master table if > not in the unicast master table. > port: cancel unicast transmission when closing port. > > port.c | 44 +++- > unicast_client.c | 66 > unicast_client.h | 19 ++ > 3 files changed, 128 insertions(+), 1 deletion(-) > > -- > 2.34.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
Re: [Linuxptp-devel] [PATCH] phc2sys: Don't exit when reading of PHC fails.
> -Original Message-
> From: Miroslav Lichvar
> Sent: Thursday, March 10, 2022 3:23 AM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH] phc2sys: Don't exit when reading of PHC
> fails.
>
> Reading of the PHC can occasionally fail with some drivers, e.g. the ice
> driver returns -EBUSY when it fails to get a lock. Change do_loop() to
> ignore the error instead of exiting.
>
> Signed-off-by: Miroslav Lichvar
> ---
Yep. Thanks for fixing this. I'd like to clarify why ice does sometimes return
-EBUSY for reviewers on this list.
The ice hardware has a semaphore to protect accesses to the clock register.
Multiple functions might access the clock at the same time. The semaphore
prevents this. If we fail to acquire the semaphore in a short time, we give up
on the gettime function and exit with -EBUSY.
Does the inner call log a debug message? if not perhaps we might want to add
such a debug message to this as well?
Thanks,
Jake
> phc2sys.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/phc2sys.c b/phc2sys.c
> index 15dd689..281ec9d 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -730,7 +730,7 @@ static int do_loop(struct phc2sys_private *priv)
> priv->master->sysoff_method,
> priv->phc_readings,
> , , ) < 0)
> - return -1;
> + continue;
> } else if (priv->master->clkid == CLOCK_REALTIME &&
> clock->sysoff_method >= 0) {
> /* use reversed sysoff */
> @@ -738,7 +738,7 @@ static int do_loop(struct phc2sys_private *priv)
> clock->sysoff_method,
> priv->phc_readings,
> , , ) < 0)
> - return -1;
> + continue;
> offset = -offset;
> ts += offset;
> } else {
> @@ -747,7 +747,7 @@ static int do_loop(struct phc2sys_private *priv)
>clock->clkid,
>priv->phc_readings,
>, , ))
> - return -1;
> + continue;
> }
> update_clock(priv, clock, offset, ts, delay);
> }
> --
> 2.35.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
Re: [Linuxptp-devel] [PATCH] Add PORT_SERVICE_STATS_NP management TLV
> -Original Message- > From: Richard Cochran > Sent: Tuesday, January 25, 2022 6:10 PM > To: Dale Smith > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH] Add PORT_SERVICE_STATS_NP > management TLV > > On Tue, Jan 25, 2022 at 05:04:44PM -0500, Dale Smith wrote: > > I don't see a way to reset/clear these counters. Should there be? It > > will take a while to overflow 64 bits, so it probably isn't needed? > > Q: If you have one missing Announce per second, when will it overflow? > > A: 2^64 = 18446744073709551616 s > = 5124095576030431 h > = 213503982334601 d > = 584542046090.6 y > = 5845420460.9 c > > (but I neglected the possible future leap seconds ;^) > > So it never overflows, practically speaking. > Even with a *significantly higher* loss of messages for the drops per second it won't overflow any time soon. And that amount of drops is unreasonable given the message rate of PTP. > Applications can simply latch the current value and subtract it from > subsequent reads in order to "reset" the counter. > It wouldn't hurt to have this hint documented, since I know I might not have thought to do that :) > Thanks, > Richard > > ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 0/2] fix fallback clock_gettime for test_phc
On 10/25/2021 12:25 AM, Miroslav Lichvar wrote: > On Fri, Oct 22, 2021 at 03:03:57PM -0700, Jacob Keller wrote: >> The current implementation of test_phc cmp has a fallback flow for comparing >> the PHC clock to the CLOCK_REAMTIME. This fallback flow calculates the >> inverse offset compared to the offsets calculated by the PTP_SYS_OFFSET >> ioctls. >> >> Fix this by replacing its implementation with the one from phc2sys. Move >> read_phc function into clockadj.c to re-use it. > > Looks good to me. > > I'd just expect a comment about the order of declarations in > clockadj_compare() that could be fixed as the code is being moved. > Not sure I follow your suggestion here? Do you just mean the way the local variables in clockadj_compare are declared? Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/2] util: fix dangling file descriptors on the error path of posix_clock_open
> -Original Message-
> From: Keller, Jacob E
> Sent: Monday, November 22, 2021 5:23 PM
> To: Vladimir Oltean ; richardcoch...@gmail.com
> Cc: linuxptp-devel@lists.sourceforge.net
> Subject: RE: [Linuxptp-devel] [PATCH 2/2] util: fix dangling file descriptors
> on the
> error path of posix_clock_open
>
>
>
> > -Original Message-
> > From: Vladimir Oltean
> > Sent: Monday, November 22, 2021 4:38 PM
> > To: richardcoch...@gmail.com
> > Cc: linuxptp-devel@lists.sourceforge.net
> > Subject: [Linuxptp-devel] [PATCH 2/2] util: fix dangling file descriptors
> > on the
> > error path of posix_clock_open
> >
> > When we determine that the "device" argument represents a path to a char
> > device, we call phc_open() and then we attempt to see, if the device
> > real path starts with "/dev/ptp", what number comes afterwards. If that
> > fails, we should close the char device we've just opened.
> >
> > Fixes: 380d023abb1f ("posix_clock_open: derive PHC index from device name if
> > possible")
> > Signed-off-by: Vladimir Oltean
>
>
> Does this really need to be a separate commit? It seems like we could have
> squashed this into the same fix as the series.
>
If you really want it to be separate, I suggest doing this one first in the
series.
Thanks,
Jake
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/2] util: fix dangling file descriptors on the error path of posix_clock_open
> -Original Message-
> From: Vladimir Oltean
> Sent: Monday, November 22, 2021 4:38 PM
> To: richardcoch...@gmail.com
> Cc: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH 2/2] util: fix dangling file descriptors on
> the
> error path of posix_clock_open
>
> When we determine that the "device" argument represents a path to a char
> device, we call phc_open() and then we attempt to see, if the device
> real path starts with "/dev/ptp", what number comes afterwards. If that
> fails, we should close the char device we've just opened.
>
> Fixes: 380d023abb1f ("posix_clock_open: derive PHC index from device name if
> possible")
> Signed-off-by: Vladimir Oltean
Does this really need to be a separate commit? It seems like we could have
squashed this into the same fix as the series.
Thanks,
Jake
> ---
> util.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/util.c b/util.c
> index 686ed5e2f110..34b0bc8e109c 100644
> --- a/util.c
> +++ b/util.c
> @@ -226,7 +226,8 @@ clockid_t posix_clock_open(const char *device, int
> *phc_index)
> fprintf(stderr,
> "failed to parse PHC index from %s\n",
> phc_device_path);
> - return -1;
> + phc_close(clkid);
> + return CLOCK_INVALID;
> }
> }
> return clkid;
> --
> 2.25.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
Re: [Linuxptp-devel] [PATCH v2 1/2] phc2sys: move read_phc into clock_adj.c
> -Original Message- > From: Richard Cochran > Sent: Monday, November 15, 2021 5:40 AM > To: Keller, Jacob E > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH v2 1/2] phc2sys: move read_phc into > clock_adj.c > > On Fri, Oct 22, 2021 at 03:03:58PM -0700, Jacob Keller wrote: > > Notice that read_phc returned 0 on failure and 1 on success. This is > > fairly non-standard, so lets update clockadj_compare to return 0 on > > success and -1 on failure. Fix the call sites to check correctly and > > report an error. > > +1 > > > +/** > > + * Compare offset between two clocks > > + * @param clkid A clock ID obtained using phc_open() or CLOCK_REALTIME > > + * @param sysclk A clock ID obtained using phc_open() or CLOCK_REALTIME > > + * @readings Number of readings to try > > + * @offset On return, the nanoseconds offset between the clocks > > + * @ts On return, the time of sysclk in nanoseconds that was used > > + * @delayOn return, the interval between two reads of sysclk > > Nit: please add the @return doxygen marker. Sure thing. Will send a v3. Thanks, Jake > > > + * > > + * Compare the offset between two clocks in a similar manner as the > > + * PTP_SYS_OFFSET ioctls. Returns the difference between sysclk and clkid > > by > > + * performing multiple reads of sysclk with a read of clkid between. > > + */ > > +int clockadj_compare(clockid_t clkid, clockid_t sysclk, int readings, > > +int64_t *offset, uint64_t *ts, int64_t *delay); > > Thanks, > Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] util: don't error out in posix_clock_open() if we can't determine PHC index
> -Original Message- > From: Richard Cochran > Sent: Saturday, November 13, 2021 6:53 AM > To: Vladimir Oltean > Cc: Ed Branch ; linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH] util: don't error out in > posix_clock_open() if > we can't determine PHC index > > On Sat, Nov 13, 2021 at 06:22:34AM -0800, Richard Cochran wrote: > > Indeed phc2sys is correct in requiring that the path to the PTP char > > device be in the "/dev/ptpN" format. > > If ever phc2sys accepted /dev/ptp_kvm or similar, that was simply a > bug in phc2sys. > > > The Debian KVM script is just plain wrong. > > Or maybe not a bug, but still the script that invokes phc2sys should > simply use `realpath /dev/ptp_kvm` > Is it possible for us to resolve symlinks ourselves before attempting to check its index value? That would be an added convenience for users. Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [Linuxptp-users] Sudden phc offset jump
On 11/3/2021 7:44 AM, ramesh t wrote: > Hello Jake, > > Please find the requested info below. > NIC driver: > driver: ice > version: 1.3.2 > firmware-version: 2.30 0x80006c8d 1.2877.0 > > Kernel version: > uname -a > Linux cyswy002r-mvnr-p004-sdlas00222a-mv-du001-55d64d7457-dmp8s > 4.19.177-rt72-2.ph3-rt #1-photon SMP PREEMPT RT Fri Mar 5 02:22:24 UTC 2021 > x86_64 GNU/Linux > > Please suggest. > > regards, > Ramesh This looks like you're using the out-of-tree driver we publish on source forge. You could try using a later version (I think we recently published the 1.6.7 driver which was published in September. There has been some work on PTP since the 1.3.x drivers and its quite possible things have improved. Alternatively, you could try a newer kernel which has the PTP support upstream now, since v5.14 Hope this helps, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] fault_reset_interval ASAP seems to ignore link down
If a port goes down because of loss of link, ptp4l tries to avoid
clearing the fault. However, if you set fault_reset_interval to ASAP,
then ptp4l immediately clears the fault and tries to send and eventually
gets a timeout.
Is this expected behavior? I feel like we might want to be checking the
port link state to ensure that we don't clear the fault if link on the
port is still down (since it will still be unusable).
> int port_state_update(struct port *p, enum fsm_event event, int mdiff)
> {
> enum port_state next = p->state_machine(p->state, event, mdiff);
>
> if (PS_FAULTY == next) {
> struct fault_interval i;
> fault_interval(p, last_fault_type(p), );
> if (clear_fault_asap()) {
> pr_notice("%s: clearing fault immediately",
> p->log_name);
> next = p->state_machine(next, EV_FAULT_CLEARED, 0);
> }
For context, I'm looking at this block
> }
>
> if (PS_INITIALIZING == next) {
> /*
> * This is a special case. Since we initialize the
> * port immediately, we can skip right to listening
> * state if all goes well.
> */
> if (port_is_enabled(p)) {
> port_disable(p);
> }
> if (port_initialize(p)) {
> event = EV_FAULT_DETECTED;
> } else {
> event = EV_INIT_COMPLETE;
> }
> next = p->state_machine(next, event, 0);
> }
>
> if (mdiff) {
> unicast_client_state_changed(p);
> }
> if (next != p->state) {
> port_show_transition(p, next, event);
> p->state = next;
> port_notify_event(p, NOTIFY_PORT_STATE);
> unicast_client_state_changed(p);
> return 1;
> }
>
> return 0;
> }
>
the block where we issue a fault cleared normally is:
>
> /*
> * When the fault timer expires we clear the fault,
> * but only if the link is up.
> */
> if (cur[N_POLLFD].revents & (POLLIN|POLLPRI)) {
> clock_fault_timeout(p, 0);
> if (port_link_status_get(p)) {
> port_dispatch(p, EV_FAULT_CLEARED, 0);
> }
> }
>
We check port_link_status_get to determine if we still have link. It
seems like we ought to check this when immediately clearing the fault
too, no?
In the setup I have, link goes down and we were using an immediate fault
clear while attempting to debug issues with Tx timestamps (Yep, i know.
ugh).
Thanks,
Jake
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 0/4] Leap second fix and improvements.
On 9/15/2021 3:08 AM, Miroslav Lichvar wrote: > I was improving the coverage of the linuxptp testsuite and noticed a bug > that needs to be fixed before the next leap second. That's the first > patch. The others are improvements to better see what is happening > around leap second, make the grandmaster flags reliable, and better > handle leap seconds on a PHC synchronized to UTC. > The whole series makes sense to me. Reviewed-by: Jacob Keller Thanks, Jake > Miroslav Lichvar (4): > clock: Accept new UTC offset after leap second. > clock: Print info message when leap flags change. > clock: Clear leap flags after leap second. > clock: Notify servo about leap second on UTC hardware clock. > > clock.c | 75 + > 1 file changed, 60 insertions(+), 15 deletions(-) > ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] Adding Percentile Filter Support
On 9/11/2021 1:51 AM, joseph.matan...@gmail.com wrote: > From: Joseph Matan > > The percentile filter can be very useful when running in a non-ptp-aware > environment. > For example, if we set a low percentile value (and the filter length is large > enough), > we can still get a good estimation of the real delay, > even if our setup is running under heavy network traffic. Ok. > (when setting the percentile value to 0.50, the median filter is a private > use-case of the percentile filter) > What does this part mean? ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] Increase the default tx_timestamp_timeout to 5
> -Original Message- > From: Richard Cochran > Sent: Wednesday, July 14, 2021 6:12 AM > To: Keller, Jacob E > Cc: Miroslav Lichvar ; linuxptp- > de...@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH] Increase the default > tx_timestamp_timeout > to 5 > > On Wed, Jul 14, 2021 at 11:20:00AM +, Keller, Jacob E wrote: > > > I think for Tx the challenges are higher: the timestamp is taken > > after we've filled in the descriptor and sent the frame. The only > > place it could reasonably be stored again is the descriptor > > writeback (since we don't get completion messages). > > Right, the would be the place to do it. > > > If I remember correctly, the challenge here is that in a traditional > > ring model the writeback is completed much earlier than the > > timestamp so we potentially delay cleanup of other packets by > > waiting to insert the timestamp into the writeback. > > If *every* frame gets a time stamp, then their write-backs would all > be delayed by the same amount. Hence no clean up operations would be > "delayed". They would all take the same amount of time. > > The only cost would be in space to keep the data for the write-back > around until the time stamp becomes available. Paying the price of > the little extra memory is well worth it, as it simplifies the time > stamping logic and removes every class of problem related to time > stamp delivery. > > IOW, KISS! > > Thanks, > Richard Yea, if you timestamp every frame regardless of whether kernel requested it or not. Makes sense to me. Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] Increase the default tx_timestamp_timeout to 5
> -Original Message- > From: Richard Cochran > Sent: Wednesday, July 14, 2021 6:03 AM > To: Keller, Jacob E > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH] Increase the default > tx_timestamp_timeout > to 5 > > On Wed, Jul 14, 2021 at 11:20:23AM +, Keller, Jacob E wrote: > > What about at least checking for the case where a timestamp was never > > started? Drivers are supposed to set a flag in the SKB when they start a > > timestamp (and not set it if they can't start it). > > How could that happen? > > Putting aside egregious driver bugs, it is hard for me to imagine a > use case that would cause this failure mode. > > Thanks, > Richard It only matters for hardware which can only handle 1 timestamp at a time, and only in the case where either an application isn't waiting for timestamps, or for when multiple applications try using it at once I guess. Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] Increase the default tx_timestamp_timeout to 5
> -Original Message- > From: Richard Cochran > Sent: Monday, July 12, 2021 6:44 PM > To: Keller, Jacob E > Cc: Miroslav Lichvar ; linuxptp- > de...@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH] Increase the default > tx_timestamp_timeout > to 5 > > On Mon, Jul 12, 2021 at 03:02:50PM +, Keller, Jacob E wrote: > > > Right. Though.. running something like ptp4l on the same connection > > could be problematic if the applications aren't working together > > because most hardware supports a single request at once, > > I wouldn't say "most". Surely some HW is like that, but I never > counted. I always hoped that HW designers would get a clue a simply > provide a time stamp for each and every frame, Tx and Rx, in the > buffer descriptors. No filters, no parsers, just a big on/off switch. > It would be way easier to implement in HW, and it would solve all of > these sorts of problems. > The hardware I've seen at least. (Ofcourse, I suppose I am Intel biased here...) I think there's two issues here: For receive, I think the issue was a belief that the cost in bytes to store the timestamp would be too high. This turns out to either be not true, or not important because the demand for useful timestamps is high enough to account for it. (Newer hardware has opted to simply add timestamping for all frames). I think for Tx the challenges are higher: the timestamp is taken after we've filled in the descriptor and sent the frame. The only place it could reasonably be stored again is the descriptor writeback (since we don't get completion messages). If I remember correctly, the challenge here is that in a traditional ring model the writeback is completed much earlier than the timestamp so we potentially delay cleanup of other packets by waiting to insert the timestamp into the writeback. I'm not sure entirely what all the complexity is here, but I know it's not as simple as the receive side where we already have the timestamp data when filling in the receive descriptor. I imagine if we used a completion model where Tx completions have their own queue it wouldn't be as much of a problem. I don't know for sure though. > > so if both > > applications send a request at the same time one of them will > > fail. They would need to either ensure they're off-sync or be > > communicating to each other about when its ok to timestamp request > > somehow. > > Oh, that will make the users of the new PHC vclock thing happy! I can > already hear the complaints and bug reports here on our lists... > > Thanks, > Richard At least with ice we support up to 64 timestamps outstanding, so it's less of an issue there Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] Increase the default tx_timestamp_timeout to 5
On 7/12/2021 6:36 PM, Richard Cochran wrote: > On Mon, Jul 12, 2021 at 05:02:58PM -0700, Vinicius Costa Gomes wrote: >> Speaking of future improvements, wouldn't it be easier if the >> kernel/driver was able to notify userspace that a timestamping request >> wasn't able to be serviced? > > It would fall to the drivers to implement that correctly. I doubt the > situation would improve. We'd only end up chasing another class of > bugs. > > Thanks, > Richard > What about at least checking for the case where a timestamp was never started? Drivers are supposed to set a flag in the SKB when they start a timestamp (and not set it if they can't start it). This could be done primarily in the core stack to send back an error of a packet had a timestamp request but the request didn't get started? This isn't going to solve the case where a timestamp went missing, of course, but it would solve the case of "I can't start a timestamp while one is already in progress" ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] Increase the default tx_timestamp_timeout to 5
> -Original Message- > From: Miroslav Lichvar > Sent: Monday, July 12, 2021 12:35 AM > To: Keller, Jacob E > Cc: Eric Decker ; linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH] Increase the default > tx_timestamp_timeout > to 5 > > On Thu, Jul 08, 2021 at 07:35:25PM +, Keller, Jacob E wrote: > > > As a future improvement, maybe it could be adaptive, e.g. once in a > > > while try waiting much longer and if that doesn't give a timestamp > > > stick to a shorter interval. That is, try to detect when the hardware > > > is not able to timestamp all packets. > > > > > > > Not sure I follow here. I guess we'd default to a long timeout and > > periodically > try shorter ones? I'm not sure this would be effective. I think the > complexity isn't > really worth it. > > That's another way to look at it. The idea is to estimate something > like the 99th percentile of the delay to maximize the performance > instead of wasting time waiting for a timestamp that is unlikely to > come. The main use case where it could help is multiple applications > doing TX timestamping on the same interface, e.g. a PTP server and > client running in different domains. > > Just an idea for future improvement. > > -- > Miroslav Lichvar Right. Though.. running something like ptp4l on the same connection could be problematic if the applications aren't working together because most hardware supports a single request at once, so if both applications send a request at the same time one of them will fail. They would need to either ensure they're off-sync or be communicating to each other about when its ok to timestamp request somehow. I do like the idea of estimating the 99% percentile over time and adjusting delay Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] Increase the default tx_timestamp_timeout to 5
> -Original Message- > From: Miroslav Lichvar > Sent: Thursday, July 08, 2021 4:10 AM > To: Eric Decker > Cc: Keller, Jacob E ; linuxptp- > de...@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH] Increase the default > tx_timestamp_timeout > to 5 > > On Thu, Jul 08, 2021 at 01:37:38AM +, Eric Decker wrote: > > If the timestamp is available in less than the timeout (5ms) will it still > > wait for > the timeout, or continue processing after the timestamp is received? > > The poll() call is waiting for the descriptor, so it should return as > soon as the timestamp is ready. The option sets the maximum time it > waits. > > I'm ok with increasing the default timeout. > > As a future improvement, maybe it could be adaptive, e.g. once in a > while try waiting much longer and if that doesn't give a timestamp > stick to a shorter interval. That is, try to detect when the hardware > is not able to timestamp all packets. > Not sure I follow here. I guess we'd default to a long timeout and periodically try shorter ones? I'm not sure this would be effective. I think the complexity isn't really worth it. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] Increase the default tx_timestamp_timeout to 5
> -Original Message- > From: Eric Decker > Sent: Wednesday, July 07, 2021 6:38 PM > To: Keller, Jacob E ; linuxptp- > de...@lists.sourceforge.net > Subject: RE: [Linuxptp-devel] [PATCH] Increase the default > tx_timestamp_timeout > to 5 > > If the timestamp is available in less than the timeout (5ms) will it still > wait for the > timeout, or continue processing after the timestamp is received? > > Eric > > It stops waiting as soon as we get a timestamp, (using select/poll). ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] tx_timestamp_timeout default
> -Original Message- > From: Hal Murray > Sent: Wednesday, July 07, 2021 8:48 PM > To: Keller, Jacob E > Cc: Richard Cochran ; linuxptp- > de...@lists.sourceforge.net; Hal Murray > Subject: Re: [Linuxptp-devel] tx_timestamp_timeout default > > > jacob.e.kel...@intel.com said: > > We get into the kthread function within a few hundred usec or less, and then > > the firmware read takes a long time, sometimes over 2 milliseconds. > > Why is it taking so long? > It's not entirely clear. > How long does it take when things go well? Is there anything complicated > going on with a "firmware read"? > We have to send an admin queue command to firmware, which then reads the register, and then replies back. > Is there a software lock, maybe because some other code is using related > hardware resources? Is the thread not getting scheduled? > I captured time using trace_printk in the function after the lock was acquired and we were consistently <200 usec to get to the point of sending the command to firmware. Unfortunately, firmware response time is slow and can take up to a couple msec to respond. On E822 devices it's not as significant problem because we're able to read the PHY register using a separate mechanism that isn't mediated by firmware. I'm not sure how to debug what's going on in firmware. > -- > These are my opinions. I hate spam. > > ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] tx_timestamp_timeout default
> -Original Message- > From: Richard Cochran > Sent: Wednesday, July 07, 2021 6:36 PM > To: Keller, Jacob E > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] tx_timestamp_timeout default > > On Wed, Jul 07, 2021 at 11:46:16PM +, Keller, Jacob E wrote: > > > Either way, I found that whether I used a kthread or not I was > > unable to avoid the timeout issue with ice hardware because the > > delay is caused by the method we must use to access the Tx > > timestamps :( We get into the kthread function within a few hundred > > usec or less, and then the firmware read takes a long time, > > sometimes over 2 milliseconds. > > Yes, but when using "work" that 2 ms can easily become 200 ms or more > on a busy RT system. The work is sched_other, and as such it is > effectively running at the lowest priority WRT the sched_fifo tasks. > > Thanks, > Richard Right, I agree we definitely still need kthreads + priority. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] tx_timestamp_timeout default
> -Original Message- > From: Richard Cochran > Sent: Wednesday, July 07, 2021 4:29 PM > To: Keller, Jacob E > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] tx_timestamp_timeout default > > On Wed, Jul 07, 2021 at 10:22:59PM +, Keller, Jacob E wrote: > > I am wondering if there would be support for either (a) increasing > > the default timeout, or (b) adding something to the PTP get > > capabilities for indicating a sort of default timeout for the > > device, and if it's not set in the config then the default timeout > > is used? > > I wouldn't mind increasing the default. > > I doubt capabilities would help, because much depends on the system > being used. We really should replace "work" with the kthread in the > drivers, and then tell people to give the kthreads sched_fifo using > chrt. > > Thanks, > Richard I implemented this as a kthread in the ice driver, and I am hoping to get some time to fix the other Intel drivers. Note that ice did not use the ptp do_aux_work kthread because of needing to handle multiple ports simultaneously across multiple PFs (the clock is shared across all PFs, so they don't all have access to the do_aux_work function which is only controlled by a single PF which allocated the clock). Either way, I found that whether I used a kthread or not I was unable to avoid the timeout issue with ice hardware because the delay is caused by the method we must use to access the Tx timestamps :( We get into the kthread function within a few hundred usec or less, and then the firmware read takes a long time, sometimes over 2 milliseconds. Ok. I'll propose a patch to increase the default. Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] tx_timestamp_timeout default
Hi, I've been working on implementing support for PTP in the E810 and E822 devices for Intel's ice driver. As part of this work we've discovered that (again...) timestamps can sometimes take longer than the 1millisecond default delay. Although we get an interrupt within 10usec, the timestamp has to be retrieved from a PHY register, and this access is unfortunately mediated by firmware. The result is that while we can sometimes recover the timestamp within the 1000usec default delay, we sometimes need as much as 2000 or 2500 usec. We've been recommending that people default to using tx_timestamp_timeout of 5 or 10 to avoid issues. I am wondering if there would be support for either (a) increasing the default timeout, or (b) adding something to the PTP get capabilities for indicating a sort of default timeout for the device, and if it's not set in the config then the default timeout is used? The reason for this need is because it is confusing for users to be immediately prompted with "this is likely a driver bug" messaging. We know that the only resolution is to wait slightly longer. This is also not the first time we've had issues with the 1msec delay being too short, I believe we've often had to recommend increasing the delay for other Intel parts in the past for similar reasons. Note I tried a number of changes to the driver implementation in order to see if I could reduce the delay, and although I was able to improve it, I was never able to get it consistently below 1000usec, as we're effectively limited by the need to read the register access over the firmware :( Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] Intel 210 to Intel 255
> -Original Message- > From: Richard Cochran > Sent: Monday, June 07, 2021 11:42 AM > To: Geva, Erez > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] Intel 210 to Intel 255 > > On Mon, Jun 07, 2021 at 02:01:28PM +, Geva, Erez wrote: > > > Jun 7 15:44:07 ipc01 ptp4l: [673.869] timed out while polling for tx > > timestamp > > Jun 7 15:44:07 ipc01 ptp4l: [673.869] increasing tx_timestamp_timeout may > correct this issue, but it is likely caused by a driver bug > > Try --tx_timestamp_timeout=100 > > HTH, > Richard > I would recommend this as well, even for the i210. At least to rule out whether or not the issue still exists if you extend the timeout. The timestamp is captured via interrupt, but I believe the igb driver might still rely on a work thread to actually finish reporting the timestamp. Even if it doesn't, there is some delay before the device gets notified of the timestamp, as well as further delay caused by reading the timestamp register. While it should still take less than the 1millisecond of time, I definitely recall cases where it took long enough to exceed the time limit in the worst case. Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v3 5/5] clockcheck: Increase minimum interval.
> -Original Message-
> From: Miroslav Lichvar
> Sent: Monday, May 31, 2021 2:08 AM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH v3 5/5] clockcheck: Increase minimum
> interval.
>
> Increase the minimum check interval to 1 second to measure the frequency
> offset more accurately and with default configuration make false
> positives less likely due to a heavily overloaded system.
>
> Signed-off-by: Miroslav Lichvar
So this is a change from 10 times a second to once a second? Seems ok to me.
Reviewed-by: Jacob Keller
Thanks,
Jake
> ---
> clockcheck.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/clockcheck.c b/clockcheck.c
> index d0b4714..f0141be 100644
> --- a/clockcheck.c
> +++ b/clockcheck.c
> @@ -23,7 +23,7 @@
> #include "clockcheck.h"
> #include "print.h"
>
> -#define CHECK_MIN_INTERVAL 1
> +#define CHECK_MIN_INTERVAL 10
> #define CHECK_MAX_FREQ 9
>
> struct clockcheck {
> --
> 2.26.3
>
>
>
> ___
> 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
Re: [Linuxptp-devel] [PATCH v3 4/5] port: Don't renew raw transport.
> -Original Message-
> From: Miroslav Lichvar
> Sent: Monday, May 31, 2021 2:08 AM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH v3 4/5] port: Don't renew raw transport.
>
> Renewing of the transport on announce/sync timeout is needed in the
> client-only mode to avoid getting stuck with a broken multicast socket
> when the link goes down.
>
> This shouldn't be necessary with the raw transport. Closing and binding
> of raw sockets can apparently be so slow that it triggers a false
> positive in the clock check.
>
> Reported-by: Amar Subramanyam
> Signed-off-by: Miroslav Lichvar
> ---
Makes sense.
Reviewed-by: Jacob Keller
> port.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/port.c b/port.c
> index fb420fb..6bf0684 100644
> --- a/port.c
> +++ b/port.c
> @@ -1806,6 +1806,12 @@ static int port_renew_transport(struct port *p)
> if (!port_is_enabled(p)) {
> return 0;
> }
> +
> + /* Closing and binding of raw sockets is too slow and unnecessary */
> + if (transport_type(p->trp) == TRANS_IEEE_802_3) {
> + return 0;
> + }
> +
> transport_close(p->trp, >fda);
> port_clear_fda(p, FD_FIRST_TIMER);
> res = transport_open(p->trp, p->iface, >fda, p->timestamping);
> --
> 2.26.3
>
>
>
> ___
> 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
Re: [Linuxptp-devel] [PATCH v2 2/3] clock: Reset clock check on best clock/port change.
> -Original Message-
> From: Miroslav Lichvar
> Sent: Wednesday, May 26, 2021 1:24 AM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH v2 2/3] clock: Reset clock check on best
> clock/port change.
>
> Reset the clock check when the best clock or port changes, together with
> the other state like current estimated delay and frequency. This avoids
> false positives if the clock is controlled by an external process when
> not synchronized by PTP (e.g. phc2sys -rr).
>
Thanks! The intent is more clear now!
Reviewed-by: Jacob Keller
> Signed-off-by: Miroslav Lichvar
> ---
> clock.c | 2 ++
> clockcheck.c | 9 -
> clockcheck.h | 6 ++
> 3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/clock.c b/clock.c
> index a073575..2dd7ef9 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -1949,6 +1949,8 @@ static void handle_state_decision_event(struct clock
> *c)
>
> if (!cid_eq(_id, >best_id) || best != c->best) {
> clock_freq_est_reset(c);
> + if (c->sanity_check)
> + clockcheck_reset(c->sanity_check);
> tsproc_reset(c->tsproc, 1);
> if (!tmv_is_zero(c->initial_delay))
> tsproc_set_delay(c->tsproc, c->initial_delay);
> diff --git a/clockcheck.c b/clockcheck.c
> index d48a578..d0b4714 100644
> --- a/clockcheck.c
> +++ b/clockcheck.c
> @@ -47,9 +47,16 @@ struct clockcheck *clockcheck_create(int freq_limit)
> if (!cc)
> return NULL;
> cc->freq_limit = freq_limit;
> + clockcheck_reset(cc);
> + return cc;
> +}
> +
> +void clockcheck_reset(struct clockcheck *cc)
> +{
> + cc->freq_known = 0;
> cc->max_freq = -CHECK_MAX_FREQ;
> cc->min_freq = CHECK_MAX_FREQ;
> - return cc;
> + cc->last_ts = 0;
> }
>
> int clockcheck_sample(struct clockcheck *cc, uint64_t ts)
> diff --git a/clockcheck.h b/clockcheck.h
> index 78aca48..1ff86eb 100644
> --- a/clockcheck.h
> +++ b/clockcheck.h
> @@ -33,6 +33,12 @@ struct clockcheck;
> */
> struct clockcheck *clockcheck_create(int freq_limit);
>
> +/**
> + * Reset a clock check.
> + * @param cc Pointer to a clock check obtained via @ref clockcheck_create().
> + */
> +void clockcheck_reset(struct clockcheck *cc);
> +
> /**
> * Perform the sanity check on a time stamp.
> * @param cc Pointer to a clock check obtained via @ref clockcheck_create().
> --
> 2.26.3
>
>
>
> ___
> 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
Re: [Linuxptp-devel] [PATCH v2 3/3] port: Don't check timestamps from non-slave ports.
> -Original Message-
> From: Miroslav Lichvar
> Sent: Wednesday, May 26, 2021 1:24 AM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH v2 3/3] port: Don't check timestamps from
> non-
> slave ports.
>
> Don't perform the sanity check on receive timestamps from ports in
> non-slave states to avoid false positives in the jbod mode, where
> the timestamps can be generated by different clocks.
>
Makes sense.
Reviewed-by: Jacob Keller
> Reported-by: Amar Subramanyam
> Signed-off-by: Miroslav Lichvar
> ---
> port.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/port.c b/port.c
> index 10bb9e1..fb420fb 100644
> --- a/port.c
> +++ b/port.c
> @@ -2744,7 +2744,10 @@ static enum fsm_event bc_event(struct port *p, int
> fd_index)
> }
> if (msg_sots_valid(msg)) {
> ts_add(>hwts.ts, -p->rx_timestamp_offset);
> - clock_check_ts(p->clock, tmv_to_nanoseconds(msg->hwts.ts));
> + if (p->state == PS_SLAVE) {
> + clock_check_ts(p->clock,
> +tmv_to_nanoseconds(msg->hwts.ts));
> + }
> }
>
> switch (msg_type(msg)) {
> --
> 2.26.3
>
>
>
> ___
> 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
Re: [Linuxptp-devel] [PATCH 2/3] clock: Reset clock check on port state change.
> -Original Message-
> From: Miroslav Lichvar
> Sent: Tuesday, May 25, 2021 5:28 AM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH 2/3] clock: Reset clock check on port state
> change.
>
> Reset the clock check to avoid false positives when switching between
> slave and non-slave state and the clock is controlled by an external
> process (e.g. phc2sys -rr).
>
The way this is worded, I expected clockcheck_reset to only be called if the
clock is controlled by an external process, but we seem to call this every time.
> Signed-off-by: Miroslav Lichvar
> ---
> clock.c | 1 +
> clockcheck.c | 9 -
> clockcheck.h | 6 ++
> 3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/clock.c b/clock.c
> index a073575..69f2b66 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -1949,6 +1949,7 @@ static void handle_state_decision_event(struct clock
> *c)
>
> if (!cid_eq(_id, >best_id) || best != c->best) {
> clock_freq_est_reset(c);
> + clockcheck_reset(c->sanity_check);
> tsproc_reset(c->tsproc, 1);
> if (!tmv_is_zero(c->initial_delay))
> tsproc_set_delay(c->tsproc, c->initial_delay);
> diff --git a/clockcheck.c b/clockcheck.c
> index d48a578..d0b4714 100644
> --- a/clockcheck.c
> +++ b/clockcheck.c
> @@ -47,9 +47,16 @@ struct clockcheck *clockcheck_create(int freq_limit)
> if (!cc)
> return NULL;
> cc->freq_limit = freq_limit;
> + clockcheck_reset(cc);
> + return cc;
> +}
> +
> +void clockcheck_reset(struct clockcheck *cc)
> +{
> + cc->freq_known = 0;
> cc->max_freq = -CHECK_MAX_FREQ;
> cc->min_freq = CHECK_MAX_FREQ;
> - return cc;
> + cc->last_ts = 0;
> }
>
> int clockcheck_sample(struct clockcheck *cc, uint64_t ts)
> diff --git a/clockcheck.h b/clockcheck.h
> index 78aca48..1ff86eb 100644
> --- a/clockcheck.h
> +++ b/clockcheck.h
> @@ -33,6 +33,12 @@ struct clockcheck;
> */
> struct clockcheck *clockcheck_create(int freq_limit);
>
> +/**
> + * Reset a clock check.
> + * @param cc Pointer to a clock check obtained via @ref clockcheck_create().
> + */
> +void clockcheck_reset(struct clockcheck *cc);
> +
> /**
> * Perform the sanity check on a time stamp.
> * @param cc Pointer to a clock check obtained via @ref clockcheck_create().
> --
> 2.26.3
>
>
>
> ___
> 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
Re: [Linuxptp-devel] [PATCH 1/3] clock: Reset state when switching port with same best clock.
> -Original Message-
> From: Miroslav Lichvar
> Sent: Tuesday, May 25, 2021 5:28 AM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH 1/3] clock: Reset state when switching port
> with
> same best clock.
>
> When the best port is changed, but the ID of the best clock doesn't
> change (e.g. a passive port is activated on link failure), reset the
> current delay and other master/link-specific state to avoid the switch
> throwing the clock off.
>
Right. Since we are on a new port, we are going to have different
characteristics, even if the clock is still the same.
Makes sense.
Reviewed-by: Jacob Keller
> Signed-off-by: Miroslav Lichvar
> ---
> clock.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/clock.c b/clock.c
> index e545a9b..a073575 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -1947,7 +1947,7 @@ static void handle_state_decision_event(struct clock
> *c)
> cid2str(_id));
> }
>
> - if (!cid_eq(_id, >best_id)) {
> + if (!cid_eq(_id, >best_id) || best != c->best) {
> clock_freq_est_reset(c);
> tsproc_reset(c->tsproc, 1);
> if (!tmv_is_zero(c->initial_delay))
> --
> 2.26.3
>
>
>
> ___
> 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
Re: [Linuxptp-devel] [PATCH 3/4] ts2phc: Update leapfile documentation
> -Original Message- > From: Lars Munch > Sent: Friday, May 14, 2021 4:34 AM > To: linuxptp-devel@lists.sourceforge.net > Subject: [Linuxptp-devel] [PATCH 3/4] ts2phc: Update leapfile documentation > > Update leapfile documentation to note the file will be > reloaded if modified. > > Signed-off-by: Lars Munch > --- > ts2phc.8 | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/ts2phc.8 b/ts2phc.8 > index 0bd523d..99067c5 100644 > --- a/ts2phc.8 > +++ b/ts2phc.8 > @@ -117,12 +117,12 @@ how well synchronized a group of local clocks are to > each other. > The default is 0 (adjust the clocks). > .TP > .B leapfile > -The path to the current leap seconds definition file. > -In a Debian system this file is provided by the tzdata package and can > -be found at /usr/share/zoneinfo/leap-seconds.list. > -The default is an empty string, which causes the program to use a hard > -coded table that reflects the known leap seconds on the date of the > -software's release. > +The path to the current leap seconds definition file. In a Debian > +system this file is provided by the tzdata package and can be found at > +/usr/share/zoneinfo/leap-seconds.list. If a leapfile is configured it > +will be reloaded if modified. The default is an empty string, which > +causes the program to use a hard coded table that reflects the known > +leap seconds on the date of the software's release. > .TP ptp4l monitors and detects when it has changed? > .B logging_level > The maximum logging level of messages which should be printed. > -- > 2.25.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
Re: [Linuxptp-devel] ptp4l L2 with rx-fcs
> -Original Message- > From: Aya Levin > Sent: Thursday, May 13, 2021 4:37 AM > To: linuxptp-devel@lists.sourceforge.net > Subject: [Linuxptp-devel] ptp4l L2 with rx-fcs > > Hi, > > While debugging the same issue, I found this thread > https://sourceforge.net/p/linuxptp/mailman/linuxptp- > users/thread/1382880293.15328.YahooMailBasic%40web161603.mail.bf1.yahoo. > com/#msg31567827 > > I also see that ptp4l fails when using the L2 option AND rx-fcs is set > (ethtool feature is specifically set). > > From documentation about rx-fcs: > This requests that the NIC append the Ethernet Frame Checksum (FCS) > to the end of the skb data. This allows sniffers and other tools to > read the CRC recorded by the NIC on receipt of the packet. > Documentation/networking/netdev-features.rst > It sure seems like this is guaranteed to cause problems for any software that processes the SKBs.. the FCS is now in the data section so it will be treated as the last 4 bytes of the data... I don't see how you could expect any different behavior? > So having these extra 4 bytes is expected. But still ptp4l regard this > suffix as TLVs. Since both (fcs and tlv) are appended at the end of the > packet, is there a way which is which? > > Was there any progress in that direction? Is this a known issue? > > Thanks in advanced, > Aya > > > ___ > 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
Re: [Linuxptp-devel] SyncE support
> -Original Message- > From: Miroslav Lichvar > Sent: Tuesday, March 16, 2021 8:59 AM > To: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] SyncE support > > On Tue, Mar 16, 2021 at 03:52:36PM +0100, Frantisek Rysanek wrote: > > On 16 Mar 2021 at 11:25, Miroslav Lichvar wrote: > > > > > > In the Intel's igc driver I saw few SYNCE registers defined, > > > but no code using them. > > > > > Whoa. igc ? oh there's an i225... didn't know about this one, thanks > > for the pointer, this definitely looks promising :-) > > Looks like a successor to the i210 generation. > > I'm sorry for misleading you. I meant the ice driver (speeds up to > 100Gb). I didn't see an out-of-tree version of the igc driver. Yep, the ice hardware is supposed to have SyncE support, though no software here yet :( Not sure about igc. > > The I225 might be an interesting NIC for experiments for a different > reason. It seems it supports the PCIe Precision Time Measurement (PTM) > feature. It is a hardware implementation of an NTP-like protocol over > PCIe, which should allow a highly accurate synchronization of the > system clock, avoiding the asymmetry on PCIe. It is not supported in > the igc driver yet, but there were some patches submitted for it. > > I measured the PCIe asymmetry with the I210 on few different > boards+CPUs and it changed a lot (between about -100ns and 100ns), so > I think PTM could make a significant improvement. > igc should support PTM, but I'm not sure why the patches haven't landed. I'm not very well connected to the igc team. > -- > Miroslav Lichvar > > > > ___ > 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
Re: [Linuxptp-devel] SyncE support
> -Original Message- > From: Frantisek Rysanek > Sent: Monday, March 15, 2021 2:40 PM > To: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] SyncE support > > Dear gentlemen, > thanks for opening this interesting topic... > > Is there any stock NIC (silicon, board), with an open-source Linux > driver, that would support SyncE off the shelf in the NIC hardware? > I believe that SyncE is supported by the E800 series 100GB hardware from Intel, but there's no existing driver/software support. > Other than that... SyncE sounds like a pretty self-contained L1 > feature, at least considering just a single NIC port. Obviously the > routing of clock signals among possibly several ports in a system and > a local master oscillator - that would be a more complicated, > proprietary story. > But at the level of a single port... what do you need? > SyncE enable / disable, and select master vs. slave role? > It would take a fairly simple extension to some existing API - makes > me wonder which one: MII, Ethtool, Netlink, or maybe the timestamping > API? I'd guess Ethtool or Netlink would be the right level of > abstraction. Or some nodes in procfs or sysfs :-) > MII is too HW-specific and the timestamping stuff operating on top of > setsockopt() feels a little too detached from hardware. > > And yes there is some additional messaging, I understand in-band in > Ethernet, which would probably have to be handled by ptp4l software, > unless offloaded to the kernel-space driver or hardware... > > Frank Rysanek > > > ___ > 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
Re: [Linuxptp-devel] SyncE support
> -Original Message- > From: Miroslav Lichvar > Sent: Monday, March 15, 2021 9:57 AM > To: linuxptp-devel@lists.sourceforge.net > Subject: [Linuxptp-devel] SyncE support > > As I understand it, there is currently no explicit support for SyncE > in the mainline kernel and linuxptp. For example, there is no access > to the Synchronization Status Message (SSM), so ptp4l cannot know if > SyncE is active on a port. > Right, this appears to be the general case today > IIRC some people use SyncE with linuxptp by disabling the frequency > control. I assume they set up everything manually with some 3rd party > HW-specific tools using custom ioctls, or maybe it's just a > client-only use case for linuxptp. > I think the only examples I've seen were client only... > There seems to be a growing interest for SyncE, but it's not clear to > me how the functionality needed for controlling and monitoring SyncE > is supposed to be split between the firmware, driver, kernel, and > ptp4l. Right. I've heard a few folks here talking about it, but I don't know the plans off hand. Given that some parts of it are device specific, it would seem to me like we need some extensions of the PTP interface that drivers which support SyncE hardware could implement, and then we could extend the LinuxPTP stack to include monitoring synce, perhaps as part of ptp4l itself or as part of a separate application? > > In 1588-2019 there is an L1_SYNC TLV specified. Does anyone know what > is actually needed for the kernel and linuxptp to support it properly, > e.g. on a boundary clock? > > Any pointers? > > -- > Miroslav Lichvar > > > > ___ > 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
Re: [Linuxptp-devel] [PATCH 1/2] Explicit length byte order swap functions.
> -Original Message- > From: Geva, Erez > Sent: Thursday, March 11, 2021 2:23 AM > To: Richard Cochran > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH 1/2] Explicit length byte order swap > functions. > > How do you want to call the 64 bits? > I think that naming should be consistence. > > Erez I think the usual scheme is "htonll" or "ntohll" for "long long". Thanks, Jake > > -Original Message- > From: Richard Cochran > Sent: Thursday, 11 March 2021 04:12 > To: Geva, Erez (ext) (DI PA DCP R 3) > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH 1/2] Explicit length byte order swap > functions. > > On Wed, Mar 10, 2021 at 11:17:48PM +0100, Erez Geva wrote: > > Replace byte order with explicit length. > > > > Add function for byte order for 64 bits. > > > > Signed-off-by: Erez Geva > > --- > > clock.c | 4 +- > > msg.c | 51 ++--- > > nsm.c | 2 +- > > port.c | 5 +- > > raw.c | 10 +-- > > tc.c| 8 +- > > tlv.c | 207 ++-- > > transport.c | 9 ++- > > udp.c | 6 +- > > udp6.c | 4 +- > > util.h | 67 + > > 11 files changed, 221 insertions(+), 152 deletions(-) > > > > diff --git a/clock.c b/clock.c > > index 7005636..5b3b4d0 100644 > > --- a/clock.c > > +++ b/clock.c > > @@ -255,12 +255,12 @@ void clock_send_notification(struct clock *c, struct > ptp_message *msg, > > if (!event_bitmask_get(s->events, event)) > > continue; > > /* send event */ > > - msg->header.sequenceId = htons(s->sequenceId); > > + msg->header.sequenceId = hton16(s->sequenceId); > > I really don't see any improvement here. > > Sorry, > Richard > > > ___ > 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
Re: [Linuxptp-devel] [PATCH 1/2] Explicit length byte order swap functions.
> -Original Message- > From: Richard Cochran > Sent: Wednesday, March 10, 2021 7:12 PM > To: Erez Geva > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH 1/2] Explicit length byte order swap > functions. > > On Wed, Mar 10, 2021 at 11:17:48PM +0100, Erez Geva wrote: > > Replace byte order with explicit length. > > > > Add function for byte order for 64 bits. > > > > Signed-off-by: Erez Geva > > --- > > clock.c | 4 +- > > msg.c | 51 ++--- > > nsm.c | 2 +- > > port.c | 5 +- > > raw.c | 10 +-- > > tc.c| 8 +- > > tlv.c | 207 ++-- > > transport.c | 9 ++- > > udp.c | 6 +- > > udp6.c | 4 +- > > util.h | 67 + > > 11 files changed, 221 insertions(+), 152 deletions(-) > > > > diff --git a/clock.c b/clock.c > > index 7005636..5b3b4d0 100644 > > --- a/clock.c > > +++ b/clock.c > > @@ -255,12 +255,12 @@ void clock_send_notification(struct clock *c, struct > ptp_message *msg, > > if (!event_bitmask_get(s->events, event)) > > continue; > > /* send event */ > > - msg->header.sequenceId = htons(s->sequenceId); > > + msg->header.sequenceId = hton16(s->sequenceId); > > I really don't see any improvement here. > > Sorry, > Richard > I'm personally a fan of the bit length being in the name myself, but I don't think it's worth churn for no other reason. I also generally prefer "cpu_to_le16" or "cpu_to_be16" instead of hotn.. but again not worth changing just to rename. Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/2] Fix Management Implementation-specific TLVs
> -Original Message-
> From: Erez Geva
> Sent: Wednesday, March 10, 2021 2:18 PM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH 2/2] Fix Management Implementation-specific
> TLVs
>
> Change Management Implementation-specific TLVs use to
> little endian instead of using host order.
>
> Although IEEE use network order, as most of us use little endian
> hardware and to retain backward compatible with most of us,
> we decide to use little endian.
>
> Signed-off-by: Erez Geva
If we were worried, we could introduce new ops for this, but I think the
fallout is low.
Thanks,
Jake
> ---
> clock.c | 29 +
> pmc.c | 80 +
> port.c | 13 +++---
> 3 files changed, 74 insertions(+), 48 deletions(-)
>
> diff --git a/clock.c b/clock.c
> index 5b3b4d0..90f3c77 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -423,25 +423,33 @@ static int clock_management_fill_response(struct clock
> *c, struct port *p,
> break;
> case TLV_TIME_STATUS_NP:
> tsn = (struct time_status_np *) tlv->data;
> - tsn->master_offset = tmv_to_nanoseconds(c->master_offset);
> - tsn->ingress_time = tmv_to_nanoseconds(c->ingress_ts);
> - tsn->cumulativeScaledRateOffset =
> + tsn->master_offset = htole64(tmv_to_nanoseconds(c-
> >master_offset));
> + tsn->ingress_time = htole64(tmv_to_nanoseconds(c-
> >ingress_ts));
> + tsn->cumulativeScaledRateOffset = htole32(
> (Integer32) (c->status.cumulativeScaledRateOffset +
> - c->nrr * POW2_41 - POW2_41);
> - tsn->scaledLastGmPhaseChange = c-
> >status.scaledLastGmPhaseChange;
> - tsn->gmTimeBaseIndicator = c->status.gmTimeBaseIndicator;
> - tsn->lastGmPhaseChange = c->status.lastGmPhaseChange;
> + c->nrr * POW2_41 - POW2_41));
> + tsn->scaledLastGmPhaseChange = htole32(c-
> >status.scaledLastGmPhaseChange);
> + tsn->gmTimeBaseIndicator = htole16(c-
> >status.gmTimeBaseIndicator);
> + tsn->lastGmPhaseChange.nanoseconds_msb =
> + htole16(c-
> >status.lastGmPhaseChange.nanoseconds_msb);
> + tsn->lastGmPhaseChange.nanoseconds_lsb =
> + htole64(c->status.lastGmPhaseChange.nanoseconds_lsb);
> + tsn->lastGmPhaseChange.fractional_nanoseconds =
> + htole16(c-
> >status.lastGmPhaseChange.fractional_nanoseconds);
> if (cid_eq(>dad.pds.grandmasterIdentity,
> >dds.clockIdentity))
> tsn->gmPresent = 0;
> else
> - tsn->gmPresent = 1;
> + tsn->gmPresent = htole32(1);
> tsn->gmIdentity = c->dad.pds.grandmasterIdentity;
> datalen = sizeof(*tsn);
> break;
> case TLV_GRANDMASTER_SETTINGS_NP:
> gsn = (struct grandmaster_settings_np *) tlv->data;
> - gsn->clockQuality = c->dds.clockQuality;
> - gsn->utc_offset = c->utc_offset;
> + gsn->clockQuality.clockClass = c->dds.clockQuality.clockClass;
> + gsn->clockQuality.clockAccuracy = c-
> >dds.clockQuality.clockAccuracy;
> + gsn->clockQuality.offsetScaledLogVariance =
> + htole16(c->dds.clockQuality.offsetScaledLogVariance);
> + gsn->utc_offset = htole16(c->utc_offset);
> gsn->time_flags = c->time_flags;
> gsn->time_source = c->time_source;
> datalen = sizeof(*gsn);
> @@ -453,6 +461,7 @@ static int clock_management_fill_response(struct clock
> *c, struct port *p,
> }
> sen = (struct subscribe_events_np *)tlv->data;
> clock_get_subscription(c, req, sen->bitmask, >duration);
> + sen->duration = htole16(sen->duration);
> datalen = sizeof(*sen);
> break;
> case TLV_SYNCHRONIZATION_UNCERTAIN_NP:
> diff --git a/pmc.c b/pmc.c
> index 1e569b5..6d5d35c 100644
> --- a/pmc.c
> +++ b/pmc.c
> @@ -137,6 +137,17 @@ static void pmc_show_signaling(struct ptp_message
> *msg, FILE *fp)
> fflush(fp);
> }
>
> +static inline uint64_t getStat(struct port_stats_np *pcp, bool rx, int index)
> +{
> + uint64_t ret;
> +
> + if (rx)
> + ret = pcp->stats.rxMsgType[index];
> + else
> + ret = pcp->stats.txMsgType[index];
> + return le64toh(ret);
> +}
> +
> static void pmc_show(struct ptp_message *msg, FILE *fp)
> {
> struct grandmaster_settings_np *gsn;
> @@ -346,15 +357,15 @@ static void pmc_show(struct ptp_message *msg, FILE
> *fp)
> IFMT "lastGmPhaseChange 0x%04hx'%016" PRIx64
> ".%04hx"
> IFMT "gmPresent %s"
> IFMT "gmIdentity
Re: [Linuxptp-devel] PORT_STATS_NP management TLV message
> -Original Message- > From: Richard Cochran > Sent: Wednesday, March 10, 2021 6:58 AM > To: Geva, Erez > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] PORT_STATS_NP management TLV message > > On Tue, Mar 09, 2021 at 10:28:48PM +, Geva, Erez wrote: > > However with the current code the endian depends on machine. > > So a linuxptp daemon that runs on a big endian (there are yet some) will > > send > the message in big endian. > > Yeah, so at the very minimum, we should: > > 1. document the oversight in the source code, and > > 2. add le64_to_cpu/cpu_to_le64 in the post_recv/pre_send paths. > > Thanks, > Richard > Can we treat it as a bug fix? Or should we treat it as an oversight that requires a new op? Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] PORT_STATS_NP management TLV message
> -Original Message- > From: Richard Cochran > Sent: Wednesday, March 10, 2021 6:54 AM > To: Keller, Jacob E > Cc: Geva, Erez ; linuxptp- > de...@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] PORT_STATS_NP management TLV message > > On Tue, Mar 09, 2021 at 11:34:01PM +, Keller, Jacob E wrote: > > Can this message be sent or processed over the network? Or do the _NP > messages always get restricted to the local socket only? > > Network, too. Ok. Then I think the best solution is to introduce a strict version which enforces either little or big endian (we can pick) and then deprecate this version. While at it we should go through all the existing _NP commands and ensure they're specific as well. If you can access this over the network there is no guarantee that the other end is the same endianness as you. Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] PORT_STATS_NP management TLV message
> -Original Message- > From: Geva, Erez > Sent: Tuesday, March 09, 2021 2:29 PM > To: Richard Cochran > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] PORT_STATS_NP management TLV message > > Hi, > > I can be live with using little endian. > This is not the first time I see using little endian with IEEE. > > However with the current code the endian depends on machine. > So a linuxptp daemon that runs on a big endian (there are yet some) will send > the > message in big endian. > > May be we should limit this TLV message to UDS only? > Since UDS always run on the same machine. > > Erez > If it only runs on the same machine there should be no issue. Even if it crosses network I think just documenting that the format is in "host" endian and adding a configuration option to pmc tool that allows you to set the format to read the command output in would be enough? Alternatively we can add a new PORTS_STATS_STRICT_NP or something which would strictly use either Big or Little endian format, and then work on deprecating the old format... Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] PORT_STATS_NP management TLV message
> -Original Message- > From: Richard Cochran > Sent: Tuesday, March 09, 2021 8:57 AM > To: Geva, Erez > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] PORT_STATS_NP management TLV message > > On Tue, Mar 09, 2021 at 01:59:33PM +, Geva, Erez wrote: > > As I add the PORT_STATS_NP message, > > I notice that the statistics is stored in unsigned integer 64 bits values, > > but using > host order instead of network order. > > Oops, that was an oversight. > > > As I know you follow the IEEE standard, > > It would be nice if you could elaborate on this message. > > This message is a custom message, and it is not covered by IEEE 1588 > or any other standard. > > > Can we fix it and rebase it to network order? > > Unfortunately, simply "fixing" it unconditionally will break existing > deployments. > > > May be add a flag for backward compatible. > > If we go with that, then the default value of "compatible" will have > to be "true". > > > What do you think? > > Maybe it is better to simply leave it as is. I myself don't use this > TLV. Since the pmc tool prints the correct values, probably nobody > would insist on having big endian in the on-the-wire format. > Can this message be sent or processed over the network? Or do the _NP messages always get restricted to the local socket only? Thanks, Jake > Thanks, > Richard > > > ___ > 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
Re: [Linuxptp-devel] [PATCH 2/2] Clock Class Threshold Feature addition for PTP4L
> -Original Message- > From: Richard Cochran > Sent: Tuesday, February 16, 2021 8:21 PM > To: Keller, Jacob E > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH 2/2] Clock Class Threshold Feature > addition > for PTP4L > > On Tue, Feb 16, 2021 at 02:08:44PM -0800, Jacob Keller wrote: > > On 2/14/2021 9:59 PM, Karthikkumar V wrote: > > > - c->clock_class_threshold = config_get_int(config, NULL, > "clockClassThreshold"); > > > + c->clock_class_threshold = config_get_int(config, NULL, > "clock_class_threshold"); > > > > > > > > > Why this change? It seems weird to remove this variable and introduce > > another? clockClassThreshold is supposed to be the standard isn't it? > > I think Karthikkumar made a delta patch to the previous one. > > (and no, there is no clockClassThreshold in 1588) > Ok, that makes more sense, thanks! > Thanks, > Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH] clock: Add read-only UDS port for monitoring.
> -Original Message-
> From: Miroslav Lichvar
> Sent: Wednesday, January 20, 2021 7:15 AM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [RFC PATCH] clock: Add read-only UDS port for
> monitoring.
>
> Add a second UDS port to allow unprivileged applications to monitor
> ptp4l. On this "read-only" port, disable non-GET actions, forwarding,
> and access to subscriptions.
>
It makes sense to remove forwarding, but I am not sure I understand the
justification for removing access to subscriptions.. if the subscription is for
read only data, why doesn't it make sense to allow that over the read only
socket?
Thanks,
Jake
> Ignore non-management messages on both UDS ports to prevent them from
> changing the clock or port state. (This should be a separate patch?)
>
> TODO:
> - add option to configure the UDS address (default /var/run/ptp4ro?)
> - chmod(0666) ?
> - update man page
>
> Signed-off-by: Miroslav Lichvar
> ---
> clock.c | 122 +++-
> port.c | 3 ++
> 2 files changed, 89 insertions(+), 36 deletions(-)
>
> diff --git a/clock.c b/clock.c
> index 08c61eb..475aa3d 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -95,10 +95,11 @@ struct clock {
> struct foreign_clock *best;
> struct ClockIdentity best_id;
> LIST_HEAD(ports_head, port) ports;
> - struct port *uds_port;
> + struct port *uds_rw_port;
> + struct port *uds_ro_port;
> struct pollfd *pollfd;
> int pollfd_valid;
> - int nports; /* does not include the UDS port */
> + int nports; /* does not include the two UDS ports */
> int last_port_number;
> int sde;
> int free_running;
> @@ -129,7 +130,8 @@ struct clock {
> struct clock_stats stats;
> int stats_interval;
> struct clockcheck *sanity_check;
> - struct interface *udsif;
> + struct interface *uds_rw_if;
> + struct interface *uds_ro_if;
> LIST_HEAD(clock_subscribers_head, clock_subscriber) subscribers;
> struct monitor *slave_event_monitor;
> };
> @@ -245,7 +247,7 @@ void clock_send_notification(struct clock *c, struct
> ptp_message *msg,
> {
> unsigned int event_pos = event / 8;
> uint8_t mask = 1 << (event % 8);
> - struct port *uds = c->uds_port;
> + struct port *uds = c->uds_rw_port;
> struct clock_subscriber *s;
>
> LIST_FOREACH(s, >subscribers, list) {
> @@ -267,13 +269,15 @@ void clock_destroy(struct clock *c)
> {
> struct port *p, *tmp;
>
> - interface_destroy(c->udsif);
> + interface_destroy(c->uds_rw_if);
> + interface_destroy(c->uds_ro_if);
> clock_flush_subscriptions(c);
> LIST_FOREACH_SAFE(p, >ports, list, tmp) {
> clock_remove_port(c, p);
> }
> monitor_destroy(c->slave_event_monitor);
> - port_close(c->uds_port);
> + port_close(c->uds_rw_port);
> + port_close(c->uds_ro_port);
> free(c->pollfd);
> if (c->clkid != CLOCK_REALTIME) {
> phc_close(c->clkid);
> @@ -442,8 +446,8 @@ static int clock_management_fill_response(struct clock
> *c, struct port *p,
> datalen = sizeof(*gsn);
> break;
> case TLV_SUBSCRIBE_EVENTS_NP:
> - if (p != c->uds_port) {
> - /* Only the UDS port allowed. */
> + if (p != c->uds_rw_port) {
> + /* Only the UDS RW port allowed. */
> break;
> }
> sen = (struct subscribe_events_np *)tlv->data;
> @@ -774,6 +778,10 @@ static int clock_utc_correct(struct clock *c, tmv_t
> ingress)
> static int forwarding(struct clock *c, struct port *p)
> {
> enum port_state ps = port_state(p);
> +
> + if (p == c->uds_ro_port)
> + return 0;
> +
> switch (ps) {
> case PS_MASTER:
> case PS_GRAND_MASTER:
> @@ -784,7 +792,7 @@ static int forwarding(struct clock *c, struct port *p)
> default:
> break;
> }
> - if (p == c->uds_port && ps != PS_FAULTY) {
> + if (p == c->uds_rw_port && ps != PS_FAULTY) {
> return 1;
> }
> return 0;
> @@ -818,7 +826,7 @@ static int clock_add_port(struct clock *c, const char
> *phc_device,
> {
> struct port *p, *piter, *lastp = NULL;
>
> - if (clock_resize_pollfd(c, c->nports + 1)) {
> + if (clock_resize_pollfd(c, c->nports + 2)) {
> return -1;
> }
> p = port_open(phc_device, phc_index, timestamping,
> @@ -1044,20 +1052,40 @@ struct clock *clock_create(enum clock_type type,
> struct config *config,
>
> /* Configure the UDS. */
> uds_ifname = config_get_string(config, NULL, "uds_address");
> - c->udsif = interface_create(uds_ifname);
> - if (config_set_section_int(config, interface_name(c->udsif),
> + c->uds_rw_if = interface_create(uds_ifname);
> + if (config_set_section_int(config, interface_name(c->uds_rw_if),
>
Re: [Linuxptp-devel] [PATCH 0/3] Convert to inclusive terminology, Part III
> -Original Message- > From: Richard Cochran > Sent: Monday, January 18, 2021 10:57 AM > To: linuxptp-devel@lists.sourceforge.net > Subject: [Linuxptp-devel] [PATCH 0/3] Convert to inclusive terminology, Part > III > > * Part III > > - Mark the existing masterOnly option as deprecated, providing an > alternative option called serverOnly. > This part looks good to me. Reviewed-by: Jacob Keller > * Background > > There is an industry wide effort underway to replace historically and > culturally loaded terms like master/slave with neutral alternatives. > The IEEE 1588 committee will most likely amend the standard, but so > far no consensus on the new terminology has been reached. > > Many of the proposed alternative terms are either awful sounding or > just plain silly. This project will take the lead by implementing > proper English language terminology that is, at the same time, both > culturally neutral and technically more accurate. > > The original designation of the PTP port roles made little sense in > the first place. Under the institution of slavery, the role of a > slave is to perform work for the master. In a PTP network it is the > "master" port that serves the slaves, the opposite of what the terms > suggest. Thus, in the context of the PTP port roles, we have chosen > to replace master/slave with client/server. > > Besides the PTP port roles, there is the area of local clock > synchronization as performed by the phc2sys and the ts2phc programs. > In the past we have applied the master/slave terminology in a > confusing manner. The phc2sys program labeled a single port both > master and slave at the same time, but in different contexts. In > contrast, the new terminology will be "time source" and "time sink", > inspired by signal nomenclature from the field of electrical > engineering. > > Thanks, > Richard > > > Richard Cochran (3): > Deprecate the masterOnly option in favor of serverOnly. > Convert the example configuration files over to the new serverOnly > option. > Update man page to reflect the new serverOnly option. > > config.c | 5 - > configs/G.8265.1.cfg | 2 +- > configs/G.8275.1.cfg | 2 +- > configs/G.8275.2.cfg | 2 +- > configs/automotive-master.cfg | 2 +- > configs/default.cfg | 2 +- > port.c| 4 ++-- > ptp4l.8 | 6 +- > 8 files changed, 16 insertions(+), 9 deletions(-) > > -- > 2.20.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
Re: [Linuxptp-devel] [PATCH 0/5] PMC Agent - Part V - phc2sys cleanups
> -Original Message- > From: Richard Cochran > Sent: Saturday, December 05, 2020 8:21 PM > To: linuxptp-devel@lists.sourceforge.net > Subject: [Linuxptp-devel] [PATCH 0/5] PMC Agent - Part V - phc2sys cleanups > > In the effort to reshape the PMC agent logic into a coherent form, it > is necessary to understand how this code is used by phc2sys. > > However, that program is a tangle of at least four different program > flows, and the time has come to sort it all out. > > This series continues the journey by simplifying the phc2sys program > flow, thereby making one of the many PMC agent's methods obsolete. > > Series looks good to me. Nice to see these improvements. > Richard Cochran (5): > phc2sys: Replace yet another magical test with a proper test. > phc2sys: Move static configuration to its own subroutine. > pmc_agent: Let the update method poll for push events. > phc2sys: Simplify the main loop. > pmc_agent: Remove an obsolete method. > > phc2sys.c | 85 ++--- > pmc_agent.c | 10 ++- > pmc_agent.h | 6 ++-- > 3 files changed, 60 insertions(+), 41 deletions(-) > > -- > 2.20.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
Re: [Linuxptp-devel] [PATCH 3/5] pmc_agent: Let the update method poll for push events.
> -Original Message-
> From: Richard Cochran
> Sent: Saturday, December 05, 2020 8:21 PM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH 3/5] pmc_agent: Let the update method poll
> for
> push events.
>
> Signed-off-by: Richard Cochran
> ---
> pmc_agent.c | 3 +++
> pmc_agent.h | 5 +++--
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/pmc_agent.c b/pmc_agent.c
> index aa2347d..6e6627d 100644
> --- a/pmc_agent.c
> +++ b/pmc_agent.c
> @@ -390,6 +390,7 @@ int pmc_agent_subscribe(struct pmc_agent *node, int
> timeout)
>
> int pmc_agent_update(struct pmc_agent *node)
> {
> + struct ptp_message *msg;
> struct timespec tp;
> uint64_t ts;
>
> @@ -411,6 +412,8 @@ int pmc_agent_update(struct pmc_agent *node)
> }
> }
>
> + run_pmc(node, 0, -1, );
> +
So, by adding this call, run_pmc_agent will now additionally check for any new
updates?
> return 0;
> }
>
> diff --git a/pmc_agent.h b/pmc_agent.h
> index f0e2c7a..dd34d30 100644
> --- a/pmc_agent.h
> +++ b/pmc_agent.h
> @@ -141,11 +141,12 @@ void pmc_agent_set_sync_offset(struct pmc_agent
> *agent, int offset);
> int pmc_agent_subscribe(struct pmc_agent *agent, int timeout);
>
> /**
> - * Queries the local ptp4l instance to update the TAI-UTC offset and
> - * the current leap second flags.
> + * Polls for push notifications from the local ptp4l service.
> *
> * In addition:
> *
> + * - Queries the local ptp4l instance to update the TAI-UTC offset and
> + * the current leap second flags.
> * - Any active port state subscription will be renewed.
> * - The port state notification callback might be invoked.
> *
> --
> 2.20.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
Re: [Linuxptp-devel] [PATCH v2 0/7] PMC Agent - Part II
> -Original Message- > From: Richard Cochran > Sent: Friday, November 27, 2020 6:30 PM > To: linuxptp-devel@lists.sourceforge.net > Subject: [Linuxptp-devel] [PATCH v2 0/7] PMC Agent - Part II > The whole series looks good to me. Thanks, Jake > Changed in V2 > ~ > - Picked up review tags from Jacob and Vladimir. > - Implemented changes from Vladimir's review. > > > Looking into the PMC agent code, there are issues that will need > resolution. This series converts the subscribe and update methods > into the canonical form. > > The first patch prepares the way for implementing consistent return > value semantics. > > The second patch renames the subscribe method, which is a > straightforward conversion. > > The remaining five patches deal with the update method. Even with the > simplifications, still the end result seems suspicious, because when > writing the documentation, it is hard to put purpose of this method > into words. This method calls into run_pmc, as do most of the > remaining methods to be converted, and that function is fairly > complex. It remains to be seen whether it can be disentangled into a > more coherent form. > > > Richard Cochran (7): > Introduce error codes for the run_pmc method. > pmc_agent: Convert the subscribe method into the canonical form. > pmc_agent: Simplify the update method. > pmc_agent: Simplify logic in update method. > pmc_agent: Remove bogus comparison between last update and now. > pmc_agent: Perform time comparison using positive logic. > pmc_agent: Rename the update method and attempt to document it. > > phc2sys.c | 10 +++-- > pmc_agent.c | 122 > pmc_agent.h | 30 - > 3 files changed, 110 insertions(+), 52 deletions(-) > > -- > 2.20.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
Re: [Linuxptp-devel] [PATCH RFC 0/1] Introduce inclusive terminology
> -Original Message- > From: Miroslav Lichvar > Sent: Thursday, October 29, 2020 10:24 AM > To: Richard Cochran > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH RFC 0/1] Introduce inclusive terminology > > On Thu, Oct 29, 2020 at 09:53:42AM -0700, Richard Cochran wrote: > > On Thu, Oct 29, 2020 at 11:58:41AM +0100, Miroslav Lichvar wrote: > > > > > That wouldn't work well, but as phc2sys doesn't use PTP to synchronize > > > the clocks, I think it can use a different terminology than ptp4l. It > > > might actually be less confusing. In the current code the same clock > > > is master and slave at the same time in different contexts. > > > > So how about using client/server in the context of the PTP and > > source/sink in phc2sys and ts2phc? > > Yes, I think I would prefer that over source/sink everywhere. To me, > server and client are network-related terms, something implementing a > network protocol, which in context of PTP has a single clock. Sink and > source work as more general terms. As an adjective of a clock, "source > clock" works well, but "sink clock" sounds weird to me. I found this > term in some DisplayPort and HDMI documentation. Maybe it's alright, I > don't know. Native speakers should give you a better feedback or > suggestions. If we used "time source" and "time sink", but that wouldn't include clock in the name. What about "target clock"? ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH v2 00/14] Dynamic sync direction for ts2phc
> -Original Message- > From: Vladimir Oltean > Sent: Monday, August 17, 2020 5:03 PM > To: Keller, Jacob E > Cc: richardcoch...@gmail.com; linuxptp-devel@lists.sourceforge.net; > fer...@gmail.com > Subject: Re: [RFC PATCH v2 00/14] Dynamic sync direction for ts2phc > > On Mon, Aug 17, 2020 at 11:57:09PM +, Keller, Jacob E wrote: > > > > Any chance you could also provide a "git range-diff"? i.e. assuming v1 > > is at dynamic-ts2phc-v1 and v2 is at dynamic-ts2phc-v2 > > > > git range-diff master dynamic-ts2phc-v1 dynamic-ts2phc-v2 > > > > This creates a sort of diff of the series that can help show what you > > changed between the versions. > > > > Thanks, > > Jake > > > > For this version, the range-diff is precisely what I've pasted in reply > to Richard here (plus an adaptation to the fact that run_pmc_events > returns void in v2 instead of int in v1): > > https://sourceforge.net/p/linuxptp/mailman/linuxptp- > devel/thread/20200817202834.hemnemv3c7edchre%40skbuf/#msg37087552 > > For the next versions, I'll keep this in mind and post a range-diff in > the cover letter. > > Thanks, > -Vladimir Great! Thanks Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH v2 00/14] Dynamic sync direction for ts2phc
> -Original Message-
> From: Vladimir Oltean
> Sent: Monday, August 17, 2020 4:19 PM
> To: richardcoch...@gmail.com
> Cc: linuxptp-devel@lists.sourceforge.net; fer...@gmail.com; Keller, Jacob E
>
> Subject: [RFC PATCH v2 00/14] Dynamic sync direction for ts2phc
>
> Changes in v2:
> - Added Jacob's review tags, addressed some feedback.
> - Dropped patch "pmc_common: fix potential memory leak in
> run_pmc_events()"
> - Reordered patches (put the tmv helpers first)
> - Fixed memory leaks
>
Any chance you could also provide a "git range-diff"? i.e. assuming v1 is at
dynamic-ts2phc-v1 and v2 is at dynamic-ts2phc-v2
git range-diff master dynamic-ts2phc-v1 dynamic-ts2phc-v2
This creates a sort of diff of the series that can help show what you changed
between the versions.
Thanks,
Jake
> As discussed in this email thread:
> https://sourceforge.net/p/linuxptp/mailman/message/37047555/
> there is a desire to synchronize multiple DSA switches in a
> boundary_clock_jbod setup, using a PPS signal, and the ts2phc program
> already offers a solid base.
>
> This patch series extends the ts2phc program to cater for that use case
> by introducing a new '-a' option and friends ('--transportSpecific').
> This makes it quite similar to phc2sys which has the same ability, just
> that ts2phc can give much better performance if the hardware can assist
> with that.
>
> The overall board design for my use case is that there's an SoC with an
> embedded DSA switch, and hanging off of 3 ports of that embedded switch
> are 3 external switches. Every networking device (the DSA master for the
> embedded switch, the embedded switch, as well as each individual
> external switch) has a PTP clock. The topology for PPS signal
> distribution is fixed - that is given by hardware ability - the
> /dev/ptp1 clock can emit a valid PPS, and all external switches can
> timestamp that PPS (it is connected in a sort-of simplex "bus" design),
> and it cannot be any other way around. It looks like this:
>
> +---+
> | LS1028A /dev/ptp0 (unused) |
> | +--+|
> | | DSA master for Felix||
> | |(internal ENETC port 2: eno2))||
> | ++--+-+ |
> | | Felix embedded L2 switch /dev/ptp1 | |
> | | PPS master, sync slave | |
> | | +--+ +--+ +--+ | |
> | | |DSA master for| |DSA master for| |DSA master for| | |
> | | | SJA1105 1 | | SJA1105 2 | | SJA1105 3 | | |
> | | |(Felix port 1)| |(Felix port 2)| |(Felix port 3)| | |
> +--+-+--+---+--+---+--+--+--+
>
> /dev/ptp2/dev/ptp3/dev/ptp4
> PPS slave, sync masterPPS slave, sync slave PPS slave, sync slave
> +---+ +---+ +---+
> | SJA1105 switch 1| | SJA1105 switch 2| | SJA1105 switch 3|
> +-+-+-+-+ +-+-+-+-+ +-+-+-+-+
> |sw1p0|sw1p1|sw1p2|sw1p3| |sw2p0|sw2p1|sw2p2|sw2p3|
> |sw3p0|sw3p1|sw3p2|sw3p3|
> +-+-+-+-+ +-+-+-+-+ +-+-+-+-+
>^
>|
> GM connected here
>
> In text, it would be described as this:
>
> cat ts2phc.cfg
> [global]
> first_step_threshold0.2
> step_threshold 0.2
> ts2phc.pulsewidth 5
> ts2phc.perout_phase 0
>
> # Felix
> [/dev/ptp1]
> ts2phc.master 1
>
> # SJA1105 switch 1
> [/dev/ptp2]
> ts2phc.channel 0
> ts2phc.extts_polarity both
>
> # SJA1105 switch 2
> [/dev/ptp3]
> ts2phc.channel 0
> ts2phc.extts_polarity both
>
> # SJA1105 switch 3
> [/dev/ptp4]
> ts2phc.channel 0
> ts2phc.extts_polarity both
>
> cat gPTP.cfg
> [global]
> gmCapable 1
> priority1 248
> priority2 248
> logAnnounceInterval 0
> logSyncInterval -3
> syncReceiptTimeout 3
> neighborPropDelayThresh 5
> min_neighbor_prop_delay -2000
> assume_two_step 1
> path_trace_enabled 1
> follow_up_info 1
> transportSpecific 0x1
> ptp_dst_mac 01:80:C2:00:00:0E
> network_transport L2
> delay_mechanism P2P
> step_threshold 0.00
Re: [Linuxptp-devel] [RFC PATCH 01/15] posix_clock_open: derive PHC index from device name if possible
> -Original Message- > From: Richard Cochran > Sent: Wednesday, August 12, 2020 8:40 AM > To: Keller, Jacob E > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [RFC PATCH 01/15] posix_clock_open: derive PHC > index from device name if possible > > On Wed, Aug 05, 2020 at 04:12:02PM -0700, Jacob Keller wrote: > > Here, we are making the implicit assumption that all ptp clock devices > > will always have /dev/ptpX format. I don't think anyone is crazy enough > > to rename these devices... > > (Not yet, anyways ;^) > > > An alternative (requiring kernel implementation maybe?) would be to read > > the phc index from the kernel somehow. It doesn't look like this is > > exported anywhere else besides the name. > > How about /sys/class/ptp/ptpX/clock_index ? > > Is that something you would like to code up for the lkml? > > Thanks, > Richard To make this fully work we'll probably want to adjust the LinuxPTP code base with a phc_index_to_dev function that scans the sys/class/ptp folder for the matching clock based on the index to obtain the name. I'll see about adding that as well (with a fallback to attempting /dev/ptpX if clock_index doesn't exist) Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH 01/15] posix_clock_open: derive PHC index from device name if possible
> -Original Message- > From: Richard Cochran > Sent: Wednesday, August 12, 2020 8:40 AM > To: Keller, Jacob E > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [RFC PATCH 01/15] posix_clock_open: derive PHC > index from device name if possible > > On Wed, Aug 05, 2020 at 04:12:02PM -0700, Jacob Keller wrote: > > Here, we are making the implicit assumption that all ptp clock devices > > will always have /dev/ptpX format. I don't think anyone is crazy enough > > to rename these devices... > > (Not yet, anyways ;^) > > > An alternative (requiring kernel implementation maybe?) would be to read > > the phc index from the kernel somehow. It doesn't look like this is > > exported anywhere else besides the name. > > How about /sys/class/ptp/ptpX/clock_index ? > > Is that something you would like to code up for the lkml? > > Thanks, > Richard Yea, I can do that. -Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] ts2phc_phc_master: specify start time to PPS master as 0.000000000
> -Original Message-
> From: Vladimir Oltean
> Sent: Tuesday, July 07, 2020 9:08 AM
> To: Keller, Jacob E ; richardcoch...@gmail.com
> Cc: linuxptp-devel@lists.sourceforge.net
> Subject: Re: [Linuxptp-devel] [PATCH] ts2phc_phc_master: specify start time to
> PPS master as 0.0
>
> What modification do you think should be done to the kernel ABI?
> This is what is currently in place:
>
> struct ptp_perout_request {
> struct ptp_clock_time start; /* Absolute start time. */
> struct ptp_clock_time period; /* Desired period, zero means disable. */
> unsigned int index; /* Which channel to configure. */
> unsigned int flags;
> unsigned int rsv[4]; /* Reserved for future use. */
> };
>
> Most interesting to me would be the "flags" field. The v2 of the ioctl
> can currently only have:
>
> #define PTP_PEROUT_ONE_SHOT (1<<0)
>
> Maybe something like this?
>
> #define PTP_PEROUT_RELATIVE_START_TIME (1<<1)
>
> with the meaning of 'just set up the clock generator to start ASAP (with
> an unspecified definition of ASAP) with this phase offset in
> nanoseconds, relative to 0 which corresponds to the closest integer
> multiple of the period in the PHC time base'.
>
> What would be the valid range of 'start' in the case where (flags &
> PTP_PEROUT_RELATIVE_START_TIME) is set?
>
What would start even be used for? We have a period already, and it sounds like
relative start wouldn't even use a start value?
> - Zero to $('period' - 1) nanoseconds, or
> - Zero to infinity, with a modulo performed by somebody (probably the
> PTP core)?
>
> Also, there's the question of what to do about the sysfs ABI. Currently
> you can't pass the flags through that:
>
> cnt = sscanf(buf, "%u %lld %u %lld %u", ,
>, ,
>, );
> if (cnt != 5)
> goto out;
>
> Do we want to permit another sscanf for optional flags at the end? Would
> the flags be textual or numbers (0, 1, 2, 3)?
I don't know how stringent the sysfs interface needs to be on avoiding ABI
breakage, but we probably want to be careful here. Personally I really dislike
this interface as I constantly have to lookup what order each thing is in. I'd
rather see it replaced with something more akin to "name=value" or "name value"
pairs.. though in practice if we simply make the current userspace tools better
then it wouldn't be that relevant since those would be easier to use overall
anwyays...
>
> Thanks,
> -Vladimir
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 2/3] clock: dump unexpected packets received on the error queues of sockets
> -Original Message- > From: Richard Cochran > Sent: Wednesday, June 24, 2020 9:58 AM > To: Vladimir Oltean > Cc: linuxptp-devel@lists.sourceforge.net; Keller, Jacob E > > Subject: Re: [PATCH v2 2/3] clock: dump unexpected packets received on the > error queues of sockets > > On Wed, Jun 24, 2020 at 07:36:47PM +0300, Vladimir Oltean wrote: > > I am reading this as: "let's be defensive even in the case where the > > kernel decides to go nuts and push us a packet on the error queue even > > if we didn't enable SO_SELECT_ERR_QUEUE". But that isn't at all what's > > going on. As stated, we opted in this SO_SELECT_ERR_QUEUE game because > > we need TX timestamps. > > But, at this point in the program, we know that no tx time stamp is > outstanding. We always send one Tx event, then immediately fetch the > time stamp. This is carefully synchronized by the program. It is > important to do this because many drivers support exactly one Tx time > stamp at a time. > > The kernel must not fabricate transmit time stamps! That would be > breaking the contract. Sure, but the POLLERR could (in theory, not in current practice) return other events/messages besides timestamps. It was invented/created prior to the Tx timestamps, wasn't it? > > > So we need to be correct, and play by the API's > > rules, which means treat the POLLERR returned event. It is a > > correctness issue, not a defense issue. > > I think you are splitting hairs here, but I disagree that the program > was incorrect. There is no reason _today_ for poll to return a > POLLERR event from this call, but, in general, I don't believe this is > guaranteed by anything. > Right. Today there's no other messages that I am aware of. > Would you prefer me leaving your name off the commit message? > > Thanks, > Richard > > ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 00/10] Slave event monitoring
> -Original Message- > From: Richard Cochran > Sent: Thursday, May 28, 2020 1:35 PM > To: Keller, Jacob E > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH 00/10] Slave event monitoring > > On Thu, May 28, 2020 at 05:07:05PM +, Keller, Jacob E wrote: > > > Presumably, other implementations won't support this non-portable > > TLV. Is there any part of our implementation that ought to support > > the official standard? > > Yes, and this series already implements the official > SLAVE_RX_SYNC_TIMING_DATA. Nothing prevents us adding the official > variant of SLAVE_TX_EVENT_TIMESTAMPS in the future, if anybody really > wants it. > > > Or should we attempt to push for improving the standard? > > Also yes. Already there was something missing from the standard for > this optional feature, and I did share that with the IEEE working > group already. > > > I suppose in some sense, since you're only sending this data over > > the local unix domain socket it's less of a concern? > > Right. We can implement what is useful now and lead the way for > others to follow. > > Thanks, > Richard Sounds good. If someone cares in the future they could implement the standard one when they want it for interop. Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 00/10] Slave event monitoring
> -Original Message- > From: Richard Cochran > Sent: Sunday, May 24, 2020 7:53 PM > To: linuxptp-devel@lists.sourceforge.net > Subject: [Linuxptp-devel] [PATCH 00/10] Slave event monitoring > > The IEEE 1588 v2.1 standard introduces a new optional feature known as > slave event monitoring. This feature defines three new TLVs. > > 1. SLAVE_RX_SYNC_TIMING_DATA > >This TLV provides the time stamps T1 and T2 from the message >exchange between master and slave. > > 2. SLAVE_TX_EVENT_TIMESTAMPS > >This provides the T3 time stamp only. > > 3. SLAVE_RX_SYNC_COMPUTED_DATA > >This TLV provides the estimated offset and path delay. > > The standard leaves the mechanism for enabling and configuring this > feature as implementation-defined. This series implements slave > monitoring by offering a configuration option that specifies a UDS > address, allowing a local client such as the 'pmc' program to monitor > the time stamps. > > While this series implements the first TLV verbatim, the second TLV > makes little sense as specified in the standard, because it omits the > T4 time stamp. For this reason a custom TLV is used that provides > both T3 and T4. The third TLV is not implemented here because it is > redundant, merely providing data already available in other management > messages. Presumably, other implementations won't support this non-portable TLV. Is there any part of our implementation that ought to support the official standard? Or should we attempt to push for improving the standard? I.e. while I agree that adding the the T4 timestamp is good, I'd like to make sure that we can interoperate well with others. I suppose in some sense, since you're only sending this data over the local unix domain socket it's less of a concern? Thanks, Jake > > Patches 7-9 then add the custom SLAVE_DELAY_TIMING_DATA_NP TLV into > ptp4l, and patch 10 adds it to pmc. > > > Richard Cochran (10): > tlv: Update macro definitions. > tlv: Encode and decode SLAVE_RX_SYNC_TIMING_DATA TLVs. > Introduce a module for slave event monitoring. > clock: Create a slave event monitor. > port: Support slave event monitoring of Sync timing data. > pmc: Show slave receive timing data TLVs attached to signaling > messages. > tlv: Encode and decode SLAVE_DELAY_TIMING_DATA_NP TLVs. > monitor: Add support for slave delay timing data TLV. > port: Support slave event monitoring of delay timing data. > pmc: Show slave delay timing data TLVs attached to signaling messages. > > clock.c| 13 +++ > clock.h| 8 ++ > config.c | 1 + > makefile | 6 +- > monitor.c | 231 > + > monitor.h | 25 ++ > pmc.c | 86 ++ > port.c | 25 +- > port_private.h | 3 + > ptp4l.8| 6 ++ > tlv.c | 163 +- > tlv.h | 67 -- > 12 files changed, 620 insertions(+), 14 deletions(-) > create mode 100644 monitor.c > create mode 100644 monitor.h > > -- > 2.20.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
Re: [Linuxptp-devel] [PATCH 5/5] pmc: Allow multiple local subscribers.
> -Original Message-
> From: Richard Cochran
> Sent: Friday, April 03, 2020 7:30 AM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH 5/5] pmc: Allow multiple local subscribers.
>
> If more than one local UDS client subscribes to push notifications,
> only the last one receives data from the ptp4l service. This happens
> because ptp4l uses the PortIdentity as a unique key to track client
> subscriptions. As a result, it is not possible for both phc2sys and
> pmc to receive push notifications at the same time, for example.
>
> This patch sets the PortIdentity.portNumber attribute of UDS clients
> to the local process ID, making each such client subscription unique.
>
> Signed-off-by: Richard Cochran
> ---
> pmc_common.c | 19 ---
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/pmc_common.c b/pmc_common.c
> index f89d87c..822dd6d 100644
> --- a/pmc_common.c
> +++ b/pmc_common.c
> @@ -18,8 +18,10 @@
> * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> */
> #include
> -#include
> #include
> +#include
> +#include
> +#include
>
> #include "notification.h"
> #include "print.h"
> @@ -353,13 +355,16 @@ struct pmc *pmc_create(struct config *cfg, enum
> transport_type transport_type,
> if (!pmc)
> return NULL;
>
> - if (transport_type != TRANS_UDS &&
> - generate_clock_identity(>port_identity.clockIdentity,
> - iface_name)) {
> - pr_err("failed to generate a clock identity");
> - goto failed;
> + if (transport_type == TRANS_UDS) {
> + pmc->port_identity.portNumber = getpid();
> + } else {
> + if (generate_clock_identity(>port_identity.clockIdentity,
> + iface_name)) {
> + pr_err("failed to generate a clock identity");
> + goto failed;
> + }
> + pmc->port_identity.portNumber = 1;
Ah, so previously we always set them to 1, but now we use the pid for local
UDS, and a clock identity for remote ones. Nice
> }
> - pmc->port_identity.portNumber = 1;
> pmc_target_all(pmc);
>
> pmc->boundary_hops = boundary_hops;
> --
> 2.20.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
Re: [Linuxptp-devel] [PATCH 2/3] clock: Dump unexpected packets received on the error queues of sockets
> -Original Message-
> From: Vladimir Oltean
> Sent: Tuesday, December 17, 2019 12:34 PM
> To: Keller, Jacob E
> Cc: richardcoch...@gmail.com; linuxptp-devel@lists.sourceforge.net
> Subject: Re: [Linuxptp-devel] [PATCH 2/3] clock: Dump unexpected packets
> received on the error queues of sockets
>
> On Tue, 17 Dec 2019 at 22:04, Keller, Jacob E
> wrote:
>
> > > -Original Message-
> > > From: Vladimir Oltean
> > > Sent: Monday, December 16, 2019 3:11 PM
> > > To: richardcoch...@gmail.com
> > > Cc: linuxptp-devel@lists.sourceforge.net
> > > Subject: [Linuxptp-devel] [PATCH 2/3] clock: Dump unexpected packets
> received
> > > on the error queues of sockets
> > > For messages that the application does not / no longer expects, such as
> > > TX timestamps delivered late, duplicate TX timestamps, general
> > > exceptional messages enqueued by the kernel in the socket error queue
> > > etc, ptp4l will be taken by surprise in clock_poll() by these, and will
> > > think that there is data, since POLLIN is set (but in fact in the socket
> > > error queue etc, ptp4l will be taken by surprise in clock_poll() by
> > > these, and will think that there is data, since POLLIN is set (but in
> > > fact POLLERR is also set, and this has an entirely different meaning).
> >
> > This double parenthetical is a bit hard to parse.
> >
>
> Sorry, this is hard for me to understand too. The paragraph should have been:
>
> For messages that the application does not / no longer expects, such as
> TX timestamps delivered late, duplicate TX timestamps, general
> exceptional messages enqueued by the kernel in the socket error queue
> etc, ptp4l will be taken by surprise in clock_poll() by these, and will
> think that there is data, since POLLIN is set (but in fact POLLERR is
> also set, and this has an entirely different meaning), and will attempt
> to dequeue them from the wrong queue, which is empty.
>
That sounds better. It still feels a bit like a run-on sentence. Maybe
something like:
For messages that the application does not / no longer expects, such as Tx
timestamps delivered late, duplicate Tx timestamps, or general exceptional
messages enqueued by the kernel in the socket error queue, ptp4l will be taken
by surprise in clock_poll(). It will think there is data, since POLLIN is set.
In fact, POLLERR is also set which gives an entirely different meaning. Because
of this, ptp4l will attempt to dequeue messages from the wrong (empty) queue.
>
> > >
> > > A very, very simple reproducer is to take a DSA switch and run:
> > >
> > > tcpdump -i eth0 -j adapter_unsynced
> > >
> > > on its DSA master net device. The above command will enable timestamping
> > > on that net device, and if both the DSA switch and the master support
> > > PTP, this will make the kernel send duplicate TX timestamps for every
> > > sent event packet, which will completely kill ptp4l until a reboot, with
> > > no indication whatsoever of what's going on.
> > >
> > > Since the messages on the error queue are unexpected, we have no need
> > > for them. And they can be in theory anything, so simply hex dumping
> > > their content and moving along sounds like a good idea.
> > >
> > > Printing them to the user is optional (and helpful), but reading them is
> > > not. With this patch, even with extraneous data delivered by a buggy
> > > kernel (which the application now loudly complains about), the
> > > synchronization keeps chugging along. Otherwise the application starts
> > > reordering packets in recvmsg() due to misinterpreting which socket
> > > queue has data available.
> > >
> > > Signed-off-by: Vladimir Oltean
> > > ---
> > > clock.c | 11 +++
> > > msg.c | 12
> > > msg.h | 7 +++
> > > 3 files changed, 30 insertions(+)
> > >
> > > diff --git a/clock.c b/clock.c
> > > index 146576ac589c..768bbb49513d 100644
> > > --- a/clock.c
> > > +++ b/clock.c
> > > @@ -1508,6 +1508,17 @@ int clock_poll(struct clock *c)
> > > LIST_FOREACH(p, >ports, list) {
> > > /* Let the ports handle their events. */
> > > for (i = 0; i < N_POLLFD; i++) {
> > > + if (cur[i].revents & POLLERR) {
> > > + unsigned char pkt[1600];
> > > +
> > > + cnt = recv(cur[i].fd, pkt, sizeof(pkt),
> > > +
Re: [Linuxptp-devel] [PATCH 3/3] port: Signal sync/follow-up mismatch events loudly
> -Original Message- > From: Vladimir Oltean > Sent: Monday, December 16, 2019 3:11 PM > To: richardcoch...@gmail.com > Cc: linuxptp-devel@lists.sourceforge.net > Subject: [Linuxptp-devel] [PATCH 3/3] port: Signal sync/follow-up mismatch > events loudly > > Ptp4l is too silent when receiving, for whatever reason, out of order > messages. If the reordering is persistent (which is either a broken > network, or a broken kernel), the behavior looks like a complete > synchronization stall, since the application is designed to never > attempt to recover from such a condition. > > At least save some people some debugging hours and be loud when the > application reaches this code path. > Excellent! This one makes sense to me. Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 1/3] ptp4l: Call recvmsg() with the MSG_DONTWAIT flag
> -Original Message-
> From: Vladimir Oltean
> Sent: Monday, December 16, 2019 3:10 PM
> To: richardcoch...@gmail.com
> Cc: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH 1/3] ptp4l: Call recvmsg() with the
> MSG_DONTWAIT flag
>
> The application's main event loop (clock_poll) is woken up by poll() and
> dispatches the socket receive queue events to the corresponding ports as
> needed.
>
> So it is a bug if poll() wakes up the process for data availability on a
> socket's receive queue, and then recvmsg(), called immediately
> afterwards, goes to sleep trying to retrieve it. This patch will
> generate an error that will be propagated to the user if this condition
> happens.
>
> Can it happen?
>
> As of this patch, ptp4l uses the SO_SELECT_ERR_QUEUE socket option,
> which means that poll() will wake the process up, with revents ==
> (POLLIN | POLLERR), if data is available in the error queue. But
> clock_poll() does not check POLLERR, just POLLIN, and draws the wrong
> conclusion that there is data available in the receive queue (when it is
> in fact available in the error queue).
>
> When the above condition happens, recvmsg() will sleep typically for a
> whole sync interval waiting for data on the event socket, and will be
> woken up when the new real frame arrives. It will not dequeue follow-up
> messages during this time (which are sent to the general message socket)
> and when it does, it will already be late for them (their seqid will be
> out of order). So it will drop them and everything that comes after. The
> synchronization process will fail.
>
> The above condition shouldn't typically happen, but exceptional kernel
> events will trigger it. It helps to be strict in ptp4l in order for
> those events to not blow up in even stranger symptoms unrelated to the
> root cause of the problem.
>
Yep, makes sense.
> Signed-off-by: Vladimir Oltean
> ---
> raw.c | 2 +-
> udp.c | 2 +-
> udp6.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/raw.c b/raw.c
> index f1c92b9f8d90..c0a8cd80855f 100644
> --- a/raw.c
> +++ b/raw.c
> @@ -278,7 +278,7 @@ static int raw_recv(struct transport *t, int fd, void
> *buf, int
> buflen,
> buflen += hlen;
> hdr = (struct eth_hdr *) ptr;
>
> - cnt = sk_receive(fd, ptr, buflen, addr, hwts, 0);
> + cnt = sk_receive(fd, ptr, buflen, addr, hwts, MSG_DONTWAIT);
>
So basically we just indicate that we do not want to sleep. Good.
This makes sense to me.
Thanks,
Jake
> if (cnt >= 0)
> cnt -= hlen;
> diff --git a/udp.c b/udp.c
> index 48af482b4526..eb1617872f37 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -210,7 +210,7 @@ no_event:
> static int udp_recv(struct transport *t, int fd, void *buf, int buflen,
> struct address *addr, struct hw_timestamp *hwts)
> {
> - return sk_receive(fd, buf, buflen, addr, hwts, 0);
> + return sk_receive(fd, buf, buflen, addr, hwts, MSG_DONTWAIT);
> }
>
> static int udp_send(struct transport *t, struct fdarray *fda,
> diff --git a/udp6.c b/udp6.c
> index 74ebc7f0cf09..06c6fad2160f 100644
> --- a/udp6.c
> +++ b/udp6.c
> @@ -227,7 +227,7 @@ no_event:
> static int udp6_recv(struct transport *t, int fd, void *buf, int buflen,
>struct address *addr, struct hw_timestamp *hwts)
> {
> - return sk_receive(fd, buf, buflen, addr, hwts, 0);
> + return sk_receive(fd, buf, buflen, addr, hwts, MSG_DONTWAIT);
> }
>
> static int udp6_send(struct transport *t, struct fdarray *fda,
> --
> 2.17.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
Re: [Linuxptp-devel] [PATCH 0/3] More strict checking against kernel bugs
> -Original Message- > From: Vladimir Oltean > Sent: Monday, December 16, 2019 3:10 PM > To: richardcoch...@gmail.com > Cc: linuxptp-devel@lists.sourceforge.net > Subject: [Linuxptp-devel] [PATCH 0/3] More strict checking against kernel bugs > > The reordering issue reported by me initially on the linuxptp-devel > list [0] with the sja1105 DSA driver turned out not to be a reordering > issue at all, in fact. > > Due to a kernel bug described in this patch [1], the DSA driver was in > race with the master Ethernet driver and would occasionally (very > rarely) deliver 2 TX timestamps to ptp4l on the event socket. > > The first TX timestamp is consumed in-band in the raw_send function. > The second is caught by the main poll() syscall in the main ptp4l event > loop - clock_poll(). > > When poll() sees the second TX-timestamped skb, it returns with revents > == (POLLIN | POLLERR). But the main loop only checks for POLLIN, and > says "yay, there's data!". So it proceeds to call recvmsg() with flags=0 > (instead of MSG_ERRQUEUE), so it doesn't see any data in > sk->sk_receive_queue. So, surprise, false alarm, the data that woke it > up was in sk->sk_error_queue. The ptp4l process goes to sleep waiting > for data. > > It sleeps for a whole sync interval. > > When it wakes up, it wakes up with the next sync, even though the > previous sync's follow-up may have arrived in the meantime. > > Apparent reordering. > > Ptp4l does not print anything, it just appears to freeze. > > So this patch set aims to improves the error reporting in ptp4l, such > that tracing back to the root cause is easier next time, and the problem > does not blow up into other, completely unrelated things. > Nice analysis. Thanks, Jake > [0]: https://sourceforge.net/p/linuxptp/mailman/message/36773629/ > [1]: https://patchwork.ozlabs.org/patch/1210871/ > > Vladimir Oltean (3): > ptp4l: Call recvmsg() with the MSG_DONTWAIT flag > clock: Dump unexpected packets received on the error queues of sockets > port: Signal sync/follow-up mismatch events loudly > > clock.c | 11 +++ > msg.c | 12 > msg.h | 7 +++ > port.c | 21 + > raw.c | 2 +- > udp.c | 2 +- > udp6.c | 2 +- > 7 files changed, 54 insertions(+), 3 deletions(-) > > -- > 2.17.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
Re: [Linuxptp-devel] [PATCH 1/2] phc_ctl: display all capability information
> -Original Message- > From: Richard Cochran > Sent: Saturday, October 12, 2019 8:26 PM > To: Keller, Jacob E > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH 1/2] phc_ctl: display all capability > information > > On Thu, Sep 26, 2019 at 10:47:21AM -0700, Jacob Keller wrote: > > @@ -320,12 +320,16 @@ static int do_caps(clockid_t clkid, int cmdc, char > *cmdv[]) > > " %d programable alarms\n" > > " %d external time stamp channels\n" > > " %d programmable periodic signals\n" > > - " %s pulse per second support", > > + " %d configurable input/output pins\n" > > + " %s pulse per second support\n" > > + " %s cross timestamping support\n", > > caps.max_adj, > > caps.n_alarm, > > caps.n_ext_ts, > > caps.n_per_out, > > - caps.pps ? "has" : "doesn't have"); > > + caps.n_pins, > > + caps.pps ? "has" : "doesn't have", > > + caps.cross_timestamping ? "has" : "doesn't have"); > > On an older kernel: > > Building m68k > /home/richard/git/linuxptp/phc_ctl.c: In function 'do_caps': > /home/richard/git/linuxptp/phc_ctl.c:318:2: error: 'struct ptp_clock_caps' > has no > member named 'cross_timestamping' > make: *** [: phc_ctl.o] Error 1 > > Probably the easiest way to handle this is to cast to a local > structure definition that is up to date... > > Thanks, > Richard Ahh, yep. Will look into that. Thanks! -Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 0/2] additions to phc_ctl
> -Original Message- > From: Keller, Jacob E > Sent: Thursday, September 26, 2019 10:47 AM > To: linuxptp-devel@lists.sourceforge.net > Cc: Keller, Jacob E > Subject: [PATCH 0/2] additions to phc_ctl > > This series provides a couple of updates to the phc_ctl utility. First, we > display additional capabillity information such as the cross timestamping > support as well as the number of pins. > > The second patch adds a new command, "pin_cfg", which will query the pin > configuration for all of the supported pins. Suggestions on how to format > this this output is appreciated. I would almost think it's better to combine > the > printing into a single pr_* buffer.. but the only way I can think to do that > would be using a local buffer to build up the longer string. > > Jacob Keller (2): > phc_ctl: display all capability information > phc_ctl: add pin_cfg command to display pin functions > > phc_ctl.c | 81 > +-- > 1 file changed, 79 insertions(+), 2 deletions(-) > > -- > 2.23.0.245.gf157bbb9169d Ping. Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] port: Deal with higher-order sync/follow-up reordering
> -Original Message-
> From: Vladimir Oltean [mailto:olte...@gmail.com]
> Sent: Monday, September 30, 2019 3:07 PM
> To: Keller, Jacob E
> Cc: richardcoch...@gmail.com; linuxptp-devel@lists.sourceforge.net
> Subject: Re: [Linuxptp-devel] [PATCH] port: Deal with higher-order
> sync/follow-up
> reordering
>
> On Tue, 1 Oct 2019 at 00:34, Keller, Jacob E wrote:
> >
> > > -Original Message-
> > > From: Vladimir Oltean [mailto:olte...@gmail.com]
> > > Sent: Monday, September 30, 2019 2:30 PM
> > > To: Keller, Jacob E
> > > Cc: richardcoch...@gmail.com; linuxptp-devel@lists.sourceforge.net
> > > Subject: Re: [Linuxptp-devel] [PATCH] port: Deal with higher-order
> > > sync/follow-up
> > > reordering
> > >
> > > Hi Jake,
> > >
> > > > Could we add a section to the man page describing this option and when
> > > > it might
> > > make sense? The tail dropping due to re-ordering is clear but may not
> > > immediately
> be
> > > obvious to users which knobs to use to help resolve the problem.
> > > >
> > >
> > > The patch does indeed touch ptp4l.8. Did you notice that and think it
> > > could use some improvement?
> > >
> > > > Thanks,
> > > > Jake
> > > >
> >
> > It would be nice if the manual page indicated the specific error message
> > that occurs
> (tail-dropped due to reordering).
> >
>
> So I posted this just to get some initial feedback for the general
> approach (removing some config options, adding others, etc) and its
> general sanity (how deep a queue is enough? how can you even know? do
> we even want to go down the rabbit hole? should we print anything in
> the benign case that out-of-order frames are received and still
> properly processed, just to make sure people are not blindly relying
> on this fail safe that could disappear any moment with bad enough
> reordering?). So if the feedback I get is only related to the printf
> strings, then I should already send out a new version?
>
I would wait on that, for at least some feedback.
I generally think the approach is good, and the configurable variable makes
sense.
Leaving the default the same as the old behavior is definitely the most
conservative approach.
> > I also am wondering if we can print a message that indicates the history
> > depth
> option might be useful, similar to how we print something about
> "tx_timestamp_timeout" when we fail to receive a Tx timestamp in time.
> >
>
> Which by the way has a completely inadequate default value, at 1 ms.
> There are even some regular Ethernet drivers for which it's too
> little. And then there are DSA drivers on top of those for which it's
> already too little. The tx_timestamp_timeout should catch all 'real'
> timestamp losses and not just badmouth the driver ("it is likely
> caused by a driver bug").
> But my point is that although I need to change tx_timestamp_timeout
> for every single installation of linuxptp, I'm not sending out a patch
> to change the default because I have no clue what a universally better
> default would be.
>
Yea, picking a good default is problematic.
> > Thanks,
> > Jake
> >
>
> Thanks,
> -Vladimir
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] port: Deal with higher-order sync/follow-up reordering
> -Original Message- > From: Vladimir Oltean [mailto:olte...@gmail.com] > Sent: Monday, September 30, 2019 2:30 PM > To: Keller, Jacob E > Cc: richardcoch...@gmail.com; linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH] port: Deal with higher-order > sync/follow-up > reordering > > Hi Jake, > > > Could we add a section to the man page describing this option and when it > > might > make sense? The tail dropping due to re-ordering is clear but may not > immediately be > obvious to users which knobs to use to help resolve the problem. > > > > The patch does indeed touch ptp4l.8. Did you notice that and think it > could use some improvement? > > > Thanks, > > Jake > > It would be nice if the manual page indicated the specific error message that occurs (tail-dropped due to reordering). I also am wondering if we can print a message that indicates the history depth option might be useful, similar to how we print something about "tx_timestamp_timeout" when we fail to receive a Tx timestamp in time. Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] port: Deal with higher-order sync/follow-up reordering
> -Original Message-
> From: Vladimir Oltean [mailto:olte...@gmail.com]
> Sent: Saturday, September 28, 2019 5:34 AM
> To: richardcoch...@gmail.com
> Cc: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH] port: Deal with higher-order sync/follow-up
> reordering
>
> So a new port config option was introduced, called
> sync_follow_up_history, with a default of 0 that keeps the same behavior
> as what was previously intended (and which nobody apparently complained
> about). So how does this solve the live lock? It doesn't:
>
> ptp4l[7502.451]: rms4 max8 freq +17765 +/- 8 delay 489 +/- 1
> ptp4l[7504.474]: rms4 max 10 freq +17764 +/- 9 delay 489 +/- 1
> ptp4l[7504.912]: Tail-dropping sync 12899 due to reordering
> ptp4l[7504.914]: Tail-dropping follow-up 12899 due to reordering
> ptp4l[7504.944]: Tail-dropping sync 12900 due to reordering
> ptp4l[7504.975]: Tail-dropping sync 12901 due to reordering
> ptp4l[7504.977]: Tail-dropping follow-up 12900 due to reordering
> ptp4l[7505.007]: Tail-dropping sync 12902 due to reordering
>
> The (important) differences are that:
> - The user at least now *knows* what is going on. Previously the only
> behavior was that ptp4l was silently dropping frames and
> synchronization halted. This is still pretty much fatal even with this
> patch, as long as the network keeps pushing frame sequences as above,
> but right now it is much more verbose.
> - The user has a knob to turn to fix this: increase
> sync_follow_up_history while striking an acceptable balance with
> logSyncInterval.
>
So what's the argument for not increasing the default history depth a little to
help alleviate the need to reconfigure it?
I do agree this is a better approach and better messaging than before, and it
is significantly more clear what's going wrong.
Could we add a section to the man page describing this option and when it might
make sense? The tail dropping due to re-ordering is clear but may not
immediately be obvious to users which knobs to use to help resolve the problem.
Thanks,
Jake
> Signed-off-by: Vladimir Oltean
> ---
> config.c| 2 +-
> configs/default.cfg | 2 +-
> e2e_tc.c| 3 +-
> port.c | 217 +---
> port_private.h | 14 +--
> ptp4l.8 | 24 ++---
> ptp4l.c | 1 -
> raw.c | 3 -
> sk.c| 16
> sk.h| 14 ---
> udp.c | 3 -
> udp6.c | 3 -
> 12 files changed, 124 insertions(+), 178 deletions(-)
>
> diff --git a/config.c b/config.c
> index 12eb1f908e36..7e47ae43b763 100644
> --- a/config.c
> +++ b/config.c
> @@ -214,7 +214,6 @@ struct config_item config_tab[] = {
> GLOB_ITEM_INT("assume_two_step", 0, 0, 1),
> PORT_ITEM_INT("boundary_clock_jbod", 0, 0, 1),
> PORT_ITEM_ENU("BMCA", BMCA_PTP, bmca_enu),
> - GLOB_ITEM_INT("check_fup_sync", 0, 0, 1),
> GLOB_ITEM_INT("clockAccuracy", 0xfe, 0, UINT8_MAX),
> GLOB_ITEM_INT("clockClass", 248, 0, UINT8_MAX),
> GLOB_ITEM_STR("clockIdentity", "00..00"),
> @@ -289,6 +288,7 @@ struct config_item config_tab[] = {
> GLOB_ITEM_DBL("step_threshold", 0.0, 0.0, DBL_MAX),
> GLOB_ITEM_INT("summary_interval", 0, INT_MIN, INT_MAX),
> PORT_ITEM_INT("syncReceiptTimeout", 0, 0, UINT8_MAX),
> + PORT_ITEM_INT("sync_follow_up_history", 0, 0, 16),
> GLOB_ITEM_INT("tc_spanning_tree", 0, 0, 1),
> GLOB_ITEM_INT("timeSource", INTERNAL_OSCILLATOR, 0x10, 0xfe),
> GLOB_ITEM_ENU("time_stamping", TS_HARDWARE, timestamping_enu),
> diff --git a/configs/default.cfg b/configs/default.cfg
> index 119df7b60663..e42124c5abec 100644
> --- a/configs/default.cfg
> +++ b/configs/default.cfg
> @@ -30,6 +30,7 @@ logMinPdelayReqInterval 0
> operLogPdelayReqInterval 0
> announceReceiptTimeout 3
> syncReceiptTimeout 0
> +sync_follow_up_history 0
> delayAsymmetry 0
> fault_reset_interval 4
> neighborPropDelayThresh 2000
> @@ -59,7 +60,6 @@ use_syslog 1
> verbose 0
> summary_interval 0
> kernel_leap 1
> -check_fup_sync 0
> #
> # Servo Options
> #
> diff --git a/e2e_tc.c b/e2e_tc.c
> index 6aaf57206067..3f47c5ce45f9 100644
> --- a/e2e_tc.c
> +++ b/e2e_tc.c
> @@ -65,7 +65,8 @@ void e2e_dispatch(struct port *p, enum fsm_event event, int
> mdiff)
> port_set_announce_tmo(p);
> break;
> case PS_UNCALIBRATED:
> - flush_last_sync(p);
> + flush_sync(p);
> + flush_follow_up(p);
> flush_delay_req(p);
> /* fall through */
> case PS_SLAVE:
> diff --git a/port.c b/port.c
> index 07ad3f069d06..5297d0702fb8 100644
> --- a/port.c
> +++ b/port.c
> @@ -48,13 +48,6 @@
> #define ALLOWED_LOST_RESPONSES 3
> #define ANNOUNCE_SPAN 1
>
>
Re: [Linuxptp-devel] [PATCH 2/2] pmc: Support querying TLV_PORT_PROPERTIES_NP
> -Original Message- > From: Petr Machata [mailto:pe...@mellanox.com] > Sent: Monday, September 16, 2019 2:56 AM > To: Keller, Jacob E > Cc: Jiri Benc ; linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH 2/2] pmc: Support querying > TLV_PORT_PROPERTIES_NP > > > Keller, Jacob E writes: > > Right, but part of namespacing is that processes inside the namespace > > should generally not know they are, and not be aware of anything > > outside the namespace... > > I'm not sure I agree the process inside the namespace should be > disallowed from knowing it is. I'm not really well versed in network > namespaces, but looking at what "ip netns id" is doing under the hood it > looks like the kernel has interfaces to just tell you (RTM_GETNSID). > > Thanks, > Petr Fair enough, I wasn't sure exactly what was hidden or not. Thanks, Jake ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 2/2] pmc: Support querying TLV_PORT_PROPERTIES_NP
> -Original Message- > From: Petr Machata [mailto:pe...@mellanox.com] > Sent: Friday, September 13, 2019 7:58 AM > To: Jiri Benc > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH 2/2] pmc: Support querying > TLV_PORT_PROPERTIES_NP > > > Jiri Benc writes: > > > pmc is different. It's not tied to the service start time, it's used by > > administrators and by various scripts from varying environment. While > > you can reasonably expect that ptp4l and phc2sys will be run in the > > same name space, it's not necessarily the case for pmc. > > Hmm, and as an end user I can't do `ip netns identify $(pidof ptp4l)` > even if I know about this issue. > > Maybe ptp4l could include the namespace name in the VLA response. Are > the messages considered a stable API? > Is the namespacing information supposed to be kept secure from processes inside the namespacing? I would want to prevent any sort of leak of namespace info in that case. ptp4l won't be able to tell the request came from within a namespace either. Hmm. It's plausible the pmc instance wouldn't even know about the device, but ptp4l itself doesn't know whether to hide that info or not. Other way around could be that ptp4l is inside a namespace while pmc is not, but again it's not obvious. I'm not sure what the best practice here is. > Alternatively I might mention the issue in the man page. > > Not being able to tell which of the PTP port identifiers represents > which actual interface is something of a problem if you have more than > about two of them. Right, but part of namespacing is that processes inside the namespace should generally not know they are, and not be aware of anything outside the namespace... Hmm. > > > ___ > 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
Re: [Linuxptp-devel] [PATCH v2 0/2] Introduce per-port stats for received and transmitted messages
> -Original Message- > From: Petr Machata [mailto:pe...@mellanox.com] > Sent: Tuesday, September 10, 2019 5:24 AM > To: linuxptp-devel@lists.sourceforge.net > Cc: Petr Machata ; Keller, Jacob E > > Subject: [PATCH v2 0/2] Introduce per-port stats for received and transmitted > messages > > Black-box switches with PTP support commonly provide per-port statistics of > number of messages sent and received, split by the message type. Like other > statistics (ip link, ethtool, etc. etc.), network operators use the PTP > message stats to monitor the (PTP) network and debug issues. > > When ptp4l is used to turn a Linux machine (be it a switch or a host) into > a PTP clock, there is no easy way to get at these stats. It would certainly > be possible to parse ingressing and egressing traffic using e.g. a u32 > classifier, or create an ad-hoc eBPF-based tool, or something similar. But > all these approaches have to work hard to extract the knowledge that ptp4l > already has. ptp4l needs to parse the traffic anyway, and for transmitted > packets obviously knows what it is sending. It is thus the natural place > to place the stats. > > To that end, patch #1 introduces the message stats into linuxptp. A logical > way to obtain these stats is then through pmc, which is implemented in > patch #2, by way of a new TLV type. > Great! One thing (unrelated to this patch) I would love to see in the future is a bit more of an indepth "tutorial" style manpage or readme that explains how to use pmc, as opposed to trying to parse the output of the ptp4l log files all the time. I know we have the pmc man page, but something very introductory that is like a "if you're new to using ptp4l, here are some good practices..." It's something that I know I forget about a lot, and would definitely be useful for others who don't know as much, and might not even be away of management interface at all. Thanks, Jake > v2: > - Patch #1: > - Add MAX_MESSAGE_TYPES with comment instead of using a bare constant. > > Petr Machata (2): > port: Introduce per-port stats for received and transmitted messages > pmc: Add a new TLV to obtain per-port statistics > > ddt.h | 7 +++ > pmc.c | 47 +++ > pmc_common.c | 1 + > port.c | 32 ++-- > port_private.h | 1 + > tlv.c | 15 +++ > tlv.h | 6 ++ > 7 files changed, 107 insertions(+), 2 deletions(-) > > -- > 2.20.1 ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 1/2] port: Introduce per-port stats for received and transmitted messages
> -Original Message-
> From: Petr Machata [mailto:pe...@mellanox.com]
> Sent: Tuesday, September 10, 2019 5:24 AM
> To: linuxptp-devel@lists.sourceforge.net
> Cc: Petr Machata ; Keller, Jacob E
>
> Subject: [PATCH v2 1/2] port: Introduce per-port stats for received and
> transmitted
> messages
>
> Add struct PortStats to keep per-port number of messages sent and received,
> split by message type. Bump TX counters after messages are sent
> successfully, and RX counters after a message is received. To keep things
> simple, reserve one counter for each theoretically possible message type,
> including the reserved ones.
>
> Signed-off-by: Petr Machata
> ---
>
> Notes:
> v2:
> - Add MAX_MESSAGE_TYPES with comment instead of using a bare constant.
>
Great, thanks!
-Jake
> ddt.h | 7 +++
> port.c | 25 +++--
> port_private.h | 1 +
> 3 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/ddt.h b/ddt.h
> index 4acaa4f..56449a3 100644
> --- a/ddt.h
> +++ b/ddt.h
> @@ -100,4 +100,11 @@ struct FaultRecord {
> struct PTPText faultDescription;
> };
>
> +/* Four bits are dedicated to messageType field */
> +#define MAX_MESSAGE_TYPES 16
> +struct PortStats {
> + uint64_t rxMsgType[MAX_MESSAGE_TYPES];
> + uint64_t txMsgType[MAX_MESSAGE_TYPES];
> +};
> +
> #endif
> diff --git a/port.c b/port.c
> index 5a4a116..471e6f4 100644
> --- a/port.c
> +++ b/port.c
> @@ -580,6 +580,16 @@ static int path_trace_ignore(struct port *p, struct
> ptp_message *m)
> return 0;
> }
>
> +static void port_stats_inc_rx(struct port *p, const struct ptp_message *msg)
> +{
> + p->stats.rxMsgType[msg_type(msg)]++;
> +}
> +
> +static void port_stats_inc_tx(struct port *p, const struct ptp_message *msg)
> +{
> + p->stats.txMsgType[msg_type(msg)]++;
> +}
> +
> static int peer_prepare_and_send(struct port *p, struct ptp_message *msg,
>enum transport_event event)
> {
> @@ -595,6 +605,7 @@ static int peer_prepare_and_send(struct port *p, struct
> ptp_message *msg,
> if (cnt <= 0) {
> return -1;
> }
> + port_stats_inc_tx(p, msg);
> if (msg_sots_valid(msg)) {
> ts_add(>hwts.ts, p->tx_timestamp_offset);
> }
> @@ -2627,6 +2638,7 @@ static enum fsm_event bc_event(struct port *p, int
> fd_index)
> msg_put(msg);
> return EV_NONE;
> }
> + port_stats_inc_rx(p, msg);
> if (port_ignore(p, msg)) {
> msg_put(msg);
> return EV_NONE;
> @@ -2691,14 +2703,22 @@ int port_forward(struct port *p, struct ptp_message
> *msg)
> {
> int cnt;
> cnt = transport_send(p->trp, >fda, TRANS_GENERAL, msg);
> - return cnt <= 0 ? -1 : 0;
> + if (cnt <= 0) {
> + return -1;
> + }
> + port_stats_inc_tx(p, msg);
> + return 0;
> }
>
> int port_forward_to(struct port *p, struct ptp_message *msg)
> {
> int cnt;
> cnt = transport_sendto(p->trp, >fda, TRANS_GENERAL, msg);
> - return cnt <= 0 ? -1 : 0;
> + if (cnt <= 0) {
> + return -1;
> + }
> + port_stats_inc_tx(p, msg);
> + return 0;
> }
>
> int port_prepare_and_send(struct port *p, struct ptp_message *msg,
> @@ -2717,6 +2737,7 @@ int port_prepare_and_send(struct port *p, struct
> ptp_message *msg,
> if (cnt <= 0) {
> return -1;
> }
> + port_stats_inc_tx(p, msg);
> if (msg_sots_valid(msg)) {
> ts_add(>hwts.ts, p->tx_timestamp_offset);
> }
> diff --git a/port_private.h b/port_private.h
> index 9a5022d..5789fbb 100644
> --- a/port_private.h
> +++ b/port_private.h
> @@ -139,6 +139,7 @@ struct port {
> struct fault_interval flt_interval_pertype[FT_CNT];
> enum fault_type last_fault_type;
> unsigned intversionNumber; /*UInteger4*/
> + struct PortStatsstats;
> /* foreignMasterDS */
> LIST_HEAD(fm, foreign_clock) foreign_masters;
> /* TC book keeping */
> --
> 2.20.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 1/2] port: Introduce per-port stats for received and transmitted messages
> -Original Message-
> From: Petr Machata [mailto:pe...@mellanox.com]
> Sent: Wednesday, September 04, 2019 1:35 AM
> To: Keller, Jacob E
> Cc: linuxptp-devel@lists.sourceforge.net
> Subject: Re: [PATCH 1/2] port: Introduce per-port stats for received and
> transmitted
> messages
>
>
> Keller, Jacob E writes:
>
> >> diff --git a/ddt.h b/ddt.h
> >> index 4acaa4f..5486203 100644
> >> --- a/ddt.h
> >> +++ b/ddt.h
> >> @@ -100,4 +100,9 @@ struct FaultRecord {
> >>struct PTPText faultDescription;
> >> };
> >>
> >> +struct PortStats {
> >> + uint64_t rxMsgType[16];
> >> + uint64_t txMsgType[16];
> >
> > Is there some define we could use that represents this 16, instead of
> > just using the number? that would make it more clear why this is 16,
> > and possibly update if that ever changes in the future.
>
> messageType in Common Message Header has four bits (Table 18 in IEEE
> 1588-2008), hence 16 different messages can be encoded. There is no
> define for it in ptp4l, at least I haven't found any. I can add one if
> you prefer, or add an explanatory comment.
I'd appreciate one of those two options.
Thanks,
Jake
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
