| Message ID | 20171207122821.30158-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
* Juergen Gross <jgross@suse.com> wrote: > In case the rsdp address in struct boot_params is specified don't try > to find the table by searching, but take the address directly as set > by the boot loader. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > drivers/acpi/osl.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c > index 3bb46cb24a99..3b25e2ad7d75 100644 > --- a/drivers/acpi/osl.c > +++ b/drivers/acpi/osl.c > @@ -45,6 +45,10 @@ > #include <linux/uaccess.h> > #include <linux/io-64-nonatomic-lo-hi.h> > > +#ifdef CONFIG_X86 > +#include <asm/setup.h> > +#endif > + > #include "internal.h" > > #define _COMPONENT ACPI_OS_SERVICES > @@ -195,6 +199,10 @@ acpi_physical_address __init acpi_os_get_root_pointer(void) > if (acpi_rsdp) > return acpi_rsdp; > #endif > +#ifdef CONFIG_X86 > + if (boot_params.hdr.acpi_rsdp_addr) > + return boot_params.hdr.acpi_rsdp_addr; > +#endif Argh, that's typical short sighted hackery, layering violations and general eyesore combined into a single patch ... Those #ifdefs are a disgrace, plus why should generic ACPI code include platform details like boot_params.hdr/acpi_rsdp_addr? It's also not very extensible to non-x86 - so someone will have to redo this work for ARM64 as well in the future ... So how about doing it right: 1) Add a __weak acpi_arch_get_root_pointer() __weak function to drivers/acpi/osl.c: __weak acpi_physical_address acpi_arch_get_root_pointer(void) { return 0; } 2) use it in acpi_os_get_root_pointer(): ... pa = acpi_arch_get_root_pointer(); if (pa) return pa; ... 3) Override the default variant in x86's acpi.c via something like: acpi_physical_address acpi_arch_get_root_pointer(void) { return boot_params.hdr.acpi_rsdp_addr; } 4) Add this to arch/x86/include/asm/acpi.h: extern acpi_physical_address acpi_arch_get_root_pointer(void); 5) Add #include <asm/acpi.h> to drivers/acpi/osl.c. That looks much cleaner, has no layering violations and is infinitely more extensible, right? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/12/17 08:05, Ingo Molnar wrote: > > * Juergen Gross <jgross@suse.com> wrote: > >> In case the rsdp address in struct boot_params is specified don't try >> to find the table by searching, but take the address directly as set >> by the boot loader. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> drivers/acpi/osl.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c >> index 3bb46cb24a99..3b25e2ad7d75 100644 >> --- a/drivers/acpi/osl.c >> +++ b/drivers/acpi/osl.c >> @@ -45,6 +45,10 @@ >> #include <linux/uaccess.h> >> #include <linux/io-64-nonatomic-lo-hi.h> >> >> +#ifdef CONFIG_X86 >> +#include <asm/setup.h> >> +#endif >> + >> #include "internal.h" >> >> #define _COMPONENT ACPI_OS_SERVICES >> @@ -195,6 +199,10 @@ acpi_physical_address __init acpi_os_get_root_pointer(void) >> if (acpi_rsdp) >> return acpi_rsdp; >> #endif >> +#ifdef CONFIG_X86 >> + if (boot_params.hdr.acpi_rsdp_addr) >> + return boot_params.hdr.acpi_rsdp_addr; >> +#endif > > Argh, that's typical short sighted hackery, layering violations and general > eyesore combined into a single patch ... > > Those #ifdefs are a disgrace, plus why should generic ACPI code include platform > details like boot_params.hdr/acpi_rsdp_addr? It's also not very extensible to > non-x86 - so someone will have to redo this work for ARM64 as well in the future > ... > > So how about doing it right: > > 1) > > Add a __weak acpi_arch_get_root_pointer() __weak function to drivers/acpi/osl.c: > > > __weak acpi_physical_address acpi_arch_get_root_pointer(void) > { > return 0; > } > > 2) > > use it in acpi_os_get_root_pointer(): > > ... > pa = acpi_arch_get_root_pointer(); > if (pa) > return pa; > ... > > 3) > > Override the default variant in x86's acpi.c via something like: > > acpi_physical_address acpi_arch_get_root_pointer(void) > { > return boot_params.hdr.acpi_rsdp_addr; > } > > 4) > > Add this to arch/x86/include/asm/acpi.h: > > extern acpi_physical_address acpi_arch_get_root_pointer(void); > > 5) > > Add #include <asm/acpi.h> to drivers/acpi/osl.c. > > > That looks much cleaner, has no layering violations and is infinitely more > extensible, right? Right. Thanks for the very constructive comment. Juergen -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/12/17 08:05, Ingo Molnar wrote: > > * Juergen Gross <jgross@suse.com> wrote: ... > acpi_physical_address acpi_arch_get_root_pointer(void) > { > return boot_params.hdr.acpi_rsdp_addr; > } > > 4) > > Add this to arch/x86/include/asm/acpi.h: > > extern acpi_physical_address acpi_arch_get_root_pointer(void); Uuh, this leads to problems for files including <asm/acpi.h> directly: acpi_physical_address won't be defined, and including <acpi/actypes.h> from arch/x86/include/asm/acpi.h will lead to: #error unknown ACPI_MACHINE_WIDTH This can only be avoided by including <linux/acpi.h> from <asm/acpi.h> which seems to be the wrong layering. So I could: a) modify the sources including <asm/acpi.h> to use <linux/acpi.h> instead b) don't use acpi_physical_address but either u64 or unsigned long. c) ? What would be your preference? Juergen -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Juergen Gross <jgross@suse.com> wrote: > On 08/12/17 08:05, Ingo Molnar wrote: > > > > * Juergen Gross <jgross@suse.com> wrote: > > ... > > > acpi_physical_address acpi_arch_get_root_pointer(void) > > { > > return boot_params.hdr.acpi_rsdp_addr; > > } > > > > 4) > > > > Add this to arch/x86/include/asm/acpi.h: > > > > extern acpi_physical_address acpi_arch_get_root_pointer(void); > > Uuh, this leads to problems for files including <asm/acpi.h> directly: > acpi_physical_address won't be defined, and including <acpi/actypes.h> > from arch/x86/include/asm/acpi.h will lead to: > > #error unknown ACPI_MACHINE_WIDTH > > This can only be avoided by including <linux/acpi.h> from <asm/acpi.h> > which seems to be the wrong layering. > > So I could: > > a) modify the sources including <asm/acpi.h> to use <linux/acpi.h> > instead > b) don't use acpi_physical_address but either u64 or unsigned long. > c) ? > > What would be your preference? Would it help if you put the prototype into linux/acpi.h perhaps? It's a generic facility in principle, even if only used by x86 at the moment. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/12/17 12:26, Ingo Molnar wrote: > > * Juergen Gross <jgross@suse.com> wrote: > >> On 08/12/17 08:05, Ingo Molnar wrote: >>> >>> * Juergen Gross <jgross@suse.com> wrote: >> >> ... >> >>> acpi_physical_address acpi_arch_get_root_pointer(void) >>> { >>> return boot_params.hdr.acpi_rsdp_addr; >>> } >>> >>> 4) >>> >>> Add this to arch/x86/include/asm/acpi.h: >>> >>> extern acpi_physical_address acpi_arch_get_root_pointer(void); >> >> Uuh, this leads to problems for files including <asm/acpi.h> directly: >> acpi_physical_address won't be defined, and including <acpi/actypes.h> >> from arch/x86/include/asm/acpi.h will lead to: >> >> #error unknown ACPI_MACHINE_WIDTH >> >> This can only be avoided by including <linux/acpi.h> from <asm/acpi.h> >> which seems to be the wrong layering. >> >> So I could: >> >> a) modify the sources including <asm/acpi.h> to use <linux/acpi.h> >> instead >> b) don't use acpi_physical_address but either u64 or unsigned long. >> c) ? >> >> What would be your preference? > > Would it help if you put the prototype into linux/acpi.h perhaps? It's a generic > facility in principle, even if only used by x86 at the moment. Yes, that seems to work. Thanks, Juergen -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index 3bb46cb24a99..3b25e2ad7d75 100644 --- a/drivers/acpi/osl.c +++ b/drivers/acpi/osl.c @@ -45,6 +45,10 @@ #include <linux/uaccess.h> #include <linux/io-64-nonatomic-lo-hi.h> +#ifdef CONFIG_X86 +#include <asm/setup.h> +#endif + #include "internal.h" #define _COMPONENT ACPI_OS_SERVICES @@ -195,6 +199,10 @@ acpi_physical_address __init acpi_os_get_root_pointer(void) if (acpi_rsdp) return acpi_rsdp; #endif +#ifdef CONFIG_X86 + if (boot_params.hdr.acpi_rsdp_addr) + return boot_params.hdr.acpi_rsdp_addr; +#endif if (efi_enabled(EFI_CONFIG_TABLES)) { if (efi.acpi20 != EFI_INVALID_TABLE_ADDR)
In case the rsdp address in struct boot_params is specified don't try to find the table by searching, but take the address directly as set by the boot loader. Signed-off-by: Juergen Gross <jgross@suse.com> --- drivers/acpi/osl.c | 8 ++++++++ 1 file changed, 8 insertions(+)