[Linuxptp-devel] [[RFC] PATCH 1/2] sk: change sk_interface_phc to sk_get_ts_info

[Jacob Keller]

this patch changes sk_interface_phc to sk_get_ts_info, by allowing the function
to store all the data returned by Ethtool's get_ts_info IOCTL in a struct. A new
struct "sk_ts_info" contains the same data as well as a field for specifying the
structure as valid (in order to support old kernels without the IOCTL). The
valid field should be set only when the IOCTL successfully populates the fields.

A follow-on patch will add new functionality possible because of these
changes. This patch only updates the programs which use the call to perform the
minimum they already do, using the new interface.

Signed-off-by: Jacob Keller <***@intel.com>
---
config.c | 2 ++
config.h | 2 ++
phc2sys.c | 6 +++---
port.c | 7 +++----
ptp4l.c | 9 ++++++---
sk.c | 25 +++++++++++++++++--------
sk.h | 21 +++++++++++++++++----
7 files changed, 50 insertions(+), 22 deletions(-)

diff --git a/config.c b/config.c
index e0d87d5..89ff9e7 100644
--- a/config.c
+++ b/config.c
@@ -394,6 +394,8 @@ int config_create_interface(char *name, struct config *cfg)
iface->transport = cfg->transport;
memcpy(&iface->pod, &cfg->pod, sizeof(cfg->pod));

+ sk_get_ts_info(name, &iface->ts_info);
+
cfg->nports++;

return i;
diff --git a/config.h b/config.h
index 7cffc32..b1f5fc6 100644
--- a/config.h
+++ b/config.h
@@ -24,6 +24,7 @@
#include "dm.h"
#include "transport.h"
#include "servo.h"
+#include "sk.h"

#define MAX_PORTS 8
#define MAX_IFNAME_SIZE 16
@@ -34,6 +35,7 @@ struct interface {
enum delay_mechanism dm;
enum transport_type transport;
struct port_defaults pod;
+ struct sk_ts_info ts_info;
};

#define CFG_IGNORE_DM (1 << 0)
diff --git a/phc2sys.c b/phc2sys.c
index fced164..a6586a2 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -298,13 +298,13 @@ int main(int argc, char *argv[])
}

if (src == CLOCK_INVALID && ethdev) {
- int phc_index = -1;
+ struct sk_ts_info ts_info;
char phc_device[16];
- if (sk_interface_phc(ethdev, &phc_index) || phc_index < 0) {
+ if (sk_get_ts_info(ethdev, &ts_info) || !ts_info.valid) {
fprintf(stderr, "can't autodiscover PHC device\n");
return -1;
}
- sprintf(phc_device, "/dev/ptp%d", phc_index);
+ sprintf(phc_device, "/dev/ptp%d", ts_info.phc_index);
src = clock_open(phc_device);
}
if (!(device || src != CLOCK_INVALID) || dst == CLOCK_INVALID) {
diff --git a/port.c b/port.c
index f2be7bc..7dcce0d 100644
--- a/port.c
+++ b/port.c
@@ -1663,7 +1663,6 @@ struct port *port_open(int phc_index,
struct clock *clock)
{
struct port *p = malloc(sizeof(*p));
- int checked_phc_index = -1;

if (!p)
return NULL;
@@ -1672,12 +1671,12 @@ struct port *port_open(int phc_index,

if (interface->transport == TRANS_UDS)
; /* UDS cannot have a PHC. */
- else if (sk_interface_phc(interface->name, &checked_phc_index))
+ else if (!interface->ts_info.valid)
pr_warning("port %d: get_ts_info not supported", number);
- else if (phc_index >= 0 && phc_index != checked_phc_index) {
+ else if (phc_index >= 0 && phc_index != interface->ts_info.phc_index) {
pr_err("port %d: PHC device mismatch", number);
pr_err("port %d: /dev/ptp%d requested, but /dev/ptp%d attached",
- number, phc_index, checked_phc_index);
+ number, phc_index, interface->ts_info.phc_index);
free(p);
return NULL;
}
diff --git a/ptp4l.c b/ptp4l.c
index bc2bc24..7064cc1 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -264,9 +264,12 @@ int main(int argc, char *argv[])
fprintf(stderr, "bad ptp device string\n");
return -1;
}
- } else if (sk_interface_phc(iface[0].name, &phc_index)) {
- fprintf(stderr, "get_ts_info not supported\n"
- "please specify ptp device\n");
+ } else if (iface[0].ts_info.valid) {
+ phc_index = iface[0].ts_info.phc_index;
+ } else {
+ fprintf(stderr, "ptp device not specified and\n"
+ "automatic determination is not\n"
+ "supported. please specify ptp device\n");
return -1;
}

diff --git a/sk.c b/sk.c
index b6ba8a5..46030d4 100644
--- a/sk.c
+++ b/sk.c
@@ -88,7 +88,7 @@ int sk_interface_index(int fd, char *name)
return ifreq.ifr_ifindex;
}

-int sk_interface_phc(char *name, int *index)
+int sk_get_ts_info(char *name, struct sk_ts_info *sk_info)
{
#ifdef ETHTOOL_GET_TS_INFO
struct ethtool_ts_info info;
@@ -98,29 +98,38 @@ int sk_interface_phc(char *name, int *index)
memset(&ifr, 0, sizeof(ifr));
memset(&info, 0, sizeof(info));
info.cmd = ETHTOOL_GET_TS_INFO;
- strcpy(ifr.ifr_name, name);
+ strncpy(ifr.ifr_name, name, IFNAMSIZ - 1);
ifr.ifr_data = (char *) &info;
fd = socket(AF_INET, SOCK_DGRAM, 0);

if (fd < 0) {
pr_err("socket failed: %m");
- return -1;
+ goto failed;
}

err = ioctl(fd, SIOCETHTOOL, &ifr);
if (err < 0) {
pr_err("ioctl SIOCETHTOOL failed: %m");
close(fd);
- return -1;
+ goto failed;
}

close(fd);
- *index = info.phc_index;

- return info.phc_index < 0 ? -1 : 0;
-#else
- return -1;
+ /* copy the necessary data to sk_info */
+ memset(sk_info, 0, sizeof(struct sk_ts_info));
+ sk_info->valid = 1;
+ sk_info->phc_index = info.phc_index;
+ sk_info->so_timestamping = info.so_timestamping;
+ sk_info->tx_types = info.tx_types;
+ sk_info->rx_filters = info.rx_filters;
+
+ return 0;
#endif
+failed:
+ /* clear data and ensure it is not marked valid */
+ memset(sk_info, 0, sizeof(struct sk_ts_info));
+ return -1;
}

int sk_interface_macaddr(char *name, unsigned char *mac, int len)
diff --git a/sk.h b/sk.h
index 671e839..44025cb 100644
--- a/sk.h
+++ b/sk.h
@@ -23,6 +23,19 @@
#include "transport.h"

/**
+ * struct sk_ts_info - supported timestamping information
+ * @valid: set to non-zero when the info struct contains valid data
+ * @info: structure containing the values returned by GET_TS_INFO
+ */
+struct sk_ts_info {
+ int valid;
+ int phc_index;
+ unsigned int so_timestamping;
+ unsigned int tx_types;
+ unsigned int rx_filters;
+};
+
+/**
* Obtain the numerical index from a network interface by name.
* @param fd An open socket.
* @param device The name of the network interface of interest.
@@ -31,12 +44,12 @@
int sk_interface_index(int fd, char *device);

/**
- * Obtain the PHC device index of a network interface.
+ * Obtain supported timestamping information
* @param name The name of the interface
- * @return index The non-negative phc index associated with this iface.
- * On error a negative integer is returned.
+ * @param info Struct containing obtained timestamping information.
+ * @return zero on success, negative on failure.
*/
-int sk_interface_phc(char *name, int *index);
+int sk_get_ts_info(char *name, struct sk_ts_info *sk_info);

/**
* Obtain the MAC address of a network interface.

[Jacob Keller]

this patch uses the new sk_get_ts_info data to check whether the selected
timestamping mode is supported on all ports. This check is not run if the port
data isn't valid, so it will only fail if the IOCTL is supported. This allows
for support of old kernels that do not support the IOCTL to still work as expected.

Signed-off-by: Jacob Keller <***@intel.com>
---
ptp4l.c | 27 ++++++++++++++++++++++++++-
1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/ptp4l.c b/ptp4l.c
index 7064cc1..3146fd4 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -22,6 +22,7 @@
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
+#include <linux/net_tstamp.h>

#include "clock.h"
#include "config.h"
@@ -135,7 +136,7 @@ int main(int argc, char *argv[])
enum timestamp_type *timestamping = &cfg_settings.timestamping;
struct clock *clock;
struct defaultDS *ds = &cfg_settings.dds;
- int phc_index = -1;
+ int phc_index = -1, required_modes = 0;

if (SIG_ERR == signal(SIGINT, handle_int_quit_term)) {
fprintf(stderr, "cannot handle SIGINT\n");
@@ -254,6 +255,30 @@ int main(int argc, char *argv[])
return -1;
}

+ if (*timestamping == TS_SOFTWARE)
+ required_modes |= SOF_TIMESTAMPING_TX_SOFTWARE |
+ SOF_TIMESTAMPING_RX_SOFTWARE |
+ SOF_TIMESTAMPING_SOFTWARE;
+ else if (*timestamping == TS_LEGACY_HW)
+ required_modes |= SOF_TIMESTAMPING_TX_HARDWARE |
+ SOF_TIMESTAMPING_RX_HARDWARE |
+ SOF_TIMESTAMPING_SYS_HARDWARE;
+ else if (*timestamping == TS_HARDWARE)
+ required_modes |= SOF_TIMESTAMPING_TX_HARDWARE |
+ SOF_TIMESTAMPING_RX_HARDWARE |
+ SOF_TIMESTAMPING_RAW_HARDWARE;
+
+ /* check whether timestamping mode is supported. */
+ for (i = 0; i < cfg_settings.nports; i++) {
+ if (iface[i].ts_info.valid &&
+ !(iface[0].ts_info.so_timestamping & required_modes)) {
+ fprintf(stderr, "interface '%s' does not support "
+ "requested timestamping mode.\n",
+ iface[i].name);
+ return -1;
+ }
+ }
+
/* determine PHC Clock index */
if (ds->free_running) {
phc_index = -1;

[Richard Cochran]

The following series implements the sk_get_ts_info function in place of the
sk_interface_phc function. This change allows for the storage of the get_ts_info
data returned by the ethtool IOCTL. This allows future enhancements which
protect against invalid modes via the use of the supported timestamping
information.
The first patch simply makes the change to sk_interface_phc, and stores the data
inside the interface structure. The second patch actually implements checks for
all of the supported timestamping modes (legacy, hw, and sw).
---
sk: change sk_interface_phc to sk_get_ts_info
ptp4l: use sk_get_ts_info to check timestamping mode

Both applied (followed up with two tiny fixups).

Thanks, Jacob!

Richard