Re: [PATCH v8 05/13] ACPI/PPTT: Add Processor Properties Topology Table parsing

From: Jeremy Linton
Date: Fri Apr 27 2018 - 12:20:53 EST


Thanks for taking a look at this.

On 04/27/2018 06:02 AM, Rafael J. Wysocki wrote:
On Thu, Apr 26, 2018 at 1:31 AM, Jeremy Linton <jeremy.linton@xxxxxxx> wrote:
ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific

An additional patch later in the set adds the ability to report
peers in the topology using find_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, within a given
package, etc.

Signed-off-by: Jeremy Linton <jeremy.linton@xxxxxxx>
Acked-by: Sudeep Holla <sudeep.holla@xxxxxxx>
drivers/acpi/pptt.c | 518 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 518 insertions(+)
create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index 000000000000..cced71ef851a
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,0 +1,518 @@
+// SPDX-License-Identifier: GPL-2.0
+ * pptt.c - parsing of Processor Properties Topology Table
+ *
+ * Copyright (C) 2018, ARM
+ *
+ * This file implements parsing of Processor Properties Topology Table (PPTT)
+ * which is optionally used to describe the processor and cache topology.
+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ *
+ * The PPTT structure is an inverted tree, with each node potentially
+ * holding one or two inverted tree data structures describing
+ * the caches available at that level. Each cache structure optionally
+ * contains properties describing the cache at a given level which can be
+ * used to override hardware probed values.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+#include <linux/acpi.h>
+#include <linux/cacheinfo.h>
+#include <acpi/processor.h>
+ * fetch_pptt_subtable() - Find/Verify that the PPTT ref is a valid subtable

The parens above are at least redundant (if not harmful). Everywhere
else in a similar context too.

The kerneldoc ones? I guess i'm confused the kernel doc example in doc-guide/kernel-doc has

* function_name() - Brief description of function.

Also kerneldoc comments document function arguments too as a rule, so
please do that here and wherever you use kerneldoc comments in the

Ok, sure.

+ *
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ *
+ * Return: acpi_subtable_header* or NULL
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(struct acpi_table_header *table_hdr,
+ u32 pptt_ref)
+ struct acpi_subtable_header *entry;
+ /* there isn't a subtable at reference 0 */
+ if (pptt_ref < sizeof(struct acpi_subtable_header))
+ return NULL;
+ if (pptt_ref + sizeof(struct acpi_subtable_header) > table_hdr->length)
+ return NULL;
+ entry = ACPI_ADD_PTR(struct acpi_subtable_header, table_hdr, pptt_ref);
+ if (entry->length == 0)
+ return NULL;
+ if (pptt_ref + entry->length > table_hdr->length)
+ return NULL;
+ return entry;

Apart from the above I'm not entirely sure why you need the changes in
patch [09/13] to go in a separate patch. All of them are new code
going into the file created by this patch, so why not to put them

Ok, I was doing that because Lorenzo asked for it, but he hasn't said much so I will collapse it back together. That makes me happy, as splitting chunks between patches is a pain anyway.