Re: [PATCH] serial: earlycon: Add missing spinlock initialization

[Peter Hurley <peter@hurleysoftware.com>]

On 11/27/2015 05:13 AM, Geert Uytterhoeven wrote:
> If an earlycon console driver needs to acquire the uart_port.lock
> spinlock for serial console output, and CONFIG_DEBUG_SPINLOCK=y:
> 
>     BUG: spinlock bad magic on CPU#0, swapper/0
>      lock: sci_ports+0x0/0x3480, .magic: 00000000, .owner: <none>/-1, .owner_cpu: 0
>     CPU: 0 PID: 0 Comm: swapper Not tainted 4.4.0-rc2-koelsch-g62ea5edf143bb1d0-dirty #2083
>     Hardware name: Generic R8A7791 (Flattened Device Tree)
>     [<c00173a0>] (unwind_backtrace) from [<c0013094>] (show_stack+0x10/0x14)
>     [<c0013094>] (show_stack) from [<c01f2338>] (dump_stack+0x70/0x8c)
>     [<c01f2338>] (dump_stack) from [<c00702d8>] (do_raw_spin_lock+0x20/0x190)
>     [<c00702d8>] (do_raw_spin_lock) from [<c0267590>] (serial_console_write+0x4c/0x130)
>     [<c0267590>] (serial_console_write) from [<c00734c4>] (call_console_drivers.constprop.13+0xc8/0xec)
>     [<c00734c4>] (call_console_drivers.constprop.13) from [<c0074ef0>] (console_unlock+0x354/0x440)
>     [<c0074ef0>] (console_unlock) from [<c0075bb4>] (register_console+0x2a0/0x394)
>     [<c0075bb4>] (register_console) from [<c06cb750>] (of_setup_earlycon+0x90/0xa4)
>     [<c06cb750>] (of_setup_earlycon) from [<c06cfb60>] (setup_of_earlycon+0x118/0x13c)
>     [<c06cfb60>] (setup_of_earlycon) from [<c06b34ac>] (do_early_param+0x64/0xb4)
>     [<c06b34ac>] (do_early_param) from [<c00472c0>] (parse_args+0x254/0x350)
>     [<c00472c0>] (parse_args) from [<c06b3860>] (parse_early_options+0x2c/0x3c)
>     [<c06b3860>] (parse_early_options) from [<c06b389c>] (parse_early_param+0x2c/0x40)
>     [<c06b389c>] (parse_early_param) from [<c06b5b08>] (setup_arch+0x520/0xaf0)
>     [<c06b5b08>] (setup_arch) from [<c06b3948>] (start_kernel+0x94/0x370)
>     [<c06b3948>] (start_kernel) from [<40008090>] (0x40008090)
> 
> Initialize the spinlock in of_setup_earlycon() and register_earlycon(),
> to fix this for both DT-based and legacy earlycon.  If the driver would
> reinitialize the spinlock again, this is harmless, as it's allowed to
> reinitialize an unlocked spinlock.

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

> Alternatives are:
>   - Drivers having an early_serial_console_write() that only performs
>     the core functionality of serial_console_write(), without acquiring
>     the lock (which may be unsafe, depending on the hardware),
>   - Drivers initializing the spinlock in their private earlycon setup
>     functions.
> 
> As uart_port is owned by generic serial_core, and uart_port.lock is
> initialized by uart_add_one_port() for the normal case, this can better
> be handled in the earlycon core.

Just to be clear here: the uart_port used by the earlycon is unrelated
to the eventual uart_port instanced by the driver (which may provide
the console that replaces the earlycon).

That means the spinlocks won't prevent concurrent h/w programming between
the earlycon and the driver.

Regards,
Peter Hurley


> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Tested on arm32 (r8a7791/koelsch) and arm64 (r8a7795/salvator-x).
> 
>  drivers/tty/serial/earlycon.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> index f09636083426d5fc..b5b2f2be6be7c613 100644
> --- a/drivers/tty/serial/earlycon.c
> +++ b/drivers/tty/serial/earlycon.c
> @@ -115,6 +115,7 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
>  	if (buf && !parse_options(&early_console_dev, buf))
>  		buf = NULL;
>  
> +	spin_lock_init(&port->lock);
>  	port->uartclk = BASE_BAUD * 16;
>  	if (port->mapbase)
>  		port->membase = earlycon_map(port->mapbase, 64);
> @@ -202,6 +203,7 @@ int __init of_setup_earlycon(unsigned long addr,
>  	int err;
>  	struct uart_port *port = &early_console_dev.port;
>  
> +	spin_lock_init(&port->lock);
>  	port->iotype = UPIO_MEM;
>  	port->mapbase = addr;
>  	port->uartclk = BASE_BAUD * 16;
>