Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751967AbbK1CPu (ORCPT <rfc822;w@1wt.eu>);
	Fri, 27 Nov 2015 21:15:50 -0500
Received: from mail-io0-f173.google.com ([209.85.223.173]:34688 "EHLO
	mail-io0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751444AbbK1CPs (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 27 Nov 2015 21:15:48 -0500
Subject: Re: [PATCH] serial: earlycon: Add missing spinlock initialization
To: Geert Uytterhoeven <geert+renesas@glider.be>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jslaby@suse.com>, Rob Herring <robh@kernel.org>
References: <1448619228-28839-1-git-send-email-geert+renesas@glider.be>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>,
        Ulrich Hecht <ulrich.hecht+renesas@gmail.com>,
        linux-serial@vger.kernel.org, linux-sh@vger.kernel.org,
        linux-kernel@vger.kernel.org
From: Peter Hurley <peter@hurleysoftware.com>
Message-ID: <56590E51.5010704@hurleysoftware.com>
Date: Fri, 27 Nov 2015 21:15:45 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.3.0
MIME-Version: 1.0
In-Reply-To: <1448619228-28839-1-git-send-email-geert+renesas@glider.be>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 4214
Lines: 88

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;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/