[PATCH 05/98] exynos_drm.h: use __u64 from linux/types.h

[PATCH 05/98] exynos_drm.h: use __u64 from linux/types.h

[Mikko Rapeli]

Fixes userspace compilation error:

drm/exynos_drm.h:30:2: error: unknown type name ?uint64_t?

Signed-off-by: Mikko Rapeli <mikko.rapeli@iki.fi>
---
 include/uapi/drm/exynos_drm.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/uapi/drm/exynos_drm.h b/include/uapi/drm/exynos_drm.h
index 5575ed1..c4468f9 100644
--- a/include/uapi/drm/exynos_drm.h
+++ b/include/uapi/drm/exynos_drm.h
@@ -15,6 +15,7 @@
 #ifndef _UAPI_EXYNOS_DRM_H_
 #define _UAPI_EXYNOS_DRM_H_
 
+#include <linux/types.h>
 #include <drm/drm.h>
 
 /**
@@ -27,7 +28,7 @@
  *	- this handle will be set by gem module of kernel side.
  */
 struct drm_exynos_gem_create {
-	uint64_t size;
+	__u64 size;
 	unsigned int flags;
 	unsigned int handle;
 };
@@ -44,7 +45,7 @@ struct drm_exynos_gem_create {
 struct drm_exynos_gem_info {
 	unsigned int handle;
 	unsigned int flags;
-	uint64_t size;
+	__u64 size;
 };
 
 /**
@@ -58,7 +59,7 @@ struct drm_exynos_gem_info {
 struct drm_exynos_vidi_connection {
 	unsigned int connection;
 	unsigned int extensions;
-	uint64_t edid;
+	__u64 edid;
 };
 
 /* memory type definitions. */
-- 
2.1.4

[PATCH 05/98] exynos_drm.h: use __u64 from linux/types.h

[Russell King - ARM Linux]

On Sat, May 30, 2015 at 05:37:57PM +0200, Mikko Rapeli wrote:
> Fixes userspace compilation error:
> 
> drm/exynos_drm.h:30:2: error: unknown type name ?uint64_t?
> 
> Signed-off-by: Mikko Rapeli <mikko.rapeli@iki.fi>

This is another thing which we need to address.  We should not be using
unsigned int/unsigned long/uintXX_t/etc in here, but the __uXX and __sXX
types.

The lesson learned from other DRM developers is that using these types
simplifies the API especially when it comes to the differences between
32-bit and 64-bit machines, and compat applications.

Note that drm/drm.h is all that should need to be included - drm/drm.h
takes care of including linux/types.h when building on Linux platforms.
(note: if your compiler doesn't set __linux__ then you're probably not
using the proper compiler...)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 05/98] exynos_drm.h: use __u64 from linux/types.h

[Christian König]

On 30.05.2015 18:46, Russell King - ARM Linux wrote:
> On Sat, May 30, 2015 at 05:37:57PM +0200, Mikko Rapeli wrote:
>> Fixes userspace compilation error:
>>
>> drm/exynos_drm.h:30:2: error: unknown type name ?uint64_t?
>>
>> Signed-off-by: Mikko Rapeli <mikko.rapeli@iki.fi>
> This is another thing which we need to address.  We should not be using
> unsigned int/unsigned long/uintXX_t/etc in here, but the __uXX and __sXX
> types.
>
> The lesson learned from other DRM developers is that using these types
> simplifies the API especially when it comes to the differences between
> 32-bit and 64-bit machines, and compat applications.
>
> Note that drm/drm.h is all that should need to be included - drm/drm.h
> takes care of including linux/types.h when building on Linux platforms.
> (note: if your compiler doesn't set __linux__ then you're probably not
> using the proper compiler...)
>

Using types that differs on 32-bit and 64-bit machines for a kernel 
interface is indeed a rather bad idea. This not only includes longs, but 
pointers as well.

But the int8_t, int16_t int32_t, int64_t and their unsigned counterparts 
are defined in stdint.h which is part of the ISO/IEC 9899:1999 standard, 
similar is true for size_t.

The __uXX, __sXX and _kernel_size_t types are linux specific as far as I 
know.

For best interoperability and standard conformance I think that 
definitions from the C standard we use should out-rule linux specific 
definitions.

Additional to that "linux/types.h" is not part of the uapi as far as I 
know, so including it in a header which is part of the uapi should be 
forbidden.

So this is a NAK from my side for the whole series, userspace programs 
should include <stdint.h> for the definition of the ISO/IEC 9899:1999 
standard types if necessary.

Regards,
Christian.

[PATCH 05/98] exynos_drm.h: use __u64 from linux/types.h

[Russell King - ARM Linux]

On Mon, Jun 01, 2015 at 10:20:10AM +0200, Christian K?nig wrote:
> Using types that differs on 32-bit and 64-bit machines for a kernel
> interface is indeed a rather bad idea. This not only includes longs, but
> pointers as well.

[cut standard stdint.h types argument which we've heard before]

You need to read Linus' rant on this subject:

 From: Linus Torvalds <torvalds@osdl.org>
 Subject: Re: [RFC] Splitting kernel headers and deprecating __KERNEL__
 Date: Mon, 29 Nov 2004 01:30:46 GMT

 Ok, this discussion has gone on for too long anyway, but let's make it
 easier for everybody. The kernel uses u8/u16/u32 because:

         - the kernel should not depend on, or pollute user-space naming.
           YOU MUST NOT USE "uint32_t" when that may not be defined, and
           user-space rules for when it is defined are arcane and totally
           arbitrary.

         - since the kernel cannot use those types for anything that is
           visible to user space anyway, there has to be alternate names.
           The tradition is to prepend two underscores, so the kernel would
           have to use "__uint32_t" etc for its header files.

         - at that point, there's no longer any valid argument that it's a
           "standard type" (it ain't), and I personally find it a lot more
           readable to just use the types that the kernel has always used:
           __u8/__u16/__u32. For stuff that is only used for the kernel,
           the shorter "u8/u16/u32" versions may be used.

 In short: having the kernel use the same names as user space is ACTIVELY
 BAD, exactly because those names have standards-defined visibility, which
 means that the kernel _cannot_ use them in all places anyway. So don't
 even _try_.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 05/98] exynos_drm.h: use __u64 from linux/types.h

[Christian König]

Yeah, completely agree with Linus on the visibility problem and that's 
exactly the reason why we don't include <stdint.h> in the kernel header 
and expect userspace to define the ISO types somewhere.

But using the types from "include/linux/types.h" and especially 
including it into the uapi headers doesn't make the situation better, 
but rather worse.

With this step we not only make the headers depend on another header 
that isn't part of the uapi, but also pollute the user space namespace 
with __sXX and __uXX types which aren't defined anywhere else.

Regards,
Christian.

On 01.06.2015 10:56, Russell King - ARM Linux wrote:
> On Mon, Jun 01, 2015 at 10:20:10AM +0200, Christian K?nig wrote:
>> Using types that differs on 32-bit and 64-bit machines for a kernel
>> interface is indeed a rather bad idea. This not only includes longs, but
>> pointers as well.
> [cut standard stdint.h types argument which we've heard before]
>
> You need to read Linus' rant on this subject:
>
>   From: Linus Torvalds <torvalds@osdl.org>
>   Subject: Re: [RFC] Splitting kernel headers and deprecating __KERNEL__
>   Date: Mon, 29 Nov 2004 01:30:46 GMT
>
>   Ok, this discussion has gone on for too long anyway, but let's make it
>   easier for everybody. The kernel uses u8/u16/u32 because:
>
>           - the kernel should not depend on, or pollute user-space naming.
>             YOU MUST NOT USE "uint32_t" when that may not be defined, and
>             user-space rules for when it is defined are arcane and totally
>             arbitrary.
>
>           - since the kernel cannot use those types for anything that is
>             visible to user space anyway, there has to be alternate names.
>             The tradition is to prepend two underscores, so the kernel would
>             have to use "__uint32_t" etc for its header files.
>
>           - at that point, there's no longer any valid argument that it's a
>             "standard type" (it ain't), and I personally find it a lot more
>             readable to just use the types that the kernel has always used:
>             __u8/__u16/__u32. For stuff that is only used for the kernel,
>             the shorter "u8/u16/u32" versions may be used.
>
>   In short: having the kernel use the same names as user space is ACTIVELY
>   BAD, exactly because those names have standards-defined visibility, which
>   means that the kernel _cannot_ use them in all places anyway. So don't
>   even _try_.
>

[PATCH 05/98] exynos_drm.h: use __u64 from linux/types.h

[Frans Klaver]

On Mon, Jun 1, 2015 at 11:08 AM, Christian K?nig
<christian.koenig@amd.com> wrote:
> Yeah, completely agree with Linus on the visibility problem and that's
> exactly the reason why we don't include <stdint.h> in the kernel header and
> expect userspace to define the ISO types somewhere.
>
> But using the types from "include/linux/types.h" and especially including it
> into the uapi headers doesn't make the situation better, but rather worse.
>
> With this step we not only make the headers depend on another header that
> isn't part of the uapi, but also pollute the user space namespace with __sXX
> and __uXX types which aren't defined anywhere else.

These __uXX and __sXX types are defined in
include/uapi/asm-generic/ll64.h and pulled in by
include/uapi/linux/types.h. Including linux/types.h is therefore
valid.

Frans

[PATCH 05/98] exynos_drm.h: use __u64 from linux/types.h

[Mikko Rapeli]

On Mon, Jun 01, 2015 at 10:20:10AM +0200, Christian K?nig wrote:
> Additional to that "linux/types.h" is not part of the uapi as far as
> I know, so including it in a header which is part of the uapi should
> be forbidden.

linux/types.h is part of uapi. See usr/include after 'make headers_install'.

-Mikko

[PATCH 05/98] exynos_drm.h: use __u64 from linux/types.h

[Russell King - ARM Linux]

On Mon, Jun 01, 2015 at 11:08:21AM +0200, Christian K?nig wrote:
> Yeah, completely agree with Linus on the visibility problem and that's
> exactly the reason why we don't include <stdint.h> in the kernel header and
> expect userspace to define the ISO types somewhere.
> 
> But using the types from "include/linux/types.h" and especially including it
> into the uapi headers doesn't make the situation better, but rather worse.
> 
> With this step we not only make the headers depend on another header that
> isn't part of the uapi, but also pollute the user space namespace with __sXX
> and __uXX types which aren't defined anywhere else.

1) Header files are permitted to pollute userspace with __-defined stuff.

2) __[su]XX types are defined as part of the kernel's uapi.
   include/uapi/linux/types.h includes asm/types.h, which in userspace
   picks up include/uapi/asm-generic/types.h.  That picks up
   asm-generic/int-ll64.h, which defines these types.

Moreover, this is done throughout the kernel headers _already_ (as you
would expect for a policy set 10+ years ago).

Please, I'm not interested in this discussion, so please don't argue
with me - I may agree with your position, but what you think and what
I think are really not relevant here.  If you have a problem, take it
up with Linus - I made that clear in my email.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 05/98] exynos_drm.h: use __u64 from linux/types.h

[Christian König]

On 01.06.2015 11:38, Russell King - ARM Linux wrote:
> On Mon, Jun 01, 2015 at 11:08:21AM +0200, Christian K?nig wrote:
>> Yeah, completely agree with Linus on the visibility problem and that's
>> exactly the reason why we don't include <stdint.h> in the kernel header and
>> expect userspace to define the ISO types somewhere.
>>
>> But using the types from "include/linux/types.h" and especially including it
>> into the uapi headers doesn't make the situation better, but rather worse.
>>
>> With this step we not only make the headers depend on another header that
>> isn't part of the uapi, but also pollute the user space namespace with __sXX
>> and __uXX types which aren't defined anywhere else.
> 1) Header files are permitted to pollute userspace with __-defined stuff.
>
> 2) __[su]XX types are defined as part of the kernel's uapi.
>     include/uapi/linux/types.h includes asm/types.h, which in userspace
>     picks up include/uapi/asm-generic/types.h.  That picks up
>     asm-generic/int-ll64.h, which defines these types.
>
> Moreover, this is done throughout the kernel headers _already_ (as you
> would expect for a policy set 10+ years ago).
>
> Please, I'm not interested in this discussion, so please don't argue
> with me - I may agree with your position, but what you think and what
> I think are really not relevant here.  If you have a problem, take it
> up with Linus - I made that clear in my email.
>
In this case I'm fine with the changes, but I'm still not sure if that's 
a good idea or not.

Sticking to ISO C still sounds better than doing something on our own. 
And it now looks a bit different than it was in 2004, stdint.h is widely 
adopted in userspace if I'm not completely mistaken.

Anyway you're perfectly right that Linus need to decide and I'm not in 
the mod to argue with him either (got way to much other things to do as 
well).

Regards,
Christian.

[PATCH 05/98] exynos_drm.h: use __u64 from linux/types.h

[Mikko Rapeli]

On Sat, May 30, 2015 at 05:46:56PM +0100, Russell King - ARM Linux wrote:
> Note that drm/drm.h is all that should need to be included - drm/drm.h
> takes care of including linux/types.h when building on Linux platforms.
> (note: if your compiler doesn't set __linux__ then you're probably not
> using the proper compiler...)

Thanks, I'll fix this by including only drm/drm.h in exynos_drm.h,
nouveau_drm.h, radeon_drm.h and via_drm.h patches.

I'm using standard gcc from Debian for the x86 compilation.

-Mikko