serial: core: fix console problems on uart_close
PATCH 1/1 serial: core: fix console problems on uart_close
Commit 761ed4a94582 ('tty: serial_core: convert uart_close to use tty_port_close') started setting the ttyport console flag for serial drivers. This is causing crashes, hangs, or garbage output on several platforms because the serial shutdown is skipped and IRQs are left enabled.
Partially revert commit 761ed4a94582 and drop reporting UART tty_ports as a console leaving the console handling to the serial_core as it was before.
---
Geert's fix seems to be inadequate for some systems as Mike reported[1]. This fix works in Mike's case.
Geert, Mugunthan, Can you all test this is enough to fix your issues.
Rob
[1] https://www.mail-archive.com/[email protected]/msg1254753.html
drivers/tty/serial/serial_core.c | 2 -- 1 file changed, 2 deletions(-)
drivers/tty/serial/serial_core.c +0 -2
@@ -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.
Signature · 1 line
2.10.1
Hi Rob,
Quoted 5 lines · Rob Herring
Commit
761ed4a94582('tty:serial_core: convertuart_closeto usetty_port_close') started setting the ttyport console flag for serial drivers. This is causing crashes, hangs, or garbage output on several platforms because the serial shutdown is skipped and IRQs are left enabled.
No, for me it hangs because uart_tty_port_shutdown() is _not_ skipped when the UART is used as a serial console. Writing to the serial port while it's shut down hangs.
Quoted 15 lines
Partially revert commit
761ed4a94582and drop reporting UARTtty_portsas a console leaving the console handling to theserial_coreas it was before.Fixes 761ed4a94582ab29 ("tty: serial_core: convert uart_close to use tty_port_close")Reported-by Niklas Söderlund <[email protected]>Reported-by Mike Galbraith <[email protected]>Reported-by Mugunthan V N <[email protected]>Cc Peter Hurley <[email protected]>Cc Geert Uytterhoeven <[email protected]>Cc Alan Cox <[email protected]>Cc Greg Kroah-Hartman <[email protected]>Cc Jiri Slaby <[email protected]>Signed-off-by Rob Herring <[email protected]>
Quoted 5 lines
---
Geert's fix seems to be inadequate for some systems as Mike reported[1]. This fix works in Mike's case.Geert, Mugunthan, Can you all test this is enough to fix your issues.
This doesn't work for me, as port->console is still false. It should be true when used as a serial console, to make sure tty_port_shutdown() skips an active serial console.
Mike: I see you are using a PC, while I'm using an ARM board (with DT). Are you using a serial console? If yes, what's the value of port->console before and after the call to uart_console() that Rob's patch below removes?
Thanks!
Quoted 10 lines
[1] https://www.mail-archive.com/[email protected]/msg1254753.html
drivers/tty/serial/serial_core.c | 2 -- 1 file changed, 2 deletions(-)drivers/tty/serial/serial_core.c +0
-0
drivers/tty/serial/serial_core.c int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport) · line 2751
Quoted 8 lines
drivers/tty/serial/serial_core.c +0
-2@@ -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.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
Quoted 3 lines · Geert Uytterhoeven
Mike: I see you are using a PC, while I'm using an ARM board (with DT). Are you using a serial console? If yes, what's the value of
port->consolebefore and after the call touart_console()that Rob's patch below removes?
Well, it's a bit larger than the average PC, it's a 64 core HP DL980, and my console is its virtual serial port.
[ 15.217487] Serial: 8250/16550 driver, 32 ports, IRQ sharing disabled [ 15.228212] MIKE port:ffff880276400000 port->console:0 [ 15.240889] MIKE port:ffff880276400000 port->console:0 [ 15.269606] 00:03: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A [ 15.282538] MIKE port:ffff8802764001a8 port->console:0 [ 15.293195] MIKE port:ffff8802764001a8 port->console:1 [ 15.322806] serial8250: ttyS1 at I/O 0x2f8 (irq = 3, base_baud = 115200) is a 16550A
(30 more calls, all 0 pre 0 post)
That's with the below from Jiri Slaby applied to quell the reboot time explosion.
--- drivers/tty/serial/serial_core.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/tty/serial/serial_core.c +6 -1
@@ -106,12 +106,17 @@ static inline struct uart_port *uart_porvoid uart_write_wakeup (struct uart_port *port) {struct uart_state *state = port->state; +struct tty_struct *ttyp; +/* * This means you called this function _after_ the port was * closed. No cookie for you. */ BUG_ON (!state); -tty_wakeup (state->port.tty); + ttyp =tty_port_tty_get (&state->port); +if (ttyp) +tty_wakeup (ttyp); +tty_kref_put (ttyp); }static void uart_stop (struct tty_struct *tty)
Quoted 16 lines · Mike Galbraith
Quoted 3 lines · Geert Uytterhoeven
Mike: I see you are using a PC, while I'm using an ARM board (with DT). Are you using a serial console? If yes, what's the value of
port->consolebefore and after the call touart_console()that Rob's patch below removes?Well, it's a bit larger than the average PC, it's a 64 core HP DL980, and my console is its virtual serial port.
[ 15.217487] Serial: 8250/16550 driver, 32 ports, IRQ sharing disabled [ 15.228212] MIKE port:ffff880276400000 port->console:0 [ 15.240889] MIKE port:ffff880276400000 port->console:0 [ 15.269606] 00:03: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A [ 15.282538] MIKE port:ffff8802764001a8 port->console:0 [ 15.293195] MIKE port:ffff8802764001a8 port->console:1 [ 15.322806] serial8250: ttyS1 at I/O 0x2f8 (irq = 3, base_baud = 115200) is a 16550A
Quoted 4 lines
(30 more calls, all 0 pre 0 post)
That's with the below from Jiri Slaby applied to quell the reboot time explosion.
This alone is enough to fix things for you?
Rob
Quoted 20 lines · Rob Herring
Quoted 18 lines · Mike Galbraith
Quoted 3 lines · Geert Uytterhoeven
Mike: I see you are using a PC, while I'm using an ARM board (with DT). Are you using a serial console? If yes, what's the value of
port->consolebefore and after the call touart_console()that Rob's patch below removes?Well, it's a bit larger than the average PC, it's a 64 core HP DL980, and my console is its virtual serial port.
[ 15.217487] Serial: 8250/16550 driver, 32 ports, IRQ sharing disabled [ 15.228212] MIKE port:ffff880276400000 port->console:0 [ 15.240889] MIKE port:ffff880276400000 port->console:0 [ 15.269606] 00:03: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A [ 15.282538] MIKE port:ffff8802764001a8 port->console:0 [ 15.293195] MIKE port:ffff8802764001a8 port->console:1 [ 15.322806] serial8250: ttyS1 at I/O 0x2f8 (irq = 3, base_baud = 115200) is a 16550A(30 more calls, all 0 pre 0 post)
Quoted 4 lines
Quoted 2 lines
That's with the below from Jiri Slaby applied to quell the reboot time explosion.
This alone is enough to fix things for you?
Yup.
-Mike