| Message ID | 1475163064-20399-1-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On Thu, Sep 29, 2016 at 10:31 AM, Geert Uytterhoeven <geert+renesas@glider.be> wrote: > The port->console flag is always false, as uart_console() is called > before the serial console has been registered. I'm not seeing how that is. Everything uart_console() depends on (port->cons, port->cons->index, and port->line) should be set already. Maybe you pass in -1 for index which gets changed to 0 is the only thing I can see. Is doing that valid if you have multiple ports? > Hence for a serial port used as the console, uart_tty_port_shutdown() > will still be called when userspace closes the port, powering it down. > This will lead to a system lock up when the serial console driver writes > to the serial port's registers. > > To fix this, move the setting of port->console after the call to > uart_configure_port(), which registers the serial console. > > Fixes: 761ed4a94582ab29 ("tty: serial_core: convert uart_close to use tty_port_close") > Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > Reported-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > RFC because of the comment "If this port is a console, then the spinlock > is already initialised", and the pre-existing code calling > uart_console() before uart_configure_port(). So the spinlock is initialized twice which is probably harmless? If the spinlock was initialized by the console, then the index would probably not be -1. Rob
Hi Rob, On Mon, Oct 3, 2016 at 8:25 PM, Rob Herring <robh@kernel.org> wrote: > On Thu, Sep 29, 2016 at 10:31 AM, Geert Uytterhoeven > <geert+renesas@glider.be> wrote: >> The port->console flag is always false, as uart_console() is called >> before the serial console has been registered. > > I'm not seeing how that is. Everything uart_console() depends on > (port->cons, port->cons->index, and port->line) should be set already. > Maybe you pass in -1 for index which gets changed to 0 is the only > thing I can see. Is doing that valid if you have multiple ports? Isn't .index always initialized to -1, and set to the actual line number when the console is registered? That's how I remember it from when register_console() was introduced (in 2.1)... >> Hence for a serial port used as the console, uart_tty_port_shutdown() >> will still be called when userspace closes the port, powering it down. >> This will lead to a system lock up when the serial console driver writes >> to the serial port's registers. >> >> To fix this, move the setting of port->console after the call to >> uart_configure_port(), which registers the serial console. >> >> Fixes: 761ed4a94582ab29 ("tty: serial_core: convert uart_close to use tty_port_close") >> Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >> Reported-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> --- >> RFC because of the comment "If this port is a console, then the spinlock >> is already initialised", and the pre-existing code calling >> uart_console() before uart_configure_port(). > > So the spinlock is initialized twice which is probably harmless? If > the spinlock was initialized by the console, then the index would > probably not be -1. Shouldn't a double initialization cause a warning, with DEBUG_SPINLOCK=y (which I have enabled)? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, Oct 3, 2016 at 2:42 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Rob, > > On Mon, Oct 3, 2016 at 8:25 PM, Rob Herring <robh@kernel.org> wrote: >> On Thu, Sep 29, 2016 at 10:31 AM, Geert Uytterhoeven >> <geert+renesas@glider.be> wrote: >>> The port->console flag is always false, as uart_console() is called >>> before the serial console has been registered. >> >> I'm not seeing how that is. Everything uart_console() depends on >> (port->cons, port->cons->index, and port->line) should be set already. >> Maybe you pass in -1 for index which gets changed to 0 is the only >> thing I can see. Is doing that valid if you have multiple ports? > > Isn't .index always initialized to -1, and set to the actual line number when > the console is registered? That's how I remember it from when > register_console() was introduced (in 2.1)... Okay, the index is also set and gets marked enabled when specified on the command line (or DT). >>> Hence for a serial port used as the console, uart_tty_port_shutdown() >>> will still be called when userspace closes the port, powering it down. >>> This will lead to a system lock up when the serial console driver writes >>> to the serial port's registers. >>> >>> To fix this, move the setting of port->console after the call to >>> uart_configure_port(), which registers the serial console. >>> >>> Fixes: 761ed4a94582ab29 ("tty: serial_core: convert uart_close to use tty_port_close") >>> Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >>> Reported-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> >>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> --- >>> RFC because of the comment "If this port is a console, then the spinlock >>> is already initialised", and the pre-existing code calling >>> uart_console() before uart_configure_port(). >> >> So the spinlock is initialized twice which is probably harmless? If >> the spinlock was initialized by the console, then the index would >> probably not be -1. > > Shouldn't a double initialization cause a warning, with DEBUG_SPINLOCK=y > (which I have enabled)? I though only missing init caused warnings. Anyway, we can't really re-order things as uart_configure_port uses the spinlock. This has existed this way for some time and is all an independent of my change and your fix. So for your fix: Acked-by: Rob Herring <robh@kernel.org> Rob
Hi Rob, On Mon, Oct 3, 2016 at 10:30 PM, Rob Herring <robh@kernel.org> wrote: > On Mon, Oct 3, 2016 at 2:42 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> On Mon, Oct 3, 2016 at 8:25 PM, Rob Herring <robh@kernel.org> wrote: >>> On Thu, Sep 29, 2016 at 10:31 AM, Geert Uytterhoeven >>> <geert+renesas@glider.be> wrote: >>>> The port->console flag is always false, as uart_console() is called >>>> before the serial console has been registered. >>> >>> I'm not seeing how that is. Everything uart_console() depends on >>> (port->cons, port->cons->index, and port->line) should be set already. >>> Maybe you pass in -1 for index which gets changed to 0 is the only >>> thing I can see. Is doing that valid if you have multiple ports? >> >> Isn't .index always initialized to -1, and set to the actual line number when >> the console is registered? That's how I remember it from when >> register_console() was introduced (in 2.1)... > > Okay, the index is also set and gets marked enabled when specified on > the command line (or DT). I'm using DT and stdout-path. Where is the index set? >>>> Hence for a serial port used as the console, uart_tty_port_shutdown() >>>> will still be called when userspace closes the port, powering it down. >>>> This will lead to a system lock up when the serial console driver writes >>>> to the serial port's registers. >>>> >>>> To fix this, move the setting of port->console after the call to >>>> uart_configure_port(), which registers the serial console. >>>> >>>> Fixes: 761ed4a94582ab29 ("tty: serial_core: convert uart_close to use tty_port_close") >>>> Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >>>> Reported-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> >>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>>> --- >>>> RFC because of the comment "If this port is a console, then the spinlock >>>> is already initialised", and the pre-existing code calling >>>> uart_console() before uart_configure_port(). >>> >>> So the spinlock is initialized twice which is probably harmless? If >>> the spinlock was initialized by the console, then the index would >>> probably not be -1. >> >> Shouldn't a double initialization cause a warning, with DEBUG_SPINLOCK=y >> (which I have enabled)? > > I though only missing init caused warnings. > > Anyway, we can't really re-order things as uart_configure_port uses > the spinlock. This has existed this way for some time and is all an > independent of my change and your fix. So for your fix: > > Acked-by: Rob Herring <robh@kernel.org> Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index 6e4f63627479db8d..ce8899c13af3d97f 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -2746,8 +2746,6 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport) uport->cons = drv->cons; uport->minor = drv->tty_driver->minor_start + uport->line; - port->console = uart_console(uport); - /* * If this port is a console, then the spinlock is already * initialised. @@ -2761,6 +2759,8 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport) uart_configure_port(drv, state, uport); + port->console = uart_console(uport); + num_groups = 2; if (uport->attr_group) num_groups++;
The port->console flag is always false, as uart_console() is called before the serial console has been registered. Hence for a serial port used as the console, uart_tty_port_shutdown() will still be called when userspace closes the port, powering it down. This will lead to a system lock up when the serial console driver writes to the serial port's registers. To fix this, move the setting of port->console after the call to uart_configure_port(), which registers the serial console. Fixes: 761ed4a94582ab29 ("tty: serial_core: convert uart_close to use tty_port_close") Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Reported-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- RFC because of the comment "If this port is a console, then the spinlock is already initialised", and the pre-existing code calling uart_console() before uart_configure_port(). Can be reproduced on Renesas boards: 1. With systemd: hangs during boot, 2. Without systemd: "telinit n" to switch to a runlevel that doesn't run a getty on ttySC0, and trigger kernel output. --- drivers/tty/serial/serial_core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)