[PATCH 0/3] Correct errors in acpi_parse_entries_array()

[PATCH 0/3] Correct errors in acpi_parse_entries_array()

[Al Stone]

In examining the function acpi_parse_entries_array() for use in another
patch series, I realized the commentary and the code do not agree.
Assuming the commentary is correct, this function will do as I need;
that is not, however, what it does.

The first patch fixes an error where only the very first count element
of a struct in an array of structs gets incremented.  My understanding
of the comments is that the count element for each struct in the array
should be incremented instead.  With the patch, it will.

The second patch causes the function to actually traverse all subtables
just like the comments indicate.  Previously, on any sort of error the
loop would terminate completely, but I really do want to visit all of
the entries even if some need to be ignored.

The final patch corrects the printout when more entries are found than
can actually be handled.  The original code would incorrectly count the
number of entries ignored since it would alway stop the traversal when
the limit on entries was reached, regardless of whether there were
additional entries that still had not been examined.

Looking at the direct and indirect users of acpi_parse_entries_array(),
they all appear to depend on the return value from the function being
either > 0 or < 0, and not a specific value.  Hence, the changes here
have no effect on them.

However, the use case I have in mind traverses the MADT in order to
count not only subtables of a specific type, but also subtables of a
specific type that contain specific values.  With these fixes, I can
make one call to acpi_parse_entries_array() using very small callback
functions and do what I need, and get the correct results.

Tested on arm64 and x86_64, with simple booting, comparison of boot
logs, and daily use over the last couple of days.


Al Stone (3):
  ACPI: fix incorrect counts returned by acpi_parse_entries_array()
  ACPI: fix acpi_parse_entries_array() so it traverses all subtables
  ACPI: fix acpi_parse_entries_array() so it reports overflow correctly

 drivers/acpi/tables.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH 1/3] ACPI: fix incorrect counts returned by acpi_parse_entries_array()

[Al Stone]

The static function acpi_parse_entries_array() is provided an array of
type struct acpi_subtable_proc that has a callback function and a count.
The count should reflect how many times the callback has been successfully
called.  However, the current code only increments the 0th element of the
array, regardless of the number of entries in the array, or which callback
has been invoked.  The fix is to use the index into the array, instead of
a pointer to the beginning of the array.

Signed-off-by: Al Stone <ahs3@redhat.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
---
 drivers/acpi/tables.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 9f0ad6e..3e167b4 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -281,7 +281,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
 			     proc[i].handler(entry, table_end))
 				return -EINVAL;
 
-			proc->count++;
+			proc[i].count++;
 			break;
 		}
 		if (i != proc_num)
@@ -416,7 +416,7 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler)
 		return -ENODEV;
 }
 
-/* 
+/*
  * The BIOS is supposed to supply a single APIC/MADT,
  * but some report two.  Provide a knob to use either.
  * (don't you wish instance 0 and 1 were not the same?)
-- 
2.7.4

[PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables

[Al Stone]

Without this patch, the acpi_parse_entries_array() function will return
the very first time there is any error found in either the array of
callback functions or if one of the callbacks returns an non-zero value.
However, the array of callbacks could still have valid entries further
on in the array, or the callbacks may be able to process subsequent
subtables without error.  The change here makes the function consistent
with its description so that it will properly return the sum of all
matching entries for all proc handlers, instead of stopping abruptly
as it does today.

Signed-off-by: Al Stone <ahs3@redhat.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
---
 drivers/acpi/tables.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 3e167b4..76c07ed 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -246,6 +246,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
 	struct acpi_subtable_header *entry;
 	unsigned long table_end;
 	int count = 0;
+	int errs_found = 0;
 	int i;
 
 	if (acpi_disabled)
@@ -278,8 +279,10 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
 			if (entry->type != proc[i].id)
 				continue;
 			if (!proc[i].handler ||
-			     proc[i].handler(entry, table_end))
-				return -EINVAL;
+			     proc[i].handler(entry, table_end)) {
+				errs_found++;
+				continue;
+			}
 
 			proc[i].count++;
 			break;
@@ -305,7 +308,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
 			id, proc->id, count - max_entries, count);
 	}
 
-	return count;
+	return (errs_found) ? -EINVAL : count;
 }
 
 int __init
-- 
2.7.4

[PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports overflow correctly

[Al Stone]

The function acpi_parse_entries_array() has a limiting parameter,
max_entries, which tells the function to stop looking at subtables
once that limit has been reached.  Further, if the limit is reached,
it is reported.  However, the logic is incorrect in that the loop
to examine all subtables will always stop when exactly max_entries
have been found, regardless of whether or not there are still subtables
to examine, and it will always report that zero subtables have been
ignored.  This change allows the loop to continue to look at all
subtables and count all the ones of interest; if we have already
reached the number of max_entries, though, we will not invoke the
callback functions.  If the max_entries limit has been exceeded,
report on that, as before, but more accurately, listing how many
subtables of interest there are in total (as was meant), and how
many entries each subtable type occupied.

Signed-off-by: Al Stone <ahs3@redhat.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
---
 drivers/acpi/tables.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 76c07ed..227312d 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -272,12 +272,11 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
 
 	while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
 	       table_end) {
-		if (max_entries && count >= max_entries)
-			break;
-
 		for (i = 0; i < proc_num; i++) {
 			if (entry->type != proc[i].id)
 				continue;
+			if (max_entries && count >= max_entries)
+				break;
 			if (!proc[i].handler ||
 			     proc[i].handler(entry, table_end)) {
 				errs_found++;
@@ -304,8 +303,11 @@ acpi_parse_entries_array(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, proc->id, count - max_entries, count);
+		pr_warn("[%4.4s] ignored %i entries of %i found\n",
+			id, count - max_entries, count);
+		for (i = 0; i < proc_num; i++)
+			pr_warn("[%4.4s] subtable 0x%02x used %i entries\n",
+				id, proc[i].id, proc[i].count);
 	}
 
 	return (errs_found) ? -EINVAL: count;
-- 
2.7.4

Re: [PATCH 1/3] ACPI: fix incorrect counts returned by acpi_parse_entries_array()

[Rafael J. Wysocki]

On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
> The static function acpi_parse_entries_array() is provided an array of
> type struct acpi_subtable_proc that has a callback function and a count.
> The count should reflect how many times the callback has been successfully
> called.  However, the current code only increments the 0th element of the
> array, regardless of the number of entries in the array, or which callback
> has been invoked.  The fix is to use the index into the array, instead of
> a pointer to the beginning of the array.

OK, so it would be good to say what the consequences of the problem are too.

> Signed-off-by: Al Stone <ahs3@redhat.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> ---
>  drivers/acpi/tables.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 9f0ad6e..3e167b4 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -281,7 +281,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
>                              proc[i].handler(entry, table_end))
>                                 return -EINVAL;
>
> -                       proc->count++;
> +                       proc[i].count++;
>                         break;
>                 }
>                 if (i != proc_num)
> @@ -416,7 +416,7 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler)
>                 return -ENODEV;
>  }
>
> -/*
> +/*
>   * The BIOS is supposed to supply a single APIC/MADT,
>   * but some report two.  Provide a knob to use either.
>   * (don't you wish instance 0 and 1 were not the same?)
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables

[Rafael J. Wysocki]

On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
> Without this patch, the acpi_parse_entries_array() function will return
> the very first time there is any error found in either the array of
> callback functions or if one of the callbacks returns an non-zero value.
> However, the array of callbacks could still have valid entries further
> on in the array, or the callbacks may be able to process subsequent
> subtables without error.  The change here makes the function consistent
> with its description so that it will properly return the sum of all
> matching entries for all proc handlers, instead of stopping abruptly
> as it does today.

I'm not sure I follow.

You seem to be saying that the function should process all of the
subtables etc even though errors have been found for some of them, but
it still will return an error in the end if there are any errors.  How
exactly does it help to continue processing in case of an error, then?

> Signed-off-by: Al Stone <ahs3@redhat.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> ---
>  drivers/acpi/tables.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 3e167b4..76c07ed 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -246,6 +246,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
>         struct acpi_subtable_header *entry;
>         unsigned long table_end;
>         int count = 0;
> +       int errs_found = 0;
>         int i;
>
>         if (acpi_disabled)
> @@ -278,8 +279,10 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
>                         if (entry->type != proc[i].id)
>                                 continue;
>                         if (!proc[i].handler ||
> -                            proc[i].handler(entry, table_end))
> -                               return -EINVAL;
> +                            proc[i].handler(entry, table_end)) {
> +                               errs_found++;
> +                               continue;
> +                       }
>
>                         proc[i].count++;
>                         break;
> @@ -305,7 +308,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
>                         id, proc->id, count - max_entries, count);
>         }
>
> -       return count;
> +       return (errs_found) ? -EINVAL : count;
>  }
>
>  int __init
> --

Re: [PATCH 1/3] ACPI: fix incorrect counts returned by acpi_parse_entries_array()

[Al Stone]

On 07/01/2016 03:25 PM, Rafael J. Wysocki wrote:
> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>> The static function acpi_parse_entries_array() is provided an array of
>> type struct acpi_subtable_proc that has a callback function and a count.
>> The count should reflect how many times the callback has been successfully
>> called.  However, the current code only increments the 0th element of the
>> array, regardless of the number of entries in the array, or which callback
>> has been invoked.  The fix is to use the index into the array, instead of
>> a pointer to the beginning of the array.
> 
> OK, so it would be good to say what the consequences of the problem are too.
> 

Hrm.  So replace the last sentence with something like:

   The fix is to use the index into the array, instead of
   a pointer to the beginning of the array, so that the count
   for each element in the array in incremented by the
   corresponding callback.

That feels a little clunky but is it closer to what you were
thinking?

Thanks for the review!

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports overflow correctly

[Rafael J. Wysocki]

On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
> The function acpi_parse_entries_array() has a limiting parameter,
> max_entries, which tells the function to stop looking at subtables
> once that limit has been reached.  Further, if the limit is reached,
> it is reported.  However, the logic is incorrect in that the loop
> to examine all subtables will always stop when exactly max_entries
> have been found, regardless of whether or not there are still subtables
> to examine, and it will always report that zero subtables have been
> ignored.  This change allows the loop to continue to look at all
> subtables and count all the ones of interest; if we have already
> reached the number of max_entries, though, we will not invoke the
> callback functions.  If the max_entries limit has been exceeded,
> report on that, as before, but more accurately, listing how many
> subtables of interest there are in total (as was meant), and how
> many entries each subtable type occupied.

The problem appears to be that, if max_entries has been reached, it
prints "ignored 0", although it should count all of the entries in
that case too in principle.  Do I think correctly?

Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables

[Al Stone]

On 07/01/2016 03:32 PM, Rafael J. Wysocki wrote:
> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>> Without this patch, the acpi_parse_entries_array() function will return
>> the very first time there is any error found in either the array of
>> callback functions or if one of the callbacks returns an non-zero value.
>> However, the array of callbacks could still have valid entries further
>> on in the array, or the callbacks may be able to process subsequent
>> subtables without error.  The change here makes the function consistent
>> with its description so that it will properly return the sum of all
>> matching entries for all proc handlers, instead of stopping abruptly
>> as it does today.
> 
> I'm not sure I follow.
> 
> You seem to be saying that the function should process all of the
> subtables etc even though errors have been found for some of them, but
> it still will return an error in the end if there are any errors.  How
> exactly does it help to continue processing in case of an error, then?

The use case I have in mind is to simply count all of the subtables of
a certain type.  If for some reason, the callback -- or any other callback
-- fails, the traversal of all the subtables stops immediately.  So, I
could have two callbacks, and if the first one fails on the first subtable
of its type, traversal stops.  The count for the second callback will be
zero which may or may not be correct.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports overflow correctly

[Al Stone]

On 07/01/2016 03:40 PM, Rafael J. Wysocki wrote:
> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>> The function acpi_parse_entries_array() has a limiting parameter,
>> max_entries, which tells the function to stop looking at subtables
>> once that limit has been reached.  Further, if the limit is reached,
>> it is reported.  However, the logic is incorrect in that the loop
>> to examine all subtables will always stop when exactly max_entries
>> have been found, regardless of whether or not there are still subtables
>> to examine, and it will always report that zero subtables have been
>> ignored.  This change allows the loop to continue to look at all
>> subtables and count all the ones of interest; if we have already
>> reached the number of max_entries, though, we will not invoke the
>> callback functions.  If the max_entries limit has been exceeded,
>> report on that, as before, but more accurately, listing how many
>> subtables of interest there are in total (as was meant), and how
>> many entries each subtable type occupied.
> 
> The problem appears to be that, if max_entries has been reached, it
> prints "ignored 0", although it should count all of the entries in
> that case too in principle.  Do I think correctly?
> 

Exactly.  That's how I interpreted the comments.  And it fit what I
needed it to do if the comments were correct.

Of course, it could be the code was correct and the comments were
wrong :).  I preferred not to think that.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH 1/3] ACPI: fix incorrect counts returned by acpi_parse_entries_array()

[Rafael J. Wysocki]

On Fri, Jul 1, 2016 at 11:36 PM, Al Stone <ahs3@redhat.com> wrote:
> On 07/01/2016 03:25 PM, Rafael J. Wysocki wrote:
>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>> The static function acpi_parse_entries_array() is provided an array of
>>> type struct acpi_subtable_proc that has a callback function and a count.
>>> The count should reflect how many times the callback has been successfully
>>> called.  However, the current code only increments the 0th element of the
>>> array, regardless of the number of entries in the array, or which callback
>>> has been invoked.  The fix is to use the index into the array, instead of
>>> a pointer to the beginning of the array.
>>
>> OK, so it would be good to say what the consequences of the problem are too.
>>
>
> Hrm.  So replace the last sentence with something like:
>
>    The fix is to use the index into the array, instead of
>    a pointer to the beginning of the array, so that the count
>    for each element in the array in incremented by the
>    corresponding callback.
>
> That feels a little clunky but is it closer to what you were
> thinking?

Well, not really.

The code is arguably incorrect, but is there anything that does not
work as expected as a result?  Any functional breakage?  Any
misleading messages printed?

Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables

[Rafael J. Wysocki]

On Fri, Jul 1, 2016 at 11:41 PM, Al Stone <ahs3@redhat.com> wrote:
> On 07/01/2016 03:32 PM, Rafael J. Wysocki wrote:
>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>> Without this patch, the acpi_parse_entries_array() function will return
>>> the very first time there is any error found in either the array of
>>> callback functions or if one of the callbacks returns an non-zero value.
>>> However, the array of callbacks could still have valid entries further
>>> on in the array, or the callbacks may be able to process subsequent
>>> subtables without error.  The change here makes the function consistent
>>> with its description so that it will properly return the sum of all
>>> matching entries for all proc handlers, instead of stopping abruptly
>>> as it does today.
>>
>> I'm not sure I follow.
>>
>> You seem to be saying that the function should process all of the
>> subtables etc even though errors have been found for some of them, but
>> it still will return an error in the end if there are any errors.  How
>> exactly does it help to continue processing in case of an error, then?
>
> The use case I have in mind is to simply count all of the subtables of
> a certain type.  If for some reason, the callback -- or any other callback
> -- fails, the traversal of all the subtables stops immediately.  So, I
> could have two callbacks, and if the first one fails on the first subtable
> of its type, traversal stops.  The count for the second callback will be
> zero which may or may not be correct.

It will be zero, because the callback has not been invoked at all.
Why is this incorrect?

Re: [PATCH 1/3] ACPI: fix incorrect counts returned by acpi_parse_entries_array()

[Al Stone]

On 07/01/2016 03:44 PM, Rafael J. Wysocki wrote:
> On Fri, Jul 1, 2016 at 11:36 PM, Al Stone <ahs3@redhat.com> wrote:
>> On 07/01/2016 03:25 PM, Rafael J. Wysocki wrote:
>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>> The static function acpi_parse_entries_array() is provided an array of
>>>> type struct acpi_subtable_proc that has a callback function and a count.
>>>> The count should reflect how many times the callback has been successfully
>>>> called.  However, the current code only increments the 0th element of the
>>>> array, regardless of the number of entries in the array, or which callback
>>>> has been invoked.  The fix is to use the index into the array, instead of
>>>> a pointer to the beginning of the array.
>>>
>>> OK, so it would be good to say what the consequences of the problem are too.
>>>
>>
>> Hrm.  So replace the last sentence with something like:
>>
>>    The fix is to use the index into the array, instead of
>>    a pointer to the beginning of the array, so that the count
>>    for each element in the array in incremented by the
>>    corresponding callback.
>>
>> That feels a little clunky but is it closer to what you were
>> thinking?
> 
> Well, not really.
> 
> The code is arguably incorrect, but is there anything that does not
> work as expected as a result?  Any functional breakage?  Any
> misleading messages printed?
> 

That's the odd thing; there is no breakage.  Of any sort.

But, no one relies on those values for anything at this point.  I've got a
couple of ideas I'm working on that are easier if it does work right, however.


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports overflow correctly

[Rafael J. Wysocki]

On Fri, Jul 1, 2016 at 11:44 PM, Al Stone <ahs3@redhat.com> wrote:
> On 07/01/2016 03:40 PM, Rafael J. Wysocki wrote:
>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>> The function acpi_parse_entries_array() has a limiting parameter,
>>> max_entries, which tells the function to stop looking at subtables
>>> once that limit has been reached.  Further, if the limit is reached,
>>> it is reported.  However, the logic is incorrect in that the loop
>>> to examine all subtables will always stop when exactly max_entries
>>> have been found, regardless of whether or not there are still subtables
>>> to examine, and it will always report that zero subtables have been
>>> ignored.  This change allows the loop to continue to look at all
>>> subtables and count all the ones of interest; if we have already
>>> reached the number of max_entries, though, we will not invoke the
>>> callback functions.  If the max_entries limit has been exceeded,
>>> report on that, as before, but more accurately, listing how many
>>> subtables of interest there are in total (as was meant), and how
>>> many entries each subtable type occupied.
>>
>> The problem appears to be that, if max_entries has been reached, it
>> prints "ignored 0", although it should count all of the entries in
>> that case too in principle.  Do I think correctly?
>>
>
> Exactly.  That's how I interpreted the comments.  And it fit what I
> needed it to do if the comments were correct.
>
> Of course, it could be the code was correct and the comments were
> wrong :).  I preferred not to think that.

I guess whoever implemented this function thought that the overhead
for counting stuff was not useful in case max_entries had been
reached.  I'm not really sure I disagree with that. :-)

I agree that printing "ignored 0" in that case is misleading, but the
fix might be to simply avoid printing how many entries have been
ignored then.  Maybe it will suffice to print how many entries have
been found and what the limit was?

Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables

[Al Stone]

On 07/01/2016 03:46 PM, Rafael J. Wysocki wrote:
> On Fri, Jul 1, 2016 at 11:41 PM, Al Stone <ahs3@redhat.com> wrote:
>> On 07/01/2016 03:32 PM, Rafael J. Wysocki wrote:
>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>> Without this patch, the acpi_parse_entries_array() function will return
>>>> the very first time there is any error found in either the array of
>>>> callback functions or if one of the callbacks returns an non-zero value.
>>>> However, the array of callbacks could still have valid entries further
>>>> on in the array, or the callbacks may be able to process subsequent
>>>> subtables without error.  The change here makes the function consistent
>>>> with its description so that it will properly return the sum of all
>>>> matching entries for all proc handlers, instead of stopping abruptly
>>>> as it does today.
>>>
>>> I'm not sure I follow.
>>>
>>> You seem to be saying that the function should process all of the
>>> subtables etc even though errors have been found for some of them, but
>>> it still will return an error in the end if there are any errors.  How
>>> exactly does it help to continue processing in case of an error, then?
>>
>> The use case I have in mind is to simply count all of the subtables of
>> a certain type.  If for some reason, the callback -- or any other callback
>> -- fails, the traversal of all the subtables stops immediately.  So, I
>> could have two callbacks, and if the first one fails on the first subtable
>> of its type, traversal stops.  The count for the second callback will be
>> zero which may or may not be correct.
> 
> It will be zero, because the callback has not been invoked at all.
> Why is this incorrect?
> 

Because there could be additional subtables after the one causing a failure
that the second callback could have counted; e.g., if the failure is on the
first subtable of 20 in the MADT, the following 19 would be ignored, even if
they were all the right subtype for the second callback.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH 1/3] ACPI: fix incorrect counts returned by acpi_parse_entries_array()

[Rafael J. Wysocki]

On Fri, Jul 1, 2016 at 11:50 PM, Al Stone <ahs3@redhat.com> wrote:
> On 07/01/2016 03:44 PM, Rafael J. Wysocki wrote:
>> On Fri, Jul 1, 2016 at 11:36 PM, Al Stone <ahs3@redhat.com> wrote:
>>> On 07/01/2016 03:25 PM, Rafael J. Wysocki wrote:
>>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>>> The static function acpi_parse_entries_array() is provided an array of
>>>>> type struct acpi_subtable_proc that has a callback function and a count.
>>>>> The count should reflect how many times the callback has been successfully
>>>>> called.  However, the current code only increments the 0th element of the
>>>>> array, regardless of the number of entries in the array, or which callback
>>>>> has been invoked.  The fix is to use the index into the array, instead of
>>>>> a pointer to the beginning of the array.
>>>>
>>>> OK, so it would be good to say what the consequences of the problem are too.
>>>>
>>>
>>> Hrm.  So replace the last sentence with something like:
>>>
>>>    The fix is to use the index into the array, instead of
>>>    a pointer to the beginning of the array, so that the count
>>>    for each element in the array in incremented by the
>>>    corresponding callback.
>>>
>>> That feels a little clunky but is it closer to what you were
>>> thinking?
>>
>> Well, not really.
>>
>> The code is arguably incorrect, but is there anything that does not
>> work as expected as a result?  Any functional breakage?  Any
>> misleading messages printed?
>>
>
> That's the odd thing; there is no breakage.  Of any sort.
>
> But, no one relies on those values for anything at this point.  I've got a
> couple of ideas I'm working on that are easier if it does work right, however.

That's information that should go into the changelog too.

"There are no functional consequences of the issue, but fixing it is
necessary for future work."

Or similar.

Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables

[Rafael J. Wysocki]

On Fri, Jul 1, 2016 at 11:55 PM, Al Stone <ahs3@redhat.com> wrote:
> On 07/01/2016 03:46 PM, Rafael J. Wysocki wrote:
>> On Fri, Jul 1, 2016 at 11:41 PM, Al Stone <ahs3@redhat.com> wrote:
>>> On 07/01/2016 03:32 PM, Rafael J. Wysocki wrote:
>>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>>> Without this patch, the acpi_parse_entries_array() function will return
>>>>> the very first time there is any error found in either the array of
>>>>> callback functions or if one of the callbacks returns an non-zero value.
>>>>> However, the array of callbacks could still have valid entries further
>>>>> on in the array, or the callbacks may be able to process subsequent
>>>>> subtables without error.  The change here makes the function consistent
>>>>> with its description so that it will properly return the sum of all
>>>>> matching entries for all proc handlers, instead of stopping abruptly
>>>>> as it does today.
>>>>
>>>> I'm not sure I follow.
>>>>
>>>> You seem to be saying that the function should process all of the
>>>> subtables etc even though errors have been found for some of them, but
>>>> it still will return an error in the end if there are any errors.  How
>>>> exactly does it help to continue processing in case of an error, then?
>>>
>>> The use case I have in mind is to simply count all of the subtables of
>>> a certain type.  If for some reason, the callback -- or any other callback
>>> -- fails, the traversal of all the subtables stops immediately.  So, I
>>> could have two callbacks, and if the first one fails on the first subtable
>>> of its type, traversal stops.  The count for the second callback will be
>>> zero which may or may not be correct.
>>
>> It will be zero, because the callback has not been invoked at all.
>> Why is this incorrect?
>>
>
> Because there could be additional subtables after the one causing a failure
> that the second callback could have counted; e.g., if the failure is on the
> first subtable of 20 in the MADT, the following 19 would be ignored, even if
> they were all the right subtype for the second callback.

Let me rephrase: Is there any practical value of invoking any more
callbacks if one of them has failed?  If so, what is it?

You are changing semantics from "abort on the first failure" to
"process everything and count errors".  That's quite a bit different
and I'm trying to understand why the latter is better.

Re: [PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports overflow correctly

[Al Stone]

On 07/01/2016 03:54 PM, Rafael J. Wysocki wrote:
> On Fri, Jul 1, 2016 at 11:44 PM, Al Stone <ahs3@redhat.com> wrote:
>> On 07/01/2016 03:40 PM, Rafael J. Wysocki wrote:
>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>> The function acpi_parse_entries_array() has a limiting parameter,
>>>> max_entries, which tells the function to stop looking at subtables
>>>> once that limit has been reached.  Further, if the limit is reached,
>>>> it is reported.  However, the logic is incorrect in that the loop
>>>> to examine all subtables will always stop when exactly max_entries
>>>> have been found, regardless of whether or not there are still subtables
>>>> to examine, and it will always report that zero subtables have been
>>>> ignored.  This change allows the loop to continue to look at all
>>>> subtables and count all the ones of interest; if we have already
>>>> reached the number of max_entries, though, we will not invoke the
>>>> callback functions.  If the max_entries limit has been exceeded,
>>>> report on that, as before, but more accurately, listing how many
>>>> subtables of interest there are in total (as was meant), and how
>>>> many entries each subtable type occupied.
>>>
>>> The problem appears to be that, if max_entries has been reached, it
>>> prints "ignored 0", although it should count all of the entries in
>>> that case too in principle.  Do I think correctly?
>>>
>>
>> Exactly.  That's how I interpreted the comments.  And it fit what I
>> needed it to do if the comments were correct.
>>
>> Of course, it could be the code was correct and the comments were
>> wrong :).  I preferred not to think that.
> 
> I guess whoever implemented this function thought that the overhead
> for counting stuff was not useful in case max_entries had been
> reached.  I'm not really sure I disagree with that. :-)

I'm not sure I disagree, either :).

> I agree that printing "ignored 0" in that case is misleading, but the
> fix might be to simply avoid printing how many entries have been
> ignored then.  Maybe it will suffice to print how many entries have
> been found and what the limit was?

That could work.

Unless I've misunderstood the code, though, the situation that seemed likely
to me is, for example, to suppose that the first five subtables out of 20 are
of a single type and cause my max_entries limit to be reached.  If I have three
callbacks, I'd end up with two other callback functions that would never get
called, even if some of the remaining 15 subtables are pertinent and could help
get the boot process further along.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH 1/3] ACPI: fix incorrect counts returned by acpi_parse_entries_array()

[Al Stone]

On 07/01/2016 03:56 PM, Rafael J. Wysocki wrote:
> On Fri, Jul 1, 2016 at 11:50 PM, Al Stone <ahs3@redhat.com> wrote:
>> On 07/01/2016 03:44 PM, Rafael J. Wysocki wrote:
>>> On Fri, Jul 1, 2016 at 11:36 PM, Al Stone <ahs3@redhat.com> wrote:
>>>> On 07/01/2016 03:25 PM, Rafael J. Wysocki wrote:
>>>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>>>> The static function acpi_parse_entries_array() is provided an array of
>>>>>> type struct acpi_subtable_proc that has a callback function and a count.
>>>>>> The count should reflect how many times the callback has been successfully
>>>>>> called.  However, the current code only increments the 0th element of the
>>>>>> array, regardless of the number of entries in the array, or which callback
>>>>>> has been invoked.  The fix is to use the index into the array, instead of
>>>>>> a pointer to the beginning of the array.
>>>>>
>>>>> OK, so it would be good to say what the consequences of the problem are too.
>>>>>
>>>>
>>>> Hrm.  So replace the last sentence with something like:
>>>>
>>>>    The fix is to use the index into the array, instead of
>>>>    a pointer to the beginning of the array, so that the count
>>>>    for each element in the array in incremented by the
>>>>    corresponding callback.
>>>>
>>>> That feels a little clunky but is it closer to what you were
>>>> thinking?
>>>
>>> Well, not really.
>>>
>>> The code is arguably incorrect, but is there anything that does not
>>> work as expected as a result?  Any functional breakage?  Any
>>> misleading messages printed?
>>>
>>
>> That's the odd thing; there is no breakage.  Of any sort.
>>
>> But, no one relies on those values for anything at this point.  I've got a
>> couple of ideas I'm working on that are easier if it does work right, however.
> 
> That's information that should go into the changelog too.
> 
> "There are no functional consequences of the issue, but fixing it is
> necessary for future work."
> 
> Or similar.
> 

Will do in v2.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports overflow correctly

[Rafael J. Wysocki]

On Sat, Jul 2, 2016 at 12:38 AM, Al Stone <ahs3@redhat.com> wrote:
> On 07/01/2016 03:54 PM, Rafael J. Wysocki wrote:
>> On Fri, Jul 1, 2016 at 11:44 PM, Al Stone <ahs3@redhat.com> wrote:
>>> On 07/01/2016 03:40 PM, Rafael J. Wysocki wrote:
>>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>>> The function acpi_parse_entries_array() has a limiting parameter,
>>>>> max_entries, which tells the function to stop looking at subtables
>>>>> once that limit has been reached.  Further, if the limit is reached,
>>>>> it is reported.  However, the logic is incorrect in that the loop
>>>>> to examine all subtables will always stop when exactly max_entries
>>>>> have been found, regardless of whether or not there are still subtables
>>>>> to examine, and it will always report that zero subtables have been
>>>>> ignored.  This change allows the loop to continue to look at all
>>>>> subtables and count all the ones of interest; if we have already
>>>>> reached the number of max_entries, though, we will not invoke the
>>>>> callback functions.  If the max_entries limit has been exceeded,
>>>>> report on that, as before, but more accurately, listing how many
>>>>> subtables of interest there are in total (as was meant), and how
>>>>> many entries each subtable type occupied.
>>>>
>>>> The problem appears to be that, if max_entries has been reached, it
>>>> prints "ignored 0", although it should count all of the entries in
>>>> that case too in principle.  Do I think correctly?
>>>>
>>>
>>> Exactly.  That's how I interpreted the comments.  And it fit what I
>>> needed it to do if the comments were correct.
>>>
>>> Of course, it could be the code was correct and the comments were
>>> wrong :).  I preferred not to think that.
>>
>> I guess whoever implemented this function thought that the overhead
>> for counting stuff was not useful in case max_entries had been
>> reached.  I'm not really sure I disagree with that. :-)
>
> I'm not sure I disagree, either :).
>
>> I agree that printing "ignored 0" in that case is misleading, but the
>> fix might be to simply avoid printing how many entries have been
>> ignored then.  Maybe it will suffice to print how many entries have
>> been found and what the limit was?
>
> That could work.
>
> Unless I've misunderstood the code, though, the situation that seemed likely
> to me is, for example, to suppose that the first five subtables out of 20 are
> of a single type and cause my max_entries limit to be reached.  If I have three
> callbacks, I'd end up with two other callback functions that would never get
> called, even if some of the remaining 15 subtables are pertinent and could help
> get the boot process further along.

That depends on what max_entries is used for which I don't recall ATM.

So before making changes here, I'd recommend looking for code that
uses max_entries in non-trivial ways and finding the reasons why it is
used.

Maybe it is just not really needed or maybe it should just be replaced
with something else.  In any case, without any research in that
direction, I'd rather do the simplest fix possible.

Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables

[Al Stone]

On 07/01/2016 04:01 PM, Rafael J. Wysocki wrote:
> On Fri, Jul 1, 2016 at 11:55 PM, Al Stone <ahs3@redhat.com> wrote:
>> On 07/01/2016 03:46 PM, Rafael J. Wysocki wrote:
>>> On Fri, Jul 1, 2016 at 11:41 PM, Al Stone <ahs3@redhat.com> wrote:
>>>> On 07/01/2016 03:32 PM, Rafael J. Wysocki wrote:
>>>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>>>> Without this patch, the acpi_parse_entries_array() function will return
>>>>>> the very first time there is any error found in either the array of
>>>>>> callback functions or if one of the callbacks returns an non-zero value.
>>>>>> However, the array of callbacks could still have valid entries further
>>>>>> on in the array, or the callbacks may be able to process subsequent
>>>>>> subtables without error.  The change here makes the function consistent
>>>>>> with its description so that it will properly return the sum of all
>>>>>> matching entries for all proc handlers, instead of stopping abruptly
>>>>>> as it does today.
>>>>>
>>>>> I'm not sure I follow.
>>>>>
>>>>> You seem to be saying that the function should process all of the
>>>>> subtables etc even though errors have been found for some of them, but
>>>>> it still will return an error in the end if there are any errors.  How
>>>>> exactly does it help to continue processing in case of an error, then?
>>>>
>>>> The use case I have in mind is to simply count all of the subtables of
>>>> a certain type.  If for some reason, the callback -- or any other callback
>>>> -- fails, the traversal of all the subtables stops immediately.  So, I
>>>> could have two callbacks, and if the first one fails on the first subtable
>>>> of its type, traversal stops.  The count for the second callback will be
>>>> zero which may or may not be correct.
>>>
>>> It will be zero, because the callback has not been invoked at all.
>>> Why is this incorrect?
>>>
>>
>> Because there could be additional subtables after the one causing a failure
>> that the second callback could have counted; e.g., if the failure is on the
>> first subtable of 20 in the MADT, the following 19 would be ignored, even if
>> they were all the right subtype for the second callback.
> 
> Let me rephrase: Is there any practical value of invoking any more
> callbacks if one of them has failed?  If so, what is it?
> 
> You are changing semantics from "abort on the first failure" to
> "process everything and count errors".  That's quite a bit different
> and I'm trying to understand why the latter is better.
> 

Agreed, it is a shift in semantics.

The practical value to me is being able to use acpi_parse_entries_array() to
solve a broader range of problems.  The situation I have is that I need to
count three different subtable types in the MADT.  I could call
acpi_table_parse_madt() three different times, or I could call
acpi_parse_entries_array() once -- it seemed to me the second makes for cleaner
code and will be slightly more efficient (one map/unmap of the table, vs
three), but that only works if all of the subtables are traversed.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH 2/3] ACPI: fix acpi_parse_entries_array() so it traverses all subtables

[Rafael J. Wysocki]

On Sat, Jul 2, 2016 at 1:07 AM, Al Stone <ahs3@redhat.com> wrote:
> On 07/01/2016 04:01 PM, Rafael J. Wysocki wrote:
>> On Fri, Jul 1, 2016 at 11:55 PM, Al Stone <ahs3@redhat.com> wrote:
>>> On 07/01/2016 03:46 PM, Rafael J. Wysocki wrote:
>>>> On Fri, Jul 1, 2016 at 11:41 PM, Al Stone <ahs3@redhat.com> wrote:
>>>>> On 07/01/2016 03:32 PM, Rafael J. Wysocki wrote:
>>>>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>>>>> Without this patch, the acpi_parse_entries_array() function will return
>>>>>>> the very first time there is any error found in either the array of
>>>>>>> callback functions or if one of the callbacks returns an non-zero value.
>>>>>>> However, the array of callbacks could still have valid entries further
>>>>>>> on in the array, or the callbacks may be able to process subsequent
>>>>>>> subtables without error.  The change here makes the function consistent
>>>>>>> with its description so that it will properly return the sum of all
>>>>>>> matching entries for all proc handlers, instead of stopping abruptly
>>>>>>> as it does today.
>>>>>>
>>>>>> I'm not sure I follow.
>>>>>>
>>>>>> You seem to be saying that the function should process all of the
>>>>>> subtables etc even though errors have been found for some of them, but
>>>>>> it still will return an error in the end if there are any errors.  How
>>>>>> exactly does it help to continue processing in case of an error, then?
>>>>>
>>>>> The use case I have in mind is to simply count all of the subtables of
>>>>> a certain type.  If for some reason, the callback -- or any other callback
>>>>> -- fails, the traversal of all the subtables stops immediately.  So, I
>>>>> could have two callbacks, and if the first one fails on the first subtable
>>>>> of its type, traversal stops.  The count for the second callback will be
>>>>> zero which may or may not be correct.
>>>>
>>>> It will be zero, because the callback has not been invoked at all.
>>>> Why is this incorrect?
>>>>
>>>
>>> Because there could be additional subtables after the one causing a failure
>>> that the second callback could have counted; e.g., if the failure is on the
>>> first subtable of 20 in the MADT, the following 19 would be ignored, even if
>>> they were all the right subtype for the second callback.
>>
>> Let me rephrase: Is there any practical value of invoking any more
>> callbacks if one of them has failed?  If so, what is it?
>>
>> You are changing semantics from "abort on the first failure" to
>> "process everything and count errors".  That's quite a bit different
>> and I'm trying to understand why the latter is better.
>>
>
> Agreed, it is a shift in semantics.
>
> The practical value to me is being able to use acpi_parse_entries_array() to
> solve a broader range of problems.  The situation I have is that I need to
> count three different subtable types in the MADT.  I could call
> acpi_table_parse_madt() three different times, or I could call
> acpi_parse_entries_array() once -- it seemed to me the second makes for cleaner
> code and will be slightly more efficient (one map/unmap of the table, vs
> three), but that only works if all of the subtables are traversed.

That makes sense, but then again please add a "motivation" part to the
changelog with that explanation.

Also, there is a slight danger here that some callbacks may assume
that they won't be invoked if one of the callbacks invoked earlier
returns an error.  Have you double checked that this is not the case?