| Message ID | 1433000370-19509-6-git-send-email-mikko.rapeli@iki.fi |
|---|---|
| State | New |
| Headers | show |
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...)
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.
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_.
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_. >
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
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.
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.
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
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. */
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(-)