[PATCH 0/2] two minor ACPI fixes

[PATCH 0/2] two minor ACPI fixes

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>


Shannon Zhao (2):
  ACPI/table: Always count matched and successfully parsed entries
  ACPI/table: Make acpi_table_init return meaningful value

 xen/drivers/acpi/tables.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

-- 
2.0.4

[PATCH 1/2] ACPI/table: Always count matched and successfully parsed entries

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

acpi_parse_entries() allows to traverse all available table entries (aka
subtables) by passing max_entries parameter equal to 0, but since its count
variable is only incremented if max_entries is not 0, the function always
returns 0 for max_entries equal to 0.  It would be more useful if it returned
the number of entries matched instead, so make it increment count in that
case too.

Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 xen/drivers/acpi/tables.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/acpi/tables.c b/xen/drivers/acpi/tables.c
index 1beca79..822689e 100644
--- a/xen/drivers/acpi/tables.c
+++ b/xen/drivers/acpi/tables.c
@@ -240,10 +240,13 @@ acpi_table_parse_entries(char *id,
 		}
 
 		if (entry->type == entry_id
-		    && (!max_entries || count++ < max_entries))
+		    && (!max_entries || count < max_entries)) {
 			if (handler(entry, table_end))
 				return -EINVAL;
 
+			count++;
+		}
+
 		entry = (struct acpi_subtable_header *)
 		    ((unsigned long)entry + entry->length);
 	}
-- 
2.0.4

[PATCH 2/2] ACPI/table: Make acpi_table_init return meaningful value

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

If ACPI fails to initialize tables, it should return one meaningful
value to tell the caller to diable acpi.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 xen/drivers/acpi/tables.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/acpi/tables.c b/xen/drivers/acpi/tables.c
index 822689e..1594e3b 100644
--- a/xen/drivers/acpi/tables.c
+++ b/xen/drivers/acpi/tables.c
@@ -330,7 +330,11 @@ static void __init check_multiple_madt(void)
 
 int __init acpi_table_init(void)
 {
-	acpi_initialize_tables(NULL, ACPI_MAX_TABLES, 0);
+	acpi_status status;
+
+	status = acpi_initialize_tables(NULL, ACPI_MAX_TABLES, 0);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
 	check_multiple_madt();
 	return 0;
 }
-- 
2.0.4

Re: [PATCH 1/2] ACPI/table: Always count matched and successfully parsed entries

[Jan Beulich]

>>> On 04.08.15 at 06:19, <zhaoshenglong@huawei.com> wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>

This is not true. All you did is port a Linux change.

> acpi_parse_entries() allows to traverse all available table entries (aka
> subtables) by passing max_entries parameter equal to 0, but since its count
> variable is only incremented if max_entries is not 0, the function always
> returns 0 for max_entries equal to 0.  It would be more useful if it returned
> the number of entries matched instead, so make it increment count in that
> case too.
> 
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>

Please mention the respective Linux commit. Without that I would
complain that "more useful" is too fuzzy (as in not making clear to
whom and why).

Jan

Re: [PATCH 2/2] ACPI/table: Make acpi_table_init return meaningful value

[Jan Beulich]

>>> On 04.08.15 at 06:19, <zhaoshenglong@huawei.com> wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> If ACPI fails to initialize tables, it should return one meaningful
> value to tell the caller to diable acpi.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  xen/drivers/acpi/tables.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/acpi/tables.c b/xen/drivers/acpi/tables.c
> index 822689e..1594e3b 100644
> --- a/xen/drivers/acpi/tables.c
> +++ b/xen/drivers/acpi/tables.c
> @@ -330,7 +330,11 @@ static void __init check_multiple_madt(void)
>  
>  int __init acpi_table_init(void)
>  {
> -	acpi_initialize_tables(NULL, ACPI_MAX_TABLES, 0);
> +	acpi_status status;
> +
> +	status = acpi_initialize_tables(NULL, ACPI_MAX_TABLES, 0);
> +	if (ACPI_FAILURE(status))
> +		return -EINVAL;

While this makes it look like you didn't directly clone Linux commit
9e3a9d1ed8, it still very much resembles that one (with 95df812dbd
on top) and hence I'd prefer if you said so (and used the same titles
and descriptions) to ease identifying their origin.

Jan

>  	check_multiple_madt();
>  	return 0;
>  }
> -- 
> 2.0.4

Re: [PATCH 1/2] ACPI/table: Always count matched and successfully parsed entries

[Shannon Zhao]

On 2015/8/12 16:51, Jan Beulich wrote:
>>>> On 04.08.15 at 06:19, <zhaoshenglong@huawei.com> wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> This is not true. All you did is port a Linux change.
> 
Will change this.

>> acpi_parse_entries() allows to traverse all available table entries (aka
>> subtables) by passing max_entries parameter equal to 0, but since its count
>> variable is only incremented if max_entries is not 0, the function always
>> returns 0 for max_entries equal to 0.  It would be more useful if it returned
>> the number of entries matched instead, so make it increment count in that
>> case too.
>>
>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Please mention the respective Linux commit. Without that I would
> complain that "more useful" is too fuzzy (as in not making clear to
> whom and why).
> 
Ok, will add it. Thanks.

-- 
Shannon