Re: [Linaro-acpi] [RFC part2 PATCH 4/9] ARM64 / ACPI: Use Parked Address in GIC structure for spin table SMP initialisation

On 2013-12-10 21:03, Grant Likely wrote: [...]

...

+/* Parked Address in ACPI GIC structure can be used as cpu release addr */ +int acpi_get_parked_address_with_gic_id(u32 gic_id, u64 *parked_address) +{

• struct acpi_table_header *table_header = NULL;
• struct acpi_subtable_header *entry;
• int err = 0;
• unsigned long table_end;
• acpi_size tbl_size;
• struct acpi_madt_generic_interrupt *processor = NULL;
• if (!parked_address)

• return -EINVAL;
  

• acpi_get_table_with_size(ACPI_SIG_MADT, 0, &table_header, &tbl_size);
• if (!table_header) {

• pr_warn(PREFIX "MADT table 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 + sizeof(struct acpi_table_madt));
  

• while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <

•       table_end) {
  

• if (entry->type != ACPI_MADT_TYPE_GENERIC_INTERRUPT
  

• 	|| BAD_MADT_ENTRY(entry, table_end))
  

• 	continue;
  

• processor = (struct acpi_madt_generic_interrupt *)entry;
  

• if (processor->gic_id == gic_id) {
  

• 	*parked_address = processor->parked_address;
  

• 	goto out;
  

• }
  

• entry = (struct acpi_subtable_header *)
  

•     ((unsigned long)entry + entry->length);
  

All of the casting in this table looks suspicious. If you have to resort to casting, then the variable types are very likely wrong.

In the case immediately above, it seems that the entry size doesn't necessarily equal the acpi_subtable_header size, in which case you should cast the values to a void* instead of an unsigned long. That would mean you can do this:

entry = ((void*)entry) + entry->length;

In fact, if I were writing the code, I would have two variables; the iterator pointer as a void* and a header pointer as a struct acpi_subtable_header*. Like so:

void *entry, *table_end; struct acpi_subtable_header *header;

entry = ((void*)table_header) + sizeof(struct acpi_table_madt); table_end = ((void*)table_header) + table_header->length; while (entry + sizeof(*header)) < table_end) { header = entry;

if (header->type != ACPI_MADT_TYPE_GENERIC_INTERRUPT ||
	BAD_MADT_ENTRY(entry, table_end))
	continue;
processor = entry;

if (processor->gic_id == gic_id) {
	*parked_address = processor->parked_address;
	goto out;
}

entry += header->length;

}

See? Much cleaner code.

Aha, much much cleaner, thanks for the guidance, will rework my patch and test it.

Hanjun