Re: [PATCH 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers
From: Marc Zyngier
Date: Wed Sep 09 2015 - 06:48:22 EST
Hi Lucasz,
Nit: Please add a version number to the patches you send - this is at
least the 4th revision of this series, and it is harder to keep track of
what I'm reviewing. git send-email --subject-prefix "PATCH v4" is your
friend.
On 09/09/15 10:30, Lukasz Anaczkowski wrote:
> ACPI subtable parsing needs to be extended to allow two or more
> handlers to be run in the same ACPI table walk, thus adding
> acpi_subtable_proc structure which stores
> () ACPI table id
> () handler that processes table
> () counter how many items has been processed
> and passing it to acpi_parse_entries_array() and
> acpi_table_parse_entries_array().
>
> This is needed to fix CPU enumeration when APIC/X2APIC entries
> are interleaved.
>
> Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@xxxxxxxxx>
> ---
> drivers/acpi/tables.c | 89 +++++++++++++++++++++++++++++++++++++++++----------
> include/linux/acpi.h | 13 ++++++++
> 2 files changed, 85 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 2e19189..13e5089 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -214,20 +214,37 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
> }
> }
>
> +/**
> + * acpi_parse_entries_array - for each proc_num find a subtable with proc->id
> + * and run proc->handler on it. Assumption is that there's only
> + * single handler for particular entry id.
> + *
> + * @id: table id (for debugging purposes)
> + * @table_size: single entry size
> + * @table_header: where does the table start?
> + * @proc: array of acpi_subtable_proc struct containing entry id
> + * and associated handler with it
> + * @proc_num: how big proc is?
> + * @max_entries: how many entries can we process?
> + *
> + * On success returns sum of all matching entries for all proc handlers.
> + * Oterwise, -ENODEV or -EINVAL is returned.
s/Oterwise/Otherwise/
> + */
> int __init
> -acpi_parse_entries(char *id, unsigned long table_size,
> - acpi_tbl_entry_handler handler,
> +acpi_parse_entries_array(char *id, unsigned long table_size,
> struct acpi_table_header *table_header,
> - int entry_id, unsigned int max_entries)
> + struct acpi_subtable_proc *proc, int proc_num,
> + unsigned int max_entries)
It seems that there is no user of this function outside of this file, so
it can be made static.
> {
> struct acpi_subtable_header *entry;
> - int count = 0;
> unsigned long table_end;
> + int count = 0;
> + int i;
>
> if (acpi_disabled)
> return -ENODEV;
>
> - if (!id || !handler)
> + if (!id)
> return -EINVAL;
>
> if (!table_size)
> @@ -247,20 +264,27 @@ acpi_parse_entries(char *id, unsigned long table_size,
>
> while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
> table_end) {
> - if (entry->type == entry_id
> - && (!max_entries || count < max_entries)) {
> - if (handler(entry, table_end))
> + if (max_entries && count >= max_entries)
> + break;
> +
> + for (i = 0; i < proc_num; i++) {
> + if (entry->type != proc[i].id)
> + continue;
> + if (!proc->handler || proc[i].handler(entry, table_end))
> return -EINVAL;
>
> - count++;
> + proc->count++;
> + break;
> }
> + if (i != proc_num)
> + count++;
>
> /*
> * If entry->length is 0, break from this loop to avoid
> * infinite loop.
> */
> if (entry->length == 0) {
> - pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id);
> + pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, proc->id);
> return -EINVAL;
> }
>
> @@ -270,17 +294,31 @@ acpi_parse_entries(char *id, unsigned long table_size,
>
> if (max_entries && count > max_entries) {
> pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n",
> - id, entry_id, count - max_entries, count);
> + id, proc->id, count - max_entries, count);
> }
>
> return count;
> }
>
> +int __init acpi_parse_entries(char *id, unsigned long table_size,
> + acpi_tbl_entry_handler handler,
> + struct acpi_table_header *table_header,
> + int entry_id, unsigned int max_entries)
> +{
> + struct acpi_subtable_proc proc = {
> + .id = entry_id,
> + .handler = handler,
> + .count = 0,
count is implicitly initialized to zero when you have a partial initializer.
> + };
> +
> + return acpi_parse_entries_array(id, table_size, table_header,
> + &proc, 1, max_entries);
> +}
> +
> int __init
> -acpi_table_parse_entries(char *id,
> +acpi_table_parse_entries_array(char *id,
> unsigned long table_size,
> - int entry_id,
> - acpi_tbl_entry_handler handler,
> + struct acpi_subtable_proc *proc, int proc_num,
> unsigned int max_entries)
> {
> struct acpi_table_header *table_header = NULL;
> @@ -291,7 +329,7 @@ acpi_table_parse_entries(char *id,
> if (acpi_disabled)
> return -ENODEV;
>
> - if (!id || !handler)
> + if (!id)
> return -EINVAL;
>
> if (!strncmp(id, ACPI_SIG_MADT, 4))
> @@ -303,14 +341,31 @@ acpi_table_parse_entries(char *id,
> return -ENODEV;
> }
>
> - count = acpi_parse_entries(id, table_size, handler, table_header,
> - entry_id, max_entries);
> + count = acpi_parse_entries_array(id, table_size, table_header,
> + proc, proc_num, max_entries);
>
> early_acpi_os_unmap_memory((char *)table_header, tbl_size);
> return count;
> }
>
> int __init
> +acpi_table_parse_entries(char *id,
> + unsigned long table_size,
> + int entry_id,
> + acpi_tbl_entry_handler handler,
> + unsigned int max_entries)
> +{
> + struct acpi_subtable_proc proc = {
> + .id = entry_id,
> + .handler = handler,
> + .count = 0,
Same here.
> + };
> +
> + return acpi_table_parse_entries_array(id, table_size, &proc, 1,
> + max_entries);
> +}
> +
> +int __init
> acpi_table_parse_madt(enum acpi_madt_type id,
> acpi_tbl_entry_handler handler, unsigned int max_entries)
> {
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index d2445fa..7a25ef9 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -135,6 +135,12 @@ static inline void acpi_initrd_override(void *data, size_t size)
> (!entry) || (unsigned long)entry + sizeof(*entry) > end || \
> ((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
>
> +struct acpi_subtable_proc {
> + int id;
> + acpi_tbl_entry_handler handler;
> + int count;
> +};
> +
> char * __acpi_map_table (unsigned long phys_addr, unsigned long size);
> void __acpi_unmap_table(char *map, unsigned long size);
> int early_acpi_boot_init(void);
> @@ -149,10 +155,17 @@ int __init acpi_parse_entries(char *id, unsigned long table_size,
> acpi_tbl_entry_handler handler,
> struct acpi_table_header *table_header,
> int entry_id, unsigned int max_entries);
> +int __init acpi_parse_entries_array(char *id, unsigned long table_size,
> + struct acpi_table_header *table_header,
> + struct acpi_subtable_proc *proc, int proc_num,
> + unsigned int max_entries);
and you can drop this one if you make it static in tables.c
> int __init acpi_table_parse_entries(char *id, unsigned long table_size,
> int entry_id,
> acpi_tbl_entry_handler handler,
> unsigned int max_entries);
> +int __init acpi_table_parse_entries_array(char *id, unsigned long table_size,
> + struct acpi_subtable_proc *proc, int proc_num,
> + unsigned int max_entries);
> int acpi_table_parse_madt(enum acpi_madt_type id,
> acpi_tbl_entry_handler handler,
> unsigned int max_entries);
>
Other than that, I gave it a quick run on arm64, and it did boot without
any observable issue. If you respin it to address the above minor
comments, you can add my:
Reviewed-by: Marc Zyngier <marc.zyngier@xxxxxxx>
Tested-by: Marc Zyngier <marc.zyngier@xxxxxxx>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/