Re: [PATCH v5 1/2] ACPI: Add early console framework for DBGP/DBG2.

[Konrad Rzeszutek Wilk <konrad@kernel.org>]

On Tue, Oct 09, 2012 at 10:36:56AM +0800, Lv Zheng wrote:
> Microsoft Debug Port Table (DBGP or DBG2) is used by the Windows SoC
> platforms to describe their debugging facilities.
> DBGP: http://msdn.microsoft.com/en-us/windows/hardware/hh134821
> DBG2: http://msdn.microsoft.com/en-us/library/windows/hardware/hh673515
> 
> This patch enables the DBGP/DBG2 debug ports as Linux early console
> launcher.  Individual early console drivers are also needed to get the
> early kernel messages dumped on the consoles.  For example, to use the
> SPI UART early console for the Low Power Intel Architecture (LPIA)
> platforms, you need to enable the following kernel configurations:
>   CONFIG_EARLY_PRINTK_ACPI=y
>   CONFIG_EARLY_PRINTK_INTEL_MID_SPI=y
> Then you need to append the following kernel parameter to the kernel
> command line in your the boot loader configuration file:
>   earlyprintk=acpi
> 
> There is a dilemma in designing this patch set.  Let me describe it in
> details.
> There should be three steps to enable an early console for an operating
> system:
> 1. Probe: In this stage, the Linux kernel can detect the early consoles
>           and the base address of their register block can be determined.
>           This can be done by parsing the descriptors in the ACPI DBGP/DBG2
>           tables.  Note that acpi_table_init() must be called before
>           parsing.
> 2. Setup: In this stage, the Linux kernel can apply user specified
>           configuration options (ex. baudrate of serial ports) for the
>           early consoles.  This is done by parsing the early parameters
>           passed to the kernel from the boot loaders.  Note that
>           parse_early_params() is called very early to allow parameters to
>           be passed to other kernel subsystems.
> 3. Start: In this stage, the Linux kernel can make the consoles ready to
>           output logs.  Since the early consoles are always used for the
>           kernel boot up debugging, this must be done as early as possible
>           to arm the kernel with the highest testability for other kernel
>           subsystems.  Note that, this stage happens when the
>           register_console() is called.
> The preferred sequence for the above steps is:
>    +-----------------+    +-------------------+    +--------------------+
>    | ACPI DBGP PROBE | -> | EARLY_PARAM SETUP | -> | EARLY_RPINTK START |
>    +-----------------+    +-------------------+    +--------------------+
> But unfortunately, in the current x86 implementation, early parameters and
> early printk initialization are called before acpi_table_init() which
> depends on the early memory mapping facility.
> There are some choices for me to design this patch set:
> 1. Invoking acpi_table_init() before parse_early_param() to maintain the
>    sequence:
>    +-----------------+    +-------------------+    +--------------------+
>    | ACPI DBGP PROBE | -> | EARLY_PARAM SETUP | -> | EARLY_RPINTK START |
>    +-----------------+    +-------------------+    +--------------------+
>    This requires other subsystem maintainers' review to ensure no
>    regressions will be introduced.  At the first glance, I found there
>    might be problems for the EFI subsystsm:
>    The EFI boot services and runtime services are mixed up in the x86
>    specific initialization process before the ACPI table initialization.
>    Things are much worse that you even cannot disable the runtime services
>    while still allow the boot services codes to be executed in the kernel
>    compilation stage.  Enabling the early consoles after the ACPI table
>    initialization will make it difficult to debug the runtime BIOS bugs.
>    If any progress is made to the kernel boot sequences, please let me
>    know.  I'll be willing to redesign the ACPI DBGP/DBG2 console probing
>    facility.  You can reach me at <lv.zheng@intel.com> and
>    <zetalog@gmail.com>.
> 2. Modifying the above sequece to make it look like:
>    +-------------------+    +-----------------+    +--------------------+
>    | EARLY_PARAM SETUP | -> | ACPI DBGP PROBE | -> | EARLY_RPINTK START |
>    +-------------------+    +-----------------+    +--------------------+
>    Early consoles started in this style will lose part of the testability
>    in the kernel boot up sequence.  If the system does not crash very
>    early, developers still can see the bufferred kernel outputs when the
>    register_console() is called.
>    Current early console implementations need to be modified to be
>    compatible with this design.  The original codes need to be split up
>    into tow parts:
>    1. Detecting hardware.  This part can be called in the PROBE stage.
>    2. Applying user parameters.  This part can be called in the SETUP
>       stage.
>    Individual early console drver maintainers need to be involved to avoid
>    regressions that might occur if we do things in this way.  And the
>    maintainers can offer better tests than I can do.
> 3. Introducing a brand new debugging facility that does not relate to the
>    current early console implementation to allow the early consoles to be
>    automatically detected.
>    +-------------------+    +--------------------+
>    | EARLY_PARAM SETUP | -> | EARLY_RPINTK START |
>    +-------------------+    +--------------------+
>    +-----------------+    +--------------------+
>    | ACPI DBGP PROBE | -> | EARLY_RPINTK START |
>    +-----------------+    +--------------------+
>    Early consoles started in this style will lose part of the testability
>    in the kernel boot up sequence.  If the system does not crash very
>    early, developers still can see the bufferred kernel outputs when the
>    register_console() is called.
>    Comparing to the solution 2, we can notice that the user configuration
>    can not be applied to the registered early consoles in this way as the
>    acpi_table_init() is still called after the parse_early_param().
>    Instead, the early consoles should derive the hardware settings used in
>    the BIOS/bootloaders.
>    This is what the patch set has done to enable this new usage model.
>    It is known as "ACPI early console launcher mode".
>    As a launcher, ACPI DBGP will not actually output kernel messages
>    without the real early console drivers, that's why the
>    CONFIG_EARLY_PRINTK_INTEL_MID_SPI still need to be enabled along with
>    the CONFIG_EARLY_PRINTK_ACPI.
>    In order to disable this facility by default and enable it at runtime,
>    an kernel parameter "earlyprintk=acpi" is introduced.  This makes the
>    actual sequence look like:
>    +-------------------+    +--------------------+
>    | EARLY_PARAM SETUP | -> | EARLY_RPINTK START |
>    +-------------------+    +....................+
>                             | ACPI DBGP LAUNCH   | ->
>                             +--------------------+
>       +-----------------+    +--------------------+
>    -> | ACPI DBGP PROBE | -> | EARLY_PRINTK START |
>       +-----------------+    +--------------------+
> 
> Version 1:
>  1. Enables single DBG2 debug port support.
> Version 2:
>  1. Applies Rui's comments.
> Version 3:
>  1. Applies Len's comments (earlycon should be disabled by default).
>  2. Enables single DBG2 debug ports support.
> Version 4:
>  1. Fixes the CodingStyle issues reported by checkpatch.pl.
>  2. Enables single DBGP debug port support.
> Version 5:
>  1. Enables multiple DBG2 debug ports support.
>  2. Applies Konrad's comments (pr_debug should be used in earlycon).
>  3. Changes kstrtoul back to simple_strtoul.
> 
> Known Issues:
> 1. The simple_strtoul function cannot be replaced by the kstrtoul in
>    the x86 specific booting codes.  The kstrtoul will return error on
>    strings like "acpi0,keep".  This will leave one CodingStyle issue
>    reported by the checkpatch.pl.
> 
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> Reviewed-by: Len Brown <len.brown@intel.com>
> Reviewed-by: Rui Zhang <rui.zhang@intel.com>
> Reviewed-by: Ying Huang <ying.huang@intel.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad@kernel.org>

Please don't include that unless I (or other folks looking at your code)
say explicitly 'Acked' or 'Reviewed-by'

> ---
>  Documentation/kernel-parameters.txt |    1 +
>  arch/x86/Kconfig.debug              |   15 +++
>  arch/x86/kernel/acpi/boot.c         |    1 +
>  arch/x86/kernel/early_printk.c      |   13 +++
>  drivers/acpi/Makefile               |    2 +
>  drivers/acpi/early_printk.c         |  173 +++++++++++++++++++++++++++++++++++
>  include/linux/acpi.h                |   22 +++++
>  7 files changed, 227 insertions(+)
>  create mode 100644 drivers/acpi/early_printk.c
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index ad7e2e5..f656765 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -763,6 +763,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			earlyprintk=serial[,ttySn[,baudrate]]
>  			earlyprintk=ttySn[,baudrate]
>  			earlyprintk=dbgp[debugController#]
> +			earlyprintk=acpi[debugController#]
>  
>  			Append ",keep" to not disable it when the real console
>  			takes over.
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index b322f12..5778082 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -59,6 +59,21 @@ config EARLY_PRINTK_DBGP
>  	  with klogd/syslogd or the X server. You should normally N here,
>  	  unless you want to debug such a crash. You need usb debug device.
>  
> +config EARLY_PRINTK_ACPI
> +	bool "Early printk launcher via ACPI debug port tables"
> +	depends on EARLY_PRINTK && ACPI
> +	---help---
> +	  Write kernel log output directly into the debug ports described
> +	  in the ACPI tables known as DBGP and DBG2.
> +
> +	  To enable such debugging facilities, you need to enable this
> +	  configuration option and append the "earlyprintk=acpi" kernel
> +	  parameter through the boot loaders.  Please refer the
> +	  "Documentation/kernel-parameters.txt" for details.  Since this
> +	  is an early console launcher, you still need to enable actual
> +	  early console drivers that are suitable for your platform.
> +	  If in doubt, say "N".
> +
>  config DEBUG_STACKOVERFLOW
>  	bool "Check for stack overflows"
>  	depends on DEBUG_KERNEL
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index b2297e5..cc10ea5 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -1518,6 +1518,7 @@ void __init acpi_boot_table_init(void)
>  		return;
>  	}
>  
> +	acpi_early_console_probe();
>  	acpi_table_parse(ACPI_SIG_BOOT, acpi_parse_sbf);
>  
>  	/*
> diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
> index 9b9f18b..bf5b596 100644
> --- a/arch/x86/kernel/early_printk.c
> +++ b/arch/x86/kernel/early_printk.c
> @@ -200,6 +200,15 @@ static inline void early_console_register(struct console *con, int keep_early)
>  	register_console(early_console);
>  }
>  
> +#ifdef CONFIG_EARLY_PRINTK_ACPI
> +#include <linux/acpi.h>
> +
> +int __init __acpi_early_console_start(struct acpi_debug_port *info)
> +{
> +	return 0;
> +}
> +#endif
> +
>  static int __init setup_early_printk(char *buf)
>  {
>  	int keep;
> @@ -236,6 +245,10 @@ static int __init setup_early_printk(char *buf)
>  		if (!strncmp(buf, "dbgp", 4) && !early_dbgp_init(buf + 4))
>  			early_console_register(&early_dbgp_console, keep);
>  #endif
> +#ifdef CONFIG_EARLY_PRINTK_ACPI
> +		if (!strncmp(buf, "acpi", 4))
> +			acpi_early_console_launch(buf + 4, keep);
> +#endif
>  #ifdef CONFIG_HVC_XEN
>  		if (!strncmp(buf, "xen", 3))
>  			early_console_register(&xenboot_console, keep);
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 47199e2..99dbd64 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -46,6 +46,8 @@ ifdef CONFIG_ACPI_VIDEO
>  acpi-y				+= video_detect.o
>  endif
>  
> +obj-$(CONFIG_EARLY_PRINTK_ACPI)	+= early_printk.o
> +
>  # These are (potentially) separate modules
>  obj-$(CONFIG_ACPI_AC) 		+= ac.o
>  obj-$(CONFIG_ACPI_BUTTON)	+= button.o
> diff --git a/drivers/acpi/early_printk.c b/drivers/acpi/early_printk.c
> new file mode 100644
> index 0000000..3e5c1f3
> --- /dev/null
> +++ b/drivers/acpi/early_printk.c
> @@ -0,0 +1,173 @@
> +/*
> + * acpi/early_printk.c - ACPI Boot-Time Debug Ports
> + *
> + * Copyright (c) 2012, Intel Corporation
> + * Author: Lv Zheng <lv.zheng@intel.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +
> +#define DEBUG

That should not be the default case.

> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/acpi.h>
> +#include <linux/bitmap.h>
> +#include <linux/bootmem.h>
> +
> +#define MAX_ACPI_DBG_PORTS	16
> +
> +DECLARE_BITMAP(acpi_early_flags, MAX_ACPI_DBG_PORTS);

static?

> +int acpi_early_enabled;

__read_mostly and you could also make it a bool.


> +
> +static int acpi_early_console_enable(u8 port, int keep)
> +{
> +	if (port >= MAX_ACPI_DBG_PORTS)
> +		return -ENODEV;
> +
> +	set_bit(port, acpi_early_flags);
> +	if (keep)
> +		set_bit(port+MAX_ACPI_DBG_PORTS, acpi_early_flags);


Huh? The bitmap is up to MAX_ACPI_DB_PORTS, but here you offset it
past that? Why?

> +	acpi_early_enabled = 1;
> +
> +	pr_debug("early: DBGx LAUNCH - console %d.\n", port);
> +
> +	return 0;
> +}
> +
> +static inline bool acpi_early_console_enabled(u8 port)
> +{
> +	BUG_ON(port >= MAX_ACPI_DBG_PORTS);
> +	return test_bit(port, acpi_early_flags);
> +}
> +
> +int __init acpi_early_console_keep(struct acpi_debug_port *info)

Why not make it 'bool' like the other (acpi_early_console_enabled)?
> +{
> +	BUG_ON(!info || info->port_index >= MAX_ACPI_DBG_PORTS);
> +	return test_bit(info->port_index+MAX_ACPI_DBG_PORTS, acpi_early_flags);
> +}
> +
> +static int __init acpi_early_console_start(struct acpi_debug_port *info)
> +{
> +	if (!acpi_early_console_enabled(info->port_index))
> +		return 0;

Not -ENODEV?
> +
> +	pr_debug("early: DBGx START - console %d(%04x:%04x).\n",
> +		 info->port_index, info->port_type, info->port_subtype);
> +	__acpi_early_console_start(info);
> +
> +	return 0;
> +}
> +
> +static int __init acpi_parse_dbg2(struct acpi_table_header *table)
> +{
> +	struct acpi_table_dbg2 *dbg2;
> +	struct acpi_dbg2_device *entry;
> +	unsigned int count = 0;
> +	unsigned long tbl_end;
> +	unsigned int max_entries;
> +	struct acpi_debug_port devinfo;
> +
> +	dbg2 = (struct acpi_table_dbg2 *)table;
> +	if (!dbg2) {
> +		pr_debug("early: DBG2 not present.\n");
> +		return -ENODEV;
> +	}
> +
> +	tbl_end = (unsigned long)table + table->length;
> +
> +	entry = (struct acpi_dbg2_device *)
> +		((unsigned long)dbg2 + dbg2->info_offset);
> +	max_entries = min_t(u32, MAX_ACPI_DBG_PORTS, dbg2->info_count);
> +
> +	while (((unsigned long)entry) + sizeof(struct acpi_dbg2_device) <
> +	       tbl_end) {

Just make it one line. Ignore the 80 characters limit here.

> +		if (entry->revision != 0) {
> +			pr_debug("early: DBG2 revision %d not supported.\n",
> +				 entry->revision);
> +			return -ENODEV;
> +		}
> +		if (!max_entries || count++ < max_entries) {

How about you just make this 'count'
> +			pr_debug("early: DBG2 PROBE - console %d(%04x:%04x).\n",
> +				 count-1,
> +				 entry->port_type, entry->port_subtype);
> +
> +			devinfo.port_index = (u8)count-1;

Then you don't this 'count -1'
> +			devinfo.port_type = entry->port_type;
> +			devinfo.port_subtype = entry->port_subtype;
> +			devinfo.register_count = entry->register_count;
> +			devinfo.registers = (struct acpi_generic_address *)
> +			    ((unsigned long)entry + entry->base_address_offset);
> +			devinfo.namepath_length = entry->namepath_length;
> +			devinfo.namepath = (char *)
> +			    ((unsigned long)entry + entry->namepath_offset);
> +			devinfo.oem_data_length = entry->oem_data_length;
> +			devinfo.oem_data = (u8 *)
> +			    ((unsigned long)entry + entry->oem_data_offset);
> +
> +			acpi_early_console_start(&devinfo);

no check of the return value to see whether you should return
immediately?
> +		}
and then do
		count++ here?

> +
> +		entry = (struct acpi_dbg2_device *)
> +		    ((unsigned long)entry + entry->length);
> +	}
> +
> +	return 0;
> +}
> +
> +static int __init acpi_parse_dbgp(struct acpi_table_header *table)
> +{
> +	struct acpi_table_dbgp *dbgp;
> +	struct acpi_debug_port devinfo;
> +
> +	dbgp = (struct acpi_table_dbgp *)table;
> +	if (!dbgp) {
> +		pr_debug("early: DBGP not present.\n");
> +		return -ENODEV;
> +	}
> +
> +	pr_debug("early: DBGP PROBE - console (%04x).\n", dbgp->type);
> +
> +	devinfo.port_index = 0;
> +	devinfo.port_type = ACPI_DBG2_SERIAL_PORT;
> +	devinfo.port_subtype = dbgp->type;
> +	devinfo.register_count = 1;
> +	devinfo.registers = (struct acpi_generic_address *)&dbgp->debug_port;
> +	devinfo.namepath_length = 0;
> +	devinfo.namepath = NULL;
> +	devinfo.oem_data_length = 0;
> +	devinfo.oem_data = NULL;
> +
> +	acpi_early_console_start(&devinfo);

how about 'return acpi_early_console_start(..)'
> +
> +	return 0;
> +}
> +
> +int __init acpi_early_console_probe(void)
> +{
> +	if (!acpi_early_enabled)
> +		return -EINVAL;
> +
> +	if (acpi_table_parse(ACPI_SIG_DBG2, acpi_parse_dbg2) != 0)
> +		acpi_table_parse(ACPI_SIG_DBGP, acpi_parse_dbgp);
> +
> +	return 0;
> +}
> +
> +int __init acpi_early_console_launch(char *s, int keep)
> +{
> +	char *e;
> +	unsigned long port = 0;
> +
> +	if (*s)
> +		port = simple_strtoul(s, &e, 10);
> +
> +	return acpi_early_console_enable(port, keep);
> +}
> +
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 4f2a762..641366c 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -430,4 +430,26 @@ acpi_status acpi_os_prepare_sleep(u8 sleep_state,
>  #define acpi_os_set_prepare_sleep(func, pm1a_ctrl, pm1b_ctrl) do { } while (0)
>  #endif
>  
> +#ifdef CONFIG_EARLY_PRINTK_ACPI
> +struct acpi_debug_port {
> +	u8 port_index;
> +	u16 port_type;
> +	u16 port_subtype;
> +	u16 register_count;
> +	struct acpi_generic_address *registers;
> +	u16 namepath_length;
> +	char *namepath;
> +	u16 oem_data_length;
> +	u8 *oem_data;
> +};
> +
> +int __init acpi_early_console_keep(struct acpi_debug_port *info);
> +int __init acpi_early_console_launch(char *s, int keep);
> +int __init acpi_early_console_probe(void);
> +/* This interface is arch specific. */
> +int __init __acpi_early_console_start(struct acpi_debug_port *info);
> +#else
> +static int acpi_early_console_probe(void) { return 0; }
> +#endif
> +
>  #endif	/*_LINUX_ACPI_H*/
> -- 
> 1.7.10
> 
> --
> 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
>