Re: [PATCH 1/2] acpi: Added acpi_subtable_proc to ACPI table parsers

[Marc Zyngier <marc.zyngier@arm.com>]

Hi Lucasz,

Nit: Please add a version number to the patches you send - this is at
least the 4th revision of this series, and it is harder to keep track of
what I'm reviewing. git send-email --subject-prefix "PATCH v4" is your
friend.

On 09/09/15 10:30, Lukasz Anaczkowski wrote:
> ACPI subtable parsing needs to be extended to allow two or more
> handlers to be run in the same ACPI table walk, thus adding
> acpi_subtable_proc structure which stores
> () ACPI table id
> () handler that processes table
> () counter how many items has been processed
> and passing it to acpi_parse_entries_array() and
> acpi_table_parse_entries_array().
> 
> This is needed to fix CPU enumeration when APIC/X2APIC entries
> are interleaved.
> 
> Signed-off-by: Lukasz Anaczkowski <lukasz.anaczkowski@intel.com>
> ---
>  drivers/acpi/tables.c | 89 +++++++++++++++++++++++++++++++++++++++++----------
>  include/linux/acpi.h  | 13 ++++++++
>  2 files changed, 85 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 2e19189..13e5089 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -214,20 +214,37 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>  	}
>  }
>  
> +/**
> + * acpi_parse_entries_array - for each proc_num find a subtable with proc->id
> + *    and run proc->handler on it. Assumption is that there's only
> + *    single handler for particular entry id.
> + *
> + * @id: table id (for debugging purposes)
> + * @table_size: single entry size
> + * @table_header: where does the table start?
> + * @proc: array of acpi_subtable_proc struct containing entry id
> + *        and associated handler with it
> + * @proc_num: how big proc is?
> + * @max_entries: how many entries can we process?
> + *
> + * On success returns sum of all matching entries for all proc handlers.
> + * Oterwise, -ENODEV or -EINVAL is returned.

s/Oterwise/Otherwise/

> + */
>  int __init
> -acpi_parse_entries(char *id, unsigned long table_size,
> -		acpi_tbl_entry_handler handler,
> +acpi_parse_entries_array(char *id, unsigned long table_size,
>  		struct acpi_table_header *table_header,
> -		int entry_id, unsigned int max_entries)
> +		struct acpi_subtable_proc *proc, int proc_num,
> +		unsigned int max_entries)

It seems that there is no user of this function outside of this file, so
it can be made static.

>  {
>  	struct acpi_subtable_header *entry;
> -	int count = 0;
>  	unsigned long table_end;
> +	int count = 0;
> +	int i;
>  
>  	if (acpi_disabled)
>  		return -ENODEV;
>  
> -	if (!id || !handler)
> +	if (!id)
>  		return -EINVAL;
>  
>  	if (!table_size)
> @@ -247,20 +264,27 @@ acpi_parse_entries(char *id, unsigned long 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))
> +		if (max_entries && count >= max_entries)
> +			break;
> +
> +		for (i = 0; i < proc_num; i++) {
> +			if (entry->type != proc[i].id)
> +				continue;
> +			if (!proc->handler || proc[i].handler(entry, table_end))
>  				return -EINVAL;
>  
> -			count++;
> +			proc->count++;
> +			break;
>  		}
> +		if (i != proc_num)
> +			count++;
>  
>  		/*
>  		 * If entry->length is 0, break from this loop to avoid
>  		 * infinite loop.
>  		 */
>  		if (entry->length == 0) {
> -			pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id);
> +			pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, proc->id);
>  			return -EINVAL;
>  		}
>  
> @@ -270,17 +294,31 @@ acpi_parse_entries(char *id, unsigned long table_size,
>  
>  	if (max_entries && count > max_entries) {
>  		pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n",
> -			id, entry_id, count - max_entries, count);
> +			id, proc->id, count - max_entries, count);
>  	}
>  
>  	return count;
>  }
>  
> +int __init acpi_parse_entries(char *id, unsigned long table_size,
> +			       acpi_tbl_entry_handler handler,
> +			       struct acpi_table_header *table_header,
> +			       int entry_id, unsigned int max_entries)
> +{
> +	struct acpi_subtable_proc proc = {
> +		.id		= entry_id,
> +		.handler	= handler,
> +		.count		= 0,

count is implicitly initialized to zero when you have a partial initializer.

> +	};
> +
> +	return acpi_parse_entries_array(id, table_size, table_header,
> +			&proc, 1, max_entries);
> +}
> +
>  int __init
> -acpi_table_parse_entries(char *id,
> +acpi_table_parse_entries_array(char *id,
>  			 unsigned long table_size,
> -			 int entry_id,
> -			 acpi_tbl_entry_handler handler,
> +			 struct acpi_subtable_proc *proc, int proc_num,
>  			 unsigned int max_entries)
>  {
>  	struct acpi_table_header *table_header = NULL;
> @@ -291,7 +329,7 @@ acpi_table_parse_entries(char *id,
>  	if (acpi_disabled)
>  		return -ENODEV;
>  
> -	if (!id || !handler)
> +	if (!id)
>  		return -EINVAL;
>  
>  	if (!strncmp(id, ACPI_SIG_MADT, 4))
> @@ -303,14 +341,31 @@ acpi_table_parse_entries(char *id,
>  		return -ENODEV;
>  	}
>  
> -	count = acpi_parse_entries(id, table_size, handler, table_header,
> -			entry_id, max_entries);
> +	count = acpi_parse_entries_array(id, table_size, table_header,
> +			proc, proc_num, max_entries);
>  
>  	early_acpi_os_unmap_memory((char *)table_header, tbl_size);
>  	return count;
>  }
>  
>  int __init
> +acpi_table_parse_entries(char *id,
> +			unsigned long table_size,
> +			int entry_id,
> +			acpi_tbl_entry_handler handler,
> +			unsigned int max_entries)
> +{
> +	struct acpi_subtable_proc proc = {
> +		.id		= entry_id,
> +		.handler	= handler,
> +		.count		= 0,

Same here.

> +	};
> +
> +	return acpi_table_parse_entries_array(id, table_size, &proc, 1,
> +						max_entries);
> +}
> +
> +int __init
>  acpi_table_parse_madt(enum acpi_madt_type id,
>  		      acpi_tbl_entry_handler handler, unsigned int max_entries)
>  {
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index d2445fa..7a25ef9 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -135,6 +135,12 @@ static inline void acpi_initrd_override(void *data, size_t size)
>  		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
>  		((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
>  
> +struct acpi_subtable_proc {
> +	int id;
> +	acpi_tbl_entry_handler handler;
> +	int count;
> +};
> +
>  char * __acpi_map_table (unsigned long phys_addr, unsigned long size);
>  void __acpi_unmap_table(char *map, unsigned long size);
>  int early_acpi_boot_init(void);
> @@ -149,10 +155,17 @@ int __init acpi_parse_entries(char *id, unsigned long table_size,
>  			      acpi_tbl_entry_handler handler,
>  			      struct acpi_table_header *table_header,
>  			      int entry_id, unsigned int max_entries);
> +int __init acpi_parse_entries_array(char *id, unsigned long table_size,
> +			      struct acpi_table_header *table_header,
> +			      struct acpi_subtable_proc *proc, int proc_num,
> +			      unsigned int max_entries);

and you can drop this one if you make it static in tables.c

>  int __init acpi_table_parse_entries(char *id, unsigned long table_size,
>  				    int entry_id,
>  				    acpi_tbl_entry_handler handler,
>  				    unsigned int max_entries);
> +int __init acpi_table_parse_entries_array(char *id, unsigned long table_size,
> +				    struct acpi_subtable_proc *proc, int proc_num,
> +				    unsigned int max_entries);
>  int acpi_table_parse_madt(enum acpi_madt_type id,
>  			  acpi_tbl_entry_handler handler,
>  			  unsigned int max_entries);
> 

Other than that, I gave it a quick run on arm64, and it did boot without
any observable issue. If you respin it to address the above minor
comments, you can add my:

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Tested-by: Marc Zyngier <marc.zyngier@arm.com>

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...