Re: [Linuxptp-devel] [PATCH v4] Rework twoStepFlag in order to handle one step on port basis.

2021-05-23 Thread Richard Cochran
On Thu, Apr 22, 2021 at 09:18:33AM +0200, Luigi 'Comio' Mantellini wrote:
> With this patch we introduce the twoStepFlag evaluation at port level.

This isn't going to work.  The two step flag is an element of a clock,
not port, data set.

8.2.1.2.1 defaultDS.twoStepFlag

The value of defaultDS.twoStepFlag shall be TRUE if the clock is a
two-step clock; otherwise, the value shall be FALSE.

Sorry,
Richard


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


Re: [Linuxptp-devel] [PATCH v4] Rework twoStepFlag in order to handle one step on port basis.

2021-04-22 Thread Geva, Erez


On 22/04/2021 09:18, Luigi 'Comio' Mantellini wrote:
> With this patch we introduce the twoStepFlag evaluation at port level.
> 
> ---
>   config.c |  2 +-
>   port.c   | 35 ++-
>   ptp4l.8  |  9 -
>   3 files changed, 39 insertions(+), 7 deletions(-)
> 
> diff --git a/config.c b/config.c
> index 4472d3d..f0e1e07 100644
> --- a/config.c
> +++ b/config.c
> @@ -322,7 +322,7 @@ struct config_item config_tab[] = {
>   PORT_ITEM_INT("ts2phc.pin_index", 0, 0, INT_MAX),
>   GLOB_ITEM_INT("ts2phc.pulsewidth", 5, 100, 99900),
>   PORT_ITEM_ENU("tsproc_mode", TSPROC_FILTER, tsproc_enu),
> - GLOB_ITEM_INT("twoStepFlag", 1, 0, 1),
> + PORT_ITEM_INT("twoStepFlag", 1, 0, 1),
>   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),
> diff --git a/port.c b/port.c
> index 10bb9e1..b700ff3 100644
> --- a/port.c
> +++ b/port.c
> @@ -3028,6 +3028,37 @@ err:
>   msg_put(msg);
>   }
>   
> +static enum timestamp_type port_harmonize_onestep(struct port *p, enum 
> timestamp_type timestamping, int twoStepFlag)
> +{
> + switch (timestamping) {
> + case TS_SOFTWARE:
> + case TS_LEGACY_HW:
> + if (!twoStepFlag) {
> + pr_err("%s: one step is only possible "
> +"with hardware time stamping", p->log_name);
> + return timestamping;
> + }
> + break;
> + case TS_HARDWARE:
> + if (!twoStepFlag) {
> + pr_debug("%s: upgrading to one step time stamping "
> +  "in order to match the port.twoStepFlag", 
> p->log_name);
> + return TS_ONESTEP;
> + }
> + break;
> + case TS_ONESTEP:
> + case TS_P2P1STEP:
> + if (twoStepFlag) {
> + pr_debug("%s: degrading to two step time stamping, "
> +  "in order to match the port.twoStepFlag", 
> p->log_name);
> + return TS_HARDWARE;
> + }
> + break;
> + }
> +
> + return timestamping;
> +}
> +

As user do configure the time_stamping, I think that changing of timestamping 
require a flag that permit it, or alternatively generate error.
I think that automatic probing of timestamping without proper configuration may 
confuse users.

We should allow time_stamping per port.

Just because I choose one step or two steps, does not guarantee I like the 
resulting timestamping.
In addition, the driver might not support the probed timestamping.

I do like probing. I just think it requires a flag that the user can decide to 
use it or not.

>   struct port *port_open(const char *phc_device,
>  int phc_index,
>  enum timestamp_type timestamping,
> @@ -3039,6 +3070,7 @@ struct port *port_open(const char *phc_device,
>   struct config *cfg = clock_config(clock);
>   struct port *p = malloc(sizeof(*p));
>   int i;
> + int twoStepFlag;
>   
>   if (!p) {
>   return NULL;
> @@ -3134,7 +3166,8 @@ struct port *port_open(const char *phc_device,
>   p->tx_timestamp_offset <<= 16;
>   p->link_status = LINK_UP;
>   p->clock = clock;
> - p->timestamping = timestamping;
> + twoStepFlag = config_get_int(cfg, p->name, "twoStepFlag");
> + p->timestamping = port_harmonize_onestep(p, timestamping, twoStepFlag);
>   p->portIdentity.clockIdentity = clock_identity(clock);
>   p->portIdentity.portNumber = number;
>   p->state = PS_INITIALIZING;
> diff --git a/ptp4l.8 b/ptp4l.8
> index fe9e150..bea4dae 100644
> --- a/ptp4l.8
> +++ b/ptp4l.8
> @@ -143,6 +143,10 @@ See UNICAST DISCOVERY OPTIONS, below.
>   .SH PORT OPTIONS
>   
>   .TP
> +.B twoStepFlag
> +Enable two-step mode for sync messages. One-step mode can be used only with
> +hardware time stamping.
> +The default is 1 (enabled).
>   .B delayAsymmetry
>   The time difference in nanoseconds of the transmit and receive
>   paths. This value should be positive when the server-to-client
> @@ -378,11 +382,6 @@ to the same subnet.
>   
>   .SH PROGRAM AND CLOCK OPTIONS
>   
> -.TP
> -.B twoStepFlag
> -Enable two-step mode for sync messages. One-step mode can be used only with
> -hardware time stamping.
> -The default is 1 (enabled).
>   .TP
>   .B clientOnly
>   The local clock is a client-only clock if enabled. The default is 0 
> (disabled).
> 

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


[Linuxptp-devel] [PATCH v4] Rework twoStepFlag in order to handle one step on port basis.

2021-04-22 Thread Luigi 'Comio' Mantellini
With this patch we introduce the twoStepFlag evaluation at port level.

---
 config.c |  2 +-
 port.c   | 35 ++-
 ptp4l.8  |  9 -
 3 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/config.c b/config.c
index 4472d3d..f0e1e07 100644
--- a/config.c
+++ b/config.c
@@ -322,7 +322,7 @@ struct config_item config_tab[] = {
PORT_ITEM_INT("ts2phc.pin_index", 0, 0, INT_MAX),
GLOB_ITEM_INT("ts2phc.pulsewidth", 5, 100, 99900),
PORT_ITEM_ENU("tsproc_mode", TSPROC_FILTER, tsproc_enu),
-   GLOB_ITEM_INT("twoStepFlag", 1, 0, 1),
+   PORT_ITEM_INT("twoStepFlag", 1, 0, 1),
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),
diff --git a/port.c b/port.c
index 10bb9e1..b700ff3 100644
--- a/port.c
+++ b/port.c
@@ -3028,6 +3028,37 @@ err:
msg_put(msg);
 }
 
+static enum timestamp_type port_harmonize_onestep(struct port *p, enum 
timestamp_type timestamping, int twoStepFlag)
+{
+   switch (timestamping) {
+   case TS_SOFTWARE:
+   case TS_LEGACY_HW:
+   if (!twoStepFlag) {
+   pr_err("%s: one step is only possible "
+  "with hardware time stamping", p->log_name);
+   return timestamping;
+   }
+   break;
+   case TS_HARDWARE:
+   if (!twoStepFlag) {
+   pr_debug("%s: upgrading to one step time stamping "
+"in order to match the port.twoStepFlag", 
p->log_name);
+   return TS_ONESTEP;
+   }
+   break;
+   case TS_ONESTEP:
+   case TS_P2P1STEP:
+   if (twoStepFlag) {
+   pr_debug("%s: degrading to two step time stamping, "
+"in order to match the port.twoStepFlag", 
p->log_name);
+   return TS_HARDWARE;
+   }
+   break;
+   }
+
+   return timestamping;
+}
+
 struct port *port_open(const char *phc_device,
   int phc_index,
   enum timestamp_type timestamping,
@@ -3039,6 +3070,7 @@ struct port *port_open(const char *phc_device,
struct config *cfg = clock_config(clock);
struct port *p = malloc(sizeof(*p));
int i;
+   int twoStepFlag;
 
if (!p) {
return NULL;
@@ -3134,7 +3166,8 @@ struct port *port_open(const char *phc_device,
p->tx_timestamp_offset <<= 16;
p->link_status = LINK_UP;
p->clock = clock;
-   p->timestamping = timestamping;
+   twoStepFlag = config_get_int(cfg, p->name, "twoStepFlag");
+   p->timestamping = port_harmonize_onestep(p, timestamping, twoStepFlag);
p->portIdentity.clockIdentity = clock_identity(clock);
p->portIdentity.portNumber = number;
p->state = PS_INITIALIZING;
diff --git a/ptp4l.8 b/ptp4l.8
index fe9e150..bea4dae 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -143,6 +143,10 @@ See UNICAST DISCOVERY OPTIONS, below.
 .SH PORT OPTIONS
 
 .TP
+.B twoStepFlag
+Enable two-step mode for sync messages. One-step mode can be used only with
+hardware time stamping.
+The default is 1 (enabled).
 .B delayAsymmetry
 The time difference in nanoseconds of the transmit and receive
 paths. This value should be positive when the server-to-client
@@ -378,11 +382,6 @@ to the same subnet.
 
 .SH PROGRAM AND CLOCK OPTIONS
 
-.TP
-.B twoStepFlag
-Enable two-step mode for sync messages. One-step mode can be used only with
-hardware time stamping.
-The default is 1 (enabled).
 .TP
 .B clientOnly
 The local clock is a client-only clock if enabled. The default is 0 (disabled).
-- 
2.31.1



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