[Linuxptp-devel] [PATCH] Use L4 timestamping unless requested otherwise

[Jiri Benc]

With UDP, use L4 timestamping unless overriden by rx_timestamp_l2only. This
allows ptp4l to work with drivers that do not support L2 timestamping (like
sfc).

The sfc driver returns -ERANGE as a response to HWTSTAMP_FILTER_PTP_V2_EVENT
which causes ptp4l to output:
ioctl SIOCSHWTSTAMP failed: Numerical result out of range
and get stuck later.

Signed-off-by: Jiri Benc <***@redhat.com>
---
raw.c | 2 +-
sk.c | 16 +++++++++++-----
sk.h | 12 +++++++-----
udp.c | 2 +-
udp6.c | 2 +-
5 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/raw.c b/raw.c
index 3fe04c1..5511582 100644
--- a/raw.c
+++ b/raw.c
@@ -206,7 +206,7 @@ static int raw_open(struct transport *t, char *name,
if (gfd < 0)
goto no_general;

- if (sk_timestamping_init(efd, name, ts_type))
+ if (sk_timestamping_init(efd, name, ts_type, TRANS_IEEE_802_3))
goto no_timestamping;

fda->fd[FD_EVENT] = efd;
diff --git a/sk.c b/sk.c
index b006c57..aabee4b 100644
--- a/sk.c
+++ b/sk.c
@@ -29,6 +29,7 @@

#include "print.h"
#include "sk.h"
+#include "transport.h"

/* globals */

@@ -36,7 +37,7 @@ int sk_tx_retries = 2, sk_prefer_layer2 = 0;

/* private methods */

-static int hwts_init(int fd, char *device)
+static int hwts_init(int fd, char *device, enum transport_type transport)
{
struct ifreq ifreq;
struct hwtstamp_config cfg, req;
@@ -49,8 +50,12 @@ static int hwts_init(int fd, char *device)

ifreq.ifr_data = (void *) &cfg;
cfg.tx_type = HWTSTAMP_TX_ON;
- cfg.rx_filter = sk_prefer_layer2 ?
- HWTSTAMP_FILTER_PTP_V2_L2_EVENT : HWTSTAMP_FILTER_PTP_V2_EVENT;
+ if (sk_prefer_layer2)
+ cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
+ else if (transport == TRANS_UDP_IPV4 || transport == TRANS_UDP_IPV6)
+ cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;
+ else
+ cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;

req = cfg;
err = ioctl(fd, SIOCSHWTSTAMP, &ifreq);
@@ -223,7 +228,8 @@ int sk_receive(int fd, void *buf, int buflen,
return cnt;
}

-int sk_timestamping_init(int fd, char *device, enum timestamp_type type)
+int sk_timestamping_init(int fd, char *device, enum timestamp_type type,
+ enum transport_type transport)
{
int flags;

@@ -247,7 +253,7 @@ int sk_timestamping_init(int fd, char *device, enum timestamp_type type)
return -1;
}

- if (type != TS_SOFTWARE && hwts_init(fd, device))
+ if (type != TS_SOFTWARE && hwts_init(fd, device, transport))
return -1;

if (setsockopt(fd, SOL_SOCKET, SO_TIMESTAMPING,
diff --git a/sk.h b/sk.h
index b2787b2..b6ce88c 100644
--- a/sk.h
+++ b/sk.h
@@ -61,12 +61,14 @@ int sk_receive(int fd, void *buf, int buflen,

/**
* Enable time stamping on a given network interface.
- * @param fd An open socket.
- * @param device The name of the network interface to configure.
- * @param type The requested flavor of time stamping.
- * @return Zero on success, non-zero otherwise.
+ * @param fd An open socket.
+ * @param device The name of the network interface to configure.
+ * @param type The requested flavor of time stamping.
+ * @param transport The type of transport used.
+ * @return Zero on success, non-zero otherwise.
*/
-int sk_timestamping_init(int fd, char *device, enum timestamp_type type);
+int sk_timestamping_init(int fd, char *device, enum timestamp_type type,
+ enum transport_type transport);

/**
* Limits the number of RECVMSG(2) calls when attempting to obtain a
diff --git a/udp.c b/udp.c
index b361ff4..d0aab6f 100644
--- a/udp.c
+++ b/udp.c
@@ -155,7 +155,7 @@ static int udp_open(struct transport *t, char *name, struct fdarray *fda,
if (gfd < 0)
goto no_general;

- if (sk_timestamping_init(efd, name, ts_type))
+ if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV4))
goto no_timestamping;

fda->fd[FD_EVENT] = efd;
diff --git a/udp6.c b/udp6.c
index 115cd83..e6023a6 100644
--- a/udp6.c
+++ b/udp6.c
@@ -154,7 +154,7 @@ static int udp6_open(struct transport *t, char *name, struct fdarray *fda,
if (gfd < 0)
goto no_general;

- if (sk_timestamping_init(efd, name, ts_type))
+ if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV6))
goto no_timestamping;

fda->fd[FD_EVENT] = efd;

[Richard Cochran]

Post by Jiri Benc
With UDP, use L4 timestamping unless overriden by rx_timestamp_l2only. This
allows ptp4l to work with drivers that do not support L2 timestamping (like
sfc).
The sfc driver returns -ERANGE as a response to HWTSTAMP_FILTER_PTP_V2_EVENT
ioctl SIOCSHWTSTAMP failed: Numerical result out of range
and get stuck later.

Yes, I can see that this is a real problem, but I think it better to
try the most general setting first, and when it fails, then try the
more specific setting.

This patch is a good start... and I'll see what I can come up with.

Thanks,
Richard

[Jiri Benc]

When L2 timestamping is not supported by the kernel driver, fallback to L4
timestamping if permitted (i.e. UDP is used and rx_timestamp_l2only config
option is not set). This allows ptp4l to work with drivers that do not
support L2 timestamping (like sfc).

The sfc driver returns -ERANGE as a response to HWTSTAMP_FILTER_PTP_V2_EVENT
which causes ptp4l to output:
ioctl SIOCSHWTSTAMP failed: Numerical result out of range
and get stuck later.

Signed-off-by: Jiri Benc <***@redhat.com>

---

Post by Richard Cochran
Yes, I can see that this is a real problem, but I think it better to
try the most general setting first, and when it fails, then try the
more specific setting.

Okay. What about this?

---
raw.c | 2 +-
sk.c | 15 ++++++++++++---
sk.h | 12 +++++++-----
udp.c | 2 +-
udp6.c | 2 +-
5 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/raw.c b/raw.c
index 3fe04c1..5511582 100644
--- a/raw.c
+++ b/raw.c
@@ -206,7 +206,7 @@ static int raw_open(struct transport *t, char *name,
if (gfd < 0)
goto no_general;

- if (sk_timestamping_init(efd, name, ts_type))
+ if (sk_timestamping_init(efd, name, ts_type, TRANS_IEEE_802_3))
goto no_timestamping;

fda->fd[FD_EVENT] = efd;
diff --git a/sk.c b/sk.c
index b006c57..edbb99d 100644
--- a/sk.c
+++ b/sk.c
@@ -29,6 +29,7 @@

#include "print.h"
#include "sk.h"
+#include "transport.h"

/* globals */

@@ -36,7 +37,7 @@ int sk_tx_retries = 2, sk_prefer_layer2 = 0;

/* private methods */

-static int hwts_init(int fd, char *device)
+static int hwts_init(int fd, char *device, enum transport_type transport)
{
struct ifreq ifreq;
struct hwtstamp_config cfg, req;
@@ -54,6 +55,13 @@ static int hwts_init(int fd, char *device)

req = cfg;
err = ioctl(fd, SIOCSHWTSTAMP, &ifreq);
+ if (err < 0 && errno == ERANGE && !sk_prefer_layer2 &&
+ (transport == TRANS_UDP_IPV4 || transport == TRANS_UDP_IPV6)) {
+ /* L2 timestamping not supported, try L4 */
+ req.rx_filter = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;
+ cfg = req;
+ err = ioctl(fd, SIOCSHWTSTAMP, &ifreq);
+ }
if (err < 0)
pr_err("ioctl SIOCSHWTSTAMP failed: %m");

@@ -223,7 +231,8 @@ int sk_receive(int fd, void *buf, int buflen,
return cnt;
}

-int sk_timestamping_init(int fd, char *device, enum timestamp_type type)
+int sk_timestamping_init(int fd, char *device, enum timestamp_type type,
+ enum transport_type transport)
{
int flags;

@@ -247,7 +256,7 @@ int sk_timestamping_init(int fd, char *device, enum timestamp_type type)
return -1;
}

- if (type != TS_SOFTWARE && hwts_init(fd, device))
+ if (type != TS_SOFTWARE && hwts_init(fd, device, transport))
return -1;

if (setsockopt(fd, SOL_SOCKET, SO_TIMESTAMPING,
diff --git a/sk.h b/sk.h
index b2787b2..b6ce88c 100644
--- a/sk.h
+++ b/sk.h
@@ -61,12 +61,14 @@ int sk_receive(int fd, void *buf, int buflen,

/**
* Enable time stamping on a given network interface.
- * @param fd An open socket.
- * @param device The name of the network interface to configure.
- * @param type The requested flavor of time stamping.
- * @return Zero on success, non-zero otherwise.
+ * @param fd An open socket.
+ * @param device The name of the network interface to configure.
+ * @param type The requested flavor of time stamping.
+ * @param transport The type of transport used.
+ * @return Zero on success, non-zero otherwise.
*/
-int sk_timestamping_init(int fd, char *device, enum timestamp_type type);
+int sk_timestamping_init(int fd, char *device, enum timestamp_type type,
+ enum transport_type transport);

/**
* Limits the number of RECVMSG(2) calls when attempting to obtain a
diff --git a/udp.c b/udp.c
index b361ff4..d0aab6f 100644
--- a/udp.c
+++ b/udp.c
@@ -155,7 +155,7 @@ static int udp_open(struct transport *t, char *name, struct fdarray *fda,
if (gfd < 0)
goto no_general;

- if (sk_timestamping_init(efd, name, ts_type))
+ if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV4))
goto no_timestamping;

fda->fd[FD_EVENT] = efd;
diff --git a/udp6.c b/udp6.c
index 115cd83..e6023a6 100644
--- a/udp6.c
+++ b/udp6.c
@@ -154,7 +154,7 @@ static int udp6_open(struct transport *t, char *name, struct fdarray *fda,
if (gfd < 0)
goto no_general;

- if (sk_timestamping_init(efd, name, ts_type))
+ if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV6))
goto no_timestamping;

fda->fd[FD_EVENT] = efd;

[Richard Cochran]

This is my idea on how to handle the case when a driver only supports
a specific time stamping option.

Compile tested only.

Richard Cochran (3):
Pass transport type to time stamping initialization function.
Try two different HWTSTAMP options.
Remove useless option selecting layer 2 time stamping.

config.c | 4 ----
config.h | 1 -
default.cfg | 1 -
gPTP.cfg | 1 -
ptp4l.c | 1 -
raw.c | 2 +-
sk.c | 45 ++++++++++++++++++++++++++++++++++-----------
sk.h | 18 +++++++-----------
udp.c | 2 +-
udp6.c | 2 +-
10 files changed, 44 insertions(+), 33 deletions(-)

[Keller, Jacob E]

ACK

Looks good to me.

- Jake

-----Original Message-----
Sent: Thursday, October 25, 2012 12:11 PM
Cc: Jiri Benc
Subject: [Linuxptp-devel] [PATCH RFC 0/3] improve HWTSTAMP selection
This is my idea on how to handle the case when a driver only supports
a specific time stamping option.
Compile tested only.
Pass transport type to time stamping initialization function.
Try two different HWTSTAMP options.
Remove useless option selecting layer 2 time stamping.
config.c | 4 ----
config.h | 1 -
default.cfg | 1 -
gPTP.cfg | 1 -
ptp4l.c | 1 -
raw.c | 2 +-
sk.c | 45 ++++++++++++++++++++++++++++++++++-----------
sk.h | 18 +++++++-----------
udp.c | 2 +-
udp6.c | 2 +-
10 files changed, 44 insertions(+), 33 deletions(-)
--
1.7.2.5
--------------------------------------------------------------------------
----
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
http://p.sf.net/sfu/appdyn_sfd2d_oct
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Jiri Benc]

Post by Richard Cochran
This is my idea on how to handle the case when a driver only supports
a specific time stamping option.
Compile tested only.

Tested-by: Jiri Benc <***@redhat.com>

Works for me and looks good.

Thanks,

Jiri

[Richard Cochran]

Suggested-by: Jiri Benc <***@redhat.com>
Signed-off-by: Richard Cochran <***@gmail.com>
---
raw.c | 2 +-
sk.c | 3 ++-
sk.h | 12 +++++++-----
udp.c | 2 +-
udp6.c | 2 +-
5 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/raw.c b/raw.c
index 3fe04c1..5511582 100644
--- a/raw.c
+++ b/raw.c
@@ -206,7 +206,7 @@ static int raw_open(struct transport *t, char *name,
if (gfd < 0)
goto no_general;

- if (sk_timestamping_init(efd, name, ts_type))
+ if (sk_timestamping_init(efd, name, ts_type, TRANS_IEEE_802_3))
goto no_timestamping;

fda->fd[FD_EVENT] = efd;
diff --git a/sk.c b/sk.c
index b006c57..cd9aa11 100644
--- a/sk.c
+++ b/sk.c
@@ -223,7 +223,8 @@ int sk_receive(int fd, void *buf, int buflen,
return cnt;
}

-int sk_timestamping_init(int fd, char *device, enum timestamp_type type)
+int sk_timestamping_init(int fd, char *device, enum timestamp_type type,
+ enum transport_type transport)
{
int flags;

diff --git a/sk.h b/sk.h
index b2787b2..b6ce88c 100644
--- a/sk.h
+++ b/sk.h
@@ -61,12 +61,14 @@ int sk_receive(int fd, void *buf, int buflen,

/**
* Enable time stamping on a given network interface.
- * @param fd An open socket.
- * @param device The name of the network interface to configure.
- * @param type The requested flavor of time stamping.
- * @return Zero on success, non-zero otherwise.
+ * @param fd An open socket.
+ * @param device The name of the network interface to configure.
+ * @param type The requested flavor of time stamping.
+ * @param transport The type of transport used.
+ * @return Zero on success, non-zero otherwise.
*/
-int sk_timestamping_init(int fd, char *device, enum timestamp_type type);
+int sk_timestamping_init(int fd, char *device, enum timestamp_type type,
+ enum transport_type transport);

/**
* Limits the number of RECVMSG(2) calls when attempting to obtain a
diff --git a/udp.c b/udp.c
index b361ff4..d0aab6f 100644
--- a/udp.c
+++ b/udp.c
@@ -155,7 +155,7 @@ static int udp_open(struct transport *t, char *name, struct fdarray *fda,
if (gfd < 0)
goto no_general;

- if (sk_timestamping_init(efd, name, ts_type))
+ if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV4))
goto no_timestamping;

fda->fd[FD_EVENT] = efd;
diff --git a/udp6.c b/udp6.c
index 115cd83..e6023a6 100644
--- a/udp6.c
+++ b/udp6.c
@@ -154,7 +154,7 @@ static int udp6_open(struct transport *t, char *name, struct fdarray *fda,
if (gfd < 0)
goto no_general;

- if (sk_timestamping_init(efd, name, ts_type))
+ if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV6))
goto no_timestamping;

fda->fd[FD_EVENT] = efd;

[Richard Cochran]

Start with the most general HWTSTAMP option. If that fails, fall back
to the option that best fits the interface's transport.

Signed-off-by: Richard Cochran <***@gmail.com>
---
sk.c | 40 +++++++++++++++++++++++++++++++---------
1 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/sk.c b/sk.c
index cd9aa11..8c9b9fa 100644
--- a/sk.c
+++ b/sk.c
@@ -36,7 +36,7 @@ int sk_tx_retries = 2, sk_prefer_layer2 = 0;

/* private methods */

-static int hwts_init(int fd, char *device)
+static int hwts_init(int fd, char *device, int rx_filter)
{
struct ifreq ifreq;
struct hwtstamp_config cfg, req;
@@ -49,13 +49,11 @@ static int hwts_init(int fd, char *device)

ifreq.ifr_data = (void *) &cfg;
cfg.tx_type = HWTSTAMP_TX_ON;
- cfg.rx_filter = sk_prefer_layer2 ?
- HWTSTAMP_FILTER_PTP_V2_L2_EVENT : HWTSTAMP_FILTER_PTP_V2_EVENT;
-
+ cfg.rx_filter = rx_filter;
req = cfg;
err = ioctl(fd, SIOCSHWTSTAMP, &ifreq);
if (err < 0)
- pr_err("ioctl SIOCSHWTSTAMP failed: %m");
+ return err;

if (memcmp(&cfg, &req, sizeof(cfg))) {

@@ -70,7 +68,7 @@ static int hwts_init(int fd, char *device)
}
}

- return err ? errno : 0;
+ return 0;
}

/* public methods */
@@ -226,7 +224,7 @@ int sk_receive(int fd, void *buf, int buflen,
int sk_timestamping_init(int fd, char *device, enum timestamp_type type,
enum transport_type transport)
{
- int flags;
+ int err, filter1, filter2, flags;

switch (type) {
case TS_SOFTWARE:
@@ -248,8 +246,32 @@ int sk_timestamping_init(int fd, char *device, enum timestamp_type type,
return -1;
}

- if (type != TS_SOFTWARE && hwts_init(fd, device))
- return -1;
+ if (type != TS_SOFTWARE) {
+ filter1 = HWTSTAMP_FILTER_PTP_V2_EVENT;
+ switch (transport) {
+ case TRANS_UDP_IPV4:
+ case TRANS_UDP_IPV6:
+ filter2 = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;
+ break;
+ case TRANS_IEEE_802_3:
+ filter2 = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
+ break;
+ case TRANS_DEVICENET:
+ case TRANS_CONTROLNET:
+ case TRANS_PROFINET:
+ case TRANS_UDS:
+ return -1;
+ }
+ err = hwts_init(fd, device, filter1);
+ if (err) {
+ pr_info("driver rejected most general HWTSTAMP filter");
+ err = hwts_init(fd, device, filter2);
+ if (err) {
+ pr_err("ioctl SIOCSHWTSTAMP failed: %m");
+ return err;
+ }
+ }
+ }

if (setsockopt(fd, SOL_SOCKET, SO_TIMESTAMPING,
&flags, sizeof(flags)) < 0) {

[Richard Cochran]

Now that the code automatically falls back to transport-specific time
stamping, this option is no longer needed.

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 4 ----
config.h | 1 -
default.cfg | 1 -
gPTP.cfg | 1 -
ptp4l.c | 1 -
sk.c | 2 +-
sk.h | 6 ------
7 files changed, 1 insertions(+), 15 deletions(-)

diff --git a/config.c b/config.c
index ef02880..6444108 100644
--- a/config.c
+++ b/config.c
@@ -209,10 +209,6 @@ static void scan_global_line(const char *s, struct config *cfg)
if (val > 0)
*cfg->tx_timestamp_retries = val;

- } else if (1 == sscanf(s, " rx_timestamp_l2only %u", &val)) {
-
- *cfg->rx_timestamp_l2only = val ? 1 : 0;
-
} else if (1 == sscanf(s, " pi_proportional_const %lf", &df)) {

if (df > 0.0 && df < 1.0)
diff --git a/config.h b/config.h
index 8840b41..7cffc32 100644
--- a/config.h
+++ b/config.h
@@ -60,7 +60,6 @@ struct config {
struct port_defaults pod;
int *assume_two_step;
int *tx_timestamp_retries;
- int *rx_timestamp_l2only;

enum servo_type clock_servo;

diff --git a/default.cfg b/default.cfg
index 9c3bc32..189ef5a 100644
--- a/default.cfg
+++ b/default.cfg
@@ -28,7 +28,6 @@ logging_level 6
path_trace_enabled 0
follow_up_info 0
tx_timestamp_retries 2
-rx_timestamp_l2only 0
use_syslog 1
verbose 0
#
diff --git a/gPTP.cfg b/gPTP.cfg
index 07cc2c2..84ac1c0 100644
--- a/gPTP.cfg
+++ b/gPTP.cfg
@@ -27,7 +27,6 @@ logging_level 6
path_trace_enabled 1
follow_up_info 1
tx_timestamp_retries 2
-rx_timestamp_l2only 1
use_syslog 1
verbose 0
#
diff --git a/ptp4l.c b/ptp4l.c
index da82774..bc2bc24 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -68,7 +68,6 @@ static struct config cfg_settings = {

.assume_two_step = &assume_two_step,
.tx_timestamp_retries = &sk_tx_retries,
- .rx_timestamp_l2only = &sk_prefer_layer2,

.clock_servo = CLOCK_SERVO_PI,

diff --git a/sk.c b/sk.c
index 8c9b9fa..2fa9ac0 100644
--- a/sk.c
+++ b/sk.c
@@ -32,7 +32,7 @@

/* globals */

-int sk_tx_retries = 2, sk_prefer_layer2 = 0;
+int sk_tx_retries = 2;

/* private methods */

diff --git a/sk.h b/sk.h
index b6ce88c..671e839 100644
--- a/sk.h
+++ b/sk.h
@@ -76,10 +76,4 @@ int sk_timestamping_init(int fd, char *device, enum timestamp_type type,
*/
extern int sk_tx_retries;

-/**
- * Request the hardware receive filter that is limited to layer 2 time
- * stamping.
- */
-extern int sk_prefer_layer2;
-
#endif