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 <[email protected]>
> 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.
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 <[email protected]>
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.
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 <[email protected]>
> 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_.
>
On Mon, Jun 1, 2015 at 11:08 AM, Christian König
<[email protected]> 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
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
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.
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.