[RFC PATCH] media: videodev2.h: include <linux/time.h> instead of <sys/time.h>

[RFC PATCH] media: videodev2.h: include <linux/time.h> instead of <sys/time.h>

[Masahiro Yamada]

Currently, linux/videodev.h is excluded from the UAPI header test since
it is not self-contained. Building it for user-space would fail.

------------------------(build log1 begin)------------------------

  CC      usr/include/linux/videodev2.h.s
In file included from <command-line>:32:0:
./usr/include/linux/videodev2.h:2320:20: error: field ‘timestamp’ has incomplete type
  struct timespec   timestamp;
                    ^~~~~~~~~

-------------------------(build log1 end)-------------------------

The in-kernel timespec definition exists in include/uapi/linux/time.h,
but just including <linux/time.h> causes a lot of redefinition errors.

------------------------(build log2 begin)------------------------

  CC      usr/include/linux/videodev2.h.s
In file included from ./usr/include/linux/videodev2.h:63:0,
                 from <command-line>:32:
./usr/include/linux/time.h:16:8: error: redefinition of ‘struct timeval’
 struct timeval {
        ^~~~~~~
In file included from /usr/include/x86_64-linux-gnu/sys/time.h:25:0,
                 from ./usr/include/linux/videodev2.h:60,
                 from <command-line>:32:
/usr/include/x86_64-linux-gnu/bits/types/struct_timeval.h:8:8: note: originally defined here
 struct timeval
        ^~~~~~~
In file included from ./usr/include/linux/videodev2.h:63:0,
                 from <command-line>:32:
./usr/include/linux/time.h:30:0: warning: "ITIMER_REAL" redefined
 #define ITIMER_REAL  0

In file included from ./usr/include/linux/videodev2.h:60:0,
                 from <command-line>:32:
/usr/include/x86_64-linux-gnu/sys/time.h:92:0: note: this is the location of the previous definition
 #define ITIMER_REAL ITIMER_REAL

In file included from ./usr/include/linux/videodev2.h:63:0,
                 from <command-line>:32:
./usr/include/linux/time.h:31:0: warning: "ITIMER_VIRTUAL" redefined
 #define ITIMER_VIRTUAL  1

In file included from ./usr/include/linux/videodev2.h:60:0,
                 from <command-line>:32:
/usr/include/x86_64-linux-gnu/sys/time.h:95:0: note: this is the location of the previous definition
 #define ITIMER_VIRTUAL ITIMER_VIRTUAL

In file included from ./usr/include/linux/videodev2.h:63:0,
                 from <command-line>:32:
./usr/include/linux/time.h:32:0: warning: "ITIMER_PROF" redefined
 #define ITIMER_PROF  2

In file included from ./usr/include/linux/videodev2.h:60:0,
                 from <command-line>:32:
/usr/include/x86_64-linux-gnu/sys/time.h:99:0: note: this is the location of the previous definition
 #define ITIMER_PROF ITIMER_PROF

In file included from ./usr/include/linux/videodev2.h:63:0,
                 from <command-line>:32:
./usr/include/linux/time.h:39:8: error: redefinition of ‘struct itimerval’
 struct itimerval {
        ^~~~~~~~~
In file included from ./usr/include/linux/videodev2.h:60:0,
                 from <command-line>:32:
/usr/include/x86_64-linux-gnu/sys/time.h:104:8: note: originally defined here
 struct itimerval
        ^~~~~~~~~

-------------------------(build log2 end)-------------------------

Replacing <sys/time.h> with <linux/time.h> solves it, and allow more
headers to join the UAPI header test.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

I am not 100% sure about the compatibility
between <sys/time.h> and <linux/time.h>, hence RFC.

But, if they were not compatible,
I guess it would have broken already.

I CCed Arnd Bergmann, who might have a better insight.

A related comment is here:
https://lkml.org/lkml/2019/6/4/1046


 include/uapi/linux/videodev2.h | 4 +---
 usr/include/Makefile           | 7 -------
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 530638dffd93..2571130aa1ca 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -57,11 +57,9 @@
 #ifndef _UAPI__LINUX_VIDEODEV2_H
 #define _UAPI__LINUX_VIDEODEV2_H
 
-#ifndef __KERNEL__
-#include <sys/time.h>
-#endif
 #include <linux/compiler.h>
 #include <linux/ioctl.h>
+#include <linux/time.h>
 #include <linux/types.h>
 #include <linux/v4l2-common.h>
 #include <linux/v4l2-controls.h>
diff --git a/usr/include/Makefile b/usr/include/Makefile
index 57b20f7b6729..dafa6cb9b07e 100644
--- a/usr/include/Makefile
+++ b/usr/include/Makefile
@@ -23,7 +23,6 @@ header-test- += asm/shmbuf.h
 header-test- += asm/signal.h
 header-test- += asm/ucontext.h
 header-test- += drm/vmwgfx_drm.h
-header-test- += linux/am437x-vpfe.h
 header-test- += linux/android/binder.h
 header-test- += linux/android/binderfs.h
 header-test-$(CONFIG_CPU_BIG_ENDIAN) += linux/byteorder/big_endian.h
@@ -33,13 +32,10 @@ header-test- += linux/elfcore.h
 header-test- += linux/errqueue.h
 header-test- += linux/fsmap.h
 header-test- += linux/hdlc/ioctl.h
-header-test- += linux/ivtv.h
 header-test- += linux/kexec.h
-header-test- += linux/matroxfb.h
 header-test- += linux/netfilter_ipv4/ipt_LOG.h
 header-test- += linux/netfilter_ipv6/ip6t_LOG.h
 header-test- += linux/nfc.h
-header-test- += linux/omap3isp.h
 header-test- += linux/omapfb.h
 header-test- += linux/patchkey.h
 header-test- += linux/phonet.h
@@ -49,9 +45,6 @@ header-test- += linux/sctp.h
 header-test- += linux/signal.h
 header-test- += linux/sysctl.h
 header-test- += linux/usb/audio.h
-header-test- += linux/v4l2-mediabus.h
-header-test- += linux/v4l2-subdev.h
-header-test- += linux/videodev2.h
 header-test- += linux/vm_sockets.h
 header-test- += sound/asequencer.h
 header-test- += sound/asoc.h
-- 
2.17.1

Re: [RFC PATCH] media: videodev2.h: include <linux/time.h> instead of <sys/time.h>

[Arnd Bergmann]

On Mon, Oct 7, 2019 at 6:10 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> Currently, linux/videodev.h is excluded from the UAPI header test since
> it is not self-contained. Building it for user-space would fail.
>

>
> Replacing <sys/time.h> with <linux/time.h> solves it, and allow more
> headers to join the UAPI header test.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> I am not 100% sure about the compatibility
> between <sys/time.h> and <linux/time.h>, hence RFC.
>
> But, if they were not compatible,
> I guess it would have broken already.
>
> I CCed Arnd Bergmann, who might have a better insight.
>
> A related comment is here:
> https://lkml.org/lkml/2019/6/4/1046

I don't think this can work, there are multiple problems here:

* linux/time.h is still incompatible with sys/time.h, so any application
  tries to include both sys/time.h and linux/videodev2.h now also
  gets the compile-time error.

* The definition of 'struct timespec' in the kernel headers may in
  fact be different from the one in the libc, and we do want to use
  the one from the C library here, otherwise different parts of the
  application may use incompatible struct layouts.

Fixing this correctly depends on one of the remaining y2038
patches that we still have to revisit. There are two aspects that
we should address:

1. The v4l subsystem needs to be changed to handle both the
    old and the new 32-bit layout for timespec (and timeval). Both
    Hans and I have created patches for this in the past, but they
    were never completed and merged.

2. The definition of 'struct timespec' in the kernel headers needs
   to be removed after every user of this struct is gone from
   the kernel. In internal kernel code, the replacement is
   timespec64 or ktime_t, and in user space interfaces, the
   correct replacement is one of __kernel_timespec (the 64-bit
   version), __kernel_old_timespec (the traditional layout) or
   timespec (from the libc headers).

        Arnd

Re: [RFC PATCH] media: videodev2.h: include <linux/time.h> instead of <sys/time.h>

[Masahiro Yamada]

Hi Arnd,

On Tue, Oct 8, 2019 at 12:26 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Oct 7, 2019 at 6:10 AM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> >
> > Currently, linux/videodev.h is excluded from the UAPI header test since
> > it is not self-contained. Building it for user-space would fail.
> >
>
> >
> > Replacing <sys/time.h> with <linux/time.h> solves it, and allow more
> > headers to join the UAPI header test.
> >
> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> > ---
> >
> > I am not 100% sure about the compatibility
> > between <sys/time.h> and <linux/time.h>, hence RFC.
> >
> > But, if they were not compatible,
> > I guess it would have broken already.
> >
> > I CCed Arnd Bergmann, who might have a better insight.
> >
> > A related comment is here:
> > https://lkml.org/lkml/2019/6/4/1046
>
> I don't think this can work, there are multiple problems here:
>
> * linux/time.h is still incompatible with sys/time.h, so any application
>   tries to include both sys/time.h and linux/videodev2.h now also
>   gets the compile-time error.

You are right.
I am so stupidly screwed up. Palm face...

Please ignore this patch,
and thank you for your expert comments!


>
> * The definition of 'struct timespec' in the kernel headers may in
>   fact be different from the one in the libc, and we do want to use
>   the one from the C library here, otherwise different parts of the
>   application may use incompatible struct layouts.
>
> Fixing this correctly depends on one of the remaining y2038
> patches that we still have to revisit. There are two aspects that
> we should address:
>
> 1. The v4l subsystem needs to be changed to handle both the
>     old and the new 32-bit layout for timespec (and timeval). Both
>     Hans and I have created patches for this in the past, but they
>     were never completed and merged.
>
> 2. The definition of 'struct timespec' in the kernel headers needs
>    to be removed after every user of this struct is gone from
>    the kernel. In internal kernel code, the replacement is
>    timespec64 or ktime_t, and in user space interfaces, the
>    correct replacement is one of __kernel_timespec (the 64-bit
>    version), __kernel_old_timespec (the traditional layout) or
>    timespec (from the libc headers).
>
>         Arnd



--
Best Regards

Masahiro Yamada