[Linuxptp-devel] [PATCH 0/3] Improve ioctl to request hardware features

[Flavio Leitner]

Hello,

Currently the user space can't tell which delay mechanism (E2E/P2P) or
transport is needed in the ioctl(). Therefore, PTP silently fails when the
hardware doesn't support a certain delay mechanism or transport.

I am presenting three patches to improve this situation. One modifies linuxptp
to use the ioctl flags field (16 bits) to pass that information from the user
space to kernel. If the hardware supports all the desired features, the ioctl
continues as before, otherwise the unsupported bits are reseted to 0 and this
information is returned to user space which can tell exactly what has failed.

The second patch is the generic part at the kernel side, based on net-next
tree.

And the last patch is the driver specifics. In this case, I chose e1000e
because it is the hardware I am using here. I am intentionally limiting the
card capabilities for testing purposes and to be an example of how the feature
bits would work. I will re-post the correct patch providing all supported
features once the basic idea proves to be acceptable.

It's backwards compatible. If an older PTP application calls the ioctl(), the
flags field will be zero and no feature is checked. Also, if an older kernel is
used with a patched linuxptp, then the failure will cause a fallback to the old
way (zeroed flags, no feedback).

Feedbacks? Thoughts?

Thanks in advance,
fbl

Flavio Leitner (3):
linuxptp: Use flags field to require HW features.
PTP: use flags to request HW features
e1000e: PTP: provide hardware features

[Flavio Leitner]

Currently the user space can't tell which delay mechanism
(E2E/P2P) or transport is needed in the ioctl(). Therefore,
PTP silently fails when the hardware doesn't support a
certain delay mechanism or transport.

This patch uses the ioctl flags field to pass that information
from the user space to kernel. If the hardware supports all
the desired features, the ioctl continues as before, otherwise
the unsupported bits are reseted to 0 and this information is
returned to user space.

It's backwards compatible. If an older PTP applications calls
the ioctl(), the flags field will be zero and no feature is
checked.

Signed-off-by: Flavio Leitner <***@redhat.com>
---
include/uapi/linux/net_tstamp.h | 21 +++++++++++++++++++++
net/core/dev_ioctl.c | 3 ---
2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index ae5df12..d63f8e9 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -44,6 +44,27 @@ struct hwtstamp_config {
int rx_filter;
};

+/* possible values for hwtstamp_config->flags */
+#define HWTSTAMP_FEATURE_FLAGS_MASK 0xFFFF
+enum hwtstamp_feature_flags {
+ /* End-to-End Delay Mechanism */
+ HWTSTAMP_DM_E2E = (1<<0),
+ /* Peer-to-Peer Delay Mechanism */
+ HWTSTAMP_DM_P2P = (1<<1),
+ /* hole: 3 bits for additional mechanisms */
+
+ HWTSTAMP_TRANS_UDS = (1<<5),
+ /* Message Transport: UDP over IPv4 */
+ HWTSTAMP_TRANS_UDP_IPV4 = (1<<6),
+ /* Message Transport: UDP over IPv6 */
+ HWTSTAMP_TRANS_UDP_IPV6 = (1<<7),
+ /* Message Transport: IEEE 802.3 Ethernet */
+ HWTSTAMP_TRANS_IEEE_802_3 = (1<<8),
+ HWTSTAMP_TRANS_DEVICENET = (1<<9),
+ HWTSTAMP_TRANS_CONTROLNET = (1<<10),
+ HWTSTAMP_TRANS_PROFINET = (1<<11),
+};
+
/* possible values for hwtstamp_config->tx_type */
enum hwtstamp_tx_types {
/*
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 5b7d0e1..9e2407e 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -193,9 +193,6 @@ static int net_hwtstamp_validate(struct ifreq *ifr)
if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
return -EFAULT;

- if (cfg.flags) /* reserved for future extensions */
- return -EINVAL;
-
tx_type = cfg.tx_type;
rx_filter = cfg.rx_filter;

[Flavio Leitner]

Adding CC to netdev.

Post by Flavio Leitner
Currently the user space can't tell which delay mechanism
(E2E/P2P) or transport is needed in the ioctl(). Therefore,
PTP silently fails when the hardware doesn't support a
certain delay mechanism or transport.
This patch uses the ioctl flags field to pass that information
from the user space to kernel. If the hardware supports all
the desired features, the ioctl continues as before, otherwise
the unsupported bits are reseted to 0 and this information is
returned to user space.
It's backwards compatible. If an older PTP applications calls
the ioctl(), the flags field will be zero and no feature is
checked.
---
include/uapi/linux/net_tstamp.h | 21 +++++++++++++++++++++
net/core/dev_ioctl.c | 3 ---
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index ae5df12..d63f8e9 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -44,6 +44,27 @@ struct hwtstamp_config {
int rx_filter;
};
+/* possible values for hwtstamp_config->flags */
+#define HWTSTAMP_FEATURE_FLAGS_MASK 0xFFFF
+enum hwtstamp_feature_flags {
+ /* End-to-End Delay Mechanism */
+ HWTSTAMP_DM_E2E = (1<<0),
+ /* Peer-to-Peer Delay Mechanism */
+ HWTSTAMP_DM_P2P = (1<<1),
+ /* hole: 3 bits for additional mechanisms */
+
+ HWTSTAMP_TRANS_UDS = (1<<5),
+ /* Message Transport: UDP over IPv4 */
+ HWTSTAMP_TRANS_UDP_IPV4 = (1<<6),
+ /* Message Transport: UDP over IPv6 */
+ HWTSTAMP_TRANS_UDP_IPV6 = (1<<7),
+ /* Message Transport: IEEE 802.3 Ethernet */
+ HWTSTAMP_TRANS_IEEE_802_3 = (1<<8),
+ HWTSTAMP_TRANS_DEVICENET = (1<<9),
+ HWTSTAMP_TRANS_CONTROLNET = (1<<10),
+ HWTSTAMP_TRANS_PROFINET = (1<<11),
+};
+
/* possible values for hwtstamp_config->tx_type */
enum hwtstamp_tx_types {
/*
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 5b7d0e1..9e2407e 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -193,9 +193,6 @@ static int net_hwtstamp_validate(struct ifreq *ifr)
if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
return -EFAULT;
- if (cfg.flags) /* reserved for future extensions */
- return -EINVAL;
-
tx_type = cfg.tx_type;
rx_filter = cfg.rx_filter;
--
1.8.3.1

[Richard Cochran]

Post by Flavio Leitner

Post by Flavio Leitner
Currently the user space can't tell which delay mechanism
(E2E/P2P) or transport is needed in the ioctl(). Therefore,
PTP silently fails when the hardware doesn't support a
certain delay mechanism or transport.

[ For netdev readers, the text below repeats what I posted to the
linuxptp-devel list. ]

SIOCSHWTSTAMP is bad enough already. Let's not make it even
uglier. ETHTOOL is a better place for this kind of thing. The
ethtool_ts_info has plenty of reserved fields, and drivers could
use them to advertise these capabilities without the risk of
breaking user space.

We already check each port's capabilities using the ethtool ioctl.
It makes perfect sense to have two calls. First, tell me what is
available, then, I'll tell you what I want. That is easy.

In contrast, the closed loop feedback of SIOCSHWTSTAMP is really
awful. The whole rx_filter field is designed around the inadequacies
of an early intel card, and the choices are obsolete and mostly
useless. There isn't even any mention of pdelay. Ptp4l is already
bad enough with the filter1, filter2 probing. Let's just leave that
ioctl alone.

Post by Flavio Leitner
What about the transport?

I suggest offering the following bits in the ethtool rx|tx_reserved
fields:

TS_SYNC (1<<0) /* silly, PTP is not possible without this */
TS_DELAY_REQ (1<<1)
TS_PDELAY_REQ (1<<2)
TS_PDELAY_RESP (1<<3)

TS_ON_LAYER2 (1<<4)
TS_ON_IPV4 (1<<5)
TS_ON_IPV6 (1<<6)

Post by Flavio Leitner
And feature combination?

My goal for linuxptp (and for the PHC subsystem) is to offer a kind of
reference implementation. I have no inclination to support every last
broken hardware out there. However, I agree that we should try to
support poorly designed cards if we can, but not at the price of
making ugly interfaces for the everyone.

Drivers with weird restrictions (for example, supports PDelay_Req but
only on IPv6) will have to just advertise one working combination.

The intel ixp465 is an example of design that we cannot ever support
in linuxptp. It can time stamp *either* the master role's events or
the slave role's events, but not both. Brilliant.

[ This chip is thankfully now obsolete, but the time stamping IP core
found its way into the phc_gbe, but with the worst mistakes
corrected. ]

Thanks,
Richard

[Flavio Leitner]

Post by Richard Cochran

Post by Flavio Leitner
Currently the user space can't tell which delay mechanism
(E2E/P2P) or transport is needed in the ioctl(). Therefore,
PTP silently fails when the hardware doesn't support a
certain delay mechanism or transport.

[ For netdev readers, the text below repeats what I posted to the
linuxptp-devel list. ]

I am reconsidering about extending ethtool instead, so I'd like to
drop these patches for now.
Thanks Richard and Jacob for the contributions,
fbl

[Flavio Leitner]

Currently the user space can't tell which delay mechanism
(E2E/P3P) or transport is needed in the ioctl(). Therefore,
PTP silently fails when the hardware doesn't support a
certain delay mechanism or transport.

This patch uses the ioctl flags field to pass that information
from the user space to kernel. If the hardware supports all
the desired features, the ioctl continues as before, otherwise
the unsupported bits are reseted to 0 and this information is
returned to user space.

It's backwards compatible. If an older ptp4l calls the ioctl(),
the flags field will be zero, so it should work as before.

It's forwards compatible. If the new ptp4l calls the ioctl(),
the old kernel will return -EINVAL and ptp4l will try again
without requiring any feature, as before the change.

Signed-off-by: Flavio Leitner <***@redhat.com>
---
dm.h | 2 ++
pmc_common.c | 2 +-
port.c | 6 +++--
raw.c | 7 ++---
sk.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++-----
sk.h | 1 +
transport.c | 6 +++--
transport.h | 4 ++-
transport_private.h | 2 +-
udp.c | 5 ++--
udp6.c | 5 ++--
uds.c | 3 ++-
12 files changed, 95 insertions(+), 22 deletions(-)

diff --git a/dm.h b/dm.h
index 2491c63..06a967a 100644
--- a/dm.h
+++ b/dm.h
@@ -24,6 +24,8 @@
* Defines the possible delay mechanisms.
*/
enum delay_mechanism {
+ /** None */
+ DM_NONE,

/** Start as E2E, but switch to P2P if a peer is detected. */
DM_AUTO,
diff --git a/pmc_common.c b/pmc_common.c
index ed3c5da..3c245b2 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -90,7 +90,7 @@ struct pmc *pmc_create(enum transport_type transport_type, char *iface_name,
goto failed;
}
if (transport_open(pmc->transport, iface_name,
- &pmc->fdarray, TS_SOFTWARE)) {
+ &pmc->fdarray, DM_NONE, TS_SOFTWARE)) {
pr_err("failed to open transport");
goto failed;
}
diff --git a/port.c b/port.c
index bd10d0a..868f77c 100644
--- a/port.c
+++ b/port.c
@@ -1316,7 +1316,8 @@ static int port_initialize(struct port *p)
goto no_timers;
}
}
- if (transport_open(p->trp, p->name, &p->fda, p->timestamping))
+ if (transport_open(p->trp, p->name, &p->fda, p->timestamping,
+ p->delayMechanism))
goto no_tropen;

for (i = 0; i < N_TIMER_FDS; i++) {
@@ -1349,7 +1350,8 @@ static int port_renew_transport(struct port *p)
}
clock_remove_fda(p->clock, p, p->fda);
transport_close(p->trp, &p->fda);
- if (transport_open(p->trp, p->name, &p->fda, p->timestamping)) {
+ if (transport_open(p->trp, p->name, &p->fda, p->timestamping,
+ p->delayMechanism)) {
return -1;
}
clock_install_fda(p->clock, p, p->fda);
diff --git a/raw.c b/raw.c
index d87edbf..957a351 100644
--- a/raw.c
+++ b/raw.c
@@ -42,6 +42,7 @@
#include "raw.h"
#include "sk.h"
#include "transport_private.h"
+#include "dm.h"

struct raw {
struct transport t;
@@ -184,8 +185,8 @@ no_socket:
return -1;
}

-static int raw_open(struct transport *t, char *name,
- struct fdarray *fda, enum timestamp_type ts_type)
+static int raw_open(struct transport *t, char *name, struct fdarray *fda,
+ enum timestamp_type ts_type, enum delay_mechanism dm)
{
struct raw *raw = container_of(t, struct raw, t);
int efd, gfd;
@@ -206,7 +207,7 @@ static int raw_open(struct transport *t, char *name,
if (gfd < 0)
goto no_general;

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

if (sk_general_init(gfd))
diff --git a/sk.c b/sk.c
index fe74164..d1a878a 100644
--- a/sk.c
+++ b/sk.c
@@ -32,6 +32,7 @@

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

/* globals */

@@ -40,10 +41,13 @@ int sk_check_fupsync;

/* private methods */

-static int hwts_init(int fd, char *device, int rx_filter, int one_step)
+static int hwts_init(int fd, char *device, int hwflags, int rx_filter,
+ int one_step)
{
struct ifreq ifreq;
struct hwtstamp_config cfg, req;
+ int unsupported_flags;
+ int check_hwflags = 1;
int err;

memset(&ifreq, 0, sizeof(ifreq));
@@ -52,16 +56,52 @@ static int hwts_init(int fd, char *device, int rx_filter, int one_step)
strncpy(ifreq.ifr_name, device, sizeof(ifreq.ifr_name));

ifreq.ifr_data = (void *) &cfg;
+ cfg.flags = hwflags & HWTSTAMP_FEATURE_FLAGS_MASK;
cfg.tx_type = one_step ? HWTSTAMP_TX_ONESTEP_SYNC : HWTSTAMP_TX_ON;
cfg.rx_filter = rx_filter;
req = cfg;
err = ioctl(fd, SIOCSHWTSTAMP, &ifreq);
- if (err < 0)
- return err;
+ if (err < 0) {
+ /* older linux kernels required flags to be zero
+ * disable hwflags checking */
+ if (errno == EINVAL) {
+ check_hwflags = 0;
+ req.flags = cfg.flags = 0;
+ err = ioctl(fd, SIOCSHWTSTAMP, &ifreq);
+ if (err < 0)
+ return err;
+ }
+ else
+ return err;
+ }

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

pr_warning("driver changed our HWTSTAMP options");
+ unsupported_flags = cfg.flags ^ req.flags;
+ unsupported_flags &= HWTSTAMP_FEATURE_FLAGS_MASK;
+ if (check_hwflags && unsupported_flags) {
+ if (unsupported_flags & HWTSTAMP_DM_E2E)
+ pr_warning("End-to-End is not supported");
+ if (unsupported_flags & HWTSTAMP_DM_P2P)
+ pr_warning("Peer-to-Peer is not supported");
+ if (unsupported_flags & HWTSTAMP_TRANS_UDS)
+ pr_warning("UDS is not supported");
+ if (unsupported_flags & HWTSTAMP_TRANS_UDP_IPV4)
+ pr_warning("UDP over IPv4 is not supported");
+ if (unsupported_flags & HWTSTAMP_TRANS_UDP_IPV6)
+ pr_warning("UDP over IPv6 is not supported");
+ if (unsupported_flags & HWTSTAMP_TRANS_IEEE_802_3)
+ pr_warning("IEEE 802.3 is not supported");
+ if (unsupported_flags & HWTSTAMP_TRANS_DEVICENET)
+ pr_warning("DeviceNet is not supported");
+ if (unsupported_flags & HWTSTAMP_TRANS_CONTROLNET)
+ pr_warning("End-to-End is not supported");
+ if (unsupported_flags & HWTSTAMP_TRANS_PROFINET)
+ pr_warning("End-to-End is not supported");
+ return -1;
+ }
+ pr_warning("flags %d not %d", cfg.flags, req.flags);
pr_warning("tx_type %d not %d", cfg.tx_type, req.tx_type);
pr_warning("rx_filter %d not %d", cfg.rx_filter, req.rx_filter);

@@ -284,9 +324,9 @@ 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)
+ enum delay_mechanism dm, enum transport_type transport)
{
- int err, filter1, filter2 = 0, flags, one_step;
+ int err, filter1, filter2 = 0, flags, one_step, hwflags = 0;

switch (type) {
case TS_SOFTWARE:
@@ -312,12 +352,32 @@ int sk_timestamping_init(int fd, char *device, enum timestamp_type type,
if (type != TS_SOFTWARE) {
filter1 = HWTSTAMP_FILTER_PTP_V2_EVENT;
one_step = type == TS_ONESTEP ? 1 : 0;
+ switch (dm) {
+ case DM_AUTO:
+ hwflags |= (HWTSTAMP_DM_P2P | HWTSTAMP_DM_E2E);
+ break;
+ case DM_E2E:
+ hwflags |= HWTSTAMP_DM_E2E;
+ break;
+ case DM_P2P:
+ hwflags |= HWTSTAMP_DM_P2P;
+ case DM_NONE:
+ break;
+ default:
+ return -1;
+ }
+
switch (transport) {
case TRANS_UDP_IPV4:
+ hwflags |= HWTSTAMP_TRANS_UDP_IPV4;
+ filter2 = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;
+ break;
case TRANS_UDP_IPV6:
+ hwflags |= HWTSTAMP_TRANS_UDP_IPV6;
filter2 = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;
break;
case TRANS_IEEE_802_3:
+ hwflags |= HWTSTAMP_TRANS_IEEE_802_3;
filter2 = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
break;
case TRANS_DEVICENET:
@@ -326,10 +386,10 @@ int sk_timestamping_init(int fd, char *device, enum timestamp_type type,
case TRANS_UDS:
return -1;
}
- err = hwts_init(fd, device, filter1, one_step);
+ err = hwts_init(fd, device, hwflags, filter1, one_step);
if (err) {
pr_info("driver rejected most general HWTSTAMP filter");
- err = hwts_init(fd, device, filter2, one_step);
+ err = hwts_init(fd, device, hwflags, filter2, one_step);
if (err) {
pr_err("ioctl SIOCSHWTSTAMP failed: %m");
return err;
diff --git a/sk.h b/sk.h
index 895840f..02a5326 100644
--- a/sk.h
+++ b/sk.h
@@ -101,6 +101,7 @@ int sk_receive(int fd, void *buf, int buflen,
* @return Zero on success, non-zero otherwise.
*/
int sk_timestamping_init(int fd, char *device, enum timestamp_type type,
+ enum delay_mechanism dm,
enum transport_type transport);

/**
diff --git a/transport.c b/transport.c
index 3c70b2b..52d89bc 100644
--- a/transport.c
+++ b/transport.c
@@ -23,6 +23,7 @@
#include "udp.h"
#include "udp6.h"
#include "uds.h"
+#include "dm.h"

int transport_close(struct transport *t, struct fdarray *fda)
{
@@ -30,9 +31,10 @@ int transport_close(struct transport *t, struct fdarray *fda)
}

int transport_open(struct transport *t, char *name,
- struct fdarray *fda, enum timestamp_type tt)
+ struct fdarray *fda, enum timestamp_type tt,
+ enum delay_mechanism dm)
{
- return t->open(t, name, fda, tt);
+ return t->open(t, name, fda, tt, dm);
}

int transport_recv(struct transport *t, int fd,
diff --git a/transport.h b/transport.h
index 34934fe..7e04c0c 100644
--- a/transport.h
+++ b/transport.h
@@ -60,12 +60,14 @@ struct hw_timestamp {
struct timespec sw;
};

+enum delay_mechanism;
struct transport;

int transport_close(struct transport *t, struct fdarray *fda);

int transport_open(struct transport *t, char *name,
- struct fdarray *fda, enum timestamp_type tt);
+ struct fdarray *fda, enum timestamp_type tt,
+ enum delay_mechanism dm);

int transport_recv(struct transport *t, int fd,
void *buf, int buflen, struct hw_timestamp *hwts);
diff --git a/transport_private.h b/transport_private.h
index 0c553e1..9f133cd 100644
--- a/transport_private.h
+++ b/transport_private.h
@@ -31,7 +31,7 @@ struct transport {
int (*close)(struct transport *t, struct fdarray *fda);

int (*open)(struct transport *t, char *name, struct fdarray *fda,
- enum timestamp_type tt);
+ enum timestamp_type tt, enum delay_mechanism dm);

int (*recv)(struct transport *t, int fd, void *buf, int buflen,
struct hw_timestamp *hwts);
diff --git a/udp.c b/udp.c
index be7f2b7..7b402a8 100644
--- a/udp.c
+++ b/udp.c
@@ -34,6 +34,7 @@
#include "sk.h"
#include "ether.h"
#include "transport_private.h"
+#include "dm.h"
#include "udp.h"

#define EVENT_PORT 319
@@ -149,7 +150,7 @@ enum { MC_PRIMARY, MC_PDELAY };
static struct in_addr mcast_addr[2];

static int udp_open(struct transport *t, char *name, struct fdarray *fda,
- enum timestamp_type ts_type)
+ enum timestamp_type ts_type, enum delay_mechanism dm)
{
struct udp *udp = container_of(t, struct udp, t);
int efd, gfd;
@@ -176,7 +177,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, TRANS_UDP_IPV4))
+ if (sk_timestamping_init(efd, name, ts_type, dm, TRANS_UDP_IPV4))
goto no_timestamping;

if (sk_general_init(gfd))
diff --git a/udp6.c b/udp6.c
index e0d1256..8dbeaa2 100644
--- a/udp6.c
+++ b/udp6.c
@@ -34,6 +34,7 @@
#include "sk.h"
#include "ether.h"
#include "transport_private.h"
+#include "dm.h"
#include "udp6.h"

#define EVENT_PORT 319
@@ -159,7 +160,7 @@ enum { MC_PRIMARY, MC_PDELAY };
static struct in6_addr mc6_addr[2];

static int udp6_open(struct transport *t, char *name, struct fdarray *fda,
- enum timestamp_type ts_type)
+ enum timestamp_type ts_type, enum delay_mechanism dm)
{
struct udp6 *udp6 = container_of(t, struct udp6, t);
int efd, gfd;
@@ -188,7 +189,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, TRANS_UDP_IPV6))
+ if (sk_timestamping_init(efd, name, ts_type, dm, TRANS_UDP_IPV6))
goto no_timestamping;

if (sk_general_init(gfd))
diff --git a/uds.c b/uds.c
index 1dbfd6c..17bacee 100644
--- a/uds.c
+++ b/uds.c
@@ -28,6 +28,7 @@
#include "contain.h"
#include "print.h"
#include "transport_private.h"
+#include "dm.h"
#include "uds.h"

char uds_path[MAX_IFNAME_SIZE + 1] = "/var/run/ptp4l";
@@ -47,7 +48,7 @@ static int uds_close(struct transport *t, struct fdarray *fda)
}

static int uds_open(struct transport *t, char *name, struct fdarray *fda,
- enum timestamp_type tt)
+ enum timestamp_type tt, enum delay_mechanism dm)
{
int fd, err;
struct sockaddr_un sa;

[Flavio Leitner]

Provide the modes and transports supported by the hardware.

Signed-off-by: Flavio Leitner <***@redhat.com>
---
drivers/net/ethernet/intel/e1000e/netdev.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 4ef7867..8bcf167 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -3498,10 +3498,6 @@ static int e1000e_config_hwtstamp(struct e1000_adapter *adapter)
if (!(adapter->flags & FLAG_HAS_HW_TIMESTAMP))
return -EINVAL;

- /* flags reserved for future extensions - must be zero */
- if (config->flags)
- return -EINVAL;
-
switch (config->tx_type) {
case HWTSTAMP_TX_OFF:
tsync_tx_ctl = 0;
@@ -5772,6 +5768,9 @@ static int e1000_mii_ioctl(struct net_device *netdev, struct ifreq *ifr,
return 0;
}

+#define E1000E_HWSTAMP_FLAGS_MASK (HWTSTAMP_DM_E2E | HWTSTAMP_DM_P2P |\
+ HWTSTAMP_TRANS_IEEE_802_3 |\
+ HWTSTAMP_TRANS_UDP_IPV4)
/**
* e1000e_hwtstamp_ioctl - control hardware time stamping
* @netdev: network interface device structure
@@ -5792,11 +5791,23 @@ static int e1000e_hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr)
{
struct e1000_adapter *adapter = netdev_priv(netdev);
struct hwtstamp_config config;
+ int flags_req;
+ int flags_unsup;
int ret_val;

if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
return -EFAULT;

+ if (config.flags & ~HWTSTAMP_FEATURE_FLAGS_MASK)
+ return -EINVAL;
+
+ flags_req = config.flags & HWTSTAMP_FEATURE_FLAGS_MASK;
+ flags_unsup = flags_req & ~E1000E_HWSTAMP_FLAGS_MASK;
+ if (flags_unsup) {
+ config.flags &= ~flags_unsup;
+ goto out;
+ }
+
adapter->hwtstamp_config = config;

ret_val = e1000e_config_hwtstamp(adapter);
@@ -5823,6 +5834,7 @@ static int e1000e_hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr)
break;
}

+out:
return copy_to_user(ifr->ifr_data, &config,
sizeof(config)) ? -EFAULT : 0;
}

[Flavio Leitner]

Adding CC to netdev.

The first patch [1/3] is the userlevel linuxptp patch, so
I am not forwarding to netdev.

This is just an example which I used for testing locally. If
the idea is acceptable, this patch must be replaced since the
card does support more features than what is listed below.

fbl

Post by Flavio Leitner
Provide the modes and transports supported by the hardware.
---
drivers/net/ethernet/intel/e1000e/netdev.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 4ef7867..8bcf167 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -3498,10 +3498,6 @@ static int e1000e_config_hwtstamp(struct e1000_adapter *adapter)
if (!(adapter->flags & FLAG_HAS_HW_TIMESTAMP))
return -EINVAL;
- /* flags reserved for future extensions - must be zero */
- if (config->flags)
- return -EINVAL;
-
switch (config->tx_type) {
tsync_tx_ctl = 0;
@@ -5772,6 +5768,9 @@ static int e1000_mii_ioctl(struct net_device *netdev, struct ifreq *ifr,
return 0;
}
+#define E1000E_HWSTAMP_FLAGS_MASK (HWTSTAMP_DM_E2E | HWTSTAMP_DM_P2P |\
+ HWTSTAMP_TRANS_IEEE_802_3 |\
+ HWTSTAMP_TRANS_UDP_IPV4)
/**
* e1000e_hwtstamp_ioctl - control hardware time stamping
@@ -5792,11 +5791,23 @@ static int e1000e_hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr)
{
struct e1000_adapter *adapter = netdev_priv(netdev);
struct hwtstamp_config config;
+ int flags_req;
+ int flags_unsup;
int ret_val;
if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
return -EFAULT;
+ if (config.flags & ~HWTSTAMP_FEATURE_FLAGS_MASK)
+ return -EINVAL;
+
+ flags_req = config.flags & HWTSTAMP_FEATURE_FLAGS_MASK;
+ flags_unsup = flags_req & ~E1000E_HWSTAMP_FLAGS_MASK;
+ if (flags_unsup) {
+ config.flags &= ~flags_unsup;
+ goto out;
+ }
+
adapter->hwtstamp_config = config;
ret_val = e1000e_config_hwtstamp(adapter);
@@ -5823,6 +5834,7 @@ static int e1000e_hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr)
break;
}
return copy_to_user(ifr->ifr_data, &config,
sizeof(config)) ? -EFAULT : 0;
}
--
1.8.3.1

[Richard Cochran]

Post by Flavio Leitner
Feedbacks? Thoughts?

1. Although I can understand why you might want to change this, I am
skeptical about the practical value of adding this check. Ethernet
hardware that supports 1588 V2 has to work in both E2E and P2P and
with L2, IPv4, and IPv6. Otherwise, it really doesn't support the
standard! Most of the newer cards do implement everything
correctly.

2. By trying different values of rx_filter, you can, in theory,
discover the card's capabilities WRT L2 verses IPv4/6. This check
is already implemented in ptp4l.

Unfortunately, you can't select between IPv4 and IPv6, but that is
how the interface was defined (not by me). IIRC, the Solarflare
card only supports IPv4, and so the user just has to know not to
try IPv6.

3. SIOCSHWTSTAMP is bad enough already. Let's not make it even
uglier. ETHTOOL is a better place for this kind of thing. The
ethtool_ts_info has plenty of reserved fields, and drivers could
use them to advertise these capabilities without the risk of
breaking user space.

4. You should really post just the kernel bits to netdev for
discussion.

Thanks,
Richard

[Flavio Leitner]

Post by Richard Cochran

Post by Flavio Leitner
Feedbacks? Thoughts?

1. Although I can understand why you might want to change this, I am
skeptical about the practical value of adding this check. Ethernet
hardware that supports 1588 V2 has to work in both E2E and P2P and
with L2, IPv4, and IPv6. Otherwise, it really doesn't support the
standard! Most of the newer cards do implement everything
correctly.

Yes, but that's not what I am seeing here. I have one card (sfc) that
doesn't support IPv6 and it is documented in the kernel sources, and
another (pch_gbe) that doesn't support P2P.

Post by Richard Cochran
2. By trying different values of rx_filter, you can, in theory,
discover the card's capabilities WRT L2 verses IPv4/6. This check
is already implemented in ptp4l.
Unfortunately, you can't select between IPv4 and IPv6, but that is
how the interface was defined (not by me). IIRC, the Solarflare
card only supports IPv4, and so the user just has to know not to
try IPv6.

That's the problem I am trying to solve. It's way better if the
application could tell you that the hardware doesn't support what
you are trying to do.

Post by Richard Cochran
3. SIOCSHWTSTAMP is bad enough already. Let's not make it even
uglier. ETHTOOL is a better place for this kind of thing. The
ethtool_ts_info has plenty of reserved fields, and drivers could
use them to advertise these capabilities without the risk of
breaking user space.

I thought about using that. However, you have to make two completely
different calls. One for checking the hardware capabilities (ETHTOOL)
and another to actually enable them (SIOCSHWTSTAMP).

Post by Richard Cochran
4. You should really post just the kernel bits to netdev for
discussion.

Indeed. I just wanted to hear the feedbacks here first.

Thanks!
fbl

[Richard Cochran]

Post by Flavio Leitner

Post by Richard Cochran
3. SIOCSHWTSTAMP is bad enough already. Let's not make it even
uglier. ETHTOOL is a better place for this kind of thing. The
ethtool_ts_info has plenty of reserved fields, and drivers could
use them to advertise these capabilities without the risk of
breaking user space.

I thought about using that. However, you have to make two completely
different calls. One for checking the hardware capabilities (ETHTOOL)
and another to actually enable them (SIOCSHWTSTAMP).

We already check each port's capabilities using the ethtool ioctl.
It makes perfect sense to have two calls. First, tell me what is
available, then, I'll tell you what I want. That is easy.

In contrast, the closed loop feedback of SIOCSHWTSTAMP is really
awful. The whole rx_filter field is designed around the inadequacies
of an early intel card, and the choices are obsolete and mostly
useless. There isn't even any mention of pdelay. Ptp4l is already
bad enough with the filter1, filter2 probing. Let's just leave that
ioctl alone.

Thanks,
Richard

[Flavio Leitner]

Post by Richard Cochran

Post by Flavio Leitner

Post by Richard Cochran
3. SIOCSHWTSTAMP is bad enough already. Let's not make it even
uglier. ETHTOOL is a better place for this kind of thing. The
ethtool_ts_info has plenty of reserved fields, and drivers could
use them to advertise these capabilities without the risk of
breaking user space.

I thought about using that. However, you have to make two completely
different calls. One for checking the hardware capabilities (ETHTOOL)
and another to actually enable them (SIOCSHWTSTAMP).

We already check each port's capabilities using the ethtool ioctl.
It makes perfect sense to have two calls. First, tell me what is
available, then, I'll tell you what I want. That is easy.

But what if a combination of such features are not possible?
See below.

Post by Richard Cochran
In contrast, the closed loop feedback of SIOCSHWTSTAMP is really
awful. The whole rx_filter field is designed around the inadequacies
of an early intel card, and the choices are obsolete and mostly
useless. There isn't even any mention of pdelay. Ptp4l is already
bad enough with the filter1, filter2 probing. Let's just leave that
ioctl alone.

The difference is that with two calls, the application is responsible
for checking if the feature combination is possible, which is not the
ideal. On the other hand with a single call, the driver can check if
the combination requested is possible or not and return properly.

fbl

[Richard Cochran]

Post by Flavio Leitner
Yes, but that's not what I am seeing here. I have one card (sfc) that
doesn't support IPv6 and it is documented in the kernel sources, and
another (pch_gbe) that doesn't support P2P.

BTW, pch_gbe does support P2P, but it only supports using one L2
multicast address. Using the same address for all event types is
permissible according to 1588. If you configure ptp4l like this

ptp_dst_mac 01:1B:19:00:00:00
p2p_dst_mac 01:1B:19:00:00:00

then it should work. But of course, I don't have this HW to test.

HTH,
Richard

[Keller, Jacob E]

Post by Richard Cochran

Post by Flavio Leitner
Feedbacks? Thoughts?

1. Although I can understand why you might want to change this, I am
skeptical about the practical value of adding this check. Ethernet
hardware that supports 1588 V2 has to work in both E2E and P2P and
with L2, IPv4, and IPv6. Otherwise, it really doesn't support the
standard! Most of the newer cards do implement everything
correctly.

I agree. If it supports V2, it has to do P2P and E2E. If it can't do
both, it doesn't support the standard. In addition, extending the
SIOCHWTSTAMP ioctl is very dangerous, because not all apps use it the
same way. We aren't the only PTP application, and should be careful
here.

Post by Richard Cochran
2. By trying different values of rx_filter, you can, in theory,
discover the card's capabilities WRT L2 verses IPv4/6. This check
is already implemented in ptp4l.

Correct, we can already check this, no need to extend the interface.

Post by Richard Cochran
Unfortunately, you can't select between IPv4 and IPv6, but that is
how the interface was defined (not by me). IIRC, the Solarflare
card only supports IPv4, and so the user just has to know not to
try IPv6.
3. SIOCSHWTSTAMP is bad enough already. Let's not make it even
uglier. ETHTOOL is a better place for this kind of thing. The
ethtool_ts_info has plenty of reserved fields, and drivers could
use them to advertise these capabilities without the risk of
breaking user space.

I agree, I would much rather extend the ethtool get_ts_info operation
with this information than to use the SIOCHWTSTAMP. This interface is
much better, easier to use, and easier to modify.

Post by Richard Cochran
4. You should really post just the kernel bits to netdev for
discussion.

Agreed.


I would much rather see an extension of the ethtool command that shows
what would be supported rather than trying to extend the HWTSTAMP i

[Flavio Leitner]

Post by Keller, Jacob E

Post by Richard Cochran

Post by Flavio Leitner
Feedbacks? Thoughts?

1. Although I can understand why you might want to change this, I am
skeptical about the practical value of adding this check. Ethernet
hardware that supports 1588 V2 has to work in both E2E and P2P and
with L2, IPv4, and IPv6. Otherwise, it really doesn't support the
standard! Most of the newer cards do implement everything
correctly.

I agree. If it supports V2, it has to do P2P and E2E. If it can't do
both, it doesn't support the standard. In addition, extending the

Indeed, but as I said in the other email, not all hardware out there
are fully compliant. What you suggest to this case?

Post by Keller, Jacob E
SIOCHWTSTAMP ioctl is very dangerous, because not all apps use it the
same way. We aren't the only PTP application, and should be careful
here.

Today the kernel blocks and return an error if flags is not zero, so
up to today, every PTP application must be passing zero, which after
this patch doesn't change anything.

Post by Keller, Jacob E

Post by Richard Cochran
2. By trying different values of rx_filter, you can, in theory,
discover the card's capabilities WRT L2 verses IPv4/6. This check
is already implemented in ptp4l.

Correct, we can already check this, no need to extend the interface.

What about the transport? And feature combination?

Post by Keller, Jacob E

Post by Richard Cochran
Unfortunately, you can't select between IPv4 and IPv6, but that is
how the interface was defined (not by me). IIRC, the Solarflare
card only supports IPv4, and so the user just has to know not to
try IPv6.
3. SIOCSHWTSTAMP is bad enough already. Let's not make it even
uglier. ETHTOOL is a better place for this kind of thing. The
ethtool_ts_info has plenty of reserved fields, and drivers could
use them to advertise these capabilities without the risk of
breaking user space.

I agree, I would much rather extend the ethtool get_ts_info operation
with this information than to use the SIOCHWTSTAMP. This interface is
much better, easier to use, and easier to modify.

See my other reply about having one or two calls.

fbl

[Richard Cochran]

Post by Flavio Leitner
What about the transport?

I suggest offering the following bits in the ethtool rx|tx_reserved
fields:

TS_SYNC (1<<0) /* silly, PTP is not possible without this */
TS_DELAY_REQ (1<<1)
TS_PDELAY_REQ (1<<2)
TS_PDELAY_RESP (1<<3)

TS_ON_LAYER2 (1<<4)
TS_ON_IPV4 (1<<5)
TS_ON_IPV6 (1<<6)

Post by Flavio Leitner
And feature combination?

My goal for linuxptp (and for the PHC subsystem) is to offer a kind of
reference implementation. I have no inclination to support every last
broken hardware out there. However, I agree that we should try to
support poorly designed cards if we can, but not at the price of
making ugly interfaces for the everyone.

Drivers with weird restrictions (for example, supports PDelay_Req but
only on IPv6) will have to just advertise one working combination.

The intel ixp465 is an example of design that we cannot ever
support. It can time stamp *either* the master role's events or the
slave role's events, but not both. Brilliant.

[ This chip is thankfully now obsolete, but the time stamping IP core
found its way into the phc_gbe, but with the worst mistakes
corrected. ]

Thanks,
Richard

[Keller, Jacob E]

Post by Richard Cochran

Post by Flavio Leitner
What about the transport?

I suggest offering the following bits in the ethtool rx|tx_reserved
TS_SYNC (1<<0) /* silly, PTP is not possible without this */
TS_DELAY_REQ (1<<1)
TS_PDELAY_REQ (1<<2)
TS_PDELAY_RESP (1<<3)
TS_ON_LAYER2 (1<<4)
TS_ON_IPV4 (1<<5)
TS_ON_IPV6 (1<<6)

Post by Flavio Leitner
And feature combination?

My goal for linuxptp (and for the PHC subsystem) is to offer a kind of
reference implementation. I have no inclination to support every last
broken hardware out there. However, I agree that we should try to
support poorly designed cards if we can, but not at the price of
making ugly interfaces for the everyone.
Drivers with weird restrictions (for example, supports PDelay_Req but
only on IPv6) will have to just advertise one working combination.
The intel ixp465 is an example of design that we cannot ever
support. It can time stamp *either* the master role's events or the
slave role's events, but not both. Brilliant.
[ This chip is thankfully now obsolete, but the time stamping IP core
found its way into the phc_gbe, but with the worst mistakes
corrected. ]
Thanks,
Richard

Yep, I wasn't around when the design for that part happened.. But
basically they assumed the PTP would all be inside the driver,
(foolishly). Because of this it was just assumed "oh, you can just swap
the registers". And the interfaces also just assumed that you would call
them again on swapping from slave to master... Horrid.

But a lot of that design happened before anyone here at Intel actually
had any idea how PTP really would work in practice. I am pretty sure it
was just a "lets tick off a checkbox" sort of thing.

But I agree, there really isn't any reason for us to attempt to support
*every* broken card, because most of them will be broken in odd ways. If
their driver can't work around it, then it really isn't our problem that
someone made a card that doesn't support PTP. However, I also think if
we can do it without a ton of trouble that is good.

I think it should be documented that "supports" in this case means "I
support these packet types over every single layer listed below".
Anything supported over only a subset of the listed layers would have to
be removed, or any layers which don't support the entire list of packets
would have to be removed.

[Richard Cochran]

Post by Keller, Jacob E
But a lot of that design happened before anyone here at Intel actually
had any idea how PTP really would work in practice. I am pretty sure it
was just a "lets tick off a checkbox" sort of thing.

Just for the record, I didn't mean to single out Intel for having bad
cards. The ixp465 was one of the first PTP hardware products, and it
had issues, but it has been out of production for years now.

The Intel cards keep getting better and better, and the i210 (and
similar parts) is really quite good. Also, Jacob and his colleagues
are doing a terrific job supporting PTP on Intel chips.

Keep up the good work!

Thanks,
Richard

[Keller, Jacob E]

Post by Richard Cochran

Post by Keller, Jacob E
But a lot of that design happened before anyone here at Intel actually
had any idea how PTP really would work in practice. I am pretty sure it
was just a "lets tick off a checkbox" sort of thing.

Just for the record, I didn't mean to single out Intel for having bad
cards. The ixp465 was one of the first PTP hardware products, and it
had issues, but it has been out of production for years now.
The Intel cards keep getting better and better, and the i210 (and
similar parts) is really quite good. Also, Jacob and his colleagues
are doing a terrific job supporting PTP on Intel chips.
Keep up the good work!

Thanks! I understand :) I didn't mean to imply you were singling us out.
It's good to hear we're doing well. It definitely took a lot of work to
get here :) Especially also thanks to your work, because without the PTP
core, and the ptp4l project we'd still be stuck with pretty much all the
old broken interfaces.

Thanks,
Jake

Post by Richard Cochran
Than

[Flavio Leitner]

Post by Richard Cochran

Post by Flavio Leitner
And feature combination?

My goal for linuxptp (and for the PHC subsystem) is to offer a kind of
reference implementation. I have no inclination to support every last
broken hardware out there. However, I agree that we should try to
support poorly designed cards if we can, but not at the price of
making ugly interfaces for the everyone.

You made a good point. I can understand that it's not possible to
support all the hardware out there. However, I think it's unnecessary
difficult when someone has the hardware and it doesn't work because the
software is somehow limited. It does sound like a software bug and you
have to troubleshoot to say that it's a hardware fault when it actually
works on a specific scenario.

I still think that extending the ioctl() provides more flexibility, but
yeah, I agree that is not pretty.

Post by Richard Cochran
Drivers with weird restrictions (for example, supports PDelay_Req but
only on IPv6) will have to just advertise one working combination.

Fair enough. Let me think more about this for while.

Thanks Richard and Jacob for your thoughts.
Regards,
fbl

[Flavio Leitner]

Resurrecting this old thread.

Post by Richard Cochran

Post by Flavio Leitner
What about the transport?

I suggest offering the following bits in the ethtool rx|tx_reserved
TS_SYNC (1<<0) /* silly, PTP is not possible without this */
TS_DELAY_REQ (1<<1)
TS_PDELAY_REQ (1<<2)
TS_PDELAY_RESP (1<<3)
TS_ON_LAYER2 (1<<4)
TS_ON_IPV4 (1<<5)
TS_ON_IPV6 (1<<6)

Hi Richard, Jacob,

I am back thinking on this and maybe I misunderstood you.

In the sfc case that doesn't support TS over IPv6, with
your suggestion, it would be a matter of not providing that
TS_ON_IPv6 bit. That's fine.

However, pch_gbe supports E2E over L2, IPv4 and IPv6
but P2P only over L2 and with an unique multicast L2 address
configuration if I am not mistaken. With those defined bits,
there is no possible way to tell so.

So I thought in splitting the bits in sections according with the
transport, i.e.:

[<transport x>, <ipv6 bits>, <ipv4 bits>, <ethernet bits>]

It seems we have 6 transports defined so far.

Regarding to the message bits supported, I am not sure if we
have to go that deeper. Or otherwise we will have to list
follow up messages too and then there will be too many bits :-)
But since you mentioned them, maybe you know a case that I don't.

Perhaps the best would be to define E2E and P2P bits only and
enable them if they are specs compliant.

TS_E2E (1<<0) /* can do E2E */
TS_P2P (1<<1) /* can do P2P */
TS_RESERVED1 (1<<2)
TS_RESERVED2 (1<<3)

So, for instance, if the card supports P2P over IPv6, then
enable TS_P2P bit into the ipv6 transport section.

we keep space for one additional transport and two bits for
future features (32 bits).

How does that sound to you guys?

Thanks
fbl

[Richard Cochran]

Post by Flavio Leitner
In the sfc case that doesn't support TS over IPv6, with
your suggestion, it would be a matter of not providing that
TS_ON_IPv6 bit. That's fine.

Or even easier, just ask the vendor for a firmware upgrade for IPv6
support. IIRC, it is not a hardware limitation of that card, and it is
something the vendor wants to support eventually.

Post by Flavio Leitner
However, pch_gbe supports E2E over L2, IPv4 and IPv6
but P2P only over L2 and with an unique multicast L2 address
configuration if I am not mistaken. With those defined bits,
there is no possible way to tell so.
So I thought in splitting the bits in sections according with the
[<transport x>, <ipv6 bits>, <ipv4 bits>, <ethernet bits>]

But this also doesn't tell the pch_gbe user that he needs to use the
special L2 MC address, either.

Post by Flavio Leitner
It seems we have 6 transports defined so far.
Regarding to the message bits supported, I am not sure if we
have to go that deeper. Or otherwise we will have to list
follow up messages too and then there will be too many bits :-)

Follow-Up messages are not event messages, and so they don't need time
stamping.

Also, we should not have bits for E2E or P2P, because these terms
entail much more than simple hardware capabilities. They imply a whole
protocol, and all this is implemented in the user space PTP stack.

The driver level ioctls should only tell us what can be time stamped,
and nothing more.

...

Post by Flavio Leitner
we keep space for one additional transport and two bits for
future features (32 bits).

I really don't want to have a combinatorial API to support every last
broken hardware. I only suggested using 7 bits total, and letting
flakey hardware just set those bits which it fully supports.

Post by Flavio Leitner
How does that sound to you guys?

I really don't think it is worth the effort for these two corner
cases. As I wrote before, the pch_gbe IP core has a long, troubled
history, and you can be happy that it works at all. In fact, it works
just fine with a proper linuxptp configuration file. Regarding the
sfc, a firmware upgrade would be the ideal solution, but that is
obviously out of our hands.

Thanks,
Richard

[Flavio Leitner]

Post by Richard Cochran

Post by Flavio Leitner
In the sfc case that doesn't support TS over IPv6, with
your suggestion, it would be a matter of not providing that
TS_ON_IPv6 bit. That's fine.

Or even easier, just ask the vendor for a firmware upgrade for IPv6
support. IIRC, it is not a hardware limitation of that card, and it is
something the vendor wants to support eventually.

I have no idea if it's hardware or just firmware. My goal is
to have a way to tell if the hardware is capable or not.

Post by Richard Cochran

Post by Flavio Leitner
However, pch_gbe supports E2E over L2, IPv4 and IPv6
but P2P only over L2 and with an unique multicast L2 address
configuration if I am not mistaken. With those defined bits,
there is no possible way to tell so.
So I thought in splitting the bits in sections according with the
[<transport x>, <ipv6 bits>, <ipv4 bits>, <ethernet bits>]

But this also doesn't tell the pch_gbe user that he needs to use the
special L2 MC address, either.

That's true and I can't find a way to indicate so. Unless adding
known caveats to some readme file.

Post by Richard Cochran

Post by Flavio Leitner
It seems we have 6 transports defined so far.
Regarding to the message bits supported, I am not sure if we
have to go that deeper. Or otherwise we will have to list
follow up messages too and then there will be too many bits :-)

Follow-Up messages are not event messages, and so they don't need time
stamping.

Right, follow-up message just carries timestamps.

Post by Richard Cochran
Also, we should not have bits for E2E or P2P, because these terms
entail much more than simple hardware capabilities. They imply a whole
protocol, and all this is implemented in the user space PTP stack.

Well, that was pretty much the idea. If the hw supports all the event
messages related to that mode, then you can enable the bit.

Post by Richard Cochran
The driver level ioctls should only tell us what can be time stamped,
and nothing more.

I see your point.

Post by Richard Cochran

Post by Flavio Leitner
How does that sound to you guys?

I really don't think it is worth the effort for these two corner
cases. As I wrote before, the pch_gbe IP core has a long, troubled
history, and you can be happy that it works at all. In fact, it works
just fine with a proper linuxptp configuration file. Regarding the
sfc, a firmware upgrade would be the ideal solution, but that is
obviously out of our hands.

I presume from what you are saying that broken hardware isn't that
common and I might be just unlucky to get my hands on two.

Thanks,
fbl

[Richard Cochran]

Post by Flavio Leitner
I presume from what you are saying that broken hardware isn't that
common and I might be just unlucky to get my hands on two.

Yes, that is just what I wanted to say. All of the other cards for
which there are Linux drivers are unproblematic (except the ixp, but
let's not talk about that).

My feeling is that linuxptp and especially the Linux kernel have
become important PTP implementations, and that vendors are taking
notice. For example, Oregano Systems recently announced that they
added Linux PHC support to their PTP stack. I don't want to send the
wrong signal to hardware vendors that we are going to bend over
backwards to support half baked (= development cost cutting) designs.

Thanks,
Richard

[Keller, Jacob E]

Post by Richard Cochran

Post by Flavio Leitner
I presume from what you are saying that broken hardware isn't that
common and I might be just unlucky to get my hands on two.

Yes, that is just what I wanted to say. All of the other cards for
which there are Linux drivers are unproblematic (except the ixp, but
let's not talk about that).
My feeling is that linuxptp and especially the Linux kernel have
become important PTP implementations, and that vendors are taking
notice. For example, Oregano Systems recently announced that they
added Linux PHC support to their PTP stack. I don't want to send the
wrong signal to hardware vendors that we are going to bend over
backwards to support half baked (= development cost cutting) designs.
Thanks,
Richard

I agree. I'm glad to hear that other places are supporting the PHC
model. This is good for more interoperability.

Quite a few early hardware releases with "PTP" support are not really
good. Mostly, hardware vendors did not necessarily understand what would
be required, or think about how it would be implemented.
(Understandably, as not many had tried to implement it yet.)

I definitely don't want to send signals that we'll spend huge effort to
support broken