Discussion:
[Linuxptp-devel] [PATCH] missing: add onestep sync to missing.h
Jacob Keller
2013-08-30 17:57:35 UTC
Permalink
Because HWTSTAMP_TX_ONESTEP_SYNC is an enum, and not a define we can't simply
use a pre-processor. Rather, we create a simple test program and compile it. If
it succeeds we define HAVE_ONESTEP_SYNC flag and uset that in missing.h. In
this way we can then define the value since it doesn't exist. This solves some
compilation problems on kernels which don't have the ONESTEP_SYNC flag. It also
works with distro kernels that have backported the SYNC flag.

Signed-off-by: Jacob Keller <***@intel.com>
---
makefile | 3 +++
missing.h | 6 ++++++
onestep_test.c | 5 +++++
3 files changed, 14 insertions(+)
create mode 100644 onestep_test.c

diff --git a/makefile b/makefile
index 71dcf0c..d966675 100644
--- a/makefile
+++ b/makefile
@@ -21,6 +21,9 @@ FEAT_CFLAGS :=
ifneq ($(shell grep --no-messages clock_adjtime /usr/include/bits/time.h),)
FEAT_CFLAGS += -D_GNU_SOURCE -DHAVE_CLOCK_ADJTIME
endif
+ifeq ($(shell gcc -o /dev/null onestep_test.c),)
+FEAT_CFLAGS += -DHAVE_ONESTEP_SYNC
+endif

DEBUG =
CC = $(CROSS_COMPILE)gcc
diff --git a/missing.h b/missing.h
index 3c2b9d8..99a8d83 100644
--- a/missing.h
+++ b/missing.h
@@ -44,6 +44,12 @@
#define FD_TO_CLOCKID(fd) ((~(clockid_t) (fd) << 3) | CLOCKFD)
#define CLOCKID_TO_FD(clk) ((unsigned int) ~((clk) >> 3))

+#ifndef HAVE_ONESTEP_SYNC
+enum _missing_hwtstamp_tx_types {
+ HWTSTAMP_TX_ONESTEP_SYNC = 2,
+}
+#endif
+
#ifndef HAVE_CLOCK_ADJTIME
static inline int clock_adjtime(clockid_t id, struct timex *tx)
{
diff --git a/onestep_test.c b/onestep_test.c
new file mode 100644
index 0000000..d6aaac2
--- /dev/null
+++ b/onestep_test.c
@@ -0,0 +1,5 @@
+#include <linux/net_tstamp.h>
+
+int main() {
+ return HWTSTAMP_TX_ONESTEP_SYNC;
+}
Richard Cochran
2013-09-03 18:35:47 UTC
Permalink
Post by Jacob Keller
Because HWTSTAMP_TX_ONESTEP_SYNC is an enum, and not a define we can't simply
use a pre-processor. Rather, we create a simple test program and compile it.
Ugh, I sure hope we aren't moving in the direction of autoconf...
Post by Jacob Keller
If
it succeeds we define HAVE_ONESTEP_SYNC flag and uset that in missing.h. In
this way we can then define the value since it doesn't exist. This solves some
compilation problems on kernels which don't have the ONESTEP_SYNC flag. It also
works with distro kernels that have backported the SYNC flag.
---
makefile | 3 +++
missing.h | 6 ++++++
onestep_test.c | 5 +++++
3 files changed, 14 insertions(+)
create mode 100644 onestep_test.c
diff --git a/makefile b/makefile
index 71dcf0c..d966675 100644
--- a/makefile
+++ b/makefile
@@ -21,6 +21,9 @@ FEAT_CFLAGS :=
ifneq ($(shell grep --no-messages clock_adjtime /usr/include/bits/time.h),)
FEAT_CFLAGS += -D_GNU_SOURCE -DHAVE_CLOCK_ADJTIME
endif
+ifeq ($(shell gcc -o /dev/null onestep_test.c),)
^^^
This won't work when cross compiling.
Post by Jacob Keller
+FEAT_CFLAGS += -DHAVE_ONESTEP_SYNC
+endif
Can't you just grep for HWTSTAMP_TX_ONESTEP_SYNC in

$(KBUILD_OUTPUT)/usr/include/linux/net_tstamp.h ?

Thanks,
Richard
Keller, Jacob E
2013-09-03 20:31:51 UTC
Permalink
-----Original Message-----
Sent: Tuesday, September 03, 2013 11:36 AM
To: Keller, Jacob E
Subject: Re: [Linuxptp-devel] [PATCH] missing: add onestep sync to
missing.h
Post by Jacob Keller
Because HWTSTAMP_TX_ONESTEP_SYNC is an enum, and not a define
we can't simply
Post by Jacob Keller
use a pre-processor. Rather, we create a simple test program and
compile it.
Ugh, I sure hope we aren't moving in the direction of autoconf...
Post by Jacob Keller
If
it succeeds we define HAVE_ONESTEP_SYNC flag and uset that in
missing.h. In
Post by Jacob Keller
this way we can then define the value since it doesn't exist. This solves
some
Post by Jacob Keller
compilation problems on kernels which don't have the ONESTEP_SYNC
flag. It also
Post by Jacob Keller
works with distro kernels that have backported the SYNC flag.
---
makefile | 3 +++
missing.h | 6 ++++++
onestep_test.c | 5 +++++
3 files changed, 14 insertions(+)
create mode 100644 onestep_test.c
diff --git a/makefile b/makefile
index 71dcf0c..d966675 100644
--- a/makefile
+++ b/makefile
@@ -21,6 +21,9 @@ FEAT_CFLAGS :=
ifneq ($(shell grep --no-messages clock_adjtime
/usr/include/bits/time.h),)
Post by Jacob Keller
FEAT_CFLAGS += -D_GNU_SOURCE -DHAVE_CLOCK_ADJTIME
endif
+ifeq ($(shell gcc -o /dev/null onestep_test.c),)
^^^
This won't work when cross compiling.
Post by Jacob Keller
+FEAT_CFLAGS += -DHAVE_ONESTEP_SYNC
+endif
Can't you just grep for HWTSTAMP_TX_ONESTEP_SYNC in
$(KBUILD_OUTPUT)/usr/include/linux/net_tstamp.h ?
Thanks,
Richard
You're right that would be a lot better :) I'll rework this.

Thanks,
Jake
Jacob Keller
2013-09-04 00:19:30 UTC
Permalink
this patch uses grep to test whether the net_tstamp.h header has
HWTSTAMP_TX_ONESTEP_SYNC flag defined. If it doesn't then we can simply define
it with the correct value. This works because proper drivers should just report
that the value is not allowed if they don't support onestep mode. This is the
cleanest way to ensure that linuxptp will still work on kernels which have not
defined the one step flag, and also works for any distributions which backport
the feature.

Signed-off-by: Jacob Keller <***@intel.com>
---
makefile | 3 +++
missing.h | 6 ++++++
2 files changed, 9 insertions(+)

diff --git a/makefile b/makefile
index 71dcf0c..d79a1df 100644
--- a/makefile
+++ b/makefile
@@ -21,6 +21,9 @@ FEAT_CFLAGS :=
ifneq ($(shell grep --no-messages clock_adjtime /usr/include/bits/time.h),)
FEAT_CFLAGS += -D_GNU_SOURCE -DHAVE_CLOCK_ADJTIME
endif
+ifneq ($(shell grep --no-messages HWTSTAMP_TX_ONESTEP_SYNC $(KBUILD_OUTPUT)/usr/include/linux/net_tstamp.h),)
+FEAT_CFLAGS += -DHAVE_ONESTEP_SYNC
+endif

DEBUG =
CC = $(CROSS_COMPILE)gcc
diff --git a/missing.h b/missing.h
index 3c2b9d8..99a8d83 100644
--- a/missing.h
+++ b/missing.h
@@ -44,6 +44,12 @@
#define FD_TO_CLOCKID(fd) ((~(clockid_t) (fd) << 3) | CLOCKFD)
#define CLOCKID_TO_FD(clk) ((unsigned int) ~((clk) >> 3))

+#ifndef HAVE_ONESTEP_SYNC
+enum _missing_hwtstamp_tx_types {
+ HWTSTAMP_TX_ONESTEP_SYNC = 2,
+}
+#endif
+
#ifndef HAVE_CLOCK_ADJTIME
static inline int clock_adjtime(clockid_t id, struct timex *tx)
{
Richard Cochran
2013-09-04 16:16:09 UTC
Permalink
Post by Jacob Keller
this patch uses grep to test whether the net_tstamp.h header has
HWTSTAMP_TX_ONESTEP_SYNC flag defined. If it doesn't then we can simply define
it with the correct value. This works because proper drivers should just report
that the value is not allowed if they don't support onestep mode. This is the
cleanest way to ensure that linuxptp will still work on kernels which have not
defined the one step flag, and also works for any distributions which backport
the feature.
Applied.

Thanks,
Richard
Keller, Jacob E
2013-09-04 17:56:36 UTC
Permalink
Post by Richard Cochran
Post by Jacob Keller
this patch uses grep to test whether the net_tstamp.h header has
HWTSTAMP_TX_ONESTEP_SYNC flag defined. If it doesn't then we can simply define
it with the correct value. This works because proper drivers should just report
that the value is not allowed if they don't support onestep mode. This is the
cleanest way to ensure that linuxptp will still work on kernels which have not
defined the one step flag, and also works for any distributions which backport
the feature.
Applied.
Thanks,
Richard
Oops!! Development error here. Forgot a ";" on the enum, and also the
grep did not work for me.. I had to check
$(KBUILD_OUTPUT)/include/uapi/linux/net_tstamp.h

Sorry for the screw-up here. I am sending a patch to apply on
Richard Cochran
2013-09-04 18:04:43 UTC
Permalink
Post by Keller, Jacob E
Post by Richard Cochran
Post by Jacob Keller
this patch uses grep to test whether the net_tstamp.h header has
HWTSTAMP_TX_ONESTEP_SYNC flag defined. If it doesn't then we can simply define
it with the correct value. This works because proper drivers should just report
that the value is not allowed if they don't support onestep mode. This is the
cleanest way to ensure that linuxptp will still work on kernels which have not
defined the one step flag, and also works for any distributions which backport
the feature.
Applied.
Thanks,
Richard
Oops!! Development error here. Forgot a ";" on the enum, and also the
grep did not work for me.. I had to check
$(KBUILD_OUTPUT)/include/uapi/linux/net_tstamp.h
It *would* be there after a 'make headers_install'.

So, I'll take the ";" hunk and ignore the other one.

Thanks,
Richard
Keller, Jacob E
2013-09-04 18:29:27 UTC
Permalink
Post by Richard Cochran
Post by Keller, Jacob E
Post by Richard Cochran
Post by Jacob Keller
this patch uses grep to test whether the net_tstamp.h header has
HWTSTAMP_TX_ONESTEP_SYNC flag defined. If it doesn't then we can simply define
it with the correct value. This works because proper drivers should just report
that the value is not allowed if they don't support onestep mode. This is the
cleanest way to ensure that linuxptp will still work on kernels which have not
defined the one step flag, and also works for any distributions which backport
the feature.
Applied.
Thanks,
Richard
Oops!! Development error here. Forgot a ";" on the enum, and also the
grep did not work for me.. I had to check
$(KBUILD_OUTPUT)/include/uapi/linux/net_tstamp.h
It *would* be there after a 'make headers_install'.
So, I'll take the ";" hunk and ignore the other one.
Thanks,
Richard
Hmm.. I have the fedora kernel headers package there though.. And it
still wasn't finding it....

Thanks,
Richard Cochran
2013-09-04 18:41:12 UTC
Permalink
Post by Keller, Jacob E
Hmm.. I have the fedora kernel headers package there though.. And it
still wasn't finding it....
You should have:

/lib/modules/`uname -r`/build/usr/include

Thanks,
Richard
Jiri Benc
2013-09-04 19:01:58 UTC
Permalink
Post by Richard Cochran
Post by Keller, Jacob E
Hmm.. I have the fedora kernel headers package there though.. And it
still wasn't finding it....
/lib/modules/`uname -r`/build/usr/include
Only if you have kernel-devel (or such, based on distribution) package
installed.

Actually, building against the raw kernel headers is not correct thing
to do. Distributions have sanitized kernel headers in a different
package (e.g. in Fedora, it's kernel-headers), those reside
in /usr/include/linux (thus already in include path) and those should
be the headers linuxptp is built against.

I think the KBUILD_OUTPUT magic should be removed and standard headers
should be used instead. Should I send a patch?

Thanks,

Jiri
--
Jiri Benc
Keller, Jacob E
2013-09-04 20:44:06 UTC
Permalink
Post by Richard Cochran
Post by Keller, Jacob E
Hmm.. I have the fedora kernel headers package there though.. And it
still wasn't finding it....
/lib/modules/`uname -r`/build/usr/include
Thanks,
Richard
I don't... I have /usr/include/linux/net_tstamp.h but
not /lib/modules/$(uname -r)/build/usr/include/linux/net_tstamp.h

I have

kernel, kernel-headers, kernel-devel

Oh figured it out. I have kernels-headers installed but not for the
kernel version I was running. I need to reboot into the proper kernel.

For some reason fedora doesn't track the kernel-headers package along
with the kernel version..

I just did a check, updated to newest kernel version and matching
kernel-headers package.. even with kernel-devel I don't get anything in

/lib/modules/$(uname -r)/build/usr/include/linux/net_tstamp.h

I agree with Jiri we should just use the standard locations for the
kernel userspace headers.


Th
Richard Cochran
2013-09-05 07:21:04 UTC
Permalink
Post by Keller, Jacob E
I agree with Jiri we should just use the standard locations for the
kernel userspace headers.
Well, we can and should support the "standard locations" in addition
to (not instead of) the DIY kernel location. It must work for cross
compiling, too.

I really don't know where redhat puts their headers. Debian seems to
place them into /lib/modules/`uname -r`/source/include/linux.

Probably the best solution would be to have the makefile invoke a
little shell script that looks around at the standard and
DYI/cross locations and prints the result back.

Thanks,
Richard
Jiri Benc
2013-09-05 07:31:35 UTC
Permalink
Post by Richard Cochran
Well, we can and should support the "standard locations" in addition
to (not instead of) the DIY kernel location. It must work for cross
compiling, too.
I really don't know where redhat puts their headers. Debian seems to
place them into /lib/modules/`uname -r`/source/include/linux.
Debian puts sanitized headers in /usr/include/linux, like anyone else.

User space programs are not supposed to compile against unsanitized
kernel headers (i.e., those directly from the kernel), those are
intended for kernel modules only.

Jiri
--
Jiri Benc
Richard Cochran
2013-09-05 07:51:47 UTC
Permalink
Post by Jiri Benc
Post by Richard Cochran
Well, we can and should support the "standard locations" in addition
to (not instead of) the DIY kernel location. It must work for cross
compiling, too.
I really don't know where redhat puts their headers. Debian seems to
place them into /lib/modules/`uname -r`/source/include/linux.
Debian puts sanitized headers in /usr/include/linux, like anyone else.
Aren't these the headers from glibc (and not the current kernel)?

In any case, even debian users are allowed to run a newer kernel.
Post by Jiri Benc
User space programs are not supposed to compile against unsanitized
kernel headers (i.e., those directly from the kernel), those are
intended for kernel modules only.
The headers produced by "make headers_install" are fine to use, and
that is what you find in $(KBUILD_OUTPUT)/usr/include.

Thanks,
Richard
Jiri Benc
2013-09-05 08:18:06 UTC
Permalink
Post by Richard Cochran
Post by Jiri Benc
Debian puts sanitized headers in /usr/include/linux, like anyone else.
Aren't these the headers from glibc (and not the current kernel)?
They come from a separate package with kernel headers. In Fedora
it's kernel-headers package, in Debian it's linux-libc-dev package
(note that despite the name, it's not glibc, it's just the kernel
headers). It's output of make headers_install of the last distribution
provided kernel. It may not be the current running kernel, of course.
Post by Richard Cochran
In any case, even debian users are allowed to run a newer kernel.
Sure. But nothing prevents you from having several different kernels
installed; choosing the one which is running may not be what the user
wants.

Plus, there's really no need to force users to install kernel-devel (or
similar) package just to build linuxptp.
Post by Richard Cochran
The headers produced by "make headers_install" are fine to use, and
that is what you find in $(KBUILD_OUTPUT)/usr/include.
That's true. If they're present. You mentioned
/lib/modules/`uname -r`/source/include/linux which are raw kernel
headers.

I don't have $(KBUILD_OUTPUT)/usr/include on any of my machines
(Fedora, RHEL, Debian, Gentoo). Basically, this works only for the
users that build their own kernel, and only if they do that without
distribution provided tools (rpmbuild etc.).

Jiri
--
Jiri Benc
Richard Cochran
2013-09-05 08:34:52 UTC
Permalink
Post by Jiri Benc
That's true. If they're present. You mentioned
/lib/modules/`uname -r`/source/include/linux which are raw kernel
headers.
Okay, I see.
Post by Jiri Benc
I don't have $(KBUILD_OUTPUT)/usr/include on any of my machines
(Fedora, RHEL, Debian, Gentoo). Basically, this works only for the
users that build their own kernel, and only if they do that without
distribution provided tools (rpmbuild etc.).
Right, so can you help out to find a way to handle both (Fedora, RHEL,
Debian, Gentoo) and $(KBUILD_OUTPUT)/usr/include?

[ It sounds like to me that compiling against a distro kernel that is
newer than the default one is not really possible. ]

Thanks,
Richard

Loading...