* Len Brown <lenb@xxxxxxxxxx> wrote:
> From: Feng Tang <feng.tang@xxxxxxxxx>
> +++ 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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
