Messages in this thread | | | Date | Fri, 10 Jul 2009 08:10:51 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH 09/12] SFI: Enable SFI to parse ACPI tables |
| |
* Len Brown <lenb@kernel.org> wrote:
> From: Feng Tang <feng.tang@intel.com>
> +++ b/drivers/sfi/sfi_acpi.c
> +static struct acpi_table_xsdt *xsdt_va;
Should be __read_mostly.
> +static struct acpi_table_header *sfi_acpi_get_table(char *signature, > + char *oem_id, char *oem_table_id, unsigned int flags) > +{ > + struct acpi_table_header *th; > + u32 tbl_cnt, i; > + u64 pa; > + > + tbl_cnt = XSDT_GET_NUM_ENTRIES(xsdt_va, u64); > + > + for (i = 0; i < tbl_cnt; i++) { > + pa = xsdt_va->table_offset_entry[i]; > + > + th = (struct acpi_table_header *)sfi_map_table(pa); > + if (!th) > + return NULL; > + > + if (strncmp(th->signature, signature, SFI_SIGNATURE_SIZE)) > + goto loop_continue; > + > + if (oem_id && strncmp(th->oem_id, oem_id, SFI_OEM_ID_SIZE)) > + goto loop_continue; > + > + if (oem_table_id && strncmp(th->oem_table_id, oem_table_id, > + SFI_OEM_TABLE_ID_SIZE)) > + goto loop_continue; > + > + return th; /* success */ > +loop_continue: > + sfi_unmap_table((struct sfi_table_header *)th); > + } > + > + return NULL;
That loop looks weird and non-standard.
The body should be factored out into a check_table(th) helper function, and the loop can thus be written as a very straightforward one:
th = NULL;
for (i = 0; i < tbl_cnt; i++) { pa = xsdt_va->table_offset_entry[i];
th = check_table(pa, signature, oem_id, oem_table_id)) if (IS_ERR(th)) return NULL; if (th) return th; }
return th;
where check_table() returns PTR_ERR(-EINVAL) on mapping failure, NULL on mismatch and a table header on match.
Also, a general comment for the whole series: the various type casts between ACPI and SFI table headers look ugly and are a bit fragile. It would be better to define explicit type conversion helper inlines between ACPI an SFI types - and those would thus be type-safe. (right now it's easy to typo a variable name and pass in something that is neither an ACPI nor an SFI header.)
So we should have two primitives:
acpi_to_sfi_table(table) sfi_to_acpi_table(table)
> +static void sfi_acpi_put_table(struct acpi_table_header *table) > +{ > + sfi_put_table((struct sfi_table_header *)table); > +}
the above could thus be written as:
static void sfi_acpi_put_table(struct acpi_table_header *table) { sfi_put_table(acpi_to_sfi_table(table)); }
> +/* > + * sfi_acpi_table_parse() > + * > + * find specified table in XSDT, run handler on it and return its return value > + */ > +int sfi_acpi_table_parse(char *signature, char *oem_id, char *oem_table_id, > + unsigned int flags, int(*handler)(struct acpi_table_header *)) > +{
i'd suggest to define a helper structure for the 'table key' fields above. They get passed around repetitively. Also, a acpi_handler_fn_t helper would be useful as well. That way the above prototype could be written as:
int sfi_acpi_table_parse(struct acpi_table_key key, unsigned int flags, acpi_handler_fn_t *handler);
which is a lot more readable and 'key' could be passed straight to the sfi parser.
> +++ b/include/linux/sfi_acpi.h
> +#ifdef CONFIG_SFI
Small nit: we dont generally put tabs into ifdefs, unless strongly warranted (which this one does not seem to be).
Ingo
| |