Re: [PATCH RFC 13/35] ACPI: Introduce acpi_parse_entries

[Julien Grall <julien.grall@linaro.org>]

Hi Stefano,

On 05/02/2015 18:29, Stefano Stabellini wrote:
> On Wed, 4 Feb 2015, parth.dixit@linaro.org wrote:
>> From: Naresh Bhat <naresh.bhat@linaro.org>
>>
>> Introduce acpi_parse_entries
>>
>> Signed-off-by: Naresh Bhat <Naresh.Bhat@linaro.org>
>> ---
>>   xen/drivers/acpi/tables.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++
>>   xen/include/xen/acpi.h    |  4 +++
>>   2 files changed, 68 insertions(+)
>>
>> diff --git a/xen/drivers/acpi/tables.c b/xen/drivers/acpi/tables.c
>> index 6754933..5314f0b 100644
>> --- a/xen/drivers/acpi/tables.c
>> +++ b/xen/drivers/acpi/tables.c
>> @@ -239,6 +239,70 @@ void __init acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>>
>>
>>   int __init
>> +acpi_parse_entries(unsigned long table_size,
>> +                acpi_table_entry_handler handler,
>> +                struct acpi_table_header *table_header,
>> +                int entry_id, unsigned int max_entries)
>> +{
>> +        struct acpi_subtable_header *entry;
>> +        int count = 0;
>> +        unsigned long table_end;
>> +
>> +        if (acpi_disabled)
>> +                return -ENODEV;
>> +
>> +        if (!handler)
>> +                return -EINVAL;
>> +
>> +        if (!table_size)
>> +                return -EINVAL;
>> +
>> +        if (!table_header) {
>> +                printk("Table header not present\n");
>> +                return -ENODEV;
>> +        }
>> +
>> +        table_end = (unsigned long)table_header + table_header->length;
>> +
>> +        /* Parse all entries looking for a match. */
>> +
>> +        entry = (struct acpi_subtable_header *)
>> +            ((unsigned long)table_header + 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)) {
>> +                                count = -EINVAL;
>> +                                goto err;
>> +                }
>> +
>> +                /*
>> +                 * If entry->length is 0, break from this loop to avoid
>> +                 * infinite loop.
>> +                 */
>> +                if (entry->length == 0) {
>> +                        printk("[0x%02x] Invalid zero length\n", entry_id);
>> +                        count = -EINVAL;
>> +                        goto err;
>> +                }
>> +
>> +                entry = (struct acpi_subtable_header *)
>> +                    ((unsigned long)entry + entry->length);
>> +        }
>> +
>> +        if (max_entries && count > max_entries) {
>> +                printk("[0x%02x] ignored %i entries of %i found\n",
>> +                        entry_id, count - max_entries, count);
>> +        }
>> +
>> +err:
>> +        return count;
>> +}
>
> This function looks remarkably similar to acpi_table_parse_entries
> below.
> Could you use acpi_table_parse_entries? If not, why?

I suspect that we don't even need to refactor the function 
acpi_table_parse_entries.

The function acpi_parse_entries in only used for the GICv2 ACPI 
initialization.

The current code in the GICv2 is manually doing the work of the 
acpi_table_parse_entries. All just for printing the Local ACPI address, 
we don't even store it...

If we call acpi_parse_madt, it will make the code more readable by 
dropping nearly 30 lines of code, and also dropping this patch.

Regards,

-- 
Julien Grall