| Message ID | 1455299022-11641-4-git-send-email-aleksey.makarov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Aleksey,
[auto build test ERROR on pm/linux-next]
[also build test ERROR on v4.5-rc3 next-20160212]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Aleksey-Makarov/ACPI-parse-the-SPCR-table/20160213-015350
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-tinyconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
arch/x86/kernel/i8259.o: In function `acpi_table_parse2':
>> i8259.c:(.text+0x2c4): multiple definition of `acpi_table_parse2'
arch/x86/kernel/setup.o:setup.c:(.text+0x3): first defined here
arch/x86/kernel/irqinit.o: In function `acpi_table_parse2':
irqinit.c:(.text+0x0): multiple definition of `acpi_table_parse2'
arch/x86/kernel/setup.o:setup.c:(.text+0x3): first defined here
arch/x86/kernel/bootflag.o: In function `acpi_table_parse2':
bootflag.c:(.text+0x0): multiple definition of `acpi_table_parse2'
arch/x86/kernel/setup.o:setup.c:(.text+0x3): first defined here
arch/x86/kernel/e820.o: In function `acpi_table_parse2':
e820.c:(.text+0x0): multiple definition of `acpi_table_parse2'
arch/x86/kernel/setup.o:setup.c:(.text+0x3): first defined here
arch/x86/kernel/pci-dma.o: In function `acpi_table_parse2':
pci-dma.c:(.text+0x0): multiple definition of `acpi_table_parse2'
arch/x86/kernel/setup.o:setup.c:(.text+0x3): first defined here
arch/x86/kernel/rtc.o: In function `acpi_table_parse2':
rtc.c:(.text+0x43): multiple definition of `acpi_table_parse2'
arch/x86/kernel/setup.o:setup.c:(.text+0x3): first defined here
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Fri, Feb 12, 2016 at 08:43:34PM +0300, Aleksey Makarov wrote: > The function acpi_table_parse() has some problems: > 1 It can be called only from __init code > 2 It does not pass any data to the handler > 3 It just throws out the value returned from the handler > > These issues are addressed in this patch Why not just fix acpi_table_parse(), like you have, and not add a new API call with a "2" at the end of it. That seems crazy to try to maintain that level of apis. But I'm not the acpi maintainer(s), so it's their call... good luck, greg k-h
On Fri, Feb 12, 2016 at 6:43 PM, Aleksey Makarov <aleksey.makarov@linaro.org> wrote: > The function acpi_table_parse() has some problems: > 1 It can be called only from __init code > 2 It does not pass any data to the handler > 3 It just throws out the value returned from the handler So why are those problems? > These issues are addressed in this patch How are they addressed? Thanks, Rafael
On Fri, Feb 12, 2016 at 7:51 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Fri, Feb 12, 2016 at 08:43:34PM +0300, Aleksey Makarov wrote: >> The function acpi_table_parse() has some problems: >> 1 It can be called only from __init code >> 2 It does not pass any data to the handler >> 3 It just throws out the value returned from the handler >> >> These issues are addressed in this patch > > Why not just fix acpi_table_parse(), like you have, and not add a new > API call with a "2" at the end of it. That seems crazy to try to > maintain that level of apis. > > But I'm not the acpi maintainer(s), so it's their call... The ACPI maintainer agrees. Thanks, Rafael
On 02/13/2016 02:07 AM, Rafael J. Wysocki wrote: > On Fri, Feb 12, 2016 at 6:43 PM, Aleksey Makarov > <aleksey.makarov@linaro.org> wrote: >> The function acpi_table_parse() has some problems: >> 1 It can be called only from __init code >> 2 It does not pass any data to the handler >> 3 It just throws out the value returned from the handler > > So why are those problems? 1. We need this function to be non-__init because we need access to some tables at unpredictable time--it may be before or after the init functions are removed. For example, SPCR (Serial Port Console Redirection) table is needed each time a new console is registered. It can be quite early (console_initcall) or when a module is inserted. 2. Having an additional pointer to void is very useful because it allows drivers to pass (local) data to the handler. Without this, we would need to have global data and a mutex as it was done in drivers/acpi/scan.c (vars ape, acpi_probe_count, acpi_probe_lock). 3. Passing the value returned from the callback is less important as it could be modelled by having a (return) field in the data passed to the handler. It is very convenient though. I use these three properties in my next patch of this series. >> These issues are addressed in this patch > > How are they addressed? A new function acpi_table_parse2() has been created that is just the old acpi_table_parse() without __init, taking the handler with additional data pointer and respecting the value returned from the handler. The old acpi_table_parse() was implemented witht this new function. Dropping __init was made possible by the previous patch where the attribute of the function early_acpi_os_unmap_memory() was changed from __init to __ref and by dropping __init from acpi_apic_instance. I understand why this approach is not good and I am going to fix it. Thank you Aleksey Makarov
Hi Rafael, Thank you for review. On 02/13/2016 02:08 AM, Rafael J. Wysocki wrote: > On Fri, Feb 12, 2016 at 7:51 PM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: >> On Fri, Feb 12, 2016 at 08:43:34PM +0300, Aleksey Makarov wrote: >>> The function acpi_table_parse() has some problems: >>> 1 It can be called only from __init code >>> 2 It does not pass any data to the handler >>> 3 It just throws out the value returned from the handler >>> >>> These issues are addressed in this patch >> >> Why not just fix acpi_table_parse(), like you have, and not add a new >> API call with a "2" at the end of it. That seems crazy to try to >> maintain that level of apis. >> >> But I'm not the acpi maintainer(s), so it's their call... > > The ACPI maintainer agrees. I see. How would you prefer it to be fixed: 1. Change the signature/implementation of acpi_table_parse(). CON: It would require extensive changes through all the kernel, which I am not sure I will be able to test (but these changes are just adding an unused arg to the handler + checking that the return value is consistent) OR 2. Have a local implementation of the function like acpi_table_parse2() CON: A bit of code duplication. Thank you Aleksey Makarov
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 6c0f079..98ae052 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -40,7 +40,7 @@ static char *mps_inti_flags_trigger[] = { "dfl", "edge", "res", "level" }; static struct acpi_table_desc initial_tables[ACPI_MAX_TABLES] __initdata; -static int acpi_apic_instance __initdata; +static int acpi_apic_instance; /* * Disable table checksum verification for the early stage due to the size @@ -374,19 +374,27 @@ acpi_table_parse_madt(enum acpi_madt_type id, } /** - * acpi_table_parse - find table with @id, run @handler on it + * acpi_table_parse2 - find table with @id, run @handler on it * @id: table id to find * @handler: handler to run + * @data: data to pass to handler * * Scan the ACPI System Descriptor Table (STD) for a table matching @id, * run @handler on it. * - * Return 0 if table found, -errno if not. + * How it differs from acpi_table_parse() + * 1. It respect the return value of the handler function + * 2. It has additional data argument to make closures + * 3. It can be called from everywhere from the kernel (no __init) + * + * Return: the value returned by the handler if table found, -errno if not. */ -int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler) +int acpi_table_parse2(char *id, + int (*handler)(struct acpi_table_header *table, void *data), void *data) { struct acpi_table_header *table = NULL; acpi_size tbl_size; + int err; if (acpi_disabled) return -ENODEV; @@ -395,16 +403,43 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler) return -EINVAL; if (strncmp(id, ACPI_SIG_MADT, 4) == 0) - acpi_get_table_with_size(id, acpi_apic_instance, &table, &tbl_size); + acpi_get_table_with_size(id, acpi_apic_instance, &table, + &tbl_size); else acpi_get_table_with_size(id, 0, &table, &tbl_size); if (table) { - handler(table); + err = handler(table, data); early_acpi_os_unmap_memory(table, tbl_size); - return 0; + return err; } else return -ENODEV; + +} + +/* + * 1. fix the signature + * 2. always return 0, don't respect the value returned from the handler + */ +static int legacy_acpi_table_handler(struct acpi_table_header *table, void *d) +{ + ((acpi_tbl_table_handler)d)(table); + return 0; +} + +/** + * acpi_table_parse - find table with @id, run @handler on it + * @id: table id to find + * @handler: handler to run + * + * Scan the ACPI System Descriptor Table (STD) for a table matching @id, + * run @handler on it. + * + * Return 0 if table found, -errno if not. + */ +int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler) +{ + return acpi_table_parse2(id, legacy_acpi_table_handler, handler); } /* diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 06ed7e5..bed9b89 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -218,6 +218,8 @@ int acpi_numa_init (void); int acpi_table_init (void); int acpi_table_parse(char *id, acpi_tbl_table_handler handler); +int acpi_table_parse2(char *id, int (*handler)(struct acpi_table_header *table, + void *data), void *data); int __init acpi_parse_entries(char *id, unsigned long table_size, acpi_tbl_entry_handler handler, struct acpi_table_header *table_header, @@ -636,6 +638,12 @@ static inline int acpi_table_parse(char *id, return -ENODEV; } +int acpi_table_parse2(char *id, int (*handler)(struct acpi_table_header *table, + void *data), void *data) +{ + return -ENODEV; +} + static inline int acpi_nvs_register(__u64 start, __u64 size) { return 0;
The function acpi_table_parse() has some problems: 1 It can be called only from __init code 2 It does not pass any data to the handler 3 It just throws out the value returned from the handler These issues are addressed in this patch Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org> --- drivers/acpi/tables.c | 49 ++++++++++++++++++++++++++++++++++++++++++------- include/linux/acpi.h | 8 ++++++++ 2 files changed, 50 insertions(+), 7 deletions(-)