Re: [Linuxptp-devel] [PATCH v2 0/2] fix fallback clock_gettime for test_phc

2021-11-24 Thread Miroslav Lichvar
On Tue, Nov 23, 2021 at 10:41:11PM +, Keller, Jacob E wrote:
> On 10/25/2021 12:25 AM, Miroslav Lichvar wrote:
> > 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?

Yes, the reverse tree order, which I think is preferred in this
project.

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] Sudden phc offset jump

2021-11-02 Thread Miroslav Lichvar
On Tue, Nov 02, 2021 at 09:02:19AM +, ramesh t via Linuxptp-devel wrote:
> hi,
> 
> We are using ptp3.0 version in C3 mode. Both ptp4l and phc2sys (to sync 
> system clock) are running fine.
> 
> Ptp4l rms value is in single digits and we see a sudden jump in phc offset as 
> captured below resulting in date change.
> 
> Oct 26 19:15:48 phc2sys_slave: [1177711.923] CLOCK_REALTIME phc offset -97 s2 
> freq +3454 delay 2479
> Oct 26 19:15:49 ptp4l_slave: [1177712.827] rms 3 max 7 freq -2363 +/- 5 delay 
> 86 +/- 1
> Oct 26 19:15:49 phc2sys_slave: [1177712.924] CLOCK_REALTIME phc offset -61 s2 
> freq +3461 delay 2433
> Oct 12 05:18:20 ptp4l_slave: [1177713.819] rms 3 max 8 freq -2364 +/- 5 delay 
> 86 +/- 1
> Oct 19 08:03:31 phc2sys_slave: [1177713.924] clockcheck: clock jumped 
> backward or running slower than expected!
> Oct 19 08:03:31 phc2sys_slave: [1177713.924] CLOCK_REALTIME phc offset 
> -645138994849912 s0 freq +3461 delay 2478
> Oct 25 18:44:33 ptp4l_slave: [1177714.811] rms 3 max 6 freq -2361 +/- 4 delay 
> 86 +/- 1
> Sep 30 01:35:47 phc2sys_slave: [1177714.924] clockcheck: clock jumped 
> backward or running slower than expected!
> Sep 30 01:35:47 phc2sys_slave: [1177714.924] CLOCK_REALTIME phc offset 
> -2310003858831797 s0 freq +3461 delay 2472
> Oct 12 07:16:54 ptp4l_slave: [1177715.803] rms 3 max 5 freq -2362 +/- 5 delay 
> 87 +/- 1
> 
> Is there any known issues with ptp3.0 version? Please suggest.

What hardware do you use? It's most likely a driver bug or something
else (e.g. an NTP client) is trying to control the clock.

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] [RFC PATCH 00/10] checksync: new program to wait for a local or remote clock to synchronize

2021-11-01 Thread Miroslav Lichvar
On Fri, Oct 29, 2021 at 03:35:30PM +0300, Vladimir Oltean wrote:
> If I look at the struct timex definition from "man adjtimex", I see that
> both maxerror and esterror are expressed in microseconds (i.e. STA_NANO
> does not affect them), is that right?

Yes, it's only microsecond resolution, but I think a new flag could be
implemented for better resolution if necessary.

-- 
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/2] fix fallback clock_gettime for test_phc

2021-10-25 Thread Miroslav Lichvar
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.

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] [RFC PATCH 00/10] checksync: new program to wait for a local or remote clock to synchronize

2021-10-14 Thread Miroslav Lichvar
On Thu, Oct 14, 2021 at 05:11:36PM +0300, Vladimir Oltean wrote:
> On Thu, Oct 14, 2021 at 09:44:21AM +0200, Miroslav Lichvar wrote:
> > I think it would be great if all PTP clocks had this status and
> > esterror/maxerror fields. For example, with the ptp_kvm driver guests
> > have access to the system clock of the host as a PHC, but there is no
> > way to check if/how well the clock is actually synchronized.
> > 
> > I had this on my todo list for a long time, but wasn't able to look
> > into it yet.
> 
> There are clearly some merits to this idea, but I don't know if it would
> cater to the use case where you want to send some traffic on a port that
> is a PTP master, and you want to know whether the slaves attached to you
> are synchronized?

No, this is meant only for distributing the information locally within
a system or possibly between a VM host and guest.

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] [RFC PATCH 00/10] checksync: new program to wait for a local or remote clock to synchronize

2021-10-14 Thread Miroslav Lichvar
On Wed, Oct 13, 2021 at 08:37:27PM +0300, Vladimir Oltean wrote:
> The other topic is how to actually wait for phc2sys to synchronize,
> since the program presented here only works with ptp4l (or at least
> that's all I tested).

If it is the system clock, phc2sys clears the STA_UNSYNC flag
(which is reported by ntp_gettime()/ntp_adjtime()) when the servo is
in the locked state. There is the esterror (and potentially also
maxerror) field, which could be set by ptp4l/phc2sys to some estimate
of the error. Any application could easily check the value to decide
if the clock is good enough for its purposes. No need to implement PTP
or have access to the ptp4l/phc2sys socket.

I think it would be great if all PTP clocks had this status and
esterror/maxerror fields. For example, with the ptp_kvm driver guests
have access to the system clock of the host as a PHC, but there is no
way to check if/how well the clock is actually synchronized.

I had this on my todo list for a long time, but wasn't able to look
into it yet.

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] [PATCH 2/2] Maintain one Sync sequence counter per destination address.

2021-10-12 Thread Miroslav Lichvar
On Mon, Oct 11, 2021 at 06:40:31AM -0700, Richard Cochran wrote:
> According to IEEE 1588, each destination should have its own, unique
> message sequence with respect to the sequenceId field.  The current
> code will generate skips in the number sequence in the presence of
> unicast clients.  Fix the issue by giving each client its own
> sequence of Sync messages.

> @@ -1557,7 +1557,7 @@ int port_tx_sync(struct port *p, struct address *dst)
>   msg->header.messageLength  = sizeof(struct sync_msg);
>   msg->header.domainNumber   = clock_domain_number(p->clock);
>   msg->header.sourcePortIdentity = p->portIdentity;
> - msg->header.sequenceId = p->seqnum.sync++;
> + msg->header.sequenceId = sequence_id;
>   msg->header.control= CTL_SYNC;
>   msg->header.logMessageInterval = p->logSyncInterval;

There is a missing change in this function for the follow up message.
It needs to use the same sequence ID.

Other than that, both patches look good to me.

Thanks,

-- 
Miroslav Lichvar



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


[Linuxptp-devel] [PATCH] clock: Split update of leap status from clock_time_properties().

2021-10-06 Thread Miroslav Lichvar
Add a separate function for the update of the grandmaster's state after
a leap second to avoid making modifications in clock_time_properties()
and call it on each master announce tx timeout.

Suggested-by: Richard Cochran 
Signed-off-by: Miroslav Lichvar 
---
 clock.c | 8 ++--
 clock.h | 7 +++
 port.c  | 1 +
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/clock.c b/clock.c
index 61c9563..bbfd1a8 100644
--- a/clock.c
+++ b/clock.c
@@ -1897,7 +1897,7 @@ void clock_sync_interval(struct clock *c, int n)
servo_sync_interval(c->servo, n < 0 ? 1.0 / (1 << -n) : 1 << n);
 }
 
-static void clock_update_utc_offset(struct clock *c)
+void clock_update_leap_status(struct clock *c)
 {
struct timespec ts;
int leap;
@@ -1932,11 +1932,7 @@ static void clock_update_utc_offset(struct clock *c)
 
 struct timePropertiesDS clock_time_properties(struct clock *c)
 {
-   struct timePropertiesDS tds;
-
-   clock_update_utc_offset(c);
-
-   tds = c->tds;
+   struct timePropertiesDS tds = c->tds;
 
switch (c->local_sync_uncertain) {
case SYNC_UNCERTAIN_DONTCARE:
diff --git a/clock.h b/clock.h
index e2a3e36..0534f21 100644
--- a/clock.h
+++ b/clock.h
@@ -339,6 +339,13 @@ enum servo_state clock_synchronize(struct clock *c, tmv_t 
ingress,
  */
 void clock_sync_interval(struct clock *c, int n);
 
+/**
+ * Update the clock leap bits and UTC offset after a leap second
+ * if operating as a grandmaster.
+ * @param c  The clock instance.
+ */
+void clock_update_leap_status(struct clock *c);
+
 /**
  * Obtain a clock's time properties data set.
  * @param c  The clock instance.
diff --git a/port.c b/port.c
index d5119b7..6e6b0aa 100644
--- a/port.c
+++ b/port.c
@@ -2705,6 +2705,7 @@ static enum fsm_event bc_event(struct port *p, int 
fd_index)
case FD_MANNO_TIMER:
pr_debug("%s: master tx announce timeout", p->log_name);
port_set_manno_tmo(p);
+   clock_update_leap_status(p->clock);
return port_tx_announce(p, NULL) ? EV_FAULT_DETECTED : EV_NONE;
 
case FD_SYNC_TX_TIMER:
-- 
2.26.3



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


Re: [Linuxptp-devel] [PATCH 3/4] clock: Clear leap flags after leap second.

2021-10-05 Thread Miroslav Lichvar
On Mon, Oct 04, 2021 at 06:56:50AM -0700, Richard Cochran wrote:
> How about this?
> 
> 1. Refactor the setting of clock.tds into a common helper function, as
>you suggest.
> 
> 2. Provide a global function to clear the LS flags, called from
>port_tx_announce explicitly.
> 
>This could be clock_update_utc_offset(), but the name might suggest
>that the clock would be stepped.  Maybe clock_clear_leap_flags()
>instead?

Or maybe clock_update_leap_status()? I have no strong preference for
any of the three.

> 3. And BTW, the naming of clock_update_grandmaster() isn't great.
>Long ago I couldn't think of a better name at the time.  It sounds
>like it would update something about a remote GM.

I think it's consistent with clock_update_slave(). If one is renamed,
should the other follow?

>Rename clock_update_grandmaster() to clock_reset_local_gm().

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] [PATCH 3/4] clock: Clear leap flags after leap second.

2021-10-04 Thread Miroslav Lichvar
On Sat, Oct 02, 2021 at 07:09:02AM -0700, Richard Cochran wrote:
> The function, clock_time_properties, is a query that should not change
> the program state.

So, would you like to move it to a new function, e.g.
clock_update_time_properties()?

> With this change, one call chain will be:
> 
> port_tx_announce
>  clock_time_properties
>   clock_update_utc_offset
>clock_update_grandmaster
> 
> That clobbers the grandmasterIdentity with the local dds.clockIdentity
> in case of BC.

The function specifically checks for grandmaster:

> > +   /* Don't do anything if not a grandmaster with a leap flag set. */
> > +   if (c->cur.stepsRemoved > 0 || leap == 0) {
> > +   return;
> > +   }
> > +

If it's a BC, stepsRemoved should be larger than zero, right? I
thought that was easier than looking for a port in the slave state.

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] [PATCH 1/4] clock: Accept new UTC offset after leap second.

2021-09-29 Thread Miroslav Lichvar
On Mon, Sep 27, 2021 at 10:34:30AM -0700, Richard Cochran wrote:
> On Wed, Sep 15, 2021 at 12:08:01PM +0200, Miroslav Lichvar wrote:
> > When the master updated the UTC offset and leap flags in an announce
> > message after a leap second, only the new flags were accepted for
> > synchronization of the local clock, which caused a one-second error to
> > appear until a state decision event updated the UTC offset.
> 
> The once second error appeared only in the case of ptp4l using SW
> time stamping and controlling clock_realtime directly, right?

I think it impacts also ptp4l using HW timestamping in a non-TAI PTP
domain.

> Or does this affect ptp4l->PHC + phc2sys PHC->clock_realtime as well?

It shouldn't when when PTP is in TAI as phc2sys updates the UTC offset
and leap flags at the same time when it gets TIME_PROPERTIES_DATA_SET.

-- 
Miroslav Lichvar



___
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

2021-09-21 Thread Miroslav Lichvar
On Tue, Sep 21, 2021 at 12:31:57PM +0300, Joseph Matan wrote:
> >
> > Enabling the weighted filtered mode can help, but not as much as
> > disabling the filter completely, at least from what I have seen in my
> > tests.
> >
> I guess it depends on the environment (devices and switches in the between
> and traffic),
> but from my experience I've received better results when I worked with the
> percentile filter in weighted filtered mode,

Can you show your results?

Here is my test comparing the median filter with 0.1 percentile and no
filter in a network with an asymmetrically distributed delay (mean 10
microseconds).

https://fedorapeople.org/~mlichvar/tmp/ptp/delayfilter.png

As you can see, the percentile filter added a significant asymmetry of
about 4 microseconds. In terms of RMS time error, it's about 3x worse
than the median filter and about 5x worse than no filter.

The shared part of the config was:
delay_filter_length 11
pi_proportional_const 0.04
pi_integral_const 0.00024

> and even much better results when I used my own weighted filter
> implementation (which I also hope to share soon).

Well, in that case I think these changes would better be submitted and
reviewed together.

-- 
Miroslav Lichvar



___
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

2021-09-20 Thread Miroslav Lichvar
On Sun, Sep 19, 2021 at 04:15:15PM +0300, Joseph Matan wrote:
> >
> >
> > > With that better estimate of delay, what do you do with the sync
> > > messages? There is no filter for them that would compensate this
> > > asymmetry, so I think this will make a negative impact on accuracy of
> > > the clock.
> >
> 
> I think there is no point in using this filter if you don't use weights
> either.

Enabling the weighted filtered mode can help, but not as much as
disabling the filter completely, at least from what I have seen in my
tests.

> But isn't that the same with the median filter ?

The median filter expects the network delay to have a symmetric
distribution. That should work well with full hardware support on the
NIC and network.

With the percentile filter you expect an asymmetric distribution. The
reported delay is more stable, but the clock is less accurate. What is
the use case here? Why not just disable the filter?

> I did it for optimization.
> After m->cnt reaches the value of m->len, there is no need to re-calculate
> m->percentile_index anymore (so the function needs to "remember" the value
> of m->percentile_index).

Saving a single multiplication? I don't think it's worth it. 

-- 
Miroslav Lichvar



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


[Linuxptp-devel] [PATCH 2/4] clock: Print info message when leap flags change.

2021-09-15 Thread Miroslav Lichvar
Log an info message when the LEAP_61 or LEAP_59 flag changes in the
accepted time properties.

Signed-off-by: Miroslav Lichvar 
---
 clock.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/clock.c b/clock.c
index f3aa616..c681cbf 100644
--- a/clock.c
+++ b/clock.c
@@ -1912,6 +1912,11 @@ struct timePropertiesDS clock_time_properties(struct 
clock *c)
 
 void clock_update_time_properties(struct clock *c, struct timePropertiesDS tds)
 {
+   if ((tds.flags ^ c->tds.flags) & (LEAP_61 | LEAP_59)) {
+   pr_info("updating time properties to %s leap second",
+   tds.flags & (LEAP_61 | LEAP_59) ?
+   (tds.flags & LEAP_61 ? "insert" : "delete") : "no");
+   }
if ((tds.flags & UTC_OFF_VALID && tds.flags & TIME_TRACEABLE &&
 tds.currentUtcOffset != c->utc_offset) ||
tds.currentUtcOffset > c->utc_offset) {
-- 
2.26.3



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


[Linuxptp-devel] [PATCH 3/4] clock: Clear leap flags after leap second.

2021-09-15 Thread Miroslav Lichvar
When operating as a grandmaster, clear the programmed leap second and
update the UTC offset when the first announce message after the leap
second is sent. This guarantees the update happens in +/- 2 announce
messages around the UTC midnight as required by the specification and
removes the responsibility from the external process which programmed
the leap second with the GRANDMASTER_SETTINGS_NP management message.

The code reads the clock directly instead of using a packet timestamp to
not depend on (timing of) sync messages.

Signed-off-by: Miroslav Lichvar 
---
 clock.c | 39 ++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/clock.c b/clock.c
index c681cbf..c6646c3 100644
--- a/clock.c
+++ b/clock.c
@@ -1892,9 +1892,46 @@ void clock_sync_interval(struct clock *c, int n)
servo_sync_interval(c->servo, n < 0 ? 1.0 / (1 << -n) : 1 << n);
 }
 
+static void clock_update_utc_offset(struct clock *c)
+{
+   struct timespec ts;
+   int leap;
+
+   if (c->tds.flags & LEAP_61) {
+   leap = 1;
+   } else if (c->tds.flags & LEAP_59) {
+   leap = -1;
+   } else {
+   leap = 0;
+   }
+
+   /* Don't do anything if not a grandmaster with a leap flag set. */
+   if (c->cur.stepsRemoved > 0 || leap == 0) {
+   return;
+   }
+
+   clock_gettime(c->clkid, );
+   if (c->time_flags & PTP_TIMESCALE) {
+   ts.tv_sec -= c->utc_offset;
+   }
+
+   /* Wait until the leap second has passed. */
+   if (leap_second_status(tmv_to_nanoseconds(timespec_to_tmv(ts)),
+  leap, , >utc_offset)) {
+   return;
+   }
+
+   c->time_flags &= ~(LEAP_61 | LEAP_59);
+   clock_update_grandmaster(c);
+}
+
 struct timePropertiesDS clock_time_properties(struct clock *c)
 {
-   struct timePropertiesDS tds = c->tds;
+   struct timePropertiesDS tds;
+
+   clock_update_utc_offset(c);
+
+   tds = c->tds;
 
switch (c->local_sync_uncertain) {
case SYNC_UNCERTAIN_DONTCARE:
-- 
2.26.3



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


[Linuxptp-devel] [PATCH 1/4] clock: Accept new UTC offset after leap second.

2021-09-15 Thread Miroslav Lichvar
When the master updated the UTC offset and leap flags in an announce
message after a leap second, only the new flags were accepted for
synchronization of the local clock, which caused a one-second error to
appear until a state decision event updated the UTC offset. Before
commit 14a97dc5da47 ("Remove redundant test on the UTC flags."), the UTC
offset was taken directly from the clock's timePropertiesDS.

Move the update of the UTC offset from clock_update_slave() to
clock_update_time_properties() to enable a synchronous update of the
flags and offset on any announce message.

Signed-off-by: Miroslav Lichvar 
Fixes: 14a97dc5da47 ("Remove redundant test on the UTC flags.")
---
 clock.c | 25 ++---
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/clock.c b/clock.c
index ec70f91..f3aa616 100644
--- a/clock.c
+++ b/clock.c
@@ -684,6 +684,7 @@ static void clock_update_grandmaster(struct clock *c)
 static void clock_update_slave(struct clock *c)
 {
struct parentDS *pds = >dad.pds;
+   struct timePropertiesDS tds;
struct ptp_message *msg;
 
if (!c->best)
@@ -696,21 +697,16 @@ static void clock_update_slave(struct clock *c)
pds->grandmasterClockQuality   = msg->announce.grandmasterClockQuality;
pds->grandmasterPriority1  = msg->announce.grandmasterPriority1;
pds->grandmasterPriority2  = msg->announce.grandmasterPriority2;
-   c->tds.currentUtcOffset= msg->announce.currentUtcOffset;
-   c->tds.flags   = msg->header.flagField[1];
-   c->tds.timeSource  = msg->announce.timeSource;
-   if (!(c->tds.flags & PTP_TIMESCALE)) {
+   tds.currentUtcOffset   = msg->announce.currentUtcOffset;
+   tds.flags  = msg->header.flagField[1];
+   tds.timeSource = msg->announce.timeSource;
+   if (!(tds.flags & PTP_TIMESCALE)) {
pr_warning("foreign master not using PTP timescale");
}
-   if (c->tds.currentUtcOffset < c->utc_offset) {
+   if (tds.currentUtcOffset < c->utc_offset) {
pr_warning("running in a temporal vortex");
}
-   if (((c->tds.flags & UTC_OFF_VALID && c->tds.flags & TIME_TRACEABLE) &&
-   (c->tds.currentUtcOffset != c->utc_offset)) ||
-   (c->tds.currentUtcOffset > c->utc_offset)) {
-   pr_info("updating UTC offset to %d", c->tds.currentUtcOffset);
-   c->utc_offset = c->tds.currentUtcOffset;
-   }
+   clock_update_time_properties(c, tds);
 }
 
 static int clock_utc_correct(struct clock *c, tmv_t ingress)
@@ -1916,6 +1912,13 @@ struct timePropertiesDS clock_time_properties(struct 
clock *c)
 
 void clock_update_time_properties(struct clock *c, struct timePropertiesDS tds)
 {
+   if ((tds.flags & UTC_OFF_VALID && tds.flags & TIME_TRACEABLE &&
+tds.currentUtcOffset != c->utc_offset) ||
+   tds.currentUtcOffset > c->utc_offset) {
+   pr_info("updating UTC offset to %d", tds.currentUtcOffset);
+   c->utc_offset = tds.currentUtcOffset;
+   }
+
c->tds = tds;
 }
 
-- 
2.26.3



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


[Linuxptp-devel] [PATCH 4/4] clock: Notify servo about leap second on UTC hardware clock.

2021-09-15 Thread Miroslav Lichvar
Don't limit servo handling of leap seconds to the system clock. If the
domain doesn't use the PTP timescale, enable a controlled correction in
the servo even if it synchronizes a hardware clock.

Signed-off-by: Miroslav Lichvar 
---
 clock.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/clock.c b/clock.c
index c6646c3..5a20059 100644
--- a/clock.c
+++ b/clock.c
@@ -715,7 +715,7 @@ static int clock_utc_correct(struct clock *c, tmv_t ingress)
int utc_offset, leap, clock_leap;
uint64_t ts;
 
-   if (!c->utc_timescale)
+   if (!c->utc_timescale && c->tds.flags & PTP_TIMESCALE)
return 0;
 
utc_offset = c->utc_offset;
@@ -729,7 +729,7 @@ static int clock_utc_correct(struct clock *c, tmv_t ingress)
}
 
/* Handle leap seconds. */
-   if ((leap || c->leap_set) && c->clkid == CLOCK_REALTIME) {
+   if (leap || c->leap_set) {
/* If the clock will be stepped, the time stamp has to be the
   target time. Ignore possible 1 second error in utc_offset. */
if (c->servo_state == SERVO_UNLOCKED) {
@@ -750,7 +750,7 @@ static int clock_utc_correct(struct clock *c, tmv_t ingress)
clock_leap = leap_second_status(ts, c->leap_set,
, _offset);
if (c->leap_set != clock_leap) {
-   if (c->kernel_leap)
+   if (c->kernel_leap && c->clkid == CLOCK_REALTIME)
sysclk_set_leap(clock_leap);
else
servo_leap(c->servo, clock_leap);
-- 
2.26.3



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


[Linuxptp-devel] [PATCH 0/4] Leap second fix and improvements.

2021-09-15 Thread Miroslav Lichvar
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.

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(-)

-- 
2.26.3



___
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

2021-09-13 Thread Miroslav Lichvar
On Sat, Sep 11, 2021 at 12:33:43PM +0300, 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.

With that better estimate of delay, what do you do with the sync
messages? There is no filter for them that would compensate this
asymmetry, so I think this will make a negative impact on accuracy of
the clock.

If you insist on using PTP in a non-PTP network, it is better to
disable the delay filter, i.e. set tsproc to a raw mode.

Few comments about the patch below, if there is a use case for it.

> @@ -238,6 +239,7 @@ struct config_item config_tab[] = {
>   PORT_ITEM_INT("delayAsymmetry", 0, INT_MIN, INT_MAX),
>   PORT_ITEM_ENU("delay_filter", FILTER_MOVING_MEDIAN, delay_filter_enu),
>   PORT_ITEM_INT("delay_filter_length", 10, 1, INT_MAX),
> + PORT_ITEM_DBL("delay_filter_percentile", 0.5, 0.0, 1.0),

I'd expect the default percentile to be different from 0.5 (which is
already the default with the default median filter).

> +struct mpercentile {
>   struct filter filter;
>   int cnt;
>   int len;
>   int index;
> + double percentile;
> + int percentile_index;

Why is percentile_index here? It seems it's needed only in one
function.

> - if (m->cnt % 2)
> - return m->samples[m->order[m->cnt / 2]];
> - else
> - return tmv_div(tmv_add(m->samples[m->order[m->cnt / 2 - 1]],
> -m->samples[m->order[m->cnt / 2]]), 2);
> + return m->samples[m->order[m->percentile_index]];

I'd prefer if the median filter was a special case here to not change
the behavior for even lengths (the default is 10).

-- 
Miroslav Lichvar



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


[Linuxptp-devel] Setting asCapable with pmc

2021-09-07 Thread Miroslav Lichvar
I was trying some tests with the pmc SET command and I noticed that
for PORT_DATA_SET_NP it parses the asCapable argument, but ptp4l
ignores the value. The neighborPropDelayThresh value is accepted. Is
this intentional?

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] [RFC] ptp4l: improved-accuracy hook

2021-08-05 Thread Miroslav Lichvar
On Thu, Aug 05, 2021 at 06:44:00AM -0700, Richard Cochran wrote:
> On Thu, Aug 05, 2021 at 06:40:35AM -0700, Richard Cochran wrote:
> > Of course you are welcome to hack it into ptp4l for your research.
> 
> And once you have the AI algorithm perfected, simply code the result
> into the device driver as Jacob suggested.

I'm not sure if it's written anywhere or enforced, but there is an
expectation that from the clock's point of view RX timestamps should
never be before start of the reception and TX timestamps should never
be later than start of the transmission.

If this algorithm that is being researched cannot keep the error on
the right side, I think it shouldn't be implemented in the driver, or
at least it should be disabled by default, as it would break
assumptions in applications that try to estimate the maximum error
(e.g. NTP). I'd prefer to keep things like that in the application, or
maybe as a library, if it could be useful to multiple applications.

Is this about mlx5 NICs?

-- 
Miroslav Lichvar



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


[Linuxptp-devel] [PATCH] lstab: Close file after reading.

2021-07-20 Thread Miroslav Lichvar
The lstab_read() function opens a file, but doesn't close it after use.

Signed-off-by: Miroslav Lichvar 
---
 lstab.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lstab.c b/lstab.c
index e6e7ad2..0d6a427 100644
--- a/lstab.c
+++ b/lstab.c
@@ -144,6 +144,7 @@ static int lstab_read(struct lstab *lstab, const char *name)
index++;
}
}
+   fclose(fp);
if (!lstab->expiration_utc) {
fprintf(stderr, "missing expiration date in '%s'\n", name);
return -1;
-- 
2.26.3



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


[Linuxptp-devel] [PATCH] Fix quoting in ptp4l man page.

2021-07-19 Thread Miroslav Lichvar
In the groff syntax lines starting with a dot or quote are requests. A
line in the servo_offset_threshold description starts with a quote,
which breaks the output. Move a word to the beginning of the line to fix
it.

Signed-off-by: Miroslav Lichvar 
---
 ptp4l.8 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ptp4l.8 b/ptp4l.8
index 7ca3474..a0779ef 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -788,8 +788,8 @@ The default value is 10.
 .TP
 .B servo_offset_threshold
 The offset threshold used in order to transition from the SERVO_LOCKED
-to the SERVO_LOCKED_STABLE state.  The transition occurs once the last
-'servo_num_offset_values' offsets are all below the threshold value.
+to the SERVO_LOCKED_STABLE state.  The transition occurs once the
+last 'servo_num_offset_values' offsets are all below the threshold value.
 The default value of offset_threshold is 0 (disabled).
 .TP
 .B slave_event_monitor
-- 
2.26.3



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


Re: [Linuxptp-devel] [PATCH] config: Add workaround for glibc getopt_long().

2021-07-19 Thread Miroslav Lichvar
On Sun, Jul 18, 2021 at 07:46:50PM -0700, Richard Cochran wrote:
> On Tue, Jul 13, 2021 at 10:31:16AM +0200, Miroslav Lichvar wrote:
> > getopt_long() in glibc allows shortened long option names, e.g.
> > ptp4l --domain works as --domainNumber. When the match is ambiguous,
> > e.g. --fault matches --fault_badpeernet_interval and
> > --fault_reset_interval, it is supposed to return an error, but that
> > works only if their struct option have different flags or vals.
> 
> I don't get it... even with your change, all of the 'vals' are zero.
> There are not different from one and other.
> 
> > https://sourceware.org/bugzilla/show_bug.cgi?id=28081
> 
> The last comment says, "you need to use a different val".

The getopt code actually checks all three fields (has_arg, flag and
val), not just val. I'll note that in the bug report.

-- 
Miroslav Lichvar



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


[Linuxptp-devel] [PATCHv2 RFC 0/6] Support for dropping root privileges

2021-07-13 Thread Miroslav Lichvar
v2:
- keep more capabilities (raw sockets, timestamping configuration)
- copy ownership of server UDS
- follow location of server UDS for client sockets
- support operation with NTPSHM servo
- move default UDS addresses to /var/run/linuxptp

This patchset implements a support for dropping all root privileges
except four capabilities that are needed to work with clocks and (raw)
sockets. This should limit impact of security issues.

A new option is added to ptp4l/phc2sys/pmc to specify the username to
which should be the process switch.

I tried few different approaches with the configuration. I think this
one will be least problematic for migrations and user experience. A
pmc/phc2sys running under root can still work with non-root ptp4l. There
are some cases which requires the permissions of the PTP clocks to be
set up for the operation as they cannot be opened before root is
dropped.

The default location of all UDS sockets is changed to /var/run/linuxptp.
The directory is created by ptp4l on first start. If the user
configuration of ptp4l is changed, the directory has to be removed.

In the v1 discussion there was a suggestion to not use libcap. I have
not looked into that yet, but I can try it if this whole thing makes
sense otherwise.

Miroslav Lichvar (6):
  util: Add functions for dropping root privileges.
  uds: Copy ownership of server socket.
  clock: Add support for dropping root privileges.
  pmc: Add support for dropping root privileges.
  phc2sys: Add support for dropping root privileges.
  config: move default UDS addresses to /var/run/linuxptp.

 clock.c |  29 +---
 config.c|   5 +-
 configs/default.cfg |   4 +-
 incdefs.sh  |  11 -
 makefile|   4 ++
 phc2sys.8   |  14 +-
 phc2sys.c   |  31 +++--
 pmc.8   |  15 --
 pmc.c   |  12 +++--
 pmc_common.c|  32 +
 ptp4l.8 |  14 +-
 uds.c   |   9 
 util.c  | 111 
 util.h  |  19 
 14 files changed, 283 insertions(+), 27 deletions(-)

-- 
2.26.3



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


[Linuxptp-devel] [PATCHv2 RFC 4/6] pmc: Add support for dropping root privileges.

2021-07-13 Thread Miroslav Lichvar
Drop root before binding the UDS socket and change the default UDS
address to follow to location of the server UDS.

Signed-off-by: Miroslav Lichvar 
---
 pmc.8| 13 +++--
 pmc.c| 10 --
 pmc_common.c | 32 
 3 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/pmc.8 b/pmc.8
index e0ab5ac..c6ee42f 100644
--- a/pmc.8
+++ b/pmc.8
@@ -81,8 +81,9 @@ Specify the boundary hops value in sent messages. The default 
is 1.
 Specify the domain number in sent messages. The default is 0.
 .TP
 .BI \-i " interface"
-Specify the network interface. The default is /var/run/pmc.$pid for the Unix 
Domain
-Socket transport and eth0 for the other transports.
+Specify the network interface. The default is pmc.$pid for the Unix Domain
+Socket transport and eth0 for the other transports. A relative path of the Unix
+Domain socket is relative to the directory of the server's socket.
 .TP
 .BI \-s " uds-address"
 Specifies the address of the server's UNIX domain socket.
@@ -144,6 +145,14 @@ options. The name of the section is the name of the 
configured port (e.g.
 .B domainNumber
 The domain attribute of the local clock. The default is 0.
 
+.TP
+.B user
+The name of the user to which should
+.B pmc
+switch in order to drop the root privileges. By default,
+.B pmc
+will keep the identity of the user under which it is started.
+
 .SH PORT OPTIONS
 .TP
 .B transportSpecific
diff --git a/pmc.c b/pmc.c
index 00d6014..194e450 100644
--- a/pmc.c
+++ b/pmc.c
@@ -546,7 +546,7 @@ static void usage(char *progname)
" -f [file] read configuration from 'file'\n"
" -hprints this message and exits\n"
" -i [dev]  interface device to use, default 'eth0'\n"
-   "   for network and '/var/run/pmc.$pid' for UDS.\n"
+   "   for network and 'pmc.$pid' for UDS.\n"
" -s [path] server address for UDS, default '/var/run/ptp4l'.\n"
" -t [hex]  transport specific field, default 0x0\n"
" -vprints the software version and exits\n"
@@ -680,8 +680,9 @@ int main(int argc, char *argv[])
 
if (!iface_name) {
if (transport_type == TRANS_UDS) {
+   /* Directory will be copied from ptp4l address */
snprintf(uds_local, sizeof(uds_local),
-"/var/run/pmc.%d", getpid());
+"pmc.%d", getpid());
iface_name = uds_local;
} else {
iface_name = "eth0";
@@ -695,6 +696,11 @@ int main(int argc, char *argv[])
print_set_syslog(1);
print_set_verbose(1);
 
+   if (drop_root_privileges(config_get_string(cfg, NULL, "user"))) {
+   config_destroy(cfg);
+   return -1;
+   }
+
pmc = pmc_create(cfg, transport_type, iface_name, boundary_hops,
 domain_number, transport_specific, zero_datalen);
if (!pmc) {
diff --git a/pmc_common.c b/pmc_common.c
index 101df55..fc2fbd8 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -18,6 +18,7 @@
  * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
  */
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -373,11 +374,39 @@ struct pmc {
int zero_length_gets;
 };
 
+static int complete_uds_address(struct config *cfg, const char *iface,
+   char *uds, size_t len)
+{
+   char *dir, buf[MAX_IFNAME_SIZE + 1];
+
+   /* Don't change absolute paths */
+   if (iface[0] == '/') {
+   if (snprintf(uds, len, "%s", iface) >= len) {
+   pr_err("UDS path too long");
+   return -1;
+   }
+   return 0;
+   }
+
+   /* Relative paths are relative to the directory of the server socket */
+   if (snprintf(buf, sizeof(buf), "%s",
+config_get_string(cfg, NULL,
+  "uds_address")) >= sizeof(buf) ||
+   !(dir = dirname(buf)) ||
+   snprintf(uds, len, "%s/%s", buf, iface) >= len) {
+   pr_err("UDS path too long");
+   return -1;
+   }
+
+   return 0;
+}
+
 struct pmc *pmc_create(struct config *cfg, enum transport_type transport_type,
   const char *iface_name, UInteger8 boundary_hops,
   UInteger8 domain_number, UInteger8 transport_specific,
   int zero_datalen)
 {
+   char uds[MAX_IFNAME_SIZE + 1];
struct pmc *pmc;
 
pmc = calloc(1, sizeof *pmc);
@@ -386,6 +415,9 @@ struct pmc *pmc_create(struct config *cfg, enum 
transport_type transport_type,
 

[Linuxptp-devel] [PATCHv2 RFC 6/6] config: move default UDS addresses to /var/run/linuxptp.

2021-07-13 Thread Miroslav Lichvar
/var/run cannot be used by non-root users to bind and unlink sockets.
Move the default server UDS addresses to /var/run/linuxptp, which will be
created by ptp4l the first time it runs.

Signed-off-by: Miroslav Lichvar 
---
 config.c| 4 ++--
 configs/default.cfg | 4 ++--
 phc2sys.8   | 4 ++--
 phc2sys.c   | 2 +-
 pmc.8   | 2 +-
 pmc.c   | 2 +-
 ptp4l.8 | 4 ++--
 7 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index 06e5fd1..bc112aa 100644
--- a/config.c
+++ b/config.c
@@ -326,8 +326,8 @@ struct config_item config_tab[] = {
GLOB_ITEM_INT("tx_timestamp_timeout", 1, 1, INT_MAX),
PORT_ITEM_INT("udp_ttl", 1, 1, 255),
PORT_ITEM_INT("udp6_scope", 0x0E, 0x00, 0x0F),
-   GLOB_ITEM_STR("uds_address", "/var/run/ptp4l"),
-   GLOB_ITEM_STR("uds_ro_address", "/var/run/ptp4lro"),
+   GLOB_ITEM_STR("uds_address", "/var/run/linuxptp/ptp4l"),
+   GLOB_ITEM_STR("uds_ro_address", "/var/run/linuxptp/ptp4lro"),
PORT_ITEM_INT("unicast_listen", 0, 0, 1),
PORT_ITEM_INT("unicast_master_table", 0, 0, INT_MAX),
PORT_ITEM_INT("unicast_req_duration", 3600, 10, INT_MAX),
diff --git a/configs/default.cfg b/configs/default.cfg
index 64ef3bd..a739b08 100644
--- a/configs/default.cfg
+++ b/configs/default.cfg
@@ -90,8 +90,8 @@ ptp_dst_mac   01:1B:19:00:00:00
 p2p_dst_mac01:80:C2:00:00:0E
 udp_ttl1
 udp6_scope 0x0E
-uds_address/var/run/ptp4l
-uds_ro_address /var/run/ptp4lro
+uds_address/var/run/linuxptp/ptp4l
+uds_ro_address /var/run/linuxptp/ptp4lro
 #
 # Default interface options
 #
diff --git a/phc2sys.8 b/phc2sys.8
index a829874..4a0151a 100644
--- a/phc2sys.8
+++ b/phc2sys.8
@@ -210,7 +210,7 @@ option is used).
 .TP
 .BI \-z " uds-address"
 Specifies the address of the server's UNIX domain socket.
-The default is /var/run/ptp4l.
+The default is /var/run/linuxptp/ptp4l.
 .TP
 .BI \-l " print-level"
 Set the maximum syslog level of messages which should be printed or sent to
@@ -388,7 +388,7 @@ Same as option
 .TP
 .B uds_address
 Specifies the address of the server's UNIX domain socket. The default
-is /var/run/ptp4
+is /var/run/linuxptp/ptp4l
 Same as option
 .B \-z
 (see above).
diff --git a/phc2sys.c b/phc2sys.c
index 35e56d7..d50202b 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -1061,7 +1061,7 @@ static void usage(char *progname)
" -u [num]   number of clock updates in summary stats (0)\n"
" -n [num]   domain number (0)\n"
" -x apply leap seconds by servo instead of 
kernel\n"
-   " -z [path]  server address for UDS (/var/run/ptp4l)\n"
+   " -z [path]  server address for UDS 
(/var/run/linuxptp/ptp4l)\n"
" -l [num]   set the logging level to 'num' (6)\n"
" -t [tag]   add tag to log messages\n"
" -m print messages to stdout\n"
diff --git a/pmc.8 b/pmc.8
index c6ee42f..d9f6467 100644
--- a/pmc.8
+++ b/pmc.8
@@ -87,7 +87,7 @@ Domain socket is relative to the directory of the server's 
socket.
 .TP
 .BI \-s " uds-address"
 Specifies the address of the server's UNIX domain socket.
-The default is /var/run/ptp4l.
+The default is /var/run/linuxptp/ptp4l.
 .TP
 .BI \-t " transport-specific-field"
 Specify the transport specific field in sent messages as a hexadecimal number.
diff --git a/pmc.c b/pmc.c
index 194e450..9435753 100644
--- a/pmc.c
+++ b/pmc.c
@@ -547,7 +547,7 @@ static void usage(char *progname)
" -hprints this message and exits\n"
" -i [dev]  interface device to use, default 'eth0'\n"
"   for network and 'pmc.$pid' for UDS.\n"
-   " -s [path] server address for UDS, default '/var/run/ptp4l'.\n"
+   " -s [path] server address for UDS, default 
'/var/run/linuxptp/ptp4l'.\n"
" -t [hex]  transport specific field, default 0x0\n"
" -vprints the software version and exits\n"
" -zsend zero length TLV values with the GET actions\n"
diff --git a/ptp4l.8 b/ptp4l.8
index 021ca9a..dbb6266 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -626,13 +626,13 @@ is only relevant with IPv6 transport.  See RFC 4291.  The 
default is
 .TP
 .B uds_address
 Specifies the address of the UNIX domain socket for receiving local
-management messages. The default is /var/run/ptp4l.
+management messages. The default is /var/run/linuxptp/ptp4l.
 .TP
 .B uds_ro_addre

[Linuxptp-devel] [PATCHv2 RFC 5/6] phc2sys: Add support for dropping root privileges.

2021-07-13 Thread Miroslav Lichvar
In the static mode, drop the privileges after opening the clocks.

In the automatic mode, drop the privileges before opening the UDS port,
which is required to get the list of interfaces, but is the part of the
operation that needs to be protected most. The non-root user must have
permissions to open the clocks.

Add special handling of the NTPSHM servo, which requires root. Create
the servo before creating clocks (which happens after dropping root in
the automatic mode) and share it amongst all clocks.

Signed-off-by: Miroslav Lichvar 
---
 phc2sys.8 | 10 ++
 phc2sys.c | 29 +
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/phc2sys.8 b/phc2sys.8
index 99fc937..a829874 100644
--- a/phc2sys.8
+++ b/phc2sys.8
@@ -393,6 +393,16 @@ Same as option
 .B \-z
 (see above).
 
+.TP
+.B user
+The name of the user to which should
+.B phc2sys
+switch in order to drop the root privileges. By default,
+.B phc2sys
+will keep the identity of the user under which it is started. With the
+.B \-a
+option the non-root user must have permissions to open the PHC devices.
+
 .SH TIME SCALE USAGE
 
 .B Ptp4l
diff --git a/phc2sys.c b/phc2sys.c
index b428b9f..35e56d7 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -108,6 +108,7 @@ struct phc2sys_private {
LIST_HEAD(clock_head, clock) clocks;
LIST_HEAD(dst_clock_head, clock) dst_clocks;
struct clock *master;
+   struct servo *shared_servo;
 };
 
 static struct config *phc2sys_config;
@@ -125,6 +126,10 @@ static struct servo *servo_add(struct phc2sys_private 
*priv,
int max_ppb;
struct servo *servo;
 
+   if (priv->shared_servo) {
+   return priv->shared_servo;
+   }
+
clockadj_init(clock->clkid);
ppb = clockadj_get_freq(clock->clkid);
/* The reading may silently fail and return 0, reset the frequency to
@@ -214,7 +219,7 @@ static void clock_cleanup(struct phc2sys_private *priv)
struct clock *c, *tmp;
 
LIST_FOREACH_SAFE(c, >clocks, list, tmp) {
-   if (c->servo) {
+   if (c->servo && !priv->shared_servo) {
servo_destroy(c->servo);
}
if (c->sanity_check) {
@@ -234,6 +239,9 @@ static void clock_cleanup(struct phc2sys_private *priv)
}
free(c);
}
+
+   if (priv->shared_servo)
+   servo_destroy(priv->shared_servo);
 }
 
 static void port_cleanup(struct phc2sys_private *priv)
@@ -329,7 +337,8 @@ static void clock_reinit(struct phc2sys_private *priv, 
struct clock *clock,
clock->phc_index = phc_index;
 
if (clock->servo) {
-   servo_destroy(clock->servo);
+   if (clock->servo != priv->shared_servo)
+   servo_destroy(clock->servo);
clock->servo = NULL;
}
 
@@ -1301,14 +1310,23 @@ int main(int argc, char *argv[])
if (priv.servo_type == CLOCK_SERVO_NTPSHM) {
config_set_int(cfg, "kernel_leap", 0);
config_set_int(cfg, "sanity_freq_limit", 0);
+
+   /* The servo needs to be created before dropping root and it
+  will be shared among all clocks */
+   priv.shared_servo = servo_create(phc2sys_config,
+priv.servo_type, 0, 0, 0);
+   if (!priv.shared_servo)
+   goto end;
}
priv.kernel_leap = config_get_int(cfg, NULL, "kernel_leap");
priv.sanity_freq_limit = config_get_int(cfg, NULL, "sanity_freq_limit");
 
-   snprintf(uds_local, sizeof(uds_local), "/var/run/phc2sys.%d",
-getpid());
+   /* Directory will be copied from ptp4l address */
+   snprintf(uds_local, sizeof(uds_local), "phc2sys.%d", getpid());
 
if (autocfg) {
+   if (drop_root_privileges(config_get_string(cfg, NULL, "user")))
+   goto end;
if (init_pmc_node(cfg, priv.agent, uds_local,
  phc2sys_recv_subscribed, ))
goto end;
@@ -1323,6 +1341,9 @@ int main(int argc, char *argv[])
goto end;
}
 
+   if (drop_root_privileges(config_get_string(cfg, NULL, "user")))
+   goto end;
+
r = -1;
 
if (wait_sync) {
-- 
2.26.3



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


[Linuxptp-devel] [PATCHv2 RFC 2/6] uds: Copy ownership of server socket.

2021-07-13 Thread Miroslav Lichvar
To not require pmc and phc2sys to run under the same user as ptp4l,
change the ownership of their socket to the server socket, so it can
send a response to their socket.

Signed-off-by: Miroslav Lichvar 
---
 uds.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/uds.c b/uds.c
index 641a672..81a3f22 100644
--- a/uds.c
+++ b/uds.c
@@ -59,6 +59,7 @@ static int uds_open(struct transport *t, struct interface 
*iface, struct fdarray
struct uds *uds = container_of(t, struct uds, t);
const char *name = interface_name(iface);
struct sockaddr_un sa;
+   struct stat st;
int fd, err;
 
fd = socket(AF_LOCAL, SOCK_DGRAM, 0);
@@ -87,6 +88,14 @@ static int uds_open(struct transport *t, struct interface 
*iface, struct fdarray
uds->address.len = sizeof(sa);
 
chmod(name, UDS_FILEMODE);
+
+   /* Copy the ownership of the server's socket to allow it
+  to send a response if running under a non-root user. */
+   if (strcmp(uds_path, name) && !stat(uds_path, ) &&
+   (st.st_uid || st.st_gid)) {
+   chown(name, st.st_uid, st.st_gid);
+   }
+
fda->fd[FD_EVENT] = -1;
fda->fd[FD_GENERAL] = fd;
return 0;
-- 
2.26.3



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


[Linuxptp-devel] [PATCHv2 RFC 3/6] clock: Add support for dropping root privileges.

2021-07-13 Thread Miroslav Lichvar
Add user option to specify the user to which ptp4l should switch after
opening the PHC device and before opening the network/UDS ports. Create
the directory that will contain the UDS-RW/RO sockets if it doesn't
exist.

In the jbod mode or with a bonded interface, all their PHC devices need
to have the permission set for the non-root user to be able to open them
later.

Modify the PHC switching function to not recreate an NTPSHM servo as it
needs root permissions to attach the memory segment.

Signed-off-by: Miroslav Lichvar 
---
 clock.c  | 29 ++---
 config.c |  1 +
 ptp4l.8  | 10 ++
 3 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/clock.c b/clock.c
index d428ae2..3e904bc 100644
--- a/clock.c
+++ b/clock.c
@@ -1218,6 +1218,16 @@ struct clock *clock_create(enum clock_type type, struct 
config *config,
return NULL;
}
 
+   create_uds_directory(config_get_string(config, NULL, "uds_address"),
+config_get_string(config, NULL, "user"));
+   create_uds_directory(config_get_string(config, NULL, "uds_ro_address"),
+config_get_string(config, NULL, "user"));
+
+   /* Drop the root privileges before opening the ports. */
+   if (drop_root_privileges(config_get_string(config, NULL, "user"))) {
+   return NULL;
+   }
+
/* Create the UDS interfaces. */
 
c->uds_rw_port = port_open(phc_device, phc_index, timestamping, 0,
@@ -1751,16 +1761,21 @@ int clock_switch_phc(struct clock *c, int phc_index)
}
fadj = (int) clockadj_get_freq(clkid);
clockadj_set_freq(clkid, fadj);
-   servo = servo_create(c->config, c->servo_type, -fadj, max_adj, 0);
-   if (!servo) {
-   pr_err("Switching PHC, failed to create clock servo");
-   phc_close(clkid);
-   return -1;
+
+   if (c->servo_type == CLOCK_SERVO_NTPSHM) {
+   servo_reset(c->servo);
+   } else {
+   servo = servo_create(c->config, c->servo_type, -fadj, max_adj, 
0);
+   if (!servo) {
+   pr_err("Switching PHC, failed to create clock servo");
+   phc_close(clkid);
+   return -1;
+   }
+   servo_destroy(c->servo);
+   c->servo = servo;
}
phc_close(c->clkid);
-   servo_destroy(c->servo);
c->clkid = clkid;
-   c->servo = servo;
c->servo_state = SERVO_UNLOCKED;
return 0;
 }
diff --git a/config.c b/config.c
index 4472d3d..06e5fd1 100644
--- a/config.c
+++ b/config.c
@@ -332,6 +332,7 @@ struct config_item config_tab[] = {
PORT_ITEM_INT("unicast_master_table", 0, 0, INT_MAX),
PORT_ITEM_INT("unicast_req_duration", 3600, 10, INT_MAX),
GLOB_ITEM_INT("use_syslog", 1, 0, 1),
+   GLOB_ITEM_STR("user", ""),
GLOB_ITEM_STR("userDescription", ""),
GLOB_ITEM_INT("utc_offset", CURRENT_UTC_OFFSET, 0, INT_MAX),
GLOB_ITEM_INT("verbose", 0, 0, 1),
diff --git a/ptp4l.8 b/ptp4l.8
index fe9e150..021ca9a 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -803,6 +803,16 @@ This option enables using the "write phase" feature of a 
PTP Hardware
 Clock.  If supported by the device, this mode uses the hardware's
 built in phase offset control instead of frequency offset control.
 The default value is 0 (disabled).
+.TP
+.B user
+The name of the user to which should
+.B ptp4l
+switch after start in order to drop the root privileges. By default,
+.B ptp4l
+will keep the identity of the user under which it is started. With the 
+.B boundary_clock_jbod
+option, or if using a bonded interface, the non-root user must have permissions
+to open the PHC devices to be able to switch between them.
 
 .SH UNICAST DISCOVERY OPTIONS
 
-- 
2.26.3



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


[Linuxptp-devel] [PATCHv2 RFC 1/6] util: Add functions for dropping root privileges.

2021-07-13 Thread Miroslav Lichvar
Add a function to switch the process UID/GID to a specified user in
order to drop the root privileges, but keep the capabilities needed to
adjust the clock, enable HW timestamping, bind to privileged ports and
raw sockets, using the libcap library.

Add a function to create a directory for a UDS address with a specified
owner where the programs will be able to bind and unlink their sockets
without root privileges.

Signed-off-by: Miroslav Lichvar 
---
 incdefs.sh |  11 +-
 makefile   |   4 ++
 util.c | 111 +
 util.h |  19 +
 4 files changed, 144 insertions(+), 1 deletion(-)

diff --git a/incdefs.sh b/incdefs.sh
index 19e620e..9889059 100755
--- a/incdefs.sh
+++ b/incdefs.sh
@@ -19,7 +19,8 @@
 # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 
 #
-# Look for functional prototypes in the C library.
+# Look for functional prototypes in the C library and development files
+# needed for optional features.
 #
 user_flags()
 {
@@ -50,6 +51,14 @@ user_flags()
fi
done
done
+
+   # Look for libcap header.
+   for d in $dirs; do
+   if test -a $d/sys/capability.h; then
+   printf " -DHAVE_LIBCAP"
+   break
+   fi
+   done
 }
 
 #
diff --git a/makefile b/makefile
index 33e7ca0..a42bd21 100644
--- a/makefile
+++ b/makefile
@@ -43,6 +43,10 @@ incdefs := $(shell $(srcdir)/incdefs.sh)
 version := $(shell $(srcdir)/version.sh $(srcdir))
 VPATH  = $(srcdir)
 
+ifneq (,$(findstring -DHAVE_LIBCAP,$(incdefs)))
+   LDLIBS += -lcap
+endif
+
 prefix = /usr/local
 sbindir= $(prefix)/sbin
 mandir = $(prefix)/man
diff --git a/util.c b/util.c
index 113467d..6a488d5 100644
--- a/util.c
+++ b/util.c
@@ -18,13 +18,25 @@
  */
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
+#include 
+#include 
+
+#ifdef HAVE_LIBCAP
+#include 
+#include 
+#include 
+#include 
+#include 
+#endif
 
 #include "address.h"
+#include "interface.h"
 #include "phc.h"
 #include "print.h"
 #include "sk.h"
@@ -753,3 +765,102 @@ int rate_limited(int interval, time_t *last)
 
return 0;
 }
+
+void create_uds_directory(const char *address, const char *user)
+{
+   char path[MAX_IFNAME_SIZE + 1], *dir;
+#ifdef HAVE_LIBCAP
+   struct passwd *pw;
+#endif
+
+   if (snprintf(path, sizeof(path), "%s", address) >= sizeof(path)) {
+   pr_err("path too long for UDS");
+   return;
+   }
+
+   dir = dirname(path);
+
+   /* Don't do anything if it already exists or cannot be created */
+   if (mkdir(dir, 0770)) {
+   if (errno != EEXIST)
+   pr_err("failed to create %s: %m", dir);
+   return;
+   }
+
+#ifdef HAVE_LIBCAP
+   if (user[0] == '\0')
+   return;
+
+   pw = getpwnam(user);
+   if (!pw) {
+   pr_err("failed to get user/group ID of %s", user);
+   rmdir(dir);
+   return;
+   }
+
+   if (chown(dir, pw->pw_uid, pw->pw_gid)) {
+   pr_err("failed to change owner of %s: %m", dir);
+   rmdir(dir);
+   return;
+   }
+#endif
+}
+
+int drop_root_privileges(const char *user)
+{
+#ifdef HAVE_LIBCAP
+   struct passwd *pw;
+   cap_t cap;
+#endif
+
+   if (user[0] == '\0')
+   return 0;
+
+#ifdef HAVE_LIBCAP
+   pw = getpwnam(user);
+   if (!pw) {
+   pr_err("failed to get user/group ID of %s", user);
+   return -1;
+   }
+
+   if (prctl(PR_SET_KEEPCAPS, 1)) {
+   pr_err("failed to set KEEPCAPS flag");
+   return -1;
+   }
+
+   if (setgroups(0, NULL)) {
+   pr_err("failed to drop supplementary groups");
+   return -1;
+   }
+
+   if (setgid(pw->pw_gid)) {
+   pr_err("failed to set group ID");
+   return -1;
+   }
+
+   if (setuid(pw->pw_uid)) {
+   pr_err("failed to set user ID");
+   return -1;
+   }
+
+   cap = cap_from_text("cap_sys_time,cap_net_admin,"
+   "cap_net_bind_service,cap_net_raw=ep");
+   if (!cap) {
+   pr_err("failed to initialize capabilities");
+   return -1;
+   }
+
+   if (cap_set_proc(cap)) {
+   pr_err("failed to set process capabilities");
+   cap_free(cap);
+   return -1;
+   }
+
+   cap_free(cap);
+
+   return 0;
+#else
+   pr_err("cannot drop root privileges without libcap");
+   return -1;
+#endif
+}
diff --git a/util.h b/util.h
index 739c8fd..454ab07 100644
--- a/

[Linuxptp-devel] [PATCH] config: Add workaround for glibc getopt_long().

2021-07-13 Thread Miroslav Lichvar
getopt_long() in glibc allows shortened long option names, e.g.
ptp4l --domain works as --domainNumber. When the match is ambiguous,
e.g. --fault matches --fault_badpeernet_interval and
--fault_reset_interval, it is supposed to return an error, but that
works only if their struct option have different flags or vals.

https://sourceware.org/bugzilla/show_bug.cgi?id=28081

Set the flag to point to the val itself for a no-op to work around this
issue and make it less likely a misremembered option name will silently
match one of multiple possible candidates.

Signed-off-by: Miroslav Lichvar 
---
 config.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/config.c b/config.c
index d77664c..79149aa 100644
--- a/config.c
+++ b/config.c
@@ -741,6 +741,8 @@ static struct option *config_alloc_longopts(void)
ci = _tab[i];
opts[i].name = ci->label;
opts[i].has_arg = required_argument;
+   /* Avoid bug in detection of ambiguous options in glibc */
+   opts[i].flag = [i].val;
}
 
return opts;
-- 
2.26.3



___
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

2021-07-12 Thread Miroslav Lichvar
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



___
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

2021-07-08 Thread Miroslav Lichvar
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.

-- 
Miroslav Lichvar



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


[Linuxptp-devel] [PATCH v3 1/5] clock: Reset state when switching port with same best clock.

2021-05-31 Thread Miroslav Lichvar
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.

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] [PATCH v3 3/5] port: Don't check timestamps from non-slave ports.

2021-05-31 Thread Miroslav Lichvar
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.

Reviewed-by: Jacob Keller 
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] [PATCH v3 5/5] clockcheck: Increase minimum interval.

2021-05-31 Thread Miroslav Lichvar
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 
---
 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] [PATCH v3 4/5] port: Don't renew raw transport.

2021-05-31 Thread Miroslav Lichvar
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 
---
 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] [PATCH v3 0/5] Fixes for sanity clock check

2021-05-31 Thread Miroslav Lichvar
v3
- added patch to avoid slow renewal of raw sockets
- added patch to increase the minimum check interval

v2
- improved commit message
- added missing NULL check

These patches make the clock check more reliable in several different
cases.

The first patch is not strictly related to the clock check, e.g. it also
fixes the path delay after switching the slave port.

Miroslav Lichvar (5):
  clock: Reset state when switching port with same best clock.
  clock: Reset clock check on best clock/port change.
  port: Don't check timestamps from non-slave ports.
  port: Don't renew raw transport.
  clockcheck: Increase minimum interval.

 clock.c  |  4 +++-
 clockcheck.c | 11 +--
 clockcheck.h |  6 ++
 port.c   | 11 ++-
 4 files changed, 28 insertions(+), 4 deletions(-)

-- 
2.26.3



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


[Linuxptp-devel] [PATCH v3 2/5] clock: Reset clock check on best clock/port change.

2021-05-31 Thread Miroslav Lichvar
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).

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


Re: [Linuxptp-devel] [PATCH v2 3/3] port: Don't check timestamps from non-slave ports.

2021-05-27 Thread Miroslav Lichvar
On Wed, May 26, 2021 at 05:45:29PM +, Amar Subramanyam wrote:
> Hi Miroslav,
> 
> We were able to reproduce the issue even without running phc2sys.
> Please find the attached strace and ptp4l logs when this issue is seen.

Ok, thanks. That's very helpful.

> From the ptp4l log, we can see that BMCA took 148 msec to run(Mono Interval 
> :: 148445967).
> The same can be observed from strace logs. In the attached strace log, BMCA 
> is executed between the timestamps 00:07:27.047830 (line number 53) to 
> 00:07:27.196275(line number 102).

I don't see that in the log.

> 00:07:27.142942 close(4)= 0
> 00:07:27.153733 close(15)   = 0
> 00:07:27.167783 socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP) = 4
> 00:07:27.167829 ioctl(4, SIOCGIFHWADDR, {ifr_name="sriov0", 
> ifr_hwaddr={sa_family=ARPHRD_ETHER, sa_data=64:4c:36:12:55:e0}}) = 0
> 00:07:27.167878 close(4)= 0
> 00:07:27.167964 socket(AF_PACKET, SOCK_RAW, 768) = 4
> 00:07:27.168023 ioctl(4, SIOCGIFINDEX, {ifr_name="sriov0", }) = 0
> 00:07:27.168095 bind(4, {sa_family=AF_PACKET, sll_protocol=htons(ETH_P_ALL), 
> sll_ifindex=if_nametoindex("sriov0"), sll_hatype=ARPHRD_NETROM, 
> sll_pkttype=PACKET_HOST, sll_halen=0}, 20) = 0
> 00:07:27.178781 setsockopt(4, SOL_SOCKET, SO_BINDTODEVICE, "sriov0", 6) = 0
> 00:07:27.178830 setsockopt(4, SOL_SOCKET, SO_ATTACH_FILTER, {len=12, 
> filter=0x635ae0}, 16) = 0
> 00:07:27.178875 setsockopt(4, SOL_PACKET, PACKET_ADD_MEMBERSHIP, 
> {mr_ifindex=if_nametoindex("sriov0"), mr_type=PACKET_MR_MULTICAST, mr_alen=6, 
> mr_address=011b1900}, 16) = 0
> 00:07:27.179151 setsockopt(4, SOL_PACKET, PACKET_ADD_MEMBERSHIP, 
> {mr_ifindex=if_nametoindex("sriov0"), mr_type=PACKET_MR_MULTICAST, mr_alen=6, 
> mr_address=0180c20e}, 16) = 0
> 00:07:27.179415 socket(AF_PACKET, SOCK_RAW, 768) = 15
> 00:07:27.179482 ioctl(15, SIOCGIFINDEX, {ifr_name="sriov0", }) = 0
> 00:07:27.179518 bind(15, {sa_family=AF_PACKET, sll_protocol=htons(ETH_P_ALL), 
> sll_ifindex=if_nametoindex("sriov0"), sll_hatype=ARPHRD_NETROM, 
> sll_pkttype=PACKET_HOST, sll_halen=0}, 20) = 0
> 00:07:27.193807 setsockopt(15, SOL_SOCKET, SO_BINDTODEVICE, "sriov0", 6) = 0

What I see is that it's the closing and binding of the raw sockets
that is slowing down ptp4l so much that a received message waits for
up to ~40 milliseconds before the clock check gets its timestamp.

ptp4l is renewing the transport on announce/sync timeout, which
according to the git log is needed in the client-only mode to not get
multicast sockets stuck when the link goes down. I think the fix here
is to avoid renewing the raw transport. It shouldn't be necessary if I
understand it correctly.

Can you please verify that the following change fixes the problem for
you?

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

-- 
Miroslav Lichvar



___
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.

2021-05-26 Thread Miroslav Lichvar
On Wed, May 26, 2021 at 02:05:01PM +, Amar Subramanyam wrote:
> Hi Miroslav,
> 
> I am afraid, the patches that was intended to fix the reported issue doesn't 
> do so. The issue that we reported was not due to timestamps from non-slave 
> port being compared in jbod mode.  But due to mono_interval (CLOCK_MONOTIC 
> interval b/w two successive calls to clock_check_sample()) becoming high due 
> to the execution BMCA algorithm between two successive calls to 
> clock_check_sample() function. We believe that this is due to execution time 
> being higher, and solution is to increase the default value of 
> sanity_freq_limit accordingly.

The execution of the BMCA took 30 milliseconds? In that case the fix
would be to make the minimum interval longer. Can you post a strace -tt
log containing the last second leading to the fault?

I suspect it's rather the PHC interval that's wrong. There might be an
unexpected interaction with phc2sys. Can you reproduce the issue with
no phc2sys running?

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] [PATCH] Add new CLOCK_STATS_NP TLV GET to pmc and clock

2021-05-26 Thread Miroslav Lichvar
On Tue, May 25, 2021 at 08:28:34PM +, Tim Martin wrote:
> +struct clock_stats_np_tlv {
> + struct stats_result offset_stats;
> + struct stats_result freq_stats;
> + struct stats_result delay_stats;
> + uint8_t offset_freq_stats_valid;
> + uint8_t delay_stats_valid;
> +} PACKED;

Floating-point formats are not generally portable. In a network
protocol they need to be converted to something else. Some of the
implemented TLVs use fixed-point formats shifting by 32 or 41 bits. In
this case I think it might be ok to just use integer nanoseconds and
parts per billion.

-- 
Miroslav Lichvar



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


[Linuxptp-devel] [PATCH v2 3/3] port: Don't check timestamps from non-slave ports.

2021-05-26 Thread Miroslav Lichvar
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.

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] [PATCH v2 1/3] clock: Reset state when switching port with same best clock.

2021-05-26 Thread Miroslav Lichvar
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.

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] [PATCH v2 2/3] clock: Reset clock check on best clock/port change.

2021-05-26 Thread Miroslav Lichvar
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).

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] [PATCH v2 0/3] Fixes for sanity clock check

2021-05-26 Thread Miroslav Lichvar
v2
- improved commit message
- added missing NULL check

These patches should make the clock check more reliable, e.g. when the
clock is controlled externally by another process, or when there are
multiple clocks in the jbod mode as was discussed recently on this list.

The first patch is not strictly related to the clock check, e.g. it also
fixes the path delay after switching the slave port.

Miroslav Lichvar (3):
  clock: Reset state when switching port with same best clock.
  clock: Reset clock check on best clock/port change.
  port: Don't check timestamps from non-slave ports.

 clock.c  | 4 +++-
 clockcheck.c | 9 -
 clockcheck.h | 6 ++
 port.c   | 5 -
 4 files changed, 21 insertions(+), 3 deletions(-)

-- 
2.26.3



___
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.

2021-05-26 Thread Miroslav Lichvar
On Tue, May 25, 2021 at 08:35:35PM +, Keller, Jacob E wrote:
> > 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.

Detecting that would be difficult and unreliable. I'll reword the
message to me it more clear.

> > if (!cid_eq(_id, >best_id) || best != c->best) {
> > clock_freq_est_reset(c);
> > +   clockcheck_reset(c->sanity_check);

And here is a missing check for NULL in case it is disabled.

Thanks,

-- 
Miroslav Lichvar



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


[Linuxptp-devel] [PATCH 3/3] port: Don't check timestamps from non-slave ports.

2021-05-25 Thread Miroslav Lichvar
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.

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] [PATCH 1/3] clock: Reset state when switching port with same best clock.

2021-05-25 Thread Miroslav Lichvar
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.

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] [PATCH 2/3] clock: Reset clock check on port state change.

2021-05-25 Thread Miroslav Lichvar
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).

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] [PATCH 0/3] Fixes for sanity clock check

2021-05-25 Thread Miroslav Lichvar
These patches should make the clock check more reliable, e.g. when the
clock is controlled externally by another process, or when there are
multiple clocks in the jbod mode as was discussed recently on this list.

The first patch is not strictly related to the clock check, e.g. it also
fixes the path delay after switching the slave port.

Miroslav Lichvar (3):
  clock: Reset state when switching port with same best clock.
  clock: Reset clock check on port state change.
  port: Don't check timestamps from non-slave ports.

 clock.c  | 3 ++-
 clockcheck.c | 9 -
 clockcheck.h | 6 ++
 port.c   | 5 -
 4 files changed, 20 insertions(+), 3 deletions(-)

-- 
2.26.3



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


Re: [Linuxptp-devel] [PATCH 1/2] Log optimization for ptp4l in jbod and client only mode (clientOnly=1 and boundary_clock_jbod=1)

2021-05-24 Thread Miroslav Lichvar
On Sun, May 23, 2021 at 06:40:58AM -0700, Richard Cochran wrote:
> On Sat, May 22, 2021 at 05:09:07PM +0300, Amar Subramanyam via Linuxptp-devel 
> wrote:
> > @@ -1983,7 +1982,9 @@ static void handle_state_decision_event(struct clock 
> > *c)
> > event = EV_RS_PASSIVE;
> > break;
> > case PS_SLAVE:
> > -   clock_update_slave(c);
> > +   if (fresh_best) {
> > +   clock_update_slave(c);
> > +   }
> 
> This is not correct.  Elements of the upstream data sets might have
> changed, and the port needs to stay up to date.

Is there something else that is not already covered by the
clock_update_time_properties() call from port.c?

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] [PATCH] Sync issues observed when ptp4l is ran with jbod and client only mode (clientOnly=1 and boundary_clock_jbod=1)

2021-05-20 Thread Miroslav Lichvar
On Mon, May 17, 2021 at 10:02:59AM +0300, Amar Subramanyam via Linuxptp-devel 
wrote:
> This patch addresses the following issues when ptp4l is run on multiple ports
> with jbod and client only mode (i.e. clientOnly=1 and boundary_clock_jbod=1):-
> 
>   1.The LISTENING port prints continuously
>   "selected best master clock 00..03
>   updating UTC offset to 37"
> 
>   We limited the log such that now it prints only when there is a
>   change in the best-master clock.
> 
>   2.The port other than SLAVE (LISTENING port) prints an error
>   "port 1: master state recommended in slave only mode
>   ptp4l[1205469.356]: port 1: defaultDS.priority1 probably misconfigured"
>   for every ANNOUNCE RECEIPT Timeout.

I think it would be better to have separate patches for the two
issues.

> -static void clock_update_slave(struct clock *c)
> +static void clock_update_slave(struct clock *c, int mdiff)
>  {
>   struct parentDS *pds = >dad.pds;
>   struct ptp_message *msg;
>  
> - if (!c->best)
> + if (!c->best || !mdiff)
>   return;

Instead of adding and checking the mdiff parameter here, not doing
anything, why not just not call the function?

> diff --git a/port.c b/port.c
> index 10bb9e1..650ca00 100644
> --- a/port.c
> +++ b/port.c
> @@ -2531,7 +2531,7 @@ void port_dispatch(struct port *p, enum fsm_event 
> event, int mdiff)
>  static void bc_dispatch(struct port *p, enum fsm_event event, int mdiff)
>  {
>   if (clock_slave_only(p->clock)) {
> - if (event == EV_RS_MASTER || event == EV_RS_GRAND_MASTER) {
> + if (event == EV_RS_GRAND_MASTER) {
>   port_slave_priority_warning(p);
>   }
>   }

Makes sense to me.

There is a comment in bmc_state_decision() referring to this code,
which might need to be updated with this change, or maybe that whole
check specific to the Automotive profile could be removed if the log
message was the only reason for it (I'm not sure).

Thanks,

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] [PATCH] Sync issues observed when ptp4l is ran with jbod and client only mode (clientOnly=1 and boundary_clock_jbod=1)

2021-05-12 Thread Miroslav Lichvar
On Tue, May 11, 2021 at 02:16:04PM +, Amar Subramanyam wrote:
> What happens here exactly is, due to the continuous triggering of  BMCA, the 
> value of mono_interval (the interval between two successive calls of 
> clock_check_sample ()) gets increased and SYNCRONIZATION FAULT occurs with 
> the default value of sanity_freq_limit (which is 2). Modifying the 
> configuration with --sanity_freq_limit=0 will prevent the FAULT from 
> occurring , but it will not address the root cause of BMCA getting triggered 
> continuously even though there is no change in successive announce messages 
> in the port. So we believe that setting --sanity_freq_limit=0 is a work 
> around and doesn't directly solve the issue.
> Hence the changes we introduced are such that BMCA is not triggered at all 
> when there is no change in successive announce messages.

Ok, so it is the clock check as I suspected. I don't see how it is
related to the BMCA or the announce timeout. The clock check is active
when the clock is in a synchronized state and it checks RX timestamps
of event messages.

If the clocks were not synchronized, sync messages received on
different ports failed the check. That's what I saw in my test, even
with your patch applied.

There is a race condition with phc2sys. It may not be fast enough to
sync the other clock before it receives an event message.

I think the fix should be one of the following:
- disable clock check in jbod mode (it cannot work reliably as it is)
- limit the check to timestamps from the synchronized port
- have a separate clock check instance for each clock, checking only
  its own timestamps

> > There might be a better name for this function. Maybe something related to 
> > its purpose rather than what it does.
> 
> Is the name "clock_get_port_client_state" fine?. Could you please propose any 
> new suggestions?

Maybe something like clock_non_client_port_announce_timer would work
better?

> > Ok, but if this optimization is useful in the jbod mode, it should be 
> > useful even in the non-jbod mode, right? Most of the port code shouldn't 
> > care about jbod.
> 
> Yes, as you suggested this change is useful in both jbod and non-jbod mode to 
> avoid unnecessary triggering of BMCA. But there is no impact seen in non jbod 
> case as there is only one port. Whereas there is clear impact on the SLAVE 
> port in jbod, slaveOnly case, as explained earlier. We didn't want to 
> introduce any new variables to the non jbod mode, hence we restricted our 
> change to the jbod mode alone.

I think it could work as an optimization to avoid unnecessary calls of
BCMA and spam in the log, but that shouldn't be specific to the jbod
mode.

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] [PATCH] Sync issues observed when ptp4l is ran with jbod and client only mode (clientOnly=1 and boundary_clock_jbod=1)

2021-05-10 Thread Miroslav Lichvar
On Fri, May 07, 2021 at 02:27:46PM +, Greg Armstrong wrote:
> Just to add, the key reason it is not supported by BC is that if true, then 
> clockClass must be 255. This clockClass is only for slave-only OC.
> 
> From IEEE1588-2019 clause 8.2.1.3.1.2:
>   If defaultDS.slaveOnly is TRUE, the initialization value [of 
> defaultDS.clockQuality.clockClass] shall be 255 as specified in 7.6.2.5.
> 
> From IEEE1588-2019 Table 4:
>   255 |   Shall be the clockClass of a slave-only PTP Instance 
> (see 9.2.2.1).
> 
> And clause 9.2.2.1 title is "Slave-Only Ordinary Clocks".

Good point. 

But this doesn't break anything at the protocol level, right?
A slaveOnly clock should never send an annoucement message with its
clockClass.

It is an extension that we can support in linuxptp, assuming we can
make it work as expected, e.g. avoid those "priority1 probably
misconfigured" warnings, etc.

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] [PATCH] Sync issues observed when ptp4l is ran with jbod and client only mode (clientOnly=1 and boundary_clock_jbod=1)

2021-05-10 Thread Miroslav Lichvar
On Fri, May 07, 2021 at 11:37:06AM +, Amar Subramanyam wrote:
> Continuous BMCA triggering in port 2 causes a SYNCHRONIZATION FAULT in 
> port1.This causes port1 to jump from SLAVE to UNCALIBRATED and vice versa 
> repeatedly. 

I'm sorry if I sound like a broken record, but what exactly is the
cause of the fault? Is it the clock check seeing timestamps from two
clocks that are not synchronized? Do the faults disappear if you set
--sanity_freq_limit=0?

That's the only fault I see in my test and in your original report.
Your patch doesn't seem to prevent that fault in my test, so I'm
confused.

> Noted. Please find the updated description and function name below, we will 
> send out the modified patch after full review. 
> 
>  + * Get port SLAVE state for client only mode.
>  + * @param c  The clock instance.
>  + * @return   Return 0 if any port is in SLAVE state, 1 otherwise.
>  + */
>  +int clock_get_port_slave_state(struct clock *c);

There might be a better name for this function. Maybe something
related to its purpose rather than what it does.

> Hence, we are clearing the Announce receipt timer for port2 (LISTENING port) 
> in the function bc_event() upon Announce receipt timeout, only when 
> boundary_clock_jbod=1 and clientOnly=1 is configured and atleast one port 
> (Port1 here) is in SLAVE/UNCALIBRATED state.

Ok, but if this optimization is useful in the jbod mode, it should be
useful even in the non-jbod mode, right? Most of the port code
shouldn't care about jbod.

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] [PATCH] Sync issues observed when ptp4l is ran with jbod and client only mode (clientOnly=1 and boundary_clock_jbod=1)

2021-05-06 Thread Miroslav Lichvar
On Tue, May 04, 2021 at 01:51:24PM +0300, Amar Subramanyam via Linuxptp-devel 
wrote:
> This patch addresses the following issues when ptp4l is ran on multiple ports
> with jbod and client only mode (i.e clientOnly=1 and boundary_clock_jbod=1):-
> 
> 1.SYNCHRONIZATION FAULT occurs at every ANNOUNCE RECEIPT Timeout on LISTENING 
> port,
>  which leads to PTP port state of SLAVE port to flap between SLAVE and 
> UNCALIBRATED
>  states continuously.

It's not clear to me what exactly is happening here and how does the
patch fix it. The faults are happening due to the clock check getting
out of order timestamps from two unsynchronized clocks, right? Any
chance it is an issue with phc2sys not synchronizing the clocks?

> +int clock_get_client_state(struct clock *c) +{
> + struct port *piter;
> +
> + if (!clock_slave_only(c)) {
> + return 1;
> + }
> +
> + LIST_FOREACH(piter, >ports, list) {
> + enum port_state ps = port_state(piter);
> + if (ps == PS_SLAVE || ps == PS_UNCALIBRATED) {
> + return 0;
> + }
> + }
> + return 1;
> +}

> + * Inform if any of the port is in SLAVE state.
> + * @param c  The clock instance.
> + * @return   Return 0 if any port is in SLAVE state, 1 otherwise.
> + */
> +int clock_get_client_state(struct clock *c);

The function seems to be specific to the slave-only mode and it checks
for two port states. The description and name of the function indicate
something else.

> +  * In multiport slave only mode, there maybe
> +  * announce messages on LISTENING port. Re-arm
> +  * the timer if any other configured port is in SLAVE state
> +  */
> + if (p->jbod && !clock_get_client_state(p->clock)) {
> + port_set_announce_tmo(p);
> + }

Why is this and the other part of the port change specific to the jbod
mode? Shouldn't the announce timer work exactly the same no matter if
it's a jbod or a real multiport clock?

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] Regarding linuxPTP static analysis..

2021-05-04 Thread Miroslav Lichvar
On Tue, May 04, 2021 at 04:43:51PM +0900, 박웅섭 wrote:
> 1.In the text->length=c->desc.userDescription.length part of clock.c line
> 368, the length declared in the static_ptp_text structure is of type signed
> int and the length declared in the text structure is unsigned int. Why did
> you write the code like this? Assigning Signed integers to unsigned
> integers can lead to overflow problems.

In my copy of the code the length field of the PTPText structure is
uint8_t. It's a structure used in the network protocol.

> 2. The memcpy function is vulnerable to security. Wouldn't it be correct to
> use memcpy_s instead of memcpy function?

Isn't that a Windows-only function?

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] [PATCH] To support Ordinary Clock-Subordinate/Slave just a bunch of devices(jbod) feature.

2021-04-27 Thread Miroslav Lichvar
On Tue, Apr 27, 2021 at 03:59:05PM +, Ramana Reddy wrote:
>  As I said, running independent clients defeats the purpose of BMC 
> algorithm and breaks the ITU-T G.8275.2
> Spec compliance. The BMC algorithm should be run locally on all ports of 
> every ordinary and boundary clock in a domain. Since it 
> runs continuously, it continually readapts to changes in the network or the 
> clocks. Pls check section 9.3 in IEEE1588-2008
> Spec for details on BMC algorithm. Also refer to Section 6.7 of A-BMCA 
> requirements of ITU-T G.8275.2 spec.

Can you please refer to the exact page and paragraph which prevents
multiple ptp4l instances to be running on a computer which has
multiple clocks?

PTP is specified from a clock point of view. If you have multiple
clocks, you need multiple PTP instances, or you can pretend it's a
single clock with the jbod option. In either case, your requirement
"BMC algorithm should be run locally on all ports of every ordinary
and boundary clock in a domain" is satisfied.

> I tried --boundary_clock_jbod=1 --clientOnly=1 with two interfaces and
> it seems to be switching them between the LISTENING and
> UNCALIBRATED/SLAVE states as expected.
>  This I believe was explained in detailed on what are the existing 
> issues, design choices we have,
> Motivation for the new changes. Pls refer attached mail from Amar.

The only relevant part from that mail I found is this:

> b.  As the other PTP redundant ports part of bundle are in LISTENING 
> state, BMCA will not be able to select best master when there is best clock 
> carried in the other ports (other than the port which is already locked). 
> Please refer to IEEE1588 section 9.2.4  where they mention the definition of 
> LISTENING and PASSIVE state interpretations.

Please explain what exactly prevents the selection. I didn't see it in
my test.

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] [PATCH] To support Ordinary Clock-Subordinate/Slave just a bunch of devices(jbod) feature.

2021-04-27 Thread Miroslav Lichvar
On Sat, Apr 24, 2021 at 06:01:46AM +, Ramana Reddy wrote:
>  Thanks. What I think is running multiple ptp4l  instances might 
> defeat the purpose of using BMCA as the chosen ptp4l
> Instance might not be carrying the best clock compared to other ptp4l 
> instances. Alternatively we might have to bring in
>  Additional Intelligence(similar to BMCA) to monitor bunch of ptp4l instances 
> to pick best one dynamically.

An advantage of having multiple independent clients is that you can
better detect failures in synchronization and avoid corrupting the
other clocks.

>  I believe Ordinary Clock can have multiple ports and in such cases 
> only one of the ports will be in Client/Slave mode and
> Other ports shall be either in Listening or passive mode based on whether the 
> other ports are receiving PTP traffic or
>  not.

The specification defines an ordinary clock as "A PTP Instance that
has a single PTP Port in its domain and maintains the timescale used
in the domain."

If you run ptp4l with multiple specified interfaces, it cannot be an
ordinary clock. The boundary_clock_jbod and clientOnly options don't
matter here.

> Also IMHO port state PASSIVE doesn’t seem to be tied to a specific clock 
> mode(BC/OC) in the standard.

Right.

I read the original patch+report again and it's not clear to me why
you need the port to be in the passive state.

I tried --boundary_clock_jbod=1 --clientOnly=1 with two interfaces and
it seems to be switching them between the LISTENING and
UNCALIBRATED/SLAVE states as expected.

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] [PATCH] To support Ordinary Clock-Subordinate/Slave just a bunch of devices(jbod) feature.

2021-04-22 Thread Miroslav Lichvar
On Thu, Apr 22, 2021 at 09:04:28AM +, Amar Subramanyam via Linuxptp-devel 
wrote:
> >But you haven't even identified a problem.  clientOnly and 
> >boundary_clock_jbod work just fine together:
> 
> 1.  Using boundary_clock_jbod has few issues which weren’t mentioned 
> earlier(see mail trail for the issues).
> 
> Also boundary_clock_jbod might not be right term to be used for Ordinary 
> Clock Client/Slave/Sub-ordinate modes.
> 
> Boundary Clock and Ordinary are two different clocks.

I don't have a solution for the described issue. I think it's
generally better to run multiple ptp4l instances.

Just a comment about terminology.

Per IEEE1588 ordinary clock has a single port and boundary clock has
multiple ports. You seem to be talking about a clock with multiple
ports. "jbod" is an option to assume all ports belong to a single
clock.

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] FYI: a summary scribble on my i210 SDP / PPS-in + 25 MHz ref etc sprees

2021-03-23 Thread Miroslav Lichvar
On Mon, Mar 22, 2021 at 06:35:48PM +0100, Frantisek Rysanek wrote:
> Anyway the time has come for me to post a link.
> Here goes my newest scribble on the aforementioned topics:
> 
> http://support.fccps.cz/industry/i210_hacks/i210_hacks.html

Interesting stuff. Thanks for the post.

That issue with oscillations around resolution of the clock, I think
you could try smaller PI constants, or you could switch to the nullf
servo, which is intended for cases like this.

-- 
Miroslav Lichvar



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


[Linuxptp-devel] [PATCH] Revert "phc2sys: Expand the validation of the PPS mode."

2021-03-22 Thread Miroslav Lichvar
Allow the -s option to be used together with the -d option again. The
PHC is used in the PPS mode to fix the offset by an integer number of
seconds to get the system clock close to the PHC.

This reverts commit 228325c1bda4.
---
 phc2sys.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/phc2sys.c b/phc2sys.c
index 4d41f06..b428b9f 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -1290,10 +1290,6 @@ int main(int argc, char *argv[])
"cannot use a pps device unless destination is 
CLOCK_REALTIME\n");
goto bad_usage;
}
-   if (hardpps_configured(pps_fd) && src_name) {
-   fprintf(stderr, "please specify -s or -d, but not both\n");
-   goto bad_usage;
-   }
 
print_set_progname(progname);
print_set_tag(config_get_string(cfg, NULL, "message_tag"));
-- 
2.26.2



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


[Linuxptp-devel] [PATCHv2] Avoid undefined integer operations.

2021-03-22 Thread Miroslav Lichvar
This fixes errors reported by the -fsanitize=undefined sanitizer.

Before accepting the message interval value from a sync message, check
if it is between -10 and 22, same as required for the delay request
interval.

In the calculation of fest/stats/nrate max_count use unsigned 1 to avoid
an invalid shift by 31.

In tmv.h operations cast values to uint64_t to avoid signed overflows
and a left-shift of a negative value.

Signed-off-by: Miroslav Lichvar 
---
 clock.c |  4 ++--
 port.c  | 12 +---
 tmv.h   |  6 +++---
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/clock.c b/clock.c
index f88df58..85d7667 100644
--- a/clock.c
+++ b/clock.c
@@ -1881,7 +1881,7 @@ void clock_sync_interval(struct clock *c, int n)
shift = sizeof(int) * 8 - 1;
pr_warning("freq_est_interval is too long");
}
-   c->fest.max_count = (1 << shift);
+   c->fest.max_count = (1U << shift);
 
shift = c->stats_interval - n;
if (shift < 0)
@@ -1890,7 +1890,7 @@ void clock_sync_interval(struct clock *c, int n)
shift = sizeof(int) * 8 - 1;
pr_warning("summary_interval is too long");
}
-   c->stats.max_count = (1 << shift);
+   c->stats.max_count = (1U << shift);
 
servo_sync_interval(c->servo, n < 0 ? 1.0 / (1 << -n) : 1 << n);
 }
diff --git a/port.c b/port.c
index cefe780..da7c327 100644
--- a/port.c
+++ b/port.c
@@ -1070,7 +1070,7 @@ static void port_nrate_initialize(struct port *p)
 
p->nrate.origin1 = tmv_zero();
p->nrate.ingress1 = tmv_zero();
-   p->nrate.max_count = (1 << shift);
+   p->nrate.max_count = (1U << shift);
p->nrate.count = 0;
p->nrate.ratio = 1.0;
p->nrate.ratio_valid = 0;
@@ -2345,8 +2345,14 @@ void process_sync(struct port *p, struct ptp_message *m)
 
if (!msg_unicast(m) &&
m->header.logMessageInterval != p->log_sync_interval) {
-   p->log_sync_interval = m->header.logMessageInterval;
-   clock_sync_interval(p->clock, p->log_sync_interval);
+   if (m->header.logMessageInterval < -10 ||
+   m->header.logMessageInterval > 22) {
+   pl_info(300, "%s: ignore bogus sync interval 2^%d",
+   p->log_name, m->header.logMessageInterval);
+   } else {
+   p->log_sync_interval = m->header.logMessageInterval;
+   clock_sync_interval(p->clock, p->log_sync_interval);
+   }
}
 
m->header.correction += p->asymmetry;
diff --git a/tmv.h b/tmv.h
index 0c1155f..e5fe110 100644
--- a/tmv.h
+++ b/tmv.h
@@ -49,7 +49,7 @@ typedef struct {
 static inline tmv_t tmv_add(tmv_t a, tmv_t b)
 {
tmv_t t;
-   t.ns = a.ns + b.ns;
+   t.ns = (uint64_t)a.ns + (uint64_t)b.ns;
return t;
 }
 
@@ -78,7 +78,7 @@ static inline int tmv_is_zero(tmv_t x)
 static inline tmv_t tmv_sub(tmv_t a, tmv_t b)
 {
tmv_t t;
-   t.ns = a.ns - b.ns;
+   t.ns = (uint64_t)a.ns - (uint64_t)b.ns;
return t;
 }
 
@@ -126,7 +126,7 @@ static inline TimeInterval tmv_to_TimeInterval(tmv_t x)
} else if (x.ns > (int64_t)MAX_TMV_TO_TIMEINTERVAL) {
return MAX_TMV_TO_TIMEINTERVAL << 16;
}
-   return x.ns << 16;
+   return (uint64_t)x.ns << 16;
 }
 
 static inline struct Timestamp tmv_to_Timestamp(tmv_t x)
-- 
2.26.2



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


Re: [Linuxptp-devel] SyncE support

2021-03-22 Thread Miroslav Lichvar
(Just few comments on some topics from this long post)

On Thu, Mar 18, 2021 at 11:57:20AM +0100, Frantisek Rysanek wrote:
> in a switch (than P2P), I've heard a credible opinion that in E2E 
> mode, a switch port needs to keep track of delay transactions passing 
> through (stateful style - ORLY?) and making E2E work for a single 
> slave is not the same as making it work for many slaves :-) etc.

I think that depends on whether the switch is a one-step clock or
two-step clock. In the one-step mode it shouldn't need to track
individual requests and responses. It should be stateless, but it may
be more difficult to implement in the silicon. I don't know if there
are any difficulties specific to the E2E vs P2P implementation.

To me it seems a big issue with PTP support in switches is that there
is very little information available about their performance, e.g.
resolution and typical accuracy. No public benchmarks. The vendors
just want all the features marked as "supported". I heard about cases
where it was so bad, that it was better disabled.

> TSC? Typically useable as 1PPS *input*. Having a timestamping 
> register (MSR?) on the inside, a neighbor to the TSC, which would 
> always contain a timestamp of the last exterior 1PPS edge, in the 
> TSC's "time domain". And maybe have another register yet, clocked the 
> same way as the TSC, but which would always reset at every 1PPS edge. 
> Should I patent this as the TSC-ng ? :-)

Yes, that would be great. Some small computers like the BeagleBone
Black can timestamp PPS directly without interrupts. If nothing else,
at least a fast GPIO suitable for polling would be a great improvement
on x86.

FWIW, some onboard NICs supported by the e1000e driver can
"cross-timestamp" using the Always Running Timer (ART), which should
avoid the asymmetry of PCIe. I have not seen any detailed description
of how it actually works.

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] SyncE support

2021-03-17 Thread Miroslav Lichvar
On Wed, Mar 17, 2021 at 04:49:01PM +0100, Frantisek Rysanek wrote:
> I understand that apart from the NIC (peripheral device / endpoint 
> function in general), PTM must also be supported by the root complex 
> and any PCI-e switches along the path... so a PTM-capable NIC alone 
> will not even suffice for this simple NTP-style mechanism to 
> function, if the host chipset is unaware of PTM.

Right, all those components are expected to support PTM. It's
implemented in the hardware. My understanding is that (at least some)
existing CPUs already support it. I have no idea about switches. I
guess even if they didn't support it, it could still perform better
than what we currently have. As a workaround, at least for testing,
the NIC can be inserted in the 16x slot which is connected directly to
the CPU. In servers with fast NICs and CPUs with large number of PCIe
lanes that could be the usual case.

> To account for asymmetry, the mechanism would have to resemble PTP = 
> there would have to be a correction field in the PCI-e messages and 
> the PCI-e switches would have to work like PTP TC switches :-)

The switches are supposed to work like NTP server and client at the
same time (boundary clock in the PTP terminology), so all PCIe links
have hardware timestamping on both ends.

BTW, at least in theory, a network using boundary clocks should
perform better than a similar network using transparent clocks,
assuming the servos are well configured and the sync interval is short
enough to minimize the time errors in the chain. Divide and conquer :).
I think transparent clocks are meant to be the simpler and cheaper
variant.

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] SyncE support

2021-03-17 Thread Miroslav Lichvar
On Tue, Mar 16, 2021 at 11:38:17PM +0100, Frantisek Rysanek wrote:
> Still I'm curious what news the i225 may bring in terms of e.g. GPIO 
> timestamping resolution. Does it get better than those 8 ns 
> I got used to with the i210 etc.

I have not had a chance to play with one yet. If the frequency of the
clock followed the increase in the symbol rate of 2.5GBASE-T, it would
have a ~4.8ns resolution.

> the packet timestamping is done by the NIC HW, so unless you have an 
> application where you need accurate timing all the way to the 
> software-based system clock, the determinism of PCIe bus latencies 
> should not matter much...

Right, but in most cases I see people are interested in the accuracy
and stability of the system clock. There don't seem to be many
applications using hardware timestamps of the PHC.

In some cases a sub-microsecond accuracy is required. The CPU<->NIC
connection is the most problematic link in the chain. You can buy
switches with good PTP support and compensate for asymmetric errors in
hardware timestamping to get the PHC accurate to few nanoseconds, but
due to the huge PCIe latency with an unknown asymmetry it's difficult
to tell how accurate the system clock actually is. With the same NIC
but different CPUs/boards you can get very different results.

> As for getting the Linux system timebase precise, I guess the random 
> component to ISR latency has several contributing factors, the PCI-e 
> bus latency being only one of them...

Normally there should be no interrupts involved. The time critical
part is just a single read of a 32-bit PCI register. With the I210
that takes about 1700 ns and the asymmetry in my measurements was up
to about 100 ns. The shortest delay I saw with faster NICs was about
450 ns, asymmetry unknown.

> also, the crystal oscillator 
> serving as a "stable" local reference for the PC chipset clock  
> synth is in reality not very stable. I don't have a precise figure, 
> but if I take the typical i210 NIC 25 MHz xtals for a benchmark,
> I've seen instability translating into ppb deviations of about +/- 10 
> to +/- 20 ppb, within the 1s period of ptp4l sampling - and a PC 
> chipset is likely to have an even worse oscillator...

I'd say that's normal for an uncompensated XO in a computer case where
the temperature can change rapidly. The PTP and phc2sys update rate
need to be higher to get better stability. Some PTP profiles use 128
messages/second.

-- 
Miroslav Lichvar



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


Re: [Linuxptp-devel] SyncE support

2021-03-16 Thread Miroslav Lichvar
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.

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.

-- 
Miroslav Lichvar



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


[Linuxptp-devel] [PATCH] Avoid invalid integer shifts.

2021-03-16 Thread Miroslav Lichvar
Before accepting the message interval value from a sync message, check
if it is between -10 and 22, same as required for the delay request
interval.

In calculation of the fest/stats/nrate max_count use unsigned 1 to avoid
an invalid shift by 31.

Signed-off-by: Miroslav Lichvar 
---
 clock.c |  4 ++--
 port.c  | 12 +---
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/clock.c b/clock.c
index f88df58..85d7667 100644
--- a/clock.c
+++ b/clock.c
@@ -1881,7 +1881,7 @@ void clock_sync_interval(struct clock *c, int n)
shift = sizeof(int) * 8 - 1;
pr_warning("freq_est_interval is too long");
}
-   c->fest.max_count = (1 << shift);
+   c->fest.max_count = (1U << shift);
 
shift = c->stats_interval - n;
if (shift < 0)
@@ -1890,7 +1890,7 @@ void clock_sync_interval(struct clock *c, int n)
shift = sizeof(int) * 8 - 1;
pr_warning("summary_interval is too long");
}
-   c->stats.max_count = (1 << shift);
+   c->stats.max_count = (1U << shift);
 
servo_sync_interval(c->servo, n < 0 ? 1.0 / (1 << -n) : 1 << n);
 }
diff --git a/port.c b/port.c
index cefe780..da7c327 100644
--- a/port.c
+++ b/port.c
@@ -1070,7 +1070,7 @@ static void port_nrate_initialize(struct port *p)
 
p->nrate.origin1 = tmv_zero();
p->nrate.ingress1 = tmv_zero();
-   p->nrate.max_count = (1 << shift);
+   p->nrate.max_count = (1U << shift);
p->nrate.count = 0;
p->nrate.ratio = 1.0;
p->nrate.ratio_valid = 0;
@@ -2345,8 +2345,14 @@ void process_sync(struct port *p, struct ptp_message *m)
 
if (!msg_unicast(m) &&
m->header.logMessageInterval != p->log_sync_interval) {
-   p->log_sync_interval = m->header.logMessageInterval;
-   clock_sync_interval(p->clock, p->log_sync_interval);
+   if (m->header.logMessageInterval < -10 ||
+   m->header.logMessageInterval > 22) {
+   pl_info(300, "%s: ignore bogus sync interval 2^%d",
+   p->log_name, m->header.logMessageInterval);
+   } else {
+   p->log_sync_interval = m->header.logMessageInterval;
+   clock_sync_interval(p->clock, p->log_sync_interval);
+   }
}
 
m->header.correction += p->asymmetry;
-- 
2.26.2



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


Re: [Linuxptp-devel] SyncE support

2021-03-16 Thread Miroslav Lichvar
On Mon, Mar 15, 2021 at 10:40:08PM +0100, Frantisek Rysanek wrote:
> 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 didn't find anything in the current mainline kernel. There may be
3rd party open source drivers. In the Intel's igc driver I saw few
SYNCE registers defined, but no code using them.

> But at the level of a single port... what do you need?
> SyncE enable / disable, and select master vs. slave role?

At minimum, get the current state of the link with information like
whether the port is TX/RX coherent to be able to provide the L1_SYNC
TLV.

> 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...

I'd expect the OSSP/ESMC part to be handled in the firmware. I think
ptp4l should just be able to see the current status and maybe flip
some bits to configure it.

-- 
Miroslav Lichvar



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


[Linuxptp-devel] SyncE support

2021-03-15 Thread Miroslav Lichvar
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.

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.

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.

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] [PATCH] pmc: Fix printed totalCorrectionField.

2021-03-15 Thread Miroslav Lichvar
The value needs to be shifted to right to get nanoseconds.

Signed-off-by: Miroslav Lichvar 
---
 pmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pmc.c b/pmc.c
index 1e569b5..250124d 100644
--- a/pmc.c
+++ b/pmc.c
@@ -68,7 +68,7 @@ static void pmc_show_delay_timing(struct 
slave_delay_timing_record *record,
IFMT "delayResponseTimestamp %" PRId64 ".%09u",
record->sequenceId,
SHOW_TIMESTAMP(record->delayOriginTimestamp),
-   record->totalCorrectionField << 16,
+   record->totalCorrectionField >> 16,
SHOW_TIMESTAMP(record->delayResponseTimestamp));
 }
 
@@ -83,7 +83,7 @@ static void pmc_show_rx_sync_timing(struct 
slave_rx_sync_timing_record *record,
IFMT "syncEventIngressTimestamp  %" PRId64 ".%09u",
record->sequenceId,
SHOW_TIMESTAMP(record->syncOriginTimestamp),
-   record->totalCorrectionField << 16,
+   record->totalCorrectionField >> 16,
record->scaledCumulativeRateOffset,
SHOW_TIMESTAMP(record->syncEventIngressTimestamp));
 }
-- 
2.26.2



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


[Linuxptp-devel] [PATCHv2] Avoid unaligned pointers to packed members.

2021-03-10 Thread Miroslav Lichvar
This fixes "taking address of packed member ... may result in an
unaligned pointer value [-Waddress-of-packed-member]" warnings from gcc.

Signed-off-by: Miroslav Lichvar 
---
 clock.c | 4 +++-
 msg.c   | 5 +++--
 tlv.c   | 6 +++---
 util.h  | 2 +-
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/clock.c b/clock.c
index 7005636..f88df58 100644
--- a/clock.c
+++ b/clock.c
@@ -350,6 +350,7 @@ static int clock_management_fill_response(struct clock *c, 
struct port *p,
struct time_status_np *tsn;
struct tlv_extra *extra;
struct PTPText *text;
+   uint16_t duration;
int datalen = 0;
 
extra = tlv_extra_alloc();
@@ -452,7 +453,8 @@ static int clock_management_fill_response(struct clock *c, 
struct port *p,
break;
}
sen = (struct subscribe_events_np *)tlv->data;
-   clock_get_subscription(c, req, sen->bitmask, >duration);
+   clock_get_subscription(c, req, sen->bitmask, );
+   memcpy(>duration, , sizeof(sen->duration));
datalen = sizeof(*sen);
break;
case TLV_SYNCHRONIZATION_UNCERTAIN_NP:
diff --git a/msg.c b/msg.c
index c4516ad..dcb397c 100644
--- a/msg.c
+++ b/msg.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -36,8 +37,8 @@ int assume_two_step = 0;
 
 struct message_storage {
unsigned char reserved[MSG_HEADROOM];
-   struct ptp_message msg;
-} PACKED;
+   struct ptp_message msg __attribute__((aligned (8)));
+};
 
 static TAILQ_HEAD(msg_pool, ptp_message) msg_pool = 
TAILQ_HEAD_INITIALIZER(msg_pool);
 
diff --git a/tlv.c b/tlv.c
index 879bb7e..98ef6e1 100644
--- a/tlv.c
+++ b/tlv.c
@@ -67,7 +67,7 @@ static void timestamp_net2host(struct Timestamp *t)
NTOHL(t->nanoseconds);
 }
 
-static uint16_t flip16(uint16_t *p)
+static uint16_t flip16(void *p)
 {
uint16_t v;
memcpy(, p, sizeof(v));
@@ -76,7 +76,7 @@ static uint16_t flip16(uint16_t *p)
return v;
 }
 
-static int64_t host2net64_unaligned(int64_t *p)
+static int64_t host2net64_unaligned(void *p)
 {
int64_t v;
memcpy(, p, sizeof(v));
@@ -85,7 +85,7 @@ static int64_t host2net64_unaligned(int64_t *p)
return v;
 }
 
-static int64_t net2host64_unaligned(int64_t *p)
+static int64_t net2host64_unaligned(void *p)
 {
int64_t v;
memcpy(, p, sizeof(v));
diff --git a/util.h b/util.h
index 41e33d4..739c8fd 100644
--- a/util.h
+++ b/util.h
@@ -57,7 +57,7 @@ const char *ts_str(enum timestamp_type ts);
  */
 int addreq(enum transport_type type, struct address *a, struct address *b);
 
-static inline uint16_t align16(uint16_t *p)
+static inline uint16_t align16(void *p)
 {
uint16_t v;
memcpy(, p, sizeof(v));
-- 
2.26.2



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


Re: [Linuxptp-devel] [PATCH] Avoid unaligned pointers to packed members.

2021-03-10 Thread Miroslav Lichvar
On Wed, Mar 10, 2021 at 07:11:07AM -0800, Richard Cochran wrote:
> On Wed, Mar 10, 2021 at 03:25:18PM +0100, Miroslav Lichvar wrote:
> > @@ -37,7 +38,7 @@ int assume_two_step = 0;
> >  struct message_storage {
> > unsigned char reserved[MSG_HEADROOM];
> > struct ptp_message msg;
> > -} PACKED;
> > +};
> 
> Is there a better way?  How about dropping PACKED but adding an
> alignment attribute on 'msg'?

Like this?

struct message_storage {
unsigned char reserved[MSG_HEADROOM];
struct ptp_message msg __attribute__ ((aligned (8)));
};

> 
> We don't about extra padding between 'reserved' and 'msg'.

Ok.

> > @@ -267,6 +268,10 @@ struct ptp_message *msg_allocate(void)
> > pool_stats.total++;
> > pool_debug("allocate", m);
> > }
> > +   if (sizeof(s->reserved) + sizeof(s->msg) != sizeof(*s)) {
> > +   pr_err("unexpected alignment");
> > +   exit(1);
> > +   }
> 
> This is clunky.  If we really _must_ have it, then it should be a one
> time check (msg_init()?) and not every time a message is allocated.
> But I would prefer avoiding this altogether.

It was meant as a protection against modification of the structure
that would leap to padding. It would normally be optimized away. But
if that is not a problem, I'll drop it.

-- 
Miroslav Lichvar



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


[Linuxptp-devel] [PATCH] Avoid unaligned pointers to packed members.

2021-03-10 Thread Miroslav Lichvar
This fixes "taking address of packed member ... may result in an
unaligned pointer value [-Waddress-of-packed-member]" warnings from gcc.

Signed-off-by: Miroslav Lichvar 
---
 clock.c | 4 +++-
 msg.c   | 7 ++-
 tlv.c   | 6 +++---
 util.h  | 2 +-
 4 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/clock.c b/clock.c
index 7005636..f88df58 100644
--- a/clock.c
+++ b/clock.c
@@ -350,6 +350,7 @@ static int clock_management_fill_response(struct clock *c, 
struct port *p,
struct time_status_np *tsn;
struct tlv_extra *extra;
struct PTPText *text;
+   uint16_t duration;
int datalen = 0;
 
extra = tlv_extra_alloc();
@@ -452,7 +453,8 @@ static int clock_management_fill_response(struct clock *c, 
struct port *p,
break;
}
sen = (struct subscribe_events_np *)tlv->data;
-   clock_get_subscription(c, req, sen->bitmask, >duration);
+   clock_get_subscription(c, req, sen->bitmask, );
+   memcpy(>duration, , sizeof(sen->duration));
datalen = sizeof(*sen);
break;
case TLV_SYNCHRONIZATION_UNCERTAIN_NP:
diff --git a/msg.c b/msg.c
index c4516ad..717fbdd 100644
--- a/msg.c
+++ b/msg.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -37,7 +38,7 @@ int assume_two_step = 0;
 struct message_storage {
unsigned char reserved[MSG_HEADROOM];
struct ptp_message msg;
-} PACKED;
+};
 
 static TAILQ_HEAD(msg_pool, ptp_message) msg_pool = 
TAILQ_HEAD_INITIALIZER(msg_pool);
 
@@ -267,6 +268,10 @@ struct ptp_message *msg_allocate(void)
pool_stats.total++;
pool_debug("allocate", m);
}
+   if (sizeof(s->reserved) + sizeof(s->msg) != sizeof(*s)) {
+   pr_err("unexpected alignment");
+   exit(1);
+   }
}
if (m) {
memset(m, 0, sizeof(*m));
diff --git a/tlv.c b/tlv.c
index 879bb7e..98ef6e1 100644
--- a/tlv.c
+++ b/tlv.c
@@ -67,7 +67,7 @@ static void timestamp_net2host(struct Timestamp *t)
NTOHL(t->nanoseconds);
 }
 
-static uint16_t flip16(uint16_t *p)
+static uint16_t flip16(void *p)
 {
uint16_t v;
memcpy(, p, sizeof(v));
@@ -76,7 +76,7 @@ static uint16_t flip16(uint16_t *p)
return v;
 }
 
-static int64_t host2net64_unaligned(int64_t *p)
+static int64_t host2net64_unaligned(void *p)
 {
int64_t v;
memcpy(, p, sizeof(v));
@@ -85,7 +85,7 @@ static int64_t host2net64_unaligned(int64_t *p)
return v;
 }
 
-static int64_t net2host64_unaligned(int64_t *p)
+static int64_t net2host64_unaligned(void *p)
 {
int64_t v;
memcpy(, p, sizeof(v));
diff --git a/util.h b/util.h
index 41e33d4..739c8fd 100644
--- a/util.h
+++ b/util.h
@@ -57,7 +57,7 @@ const char *ts_str(enum timestamp_type ts);
  */
 int addreq(enum transport_type type, struct address *a, struct address *b);
 
-static inline uint16_t align16(uint16_t *p)
+static inline uint16_t align16(void *p)
 {
uint16_t v;
memcpy(, p, sizeof(v));
-- 
2.26.2



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


Re: [Linuxptp-devel] [v2] msg: bump to IEEE 1588-2019 version

2021-02-25 Thread Miroslav Lichvar
On Thu, Feb 25, 2021 at 09:09:23AM -0800, Richard Cochran wrote:
> I think we should have pmc print versionNumber 2.1 here (format %u.%u)
> and then ask Miroslav to adapt the test suite...
> 
> Miroslav, I'm thinking the way to handle this in the test suite is to
> accept both versionNumber 2 and versionNumber 2.1.

Works for me. The test is using regexps, so easy to match multiple
strings. There are already some instances of that. I'll update it.

-- 
Miroslav Lichvar



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


[Linuxptp-devel] [PATCH] sk: Don't return error for zero-length messages.

2021-02-23 Thread Miroslav Lichvar
The recvmsg() call can return zero for a zero-length UDP message, which
should be handled as a bad message and not a fault of the port. This was
addressed in commit 6b61ba29c78e ("Avoid fault when receiving zero
length packets"), but later regressed in commit a6e0b83bd503
("sk: Convey transmit path errors to the caller.").

Signed-off-by: Miroslav Lichvar 
Fixes: a6e0b83bd503 ("sk: Convey transmit path errors to the caller.")
---
 sk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sk.c b/sk.c
index c9ef4d2..8be0708 100644
--- a/sk.c
+++ b/sk.c
@@ -391,7 +391,7 @@ int sk_receive(int fd, void *buf, int buflen,
 
if (!ts) {
memset(>ts, 0, sizeof(hwts->ts));
-   return cnt < 1 ? -errno : cnt;
+   return cnt < 0 ? -errno : cnt;
}
 
switch (hwts->type) {
@@ -407,7 +407,7 @@ int sk_receive(int fd, void *buf, int buflen,
hwts->ts = timespec_to_tmv(ts[1]);
break;
}
-   return cnt < 1 ? -errno : cnt;
+   return cnt < 0 ? -errno : cnt;
 }
 
 int sk_set_priority(int fd, int family, uint8_t dscp)
-- 
2.26.2



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


[Linuxptp-devel] [PATCHv3 4/6] clock: Rename UDS variables to read-write.

2021-02-11 Thread Miroslav Lichvar
In preparation for a new read-only UDS port, rename variables of the
current UDS port to make it clear it is read-write, as opposed to
read-only.

Signed-off-by: Miroslav Lichvar 
---
 clock.c | 49 +
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/clock.c b/clock.c
index 02913f3..b90c805 100644
--- a/clock.c
+++ b/clock.c
@@ -95,7 +95,7 @@ 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 pollfd *pollfd;
int pollfd_valid;
int nports; /* does not include the UDS port */
@@ -129,7 +129,7 @@ struct clock {
struct clock_stats stats;
int stats_interval;
struct clockcheck *sanity_check;
-   struct interface *udsif;
+   struct interface *uds_rw_if;
LIST_HEAD(clock_subscribers_head, clock_subscriber) subscribers;
struct monitor *slave_event_monitor;
 };
@@ -243,7 +243,7 @@ static void clock_prune_subscriptions(struct clock *c)
 void clock_send_notification(struct clock *c, struct ptp_message *msg,
 enum notification event)
 {
-   struct port *uds = c->uds_port;
+   struct port *uds = c->uds_rw_port;
struct clock_subscriber *s;
 
LIST_FOREACH(s, >subscribers, list) {
@@ -265,13 +265,13 @@ void clock_destroy(struct clock *c)
 {
struct port *p, *tmp;
 
-   interface_destroy(c->udsif);
+   interface_destroy(c->uds_rw_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);
free(c->pollfd);
if (c->clkid != CLOCK_REALTIME) {
phc_close(c->clkid);
@@ -440,7 +440,7 @@ 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) {
+   if (p != c->uds_rw_port) {
/* Only the UDS port allowed. */
break;
}
@@ -782,7 +782,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;
@@ -1042,20 +1042,20 @@ 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),
   "announceReceiptTimeout", 0)) {
return NULL;
}
-   if (config_set_section_int(config, interface_name(c->udsif),
+   if (config_set_section_int(config, interface_name(c->uds_rw_if),
"delay_mechanism", DM_AUTO)) {
return NULL;
}
-   if (config_set_section_int(config, interface_name(c->udsif),
+   if (config_set_section_int(config, interface_name(c->uds_rw_if),
"network_transport", TRANS_UDS)) {
return NULL;
}
-   if (config_set_section_int(config, interface_name(c->udsif),
+   if (config_set_section_int(config, interface_name(c->uds_rw_if),
   "delay_filter_length", 1)) {
return NULL;
}
@@ -1179,14 +1179,15 @@ struct clock *clock_create(enum clock_type type, struct 
config *config,
}
 
/* Create the UDS interface. */
-   c->uds_port = port_open(phc_device, phc_index, timestamping, 0, 
c->udsif, c);
-   if (!c->uds_port) {
+   c->uds_rw_port = port_open(phc_device, phc_index, timestamping, 0,
+  c->uds_rw_if, c);
+   if (!c->uds_rw_port) {
pr_err("failed to open the UDS port");
return NULL;
}
clock_fda_changed(c);
 
-   c->slave_event_monitor = monitor_create(config, c->uds_port);
+   c->slave_event_monitor = monitor_create(config, c->uds_rw_port);
if (!c->slave_event_monitor) {
pr_err("failed to create slave event monitor");
return NULL;
@@ -1205,7 +1206,7 @@ struct clock *clock_create(en

[Linuxptp-devel] [PATCHv3 1/6] port: Don't assume transport from port number.

2021-02-11 Thread Miroslav Lichvar
In port_open(), don't assume that UDS ports always have to have a zero
number. Check the transport directly to make the code cleaner. While at
it, switch all checks for the UDS in the port code to use a helper
function.

Signed-off-by: Miroslav Lichvar 
---
 port.c | 40 +++-
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/port.c b/port.c
index f44d239..9be611c 100644
--- a/port.c
+++ b/port.c
@@ -771,6 +771,11 @@ static int port_is_ieee8021as(struct port *p)
return p->follow_up_info ? 1 : 0;
 }
 
+static int port_is_uds(struct port *p)
+{
+   return transport_type(p->trp) == TRANS_UDS;
+}
+
 static void port_management_send_error(struct port *p, struct port *ingress,
   struct ptp_message *msg, int error_id)
 {
@@ -1755,7 +1760,7 @@ int port_initialize(struct port *p)
}
 
/* No need to open rtnl socket on UDS port. */
-   if (transport_type(p->trp) != TRANS_UDS) {
+   if (!port_is_uds(p)) {
/*
 * The delay timer is usually started when the device
 * transitions to PS_LISTENING. But, we are skipping the state
@@ -3014,7 +3019,6 @@ struct port *port_open(const char *phc_device,
enum clock_type type = clock_type(clock);
struct config *cfg = clock_config(clock);
struct port *p = malloc(sizeof(*p));
-   enum transport_type transport;
int i;
 
if (!p) {
@@ -3050,24 +3054,28 @@ struct port *port_open(const char *phc_device,
 
p->phc_index = phc_index;
p->jbod = config_get_int(cfg, interface_name(interface), 
"boundary_clock_jbod");
-   transport = config_get_int(cfg, interface_name(interface), 
"network_transport");
p->master_only = config_get_int(cfg, interface_name(interface), 
"masterOnly");
p->bmca = config_get_int(cfg, interface_name(interface), "BMCA");
+   p->trp = transport_create(cfg, config_get_int(cfg,
+ interface_name(interface), "network_transport"));
+   if (!p->trp) {
+   goto err_log_name;
+   }
 
-   if (p->bmca == BMCA_NOOP && transport != TRANS_UDS) {
+   if (p->bmca == BMCA_NOOP && !port_is_uds(p)) {
if (p->master_only) {
p->state_machine = designated_master_fsm;
} else if (clock_slave_only(clock)) {
p->state_machine = designated_slave_fsm;
} else {
pr_err("Please enable at least one of masterOnly or 
clientOnly when BMCA == noop.\n");
-   goto err_log_name;
+   goto err_transport;
}
} else {
p->state_machine = clock_slave_only(clock) ? ptp_slave_fsm : 
ptp_fsm;
}
 
-   if (transport == TRANS_UDS) {
+   if (port_is_uds(p)) {
; /* UDS cannot have a PHC. */
} else if (!interface_tsinfo_valid(interface)) {
pr_warning("%s: get_ts_info not supported", p->log_name);
@@ -3087,14 +3095,14 @@ struct port *port_open(const char *phc_device,
pr_err("%s: /dev/ptp%d requested, ptp%d attached",
   p->log_name, phc_index,
   interface_phc_index(interface));
-   goto err_log_name;
+   goto err_transport;
}
}
 
p->iface = interface;
p->asymmetry = config_get_int(cfg, p->name, "delayAsymmetry");
p->asymmetry <<= 16;
-   p->announce_span = transport == TRANS_UDS ? 0 : ANNOUNCE_SPAN;
+   p->announce_span = port_is_uds(p) ? 0 : ANNOUNCE_SPAN;
p->follow_up_info = config_get_int(cfg, p->name, "follow_up_info");
p->freq_est_interval = config_get_int(cfg, p->name, 
"freq_est_interval");
p->msg_interval_request = config_get_int(cfg, p->name, 
"msg_interval_request");
@@ -3107,10 +3115,6 @@ struct port *port_open(const char *phc_device,
p->tx_timestamp_offset <<= 16;
p->link_status = LINK_UP;
p->clock = clock;
-   p->trp = transport_create(cfg, transport);
-   if (!p->trp) {
-   goto err_log_name;
-   }
p->timestamping = timestamping;
p->portIdentity.clockIdentity = clock_identity(clock);
p->portIdentity.portNumber = number;
@@ -3119,23 +3123,25 @@ struct port *port_open(const char *phc_device,
p->versionNumber = PTP_VERSION;
p->slave_event_monitor = clock_slave_monitor(clock);
 
-   if (number && unicast_client_initialize(p)) {
+   if (!port_is_uds(p) && unicast_client_initialize(p)) {
  

[Linuxptp-devel] [PATCHv3 3/6] clock: Don't allow COMMAND action on non-UDS port.

2021-02-11 Thread Miroslav Lichvar
No COMMAND actions are currently supported, but check the port early in
clock_manage() before reaching port_manage().

Signed-off-by: Miroslav Lichvar 
---
 clock.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/clock.c b/clock.c
index 6aa9b10..02913f3 100644
--- a/clock.c
+++ b/clock.c
@@ -1422,6 +1422,11 @@ int clock_manage(struct clock *c, struct port *p, struct 
ptp_message *msg)
return changed;
break;
case COMMAND:
+   if (p != c->uds_port) {
+   /* Sorry, only allowed on the UDS port. */
+   clock_management_send_error(p, msg, TLV_NOT_SUPPORTED);
+   return changed;
+   }
break;
default:
return changed;
-- 
2.26.2



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


[Linuxptp-devel] [PATCHv3 6/6] timemaster: Set uds_ro_address for ptp4l instances.

2021-02-11 Thread Miroslav Lichvar
This prevents conflicts on the new UDS-RO port.

Signed-off-by: Miroslav Lichvar 
---
 timemaster.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/timemaster.c b/timemaster.c
index fb27d72..616a99a 100644
--- a/timemaster.c
+++ b/timemaster.c
@@ -712,7 +712,7 @@ static int add_ptp_source(struct ptp_domain *source,
  char **ntp_config, struct script *script)
 {
struct config_file *config_file;
-   char **command, *uds_path, **interfaces, *message_tag;
+   char **command, *uds_path, *uds_path2, **interfaces, *message_tag;
char ts_interface[IF_NAMESIZE];
int i, j, num_interfaces, *phc, *phcs, hw_ts, sw_ts;
struct sk_ts_info ts_info;
@@ -809,6 +809,8 @@ static int add_ptp_source(struct ptp_domain *source,
 
uds_path = string_newf("%s/ptp4l.%d.socket",
   config->rundir, *shm_segment);
+   uds_path2 = string_newf("%s/ptp4lro.%d.socket",
+   config->rundir, *shm_segment);
 
message_tag = string_newf("[%d", source->domain);
for (j = 0; interfaces[j]; j++)
@@ -832,8 +834,10 @@ static int add_ptp_source(struct ptp_domain *source,
   "clientOnly 1\n"
   "domainNumber %d\n"
   "uds_address %s\n"
+  "uds_ro_address %s\n"
   "message_tag %s\n",
-  source->domain, uds_path, message_tag);
+  source->domain, uds_path, uds_path2,
+  message_tag);
 
if (phcs[i] >= 0) {
/* HW time stamping */
@@ -868,6 +872,7 @@ static int add_ptp_source(struct ptp_domain *source,
 
free(message_tag);
free(uds_path);
+   free(uds_path2);
free(interfaces);
}
 
-- 
2.26.2



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


[Linuxptp-devel] [PATCHv3 5/6] clock: Add read-only UDS port for monitoring.

2021-02-11 Thread Miroslav Lichvar
Add a second UDS port to allow untrusted applications to monitor ptp4l.
On this "read-only" UDS port disable non-GET actions and forwarding.
The path can be configured with the uds_ro_address option (default is
/var/run/ptp4lro).

Forwarding is disabled to limit the access to the local ptp4l instance.

Subscriptions are not enabled to prevent the applications from making a
large number of subscriptions or interfere with applications that have
access to the read-write UDS port.

Signed-off-by: Miroslav Lichvar 
---
 clock.c | 72 +
 config.c|  1 +
 configs/default.cfg |  1 +
 ptp4l.8 |  6 
 4 files changed, 67 insertions(+), 13 deletions(-)

diff --git a/clock.c b/clock.c
index b90c805..e6be5fd 100644
--- a/clock.c
+++ b/clock.c
@@ -96,9 +96,10 @@ struct clock {
struct ClockIdentity best_id;
LIST_HEAD(ports_head, port) ports;
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;
@@ -130,6 +131,7 @@ struct clock {
int stats_interval;
struct clockcheck *sanity_check;
struct interface *uds_rw_if;
+   struct interface *uds_ro_if;
LIST_HEAD(clock_subscribers_head, clock_subscriber) subscribers;
struct monitor *slave_event_monitor;
 };
@@ -266,12 +268,14 @@ void clock_destroy(struct clock *c)
struct port *p, *tmp;
 
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_rw_port);
+   port_close(c->uds_ro_port);
free(c->pollfd);
if (c->clkid != CLOCK_REALTIME) {
phc_close(c->clkid);
@@ -441,7 +445,7 @@ static int clock_management_fill_response(struct clock *c, 
struct port *p,
break;
case TLV_SUBSCRIBE_EVENTS_NP:
if (p != c->uds_rw_port) {
-   /* Only the UDS port allowed. */
+   /* Only the UDS-RW port allowed. */
break;
}
sen = (struct subscribe_events_np *)tlv->data;
@@ -772,6 +776,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:
@@ -816,7 +824,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,
@@ -1041,6 +1049,7 @@ struct clock *clock_create(enum clock_type type, struct 
config *config,
}
 
/* Configure the UDS. */
+
uds_ifname = config_get_string(config, NULL, "uds_address");
c->uds_rw_if = interface_create(uds_ifname);
if (config_set_section_int(config, interface_name(c->uds_rw_if),
@@ -1060,6 +1069,25 @@ struct clock *clock_create(enum clock_type type, struct 
config *config,
return NULL;
}
 
+   uds_ifname = config_get_string(config, NULL, "uds_ro_address");
+   c->uds_ro_if = interface_create(uds_ifname);
+   if (config_set_section_int(config, interface_name(c->uds_ro_if),
+  "announceReceiptTimeout", 0)) {
+   return NULL;
+   }
+   if (config_set_section_int(config, interface_name(c->uds_ro_if),
+  "delay_mechanism", DM_AUTO)) {
+   return NULL;
+   }
+   if (config_set_section_int(config, interface_name(c->uds_ro_if),
+  "network_transport", TRANS_UDS)) {
+   return NULL;
+   }
+   if (config_set_section_int(config, interface_name(c->uds_ro_if),
+  "delay_filter_length", 1)) {
+   return NULL;
+   }
+
c->config = config;
c->free_running = config_get_int(config, NULL, "free_running");
c->freq_est_interval = config_get_int(config, NULL, 
"freq_est_interval");
@@ -1178,11 +1206,18 @@ struct clock *clock_create(enum clock_type type, struct 
config *config,
return NULL;
  

[Linuxptp-devel] [PATCHv3 2/6] port: Ignore non-management messages on UDS port.

2021-02-11 Thread Miroslav Lichvar
Drop non-management messages on the UDS port early in the processing to
prevent them from changing the port or clock state.

Signed-off-by: Miroslav Lichvar 
---
 port.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/port.c b/port.c
index 9be611c..2bb974c 100644
--- a/port.c
+++ b/port.c
@@ -56,6 +56,7 @@ enum syfu_event {
 };
 
 static int port_is_ieee8021as(struct port *p);
+static int port_is_uds(struct port *p);
 static void port_nrate_initialize(struct port *p);
 
 static int announce_compare(struct ptp_message *m1, struct ptp_message *m2)
@@ -691,6 +692,9 @@ static int port_ignore(struct port *p, struct ptp_message 
*m)
 {
struct ClockIdentity c1, c2;
 
+   if (port_is_uds(p) && msg_type(m) != MANAGEMENT) {
+   return 1;
+   }
if (incapable_ignore(p, m)) {
return 1;
}
-- 
2.26.2



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


[Linuxptp-devel] [PATCHv3 0/6] GET-only UDS port

2021-02-11 Thread Miroslav Lichvar
v3:
- rebased on current HEAD
- changes suggested by Richard
v2:
- renamed uds_address2 option to uds_ro_address
- added a helper function for UDS check in port.c

This patchset adds a new UDS port to be used by untrusted applications
for monitoring purposes.

The first four patches are cleanup and preparation.

The fifth patch is the main change. As this could easily lead to
security issues, please check what code is exposed on the UDS-RO port.
We need to be sure that it cannot be exploited to cause crashes, changes
in the port or clock state, etc.

The intention is to drop any non-management messages just few calls
after transport_recv() and respond with errors to non-GET actions in
clock_manage(). Only GET actions can get to port_manage(). No message
should be able to get to clock_management_set() and
port_management_set().

Miroslav Lichvar (6):
  port: Don't assume transport from port number.
  port: Ignore non-management messages on UDS port.
  clock: Don't allow COMMAND action on non-UDS port.
  clock: Rename UDS variables to read-write.
  clock: Add read-only UDS port for monitoring.
  timemaster: Set uds_ro_address for ptp4l instances.

 clock.c | 122 +++-
 config.c|   1 +
 configs/default.cfg |   1 +
 port.c  |  44 ++--
 ptp4l.8 |   6 +++
 timemaster.c|   9 +++-
 6 files changed, 129 insertions(+), 54 deletions(-)

-- 
2.26.2



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


Re: [Linuxptp-devel] [PATCH] This code changes brings in the ability to program the acceptable clockClass threshold beyond which device will move to holdover/free-run. Default clockClass threshold is

2021-02-11 Thread Miroslav Lichvar
On Thu, Feb 11, 2021 at 10:17:32AM +, Karthikkumar V wrote:
> --- a/config.c
> +++ b/config.c
> @@ -332,6 +332,7 @@ struct config_item config_tab[] = {
>   GLOB_ITEM_INT("utc_offset", CURRENT_UTC_OFFSET, 0, INT_MAX),
>   GLOB_ITEM_INT("verbose", 0, 0, 1),
>   GLOB_ITEM_INT("write_phase_mode", 0, 0, 1),
> + GLOB_ITEM_INT("clockClassThreshold", CLOCK_CLASS_THRESHOLD_DEFAULT, 6, 
> CLOCK_CLASS_THRESHOLD_DEFAULT),
>  };

As this doesn't seem to come from the PTP specification, maybe it
would be better to name it clock_class_threshold?

Please add a description to the ptp4l man page.

-- 
Miroslav Lichvar



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


[Linuxptp-devel] [PATCHv2 5/6] clock: Add read-only UDS port for monitoring.

2021-01-28 Thread Miroslav Lichvar
Add a second UDS port to allow untrusted applications to monitor ptp4l.
On this "read-only" UDS port disable non-GET actions and forwarding.
The path can be configured with the uds_ro_address option (default is
/var/run/ptp4lro).

Forwarding is disabled to limit the access to the local ptp4l instance.

Subscriptions are not enabled to prevent the applications from making a
large number of subscriptions or interfere with applications that have
access to the read-write UDS port.

Signed-off-by: Miroslav Lichvar 
---
 clock.c | 74 +
 config.c|  1 +
 configs/default.cfg |  1 +
 ptp4l.8 |  6 
 4 files changed, 69 insertions(+), 13 deletions(-)

diff --git a/clock.c b/clock.c
index 12f4d8c..4e1d195 100644
--- a/clock.c
+++ b/clock.c
@@ -96,9 +96,10 @@ struct clock {
struct ClockIdentity best_id;
LIST_HEAD(ports_head, port) ports;
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;
@@ -130,6 +131,7 @@ struct clock {
int stats_interval;
struct clockcheck *sanity_check;
struct interface *uds_rw_if;
+   struct interface *uds_ro_if;
LIST_HEAD(clock_subscribers_head, clock_subscriber) subscribers;
struct monitor *slave_event_monitor;
 };
@@ -268,12 +270,14 @@ void clock_destroy(struct clock *c)
struct port *p, *tmp;
 
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_rw_port);
+   port_close(c->uds_ro_port);
free(c->pollfd);
if (c->clkid != CLOCK_REALTIME) {
phc_close(c->clkid);
@@ -443,7 +447,7 @@ static int clock_management_fill_response(struct clock *c, 
struct port *p,
break;
case TLV_SUBSCRIBE_EVENTS_NP:
if (p != c->uds_rw_port) {
-   /* Only the UDS port allowed. */
+   /* 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:
@@ -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,
@@ -1043,6 +1051,7 @@ struct clock *clock_create(enum clock_type type, struct 
config *config,
}
 
/* Configure the UDS. */
+
uds_ifname = config_get_string(config, NULL, "uds_address");
c->uds_rw_if = interface_create(uds_ifname);
if (config_set_section_int(config, interface_name(c->uds_rw_if),
@@ -1062,6 +1071,25 @@ struct clock *clock_create(enum clock_type type, struct 
config *config,
return NULL;
}
 
+   uds_ifname = config_get_string(config, NULL, "uds_ro_address");
+   c->uds_ro_if = interface_create(uds_ifname);
+   if (config_set_section_int(config, interface_name(c->uds_ro_if),
+  "announceReceiptTimeout", 0)) {
+   return NULL;
+   }
+   if (config_set_section_int(config, interface_name(c->uds_ro_if),
+  "delay_mechanism", DM_AUTO)) {
+   return NULL;
+   }
+   if (config_set_section_int(config, interface_name(c->uds_ro_if),
+  "network_transport", TRANS_UDS)) {
+   return NULL;
+   }
+   if (config_set_section_int(config, interface_name(c->uds_ro_if),
+  "delay_filter_length", 1)) {
+   return NULL;
+   }
+
c->config = config;
c->free_running = config_get_int(config, NULL, "free_running");
c->freq_est_interval = config_get_int(config, NULL, 
"freq_est_interval");
@@ -1180,11 +1208,12 @@ struct clock *clock_create(enum clock_type type, struct 
config *config,
return NULL;
  

[Linuxptp-devel] [PATCHv2 6/6] timemaster: Set uds_ro_address for ptp4l instances.

2021-01-28 Thread Miroslav Lichvar
This prevents conflicts on the new UDS-RO port.

Signed-off-by: Miroslav Lichvar 
---
 timemaster.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/timemaster.c b/timemaster.c
index fb27d72..616a99a 100644
--- a/timemaster.c
+++ b/timemaster.c
@@ -712,7 +712,7 @@ static int add_ptp_source(struct ptp_domain *source,
  char **ntp_config, struct script *script)
 {
struct config_file *config_file;
-   char **command, *uds_path, **interfaces, *message_tag;
+   char **command, *uds_path, *uds_path2, **interfaces, *message_tag;
char ts_interface[IF_NAMESIZE];
int i, j, num_interfaces, *phc, *phcs, hw_ts, sw_ts;
struct sk_ts_info ts_info;
@@ -809,6 +809,8 @@ static int add_ptp_source(struct ptp_domain *source,
 
uds_path = string_newf("%s/ptp4l.%d.socket",
   config->rundir, *shm_segment);
+   uds_path2 = string_newf("%s/ptp4lro.%d.socket",
+   config->rundir, *shm_segment);
 
message_tag = string_newf("[%d", source->domain);
for (j = 0; interfaces[j]; j++)
@@ -832,8 +834,10 @@ static int add_ptp_source(struct ptp_domain *source,
   "clientOnly 1\n"
   "domainNumber %d\n"
   "uds_address %s\n"
+  "uds_ro_address %s\n"
   "message_tag %s\n",
-  source->domain, uds_path, message_tag);
+  source->domain, uds_path, uds_path2,
+  message_tag);
 
if (phcs[i] >= 0) {
/* HW time stamping */
@@ -868,6 +872,7 @@ static int add_ptp_source(struct ptp_domain *source,
 
free(message_tag);
free(uds_path);
+   free(uds_path2);
free(interfaces);
}
 
-- 
2.26.2



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


[Linuxptp-devel] [PATCHv2 2/6] port: Ignore non-management messages on UDS port.

2021-01-28 Thread Miroslav Lichvar
Drop non-management messages on the UDS port early in the processing to
prevent them from changing the port or clock state.

Signed-off-by: Miroslav Lichvar 
---
 port.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/port.c b/port.c
index 87f2c06..f491252 100644
--- a/port.c
+++ b/port.c
@@ -691,6 +691,9 @@ static int port_ignore(struct port *p, struct ptp_message 
*m)
 {
struct ClockIdentity c1, c2;
 
+   if (transport_type(p->trp) == TRANS_UDS && msg_type(m) != MANAGEMENT) {
+   return 1;
+   }
if (incapable_ignore(p, m)) {
return 1;
}
-- 
2.26.2



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


[Linuxptp-devel] [PATCHv2 1/6] port: Don't assume transport from port number.

2021-01-28 Thread Miroslav Lichvar
In port_open(), don't assume that UDS ports always have to have a zero
number. Check the transport directly to make the code cleaner. While at
it, switch all checks for the UDS in the port code to use a helper
function.

Signed-off-by: Miroslav Lichvar 
---
 port.c | 40 +++-
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/port.c b/port.c
index 0a93c06..87f2c06 100644
--- a/port.c
+++ b/port.c
@@ -771,6 +771,11 @@ static int port_is_ieee8021as(struct port *p)
return p->follow_up_info ? 1 : 0;
 }
 
+static int port_is_uds(struct port *p)
+{
+   return transport_type(p->trp) == TRANS_UDS;
+}
+
 static void port_management_send_error(struct port *p, struct port *ingress,
   struct ptp_message *msg, int error_id)
 {
@@ -1755,7 +1760,7 @@ int port_initialize(struct port *p)
}
 
/* No need to open rtnl socket on UDS port. */
-   if (transport_type(p->trp) != TRANS_UDS) {
+   if (!port_is_uds(p)) {
/*
 * The delay timer is usually started when the device
 * transitions to PS_LISTENING. But, we are skipping the state
@@ -3008,7 +3013,6 @@ struct port *port_open(const char *phc_device,
enum clock_type type = clock_type(clock);
struct config *cfg = clock_config(clock);
struct port *p = malloc(sizeof(*p));
-   enum transport_type transport;
int i;
 
if (!p) {
@@ -3038,24 +3042,28 @@ struct port *port_open(const char *phc_device,
 
p->phc_index = phc_index;
p->jbod = config_get_int(cfg, interface_name(interface), 
"boundary_clock_jbod");
-   transport = config_get_int(cfg, interface_name(interface), 
"network_transport");
p->master_only = config_get_int(cfg, interface_name(interface), 
"masterOnly");
p->bmca = config_get_int(cfg, interface_name(interface), "BMCA");
+   p->trp = transport_create(cfg, config_get_int(cfg,
+ interface_name(interface), "network_transport"));
+   if (!p->trp) {
+   goto err_port;
+   }
 
-   if (p->bmca == BMCA_NOOP && transport != TRANS_UDS) {
+   if (p->bmca == BMCA_NOOP && !port_is_uds(p)) {
if (p->master_only) {
p->state_machine = designated_master_fsm;
} else if (clock_slave_only(clock)) {
p->state_machine = designated_slave_fsm;
} else {
pr_err("Please enable at least one of masterOnly or 
clientOnly when BMCA == noop.\n");
-   goto err_port;
+   goto err_transport;
}
} else {
p->state_machine = clock_slave_only(clock) ? ptp_slave_fsm : 
ptp_fsm;
}
 
-   if (transport == TRANS_UDS) {
+   if (port_is_uds(p)) {
; /* UDS cannot have a PHC. */
} else if (!interface_tsinfo_valid(interface)) {
pr_warning("port %d: get_ts_info not supported", number);
@@ -3075,7 +3083,7 @@ struct port *port_open(const char *phc_device,
pr_err("port %d: /dev/ptp%d requested, ptp%d attached",
   number, phc_index,
   interface_phc_index(interface));
-   goto err_port;
+   goto err_transport;
}
}
 
@@ -3083,7 +3091,7 @@ struct port *port_open(const char *phc_device,
p->iface = interface;
p->asymmetry = config_get_int(cfg, p->name, "delayAsymmetry");
p->asymmetry <<= 16;
-   p->announce_span = transport == TRANS_UDS ? 0 : ANNOUNCE_SPAN;
+   p->announce_span = port_is_uds(p) ? 0 : ANNOUNCE_SPAN;
p->follow_up_info = config_get_int(cfg, p->name, "follow_up_info");
p->freq_est_interval = config_get_int(cfg, p->name, 
"freq_est_interval");
p->msg_interval_request = config_get_int(cfg, p->name, 
"msg_interval_request");
@@ -3096,10 +3104,6 @@ struct port *port_open(const char *phc_device,
p->tx_timestamp_offset <<= 16;
p->link_status = LINK_UP;
p->clock = clock;
-   p->trp = transport_create(cfg, transport);
-   if (!p->trp) {
-   goto err_port;
-   }
p->timestamping = timestamping;
p->portIdentity.clockIdentity = clock_identity(clock);
p->portIdentity.portNumber = number;
@@ -3108,23 +3112,25 @@ struct port *port_open(const char *phc_device,
p->versionNumber = PTP_VERSION;
p->slave_event_monitor = clock_slave_monitor(clock);
 
-   if (number && unicast_client_initialize(p)) {
+   if (!por

[Linuxptp-devel] [PATCHv2 0/6] GET-only UDS port

2021-01-28 Thread Miroslav Lichvar
v2:
- renamed uds_address2 option to uds_ro_address
- added a helper function for UDS check in port.c

This patchset adds a new UDS port to be used by untrusted applications
for monitoring purposes.

The first four patches are cleanup and preparation.

The fifth patch is the main change. As this could easily lead to
security issues, please check what code is exposed on the UDS-RO port.
We need to be sure that it cannot be exploited to cause crashes, changes
in the port or clock state, etc.

The intention is to drop any non-management messages just few calls
after transport_recv() and respond with errors to non-GET actions in
clock_manage(). Only GET actions can get to port_manage(). No message
should be able to get to clock_management_set() and
port_management_set().

Miroslav Lichvar (6):
  port: Don't assume transport from port number.
  port: Ignore non-management messages on UDS port.
  clock: Don't allow COMMAND action on non-UDS port.
  clock: Rename UDS variables to read-write.
  clock: Add read-only UDS port for monitoring.
  timemaster: Set uds_ro_address for ptp4l instances.

 clock.c | 124 +++-
 config.c|   1 +
 configs/default.cfg |   1 +
 port.c  |  43 +--
 ptp4l.8 |   6 +++
 timemaster.c|   9 +++-
 6 files changed, 130 insertions(+), 54 deletions(-)

-- 
2.26.2



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


[Linuxptp-devel] [PATCHv2 4/6] clock: Rename UDS variables to read-write.

2021-01-28 Thread Miroslav Lichvar
In preparation for a new read-only UDS port, rename variables of the
current UDS port to make it clear it is read-write, as opposed to
read-only.

Signed-off-by: Miroslav Lichvar 
---
 clock.c | 49 +
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/clock.c b/clock.c
index aff9589..12f4d8c 100644
--- a/clock.c
+++ b/clock.c
@@ -95,7 +95,7 @@ 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 pollfd *pollfd;
int pollfd_valid;
int nports; /* does not include the UDS port */
@@ -129,7 +129,7 @@ struct clock {
struct clock_stats stats;
int stats_interval;
struct clockcheck *sanity_check;
-   struct interface *udsif;
+   struct interface *uds_rw_if;
LIST_HEAD(clock_subscribers_head, clock_subscriber) subscribers;
struct monitor *slave_event_monitor;
 };
@@ -245,7 +245,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 +267,13 @@ void clock_destroy(struct clock *c)
 {
struct port *p, *tmp;
 
-   interface_destroy(c->udsif);
+   interface_destroy(c->uds_rw_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);
free(c->pollfd);
if (c->clkid != CLOCK_REALTIME) {
phc_close(c->clkid);
@@ -442,7 +442,7 @@ 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) {
+   if (p != c->uds_rw_port) {
/* Only the UDS port allowed. */
break;
}
@@ -784,7 +784,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;
@@ -1044,20 +1044,20 @@ 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),
   "announceReceiptTimeout", 0)) {
return NULL;
}
-   if (config_set_section_int(config, interface_name(c->udsif),
+   if (config_set_section_int(config, interface_name(c->uds_rw_if),
"delay_mechanism", DM_AUTO)) {
return NULL;
}
-   if (config_set_section_int(config, interface_name(c->udsif),
+   if (config_set_section_int(config, interface_name(c->uds_rw_if),
"network_transport", TRANS_UDS)) {
return NULL;
}
-   if (config_set_section_int(config, interface_name(c->udsif),
+   if (config_set_section_int(config, interface_name(c->uds_rw_if),
   "delay_filter_length", 1)) {
return NULL;
}
@@ -1181,14 +1181,15 @@ struct clock *clock_create(enum clock_type type, struct 
config *config,
}
 
/* Create the UDS interface. */
-   c->uds_port = port_open(phc_device, phc_index, timestamping, 0, 
c->udsif, c);
-   if (!c->uds_port) {
+   c->uds_rw_port = port_open(phc_device, phc_index, timestamping, 0,
+  c->uds_rw_if, c);
+   if (!c->uds_rw_port) {
pr_err("failed to open the UDS port");
return NULL;
}
clock_fda_changed(c);
 
-   c->slave_event_monitor = monitor_create(config, c->uds_port);
+   c->slave_event_monitor = monitor_create(config, c->uds_rw_port);
if (!c->slave_event_monitor) {
pr_err("failed to create slave event monitor");
return NULL;
@@ -1207,7 +1208,7 @@ struct clock *clock_create(enum clock_type type, 

[Linuxptp-devel] [PATCHv2 3/6] clock: Don't allow COMMAND action on non-UDS port.

2021-01-28 Thread Miroslav Lichvar
No COMMAND actions are currently supported, but check the port early in
clock_manage() before reaching port_manage().

Signed-off-by: Miroslav Lichvar 
---
 clock.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/clock.c b/clock.c
index 08c61eb..aff9589 100644
--- a/clock.c
+++ b/clock.c
@@ -1424,6 +1424,11 @@ int clock_manage(struct clock *c, struct port *p, struct 
ptp_message *msg)
return changed;
break;
case COMMAND:
+   if (p != c->uds_port) {
+   /* Sorry, only allowed on the UDS port. */
+   clock_management_send_error(p, msg, TLV_NOT_SUPPORTED);
+   return changed;
+   }
break;
default:
return changed;
-- 
2.26.2



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


[Linuxptp-devel] [PATCH 1/6] port: Don't assume transport from port number.

2021-01-26 Thread Miroslav Lichvar
In port_open(), don't assume that UDS ports always have to have a zero
number. Check the transport directly to make make the code cleaner.

Signed-off-by: Miroslav Lichvar 
---
 port.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/port.c b/port.c
index 0a93c06..6136153 100644
--- a/port.c
+++ b/port.c
@@ -3108,23 +3108,25 @@ struct port *port_open(const char *phc_device,
p->versionNumber = PTP_VERSION;
p->slave_event_monitor = clock_slave_monitor(clock);
 
-   if (number && unicast_client_initialize(p)) {
+   if (transport != TRANS_UDS && unicast_client_initialize(p)) {
goto err_transport;
}
if (unicast_client_enabled(p) &&
config_set_section_int(cfg, p->name, "hybrid_e2e", 1)) {
goto err_uc_client;
}
-   if (number && unicast_service_initialize(p)) {
+   if (transport != TRANS_UDS && unicast_service_initialize(p)) {
goto err_uc_client;
}
p->hybrid_e2e = config_get_int(cfg, p->name, "hybrid_e2e");
 
-   if (number && type == CLOCK_TYPE_P2P && p->delayMechanism != DM_P2P) {
+   if (transport != TRANS_UDS &&
+   type == CLOCK_TYPE_P2P && p->delayMechanism != DM_P2P) {
pr_err("port %d: P2P TC needs P2P ports", number);
goto err_uc_service;
}
-   if (number && type == CLOCK_TYPE_E2E && p->delayMechanism != DM_E2E) {
+   if (transport != TRANS_UDS &&
+   type == CLOCK_TYPE_E2E && p->delayMechanism != DM_E2E) {
pr_err("port %d: E2E TC needs E2E ports", number);
goto err_uc_service;
}
@@ -3158,7 +3160,7 @@ struct port *port_open(const char *phc_device,
 
port_clear_fda(p, N_POLLFD);
p->fault_fd = -1;
-   if (number) {
+   if (transport != TRANS_UDS) {
p->fault_fd = timerfd_create(CLOCK_MONOTONIC, 0);
if (p->fault_fd < 0) {
pr_err("timerfd_create failed: %m");
-- 
2.26.2



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


[Linuxptp-devel] [PATCH 5/6] clock: Add read-only UDS port for monitoring.

2021-01-26 Thread Miroslav Lichvar
Add a second UDS port to allow untrusted applications to monitor ptp4l.
On this "read-only" UDS port disable non-GET actions and forwarding.
The path can be configured with the uds_address2 option (default is
/var/run/ptp4lro).

Forwarding is disabled to limit the access to the local ptp4l instance.

Subscriptions are not enabled to prevent the applications from making a
large number of subscriptions or interfere with applications that have
access to the read-write UDS port.

Signed-off-by: Miroslav Lichvar 
---
 clock.c | 74 +
 config.c|  1 +
 configs/default.cfg |  1 +
 ptp4l.8 |  6 
 4 files changed, 69 insertions(+), 13 deletions(-)

diff --git a/clock.c b/clock.c
index 12f4d8c..fb11c54 100644
--- a/clock.c
+++ b/clock.c
@@ -96,9 +96,10 @@ struct clock {
struct ClockIdentity best_id;
LIST_HEAD(ports_head, port) ports;
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;
@@ -130,6 +131,7 @@ struct clock {
int stats_interval;
struct clockcheck *sanity_check;
struct interface *uds_rw_if;
+   struct interface *uds_ro_if;
LIST_HEAD(clock_subscribers_head, clock_subscriber) subscribers;
struct monitor *slave_event_monitor;
 };
@@ -268,12 +270,14 @@ void clock_destroy(struct clock *c)
struct port *p, *tmp;
 
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_rw_port);
+   port_close(c->uds_ro_port);
free(c->pollfd);
if (c->clkid != CLOCK_REALTIME) {
phc_close(c->clkid);
@@ -443,7 +447,7 @@ static int clock_management_fill_response(struct clock *c, 
struct port *p,
break;
case TLV_SUBSCRIBE_EVENTS_NP:
if (p != c->uds_rw_port) {
-   /* Only the UDS port allowed. */
+   /* 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:
@@ -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,
@@ -1043,6 +1051,7 @@ struct clock *clock_create(enum clock_type type, struct 
config *config,
}
 
/* Configure the UDS. */
+
uds_ifname = config_get_string(config, NULL, "uds_address");
c->uds_rw_if = interface_create(uds_ifname);
if (config_set_section_int(config, interface_name(c->uds_rw_if),
@@ -1062,6 +1071,25 @@ struct clock *clock_create(enum clock_type type, struct 
config *config,
return NULL;
}
 
+   uds_ifname = config_get_string(config, NULL, "uds_address2");
+   c->uds_ro_if = interface_create(uds_ifname);
+   if (config_set_section_int(config, interface_name(c->uds_ro_if),
+  "announceReceiptTimeout", 0)) {
+   return NULL;
+   }
+   if (config_set_section_int(config, interface_name(c->uds_ro_if),
+  "delay_mechanism", DM_AUTO)) {
+   return NULL;
+   }
+   if (config_set_section_int(config, interface_name(c->uds_ro_if),
+  "network_transport", TRANS_UDS)) {
+   return NULL;
+   }
+   if (config_set_section_int(config, interface_name(c->uds_ro_if),
+  "delay_filter_length", 1)) {
+   return NULL;
+   }
+
c->config = config;
c->free_running = config_get_int(config, NULL, "free_running");
c->freq_est_interval = config_get_int(config, NULL, 
"freq_est_interval");
@@ -1180,11 +1208,12 @@ struct clock *clock_create(enum clock_type type, struct 
config *config,
return NULL;
  

[Linuxptp-devel] [PATCH 2/6] port: Ignore non-management messages on UDS port.

2021-01-26 Thread Miroslav Lichvar
Drop non-management messages on the UDS port early in the processing to
prevent them from changing the port or clock state.

Signed-off-by: Miroslav Lichvar 
---
 port.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/port.c b/port.c
index 6136153..ed33131 100644
--- a/port.c
+++ b/port.c
@@ -691,6 +691,9 @@ static int port_ignore(struct port *p, struct ptp_message 
*m)
 {
struct ClockIdentity c1, c2;
 
+   if (transport_type(p->trp) == TRANS_UDS && msg_type(m) != MANAGEMENT) {
+   return 1;
+   }
if (incapable_ignore(p, m)) {
return 1;
}
-- 
2.26.2



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


[Linuxptp-devel] [PATCH 6/6] timemaster: Set uds_address2 for ptp4l instances.

2021-01-26 Thread Miroslav Lichvar
This prevents conflicts on the new UDS-RO port.

Signed-off-by: Miroslav Lichvar 
---
 timemaster.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/timemaster.c b/timemaster.c
index fb27d72..c180a78 100644
--- a/timemaster.c
+++ b/timemaster.c
@@ -712,7 +712,7 @@ static int add_ptp_source(struct ptp_domain *source,
  char **ntp_config, struct script *script)
 {
struct config_file *config_file;
-   char **command, *uds_path, **interfaces, *message_tag;
+   char **command, *uds_path, *uds_path2, **interfaces, *message_tag;
char ts_interface[IF_NAMESIZE];
int i, j, num_interfaces, *phc, *phcs, hw_ts, sw_ts;
struct sk_ts_info ts_info;
@@ -809,6 +809,8 @@ static int add_ptp_source(struct ptp_domain *source,
 
uds_path = string_newf("%s/ptp4l.%d.socket",
   config->rundir, *shm_segment);
+   uds_path2 = string_newf("%s/ptp4lro.%d.socket",
+   config->rundir, *shm_segment);
 
message_tag = string_newf("[%d", source->domain);
for (j = 0; interfaces[j]; j++)
@@ -832,8 +834,10 @@ static int add_ptp_source(struct ptp_domain *source,
   "clientOnly 1\n"
   "domainNumber %d\n"
   "uds_address %s\n"
+  "uds_address2 %s\n"
   "message_tag %s\n",
-  source->domain, uds_path, message_tag);
+  source->domain, uds_path, uds_path2,
+  message_tag);
 
if (phcs[i] >= 0) {
/* HW time stamping */
@@ -868,6 +872,7 @@ static int add_ptp_source(struct ptp_domain *source,
 
free(message_tag);
free(uds_path);
+   free(uds_path2);
free(interfaces);
}
 
-- 
2.26.2



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


[Linuxptp-devel] [PATCH 0/6] GET-only UDS port

2021-01-26 Thread Miroslav Lichvar
This patchset adds a new UDS port to be used by untrusted applications
for monitoring purposes.

The first four patches are cleanup and preparation. Please feel free to
skip the first patch.

The fifth patch is the main change. As this could easily lead to
security issues, please check what code is exposed on the UDS-RO port.
We need to be sure that it cannot be exploited to cause crashes, changes
in the port or clock state, etc.

The intention is to drop any non-management messages just few calls
after transport_recv() and respond with errors to non-GET actions in
clock_manage(). Only GET actions can get to port_manage(). No message
should be able to get to clock_management_set() and
port_management_set().

Miroslav Lichvar (6):
  port: Don't assume transport from port number.
  port: Ignore non-management messages on UDS port.
  clock: Don't allow COMMAND action on non-UDS port.
  clock: Rename UDS variables to read-write.
  clock: Add read-only UDS port for monitoring.
  timemaster: Set uds_address2 for ptp4l instances.

 clock.c | 124 +++-
 config.c|   1 +
 configs/default.cfg |   1 +
 port.c  |  15 --
 ptp4l.8 |   6 +++
 timemaster.c|   9 +++-
 6 files changed, 114 insertions(+), 42 deletions(-)

-- 
2.26.2



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


[Linuxptp-devel] [PATCH 3/6] clock: Don't allow COMMAND action on non-UDS port.

2021-01-26 Thread Miroslav Lichvar
No COMMAND actions are currently supported, but check the port early in
clock_manage() before reaching port_manage().

Signed-off-by: Miroslav Lichvar 
---
 clock.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/clock.c b/clock.c
index 08c61eb..aff9589 100644
--- a/clock.c
+++ b/clock.c
@@ -1424,6 +1424,11 @@ int clock_manage(struct clock *c, struct port *p, struct 
ptp_message *msg)
return changed;
break;
case COMMAND:
+   if (p != c->uds_port) {
+   /* Sorry, only allowed on the UDS port. */
+   clock_management_send_error(p, msg, TLV_NOT_SUPPORTED);
+   return changed;
+   }
break;
default:
return changed;
-- 
2.26.2



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


[Linuxptp-devel] [PATCH 4/6] clock: Rename UDS variables to read-write.

2021-01-26 Thread Miroslav Lichvar
In preparation for a new read-only UDS port, rename variables of the
current UDS port to make it clear it is read-write, as opposed to
read-only.

Signed-off-by: Miroslav Lichvar 
---
 clock.c | 49 +
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/clock.c b/clock.c
index aff9589..12f4d8c 100644
--- a/clock.c
+++ b/clock.c
@@ -95,7 +95,7 @@ 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 pollfd *pollfd;
int pollfd_valid;
int nports; /* does not include the UDS port */
@@ -129,7 +129,7 @@ struct clock {
struct clock_stats stats;
int stats_interval;
struct clockcheck *sanity_check;
-   struct interface *udsif;
+   struct interface *uds_rw_if;
LIST_HEAD(clock_subscribers_head, clock_subscriber) subscribers;
struct monitor *slave_event_monitor;
 };
@@ -245,7 +245,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 +267,13 @@ void clock_destroy(struct clock *c)
 {
struct port *p, *tmp;
 
-   interface_destroy(c->udsif);
+   interface_destroy(c->uds_rw_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);
free(c->pollfd);
if (c->clkid != CLOCK_REALTIME) {
phc_close(c->clkid);
@@ -442,7 +442,7 @@ 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) {
+   if (p != c->uds_rw_port) {
/* Only the UDS port allowed. */
break;
}
@@ -784,7 +784,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;
@@ -1044,20 +1044,20 @@ 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),
   "announceReceiptTimeout", 0)) {
return NULL;
}
-   if (config_set_section_int(config, interface_name(c->udsif),
+   if (config_set_section_int(config, interface_name(c->uds_rw_if),
"delay_mechanism", DM_AUTO)) {
return NULL;
}
-   if (config_set_section_int(config, interface_name(c->udsif),
+   if (config_set_section_int(config, interface_name(c->uds_rw_if),
"network_transport", TRANS_UDS)) {
return NULL;
}
-   if (config_set_section_int(config, interface_name(c->udsif),
+   if (config_set_section_int(config, interface_name(c->uds_rw_if),
   "delay_filter_length", 1)) {
return NULL;
}
@@ -1181,14 +1181,15 @@ struct clock *clock_create(enum clock_type type, struct 
config *config,
}
 
/* Create the UDS interface. */
-   c->uds_port = port_open(phc_device, phc_index, timestamping, 0, 
c->udsif, c);
-   if (!c->uds_port) {
+   c->uds_rw_port = port_open(phc_device, phc_index, timestamping, 0,
+  c->uds_rw_if, c);
+   if (!c->uds_rw_port) {
pr_err("failed to open the UDS port");
return NULL;
}
clock_fda_changed(c);
 
-   c->slave_event_monitor = monitor_create(config, c->uds_port);
+   c->slave_event_monitor = monitor_create(config, c->uds_rw_port);
if (!c->slave_event_monitor) {
pr_err("failed to create slave event monitor");
return NULL;
@@ -1207,7 +1208,7 @@ struct clock *clock_create(enum clock_type type, 

  1   2   3   4   >