Date:	Fri, 11 Oct 2013 19:19:54 +0100
From:	Matt Fleming <matt@...sole-pimps.org>
To:	"H. Peter Anvin" <hpa@...or.com>
Cc:	linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
	Matt Fleming <matt.fleming@...el.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH 1/2] x86/efi: Include linux/efi.h in asm/efi.h

On Fri, 11 Oct, at 08:25:26AM, H. Peter Anvin wrote:
> The patch description doesn't match what the patch does. 

Oh, bah. I see I wrote the patch description in a weird way. It should
have said something like,

  "Every file that includes asm/efi.h also includes linux/efi.h. Move
   the inclusion of the linux file into asm/efi.h so that users only
   need to include one header."

> We do not normally have the asm file include the linux file, which is
> what the patch seems to do.

Ingo suggested this because no file in arch/x86 includes asm/efi.h
without also including linux/efi.h, because asm/efi.h makes use of the
definitions in linux/efi.h. Admittedly this whole thing is a little
backwards.

I know that the usual idiom is to include the linux file, but x86 is the
only architecture to provide an asm/efi.h header, which means that to
stick with the usual idiom, we'd need an #ifdef CONFIG_X86 in
linux/efi.h to automatically include the asm file.

Unless we have an empty efi.h in include/asm-generic?

In fact, now I think about it, that would be a much better solution
because there's already a bunch of x86-specific defintions in
linux/efi.h, so the asm-generic/efi.h wouldn't be empty because we could
split out things like...


#ifdef CONFIG_X86
extern void efi_late_init(void);
extern void efi_free_boot_services(void);
extern efi_status_t efi_query_variable_store(u32 attributes, unsigned long size);
#else
static inline void efi_late_init(void) {}
static inline void efi_free_boot_services(void) {}

static inline efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
{
	return EFI_SUCCESS;
}
#endif


Thoughts?
 
[ Patch included because I dropped Ingo from original Cc list ]

> Matt Fleming <matt@...sole-pimps.org> wrote:
> >From: Matt Fleming <matt.fleming@...el.com>
> >
> >Every file that includes asm/efi.h also includes linux/efi.h. Just
> >include linux/efi.h directly and avoid the duplication.
> >
> >Cc: H. Peter Anvin <hpa@...or.com>
> >Cc: Thomas Gleixner <tglx@...utronix.de>
> >Suggested-by: Ingo Molnar <mingo@...nel.org>
> >Signed-off-by: Matt Fleming <matt.fleming@...el.com>
> >---
> > arch/x86/boot/compressed/eboot.c | 1 -
> > arch/x86/include/asm/efi.h       | 2 ++
> > arch/x86/kernel/setup.c          | 1 -
> > arch/x86/platform/efi/efi.c      | 1 -
> > arch/x86/platform/efi/efi_32.c   | 1 -
> > arch/x86/platform/efi/efi_64.c   | 1 -
> > arch/x86/platform/uv/bios_uv.c   | 1 -
> > 7 files changed, 2 insertions(+), 6 deletions(-)
> >
> >diff --git a/arch/x86/boot/compressed/eboot.c
> >b/arch/x86/boot/compressed/eboot.c
> >index b7388a4..3f1dae2 100644
> >--- a/arch/x86/boot/compressed/eboot.c
> >+++ b/arch/x86/boot/compressed/eboot.c
> >@@ -7,7 +7,6 @@
> >  *
> >*
> >-----------------------------------------------------------------------
> >*/
> > 
> >-#include <linux/efi.h>
> > #include <linux/pci.h>
> > #include <asm/efi.h>
> > #include <asm/setup.h>
> >diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> >index 0062a01..b10ea9e 100644
> >--- a/arch/x86/include/asm/efi.h
> >+++ b/arch/x86/include/asm/efi.h
> >@@ -1,6 +1,8 @@
> > #ifndef _ASM_X86_EFI_H
> > #define _ASM_X86_EFI_H
> > 
> >+#include <linux/efi.h>
> >+
> > #ifdef CONFIG_X86_32
> > 
> > #define EFI_LOADER_SIGNATURE	"EL32"
> >diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> >index f0de629..35e9883 100644
> >--- a/arch/x86/kernel/setup.c
> >+++ b/arch/x86/kernel/setup.c
> >@@ -37,7 +37,6 @@
> > #include <linux/root_dev.h>
> > #include <linux/highmem.h>
> > #include <linux/module.h>
> >-#include <linux/efi.h>
> > #include <linux/init.h>
> > #include <linux/edd.h>
> > #include <linux/iscsi_ibft.h>
> >diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> >index c7e22ab..543a4d9 100644
> >--- a/arch/x86/platform/efi/efi.c
> >+++ b/arch/x86/platform/efi/efi.c
> >@@ -30,7 +30,6 @@
> > 
> > #include <linux/kernel.h>
> > #include <linux/init.h>
> >-#include <linux/efi.h>
> > #include <linux/efi-bgrt.h>
> > #include <linux/export.h>
> > #include <linux/bootmem.h>
> >diff --git a/arch/x86/platform/efi/efi_32.c
> >b/arch/x86/platform/efi/efi_32.c
> >index 40e4469..dd566d1 100644
> >--- a/arch/x86/platform/efi/efi_32.c
> >+++ b/arch/x86/platform/efi/efi_32.c
> >@@ -22,7 +22,6 @@
> > #include <linux/kernel.h>
> > #include <linux/types.h>
> > #include <linux/ioport.h>
> >-#include <linux/efi.h>
> > 
> > #include <asm/io.h>
> > #include <asm/desc.h>
> >diff --git a/arch/x86/platform/efi/efi_64.c
> >b/arch/x86/platform/efi/efi_64.c
> >index 39a0e7f..f146de9 100644
> >--- a/arch/x86/platform/efi/efi_64.c
> >+++ b/arch/x86/platform/efi/efi_64.c
> >@@ -23,7 +23,6 @@
> > #include <linux/bootmem.h>
> > #include <linux/ioport.h>
> > #include <linux/module.h>
> >-#include <linux/efi.h>
> > #include <linux/uaccess.h>
> > #include <linux/io.h>
> > #include <linux/reboot.h>
> >diff --git a/arch/x86/platform/uv/bios_uv.c
> >b/arch/x86/platform/uv/bios_uv.c
> >index 7666121..e55b074 100644
> >--- a/arch/x86/platform/uv/bios_uv.c
> >+++ b/arch/x86/platform/uv/bios_uv.c
> >@@ -19,7 +19,6 @@
> >  *  Copyright (c) Russ Anderson <rja@....com>
> >  */
> > 
> >-#include <linux/efi.h>
> > #include <linux/export.h>
> > #include <asm/efi.h>
> > #include <linux/io.h>
> 
> -- 
> Sent from my mobile phone.  Please pardon brevity and lack of formatting.

-- 
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists