| Message ID | 1365809306-1323-1-git-send-email-nm@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> An ifdef in drm.h expects to be compiled with full-fledged Linux > toolchain, but it's common to compile kernel with just bare-metal > toolchain which doesn't define __linux__. So, also add __KERNEL__ > check. Seems okay, pushed. Dave.
Hello, On Fri, 12 Apr 2013 18:28:26 -0500 Nishanth Menon <nm@ti.com> wrote: > From: Paul Sokolovsky <paul.sokolovsky@linaro.org> > > An ifdef in drm.h expects to be compiled with full-fledged Linux > toolchain, but it's common to compile kernel with just bare-metal > toolchain which doesn't define __linux__. So, also add __KERNEL__ > check. > > [nm@ti.com: port forward to 3.9-rc6 and post to dri devel for > feedback as RFC] Signed-off-by: Paul Sokolovsky > <paul.sokolovsky@linaro.org> --- > Paul, Dri devel list, > I picked up this patch from linaro tree: > https://git.linaro.org/gitweb?p=people/asac/android/kernel/lt-ti.git;a=patch;h=719fbc876740cf75e82dd082ae5a00dfcf6fff7a > Discussion thread: > http://lists.linaro.org/pipermail/linaro-dev/2011-June/thread.html#4874 > Seems to me as a valid fix even for upstream perhaps? Yes, IIRC, per the discussion you quote above, I sent this patch for review of our (Linaro's) kernel folks to see if it's ok (the patch is simple, story why it's needed may be not such, though I was positive it's needed). It might be forgotten somehow, thanks for picking it up! > Regards, Nishanth Menon > > include/uapi/drm/drm.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index 8d1e2bb..73a99e4 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -36,7 +36,7 @@ > #ifndef _DRM_H_ > #define _DRM_H_ > > -#if defined(__linux__) > +#if defined(__KERNEL__) || defined(__linux__) > > #include <linux/types.h> > #include <asm/ioctl.h>
On Tuesday 16 April 2013 12:48:28 Paul Sokolovsky wrote: > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > index 8d1e2bb..73a99e4 100644 > > --- a/include/uapi/drm/drm.h > > +++ b/include/uapi/drm/drm.h > > @@ -36,7 +36,7 @@ > > #ifndef _DRM_H_ > > #define _DRM_H_ > > > > -#if defined(__linux__) > > +#if defined(__KERNEL__) || defined(__linux__) > > > > #include <linux/types.h> > > #include <asm/ioctl.h> This is still completely bogus, the __KERNEL__ symbol has no significance here. Either make the compiler define __linux__, or remove this #ifdef completely. Arnd
On 12:50-20130416, Arnd Bergmann wrote: > On Tuesday 16 April 2013 12:48:28 Paul Sokolovsky wrote: > > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > > index 8d1e2bb..73a99e4 100644 > > > --- a/include/uapi/drm/drm.h > > > +++ b/include/uapi/drm/drm.h > > > @@ -36,7 +36,7 @@ > > > #ifndef _DRM_H_ > > > #define _DRM_H_ > > > > > > -#if defined(__linux__) > > > +#if defined(__KERNEL__) || defined(__linux__) > > > > > > #include <linux/types.h> > > > #include <asm/ioctl.h> > > This is still completely bogus, the __KERNEL__ symbol has no significance here. > Either make the compiler define __linux__, or remove this #ifdef completely. > Searching the v.39-rc7 tag, and greping for _linux_ a few interesting list pops up. (pruned): arch/arc/Makefile:cflags-y += -mA7 -fno-common -pipe -fno-builtin -D__linux__ arch/h8300/Makefile:KBUILD_CFLAGS += -D__linux__ arch/hexagon/Makefile:KBUILD_CFLAGS += -ffixed-$(TIR_NAME) -DTHREADINFO_REG=$(TIR_NAME) -D__linux__ arch/score/Makefile: -D__linux__ -ffunction-sections -ffreestanding arch/xtensa/Makefile:KBUILD_CFLAGS += -ffreestanding -D__linux__ ^^ these architectures seem to bypass the pain entirely by defining __linux__ arch/mips/include/uapi/asm/sgidefs.h:#ifndef __linux__ drivers/gpu/drm/radeon/radeon_cp.c:#ifdef __linux__ {snip} drivers/scsi/aic7xxx/aic7770.c:#ifdef __linux__ drivers/scsi/aic7xxx/aic79xx.h:#ifndef __linux__ {snip} drivers/scsi/aic7xxx/aic79xx_core.c:#ifdef __linux__ {snip} drivers/scsi/aic7xxx/aic79xx_pci.c:#ifdef __linux__ drivers/scsi/aic7xxx/aic7xxx.h:#ifndef __linux__ drivers/scsi/aic7xxx/aic7xxx.h:#ifndef __linux__ drivers/scsi/aic7xxx/aic7xxx_93cx6.c:#ifdef __linux__ drivers/scsi/aic7xxx/aic7xxx_core.c:#ifdef __linux__ {snip} drivers/scsi/aic7xxx/aic7xxx_pci.c:#ifdef __linux__ drivers/scsi/aic7xxx/aicasm/aicasm.h:#ifdef __linux__ drivers/scsi/aic7xxx/aicasm/aicasm_gram.y:#ifdef __linux__ drivers/scsi/aic7xxx/aicasm/aicasm_macro_scan.l:#ifdef __linux__ drivers/scsi/aic7xxx/aicasm/aicasm_scan.l:#ifdef __linux__ drivers/scsi/aic7xxx/aicasm/aicasm_symbol.c:#ifdef __linux__ drivers/scsi/aic7xxx/aicasm/aicasm_symbol.h:#ifdef __linux__ drivers/scsi/dpt/osd_defs.h:#if (defined(__linux__)) drivers/staging/ced1401/machine.h:#if (defined(__linux__) || defined(_linux) || defined(__linux)) && !defined(LINUX) include/acpi/platform/acenv.h:#if defined(_LINUX) || defined(__linux__) include/linux/coda.h:#if defined(__linux__) include/uapi/drm/drm.h:#if defined(__linux__) include/uapi/linux/coda.h:#if defined(__linux__) include/uapi/linux/fuse.h:#ifdef __linux__ And then we have the following as well.. fs/ext4/ext4.h:#if defined(__KERNEL__) || defined(__linux__) Trying out a few different prebuilt compilers I had around, I see: http://pastebin.com/bTVDLTb1 So, is our approach just to use __linux__ for builds? I am trying to understand rationale behind why #include <linux/types.h> #include <asm/ioctl.h> would want __linux__ and why __KERNEL__ check is un-wanted. Ofcourse, I cant comment about the "One of the BSDs" in else options.. and why we'd like to keep it around in kernel :)
On Tuesday 16 April 2013, Nishanth Menon wrote: > On 12:50-20130416, Arnd Bergmann wrote: > > On Tuesday 16 April 2013 12:48:28 Paul Sokolovsky wrote: > > > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > > > index 8d1e2bb..73a99e4 100644 > > > > --- a/include/uapi/drm/drm.h > > > > +++ b/include/uapi/drm/drm.h > > > > @@ -36,7 +36,7 @@ > > > > #ifndef _DRM_H_ > > > > #define _DRM_H_ > > > > > > > > -#if defined(__linux__) > > > > +#if defined(__KERNEL__) || defined(__linux__) > > > > > > > > #include <linux/types.h> > > > > #include <asm/ioctl.h> > > > > This is still completely bogus, the __KERNEL__ symbol has no significance here. > > Either make the compiler define __linux__, or remove this #ifdef completely. > > > Searching the v.39-rc7 tag, and greping for _linux_ a few interesting > list pops up. (pruned): > arch/arc/Makefile:cflags-y += -mA7 -fno-common -pipe -fno-builtin -D__linux__ > arch/h8300/Makefile:KBUILD_CFLAGS += -D__linux__ > arch/hexagon/Makefile:KBUILD_CFLAGS += -ffixed-$(TIR_NAME) -DTHREADINFO_REG=$(TIR_NAME) -D__linux__ > arch/score/Makefile: -D__linux__ -ffunction-sections -ffreestanding > arch/xtensa/Makefile:KBUILD_CFLAGS += -ffreestanding -D__linux__ > ^^ these architectures seem to bypass the pain entirely by defining > __linux__ Right, that seems like a reasonable approach when the compilers are actually known to be compatible. > arch/mips/include/uapi/asm/sgidefs.h:#ifndef __linux__ On MIPS, they are not. If you are building a Linux kernel with a gcc that was targetted at another ABI, the system call interface may change, so they forbid that here. > drivers/gpu/drm/radeon/radeon_cp.c:#ifdef __linux__ > {snip} > drivers/scsi/aic7xxx/aic7770.c:#ifdef __linux__ > drivers/scsi/aic7xxx/aic79xx.h:#ifndef __linux__ > {snip} > drivers/scsi/aic7xxx/aic79xx_core.c:#ifdef __linux__ > {snip} > drivers/scsi/aic7xxx/aic79xx_pci.c:#ifdef __linux__ > drivers/scsi/aic7xxx/aic7xxx.h:#ifndef __linux__ > drivers/scsi/aic7xxx/aic7xxx.h:#ifndef __linux__ > drivers/scsi/aic7xxx/aic7xxx_93cx6.c:#ifdef __linux__ > drivers/scsi/aic7xxx/aic7xxx_core.c:#ifdef __linux__ > {snip} > drivers/scsi/aic7xxx/aic7xxx_pci.c:#ifdef __linux__ > drivers/scsi/aic7xxx/aicasm/aicasm.h:#ifdef __linux__ > drivers/scsi/aic7xxx/aicasm/aicasm_macro_scan.l:#ifdef __linux__ > drivers/scsi/aic7xxx/aicasm/aicasm_scan.l:#ifdef __linux__ > drivers/scsi/aic7xxx/aicasm/aicasm_symbol.c:#ifdef __linux__ > drivers/scsi/aic7xxx/aicasm/aicasm_symbol.h:#ifdef __linux__ > drivers/scsi/dpt/osd_defs.h:#if (defined(__linux__)) > drivers/staging/ced1401/machine.h:#if (defined(__linux__) || defined(_linux) || defined(__linux)) && !defined(LINUX) These are all drivers that are shared with another OS, or used to be. In most of them, we can probably just remove the #else path, since I don't think they are getting synchronized any more. > include/acpi/platform/acenv.h:#if defined(_LINUX) || defined(__linux__) The acpi header files are maintained outside of Linux and are kept OS-independent. > include/linux/coda.h:#if defined(__linux__) > include/uapi/drm/drm.h:#if defined(__linux__) > include/uapi/linux/coda.h:#if defined(__linux__) > include/uapi/linux/fuse.h:#ifdef __linux__ In case of coda, we should not need to care any more, that header just got broken by the uapi-split for other operating systems. The drm.h and fuse.h header files are in theory still kept OS-agnostic and are actively maintained. > And then we have the following as well.. > fs/ext4/ext4.h:#if defined(__KERNEL__) || defined(__linux__) This seems to have been copied from the ext2 utils. The ext2/ext3 versions of this file don't have support for other operating systems. > Trying out a few different prebuilt compilers I had around, I see: > http://pastebin.com/bTVDLTb1 > > So, is our approach just to use __linux__ for builds? I am trying to > understand rationale behind why #include <linux/types.h> #include <asm/ioctl.h> > would want __linux__ and why __KERNEL__ check is un-wanted. > Ofcourse, I cant comment about the "One of the BSDs" in else options.. > and why we'd like to keep it around in kernel :) We might be in a similar situation on ARM that we are in on MIPS. For instance, there are some compilers that are targetting (old) Android that have a slightly different ABI, and building a kernel with those results in a system call ABI that is incompatible with user space built with a standard compiler. The safest approach is probably to bail out early if __linux__ is not set, and force anyone that wants to use a strange compiler to set the macro manually. Arnd
On Tue, 2013-04-16 at 12:50 +0200, Arnd Bergmann wrote: > On Tuesday 16 April 2013 12:48:28 Paul Sokolovsky wrote: > > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > > index 8d1e2bb..73a99e4 100644 > > > --- a/include/uapi/drm/drm.h > > > +++ b/include/uapi/drm/drm.h > > > @@ -36,7 +36,7 @@ > > > #ifndef _DRM_H_ > > > #define _DRM_H_ > > > > > > -#if defined(__linux__) > > > +#if defined(__KERNEL__) || defined(__linux__) > > > > > > #include <linux/types.h> > > > #include <asm/ioctl.h> > > This is still completely bogus, the __KERNEL__ symbol has no significance here. > Either make the compiler define __linux__, or remove this #ifdef completely. I don't have the full background here but thought I would point out that a similar issue seems to have just cropped up with fuse.h ... http://lkml.org/lkml/2013/4/15/487
On Wed, Apr 17, 2013 at 6:43 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> drivers/scsi/aic7xxx/aicasm/aicasm_symbol.c:#ifdef __linux__ >> drivers/scsi/aic7xxx/aicasm/aicasm_symbol.h:#ifdef __linux__ >> drivers/scsi/dpt/osd_defs.h:#if (defined(__linux__)) >> drivers/staging/ced1401/machine.h:#if (defined(__linux__) || defined(_linux) || defined(__linux)) && !defined(LINUX) > > These are all drivers that are shared with another OS, or used > to be. In most of them, we can probably just remove the > #else path, since I don't think they are getting synchronized > any more. note that DRM gets periodically ported to the *BSD's, so possibly the same applies here.. BR, -R
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 8d1e2bb..73a99e4 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -36,7 +36,7 @@ #ifndef _DRM_H_ #define _DRM_H_ -#if defined(__linux__) +#if defined(__KERNEL__) || defined(__linux__) #include <linux/types.h> #include <asm/ioctl.h>