* [RFC] ACPI: Add new function to get table entries
@ 2014-03-19 23:09 Ashwin Chaugule
2014-03-20 8:25 ` [Linaro-acpi] " Hanjun Guo
2014-03-20 10:05 ` Tomasz Nowicki
0 siblings, 2 replies; 13+ messages in thread
From: Ashwin Chaugule @ 2014-03-19 23:09 UTC (permalink / raw)
To: linaro-acpi; +Cc: rjw, lenb, linux-acpi, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries
2014-03-19 23:09 [RFC] ACPI: Add new function to get table entries Ashwin Chaugule
@ 2014-03-20 8:25 ` Hanjun Guo
2014-03-20 13:17 ` Ashwin Chaugule
2014-03-20 10:05 ` Tomasz Nowicki
1 sibling, 1 reply; 13+ messages in thread
From: Hanjun Guo @ 2014-03-20 8:25 UTC (permalink / raw)
To: Ashwin Chaugule; +Cc: linaro-acpi, linux-acpi, rjw, lenb
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries
2014-03-19 23:09 [RFC] ACPI: Add new function to get table entries Ashwin Chaugule
2014-03-20 8:25 ` [Linaro-acpi] " Hanjun Guo
@ 2014-03-20 10:05 ` Tomasz Nowicki
2014-03-20 10:16 ` Tomasz Nowicki
2014-03-20 13:25 ` Ashwin Chaugule
1 sibling, 2 replies; 13+ messages in thread
From: Tomasz Nowicki @ 2014-03-20 10:05 UTC (permalink / raw)
To: Ashwin Chaugule, linaro-acpi; +Cc: linux-acpi, rjw, lenb
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,
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries
2014-03-20 10:05 ` Tomasz Nowicki
@ 2014-03-20 10:16 ` Tomasz Nowicki
2014-03-20 13:27 ` Ashwin Chaugule
2014-03-20 13:25 ` Ashwin Chaugule
1 sibling, 1 reply; 13+ messages in thread
From: Tomasz Nowicki @ 2014-03-20 10:16 UTC (permalink / raw)
To: Ashwin Chaugule, linaro-acpi; +Cc: linux-acpi, rjw, lenb
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,
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries
2014-03-20 8:25 ` [Linaro-acpi] " Hanjun Guo
@ 2014-03-20 13:17 ` Ashwin Chaugule
0 siblings, 0 replies; 13+ messages in thread
From: Ashwin Chaugule @ 2014-03-20 13:17 UTC (permalink / raw)
To: Hanjun Guo; +Cc: linaro-acpi, linux-acpi, rjw, lenb
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries
2014-03-20 10:05 ` Tomasz Nowicki
2014-03-20 10:16 ` Tomasz Nowicki
@ 2014-03-20 13:25 ` Ashwin Chaugule
2014-03-20 13:48 ` Tomasz Nowicki
1 sibling, 1 reply; 13+ messages in thread
From: Ashwin Chaugule @ 2014-03-20 13:25 UTC (permalink / raw)
To: Tomasz Nowicki; +Cc: linaro-acpi, linux-acpi, rjw, Len Brown
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries
2014-03-20 10:16 ` Tomasz Nowicki
@ 2014-03-20 13:27 ` Ashwin Chaugule
2014-03-20 14:06 ` Tomasz Nowicki
0 siblings, 1 reply; 13+ messages in thread
From: Ashwin Chaugule @ 2014-03-20 13:27 UTC (permalink / raw)
To: Tomasz Nowicki; +Cc: linaro-acpi, linux-acpi, rjw, Len Brown
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries
2014-03-20 13:25 ` Ashwin Chaugule
@ 2014-03-20 13:48 ` Tomasz Nowicki
2014-03-20 13:53 ` Ashwin Chaugule
0 siblings, 1 reply; 13+ messages in thread
From: Tomasz Nowicki @ 2014-03-20 13:48 UTC (permalink / raw)
To: Ashwin Chaugule; +Cc: linaro-acpi, linux-acpi, rjw, Len Brown
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries
2014-03-20 13:48 ` Tomasz Nowicki
@ 2014-03-20 13:53 ` Ashwin Chaugule
2014-03-20 14:06 ` Tomasz Nowicki
0 siblings, 1 reply; 13+ messages in thread
From: Ashwin Chaugule @ 2014-03-20 13:53 UTC (permalink / raw)
To: Tomasz Nowicki; +Cc: linaro-acpi, linux-acpi, rjw, Len Brown
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries
2014-03-20 13:53 ` Ashwin Chaugule
@ 2014-03-20 14:06 ` Tomasz Nowicki
0 siblings, 0 replies; 13+ messages in thread
From: Tomasz Nowicki @ 2014-03-20 14:06 UTC (permalink / raw)
To: Ashwin Chaugule; +Cc: linaro-acpi, linux-acpi, rjw, Len Brown
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries
2014-03-20 13:27 ` Ashwin Chaugule
@ 2014-03-20 14:06 ` Tomasz Nowicki
2014-03-20 14:39 ` Ashwin Chaugule
0 siblings, 1 reply; 13+ messages in thread
From: Tomasz Nowicki @ 2014-03-20 14:06 UTC (permalink / raw)
To: Ashwin Chaugule; +Cc: linaro-acpi, linux-acpi, rjw, Len Brown
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries
2014-03-20 14:06 ` Tomasz Nowicki
@ 2014-03-20 14:39 ` Ashwin Chaugule
2014-03-20 14:59 ` Tomasz Nowicki
0 siblings, 1 reply; 13+ messages in thread
From: Ashwin Chaugule @ 2014-03-20 14:39 UTC (permalink / raw)
To: Tomasz Nowicki; +Cc: linaro-acpi, linux-acpi, rjw, Len Brown
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Linaro-acpi] [RFC] ACPI: Add new function to get table entries
2014-03-20 14:39 ` Ashwin Chaugule
@ 2014-03-20 14:59 ` Tomasz Nowicki
0 siblings, 0 replies; 13+ messages in thread
From: Tomasz Nowicki @ 2014-03-20 14:59 UTC (permalink / raw)
To: Ashwin Chaugule; +Cc: linaro-acpi, linux-acpi, rjw, Len Brown
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-03-20 14:59 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-19 23:09 [RFC] ACPI: Add new function to get table entries Ashwin Chaugule
2014-03-20 8:25 ` [Linaro-acpi] " Hanjun Guo
2014-03-20 13:17 ` Ashwin Chaugule
2014-03-20 10:05 ` Tomasz Nowicki
2014-03-20 10:16 ` Tomasz Nowicki
2014-03-20 13:27 ` Ashwin Chaugule
2014-03-20 14:06 ` Tomasz Nowicki
2014-03-20 14:39 ` Ashwin Chaugule
2014-03-20 14:59 ` Tomasz Nowicki
2014-03-20 13:25 ` Ashwin Chaugule
2014-03-20 13:48 ` Tomasz Nowicki
2014-03-20 13:53 ` Ashwin Chaugule
2014-03-20 14:06 ` Tomasz Nowicki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.