From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH RFC 13/35] ACPI: Introduce acpi_parse_entries Date: Wed, 11 Feb 2015 13:26:14 +0800 Message-ID: <54DAE7F6.8010404@linaro.org> References: <1423058539-26403-1-git-send-email-parth.dixit@linaro.org> <1423058539-26403-14-git-send-email-parth.dixit@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini , parth.dixit@linaro.org Cc: ian.campbell@citrix.com, Naresh Bhat , tim@xen.org, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, jbeulich@suse.com, christoffer.dall@linaro.org List-Id: xen-devel@lists.xenproject.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 >> >> Introduce acpi_parse_entries >> >> Signed-off-by: Naresh Bhat >> --- >> 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