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

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

[Al Stone]

Several strange things were found while prototyping a way to correctly
report whether or not an IORT is needed by an arm64 platform.  What the
prototype needed to do was iterate over the MADT subtables and look for
certain types of subtable, count how many there were by type, and capture
some info about some of the subtables.

When I first read through the acpi_parse_entries_array() function, the
comments seemed to describe precisely what I needed to do.  However, what
I found in trying to use the function is that that is not exactly how it
works.

So, the first patch fixes the counts of subtable types found so they are
correct.  Fortunately, nothing has relied on them being correct in the 
past, just non-zero or not.  This has also been noted by Baoquan He in a
patch he recently submitted.

The second patch fixes a problem where iterating over the subtables would
end earlier than necessary, again throwing off the counts; in this case,
I only needed to know how many of certain types of subtables were present,
but would never be able to get to a complete count.

The third patch removes part of a message that always reported an incorrect
value for the number of subtables skipped when there's a limit.  It always
reported zero, so it's never really been useful.

Longer term, I would like to simplify the way MADT and other ACPI tables
with subtables are parsed; mostly, I want to get rid of the need for using
file scoped variables.  The acpi_parse_entries_array() function can be a
good basis to build upon, as long as it works as expected.

Changes since v1:
  -- Much rewrite of the cover letter and changelogs to make sure the 
     motivation for the changes was captured (Rafael)
  -- Verified that the changes in semantics for the function are harmless;
     direct calls, parents/grandparents/.. of the direct calls were checked
     to make sure they did not depend on the current behavior in such a way
     as to break them with these changes (Rafael)
  -- Patch 3/3 was simplified; on further investigation, should additional
     callbacks be made to get the total skipped, there could be side effects
     or dependencies between callbacks that would make the original plan of
     iterating over all callbacks risky, for very little value (Rafael)

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: do not report the number of entries ignored by
    acpi_parse_entries()

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

-- 
2.7.4

[PATCH v2 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 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 result is that we know the total number of callbacks
made but we cannot determine which callbacks were made, nor how often.
The fix is to index into the array of structs and increment the proper
counts.

There is one place in the x86 code for acpi_parse_madt_lapic_entries()
where the counts for each callback are used.  If no LAPICs *and* no
X2APICs are found, an ENODEV is supposed to be returned; as it stands,
the count of X2APICs will always be zero, regardless of what is in the
MADT.  Should there be no LAPICs, ENODEV will be returned in error, if
there are X2APICs in the MADT.

Otherwise, there are no other functional consequences of the count being
done as it currently is; all other uses simply check that the return value
from acpi_parse_entries_array() or passed back via its callers is either
non-zero, an error, or in one case just ignored.

In future patches, I will also need these counts to be correct; I need
to count the number of instances of subtables of certain types within
the MADT to determine whether or not an ACPI IORT is required or not,
and report when it is not present when it should be.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index fa6fd19..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)
-- 
2.7.4

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

[Al Stone]

The acpi_parse_entries_array() function currently returns the very first
time there is any error found by one of the callback functions, or if one
of the callbacks returns a non-zero value.  However, the ACPI subtables
being traversed could still have valid entries that could be used by one
of the callback functions.  And, if the comments are correct, that is
what should happen -- always traverse all of the subtables, calling as
many of the callbacks as possible.

This patch makes the function consistent with its description so that it
will properly invoke all callbacks for all matching entries, for all
subtables, instead of stopping abruptly as it does today.

This does change the semantics of using acpi_parse_entries_array().  In
examining all users of the function, none of them rely on the current
behavior; that is, there appears to be no assumption that either all
subtables are traversed and all callbacks invoked, or that the function
will return immediately on any error from a callback.  Each callback
operates independently.  Hence, there should be no functional change
due to this change in semantics.

Future patches being prepared will rely on this new behavior; indeed,
they were written assuming the acpi_parse_entries_array() function
operated as its comments describe.  For example, a callback that
counts the number of subtables of a specific type can now be assured
that as many subtables as possible have been enumerated.

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..0d5d17f 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 = 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;
+			     (!errs && proc[i].handler(entry, table_end))) {
+				errs++;
+				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 ? -EINVAL : count;
 }
 
 int __init
-- 
2.7.4

[PATCH v2 3/3] ACPI: do not report the number of entries ignored by acpi_parse_entries()

[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.  If the limit is reached, it is
reported.  However, the logic is incorrect in that the loop to
examine all subtables will always report that zero subtables have
been ignored since it does not continue once the max_entries have
been reached.

One approach to fixing this would be to correct the logic so that
all subtables are examined, even if we have hit the max_entries, but
without executing all the callback functions.  This could be risky
since we cannot guarantee that no callback will ever have side effects
that another callback depends on to work correctly.

So, the simplest approach is to just remove the part of the error
message that will always be incorrect.

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 0d5d17f..984260d 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -304,8 +304,8 @@ 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:0x%02x] found the maximum %i entries\n",
+			id, proc->id, count);
 	}
 
 	return (errs_found) ? -EINVAL : count;
-- 
2.7.4

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

[Rafael J. Wysocki]

On Friday, August 19, 2016 06:48:10 PM Al Stone wrote:
> Several strange things were found while prototyping a way to correctly
> report whether or not an IORT is needed by an arm64 platform.  What the
> prototype needed to do was iterate over the MADT subtables and look for
> certain types of subtable, count how many there were by type, and capture
> some info about some of the subtables.
> 
> When I first read through the acpi_parse_entries_array() function, the
> comments seemed to describe precisely what I needed to do.  However, what
> I found in trying to use the function is that that is not exactly how it
> works.
> 
> So, the first patch fixes the counts of subtable types found so they are
> correct.  Fortunately, nothing has relied on them being correct in the 
> past, just non-zero or not.  This has also been noted by Baoquan He in a
> patch he recently submitted.
> 
> The second patch fixes a problem where iterating over the subtables would
> end earlier than necessary, again throwing off the counts; in this case,
> I only needed to know how many of certain types of subtables were present,
> but would never be able to get to a complete count.
> 
> The third patch removes part of a message that always reported an incorrect
> value for the number of subtables skipped when there's a limit.  It always
> reported zero, so it's never really been useful.
> 
> Longer term, I would like to simplify the way MADT and other ACPI tables
> with subtables are parsed; mostly, I want to get rid of the need for using
> file scoped variables.  The acpi_parse_entries_array() function can be a
> good basis to build upon, as long as it works as expected.
> 
> Changes since v1:
>   -- Much rewrite of the cover letter and changelogs to make sure the 
>      motivation for the changes was captured (Rafael)
>   -- Verified that the changes in semantics for the function are harmless;
>      direct calls, parents/grandparents/.. of the direct calls were checked
>      to make sure they did not depend on the current behavior in such a way
>      as to break them with these changes (Rafael)
>   -- Patch 3/3 was simplified; on further investigation, should additional
>      callbacks be made to get the total skipped, there could be side effects
>      or dependencies between callbacks that would make the original plan of
>      iterating over all callbacks risky, for very little value (Rafael)
> 
> 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: do not report the number of entries ignored by
>     acpi_parse_entries()

All applied.

Thanks,
Rafael