[PATCH v2 0/9] ACPI: parse the SPCR table

[PATCH v2 0/9] ACPI: parse the SPCR table

[Aleksey Makarov]

This patchset is based on the patchset by Leif Lindholm [1]

'ARM Server Base Boot Requirements' [2] mentions SPCR (Serial Port
Console Redirection Table) [3] as a mandatory ACPI table that
specifies the configuration of serial console.

SPCR support is included in QEMU's ARM mach-virt since 2.4 release.

Parse the SPCR table and check if any registered console matches the
description.  If it does, enable that console.

Also add an implementation of this member function to the pl011 and 8250
drivers, fix a minor issue in printk.c and use SPCR to tell if SBSA serial
driver should use 32-bit access to registers.
To be able to access SPCR table each time any console is registered
change __init to __ref for early_acpi_os_unmap_memory()
and introduce acpi_table_parse2() in separate patches.

The patchset should be applied to tty-next.

v2:
- don't use SPCR if user specified console in command line
- fix initialization order of newcon->index = 0
- rename some variables at printk.c (Joe Perches, Peter Hurley)
- enable ACPI_SPCR_TABLE in a separate patch (Andy Shevchenko)
- remove the retry loop for console registering (Peter Hurley).
  Instead, obtain SPCR with acpi_get_table().  That works after
  call to acpi_early_init() i. e. in any *_initcall()
- describe design decision behind introducing acpi_match() (Peter Hurley)
- fix compilation for x86 + ACPI (Graeme Gregory)
- introduce DBG2 constants in a separate patch (Andy Shevchenko)
- fix a typo in DBG2 constants (Andy Shevchenko)
- add ACPI_DBG2_ARM_SBSA_32BIT constant (Christopher Covington)
- add support for ACPI_DBG2_ARM_SBSA_* consoles (Christopher Covington)
- add documentation for functions
- add a patch that uses SPCR to find if SBSA serial driver should use 32-bit
  accessor functions (Christopher Covington)
- change __init to __ref for early_acpi_os_unmap_memory() in a separate patch
- introduce acpi_table_parse2() in a separate patch
- fix fetching the SPCR table early (Mark Salter)
- add a patch from Mark Salter that introduces support for matching 8250-based
  consoles

v1:
https://lkml.kernel.org/g/1453722324-22407-1-git-send-email-aleksey.makarov@linaro.org

[1] https://lkml.kernel.org/g/1441716217-23786-1-git-send-email-leif.lindholm@linaro.org
[2] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044a/index.html
[3] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx

Aleksey Makarov (8):
  printk: fix name and type of some variables
  ACPI: Change __init to __ref for early_acpi_os_unmap_memory()
  ACPI: introduce acpi_table_parse2()
  ACPI: parse SPCR and enable matching console
  ACPI: enable ACPI_SPCR_TABLE on ARM64
  ACPI: add definition of DBG2 subtypes
  serial: pl011: add acpi_match
  serial: pl011: use SPCR to setup 32-bit access

Mark Salter (1):
  serial: 8250: add acpi_match

 arch/arm64/Kconfig                  |   1 +
 drivers/acpi/Kconfig                |   3 +
 drivers/acpi/Makefile               |   1 +
 drivers/acpi/osl.c                  |   6 +-
 drivers/acpi/spcr.c                 | 106 ++++++++++++++++++++++++++++++++++++
 drivers/acpi/tables.c               |  49 ++++++++++++++---
 drivers/tty/serial/8250/8250_core.c |  23 ++++++++
 drivers/tty/serial/amba-pl011.c     |  24 ++++++++
 include/acpi/actbl2.h               |   5 ++
 include/linux/acpi.h                |   8 +++
 include/linux/console.h             |  17 ++++++
 kernel/printk/printk.c              |  40 ++++++++++----
 12 files changed, 264 insertions(+), 19 deletions(-)
 create mode 100644 drivers/acpi/spcr.c

-- 
2.7.0

[PATCH v2 1/9] printk: fix name and type of some variables

[Aleksey Makarov]

The variable preferred_console is used only inside register_console()
and its semantics is boolean.  It is negative when no console has been
made preferred.

The variable selected_console holds an index into the console_cmdline
array, pointing to the console that was made preferred.

Rename the variables:
selected_console -> preferred_console
preferred_console -> has_preferred
and make the new has_preferred local static bool.

This is a preparatory patch.  To introduce selection with
ACPI SPCR table we should decide which value (old) preferred_console
should hold in the case when the console was set by ACPI.

Renaming was suggested by Peter Hurley

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
Tested-by: Christopher Covington <cov@codeaurora.org>
---
 kernel/printk/printk.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index c963ba5..0cc9542 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -143,7 +143,6 @@ static struct console *exclusive_console;
 
 static struct console_cmdline console_cmdline[MAX_CMDLINECONSOLES];
 
-static int selected_console = -1;
 static int preferred_console = -1;
 int console_set_on_cmdline;
 EXPORT_SYMBOL(console_set_on_cmdline);
@@ -1997,14 +1996,14 @@ static int __add_preferred_console(char *name, int idx, char *options,
 	     i++, c++) {
 		if (strcmp(c->name, name) == 0 && c->index == idx) {
 			if (!brl_options)
-				selected_console = i;
+				preferred_console = i;
 			return 0;
 		}
 	}
 	if (i == MAX_CMDLINECONSOLES)
 		return -E2BIG;
 	if (!brl_options)
-		selected_console = i;
+		preferred_console = i;
 	strlcpy(c->name, name, sizeof(c->name));
 	c->options = options;
 	braille_set_options(c, brl_options);
@@ -2490,6 +2489,7 @@ void register_console(struct console *newcon)
 	unsigned long flags;
 	struct console *bcon = NULL;
 	struct console_cmdline *c;
+	static bool has_preferred;
 
 	if (console_drivers)
 		for_each_console(bcon)
@@ -2516,15 +2516,15 @@ void register_console(struct console *newcon)
 	if (console_drivers && console_drivers->flags & CON_BOOT)
 		bcon = console_drivers;
 
-	if (preferred_console < 0 || bcon || !console_drivers)
-		preferred_console = selected_console;
+	if (!has_preferred || bcon || !console_drivers)
+		has_preferred = preferred_console >= 0;
 
 	/*
 	 *	See if we want to use this console driver. If we
 	 *	didn't select a console we take the first one
 	 *	that registers here.
 	 */
-	if (preferred_console < 0) {
+	if (!has_preferred) {
 		if (newcon->index < 0)
 			newcon->index = 0;
 		if (newcon->setup == NULL ||
@@ -2532,7 +2532,7 @@ void register_console(struct console *newcon)
 			newcon->flags |= CON_ENABLED;
 			if (newcon->device) {
 				newcon->flags |= CON_CONSDEV;
-				preferred_console = 0;
+				has_preferred = true;
 			}
 		}
 	}
@@ -2565,9 +2565,9 @@ void register_console(struct console *newcon)
 		}
 
 		newcon->flags |= CON_ENABLED;
-		if (i == selected_console) {
+		if (i == preferred_console) {
 			newcon->flags |= CON_CONSDEV;
-			preferred_console = selected_console;
+			has_preferred = true;
 		}
 		break;
 	}
-- 
2.7.0

[PATCH v2 2/9] ACPI: Change __init to __ref for early_acpi_os_unmap_memory()

[Aleksey Makarov]

early_acpi_os_unmap_memory() is marked as __init because it calls
__acpi_unmap_table(), but only when acpi_gbl_permanent_mmap is not set.

acpi_gbl_permanent_mmap is set in __init acpi_early_init()
so it is safe to call early_acpi_os_unmap_memory() from anywhere

We need this function to be non-__init because we need access to
some tables at unpredictable time--it may be before or after
acpi_gbl_permanent_mmap is set.  For example, SPCR (Serial Port Console
Redirection) table is needed each time a new console is registered.
It can be quite early (console_initcall) or when a module is inserted.
When this table accessed before acpi_gbl_permanent_mmap is set,
the pointer should be unmapped.  This is exactly what this function
does.

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
---
 drivers/acpi/osl.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 67da6fb..8a552cd 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -497,7 +497,11 @@ void __ref acpi_os_unmap_memory(void *virt, acpi_size size)
 }
 EXPORT_SYMBOL_GPL(acpi_os_unmap_memory);
 
-void __init early_acpi_os_unmap_memory(void __iomem *virt, acpi_size size)
+/*
+ * acpi_gbl_permanent_mmap is set in __init acpi_early_init()
+ * so it is safe to call early_acpi_os_unmap_memory() from anywhere
+ */
+void __ref early_acpi_os_unmap_memory(void __iomem *virt, acpi_size size)
 {
 	if (!acpi_gbl_permanent_mmap)
 		__acpi_unmap_table(virt, size);
-- 
2.7.0

[PATCH v2 3/9] ACPI: introduce acpi_table_parse2()

[Aleksey Makarov]

The function acpi_table_parse() has some problems:
1 It can be called only from __init code
2 It does not pass any data to the handler
3 It just throws out the value returned from the handler

These issues are addressed in this patch

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
---
 drivers/acpi/tables.c | 49 ++++++++++++++++++++++++++++++++++++++++++-------
 include/linux/acpi.h  |  8 ++++++++
 2 files changed, 50 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 6c0f079..98ae052 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -40,7 +40,7 @@ static char *mps_inti_flags_trigger[] = { "dfl", "edge", "res", "level" };
 
 static struct acpi_table_desc initial_tables[ACPI_MAX_TABLES] __initdata;
 
-static int acpi_apic_instance __initdata;
+static int acpi_apic_instance;
 
 /*
  * Disable table checksum verification for the early stage due to the size
@@ -374,19 +374,27 @@ acpi_table_parse_madt(enum acpi_madt_type id,
 }
 
 /**
- * acpi_table_parse - find table with @id, run @handler on it
+ * acpi_table_parse2 - find table with @id, run @handler on it
  * @id: table id to find
  * @handler: handler to run
+ * @data: data to pass to handler
  *
  * Scan the ACPI System Descriptor Table (STD) for a table matching @id,
  * run @handler on it.
  *
- * Return 0 if table found, -errno if not.
+ * How it differs from acpi_table_parse()
+ * 1. It respect the return value of the handler function
+ * 2. It has additional data argument to make closures
+ * 3. It can be called from everywhere from the kernel (no __init)
+ *
+ * Return: the value returned by the handler if table found, -errno if not.
  */
-int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler)
+int acpi_table_parse2(char *id,
+	int (*handler)(struct acpi_table_header *table, void *data), void *data)
 {
 	struct acpi_table_header *table = NULL;
 	acpi_size tbl_size;
+	int err;
 
 	if (acpi_disabled)
 		return -ENODEV;
@@ -395,16 +403,43 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler)
 		return -EINVAL;
 
 	if (strncmp(id, ACPI_SIG_MADT, 4) == 0)
-		acpi_get_table_with_size(id, acpi_apic_instance, &table, &tbl_size);
+		acpi_get_table_with_size(id, acpi_apic_instance, &table,
+					 &tbl_size);
 	else
 		acpi_get_table_with_size(id, 0, &table, &tbl_size);
 
 	if (table) {
-		handler(table);
+		err = handler(table, data);
 		early_acpi_os_unmap_memory(table, tbl_size);
-		return 0;
+		return err;
 	} else
 		return -ENODEV;
+
+}
+
+/*
+ * 1. fix the signature
+ * 2. always return 0, don't respect the value returned from the handler
+ */
+static int legacy_acpi_table_handler(struct acpi_table_header *table, void *d)
+{
+	((acpi_tbl_table_handler)d)(table);
+	return 0;
+}
+
+/**
+ * acpi_table_parse - find table with @id, run @handler on it
+ * @id: table id to find
+ * @handler: handler to run
+ *
+ * Scan the ACPI System Descriptor Table (STD) for a table matching @id,
+ * run @handler on it.
+ *
+ * Return 0 if table found, -errno if not.
+ */
+int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler)
+{
+	return acpi_table_parse2(id, legacy_acpi_table_handler, handler);
 }
 
 /* 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 06ed7e5..bed9b89 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -218,6 +218,8 @@ int acpi_numa_init (void);
 
 int acpi_table_init (void);
 int acpi_table_parse(char *id, acpi_tbl_table_handler handler);
+int acpi_table_parse2(char *id, int (*handler)(struct acpi_table_header *table,
+					       void *data), void *data);
 int __init acpi_parse_entries(char *id, unsigned long table_size,
 			      acpi_tbl_entry_handler handler,
 			      struct acpi_table_header *table_header,
@@ -636,6 +638,12 @@ static inline int acpi_table_parse(char *id,
 	return -ENODEV;
 }
 
+int acpi_table_parse2(char *id, int (*handler)(struct acpi_table_header *table,
+					       void *data), void *data)
+{
+	return -ENODEV;
+}
+
 static inline int acpi_nvs_register(__u64 start, __u64 size)
 {
 	return 0;
-- 
2.7.0

[PATCH v2 4/9] ACPI: parse SPCR and enable matching console

[Aleksey Makarov]

'ARM Server Base Boot Requiremets' [1] mentions SPCR (Serial Port
Console Redirection Table) [2] as a mandatory ACPI table that
specifies the configuration of serial console.

Parse this table and check if any registered console match the
description.  If it does, enable that console.

In the original submission [3] the ACPI devices are traversed to find
one that has the same address as specified by SPCR. Then while
registering a serial device it is checked if it has an ACPI companion
and it is the same device that was found at traversal. If so,
add_preferred_console() was used to specify the device as preferred.
The console name and index are known at the time of registration of
serial device.

The problem is that SPCR can specify the console not only by the base
address but also with PCI Device ID/PCI Vendor ID or PCI Bus
Number/PCI Device Number.  In general, there is no way to get serial
name and index from the SPCR table.

To implement that, introduce a new member

        int (*acpi_match)(struct console *, struct acpi_table_spcr *)

of struct console.  It allows drivers to check if they provide a
matching console device.

[1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044a/index.html
[2] http://msdn.microsoft.com/en-us/library/windows/hardware/dn639131(v=vs.85).aspx
[3] https://lkml.kernel.org/g/1441716217-23786-1-git-send-email-leif.lindholm@linaro.org

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
---
 drivers/acpi/Kconfig    |  3 ++
 drivers/acpi/Makefile   |  1 +
 drivers/acpi/spcr.c     | 77 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/console.h | 12 ++++++++
 kernel/printk/printk.c  | 22 ++++++++++++--
 5 files changed, 113 insertions(+), 2 deletions(-)
 create mode 100644 drivers/acpi/spcr.c

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 65fb483..5611eb6 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -77,6 +77,9 @@ config ACPI_DEBUGGER_USER
 
 endif
 
+config ACPI_SPCR_TABLE
+	bool
+
 config ACPI_SLEEP
 	bool
 	depends on SUSPEND || HIBERNATION
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 346101c..708b143 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_ACPI_EC_DEBUGFS)	+= ec_sys.o
 obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
 obj-$(CONFIG_ACPI_BGRT)		+= bgrt.o
 obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc_acpi.o
+obj-$(CONFIG_ACPI_SPCR_TABLE)	+= spcr.o
 obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
 
 # processor has its own "processor." module_param namespace
diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
new file mode 100644
index 0000000..0475840
--- /dev/null
+++ b/drivers/acpi/spcr.c
@@ -0,0 +1,77 @@
+/*
+ * Copyright (c) 2012, Intel Corporation
+ * Copyright (c) 2015, Red Hat, Inc.
+ * Copyright (c) 2015, 2016 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/console.h>
+#include <linux/kernel.h>
+
+struct spcr_table_handler_match_data {
+	struct console *console;
+	char **options;
+};
+
+static int spcr_table_handler_match(struct acpi_table_header *t, void *d)
+{
+	struct acpi_table_spcr *table = (struct acpi_table_spcr *)t;
+	struct spcr_table_handler_match_data *data = d;
+	int err;
+
+	if (table->header.revision < 2)
+		return -EOPNOTSUPP;
+
+	err = data->console->acpi_match(data->console, table);
+	if (err < 0)
+		return err;
+
+	if (data->options) {
+		switch (table->baud_rate) {
+		case 3:
+			*data->options = "9600";
+			break;
+		case 4:
+			*data->options = "19200";
+			break;
+		case 6:
+			*data->options = "57600";
+			break;
+		case 7:
+			*data->options = "115200";
+			break;
+		default:
+			*data->options = "";
+			break;
+		}
+	}
+
+	return err;
+}
+
+/**
+ * acpi_console_match - Check if console matches one specified by SPCR.
+ *
+ * @console:	console to match
+ * @options:	if the console matches, this will return options for the console
+ *		as in kernel command line
+ *
+ * Return: a non-error value if the console matches.
+ */
+int acpi_console_match(struct console *console, char **options)
+{
+	struct spcr_table_handler_match_data d = {
+		.console = console,
+		.options = options,
+	};
+
+	if (acpi_disabled || !console->acpi_match || console_set_on_cmdline)
+		return -ENODEV;
+
+	return acpi_table_parse2(ACPI_SIG_SPCR, spcr_table_handler_match, &d);
+}
diff --git a/include/linux/console.h b/include/linux/console.h
index ea731af..dcc2b59 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -15,6 +15,7 @@
 #define _LINUX_CONSOLE_H_ 1
 
 #include <linux/types.h>
+#include <linux/errno.h>
 
 struct vc_data;
 struct console_font_op;
@@ -117,6 +118,7 @@ static inline int con_debug_leave(void)
 #define CON_BRL		(32) /* Used for a braille device */
 #define CON_EXTENDED	(64) /* Use the extended output format a la /dev/kmsg */
 
+struct acpi_table_spcr;
 struct console {
 	char	name[16];
 	void	(*write)(struct console *, const char *, unsigned);
@@ -125,6 +127,7 @@ struct console {
 	void	(*unblank)(void);
 	int	(*setup)(struct console *, char *);
 	int	(*match)(struct console *, char *name, int idx, char *options);
+	int	(*acpi_match)(struct console *, struct acpi_table_spcr *);
 	short	flags;
 	short	index;
 	int	cflag;
@@ -132,6 +135,15 @@ struct console {
 	struct	 console *next;
 };
 
+#ifdef CONFIG_ACPI_SPCR_TABLE
+int acpi_console_match(struct console *c, char **options);
+#else
+static inline int acpi_console_match(struct console *c, char **options)
+{
+	return -ENODEV;
+}
+#endif
+
 /*
  * for_each_console() allows you to iterate on each console
  */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 0cc9542..5c270cf 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2572,8 +2572,26 @@ void register_console(struct console *newcon)
 		break;
 	}
 
-	if (!(newcon->flags & CON_ENABLED))
-		return;
+	if (!(newcon->flags & CON_ENABLED)) {
+		char *opts;
+		int err;
+
+		if (newcon->flags & CON_BOOT)
+			return;
+
+		err = acpi_console_match(newcon, &opts);
+		if (err < 0)
+			return;
+
+		if (newcon->index < 0)
+			newcon->index = 0;
+
+		if (newcon->setup && newcon->setup(newcon, opts) != 0)
+			return;
+
+		newcon->flags |= CON_ENABLED | CON_CONSDEV;
+		has_preferred = true;
+	}
 
 	/*
 	 * If we have a bootconsole, and are switching to a real console,
-- 
2.7.0

[PATCH v2 5/9] ACPI: enable ACPI_SPCR_TABLE on ARM64

[Aleksey Makarov]

SBBR mentions SPCR as a mandatory ACPI table.
So enable it for ARM64

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
Tested-by: Christopher Covington <cov@codeaurora.org>
---
 arch/arm64/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 6bb21d8..75739cd 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -3,6 +3,7 @@ config ARM64
 	select ACPI_CCA_REQUIRED if ACPI
 	select ACPI_GENERIC_GSI if ACPI
 	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
+	select ACPI_SPCR_TABLE if ACPI
 	select ARCH_HAS_DEVMEM_IS_ALLOWED
 	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
 	select ARCH_HAS_ELF_RANDOMIZE
-- 
2.7.0

[PATCH v2 6/9] ACPI: add definition of DBG2 subtypes

[Aleksey Makarov]

The recent version of Microsoft Debug Port Table 2 (DBG2) [1]
specifies additional serial debug port subtypes.  These constants
are also referred by Serial Port Console Redirection Table (SPCR) [2]

Add these constants.

[1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639131(v=vs.85).aspx
[2] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
Tested-by: Christopher Covington <cov@codeaurora.org>
---
 include/acpi/actbl2.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index a4ef625..e9930ab 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -371,6 +371,11 @@ struct acpi_dbg2_device {
 
 #define ACPI_DBG2_16550_COMPATIBLE  0x0000
 #define ACPI_DBG2_16550_SUBSET      0x0001
+#define ACPI_DBG2_ARM_PL011         0x0003
+#define ACPI_DBG2_ARM_SBSA_32BIT    0x000d
+#define ACPI_DBG2_ARM_SBSA_GENERIC  0x000e
+#define ACPI_DBG2_ARM_DCC           0x000f
+#define ACPI_DBG2_BCM2835           0x0010
 
 #define ACPI_DBG2_1394_STANDARD     0x0000
 
-- 
2.7.0

[PATCH v2 7/9] serial: pl011: add acpi_match

[Aleksey Makarov]

Add an implementation of acpi_match() to the pl011 driver.
It allows console to check if it matches the console specified by
ACPI SPCR table.

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
Tested-by: Christopher Covington <cov@codeaurora.org>
---
 drivers/tty/serial/amba-pl011.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 500232a..f613bff 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2285,12 +2285,34 @@ static int __init pl011_console_setup(struct console *co, char *options)
 	return uart_set_options(&uap->port, co, baud, parity, bits, flow);
 }
 
+static int __init pl011_console_acpi_match(struct console *co,
+					   struct acpi_table_spcr *spcr)
+{
+	struct uart_amba_port *uap = amba_ports[co->index < 0 ? 0 : co->index];
+
+	if (uap->vendor->access_32b) {
+		if (spcr->interface_type != ACPI_DBG2_ARM_SBSA_32BIT)
+			return -ENODEV;
+	} else {
+		if (spcr->interface_type != ACPI_DBG2_ARM_PL011 &&
+		    spcr->interface_type != ACPI_DBG2_ARM_SBSA_GENERIC)
+			return -ENODEV;
+	}
+
+	if (spcr->serial_port.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY ||
+	    spcr->serial_port.address != (u64)uap->port.mapbase)
+		return -ENODEV;
+
+	return 0;
+}
+
 static struct uart_driver amba_reg;
 static struct console amba_console = {
 	.name		= "ttyAMA",
 	.write		= pl011_console_write,
 	.device		= uart_console_device,
 	.setup		= pl011_console_setup,
+	.acpi_match	= pl011_console_acpi_match,
 	.flags		= CON_PRINTBUFFER,
 	.index		= -1,
 	.data		= &amba_reg,
-- 
2.7.0

[PATCH v2 8/9] serial: 8250: add acpi_match

[Aleksey Makarov]

From: Mark Salter <msalter@redhat.com>

Add an implementation of acpi_match() to the 8250 driver.
It allows console to check if it matches the console specified by
ACPI SPCR table.

Signed-off-by: Mark Salter <msalter@redhat.com>
Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
---
 drivers/tty/serial/8250/8250_core.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 7775221..059338a 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -40,6 +40,7 @@
 #ifdef CONFIG_SPARC
 #include <linux/sunserialcore.h>
 #endif
+#include <linux/acpi.h>
 
 #include <asm/irq.h>
 
@@ -669,12 +670,34 @@ static int univ8250_console_match(struct console *co, char *name, int idx,
 	return -ENODEV;
 }
 
+static int __init univ8250_console_acpi_match(struct console *co,
+					      struct acpi_table_spcr *spcr)
+{
+	int index = co->index >= 0 ? co->index : 0;
+	struct uart_port *port = &serial8250_ports[index].port;
+
+	if (spcr->interface_type != ACPI_DBG2_16550_SUBSET &&
+	    spcr->interface_type != ACPI_DBG2_16550_COMPATIBLE)
+		return -ENODEV;
+
+	if (spcr->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY &&
+	    spcr->serial_port.address == (u64)port->mapbase)
+		return 0;
+
+	if (spcr->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_IO &&
+	    spcr->serial_port.address == (u64)port->iobase)
+		return 0;
+
+	return -ENODEV;
+}
+
 static struct console univ8250_console = {
 	.name		= "ttyS",
 	.write		= univ8250_console_write,
 	.device		= uart_console_device,
 	.setup		= univ8250_console_setup,
 	.match		= univ8250_console_match,
+	.acpi_match	= univ8250_console_acpi_match,
 	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
 	.index		= -1,
 	.data		= &serial8250_reg,
-- 
2.7.0

[PATCH v2 9/9] serial: pl011: use SPCR to setup 32-bit access

[Aleksey Makarov]

Some implementations of ARM SBSA serial port hardware require that access
to the registers should be 32-bit.  Unfortunately, the only way for
the driver to tell if it's the case is to use the data from ACPI SPCR/DBG2
tables.  In this case the value of the 'Interface Type' field of the SPCR
table is ACPI_DBG2_ARM_SBSA_32BIT.

How this value is described in the DBG2 spec:
"(deprecated) ARM SBSA (2.x only) Generic UART supporting only
32-bit accesses"

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
Tested-by: Christopher Covington <cov@codeaurora.org>
---
 drivers/acpi/spcr.c             | 29 +++++++++++++++++++++++++++++
 drivers/tty/serial/amba-pl011.c |  2 ++
 include/linux/console.h         |  5 +++++
 3 files changed, 36 insertions(+)

diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
index 0475840..df1741f 100644
--- a/drivers/acpi/spcr.c
+++ b/drivers/acpi/spcr.c
@@ -75,3 +75,32 @@ int acpi_console_match(struct console *console, char **options)
 
 	return acpi_table_parse2(ACPI_SIG_SPCR, spcr_table_handler_match, &d);
 }
+
+static int spcr_table_handler_32_bit(struct acpi_table_header *t,
+				     void *data)
+{
+	struct acpi_table_spcr *table = (struct acpi_table_spcr *)t;
+
+	return table->interface_type == ACPI_DBG2_ARM_SBSA_32BIT;
+}
+
+/**
+ * acpi_console_sbsa_32bit - Tell if SPCR specifies 32-bit SBSA.
+ *
+ * Some implementations of ARM SBSA serial port hardware require that access
+ * to the registers should be 32-bit.  Unfortunately, the only way for
+ * the driver to tell if it's the case is to use the data from ACPI SPCR/DBG2
+ * tables.  In this case the value of the 'Interface Type' field of the SPCR
+ * table is ACPI_DBG2_ARM_SBSA_32BIT.
+ *
+ * Return: true if access should be 32-bit wide.
+ */
+bool acpi_console_sbsa_32bit(void)
+{
+	if (acpi_disabled)
+		return false;
+
+	return acpi_table_parse2(ACPI_SIG_SPCR,
+				 spcr_table_handler_32_bit, NULL) > 0;
+}
+EXPORT_SYMBOL(acpi_console_sbsa_32bit);
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index f613bff..699a3ce 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2563,6 +2563,8 @@ static int sbsa_uart_probe(struct platform_device *pdev)
 		baudrate = 115200;
 	}
 
+	vendor_sbsa.access_32b = acpi_console_sbsa_32bit();
+
 	portnr = pl011_find_free_port();
 	if (portnr < 0)
 		return portnr;
diff --git a/include/linux/console.h b/include/linux/console.h
index dcc2b59..027ef50 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -137,11 +137,16 @@ struct console {
 
 #ifdef CONFIG_ACPI_SPCR_TABLE
 int acpi_console_match(struct console *c, char **options);
+bool acpi_console_sbsa_32bit(void);
 #else
 static inline int acpi_console_match(struct console *c, char **options)
 {
 	return -ENODEV;
 }
+static inline bool acpi_console_sbsa_32bit(void)
+{
+	return false;
+}
 #endif
 
 /*
-- 
2.7.0

Re: [PATCH v2 3/9] ACPI: introduce acpi_table_parse2()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1919 bytes --]

Hi Aleksey,

[auto build test ERROR on pm/linux-next]
[also build test ERROR on v4.5-rc3 next-20160212]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Aleksey-Makarov/ACPI-parse-the-SPCR-table/20160213-015350
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-tinyconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   arch/x86/kernel/i8259.o: In function `acpi_table_parse2':
>> i8259.c:(.text+0x2c4): multiple definition of `acpi_table_parse2'
   arch/x86/kernel/setup.o:setup.c:(.text+0x3): first defined here
   arch/x86/kernel/irqinit.o: In function `acpi_table_parse2':
   irqinit.c:(.text+0x0): multiple definition of `acpi_table_parse2'
   arch/x86/kernel/setup.o:setup.c:(.text+0x3): first defined here
   arch/x86/kernel/bootflag.o: In function `acpi_table_parse2':
   bootflag.c:(.text+0x0): multiple definition of `acpi_table_parse2'
   arch/x86/kernel/setup.o:setup.c:(.text+0x3): first defined here
   arch/x86/kernel/e820.o: In function `acpi_table_parse2':
   e820.c:(.text+0x0): multiple definition of `acpi_table_parse2'
   arch/x86/kernel/setup.o:setup.c:(.text+0x3): first defined here
   arch/x86/kernel/pci-dma.o: In function `acpi_table_parse2':
   pci-dma.c:(.text+0x0): multiple definition of `acpi_table_parse2'
   arch/x86/kernel/setup.o:setup.c:(.text+0x3): first defined here
   arch/x86/kernel/rtc.o: In function `acpi_table_parse2':
   rtc.c:(.text+0x43): multiple definition of `acpi_table_parse2'
   arch/x86/kernel/setup.o:setup.c:(.text+0x3): first defined here

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6205 bytes --]

Re: [PATCH v2 3/9] ACPI: introduce acpi_table_parse2()

[Greg Kroah-Hartman]

On Fri, Feb 12, 2016 at 08:43:34PM +0300, Aleksey Makarov wrote:
> The function acpi_table_parse() has some problems:
> 1 It can be called only from __init code
> 2 It does not pass any data to the handler
> 3 It just throws out the value returned from the handler
> 
> These issues are addressed in this patch

Why not just fix acpi_table_parse(), like you have, and not add a new
API call with a "2" at the end of it.  That seems crazy to try to
maintain that level of apis.

But I'm not the acpi maintainer(s), so it's their call...

good luck,

greg k-h

Re: [PATCH v2 4/9] ACPI: parse SPCR and enable matching console

[Greg Kroah-Hartman]

On Fri, Feb 12, 2016 at 08:43:35PM +0300, Aleksey Makarov wrote:
> 'ARM Server Base Boot Requiremets' [1] mentions SPCR (Serial Port
> Console Redirection Table) [2] as a mandatory ACPI table that
> specifies the configuration of serial console.
> 
> Parse this table and check if any registered console match the
> description.  If it does, enable that console.
> 
> In the original submission [3] the ACPI devices are traversed to find
> one that has the same address as specified by SPCR. Then while
> registering a serial device it is checked if it has an ACPI companion
> and it is the same device that was found at traversal. If so,
> add_preferred_console() was used to specify the device as preferred.
> The console name and index are known at the time of registration of
> serial device.
> 
> The problem is that SPCR can specify the console not only by the base
> address but also with PCI Device ID/PCI Vendor ID or PCI Bus
> Number/PCI Device Number.  In general, there is no way to get serial
> name and index from the SPCR table.
> 
> To implement that, introduce a new member
> 
>         int (*acpi_match)(struct console *, struct acpi_table_spcr *)
> 
> of struct console.  It allows drivers to check if they provide a
> matching console device.
> 
> [1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044a/index.html
> [2] http://msdn.microsoft.com/en-us/library/windows/hardware/dn639131(v=vs.85).aspx
> [3] https://lkml.kernel.org/g/1441716217-23786-1-git-send-email-leif.lindholm@linaro.org
> 
> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
> ---
>  drivers/acpi/Kconfig    |  3 ++
>  drivers/acpi/Makefile   |  1 +
>  drivers/acpi/spcr.c     | 77 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/console.h | 12 ++++++++
>  kernel/printk/printk.c  | 22 ++++++++++++--
>  5 files changed, 113 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/acpi/spcr.c
> 
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 65fb483..5611eb6 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -77,6 +77,9 @@ config ACPI_DEBUGGER_USER
>  
>  endif
>  
> +config ACPI_SPCR_TABLE
> +	bool
> +
>  config ACPI_SLEEP
>  	bool
>  	depends on SUSPEND || HIBERNATION
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 346101c..708b143 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -81,6 +81,7 @@ obj-$(CONFIG_ACPI_EC_DEBUGFS)	+= ec_sys.o
>  obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
>  obj-$(CONFIG_ACPI_BGRT)		+= bgrt.o
>  obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc_acpi.o
> +obj-$(CONFIG_ACPI_SPCR_TABLE)	+= spcr.o
>  obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
>  
>  # processor has its own "processor." module_param namespace
> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
> new file mode 100644
> index 0000000..0475840
> --- /dev/null
> +++ b/drivers/acpi/spcr.c
> @@ -0,0 +1,77 @@
> +/*
> + * Copyright (c) 2012, Intel Corporation
> + * Copyright (c) 2015, Red Hat, Inc.
> + * Copyright (c) 2015, 2016 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/console.h>
> +#include <linux/kernel.h>
> +
> +struct spcr_table_handler_match_data {
> +	struct console *console;
> +	char **options;
> +};
> +
> +static int spcr_table_handler_match(struct acpi_table_header *t, void *d)
> +{
> +	struct acpi_table_spcr *table = (struct acpi_table_spcr *)t;
> +	struct spcr_table_handler_match_data *data = d;
> +	int err;
> +
> +	if (table->header.revision < 2)
> +		return -EOPNOTSUPP;
> +
> +	err = data->console->acpi_match(data->console, table);
> +	if (err < 0)
> +		return err;
> +
> +	if (data->options) {
> +		switch (table->baud_rate) {
> +		case 3:
> +			*data->options = "9600";
> +			break;
> +		case 4:
> +			*data->options = "19200";
> +			break;
> +		case 6:
> +			*data->options = "57600";
> +			break;
> +		case 7:
> +			*data->options = "115200";
> +			break;
> +		default:
> +			*data->options = "";
> +			break;
> +		}
> +	}
> +
> +	return err;
> +}
> +
> +/**
> + * acpi_console_match - Check if console matches one specified by SPCR.
> + *
> + * @console:	console to match
> + * @options:	if the console matches, this will return options for the console
> + *		as in kernel command line
> + *
> + * Return: a non-error value if the console matches.
> + */
> +int acpi_console_match(struct console *console, char **options)
> +{
> +	struct spcr_table_handler_match_data d = {
> +		.console = console,
> +		.options = options,
> +	};
> +
> +	if (acpi_disabled || !console->acpi_match || console_set_on_cmdline)
> +		return -ENODEV;
> +
> +	return acpi_table_parse2(ACPI_SIG_SPCR, spcr_table_handler_match, &d);
> +}
> diff --git a/include/linux/console.h b/include/linux/console.h
> index ea731af..dcc2b59 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -15,6 +15,7 @@
>  #define _LINUX_CONSOLE_H_ 1
>  
>  #include <linux/types.h>
> +#include <linux/errno.h>
>  
>  struct vc_data;
>  struct console_font_op;
> @@ -117,6 +118,7 @@ static inline int con_debug_leave(void)
>  #define CON_BRL		(32) /* Used for a braille device */
>  #define CON_EXTENDED	(64) /* Use the extended output format a la /dev/kmsg */
>  
> +struct acpi_table_spcr;
>  struct console {
>  	char	name[16];
>  	void	(*write)(struct console *, const char *, unsigned);
> @@ -125,6 +127,7 @@ struct console {
>  	void	(*unblank)(void);
>  	int	(*setup)(struct console *, char *);
>  	int	(*match)(struct console *, char *name, int idx, char *options);
> +	int	(*acpi_match)(struct console *, struct acpi_table_spcr *);

Are we expected to add a new 'match' callback for every single type of
firmware out there?  I really don't want to see that, isn't there some
other way to do this without having to add this?

Why not just hook into the match callback instead somehow?

thanks,

greg k-h

Re: [PATCH v2 8/9] serial: 8250: add acpi_match

[Greg Kroah-Hartman]

On Fri, Feb 12, 2016 at 08:43:39PM +0300, Aleksey Makarov wrote:
> From: Mark Salter <msalter@redhat.com>
> 
> Add an implementation of acpi_match() to the 8250 driver.
> It allows console to check if it matches the console specified by
> ACPI SPCR table.
> 
> Signed-off-by: Mark Salter <msalter@redhat.com>
> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
> ---
>  drivers/tty/serial/8250/8250_core.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 7775221..059338a 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -40,6 +40,7 @@
>  #ifdef CONFIG_SPARC
>  #include <linux/sunserialcore.h>
>  #endif
> +#include <linux/acpi.h>
>  
>  #include <asm/irq.h>
>  
> @@ -669,12 +670,34 @@ static int univ8250_console_match(struct console *co, char *name, int idx,
>  	return -ENODEV;
>  }
>  
> +static int __init univ8250_console_acpi_match(struct console *co,
> +					      struct acpi_table_spcr *spcr)
> +{
> +	int index = co->index >= 0 ? co->index : 0;
> +	struct uart_port *port = &serial8250_ports[index].port;
> +
> +	if (spcr->interface_type != ACPI_DBG2_16550_SUBSET &&
> +	    spcr->interface_type != ACPI_DBG2_16550_COMPATIBLE)
> +		return -ENODEV;
> +
> +	if (spcr->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY &&
> +	    spcr->serial_port.address == (u64)port->mapbase)
> +		return 0;
> +
> +	if (spcr->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_IO &&
> +	    spcr->serial_port.address == (u64)port->iobase)
> +		return 0;
> +
> +	return -ENODEV;
> +}

Ick, I don't like this, you are doing almost the same thing that
univ8250_console_match does, yet you have to know the internals of the
acpi_table_spcr structure, which is insane (again, do you want to do
this for every firmware type?)

Isn't this something that resources are supposed to handle for us?  What
happened to getting this type of information from the firmware in a
non-firmware-specific way to pass to the drivers?  Please work on that
instead, then you don't have to modify each and every single driver you
want to work with your system, they will all "just work" automagically.

Please redo this series, as it is, I don't like it at all.

thanks,

greg k-h

RE: [PATCH v2 6/9] ACPI: add definition of DBG2 subtypes

[Moore, Robert]

OK, we'll take these for the next release of ACPICA.
Thanks,
Bob


> -----Original Message-----
> From: Aleksey Makarov [mailto:aleksey.makarov@linaro.org]
> Sent: Friday, February 12, 2016 9:44 AM
> To: linux-acpi@vger.kernel.org
> Cc: linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; Aleksey Makarov; Russell King; Greg Kroah-
> Hartman; Rafael J . Wysocki; Leif Lindholm; Graeme Gregory; Al Stone;
> Christopher Covington; Moore, Robert; Zheng, Lv; Wysocki, Rafael J; Len
> Brown; devel@acpica.org
> Subject: [PATCH v2 6/9] ACPI: add definition of DBG2 subtypes
> 
> The recent version of Microsoft Debug Port Table 2 (DBG2) [1] specifies
> additional serial debug port subtypes.  These constants are also referred
> by Serial Port Console Redirection Table (SPCR) [2]
> 
> Add these constants.
> 
> [1] https://msdn.microsoft.com/en-
> us/library/windows/hardware/dn639131(v=vs.85).aspx
> [2] https://msdn.microsoft.com/en-
> us/library/windows/hardware/dn639132(v=vs.85).aspx
> 
> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
> Tested-by: Christopher Covington <cov@codeaurora.org>
> ---
>  include/acpi/actbl2.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h index
> a4ef625..e9930ab 100644
> --- a/include/acpi/actbl2.h
> +++ b/include/acpi/actbl2.h
> @@ -371,6 +371,11 @@ struct acpi_dbg2_device {
> 
>  #define ACPI_DBG2_16550_COMPATIBLE  0x0000
>  #define ACPI_DBG2_16550_SUBSET      0x0001
> +#define ACPI_DBG2_ARM_PL011         0x0003
> +#define ACPI_DBG2_ARM_SBSA_32BIT    0x000d
> +#define ACPI_DBG2_ARM_SBSA_GENERIC  0x000e
> +#define ACPI_DBG2_ARM_DCC           0x000f
> +#define ACPI_DBG2_BCM2835           0x0010
> 
>  #define ACPI_DBG2_1394_STANDARD     0x0000
> 
> --
> 2.7.0

Re: [PATCH v2 3/9] ACPI: introduce acpi_table_parse2()

[Rafael J. Wysocki]

On Fri, Feb 12, 2016 at 6:43 PM, Aleksey Makarov
<aleksey.makarov@linaro.org> wrote:
> The function acpi_table_parse() has some problems:
> 1 It can be called only from __init code
> 2 It does not pass any data to the handler
> 3 It just throws out the value returned from the handler

So why are those problems?

> These issues are addressed in this patch

How are they addressed?

Thanks,
Rafael

Re: [PATCH v2 3/9] ACPI: introduce acpi_table_parse2()

[Rafael J. Wysocki]

On Fri, Feb 12, 2016 at 7:51 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, Feb 12, 2016 at 08:43:34PM +0300, Aleksey Makarov wrote:
>> The function acpi_table_parse() has some problems:
>> 1 It can be called only from __init code
>> 2 It does not pass any data to the handler
>> 3 It just throws out the value returned from the handler
>>
>> These issues are addressed in this patch
>
> Why not just fix acpi_table_parse(), like you have, and not add a new
> API call with a "2" at the end of it.  That seems crazy to try to
> maintain that level of apis.
>
> But I'm not the acpi maintainer(s), so it's their call...

The ACPI maintainer agrees.

Thanks,
Rafael

Re: [PATCH v2 3/9] ACPI: introduce acpi_table_parse2()

[Aleksey Makarov]

On 02/13/2016 02:07 AM, Rafael J. Wysocki wrote:
> On Fri, Feb 12, 2016 at 6:43 PM, Aleksey Makarov
> <aleksey.makarov@linaro.org> wrote:
>> The function acpi_table_parse() has some problems:
>> 1 It can be called only from __init code
>> 2 It does not pass any data to the handler
>> 3 It just throws out the value returned from the handler
> 
> So why are those problems?

1.  We need this function to be non-__init because we need access to
some tables at unpredictable time--it may be before or after
the init functions are removed.  For example, SPCR (Serial Port Console
Redirection) table is needed each time a new console is registered.
It can be quite early (console_initcall) or when a module is inserted.

2. Having an additional pointer to void is very useful because it allows
drivers to pass (local) data to the handler.  Without this, we would need 
to have global data and a mutex as it was done in drivers/acpi/scan.c
(vars ape, acpi_probe_count, acpi_probe_lock).

3. Passing the value returned from the callback is less important as it 
could be modelled by having a (return) field in the data passed to 
the handler.  It is very convenient though.

I use these three properties in my next patch of this series.

>> These issues are addressed in this patch
> 
> How are they addressed?

A new function acpi_table_parse2() has been created that is just 
the old acpi_table_parse() without __init, taking the handler with 
additional data pointer and respecting the value returned from 
the handler.  The old acpi_table_parse() was implemented witht this 
new function.  Dropping __init was made possible by the previous 
patch where the attribute of the function early_acpi_os_unmap_memory()
was changed from __init to __ref and by dropping __init from 
acpi_apic_instance.

I understand why this approach is not good and I am going to fix it.

Thank you
Aleksey Makarov

Re: [PATCH v2 3/9] ACPI: introduce acpi_table_parse2()

[Aleksey Makarov]

Hi Rafael, 

Thank you for review.

On 02/13/2016 02:08 AM, Rafael J. Wysocki wrote:
> On Fri, Feb 12, 2016 at 7:51 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> On Fri, Feb 12, 2016 at 08:43:34PM +0300, Aleksey Makarov wrote:
>>> The function acpi_table_parse() has some problems:
>>> 1 It can be called only from __init code
>>> 2 It does not pass any data to the handler
>>> 3 It just throws out the value returned from the handler
>>>
>>> These issues are addressed in this patch
>>
>> Why not just fix acpi_table_parse(), like you have, and not add a new
>> API call with a "2" at the end of it.  That seems crazy to try to
>> maintain that level of apis.
>>
>> But I'm not the acpi maintainer(s), so it's their call...
> 
> The ACPI maintainer agrees.

I see. 

How would you prefer it to be fixed:

1. Change the signature/implementation of acpi_table_parse(). 
CON: It would require extensive changes through all the kernel,
which I am not sure I will be able to test (but these changes are
just adding an unused arg to the handler + checking that the 
return value is consistent)

OR

2. Have a local implementation of the function like acpi_table_parse2()
CON: A bit of code duplication.

Thank you
Aleksey Makarov