| Message ID | 1448619228-28839-1-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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. 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. 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(+)