Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1466754 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/3] Correct errors in acpi_parse_entries_array() Al Stone <ahs3@redhat.com> - 2016-08-20 02:50 +0200
    [PATCH v2 1/3] ACPI: fix incorrect counts returned by acpi_parse_entries_array() Al Stone <ahs3@redhat.com> - 2016-08-20 02:50 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web