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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Stefano,

On 05/02/2015 18:29, Stefano Stabellini wrote:
> On Wed, 4 Feb 2015, parth.dixit@xxxxxxxxxx wrote:
> > From: Naresh Bhat <naresh.bhat@xxxxxxxxxx>
> >
> > Introduce acpi_parse_entries
> >
> > Signed-off-by: Naresh Bhat <Naresh.Bhat@xxxxxxxxxx>
> > ---
> >   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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel