| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <56D987BC.7070209@linaro.org>
Date: Fri, 4 Mar 2016 16:03:56 +0300
From: Aleksey Makarov <aleksey.makarov@...aro.org>
To: Peter Hurley <peter@...leysoftware.com>, linux-acpi@...r.kernel.org
Cc: linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
Russell King <linux@....linux.org.uk>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J . Wysocki" <rjw@...ysocki.net>,
Leif Lindholm <leif.lindholm@...aro.org>,
Graeme Gregory <graeme.gregory@...aro.org>,
Al Stone <ahs3@...hat.com>,
Christopher Covington <cov@...eaurora.org>,
Matthias Brugger <mbrugger@...e.com>,
Jonathan Corbet <corbet@....net>, Jiri Slaby <jslaby@...e.com>,
linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 5/7] ACPI: serial: implement earlycon on ACPI DBG2 port
On 03/03/2016 08:48 PM, Peter Hurley wrote:
> On 03/01/2016 08:57 AM, Aleksey Makarov wrote:
>>
>>
>> On 03/01/2016 06:53 PM, Peter Hurley wrote:
>>> On 02/29/2016 04:42 AM, Aleksey Makarov wrote:
>>>> Add ACPI_DBG2_EARLYCON_DECLARE() macros that declares
>>>> an earlycon on the serial port specified in the DBG2 ACPI table.
>>>>
>>>> Pass the string "earlycon=acpi_dbg2" to the kernel to activate it.
>>>>
>>>> Callbacks for EARLYCON_DECLARE() and OF_EARLYCON_DECLARE()
>>>> can also be used for this macros.
>>>>
>>>> Signed-off-by: Aleksey Makarov <aleksey.makarov@...aro.org>
>>>> ---
>>>> Documentation/kernel-parameters.txt | 3 ++
>>>> drivers/tty/serial/earlycon.c | 60 +++++++++++++++++++++++++++++++++++++
>>>> include/linux/acpi_dbg2.h | 20 +++++++++++++
>>>> 3 files changed, 83 insertions(+)
>>>>
>>>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>>>> index e0a21e4..19b947b 100644
>>>> --- a/Documentation/kernel-parameters.txt
>>>> +++ b/Documentation/kernel-parameters.txt
>>>> @@ -1072,6 +1072,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>>> A valid base address must be provided, and the serial
>>>> port must already be setup and configured.
>>>>
>>>> + acpi_dbg2
>>>> + Use serial port specified by the DBG2 ACPI table.
>>>> +
>>>> earlyprintk= [X86,SH,BLACKFIN,ARM,M68k]
>>>> earlyprintk=vga
>>>> earlyprintk=efi
>>>> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
>>>> index d217366..9ba3a04 100644
>>>> --- a/drivers/tty/serial/earlycon.c
>>>> +++ b/drivers/tty/serial/earlycon.c
>>>> @@ -22,6 +22,7 @@
>>>> #include <linux/sizes.h>
>>>> #include <linux/of.h>
>>>> #include <linux/of_fdt.h>
>>>> +#include <linux/acpi.h>
>>>>
>>>> #ifdef CONFIG_FIX_EARLYCON_MEM
>>>> #include <asm/fixmap.h>
>>>> @@ -200,6 +201,8 @@ int __init setup_earlycon(char *buf)
>>>> return -ENOENT;
>>>> }
>>>>
>>>> +static bool setup_dbg2_earlycon;
>>>> +
>>>> /* early_param wrapper for setup_earlycon() */
>>>> static int __init param_setup_earlycon(char *buf)
>>>> {
>>>> @@ -212,6 +215,11 @@ static int __init param_setup_earlycon(char *buf)
>>>> if (!buf || !buf[0])
>>>> return early_init_dt_scan_chosen_serial();
>>>>
>>>> + if (!strcmp(buf, "acpi_dbg2")) {
>>>> + setup_dbg2_earlycon = true;
>>>> + return 0;
>>>> + }
>>>
>>> So this series doesn't start an ACPI earlycon at early_param time?
>>> That doesn't seem very useful.
>>>
>>> When does the ACPI earlycon actually start?
>>> And don't say "when the DBG2 table is probed"; that much is obvious.
>>
>> ACPI earlycon starts as soon as ACPI tables become accessible (setup_arch()).
>> I think that is still quite early.
>
> I see now; the probe is in patch 6/7.
>
> setup_arch()
> acpi_boot_table_init()
> acpi_probe_device_table()
> ...
> acpi_dbg2_setup()
> ->setup()
> acpi_setup_earlycon()
>
>
>>>> +
>>>> err = setup_earlycon(buf);
>>>> if (err == -ENOENT || err == -EALREADY)
>>>> return 0;
>>>> @@ -286,3 +294,55 @@ int __init of_setup_earlycon(const struct earlycon_id *match,
>>>> }
>>>>
>>>> #endif /* CONFIG_OF_EARLY_FLATTREE */
>>>> +
>>>> +#ifdef CONFIG_ACPI_DBG2_TABLE
>>>> +
>>>> +int __init acpi_setup_earlycon(struct acpi_dbg2_device *device, void *d)
>>>> +{
>>>> + int err;
>>>> + struct uart_port *port = &early_console_dev.port;
>>>> + int (*setup)(struct earlycon_device *, const char *) = d;
>>>> + struct acpi_generic_address *reg;
>>>> +
>>>> + if (!setup_dbg2_earlycon)
>>>> + return -ENODEV;
>>>> +
>>>> + if (device->register_count < 1)
>>>> + return -ENODEV;
>>>> +
>>>> + if (device->base_address_offset >= device->length)
>>>> + return -EINVAL;
>>>> +
>>>> + reg = (void *)device + device->base_address_offset;
>>>> +
>>>> + if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
>>>> + reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO)
>>>> + return -EINVAL;
>>>> +
>>>> + spin_lock_init(&port->lock);
>>>> + port->uartclk = BASE_BAUD * 16;
>>>> +
>>>> + if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
>>>> + if (device->port_type == ACPI_DBG2_ARM_SBSA_32BIT)
>>>> + port->iotype = UPIO_MEM32;
>>>> + else
>>>> + port->iotype = UPIO_MEM;
>>>> + port->mapbase = reg->address;
>>>> + port->membase = earlycon_map(reg->address, SZ_4K);
>>>> + } else {
>>>> + port->iotype = UPIO_PORT;
>>>> + port->iobase = reg->address;
>>>> + }
>>>> +
>>>> + early_console_dev.con->data = &early_console_dev;
>>>> + err = setup(&early_console_dev, NULL);
>>>> + if (err < 0)
>>>> + return err;
>>>> + if (!early_console_dev.con->write)
>>>> + return -ENODEV;
>>>> +
>>>> + register_console(early_console_dev.con);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +#endif /* CONFIG_ACPI_DBG2_TABLE */
>>>> diff --git a/include/linux/acpi_dbg2.h b/include/linux/acpi_dbg2.h
>>>> index 125ae7e..b653752 100644
>>>> --- a/include/linux/acpi_dbg2.h
>>>> +++ b/include/linux/acpi_dbg2.h
>>>> @@ -37,12 +37,32 @@ int acpi_dbg2_setup(struct acpi_table_header *header, const void *data);
>>>> ACPI_DECLARE_PROBE_ENTRY(dbg2, name, ACPI_SIG_DBG2, \
>>>> acpi_dbg2_setup, &__acpi_dbg2_data_##name)
>>>>
>>>> +int acpi_setup_earlycon(struct acpi_dbg2_device *device, void *d);
>>>> +
>>>> +/**
>>>> + * ACPI_DBG2_EARLYCON_DECLARE() - Define handler for ACPI GDB2 serial port
>>>> + * @name: Identifier to compose name of table data
>>>> + * @subtype: Subtype of the port
>>>> + * @console_setup: Function to be called to setup the port
>>>> + *
>>>> + * Type of the console_setup() callback is
>>>> + * int (*setup)(struct earlycon_device *, const char *)
>>>> + * It's the type of callback of of_setup_earlycon().
>>>> + */
>>>> +#define ACPI_DBG2_EARLYCON_DECLARE(name, subtype, console_setup) \
>>>> + ACPI_DBG2_DECLARE(name, ACPI_DBG2_SERIAL_PORT, subtype, \
>>>> + acpi_setup_earlycon, console_setup)
>>>> +
>>>> #else
>>>>
>>>> #define ACPI_DBG2_DECLARE(name, type, subtype, setup_fn, data_ptr) \
>>>> static const void *__acpi_dbg_data_##name[] \
>>>> __used __initdata = { (void *)setup_fn, (void *)data_ptr }
>>>>
>>>> +#define ACPI_DBG2_EARLYCON_DECLARE(name, subtype, console_setup) \
>>>> + static const void *__acpi_dbg_data_serial_##name[] \
>>>> + __used __initdata = { (void *)console_setup }
>
> console_setup is a terrible macro argument name; console_setup() is an
> actual kernel function (although file-scope).
> Please change it to something short and generic.
Is 'setup_fn' ok?
> Honestly, I'd just prefer you skip all this apparatus that makes
> ACPI earlycon appear to be like OF earlycon code-wise, but without any of
> the real underpinning or flexibility.
Actually it was Mark Salter who asked to introduce such macros.
https://lkml.kernel.org/g/1441730339.5459.8.camel@redhat.com
I think reusing the OF functions is a good decision.
Your "but without any of the real underpinning or flexibility" is unfounded.
> This would be trivial to parse the ACPI table and invoke
> setup_earlycon() with a string specifier instead.
>
> For example,
>
> int __init acpi_earlycon_setup(struct acpi_dbg2_device *dbg2)
> {
> char opts[64];
> struct acpi_generic_addr *addr = (void*)dbg2 + dbg2->base_address_offset;
> int mmio = addr->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY;
>
> if (dbg2->port_type != ACPI_DBG2_SERIAL_PORT)
> return 0;
>
> switch (dbg2->port_subtype) {
> case ACPI_DBG2_ARM_PL011:
> case ACPI_DBG2_ARM_SBSA_GENERIC:
> case ACPI_DBG2_BCM2835:
> sprintf(opts, "pl011,%s,0x%llx", mmio, addr->address);
> break;
> case ACPI_DBG2_ARM_SBSA_32BIT:
> sprintf(opts, "pl011,mmio32,0x%llx", addr->address);
> break;
> case ACPI_DBG2_16550_COMPATIBLE:
> case ACPI_DBG2_16550_SUBSET:
> sprintf(opts, "uart,%s,0x%llx", mmio, addr->address);
> break;
> default:
> return 0;
> }
>
> return setup_earlycon(opts);
> }
- Note that this decision forces setting earlycon on GDB2 debug port.
DBG2 does not specify that it should be exactly earlycon.
- You missed ACPI_DBG2_ARM_DCC. And actually I think the list of
debug ports is open. You will have to make up names like "uart" "pl011"
each time a new port is introduced into the specs.
- Most important thing, this way you disclose the internals of serial ports
to the generic earlycon.c Such info as access mode should stay
in the respective drivers.
- I would not like printing address and then parsing it back.
> This supports every earlycon ACPI DBG2 declares, not just the ARM_PL011
> subtype of your series.
To support earlycon on other types of debug port just add literally one
string of code (as in pl011).
>
>
>
>>>> +
>>>> #endif
>>>>
>>>> #endif
>>>>
>>>
>
Powered by blists - more mailing lists