Re: [PATCH 09/12] SFI: Enable SFI to parse ACPI tables

Ingo Molnar
2009-07-10T06:11:18Z

* 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