[RFC] ACPI: Add new function to get table entries

[RFC] ACPI: Add new function to get table entries

[Ashwin Chaugule]

The acpi_table_parse() function has a callback that
passes a pointer to a table_header. Add a new function
which takes this pointer and parses its entries. This
eliminates the need to re-traverse all the tables for
each call. e.g. as in acpi_table_parse_madt() which is
normally called after acpi_table_parse().

Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
---
 drivers/acpi/tables.c | 69 +++++++++++++++++++++++++++++++++++----------------
 include/linux/acpi.h  |  4 +++
 2 files changed, 51 insertions(+), 22 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 5837f85..b7bd6a3 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -199,19 +199,15 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 	}
 }
 
-
 int __init
-acpi_table_parse_entries(char *id,
-			     unsigned long table_size,
-			     int entry_id,
-			     acpi_tbl_entry_handler handler,
-			     unsigned int max_entries)
+acpi_parse_entries(unsigned long table_size,
+		acpi_tbl_entry_handler handler,
+		struct acpi_table_header *table_header,
+		int entry_id, unsigned int max_entries)
 {
-	struct acpi_table_header *table_header = NULL;
 	struct acpi_subtable_header *entry;
 	unsigned int count = 0;
 	unsigned long table_end;
-	acpi_size tbl_size;
 
 	if (acpi_disabled)
 		return -ENODEV;
@@ -219,16 +215,14 @@ acpi_table_parse_entries(char *id,
 	if (!handler)
 		return -EINVAL;
 
-	if (strncmp(id, ACPI_SIG_MADT, 4) == 0)
-		acpi_get_table_with_size(id, acpi_apic_instance, &table_header, &tbl_size);
-	else
-		acpi_get_table_with_size(id, 0, &table_header, &tbl_size);
-
 	if (!table_header) {
-		printk(KERN_WARNING PREFIX "%4.4s not present\n", id);
+		pr_warn(PREFIX "Table header not present\n");
 		return -ENODEV;
 	}
 
+	if (!table_size)
+		table_size = table_header->length;
+
 	table_end = (unsigned long)table_header + table_header->length;
 
 	/* Parse all entries looking for a match. */
@@ -240,15 +234,18 @@ acpi_table_parse_entries(char *id,
 	       table_end) {
 		if (entry->type == entry_id
 		    && (!max_entries || count++ < max_entries))
-			if (handler(entry, table_end))
+			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) {
-			pr_err(PREFIX "[%4.4s:0x%02x] Invalid zero length\n", id, entry_id);
+			pr_err(PREFIX "[0x%02x] Invalid zero length\n", entry_id);
+			count = -EINVAL;
 			goto err;
 		}
 
@@ -256,15 +253,43 @@ acpi_table_parse_entries(char *id,
 		    ((unsigned long)entry + entry->length);
 	}
 	if (max_entries && count > max_entries) {
-		printk(KERN_WARNING PREFIX "[%4.4s:0x%02x] ignored %i entries of "
-		       "%i found\n", id, entry_id, count - max_entries, count);
+		printk(KERN_WARNING PREFIX "[0x%02x] ignored %i entries of "
+		       "%i found\n", entry_id, count - max_entries, count);
 	}
 
-	early_acpi_os_unmap_memory((char *)table_header, tbl_size);
-	return count;
 err:
-	early_acpi_os_unmap_memory((char *)table_header, tbl_size);
-	return -EINVAL;
+	early_acpi_os_unmap_memory((char *)table_header, (acpi_size)table_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_table_header *table_header = NULL;
+	acpi_size tbl_size;
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	if (!handler)
+		return -EINVAL;
+
+	if (strncmp(id, ACPI_SIG_MADT, 4) == 0)
+		acpi_get_table_with_size(id, acpi_apic_instance, &table_header, &tbl_size);
+	else
+		acpi_get_table_with_size(id, 0, &table_header, &tbl_size);
+
+	if (!table_header) {
+		pr_warn("%4.4s not present\n", id);
+		return -ENODEV;
+	}
+
+	return acpi_parse_entries(table_header->length, handler, table_header,
+			entry_id, max_entries);
 }
 
 int __init
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 1151a1d..da959fd 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -118,6 +118,10 @@ int acpi_numa_init (void);
 
 int acpi_table_init (void);
 int acpi_table_parse(char *id, acpi_tbl_table_handler handler);
+int __init acpi_parse_entries(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_table_parse_entries(char *id, unsigned long table_size,
 				    int entry_id,
 				    acpi_tbl_entry_handler handler,
-- 
1.8.3.2

Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries

[Hanjun Guo]

Hi Ashwin,

On 2014-3-20 7:09, Ashwin Chaugule wrote:
> The acpi_table_parse() function has a callback that
> passes a pointer to a table_header. Add a new function
> which takes this pointer and parses its entries. This
> eliminates the need to re-traverse all the tables for
> each call. e.g. as in acpi_table_parse_madt() which is
> normally called after acpi_table_parse().
> 
> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> ---
>  drivers/acpi/tables.c | 69 +++++++++++++++++++++++++++++++++++----------------
>  include/linux/acpi.h  |  4 +++
>  2 files changed, 51 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 5837f85..b7bd6a3 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -199,19 +199,15 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>  	}
>  }
>  
> -
>  int __init
> -acpi_table_parse_entries(char *id,
> -			     unsigned long table_size,
> -			     int entry_id,
> -			     acpi_tbl_entry_handler handler,
> -			     unsigned int max_entries)
> +acpi_parse_entries(unsigned long table_size,
> +		acpi_tbl_entry_handler handler,
> +		struct acpi_table_header *table_header,
> +		int entry_id, unsigned int max_entries)
>  {
> -	struct acpi_table_header *table_header = NULL;
>  	struct acpi_subtable_header *entry;
>  	unsigned int count = 0;
>  	unsigned long table_end;
> -	acpi_size tbl_size;
>  
>  	if (acpi_disabled)
>  		return -ENODEV;
> @@ -219,16 +215,14 @@ acpi_table_parse_entries(char *id,
>  	if (!handler)
>  		return -EINVAL;
>  
> -	if (strncmp(id, ACPI_SIG_MADT, 4) == 0)
> -		acpi_get_table_with_size(id, acpi_apic_instance, &table_header, &tbl_size);
> -	else
> -		acpi_get_table_with_size(id, 0, &table_header, &tbl_size);
> -
>  	if (!table_header) {
> -		printk(KERN_WARNING PREFIX "%4.4s not present\n", id);
> +		pr_warn(PREFIX "Table header not present\n");

Can you rebase this patch on the branch of linux-next of linux-pm tree?
PREFIX was removed by me and I used pr_fmt() instead.

Thanks
Hanjun

Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries

[Ashwin Chaugule]

On 20 March 2014 04:25, Hanjun Guo <hanjun.guo@linaro.org> wrote:

>
> Can you rebase this patch on the branch of linux-next of linux-pm tree?
> PREFIX was removed by me and I used pr_fmt() instead.


Will do.

Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries

[Tomasz Nowicki]

On 20.03.2014 00:09, Ashwin Chaugule wrote:
> The acpi_table_parse() function has a callback that
> passes a pointer to a table_header. Add a new function
> which takes this pointer and parses its entries. This
> eliminates the need to re-traverse all the tables for
> each call. e.g. as in acpi_table_parse_madt() which is
> normally called after acpi_table_parse().
>
> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> ---
>   drivers/acpi/tables.c | 69 +++++++++++++++++++++++++++++++++++----------------
>   include/linux/acpi.h  |  4 +++
>   2 files changed, 51 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 5837f85..b7bd6a3 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -199,19 +199,15 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>   	}
>   }
>
> -
>   int __init
> -acpi_table_parse_entries(char *id,
> -			     unsigned long table_size,
> -			     int entry_id,
> -			     acpi_tbl_entry_handler handler,
> -			     unsigned int max_entries)
> +acpi_parse_entries(unsigned long table_size,
> +		acpi_tbl_entry_handler handler,
> +		struct acpi_table_header *table_header,
> +		int entry_id, unsigned int max_entries)
>   {
> -	struct acpi_table_header *table_header = NULL;
>   	struct acpi_subtable_header *entry;
>   	unsigned int count = 0;
>   	unsigned long table_end;
> -	acpi_size tbl_size;
>
>   	if (acpi_disabled)
>   		return -ENODEV;
> @@ -219,16 +215,14 @@ acpi_table_parse_entries(char *id,
>   	if (!handler)
>   		return -EINVAL;
>
> -	if (strncmp(id, ACPI_SIG_MADT, 4) == 0)
> -		acpi_get_table_with_size(id, acpi_apic_instance, &table_header, &tbl_size);
> -	else
> -		acpi_get_table_with_size(id, 0, &table_header, &tbl_size);
> -
>   	if (!table_header) {
> -		printk(KERN_WARNING PREFIX "%4.4s not present\n", id);
> +		pr_warn(PREFIX "Table header not present\n");
>   		return -ENODEV;
>   	}
>
> +	if (!table_size)
> +		table_size = table_header->length;
> +

If table_size == 0, that would causes 'entry' and 'table_end' are equal 
and no entries at all. For that case I would suggest to print error and 
return.

Tomasz

>   	table_end = (unsigned long)table_header + table_header->length;
>
>   	/* Parse all entries looking for a match. */
> @@ -240,15 +234,18 @@ acpi_table_parse_entries(char *id,
>   	       table_end) {
>   		if (entry->type == entry_id
>   		    && (!max_entries || count++ < max_entries))
> -			if (handler(entry, table_end))
> +			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) {
> -			pr_err(PREFIX "[%4.4s:0x%02x] Invalid zero length\n", id, entry_id);
> +			pr_err(PREFIX "[0x%02x] Invalid zero length\n", entry_id);
> +			count = -EINVAL;
>   			goto err;
>   		}
>
> @@ -256,15 +253,43 @@ acpi_table_parse_entries(char *id,
>   		    ((unsigned long)entry + entry->length);
>   	}
>   	if (max_entries && count > max_entries) {
> -		printk(KERN_WARNING PREFIX "[%4.4s:0x%02x] ignored %i entries of "
> -		       "%i found\n", id, entry_id, count - max_entries, count);
> +		printk(KERN_WARNING PREFIX "[0x%02x] ignored %i entries of "
> +		       "%i found\n", entry_id, count - max_entries, count);
>   	}
>
> -	early_acpi_os_unmap_memory((char *)table_header, tbl_size);
> -	return count;
>   err:
> -	early_acpi_os_unmap_memory((char *)table_header, tbl_size);
> -	return -EINVAL;
> +	early_acpi_os_unmap_memory((char *)table_header, (acpi_size)table_size);
> +	return count;

Changelog is saying that acpi_parse_entries() takes already mapped table 
and parses entries. IMO, caller should takes care about unmapping table. 
It can be still parsed using different ID later on.

Tomasz

> +}
> +
> +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_table_header *table_header = NULL;
> +	acpi_size tbl_size;
> +
> +	if (acpi_disabled)
> +		return -ENODEV;
> +
> +	if (!handler)
> +		return -EINVAL;
> +
> +	if (strncmp(id, ACPI_SIG_MADT, 4) == 0)
> +		acpi_get_table_with_size(id, acpi_apic_instance, &table_header, &tbl_size);
> +	else
> +		acpi_get_table_with_size(id, 0, &table_header, &tbl_size);
> +
> +	if (!table_header) {
> +		pr_warn("%4.4s not present\n", id);
> +		return -ENODEV;
> +	}
> +
> +	return acpi_parse_entries(table_header->length, handler, table_header,
> +			entry_id, max_entries);
>   }
>
>   int __init
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 1151a1d..da959fd 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -118,6 +118,10 @@ int acpi_numa_init (void);
>
>   int acpi_table_init (void);
>   int acpi_table_parse(char *id, acpi_tbl_table_handler handler);
> +int __init acpi_parse_entries(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_table_parse_entries(char *id, unsigned long table_size,
>   				    int entry_id,
>   				    acpi_tbl_entry_handler handler,
>

Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries

[Tomasz Nowicki]

On 20.03.2014 11:05, Tomasz Nowicki wrote:
>
>
> On 20.03.2014 00:09, Ashwin Chaugule wrote:
>> The acpi_table_parse() function has a callback that
>> passes a pointer to a table_header. Add a new function
>> which takes this pointer and parses its entries. This
>> eliminates the need to re-traverse all the tables for
>> each call. e.g. as in acpi_table_parse_madt() which is
>> normally called after acpi_table_parse().
>>
>> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
>> ---
>>   drivers/acpi/tables.c | 69
>> +++++++++++++++++++++++++++++++++++----------------
>>   include/linux/acpi.h  |  4 +++
>>   2 files changed, 51 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
>> index 5837f85..b7bd6a3 100644
>> --- a/drivers/acpi/tables.c
>> +++ b/drivers/acpi/tables.c
>> @@ -199,19 +199,15 @@ void acpi_table_print_madt_entry(struct
>> acpi_subtable_header *header)
>>       }
>>   }
>>
>> -
>>   int __init
>> -acpi_table_parse_entries(char *id,
>> -                 unsigned long table_size,
>> -                 int entry_id,
>> -                 acpi_tbl_entry_handler handler,
>> -                 unsigned int max_entries)
>> +acpi_parse_entries(unsigned long table_size,
>> +        acpi_tbl_entry_handler handler,
>> +        struct acpi_table_header *table_header,
>> +        int entry_id, unsigned int max_entries)
>>   {
>> -    struct acpi_table_header *table_header = NULL;
>>       struct acpi_subtable_header *entry;
>>       unsigned int count = 0;
>>       unsigned long table_end;
>> -    acpi_size tbl_size;
>>
>>       if (acpi_disabled)
>>           return -ENODEV;
>> @@ -219,16 +215,14 @@ acpi_table_parse_entries(char *id,
>>       if (!handler)
>>           return -EINVAL;
>>
>> -    if (strncmp(id, ACPI_SIG_MADT, 4) == 0)
>> -        acpi_get_table_with_size(id, acpi_apic_instance,
>> &table_header, &tbl_size);
>> -    else
>> -        acpi_get_table_with_size(id, 0, &table_header, &tbl_size);
>> -
>>       if (!table_header) {
>> -        printk(KERN_WARNING PREFIX "%4.4s not present\n", id);
>> +        pr_warn(PREFIX "Table header not present\n");
>>           return -ENODEV;
>>       }
>>
>> +    if (!table_size)
>> +        table_size = table_header->length;
>> +
>
> If table_size == 0, that would causes 'entry' and 'table_end' are equal
> and no entries at all. For that case I would suggest to print error and
> return.
>
> Tomasz
>
>>       table_end = (unsigned long)table_header + table_header->length;
>>
>>       /* Parse all entries looking for a match. */
>> @@ -240,15 +234,18 @@ acpi_table_parse_entries(char *id,
>>              table_end) {
>>           if (entry->type == entry_id
>>               && (!max_entries || count++ < max_entries))
>> -            if (handler(entry, table_end))
>> +            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) {
>> -            pr_err(PREFIX "[%4.4s:0x%02x] Invalid zero length\n", id,
>> entry_id);
>> +            pr_err(PREFIX "[0x%02x] Invalid zero length\n", entry_id);
>> +            count = -EINVAL;
>>               goto err;
>>           }
>>
>> @@ -256,15 +253,43 @@ acpi_table_parse_entries(char *id,
>>               ((unsigned long)entry + entry->length);
>>       }
>>       if (max_entries && count > max_entries) {
>> -        printk(KERN_WARNING PREFIX "[%4.4s:0x%02x] ignored %i entries
>> of "
>> -               "%i found\n", id, entry_id, count - max_entries, count);
>> +        printk(KERN_WARNING PREFIX "[0x%02x] ignored %i entries of "
>> +               "%i found\n", entry_id, count - max_entries, count);
>>       }
>>
>> -    early_acpi_os_unmap_memory((char *)table_header, tbl_size);
>> -    return count;
>>   err:
>> -    early_acpi_os_unmap_memory((char *)table_header, tbl_size);
>> -    return -EINVAL;
>> +    early_acpi_os_unmap_memory((char *)table_header,
>> (acpi_size)table_size);
>> +    return count;
>
> Changelog is saying that acpi_parse_entries() takes already mapped table
> and parses entries. IMO, caller should takes care about unmapping table.
> It can be still parsed using different ID later on.
>
> Tomasz
>
>> +}
>> +
>> +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_table_header *table_header = NULL;
>> +    acpi_size tbl_size;
>> +
>> +    if (acpi_disabled)
>> +        return -ENODEV;
>> +
>> +    if (!handler)
>> +        return -EINVAL;
>> +
>> +    if (strncmp(id, ACPI_SIG_MADT, 4) == 0)
>> +        acpi_get_table_with_size(id, acpi_apic_instance,
>> &table_header, &tbl_size);
>> +    else
>> +        acpi_get_table_with_size(id, 0, &table_header, &tbl_size);
>> +
>> +    if (!table_header) {
>> +        pr_warn("%4.4s not present\n", id);
>> +        return -ENODEV;
>> +    }
>> +
>> +    return acpi_parse_entries(table_header->length, handler,
>> table_header,
>> +            entry_id, max_entries);
Also, first argument of acpi_parse_entries() expect subtable start point 
offset, but now it points to the end of table.

Tomasz
>>   }
>>
>>   int __init
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 1151a1d..da959fd 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -118,6 +118,10 @@ int acpi_numa_init (void);
>>
>>   int acpi_table_init (void);
>>   int acpi_table_parse(char *id, acpi_tbl_table_handler handler);
>> +int __init acpi_parse_entries(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_table_parse_entries(char *id, unsigned long table_size,
>>                       int entry_id,
>>                       acpi_tbl_entry_handler handler,
>>

Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries

[Ashwin Chaugule]

On 20 March 2014 06:16, Tomasz Nowicki <tomasz.nowicki@linaro.org> wrote:

>>> +    return acpi_parse_entries(table_header->length, handler,
>>> table_header,
>>> +            entry_id, max_entries);
>
> Also, first argument of acpi_parse_entries() expect subtable start point
> offset, but now it points to the end of table.
>


Not sure what you mean. First arg of acpi_parse_entries expects
table_size, not an offset.

Ashwin

Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries

[Tomasz Nowicki]

On 20.03.2014 14:27, Ashwin Chaugule wrote:
> On 20 March 2014 06:16, Tomasz Nowicki <tomasz.nowicki@linaro.org> wrote:
>
>>>> +    return acpi_parse_entries(table_header->length, handler,
>>>> table_header,
>>>> +            entry_id, max_entries);
>>
>> Also, first argument of acpi_parse_entries() expect subtable start point
>> offset, but now it points to the end of table.
>>
>
>
> Not sure what you mean. First arg of acpi_parse_entries expects
> table_size, not an offset.

As I mentioned in previous mail, first args of acpi_parse_entries will 
lead us to first MADT entry list. By passing table_header->length we 
will get at the end of MADT entry list. IMO, we should use table_size 
(snd argument of acpi_table_parse_entries()) instead of 
table_header->length.

Tomasz

Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries

[Ashwin Chaugule]

On 20 March 2014 10:06, Tomasz Nowicki <tomasz.nowicki@linaro.org> wrote:
> On 20.03.2014 14:27, Ashwin Chaugule wrote:
>>
>> On 20 March 2014 06:16, Tomasz Nowicki <tomasz.nowicki@linaro.org> wrote:
>>
>>>>> +    return acpi_parse_entries(table_header->length, handler,
>>>>> table_header,
>>>>> +            entry_id, max_entries);
>>>
>>>
>>> Also, first argument of acpi_parse_entries() expect subtable start point
>>> offset, but now it points to the end of table.
>>>
>>
>>
>> Not sure what you mean. First arg of acpi_parse_entries expects
>> table_size, not an offset.
>
>
> As I mentioned in previous mail, first args of acpi_parse_entries will lead
> us to first MADT entry list. By passing table_header->length we will get at
> the end of MADT entry list. IMO, we should use table_size (snd argument of
> acpi_table_parse_entries()) instead of table_header->length.
>

Right. I get the difference between tbl_size and table_header->length.
But we need to rethink this apci_parse_entries() function and its use
case again. If the caller is expected to pass tbl_size, then we're
back full circle to using acpi_get_table_with_size() in the caller,
which is kinda self defeating. :)

Ashwin

Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries

[Tomasz Nowicki]

On 20.03.2014 15:39, Ashwin Chaugule wrote:
> On 20 March 2014 10:06, Tomasz Nowicki <tomasz.nowicki@linaro.org> wrote:
>> On 20.03.2014 14:27, Ashwin Chaugule wrote:
>>>
>>> On 20 March 2014 06:16, Tomasz Nowicki <tomasz.nowicki@linaro.org> wrote:
>>>
>>>>>> +    return acpi_parse_entries(table_header->length, handler,
>>>>>> table_header,
>>>>>> +            entry_id, max_entries);
>>>>
>>>>
>>>> Also, first argument of acpi_parse_entries() expect subtable start point
>>>> offset, but now it points to the end of table.
>>>>
>>>
>>>
>>> Not sure what you mean. First arg of acpi_parse_entries expects
>>> table_size, not an offset.
>>
>>
>> As I mentioned in previous mail, first args of acpi_parse_entries will lead
>> us to first MADT entry list. By passing table_header->length we will get at
>> the end of MADT entry list. IMO, we should use table_size (snd argument of
>> acpi_table_parse_entries()) instead of table_header->length.
>>
>
> Right. I get the difference between tbl_size and table_header->length.
> But we need to rethink this apci_parse_entries() function and its use
> case again. If the caller is expected to pass tbl_size, then we're
> back full circle to using acpi_get_table_with_size() in the caller,
> which is kinda self defeating. :)

I believe that the most useful case for apci_parse_entries() is when you 
already get table pointer and want to do many things on entries. Then 
you don't have to traverse all ACPI tables list and do map/unmap dance 
over and over again :) and that's what your changelog is saying too :)

Tomasz

Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries

[Ashwin Chaugule]

Hi Tomasz,

On 20 March 2014 06:05, Tomasz Nowicki <tomasz.nowicki@linaro.org> wrote:

>> +       if (!table_size)
>> +               table_size = table_header->length;
>> +
>
>
> If table_size == 0, that would causes 'entry' and 'table_end' are equal and
> no entries at all. For that case I would suggest to print error and return.
>

Hm. tbl_size and table_header->length are not the same. This needs to
be revisited.


>
> Changelog is saying that acpi_parse_entries() takes already mapped table and
> parses entries. IMO, caller should takes care about unmapping table. It can
> be still parsed using different ID later on.
>

Although I generally agree that mapping/umapping should be done in the
caller, it seems umapping in this manner is baked in APCI core code
and I didn't want to change too many things at once without causing
unintentional side effects elsewhere.


Thanks,
Ashwin

Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries

[Tomasz Nowicki]

On 20.03.2014 14:25, Ashwin Chaugule wrote:
> Hi Tomasz,
>
> On 20 March 2014 06:05, Tomasz Nowicki <tomasz.nowicki@linaro.org> wrote:
>
>>> +       if (!table_size)
>>> +               table_size = table_header->length;
>>> +
>>
>>
>> If table_size == 0, that would causes 'entry' and 'table_end' are equal and
>> no entries at all. For that case I would suggest to print error and return.
>>
>
> Hm. tbl_size and table_header->length are not the same. This needs to
> be revisited.
Indeed, table_size is used to find out where entry list starts.
table_header->length in turn, is used to find out end of entry list. If 
we pass table_size = 0, then we will parse none of entries.
>
>
>>
>> Changelog is saying that acpi_parse_entries() takes already mapped table and
>> parses entries. IMO, caller should takes care about unmapping table. It can
>> be still parsed using different ID later on.
>>
>
> Although I generally agree that mapping/umapping should be done in the
> caller, it seems umapping in this manner is baked in APCI core code
> and I didn't want to change too many things at once without causing
> unintentional side effects elsewhere.

You will not cause any side effects if unmapping will stay in 
acpi_table_parse_entries().

Tomasz

Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries

[Ashwin Chaugule]

On 20 March 2014 09:48, Tomasz Nowicki <tomasz.nowicki@linaro.org> wrote:

>>
>> Although I generally agree that mapping/umapping should be done in the
>> caller, it seems umapping in this manner is baked in APCI core code
>> and I didn't want to change too many things at once without causing
>> unintentional side effects elsewhere.
>
>
> You will not cause any side effects if unmapping will stay in
> acpi_table_parse_entries().

What if callers only call acpi_parse_entries()?

Ashwin

Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries

[Tomasz Nowicki]

On 20.03.2014 14:53, Ashwin Chaugule wrote:
> On 20 March 2014 09:48, Tomasz Nowicki <tomasz.nowicki@linaro.org> wrote:
>
>>>
>>> Although I generally agree that mapping/umapping should be done in the
>>> caller, it seems umapping in this manner is baked in APCI core code
>>> and I didn't want to change too many things at once without causing
>>> unintentional side effects elsewhere.
>>
>>
>> You will not cause any side effects if unmapping will stay in
>> acpi_table_parse_entries().
>
> What if callers only call acpi_parse_entries()?

acpi_parse_entries() requires already mapped MADT table and as you 
agreed on caller mapping/unmapping responsibility, caller need to unmap 
MADT table after all. I mean sth like that:

1. Get MADT table pointer
2. Map MADT table
3. Use acpi_parse_entries() as many times as you wish (some calls can 
return error but you don't have to map MADT again)
4. Unmap MADT table

What do you think?

Tomasz